diff --git a/internal/operator-controller/applier/boxcutter.go b/internal/operator-controller/applier/boxcutter.go index b2621f11d9..3abe5e9223 100644 --- a/internal/operator-controller/applier/boxcutter.go +++ b/internal/operator-controller/applier/boxcutter.go @@ -465,6 +465,16 @@ func (bc *Boxcutter) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.Clust } } + // Refuse to adopt a ClusterObjectDeployment that is already controlled by a + // different owner. We adopt one that has no controller (stamping our controller + // ownerReference via the apply below) or one we already control, but taking over a + // deployment controlled by someone else would fight that owner. Returning an + // ordinary error lets the ClusterExtension report Progressing=True/Retrying and + // use the normal controller retry backoff. + if err := bc.checkAdoptable(ctx, ext); err != nil { + return false, "", err + } + // Generate the desired ClusterObjectDeployment and the Secrets its template refs. cod, secrets, err := bc.RevisionGenerator.GenerateClusterObjectDeployment(ctx, contentFS, ext, objectLabels, revisionAnnotations) if err != nil { @@ -522,6 +532,34 @@ func (bc *Boxcutter) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.Clust return true, "", nil } +// checkAdoptable verifies that the applier may own the ClusterObjectDeployment named +// after the ClusterExtension. It is adoptable when it does not yet exist, has no +// controller ownerReference, is already controlled by this ClusterExtension, or is +// controlled by a reference that matches this ClusterExtension by group, kind, and name +// but with a different UID — the latter indicates the ClusterExtension was deleted and +// recreated (for example a backup/restore), which comes back with a fresh UID, so the +// deployment is logically still ours. When it is controlled by a genuinely different +// owner, it returns an error rather than fighting that owner for control. +func (bc *Boxcutter) checkAdoptable(ctx context.Context, ext *ocv1.ClusterExtension) error { + existing := &ocv1.ClusterObjectDeployment{} + switch err := bc.Client.Get(ctx, client.ObjectKey{Name: ext.Name}, existing); { + case apierrors.IsNotFound(err): + return nil + case err != nil: + return fmt.Errorf("getting ClusterObjectDeployment %q: %w", ext.Name, err) + } + + controller := metav1.GetControllerOf(existing) + if controller == nil || controller.UID == ext.UID { + return nil + } + + return fmt.Errorf( + "ClusterObjectDeployment %q is already controlled by %s %q; refusing to adopt it", + ext.Name, controller.Kind, controller.Name, + ) +} + // runPreflights runs the configured preflight checks for a desired apply. It determines whether // this is an install or an upgrade from whether the ClusterObjectDeployment already exists (a // direct, controller-owned signal, rather than our own ClusterExtension status which may be stale diff --git a/internal/operator-controller/applier/boxcutter_test.go b/internal/operator-controller/applier/boxcutter_test.go index 6d7351f403..4bdb426929 100644 --- a/internal/operator-controller/applier/boxcutter_test.go +++ b/internal/operator-controller/applier/boxcutter_test.go @@ -18,6 +18,7 @@ import ( "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/reconcile" ocv1 "github.com/operator-framework/operator-controller/api/v1" ocv1ac "github.com/operator-framework/operator-controller/applyconfigurations/api/v1" @@ -869,6 +870,100 @@ func TestBoxcutterApply_NilContentUsesExistingCOD(t *testing.T) { }) } +// TestBoxcutterApply_Adoption verifies the applier only adopts an existing +// ClusterObjectDeployment when it is not already controlled by a different owner. +func TestBoxcutterApply_Adoption(t *testing.T) { + type testCase struct { + name string + ext *ocv1.ClusterExtension + cod *ocv1.ClusterObjectDeployment + assert func(t *testing.T, cl client.Client, applyCompleted bool, applyStatus string, applyErr error) + wantExtControllerOwner bool + } + + for _, tc := range []testCase{ + { + name: "adopts an existing deployment with no controller owner reference", + ext: &ocv1.ClusterExtension{ObjectMeta: metav1.ObjectMeta{Name: "test-ext", UID: "test-uid"}}, + cod: &ocv1.ClusterObjectDeployment{ + ObjectMeta: metav1.ObjectMeta{Name: "test-ext"}, + }, + assert: func(t *testing.T, _ client.Client, applyCompleted bool, applyStatus string, applyErr error) { + require.NoError(t, applyErr) + require.True(t, applyCompleted) + require.Empty(t, applyStatus) + }, + wantExtControllerOwner: true, + }, + { + name: "proceeds when the existing deployment is already controlled by this extension", + ext: &ocv1.ClusterExtension{ObjectMeta: metav1.ObjectMeta{Name: "test-ext", UID: "test-uid"}}, + cod: &ocv1.ClusterObjectDeployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-ext", + OwnerReferences: []metav1.OwnerReference{*metav1.NewControllerRef(&ocv1.ClusterExtension{ObjectMeta: metav1.ObjectMeta{Name: "test-ext", UID: "test-uid"}}, ocv1.GroupVersion.WithKind(ocv1.ClusterExtensionKind))}, + }, + }, + assert: func(t *testing.T, _ client.Client, applyCompleted bool, applyStatus string, applyErr error) { + require.NoError(t, applyErr) + require.True(t, applyCompleted) + require.Empty(t, applyStatus) + }, + wantExtControllerOwner: true, + }, + { + name: "refuses to adopt a deployment controlled by a different owner", + ext: &ocv1.ClusterExtension{ObjectMeta: metav1.ObjectMeta{Name: "test-ext", UID: "test-uid"}}, + cod: &ocv1.ClusterObjectDeployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-ext", + OwnerReferences: []metav1.OwnerReference{*metav1.NewControllerRef(&ocv1.ClusterExtension{ObjectMeta: metav1.ObjectMeta{Name: "other-ext", UID: "other-uid"}}, ocv1.GroupVersion.WithKind(ocv1.ClusterExtensionKind))}, + }, + }, + assert: func(t *testing.T, cl client.Client, applyCompleted bool, applyStatus string, applyErr error) { + require.Error(t, applyErr) + assert.False(t, applyCompleted) + assert.Empty(t, applyStatus) + + t.Log("by surfacing a non-terminal error so retries occur without CE changes") + require.NotErrorIs(t, applyErr, reconcile.TerminalError(nil), "error was terminal, but expected non-terminal error so that CE retries occur") + + t.Log("by naming the deployment and the conflicting owner in the message") + assert.Contains(t, applyErr.Error(), "test-ext") + assert.Contains(t, applyErr.Error(), "other-ext") + + secretList := &corev1.SecretList{} + require.NoError(t, cl.List(t.Context(), secretList, client.InNamespace("olmv1-system"))) + assert.Empty(t, secretList.Items) + }, + wantExtControllerOwner: false, + }, + } { + t.Run(tc.name, func(t *testing.T) { + scheme := codTestScheme(t) + cl := fake.NewClientBuilder().WithScheme(scheme).WithObjects(tc.cod).Build() + bc := newCODBoxcutter(t, scheme, cl, []client.Object{ + &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "test-cm", Namespace: "test-ns"}}, + }) + + actualCompleted, actualStatus, actualErr := bc.Apply(t.Context(), dummyBundle, tc.ext, map[string]string{"some": "label"}, map[string]string{}) + tc.assert(t, cl, actualCompleted, actualStatus, actualErr) + + cod := &ocv1.ClusterObjectDeployment{} + require.NoError(t, cl.Get(t.Context(), client.ObjectKey{Name: tc.ext.Name}, cod)) + owner := metav1.GetControllerOf(cod) + if tc.wantExtControllerOwner { + require.NotNil(t, owner) + assert.Equal(t, tc.ext.Name, owner.Name) + assert.Equal(t, tc.ext.UID, owner.UID) + assert.Equal(t, ocv1.ClusterExtensionKind, owner.Kind) + } else { + require.Equal(t, metav1.GetControllerOf(tc.cod), owner) + } + }) + } +} + // TestBoxcutterApply_Preflights verifies which preflight hook runs for each rollout state // and that a preflight failure propagates out of Apply. Each row is a sequence of applies // against the same Boxcutter: a step renders its own objects, so identical content between diff --git a/test/e2e/features/cod_adoption_conflict.feature b/test/e2e/features/cod_adoption_conflict.feature new file mode 100644 index 0000000000..d3d454f2d0 --- /dev/null +++ b/test/e2e/features/cod_adoption_conflict.feature @@ -0,0 +1,56 @@ +@BoxcutterRuntime +Feature: ClusterExtension refuses to adopt a ClusterObjectDeployment owned by another resource + + As an OLM user, I want a ClusterExtension to report and retry an adoption conflict, + so that it does not take over a ClusterObjectDeployment controlled by another owner + and can proceed after that deployment is removed. + + Background: + Given OLM is available + And an image registry is available + And namespace "${TEST_NAMESPACE}" is available + And a catalog "test" with packages: + | package | version | channel | replaces | contents | + | test | 1.2.0 | beta | | CRD, Deployment, ConfigMap | + + Scenario: ClusterExtension retries after a foreign-owned ClusterObjectDeployment is removed + # Note: we're using a namespace owner in lieu of an existing controller to simulate foreign controller ownership + Given ClusterObjectDeployment with Namespace owner "${TEST_NAMESPACE}" is applied + """ + apiVersion: olm.operatorframework.io/v1 + kind: ClusterObjectDeployment + metadata: + name: ${NAME} + spec: + template: + spec: + collisionProtection: Prevent + phases: + - name: resources + objects: [] + """ + When ClusterExtension is applied + """ + apiVersion: olm.operatorframework.io/v1 + kind: ClusterExtension + metadata: + name: ${NAME} + spec: + namespace: ${TEST_NAMESPACE} + source: + sourceType: Catalog + catalog: + packageName: ${PACKAGE:test} + version: 1.2.0 + selector: + matchLabels: + "olm.operatorframework.io/metadata.name": ${CATALOG:test} + """ + Then ClusterExtension reports Progressing as True with Reason Retrying and Message includes: + """ + ClusterObjectDeployment "${NAME}" is already controlled by Namespace "${TEST_NAMESPACE}" + """ + When resource "clusterobjectdeployment/${NAME}" is removed + Then ClusterExtension is rolled out + And ClusterExtension is available + And ClusterObjectDeployment "${NAME}" is owned by ClusterExtension "${NAME}" diff --git a/test/e2e/steps/group_steps.go b/test/e2e/steps/group_steps.go index 685c4914b4..8451d3b59f 100644 --- a/test/e2e/steps/group_steps.go +++ b/test/e2e/steps/group_steps.go @@ -17,9 +17,9 @@ import ( ocv1 "github.com/operator-framework/operator-controller/api/v1" ) -// An empty owner name creates an ownerless revision. Resolve a real cluster-scoped -// owner before creation so garbage collection and reconciliation see valid metadata. -func ClusterObjectSetWithNamespaceOwnerApplied(ctx context.Context, ownerName string, template *godog.DocString) error { +// ResourceWithNamespaceOwnerApplied resolves a real cluster-scoped Namespace owner +// before applying the resource so garbage collection sees valid owner metadata. +func ResourceWithNamespaceOwnerApplied(ctx context.Context, ownerName string, template *godog.DocString) error { sc := scenarioCtx(ctx) obj, err := toUnstructured(substituteScenarioVars(template.Content, sc)) if err != nil { diff --git a/test/e2e/steps/steps.go b/test/e2e/steps/steps.go index d524d5ccdd..90ee4cc27f 100644 --- a/test/e2e/steps/steps.go +++ b/test/e2e/steps/steps.go @@ -134,7 +134,8 @@ func RegisterSteps(sc *godog.ScenarioContext) { sc.Step(`^(?i)ClusterObjectDeployment "([^"]+)" consistently reports ([[:alnum:]]+) as ([[:alnum:]]+) with Reason ([[:alnum:]]+) for "([^"]+)"$`, ClusterObjectDeploymentConsistentlyReportsCondition) sc.Step(`^(?i)ClusterExtension reports ([[:alnum:]]+) transition between (\d+) and (\d+) minutes since ClusterObjectSet "([^"]+)" was created$`, ClusterExtensionReportsConditionTransitionTime) sc.Step(`^(?i)ClusterObjectSet is applied(?:\s+.*)?$`, ResourceIsApplied) - sc.Step(`^(?i)ClusterObjectSet with Namespace owner "([^"]*)" is applied$`, ClusterObjectSetWithNamespaceOwnerApplied) + sc.Step(`^(?i)ClusterObjectSet with Namespace owner "([^"]*)" is applied$`, ResourceWithNamespaceOwnerApplied) + sc.Step(`^(?i)ClusterObjectDeployment with Namespace owner "([^"]*)" is applied$`, ResourceWithNamespaceOwnerApplied) sc.Step(`^(?i)ClusterObjectSet "([^"]+)" reconciliation is triggered$`, TriggerClusterObjectSetReconciliation) sc.Step(`^(?i)ClusterObjectSet "([^"]+)" has observed phase "([^"]+)" with a non-empty digest$`, ClusterObjectSetHasObservedPhase) sc.Step(`^(?i)ClusterObjectSet "([^"]+)" is archived$`, ClusterObjectSetIsArchived)