Cleanup orphaned IAM service account roles in direct render #11497
Conversation
k8s-ci-robot
added
cncf-cla: yes
| @@ -441,3 +444,41 @@ func (b *IAMModelBuilder) buildAWSIAMRolePolicy(role iam.Subject) (fi.Resource, | |||
|
|
|||
| return fi.NewStringResource(policy), nil | |||
| } | |||
|
|
|||
| func (b *IAMModelBuilder) FindDeletions(context *fi.ModelBuilderContext, cloud fi.Cloud) error { | |||
There was a problem hiding this comment.
Is model the right place to implement this?
There was a problem hiding this comment.
I think this is no less awkward than what I did with SG rules.
The challenge is that there is no relationship between tasks and models. Another model could also provision iam roles if that made sense.
An easy solution to this would be that the model add an additional tag to the resources it creates. And then replace ownershipTag below with that tag. E.g kops.k8s.io/<cluster name>/owner: awsmodel/iammodelbuilder
There was a problem hiding this comment.
I think this is no less awkward than what I did with SG rules.
I think #10251 is not yet merged.
The challenge is that there is no relationship between tasks and models.
It's always a challenge to track and handle orphaned resources and various hacks were made for that reason. I think the main reason this was not made easier initially was the lack of tags at that time. Another reason is that we
Moving this logic to the model not such a good idea and if we don't find a good alternative now will become a permanent thing.
There was a problem hiding this comment.
This is absolutely the right place to implement this. The model knows what is supposed to be there, so after it has constructed what should be there it can compare that against what actually is there. Just like any well-behaved controller.
One could talk about consolidating the cluster and instancegroup destruction code into this mechanism in order to reduce the possibility of logic discrepancies, but that would be for future PRs.
There was a problem hiding this comment.
The challenge is that there is no relationship between tasks and models.
There is a relationship: models create tasks. This PR uses the tasks generated by Loader.BuildTasks() as a record of which objects the model says should exist. This is why FindDeletions runs as a second pass.
There was a problem hiding this comment.
I was not opposed to this, just not a fan of adding AWS SDK code in model.
Also, the FindDeletions name is something that I would see more as FindOrphaned and leave FindDeletions for tasks.
In general, making use of this may fix some of the awkwardness in some task deletions.
There was a problem hiding this comment.
Well, this particular ModelBuilder is AWS-specific and in a directory for AWS-specific model code.
I made HasDeletions require ModelBuilder so that both could be added to Loader.Builders. I wanted to avoid errors where something that was both didn't get missed off of one of the two lists.
But HasDeletions is closer to Task than ModelBuilder, despite needing to take a ModelBuilderContext in order to construct new Tasks.
|
IAM role tags were added in kops 1.20 (4ee5d7a) which means if a cluster was last updated with kops <1.20 and then deleted with a version of kops that includes this PR, the untagged roles would not be cleaned up. Is that something we should be concerned with? |
|
|
|
I don't think deleting a cluster with a newer version of kops than was used to create it is a use case we should spend a lot of time supporting. We could do the |
|
This looks good to me. Do the new deletions show up in the dryrun output? It would be inconvenient if someone created an IAM role externally that followed the naming and tagging convention and they inadvertently end up deleting the role after a kops upgrade. |
|
Deletions do show up in the dryrun output. This is a big reason why I used |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rifelpet 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 |
k8s-ci-robot
added
the
approved
No description provided.