diff --git a/pkg/reconciler/knativeserving/common/aggregated_rules.go b/pkg/reconciler/common/aggregated_rules.go similarity index 81% rename from pkg/reconciler/knativeserving/common/aggregated_rules.go rename to pkg/reconciler/common/aggregated_rules.go index bf97fe63c5..066d7cef70 100644 --- a/pkg/reconciler/knativeserving/common/aggregated_rules.go +++ b/pkg/reconciler/common/aggregated_rules.go @@ -26,12 +26,13 @@ type unstructuredGetter interface { Get(obj *unstructured.Unstructured) (*unstructured.Unstructured, error) } -// AggregationRuleTransform +// AggregationRuleTransform preserves the rules of aggregated ClusterRoles. +// The Kubernetes aggregation controller manages the rules field of aggregated +// ClusterRoles. Without this transform, manifestival overwrites the aggregated +// rules with the empty rules from the manifest, causing a race condition. func AggregationRuleTransform(client unstructuredGetter) mf.Transformer { return func(u *unstructured.Unstructured) error { if u.GetKind() == "ClusterRole" && u.Object["aggregationRule"] != nil { - // we rely on the controller manager to fill in rules so - // ours will always trigger an unnecessary update current, err := client.Get(u) if errors.IsNotFound(err) { return nil diff --git a/pkg/reconciler/knativeserving/common/aggregated_rules_test.go b/pkg/reconciler/common/aggregated_rules_test.go similarity index 77% rename from pkg/reconciler/knativeserving/common/aggregated_rules_test.go rename to pkg/reconciler/common/aggregated_rules_test.go index 304b4c5ef4..e66b599feb 100644 --- a/pkg/reconciler/knativeserving/common/aggregated_rules_test.go +++ b/pkg/reconciler/common/aggregated_rules_test.go @@ -74,6 +74,41 @@ func TestAggregationRuleTransform(t *testing.T) { serving.knative.dev/controller: "true" rules: [] overwriteExpected: false +- name: "existing eventing aggregated role has rules" + input: + kind: ClusterRole + apiVersion: rbac.authorization.k8s.io/v1 + metadata: + name: channelable-manipulator + aggregationRule: + clusterRoleSelectors: + - matchLabels: + duck.knative.dev/channelable: "true" + rules: [] + existing: + kind: ClusterRole + apiVersion: rbac.authorization.k8s.io/v1 + metadata: + name: channelable-manipulator + aggregationRule: + clusterRoleSelectors: + - matchLabels: + duck.knative.dev/channelable: "true" + rules: + - apiGroups: + - messaging.knative.dev + resources: + - channels + - channels/status + verbs: + - create + - get + - list + - watch + - update + - patch + - delete + overwriteExpected: true `) err := yaml.Unmarshal(testData, &tests) if err != nil { diff --git a/pkg/reconciler/common/transformers.go b/pkg/reconciler/common/transformers.go index dc7ae82d86..5bf1965d14 100644 --- a/pkg/reconciler/common/transformers.go +++ b/pkg/reconciler/common/transformers.go @@ -65,6 +65,7 @@ func Transform(ctx context.Context, manifest *mf.Manifest, instance base.KCompon logger.Debug("Transforming manifest") transformers := transformers(ctx, instance) + transformers = append(transformers, AggregationRuleTransform(manifest.Client)) transformers = append(transformers, extra...) m, err := manifest.Transform(transformers...) diff --git a/pkg/reconciler/knativeserving/knativeserving.go b/pkg/reconciler/knativeserving/knativeserving.go index bd13982674..b1446f40f6 100644 --- a/pkg/reconciler/knativeserving/knativeserving.go +++ b/pkg/reconciler/knativeserving/knativeserving.go @@ -160,7 +160,6 @@ func (r *Reconciler) transform(ctx context.Context, manifest *mf.Manifest, comp extra = append(extra, common.InjectOwner(instance, anchorOwner), ksc.CustomCertsTransform(instance, logger), - ksc.AggregationRuleTransform(manifest.Client), // Ensure all resources have the selector applied so that the controller re-queues applied resources when they change. common.InjectLabel(SelectorKey, SelectorValue), )