diff --git a/api/v1/clusterobjectset_types.go b/api/v1/clusterobjectset_types.go index ed1a4001e0..d7e755992c 100644 --- a/api/v1/clusterobjectset_types.go +++ b/api/v1/clusterobjectset_types.go @@ -510,6 +510,39 @@ 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. + // + // +kubebuilder:validation:XValidation:rule="self == oldSelf || oldSelf.size() == 0",message="observedPhases is immutable" + // +kubebuilder:validation:MaxItems=20 + // +listType=map + // +listMapKey=name + // +optional + ObservedPhases []ObservedPhase `json:"observedPhases,omitempty"` +} + +// 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 + // +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 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"` } // +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..75f5fb1d60 --- /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 digest of the phase's resolved object content + // at first successful resolution, in the format ":". + 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 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/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/draft/concepts/large-bundle-support.md b/docs/draft/concepts/large-bundle-support.md index 325aaacc1c..dba87b982f 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 +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: - 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..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 @@ -621,6 +621,49 @@ 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 digest of + a resolved phase. + properties: + digest: + description: |- + 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 b34730b774..007005a3a3 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,26 @@ func (c *ClusterObjectSetReconciler) reconcile(ctx context.Context, cos *ocv1.Cl return c.delete(ctx, cos) } - phases, opts, err := c.buildBoxcutterPhases(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, 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) } + 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()) @@ -462,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 @@ -476,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{ @@ -485,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 { @@ -496,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{} @@ -510,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, @@ -693,3 +721,99 @@ func markAsArchived(cos *ocv1.ClusterObjectSet) bool { updated := markAsNotProgressing(cos, ocv1.ClusterObjectSetReasonArchived, msg) return markAsAvailableUnknown(cos, ocv1.ClusterObjectSetReasonArchived, msg) || updated } + +// computePhaseDigest computes a deterministic SHA-256 digest of a phase's +// 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": name, + "objects": objects, + } + data, err := json.Marshal(phaseMap) + if err != nil { + 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 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 { + 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 +} + +// 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) + } + } + } + + var mutableSecrets []string + 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 { + 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 15300f63ca..fe5dfb3a79 100644 --- a/internal/operator-controller/controllers/clusterobjectset_controller_internal_test.go +++ b/internal/operator-controller/controllers/clusterobjectset_controller_internal_test.go @@ -2,15 +2,21 @@ package controllers import ( "context" + "fmt" + "strings" "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" "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 +241,253 @@ 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 TestComputePhaseDigest(t *testing.T) { + 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) { + objs := []client.Object{makeObj("v1", "ConfigMap", "cm1")} + hash1, err := computePhaseDigest("deploy", objs) + require.NoError(t, err) + 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) { + h1, err := computePhaseDigest("deploy", []client.Object{makeObj("v1", "ConfigMap", "cm1")}) + require.NoError(t, err) + 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) { + 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("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) { + hash, err := computePhaseDigest("empty", nil) + require.NoError(t, err) + assert.NotEmpty(t, hash) + }) + + t.Run("digest has sha256 prefix", func(t *testing.T) { + 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) + }) +} + +func TestVerifyObservedPhases(t *testing.T) { + t.Run("passes when digests match", func(t *testing.T) { + 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: "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 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("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"}, + } + err := verifyObservedPhases(stored, current) + require.Error(t, err) + assert.Contains(t, err.Error(), "number of phases has changed") + }) + + t.Run("fails with empty stored", func(t *testing.T) { + current := []ocv1.ObservedPhase{{Name: "deploy", Digest: "sha256:abc123"}} + err := verifyObservedPhases(nil, current) + require.Error(t, err) + assert.Contains(t, err.Error(), "unexpectedly empty") + }) +} + +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("checks secret only once when referenced multiple times", 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..f52b31a39d 100644 --- a/manifests/experimental-e2e.yaml +++ b/manifests/experimental-e2e.yaml @@ -1945,6 +1945,49 @@ 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 digest of + a resolved phase. + properties: + digest: + description: |- + 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 9a8fa0b406..b8e48ba2f8 100644 --- a/manifests/experimental.yaml +++ b/manifests/experimental.yaml @@ -1906,6 +1906,49 @@ 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 digest of + a resolved phase. + properties: + digest: + description: |- + 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/features/revision.feature b/test/e2e/features/revision.feature index dd6d9e9407..38ed160279 100644 --- a/test/e2e/features/revision.feature +++ b/test/e2e/features/revision.feature @@ -442,4 +442,162 @@ 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 digest + + 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 + """ + 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: + """ + 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 + 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 digest + # 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 1 phase(s) has changed: phase "resources" + """ + # 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 diff --git a/test/e2e/steps/steps.go b/test/e2e/steps/steps.go index 2b5603ab79..c9748d99d3 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 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) @@ -508,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) @@ -597,6 +601,48 @@ 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 { + normalizedActual := strings.Join(strings.Fields(actualMsg), " ") + return strings.Contains(normalizedActual, 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 digest. 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")].digest}`, 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 {