diff --git a/pkg/controller/install/webhook.go b/pkg/controller/install/webhook.go index d3dee19fb5..91bc83fd3e 100644 --- a/pkg/controller/install/webhook.go +++ b/pkg/controller/install/webhook.go @@ -261,6 +261,34 @@ 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 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) + } + } + // 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 +305,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..0f4692c68f --- /dev/null +++ b/pkg/controller/install/webhook_test.go @@ -0,0 +1,215 @@ +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" + + 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" +) + +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) *operatorsv1alpha1.ClusterServiceVersion { + return &operatorsv1alpha1.ClusterServiceVersion{ + ObjectMeta: metav1.ObjectMeta{ + Name: testCSVName, + Namespace: testNamespace, + }, + 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: operatorsv1alpha1.CustomResourceDefinitions{ + Owned: []operatorsv1alpha1.CRDDescription{ + {Name: crdName, Version: "v2", Kind: "Test"}, + }, + }, + }, + } +} + +// conversionDesc returns a WebhookDescription of type ConversionWebhook for crdName. +func conversionDesc(crdName string) operatorsv1alpha1.WebhookDescription { + port := int32(443) + return operatorsv1alpha1.WebhookDescription{ + Type: operatorsv1alpha1.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 cleared, 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.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) +} + +// 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) +} diff --git a/pkg/controller/operators/catalog/step.go b/pkg/controller/operators/catalog/step.go index e2afbde78c..7d910bfc51 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" @@ -49,18 +50,51 @@ type builder struct { eventRecorder record.EventRecorder annotator alongside.Annotator + + // 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 } 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 { @@ -132,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{} @@ -176,6 +216,47 @@ 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. + // 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 || + crd.Spec.Conversion.Strategy == "" || + crd.Spec.Conversion.Strategy == apiextensionsv1.NoneConverter + // 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 && b.csvConversionCache[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 +370,44 @@ 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 { + logrus.Warnf("csvDeclaresConversionWebhookForCRD: failed to unmarshal CSV manifest for %q: %v", step.Resource.Name, err) + 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..13b5628d04 --- /dev/null +++ b/pkg/controller/operators/catalog/step_deference_test.go @@ -0,0 +1,440 @@ +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" + "k8s.io/apimachinery/pkg/runtime/schema" + fakedynamic "k8s.io/client-go/dynamic/fake" + k8sfake "k8s.io/client-go/kubernetes/fake" + "k8s.io/client-go/tools/record" + apiregistrationfake "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/fake" + + 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" +) + +// staticResolver implements ManifestResolver and always returns the same manifest. +type staticResolver struct{ manifest string } + +func (r staticResolver) ManifestForStep(_ *operatorsv1alpha1.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 := &operatorsv1alpha1.ClusterServiceVersion{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "operators.coreos.com/v1alpha1", + Kind: "ClusterServiceVersion", + }, + ObjectMeta: metav1.ObjectMeta{Name: csvName}, + Spec: operatorsv1alpha1.ClusterServiceVersionSpec{ + WebhookDefinitions: []operatorsv1alpha1.WebhookDescription{ + { + Type: operatorsv1alpha1.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 := &operatorsv1alpha1.InstallPlan{ + Status: operatorsv1alpha1.InstallPlanStatus{ + Plan: []*operatorsv1alpha1.Step{ + { + Resource: operatorsv1alpha1.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) operatorsv1alpha1.Step { + return operatorsv1alpha1.Step{ + Status: operatorsv1alpha1.StepStatusUnknown, + Resource: operatorsv1alpha1.StepResource{ + Kind: crdKind, + Name: crdName, + Version: "v1", + }, + Resolving: csvName, + } +} + +// 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) { + 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 := &operatorsv1alpha1.InstallPlan{ + Status: operatorsv1alpha1.InstallPlanStatus{ + Plan: []*operatorsv1alpha1.Step{ + {Resource: operatorsv1alpha1.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 := &operatorsv1alpha1.InstallPlan{ + Status: operatorsv1alpha1.InstallPlanStatus{ + Plan: []*operatorsv1alpha1.Step{ + {Resource: operatorsv1alpha1.StepResource{Kind: "ClusterServiceVersion", Name: csvName, Manifest: csvJSON}}, + }, + }, + } + assert.False(t, csvDeclaresConversionWebhookForCRD(plan, csvName, crdName)) +} + +func TestCsvDeclaresConversionWebhook_ReturnsFalseWhenCSVAbsentFromPlan(t *testing.T) { + plan := &operatorsv1alpha1.InstallPlan{ + Status: operatorsv1alpha1.InstallPlanStatus{Plan: []*operatorsv1alpha1.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") +} + +// 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" + + 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") +}