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/object-controller/controllers/clusterobjectdeployment_controller.go b/internal/object-controller/controllers/clusterobjectdeployment_controller.go index b5da2ac791..42381fa5e3 100644 --- a/internal/object-controller/controllers/clusterobjectdeployment_controller.go +++ b/internal/object-controller/controllers/clusterobjectdeployment_controller.go @@ -38,6 +38,17 @@ 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, + labels.OLMPropertiesKey, +} + // ClusterObjectDeploymentReconciler reconciles ClusterObjectDeployment objects by // managing their owned ClusterObjectSet revisions. type ClusterObjectDeploymentReconciler struct { @@ -103,7 +114,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) @@ -126,6 +137,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 @@ -225,6 +243,47 @@ func archivedTeardownComplete(rev *ocv1.ClusterObjectSet) bool { return cond != nil && cond.Reason == ocv1.ClusterObjectSetReasonArchived } +// 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. + 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) + } + + // Inspect any existing controller owner reference. + for _, ref := range rev.OwnerReferences { + 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 + } + } + + // No controller owner: adopt this orphan revision. + updated := rev.DeepCopy() + if err := controllerutil.SetControllerReference(cod, updated, c.Scheme); err != nil { + return fmt.Errorf("adopting orphan 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. // @@ -322,13 +381,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) } @@ -439,11 +498,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_internal_test.go b/internal/object-controller/controllers/clusterobjectdeployment_controller_internal_test.go index a47a384b9f..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 { @@ -42,18 +46,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 +65,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) { @@ -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) + }) +} diff --git a/internal/object-controller/controllers/clusterobjectdeployment_controller_test.go b/internal/object-controller/controllers/clusterobjectdeployment_controller_test.go index a80fd2758f..03b38fcf85 100644 --- a/internal/object-controller/controllers/clusterobjectdeployment_controller_test.go +++ b/internal/object-controller/controllers/clusterobjectdeployment_controller_test.go @@ -831,3 +831,36 @@ 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", + labels.OLMPropertiesKey: `[{"type":"olm.package","value":{"packageName":"test-package","version":"1.0.0"}}]`, + "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") + 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"] + require.False(t, exists, "non-bundle annotations must not be copied from COD to COS") +} diff --git a/internal/operator-controller/applier/boxcutter.go b/internal/operator-controller/applier/boxcutter.go index 0232ed3da7..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, @@ -52,6 +50,7 @@ type ClusterObjectSetGenerator interface { type SimpleRevisionGenerator struct { Scheme *runtime.Scheme ManifestProvider ManifestProvider + SystemNamespace string } func (r *SimpleRevisionGenerator) GenerateRevisionFromHelmRelease( @@ -107,39 +106,13 @@ func (r *SimpleRevisionGenerator) GenerateRevisionFromHelmRelease( return rev, nil } -func (r *SimpleRevisionGenerator) GenerateRevision( +// 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, - 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 - } - } - - // objectLabels + 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 +135,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. @@ -461,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 } @@ -494,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). @@ -502,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 ( @@ -780,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) @@ -807,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)) @@ -872,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_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 -} diff --git a/internal/operator-controller/applier/boxcutter_test.go b/internal/operator-controller/applier/boxcutter_test.go index d6c2ac5778..81f44e995a 100644 --- a/internal/operator-controller/applier/boxcutter_test.go +++ b/internal/operator-controller/applier/boxcutter_test.go @@ -2,30 +2,22 @@ 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" + ctrl "sigs.k8s.io/controller-runtime" "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 +149,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 +748,214 @@ 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) +} + +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) +} diff --git a/internal/operator-controller/applier/clusterobjectdeployment.go b/internal/operator-controller/applier/clusterobjectdeployment.go new file mode 100644 index 0000000000..b3ce106bbe --- /dev/null +++ b/internal/operator-controller/applier/clusterobjectdeployment.go @@ -0,0 +1,153 @@ +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/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) + + ownerLabels := map[string]string{ + labels.OwnerKindKey: ocv1.ClusterExtensionKind, + labels.OwnerNameKey: ext.Name, + } + + templateSpec := ocv1ac.ClusterObjectDeploymentTemplateSpec(). + WithPhases(phases...). + WithProgressionProbes(defaultProgressionProbes...). + WithCollisionProtection(collisionProtection) + + if p := ext.Spec.ProgressDeadlineMinutes; p > 0 { + 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(). + WithTemplate(template) + + return ocv1ac.ClusterObjectDeployment(ext.Name). + WithAnnotations(annotations). + WithLabels(ownerLabels). + 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{ + labels.OLMPropertiesKey, + } { + 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 new file mode 100644 index 0000000000..15781ba8ef --- /dev/null +++ b/internal/operator-controller/applier/clusterobjectdeployment_test.go @@ -0,0 +1,193 @@ +package applier + +import ( + "context" + "io/fs" + "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" + 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" + "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" +) + +// 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{ + 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) + + // 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) + 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]) + } +} diff --git a/internal/operator-controller/applier/secretpacker.go b/internal/operator-controller/applier/secretpacker.go index 4eaf6bc68a..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 } @@ -138,8 +140,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 +150,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 +165,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. @@ -174,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) @@ -188,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 e80ea3e36a..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", } @@ -43,11 +42,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 +212,46 @@ 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 the same content twice with independent packers. + packer1 := SecretPacker{ + OwnerName: "ext", + SystemNamespace: "olmv1-system", + } + result1, err := packer1.Pack(phases) + require.NoError(t, err) + require.Len(t, result1.Secrets, 1) + + packer2 := SecretPacker{ + OwnerName: "ext", + 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-". 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 content-addressed, not revision-dependent") + 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", 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) 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" ) 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. 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"