From b75ac4a61029bd3f0d9e6896484313939103f996 Mon Sep 17 00:00:00 2001 From: Francesco Giudici Date: Thu, 1 Oct 2026 14:06:41 +0200 Subject: [PATCH 01/10] Add ClusterObjectSet group API specification Require an immutable group identifier for revision tracking and display it in the Group printer column. Define revision sequences by group regardless of owner references and describe ClusterObjectSet independently of its current CE parent. Co-authored-by: Codex Signed-off-by: Francesco Giudici --- api/v1/clusterobjectset_types.go | 35 ++++++++++++++------- docs/draft/concepts/clusterobjectsets.md | 29 ++++++++++++----- docs/draft/concepts/large-bundle-support.md | 2 ++ 3 files changed, 48 insertions(+), 18 deletions(-) diff --git a/api/v1/clusterobjectset_types.go b/api/v1/clusterobjectset_types.go index d68ad9efca..4a6548f265 100644 --- a/api/v1/clusterobjectset_types.go +++ b/api/v1/clusterobjectset_types.go @@ -42,6 +42,18 @@ const ( // ClusterObjectSetSpec defines the desired state of ClusterObjectSet. type ClusterObjectSetSpec struct { + // group is a required, immutable identifier that links related revisions together. + // All revisions sharing the same group form an ordered sequence, regardless of owner references. + // The value must be 1 to 52 characters long, start with a lowercase letter, + // contain only lowercase letters, digits or hyphens, and end with a letter or digit. + // + // +required + // +kubebuilder:validation:MinLength=1 + // +kubebuilder:validation:MaxLength=52 + // +kubebuilder:validation:XValidation:rule=`self.matches('^[a-z]([a-z0-9-]*[a-z0-9])?$')`,message="group must start with a lowercase letter, contain only lowercase letters, digits or hyphens, and end with a letter or digit" + // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="group is immutable" + Group string `json:"group,omitempty"` + // lifecycleState specifies the lifecycle state of the ClusterObjectSet. // // When set to "Active", the revision is actively managed and reconciled. @@ -60,11 +72,11 @@ type ClusterObjectSetSpec struct { // +kubebuilder:validation:XValidation:rule="oldSelf == 'Active' || oldSelf == 'Archived' && oldSelf == self", message="cannot un-archive" LifecycleState ClusterObjectSetLifecycleState `json:"lifecycleState,omitempty"` - // revision is a required, immutable sequence number representing a specific revision - // of the parent ClusterExtension. + // revision is a required, immutable sequence number identifying a specific + // ClusterObjectSet within a sequence of related revisions. // // The revision field must be a positive integer. - // Each ClusterObjectSet belonging to the same parent ClusterExtension must have a unique revision number. + // Each ClusterObjectSet in the same revision sequence must have a unique revision number. // The revision number must always be the previous revision number plus one, or 1 for the first revision. // // +required @@ -607,15 +619,16 @@ type ObjectCounts struct { // +kubebuilder:object:root=true // +kubebuilder:resource:scope=Cluster // +kubebuilder:subresource:status +// +kubebuilder:printcolumn:name="Group",type=string,JSONPath=`.spec.group` // +kubebuilder:printcolumn:name="Ready",type=string,JSONPath=`.status.conditions[?(@.type=='Ready')].status` // +kubebuilder:printcolumn:name=Age,type=date,JSONPath=`.metadata.creationTimestamp` -// ClusterObjectSet represents an immutable snapshot of Kubernetes objects -// for a specific version of a ClusterExtension. Each revision contains objects -// organized into phases that roll out sequentially. The same object can only be managed by a single revision -// at a time. Ownership of objects is transitioned from one revision to the next as the extension is upgraded -// or reconfigured. Once the latest revision has rolled out successfully, previous active revisions are archived for -// posterity. +// ClusterObjectSet represents an immutable snapshot of Kubernetes objects to +// apply and manage on the cluster. Each revision contains objects organized into +// phases that roll out sequentially. The same object can only be managed by a +// single revision at a time. Ownership of objects is transitioned from one revision +// to the next as new revisions are rolled out. Once the latest revision has rolled +// out successfully, previous active revisions are archived for posterity. type ClusterObjectSet struct { metav1.TypeMeta `json:",inline"` @@ -625,8 +638,8 @@ type ClusterObjectSet struct { metav1.ObjectMeta `json:"metadata,omitempty"` // spec defines the desired state of the ClusterObjectSet. - // +optional - Spec ClusterObjectSetSpec `json:"spec,omitempty"` + // +required + Spec ClusterObjectSetSpec `json:"spec,omitzero"` // status is optional and defines the observed state of the ClusterObjectSet. // +optional diff --git a/docs/draft/concepts/clusterobjectsets.md b/docs/draft/concepts/clusterobjectsets.md index 96c9a67147..c9acd17ff0 100644 --- a/docs/draft/concepts/clusterobjectsets.md +++ b/docs/draft/concepts/clusterobjectsets.md @@ -6,10 +6,11 @@ This document explains what ClusterObjectSets are, how they enable safe phased r A `ClusterObjectSet` is a cluster-scoped Kubernetes API that represents a versioned set of Kubernetes resources organized into ordered phases. It provides a declarative way to roll out a group of related resources sequentially, with built-in readiness checks between phases. -The revision content — the `revision` number, `collisionProtection` strategy, and `phases` (including all objects within them) — is immutable once set. This guarantees that the record of what was deployed at a given revision cannot change after creation. Other fields like `lifecycleState` can change over the object's lifecycle (e.g. transitioning from `Active` to `Archived`), and optional fields like `progressDeadlineMinutes` and `progressionProbes` can be configured independently. +The revision content — the `group` identifier, `revision` number, `collisionProtection` strategy, and `phases` (including all objects within them) — is immutable once set. This guarantees that the record of what was deployed at a given revision cannot change after creation. Other fields like `lifecycleState` can change over the object's lifecycle (e.g. transitioning from `Active` to `Archived`), and optional fields like `progressDeadlineMinutes` and `progressionProbes` can be configured independently. Each ClusterObjectSet has: +- A **group** — a required, immutable identifier linking revisions into one sequence, regardless of their owner references - A **revision number** — an immutable, sequential integer that identifies the version - A **lifecycle state** — either `Active` (being reconciled) or `Archived` (inactive); transitions from Active to Archived but not back - A list of **phases** — immutable, ordered groups of Kubernetes objects that are applied sequentially @@ -18,6 +19,13 @@ Each ClusterObjectSet has: ClusterObjectSets can be used by any controller or system that needs to manage the rollout of a set of Kubernetes resources in a controlled, phased manner. Within OLM, the operator-controller uses ClusterObjectSets as the mechanism to deploy and upgrade ClusterExtensions. +The `group` value must be 1–52 characters long, start with a lowercase letter, +contain only lowercase letters, digits or hyphens, and end with a letter or digit. +Revisions in the same group can transfer managed objects between them even when +they have different controller owners or no owner references. They share the +server-side apply field manager `cos-group/`. Different groups remain +subject to normal object collision protection. + ### Controller deployment The experimental `object-controller` runs in its own Deployment with its own service @@ -85,7 +93,7 @@ When a ClusterObjectSet is `Active`, the controller actively reconciles it using When transitioning from one revision to the next: -1. A new ClusterObjectSet is created with the next sequential revision number +1. A new ClusterObjectSet is created with the same group and the next sequential revision number 2. Both old and new revisions remain `Active` during the transition 3. Objects transition ownership from the old revision to the new one 4. Once the new revision reports `Succeeded`, the old revision can be archived @@ -261,6 +269,10 @@ When a ClusterExtension is upgraded to a new version: A `ClusterExtension` owns one or more `ClusterObjectSet` resources. The ClusterExtension's `.status.activeRevisions` field lists all currently active (non-archived) revisions along with their status conditions. +Operator-controller sets `spec.group` to the ClusterExtension's name and retains +the ClusterExtension controller owner reference for garbage collection. Revision +lookup uses the group; labels and owner references do not determine membership. + When created by operator-controller, ClusterObjectSets carry labels that identify the parent: ```yaml @@ -284,6 +296,7 @@ kind: ClusterObjectSet metadata: name: my-app-rev1 spec: + group: my-app revision: 1 lifecycleState: Active collisionProtection: Prevent @@ -391,6 +404,7 @@ kind: ClusterObjectSet metadata: name: my-app-rev1 spec: + group: my-app revision: 1 lifecycleState: Active collisionProtection: Prevent @@ -427,8 +441,9 @@ Multiple objects can reference different keys within the same Secret, allowing e # List all ClusterObjectSets kubectl get clusterobjectsets -# List revisions for a specific extension -kubectl get clusterobjectsets -l olm.operatorframework.io/owner-name=my-extension +# List revisions in a specific extension's group +kubectl get clusterobjectsets -o json | \ + jq -r '.items[] | select(.spec.group == "my-extension") | .metadata.name' # View full details for a specific revision kubectl get clusterobjectset -o yaml @@ -440,7 +455,7 @@ kubectl -n olmv1-system logs deployment/object-controller-controller-manager Example output: ``` -NAME READY AGE -my-extension-abc12 Unknown 2d -my-extension-def34 True 1h +NAME GROUP READY AGE +my-extension-abc12 my-extension Unknown 2d +my-extension-def34 my-extension True 1h ``` diff --git a/docs/draft/concepts/large-bundle-support.md b/docs/draft/concepts/large-bundle-support.md index a407258549..bef7b0219f 100644 --- a/docs/draft/concepts/large-bundle-support.md +++ b/docs/draft/concepts/large-bundle-support.md @@ -207,6 +207,7 @@ kind: ClusterObjectSet metadata: name: my-extension-1 spec: + group: my-extension revision: 1 lifecycleState: Active collisionProtection: Prevent @@ -298,6 +299,7 @@ kind: ClusterObjectSet metadata: name: my-extension-1 spec: + group: my-extension revision: 1 lifecycleState: Active collisionProtection: Prevent From e90a0e78334c1cd4b2a19d1872aa89a557e0520a Mon Sep 17 00:00:00 2001 From: Francesco Giudici Date: Thu, 1 Oct 2026 14:09:01 +0200 Subject: [PATCH 02/10] Generate ClusterObjectSet group API artifacts Regenerate apply configurations and experimental CRDs and manifests from the group API specification. Co-authored-by: Codex Signed-off-by: Francesco Giudici --- .../api/v1/clusterobjectset.go | 12 +++--- .../api/v1/clusterobjectsetspec.go | 19 +++++++-- applyconfigurations/internal/internal.go | 3 ++ ...peratorframework.io_clusterobjectsets.yaml | 40 ++++++++++++++----- manifests/experimental-e2e.yaml | 40 ++++++++++++++----- manifests/experimental.yaml | 40 ++++++++++++++----- 6 files changed, 118 insertions(+), 36 deletions(-) diff --git a/applyconfigurations/api/v1/clusterobjectset.go b/applyconfigurations/api/v1/clusterobjectset.go index 040648390d..57a89c5464 100644 --- a/applyconfigurations/api/v1/clusterobjectset.go +++ b/applyconfigurations/api/v1/clusterobjectset.go @@ -30,12 +30,12 @@ import ( // ClusterObjectSetApplyConfiguration represents a declarative configuration of the ClusterObjectSet type for use // with apply. // -// ClusterObjectSet represents an immutable snapshot of Kubernetes objects -// for a specific version of a ClusterExtension. Each revision contains objects -// organized into phases that roll out sequentially. The same object can only be managed by a single revision -// at a time. Ownership of objects is transitioned from one revision to the next as the extension is upgraded -// or reconfigured. Once the latest revision has rolled out successfully, previous active revisions are archived for -// posterity. +// ClusterObjectSet represents an immutable snapshot of Kubernetes objects to +// apply and manage on the cluster. Each revision contains objects organized into +// phases that roll out sequentially. The same object can only be managed by a +// single revision at a time. Ownership of objects is transitioned from one revision +// to the next as new revisions are rolled out. Once the latest revision has rolled +// out successfully, previous active revisions are archived for posterity. type ClusterObjectSetApplyConfiguration struct { metav1.TypeMetaApplyConfiguration `json:",inline"` // metadata is the standard object's metadata. diff --git a/applyconfigurations/api/v1/clusterobjectsetspec.go b/applyconfigurations/api/v1/clusterobjectsetspec.go index afba3d87d4..4e12c1bbc8 100644 --- a/applyconfigurations/api/v1/clusterobjectsetspec.go +++ b/applyconfigurations/api/v1/clusterobjectsetspec.go @@ -27,6 +27,11 @@ import ( // // ClusterObjectSetSpec defines the desired state of ClusterObjectSet. type ClusterObjectSetSpecApplyConfiguration struct { + // group is a required, immutable identifier that links related revisions together. + // All revisions sharing the same group form an ordered sequence, regardless of owner references. + // The value must be 1 to 52 characters long, start with a lowercase letter, + // contain only lowercase letters, digits or hyphens, and end with a letter or digit. + Group *string `json:"group,omitempty"` // lifecycleState specifies the lifecycle state of the ClusterObjectSet. // // When set to "Active", the revision is actively managed and reconciled. @@ -40,11 +45,11 @@ type ClusterObjectSetSpecApplyConfiguration struct { // moving from one revision to another. The old revision will not be set to "Archived" until the // new revision has been completely rolled out. LifecycleState *apiv1.ClusterObjectSetLifecycleState `json:"lifecycleState,omitempty"` - // revision is a required, immutable sequence number representing a specific revision - // of the parent ClusterExtension. + // revision is a required, immutable sequence number identifying a specific + // ClusterObjectSet within a sequence of related revisions. // // The revision field must be a positive integer. - // Each ClusterObjectSet belonging to the same parent ClusterExtension must have a unique revision number. + // Each ClusterObjectSet in the same revision sequence must have a unique revision number. // The revision number must always be the previous revision number plus one, or 1 for the first revision. Revision *int64 `json:"revision,omitempty"` // phases is an optional, immutable list of phases that group objects to be applied together. @@ -102,6 +107,14 @@ func ClusterObjectSetSpec() *ClusterObjectSetSpecApplyConfiguration { return &ClusterObjectSetSpecApplyConfiguration{} } +// WithGroup sets the Group field in the declarative configuration to the given value +// and returns the receiver, so that objects can be built by chaining "With" function invocations. +// If called multiple times, the Group field is set to the value of the last call. +func (b *ClusterObjectSetSpecApplyConfiguration) WithGroup(value string) *ClusterObjectSetSpecApplyConfiguration { + b.Group = &value + return b +} + // WithLifecycleState sets the LifecycleState field in the declarative configuration to the given value // and returns the receiver, so that objects can be built by chaining "With" function invocations. // If called multiple times, the LifecycleState field is set to the value of the last call. diff --git a/applyconfigurations/internal/internal.go b/applyconfigurations/internal/internal.go index 12d01d336f..4ba2e695b1 100644 --- a/applyconfigurations/internal/internal.go +++ b/applyconfigurations/internal/internal.go @@ -302,6 +302,9 @@ var schemaYAML = typed.YAMLObject(`types: - name: collisionProtection type: namedType: com.github.operator-framework.operator-controller.api.v1.CollisionProtection + - name: group + type: + scalar: string - name: lifecycleState type: namedType: com.github.operator-framework.operator-controller.api.v1.ClusterObjectSetLifecycleState diff --git a/helm/olmv1/base/object-controller/crd/experimental/olm.operatorframework.io_clusterobjectsets.yaml b/helm/olmv1/base/object-controller/crd/experimental/olm.operatorframework.io_clusterobjectsets.yaml index b8625ab136..36ee6cb499 100644 --- a/helm/olmv1/base/object-controller/crd/experimental/olm.operatorframework.io_clusterobjectsets.yaml +++ b/helm/olmv1/base/object-controller/crd/experimental/olm.operatorframework.io_clusterobjectsets.yaml @@ -16,6 +16,9 @@ spec: scope: Cluster versions: - additionalPrinterColumns: + - jsonPath: .spec.group + name: Group + type: string - jsonPath: .status.conditions[?(@.type=='Ready')].status name: Ready type: string @@ -26,12 +29,12 @@ spec: schema: openAPIV3Schema: description: |- - ClusterObjectSet represents an immutable snapshot of Kubernetes objects - for a specific version of a ClusterExtension. Each revision contains objects - organized into phases that roll out sequentially. The same object can only be managed by a single revision - at a time. Ownership of objects is transitioned from one revision to the next as the extension is upgraded - or reconfigured. Once the latest revision has rolled out successfully, previous active revisions are archived for - posterity. + ClusterObjectSet represents an immutable snapshot of Kubernetes objects to + apply and manage on the cluster. Each revision contains objects organized into + phases that roll out sequentially. The same object can only be managed by a + single revision at a time. Ownership of objects is transitioned from one revision + to the next as new revisions are rolled out. Once the latest revision has rolled + out successfully, previous active revisions are archived for posterity. properties: apiVersion: description: |- @@ -70,6 +73,22 @@ spec: x-kubernetes-validations: - message: collisionProtection is immutable rule: self == oldSelf + group: + description: |- + group is a required, immutable identifier that links related revisions together. + All revisions sharing the same group form an ordered sequence, regardless of owner references. + The value must be 1 to 52 characters long, start with a lowercase letter, + contain only lowercase letters, digits or hyphens, and end with a letter or digit. + maxLength: 52 + minLength: 1 + type: string + x-kubernetes-validations: + - message: group must start with a lowercase letter, contain only + lowercase letters, digits or hyphens, and end with a letter or + digit + rule: self.matches('^[a-z]([a-z0-9-]*[a-z0-9])?$') + - message: group is immutable + rule: self == oldSelf lifecycleState: description: |- lifecycleState specifies the lifecycle state of the ClusterObjectSet. @@ -518,11 +537,11 @@ spec: x-kubernetes-list-type: atomic revision: description: |- - revision is a required, immutable sequence number representing a specific revision - of the parent ClusterExtension. + revision is a required, immutable sequence number identifying a specific + ClusterObjectSet within a sequence of related revisions. The revision field must be a positive integer. - Each ClusterObjectSet belonging to the same parent ClusterExtension must have a unique revision number. + Each ClusterObjectSet in the same revision sequence must have a unique revision number. The revision number must always be the previous revision number plus one, or 1 for the first revision. format: int64 minimum: 1 @@ -532,6 +551,7 @@ spec: rule: self == oldSelf required: - collisionProtection + - group - lifecycleState - revision type: object @@ -754,6 +774,8 @@ spec: x-kubernetes-validations: - message: completedAt cannot be removed once set rule: '!has(oldSelf.completedAt) || has(self.completedAt)' + required: + - spec type: object served: true storage: true diff --git a/manifests/experimental-e2e.yaml b/manifests/experimental-e2e.yaml index af5be46e93..01d9d4eb76 100644 --- a/manifests/experimental-e2e.yaml +++ b/manifests/experimental-e2e.yaml @@ -1427,6 +1427,9 @@ spec: scope: Cluster versions: - additionalPrinterColumns: + - jsonPath: .spec.group + name: Group + type: string - jsonPath: .status.conditions[?(@.type=='Ready')].status name: Ready type: string @@ -1437,12 +1440,12 @@ spec: schema: openAPIV3Schema: description: |- - ClusterObjectSet represents an immutable snapshot of Kubernetes objects - for a specific version of a ClusterExtension. Each revision contains objects - organized into phases that roll out sequentially. The same object can only be managed by a single revision - at a time. Ownership of objects is transitioned from one revision to the next as the extension is upgraded - or reconfigured. Once the latest revision has rolled out successfully, previous active revisions are archived for - posterity. + ClusterObjectSet represents an immutable snapshot of Kubernetes objects to + apply and manage on the cluster. Each revision contains objects organized into + phases that roll out sequentially. The same object can only be managed by a + single revision at a time. Ownership of objects is transitioned from one revision + to the next as new revisions are rolled out. Once the latest revision has rolled + out successfully, previous active revisions are archived for posterity. properties: apiVersion: description: |- @@ -1481,6 +1484,22 @@ spec: x-kubernetes-validations: - message: collisionProtection is immutable rule: self == oldSelf + group: + description: |- + group is a required, immutable identifier that links related revisions together. + All revisions sharing the same group form an ordered sequence, regardless of owner references. + The value must be 1 to 52 characters long, start with a lowercase letter, + contain only lowercase letters, digits or hyphens, and end with a letter or digit. + maxLength: 52 + minLength: 1 + type: string + x-kubernetes-validations: + - message: group must start with a lowercase letter, contain only + lowercase letters, digits or hyphens, and end with a letter or + digit + rule: self.matches('^[a-z]([a-z0-9-]*[a-z0-9])?$') + - message: group is immutable + rule: self == oldSelf lifecycleState: description: |- lifecycleState specifies the lifecycle state of the ClusterObjectSet. @@ -1929,11 +1948,11 @@ spec: x-kubernetes-list-type: atomic revision: description: |- - revision is a required, immutable sequence number representing a specific revision - of the parent ClusterExtension. + revision is a required, immutable sequence number identifying a specific + ClusterObjectSet within a sequence of related revisions. The revision field must be a positive integer. - Each ClusterObjectSet belonging to the same parent ClusterExtension must have a unique revision number. + Each ClusterObjectSet in the same revision sequence must have a unique revision number. The revision number must always be the previous revision number plus one, or 1 for the first revision. format: int64 minimum: 1 @@ -1943,6 +1962,7 @@ spec: rule: self == oldSelf required: - collisionProtection + - group - lifecycleState - revision type: object @@ -2165,6 +2185,8 @@ spec: x-kubernetes-validations: - message: completedAt cannot be removed once set rule: '!has(oldSelf.completedAt) || has(self.completedAt)' + required: + - spec type: object served: true storage: true diff --git a/manifests/experimental.yaml b/manifests/experimental.yaml index d1664fc010..7a5e03d745 100644 --- a/manifests/experimental.yaml +++ b/manifests/experimental.yaml @@ -1388,6 +1388,9 @@ spec: scope: Cluster versions: - additionalPrinterColumns: + - jsonPath: .spec.group + name: Group + type: string - jsonPath: .status.conditions[?(@.type=='Ready')].status name: Ready type: string @@ -1398,12 +1401,12 @@ spec: schema: openAPIV3Schema: description: |- - ClusterObjectSet represents an immutable snapshot of Kubernetes objects - for a specific version of a ClusterExtension. Each revision contains objects - organized into phases that roll out sequentially. The same object can only be managed by a single revision - at a time. Ownership of objects is transitioned from one revision to the next as the extension is upgraded - or reconfigured. Once the latest revision has rolled out successfully, previous active revisions are archived for - posterity. + ClusterObjectSet represents an immutable snapshot of Kubernetes objects to + apply and manage on the cluster. Each revision contains objects organized into + phases that roll out sequentially. The same object can only be managed by a + single revision at a time. Ownership of objects is transitioned from one revision + to the next as new revisions are rolled out. Once the latest revision has rolled + out successfully, previous active revisions are archived for posterity. properties: apiVersion: description: |- @@ -1442,6 +1445,22 @@ spec: x-kubernetes-validations: - message: collisionProtection is immutable rule: self == oldSelf + group: + description: |- + group is a required, immutable identifier that links related revisions together. + All revisions sharing the same group form an ordered sequence, regardless of owner references. + The value must be 1 to 52 characters long, start with a lowercase letter, + contain only lowercase letters, digits or hyphens, and end with a letter or digit. + maxLength: 52 + minLength: 1 + type: string + x-kubernetes-validations: + - message: group must start with a lowercase letter, contain only + lowercase letters, digits or hyphens, and end with a letter or + digit + rule: self.matches('^[a-z]([a-z0-9-]*[a-z0-9])?$') + - message: group is immutable + rule: self == oldSelf lifecycleState: description: |- lifecycleState specifies the lifecycle state of the ClusterObjectSet. @@ -1890,11 +1909,11 @@ spec: x-kubernetes-list-type: atomic revision: description: |- - revision is a required, immutable sequence number representing a specific revision - of the parent ClusterExtension. + revision is a required, immutable sequence number identifying a specific + ClusterObjectSet within a sequence of related revisions. The revision field must be a positive integer. - Each ClusterObjectSet belonging to the same parent ClusterExtension must have a unique revision number. + Each ClusterObjectSet in the same revision sequence must have a unique revision number. The revision number must always be the previous revision number plus one, or 1 for the first revision. format: int64 minimum: 1 @@ -1904,6 +1923,7 @@ spec: rule: self == oldSelf required: - collisionProtection + - group - lifecycleState - revision type: object @@ -2126,6 +2146,8 @@ spec: x-kubernetes-validations: - message: completedAt cannot be removed once set rule: '!has(oldSelf.completedAt) || has(self.completedAt)' + required: + - spec type: object served: true storage: true From 77144eb673f5fb265ae938c2cc283b20ff5c8c1f Mon Sep 17 00:00:00 2001 From: Francesco Giudici Date: Thu, 1 Oct 2026 14:49:02 +0200 Subject: [PATCH 03/10] Track ClusterObjectSet revisions by group Populate and index spec.group, query revision chains by group, and map COS events to the ClusterExtension named by the group. Share SSA field ownership across revisions while retaining the OLM metadata prefix. Use the whole group for sibling discovery, numbering, status, migration lookup, fallback, and retention regardless of owner references. Keep CE controller ownership for generated and Helm-migrated revisions. Cover admission, cache isolation, owner changes, event mapping, lifecycle behavior across owners, and SSA handoff. Mark the CE integration points that future COD support will replace. Co-authored-by: Codex Signed-off-by: Francesco Giudici --- api/v1/clusterobjectset_types_test.go | 114 ++++++++++- api/v1/validation_test.go | 3 + cmd/object-controller/main.go | 7 +- cmd/object-controller/main_test.go | 1 + cmd/operator-controller/main.go | 31 ++- .../clusterobjectset_controller.go | 25 ++- ...usterobjectset_controller_internal_test.go | 63 +++++- .../clusterobjectset_controller_test.go | 41 ++-- .../controllers/indexes_test.go | 64 ++++++ .../controllers/resolve_ref_test.go | 17 +- .../controllers/revision_engine_factory.go | 32 +-- .../revision_engine_factory_test.go | 173 ++++++++++++++++ .../operator-controller/applier/boxcutter.go | 36 ++-- .../applier/boxcutter_test.go | 187 ++++++++++++++++-- .../controllers/boxcutter_reconcile_steps.go | 19 +- ...boxcutter_reconcile_steps_internal_test.go | 72 +++++++ .../boxcutter_reconcile_steps_test.go | 124 +++++++----- .../clusterextension_controller.go | 46 +++-- ...usterextension_controller_internal_test.go | 46 +++++ .../revisions/clusterobjectsets.go | 41 ++++ .../revisions/clusterobjectsets_test.go | 88 +++++++++ test/indexes.go | 28 +++ 22 files changed, 1053 insertions(+), 205 deletions(-) create mode 100644 internal/object-controller/controllers/indexes_test.go create mode 100644 internal/object-controller/controllers/revision_engine_factory_test.go create mode 100644 internal/operator-controller/controllers/boxcutter_reconcile_steps_internal_test.go create mode 100644 internal/operator-controller/controllers/clusterextension_controller_internal_test.go create mode 100644 internal/operator-controller/revisions/clusterobjectsets.go create mode 100644 internal/operator-controller/revisions/clusterobjectsets_test.go create mode 100644 test/indexes.go diff --git a/api/v1/clusterobjectset_types_test.go b/api/v1/clusterobjectset_types_test.go index 6770fbb129..e00a1a2e85 100644 --- a/api/v1/clusterobjectset_types_test.go +++ b/api/v1/clusterobjectset_types_test.go @@ -3,12 +3,14 @@ package v1 import ( "context" "fmt" + "strings" "testing" "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/utils/ptr" ) func TestClusterObjectSetImmutability(t *testing.T) { @@ -16,12 +18,51 @@ func TestClusterObjectSetImmutability(t *testing.T) { ctx := context.Background() i := 0 for name, tc := range map[string]struct { - spec ClusterObjectSetSpec - updateFunc func(*ClusterObjectSet) - allowed bool + spec ClusterObjectSetSpec + updateFunc func(*ClusterObjectSet) + allowed bool + expectedError string }{ + "group is immutable": { + spec: ClusterObjectSetSpec{ + Group: "test-group", + LifecycleState: ClusterObjectSetLifecycleStateActive, + Revision: 1, + CollisionProtection: CollisionProtectionPrevent, + }, + updateFunc: func(cos *ClusterObjectSet) { + cos.Spec.Group = "another-group" + }, + expectedError: "group is immutable", + }, + "group cannot be cleared": { + spec: ClusterObjectSetSpec{ + Group: "test-group", + LifecycleState: ClusterObjectSetLifecycleStateActive, + Revision: 1, + CollisionProtection: CollisionProtectionPrevent, + }, + updateFunc: func(cos *ClusterObjectSet) { + cos.Spec.Group = "" + }, + expectedError: "spec.group: Required value", + }, + "unchanged group permits lifecycle update": { + spec: ClusterObjectSetSpec{ + Group: "test-group", + LifecycleState: ClusterObjectSetLifecycleStateActive, + Revision: 1, + CollisionProtection: CollisionProtectionPrevent, + }, + updateFunc: func(cos *ClusterObjectSet) { + cos.Spec.Group = "test-group" + cos.Spec.LifecycleState = ClusterObjectSetLifecycleStateArchived + }, + allowed: true, + }, "revision is immutable": { spec: ClusterObjectSetSpec{ + Group: "test-group", LifecycleState: ClusterObjectSetLifecycleStateActive, Revision: 1, CollisionProtection: CollisionProtectionPrevent, @@ -32,6 +73,7 @@ func TestClusterObjectSetImmutability(t *testing.T) { }, "phases may be initially empty": { spec: ClusterObjectSetSpec{ + Group: "test-group", LifecycleState: ClusterObjectSetLifecycleStateActive, Revision: 1, CollisionProtection: CollisionProtectionPrevent, @@ -49,6 +91,7 @@ func TestClusterObjectSetImmutability(t *testing.T) { }, "phases may be initially unset": { spec: ClusterObjectSetSpec{ + Group: "test-group", LifecycleState: ClusterObjectSetLifecycleStateActive, Revision: 1, CollisionProtection: CollisionProtectionPrevent, @@ -65,6 +108,7 @@ func TestClusterObjectSetImmutability(t *testing.T) { }, "phases are immutable if not empty": { spec: ClusterObjectSetSpec{ + Group: "test-group", LifecycleState: ClusterObjectSetLifecycleStateActive, Revision: 1, CollisionProtection: CollisionProtectionPrevent, @@ -86,6 +130,7 @@ func TestClusterObjectSetImmutability(t *testing.T) { }, "spec collisionProtection is immutable": { spec: ClusterObjectSetSpec{ + Group: "test-group", LifecycleState: ClusterObjectSetLifecycleStateActive, Revision: 1, CollisionProtection: CollisionProtectionPrevent, @@ -112,6 +157,9 @@ func TestClusterObjectSetImmutability(t *testing.T) { if !tc.allowed && !errors.IsInvalid(err) { t.Fatal("expected update to fail due to invalid payload, but got:", err) } + if tc.expectedError != "" { + require.ErrorContains(t, err, tc.expectedError) + } }) } } @@ -351,6 +399,7 @@ func TestClusterObjectSetValidity(t *testing.T) { }, Spec: tc.spec, } + cos.Spec.Group = "test-group" i = i + 1 err := c.Create(ctx, cos) if tc.valid && err != nil { @@ -363,6 +412,65 @@ func TestClusterObjectSetValidity(t *testing.T) { } } +func TestClusterObjectSetSpecValidation(t *testing.T) { + c := newClient(t) + t.Run("missing spec", func(t *testing.T) { + cos := &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": GroupVersion.String(), + "kind": ClusterObjectSetKind, + "metadata": map[string]any{"generateName": "spec-validation-"}, + }} + err := c.Create(t.Context(), cos) + require.True(t, errors.IsInvalid(err), "%v", err) + require.ErrorContains(t, err, "spec: Required") + }) +} + +func TestClusterObjectSetGroupValidation(t *testing.T) { + c := newClient(t) + for _, tc := range []struct { + name string + group *string + valid bool + }{ + {name: "missing group", valid: false}, + {name: "empty group", group: ptr.To(""), valid: false}, + {name: "one character", group: ptr.To("a"), valid: true}, + {name: "lowercase hyphens and digits", group: ptr.To("my-group-1"), valid: true}, + {name: "maximum length", group: ptr.To(strings.Repeat("a", 52)), valid: true}, + {name: "over maximum length", group: ptr.To(strings.Repeat("a", 53)), valid: false}, + {name: "starts with digit", group: ptr.To("1group"), valid: false}, + {name: "starts with hyphen", group: ptr.To("-group"), valid: false}, + {name: "ends with hyphen", group: ptr.To("group-"), valid: false}, + {name: "uppercase", group: ptr.To("Group"), valid: false}, + {name: "underscore", group: ptr.To("my_group"), valid: false}, + {name: "dot", group: ptr.To("my.group"), valid: false}, + } { + t.Run(tc.name, func(t *testing.T) { + cos := &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": GroupVersion.String(), + "kind": ClusterObjectSetKind, + "metadata": map[string]any{"generateName": "group-validation-"}, + }} + spec := map[string]any{ + "revision": int64(1), "lifecycleState": string(ClusterObjectSetLifecycleStateActive), + "collisionProtection": string(CollisionProtectionPrevent), + } + if tc.group != nil { + spec["group"] = *tc.group + } + cos.Object["spec"] = spec + err := c.Create(t.Context(), cos) + if tc.valid { + require.NoError(t, err) + } else { + require.True(t, errors.IsInvalid(err), "%v", err) + require.ErrorContains(t, err, "spec.group") + } + }) + } +} + func configMap() unstructured.Unstructured { return unstructured.Unstructured{ Object: map[string]interface{}{ diff --git a/api/v1/validation_test.go b/api/v1/validation_test.go index 2c77c17663..60453733b6 100644 --- a/api/v1/validation_test.go +++ b/api/v1/validation_test.go @@ -34,6 +34,7 @@ func TestValidate(t *testing.T) { return s } defaultRevisionSpec := func(s *ClusterObjectSetSpec) *ClusterObjectSetSpec { + s.Group = "test-group" s.Revision = 1 s.CollisionProtection = CollisionProtectionPrevent return s @@ -188,6 +189,7 @@ func TestClusterObjectSetCompletedAtImmutable(t *testing.T) { cos := &ClusterObjectSet{ ObjectMeta: metav1.ObjectMeta{Name: "cos-completedat-immutable"}, Spec: ClusterObjectSetSpec{ + Group: "test-group", Revision: 1, CollisionProtection: CollisionProtectionPrevent, LifecycleState: ClusterObjectSetLifecycleStateActive, @@ -218,6 +220,7 @@ func TestClusterObjectSetCompletedAtCannotBeRemoved(t *testing.T) { cos := &ClusterObjectSet{ ObjectMeta: metav1.ObjectMeta{Name: "cos-completedat-noremove"}, Spec: ClusterObjectSetSpec{ + Group: "test-group", Revision: 1, CollisionProtection: CollisionProtectionPrevent, LifecycleState: ClusterObjectSetLifecycleStateActive, diff --git a/cmd/object-controller/main.go b/cmd/object-controller/main.go index a6ceec21ee..f6616d6cbf 100644 --- a/cmd/object-controller/main.go +++ b/cmd/object-controller/main.go @@ -17,6 +17,7 @@ limitations under the License. package main import ( + "context" "crypto/tls" "flag" "fmt" @@ -181,10 +182,12 @@ func newManager(cfg *config, restConfig *rest.Config) (manager.Manager, error) { if err != nil { return nil, fmt.Errorf("creating discovery client: %w", err) } - // Keep the field owner prefix unchanged so existing objects can be reconciled after migration. + if err := controllers.SetupIndexes(context.Background(), mgr.GetFieldIndexer()); err != nil { + return nil, fmt.Errorf("indexing ClusterObjectSet group: %w", err) + } factory, err := controllers.NewDefaultRevisionEngineFactory( mgr.GetScheme(), trackingCache, memory.NewMemCacheClient(discoveryClient), - mgr.GetRESTMapper(), "olm.operatorframework.io", mgr.GetConfig(), + mgr.GetRESTMapper(), mgr.GetConfig(), ) if err != nil { return nil, fmt.Errorf("creating revision engine factory: %w", err) diff --git a/cmd/object-controller/main_test.go b/cmd/object-controller/main_test.go index 9453967b35..b82f730028 100644 --- a/cmd/object-controller/main_test.go +++ b/cmd/object-controller/main_test.go @@ -116,6 +116,7 @@ func TestStandaloneController(t *testing.T) { cos := &ocv1.ClusterObjectSet{ ObjectMeta: metav1.ObjectMeta{Name: name}, Spec: ocv1.ClusterObjectSetSpec{ + Group: name, LifecycleState: ocv1.ClusterObjectSetLifecycleStateActive, Revision: 1, CollisionProtection: ocv1.CollisionProtectionPrevent, Phases: []ocv1.ClusterObjectSetPhase{{Name: "deploy", Objects: []ocv1.ClusterObjectSetObject{obj}}}, diff --git a/cmd/operator-controller/main.go b/cmd/operator-controller/main.go index dfae2b6a97..28547cec4d 100644 --- a/cmd/operator-controller/main.go +++ b/cmd/operator-controller/main.go @@ -45,19 +45,17 @@ import ( crcache "sigs.k8s.io/controller-runtime/pkg/cache" "sigs.k8s.io/controller-runtime/pkg/certwatcher" "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/event" crfinalizer "sigs.k8s.io/controller-runtime/pkg/finalizer" - crhandler "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/healthz" "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/manager" "sigs.k8s.io/controller-runtime/pkg/metrics/filters" "sigs.k8s.io/controller-runtime/pkg/metrics/server" - "sigs.k8s.io/controller-runtime/pkg/predicate" helmclient "github.com/operator-framework/helm-operator-plugins/pkg/client" ocv1 "github.com/operator-framework/operator-controller/api/v1" + clusterobjctrl "github.com/operator-framework/operator-controller/internal/object-controller/controllers" "github.com/operator-framework/operator-controller/internal/operator-controller/action" "github.com/operator-framework/operator-controller/internal/operator-controller/applier" "github.com/operator-framework/operator-controller/internal/operator-controller/catalogmetadata/cache" @@ -419,6 +417,13 @@ func run() error { } cl := mgr.GetClient() + // TODO(COD): Remove this registration when COD replaces this manager's + // direct ClusterExtension-to-COS queries. + if features.OperatorControllerFeatureGate.Enabled(features.BoxcutterRuntime) { + if err := clusterobjctrl.SetupIndexes(context.Background(), mgr.GetFieldIndexer()); err != nil { + return fmt.Errorf("indexing ClusterObjectSet group: %w", err) + } + } catalogsCachePath := filepath.Join(cfg.cachePath, "catalogs") if err := os.MkdirAll(catalogsCachePath, 0700); err != nil { @@ -457,10 +462,7 @@ func run() error { } var trackingCache managedcache.TrackingCache - var ctrlBuilderOpts []controllers.ControllerBuilderOption - if features.OperatorControllerFeatureGate.Enabled(features.BoxcutterRuntime) { - ctrlBuilderOpts = append(ctrlBuilderOpts, controllers.WithOwns(&ocv1.ClusterObjectSet{})) - } else { + if !features.OperatorControllerFeatureGate.Enabled(features.BoxcutterRuntime) { trackingCache, err = managedcache.NewTrackingCache( ctrl.Log.WithName("trackingCache"), mgr.GetConfig(), @@ -472,21 +474,16 @@ func run() error { if err := mgr.Add(trackingCache); err != nil { return fmt.Errorf("unable to add tracking cache to manager: %w", err) } - ctrlBuilderOpts = append(ctrlBuilderOpts, controllers.WithWatchesRawSource( - trackingCache.Source( - crhandler.EnqueueRequestForOwner(mgr.GetScheme(), mgr.GetRESTMapper(), &ocv1.ClusterExtension{}), - predicate.ResourceVersionChangedPredicate{}, - predicate.Funcs{ - CreateFunc: func(event.TypedCreateEvent[client.Object]) bool { return false }, - }, - ), - )) } ceReconciler := &controllers.ClusterExtensionReconciler{ Client: cl, } - _, err = ceReconciler.SetupWithManager(mgr, ctrlBuilderOpts...) + if features.OperatorControllerFeatureGate.Enabled(features.BoxcutterRuntime) { + _, err = ceReconciler.SetupWithManagerForBoxcutter(mgr) + } else { + _, err = ceReconciler.SetupWithManagerForHelm(mgr, trackingCache) + } if err != nil { setupLog.Error(err, "unable to create controller", "controller", "ClusterExtension") return err diff --git a/internal/object-controller/controllers/clusterobjectset_controller.go b/internal/object-controller/controllers/clusterobjectset_controller.go index 83755bbb6e..570bd09cd4 100644 --- a/internal/object-controller/controllers/clusterobjectset_controller.go +++ b/internal/object-controller/controllers/clusterobjectset_controller.go @@ -440,6 +440,17 @@ type Sourcoser interface { Source(handler handler.EventHandler, predicates ...predicate.Predicate) source.Source } +// SetupIndexes registers the cache indexes used to look up ClusterObjectSet revisions. +func SetupIndexes(ctx context.Context, indexer client.FieldIndexer) error { + return indexer.IndexField(ctx, &ocv1.ClusterObjectSet{}, ".spec.group", func(obj client.Object) []string { + cos, ok := obj.(*ocv1.ClusterObjectSet) + if !ok || cos.Spec.Group == "" { + return nil + } + return []string{cos.Spec.Group} + }) +} + func (c *ClusterObjectSetReconciler) SetupWithManager(mgr ctrl.Manager) error { c.Clock = clock.RealClock{} return ctrl.NewControllerManagedBy(mgr). @@ -523,14 +534,15 @@ func (c *ClusterObjectSetReconciler) removeFinalizer(ctx context.Context, obj cl return nil } -// listSiblingRevisions returns all active revisions belonging to the same owner, excluding the current one. +// listSiblingRevisions returns active revisions in the same group, +// excluding the current one. // This includes both lower and higher revision numbers, enabling boxcutter to properly classify // sibling owners and avoid reporting false collisions during revision handover. func (c *ClusterObjectSetReconciler) listSiblingRevisions(ctx context.Context, cos *ocv1.ClusterObjectSet) ([]*ocv1.ClusterObjectSet, error) { return c.listOtherActiveRevisions(ctx, cos, func(*ocv1.ClusterObjectSet) bool { return true }) } -// listPreviousRevisions returns active revisions belonging to the same owner with lower revision numbers. +// listPreviousRevisions returns active revisions in the same group with lower revision numbers. func (c *ClusterObjectSetReconciler) listPreviousRevisions(ctx context.Context, cos *ocv1.ClusterObjectSet) ([]*ocv1.ClusterObjectSet, error) { return c.listOtherActiveRevisions(ctx, cos, func(r *ocv1.ClusterObjectSet) bool { return r.Spec.Revision < cos.Spec.Revision @@ -542,15 +554,8 @@ func (c *ClusterObjectSetReconciler) listOtherActiveRevisions( cos *ocv1.ClusterObjectSet, predicate func(*ocv1.ClusterObjectSet) bool, ) ([]*ocv1.ClusterObjectSet, error) { - ownerLabel, ok := cos.Labels[labels.OwnerNameKey] - if !ok { - return nil, nil - } - revList := &ocv1.ClusterObjectSetList{} - if err := c.TrackingCache.List(ctx, revList, client.MatchingLabels{ - labels.OwnerNameKey: ownerLabel, - }); err != nil { + if err := c.Client.List(ctx, revList, client.MatchingFields{".spec.group": cos.Spec.Group}); err != nil { return nil, fmt.Errorf("listing revisions: %w", err) } diff --git a/internal/object-controller/controllers/clusterobjectset_controller_internal_test.go b/internal/object-controller/controllers/clusterobjectset_controller_internal_test.go index eda064db36..638ac89d10 100644 --- a/internal/object-controller/controllers/clusterobjectset_controller_internal_test.go +++ b/internal/object-controller/controllers/clusterobjectset_controller_internal_test.go @@ -24,6 +24,7 @@ import ( ocv1 "github.com/operator-framework/operator-controller/api/v1" "github.com/operator-framework/operator-controller/internal/shared/labels" + "github.com/operator-framework/operator-controller/test" ) func Test_ClusterObjectSetReconciler_listSiblingRevisions(t *testing.T) { @@ -85,7 +86,7 @@ func Test_ClusterObjectSetReconciler_listSiblingRevisions(t *testing.T) { expectedRevs: []string{"rev-1"}, }, { - name: "should only include revisions matching owner label", + name: "should only include revisions in the same group", existingObjs: func() []client.Object { ext := newTestClusterExtensionInternal() ext2 := newTestClusterExtensionInternal() @@ -94,7 +95,7 @@ func Test_ClusterObjectSetReconciler_listSiblingRevisions(t *testing.T) { rev1 := newTestClusterObjectSetInternal(t, "rev-1") rev2 := newTestClusterObjectSetInternal(t, "rev-2") - rev2.Labels[labels.OwnerNameKey] = "test-ext-2" + rev2.Spec.Group = "test-ext-2" rev3 := newTestClusterObjectSetInternal(t, "rev-3") require.NoError(t, controllerutil.SetControllerReference(ext, rev1, testScheme)) require.NoError(t, controllerutil.SetControllerReference(ext2, rev2, testScheme)) @@ -105,16 +106,18 @@ func Test_ClusterObjectSetReconciler_listSiblingRevisions(t *testing.T) { expectedRevs: []string{"rev-1"}, }, { - name: "should return empty list when owner label missing", + name: "should include revisions when owner label is missing", existingObjs: func() []client.Object { ext := newTestClusterExtensionInternal() rev1 := newTestClusterObjectSetInternal(t, "rev-1") - delete(rev1.Labels, labels.OwnerNameKey) + rev2 := newTestClusterObjectSetInternal(t, "rev-2") + delete(rev2.Labels, labels.OwnerNameKey) require.NoError(t, controllerutil.SetControllerReference(ext, rev1, testScheme)) - return []client.Object{ext, rev1} + require.NoError(t, controllerutil.SetControllerReference(ext, rev2, testScheme)) + return []client.Object{ext, rev1, rev2} }, currentRev: "rev-1", - expectedRevs: []string{}, + expectedRevs: []string{"rev-2"}, }, } { t.Run(tc.name, func(t *testing.T) { @@ -178,6 +181,50 @@ func Test_ClusterObjectSetReconciler_listPreviousRevisions(t *testing.T) { } } +func Test_ClusterObjectSetReconciler_OwnerChangesPreserveRevisionChain(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, ocv1.AddToScheme(scheme)) + ext := newTestClusterExtensionInternal() + owner := *metav1.NewControllerRef(ext, ocv1.GroupVersion.WithKind(ocv1.ClusterExtensionKind)) + foreignOwner := owner + foreignOwner.Kind = "OtherController" + foreignOwner.Name = "other-owner" + foreignOwner.UID = "other-uid" + previous := newTestClusterObjectSetInternal(t, "rev-1") + current := newTestClusterObjectSetInternal(t, "rev-2") + current.OwnerReferences = []metav1.OwnerReference{owner} + otherGroup := newTestClusterObjectSetInternal(t, "other-1") + otherGroup.Spec.Group = "other-group" + otherGroup.OwnerReferences = []metav1.OwnerReference{owner} + cl := test.WithIndexes(t, fake.NewClientBuilder().WithScheme(scheme), SetupIndexes). + WithObjects(previous, current, otherGroup).Build() + r := &ClusterObjectSetReconciler{Client: cl} + + for _, tc := range []struct { + name string + owners []metav1.OwnerReference + }{ + {name: "same owner", owners: []metav1.OwnerReference{owner}}, + {name: "different owner", owners: []metav1.OwnerReference{foreignOwner}}, + {name: "owner removed"}, + {name: "owner added again", owners: []metav1.OwnerReference{owner}}, + } { + t.Run(tc.name, func(t *testing.T) { + require.NoError(t, cl.Get(t.Context(), client.ObjectKeyFromObject(previous), previous)) + previous.OwnerReferences = tc.owners + require.NoError(t, cl.Update(t.Context(), previous)) + siblings, err := r.listSiblingRevisions(t.Context(), current) + require.NoError(t, err) + require.Len(t, siblings, 1) + require.Equal(t, previous.Name, siblings[0].Name) + predecessors, err := r.listPreviousRevisions(t.Context(), current) + require.NoError(t, err) + require.Len(t, predecessors, 1) + require.Equal(t, previous.Name, predecessors[0].Name) + }) + } +} + func callRevisionLister( t *testing.T, testScheme *runtime.Scheme, @@ -187,8 +234,7 @@ func callRevisionLister( ) []string { t.Helper() mockCtrl := gomock.NewController(t) - testClient := fake.NewClientBuilder(). - WithScheme(testScheme). + testClient := test.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), SetupIndexes). WithObjects(existingObjs...). Build() @@ -245,6 +291,7 @@ func newTestClusterObjectSetInternal(t *testing.T, name string) *ocv1.ClusterObj }, }, Spec: ocv1.ClusterObjectSetSpec{ + Group: "test-ext", Revision: revNum, Phases: []ocv1.ClusterObjectSetPhase{ { diff --git a/internal/object-controller/controllers/clusterobjectset_controller_test.go b/internal/object-controller/controllers/clusterobjectset_controller_test.go index aa3efa03b9..91cb3539ad 100644 --- a/internal/object-controller/controllers/clusterobjectset_controller_test.go +++ b/internal/object-controller/controllers/clusterobjectset_controller_test.go @@ -33,6 +33,7 @@ import ( "github.com/operator-framework/operator-controller/internal/shared/labels" mockcontrollers "github.com/operator-framework/operator-controller/internal/testutil/mock/controllers" mockmachinery "github.com/operator-framework/operator-controller/internal/testutil/mock/machinery" + "github.com/operator-framework/operator-controller/test" ) const clusterObjectSetName = "test-ext-1" @@ -595,8 +596,13 @@ func Test_ClusterObjectSetReconciler_Reconcile_RevisionReconciliation(t *testing ext := newTestClusterExtension() prevRev1 := newTestClusterObjectSet(t, "test-ext-1", ext, testScheme) prevRev2 := newTestClusterObjectSet(t, "test-ext-2", ext, testScheme) + prevRev1.OwnerReferences = nil + prevRev2.OwnerReferences[0].Name = "other-owner" + prevRev2.OwnerReferences[0].UID = "other-uid" rev := newTestClusterObjectSet(t, "test-ext-3", ext, testScheme) - return []client.Object{ext, prevRev1, prevRev2, rev} + otherGroup := newTestClusterObjectSet(t, "other-group-1", ext, testScheme) + otherGroup.Spec.Group = "other-group" + return []client.Object{ext, prevRev1, prevRev2, rev, otherGroup} }, validate: func(t *testing.T, c client.Client) { rev := &ocv1.ClusterObjectSet{} @@ -617,6 +623,8 @@ func Test_ClusterObjectSetReconciler_Reconcile_RevisionReconciliation(t *testing }, rev) require.NoError(t, err) require.Equal(t, ocv1.ClusterObjectSetLifecycleStateActive, rev.Spec.LifecycleState) + require.NoError(t, c.Get(t.Context(), client.ObjectKey{Name: "other-group-1"}, rev)) + require.Equal(t, ocv1.ClusterObjectSetLifecycleStateActive, rev.Spec.LifecycleState) }, }, } { @@ -624,8 +632,7 @@ func Test_ClusterObjectSetReconciler_Reconcile_RevisionReconciliation(t *testing mockCtrl := gomock.NewController(t) // create extension and cluster extension - testClient := fake.NewClientBuilder(). - WithScheme(testScheme). + testClient := test.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), controllers.SetupIndexes). WithStatusSubresource(&ocv1.ClusterObjectSet{}). WithObjects(tc.existingObjs()...). Build() @@ -728,8 +735,7 @@ func Test_ClusterObjectSetReconciler_Reconcile_CompletedAt(t *testing.T) { t.Run(tc.name, func(t *testing.T) { mockCtrl := gomock.NewController(t) - testClient := fake.NewClientBuilder(). - WithScheme(testScheme). + testClient := test.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), controllers.SetupIndexes). WithStatusSubresource(&ocv1.ClusterObjectSet{}). WithObjects(tc.existingObjs()...). Build() @@ -842,8 +848,7 @@ func Test_ClusterObjectSetReconciler_Reconcile_ValidationError_Retries(t *testin rev1 := newTestClusterObjectSet(t, clusterObjectSetName, ext, testScheme) // create extension and cluster extension - testClient := fake.NewClientBuilder(). - WithScheme(testScheme). + testClient := test.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), controllers.SetupIndexes). WithStatusSubresource(&ocv1.ClusterObjectSet{}). WithObjects(ext, rev1). Build() @@ -941,8 +946,7 @@ func Test_ClusterObjectSetReconciler_Reconcile_VerifyReferencedSecrets(t *testin }, }} - testClient := fake.NewClientBuilder(). - WithScheme(testScheme). + testClient := test.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), controllers.SetupIndexes). WithStatusSubresource(&ocv1.ClusterObjectSet{}). WithObjects(append([]client.Object{ext, rev1}, tc.secrets...)...). WithInterceptorFuncs(interceptor.Funcs{ @@ -1014,8 +1018,7 @@ func Test_ClusterObjectSetReconciler_Reconcile_ReferencedContentValidationRecove Data: map[string][]byte{"configmap": []byte("not-json")}, } - testClient := fake.NewClientBuilder(). - WithScheme(testScheme). + testClient := test.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), controllers.SetupIndexes). WithStatusSubresource(&ocv1.ClusterObjectSet{}). WithObjects(ext, rev1, badSecret). Build() @@ -1105,8 +1108,7 @@ func Test_ClusterObjectSetReconciler_Reconcile_BuildPhasesErrorClassification(t tc.setupCOS(rev1) } - builder := fake.NewClientBuilder(). - WithScheme(testScheme). + builder := test.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), controllers.SetupIndexes). WithStatusSubresource(&ocv1.ClusterObjectSet{}). WithObjects(ext, rev1) if tc.listFails { @@ -1398,8 +1400,7 @@ func Test_ClusterObjectSetReconciler_Reconcile_ArchivalAndDeletion(t *testing.T) mockCtrl := gomock.NewController(t) // create extension and cluster extension - testClient := fake.NewClientBuilder(). - WithScheme(testScheme). + testClient := test.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), controllers.SetupIndexes). WithStatusSubresource(&ocv1.ClusterObjectSet{}). WithObjects(tc.existingObjs()...). Build() @@ -1671,8 +1672,7 @@ func Test_ClusterObjectSetReconciler_Reconcile_ProgressDeadline(t *testing.T) { mockCtrl := gomock.NewController(t) // create extension and cluster extension - testClient := fake.NewClientBuilder(). - WithScheme(testScheme). + testClient := test.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), controllers.SetupIndexes). WithStatusSubresource(&ocv1.ClusterObjectSet{}). WithObjects(tc.existingObjs()...). Build() @@ -1809,6 +1809,7 @@ func newTestClusterObjectSet(t *testing.T, revisionName string, ext *ocv1.Cluste }, }, Spec: ocv1.ClusterObjectSetSpec{ + Group: ext.Name, LifecycleState: ocv1.ClusterObjectSetLifecycleStateActive, Revision: revNum, Phases: []ocv1.ClusterObjectSetPhase{ @@ -2220,8 +2221,7 @@ func Test_ClusterObjectSetReconciler_Reconcile_ForeignRevisionCollision(t *testi } { t.Run(tc.name, func(t *testing.T) { mockCtrl := gomock.NewController(t) - testClient := fake.NewClientBuilder(). - WithScheme(testScheme). + testClient := test.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), controllers.SetupIndexes). WithStatusSubresource(&ocv1.ClusterObjectSet{}). WithObjects(tc.existingObjs()...). Build() @@ -2322,8 +2322,7 @@ func Test_ClusterObjectSetReconciler_getScopedClient_Errors(t *testing.T) { ext := newTestClusterExtension() rev := newTestClusterObjectSet(t, "test-rev", ext, testScheme) - testClient := fake.NewClientBuilder(). - WithScheme(testScheme). + testClient := test.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), controllers.SetupIndexes). WithObjects(ext, rev). Build() diff --git a/internal/object-controller/controllers/indexes_test.go b/internal/object-controller/controllers/indexes_test.go new file mode 100644 index 0000000000..98516c9fa8 --- /dev/null +++ b/internal/object-controller/controllers/indexes_test.go @@ -0,0 +1,64 @@ +//go:build !standard + +package controllers_test + +import ( + "testing" + + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + ocv1 "github.com/operator-framework/operator-controller/api/v1" + "github.com/operator-framework/operator-controller/internal/object-controller/controllers" + "github.com/operator-framework/operator-controller/test" +) + +func TestSetupIndexes_GroupLookup(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, ocv1.AddToScheme(scheme)) + cl := test.WithIndexes(t, fake.NewClientBuilder().WithScheme(scheme), controllers.SetupIndexes). + WithObjects( + &ocv1.ClusterObjectSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "matching-group", + Labels: map[string]string{ + "olm.operatorframework.io/owner-name": "other-group", + }, + }, + Spec: ocv1.ClusterObjectSetSpec{Group: "my-group"}, + }, + &ocv1.ClusterObjectSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "other-group", + Labels: map[string]string{ + "olm.operatorframework.io/owner-name": "my-group", + }, + }, + Spec: ocv1.ClusterObjectSetSpec{Group: "other-group"}, + }, + &ocv1.ClusterObjectSet{ObjectMeta: metav1.ObjectMeta{Name: "missing-group"}}, + ). + Build() + + for _, tc := range []struct { + group string + want []string + }{ + {group: "my-group", want: []string{"matching-group"}}, + {group: "other-group", want: []string{"other-group"}}, + {group: ""}, + } { + t.Run(tc.group, func(t *testing.T) { + list := &ocv1.ClusterObjectSetList{} + require.NoError(t, cl.List(t.Context(), list, client.MatchingFields{".spec.group": tc.group})) + names := make([]string, 0, len(list.Items)) + for _, cos := range list.Items { + names = append(names, cos.Name) + } + require.ElementsMatch(t, tc.want, names) + }) + } +} diff --git a/internal/object-controller/controllers/resolve_ref_test.go b/internal/object-controller/controllers/resolve_ref_test.go index 1391d12dfe..a9e926023b 100644 --- a/internal/object-controller/controllers/resolve_ref_test.go +++ b/internal/object-controller/controllers/resolve_ref_test.go @@ -26,6 +26,7 @@ import ( ocv1 "github.com/operator-framework/operator-controller/api/v1" "github.com/operator-framework/operator-controller/internal/object-controller/controllers" "github.com/operator-framework/operator-controller/internal/shared/labels" + "github.com/operator-framework/operator-controller/test" ) func newSchemeWithCoreV1(t *testing.T) *apimachineryruntime.Scheme { @@ -68,8 +69,7 @@ func TestResolveObjectRef_PlainJSON(t *testing.T) { Key: "my-key", }) - fakeClient := fake.NewClientBuilder(). - WithScheme(testScheme). + fakeClient := test.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), controllers.SetupIndexes). WithObjects(secret, cos). WithStatusSubresource(&ocv1.ClusterObjectSet{}). Build() @@ -131,8 +131,7 @@ func TestResolveObjectRef_GzipCompressed(t *testing.T) { Key: "my-key", }) - fakeClient := fake.NewClientBuilder(). - WithScheme(testScheme). + fakeClient := test.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), controllers.SetupIndexes). WithObjects(secret, cos). WithStatusSubresource(&ocv1.ClusterObjectSet{}). Build() @@ -165,8 +164,7 @@ func TestResolveObjectRef_SecretNotFound(t *testing.T) { Key: "my-key", }) - fakeClient := fake.NewClientBuilder(). - WithScheme(testScheme). + fakeClient := test.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), controllers.SetupIndexes). WithObjects(cos). WithStatusSubresource(&ocv1.ClusterObjectSet{}). Build() @@ -206,8 +204,7 @@ func TestResolveObjectRef_KeyNotFound(t *testing.T) { Key: "missing-key", }) - fakeClient := fake.NewClientBuilder(). - WithScheme(testScheme). + fakeClient := test.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), controllers.SetupIndexes). WithObjects(secret, cos). WithStatusSubresource(&ocv1.ClusterObjectSet{}). Build() @@ -256,8 +253,7 @@ func TestResolveObjectRef_InvalidJSON(t *testing.T) { Key: "my-key", }) - fakeClient := fake.NewClientBuilder(). - WithScheme(testScheme). + fakeClient := test.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), controllers.SetupIndexes). WithObjects(secret, cos). WithStatusSubresource(&ocv1.ClusterObjectSet{}). Build() @@ -295,6 +291,7 @@ func newRefTestCOS(name string, ref ocv1.ObjectSourceRef) *ocv1.ClusterObjectSet }, }, Spec: ocv1.ClusterObjectSetSpec{ + Group: "ref-test", LifecycleState: ocv1.ClusterObjectSetLifecycleStateActive, Revision: 1, CollisionProtection: ocv1.CollisionProtectionPrevent, diff --git a/internal/object-controller/controllers/revision_engine_factory.go b/internal/object-controller/controllers/revision_engine_factory.go index d361644ca0..6b915beda8 100644 --- a/internal/object-controller/controllers/revision_engine_factory.go +++ b/internal/object-controller/controllers/revision_engine_factory.go @@ -37,23 +37,25 @@ type RevisionEngineFactory interface { // defaultRevisionEngineFactory creates boxcutter RevisionEngines. type defaultRevisionEngineFactory struct { - Scheme *runtime.Scheme - TrackingCache managedcache.TrackingCache - DiscoveryClient discovery.CachedDiscoveryInterface - RESTMapper meta.RESTMapper - FieldOwnerPrefix string - Client client.Client + Scheme *runtime.Scheme + TrackingCache managedcache.TrackingCache + DiscoveryClient discovery.CachedDiscoveryInterface + RESTMapper meta.RESTMapper + Client client.Client } // CreateRevisionEngine constructs a boxcutter RevisionEngine for the given ClusterObjectSet. func (f *defaultRevisionEngineFactory) CreateRevisionEngine(_ context.Context, rev *ocv1.ClusterObjectSet) (RevisionEngine, error) { + fieldOwner := "cos-group/" + rev.Spec.Group + systemPrefix := ocv1.GroupVersion.Group + managedBy := ocv1.GroupVersion.Group return machinery.NewRevisionEngine( machinery.NewPhaseEngine( machinery.NewObjectEngine( f.Scheme, f.TrackingCache, f.Client, - machinery.NewComparator(f.DiscoveryClient, f.Scheme, f.FieldOwnerPrefix), - f.FieldOwnerPrefix, f.FieldOwnerPrefix, - f.FieldOwnerPrefix, // managedBy + machinery.NewComparator(f.DiscoveryClient, f.Scheme, fieldOwner), + fieldOwner, systemPrefix, + managedBy, f.Client, ), validation.NewClusterPhaseValidator(f.RESTMapper, f.Client), @@ -68,7 +70,6 @@ func NewDefaultRevisionEngineFactory( trackingCache managedcache.TrackingCache, discoveryClient discovery.CachedDiscoveryInterface, restMapper meta.RESTMapper, - fieldOwnerPrefix string, baseConfig *rest.Config, ) (RevisionEngineFactory, error) { if baseConfig == nil { @@ -79,11 +80,10 @@ func NewDefaultRevisionEngineFactory( return nil, fmt.Errorf("failed to create client: %w", err) } return &defaultRevisionEngineFactory{ - Scheme: scheme, - TrackingCache: trackingCache, - DiscoveryClient: discoveryClient, - RESTMapper: restMapper, - FieldOwnerPrefix: fieldOwnerPrefix, - Client: c, + Scheme: scheme, + TrackingCache: trackingCache, + DiscoveryClient: discoveryClient, + RESTMapper: restMapper, + Client: c, }, nil } diff --git a/internal/object-controller/controllers/revision_engine_factory_test.go b/internal/object-controller/controllers/revision_engine_factory_test.go new file mode 100644 index 0000000000..527d4680f4 --- /dev/null +++ b/internal/object-controller/controllers/revision_engine_factory_test.go @@ -0,0 +1,173 @@ +//go:build !standard + +/* +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_test + +import ( + "context" + "testing" + "time" + + "github.com/go-logr/logr" + "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" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/client-go/discovery" + "k8s.io/client-go/discovery/cached/memory" + "pkg.package-operator.run/boxcutter" + "pkg.package-operator.run/boxcutter/managedcache" + "pkg.package-operator.run/boxcutter/ownerhandling" + "sigs.k8s.io/controller-runtime/pkg/cache" + "sigs.k8s.io/controller-runtime/pkg/client" + + ocv1 "github.com/operator-framework/operator-controller/api/v1" + "github.com/operator-framework/operator-controller/internal/object-controller/controllers" +) + +func TestDefaultRevisionEngineFactory_FieldOwnership(t *testing.T) { + scheme := newScheme(t) + require.NoError(t, corev1.AddToScheme(scheme)) + cl, err := client.New(config, client.Options{Scheme: scheme}) + require.NoError(t, err) + + // A core resource serves as the owner to isolate the factory's SSA and + // metadata configuration from the COS controller's lifecycle behavior. + owner := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{GenerateName: "factory-field-owner-"}} + require.NoError(t, cl.Create(t.Context(), owner)) + objectKey := client.ObjectKey{Namespace: owner.Name, Name: "payload"} + t.Cleanup(func() { + require.NoError(t, client.IgnoreNotFound(cl.Delete(context.Background(), &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Namespace: objectKey.Namespace, Name: objectKey.Name}, + }))) + require.NoError(t, cl.Delete(context.Background(), owner)) + }) + + trackingCache, err := managedcache.NewTrackingCache(logr.Discard(), config, cache.Options{ + Scheme: scheme, + Mapper: cl.RESTMapper(), + }) + require.NoError(t, err) + ctx, cancel := context.WithTimeout(t.Context(), 30*time.Second) + cacheDone := make(chan error, 1) + go func() { cacheDone <- trackingCache.Start(ctx) }() + t.Cleanup(func() { + cancel() + select { + case err := <-cacheDone: + require.NoError(t, err) + case <-time.After(5 * time.Second): + t.Fatal("tracking cache did not stop") + } + }) + require.NoError(t, trackingCache.Watch(ctx, owner, sets.New(corev1.SchemeGroupVersion.WithKind("ConfigMap")))) + + discoveryClient, err := discovery.NewDiscoveryClientForConfig(config) + require.NoError(t, err) + factory, err := controllers.NewDefaultRevisionEngineFactory( + scheme, trackingCache, memory.NewMemCacheClient(discoveryClient), cl.RESTMapper(), config, + ) + require.NoError(t, err) + cos := &ocv1.ClusterObjectSet{ + ObjectMeta: metav1.ObjectMeta{Name: "factory-test-1"}, + Spec: ocv1.ClusterObjectSetSpec{Group: "factory-test", Revision: 1}, + } + engine, err := factory.CreateRevisionEngine(ctx, cos) + require.NoError(t, err) + + obj := &corev1.ConfigMap{ + TypeMeta: metav1.TypeMeta{APIVersion: "v1", Kind: "ConfigMap"}, + ObjectMeta: metav1.ObjectMeta{Namespace: objectKey.Namespace, Name: objectKey.Name}, + Data: map[string]string{"value": "revision-1"}, + } + revision := boxcutter.NewRevisionWithOwner(cos.Name, cos.Spec.Revision, + []boxcutter.Phase{boxcutter.NewPhase("deploy", []client.Object{obj})}, + owner, ownerhandling.NewNative(scheme), + ) + result, err := engine.Reconcile(ctx, revision) + require.NoError(t, err) + require.NotNil(t, result) + require.Nil(t, result.GetValidationError(), "%v", result.GetValidationError()) + + actual := &corev1.ConfigMap{} + require.NoError(t, cl.Get(ctx, objectKey, actual)) + require.Equal(t, "1", actual.Annotations["olm.operatorframework.io/revision"]) + require.Equal(t, "olm.operatorframework.io", actual.Labels["app.kubernetes.io/managed-by"]) + + var applyManagers []string + for _, fields := range actual.ManagedFields { + if fields.Operation == metav1.ManagedFieldsOperationApply { + applyManagers = append(applyManagers, fields.Manager) + } + } + require.Contains(t, applyManagers, "cos-group/factory-test") + + // A subsequent revision updates the same field using the same SSA manager. + cos.Name = "factory-test-2" + cos.Spec.Revision = 2 + engine, err = factory.CreateRevisionEngine(ctx, cos) + require.NoError(t, err) + obj.Data["value"] = "revision-2" + objectMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(obj) + require.NoError(t, err) + desired := &unstructured.Unstructured{Object: objectMap} + revision = boxcutter.NewRevisionWithOwner(cos.Name, cos.Spec.Revision, + []boxcutter.Phase{boxcutter.NewPhase("deploy", []client.Object{desired})}, + owner, ownerhandling.NewNative(scheme), + ) + require.Eventually(t, func() bool { + if _, err := engine.Reconcile(ctx, revision); err != nil { + return false + } + return cl.Get(ctx, objectKey, actual) == nil && actual.Data["value"] == "revision-2" + }, 5*time.Second, 10*time.Millisecond) + applyManagers = nil + for _, fields := range actual.ManagedFields { + if fields.Operation == metav1.ManagedFieldsOperationApply { + applyManagers = append(applyManagers, fields.Manager) + } + } + require.Equal(t, []string{"cos-group/factory-test"}, applyManagers) + require.Equal(t, "2", actual.Annotations["olm.operatorframework.io/revision"]) + + // Another group gets its own SSA manager. + cos.Name = "factory-other-1" + cos.Spec.Group = "factory-other" + cos.Spec.Revision = 1 + engine, err = factory.CreateRevisionEngine(ctx, cos) + require.NoError(t, err) + other := desired.DeepCopy() + other.SetName("other-payload") + t.Cleanup(func() { require.NoError(t, client.IgnoreNotFound(cl.Delete(context.Background(), other))) }) + revision = boxcutter.NewRevisionWithOwner(cos.Name, cos.Spec.Revision, + []boxcutter.Phase{boxcutter.NewPhase("deploy", []client.Object{other})}, + owner, ownerhandling.NewNative(scheme), + ) + _, err = engine.Reconcile(ctx, revision) + require.NoError(t, err) + require.NoError(t, cl.Get(ctx, client.ObjectKeyFromObject(other), actual)) + applyManagers = nil + for _, fields := range actual.ManagedFields { + if fields.Operation == metav1.ManagedFieldsOperationApply { + applyManagers = append(applyManagers, fields.Manager) + } + } + require.Equal(t, []string{"cos-group/factory-other"}, applyManagers) +} diff --git a/internal/operator-controller/applier/boxcutter.go b/internal/operator-controller/applier/boxcutter.go index 87155bb592..77381dc49b 100644 --- a/internal/operator-controller/applier/boxcutter.go +++ b/internal/operator-controller/applier/boxcutter.go @@ -1,13 +1,11 @@ package applier import ( - "cmp" "context" "errors" "fmt" "io/fs" "maps" - "slices" "strings" "github.com/cert-manager/cert-manager/pkg/apis/certmanager" @@ -32,6 +30,7 @@ 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" @@ -236,6 +235,7 @@ func (r *SimpleRevisionGenerator) buildClusterObjectSet( phases := PhaseSort(objects) spec := ocv1ac.ClusterObjectSetSpec(). + WithGroup(ext.Name). WithLifecycleState(ocv1.ClusterObjectSetLifecycleStateActive). WithPhases(phases...). WithProgressionProbes(defaultProgressionProbes...) @@ -254,6 +254,8 @@ func (r *SimpleRevisionGenerator) buildClusterObjectSet( // BoxcutterStorageMigrator migrates ClusterExtensions from Helm-based storage to // ClusterObjectSet storage, enabling upgrades from older operator-controller versions. +// TODO(COD): Adapt Helm storage migration for ClusterObjectDeployment-backed +// installations and remove direct COS discovery when that migration path is in place. type BoxcutterStorageMigrator struct { ActionClientGetter helmclient.ActionClientGetter RevisionGenerator ClusterObjectSetGenerator @@ -280,14 +282,12 @@ func (m *BoxcutterStorageMigrator) Migrate(ctx context.Context, ext *ocv1.Cluste if ext.Spec.Namespace == "" { return nil } - existingRevisionList := ocv1.ClusterObjectSetList{} - if err := m.Client.List(ctx, &existingRevisionList, client.MatchingLabels{ - labels.OwnerNameKey: ext.Name, - }); err != nil { + existingRevisions, err := revisions.ListForClusterExtension(ctx, m.Client, ext) + if err != nil { return fmt.Errorf("listing ClusterObjectSets before attempting migration: %w", err) } - if len(existingRevisionList.Items) != 0 { - return m.ensureMigratedRevisionStatus(ctx, existingRevisionList.Items) + if len(existingRevisions) != 0 { + return m.ensureMigratedRevisionStatus(ctx, existingRevisions) } ac, err := m.ActionClientGetter.ActionClientFor(ctx, ext) @@ -449,6 +449,8 @@ func (m *BoxcutterStorageMigrator) ensureRevisionStatus(ctx context.Context, nam return nil } +// TODO(COD): Replace direct COS creation, revision discovery and retention with +// ClusterObjectDeployment reconciliation. type Boxcutter struct { Client client.Client Scheme *runtime.Scheme @@ -464,9 +466,9 @@ func (bc *Boxcutter) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.Clust // 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 := bc.getExistingRevisions(ctx, ext.GetName()) + existingRevisions, err := revisions.ListForClusterExtension(ctx, bc.Client, ext) if err != nil { - return false, "", err + return false, "", fmt.Errorf("listing revisions: %w", err) } // If contentFS is nil, we're maintaining the current state without catalog access. @@ -644,20 +646,6 @@ func (bc *Boxcutter) garbageCollectOldRevisions(ctx context.Context, revisionLis return nil } -// getExistingRevisions returns the list of ClusterObjectSets for a ClusterExtension with name extName in revision order (oldest to newest) -func (bc *Boxcutter) getExistingRevisions(ctx context.Context, extName string) ([]ocv1.ClusterObjectSet, error) { - existingRevisionList := &ocv1.ClusterObjectSetList{} - if err := bc.Client.List(ctx, existingRevisionList, client.MatchingLabels{ - labels.OwnerNameKey: extName, - }); err != nil { - return nil, fmt.Errorf("listing revisions: %w", err) - } - slices.SortFunc(existingRevisionList.Items, func(a, b ocv1.ClusterObjectSet) int { - return cmp.Compare(a.Spec.Revision, b.Spec.Revision) - }) - return existingRevisionList.Items, nil -} - func latestRevisionNumber(prevRevisions []ocv1.ClusterObjectSet) int64 { if len(prevRevisions) == 0 { return 0 diff --git a/internal/operator-controller/applier/boxcutter_test.go b/internal/operator-controller/applier/boxcutter_test.go index f42ed763f1..9ba2346313 100644 --- a/internal/operator-controller/applier/boxcutter_test.go +++ b/internal/operator-controller/applier/boxcutter_test.go @@ -29,12 +29,14 @@ import ( 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/shared/labels" bundlecsv "github.com/operator-framework/operator-controller/internal/testing/bundle/csv" bundlefs "github.com/operator-framework/operator-controller/internal/testing/bundle/fs" mockapplier "github.com/operator-framework/operator-controller/internal/testutil/mock/applier" mockctrlclient "github.com/operator-framework/operator-controller/internal/testutil/mock/ctrlclient" + "github.com/operator-framework/operator-controller/test" ) var ( @@ -103,6 +105,7 @@ func Test_SimpleRevisionGenerator_GenerateRevisionFromHelmRelease(t *testing.T) labels.OwnerNameKey: "test-123", }). WithSpec(ocv1ac.ClusterObjectSetSpec(). + WithGroup(ext.Name). WithLifecycleState(ocv1.ClusterObjectSetLifecycleStateActive). WithCollisionProtection(ocv1.CollisionProtectionNone). WithRevision(1). @@ -148,6 +151,7 @@ func Test_SimpleRevisionGenerator_GenerateRevisionFromHelmRelease(t *testing.T) assert.Equal(t, expected.Labels, rev.Labels) assert.Equal(t, expected.Annotations, rev.Annotations) assert.Equal(t, expected.Spec.LifecycleState, rev.Spec.LifecycleState) + assert.Equal(t, expected.Spec.Group, rev.Spec.Group) assert.Equal(t, expected.Spec.CollisionProtection, rev.Spec.CollisionProtection) assert.Equal(t, expected.Spec.Revision, rev.Spec.Revision) assert.Equal(t, expected.Spec.Phases, rev.Spec.Phases) @@ -205,6 +209,7 @@ func Test_SimpleRevisionGenerator_GenerateRevision(t *testing.T) { 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", @@ -522,6 +527,7 @@ func TestBoxcutter_Apply(t *testing.T) { }, }, Spec: ocv1.ClusterObjectSetSpec{ + Group: ext.Name, Revision: 1, Phases: []ocv1.ClusterObjectSetPhase{ { @@ -544,6 +550,9 @@ func TestBoxcutter_Apply(t *testing.T) { }, } + 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 { @@ -584,6 +593,7 @@ func TestBoxcutter_Apply(t *testing.T) { labels.OwnerNameKey: ext.Name, }). WithSpec(ocv1ac.ClusterObjectSetSpec(). + WithGroup(ext.Name). WithPhases( ocv1ac.ClusterObjectSetPhase(). WithName(string(applier.PhaseDeploy)). @@ -607,7 +617,7 @@ func TestBoxcutter_Apply(t *testing.T) { }, validate: func(t *testing.T, c client.Client) { revList := &ocv1.ClusterObjectSetList{} - err := c.List(t.Context(), revList, client.MatchingLabels{labels.OwnerNameKey: ext.Name}) + err := c.List(t.Context(), revList, client.MatchingFields{".spec.group": ext.Name}) require.NoError(t, err) require.Len(t, revList.Items, 1) @@ -617,6 +627,10 @@ func TestBoxcutter_Apply(t *testing.T) { 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) }, }, { @@ -632,6 +646,7 @@ func TestBoxcutter_Apply(t *testing.T) { labels.OwnerNameKey: ext.Name, }). WithSpec(ocv1ac.ClusterObjectSetSpec(). + WithGroup(ext.Name). WithPhases( ocv1ac.ClusterObjectSetPhase(). WithName(string(applier.PhaseDeploy)). @@ -654,11 +669,11 @@ func TestBoxcutter_Apply(t *testing.T) { return m }, existingObjs: []client.Object{ - defaultDesiredRevision, + revisionWithoutOwnerLabel, }, validate: func(t *testing.T, c client.Client) { revList := &ocv1.ClusterObjectSetList{} - err := c.List(context.Background(), revList, client.MatchingLabels{labels.OwnerNameKey: ext.Name}) + 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) @@ -678,6 +693,7 @@ func TestBoxcutter_Apply(t *testing.T) { labels.OwnerNameKey: ext.Name, }). WithSpec(ocv1ac.ClusterObjectSetSpec(). + WithGroup(ext.Name). WithPhases( ocv1ac.ClusterObjectSetPhase(). WithName(string(applier.PhaseDeploy)). @@ -705,7 +721,7 @@ func TestBoxcutter_Apply(t *testing.T) { }, validate: func(t *testing.T, c client.Client) { revList := &ocv1.ClusterObjectSetList{} - err := c.List(context.Background(), revList, client.MatchingLabels{labels.OwnerNameKey: ext.Name}) + err := c.List(context.Background(), revList, client.MatchingFields{".spec.group": ext.Name}) require.NoError(t, err) require.Len(t, revList.Items, 2) @@ -736,13 +752,13 @@ func TestBoxcutter_Apply(t *testing.T) { validate: func(t *testing.T, c client.Client) { // Ensure no revisions were created revList := &ocv1.ClusterObjectSetList{} - err := c.List(context.Background(), revList, client.MatchingLabels{labels.OwnerNameKey: ext.Name}) + 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", + 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) @@ -753,7 +769,7 @@ func TestBoxcutter_Apply(t *testing.T) { WithLabels(map[string]string{ labels.OwnerNameKey: ext.Name, }). - WithSpec(ocv1ac.ClusterObjectSetSpec()), nil + 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 @@ -762,11 +778,16 @@ func TestBoxcutter_Apply(t *testing.T) { &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, }, @@ -779,6 +800,7 @@ func TestBoxcutter_Apply(t *testing.T) { }, }, Spec: ocv1.ClusterObjectSetSpec{ + Group: ext.Name, LifecycleState: ocv1.ClusterObjectSetLifecycleStateArchived, Revision: 2, }, @@ -791,6 +813,7 @@ func TestBoxcutter_Apply(t *testing.T) { }, }, Spec: ocv1.ClusterObjectSetSpec{ + Group: ext.Name, LifecycleState: ocv1.ClusterObjectSetLifecycleStateArchived, Revision: 3, }, @@ -803,6 +826,7 @@ func TestBoxcutter_Apply(t *testing.T) { }, }, Spec: ocv1.ClusterObjectSetSpec{ + Group: ext.Name, LifecycleState: ocv1.ClusterObjectSetLifecycleStateArchived, Revision: 4, }, @@ -815,6 +839,7 @@ func TestBoxcutter_Apply(t *testing.T) { }, }, Spec: ocv1.ClusterObjectSetSpec{ + Group: ext.Name, LifecycleState: ocv1.ClusterObjectSetLifecycleStateArchived, Revision: 5, }, @@ -822,11 +847,16 @@ func TestBoxcutter_Apply(t *testing.T) { &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, }, @@ -841,10 +871,17 @@ func TestBoxcutter_Apply(t *testing.T) { // Verify garbage collection: should only keep the limit + 1 (current) revisions revList := &ocv1.ClusterObjectSetList{} - err = c.List(t.Context(), revList) + 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 - assert.LessOrEqual(t, len(revList.Items), applier.ClusterObjectSetRetentionLimit+1) + 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) }, }, { @@ -859,7 +896,7 @@ func TestBoxcutter_Apply(t *testing.T) { WithLabels(map[string]string{ labels.OwnerNameKey: ext.Name, }). - WithSpec(ocv1ac.ClusterObjectSetSpec()), nil + 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 @@ -873,6 +910,7 @@ func TestBoxcutter_Apply(t *testing.T) { }, }, Spec: ocv1.ClusterObjectSetSpec{ + Group: ext.Name, LifecycleState: ocv1.ClusterObjectSetLifecycleStateArchived, Revision: 1, }, @@ -885,6 +923,7 @@ func TestBoxcutter_Apply(t *testing.T) { }, }, Spec: ocv1.ClusterObjectSetSpec{ + Group: ext.Name, // index beyond the retention limit but active; should be preserved LifecycleState: ocv1.ClusterObjectSetLifecycleStateActive, Revision: 2, @@ -898,6 +937,7 @@ func TestBoxcutter_Apply(t *testing.T) { }, }, Spec: ocv1.ClusterObjectSetSpec{ + Group: ext.Name, LifecycleState: ocv1.ClusterObjectSetLifecycleStateActive, Revision: 3, }, @@ -910,6 +950,7 @@ func TestBoxcutter_Apply(t *testing.T) { }, }, Spec: ocv1.ClusterObjectSetSpec{ + Group: ext.Name, // archived but should be preserved since it is within the limit LifecycleState: ocv1.ClusterObjectSetLifecycleStateArchived, Revision: 4, @@ -923,6 +964,7 @@ func TestBoxcutter_Apply(t *testing.T) { }, }, Spec: ocv1.ClusterObjectSetSpec{ + Group: ext.Name, LifecycleState: ocv1.ClusterObjectSetLifecycleStateActive, Revision: 5, }, @@ -935,6 +977,7 @@ func TestBoxcutter_Apply(t *testing.T) { }, }, Spec: ocv1.ClusterObjectSetSpec{ + Group: ext.Name, LifecycleState: ocv1.ClusterObjectSetLifecycleStateActive, Revision: 6, }, @@ -947,6 +990,7 @@ func TestBoxcutter_Apply(t *testing.T) { }, }, Spec: ocv1.ClusterObjectSetSpec{ + Group: ext.Name, LifecycleState: ocv1.ClusterObjectSetLifecycleStateActive, Revision: 7, }, @@ -982,6 +1026,7 @@ func TestBoxcutter_Apply(t *testing.T) { labels.OwnerNameKey: ext.Name, }). WithSpec(ocv1ac.ClusterObjectSetSpec(). + WithGroup(ext.Name). WithPhases( ocv1ac.ClusterObjectSetPhase(). WithName(string(applier.PhaseDeploy)). @@ -1017,6 +1062,7 @@ func TestBoxcutter_Apply(t *testing.T) { }, }, Spec: ocv1.ClusterObjectSetSpec{ + Group: ext.Name, Revision: 1, Phases: []ocv1.ClusterObjectSetPhase{ { @@ -1041,7 +1087,7 @@ func TestBoxcutter_Apply(t *testing.T) { }, validate: func(t *testing.T, c client.Client) { revList := &ocv1.ClusterObjectSetList{} - err := c.List(context.Background(), revList, client.MatchingLabels{labels.OwnerNameKey: ext.Name}) + 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) @@ -1061,7 +1107,16 @@ func TestBoxcutter_Apply(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { // Setup - cb := fake.NewClientBuilder().WithScheme(testScheme).WithObjects(tc.existingObjs...) + cb := test.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), coscontrollers.SetupIndexes). + WithObjects(tc.existingObjs...) + // A misleading owner label must not let another group's revisions + // affect numbering or retention for this extension. + cb.WithObjects(&ocv1.ClusterObjectSet{ + ObjectMeta: metav1.ObjectMeta{Name: "other-group-99", Labels: map[string]string{labels.OwnerNameKey: ext.Name}}, + Spec: ocv1.ClusterObjectSetSpec{ + Group: "other-group", Revision: 99, LifecycleState: ocv1.ClusterObjectSetLifecycleStateArchived, + }, + }) if tc.clientIterceptor != nil { cb.WithInterceptorFuncs(*tc.clientIterceptor) } @@ -1109,6 +1164,54 @@ func TestBoxcutter_Apply(t *testing.T) { tc.validate(t, fakeClient) } } + require.NoError(t, fakeClient.Get(t.Context(), client.ObjectKey{Name: "other-group-99"}, &ocv1.ClusterObjectSet{})) + }) + } +} + +func TestBoxcutter_Apply_FallbackUsesGroup(t *testing.T) { + for _, tc := range []struct { + name string + group string + ownerName string + wantError bool + }{ + {name: "ownerless", group: "test-ext"}, + {name: "different controller owner", group: "test-ext", ownerName: "other-ext"}, + {name: "different group with matching owner", group: "other-group", ownerName: "test-ext", wantError: true}, + } { + 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"}} + cos := &ocv1.ClusterObjectSet{ + ObjectMeta: metav1.ObjectMeta{Name: "revision-1"}, + Spec: ocv1.ClusterObjectSetSpec{ + Group: tc.group, Revision: 1, LifecycleState: ocv1.ClusterObjectSetLifecycleStateActive, + }, + } + if tc.ownerName != "" { + owner := *metav1.NewControllerRef(ext, ocv1.GroupVersion.WithKind(ocv1.ClusterExtensionKind)) + owner.Name = tc.ownerName + if tc.ownerName != ext.Name { + owner.UID = "other-owner-uid" + } + cos.OwnerReferences = []metav1.OwnerReference{owner} + } + cl := test.WithIndexes(t, fake.NewClientBuilder().WithScheme(scheme), coscontrollers.SetupIndexes). + WithObjects(cos).Build() + bc := &applier.Boxcutter{ + Client: cl, Scheme: scheme, + RevisionGenerator: mockapplier.NewMockClusterObjectSetGenerator(gomock.NewController(t)), + } + completed, _, err := bc.Apply(t.Context(), nil, ext, nil, nil) + if tc.wantError { + require.ErrorContains(t, err, "no revision installed") + require.False(t, completed) + } else { + require.NoError(t, err) + require.True(t, completed) + } }) } } @@ -1120,7 +1223,7 @@ func TestBoxcutterStorageMigrator(t *testing.T) { WithLabels(map[string]string{ labels.OwnerNameKey: ext.Name, }). - WithSpec(ocv1ac.ClusterObjectSetSpec()) + WithSpec(ocv1ac.ClusterObjectSetSpec().WithGroup(ext.Name)) } // newStorageMigratorGenerator creates a gomock ClusterObjectSetGenerator for storage migrator tests. @@ -1135,12 +1238,53 @@ func TestBoxcutterStorageMigrator(t *testing.T) { return m } + t.Run("looks up only the group regardless of owner metadata", func(t *testing.T) { + testScheme := runtime.NewScheme() + require.NoError(t, ocv1.AddToScheme(testScheme)) + ext := &ocv1.ClusterExtension{ObjectMeta: metav1.ObjectMeta{Name: "migration-group", UID: "migration-group-uid"}, Spec: ocv1.ClusterExtensionSpec{Namespace: "test-namespace"}} + other := &ocv1.ClusterObjectSet{ + ObjectMeta: metav1.ObjectMeta{Name: "other-group-1", Labels: map[string]string{labels.OwnerNameKey: ext.Name}}, + Spec: ocv1.ClusterObjectSetSpec{Group: "other-group", Revision: 1}, + } + cl := test.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), coscontrollers.SetupIndexes). + WithStatusSubresource(&ocv1.ClusterObjectSet{}). + WithObjects(other).Build() + sm := &applier.BoxcutterStorageMigrator{ + RevisionGenerator: newStorageMigratorGenerator(t), + ActionClientGetter: newMockActionGetter(gomock.NewController(t), mockActionGetterConfig{ + currentRel: &release.Release{Name: ext.Name, Info: &release.Info{Status: release.StatusDeployed}}, + }), + Client: cl, Scheme: testScheme, FieldOwner: "test-owner", + } + require.NoError(t, sm.Migrate(t.Context(), ext, nil)) + rev := &ocv1.ClusterObjectSet{} + require.NoError(t, cl.Get(t.Context(), client.ObjectKey{Name: "test-revision"}, rev)) + require.Equal(t, ext.Name, rev.Spec.Group) + delete(rev.Labels, labels.OwnerNameKey) + require.NoError(t, cl.Update(t.Context(), rev)) + // The existing revision must short-circuit migration without Helm access. + sm.ActionClientGetter = nil + sm.RevisionGenerator = nil + require.NoError(t, sm.Migrate(t.Context(), ext, nil)) + foreignOwner := *metav1.NewControllerRef(ext, ocv1.GroupVersion.WithKind(ocv1.ClusterExtensionKind)) + foreignOwner.Name = "other-owner" + foreignOwner.UID = "other-uid" + for _, owners := range [][]metav1.OwnerReference{{foreignOwner}, nil} { + require.NoError(t, cl.Get(t.Context(), client.ObjectKeyFromObject(rev), rev)) + rev.OwnerReferences = owners + require.NoError(t, cl.Update(t.Context(), rev)) + require.NoError(t, sm.Migrate(t.Context(), ext, nil)) + } + require.NoError(t, cl.Get(t.Context(), client.ObjectKeyFromObject(other), other)) + require.Empty(t, other.Status.Conditions) + }) + t.Run("creates revision", func(t *testing.T) { testScheme := runtime.NewScheme() require.NoError(t, ocv1.AddToScheme(testScheme)) ext := &ocv1.ClusterExtension{ - ObjectMeta: metav1.ObjectMeta{Name: "test123"}, + ObjectMeta: metav1.ObjectMeta{Name: "test123", UID: "test123-uid"}, Spec: ocv1.ClusterExtensionSpec{Namespace: "test-namespace"}, } ctrl := gomock.NewController(t) @@ -1164,12 +1308,25 @@ func TestBoxcutterStorageMigrator(t *testing.T) { } mockClient.EXPECT().List(gomock.Any(), gomock.Any(), gomock.Any()). - Return(nil) + DoAndReturn(func(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error { + listOpts := (&client.ListOptions{}).ApplyOptions(opts) + require.Equal(t, ".spec.group"+"="+ext.Name, listOpts.FieldSelector.String()) + require.Nil(t, listOpts.LabelSelector) + return nil + }) mockClient.EXPECT().Apply(gomock.Any(), gomock.Any(), gomock.Any()). DoAndReturn(func(ctx context.Context, obj runtime.ApplyConfiguration, opts ...client.ApplyOption) error { // Verify the migration marker label is set before apply rev := obj.(*ocv1ac.ClusterObjectSetApplyConfiguration) require.Equal(t, "true", rev.Labels[labels.MigratedFromHelmKey], "Migration marker label should be set") + require.Equal(t, ptr.To(ext.Name), rev.Spec.Group) + require.Len(t, rev.OwnerReferences, 1) + owner := rev.OwnerReferences[0] + require.Equal(t, ptr.To(ext.Name), owner.Name) + require.Equal(t, ptr.To(ext.UID), owner.UID) + require.Equal(t, ptr.To(ocv1.ClusterExtensionKind), owner.Kind) + require.Equal(t, ptr.To(true), owner.Controller) + require.Equal(t, ptr.To(true), owner.BlockOwnerDeletion) return nil }) mockClient.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). diff --git a/internal/operator-controller/controllers/boxcutter_reconcile_steps.go b/internal/operator-controller/controllers/boxcutter_reconcile_steps.go index e623aedda2..eab11f5cef 100644 --- a/internal/operator-controller/controllers/boxcutter_reconcile_steps.go +++ b/internal/operator-controller/controllers/boxcutter_reconcile_steps.go @@ -17,12 +17,10 @@ limitations under the License. package controllers import ( - "cmp" "context" "errors" "fmt" "io/fs" - "slices" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -30,29 +28,24 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" ocv1 "github.com/operator-framework/operator-controller/api/v1" + "github.com/operator-framework/operator-controller/internal/operator-controller/revisions" "github.com/operator-framework/operator-controller/internal/shared/labels" ) +// TODO(COD): Read revision state from ClusterObjectDeployment status instead of +// discovering COS revisions directly from ClusterExtension reconciliation. type BoxcutterRevisionStatesGetter struct { Reader client.Reader } func (d *BoxcutterRevisionStatesGetter) GetRevisionStates(ctx context.Context, ext *ocv1.ClusterExtension) (*RevisionStates, error) { - // TODO: boxcutter applier has a nearly identical bit of code for listing and sorting revisions - // only difference here is that it sorts in reverse order to start iterating with the most - // recent revisions. We should consolidate to avoid code duplication. - existingRevisionList := &ocv1.ClusterObjectSetList{} - if err := d.Reader.List(ctx, existingRevisionList, client.MatchingLabels{ - labels.OwnerNameKey: ext.Name, - }); err != nil { + existingRevisions, err := revisions.ListForClusterExtension(ctx, d.Reader, ext) + if err != nil { return nil, fmt.Errorf("listing revisions: %w", err) } - slices.SortFunc(existingRevisionList.Items, func(a, b ocv1.ClusterObjectSet) int { - return cmp.Compare(a.Spec.Revision, b.Spec.Revision) - }) rs := &RevisionStates{} - for _, rev := range existingRevisionList.Items { + for _, rev := range existingRevisions { if rev.Spec.LifecycleState == ocv1.ClusterObjectSetLifecycleStateArchived { continue } diff --git a/internal/operator-controller/controllers/boxcutter_reconcile_steps_internal_test.go b/internal/operator-controller/controllers/boxcutter_reconcile_steps_internal_test.go new file mode 100644 index 0000000000..65a3c93c94 --- /dev/null +++ b/internal/operator-controller/controllers/boxcutter_reconcile_steps_internal_test.go @@ -0,0 +1,72 @@ +/* +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" + "testing" + + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + apimachineryruntime "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + ocv1 "github.com/operator-framework/operator-controller/api/v1" + coscontrollers "github.com/operator-framework/operator-controller/internal/object-controller/controllers" + "github.com/operator-framework/operator-controller/test" +) + +func TestBoxcutterRevisionStatesGetter_ClassifiesByCompletedAt(t *testing.T) { + sch := apimachineryruntime.NewScheme() + require.NoError(t, ocv1.AddToScheme(sch)) + + const extName = "test-ext" + + newRevision := func(name string, revision int64, completed bool) *ocv1.ClusterObjectSet { + cos := &ocv1.ClusterObjectSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + }, + Spec: ocv1.ClusterObjectSetSpec{Group: extName, Revision: revision}, + } + if completed { + cos.Status.CompletedAt = metav1.Now() + } + return cos + } + + completed := newRevision("test-ext-1", 1, true) + rollingOut := newRevision("test-ext-2", 2, false) + + cl := test.WithIndexes(t, fake.NewClientBuilder().WithScheme(sch), coscontrollers.SetupIndexes). + WithObjects(completed, rollingOut). + Build() + + getter := &BoxcutterRevisionStatesGetter{Reader: cl} + states, err := getter.GetRevisionStates(context.Background(), &ocv1.ClusterExtension{ + ObjectMeta: metav1.ObjectMeta{Name: extName}, + }) + require.NoError(t, err) + + // A revision with completedAt set is Installed. + require.NotNil(t, states.Installed) + require.Equal(t, "test-ext-1", states.Installed.RevisionName) + + // A revision without completedAt is still RollingOut. + require.Len(t, states.RollingOut, 1) + require.Equal(t, "test-ext-2", states.RollingOut[0].RevisionName) +} diff --git a/internal/operator-controller/controllers/boxcutter_reconcile_steps_test.go b/internal/operator-controller/controllers/boxcutter_reconcile_steps_test.go index 5ce9b7259a..39bf3c300e 100644 --- a/internal/operator-controller/controllers/boxcutter_reconcile_steps_test.go +++ b/internal/operator-controller/controllers/boxcutter_reconcile_steps_test.go @@ -1,73 +1,101 @@ -/* -Copyright 2026. +//go:build !standard -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 +package controllers_test import ( "context" "testing" + "time" "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - apimachineryruntime "k8s.io/apimachinery/pkg/runtime" - "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/cache" + "sigs.k8s.io/controller-runtime/pkg/client" ocv1 "github.com/operator-framework/operator-controller/api/v1" + coscontrollers "github.com/operator-framework/operator-controller/internal/object-controller/controllers" + "github.com/operator-framework/operator-controller/internal/operator-controller/controllers" "github.com/operator-framework/operator-controller/internal/shared/labels" ) -func TestBoxcutterRevisionStatesGetter_ClassifiesByCompletedAt(t *testing.T) { - sch := apimachineryruntime.NewScheme() - require.NoError(t, ocv1.AddToScheme(sch)) - - const extName = "test-ext" - - newRevision := func(name string, revision int64, completed bool) *ocv1.ClusterObjectSet { +func TestBoxcutterRevisionStatesGetter_GroupIndex(t *testing.T) { + scheme := newScheme(t) + cl, err := client.New(config, client.Options{Scheme: scheme}) + require.NoError(t, err) + group := "cache-group" + ext := &ocv1.ClusterExtension{ObjectMeta: metav1.ObjectMeta{Name: group, UID: "cache-group-uid"}} + for _, tc := range []struct { + name string + group string + revision int64 + archived bool + succeeded bool + ownerName string + }{ + {name: "cache-installed", group: group, revision: 1, succeeded: true, ownerName: "other-owner"}, + {name: "cache-rolling", group: group, revision: 2}, + {name: "cache-archived", group: group, revision: 3, archived: true, succeeded: true, ownerName: group}, + {name: "cache-other-group", group: "other-group", revision: 99, succeeded: true, ownerName: group}, + } { cos := &ocv1.ClusterObjectSet{ - ObjectMeta: metav1.ObjectMeta{ - Name: name, - Labels: map[string]string{labels.OwnerNameKey: extName}, + ObjectMeta: metav1.ObjectMeta{Name: tc.name}, + Spec: ocv1.ClusterObjectSetSpec{ + Group: tc.group, Revision: tc.revision, + LifecycleState: ocv1.ClusterObjectSetLifecycleStateActive, + CollisionProtection: ocv1.CollisionProtectionPrevent, }, - Spec: ocv1.ClusterObjectSetSpec{Revision: revision}, } - if completed { + if tc.group != group { + cos.Labels = map[string]string{labels.OwnerNameKey: group} + } + if tc.ownerName != "" { + owner := *metav1.NewControllerRef(ext, ocv1.GroupVersion.WithKind(ocv1.ClusterExtensionKind)) + owner.Name = tc.ownerName + if tc.ownerName != ext.Name { + owner.UID = "other-owner-uid" + } + cos.OwnerReferences = []metav1.OwnerReference{owner} + } + if tc.archived { + cos.Spec.LifecycleState = ocv1.ClusterObjectSetLifecycleStateArchived + } + require.NoError(t, cl.Create(t.Context(), cos)) + t.Cleanup(func() { require.NoError(t, client.IgnoreNotFound(cl.Delete(context.Background(), cos))) }) + if tc.succeeded { cos.Status.CompletedAt = metav1.Now() + require.NoError(t, cl.Status().Update(t.Context(), cos)) } - return cos } - completed := newRevision("test-ext-1", 1, true) - rollingOut := newRevision("test-ext-2", 2, false) - - cl := fake.NewClientBuilder(). - WithScheme(sch). - WithObjects(completed, rollingOut). - Build() - - getter := &BoxcutterRevisionStatesGetter{Reader: cl} - states, err := getter.GetRevisionStates(context.Background(), &ocv1.ClusterExtension{ - ObjectMeta: metav1.ObjectMeta{Name: extName}, + managerCache, err := cache.New(config, cache.Options{Scheme: scheme}) + require.NoError(t, err) + ctx, cancel := context.WithTimeout(t.Context(), 30*time.Second) + require.NoError(t, coscontrollers.SetupIndexes(ctx, managerCache)) + done := make(chan error, 1) + go func() { done <- managerCache.Start(ctx) }() + t.Cleanup(func() { + cancel() + select { + case err := <-done: + require.NoError(t, err) + case <-time.After(5 * time.Second): + t.Fatal("manager cache did not stop") + } }) + require.True(t, managerCache.WaitForCacheSync(ctx)) + cachedClient, err := client.New(config, client.Options{Scheme: scheme, Cache: &client.CacheOptions{Reader: managerCache}}) + require.NoError(t, err) + getter := controllers.BoxcutterRevisionStatesGetter{Reader: cachedClient} + states, err := getter.GetRevisionStates(ctx, ext) require.NoError(t, err) - - // A revision with completedAt set is Installed. require.NotNil(t, states.Installed) - require.Equal(t, "test-ext-1", states.Installed.RevisionName) - - // A revision without completedAt is still RollingOut. + require.Equal(t, "cache-installed", states.Installed.RevisionName) require.Len(t, states.RollingOut, 1) - require.Equal(t, "test-ext-2", states.RollingOut[0].RevisionName) + require.Equal(t, "cache-rolling", states.RollingOut[0].RevisionName) + + require.NoError(t, cl.Delete(ctx, &ocv1.ClusterObjectSet{ObjectMeta: metav1.ObjectMeta{Name: "cache-rolling"}})) + require.Eventually(t, func() bool { + states, err := getter.GetRevisionStates(ctx, ext) + return err == nil && len(states.RollingOut) == 0 + }, 5*time.Second, 10*time.Millisecond) } diff --git a/internal/operator-controller/controllers/clusterextension_controller.go b/internal/operator-controller/controllers/clusterextension_controller.go index 70e6d6f572..2367ad88a1 100644 --- a/internal/operator-controller/controllers/clusterextension_controller.go +++ b/internal/operator-controller/controllers/clusterextension_controller.go @@ -32,6 +32,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/sets" + "pkg.package-operator.run/boxcutter/managedcache" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" @@ -42,7 +43,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/predicate" "sigs.k8s.io/controller-runtime/pkg/reconcile" - "sigs.k8s.io/controller-runtime/pkg/source" "github.com/operator-framework/api/pkg/operators/v1alpha1" helmclient "github.com/operator-framework/helm-operator-plugins/pkg/client" @@ -420,23 +420,39 @@ func collectDeprecationMessages(entries []declcfg.DeprecationEntry) []string { return messages } -type ControllerBuilderOption func(builder *ctrl.Builder) +// 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. +func (r *ClusterExtensionReconciler) SetupWithManagerForBoxcutter(mgr ctrl.Manager) (crcontroller.Controller, error) { + return r.newControllerBuilder(mgr). + Watches(&ocv1.ClusterObjectSet{}, + crhandler.EnqueueRequestsFromMapFunc(clusterExtensionRequestsForClusterObjectSet)). + Build(r) +} -func WithOwns(obj client.Object) ControllerBuilderOption { - return func(builder *ctrl.Builder) { - builder.Owns(obj) - } +// SetupWithManagerForHelm sets up the controller with watches for managed Helm resources. +func (r *ClusterExtensionReconciler) SetupWithManagerForHelm(mgr ctrl.Manager, trackingCache managedcache.TrackingCache) (crcontroller.Controller, error) { + return r.newControllerBuilder(mgr). + WatchesRawSource(trackingCache.Source( + crhandler.EnqueueRequestForOwner(mgr.GetScheme(), mgr.GetRESTMapper(), &ocv1.ClusterExtension{}), + predicate.ResourceVersionChangedPredicate{}, + predicate.Funcs{ + CreateFunc: func(event.TypedCreateEvent[client.Object]) bool { return false }, + }, + )). + Build(r) } -func WithWatchesRawSource(src source.Source) ControllerBuilderOption { - return func(b *ctrl.Builder) { - b.WatchesRawSource(src) +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}}} } -// SetupWithManager sets up the controller with the Manager. -func (r *ClusterExtensionReconciler) SetupWithManager(mgr ctrl.Manager, opts ...ControllerBuilderOption) (crcontroller.Controller, error) { - ctrlBuilder := ctrl.NewControllerManagedBy(mgr). +func (r *ClusterExtensionReconciler) newControllerBuilder(mgr ctrl.Manager) *ctrl.Builder { + return ctrl.NewControllerManagedBy(mgr). For(&ocv1.ClusterExtension{}). Named("controller-operator-cluster-extension-controller"). Watches(&ocv1.ClusterCatalog{}, @@ -458,12 +474,6 @@ func (r *ClusterExtensionReconciler) SetupWithManager(mgr ctrl.Manager, opts ... return true }, })) - - for _, applyOpt := range opts { - applyOpt(ctrlBuilder) - } - - return ctrlBuilder.Build(r) } func wrapErrorWithResolutionInfo(resolved ocv1.BundleMetadata, err error) error { diff --git a/internal/operator-controller/controllers/clusterextension_controller_internal_test.go b/internal/operator-controller/controllers/clusterextension_controller_internal_test.go new file mode 100644 index 0000000000..31363ea79e --- /dev/null +++ b/internal/operator-controller/controllers/clusterextension_controller_internal_test.go @@ -0,0 +1,46 @@ +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/revisions/clusterobjectsets.go b/internal/operator-controller/revisions/clusterobjectsets.go new file mode 100644 index 0000000000..ae9b38feca --- /dev/null +++ b/internal/operator-controller/revisions/clusterobjectsets.go @@ -0,0 +1,41 @@ +/* +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 revisions + +import ( + "cmp" + "context" + "slices" + + "sigs.k8s.io/controller-runtime/pkg/client" + + ocv1 "github.com/operator-framework/operator-controller/api/v1" +) + +// ListForClusterExtension returns all COS revisions in the extension's group, oldest first. +// CE-generated revisions use the extension name as their group. Owner references +// do not affect revision association. Lifecycle filtering is left to callers. +func ListForClusterExtension(ctx context.Context, reader client.Reader, ext *ocv1.ClusterExtension) ([]ocv1.ClusterObjectSet, error) { + list := &ocv1.ClusterObjectSetList{} + if err := reader.List(ctx, list, client.MatchingFields{".spec.group": ext.Name}); err != nil { + return nil, err + } + slices.SortFunc(list.Items, func(a, b ocv1.ClusterObjectSet) int { + return cmp.Compare(a.Spec.Revision, b.Spec.Revision) + }) + return list.Items, nil +} diff --git a/internal/operator-controller/revisions/clusterobjectsets_test.go b/internal/operator-controller/revisions/clusterobjectsets_test.go new file mode 100644 index 0000000000..208f1d92dd --- /dev/null +++ b/internal/operator-controller/revisions/clusterobjectsets_test.go @@ -0,0 +1,88 @@ +/* +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 revisions_test + +import ( + "testing" + + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/utils/ptr" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + ocv1 "github.com/operator-framework/operator-controller/api/v1" + coscontrollers "github.com/operator-framework/operator-controller/internal/object-controller/controllers" + "github.com/operator-framework/operator-controller/internal/operator-controller/revisions" + "github.com/operator-framework/operator-controller/internal/shared/labels" + "github.com/operator-framework/operator-controller/test" +) + +func TestListForClusterExtension(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, ocv1.AddToScheme(scheme)) + ext := &ocv1.ClusterExtension{ObjectMeta: metav1.ObjectMeta{Name: "test-ext", UID: "current-uid"}} + owner := *metav1.NewControllerRef(ext, ocv1.GroupVersion.WithKind(ocv1.ClusterExtensionKind)) + differentName, differentKind, differentIdentity := owner, owner, owner + differentName.Name = "other-ext" + differentKind.Kind = "OtherController" + differentIdentity.UID = "previous-uid" + differentIdentity.APIVersion = "olm.operatorframework.io/v2" + nonController, unspecifiedController := owner, owner + nonController.Controller = ptr.To(false) + unspecifiedController.Controller = nil + + objects := make([]client.Object, 0, 9) + for _, tc := range []struct { + name string + group string + owners []metav1.OwnerReference + revision int64 + }{ + {name: "matching-owner", group: ext.Name, owners: []metav1.OwnerReference{owner}, revision: 2}, + {name: "oldest", group: ext.Name, owners: []metav1.OwnerReference{differentIdentity}, revision: 1}, + {name: "other-group", group: "other-group", owners: []metav1.OwnerReference{owner}, revision: 99}, + {name: "other-name", group: ext.Name, owners: []metav1.OwnerReference{differentName}, revision: 100}, + {name: "other-kind", group: ext.Name, owners: []metav1.OwnerReference{differentKind}, revision: 101}, + {name: "ownerless", group: ext.Name, revision: 102}, + {name: "non-controller", group: ext.Name, owners: []metav1.OwnerReference{nonController}, revision: 103}, + {name: "unspecified-controller", group: ext.Name, owners: []metav1.OwnerReference{unspecifiedController}, revision: 104}, + {name: "foreign-controller-with-ce-owner", group: ext.Name, owners: []metav1.OwnerReference{differentKind, nonController}, revision: 105}, + } { + objects = append(objects, &ocv1.ClusterObjectSet{ + ObjectMeta: metav1.ObjectMeta{Name: tc.name, OwnerReferences: tc.owners}, + Spec: ocv1.ClusterObjectSetSpec{Group: tc.group, Revision: tc.revision}, + }) + } + // Labels cannot override group membership, regardless of owner references. + for _, object := range objects[2:] { + object.SetLabels(map[string]string{labels.OwnerNameKey: ext.Name}) + } + cl := test.WithIndexes(t, fake.NewClientBuilder().WithScheme(scheme), coscontrollers.SetupIndexes). + WithObjects(objects...).Build() + got, err := revisions.ListForClusterExtension(t.Context(), cl, ext) + require.NoError(t, err) + names := make([]string, 0, len(got)) + for _, cos := range got { + names = append(names, cos.Name) + } + require.Equal(t, []string{ + "oldest", "matching-owner", "other-name", "other-kind", "ownerless", + "non-controller", "unspecified-controller", "foreign-controller-with-ce-owner", + }, names) +} diff --git a/test/indexes.go b/test/indexes.go new file mode 100644 index 0000000000..032d578fbe --- /dev/null +++ b/test/indexes.go @@ -0,0 +1,28 @@ +package test + +import ( + "context" + "testing" + + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +// WithIndexes registers controller cache indexes on a fake client builder. +// The builder must already have its scheme configured. +func WithIndexes(t *testing.T, builder *fake.ClientBuilder, setup func(context.Context, client.FieldIndexer) error) *fake.ClientBuilder { + t.Helper() + if err := setup(t.Context(), fakeFieldIndexer{builder: builder}); err != nil { + t.Fatalf("setting up fake client indexes: %v", err) + } + return builder +} + +type fakeFieldIndexer struct { + builder *fake.ClientBuilder +} + +func (f fakeFieldIndexer) IndexField(_ context.Context, obj client.Object, field string, extract client.IndexerFunc) error { + f.builder.WithIndex(obj, field, extract) + return nil +} From b13b91b102dfae20834b707d5c0f39ea2fca82af Mon Sep 17 00:00:00 2001 From: Francesco Giudici Date: Thu, 1 Oct 2026 14:49:27 +0200 Subject: [PATCH 04/10] Cover group tracking in experimental E2E tests Check generated groups, shared SSA ownership during rollout, pruning isolation, and cascade cleanup. Update direct COS fixtures and query revision associations by spec.group. Exercise handoffs between different owners and between owned and ownerless revisions. Verify the workload UID and contents survive previous-revision archival. Verify different groups still collide even when they share a controller owner. Co-authored-by: Codex Signed-off-by: Francesco Giudici --- test/e2e/features/group.feature | 211 ++++++++++++++++++++++++++++ test/e2e/features/install.feature | 2 + test/e2e/features/revision.feature | 9 ++ test/e2e/features/uninstall.feature | 10 ++ test/e2e/features/update.feature | 4 + test/e2e/steps/group_steps.go | 211 ++++++++++++++++++++++++++++ test/e2e/steps/hooks.go | 3 + test/e2e/steps/steps.go | 41 +++--- 8 files changed, 475 insertions(+), 16 deletions(-) create mode 100644 test/e2e/features/group.feature create mode 100644 test/e2e/steps/group_steps.go diff --git a/test/e2e/features/group.feature b/test/e2e/features/group.feature new file mode 100644 index 0000000000..2dc31a07e9 --- /dev/null +++ b/test/e2e/features/group.feature @@ -0,0 +1,211 @@ +@BoxcutterRuntime +Feature: Track ClusterObjectSet revisions by group + + Background: + Given OLM is available + And an image registry is available + And namespace "${TEST_NAMESPACE}" is available + + Scenario Outline: Group handoff preserves objects across + Given resource is applied + """ + apiVersion: v1 + kind: Namespace + metadata: + name: ${TEST_NAMESPACE}-other + """ + And ClusterObjectSet with Namespace owner "" is applied + """ + apiVersion: olm.operatorframework.io/v1 + kind: ClusterObjectSet + metadata: + name: ${NAME}-1 + spec: + group: ${NAME} + revision: 1 + lifecycleState: Active + collisionProtection: Prevent + phases: + - name: config + objects: + - object: + apiVersion: v1 + kind: ConfigMap + metadata: + name: ${NAME}-payload + namespace: ${TEST_NAMESPACE} + data: + revision: "1" + """ + Then ClusterObjectSet "${NAME}-1" reports Ready as True with Reason AllObjectsReady + And ClusterObjectSet "${NAME}-1" phase objects use SSA manager "cos-group/${NAME}" + And resource "configmap/${NAME}-payload" UID is remembered + When ClusterObjectSet with Namespace owner "" is applied + """ + apiVersion: olm.operatorframework.io/v1 + kind: ClusterObjectSet + metadata: + name: ${NAME}-2 + spec: + group: ${NAME} + revision: 2 + lifecycleState: Active + collisionProtection: Prevent + phases: + - name: config + objects: + - object: + apiVersion: v1 + kind: ConfigMap + metadata: + name: ${NAME}-payload + namespace: ${TEST_NAMESPACE} + data: + revision: "2" + """ + Then ClusterObjectSet "${NAME}-2" reports Ready as True with Reason AllObjectsReady + And ClusterObjectSet "${NAME}-1" is archived + And ClusterObjectSet "${NAME}-2" phase objects use SSA manager "cos-group/${NAME}" + And resource "configmap/${NAME}-payload" matches + """ + data: + revision: "2" + """ + And resource "configmap/${NAME}-payload" keeps its UID + + # An empty owner creates an ownerless COS. Namespace owners are real, + # cluster-scoped objects, independent of CE and future COD controllers. + Examples: + | ownership | lower-owner | higher-owner | + | different controllers | ${TEST_NAMESPACE} | ${TEST_NAMESPACE}-other | + | owned to ownerless | ${TEST_NAMESPACE} | | + | ownerless to owned | | ${TEST_NAMESPACE} | + + Scenario: Different groups collide even with the same controller owner + Given ClusterObjectSet with Namespace owner "${TEST_NAMESPACE}" is applied + """ + apiVersion: olm.operatorframework.io/v1 + kind: ClusterObjectSet + metadata: + name: ${NAME}-1 + spec: + group: ${NAME} + revision: 1 + lifecycleState: Active + collisionProtection: Prevent + phases: + - name: config + objects: + - object: + apiVersion: v1 + kind: ConfigMap + metadata: + name: ${NAME}-payload + namespace: ${TEST_NAMESPACE} + data: + revision: "1" + """ + Then ClusterObjectSet "${NAME}-1" reports Ready as True with Reason AllObjectsReady + And resource "configmap/${NAME}-payload" UID is remembered + When ClusterObjectSet with Namespace owner "${TEST_NAMESPACE}" is applied + """ + apiVersion: olm.operatorframework.io/v1 + kind: ClusterObjectSet + metadata: + name: ${NAME}-other-2 + spec: + group: ${NAME}-other + revision: 2 + lifecycleState: Active + collisionProtection: Prevent + phases: + - name: config + objects: + - object: + apiVersion: v1 + kind: ConfigMap + metadata: + name: ${NAME}-payload + namespace: ${TEST_NAMESPACE} + data: + revision: "2" + """ + Then ClusterObjectSet "${NAME}-other-2" reports Ready as False with Reason ObjectCollision and Message includes: + """ + Cannot take ownership of 1 object(s) + """ + And ClusterObjectSet "${NAME}-1" phase objects use SSA manager "cos-group/${NAME}" + And resource "configmap/${NAME}-payload" matches + """ + data: + revision: "1" + """ + And resource "configmap/${NAME}-payload" keeps its UID + And resource "clusterobjectset/${NAME}-1" matches + """ + spec: + lifecycleState: Active + """ + + Scenario: Revision pruning is isolated by group + Given a catalog "test" with packages: + | package | version | channel | replaces | contents | + | test | 1.0.0 | beta | | CRD, Deployment, ConfigMap | + | test | 1.0.1 | beta | 1.0.0 | CRD, Deployment, ConfigMap | + | test | 1.0.2 | beta | 1.0.1 | CRD, Deployment, ConfigMap | + | test | 1.0.3 | beta | 1.0.2 | CRD, Deployment, ConfigMap | + | test | 1.0.4 | beta | 1.0.3 | CRD, Deployment, ConfigMap | + | test | 1.0.5 | beta | 1.0.4 | CRD, Deployment, ConfigMap | + | test | 1.0.6 | beta | 1.0.5 | CRD, Deployment, ConfigMap | + And ClusterObjectSet is applied + """ + apiVersion: olm.operatorframework.io/v1 + kind: ClusterObjectSet + metadata: + name: ${COS_NAME} + labels: + olm.operatorframework.io/owner-name: ${NAME} + spec: + group: ${NAME}-other + revision: 1 + lifecycleState: Archived + collisionProtection: Prevent + """ + And ClusterExtension is applied + """ + apiVersion: olm.operatorframework.io/v1 + kind: ClusterExtension + metadata: + name: ${NAME} + spec: + namespace: ${TEST_NAMESPACE} + source: + sourceType: Catalog + catalog: + packageName: ${PACKAGE:test} + version: 1.0.0 + selector: + matchLabels: + olm.operatorframework.io/metadata.name: ${CATALOG:test} + """ + And ClusterExtension is rolled out + When ClusterExtension version is updated to "1.0.1" + Then ClusterExtension is rolled out + When ClusterExtension version is updated to "1.0.2" + Then ClusterExtension is rolled out + When ClusterExtension version is updated to "1.0.3" + Then ClusterExtension is rolled out + When ClusterExtension version is updated to "1.0.4" + Then ClusterExtension is rolled out + When ClusterExtension version is updated to "1.0.5" + Then ClusterExtension is rolled out + When ClusterExtension version is updated to "1.0.6" + Then ClusterExtension is rolled out + And ClusterExtension is available + And ClusterExtension reports "${NAME}-7" as active revision + 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 + 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 96b2fea0be..3a9ce02224 100644 --- a/test/e2e/features/install.feature +++ b/test/e2e/features/install.feature @@ -465,6 +465,8 @@ Feature: Install ClusterExtension And ClusterExtension is available 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 diff --git a/test/e2e/features/revision.feature b/test/e2e/features/revision.feature index 74dbb72861..bbf7ecb939 100644 --- a/test/e2e/features/revision.feature +++ b/test/e2e/features/revision.feature @@ -15,6 +15,7 @@ Feature: Install ClusterObjectSet metadata: name: ${COS_NAME} spec: + group: ${NAME} lifecycleState: Active collisionProtection: Prevent progressionProbes: @@ -75,6 +76,7 @@ Feature: Install ClusterObjectSet metadata: name: ${COS_NAME} spec: + group: ${NAME} lifecycleState: Active collisionProtection: Prevent progressionProbes: @@ -161,6 +163,7 @@ Feature: Install ClusterObjectSet metadata: name: ${COS_NAME} spec: + group: ${NAME} lifecycleState: Active collisionProtection: Prevent progressionProbes: @@ -219,6 +222,7 @@ Feature: Install ClusterObjectSet metadata: name: ${COS_NAME} spec: + group: ${NAME} lifecycleState: Active collisionProtection: Prevent progressionProbes: @@ -403,6 +407,7 @@ Feature: Install ClusterObjectSet metadata: name: ${COS_NAME} spec: + group: ${NAME} lifecycleState: Active collisionProtection: Prevent phases: @@ -454,6 +459,7 @@ Feature: Install ClusterObjectSet metadata: name: ${COS_NAME} spec: + group: ${NAME} lifecycleState: Active collisionProtection: Prevent phases: @@ -502,6 +508,7 @@ Feature: Install ClusterObjectSet metadata: name: ${COS_NAME} spec: + group: ${NAME} lifecycleState: Active collisionProtection: Prevent phases: @@ -585,6 +592,7 @@ Feature: Install ClusterObjectSet metadata: name: ${COS_NAME} spec: + group: ${NAME} lifecycleState: Active collisionProtection: Prevent progressDeadlineMinutes: 1 @@ -651,6 +659,7 @@ Feature: Install ClusterObjectSet metadata: name: ${COS_NAME} spec: + group: ${NAME} lifecycleState: Active collisionProtection: Prevent progressDeadlineMinutes: 1 diff --git a/test/e2e/features/uninstall.feature b/test/e2e/features/uninstall.feature index bd2fd7b5f4..973d90ab5a 100644 --- a/test/e2e/features/uninstall.feature +++ b/test/e2e/features/uninstall.feature @@ -34,3 +34,13 @@ Feature: Uninstall ClusterExtension When ClusterExtension is removed Then the ClusterExtension's constituent resources are removed + @BoxcutterRuntime + Scenario: Removing ClusterExtension cascades to grouped revisions and their content 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 ClusterObjectSet "${NAME}-1" referred secrets are remembered + When ClusterExtension is removed + Then resource "clusterobjectset/${NAME}-1" 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 db1b347b38..3c39d34cbc 100644 --- a/test/e2e/features/update.feature +++ b/test/e2e/features/update.feature @@ -310,6 +310,8 @@ Feature: Update ClusterExtension """ And ClusterExtension is rolled out And ClusterExtension is available + And ClusterObjectSet "${NAME}-1" has group "${NAME}" + And ClusterObjectSet "${NAME}-1" phase objects use SSA manager "cos-group/${NAME}" When ClusterExtension version is updated to "1.2.0" Then bundle "${PACKAGE:test}.1.2.0" is installed in version "1.2.0" And ClusterExtension is rolled out @@ -318,6 +320,8 @@ Feature: Update ClusterExtension And ClusterObjectSet "${NAME}-2" reports Ready as True with Reason AllObjectsReady And ClusterObjectSet "${NAME}-1" is archived And ClusterObjectSet "${NAME}-1" phase objects are not found or not owned by the revision + And ClusterObjectSet "${NAME}-2" has group "${NAME}" + And ClusterObjectSet "${NAME}-2" phase objects use SSA manager "cos-group/${NAME}" @BoxcutterRuntime Scenario: Report all active revisions on ClusterExtension diff --git a/test/e2e/steps/group_steps.go b/test/e2e/steps/group_steps.go new file mode 100644 index 0000000000..204fd42dee --- /dev/null +++ b/test/e2e/steps/group_steps.go @@ -0,0 +1,211 @@ +package steps + +import ( + "context" + "encoding/json" + "fmt" + "strings" + + "github.com/cucumber/godog" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + "k8s.io/utils/ptr" + "sigs.k8s.io/yaml" + + ocv1 "github.com/operator-framework/operator-controller/api/v1" +) + +// An empty owner name creates an ownerless revision. Resolve a real cluster-scoped +// owner before creation so garbage collection and reconciliation see valid metadata. +func ClusterObjectSetWithNamespaceOwnerApplied(ctx context.Context, ownerName string, template *godog.DocString) error { + sc := scenarioCtx(ctx) + obj, err := toUnstructured(substituteScenarioVars(template.Content, sc)) + if err != nil { + return err + } + ownerName = substituteScenarioVars(ownerName, sc) + if ownerName != "" { + owner, err := getResource("namespace", ownerName, "") + if err != nil { + return err + } + obj.SetOwnerReferences([]metav1.OwnerReference{{ + APIVersion: "v1", Kind: "Namespace", Name: ownerName, UID: owner.GetUID(), + Controller: ptr.To(true), BlockOwnerDeletion: ptr.To(true), + }}) + } else { + obj.SetOwnerReferences(nil) + } + content, err := yaml.Marshal(obj.Object) + if err != nil { + return err + } + return ResourceIsApplied(ctx, &godog.DocString{Content: string(content)}) +} + +func RememberResourceUID(ctx context.Context, resourceName string) error { + sc := scenarioCtx(ctx) + resourceName = substituteScenarioVars(resourceName, sc) + kind, name, found := strings.Cut(resourceName, "/") + if !found { + return fmt.Errorf("resource %s is not in the format /", resourceName) + } + obj, err := getResource(kind, name, sc.namespace) + if err != nil { + return err + } + if obj.GetUID() == "" { + return fmt.Errorf("resource %s has no UID", resourceName) + } + if sc.resourceUIDs == nil { + sc.resourceUIDs = map[string]types.UID{} + } + sc.resourceUIDs[resourceName] = obj.GetUID() + return nil +} + +func ResourceKeepsUID(ctx context.Context, resourceName string) error { + sc := scenarioCtx(ctx) + resourceName = substituteScenarioVars(resourceName, sc) + uid, found := sc.resourceUIDs[resourceName] + if !found { + return fmt.Errorf("no UID remembered for resource %s", resourceName) + } + kind, name, _ := strings.Cut(resourceName, "/") + obj, err := getResource(kind, name, sc.namespace) + if err != nil { + return err + } + if obj.GetUID() != uid { + return fmt.Errorf("resource %s was replaced: UID changed from %s to %s", resourceName, uid, obj.GetUID()) + } + return nil +} + +// A local cache index does not make spec.group selectable through kubectl. +func listClusterObjectSetsInGroup(ctx context.Context, group string) ([]ocv1.ClusterObjectSet, error) { + out, err := k8sClient(ctx, "get", "clusterobjectsets", "-o", "json") + if err != nil { + return nil, err + } + var list ocv1.ClusterObjectSetList + if err := json.Unmarshal([]byte(out), &list); err != nil { + return nil, err + } + var revisions []ocv1.ClusterObjectSet + for _, rev := range list.Items { + if rev.Spec.Group == group { + revisions = append(revisions, rev) + } + } + return revisions, nil +} + +func ClusterObjectSetHasGroup(ctx context.Context, revisionName, group string) error { + sc := scenarioCtx(ctx) + revisionName = substituteScenarioVars(revisionName, sc) + group = substituteScenarioVars(group, sc) + waitFor(ctx, func() bool { + obj, err := getResource("clusterobjectset", revisionName, "") + if err != nil { + return false + } + var cos ocv1.ClusterObjectSet + return runtime.DefaultUnstructuredConverter.FromUnstructured(obj.Object, &cos) == nil && cos.Spec.Group == group + }) + return nil +} + +// ClusterObjectSetObjectsUseSSAManager checks actual managed fields on every +// inline or externally stored phase object, including their revision owner. +func ClusterObjectSetObjectsUseSSAManager(ctx context.Context, revisionName, manager string) error { + sc := scenarioCtx(ctx) + revisionName = substituteScenarioVars(revisionName, sc) + manager = substituteScenarioVars(manager, sc) + waitFor(ctx, func() bool { + obj, err := getResource("clusterobjectset", revisionName, "") + if err != nil { + return false + } + var cos ocv1.ClusterObjectSet + if err := runtime.DefaultUnstructuredConverter.FromUnstructured(obj.Object, &cos); err != nil { + return false + } + count := 0 + for _, phase := range cos.Spec.Phases { + for _, phaseObj := range phase.Objects { + desired := &phaseObj.Object + if phaseObj.Ref.Name != "" { + desired, err = resolveObjectRef(phaseObj.Ref) + if err != nil { + return false + } + } + // kubectl's JSON/YAML printers omit managedFields by default. + args := []string{"get", desired.GetKind(), desired.GetName(), "-o", "json", "--show-managed-fields"} + if desired.GetNamespace() != "" { + args = append(args, "-n", desired.GetNamespace()) + } + out, err := k8sClient(ctx, args...) + if err != nil { + return false + } + actual := &unstructured.Unstructured{} + if err := json.Unmarshal([]byte(out), &actual.Object); err != nil { + return false + } + owner := metav1.GetControllerOf(actual) + if owner == nil || owner.Kind != ocv1.ClusterObjectSetKind || owner.UID != cos.UID { + return false + } + found := false + for _, fields := range actual.GetManagedFields() { + if fields.Operation != metav1.ManagedFieldsOperationApply { + continue + } + if strings.HasPrefix(fields.Manager, "cos-group/") && fields.Manager != manager { + return false + } + if fields.Manager == manager { + found = true + } + } + if !found { + return false + } + count++ + } + } + return count > 0 + }) + return nil +} + +func RememberClusterObjectSetSecrets(ctx context.Context, revisionName string) error { + sc := scenarioCtx(ctx) + revisionName = substituteScenarioVars(revisionName, sc) + names, err := collectReferredSecretNames(ctx, revisionName) + if err != nil { + return err + } + for _, name := range names { + sc.revisionSecrets = append(sc.revisionSecrets, resource{kind: "secret", name: name, namespace: olmNamespace}) + } + return nil +} + +func RememberedRevisionSecretsRemoved(ctx context.Context) error { + sc := scenarioCtx(ctx) + if len(sc.revisionSecrets) == 0 { + return fmt.Errorf("no revision secrets were remembered") + } + for _, secret := range sc.revisionSecrets { + waitFor(ctx, func() bool { + out, err := k8sClient(ctx, "get", secret.kind, secret.name, "-n", secret.namespace, "--ignore-not-found", "-o", "name") + return err == nil && strings.TrimSpace(out) == "" + }) + } + return nil +} diff --git a/test/e2e/steps/hooks.go b/test/e2e/steps/hooks.go index ee93303408..3278a31e0e 100644 --- a/test/e2e/steps/hooks.go +++ b/test/e2e/steps/hooks.go @@ -17,6 +17,7 @@ import ( "golang.org/x/sync/errgroup" appsv1 "k8s.io/api/apps/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/types" "k8s.io/component-base/featuregate" "k8s.io/klog/v2/textlogger" "sigs.k8s.io/controller-runtime/pkg/client" @@ -56,6 +57,8 @@ type scenarioContext struct { leaderPods map[string]string // component name -> leader pod name deploymentRestores []deploymentRestore extensionObjects []client.Object + revisionSecrets []resource + resourceUIDs map[string]types.UID proxy *recordingProxy catalogAddr string catalogCleanup func() diff --git a/test/e2e/steps/steps.go b/test/e2e/steps/steps.go index 9ecc41fc8e..271e88bed7 100644 --- a/test/e2e/steps/steps.go +++ b/test/e2e/steps/steps.go @@ -129,6 +129,7 @@ func RegisterSteps(sc *godog.ScenarioContext) { sc.Step(`^(?i)ClusterObjectSet "([^"]+)" reports ([[:alnum:]]+) as ([[:alnum:]]+) with Reason ([[:alnum:]]+) and Message includes:$`, ClusterObjectSetReportsConditionWithMessageFragment) sc.Step(`^(?i)ClusterExtension reports ([[:alnum:]]+) transition between (\d+) and (\d+) minutes since its creation$`, ClusterExtensionReportsConditionTransitionTime) sc.Step(`^(?i)ClusterObjectSet is applied(?:\s+.*)?$`, ResourceIsApplied) + sc.Step(`^(?i)ClusterObjectSet with Namespace owner "([^"]*)" is applied$`, ClusterObjectSetWithNamespaceOwnerApplied) sc.Step(`^(?i)ClusterObjectSet "([^"]+)" reconciliation is triggered$`, TriggerClusterObjectSetReconciliation) sc.Step(`^(?i)ClusterObjectSet "([^"]+)" has observed phase "([^"]+)" with a non-empty digest$`, ClusterObjectSetHasObservedPhase) sc.Step(`^(?i)ClusterObjectSet "([^"]+)" is archived$`, ClusterObjectSetIsArchived) @@ -136,6 +137,12 @@ func RegisterSteps(sc *godog.ScenarioContext) { sc.Step(`^(?i)ClusterObjectSet "([^"]+)" reports aggregate object counts: total=(\d+), present=(\d+), synced=(\d+), ready=(\d+)$`, ClusterObjectSetHasAggregateObjectCounts) sc.Step(`^(?i)ClusterObjectSet "([^"]+)" contains annotation "([^"]+)" with value$`, ClusterObjectSetHasAnnotationWithValue) sc.Step(`^(?i)ClusterObjectSet "([^"]+)" has label "([^"]+)" with value "([^"]+)"$`, ClusterObjectSetHasLabelWithValue) + sc.Step(`^(?i)ClusterObjectSet "([^"]+)" has group "([^"]+)"$`, ClusterObjectSetHasGroup) + sc.Step(`^(?i)ClusterObjectSet "([^"]+)" phase objects use SSA manager "([^"]+)"$`, ClusterObjectSetObjectsUseSSAManager) + sc.Step(`^(?i)ClusterObjectSet "([^"]+)" referred secrets are remembered$`, RememberClusterObjectSetSecrets) + sc.Step(`^(?i)the remembered revision secrets are removed$`, RememberedRevisionSecretsRemoved) + sc.Step(`^(?i)resource "([^"]+)" UID is remembered$`, RememberResourceUID) + sc.Step(`^(?i)resource "([^"]+)" keeps its UID$`, ResourceKeepsUID) sc.Step(`^(?i)ClusterObjectSet "([^"]+)" phase objects are not found or not owned by the revision$`, ClusterObjectSetObjectsNotFoundOrNotOwned) sc.Step(`^(?i)ClusterObjectSet "([^"]+)" phase objects are managed in Kubernetes secrets$`, ClusterObjectSetPhaseObjectsManagedInSecrets) sc.Step(`^(?i)ClusterObjectSet "([^"]+)" referred secrets exist in "([^"]+)" namespace$`, ClusterObjectSetReferredSecretsExist) @@ -596,14 +603,24 @@ func ClusterExtensionOwnsClusterObjectSets(ctx context.Context, extName string, sc := scenarioCtx(ctx) extName = substituteScenarioVars(extName, sc) waitFor(ctx, func() bool { - out, err := k8sClient(ctx, "get", "clusterobjectsets", - "-l", fmt.Sprintf("olm.operatorframework.io/owner-name=%s", extName), - "-o", "jsonpath={.items[*].metadata.name}") + revisions, err := listClusterObjectSetsInGroup(ctx, extName) if err != nil { return false } - names := strings.Fields(strings.TrimSpace(out)) - return len(names) == expectedCount + if len(revisions) != expectedCount { + return false + } + ext, err := getResource("clusterextension", extName, "") + if err != nil { + return false + } + for _, rev := range revisions { + owner := metav1.GetControllerOf(&rev) + if owner == nil || owner.Kind != ocv1.ClusterExtensionKind || owner.Name != extName || owner.UID != ext.GetUID() { + return false + } + } + return true }) return nil } @@ -2394,21 +2411,13 @@ func resolveObjectRef(ref ocv1.ObjectSourceRef) (*unstructured.Unstructured, err // latestActiveRevisionForExtension returns the latest active revision for the extension called extName func latestActiveRevisionForExtension(extName string) (*ocv1.ClusterObjectSet, error) { - out, err := k8sClient(context.Background(), "get", "clusterobjectsets", "-l", fmt.Sprintf("olm.operatorframework.io/owner-name=%s", extName), "-o", "json") + revisions, err := listClusterObjectSetsInGroup(context.Background(), extName) if err != nil { return nil, fmt.Errorf("error listing revisions for extension '%s': %w", extName, err) } - if strings.TrimSpace(out) == "" { - return nil, fmt.Errorf("no revisions found for extension '%s'", extName) - } - var revisionList ocv1.ClusterObjectSetList - if err := json.Unmarshal([]byte(out), &revisionList); err != nil { - return nil, fmt.Errorf("error unmarshalling revisions for extension '%s': %w", extName, err) - } - var latest *ocv1.ClusterObjectSet - for i := range revisionList.Items { - rev := &revisionList.Items[i] + for i := range revisions { + rev := &revisions[i] if rev.Spec.LifecycleState != ocv1.ClusterObjectSetLifecycleStateActive { continue } From bddfaa9428185d63051cee7512b43158ed9f04ec Mon Sep 17 00:00:00 2001 From: Francesco Giudici Date: Tue, 13 Oct 2026 11:50:35 +0200 Subject: [PATCH 05/10] Allow selecting ClusterObjectSets by group Declare spec.group as a selectable field in the experimental COS CRD and regenerate the installation manifests. Document the kubectl field selector in place of filtering a full resource list with jq. Verify API-server filtering with an uncached envtest client, covering multiple revisions in one group, a different group, and no matches. Co-authored-by: Codex Signed-off-by: Francesco Giudici --- api/v1/clusterobjectset_types.go | 1 + api/v1/clusterobjectset_types_test.go | 45 +++++++++++++++++++ docs/draft/concepts/clusterobjectsets.md | 3 +- ...peratorframework.io_clusterobjectsets.yaml | 2 + manifests/experimental-e2e.yaml | 2 + manifests/experimental.yaml | 2 + 6 files changed, 53 insertions(+), 2 deletions(-) diff --git a/api/v1/clusterobjectset_types.go b/api/v1/clusterobjectset_types.go index 4a6548f265..ca830ac85e 100644 --- a/api/v1/clusterobjectset_types.go +++ b/api/v1/clusterobjectset_types.go @@ -619,6 +619,7 @@ type ObjectCounts struct { // +kubebuilder:object:root=true // +kubebuilder:resource:scope=Cluster // +kubebuilder:subresource:status +// +kubebuilder:selectablefield:JSONPath=`.spec.group` // +kubebuilder:printcolumn:name="Group",type=string,JSONPath=`.spec.group` // +kubebuilder:printcolumn:name="Ready",type=string,JSONPath=`.status.conditions[?(@.type=='Ready')].status` // +kubebuilder:printcolumn:name=Age,type=date,JSONPath=`.metadata.creationTimestamp` diff --git a/api/v1/clusterobjectset_types_test.go b/api/v1/clusterobjectset_types_test.go index e00a1a2e85..0a864cec48 100644 --- a/api/v1/clusterobjectset_types_test.go +++ b/api/v1/clusterobjectset_types_test.go @@ -11,6 +11,7 @@ import ( 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" ) func TestClusterObjectSetImmutability(t *testing.T) { @@ -471,6 +472,50 @@ func TestClusterObjectSetGroupValidation(t *testing.T) { } } +func TestClusterObjectSetGroupFieldSelector(t *testing.T) { + // Use an uncached client so filtering is performed by the API server. + c := newClient(t) + for _, fixture := range []struct { + name string + group string + revision int64 + }{ + {name: "field-selector-a-1", group: "field-selector-a", revision: 1}, + {name: "field-selector-a-2", group: "field-selector-a", revision: 2}, + {name: "field-selector-b-1", group: "field-selector-b", revision: 1}, + } { + cos := &ClusterObjectSet{ + ObjectMeta: metav1.ObjectMeta{Name: fixture.name}, + Spec: ClusterObjectSetSpec{ + Group: fixture.group, + Revision: fixture.revision, + LifecycleState: ClusterObjectSetLifecycleStateActive, + CollisionProtection: CollisionProtectionPrevent, + }, + } + require.NoError(t, c.Create(t.Context(), cos)) + } + + for _, tc := range []struct { + group string + want []string + }{ + {group: "field-selector-a", want: []string{"field-selector-a-1", "field-selector-a-2"}}, + {group: "field-selector-b", want: []string{"field-selector-b-1"}}, + {group: "field-selector-missing"}, + } { + t.Run(tc.group, func(t *testing.T) { + var list ClusterObjectSetList + require.NoError(t, c.List(t.Context(), &list, client.MatchingFields{"spec.group": tc.group})) + names := make([]string, 0, len(list.Items)) + for _, cos := range list.Items { + names = append(names, cos.Name) + } + require.ElementsMatch(t, tc.want, names) + }) + } +} + func configMap() unstructured.Unstructured { return unstructured.Unstructured{ Object: map[string]interface{}{ diff --git a/docs/draft/concepts/clusterobjectsets.md b/docs/draft/concepts/clusterobjectsets.md index c9acd17ff0..35f2cfacaf 100644 --- a/docs/draft/concepts/clusterobjectsets.md +++ b/docs/draft/concepts/clusterobjectsets.md @@ -442,8 +442,7 @@ Multiple objects can reference different keys within the same Secret, allowing e kubectl get clusterobjectsets # List revisions in a specific extension's group -kubectl get clusterobjectsets -o json | \ - jq -r '.items[] | select(.spec.group == "my-extension") | .metadata.name' +kubectl get clusterobjectsets --field-selector spec.group=my-extension # View full details for a specific revision kubectl get clusterobjectset -o yaml diff --git a/helm/olmv1/base/object-controller/crd/experimental/olm.operatorframework.io_clusterobjectsets.yaml b/helm/olmv1/base/object-controller/crd/experimental/olm.operatorframework.io_clusterobjectsets.yaml index 36ee6cb499..2a670efae6 100644 --- a/helm/olmv1/base/object-controller/crd/experimental/olm.operatorframework.io_clusterobjectsets.yaml +++ b/helm/olmv1/base/object-controller/crd/experimental/olm.operatorframework.io_clusterobjectsets.yaml @@ -777,6 +777,8 @@ spec: required: - spec type: object + selectableFields: + - jsonPath: .spec.group served: true storage: true subresources: diff --git a/manifests/experimental-e2e.yaml b/manifests/experimental-e2e.yaml index 01d9d4eb76..528bb4ecc9 100644 --- a/manifests/experimental-e2e.yaml +++ b/manifests/experimental-e2e.yaml @@ -2188,6 +2188,8 @@ spec: required: - spec type: object + selectableFields: + - jsonPath: .spec.group served: true storage: true subresources: diff --git a/manifests/experimental.yaml b/manifests/experimental.yaml index 7a5e03d745..74034abb7a 100644 --- a/manifests/experimental.yaml +++ b/manifests/experimental.yaml @@ -2149,6 +2149,8 @@ spec: required: - spec type: object + selectableFields: + - jsonPath: .spec.group served: true storage: true subresources: From 69e33d8dc9c35ab48b8233210b416c983f7af793 Mon Sep 17 00:00:00 2001 From: Francesco Giudici Date: Tue, 13 Oct 2026 13:21:08 +0200 Subject: [PATCH 06/10] Decouple revision lookup from ClusterExtension Rename ListForClusterExtension to ListForGroup and accept a group string. Have CE callers pass the extension name explicitly, preserving group selection, revision ordering, error propagation, and lifecycle handling. Use a group distinct from the owner name in the existing lookup test to verify that the helper depends only on its group argument. Co-authored-by: Codex Signed-off-by: Francesco Giudici --- .../operator-controller/applier/boxcutter.go | 4 ++-- .../controllers/boxcutter_reconcile_steps.go | 2 +- .../revisions/clusterobjectsets.go | 10 ++++----- .../revisions/clusterobjectsets_test.go | 21 ++++++++++--------- 4 files changed, 19 insertions(+), 18 deletions(-) diff --git a/internal/operator-controller/applier/boxcutter.go b/internal/operator-controller/applier/boxcutter.go index 77381dc49b..4b45ebcff8 100644 --- a/internal/operator-controller/applier/boxcutter.go +++ b/internal/operator-controller/applier/boxcutter.go @@ -282,7 +282,7 @@ func (m *BoxcutterStorageMigrator) Migrate(ctx context.Context, ext *ocv1.Cluste if ext.Spec.Namespace == "" { return nil } - existingRevisions, err := revisions.ListForClusterExtension(ctx, m.Client, ext) + existingRevisions, err := revisions.ListForGroup(ctx, m.Client, ext.Name) if err != nil { return fmt.Errorf("listing ClusterObjectSets before attempting migration: %w", err) } @@ -466,7 +466,7 @@ func (bc *Boxcutter) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.Clust // 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.ListForClusterExtension(ctx, bc.Client, ext) + existingRevisions, err := revisions.ListForGroup(ctx, bc.Client, ext.Name) if err != nil { return false, "", fmt.Errorf("listing revisions: %w", err) } diff --git a/internal/operator-controller/controllers/boxcutter_reconcile_steps.go b/internal/operator-controller/controllers/boxcutter_reconcile_steps.go index eab11f5cef..466eb51a3f 100644 --- a/internal/operator-controller/controllers/boxcutter_reconcile_steps.go +++ b/internal/operator-controller/controllers/boxcutter_reconcile_steps.go @@ -39,7 +39,7 @@ type BoxcutterRevisionStatesGetter struct { } func (d *BoxcutterRevisionStatesGetter) GetRevisionStates(ctx context.Context, ext *ocv1.ClusterExtension) (*RevisionStates, error) { - existingRevisions, err := revisions.ListForClusterExtension(ctx, d.Reader, ext) + existingRevisions, err := revisions.ListForGroup(ctx, d.Reader, ext.Name) if err != nil { return nil, fmt.Errorf("listing revisions: %w", err) } diff --git a/internal/operator-controller/revisions/clusterobjectsets.go b/internal/operator-controller/revisions/clusterobjectsets.go index ae9b38feca..04599f82de 100644 --- a/internal/operator-controller/revisions/clusterobjectsets.go +++ b/internal/operator-controller/revisions/clusterobjectsets.go @@ -26,12 +26,12 @@ import ( ocv1 "github.com/operator-framework/operator-controller/api/v1" ) -// ListForClusterExtension returns all COS revisions in the extension's group, oldest first. -// CE-generated revisions use the extension name as their group. Owner references -// do not affect revision association. Lifecycle filtering is left to callers. -func ListForClusterExtension(ctx context.Context, reader client.Reader, ext *ocv1.ClusterExtension) ([]ocv1.ClusterObjectSet, error) { +// ListForGroup returns all COS revisions in the group, oldest first. +// Owner references do not affect revision association. Lifecycle filtering is +// left to callers. +func ListForGroup(ctx context.Context, reader client.Reader, group string) ([]ocv1.ClusterObjectSet, error) { list := &ocv1.ClusterObjectSetList{} - if err := reader.List(ctx, list, client.MatchingFields{".spec.group": ext.Name}); err != nil { + if err := reader.List(ctx, list, client.MatchingFields{".spec.group": group}); err != nil { return nil, err } slices.SortFunc(list.Items, func(a, b ocv1.ClusterObjectSet) int { diff --git a/internal/operator-controller/revisions/clusterobjectsets_test.go b/internal/operator-controller/revisions/clusterobjectsets_test.go index 208f1d92dd..670ac30d7b 100644 --- a/internal/operator-controller/revisions/clusterobjectsets_test.go +++ b/internal/operator-controller/revisions/clusterobjectsets_test.go @@ -33,9 +33,10 @@ import ( "github.com/operator-framework/operator-controller/test" ) -func TestListForClusterExtension(t *testing.T) { +func TestListForGroup(t *testing.T) { scheme := runtime.NewScheme() require.NoError(t, ocv1.AddToScheme(scheme)) + const group = "test-group" ext := &ocv1.ClusterExtension{ObjectMeta: metav1.ObjectMeta{Name: "test-ext", UID: "current-uid"}} owner := *metav1.NewControllerRef(ext, ocv1.GroupVersion.WithKind(ocv1.ClusterExtensionKind)) differentName, differentKind, differentIdentity := owner, owner, owner @@ -54,15 +55,15 @@ func TestListForClusterExtension(t *testing.T) { owners []metav1.OwnerReference revision int64 }{ - {name: "matching-owner", group: ext.Name, owners: []metav1.OwnerReference{owner}, revision: 2}, - {name: "oldest", group: ext.Name, owners: []metav1.OwnerReference{differentIdentity}, revision: 1}, + {name: "matching-owner", group: group, owners: []metav1.OwnerReference{owner}, revision: 2}, + {name: "oldest", group: group, owners: []metav1.OwnerReference{differentIdentity}, revision: 1}, {name: "other-group", group: "other-group", owners: []metav1.OwnerReference{owner}, revision: 99}, - {name: "other-name", group: ext.Name, owners: []metav1.OwnerReference{differentName}, revision: 100}, - {name: "other-kind", group: ext.Name, owners: []metav1.OwnerReference{differentKind}, revision: 101}, - {name: "ownerless", group: ext.Name, revision: 102}, - {name: "non-controller", group: ext.Name, owners: []metav1.OwnerReference{nonController}, revision: 103}, - {name: "unspecified-controller", group: ext.Name, owners: []metav1.OwnerReference{unspecifiedController}, revision: 104}, - {name: "foreign-controller-with-ce-owner", group: ext.Name, owners: []metav1.OwnerReference{differentKind, nonController}, revision: 105}, + {name: "other-name", group: group, owners: []metav1.OwnerReference{differentName}, revision: 100}, + {name: "other-kind", group: group, owners: []metav1.OwnerReference{differentKind}, revision: 101}, + {name: "ownerless", group: group, revision: 102}, + {name: "non-controller", group: group, owners: []metav1.OwnerReference{nonController}, revision: 103}, + {name: "unspecified-controller", group: group, owners: []metav1.OwnerReference{unspecifiedController}, revision: 104}, + {name: "foreign-controller-with-ce-owner", group: group, owners: []metav1.OwnerReference{differentKind, nonController}, revision: 105}, } { objects = append(objects, &ocv1.ClusterObjectSet{ ObjectMeta: metav1.ObjectMeta{Name: tc.name, OwnerReferences: tc.owners}, @@ -75,7 +76,7 @@ func TestListForClusterExtension(t *testing.T) { } cl := test.WithIndexes(t, fake.NewClientBuilder().WithScheme(scheme), coscontrollers.SetupIndexes). WithObjects(objects...).Build() - got, err := revisions.ListForClusterExtension(t.Context(), cl, ext) + got, err := revisions.ListForGroup(t.Context(), cl, group) require.NoError(t, err) names := make([]string, 0, len(got)) for _, cos := range got { From 7b7571a6971e7309c0c2933ce193b7a2a09eb838 Mon Sep 17 00:00:00 2001 From: Francesco Giudici Date: Wed, 14 Oct 2026 12:24:05 +0200 Subject: [PATCH 07/10] Address review feedback for COS revision groups Show revision numbers in ClusterObjectSet listings and mark the future move of archival decisions to ClusterObjectDeployment. Clarify group handoff scenarios and UID preservation, rename the owner placeholders, and cover handoffs between two ownerless revisions. Remove obsolete owner-label lookup fixtures and use API-server group selection in the E2E helper. Co-authored-by: Codex Signed-off-by: Francesco Giudici --- api/v1/clusterobjectset_types.go | 1 + ...peratorframework.io_clusterobjectsets.yaml | 3 ++ .../clusterobjectset_controller.go | 4 +- ...usterobjectset_controller_internal_test.go | 52 ++++++++++--------- manifests/experimental-e2e.yaml | 3 ++ manifests/experimental.yaml | 3 ++ test/e2e/features/group.feature | 30 ++++++++--- test/e2e/steps/group_steps.go | 12 ++--- 8 files changed, 66 insertions(+), 42 deletions(-) diff --git a/api/v1/clusterobjectset_types.go b/api/v1/clusterobjectset_types.go index ca830ac85e..039e7e6ca0 100644 --- a/api/v1/clusterobjectset_types.go +++ b/api/v1/clusterobjectset_types.go @@ -621,6 +621,7 @@ type ObjectCounts struct { // +kubebuilder:subresource:status // +kubebuilder:selectablefield:JSONPath=`.spec.group` // +kubebuilder:printcolumn:name="Group",type=string,JSONPath=`.spec.group` +// +kubebuilder:printcolumn:name="Revision",type=integer,JSONPath=`.spec.revision` // +kubebuilder:printcolumn:name="Ready",type=string,JSONPath=`.status.conditions[?(@.type=='Ready')].status` // +kubebuilder:printcolumn:name=Age,type=date,JSONPath=`.metadata.creationTimestamp` diff --git a/helm/olmv1/base/object-controller/crd/experimental/olm.operatorframework.io_clusterobjectsets.yaml b/helm/olmv1/base/object-controller/crd/experimental/olm.operatorframework.io_clusterobjectsets.yaml index 2a670efae6..08cb039bab 100644 --- a/helm/olmv1/base/object-controller/crd/experimental/olm.operatorframework.io_clusterobjectsets.yaml +++ b/helm/olmv1/base/object-controller/crd/experimental/olm.operatorframework.io_clusterobjectsets.yaml @@ -19,6 +19,9 @@ spec: - jsonPath: .spec.group name: Group type: string + - jsonPath: .spec.revision + name: Revision + type: integer - jsonPath: .status.conditions[?(@.type=='Ready')].status name: Ready type: string diff --git a/internal/object-controller/controllers/clusterobjectset_controller.go b/internal/object-controller/controllers/clusterobjectset_controller.go index 570bd09cd4..6c3690606d 100644 --- a/internal/object-controller/controllers/clusterobjectset_controller.go +++ b/internal/object-controller/controllers/clusterobjectset_controller.go @@ -321,7 +321,9 @@ func (c *ClusterObjectSetReconciler) reconcile(ctx context.Context, cos *ocv1.Cl //nolint:nestif if rres.IsComplete() && !superseded { - // Archive previous revisions + // Archive previous revisions. + // TODO(COD): Move the decision to archive previous revisions to + // ClusterObjectDeployment reconciliation when that controller is introduced. previous, err := c.listPreviousRevisions(ctx, cos) if err != nil { return ctrl.Result{}, fmt.Errorf("listing previous revisions: %v", err) diff --git a/internal/object-controller/controllers/clusterobjectset_controller_internal_test.go b/internal/object-controller/controllers/clusterobjectset_controller_internal_test.go index 638ac89d10..90c4358a95 100644 --- a/internal/object-controller/controllers/clusterobjectset_controller_internal_test.go +++ b/internal/object-controller/controllers/clusterobjectset_controller_internal_test.go @@ -23,7 +23,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" ocv1 "github.com/operator-framework/operator-controller/api/v1" - "github.com/operator-framework/operator-controller/internal/shared/labels" "github.com/operator-framework/operator-controller/test" ) @@ -105,20 +104,6 @@ func Test_ClusterObjectSetReconciler_listSiblingRevisions(t *testing.T) { currentRev: "rev-3", expectedRevs: []string{"rev-1"}, }, - { - name: "should include revisions when owner label is missing", - existingObjs: func() []client.Object { - ext := newTestClusterExtensionInternal() - rev1 := newTestClusterObjectSetInternal(t, "rev-1") - rev2 := newTestClusterObjectSetInternal(t, "rev-2") - delete(rev2.Labels, labels.OwnerNameKey) - require.NoError(t, controllerutil.SetControllerReference(ext, rev1, testScheme)) - require.NoError(t, controllerutil.SetControllerReference(ext, rev2, testScheme)) - return []client.Object{ext, rev1, rev2} - }, - currentRev: "rev-1", - expectedRevs: []string{"rev-2"}, - }, } { t.Run(tc.name, func(t *testing.T) { result := callRevisionLister(t, testScheme, tc.existingObjs(), tc.currentRev, @@ -201,17 +186,37 @@ func Test_ClusterObjectSetReconciler_OwnerChangesPreserveRevisionChain(t *testin r := &ClusterObjectSetReconciler{Client: cl} for _, tc := range []struct { - name string - owners []metav1.OwnerReference + name string + previousOwners []metav1.OwnerReference + currentOwners []metav1.OwnerReference }{ - {name: "same owner", owners: []metav1.OwnerReference{owner}}, - {name: "different owner", owners: []metav1.OwnerReference{foreignOwner}}, - {name: "owner removed"}, - {name: "owner added again", owners: []metav1.OwnerReference{owner}}, + { + name: "same owner", + previousOwners: []metav1.OwnerReference{owner}, + currentOwners: []metav1.OwnerReference{owner}, + }, + { + name: "different owner", + previousOwners: []metav1.OwnerReference{foreignOwner}, + currentOwners: []metav1.OwnerReference{owner}, + }, + { + name: "owner removed", + currentOwners: []metav1.OwnerReference{owner}, + }, + { + name: "owner added again", + previousOwners: []metav1.OwnerReference{owner}, + currentOwners: []metav1.OwnerReference{owner}, + }, + {name: "both revisions ownerless"}, } { t.Run(tc.name, func(t *testing.T) { + require.NoError(t, cl.Get(t.Context(), client.ObjectKeyFromObject(current), current)) + current.OwnerReferences = tc.currentOwners + require.NoError(t, cl.Update(t.Context(), current)) require.NoError(t, cl.Get(t.Context(), client.ObjectKeyFromObject(previous), previous)) - previous.OwnerReferences = tc.owners + previous.OwnerReferences = tc.previousOwners require.NoError(t, cl.Update(t.Context(), previous)) siblings, err := r.listSiblingRevisions(t.Context(), current) require.NoError(t, err) @@ -286,9 +291,6 @@ func newTestClusterObjectSetInternal(t *testing.T, name string) *ocv1.ClusterObj Name: name, UID: types.UID(name), Generation: int64(1), - Labels: map[string]string{ - labels.OwnerNameKey: "test-ext", - }, }, Spec: ocv1.ClusterObjectSetSpec{ Group: "test-ext", diff --git a/manifests/experimental-e2e.yaml b/manifests/experimental-e2e.yaml index 528bb4ecc9..8ef8b11983 100644 --- a/manifests/experimental-e2e.yaml +++ b/manifests/experimental-e2e.yaml @@ -1430,6 +1430,9 @@ spec: - jsonPath: .spec.group name: Group type: string + - jsonPath: .spec.revision + name: Revision + type: integer - jsonPath: .status.conditions[?(@.type=='Ready')].status name: Ready type: string diff --git a/manifests/experimental.yaml b/manifests/experimental.yaml index 74034abb7a..a887166aaa 100644 --- a/manifests/experimental.yaml +++ b/manifests/experimental.yaml @@ -1391,6 +1391,9 @@ spec: - jsonPath: .spec.group name: Group type: string + - jsonPath: .spec.revision + name: Revision + type: integer - jsonPath: .status.conditions[?(@.type=='Ready')].status name: Ready type: string diff --git a/test/e2e/features/group.feature b/test/e2e/features/group.feature index 2dc31a07e9..ceb3352923 100644 --- a/test/e2e/features/group.feature +++ b/test/e2e/features/group.feature @@ -1,12 +1,27 @@ @BoxcutterRuntime Feature: Track ClusterObjectSet revisions by group + As a ClusterObjectSet user, I want to group successive revisions so that a new + revision can update the Kubernetes objects managed by an earlier revision + without deleting and recreating them. This handoff should work even when the + ClusterObjectSets have different controller owners or no owner. I also want + collision protection between groups, and pruning a group's old revisions + should leave other groups' revisions untouched. + Background: Given OLM is available And an image registry is available And namespace "${TEST_NAMESPACE}" is available - Scenario Outline: Group handoff preserves objects across + Scenario Outline: Group handoff preserves objects when + Both ClusterObjectSets have the same group. Their controller owner references + point to different Namespaces, or one or both ClusterObjectSets have no owner + reference. These owner references must not determine group membership. + + The new revision must update the existing ConfigMap without deleting and + recreating it. The UID saved before handoff must still match after the new + revision is ready and the previous revision is archived. + Given resource is applied """ apiVersion: v1 @@ -14,7 +29,7 @@ Feature: Track ClusterObjectSet revisions by group metadata: name: ${TEST_NAMESPACE}-other """ - And ClusterObjectSet with Namespace owner "" is applied + And ClusterObjectSet with Namespace owner "" is applied """ apiVersion: olm.operatorframework.io/v1 kind: ClusterObjectSet @@ -40,7 +55,7 @@ Feature: Track ClusterObjectSet revisions by group Then ClusterObjectSet "${NAME}-1" reports Ready as True with Reason AllObjectsReady And ClusterObjectSet "${NAME}-1" phase objects use SSA manager "cos-group/${NAME}" And resource "configmap/${NAME}-payload" UID is remembered - When ClusterObjectSet with Namespace owner "" is applied + When ClusterObjectSet with Namespace owner "" is applied """ apiVersion: olm.operatorframework.io/v1 kind: ClusterObjectSet @@ -76,10 +91,11 @@ Feature: Track ClusterObjectSet revisions by group # An empty owner creates an ownerless COS. Namespace owners are real, # cluster-scoped objects, independent of CE and future COD controllers. Examples: - | ownership | lower-owner | higher-owner | - | different controllers | ${TEST_NAMESPACE} | ${TEST_NAMESPACE}-other | - | owned to ownerless | ${TEST_NAMESPACE} | | - | ownerless to owned | | ${TEST_NAMESPACE} | + | revision-ownership | lower-revision-owner | higher-revision-owner | + | revisions have different controller owners | ${TEST_NAMESPACE} | ${TEST_NAMESPACE}-other | + | an owned revision is followed by an ownerless revision | ${TEST_NAMESPACE} | | + | an ownerless revision is followed by an owned revision | | ${TEST_NAMESPACE} | + | both revisions have no controller owner | | | Scenario: Different groups collide even with the same controller owner Given ClusterObjectSet with Namespace owner "${TEST_NAMESPACE}" is applied diff --git a/test/e2e/steps/group_steps.go b/test/e2e/steps/group_steps.go index 204fd42dee..685c4914b4 100644 --- a/test/e2e/steps/group_steps.go +++ b/test/e2e/steps/group_steps.go @@ -84,9 +84,9 @@ func ResourceKeepsUID(ctx context.Context, resourceName string) error { return nil } -// A local cache index does not make spec.group selectable through kubectl. +// listClusterObjectSetsInGroup selects revisions by group on the API server. func listClusterObjectSetsInGroup(ctx context.Context, group string) ([]ocv1.ClusterObjectSet, error) { - out, err := k8sClient(ctx, "get", "clusterobjectsets", "-o", "json") + out, err := k8sClient(ctx, "get", "clusterobjectsets", "--field-selector", "spec.group="+group, "-o", "json") if err != nil { return nil, err } @@ -94,13 +94,7 @@ func listClusterObjectSetsInGroup(ctx context.Context, group string) ([]ocv1.Clu if err := json.Unmarshal([]byte(out), &list); err != nil { return nil, err } - var revisions []ocv1.ClusterObjectSet - for _, rev := range list.Items { - if rev.Spec.Group == group { - revisions = append(revisions, rev) - } - } - return revisions, nil + return list.Items, nil } func ClusterObjectSetHasGroup(ctx context.Context, revisionName, group string) error { From 6ec1edad457a95e95e6f42560582100bb558139f Mon Sep 17 00:00:00 2001 From: Francesco Giudici Date: Fri, 16 Oct 2026 09:33:11 +0200 Subject: [PATCH 08/10] Mark future archival documentation update for COD Add the TODO beside the ClusterObjectSet API description so the archival wording can be updated when decisions move to ClusterObjectDeployment. Keep the note separate from godoc to avoid publishing internal TODOs. Co-authored-by: Codex Signed-off-by: Francesco Giudici --- api/v1/clusterobjectset_types.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/api/v1/clusterobjectset_types.go b/api/v1/clusterobjectset_types.go index 039e7e6ca0..672f29f031 100644 --- a/api/v1/clusterobjectset_types.go +++ b/api/v1/clusterobjectset_types.go @@ -614,6 +614,9 @@ type ObjectCounts struct { Ready int64 `json:"ready,omitempty"` } +// TODO(COD): Update the archival description below when archival decisions move +// to ClusterObjectDeployment reconciliation. + // +genclient // +genclient:nonNamespaced // +kubebuilder:object:root=true From a13cd6076262a2fff2b20b0632619462e65a5d4a Mon Sep 17 00:00:00 2001 From: Francesco Giudici Date: Fri, 16 Oct 2026 13:06:18 +0200 Subject: [PATCH 09/10] Address review feedback for COS group tracking Make validation fixtures explicit, clarify group-based lookup and archival coverage, and separate the SSA factory integration scenarios. Move the shared fake-client index helper into internal/testutil. Require Active when selecting revisions, register the Helm tracking cache alongside controller setup, and generalize index setup errors. Give cross-group numbering and retention dedicated Apply test cases. Remove the unrelated fixture from shared setup and drop redundant fallback coverage; the fallback logic itself is unchanged. Remove the unenforced consecutive revision requirement and regenerate the affected API descriptions. Mark the future COD documentation update. Co-authored-by: Codex Signed-off-by: Francesco Giudici --- api/v1/clusterobjectset_types.go | 1 - api/v1/clusterobjectset_types_test.go | 35 +++- .../api/v1/clusterobjectsetspec.go | 1 - cmd/object-controller/main.go | 2 +- cmd/operator-controller/main.go | 5 +- docs/draft/concepts/clusterobjectsets.md | 2 + ...peratorframework.io_clusterobjectsets.yaml | 1 - .../clusterobjectset_controller.go | 2 +- ...usterobjectset_controller_internal_test.go | 40 ++-- .../clusterobjectset_controller_test.go | 37 ++-- .../controllers/indexes_test.go | 4 +- .../controllers/resolve_ref_test.go | 12 +- .../revision_engine_factory_test.go | 196 ++++++++++-------- .../applier/boxcutter_test.go | 140 +++++++++---- ...boxcutter_reconcile_steps_internal_test.go | 4 +- .../clusterextension_controller.go | 4 + .../revisions/clusterobjectsets_test.go | 4 +- {test => internal/testutil}/indexes.go | 2 +- manifests/experimental-e2e.yaml | 1 - manifests/experimental.yaml | 1 - 20 files changed, 292 insertions(+), 202 deletions(-) rename {test => internal/testutil}/indexes.go (97%) diff --git a/api/v1/clusterobjectset_types.go b/api/v1/clusterobjectset_types.go index 672f29f031..c5883036af 100644 --- a/api/v1/clusterobjectset_types.go +++ b/api/v1/clusterobjectset_types.go @@ -77,7 +77,6 @@ type ClusterObjectSetSpec struct { // // The revision field must be a positive integer. // Each ClusterObjectSet in the same revision sequence must have a unique revision number. - // The revision number must always be the previous revision number plus one, or 1 for the first revision. // // +required // +kubebuilder:validation:Minimum:=1 diff --git a/api/v1/clusterobjectset_types_test.go b/api/v1/clusterobjectset_types_test.go index 0a864cec48..46086ee4d8 100644 --- a/api/v1/clusterobjectset_types_test.go +++ b/api/v1/clusterobjectset_types_test.go @@ -168,13 +168,13 @@ func TestClusterObjectSetImmutability(t *testing.T) { func TestClusterObjectSetValidity(t *testing.T) { c := newClient(t) ctx := context.Background() - i := 0 for name, tc := range map[string]struct { spec ClusterObjectSetSpec valid bool }{ "revision cannot be negative": { spec: ClusterObjectSetSpec{ + Group: "validity-negative-revision", LifecycleState: ClusterObjectSetLifecycleStateActive, Revision: -1, }, @@ -182,12 +182,14 @@ func TestClusterObjectSetValidity(t *testing.T) { }, "revision cannot be zero": { spec: ClusterObjectSetSpec{ + Group: "validity-zero-revision", LifecycleState: ClusterObjectSetLifecycleStateActive, }, valid: false, }, "revision must be positive": { spec: ClusterObjectSetSpec{ + Group: "validity-positive-revision", LifecycleState: ClusterObjectSetLifecycleStateActive, Revision: 1, CollisionProtection: CollisionProtectionPrevent, @@ -196,12 +198,14 @@ func TestClusterObjectSetValidity(t *testing.T) { }, "lifecycleState must be set": { spec: ClusterObjectSetSpec{ + Group: "validity-missing-lifecycle", Revision: 1, }, valid: false, }, "phases must have no more than 20 phases": { spec: ClusterObjectSetSpec{ + Group: "validity-too-many-phases", LifecycleState: ClusterObjectSetLifecycleStateActive, Revision: 1, Phases: make([]ClusterObjectSetPhase, 21), @@ -210,6 +214,7 @@ func TestClusterObjectSetValidity(t *testing.T) { }, "phases entries must have no more than 50 objects": { spec: ClusterObjectSetSpec{ + Group: "validity-too-many-objects", LifecycleState: ClusterObjectSetLifecycleStateActive, Revision: 1, Phases: []ClusterObjectSetPhase{ @@ -223,6 +228,7 @@ func TestClusterObjectSetValidity(t *testing.T) { }, "phases entry names cannot be empty": { spec: ClusterObjectSetSpec{ + Group: "validity-empty-phase-name", LifecycleState: ClusterObjectSetLifecycleStateActive, Revision: 1, Phases: []ClusterObjectSetPhase{ @@ -235,6 +241,7 @@ func TestClusterObjectSetValidity(t *testing.T) { }, "phases entry names cannot start with symbols": { spec: ClusterObjectSetSpec{ + Group: "validity-symbol-phase-name", LifecycleState: ClusterObjectSetLifecycleStateActive, Revision: 1, Phases: []ClusterObjectSetPhase{ @@ -247,6 +254,7 @@ func TestClusterObjectSetValidity(t *testing.T) { }, "phases entry names cannot start with numeric characters": { spec: ClusterObjectSetSpec{ + Group: "validity-numeric-phase-name", LifecycleState: ClusterObjectSetLifecycleStateActive, Revision: 1, Phases: []ClusterObjectSetPhase{ @@ -259,6 +267,7 @@ func TestClusterObjectSetValidity(t *testing.T) { }, "spec collisionProtection accepts Prevent": { spec: ClusterObjectSetSpec{ + Group: "validity-prevent-collisions", LifecycleState: ClusterObjectSetLifecycleStateActive, Revision: 1, CollisionProtection: CollisionProtectionPrevent, @@ -267,6 +276,7 @@ func TestClusterObjectSetValidity(t *testing.T) { }, "spec collisionProtection accepts IfNoController": { spec: ClusterObjectSetSpec{ + Group: "validity-if-no-controller", LifecycleState: ClusterObjectSetLifecycleStateActive, Revision: 1, CollisionProtection: CollisionProtectionIfNoController, @@ -275,6 +285,7 @@ func TestClusterObjectSetValidity(t *testing.T) { }, "spec collisionProtection accepts None": { spec: ClusterObjectSetSpec{ + Group: "validity-allow-collisions", LifecycleState: ClusterObjectSetLifecycleStateActive, Revision: 1, CollisionProtection: CollisionProtectionNone, @@ -283,6 +294,7 @@ func TestClusterObjectSetValidity(t *testing.T) { }, "spec collisionProtection is required": { spec: ClusterObjectSetSpec{ + Group: "validity-required-collision-protection", LifecycleState: ClusterObjectSetLifecycleStateActive, Revision: 1, }, @@ -290,6 +302,7 @@ func TestClusterObjectSetValidity(t *testing.T) { }, "spec collisionProtection rejects invalid values": { spec: ClusterObjectSetSpec{ + Group: "validity-invalid-collision-protection", LifecycleState: ClusterObjectSetLifecycleStateActive, Revision: 1, CollisionProtection: CollisionProtection("Invalid"), @@ -298,6 +311,7 @@ func TestClusterObjectSetValidity(t *testing.T) { }, "spec collisionProtection must be set": { spec: ClusterObjectSetSpec{ + Group: "validity-unset-collision-protection", LifecycleState: ClusterObjectSetLifecycleStateActive, Revision: 1, }, @@ -305,6 +319,7 @@ func TestClusterObjectSetValidity(t *testing.T) { }, "object collisionProtection is optional": { spec: ClusterObjectSetSpec{ + Group: "validity-optional-object-collision-protection", LifecycleState: ClusterObjectSetLifecycleStateActive, Revision: 1, CollisionProtection: CollisionProtectionPrevent, @@ -323,6 +338,7 @@ func TestClusterObjectSetValidity(t *testing.T) { }, "object with inline object is valid": { spec: ClusterObjectSetSpec{ + Group: "validity-inline-object", LifecycleState: ClusterObjectSetLifecycleStateActive, Revision: 1, CollisionProtection: CollisionProtectionPrevent, @@ -341,6 +357,7 @@ func TestClusterObjectSetValidity(t *testing.T) { }, "object with ref is valid": { spec: ClusterObjectSetSpec{ + Group: "validity-ref-object", LifecycleState: ClusterObjectSetLifecycleStateActive, Revision: 1, CollisionProtection: CollisionProtectionPrevent, @@ -359,6 +376,7 @@ func TestClusterObjectSetValidity(t *testing.T) { }, "object with both object and ref is invalid": { spec: ClusterObjectSetSpec{ + Group: "validity-object-and-ref", LifecycleState: ClusterObjectSetLifecycleStateActive, Revision: 1, CollisionProtection: CollisionProtectionPrevent, @@ -378,6 +396,7 @@ func TestClusterObjectSetValidity(t *testing.T) { }, "object with neither object nor ref is invalid": { spec: ClusterObjectSetSpec{ + Group: "validity-object-and-ref-missing", LifecycleState: ClusterObjectSetLifecycleStateActive, Revision: 1, CollisionProtection: CollisionProtectionPrevent, @@ -396,12 +415,10 @@ func TestClusterObjectSetValidity(t *testing.T) { t.Run(name, func(t *testing.T) { cos := &ClusterObjectSet{ ObjectMeta: metav1.ObjectMeta{ - Name: fmt.Sprintf("bar%d", i), + Name: fmt.Sprintf("%s-%d", tc.spec.Group, tc.spec.Revision), }, Spec: tc.spec, } - cos.Spec.Group = "test-group" - i = i + 1 err := c.Create(ctx, cos) if tc.valid && err != nil { t.Fatal("expected create to succeed, but got:", err) @@ -419,7 +436,7 @@ func TestClusterObjectSetSpecValidation(t *testing.T) { cos := &unstructured.Unstructured{Object: map[string]any{ "apiVersion": GroupVersion.String(), "kind": ClusterObjectSetKind, - "metadata": map[string]any{"generateName": "spec-validation-"}, + "metadata": map[string]any{"name": "spec-validation-1"}, }} err := c.Create(t.Context(), cos) require.True(t, errors.IsInvalid(err), "%v", err) @@ -429,7 +446,7 @@ func TestClusterObjectSetSpecValidation(t *testing.T) { func TestClusterObjectSetGroupValidation(t *testing.T) { c := newClient(t) - for _, tc := range []struct { + for i, tc := range []struct { name string group *string valid bool @@ -448,10 +465,14 @@ func TestClusterObjectSetGroupValidation(t *testing.T) { {name: "dot", group: ptr.To("my.group"), valid: false}, } { t.Run(tc.name, func(t *testing.T) { + objectName := fmt.Sprintf("group-validation-%d", i+1) + if tc.valid { + objectName = fmt.Sprintf("%s-1", *tc.group) + } cos := &unstructured.Unstructured{Object: map[string]any{ "apiVersion": GroupVersion.String(), "kind": ClusterObjectSetKind, - "metadata": map[string]any{"generateName": "group-validation-"}, + "metadata": map[string]any{"name": objectName}, }} spec := map[string]any{ "revision": int64(1), "lifecycleState": string(ClusterObjectSetLifecycleStateActive), diff --git a/applyconfigurations/api/v1/clusterobjectsetspec.go b/applyconfigurations/api/v1/clusterobjectsetspec.go index 4e12c1bbc8..241a21b425 100644 --- a/applyconfigurations/api/v1/clusterobjectsetspec.go +++ b/applyconfigurations/api/v1/clusterobjectsetspec.go @@ -50,7 +50,6 @@ type ClusterObjectSetSpecApplyConfiguration struct { // // The revision field must be a positive integer. // Each ClusterObjectSet in the same revision sequence must have a unique revision number. - // The revision number must always be the previous revision number plus one, or 1 for the first revision. Revision *int64 `json:"revision,omitempty"` // phases is an optional, immutable list of phases that group objects to be applied together. // diff --git a/cmd/object-controller/main.go b/cmd/object-controller/main.go index f6616d6cbf..9dcc9e50bd 100644 --- a/cmd/object-controller/main.go +++ b/cmd/object-controller/main.go @@ -183,7 +183,7 @@ func newManager(cfg *config, restConfig *rest.Config) (manager.Manager, error) { return nil, fmt.Errorf("creating discovery client: %w", err) } if err := controllers.SetupIndexes(context.Background(), mgr.GetFieldIndexer()); err != nil { - return nil, fmt.Errorf("indexing ClusterObjectSet group: %w", err) + return nil, fmt.Errorf("setting up ClusterObjectSet indexes: %w", err) } factory, err := controllers.NewDefaultRevisionEngineFactory( mgr.GetScheme(), trackingCache, memory.NewMemCacheClient(discoveryClient), diff --git a/cmd/operator-controller/main.go b/cmd/operator-controller/main.go index 28547cec4d..0192d06579 100644 --- a/cmd/operator-controller/main.go +++ b/cmd/operator-controller/main.go @@ -421,7 +421,7 @@ func run() error { // direct ClusterExtension-to-COS queries. if features.OperatorControllerFeatureGate.Enabled(features.BoxcutterRuntime) { if err := clusterobjctrl.SetupIndexes(context.Background(), mgr.GetFieldIndexer()); err != nil { - return fmt.Errorf("indexing ClusterObjectSet group: %w", err) + return fmt.Errorf("setting up ClusterObjectSet indexes: %w", err) } } @@ -471,9 +471,6 @@ func run() error { if err != nil { return fmt.Errorf("unable to create tracking cache: %w", err) } - if err := mgr.Add(trackingCache); err != nil { - return fmt.Errorf("unable to add tracking cache to manager: %w", err) - } } ceReconciler := &controllers.ClusterExtensionReconciler{ diff --git a/docs/draft/concepts/clusterobjectsets.md b/docs/draft/concepts/clusterobjectsets.md index 35f2cfacaf..1401e4c4aa 100644 --- a/docs/draft/concepts/clusterobjectsets.md +++ b/docs/draft/concepts/clusterobjectsets.md @@ -17,6 +17,8 @@ Each ClusterObjectSet has: - A **collision protection** strategy — immutable, controlling how pre-existing objects are handled - **Status conditions** — reporting rollout progress, availability, and success + ClusterObjectSets can be used by any controller or system that needs to manage the rollout of a set of Kubernetes resources in a controlled, phased manner. Within OLM, the operator-controller uses ClusterObjectSets as the mechanism to deploy and upgrade ClusterExtensions. The `group` value must be 1–52 characters long, start with a lowercase letter, diff --git a/helm/olmv1/base/object-controller/crd/experimental/olm.operatorframework.io_clusterobjectsets.yaml b/helm/olmv1/base/object-controller/crd/experimental/olm.operatorframework.io_clusterobjectsets.yaml index 08cb039bab..c5c141ed09 100644 --- a/helm/olmv1/base/object-controller/crd/experimental/olm.operatorframework.io_clusterobjectsets.yaml +++ b/helm/olmv1/base/object-controller/crd/experimental/olm.operatorframework.io_clusterobjectsets.yaml @@ -545,7 +545,6 @@ spec: The revision field must be a positive integer. Each ClusterObjectSet in the same revision sequence must have a unique revision number. - The revision number must always be the previous revision number plus one, or 1 for the first revision. format: int64 minimum: 1 type: integer diff --git a/internal/object-controller/controllers/clusterobjectset_controller.go b/internal/object-controller/controllers/clusterobjectset_controller.go index 6c3690606d..1962bb4758 100644 --- a/internal/object-controller/controllers/clusterobjectset_controller.go +++ b/internal/object-controller/controllers/clusterobjectset_controller.go @@ -567,7 +567,7 @@ func (c *ClusterObjectSetReconciler) listOtherActiveRevisions( if r.Name == cos.Name { continue } - if r.Spec.LifecycleState == ocv1.ClusterObjectSetLifecycleStateArchived || + if r.Spec.LifecycleState != ocv1.ClusterObjectSetLifecycleStateActive || !r.DeletionTimestamp.IsZero() { continue } diff --git a/internal/object-controller/controllers/clusterobjectset_controller_internal_test.go b/internal/object-controller/controllers/clusterobjectset_controller_internal_test.go index 90c4358a95..61e2d097c7 100644 --- a/internal/object-controller/controllers/clusterobjectset_controller_internal_test.go +++ b/internal/object-controller/controllers/clusterobjectset_controller_internal_test.go @@ -23,7 +23,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" ocv1 "github.com/operator-framework/operator-controller/api/v1" - "github.com/operator-framework/operator-controller/test" + "github.com/operator-framework/operator-controller/internal/testutil" ) func Test_ClusterObjectSetReconciler_listSiblingRevisions(t *testing.T) { @@ -175,15 +175,6 @@ func Test_ClusterObjectSetReconciler_OwnerChangesPreserveRevisionChain(t *testin foreignOwner.Kind = "OtherController" foreignOwner.Name = "other-owner" foreignOwner.UID = "other-uid" - previous := newTestClusterObjectSetInternal(t, "rev-1") - current := newTestClusterObjectSetInternal(t, "rev-2") - current.OwnerReferences = []metav1.OwnerReference{owner} - otherGroup := newTestClusterObjectSetInternal(t, "other-1") - otherGroup.Spec.Group = "other-group" - otherGroup.OwnerReferences = []metav1.OwnerReference{owner} - cl := test.WithIndexes(t, fake.NewClientBuilder().WithScheme(scheme), SetupIndexes). - WithObjects(previous, current, otherGroup).Build() - r := &ClusterObjectSetReconciler{Client: cl} for _, tc := range []struct { name string @@ -201,23 +192,23 @@ func Test_ClusterObjectSetReconciler_OwnerChangesPreserveRevisionChain(t *testin currentOwners: []metav1.OwnerReference{owner}, }, { - name: "owner removed", + name: "previous revision ownerless", currentOwners: []metav1.OwnerReference{owner}, }, - { - name: "owner added again", - previousOwners: []metav1.OwnerReference{owner}, - currentOwners: []metav1.OwnerReference{owner}, - }, {name: "both revisions ownerless"}, } { t.Run(tc.name, func(t *testing.T) { - require.NoError(t, cl.Get(t.Context(), client.ObjectKeyFromObject(current), current)) - current.OwnerReferences = tc.currentOwners - require.NoError(t, cl.Update(t.Context(), current)) - require.NoError(t, cl.Get(t.Context(), client.ObjectKeyFromObject(previous), previous)) + previous := newTestClusterObjectSetInternal(t, "rev-1") previous.OwnerReferences = tc.previousOwners - require.NoError(t, cl.Update(t.Context(), previous)) + current := newTestClusterObjectSetInternal(t, "rev-2") + current.OwnerReferences = tc.currentOwners + otherGroup := newTestClusterObjectSetInternal(t, "other-1") + otherGroup.Spec.Group = "other-group" + otherGroup.OwnerReferences = []metav1.OwnerReference{owner} + cl := testutil.WithIndexes(t, fake.NewClientBuilder().WithScheme(scheme), SetupIndexes). + WithObjects(previous, current, otherGroup).Build() + r := &ClusterObjectSetReconciler{Client: cl} + siblings, err := r.listSiblingRevisions(t.Context(), current) require.NoError(t, err) require.Len(t, siblings, 1) @@ -239,7 +230,7 @@ func callRevisionLister( ) []string { t.Helper() mockCtrl := gomock.NewController(t) - testClient := test.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), SetupIndexes). + testClient := testutil.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), SetupIndexes). WithObjects(existingObjs...). Build() @@ -293,8 +284,9 @@ func newTestClusterObjectSetInternal(t *testing.T, name string) *ocv1.ClusterObj Generation: int64(1), }, Spec: ocv1.ClusterObjectSetSpec{ - Group: "test-ext", - Revision: revNum, + Group: "test-ext", + LifecycleState: ocv1.ClusterObjectSetLifecycleStateActive, + Revision: revNum, Phases: []ocv1.ClusterObjectSetPhase{ { Name: "everything", diff --git a/internal/object-controller/controllers/clusterobjectset_controller_test.go b/internal/object-controller/controllers/clusterobjectset_controller_test.go index 91cb3539ad..f5e830526a 100644 --- a/internal/object-controller/controllers/clusterobjectset_controller_test.go +++ b/internal/object-controller/controllers/clusterobjectset_controller_test.go @@ -31,9 +31,9 @@ import ( ocv1 "github.com/operator-framework/operator-controller/api/v1" "github.com/operator-framework/operator-controller/internal/object-controller/controllers" "github.com/operator-framework/operator-controller/internal/shared/labels" + "github.com/operator-framework/operator-controller/internal/testutil" mockcontrollers "github.com/operator-framework/operator-controller/internal/testutil/mock/controllers" mockmachinery "github.com/operator-framework/operator-controller/internal/testutil/mock/machinery" - "github.com/operator-framework/operator-controller/test" ) const clusterObjectSetName = "test-ext-1" @@ -587,22 +587,23 @@ func Test_ClusterObjectSetReconciler_Reconcile_RevisionReconciliation(t *testing }, }, { - name: "archive previous revisions on successful rollout", + name: "archive same-group predecessors regardless of controller ownership", revisionResult: newMockRevisionResult(mockCtrl, revisionResultConfig{ isComplete: true, }), reconcilingRevisionName: "test-ext-3", existingObjs: func() []client.Object { ext := newTestClusterExtension() - prevRev1 := newTestClusterObjectSet(t, "test-ext-1", ext, testScheme) - prevRev2 := newTestClusterObjectSet(t, "test-ext-2", ext, testScheme) - prevRev1.OwnerReferences = nil - prevRev2.OwnerReferences[0].Name = "other-owner" - prevRev2.OwnerReferences[0].UID = "other-uid" + ownerlessPrevious := newTestClusterObjectSet(t, "test-ext-1", ext, testScheme) + ownerlessPrevious.OwnerReferences = nil + differentOwnerPrevious := newTestClusterObjectSet(t, "test-ext-2", ext, testScheme) + differentOwnerPrevious.OwnerReferences[0].Name = "other-owner" + differentOwnerPrevious.OwnerReferences[0].UID = "other-uid" rev := newTestClusterObjectSet(t, "test-ext-3", ext, testScheme) + // Sharing the current revision's owner must not cause another group to be archived. otherGroup := newTestClusterObjectSet(t, "other-group-1", ext, testScheme) otherGroup.Spec.Group = "other-group" - return []client.Object{ext, prevRev1, prevRev2, rev, otherGroup} + return []client.Object{ext, ownerlessPrevious, differentOwnerPrevious, rev, otherGroup} }, validate: func(t *testing.T, c client.Client) { rev := &ocv1.ClusterObjectSet{} @@ -632,7 +633,7 @@ func Test_ClusterObjectSetReconciler_Reconcile_RevisionReconciliation(t *testing mockCtrl := gomock.NewController(t) // create extension and cluster extension - testClient := test.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), controllers.SetupIndexes). + testClient := testutil.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), controllers.SetupIndexes). WithStatusSubresource(&ocv1.ClusterObjectSet{}). WithObjects(tc.existingObjs()...). Build() @@ -735,7 +736,7 @@ func Test_ClusterObjectSetReconciler_Reconcile_CompletedAt(t *testing.T) { t.Run(tc.name, func(t *testing.T) { mockCtrl := gomock.NewController(t) - testClient := test.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), controllers.SetupIndexes). + testClient := testutil.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), controllers.SetupIndexes). WithStatusSubresource(&ocv1.ClusterObjectSet{}). WithObjects(tc.existingObjs()...). Build() @@ -848,7 +849,7 @@ func Test_ClusterObjectSetReconciler_Reconcile_ValidationError_Retries(t *testin rev1 := newTestClusterObjectSet(t, clusterObjectSetName, ext, testScheme) // create extension and cluster extension - testClient := test.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), controllers.SetupIndexes). + testClient := testutil.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), controllers.SetupIndexes). WithStatusSubresource(&ocv1.ClusterObjectSet{}). WithObjects(ext, rev1). Build() @@ -946,7 +947,7 @@ func Test_ClusterObjectSetReconciler_Reconcile_VerifyReferencedSecrets(t *testin }, }} - testClient := test.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), controllers.SetupIndexes). + testClient := testutil.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), controllers.SetupIndexes). WithStatusSubresource(&ocv1.ClusterObjectSet{}). WithObjects(append([]client.Object{ext, rev1}, tc.secrets...)...). WithInterceptorFuncs(interceptor.Funcs{ @@ -1018,7 +1019,7 @@ func Test_ClusterObjectSetReconciler_Reconcile_ReferencedContentValidationRecove Data: map[string][]byte{"configmap": []byte("not-json")}, } - testClient := test.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), controllers.SetupIndexes). + testClient := testutil.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), controllers.SetupIndexes). WithStatusSubresource(&ocv1.ClusterObjectSet{}). WithObjects(ext, rev1, badSecret). Build() @@ -1108,7 +1109,7 @@ func Test_ClusterObjectSetReconciler_Reconcile_BuildPhasesErrorClassification(t tc.setupCOS(rev1) } - builder := test.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), controllers.SetupIndexes). + builder := testutil.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), controllers.SetupIndexes). WithStatusSubresource(&ocv1.ClusterObjectSet{}). WithObjects(ext, rev1) if tc.listFails { @@ -1400,7 +1401,7 @@ func Test_ClusterObjectSetReconciler_Reconcile_ArchivalAndDeletion(t *testing.T) mockCtrl := gomock.NewController(t) // create extension and cluster extension - testClient := test.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), controllers.SetupIndexes). + testClient := testutil.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), controllers.SetupIndexes). WithStatusSubresource(&ocv1.ClusterObjectSet{}). WithObjects(tc.existingObjs()...). Build() @@ -1672,7 +1673,7 @@ func Test_ClusterObjectSetReconciler_Reconcile_ProgressDeadline(t *testing.T) { mockCtrl := gomock.NewController(t) // create extension and cluster extension - testClient := test.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), controllers.SetupIndexes). + testClient := testutil.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), controllers.SetupIndexes). WithStatusSubresource(&ocv1.ClusterObjectSet{}). WithObjects(tc.existingObjs()...). Build() @@ -2221,7 +2222,7 @@ func Test_ClusterObjectSetReconciler_Reconcile_ForeignRevisionCollision(t *testi } { t.Run(tc.name, func(t *testing.T) { mockCtrl := gomock.NewController(t) - testClient := test.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), controllers.SetupIndexes). + testClient := testutil.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), controllers.SetupIndexes). WithStatusSubresource(&ocv1.ClusterObjectSet{}). WithObjects(tc.existingObjs()...). Build() @@ -2322,7 +2323,7 @@ func Test_ClusterObjectSetReconciler_getScopedClient_Errors(t *testing.T) { ext := newTestClusterExtension() rev := newTestClusterObjectSet(t, "test-rev", ext, testScheme) - testClient := test.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), controllers.SetupIndexes). + testClient := testutil.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), controllers.SetupIndexes). WithObjects(ext, rev). Build() diff --git a/internal/object-controller/controllers/indexes_test.go b/internal/object-controller/controllers/indexes_test.go index 98516c9fa8..9d9b838f4e 100644 --- a/internal/object-controller/controllers/indexes_test.go +++ b/internal/object-controller/controllers/indexes_test.go @@ -13,13 +13,13 @@ import ( ocv1 "github.com/operator-framework/operator-controller/api/v1" "github.com/operator-framework/operator-controller/internal/object-controller/controllers" - "github.com/operator-framework/operator-controller/test" + "github.com/operator-framework/operator-controller/internal/testutil" ) func TestSetupIndexes_GroupLookup(t *testing.T) { scheme := runtime.NewScheme() require.NoError(t, ocv1.AddToScheme(scheme)) - cl := test.WithIndexes(t, fake.NewClientBuilder().WithScheme(scheme), controllers.SetupIndexes). + cl := testutil.WithIndexes(t, fake.NewClientBuilder().WithScheme(scheme), controllers.SetupIndexes). WithObjects( &ocv1.ClusterObjectSet{ ObjectMeta: metav1.ObjectMeta{ diff --git a/internal/object-controller/controllers/resolve_ref_test.go b/internal/object-controller/controllers/resolve_ref_test.go index a9e926023b..ddaf2cc218 100644 --- a/internal/object-controller/controllers/resolve_ref_test.go +++ b/internal/object-controller/controllers/resolve_ref_test.go @@ -26,7 +26,7 @@ import ( ocv1 "github.com/operator-framework/operator-controller/api/v1" "github.com/operator-framework/operator-controller/internal/object-controller/controllers" "github.com/operator-framework/operator-controller/internal/shared/labels" - "github.com/operator-framework/operator-controller/test" + "github.com/operator-framework/operator-controller/internal/testutil" ) func newSchemeWithCoreV1(t *testing.T) *apimachineryruntime.Scheme { @@ -69,7 +69,7 @@ func TestResolveObjectRef_PlainJSON(t *testing.T) { Key: "my-key", }) - fakeClient := test.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), controllers.SetupIndexes). + fakeClient := testutil.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), controllers.SetupIndexes). WithObjects(secret, cos). WithStatusSubresource(&ocv1.ClusterObjectSet{}). Build() @@ -131,7 +131,7 @@ func TestResolveObjectRef_GzipCompressed(t *testing.T) { Key: "my-key", }) - fakeClient := test.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), controllers.SetupIndexes). + fakeClient := testutil.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), controllers.SetupIndexes). WithObjects(secret, cos). WithStatusSubresource(&ocv1.ClusterObjectSet{}). Build() @@ -164,7 +164,7 @@ func TestResolveObjectRef_SecretNotFound(t *testing.T) { Key: "my-key", }) - fakeClient := test.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), controllers.SetupIndexes). + fakeClient := testutil.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), controllers.SetupIndexes). WithObjects(cos). WithStatusSubresource(&ocv1.ClusterObjectSet{}). Build() @@ -204,7 +204,7 @@ func TestResolveObjectRef_KeyNotFound(t *testing.T) { Key: "missing-key", }) - fakeClient := test.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), controllers.SetupIndexes). + fakeClient := testutil.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), controllers.SetupIndexes). WithObjects(secret, cos). WithStatusSubresource(&ocv1.ClusterObjectSet{}). Build() @@ -253,7 +253,7 @@ func TestResolveObjectRef_InvalidJSON(t *testing.T) { Key: "my-key", }) - fakeClient := test.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), controllers.SetupIndexes). + fakeClient := testutil.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), controllers.SetupIndexes). WithObjects(secret, cos). WithStatusSubresource(&ocv1.ClusterObjectSet{}). Build() diff --git a/internal/object-controller/controllers/revision_engine_factory_test.go b/internal/object-controller/controllers/revision_engine_factory_test.go index 527d4680f4..c328993ed4 100644 --- a/internal/object-controller/controllers/revision_engine_factory_test.go +++ b/internal/object-controller/controllers/revision_engine_factory_test.go @@ -20,10 +20,12 @@ package controllers_test import ( "context" + "fmt" "testing" "time" "github.com/go-logr/logr" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -42,7 +44,108 @@ import ( "github.com/operator-framework/operator-controller/internal/object-controller/controllers" ) +// TestDefaultRevisionEngineFactory_FieldOwnership uses envtest to verify the +// factory's SSA manager and OLM metadata configuration against a real API server. func TestDefaultRevisionEngineFactory_FieldOwnership(t *testing.T) { + t.Run("revisions in the same group reuse the SSA manager", func(t *testing.T) { + env := newRevisionEngineFactoryTestEnv(t) + objectKey := client.ObjectKey{Namespace: env.owner.Name, Name: "payload"} + + for _, revisionNumber := range []int64{1, 2} { + cos := &ocv1.ClusterObjectSet{ + ObjectMeta: metav1.ObjectMeta{Name: fmt.Sprintf("factory-test-%d", revisionNumber)}, + Spec: ocv1.ClusterObjectSetSpec{Group: "factory-test", Revision: revisionNumber}, + } + expectedRevision := fmt.Sprint(revisionNumber) + expectedValue := "revision-" + expectedRevision + desired := &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": map[string]any{"namespace": objectKey.Namespace, "name": objectKey.Name}, + "data": map[string]any{"value": expectedValue}, + }} + engine, err := env.factory.CreateRevisionEngine(env.ctx, cos) + require.NoError(t, err) + revision := boxcutter.NewRevisionWithOwner(cos.Name, cos.Spec.Revision, + []boxcutter.Phase{boxcutter.NewPhase("deploy", []client.Object{desired})}, + env.owner, ownerhandling.NewNative(env.scheme), + ) + + // Reconcile until the cached engine applies this revision's value. + actual := &corev1.ConfigMap{} + require.EventuallyWithT(t, func(collect *assert.CollectT) { + result, err := engine.Reconcile(env.ctx, revision) + if !assert.NoError(collect, err) || !assert.NotNil(collect, result) { + return + } + assert.Nil(collect, result.GetValidationError()) + if assert.NoError(collect, env.client.Get(env.ctx, objectKey, actual)) { + assert.Equal(collect, expectedValue, actual.Data["value"]) + } + }, 5*time.Second, 10*time.Millisecond) + + // Both revisions must leave one group-scoped SSA manager and OLM metadata. + require.Equal(t, []string{"cos-group/factory-test"}, applyFieldManagers(actual)) + require.Equal(t, expectedRevision, actual.Annotations["olm.operatorframework.io/revision"]) + require.Equal(t, "olm.operatorframework.io", actual.Labels["app.kubernetes.io/managed-by"]) + } + }) + + t.Run("different groups use distinct SSA managers", func(t *testing.T) { + env := newRevisionEngineFactoryTestEnv(t) + + for _, tc := range []struct { + group string + objectName string + expectedManager string + }{ + {group: "factory-test", objectName: "payload", expectedManager: "cos-group/factory-test"}, + {group: "factory-other", objectName: "other-payload", expectedManager: "cos-group/factory-other"}, + } { + cos := &ocv1.ClusterObjectSet{ + ObjectMeta: metav1.ObjectMeta{Name: tc.group + "-1"}, + Spec: ocv1.ClusterObjectSetSpec{Group: tc.group, Revision: 1}, + } + desired := &corev1.ConfigMap{ + TypeMeta: metav1.TypeMeta{APIVersion: "v1", Kind: "ConfigMap"}, + ObjectMeta: metav1.ObjectMeta{Namespace: env.owner.Name, Name: tc.objectName}, + Data: map[string]string{"value": "revision-1"}, + } + engine, err := env.factory.CreateRevisionEngine(env.ctx, cos) + require.NoError(t, err) + revision := boxcutter.NewRevisionWithOwner(cos.Name, cos.Spec.Revision, + []boxcutter.Phase{boxcutter.NewPhase("deploy", []client.Object{desired})}, + env.owner, ownerhandling.NewNative(env.scheme), + ) + + // Apply each group's revision to its own ConfigMap. + result, err := engine.Reconcile(env.ctx, revision) + require.NoError(t, err) + require.NotNil(t, result) + require.Nil(t, result.GetValidationError()) + + // Each resource must record its group's manager and the OLM metadata. + actual := &corev1.ConfigMap{} + require.NoError(t, env.client.Get(env.ctx, client.ObjectKeyFromObject(desired), actual)) + require.Equal(t, "revision-1", actual.Data["value"]) + require.Equal(t, []string{tc.expectedManager}, applyFieldManagers(actual)) + require.Equal(t, "1", actual.Annotations["olm.operatorframework.io/revision"]) + require.Equal(t, "olm.operatorframework.io", actual.Labels["app.kubernetes.io/managed-by"]) + } + }) +} + +type revisionEngineFactoryTestEnv struct { + ctx context.Context + client client.Client + scheme *runtime.Scheme + owner *corev1.Namespace + factory controllers.RevisionEngineFactory +} + +func newRevisionEngineFactoryTestEnv(t *testing.T) *revisionEngineFactoryTestEnv { + t.Helper() + scheme := newScheme(t) require.NoError(t, corev1.AddToScheme(scheme)) cl, err := client.New(config, client.Options{Scheme: scheme}) @@ -52,11 +155,8 @@ func TestDefaultRevisionEngineFactory_FieldOwnership(t *testing.T) { // metadata configuration from the COS controller's lifecycle behavior. owner := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{GenerateName: "factory-field-owner-"}} require.NoError(t, cl.Create(t.Context(), owner)) - objectKey := client.ObjectKey{Namespace: owner.Name, Name: "payload"} t.Cleanup(func() { - require.NoError(t, client.IgnoreNotFound(cl.Delete(context.Background(), &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{Namespace: objectKey.Namespace, Name: objectKey.Name}, - }))) + require.NoError(t, cl.DeleteAllOf(context.Background(), &corev1.ConfigMap{}, client.InNamespace(owner.Name))) require.NoError(t, cl.Delete(context.Background(), owner)) }) @@ -85,89 +185,17 @@ func TestDefaultRevisionEngineFactory_FieldOwnership(t *testing.T) { scheme, trackingCache, memory.NewMemCacheClient(discoveryClient), cl.RESTMapper(), config, ) require.NoError(t, err) - cos := &ocv1.ClusterObjectSet{ - ObjectMeta: metav1.ObjectMeta{Name: "factory-test-1"}, - Spec: ocv1.ClusterObjectSetSpec{Group: "factory-test", Revision: 1}, + return &revisionEngineFactoryTestEnv{ + ctx: ctx, client: cl, scheme: scheme, owner: owner, factory: factory, } - engine, err := factory.CreateRevisionEngine(ctx, cos) - require.NoError(t, err) - - obj := &corev1.ConfigMap{ - TypeMeta: metav1.TypeMeta{APIVersion: "v1", Kind: "ConfigMap"}, - ObjectMeta: metav1.ObjectMeta{Namespace: objectKey.Namespace, Name: objectKey.Name}, - Data: map[string]string{"value": "revision-1"}, - } - revision := boxcutter.NewRevisionWithOwner(cos.Name, cos.Spec.Revision, - []boxcutter.Phase{boxcutter.NewPhase("deploy", []client.Object{obj})}, - owner, ownerhandling.NewNative(scheme), - ) - result, err := engine.Reconcile(ctx, revision) - require.NoError(t, err) - require.NotNil(t, result) - require.Nil(t, result.GetValidationError(), "%v", result.GetValidationError()) - - actual := &corev1.ConfigMap{} - require.NoError(t, cl.Get(ctx, objectKey, actual)) - require.Equal(t, "1", actual.Annotations["olm.operatorframework.io/revision"]) - require.Equal(t, "olm.operatorframework.io", actual.Labels["app.kubernetes.io/managed-by"]) - - var applyManagers []string - for _, fields := range actual.ManagedFields { - if fields.Operation == metav1.ManagedFieldsOperationApply { - applyManagers = append(applyManagers, fields.Manager) - } - } - require.Contains(t, applyManagers, "cos-group/factory-test") +} - // A subsequent revision updates the same field using the same SSA manager. - cos.Name = "factory-test-2" - cos.Spec.Revision = 2 - engine, err = factory.CreateRevisionEngine(ctx, cos) - require.NoError(t, err) - obj.Data["value"] = "revision-2" - objectMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(obj) - require.NoError(t, err) - desired := &unstructured.Unstructured{Object: objectMap} - revision = boxcutter.NewRevisionWithOwner(cos.Name, cos.Spec.Revision, - []boxcutter.Phase{boxcutter.NewPhase("deploy", []client.Object{desired})}, - owner, ownerhandling.NewNative(scheme), - ) - require.Eventually(t, func() bool { - if _, err := engine.Reconcile(ctx, revision); err != nil { - return false - } - return cl.Get(ctx, objectKey, actual) == nil && actual.Data["value"] == "revision-2" - }, 5*time.Second, 10*time.Millisecond) - applyManagers = nil - for _, fields := range actual.ManagedFields { - if fields.Operation == metav1.ManagedFieldsOperationApply { - applyManagers = append(applyManagers, fields.Manager) - } - } - require.Equal(t, []string{"cos-group/factory-test"}, applyManagers) - require.Equal(t, "2", actual.Annotations["olm.operatorframework.io/revision"]) - - // Another group gets its own SSA manager. - cos.Name = "factory-other-1" - cos.Spec.Group = "factory-other" - cos.Spec.Revision = 1 - engine, err = factory.CreateRevisionEngine(ctx, cos) - require.NoError(t, err) - other := desired.DeepCopy() - other.SetName("other-payload") - t.Cleanup(func() { require.NoError(t, client.IgnoreNotFound(cl.Delete(context.Background(), other))) }) - revision = boxcutter.NewRevisionWithOwner(cos.Name, cos.Spec.Revision, - []boxcutter.Phase{boxcutter.NewPhase("deploy", []client.Object{other})}, - owner, ownerhandling.NewNative(scheme), - ) - _, err = engine.Reconcile(ctx, revision) - require.NoError(t, err) - require.NoError(t, cl.Get(ctx, client.ObjectKeyFromObject(other), actual)) - applyManagers = nil - for _, fields := range actual.ManagedFields { +func applyFieldManagers(obj client.Object) []string { + var managers []string + for _, fields := range obj.GetManagedFields() { if fields.Operation == metav1.ManagedFieldsOperationApply { - applyManagers = append(applyManagers, fields.Manager) + managers = append(managers, fields.Manager) } } - require.Equal(t, []string{"cos-group/factory-other"}, applyManagers) + return managers } diff --git a/internal/operator-controller/applier/boxcutter_test.go b/internal/operator-controller/applier/boxcutter_test.go index 9ba2346313..d6c2ac5778 100644 --- a/internal/operator-controller/applier/boxcutter_test.go +++ b/internal/operator-controller/applier/boxcutter_test.go @@ -34,9 +34,9 @@ import ( "github.com/operator-framework/operator-controller/internal/shared/labels" bundlecsv "github.com/operator-framework/operator-controller/internal/testing/bundle/csv" bundlefs "github.com/operator-framework/operator-controller/internal/testing/bundle/fs" + "github.com/operator-framework/operator-controller/internal/testutil" mockapplier "github.com/operator-framework/operator-controller/internal/testutil/mock/applier" mockctrlclient "github.com/operator-framework/operator-controller/internal/testutil/mock/ctrlclient" - "github.com/operator-framework/operator-controller/test" ) var ( @@ -550,6 +550,7 @@ func TestBoxcutter_Apply(t *testing.T) { }, } + // Exercise group-based discovery when the existing revision lacks the owner-name label. revisionWithoutOwnerLabel := defaultDesiredRevision.DeepCopy() revisionWithoutOwnerLabel.Labels = nil @@ -634,7 +635,7 @@ func TestBoxcutter_Apply(t *testing.T) { }, }, { - name: "no change, revision exists", + name: "no change, existing revision has no owner label", mockBuilder: func(t *testing.T) applier.ClusterObjectSetGenerator { ctrl := gomock.NewController(t) m := mockapplier.NewMockClusterObjectSetGenerator(ctrl) @@ -1107,16 +1108,8 @@ func TestBoxcutter_Apply(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { // Setup - cb := test.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), coscontrollers.SetupIndexes). + cb := testutil.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), coscontrollers.SetupIndexes). WithObjects(tc.existingObjs...) - // A misleading owner label must not let another group's revisions - // affect numbering or retention for this extension. - cb.WithObjects(&ocv1.ClusterObjectSet{ - ObjectMeta: metav1.ObjectMeta{Name: "other-group-99", Labels: map[string]string{labels.OwnerNameKey: ext.Name}}, - Spec: ocv1.ClusterObjectSetSpec{ - Group: "other-group", Revision: 99, LifecycleState: ocv1.ClusterObjectSetLifecycleStateArchived, - }, - }) if tc.clientIterceptor != nil { cb.WithInterceptorFuncs(*tc.clientIterceptor) } @@ -1164,54 +1157,111 @@ func TestBoxcutter_Apply(t *testing.T) { tc.validate(t, fakeClient) } } - require.NoError(t, fakeClient.Get(t.Context(), client.ObjectKey{Name: "other-group-99"}, &ocv1.ClusterObjectSet{})) }) } } -func TestBoxcutter_Apply_FallbackUsesGroup(t *testing.T) { +func TestBoxcutter_Apply_GroupIsolation(t *testing.T) { for _, tc := range []struct { - name string - group string - ownerName string - wantError bool + name string + extensionRevisions []int64 + otherGroupRevision int64 + wantRevision int64 + wantRemainingNames []string }{ - {name: "ownerless", group: "test-ext"}, - {name: "different controller owner", group: "test-ext", ownerName: "other-ext"}, - {name: "different group with matching owner", group: "other-group", ownerName: "test-ext", wantError: true}, + { + 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"}} - cos := &ocv1.ClusterObjectSet{ - ObjectMeta: metav1.ObjectMeta{Name: "revision-1"}, + 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: tc.group, Revision: 1, LifecycleState: ocv1.ClusterObjectSetLifecycleStateActive, + Group: "other-group", Revision: tc.otherGroupRevision, LifecycleState: ocv1.ClusterObjectSetLifecycleStateArchived, }, } - if tc.ownerName != "" { - owner := *metav1.NewControllerRef(ext, ocv1.GroupVersion.WithKind(ocv1.ClusterExtensionKind)) - owner.Name = tc.ownerName - if tc.ownerName != ext.Name { - owner.UID = "other-owner-uid" - } - cos.OwnerReferences = []metav1.OwnerReference{owner} - } - cl := test.WithIndexes(t, fake.NewClientBuilder().WithScheme(scheme), coscontrollers.SetupIndexes). - WithObjects(cos).Build() - bc := &applier.Boxcutter{ - Client: cl, Scheme: scheme, - RevisionGenerator: mockapplier.NewMockClusterObjectSetGenerator(gomock.NewController(t)), - } - completed, _, err := bc.Apply(t.Context(), nil, ext, nil, nil) - if tc.wantError { - require.ErrorContains(t, err, "no revision installed") - require.False(t, completed) - } else { - require.NoError(t, err) - require.True(t, completed) + 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) }) } } @@ -1246,7 +1296,7 @@ func TestBoxcutterStorageMigrator(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Name: "other-group-1", Labels: map[string]string{labels.OwnerNameKey: ext.Name}}, Spec: ocv1.ClusterObjectSetSpec{Group: "other-group", Revision: 1}, } - cl := test.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), coscontrollers.SetupIndexes). + cl := testutil.WithIndexes(t, fake.NewClientBuilder().WithScheme(testScheme), coscontrollers.SetupIndexes). WithStatusSubresource(&ocv1.ClusterObjectSet{}). WithObjects(other).Build() sm := &applier.BoxcutterStorageMigrator{ diff --git a/internal/operator-controller/controllers/boxcutter_reconcile_steps_internal_test.go b/internal/operator-controller/controllers/boxcutter_reconcile_steps_internal_test.go index 65a3c93c94..9403103696 100644 --- a/internal/operator-controller/controllers/boxcutter_reconcile_steps_internal_test.go +++ b/internal/operator-controller/controllers/boxcutter_reconcile_steps_internal_test.go @@ -27,7 +27,7 @@ import ( ocv1 "github.com/operator-framework/operator-controller/api/v1" coscontrollers "github.com/operator-framework/operator-controller/internal/object-controller/controllers" - "github.com/operator-framework/operator-controller/test" + "github.com/operator-framework/operator-controller/internal/testutil" ) func TestBoxcutterRevisionStatesGetter_ClassifiesByCompletedAt(t *testing.T) { @@ -52,7 +52,7 @@ func TestBoxcutterRevisionStatesGetter_ClassifiesByCompletedAt(t *testing.T) { completed := newRevision("test-ext-1", 1, true) rollingOut := newRevision("test-ext-2", 2, false) - cl := test.WithIndexes(t, fake.NewClientBuilder().WithScheme(sch), coscontrollers.SetupIndexes). + cl := testutil.WithIndexes(t, fake.NewClientBuilder().WithScheme(sch), coscontrollers.SetupIndexes). WithObjects(completed, rollingOut). Build() diff --git a/internal/operator-controller/controllers/clusterextension_controller.go b/internal/operator-controller/controllers/clusterextension_controller.go index 2367ad88a1..25a071b29a 100644 --- a/internal/operator-controller/controllers/clusterextension_controller.go +++ b/internal/operator-controller/controllers/clusterextension_controller.go @@ -432,6 +432,10 @@ func (r *ClusterExtensionReconciler) SetupWithManagerForBoxcutter(mgr ctrl.Manag // SetupWithManagerForHelm sets up the controller with watches for managed Helm resources. func (r *ClusterExtensionReconciler) SetupWithManagerForHelm(mgr ctrl.Manager, trackingCache managedcache.TrackingCache) (crcontroller.Controller, error) { + if err := mgr.Add(trackingCache); err != nil { + return nil, fmt.Errorf("unable to add tracking cache to manager: %w", err) + } + return r.newControllerBuilder(mgr). WatchesRawSource(trackingCache.Source( crhandler.EnqueueRequestForOwner(mgr.GetScheme(), mgr.GetRESTMapper(), &ocv1.ClusterExtension{}), diff --git a/internal/operator-controller/revisions/clusterobjectsets_test.go b/internal/operator-controller/revisions/clusterobjectsets_test.go index 670ac30d7b..4b91b37979 100644 --- a/internal/operator-controller/revisions/clusterobjectsets_test.go +++ b/internal/operator-controller/revisions/clusterobjectsets_test.go @@ -30,7 +30,7 @@ import ( coscontrollers "github.com/operator-framework/operator-controller/internal/object-controller/controllers" "github.com/operator-framework/operator-controller/internal/operator-controller/revisions" "github.com/operator-framework/operator-controller/internal/shared/labels" - "github.com/operator-framework/operator-controller/test" + "github.com/operator-framework/operator-controller/internal/testutil" ) func TestListForGroup(t *testing.T) { @@ -74,7 +74,7 @@ func TestListForGroup(t *testing.T) { for _, object := range objects[2:] { object.SetLabels(map[string]string{labels.OwnerNameKey: ext.Name}) } - cl := test.WithIndexes(t, fake.NewClientBuilder().WithScheme(scheme), coscontrollers.SetupIndexes). + cl := testutil.WithIndexes(t, fake.NewClientBuilder().WithScheme(scheme), coscontrollers.SetupIndexes). WithObjects(objects...).Build() got, err := revisions.ListForGroup(t.Context(), cl, group) require.NoError(t, err) diff --git a/test/indexes.go b/internal/testutil/indexes.go similarity index 97% rename from test/indexes.go rename to internal/testutil/indexes.go index 032d578fbe..0233d800ba 100644 --- a/test/indexes.go +++ b/internal/testutil/indexes.go @@ -1,4 +1,4 @@ -package test +package testutil import ( "context" diff --git a/manifests/experimental-e2e.yaml b/manifests/experimental-e2e.yaml index 8ef8b11983..89cc5acea5 100644 --- a/manifests/experimental-e2e.yaml +++ b/manifests/experimental-e2e.yaml @@ -1956,7 +1956,6 @@ spec: The revision field must be a positive integer. Each ClusterObjectSet in the same revision sequence must have a unique revision number. - The revision number must always be the previous revision number plus one, or 1 for the first revision. format: int64 minimum: 1 type: integer diff --git a/manifests/experimental.yaml b/manifests/experimental.yaml index a887166aaa..dcd0e14e04 100644 --- a/manifests/experimental.yaml +++ b/manifests/experimental.yaml @@ -1917,7 +1917,6 @@ spec: The revision field must be a positive integer. Each ClusterObjectSet in the same revision sequence must have a unique revision number. - The revision number must always be the previous revision number plus one, or 1 for the first revision. format: int64 minimum: 1 type: integer From 4a2103d60bd0ea1083a93319f4546e0b087222c8 Mon Sep 17 00:00:00 2001 From: Francesco Giudici Date: Fri, 16 Oct 2026 15:26:18 +0200 Subject: [PATCH 10/10] Fix object count scenario with required revision group Signed-off-by: Francesco Giudici --- test/e2e/features/revision.feature | 1 + 1 file changed, 1 insertion(+) diff --git a/test/e2e/features/revision.feature b/test/e2e/features/revision.feature index bbf7ecb939..a52e48a1ee 100644 --- a/test/e2e/features/revision.feature +++ b/test/e2e/features/revision.feature @@ -723,6 +723,7 @@ Feature: Install ClusterObjectSet metadata: name: ${COS_NAME} spec: + group: ${NAME} lifecycleState: Active collisionProtection: Prevent progressionProbes: