API: Extend FileIO and add EncryptingFileIO. #9592

rdblue · 2024-01-31T01:08:31Z

This adds EncryptingFileIO to handle metadata file encryption without breaking existing interfaces that pass FileIO without an EncryptionManager. It also updates FileIO to have methods for opening ManifestFile, DataFile, and DeleteFile directly to avoid needing to check whether a given FileIO supports encryption. The default implementation in FileIO fails if metadata files are encrypted.

rdblue · 2024-01-31T01:08:44Z

@ggershinsky, here is what I have so far. It would be great to get your input on it!

ggershinsky · 2024-02-09T06:47:32Z

@rdblue Thanks for the PR. I am about to complete applying it to the e2e manifest/list/table encryption. So far, everything works well. I'll send updated patches next week.

ggershinsky · 2024-02-09T06:52:53Z

api/src/main/java/org/apache/iceberg/encryption/EncryptingFileIO.java

+
+  public InputFile newDecryptingInputFile(String path, long length, ByteBuffer buffer) {
+    // TODO: is the length correct for the encrypted file? It may be the length of the plaintext
+    // stream


Yep, this is the plaintext stream length (which is recorded in the ContentFile and ManifestFile objects). I have updated the AesGcmInput classes to work with the verified plaintext length values (instead of untrusted file length values) - actually, the code becomes more compact and intuitive. I'll send this patch along with other e2e updates next week.

ggershinsky · 2024-02-12T08:28:12Z

Everything works with this PR. But there are two options for its integration with other PRs, lets discuss them in the call.

api/src/main/java/org/apache/iceberg/io/FileIO.java

rdblue · 2024-02-18T20:57:11Z

core/src/test/java/org/apache/iceberg/hadoop/TestCatalogUtilDropTable.java

@@ -201,6 +188,25 @@ public void shouldNotDropDataFilesIfGcNotEnabled() {
        .containsAll(metadataLocations);
  }

+  private static FileIO createMockFileIO(FileIO wrapped) {


These tests needed the mock object to support the added newInputFile methods. Rather than continuing to change each test individually, I refactored to have one method that is used throughout to create the mock FileIO.

rdblue · 2024-02-18T20:57:59Z

core/src/main/java/org/apache/iceberg/io/ContentCache.java

@@ -232,80 +237,63 @@ public long getLength() {
    @Override
    public SeekableInputStream newStream() {
      try {
-        // read from cache if file length is less than or equal to maximum length allowed to
-        // cache.
-        if (getLength() <= contentCache.maxContentLength()) {


This check duplicated the one in tryCache.

rdblue · 2024-02-18T21:03:04Z

core/src/main/java/org/apache/iceberg/io/ContentCache.java

 import org.apache.iceberg.relocated.com.google.common.collect.Lists;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;

 /**
 * Class that provides file-content caching during reading.
 *
- * <p>The file-content caching is initiated by calling {@link ContentCache#tryCache(FileIO, String,
- * long)}. Given a FileIO, a file location string, and file length that is within allowed limit,
+ * <p>The file-content caching is initiated by calling {@link ContentCache#tryCache(InputFile)}.


The changes here aren't strictly necessary, but clean up several problems I noticed while updating the code to call newInputFile(ManifestFile) instead of newInputFile(String,long). The class leaked the CacheEntry class in its public API (those methods are now deprecated) and had duplicate checks, overly verbose debug logs, and odd error handling. I've simplified it a bit and made the naming more clear for internal methods and classes.

rdblue · 2024-02-18T21:04:47Z

core/src/main/java/org/apache/iceberg/ManifestFiles.java

-          "ContentCache creation failed. Check that all manifest caching configurations has valid value.");
-      LOG.debug("FileIO-level cache stats: {}", CONTENT_CACHES.stats());
-      return cache.tryCache(io, path, length);
+  private static InputFile newInputFile(FileIO io, ManifestFile manifest) {


I simplified the logic here in addition to passing ManifestFile through to newInputFile.

rdblue · 2024-02-18T21:06:35Z

spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/source/BaseReader.java

+      this.lazyInputFiles =
+          EncryptingFileIO.create(table().io(), table().encryption())
+              .bulkDecrypt(
+                  () -> taskGroup.tasks().stream().flatMap(this::referencedFiles).iterator());


These changes demonstrate how EncryptingFileIO is cleaner for bulk decryption. This section has been broken by contributors several times in efforts to simplify the logic, but that end up breaking the bulk decrypt behavior.

RussellSpitzer · 2024-02-19T20:45:36Z

api/src/main/java/org/apache/iceberg/encryption/EncryptingFileIO.java

+public class EncryptingFileIO implements FileIO, Serializable {
+  public static EncryptingFileIO create(FileIO io, EncryptionManager em) {
+    if (io instanceof EncryptingFileIO) {
+      return (EncryptingFileIO) io;


Should we check if IO's Encryption Manager matches the one being passed? Just a little worried about users of this API potentially calling this method and having their EM ignored.

nit: Perhaps "wrap" instead of create?

I thought about this, but it felt like overkill. It would make little sense to double-wrap encryption and only the outer one would be used. There's also the consideration of where the FileIO and EncryptionManager come from and I think nearly all of the time it would be from the same table. The check looked like dead code to me.

I also considered other names, but this is creating a thing with new capabilities rather than wrapping one thing in another. I also considered "combine" but didn't think it was substantially better.

RussellSpitzer · 2024-02-19T20:58:10Z

api/src/main/java/org/apache/iceberg/io/FileIO.java

+  default InputFile newInputFile(DataFile file) {
+    Preconditions.checkArgument(
+        file.keyMetadata() == null,
+        "Cannot decrypt data file: {} (use EncryptingFileIO)",


nit: Suggestion on error

Cannot decrypt encrypted data file without configured EncryptingFileIO: {}

I think it's clear that the data file is encrypted, so I would omit that addition. And the "without configured EncryptingFileIO" is a stronger statement that isn't quite right. You can decrypt a data file using an EncryptionManager and newInputFile(String) but if you want to use this method, you must use EncryptingFileIO.

RussellSpitzer

Left a few nit comments but this makes sense to me

rdblue · 2024-02-19T22:42:46Z

I'm going to merge this to unblock @ggershinsky and we can continue discussing the create method changes in a follow up. Thanks, @RussellSpitzer!

- Testing with Iceberg 1.5.0 RC0 and Nessie 0.77.1 - Remove deprecated usage - InputFile.length is being called since apache/iceberg#9592 - Handle JDBC catalog test failures

- Testing with Iceberg 1.5.0 RC0 and Nessie 0.77.1 - Remove deprecated usage - InputFile.length is being called since apache/iceberg#9592 - Handle JDBC catalog test failures Co-authored-by: Eduard Tudenhoefner <[email protected]>

- Testing with Iceberg 1.5.0 RC4 and Nessie 0.77.1 - Remove deprecated usage - InputFile.length is being called since apache/iceberg#9592 - Handle JDBC catalog test failures Co-authored-by: Eduard Tudenhoefner <[email protected]>

- Bump Iceberg to 1.5.0 and Nessie to 0.77.1 - Remove deprecated usage - InputFile.length is being called since apache/iceberg#9592 Co-authored-by: Eduard Tudenhoefner <[email protected]>

API: Extend FileIO and add EncryptingFileIO.

b968740

github-actions bot added API spark core labels Jan 31, 2024

Use new FileIO mehtods in ManifestFiles, fix ContentCache.

a3432f4

ggershinsky reviewed Feb 9, 2024

View reviewed changes

ggershinsky reviewed Feb 12, 2024

View reviewed changes

api/src/main/java/org/apache/iceberg/io/FileIO.java Outdated Show resolved Hide resolved

api/src/main/java/org/apache/iceberg/io/FileIO.java Outdated Show resolved Hide resolved

api/src/main/java/org/apache/iceberg/io/FileIO.java Outdated Show resolved Hide resolved

rdblue added 4 commits February 18, 2024 12:23

Fix test failures and review comments.

1fbc85a

Add back public method as deprecated.

18f758e

Fix revapi for ContentCache.

491754a

Deprecate ContentCache methods that expose private classes.

888dffc

rdblue commented Feb 18, 2024

View reviewed changes

Apply spotless.

a3013c1

rdblue commented Feb 18, 2024

View reviewed changes

rdblue added 2 commits February 18, 2024 13:09

Return CacheEntry.

3d51914

Fix uses of getIfPresent.

0859f18

RussellSpitzer reviewed Feb 19, 2024

View reviewed changes

RussellSpitzer approved these changes Feb 19, 2024

View reviewed changes

rdblue merged commit 4c5208a into apache:main Feb 19, 2024
42 checks passed

nastra added a commit to ajantha-bhat/trino that referenced this pull request Feb 20, 2024

InputFile.length is being called since apache/iceberg#9592

0c7b54c

ajantha-bhat pushed a commit to ajantha-bhat/trino that referenced this pull request Feb 21, 2024

InputFile.length is being called since apache/iceberg#9592

2b32011

ajantha-bhat mentioned this pull request Feb 21, 2024

WIP: Test CI for upcoming Iceberg and Nessie version bump trinodb/trino#20795

Closed

ajantha-bhat mentioned this pull request Feb 27, 2024

WIP: Test Upcoming Iceberg and Nessie versions trinodb/trino#20846

Closed

ajantha-bhat mentioned this pull request Mar 3, 2024

WIP: Test Upcoming Iceberg and Nessie versions trinodb/trino#20911

Closed

bitsondatadev pushed a commit to bitsondatadev/iceberg that referenced this pull request Mar 3, 2024

API: Extend FileIO and add EncryptingFileIO. (apache#9592)

453054a

ajantha-bhat mentioned this pull request Mar 6, 2024

WIP: Test Upcoming Iceberg and Nessie versions trinodb/trino#20957

Closed

ajantha-bhat mentioned this pull request Mar 12, 2024

Bump Iceberg and Nessie versions trinodb/trino#21034

Merged

amogh-jahagirdar mentioned this pull request Mar 13, 2024

API: Fix default FileIO#newInputFile ManifestFile, DataFile and DeleteFile implementation to pass lengths #9953

Merged

hsiang-c mentioned this pull request Apr 22, 2024

Data: Adopt EncryptingFileIO in Java read API #10201

Closed

devangjhabakh pushed a commit to cdouglas/iceberg that referenced this pull request Apr 22, 2024

API: Extend FileIO and add EncryptingFileIO. (apache#9592)

652ece6

zachdisc pushed a commit to zachdisc/iceberg that referenced this pull request Dec 23, 2024

API: Extend FileIO and add EncryptingFileIO. (apache#9592)

d673408

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API: Extend FileIO and add EncryptingFileIO. #9592

API: Extend FileIO and add EncryptingFileIO. #9592

rdblue commented Jan 31, 2024

rdblue commented Jan 31, 2024

ggershinsky commented Feb 9, 2024

ggershinsky Feb 9, 2024 •

edited

Loading

ggershinsky commented Feb 12, 2024

rdblue Feb 18, 2024

rdblue Feb 18, 2024

rdblue Feb 18, 2024

rdblue Feb 18, 2024

rdblue Feb 18, 2024

RussellSpitzer Feb 19, 2024

rdblue Feb 19, 2024

RussellSpitzer Feb 19, 2024

rdblue Feb 19, 2024

RussellSpitzer left a comment

rdblue commented Feb 19, 2024

API: Extend FileIO and add EncryptingFileIO. #9592

API: Extend FileIO and add EncryptingFileIO. #9592

Conversation

rdblue commented Jan 31, 2024

rdblue commented Jan 31, 2024

ggershinsky commented Feb 9, 2024

ggershinsky Feb 9, 2024 • edited Loading

Choose a reason for hiding this comment

ggershinsky commented Feb 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RussellSpitzer left a comment

Choose a reason for hiding this comment

rdblue commented Feb 19, 2024

ggershinsky Feb 9, 2024 •

edited

Loading