diff --git a/api/v1/clusterobjectset_types.go b/api/v1/clusterobjectset_types.go index ed1a4001e0..67f962370f 100644 --- a/api/v1/clusterobjectset_types.go +++ b/api/v1/clusterobjectset_types.go @@ -510,6 +510,31 @@ type ClusterObjectSetStatus struct { // +listMapKey=type // +optional Conditions []metav1.Condition `json:"conditions,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. + // + // +listType=map + // +listMapKey=name + // +optional + ObservedPhases []ObservedPhase `json:"observedPhases,omitempty"` +} + +// ObservedPhase records the observed content hash 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 phase's resolved + // object content at first successful reconciliation. + // + // +required + Hash string `json:"hash"` } // +genclient diff --git a/api/v1/zz_generated.deepcopy.go b/api/v1/zz_generated.deepcopy.go index 8d1cea0245..384439787b 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.ObservedPhases != nil { + in, out := &in.ObservedPhases, &out.ObservedPhases + *out = make([]ObservedPhase, 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 *ObservedPhase) DeepCopyInto(out *ObservedPhase) { + *out = *in +} + +// 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(ObservedPhase) + 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..6203563de7 100644 --- a/applyconfigurations/api/v1/clusterobjectsetstatus.go +++ b/applyconfigurations/api/v1/clusterobjectsetstatus.go @@ -46,6 +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"` + // 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 @@ -66,3 +72,16 @@ func (b *ClusterObjectSetStatusApplyConfiguration) WithConditions(values ...*met } return b } + +// 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 ObservedPhases field. +func (b *ClusterObjectSetStatusApplyConfiguration) WithObservedPhases(values ...*ObservedPhaseApplyConfiguration) *ClusterObjectSetStatusApplyConfiguration { + for i := range values { + if values[i] == nil { + panic("nil value passed to WithObservedPhases") + } + b.ObservedPhases = append(b.ObservedPhases, *values[i]) + } + return b +} diff --git a/applyconfigurations/api/v1/observedphase.go b/applyconfigurations/api/v1/observedphase.go new file mode 100644 index 0000000000..2be4332fbe --- /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 hash of a resolved phase. +type ObservedPhaseApplyConfiguration struct { + // name is the phase name matching a phase in spec.phases. + Name *string `json:"name,omitempty"` + // hash is the hex-encoded SHA-256 hash of the phase's resolved + // object content at first successful reconciliation. + Hash *string `json:"hash,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 +} + +// 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 *ObservedPhaseApplyConfiguration) WithHash(value string) *ObservedPhaseApplyConfiguration { + b.Hash = &value + return b +} diff --git a/applyconfigurations/utils.go b/applyconfigurations/utils.go index 7b1373e4a1..6a467f96a6 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("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 325aaacc1c..dd70a89d6a 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 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 @@ -388,6 +392,14 @@ Key properties: ### COS reconciler behavior +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 +hash and compares it against the hashes 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 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..abbe970362 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 + observedPhases: + description: |- + 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: ObservedPhase records the observed content hash of + a resolved phase. + properties: + hash: + description: |- + hash is the hex-encoded SHA-256 hash of the phase's resolved + object content at first successful reconciliation. + type: string + name: + description: name is the phase name matching a phase in spec.phases. + 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 055f962ce6..50e2a85b92 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,12 +146,36 @@ func (c *ClusterObjectSetReconciler) reconcile(ctx context.Context, cos *ocv1.Cl return c.delete(ctx, cos) } + 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 + } + phases, 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 := computePhaseHash(phase) + if err != nil { + setRetryingConditions(cos, err.Error()) + return ctrl.Result{}, fmt.Errorf("computing phase hash: %v", err) + } + currentPhases = append(currentPhases, ocv1.ObservedPhase{Name: phase.GetName(), Hash: 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 + } + siblings, err := c.siblingRevisionNames(ctx, cos) if err != nil { setRetryingConditions(cos, err.Error()) @@ -344,7 +370,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 { @@ -354,8 +380,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 { + return false + } } return true }, @@ -366,7 +394,7 @@ func (c *ClusterObjectSetReconciler) SetupWithManager(mgr ctrl.Manager) error { &ocv1.ClusterObjectSet{}, builder.WithPredicates( predicate.ResourceVersionChangedPredicate{}, - skipProgressDeadlineExceededPredicate, + skipTerminallyBlockedPredicate, ), ). WatchesRawSource( @@ -739,3 +767,81 @@ func markAsArchived(cos *ocv1.ClusterObjectSet) bool { updated := markAsNotProgressing(cos, ocv1.ClusterObjectSetReasonArchived, msg) return markAsAvailableUnknown(cos, ocv1.ClusterObjectSetReasonArchived, msg) || updated } + +// computePhaseHash computes a deterministic SHA-256 hash of a phase's +// resolved content (name + objects). JSON serialization of each object +// produces a canonical encoding with sorted map keys. +func computePhaseHash(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 +} + +// verifyObservedPhases compares current per-phase hashes against stored +// hashes. 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.Hash + } + for _, c := range current { + if prev, ok := storedMap[c.Name]; ok && prev != c.Hash { + return fmt.Errorf( + "resolved content of phase %q has changed (expected hash %s, got %s): "+ + "a referenced object source may have been deleted and recreated with different content", + c.Name, prev, c.Hash) + } + } + return nil +} + +// 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 + } + 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) + } + } + } + + 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 fmt.Errorf("getting Secret %s/%s: %w", ref.namespace, ref.name, err) + } + + 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) + } + } + + 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 15300f63ca..3fae24632b 100644 --- a/internal/operator-controller/controllers/clusterobjectset_controller_internal_test.go +++ b/internal/operator-controller/controllers/clusterobjectset_controller_internal_test.go @@ -5,12 +5,17 @@ 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/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" @@ -235,3 +240,208 @@ 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 TestComputePhaseHash(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}, + }, + } + } + + t.Run("deterministic for same content", func(t *testing.T) { + phase := makePhase("deploy", makeObj("v1", "ConfigMap", "cm1")) + hash1, err := computePhaseHash(phase) + require.NoError(t, err) + hash2, err := computePhaseHash(phase) + require.NoError(t, err) + assert.Equal(t, hash1, hash2) + }) + + 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 := computePhaseHash(p1) + require.NoError(t, err) + h2, err := computePhaseHash(p2) + 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 := computePhaseHash(p1) + require.NoError(t, err) + h2, err := computePhaseHash(p2) + require.NoError(t, err) + assert.NotEqual(t, h1, h2) + }) + + t.Run("empty phase produces valid hash", func(t *testing.T) { + phase := makePhase("empty") + hash, err := computePhaseHash(phase) + require.NoError(t, err) + assert.NotEmpty(t, hash) + }) +} + +func TestVerifyObservedPhases(t *testing.T) { + t.Run("passes when hashes match", func(t *testing.T) { + stored := []ocv1.ObservedPhase{{Name: "deploy", Hash: "abc123"}} + current := []ocv1.ObservedPhase{{Name: "deploy", Hash: "abc123"}} + assert.NoError(t, verifyObservedPhases(stored, current)) + }) + + t.Run("fails when hash changes", func(t *testing.T) { + stored := []ocv1.ObservedPhase{{Name: "deploy", Hash: "abc123"}} + current := []ocv1.ObservedPhase{{Name: "deploy", Hash: "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", Hash: "abc123"}} + current := []ocv1.ObservedPhase{ + {Name: "deploy", Hash: "abc123"}, + {Name: "crds", Hash: "new-hash"}, + } + assert.NoError(t, verifyObservedPhases(stored, current)) + }) + + t.Run("passes with empty stored", func(t *testing.T) { + current := []ocv1.ObservedPhase{{Name: "deploy", Hash: "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)) + + 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 when all secrets are immutable", 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", + }, + }}, + }}, + }, + } + + err := reconciler.verifyReferencedSecretsImmutable(t.Context(), cos) + require.NoError(t, err) + }) + + 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.verifyReferencedSecretsImmutable(t.Context(), cos) + require.Error(t, err) + assert.Contains(t, err.Error(), "not immutable") + }) + + 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 + }}, + }}, + }, + } + + err := reconciler.verifyReferencedSecretsImmutable(t.Context(), cos) + require.NoError(t, err) + }) + + 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"}}, + }, + }}, + }, + } + + err := reconciler.verifyReferencedSecretsImmutable(t.Context(), cos) + require.NoError(t, err) + }) +} 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..e6c5036645 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 + observedPhases: + description: |- + 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: ObservedPhase records the observed content hash of + a resolved phase. + properties: + hash: + description: |- + hash is the hex-encoded SHA-256 hash of the phase's resolved + object content at first successful reconciliation. + type: string + name: + description: name is the phase name matching a phase in spec.phases. + 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..08d9576a96 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 + observedPhases: + description: |- + 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: ObservedPhase records the observed content hash of + a resolved phase. + properties: + hash: + description: |- + hash is the hex-encoded SHA-256 hash of the phase's resolved + object content at first successful reconciliation. + type: string + name: + description: name is the phase name matching a phase in spec.phases. + 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..eb94220671 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 phase "resources" 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 phase "resources" 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: + """ + 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 2b5603ab79..eb24f23b5e 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 phase "([^"]+)" with a non-empty hash$`, 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) @@ -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 +} + +// ClusterObjectSetHasObservedPhase waits for the named ClusterObjectSet to have +// an observedPhases entry matching the given phase name with a non-empty hash. Polls with timeout. +func ClusterObjectSetHasObservedPhase(ctx context.Context, cosName, phaseName string) error { + sc := scenarioCtx(ctx) + cosName = substituteScenarioVars(cosName, sc) + phaseName = substituteScenarioVars(phaseName, sc) + + waitFor(ctx, func() bool { + out, err := k8sClient("get", "clusterobjectset", cosName, "-o", + fmt.Sprintf(`jsonpath={.status.observedPhases[?(@.name=="%s")].hash}`, phaseName)) + 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 {