-
Notifications
You must be signed in to change notification settings - Fork 13.3k
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
[FLINK-14881] [s3|kinesis] Add support for IAM Roles for Service Accounts on AWS EKS #12008
Conversation
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Automated ChecksLast check on commit 444df93 (Wed May 06 13:48:27 UTC 2020) Warnings:
Mention the bot in a comment to re-run the automated checks. Review Progress
Please see the Pull Request Review Guide for a full explanation of the review process. The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commandsThe @flinkbot bot supports the following commands:
|
…5.9 and add aws-java-sdk-sts dependency to support WebIdentityToken
@flinkbot run azure |
Thanks a lot for your contribution. I manually re-triggered the failed job on Azure: https://dev.azure.com/apache-flink/apache-flink/_build/results?buildId=721&view=results |
CI has passed ✅ |
Thanks @rmetzger! |
@tweise (kinesis) @pnowojski (s3): What's your take on this change? |
It would be great if someone could take a second look. |
Okay, looks like nobody has time for this. I will test it myself, because it would be a bummer if this change didn't make it into 1.11.0 |
public static final String AWS_ROLE_ARN = roleArn(AWS_CREDENTIALS_PROVIDER); | ||
|
||
/** The role session name to use when credential provider type is set to ASSUME_ROLE. */ | ||
/** The role session name to use when credential provider type is set to ASSUME_ROLE or WEB_IDENTITY_TOKEN. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you document this also in docs/dev/connectors/kinesis.md
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean in general that this authentication method exists, not this particular line of code :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw there was no dedicated section for setting up access to Kinesis. I thought it makes sense to add a new section with some details.
I'm not a technical writer, so please let me know what you think :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot. It reads pretty well!
@rmetzger I don't have time to test this change, but I also think it should be merged before 1.11.0 so that any hidden issues can be caught as part of the release candidate testing. As we have just discovered with the kinesis producer in 1.10, benign dependency changes can have unforeseen consequences, so putting this through a thorough benchmark would be in order before release. |
@aroch Thanks a lot for updating the docs! I'm still working on this. |
Actually, I managed to get a debian based docker image. I still have the mentioned exception though. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a "doesn't break things" point of view, I tested Flink on EKS with a Kinesis job, checkpointing to S3, and it properly got the right access to these services in the EKS environment. No objections to merge from that point of view.
I have found two minor license documentation issues. Once these are resolved, I will merge the PR.
Let me know if you don't have time to resolve these, then I can fix & merge.
- com.amazonaws:aws-java-sdk-dynamodb:1.11.754 | ||
- com.amazonaws:aws-java-sdk-kms:1.11.754 | ||
- com.amazonaws:aws-java-sdk-s3:1.11.754 | ||
- com.amazonaws:jmespath-java:1.11.754 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The com.amazonaws:aws-java-sdk-sts:1.11.754
dependency is missing here?
- com.amazonaws:aws-java-sdk-core:1.11.754 | ||
- com.amazonaws:aws-java-sdk-dynamodb:1.11.754 | ||
- com.amazonaws:aws-java-sdk-kms:1.11.754 | ||
- com.amazonaws:aws-java-sdk-s3:1.11.754 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The com.amazonaws:aws-java-sdk-sts:1.11.754 dependency is missing here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot. Merging
…bIdentityToken This closes apache#12008
What is the purpose of the change
IAM Roles for Service Accounts have many advantages when deploying Flink on AWS EKS.
From AWS documentation:
In order to support for IAM Roles for Service Accounts we need to add support for a relatively new authentication method in AWS SDK called WebIdentityToken.
This pull request adds support for WebIdentityToken authentication method when using AWS services. This includes S3 and Kinesis.
Brief change log
S3
Kinesis
Verifying this change
It is tricky to verify this change by end-to-end test as Kinesalite doesn't support authentication methods. Minio does support it but requires extra containers to be launched to do the actual token verification that will complicate and lengthen the s3 tests.
Added configuration unit tests in
AWSUtilTest
As this change is mainly a dependency change, I verified it manually on an actual EKS cluster.
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (yes / no)Documentation
Currently I could not find the deployment on EKS documented anywhere. If needed i'm willing to add docs if that would be helpful.