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

Support the cross-account access using IAM role for S3 PinotFS #10009

Merged
merged 2 commits into from
Dec 21, 2022

Conversation

snleee
Copy link
Contributor

@snleee snleee commented Dec 20, 2022

DM-404
When accountA tries to access data in S3 bucket owned by accountB, AWS provides a way to establish the access to the S3 bucket using cross-account IAM role. This approach is preferred in some cases because accountKey, secretKey doesn't need to be exposed.

https://repost.aws/knowledge-center/cross-account-access-s3

When accountA tries to access data in S3 bucket owned by accountB,
AWS provides a way to establish the access to the S3 bucket
using cross-account IAM role. This approach is preferred in
some cases because `accountKey, secretKey` doesn't need to be
exposed.

https://repost.aws/knowledge-center/cross-account-access-s3
@snleee snleee requested a review from KKcorps December 20, 2022 07:32
@snleee
Copy link
Contributor Author

snleee commented Dec 20, 2022

This is a bit complex to add the test because it involves in multiple AWS accounts to try out. I have tested this locally using 2 different AWS accounts and the IAM role based access was successfully established.

Copy link
Contributor

@navina navina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor comments. lgtm!

@codecov-commenter
Copy link

codecov-commenter commented Dec 20, 2022

Codecov Report

Merging #10009 (447e823) into master (f57d922) will increase coverage by 45.33%.
The diff coverage is 0.00%.

@@              Coverage Diff              @@
##             master   #10009       +/-   ##
=============================================
+ Coverage     25.06%   70.40%   +45.33%     
- Complexity       44     5681     +5637     
=============================================
  Files          1979     1992       +13     
  Lines        106930   107324      +394     
  Branches      16277    16318       +41     
=============================================
+ Hits          26800    75558    +48758     
+ Misses        77405    26494    -50911     
- Partials       2725     5272     +2547     
Flag Coverage Δ
integration1 25.11% <0.00%> (+0.05%) ⬆️
integration2 24.43% <0.00%> (?)
unittests1 67.87% <ø> (?)
unittests2 13.59% <0.00%> (?)

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.65% <0.00%> (+40.65%) ⬆️
...ache/pinot/core/operator/docidsets/OrDocIdSet.java 86.36% <0.00%> (-9.10%) ⬇️
...tream/kafka20/server/KafkaDataServerStartable.java 72.91% <0.00%> (-6.25%) ⬇️
...pache/pinot/core/query/utils/idset/EmptyIdSet.java 25.00% <0.00%> (ø)
...anager/realtime/SegmentBuildTimeLeaseExtender.java 63.23% <0.00%> (ø)
...apache/pinot/ingestion/jobs/SegmentUriPushJob.java 0.00% <0.00%> (ø)
...he/pinot/ingestion/utils/JobPreparationHelper.java 0.00% <0.00%> (ø)
.../pinot/ingestion/jobs/SegmentPreprocessingJob.java 0.00% <0.00%> (ø)
.../pinot/ingestion/common/PinotIngestionJobType.java 0.00% <0.00%> (ø)
... and 1449 more

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

@KKcorps
Copy link
Contributor

KKcorps commented Dec 21, 2022

Yeah, I also haven't figured out how to write a test for this. I tried doing it via Localstack once but it has some bugs with mock IAM due to which the access doesn't work properly if you create multiple accounts.

Copy link
Contributor

@KKcorps KKcorps left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for moving the S3Config to a seperate class.

@snleee snleee merged commit 20ff018 into apache:master Dec 21, 2022
@snleee snleee deleted the s3fs-iam-role branch December 21, 2022 16:00
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.

4 participants