Skip to content

Commit 36bae8d

Browse files
committed
Add additive managed-policy reconciliation mode for Role
Introduce an opt-in annotation on the Role resource: iam.services.k8s.aws/policy-reconciliation-mode: additive When set, the controller attaches the managed policies listed in Spec.Policies but never detaches managed policies that were attached out-of-band (e.g. by a RolePolicyAttachment resource, or by a role shared across clusters/accounts). The default behavior (annotation absent) remains authoritative and unchanged, so existing Role manifests are fully backward compatible. - hooks.go: add isAdditivePolicyMode() + comparePolicies(); skip the detach set in syncManagedPolicies when additive. - generator.yaml: mark Spec.Policies compare is_ignored so comparison is delegated to comparePolicies() in customPreCompare (mode-aware). - delta.go: drop the generated Spec.Policies comparison accordingly. - delete path (template + sdk.go): clear the annotation on the delete copy so DeleteRole still performs a full detach (IAM requires no attached managed policies before deletion). - delta_test.go: unit tests for authoritative vs additive comparison. - e2e: test_role_additive_policies.py verifies out-of-band policies are not detached, new desired policies are still attached, and the resource stays synced (no drift loop).
1 parent 97b89f0 commit 36bae8d

7 files changed

Lines changed: 355 additions & 7 deletions

File tree

generator.yaml

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -235,11 +235,17 @@ resources:
235235
# In order to support attaching zero or more policies to a role, we use
236236
# custom update code path code that uses the Attach/DetachRolePolicy API
237237
# calls to manage the set of PolicyARNs attached to this Role.
238+
#
239+
# Comparison is delegated to comparePolicies() in customPreCompare so that
240+
# it can honor the additive policy-reconciliation mode annotation, where
241+
# managed policies attached out-of-band are never treated as drift.
238242
Policies:
239243
type: "[]*string"
240244
references:
241245
resource: Policy
242246
path: Status.ACKResourceMetadata.ARN
247+
compare:
248+
is_ignored: true
243249
# These are policy documents that are added to the Role using the
244250
# Put/DeleteRolePolicy APIs, as compared to the Attach/DetachRolePolicy
245251
# APIs that are for non-inline managed policies.

pkg/resource/role/delta.go

Lines changed: 0 additions & 7 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

pkg/resource/role/delta_test.go

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -419,3 +419,105 @@ func TestNewResourceDelta_AssumeRolePolicyDocument(t *testing.T) {
419419
})
420420
}
421421
}
422+
423+
// roleWithManagedPolicies builds a *resource with the supplied managed policy
424+
// ARNs and, optionally, the additive policy-reconciliation mode annotation.
425+
func roleWithManagedPolicies(additive bool, arns ...string) *resource {
426+
policies := make([]*string, 0, len(arns))
427+
for _, a := range arns {
428+
policies = append(policies, aws.String(a))
429+
}
430+
r := &resource{
431+
ko: &svcapitypes.Role{
432+
Spec: svcapitypes.RoleSpec{
433+
Name: aws.String("test-role"),
434+
Policies: policies,
435+
},
436+
},
437+
}
438+
if additive {
439+
r.ko.ObjectMeta.Annotations = map[string]string{
440+
policyReconcileModeAnnotation: "additive",
441+
}
442+
}
443+
return r
444+
}
445+
446+
func TestNewResourceDelta_ManagedPolicies(t *testing.T) {
447+
const (
448+
arnA = "arn:aws:iam::aws:policy/A"
449+
arnB = "arn:aws:iam::aws:policy/B"
450+
arnC = "arn:aws:iam::aws:policy/C"
451+
)
452+
tests := []struct {
453+
name string
454+
desiredAdditiv bool
455+
desired []string
456+
latest []string
457+
wantDiff bool
458+
}{
459+
{
460+
name: "authoritative: identical sets produce no diff",
461+
desired: []string{arnA, arnB},
462+
latest: []string{arnA, arnB},
463+
wantDiff: false,
464+
},
465+
{
466+
name: "authoritative: extra attached policy produces a diff",
467+
desired: []string{arnA},
468+
latest: []string{arnA, arnB},
469+
wantDiff: true,
470+
},
471+
{
472+
name: "authoritative: missing desired policy produces a diff",
473+
desired: []string{arnA, arnB},
474+
latest: []string{arnA},
475+
wantDiff: true,
476+
},
477+
{
478+
name: "additive: identical sets produce no diff",
479+
desiredAdditiv: true,
480+
desired: []string{arnA, arnB},
481+
latest: []string{arnA, arnB},
482+
wantDiff: false,
483+
},
484+
{
485+
name: "additive: extra out-of-band policy produces no diff",
486+
desiredAdditiv: true,
487+
desired: []string{arnA},
488+
latest: []string{arnA, arnB, arnC},
489+
wantDiff: false,
490+
},
491+
{
492+
name: "additive: missing desired policy still produces a diff",
493+
desiredAdditiv: true,
494+
desired: []string{arnA, arnB},
495+
latest: []string{arnA},
496+
wantDiff: true,
497+
},
498+
{
499+
name: "additive: empty desired never drifts against attached policies",
500+
desiredAdditiv: true,
501+
desired: []string{},
502+
latest: []string{arnA, arnB},
503+
wantDiff: false,
504+
},
505+
}
506+
507+
for _, tc := range tests {
508+
t.Run(tc.name, func(t *testing.T) {
509+
a := roleWithManagedPolicies(tc.desiredAdditiv, tc.desired...)
510+
b := roleWithManagedPolicies(false, tc.latest...)
511+
512+
delta := newResourceDelta(a, b)
513+
514+
if tc.wantDiff {
515+
assert.True(t, delta.DifferentAt("Spec.Policies"),
516+
"expected a diff at Spec.Policies but got none")
517+
} else {
518+
assert.False(t, delta.DifferentAt("Spec.Policies"),
519+
"expected no diff at Spec.Policies but got one")
520+
}
521+
})
522+
}
523+
}

