From fe28966f1508246d33709d2dc277dcc86516b12e Mon Sep 17 00:00:00 2001 From: "Per G. da Silva" Date: Mon, 26 Oct 2026 18:04:55 +0100 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8=20Apply=20a=20ClusterObjectDeployment?= =?UTF-8?q?=20instead=20of=20managing=20ClusterObjectSets?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Switches the operator-controller ClusterExtension applier to render and apply a single ClusterObjectDeployment (COD) instead of directly creating and managing ClusterObjectSet (COS) revisions. The COD controller owns revision lifecycle (creation, rollout progress, archival, and history pruning), so the operator-controller renders and applies one top-level object per extension. All changes are experimental-only (ClusterObjectDeployment is an experimental API); standard manifests are unaffected. - Render and apply a packed ClusterObjectDeployment generated from bundle content, replacing direct ClusterObjectSet creation and management. - Use revision-independent packed Secret names, and propagate bundle annotations (including olm.properties) from the COD through its template.metadata to each revision. - Add a ClusterObjectDeployment applyconfiguration builder. - Set revision owner labels via the COD template metadata. - Grant operator-controller the RBAC it needs for ClusterObjectDeployments. - Align the install/uninstall e2e suites with the ClusterExtension -> COD -> ClusterObjectSet ownership model, and remove the now-obsolete applier revision-bounds test (revision management moved to the COD controller). Co-Authored-By: Claude Opus 4.8 Signed-off-by: Per G. da Silva --- cmd/operator-controller/main.go | 2 + .../operator-controller/applier/boxcutter.go | 450 ++---- .../applier/boxcutter_revision_bounds_test.go | 260 ---- .../applier/boxcutter_test.go | 1356 +++-------------- .../applier/clusterobjectdeployment.go | 155 ++ .../applier/clusterobjectdeployment_test.go | 195 +++ .../operator-controller/applier/provider.go | 8 +- .../applier/secretpacker.go | 54 +- .../applier/secretpacker_test.go | 44 +- .../controllers/boxcutter_reconcile_steps.go | 28 +- .../boxcutter_reconcile_steps_apply_test.go | 180 --- .../clusterextension_controller.go | 23 +- ...usterextension_controller_internal_test.go | 46 - .../clusterobjectdeployment_status.go | 163 ++ .../clusterobjectdeployment_status_test.go | 276 ++++ .../controllers/common_controller.go | 97 -- .../controllers/common_controller_test.go | 150 -- internal/shared/labels/labels.go | 6 + .../testutil/mock/applier/mock_applier.go | 52 +- internal/testutil/mock/generate.go | 2 +- test/e2e/features/group.feature | 4 +- test/e2e/features/install.feature | 43 +- test/e2e/features/status.feature | 9 +- test/e2e/features/uninstall.feature | 10 +- test/e2e/features/update.feature | 6 +- test/e2e/steps/steps.go | 166 +- 26 files changed, 1511 insertions(+), 2274 deletions(-) delete mode 100644 internal/operator-controller/applier/boxcutter_revision_bounds_test.go create mode 100644 internal/operator-controller/applier/clusterobjectdeployment.go create mode 100644 internal/operator-controller/applier/clusterobjectdeployment_test.go delete mode 100644 internal/operator-controller/controllers/boxcutter_reconcile_steps_apply_test.go delete mode 100644 internal/operator-controller/controllers/clusterextension_controller_internal_test.go create mode 100644 internal/operator-controller/controllers/clusterobjectdeployment_status.go create mode 100644 internal/operator-controller/controllers/clusterobjectdeployment_status_test.go diff --git a/cmd/operator-controller/main.go b/cmd/operator-controller/main.go index a77aa20ee5..98461c743f 100644 --- a/cmd/operator-controller/main.go +++ b/cmd/operator-controller/main.go @@ -616,6 +616,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{ @@ -645,6 +646,7 @@ func (c *boxcutterReconcilerConfigurator) Configure(ceReconciler *controllers.Cl controllers.UnpackBundle(c.imagePuller, c.imageCache), controllers.ValidateInstallNamespace(coreClient), controllers.ApplyBundleWithBoxcutter(appl.Apply), + controllers.ReflectClusterObjectDeploymentStatus(c.mgr.GetClient()), } return nil diff --git a/internal/operator-controller/applier/boxcutter.go b/internal/operator-controller/applier/boxcutter.go index 0232ed3da7..e558e206ff 100644 --- a/internal/operator-controller/applier/boxcutter.go +++ b/internal/operator-controller/applier/boxcutter.go @@ -6,6 +6,7 @@ import ( "fmt" "io/fs" "maps" + "sort" "strings" "github.com/cert-manager/cert-manager/pkg/apis/certmanager" @@ -18,11 +19,10 @@ 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" "sigs.k8s.io/controller-runtime/pkg/client/apiutil" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/yaml" @@ -31,17 +31,33 @@ 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" + hashutil "github.com/operator-framework/operator-controller/internal/shared/util/hash" ) -const ( - ClusterObjectSetRetentionLimit = 5 -) - -type ClusterObjectSetGenerator interface { - GenerateRevision(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1ac.ClusterObjectSetApplyConfiguration, error) +// contentHashAnnotation records a fingerprint of the rendered content on the +// ClusterObjectDeployment. The applier uses it to detect when the content is +// unchanged between reconciles and skip redundant preflights. It lives on the +// ClusterObjectDeployment's top-level metadata (not its template), so it does not +// affect the template hash the ClusterObjectDeployment controller uses to cut +// revisions. +const contentHashAnnotation = "olm.operatorframework.io/content-hash" + +// RevisionGenerator renders bundle content into the revision-bearing artifacts +// the applier applies: a ClusterObjectDeployment (the primary path) or, for the +// Helm-based migration path, a ClusterObjectSet revision directly. +// +// TODO: reconsider this name (e.g. ClusterObjectDeploymentGenerator) once the +// Helm-based ClusterExtension -> boxcutter-based ClusterExtension migration work +// removes or relocates GenerateRevisionFromHelmRelease, so this interface no +// longer also produces a ClusterObjectSet. +type RevisionGenerator interface { + 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 +68,7 @@ type ClusterObjectSetGenerator interface { type SimpleRevisionGenerator struct { Scheme *runtime.Scheme ManifestProvider ManifestProvider + SystemNamespace string } func (r *SimpleRevisionGenerator) GenerateRevisionFromHelmRelease( @@ -107,39 +124,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 +153,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. @@ -258,7 +240,7 @@ func (r *SimpleRevisionGenerator) buildClusterObjectSet( // installations and remove direct COS discovery when that migration path is in place. type BoxcutterStorageMigrator struct { ActionClientGetter helmclient.ActionClientGetter - RevisionGenerator ClusterObjectSetGenerator + RevisionGenerator RevisionGenerator Client boxcutterStorageMigratorClient Scheme *runtime.Scheme FieldOwner string @@ -454,38 +436,40 @@ func (m *BoxcutterStorageMigrator) ensureRevisionStatus(ctx context.Context, nam type Boxcutter struct { Client client.Client Scheme *runtime.Scheme - RevisionGenerator ClusterObjectSetGenerator + RevisionGenerator RevisionGenerator Preflights []Preflight FieldOwner string SystemNamespace string } 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. + // + // TODO: a nil contentFS is an indirect signal for "resolved bundle content is + // unavailable". It conflates "catalog access" into the applier, which has no knowledge + // of the source type used to resolve the bundle. This maintain-current-state decision + // should move to a higher layer and key off a more direct signal than contentFS==nil. 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: + // Maintain current state: the ClusterObjectDeployment controller keeps + // reconciling the workload even without catalog access. The returned bool is + // advisory here — ClusterExtension status is derived from the + // ClusterObjectDeployment's rollout conditions, not from this value. + 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 +478,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,161 +486,91 @@ 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) + // Fingerprint the rendered content so a later reconcile can tell whether anything + // actually changed. Packed Secret names are content-addressed, so their sorted set + // uniquely identifies the content. Stamp it on the ClusterObjectDeployment's top-level + // metadata (not its template) and compare against the already-applied value below. + secretNames := make([]string, len(secrets)) + for i := range secrets { + secretNames[i] = secrets[i].Name + } + sort.Strings(secretNames) + contentHash := hashutil.DeepHashObject(secretNames) + cod.WithAnnotations(map[string]string{contentHashAnnotation: contentHash}) - switch { - case apierrors.IsInvalid(err): - state = StateNeedsUpgrade - case err == nil: - state = StateUnchanged - default: - return false, "", fmt.Errorf("patching %s Revision: %w", currentRevision.Name, err) - } + if err := bc.runPreflights(ctx, ext, contentHash, secrets); err != nil { + return false, "", err } - // Preflights - plainObjs := getObjects(desiredRevision) - for _, preflight := range bc.Preflights { - if shouldSkipPreflight(ctx, preflight, ext, state) { - continue + // 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. + for i := range secrets { + if err := controllerutil.SetControllerReference(ext, &secrets[i], bc.Scheme); err != nil { + return false, "", fmt.Errorf("setting owner reference on ref Secret %q: %w", secrets[i].Name, err) } - switch state { - case StateNeedsInstall: - err := preflight.Install(ctx, plainObjs) - if 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 { - return false, "", err + if err := bc.Client.Create(ctx, &secrets[i]); err != nil { + if !apierrors.IsAlreadyExists(err) { + return false, "", fmt.Errorf("creating ref Secret %q: %w", secrets[i].Name, 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) - } + // 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) } 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) +// runPreflights runs the configured preflight checks for a desired apply. It determines whether +// this is an install or an upgrade from whether the ClusterObjectDeployment already exists (a +// direct, controller-owned signal, rather than our own ClusterExtension status which may be stale +// or modified externally), and short-circuits — skipping the preflights entirely — when the +// already-applied deployment carries a matching content hash, so the CRD-safety preflights do not +// re-run on every steady-state reconcile. +func (bc *Boxcutter) runPreflights(ctx context.Context, ext *ocv1.ClusterExtension, contentHash string, secrets []corev1.Secret) error { + state := StateNeedsInstall + existing := &ocv1.ClusterObjectDeployment{} + switch getErr := bc.Client.Get(ctx, client.ObjectKey{Name: ext.Name}, existing); { + case getErr == nil: + if existing.Annotations[contentHashAnnotation] == contentHash { + state = StateUnchanged + } else { + state = StateNeedsUpgrade + } + case !apierrors.IsNotFound(getErr): + return fmt.Errorf("getting ClusterObjectDeployment %q: %w", ext.Name, getErr) } - 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) + if state == StateUnchanged { + return nil } - // 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) + // Decode the packed objects only when preflights will actually run. + plainObjs, err := objectsFromSecrets(secrets) if err != nil { - return fmt.Errorf("packing objects into Secrets: %w", err) + return fmt.Errorf("decoding packed objects for preflights: %w", err) } - 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 { - if !apierrors.IsAlreadyExists(err) { - return fmt.Errorf("creating ref Secret %q: %w", packResult.Secrets[i].Name, err) - } + for _, preflight := range bc.Preflights { + if shouldSkipPreflight(ctx, preflight, ext, state) { + continue } - } - - // 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) - } - - // 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 -} - -// 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) + switch state { + case StateNeedsInstall: + if err := preflight.Install(ctx, plainObjs); err != nil { + return err + } + case StateNeedsUpgrade: + if err := preflight.Upgrade(ctx, plainObjs); err != nil { + return err } } } return nil } -func latestRevisionNumber(prevRevisions []ocv1.ClusterObjectSet) int64 { - if len(prevRevisions) == 0 { - return 0 - } - return prevRevisions[len(prevRevisions)-1].Spec.Revision -} - var ( // defaultProgressionProbes is the default set of progression probes used to check for phase readiness defaultProgressionProbes = []*ocv1ac.ProgressionProbeApplyConfiguration{ @@ -780,26 +694,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 +701,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 +744,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..6d7351f403 100644 --- a/internal/operator-controller/applier/boxcutter_test.go +++ b/internal/operator-controller/applier/boxcutter_test.go @@ -3,29 +3,21 @@ package applier_test import ( "context" "errors" - "fmt" - "io/fs" "strings" "testing" - "testing/fstest" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.uber.org/mock/gomock" "helm.sh/helm/v3/pkg/release" "helm.sh/helm/v3/pkg/storage/driver" - appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" - apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/util/validation/field" - k8scheme "k8s.io/client-go/kubernetes/scheme" "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" - "sigs.k8s.io/controller-runtime/pkg/client/interceptor" ocv1 "github.com/operator-framework/operator-controller/api/v1" ocv1ac "github.com/operator-framework/operator-controller/applyconfigurations/api/v1" @@ -157,1115 +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 { @@ -1276,11 +159,11 @@ func TestBoxcutterStorageMigrator(t *testing.T) { WithSpec(ocv1ac.ClusterObjectSetSpec().WithGroup(ext.Name)) } - // newStorageMigratorGenerator creates a gomock ClusterObjectSetGenerator for storage migrator tests. + // newStorageMigratorGenerator creates a gomock RevisionGenerator for storage migrator tests. // GenerateRevision is not expected to be called. GenerateRevisionFromHelmRelease returns the default result. - newStorageMigratorGenerator := func(t *testing.T) *mockapplier.MockClusterObjectSetGenerator { + newStorageMigratorGenerator := func(t *testing.T) *mockapplier.MockRevisionGenerator { ctrl := gomock.NewController(t) - m := mockapplier.NewMockClusterObjectSetGenerator(ctrl) + m := mockapplier.NewMockRevisionGenerator(ctrl) m.EXPECT().GenerateRevisionFromHelmRelease(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn( func(ctx context.Context, helmRelease *release.Release, e *ocv1.ClusterExtension, objectLabels map[string]string) (*ocv1ac.ClusterObjectSetApplyConfiguration, error) { return defaultHelmRevisionResult(e), nil @@ -1421,7 +304,7 @@ func TestBoxcutterStorageMigrator(t *testing.T) { } // GenerateRevisionFromHelmRelease should not be called when revisions already exist ctrl := gomock.NewController(t) - brb := mockapplier.NewMockClusterObjectSetGenerator(ctrl) + brb := mockapplier.NewMockRevisionGenerator(ctrl) mag := newMockActionGetter(ctrl, mockActionGetterConfig{}) mockClient := mockctrlclient.NewMockClient(ctrl) @@ -1470,7 +353,7 @@ func TestBoxcutterStorageMigrator(t *testing.T) { Spec: ocv1.ClusterExtensionSpec{Namespace: "test-namespace"}, } ctrl := gomock.NewController(t) - brb := mockapplier.NewMockClusterObjectSetGenerator(ctrl) + brb := mockapplier.NewMockRevisionGenerator(ctrl) mag := newMockActionGetter(ctrl, mockActionGetterConfig{}) mockClient := mockctrlclient.NewMockClient(ctrl) @@ -1541,7 +424,7 @@ func TestBoxcutterStorageMigrator(t *testing.T) { Spec: ocv1.ClusterExtensionSpec{Namespace: "test-namespace"}, } ctrl := gomock.NewController(t) - brb := mockapplier.NewMockClusterObjectSetGenerator(ctrl) + brb := mockapplier.NewMockRevisionGenerator(ctrl) mag := newMockActionGetter(ctrl, mockActionGetterConfig{}) mockClient := mockctrlclient.NewMockClient(ctrl) @@ -1614,7 +497,7 @@ func TestBoxcutterStorageMigrator(t *testing.T) { Spec: ocv1.ClusterExtensionSpec{Namespace: "test-namespace"}, } ctrl := gomock.NewController(t) - brb := mockapplier.NewMockClusterObjectSetGenerator(ctrl) + brb := mockapplier.NewMockRevisionGenerator(ctrl) mag := newMockActionGetter(ctrl, mockActionGetterConfig{}) mockClient := mockctrlclient.NewMockClient(ctrl) @@ -1678,7 +561,7 @@ func TestBoxcutterStorageMigrator(t *testing.T) { } ctrl := gomock.NewController(t) - brb := mockapplier.NewMockClusterObjectSetGenerator(ctrl) + brb := mockapplier.NewMockRevisionGenerator(ctrl) brb.EXPECT().GenerateRevisionFromHelmRelease(gomock.Any(), expectedRelease, gomock.Any(), gomock.Any()). DoAndReturn(func(ctx context.Context, helmRelease *release.Release, e *ocv1.ClusterExtension, objectLabels map[string]string) (*ocv1ac.ClusterObjectSetApplyConfiguration, error) { return defaultHelmRevisionResult(e), nil @@ -1769,7 +652,7 @@ func TestBoxcutterStorageMigrator(t *testing.T) { } ctrl := gomock.NewController(t) // GenerateRevisionFromHelmRelease should NOT be called when no deployed release exists - brb := mockapplier.NewMockClusterObjectSetGenerator(ctrl) + brb := mockapplier.NewMockRevisionGenerator(ctrl) mag := newMockActionGetter(ctrl, mockActionGetterConfig{ currentRel: &release.Release{ Name: "test123", @@ -1816,7 +699,7 @@ func TestBoxcutterStorageMigrator(t *testing.T) { Spec: ocv1.ClusterExtensionSpec{Namespace: "test-namespace"}, } ctrl := gomock.NewController(t) - brb := mockapplier.NewMockClusterObjectSetGenerator(ctrl) + brb := mockapplier.NewMockRevisionGenerator(ctrl) mag := newMockActionGetter(ctrl, mockActionGetterConfig{ getClientErr: driver.ErrReleaseNotFound, }) @@ -1849,7 +732,7 @@ func TestBoxcutterStorageMigrator(t *testing.T) { // A managed-namespace extension never had a Helm release, so migration must be a no-op: // no List, no action client, and no revision generation. No expectations are set, so // gomock fails the test if any of these are called. - brb := mockapplier.NewMockClusterObjectSetGenerator(ctrl) + brb := mockapplier.NewMockRevisionGenerator(ctrl) mag := newMockActionGetter(ctrl, mockActionGetterConfig{}) mockClient := mockctrlclient.NewMockClient(ctrl) @@ -1865,3 +748,220 @@ 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) + }) +} + +// TestBoxcutterApply_Preflights verifies which preflight hook runs for each rollout state +// and that a preflight failure propagates out of Apply. Each row is a sequence of applies +// against the same Boxcutter: a step renders its own objects, so identical content between +// steps is detected as unchanged (skip) and changed content is detected as an upgrade. +func TestBoxcutterApply_Preflights(t *testing.T) { + cmA := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "cm-a", Namespace: "test-ns"}} + cmB := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "cm-b", Namespace: "test-ns"}} + + type applyStep struct { + objects []client.Object + wantErr string // substring expected in the apply error; "" means the apply must succeed + } + for _, tc := range []struct { + name string + installErr error + upgradeErr error + steps []applyStep + wantInstall int + wantUpgrade int + }{ + { + name: "fresh install runs the install preflight", + steps: []applyStep{{objects: []client.Object{cmA}}}, + wantInstall: 1, + }, + { + name: "unchanged re-apply skips both preflights", + steps: []applyStep{{objects: []client.Object{cmA}}, {objects: []client.Object{cmA}}}, + wantInstall: 1, + }, + { + name: "changed content runs the upgrade preflight", + steps: []applyStep{{objects: []client.Object{cmA}}, {objects: []client.Object{cmB}}}, + wantInstall: 1, + wantUpgrade: 1, + }, + { + name: "install preflight failure propagates out of Apply", + installErr: errors.New("install boom"), + steps: []applyStep{{objects: []client.Object{cmA}, wantErr: "install boom"}}, + wantInstall: 1, + }, + { + name: "upgrade preflight failure propagates out of Apply", + upgradeErr: errors.New("upgrade boom"), + steps: []applyStep{ + {objects: []client.Object{cmA}}, + {objects: []client.Object{cmB}, wantErr: "upgrade boom"}, + }, + wantInstall: 1, + wantUpgrade: 1, + }, + } { + t.Run(tc.name, func(t *testing.T) { + scheme := codTestScheme(t) + cl := fake.NewClientBuilder().WithScheme(scheme).Build() + ctrl := gomock.NewController(t) + + // One Get per apply; return each step's objects in call order. + mp := mockapplier.NewMockManifestProvider(ctrl) + for _, step := range tc.steps { + mp.EXPECT().Get(gomock.Any(), gomock.Any()).Return(step.objects, nil).Times(1) + } + + // A non-crdupgradesafety mock preflight is never skipped, so Install/Upgrade are + // invoked exactly per the rollout state. Times(0) asserts the hook is not called. + pf := mockapplier.NewMockPreflight(ctrl) + pf.EXPECT().Install(gomock.Any(), gomock.Any()).Return(tc.installErr).Times(tc.wantInstall) + pf.EXPECT().Upgrade(gomock.Any(), gomock.Any()).Return(tc.upgradeErr).Times(tc.wantUpgrade) + + bc := &applier.Boxcutter{ + Client: cl, + Scheme: scheme, + RevisionGenerator: &applier.SimpleRevisionGenerator{ + Scheme: scheme, + ManifestProvider: mp, + SystemNamespace: "olmv1-system", + }, + Preflights: []applier.Preflight{pf}, + FieldOwner: "test-owner", + SystemNamespace: "olmv1-system", + } + ext := &ocv1.ClusterExtension{ObjectMeta: metav1.ObjectMeta{Name: "test-ext", UID: "test-uid"}} + + for i, step := range tc.steps { + _, _, err := bc.Apply(t.Context(), dummyBundle, ext, map[string]string{"some": "label"}, map[string]string{}) + if step.wantErr != "" { + require.Errorf(t, err, "apply step %d", i) + assert.Containsf(t, err.Error(), step.wantErr, "apply step %d", i) + continue + } + require.NoErrorf(t, err, "apply step %d", i) + } + }) + } +} diff --git a/internal/operator-controller/applier/clusterobjectdeployment.go b/internal/operator-controller/applier/clusterobjectdeployment.go new file mode 100644 index 0000000000..578c38e53b --- /dev/null +++ b/internal/operator-controller/applier/clusterobjectdeployment.go @@ -0,0 +1,155 @@ +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 { + 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 and the bundle-metadata annotations to each revision + // through the template metadata. The COD controller copies template.metadata onto every + // ClusterObjectSet it creates verbatim, without knowing any specific key, so OLM-specific + // annotations (e.g. olm.properties, bundle metadata) travel through this generic channel + // rather than being special-cased by the generic controller. + templateMetadata := ocv1ac.ClusterObjectDeploymentTemplateMetadata(). + WithLabels(ownerLabels) + if len(annotations) > 0 { + templateMetadata.WithAnnotations(annotations) + } + + template := ocv1ac.ClusterObjectDeploymentTemplate(). + WithMetadata(templateMetadata). + WithSpec(templateSpec) + + spec := ocv1ac.ClusterObjectDeploymentSpec(). + WithTemplate(template) + + return ocv1ac.ClusterObjectDeployment(ext.Name). + 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..9b479936b6 --- /dev/null +++ b/internal/operator-controller/applier/clusterobjectdeployment_test.go @@ -0,0 +1,195 @@ +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) + require.NotNil(t, result.Spec.Template.Metadata) + assert.Equal(t, "test-value", result.Spec.Template.Metadata.Annotations["test-key"], "annotations propagate via template metadata") + 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 the template metadata (propagated to each revision)") + require.NotNil(t, cod.Spec.Template.Metadata) + assert.Equal(t, "test-bundle", cod.Spec.Template.Metadata.Annotations[labels.BundleNameKey]) + assert.Equal(t, "test-package", cod.Spec.Template.Metadata.Annotations[labels.PackageNameKey]) + assert.Equal(t, "1.0.0", cod.Spec.Template.Metadata.Annotations[labels.BundleVersionKey]) + assert.Equal(t, "test-catalog/test-package@sha256:abc123", cod.Spec.Template.Metadata.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/provider.go b/internal/operator-controller/applier/provider.go index d4db0a8570..2a8c57032f 100644 --- a/internal/operator-controller/applier/provider.go +++ b/internal/operator-controller/applier/provider.go @@ -200,9 +200,11 @@ func extensionConfigBytes(ext *ocv1.ClusterExtension) []byte { } func getBundleAnnotations(bundleFS fs.FS) (map[string]string, error) { - // The need to get the underlying bundle in order to extract its annotations - // will go away once we have a bundle interface that can surface the annotations independently of the - // underlying bundle format... + // TODO: this reaches into the registry+v1 CSV, which is too format-specific for a function + // that generically generates a ClusterObjectDeployment from a bundleFS. Annotation + // extraction should move behind a bundle-format interface — each format providing the subset + // or translation of annotations that makes sense for the COD (conceptually a + // getCODAnnotationsFromBundle) — rather than assuming registry+v1 here. rv1, err := source.FromFS(bundleFS).GetBundle() if err != nil { return nil, err diff --git a/internal/operator-controller/applier/secretpacker.go b/internal/operator-controller/applier/secretpacker.go index 4eaf6bc68a..7f371bf5c8 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,8 @@ 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]) + // h.Sum(nil)[:8] is 8 bytes, which %x renders as 16 hex characters. + 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 +176,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 +223,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/boxcutter_reconcile_steps.go b/internal/operator-controller/controllers/boxcutter_reconcile_steps.go index 12e9c8294d..0f7154b409 100644 --- a/internal/operator-controller/controllers/boxcutter_reconcile_steps.go +++ b/internal/operator-controller/controllers/boxcutter_reconcile_steps.go @@ -22,6 +22,7 @@ import ( "fmt" "io/fs" + apierrors "k8s.io/apimachinery/pkg/api/errors" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/log" @@ -126,10 +127,31 @@ func ApplyBundleWithBoxcutter(apply func(ctx context.Context, contentFS fs.FS, e return nil, err } - setAvailableFromRevisionStates(ext, state.revisionStates) - setProgressingFromRevisionStates(ext, state.revisionStates) - setInstalledStatusFromRevisionStates(ext, state.revisionStates) + // Success-path status (Available, Progressing, Installed) is reflected from the + // ClusterObjectDeployment by the ReflectClusterObjectDeploymentStatus step. + return nil, nil + } +} +// ReflectClusterObjectDeploymentStatus reads the ClusterObjectDeployment the applier manages for the +// ClusterExtension and maps its rolled-up Available, Progressing, and installed-bundle state onto the +// ClusterExtension. The deployment is the single object the ClusterExtension observes for rollout +// health, replacing per-ClusterObjectSet observation. +func ReflectClusterObjectDeploymentStatus(reader client.Reader) ReconcileStepFunc { + return func(ctx context.Context, _ *reconcileState, ext *ocv1.ClusterExtension) (*ctrl.Result, error) { + cod := &ocv1.ClusterObjectDeployment{} + switch err := reader.Get(ctx, client.ObjectKey{Name: ext.Name}, cod); { + case apierrors.IsNotFound(err): + // The deployment was just applied and is not yet visible in the cache; its status is empty + // anyway. Mapping from an empty deployment reports "rolling out"; the watch re-triggers + // reconciliation once the deployment and its status become visible. + cod = &ocv1.ClusterObjectDeployment{} + case err != nil: + return nil, fmt.Errorf("getting ClusterObjectDeployment %q: %w", ext.Name, err) + } + setAvailableFromClusterObjectDeployment(ext, cod) + setProgressingFromClusterObjectDeployment(ext, cod) + setInstalledFromClusterObjectDeployment(ext, cod) return nil, nil } } diff --git a/internal/operator-controller/controllers/boxcutter_reconcile_steps_apply_test.go b/internal/operator-controller/controllers/boxcutter_reconcile_steps_apply_test.go deleted file mode 100644 index ba5a61c0d7..0000000000 --- a/internal/operator-controller/controllers/boxcutter_reconcile_steps_apply_test.go +++ /dev/null @@ -1,180 +0,0 @@ -/* -Copyright 2026. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package controllers - -import ( - "context" - "io/fs" - "testing" - "testing/fstest" - - "github.com/stretchr/testify/require" - apimeta "k8s.io/apimachinery/pkg/api/meta" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - - ocv1 "github.com/operator-framework/operator-controller/api/v1" -) - -func TestApplyBundleWithBoxcutter(t *testing.T) { - type args struct { - initialConditions []metav1.Condition - revisionStates *RevisionStates - } - type want struct { - progressing *metav1.Condition - } - - for _, tc := range []struct { - name string - args args - want want - }{ - { - name: "installed and rolling out revision during update", - args: args{ - revisionStates: &RevisionStates{ - Installed: &RevisionMetadata{ - RevisionName: "ce-1", - BundleMetadata: ocv1.BundleMetadata{ - Name: "test-bundle", - Version: "1.0.0", - }, - }, - RollingOut: []*RevisionMetadata{ - {RevisionName: "ce-2"}, - }, - }, - }, - want: want{ - progressing: &metav1.Condition{ - Type: ocv1.TypeProgressing, - Status: metav1.ConditionTrue, - Reason: ocv1.ReasonRollingOut, - Message: "Revision is rolling out.", - }, - }, - }, - { - name: "new installed revision after update", - args: args{ - revisionStates: &RevisionStates{ - Installed: &RevisionMetadata{ - RevisionName: "ce-2", - BundleMetadata: ocv1.BundleMetadata{ - Name: "test-bundle", - Version: "1.0.1", - }, - }, - }, - }, - want: want{ - progressing: &metav1.Condition{ - Type: ocv1.TypeProgressing, - Status: metav1.ConditionTrue, - Reason: ocv1.ReasonSucceeded, - Message: "Desired state reached", - }, - }, - }, - { - name: "ongoing installation", - args: args{ - revisionStates: &RevisionStates{ - RollingOut: []*RevisionMetadata{ - {RevisionName: "ce-1"}, - }, - }, - }, - want: want{ - progressing: &metav1.Condition{ - Type: ocv1.TypeProgressing, - Status: metav1.ConditionTrue, - Reason: ocv1.ReasonRollingOut, - Message: "Revision is rolling out.", - }, - }, - }, - { - name: "rolling revision without Ready resets stale Progressing to RollingOut default", - args: args{ - initialConditions: []metav1.Condition{ - { - Type: ocv1.TypeProgressing, - Status: metav1.ConditionTrue, - Reason: ocv1.ReasonSucceeded, - Message: "Desired state reached", - }, - }, - revisionStates: &RevisionStates{ - RollingOut: []*RevisionMetadata{ - // Freshly created revision that has not reconciled yet: - // it has no Ready condition. - {RevisionName: "ce-1"}, - }, - }, - }, - want: want{ - progressing: &metav1.Condition{ - Type: ocv1.TypeProgressing, - Status: metav1.ConditionTrue, - Reason: ocv1.ReasonRollingOut, - Message: "Revision is rolling out.", - }, - }, - }, - } { - t.Run(tc.name, func(t *testing.T) { - ctx := context.Background() - - ext := &ocv1.ClusterExtension{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-ext", - Generation: 1, - }, - Status: ocv1.ClusterExtensionStatus{ - Conditions: tc.args.initialConditions, - }, - } - - state := &reconcileState{ - revisionStates: tc.args.revisionStates, - resolvedRevisionMetadata: &RevisionMetadata{ - BundleMetadata: ocv1.BundleMetadata{ - Name: "test-bundle", - Version: "1.0.0", - }, - }, - imageFS: fstest.MapFS{}, - } - - stepFunc := ApplyBundleWithBoxcutter(func(_ context.Context, _ fs.FS, _ *ocv1.ClusterExtension, _, _ map[string]string) (bool, string, error) { - return true, "", nil - }) - result, err := stepFunc(ctx, state, ext) - require.NoError(t, err) - require.Nil(t, result) - - if tc.want.progressing != nil { - got := apimeta.FindStatusCondition(ext.Status.Conditions, ocv1.TypeProgressing) - require.NotNil(t, got, "Progressing condition not found") - require.Equal(t, tc.want.progressing.Status, got.Status, "Progressing.Status mismatch") - require.Equal(t, tc.want.progressing.Reason, got.Reason, "Progressing.Reason mismatch") - require.Equal(t, tc.want.progressing.Message, got.Message, "Progressing.Message mismatch") - } - }) - } -} diff --git a/internal/operator-controller/controllers/clusterextension_controller.go b/internal/operator-controller/controllers/clusterextension_controller.go index 25a071b29a..8099f29960 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) @@ -420,13 +425,13 @@ func collectDeprecationMessages(entries []declcfg.DeprecationEntry) []string { return messages } -// SetupWithManagerForBoxcutter sets up the controller with ClusterObjectSet revision watches. -// TODO(COD): Replace this COS watch and mapper with a COD watch when ClusterExtension -// reconciliation manages ClusterObjectDeployments. +// SetupWithManagerForBoxcutter sets up the controller to watch the ClusterObjectDeployment it manages +// for each ClusterExtension. The ClusterExtension observes the deployment's rolled-up rollout status, +// and the deployment is controller-owned by the ClusterExtension, so an owner watch re-reconciles the +// ClusterExtension whenever its deployment's status changes. func (r *ClusterExtensionReconciler) SetupWithManagerForBoxcutter(mgr ctrl.Manager) (crcontroller.Controller, error) { return r.newControllerBuilder(mgr). - Watches(&ocv1.ClusterObjectSet{}, - crhandler.EnqueueRequestsFromMapFunc(clusterExtensionRequestsForClusterObjectSet)). + Owns(&ocv1.ClusterObjectDeployment{}). Build(r) } @@ -447,14 +452,6 @@ func (r *ClusterExtensionReconciler) SetupWithManagerForHelm(mgr ctrl.Manager, t Build(r) } -func clusterExtensionRequestsForClusterObjectSet(_ context.Context, obj client.Object) []reconcile.Request { - cos, ok := obj.(*ocv1.ClusterObjectSet) - if !ok || cos.Spec.Group == "" { - return nil - } - return []reconcile.Request{{NamespacedName: types.NamespacedName{Name: cos.Spec.Group}}} -} - func (r *ClusterExtensionReconciler) newControllerBuilder(mgr ctrl.Manager) *ctrl.Builder { return ctrl.NewControllerManagedBy(mgr). For(&ocv1.ClusterExtension{}). diff --git a/internal/operator-controller/controllers/clusterextension_controller_internal_test.go b/internal/operator-controller/controllers/clusterextension_controller_internal_test.go deleted file mode 100644 index 31363ea79e..0000000000 --- a/internal/operator-controller/controllers/clusterextension_controller_internal_test.go +++ /dev/null @@ -1,46 +0,0 @@ -package controllers - -import ( - "testing" - - "github.com/stretchr/testify/require" - corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/types" - "k8s.io/utils/ptr" - "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/reconcile" - - ocv1 "github.com/operator-framework/operator-controller/api/v1" -) - -func TestClusterExtensionRequestsForClusterObjectSet(t *testing.T) { - for _, tc := range []struct { - name string - obj client.Object - want []reconcile.Request - }{ - { - name: "group without owner", - obj: &ocv1.ClusterObjectSet{Spec: ocv1.ClusterObjectSetSpec{Group: "my-extension"}}, - want: []reconcile.Request{{NamespacedName: types.NamespacedName{Name: "my-extension"}}}, - }, - { - name: "group takes precedence over owner metadata", - obj: &ocv1.ClusterObjectSet{ - ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{"olm.operatorframework.io/owner-name": "other"}, - OwnerReferences: []metav1.OwnerReference{{Kind: ocv1.ClusterExtensionKind, Name: "other", Controller: ptr.To(true)}}, - }, - Spec: ocv1.ClusterObjectSetSpec{Group: "my-extension"}, - }, - want: []reconcile.Request{{NamespacedName: types.NamespacedName{Name: "my-extension"}}}, - }, - {name: "empty group", obj: &ocv1.ClusterObjectSet{}}, - {name: "other resource", obj: &corev1.ConfigMap{}}, - } { - t.Run(tc.name, func(t *testing.T) { - require.Equal(t, tc.want, clusterExtensionRequestsForClusterObjectSet(t.Context(), tc.obj)) - }) - } -} diff --git a/internal/operator-controller/controllers/clusterobjectdeployment_status.go b/internal/operator-controller/controllers/clusterobjectdeployment_status.go new file mode 100644 index 0000000000..34e81d3281 --- /dev/null +++ b/internal/operator-controller/controllers/clusterobjectdeployment_status.go @@ -0,0 +1,163 @@ +package controllers + +import ( + "fmt" + + apimeta "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + ocv1 "github.com/operator-framework/operator-controller/api/v1" + "github.com/operator-framework/operator-controller/internal/shared/labels" +) + +// The functions in this file map a ClusterObjectDeployment's rolled-up status onto the owning +// ClusterExtension's conditions. The ClusterObjectDeployment is the single object the ClusterExtension +// observes for rollout health: its Available and Progressing conditions aggregate the health of the +// underlying ClusterObjectSet revisions (including the progress deadline and head-revision blocking +// reasons), and its template carries the bundle identity. + +// availableTrueMessage is the message surfaced on the ClusterExtension Available condition when the +// ClusterObjectDeployment is available. It is intentionally decoupled from the deployment's own message. +const availableTrueMessage = "Objects are available and pass all probes." + +// setAvailableFromClusterObjectDeployment maps the deployment's Available condition onto the +// ClusterExtension's Available condition. An available (True) deployment is re-emitted with a +// ClusterExtension-owned reason and message; other states are surfaced as-is with the Type rewritten. +func setAvailableFromClusterObjectDeployment(ext *ocv1.ClusterExtension, cod *ocv1.ClusterObjectDeployment) { + available := apimeta.FindStatusCondition(cod.Status.Conditions, ocv1.ClusterObjectDeploymentTypeAvailable) + if available == nil { + // The deployment has not reported availability yet; leave any prior value in place. + return + } + a := *available + a.Type = ocv1.TypeAvailable + a.ObservedGeneration = ext.GetGeneration() + if a.Status == metav1.ConditionTrue { + a.Reason = ocv1.ReasonProbesSucceeded + a.Message = availableTrueMessage + } + apimeta.SetStatusCondition(&ext.Status.Conditions, a) +} + +// setProgressingFromClusterObjectDeployment maps the deployment's Progressing condition onto the +// ClusterExtension's Progressing condition. +func setProgressingFromClusterObjectDeployment(ext *ocv1.ClusterExtension, cod *ocv1.ClusterObjectDeployment) { + prog := progressingConditionFromClusterObjectDeployment( + apimeta.FindStatusCondition(cod.Status.Conditions, ocv1.ClusterObjectDeploymentTypeProgressing)) + prog.ObservedGeneration = ext.GetGeneration() + apimeta.SetStatusCondition(&ext.Status.Conditions, prog) +} + +// progressingConditionFromClusterObjectDeployment translates a ClusterObjectDeployment Progressing +// condition into a ClusterExtension Progressing condition. The ClusterExtension follows the OLMv1 +// convention where a completed rollout stays Progressing=True/Succeeded and only terminal states are +// False, so the status is recomputed per reason rather than copied from the deployment. A nil deployment +// condition (e.g. a freshly created deployment that has not reconciled) is treated as rolling out. +func progressingConditionFromClusterObjectDeployment(codProgressing *metav1.Condition) metav1.Condition { + prog := metav1.Condition{Type: ocv1.TypeProgressing, Status: metav1.ConditionTrue} + if codProgressing == nil { + prog.Reason = ocv1.ReasonRollingOut + prog.Message = "Revision is rolling out." + return prog + } + prog.Message = codProgressing.Message + switch codProgressing.Reason { + case ocv1.ClusterObjectDeploymentReasonNewClusterObjectSetProgressed: + // The deployment reports a completed rollout as Progressing=False; the ClusterExtension keeps + // Progressing=True/Succeeded to signal "reconciled, desired state reached". + prog.Reason = ocv1.ReasonSucceeded + prog.Message = "Desired state reached" + case ocv1.ClusterObjectDeploymentReasonRevisionObjectCollision, + ocv1.ClusterObjectDeploymentReasonRevisionPreconditionsNotMet: + // The head revision is impeded by a foreign-owned object or an unmet precondition. These clear + // on their own once the obstruction is resolved, so the extension keeps retrying. + prog.Reason = ocv1.ReasonRetrying + case ocv1.ClusterObjectDeploymentReasonProgressDeadlineExceeded: + prog.Status = metav1.ConditionFalse + prog.Reason = ocv1.ReasonProgressDeadlineExceeded + case ocv1.ClusterObjectDeploymentReasonInvalidSpecification, + ocv1.ClusterObjectDeploymentReasonRevisionInvalidSpecification: + // An invalid deployment template or revision spec needs a spec change to proceed; it does not + // retry on its own, so report it as terminal. + prog.Status = metav1.ConditionFalse + prog.Reason = ocv1.ReasonBlocked + default: + // NewClusterObjectSetProgressing and any future rolling reason. + prog.Reason = ocv1.ReasonRollingOut + } + return prog +} + +// setInstalledFromClusterObjectDeployment maps the deployment onto the ClusterExtension's Installed +// condition and .status.install. The installed bundle is only promoted once the current rollout has +// completed; while a rollout is in flight the previously-installed bundle is left untouched, so an +// in-progress upgrade keeps reporting the old bundle until the new one succeeds. Resolution is gated +// while a revision is rolling out, so the deployment template still names the just-completed bundle at +// the moment completion is observed. +func setInstalledFromClusterObjectDeployment(ext *ocv1.ClusterExtension, cod *ocv1.ClusterObjectDeployment) { + if clusterObjectDeploymentRolloutComplete(cod) { + bundle := bundleMetadataFromClusterObjectDeployment(cod) + setInstallStatus(ext, &ocv1.ClusterExtensionInstallStatus{Bundle: bundle}) + // Surface the resolved bundle image reference in the message, matching the + // pre-ClusterObjectDeployment status which reported the image (the bundle-reference + // annotation) rather than the bundle name. BundleMetadata has no image field, so read + // the annotation the applier stamps onto the template; fall back to the bundle name if + // it is absent (e.g. a revision created without the annotation). + installed := bundle.Name + if ref := cod.Spec.Template.Metadata.Annotations[labels.BundleReferenceKey]; ref != "" { + installed = ref + } + setInstalledStatusConditionSuccess(ext, fmt.Sprintf("Installed bundle %s successfully", installed)) + return + } + if ext.Status.Install == nil { + // Nothing has ever been installed (first install in progress or failing). + setInstalledStatusConditionFalse(ext, installFailureReasonFromClusterObjectDeployment(cod), "No bundle installed") + } + // Otherwise a bundle is already installed (from a prior completion) and a new rollout is in flight; + // leave Installed=True and .status.install unchanged until the rollout completes. +} + +// clusterObjectDeploymentRolloutComplete reports whether the deployment's current-generation rollout +// has finished successfully. ObservedGeneration is checked so a stale "progressed" condition from a +// prior template generation is not mistaken for completion of the current one. +func clusterObjectDeploymentRolloutComplete(cod *ocv1.ClusterObjectDeployment) bool { + prog := apimeta.FindStatusCondition(cod.Status.Conditions, ocv1.ClusterObjectDeploymentTypeProgressing) + return prog != nil && + prog.Status == metav1.ConditionFalse && + prog.Reason == ocv1.ClusterObjectDeploymentReasonNewClusterObjectSetProgressed && + prog.ObservedGeneration == cod.Generation +} + +// bundleMetadataFromClusterObjectDeployment extracts the bundle identity the applier stamped onto the +// deployment's template metadata. During a rollout this names the bundle being rolled out, so callers +// must only promote it to .status.install once the rollout is complete. +func bundleMetadataFromClusterObjectDeployment(cod *ocv1.ClusterObjectDeployment) ocv1.BundleMetadata { + ann := cod.Spec.Template.Metadata.Annotations + bm := ocv1.BundleMetadata{ + Name: ann[labels.BundleNameKey], + Version: ann[labels.BundleVersionKey], + } + if release, ok := ann[labels.BundleReleaseKey]; ok { + bm.Release = &release + } + return bm +} + +// installFailureReasonFromClusterObjectDeployment derives the Installed=False reason for a +// not-yet-installed extension from the deployment's Progressing reason: a blocking reason maps to +// Failed, while a healthy in-progress rollout maps to Absent. +func installFailureReasonFromClusterObjectDeployment(cod *ocv1.ClusterObjectDeployment) string { + prog := apimeta.FindStatusCondition(cod.Status.Conditions, ocv1.ClusterObjectDeploymentTypeProgressing) + if prog != nil { + switch prog.Reason { + case ocv1.ClusterObjectDeploymentReasonRevisionObjectCollision, + ocv1.ClusterObjectDeploymentReasonRevisionPreconditionsNotMet, + ocv1.ClusterObjectDeploymentReasonRevisionInvalidSpecification, + ocv1.ClusterObjectDeploymentReasonInvalidSpecification, + ocv1.ClusterObjectDeploymentReasonProgressDeadlineExceeded: + return ocv1.ReasonFailed + } + } + return ocv1.ReasonAbsent +} diff --git a/internal/operator-controller/controllers/clusterobjectdeployment_status_test.go b/internal/operator-controller/controllers/clusterobjectdeployment_status_test.go new file mode 100644 index 0000000000..a1f116a12f --- /dev/null +++ b/internal/operator-controller/controllers/clusterobjectdeployment_status_test.go @@ -0,0 +1,276 @@ +/* +Copyright 2026. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controllers + +import ( + "testing" + + "github.com/stretchr/testify/require" + apimeta "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/utils/ptr" + "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" +) + +// codWithProgressing builds a ClusterObjectDeployment whose Progressing condition has the given +// status/reason, observed at the deployment's generation. +func codWithProgressing(name string, generation int64, status metav1.ConditionStatus, reason, message string) *ocv1.ClusterObjectDeployment { + cod := &ocv1.ClusterObjectDeployment{ + ObjectMeta: metav1.ObjectMeta{Name: name, Generation: generation}, + } + apimeta.SetStatusCondition(&cod.Status.Conditions, metav1.Condition{ + Type: ocv1.ClusterObjectDeploymentTypeProgressing, + Status: status, + Reason: reason, + Message: message, + ObservedGeneration: generation, + }) + return cod +} + +func TestProgressingConditionFromClusterObjectDeployment(t *testing.T) { + for _, tc := range []struct { + name string + cod *metav1.Condition + want metav1.Condition + }{ + { + name: "nil deployment condition defaults to RollingOut", + cod: nil, + want: metav1.Condition{Status: metav1.ConditionTrue, Reason: ocv1.ReasonRollingOut, Message: "Revision is rolling out."}, + }, + { + name: "progressing maps to RollingOut", + cod: &metav1.Condition{Status: metav1.ConditionTrue, Reason: ocv1.ClusterObjectDeploymentReasonNewClusterObjectSetProgressing, Message: "latest revision is progressing"}, + want: metav1.Condition{Status: metav1.ConditionTrue, Reason: ocv1.ReasonRollingOut, Message: "latest revision is progressing"}, + }, + { + name: "progressed (deployment False) maps to Succeeded (extension True)", + cod: &metav1.Condition{Status: metav1.ConditionFalse, Reason: ocv1.ClusterObjectDeploymentReasonNewClusterObjectSetProgressed, Message: "latest revision has progressed"}, + want: metav1.Condition{Status: metav1.ConditionTrue, Reason: ocv1.ReasonSucceeded, Message: "Desired state reached"}, + }, + { + name: "object collision (deployment False) maps to Retrying (extension True)", + cod: &metav1.Condition{Status: metav1.ConditionFalse, Reason: ocv1.ClusterObjectDeploymentReasonRevisionObjectCollision, Message: "owned by another controller"}, + want: metav1.Condition{Status: metav1.ConditionTrue, Reason: ocv1.ReasonRetrying, Message: "owned by another controller"}, + }, + { + name: "preconditions not met maps to Retrying", + cod: &metav1.Condition{Status: metav1.ConditionTrue, Reason: ocv1.ClusterObjectDeploymentReasonRevisionPreconditionsNotMet, Message: "waiting on secret"}, + want: metav1.Condition{Status: metav1.ConditionTrue, Reason: ocv1.ReasonRetrying, Message: "waiting on secret"}, + }, + { + name: "progress deadline exceeded passes through", + cod: &metav1.Condition{Status: metav1.ConditionFalse, Reason: ocv1.ClusterObjectDeploymentReasonProgressDeadlineExceeded, Message: "deadline exceeded"}, + want: metav1.Condition{Status: metav1.ConditionFalse, Reason: ocv1.ReasonProgressDeadlineExceeded, Message: "deadline exceeded"}, + }, + { + name: "deployment invalid specification maps to Blocked", + cod: &metav1.Condition{Status: metav1.ConditionFalse, Reason: ocv1.ClusterObjectDeploymentReasonInvalidSpecification, Message: "bad template metadata"}, + want: metav1.Condition{Status: metav1.ConditionFalse, Reason: ocv1.ReasonBlocked, Message: "bad template metadata"}, + }, + { + name: "revision invalid specification maps to Blocked", + cod: &metav1.Condition{Status: metav1.ConditionFalse, Reason: ocv1.ClusterObjectDeploymentReasonRevisionInvalidSpecification, Message: "bad revision spec"}, + want: metav1.Condition{Status: metav1.ConditionFalse, Reason: ocv1.ReasonBlocked, Message: "bad revision spec"}, + }, + } { + t.Run(tc.name, func(t *testing.T) { + got := progressingConditionFromClusterObjectDeployment(tc.cod) + require.Equal(t, ocv1.TypeProgressing, got.Type) + require.Equal(t, tc.want.Status, got.Status, "Status") + require.Equal(t, tc.want.Reason, got.Reason, "Reason") + require.Equal(t, tc.want.Message, got.Message, "Message") + }) + } +} + +func TestSetAvailableFromClusterObjectDeployment(t *testing.T) { + t.Run("available deployment re-emits ProbesSucceeded", func(t *testing.T) { + ext := &ocv1.ClusterExtension{ObjectMeta: metav1.ObjectMeta{Generation: 3}} + cod := &ocv1.ClusterObjectDeployment{} + apimeta.SetStatusCondition(&cod.Status.Conditions, metav1.Condition{ + Type: ocv1.ClusterObjectDeploymentTypeAvailable, Status: metav1.ConditionTrue, + Reason: ocv1.ClusterObjectDeploymentReasonAvailable, Message: "an active revision is ready", + }) + setAvailableFromClusterObjectDeployment(ext, cod) + got := apimeta.FindStatusCondition(ext.Status.Conditions, ocv1.TypeAvailable) + require.NotNil(t, got) + require.Equal(t, metav1.ConditionTrue, got.Status) + require.Equal(t, ocv1.ReasonProbesSucceeded, got.Reason) + require.Equal(t, availableTrueMessage, got.Message) + require.Equal(t, int64(3), got.ObservedGeneration) + }) + + t.Run("unavailable deployment surfaces reason as-is", func(t *testing.T) { + ext := &ocv1.ClusterExtension{} + cod := &ocv1.ClusterObjectDeployment{} + apimeta.SetStatusCondition(&cod.Status.Conditions, metav1.Condition{ + Type: ocv1.ClusterObjectDeploymentTypeAvailable, Status: metav1.ConditionFalse, + Reason: ocv1.ClusterObjectDeploymentReasonUnavailable, Message: "no active revision is ready", + }) + setAvailableFromClusterObjectDeployment(ext, cod) + got := apimeta.FindStatusCondition(ext.Status.Conditions, ocv1.TypeAvailable) + require.NotNil(t, got) + require.Equal(t, metav1.ConditionFalse, got.Status) + require.Equal(t, ocv1.ClusterObjectDeploymentReasonUnavailable, got.Reason) + }) + + t.Run("missing deployment Available condition leaves extension unchanged", func(t *testing.T) { + ext := &ocv1.ClusterExtension{} + setAvailableFromClusterObjectDeployment(ext, &ocv1.ClusterObjectDeployment{}) + require.Nil(t, apimeta.FindStatusCondition(ext.Status.Conditions, ocv1.TypeAvailable)) + }) +} + +func TestClusterObjectDeploymentRolloutComplete(t *testing.T) { + t.Run("complete at current generation", func(t *testing.T) { + require.True(t, clusterObjectDeploymentRolloutComplete( + codWithProgressing("ext", 2, metav1.ConditionFalse, ocv1.ClusterObjectDeploymentReasonNewClusterObjectSetProgressed, ""))) + }) + t.Run("not complete while rolling out", func(t *testing.T) { + require.False(t, clusterObjectDeploymentRolloutComplete( + codWithProgressing("ext", 2, metav1.ConditionTrue, ocv1.ClusterObjectDeploymentReasonNewClusterObjectSetProgressing, ""))) + }) + t.Run("not complete when condition observed a stale generation", func(t *testing.T) { + cod := codWithProgressing("ext", 5, metav1.ConditionFalse, ocv1.ClusterObjectDeploymentReasonNewClusterObjectSetProgressed, "") + // Template advanced to generation 6 but the "progressed" condition is still from generation 5. + cod.Generation = 6 + require.False(t, clusterObjectDeploymentRolloutComplete(cod)) + }) + t.Run("not complete when no Progressing condition", func(t *testing.T) { + require.False(t, clusterObjectDeploymentRolloutComplete(&ocv1.ClusterObjectDeployment{})) + }) +} + +func TestBundleMetadataFromClusterObjectDeployment(t *testing.T) { + cod := &ocv1.ClusterObjectDeployment{} + cod.Spec.Template.Metadata.Annotations = map[string]string{ + labels.BundleNameKey: "test.1.2.0", + labels.BundleVersionKey: "1.2.0", + labels.BundleReleaseKey: "2", + } + bm := bundleMetadataFromClusterObjectDeployment(cod) + require.Equal(t, "test.1.2.0", bm.Name) + require.Equal(t, "1.2.0", bm.Version) + require.Equal(t, ptr.To("2"), bm.Release) + + t.Run("release omitted when annotation absent", func(t *testing.T) { + cod := &ocv1.ClusterObjectDeployment{} + cod.Spec.Template.Metadata.Annotations = map[string]string{labels.BundleNameKey: "x", labels.BundleVersionKey: "1"} + require.Nil(t, bundleMetadataFromClusterObjectDeployment(cod).Release) + }) +} + +func TestSetInstalledFromClusterObjectDeployment(t *testing.T) { + bundleAnnotations := map[string]string{labels.BundleNameKey: "test.1.2.0", labels.BundleVersionKey: "1.2.0"} + + t.Run("completed rollout promotes the deployment template bundle", func(t *testing.T) { + ext := &ocv1.ClusterExtension{ObjectMeta: metav1.ObjectMeta{Generation: 1}} + cod := codWithProgressing("ext", 1, metav1.ConditionFalse, ocv1.ClusterObjectDeploymentReasonNewClusterObjectSetProgressed, "") + cod.Spec.Template.Metadata.Annotations = map[string]string{ + labels.BundleNameKey: "test.1.2.0", + labels.BundleVersionKey: "1.2.0", + labels.BundleReferenceKey: "registry.example/test@sha256:abc", + } + setInstalledFromClusterObjectDeployment(ext, cod) + require.NotNil(t, ext.Status.Install) + require.Equal(t, "test.1.2.0", ext.Status.Install.Bundle.Name) + installed := apimeta.FindStatusCondition(ext.Status.Conditions, ocv1.TypeInstalled) + require.Equal(t, metav1.ConditionTrue, installed.Status) + // The message surfaces the image reference (bundle-reference annotation), not the name. + require.Equal(t, "Installed bundle registry.example/test@sha256:abc successfully", installed.Message) + }) + + t.Run("completed rollout falls back to the bundle name when no image reference", func(t *testing.T) { + ext := &ocv1.ClusterExtension{ObjectMeta: metav1.ObjectMeta{Generation: 1}} + cod := codWithProgressing("ext", 1, metav1.ConditionFalse, ocv1.ClusterObjectDeploymentReasonNewClusterObjectSetProgressed, "") + cod.Spec.Template.Metadata.Annotations = bundleAnnotations + setInstalledFromClusterObjectDeployment(ext, cod) + installed := apimeta.FindStatusCondition(ext.Status.Conditions, ocv1.TypeInstalled) + require.Equal(t, metav1.ConditionTrue, installed.Status) + require.Equal(t, "Installed bundle test.1.2.0 successfully", installed.Message) + }) + + t.Run("in-progress first install reports not installed", func(t *testing.T) { + ext := &ocv1.ClusterExtension{} + cod := codWithProgressing("ext", 1, metav1.ConditionTrue, ocv1.ClusterObjectDeploymentReasonNewClusterObjectSetProgressing, "") + setInstalledFromClusterObjectDeployment(ext, cod) + require.Nil(t, ext.Status.Install) + installed := apimeta.FindStatusCondition(ext.Status.Conditions, ocv1.TypeInstalled) + require.Equal(t, metav1.ConditionFalse, installed.Status) + require.Equal(t, ocv1.ReasonAbsent, installed.Reason) + }) + + t.Run("in-progress upgrade keeps the previously installed bundle", func(t *testing.T) { + // A prior completion recorded v1.2.0; a new revision (v1.3.0) is now rolling out. + ext := &ocv1.ClusterExtension{ + Status: ocv1.ClusterExtensionStatus{Install: &ocv1.ClusterExtensionInstallStatus{ + Bundle: ocv1.BundleMetadata{Name: "test.1.2.0", Version: "1.2.0"}, + }}, + } + cod := codWithProgressing("ext", 2, metav1.ConditionTrue, ocv1.ClusterObjectDeploymentReasonNewClusterObjectSetProgressing, "") + cod.Spec.Template.Metadata.Annotations = map[string]string{labels.BundleNameKey: "test.1.3.0", labels.BundleVersionKey: "1.3.0"} + setInstalledFromClusterObjectDeployment(ext, cod) + require.Equal(t, "test.1.2.0", ext.Status.Install.Bundle.Name, "installed bundle must not advance until the rollout completes") + }) + + t.Run("blocked first install reports Failed", func(t *testing.T) { + ext := &ocv1.ClusterExtension{} + cod := codWithProgressing("ext", 1, metav1.ConditionFalse, ocv1.ClusterObjectDeploymentReasonRevisionObjectCollision, "collision") + setInstalledFromClusterObjectDeployment(ext, cod) + require.Equal(t, ocv1.ReasonFailed, apimeta.FindStatusCondition(ext.Status.Conditions, ocv1.TypeInstalled).Reason) + }) +} + +func TestReflectClusterObjectDeploymentStatus(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, ocv1.AddToScheme(scheme)) + + t.Run("maps a completed deployment onto the extension", func(t *testing.T) { + cod := codWithProgressing("test-ext", 1, metav1.ConditionFalse, ocv1.ClusterObjectDeploymentReasonNewClusterObjectSetProgressed, "latest revision has progressed") + apimeta.SetStatusCondition(&cod.Status.Conditions, metav1.Condition{ + Type: ocv1.ClusterObjectDeploymentTypeAvailable, Status: metav1.ConditionTrue, + Reason: ocv1.ClusterObjectDeploymentReasonAvailable, ObservedGeneration: 1, + }) + cod.Spec.Template.Metadata.Annotations = map[string]string{labels.BundleNameKey: "test.1.2.0", labels.BundleVersionKey: "1.2.0"} + cl := fake.NewClientBuilder().WithScheme(scheme).WithObjects(cod).Build() + ext := &ocv1.ClusterExtension{ObjectMeta: metav1.ObjectMeta{Name: "test-ext", Generation: 1}} + + res, err := ReflectClusterObjectDeploymentStatus(cl)(t.Context(), &reconcileState{}, ext) + require.NoError(t, err) + require.Nil(t, res) + require.Equal(t, ocv1.ReasonProbesSucceeded, apimeta.FindStatusCondition(ext.Status.Conditions, ocv1.TypeAvailable).Reason) + require.Equal(t, ocv1.ReasonSucceeded, apimeta.FindStatusCondition(ext.Status.Conditions, ocv1.TypeProgressing).Reason) + require.Equal(t, "test.1.2.0", ext.Status.Install.Bundle.Name) + }) + + t.Run("absent deployment reports rolling out and not installed", func(t *testing.T) { + cl := fake.NewClientBuilder().WithScheme(scheme).Build() + ext := &ocv1.ClusterExtension{ObjectMeta: metav1.ObjectMeta{Name: "test-ext"}} + + res, err := ReflectClusterObjectDeploymentStatus(cl)(t.Context(), &reconcileState{}, ext) + require.NoError(t, err) + require.Nil(t, res) + require.Equal(t, ocv1.ReasonRollingOut, apimeta.FindStatusCondition(ext.Status.Conditions, ocv1.TypeProgressing).Reason) + require.Equal(t, metav1.ConditionFalse, apimeta.FindStatusCondition(ext.Status.Conditions, ocv1.TypeInstalled).Status) + }) +} diff --git a/internal/operator-controller/controllers/common_controller.go b/internal/operator-controller/controllers/common_controller.go index 58c98fc1d6..3e982379b7 100644 --- a/internal/operator-controller/controllers/common_controller.go +++ b/internal/operator-controller/controllers/common_controller.go @@ -69,60 +69,6 @@ func setInstalledStatusFromRevisionStates(ext *ocv1.ClusterExtension, revisionSt setInstalledStatusConditionSuccess(ext, fmt.Sprintf("Installed bundle %s successfully", revisionStates.Installed.Image)) } -// availableTrueMessage is the message surfaced on the ClusterExtension Available condition when the -// active revision is Ready. It is intentionally decoupled from the ClusterObjectSet Ready condition's -// own message. -const availableTrueMessage = "Objects are available and pass all probes." - -// availableFromReady maps a ClusterObjectSet Ready condition onto a ClusterExtension Available condition. -// Non-ready states are surfaced as-is (only the Type is rewritten to Available). The ready (True) state is -// re-emitted with a ClusterExtension-owned reason and message. -func availableFromReady(ready metav1.Condition, generation int64) metav1.Condition { - a := ready - a.Type = ocv1.TypeAvailable - a.ObservedGeneration = generation - if a.Status == metav1.ConditionTrue { - a.Reason = ocv1.ReasonProbesSucceeded - a.Message = availableTrueMessage - } - return a -} - -// setAvailableFromRevisionStates sets the Available status condition based on the given revision states -func setAvailableFromRevisionStates(ext *ocv1.ClusterExtension, revisionStates *RevisionStates) { - if i := revisionStates.Installed; i != nil { - ready := apimeta.FindStatusCondition(i.Conditions, ocv1.ClusterObjectSetTypeReady) - if ready != nil { - apimeta.SetStatusCondition(&ext.Status.Conditions, availableFromReady(*ready, ext.GetGeneration())) - } - } -} - -// setProgressingFromRevisionStates sets the Progressing status condition based on the given revision states -// The Progressing condition is derived from the Ready condition of either: -// - the latest rolling out revision (when one exists); OR -// - the installed revision -// When the source revision has no Ready condition yet (e.g. a freshly created revision that has -// not reconciled), a nil condition is passed so the helper applies the RollingOut default rather than -// leaving Progressing stale. -func setProgressingFromRevisionStates(ext *ocv1.ClusterExtension, revisionStates *RevisionStates) { - if len(revisionStates.RollingOut) > 0 { - revisionMeta := revisionStates.RollingOut[len(revisionStates.RollingOut)-1] - readyCond := apimeta.FindStatusCondition(revisionMeta.Conditions, ocv1.ClusterObjectSetTypeReady) - setProgressingFromReady(ext, readyCond, false) - } else if revisionStates.Installed != nil { - setProgressingFromReady(ext, apimeta.FindStatusCondition(revisionStates.Installed.Conditions, ocv1.ClusterObjectSetTypeReady), true) - } -} - -// setProgressingFromReady derives the correct Progressing condition for the ClusterExtension from the given -// revision Ready condition, and whether the revision is complete or not -func setProgressingFromReady(ext *ocv1.ClusterExtension, readyCond *metav1.Condition, isRevisionCompleted bool) { - prog := progressingFromReady(readyCond, isRevisionCompleted) - prog.ObservedGeneration = ext.GetGeneration() - apimeta.SetStatusCondition(&ext.Status.Conditions, prog) -} - // determineFailureReason determines the appropriate reason for the Installed condition // when no bundle is installed (Installed: False). // @@ -231,46 +177,3 @@ func setStatusProgressing(ext *ocv1.ClusterExtension, err error) { SetStatusCondition(&ext.Status.Conditions, progressingCond) } - -// progressingFromReady reconstructs the ClusterExtension Progressing condition -// for a single revision from that revision's ClusterObjectSet Ready condition -// and whether the revision has completed its rollout (status.completedAt set). -// -// This replaces the previous behavior of mirroring the ClusterObjectSet Progressing -// condition, which no longer exists. ObservedGeneration is left unset; callers set it. -func progressingFromReady(ready *metav1.Condition, completed bool) metav1.Condition { - cond := metav1.Condition{ - Type: ocv1.TypeProgressing, - Status: metav1.ConditionTrue, - } - if completed { - cond.Reason = ocv1.ReasonSucceeded - cond.Message = "Desired state reached" - return cond - } - if ready == nil { - cond.Reason = ocv1.ReasonRollingOut - cond.Message = "Revision is rolling out." - return cond - } - cond.Message = ready.Message - switch ready.Reason { - case ocv1.ClusterObjectSetReasonInvalidSpecification: - // An invalid revision spec needs a spec change to proceed; the revision does not - // retry on its own, so report it as a terminal Progressing=False/Blocked. - cond.Status = metav1.ConditionFalse - cond.Reason = ocv1.ReasonBlocked - case ocv1.ReasonProgressDeadlineExceeded: - cond.Status = metav1.ConditionFalse - cond.Reason = ocv1.ReasonProgressDeadlineExceeded - case ocv1.ClusterObjectSetReasonObjectCollision, ocv1.ClusterObjectSetReasonPreconditionsNotMet: - // A foreign-owned object (ObjectCollision) or an unmet preflight precondition - // (PreconditionsNotMet) prevents the rollout from proceeding, but the revision keeps - // retrying and recovers once the obstruction clears. - cond.Reason = ocv1.ReasonRetrying - default: - // Any other (non-error) reason on a not-yet-complete revision is rolling out. - cond.Reason = ocv1.ReasonRollingOut - } - return cond -} diff --git a/internal/operator-controller/controllers/common_controller_test.go b/internal/operator-controller/controllers/common_controller_test.go index 3882d38e6c..a67a3309c2 100644 --- a/internal/operator-controller/controllers/common_controller_test.go +++ b/internal/operator-controller/controllers/common_controller_test.go @@ -512,153 +512,3 @@ func TestDetermineFailureReason(t *testing.T) { }) } } - -func TestProgressingFromReady(t *testing.T) { - for _, tc := range []struct { - name string - ready *metav1.Condition - completed bool - expected metav1.Condition - }{ - { - name: "completed revision maps to Succeeded", - ready: &metav1.Condition{Type: ocv1.ClusterObjectSetTypeReady, Status: metav1.ConditionTrue, Reason: ocv1.ClusterObjectSetReasonAllObjectsReady, Message: "ok"}, - completed: true, - expected: metav1.Condition{Type: ocv1.TypeProgressing, Status: metav1.ConditionTrue, Reason: ocv1.ReasonSucceeded, Message: "Desired state reached"}, - }, - { - name: "nil ready on rolling revision defaults to RollingOut", - ready: nil, - completed: false, - expected: metav1.Condition{Type: ocv1.TypeProgressing, Status: metav1.ConditionTrue, Reason: ocv1.ReasonRollingOut, Message: "Revision is rolling out."}, - }, - { - name: "objects not ready maps to RollingOut", - ready: &metav1.Condition{Type: ocv1.ClusterObjectSetTypeReady, Status: metav1.ConditionFalse, Reason: ocv1.ClusterObjectSetReasonObjectsNotReady, Message: "Objects are not yet ready."}, - completed: false, - expected: metav1.Condition{Type: ocv1.TypeProgressing, Status: metav1.ConditionTrue, Reason: ocv1.ReasonRollingOut, Message: "Objects are not yet ready."}, - }, - { - name: "superseded maps to RollingOut", - ready: &metav1.Condition{Type: ocv1.ClusterObjectSetTypeReady, Status: metav1.ConditionFalse, Reason: ocv1.ClusterObjectSetReasonSuperseded, Message: "Object(s) have been adopted by a newer revision of the object set."}, - completed: false, - expected: metav1.Condition{Type: ocv1.TypeProgressing, Status: metav1.ConditionTrue, Reason: ocv1.ReasonRollingOut, Message: "Object(s) have been adopted by a newer revision of the object set."}, - }, - { - name: "archiving maps to RollingOut", - ready: &metav1.Condition{Type: ocv1.ClusterObjectSetTypeReady, Status: metav1.ConditionFalse, Reason: ocv1.ClusterObjectSetReasonArchiving, Message: "The object set is being archived; removing owned objects."}, - completed: false, - expected: metav1.Condition{Type: ocv1.TypeProgressing, Status: metav1.ConditionTrue, Reason: ocv1.ReasonRollingOut, Message: "The object set is being archived; removing owned objects."}, - }, - { - name: "object collision maps to Retrying", - ready: &metav1.Condition{Type: ocv1.ClusterObjectSetTypeReady, Status: metav1.ConditionUnknown, Reason: ocv1.ClusterObjectSetReasonObjectCollision, Message: "boom"}, - completed: false, - expected: metav1.Condition{Type: ocv1.TypeProgressing, Status: metav1.ConditionTrue, Reason: ocv1.ReasonRetrying, Message: "boom"}, - }, - { - name: "invalid specification maps to Blocked", - ready: &metav1.Condition{Type: ocv1.ClusterObjectSetTypeReady, Status: metav1.ConditionFalse, Reason: ocv1.ClusterObjectSetReasonInvalidSpecification, Message: "manual fix needed"}, - completed: false, - expected: metav1.Condition{Type: ocv1.TypeProgressing, Status: metav1.ConditionFalse, Reason: ocv1.ReasonBlocked, Message: "manual fix needed"}, - }, - { - name: "preconditions not met maps to Retrying", - ready: &metav1.Condition{Type: ocv1.ClusterObjectSetTypeReady, Status: metav1.ConditionFalse, Reason: ocv1.ClusterObjectSetReasonPreconditionsNotMet, Message: "waiting on a precondition"}, - completed: false, - expected: metav1.Condition{Type: ocv1.TypeProgressing, Status: metav1.ConditionTrue, Reason: ocv1.ReasonRetrying, Message: "waiting on a precondition"}, - }, - { - name: "deadline exceeded maps to ProgressDeadlineExceeded", - ready: &metav1.Condition{Type: ocv1.ClusterObjectSetTypeReady, Status: metav1.ConditionFalse, Reason: ocv1.ReasonProgressDeadlineExceeded, Message: "too slow"}, - completed: false, - expected: metav1.Condition{Type: ocv1.TypeProgressing, Status: metav1.ConditionFalse, Reason: ocv1.ReasonProgressDeadlineExceeded, Message: "too slow"}, - }, - } { - t.Run(tc.name, func(t *testing.T) { - got := progressingFromReady(tc.ready, tc.completed) - require.Equal(t, tc.expected.Type, got.Type) - require.Equal(t, tc.expected.Status, got.Status) - require.Equal(t, tc.expected.Reason, got.Reason) - require.Equal(t, tc.expected.Message, got.Message) - }) - } -} - -func TestSetAvailableFromRevisionStates(t *testing.T) { - for _, tc := range []struct { - name string - revisionStates *RevisionStates - // expectAvailable, when non-nil, is the Available condition expected on the ClusterExtension. - // When nil, the ClusterExtension must not surface an Available condition at all. - expectAvailable *metav1.Condition - }{ - { - name: "installed revision with Ready=True is remapped to Available=True with a decoupled reason and message", - revisionStates: &RevisionStates{ - Installed: &RevisionMetadata{ - RevisionName: "rev-1", - Conditions: []metav1.Condition{{ - Type: ocv1.ClusterObjectSetTypeReady, - Status: metav1.ConditionTrue, - Reason: ocv1.ClusterObjectSetReasonAllObjectsReady, - Message: "All objects are at the desired state", - }}, - }, - }, - // The ClusterExtension Available condition is deliberately decoupled from the - // ClusterObjectSet Ready condition's reason/message for the ready (True) state. - expectAvailable: &metav1.Condition{ - Status: metav1.ConditionTrue, - Reason: ocv1.ReasonProbesSucceeded, - Message: "Objects are available and pass all probes.", - }, - }, - { - name: "installed revision with Ready=False carries status, reason and message through", - revisionStates: &RevisionStates{ - Installed: &RevisionMetadata{ - RevisionName: "rev-1", - Conditions: []metav1.Condition{{ - Type: ocv1.ClusterObjectSetTypeReady, - Status: metav1.ConditionFalse, - Reason: ocv1.ClusterObjectSetReasonObjectsNotReady, - Message: "probe failed", - }}, - }, - }, - expectAvailable: &metav1.Condition{ - Status: metav1.ConditionFalse, - Reason: ocv1.ClusterObjectSetReasonObjectsNotReady, - Message: "probe failed", - }, - }, - { - name: "no installed revision sets no Available condition", - revisionStates: &RevisionStates{}, - expectAvailable: nil, - }, - } { - t.Run(tc.name, func(t *testing.T) { - ext := &ocv1.ClusterExtension{ObjectMeta: metav1.ObjectMeta{Name: "test-ext", Generation: 7}} - - setAvailableFromRevisionStates(ext, tc.revisionStates) - - // The ClusterExtension must never surface the ClusterObjectSet's own Ready condition type. - require.Nil(t, meta.FindStatusCondition(ext.Status.Conditions, ocv1.ClusterObjectSetTypeReady), - "CE must not expose the COS Ready condition type directly") - - got := meta.FindStatusCondition(ext.Status.Conditions, ocv1.TypeAvailable) - if tc.expectAvailable == nil { - require.Nil(t, got, "CE must not expose an Available condition") - return - } - require.NotNil(t, got, "CE must expose an Available condition") - require.Equal(t, tc.expectAvailable.Status, got.Status) - require.Equal(t, tc.expectAvailable.Reason, got.Reason) - require.Equal(t, tc.expectAvailable.Message, got.Message) - require.Equal(t, ext.GetGeneration(), got.ObservedGeneration, - "Available condition should observe the ClusterExtension generation") - }) - } -} 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..5e0022c3c1 100644 --- a/internal/testutil/mock/applier/mock_applier.go +++ b/internal/testutil/mock/applier/mock_applier.go @@ -1,9 +1,9 @@ // Code generated by MockGen. DO NOT EDIT. -// Source: github.com/operator-framework/operator-controller/internal/operator-controller/applier (interfaces: Preflight,HelmReleaseToObjectsConverterInterface,HelmChartProvider,ClusterObjectSetGenerator,ManifestProvider) +// Source: github.com/operator-framework/operator-controller/internal/operator-controller/applier (interfaces: Preflight,HelmReleaseToObjectsConverterInterface,HelmChartProvider,RevisionGenerator,ManifestProvider) // // Generated by this command: // -// mockgen -destination=applier/mock_applier.go -package=applier github.com/operator-framework/operator-controller/internal/operator-controller/applier Preflight,HelmReleaseToObjectsConverterInterface,HelmChartProvider,ClusterObjectSetGenerator,ManifestProvider +// mockgen -destination=applier/mock_applier.go -package=applier github.com/operator-framework/operator-controller/internal/operator-controller/applier Preflight,HelmReleaseToObjectsConverterInterface,HelmChartProvider,RevisionGenerator,ManifestProvider // // Package applier is a generated GoMock package. @@ -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" ) @@ -152,47 +153,48 @@ func (mr *MockHelmChartProviderMockRecorder) Get(bundle, clusterExtension any) * return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Get", reflect.TypeOf((*MockHelmChartProvider)(nil).Get), bundle, clusterExtension) } -// MockClusterObjectSetGenerator is a mock of ClusterObjectSetGenerator interface. -type MockClusterObjectSetGenerator struct { +// MockRevisionGenerator is a mock of RevisionGenerator interface. +type MockRevisionGenerator struct { ctrl *gomock.Controller - recorder *MockClusterObjectSetGeneratorMockRecorder + recorder *MockRevisionGeneratorMockRecorder isgomock struct{} } -// MockClusterObjectSetGeneratorMockRecorder is the mock recorder for MockClusterObjectSetGenerator. -type MockClusterObjectSetGeneratorMockRecorder struct { - mock *MockClusterObjectSetGenerator +// MockRevisionGeneratorMockRecorder is the mock recorder for MockRevisionGenerator. +type MockRevisionGeneratorMockRecorder struct { + mock *MockRevisionGenerator } -// NewMockClusterObjectSetGenerator creates a new mock instance. -func NewMockClusterObjectSetGenerator(ctrl *gomock.Controller) *MockClusterObjectSetGenerator { - mock := &MockClusterObjectSetGenerator{ctrl: ctrl} - mock.recorder = &MockClusterObjectSetGeneratorMockRecorder{mock} +// NewMockRevisionGenerator creates a new mock instance. +func NewMockRevisionGenerator(ctrl *gomock.Controller) *MockRevisionGenerator { + mock := &MockRevisionGenerator{ctrl: ctrl} + mock.recorder = &MockRevisionGeneratorMockRecorder{mock} return mock } // EXPECT returns an object that allows the caller to indicate expected use. -func (m *MockClusterObjectSetGenerator) EXPECT() *MockClusterObjectSetGeneratorMockRecorder { +func (m *MockRevisionGenerator) EXPECT() *MockRevisionGeneratorMockRecorder { 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 *MockRevisionGenerator) 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 *MockRevisionGeneratorMockRecorder) 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((*MockRevisionGenerator)(nil).GenerateClusterObjectDeployment), ctx, bundleFS, ext, objectLabels, revisionAnnotations) } // GenerateRevisionFromHelmRelease mocks base method. -func (m *MockClusterObjectSetGenerator) GenerateRevisionFromHelmRelease(ctx context.Context, helmRelease *release.Release, ext *v1.ClusterExtension, objectLabels map[string]string) (*v10.ClusterObjectSetApplyConfiguration, error) { +func (m *MockRevisionGenerator) GenerateRevisionFromHelmRelease(ctx context.Context, helmRelease *release.Release, ext *v1.ClusterExtension, objectLabels map[string]string) (*v10.ClusterObjectSetApplyConfiguration, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "GenerateRevisionFromHelmRelease", ctx, helmRelease, ext, objectLabels) ret0, _ := ret[0].(*v10.ClusterObjectSetApplyConfiguration) @@ -201,9 +203,9 @@ func (m *MockClusterObjectSetGenerator) GenerateRevisionFromHelmRelease(ctx cont } // GenerateRevisionFromHelmRelease indicates an expected call of GenerateRevisionFromHelmRelease. -func (mr *MockClusterObjectSetGeneratorMockRecorder) GenerateRevisionFromHelmRelease(ctx, helmRelease, ext, objectLabels any) *gomock.Call { +func (mr *MockRevisionGeneratorMockRecorder) GenerateRevisionFromHelmRelease(ctx, helmRelease, ext, objectLabels any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GenerateRevisionFromHelmRelease", reflect.TypeOf((*MockClusterObjectSetGenerator)(nil).GenerateRevisionFromHelmRelease), ctx, helmRelease, ext, objectLabels) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GenerateRevisionFromHelmRelease", reflect.TypeOf((*MockRevisionGenerator)(nil).GenerateRevisionFromHelmRelease), ctx, helmRelease, ext, objectLabels) } // MockManifestProvider is a mock of ManifestProvider interface. diff --git a/internal/testutil/mock/generate.go b/internal/testutil/mock/generate.go index 35338a0dd4..25e0e5f495 100644 --- a/internal/testutil/mock/generate.go +++ b/internal/testutil/mock/generate.go @@ -31,7 +31,7 @@ package mock //go:generate mockgen -destination=catalogdservice/mock_graphqlservice.go -package=catalogdservice github.com/operator-framework/operator-controller/internal/catalogd/service GraphQLService // Internal interfaces — operator-controller applier -//go:generate mockgen -destination=applier/mock_applier.go -package=applier github.com/operator-framework/operator-controller/internal/operator-controller/applier Preflight,HelmReleaseToObjectsConverterInterface,HelmChartProvider,ClusterObjectSetGenerator,ManifestProvider +//go:generate mockgen -destination=applier/mock_applier.go -package=applier github.com/operator-framework/operator-controller/internal/operator-controller/applier Preflight,HelmReleaseToObjectsConverterInterface,HelmChartProvider,RevisionGenerator,ManifestProvider // Internal interfaces — operator-controller catalogmetadata //go:generate mockgen -destination=catalogclient/mock_cache.go -package=catalogclient github.com/operator-framework/operator-controller/internal/operator-controller/catalogmetadata/client Cache diff --git a/test/e2e/features/group.feature b/test/e2e/features/group.feature index 82d3da2d9e..143cb33eb9 100644 --- a/test/e2e/features/group.feature +++ b/test/e2e/features/group.feature @@ -221,6 +221,8 @@ Feature: Track ClusterObjectSet revisions by group And ClusterObjectSet "${NAME}-7" has group "${NAME}" And resource "clusterobjectset/${NAME}-1" is eventually not found # The existing retention policy keeps five prior revisions plus the current one. - And ClusterExtension "${NAME}" owns 6 ClusterObjectSets + # operator-controller installs through a ClusterObjectDeployment, so the deployment + # (named after the ClusterExtension) is the controller owner of the group's revisions. + And ClusterObjectDeployment "${NAME}" owns 6 ClusterObjectSets And resource "clusterobjectset/${COS_NAME}" exists And ClusterObjectSet "${COS_NAME}" has group "${NAME}-other" diff --git a/test/e2e/features/install.feature b/test/e2e/features/install.feature index 6186406fcb..c315f50d69 100644 --- a/test/e2e/features/install.feature +++ b/test/e2e/features/install.feature @@ -351,7 +351,9 @@ Feature: Install ClusterExtension | package | version | channel | replaces | contents | | test | 1.0.2 | alpha | | BadImage | And min value for ClusterExtension .spec.progressDeadlineMinutes is set to 1 - And min value for ClusterObjectSet .spec.progressDeadlineMinutes is set to 1 + # The applier routes progressDeadlineMinutes through the ClusterObjectDeployment template; the + # deployment enforces the rollout deadline itself, so relax its CRD minimum to accept the value of 1. + And min value for ClusterObjectDeployment .spec.template.spec.progressDeadlineMinutes is set to 1 When ClusterExtension is applied """ apiVersion: olm.operatorframework.io/v1 @@ -370,10 +372,12 @@ Feature: Install ClusterExtension matchLabels: "olm.operatorframework.io/metadata.name": ${CATALOG:test} """ - Then ClusterObjectSet "${NAME}-1" reports Ready as False with Reason ProgressDeadlineExceeded + # Post-#3055 the deployment enforces the deadline itself (its own Progressing condition); the + # ClusterObjectSet revisions it creates do not carry progressDeadlineMinutes. + Then ClusterObjectDeployment "${NAME}" reports Progressing as False with Reason ProgressDeadlineExceeded And ClusterExtension reports Progressing as False with Reason ProgressDeadlineExceeded and Message: """ - Revision has not rolled out for 1 minute(s). Last status: Objects are not yet ready. + latest revision exceeded its progress deadline """ And ClusterExtension reports Progressing transition between 1 and 2 minutes since ClusterObjectSet "${NAME}-1" was created @@ -384,7 +388,9 @@ Feature: Install ClusterExtension | package | version | channel | replaces | contents | | test | 1.0.3 | alpha | | BadImage | And min value for ClusterExtension .spec.progressDeadlineMinutes is set to 1 - And min value for ClusterObjectSet .spec.progressDeadlineMinutes is set to 1 + # The applier routes progressDeadlineMinutes through the ClusterObjectDeployment template; the + # deployment enforces the rollout deadline itself, so relax its CRD minimum to accept the value of 1. + And min value for ClusterObjectDeployment .spec.template.spec.progressDeadlineMinutes is set to 1 When ClusterExtension is applied """ apiVersion: olm.operatorframework.io/v1 @@ -404,10 +410,12 @@ Feature: Install ClusterExtension matchLabels: "olm.operatorframework.io/metadata.name": ${CATALOG:test} """ - Then ClusterObjectSet "${NAME}-1" reports Ready as False with Reason ProgressDeadlineExceeded + # Post-#3055 the deployment enforces the deadline itself (its own Progressing condition); the + # ClusterObjectSet revisions it creates do not carry progressDeadlineMinutes. + Then ClusterObjectDeployment "${NAME}" reports Progressing as False with Reason ProgressDeadlineExceeded And ClusterExtension reports Progressing as False with Reason ProgressDeadlineExceeded and Message: """ - Revision has not rolled out for 1 minute(s). Last status: Objects are not yet ready. + latest revision exceeded its progress deadline """ And ClusterExtension reports Progressing transition between 1 and 2 minutes since ClusterObjectSet "${NAME}-1" was created @@ -440,7 +448,7 @@ Feature: Install ClusterExtension """ @BoxcutterRuntime - Scenario: ClusterObjectSet is labeled with owner information + Scenario: ClusterExtension installs via a ClusterObjectDeployment that owns the revision Given a catalog "test" with packages: | package | version | channel | replaces | contents | | test | 1.2.0 | beta | | CRD, Deployment, ConfigMap | @@ -463,10 +471,22 @@ Feature: Install ClusterExtension """ Then ClusterExtension is rolled out And ClusterExtension is available + # The ClusterExtension records the installed bundle identity and rolls revision health up + # onto its own conditions: Progressing (Succeeded) and Available (ProbesSucceeded, mapped + # from the active ClusterObjectSet's Ready condition). + And bundle "${PACKAGE:test}.1.2.0" is installed in version "1.2.0" + And ClusterExtension reports Progressing as True with Reason Succeeded + And ClusterExtension reports Available as True with Reason ProbesSucceeded + # operator-controller installs through a ClusterObjectDeployment; the ownership chain is + # ClusterExtension -> ClusterObjectDeployment -> ClusterObjectSet revision. + And ClusterObjectDeployment "${NAME}" reports Available as True with Reason Available + And ClusterObjectDeployment "${NAME}" reports Progressing as False with Reason NewClusterObjectSetProgressed + And ClusterObjectDeployment "${NAME}" is owned by ClusterExtension "${NAME}" + And ClusterObjectDeployment "${NAME}" owns 1 ClusterObjectSet + And ClusterObjectSet "${NAME}-1" reports Ready as True with Reason AllObjectsReady And ClusterObjectSet "${NAME}-1" has label "olm.operatorframework.io/owner-kind" with value "ClusterExtension" And ClusterObjectSet "${NAME}-1" has label "olm.operatorframework.io/owner-name" with value "${NAME}" And ClusterObjectSet "${NAME}-1" has group "${NAME}" - And ClusterExtension "${NAME}" owns 1 ClusterObjectSet @BoxcutterRuntime Scenario: ClusterObjectSet objects are externalized to immutable Secrets @@ -496,10 +516,9 @@ Feature: Install ClusterExtension And ClusterObjectSet "${NAME}-1" referred secrets exist in "${OLM_NAMESPACE}" namespace And ClusterObjectSet "${NAME}-1" referred secrets are immutable And ClusterObjectSet "${NAME}-1" referred secrets contain labels - | key | value | - | olm.operatorframework.io/revision-name | ${NAME}-1 | - | olm.operatorframework.io/owner-name | ${NAME} | - And ClusterObjectSet "${NAME}-1" referred secrets are owned by the object set + | key | value | + | olm.operatorframework.io/owner-name | ${NAME} | + And ClusterObjectSet "${NAME}-1" referred secrets are owned by ClusterExtension "${NAME}" And ClusterObjectSet "${NAME}-1" referred secrets have type "olm.operatorframework.io/object-data" @DeploymentConfig diff --git a/test/e2e/features/status.feature b/test/e2e/features/status.feature index f4b688cba5..0322095651 100644 --- a/test/e2e/features/status.feature +++ b/test/e2e/features/status.feature @@ -30,16 +30,21 @@ Feature: Report status of the managed ClusterExtension workload And ClusterExtension is rolled out And ClusterExtension is available + # The ClusterExtension Available condition is derived from the ClusterObjectDeployment's + # (summarized) Available condition, which reports Reason=Unavailable when no active revision + # is ready. The more granular revision-level reason (ObjectsNotReady) is still available on the + # ClusterObjectSet. TODO: surface the degraded revision's reason on the COD's Available + # condition so the ClusterExtension can report it too (tracked for a future PR). @BoxcutterRuntime Scenario: Report availability change when managed workload is not ready When deployment "test-operator-${SCENARIO_ID}" reports as not ready - Then ClusterExtension reports Available as False with Reason ObjectsNotReady + Then ClusterExtension reports Available as False with Reason Unavailable And ClusterObjectSet "${NAME}-1" reports Ready as False with Reason ObjectsNotReady @BoxcutterRuntime Scenario: Report availability change when managed workload restores its readiness Given deployment "test-operator-${SCENARIO_ID}" reports as not ready - And ClusterExtension reports Available as False with Reason ObjectsNotReady + And ClusterExtension reports Available as False with Reason Unavailable And ClusterObjectSet "${NAME}-1" reports Ready as False with Reason ObjectsNotReady When deployment "test-operator-${SCENARIO_ID}" reports as ready Then ClusterExtension is available diff --git a/test/e2e/features/uninstall.feature b/test/e2e/features/uninstall.feature index 973d90ab5a..1d95cfdd9b 100644 --- a/test/e2e/features/uninstall.feature +++ b/test/e2e/features/uninstall.feature @@ -35,12 +35,16 @@ Feature: Uninstall ClusterExtension Then the ClusterExtension's constituent resources are removed @BoxcutterRuntime - Scenario: Removing ClusterExtension cascades to grouped revisions and their content Secrets + Scenario: Removing ClusterExtension cascades to its ClusterObjectDeployment, revisions, and content Secrets + # Ownership chain is ClusterExtension -> ClusterObjectDeployment -> ClusterObjectSet, and the + # ClusterExtension owns the content Secrets directly, so deleting the ClusterExtension + # garbage-collects the whole tree including the Secrets. Given ClusterObjectSet "${NAME}-1" has group "${NAME}" - And ClusterExtension "${NAME}" owns 1 ClusterObjectSet - And ClusterObjectSet "${NAME}-1" referred secrets are owned by the object set + And ClusterObjectDeployment "${NAME}" is owned by ClusterExtension "${NAME}" + And ClusterObjectDeployment "${NAME}" owns 1 ClusterObjectSet And ClusterObjectSet "${NAME}-1" referred secrets are remembered When ClusterExtension is removed Then resource "clusterobjectset/${NAME}-1" is eventually not found + And resource "clusterobjectdeployment/${NAME}" is eventually not found And the ClusterExtension's constituent resources are removed And the remembered revision secrets are removed diff --git a/test/e2e/features/update.feature b/test/e2e/features/update.feature index 222efd4130..7fad40d097 100644 --- a/test/e2e/features/update.feature +++ b/test/e2e/features/update.feature @@ -253,7 +253,7 @@ Feature: Update ClusterExtension """ Then ClusterExtension "${NAME}-dup" reports Progressing as True with Reason Retrying and Message includes: """ - because they are owned by another controller or already exist + owned by another controller or already present """ And ClusterExtension "${NAME}" reports Installed as True # Force a second revision on the dup via env var change — collision must persist @@ -281,10 +281,10 @@ Feature: Update ClusterExtension "olm.operatorframework.io/metadata.name": ${CATALOG:test} version: 1.0.1 """ - Then ClusterExtension "${NAME}-dup" owns 2 ClusterObjectSets + Then ClusterObjectDeployment "${NAME}-dup" owns 2 ClusterObjectSets And ClusterExtension "${NAME}-dup" reports Progressing as True with Reason Retrying and Message includes: """ - because they are owned by another controller or already exist + owned by another controller or already present """ And ClusterExtension "${NAME}" reports Installed as True diff --git a/test/e2e/steps/steps.go b/test/e2e/steps/steps.go index 123c93213f..6dd2f0071e 100644 --- a/test/e2e/steps/steps.go +++ b/test/e2e/steps/steps.go @@ -154,6 +154,7 @@ func RegisterSteps(sc *godog.ScenarioContext) { sc.Step(`^(?i)ClusterObjectSet "([^"]+)" referred secrets are immutable$`, ClusterObjectSetReferredSecretsAreImmutable) sc.Step(`^(?i)ClusterObjectSet "([^"]+)" referred secrets contain labels$`, ClusterObjectSetReferredSecretsContainLabels) sc.Step(`^(?i)ClusterObjectSet "([^"]+)" referred secrets are owned by the object set$`, ClusterObjectSetReferredSecretsOwnedByObjectSet) + sc.Step(`^(?i)ClusterObjectSet "([^"]+)" referred secrets are owned by ClusterExtension "([^"]+)"$`, ClusterObjectSetReferredSecretsOwnedByClusterExtension) sc.Step(`^(?i)ClusterObjectSet "([^"]+)" referred secrets have type "([^"]+)"$`, ClusterObjectSetReferredSecretsHaveType) sc.Step(`^(?i)resource "([^"]+)" is installed$`, ResourceAvailable) @@ -210,6 +211,8 @@ func RegisterSteps(sc *godog.ScenarioContext) { sc.Step(`^(?i)min value for (ClusterExtension|ClusterObjectSet|ClusterObjectDeployment) ((?:\.[a-zA-Z]+)+) is set to (\d+)$`, SetCRDFieldMinValue) sc.Step(`^(?i)ClusterExtension "([^"]+)" owns (\d+) ClusterObjectSets?$`, ClusterExtensionOwnsClusterObjectSets) + sc.Step(`^(?i)ClusterObjectDeployment "([^"]+)" owns (\d+) ClusterObjectSets?$`, ClusterObjectDeploymentOwnsClusterObjectSets) + sc.Step(`^(?i)ClusterObjectDeployment "([^"]+)" is owned by ClusterExtension "([^"]+)"$`, ClusterObjectDeploymentIsOwnedByClusterExtension) sc.Step(`^(?i)ClusterExtension "([^"]+)" reports ([[:alnum:]]+) as ([[:alnum:]]+)$`, NamedClusterExtensionReportsCondition) sc.Step(`^(?i)ClusterExtension "([^"]+)" reports ([[:alnum:]]+) as ([[:alnum:]]+) with Reason ([[:alnum:]]+) and Message includes:$`, NamedClusterExtensionReportsConditionWithMessageFragment) @@ -630,6 +633,52 @@ func ClusterExtensionOwnsClusterObjectSets(ctx context.Context, extName string, return nil } +// ClusterObjectDeploymentOwnsClusterObjectSets waits until the named ClusterObjectDeployment is the +// controller owner of exactly expectedCount ClusterObjectSet revisions in its group. +func ClusterObjectDeploymentOwnsClusterObjectSets(ctx context.Context, codName string, expectedCount int) error { + sc := scenarioCtx(ctx) + codName = substituteScenarioVars(codName, sc) + waitFor(ctx, func() bool { + revisions, err := listClusterObjectSetsInGroup(ctx, codName) + if err != nil || len(revisions) != expectedCount { + return false + } + cod, err := getResource("clusterobjectdeployment", codName, "") + if err != nil { + return false + } + for i := range revisions { + owner := metav1.GetControllerOf(&revisions[i]) + if owner == nil || owner.Kind != ocv1.ClusterObjectDeploymentKind || owner.Name != codName || owner.UID != cod.GetUID() { + return false + } + } + return true + }) + return nil +} + +// ClusterObjectDeploymentIsOwnedByClusterExtension waits until the named ClusterObjectDeployment has +// the named ClusterExtension as its controller owner. +func ClusterObjectDeploymentIsOwnedByClusterExtension(ctx context.Context, codName, extName string) error { + sc := scenarioCtx(ctx) + codName = substituteScenarioVars(codName, sc) + extName = substituteScenarioVars(extName, sc) + waitFor(ctx, func() bool { + cod, err := getResource("clusterobjectdeployment", codName, "") + if err != nil { + return false + } + ext, err := getResource("clusterextension", extName, "") + if err != nil { + return false + } + owner := metav1.GetControllerOf(cod) + return owner != nil && owner.Kind == ocv1.ClusterExtensionKind && owner.Name == extName && owner.UID == ext.GetUID() + }) + return nil +} + // NamedClusterExtensionReportsCondition waits for a specific ClusterExtension (by name) to have a condition // matching type and status. Polls with timeout. func NamedClusterExtensionReportsCondition(ctx context.Context, extName, conditionType, conditionStatus string) error { @@ -1477,6 +1526,45 @@ func ClusterObjectSetReferredSecretsHaveType(ctx context.Context, revisionName, return nil } +// ClusterObjectSetReferredSecretsOwnedByClusterExtension verifies that every referred Secret of the +// named revision is controller-owned by the named ClusterExtension. On the current operator-controller +// path the ClusterExtension owns the content Secrets directly (so deleting it garbage-collects them). +func ClusterObjectSetReferredSecretsOwnedByClusterExtension(ctx context.Context, revisionName, extName string) error { + sc := scenarioCtx(ctx) + revisionName = substituteScenarioVars(strings.TrimSpace(revisionName), sc) + extName = substituteScenarioVars(strings.TrimSpace(extName), sc) + + ext, err := getResource("clusterextension", extName, "") + if err != nil { + return fmt.Errorf("getting ClusterExtension %q: %w", extName, err) + } + + secrets, err := listReferredSecrets(ctx, revisionName) + if err != nil { + return err + } + if len(secrets) == 0 { + return fmt.Errorf("no referred secrets found for revision %q", revisionName) + } + + for _, s := range secrets { + found := false + for _, ref := range s.OwnerReferences { + if ref.Kind == ocv1.ClusterExtensionKind && ref.Name == extName && ref.UID == ext.GetUID() { + if ref.Controller == nil || !*ref.Controller { + return fmt.Errorf("secret %s/%s has ownerReference to ClusterExtension but controller is not true", s.Namespace, s.Name) + } + found = true + break + } + } + if !found { + return fmt.Errorf("secret %s/%s is not owned by ClusterExtension %q (uid %s)", s.Namespace, s.Name, extName, ext.GetUID()) + } + } + return nil +} + // collectReferredSecretNames returns the unique set of Secret names referenced by the ClusterObjectSet's phase objects. func collectReferredSecretNames(ctx context.Context, revisionName string) ([]string, error) { var names []string @@ -1514,19 +1602,79 @@ func collectReferredSecretNames(ctx context.Context, revisionName string) ([]str return names, nil } -// listReferredSecrets lists all Secrets in the OLM namespace that have the revision-name label -// matching the given revision name. +type referredSecretRef struct { + name string + namespace string +} + +// collectReferredSecretRefs returns the unique Secrets (name + namespace) referenced by the +// ClusterObjectSet's phase objects. An empty ref namespace defaults to the OLM system namespace. +func collectReferredSecretRefs(ctx context.Context, revisionName string) ([]referredSecretRef, error) { + var refs []referredSecretRef + seen := sets.New[string]() + + var obj *unstructured.Unstructured + waitFor(ctx, func() bool { + var err error + obj, err = getResource("clusterobjectset", revisionName, "") + return err == nil + }) + + phases, _, _ := unstructured.NestedSlice(obj.Object, "spec", "phases") + for _, p := range phases { + phase, ok := p.(map[string]interface{}) + if !ok { + continue + } + objects, _, _ := unstructured.NestedSlice(phase, "objects") + for _, o := range objects { + phaseObj, ok := o.(map[string]interface{}) + if !ok { + continue + } + name, _, _ := unstructured.NestedString(phaseObj, "ref", "name") + if name == "" { + continue + } + ns, _, _ := unstructured.NestedString(phaseObj, "ref", "namespace") + if ns == "" { + ns = olmNamespace + } + key := ns + "/" + name + if seen.Has(key) { + continue + } + seen.Insert(key) + refs = append(refs, referredSecretRef{name: name, namespace: ns}) + } + } + if len(refs) == 0 { + return nil, fmt.Errorf("no referred secret refs found in ClusterObjectSet %q", revisionName) + } + return refs, nil +} + +// listReferredSecrets returns the Secrets referenced by the named ClusterObjectSet's phase refs. +// Discovery is driven off the ClusterObjectSet spec rather than a label, so it is robust to how the +// Secrets are named or labeled. func listReferredSecrets(ctx context.Context, revisionName string) ([]corev1.Secret, error) { - out, err := k8sClient(ctx, "get", "secrets", "-n", olmNamespace, - "-l", "olm.operatorframework.io/revision-name="+revisionName, "-o", "json") + refs, err := collectReferredSecretRefs(ctx, revisionName) if err != nil { - return nil, fmt.Errorf("listing referred secrets for revision %q: %w", revisionName, err) + return nil, err } - var secretList corev1.SecretList - if err := json.Unmarshal([]byte(out), &secretList); err != nil { - return nil, fmt.Errorf("unmarshalling Secret list: %w", err) + secrets := make([]corev1.Secret, 0, len(refs)) + for _, r := range refs { + out, err := k8sClient(ctx, "get", "secret", r.name, "-n", r.namespace, "-o", "json") + if err != nil { + return nil, fmt.Errorf("getting referred secret %s/%s: %w", r.namespace, r.name, err) + } + var s corev1.Secret + if err := json.Unmarshal([]byte(out), &s); err != nil { + return nil, fmt.Errorf("unmarshalling secret %s/%s: %w", r.namespace, r.name, err) + } + secrets = append(secrets, s) } - return secretList.Items, nil + return secrets, nil } // ResourceAvailable waits for the specified resource (kind/name format) to exist in the test namespace. Polls with timeout.