From 237203ff69cb750bbe37980e1fae5fa4b4aaca11 Mon Sep 17 00:00:00 2001 From: Predrag Knezevic Date: Wed, 8 Apr 2026 17:23:43 +0200 Subject: [PATCH 1/5] Ensure COS phase immutability for referenced object approach ClusterObjectSet phases are immutable by design, but when objects are stored in external Secrets via refs, the Secret content could be changed by deleting and recreating the Secret with the same name. This enforces phase immutability for the referenced object approach by verifying that referenced Secrets are marked immutable and that their content hashes have not changed since first resolution. On first successful reconciliation, SHA-256 hashes of referenced Secret data are recorded in .status.observedObjectContainers. On subsequent reconciles, the hashes are compared and reconciliation is blocked with Progressing=False/Reason=Blocked if any mismatch is detected. Co-Authored-By: Claude Opus 4.6 --- api/v1/clusterobjectset_types.go | 24 ++ api/v1/zz_generated.deepcopy.go | 20 ++ .../api/v1/clusterobjectsetstatus.go | 17 ++ .../api/v1/observedobjectcontainer.go | 53 ++++ applyconfigurations/utils.go | 2 + docs/draft/concepts/large-bundle-support.md | 27 +- ...peratorframework.io_clusterobjectsets.yaml | 27 ++ .../clusterobjectset_controller.go | 152 +++++++++++- ...usterobjectset_controller_internal_test.go | 234 ++++++++++++++++++ .../controllers/resolve_ref_test.go | 5 + manifests/experimental-e2e.yaml | 27 ++ manifests/experimental.yaml | 27 ++ test/e2e/features/revision.feature | 133 +++++++++- test/e2e/steps/steps.go | 44 ++++ 14 files changed, 779 insertions(+), 13 deletions(-) create mode 100644 applyconfigurations/api/v1/observedobjectcontainer.go diff --git a/api/v1/clusterobjectset_types.go b/api/v1/clusterobjectset_types.go index ed1a4001e0..3ed14d2406 100644 --- a/api/v1/clusterobjectset_types.go +++ b/api/v1/clusterobjectset_types.go @@ -510,6 +510,30 @@ type ClusterObjectSetStatus struct { // +listMapKey=type // +optional Conditions []metav1.Condition `json:"conditions,omitempty"` + + // observedObjectContainers records the content hashes of referenced Secrets + // at first successful resolution. This is used to detect if a referenced + // Secret was deleted and recreated with different content. + // + // +listType=map + // +listMapKey=name + // +optional + ObservedObjectContainers []ObservedObjectContainer `json:"observedObjectContainers,omitempty"` +} + +// ObservedObjectContainer records the observed state of a referenced Secret +// used as an object source. +type ObservedObjectContainer struct { + // name identifies the referenced Secret in "namespace/name" format. + // + // +required + Name string `json:"name"` + + // hash is the hex-encoded SHA-256 hash of the Secret's .data field + // at first successful resolution. + // + // +required + Hash string `json:"hash"` } // +genclient diff --git a/api/v1/zz_generated.deepcopy.go b/api/v1/zz_generated.deepcopy.go index 8d1cea0245..ea50ebae21 100644 --- a/api/v1/zz_generated.deepcopy.go +++ b/api/v1/zz_generated.deepcopy.go @@ -555,6 +555,11 @@ func (in *ClusterObjectSetStatus) DeepCopyInto(out *ClusterObjectSetStatus) { (*in)[i].DeepCopyInto(&(*out)[i]) } } + if in.ObservedObjectContainers != nil { + in, out := &in.ObservedObjectContainers, &out.ObservedObjectContainers + *out = make([]ObservedObjectContainer, len(*in)) + copy(*out, *in) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ClusterObjectSetStatus. @@ -664,6 +669,21 @@ func (in *ObjectSourceRef) DeepCopy() *ObjectSourceRef { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *ObservedObjectContainer) DeepCopyInto(out *ObservedObjectContainer) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ObservedObjectContainer. +func (in *ObservedObjectContainer) DeepCopy() *ObservedObjectContainer { + if in == nil { + return nil + } + out := new(ObservedObjectContainer) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *PreflightConfig) DeepCopyInto(out *PreflightConfig) { *out = *in diff --git a/applyconfigurations/api/v1/clusterobjectsetstatus.go b/applyconfigurations/api/v1/clusterobjectsetstatus.go index 987dd98394..aa624f5059 100644 --- a/applyconfigurations/api/v1/clusterobjectsetstatus.go +++ b/applyconfigurations/api/v1/clusterobjectsetstatus.go @@ -46,6 +46,10 @@ type ClusterObjectSetStatusApplyConfiguration struct { // The Succeeded condition represents whether the revision has successfully completed its rollout: // - When status is True and reason is Succeeded, the ClusterObjectSet has successfully completed its rollout. This condition is set once and persists even if the revision later becomes unavailable. Conditions []metav1.ConditionApplyConfiguration `json:"conditions,omitempty"` + // observedObjectContainers records the content hashes of referenced Secrets + // at first successful resolution. This is used to detect if a referenced + // Secret was deleted and recreated with different content. + ObservedObjectContainers []ObservedObjectContainerApplyConfiguration `json:"observedObjectContainers,omitempty"` } // ClusterObjectSetStatusApplyConfiguration constructs a declarative configuration of the ClusterObjectSetStatus type for use with @@ -66,3 +70,16 @@ func (b *ClusterObjectSetStatusApplyConfiguration) WithConditions(values ...*met } return b } + +// WithObservedObjectContainers adds the given value to the ObservedObjectContainers field in the declarative configuration +// and returns the receiver, so that objects can be build by chaining "With" function invocations. +// If called multiple times, values provided by each call will be appended to the ObservedObjectContainers field. +func (b *ClusterObjectSetStatusApplyConfiguration) WithObservedObjectContainers(values ...*ObservedObjectContainerApplyConfiguration) *ClusterObjectSetStatusApplyConfiguration { + for i := range values { + if values[i] == nil { + panic("nil value passed to WithObservedObjectContainers") + } + b.ObservedObjectContainers = append(b.ObservedObjectContainers, *values[i]) + } + return b +} diff --git a/applyconfigurations/api/v1/observedobjectcontainer.go b/applyconfigurations/api/v1/observedobjectcontainer.go new file mode 100644 index 0000000000..b8098355fa --- /dev/null +++ b/applyconfigurations/api/v1/observedobjectcontainer.go @@ -0,0 +1,53 @@ +/* +Copyright 2022. + +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. +*/ +// Code generated by controller-gen-v0.20. DO NOT EDIT. + +package v1 + +// ObservedObjectContainerApplyConfiguration represents a declarative configuration of the ObservedObjectContainer type for use +// with apply. +// +// ObservedObjectContainer records the observed state of a referenced Secret +// used as an object source. +type ObservedObjectContainerApplyConfiguration struct { + // name identifies the referenced Secret in "namespace/name" format. + Name *string `json:"name,omitempty"` + // hash is the hex-encoded SHA-256 hash of the Secret's .data field + // at first successful resolution. + Hash *string `json:"hash,omitempty"` +} + +// ObservedObjectContainerApplyConfiguration constructs a declarative configuration of the ObservedObjectContainer type for use with +// apply. +func ObservedObjectContainer() *ObservedObjectContainerApplyConfiguration { + return &ObservedObjectContainerApplyConfiguration{} +} + +// WithName sets the Name 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 Name field is set to the value of the last call. +func (b *ObservedObjectContainerApplyConfiguration) WithName(value string) *ObservedObjectContainerApplyConfiguration { + b.Name = &value + return b +} + +// WithHash sets the Hash 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 Hash field is set to the value of the last call. +func (b *ObservedObjectContainerApplyConfiguration) WithHash(value string) *ObservedObjectContainerApplyConfiguration { + b.Hash = &value + return b +} diff --git a/applyconfigurations/utils.go b/applyconfigurations/utils.go index 7b1373e4a1..1b18f7da36 100644 --- a/applyconfigurations/utils.go +++ b/applyconfigurations/utils.go @@ -83,6 +83,8 @@ func ForKind(kind schema.GroupVersionKind) interface{} { return &apiv1.ObjectSelectorApplyConfiguration{} case v1.SchemeGroupVersion.WithKind("ObjectSourceRef"): return &apiv1.ObjectSourceRefApplyConfiguration{} + case v1.SchemeGroupVersion.WithKind("ObservedObjectContainer"): + return &apiv1.ObservedObjectContainerApplyConfiguration{} case v1.SchemeGroupVersion.WithKind("PreflightConfig"): return &apiv1.PreflightConfigApplyConfiguration{} case v1.SchemeGroupVersion.WithKind("ProgressionProbe"): diff --git a/docs/draft/concepts/large-bundle-support.md b/docs/draft/concepts/large-bundle-support.md index 325aaacc1c..bfc053f11f 100644 --- a/docs/draft/concepts/large-bundle-support.md +++ b/docs/draft/concepts/large-bundle-support.md @@ -142,14 +142,18 @@ Recommended conventions: 1. **Secret type**: Secrets should use the dedicated type `olm.operatorframework.io/object-data` to distinguish them from user-created Secrets and enable easy identification. The system always sets this type on - Secrets it creates. The reconciler does not enforce the type when resolving - refs — Secrets with any type are accepted — but producers should set it for - consistency. - -2. **Immutability**: Secrets should set `immutable: true`. Because COS phases - are immutable, the content backing a ref should not change after creation. - Mutable referenced Secrets are not rejected, but modifying them after the - COS is created leads to undefined behavior. + Secrets it creates. The reconciler does not enforce the Secret type when + resolving refs, but it does enforce that referenced Secrets have + `immutable: true` set and that their content has not changed since first + resolution. + +2. **Immutability**: Secrets must set `immutable: true`. The reconciler verifies + that all referenced Secrets have `immutable: true` set before proceeding. + Mutable referenced Secrets are rejected and reconciliation is blocked with + `Progressing=False, Reason=Blocked`. Additionally, the reconciler records + content hashes of referenced Secrets on first resolution and blocks + reconciliation if the content changes (e.g., if a Secret is deleted and + recreated with the same name but different data). 3. **Owner references**: Referenced Secrets should carry an ownerReference to the COS so that Kubernetes garbage collection removes them when the COS is @@ -388,6 +392,13 @@ Key properties: ### COS reconciler behavior +Before resolving individual object refs, the reconciler verifies all referenced +Secrets: each must have `immutable: true` set, and its content hash must match +the hash recorded in `.status.observedObjectContainers` (if present). If any Secret +fails verification, reconciliation is blocked with `Progressing=False, +Reason=Blocked`. On first successful verification, content hashes are persisted +to status for future comparisons. + When processing a COS phase: - For each object entry in the phase: - If `object` is set, use it directly (current behavior, unchanged). diff --git a/helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterobjectsets.yaml b/helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterobjectsets.yaml index eb4f55a9ea..4d529a5d07 100644 --- a/helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterobjectsets.yaml +++ b/helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterobjectsets.yaml @@ -621,6 +621,33 @@ spec: x-kubernetes-list-map-keys: - type x-kubernetes-list-type: map + observedObjectContainers: + description: |- + observedObjectContainers records the content hashes of referenced Secrets + at first successful resolution. This is used to detect if a referenced + Secret was deleted and recreated with different content. + items: + description: |- + ObservedObjectContainer records the observed state of a referenced Secret + used as an object source. + properties: + hash: + description: |- + hash is the hex-encoded SHA-256 hash of the Secret's .data field + at first successful resolution. + type: string + name: + description: name identifies the referenced Secret in "namespace/name" + format. + type: string + required: + - hash + - name + type: object + type: array + x-kubernetes-list-map-keys: + - name + x-kubernetes-list-type: map type: object type: object served: true diff --git a/internal/operator-controller/controllers/clusterobjectset_controller.go b/internal/operator-controller/controllers/clusterobjectset_controller.go index b34730b774..20eed825d0 100644 --- a/internal/operator-controller/controllers/clusterobjectset_controller.go +++ b/internal/operator-controller/controllers/clusterobjectset_controller.go @@ -6,6 +6,7 @@ import ( "bytes" "compress/gzip" "context" + "crypto/sha256" "encoding/json" "errors" "fmt" @@ -15,6 +16,7 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/equality" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -144,6 +146,21 @@ func (c *ClusterObjectSetReconciler) reconcile(ctx context.Context, cos *ocv1.Cl return c.delete(ctx, cos) } + resolvedHashes, err := c.verifyReferencedSecrets(ctx, cos) + if err != nil { + var sce *secretContentChangedError + if errors.As(err, &sce) { + l.Error(err, "referenced Secret content changed, blocking reconciliation") + } else { + l.Error(err, "referenced Secret verification failed, blocking reconciliation") + } + markAsNotProgressing(cos, ocv1.ClusterObjectSetReasonBlocked, err.Error()) + return ctrl.Result{}, nil + } + cos.Status.ObservedObjectContainers = mergeObservedObjectContainers( + cos.Status.ObservedObjectContainers, resolvedHashes, + ) + phases, opts, err := c.buildBoxcutterPhases(ctx, cos) if err != nil { setRetryingConditions(cos, err.Error()) @@ -333,7 +350,7 @@ type Sourcoser interface { } func (c *ClusterObjectSetReconciler) SetupWithManager(mgr ctrl.Manager) error { - skipProgressDeadlineExceededPredicate := predicate.Funcs{ + skipTerminallyBlockedPredicate := predicate.Funcs{ UpdateFunc: func(e event.UpdateEvent) bool { rev, ok := e.ObjectNew.(*ocv1.ClusterObjectSet) if !ok { @@ -343,8 +360,10 @@ func (c *ClusterObjectSetReconciler) SetupWithManager(mgr ctrl.Manager) error { if !rev.DeletionTimestamp.IsZero() { return true } - if cnd := meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterObjectSetTypeProgressing); cnd != nil && cnd.Status == metav1.ConditionFalse && cnd.Reason == ocv1.ReasonProgressDeadlineExceeded { - return false + if cnd := meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterObjectSetTypeProgressing); cnd != nil && cnd.Status == metav1.ConditionFalse { + if cnd.Reason == ocv1.ReasonProgressDeadlineExceeded || cnd.Reason == ocv1.ClusterObjectSetReasonBlocked { + return false + } } return true }, @@ -355,7 +374,7 @@ func (c *ClusterObjectSetReconciler) SetupWithManager(mgr ctrl.Manager) error { &ocv1.ClusterObjectSet{}, builder.WithPredicates( predicate.ResourceVersionChangedPredicate{}, - skipProgressDeadlineExceededPredicate, + skipTerminallyBlockedPredicate, ), ). WatchesRawSource( @@ -693,3 +712,128 @@ func markAsArchived(cos *ocv1.ClusterObjectSet) bool { updated := markAsNotProgressing(cos, ocv1.ClusterObjectSetReasonArchived, msg) return markAsAvailableUnknown(cos, ocv1.ClusterObjectSetReasonArchived, msg) || updated } + +// secretContentChangedError indicates that a referenced Secret's content +// has changed since it was first resolved (e.g., deleted and recreated +// with different data). +type secretContentChangedError struct { + secretKey string + expectedHash string + actualHash string +} + +func (e *secretContentChangedError) Error() string { + return fmt.Sprintf("content of referenced Secret %s has changed (expected hash %s, got %s): "+ + "the Secret may have been deleted and recreated with different content", + e.secretKey, e.expectedHash, e.actualHash) +} + +// computeSecretHash computes a deterministic SHA-256 hash of a Secret's +// .Data field. JSON serialization is used to produce a canonical encoding +// with sorted map keys. +func computeSecretHash(secret *corev1.Secret) string { + data, _ := json.Marshal(secret.Data) + sum := sha256.Sum256(data) + return fmt.Sprintf("%x", sum) +} + +// mergeObservedObjectContainers merges newly resolved containers into the +// existing list, adding entries that are not yet present. This ensures that +// late-arriving Secrets (e.g., not yet in the informer cache on first +// reconcile) get their hashes recorded on subsequent reconciles. +func mergeObservedObjectContainers(existing, resolved []ocv1.ObservedObjectContainer) []ocv1.ObservedObjectContainer { + if len(resolved) == 0 { + return existing + } + seen := make(map[string]struct{}, len(existing)) + for _, e := range existing { + seen[e.Name] = struct{}{} + } + merged := existing + for _, r := range resolved { + if _, ok := seen[r.Name]; !ok { + merged = append(merged, r) + seen[r.Name] = struct{}{} + } + } + return merged +} + +// verifyReferencedSecrets iterates over all Secret refs in the COS phases, +// fetches each unique Secret, and verifies: +// 1. The Secret has Immutable set to true. +// 2. The Secret's content hash matches the previously recorded hash (if any). +// +// On first successful verification (no existing hashes in status), it returns +// the computed hashes for the caller to persist. Returns a secretContentChangedError +// if a hash mismatch is detected. +func (c *ClusterObjectSetReconciler) verifyReferencedSecrets(ctx context.Context, cos *ocv1.ClusterObjectSet) ([]ocv1.ObservedObjectContainer, error) { + // Collect unique secret references + type secretRef struct { + name string + namespace string + } + seen := make(map[secretRef]struct{}) + var refs []secretRef + + for _, phase := range cos.Spec.Phases { + for _, obj := range phase.Objects { + if obj.Ref.Name == "" { + continue + } + sr := secretRef{name: obj.Ref.Name, namespace: obj.Ref.Namespace} + if _, ok := seen[sr]; !ok { + seen[sr] = struct{}{} + refs = append(refs, sr) + } + } + } + + if len(refs) == 0 { + return nil, nil + } + + // Build a lookup map from existing observed refs + observedMap := make(map[string]string, len(cos.Status.ObservedObjectContainers)) + for _, observed := range cos.Status.ObservedObjectContainers { + observedMap[observed.Name] = observed.Hash + } + + var resolvedRefs []ocv1.ObservedObjectContainer + for _, ref := range refs { + secret := &corev1.Secret{} + key := client.ObjectKey{Name: ref.name, Namespace: ref.namespace} + if err := c.Client.Get(ctx, key, secret); err != nil { + if apierrors.IsNotFound(err) { + // Secret not yet available — skip verification. + // resolveObjectRef will handle the not-found with a retryable error. + continue + } + return nil, fmt.Errorf("getting Secret %s/%s: %w", ref.namespace, ref.name, err) + } + + if secret.Immutable == nil || !*secret.Immutable { + return nil, fmt.Errorf("secret %s/%s is not immutable: referenced secrets must have immutable set to true", ref.namespace, ref.name) + } + + secretKey := fmt.Sprintf("%s/%s", ref.namespace, ref.name) + currentHash := computeSecretHash(secret) + + if previousHash, ok := observedMap[secretKey]; ok { + if currentHash != previousHash { + return nil, &secretContentChangedError{ + secretKey: secretKey, + expectedHash: previousHash, + actualHash: currentHash, + } + } + } + + resolvedRefs = append(resolvedRefs, ocv1.ObservedObjectContainer{ + Name: secretKey, + Hash: currentHash, + }) + } + + return resolvedRefs, nil +} diff --git a/internal/operator-controller/controllers/clusterobjectset_controller_internal_test.go b/internal/operator-controller/controllers/clusterobjectset_controller_internal_test.go index 15300f63ca..11e313e676 100644 --- a/internal/operator-controller/controllers/clusterobjectset_controller_internal_test.go +++ b/internal/operator-controller/controllers/clusterobjectset_controller_internal_test.go @@ -5,12 +5,15 @@ import ( "testing" "time" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" @@ -235,3 +238,234 @@ func (m *mockTrackingCacheInternal) Watch(ctx context.Context, user client.Objec func (m *mockTrackingCacheInternal) Source(h handler.EventHandler, predicates ...predicate.Predicate) source.Source { return nil } + +func TestComputeSecretHash(t *testing.T) { + t.Run("deterministic for same data", func(t *testing.T) { + secret := &corev1.Secret{ + Data: map[string][]byte{ + "key1": []byte("value1"), + "key2": []byte("value2"), + }, + } + hash1 := computeSecretHash(secret) + hash2 := computeSecretHash(secret) + assert.Equal(t, hash1, hash2) + }) + + t.Run("different for different data", func(t *testing.T) { + s1 := &corev1.Secret{Data: map[string][]byte{"key": []byte("value1")}} + s2 := &corev1.Secret{Data: map[string][]byte{"key": []byte("value2")}} + assert.NotEqual(t, computeSecretHash(s1), computeSecretHash(s2)) + }) + + t.Run("different for different keys", func(t *testing.T) { + s1 := &corev1.Secret{Data: map[string][]byte{"key1": []byte("value")}} + s2 := &corev1.Secret{Data: map[string][]byte{"key2": []byte("value")}} + assert.NotEqual(t, computeSecretHash(s1), computeSecretHash(s2)) + }) + + t.Run("empty data", func(t *testing.T) { + secret := &corev1.Secret{Data: map[string][]byte{}} + hash := computeSecretHash(secret) + assert.NotEmpty(t, hash) + }) +} + +func TestVerifyReferencedSecrets(t *testing.T) { + testScheme := runtime.NewScheme() + require.NoError(t, ocv1.AddToScheme(testScheme)) + require.NoError(t, corev1.AddToScheme(testScheme)) + + immutableSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-secret", + Namespace: "test-ns", + }, + Immutable: ptr.To(true), + Data: map[string][]byte{ + "obj": []byte(`{"apiVersion":"v1","kind":"ConfigMap"}`), + }, + } + + t.Run("succeeds on first resolution with no existing hashes", func(t *testing.T) { + testClient := fake.NewClientBuilder(). + WithScheme(testScheme). + WithObjects(immutableSecret.DeepCopy()). + Build() + + reconciler := &ClusterObjectSetReconciler{Client: testClient} + + cos := &ocv1.ClusterObjectSet{ + Spec: ocv1.ClusterObjectSetSpec{ + Phases: []ocv1.ClusterObjectSetPhase{{ + Name: "phase1", + Objects: []ocv1.ClusterObjectSetObject{{ + Ref: ocv1.ObjectSourceRef{ + Name: "test-secret", + Namespace: "test-ns", + Key: "obj", + }, + }}, + }}, + }, + } + + refs, err := reconciler.verifyReferencedSecrets(t.Context(), cos) + require.NoError(t, err) + require.Len(t, refs, 1) + assert.Equal(t, "test-ns/test-secret", refs[0].Name) + assert.NotEmpty(t, refs[0].Hash) + }) + + t.Run("succeeds when hash matches existing status", func(t *testing.T) { + secret := immutableSecret.DeepCopy() + testClient := fake.NewClientBuilder(). + WithScheme(testScheme). + WithObjects(secret). + Build() + + reconciler := &ClusterObjectSetReconciler{Client: testClient} + expectedHash := computeSecretHash(secret) + + cos := &ocv1.ClusterObjectSet{ + Spec: ocv1.ClusterObjectSetSpec{ + Phases: []ocv1.ClusterObjectSetPhase{{ + Name: "phase1", + Objects: []ocv1.ClusterObjectSetObject{{ + Ref: ocv1.ObjectSourceRef{ + Name: "test-secret", + Namespace: "test-ns", + Key: "obj", + }, + }}, + }}, + }, + Status: ocv1.ClusterObjectSetStatus{ + ObservedObjectContainers: []ocv1.ObservedObjectContainer{ + {Name: "test-ns/test-secret", Hash: expectedHash}, + }, + }, + } + + refs, err := reconciler.verifyReferencedSecrets(t.Context(), cos) + require.NoError(t, err) + require.Len(t, refs, 1) + assert.Equal(t, expectedHash, refs[0].Hash) + }) + + t.Run("rejects non-immutable secret", func(t *testing.T) { + mutableSecret := immutableSecret.DeepCopy() + mutableSecret.Immutable = nil + + testClient := fake.NewClientBuilder(). + WithScheme(testScheme). + WithObjects(mutableSecret). + Build() + + reconciler := &ClusterObjectSetReconciler{Client: testClient} + + cos := &ocv1.ClusterObjectSet{ + Spec: ocv1.ClusterObjectSetSpec{ + Phases: []ocv1.ClusterObjectSetPhase{{ + Name: "phase1", + Objects: []ocv1.ClusterObjectSetObject{{ + Ref: ocv1.ObjectSourceRef{ + Name: "test-secret", + Namespace: "test-ns", + Key: "obj", + }, + }}, + }}, + }, + } + + _, err := reconciler.verifyReferencedSecrets(t.Context(), cos) + require.Error(t, err) + assert.Contains(t, err.Error(), "not immutable") + }) + + t.Run("rejects secret with changed content hash", func(t *testing.T) { + secret := immutableSecret.DeepCopy() + testClient := fake.NewClientBuilder(). + WithScheme(testScheme). + WithObjects(secret). + Build() + + reconciler := &ClusterObjectSetReconciler{Client: testClient} + + cos := &ocv1.ClusterObjectSet{ + Spec: ocv1.ClusterObjectSetSpec{ + Phases: []ocv1.ClusterObjectSetPhase{{ + Name: "phase1", + Objects: []ocv1.ClusterObjectSetObject{{ + Ref: ocv1.ObjectSourceRef{ + Name: "test-secret", + Namespace: "test-ns", + Key: "obj", + }, + }}, + }}, + }, + Status: ocv1.ClusterObjectSetStatus{ + ObservedObjectContainers: []ocv1.ObservedObjectContainer{ + {Name: "test-ns/test-secret", Hash: "different-hash-value"}, + }, + }, + } + + _, err := reconciler.verifyReferencedSecrets(t.Context(), cos) + require.Error(t, err) + var sce *secretContentChangedError + require.ErrorAs(t, err, &sce) + assert.Contains(t, err.Error(), "content of referenced Secret") + }) + + t.Run("skips phases with inline objects only", func(t *testing.T) { + testClient := fake.NewClientBuilder(). + WithScheme(testScheme). + Build() + + reconciler := &ClusterObjectSetReconciler{Client: testClient} + + cos := &ocv1.ClusterObjectSet{ + Spec: ocv1.ClusterObjectSetSpec{ + Phases: []ocv1.ClusterObjectSetPhase{{ + Name: "phase1", + Objects: []ocv1.ClusterObjectSetObject{{ + // Inline object, no ref + }}, + }}, + }, + } + + refs, err := reconciler.verifyReferencedSecrets(t.Context(), cos) + require.NoError(t, err) + assert.Nil(t, refs) + }) + + t.Run("deduplicates refs to same secret", func(t *testing.T) { + secret := immutableSecret.DeepCopy() + testClient := fake.NewClientBuilder(). + WithScheme(testScheme). + WithObjects(secret). + Build() + + reconciler := &ClusterObjectSetReconciler{Client: testClient} + + cos := &ocv1.ClusterObjectSet{ + Spec: ocv1.ClusterObjectSetSpec{ + Phases: []ocv1.ClusterObjectSetPhase{{ + Name: "phase1", + Objects: []ocv1.ClusterObjectSetObject{ + {Ref: ocv1.ObjectSourceRef{Name: "test-secret", Namespace: "test-ns", Key: "obj"}}, + {Ref: ocv1.ObjectSourceRef{Name: "test-secret", Namespace: "test-ns", Key: "obj2"}}, + }, + }}, + }, + } + + refs, err := reconciler.verifyReferencedSecrets(t.Context(), cos) + require.NoError(t, err) + assert.Len(t, refs, 1) + }) +} diff --git a/internal/operator-controller/controllers/resolve_ref_test.go b/internal/operator-controller/controllers/resolve_ref_test.go index da3465ca7b..53e0df2c93 100644 --- a/internal/operator-controller/controllers/resolve_ref_test.go +++ b/internal/operator-controller/controllers/resolve_ref_test.go @@ -14,6 +14,7 @@ import ( apimachineryruntime "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" clocktesting "k8s.io/utils/clock/testing" + "k8s.io/utils/ptr" "pkg.package-operator.run/boxcutter/machinery" machinerytypes "pkg.package-operator.run/boxcutter/machinery/types" ctrl "sigs.k8s.io/controller-runtime" @@ -51,6 +52,7 @@ func TestResolveObjectRef_PlainJSON(t *testing.T) { Name: "test-secret", Namespace: "olmv1-system", }, + Immutable: ptr.To(true), Data: map[string][]byte{ "my-key": cmData, }, @@ -112,6 +114,7 @@ func TestResolveObjectRef_GzipCompressed(t *testing.T) { Name: "test-secret-gz", Namespace: "olmv1-system", }, + Immutable: ptr.To(true), Data: map[string][]byte{ "my-key": buf.Bytes(), }, @@ -184,6 +187,7 @@ func TestResolveObjectRef_KeyNotFound(t *testing.T) { Name: "test-secret-nokey", Namespace: "olmv1-system", }, + Immutable: ptr.To(true), Data: map[string][]byte{ "other-key": []byte("{}"), }, @@ -223,6 +227,7 @@ func TestResolveObjectRef_InvalidJSON(t *testing.T) { Name: "test-secret-invalid", Namespace: "olmv1-system", }, + Immutable: ptr.To(true), Data: map[string][]byte{ "my-key": []byte("not-valid-json"), }, diff --git a/manifests/experimental-e2e.yaml b/manifests/experimental-e2e.yaml index 4a27cf2056..32781b152b 100644 --- a/manifests/experimental-e2e.yaml +++ b/manifests/experimental-e2e.yaml @@ -1945,6 +1945,33 @@ spec: x-kubernetes-list-map-keys: - type x-kubernetes-list-type: map + observedObjectContainers: + description: |- + observedObjectContainers records the content hashes of referenced Secrets + at first successful resolution. This is used to detect if a referenced + Secret was deleted and recreated with different content. + items: + description: |- + ObservedObjectContainer records the observed state of a referenced Secret + used as an object source. + properties: + hash: + description: |- + hash is the hex-encoded SHA-256 hash of the Secret's .data field + at first successful resolution. + type: string + name: + description: name identifies the referenced Secret in "namespace/name" + format. + type: string + required: + - hash + - name + type: object + type: array + x-kubernetes-list-map-keys: + - name + x-kubernetes-list-type: map type: object type: object served: true diff --git a/manifests/experimental.yaml b/manifests/experimental.yaml index 9a8fa0b406..8272f4eb3a 100644 --- a/manifests/experimental.yaml +++ b/manifests/experimental.yaml @@ -1906,6 +1906,33 @@ spec: x-kubernetes-list-map-keys: - type x-kubernetes-list-type: map + observedObjectContainers: + description: |- + observedObjectContainers records the content hashes of referenced Secrets + at first successful resolution. This is used to detect if a referenced + Secret was deleted and recreated with different content. + items: + description: |- + ObservedObjectContainer records the observed state of a referenced Secret + used as an object source. + properties: + hash: + description: |- + hash is the hex-encoded SHA-256 hash of the Secret's .data field + at first successful resolution. + type: string + name: + description: name identifies the referenced Secret in "namespace/name" + format. + type: string + required: + - hash + - name + type: object + type: array + x-kubernetes-list-map-keys: + - name + x-kubernetes-list-type: map type: object type: object served: true diff --git a/test/e2e/features/revision.feature b/test/e2e/features/revision.feature index dd6d9e9407..ab6af3ae07 100644 --- a/test/e2e/features/revision.feature +++ b/test/e2e/features/revision.feature @@ -442,4 +442,135 @@ Feature: Install ClusterObjectSet Then ClusterObjectSet "${COS_NAME}" reports Progressing as True with Reason Succeeded And ClusterObjectSet "${COS_NAME}" reports Available as True with Reason ProbesSucceeded And resource "configmap/test-configmap-ref" is installed - And resource "deployment/test-httpd" is installed \ No newline at end of file + And resource "deployment/test-httpd" is installed + And ClusterObjectSet "${COS_NAME}" has observed object container "${TEST_NAMESPACE}/${COS_NAME}-ref-secret" with a non-empty hash + + Scenario: ClusterObjectSet blocks reconciliation when referenced Secret is not immutable + Given ServiceAccount "olm-sa" with needed permissions is available in test namespace + And resource is applied + """ + apiVersion: v1 + kind: Secret + metadata: + name: ${COS_NAME}-mutable-secret + namespace: ${TEST_NAMESPACE} + type: olm.operatorframework.io/object-data + stringData: + configmap: | + { + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": { + "name": "test-cm-mutable", + "namespace": "${TEST_NAMESPACE}" + }, + "data": { + "key": "value" + } + } + """ + When ClusterObjectSet is applied + """ + apiVersion: olm.operatorframework.io/v1 + kind: ClusterObjectSet + metadata: + annotations: + olm.operatorframework.io/service-account-name: olm-sa + olm.operatorframework.io/service-account-namespace: ${TEST_NAMESPACE} + name: ${COS_NAME} + spec: + lifecycleState: Active + collisionProtection: Prevent + phases: + - name: resources + objects: + - ref: + name: ${COS_NAME}-mutable-secret + namespace: ${TEST_NAMESPACE} + key: configmap + revision: 1 + """ + Then ClusterObjectSet "${COS_NAME}" reports Progressing as False with Reason Blocked and Message: + """ + secret ${TEST_NAMESPACE}/${COS_NAME}-mutable-secret is not immutable: referenced secrets must have immutable set to true + """ + + Scenario: ClusterObjectSet blocks reconciliation when referenced Secret content changes + Given ServiceAccount "olm-sa" with needed permissions is available in test namespace + When resource is applied + """ + apiVersion: v1 + kind: Secret + metadata: + name: ${COS_NAME}-change-secret + namespace: ${TEST_NAMESPACE} + immutable: true + type: olm.operatorframework.io/object-data + stringData: + configmap: | + { + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": { + "name": "test-cm-change", + "namespace": "${TEST_NAMESPACE}" + }, + "data": { + "key": "original-value" + } + } + """ + And ClusterObjectSet is applied + """ + apiVersion: olm.operatorframework.io/v1 + kind: ClusterObjectSet + metadata: + annotations: + olm.operatorframework.io/service-account-name: olm-sa + olm.operatorframework.io/service-account-namespace: ${TEST_NAMESPACE} + name: ${COS_NAME} + spec: + lifecycleState: Active + collisionProtection: Prevent + phases: + - name: resources + objects: + - ref: + name: ${COS_NAME}-change-secret + namespace: ${TEST_NAMESPACE} + key: configmap + revision: 1 + """ + Then ClusterObjectSet "${COS_NAME}" reports Progressing as True with Reason Succeeded + And ClusterObjectSet "${COS_NAME}" reports Available as True with Reason ProbesSucceeded + And ClusterObjectSet "${COS_NAME}" has observed object container "${TEST_NAMESPACE}/${COS_NAME}-change-secret" with a non-empty hash + # Delete the immutable Secret and recreate with different content + When resource "secret/${COS_NAME}-change-secret" is removed + And resource is applied + """ + apiVersion: v1 + kind: Secret + metadata: + name: ${COS_NAME}-change-secret + namespace: ${TEST_NAMESPACE} + immutable: true + type: olm.operatorframework.io/object-data + stringData: + configmap: | + { + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": { + "name": "test-cm-change", + "namespace": "${TEST_NAMESPACE}" + }, + "data": { + "key": "TAMPERED-value" + } + } + """ + And ClusterObjectSet "${COS_NAME}" reconciliation is triggered + Then ClusterObjectSet "${COS_NAME}" reports Progressing as False with Reason Blocked and Message includes: + """ + content of referenced Secret ${TEST_NAMESPACE}/${COS_NAME}-change-secret has changed + """ \ No newline at end of file diff --git a/test/e2e/steps/steps.go b/test/e2e/steps/steps.go index 2b5603ab79..b7e74f4c0e 100644 --- a/test/e2e/steps/steps.go +++ b/test/e2e/steps/steps.go @@ -90,8 +90,11 @@ func RegisterSteps(sc *godog.ScenarioContext) { sc.Step(`^(?i)ClusterExtension reports ([[:alnum:]]+) as ([[:alnum:]]+)$`, ClusterExtensionReportsConditionWithoutReason) sc.Step(`^(?i)ClusterObjectSet "([^"]+)" reports ([[:alnum:]]+) as ([[:alnum:]]+) with Reason ([[:alnum:]]+)$`, ClusterObjectSetReportsConditionWithoutMsg) sc.Step(`^(?i)ClusterObjectSet "([^"]+)" reports ([[:alnum:]]+) as ([[:alnum:]]+) with Reason ([[:alnum:]]+) and Message:$`, ClusterObjectSetReportsConditionWithMsg) + 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 "([^"]+)" reconciliation is triggered$`, TriggerClusterObjectSetReconciliation) + sc.Step(`^(?i)ClusterObjectSet "([^"]+)" has observed object container "([^"]+)" with a non-empty hash$`, ClusterObjectSetHasObservedObjectContainer) 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) @@ -597,6 +600,47 @@ func ClusterObjectSetReportsConditionWithMsg(ctx context.Context, revisionName, return waitForCondition(ctx, "clusterobjectset", substituteScenarioVars(revisionName, scenarioCtx(ctx)), conditionType, conditionStatus, &conditionReason, messageComparison(ctx, msg)) } +// ClusterObjectSetReportsConditionWithMessageFragment waits for the named ClusterObjectSet to have a condition +// matching type, status, reason, with a message containing the specified fragment. Polls with timeout. +func ClusterObjectSetReportsConditionWithMessageFragment(ctx context.Context, revisionName, conditionType, conditionStatus, conditionReason string, msgFragment *godog.DocString) error { + msgCmp := alwaysMatch + if msgFragment != nil { + expectedMsgFragment := substituteScenarioVars(strings.Join(strings.Fields(msgFragment.Content), " "), scenarioCtx(ctx)) + msgCmp = func(actualMsg string) bool { + return strings.Contains(actualMsg, expectedMsgFragment) + } + } + return waitForCondition(ctx, "clusterobjectset", substituteScenarioVars(revisionName, scenarioCtx(ctx)), conditionType, conditionStatus, &conditionReason, msgCmp) +} + +// TriggerClusterObjectSetReconciliation annotates the named ClusterObjectSet +// to trigger a new reconciliation cycle. +func TriggerClusterObjectSetReconciliation(ctx context.Context, cosName string) error { + sc := scenarioCtx(ctx) + cosName = substituteScenarioVars(cosName, sc) + _, err := k8sClient("annotate", "clusterobjectset", cosName, "--overwrite", + fmt.Sprintf("e2e-trigger=%d", time.Now().UnixNano())) + return err +} + +// ClusterObjectSetHasObservedObjectContainer waits for the named ClusterObjectSet to have +// an observedObjectContainers entry matching the given name with a non-empty hash. Polls with timeout. +func ClusterObjectSetHasObservedObjectContainer(ctx context.Context, cosName, expectedName string) error { + sc := scenarioCtx(ctx) + cosName = substituteScenarioVars(cosName, sc) + expectedName = substituteScenarioVars(expectedName, sc) + + waitFor(ctx, func() bool { + out, err := k8sClient("get", "clusterobjectset", cosName, "-o", + fmt.Sprintf(`jsonpath={.status.observedObjectContainers[?(@.name=="%s")].hash}`, expectedName)) + if err != nil { + return false + } + return strings.TrimSpace(out) != "" + }) + return nil +} + // ClusterObjectSetIsArchived waits for the named ClusterObjectSet to have Progressing=False // with reason Archived. Polls with timeout. func ClusterObjectSetIsArchived(ctx context.Context, revisionName string) error { From 3db997b23f324ef5916dd7cfa630aee9d5991e67 Mon Sep 17 00:00:00 2001 From: Predrag Knezevic Date: Thu, 9 Apr 2026 09:12:11 +0200 Subject: [PATCH 2/5] Replace per-Secret hashing with source-agnostic phase content hashing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses PR review feedback: - Compute SHA-256 hash per resolved phase (name + objects) instead of per-Secret, making immutability verification source-agnostic and futureproof for bundle refs. - Move hash computation after buildBoxcutterPhases succeeds so hashes are only persisted for successfully resolved content. - Remove Blocked reason from skipTerminallyBlockedPredicate so blocked COS can be re-reconciled when the underlying issue is fixed. API: rename ObservedObjectContainer → ObservedPhase, field observedObjectContainers → observedPhases in ClusterObjectSetStatus. Co-Authored-By: Claude Opus 4.6 --- api/v1/clusterobjectset_types.go | 23 +-- api/v1/zz_generated.deepcopy.go | 14 +- .../api/v1/clusterobjectsetstatus.go | 20 +- .../api/v1/observedobjectcontainer.go | 53 ------ applyconfigurations/api/v1/observedphase.go | 52 +++++ applyconfigurations/utils.go | 4 +- docs/api-reference/olmv1-api-reference.md | 2 + docs/draft/concepts/large-bundle-support.md | 19 +- ...peratorframework.io_clusterobjectsets.yaml | 26 +-- .../clusterobjectset_controller.go | 150 ++++++--------- ...usterobjectset_controller_internal_test.go | 180 ++++++++---------- manifests/experimental-e2e.yaml | 26 +-- manifests/experimental.yaml | 26 +-- test/e2e/features/revision.feature | 6 +- test/e2e/steps/steps.go | 12 +- 15 files changed, 278 insertions(+), 335 deletions(-) delete mode 100644 applyconfigurations/api/v1/observedobjectcontainer.go create mode 100644 applyconfigurations/api/v1/observedphase.go diff --git a/api/v1/clusterobjectset_types.go b/api/v1/clusterobjectset_types.go index 3ed14d2406..4b6e2b4953 100644 --- a/api/v1/clusterobjectset_types.go +++ b/api/v1/clusterobjectset_types.go @@ -511,29 +511,30 @@ type ClusterObjectSetStatus struct { // +optional Conditions []metav1.Condition `json:"conditions,omitempty"` - // observedObjectContainers records the content hashes of referenced Secrets - // at first successful resolution. This is used to detect if a referenced - // Secret was deleted and recreated with different content. + // observedPhases records the content hashes of resolved phases + // at first successful reconciliation. This is used to detect if + // referenced object sources were deleted and recreated with + // different content. Each entry covers all fully-resolved object + // manifests within a phase, making it source-agnostic. // // +listType=map // +listMapKey=name // +optional - ObservedObjectContainers []ObservedObjectContainer `json:"observedObjectContainers,omitempty"` + ObservedPhases []ObservedPhase `json:"observedPhases,omitempty"` } -// ObservedObjectContainer records the observed state of a referenced Secret -// used as an object source. -type ObservedObjectContainer struct { - // name identifies the referenced Secret in "namespace/name" format. +// ObservedPhase records the observed content digest of a resolved phase. +type ObservedPhase struct { + // name is the phase name matching a phase in spec.phases. // // +required Name string `json:"name"` - // hash is the hex-encoded SHA-256 hash of the Secret's .data field - // at first successful resolution. + // digest is the hex-encoded SHA-256 digest of the phase's resolved + // object content at first successful reconciliation. // // +required - Hash string `json:"hash"` + Digest string `json:"digest"` } // +genclient diff --git a/api/v1/zz_generated.deepcopy.go b/api/v1/zz_generated.deepcopy.go index ea50ebae21..384439787b 100644 --- a/api/v1/zz_generated.deepcopy.go +++ b/api/v1/zz_generated.deepcopy.go @@ -555,9 +555,9 @@ func (in *ClusterObjectSetStatus) DeepCopyInto(out *ClusterObjectSetStatus) { (*in)[i].DeepCopyInto(&(*out)[i]) } } - if in.ObservedObjectContainers != nil { - in, out := &in.ObservedObjectContainers, &out.ObservedObjectContainers - *out = make([]ObservedObjectContainer, len(*in)) + if in.ObservedPhases != nil { + in, out := &in.ObservedPhases, &out.ObservedPhases + *out = make([]ObservedPhase, len(*in)) copy(*out, *in) } } @@ -670,16 +670,16 @@ func (in *ObjectSourceRef) DeepCopy() *ObjectSourceRef { } // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *ObservedObjectContainer) DeepCopyInto(out *ObservedObjectContainer) { +func (in *ObservedPhase) DeepCopyInto(out *ObservedPhase) { *out = *in } -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ObservedObjectContainer. -func (in *ObservedObjectContainer) DeepCopy() *ObservedObjectContainer { +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ObservedPhase. +func (in *ObservedPhase) DeepCopy() *ObservedPhase { if in == nil { return nil } - out := new(ObservedObjectContainer) + out := new(ObservedPhase) in.DeepCopyInto(out) return out } diff --git a/applyconfigurations/api/v1/clusterobjectsetstatus.go b/applyconfigurations/api/v1/clusterobjectsetstatus.go index aa624f5059..6203563de7 100644 --- a/applyconfigurations/api/v1/clusterobjectsetstatus.go +++ b/applyconfigurations/api/v1/clusterobjectsetstatus.go @@ -46,10 +46,12 @@ type ClusterObjectSetStatusApplyConfiguration struct { // The Succeeded condition represents whether the revision has successfully completed its rollout: // - When status is True and reason is Succeeded, the ClusterObjectSet has successfully completed its rollout. This condition is set once and persists even if the revision later becomes unavailable. Conditions []metav1.ConditionApplyConfiguration `json:"conditions,omitempty"` - // observedObjectContainers records the content hashes of referenced Secrets - // at first successful resolution. This is used to detect if a referenced - // Secret was deleted and recreated with different content. - ObservedObjectContainers []ObservedObjectContainerApplyConfiguration `json:"observedObjectContainers,omitempty"` + // observedPhases records the content hashes of resolved phases + // at first successful reconciliation. This is used to detect if + // referenced object sources were deleted and recreated with + // different content. Each entry covers all fully-resolved object + // manifests within a phase, making it source-agnostic. + ObservedPhases []ObservedPhaseApplyConfiguration `json:"observedPhases,omitempty"` } // ClusterObjectSetStatusApplyConfiguration constructs a declarative configuration of the ClusterObjectSetStatus type for use with @@ -71,15 +73,15 @@ func (b *ClusterObjectSetStatusApplyConfiguration) WithConditions(values ...*met return b } -// WithObservedObjectContainers adds the given value to the ObservedObjectContainers field in the declarative configuration +// WithObservedPhases adds the given value to the ObservedPhases field in the declarative configuration // and returns the receiver, so that objects can be build by chaining "With" function invocations. -// If called multiple times, values provided by each call will be appended to the ObservedObjectContainers field. -func (b *ClusterObjectSetStatusApplyConfiguration) WithObservedObjectContainers(values ...*ObservedObjectContainerApplyConfiguration) *ClusterObjectSetStatusApplyConfiguration { +// If called multiple times, values provided by each call will be appended to the ObservedPhases field. +func (b *ClusterObjectSetStatusApplyConfiguration) WithObservedPhases(values ...*ObservedPhaseApplyConfiguration) *ClusterObjectSetStatusApplyConfiguration { for i := range values { if values[i] == nil { - panic("nil value passed to WithObservedObjectContainers") + panic("nil value passed to WithObservedPhases") } - b.ObservedObjectContainers = append(b.ObservedObjectContainers, *values[i]) + b.ObservedPhases = append(b.ObservedPhases, *values[i]) } return b } diff --git a/applyconfigurations/api/v1/observedobjectcontainer.go b/applyconfigurations/api/v1/observedobjectcontainer.go deleted file mode 100644 index b8098355fa..0000000000 --- a/applyconfigurations/api/v1/observedobjectcontainer.go +++ /dev/null @@ -1,53 +0,0 @@ -/* -Copyright 2022. - -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. -*/ -// Code generated by controller-gen-v0.20. DO NOT EDIT. - -package v1 - -// ObservedObjectContainerApplyConfiguration represents a declarative configuration of the ObservedObjectContainer type for use -// with apply. -// -// ObservedObjectContainer records the observed state of a referenced Secret -// used as an object source. -type ObservedObjectContainerApplyConfiguration struct { - // name identifies the referenced Secret in "namespace/name" format. - Name *string `json:"name,omitempty"` - // hash is the hex-encoded SHA-256 hash of the Secret's .data field - // at first successful resolution. - Hash *string `json:"hash,omitempty"` -} - -// ObservedObjectContainerApplyConfiguration constructs a declarative configuration of the ObservedObjectContainer type for use with -// apply. -func ObservedObjectContainer() *ObservedObjectContainerApplyConfiguration { - return &ObservedObjectContainerApplyConfiguration{} -} - -// WithName sets the Name 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 Name field is set to the value of the last call. -func (b *ObservedObjectContainerApplyConfiguration) WithName(value string) *ObservedObjectContainerApplyConfiguration { - b.Name = &value - return b -} - -// WithHash sets the Hash 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 Hash field is set to the value of the last call. -func (b *ObservedObjectContainerApplyConfiguration) WithHash(value string) *ObservedObjectContainerApplyConfiguration { - b.Hash = &value - return b -} diff --git a/applyconfigurations/api/v1/observedphase.go b/applyconfigurations/api/v1/observedphase.go new file mode 100644 index 0000000000..8a7db07d0b --- /dev/null +++ b/applyconfigurations/api/v1/observedphase.go @@ -0,0 +1,52 @@ +/* +Copyright 2022. + +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. +*/ +// Code generated by controller-gen-v0.20. DO NOT EDIT. + +package v1 + +// ObservedPhaseApplyConfiguration represents a declarative configuration of the ObservedPhase type for use +// with apply. +// +// ObservedPhase records the observed content digest of a resolved phase. +type ObservedPhaseApplyConfiguration struct { + // name is the phase name matching a phase in spec.phases. + Name *string `json:"name,omitempty"` + // digest is the hex-encoded SHA-256 digest of the phase's resolved + // object content at first successful reconciliation. + Digest *string `json:"digest,omitempty"` +} + +// ObservedPhaseApplyConfiguration constructs a declarative configuration of the ObservedPhase type for use with +// apply. +func ObservedPhase() *ObservedPhaseApplyConfiguration { + return &ObservedPhaseApplyConfiguration{} +} + +// WithName sets the Name 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 Name field is set to the value of the last call. +func (b *ObservedPhaseApplyConfiguration) WithName(value string) *ObservedPhaseApplyConfiguration { + b.Name = &value + return b +} + +// WithDigest sets the Digest 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 Digest field is set to the value of the last call. +func (b *ObservedPhaseApplyConfiguration) WithDigest(value string) *ObservedPhaseApplyConfiguration { + b.Digest = &value + return b +} diff --git a/applyconfigurations/utils.go b/applyconfigurations/utils.go index 1b18f7da36..6a467f96a6 100644 --- a/applyconfigurations/utils.go +++ b/applyconfigurations/utils.go @@ -83,8 +83,8 @@ func ForKind(kind schema.GroupVersionKind) interface{} { return &apiv1.ObjectSelectorApplyConfiguration{} case v1.SchemeGroupVersion.WithKind("ObjectSourceRef"): return &apiv1.ObjectSourceRefApplyConfiguration{} - case v1.SchemeGroupVersion.WithKind("ObservedObjectContainer"): - return &apiv1.ObservedObjectContainerApplyConfiguration{} + case v1.SchemeGroupVersion.WithKind("ObservedPhase"): + return &apiv1.ObservedPhaseApplyConfiguration{} case v1.SchemeGroupVersion.WithKind("PreflightConfig"): return &apiv1.PreflightConfigApplyConfiguration{} case v1.SchemeGroupVersion.WithKind("ProgressionProbe"): diff --git a/docs/api-reference/olmv1-api-reference.md b/docs/api-reference/olmv1-api-reference.md index 5b5eeb2361..332f5f0b9e 100644 --- a/docs/api-reference/olmv1-api-reference.md +++ b/docs/api-reference/olmv1-api-reference.md @@ -477,6 +477,8 @@ _Appears in:_ + + #### PreflightConfig diff --git a/docs/draft/concepts/large-bundle-support.md b/docs/draft/concepts/large-bundle-support.md index bfc053f11f..dba87b982f 100644 --- a/docs/draft/concepts/large-bundle-support.md +++ b/docs/draft/concepts/large-bundle-support.md @@ -151,9 +151,9 @@ Recommended conventions: that all referenced Secrets have `immutable: true` set before proceeding. Mutable referenced Secrets are rejected and reconciliation is blocked with `Progressing=False, Reason=Blocked`. Additionally, the reconciler records - content hashes of referenced Secrets on first resolution and blocks - reconciliation if the content changes (e.g., if a Secret is deleted and - recreated with the same name but different data). + content hashes of the resolved phases on first successful reconciliation + and blocks reconciliation if the content changes (e.g., if a Secret is + deleted and recreated with the same name but different data). 3. **Owner references**: Referenced Secrets should carry an ownerReference to the COS so that Kubernetes garbage collection removes them when the COS is @@ -392,12 +392,13 @@ Key properties: ### COS reconciler behavior -Before resolving individual object refs, the reconciler verifies all referenced -Secrets: each must have `immutable: true` set, and its content hash must match -the hash recorded in `.status.observedObjectContainers` (if present). If any Secret -fails verification, reconciliation is blocked with `Progressing=False, -Reason=Blocked`. On first successful verification, content hashes are persisted -to status for future comparisons. +Before resolving individual object refs, the reconciler verifies that all +referenced Secrets have `immutable: true` set. After successfully building +the phases (resolving all refs), the reconciler computes a per-phase content +digest and compares it against the digests recorded in `.status.observedPhases` +(if present). If any phase's content has changed, reconciliation is blocked +with `Progressing=False, Reason=Blocked`. On first successful build, phase +content digests are persisted to status for future comparisons. When processing a COS phase: - For each object entry in the phase: diff --git a/helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterobjectsets.yaml b/helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterobjectsets.yaml index 4d529a5d07..1ba720c8fc 100644 --- a/helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterobjectsets.yaml +++ b/helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterobjectsets.yaml @@ -621,27 +621,27 @@ spec: x-kubernetes-list-map-keys: - type x-kubernetes-list-type: map - observedObjectContainers: + observedPhases: description: |- - observedObjectContainers records the content hashes of referenced Secrets - at first successful resolution. This is used to detect if a referenced - Secret was deleted and recreated with different content. + observedPhases records the content hashes of resolved phases + at first successful reconciliation. This is used to detect if + referenced object sources were deleted and recreated with + different content. Each entry covers all fully-resolved object + manifests within a phase, making it source-agnostic. items: - description: |- - ObservedObjectContainer records the observed state of a referenced Secret - used as an object source. + description: ObservedPhase records the observed content digest of + a resolved phase. properties: - hash: + digest: description: |- - hash is the hex-encoded SHA-256 hash of the Secret's .data field - at first successful resolution. + digest is the hex-encoded SHA-256 digest of the phase's resolved + object content at first successful reconciliation. type: string name: - description: name identifies the referenced Secret in "namespace/name" - format. + description: name is the phase name matching a phase in spec.phases. type: string required: - - hash + - digest - name type: object type: array diff --git a/internal/operator-controller/controllers/clusterobjectset_controller.go b/internal/operator-controller/controllers/clusterobjectset_controller.go index 20eed825d0..e8816664f6 100644 --- a/internal/operator-controller/controllers/clusterobjectset_controller.go +++ b/internal/operator-controller/controllers/clusterobjectset_controller.go @@ -146,20 +146,11 @@ func (c *ClusterObjectSetReconciler) reconcile(ctx context.Context, cos *ocv1.Cl return c.delete(ctx, cos) } - resolvedHashes, err := c.verifyReferencedSecrets(ctx, cos) - if err != nil { - var sce *secretContentChangedError - if errors.As(err, &sce) { - l.Error(err, "referenced Secret content changed, blocking reconciliation") - } else { - l.Error(err, "referenced Secret verification failed, blocking reconciliation") - } + if err := c.verifyReferencedSecretsImmutable(ctx, cos); err != nil { + l.Error(err, "referenced Secret verification failed, blocking reconciliation") markAsNotProgressing(cos, ocv1.ClusterObjectSetReasonBlocked, err.Error()) return ctrl.Result{}, nil } - cos.Status.ObservedObjectContainers = mergeObservedObjectContainers( - cos.Status.ObservedObjectContainers, resolvedHashes, - ) phases, opts, err := c.buildBoxcutterPhases(ctx, cos) if err != nil { @@ -167,6 +158,24 @@ func (c *ClusterObjectSetReconciler) reconcile(ctx context.Context, cos *ocv1.Cl return ctrl.Result{}, fmt.Errorf("converting to boxcutter revision: %v", err) } + currentPhases := make([]ocv1.ObservedPhase, 0, len(phases)) + for _, phase := range phases { + hash, err := computePhaseDigest(phase) + if err != nil { + setRetryingConditions(cos, err.Error()) + return ctrl.Result{}, fmt.Errorf("computing phase digest: %v", err) + } + currentPhases = append(currentPhases, ocv1.ObservedPhase{Name: phase.GetName(), Digest: hash}) + } + + if len(cos.Status.ObservedPhases) == 0 { + cos.Status.ObservedPhases = currentPhases + } else if err := verifyObservedPhases(cos.Status.ObservedPhases, currentPhases); err != nil { + l.Error(err, "resolved phases content changed, blocking reconciliation") + markAsNotProgressing(cos, ocv1.ClusterObjectSetReasonBlocked, err.Error()) + return ctrl.Result{}, nil + } + revisionEngine, err := c.RevisionEngineFactory.CreateRevisionEngine(ctx, cos) if err != nil { setRetryingConditions(cos, err.Error()) @@ -361,7 +370,7 @@ func (c *ClusterObjectSetReconciler) SetupWithManager(mgr ctrl.Manager) error { return true } if cnd := meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterObjectSetTypeProgressing); cnd != nil && cnd.Status == metav1.ConditionFalse { - if cnd.Reason == ocv1.ReasonProgressDeadlineExceeded || cnd.Reason == ocv1.ClusterObjectSetReasonBlocked { + if cnd.Reason == ocv1.ReasonProgressDeadlineExceeded { return false } } @@ -713,62 +722,44 @@ func markAsArchived(cos *ocv1.ClusterObjectSet) bool { return markAsAvailableUnknown(cos, ocv1.ClusterObjectSetReasonArchived, msg) || updated } -// secretContentChangedError indicates that a referenced Secret's content -// has changed since it was first resolved (e.g., deleted and recreated -// with different data). -type secretContentChangedError struct { - secretKey string - expectedHash string - actualHash string -} - -func (e *secretContentChangedError) Error() string { - return fmt.Sprintf("content of referenced Secret %s has changed (expected hash %s, got %s): "+ - "the Secret may have been deleted and recreated with different content", - e.secretKey, e.expectedHash, e.actualHash) -} - -// computeSecretHash computes a deterministic SHA-256 hash of a Secret's -// .Data field. JSON serialization is used to produce a canonical encoding -// with sorted map keys. -func computeSecretHash(secret *corev1.Secret) string { - data, _ := json.Marshal(secret.Data) - sum := sha256.Sum256(data) - return fmt.Sprintf("%x", sum) +// computePhaseDigest computes a deterministic SHA-256 digest of a phase's +// resolved content (name + objects). JSON serialization of each object +// produces a canonical encoding with sorted map keys. +func computePhaseDigest(phase boxcutter.Phase) (string, error) { + h := sha256.New() + fmt.Fprintf(h, "phase:%s\n", phase.GetName()) + for _, obj := range phase.GetObjects() { + data, err := json.Marshal(obj) + if err != nil { + return "", fmt.Errorf("marshaling object in phase %q: %w", phase.GetName(), err) + } + h.Write(data) + } + return fmt.Sprintf("%x", h.Sum(nil)), nil } -// mergeObservedObjectContainers merges newly resolved containers into the -// existing list, adding entries that are not yet present. This ensures that -// late-arriving Secrets (e.g., not yet in the informer cache on first -// reconcile) get their hashes recorded on subsequent reconciles. -func mergeObservedObjectContainers(existing, resolved []ocv1.ObservedObjectContainer) []ocv1.ObservedObjectContainer { - if len(resolved) == 0 { - return existing - } - seen := make(map[string]struct{}, len(existing)) - for _, e := range existing { - seen[e.Name] = struct{}{} - } - merged := existing - for _, r := range resolved { - if _, ok := seen[r.Name]; !ok { - merged = append(merged, r) - seen[r.Name] = struct{}{} +// verifyObservedPhases compares current per-phase digests against stored +// digests. Returns an error naming the first mismatched phase. +func verifyObservedPhases(stored, current []ocv1.ObservedPhase) error { + storedMap := make(map[string]string, len(stored)) + for _, s := range stored { + storedMap[s.Name] = s.Digest + } + for _, c := range current { + if prev, ok := storedMap[c.Name]; ok && prev != c.Digest { + return fmt.Errorf( + "resolved content of phase %q has changed (expected digest %s, got %s): "+ + "a referenced object source may have been deleted and recreated with different content", + c.Name, prev, c.Digest) } } - return merged + return nil } -// verifyReferencedSecrets iterates over all Secret refs in the COS phases, -// fetches each unique Secret, and verifies: -// 1. The Secret has Immutable set to true. -// 2. The Secret's content hash matches the previously recorded hash (if any). -// -// On first successful verification (no existing hashes in status), it returns -// the computed hashes for the caller to persist. Returns a secretContentChangedError -// if a hash mismatch is detected. -func (c *ClusterObjectSetReconciler) verifyReferencedSecrets(ctx context.Context, cos *ocv1.ClusterObjectSet) ([]ocv1.ObservedObjectContainer, error) { - // Collect unique secret references +// verifyReferencedSecretsImmutable checks that all referenced Secrets +// have Immutable set to true. This is a fast-fail validation that +// provides a clear error message for misconfigured Secrets. +func (c *ClusterObjectSetReconciler) verifyReferencedSecretsImmutable(ctx context.Context, cos *ocv1.ClusterObjectSet) error { type secretRef struct { name string namespace string @@ -789,17 +780,6 @@ func (c *ClusterObjectSetReconciler) verifyReferencedSecrets(ctx context.Context } } - if len(refs) == 0 { - return nil, nil - } - - // Build a lookup map from existing observed refs - observedMap := make(map[string]string, len(cos.Status.ObservedObjectContainers)) - for _, observed := range cos.Status.ObservedObjectContainers { - observedMap[observed.Name] = observed.Hash - } - - var resolvedRefs []ocv1.ObservedObjectContainer for _, ref := range refs { secret := &corev1.Secret{} key := client.ObjectKey{Name: ref.name, Namespace: ref.namespace} @@ -809,31 +789,13 @@ func (c *ClusterObjectSetReconciler) verifyReferencedSecrets(ctx context.Context // resolveObjectRef will handle the not-found with a retryable error. continue } - return nil, fmt.Errorf("getting Secret %s/%s: %w", ref.namespace, ref.name, err) + return fmt.Errorf("getting Secret %s/%s: %w", ref.namespace, ref.name, err) } if secret.Immutable == nil || !*secret.Immutable { - return nil, fmt.Errorf("secret %s/%s is not immutable: referenced secrets must have immutable set to true", ref.namespace, ref.name) + return fmt.Errorf("secret %s/%s is not immutable: referenced secrets must have immutable set to true", ref.namespace, ref.name) } - - secretKey := fmt.Sprintf("%s/%s", ref.namespace, ref.name) - currentHash := computeSecretHash(secret) - - if previousHash, ok := observedMap[secretKey]; ok { - if currentHash != previousHash { - return nil, &secretContentChangedError{ - secretKey: secretKey, - expectedHash: previousHash, - actualHash: currentHash, - } - } - } - - resolvedRefs = append(resolvedRefs, ocv1.ObservedObjectContainer{ - Name: secretKey, - Hash: currentHash, - }) } - return resolvedRefs, nil + return nil } diff --git a/internal/operator-controller/controllers/clusterobjectset_controller_internal_test.go b/internal/operator-controller/controllers/clusterobjectset_controller_internal_test.go index 11e313e676..594a5034e6 100644 --- a/internal/operator-controller/controllers/clusterobjectset_controller_internal_test.go +++ b/internal/operator-controller/controllers/clusterobjectset_controller_internal_test.go @@ -9,11 +9,13 @@ import ( "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/runtime/schema" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/utils/ptr" + "pkg.package-operator.run/boxcutter" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" @@ -239,39 +241,90 @@ func (m *mockTrackingCacheInternal) Source(h handler.EventHandler, predicates .. return nil } -func TestComputeSecretHash(t *testing.T) { - t.Run("deterministic for same data", func(t *testing.T) { - secret := &corev1.Secret{ - Data: map[string][]byte{ - "key1": []byte("value1"), - "key2": []byte("value2"), +func TestComputePhaseDigest(t *testing.T) { + makePhase := func(name string, objs ...client.Object) boxcutter.Phase { + return boxcutter.NewPhase(name, objs) + } + + makeObj := func(apiVersion, kind, name string) *unstructured.Unstructured { + return &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": apiVersion, + "kind": kind, + "metadata": map[string]interface{}{"name": name}, }, } - hash1 := computeSecretHash(secret) - hash2 := computeSecretHash(secret) + } + + t.Run("deterministic for same content", func(t *testing.T) { + phase := makePhase("deploy", makeObj("v1", "ConfigMap", "cm1")) + hash1, err := computePhaseDigest(phase) + require.NoError(t, err) + hash2, err := computePhaseDigest(phase) + require.NoError(t, err) assert.Equal(t, hash1, hash2) }) - t.Run("different for different data", func(t *testing.T) { - s1 := &corev1.Secret{Data: map[string][]byte{"key": []byte("value1")}} - s2 := &corev1.Secret{Data: map[string][]byte{"key": []byte("value2")}} - assert.NotEqual(t, computeSecretHash(s1), computeSecretHash(s2)) + t.Run("different for different object content", func(t *testing.T) { + p1 := makePhase("deploy", makeObj("v1", "ConfigMap", "cm1")) + p2 := makePhase("deploy", makeObj("v1", "ConfigMap", "cm2")) + h1, err := computePhaseDigest(p1) + require.NoError(t, err) + h2, err := computePhaseDigest(p2) + require.NoError(t, err) + assert.NotEqual(t, h1, h2) }) - t.Run("different for different keys", func(t *testing.T) { - s1 := &corev1.Secret{Data: map[string][]byte{"key1": []byte("value")}} - s2 := &corev1.Secret{Data: map[string][]byte{"key2": []byte("value")}} - assert.NotEqual(t, computeSecretHash(s1), computeSecretHash(s2)) + t.Run("different for different phase names", func(t *testing.T) { + obj := makeObj("v1", "ConfigMap", "cm1") + p1 := makePhase("deploy", obj) + p2 := makePhase("crds", obj) + h1, err := computePhaseDigest(p1) + require.NoError(t, err) + h2, err := computePhaseDigest(p2) + require.NoError(t, err) + assert.NotEqual(t, h1, h2) }) - t.Run("empty data", func(t *testing.T) { - secret := &corev1.Secret{Data: map[string][]byte{}} - hash := computeSecretHash(secret) + t.Run("empty phase produces valid digest", func(t *testing.T) { + phase := makePhase("empty") + hash, err := computePhaseDigest(phase) + require.NoError(t, err) assert.NotEmpty(t, hash) }) } -func TestVerifyReferencedSecrets(t *testing.T) { +func TestVerifyObservedPhases(t *testing.T) { + t.Run("passes when digests match", func(t *testing.T) { + stored := []ocv1.ObservedPhase{{Name: "deploy", Digest: "abc123"}} + current := []ocv1.ObservedPhase{{Name: "deploy", Digest: "abc123"}} + assert.NoError(t, verifyObservedPhases(stored, current)) + }) + + t.Run("fails when digest changes", func(t *testing.T) { + stored := []ocv1.ObservedPhase{{Name: "deploy", Digest: "abc123"}} + current := []ocv1.ObservedPhase{{Name: "deploy", Digest: "def456"}} + err := verifyObservedPhases(stored, current) + require.Error(t, err) + assert.Contains(t, err.Error(), `resolved content of phase "deploy" has changed`) + }) + + t.Run("passes when current has new phase not in stored", func(t *testing.T) { + stored := []ocv1.ObservedPhase{{Name: "deploy", Digest: "abc123"}} + current := []ocv1.ObservedPhase{ + {Name: "deploy", Digest: "abc123"}, + {Name: "crds", Digest: "new-digest"}, + } + assert.NoError(t, verifyObservedPhases(stored, current)) + }) + + t.Run("passes with empty stored", func(t *testing.T) { + current := []ocv1.ObservedPhase{{Name: "deploy", Digest: "abc123"}} + assert.NoError(t, verifyObservedPhases(nil, current)) + }) +} + +func TestVerifyReferencedSecretsImmutable(t *testing.T) { testScheme := runtime.NewScheme() require.NoError(t, ocv1.AddToScheme(testScheme)) require.NoError(t, corev1.AddToScheme(testScheme)) @@ -287,7 +340,7 @@ func TestVerifyReferencedSecrets(t *testing.T) { }, } - t.Run("succeeds on first resolution with no existing hashes", func(t *testing.T) { + t.Run("succeeds when all secrets are immutable", func(t *testing.T) { testClient := fake.NewClientBuilder(). WithScheme(testScheme). WithObjects(immutableSecret.DeepCopy()). @@ -310,47 +363,8 @@ func TestVerifyReferencedSecrets(t *testing.T) { }, } - refs, err := reconciler.verifyReferencedSecrets(t.Context(), cos) - require.NoError(t, err) - require.Len(t, refs, 1) - assert.Equal(t, "test-ns/test-secret", refs[0].Name) - assert.NotEmpty(t, refs[0].Hash) - }) - - t.Run("succeeds when hash matches existing status", func(t *testing.T) { - secret := immutableSecret.DeepCopy() - testClient := fake.NewClientBuilder(). - WithScheme(testScheme). - WithObjects(secret). - Build() - - reconciler := &ClusterObjectSetReconciler{Client: testClient} - expectedHash := computeSecretHash(secret) - - cos := &ocv1.ClusterObjectSet{ - Spec: ocv1.ClusterObjectSetSpec{ - Phases: []ocv1.ClusterObjectSetPhase{{ - Name: "phase1", - Objects: []ocv1.ClusterObjectSetObject{{ - Ref: ocv1.ObjectSourceRef{ - Name: "test-secret", - Namespace: "test-ns", - Key: "obj", - }, - }}, - }}, - }, - Status: ocv1.ClusterObjectSetStatus{ - ObservedObjectContainers: []ocv1.ObservedObjectContainer{ - {Name: "test-ns/test-secret", Hash: expectedHash}, - }, - }, - } - - refs, err := reconciler.verifyReferencedSecrets(t.Context(), cos) + err := reconciler.verifyReferencedSecretsImmutable(t.Context(), cos) require.NoError(t, err) - require.Len(t, refs, 1) - assert.Equal(t, expectedHash, refs[0].Hash) }) t.Run("rejects non-immutable secret", func(t *testing.T) { @@ -379,47 +393,11 @@ func TestVerifyReferencedSecrets(t *testing.T) { }, } - _, err := reconciler.verifyReferencedSecrets(t.Context(), cos) + err := reconciler.verifyReferencedSecretsImmutable(t.Context(), cos) require.Error(t, err) assert.Contains(t, err.Error(), "not immutable") }) - t.Run("rejects secret with changed content hash", func(t *testing.T) { - secret := immutableSecret.DeepCopy() - testClient := fake.NewClientBuilder(). - WithScheme(testScheme). - WithObjects(secret). - Build() - - reconciler := &ClusterObjectSetReconciler{Client: testClient} - - cos := &ocv1.ClusterObjectSet{ - Spec: ocv1.ClusterObjectSetSpec{ - Phases: []ocv1.ClusterObjectSetPhase{{ - Name: "phase1", - Objects: []ocv1.ClusterObjectSetObject{{ - Ref: ocv1.ObjectSourceRef{ - Name: "test-secret", - Namespace: "test-ns", - Key: "obj", - }, - }}, - }}, - }, - Status: ocv1.ClusterObjectSetStatus{ - ObservedObjectContainers: []ocv1.ObservedObjectContainer{ - {Name: "test-ns/test-secret", Hash: "different-hash-value"}, - }, - }, - } - - _, err := reconciler.verifyReferencedSecrets(t.Context(), cos) - require.Error(t, err) - var sce *secretContentChangedError - require.ErrorAs(t, err, &sce) - assert.Contains(t, err.Error(), "content of referenced Secret") - }) - t.Run("skips phases with inline objects only", func(t *testing.T) { testClient := fake.NewClientBuilder(). WithScheme(testScheme). @@ -438,9 +416,8 @@ func TestVerifyReferencedSecrets(t *testing.T) { }, } - refs, err := reconciler.verifyReferencedSecrets(t.Context(), cos) + err := reconciler.verifyReferencedSecretsImmutable(t.Context(), cos) require.NoError(t, err) - assert.Nil(t, refs) }) t.Run("deduplicates refs to same secret", func(t *testing.T) { @@ -464,8 +441,7 @@ func TestVerifyReferencedSecrets(t *testing.T) { }, } - refs, err := reconciler.verifyReferencedSecrets(t.Context(), cos) + err := reconciler.verifyReferencedSecretsImmutable(t.Context(), cos) require.NoError(t, err) - assert.Len(t, refs, 1) }) } diff --git a/manifests/experimental-e2e.yaml b/manifests/experimental-e2e.yaml index 32781b152b..5273072d67 100644 --- a/manifests/experimental-e2e.yaml +++ b/manifests/experimental-e2e.yaml @@ -1945,27 +1945,27 @@ spec: x-kubernetes-list-map-keys: - type x-kubernetes-list-type: map - observedObjectContainers: + observedPhases: description: |- - observedObjectContainers records the content hashes of referenced Secrets - at first successful resolution. This is used to detect if a referenced - Secret was deleted and recreated with different content. + observedPhases records the content hashes of resolved phases + at first successful reconciliation. This is used to detect if + referenced object sources were deleted and recreated with + different content. Each entry covers all fully-resolved object + manifests within a phase, making it source-agnostic. items: - description: |- - ObservedObjectContainer records the observed state of a referenced Secret - used as an object source. + description: ObservedPhase records the observed content digest of + a resolved phase. properties: - hash: + digest: description: |- - hash is the hex-encoded SHA-256 hash of the Secret's .data field - at first successful resolution. + digest is the hex-encoded SHA-256 digest of the phase's resolved + object content at first successful reconciliation. type: string name: - description: name identifies the referenced Secret in "namespace/name" - format. + description: name is the phase name matching a phase in spec.phases. type: string required: - - hash + - digest - name type: object type: array diff --git a/manifests/experimental.yaml b/manifests/experimental.yaml index 8272f4eb3a..f5fdc2314e 100644 --- a/manifests/experimental.yaml +++ b/manifests/experimental.yaml @@ -1906,27 +1906,27 @@ spec: x-kubernetes-list-map-keys: - type x-kubernetes-list-type: map - observedObjectContainers: + observedPhases: description: |- - observedObjectContainers records the content hashes of referenced Secrets - at first successful resolution. This is used to detect if a referenced - Secret was deleted and recreated with different content. + observedPhases records the content hashes of resolved phases + at first successful reconciliation. This is used to detect if + referenced object sources were deleted and recreated with + different content. Each entry covers all fully-resolved object + manifests within a phase, making it source-agnostic. items: - description: |- - ObservedObjectContainer records the observed state of a referenced Secret - used as an object source. + description: ObservedPhase records the observed content digest of + a resolved phase. properties: - hash: + digest: description: |- - hash is the hex-encoded SHA-256 hash of the Secret's .data field - at first successful resolution. + digest is the hex-encoded SHA-256 digest of the phase's resolved + object content at first successful reconciliation. type: string name: - description: name identifies the referenced Secret in "namespace/name" - format. + description: name is the phase name matching a phase in spec.phases. type: string required: - - hash + - digest - name type: object type: array diff --git a/test/e2e/features/revision.feature b/test/e2e/features/revision.feature index ab6af3ae07..f7347242f4 100644 --- a/test/e2e/features/revision.feature +++ b/test/e2e/features/revision.feature @@ -443,7 +443,7 @@ Feature: Install ClusterObjectSet And ClusterObjectSet "${COS_NAME}" reports Available as True with Reason ProbesSucceeded And resource "configmap/test-configmap-ref" is installed And resource "deployment/test-httpd" is installed - And ClusterObjectSet "${COS_NAME}" has observed object container "${TEST_NAMESPACE}/${COS_NAME}-ref-secret" with a non-empty hash + And ClusterObjectSet "${COS_NAME}" has observed phase "resources" with a non-empty digest Scenario: ClusterObjectSet blocks reconciliation when referenced Secret is not immutable Given ServiceAccount "olm-sa" with needed permissions is available in test namespace @@ -543,7 +543,7 @@ Feature: Install ClusterObjectSet """ Then ClusterObjectSet "${COS_NAME}" reports Progressing as True with Reason Succeeded And ClusterObjectSet "${COS_NAME}" reports Available as True with Reason ProbesSucceeded - And ClusterObjectSet "${COS_NAME}" has observed object container "${TEST_NAMESPACE}/${COS_NAME}-change-secret" with a non-empty hash + And ClusterObjectSet "${COS_NAME}" has observed phase "resources" with a non-empty digest # Delete the immutable Secret and recreate with different content When resource "secret/${COS_NAME}-change-secret" is removed And resource is applied @@ -572,5 +572,5 @@ Feature: Install ClusterObjectSet And ClusterObjectSet "${COS_NAME}" reconciliation is triggered Then ClusterObjectSet "${COS_NAME}" reports Progressing as False with Reason Blocked and Message includes: """ - content of referenced Secret ${TEST_NAMESPACE}/${COS_NAME}-change-secret has changed + resolved content of phase "resources" has changed """ \ No newline at end of file diff --git a/test/e2e/steps/steps.go b/test/e2e/steps/steps.go index b7e74f4c0e..1107514bd8 100644 --- a/test/e2e/steps/steps.go +++ b/test/e2e/steps/steps.go @@ -94,7 +94,7 @@ func RegisterSteps(sc *godog.ScenarioContext) { 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 "([^"]+)" reconciliation is triggered$`, TriggerClusterObjectSetReconciliation) - sc.Step(`^(?i)ClusterObjectSet "([^"]+)" has observed object container "([^"]+)" with a non-empty hash$`, ClusterObjectSetHasObservedObjectContainer) + 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) @@ -623,16 +623,16 @@ func TriggerClusterObjectSetReconciliation(ctx context.Context, cosName string) return err } -// ClusterObjectSetHasObservedObjectContainer waits for the named ClusterObjectSet to have -// an observedObjectContainers entry matching the given name with a non-empty hash. Polls with timeout. -func ClusterObjectSetHasObservedObjectContainer(ctx context.Context, cosName, expectedName string) error { +// ClusterObjectSetHasObservedPhase waits for the named ClusterObjectSet to have +// an observedPhases entry matching the given phase name with a non-empty digest. Polls with timeout. +func ClusterObjectSetHasObservedPhase(ctx context.Context, cosName, phaseName string) error { sc := scenarioCtx(ctx) cosName = substituteScenarioVars(cosName, sc) - expectedName = substituteScenarioVars(expectedName, sc) + phaseName = substituteScenarioVars(phaseName, sc) waitFor(ctx, func() bool { out, err := k8sClient("get", "clusterobjectset", cosName, "-o", - fmt.Sprintf(`jsonpath={.status.observedObjectContainers[?(@.name=="%s")].hash}`, expectedName)) + fmt.Sprintf(`jsonpath={.status.observedPhases[?(@.name=="%s")].digest}`, phaseName)) if err != nil { return false } From eb25334dbe6a9654aa84a3569ece07b2896091f7 Mon Sep 17 00:00:00 2001 From: Predrag Knezevic Date: Thu, 9 Apr 2026 18:05:11 +0200 Subject: [PATCH 3/5] Address review feedback: delimiter in digest, wording fix, whitespace normalization MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add newline delimiter between objects in computePhaseDigest to prevent ambiguous concatenation encodings - Fix API doc: "first successful reconciliation" → "first successful resolution" to match actual behavior - Normalize whitespace in actual condition messages in e2e message fragment comparisons to reduce flakiness - Revert predicate rename to skipProgressDeadlineExceededPredicate Co-Authored-By: Claude Opus 4.6 --- api/v1/clusterobjectset_types.go | 12 +++++-- applyconfigurations/api/v1/observedphase.go | 4 +-- .../crd-ref-docs-gen-config.yaml | 2 +- docs/api-reference/olmv1-api-reference.md | 2 -- ...peratorframework.io_clusterobjectsets.yaml | 20 ++++++++++-- .../clusterobjectset_controller.go | 32 +++++++++---------- ...usterobjectset_controller_internal_test.go | 24 +++++++++----- manifests/experimental-e2e.yaml | 20 ++++++++++-- manifests/experimental.yaml | 20 ++++++++++-- test/e2e/steps/steps.go | 6 ++-- 10 files changed, 102 insertions(+), 40 deletions(-) diff --git a/api/v1/clusterobjectset_types.go b/api/v1/clusterobjectset_types.go index 4b6e2b4953..d7e755992c 100644 --- a/api/v1/clusterobjectset_types.go +++ b/api/v1/clusterobjectset_types.go @@ -517,6 +517,8 @@ type ClusterObjectSetStatus struct { // different content. Each entry covers all fully-resolved object // manifests within a phase, making it source-agnostic. // + // +kubebuilder:validation:XValidation:rule="self == oldSelf || oldSelf.size() == 0",message="observedPhases is immutable" + // +kubebuilder:validation:MaxItems=20 // +listType=map // +listMapKey=name // +optional @@ -528,12 +530,18 @@ type ObservedPhase struct { // name is the phase name matching a phase in spec.phases. // // +required + // +kubebuilder:validation:MinLength=1 + // +kubebuilder:validation:MaxLength=63 + // +kubebuilder:validation:XValidation:rule=`!format.dns1123Label().validate(self).hasValue()`,message="the value must consist of only lowercase alphanumeric characters and hyphens, and must start with an alphabetic character and end with an alphanumeric character." Name string `json:"name"` - // digest is the hex-encoded SHA-256 digest of the phase's resolved - // object content at first successful reconciliation. + // digest is the digest of the phase's resolved object content + // at first successful resolution, in the format ":". // // +required + // +kubebuilder:validation:MinLength=1 + // +kubebuilder:validation:MaxLength=256 + // +kubebuilder:validation:XValidation:rule=`self.matches('^[a-z0-9]+:[a-f0-9]+$')`,message="digest must be in the format ':'" Digest string `json:"digest"` } diff --git a/applyconfigurations/api/v1/observedphase.go b/applyconfigurations/api/v1/observedphase.go index 8a7db07d0b..75f5fb1d60 100644 --- a/applyconfigurations/api/v1/observedphase.go +++ b/applyconfigurations/api/v1/observedphase.go @@ -24,8 +24,8 @@ package v1 type ObservedPhaseApplyConfiguration struct { // name is the phase name matching a phase in spec.phases. Name *string `json:"name,omitempty"` - // digest is the hex-encoded SHA-256 digest of the phase's resolved - // object content at first successful reconciliation. + // digest is the digest of the phase's resolved object content + // at first successful resolution, in the format ":". Digest *string `json:"digest,omitempty"` } diff --git a/docs/api-reference/crd-ref-docs-gen-config.yaml b/docs/api-reference/crd-ref-docs-gen-config.yaml index d7eb1e4e33..c42240a46c 100644 --- a/docs/api-reference/crd-ref-docs-gen-config.yaml +++ b/docs/api-reference/crd-ref-docs-gen-config.yaml @@ -1,5 +1,5 @@ processor: - ignoreTypes: [ClusterObjectSet, ClusterObjectSetList] + ignoreTypes: [ClusterObjectSet, ClusterObjectSetList, ObservedPhase] ignoreFields: [] render: diff --git a/docs/api-reference/olmv1-api-reference.md b/docs/api-reference/olmv1-api-reference.md index 332f5f0b9e..5b5eeb2361 100644 --- a/docs/api-reference/olmv1-api-reference.md +++ b/docs/api-reference/olmv1-api-reference.md @@ -477,8 +477,6 @@ _Appears in:_ - - #### PreflightConfig diff --git a/helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterobjectsets.yaml b/helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterobjectsets.yaml index 1ba720c8fc..19c53bad9e 100644 --- a/helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterobjectsets.yaml +++ b/helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterobjectsets.yaml @@ -634,20 +634,36 @@ spec: properties: digest: description: |- - digest is the hex-encoded SHA-256 digest of the phase's resolved - object content at first successful reconciliation. + digest is the digest of the phase's resolved object content + at first successful resolution, in the format ":". + maxLength: 256 + minLength: 1 type: string + x-kubernetes-validations: + - message: digest must be in the format ':' + rule: self.matches('^[a-z0-9]+:[a-f0-9]+$') name: description: name is the phase name matching a phase in spec.phases. + maxLength: 63 + minLength: 1 type: string + x-kubernetes-validations: + - message: the value must consist of only lowercase alphanumeric + characters and hyphens, and must start with an alphabetic + character and end with an alphanumeric character. + rule: '!format.dns1123Label().validate(self).hasValue()' required: - digest - name type: object + maxItems: 20 type: array x-kubernetes-list-map-keys: - name x-kubernetes-list-type: map + x-kubernetes-validations: + - message: observedPhases is immutable + rule: self == oldSelf || oldSelf.size() == 0 type: object type: object served: true diff --git a/internal/operator-controller/controllers/clusterobjectset_controller.go b/internal/operator-controller/controllers/clusterobjectset_controller.go index e8816664f6..5722c2a013 100644 --- a/internal/operator-controller/controllers/clusterobjectset_controller.go +++ b/internal/operator-controller/controllers/clusterobjectset_controller.go @@ -359,7 +359,7 @@ type Sourcoser interface { } func (c *ClusterObjectSetReconciler) SetupWithManager(mgr ctrl.Manager) error { - skipTerminallyBlockedPredicate := predicate.Funcs{ + skipProgressDeadlineExceededPredicate := predicate.Funcs{ UpdateFunc: func(e event.UpdateEvent) bool { rev, ok := e.ObjectNew.(*ocv1.ClusterObjectSet) if !ok { @@ -369,10 +369,8 @@ func (c *ClusterObjectSetReconciler) SetupWithManager(mgr ctrl.Manager) error { if !rev.DeletionTimestamp.IsZero() { return true } - if cnd := meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterObjectSetTypeProgressing); cnd != nil && cnd.Status == metav1.ConditionFalse { - if cnd.Reason == ocv1.ReasonProgressDeadlineExceeded { - return false - } + if cnd := meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterObjectSetTypeProgressing); cnd != nil && cnd.Status == metav1.ConditionFalse && cnd.Reason == ocv1.ReasonProgressDeadlineExceeded { + return false } return true }, @@ -383,7 +381,7 @@ func (c *ClusterObjectSetReconciler) SetupWithManager(mgr ctrl.Manager) error { &ocv1.ClusterObjectSet{}, builder.WithPredicates( predicate.ResourceVersionChangedPredicate{}, - skipTerminallyBlockedPredicate, + skipProgressDeadlineExceededPredicate, ), ). WatchesRawSource( @@ -723,19 +721,19 @@ func markAsArchived(cos *ocv1.ClusterObjectSet) bool { } // computePhaseDigest computes a deterministic SHA-256 digest of a phase's -// resolved content (name + objects). JSON serialization of each object -// produces a canonical encoding with sorted map keys. +// resolved content (name + objects). JSON serialization of unstructured +// objects produces a canonical encoding with sorted map keys. func computePhaseDigest(phase boxcutter.Phase) (string, error) { - h := sha256.New() - fmt.Fprintf(h, "phase:%s\n", phase.GetName()) - for _, obj := range phase.GetObjects() { - data, err := json.Marshal(obj) - if err != nil { - return "", fmt.Errorf("marshaling object in phase %q: %w", phase.GetName(), err) - } - h.Write(data) + phaseMap := map[string]any{ + "name": phase.GetName(), + "objects": phase.GetObjects(), + } + data, err := json.Marshal(phaseMap) + if err != nil { + return "", fmt.Errorf("marshaling phase %q: %w", phase.GetName(), err) } - return fmt.Sprintf("%x", h.Sum(nil)), nil + h := sha256.Sum256(data) + return fmt.Sprintf("sha256:%x", h), nil } // verifyObservedPhases compares current per-phase digests against stored diff --git a/internal/operator-controller/controllers/clusterobjectset_controller_internal_test.go b/internal/operator-controller/controllers/clusterobjectset_controller_internal_test.go index 594a5034e6..711abf0b2e 100644 --- a/internal/operator-controller/controllers/clusterobjectset_controller_internal_test.go +++ b/internal/operator-controller/controllers/clusterobjectset_controller_internal_test.go @@ -2,6 +2,7 @@ package controllers import ( "context" + "strings" "testing" "time" @@ -292,34 +293,41 @@ func TestComputePhaseDigest(t *testing.T) { require.NoError(t, err) assert.NotEmpty(t, hash) }) + + t.Run("digest has sha256 prefix", func(t *testing.T) { + phase := makePhase("deploy", makeObj("v1", "ConfigMap", "cm1")) + digest, err := computePhaseDigest(phase) + require.NoError(t, err) + assert.True(t, strings.HasPrefix(digest, "sha256:"), "digest should start with sha256: prefix, got %s", digest) + }) } func TestVerifyObservedPhases(t *testing.T) { t.Run("passes when digests match", func(t *testing.T) { - stored := []ocv1.ObservedPhase{{Name: "deploy", Digest: "abc123"}} - current := []ocv1.ObservedPhase{{Name: "deploy", Digest: "abc123"}} + stored := []ocv1.ObservedPhase{{Name: "deploy", Digest: "sha256:abc123"}} + current := []ocv1.ObservedPhase{{Name: "deploy", Digest: "sha256:abc123"}} assert.NoError(t, verifyObservedPhases(stored, current)) }) t.Run("fails when digest changes", func(t *testing.T) { - stored := []ocv1.ObservedPhase{{Name: "deploy", Digest: "abc123"}} - current := []ocv1.ObservedPhase{{Name: "deploy", Digest: "def456"}} + stored := []ocv1.ObservedPhase{{Name: "deploy", Digest: "sha256:abc123"}} + current := []ocv1.ObservedPhase{{Name: "deploy", Digest: "sha256:def456"}} err := verifyObservedPhases(stored, current) require.Error(t, err) assert.Contains(t, err.Error(), `resolved content of phase "deploy" has changed`) }) t.Run("passes when current has new phase not in stored", func(t *testing.T) { - stored := []ocv1.ObservedPhase{{Name: "deploy", Digest: "abc123"}} + stored := []ocv1.ObservedPhase{{Name: "deploy", Digest: "sha256:abc123"}} current := []ocv1.ObservedPhase{ - {Name: "deploy", Digest: "abc123"}, - {Name: "crds", Digest: "new-digest"}, + {Name: "deploy", Digest: "sha256:abc123"}, + {Name: "crds", Digest: "sha256:def456"}, } assert.NoError(t, verifyObservedPhases(stored, current)) }) t.Run("passes with empty stored", func(t *testing.T) { - current := []ocv1.ObservedPhase{{Name: "deploy", Digest: "abc123"}} + current := []ocv1.ObservedPhase{{Name: "deploy", Digest: "sha256:abc123"}} assert.NoError(t, verifyObservedPhases(nil, current)) }) } diff --git a/manifests/experimental-e2e.yaml b/manifests/experimental-e2e.yaml index 5273072d67..f52b31a39d 100644 --- a/manifests/experimental-e2e.yaml +++ b/manifests/experimental-e2e.yaml @@ -1958,20 +1958,36 @@ spec: properties: digest: description: |- - digest is the hex-encoded SHA-256 digest of the phase's resolved - object content at first successful reconciliation. + digest is the digest of the phase's resolved object content + at first successful resolution, in the format ":". + maxLength: 256 + minLength: 1 type: string + x-kubernetes-validations: + - message: digest must be in the format ':' + rule: self.matches('^[a-z0-9]+:[a-f0-9]+$') name: description: name is the phase name matching a phase in spec.phases. + maxLength: 63 + minLength: 1 type: string + x-kubernetes-validations: + - message: the value must consist of only lowercase alphanumeric + characters and hyphens, and must start with an alphabetic + character and end with an alphanumeric character. + rule: '!format.dns1123Label().validate(self).hasValue()' required: - digest - name type: object + maxItems: 20 type: array x-kubernetes-list-map-keys: - name x-kubernetes-list-type: map + x-kubernetes-validations: + - message: observedPhases is immutable + rule: self == oldSelf || oldSelf.size() == 0 type: object type: object served: true diff --git a/manifests/experimental.yaml b/manifests/experimental.yaml index f5fdc2314e..b8e48ba2f8 100644 --- a/manifests/experimental.yaml +++ b/manifests/experimental.yaml @@ -1919,20 +1919,36 @@ spec: properties: digest: description: |- - digest is the hex-encoded SHA-256 digest of the phase's resolved - object content at first successful reconciliation. + digest is the digest of the phase's resolved object content + at first successful resolution, in the format ":". + maxLength: 256 + minLength: 1 type: string + x-kubernetes-validations: + - message: digest must be in the format ':' + rule: self.matches('^[a-z0-9]+:[a-f0-9]+$') name: description: name is the phase name matching a phase in spec.phases. + maxLength: 63 + minLength: 1 type: string + x-kubernetes-validations: + - message: the value must consist of only lowercase alphanumeric + characters and hyphens, and must start with an alphabetic + character and end with an alphanumeric character. + rule: '!format.dns1123Label().validate(self).hasValue()' required: - digest - name type: object + maxItems: 20 type: array x-kubernetes-list-map-keys: - name x-kubernetes-list-type: map + x-kubernetes-validations: + - message: observedPhases is immutable + rule: self == oldSelf || oldSelf.size() == 0 type: object type: object served: true diff --git a/test/e2e/steps/steps.go b/test/e2e/steps/steps.go index 1107514bd8..c9748d99d3 100644 --- a/test/e2e/steps/steps.go +++ b/test/e2e/steps/steps.go @@ -511,7 +511,8 @@ func ClusterExtensionReportsConditionWithMessageFragment(ctx context.Context, co if msgFragment != nil { expectedMsgFragment := substituteScenarioVars(strings.Join(strings.Fields(msgFragment.Content), " "), scenarioCtx(ctx)) msgCmp = func(actualMsg string) bool { - return strings.Contains(actualMsg, expectedMsgFragment) + normalizedActual := strings.Join(strings.Fields(actualMsg), " ") + return strings.Contains(normalizedActual, expectedMsgFragment) } } return waitForExtensionCondition(ctx, conditionType, conditionStatus, &conditionReason, msgCmp) @@ -607,7 +608,8 @@ func ClusterObjectSetReportsConditionWithMessageFragment(ctx context.Context, re if msgFragment != nil { expectedMsgFragment := substituteScenarioVars(strings.Join(strings.Fields(msgFragment.Content), " "), scenarioCtx(ctx)) msgCmp = func(actualMsg string) bool { - return strings.Contains(actualMsg, expectedMsgFragment) + normalizedActual := strings.Join(strings.Fields(actualMsg), " ") + return strings.Contains(normalizedActual, expectedMsgFragment) } } return waitForCondition(ctx, "clusterobjectset", substituteScenarioVars(revisionName, scenarioCtx(ctx)), conditionType, conditionStatus, &conditionReason, msgCmp) From 1224ca301ca23eb8bd243860a855f709d7d4b928 Mon Sep 17 00:00:00 2001 From: Predrag Knezevic Date: Sat, 11 Apr 2026 01:59:38 +0200 Subject: [PATCH 4/5] Address review feedback: pre-mutation hashing, collect all errors, recovery scenario - Move phase digest computation before controller mutations in buildBoxcutterPhases - Collect all mismatched phases in verifyObservedPhases instead of failing on first - Collect all mutable secrets in verifyReferencedSecretsImmutable - Add determinism and multi-mismatch tests, empty stored error check - Fix e2e scenario title double negative, add recovery scenario - Rename dedup test for clarity Co-Authored-By: Claude Opus 4.6 --- .../clusterobjectset_controller.go | 88 +++++++++++------- ...usterobjectset_controller_internal_test.go | 92 +++++++++++++------ test/e2e/features/revision.feature | 33 ++++++- 3 files changed, 149 insertions(+), 64 deletions(-) diff --git a/internal/operator-controller/controllers/clusterobjectset_controller.go b/internal/operator-controller/controllers/clusterobjectset_controller.go index 5722c2a013..007005a3a3 100644 --- a/internal/operator-controller/controllers/clusterobjectset_controller.go +++ b/internal/operator-controller/controllers/clusterobjectset_controller.go @@ -152,22 +152,12 @@ func (c *ClusterObjectSetReconciler) reconcile(ctx context.Context, cos *ocv1.Cl return ctrl.Result{}, nil } - phases, opts, err := c.buildBoxcutterPhases(ctx, cos) + phases, currentPhases, opts, err := c.buildBoxcutterPhases(ctx, cos) if err != nil { setRetryingConditions(cos, err.Error()) return ctrl.Result{}, fmt.Errorf("converting to boxcutter revision: %v", err) } - currentPhases := make([]ocv1.ObservedPhase, 0, len(phases)) - for _, phase := range phases { - hash, err := computePhaseDigest(phase) - if err != nil { - setRetryingConditions(cos, err.Error()) - return ctrl.Result{}, fmt.Errorf("computing phase digest: %v", err) - } - currentPhases = append(currentPhases, ocv1.ObservedPhase{Name: phase.GetName(), Digest: hash}) - } - if len(cos.Status.ObservedPhases) == 0 { cos.Status.ObservedPhases = currentPhases } else if err := verifyObservedPhases(cos.Status.ObservedPhases, currentPhases); err != nil { @@ -488,10 +478,10 @@ func (c *ClusterObjectSetReconciler) listPreviousRevisions(ctx context.Context, return previous, nil } -func (c *ClusterObjectSetReconciler) buildBoxcutterPhases(ctx context.Context, cos *ocv1.ClusterObjectSet) ([]boxcutter.Phase, []boxcutter.RevisionReconcileOption, error) { +func (c *ClusterObjectSetReconciler) buildBoxcutterPhases(ctx context.Context, cos *ocv1.ClusterObjectSet) ([]boxcutter.Phase, []ocv1.ObservedPhase, []boxcutter.RevisionReconcileOption, error) { previous, err := c.listPreviousRevisions(ctx, cos) if err != nil { - return nil, nil, fmt.Errorf("listing previous revisions: %w", err) + return nil, nil, nil, fmt.Errorf("listing previous revisions: %w", err) } // Convert to []client.Object for boxcutter @@ -502,7 +492,7 @@ func (c *ClusterObjectSetReconciler) buildBoxcutterPhases(ctx context.Context, c progressionProbes, err := buildProgressionProbes(cos.Spec.ProgressionProbes) if err != nil { - return nil, nil, err + return nil, nil, nil, err } opts := []boxcutter.RevisionReconcileOption{ @@ -511,9 +501,10 @@ func (c *ClusterObjectSetReconciler) buildBoxcutterPhases(ctx context.Context, c boxcutter.WithAggregatePhaseReconcileErrors(), } - phases := make([]boxcutter.Phase, 0) + phases := make([]boxcutter.Phase, 0, len(cos.Spec.Phases)) + observedPhases := make([]ocv1.ObservedPhase, 0, len(cos.Spec.Phases)) for _, specPhase := range cos.Spec.Phases { - objs := make([]client.Object, 0) + objs := make([]client.Object, 0, len(specPhase.Objects)) for _, specObj := range specPhase.Objects { var obj *unstructured.Unstructured switch { @@ -522,13 +513,25 @@ func (c *ClusterObjectSetReconciler) buildBoxcutterPhases(ctx context.Context, c case specObj.Ref.Name != "": resolved, err := c.resolveObjectRef(ctx, specObj.Ref) if err != nil { - return nil, nil, fmt.Errorf("resolving ref in phase %q: %w", specPhase.Name, err) + return nil, nil, nil, fmt.Errorf("resolving ref in phase %q: %w", specPhase.Name, err) } obj = resolved default: - return nil, nil, fmt.Errorf("object in phase %q has neither object nor ref", specPhase.Name) + return nil, nil, nil, fmt.Errorf("object in phase %q has neither object nor ref", specPhase.Name) } + objs = append(objs, obj) + } + + // Compute digest from the user-provided objects before controller mutations. + digest, err := computePhaseDigest(specPhase.Name, objs) + if err != nil { + return nil, nil, nil, fmt.Errorf("computing phase digest: %w", err) + } + observedPhases = append(observedPhases, ocv1.ObservedPhase{Name: specPhase.Name, Digest: digest}) + + // Apply controller mutations after digest computation. + for i, obj := range objs { objLabels := obj.GetLabels() if objLabels == nil { objLabels = map[string]string{} @@ -536,17 +539,16 @@ func (c *ClusterObjectSetReconciler) buildBoxcutterPhases(ctx context.Context, c objLabels[labels.OwnerNameKey] = cos.Labels[labels.OwnerNameKey] obj.SetLabels(objLabels) - switch cp := EffectiveCollisionProtection(cos.Spec.CollisionProtection, specPhase.CollisionProtection, specObj.CollisionProtection); cp { + switch cp := EffectiveCollisionProtection(cos.Spec.CollisionProtection, specPhase.CollisionProtection, specPhase.Objects[i].CollisionProtection); cp { case ocv1.CollisionProtectionIfNoController, ocv1.CollisionProtectionNone: opts = append(opts, boxcutter.WithObjectReconcileOptions( obj, boxcutter.WithCollisionProtection(cp))) } - - objs = append(objs, obj) } + phases = append(phases, boxcutter.NewPhase(specPhase.Name, objs)) } - return phases, opts, nil + return phases, observedPhases, opts, nil } // resolveObjectRef fetches the referenced Secret, reads the value at the specified key, @@ -721,36 +723,48 @@ func markAsArchived(cos *ocv1.ClusterObjectSet) bool { } // computePhaseDigest computes a deterministic SHA-256 digest of a phase's -// resolved content (name + objects). JSON serialization of unstructured -// objects produces a canonical encoding with sorted map keys. -func computePhaseDigest(phase boxcutter.Phase) (string, error) { +// resolved content (name + objects) before any controller mutations. +// JSON serialization of unstructured objects produces a canonical encoding +// with sorted map keys. +func computePhaseDigest(name string, objects []client.Object) (string, error) { phaseMap := map[string]any{ - "name": phase.GetName(), - "objects": phase.GetObjects(), + "name": name, + "objects": objects, } data, err := json.Marshal(phaseMap) if err != nil { - return "", fmt.Errorf("marshaling phase %q: %w", phase.GetName(), err) + return "", fmt.Errorf("marshaling phase %q: %w", name, err) } h := sha256.Sum256(data) return fmt.Sprintf("sha256:%x", h), nil } // verifyObservedPhases compares current per-phase digests against stored -// digests. Returns an error naming the first mismatched phase. +// digests. Returns an error listing all mismatched phases. func verifyObservedPhases(stored, current []ocv1.ObservedPhase) error { + if len(stored) == 0 { + return fmt.Errorf("stored observedPhases is unexpectedly empty") + } + if len(stored) != len(current) { + return fmt.Errorf("number of phases has changed (expected %d phases, got %d)", len(stored), len(current)) + } storedMap := make(map[string]string, len(stored)) for _, s := range stored { storedMap[s.Name] = s.Digest } + var mismatches []string for _, c := range current { if prev, ok := storedMap[c.Name]; ok && prev != c.Digest { - return fmt.Errorf( - "resolved content of phase %q has changed (expected digest %s, got %s): "+ - "a referenced object source may have been deleted and recreated with different content", - c.Name, prev, c.Digest) + mismatches = append(mismatches, fmt.Sprintf( + "phase %q (expected digest %s, got %s)", c.Name, prev, c.Digest)) } } + if len(mismatches) > 0 { + return fmt.Errorf( + "resolved content of %d phase(s) has changed: %s; "+ + "a referenced object source may have been deleted and recreated with different content", + len(mismatches), strings.Join(mismatches, "; ")) + } return nil } @@ -778,6 +792,7 @@ func (c *ClusterObjectSetReconciler) verifyReferencedSecretsImmutable(ctx contex } } + var mutableSecrets []string for _, ref := range refs { secret := &corev1.Secret{} key := client.ObjectKey{Name: ref.name, Namespace: ref.namespace} @@ -791,9 +806,14 @@ func (c *ClusterObjectSetReconciler) verifyReferencedSecretsImmutable(ctx contex } if secret.Immutable == nil || !*secret.Immutable { - return fmt.Errorf("secret %s/%s is not immutable: referenced secrets must have immutable set to true", ref.namespace, ref.name) + mutableSecrets = append(mutableSecrets, fmt.Sprintf("%s/%s", ref.namespace, ref.name)) } } + if len(mutableSecrets) > 0 { + return fmt.Errorf("the following secrets are not immutable (referenced secrets must have immutable set to true): %s", + strings.Join(mutableSecrets, ", ")) + } + return nil } diff --git a/internal/operator-controller/controllers/clusterobjectset_controller_internal_test.go b/internal/operator-controller/controllers/clusterobjectset_controller_internal_test.go index 711abf0b2e..fe5dfb3a79 100644 --- a/internal/operator-controller/controllers/clusterobjectset_controller_internal_test.go +++ b/internal/operator-controller/controllers/clusterobjectset_controller_internal_test.go @@ -2,6 +2,7 @@ package controllers import ( "context" + "fmt" "strings" "testing" "time" @@ -16,7 +17,6 @@ import ( "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/utils/ptr" - "pkg.package-operator.run/boxcutter" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" @@ -243,10 +243,6 @@ func (m *mockTrackingCacheInternal) Source(h handler.EventHandler, predicates .. } func TestComputePhaseDigest(t *testing.T) { - makePhase := func(name string, objs ...client.Object) boxcutter.Phase { - return boxcutter.NewPhase(name, objs) - } - makeObj := func(apiVersion, kind, name string) *unstructured.Unstructured { return &unstructured.Unstructured{ Object: map[string]interface{}{ @@ -258,45 +254,66 @@ func TestComputePhaseDigest(t *testing.T) { } t.Run("deterministic for same content", func(t *testing.T) { - phase := makePhase("deploy", makeObj("v1", "ConfigMap", "cm1")) - hash1, err := computePhaseDigest(phase) + objs := []client.Object{makeObj("v1", "ConfigMap", "cm1")} + hash1, err := computePhaseDigest("deploy", objs) require.NoError(t, err) - hash2, err := computePhaseDigest(phase) + hash2, err := computePhaseDigest("deploy", objs) require.NoError(t, err) assert.Equal(t, hash1, hash2) }) + t.Run("deterministic with many objects", func(t *testing.T) { + objs := make([]client.Object, 100) + for i := range objs { + objs[i] = makeObj("v1", "ConfigMap", fmt.Sprintf("cm-%d", i)) + } + var first string + for i := range 100 { + digest, err := computePhaseDigest("deploy", objs) + require.NoError(t, err) + if i == 0 { + first = digest + } else { + assert.Equal(t, first, digest, "digest changed on iteration %d", i) + } + } + }) + t.Run("different for different object content", func(t *testing.T) { - p1 := makePhase("deploy", makeObj("v1", "ConfigMap", "cm1")) - p2 := makePhase("deploy", makeObj("v1", "ConfigMap", "cm2")) - h1, err := computePhaseDigest(p1) + h1, err := computePhaseDigest("deploy", []client.Object{makeObj("v1", "ConfigMap", "cm1")}) require.NoError(t, err) - h2, err := computePhaseDigest(p2) + h2, err := computePhaseDigest("deploy", []client.Object{makeObj("v1", "ConfigMap", "cm2")}) require.NoError(t, err) assert.NotEqual(t, h1, h2) }) t.Run("different for different phase names", func(t *testing.T) { - obj := makeObj("v1", "ConfigMap", "cm1") - p1 := makePhase("deploy", obj) - p2 := makePhase("crds", obj) - h1, err := computePhaseDigest(p1) + objs := []client.Object{makeObj("v1", "ConfigMap", "cm1")} + h1, err := computePhaseDigest("deploy", objs) + require.NoError(t, err) + h2, err := computePhaseDigest("crds", objs) + require.NoError(t, err) + assert.NotEqual(t, h1, h2) + }) + + t.Run("different order produces different digest", func(t *testing.T) { + obj1 := makeObj("v1", "ConfigMap", "cm1") + obj2 := makeObj("v1", "ConfigMap", "cm2") + h1, err := computePhaseDigest("deploy", []client.Object{obj1, obj2}) require.NoError(t, err) - h2, err := computePhaseDigest(p2) + h2, err := computePhaseDigest("deploy", []client.Object{obj2, obj1}) require.NoError(t, err) assert.NotEqual(t, h1, h2) }) t.Run("empty phase produces valid digest", func(t *testing.T) { - phase := makePhase("empty") - hash, err := computePhaseDigest(phase) + hash, err := computePhaseDigest("empty", nil) require.NoError(t, err) assert.NotEmpty(t, hash) }) t.Run("digest has sha256 prefix", func(t *testing.T) { - phase := makePhase("deploy", makeObj("v1", "ConfigMap", "cm1")) - digest, err := computePhaseDigest(phase) + digest, err := computePhaseDigest("deploy", []client.Object{makeObj("v1", "ConfigMap", "cm1")}) require.NoError(t, err) assert.True(t, strings.HasPrefix(digest, "sha256:"), "digest should start with sha256: prefix, got %s", digest) }) @@ -314,21 +331,42 @@ func TestVerifyObservedPhases(t *testing.T) { current := []ocv1.ObservedPhase{{Name: "deploy", Digest: "sha256:def456"}} err := verifyObservedPhases(stored, current) require.Error(t, err) - assert.Contains(t, err.Error(), `resolved content of phase "deploy" has changed`) + assert.Contains(t, err.Error(), `resolved content of 1 phase(s) has changed`) + assert.Contains(t, err.Error(), `phase "deploy"`) + }) + + t.Run("reports all mismatched phases", func(t *testing.T) { + stored := []ocv1.ObservedPhase{ + {Name: "deploy", Digest: "sha256:aaa"}, + {Name: "crds", Digest: "sha256:bbb"}, + } + current := []ocv1.ObservedPhase{ + {Name: "deploy", Digest: "sha256:xxx"}, + {Name: "crds", Digest: "sha256:yyy"}, + } + err := verifyObservedPhases(stored, current) + require.Error(t, err) + assert.Contains(t, err.Error(), `resolved content of 2 phase(s) has changed`) + assert.Contains(t, err.Error(), `phase "deploy"`) + assert.Contains(t, err.Error(), `phase "crds"`) }) - t.Run("passes when current has new phase not in stored", func(t *testing.T) { + t.Run("fails when phase count changes", func(t *testing.T) { stored := []ocv1.ObservedPhase{{Name: "deploy", Digest: "sha256:abc123"}} current := []ocv1.ObservedPhase{ {Name: "deploy", Digest: "sha256:abc123"}, {Name: "crds", Digest: "sha256:def456"}, } - assert.NoError(t, verifyObservedPhases(stored, current)) + err := verifyObservedPhases(stored, current) + require.Error(t, err) + assert.Contains(t, err.Error(), "number of phases has changed") }) - t.Run("passes with empty stored", func(t *testing.T) { + t.Run("fails with empty stored", func(t *testing.T) { current := []ocv1.ObservedPhase{{Name: "deploy", Digest: "sha256:abc123"}} - assert.NoError(t, verifyObservedPhases(nil, current)) + err := verifyObservedPhases(nil, current) + require.Error(t, err) + assert.Contains(t, err.Error(), "unexpectedly empty") }) } @@ -428,7 +466,7 @@ func TestVerifyReferencedSecretsImmutable(t *testing.T) { require.NoError(t, err) }) - t.Run("deduplicates refs to same secret", func(t *testing.T) { + t.Run("checks secret only once when referenced multiple times", func(t *testing.T) { secret := immutableSecret.DeepCopy() testClient := fake.NewClientBuilder(). WithScheme(testScheme). diff --git a/test/e2e/features/revision.feature b/test/e2e/features/revision.feature index f7347242f4..60093c5214 100644 --- a/test/e2e/features/revision.feature +++ b/test/e2e/features/revision.feature @@ -445,7 +445,7 @@ Feature: Install ClusterObjectSet And resource "deployment/test-httpd" is installed And ClusterObjectSet "${COS_NAME}" has observed phase "resources" with a non-empty digest - Scenario: ClusterObjectSet blocks reconciliation when referenced Secret is not immutable + Scenario: ClusterObjectSet blocks reconciliation when referenced Secret is mutable Given ServiceAccount "olm-sa" with needed permissions is available in test namespace And resource is applied """ @@ -492,7 +492,7 @@ Feature: Install ClusterObjectSet """ Then ClusterObjectSet "${COS_NAME}" reports Progressing as False with Reason Blocked and Message: """ - secret ${TEST_NAMESPACE}/${COS_NAME}-mutable-secret is not immutable: referenced secrets must have immutable set to true + the following secrets are not immutable (referenced secrets must have immutable set to true): ${TEST_NAMESPACE}/${COS_NAME}-mutable-secret """ Scenario: ClusterObjectSet blocks reconciliation when referenced Secret content changes @@ -573,4 +573,31 @@ Feature: Install ClusterObjectSet Then ClusterObjectSet "${COS_NAME}" reports Progressing as False with Reason Blocked and Message includes: """ resolved content of phase "resources" has changed - """ \ No newline at end of file + """ + # Restore original content — COS should recover + When resource "secret/${COS_NAME}-change-secret" is removed + And resource is applied + """ + apiVersion: v1 + kind: Secret + metadata: + name: ${COS_NAME}-change-secret + namespace: ${TEST_NAMESPACE} + immutable: true + type: olm.operatorframework.io/object-data + stringData: + configmap: | + { + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": { + "name": "test-cm-change", + "namespace": "${TEST_NAMESPACE}" + }, + "data": { + "key": "original-value" + } + } + """ + And ClusterObjectSet "${COS_NAME}" reconciliation is triggered + Then ClusterObjectSet "${COS_NAME}" reports Progressing as True with Reason Succeeded From 567535a008e6e0f5838df3f666bbf2285e25f40a Mon Sep 17 00:00:00 2001 From: Predrag Knezevic Date: Sat, 11 Apr 2026 02:39:03 +0200 Subject: [PATCH 5/5] Fix e2e assertion to match new multi-phase error message format The verifyObservedPhases error message changed from "resolved content of phase X has changed" to "resolved content of N phase(s) has changed: phase X (expected...)" when collecting all mismatches. Update the e2e assertion fragment to match the new format. Co-Authored-By: Claude Opus 4.6 --- test/e2e/features/revision.feature | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/e2e/features/revision.feature b/test/e2e/features/revision.feature index 60093c5214..38ed160279 100644 --- a/test/e2e/features/revision.feature +++ b/test/e2e/features/revision.feature @@ -572,7 +572,7 @@ Feature: Install ClusterObjectSet And ClusterObjectSet "${COS_NAME}" reconciliation is triggered Then ClusterObjectSet "${COS_NAME}" reports Progressing as False with Reason Blocked and Message includes: """ - resolved content of phase "resources" has changed + resolved content of 1 phase(s) has changed: phase "resources" """ # Restore original content — COS should recover When resource "secret/${COS_NAME}-change-secret" is removed