Move AggregationRuleTransform to shared common package to fix eventing RBAC race

pkg/reconciler/knativeserving/common/aggregated_rules.go → 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

pkg/reconciler/knativeserving/common/aggregated_rules_test.go → 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 {

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...)

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),
 	)