diff --git a/pkg/controller/operconfig/operconfig_controller.go b/pkg/controller/operconfig/operconfig_controller.go index a04e0faf38..6a86cde368 100644 --- a/pkg/controller/operconfig/operconfig_controller.go +++ b/pkg/controller/operconfig/operconfig_controller.go @@ -215,8 +215,9 @@ func (r *ReconcileOperConfig) Reconcile(ctx context.Context, request reconcile.R r.status.SetDegraded(statusmanager.OperatorConfig, "NoOperatorConfig", fmt.Sprintf("Operator configuration %s was deleted", request.String())) // Request object not found, could have been deleted after reconcile request. - // Owned objects are automatically garbage collected, since we set - // the ownerReference (see https://kubernetes.io/docs/concepts/workloads/controllers/garbage-collection/). + // Most owned objects are automatically garbage collected, since we + // set the ownerReference. CRDs are excluded from controller + // ownership to avoid conflicts with other operators. // Return and don't requeue return reconcile.Result{}, nil } @@ -477,9 +478,15 @@ func (r *ReconcileOperConfig) Reconcile(ctx context.Context, request reconcile.R var degradedErr error for _, obj := range objs { // TODO: OwnerRef for non default clusters. For HyperShift this should probably be HostedControlPlane CR - if apply.GetClusterName(obj) == "" { + clusterName := apply.GetClusterName(obj) + // Skip controller ownership on CRDs: GC-deleting a CRD destroys + // all its CRs (data loss), and other operators (e.g. MetalLB via + // ClusterExtension/boxcutter) may legitimately claim controller + // ownership of the same CRDs, which conflicts because Kubernetes + // only allows a single controller ownerReference per object. + if clusterName == "" && !isCRD(obj) { // Mark the object to be GC'd if the owner is deleted. - if err := controllerutil.SetControllerReference(operConfig, obj, r.client.ClientFor(apply.GetClusterName(obj)).Scheme()); err != nil { + if err := controllerutil.SetControllerReference(operConfig, obj, r.client.ClientFor(clusterName).Scheme()); err != nil { err = errors.Wrapf(err, "could not set reference for (%s) %s/%s", obj.GroupVersionKind(), obj.GetNamespace(), obj.GetName()) log.Println(err) r.status.SetDegraded(statusmanager.OperatorConfig, "InternalError", @@ -563,6 +570,11 @@ func updateIPsecMetric(newOperConfigSpec *operv1.NetworkSpec) { } } +func isCRD(obj *uns.Unstructured) bool { + gvk := obj.GroupVersionKind() + return gvk.Kind == "CustomResourceDefinition" && gvk.Group == "apiextensions.k8s.io" +} + func reconcileOperConfig(ctx context.Context, obj crclient.Object) []reconcile.Request { log.Printf("%s %s/%s changed, triggering operconf reconciliation", obj.GetObjectKind().GroupVersionKind().Kind, obj.GetNamespace(), obj.GetName()) // Update reconcile.Request object to align with unnamespaced default network, diff --git a/pkg/controller/operconfig/operconfig_controller_test.go b/pkg/controller/operconfig/operconfig_controller_test.go new file mode 100644 index 0000000000..70b55e7b02 --- /dev/null +++ b/pkg/controller/operconfig/operconfig_controller_test.go @@ -0,0 +1,119 @@ +package operconfig + +import ( + "testing" + + operv1 "github.com/openshift/api/operator/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + uns "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + utilruntime "k8s.io/apimachinery/pkg/util/runtime" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" +) + +func TestSetControllerReferenceSkipsCRDs(t *testing.T) { + scheme := runtime.NewScheme() + utilruntime.Must(operv1.Install(scheme)) + + owner := &operv1.Network{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster", + UID: "test-uid", + }, + } + + tests := []struct { + name string + obj *uns.Unstructured + expectOwnerRef bool + }{ + { + name: "Deployment gets controller ownerReference", + obj: &uns.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "apps/v1", + "kind": "Deployment", + "metadata": map[string]interface{}{ + "name": "test-deployment", + "namespace": "openshift-network-operator", + }, + }, + }, + expectOwnerRef: true, + }, + { + name: "CRD does not get controller ownerReference", + obj: &uns.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "apiextensions.k8s.io/v1", + "kind": "CustomResourceDefinition", + "metadata": map[string]interface{}{ + "name": "frrconfigurations.frrk8s.metallb.io", + }, + }, + }, + expectOwnerRef: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if !isCRD(tc.obj) { + if err := controllerutil.SetControllerReference(owner, tc.obj, scheme); err != nil { + t.Fatalf("SetControllerReference failed: %v", err) + } + } + + controllerRef := metav1.GetControllerOf(tc.obj) + if tc.expectOwnerRef && controllerRef == nil { + t.Error("expected controller ownerReference, got none") + } + if !tc.expectOwnerRef && controllerRef != nil { + t.Errorf("expected no controller ownerReference, got %v", controllerRef) + } + }) + } +} + +func TestIsCRD(t *testing.T) { + tests := []struct { + name string + apiVersion string + kind string + expected bool + }{ + { + name: "apiextensions CRD", + apiVersion: "apiextensions.k8s.io/v1", + kind: "CustomResourceDefinition", + expected: true, + }, + { + name: "non-CRD resource with same Kind in different group", + apiVersion: "example.com/v1", + kind: "CustomResourceDefinition", + expected: false, + }, + { + name: "Deployment", + apiVersion: "apps/v1", + kind: "Deployment", + expected: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + obj := &uns.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": tc.apiVersion, + "kind": tc.kind, + "metadata": map[string]interface{}{"name": "test"}, + }, + } + if got := isCRD(obj); got != tc.expected { + t.Errorf("isCRD() = %v, want %v", got, tc.expected) + } + }) + } +}