From f88c1e5ed0b4aa34421b5e7348319d3f971c16a1 Mon Sep 17 00:00:00 2001 From: Francesco Giudici Date: Thu, 1 Oct 2026 14:06:41 +0200 Subject: [PATCH 1/6] 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 57cab880ff..d18d929714 100644 --- a/api/v1/clusterobjectset_types.go +++ b/api/v1/clusterobjectset_types.go @@ -39,6 +39,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. @@ -57,11 +69,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 @@ -561,15 +573,16 @@ type ObservedPhase 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"` @@ -579,8 +592,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 86158d145f..eee564fc42 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 @@ -256,6 +264,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 @@ -279,6 +291,7 @@ kind: ClusterObjectSet metadata: name: my-app-rev1 spec: + group: my-app revision: 1 lifecycleState: Active collisionProtection: Prevent @@ -386,6 +399,7 @@ kind: ClusterObjectSet metadata: name: my-app-rev1 spec: + group: my-app revision: 1 lifecycleState: Active collisionProtection: Prevent @@ -422,8 +436,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 @@ -435,7 +450,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 554e2a5b8f..f78231c934 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 fc11214506e58a7582ea96853b259a9d44f8c6c2 Mon Sep 17 00:00:00 2001 From: Francesco Giudici Date: Thu, 1 Oct 2026 14:09:01 +0200 Subject: [PATCH 2/6] 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 1da3b4c1a6..bca0675a8e 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 0df374be97..b1f176d1bc 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 @@ -673,6 +693,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 4c2cce2adc..322e8d68ff 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 @@ -2084,6 +2104,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 146a9513e8..6186b44176 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 @@ -2045,6 +2065,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 8420c794e410f4b1463c02e10a9a79ec1a244e2c Mon Sep 17 00:00:00 2001 From: Francesco Giudici Date: Thu, 1 Oct 2026 14:49:02 +0200 Subject: [PATCH 3/6] 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 ed08eed75c..4dac32b8dd 100644 --- a/internal/object-controller/controllers/clusterobjectset_controller.go +++ b/internal/object-controller/controllers/clusterobjectset_controller.go @@ -412,6 +412,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). @@ -495,14 +506,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 @@ -514,15 +526,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 4c6d5595d1..be790cb02b 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 3d030bd9ae..42d18ff60f 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" @@ -451,8 +452,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{} @@ -473,6 +479,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) }, }, } { @@ -480,8 +488,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() @@ -584,8 +591,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() @@ -698,8 +704,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() @@ -797,8 +802,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{ @@ -864,8 +868,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() @@ -956,8 +959,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 { @@ -1249,8 +1251,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() @@ -1482,8 +1483,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() @@ -1552,6 +1552,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{ @@ -1963,8 +1964,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() @@ -2065,8 +2065,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 eba5c81a8b..c2b505af2e 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 e64c1b2be7b271fb592b0277690007e16987f769 Mon Sep 17 00:00:00 2001 From: Francesco Giudici Date: Thu, 1 Oct 2026 14:49:27 +0200 Subject: [PATCH 4/6] 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 3ae8b98ccc..95d0eec364 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 75c93d0fa7..9cf0b15056 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 94520d0f18..d24ad0ad9a 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 330dffc41e..9840005bfe 100644 --- a/test/e2e/steps/steps.go +++ b/test/e2e/steps/steps.go @@ -129,11 +129,18 @@ 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) 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) @@ -550,14 +557,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 } @@ -2298,21 +2315,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 b2a0a8a2e8d3eb854cf739276ba14391a3a2a629 Mon Sep 17 00:00:00 2001 From: Francesco Giudici Date: Tue, 13 Oct 2026 11:50:35 +0200 Subject: [PATCH 5/6] 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 d18d929714..ab9b5f6eae 100644 --- a/api/v1/clusterobjectset_types.go +++ b/api/v1/clusterobjectset_types.go @@ -573,6 +573,7 @@ type ObservedPhase 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 eee564fc42..5952c6561b 100644 --- a/docs/draft/concepts/clusterobjectsets.md +++ b/docs/draft/concepts/clusterobjectsets.md @@ -437,8 +437,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 b1f176d1bc..aae0408370 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 @@ -696,6 +696,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 322e8d68ff..4b4edb0dc7 100644 --- a/manifests/experimental-e2e.yaml +++ b/manifests/experimental-e2e.yaml @@ -2107,6 +2107,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 6186b44176..e0c6c9cc13 100644 --- a/manifests/experimental.yaml +++ b/manifests/experimental.yaml @@ -2068,6 +2068,8 @@ spec: required: - spec type: object + selectableFields: + - jsonPath: .spec.group served: true storage: true subresources: From 6c66220c7a4104e31c88d873791cf04b3ebcd76b Mon Sep 17 00:00:00 2001 From: Francesco Giudici Date: Tue, 13 Oct 2026 13:21:08 +0200 Subject: [PATCH 6/6] 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 {