API, Core: Introduce a generic abstraction to replace ManifestListFile

[gaborkaszab]

ManifestListFile and its implementation contains nothing that is specific to manifest lists. It is more generally related to files that use TableMetadata.encryptionKeys to store encrypted encryption key metadata that are referred to by a key ID.

This PR introduces a more general interface that can be used accross multiple file types like manifest lists, V4 root manifests, table statistics and partition statistics. The less general functionality specific to manifest lists is deprecated or removed where possible.

[gaborkaszab]

I dropped BaseManifestListFile because that was package private. Now, there is no implementation in the library that can call this function, still I don't think we can drop this, because it'd break API for users that happen to implement their own ManifestListFile. Not likely, but technically feasible.

[gaborkaszab]

Thanks for taking a look, @stevenzwu !

[varun-lakhyani]

Overall LGTM.

• Data types / return types are consistently migrated to EncryptedFile.
• Deprecation javadocs are clear
• ManifestListFile's own deprecation javadoc + extends EncryptedFile explains most of the seemingly-confusing type/return changes across the PR (e.g. toManifestListFile() returning EncryptedFile).

[anoopj]

Shouldn't the removal version be the next major version (2.0.0) as you have done in the other deprecation tag in this PR?

[gaborkaszab]

In core/ we can remove stuff in the next minor release, while in api/ it's stricter, the next major is the closest one for removal.

[gaborkaszab]

Giving this a second thought I think you're right. As long as we keep the ManifestListFile interface (until 2.0.0 as it's in api/) we have to keep the encryption/decryption methods for that class. Otherwise it might be a behavior change for custom implementation if we silently fall back to the method for EncryptedFile.

[anoopj]

Carry over from existing design, but this method feels like it belongs on the encryption service rather than on the file value type. The file only supplies the key ID. I don't think it can simply be dropped, though: the caller (EncryptingFileIO) is in api and EncryptionUtil is in core, so api can't call it directly.

If we want it off EncryptedFile, the clean version is to move the operation onto EncryptionManager (impl already in core, and EncryptingFileIO already holds one), keyed by encryptionKeyID().

It's fine if we want to do in a followup.

cc @rdblue in case he has any thoughts.

[gaborkaszab]

Hmm, with this the only purpose of putting this functionality into EncryptionManager is that its implementation in core/ can delegate to EncryptionUtil to get around the issue that EncryptingFileIO.newInputFile() is in api/ and can't call the util method in core/. I'm not entirely comfortable with that design, while I'm not comfortable with the current one either. At least we keep existing api with the current one.

[stevenzwu]

I likes Anoop suggestion here of dropping this method and let callers use the EncryptionUtil.decryptXXX directly. Since we are defining a new interface, it seems like a good opportunity to deprecate. Then this class becomes just a POJO envelope interface.

[gaborkaszab]

I gave this a try and in the latest version of this PR you can see how this can be implemented. I dropped the decryptKeyMetadata method from the new interface. However, to be able to use the underlying EncryptionUtil.decryptKeyMetadata() that is in core/, from EncryptingFileIO that is in api/, I had to introduce the same in EncryptionManager. Basically, this is inline with @anoopj 's suggestion. Let me know what you think!

[gaborkaszab]

Thanks for looking into this @stevenzwu @huaxingao @anoopj !
I renamed the new interface, also went for the simpler method keyId() while keeping compatibility with custom ManifestListFile implementations.

[gaborkaszab]

Giving this a second thought I think you're right. As long as we keep the ManifestListFile interface (until 2.0.0 as it's in api/) we have to keep the encryption/decryption methods for that class. Otherwise it might be a behavior change for custom implementation if we silently fall back to the method for EncryptedFile.

[gaborkaszab]

Hmm, with this the only purpose of putting this functionality into EncryptionManager is that its implementation in core/ can delegate to EncryptionUtil to get around the issue that EncryptingFileIO.newInputFile() is in api/ and can't call the util method in core/. I'm not entirely comfortable with that design, while I'm not comfortable with the current one either. At least we keep existing api with the current one.

[gaborkaszab]

Thank you for the reviews @stevenzwu and @anoopj !

@rdblue Would you like to take a look yourself?

[huaxingao]

LGTM

[gaborkaszab]

Hi @stevenzwu @anoopj @huaxingao
Thanks for the approvals! In the meantime, I double-checked the code around this PR and I found some additional usage related to ManifestListFile, this time in EncryptionManager's encrypt path. I got rid of the urge for them and introduced more generic functions as I already did for the decrypt path. I eliminated the usage of BaseManifestListFile too and managed to get rid of the class.

Would you mind taking another look?

[RussellSpitzer]

