Separate EKS-related IAM resources #2662

johananl · 2021-08-11T12:54:40Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
This PR works around the AWS character limit for managed policies. In the e2e tests the AWSIAMManagedPolicyControllers policy is currently very close to the AWS limit, which means that the next time a feature adds a few lines to this policy, the CloudFormation template would error out (I've already hit this limit while working on #2271, hence this issue + PR). More info and repro instructions in #2642.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #2642

Special notes for your reviewer:

Checklist:

squashed commits
adds unit tests

Release note:

Move EKS-related resources from `AWSIAMManagedPolicyControllers` to a separate policy to work around AWS size limit for managed policies.

k8s-ci-robot · 2021-08-11T12:54:47Z

Welcome @johananl!

It looks like this is your first PR to kubernetes-sigs/cluster-api-provider-aws 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api-provider-aws has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2021-08-11T12:54:48Z

Hi @johananl. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

randomvariable · 2021-08-11T13:27:29Z

/ok-to-test

sedefsavas · 2021-08-12T00:46:58Z

cmd/clusterawsadm/cloudformation/bootstrap/template.go

@@ -96,6 +97,16 @@ func (t Template) RenderCloudFormation() *cloudformation.Template {
 		Roles:             t.controllersPolicyRoleAttachments(),
 	}

+	if t.Spec.EKS.Enable {


This field is going to be changed with #2648

Let's wait for it to be merged first and then rebase.
/hold

Currently the AWSIAMManagedPolicyControllers policy is quite large and sometimes exceeds the maximum number of characters AWS allows in managed policies. Separating the policy into smaller policies can solve this problem since multiple policies can be attached to an IAM user/role. It makes sense to separate EKS-related resources specifically because they are grouped nicely under a conditional and refer to one logical entity.

johananl · 2021-08-13T13:56:41Z

Let's wait for it to be merged first and then rebase.

@sedefsavas done.

randomvariable · 2021-08-18T11:45:50Z

Given the impact of this is creating new policies, I think we may need to bump the minor version to v0.8.0

johananl · 2021-08-19T09:48:23Z

Given the impact of this is creating new policies, I think we may need to bump the minor version to v0.8.0

OK. Is there anything I should do to make this happen @randomvariable?

Also, if we're incrementing the version for this, should we proactively move any other IAM statements to new policies while we're at it? Right now the EKS-related statements seem to make the biggest difference in terms of policy size.

johananl · 2021-08-19T09:49:46Z

/unhold

randomvariable · 2021-08-19T10:14:29Z

Also, if we're incrementing the version for this, should we proactively move any other IAM statements to new policies while we're at it?

I think so. Can you create an issue documenting any suggested changes as we need to think carefully about upgrades here.

I'm placing the hold back on because we haven't decided on a branch split yet - will raise at next office hours.

/hold

randomvariable · 2021-08-19T10:16:40Z

This is big enough impact in terms of upgrades that changes should be recorded in an ADR too, see https://github.com/kubernetes-sigs/cluster-api-provider-aws/tree/main/docs/adr

johananl · 2021-08-20T13:39:18Z

Can you create an issue documenting any suggested changes as we need to think carefully about upgrades here.

@randomvariable I took a closer look at the various IAM policies. AFAICT the only policy which has a risk of hitting the 6144-char size limit is AWSIAMManagedPolicyControllers. Within that policy, EKS-related resources amount to ~50% of the policy size, which makes them good candidates for moving into a new policy, especially given the fact that these statements already live neatly under a conditional based on the t.Spec.EKS.Disable knob.

Given the above and given that we have to balance between the per-policy size limit and the limit on the number of policies which can be attached to an IAM entity (default: 10), I don't think we need to do anything other than break out the EKS-related resources from AWSIAMManagedPolicyControllers at the moment. We could, of course, go more fine-grained than that, but in terms of policy size nothing else seems to justify creating a new policy.

As far as upgrades and ADRs go, I'm happy to follow any necessary process, however do you feel this is indeed such a big change? We're talking about moving IAM statements which are already applied to a new policy that's attached to the same entities (roles/users/groups). Assuming we don't make any policy logic mistakes and the "allow/deny" end result is identical, AFAICT the users don't need to do anything special when upgrading and I can't find any reason for downtime or other problems. Following the same logic, this change also doesn't feel like an architectural change to me.

What do you think?

invidian · 2021-08-23T17:16:48Z

cmd/clusterawsadm/cloudformation/bootstrap/cluster_api_controller.go

+	}
+}
+
+// ControllersPolicyEKS creates a policy from a template for AWS Controllers.


@johananl I think it would be nice to document the 10 policies limit somewhere in here, to indicate why other resources has not been put in separate policies. We could as well do this in #2271 though.

I think it's ok for now. The 10 policies limit should be relatively well understood by folk in the IAM space.

randomvariable · 2021-08-24T09:08:01Z

/lgtm
/approve
/unhold

k8s-ci-robot · 2021-08-24T09:08:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: randomvariable

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:

~~OWNERS~~ [randomvariable]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority labels Aug 11, 2021

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 11, 2021

k8s-ci-robot requested review from dthorsen and randomvariable August 11, 2021 12:54

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 11, 2021

johananl changed the title ~~🐛 Separate EKS-related resources~~ Separate EKS-related resources Aug 11, 2021

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 11, 2021

invidian mentioned this pull request Aug 11, 2021

✨ Add support for Ignition-based bootstrap data and Flatcar Container Linux #2271

Merged

3 tasks

sedefsavas reviewed Aug 12, 2021

View reviewed changes

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 12, 2021

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 13, 2021

johananl changed the title ~~Separate EKS-related resources~~ Separate EKS-related IAM resources Aug 13, 2021

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 19, 2021

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 19, 2021

randomvariable added the adr-required Denotes an issue or PR contains a decision that needs documenting using an ADR. label Aug 19, 2021

invidian reviewed Aug 23, 2021

View reviewed changes

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 24, 2021

k8s-ci-robot assigned randomvariable Aug 24, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 24, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 24, 2021

k8s-ci-robot merged commit 9c2e028 into kubernetes-sigs:main Aug 24, 2021

k8s-ci-robot added this to the v0.7.0 milestone Aug 24, 2021

invidian deleted the johananl/compact-iam-policies branch August 24, 2021 09:34

johananl mentioned this pull request Nov 3, 2021

REQUEST: New membership for johananl kubernetes/org#3082

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate EKS-related IAM resources #2662

Separate EKS-related IAM resources #2662

johananl commented Aug 11, 2021

k8s-ci-robot commented Aug 11, 2021

k8s-ci-robot commented Aug 11, 2021

randomvariable commented Aug 11, 2021

sedefsavas Aug 12, 2021

johananl commented Aug 13, 2021

randomvariable commented Aug 18, 2021

johananl commented Aug 19, 2021

johananl commented Aug 19, 2021

randomvariable commented Aug 19, 2021

randomvariable commented Aug 19, 2021

johananl commented Aug 20, 2021

invidian Aug 23, 2021

randomvariable Aug 24, 2021

randomvariable commented Aug 24, 2021

k8s-ci-robot commented Aug 24, 2021

Separate EKS-related IAM resources #2662

Separate EKS-related IAM resources #2662

Conversation

johananl commented Aug 11, 2021

k8s-ci-robot commented Aug 11, 2021

k8s-ci-robot commented Aug 11, 2021

randomvariable commented Aug 11, 2021

sedefsavas Aug 12, 2021

Choose a reason for hiding this comment

johananl commented Aug 13, 2021

randomvariable commented Aug 18, 2021

johananl commented Aug 19, 2021

johananl commented Aug 19, 2021

randomvariable commented Aug 19, 2021

randomvariable commented Aug 19, 2021

johananl commented Aug 20, 2021

invidian Aug 23, 2021

Choose a reason for hiding this comment

randomvariable Aug 24, 2021

Choose a reason for hiding this comment

randomvariable commented Aug 24, 2021

k8s-ci-robot commented Aug 24, 2021