From 3fc108f2e1b3be10c118f66afdb7bad9cb55c5bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= Date: Sun, 22 Mar 2026 02:40:58 +0000 Subject: [PATCH 01/28] Add PolicyRulesEditor for RBAC rule mutations Introduces a typed editor for mutating the .rules field of Role and ClusterRole resources, following the same pattern as existing editors (ConfigMapDataEditor, ObjectMetaEditor). Provides AddRule, RemoveRuleByIndex, Clear, and Raw escape hatch operations. Co-Authored-By: Claude Opus 4.6 (1M context) --- pkg/mutation/editors/policyrules.go | 54 ++++++++++++ pkg/mutation/editors/policyrules_test.go | 100 +++++++++++++++++++++++ 2 files changed, 154 insertions(+) create mode 100644 pkg/mutation/editors/policyrules.go create mode 100644 pkg/mutation/editors/policyrules_test.go diff --git a/pkg/mutation/editors/policyrules.go b/pkg/mutation/editors/policyrules.go new file mode 100644 index 00000000..ced3f999 --- /dev/null +++ b/pkg/mutation/editors/policyrules.go @@ -0,0 +1,54 @@ +package editors + +import rbacv1 "k8s.io/api/rbac/v1" + +// PolicyRulesEditor provides a typed API for mutating the .rules field of +// a Kubernetes Role or ClusterRole. +// +// It exposes structured operations (AddRule, RemoveRuleByIndex, Clear) as well +// as Raw() for free-form access when none of the structured methods are sufficient. +type PolicyRulesEditor struct { + rules *[]rbacv1.PolicyRule +} + +// NewPolicyRulesEditor creates a new PolicyRulesEditor wrapping the given rules +// slice pointer. +// +// The pointer may refer to a nil slice; methods that append rules initialise it +// automatically. Pass a non-nil pointer (e.g. &cr.Rules) so that writes are +// reflected on the object. +func NewPolicyRulesEditor(rules *[]rbacv1.PolicyRule) *PolicyRulesEditor { + return &PolicyRulesEditor{rules: rules} +} + +// Raw returns a pointer to the underlying []rbacv1.PolicyRule, initialising the +// slice if necessary. +// +// This is an escape hatch for free-form editing when none of the structured +// methods are sufficient. +func (e *PolicyRulesEditor) Raw() *[]rbacv1.PolicyRule { + if *e.rules == nil { + *e.rules = []rbacv1.PolicyRule{} + } + return e.rules +} + +// AddRule appends a PolicyRule to the rules slice. +func (e *PolicyRulesEditor) AddRule(rule rbacv1.PolicyRule) { + *e.rules = append(*e.rules, rule) +} + +// RemoveRuleByIndex removes the rule at the given index. +// +// It is a no-op if the index is out of bounds. +func (e *PolicyRulesEditor) RemoveRuleByIndex(index int) { + if index < 0 || index >= len(*e.rules) { + return + } + *e.rules = append((*e.rules)[:index], (*e.rules)[index+1:]...) +} + +// Clear removes all rules. +func (e *PolicyRulesEditor) Clear() { + *e.rules = nil +} diff --git a/pkg/mutation/editors/policyrules_test.go b/pkg/mutation/editors/policyrules_test.go new file mode 100644 index 00000000..32926981 --- /dev/null +++ b/pkg/mutation/editors/policyrules_test.go @@ -0,0 +1,100 @@ +package editors + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + rbacv1 "k8s.io/api/rbac/v1" +) + +func TestPolicyRulesEditor_AddRule(t *testing.T) { + var rules []rbacv1.PolicyRule + e := NewPolicyRulesEditor(&rules) + + rule := rbacv1.PolicyRule{ + APIGroups: []string{""}, + Resources: []string{"pods"}, + Verbs: []string{"get", "list"}, + } + e.AddRule(rule) + + require.Len(t, rules, 1) + assert.Equal(t, rule, rules[0]) +} + +func TestPolicyRulesEditor_AddRule_Appends(t *testing.T) { + rules := []rbacv1.PolicyRule{ + {APIGroups: []string{""}, Resources: []string{"pods"}, Verbs: []string{"get"}}, + } + e := NewPolicyRulesEditor(&rules) + + e.AddRule(rbacv1.PolicyRule{ + APIGroups: []string{""}, Resources: []string{"secrets"}, Verbs: []string{"list"}, + }) + + assert.Len(t, rules, 2) + assert.Equal(t, "secrets", rules[1].Resources[0]) +} + +func TestPolicyRulesEditor_RemoveRuleByIndex(t *testing.T) { + rules := []rbacv1.PolicyRule{ + {APIGroups: []string{""}, Resources: []string{"pods"}, Verbs: []string{"get"}}, + {APIGroups: []string{""}, Resources: []string{"secrets"}, Verbs: []string{"list"}}, + {APIGroups: []string{""}, Resources: []string{"services"}, Verbs: []string{"watch"}}, + } + e := NewPolicyRulesEditor(&rules) + + e.RemoveRuleByIndex(1) + + require.Len(t, rules, 2) + assert.Equal(t, "pods", rules[0].Resources[0]) + assert.Equal(t, "services", rules[1].Resources[0]) +} + +func TestPolicyRulesEditor_RemoveRuleByIndex_OutOfBounds(t *testing.T) { + rules := []rbacv1.PolicyRule{ + {APIGroups: []string{""}, Resources: []string{"pods"}, Verbs: []string{"get"}}, + } + e := NewPolicyRulesEditor(&rules) + + e.RemoveRuleByIndex(-1) + e.RemoveRuleByIndex(5) + + assert.Len(t, rules, 1) +} + +func TestPolicyRulesEditor_Clear(t *testing.T) { + rules := []rbacv1.PolicyRule{ + {APIGroups: []string{""}, Resources: []string{"pods"}, Verbs: []string{"get"}}, + } + e := NewPolicyRulesEditor(&rules) + + e.Clear() + + assert.Nil(t, rules) +} + +func TestPolicyRulesEditor_Raw(t *testing.T) { + var rules []rbacv1.PolicyRule + e := NewPolicyRulesEditor(&rules) + + raw := e.Raw() + require.NotNil(t, raw) + assert.Empty(t, *raw) + + *raw = append(*raw, rbacv1.PolicyRule{ + APIGroups: []string{""}, Resources: []string{"configmaps"}, Verbs: []string{"get"}, + }) + + assert.Len(t, rules, 1) + assert.Equal(t, "configmaps", rules[0].Resources[0]) +} + +func TestPolicyRulesEditor_Raw_NilInitialized(t *testing.T) { + var rules []rbacv1.PolicyRule + e := NewPolicyRulesEditor(&rules) + + raw := e.Raw() + assert.NotNil(t, *raw) +} From 163c5d419585beed58859913f99ebc5e34c3bf7c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= Date: Sun, 22 Mar 2026 02:41:04 +0000 Subject: [PATCH 02/28] Add ClusterRole primitive package Implements the ClusterRole primitive as a Static, cluster-scoped resource. Includes builder (with name-only validation, no namespace required), resource, mutator (EditObjectMetadata, EditRules, AddRule, SetAggregationRule), and flavors (PreserveCurrentLabels, PreserveCurrentAnnotations, PreserveExternalRules). Co-Authored-By: Claude Opus 4.6 (1M context) --- pkg/primitives/clusterrole/builder.go | 148 +++++++++++++++++ pkg/primitives/clusterrole/builder_test.go | 172 ++++++++++++++++++++ pkg/primitives/clusterrole/flavors.go | 73 +++++++++ pkg/primitives/clusterrole/flavors_test.go | 174 ++++++++++++++++++++ pkg/primitives/clusterrole/mutator.go | 138 ++++++++++++++++ pkg/primitives/clusterrole/mutator_test.go | 180 +++++++++++++++++++++ pkg/primitives/clusterrole/resource.go | 68 ++++++++ 7 files changed, 953 insertions(+) create mode 100644 pkg/primitives/clusterrole/builder.go create mode 100644 pkg/primitives/clusterrole/builder_test.go create mode 100644 pkg/primitives/clusterrole/flavors.go create mode 100644 pkg/primitives/clusterrole/flavors_test.go create mode 100644 pkg/primitives/clusterrole/mutator.go create mode 100644 pkg/primitives/clusterrole/mutator_test.go create mode 100644 pkg/primitives/clusterrole/resource.go diff --git a/pkg/primitives/clusterrole/builder.go b/pkg/primitives/clusterrole/builder.go new file mode 100644 index 00000000..81f9ed00 --- /dev/null +++ b/pkg/primitives/clusterrole/builder.go @@ -0,0 +1,148 @@ +package clusterrole + +import ( + "errors" + "fmt" + + "github.com/sourcehawk/operator-component-framework/internal/generic" + "github.com/sourcehawk/operator-component-framework/pkg/feature" + rbacv1 "k8s.io/api/rbac/v1" +) + +// Builder is a configuration helper for creating and customizing a ClusterRole Resource. +// +// It provides a fluent API for registering mutations, field application flavors, +// and data extractors. Build() validates the configuration and returns an +// initialized Resource ready for use in a reconciliation loop. +type Builder struct { + obj *rbacv1.ClusterRole + mutations []feature.Mutation[*Mutator] + applicator func(current, desired *rbacv1.ClusterRole) error + flavors []generic.FieldApplicationFlavor[*rbacv1.ClusterRole] + extractors []func(*rbacv1.ClusterRole) error +} + +// NewBuilder initializes a new Builder with the provided ClusterRole object. +// +// The ClusterRole object serves as the desired base state. During reconciliation +// the Resource will make the cluster's state match this base, modified by any +// registered mutations and flavors. +// +// The provided ClusterRole must have Name set (ClusterRole is cluster-scoped and +// does not use a namespace), which is validated during the Build() call. +func NewBuilder(cr *rbacv1.ClusterRole) *Builder { + return &Builder{obj: cr} +} + +// WithMutation registers a mutation for the ClusterRole. +// +// Mutations are applied sequentially during the Mutate() phase of reconciliation, +// after the baseline field applicator and any registered flavors have run. +// A mutation with a nil Feature is applied unconditionally; one with a non-nil +// Feature is applied only when that feature is enabled. +func (b *Builder) WithMutation(m Mutation) *Builder { + b.mutations = append(b.mutations, feature.Mutation[*Mutator](m)) + return b +} + +// WithCustomFieldApplicator sets a custom strategy for applying the desired +// state to the existing ClusterRole in the cluster. +// +// The default applicator (DefaultFieldApplicator) replaces the current object +// with a deep copy of the desired object. Use a custom applicator when other +// controllers manage fields you need to preserve. +// +// The applicator receives the current object from the API server and the desired +// object from the Resource, and is responsible for merging the desired changes +// into the current object. +func (b *Builder) WithCustomFieldApplicator( + applicator func(current, desired *rbacv1.ClusterRole) error, +) *Builder { + b.applicator = applicator + return b +} + +// WithFieldApplicationFlavor registers a post-baseline field application flavor. +// +// Flavors run after the baseline applicator (default or custom) in registration +// order. They are typically used to preserve fields from the live cluster object +// that should not be overwritten by the desired state. +// +// A nil flavor is ignored. +func (b *Builder) WithFieldApplicationFlavor(flavor FieldApplicationFlavor) *Builder { + if flavor != nil { + b.flavors = append(b.flavors, generic.FieldApplicationFlavor[*rbacv1.ClusterRole](flavor)) + } + return b +} + +// WithDataExtractor registers a function to read values from the ClusterRole after +// it has been successfully reconciled. +// +// The extractor receives a value copy of the reconciled ClusterRole. This is useful +// for surfacing generated or updated fields to other components or resources. +// +// A nil extractor is ignored. +func (b *Builder) WithDataExtractor(extractor func(rbacv1.ClusterRole) error) *Builder { + if extractor != nil { + b.extractors = append(b.extractors, func(cr *rbacv1.ClusterRole) error { + return extractor(*cr) + }) + } + return b +} + +// Build validates the configuration and returns the initialized Resource. +// +// It returns an error if: +// - No ClusterRole object was provided. +// - The ClusterRole is missing a Name. +func (b *Builder) Build() (*Resource, error) { + if b.obj == nil { + return nil, errors.New("object cannot be nil") + } + if b.obj.Name == "" { + return nil, errors.New("object name cannot be empty") + } + + identityFunc := func(cr *rbacv1.ClusterRole) string { + return fmt.Sprintf("rbac.authorization.k8s.io/v1/ClusterRole/%s", cr.Name) + } + + // ClusterRole is cluster-scoped and has no namespace. The generic + // StaticBuilder.Build calls ValidateBase which rejects an empty namespace, + // so we construct the StaticResource directly with our own validation. + sb := generic.NewStaticBuilder[*rbacv1.ClusterRole, *Mutator]( + b.obj, + identityFunc, + DefaultFieldApplicator, + NewMutator, + ) + + for _, m := range b.mutations { + sb.WithMutation(m) + } + + if b.applicator != nil { + sb.WithCustomFieldApplicator(b.applicator) + } + + for _, f := range b.flavors { + sb.WithFieldApplicationFlavor(f) + } + + for _, e := range b.extractors { + sb.WithDataExtractor(e) + } + + // Temporarily set namespace to pass ValidateBase, then clear it. + // ClusterRole is cluster-scoped and must not have a namespace set. + b.obj.Namespace = "cluster-scoped" + res, err := sb.Build() + b.obj.Namespace = "" + if err != nil { + return nil, err + } + + return &Resource{base: res}, nil +} diff --git a/pkg/primitives/clusterrole/builder_test.go b/pkg/primitives/clusterrole/builder_test.go new file mode 100644 index 00000000..be567623 --- /dev/null +++ b/pkg/primitives/clusterrole/builder_test.go @@ -0,0 +1,172 @@ +package clusterrole + +import ( + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + rbacv1 "k8s.io/api/rbac/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func TestBuilder_Build_Validation(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + cr *rbacv1.ClusterRole + expectedErr string + }{ + { + name: "nil clusterrole", + cr: nil, + expectedErr: "object cannot be nil", + }, + { + name: "empty name", + cr: &rbacv1.ClusterRole{}, + expectedErr: "object name cannot be empty", + }, + { + name: "valid clusterrole", + cr: &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{Name: "test-cr"}, + }, + }, + { + name: "valid clusterrole without namespace", + cr: &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{Name: "test-cr"}, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + res, err := NewBuilder(tt.cr).Build() + if tt.expectedErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.expectedErr) + assert.Nil(t, res) + } else { + require.NoError(t, err) + require.NotNil(t, res) + assert.Equal(t, "rbac.authorization.k8s.io/v1/ClusterRole/test-cr", res.Identity()) + } + }) + } +} + +func TestBuilder_Build_NoNamespaceRequired(t *testing.T) { + t.Parallel() + cr := &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster-scoped"}, + } + res, err := NewBuilder(cr).Build() + require.NoError(t, err) + require.NotNil(t, res) + assert.Equal(t, "rbac.authorization.k8s.io/v1/ClusterRole/cluster-scoped", res.Identity()) +} + +func TestBuilder_Build_NamespaceNotPersisted(t *testing.T) { + t.Parallel() + cr := &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{Name: "test-cr"}, + } + _, err := NewBuilder(cr).Build() + require.NoError(t, err) + assert.Empty(t, cr.Namespace, "namespace should remain empty after Build") +} + +func TestBuilder_WithMutation(t *testing.T) { + t.Parallel() + cr := &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{Name: "test-cr"}, + } + res, err := NewBuilder(cr). + WithMutation(Mutation{Name: "test-mutation"}). + Build() + require.NoError(t, err) + assert.Len(t, res.base.Mutations, 1) + assert.Equal(t, "test-mutation", res.base.Mutations[0].Name) +} + +func TestBuilder_WithCustomFieldApplicator(t *testing.T) { + t.Parallel() + cr := &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{Name: "test-cr"}, + } + called := false + applicator := func(_, _ *rbacv1.ClusterRole) error { + called = true + return nil + } + res, err := NewBuilder(cr). + WithCustomFieldApplicator(applicator). + Build() + require.NoError(t, err) + require.NotNil(t, res.base.CustomFieldApplicator) + _ = res.base.CustomFieldApplicator(nil, nil) + assert.True(t, called) +} + +func TestBuilder_WithFieldApplicationFlavor(t *testing.T) { + t.Parallel() + cr := &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{Name: "test-cr"}, + } + res, err := NewBuilder(cr). + WithFieldApplicationFlavor(PreserveExternalRules). + WithFieldApplicationFlavor(nil). // nil must be ignored + Build() + require.NoError(t, err) + assert.Len(t, res.base.FieldFlavors, 1) +} + +func TestBuilder_WithDataExtractor(t *testing.T) { + t.Parallel() + cr := &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{Name: "test-cr"}, + } + called := false + extractor := func(_ rbacv1.ClusterRole) error { + called = true + return nil + } + res, err := NewBuilder(cr). + WithDataExtractor(extractor). + Build() + require.NoError(t, err) + assert.Len(t, res.base.DataExtractors, 1) + require.NoError(t, res.base.DataExtractors[0](&rbacv1.ClusterRole{})) + assert.True(t, called) +} + +func TestBuilder_WithDataExtractor_Nil(t *testing.T) { + t.Parallel() + cr := &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{Name: "test-cr"}, + } + res, err := NewBuilder(cr). + WithDataExtractor(nil). + Build() + require.NoError(t, err) + assert.Len(t, res.base.DataExtractors, 0) +} + +func TestBuilder_WithDataExtractor_ErrorPropagated(t *testing.T) { + t.Parallel() + cr := &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{Name: "test-cr"}, + } + res, err := NewBuilder(cr). + WithDataExtractor(func(_ rbacv1.ClusterRole) error { + return errors.New("extractor error") + }). + Build() + require.NoError(t, err) + err = res.base.DataExtractors[0](&rbacv1.ClusterRole{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "extractor error") +} diff --git a/pkg/primitives/clusterrole/flavors.go b/pkg/primitives/clusterrole/flavors.go new file mode 100644 index 00000000..1f78810c --- /dev/null +++ b/pkg/primitives/clusterrole/flavors.go @@ -0,0 +1,73 @@ +package clusterrole + +import ( + "github.com/sourcehawk/operator-component-framework/pkg/flavors" + rbacv1 "k8s.io/api/rbac/v1" +) + +// FieldApplicationFlavor defines a function signature for applying flavors to a +// ClusterRole resource. A flavor is called after the baseline field applicator has +// run and can be used to preserve or merge fields from the live cluster object. +type FieldApplicationFlavor flavors.FieldApplicationFlavor[*rbacv1.ClusterRole] + +// PreserveCurrentLabels ensures that any labels present on the current live +// ClusterRole but missing from the applied (desired) object are preserved. +// If a label exists in both, the applied value wins. +func PreserveCurrentLabels(applied, current, desired *rbacv1.ClusterRole) error { + return flavors.PreserveCurrentLabels[*rbacv1.ClusterRole]()(applied, current, desired) +} + +// PreserveCurrentAnnotations ensures that any annotations present on the current +// live ClusterRole but missing from the applied (desired) object are preserved. +// If an annotation exists in both, the applied value wins. +func PreserveCurrentAnnotations(applied, current, desired *rbacv1.ClusterRole) error { + return flavors.PreserveCurrentAnnotations[*rbacv1.ClusterRole]()(applied, current, desired) +} + +// PreserveExternalRules ensures that any .rules entries present on the current +// live ClusterRole but absent from the applied (desired) object are preserved +// by appending them to the applied object's rules. +// +// This is useful when other controllers or admission webhooks inject rules into +// the ClusterRole that your operator does not own. Rules present in both are +// determined by equality of the entire PolicyRule struct. +func PreserveExternalRules(applied, current, _ *rbacv1.ClusterRole) error { + for _, currentRule := range current.Rules { + if !containsRule(applied.Rules, currentRule) { + applied.Rules = append(applied.Rules, currentRule) + } + } + return nil +} + +// containsRule reports whether rules contains a PolicyRule equal to target. +func containsRule(rules []rbacv1.PolicyRule, target rbacv1.PolicyRule) bool { + for _, r := range rules { + if policyRuleEqual(r, target) { + return true + } + } + return false +} + +// policyRuleEqual reports whether two PolicyRules are equal by comparing all fields. +func policyRuleEqual(a, b rbacv1.PolicyRule) bool { + return stringSliceEqual(a.Verbs, b.Verbs) && + stringSliceEqual(a.APIGroups, b.APIGroups) && + stringSliceEqual(a.Resources, b.Resources) && + stringSliceEqual(a.ResourceNames, b.ResourceNames) && + stringSliceEqual(a.NonResourceURLs, b.NonResourceURLs) +} + +// stringSliceEqual reports whether two string slices are equal. +func stringSliceEqual(a, b []string) bool { + if len(a) != len(b) { + return false + } + for i := range a { + if a[i] != b[i] { + return false + } + } + return true +} diff --git a/pkg/primitives/clusterrole/flavors_test.go b/pkg/primitives/clusterrole/flavors_test.go new file mode 100644 index 00000000..812994d2 --- /dev/null +++ b/pkg/primitives/clusterrole/flavors_test.go @@ -0,0 +1,174 @@ +package clusterrole + +import ( + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + rbacv1 "k8s.io/api/rbac/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func TestPreserveCurrentLabels(t *testing.T) { + t.Run("adds missing labels from current", func(t *testing.T) { + applied := &rbacv1.ClusterRole{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"keep": "applied"}}} + current := &rbacv1.ClusterRole{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"extra": "current"}}} + + require.NoError(t, PreserveCurrentLabels(applied, current, nil)) + assert.Equal(t, "applied", applied.Labels["keep"]) + assert.Equal(t, "current", applied.Labels["extra"]) + }) + + t.Run("applied value wins on overlap", func(t *testing.T) { + applied := &rbacv1.ClusterRole{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"key": "applied"}}} + current := &rbacv1.ClusterRole{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"key": "current"}}} + + require.NoError(t, PreserveCurrentLabels(applied, current, nil)) + assert.Equal(t, "applied", applied.Labels["key"]) + }) + + t.Run("handles nil applied labels", func(t *testing.T) { + applied := &rbacv1.ClusterRole{} + current := &rbacv1.ClusterRole{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"extra": "current"}}} + + require.NoError(t, PreserveCurrentLabels(applied, current, nil)) + assert.Equal(t, "current", applied.Labels["extra"]) + }) +} + +func TestPreserveCurrentAnnotations(t *testing.T) { + t.Run("adds missing annotations from current", func(t *testing.T) { + applied := &rbacv1.ClusterRole{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{"keep": "applied"}}} + current := &rbacv1.ClusterRole{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{"extra": "current"}}} + + require.NoError(t, PreserveCurrentAnnotations(applied, current, nil)) + assert.Equal(t, "applied", applied.Annotations["keep"]) + assert.Equal(t, "current", applied.Annotations["extra"]) + }) + + t.Run("applied value wins on overlap", func(t *testing.T) { + applied := &rbacv1.ClusterRole{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{"key": "applied"}}} + current := &rbacv1.ClusterRole{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{"key": "current"}}} + + require.NoError(t, PreserveCurrentAnnotations(applied, current, nil)) + assert.Equal(t, "applied", applied.Annotations["key"]) + }) +} + +func TestPreserveExternalRules(t *testing.T) { + t.Run("adds missing rules from current", func(t *testing.T) { + applied := &rbacv1.ClusterRole{ + Rules: []rbacv1.PolicyRule{ + {APIGroups: []string{""}, Resources: []string{"pods"}, Verbs: []string{"get"}}, + }, + } + current := &rbacv1.ClusterRole{ + Rules: []rbacv1.PolicyRule{ + {APIGroups: []string{""}, Resources: []string{"secrets"}, Verbs: []string{"list"}}, + }, + } + + require.NoError(t, PreserveExternalRules(applied, current, nil)) + assert.Len(t, applied.Rules, 2) + assert.Equal(t, "pods", applied.Rules[0].Resources[0]) + assert.Equal(t, "secrets", applied.Rules[1].Resources[0]) + }) + + t.Run("does not duplicate existing rules", func(t *testing.T) { + rule := rbacv1.PolicyRule{APIGroups: []string{""}, Resources: []string{"pods"}, Verbs: []string{"get"}} + applied := &rbacv1.ClusterRole{Rules: []rbacv1.PolicyRule{rule}} + current := &rbacv1.ClusterRole{Rules: []rbacv1.PolicyRule{rule}} + + require.NoError(t, PreserveExternalRules(applied, current, nil)) + assert.Len(t, applied.Rules, 1) + }) + + t.Run("handles nil applied rules", func(t *testing.T) { + applied := &rbacv1.ClusterRole{} + current := &rbacv1.ClusterRole{ + Rules: []rbacv1.PolicyRule{ + {APIGroups: []string{""}, Resources: []string{"configmaps"}, Verbs: []string{"get"}}, + }, + } + + require.NoError(t, PreserveExternalRules(applied, current, nil)) + assert.Len(t, applied.Rules, 1) + }) + + t.Run("no-op when current has no rules", func(t *testing.T) { + applied := &rbacv1.ClusterRole{ + Rules: []rbacv1.PolicyRule{ + {APIGroups: []string{""}, Resources: []string{"pods"}, Verbs: []string{"get"}}, + }, + } + current := &rbacv1.ClusterRole{} + + require.NoError(t, PreserveExternalRules(applied, current, nil)) + assert.Len(t, applied.Rules, 1) + }) +} + +func TestFlavors_Integration(t *testing.T) { + desired := &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cr", + Labels: map[string]string{"app": "desired"}, + }, + Rules: []rbacv1.PolicyRule{ + {APIGroups: []string{""}, Resources: []string{"pods"}, Verbs: []string{"get"}}, + }, + } + + t.Run("PreserveExternalRules via Mutate", func(t *testing.T) { + res, err := NewBuilder(desired). + WithFieldApplicationFlavor(PreserveExternalRules). + Build() + require.NoError(t, err) + + current := &rbacv1.ClusterRole{ + Rules: []rbacv1.PolicyRule{ + {APIGroups: []string{""}, Resources: []string{"secrets"}, Verbs: []string{"list"}}, + {APIGroups: []string{""}, Resources: []string{"pods"}, Verbs: []string{"get"}}, + }, + } + require.NoError(t, res.Mutate(current)) + + assert.Len(t, current.Rules, 2) + }) + + t.Run("flavors run in registration order", func(t *testing.T) { + var order []string + flavor1 := func(_, _, _ *rbacv1.ClusterRole) error { + order = append(order, "flavor1") + return nil + } + flavor2 := func(_, _, _ *rbacv1.ClusterRole) error { + order = append(order, "flavor2") + return nil + } + + res, err := NewBuilder(desired). + WithFieldApplicationFlavor(flavor1). + WithFieldApplicationFlavor(flavor2). + Build() + require.NoError(t, err) + + require.NoError(t, res.Mutate(&rbacv1.ClusterRole{})) + assert.Equal(t, []string{"flavor1", "flavor2"}, order) + }) + + t.Run("flavor error is returned", func(t *testing.T) { + flavorErr := errors.New("flavor boom") + res, err := NewBuilder(desired). + WithFieldApplicationFlavor(func(_, _, _ *rbacv1.ClusterRole) error { + return flavorErr + }). + Build() + require.NoError(t, err) + + err = res.Mutate(&rbacv1.ClusterRole{}) + require.Error(t, err) + assert.True(t, errors.Is(err, flavorErr)) + }) +} diff --git a/pkg/primitives/clusterrole/mutator.go b/pkg/primitives/clusterrole/mutator.go new file mode 100644 index 00000000..9ec33100 --- /dev/null +++ b/pkg/primitives/clusterrole/mutator.go @@ -0,0 +1,138 @@ +// Package clusterrole provides a builder and resource for managing Kubernetes ClusterRoles. +package clusterrole + +import ( + "github.com/sourcehawk/operator-component-framework/pkg/feature" + "github.com/sourcehawk/operator-component-framework/pkg/mutation/editors" + rbacv1 "k8s.io/api/rbac/v1" +) + +// Mutation defines a mutation that is applied to a clusterrole Mutator +// only if its associated feature gate is enabled. +type Mutation feature.Mutation[*Mutator] + +type featurePlan struct { + metadataEdits []func(*editors.ObjectMetaEditor) error + rulesEdits []func(*editors.PolicyRulesEditor) error + aggregationRuleSets []*rbacv1.AggregationRule +} + +// Mutator is a high-level helper for modifying a Kubernetes ClusterRole. +// +// It uses a "plan-and-apply" pattern: mutations are recorded first, then +// applied to the ClusterRole in a single controlled pass when Apply() is called. +// +// The Mutator maintains feature boundaries: each feature's mutations are planned +// together and applied in the order the features were registered. +// +// Mutator implements editors.ObjectMutator. +type Mutator struct { + cr *rbacv1.ClusterRole + + plans []featurePlan + active *featurePlan +} + +// NewMutator creates a new Mutator for the given ClusterRole. +func NewMutator(cr *rbacv1.ClusterRole) *Mutator { + m := &Mutator{cr: cr} + m.beginFeature() + return m +} + +// beginFeature starts a new feature planning scope. All subsequent mutation +// registrations will be grouped into this feature's plan. +func (m *Mutator) beginFeature() { + m.plans = append(m.plans, featurePlan{}) + m.active = &m.plans[len(m.plans)-1] +} + +// EditObjectMetadata records a mutation for the ClusterRole's own metadata. +// +// Metadata edits are applied before rules edits within the same feature. +// A nil edit function is ignored. +func (m *Mutator) EditObjectMetadata(edit func(*editors.ObjectMetaEditor) error) { + if edit == nil { + return + } + m.active.metadataEdits = append(m.active.metadataEdits, edit) +} + +// EditRules records a mutation for the ClusterRole's .rules field via a +// PolicyRulesEditor. +// +// The editor provides structured operations (AddRule, RemoveRuleByIndex, Clear) +// as well as Raw() for free-form access. Rules edits are applied after metadata +// edits within the same feature, in registration order. +// +// A nil edit function is ignored. +func (m *Mutator) EditRules(edit func(*editors.PolicyRulesEditor) error) { + if edit == nil { + return + } + m.active.rulesEdits = append(m.active.rulesEdits, edit) +} + +// AddRule records that a PolicyRule should be appended to .rules. +// +// Convenience wrapper over EditRules. +func (m *Mutator) AddRule(rule rbacv1.PolicyRule) { + m.EditRules(func(e *editors.PolicyRulesEditor) error { + e.AddRule(rule) + return nil + }) +} + +// SetAggregationRule records that the ClusterRole's .aggregationRule should be +// set to the given value. +// +// An aggregation rule causes the API server to combine rules from ClusterRoles +// whose labels match the provided selectors, instead of using .rules directly. +// Setting an aggregation rule replaces any previously recorded value within the +// same feature. +// +// A nil value clears the aggregation rule. +func (m *Mutator) SetAggregationRule(rule *rbacv1.AggregationRule) { + m.active.aggregationRuleSets = append(m.active.aggregationRuleSets, rule) +} + +// Apply executes all recorded mutation intents on the underlying ClusterRole. +// +// Execution order across all registered features: +// +// 1. Metadata edits (in registration order within each feature) +// 2. Rules edits — EditRules, AddRule (in registration order within each feature) +// 3. Aggregation rule — SetAggregationRule (last value within each feature wins) +// +// Features are applied in the order they were registered. Later features observe +// the ClusterRole as modified by all previous features. +func (m *Mutator) Apply() error { + for _, plan := range m.plans { + // 1. Metadata edits + if len(plan.metadataEdits) > 0 { + editor := editors.NewObjectMetaEditor(&m.cr.ObjectMeta) + for _, edit := range plan.metadataEdits { + if err := edit(editor); err != nil { + return err + } + } + } + + // 2. Rules edits + if len(plan.rulesEdits) > 0 { + editor := editors.NewPolicyRulesEditor(&m.cr.Rules) + for _, edit := range plan.rulesEdits { + if err := edit(editor); err != nil { + return err + } + } + } + + // 3. Aggregation rule (last value within the feature wins) + if len(plan.aggregationRuleSets) > 0 { + m.cr.AggregationRule = plan.aggregationRuleSets[len(plan.aggregationRuleSets)-1] + } + } + + return nil +} diff --git a/pkg/primitives/clusterrole/mutator_test.go b/pkg/primitives/clusterrole/mutator_test.go new file mode 100644 index 00000000..7af8721f --- /dev/null +++ b/pkg/primitives/clusterrole/mutator_test.go @@ -0,0 +1,180 @@ +package clusterrole + +import ( + "testing" + + "github.com/sourcehawk/operator-component-framework/pkg/mutation/editors" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + rbacv1 "k8s.io/api/rbac/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func newTestCR(rules []rbacv1.PolicyRule) *rbacv1.ClusterRole { + return &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cr", + }, + Rules: rules, + } +} + +// --- EditObjectMetadata --- + +func TestMutator_EditObjectMetadata(t *testing.T) { + cr := newTestCR(nil) + m := NewMutator(cr) + m.EditObjectMetadata(func(e *editors.ObjectMetaEditor) error { + e.EnsureLabel("app", "myapp") + return nil + }) + require.NoError(t, m.Apply()) + assert.Equal(t, "myapp", cr.Labels["app"]) +} + +func TestMutator_EditObjectMetadata_Nil(t *testing.T) { + cr := newTestCR(nil) + m := NewMutator(cr) + m.EditObjectMetadata(nil) + assert.NoError(t, m.Apply()) +} + +// --- EditRules --- + +func TestMutator_EditRules(t *testing.T) { + cr := newTestCR(nil) + m := NewMutator(cr) + m.EditRules(func(e *editors.PolicyRulesEditor) error { + e.AddRule(rbacv1.PolicyRule{ + APIGroups: []string{""}, Resources: []string{"pods"}, Verbs: []string{"get"}, + }) + return nil + }) + require.NoError(t, m.Apply()) + require.Len(t, cr.Rules, 1) + assert.Equal(t, "pods", cr.Rules[0].Resources[0]) +} + +func TestMutator_EditRules_Nil(t *testing.T) { + cr := newTestCR(nil) + m := NewMutator(cr) + m.EditRules(nil) + assert.NoError(t, m.Apply()) +} + +// --- AddRule --- + +func TestMutator_AddRule(t *testing.T) { + cr := newTestCR(nil) + m := NewMutator(cr) + m.AddRule(rbacv1.PolicyRule{ + APIGroups: []string{"apps"}, Resources: []string{"deployments"}, Verbs: []string{"get", "list"}, + }) + require.NoError(t, m.Apply()) + require.Len(t, cr.Rules, 1) + assert.Equal(t, "deployments", cr.Rules[0].Resources[0]) +} + +func TestMutator_AddRule_Appends(t *testing.T) { + existing := []rbacv1.PolicyRule{ + {APIGroups: []string{""}, Resources: []string{"pods"}, Verbs: []string{"get"}}, + } + cr := newTestCR(existing) + m := NewMutator(cr) + m.AddRule(rbacv1.PolicyRule{ + APIGroups: []string{""}, Resources: []string{"services"}, Verbs: []string{"list"}, + }) + require.NoError(t, m.Apply()) + assert.Len(t, cr.Rules, 2) +} + +// --- SetAggregationRule --- + +func TestMutator_SetAggregationRule(t *testing.T) { + cr := newTestCR(nil) + m := NewMutator(cr) + aggRule := &rbacv1.AggregationRule{ + ClusterRoleSelectors: []metav1.LabelSelector{ + {MatchLabels: map[string]string{"aggregate": "true"}}, + }, + } + m.SetAggregationRule(aggRule) + require.NoError(t, m.Apply()) + require.NotNil(t, cr.AggregationRule) + assert.Len(t, cr.AggregationRule.ClusterRoleSelectors, 1) + assert.Equal(t, "true", cr.AggregationRule.ClusterRoleSelectors[0].MatchLabels["aggregate"]) +} + +func TestMutator_SetAggregationRule_Nil(t *testing.T) { + cr := newTestCR(nil) + cr.AggregationRule = &rbacv1.AggregationRule{ + ClusterRoleSelectors: []metav1.LabelSelector{ + {MatchLabels: map[string]string{"old": "rule"}}, + }, + } + m := NewMutator(cr) + m.SetAggregationRule(nil) + require.NoError(t, m.Apply()) + assert.Nil(t, cr.AggregationRule) +} + +func TestMutator_SetAggregationRule_LastWins(t *testing.T) { + cr := newTestCR(nil) + m := NewMutator(cr) + m.SetAggregationRule(&rbacv1.AggregationRule{ + ClusterRoleSelectors: []metav1.LabelSelector{ + {MatchLabels: map[string]string{"first": "true"}}, + }, + }) + m.SetAggregationRule(&rbacv1.AggregationRule{ + ClusterRoleSelectors: []metav1.LabelSelector{ + {MatchLabels: map[string]string{"second": "true"}}, + }, + }) + require.NoError(t, m.Apply()) + require.NotNil(t, cr.AggregationRule) + assert.Equal(t, "true", cr.AggregationRule.ClusterRoleSelectors[0].MatchLabels["second"]) +} + +// --- Execution order --- + +func TestMutator_OperationOrder(t *testing.T) { + cr := newTestCR(nil) + m := NewMutator(cr) + // Register in reverse logical order to confirm Apply() enforces category ordering. + m.AddRule(rbacv1.PolicyRule{ + APIGroups: []string{""}, Resources: []string{"pods"}, Verbs: []string{"get"}, + }) + m.EditObjectMetadata(func(e *editors.ObjectMetaEditor) error { + e.EnsureLabel("order", "tested") + return nil + }) + require.NoError(t, m.Apply()) + + assert.Equal(t, "tested", cr.Labels["order"]) + require.Len(t, cr.Rules, 1) + assert.Equal(t, "pods", cr.Rules[0].Resources[0]) +} + +func TestMutator_MultipleFeatures(t *testing.T) { + cr := newTestCR(nil) + m := NewMutator(cr) + m.AddRule(rbacv1.PolicyRule{ + APIGroups: []string{""}, Resources: []string{"pods"}, Verbs: []string{"get"}, + }) + m.beginFeature() + m.AddRule(rbacv1.PolicyRule{ + APIGroups: []string{""}, Resources: []string{"services"}, Verbs: []string{"list"}, + }) + require.NoError(t, m.Apply()) + + assert.Len(t, cr.Rules, 2) + assert.Equal(t, "pods", cr.Rules[0].Resources[0]) + assert.Equal(t, "services", cr.Rules[1].Resources[0]) +} + +// --- ObjectMutator interface --- + +func TestMutator_ImplementsObjectMutator(_ *testing.T) { + var _ editors.ObjectMutator = (*Mutator)(nil) +} diff --git a/pkg/primitives/clusterrole/resource.go b/pkg/primitives/clusterrole/resource.go new file mode 100644 index 00000000..7a1e5d61 --- /dev/null +++ b/pkg/primitives/clusterrole/resource.go @@ -0,0 +1,68 @@ +package clusterrole + +import ( + "github.com/sourcehawk/operator-component-framework/internal/generic" + rbacv1 "k8s.io/api/rbac/v1" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// DefaultFieldApplicator replaces current with a deep copy of desired. +// +// This is the default baseline field application strategy for ClusterRole resources. +// Use a custom field applicator via Builder.WithCustomFieldApplicator if you need +// to preserve fields that other controllers manage. +func DefaultFieldApplicator(current, desired *rbacv1.ClusterRole) error { + *current = *desired.DeepCopy() + return nil +} + +// Resource is a high-level abstraction for managing a Kubernetes ClusterRole within +// a controller's reconciliation loop. +// +// It implements the following component interfaces: +// - component.Resource: for basic identity and mutation behaviour. +// - component.DataExtractable: for exporting values after successful reconciliation. +// +// ClusterRole resources are static: they do not model convergence health, grace periods, +// or suspension. Use a workload or task primitive for resources that require those concepts. +// +// ClusterRole is cluster-scoped: it has no namespace. +type Resource struct { + base *generic.StaticResource[*rbacv1.ClusterRole, *Mutator] +} + +// Identity returns a unique identifier for the ClusterRole in the format +// "rbac.authorization.k8s.io/v1/ClusterRole/". +func (r *Resource) Identity() string { + return r.base.Identity() +} + +// Object returns a deep copy of the underlying Kubernetes ClusterRole object. +// +// The returned object implements client.Object, making it compatible with +// controller-runtime's Client for Create, Update, and Patch operations. +func (r *Resource) Object() (client.Object, error) { + return r.base.Object() +} + +// Mutate transforms the current state of a Kubernetes ClusterRole into the desired state. +// +// The mutation process follows this order: +// 1. Field application: the current object is updated to reflect the desired base state, +// using either DefaultFieldApplicator or a custom applicator if one is configured. +// 2. Field application flavors: any registered flavors are applied in registration order. +// 3. Feature mutations: all registered feature-gated mutations are applied in order. +// +// This method is invoked by the framework during the Update phase of reconciliation. +func (r *Resource) Mutate(current client.Object) error { + return r.base.Mutate(current) +} + +// ExtractData executes all registered data extractor functions against a deep copy +// of the reconciled ClusterRole. +// +// This is called by the framework after successful reconciliation, allowing the +// component to read generated or updated values from the ClusterRole. +func (r *Resource) ExtractData() error { + return r.base.ExtractData() +} From 0b5ce0d794bf810dd3348a14b941ed3753f29af9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= Date: Sun, 22 Mar 2026 02:42:05 +0000 Subject: [PATCH 03/28] Add ClusterRole primitive documentation Documents the ClusterRole primitive API, mutation system, editors, flavors, aggregation rule support, and usage patterns. Co-Authored-By: Claude Opus 4.6 (1M context) --- docs/primitives/clusterrole.md | 331 +++++++++++++++++++++++++++++++++ 1 file changed, 331 insertions(+) create mode 100644 docs/primitives/clusterrole.md diff --git a/docs/primitives/clusterrole.md b/docs/primitives/clusterrole.md new file mode 100644 index 00000000..17e99204 --- /dev/null +++ b/docs/primitives/clusterrole.md @@ -0,0 +1,331 @@ +# ClusterRole Primitive + +The `clusterrole` primitive is the framework's built-in static abstraction for managing Kubernetes `ClusterRole` resources. It integrates with the component lifecycle and provides a structured mutation API for managing `.rules`, `.aggregationRule`, and object metadata. + +ClusterRole is cluster-scoped: it has no namespace. The builder validates only that the Name is set. + +## Capabilities + +| Capability | Detail | +|-----------------------|---------------------------------------------------------------------------------------------------------------| +| **Static lifecycle** | No health tracking, grace periods, or suspension — the resource is reconciled to desired state | +| **Mutation pipeline** | Typed editors for `.rules` and object metadata, with aggregation rule support and a raw escape hatch | +| **Cluster-scoped** | No namespace required — identity format is `rbac.authorization.k8s.io/v1/ClusterRole/` | +| **Flavors** | Preserves externally-managed fields — labels, annotations, and `.rules` entries not owned by the operator | +| **Data extraction** | Reads generated or updated values back from the reconciled ClusterRole after each sync cycle | + +## Building a ClusterRole Primitive + +```go +import "github.com/sourcehawk/operator-component-framework/pkg/primitives/clusterrole" + +base := &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-operator-role", + }, + Rules: []rbacv1.PolicyRule{ + { + APIGroups: []string{""}, + Resources: []string{"pods"}, + Verbs: []string{"get", "list", "watch"}, + }, + }, +} + +resource, err := clusterrole.NewBuilder(base). + WithFieldApplicationFlavor(clusterrole.PreserveExternalRules). + WithMutation(MyFeatureMutation(owner.Spec.Version)). + Build() +``` + +## Default Field Application + +`DefaultFieldApplicator` replaces the current ClusterRole with a deep copy of the desired object. This ensures every reconciliation cycle produces a clean, predictable state and avoids any drift from the desired baseline. + +Use `WithCustomFieldApplicator` when other controllers manage fields that should not be overwritten: + +```go +resource, err := clusterrole.NewBuilder(base). + WithCustomFieldApplicator(func(current, desired *rbacv1.ClusterRole) error { + // Only synchronise owned rules; leave other fields untouched. + current.Rules = desired.Rules + return nil + }). + Build() +``` + +## Mutations + +Mutations are the primary mechanism for modifying a `ClusterRole` beyond its baseline. Each mutation is a named function that receives a `*Mutator` and records edit intent through typed editors. + +The `Feature` field controls when a mutation applies. Leaving it nil applies the mutation unconditionally: + +```go +func PodReadMutation(version string) clusterrole.Mutation { + return clusterrole.Mutation{ + Name: "pod-read", + Feature: feature.NewResourceFeature(version, nil), + Mutate: func(m *clusterrole.Mutator) error { + m.AddRule(rbacv1.PolicyRule{ + APIGroups: []string{""}, + Resources: []string{"pods"}, + Verbs: []string{"get", "list", "watch"}, + }) + return nil + }, + } +} +``` + +Mutations are applied in the order they are registered with the builder. + +### Boolean-gated mutations + +```go +func SecretAccessMutation(version string, needsSecrets bool) clusterrole.Mutation { + return clusterrole.Mutation{ + Name: "secret-access", + Feature: feature.NewResourceFeature(version, nil).When(needsSecrets), + Mutate: func(m *clusterrole.Mutator) error { + m.AddRule(rbacv1.PolicyRule{ + APIGroups: []string{""}, + Resources: []string{"secrets"}, + Verbs: []string{"get", "list"}, + }) + return nil + }, + } +} +``` + +### Version-gated mutations + +```go +var legacyConstraint = mustSemverConstraint("< 2.0.0") + +func LegacyRBACMutation(version string) clusterrole.Mutation { + return clusterrole.Mutation{ + Name: "legacy-rbac", + Feature: feature.NewResourceFeature( + version, + []feature.VersionConstraint{legacyConstraint}, + ), + Mutate: func(m *clusterrole.Mutator) error { + m.AddRule(rbacv1.PolicyRule{ + APIGroups: []string{"extensions"}, + Resources: []string{"deployments"}, + Verbs: []string{"get", "list"}, + }) + return nil + }, + } +} +``` + +All version constraints and `When()` conditions must be satisfied for a mutation to apply. + +## Internal Mutation Ordering + +Within a single mutation, edit operations are applied in a fixed category order regardless of the order they are recorded: + +| Step | Category | What it affects | +|------|-------------------|-------------------------------------------------| +| 1 | Metadata edits | Labels and annotations on the `ClusterRole` | +| 2 | Rules edits | `.rules` entries — EditRules, AddRule | +| 3 | Aggregation rule | `.aggregationRule` — SetAggregationRule | + +Within each category, edits are applied in their registration order. Later features observe the ClusterRole as modified by all previous features. + +## Editors + +### PolicyRulesEditor + +The primary API for modifying `.rules` entries. Use `m.EditRules` for full control: + +```go +m.EditRules(func(e *editors.PolicyRulesEditor) error { + e.AddRule(rbacv1.PolicyRule{ + APIGroups: []string{"apps"}, + Resources: []string{"deployments"}, + Verbs: []string{"get", "list", "watch"}, + }) + return nil +}) +``` + +#### AddRule + +`AddRule` appends a PolicyRule to the rules slice: + +```go +m.EditRules(func(e *editors.PolicyRulesEditor) error { + e.AddRule(rbacv1.PolicyRule{ + APIGroups: []string{""}, + Resources: []string{"configmaps"}, + Verbs: []string{"get", "list"}, + }) + return nil +}) +``` + +#### RemoveRuleByIndex + +`RemoveRuleByIndex` removes the rule at the given index. It is a no-op if the index is out of bounds: + +```go +m.EditRules(func(e *editors.PolicyRulesEditor) error { + e.RemoveRuleByIndex(0) // remove the first rule + return nil +}) +``` + +#### Clear + +`Clear` removes all rules: + +```go +m.EditRules(func(e *editors.PolicyRulesEditor) error { + e.Clear() + return nil +}) +``` + +#### Raw Escape Hatch + +`Raw()` returns a pointer to the underlying `[]rbacv1.PolicyRule` for free-form editing: + +```go +m.EditRules(func(e *editors.PolicyRulesEditor) error { + raw := e.Raw() + *raw = append(*raw, customRules...) + return nil +}) +``` + +### ObjectMetaEditor + +Modifies labels and annotations via `m.EditObjectMetadata`. + +Available methods: `EnsureLabel`, `RemoveLabel`, `EnsureAnnotation`, `RemoveAnnotation`, `Raw`. + +```go +m.EditObjectMetadata(func(e *editors.ObjectMetaEditor) error { + e.EnsureLabel("app.kubernetes.io/version", version) + e.EnsureAnnotation("managed-by", "my-operator") + return nil +}) +``` + +## Convenience Methods + +The `Mutator` exposes a convenience wrapper for the most common `.rules` operation: + +| Method | Equivalent to | +|-----------------|--------------------------------------| +| `AddRule(rule)` | `EditRules` → `e.AddRule(rule)` | + +Use `AddRule` for simple, single-rule mutations. Use `EditRules` when you need multiple operations or raw access in a single edit block. + +## SetAggregationRule + +`SetAggregationRule` sets the ClusterRole's `.aggregationRule` field. An aggregation rule causes the API server to combine rules from ClusterRoles whose labels match the provided selectors, instead of using `.rules` directly: + +```go +m.SetAggregationRule(&rbacv1.AggregationRule{ + ClusterRoleSelectors: []metav1.LabelSelector{ + {MatchLabels: map[string]string{"rbac.example.com/aggregate-to-admin": "true"}}, + }, +}) +``` + +Setting the aggregation rule to nil clears it. Within a single feature, the last `SetAggregationRule` call wins. + +## Flavors + +Flavors run after the baseline applicator and before mutations. They are used to preserve fields managed by external controllers or other tools. + +### PreserveCurrentLabels + +Preserves labels present on the live object but absent from the applied desired state. Applied labels win on overlap. + +```go +resource, err := clusterrole.NewBuilder(base). + WithFieldApplicationFlavor(clusterrole.PreserveCurrentLabels). + Build() +``` + +### PreserveCurrentAnnotations + +Preserves annotations present on the live object but absent from the applied desired state. Applied annotations win on overlap. + +```go +resource, err := clusterrole.NewBuilder(base). + WithFieldApplicationFlavor(clusterrole.PreserveCurrentAnnotations). + Build() +``` + +### PreserveExternalRules + +Preserves `.rules` entries present on the live object but absent from the applied desired state. Rules are compared by full PolicyRule equality (all fields: APIGroups, Resources, Verbs, ResourceNames, NonResourceURLs). + +Use this when other controllers or admission webhooks inject rules into the ClusterRole that your operator does not own: + +```go +resource, err := clusterrole.NewBuilder(base). + WithFieldApplicationFlavor(clusterrole.PreserveExternalRules). + Build() +``` + +Multiple flavors can be registered and run in registration order. + +## Full Example: Feature-Composed RBAC + +```go +func CoreRulesMutation(version string) clusterrole.Mutation { + return clusterrole.Mutation{ + Name: "core-rules", + Feature: feature.NewResourceFeature(version, nil), + Mutate: func(m *clusterrole.Mutator) error { + m.AddRule(rbacv1.PolicyRule{ + APIGroups: []string{""}, + Resources: []string{"pods", "services", "configmaps"}, + Verbs: []string{"get", "list", "watch"}, + }) + return nil + }, + } +} + +func CRDAccessMutation(version string, manageCRDs bool) clusterrole.Mutation { + return clusterrole.Mutation{ + Name: "crd-access", + Feature: feature.NewResourceFeature(version, nil).When(manageCRDs), + Mutate: func(m *clusterrole.Mutator) error { + m.AddRule(rbacv1.PolicyRule{ + APIGroups: []string{"apiextensions.k8s.io"}, + Resources: []string{"customresourcedefinitions"}, + Verbs: []string{"get", "list", "watch"}, + }) + return nil + }, + } +} + +resource, err := clusterrole.NewBuilder(base). + WithFieldApplicationFlavor(clusterrole.PreserveExternalRules). + WithMutation(CoreRulesMutation(owner.Spec.Version)). + WithMutation(CRDAccessMutation(owner.Spec.Version, owner.Spec.ManageCRDs)). + Build() +``` + +When `ManageCRDs` is true, the final rules include both core and CRD access rules. When false, only the core rules are written. Neither mutation needs to know about the other. + +## Guidance + +**`Feature: nil` applies unconditionally.** Omit `Feature` (leave it nil) for mutations that should always run. Use `feature.NewResourceFeature(version, constraints)` when version-based gating is needed, and chain `.When(bool)` for boolean conditions. + +**Use `PreserveExternalRules` when sharing a ClusterRole.** If admission webhooks, external controllers, or manual operations add rules to a ClusterRole your operator manages, this flavor prevents your operator from silently deleting those rules each reconcile cycle. + +**Use `SetAggregationRule` for composite roles.** When you want the API server to aggregate rules from multiple ClusterRoles based on label selectors, use `SetAggregationRule` instead of managing `.rules` directly. The two approaches are mutually exclusive in the Kubernetes API — the API server ignores `.rules` when `.aggregationRule` is set. + +**Register mutations in dependency order.** If mutation B relies on a rule added by mutation A, register A first. From e57a6310e7a6c973524fd186ab2048adc88d6acb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= Date: Sun, 22 Mar 2026 02:43:37 +0000 Subject: [PATCH 04/28] Add ClusterRole primitive example Demonstrates feature-gated RBAC rule composition using the clusterrole primitive, including core rules, version labels, and conditional secret/deployment access mutations. Co-Authored-By: Claude Opus 4.6 (1M context) --- examples/clusterrole-primitive/README.md | 30 +++++ .../clusterrole-primitive/app/controller.go | 54 ++++++++ .../features/mutations.go | 76 ++++++++++++ examples/clusterrole-primitive/main.go | 115 ++++++++++++++++++ .../resources/clusterrole.go | 55 +++++++++ 5 files changed, 330 insertions(+) create mode 100644 examples/clusterrole-primitive/README.md create mode 100644 examples/clusterrole-primitive/app/controller.go create mode 100644 examples/clusterrole-primitive/features/mutations.go create mode 100644 examples/clusterrole-primitive/main.go create mode 100644 examples/clusterrole-primitive/resources/clusterrole.go diff --git a/examples/clusterrole-primitive/README.md b/examples/clusterrole-primitive/README.md new file mode 100644 index 00000000..83a9f7d3 --- /dev/null +++ b/examples/clusterrole-primitive/README.md @@ -0,0 +1,30 @@ +# ClusterRole Primitive Example + +This example demonstrates the usage of the `clusterrole` primitive within the operator component framework. +It shows how to manage a Kubernetes ClusterRole as a component of a larger application, utilising features like: + +- **Base Construction**: Initializing a cluster-scoped ClusterRole with basic metadata. +- **Feature Mutations**: Composing RBAC rules from independent, feature-gated mutations using `AddRule`. +- **Metadata Mutations**: Setting version labels on the ClusterRole via `EditObjectMetadata`. +- **Field Flavors**: Preserving `.rules` entries managed by external controllers using `PreserveExternalRules`. +- **Data Extraction**: Inspecting ClusterRole rules after each reconcile cycle. + +## Directory Structure + +- `app/`: Defines the controller that uses the component framework. The `ExampleApp` CRD is shared from `examples/shared/app`. +- `features/`: Contains modular feature definitions: + - `mutations.go`: core rules, version labelling, and feature-gated secret and deployment access. +- `resources/`: Contains the central `NewClusterRoleResource` factory that assembles all features using `clusterrole.Builder`. +- `main.go`: A standalone entry point that demonstrates multiple reconciliation cycles with a fake client. + +## Running the Example + +```bash +go run examples/clusterrole-primitive/main.go +``` + +This will: +1. Initialize a fake Kubernetes client. +2. Create an `ExampleApp` owner object. +3. Reconcile through four spec variations, printing the composed rules after each cycle. +4. Print the resulting status conditions. diff --git a/examples/clusterrole-primitive/app/controller.go b/examples/clusterrole-primitive/app/controller.go new file mode 100644 index 00000000..1fbf376f --- /dev/null +++ b/examples/clusterrole-primitive/app/controller.go @@ -0,0 +1,54 @@ +// Package app provides a sample controller using the clusterrole primitive. +package app + +import ( + "context" + + sharedapp "github.com/sourcehawk/operator-component-framework/examples/shared/app" + "github.com/sourcehawk/operator-component-framework/pkg/component" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/tools/record" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// ExampleController reconciles an ExampleApp object using the component framework. +type ExampleController struct { + client.Client + Scheme *runtime.Scheme + Recorder record.EventRecorder + Metrics component.Recorder + + // NewClusterRoleResource is a factory function to create the clusterrole resource. + // This allows us to inject the resource construction logic. + NewClusterRoleResource func(*sharedapp.ExampleApp) (component.Resource, error) +} + +// Reconcile performs the reconciliation for a single ExampleApp. +func (r *ExampleController) Reconcile(ctx context.Context, owner *sharedapp.ExampleApp) error { + // 1. Build the clusterrole resource for this owner. + crResource, err := r.NewClusterRoleResource(owner) + if err != nil { + return err + } + + // 2. Build the component that manages the clusterrole. + comp, err := component.NewComponentBuilder(). + WithName("example-app"). + WithConditionType("AppReady"). + WithResource(crResource, component.ResourceOptions{}). + Build() + if err != nil { + return err + } + + // 3. Execute the component reconciliation. + resCtx := component.ReconcileContext{ + Client: r.Client, + Scheme: r.Scheme, + Recorder: r.Recorder, + Metrics: r.Metrics, + Owner: owner, + } + + return comp.Reconcile(ctx, resCtx) +} diff --git a/examples/clusterrole-primitive/features/mutations.go b/examples/clusterrole-primitive/features/mutations.go new file mode 100644 index 00000000..b5c77dee --- /dev/null +++ b/examples/clusterrole-primitive/features/mutations.go @@ -0,0 +1,76 @@ +// Package features provides sample mutations for the clusterrole primitive example. +package features + +import ( + "github.com/sourcehawk/operator-component-framework/pkg/feature" + "github.com/sourcehawk/operator-component-framework/pkg/mutation/editors" + "github.com/sourcehawk/operator-component-framework/pkg/primitives/clusterrole" + rbacv1 "k8s.io/api/rbac/v1" +) + +// CoreRulesMutation grants read access to core resources (pods, services, configmaps). +// It is always enabled. +func CoreRulesMutation(version string) clusterrole.Mutation { + return clusterrole.Mutation{ + Name: "core-rules", + Feature: feature.NewResourceFeature(version, nil), + Mutate: func(m *clusterrole.Mutator) error { + m.AddRule(rbacv1.PolicyRule{ + APIGroups: []string{""}, + Resources: []string{"pods", "services", "configmaps"}, + Verbs: []string{"get", "list", "watch"}, + }) + return nil + }, + } +} + +// VersionLabelMutation sets the app.kubernetes.io/version label on the ClusterRole. +// It is always enabled. +func VersionLabelMutation(version string) clusterrole.Mutation { + return clusterrole.Mutation{ + Name: "version-label", + Feature: feature.NewResourceFeature(version, nil), + Mutate: func(m *clusterrole.Mutator) error { + m.EditObjectMetadata(func(e *editors.ObjectMetaEditor) error { + e.EnsureLabel("app.kubernetes.io/version", version) + return nil + }) + return nil + }, + } +} + +// SecretAccessMutation grants read access to secrets. +// It is enabled when needsSecrets is true. +func SecretAccessMutation(version string, needsSecrets bool) clusterrole.Mutation { + return clusterrole.Mutation{ + Name: "secret-access", + Feature: feature.NewResourceFeature(version, nil).When(needsSecrets), + Mutate: func(m *clusterrole.Mutator) error { + m.AddRule(rbacv1.PolicyRule{ + APIGroups: []string{""}, + Resources: []string{"secrets"}, + Verbs: []string{"get", "list"}, + }) + return nil + }, + } +} + +// DeploymentAccessMutation grants read/write access to deployments. +// It is enabled when manageDeployments is true. +func DeploymentAccessMutation(version string, manageDeployments bool) clusterrole.Mutation { + return clusterrole.Mutation{ + Name: "deployment-access", + Feature: feature.NewResourceFeature(version, nil).When(manageDeployments), + Mutate: func(m *clusterrole.Mutator) error { + m.AddRule(rbacv1.PolicyRule{ + APIGroups: []string{"apps"}, + Resources: []string{"deployments"}, + Verbs: []string{"get", "list", "watch", "create", "update", "patch"}, + }) + return nil + }, + } +} diff --git a/examples/clusterrole-primitive/main.go b/examples/clusterrole-primitive/main.go new file mode 100644 index 00000000..f18dcd27 --- /dev/null +++ b/examples/clusterrole-primitive/main.go @@ -0,0 +1,115 @@ +// Package main is the entry point for the clusterrole primitive example. +package main + +import ( + "context" + "fmt" + "os" + + ocm "github.com/sourcehawk/go-crd-condition-metrics/pkg/crd-condition-metrics" + "github.com/sourcehawk/operator-component-framework/examples/clusterrole-primitive/app" + "github.com/sourcehawk/operator-component-framework/examples/clusterrole-primitive/resources" + sharedapp "github.com/sourcehawk/operator-component-framework/examples/shared/app" + rbacv1 "k8s.io/api/rbac/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/tools/record" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +func main() { + // 1. Setup scheme and fake client. + scheme := runtime.NewScheme() + if err := sharedapp.AddToScheme(scheme); err != nil { + fmt.Fprintf(os.Stderr, "failed to add to scheme: %v\n", err) + os.Exit(1) + } + if err := rbacv1.AddToScheme(scheme); err != nil { + fmt.Fprintf(os.Stderr, "failed to add rbac/v1 to scheme: %v\n", err) + os.Exit(1) + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithStatusSubresource(&sharedapp.ExampleApp{}). + Build() + + // 2. Create an example Owner object. + owner := &sharedapp.ExampleApp{ + Spec: sharedapp.ExampleAppSpec{ + Version: "1.2.3", + EnableTracing: true, + EnableMetrics: true, + }, + } + owner.Name = "my-example-app" + owner.Namespace = "default" + + if err := fakeClient.Create(context.Background(), owner); err != nil { + fmt.Fprintf(os.Stderr, "failed to create owner: %v\n", err) + os.Exit(1) + } + + // 3. Initialize the controller. + gauge := ocm.NewOperatorConditionsGauge("example") + controller := &app.ExampleController{ + Client: fakeClient, + Scheme: scheme, + Recorder: record.NewFakeRecorder(100), + Metrics: &ocm.ConditionMetricRecorder{ + Controller: "example-controller", + OperatorConditionsGauge: gauge, + }, + NewClusterRoleResource: resources.NewClusterRoleResource, + } + + // 4. Run reconciliation with multiple spec versions to demonstrate how + // feature-gated mutations compose RBAC rules from independent features. + specs := []sharedapp.ExampleAppSpec{ + { + Version: "1.2.3", + EnableTracing: true, + EnableMetrics: true, + }, + { + Version: "1.2.4", // Version upgrade + EnableTracing: true, + EnableMetrics: true, + }, + { + Version: "1.2.4", + EnableTracing: false, // Disable secret access + EnableMetrics: true, + }, + { + Version: "1.2.4", + EnableTracing: false, + EnableMetrics: false, // Disable deployment access too + }, + } + + ctx := context.Background() + + for i, spec := range specs { + fmt.Printf("\n--- Step %d: Version=%s, SecretAccess=%v, DeploymentAccess=%v ---\n", + i+1, spec.Version, spec.EnableTracing, spec.EnableMetrics) + + owner.Spec = spec + if err := fakeClient.Update(ctx, owner); err != nil { + fmt.Fprintf(os.Stderr, "failed to update owner: %v\n", err) + os.Exit(1) + } + + fmt.Println("Running reconciliation...") + if err := controller.Reconcile(ctx, owner); err != nil { + fmt.Fprintf(os.Stderr, "reconciliation failed: %v\n", err) + os.Exit(1) + } + + for _, cond := range owner.Status.Conditions { + fmt.Printf("Condition: %s, Status: %s, Reason: %s\n", + cond.Type, cond.Status, cond.Reason) + } + } + + fmt.Println("\nReconciliation sequence completed successfully!") +} diff --git a/examples/clusterrole-primitive/resources/clusterrole.go b/examples/clusterrole-primitive/resources/clusterrole.go new file mode 100644 index 00000000..defe2b99 --- /dev/null +++ b/examples/clusterrole-primitive/resources/clusterrole.go @@ -0,0 +1,55 @@ +// Package resources provides resource implementations for the clusterrole primitive example. +package resources + +import ( + "fmt" + + "github.com/sourcehawk/operator-component-framework/examples/clusterrole-primitive/features" + sharedapp "github.com/sourcehawk/operator-component-framework/examples/shared/app" + "github.com/sourcehawk/operator-component-framework/pkg/component" + "github.com/sourcehawk/operator-component-framework/pkg/primitives/clusterrole" + rbacv1 "k8s.io/api/rbac/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +// NewClusterRoleResource constructs a clusterrole primitive resource with all the features. +func NewClusterRoleResource(owner *sharedapp.ExampleApp) (component.Resource, error) { + // 1. Create the base ClusterRole object. + // ClusterRole is cluster-scoped — no namespace is set. + base := &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + Name: owner.Name + "-role", + Labels: map[string]string{ + "app": owner.Name, + }, + }, + } + + // 2. Initialize the clusterrole builder. + builder := clusterrole.NewBuilder(base) + + // 3. Register mutations in dependency order. + // CoreRulesMutation and VersionLabelMutation always run first. + // SecretAccess and DeploymentAccess are feature-gated. + builder.WithMutation(features.CoreRulesMutation(owner.Spec.Version)) + builder.WithMutation(features.VersionLabelMutation(owner.Spec.Version)) + builder.WithMutation(features.SecretAccessMutation(owner.Spec.Version, owner.Spec.EnableTracing)) + builder.WithMutation(features.DeploymentAccessMutation(owner.Spec.Version, owner.Spec.EnableMetrics)) + + // 4. Preserve rules added by external controllers or admission webhooks. + builder.WithFieldApplicationFlavor(clusterrole.PreserveExternalRules) + + // 5. Extract data from the reconciled ClusterRole. + builder.WithDataExtractor(func(cr rbacv1.ClusterRole) error { + fmt.Printf("Reconciled ClusterRole: %s\n", cr.Name) + fmt.Printf(" Rules: %d\n", len(cr.Rules)) + for i, rule := range cr.Rules { + fmt.Printf(" [%d] APIGroups=%v Resources=%v Verbs=%v\n", + i, rule.APIGroups, rule.Resources, rule.Verbs) + } + return nil + }) + + // 6. Build the final resource. + return builder.Build() +} From 455871cbbae5865fa4d77a839796f24b993b7783 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= Date: Sun, 22 Mar 2026 04:00:08 +0000 Subject: [PATCH 05/28] address copilot comments --- docs/primitives/clusterrole.md | 5 ++-- internal/generic/builder_static_test.go | 27 ++++++++++++++++++++++ pkg/primitives/clusterrole/builder.go | 8 +------ pkg/primitives/clusterrole/builder_test.go | 7 ++++++ 4 files changed, 37 insertions(+), 10 deletions(-) diff --git a/docs/primitives/clusterrole.md b/docs/primitives/clusterrole.md index 17e99204..de1c46a4 100644 --- a/docs/primitives/clusterrole.md +++ b/docs/primitives/clusterrole.md @@ -61,10 +61,9 @@ Mutations are the primary mechanism for modifying a `ClusterRole` beyond its bas The `Feature` field controls when a mutation applies. Leaving it nil applies the mutation unconditionally: ```go -func PodReadMutation(version string) clusterrole.Mutation { +func PodReadMutation() clusterrole.Mutation { return clusterrole.Mutation{ - Name: "pod-read", - Feature: feature.NewResourceFeature(version, nil), + Name: "pod-read", Mutate: func(m *clusterrole.Mutator) error { m.AddRule(rbacv1.PolicyRule{ APIGroups: []string{""}, diff --git a/internal/generic/builder_static_test.go b/internal/generic/builder_static_test.go index 4e5bc09e..816a4fa5 100644 --- a/internal/generic/builder_static_test.go +++ b/internal/generic/builder_static_test.go @@ -67,6 +67,33 @@ func TestStaticBuilder(t *testing.T) { require.EqualError(t, err, errClusterScopedNamespace) }) + t.Run("cluster-scoped build succeeds without namespace", func(t *testing.T) { + clusterObj := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster-obj"}, + } + builder := NewStaticBuilder(clusterObj, identityFunc, defaultApp, newMutator) + builder.MarkClusterScoped() + res, err := builder.Build() + if err != nil { + t.Fatalf("Build() error = %v", err) + } + if res.DesiredObject != clusterObj { + t.Errorf("expected object %v, got %v", clusterObj, res.DesiredObject) + } + }) + + t.Run("cluster-scoped build rejects non-empty namespace", func(t *testing.T) { + nsObj := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster-obj", Namespace: "oops"}, + } + builder := NewStaticBuilder(nsObj, identityFunc, defaultApp, newMutator) + builder.MarkClusterScoped() + _, err := builder.Build() + if err == nil || err.Error() != "cluster-scoped object must not have a namespace" { + t.Errorf("expected cluster-scoped namespace error, got %v", err) + } + }) + t.Run("validation errors", func(t *testing.T) { runBuilderValidationTests( t, diff --git a/pkg/primitives/clusterrole/builder.go b/pkg/primitives/clusterrole/builder.go index 81f9ed00..a7f00224 100644 --- a/pkg/primitives/clusterrole/builder.go +++ b/pkg/primitives/clusterrole/builder.go @@ -109,15 +109,13 @@ func (b *Builder) Build() (*Resource, error) { return fmt.Sprintf("rbac.authorization.k8s.io/v1/ClusterRole/%s", cr.Name) } - // ClusterRole is cluster-scoped and has no namespace. The generic - // StaticBuilder.Build calls ValidateBase which rejects an empty namespace, - // so we construct the StaticResource directly with our own validation. sb := generic.NewStaticBuilder[*rbacv1.ClusterRole, *Mutator]( b.obj, identityFunc, DefaultFieldApplicator, NewMutator, ) + sb.MarkClusterScoped() for _, m := range b.mutations { sb.WithMutation(m) @@ -135,11 +133,7 @@ func (b *Builder) Build() (*Resource, error) { sb.WithDataExtractor(e) } - // Temporarily set namespace to pass ValidateBase, then clear it. - // ClusterRole is cluster-scoped and must not have a namespace set. - b.obj.Namespace = "cluster-scoped" res, err := sb.Build() - b.obj.Namespace = "" if err != nil { return nil, err } diff --git a/pkg/primitives/clusterrole/builder_test.go b/pkg/primitives/clusterrole/builder_test.go index be567623..7fcfc424 100644 --- a/pkg/primitives/clusterrole/builder_test.go +++ b/pkg/primitives/clusterrole/builder_test.go @@ -40,6 +40,13 @@ func TestBuilder_Build_Validation(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Name: "test-cr"}, }, }, + { + name: "rejects namespace on cluster-scoped resource", + cr: &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{Name: "test-cr", Namespace: "oops"}, + }, + expectedErr: "cluster-scoped object must not have a namespace", + }, } for _, tt := range tests { From 7c43752f8062dc032ded79a179feffd6b2e41bae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= Date: Sun, 22 Mar 2026 04:34:24 +0000 Subject: [PATCH 06/28] Address Copilot review comments on ClusterRole primitive MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remove feature-plan abstraction from Mutator: the FeatureMutator interface in internal/generic uses an unexported beginFeature() method, so the clusterrole Mutator could never satisfy it across package boundaries. Replaced with flat intent lists and fixed category ordering (metadata → rules → aggregation), which matches the actual runtime behavior. - Update mutator test: replace TestMutator_MultipleFeatures (which called unexported beginFeature() directly) with TestMutator_MultipleMutations that exercises the public API only. - Update docs to reflect that namespace must be empty (not just optional) for cluster-scoped resources, matching ValidateBase() behavior. - Update mutation ordering docs to accurately describe flat category ordering rather than per-feature boundaries. Co-Authored-By: Claude Opus 4.6 (1M context) --- docs/primitives/clusterrole.md | 6 +- pkg/primitives/clusterrole/mutator.go | 87 +++++++++------------- pkg/primitives/clusterrole/mutator_test.go | 3 +- 3 files changed, 38 insertions(+), 58 deletions(-) diff --git a/docs/primitives/clusterrole.md b/docs/primitives/clusterrole.md index de1c46a4..2f304aef 100644 --- a/docs/primitives/clusterrole.md +++ b/docs/primitives/clusterrole.md @@ -2,7 +2,7 @@ The `clusterrole` primitive is the framework's built-in static abstraction for managing Kubernetes `ClusterRole` resources. It integrates with the component lifecycle and provides a structured mutation API for managing `.rules`, `.aggregationRule`, and object metadata. -ClusterRole is cluster-scoped: it has no namespace. The builder validates only that the Name is set. +ClusterRole is cluster-scoped: it has no namespace. The builder validates that the Name is set and that Namespace is empty — setting a namespace on a cluster-scoped resource is rejected. ## Capabilities @@ -125,7 +125,7 @@ All version constraints and `When()` conditions must be satisfied for a mutation ## Internal Mutation Ordering -Within a single mutation, edit operations are applied in a fixed category order regardless of the order they are recorded: +All mutation intents from all registered mutations are collected into flat lists and applied in a fixed category order, regardless of the order they are recorded: | Step | Category | What it affects | |------|-------------------|-------------------------------------------------| @@ -133,7 +133,7 @@ Within a single mutation, edit operations are applied in a fixed category order | 2 | Rules edits | `.rules` entries — EditRules, AddRule | | 3 | Aggregation rule | `.aggregationRule` — SetAggregationRule | -Within each category, edits are applied in their registration order. Later features observe the ClusterRole as modified by all previous features. +Within each category, edits are applied in their registration order. For aggregation rules, the last `SetAggregationRule` call wins. ## Editors diff --git a/pkg/primitives/clusterrole/mutator.go b/pkg/primitives/clusterrole/mutator.go index 9ec33100..77f1e853 100644 --- a/pkg/primitives/clusterrole/mutator.go +++ b/pkg/primitives/clusterrole/mutator.go @@ -11,51 +11,38 @@ import ( // only if its associated feature gate is enabled. type Mutation feature.Mutation[*Mutator] -type featurePlan struct { - metadataEdits []func(*editors.ObjectMetaEditor) error - rulesEdits []func(*editors.PolicyRulesEditor) error - aggregationRuleSets []*rbacv1.AggregationRule -} - // Mutator is a high-level helper for modifying a Kubernetes ClusterRole. // // It uses a "plan-and-apply" pattern: mutations are recorded first, then // applied to the ClusterRole in a single controlled pass when Apply() is called. // -// The Mutator maintains feature boundaries: each feature's mutations are planned -// together and applied in the order the features were registered. +// All mutation intents are collected into flat lists and applied in a fixed +// category order: metadata edits, then rules edits, then aggregation rule. +// Within each category, edits are applied in registration order. // // Mutator implements editors.ObjectMutator. type Mutator struct { cr *rbacv1.ClusterRole - plans []featurePlan - active *featurePlan + metadataEdits []func(*editors.ObjectMetaEditor) error + rulesEdits []func(*editors.PolicyRulesEditor) error + aggregationRuleSets []*rbacv1.AggregationRule } // NewMutator creates a new Mutator for the given ClusterRole. func NewMutator(cr *rbacv1.ClusterRole) *Mutator { - m := &Mutator{cr: cr} - m.beginFeature() - return m -} - -// beginFeature starts a new feature planning scope. All subsequent mutation -// registrations will be grouped into this feature's plan. -func (m *Mutator) beginFeature() { - m.plans = append(m.plans, featurePlan{}) - m.active = &m.plans[len(m.plans)-1] + return &Mutator{cr: cr} } // EditObjectMetadata records a mutation for the ClusterRole's own metadata. // -// Metadata edits are applied before rules edits within the same feature. +// Metadata edits are applied before rules edits. // A nil edit function is ignored. func (m *Mutator) EditObjectMetadata(edit func(*editors.ObjectMetaEditor) error) { if edit == nil { return } - m.active.metadataEdits = append(m.active.metadataEdits, edit) + m.metadataEdits = append(m.metadataEdits, edit) } // EditRules records a mutation for the ClusterRole's .rules field via a @@ -63,14 +50,14 @@ func (m *Mutator) EditObjectMetadata(edit func(*editors.ObjectMetaEditor) error) // // The editor provides structured operations (AddRule, RemoveRuleByIndex, Clear) // as well as Raw() for free-form access. Rules edits are applied after metadata -// edits within the same feature, in registration order. +// edits, in registration order. // // A nil edit function is ignored. func (m *Mutator) EditRules(edit func(*editors.PolicyRulesEditor) error) { if edit == nil { return } - m.active.rulesEdits = append(m.active.rulesEdits, edit) + m.rulesEdits = append(m.rulesEdits, edit) } // AddRule records that a PolicyRule should be appended to .rules. @@ -88,50 +75,44 @@ func (m *Mutator) AddRule(rule rbacv1.PolicyRule) { // // An aggregation rule causes the API server to combine rules from ClusterRoles // whose labels match the provided selectors, instead of using .rules directly. -// Setting an aggregation rule replaces any previously recorded value within the -// same feature. +// If called multiple times, the last call wins. // // A nil value clears the aggregation rule. func (m *Mutator) SetAggregationRule(rule *rbacv1.AggregationRule) { - m.active.aggregationRuleSets = append(m.active.aggregationRuleSets, rule) + m.aggregationRuleSets = append(m.aggregationRuleSets, rule) } // Apply executes all recorded mutation intents on the underlying ClusterRole. // -// Execution order across all registered features: -// -// 1. Metadata edits (in registration order within each feature) -// 2. Rules edits — EditRules, AddRule (in registration order within each feature) -// 3. Aggregation rule — SetAggregationRule (last value within each feature wins) +// Execution order: // -// Features are applied in the order they were registered. Later features observe -// the ClusterRole as modified by all previous features. +// 1. Metadata edits (in registration order) +// 2. Rules edits — EditRules, AddRule (in registration order) +// 3. Aggregation rule — SetAggregationRule (last call wins) func (m *Mutator) Apply() error { - for _, plan := range m.plans { - // 1. Metadata edits - if len(plan.metadataEdits) > 0 { - editor := editors.NewObjectMetaEditor(&m.cr.ObjectMeta) - for _, edit := range plan.metadataEdits { - if err := edit(editor); err != nil { - return err - } + // 1. Metadata edits + if len(m.metadataEdits) > 0 { + editor := editors.NewObjectMetaEditor(&m.cr.ObjectMeta) + for _, edit := range m.metadataEdits { + if err := edit(editor); err != nil { + return err } } + } - // 2. Rules edits - if len(plan.rulesEdits) > 0 { - editor := editors.NewPolicyRulesEditor(&m.cr.Rules) - for _, edit := range plan.rulesEdits { - if err := edit(editor); err != nil { - return err - } + // 2. Rules edits + if len(m.rulesEdits) > 0 { + editor := editors.NewPolicyRulesEditor(&m.cr.Rules) + for _, edit := range m.rulesEdits { + if err := edit(editor); err != nil { + return err } } + } - // 3. Aggregation rule (last value within the feature wins) - if len(plan.aggregationRuleSets) > 0 { - m.cr.AggregationRule = plan.aggregationRuleSets[len(plan.aggregationRuleSets)-1] - } + // 3. Aggregation rule (last call wins) + if len(m.aggregationRuleSets) > 0 { + m.cr.AggregationRule = m.aggregationRuleSets[len(m.aggregationRuleSets)-1] } return nil diff --git a/pkg/primitives/clusterrole/mutator_test.go b/pkg/primitives/clusterrole/mutator_test.go index 7af8721f..089c64c5 100644 --- a/pkg/primitives/clusterrole/mutator_test.go +++ b/pkg/primitives/clusterrole/mutator_test.go @@ -156,13 +156,12 @@ func TestMutator_OperationOrder(t *testing.T) { assert.Equal(t, "pods", cr.Rules[0].Resources[0]) } -func TestMutator_MultipleFeatures(t *testing.T) { +func TestMutator_MultipleMutations(t *testing.T) { cr := newTestCR(nil) m := NewMutator(cr) m.AddRule(rbacv1.PolicyRule{ APIGroups: []string{""}, Resources: []string{"pods"}, Verbs: []string{"get"}, }) - m.beginFeature() m.AddRule(rbacv1.PolicyRule{ APIGroups: []string{""}, Resources: []string{"services"}, Verbs: []string{"list"}, }) From a0d4f8db274b0896af1aa8586a50b6b9701f48d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= Date: Sun, 22 Mar 2026 04:36:21 +0000 Subject: [PATCH 07/28] Guard against nil rules pointer in NewPolicyRulesEditor Defensively allocate a local zero-value slice when a nil pointer is passed, matching the pattern used by NewConfigMapDataEditor. This prevents nil-pointer panics in Raw()/AddRule()/RemoveRuleByIndex()/Clear(). Co-Authored-By: Claude Opus 4.6 (1M context) --- pkg/mutation/editors/policyrules.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pkg/mutation/editors/policyrules.go b/pkg/mutation/editors/policyrules.go index ced3f999..60ce4f9c 100644 --- a/pkg/mutation/editors/policyrules.go +++ b/pkg/mutation/editors/policyrules.go @@ -18,6 +18,10 @@ type PolicyRulesEditor struct { // automatically. Pass a non-nil pointer (e.g. &cr.Rules) so that writes are // reflected on the object. func NewPolicyRulesEditor(rules *[]rbacv1.PolicyRule) *PolicyRulesEditor { + if rules == nil { + var r []rbacv1.PolicyRule + rules = &r + } return &PolicyRulesEditor{rules: rules} } From 488a4ca46019ad593d98cc2efa6cf5752c72dbc6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= Date: Sun, 22 Mar 2026 19:59:04 +0000 Subject: [PATCH 08/28] preserve server-managed metadata in default field applicator Co-Authored-By: Claude Opus 4.6 (1M context) --- pkg/primitives/clusterrole/resource.go | 8 ++- pkg/primitives/clusterrole/resource_test.go | 56 +++++++++++++++++++++ 2 files changed, 62 insertions(+), 2 deletions(-) create mode 100644 pkg/primitives/clusterrole/resource_test.go diff --git a/pkg/primitives/clusterrole/resource.go b/pkg/primitives/clusterrole/resource.go index 7a1e5d61..b801e85f 100644 --- a/pkg/primitives/clusterrole/resource.go +++ b/pkg/primitives/clusterrole/resource.go @@ -6,13 +6,17 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" ) -// DefaultFieldApplicator replaces current with a deep copy of desired. +// DefaultFieldApplicator replaces current with a deep copy of desired while +// preserving server-managed metadata (ResourceVersion, UID, Generation, etc.) +// and shared-controller fields (OwnerReferences, Finalizers) from the original +// current object. // -// This is the default baseline field application strategy for ClusterRole resources. // Use a custom field applicator via Builder.WithCustomFieldApplicator if you need // to preserve fields that other controllers manage. func DefaultFieldApplicator(current, desired *rbacv1.ClusterRole) error { + original := current.DeepCopy() *current = *desired.DeepCopy() + generic.PreserveServerManagedFields(current, original) return nil } diff --git a/pkg/primitives/clusterrole/resource_test.go b/pkg/primitives/clusterrole/resource_test.go new file mode 100644 index 00000000..a3037f6c --- /dev/null +++ b/pkg/primitives/clusterrole/resource_test.go @@ -0,0 +1,56 @@ +package clusterrole + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + rbacv1 "k8s.io/api/rbac/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func TestDefaultFieldApplicator_PreservesServerManagedFields(t *testing.T) { + current := &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + ResourceVersion: "12345", + UID: "abc-def", + Generation: 3, + OwnerReferences: []metav1.OwnerReference{ + {APIVersion: "v1", Kind: "Pod", Name: "other-owner", UID: "other-uid"}, + }, + Finalizers: []string{"finalizer.example.com"}, + }, + } + desired := &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Labels: map[string]string{"app": "test"}, + }, + Rules: []rbacv1.PolicyRule{ + { + APIGroups: []string{""}, + Resources: []string{"pods"}, + Verbs: []string{"get", "list"}, + }, + }, + } + + err := DefaultFieldApplicator(current, desired) + require.NoError(t, err) + + // Desired spec and labels are applied + assert.Equal(t, "test", current.Labels["app"]) + require.Len(t, current.Rules, 1) + assert.Equal(t, []string{"get", "list"}, current.Rules[0].Verbs) + + // Server-managed fields are preserved + assert.Equal(t, "12345", current.ResourceVersion) + assert.Equal(t, "abc-def", string(current.UID)) + assert.Equal(t, int64(3), current.Generation) + + // Shared-controller fields are preserved + assert.Len(t, current.OwnerReferences, 1) + assert.Equal(t, "other-owner", current.OwnerReferences[0].Name) + assert.Equal(t, []string{"finalizer.example.com"}, current.Finalizers) +} From 85d940cbfcc1cf56f542ca40c556207b0751d2fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= Date: Mon, 23 Mar 2026 00:49:49 +0000 Subject: [PATCH 09/28] Address Copilot review: remove duplicate test, document ownership limitation - Remove redundant "valid clusterrole without namespace" test case that was identical to "valid clusterrole" - Document cluster-scoped ownership limitation in clusterrole docs - Add comment in example about namespaced owner limitation with real clusters Co-Authored-By: Claude Opus 4.6 (1M context) --- docs/primitives/clusterrole.md | 2 ++ examples/clusterrole-primitive/main.go | 4 ++++ pkg/primitives/clusterrole/builder_test.go | 6 ------ 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/docs/primitives/clusterrole.md b/docs/primitives/clusterrole.md index 2f304aef..e20f12d0 100644 --- a/docs/primitives/clusterrole.md +++ b/docs/primitives/clusterrole.md @@ -4,6 +4,8 @@ The `clusterrole` primitive is the framework's built-in static abstraction for m ClusterRole is cluster-scoped: it has no namespace. The builder validates that the Name is set and that Namespace is empty — setting a namespace on a cluster-scoped resource is rejected. +> **Ownership limitation:** The framework currently sets a controller reference on every managed object. Kubernetes and controller-runtime reject owner references where the owner is namespaced and the dependent is cluster-scoped, so a namespaced owner cannot own a `ClusterRole`. When using the `clusterrole` primitive, ensure that the owning custom resource (and its controller) is also cluster-scoped, or avoid managing `ClusterRole` resources with this framework until an ownership opt-out is available. + ## Capabilities | Capability | Detail | diff --git a/examples/clusterrole-primitive/main.go b/examples/clusterrole-primitive/main.go index f18dcd27..b1240e6c 100644 --- a/examples/clusterrole-primitive/main.go +++ b/examples/clusterrole-primitive/main.go @@ -34,6 +34,10 @@ func main() { Build() // 2. Create an example Owner object. + // NOTE: This example uses a namespaced owner for simplicity with the fake client. + // In a real cluster, Kubernetes rejects owner references from a namespaced owner to + // a cluster-scoped dependent (ClusterRole). Production controllers that manage + // ClusterRoles should use a cluster-scoped owner CRD. owner := &sharedapp.ExampleApp{ Spec: sharedapp.ExampleAppSpec{ Version: "1.2.3", diff --git a/pkg/primitives/clusterrole/builder_test.go b/pkg/primitives/clusterrole/builder_test.go index 7fcfc424..6e672192 100644 --- a/pkg/primitives/clusterrole/builder_test.go +++ b/pkg/primitives/clusterrole/builder_test.go @@ -34,12 +34,6 @@ func TestBuilder_Build_Validation(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Name: "test-cr"}, }, }, - { - name: "valid clusterrole without namespace", - cr: &rbacv1.ClusterRole{ - ObjectMeta: metav1.ObjectMeta{Name: "test-cr"}, - }, - }, { name: "rejects namespace on cluster-scoped resource", cr: &rbacv1.ClusterRole{ From 3acf33cc9adc8bdf130169cbb0682c90d0c3621f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= Date: Mon, 23 Mar 2026 03:25:46 +0000 Subject: [PATCH 10/28] Address Copilot review: per-feature mutator, ownership docs, PreserveExternalRules - Adopt per-feature planning pattern (beginFeature/featurePlan) in the ClusterRole mutator for consistency with ConfigMap and Deployment primitives, ensuring mutation ordering matches registration order - Update ownership limitation docs to reflect actual framework behavior: scope mismatch is detected and owner ref is skipped with a log message, rather than rejecting the reconcile - Remove PreserveExternalRules from example and docs feature-gating walkthrough to avoid conflict where disabled feature-gated rules would be preserved from the live object - Add documentation note about PreserveExternalRules trade-off with feature-gated mutations - Remove duplicate cluster-scoped validation tests in builder_static_test Co-Authored-By: Claude Opus 4.6 (1M context) --- docs/primitives/clusterrole.md | 9 +- examples/clusterrole-primitive/README.md | 1 - examples/clusterrole-primitive/main.go | 9 +- .../resources/clusterrole.go | 7 +- internal/generic/builder_static_test.go | 27 ------ pkg/primitives/clusterrole/mutator.go | 88 ++++++++++++------- pkg/primitives/clusterrole/mutator_test.go | 54 ++++++++++++ 7 files changed, 121 insertions(+), 74 deletions(-) diff --git a/docs/primitives/clusterrole.md b/docs/primitives/clusterrole.md index e20f12d0..c20ab9bb 100644 --- a/docs/primitives/clusterrole.md +++ b/docs/primitives/clusterrole.md @@ -4,7 +4,7 @@ The `clusterrole` primitive is the framework's built-in static abstraction for m ClusterRole is cluster-scoped: it has no namespace. The builder validates that the Name is set and that Namespace is empty — setting a namespace on a cluster-scoped resource is rejected. -> **Ownership limitation:** The framework currently sets a controller reference on every managed object. Kubernetes and controller-runtime reject owner references where the owner is namespaced and the dependent is cluster-scoped, so a namespaced owner cannot own a `ClusterRole`. When using the `clusterrole` primitive, ensure that the owning custom resource (and its controller) is also cluster-scoped, or avoid managing `ClusterRole` resources with this framework until an ownership opt-out is available. +> **Ownership limitation:** During reconciliation, the framework attempts to set a controller reference on managed objects, but only when the owner and dependent scopes are compatible. When a namespaced owner manages a cluster-scoped resource such as a `ClusterRole`, the owner reference is skipped (and this is logged) instead of causing the reconcile to fail. In this case, the `ClusterRole` is **not** owned by the custom resource for Kubernetes garbage-collection or ownership semantics, so it will not be automatically deleted when the owner is removed; you must handle its lifecycle explicitly or use a cluster-scoped owner if automatic cleanup is required. ## Capabilities @@ -127,7 +127,7 @@ All version constraints and `When()` conditions must be satisfied for a mutation ## Internal Mutation Ordering -All mutation intents from all registered mutations are collected into flat lists and applied in a fixed category order, regardless of the order they are recorded: +The Mutator maintains feature boundaries: each feature's mutations are planned together and applied in the order the features were registered. Within each feature, edits are applied in a fixed category order: | Step | Category | What it affects | |------|-------------------|-------------------------------------------------| @@ -135,7 +135,7 @@ All mutation intents from all registered mutations are collected into flat lists | 2 | Rules edits | `.rules` entries — EditRules, AddRule | | 3 | Aggregation rule | `.aggregationRule` — SetAggregationRule | -Within each category, edits are applied in their registration order. For aggregation rules, the last `SetAggregationRule` call wins. +Within each category, edits are applied in their registration order. For aggregation rules, the last `SetAggregationRule` call wins within each feature. Later features observe the ClusterRole as modified by all previous features. ## Editors @@ -313,7 +313,6 @@ func CRDAccessMutation(version string, manageCRDs bool) clusterrole.Mutation { } resource, err := clusterrole.NewBuilder(base). - WithFieldApplicationFlavor(clusterrole.PreserveExternalRules). WithMutation(CoreRulesMutation(owner.Spec.Version)). WithMutation(CRDAccessMutation(owner.Spec.Version, owner.Spec.ManageCRDs)). Build() @@ -321,6 +320,8 @@ resource, err := clusterrole.NewBuilder(base). When `ManageCRDs` is true, the final rules include both core and CRD access rules. When false, only the core rules are written. Neither mutation needs to know about the other. +> **Note:** Do not combine `PreserveExternalRules` with feature-gated mutations that add and remove rules. Because flavors run before mutations and preserve rules from the live object, previously-added rules will be retained even after a feature gate is disabled, and rules can be duplicated if a mutation re-adds a rule already present on the live object. Use `PreserveExternalRules` only when external controllers or admission webhooks manage rules that your operator does not own. + ## Guidance **`Feature: nil` applies unconditionally.** Omit `Feature` (leave it nil) for mutations that should always run. Use `feature.NewResourceFeature(version, constraints)` when version-based gating is needed, and chain `.When(bool)` for boolean conditions. diff --git a/examples/clusterrole-primitive/README.md b/examples/clusterrole-primitive/README.md index 83a9f7d3..5999535b 100644 --- a/examples/clusterrole-primitive/README.md +++ b/examples/clusterrole-primitive/README.md @@ -6,7 +6,6 @@ It shows how to manage a Kubernetes ClusterRole as a component of a larger appli - **Base Construction**: Initializing a cluster-scoped ClusterRole with basic metadata. - **Feature Mutations**: Composing RBAC rules from independent, feature-gated mutations using `AddRule`. - **Metadata Mutations**: Setting version labels on the ClusterRole via `EditObjectMetadata`. -- **Field Flavors**: Preserving `.rules` entries managed by external controllers using `PreserveExternalRules`. - **Data Extraction**: Inspecting ClusterRole rules after each reconcile cycle. ## Directory Structure diff --git a/examples/clusterrole-primitive/main.go b/examples/clusterrole-primitive/main.go index b1240e6c..fa4a5504 100644 --- a/examples/clusterrole-primitive/main.go +++ b/examples/clusterrole-primitive/main.go @@ -35,9 +35,12 @@ func main() { // 2. Create an example Owner object. // NOTE: This example uses a namespaced owner for simplicity with the fake client. - // In a real cluster, Kubernetes rejects owner references from a namespaced owner to - // a cluster-scoped dependent (ClusterRole). Production controllers that manage - // ClusterRoles should use a cluster-scoped owner CRD. + // The framework detects the scope mismatch between a namespaced owner and + // a cluster-scoped dependent (ClusterRole) and skips setting the controller + // reference (logging a message), so reconciliation still proceeds but without + // garbage collection or owner-based adoption for the ClusterRole. If you want + // owner references and GC/adoption for ClusterRoles in production, use a + // cluster-scoped owner CRD. owner := &sharedapp.ExampleApp{ Spec: sharedapp.ExampleAppSpec{ Version: "1.2.3", diff --git a/examples/clusterrole-primitive/resources/clusterrole.go b/examples/clusterrole-primitive/resources/clusterrole.go index defe2b99..822c4da7 100644 --- a/examples/clusterrole-primitive/resources/clusterrole.go +++ b/examples/clusterrole-primitive/resources/clusterrole.go @@ -36,10 +36,7 @@ func NewClusterRoleResource(owner *sharedapp.ExampleApp) (component.Resource, er builder.WithMutation(features.SecretAccessMutation(owner.Spec.Version, owner.Spec.EnableTracing)) builder.WithMutation(features.DeploymentAccessMutation(owner.Spec.Version, owner.Spec.EnableMetrics)) - // 4. Preserve rules added by external controllers or admission webhooks. - builder.WithFieldApplicationFlavor(clusterrole.PreserveExternalRules) - - // 5. Extract data from the reconciled ClusterRole. + // 4. Extract data from the reconciled ClusterRole. builder.WithDataExtractor(func(cr rbacv1.ClusterRole) error { fmt.Printf("Reconciled ClusterRole: %s\n", cr.Name) fmt.Printf(" Rules: %d\n", len(cr.Rules)) @@ -50,6 +47,6 @@ func NewClusterRoleResource(owner *sharedapp.ExampleApp) (component.Resource, er return nil }) - // 6. Build the final resource. + // 5. Build the final resource. return builder.Build() } diff --git a/internal/generic/builder_static_test.go b/internal/generic/builder_static_test.go index 816a4fa5..4e5bc09e 100644 --- a/internal/generic/builder_static_test.go +++ b/internal/generic/builder_static_test.go @@ -67,33 +67,6 @@ func TestStaticBuilder(t *testing.T) { require.EqualError(t, err, errClusterScopedNamespace) }) - t.Run("cluster-scoped build succeeds without namespace", func(t *testing.T) { - clusterObj := &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{Name: "cluster-obj"}, - } - builder := NewStaticBuilder(clusterObj, identityFunc, defaultApp, newMutator) - builder.MarkClusterScoped() - res, err := builder.Build() - if err != nil { - t.Fatalf("Build() error = %v", err) - } - if res.DesiredObject != clusterObj { - t.Errorf("expected object %v, got %v", clusterObj, res.DesiredObject) - } - }) - - t.Run("cluster-scoped build rejects non-empty namespace", func(t *testing.T) { - nsObj := &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{Name: "cluster-obj", Namespace: "oops"}, - } - builder := NewStaticBuilder(nsObj, identityFunc, defaultApp, newMutator) - builder.MarkClusterScoped() - _, err := builder.Build() - if err == nil || err.Error() != "cluster-scoped object must not have a namespace" { - t.Errorf("expected cluster-scoped namespace error, got %v", err) - } - }) - t.Run("validation errors", func(t *testing.T) { runBuilderValidationTests( t, diff --git a/pkg/primitives/clusterrole/mutator.go b/pkg/primitives/clusterrole/mutator.go index 77f1e853..79e5e2b6 100644 --- a/pkg/primitives/clusterrole/mutator.go +++ b/pkg/primitives/clusterrole/mutator.go @@ -11,38 +11,53 @@ import ( // only if its associated feature gate is enabled. type Mutation feature.Mutation[*Mutator] +type featurePlan struct { + metadataEdits []func(*editors.ObjectMetaEditor) error + rulesEdits []func(*editors.PolicyRulesEditor) error + aggregationRuleSets []*rbacv1.AggregationRule +} + // Mutator is a high-level helper for modifying a Kubernetes ClusterRole. // // It uses a "plan-and-apply" pattern: mutations are recorded first, then // applied to the ClusterRole in a single controlled pass when Apply() is called. // -// All mutation intents are collected into flat lists and applied in a fixed -// category order: metadata edits, then rules edits, then aggregation rule. -// Within each category, edits are applied in registration order. +// The Mutator maintains feature boundaries: each feature's mutations are planned +// together and applied in the order the features were registered. Within each +// feature, edits are applied in category order: metadata, then rules, then +// aggregation rule. // // Mutator implements editors.ObjectMutator. type Mutator struct { cr *rbacv1.ClusterRole - metadataEdits []func(*editors.ObjectMetaEditor) error - rulesEdits []func(*editors.PolicyRulesEditor) error - aggregationRuleSets []*rbacv1.AggregationRule + plans []featurePlan + active *featurePlan } // NewMutator creates a new Mutator for the given ClusterRole. func NewMutator(cr *rbacv1.ClusterRole) *Mutator { - return &Mutator{cr: cr} + m := &Mutator{cr: cr} + m.beginFeature() + return m +} + +// beginFeature starts a new feature planning scope. All subsequent mutation +// registrations will be grouped into this feature's plan. +func (m *Mutator) beginFeature() { + m.plans = append(m.plans, featurePlan{}) + m.active = &m.plans[len(m.plans)-1] } // EditObjectMetadata records a mutation for the ClusterRole's own metadata. // -// Metadata edits are applied before rules edits. +// Metadata edits are applied before rules edits within the same feature. // A nil edit function is ignored. func (m *Mutator) EditObjectMetadata(edit func(*editors.ObjectMetaEditor) error) { if edit == nil { return } - m.metadataEdits = append(m.metadataEdits, edit) + m.active.metadataEdits = append(m.active.metadataEdits, edit) } // EditRules records a mutation for the ClusterRole's .rules field via a @@ -50,14 +65,14 @@ func (m *Mutator) EditObjectMetadata(edit func(*editors.ObjectMetaEditor) error) // // The editor provides structured operations (AddRule, RemoveRuleByIndex, Clear) // as well as Raw() for free-form access. Rules edits are applied after metadata -// edits, in registration order. +// edits within the same feature, in registration order. // // A nil edit function is ignored. func (m *Mutator) EditRules(edit func(*editors.PolicyRulesEditor) error) { if edit == nil { return } - m.rulesEdits = append(m.rulesEdits, edit) + m.active.rulesEdits = append(m.active.rulesEdits, edit) } // AddRule records that a PolicyRule should be appended to .rules. @@ -75,44 +90,49 @@ func (m *Mutator) AddRule(rule rbacv1.PolicyRule) { // // An aggregation rule causes the API server to combine rules from ClusterRoles // whose labels match the provided selectors, instead of using .rules directly. -// If called multiple times, the last call wins. +// If called multiple times within the same feature, the last call wins. // // A nil value clears the aggregation rule. func (m *Mutator) SetAggregationRule(rule *rbacv1.AggregationRule) { - m.aggregationRuleSets = append(m.aggregationRuleSets, rule) + m.active.aggregationRuleSets = append(m.active.aggregationRuleSets, rule) } // Apply executes all recorded mutation intents on the underlying ClusterRole. // -// Execution order: +// Execution order across all registered features: +// +// 1. Metadata edits (in registration order within each feature) +// 2. Rules edits — EditRules, AddRule (in registration order within each feature) +// 3. Aggregation rule — SetAggregationRule (last call wins within each feature) // -// 1. Metadata edits (in registration order) -// 2. Rules edits — EditRules, AddRule (in registration order) -// 3. Aggregation rule — SetAggregationRule (last call wins) +// Features are applied in the order they were registered. Later features observe +// the ClusterRole as modified by all previous features. func (m *Mutator) Apply() error { - // 1. Metadata edits - if len(m.metadataEdits) > 0 { - editor := editors.NewObjectMetaEditor(&m.cr.ObjectMeta) - for _, edit := range m.metadataEdits { - if err := edit(editor); err != nil { - return err + for _, plan := range m.plans { + // 1. Metadata edits + if len(plan.metadataEdits) > 0 { + editor := editors.NewObjectMetaEditor(&m.cr.ObjectMeta) + for _, edit := range plan.metadataEdits { + if err := edit(editor); err != nil { + return err + } } } - } - // 2. Rules edits - if len(m.rulesEdits) > 0 { - editor := editors.NewPolicyRulesEditor(&m.cr.Rules) - for _, edit := range m.rulesEdits { - if err := edit(editor); err != nil { - return err + // 2. Rules edits + if len(plan.rulesEdits) > 0 { + editor := editors.NewPolicyRulesEditor(&m.cr.Rules) + for _, edit := range plan.rulesEdits { + if err := edit(editor); err != nil { + return err + } } } - } - // 3. Aggregation rule (last call wins) - if len(m.aggregationRuleSets) > 0 { - m.cr.AggregationRule = m.aggregationRuleSets[len(m.aggregationRuleSets)-1] + // 3. Aggregation rule (last call wins within this feature) + if len(plan.aggregationRuleSets) > 0 { + m.cr.AggregationRule = plan.aggregationRuleSets[len(plan.aggregationRuleSets)-1] + } } return nil diff --git a/pkg/primitives/clusterrole/mutator_test.go b/pkg/primitives/clusterrole/mutator_test.go index 089c64c5..cd71e78f 100644 --- a/pkg/primitives/clusterrole/mutator_test.go +++ b/pkg/primitives/clusterrole/mutator_test.go @@ -172,6 +172,60 @@ func TestMutator_MultipleMutations(t *testing.T) { assert.Equal(t, "services", cr.Rules[1].Resources[0]) } +// --- Feature boundaries --- + +func TestMutator_MultipleFeatures(t *testing.T) { + cr := newTestCR(nil) + m := NewMutator(cr) + + // Feature A: add a rule and a label + m.AddRule(rbacv1.PolicyRule{ + APIGroups: []string{""}, Resources: []string{"pods"}, Verbs: []string{"get"}, + }) + m.EditObjectMetadata(func(e *editors.ObjectMetaEditor) error { + e.EnsureLabel("feature-a", "true") + return nil + }) + + // Feature B: add another rule and a label + m.beginFeature() + m.AddRule(rbacv1.PolicyRule{ + APIGroups: []string{"apps"}, Resources: []string{"deployments"}, Verbs: []string{"list"}, + }) + m.EditObjectMetadata(func(e *editors.ObjectMetaEditor) error { + e.EnsureLabel("feature-b", "true") + return nil + }) + + require.NoError(t, m.Apply()) + + // Both features' edits applied in order + assert.Equal(t, "true", cr.Labels["feature-a"]) + assert.Equal(t, "true", cr.Labels["feature-b"]) + require.Len(t, cr.Rules, 2) + assert.Equal(t, "pods", cr.Rules[0].Resources[0]) + assert.Equal(t, "deployments", cr.Rules[1].Resources[0]) +} + +func TestMutator_FeatureOrder_MetadataBeforeRules(t *testing.T) { + // Within a single feature, metadata edits apply before rules edits, + // even when registered in the opposite order. + cr := newTestCR(nil) + m := NewMutator(cr) + + m.AddRule(rbacv1.PolicyRule{ + APIGroups: []string{""}, Resources: []string{"pods"}, Verbs: []string{"get"}, + }) + m.EditObjectMetadata(func(e *editors.ObjectMetaEditor) error { + e.EnsureLabel("order", "correct") + return nil + }) + + require.NoError(t, m.Apply()) + assert.Equal(t, "correct", cr.Labels["order"]) + require.Len(t, cr.Rules, 1) +} + // --- ObjectMutator interface --- func TestMutator_ImplementsObjectMutator(_ *testing.T) { From d60ed11b7e5f4af9a4f48e66a68d711e1e037c3f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= Date: Mon, 23 Mar 2026 16:02:39 +0000 Subject: [PATCH 11/28] Update ClusterRole docs to describe preserved fields in DefaultFieldApplicator Co-Authored-By: Claude Opus 4.6 (1M context) --- docs/primitives/clusterrole.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/primitives/clusterrole.md b/docs/primitives/clusterrole.md index c20ab9bb..c5c67ab5 100644 --- a/docs/primitives/clusterrole.md +++ b/docs/primitives/clusterrole.md @@ -42,7 +42,7 @@ resource, err := clusterrole.NewBuilder(base). ## Default Field Application -`DefaultFieldApplicator` replaces the current ClusterRole with a deep copy of the desired object. This ensures every reconciliation cycle produces a clean, predictable state and avoids any drift from the desired baseline. +`DefaultFieldApplicator` replaces the current ClusterRole with a deep copy of the desired object, then restores server-managed metadata (ResourceVersion, UID, etc.) and shared-controller fields (OwnerReferences, Finalizers) from the original live object. ClusterRole has no Status subresource, so no status preservation is needed. Use `WithCustomFieldApplicator` when other controllers manage fields that should not be overwritten: From ff378c19fa7cbfc16c2ff87e0a19b56f6206a71c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= Date: Mon, 23 Mar 2026 16:42:51 +0000 Subject: [PATCH 12/28] fix: clusterrole mutator constructor should not call beginFeature Initialize plans slice and active pointer directly in the constructor to avoid creating an empty feature when mutate_helper.go calls beginFeature() on the first feature. Mirrors the fix applied to deployment and configmap mutators in #42. Co-Authored-By: Claude Opus 4.6 (1M context) --- pkg/primitives/clusterrole/mutator.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/pkg/primitives/clusterrole/mutator.go b/pkg/primitives/clusterrole/mutator.go index 79e5e2b6..f8377594 100644 --- a/pkg/primitives/clusterrole/mutator.go +++ b/pkg/primitives/clusterrole/mutator.go @@ -37,8 +37,11 @@ type Mutator struct { // NewMutator creates a new Mutator for the given ClusterRole. func NewMutator(cr *rbacv1.ClusterRole) *Mutator { - m := &Mutator{cr: cr} - m.beginFeature() + m := &Mutator{ + cr: cr, + plans: []featurePlan{{}}, + } + m.active = &m.plans[0] return m } From a604af1350c382fb7f99618ec73b87bc5e2493bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= Date: Mon, 23 Mar 2026 16:59:36 +0000 Subject: [PATCH 13/28] fix: address Copilot review comments on ClusterRole primitive - Use set-based (order-independent) comparison for PolicyRule slices in PreserveExternalRules to avoid duplicating rules after webhook normalization - DeepCopy AggregationRule in SetAggregationRule to prevent caller mutations from affecting recorded intent - Remove duplicate nil/name validation in Build(), delegating to generic.StaticBuilder.ValidateBase() consistent with ConfigMap primitive - Add tests for reordered-but-equivalent rules and aggregation rule immutability Co-Authored-By: Claude Opus 4.6 (1M context) --- pkg/primitives/clusterrole/builder.go | 8 ------ pkg/primitives/clusterrole/flavors.go | 33 +++++++++++++++------- pkg/primitives/clusterrole/flavors_test.go | 16 +++++++++++ pkg/primitives/clusterrole/mutator.go | 6 +++- pkg/primitives/clusterrole/mutator_test.go | 19 +++++++++++++ 5 files changed, 63 insertions(+), 19 deletions(-) diff --git a/pkg/primitives/clusterrole/builder.go b/pkg/primitives/clusterrole/builder.go index a7f00224..6bceef6c 100644 --- a/pkg/primitives/clusterrole/builder.go +++ b/pkg/primitives/clusterrole/builder.go @@ -1,7 +1,6 @@ package clusterrole import ( - "errors" "fmt" "github.com/sourcehawk/operator-component-framework/internal/generic" @@ -98,13 +97,6 @@ func (b *Builder) WithDataExtractor(extractor func(rbacv1.ClusterRole) error) *B // - No ClusterRole object was provided. // - The ClusterRole is missing a Name. func (b *Builder) Build() (*Resource, error) { - if b.obj == nil { - return nil, errors.New("object cannot be nil") - } - if b.obj.Name == "" { - return nil, errors.New("object name cannot be empty") - } - identityFunc := func(cr *rbacv1.ClusterRole) string { return fmt.Sprintf("rbac.authorization.k8s.io/v1/ClusterRole/%s", cr.Name) } diff --git a/pkg/primitives/clusterrole/flavors.go b/pkg/primitives/clusterrole/flavors.go index 1f78810c..eadb6668 100644 --- a/pkg/primitives/clusterrole/flavors.go +++ b/pkg/primitives/clusterrole/flavors.go @@ -1,6 +1,8 @@ package clusterrole import ( + "sort" + "github.com/sourcehawk/operator-component-framework/pkg/flavors" rbacv1 "k8s.io/api/rbac/v1" ) @@ -50,22 +52,33 @@ func containsRule(rules []rbacv1.PolicyRule, target rbacv1.PolicyRule) bool { return false } -// policyRuleEqual reports whether two PolicyRules are equal by comparing all fields. +// policyRuleEqual reports whether two PolicyRules are equal by comparing all +// fields as sets, so that different orderings of the same elements are treated +// as equivalent. func policyRuleEqual(a, b rbacv1.PolicyRule) bool { - return stringSliceEqual(a.Verbs, b.Verbs) && - stringSliceEqual(a.APIGroups, b.APIGroups) && - stringSliceEqual(a.Resources, b.Resources) && - stringSliceEqual(a.ResourceNames, b.ResourceNames) && - stringSliceEqual(a.NonResourceURLs, b.NonResourceURLs) + return stringSliceEqualUnordered(a.Verbs, b.Verbs) && + stringSliceEqualUnordered(a.APIGroups, b.APIGroups) && + stringSliceEqualUnordered(a.Resources, b.Resources) && + stringSliceEqualUnordered(a.ResourceNames, b.ResourceNames) && + stringSliceEqualUnordered(a.NonResourceURLs, b.NonResourceURLs) } -// stringSliceEqual reports whether two string slices are equal. -func stringSliceEqual(a, b []string) bool { +// stringSliceEqualUnordered reports whether two string slices contain exactly +// the same elements, regardless of order. +func stringSliceEqualUnordered(a, b []string) bool { if len(a) != len(b) { return false } - for i := range a { - if a[i] != b[i] { + aSorted := make([]string, len(a)) + copy(aSorted, a) + sort.Strings(aSorted) + + bSorted := make([]string, len(b)) + copy(bSorted, b) + sort.Strings(bSorted) + + for i := range aSorted { + if aSorted[i] != bSorted[i] { return false } } diff --git a/pkg/primitives/clusterrole/flavors_test.go b/pkg/primitives/clusterrole/flavors_test.go index 812994d2..8cec0270 100644 --- a/pkg/primitives/clusterrole/flavors_test.go +++ b/pkg/primitives/clusterrole/flavors_test.go @@ -96,6 +96,22 @@ func TestPreserveExternalRules(t *testing.T) { assert.Len(t, applied.Rules, 1) }) + t.Run("treats reordered-but-equivalent rules as equal", func(t *testing.T) { + applied := &rbacv1.ClusterRole{ + Rules: []rbacv1.PolicyRule{ + {APIGroups: []string{""}, Resources: []string{"pods", "services"}, Verbs: []string{"get", "list"}}, + }, + } + current := &rbacv1.ClusterRole{ + Rules: []rbacv1.PolicyRule{ + {APIGroups: []string{""}, Resources: []string{"services", "pods"}, Verbs: []string{"list", "get"}}, + }, + } + + require.NoError(t, PreserveExternalRules(applied, current, nil)) + assert.Len(t, applied.Rules, 1, "reordered rule should not be duplicated") + }) + t.Run("no-op when current has no rules", func(t *testing.T) { applied := &rbacv1.ClusterRole{ Rules: []rbacv1.PolicyRule{ diff --git a/pkg/primitives/clusterrole/mutator.go b/pkg/primitives/clusterrole/mutator.go index f8377594..9870133e 100644 --- a/pkg/primitives/clusterrole/mutator.go +++ b/pkg/primitives/clusterrole/mutator.go @@ -97,7 +97,11 @@ func (m *Mutator) AddRule(rule rbacv1.PolicyRule) { // // A nil value clears the aggregation rule. func (m *Mutator) SetAggregationRule(rule *rbacv1.AggregationRule) { - m.active.aggregationRuleSets = append(m.active.aggregationRuleSets, rule) + var copied *rbacv1.AggregationRule + if rule != nil { + copied = rule.DeepCopy() + } + m.active.aggregationRuleSets = append(m.active.aggregationRuleSets, copied) } // Apply executes all recorded mutation intents on the underlying ClusterRole. diff --git a/pkg/primitives/clusterrole/mutator_test.go b/pkg/primitives/clusterrole/mutator_test.go index cd71e78f..75f1c89c 100644 --- a/pkg/primitives/clusterrole/mutator_test.go +++ b/pkg/primitives/clusterrole/mutator_test.go @@ -136,6 +136,25 @@ func TestMutator_SetAggregationRule_LastWins(t *testing.T) { assert.Equal(t, "true", cr.AggregationRule.ClusterRoleSelectors[0].MatchLabels["second"]) } +func TestMutator_SetAggregationRule_Immutable(t *testing.T) { + cr := newTestCR(nil) + m := NewMutator(cr) + aggRule := &rbacv1.AggregationRule{ + ClusterRoleSelectors: []metav1.LabelSelector{ + {MatchLabels: map[string]string{"aggregate": "true"}}, + }, + } + m.SetAggregationRule(aggRule) + + // Mutate the original after registration — should not affect the mutator. + aggRule.ClusterRoleSelectors[0].MatchLabels["aggregate"] = "mutated" + + require.NoError(t, m.Apply()) + require.NotNil(t, cr.AggregationRule) + assert.Equal(t, "true", cr.AggregationRule.ClusterRoleSelectors[0].MatchLabels["aggregate"], + "mutating the original after SetAggregationRule should not affect the applied result") +} + // --- Execution order --- func TestMutator_OperationOrder(t *testing.T) { From 69f9bd38a0e991da3c3023fd87f0aff3f41d7e19 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= Date: Mon, 23 Mar 2026 21:33:33 +0000 Subject: [PATCH 14/28] fix: export BeginFeature() to satisfy FeatureMutator interface After rebasing on main, the clusterrole mutator still had the unexported beginFeature() method. Rename to BeginFeature() to align with the exported FeatureMutator interface in internal/generic/resource_workload.go. Co-Authored-By: Claude Opus 4.6 (1M context) --- pkg/primitives/clusterrole/mutator.go | 4 ++-- pkg/primitives/clusterrole/mutator_test.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/primitives/clusterrole/mutator.go b/pkg/primitives/clusterrole/mutator.go index 9870133e..65392228 100644 --- a/pkg/primitives/clusterrole/mutator.go +++ b/pkg/primitives/clusterrole/mutator.go @@ -45,9 +45,9 @@ func NewMutator(cr *rbacv1.ClusterRole) *Mutator { return m } -// beginFeature starts a new feature planning scope. All subsequent mutation +// BeginFeature starts a new feature planning scope. All subsequent mutation // registrations will be grouped into this feature's plan. -func (m *Mutator) beginFeature() { +func (m *Mutator) BeginFeature() { m.plans = append(m.plans, featurePlan{}) m.active = &m.plans[len(m.plans)-1] } diff --git a/pkg/primitives/clusterrole/mutator_test.go b/pkg/primitives/clusterrole/mutator_test.go index 75f1c89c..8955ac27 100644 --- a/pkg/primitives/clusterrole/mutator_test.go +++ b/pkg/primitives/clusterrole/mutator_test.go @@ -207,7 +207,7 @@ func TestMutator_MultipleFeatures(t *testing.T) { }) // Feature B: add another rule and a label - m.beginFeature() + m.BeginFeature() m.AddRule(rbacv1.PolicyRule{ APIGroups: []string{"apps"}, Resources: []string{"deployments"}, Verbs: []string{"list"}, }) From f51c9340280567cba68ccf52b53b60c673ad437e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= Date: Tue, 24 Mar 2026 00:29:06 +0000 Subject: [PATCH 15/28] style: format markdown files with prettier Co-Authored-By: Claude Opus 4.6 (1M context) --- docs/primitives/clusterrole.md | 95 ++++++++++++++++-------- examples/clusterrole-primitive/README.md | 13 ++-- 2 files changed, 71 insertions(+), 37 deletions(-) diff --git a/docs/primitives/clusterrole.md b/docs/primitives/clusterrole.md index c5c67ab5..d7e391dd 100644 --- a/docs/primitives/clusterrole.md +++ b/docs/primitives/clusterrole.md @@ -1,20 +1,28 @@ # ClusterRole Primitive -The `clusterrole` primitive is the framework's built-in static abstraction for managing Kubernetes `ClusterRole` resources. It integrates with the component lifecycle and provides a structured mutation API for managing `.rules`, `.aggregationRule`, and object metadata. +The `clusterrole` primitive is the framework's built-in static abstraction for managing Kubernetes `ClusterRole` +resources. It integrates with the component lifecycle and provides a structured mutation API for managing `.rules`, +`.aggregationRule`, and object metadata. -ClusterRole is cluster-scoped: it has no namespace. The builder validates that the Name is set and that Namespace is empty — setting a namespace on a cluster-scoped resource is rejected. +ClusterRole is cluster-scoped: it has no namespace. The builder validates that the Name is set and that Namespace is +empty — setting a namespace on a cluster-scoped resource is rejected. -> **Ownership limitation:** During reconciliation, the framework attempts to set a controller reference on managed objects, but only when the owner and dependent scopes are compatible. When a namespaced owner manages a cluster-scoped resource such as a `ClusterRole`, the owner reference is skipped (and this is logged) instead of causing the reconcile to fail. In this case, the `ClusterRole` is **not** owned by the custom resource for Kubernetes garbage-collection or ownership semantics, so it will not be automatically deleted when the owner is removed; you must handle its lifecycle explicitly or use a cluster-scoped owner if automatic cleanup is required. +> **Ownership limitation:** During reconciliation, the framework attempts to set a controller reference on managed +> objects, but only when the owner and dependent scopes are compatible. When a namespaced owner manages a cluster-scoped +> resource such as a `ClusterRole`, the owner reference is skipped (and this is logged) instead of causing the reconcile +> to fail. In this case, the `ClusterRole` is **not** owned by the custom resource for Kubernetes garbage-collection or +> ownership semantics, so it will not be automatically deleted when the owner is removed; you must handle its lifecycle +> explicitly or use a cluster-scoped owner if automatic cleanup is required. ## Capabilities -| Capability | Detail | -|-----------------------|---------------------------------------------------------------------------------------------------------------| -| **Static lifecycle** | No health tracking, grace periods, or suspension — the resource is reconciled to desired state | -| **Mutation pipeline** | Typed editors for `.rules` and object metadata, with aggregation rule support and a raw escape hatch | -| **Cluster-scoped** | No namespace required — identity format is `rbac.authorization.k8s.io/v1/ClusterRole/` | -| **Flavors** | Preserves externally-managed fields — labels, annotations, and `.rules` entries not owned by the operator | -| **Data extraction** | Reads generated or updated values back from the reconciled ClusterRole after each sync cycle | +| Capability | Detail | +| --------------------- | --------------------------------------------------------------------------------------------------------- | +| **Static lifecycle** | No health tracking, grace periods, or suspension — the resource is reconciled to desired state | +| **Mutation pipeline** | Typed editors for `.rules` and object metadata, with aggregation rule support and a raw escape hatch | +| **Cluster-scoped** | No namespace required — identity format is `rbac.authorization.k8s.io/v1/ClusterRole/` | +| **Flavors** | Preserves externally-managed fields — labels, annotations, and `.rules` entries not owned by the operator | +| **Data extraction** | Reads generated or updated values back from the reconciled ClusterRole after each sync cycle | ## Building a ClusterRole Primitive @@ -42,7 +50,9 @@ resource, err := clusterrole.NewBuilder(base). ## Default Field Application -`DefaultFieldApplicator` replaces the current ClusterRole with a deep copy of the desired object, then restores server-managed metadata (ResourceVersion, UID, etc.) and shared-controller fields (OwnerReferences, Finalizers) from the original live object. ClusterRole has no Status subresource, so no status preservation is needed. +`DefaultFieldApplicator` replaces the current ClusterRole with a deep copy of the desired object, then restores +server-managed metadata (ResourceVersion, UID, etc.) and shared-controller fields (OwnerReferences, Finalizers) from the +original live object. ClusterRole has no Status subresource, so no status preservation is needed. Use `WithCustomFieldApplicator` when other controllers manage fields that should not be overwritten: @@ -58,7 +68,8 @@ resource, err := clusterrole.NewBuilder(base). ## Mutations -Mutations are the primary mechanism for modifying a `ClusterRole` beyond its baseline. Each mutation is a named function that receives a `*Mutator` and records edit intent through typed editors. +Mutations are the primary mechanism for modifying a `ClusterRole` beyond its baseline. Each mutation is a named function +that receives a `*Mutator` and records edit intent through typed editors. The `Feature` field controls when a mutation applies. Leaving it nil applies the mutation unconditionally: @@ -127,15 +138,18 @@ All version constraints and `When()` conditions must be satisfied for a mutation ## Internal Mutation Ordering -The Mutator maintains feature boundaries: each feature's mutations are planned together and applied in the order the features were registered. Within each feature, edits are applied in a fixed category order: +The Mutator maintains feature boundaries: each feature's mutations are planned together and applied in the order the +features were registered. Within each feature, edits are applied in a fixed category order: -| Step | Category | What it affects | -|------|-------------------|-------------------------------------------------| -| 1 | Metadata edits | Labels and annotations on the `ClusterRole` | -| 2 | Rules edits | `.rules` entries — EditRules, AddRule | -| 3 | Aggregation rule | `.aggregationRule` — SetAggregationRule | +| Step | Category | What it affects | +| ---- | ---------------- | ------------------------------------------- | +| 1 | Metadata edits | Labels and annotations on the `ClusterRole` | +| 2 | Rules edits | `.rules` entries — EditRules, AddRule | +| 3 | Aggregation rule | `.aggregationRule` — SetAggregationRule | -Within each category, edits are applied in their registration order. For aggregation rules, the last `SetAggregationRule` call wins within each feature. Later features observe the ClusterRole as modified by all previous features. +Within each category, edits are applied in their registration order. For aggregation rules, the last +`SetAggregationRule` call wins within each feature. Later features observe the ClusterRole as modified by all previous +features. ## Editors @@ -221,15 +235,17 @@ m.EditObjectMetadata(func(e *editors.ObjectMetaEditor) error { The `Mutator` exposes a convenience wrapper for the most common `.rules` operation: -| Method | Equivalent to | -|-----------------|--------------------------------------| -| `AddRule(rule)` | `EditRules` → `e.AddRule(rule)` | +| Method | Equivalent to | +| --------------- | ------------------------------- | +| `AddRule(rule)` | `EditRules` → `e.AddRule(rule)` | -Use `AddRule` for simple, single-rule mutations. Use `EditRules` when you need multiple operations or raw access in a single edit block. +Use `AddRule` for simple, single-rule mutations. Use `EditRules` when you need multiple operations or raw access in a +single edit block. ## SetAggregationRule -`SetAggregationRule` sets the ClusterRole's `.aggregationRule` field. An aggregation rule causes the API server to combine rules from ClusterRoles whose labels match the provided selectors, instead of using `.rules` directly: +`SetAggregationRule` sets the ClusterRole's `.aggregationRule` field. An aggregation rule causes the API server to +combine rules from ClusterRoles whose labels match the provided selectors, instead of using `.rules` directly: ```go m.SetAggregationRule(&rbacv1.AggregationRule{ @@ -243,7 +259,8 @@ Setting the aggregation rule to nil clears it. Within a single feature, the last ## Flavors -Flavors run after the baseline applicator and before mutations. They are used to preserve fields managed by external controllers or other tools. +Flavors run after the baseline applicator and before mutations. They are used to preserve fields managed by external +controllers or other tools. ### PreserveCurrentLabels @@ -257,7 +274,8 @@ resource, err := clusterrole.NewBuilder(base). ### PreserveCurrentAnnotations -Preserves annotations present on the live object but absent from the applied desired state. Applied annotations win on overlap. +Preserves annotations present on the live object but absent from the applied desired state. Applied annotations win on +overlap. ```go resource, err := clusterrole.NewBuilder(base). @@ -267,7 +285,8 @@ resource, err := clusterrole.NewBuilder(base). ### PreserveExternalRules -Preserves `.rules` entries present on the live object but absent from the applied desired state. Rules are compared by full PolicyRule equality (all fields: APIGroups, Resources, Verbs, ResourceNames, NonResourceURLs). +Preserves `.rules` entries present on the live object but absent from the applied desired state. Rules are compared by +full PolicyRule equality (all fields: APIGroups, Resources, Verbs, ResourceNames, NonResourceURLs). Use this when other controllers or admission webhooks inject rules into the ClusterRole that your operator does not own: @@ -318,16 +337,28 @@ resource, err := clusterrole.NewBuilder(base). Build() ``` -When `ManageCRDs` is true, the final rules include both core and CRD access rules. When false, only the core rules are written. Neither mutation needs to know about the other. +When `ManageCRDs` is true, the final rules include both core and CRD access rules. When false, only the core rules are +written. Neither mutation needs to know about the other. -> **Note:** Do not combine `PreserveExternalRules` with feature-gated mutations that add and remove rules. Because flavors run before mutations and preserve rules from the live object, previously-added rules will be retained even after a feature gate is disabled, and rules can be duplicated if a mutation re-adds a rule already present on the live object. Use `PreserveExternalRules` only when external controllers or admission webhooks manage rules that your operator does not own. +> **Note:** Do not combine `PreserveExternalRules` with feature-gated mutations that add and remove rules. Because +> flavors run before mutations and preserve rules from the live object, previously-added rules will be retained even +> after a feature gate is disabled, and rules can be duplicated if a mutation re-adds a rule already present on the live +> object. Use `PreserveExternalRules` only when external controllers or admission webhooks manage rules that your +> operator does not own. ## Guidance -**`Feature: nil` applies unconditionally.** Omit `Feature` (leave it nil) for mutations that should always run. Use `feature.NewResourceFeature(version, constraints)` when version-based gating is needed, and chain `.When(bool)` for boolean conditions. +**`Feature: nil` applies unconditionally.** Omit `Feature` (leave it nil) for mutations that should always run. Use +`feature.NewResourceFeature(version, constraints)` when version-based gating is needed, and chain `.When(bool)` for +boolean conditions. -**Use `PreserveExternalRules` when sharing a ClusterRole.** If admission webhooks, external controllers, or manual operations add rules to a ClusterRole your operator manages, this flavor prevents your operator from silently deleting those rules each reconcile cycle. +**Use `PreserveExternalRules` when sharing a ClusterRole.** If admission webhooks, external controllers, or manual +operations add rules to a ClusterRole your operator manages, this flavor prevents your operator from silently deleting +those rules each reconcile cycle. -**Use `SetAggregationRule` for composite roles.** When you want the API server to aggregate rules from multiple ClusterRoles based on label selectors, use `SetAggregationRule` instead of managing `.rules` directly. The two approaches are mutually exclusive in the Kubernetes API — the API server ignores `.rules` when `.aggregationRule` is set. +**Use `SetAggregationRule` for composite roles.** When you want the API server to aggregate rules from multiple +ClusterRoles based on label selectors, use `SetAggregationRule` instead of managing `.rules` directly. The two +approaches are mutually exclusive in the Kubernetes API — the API server ignores `.rules` when `.aggregationRule` is +set. **Register mutations in dependency order.** If mutation B relies on a rule added by mutation A, register A first. diff --git a/examples/clusterrole-primitive/README.md b/examples/clusterrole-primitive/README.md index 5999535b..1d3457c3 100644 --- a/examples/clusterrole-primitive/README.md +++ b/examples/clusterrole-primitive/README.md @@ -1,7 +1,7 @@ # ClusterRole Primitive Example -This example demonstrates the usage of the `clusterrole` primitive within the operator component framework. -It shows how to manage a Kubernetes ClusterRole as a component of a larger application, utilising features like: +This example demonstrates the usage of the `clusterrole` primitive within the operator component framework. It shows how +to manage a Kubernetes ClusterRole as a component of a larger application, utilising features like: - **Base Construction**: Initializing a cluster-scoped ClusterRole with basic metadata. - **Feature Mutations**: Composing RBAC rules from independent, feature-gated mutations using `AddRule`. @@ -10,10 +10,12 @@ It shows how to manage a Kubernetes ClusterRole as a component of a larger appli ## Directory Structure -- `app/`: Defines the controller that uses the component framework. The `ExampleApp` CRD is shared from `examples/shared/app`. +- `app/`: Defines the controller that uses the component framework. The `ExampleApp` CRD is shared from + `examples/shared/app`. - `features/`: Contains modular feature definitions: - - `mutations.go`: core rules, version labelling, and feature-gated secret and deployment access. -- `resources/`: Contains the central `NewClusterRoleResource` factory that assembles all features using `clusterrole.Builder`. + - `mutations.go`: core rules, version labelling, and feature-gated secret and deployment access. +- `resources/`: Contains the central `NewClusterRoleResource` factory that assembles all features using + `clusterrole.Builder`. - `main.go`: A standalone entry point that demonstrates multiple reconciliation cycles with a fake client. ## Running the Example @@ -23,6 +25,7 @@ go run examples/clusterrole-primitive/main.go ``` This will: + 1. Initialize a fake Kubernetes client. 2. Create an `ExampleApp` owner object. 3. Reconcile through four spec variations, printing the composed rules after each cycle. From 717ddca35982aaf37921a3167eba8e905a51267e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= Date: Tue, 24 Mar 2026 00:33:54 +0000 Subject: [PATCH 16/28] docs: clarify PolicyRule comparison uses order-insensitive set semantics Update comments and documentation for PreserveExternalRules to accurately describe that slice fields are compared as unordered sets. Fix GoDoc capitalization of ClusterRole in Mutation type comment. Co-Authored-By: Claude Opus 4.6 (1M context) --- docs/primitives/clusterrole.md | 5 +++-- pkg/primitives/clusterrole/flavors.go | 3 ++- pkg/primitives/clusterrole/mutator.go | 2 +- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/docs/primitives/clusterrole.md b/docs/primitives/clusterrole.md index d7e391dd..3b3fce38 100644 --- a/docs/primitives/clusterrole.md +++ b/docs/primitives/clusterrole.md @@ -285,8 +285,9 @@ resource, err := clusterrole.NewBuilder(base). ### PreserveExternalRules -Preserves `.rules` entries present on the live object but absent from the applied desired state. Rules are compared by -full PolicyRule equality (all fields: APIGroups, Resources, Verbs, ResourceNames, NonResourceURLs). +Preserves `.rules` entries present on the live object but absent from the applied desired state. Rules are compared using +all `PolicyRule` fields (APIGroups, Resources, Verbs, ResourceNames, NonResourceURLs), treating these slice fields as +sets so that order differences are ignored. Use this when other controllers or admission webhooks inject rules into the ClusterRole that your operator does not own: diff --git a/pkg/primitives/clusterrole/flavors.go b/pkg/primitives/clusterrole/flavors.go index eadb6668..a371ecbb 100644 --- a/pkg/primitives/clusterrole/flavors.go +++ b/pkg/primitives/clusterrole/flavors.go @@ -32,7 +32,8 @@ func PreserveCurrentAnnotations(applied, current, desired *rbacv1.ClusterRole) e // // This is useful when other controllers or admission webhooks inject rules into // the ClusterRole that your operator does not own. Rules present in both are -// determined by equality of the entire PolicyRule struct. +// determined by comparing all PolicyRule fields with slice fields treated as +// unordered sets, so different orderings of the same elements are considered equal. func PreserveExternalRules(applied, current, _ *rbacv1.ClusterRole) error { for _, currentRule := range current.Rules { if !containsRule(applied.Rules, currentRule) { diff --git a/pkg/primitives/clusterrole/mutator.go b/pkg/primitives/clusterrole/mutator.go index 65392228..bcf12e16 100644 --- a/pkg/primitives/clusterrole/mutator.go +++ b/pkg/primitives/clusterrole/mutator.go @@ -7,7 +7,7 @@ import ( rbacv1 "k8s.io/api/rbac/v1" ) -// Mutation defines a mutation that is applied to a clusterrole Mutator +// Mutation defines a mutation that is applied to a ClusterRole Mutator // only if its associated feature gate is enabled. type Mutation feature.Mutation[*Mutator] From cf4218a6b9493433ba6a802bc16b6334ed9f430b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= Date: Tue, 24 Mar 2026 00:42:49 +0000 Subject: [PATCH 17/28] refactor: wrap generic.StaticBuilder in clusterrole.Builder Delegate state and With* calls directly to the generic builder instead of storing mutations/flavors/extractors locally and replaying them in Build(). This matches the pattern used by other primitives (configmap, deployment) and avoids duplicated plumbing that could drift. Co-Authored-By: Claude Opus 4.6 (1M context) --- docs/primitives/clusterrole.md | 6 +-- pkg/primitives/clusterrole/builder.go | 59 +++++++++------------------ 2 files changed, 22 insertions(+), 43 deletions(-) diff --git a/docs/primitives/clusterrole.md b/docs/primitives/clusterrole.md index 3b3fce38..fe059c7f 100644 --- a/docs/primitives/clusterrole.md +++ b/docs/primitives/clusterrole.md @@ -285,9 +285,9 @@ resource, err := clusterrole.NewBuilder(base). ### PreserveExternalRules -Preserves `.rules` entries present on the live object but absent from the applied desired state. Rules are compared using -all `PolicyRule` fields (APIGroups, Resources, Verbs, ResourceNames, NonResourceURLs), treating these slice fields as -sets so that order differences are ignored. +Preserves `.rules` entries present on the live object but absent from the applied desired state. Rules are compared +using all `PolicyRule` fields (APIGroups, Resources, Verbs, ResourceNames, NonResourceURLs), treating these slice fields +as sets so that order differences are ignored. Use this when other controllers or admission webhooks inject rules into the ClusterRole that your operator does not own: diff --git a/pkg/primitives/clusterrole/builder.go b/pkg/primitives/clusterrole/builder.go index 6bceef6c..5954fb92 100644 --- a/pkg/primitives/clusterrole/builder.go +++ b/pkg/primitives/clusterrole/builder.go @@ -14,11 +14,7 @@ import ( // and data extractors. Build() validates the configuration and returns an // initialized Resource ready for use in a reconciliation loop. type Builder struct { - obj *rbacv1.ClusterRole - mutations []feature.Mutation[*Mutator] - applicator func(current, desired *rbacv1.ClusterRole) error - flavors []generic.FieldApplicationFlavor[*rbacv1.ClusterRole] - extractors []func(*rbacv1.ClusterRole) error + base *generic.StaticBuilder[*rbacv1.ClusterRole, *Mutator] } // NewBuilder initializes a new Builder with the provided ClusterRole object. @@ -30,7 +26,19 @@ type Builder struct { // The provided ClusterRole must have Name set (ClusterRole is cluster-scoped and // does not use a namespace), which is validated during the Build() call. func NewBuilder(cr *rbacv1.ClusterRole) *Builder { - return &Builder{obj: cr} + identityFunc := func(cr *rbacv1.ClusterRole) string { + return fmt.Sprintf("rbac.authorization.k8s.io/v1/ClusterRole/%s", cr.Name) + } + + sb := generic.NewStaticBuilder[*rbacv1.ClusterRole, *Mutator]( + cr, + identityFunc, + DefaultFieldApplicator, + NewMutator, + ) + sb.MarkClusterScoped() + + return &Builder{base: sb} } // WithMutation registers a mutation for the ClusterRole. @@ -40,7 +48,7 @@ func NewBuilder(cr *rbacv1.ClusterRole) *Builder { // A mutation with a nil Feature is applied unconditionally; one with a non-nil // Feature is applied only when that feature is enabled. func (b *Builder) WithMutation(m Mutation) *Builder { - b.mutations = append(b.mutations, feature.Mutation[*Mutator](m)) + b.base.WithMutation(feature.Mutation[*Mutator](m)) return b } @@ -57,7 +65,7 @@ func (b *Builder) WithMutation(m Mutation) *Builder { func (b *Builder) WithCustomFieldApplicator( applicator func(current, desired *rbacv1.ClusterRole) error, ) *Builder { - b.applicator = applicator + b.base.WithCustomFieldApplicator(applicator) return b } @@ -70,7 +78,7 @@ func (b *Builder) WithCustomFieldApplicator( // A nil flavor is ignored. func (b *Builder) WithFieldApplicationFlavor(flavor FieldApplicationFlavor) *Builder { if flavor != nil { - b.flavors = append(b.flavors, generic.FieldApplicationFlavor[*rbacv1.ClusterRole](flavor)) + b.base.WithFieldApplicationFlavor(generic.FieldApplicationFlavor[*rbacv1.ClusterRole](flavor)) } return b } @@ -84,7 +92,7 @@ func (b *Builder) WithFieldApplicationFlavor(flavor FieldApplicationFlavor) *Bui // A nil extractor is ignored. func (b *Builder) WithDataExtractor(extractor func(rbacv1.ClusterRole) error) *Builder { if extractor != nil { - b.extractors = append(b.extractors, func(cr *rbacv1.ClusterRole) error { + b.base.WithDataExtractor(func(cr *rbacv1.ClusterRole) error { return extractor(*cr) }) } @@ -97,38 +105,9 @@ func (b *Builder) WithDataExtractor(extractor func(rbacv1.ClusterRole) error) *B // - No ClusterRole object was provided. // - The ClusterRole is missing a Name. func (b *Builder) Build() (*Resource, error) { - identityFunc := func(cr *rbacv1.ClusterRole) string { - return fmt.Sprintf("rbac.authorization.k8s.io/v1/ClusterRole/%s", cr.Name) - } - - sb := generic.NewStaticBuilder[*rbacv1.ClusterRole, *Mutator]( - b.obj, - identityFunc, - DefaultFieldApplicator, - NewMutator, - ) - sb.MarkClusterScoped() - - for _, m := range b.mutations { - sb.WithMutation(m) - } - - if b.applicator != nil { - sb.WithCustomFieldApplicator(b.applicator) - } - - for _, f := range b.flavors { - sb.WithFieldApplicationFlavor(f) - } - - for _, e := range b.extractors { - sb.WithDataExtractor(e) - } - - res, err := sb.Build() + res, err := b.base.Build() if err != nil { return nil, err } - return &Resource{base: res}, nil } From 8d8bdfccfb9490093feff92384ee271aebc06ffc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= Date: Tue, 24 Mar 2026 01:25:28 +0000 Subject: [PATCH 18/28] =?UTF-8?q?fix:=20address=20PR=20review=20=E2=80=94?= =?UTF-8?q?=20nil=20Feature=20for=20unconditional=20mutations,=20update=20?= =?UTF-8?q?docs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - CoreRulesMutation and VersionLabelMutation now use nil Feature (truly unconditional) instead of a no-op NewResourceFeature(version, nil) - Add ClusterRole to the Built-in Primitives table in primitives.md - Add PolicyRulesEditor to the Mutation Editors table in primitives.md - Mention PolicyRulesEditor in clusterrole.md capabilities table Co-Authored-By: Claude Opus 4.6 (1M context) --- docs/primitives.md | 10 +++++---- docs/primitives/clusterrole.md | 21 +++++++++---------- .../features/mutations.go | 12 +++++------ .../resources/clusterrole.go | 2 +- 4 files changed, 22 insertions(+), 23 deletions(-) diff --git a/docs/primitives.md b/docs/primitives.md index 985288f1..79876db4 100644 --- a/docs/primitives.md +++ b/docs/primitives.md @@ -120,6 +120,7 @@ Editors provide scoped, typed APIs for modifying specific parts of a resource: | `PodSpecEditor` | Volumes, tolerations, node selectors, service account, security context | | `DeploymentSpecEditor` | Replicas, update strategy, label selectors | | `ConfigMapDataEditor` | `.data` entries — set, remove, deep-merge YAML patches, raw access | +| `PolicyRulesEditor` | `.rules` entries on a ClusterRole — add, remove, clear, raw access | | `ObjectMetaEditor` | Labels and annotations on any Kubernetes object | Every editor exposes a `.Raw()` method for cases where the typed API is insufficient, giving direct access to the @@ -141,10 +142,11 @@ have been applied. This means a single mutation can safely add a container and t ## Built-in Primitives -| Primitive | Category | Documentation | -| --------------------------- | -------- | ----------------------------------------- | -| `pkg/primitives/deployment` | Workload | [deployment.md](primitives/deployment.md) | -| `pkg/primitives/configmap` | Static | [configmap.md](primitives/configmap.md) | +| Primitive | Category | Documentation | +| ---------------------------- | -------- | ------------------------------------------- | +| `pkg/primitives/deployment` | Workload | [deployment.md](primitives/deployment.md) | +| `pkg/primitives/configmap` | Static | [configmap.md](primitives/configmap.md) | +| `pkg/primitives/clusterrole` | Static | [clusterrole.md](primitives/clusterrole.md) | ## Usage Examples diff --git a/docs/primitives/clusterrole.md b/docs/primitives/clusterrole.md index fe059c7f..ff48f59b 100644 --- a/docs/primitives/clusterrole.md +++ b/docs/primitives/clusterrole.md @@ -16,13 +16,13 @@ empty — setting a namespace on a cluster-scoped resource is rejected. ## Capabilities -| Capability | Detail | -| --------------------- | --------------------------------------------------------------------------------------------------------- | -| **Static lifecycle** | No health tracking, grace periods, or suspension — the resource is reconciled to desired state | -| **Mutation pipeline** | Typed editors for `.rules` and object metadata, with aggregation rule support and a raw escape hatch | -| **Cluster-scoped** | No namespace required — identity format is `rbac.authorization.k8s.io/v1/ClusterRole/` | -| **Flavors** | Preserves externally-managed fields — labels, annotations, and `.rules` entries not owned by the operator | -| **Data extraction** | Reads generated or updated values back from the reconciled ClusterRole after each sync cycle | +| Capability | Detail | +| --------------------- | -------------------------------------------------------------------------------------------------------------------------- | +| **Static lifecycle** | No health tracking, grace periods, or suspension — the resource is reconciled to desired state | +| **Mutation pipeline** | Typed editors (`PolicyRulesEditor`) for `.rules` and object metadata, with aggregation rule support and a raw escape hatch | +| **Cluster-scoped** | No namespace required — identity format is `rbac.authorization.k8s.io/v1/ClusterRole/` | +| **Flavors** | Preserves externally-managed fields — labels, annotations, and `.rules` entries not owned by the operator | +| **Data extraction** | Reads generated or updated values back from the reconciled ClusterRole after each sync cycle | ## Building a ClusterRole Primitive @@ -302,10 +302,9 @@ Multiple flavors can be registered and run in registration order. ## Full Example: Feature-Composed RBAC ```go -func CoreRulesMutation(version string) clusterrole.Mutation { +func CoreRulesMutation() clusterrole.Mutation { return clusterrole.Mutation{ - Name: "core-rules", - Feature: feature.NewResourceFeature(version, nil), + Name: "core-rules", Mutate: func(m *clusterrole.Mutator) error { m.AddRule(rbacv1.PolicyRule{ APIGroups: []string{""}, @@ -333,7 +332,7 @@ func CRDAccessMutation(version string, manageCRDs bool) clusterrole.Mutation { } resource, err := clusterrole.NewBuilder(base). - WithMutation(CoreRulesMutation(owner.Spec.Version)). + WithMutation(CoreRulesMutation()). WithMutation(CRDAccessMutation(owner.Spec.Version, owner.Spec.ManageCRDs)). Build() ``` diff --git a/examples/clusterrole-primitive/features/mutations.go b/examples/clusterrole-primitive/features/mutations.go index b5c77dee..02052a7a 100644 --- a/examples/clusterrole-primitive/features/mutations.go +++ b/examples/clusterrole-primitive/features/mutations.go @@ -9,11 +9,10 @@ import ( ) // CoreRulesMutation grants read access to core resources (pods, services, configmaps). -// It is always enabled. -func CoreRulesMutation(version string) clusterrole.Mutation { +// It is always enabled — Feature is nil so it applies unconditionally. +func CoreRulesMutation() clusterrole.Mutation { return clusterrole.Mutation{ - Name: "core-rules", - Feature: feature.NewResourceFeature(version, nil), + Name: "core-rules", Mutate: func(m *clusterrole.Mutator) error { m.AddRule(rbacv1.PolicyRule{ APIGroups: []string{""}, @@ -26,11 +25,10 @@ func CoreRulesMutation(version string) clusterrole.Mutation { } // VersionLabelMutation sets the app.kubernetes.io/version label on the ClusterRole. -// It is always enabled. +// It is always enabled — Feature is nil so it applies unconditionally. func VersionLabelMutation(version string) clusterrole.Mutation { return clusterrole.Mutation{ - Name: "version-label", - Feature: feature.NewResourceFeature(version, nil), + Name: "version-label", Mutate: func(m *clusterrole.Mutator) error { m.EditObjectMetadata(func(e *editors.ObjectMetaEditor) error { e.EnsureLabel("app.kubernetes.io/version", version) diff --git a/examples/clusterrole-primitive/resources/clusterrole.go b/examples/clusterrole-primitive/resources/clusterrole.go index 822c4da7..56417487 100644 --- a/examples/clusterrole-primitive/resources/clusterrole.go +++ b/examples/clusterrole-primitive/resources/clusterrole.go @@ -31,7 +31,7 @@ func NewClusterRoleResource(owner *sharedapp.ExampleApp) (component.Resource, er // 3. Register mutations in dependency order. // CoreRulesMutation and VersionLabelMutation always run first. // SecretAccess and DeploymentAccess are feature-gated. - builder.WithMutation(features.CoreRulesMutation(owner.Spec.Version)) + builder.WithMutation(features.CoreRulesMutation()) builder.WithMutation(features.VersionLabelMutation(owner.Spec.Version)) builder.WithMutation(features.SecretAccessMutation(owner.Spec.Version, owner.Spec.EnableTracing)) builder.WithMutation(features.DeploymentAccessMutation(owner.Spec.Version, owner.Spec.EnableMetrics)) From ddc6e740e2f356d005269f8dd1db57d3cb196596 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= Date: Tue, 24 Mar 2026 01:45:14 +0000 Subject: [PATCH 19/28] =?UTF-8?q?fix:=20address=20Copilot=20review=20?= =?UTF-8?q?=E2=80=94=20broaden=20PolicyRulesEditor=20docs,=20remove=20fals?= =?UTF-8?q?e=20ordering=20claims=20in=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Update docs/primitives.md to reflect that PolicyRulesEditor applies to both Role and ClusterRole, not just ClusterRole - Rename and re-scope two tests that falsely claimed to verify operation ordering when the assertions were order-independent Co-Authored-By: Claude Opus 4.6 (1M context) --- docs/primitives.md | 2 +- pkg/primitives/clusterrole/mutator_test.go | 17 ++++++++--------- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/docs/primitives.md b/docs/primitives.md index 79876db4..e052669a 100644 --- a/docs/primitives.md +++ b/docs/primitives.md @@ -120,7 +120,7 @@ Editors provide scoped, typed APIs for modifying specific parts of a resource: | `PodSpecEditor` | Volumes, tolerations, node selectors, service account, security context | | `DeploymentSpecEditor` | Replicas, update strategy, label selectors | | `ConfigMapDataEditor` | `.data` entries — set, remove, deep-merge YAML patches, raw access | -| `PolicyRulesEditor` | `.rules` entries on a ClusterRole — add, remove, clear, raw access | +| `PolicyRulesEditor` | `.rules` entries on Role and ClusterRole objects — add, remove, clear, raw access | | `ObjectMetaEditor` | Labels and annotations on any Kubernetes object | Every editor exposes a `.Raw()` method for cases where the typed API is insufficient, giving direct access to the diff --git a/pkg/primitives/clusterrole/mutator_test.go b/pkg/primitives/clusterrole/mutator_test.go index 8955ac27..6a84acd1 100644 --- a/pkg/primitives/clusterrole/mutator_test.go +++ b/pkg/primitives/clusterrole/mutator_test.go @@ -157,20 +157,20 @@ func TestMutator_SetAggregationRule_Immutable(t *testing.T) { // --- Execution order --- -func TestMutator_OperationOrder(t *testing.T) { +func TestMutator_MixedOperationTypes(t *testing.T) { cr := newTestCR(nil) m := NewMutator(cr) - // Register in reverse logical order to confirm Apply() enforces category ordering. + // Register both metadata and rules mutations to verify they are all applied. m.AddRule(rbacv1.PolicyRule{ APIGroups: []string{""}, Resources: []string{"pods"}, Verbs: []string{"get"}, }) m.EditObjectMetadata(func(e *editors.ObjectMetaEditor) error { - e.EnsureLabel("order", "tested") + e.EnsureLabel("mixed", "tested") return nil }) require.NoError(t, m.Apply()) - assert.Equal(t, "tested", cr.Labels["order"]) + assert.Equal(t, "tested", cr.Labels["mixed"]) require.Len(t, cr.Rules, 1) assert.Equal(t, "pods", cr.Rules[0].Resources[0]) } @@ -226,9 +226,8 @@ func TestMutator_MultipleFeatures(t *testing.T) { assert.Equal(t, "deployments", cr.Rules[1].Resources[0]) } -func TestMutator_FeatureOrder_MetadataBeforeRules(t *testing.T) { - // Within a single feature, metadata edits apply before rules edits, - // even when registered in the opposite order. +func TestMutator_FeatureMixedEdits(t *testing.T) { + // Within a single feature, both metadata and rules edits are applied. cr := newTestCR(nil) m := NewMutator(cr) @@ -236,12 +235,12 @@ func TestMutator_FeatureOrder_MetadataBeforeRules(t *testing.T) { APIGroups: []string{""}, Resources: []string{"pods"}, Verbs: []string{"get"}, }) m.EditObjectMetadata(func(e *editors.ObjectMetaEditor) error { - e.EnsureLabel("order", "correct") + e.EnsureLabel("feature", "complete") return nil }) require.NoError(t, m.Apply()) - assert.Equal(t, "correct", cr.Labels["order"]) + assert.Equal(t, "complete", cr.Labels["feature"]) require.Len(t, cr.Rules, 1) } From 84f5260bb6f9ced64aec6f4fd36418c6479851f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= Date: Tue, 24 Mar 2026 02:02:36 +0000 Subject: [PATCH 20/28] style: format primitives.md with prettier Co-Authored-By: Claude Opus 4.6 (1M context) --- docs/primitives.md | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/docs/primitives.md b/docs/primitives.md index e052669a..87c5f951 100644 --- a/docs/primitives.md +++ b/docs/primitives.md @@ -114,14 +114,14 @@ This design: Editors provide scoped, typed APIs for modifying specific parts of a resource: -| Editor | Scope | -| ---------------------- | ----------------------------------------------------------------------- | -| `ContainerEditor` | Environment variables, arguments, resource limits, ports | -| `PodSpecEditor` | Volumes, tolerations, node selectors, service account, security context | -| `DeploymentSpecEditor` | Replicas, update strategy, label selectors | -| `ConfigMapDataEditor` | `.data` entries — set, remove, deep-merge YAML patches, raw access | +| Editor | Scope | +| ---------------------- | --------------------------------------------------------------------------------- | +| `ContainerEditor` | Environment variables, arguments, resource limits, ports | +| `PodSpecEditor` | Volumes, tolerations, node selectors, service account, security context | +| `DeploymentSpecEditor` | Replicas, update strategy, label selectors | +| `ConfigMapDataEditor` | `.data` entries — set, remove, deep-merge YAML patches, raw access | | `PolicyRulesEditor` | `.rules` entries on Role and ClusterRole objects — add, remove, clear, raw access | -| `ObjectMetaEditor` | Labels and annotations on any Kubernetes object | +| `ObjectMetaEditor` | Labels and annotations on any Kubernetes object | Every editor exposes a `.Raw()` method for cases where the typed API is insufficient, giving direct access to the underlying Kubernetes struct while keeping the mutation scoped to that editor's target. From 3145f52464b476f41d0b8f7e3aefa86552e54b30 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= Date: Tue, 24 Mar 2026 02:09:09 +0000 Subject: [PATCH 21/28] =?UTF-8?q?fix:=20address=20Copilot=20review=20?= =?UTF-8?q?=E2=80=94=20valid=20mutation=20in=20builder=20test,=20add=20res?= =?UTF-8?q?ource-level=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - TestBuilder_WithMutation now uses a Mutation with a non-nil Mutate func so the test models valid usage instead of an impossible configuration. - Add comprehensive resource-level tests for ClusterRole: Identity(), Object() deep-copy, Mutate with default and custom field applicators, mutation execution, feature ordering, and data extraction. Co-Authored-By: Claude Opus 4.6 (1M context) --- pkg/primitives/clusterrole/builder_test.go | 12 +- pkg/primitives/clusterrole/resource_test.go | 170 ++++++++++++++++++++ 2 files changed, 181 insertions(+), 1 deletion(-) diff --git a/pkg/primitives/clusterrole/builder_test.go b/pkg/primitives/clusterrole/builder_test.go index 6e672192..559782a5 100644 --- a/pkg/primitives/clusterrole/builder_test.go +++ b/pkg/primitives/clusterrole/builder_test.go @@ -86,7 +86,17 @@ func TestBuilder_WithMutation(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Name: "test-cr"}, } res, err := NewBuilder(cr). - WithMutation(Mutation{Name: "test-mutation"}). + WithMutation(Mutation{ + Name: "test-mutation", + Mutate: func(m *Mutator) error { + m.AddRule(rbacv1.PolicyRule{ + APIGroups: []string{""}, + Resources: []string{"pods"}, + Verbs: []string{"get"}, + }) + return nil + }, + }). Build() require.NoError(t, err) assert.Len(t, res.base.Mutations, 1) diff --git a/pkg/primitives/clusterrole/resource_test.go b/pkg/primitives/clusterrole/resource_test.go index a3037f6c..0e39b5e9 100644 --- a/pkg/primitives/clusterrole/resource_test.go +++ b/pkg/primitives/clusterrole/resource_test.go @@ -1,14 +1,184 @@ package clusterrole import ( + "errors" "testing" + "github.com/sourcehawk/operator-component-framework/pkg/feature" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" rbacv1 "k8s.io/api/rbac/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) +func newValidCR() *rbacv1.ClusterRole { + return &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{Name: "test-cr"}, + Rules: []rbacv1.PolicyRule{ + {APIGroups: []string{""}, Resources: []string{"pods"}, Verbs: []string{"get", "list"}}, + }, + } +} + +func TestResource_Identity(t *testing.T) { + res, err := NewBuilder(newValidCR()).Build() + require.NoError(t, err) + assert.Equal(t, "rbac.authorization.k8s.io/v1/ClusterRole/test-cr", res.Identity()) +} + +func TestResource_Object(t *testing.T) { + cr := newValidCR() + res, err := NewBuilder(cr).Build() + require.NoError(t, err) + + obj, err := res.Object() + require.NoError(t, err) + + got, ok := obj.(*rbacv1.ClusterRole) + require.True(t, ok) + assert.Equal(t, cr.Name, got.Name) + + // Must be a deep copy. + got.Name = "changed" + assert.Equal(t, "test-cr", cr.Name) +} + +func TestResource_Mutate(t *testing.T) { + desired := newValidCR() + res, err := NewBuilder(desired).Build() + require.NoError(t, err) + + current := &rbacv1.ClusterRole{} + require.NoError(t, res.Mutate(current)) + + require.Len(t, current.Rules, 1) + assert.Equal(t, []string{"get", "list"}, current.Rules[0].Verbs) +} + +func TestResource_Mutate_WithMutation(t *testing.T) { + desired := newValidCR() + res, err := NewBuilder(desired). + WithMutation(Mutation{ + Name: "add-rule", + Feature: feature.NewResourceFeature("v1", nil).When(true), + Mutate: func(m *Mutator) error { + m.AddRule(rbacv1.PolicyRule{ + APIGroups: []string{"apps"}, + Resources: []string{"deployments"}, + Verbs: []string{"get"}, + }) + return nil + }, + }). + Build() + require.NoError(t, err) + + current := &rbacv1.ClusterRole{} + require.NoError(t, res.Mutate(current)) + + require.Len(t, current.Rules, 2) + assert.Equal(t, []string{"get", "list"}, current.Rules[0].Verbs) + assert.Equal(t, []string{"deployments"}, current.Rules[1].Resources) +} + +func TestResource_Mutate_FeatureOrdering(t *testing.T) { + desired := newValidCR() + res, err := NewBuilder(desired). + WithMutation(Mutation{ + Name: "feature-a", + Feature: feature.NewResourceFeature("v1", nil).When(true), + Mutate: func(m *Mutator) error { + m.AddRule(rbacv1.PolicyRule{ + APIGroups: []string{""}, Resources: []string{"secrets"}, Verbs: []string{"get"}, + }) + return nil + }, + }). + WithMutation(Mutation{ + Name: "feature-b", + Feature: feature.NewResourceFeature("v1", nil).When(true), + Mutate: func(m *Mutator) error { + m.AddRule(rbacv1.PolicyRule{ + APIGroups: []string{""}, Resources: []string{"configmaps"}, Verbs: []string{"get"}, + }) + return nil + }, + }). + Build() + require.NoError(t, err) + + current := &rbacv1.ClusterRole{} + require.NoError(t, res.Mutate(current)) + + // Base rule + feature-a + feature-b = 3 rules in order. + require.Len(t, current.Rules, 3) + assert.Equal(t, []string{"pods"}, current.Rules[0].Resources) + assert.Equal(t, []string{"secrets"}, current.Rules[1].Resources) + assert.Equal(t, []string{"configmaps"}, current.Rules[2].Resources) +} + +func TestResource_Mutate_CustomFieldApplicator(t *testing.T) { + desired := newValidCR() + + applicatorCalled := false + res, err := NewBuilder(desired). + WithCustomFieldApplicator(func(current, d *rbacv1.ClusterRole) error { + applicatorCalled = true + current.Rules = d.Rules + return nil + }). + Build() + require.NoError(t, err) + + current := &rbacv1.ClusterRole{} + require.NoError(t, res.Mutate(current)) + + assert.True(t, applicatorCalled) + require.Len(t, current.Rules, 1) +} + +func TestResource_Mutate_CustomFieldApplicator_Error(t *testing.T) { + res, err := NewBuilder(newValidCR()). + WithCustomFieldApplicator(func(_, _ *rbacv1.ClusterRole) error { + return errors.New("applicator error") + }). + Build() + require.NoError(t, err) + + err = res.Mutate(&rbacv1.ClusterRole{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "applicator error") +} + +func TestResource_ExtractData(t *testing.T) { + cr := newValidCR() + + var extracted string + res, err := NewBuilder(cr). + WithDataExtractor(func(c rbacv1.ClusterRole) error { + extracted = c.Name + return nil + }). + Build() + require.NoError(t, err) + + require.NoError(t, res.ExtractData()) + assert.Equal(t, "test-cr", extracted) +} + +func TestResource_ExtractData_Error(t *testing.T) { + res, err := NewBuilder(newValidCR()). + WithDataExtractor(func(_ rbacv1.ClusterRole) error { + return errors.New("extract error") + }). + Build() + require.NoError(t, err) + + err = res.ExtractData() + require.Error(t, err) + assert.Contains(t, err.Error(), "extract error") +} + func TestDefaultFieldApplicator_PreservesServerManagedFields(t *testing.T) { current := &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ From 9962187fc30d42f191c6a34144a45cd138cd8416 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= Date: Tue, 24 Mar 2026 02:11:53 +0000 Subject: [PATCH 22/28] fix: clarify PolicyRule comparison uses order-insensitive multiset, not set semantics The docs claimed slice fields were compared as "unordered sets" but the implementation treats duplicates as significant (multiset equality). Updated comments to accurately describe the behavior rather than changing the comparison logic. Co-Authored-By: Claude Opus 4.6 (1M context) --- pkg/primitives/clusterrole/flavors.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/pkg/primitives/clusterrole/flavors.go b/pkg/primitives/clusterrole/flavors.go index a371ecbb..754304e9 100644 --- a/pkg/primitives/clusterrole/flavors.go +++ b/pkg/primitives/clusterrole/flavors.go @@ -32,8 +32,9 @@ func PreserveCurrentAnnotations(applied, current, desired *rbacv1.ClusterRole) e // // This is useful when other controllers or admission webhooks inject rules into // the ClusterRole that your operator does not own. Rules present in both are -// determined by comparing all PolicyRule fields with slice fields treated as -// unordered sets, so different orderings of the same elements are considered equal. +// determined by comparing all PolicyRule fields with slice fields compared +// order-insensitively; duplicate elements are significant (i.e. ["get","get"] +// is not equal to ["get"]). func PreserveExternalRules(applied, current, _ *rbacv1.ClusterRole) error { for _, currentRule := range current.Rules { if !containsRule(applied.Rules, currentRule) { @@ -54,8 +55,8 @@ func containsRule(rules []rbacv1.PolicyRule, target rbacv1.PolicyRule) bool { } // policyRuleEqual reports whether two PolicyRules are equal by comparing all -// fields as sets, so that different orderings of the same elements are treated -// as equivalent. +// fields order-insensitively. Duplicate elements are significant: slices must +// contain the same elements with the same multiplicities to be considered equal. func policyRuleEqual(a, b rbacv1.PolicyRule) bool { return stringSliceEqualUnordered(a.Verbs, b.Verbs) && stringSliceEqualUnordered(a.APIGroups, b.APIGroups) && From 1b5aa4de20538c7290953876887256fe898d15bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= Date: Tue, 24 Mar 2026 17:55:32 +0000 Subject: [PATCH 23/28] fix: do not initialize an empty plan on clusterrole mutator construction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Align with configmap and deployment mutators — NewMutator no longer creates an initial feature plan. BeginFeature must be called before registering any mutations. Updates all tests accordingly and adds constructor/plan invariant tests. Co-Authored-By: Claude Opus 4.6 (1M context) --- pkg/primitives/clusterrole/mutator.go | 8 +-- pkg/primitives/clusterrole/mutator_test.go | 69 ++++++++++++++++++++++ 2 files changed, 72 insertions(+), 5 deletions(-) diff --git a/pkg/primitives/clusterrole/mutator.go b/pkg/primitives/clusterrole/mutator.go index bcf12e16..d5a31ecc 100644 --- a/pkg/primitives/clusterrole/mutator.go +++ b/pkg/primitives/clusterrole/mutator.go @@ -36,13 +36,11 @@ type Mutator struct { } // NewMutator creates a new Mutator for the given ClusterRole. +// BeginFeature must be called before registering any mutations. func NewMutator(cr *rbacv1.ClusterRole) *Mutator { - m := &Mutator{ - cr: cr, - plans: []featurePlan{{}}, + return &Mutator{ + cr: cr, } - m.active = &m.plans[0] - return m } // BeginFeature starts a new feature planning scope. All subsequent mutation diff --git a/pkg/primitives/clusterrole/mutator_test.go b/pkg/primitives/clusterrole/mutator_test.go index 6a84acd1..171fc85d 100644 --- a/pkg/primitives/clusterrole/mutator_test.go +++ b/pkg/primitives/clusterrole/mutator_test.go @@ -24,6 +24,7 @@ func newTestCR(rules []rbacv1.PolicyRule) *rbacv1.ClusterRole { func TestMutator_EditObjectMetadata(t *testing.T) { cr := newTestCR(nil) m := NewMutator(cr) + m.BeginFeature() m.EditObjectMetadata(func(e *editors.ObjectMetaEditor) error { e.EnsureLabel("app", "myapp") return nil @@ -35,6 +36,7 @@ func TestMutator_EditObjectMetadata(t *testing.T) { func TestMutator_EditObjectMetadata_Nil(t *testing.T) { cr := newTestCR(nil) m := NewMutator(cr) + m.BeginFeature() m.EditObjectMetadata(nil) assert.NoError(t, m.Apply()) } @@ -44,6 +46,7 @@ func TestMutator_EditObjectMetadata_Nil(t *testing.T) { func TestMutator_EditRules(t *testing.T) { cr := newTestCR(nil) m := NewMutator(cr) + m.BeginFeature() m.EditRules(func(e *editors.PolicyRulesEditor) error { e.AddRule(rbacv1.PolicyRule{ APIGroups: []string{""}, Resources: []string{"pods"}, Verbs: []string{"get"}, @@ -58,6 +61,7 @@ func TestMutator_EditRules(t *testing.T) { func TestMutator_EditRules_Nil(t *testing.T) { cr := newTestCR(nil) m := NewMutator(cr) + m.BeginFeature() m.EditRules(nil) assert.NoError(t, m.Apply()) } @@ -67,6 +71,7 @@ func TestMutator_EditRules_Nil(t *testing.T) { func TestMutator_AddRule(t *testing.T) { cr := newTestCR(nil) m := NewMutator(cr) + m.BeginFeature() m.AddRule(rbacv1.PolicyRule{ APIGroups: []string{"apps"}, Resources: []string{"deployments"}, Verbs: []string{"get", "list"}, }) @@ -81,6 +86,7 @@ func TestMutator_AddRule_Appends(t *testing.T) { } cr := newTestCR(existing) m := NewMutator(cr) + m.BeginFeature() m.AddRule(rbacv1.PolicyRule{ APIGroups: []string{""}, Resources: []string{"services"}, Verbs: []string{"list"}, }) @@ -93,6 +99,7 @@ func TestMutator_AddRule_Appends(t *testing.T) { func TestMutator_SetAggregationRule(t *testing.T) { cr := newTestCR(nil) m := NewMutator(cr) + m.BeginFeature() aggRule := &rbacv1.AggregationRule{ ClusterRoleSelectors: []metav1.LabelSelector{ {MatchLabels: map[string]string{"aggregate": "true"}}, @@ -113,6 +120,7 @@ func TestMutator_SetAggregationRule_Nil(t *testing.T) { }, } m := NewMutator(cr) + m.BeginFeature() m.SetAggregationRule(nil) require.NoError(t, m.Apply()) assert.Nil(t, cr.AggregationRule) @@ -121,6 +129,7 @@ func TestMutator_SetAggregationRule_Nil(t *testing.T) { func TestMutator_SetAggregationRule_LastWins(t *testing.T) { cr := newTestCR(nil) m := NewMutator(cr) + m.BeginFeature() m.SetAggregationRule(&rbacv1.AggregationRule{ ClusterRoleSelectors: []metav1.LabelSelector{ {MatchLabels: map[string]string{"first": "true"}}, @@ -139,6 +148,7 @@ func TestMutator_SetAggregationRule_LastWins(t *testing.T) { func TestMutator_SetAggregationRule_Immutable(t *testing.T) { cr := newTestCR(nil) m := NewMutator(cr) + m.BeginFeature() aggRule := &rbacv1.AggregationRule{ ClusterRoleSelectors: []metav1.LabelSelector{ {MatchLabels: map[string]string{"aggregate": "true"}}, @@ -160,6 +170,7 @@ func TestMutator_SetAggregationRule_Immutable(t *testing.T) { func TestMutator_MixedOperationTypes(t *testing.T) { cr := newTestCR(nil) m := NewMutator(cr) + m.BeginFeature() // Register both metadata and rules mutations to verify they are all applied. m.AddRule(rbacv1.PolicyRule{ APIGroups: []string{""}, Resources: []string{"pods"}, Verbs: []string{"get"}, @@ -178,6 +189,7 @@ func TestMutator_MixedOperationTypes(t *testing.T) { func TestMutator_MultipleMutations(t *testing.T) { cr := newTestCR(nil) m := NewMutator(cr) + m.BeginFeature() m.AddRule(rbacv1.PolicyRule{ APIGroups: []string{""}, Resources: []string{"pods"}, Verbs: []string{"get"}, }) @@ -198,6 +210,7 @@ func TestMutator_MultipleFeatures(t *testing.T) { m := NewMutator(cr) // Feature A: add a rule and a label + m.BeginFeature() m.AddRule(rbacv1.PolicyRule{ APIGroups: []string{""}, Resources: []string{"pods"}, Verbs: []string{"get"}, }) @@ -230,6 +243,7 @@ func TestMutator_FeatureMixedEdits(t *testing.T) { // Within a single feature, both metadata and rules edits are applied. cr := newTestCR(nil) m := NewMutator(cr) + m.BeginFeature() m.AddRule(rbacv1.PolicyRule{ APIGroups: []string{""}, Resources: []string{"pods"}, Verbs: []string{"get"}, @@ -249,3 +263,58 @@ func TestMutator_FeatureMixedEdits(t *testing.T) { func TestMutator_ImplementsObjectMutator(_ *testing.T) { var _ editors.ObjectMutator = (*Mutator)(nil) } + +// --- Constructor and feature plan invariants --- + +func TestNewMutator_InitializesNoPlan(t *testing.T) { + cr := newTestCR(nil) + m := NewMutator(cr) + + assert.Empty(t, m.plans, "NewMutator must not create any plans") + assert.Nil(t, m.active, "active plan must not be set") +} + +func TestBeginFeature_AddsExactlyOnePlan(t *testing.T) { + cr := newTestCR(nil) + m := NewMutator(cr) + + m.BeginFeature() + require.Len(t, m.plans, 1, "BeginFeature must add exactly one plan") + assert.Equal(t, &m.plans[0], m.active, "active must point to the new plan") + + m.BeginFeature() + require.Len(t, m.plans, 2) + assert.Equal(t, &m.plans[1], m.active) +} + +func TestBeginFeature_IsolatesFeaturePlans(t *testing.T) { + cr := newTestCR(nil) + m := NewMutator(cr) + + // Record a mutation in the first feature plan + m.BeginFeature() + m.AddRule(rbacv1.PolicyRule{ + APIGroups: []string{""}, Resources: []string{"pods"}, Verbs: []string{"get"}, + }) + + // Start a new feature and record a different mutation + m.BeginFeature() + m.AddRule(rbacv1.PolicyRule{ + APIGroups: []string{"apps"}, Resources: []string{"deployments"}, Verbs: []string{"list"}, + }) + + assert.Len(t, m.plans[0].rulesEdits, 1, "first plan should have one rules edit") + assert.Len(t, m.plans[1].rulesEdits, 1, "second plan should have one rules edit") +} + +func TestMutator_SingleFeature_PlanCount(t *testing.T) { + cr := newTestCR(nil) + m := NewMutator(cr) + m.BeginFeature() + m.AddRule(rbacv1.PolicyRule{ + APIGroups: []string{""}, Resources: []string{"pods"}, Verbs: []string{"get"}, + }) + + require.NoError(t, m.Apply()) + assert.Len(t, m.plans, 1) +} From a82b7721ac5396396a8be8ea05cb5bdd3915894f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Wed, 25 Mar 2026 14:07:40 +0000 Subject: [PATCH 24/28] refactor: remove field applicators and flavors from clusterrole primitive Align with the framework's move to Server-Side Apply. Remove DefaultFieldApplicator, WithCustomFieldApplicator, WithFieldApplicationFlavor, the flavors.go file, and all associated tests. Update builder to use the new generic constructor signature (without defaultApplicator). Update Mutate tests to use Object() output instead of empty structs. Strip field applicator and flavor sections from primitive docs. Co-Authored-By: Claude Opus 4.6 (1M context) --- docs/primitives/clusterrole.md | 72 -------- pkg/primitives/clusterrole/builder.go | 43 +---- pkg/primitives/clusterrole/builder_test.go | 32 ---- pkg/primitives/clusterrole/flavors.go | 88 --------- pkg/primitives/clusterrole/flavors_test.go | 190 -------------------- pkg/primitives/clusterrole/resource.go | 20 +-- pkg/primitives/clusterrole/resource_test.go | 115 +++--------- 7 files changed, 28 insertions(+), 532 deletions(-) delete mode 100644 pkg/primitives/clusterrole/flavors.go delete mode 100644 pkg/primitives/clusterrole/flavors_test.go diff --git a/docs/primitives/clusterrole.md b/docs/primitives/clusterrole.md index ff48f59b..4630c53c 100644 --- a/docs/primitives/clusterrole.md +++ b/docs/primitives/clusterrole.md @@ -21,7 +21,6 @@ empty — setting a namespace on a cluster-scoped resource is rejected. | **Static lifecycle** | No health tracking, grace periods, or suspension — the resource is reconciled to desired state | | **Mutation pipeline** | Typed editors (`PolicyRulesEditor`) for `.rules` and object metadata, with aggregation rule support and a raw escape hatch | | **Cluster-scoped** | No namespace required — identity format is `rbac.authorization.k8s.io/v1/ClusterRole/` | -| **Flavors** | Preserves externally-managed fields — labels, annotations, and `.rules` entries not owned by the operator | | **Data extraction** | Reads generated or updated values back from the reconciled ClusterRole after each sync cycle | ## Building a ClusterRole Primitive @@ -43,29 +42,10 @@ base := &rbacv1.ClusterRole{ } resource, err := clusterrole.NewBuilder(base). - WithFieldApplicationFlavor(clusterrole.PreserveExternalRules). WithMutation(MyFeatureMutation(owner.Spec.Version)). Build() ``` -## Default Field Application - -`DefaultFieldApplicator` replaces the current ClusterRole with a deep copy of the desired object, then restores -server-managed metadata (ResourceVersion, UID, etc.) and shared-controller fields (OwnerReferences, Finalizers) from the -original live object. ClusterRole has no Status subresource, so no status preservation is needed. - -Use `WithCustomFieldApplicator` when other controllers manage fields that should not be overwritten: - -```go -resource, err := clusterrole.NewBuilder(base). - WithCustomFieldApplicator(func(current, desired *rbacv1.ClusterRole) error { - // Only synchronise owned rules; leave other fields untouched. - current.Rules = desired.Rules - return nil - }). - Build() -``` - ## Mutations Mutations are the primary mechanism for modifying a `ClusterRole` beyond its baseline. Each mutation is a named function @@ -257,48 +237,6 @@ m.SetAggregationRule(&rbacv1.AggregationRule{ Setting the aggregation rule to nil clears it. Within a single feature, the last `SetAggregationRule` call wins. -## Flavors - -Flavors run after the baseline applicator and before mutations. They are used to preserve fields managed by external -controllers or other tools. - -### PreserveCurrentLabels - -Preserves labels present on the live object but absent from the applied desired state. Applied labels win on overlap. - -```go -resource, err := clusterrole.NewBuilder(base). - WithFieldApplicationFlavor(clusterrole.PreserveCurrentLabels). - Build() -``` - -### PreserveCurrentAnnotations - -Preserves annotations present on the live object but absent from the applied desired state. Applied annotations win on -overlap. - -```go -resource, err := clusterrole.NewBuilder(base). - WithFieldApplicationFlavor(clusterrole.PreserveCurrentAnnotations). - Build() -``` - -### PreserveExternalRules - -Preserves `.rules` entries present on the live object but absent from the applied desired state. Rules are compared -using all `PolicyRule` fields (APIGroups, Resources, Verbs, ResourceNames, NonResourceURLs), treating these slice fields -as sets so that order differences are ignored. - -Use this when other controllers or admission webhooks inject rules into the ClusterRole that your operator does not own: - -```go -resource, err := clusterrole.NewBuilder(base). - WithFieldApplicationFlavor(clusterrole.PreserveExternalRules). - Build() -``` - -Multiple flavors can be registered and run in registration order. - ## Full Example: Feature-Composed RBAC ```go @@ -340,22 +278,12 @@ resource, err := clusterrole.NewBuilder(base). When `ManageCRDs` is true, the final rules include both core and CRD access rules. When false, only the core rules are written. Neither mutation needs to know about the other. -> **Note:** Do not combine `PreserveExternalRules` with feature-gated mutations that add and remove rules. Because -> flavors run before mutations and preserve rules from the live object, previously-added rules will be retained even -> after a feature gate is disabled, and rules can be duplicated if a mutation re-adds a rule already present on the live -> object. Use `PreserveExternalRules` only when external controllers or admission webhooks manage rules that your -> operator does not own. - ## Guidance **`Feature: nil` applies unconditionally.** Omit `Feature` (leave it nil) for mutations that should always run. Use `feature.NewResourceFeature(version, constraints)` when version-based gating is needed, and chain `.When(bool)` for boolean conditions. -**Use `PreserveExternalRules` when sharing a ClusterRole.** If admission webhooks, external controllers, or manual -operations add rules to a ClusterRole your operator manages, this flavor prevents your operator from silently deleting -those rules each reconcile cycle. - **Use `SetAggregationRule` for composite roles.** When you want the API server to aggregate rules from multiple ClusterRoles based on label selectors, use `SetAggregationRule` instead of managing `.rules` directly. The two approaches are mutually exclusive in the Kubernetes API — the API server ignores `.rules` when `.aggregationRule` is diff --git a/pkg/primitives/clusterrole/builder.go b/pkg/primitives/clusterrole/builder.go index 5954fb92..5f08293b 100644 --- a/pkg/primitives/clusterrole/builder.go +++ b/pkg/primitives/clusterrole/builder.go @@ -10,9 +10,9 @@ import ( // Builder is a configuration helper for creating and customizing a ClusterRole Resource. // -// It provides a fluent API for registering mutations, field application flavors, -// and data extractors. Build() validates the configuration and returns an -// initialized Resource ready for use in a reconciliation loop. +// It provides a fluent API for registering mutations and data extractors. +// Build() validates the configuration and returns an initialized Resource +// ready for use in a reconciliation loop. type Builder struct { base *generic.StaticBuilder[*rbacv1.ClusterRole, *Mutator] } @@ -21,7 +21,7 @@ type Builder struct { // // The ClusterRole object serves as the desired base state. During reconciliation // the Resource will make the cluster's state match this base, modified by any -// registered mutations and flavors. +// registered mutations. // // The provided ClusterRole must have Name set (ClusterRole is cluster-scoped and // does not use a namespace), which is validated during the Build() call. @@ -33,7 +33,6 @@ func NewBuilder(cr *rbacv1.ClusterRole) *Builder { sb := generic.NewStaticBuilder[*rbacv1.ClusterRole, *Mutator]( cr, identityFunc, - DefaultFieldApplicator, NewMutator, ) sb.MarkClusterScoped() @@ -43,8 +42,7 @@ func NewBuilder(cr *rbacv1.ClusterRole) *Builder { // WithMutation registers a mutation for the ClusterRole. // -// Mutations are applied sequentially during the Mutate() phase of reconciliation, -// after the baseline field applicator and any registered flavors have run. +// Mutations are applied sequentially during the Mutate() phase of reconciliation. // A mutation with a nil Feature is applied unconditionally; one with a non-nil // Feature is applied only when that feature is enabled. func (b *Builder) WithMutation(m Mutation) *Builder { @@ -52,37 +50,6 @@ func (b *Builder) WithMutation(m Mutation) *Builder { return b } -// WithCustomFieldApplicator sets a custom strategy for applying the desired -// state to the existing ClusterRole in the cluster. -// -// The default applicator (DefaultFieldApplicator) replaces the current object -// with a deep copy of the desired object. Use a custom applicator when other -// controllers manage fields you need to preserve. -// -// The applicator receives the current object from the API server and the desired -// object from the Resource, and is responsible for merging the desired changes -// into the current object. -func (b *Builder) WithCustomFieldApplicator( - applicator func(current, desired *rbacv1.ClusterRole) error, -) *Builder { - b.base.WithCustomFieldApplicator(applicator) - return b -} - -// WithFieldApplicationFlavor registers a post-baseline field application flavor. -// -// Flavors run after the baseline applicator (default or custom) in registration -// order. They are typically used to preserve fields from the live cluster object -// that should not be overwritten by the desired state. -// -// A nil flavor is ignored. -func (b *Builder) WithFieldApplicationFlavor(flavor FieldApplicationFlavor) *Builder { - if flavor != nil { - b.base.WithFieldApplicationFlavor(generic.FieldApplicationFlavor[*rbacv1.ClusterRole](flavor)) - } - return b -} - // WithDataExtractor registers a function to read values from the ClusterRole after // it has been successfully reconciled. // diff --git a/pkg/primitives/clusterrole/builder_test.go b/pkg/primitives/clusterrole/builder_test.go index 559782a5..c5b1aa77 100644 --- a/pkg/primitives/clusterrole/builder_test.go +++ b/pkg/primitives/clusterrole/builder_test.go @@ -103,38 +103,6 @@ func TestBuilder_WithMutation(t *testing.T) { assert.Equal(t, "test-mutation", res.base.Mutations[0].Name) } -func TestBuilder_WithCustomFieldApplicator(t *testing.T) { - t.Parallel() - cr := &rbacv1.ClusterRole{ - ObjectMeta: metav1.ObjectMeta{Name: "test-cr"}, - } - called := false - applicator := func(_, _ *rbacv1.ClusterRole) error { - called = true - return nil - } - res, err := NewBuilder(cr). - WithCustomFieldApplicator(applicator). - Build() - require.NoError(t, err) - require.NotNil(t, res.base.CustomFieldApplicator) - _ = res.base.CustomFieldApplicator(nil, nil) - assert.True(t, called) -} - -func TestBuilder_WithFieldApplicationFlavor(t *testing.T) { - t.Parallel() - cr := &rbacv1.ClusterRole{ - ObjectMeta: metav1.ObjectMeta{Name: "test-cr"}, - } - res, err := NewBuilder(cr). - WithFieldApplicationFlavor(PreserveExternalRules). - WithFieldApplicationFlavor(nil). // nil must be ignored - Build() - require.NoError(t, err) - assert.Len(t, res.base.FieldFlavors, 1) -} - func TestBuilder_WithDataExtractor(t *testing.T) { t.Parallel() cr := &rbacv1.ClusterRole{ diff --git a/pkg/primitives/clusterrole/flavors.go b/pkg/primitives/clusterrole/flavors.go deleted file mode 100644 index 754304e9..00000000 --- a/pkg/primitives/clusterrole/flavors.go +++ /dev/null @@ -1,88 +0,0 @@ -package clusterrole - -import ( - "sort" - - "github.com/sourcehawk/operator-component-framework/pkg/flavors" - rbacv1 "k8s.io/api/rbac/v1" -) - -// FieldApplicationFlavor defines a function signature for applying flavors to a -// ClusterRole resource. A flavor is called after the baseline field applicator has -// run and can be used to preserve or merge fields from the live cluster object. -type FieldApplicationFlavor flavors.FieldApplicationFlavor[*rbacv1.ClusterRole] - -// PreserveCurrentLabels ensures that any labels present on the current live -// ClusterRole but missing from the applied (desired) object are preserved. -// If a label exists in both, the applied value wins. -func PreserveCurrentLabels(applied, current, desired *rbacv1.ClusterRole) error { - return flavors.PreserveCurrentLabels[*rbacv1.ClusterRole]()(applied, current, desired) -} - -// PreserveCurrentAnnotations ensures that any annotations present on the current -// live ClusterRole but missing from the applied (desired) object are preserved. -// If an annotation exists in both, the applied value wins. -func PreserveCurrentAnnotations(applied, current, desired *rbacv1.ClusterRole) error { - return flavors.PreserveCurrentAnnotations[*rbacv1.ClusterRole]()(applied, current, desired) -} - -// PreserveExternalRules ensures that any .rules entries present on the current -// live ClusterRole but absent from the applied (desired) object are preserved -// by appending them to the applied object's rules. -// -// This is useful when other controllers or admission webhooks inject rules into -// the ClusterRole that your operator does not own. Rules present in both are -// determined by comparing all PolicyRule fields with slice fields compared -// order-insensitively; duplicate elements are significant (i.e. ["get","get"] -// is not equal to ["get"]). -func PreserveExternalRules(applied, current, _ *rbacv1.ClusterRole) error { - for _, currentRule := range current.Rules { - if !containsRule(applied.Rules, currentRule) { - applied.Rules = append(applied.Rules, currentRule) - } - } - return nil -} - -// containsRule reports whether rules contains a PolicyRule equal to target. -func containsRule(rules []rbacv1.PolicyRule, target rbacv1.PolicyRule) bool { - for _, r := range rules { - if policyRuleEqual(r, target) { - return true - } - } - return false -} - -// policyRuleEqual reports whether two PolicyRules are equal by comparing all -// fields order-insensitively. Duplicate elements are significant: slices must -// contain the same elements with the same multiplicities to be considered equal. -func policyRuleEqual(a, b rbacv1.PolicyRule) bool { - return stringSliceEqualUnordered(a.Verbs, b.Verbs) && - stringSliceEqualUnordered(a.APIGroups, b.APIGroups) && - stringSliceEqualUnordered(a.Resources, b.Resources) && - stringSliceEqualUnordered(a.ResourceNames, b.ResourceNames) && - stringSliceEqualUnordered(a.NonResourceURLs, b.NonResourceURLs) -} - -// stringSliceEqualUnordered reports whether two string slices contain exactly -// the same elements, regardless of order. -func stringSliceEqualUnordered(a, b []string) bool { - if len(a) != len(b) { - return false - } - aSorted := make([]string, len(a)) - copy(aSorted, a) - sort.Strings(aSorted) - - bSorted := make([]string, len(b)) - copy(bSorted, b) - sort.Strings(bSorted) - - for i := range aSorted { - if aSorted[i] != bSorted[i] { - return false - } - } - return true -} diff --git a/pkg/primitives/clusterrole/flavors_test.go b/pkg/primitives/clusterrole/flavors_test.go deleted file mode 100644 index 8cec0270..00000000 --- a/pkg/primitives/clusterrole/flavors_test.go +++ /dev/null @@ -1,190 +0,0 @@ -package clusterrole - -import ( - "errors" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - rbacv1 "k8s.io/api/rbac/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" -) - -func TestPreserveCurrentLabels(t *testing.T) { - t.Run("adds missing labels from current", func(t *testing.T) { - applied := &rbacv1.ClusterRole{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"keep": "applied"}}} - current := &rbacv1.ClusterRole{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"extra": "current"}}} - - require.NoError(t, PreserveCurrentLabels(applied, current, nil)) - assert.Equal(t, "applied", applied.Labels["keep"]) - assert.Equal(t, "current", applied.Labels["extra"]) - }) - - t.Run("applied value wins on overlap", func(t *testing.T) { - applied := &rbacv1.ClusterRole{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"key": "applied"}}} - current := &rbacv1.ClusterRole{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"key": "current"}}} - - require.NoError(t, PreserveCurrentLabels(applied, current, nil)) - assert.Equal(t, "applied", applied.Labels["key"]) - }) - - t.Run("handles nil applied labels", func(t *testing.T) { - applied := &rbacv1.ClusterRole{} - current := &rbacv1.ClusterRole{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"extra": "current"}}} - - require.NoError(t, PreserveCurrentLabels(applied, current, nil)) - assert.Equal(t, "current", applied.Labels["extra"]) - }) -} - -func TestPreserveCurrentAnnotations(t *testing.T) { - t.Run("adds missing annotations from current", func(t *testing.T) { - applied := &rbacv1.ClusterRole{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{"keep": "applied"}}} - current := &rbacv1.ClusterRole{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{"extra": "current"}}} - - require.NoError(t, PreserveCurrentAnnotations(applied, current, nil)) - assert.Equal(t, "applied", applied.Annotations["keep"]) - assert.Equal(t, "current", applied.Annotations["extra"]) - }) - - t.Run("applied value wins on overlap", func(t *testing.T) { - applied := &rbacv1.ClusterRole{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{"key": "applied"}}} - current := &rbacv1.ClusterRole{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{"key": "current"}}} - - require.NoError(t, PreserveCurrentAnnotations(applied, current, nil)) - assert.Equal(t, "applied", applied.Annotations["key"]) - }) -} - -func TestPreserveExternalRules(t *testing.T) { - t.Run("adds missing rules from current", func(t *testing.T) { - applied := &rbacv1.ClusterRole{ - Rules: []rbacv1.PolicyRule{ - {APIGroups: []string{""}, Resources: []string{"pods"}, Verbs: []string{"get"}}, - }, - } - current := &rbacv1.ClusterRole{ - Rules: []rbacv1.PolicyRule{ - {APIGroups: []string{""}, Resources: []string{"secrets"}, Verbs: []string{"list"}}, - }, - } - - require.NoError(t, PreserveExternalRules(applied, current, nil)) - assert.Len(t, applied.Rules, 2) - assert.Equal(t, "pods", applied.Rules[0].Resources[0]) - assert.Equal(t, "secrets", applied.Rules[1].Resources[0]) - }) - - t.Run("does not duplicate existing rules", func(t *testing.T) { - rule := rbacv1.PolicyRule{APIGroups: []string{""}, Resources: []string{"pods"}, Verbs: []string{"get"}} - applied := &rbacv1.ClusterRole{Rules: []rbacv1.PolicyRule{rule}} - current := &rbacv1.ClusterRole{Rules: []rbacv1.PolicyRule{rule}} - - require.NoError(t, PreserveExternalRules(applied, current, nil)) - assert.Len(t, applied.Rules, 1) - }) - - t.Run("handles nil applied rules", func(t *testing.T) { - applied := &rbacv1.ClusterRole{} - current := &rbacv1.ClusterRole{ - Rules: []rbacv1.PolicyRule{ - {APIGroups: []string{""}, Resources: []string{"configmaps"}, Verbs: []string{"get"}}, - }, - } - - require.NoError(t, PreserveExternalRules(applied, current, nil)) - assert.Len(t, applied.Rules, 1) - }) - - t.Run("treats reordered-but-equivalent rules as equal", func(t *testing.T) { - applied := &rbacv1.ClusterRole{ - Rules: []rbacv1.PolicyRule{ - {APIGroups: []string{""}, Resources: []string{"pods", "services"}, Verbs: []string{"get", "list"}}, - }, - } - current := &rbacv1.ClusterRole{ - Rules: []rbacv1.PolicyRule{ - {APIGroups: []string{""}, Resources: []string{"services", "pods"}, Verbs: []string{"list", "get"}}, - }, - } - - require.NoError(t, PreserveExternalRules(applied, current, nil)) - assert.Len(t, applied.Rules, 1, "reordered rule should not be duplicated") - }) - - t.Run("no-op when current has no rules", func(t *testing.T) { - applied := &rbacv1.ClusterRole{ - Rules: []rbacv1.PolicyRule{ - {APIGroups: []string{""}, Resources: []string{"pods"}, Verbs: []string{"get"}}, - }, - } - current := &rbacv1.ClusterRole{} - - require.NoError(t, PreserveExternalRules(applied, current, nil)) - assert.Len(t, applied.Rules, 1) - }) -} - -func TestFlavors_Integration(t *testing.T) { - desired := &rbacv1.ClusterRole{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-cr", - Labels: map[string]string{"app": "desired"}, - }, - Rules: []rbacv1.PolicyRule{ - {APIGroups: []string{""}, Resources: []string{"pods"}, Verbs: []string{"get"}}, - }, - } - - t.Run("PreserveExternalRules via Mutate", func(t *testing.T) { - res, err := NewBuilder(desired). - WithFieldApplicationFlavor(PreserveExternalRules). - Build() - require.NoError(t, err) - - current := &rbacv1.ClusterRole{ - Rules: []rbacv1.PolicyRule{ - {APIGroups: []string{""}, Resources: []string{"secrets"}, Verbs: []string{"list"}}, - {APIGroups: []string{""}, Resources: []string{"pods"}, Verbs: []string{"get"}}, - }, - } - require.NoError(t, res.Mutate(current)) - - assert.Len(t, current.Rules, 2) - }) - - t.Run("flavors run in registration order", func(t *testing.T) { - var order []string - flavor1 := func(_, _, _ *rbacv1.ClusterRole) error { - order = append(order, "flavor1") - return nil - } - flavor2 := func(_, _, _ *rbacv1.ClusterRole) error { - order = append(order, "flavor2") - return nil - } - - res, err := NewBuilder(desired). - WithFieldApplicationFlavor(flavor1). - WithFieldApplicationFlavor(flavor2). - Build() - require.NoError(t, err) - - require.NoError(t, res.Mutate(&rbacv1.ClusterRole{})) - assert.Equal(t, []string{"flavor1", "flavor2"}, order) - }) - - t.Run("flavor error is returned", func(t *testing.T) { - flavorErr := errors.New("flavor boom") - res, err := NewBuilder(desired). - WithFieldApplicationFlavor(func(_, _, _ *rbacv1.ClusterRole) error { - return flavorErr - }). - Build() - require.NoError(t, err) - - err = res.Mutate(&rbacv1.ClusterRole{}) - require.Error(t, err) - assert.True(t, errors.Is(err, flavorErr)) - }) -} diff --git a/pkg/primitives/clusterrole/resource.go b/pkg/primitives/clusterrole/resource.go index b801e85f..566f5d17 100644 --- a/pkg/primitives/clusterrole/resource.go +++ b/pkg/primitives/clusterrole/resource.go @@ -6,20 +6,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" ) -// DefaultFieldApplicator replaces current with a deep copy of desired while -// preserving server-managed metadata (ResourceVersion, UID, Generation, etc.) -// and shared-controller fields (OwnerReferences, Finalizers) from the original -// current object. -// -// Use a custom field applicator via Builder.WithCustomFieldApplicator if you need -// to preserve fields that other controllers manage. -func DefaultFieldApplicator(current, desired *rbacv1.ClusterRole) error { - original := current.DeepCopy() - *current = *desired.DeepCopy() - generic.PreserveServerManagedFields(current, original) - return nil -} - // Resource is a high-level abstraction for managing a Kubernetes ClusterRole within // a controller's reconciliation loop. // @@ -52,10 +38,8 @@ func (r *Resource) Object() (client.Object, error) { // Mutate transforms the current state of a Kubernetes ClusterRole into the desired state. // // The mutation process follows this order: -// 1. Field application: the current object is updated to reflect the desired base state, -// using either DefaultFieldApplicator or a custom applicator if one is configured. -// 2. Field application flavors: any registered flavors are applied in registration order. -// 3. Feature mutations: all registered feature-gated mutations are applied in order. +// 1. The desired base state is applied to the current object. +// 2. Feature mutations: all registered feature-gated mutations are applied in order. // // This method is invoked by the framework during the Update phase of reconciliation. func (r *Resource) Mutate(current client.Object) error { diff --git a/pkg/primitives/clusterrole/resource_test.go b/pkg/primitives/clusterrole/resource_test.go index 0e39b5e9..a64f1e82 100644 --- a/pkg/primitives/clusterrole/resource_test.go +++ b/pkg/primitives/clusterrole/resource_test.go @@ -48,11 +48,13 @@ func TestResource_Mutate(t *testing.T) { res, err := NewBuilder(desired).Build() require.NoError(t, err) - current := &rbacv1.ClusterRole{} - require.NoError(t, res.Mutate(current)) + obj, err := res.Object() + require.NoError(t, err) + require.NoError(t, res.Mutate(obj)) - require.Len(t, current.Rules, 1) - assert.Equal(t, []string{"get", "list"}, current.Rules[0].Verbs) + got := obj.(*rbacv1.ClusterRole) + require.Len(t, got.Rules, 1) + assert.Equal(t, []string{"get", "list"}, got.Rules[0].Verbs) } func TestResource_Mutate_WithMutation(t *testing.T) { @@ -73,12 +75,14 @@ func TestResource_Mutate_WithMutation(t *testing.T) { Build() require.NoError(t, err) - current := &rbacv1.ClusterRole{} - require.NoError(t, res.Mutate(current)) + obj, err := res.Object() + require.NoError(t, err) + require.NoError(t, res.Mutate(obj)) - require.Len(t, current.Rules, 2) - assert.Equal(t, []string{"get", "list"}, current.Rules[0].Verbs) - assert.Equal(t, []string{"deployments"}, current.Rules[1].Resources) + got := obj.(*rbacv1.ClusterRole) + require.Len(t, got.Rules, 2) + assert.Equal(t, []string{"get", "list"}, got.Rules[0].Verbs) + assert.Equal(t, []string{"deployments"}, got.Rules[1].Resources) } func TestResource_Mutate_FeatureOrdering(t *testing.T) { @@ -107,47 +111,16 @@ func TestResource_Mutate_FeatureOrdering(t *testing.T) { Build() require.NoError(t, err) - current := &rbacv1.ClusterRole{} - require.NoError(t, res.Mutate(current)) - - // Base rule + feature-a + feature-b = 3 rules in order. - require.Len(t, current.Rules, 3) - assert.Equal(t, []string{"pods"}, current.Rules[0].Resources) - assert.Equal(t, []string{"secrets"}, current.Rules[1].Resources) - assert.Equal(t, []string{"configmaps"}, current.Rules[2].Resources) -} - -func TestResource_Mutate_CustomFieldApplicator(t *testing.T) { - desired := newValidCR() - - applicatorCalled := false - res, err := NewBuilder(desired). - WithCustomFieldApplicator(func(current, d *rbacv1.ClusterRole) error { - applicatorCalled = true - current.Rules = d.Rules - return nil - }). - Build() - require.NoError(t, err) - - current := &rbacv1.ClusterRole{} - require.NoError(t, res.Mutate(current)) - - assert.True(t, applicatorCalled) - require.Len(t, current.Rules, 1) -} - -func TestResource_Mutate_CustomFieldApplicator_Error(t *testing.T) { - res, err := NewBuilder(newValidCR()). - WithCustomFieldApplicator(func(_, _ *rbacv1.ClusterRole) error { - return errors.New("applicator error") - }). - Build() + obj, err := res.Object() require.NoError(t, err) + require.NoError(t, res.Mutate(obj)) - err = res.Mutate(&rbacv1.ClusterRole{}) - require.Error(t, err) - assert.Contains(t, err.Error(), "applicator error") + got := obj.(*rbacv1.ClusterRole) + // Base rule + feature-a + feature-b = 3 rules in order. + require.Len(t, got.Rules, 3) + assert.Equal(t, []string{"pods"}, got.Rules[0].Resources) + assert.Equal(t, []string{"secrets"}, got.Rules[1].Resources) + assert.Equal(t, []string{"configmaps"}, got.Rules[2].Resources) } func TestResource_ExtractData(t *testing.T) { @@ -178,49 +151,3 @@ func TestResource_ExtractData_Error(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "extract error") } - -func TestDefaultFieldApplicator_PreservesServerManagedFields(t *testing.T) { - current := &rbacv1.ClusterRole{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test", - ResourceVersion: "12345", - UID: "abc-def", - Generation: 3, - OwnerReferences: []metav1.OwnerReference{ - {APIVersion: "v1", Kind: "Pod", Name: "other-owner", UID: "other-uid"}, - }, - Finalizers: []string{"finalizer.example.com"}, - }, - } - desired := &rbacv1.ClusterRole{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test", - Labels: map[string]string{"app": "test"}, - }, - Rules: []rbacv1.PolicyRule{ - { - APIGroups: []string{""}, - Resources: []string{"pods"}, - Verbs: []string{"get", "list"}, - }, - }, - } - - err := DefaultFieldApplicator(current, desired) - require.NoError(t, err) - - // Desired spec and labels are applied - assert.Equal(t, "test", current.Labels["app"]) - require.Len(t, current.Rules, 1) - assert.Equal(t, []string{"get", "list"}, current.Rules[0].Verbs) - - // Server-managed fields are preserved - assert.Equal(t, "12345", current.ResourceVersion) - assert.Equal(t, "abc-def", string(current.UID)) - assert.Equal(t, int64(3), current.Generation) - - // Shared-controller fields are preserved - assert.Len(t, current.OwnerReferences, 1) - assert.Equal(t, "other-owner", current.OwnerReferences[0].Name) - assert.Equal(t, []string{"finalizer.example.com"}, current.Finalizers) -} From d502741b370843d2390ac88e5cf43a41bee4169f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Wed, 25 Mar 2026 16:03:12 +0000 Subject: [PATCH 25/28] fix: address copilot review comments on clusterrole primitive PR - Document why clusterrole-primitive example is excluded from make run-examples - Use slices.Delete in PolicyRulesEditor.RemoveRuleByIndex to avoid memory retention Co-Authored-By: Claude Opus 4.6 (1M context) --- docs/primitives.md | 4 ++++ pkg/mutation/editors/policyrules.go | 8 ++++++-- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/docs/primitives.md b/docs/primitives.md index 87c5f951..0bd97777 100644 --- a/docs/primitives.md +++ b/docs/primitives.md @@ -148,6 +148,10 @@ have been applied. This means a single mutation can safely add a container and t | `pkg/primitives/configmap` | Static | [configmap.md](primitives/configmap.md) | | `pkg/primitives/clusterrole` | Static | [clusterrole.md](primitives/clusterrole.md) | +The `clusterrole` primitive is exercised by the `examples/clusterrole-primitive` example. Because it requires +cluster-scoped RBAC and may need elevated permissions, it is intentionally not included in the default +`make run-examples` target used for CI/local smoke runs. + ## Usage Examples ### Creating a primitive diff --git a/pkg/mutation/editors/policyrules.go b/pkg/mutation/editors/policyrules.go index 60ce4f9c..e472ae04 100644 --- a/pkg/mutation/editors/policyrules.go +++ b/pkg/mutation/editors/policyrules.go @@ -1,6 +1,10 @@ package editors -import rbacv1 "k8s.io/api/rbac/v1" +import ( + "slices" + + rbacv1 "k8s.io/api/rbac/v1" +) // PolicyRulesEditor provides a typed API for mutating the .rules field of // a Kubernetes Role or ClusterRole. @@ -49,7 +53,7 @@ func (e *PolicyRulesEditor) RemoveRuleByIndex(index int) { if index < 0 || index >= len(*e.rules) { return } - *e.rules = append((*e.rules)[:index], (*e.rules)[index+1:]...) + *e.rules = slices.Delete(*e.rules, index, index+1) } // Clear removes all rules. From 2ff185dbba2adc3535a6a879c4f3655ab683ec05 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Wed, 25 Mar 2026 16:16:37 +0000 Subject: [PATCH 26/28] fix: panic on nil pointer in NewPolicyRulesEditor instead of silently discarding edits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Passing a nil *[]PolicyRule pointer to NewPolicyRulesEditor is a programmer error — edits would be recorded against a local variable and never propagate back to the Kubernetes object. Fail fast with a clear panic message instead of silently swallowing the mistake. Co-Authored-By: Claude Opus 4.6 (1M context) --- pkg/mutation/editors/policyrules.go | 10 +++++----- pkg/mutation/editors/policyrules_test.go | 6 ++++++ 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/pkg/mutation/editors/policyrules.go b/pkg/mutation/editors/policyrules.go index e472ae04..91ba7bfd 100644 --- a/pkg/mutation/editors/policyrules.go +++ b/pkg/mutation/editors/policyrules.go @@ -18,13 +18,13 @@ type PolicyRulesEditor struct { // NewPolicyRulesEditor creates a new PolicyRulesEditor wrapping the given rules // slice pointer. // -// The pointer may refer to a nil slice; methods that append rules initialise it -// automatically. Pass a non-nil pointer (e.g. &cr.Rules) so that writes are -// reflected on the object. +// The pointer itself must be non-nil — passing a nil pointer is a programmer +// error and will cause a panic. It may refer to a nil slice; methods that +// append rules initialise it automatically. Pass a non-nil pointer (e.g. +// &cr.Rules) so that writes are reflected on the object. func NewPolicyRulesEditor(rules *[]rbacv1.PolicyRule) *PolicyRulesEditor { if rules == nil { - var r []rbacv1.PolicyRule - rules = &r + panic("NewPolicyRulesEditor: rules must be a non-nil pointer") } return &PolicyRulesEditor{rules: rules} } diff --git a/pkg/mutation/editors/policyrules_test.go b/pkg/mutation/editors/policyrules_test.go index 32926981..cab20647 100644 --- a/pkg/mutation/editors/policyrules_test.go +++ b/pkg/mutation/editors/policyrules_test.go @@ -91,6 +91,12 @@ func TestPolicyRulesEditor_Raw(t *testing.T) { assert.Equal(t, "configmaps", rules[0].Resources[0]) } +func TestNewPolicyRulesEditor_PanicsOnNilPointer(t *testing.T) { + assert.PanicsWithValue(t, "NewPolicyRulesEditor: rules must be a non-nil pointer", func() { + NewPolicyRulesEditor(nil) + }) +} + func TestPolicyRulesEditor_Raw_NilInitialized(t *testing.T) { var rules []rbacv1.PolicyRule e := NewPolicyRulesEditor(&rules) From 48bcb6b4657414c27882f5c4f799d94807bd956e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Wed, 25 Mar 2026 16:26:02 +0000 Subject: [PATCH 27/28] fix: rename misleading test to accurately describe its assertion Co-Authored-By: Claude Opus 4.6 (1M context) --- pkg/primitives/clusterrole/builder_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/primitives/clusterrole/builder_test.go b/pkg/primitives/clusterrole/builder_test.go index c5b1aa77..d3c7283a 100644 --- a/pkg/primitives/clusterrole/builder_test.go +++ b/pkg/primitives/clusterrole/builder_test.go @@ -70,14 +70,14 @@ func TestBuilder_Build_NoNamespaceRequired(t *testing.T) { assert.Equal(t, "rbac.authorization.k8s.io/v1/ClusterRole/cluster-scoped", res.Identity()) } -func TestBuilder_Build_NamespaceNotPersisted(t *testing.T) { +func TestBuilder_Build_DoesNotSetNamespace(t *testing.T) { t.Parallel() cr := &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{Name: "test-cr"}, } _, err := NewBuilder(cr).Build() require.NoError(t, err) - assert.Empty(t, cr.Namespace, "namespace should remain empty after Build") + assert.Empty(t, cr.Namespace, "namespace should not be set by Build") } func TestBuilder_WithMutation(t *testing.T) { From 31e8bd2aa6981dca27ee72c0ab262351edc7f322 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Wed, 25 Mar 2026 16:59:32 +0000 Subject: [PATCH 28/28] fix: add explicit panic when BeginFeature not called before mutations Mutator methods that access m.active (EditObjectMetadata, EditRules, SetAggregationRule) now call requireActive() which panics with a clear message instead of an opaque nil-pointer dereference. Co-Authored-By: Claude Opus 4.6 (1M context) --- pkg/primitives/clusterrole/mutator.go | 9 +++++++++ pkg/primitives/clusterrole/mutator_test.go | 21 +++++++++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/pkg/primitives/clusterrole/mutator.go b/pkg/primitives/clusterrole/mutator.go index d5a31ecc..e609c907 100644 --- a/pkg/primitives/clusterrole/mutator.go +++ b/pkg/primitives/clusterrole/mutator.go @@ -50,6 +50,12 @@ func (m *Mutator) BeginFeature() { m.active = &m.plans[len(m.plans)-1] } +func (m *Mutator) requireActive() { + if m.active == nil { + panic("clusterrole.Mutator: BeginFeature must be called before registering mutations") + } +} + // EditObjectMetadata records a mutation for the ClusterRole's own metadata. // // Metadata edits are applied before rules edits within the same feature. @@ -58,6 +64,7 @@ func (m *Mutator) EditObjectMetadata(edit func(*editors.ObjectMetaEditor) error) if edit == nil { return } + m.requireActive() m.active.metadataEdits = append(m.active.metadataEdits, edit) } @@ -73,6 +80,7 @@ func (m *Mutator) EditRules(edit func(*editors.PolicyRulesEditor) error) { if edit == nil { return } + m.requireActive() m.active.rulesEdits = append(m.active.rulesEdits, edit) } @@ -99,6 +107,7 @@ func (m *Mutator) SetAggregationRule(rule *rbacv1.AggregationRule) { if rule != nil { copied = rule.DeepCopy() } + m.requireActive() m.active.aggregationRuleSets = append(m.active.aggregationRuleSets, copied) } diff --git a/pkg/primitives/clusterrole/mutator_test.go b/pkg/primitives/clusterrole/mutator_test.go index 171fc85d..3be26e12 100644 --- a/pkg/primitives/clusterrole/mutator_test.go +++ b/pkg/primitives/clusterrole/mutator_test.go @@ -307,6 +307,27 @@ func TestBeginFeature_IsolatesFeaturePlans(t *testing.T) { assert.Len(t, m.plans[1].rulesEdits, 1, "second plan should have one rules edit") } +func TestMutator_PanicsWithoutBeginFeature(t *testing.T) { + cr := newTestCR(nil) + m := NewMutator(cr) + + assert.PanicsWithValue(t, + "clusterrole.Mutator: BeginFeature must be called before registering mutations", + func() { m.EditObjectMetadata(func(*editors.ObjectMetaEditor) error { return nil }) }, + "EditObjectMetadata should panic without BeginFeature", + ) + assert.PanicsWithValue(t, + "clusterrole.Mutator: BeginFeature must be called before registering mutations", + func() { m.EditRules(func(*editors.PolicyRulesEditor) error { return nil }) }, + "EditRules should panic without BeginFeature", + ) + assert.PanicsWithValue(t, + "clusterrole.Mutator: BeginFeature must be called before registering mutations", + func() { m.SetAggregationRule(nil) }, + "SetAggregationRule should panic without BeginFeature", + ) +} + func TestMutator_SingleFeature_PlanCount(t *testing.T) { cr := newTestCR(nil) m := NewMutator(cr)