PARQUET-1817: Crypto Properties Factory by shangxinli · Pull Request #769 · apache/parquet-java

shangxinli · 2020-03-16T15:26:56Z

Make sure you have checked all steps below.

Jira

My PR addresses the following Parquet Jira issues and references them in the PR title. For example, "PARQUET-1234: My Parquet PR"
- https://issues.apache.org/jira/browse/PARQUET-XXX
- In case you are adding a dependency, check if the license complies with the ASF 3rd Party License Policy.

Tests

My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

My commits all reference Jira issues in their subject lines. In addition, my commits follow the guidelines from "How to write a good git commit message":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters (not including Jira issue reference)
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"

Documentation

In case of new functionality, my PR adds documentation that describes how to use it.
- All the public functions and the classes in the PR contain Javadoc that explain what it does

ggershinsky · 2020-03-17T09:28:13Z

Travis fails because the branch is broken (on parquet-format-structures - unhandled mix of encryption and bloom filters). A known problem, I'll send a fix for that today.

ggershinsky · 2020-03-17T13:48:42Z

the branch is fixed by #771 . Once it is merged, your pr should pass CI checks after rebase.

gszadovszky · 2020-03-18T08:55:48Z

#771 is merged, Travis restarted.

ggershinsky · 2020-03-18T09:52:07Z

"[WARNING] Files with unapproved licenses:
/home/travis/build/apache/parquet-mr/parquet-hadoop/src/main/java/org/apache/parquet/crypto/CryptoPropertiesFactory.java"

shangxinli · 2020-03-18T14:44:18Z

Fixed it. Thanks Gidon and Gabor!

ggershinsky · 2020-03-19T12:57:21Z

@shangxinli @gszadovszky my pull request is ready, but now it depends on this one. I'll send it after 769 is merged.

shangxinli · 2020-03-19T16:45:40Z

@shangxinli @gszadovszky my pull request is ready, but now it depends on this one. I'll send it after 769 is merged.

The build is green now. Once @gszadovszky review the code, he or myself can merge it and you are ready to go.

gszadovszky

I assume we will see some implementations of this class in the unit tests of the crypto feature. Just double check...

parquet-hadoop/src/main/java/org/apache/parquet/crypto/CryptoPropertiesFactory.java

shangxinli · 2020-03-23T21:19:32Z

I assume we will see some implementations of this class in the unit tests of the crypto feature. Just double check...

Yes.

shangxinli · 2020-03-23T23:54:53Z

parquet-hadoop/src/main/java/org/apache/parquet/crypto/CryptoPropertiesFactory.java

+   * the unit test (TBD) for example
+   *
+   * @param fileHadoopConfig Configuration that is used to pass the needed information, e.g. KMS uri
+   * @param tempFilePath File path of the parquet file


@ggershinsky, can you make comments about what to be used by this field? Our implementation doesn't need it so far.

I'd suggest also to add a comment to the interface itself, to remind about its purpose (this helps to understand the method parameters), something like "CryptoPropertiesFactory interface enables transparent activation of Parquet encryption. It's custom implementations produce encryption and decryption properties for each Parquet file, using the information available in Parquet file writers and readers: file path, file extended schema (in writers only) - and also Hadoop configuration properties that can pass custom parameters required by a crypto factory. A factory implementation can use or ignore any of these parameters."

As for the "tempFilePath" - "File path of the parquet file being written. Can be used for AAD prefix creation, key material management, etc. Implementations must not presume the path is permanent, as the file can be moved or renamed later"

ggershinsky · 2020-03-24T14:10:58Z

parquet-hadoop/src/main/java/org/apache/parquet/crypto/CryptoPropertiesFactory.java

+
+import java.io.IOException;
+
+import static org.apache.parquet.Preconditions.checkNotNull;


this import is not needed

ggershinsky · 2020-03-25T07:27:31Z

strange. All Travis checks are green,
https://travis-ci.org/github/apache/parquet-mr/builds/666364206?utm_source=github_status&utm_medium=notification
but the PR is stuck on yellow.

chenjunjiedada · 2020-03-25T08:46:12Z

@ggershinsky , FYI, Iceberg has a PR that has the same status.

gszadovszky

Last time I've forgot about one major thing. (Sorry for the late idea.)
parquet-mr library is usually used for either reading or writing at a time but not both. I think, it is not a good practice to expect both encryption and decryption properties while only one would be required.
What do you guys think?

parquet-hadoop/src/main/java/org/apache/parquet/crypto/CryptoPropertiesFactory.java

ggershinsky · 2020-03-25T12:33:14Z

Last time I've forgot about one major thing. (Sorry for the late idea.)
parquet-mr library is usually used for either reading or writing at a time but not both. I think, it is not a good practice to expect both encryption and decryption properties while only one would be required.
What do you guys think?

On one hand, its certainly possible to split this into two interfaces, to make it fit the parquet-mr practices. On the other hand, a person working with encryption in eg Spark shell might want to both write and read the files. He would define two Hadoop config params for that, "parquet.encryption.factory.class" and "parquet.decryption.factory.class", which is a bit cumbersome. Or maybe one parameter is enough - if the class implements both interfaces, and we'll load it as either encryption or decryption as needed (from writer or reader, respectively). If this works in Java (currently can't see why not), then could be the right thing to do.

parquet-hadoop/src/main/java/org/apache/parquet/crypto/CryptoPropertiesFactory.java

…ropertiesFactory.java Co-Authored-By: Gabor Szadovszky <gabor@apache.org>

gszadovszky · 2020-03-25T14:46:46Z

Last time I've forgot about one major thing. (Sorry for the late idea.)
parquet-mr library is usually used for either reading or writing at a time but not both. I think, it is not a good practice to expect both encryption and decryption properties while only one would be required.
What do you guys think?

On one hand, its certainly possible to split this into two interfaces, to make it fit the parquet-mr practices. On the other hand, a person working with encryption in eg Spark shell might want to both write and read the files. He would define two Hadoop config params for that, "parquet.encryption.factory.class" and "parquet.decryption.factory.class", which is a bit cumbersome. Or maybe one parameter is enough - if the class implements both interfaces, and we'll load it as either encryption or decryption as needed (from writer or reader, respectively). If this works in Java (currently can't see why not), then could be the right thing to do.

I like this idea. It should work just fine.

shangxinli · 2020-03-25T14:53:18Z

Last time I've forgot about one major thing. (Sorry for the late idea.)
parquet-mr library is usually used for either reading or writing at a time but not both. I think, it is not a good practice to expect both encryption and decryption properties while only one would be required.
What do you guys think?

On one hand, its certainly possible to split this into two interfaces, to make it fit the parquet-mr practices. On the other hand, a person working with encryption in eg Spark shell might want to both write and read the files. He would define two Hadoop config params for that, "parquet.encryption.factory.class" and "parquet.decryption.factory.class", which is a bit cumbersome. Or maybe one parameter is enough - if the class implements both interfaces, and we'll load it as either encryption or decryption as needed (from writer or reader, respectively). If this works in Java (currently can't see why not), then could be the right thing to do.

I agree with Gidon. Aside from that, some ETL transform jobs need to decrypt raw data Parquet files and encrypt transformed Parquet files.

…ropertiesFactory.java Co-Authored-By: ggershinsky <ggershinsky@users.noreply.github.com>

shangxinli · 2020-03-25T21:41:14Z

@gszadovszky, The latest commit needs to be changed and I think it would be better to add unit tests now than TBD. You can hold off for now and I will let you know when it is ready for review.

…ce and add unit tests

shangxinli · 2020-03-26T17:19:22Z

@gszadovszky, I think the change is ready to review now.

parquet-hadoop/src/main/java/org/apache/parquet/crypto/DecryptionPropertiesFactory.java

parquet-hadoop/src/main/java/org/apache/parquet/crypto/EncryptionPropertiesFactory.java

parquet-hadoop/src/main/java/org/apache/parquet/crypto/ParquetCryptoRuntimeException.java

parquet-hadoop/src/test/java/org/apache/parquet/crypto/SampleDecryptionPropertiesFactory.java

parquet-hadoop/src/test/java/org/apache/parquet/crypto/SampleEncryptionPropertiesFactory.java

…ionPropertiesFactory.java Co-Authored-By: Gabor Szadovszky <gabor@apache.org>

…CryptoRuntimeException.java Co-Authored-By: Gabor Szadovszky <gabor@apache.org>

…ionPropertiesFactory.java Co-Authored-By: Gabor Szadovszky <gabor@apache.org>

