-
Notifications
You must be signed in to change notification settings - Fork 424
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
Optionally return full role arn including path during token verification #632
Conversation
Signed-off-by: Terry Cain <[email protected]>
|
Welcome @terrycain! |
Hi @terrycain. Thanks for your PR. I'm waiting for a kubernetes-sigs 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. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: terrycain The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@dims / @nckturner / @nnmin-aws can we get a review on this please? |
/test |
@nnmin-aws: The
Use In response to this:
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. |
/test all |
Thanks @nnmin-aws the tests seem to have passed, what now? |
@nnmin-aws @dims @nckturner what else is required for this PR? There's been no progress for nearly 2 months, some guidance would be appreciated. |
@terrycain @dims apology for the delay! i am so sorry about it. We were hesitating at the change because it requires additional invocation of aws IAM and might increase latency of authN. There is another PR actually merged https://github.com/kubernetes-sigs/aws-iam-authenticator/pull/670/files#diff-5de38ab8d77d935766314beeb9a9e3b7165eb470e656dbb5e4fdf9bb60ae59de. which removed the path from the arn comparison. Could you please kindly pull latest release and verify if it works for your scenario? If it doesn't, we can check this PR again. Thank you! |
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
What this PR does / why we need it: When creating EKS nodegroups where the node's IAM role contains a path, the nodes fail to join the cluster. This adds the ability to look up a role's full ARN including the path and return that when verifying a token.
The reason this is needed is because the assume-role ARN that is returned by
sts.get-caller-idendentity
does not include the IAM role's path.I've added a "role cache" which calles
iam.list-roles
and stores a mapping between the roles unique role ID and the full ARN, as theUserID
returned bysts.get-caller-identity
contains the roles unique ID therefore looking up the existing role's correct ARN is possible.This does require the clusters IAM role to have the
iam:ListRoles
action. If the role cache gets an error when calling list roles and its not a transient error, it'll disable all future lookups and the functionality will revert to the original behaviour. I figured it would be better to make this more an opt in rather than a requirement as else it might just break nodes joining a cluster whenever its released.Which issue(s) this PR fixes:
Fixes #268
Fixes #153