Ensure the ServiceAccount IRSA annotation is in place on each reconciliation loop #87
Conversation
…liation loop Signed-off-by: Matt Wise <[email protected]>
Signed-off-by: Matt Wise <[email protected]>
|
You can see our internal PR at Nextdoor#4 - with some of the comments and discussions we have had. We've been running our own fork based on this code for the last 3-4 weeks now with great success. |
diranged
changed the title
sure. I will check those conversations too. will update in couple of days. |
|
@mnkg561 Any update on this? I am digging into a race-condition error in iam.go right now .. but when I fix it, it would be nice to know that this change got landed. We're still using our custom build in our environment and it's been working well (with the exception of a delete race condition)... |
There was a problem hiding this comment.
My bad- I was busy with other project https://github.com/keikoproj/alert-manager and everytime i tried to do the review i lost track of it since its a big PR.
We should meet up some time to understand more on how you guys are using so we can learn from each other.
And, Thank you for the contribution. I like the way you expanded ServiceAccount methods- neat!!
| //oh oh.. This is delete use case | ||
| //Lets make sure to clean up the iam role | ||
| // oh oh.. This is delete use case | ||
| // Lets make sure to clean up the iam role | ||
| if iamRole.Status.RetryCount != 0 { | ||
| iamRole.Status.RetryCount = iamRole.Status.RetryCount + 1 | ||
| } | ||
| log.Info("Iamrole delete request") | ||
| if iamRole.Status.State != iammanagerv1alpha1.PolicyNotAllowed { |
There was a problem hiding this comment.
If we are checking for RoleName presence then this line probably NOT required anymore-
| IamRoleARN: iamRole.Status.RoleARN, | ||
| ServiceAccountName: saName, | ||
| } | ||
| if _, err := rbac.EnsureServiceAccount(ctx, saRequest); err != nil { |
There was a problem hiding this comment.
I would rather want here a comparison instead of "updating" anything- Lets do the comparison here - Like Service account should've had annotation but doesn't then it qualifies for "fall through". Observe we don't do any "updates" here in this section rather just compare it and do the updates in "default" section
There was a problem hiding this comment.
Also- its a duplicate code here and default section
| shouldManageSa, saName := utils.ParseIRSAAnnotation(ctx, iamRole) | ||
|
|
||
| // Get a RBAC object we can use to modify the ServiceAccount if we need to. | ||
| rbac, err := k8s.NewK8sClient() |
There was a problem hiding this comment.
We probably shouldn't do this since there are users who doesn't use IRSA and getting the client at the front doesn't like a good idea. We should get the client only if it is needed- For example: If shouldManageSa -> get the reusable client so you can use it to get the service account for the comparison and update the service account at the end if needed
| currentVal, _ := sa.Annotations[constants.ServiceAccountRoleAnnotation] | ||
|
|
||
| // Does it match? If so, we're done here. Return. | ||
| if currentVal == req.IamRoleARN { |
There was a problem hiding this comment.
How are you guys using IRSA? Can you have multiple iam roles per namespace?
Scenario:
- IamRole1 created with IRSA annotation serviceaccount -> sa-namespace
- Pods with sa-namespace now have IamRole1 access
- IamRole2 create with IRSA annotation with same service account name sa-namespace
- This code will overwrite with IamRole2 and pods in step2 will lose the access to IamRole1
Our use case is one iam role per namespace so we probably wound't run into this issue but we have to think more here than just updating with new iam role ARN. (Thinking out loud) We either have to decline iamrole creation request stating that Service account is already associated with other iam role. Let me know if you have any ideas?
| r.Recorder.Event(&iamRole, v1.EventTypeWarning, string(iammanagerv1alpha1.Error), "unable to delete the role due to "+err.Error()) | ||
|
|
||
| return ctrl.Result{RequeueAfter: 30 * time.Second}, nil | ||
| if roleName != "" { |
There was a problem hiding this comment.
Now, That i'm checking more on IRSA- we might have to clean up the service account too to make sure we no longer have "deleted iam role" annotation to the service account
|
@mnkg561 Hey.. I'm circling back here. We've been running this PR in in production for 6+ months and it's pretty good and stable. Do you think you'd be willing to finish whatever is necessary to get it over the hump and merge it in? Or give me some explicit guidelines on what you think needs to be finished? |
close #86
What did I want?
The original code only created the
ServiceAccountresource and set theeks.amazonaws.com/role-arnannotation once - at creation time. If that failed for any reason, or became out of date, the controller did not fix it for you. The controllers job should be to constantly ensure that the desired state of the world is correct, including the ServiceAccount link.What did I do?
A few things.. primarily I added in the "create/update service account" into the handler for state=ready. While I was here, I wanted to introduce some logic to get the current state of the ServiceAccount so that we can make a decision as to whether or not we need to apply a change or not. Finally, in order to do that cleanly, I moved some constants around because there were package import issues.
Why the new constants package?
The original constants were stored in
internal/config.. butinternal/config/config.godepends onk8s/client.go. That made it impossible to import a constant into thek8spackage. I have pulled the true global constants out of theinternal/configpackage and put them into a truly isolatedconstantspackage.Whats up with the
EnsureServiceAccountfunction?The existing
CreateOrUpdateServiceAccountfunction is good in that it is fairly simple. I wanted to add some logic to prevent making create/update calls if they are unnecessary, and I did not want to introduce that complexity into the existing function.