Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding ClientEncryptionS3PinotFS #8933

Closed
wants to merge 5 commits into from

Conversation

xiangfu0
Copy link
Contributor

@xiangfu0 xiangfu0 commented Jun 20, 2022

https://docs.amazonaws.cn/en_us/sdk-for-java/v1/developer-guide/examples-crypto-masterkey.html

To use client side encryption, you can specify either

  • kmsCmkId which contains the id of the KMS key as the value
  • aesHexSecret which contains a custom generated AES 256 key.

The Filesystem for scheme s3 also needs to be configured to org.apache.pinot.plugin.filesystem.ClientEncryptionS3PinotFS

Example -

Controller conf for deep storage

controller.data.dir=s3://my-bucket/quickstart-data
controller.local.temp.dir=/tmp/pinot/data/controller/index

pinot.controller.storage.factory.class.s3=org.apache.pinot.plugin.filesystem.ClientEncryptionS3PinotFS
pinot.controller.segment.fetcher.protocols=file,http,s3
pinot.controller.segment.fetcher.s3.class=org.apache.pinot.common.utils.fetcher.PinotFSSegmentFetcher
pinot.controller.storage.factory.s3.region=us-east-1
pinot.controller.storage.factory.s3.accessKey=XXXX
pinot.controller.storage.factory.s3.secretKey=XXXX
pinot.controller.storage.factory.s3.kmsCmkId=703373a8-3e4b-4eb8-8a3f-03699a0d0927

Ingestion spec

pinotFSSpecs:
    - scheme: s3
      className: org.apache.pinot.plugin.filesystem.S3PinotFS
      configs:
        region: 'us-east-1'
        accessKey: 'XXXX'
        secretKey: 'XXXX'
        kmsCmkId: '703373a8-3e4b-4eb8-8a3f-03699a0d0927'

@npawar npawar requested a review from KKcorps June 20, 2022 21:56
@xiangfu0 xiangfu0 force-pushed the s3-pinot-fs-improvement branch 4 times, most recently from 21fd4e2 to c4776bd Compare June 21, 2022 09:03
@xiangfu0 xiangfu0 force-pushed the s3-pinot-fs-improvement branch from c4776bd to 42bc816 Compare June 21, 2022 09:34
pinot-plugins/pom.xml Outdated Show resolved Hide resolved
pinot-plugins/pom.xml Outdated Show resolved Hide resolved
@KKcorps
Copy link
Contributor

KKcorps commented Jun 21, 2022

Weird that AWS Java SDK 2 doesn't have encryption based S3 client. The issue seems to be open from last 5 years - aws/aws-sdk-java-v2#34

@xiangfu0 xiangfu0 force-pushed the s3-pinot-fs-improvement branch from 42bc816 to c5449b7 Compare June 21, 2022 18:35
@xiangfu0
Copy link
Contributor Author

This also requires some refactor on the common utils for S3PinotFS as well.

@xiangfu0
Copy link
Contributor Author

xiangfu0 commented Jun 21, 2022

Weird that AWS Java SDK 2 doesn't have encryption based S3 client. The issue seems to be open from last 5 years - aws/aws-sdk-java-v2#34

Right, so for the client-side encryption, we need to use new lib and write a new PinotFS implementation 🤦

@xiangfu0 xiangfu0 force-pushed the s3-pinot-fs-improvement branch from c5449b7 to f72ad06 Compare June 21, 2022 23:59
@KKcorps KKcorps changed the title [WIP]Adding ClientEncryptionS3PinotFS Adding ClientEncryptionS3PinotFS Jun 29, 2022
@KKcorps
Copy link
Contributor

KKcorps commented Jun 29, 2022

Added some unit tests as well.

@KKcorps KKcorps force-pushed the s3-pinot-fs-improvement branch from 63b7537 to cd6ca03 Compare June 29, 2022 16:22
@codecov-commenter
Copy link

codecov-commenter commented Jun 29, 2022

Codecov Report

Merging #8933 (0316f6c) into master (93fa0e7) will decrease coverage by 0.09%.
The diff coverage is 37.45%.

@@             Coverage Diff              @@
##             master    #8933      +/-   ##
============================================
- Coverage     70.39%   70.30%   -0.10%     
- Complexity     5694     5724      +30     
============================================
  Files          1991     1992       +1     
  Lines        107634   107907     +273     
  Branches      16361    16402      +41     
============================================
+ Hits          75773    75868      +95     
- Misses        26553    26710     +157     
- Partials       5308     5329      +21     
Flag Coverage Δ
integration1 24.55% <0.00%> (-0.08%) ⬇️
integration2 24.25% <0.00%> (-0.07%) ⬇️
unittests1 67.97% <ø> (-0.02%) ⬇️
unittests2 13.66% <37.45%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...a/org/apache/pinot/plugin/filesystem/S3Config.java 0.00% <0.00%> (ø)
.../org/apache/pinot/plugin/filesystem/S3PinotFS.java 40.92% <ø> (+0.26%) ⬆️
...t/plugin/filesystem/ClientEncryptionS3PinotFS.java 38.00% <38.00%> (ø)
...ore/query/scheduler/resources/ResourceManager.java 88.46% <0.00%> (-11.54%) ⬇️
...n/java/org/apache/pinot/common/utils/URIUtils.java 66.66% <0.00%> (-7.41%) ⬇️
...tream/kafka20/server/KafkaDataServerStartable.java 72.91% <0.00%> (-6.25%) ⬇️
.../predicate/NotEqualsPredicateEvaluatorFactory.java 68.75% <0.00%> (-5.36%) ⬇️
...elix/core/periodictask/ControllerPeriodicTask.java 66.07% <0.00%> (-5.36%) ⬇️
.../filter/predicate/InPredicateEvaluatorFactory.java 74.43% <0.00%> (-4.52%) ⬇️
.../pinot/core/query/scheduler/PriorityScheduler.java 77.77% <0.00%> (-2.78%) ⬇️
... and 25 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@KKcorps KKcorps force-pushed the s3-pinot-fs-improvement branch 3 times, most recently from 19c46dc to 66b4254 Compare June 30, 2022 18:21
@KKcorps KKcorps force-pushed the s3-pinot-fs-improvement branch from a2860e6 to 66b4254 Compare July 20, 2022 09:03
@KKcorps KKcorps force-pushed the s3-pinot-fs-improvement branch from 2e9ee80 to 85873f1 Compare July 27, 2022 17:05
@xiangfu0 xiangfu0 force-pushed the s3-pinot-fs-improvement branch from 85873f1 to 7c5b03d Compare January 8, 2023 08:23
@xiangfu0
Copy link
Contributor Author

Close since no further actions.

@xiangfu0 xiangfu0 closed this Mar 17, 2023
@rino-kadijk
Copy link
Contributor

I recommend also looking into a key rotation strategy

@vmarchaud
Copy link

Close since no further actions.

Not sure to understand why it didn't got merged ? The S3 plugin doesn't support AES 256 encryption currently right ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants