Remove IAM role from its instance profile before deleting orphaned role

[shreyaabaranwal]

Fixes #18838

When the last instance group of a role is deleted (for example kops delete ig bastions), the next kops update cluster --yes tries to garbage-collect the orphaned IAM role. The role is still in its instance profile, so DeleteRole fails with DeleteConflict. The task keeps retrying until the 10 minute deadline and the whole update fails. Automation like update --yes && rolling-update --yes stops halfway, and every later update fails the same way until the profile is fixed by hand.

The delete branch of IAMRole.RenderAWS removes inline and managed policies and then calls DeleteRole, but never removes the role from its instance profile. FindDeletions also doesn't queue the orphaned profile.

What this PR changes:

• Before touching the role's policies, look up the instance profile with the same name as the role (instance group roles and profiles are both named from IAMName). If it is tagged kubernetes.io/cluster/<cluster>: owned, remove the role from it and delete it, then delete the role as before.
• If that profile exists but isn't owned by the cluster, log a warning and skip the role instead of failing the update.
• In FindDeletions, don't queue a role whose same-name IAMInstanceProfile is still in the model. This covers spec.iam.profile pointing at kops's own profile, where the role drops out of the model but running nodes still use the profile.
• Roles without an instance profile (IRSA service account roles) are deleted exactly as before.

I used GetInstanceProfile with the known name rather than adding ListInstanceProfilesForRole to the IAM interface, to keep the change small and easy to backport. I kept this in the role task instead of queuing a separate profile deletion, because there's no ordering between deletion tasks from FindDeletions today, so the role and profile deletions would run in parallel.

Mock changes:

mockiam didn't behave like AWS here, which is why no test caught this. DeleteRole now returns DeleteConflictException while the role is in an instance profile and NoSuchEntityException for a missing role. DeleteInstanceProfile returns DeleteConflictException while the profile still has a role. I also added DetachRolePolicy, which was missing.

Tests:

• iamrole_test.go: owned role in an owned profile (role and profile are deleted), profile not owned by the cluster (nothing is detached or deleted, no error), and a role without a profile (unchanged behaviour). The first two fail on master with the same DeleteConflict deadline error as the issue.
• iam_test.go: FindDeletions doesn't queue a role whose profile is still in the model.
• go test ./cmd/kops/... (lifecycle and integration tests) pass with no golden file changes. make quick-ci and verify-golangci-lint pass.

Known limits:

• Only the same-name profile is checked. If someone added the role to a differently named profile by hand, the update still fails like before.
• If the same-name profile isn't owned, the leftover role shows up as a pending change with a warning on every update.

Open question:

If an instance group switches from the managed profile to a custom spec.iam.profile, the old instances keep using the managed profile until a rolling update. With this change, the next update --yes would remove that profile before the rolling update. Is that acceptable, or would you prefer to skip it in that case?

Possible follow-ups (not in this PR):

• kops delete cluster has the same gap for untagged profiles.
• iaminstanceprofile.go:135 dereferences a.Name inside the a == nil branch.
• The mock ListAttachedRolePolicies compares role name pointers instead of values.

[kubernetes-prow]

Hi @shreyaabaranwal. 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 /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.

Details

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-sigs/prow repository.

[kubernetes-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign hakman for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

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