From dda73ff52a34ce07135e57f1c5ed1b4181ac1b4b Mon Sep 17 00:00:00 2001 From: "Per G. da Silva" Date: Thu, 22 Oct 2026 18:57:03 +0200 Subject: [PATCH 01/13] =?UTF-8?q?=E2=9C=A8=20make=20packed=20Secret=20name?= =?UTF-8?q?s=20revision-independent?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Change SecretPacker to use OwnerName instead of RevisionName for Secret name prefixes and labels. This makes Secret names content-addressed and independent of any specific revision, enabling Secrets to be created before a revision exists. - secretNameFromData now prefixes with p.OwnerName instead of p.RevisionName - newSecret drops labels.RevisionNameKey, keeping only labels.OwnerNameKey - RevisionName field retained for boxcutter.go compatibility - Added TestSecretPacker_NameIsRevisionIndependent to verify behavior - Updated existing test assertions to match new owner-based naming Co-Authored-By: Claude Opus 4.8 Signed-off-by: Per G. da Silva --- .../applier/secretpacker.go | 7 ++- .../applier/secretpacker_test.go | 45 ++++++++++++++++++- 2 files changed, 46 insertions(+), 6 deletions(-) diff --git a/internal/operator-controller/applier/secretpacker.go b/internal/operator-controller/applier/secretpacker.go index 4eaf6bc68a..65ad338850 100644 --- a/internal/operator-controller/applier/secretpacker.go +++ b/internal/operator-controller/applier/secretpacker.go @@ -138,8 +138,7 @@ func (p *SecretPacker) newSecret(data map[string][]byte) corev1.Secret { Name: p.secretNameFromData(data), Namespace: p.SystemNamespace, Labels: map[string]string{ - labels.RevisionNameKey: p.RevisionName, - labels.OwnerNameKey: p.OwnerName, + labels.OwnerNameKey: p.OwnerName, }, }, Immutable: ptr.To(true), @@ -149,7 +148,7 @@ func (p *SecretPacker) newSecret(data map[string][]byte) corev1.Secret { } // secretNameFromData computes a content-addressable Secret name from the -// data entries. The name is "-" where hash is the +// data entries. The name is "-" where hash is the // first 16 hex characters of the SHA-256 digest of the sorted // concatenated keys and values. func (p *SecretPacker) secretNameFromData(data map[string][]byte) string { @@ -164,7 +163,7 @@ func (p *SecretPacker) secretNameFromData(data map[string][]byte) string { h.Write([]byte(k)) h.Write(data[k]) } - return fmt.Sprintf("%s-%x", p.RevisionName, h.Sum(nil)[:8]) + return fmt.Sprintf("%s-%x", p.OwnerName, h.Sum(nil)[:8]) } // contentHash returns a base64url-encoded (no padding) SHA-256 hash of data. diff --git a/internal/operator-controller/applier/secretpacker_test.go b/internal/operator-controller/applier/secretpacker_test.go index e80ea3e36a..755bc853aa 100644 --- a/internal/operator-controller/applier/secretpacker_test.go +++ b/internal/operator-controller/applier/secretpacker_test.go @@ -43,11 +43,10 @@ func TestSecretPacker_Pack(t *testing.T) { require.NoError(t, err) require.Len(t, result.Secrets, 1) - assert.True(t, strings.HasPrefix(result.Secrets[0].Name, "my-ext-3-"), "Secret name should be content-addressable with revision prefix") + assert.True(t, strings.HasPrefix(result.Secrets[0].Name, "my-ext-"), "Secret name should be content-addressable with owner prefix") assert.Equal(t, "olmv1-system", result.Secrets[0].Namespace) assert.True(t, *result.Secrets[0].Immutable) assert.Equal(t, labels.SecretTypeObjectData, result.Secrets[0].Type) - assert.Equal(t, "my-ext-3", result.Secrets[0].Labels[labels.RevisionNameKey]) assert.Equal(t, "my-ext", result.Secrets[0].Labels[labels.OwnerNameKey]) ref, ok := result.Refs[[2]int{0, 0}] @@ -214,6 +213,48 @@ func TestSecretPacker_Pack(t *testing.T) { }) } +func TestSecretPacker_NameIsRevisionIndependent(t *testing.T) { + phases := []ocv1.ClusterObjectSetPhase{{ + Name: "deploy", + Objects: []ocv1.ClusterObjectSetObject{ + {Object: testConfigMap("test-cm", "default")}, + }, + }} + + // Pack with first packer (OwnerName: "ext", RevisionName: "ext-1") + packer1 := SecretPacker{ + OwnerName: "ext", + RevisionName: "ext-1", + SystemNamespace: "olmv1-system", + } + result1, err := packer1.Pack(phases) + require.NoError(t, err) + require.Len(t, result1.Secrets, 1) + + // Pack with second packer (same OwnerName, different RevisionName) + packer2 := SecretPacker{ + OwnerName: "ext", + RevisionName: "ext-2", + SystemNamespace: "olmv1-system", + } + result2, err := packer2.Pack(phases) + require.NoError(t, err) + require.Len(t, result2.Secrets, 1) + + // Assert both Secret names are identical and begin with "ext-" + assert.Equal(t, result1.Secrets[0].Name, result2.Secrets[0].Name, + "Secret names should be identical regardless of RevisionName") + assert.True(t, strings.HasPrefix(result1.Secrets[0].Name, "ext-"), + "Secret name should be prefixed with OwnerName") + + // Assert labels have OwnerNameKey but no RevisionNameKey + secret := result1.Secrets[0] + assert.Equal(t, "ext", secret.Labels[labels.OwnerNameKey], + "Secret should have OwnerNameKey label") + assert.NotContains(t, secret.Labels, labels.RevisionNameKey, + "Secret should not have RevisionNameKey label") +} + func testConfigMap(name, namespace string) unstructured.Unstructured { obj := map[string]interface{}{ "apiVersion": "v1", From a24f86e053f0be091c7c42d56b5f08d7f4fc700a Mon Sep 17 00:00:00 2001 From: "Per G. da Silva" Date: Thu, 22 Oct 2026 19:03:02 +0200 Subject: [PATCH 02/13] =?UTF-8?q?=E2=9C=A8=20propagate=20bundle=20annotati?= =?UTF-8?q?ons=20from=20ClusterObjectDeployment=20to=20its=20revisions?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The COD controller now copies bundle-metadata annotations (package name, bundle name, bundle version, bundle release, and bundle reference) from the ClusterObjectDeployment onto each ClusterObjectSet it creates. This ensures the ClusterExtension status reader—which reads these annotations off the COS—continues working during the refactor to make operator-controller write CODs instead of managing COSes directly. Only the known bundle-metadata annotation keys are copied; non-bundle annotations on the COD are not propagated to revisions. Co-Authored-By: Claude Opus 4.8 Signed-off-by: Per G. da Silva --- .../clusterobjectdeployment_controller.go | 26 +++++++++++++++- ...clusterobjectdeployment_controller_test.go | 31 +++++++++++++++++++ 2 files changed, 56 insertions(+), 1 deletion(-) diff --git a/internal/object-controller/controllers/clusterobjectdeployment_controller.go b/internal/object-controller/controllers/clusterobjectdeployment_controller.go index b5da2ac791..9f8b3e8299 100644 --- a/internal/object-controller/controllers/clusterobjectdeployment_controller.go +++ b/internal/object-controller/controllers/clusterobjectdeployment_controller.go @@ -38,6 +38,16 @@ import ( // defensively for callers (e.g. tests) that bypass API-server defaulting. const defaultRevisionHistoryLimit = 5 +// bundleAnnotationKeys lists the bundle-metadata annotation keys that are +// propagated from a ClusterObjectDeployment onto each ClusterObjectSet it creates. +var bundleAnnotationKeys = []string{ + labels.PackageNameKey, + labels.BundleNameKey, + labels.BundleVersionKey, + labels.BundleReleaseKey, + labels.BundleReferenceKey, +} + // ClusterObjectDeploymentReconciler reconciles ClusterObjectDeployment objects by // managing their owned ClusterObjectSet revisions. type ClusterObjectDeploymentReconciler struct { @@ -439,11 +449,25 @@ func buildClusterObjectSet(cod *ocv1.ClusterObjectDeployment, revision int64, ha } revLabels[labels.TemplateHashKey] = hash + revAnnotations := maps.Clone(cod.Spec.Template.Metadata.Annotations) + if revAnnotations == nil { + revAnnotations = map[string]string{} + } + // Also copy bundle-metadata annotations from the deployment's own annotations so the + // ClusterExtension status reader (which reads these annotations off the COS) keeps + // working during the refactor to make operator-controller write CODs instead of + // managing COSes directly. + for _, key := range bundleAnnotationKeys { + if value, ok := cod.Annotations[key]; ok { + revAnnotations[key] = value + } + } + return &ocv1.ClusterObjectSet{ ObjectMeta: metav1.ObjectMeta{ Name: fmt.Sprintf("%s-%d", cod.Name, revision), Labels: revLabels, - Annotations: maps.Clone(cod.Spec.Template.Metadata.Annotations), + Annotations: revAnnotations, }, Spec: ocv1.ClusterObjectSetSpec{ Group: cod.Name, diff --git a/internal/object-controller/controllers/clusterobjectdeployment_controller_test.go b/internal/object-controller/controllers/clusterobjectdeployment_controller_test.go index a80fd2758f..737bcbfcd9 100644 --- a/internal/object-controller/controllers/clusterobjectdeployment_controller_test.go +++ b/internal/object-controller/controllers/clusterobjectdeployment_controller_test.go @@ -831,3 +831,34 @@ func Test_ClusterObjectDeploymentReconciler_RevisionBounds(t *testing.T) { }) } } + +func Test_ClusterObjectDeploymentReconciler_PropagatesBundleAnnotations(t *testing.T) { + cod := codFixture() + cod.Annotations = map[string]string{ + labels.PackageNameKey: "test-package", + labels.BundleNameKey: "test-bundle", + labels.BundleVersionKey: "1.0.0+2", + labels.BundleReferenceKey: "registry.example.com/bundle@sha256:abcdef", + labels.BundleReleaseKey: "2", + "example.com/non-bundle": "should-not-be-copied", + } + + c := newCODClient(t, cod) + + _, err := reconcileCOD(t, c) + require.NoError(t, err) + + cos, ok := getCOS(t, c, clusterObjectDeploymentName+"-1") + require.True(t, ok, "revision 1 should be created") + + // Bundle-metadata annotations should be copied + require.Equal(t, "test-package", cos.Annotations[labels.PackageNameKey], "PackageNameKey should be copied") + require.Equal(t, "test-bundle", cos.Annotations[labels.BundleNameKey], "BundleNameKey should be copied") + require.Equal(t, "1.0.0+2", cos.Annotations[labels.BundleVersionKey], "BundleVersionKey should be copied") + require.Equal(t, "registry.example.com/bundle@sha256:abcdef", cos.Annotations[labels.BundleReferenceKey], "BundleReferenceKey should be copied") + require.Equal(t, "2", cos.Annotations[labels.BundleReleaseKey], "BundleReleaseKey should be copied") + + // Non-bundle annotation should NOT be copied + _, exists := cos.Annotations["example.com/non-bundle"] + require.False(t, exists, "non-bundle annotations must not be copied from COD to COS") +} From ce92e54c0cbf1eae0254278de523ea971bd4fce5 Mon Sep 17 00:00:00 2001 From: "Per G. da Silva" Date: Thu, 22 Oct 2026 19:08:21 +0200 Subject: [PATCH 03/13] =?UTF-8?q?=E2=9C=A8=20add=20ClusterObjectDeployment?= =?UTF-8?q?=20applyconfiguration=20builder?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Introduce buildClusterObjectDeployment, a pure helper function that constructs a COD applyconfiguration from rendered content. This mirrors the structure of the existing buildClusterObjectSet but produces a COD with a template instead of directly setting COS-specific fields. The function: - Sets metadata (name, annotations, owner labels) - Configures spec.template.spec with: - PhaseSort-ordered phases - Default progression probes - Collision protection strategy - Optional progress deadline when > 0 This builder will be used in later tasks to produce the COD that the applier will server-side-apply, as part of the refactor making operator-controller write a COD instead of managing ClusterObjectSets directly. Co-Authored-By: Claude Opus 4.8 Signed-off-by: Per G. da Silva --- .../applier/clusterobjectdeployment.go | 46 +++++++++++++ .../applier/clusterobjectdeployment_test.go | 66 +++++++++++++++++++ 2 files changed, 112 insertions(+) create mode 100644 internal/operator-controller/applier/clusterobjectdeployment.go create mode 100644 internal/operator-controller/applier/clusterobjectdeployment_test.go diff --git a/internal/operator-controller/applier/clusterobjectdeployment.go b/internal/operator-controller/applier/clusterobjectdeployment.go new file mode 100644 index 0000000000..62095f5be2 --- /dev/null +++ b/internal/operator-controller/applier/clusterobjectdeployment.go @@ -0,0 +1,46 @@ +package applier + +import ( + ocv1 "github.com/operator-framework/operator-controller/api/v1" + ocv1ac "github.com/operator-framework/operator-controller/applyconfigurations/api/v1" + "github.com/operator-framework/operator-controller/internal/shared/labels" +) + +// buildClusterObjectDeployment constructs a ClusterObjectDeployment applyconfiguration +// from rendered content. It mirrors the structure of buildClusterObjectSet but produces +// a COD with a template instead of directly setting COS-specific fields. +func buildClusterObjectDeployment( + objects []ocv1ac.ClusterObjectSetObjectApplyConfiguration, + ext *ocv1.ClusterExtension, + annotations map[string]string, + collisionProtection ocv1.CollisionProtection, +) *ocv1ac.ClusterObjectDeploymentApplyConfiguration { + if annotations == nil { + annotations = make(map[string]string) + } + + phases := PhaseSort(objects) + + templateSpec := ocv1ac.ClusterObjectDeploymentTemplateSpec(). + WithPhases(phases...). + WithProgressionProbes(defaultProgressionProbes...). + WithCollisionProtection(collisionProtection) + + if p := ext.Spec.ProgressDeadlineMinutes; p > 0 { + templateSpec.WithProgressDeadlineMinutes(p) + } + + template := ocv1ac.ClusterObjectDeploymentTemplate(). + WithSpec(templateSpec) + + spec := ocv1ac.ClusterObjectDeploymentSpec(). + WithTemplate(template) + + return ocv1ac.ClusterObjectDeployment(ext.Name). + WithAnnotations(annotations). + WithLabels(map[string]string{ + labels.OwnerKindKey: ocv1.ClusterExtensionKind, + labels.OwnerNameKey: ext.Name, + }). + WithSpec(spec) +} diff --git a/internal/operator-controller/applier/clusterobjectdeployment_test.go b/internal/operator-controller/applier/clusterobjectdeployment_test.go new file mode 100644 index 0000000000..b23e68286b --- /dev/null +++ b/internal/operator-controller/applier/clusterobjectdeployment_test.go @@ -0,0 +1,66 @@ +package applier + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + + ocv1 "github.com/operator-framework/operator-controller/api/v1" + ocv1ac "github.com/operator-framework/operator-controller/applyconfigurations/api/v1" + "github.com/operator-framework/operator-controller/internal/shared/labels" +) + +func TestBuildClusterObjectDeployment(t *testing.T) { + ext := &ocv1.ClusterExtension{ + ObjectMeta: metav1.ObjectMeta{ + Name: "ext", + }, + Spec: ocv1.ClusterExtensionSpec{ + ProgressDeadlineMinutes: 15, + }, + } + + // Create a valid unstructured object with proper GVK + obj := &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": map[string]interface{}{ + "name": "test-cm", + }, + }, + } + obj.SetGroupVersionKind(corev1.SchemeGroupVersion.WithKind("ConfigMap")) + + objects := []ocv1ac.ClusterObjectSetObjectApplyConfiguration{ + *ocv1ac.ClusterObjectSetObject().WithObject(*obj), + } + + annotations := map[string]string{ + "test-key": "test-value", + } + + collisionProtection := ocv1.CollisionProtectionPrevent + + result := buildClusterObjectDeployment(objects, ext, annotations, collisionProtection) + + require.NotNil(t, result) + assert.Equal(t, "ext", *result.Name) + assert.Equal(t, "test-value", result.Annotations["test-key"]) + assert.Equal(t, ocv1.ClusterExtensionKind, result.Labels[labels.OwnerKindKey]) + assert.Equal(t, "ext", result.Labels[labels.OwnerNameKey]) + + require.NotNil(t, result.Spec) + require.NotNil(t, result.Spec.Template) + require.NotNil(t, result.Spec.Template.Spec) + + templateSpec := result.Spec.Template.Spec + assert.Len(t, templateSpec.Phases, 1) + assert.Equal(t, ocv1.CollisionProtectionPrevent, *templateSpec.CollisionProtection) + assert.Equal(t, int32(15), *templateSpec.ProgressDeadlineMinutes) + assert.NotEmpty(t, templateSpec.ProgressionProbes) +} From 2ff47949f7df48c97a7f72247cb5f3caeceaf658 Mon Sep 17 00:00:00 2001 From: "Per G. da Silva" Date: Thu, 22 Oct 2026 19:17:19 +0200 Subject: [PATCH 04/13] =?UTF-8?q?=E2=9C=A8=20generate=20a=20packed=20Clust?= =?UTF-8?q?erObjectDeployment=20from=20bundle=20content?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add GenerateClusterObjectDeployment method to SimpleRevisionGenerator that renders bundle manifests into a ClusterObjectDeployment with content-addressed Secrets. This provides the generator seam that will be used by the applier when writing CODs instead of managing ClusterObjectSets directly. Key changes: - Extract shared object-rendering logic from GenerateRevision into renderObjects helper (DRY principle) - Add GenerateClusterObjectDeployment that packs objects into Secrets and returns a COD applyconfiguration with only refs - Bundle-metadata annotations are placed on COD metadata - CollisionProtection is set to Prevent - Add SystemNamespace field to SimpleRevisionGenerator for Secret packing - Add replaceInlineWithRefsInCOD helper for COD template phases Co-Authored-By: Claude Sonnet 4.5 Signed-off-by: Per G. da Silva --- cmd/operator-controller/main.go | 1 + .../operator-controller/applier/boxcutter.go | 44 +++++-- .../applier/clusterobjectdeployment.go | 101 +++++++++++++++ .../applier/clusterobjectdeployment_test.go | 120 ++++++++++++++++++ 4 files changed, 253 insertions(+), 13 deletions(-) diff --git a/cmd/operator-controller/main.go b/cmd/operator-controller/main.go index 0192d06579..356c37c93c 100644 --- a/cmd/operator-controller/main.go +++ b/cmd/operator-controller/main.go @@ -618,6 +618,7 @@ func (c *boxcutterReconcilerConfigurator) Configure(ceReconciler *controllers.Cl rg := &applier.SimpleRevisionGenerator{ Scheme: c.mgr.GetScheme(), ManifestProvider: c.regv1ManifestProvider, + SystemNamespace: cfg.systemNamespace, } fieldOwner := fmt.Sprintf("%s/clusterextension-controller", fieldOwnerPrefix) appl := &applier.Boxcutter{ diff --git a/internal/operator-controller/applier/boxcutter.go b/internal/operator-controller/applier/boxcutter.go index 0232ed3da7..0a8d804422 100644 --- a/internal/operator-controller/applier/boxcutter.go +++ b/internal/operator-controller/applier/boxcutter.go @@ -52,6 +52,7 @@ type ClusterObjectSetGenerator interface { type SimpleRevisionGenerator struct { Scheme *runtime.Scheme ManifestProvider ManifestProvider + SystemNamespace string } func (r *SimpleRevisionGenerator) GenerateRevisionFromHelmRelease( @@ -139,7 +140,33 @@ func (r *SimpleRevisionGenerator) GenerateRevision( } } - // objectLabels + // Prepare object annotations from revision annotations + annotationUpdates := map[string]string{} + if v := revisionAnnotations[labels.BundleVersionKey]; v != "" { + annotationUpdates[labels.BundleVersionKey] = v + } + if v := revisionAnnotations[labels.PackageNameKey]; v != "" { + annotationUpdates[labels.PackageNameKey] = v + } + + // Render objects with labels and annotations + objs, err := r.renderObjects(ctx, plain, objectLabels, annotationUpdates) + if err != nil { + return nil, err + } + + rev := r.buildClusterObjectSet(objs, ext, revisionAnnotations) + rev.Spec.WithCollisionProtection(ocv1.CollisionProtectionPrevent) + return rev, nil +} + +// renderObjects converts plain client.Objects to ClusterObjectSetObjectApplyConfigurations +// with labels and annotations applied. This is shared logic used by both COS and COD generation. +func (r *SimpleRevisionGenerator) renderObjects( + ctx context.Context, + plain []client.Object, + objectLabels, objectAnnotations map[string]string, +) ([]ocv1ac.ClusterObjectSetObjectApplyConfiguration, error) { objs := make([]ocv1ac.ClusterObjectSetObjectApplyConfiguration, 0, len(plain)) for _, obj := range plain { obj.SetLabels(mergeStringMaps(obj.GetLabels(), objectLabels)) @@ -162,23 +189,14 @@ func (r *SimpleRevisionGenerator) GenerateRevision( } sanitizedUnstructured(ctx, &unstr) - annotationUpdates := map[string]string{} - if v := revisionAnnotations[labels.BundleVersionKey]; v != "" { - annotationUpdates[labels.BundleVersionKey] = v - } - if v := revisionAnnotations[labels.PackageNameKey]; v != "" { - annotationUpdates[labels.PackageNameKey] = v - } - if len(annotationUpdates) > 0 { - unstr.SetAnnotations(mergeStringMaps(unstr.GetAnnotations(), annotationUpdates)) + if len(objectAnnotations) > 0 { + unstr.SetAnnotations(mergeStringMaps(unstr.GetAnnotations(), objectAnnotations)) } objs = append(objs, *ocv1ac.ClusterObjectSetObject(). WithObject(unstr)) } - rev := r.buildClusterObjectSet(objs, ext, revisionAnnotations) - rev.Spec.WithCollisionProtection(ocv1.CollisionProtectionPrevent) - return rev, nil + return objs, nil } // sanitizedUnstructured takes an unstructured obj, removes status if present, and returns a sanitized copy containing only the allowed metadata entries set below. diff --git a/internal/operator-controller/applier/clusterobjectdeployment.go b/internal/operator-controller/applier/clusterobjectdeployment.go index 62095f5be2..5fb1bbeba7 100644 --- a/internal/operator-controller/applier/clusterobjectdeployment.go +++ b/internal/operator-controller/applier/clusterobjectdeployment.go @@ -1,8 +1,15 @@ package applier import ( + "context" + "fmt" + "io/fs" + + corev1 "k8s.io/api/core/v1" + ocv1 "github.com/operator-framework/operator-controller/api/v1" ocv1ac "github.com/operator-framework/operator-controller/applyconfigurations/api/v1" + "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/bundle/source" "github.com/operator-framework/operator-controller/internal/shared/labels" ) @@ -44,3 +51,97 @@ func buildClusterObjectDeployment( }). WithSpec(spec) } + +// GenerateClusterObjectDeployment renders bundle content into a ClusterObjectDeployment +// with packed Secrets. It returns the COD applyconfiguration with ref'd phases and the +// content-addressed Secrets to create. +func (r *SimpleRevisionGenerator) GenerateClusterObjectDeployment( + ctx context.Context, + bundleFS fs.FS, + ext *ocv1.ClusterExtension, + objectLabels, revisionAnnotations map[string]string, +) (*ocv1ac.ClusterObjectDeploymentApplyConfiguration, []corev1.Secret, error) { + // Get manifests from ManifestProvider + plain, err := r.ManifestProvider.Get(bundleFS, ext) + if err != nil { + return nil, nil, err + } + + if revisionAnnotations == nil { + revisionAnnotations = map[string]string{} + } + + // Get bundle annotations + bundleAnnotations, err := getBundleAnnotations(bundleFS) + if err != nil { + return nil, nil, fmt.Errorf("error getting bundle annotations: %w", err) + } + + // Merge bundle annotations with revision annotations for object annotations + objectAnnotations := map[string]string{} + if v := revisionAnnotations[labels.BundleVersionKey]; v != "" { + objectAnnotations[labels.BundleVersionKey] = v + } + if v := revisionAnnotations[labels.PackageNameKey]; v != "" { + objectAnnotations[labels.PackageNameKey] = v + } + + // Render objects with labels and annotations + objs, err := r.renderObjects(ctx, plain, objectLabels, objectAnnotations) + if err != nil { + return nil, nil, err + } + + // Build COD with bundle metadata annotations + codAnnotations := make(map[string]string) + for k, v := range revisionAnnotations { + codAnnotations[k] = v + } + // Add bundle properties annotations + for _, key := range []string{ + source.PropertyOLMProperties, + } { + if value, ok := bundleAnnotations[key]; ok { + codAnnotations[key] = value + } + } + + // Create COD applyconfiguration (with inline objects initially) + cod := buildClusterObjectDeployment(objs, ext, codAnnotations, ocv1.CollisionProtectionPrevent) + + // Pack objects into Secrets + phases := extractPhasesForPacking(cod.Spec.Template.Spec.Phases) + packer := &SecretPacker{ + OwnerName: ext.Name, + SystemNamespace: r.SystemNamespace, + } + packResult, err := packer.Pack(phases) + if err != nil { + return nil, nil, fmt.Errorf("packing objects into Secrets: %w", err) + } + + // Replace inline objects with refs in COD template phases + replaceInlineWithRefsInCOD(cod, packResult) + + return cod, packResult.Secrets, nil +} + +// replaceInlineWithRefsInCOD replaces inline objects in a COD's template phases with refs. +func replaceInlineWithRefsInCOD(cod *ocv1ac.ClusterObjectDeploymentApplyConfiguration, pack *PackResult) { + if cod.Spec == nil || cod.Spec.Template == nil || cod.Spec.Template.Spec == nil { + return + } + for phaseIdx := range cod.Spec.Template.Spec.Phases { + for objIdx := range cod.Spec.Template.Spec.Phases[phaseIdx].Objects { + ref, ok := pack.Refs[[2]int{phaseIdx, objIdx}] + if !ok { + continue + } + cod.Spec.Template.Spec.Phases[phaseIdx].Objects[objIdx].Object = nil + cod.Spec.Template.Spec.Phases[phaseIdx].Objects[objIdx].Ref = ocv1ac.ObjectSourceRef(). + WithName(ref.Name). + WithNamespace(ref.Namespace). + WithKey(ref.Key) + } + } +} diff --git a/internal/operator-controller/applier/clusterobjectdeployment_test.go b/internal/operator-controller/applier/clusterobjectdeployment_test.go index b23e68286b..c8e4930164 100644 --- a/internal/operator-controller/applier/clusterobjectdeployment_test.go +++ b/internal/operator-controller/applier/clusterobjectdeployment_test.go @@ -1,6 +1,8 @@ package applier import ( + "context" + "io/fs" "testing" "github.com/stretchr/testify/assert" @@ -8,12 +10,29 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + k8scheme "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/controller-runtime/pkg/client" ocv1 "github.com/operator-framework/operator-controller/api/v1" ocv1ac "github.com/operator-framework/operator-controller/applyconfigurations/api/v1" + bundlecsv "github.com/operator-framework/operator-controller/internal/testing/bundle/csv" + bundlefs "github.com/operator-framework/operator-controller/internal/testing/bundle/fs" "github.com/operator-framework/operator-controller/internal/shared/labels" ) +// fakeManifestProvider is a test double that returns a fixed list of objects +type fakeManifestProvider struct { + objects []client.Object + err error +} + +func (f *fakeManifestProvider) Get(_ fs.FS, _ *ocv1.ClusterExtension) ([]client.Object, error) { + if f.err != nil { + return nil, f.err + } + return f.objects, nil +} + func TestBuildClusterObjectDeployment(t *testing.T) { ext := &ocv1.ClusterExtension{ ObjectMeta: metav1.ObjectMeta{ @@ -64,3 +83,104 @@ func TestBuildClusterObjectDeployment(t *testing.T) { assert.Equal(t, int32(15), *templateSpec.ProgressDeadlineMinutes) assert.NotEmpty(t, templateSpec.ProgressionProbes) } + +func TestGenerateClusterObjectDeployment_PacksAndRefs(t *testing.T) { + // Create a test bundle with proper metadata + testBundle := bundlefs.Builder(). + WithPackageName("test-package"). + WithCSV(bundlecsv.Builder(). + WithName("test-csv"). + WithAnnotations(map[string]string{ + labels.BundleNameKey: "test-bundle", + labels.PackageNameKey: "test-package", + labels.BundleVersionKey: "1.0.0", + }). + Build()). + Build() + + fakeProvider := &fakeManifestProvider{ + objects: []client.Object{ + &corev1.ConfigMap{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "v1", + Kind: "ConfigMap", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cm", + }, + }, + &corev1.Secret{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "v1", + Kind: "Secret", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "test-secret", + }, + }, + }, + } + + gen := &SimpleRevisionGenerator{ + Scheme: k8scheme.Scheme, + ManifestProvider: fakeProvider, + SystemNamespace: "test-system", + } + + ext := &ocv1.ClusterExtension{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-ext", + }, + } + + bundleAnnotations := map[string]string{ + labels.BundleNameKey: "test-bundle", + labels.PackageNameKey: "test-package", + labels.BundleVersionKey: "1.0.0", + labels.BundleReferenceKey: "test-catalog/test-package@sha256:abc123", + } + + cod, secrets, err := gen.GenerateClusterObjectDeployment( + context.Background(), + testBundle, + ext, + map[string]string{"test-label": "test-value"}, + bundleAnnotations, + ) + + require.NoError(t, err) + require.NotNil(t, cod) + require.NotEmpty(t, secrets) + + t.Log("by checking bundle annotations are on COD metadata") + assert.Equal(t, "test-bundle", cod.Annotations[labels.BundleNameKey]) + assert.Equal(t, "test-package", cod.Annotations[labels.PackageNameKey]) + assert.Equal(t, "1.0.0", cod.Annotations[labels.BundleVersionKey]) + assert.Equal(t, "test-catalog/test-package@sha256:abc123", cod.Annotations[labels.BundleReferenceKey]) + + t.Log("by checking COD name matches extension") + assert.Equal(t, "test-ext", *cod.Name) + + t.Log("by checking COD has owner labels") + assert.Equal(t, ocv1.ClusterExtensionKind, cod.Labels[labels.OwnerKindKey]) + assert.Equal(t, "test-ext", cod.Labels[labels.OwnerNameKey]) + + t.Log("by checking template spec has collision protection set to Prevent") + require.NotNil(t, cod.Spec.Template.Spec.CollisionProtection) + assert.Equal(t, ocv1.CollisionProtectionPrevent, *cod.Spec.Template.Spec.CollisionProtection) + + t.Log("by checking all phases contain only refs, no inline objects") + require.NotNil(t, cod.Spec.Template.Spec.Phases) + for _, phase := range cod.Spec.Template.Spec.Phases { + for _, obj := range phase.Objects { + assert.Nil(t, obj.Object, "phase %s should have no inline objects", *phase.Name) + assert.NotNil(t, obj.Ref, "phase %s should have refs", *phase.Name) + } + } + + t.Log("by checking Secrets are named with extension prefix") + for _, secret := range secrets { + assert.Contains(t, secret.Name, "test-ext-", "Secret name should be prefixed with extension name") + assert.Equal(t, "test-ext", secret.Labels[labels.OwnerNameKey]) + } +} From 205ab8bec6c621412e95516644b646753e002207 Mon Sep 17 00:00:00 2001 From: "Per G. da Silva" Date: Thu, 22 Oct 2026 19:38:19 +0200 Subject: [PATCH 05/13] =?UTF-8?q?=E2=9C=A8=20apply=20a=20ClusterObjectDepl?= =?UTF-8?q?oyment=20instead=20of=20managing=20ClusterObjectSets?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rewrite Boxcutter.Apply to server-side apply a single ClusterObjectDeployment (COD) per ClusterExtension and create its referenced, content-addressed Secrets, instead of creating and garbage-collecting ClusterObjectSet revisions directly. The COD controller now owns revision creation, rollout and retention. - nil content: Get the COD named ext.Name; exists -> maintain state, NotFound -> error. - else: generate COD + Secrets, set CE owner refs, run preflights (Install when Status.Install is nil, else Upgrade) on objects reconstructed from the packed Secrets, create Secrets then SSA-apply the COD. Drop the COS install/upgrade state machine, revision numbering and retention helpers, Secret ownerReference patching, the unused SecretPacker.RevisionName field, and the dead SimpleRevisionGenerator.GenerateRevision path. Trim the ClusterObjectSetGenerator interface to GenerateClusterObjectDeployment and GenerateRevisionFromHelmRelease (migrator stays COS-based) and regenerate its mock. Co-Authored-By: Claude Opus 4.8 Signed-off-by: Per G. da Silva --- .../operator-controller/applier/boxcutter.go | 389 +---- .../applier/boxcutter_test.go | 1264 ++--------------- .../applier/secretpacker.go | 46 +- .../applier/secretpacker_test.go | 11 +- .../testutil/mock/applier/mock_applier.go | 20 +- 5 files changed, 265 insertions(+), 1465 deletions(-) diff --git a/internal/operator-controller/applier/boxcutter.go b/internal/operator-controller/applier/boxcutter.go index 0a8d804422..49470eeadc 100644 --- a/internal/operator-controller/applier/boxcutter.go +++ b/internal/operator-controller/applier/boxcutter.go @@ -18,7 +18,6 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/types" metav1ac "k8s.io/client-go/applyconfigurations/meta/v1" "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" @@ -31,17 +30,16 @@ import ( ocv1 "github.com/operator-framework/operator-controller/api/v1" ocv1ac "github.com/operator-framework/operator-controller/applyconfigurations/api/v1" "github.com/operator-framework/operator-controller/internal/operator-controller/revisions" - "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/bundle/source" "github.com/operator-framework/operator-controller/internal/shared/labels" "github.com/operator-framework/operator-controller/internal/shared/util/cache" ) -const ( - ClusterObjectSetRetentionLimit = 5 -) - type ClusterObjectSetGenerator interface { - GenerateRevision(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1ac.ClusterObjectSetApplyConfiguration, error) + GenerateClusterObjectDeployment( + ctx context.Context, + bundleFS fs.FS, ext *ocv1.ClusterExtension, + objectLabels, revisionAnnotations map[string]string, + ) (*ocv1ac.ClusterObjectDeploymentApplyConfiguration, []corev1.Secret, error) GenerateRevisionFromHelmRelease( ctx context.Context, helmRelease *release.Release, ext *ocv1.ClusterExtension, @@ -108,58 +106,6 @@ func (r *SimpleRevisionGenerator) GenerateRevisionFromHelmRelease( return rev, nil } -func (r *SimpleRevisionGenerator) GenerateRevision( - ctx context.Context, - bundleFS fs.FS, ext *ocv1.ClusterExtension, - objectLabels, revisionAnnotations map[string]string, -) (*ocv1ac.ClusterObjectSetApplyConfiguration, error) { - // extract plain manifests - plain, err := r.ManifestProvider.Get(bundleFS, ext) - if err != nil { - return nil, err - } - - if revisionAnnotations == nil { - revisionAnnotations = map[string]string{} - } - - // add bundle properties of interest to revision annotations - bundleAnnotations, err := getBundleAnnotations(bundleFS) - if err != nil { - return nil, fmt.Errorf("error getting bundle annotations: %w", err) - } - - // we don't care about all of the bundle and csv annotations as they can be quite confusing - // e.g. 'createdAt', 'capabilities', etc. - for _, key := range []string{ - // used by other operators that care about the bundle properties (e.g. maxOpenShiftVersion) - source.PropertyOLMProperties, - } { - if value, ok := bundleAnnotations[key]; ok { - revisionAnnotations[key] = value - } - } - - // Prepare object annotations from revision annotations - annotationUpdates := map[string]string{} - if v := revisionAnnotations[labels.BundleVersionKey]; v != "" { - annotationUpdates[labels.BundleVersionKey] = v - } - if v := revisionAnnotations[labels.PackageNameKey]; v != "" { - annotationUpdates[labels.PackageNameKey] = v - } - - // Render objects with labels and annotations - objs, err := r.renderObjects(ctx, plain, objectLabels, annotationUpdates) - if err != nil { - return nil, err - } - - rev := r.buildClusterObjectSet(objs, ext, revisionAnnotations) - rev.Spec.WithCollisionProtection(ocv1.CollisionProtectionPrevent) - return rev, nil -} - // renderObjects converts plain client.Objects to ClusterObjectSetObjectApplyConfigurations // with labels and annotations applied. This is shared logic used by both COS and COD generation. func (r *SimpleRevisionGenerator) renderObjects( @@ -479,31 +425,27 @@ type Boxcutter struct { } func (bc *Boxcutter) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (bool, string, error) { - // List existing revisions first to validate cluster connectivity before checking contentFS. - // This ensures we fail fast on API errors rather than attempting fallback behavior when - // cluster access is unavailable (since the ClusterObjectSet controller also requires - // API access to maintain resources). The revision list is also needed to determine if fallback - // is possible when contentFS is nil (at least one revision must exist). - existingRevisions, err := revisions.ListForGroup(ctx, bc.Client, ext.Name) - if err != nil { - return false, "", fmt.Errorf("listing revisions: %w", err) - } - // If contentFS is nil, we're maintaining the current state without catalog access. - // In this case, we should use the existing installed revision without generating a new one. + // In this case, we rely on the already-installed ClusterObjectDeployment (and its + // ClusterObjectDeployment controller) to keep reconciling the workload. if contentFS == nil { - if len(existingRevisions) == 0 { - return false, "", fmt.Errorf("catalog content unavailable and no revision installed") + existing := &ocv1.ClusterObjectDeployment{} + err := bc.Client.Get(ctx, client.ObjectKey{Name: ext.Name}, existing) + switch { + case err == nil: + // Returning true signals that the rollout has succeeded using the current + // deployment, which the ClusterObjectDeployment controller continues to + // reconcile even when catalog access is unavailable. + return true, "", nil + case apierrors.IsNotFound(err): + return false, "", fmt.Errorf("catalog content unavailable and no deployment installed") + default: + return false, "", fmt.Errorf("getting ClusterObjectDeployment %q: %w", ext.Name, err) } - // Returning true here signals that the rollout has succeeded using the current revision. - // This assumes the ClusterObjectSet controller is running and will continue to - // reconcile, apply, and maintain the resources defined in that revision via Server-Side Apply, - // ensuring the workload keeps running even when catalog access is unavailable. - return true, "", nil } - // Generate desired revision - desiredRevision, err := bc.RevisionGenerator.GenerateRevision(ctx, contentFS, ext, objectLabels, revisionAnnotations) + // Generate the desired ClusterObjectDeployment and the Secrets its template refs. + cod, secrets, err := bc.RevisionGenerator.GenerateClusterObjectDeployment(ctx, contentFS, ext, objectLabels, revisionAnnotations) if err != nil { return false, "", err } @@ -512,7 +454,7 @@ func (bc *Boxcutter) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.Clust if err != nil { return false, "", fmt.Errorf("get GVK for owner: %w", err) } - desiredRevision.WithOwnerReferences(metav1ac.OwnerReference(). + cod.WithOwnerReferences(metav1ac.OwnerReference(). WithAPIVersion(gvk.GroupVersion().String()). WithKind(gvk.Kind). WithName(ext.Name). @@ -520,159 +462,70 @@ func (bc *Boxcutter) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.Clust WithBlockOwnerDeletion(true). WithController(true)) - currentRevision := &ocv1.ClusterObjectSet{} - state := StateNeedsInstall - if len(existingRevisions) > 0 { - currentRevision = &existingRevisions[len(existingRevisions)-1] - desiredRevision.Spec.WithRevision(currentRevision.Spec.Revision) - desiredRevision.WithName(currentRevision.Name) - - // Save inline objects before externalization (needed for preflights + createExternalizedRevision) - savedInline := saveInlineObjects(desiredRevision) - - // Externalize with CURRENT revision name so refs match existing COS - phases := extractPhasesForPacking(desiredRevision.Spec.Phases) - packer := &SecretPacker{ - RevisionName: currentRevision.Name, - OwnerName: ext.Name, - SystemNamespace: bc.SystemNamespace, - } - packResult, err := packer.Pack(phases) - if err != nil { - return false, "", fmt.Errorf("packing for SSA comparison: %w", err) - } - replaceInlineWithRefs(desiredRevision, packResult) - - // SSA patch (refs-vs-refs) — just checking for changes. - err = bc.Client.Apply(ctx, desiredRevision, client.FieldOwner(bc.FieldOwner), client.ForceOwnership) - - // Restore inline objects for preflights + createExternalizedRevision - restoreInlineObjects(desiredRevision, savedInline) - - switch { - case apierrors.IsInvalid(err): - state = StateNeedsUpgrade - case err == nil: - state = StateUnchanged - default: - return false, "", fmt.Errorf("patching %s Revision: %w", currentRevision.Name, err) - } + // Preflights run on the rendered objects. Choose Install vs Upgrade based on whether + // the extension has recorded a prior successful install. + state := StateNeedsUpgrade + if ext.Status.Install == nil { + state = StateNeedsInstall + } + plainObjs, err := objectsFromSecrets(secrets) + if err != nil { + return false, "", fmt.Errorf("decoding packed objects for preflights: %w", err) } - - // Preflights - plainObjs := getObjects(desiredRevision) for _, preflight := range bc.Preflights { if shouldSkipPreflight(ctx, preflight, ext, state) { continue } switch state { case StateNeedsInstall: - err := preflight.Install(ctx, plainObjs) - if err != nil { + if err := preflight.Install(ctx, plainObjs); err != nil { return false, "", err } - // TODO: jlanford's IDE says that "StateNeedsUpgrade" condition is always true, but - // it isn't immediately obvious why that is. Perhaps len(existingRevisions) is - // always greater than 0 (seems unlikely), or shouldSkipPreflight always returns - // true (and we continue) when state == StateNeedsInstall? case StateNeedsUpgrade: - err := preflight.Upgrade(ctx, plainObjs) - if err != nil { + if err := preflight.Upgrade(ctx, plainObjs); err != nil { return false, "", err } } } - if state != StateUnchanged { - if err := bc.createExternalizedRevision(ctx, ext, desiredRevision, existingRevisions); err != nil { - return false, "", err - } - } else if currentRevision.Name != "" { - // In-place patch succeeded. Ensure any existing ref Secrets have ownerReferences - // (crash recovery for Step 3 failures). - if err := bc.ensureSecretOwnerReferences(ctx, currentRevision); err != nil { - return false, "", fmt.Errorf("ensuring ownerReferences on ref Secrets: %w", err) - } - } - - return true, "", nil -} - -// createExternalizedRevision creates a new COS with all objects externalized to Secrets. -// It follows a crash-safe three-step sequence: create Secrets, create COS, patch ownerRefs. -func (bc *Boxcutter) createExternalizedRevision(ctx context.Context, ext *ocv1.ClusterExtension, desiredRevision *ocv1ac.ClusterObjectSetApplyConfiguration, existingRevisions []ocv1.ClusterObjectSet) error { - prevRevisions := existingRevisions - latest := latestRevisionNumber(prevRevisions) - if latest >= ocv1.ClusterObjectSetMaxRevision { - return fmt.Errorf("revision limit %d reached for group %q", ocv1.ClusterObjectSetMaxRevision, ext.Name) - } - revisionNumber := latest + 1 - - revisionName := fmt.Sprintf("%s-%d", ext.Name, revisionNumber) - desiredRevision.WithName(revisionName) - desiredRevision.Spec.WithRevision(revisionNumber) - - if err := bc.garbageCollectOldRevisions(ctx, prevRevisions); err != nil { - return fmt.Errorf("garbage collecting old revisions: %w", err) - } - - // Externalize: pack inline objects into Secrets and replace with refs - phases := extractPhasesForPacking(desiredRevision.Spec.Phases) - packer := &SecretPacker{ - RevisionName: revisionName, - OwnerName: ext.Name, - SystemNamespace: bc.SystemNamespace, - } - packResult, err := packer.Pack(phases) - if err != nil { - return fmt.Errorf("packing objects into Secrets: %w", err) + // Create the referenced Secrets before applying the COD so the referenced content + // always exists for the ClusterObjectDeployment controller. Secrets are + // content-addressed and immutable, so AlreadyExists is expected and tolerated. + secretOwnerRef := metav1.OwnerReference{ + APIVersion: gvk.GroupVersion().String(), + Kind: gvk.Kind, + Name: ext.Name, + UID: ext.UID, + Controller: ptr.To(true), + BlockOwnerDeletion: ptr.To(true), } - replaceInlineWithRefs(desiredRevision, packResult) - - // Step 1: Create Secrets (skip AlreadyExists) - for i := range packResult.Secrets { - if err := bc.Client.Create(ctx, &packResult.Secrets[i]); err != nil { + for i := range secrets { + ensureOwnerReference(&secrets[i], secretOwnerRef) + if err := bc.Client.Create(ctx, &secrets[i]); err != nil { if !apierrors.IsAlreadyExists(err) { - return fmt.Errorf("creating ref Secret %q: %w", packResult.Secrets[i].Name, err) + return false, "", fmt.Errorf("creating ref Secret %q: %w", secrets[i].Name, err) } } } - // Step 2: Create COS with refs via SSA - if err := bc.Client.Apply(ctx, desiredRevision, client.FieldOwner(bc.FieldOwner), client.ForceOwnership); err != nil { - return fmt.Errorf("creating new Revision: %w", err) + // Server-side apply the ClusterObjectDeployment. The ClusterObjectDeployment + // controller owns revision (ClusterObjectSet) creation, rollout and retention. + if err := bc.Client.Apply(ctx, cod, client.FieldOwner(bc.FieldOwner), client.ForceOwnership); err != nil { + return false, "", fmt.Errorf("applying ClusterObjectDeployment: %w", err) } - // Step 3: Patch ownerReferences onto Secrets using the COS's UID - if err := bc.patchSecretOwnerReferences(ctx, desiredRevision, packResult.Secrets); err != nil { - return fmt.Errorf("patching ownerReferences on ref Secrets: %w", err) - } - return nil + return true, "", nil } -// garbageCollectOldRevisions deletes archived revisions beyond ClusterObjectSetRetentionLimit. -// Active revisions are never deleted. revisionList must be sorted oldest to newest. -func (bc *Boxcutter) garbageCollectOldRevisions(ctx context.Context, revisionList []ocv1.ClusterObjectSet) error { - for index, r := range revisionList { - // Only delete archived revisions that are beyond the limit - if index < len(revisionList)-ClusterObjectSetRetentionLimit && r.Spec.LifecycleState == ocv1.ClusterObjectSetLifecycleStateArchived { - if err := bc.Client.Delete(ctx, &ocv1.ClusterObjectSet{ - ObjectMeta: metav1.ObjectMeta{ - Name: r.Name, - }, - }); err != nil && !apierrors.IsNotFound(err) { - return fmt.Errorf("deleting archived revision: %w", err) - } +// ensureOwnerReference appends the given owner reference to the object if an owner +// reference with the same UID is not already present. +func ensureOwnerReference(obj metav1.Object, ref metav1.OwnerReference) { + for _, existing := range obj.GetOwnerReferences() { + if existing.UID == ref.UID { + return } } - return nil -} - -func latestRevisionNumber(prevRevisions []ocv1.ClusterObjectSet) int64 { - if len(prevRevisions) == 0 { - return 0 - } - return prevRevisions[len(prevRevisions)-1].Spec.Revision + obj.SetOwnerReferences(append(obj.GetOwnerReferences(), ref)) } var ( @@ -798,26 +651,6 @@ func splitManifestDocuments(file string) []string { return docs } -// getObjects returns a slice of all objects in the revision -func getObjects(rev *ocv1ac.ClusterObjectSetApplyConfiguration) []client.Object { - if rev.Spec == nil { - return nil - } - totalObjects := 0 - for _, phase := range rev.Spec.Phases { - totalObjects += len(phase.Objects) - } - objs := make([]client.Object, 0, totalObjects) - for _, phase := range rev.Spec.Phases { - for i := range phase.Objects { - if phase.Objects[i].Object != nil { - objs = append(objs, phase.Objects[i].Object) - } - } - } - return objs -} - func mergeStringMaps(m1, m2 map[string]string) map[string]string { merged := make(map[string]string, len(m1)+len(m2)) maps.Copy(merged, m1) @@ -825,28 +658,6 @@ func mergeStringMaps(m1, m2 map[string]string) map[string]string { return merged } -// saveInlineObjects saves Object pointers from each phase/object position. -func saveInlineObjects(rev *ocv1ac.ClusterObjectSetApplyConfiguration) [][]*unstructured.Unstructured { - saved := make([][]*unstructured.Unstructured, len(rev.Spec.Phases)) - for i, p := range rev.Spec.Phases { - saved[i] = make([]*unstructured.Unstructured, len(p.Objects)) - for j, o := range p.Objects { - saved[i][j] = o.Object - } - } - return saved -} - -// restoreInlineObjects restores saved inline objects and clears refs. -func restoreInlineObjects(rev *ocv1ac.ClusterObjectSetApplyConfiguration, saved [][]*unstructured.Unstructured) { - for i := range saved { - for j := range saved[i] { - rev.Spec.Phases[i].Objects[j].Object = saved[i][j] - rev.Spec.Phases[i].Objects[j].Ref = nil - } - } -} - // extractPhasesForPacking converts apply configuration phases to API types for SecretPacker. func extractPhasesForPacking(phases []ocv1ac.ClusterObjectSetPhaseApplyConfiguration) []ocv1.ClusterObjectSetPhase { result := make([]ocv1.ClusterObjectSetPhase, 0, len(phases)) @@ -890,85 +701,3 @@ func replaceInlineWithRefs(rev *ocv1ac.ClusterObjectSetApplyConfiguration, pack } } } - -// patchSecretOwnerReferences fetches the COS to get its UID, then patches ownerReferences onto all Secrets. -func (bc *Boxcutter) patchSecretOwnerReferences(ctx context.Context, rev *ocv1ac.ClusterObjectSetApplyConfiguration, secrets []corev1.Secret) error { - if len(secrets) == 0 { - return nil - } - - // Fetch the COS to get its UID - cos := &ocv1.ClusterObjectSet{} - if err := bc.Client.Get(ctx, client.ObjectKey{Name: *rev.GetName()}, cos); err != nil { - return fmt.Errorf("getting COS %q for ownerReference: %w", *rev.GetName(), err) - } - - return bc.patchOwnerRefsOnSecrets(ctx, cos.Name, cos.UID, secrets) -} - -// ensureSecretOwnerReferences checks referenced Secrets on an existing COS and patches missing ownerReferences. -// This handles crash recovery when Step 3 (patching ownerRefs) failed on a previous reconciliation. -func (bc *Boxcutter) ensureSecretOwnerReferences(ctx context.Context, cos *ocv1.ClusterObjectSet) error { - // List Secrets with the revision-name label - secretList := &corev1.SecretList{} - if err := bc.Client.List(ctx, secretList, - client.InNamespace(bc.SystemNamespace), - client.MatchingLabels{labels.RevisionNameKey: cos.Name}, - ); err != nil { - return fmt.Errorf("listing ref Secrets for revision %q: %w", cos.Name, err) - } - - var needsPatch []corev1.Secret - for _, s := range secretList.Items { - hasOwnerRef := false - for _, ref := range s.OwnerReferences { - if ref.UID == cos.UID { - hasOwnerRef = true - break - } - } - if !hasOwnerRef { - needsPatch = append(needsPatch, s) - } - } - - if len(needsPatch) == 0 { - return nil - } - - return bc.patchOwnerRefsOnSecrets(ctx, cos.Name, cos.UID, needsPatch) -} - -// patchOwnerRefsOnSecrets patches ownerReferences onto the given Secrets, pointing to the COS. -func (bc *Boxcutter) patchOwnerRefsOnSecrets(ctx context.Context, cosName string, cosUID types.UID, secrets []corev1.Secret) error { - ownerRef := metav1.OwnerReference{ - APIVersion: ocv1.GroupVersion.String(), - Kind: ocv1.ClusterObjectSetKind, - Name: cosName, - UID: cosUID, - Controller: ptr.To(true), - BlockOwnerDeletion: ptr.To(true), - } - - for i := range secrets { - s := &secrets[i] - // Check if ownerRef already set - alreadySet := false - for _, ref := range s.OwnerReferences { - if ref.UID == cosUID { - alreadySet = true - break - } - } - if alreadySet { - continue - } - - patch := client.MergeFrom(s.DeepCopy()) - s.OwnerReferences = append(s.OwnerReferences, ownerRef) - if err := bc.Client.Patch(ctx, s, patch); err != nil { - return fmt.Errorf("patching ownerReference on Secret %s/%s: %w", s.Namespace, s.Name, err) - } - } - return nil -} diff --git a/internal/operator-controller/applier/boxcutter_test.go b/internal/operator-controller/applier/boxcutter_test.go index d6c2ac5778..01764667fd 100644 --- a/internal/operator-controller/applier/boxcutter_test.go +++ b/internal/operator-controller/applier/boxcutter_test.go @@ -2,30 +2,21 @@ package applier_test import ( "context" - "errors" - "fmt" - "io/fs" "strings" "testing" - "testing/fstest" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.uber.org/mock/gomock" "helm.sh/helm/v3/pkg/release" "helm.sh/helm/v3/pkg/storage/driver" - appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" - apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/util/validation/field" - k8scheme "k8s.io/client-go/kubernetes/scheme" "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" - "sigs.k8s.io/controller-runtime/pkg/client/interceptor" ocv1 "github.com/operator-framework/operator-controller/api/v1" ocv1ac "github.com/operator-framework/operator-controller/applyconfigurations/api/v1" @@ -157,1115 +148,6 @@ func Test_SimpleRevisionGenerator_GenerateRevisionFromHelmRelease(t *testing.T) assert.Equal(t, expected.Spec.Phases, rev.Spec.Phases) } -func Test_SimpleRevisionGenerator_GenerateRevision(t *testing.T) { - ctrl := gomock.NewController(t) - r := mockapplier.NewMockManifestProvider(ctrl) - r.EXPECT().Get(gomock.Any(), gomock.Any()).Return([]client.Object{ - &corev1.Service{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-service", - }, - }, - &appsv1.Deployment{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-deployment", - Namespace: "test-ns", - Labels: map[string]string{"my-label": "my-label-value"}, - Annotations: map[string]string{"my-annotation": "my-annotation-value"}, - // Fields to be sanitized - Finalizers: []string{"test"}, - OwnerReferences: []metav1.OwnerReference{{Kind: "TestOwner"}}, - CreationTimestamp: metav1.Time{Time: metav1.Now().Time}, - UID: "1a2b3c4d", - ResourceVersion: "12345", - Generation: 123, - ManagedFields: []metav1.ManagedFieldsEntry{{Manager: "test-manager"}}, - DeletionTimestamp: &metav1.Time{Time: metav1.Now().Time}, - DeletionGracePeriodSeconds: func(i int64) *int64 { return &i }(30), - }, Status: appsv1.DeploymentStatus{ - Replicas: 3, - }, - }, - }, nil).AnyTimes() - - b := applier.SimpleRevisionGenerator{ - Scheme: k8scheme.Scheme, - ManifestProvider: r, - } - - ext := &ocv1.ClusterExtension{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-extension", - }, - Spec: ocv1.ClusterExtensionSpec{ - Namespace: "test-namespace", - }, - } - - rev, err := b.GenerateRevision(t.Context(), dummyBundle, ext, map[string]string{}, map[string]string{ - labels.BundleVersionKey: "1.0.0", - labels.PackageNameKey: "test-package", - }) - require.NoError(t, err) - - t.Log("by checking the olm.operatorframework.io/owner-name and owner-kind labels are set") - require.Equal(t, ptr.To(ext.Name), rev.Spec.Group) - require.Equal(t, map[string]string{ - labels.OwnerKindKey: ocv1.ClusterExtensionKind, - labels.OwnerNameKey: "test-extension", - }, rev.Labels) - t.Log("by checking the revision number is not set (defaults to zero value)") - require.Nil(t, rev.Spec.Revision) - t.Log("by checking the spec-level collisionProtection is set") - require.Equal(t, ptr.To(ocv1.CollisionProtectionPrevent), rev.Spec.CollisionProtection) - t.Log("by checking the rendered objects are present in the correct phases") - require.Equal(t, []ocv1ac.ClusterObjectSetPhaseApplyConfiguration{ - *ocv1ac.ClusterObjectSetPhase(). - WithName(string(applier.PhaseInfrastructure)). - WithObjects( - ocv1ac.ClusterObjectSetObject(). - WithObject(unstructured.Unstructured{ - Object: map[string]interface{}{ - "apiVersion": "v1", - "kind": "Service", - "metadata": map[string]interface{}{ - "name": "test-service", - "annotations": map[string]interface{}{ - "olm.operatorframework.io/bundle-version": "1.0.0", - "olm.operatorframework.io/package-name": "test-package", - }, - }, - "spec": map[string]interface{}{}, - }, - }), - ), - *ocv1ac.ClusterObjectSetPhase(). - WithName(string(applier.PhaseDeploy)). - WithObjects( - ocv1ac.ClusterObjectSetObject(). - WithObject(unstructured.Unstructured{ - Object: map[string]interface{}{ - "apiVersion": "apps/v1", - "kind": "Deployment", - "metadata": map[string]interface{}{ - "name": "test-deployment", - "namespace": "test-ns", - "labels": map[string]interface{}{ - "my-label": "my-label-value", - }, - "annotations": map[string]interface{}{ - "my-annotation": "my-annotation-value", - "olm.operatorframework.io/bundle-version": "1.0.0", - "olm.operatorframework.io/package-name": "test-package", - }, - }, - "spec": map[string]interface{}{ - "selector": nil, - "template": map[string]interface{}{ - "metadata": map[string]interface{}{}, - "spec": map[string]interface{}{ - "containers": nil, - }, - }, - "strategy": map[string]interface{}{}, - }, - }, - }), - ), - }, rev.Spec.Phases) -} - -func Test_SimpleRevisionGenerator_GenerateRevision_BundleAnnotations(t *testing.T) { - ctrl := gomock.NewController(t) - r := mockapplier.NewMockManifestProvider(ctrl) - r.EXPECT().Get(gomock.Any(), gomock.Any()).Return([]client.Object{}, nil).AnyTimes() - - b := applier.SimpleRevisionGenerator{ - Scheme: k8scheme.Scheme, - ManifestProvider: r, - } - - ext := &ocv1.ClusterExtension{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-extension", - }, - Spec: ocv1.ClusterExtensionSpec{ - Namespace: "test-namespace", - }, - } - - t.Run("bundle properties are copied to the olm.properties annotation", func(t *testing.T) { - bundleFS := bundlefs.Builder(). - WithPackageName("test-package"). - WithBundleProperty("olm.bundle.property", "some-value"). - WithBundleProperty("olm.another.bundle.property", "some-other-value"). - WithCSV(bundlecsv.Builder().WithName("test-csv").Build()). - Build() - - rev, err := b.GenerateRevision(t.Context(), bundleFS, ext, map[string]string{}, map[string]string{}) - require.NoError(t, err) - - t.Log("by checking bundle properties are added to the revision annotations") - require.NotNil(t, rev.Annotations) - require.JSONEq(t, `[{"type":"olm.bundle.property","value":"some-value"},{"type":"olm.another.bundle.property","value":"some-other-value"}]`, rev.Annotations["olm.properties"]) - }) - - t.Run("olm.properties should not be present if there are no bundle properties", func(t *testing.T) { - bundleFS := bundlefs.Builder(). - WithPackageName("test-package"). - WithCSV(bundlecsv.Builder().WithName("test-csv").Build()). - Build() - - rev, err := b.GenerateRevision(t.Context(), bundleFS, ext, map[string]string{}, map[string]string{}) - require.NoError(t, err) - - t.Log("by checking olm.properties is not present in the revision annotations") - _, ok := rev.Annotations["olm.properties"] - require.False(t, ok, "olm.properties should not be present in the revision annotations") - }) - - t.Run("csv annotations are not added to the revision annotations", func(t *testing.T) { - bundleFS := bundlefs.Builder(). - WithPackageName("test-package"). - WithBundleProperty("olm.bundle.property", "some-value"). - WithCSV(bundlecsv.Builder(). - WithName("test-csv"). - WithAnnotations(map[string]string{ - "some.csv.annotation": "some-other-value", - }). - Build()). - Build() - - rev, err := b.GenerateRevision(t.Context(), bundleFS, ext, map[string]string{}, map[string]string{}) - require.NoError(t, err) - - t.Log("by checking csv annotations are not added to the revision annotations") - _, ok := rev.Annotations["olm.csv.annotation"] - require.False(t, ok, "csv annotation should not be present in the revision annotations") - }) - - t.Run("errors getting bundle properties are surfaced", func(t *testing.T) { - _, err := b.GenerateRevision(t.Context(), fstest.MapFS{}, ext, map[string]string{}, map[string]string{}) - require.Error(t, err) - require.Contains(t, err.Error(), "metadata/annotations.yaml: file does not exist") - }) -} - -func Test_SimpleRevisionGenerator_Renderer_Integration(t *testing.T) { - ext := &ocv1.ClusterExtension{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-extension", - }, - } - ctrl := gomock.NewController(t) - r := mockapplier.NewMockManifestProvider(ctrl) - r.EXPECT().Get(dummyBundle, ext).DoAndReturn( - func(b fs.FS, e *ocv1.ClusterExtension) ([]client.Object, error) { - t.Log("by checking renderer was called with the correct parameters") - require.Equal(t, dummyBundle, b) - require.Equal(t, ext, e) - return nil, nil - }).AnyTimes() - b := applier.SimpleRevisionGenerator{ - Scheme: k8scheme.Scheme, - ManifestProvider: r, - } - - _, err := b.GenerateRevision(t.Context(), dummyBundle, ext, map[string]string{}, map[string]string{}) - require.NoError(t, err) -} - -func Test_SimpleRevisionGenerator_AppliesObjectLabelsAndRevisionAnnotations(t *testing.T) { - renderedObjs := []client.Object{ - &corev1.Service{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-service", - Labels: map[string]string{ - "app": "test-obj", - }, - }, - }, - &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-configmap", - Labels: map[string]string{ - "app": "test-obj", - }, - }, - }, - } - ctrl := gomock.NewController(t) - r := mockapplier.NewMockManifestProvider(ctrl) - r.EXPECT().Get(gomock.Any(), gomock.Any()).Return(renderedObjs, nil).AnyTimes() - - b := applier.SimpleRevisionGenerator{ - Scheme: k8scheme.Scheme, - ManifestProvider: r, - } - - revAnnotations := map[string]string{ - "other": "value", - } - - rev, err := b.GenerateRevision(t.Context(), dummyBundle, &ocv1.ClusterExtension{ - Spec: ocv1.ClusterExtensionSpec{ - Namespace: "test-namespace", - }, - }, map[string]string{ - "some": "value", - }, revAnnotations) - require.NoError(t, err) - t.Log("by checking the rendered objects contain the given object labels") - for _, phase := range rev.Spec.Phases { - for _, revObj := range phase.Objects { - require.Equal(t, map[string]string{ - "app": "test-obj", - "some": "value", - }, revObj.Object.GetLabels()) - } - } - t.Log("by checking the generated revision contain the given annotations") - require.Equal(t, revAnnotations, rev.Annotations) -} - -func Test_SimpleRevisionGenerator_PropagatesProgressDeadlineMinutes(t *testing.T) { - ctrl := gomock.NewController(t) - r := mockapplier.NewMockManifestProvider(ctrl) - r.EXPECT().Get(gomock.Any(), gomock.Any()).Return([]client.Object{}, nil).AnyTimes() - - b := applier.SimpleRevisionGenerator{ - Scheme: k8scheme.Scheme, - ManifestProvider: r, - } - - type args struct { - progressDeadlineMinutes *int32 - } - type want struct { - progressDeadlineMinutes *int32 - } - type testCase struct { - args args - want want - } - for name, tc := range map[string]testCase{ - "propagates when set": { - args: args{ - progressDeadlineMinutes: ptr.To(int32(10)), - }, - want: want{ - progressDeadlineMinutes: ptr.To(int32(10)), - }, - }, - "do not propagate when unset": { - want: want{ - progressDeadlineMinutes: nil, - }, - }, - } { - ext := &ocv1.ClusterExtension{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-extension", - }, - Spec: ocv1.ClusterExtensionSpec{ - Namespace: "test-namespace", - }, - } - empty := map[string]string{} - t.Run(name, func(t *testing.T) { - if pd := tc.args.progressDeadlineMinutes; pd != nil { - ext.Spec.ProgressDeadlineMinutes = *pd - } - - rev, err := b.GenerateRevision(t.Context(), dummyBundle, ext, empty, empty) - require.NoError(t, err) - require.Equal(t, tc.want.progressDeadlineMinutes, rev.Spec.ProgressDeadlineMinutes) - }) - } -} - -func Test_SimpleRevisionGenerator_Failure(t *testing.T) { - ctrl := gomock.NewController(t) - r := mockapplier.NewMockManifestProvider(ctrl) - r.EXPECT().Get(gomock.Any(), gomock.Any()).Return(nil, fmt.Errorf("some-error")).AnyTimes() - - b := applier.SimpleRevisionGenerator{ - Scheme: k8scheme.Scheme, - ManifestProvider: r, - } - - rev, err := b.GenerateRevision(t.Context(), fstest.MapFS{}, &ocv1.ClusterExtension{ - Spec: ocv1.ClusterExtensionSpec{ - Namespace: "test-namespace", - }, - }, map[string]string{}, map[string]string{}) - require.Nil(t, rev) - t.Log("by checking rendering errors are propagated") - require.Error(t, err) - require.Contains(t, err.Error(), "some-error") -} - -func TestBoxcutter_Apply(t *testing.T) { - testScheme := runtime.NewScheme() - require.NoError(t, ocv1.AddToScheme(testScheme)) - require.NoError(t, corev1.AddToScheme(testScheme)) - - // This is the revision that the mock builder will produce by default. - // We calculate its hash to use in the tests. - ext := &ocv1.ClusterExtension{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-ext", - UID: "test-uid", - }, - } - defaultDesiredRevision := &ocv1.ClusterObjectSet{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-ext-1", - UID: "rev-uid-1", - Labels: map[string]string{ - labels.OwnerNameKey: ext.Name, - }, - }, - Spec: ocv1.ClusterObjectSetSpec{ - Group: ext.Name, - Revision: 1, - Phases: []ocv1.ClusterObjectSetPhase{ - { - Name: string(applier.PhaseDeploy), - Objects: []ocv1.ClusterObjectSetObject{ - { - Object: unstructured.Unstructured{ - Object: map[string]interface{}{ - "apiVersion": "v1", - "kind": "ConfigMap", - "metadata": map[string]interface{}{ - "name": "test-cm", - }, - }, - }, - }, - }, - }, - }, - }, - } - - // Exercise group-based discovery when the existing revision lacks the owner-name label. - revisionWithoutOwnerLabel := defaultDesiredRevision.DeepCopy() - revisionWithoutOwnerLabel.Labels = nil - - allowedRevisionValue := func(revNum int64) *interceptor.Funcs { - return &interceptor.Funcs{ - Apply: func(ctx context.Context, client client.WithWatch, obj runtime.ApplyConfiguration, opts ...client.ApplyOption) error { - cos, ok := obj.(*ocv1ac.ClusterObjectSetApplyConfiguration) - if !ok { - return fmt.Errorf("expected ClusterObjectSetApplyConfiguration, got %T", obj) - } - if cos.Spec == nil || cos.Spec.Revision == nil || *cos.Spec.Revision != revNum { - gk := ocv1.SchemeGroupVersion.WithKind("ClusterObjectSet").GroupKind() - name := "" - if n := cos.GetName(); n != nil { - name = *n - } - return apierrors.NewInvalid(gk, name, field.ErrorList{field.Invalid(field.NewPath("spec.phases"), "immutable", "spec.phases is immutable")}) - } - return client.Apply(ctx, obj, opts...) - }, - } - } - testCases := []struct { - name string - mockBuilder func(t *testing.T) applier.ClusterObjectSetGenerator - existingObjs []client.Object - expectedErr string - validate func(t *testing.T, c client.Client) - clientIterceptor *interceptor.Funcs - }{ - { - name: "first revision", - mockBuilder: func(t *testing.T) applier.ClusterObjectSetGenerator { - ctrl := gomock.NewController(t) - m := mockapplier.NewMockClusterObjectSetGenerator(ctrl) - m.EXPECT().GenerateRevision(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn( - func(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1ac.ClusterObjectSetApplyConfiguration, error) { - return ocv1ac.ClusterObjectSet(""). - WithAnnotations(revisionAnnotations). - WithLabels(map[string]string{ - labels.OwnerNameKey: ext.Name, - }). - WithSpec(ocv1ac.ClusterObjectSetSpec(). - WithGroup(ext.Name). - WithPhases( - ocv1ac.ClusterObjectSetPhase(). - WithName(string(applier.PhaseDeploy)). - WithObjects( - ocv1ac.ClusterObjectSetObject(). - WithObject(unstructured.Unstructured{ - Object: map[string]interface{}{ - "apiVersion": "v1", - "kind": "ConfigMap", - "metadata": map[string]interface{}{ - "name": "test-cm", - }, - }, - }), - ), - ), - ), nil - }).AnyTimes() - m.EXPECT().GenerateRevisionFromHelmRelease(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil).AnyTimes() - return m - }, - validate: func(t *testing.T, c client.Client) { - revList := &ocv1.ClusterObjectSetList{} - err := c.List(t.Context(), revList, client.MatchingFields{".spec.group": ext.Name}) - require.NoError(t, err) - require.Len(t, revList.Items, 1) - - rev := revList.Items[0] - assert.Equal(t, "test-ext-1", rev.Name) - assert.Equal(t, int64(1), rev.Spec.Revision) - assert.Len(t, rev.OwnerReferences, 1) - assert.Equal(t, ext.Name, rev.OwnerReferences[0].Name) - assert.Equal(t, ext.UID, rev.OwnerReferences[0].UID) - assert.Equal(t, ocv1.GroupVersion.String(), rev.OwnerReferences[0].APIVersion) - assert.Equal(t, ocv1.ClusterExtensionKind, rev.OwnerReferences[0].Kind) - assert.Equal(t, ptr.To(true), rev.OwnerReferences[0].Controller) - assert.Equal(t, ptr.To(true), rev.OwnerReferences[0].BlockOwnerDeletion) - }, - }, - { - name: "no change, existing revision has no owner label", - mockBuilder: func(t *testing.T) applier.ClusterObjectSetGenerator { - ctrl := gomock.NewController(t) - m := mockapplier.NewMockClusterObjectSetGenerator(ctrl) - m.EXPECT().GenerateRevision(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn( - func(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1ac.ClusterObjectSetApplyConfiguration, error) { - return ocv1ac.ClusterObjectSet(""). - WithAnnotations(revisionAnnotations). - WithLabels(map[string]string{ - labels.OwnerNameKey: ext.Name, - }). - WithSpec(ocv1ac.ClusterObjectSetSpec(). - WithGroup(ext.Name). - WithPhases( - ocv1ac.ClusterObjectSetPhase(). - WithName(string(applier.PhaseDeploy)). - WithObjects( - ocv1ac.ClusterObjectSetObject(). - WithObject(unstructured.Unstructured{ - Object: map[string]interface{}{ - "apiVersion": "v1", - "kind": "ConfigMap", - "metadata": map[string]interface{}{ - "name": "test-cm", - }, - }, - }), - ), - ), - ), nil - }).AnyTimes() - m.EXPECT().GenerateRevisionFromHelmRelease(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil).AnyTimes() - return m - }, - existingObjs: []client.Object{ - revisionWithoutOwnerLabel, - }, - validate: func(t *testing.T, c client.Client) { - revList := &ocv1.ClusterObjectSetList{} - err := c.List(context.Background(), revList, client.MatchingFields{".spec.group": ext.Name}) - require.NoError(t, err) - // No new revision should be created - require.Len(t, revList.Items, 1) - assert.Equal(t, "test-ext-1", revList.Items[0].Name) - }, - }, - { - name: "new revision created when objects in new revision are different", - mockBuilder: func(t *testing.T) applier.ClusterObjectSetGenerator { - ctrl := gomock.NewController(t) - m := mockapplier.NewMockClusterObjectSetGenerator(ctrl) - m.EXPECT().GenerateRevision(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn( - func(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1ac.ClusterObjectSetApplyConfiguration, error) { - return ocv1ac.ClusterObjectSet(""). - WithAnnotations(revisionAnnotations). - WithLabels(map[string]string{ - labels.OwnerNameKey: ext.Name, - }). - WithSpec(ocv1ac.ClusterObjectSetSpec(). - WithGroup(ext.Name). - WithPhases( - ocv1ac.ClusterObjectSetPhase(). - WithName(string(applier.PhaseDeploy)). - WithObjects( - ocv1ac.ClusterObjectSetObject(). - WithObject(unstructured.Unstructured{ - Object: map[string]interface{}{ - "apiVersion": "v1", - "kind": "Secret", - "metadata": map[string]interface{}{ - "name": "new-secret", - }, - }, - }), - ), - ), - ), nil - }).AnyTimes() - m.EXPECT().GenerateRevisionFromHelmRelease(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil).AnyTimes() - return m - }, - clientIterceptor: allowedRevisionValue(2), - existingObjs: []client.Object{ - defaultDesiredRevision, - }, - validate: func(t *testing.T, c client.Client) { - revList := &ocv1.ClusterObjectSetList{} - err := c.List(context.Background(), revList, client.MatchingFields{".spec.group": ext.Name}) - require.NoError(t, err) - require.Len(t, revList.Items, 2) - - // Find the new revision (rev 2) - var newRev ocv1.ClusterObjectSet - for _, r := range revList.Items { - if r.Spec.Revision == 2 { - newRev = r - break - } - } - require.NotNil(t, newRev) - - assert.Equal(t, "test-ext-2", newRev.Name) - assert.Equal(t, int64(2), newRev.Spec.Revision) - }, - }, - { - name: "error from GenerateRevision", - mockBuilder: func(t *testing.T) applier.ClusterObjectSetGenerator { - ctrl := gomock.NewController(t) - m := mockapplier.NewMockClusterObjectSetGenerator(ctrl) - m.EXPECT().GenerateRevision(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, errors.New("render boom")).AnyTimes() - m.EXPECT().GenerateRevisionFromHelmRelease(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil).AnyTimes() - return m - }, - expectedErr: "render boom", - validate: func(t *testing.T, c client.Client) { - // Ensure no revisions were created - revList := &ocv1.ClusterObjectSetList{} - err := c.List(context.Background(), revList, client.MatchingFields{".spec.group": ext.Name}) - require.NoError(t, err) - assert.Empty(t, revList.Items) - }, - }, - { - name: "keep at most 5 past revisions across controller owners", - mockBuilder: func(t *testing.T) applier.ClusterObjectSetGenerator { - ctrl := gomock.NewController(t) - m := mockapplier.NewMockClusterObjectSetGenerator(ctrl) - m.EXPECT().GenerateRevision(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn( - func(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1ac.ClusterObjectSetApplyConfiguration, error) { - return ocv1ac.ClusterObjectSet(""). - WithAnnotations(revisionAnnotations). - WithLabels(map[string]string{ - labels.OwnerNameKey: ext.Name, - }). - WithSpec(ocv1ac.ClusterObjectSetSpec().WithGroup(ext.Name)), nil - }).AnyTimes() - m.EXPECT().GenerateRevisionFromHelmRelease(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil).AnyTimes() - return m - }, - existingObjs: []client.Object{ - &ocv1.ClusterObjectSet{ - ObjectMeta: metav1.ObjectMeta{ - Name: "rev-1", - OwnerReferences: []metav1.OwnerReference{{ - APIVersion: ocv1.GroupVersion.String(), Kind: ocv1.ClusterExtensionKind, - Name: "other-owner", UID: "other-uid", Controller: ptr.To(true), - }}, - Labels: map[string]string{ - labels.OwnerNameKey: ext.Name, - }, - }, - Spec: ocv1.ClusterObjectSetSpec{ - Group: ext.Name, - LifecycleState: ocv1.ClusterObjectSetLifecycleStateArchived, - Revision: 1, - }, - }, - &ocv1.ClusterObjectSet{ - ObjectMeta: metav1.ObjectMeta{ - Name: "rev-2", - Labels: map[string]string{ - labels.OwnerNameKey: ext.Name, - }, - }, - Spec: ocv1.ClusterObjectSetSpec{ - Group: ext.Name, - LifecycleState: ocv1.ClusterObjectSetLifecycleStateArchived, - Revision: 2, - }, - }, - &ocv1.ClusterObjectSet{ - ObjectMeta: metav1.ObjectMeta{ - Name: "rev-3", - Labels: map[string]string{ - labels.OwnerNameKey: ext.Name, - }, - }, - Spec: ocv1.ClusterObjectSetSpec{ - Group: ext.Name, - LifecycleState: ocv1.ClusterObjectSetLifecycleStateArchived, - Revision: 3, - }, - }, - &ocv1.ClusterObjectSet{ - ObjectMeta: metav1.ObjectMeta{ - Name: "rev-4", - Labels: map[string]string{ - labels.OwnerNameKey: ext.Name, - }, - }, - Spec: ocv1.ClusterObjectSetSpec{ - Group: ext.Name, - LifecycleState: ocv1.ClusterObjectSetLifecycleStateArchived, - Revision: 4, - }, - }, - &ocv1.ClusterObjectSet{ - ObjectMeta: metav1.ObjectMeta{ - Name: "rev-5", - Labels: map[string]string{ - labels.OwnerNameKey: ext.Name, - }, - }, - Spec: ocv1.ClusterObjectSetSpec{ - Group: ext.Name, - LifecycleState: ocv1.ClusterObjectSetLifecycleStateArchived, - Revision: 5, - }, - }, - &ocv1.ClusterObjectSet{ - ObjectMeta: metav1.ObjectMeta{ - Name: "rev-6", - OwnerReferences: []metav1.OwnerReference{{ - APIVersion: ocv1.GroupVersion.String(), Kind: ocv1.ClusterExtensionKind, - Name: "other-owner", UID: "other-uid", Controller: ptr.To(true), - }}, - Labels: map[string]string{ - labels.OwnerNameKey: ext.Name, - }, - }, - Spec: ocv1.ClusterObjectSetSpec{ - Group: ext.Name, - LifecycleState: ocv1.ClusterObjectSetLifecycleStateArchived, - Revision: 6, - }, - }, - }, - clientIterceptor: allowedRevisionValue(7), - validate: func(t *testing.T, c client.Client) { - rev1 := &ocv1.ClusterObjectSet{} - err := c.Get(t.Context(), client.ObjectKey{Name: "rev-1"}, rev1) - require.Error(t, err) - assert.True(t, apierrors.IsNotFound(err)) - - // Verify garbage collection: should only keep the limit + 1 (current) revisions - revList := &ocv1.ClusterObjectSetList{} - err = c.List(t.Context(), revList, client.MatchingFields{".spec.group": ext.Name}) - require.NoError(t, err) - // Should have ClusterObjectSetRetentionLimit (5) + current (1) = 6 revisions max - names := make([]string, 0, len(revList.Items)) - for _, rev := range revList.Items { - names = append(names, rev.Name) - } - require.ElementsMatch(t, []string{"rev-2", "rev-3", "rev-4", "rev-5", "rev-6", "test-ext-7"}, names) - created := &ocv1.ClusterObjectSet{} - require.NoError(t, c.Get(t.Context(), client.ObjectKey{Name: "test-ext-7"}, created)) - require.Equal(t, int64(7), created.Spec.Revision) - }, - }, - { - name: "keep active revisions when they are out of limit", - mockBuilder: func(t *testing.T) applier.ClusterObjectSetGenerator { - ctrl := gomock.NewController(t) - m := mockapplier.NewMockClusterObjectSetGenerator(ctrl) - m.EXPECT().GenerateRevision(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn( - func(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1ac.ClusterObjectSetApplyConfiguration, error) { - return ocv1ac.ClusterObjectSet(""). - WithAnnotations(revisionAnnotations). - WithLabels(map[string]string{ - labels.OwnerNameKey: ext.Name, - }). - WithSpec(ocv1ac.ClusterObjectSetSpec().WithGroup(ext.Name)), nil - }).AnyTimes() - m.EXPECT().GenerateRevisionFromHelmRelease(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil).AnyTimes() - return m - }, - existingObjs: []client.Object{ - &ocv1.ClusterObjectSet{ - ObjectMeta: metav1.ObjectMeta{ - Name: "rev-1", - Labels: map[string]string{ - labels.OwnerNameKey: ext.Name, - }, - }, - Spec: ocv1.ClusterObjectSetSpec{ - Group: ext.Name, - LifecycleState: ocv1.ClusterObjectSetLifecycleStateArchived, - Revision: 1, - }, - }, - &ocv1.ClusterObjectSet{ - ObjectMeta: metav1.ObjectMeta{ - Name: "rev-2", - Labels: map[string]string{ - labels.OwnerNameKey: ext.Name, - }, - }, - Spec: ocv1.ClusterObjectSetSpec{ - Group: ext.Name, - // index beyond the retention limit but active; should be preserved - LifecycleState: ocv1.ClusterObjectSetLifecycleStateActive, - Revision: 2, - }, - }, - &ocv1.ClusterObjectSet{ - ObjectMeta: metav1.ObjectMeta{ - Name: "rev-3", - Labels: map[string]string{ - labels.OwnerNameKey: ext.Name, - }, - }, - Spec: ocv1.ClusterObjectSetSpec{ - Group: ext.Name, - LifecycleState: ocv1.ClusterObjectSetLifecycleStateActive, - Revision: 3, - }, - }, - &ocv1.ClusterObjectSet{ - ObjectMeta: metav1.ObjectMeta{ - Name: "rev-4", - Labels: map[string]string{ - labels.OwnerNameKey: ext.Name, - }, - }, - Spec: ocv1.ClusterObjectSetSpec{ - Group: ext.Name, - // archived but should be preserved since it is within the limit - LifecycleState: ocv1.ClusterObjectSetLifecycleStateArchived, - Revision: 4, - }, - }, - &ocv1.ClusterObjectSet{ - ObjectMeta: metav1.ObjectMeta{ - Name: "rev-5", - Labels: map[string]string{ - labels.OwnerNameKey: ext.Name, - }, - }, - Spec: ocv1.ClusterObjectSetSpec{ - Group: ext.Name, - LifecycleState: ocv1.ClusterObjectSetLifecycleStateActive, - Revision: 5, - }, - }, - &ocv1.ClusterObjectSet{ - ObjectMeta: metav1.ObjectMeta{ - Name: "rev-6", - Labels: map[string]string{ - labels.OwnerNameKey: ext.Name, - }, - }, - Spec: ocv1.ClusterObjectSetSpec{ - Group: ext.Name, - LifecycleState: ocv1.ClusterObjectSetLifecycleStateActive, - Revision: 6, - }, - }, - &ocv1.ClusterObjectSet{ - ObjectMeta: metav1.ObjectMeta{ - Name: "rev-7", - Labels: map[string]string{ - labels.OwnerNameKey: ext.Name, - }, - }, - Spec: ocv1.ClusterObjectSetSpec{ - Group: ext.Name, - LifecycleState: ocv1.ClusterObjectSetLifecycleStateActive, - Revision: 7, - }, - }, - }, - clientIterceptor: allowedRevisionValue(8), - validate: func(t *testing.T, c client.Client) { - rev1 := &ocv1.ClusterObjectSet{} - err := c.Get(t.Context(), client.ObjectKey{Name: "rev-1"}, rev1) - require.Error(t, err) - assert.True(t, apierrors.IsNotFound(err)) - - rev2 := &ocv1.ClusterObjectSet{} - err = c.Get(t.Context(), client.ObjectKey{Name: "rev-2"}, rev2) - require.NoError(t, err) - - // Verify active revisions are kept even if beyond the limit - rev4 := &ocv1.ClusterObjectSet{} - err = c.Get(t.Context(), client.ObjectKey{Name: "rev-4"}, rev4) - require.NoError(t, err, "active revision 4 should still exist even though it's beyond the limit") - }, - }, - { - name: "annotation-only update (same phases, different annotations)", - mockBuilder: func(t *testing.T) applier.ClusterObjectSetGenerator { - ctrl := gomock.NewController(t) - m := mockapplier.NewMockClusterObjectSetGenerator(ctrl) - m.EXPECT().GenerateRevision(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn( - func(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1ac.ClusterObjectSetApplyConfiguration, error) { - return ocv1ac.ClusterObjectSet(""). - WithAnnotations(revisionAnnotations). - WithLabels(map[string]string{ - labels.OwnerNameKey: ext.Name, - }). - WithSpec(ocv1ac.ClusterObjectSetSpec(). - WithGroup(ext.Name). - WithPhases( - ocv1ac.ClusterObjectSetPhase(). - WithName(string(applier.PhaseDeploy)). - WithObjects( - ocv1ac.ClusterObjectSetObject(). - WithObject(unstructured.Unstructured{ - Object: map[string]interface{}{ - "apiVersion": "v1", - "kind": "ConfigMap", - "metadata": map[string]interface{}{ - "name": "test-cm", - }, - }, - }), - ), - ), - ), nil - }).AnyTimes() - m.EXPECT().GenerateRevisionFromHelmRelease(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil).AnyTimes() - return m - }, - existingObjs: []client.Object{ - ext, - &ocv1.ClusterObjectSet{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-ext-1", - Annotations: map[string]string{ - labels.BundleVersionKey: "1.0.0", - labels.PackageNameKey: "test-package", - }, - Labels: map[string]string{ - labels.OwnerNameKey: ext.Name, - }, - }, - Spec: ocv1.ClusterObjectSetSpec{ - Group: ext.Name, - Revision: 1, - Phases: []ocv1.ClusterObjectSetPhase{ - { - Name: string(applier.PhaseDeploy), - Objects: []ocv1.ClusterObjectSetObject{ - { - Object: unstructured.Unstructured{ - Object: map[string]interface{}{ - "apiVersion": "v1", - "kind": "ConfigMap", - "metadata": map[string]interface{}{ - "name": "test-cm", - }, - }, - }, - }, - }, - }, - }, - }, - }, - }, - validate: func(t *testing.T, c client.Client) { - revList := &ocv1.ClusterObjectSetList{} - err := c.List(context.Background(), revList, client.MatchingFields{".spec.group": ext.Name}) - require.NoError(t, err) - // Should still be only 1 revision (in-place update, not new revision) - require.Len(t, revList.Items, 1) - - rev := revList.Items[0] - assert.Equal(t, "test-ext-1", rev.Name) - assert.Equal(t, int64(1), rev.Spec.Revision) - // Verify annotations were updated - assert.Equal(t, "1.0.1", rev.Annotations[labels.BundleVersionKey]) - assert.Equal(t, "test-package", rev.Annotations[labels.PackageNameKey]) - // Verify owner label is still present - assert.Equal(t, ext.Name, rev.Labels[labels.OwnerNameKey]) - }, - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - // Setup - cb := testutil.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), coscontrollers.SetupIndexes). - WithObjects(tc.existingObjs...) - if tc.clientIterceptor != nil { - cb.WithInterceptorFuncs(*tc.clientIterceptor) - } - fakeClient := cb.Build() - - boxcutter := &applier.Boxcutter{ - Client: fakeClient, - Scheme: testScheme, - RevisionGenerator: tc.mockBuilder(t), - FieldOwner: "test-owner", - SystemNamespace: "olmv1-system", - } - - // We need a dummy fs.FS - testFS := fstest.MapFS{} - - // Execute - revisionAnnotations := map[string]string{} - if tc.name == "annotation-only update (same phases, different annotations)" { - // For annotation-only update test, pass NEW annotations - revisionAnnotations = map[string]string{ - labels.BundleVersionKey: "1.0.1", - labels.PackageNameKey: "test-package", - } - } - completed, status, err := boxcutter.Apply(t.Context(), testFS, ext, nil, revisionAnnotations) - - // Assert - if tc.expectedErr != "" { - require.Error(t, err) - assert.Contains(t, err.Error(), tc.expectedErr) - assert.False(t, completed) - assert.Empty(t, status) - } else { - require.NoError(t, err) - assert.True(t, completed) - assert.Empty(t, status) - } - - if tc.validate != nil { - // For the client create error, we need a client that *will* error. - // Since we can't do that easily, we will skip validation for that specific path - // as the state won't be what we expect. - if tc.name != "error from client create" { - tc.validate(t, fakeClient) - } - } - }) - } -} - -func TestBoxcutter_Apply_GroupIsolation(t *testing.T) { - for _, tc := range []struct { - name string - extensionRevisions []int64 - otherGroupRevision int64 - wantRevision int64 - wantRemainingNames []string - }{ - { - name: "numbering ignores another group's higher revision", - extensionRevisions: []int64{1}, - otherGroupRevision: 99, - wantRevision: 2, - wantRemainingNames: []string{"test-ext-1", "test-ext-2", "other-group-99"}, - }, - { - name: "retention preserves another group's older revision", - extensionRevisions: []int64{2, 3, 4, 5, 6, 7}, - otherGroupRevision: 1, - wantRevision: 8, - wantRemainingNames: []string{"test-ext-3", "test-ext-4", "test-ext-5", "test-ext-6", "test-ext-7", "test-ext-8", "other-group-1"}, - }, - } { - t.Run(tc.name, func(t *testing.T) { - scheme := runtime.NewScheme() - require.NoError(t, ocv1.AddToScheme(scheme)) - ext := &ocv1.ClusterExtension{ObjectMeta: metav1.ObjectMeta{Name: "test-ext", UID: "test-ext-uid"}} - owner := *metav1.NewControllerRef(ext, ocv1.GroupVersion.WithKind(ocv1.ClusterExtensionKind)) - objects := make([]client.Object, 0, len(tc.extensionRevisions)+1) - for _, revision := range tc.extensionRevisions { - objects = append(objects, &ocv1.ClusterObjectSet{ - ObjectMeta: metav1.ObjectMeta{ - Name: fmt.Sprintf("test-ext-%d", revision), - Labels: map[string]string{labels.OwnerNameKey: ext.Name}, - OwnerReferences: []metav1.OwnerReference{owner}, - }, - Spec: ocv1.ClusterObjectSetSpec{ - Group: ext.Name, Revision: revision, LifecycleState: ocv1.ClusterObjectSetLifecycleStateArchived, - }, - }) - } - // Even with a matching label and controller owner, another group is - // excluded from both revision numbering and retention. - otherGroup := &ocv1.ClusterObjectSet{ - ObjectMeta: metav1.ObjectMeta{ - Name: fmt.Sprintf("other-group-%d", tc.otherGroupRevision), - Labels: map[string]string{labels.OwnerNameKey: ext.Name}, - OwnerReferences: []metav1.OwnerReference{owner}, - }, - Spec: ocv1.ClusterObjectSetSpec{ - Group: "other-group", Revision: tc.otherGroupRevision, LifecycleState: ocv1.ClusterObjectSetLifecycleStateArchived, - }, - } - objects = append(objects, otherGroup) - cl := testutil.WithIndexes(t, fake.NewClientBuilder().WithScheme(scheme), coscontrollers.SetupIndexes). - WithObjects(objects...). - WithInterceptorFuncs(interceptor.Funcs{ - Apply: func(ctx context.Context, cl client.WithWatch, obj runtime.ApplyConfiguration, opts ...client.ApplyOption) error { - cos, ok := obj.(*ocv1ac.ClusterObjectSetApplyConfiguration) - if !ok { - return fmt.Errorf("expected ClusterObjectSetApplyConfiguration, got %T", obj) - } - // The fake client does not enforce spec immutability. Reject - // updates to existing COS so Apply creates a new revision. - err := cl.Get(ctx, client.ObjectKey{Name: *cos.GetName()}, &ocv1.ClusterObjectSet{}) - if err == nil { - return apierrors.NewInvalid(ocv1.GroupVersion.WithKind(ocv1.ClusterObjectSetKind).GroupKind(), *cos.GetName(), - field.ErrorList{field.Invalid(field.NewPath("spec.phases"), "updated phases", "spec.phases is immutable")}) - } - if !apierrors.IsNotFound(err) { - return err - } - return cl.Apply(ctx, obj, opts...) - }, - }).Build() - generator := mockapplier.NewMockClusterObjectSetGenerator(gomock.NewController(t)) - generator.EXPECT().GenerateRevision(gomock.Any(), gomock.Any(), ext, gomock.Any(), gomock.Any()).Return( - ocv1ac.ClusterObjectSet("").WithSpec(ocv1ac.ClusterObjectSetSpec(). - WithGroup(ext.Name).WithLifecycleState(ocv1.ClusterObjectSetLifecycleStateActive)), nil) - boxcutter := &applier.Boxcutter{Client: cl, Scheme: scheme, RevisionGenerator: generator, FieldOwner: "test-owner"} - - completed, status, err := boxcutter.Apply(t.Context(), fstest.MapFS{}, ext, nil, nil) - require.NoError(t, err) - require.True(t, completed) - require.Empty(t, status) - - var created ocv1.ClusterObjectSet - require.NoError(t, cl.Get(t.Context(), client.ObjectKey{Name: fmt.Sprintf("test-ext-%d", tc.wantRevision)}, &created)) - require.Equal(t, tc.wantRevision, created.Spec.Revision) - require.Equal(t, ext.Name, created.Spec.Group) - var remaining ocv1.ClusterObjectSetList - require.NoError(t, cl.List(t.Context(), &remaining)) - names := make([]string, 0, len(remaining.Items)) - for _, revision := range remaining.Items { - names = append(names, revision.Name) - } - require.ElementsMatch(t, tc.wantRemainingNames, names) - var retainedOtherGroup ocv1.ClusterObjectSet - require.NoError(t, cl.Get(t.Context(), client.ObjectKeyFromObject(otherGroup), &retainedOtherGroup)) - require.Equal(t, otherGroup.Spec, retainedOtherGroup.Spec) - require.Equal(t, otherGroup.OwnerReferences, retainedOtherGroup.OwnerReferences) - }) - } -} - func TestBoxcutterStorageMigrator(t *testing.T) { // defaultHelmRevisionResult returns the default result for GenerateRevisionFromHelmRelease in storage migrator tests. defaultHelmRevisionResult := func(ext *ocv1.ClusterExtension) *ocv1ac.ClusterObjectSetApplyConfiguration { @@ -1865,3 +747,149 @@ func TestBoxcutterStorageMigrator(t *testing.T) { require.NoError(t, err) }) } + +// codTestScheme returns a scheme with the types needed for COD-based Apply tests. +func codTestScheme(t *testing.T) *runtime.Scheme { + s := runtime.NewScheme() + require.NoError(t, ocv1.AddToScheme(s)) + require.NoError(t, corev1.AddToScheme(s)) + return s +} + +// newCODBoxcutter builds a Boxcutter backed by a real SimpleRevisionGenerator whose +// ManifestProvider renders the given objects. +func newCODBoxcutter(t *testing.T, scheme *runtime.Scheme, c client.Client, objs []client.Object) *applier.Boxcutter { + ctrl := gomock.NewController(t) + mp := mockapplier.NewMockManifestProvider(ctrl) + mp.EXPECT().Get(gomock.Any(), gomock.Any()).Return(objs, nil).AnyTimes() + return &applier.Boxcutter{ + Client: c, + Scheme: scheme, + RevisionGenerator: &applier.SimpleRevisionGenerator{ + Scheme: scheme, + ManifestProvider: mp, + SystemNamespace: "olmv1-system", + }, + FieldOwner: "test-owner", + SystemNamespace: "olmv1-system", + } +} + +func TestBoxcutterApply_WritesClusterObjectDeployment(t *testing.T) { + scheme := codTestScheme(t) + ext := &ocv1.ClusterExtension{ + ObjectMeta: metav1.ObjectMeta{Name: "test-ext", UID: "test-uid"}, + } + cl := fake.NewClientBuilder().WithScheme(scheme).Build() + bc := newCODBoxcutter(t, scheme, cl, []client.Object{ + &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "test-cm", Namespace: "test-ns"}}, + }) + + completed, status, err := bc.Apply(t.Context(), dummyBundle, ext, map[string]string{"some": "label"}, map[string]string{}) + require.NoError(t, err) + require.True(t, completed) + require.Empty(t, status) + + t.Log("by checking exactly one ClusterObjectDeployment named after the extension exists") + codList := &ocv1.ClusterObjectDeploymentList{} + require.NoError(t, cl.List(t.Context(), codList)) + require.Len(t, codList.Items, 1) + cod := codList.Items[0] + assert.Equal(t, ext.Name, cod.Name) + + t.Log("by checking the COD owner reference points to the ClusterExtension") + require.Len(t, cod.OwnerReferences, 1) + assert.Equal(t, ext.Name, cod.OwnerReferences[0].Name) + assert.Equal(t, ext.UID, cod.OwnerReferences[0].UID) + assert.Equal(t, ocv1.GroupVersion.String(), cod.OwnerReferences[0].APIVersion) + assert.Equal(t, ocv1.ClusterExtensionKind, cod.OwnerReferences[0].Kind) + assert.Equal(t, ptr.To(true), cod.OwnerReferences[0].Controller) + assert.Equal(t, ptr.To(true), cod.OwnerReferences[0].BlockOwnerDeletion) + + t.Log("by checking the template phases reference packed Secrets instead of inline objects") + require.NotEmpty(t, cod.Spec.Template.Spec.Phases) + refNames := map[string]struct{}{} + sawRef := false + for _, ph := range cod.Spec.Template.Spec.Phases { + for _, o := range ph.Objects { + require.Nil(t, o.Object.Object, "object should be externalized to a Secret ref") + require.NotNil(t, o.Ref) + refNames[o.Ref.Name] = struct{}{} + sawRef = true + } + } + require.True(t, sawRef) + + t.Log("by checking the referenced Secrets exist in the system namespace owned by the ClusterExtension") + secretList := &corev1.SecretList{} + require.NoError(t, cl.List(t.Context(), secretList, client.InNamespace("olmv1-system"))) + require.NotEmpty(t, secretList.Items) + for _, s := range secretList.Items { + require.Len(t, s.OwnerReferences, 1) + assert.Equal(t, ext.Name, s.OwnerReferences[0].Name) + assert.Equal(t, ext.UID, s.OwnerReferences[0].UID) + assert.Equal(t, ocv1.ClusterExtensionKind, s.OwnerReferences[0].Kind) + assert.Equal(t, ptr.To(true), s.OwnerReferences[0].Controller) + _, ok := refNames[s.Name] + assert.True(t, ok, "Secret %q should be referenced by the COD", s.Name) + } + + t.Log("by checking no ClusterObjectSet is created directly by the applier") + cosList := &ocv1.ClusterObjectSetList{} + require.NoError(t, cl.List(t.Context(), cosList)) + assert.Empty(t, cosList.Items) +} + +func TestBoxcutterApply_NilContentUsesExistingCOD(t *testing.T) { + scheme := codTestScheme(t) + ext := &ocv1.ClusterExtension{ + ObjectMeta: metav1.ObjectMeta{Name: "test-ext", UID: "test-uid"}, + } + + t.Run("errors when no deployment installed", func(t *testing.T) { + cl := fake.NewClientBuilder().WithScheme(scheme).Build() + bc := newCODBoxcutter(t, scheme, cl, nil) + completed, status, err := bc.Apply(t.Context(), nil, ext, nil, nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "catalog content unavailable and no deployment installed") + assert.False(t, completed) + assert.Empty(t, status) + }) + + t.Run("maintains current state when a deployment exists", func(t *testing.T) { + cl := fake.NewClientBuilder().WithScheme(scheme).WithObjects( + &ocv1.ClusterObjectDeployment{ObjectMeta: metav1.ObjectMeta{Name: ext.Name}}, + ).Build() + bc := newCODBoxcutter(t, scheme, cl, nil) + completed, status, err := bc.Apply(t.Context(), nil, ext, nil, nil) + require.NoError(t, err) + assert.True(t, completed) + assert.Empty(t, status) + }) +} + +func TestBoxcutterApply_UnchangedIsNoop(t *testing.T) { + scheme := codTestScheme(t) + ext := &ocv1.ClusterExtension{ + ObjectMeta: metav1.ObjectMeta{Name: "test-ext", UID: "test-uid"}, + } + cl := fake.NewClientBuilder().WithScheme(scheme).Build() + bc := newCODBoxcutter(t, scheme, cl, []client.Object{ + &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "test-cm", Namespace: "test-ns"}}, + }) + + _, _, err := bc.Apply(t.Context(), dummyBundle, ext, nil, map[string]string{}) + require.NoError(t, err) + + first := &ocv1.ClusterObjectDeployment{} + require.NoError(t, cl.Get(t.Context(), client.ObjectKey{Name: ext.Name}, first)) + + _, _, err = bc.Apply(t.Context(), dummyBundle, ext, nil, map[string]string{}) + require.NoError(t, err) + + second := &ocv1.ClusterObjectDeployment{} + require.NoError(t, cl.Get(t.Context(), client.ObjectKey{Name: ext.Name}, second)) + + t.Log("by checking applying identical content twice does not bump the COD generation") + assert.Equal(t, first.Generation, second.Generation) +} diff --git a/internal/operator-controller/applier/secretpacker.go b/internal/operator-controller/applier/secretpacker.go index 65ad338850..24c2cb8127 100644 --- a/internal/operator-controller/applier/secretpacker.go +++ b/internal/operator-controller/applier/secretpacker.go @@ -7,11 +7,14 @@ import ( "encoding/base64" "encoding/json" "fmt" + "io" "sort" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/utils/ptr" + "sigs.k8s.io/controller-runtime/pkg/client" ocv1 "github.com/operator-framework/operator-controller/api/v1" "github.com/operator-framework/operator-controller/internal/shared/labels" @@ -31,7 +34,6 @@ const ( // SecretPacker packs serialized objects from COS phases into one or more // immutable Secrets. type SecretPacker struct { - RevisionName string OwnerName string SystemNamespace string } @@ -173,6 +175,39 @@ func contentHash(data []byte) string { return base64.RawURLEncoding.EncodeToString(h[:]) } +// objectsFromSecrets reconstructs the packed objects stored in the given Secrets. +// It is the inverse of SecretPacker.Pack and is used to recover the rendered +// objects (e.g. for preflight checks) without re-rendering the bundle. Objects +// are returned in a deterministic order (by Secret order, then by sorted data key). +func objectsFromSecrets(secrets []corev1.Secret) ([]client.Object, error) { + var objs []client.Object + for i := range secrets { + data := secrets[i].Data + keys := make([]string, 0, len(data)) + for k := range data { + keys = append(keys, k) + } + sort.Strings(keys) + for _, k := range keys { + raw := data[k] + // Large objects are gzip-compressed by Pack; detect via the gzip magic bytes. + if len(raw) >= 2 && raw[0] == 0x1f && raw[1] == 0x8b { + decompressed, err := gunzipData(raw) + if err != nil { + return nil, fmt.Errorf("decompressing packed object %q in Secret %q: %w", k, secrets[i].Name, err) + } + raw = decompressed + } + u := &unstructured.Unstructured{} + if err := u.UnmarshalJSON(raw); err != nil { + return nil, fmt.Errorf("decoding packed object %q in Secret %q: %w", k, secrets[i].Name, err) + } + objs = append(objs, u) + } + } + return objs, nil +} + func gzipData(data []byte) ([]byte, error) { var buf bytes.Buffer w, err := gzip.NewWriterLevel(&buf, gzip.BestCompression) @@ -187,3 +222,12 @@ func gzipData(data []byte) ([]byte, error) { } return buf.Bytes(), nil } + +func gunzipData(data []byte) ([]byte, error) { + r, err := gzip.NewReader(bytes.NewReader(data)) + if err != nil { + return nil, err + } + defer r.Close() + return io.ReadAll(r) +} diff --git a/internal/operator-controller/applier/secretpacker_test.go b/internal/operator-controller/applier/secretpacker_test.go index 755bc853aa..a32bc5d2b6 100644 --- a/internal/operator-controller/applier/secretpacker_test.go +++ b/internal/operator-controller/applier/secretpacker_test.go @@ -19,7 +19,6 @@ import ( func TestSecretPacker_Pack(t *testing.T) { packer := SecretPacker{ - RevisionName: "my-ext-3", OwnerName: "my-ext", SystemNamespace: "olmv1-system", } @@ -221,29 +220,27 @@ func TestSecretPacker_NameIsRevisionIndependent(t *testing.T) { }, }} - // Pack with first packer (OwnerName: "ext", RevisionName: "ext-1") + // Pack the same content twice with independent packers. packer1 := SecretPacker{ OwnerName: "ext", - RevisionName: "ext-1", SystemNamespace: "olmv1-system", } result1, err := packer1.Pack(phases) require.NoError(t, err) require.Len(t, result1.Secrets, 1) - // Pack with second packer (same OwnerName, different RevisionName) packer2 := SecretPacker{ OwnerName: "ext", - RevisionName: "ext-2", SystemNamespace: "olmv1-system", } result2, err := packer2.Pack(phases) require.NoError(t, err) require.Len(t, result2.Secrets, 1) - // Assert both Secret names are identical and begin with "ext-" + // Assert both Secret names are identical and begin with "ext-". Names are + // content-addressed, so identical content yields identical names. assert.Equal(t, result1.Secrets[0].Name, result2.Secrets[0].Name, - "Secret names should be identical regardless of RevisionName") + "Secret names should be content-addressed, not revision-dependent") assert.True(t, strings.HasPrefix(result1.Secrets[0].Name, "ext-"), "Secret name should be prefixed with OwnerName") diff --git a/internal/testutil/mock/applier/mock_applier.go b/internal/testutil/mock/applier/mock_applier.go index 5ff9b4e08c..ab386d32fc 100644 --- a/internal/testutil/mock/applier/mock_applier.go +++ b/internal/testutil/mock/applier/mock_applier.go @@ -19,6 +19,7 @@ import ( gomock "go.uber.org/mock/gomock" chart "helm.sh/helm/v3/pkg/chart" release "helm.sh/helm/v3/pkg/release" + v11 "k8s.io/api/core/v1" client "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -176,19 +177,20 @@ func (m *MockClusterObjectSetGenerator) EXPECT() *MockClusterObjectSetGeneratorM return m.recorder } -// GenerateRevision mocks base method. -func (m *MockClusterObjectSetGenerator) GenerateRevision(ctx context.Context, bundleFS fs.FS, ext *v1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*v10.ClusterObjectSetApplyConfiguration, error) { +// GenerateClusterObjectDeployment mocks base method. +func (m *MockClusterObjectSetGenerator) GenerateClusterObjectDeployment(ctx context.Context, bundleFS fs.FS, ext *v1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*v10.ClusterObjectDeploymentApplyConfiguration, []v11.Secret, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GenerateRevision", ctx, bundleFS, ext, objectLabels, revisionAnnotations) - ret0, _ := ret[0].(*v10.ClusterObjectSetApplyConfiguration) - ret1, _ := ret[1].(error) - return ret0, ret1 + ret := m.ctrl.Call(m, "GenerateClusterObjectDeployment", ctx, bundleFS, ext, objectLabels, revisionAnnotations) + ret0, _ := ret[0].(*v10.ClusterObjectDeploymentApplyConfiguration) + ret1, _ := ret[1].([]v11.Secret) + ret2, _ := ret[2].(error) + return ret0, ret1, ret2 } -// GenerateRevision indicates an expected call of GenerateRevision. -func (mr *MockClusterObjectSetGeneratorMockRecorder) GenerateRevision(ctx, bundleFS, ext, objectLabels, revisionAnnotations any) *gomock.Call { +// GenerateClusterObjectDeployment indicates an expected call of GenerateClusterObjectDeployment. +func (mr *MockClusterObjectSetGeneratorMockRecorder) GenerateClusterObjectDeployment(ctx, bundleFS, ext, objectLabels, revisionAnnotations any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GenerateRevision", reflect.TypeOf((*MockClusterObjectSetGenerator)(nil).GenerateRevision), ctx, bundleFS, ext, objectLabels, revisionAnnotations) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GenerateClusterObjectDeployment", reflect.TypeOf((*MockClusterObjectSetGenerator)(nil).GenerateClusterObjectDeployment), ctx, bundleFS, ext, objectLabels, revisionAnnotations) } // GenerateRevisionFromHelmRelease mocks base method. From 35181365620416230b67fcf0e463303a7073d260 Mon Sep 17 00:00:00 2001 From: "Per G. da Silva" Date: Thu, 22 Oct 2026 19:38:23 +0200 Subject: [PATCH 06/13] =?UTF-8?q?=F0=9F=8C=B1=20fix=20gofmt/gci=20formatti?= =?UTF-8?q?ng=20in=20COD=20test=20files?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit These pre-existing formatting issues (import ordering and map-literal alignment) failed make lint; apply the gofmt/gci auto-fixes so the lint gate passes. Formatting-only, no behavior change. Co-Authored-By: Claude Opus 4.8 Signed-off-by: Per G. da Silva --- .../operator-controller/applier/clusterobjectdeployment_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/operator-controller/applier/clusterobjectdeployment_test.go b/internal/operator-controller/applier/clusterobjectdeployment_test.go index c8e4930164..0faf868465 100644 --- a/internal/operator-controller/applier/clusterobjectdeployment_test.go +++ b/internal/operator-controller/applier/clusterobjectdeployment_test.go @@ -15,9 +15,9 @@ import ( ocv1 "github.com/operator-framework/operator-controller/api/v1" ocv1ac "github.com/operator-framework/operator-controller/applyconfigurations/api/v1" + "github.com/operator-framework/operator-controller/internal/shared/labels" bundlecsv "github.com/operator-framework/operator-controller/internal/testing/bundle/csv" bundlefs "github.com/operator-framework/operator-controller/internal/testing/bundle/fs" - "github.com/operator-framework/operator-controller/internal/shared/labels" ) // fakeManifestProvider is a test double that returns a fixed list of objects From f5ec428274b937bcdc62c790a7952558bf991024 Mon Sep 17 00:00:00 2001 From: "Per G. da Silva" Date: Thu, 22 Oct 2026 19:48:36 +0200 Subject: [PATCH 07/13] =?UTF-8?q?=E2=9C=A8=20adopt=20pre-existing=20Cluste?= =?UTF-8?q?rObjectSet=20revisions=20without=20re-rollout?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When operator-controller writes a COD for an extension that already has an equivalent ClusterObjectSet (COS) revision (matching TemplateHashKey), the COD controller now adopts the existing revision by setting the controller owner reference instead of creating a spurious new revision. This change: - Exports TemplateHash() for use in tests and external packages - Implements ensureRevisionOwnership() in the COD controller to adopt existing revisions that match the template hash but lack ownership - Adds TestBoxcutterApply_AdoptsExistingRevisionWithoutReroll to verify that adoption happens without creating a new COS revision The hash parity between the applier's COD template and the COD controller's templateHash computation already held (Tasks 1-5 produced matching hashes), but the adoption logic (setting owner reference) was missing. This implementation completes the adoption feature. Co-Authored-By: Claude Opus 4.8 Signed-off-by: Per G. da Silva --- .../clusterobjectdeployment_controller.go | 40 ++++++++++- ...jectdeployment_controller_internal_test.go | 8 +-- .../applier/boxcutter_test.go | 66 +++++++++++++++++++ 3 files changed, 107 insertions(+), 7 deletions(-) diff --git a/internal/object-controller/controllers/clusterobjectdeployment_controller.go b/internal/object-controller/controllers/clusterobjectdeployment_controller.go index 9f8b3e8299..ae4b536ec8 100644 --- a/internal/object-controller/controllers/clusterobjectdeployment_controller.go +++ b/internal/object-controller/controllers/clusterobjectdeployment_controller.go @@ -113,7 +113,7 @@ func (c *ClusterObjectDeploymentReconciler) reconcile(ctx context.Context, cod * // changed (or no active revision exists) and reflects the latest revision's // rollout progress in the deployment's Available and Progressing conditions. func (c *ClusterObjectDeploymentReconciler) reconcileRollout(ctx context.Context, cod *ocv1.ClusterObjectDeployment, revisions []ocv1.ClusterObjectSet) error { - hash := templateHash(cod.Spec.Template) + hash := TemplateHash(cod.Spec.Template) setAvailable(cod, revisions) @@ -136,6 +136,13 @@ func (c *ClusterObjectDeploymentReconciler) reconcileRollout(ctx context.Context return nil } + // Adopt the latest revision if it's not already owned by this deployment. + // This handles the case where a ClusterObjectSet with the correct template hash + // exists but doesn't have an owner reference to this ClusterObjectDeployment. + if err := c.ensureRevisionOwnership(ctx, cod, latest); err != nil { + return err + } + // A revision that has exceeded its progress deadline reports Ready=False with // reason ProgressDeadlineExceeded and never records completedAt. Surface that // terminal rollout state on the deployment's Progressing condition so it is @@ -235,6 +242,33 @@ func archivedTeardownComplete(rev *ocv1.ClusterObjectSet) bool { return cond != nil && cond.Reason == ocv1.ClusterObjectSetReasonArchived } +// ensureRevisionOwnership ensures the given revision has a controller owner +// reference pointing to the deployment. If the revision already has the correct +// owner reference, this is a no-op. +func (c *ClusterObjectDeploymentReconciler) ensureRevisionOwnership(ctx context.Context, cod *ocv1.ClusterObjectDeployment, rev *ocv1.ClusterObjectSet) error { + // Check if the revision already has the correct controller owner reference + for _, ref := range rev.OwnerReferences { + if ref.Controller != nil && *ref.Controller && + ref.UID == cod.UID { + // Already owned by this deployment + return nil + } + } + + // Set the controller owner reference + updated := rev.DeepCopy() + if err := controllerutil.SetControllerReference(cod, updated, c.Scheme); err != nil { + return fmt.Errorf("setting controller reference on revision %q: %w", rev.Name, err) + } + + // Update the revision + if err := c.Client.Update(ctx, updated); err != nil { + return fmt.Errorf("adopting revision %q: %w", rev.Name, err) + } + + return nil +} + // createRevision builds and creates the next ClusterObjectSet revision owned by // the deployment. Any create error, including AlreadyExists, is returned. // @@ -332,13 +366,13 @@ func mapClusterObjectSetToClusterObjectDeployment(_ context.Context, obj client. return []ctrl.Request{{NamespacedName: types.NamespacedName{Name: cos.Spec.Group}}} } -// templateHash returns a stable digest of the deployment template. Any change to +// TemplateHash returns a stable digest of the deployment template. Any change to // the template (including changes to fields that are mutable on a ClusterObjectSet) // produces a different digest, which is how the controller decides a new revision // is required. The shared DeepHashObject helper base36-encodes a sha224 of the // template's JSON, which is label-safe and short enough to use as a label value // without truncation. -func templateHash(template ocv1.ClusterObjectDeploymentTemplate) string { +func TemplateHash(template ocv1.ClusterObjectDeploymentTemplate) string { return hashutil.DeepHashObject(template) } diff --git a/internal/object-controller/controllers/clusterobjectdeployment_controller_internal_test.go b/internal/object-controller/controllers/clusterobjectdeployment_controller_internal_test.go index a47a384b9f..90910bc116 100644 --- a/internal/object-controller/controllers/clusterobjectdeployment_controller_internal_test.go +++ b/internal/object-controller/controllers/clusterobjectdeployment_controller_internal_test.go @@ -42,18 +42,18 @@ func testTemplate() ocv1.ClusterObjectDeploymentTemplate { func Test_templateHash_isStableAndSensitive(t *testing.T) { tmpl := testTemplate() - h1 := templateHash(tmpl) + h1 := TemplateHash(tmpl) require.NotEmpty(t, h1) require.LessOrEqual(t, len(h1), 63, "digest must fit within the label-value length limit") require.Regexp(t, "^[0-9a-z]+$", h1, "digest must be label-value safe") - h2 := templateHash(tmpl) + h2 := TemplateHash(tmpl) require.Equal(t, h1, h2, "same template must hash the same") // A change to a phase object changes the hash. changed := testTemplate() changed.Spec.Phases[0].Objects[0].Object.Object["data"] = map[string]interface{}{"k": "v"} - require.NotEqual(t, h1, templateHash(changed)) + require.NotEqual(t, h1, TemplateHash(changed)) // A change to ONLY progressionProbes (a mutable COS field) still changes the hash. probesChanged := testTemplate() @@ -61,7 +61,7 @@ func Test_templateHash_isStableAndSensitive(t *testing.T) { Selector: ocv1.ObjectSelector{Type: ocv1.SelectorTypeGroupKind, GroupKind: metav1.GroupKind{Kind: "ConfigMap"}}, Assertions: []ocv1.Assertion{{Type: ocv1.ProbeTypeFieldValue}}, }} - require.NotEqual(t, h1, templateHash(probesChanged), "a progressionProbes-only change must create a new revision") + require.NotEqual(t, h1, TemplateHash(probesChanged), "a progressionProbes-only change must create a new revision") } func Test_nextRevisionNumber(t *testing.T) { diff --git a/internal/operator-controller/applier/boxcutter_test.go b/internal/operator-controller/applier/boxcutter_test.go index 01764667fd..81f44e995a 100644 --- a/internal/operator-controller/applier/boxcutter_test.go +++ b/internal/operator-controller/applier/boxcutter_test.go @@ -15,6 +15,7 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/utils/ptr" + ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" @@ -893,3 +894,68 @@ func TestBoxcutterApply_UnchangedIsNoop(t *testing.T) { t.Log("by checking applying identical content twice does not bump the COD generation") assert.Equal(t, first.Generation, second.Generation) } + +func TestBoxcutterApply_AdoptsExistingRevisionWithoutReroll(t *testing.T) { + scheme := codTestScheme(t) + ext := &ocv1.ClusterExtension{ + ObjectMeta: metav1.ObjectMeta{Name: "test-ext", UID: "test-uid"}, + } + cl := testutil.WithIndexes(t, fake.NewClientBuilder().WithScheme(scheme), coscontrollers.SetupIndexes). + WithStatusSubresource(&ocv1.ClusterObjectSet{}, &ocv1.ClusterObjectDeployment{}). + Build() + bc := newCODBoxcutter(t, scheme, cl, []client.Object{ + &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "test-cm", Namespace: "test-ns"}}, + }) + + t.Log("by applying the COD for the first time to get the template") + _, _, err := bc.Apply(t.Context(), dummyBundle, ext, nil, map[string]string{}) + require.NoError(t, err) + + t.Log("by reading the COD to extract its template") + cod := &ocv1.ClusterObjectDeployment{} + require.NoError(t, cl.Get(t.Context(), client.ObjectKey{Name: ext.Name}, cod)) + require.NotNil(t, cod.Spec.Template) + + t.Log("by computing the template hash that the COD controller would compute") + templateHash := coscontrollers.TemplateHash(cod.Spec.Template) + + t.Log("by creating a pre-existing COS in the same group with the matching template hash") + existingCOS := &ocv1.ClusterObjectSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: ext.Name + "-1", + Labels: map[string]string{ + labels.TemplateHashKey: templateHash, + }, + }, + Spec: ocv1.ClusterObjectSetSpec{ + Group: ext.Name, + Revision: 1, + LifecycleState: ocv1.ClusterObjectSetLifecycleStateActive, + Phases: cod.Spec.Template.Spec.Phases, + }, + } + require.NoError(t, cl.Create(t.Context(), existingCOS)) + + t.Log("by reconciling the COD controller") + reconciler := &coscontrollers.ClusterObjectDeploymentReconciler{ + Client: cl, + Scheme: scheme, + } + _, err = reconciler.Reconcile(t.Context(), ctrl.Request{NamespacedName: client.ObjectKey{Name: ext.Name}}) + require.NoError(t, err) + + t.Log("by verifying no new revision was created") + cosList := &ocv1.ClusterObjectSetList{} + require.NoError(t, cl.List(t.Context(), cosList)) + assert.Len(t, cosList.Items, 1, "should have exactly one COS (the pre-existing one)") + assert.Equal(t, existingCOS.Name, cosList.Items[0].Name) + + t.Log("by verifying the existing COS was adopted (has controller owner ref pointing to the COD)") + adoptedCOS := &ocv1.ClusterObjectSet{} + require.NoError(t, cl.Get(t.Context(), client.ObjectKeyFromObject(existingCOS), adoptedCOS)) + require.Len(t, adoptedCOS.OwnerReferences, 1) + assert.Equal(t, cod.Name, adoptedCOS.OwnerReferences[0].Name) + assert.Equal(t, cod.UID, adoptedCOS.OwnerReferences[0].UID) + assert.Equal(t, ocv1.ClusterObjectDeploymentKind, adoptedCOS.OwnerReferences[0].Kind) + assert.Equal(t, ptr.To(true), adoptedCOS.OwnerReferences[0].Controller) +} From 00a07db0c9f0fe86a11ea4561f4dd2874a5f473a Mon Sep 17 00:00:00 2001 From: "Per G. da Silva" Date: Thu, 22 Oct 2026 19:56:29 +0200 Subject: [PATCH 08/13] =?UTF-8?q?=F0=9F=8C=B1=20add=20defensive=20group-sc?= =?UTF-8?q?ope=20guard=20and=20improve=20error=20message=20in=20ensureRevi?= =?UTF-8?q?sionOwnership?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address code-quality feedback on the adoption feature: 1. Add defensive group-scope guard: ensureRevisionOwnership now validates that rev.Spec.Group == cod.Name before attempting adoption, returning a clear error if they mismatch. This prevents adopting a revision from another deployment's group. 2. Improve error message when SetControllerReference fails: now includes a hint that the failure may be due to the revision already being owned by another controller. 3. Add comprehensive unit tests (Test_ensureRevisionOwnership) covering: - Rejection of revision with mismatched group (defensive guard) - Successful adoption of revision with matching group - No-op when revision is already owned Co-Authored-By: Claude Opus 4.8 Signed-off-by: Per G. da Silva --- .../clusterobjectdeployment_controller.go | 8 +- ...jectdeployment_controller_internal_test.go | 96 +++++++++++++++++++ 2 files changed, 103 insertions(+), 1 deletion(-) diff --git a/internal/object-controller/controllers/clusterobjectdeployment_controller.go b/internal/object-controller/controllers/clusterobjectdeployment_controller.go index ae4b536ec8..c64dd33ea4 100644 --- a/internal/object-controller/controllers/clusterobjectdeployment_controller.go +++ b/internal/object-controller/controllers/clusterobjectdeployment_controller.go @@ -246,6 +246,12 @@ func archivedTeardownComplete(rev *ocv1.ClusterObjectSet) bool { // reference pointing to the deployment. If the revision already has the correct // owner reference, this is a no-op. func (c *ClusterObjectDeploymentReconciler) ensureRevisionOwnership(ctx context.Context, cod *ocv1.ClusterObjectDeployment, rev *ocv1.ClusterObjectSet) error { + // Defensive guard: only adopt revisions that belong to this deployment's group. + // The group is the deployment name, and siblinghood is defined by spec.group. + if rev.Spec.Group != cod.Name { + return fmt.Errorf("cannot adopt revision %q: spec.group %q does not match deployment name %q", rev.Name, rev.Spec.Group, cod.Name) + } + // Check if the revision already has the correct controller owner reference for _, ref := range rev.OwnerReferences { if ref.Controller != nil && *ref.Controller && @@ -258,7 +264,7 @@ func (c *ClusterObjectDeploymentReconciler) ensureRevisionOwnership(ctx context. // Set the controller owner reference updated := rev.DeepCopy() if err := controllerutil.SetControllerReference(cod, updated, c.Scheme); err != nil { - return fmt.Errorf("setting controller reference on revision %q: %w", rev.Name, err) + return fmt.Errorf("cannot adopt revision %q: %w (may already be owned by another controller)", rev.Name, err) } // Update the revision diff --git a/internal/object-controller/controllers/clusterobjectdeployment_controller_internal_test.go b/internal/object-controller/controllers/clusterobjectdeployment_controller_internal_test.go index 90910bc116..59fb8064aa 100644 --- a/internal/object-controller/controllers/clusterobjectdeployment_controller_internal_test.go +++ b/internal/object-controller/controllers/clusterobjectdeployment_controller_internal_test.go @@ -11,12 +11,16 @@ import ( apimeta "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" + "k8s.io/utils/ptr" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" ocv1 "github.com/operator-framework/operator-controller/api/v1" "github.com/operator-framework/operator-controller/internal/shared/labels" + "github.com/operator-framework/operator-controller/internal/testutil" ) func testTemplate() ocv1.ClusterObjectDeploymentTemplate { @@ -212,3 +216,95 @@ func Test_mapClusterObjectSetToClusterObjectDeployment(t *testing.T) { }) } } + +func Test_ensureRevisionOwnership(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, ocv1.AddToScheme(scheme)) + + cod := &ocv1.ClusterObjectDeployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cod", + UID: "cod-uid", + }, + Spec: ocv1.ClusterObjectDeploymentSpec{Template: testTemplate()}, + } + + newClient := func(t *testing.T, objs ...client.Object) client.Client { + t.Helper() + return testutil.WithIndexes(t, fake.NewClientBuilder().WithScheme(scheme), SetupIndexes). + WithStatusSubresource(&ocv1.ClusterObjectDeployment{}, &ocv1.ClusterObjectSet{}). + WithObjects(objs...). + Build() + } + + t.Run("rejects revision with mismatched group", func(t *testing.T) { + // A revision from a different group must not be adopted. + rev := &ocv1.ClusterObjectSet{ + ObjectMeta: metav1.ObjectMeta{Name: "other-group-1"}, + Spec: ocv1.ClusterObjectSetSpec{Group: "other-group", Revision: 1}, + } + c := newClient(t, cod, rev) + reconciler := &ClusterObjectDeploymentReconciler{Client: c, Scheme: scheme} + + err := reconciler.ensureRevisionOwnership(t.Context(), cod, rev) + require.Error(t, err) + require.Contains(t, err.Error(), "cannot adopt revision") + require.Contains(t, err.Error(), "spec.group \"other-group\" does not match deployment name \"test-cod\"") + + // Verify the revision was NOT adopted (no owner reference added) + updated := &ocv1.ClusterObjectSet{} + require.NoError(t, c.Get(t.Context(), client.ObjectKeyFromObject(rev), updated)) + require.Empty(t, updated.OwnerReferences) + }) + + t.Run("adopts revision with matching group", func(t *testing.T) { + // A revision in the same group can be adopted. + rev := &ocv1.ClusterObjectSet{ + ObjectMeta: metav1.ObjectMeta{Name: "test-cod-1"}, + Spec: ocv1.ClusterObjectSetSpec{Group: "test-cod", Revision: 1}, + } + c := newClient(t, cod, rev) + reconciler := &ClusterObjectDeploymentReconciler{Client: c, Scheme: scheme} + + err := reconciler.ensureRevisionOwnership(t.Context(), cod, rev) + require.NoError(t, err) + + // Verify the revision was adopted (owner reference set) + updated := &ocv1.ClusterObjectSet{} + require.NoError(t, c.Get(t.Context(), client.ObjectKeyFromObject(rev), updated)) + require.Len(t, updated.OwnerReferences, 1) + require.Equal(t, cod.Name, updated.OwnerReferences[0].Name) + require.Equal(t, cod.UID, updated.OwnerReferences[0].UID) + require.Equal(t, ptr.To(true), updated.OwnerReferences[0].Controller) + }) + + t.Run("no-op when already owned", func(t *testing.T) { + // A revision already owned by the deployment should not be updated. + rev := &ocv1.ClusterObjectSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cod-1", + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: ocv1.GroupVersion.String(), + Kind: "ClusterObjectDeployment", + Name: cod.Name, + UID: cod.UID, + Controller: ptr.To(true), + }, + }, + }, + Spec: ocv1.ClusterObjectSetSpec{Group: "test-cod", Revision: 1}, + } + c := newClient(t, cod, rev) + reconciler := &ClusterObjectDeploymentReconciler{Client: c, Scheme: scheme} + + err := reconciler.ensureRevisionOwnership(t.Context(), cod, rev) + require.NoError(t, err) + + // Verify the revision still has the same owner reference (no update) + updated := &ocv1.ClusterObjectSet{} + require.NoError(t, c.Get(t.Context(), client.ObjectKeyFromObject(rev), updated)) + require.Len(t, updated.OwnerReferences, 1) + require.Equal(t, cod.UID, updated.OwnerReferences[0].UID) + }) +} From 602a573b1d6782031c62b97656f4807a99aea310 Mon Sep 17 00:00:00 2001 From: "Per G. da Silva" Date: Thu, 22 Oct 2026 20:04:29 +0200 Subject: [PATCH 09/13] =?UTF-8?q?=E2=9C=A8=20grant=20operator-controller?= =?UTF-8?q?=20RBAC=20for=20ClusterObjectDeployments?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add kubebuilder RBAC markers to the ClusterExtension reconciler to grant permissions for ClusterObjectDeployments, ClusterObjectSets, and Secrets. This enables the operator-controller to create and manage ClusterObjectDeployments and their referenced Secrets using the boxcutter applier. Co-Authored-By: Claude Opus 4.8 Signed-off-by: Per G. da Silva --- .../controllers/clusterextension_controller.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/internal/operator-controller/controllers/clusterextension_controller.go b/internal/operator-controller/controllers/clusterextension_controller.go index 25a071b29a..efa8386c62 100644 --- a/internal/operator-controller/controllers/clusterextension_controller.go +++ b/internal/operator-controller/controllers/clusterextension_controller.go @@ -122,6 +122,11 @@ type RevisionStatesGetter interface { // The operator controller needs to watch all the bundle objects and reconcile accordingly. Though not ideal, but these permissions are required. // This has been taken from rukpak, and an issue was created before to discuss it: https://github.com/operator-framework/rukpak/issues/800. +// +//+kubebuilder:rbac:groups=olm.operatorframework.io,resources=clusterobjectdeployments,verbs=get;list;watch;create;update;patch;delete +//+kubebuilder:rbac:groups=olm.operatorframework.io,resources=clusterobjectsets,verbs=get;list;watch;create;update;patch;delete +//+kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;watch;create + func (r *ClusterExtensionReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { l := log.FromContext(ctx).WithName("cluster-extension") ctx = log.IntoContext(ctx, l) From 29d04d8de5ebfda3202d85ad8c3b279c98c4dd46 Mon Sep 17 00:00:00 2001 From: "Per G. da Silva" Date: Thu, 22 Oct 2026 20:11:25 +0200 Subject: [PATCH 10/13] =?UTF-8?q?=E2=9C=A8=20add=20e2e=20for=20ClusterExte?= =?UTF-8?q?nsion=20rollout=20via=20ClusterObjectDeployment?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds cod_extension_rollout.feature (@BoxcutterRuntime) proving a ClusterExtension installs via a ClusterObjectDeployment, which creates ClusterObjectSet revisions for the bundle. The scenario verifies the COD exists, the COS becomes Ready, and the ClusterExtension reports Installed=True with the expected bundle. Uses existing catalog "test" package "test" 1.2.0 and existing e2e steps (no new step implementation required). Co-Authored-By: Claude Opus 4.8 Signed-off-by: Per G. da Silva --- .../features/cod_extension_rollout.feature | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) create mode 100644 test/e2e/features/cod_extension_rollout.feature diff --git a/test/e2e/features/cod_extension_rollout.feature b/test/e2e/features/cod_extension_rollout.feature new file mode 100644 index 0000000000..32437f79a3 --- /dev/null +++ b/test/e2e/features/cod_extension_rollout.feature @@ -0,0 +1,38 @@ +@BoxcutterRuntime +Feature: ClusterExtension rollout via ClusterObjectDeployment + + As an OLM user installing a ClusterExtension, I want the operator-controller to + create a ClusterObjectDeployment that manages the rollout lifecycle, creating + ClusterObjectSet revisions for the installed bundle, so that extensions benefit + from the full COD/COS revision management system. + + Background: + Given OLM is available + And an image registry is available + And namespace "${TEST_NAMESPACE}" is available + + Scenario: ClusterExtension creates a ClusterObjectDeployment and installs via ClusterObjectSet + Given a catalog "test" with packages: + | package | version | channel | replaces | contents | + | test | 1.2.0 | beta | | CRD, Deployment, ConfigMap | + When ClusterExtension is applied + """ + apiVersion: olm.operatorframework.io/v1 + kind: ClusterExtension + metadata: + name: ${NAME} + spec: + namespace: ${TEST_NAMESPACE} + source: + sourceType: Catalog + catalog: + packageName: ${PACKAGE:test} + selector: + matchLabels: + "olm.operatorframework.io/metadata.name": ${CATALOG:test} + """ + Then resource "clusterobjectdeployment/${NAME}" is available + And ClusterObjectSet "${NAME}-1" reports Ready as True with Reason AllObjectsReady + And ClusterExtension is available + And ClusterExtension reports Installed as True + And bundle "${PACKAGE:test}.1.2.0" is installed in version "1.2.0" From 84fa50437c0a261bbe680dd6dc2b177236899aeb Mon Sep 17 00:00:00 2001 From: "Per G. da Silva" Date: Thu, 22 Oct 2026 20:28:52 +0200 Subject: [PATCH 11/13] =?UTF-8?q?=F0=9F=90=9B=20propagate=20olm.properties?= =?UTF-8?q?=20annotation=20from=20COD=20to=20COS?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Before the COD refactor, SimpleRevisionGenerator placed the olm.properties bundle annotation on ClusterObjectSet revisions. After the refactor, the generator sets it on ClusterObjectDeployment metadata, but the COD controller's annotation propagation list did not include it, causing a regression: external consumers (e.g., maxOpenShiftVersion tooling) that read olm.properties from COS revisions no longer found it. Restore parity by adding a shared OLMPropertiesKey constant in internal/shared/labels and including it in the COD controller's bundleAnnotationKeys slice. The applier now references the shared constant, and source.PropertyOLMProperties references it to prevent drift. The propagation test now verifies olm.properties reaches the COS. Co-Authored-By: Claude Opus 4.8 Signed-off-by: Per G. da Silva --- .../controllers/clusterobjectdeployment_controller.go | 1 + .../controllers/clusterobjectdeployment_controller_test.go | 2 ++ .../operator-controller/applier/clusterobjectdeployment.go | 3 +-- internal/operator-controller/rukpak/bundle/source/source.go | 5 ++++- internal/shared/labels/labels.go | 6 ++++++ 5 files changed, 14 insertions(+), 3 deletions(-) diff --git a/internal/object-controller/controllers/clusterobjectdeployment_controller.go b/internal/object-controller/controllers/clusterobjectdeployment_controller.go index c64dd33ea4..6c2c2f0d9a 100644 --- a/internal/object-controller/controllers/clusterobjectdeployment_controller.go +++ b/internal/object-controller/controllers/clusterobjectdeployment_controller.go @@ -46,6 +46,7 @@ var bundleAnnotationKeys = []string{ labels.BundleVersionKey, labels.BundleReleaseKey, labels.BundleReferenceKey, + labels.OLMPropertiesKey, } // ClusterObjectDeploymentReconciler reconciles ClusterObjectDeployment objects by diff --git a/internal/object-controller/controllers/clusterobjectdeployment_controller_test.go b/internal/object-controller/controllers/clusterobjectdeployment_controller_test.go index 737bcbfcd9..03b38fcf85 100644 --- a/internal/object-controller/controllers/clusterobjectdeployment_controller_test.go +++ b/internal/object-controller/controllers/clusterobjectdeployment_controller_test.go @@ -840,6 +840,7 @@ func Test_ClusterObjectDeploymentReconciler_PropagatesBundleAnnotations(t *testi labels.BundleVersionKey: "1.0.0+2", labels.BundleReferenceKey: "registry.example.com/bundle@sha256:abcdef", labels.BundleReleaseKey: "2", + labels.OLMPropertiesKey: `[{"type":"olm.package","value":{"packageName":"test-package","version":"1.0.0"}}]`, "example.com/non-bundle": "should-not-be-copied", } @@ -857,6 +858,7 @@ func Test_ClusterObjectDeploymentReconciler_PropagatesBundleAnnotations(t *testi require.Equal(t, "1.0.0+2", cos.Annotations[labels.BundleVersionKey], "BundleVersionKey should be copied") require.Equal(t, "registry.example.com/bundle@sha256:abcdef", cos.Annotations[labels.BundleReferenceKey], "BundleReferenceKey should be copied") require.Equal(t, "2", cos.Annotations[labels.BundleReleaseKey], "BundleReleaseKey should be copied") + require.JSONEq(t, `[{"type":"olm.package","value":{"packageName":"test-package","version":"1.0.0"}}]`, cos.Annotations[labels.OLMPropertiesKey], "OLMPropertiesKey should be copied") // Non-bundle annotation should NOT be copied _, exists := cos.Annotations["example.com/non-bundle"] diff --git a/internal/operator-controller/applier/clusterobjectdeployment.go b/internal/operator-controller/applier/clusterobjectdeployment.go index 5fb1bbeba7..b144d283b7 100644 --- a/internal/operator-controller/applier/clusterobjectdeployment.go +++ b/internal/operator-controller/applier/clusterobjectdeployment.go @@ -9,7 +9,6 @@ import ( ocv1 "github.com/operator-framework/operator-controller/api/v1" ocv1ac "github.com/operator-framework/operator-controller/applyconfigurations/api/v1" - "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/bundle/source" "github.com/operator-framework/operator-controller/internal/shared/labels" ) @@ -99,7 +98,7 @@ func (r *SimpleRevisionGenerator) GenerateClusterObjectDeployment( } // Add bundle properties annotations for _, key := range []string{ - source.PropertyOLMProperties, + labels.OLMPropertiesKey, } { if value, ok := bundleAnnotations[key]; ok { codAnnotations[key] = value diff --git a/internal/operator-controller/rukpak/bundle/source/source.go b/internal/operator-controller/rukpak/bundle/source/source.go index 49ed0c32bf..dc9150ad84 100644 --- a/internal/operator-controller/rukpak/bundle/source/source.go +++ b/internal/operator-controller/rukpak/bundle/source/source.go @@ -18,10 +18,13 @@ import ( "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/bundle" registry "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/operator-registry" + "github.com/operator-framework/operator-controller/internal/shared/labels" ) const ( - PropertyOLMProperties = "olm.properties" + // PropertyOLMProperties is the annotation key for bundle OLM properties. + // This references the shared constant to ensure consistency across components. + PropertyOLMProperties = labels.OLMPropertiesKey ) type BundleSource interface { diff --git a/internal/shared/labels/labels.go b/internal/shared/labels/labels.go index 05420566ae..aaa9334e68 100644 --- a/internal/shared/labels/labels.go +++ b/internal/shared/labels/labels.go @@ -59,4 +59,10 @@ const ( // that were created during migration from Helm releases. This label is used // to distinguish migrated revisions from those created by normal Boxcutter operation. MigratedFromHelmKey = "olm.operatorframework.io/migrated-from-helm" + + // OLMPropertiesKey is the annotation key used to store bundle OLM properties + // (from metadata/properties.yaml) as a JSON array of property.Property objects. + // It is propagated from bundle metadata to ClusterObjectDeployments and their + // ClusterObjectSet revisions. + OLMPropertiesKey = "olm.properties" ) From 140d1fdb78dcbae392f3d8cb32036cd6645d2a24 Mon Sep 17 00:00:00 2001 From: "Per G. da Silva" Date: Fri, 23 Oct 2026 09:49:23 +0200 Subject: [PATCH 12/13] =?UTF-8?q?=F0=9F=8C=B1=20reconcile=20ClusterObjectD?= =?UTF-8?q?eployment=20adoption=20with=20canonical=20revision=20names=20(#?= =?UTF-8?q?3041)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rebasing onto main (which merged #3041) surfaced two integration points: - Remove the obsolete applier-level revision-bounds test. #3041 added TestBoxcutter_Apply_RevisionBounds, which exercises the applier's createExternalizedRevision / ClusterObjectSetRetentionLimit path that this branch removes. The equivalent revision-limit enforcement and its test now live in the ClusterObjectDeployment controller (Test_ClusterObjectDeploymentReconciler_RevisionBounds). - Make ensureRevisionOwnership tolerant of foreign-owned revisions. #3041's "unchanged template at maximum still reconciles" case models a ClusterObjectSet in the group controlled by another deployment and expects reconcile to be a no-op, not an error. Adoption now claims only true orphans (no controller owner) and leaves a revision controlled by a different controller untouched rather than erroring, so a foreign-owned sibling cannot wedge reconciliation. Co-Authored-By: Claude Opus 4.8 Signed-off-by: Per G. da Silva --- .../clusterobjectdeployment_controller.go | 26 +- .../applier/boxcutter_revision_bounds_test.go | 260 ------------------ 2 files changed, 17 insertions(+), 269 deletions(-) delete mode 100644 internal/operator-controller/applier/boxcutter_revision_bounds_test.go diff --git a/internal/object-controller/controllers/clusterobjectdeployment_controller.go b/internal/object-controller/controllers/clusterobjectdeployment_controller.go index 6c2c2f0d9a..42381fa5e3 100644 --- a/internal/object-controller/controllers/clusterobjectdeployment_controller.go +++ b/internal/object-controller/controllers/clusterobjectdeployment_controller.go @@ -243,9 +243,11 @@ func archivedTeardownComplete(rev *ocv1.ClusterObjectSet) bool { return cond != nil && cond.Reason == ocv1.ClusterObjectSetReasonArchived } -// ensureRevisionOwnership ensures the given revision has a controller owner -// reference pointing to the deployment. If the revision already has the correct -// owner reference, this is a no-op. +// ensureRevisionOwnership adopts an orphan revision (one with no controller owner) +// by setting this deployment as its controller. It is a no-op if the revision is +// already owned by this deployment, or if it is already controlled by a different +// controller — a foreign-controlled revision sharing this group is tolerated, not +// stolen and not treated as an error. func (c *ClusterObjectDeploymentReconciler) ensureRevisionOwnership(ctx context.Context, cod *ocv1.ClusterObjectDeployment, rev *ocv1.ClusterObjectSet) error { // Defensive guard: only adopt revisions that belong to this deployment's group. // The group is the deployment name, and siblinghood is defined by spec.group. @@ -253,19 +255,25 @@ func (c *ClusterObjectDeploymentReconciler) ensureRevisionOwnership(ctx context. return fmt.Errorf("cannot adopt revision %q: spec.group %q does not match deployment name %q", rev.Name, rev.Spec.Group, cod.Name) } - // Check if the revision already has the correct controller owner reference + // Inspect any existing controller owner reference. for _, ref := range rev.OwnerReferences { - if ref.Controller != nil && *ref.Controller && - ref.UID == cod.UID { - // Already owned by this deployment + if ref.Controller != nil && *ref.Controller { + if ref.UID == cod.UID { + // Already owned by this deployment. + return nil + } + // Owned by a different controller. Siblinghood is defined by spec.group, + // so a foreign-controlled revision may legitimately share this group; we + // do not steal it, and we do not treat it as an error (that would wedge + // reconciliation). Leave it untouched. return nil } } - // Set the controller owner reference + // No controller owner: adopt this orphan revision. updated := rev.DeepCopy() if err := controllerutil.SetControllerReference(cod, updated, c.Scheme); err != nil { - return fmt.Errorf("cannot adopt revision %q: %w (may already be owned by another controller)", rev.Name, err) + return fmt.Errorf("adopting orphan revision %q: %w", rev.Name, err) } // Update the revision diff --git a/internal/operator-controller/applier/boxcutter_revision_bounds_test.go b/internal/operator-controller/applier/boxcutter_revision_bounds_test.go deleted file mode 100644 index 3992cd2706..0000000000 --- a/internal/operator-controller/applier/boxcutter_revision_bounds_test.go +++ /dev/null @@ -1,260 +0,0 @@ -package applier_test - -import ( - "context" - "fmt" - "math" - "strings" - "testing" - "testing/fstest" - - "github.com/stretchr/testify/require" - "go.uber.org/mock/gomock" - corev1 "k8s.io/api/core/v1" - apierrors "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/util/validation/field" - "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/client/fake" - "sigs.k8s.io/controller-runtime/pkg/client/interceptor" - - ocv1 "github.com/operator-framework/operator-controller/api/v1" - ocv1ac "github.com/operator-framework/operator-controller/applyconfigurations/api/v1" - coscontrollers "github.com/operator-framework/operator-controller/internal/object-controller/controllers" - "github.com/operator-framework/operator-controller/internal/operator-controller/applier" - "github.com/operator-framework/operator-controller/internal/testutil" - mockapplier "github.com/operator-framework/operator-controller/internal/testutil/mock/applier" -) - -func TestBoxcutter_Apply_RevisionBounds(t *testing.T) { - for _, tc := range []struct { - name string - latest int64 - changed bool - exhausted bool - }{ - { - name: "allocates final allowed revision across owners", - latest: ocv1.ClusterObjectSetMaxRevision - 1, - changed: true, - exhausted: false, - }, - { - name: "exhaustion preserves history and creates nothing", - latest: ocv1.ClusterObjectSetMaxRevision, - changed: true, - exhausted: true, - }, - { - name: "unchanged content at maximum still reconciles", - latest: ocv1.ClusterObjectSetMaxRevision, - changed: false, - exhausted: false, - }, - { - name: "out of range stored revision cannot overflow", - latest: math.MaxInt64, - changed: true, - exhausted: true, - }, - } { - t.Run(tc.name, func(t *testing.T) { - fixture := newRevisionBoundsFixture(t, tc.latest, tc.changed) - - completed, status, err := fixture.applier.Apply(t.Context(), fstest.MapFS{}, fixture.extension, nil, nil) - - require.Empty(t, status) - require.Equal(t, 1, fixture.requests.comparisons) - if tc.exhausted { - require.ErrorContains(t, err, fmt.Sprintf("revision limit %d reached for group %q", ocv1.ClusterObjectSetMaxRevision, fixture.extension.Name)) - require.False(t, completed) - } else { - require.NoError(t, err) - require.True(t, completed) - } - - if tc.exhausted || !tc.changed { - require.Zero(t, fixture.requests.newApplies) - require.Zero(t, fixture.requests.creates) - require.Zero(t, fixture.requests.deletes) - require.Zero(t, fixture.requests.patches) - for _, obj := range fixture.seededObjects { - retained := obj.DeepCopyObject().(client.Object) - require.NoError(t, fixture.client.Get(t.Context(), client.ObjectKeyFromObject(obj), retained)) - } - return - } - - require.Equal(t, 1, fixture.requests.newApplies) - require.Positive(t, fixture.requests.creates, "new revision content must be externalized") - var created ocv1.ClusterObjectSet - key := client.ObjectKey{ - Name: fmt.Sprintf("%s-%d", fixture.extension.Name, ocv1.ClusterObjectSetMaxRevision), - } - require.NoError(t, fixture.client.Get(t.Context(), key, &created)) - require.Len(t, created.Name, 63) - require.Equal(t, ocv1.ClusterObjectSetMaxRevision, created.Spec.Revision) - require.Equal(t, fixture.extension.Name, created.Spec.Group) - }) - } -} - -type revisionBoundsFixture struct { - extension *ocv1.ClusterExtension - applier *applier.Boxcutter - client client.Client - seededObjects []client.Object - requests revisionBoundsRequests -} - -type revisionBoundsRequests struct { - comparisons int - newApplies int - creates int - deletes int - patches int -} - -// newRevisionBoundsFixture seeds history and externalized content, and models -// API immutability rejection when changed content requires another revision. -func newRevisionBoundsFixture(t *testing.T, latestRevision int64, changed bool) *revisionBoundsFixture { - t.Helper() - scheme := runtime.NewScheme() - require.NoError(t, ocv1.AddToScheme(scheme)) - require.NoError(t, corev1.AddToScheme(scheme)) - fixture := &revisionBoundsFixture{ - extension: &ocv1.ClusterExtension{ - ObjectMeta: metav1.ObjectMeta{ - Name: strings.Repeat("a", 52), - UID: "extension-uid", - }, - }, - } - ext := fixture.extension - owner := *metav1.NewControllerRef(ext, ocv1.GroupVersion.WithKind(ocv1.ClusterExtensionKind)) - // Enough archived revisions to trigger retention if the guard runs too late. - for revision := int64(1); revision <= int64(applier.ClusterObjectSetRetentionLimit)+1; revision++ { - fixture.seededObjects = append(fixture.seededObjects, &ocv1.ClusterObjectSet{ - ObjectMeta: metav1.ObjectMeta{ - Name: fmt.Sprintf("%s-%d", ext.Name, revision), - OwnerReferences: []metav1.OwnerReference{owner}, - }, - Spec: ocv1.ClusterObjectSetSpec{ - Group: ext.Name, - Revision: revision, - LifecycleState: ocv1.ClusterObjectSetLifecycleStateArchived, - CollisionProtection: ocv1.CollisionProtectionPrevent, - }, - }) - } - latest := &ocv1.ClusterObjectSet{ - ObjectMeta: metav1.ObjectMeta{ - Name: fmt.Sprintf("%s-%d", ext.Name, latestRevision), - UID: "latest-uid", - }, - Spec: ocv1.ClusterObjectSetSpec{ - Group: ext.Name, - Revision: latestRevision, - LifecycleState: ocv1.ClusterObjectSetLifecycleStateActive, - CollisionProtection: ocv1.CollisionProtectionPrevent, - }, - } - // The latest revision is deliberately owned by someone else (or unowned). - // Neither group-wide numbering nor exhaustion may depend on CE ownership. - if changed { - foreignOwner := owner - foreignOwner.Name = "other-extension" - foreignOwner.UID = "other-uid" - latest.OwnerReferences = []metav1.OwnerReference{foreignOwner} - } - payload := unstructured.Unstructured{ - Object: map[string]any{ - "apiVersion": "v1", - "kind": "ConfigMap", - "metadata": map[string]any{ - "name": "payload", - }, - }, - } - packer := applier.SecretPacker{ - RevisionName: latest.Name, - OwnerName: ext.Name, - SystemNamespace: "olmv1-system", - } - pack, err := packer.Pack([]ocv1.ClusterObjectSetPhase{ - { - Name: "deploy", - Objects: []ocv1.ClusterObjectSetObject{ - { - Object: payload, - }, - }, - }, - }) - require.NoError(t, err) - latest.Spec.Phases = []ocv1.ClusterObjectSetPhase{ - { - Name: "deploy", - Objects: []ocv1.ClusterObjectSetObject{ - { - Ref: pack.Refs[[2]int{0, 0}], - }, - }, - }, - } - fixture.seededObjects = append(fixture.seededObjects, latest) - for i := range pack.Secrets { - pack.Secrets[i].OwnerReferences = []metav1.OwnerReference{ - *metav1.NewControllerRef(latest, ocv1.GroupVersion.WithKind(ocv1.ClusterObjectSetKind)), - } - fixture.seededObjects = append(fixture.seededObjects, &pack.Secrets[i]) - } - - fixture.client = testutil.WithIndexes(t, fake.NewClientBuilder().WithScheme(scheme), coscontrollers.SetupIndexes). - WithObjects(fixture.seededObjects...).WithInterceptorFuncs(interceptor.Funcs{ - Apply: func(ctx context.Context, cl client.WithWatch, obj runtime.ApplyConfiguration, opts ...client.ApplyOption) error { - cos := obj.(*ocv1ac.ClusterObjectSetApplyConfiguration) - if *cos.Name == latest.Name { - fixture.requests.comparisons++ - if changed { - return apierrors.NewInvalid(ocv1.GroupVersion.WithKind(ocv1.ClusterObjectSetKind).GroupKind(), latest.Name, - field.ErrorList{field.Invalid(field.NewPath("spec", "phases"), "changed", "phases is immutable")}) - } - } else { - fixture.requests.newApplies++ - } - return cl.Apply(ctx, obj, opts...) - }, - Create: func(ctx context.Context, cl client.WithWatch, obj client.Object, opts ...client.CreateOption) error { - fixture.requests.creates++ - return cl.Create(ctx, obj, opts...) - }, - Delete: func(ctx context.Context, cl client.WithWatch, obj client.Object, opts ...client.DeleteOption) error { - fixture.requests.deletes++ - return cl.Delete(ctx, obj, opts...) - }, - Patch: func(ctx context.Context, cl client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { - fixture.requests.patches++ - return cl.Patch(ctx, obj, patch, opts...) - }, - }).Build() - generator := mockapplier.NewMockClusterObjectSetGenerator(gomock.NewController(t)) - generator.EXPECT().GenerateRevision(gomock.Any(), gomock.Any(), ext, gomock.Any(), gomock.Any()).Return( - ocv1ac.ClusterObjectSet("").WithSpec(ocv1ac.ClusterObjectSetSpec(). - WithGroup(ext.Name). - WithLifecycleState(ocv1.ClusterObjectSetLifecycleStateActive). - WithCollisionProtection(ocv1.CollisionProtectionPrevent). - WithPhases(ocv1ac.ClusterObjectSetPhase(). - WithName("deploy"). - WithObjects(ocv1ac.ClusterObjectSetObject().WithObject(payload)))), nil) - fixture.applier = &applier.Boxcutter{ - Client: fixture.client, - Scheme: scheme, - RevisionGenerator: generator, - FieldOwner: "test-owner", - SystemNamespace: "olmv1-system", - } - return fixture -} From c98ff9fd461a8bf5c7dbafcacf13f5581d55f852 Mon Sep 17 00:00:00 2001 From: "Per G. da Silva" Date: Fri, 23 Oct 2026 19:30:02 +0200 Subject: [PATCH 13/13] =?UTF-8?q?=E2=9C=A8=20set=20revision=20owner=20labe?= =?UTF-8?q?ls=20via=20ClusterObjectDeployment=20template=20metadata?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The direct-ClusterObjectSet path labeled every revision with the owner-kind and owner-name labels. The ClusterObjectDeployment path had moved those labels onto the deployment object only, leaving each ClusterObjectSet revision with just the controller-managed template-hash label. Set spec.template.metadata.labels on the generated ClusterObjectDeployment to the owner labels so the object-controller propagates them verbatim onto every revision it creates, restoring the pre-refactor revision labeling through the template metadata channel. Co-Authored-By: Claude Opus 4.8 Signed-off-by: Per G. da Silva --- .../applier/clusterobjectdeployment.go | 15 +++++++++++---- .../applier/clusterobjectdeployment_test.go | 7 +++++++ 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/internal/operator-controller/applier/clusterobjectdeployment.go b/internal/operator-controller/applier/clusterobjectdeployment.go index b144d283b7..b3ce106bbe 100644 --- a/internal/operator-controller/applier/clusterobjectdeployment.go +++ b/internal/operator-controller/applier/clusterobjectdeployment.go @@ -27,6 +27,11 @@ func buildClusterObjectDeployment( phases := PhaseSort(objects) + ownerLabels := map[string]string{ + labels.OwnerKindKey: ocv1.ClusterExtensionKind, + labels.OwnerNameKey: ext.Name, + } + templateSpec := ocv1ac.ClusterObjectDeploymentTemplateSpec(). WithPhases(phases...). WithProgressionProbes(defaultProgressionProbes...). @@ -36,7 +41,12 @@ func buildClusterObjectDeployment( templateSpec.WithProgressDeadlineMinutes(p) } + // Propagate the owner labels to each revision through the template metadata so every + // ClusterObjectSet the deployment creates carries them, matching the labeling the + // direct-ClusterObjectSet path applied before the ClusterObjectDeployment refactor. template := ocv1ac.ClusterObjectDeploymentTemplate(). + WithMetadata(ocv1ac.ClusterObjectDeploymentTemplateMetadata(). + WithLabels(ownerLabels)). WithSpec(templateSpec) spec := ocv1ac.ClusterObjectDeploymentSpec(). @@ -44,10 +54,7 @@ func buildClusterObjectDeployment( return ocv1ac.ClusterObjectDeployment(ext.Name). WithAnnotations(annotations). - WithLabels(map[string]string{ - labels.OwnerKindKey: ocv1.ClusterExtensionKind, - labels.OwnerNameKey: ext.Name, - }). + WithLabels(ownerLabels). WithSpec(spec) } diff --git a/internal/operator-controller/applier/clusterobjectdeployment_test.go b/internal/operator-controller/applier/clusterobjectdeployment_test.go index 0faf868465..15781ba8ef 100644 --- a/internal/operator-controller/applier/clusterobjectdeployment_test.go +++ b/internal/operator-controller/applier/clusterobjectdeployment_test.go @@ -77,6 +77,13 @@ func TestBuildClusterObjectDeployment(t *testing.T) { require.NotNil(t, result.Spec.Template) require.NotNil(t, result.Spec.Template.Spec) + // The owner labels must be propagated to each revision via the template metadata so + // every ClusterObjectSet the deployment creates carries them, matching the labeling + // the direct-ClusterObjectSet path used before the ClusterObjectDeployment refactor. + require.NotNil(t, result.Spec.Template.Metadata) + assert.Equal(t, ocv1.ClusterExtensionKind, result.Spec.Template.Metadata.Labels[labels.OwnerKindKey]) + assert.Equal(t, "ext", result.Spec.Template.Metadata.Labels[labels.OwnerNameKey]) + templateSpec := result.Spec.Template.Spec assert.Len(t, templateSpec.Phases, 1) assert.Equal(t, ocv1.CollisionProtectionPrevent, *templateSpec.CollisionProtection)