-
Notifications
You must be signed in to change notification settings - Fork 302
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
Add cluster details to the sts through headers #649
Conversation
This issue is currently awaiting triage. If cloud-provider-aws contributors determine this is a relevant issue, they will accept it by applying the The Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Hi @kmala. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/ok-to-test |
/ok-to-test |
/assign @nckturner |
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.
Couple nits, nothing major.
/lgtm
/retest |
@kmala: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
/retest |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cartermckinnon, nckturner The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[release-1.23] Cherry pick of #649: Add cluster details to the sts through headers
Just as a heads up: So far we were (mis)using the AWS CCM and configured it using a cloud-config that looked like this:
Even though the field was clearly named So maybe this "break" would be worth highlighting in the release notes. |
There is no validation before this PR but it wouldn't have worked right?
Yes, added to the release notes. |
It worked just fine with a broken ARN value in the
Thanks, but where can I find the release notes for 0.29? I cannot see a GitHub release and https://github.com/kubernetes/cloud-provider-aws/blob/master/docs/CHANGELOG.md stopped at 1.22. Just so in the future I know where to look :-) |
Description: Add caller info (source account and source arn) to STS requests that legacy cloud provider makes on behalf of customer through request headers, this is for confusion deputy issue protection. With the change customer is able to configure global conditional key in their IAM role in addition to trust policies https://docs.aws.amazon.com/IAM/latest/UserGuide/confused-deputy.html. Upstream PR, Issue, KEP, etc. links: We did same change in CCM kubernetes/cloud-provider-aws#649 but for versions < 1.27, KCM still has in-tree legacy cloud provider. This patch is for these legacy cloud providers in KCM. If this patch is based on an upstream commit, how (if at all) do this patch and the upstream source differ? N/A If this patch's changes have not been added by upstream, why not? Versions < 1.27 are out of support upstream. Other patches related to this patch: N/A Changes made to this patch after its initial creation and reasons for these changes: N/A Kubernetes version this patch can be dropped: Till 1.22 is out of EKS support. Signed-off-by: Leo Li <haoranr@amazon.com>
Description: Add caller info (source account and source arn) to STS requests that legacy cloud provider makes on behalf of customer through request headers, this is for confusion deputy issue protection. With the change customer is able to configure global conditional key in their IAM role in addition to trust policies https://docs.aws.amazon.com/IAM/latest/UserGuide/confused-deputy.html. Upstream PR, Issue, KEP, etc. links: We did same change in CCM kubernetes/cloud-provider-aws#649 but for versions < 1.27, KCM still has in-tree legacy cloud provider. This patch is for these legacy cloud providers in KCM. If this patch is based on an upstream commit, how (if at all) do this patch and the upstream source differ? N/A If this patch's changes have not been added by upstream, why not? Versions < 1.27 are out of support upstream. Other patches related to this patch: N/A Changes made to this patch after its initial creation and reasons for these changes: N/A Kubernetes version this patch can be dropped: Till 1.22 is out of EKS support. Signed-off-by: Leo Li <haoranr@amazon.com>
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR would add additional headers passing the clusters arn and cx account id when making calls to sts and making calls using an assumed roles.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: