-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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 support for permission boundaries for AWS IAM Roles #9773
Conversation
Welcome @victorfrancax1! |
Hi @victorfrancax1. 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 |
overall this looks good. Can you add an integration test case for this? It should just involve setting the new api field with a dummy value in the two My only question is around the API design. Would it make sense to ever have separate permission boundaries for the master role vs the node role? If so we'd want the API to be able to support that. |
Hey @rifelpet , thanks for reviewing! I added an integration test case as requested. About the API design: although being possible, it's seems very unlikely to me that people would need to set permission boundaries per instance role, since they were designed mainly to prevent privilege escalation. The furthest I would go personally is configuring different boundaries for each cluster, which is supported by this implementation. As stated by the author of the original issue (can't tag him here for some reason), should an edge case arise, we could suggest that they provide their own roles to |
/test pull-kops-e2e-k8s-containerd |
f0eca31
to
cddbc9b
Compare
one last comment and I think this will be good to merge :) thanks for adding this! |
/lgtm This will land in Kops 1.19. I believe there will be another alpha release soon if you want to use it in an official build. Thanks again! |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rifelpet, victorfrancax1 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 |
One more thing @victorfrancax1 would you mind adding some docs here? This can be done in a separate PR. |
Refs #7286
Also adding Terrraform and Cloudformation support.