* PARQUET-1228: Format Structures encryption (#613) * PARQUET-1286: Crypto package (#614) * PARQUET-1818: Fix bloom/encryption collision in format-structures (#771) * PARQUET-1817: Crypto Properties Factory (#769) * PARQUET-1229: Parquet MR encryption (#776) * PARQUET-1807: Encryption: Interop and Function test suite for Java version (#782) * PARQUET-1373: Encryption key tools (#615) Co-authored-by: shangxinli <31421745+shangxinli@users.noreply.github.com> Co-authored-by: Maya Anderson <mayaa@il.ibm.com>

PARQUET-1817: Crypto Properties Factory

7185aba

shangxinli force-pushed the parquet-1817 branch from 0760281 to 7185aba Compare March 16, 2020 16:00

Fix build error: 'unapproved licenses'

85ae203

gszadovszky requested changes Mar 23, 2020

View reviewed changes

Address feedback

86ac333

shangxinli commented Mar 23, 2020

View reviewed changes

ggershinsky reviewed Mar 24, 2020

View reviewed changes

Address feedback from Gidon

69b1920

gszadovszky requested changes Mar 25, 2020

View reviewed changes

ggershinsky reviewed Mar 25, 2020

View reviewed changes

parquet-hadoop/src/main/java/org/apache/parquet/crypto/CryptoPropertiesFactory.java Outdated Show resolved Hide resolved

parquet-hadoop/src/main/java/org/apache/parquet/crypto/CryptoPropertiesFactory.java Outdated Show resolved Hide resolved

shangxinli and others added 3 commits March 25, 2020 07:23

Update parquet-hadoop/src/main/java/org/apache/parquet/crypto/CryptoP…

85d0326

…ropertiesFactory.java Co-Authored-By: Gabor Szadovszky <gabor@apache.org>

Update parquet-hadoop/src/main/java/org/apache/parquet/crypto/CryptoP…

57a995c

…ropertiesFactory.java Co-Authored-By: Gabor Szadovszky <gabor@apache.org>

Update parquet-hadoop/src/main/java/org/apache/parquet/crypto/CryptoP…

e21d35d

…ropertiesFactory.java Co-Authored-By: Gabor Szadovszky <gabor@apache.org>

shangxinli and others added 4 commits March 25, 2020 07:57

Address feedback round apache#3

fadcf54

Update parquet-hadoop/src/main/java/org/apache/parquet/crypto/CryptoP…

9bf021e

…ropertiesFactory.java Co-Authored-By: ggershinsky <ggershinsky@users.noreply.github.com>

Update parquet-hadoop/src/main/java/org/apache/parquet/crypto/CryptoP…

fd1b10d

…ropertiesFactory.java Co-Authored-By: ggershinsky <ggershinsky@users.noreply.github.com>

Address feedback round apache#4

f72c676

Split encryption and decryption interface to follow parquet-mr practi…

d72b2a1

…ce and add unit tests

shangxinli force-pushed the parquet-1817 branch from a7f02a2 to d72b2a1 Compare March 26, 2020 15:35

gszadovszky requested changes Mar 27, 2020

View reviewed changes

shangxinli and others added 4 commits March 27, 2020 08:18

Update parquet-hadoop/src/main/java/org/apache/parquet/crypto/Decrypt…

c2edf3d

…ionPropertiesFactory.java Co-Authored-By: Gabor Szadovszky <gabor@apache.org>

Update parquet-hadoop/src/main/java/org/apache/parquet/crypto/Parquet…

b223525

…CryptoRuntimeException.java Co-Authored-By: Gabor Szadovszky <gabor@apache.org>

Update parquet-hadoop/src/main/java/org/apache/parquet/crypto/Encrypt…

60167c0

…ionPropertiesFactory.java Co-Authored-By: Gabor Szadovszky <gabor@apache.org>

Change varaible names in test to follow Java coding practice

8bb18eb

shangxinli force-pushed the parquet-1817 branch from bb307db to 8bb18eb Compare March 28, 2020 19:43

gszadovszky approved these changes Mar 30, 2020

View reviewed changes

gszadovszky merged commit bd876bd into apache:encryption Mar 30, 2020

shangxinli added a commit to shangxinli/parquet-mr that referenced this pull request Apr 20, 2020

PARQUET-1817: Crypto Properties Factory (apache#769)

e862df3

gszadovszky pushed a commit that referenced this pull request Apr 22, 2020

PARQUET-1817: Crypto Properties Factory (#769)

42827b1

shangxinli deleted the parquet-1817 branch June 7, 2020 21:51

shangxinli added a commit to shangxinli/parquet-mr that referenced this pull request Jul 15, 2020

PARQUET-1817: Crypto Properties Factory (apache#769)

fa827bb

gszadovszky pushed a commit that referenced this pull request Jul 28, 2020

PARQUET-1817: Crypto Properties Factory (#769)

71ad811

asfimport mentioned this pull request Jun 23, 2024

Crypto Properties Factory #2471

Closed


		import java.io.IOException;

		import static org.apache.parquet.Preconditions.checkNotNull;

Conversation

shangxinli commented Mar 16, 2020

Jira

Tests

Commits

Documentation

Uh oh!

ggershinsky commented Mar 17, 2020

Uh oh!

ggershinsky commented Mar 17, 2020

Uh oh!

gszadovszky commented Mar 18, 2020

Uh oh!

ggershinsky commented Mar 18, 2020

Uh oh!

shangxinli commented Mar 18, 2020

Uh oh!

ggershinsky commented Mar 19, 2020

Uh oh!

shangxinli commented Mar 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gszadovszky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shangxinli commented Mar 23, 2020

Uh oh!

shangxinli Mar 23, 2020

Choose a reason for hiding this comment

Uh oh!

ggershinsky Mar 24, 2020

Choose a reason for hiding this comment

Uh oh!

shangxinli Mar 24, 2020

Choose a reason for hiding this comment

Uh oh!

ggershinsky Mar 24, 2020

Choose a reason for hiding this comment

Uh oh!

shangxinli Mar 24, 2020

Choose a reason for hiding this comment

Uh oh!

ggershinsky commented Mar 25, 2020

Uh oh!

chenjunjiedada commented Mar 25, 2020

Uh oh!

gszadovszky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ggershinsky commented Mar 25, 2020

Uh oh!

Uh oh!

Uh oh!

gszadovszky commented Mar 25, 2020

Uh oh!

shangxinli commented Mar 25, 2020

Uh oh!

shangxinli commented Mar 25, 2020

Uh oh!

shangxinli commented Mar 26, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

shangxinli commented Mar 19, 2020 •

edited

Loading