platform/api/aws: add additional checks in ensureInstanceProfile #1058
Conversation
Adds some additional checks in the `ensureInstanceProfile` function that not only does the instance profile exist but it has the correct role (with the correct policies) attached to it.
|
LGTM but I'm no golang programmer :) |
| return fmt.Errorf("getting instance profile %q: %v", name, err) | ||
| } else if err == nil { |
There was a problem hiding this comment.
Can't we just fold back this whole block with the rest of the logic below and just be more idempotent? E.g.:
- CreateInstanceProfile, ignore EntityAlreadyExists
- CreateRole, ignore EntityAlreadyExists
- PutRolePolicy (seems like that API is already idempotent)
- AddRoleToInstanceProfile, ignore EntityAlreadyExists
There was a problem hiding this comment.
I do like the approach, the main deviation from what you have listed is AddRoleToInstanceProfile, in testing this is actually not returning an EntityAlreadyExists but rather a LimitExceeded because the default limit for InstanceSessionsPerInstanceProfile is 1 (essentially each instance profile can only have one role).
I'm not sure I feel that error is sufficient towards ensuring that the correct Role is attached to the InstanceProfile. Perhaps I'll just add a check there in addition to ignoring the EntityAlreadyExists errors on CreateInstanceProfile & CreateRole
|
Closing as this is extremely old |
|
no longer useful? |
|
@dustymabe not necessarily, the bug is still there but it's an extreme corner case that users aren't likely to hit. I still have the branch on my fork if we decide to go back in the future to fix it. |
Adds some additional checks in the
ensureInstanceProfilefunction thatnot only does the instance profile exist but it has the correct role
(with the correct policies) attached to it.
Closes #1047