From 0ba2786894509bc6888941b0a3319e6c42f4d967 Mon Sep 17 00:00:00 2001 From: Raimund Sacherer Date: Tue, 20 Oct 2026 16:59:06 +0200 Subject: [PATCH 1/8] fix(catalog): defer CRD storage-version promotion until conversion webhook is live OLMv0 applies CRD updates in two separate controller writes: catalog-operator sets the new storage version with conversion=None, and olm-operator wires the conversion webhook seconds later. In the gap the API server uses the new schema, silently pruning fields from old-format writes that are absent in the new schema. This commit adds the olm-operator side of the fix: - Add PendingStorageVersionAnnotation constant (olm.operatorframework.io/pending-storage-version) as the hand-off signal between catalog-operator and olm-operator. - In createOrUpdateConversionWebhook, read the annotation and promote the storage version in the same Update call that activates the webhook (atomic). If the annotation names a version absent from the spec, log a warning and leave the CRD untouched so webhook wiring still succeeds. The companion catalog-operator change (step.go) that writes the annotation will follow in the next commit. Fixes: OCPBUGS-130601 Co-Authored-By: Claude Sonnet 4.6 (1M context) Signed-off-by: Raimund Sacherer --- pkg/controller/install/webhook.go | 30 ++++ pkg/controller/install/webhook_test.go | 216 +++++++++++++++++++++++++ 2 files changed, 246 insertions(+) create mode 100644 pkg/controller/install/webhook_test.go diff --git a/pkg/controller/install/webhook.go b/pkg/controller/install/webhook.go index d3dee19fb5..8e66ddc6f0 100644 --- a/pkg/controller/install/webhook.go +++ b/pkg/controller/install/webhook.go @@ -261,6 +261,28 @@ func (i *StrategyDeploymentInstaller) createOrUpdateConversionWebhook(caPEM []by }, } + // If catalog-operator deferred a storage-version promotion, apply it now in the + // same write that activates the conversion webhook, making the transition atomic. + // See PendingStorageVersionAnnotation for the full protocol. + if pending, ok := crd.Annotations[PendingStorageVersionAnnotation]; ok && pending != "" { + found := false + for _, v := range crd.Spec.Versions { + if v.Name == pending { + found = true + break + } + } + if found { + for idx := range crd.Spec.Versions { + crd.Spec.Versions[idx].Storage = crd.Spec.Versions[idx].Name == pending + } + delete(crd.Annotations, PendingStorageVersionAnnotation) + log.Infof("atomically promoted storage version %q on CRD %s with conversion webhook", pending, conversionCRD) + } else { + log.Warnf("pending storage version %q not found in CRD %s spec; skipping promotion (annotation retained for investigation)", pending, conversionCRD) + } + } + // update CRD conversion Specs if _, err = i.strategyClient.GetOpClient().ApiextensionsInterface().ApiextensionsV1().CustomResourceDefinitions().Update(context.TODO(), crd, metav1.UpdateOptions{}); err != nil { return fmt.Errorf("Error updating CRD with Conversion info: %w", err) @@ -277,6 +299,14 @@ func (i *StrategyDeploymentInstaller) createOrUpdateConversionWebhook(caPEM []by const WebhookDescKey = "olm.webhook-description-generate-name" const WebhookHashKey = "olm.webhook-description-hash" +// PendingStorageVersionAnnotation is written by catalog-operator when it defers a +// storage-version promotion. The value is the version name to promote. +// createOrUpdateConversionWebhook reads it and promotes storage atomically with +// webhook activation, then deletes it. If the operator is deleted before +// olm-operator processes it the annotation remains on the CRD; a later reinstall +// completes the promotion. +const PendingStorageVersionAnnotation = "olm.operatorframework.io/pending-storage-version" + // addWebhookLabels adds webhook labels to an object func addWebhookLabels(object metav1.Object, webhookDesc v1alpha1.WebhookDescription) error { labels := object.GetLabels() diff --git a/pkg/controller/install/webhook_test.go b/pkg/controller/install/webhook_test.go new file mode 100644 index 0000000000..158515cddb --- /dev/null +++ b/pkg/controller/install/webhook_test.go @@ -0,0 +1,216 @@ +package install + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + apiextensionsfake "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/fake" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + k8sfake "k8s.io/client-go/kubernetes/fake" + apiregistrationfake "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/fake" + + v1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1" + "github.com/operator-framework/operator-lifecycle-manager/pkg/api/wrappers" + "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient" + "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorlister/operatorlisterfakes" +) + +const ( + testCRDName = "tests.example.com" + testNamespace = "test-namespace" + testCSVName = "test-operator.v1.0.0" +) + +// singletonCSV returns a CSV that satisfies isSingletonOperator (AllNamespaces only). +func singletonCSV(crdName string) *v1alpha1.ClusterServiceVersion { + port := int32(443) + _ = port + return &v1alpha1.ClusterServiceVersion{ + ObjectMeta: metav1.ObjectMeta{ + Name: testCSVName, + Namespace: testNamespace, + }, + Spec: v1alpha1.ClusterServiceVersionSpec{ + InstallModes: []v1alpha1.InstallMode{ + {Type: v1alpha1.InstallModeTypeAllNamespaces, Supported: true}, + {Type: v1alpha1.InstallModeTypeSingleNamespace, Supported: false}, + {Type: v1alpha1.InstallModeTypeMultiNamespace, Supported: false}, + {Type: v1alpha1.InstallModeTypeOwnNamespace, Supported: false}, + }, + CustomResourceDefinitions: v1alpha1.CustomResourceDefinitions{ + Owned: []v1alpha1.CRDDescription{ + {Name: crdName, Version: "v2", Kind: "Test"}, + }, + }, + }, + } +} + +// conversionDesc returns a WebhookDescription of type ConversionWebhook for crdName. +func conversionDesc(crdName string) v1alpha1.WebhookDescription { + port := int32(443) + return v1alpha1.WebhookDescription{ + Type: v1alpha1.ConversionWebhook, + GenerateName: "test-webhook", + ConversionCRDs: []string{crdName}, + AdmissionReviewVersions: []string{"v1"}, + ContainerPort: port, + } +} + +// makeInstaller builds a StrategyDeploymentInstaller backed by a fake apiextensions client +// that has crd pre-loaded. Returns the installer and the fake client for post-call assertions. +func makeInstaller(t *testing.T, crd *apiextensionsv1.CustomResourceDefinition) (*StrategyDeploymentInstaller, *apiextensionsfake.Clientset) { + t.Helper() + crdFake := apiextensionsfake.NewSimpleClientset(crd) + opClient := operatorclient.NewClient( + k8sfake.NewSimpleClientset(), + crdFake, + apiregistrationfake.NewSimpleClientset(), + ) + fakeLister := &operatorlisterfakes.FakeOperatorLister{} + strategyClient := wrappers.NewInstallStrategyDeploymentClient(opClient, fakeLister, testNamespace) + installer := &StrategyDeploymentInstaller{ + strategyClient: strategyClient, + owner: singletonCSV(crd.Name), + } + return installer, crdFake +} + +// crdWithVersions builds a minimal CRD with the given spec versions and optional annotations. +func crdWithVersions(annotations map[string]string, versions ...apiextensionsv1.CustomResourceDefinitionVersion) *apiextensionsv1.CustomResourceDefinition { + return &apiextensionsv1.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{ + Name: testCRDName, + Annotations: annotations, + }, + Spec: apiextensionsv1.CustomResourceDefinitionSpec{ + Group: "example.com", + Names: apiextensionsv1.CustomResourceDefinitionNames{ + Plural: "tests", + Kind: "Test", + }, + Scope: apiextensionsv1.ClusterScoped, + Versions: versions, + }, + } +} + +func crdVersion(name string, storage bool) apiextensionsv1.CustomResourceDefinitionVersion { + return apiextensionsv1.CustomResourceDefinitionVersion{ + Name: name, + Served: true, + Storage: storage, + } +} + +// TestConversionWebhook_PromotesPendingStorageVersion verifies that when the +// PendingStorageVersionAnnotation is set to a version that exists in the CRD spec, +// createOrUpdateConversionWebhook promotes that version to storage=true, clears the +// annotation, and sets the conversion webhook — all in one Update call. +func TestConversionWebhook_PromotesPendingStorageVersion(t *testing.T) { + crd := crdWithVersions( + map[string]string{PendingStorageVersionAnnotation: "v3"}, + crdVersion("v2", true), + crdVersion("v3", false), + ) + installer, crdFake := makeInstaller(t, crd) + + err := installer.createOrUpdateConversionWebhook([]byte("fake-ca"), conversionDesc(testCRDName)) + require.NoError(t, err) + + updated, err := crdFake.ApiextensionsV1().CustomResourceDefinitions().Get(context.TODO(), testCRDName, metav1.GetOptions{}) + require.NoError(t, err) + + for _, v := range updated.Spec.Versions { + if v.Name == "v3" { + assert.True(t, v.Storage, "v3 should be the storage version after promotion") + } else { + assert.False(t, v.Storage, "v2 should no longer be the storage version after promotion") + } + } + assert.NotContains(t, updated.Annotations, PendingStorageVersionAnnotation, "annotation should be cleared after promotion") + require.NotNil(t, updated.Spec.Conversion, "conversion webhook should be set") + assert.Equal(t, apiextensionsv1.WebhookConverter, updated.Spec.Conversion.Strategy) +} + +// TestConversionWebhook_NoAnnotation verifies that when no PendingStorageVersionAnnotation +// is present, storage markers are unchanged and only the conversion webhook is set. +func TestConversionWebhook_NoAnnotation(t *testing.T) { + crd := crdWithVersions( + nil, + crdVersion("v2", true), + crdVersion("v3", false), + ) + installer, crdFake := makeInstaller(t, crd) + + err := installer.createOrUpdateConversionWebhook([]byte("fake-ca"), conversionDesc(testCRDName)) + require.NoError(t, err) + + updated, err := crdFake.ApiextensionsV1().CustomResourceDefinitions().Get(context.TODO(), testCRDName, metav1.GetOptions{}) + require.NoError(t, err) + + for _, v := range updated.Spec.Versions { + if v.Name == "v2" { + assert.True(t, v.Storage, "v2 should remain the storage version") + } else { + assert.False(t, v.Storage, "v3 should remain non-storage") + } + } + require.NotNil(t, updated.Spec.Conversion) + assert.Equal(t, apiextensionsv1.WebhookConverter, updated.Spec.Conversion.Strategy) +} + +// TestConversionWebhook_StaleAnnotationVersionNotInSpec verifies that when the +// PendingStorageVersionAnnotation names a version that does not exist in the CRD spec, +// storage markers are left untouched, the annotation is retained, and the conversion +// webhook is still set successfully. +func TestConversionWebhook_StaleAnnotationVersionNotInSpec(t *testing.T) { + crd := crdWithVersions( + map[string]string{PendingStorageVersionAnnotation: "v99"}, + crdVersion("v2", true), + crdVersion("v3", false), + ) + installer, crdFake := makeInstaller(t, crd) + + err := installer.createOrUpdateConversionWebhook([]byte("fake-ca"), conversionDesc(testCRDName)) + require.NoError(t, err) + + updated, err := crdFake.ApiextensionsV1().CustomResourceDefinitions().Get(context.TODO(), testCRDName, metav1.GetOptions{}) + require.NoError(t, err) + + for _, v := range updated.Spec.Versions { + if v.Name == "v2" { + assert.True(t, v.Storage, "v2 should remain storage when annotation version is absent from spec") + } else { + assert.False(t, v.Storage) + } + } + assert.Contains(t, updated.Annotations, PendingStorageVersionAnnotation, "stale annotation should be retained for investigation") + require.NotNil(t, updated.Spec.Conversion) + assert.Equal(t, apiextensionsv1.WebhookConverter, updated.Spec.Conversion.Strategy) +} + +// TestConversionWebhook_EmptyAnnotationSkipsPromotion verifies that an empty-string +// PendingStorageVersionAnnotation does not trigger the promotion block. +func TestConversionWebhook_EmptyAnnotationSkipsPromotion(t *testing.T) { + crd := crdWithVersions( + map[string]string{PendingStorageVersionAnnotation: ""}, + crdVersion("v2", true), + ) + installer, crdFake := makeInstaller(t, crd) + + err := installer.createOrUpdateConversionWebhook([]byte("fake-ca"), conversionDesc(testCRDName)) + require.NoError(t, err) + + updated, err := crdFake.ApiextensionsV1().CustomResourceDefinitions().Get(context.TODO(), testCRDName, metav1.GetOptions{}) + require.NoError(t, err) + + require.Len(t, updated.Spec.Versions, 1) + assert.True(t, updated.Spec.Versions[0].Storage, "storage marker should be unchanged when annotation is empty") + require.NotNil(t, updated.Spec.Conversion) + assert.Equal(t, apiextensionsv1.WebhookConverter, updated.Spec.Conversion.Strategy) +} From 98b80c6b04004e22918cd0696e810d3bd4bc2c71 Mon Sep 17 00:00:00 2001 From: Raimund Sacherer Date: Tue, 20 Oct 2026 17:08:12 +0200 Subject: [PATCH 2/8] fix(catalog): defer CRD storage-version promotion to conversion webhook write MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Introduces the catalog-operator side of the atomic storage-version promotion fix for OCPBUGS-130601. catalog-operator applies CRD bundles with the conversion webhook stripped (OCPBUGS-106160 fix). When the bundle promotes the CRD storage version, a ~3s window opens where the new schema is active but no conversion webhook is live. Existing operator writes using the old field layout are pruned silently by the API server. Changes: - storageVersionInSpec: helper returning the storage=true version name. - csvDeclaresConversionWebhookForCRD: guard that scans plan.Status.Plan for the CSV manifest and returns true only when the CSV declares a ConversionWebhook for the CRD being updated. Prevents deference for operators that legitimately promote storage without a webhook (those would be stuck at the old version forever if deferred). - Deference block in NewCRDV1Step: fires after SafeStorageVersionUpgrade when all three conditions are true — storage version changes, the bundle carries no conversion strategy, and the CSV declares a conversion webhook for this CRD. Reverts the storage marker to the current version and records the intended target in PendingStorageVersionAnnotation. The olm-operator reads this annotation in createOrUpdateConversionWebhook and promotes storage atomically with webhook activation (see prior commit). Fixes: OCPBUGS-130601 Co-Authored-By: Claude Sonnet 4.6 (1M context) Signed-off-by: Raimund Sacherer --- pkg/controller/operators/catalog/step.go | 64 +++ .../operators/catalog/step_deference_test.go | 367 ++++++++++++++++++ 2 files changed, 431 insertions(+) create mode 100644 pkg/controller/operators/catalog/step_deference_test.go diff --git a/pkg/controller/operators/catalog/step.go b/pkg/controller/operators/catalog/step.go index e2afbde78c..46c37ee098 100644 --- a/pkg/controller/operators/catalog/step.go +++ b/pkg/controller/operators/catalog/step.go @@ -2,6 +2,7 @@ package catalog import ( "context" + "encoding/json" "fmt" "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/install" @@ -176,6 +177,32 @@ Validation Error: %s return fmt.Errorf("checking CRD for potential data loss updating %q: %w", step.Resource.Name, err) } + // Defer storage-version promotion when the incoming CRD promotes storage + // but carries no conversion webhook. Atomically promoting storage with + // the webhook (see createOrUpdateConversionWebhook) eliminates the window + // where a new storage version is active without field conversion. + // Guard: only defer when the CSV that owns this CRD declares a + // ConversionWebhook for it; operators that legitimately promote storage + // without a webhook would otherwise be stuck at the old version forever. + if newVer, ok := storageVersionInSpec(crd); ok { + if currentVer, hasCurrent := storageVersionInSpec(currentCRD); hasCurrent && newVer != currentVer { + conversionMissing := crd.Spec.Conversion == nil || + crd.Spec.Conversion.Strategy == "" || + crd.Spec.Conversion.Strategy == apiextensionsv1.NoneConverter + if conversionMissing && csvDeclaresConversionWebhookForCRD(b.plan, step.Resolving, step.Resource.Name) { + for idx := range crd.Spec.Versions { + crd.Spec.Versions[idx].Storage = crd.Spec.Versions[idx].Name == currentVer + } + if crd.Annotations == nil { + crd.Annotations = map[string]string{} + } + crd.Annotations[install.PendingStorageVersionAnnotation] = newVer + b.logger.Infof("deferring storage-version promotion on %q: keeping %q as storage, pending %q until conversion webhook is active", + step.Resource.Name, currentVer, newVer) + } + } + } + // Update CRD to new version setInstalledAlongsideAnnotation(b.annotator, crd, b.plan.GetNamespace(), step.Resolving, b.csvLister, crd, currentCRD) _, err = client.CustomResourceDefinitions().Update(context.TODO(), crd, metav1.UpdateOptions{}) @@ -289,6 +316,43 @@ func (b *builder) NewCRDV1Beta1Step(client apiextensionsv1beta1client.Apiextensi } } +// storageVersionInSpec returns the name of the version marked Storage: true in the CRD spec. +func storageVersionInSpec(crd *apiextensionsv1.CustomResourceDefinition) (string, bool) { + for _, v := range crd.Spec.Versions { + if v.Storage { + return v.Name, true + } + } + return "", false +} + +// csvDeclaresConversionWebhookForCRD returns true when the CSV being installed in +// plan.Status.Plan declares a ConversionWebhook that lists crdName. +// plan.Status.Plan contains raw manifests for all steps before any step executes, +// so this works for both fresh installs and upgrades. +func csvDeclaresConversionWebhookForCRD(plan *v1alpha1.InstallPlan, csvName, crdName string) bool { + for _, step := range plan.Status.Plan { + if step.Resource.Kind != "ClusterServiceVersion" || step.Resource.Name != csvName { + continue + } + csv := &v1alpha1.ClusterServiceVersion{} + if err := json.Unmarshal([]byte(step.Resource.Manifest), csv); err != nil { + return false + } + for _, wh := range csv.Spec.WebhookDefinitions { + if wh.Type != v1alpha1.ConversionWebhook { + continue + } + for _, c := range wh.ConversionCRDs { + if c == crdName { + return true + } + } + } + } + return false +} + func setInstalledAlongsideAnnotation(a alongside.Annotator, dst metav1.Object, namespace string, name string, lister listersv1alpha1.ClusterServiceVersionLister, srcs ...metav1.Object) { var nns []alongside.NamespacedName diff --git a/pkg/controller/operators/catalog/step_deference_test.go b/pkg/controller/operators/catalog/step_deference_test.go new file mode 100644 index 0000000000..cb0149d900 --- /dev/null +++ b/pkg/controller/operators/catalog/step_deference_test.go @@ -0,0 +1,367 @@ +package catalog + +import ( + "context" + "encoding/json" + "testing" + + "github.com/sirupsen/logrus" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + apiextensionsfake "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/fake" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + fakedynamic "k8s.io/client-go/dynamic/fake" + k8sfake "k8s.io/client-go/kubernetes/fake" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/client-go/tools/record" + apiregistrationfake "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/fake" + + v1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1" + "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/install" + "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient" + "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorlister/operatorlisterfakes" +) + +// staticResolver implements ManifestResolver and always returns the same manifest. +type staticResolver struct{ manifest string } + +func (r staticResolver) ManifestForStep(_ *v1alpha1.Step) (string, error) { + return r.manifest, nil +} + +// minimalSchema returns a bare-minimum schema accepted by the CRD API. +func minimalSchema() *apiextensionsv1.CustomResourceValidation { + return &apiextensionsv1.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{Type: "object"}, + } +} + +// buildCRD builds a CRD with the given versions and optional annotations. +func buildCRD(name string, annotations map[string]string, versions ...apiextensionsv1.CustomResourceDefinitionVersion) *apiextensionsv1.CustomResourceDefinition { + return &apiextensionsv1.CustomResourceDefinition{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "apiextensions.k8s.io/v1", + Kind: "CustomResourceDefinition", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Annotations: annotations, + }, + Spec: apiextensionsv1.CustomResourceDefinitionSpec{ + Group: "example.com", + Names: apiextensionsv1.CustomResourceDefinitionNames{ + Plural: "tests", + Kind: "Test", + }, + Scope: apiextensionsv1.ClusterScoped, + Versions: versions, + }, + } +} + +func storedVersion(name string, storage bool) apiextensionsv1.CustomResourceDefinitionVersion { + return apiextensionsv1.CustomResourceDefinitionVersion{ + Name: name, + Served: true, + Storage: storage, + Schema: minimalSchema(), + } +} + +// crdManifest JSON-encodes a CRD so it can be used as a step manifest. +func crdManifest(t *testing.T, crd *apiextensionsv1.CustomResourceDefinition) string { + t.Helper() + b, err := json.Marshal(crd) + require.NoError(t, err) + return string(b) +} + +// csvManifestWithConversionWebhook JSON-encodes a CSV that declares a ConversionWebhook for crdName. +func csvManifestWithConversionWebhook(t *testing.T, csvName, crdName string) string { + t.Helper() + csv := &v1alpha1.ClusterServiceVersion{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "operators.coreos.com/v1alpha1", + Kind: "ClusterServiceVersion", + }, + ObjectMeta: metav1.ObjectMeta{Name: csvName}, + Spec: v1alpha1.ClusterServiceVersionSpec{ + WebhookDefinitions: []v1alpha1.WebhookDescription{ + { + Type: v1alpha1.ConversionWebhook, + GenerateName: "test-webhook", + ConversionCRDs: []string{crdName}, + }, + }, + }, + } + b, err := json.Marshal(csv) + require.NoError(t, err) + return string(b) +} + +// buildBuilder creates a builder wired with a fake apiextensions client that has +// currentCRD pre-loaded, and an InstallPlan whose Status.Plan contains csvManifest. +func buildBuilder(t *testing.T, currentCRD *apiextensionsv1.CustomResourceDefinition, newCRD *apiextensionsv1.CustomResourceDefinition, csvName, csvManifest string) (*builder, *apiextensionsfake.Clientset) { + t.Helper() + + crdFake := apiextensionsfake.NewSimpleClientset(currentCRD) + opClient := operatorclient.NewClient( + k8sfake.NewSimpleClientset(), + crdFake, + apiregistrationfake.NewSimpleClientset(), + ) + // validateExistingCRs lists resources via the dynamic client; the fake client + // panics unless every GVR it will list is registered. Register the CRD's + // group/resource for all served versions so the list returns empty (no CRs exist). + gvrToListKind := map[schema.GroupVersionResource]string{ + {Group: currentCRD.Spec.Group, Version: "v2", Resource: currentCRD.Spec.Names.Plural}: "TestList", + {Group: currentCRD.Spec.Group, Version: "v3", Resource: currentCRD.Spec.Names.Plural}: "TestList", + } + dynamicClient := fakedynamic.NewSimpleDynamicClientWithCustomListKinds(runtime.NewScheme(), gvrToListKind) + fakeLister := &operatorlisterfakes.FakeOperatorLister{} + csvFakeLister := &operatorlisterfakes.FakeClusterServiceVersionLister{} + + plan := &v1alpha1.InstallPlan{ + Status: v1alpha1.InstallPlanStatus{ + Plan: []*v1alpha1.Step{ + { + Resource: v1alpha1.StepResource{ + Kind: "ClusterServiceVersion", + Name: csvName, + Manifest: csvManifest, + }, + }, + }, + }, + } + + logger := logrus.NewEntry(logrus.New()) + recorder := record.NewFakeRecorder(10) + _ = fakeLister + + b := newBuilder(plan, csvFakeLister, opClient, dynamicClient, staticResolver{manifest: crdManifest(t, newCRD)}, logger, recorder) + return b, crdFake +} + +// stepFor builds a Step for a CRD update (status Unknown = triggers the create/update path). +func stepFor(crdName, csvName string) v1alpha1.Step { + return v1alpha1.Step{ + Status: v1alpha1.StepStatusUnknown, + Resource: v1alpha1.StepResource{ + Kind: crdKind, + Name: crdName, + Version: "v1", + }, + Resolving: csvName, + } +} + +// ── storageVersionInSpec ────────────────────────────────────────────────────── + +func TestStorageVersionInSpec_ReturnsStorageVersion(t *testing.T) { + crd := buildCRD("test.example.com", nil, + storedVersion("v2", false), + storedVersion("v3", true), + ) + got, ok := storageVersionInSpec(crd) + assert.True(t, ok) + assert.Equal(t, "v3", got) +} + +func TestStorageVersionInSpec_NoneStorageReturnsFalse(t *testing.T) { + crd := buildCRD("test.example.com", nil, + storedVersion("v2", false), + storedVersion("v3", false), + ) + got, ok := storageVersionInSpec(crd) + assert.False(t, ok) + assert.Empty(t, got) +} + +func TestStorageVersionInSpec_SingleVersion(t *testing.T) { + crd := buildCRD("test.example.com", nil, storedVersion("v1", true)) + got, ok := storageVersionInSpec(crd) + assert.True(t, ok) + assert.Equal(t, "v1", got) +} + +// ── csvDeclaresConversionWebhookForCRD ──────────────────────────────────────── + +func TestCsvDeclaresConversionWebhook_ReturnsTrueWhenDeclared(t *testing.T) { + csvName := "operator.v1.0.0" + crdName := "tests.example.com" + csvJSON := csvManifestWithConversionWebhook(t, csvName, crdName) + + plan := &v1alpha1.InstallPlan{ + Status: v1alpha1.InstallPlanStatus{ + Plan: []*v1alpha1.Step{ + {Resource: v1alpha1.StepResource{Kind: "ClusterServiceVersion", Name: csvName, Manifest: csvJSON}}, + }, + }, + } + assert.True(t, csvDeclaresConversionWebhookForCRD(plan, csvName, crdName)) +} + +func TestCsvDeclaresConversionWebhook_ReturnsFalseWhenNotDeclared(t *testing.T) { + csvName := "operator.v1.0.0" + crdName := "tests.example.com" + csvJSON := csvManifestWithConversionWebhook(t, csvName, "other.example.com") + + plan := &v1alpha1.InstallPlan{ + Status: v1alpha1.InstallPlanStatus{ + Plan: []*v1alpha1.Step{ + {Resource: v1alpha1.StepResource{Kind: "ClusterServiceVersion", Name: csvName, Manifest: csvJSON}}, + }, + }, + } + assert.False(t, csvDeclaresConversionWebhookForCRD(plan, csvName, crdName)) +} + +func TestCsvDeclaresConversionWebhook_ReturnsFalseWhenCSVAbsentFromPlan(t *testing.T) { + plan := &v1alpha1.InstallPlan{ + Status: v1alpha1.InstallPlanStatus{Plan: []*v1alpha1.Step{}}, + } + assert.False(t, csvDeclaresConversionWebhookForCRD(plan, "missing-csv", "tests.example.com")) +} + +// ── deference block in NewCRDV1Step ────────────────────────────────────────── + +func TestNewCRDV1Step_DefersStorageVersionWhenNoConversionAndCSVHasWebhook(t *testing.T) { + const crdName = "tests.example.com" + const csvName = "operator.v1.0.0" + + currentCRD := buildCRD(crdName, nil, storedVersion("v2", true)) + currentCRD.Status.StoredVersions = []string{"v2"} + + newCRD := buildCRD(crdName, nil, + storedVersion("v2", false), + storedVersion("v3", true), + ) + + b, crdFake := buildBuilder(t, currentCRD, newCRD, csvName, csvManifestWithConversionWebhook(t, csvName, crdName)) + step := stepFor(crdName, csvName) + + s, err := b.create(step) + require.NoError(t, err) + _, err = s.Status() + require.NoError(t, err) + + updated, err := crdFake.ApiextensionsV1().CustomResourceDefinitions().Get(context.TODO(), crdName, metav1.GetOptions{}) + require.NoError(t, err) + + assert.Equal(t, "v3", updated.Annotations[install.PendingStorageVersionAnnotation], + "annotation should record the deferred storage version") + for _, v := range updated.Spec.Versions { + if v.Name == "v2" { + assert.True(t, v.Storage, "v2 should remain storage=true while promotion is deferred") + } else { + assert.False(t, v.Storage, "v3 should remain storage=false while promotion is deferred") + } + } +} + +func TestNewCRDV1Step_SkipsDeferenceWhenBundleHasWebhookConversion(t *testing.T) { + const crdName = "tests.example.com" + const csvName = "operator.v1.0.0" + + currentCRD := buildCRD(crdName, nil, storedVersion("v2", true)) + currentCRD.Status.StoredVersions = []string{"v2"} + + newCRD := buildCRD(crdName, nil, + storedVersion("v2", false), + storedVersion("v3", true), + ) + path := "/" + port := int32(443) + newCRD.Spec.Conversion = &apiextensionsv1.CustomResourceConversion{ + Strategy: apiextensionsv1.WebhookConverter, + Webhook: &apiextensionsv1.WebhookConversion{ + ClientConfig: &apiextensionsv1.WebhookClientConfig{ + Service: &apiextensionsv1.ServiceReference{Path: &path, Port: &port}, + }, + }, + } + + b, crdFake := buildBuilder(t, currentCRD, newCRD, csvName, csvManifestWithConversionWebhook(t, csvName, crdName)) + step := stepFor(crdName, csvName) + + s, err := b.create(step) + require.NoError(t, err) + _, err = s.Status() + require.NoError(t, err) + + updated, err := crdFake.ApiextensionsV1().CustomResourceDefinitions().Get(context.TODO(), crdName, metav1.GetOptions{}) + require.NoError(t, err) + + assert.NotContains(t, updated.Annotations, install.PendingStorageVersionAnnotation, + "annotation should not be set when bundle already has webhook conversion") + var storageVersions []string + for _, v := range updated.Spec.Versions { + if v.Storage { + storageVersions = append(storageVersions, v.Name) + } + } + assert.Equal(t, []string{"v3"}, storageVersions, "v3 should be storage=true as written by the bundle") +} + +func TestNewCRDV1Step_SkipsDeferenceWhenStorageVersionUnchanged(t *testing.T) { + const crdName = "tests.example.com" + const csvName = "operator.v1.0.0" + + currentCRD := buildCRD(crdName, nil, storedVersion("v2", true)) + currentCRD.Status.StoredVersions = []string{"v2"} + newCRD := buildCRD(crdName, nil, storedVersion("v2", true)) + + b, crdFake := buildBuilder(t, currentCRD, newCRD, csvName, csvManifestWithConversionWebhook(t, csvName, crdName)) + step := stepFor(crdName, csvName) + + s, err := b.create(step) + require.NoError(t, err) + _, err = s.Status() + require.NoError(t, err) + + updated, err := crdFake.ApiextensionsV1().CustomResourceDefinitions().Get(context.TODO(), crdName, metav1.GetOptions{}) + require.NoError(t, err) + + assert.NotContains(t, updated.Annotations, install.PendingStorageVersionAnnotation, + "annotation should not be set when storage version does not change") +} + +func TestNewCRDV1Step_SkipsDeferenceWhenCSVHasNoConversionWebhook(t *testing.T) { + const crdName = "tests.example.com" + const csvName = "operator.v1.0.0" + + currentCRD := buildCRD(crdName, nil, storedVersion("v2", true)) + currentCRD.Status.StoredVersions = []string{"v2"} + newCRD := buildCRD(crdName, nil, + storedVersion("v2", false), + storedVersion("v3", true), + ) + + // CSV does NOT list a ConversionWebhook for this CRD + csvNoWebhook := csvManifestWithConversionWebhook(t, csvName, "other.example.com") + + b, crdFake := buildBuilder(t, currentCRD, newCRD, csvName, csvNoWebhook) + step := stepFor(crdName, csvName) + + s, err := b.create(step) + require.NoError(t, err) + _, err = s.Status() + require.NoError(t, err) + + updated, err := crdFake.ApiextensionsV1().CustomResourceDefinitions().Get(context.TODO(), crdName, metav1.GetOptions{}) + require.NoError(t, err) + + assert.NotContains(t, updated.Annotations, install.PendingStorageVersionAnnotation, + "annotation should not be set when CSV declares no conversion webhook for this CRD") + var storageVersions []string + for _, v := range updated.Spec.Versions { + if v.Storage { + storageVersions = append(storageVersions, v.Name) + } + } + assert.Equal(t, []string{"v3"}, storageVersions, "v3 should be written as storage=true unchanged") +} From 043237c52b55e037267580c0469312689d0d1960 Mon Sep 17 00:00:00 2001 From: Raimund Sacherer Date: Tue, 20 Oct 2026 17:18:40 +0200 Subject: [PATCH 3/8] fix(catalog): guard deferral against currentVer absent from new bundle spec MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review finding: when SafeStorageVersionUpgrade passes (Status.StoredVersions is empty so no stored-version check applies) but the current spec storage version was dropped from the new bundle, the deferral loop set all new versions to Storage=false — producing an invalid CRD the API server rejects with 422. Add a pre-check: only defer when currentVer is still present in the incoming bundle's spec. If it was removed, write the bundle as-is (new storage version promoted immediately without deferral). Also adds: - Log warning when csvDeclaresConversionWebhookForCRD fails to unmarshal the CSV manifest, making the silent false-return visible in operator logs. - Remove dead port variable in webhook_test.go singletonCSV helper. Co-Authored-By: Claude Sonnet 4.6 (1M context) Signed-off-by: Raimund Sacherer --- pkg/controller/install/webhook_test.go | 2 - pkg/controller/operators/catalog/step.go | 13 ++++++- .../operators/catalog/step_deference_test.go | 38 +++++++++++++++++++ 3 files changed, 50 insertions(+), 3 deletions(-) diff --git a/pkg/controller/install/webhook_test.go b/pkg/controller/install/webhook_test.go index 158515cddb..301f4520a2 100644 --- a/pkg/controller/install/webhook_test.go +++ b/pkg/controller/install/webhook_test.go @@ -26,8 +26,6 @@ const ( // singletonCSV returns a CSV that satisfies isSingletonOperator (AllNamespaces only). func singletonCSV(crdName string) *v1alpha1.ClusterServiceVersion { - port := int32(443) - _ = port return &v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ Name: testCSVName, diff --git a/pkg/controller/operators/catalog/step.go b/pkg/controller/operators/catalog/step.go index 46c37ee098..29c38c2d25 100644 --- a/pkg/controller/operators/catalog/step.go +++ b/pkg/controller/operators/catalog/step.go @@ -189,7 +189,17 @@ Validation Error: %s conversionMissing := crd.Spec.Conversion == nil || crd.Spec.Conversion.Strategy == "" || crd.Spec.Conversion.Strategy == apiextensionsv1.NoneConverter - if conversionMissing && csvDeclaresConversionWebhookForCRD(b.plan, step.Resolving, step.Resource.Name) { + // Also verify currentVer is still present in the new bundle spec. + // If it was removed, reverting storage to currentVer would produce + // a CRD with no Storage=true version (API server rejects with 422). + currentVerInNewSpec := false + for _, v := range crd.Spec.Versions { + if v.Name == currentVer { + currentVerInNewSpec = true + break + } + } + if conversionMissing && currentVerInNewSpec && csvDeclaresConversionWebhookForCRD(b.plan, step.Resolving, step.Resource.Name) { for idx := range crd.Spec.Versions { crd.Spec.Versions[idx].Storage = crd.Spec.Versions[idx].Name == currentVer } @@ -337,6 +347,7 @@ func csvDeclaresConversionWebhookForCRD(plan *v1alpha1.InstallPlan, csvName, crd } csv := &v1alpha1.ClusterServiceVersion{} if err := json.Unmarshal([]byte(step.Resource.Manifest), csv); err != nil { + logrus.Warnf("csvDeclaresConversionWebhookForCRD: failed to unmarshal CSV manifest for %q: %v", step.Resource.Name, err) return false } for _, wh := range csv.Spec.WebhookDefinitions { diff --git a/pkg/controller/operators/catalog/step_deference_test.go b/pkg/controller/operators/catalog/step_deference_test.go index cb0149d900..59bb085567 100644 --- a/pkg/controller/operators/catalog/step_deference_test.go +++ b/pkg/controller/operators/catalog/step_deference_test.go @@ -330,6 +330,44 @@ func TestNewCRDV1Step_SkipsDeferenceWhenStorageVersionUnchanged(t *testing.T) { "annotation should not be set when storage version does not change") } +// TestNewCRDV1Step_SkipsDeferenceWhenCurrentVersionDroppedFromNewSpec verifies that +// the deferral block does not revert storage markers when the current storage version +// is absent from the new bundle's spec. Reverting in that case would produce a CRD +// with no Storage=true version, which the API server rejects with 422. +func TestNewCRDV1Step_SkipsDeferenceWhenCurrentVersionDroppedFromNewSpec(t *testing.T) { + const crdName = "tests.example.com" + const csvName = "operator.v1.0.0" + + // Current cluster CRD: only v2 as storage; Status.StoredVersions is empty so + // SafeStorageVersionUpgrade won't block the removal of v2 from the new spec. + currentCRD := buildCRD(crdName, nil, storedVersion("v2", true)) + // No Status.StoredVersions — SafeStorageVersionUpgrade passes even though v2 is dropped. + + // New bundle: drops v2 entirely, promotes v3 as the sole version. + newCRD := buildCRD(crdName, nil, storedVersion("v3", true)) + + b, crdFake := buildBuilder(t, currentCRD, newCRD, csvName, csvManifestWithConversionWebhook(t, csvName, crdName)) + step := stepFor(crdName, csvName) + + s, err := b.create(step) + require.NoError(t, err) + _, err = s.Status() + require.NoError(t, err) + + updated, err := crdFake.ApiextensionsV1().CustomResourceDefinitions().Get(context.TODO(), crdName, metav1.GetOptions{}) + require.NoError(t, err) + + assert.NotContains(t, updated.Annotations, install.PendingStorageVersionAnnotation, + "annotation should not be set when currentVer is absent from the new bundle spec — reverting would produce zero storage versions") + var storageVersions []string + for _, v := range updated.Spec.Versions { + if v.Storage { + storageVersions = append(storageVersions, v.Name) + } + } + assert.Equal(t, []string{"v3"}, storageVersions, "v3 should be written as storage=true as the bundle intended") +} + func TestNewCRDV1Step_SkipsDeferenceWhenCSVHasNoConversionWebhook(t *testing.T) { const crdName = "tests.example.com" const csvName = "operator.v1.0.0" From 691afc67c817c9ceaa1627fe2c7e55745af735c4 Mon Sep 17 00:00:00 2001 From: Raimund Sacherer Date: Wed, 21 Oct 2026 09:50:47 +0200 Subject: [PATCH 4/8] fix(install): clear stale PendingStorageVersionAnnotation instead of retaining MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the pending storage version named in the annotation is absent from the CRD spec (stale or manually edited annotation), the previous behaviour logged a warning and left the annotation in place. This caused the warning to fire on every future createOrUpdateConversionWebhook call with no automated recovery path — the operator had to manually remove the annotation via kubectl. Fix: log the warning once (so the inconsistency is visible in operator logs), then delete the annotation in the same Update call that wires the webhook. The CRD's existing storage markers are untouched so webhook wiring still succeeds. Co-Authored-By: Claude Sonnet 4.6 (1M context) Signed-off-by: Raimund Sacherer --- pkg/controller/install/webhook.go | 8 +++++++- pkg/controller/install/webhook_test.go | 3 ++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/pkg/controller/install/webhook.go b/pkg/controller/install/webhook.go index 8e66ddc6f0..7a75db8422 100644 --- a/pkg/controller/install/webhook.go +++ b/pkg/controller/install/webhook.go @@ -279,7 +279,13 @@ func (i *StrategyDeploymentInstaller) createOrUpdateConversionWebhook(caPEM []by delete(crd.Annotations, PendingStorageVersionAnnotation) log.Infof("atomically promoted storage version %q on CRD %s with conversion webhook", pending, conversionCRD) } else { - log.Warnf("pending storage version %q not found in CRD %s spec; skipping promotion (annotation retained for investigation)", pending, conversionCRD) + // Log once so the inconsistency is visible, then clear the annotation. + // Retaining it would cause this warning to fire on every future + // createOrUpdateConversionWebhook call with no way to recover except + // manual intervention. crd.Spec.Versions is left untouched so webhook + // wiring still succeeds with the existing storage markers. + log.Warnf("pending storage version %q not found in CRD %s spec; skipping promotion and clearing stale annotation", pending, conversionCRD) + delete(crd.Annotations, PendingStorageVersionAnnotation) } } diff --git a/pkg/controller/install/webhook_test.go b/pkg/controller/install/webhook_test.go index 301f4520a2..a54aec5baa 100644 --- a/pkg/controller/install/webhook_test.go +++ b/pkg/controller/install/webhook_test.go @@ -187,7 +187,8 @@ func TestConversionWebhook_StaleAnnotationVersionNotInSpec(t *testing.T) { assert.False(t, v.Storage) } } - assert.Contains(t, updated.Annotations, PendingStorageVersionAnnotation, "stale annotation should be retained for investigation") + assert.NotContains(t, updated.Annotations, PendingStorageVersionAnnotation, + "stale annotation should be cleared after warning so future reconciles do not loop indefinitely") require.NotNil(t, updated.Spec.Conversion) assert.Equal(t, apiextensionsv1.WebhookConverter, updated.Spec.Conversion.Strategy) } From 74f725ae1bc445c7e9e3dcbf630a242a3ae19061 Mon Sep 17 00:00:00 2001 From: Raimund Sacherer Date: Wed, 21 Oct 2026 10:20:28 +0200 Subject: [PATCH 5/8] refactor(catalog): parse CSV manifests once per builder for conversion webhook cache csvDeclaresConversionWebhookForCRD was called once per CRD step that reached the storage-version deference check, and each call parsed the full CSV manifest from plan.Status.Plan. For a bundle with multiple CRDs owned by the same CSV, this caused repeated json.Unmarshal of the same (potentially large) manifest. Replace with a cache built once in newBuilder: buildCSVConversionWebhookCache scans plan.Status.Plan a single time, parses each CSV manifest exactly once, and records all (csvName, crdName) pairs where a ConversionWebhook is declared. The deference block now does an O(1) map lookup instead of a linear scan with an unmarshal on every call. csvDeclaresConversionWebhookForCRD is retained for its direct unit tests which cover the parsing logic independently. Co-Authored-By: Claude Sonnet 4.6 (1M context) Signed-off-by: Raimund Sacherer --- pkg/controller/operators/catalog/step.go | 49 ++++++++++++++++++++---- 1 file changed, 41 insertions(+), 8 deletions(-) diff --git a/pkg/controller/operators/catalog/step.go b/pkg/controller/operators/catalog/step.go index 29c38c2d25..564663bb47 100644 --- a/pkg/controller/operators/catalog/step.go +++ b/pkg/controller/operators/catalog/step.go @@ -50,18 +50,51 @@ type builder struct { eventRecorder record.EventRecorder annotator alongside.Annotator + + // csvConversionCache is built once from plan.Status.Plan. Key is "csvName/crdName"; + // value is true when that CSV declares a ConversionWebhook for that CRD. + // This avoids re-parsing the CSV manifest on every CRD step that reaches the + // storage-version deference check. + csvConversionCache map[string]bool } func newBuilder(plan *v1alpha1.InstallPlan, csvLister listersv1alpha1.ClusterServiceVersionLister, opclient operatorclient.ClientInterface, dynamicClient dynamic.Interface, manifestResolver ManifestResolver, logger logrus.FieldLogger, er record.EventRecorder) *builder { return &builder{ - plan: plan, - csvLister: csvLister, - opclient: opclient, - dynamicClient: dynamicClient, - manifestResolver: manifestResolver, - logger: logger, - eventRecorder: er, + plan: plan, + csvLister: csvLister, + opclient: opclient, + dynamicClient: dynamicClient, + manifestResolver: manifestResolver, + logger: logger, + eventRecorder: er, + csvConversionCache: buildCSVConversionWebhookCache(plan), + } +} + +// buildCSVConversionWebhookCache scans plan.Status.Plan once and returns a map whose +// keys are "csvName/crdName" for every (CSV, CRD) pair where the CSV declares a +// ConversionWebhook for that CRD. All CSV manifests are parsed exactly once. +func buildCSVConversionWebhookCache(plan *v1alpha1.InstallPlan) map[string]bool { + cache := make(map[string]bool) + for _, step := range plan.Status.Plan { + if step.Resource.Kind != "ClusterServiceVersion" { + continue + } + csv := &v1alpha1.ClusterServiceVersion{} + if err := json.Unmarshal([]byte(step.Resource.Manifest), csv); err != nil { + logrus.Warnf("buildCSVConversionWebhookCache: failed to parse CSV manifest for %q: %v", step.Resource.Name, err) + continue + } + for _, wh := range csv.Spec.WebhookDefinitions { + if wh.Type != v1alpha1.ConversionWebhook { + continue + } + for _, crdName := range wh.ConversionCRDs { + cache[step.Resource.Name+"/"+crdName] = true + } + } } + return cache } type notSupportedStepperErr struct { @@ -199,7 +232,7 @@ Validation Error: %s break } } - if conversionMissing && currentVerInNewSpec && csvDeclaresConversionWebhookForCRD(b.plan, step.Resolving, step.Resource.Name) { + if conversionMissing && currentVerInNewSpec && b.csvConversionCache[step.Resolving+"/"+step.Resource.Name] { for idx := range crd.Spec.Versions { crd.Spec.Versions[idx].Storage = crd.Spec.Versions[idx].Name == currentVer } From 3fde9dffb89633b3e3c20452435a115bda15548b Mon Sep 17 00:00:00 2001 From: Raimund Sacherer Date: Wed, 21 Oct 2026 12:45:01 +0200 Subject: [PATCH 6/8] docs: update code comments for correctness and style - step.go deference block: document all three guards explicitly (conversion missing, current version present in new spec, CSV declares webhook). The currentVerInNewSpec guard added in a prior commit was not reflected in the comment. - step.go csvConversionCache field: replace semicolon with period. - webhook.go PendingStorageVersionAnnotation: replace semicolon with period. Co-Authored-By: Claude Sonnet 4.6 (1M context) Signed-off-by: Raimund Sacherer --- pkg/controller/install/webhook.go | 2 +- pkg/controller/operators/catalog/step.go | 15 ++++++++++----- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/pkg/controller/install/webhook.go b/pkg/controller/install/webhook.go index 7a75db8422..91bc83fd3e 100644 --- a/pkg/controller/install/webhook.go +++ b/pkg/controller/install/webhook.go @@ -309,7 +309,7 @@ const WebhookHashKey = "olm.webhook-description-hash" // storage-version promotion. The value is the version name to promote. // createOrUpdateConversionWebhook reads it and promotes storage atomically with // webhook activation, then deletes it. If the operator is deleted before -// olm-operator processes it the annotation remains on the CRD; a later reinstall +// olm-operator processes it, the annotation remains on the CRD. A later reinstall // completes the promotion. const PendingStorageVersionAnnotation = "olm.operatorframework.io/pending-storage-version" diff --git a/pkg/controller/operators/catalog/step.go b/pkg/controller/operators/catalog/step.go index 564663bb47..7155268ec9 100644 --- a/pkg/controller/operators/catalog/step.go +++ b/pkg/controller/operators/catalog/step.go @@ -51,8 +51,8 @@ type builder struct { annotator alongside.Annotator - // csvConversionCache is built once from plan.Status.Plan. Key is "csvName/crdName"; - // value is true when that CSV declares a ConversionWebhook for that CRD. + // csvConversionCache is built once from plan.Status.Plan. The key is "csvName/crdName". + // The value is true when that CSV declares a ConversionWebhook for that CRD. // This avoids re-parsing the CSV manifest on every CRD step that reaches the // storage-version deference check. csvConversionCache map[string]bool @@ -214,9 +214,14 @@ Validation Error: %s // but carries no conversion webhook. Atomically promoting storage with // the webhook (see createOrUpdateConversionWebhook) eliminates the window // where a new storage version is active without field conversion. - // Guard: only defer when the CSV that owns this CRD declares a - // ConversionWebhook for it; operators that legitimately promote storage - // without a webhook would otherwise be stuck at the old version forever. + // Three guards must all be true before deferral fires: + // 1. The bundle carries no conversion webhook (conversionMissing). + // 2. The current storage version is still present in the new bundle spec. + // If it was removed, reverting to it would produce a CRD with no + // Storage=true version, which the API server rejects with 422. + // 3. The CSV that owns this CRD declares a ConversionWebhook for it. + // Without this guard, operators that legitimately promote storage + // without a webhook would be stuck at the old version forever. if newVer, ok := storageVersionInSpec(crd); ok { if currentVer, hasCurrent := storageVersionInSpec(currentCRD); hasCurrent && newVer != currentVer { conversionMissing := crd.Spec.Conversion == nil || From 765d72cae887c7abf52efc4d05010ba8f0240990 Mon Sep 17 00:00:00 2001 From: Raimund Sacherer Date: Wed, 21 Oct 2026 13:18:07 +0200 Subject: [PATCH 7/8] test: fix import alias and gofmt issues flagged by golangci-lint The project's importas linter requires the operator-framework/api v1alpha1 package to be aliased as operatorsv1alpha1. Update both new test files. Also apply gofmt formatting to step_deference_test.go. Co-Authored-By: Claude Sonnet 4.6 (1M context) Signed-off-by: Raimund Sacherer --- pkg/controller/install/webhook_test.go | 30 +++++------ .../operators/catalog/step_deference_test.go | 50 +++++++++---------- 2 files changed, 40 insertions(+), 40 deletions(-) diff --git a/pkg/controller/install/webhook_test.go b/pkg/controller/install/webhook_test.go index a54aec5baa..0f4692c68f 100644 --- a/pkg/controller/install/webhook_test.go +++ b/pkg/controller/install/webhook_test.go @@ -12,7 +12,7 @@ import ( k8sfake "k8s.io/client-go/kubernetes/fake" apiregistrationfake "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/fake" - v1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1" + operatorsv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1" "github.com/operator-framework/operator-lifecycle-manager/pkg/api/wrappers" "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient" "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorlister/operatorlisterfakes" @@ -25,21 +25,21 @@ const ( ) // singletonCSV returns a CSV that satisfies isSingletonOperator (AllNamespaces only). -func singletonCSV(crdName string) *v1alpha1.ClusterServiceVersion { - return &v1alpha1.ClusterServiceVersion{ +func singletonCSV(crdName string) *operatorsv1alpha1.ClusterServiceVersion { + return &operatorsv1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ Name: testCSVName, Namespace: testNamespace, }, - Spec: v1alpha1.ClusterServiceVersionSpec{ - InstallModes: []v1alpha1.InstallMode{ - {Type: v1alpha1.InstallModeTypeAllNamespaces, Supported: true}, - {Type: v1alpha1.InstallModeTypeSingleNamespace, Supported: false}, - {Type: v1alpha1.InstallModeTypeMultiNamespace, Supported: false}, - {Type: v1alpha1.InstallModeTypeOwnNamespace, Supported: false}, + Spec: operatorsv1alpha1.ClusterServiceVersionSpec{ + InstallModes: []operatorsv1alpha1.InstallMode{ + {Type: operatorsv1alpha1.InstallModeTypeAllNamespaces, Supported: true}, + {Type: operatorsv1alpha1.InstallModeTypeSingleNamespace, Supported: false}, + {Type: operatorsv1alpha1.InstallModeTypeMultiNamespace, Supported: false}, + {Type: operatorsv1alpha1.InstallModeTypeOwnNamespace, Supported: false}, }, - CustomResourceDefinitions: v1alpha1.CustomResourceDefinitions{ - Owned: []v1alpha1.CRDDescription{ + CustomResourceDefinitions: operatorsv1alpha1.CustomResourceDefinitions{ + Owned: []operatorsv1alpha1.CRDDescription{ {Name: crdName, Version: "v2", Kind: "Test"}, }, }, @@ -48,10 +48,10 @@ func singletonCSV(crdName string) *v1alpha1.ClusterServiceVersion { } // conversionDesc returns a WebhookDescription of type ConversionWebhook for crdName. -func conversionDesc(crdName string) v1alpha1.WebhookDescription { +func conversionDesc(crdName string) operatorsv1alpha1.WebhookDescription { port := int32(443) - return v1alpha1.WebhookDescription{ - Type: v1alpha1.ConversionWebhook, + return operatorsv1alpha1.WebhookDescription{ + Type: operatorsv1alpha1.ConversionWebhook, GenerateName: "test-webhook", ConversionCRDs: []string{crdName}, AdmissionReviewVersions: []string{"v1"}, @@ -164,7 +164,7 @@ func TestConversionWebhook_NoAnnotation(t *testing.T) { // TestConversionWebhook_StaleAnnotationVersionNotInSpec verifies that when the // PendingStorageVersionAnnotation names a version that does not exist in the CRD spec, -// storage markers are left untouched, the annotation is retained, and the conversion +// storage markers are left untouched, the annotation is cleared, and the conversion // webhook is still set successfully. func TestConversionWebhook_StaleAnnotationVersionNotInSpec(t *testing.T) { crd := crdWithVersions( diff --git a/pkg/controller/operators/catalog/step_deference_test.go b/pkg/controller/operators/catalog/step_deference_test.go index 59bb085567..ff088638e4 100644 --- a/pkg/controller/operators/catalog/step_deference_test.go +++ b/pkg/controller/operators/catalog/step_deference_test.go @@ -12,13 +12,13 @@ import ( apiextensionsfake "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/fake" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" fakedynamic "k8s.io/client-go/dynamic/fake" k8sfake "k8s.io/client-go/kubernetes/fake" - "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/client-go/tools/record" apiregistrationfake "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/fake" - v1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1" + operatorsv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1" "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/install" "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient" "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorlister/operatorlisterfakes" @@ -27,7 +27,7 @@ import ( // staticResolver implements ManifestResolver and always returns the same manifest. type staticResolver struct{ manifest string } -func (r staticResolver) ManifestForStep(_ *v1alpha1.Step) (string, error) { +func (r staticResolver) ManifestForStep(_ *operatorsv1alpha1.Step) (string, error) { return r.manifest, nil } @@ -81,16 +81,16 @@ func crdManifest(t *testing.T, crd *apiextensionsv1.CustomResourceDefinition) st // csvManifestWithConversionWebhook JSON-encodes a CSV that declares a ConversionWebhook for crdName. func csvManifestWithConversionWebhook(t *testing.T, csvName, crdName string) string { t.Helper() - csv := &v1alpha1.ClusterServiceVersion{ + csv := &operatorsv1alpha1.ClusterServiceVersion{ TypeMeta: metav1.TypeMeta{ APIVersion: "operators.coreos.com/v1alpha1", Kind: "ClusterServiceVersion", }, ObjectMeta: metav1.ObjectMeta{Name: csvName}, - Spec: v1alpha1.ClusterServiceVersionSpec{ - WebhookDefinitions: []v1alpha1.WebhookDescription{ + Spec: operatorsv1alpha1.ClusterServiceVersionSpec{ + WebhookDefinitions: []operatorsv1alpha1.WebhookDescription{ { - Type: v1alpha1.ConversionWebhook, + Type: operatorsv1alpha1.ConversionWebhook, GenerateName: "test-webhook", ConversionCRDs: []string{crdName}, }, @@ -124,11 +124,11 @@ func buildBuilder(t *testing.T, currentCRD *apiextensionsv1.CustomResourceDefini fakeLister := &operatorlisterfakes.FakeOperatorLister{} csvFakeLister := &operatorlisterfakes.FakeClusterServiceVersionLister{} - plan := &v1alpha1.InstallPlan{ - Status: v1alpha1.InstallPlanStatus{ - Plan: []*v1alpha1.Step{ + plan := &operatorsv1alpha1.InstallPlan{ + Status: operatorsv1alpha1.InstallPlanStatus{ + Plan: []*operatorsv1alpha1.Step{ { - Resource: v1alpha1.StepResource{ + Resource: operatorsv1alpha1.StepResource{ Kind: "ClusterServiceVersion", Name: csvName, Manifest: csvManifest, @@ -147,10 +147,10 @@ func buildBuilder(t *testing.T, currentCRD *apiextensionsv1.CustomResourceDefini } // stepFor builds a Step for a CRD update (status Unknown = triggers the create/update path). -func stepFor(crdName, csvName string) v1alpha1.Step { - return v1alpha1.Step{ - Status: v1alpha1.StepStatusUnknown, - Resource: v1alpha1.StepResource{ +func stepFor(crdName, csvName string) operatorsv1alpha1.Step { + return operatorsv1alpha1.Step{ + Status: operatorsv1alpha1.StepStatusUnknown, + Resource: operatorsv1alpha1.StepResource{ Kind: crdKind, Name: crdName, Version: "v1", @@ -195,10 +195,10 @@ func TestCsvDeclaresConversionWebhook_ReturnsTrueWhenDeclared(t *testing.T) { crdName := "tests.example.com" csvJSON := csvManifestWithConversionWebhook(t, csvName, crdName) - plan := &v1alpha1.InstallPlan{ - Status: v1alpha1.InstallPlanStatus{ - Plan: []*v1alpha1.Step{ - {Resource: v1alpha1.StepResource{Kind: "ClusterServiceVersion", Name: csvName, Manifest: csvJSON}}, + plan := &operatorsv1alpha1.InstallPlan{ + Status: operatorsv1alpha1.InstallPlanStatus{ + Plan: []*operatorsv1alpha1.Step{ + {Resource: operatorsv1alpha1.StepResource{Kind: "ClusterServiceVersion", Name: csvName, Manifest: csvJSON}}, }, }, } @@ -210,10 +210,10 @@ func TestCsvDeclaresConversionWebhook_ReturnsFalseWhenNotDeclared(t *testing.T) crdName := "tests.example.com" csvJSON := csvManifestWithConversionWebhook(t, csvName, "other.example.com") - plan := &v1alpha1.InstallPlan{ - Status: v1alpha1.InstallPlanStatus{ - Plan: []*v1alpha1.Step{ - {Resource: v1alpha1.StepResource{Kind: "ClusterServiceVersion", Name: csvName, Manifest: csvJSON}}, + plan := &operatorsv1alpha1.InstallPlan{ + Status: operatorsv1alpha1.InstallPlanStatus{ + Plan: []*operatorsv1alpha1.Step{ + {Resource: operatorsv1alpha1.StepResource{Kind: "ClusterServiceVersion", Name: csvName, Manifest: csvJSON}}, }, }, } @@ -221,8 +221,8 @@ func TestCsvDeclaresConversionWebhook_ReturnsFalseWhenNotDeclared(t *testing.T) } func TestCsvDeclaresConversionWebhook_ReturnsFalseWhenCSVAbsentFromPlan(t *testing.T) { - plan := &v1alpha1.InstallPlan{ - Status: v1alpha1.InstallPlanStatus{Plan: []*v1alpha1.Step{}}, + plan := &operatorsv1alpha1.InstallPlan{ + Status: operatorsv1alpha1.InstallPlanStatus{Plan: []*operatorsv1alpha1.Step{}}, } assert.False(t, csvDeclaresConversionWebhookForCRD(plan, "missing-csv", "tests.example.com")) } From 7c05b03f5ea89b19f383cabe95204924d1009392 Mon Sep 17 00:00:00 2001 From: Raimund Sacherer Date: Wed, 21 Oct 2026 16:01:01 +0200 Subject: [PATCH 8/8] fix(catalog): strip reserved annotation from bundle CRDs after unmarshal UnmarshalV1 preserves all annotations from the bundle manifest. A bundle that ships PendingStorageVersionAnnotation in its CRD manifest would cause createOrUpdateConversionWebhook to read it as a catalog-generated hand-off and promote the wrong storage version, recreating the data-loss window this fix was designed to close. Strip the annotation immediately after UnmarshalV1, before any branch, so only the deference block can set it. Reported by CodeRabbit on PR #3945. Signed-off-by: Raimund Sacherer Co-Authored-By: Claude Sonnet 4.6 (1M context) --- pkg/controller/operators/catalog/step.go | 6 ++++ .../operators/catalog/step_deference_test.go | 35 +++++++++++++++++++ 2 files changed, 41 insertions(+) diff --git a/pkg/controller/operators/catalog/step.go b/pkg/controller/operators/catalog/step.go index 7155268ec9..7d910bfc51 100644 --- a/pkg/controller/operators/catalog/step.go +++ b/pkg/controller/operators/catalog/step.go @@ -166,6 +166,12 @@ func (b *builder) NewCRDV1Step(client apiextensionsv1client.ApiextensionsV1Inter return v1alpha1.StepStatusUnknown, err } + // Strip the reserved hand-off annotation before any further processing. + // UnmarshalV1 preserves all bundle annotations; a bundle that ships this + // annotation would otherwise reach createOrUpdateConversionWebhook and + // trigger an unintended storage-version promotion. + delete(crd.Annotations, install.PendingStorageVersionAnnotation) + setInstalledAlongsideAnnotation(b.annotator, crd, b.plan.GetNamespace(), step.Resolving, b.csvLister, crd) if crd.Labels == nil { crd.Labels = map[string]string{} diff --git a/pkg/controller/operators/catalog/step_deference_test.go b/pkg/controller/operators/catalog/step_deference_test.go index ff088638e4..13b5628d04 100644 --- a/pkg/controller/operators/catalog/step_deference_test.go +++ b/pkg/controller/operators/catalog/step_deference_test.go @@ -159,6 +159,41 @@ func stepFor(crdName, csvName string) operatorsv1alpha1.Step { } } +// TestNewCRDV1Step_StripsBundleSuppliedPendingAnnotation verifies that when a bundle +// CRD arrives with PendingStorageVersionAnnotation already set in its manifest, the +// annotation is removed before the CRD is written to the cluster. Only the deference +// block may set this annotation; a bundle-supplied value must never reach the promotion +// block in createOrUpdateConversionWebhook. +func TestNewCRDV1Step_StripsBundleSuppliedPendingAnnotation(t *testing.T) { + const crdName = "tests.example.com" + const csvName = "operator.v1.0.0" + + currentCRD := buildCRD(crdName, nil, storedVersion("v2", true)) + currentCRD.Status.StoredVersions = []string{"v2"} + + // New bundle carries the same storage version (no promotion) but has the + // reserved annotation pre-set in its manifest — simulating a malicious or + // accidentally-annotated bundle. + newCRD := buildCRD(crdName, + map[string]string{install.PendingStorageVersionAnnotation: "v2"}, + storedVersion("v2", true), + ) + + b, crdFake := buildBuilder(t, currentCRD, newCRD, csvName, csvManifestWithConversionWebhook(t, csvName, crdName)) + step := stepFor(crdName, csvName) + + s, err := b.create(step) + require.NoError(t, err) + _, err = s.Status() + require.NoError(t, err) + + updated, err := crdFake.ApiextensionsV1().CustomResourceDefinitions().Get(context.TODO(), crdName, metav1.GetOptions{}) + require.NoError(t, err) + + assert.NotContains(t, updated.Annotations, install.PendingStorageVersionAnnotation, + "bundle-supplied PendingStorageVersionAnnotation must be stripped before writing to the cluster") +} + // ── storageVersionInSpec ────────────────────────────────────────────────────── func TestStorageVersionInSpec_ReturnsStorageVersion(t *testing.T) {