@stevenzwu and @huaxingao Any reason we aren't merging this yet?

[RussellSpitzer]

Name here is a bit weird. "With Key Id" has a lot of weight here and is only really understandable if you know the context is encryption. This also wouldn't be a file as much as it would be just a path.

[RussellSpitzer]

I'm a little suspicious of adding a new public interface for this (in API too). The interface adds two new public fields and we never actually need it in this Pr. The only caller who uses it is "newInputFile" Which probably should just have another extension newInputFile(location, encryptionKeyId).

I'm not sure that this warrants a new type.

[gaborkaszab]

Thanks for taking a look, @RussellSpitzer !
This went through a number of iterations, and now that we moved the decryptKeyMetadata() method from the new, common interface to EncryptionManager and EncryptionUtil, not much is left there. Just took a quick look and I see it feasible to just simply get rid of the new interface and also remove usage of the existing ManifestListFile and pass the location and key ID wherever needed.
cc @stevenzwu for your PR that builds on this.

Let me give this a try and see where this break if it does. Will come back soon.

[gaborkaszab]

Hey @RussellSpitzer ,
You were right, I managed to get rid of the new interface by introducing versions of functions that accept location and keyId instead of these classes/interfaces. Thanks for the fresh pair of eyes!

@stevenzwu I think this PR is still something you can build the Snapshot.rootLocation abstraction on.

[gaborkaszab]

FYI, there was a conflict with a recent change: #17984
I made some follow-up cleanup for that: #18150
And made this PR dependent on that follow-up

[gaborkaszab]

FYI, there was a conflict with a recent change: #17984 I made some follow-up cleanup for that: #18150 And made this PR dependent on that follow-up

FYI, the above got merged. I rebased now to eliminate the conflict. This is way simpler now, no new interfaces. Please review @RussellSpitzer @stevenzwu @anoopj @huaxingao

[RussellSpitzer]

I probably would change this to require that keyId is nonNull.

I don't think we want folks calling this method if they don't expect to be using a key

[RussellSpitzer]

Precondition keyId.notNull

[gaborkaszab]

I think such a precondition would break the contract of EncryptionFileIO. If I'm not mistaken, EncryptingFileIO theoretically can be created even if the table is not encrypted, and other functions here also seem to branch on key_metadata being null or not, where null defaults to the non-encrypting case. I think for consistency we should follow that pattern with the keyId variation of newFileIO functions.
WDYT @RussellSpitzer ?

[RussellSpitzer]

I'm not sure I follow. In this PR we now provide 2 APIs

new inputFile(String location)

and

new InputFile(String location, String keyID)

---

Usually I would expect the first to call the second. But here we have the second call the first.

The question is what is the point of the first API if I have the second one and can pass String keyID) as null. My assumption would be that if I pass through a keyId I am attempting to have that used by the fileIO. If I have passed through "null" I'm probably not doing something right because I should have called "new inputFile(location)".

We also should consider the integration of this API with the "Length" passthrough

Let me jump down to default definition to add some more comments

[RussellSpitzer]

I think overall we are still stuck a bit too much mimicking the logic that was here before for ManifestLists without thinking about how we would add this API by first principals.

We are essentially adding a new API that a FileIO user can call if they have key metadata. We know they also will have a path and may have a length. So it probably makes sense to build this up similar to the other methods in FileIO.

Right now we have

  /** Get a {@link InputFile} instance to read bytes from the file at the given path. */
  InputFile newInputFile(String path);

  /**
   * Get a {@link InputFile} instance to read bytes from the file at the given path, with a known
   * file length.
   */
  default InputFile newInputFile(String path, long length) {
    return newInputFile(path);
  }

So we probably also need

 /**
   * Get a {@link InputFile} instance to read bytes from the file at the given path, with a known
   * file length.
   */
  default InputFile newInputFile(String path, long length, String keyMetadata) {
    throws UnsupportedOperationException(can't use without encrypting file io)
  }

Then the question would be if you are in EncryptingFileIO should the implementation fail if keyMetadata is null? Because the library user has expressly used a method which implies keyMetadata is important, so is it OK to just fall back to non-ecryption behavior? I don't feel comfortable with that but others may have a different opinion.

[gaborkaszab]

I think having a length param could make sense as you described above to be consistent with other function and to have a length available in the "fallback to unencrypted read" case.
My understanding might not be that strong here, but I think for manifests list files we are in a weird situation because we don't directly keep the length of the physical encrypted file. We do keep the location and keyId on the snapshot, and in turn we can get the keyMetadata based on the keyId. When it comes to reading we assume that the length is integrated into keyMetadata. So for manifest list files I don't think there is a straightforward way to use the proposed newInputFile(path, length, keyMetadata or keyId).

Just a general comment, that with the proposed new function, we could technically wipe out all the variations of newInputFile(DataFile), newInputFile(DeleteFile), newInputFile(ManifestFile) because all they need is a path, a length and keyMetadata. Unfortunately, we keep no length for manifest list files as described above.
Should we proposed keeping the length in Snapshot for the new V4 root manifest since we are actively working on it and then we can avoid relying on length being embedded into keyMetadata and could use length as a param for newInputFile? cc @amogh-jahagirdar @stevenzwu

On the question of should we fail if keyMetadata is null or fallback to the unencrypted path, I think the latter is cleaner on the caller side. See my example on the other comment.

[RussellSpitzer]

While this made sense for "manifest List" i'm not sure it makes sense for a generic path. The caller of this method has control over whether or not they are passing through a String. If they choose to pass through a string shouldn't we be using it?

Do we have other examples of API's where an argument can be ignored if it is null and fall back to another polymorphism?

[gaborkaszab]

On the general case what I have in mind is that we don't want the callers to branch on whether keyId is null or not. It's a cleaner implementation to call newInputFile(location, keyId) unconditionally for files that might be encrypted with the key-id based mechanism and then internally we can decide which path to go.
One such implementation would look like this:

StatisticsFile statFile = // get the stats file from table metadata
EncryptingFileIO io = EncryptingFileIO.combine(table.io(), table.encryption());
InputFile inputFile = io.newInputFile(statFile.path(), statFile.keyId());

This way the caller code is clean enough, no need to branch on keyId being null, we can fallback to the unencrypted case inside newInputFile.
This is what BaseSnapshot.cacheManifests() -> ManifestLists.newInputFile() -> io.newInputFile()` path does now.

About other such APIs, I'm not sure about other APIs, but the keyMetadata based ones seem to do the same here, and for me this seems to give a nice flexibility: even if we have an EncryptionFileIO there might be unencrypted files without keyMetadata or keyId and then we can silently fallback to the unencrypted case without making the caller to make this decision.

[gaborkaszab]

Thanks for the comments, @RussellSpitzer !

TLDR of my comments:
- newInputFile(path, length, keyMetadata or keyId) could make and we could use this in general replacing many of the existing variations of newInputFile functions. However, we keep no length directly for manifest list files, this might not work on that path.

• Shared an example where not throwing for null keyId in newInputFile would be desired from the user's side.

[gaborkaszab]

On the general case what I have in mind is that we don't want the callers to branch on whether keyId is null or not. It's a cleaner implementation to call newInputFile(location, keyId) unconditionally for files that might be encrypted with the key-id based mechanism and then internally we can decide which path to go.
One such implementation would look like this:

StatisticsFile statFile = // get the stats file from table metadata
EncryptingFileIO io = EncryptingFileIO.combine(table.io(), table.encryption());
InputFile inputFile = io.newInputFile(statFile.path(), statFile.keyId());

This way the caller code is clean enough, no need to branch on keyId being null, we can fallback to the unencrypted case inside newInputFile.
This is what BaseSnapshot.cacheManifests() -> ManifestLists.newInputFile() -> io.newInputFile()` path does now.

About other such APIs, I'm not sure about other APIs, but the keyMetadata based ones seem to do the same here, and for me this seems to give a nice flexibility: even if we have an EncryptionFileIO there might be unencrypted files without keyMetadata or keyId and then we can silently fallback to the unencrypted case without making the caller to make this decision.

[gaborkaszab]

I think having a length param could make sense as you described above to be consistent with other function and to have a length available in the "fallback to unencrypted read" case.
My understanding might not be that strong here, but I think for manifests list files we are in a weird situation because we don't directly keep the length of the physical encrypted file. We do keep the location and keyId on the snapshot, and in turn we can get the keyMetadata based on the keyId. When it comes to reading we assume that the length is integrated into keyMetadata. So for manifest list files I don't think there is a straightforward way to use the proposed newInputFile(path, length, keyMetadata or keyId).

Just a general comment, that with the proposed new function, we could technically wipe out all the variations of newInputFile(DataFile), newInputFile(DeleteFile), newInputFile(ManifestFile) because all they need is a path, a length and keyMetadata. Unfortunately, we keep no length for manifest list files as described above.
Should we proposed keeping the length in Snapshot for the new V4 root manifest since we are actively working on it and then we can avoid relying on length being embedded into keyMetadata and could use length as a param for newInputFile? cc @amogh-jahagirdar @stevenzwu

On the question of should we fail if keyMetadata is null or fallback to the unencrypted path, I think the latter is cleaner on the caller side. See my example on the other comment.