Skip to content

Commit eb25334

Browse files
pedjakclaude
pedjak
and
claude
committed
Address review feedback: delimiter in digest, wording fix, whitespace normalization
- 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 <noreply@anthropic.com>
1 parent 3db997b commit eb25334

10 files changed

Lines changed: 102 additions & 40 deletions

File tree

‎api/v1/clusterobjectset_types.go‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -517,6 +517,8 @@ type ClusterObjectSetStatus struct {
517517
// different content. Each entry covers all fully-resolved object
518518
// manifests within a phase, making it source-agnostic.
519519
//
520+
// +kubebuilder:validation:XValidation:rule="self == oldSelf || oldSelf.size() == 0",message="observedPhases is immutable"
521+
// +kubebuilder:validation:MaxItems=20
520522
// +listType=map
521523
// +listMapKey=name
522524
// +optional
@@ -528,12 +530,18 @@ type ObservedPhase struct {
528530
// name is the phase name matching a phase in spec.phases.
529531
//
530532
// +required
533+
// +kubebuilder:validation:MinLength=1
534+
// +kubebuilder:validation:MaxLength=63
535+
// +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."
531536
Name string `json:"name"`
532537

533-
// digest is the hex-encoded SHA-256 digest of the phase's resolved
534-
// object content at first successful reconciliation.
538+
// digest is the digest of the phase's resolved object content
539+
// at first successful resolution, in the format "<algorithm>:<hex>".
535540
//
536541
// +required
542+
// +kubebuilder:validation:MinLength=1
543+
// +kubebuilder:validation:MaxLength=256
544+
// +kubebuilder:validation:XValidation:rule=`self.matches('^[a-z0-9]+:[a-f0-9]+$')`,message="digest must be in the format '<algorithm>:<hex>'"
537545
Digest string `json:"digest"`
538546
}
539547

‎applyconfigurations/api/v1/observedphase.go‎

Lines changed: 2 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎docs/api-reference/crd-ref-docs-gen-config.yaml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
processor:
2-
ignoreTypes: [ClusterObjectSet, ClusterObjectSetList]
2+
ignoreTypes: [ClusterObjectSet, ClusterObjectSetList, ObservedPhase]
33
ignoreFields: []
44

55
render:

‎docs/api-reference/olmv1-api-reference.md‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -477,8 +477,6 @@ _Appears in:_
477477

478478

479479

480-
481-
482480
#### PreflightConfig
483481

484482

‎helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterobjectsets.yaml‎

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -634,20 +634,36 @@ spec:
634634
properties:
635635
digest:
636636
description: |-
637-
digest is the hex-encoded SHA-256 digest of the phase's resolved
638-
object content at first successful reconciliation.
637+
digest is the digest of the phase's resolved object content
638+
at first successful resolution, in the format "<algorithm>:<hex>".
639+
maxLength: 256
640+
minLength: 1
639641
type: string
642+
x-kubernetes-validations:
643+
- message: digest must be in the format '<algorithm>:<hex>'
644+
rule: self.matches('^[a-z0-9]+:[a-f0-9]+$')
640645
name:
641646
description: name is the phase name matching a phase in spec.phases.
647+
maxLength: 63
648+
minLength: 1
642649
type: string
650+
x-kubernetes-validations:
651+
- message: the value must consist of only lowercase alphanumeric
652+
characters and hyphens, and must start with an alphabetic
653+
character and end with an alphanumeric character.
654+
rule: '!format.dns1123Label().validate(self).hasValue()'
643655
required:
644656
- digest
645657
- name
646658
type: object
659+
maxItems: 20
647660
type: array
648661
x-kubernetes-list-map-keys:
649662
- name
650663
x-kubernetes-list-type: map
664+
x-kubernetes-validations:
665+
- message: observedPhases is immutable
666+
rule: self == oldSelf || oldSelf.size() == 0
651667
type: object
652668
type: object
653669
served: true

‎internal/operator-controller/controllers/clusterobjectset_controller.go‎

Lines changed: 15 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -359,7 +359,7 @@ type Sourcoser interface {
359359
}
360360

361361
func (c *ClusterObjectSetReconciler) SetupWithManager(mgr ctrl.Manager) error {
362-
skipTerminallyBlockedPredicate := predicate.Funcs{
362+
skipProgressDeadlineExceededPredicate := predicate.Funcs{
363363
UpdateFunc: func(e event.UpdateEvent) bool {
364364
rev, ok := e.ObjectNew.(*ocv1.ClusterObjectSet)
365365
if !ok {
@@ -369,10 +369,8 @@ func (c *ClusterObjectSetReconciler) SetupWithManager(mgr ctrl.Manager) error {
369369
if !rev.DeletionTimestamp.IsZero() {
370370
return true
371371
}
372-
if cnd := meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterObjectSetTypeProgressing); cnd != nil && cnd.Status == metav1.ConditionFalse {
373-
if cnd.Reason == ocv1.ReasonProgressDeadlineExceeded {
374-
return false
375-
}
372+
if cnd := meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterObjectSetTypeProgressing); cnd != nil && cnd.Status == metav1.ConditionFalse && cnd.Reason == ocv1.ReasonProgressDeadlineExceeded {
373+
return false
376374
}
377375
return true
378376
},
@@ -383,7 +381,7 @@ func (c *ClusterObjectSetReconciler) SetupWithManager(mgr ctrl.Manager) error {
383381
&ocv1.ClusterObjectSet{},
384382
builder.WithPredicates(
385383
predicate.ResourceVersionChangedPredicate{},
386-
skipTerminallyBlockedPredicate,
384+
skipProgressDeadlineExceededPredicate,
387385
),
388386
).
389387
WatchesRawSource(
@@ -723,19 +721,19 @@ func markAsArchived(cos *ocv1.ClusterObjectSet) bool {
723721
}
724722

725723
// computePhaseDigest computes a deterministic SHA-256 digest of a phase's
726-
// resolved content (name + objects). JSON serialization of each object
727-
// produces a canonical encoding with sorted map keys.
724+
// resolved content (name + objects). JSON serialization of unstructured
725+
// objects produces a canonical encoding with sorted map keys.
728726
func computePhaseDigest(phase boxcutter.Phase) (string, error) {
729-
h := sha256.New()
730-
fmt.Fprintf(h, "phase:%s\n", phase.GetName())
731-
for _, obj := range phase.GetObjects() {
732-
data, err := json.Marshal(obj)
733-
if err != nil {
734-
return "", fmt.Errorf("marshaling object in phase %q: %w", phase.GetName(), err)
735-
}
736-
h.Write(data)
727+
phaseMap := map[string]any{
728+
"name": phase.GetName(),
729+
"objects": phase.GetObjects(),
730+
}
731+
data, err := json.Marshal(phaseMap)
732+
if err != nil {
733+
return "", fmt.Errorf("marshaling phase %q: %w", phase.GetName(), err)
737734
}
738-
return fmt.Sprintf("%x", h.Sum(nil)), nil
735+
h := sha256.Sum256(data)
736+
return fmt.Sprintf("sha256:%x", h), nil
739737
}
740738

741739
// verifyObservedPhases compares current per-phase digests against stored

‎internal/operator-controller/controllers/clusterobjectset_controller_internal_test.go‎

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package controllers
22

33
import (
44
"context"
5+
"strings"
56
"testing"
67
"time"
78

@@ -292,34 +293,41 @@ func TestComputePhaseDigest(t *testing.T) {
292293
require.NoError(t, err)
293294
assert.NotEmpty(t, hash)
294295
})
296+
297+
t.Run("digest has sha256 prefix", func(t *testing.T) {
298+
phase := makePhase("deploy", makeObj("v1", "ConfigMap", "cm1"))
299+
digest, err := computePhaseDigest(phase)
300+
require.NoError(t, err)
301+
assert.True(t, strings.HasPrefix(digest, "sha256:"), "digest should start with sha256: prefix, got %s", digest)
302+
})
295303
}
296304

297305
func TestVerifyObservedPhases(t *testing.T) {
298306
t.Run("passes when digests match", func(t *testing.T) {
299-
stored := []ocv1.ObservedPhase{{Name: "deploy", Digest: "abc123"}}
300-
current := []ocv1.ObservedPhase{{Name: "deploy", Digest: "abc123"}}
307+
stored := []ocv1.ObservedPhase{{Name: "deploy", Digest: "sha256:abc123"}}
308+
current := []ocv1.ObservedPhase{{Name: "deploy", Digest: "sha256:abc123"}}
301309
assert.NoError(t, verifyObservedPhases(stored, current))
302310
})
303311

304312
t.Run("fails when digest changes", func(t *testing.T) {
305-
stored := []ocv1.ObservedPhase{{Name: "deploy", Digest: "abc123"}}
306-
current := []ocv1.ObservedPhase{{Name: "deploy", Digest: "def456"}}
313+
stored := []ocv1.ObservedPhase{{Name: "deploy", Digest: "sha256:abc123"}}
314+
current := []ocv1.ObservedPhase{{Name: "deploy", Digest: "sha256:def456"}}
307315
err := verifyObservedPhases(stored, current)
308316
require.Error(t, err)
309317
assert.Contains(t, err.Error(), `resolved content of phase "deploy" has changed`)
310318
})
311319

312320
t.Run("passes when current has new phase not in stored", func(t *testing.T) {
313-
stored := []ocv1.ObservedPhase{{Name: "deploy", Digest: "abc123"}}
321+
stored := []ocv1.ObservedPhase{{Name: "deploy", Digest: "sha256:abc123"}}
314322
current := []ocv1.ObservedPhase{
315-
{Name: "deploy", Digest: "abc123"},
316-
{Name: "crds", Digest: "new-digest"},
323+
{Name: "deploy", Digest: "sha256:abc123"},
324+
{Name: "crds", Digest: "sha256:def456"},
317325
}
318326
assert.NoError(t, verifyObservedPhases(stored, current))
319327
})
320328

321329
t.Run("passes with empty stored", func(t *testing.T) {
322-
current := []ocv1.ObservedPhase{{Name: "deploy", Digest: "abc123"}}
330+
current := []ocv1.ObservedPhase{{Name: "deploy", Digest: "sha256:abc123"}}
323331
assert.NoError(t, verifyObservedPhases(nil, current))
324332
})
325333
}

‎manifests/experimental-e2e.yaml‎

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1958,20 +1958,36 @@ spec:
19581958
properties:
19591959
digest:
19601960
description: |-
1961-
digest is the hex-encoded SHA-256 digest of the phase's resolved
1962-
object content at first successful reconciliation.
1961+
digest is the digest of the phase's resolved object content
1962+
at first successful resolution, in the format "<algorithm>:<hex>".
1963+
maxLength: 256
1964+
minLength: 1
19631965
type: string
1966+
x-kubernetes-validations:
1967+
- message: digest must be in the format '<algorithm>:<hex>'
1968+
rule: self.matches('^[a-z0-9]+:[a-f0-9]+$')
19641969
name:
19651970
description: name is the phase name matching a phase in spec.phases.
1971+
maxLength: 63
1972+
minLength: 1
19661973
type: string
1974+
x-kubernetes-validations:
1975+
- message: the value must consist of only lowercase alphanumeric
1976+
characters and hyphens, and must start with an alphabetic
1977+
character and end with an alphanumeric character.
1978+
rule: '!format.dns1123Label().validate(self).hasValue()'
19671979
required:
19681980
- digest
19691981
- name
19701982
type: object
1983+
maxItems: 20
19711984
type: array
19721985
x-kubernetes-list-map-keys:
19731986
- name
19741987
x-kubernetes-list-type: map
1988+
x-kubernetes-validations:
1989+
- message: observedPhases is immutable
1990+
rule: self == oldSelf || oldSelf.size() == 0
19751991
type: object
19761992
type: object
19771993
served: true

‎manifests/experimental.yaml‎

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1919,20 +1919,36 @@ spec:
19191919
properties:
19201920
digest:
19211921
description: |-
1922-
digest is the hex-encoded SHA-256 digest of the phase's resolved
1923-
object content at first successful reconciliation.
1922+
digest is the digest of the phase's resolved object content
1923+
at first successful resolution, in the format "<algorithm>:<hex>".
1924+
maxLength: 256
1925+
minLength: 1
19241926
type: string
1927+
x-kubernetes-validations:
1928+
- message: digest must be in the format '<algorithm>:<hex>'
1929+
rule: self.matches('^[a-z0-9]+:[a-f0-9]+$')
19251930
name:
19261931
description: name is the phase name matching a phase in spec.phases.
1932+
maxLength: 63
1933+
minLength: 1
19271934
type: string
1935+
x-kubernetes-validations:
1936+
- message: the value must consist of only lowercase alphanumeric
1937+
characters and hyphens, and must start with an alphabetic
1938+
character and end with an alphanumeric character.
1939+
rule: '!format.dns1123Label().validate(self).hasValue()'
19281940
required:
19291941
- digest
19301942
- name
19311943
type: object
1944+
maxItems: 20
19321945
type: array
19331946
x-kubernetes-list-map-keys:
19341947
- name
19351948
x-kubernetes-list-type: map
1949+
x-kubernetes-validations:
1950+
- message: observedPhases is immutable
1951+
rule: self == oldSelf || oldSelf.size() == 0
19361952
type: object
19371953
type: object
19381954
served: true

‎test/e2e/steps/steps.go‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -511,7 +511,8 @@ func ClusterExtensionReportsConditionWithMessageFragment(ctx context.Context, co
511511
if msgFragment != nil {
512512
expectedMsgFragment := substituteScenarioVars(strings.Join(strings.Fields(msgFragment.Content), " "), scenarioCtx(ctx))
513513
msgCmp = func(actualMsg string) bool {
514-
return strings.Contains(actualMsg, expectedMsgFragment)
514+
normalizedActual := strings.Join(strings.Fields(actualMsg), " ")
515+
return strings.Contains(normalizedActual, expectedMsgFragment)
515516
}
516517
}
517518
return waitForExtensionCondition(ctx, conditionType, conditionStatus, &conditionReason, msgCmp)
@@ -607,7 +608,8 @@ func ClusterObjectSetReportsConditionWithMessageFragment(ctx context.Context, re
607608
if msgFragment != nil {
608609
expectedMsgFragment := substituteScenarioVars(strings.Join(strings.Fields(msgFragment.Content), " "), scenarioCtx(ctx))
609610
msgCmp = func(actualMsg string) bool {
610-
return strings.Contains(actualMsg, expectedMsgFragment)
611+
normalizedActual := strings.Join(strings.Fields(actualMsg), " ")
612+
return strings.Contains(normalizedActual, expectedMsgFragment)
611613
}
612614
}
613615
return waitForCondition(ctx, "clusterobjectset", substituteScenarioVars(revisionName, scenarioCtx(ctx)), conditionType, conditionStatus, &conditionReason, msgCmp)

0 commit comments

Comments
 (0)