fix: attach policies to Role on create
#136
Conversation
There was a problem hiding this comment.
thanks @michaelhtm! should we be calling the syncManagedPolicies instead?
pkg/resource/role/sdk.go
Outdated
| err := rm.addManagedPolicy(ctx, &resource{ko}, p) | ||
| if err != nil { | ||
| return &resource{ko}, err | ||
| } |
There was a problem hiding this comment.
can we use the sync function here?
There was a problem hiding this comment.
sync would require ko.Spec.Policies to be nil, so that we can add all the Policies.
I can set it to nil, call syncPolicies, and then set it back to the ko.Spec.Policies = desired.ko.Spec.Policies
Is there a reason we must call sync?
There was a problem hiding this comment.
Not a requirement, but now we have two ways I think I used a different function for EC2 and @a-hilaly uses the one sync in lambda. We should probably choose a way.
| if err != nil { | ||
| return &resource{ko}, err | ||
| } | ||
| } |
michaelhtm
changed the title
michaelhtm
changed the title
policies to Role on create
michaelhtm
changed the title
policies to Role on createpolicies to Role on create
| ko.Spec.AssumeRolePolicyDocument = &doc | ||
| } | ||
| } | ||
| for _, p := range desired.ko.Spec.Policies { | ||
| err := rm.addManagedPolicy(ctx, &resource{ko}, p) | ||
| if err != nil { | ||
| return &resource{ko}, err | ||
| } | ||
| } | ||
| for n, p := range desired.ko.Spec.InlinePolicies { | ||
| err := rm.addInlinePolicy(ctx, &resource{ko}, n, p) | ||
| if err != nil { | ||
| return &resource{ko}, err | ||
| } | ||
| } | ||
| ackcondition.SetSynced(&resource{ko}, corev1.ConditionFalse, nil, nil) |
There was a problem hiding this comment.
nit: we can move it into a hook, just a suggestion.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: michaelhtm, rushmash91 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 |
michaelhtm
force-pushed
the
fix/addpoliciesoncreate
branch
from
9dbe271 to
589da29
Compare
|
@michaelhtm: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
Description of changes:
Currently the controller attaches policies to Role 30 seconds after
creating them.
These changes attach the policies immediately after Role creation.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.