pkg/resource/role/hooks.go

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ package role
1616
import (
1717
"context"
1818
"net/url"
19+
"strings"
1920

2021
ackcompare "github.com/aws-controllers-k8s/runtime/pkg/compare"
2122
ackrtlog "github.com/aws-controllers-k8s/runtime/pkg/runtime/log"
@@ -93,6 +94,7 @@ func (rm *resourceManager) syncManagedPolicies(
9394
toDelete := []*string{}
9495

9596
existingPolicies := latest.ko.Spec.Policies
97+
additive := isAdditivePolicyMode(desired)
9698

9799
for _, p := range desired.ko.Spec.Policies {
98100
if !ackutil.InStringPs(*p, existingPolicies) {
@@ -102,6 +104,14 @@ func (rm *resourceManager) syncManagedPolicies(
102104

103105
for _, p := range existingPolicies {
104106
if !ackutil.InStringPs(*p, desired.ko.Spec.Policies) {
107+
if additive {
108+
// In additive mode the controller never detaches managed
109+
// policies that were attached out-of-band (for example by a
110+
// RolePolicyAttachment resource or another cluster/account).
111+
// The delete path clears the mode annotation so that role
112+
// teardown still performs a full detach.
113+
continue
114+
}
105115
toDelete = append(toDelete, p)
106116
}
107117
}
@@ -373,6 +383,57 @@ func customPreCompare(
373383
b *resource,
374384
) {
375385
compareTags(delta, a, b)
386+
comparePolicies(delta, a, b)
387+
}
388+
389+
// policyReconcileModeAnnotation, when set to "additive" on a Role, tells the
390+
// controller to attach the managed policies listed in Spec.Policies without
391+
// ever detaching managed policies that are attached out-of-band (for example
392+
// by a RolePolicyAttachment resource or a role shared across clusters or
393+
// accounts). Any other value, or the annotation being absent, preserves the
394+
// default "authoritative" behavior where Spec.Policies is the exact desired set
395+
// and any drift is removed.
396+
const policyReconcileModeAnnotation = "iam.services.k8s.aws/policy-reconciliation-mode"
397+
398+
// isAdditivePolicyMode returns true when the supplied Role has opted into
399+
// additive managed-policy reconciliation via annotation.
400+
func isAdditivePolicyMode(r *resource) bool {
401+
if r == nil || r.ko == nil {
402+
return false
403+
}
404+
v, ok := r.ko.ObjectMeta.Annotations[policyReconcileModeAnnotation]
405+
return ok && strings.EqualFold(strings.TrimSpace(v), "additive")
406+
}
407+
408+
// comparePolicies compares the desired (a) and latest (b) managed policy sets.
409+
//
410+
// This replaces the generated Spec.Policies comparison (which is marked
411+
// is_ignored in generator.yaml) so that comparison can be mode-aware. In the
412+
// default authoritative mode it reproduces the generated exact-set comparison.
413+
// In additive mode only missing desired policies are reported as drift; extra
414+
// policies attached out-of-band are ignored so the controller never detaches
415+
// them and does not requeue endlessly.
416+
func comparePolicies(
417+
delta *ackcompare.Delta,
418+
a *resource,
419+
b *resource,
420+
) {
421+
if isAdditivePolicyMode(a) {
422+
for _, p := range a.ko.Spec.Policies {
423+
if !ackutil.InStringPs(*p, b.ko.Spec.Policies) {
424+
delta.Add("Spec.Policies", a.ko.Spec.Policies, b.ko.Spec.Policies)
425+
return
426+
}
427+
}
428+
return
429+
}
430+
if len(a.ko.Spec.Policies) != len(b.ko.Spec.Policies) {
431+
delta.Add("Spec.Policies", a.ko.Spec.Policies, b.ko.Spec.Policies)
432+
} else if len(a.ko.Spec.Policies) > 0 {
433+
if !ackcompare.SliceStringPEqual(a.ko.Spec.Policies, b.ko.Spec.Policies) {
434+
delta.Add("Spec.Policies", a.ko.Spec.Policies, b.ko.Spec.Policies)
435+
}
436+
}
376437
}
377438

378439
// compareTags is a custom comparison function for comparing lists of Tag

pkg/resource/role/sdk.go

Lines changed: 7 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

templates/hooks/role/sdk_delete_pre_build_request.go.tpl

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,12 @@
11
// This deletes all associated managed and inline policies from the role
22
roleCpy := r.ko.DeepCopy()
3+
// Clear the additive policy-reconciliation annotation on the copy so that
4+
// syncManagedPolicies performs a full detach of every attached managed
5+
// policy. IAM requires a role to have no attached managed policies before
6+
// DeleteRole succeeds, so teardown must always be authoritative.
7+
if roleCpy.ObjectMeta.Annotations != nil {
8+
delete(roleCpy.ObjectMeta.Annotations, policyReconcileModeAnnotation)
9+
}
310
roleCpy.Spec.Policies = nil
411
if err := rm.syncManagedPolicies(ctx, &resource{ko: roleCpy}, r); err != nil {
512
return nil, err

0 commit comments

Comments
 (0)