Skip to content

Commit d840c14

Browse files
refactor: Use resolution digest instead of version-only check
Replace simple version comparison with comprehensive SHA256 digest of all catalog filter inputs (version, channels, selector, upgradeConstraintPolicy). Benefits: - Detects ALL catalog filter changes, not just version - Future-proof for new filter fields - Follows container/OCI digest pattern (sha256:...) - More correct: changing channels while rolling out now creates new revision Implementation: - Add calculateResolutionDigest() to hash catalog filter inputs - Add ResolutionDigest field to RevisionMetadata - Store digest as annotation on revisions (labels.ResolutionDigestKey) - Compare digests to detect spec changes during rollout Test fixes: - TestClusterExtensionBoxcutterApplierFailsDoesNotLeakDeprecationErrors: Updated expectations to match correct behavior - when resolution succeeds with deprecation=nil, we have catalog data and should set Deprecated=False Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
1 parent 7452c0b commit d840c14

File tree

5 files changed

+102
-39
lines changed

5 files changed

+102
-39
lines changed

internal/operator-controller/controllers/boxcutter_reconcile_steps.go

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -62,10 +62,11 @@ func (d *BoxcutterRevisionStatesGetter) GetRevisionStates(ctx context.Context, e
6262
// is fairly decoupled from this code where we get the annotations back out. We may want to co-locate
6363
// the set/get logic a bit better to make it more maintainable and less likely to get out of sync.
6464
rm := &RevisionMetadata{
65-
RevisionName: rev.Name,
66-
Package: rev.Annotations[labels.PackageNameKey],
67-
Image: rev.Annotations[labels.BundleReferenceKey],
68-
Conditions: rev.Status.Conditions,
65+
RevisionName: rev.Name,
66+
Package: rev.Annotations[labels.PackageNameKey],
67+
Image: rev.Annotations[labels.BundleReferenceKey],
68+
ResolutionDigest: rev.Annotations[labels.ResolutionDigestKey],
69+
Conditions: rev.Status.Conditions,
6970
BundleMetadata: ocv1.BundleMetadata{
7071
Name: rev.Annotations[labels.BundleNameKey],
7172
Version: rev.Annotations[labels.BundleVersionKey],
@@ -100,10 +101,11 @@ func ApplyBundleWithBoxcutter(apply func(ctx context.Context, contentFS fs.FS, e
100101
return func(ctx context.Context, state *reconcileState, ext *ocv1.ClusterExtension) (*ctrl.Result, error) {
101102
l := log.FromContext(ctx)
102103
revisionAnnotations := map[string]string{
103-
labels.BundleNameKey: state.resolvedRevisionMetadata.Name,
104-
labels.PackageNameKey: state.resolvedRevisionMetadata.Package,
105-
labels.BundleVersionKey: state.resolvedRevisionMetadata.Version,
106-
labels.BundleReferenceKey: state.resolvedRevisionMetadata.Image,
104+
labels.BundleNameKey: state.resolvedRevisionMetadata.Name,
105+
labels.PackageNameKey: state.resolvedRevisionMetadata.Package,
106+
labels.BundleVersionKey: state.resolvedRevisionMetadata.Version,
107+
labels.BundleReferenceKey: state.resolvedRevisionMetadata.Image,
108+
labels.ResolutionDigestKey: state.resolvedRevisionMetadata.ResolutionDigest,
107109
}
108110
objLbls := map[string]string{
109111
labels.OwnerKindKey: ocv1.ClusterExtensionKind,

internal/operator-controller/controllers/clusterextension_controller.go

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -499,9 +499,10 @@ func clusterExtensionRequestsForCatalog(c client.Reader, logger logr.Logger) crh
499499
}
500500

501501
type RevisionMetadata struct {
502-
RevisionName string
503-
Package string
504-
Image string
502+
RevisionName string
503+
Package string
504+
Image string
505+
ResolutionDigest string
505506
ocv1.BundleMetadata
506507
Conditions []metav1.Condition
507508
}
@@ -535,8 +536,9 @@ func (d *HelmRevisionStatesGetter) GetRevisionStates(ctx context.Context, ext *o
535536
for _, rel := range relhis {
536537
if rel.Info != nil && rel.Info.Status == release.StatusDeployed {
537538
rs.Installed = &RevisionMetadata{
538-
Package: rel.Labels[labels.PackageNameKey],
539-
Image: rel.Labels[labels.BundleReferenceKey],
539+
Package: rel.Labels[labels.PackageNameKey],
540+
Image: rel.Labels[labels.BundleReferenceKey],
541+
ResolutionDigest: rel.Labels[labels.ResolutionDigestKey],
540542
BundleMetadata: ocv1.BundleMetadata{
541543
Name: rel.Labels[labels.BundleNameKey],
542544
Version: rel.Labels[labels.BundleVersionKey],

internal/operator-controller/controllers/clusterextension_controller_test.go

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -786,7 +786,10 @@ func TestClusterExtensionBoxcutterApplierFailsDoesNotLeakDeprecationErrors(t *te
786786
d.RevisionStatesGetter = &MockRevisionStatesGetter{
787787
RevisionStates: &controllers.RevisionStates{
788788
RollingOut: []*controllers.RevisionMetadata{{
789-
BundleMetadata: ocv1.BundleMetadata{Version: "1.0.0"},
789+
// Different digest from current spec - ensures we call the resolver
790+
// (This simulates a rolling-out revision from a previous reconcile)
791+
ResolutionDigest: "different-digest-from-previous-reconcile",
792+
BundleMetadata: ocv1.BundleMetadata{Version: "1.0.0"},
790793
}},
791794
},
792795
}
@@ -847,21 +850,18 @@ func TestClusterExtensionBoxcutterApplierFailsDoesNotLeakDeprecationErrors(t *te
847850

848851
deprecatedCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1.TypeDeprecated)
849852
require.NotNil(t, deprecatedCond)
850-
require.Equal(t, metav1.ConditionUnknown, deprecatedCond.Status, "no catalog data during rollout, so Unknown")
851-
require.Equal(t, ocv1.ReasonDeprecationStatusUnknown, deprecatedCond.Reason)
852-
require.Equal(t, "deprecation status unknown: catalog data unavailable", deprecatedCond.Message)
853+
require.Equal(t, metav1.ConditionFalse, deprecatedCond.Status, "catalog data available from resolution, not deprecated")
854+
require.Equal(t, ocv1.ReasonNotDeprecated, deprecatedCond.Reason)
853855

854856
packageCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1.TypePackageDeprecated)
855857
require.NotNil(t, packageCond)
856-
require.Equal(t, metav1.ConditionUnknown, packageCond.Status, "no catalog data during rollout, so Unknown")
857-
require.Equal(t, ocv1.ReasonDeprecationStatusUnknown, packageCond.Reason)
858-
require.Equal(t, "deprecation status unknown: catalog data unavailable", packageCond.Message)
858+
require.Equal(t, metav1.ConditionFalse, packageCond.Status, "catalog data available from resolution, package not deprecated")
859+
require.Equal(t, ocv1.ReasonNotDeprecated, packageCond.Reason)
859860

860861
channelCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1.TypeChannelDeprecated)
861862
require.NotNil(t, channelCond)
862-
require.Equal(t, metav1.ConditionUnknown, channelCond.Status, "no catalog data during rollout, so Unknown")
863-
require.Equal(t, ocv1.ReasonDeprecationStatusUnknown, channelCond.Reason)
864-
require.Equal(t, "deprecation status unknown: catalog data unavailable", channelCond.Message)
863+
require.Equal(t, metav1.ConditionFalse, channelCond.Status, "catalog data available from resolution, channel not deprecated")
864+
require.Equal(t, ocv1.ReasonNotDeprecated, channelCond.Reason)
865865

866866
bundleCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1.TypeBundleDeprecated)
867867
require.NotNil(t, bundleCond)

internal/operator-controller/controllers/clusterextension_reconcile_steps.go

Lines changed: 70 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@ package controllers
1818

1919
import (
2020
"context"
21+
"crypto/sha256"
22+
"encoding/json"
2123
"errors"
2224
"fmt"
2325

@@ -37,6 +39,49 @@ import (
3739
imageutil "github.com/operator-framework/operator-controller/internal/shared/util/image"
3840
)
3941

42+
// resolutionInputs captures the catalog filter fields that affect bundle resolution.
43+
// Changes to any of these fields require re-resolution.
44+
type resolutionInputs struct {
45+
Version string `json:"version"`
46+
Channels []string `json:"channels,omitempty"`
47+
Selector string `json:"selector,omitempty"`
48+
UpgradeConstraintPolicy string `json:"upgradeConstraintPolicy,omitempty"`
49+
}
50+
51+
// calculateResolutionDigest computes a SHA256 hash of the catalog filter inputs
52+
// that affect bundle resolution. This digest enables detection of spec changes that
53+
// require re-resolution versus when a rolling-out revision can be reused.
54+
func calculateResolutionDigest(catalog *ocv1.CatalogFilter) (string, error) {
55+
if catalog == nil {
56+
return "", nil
57+
}
58+
59+
inputs := resolutionInputs{
60+
Version: catalog.Version,
61+
Channels: catalog.Channels,
62+
UpgradeConstraintPolicy: string(catalog.UpgradeConstraintPolicy),
63+
}
64+
65+
// Convert selector to a canonical string representation
66+
if catalog.Selector != nil {
67+
selectorBytes, err := json.Marshal(catalog.Selector)
68+
if err != nil {
69+
return "", fmt.Errorf("marshaling selector: %w", err)
70+
}
71+
inputs.Selector = string(selectorBytes)
72+
}
73+
74+
// Marshal to JSON for consistent hashing
75+
inputsBytes, err := json.Marshal(inputs)
76+
if err != nil {
77+
return "", fmt.Errorf("marshaling resolution inputs: %w", err)
78+
}
79+
80+
// Compute SHA256 hash and return hex string
81+
hash := sha256.Sum256(inputsBytes)
82+
return fmt.Sprintf("%x", hash), nil
83+
}
84+
4085
func HandleFinalizers(f finalizer.Finalizer) ReconcileStepFunc {
4186
return func(ctx context.Context, state *reconcileState, ext *ocv1.ClusterExtension) (*ctrl.Result, error) {
4287
l := log.FromContext(ctx)
@@ -141,22 +186,22 @@ func ResolveBundle(r resolve.Resolver, c client.Client) ReconcileStepFunc {
141186
l := log.FromContext(ctx)
142187

143188
// If already rolling out, check if the latest rolling-out revision matches the current spec.
144-
// If spec has changed (e.g., version updated), we need to resolve a new bundle instead of
145-
// reusing the rolling-out revision.
189+
// If spec has changed (e.g., version, channels, selector, upgradeConstraintPolicy), we need
190+
// to resolve a new bundle instead of reusing the rolling-out revision.
146191
//
147192
// Note: RollingOut slice is sorted oldest→newest, so use the last element (most recent).
148193
// This avoids re-resolving when a newer revision already matches the spec, even if an
149194
// older revision is still stuck rolling out.
150195
if len(state.revisionStates.RollingOut) > 0 {
151196
latestRollingOutRevision := state.revisionStates.RollingOut[len(state.revisionStates.RollingOut)-1]
152-
specVersion := ""
153-
if ext.Spec.Source.Catalog != nil {
154-
specVersion = ext.Spec.Source.Catalog.Version
155-
}
156197

157-
// If spec version matches latest rolling-out revision version (or no version specified),
158-
// reuse the existing rolling-out revision
159-
if specVersion == "" || specVersion == latestRollingOutRevision.Version {
198+
// Calculate digest of current spec's catalog filter
199+
currentDigest, err := calculateResolutionDigest(ext.Spec.Source.Catalog)
200+
if err != nil {
201+
l.Error(err, "failed to calculate resolution digest from spec")
202+
// On digest calculation error, fall through to re-resolve for safety
203+
} else if currentDigest == latestRollingOutRevision.ResolutionDigest {
204+
// Resolution inputs haven't changed - reuse the rolling-out revision
160205
installedBundleName := ""
161206
if state.revisionStates.Installed != nil {
162207
installedBundleName = state.revisionStates.Installed.Name
@@ -165,7 +210,7 @@ func ResolveBundle(r resolve.Resolver, c client.Client) ReconcileStepFunc {
165210
state.resolvedRevisionMetadata = latestRollingOutRevision
166211
return nil, nil
167212
}
168-
// Spec version changed - fall through to resolve new bundle from catalog
213+
// Resolution inputs changed - fall through to resolve new bundle from catalog
169214
}
170215

171216
// Resolve a new bundle from the catalog
@@ -205,9 +250,17 @@ func ResolveBundle(r resolve.Resolver, c client.Client) ReconcileStepFunc {
205250
return handleResolutionError(ctx, c, state, ext, err)
206251
}
207252

253+
// Calculate digest of the resolution inputs for change detection
254+
digest, err := calculateResolutionDigest(ext.Spec.Source.Catalog)
255+
if err != nil {
256+
l.Error(err, "failed to calculate resolution digest, continuing without it")
257+
digest = ""
258+
}
259+
208260
state.resolvedRevisionMetadata = &RevisionMetadata{
209-
Package: resolvedBundle.Package,
210-
Image: resolvedBundle.Image,
261+
Package: resolvedBundle.Package,
262+
Image: resolvedBundle.Image,
263+
ResolutionDigest: digest,
211264
// TODO: Right now, operator-controller only supports registry+v1 bundles and has no concept
212265
// of a "release" field. If/when we add a release field concept or a new bundle format
213266
// we need to re-evaluate use of `AsLegacyRegistryV1Version` so that we avoid propagating
@@ -426,10 +479,11 @@ func ApplyBundle(a Applier) ReconcileStepFunc {
426479
return func(ctx context.Context, state *reconcileState, ext *ocv1.ClusterExtension) (*ctrl.Result, error) {
427480
l := log.FromContext(ctx)
428481
revisionAnnotations := map[string]string{
429-
labels.BundleNameKey: state.resolvedRevisionMetadata.Name,
430-
labels.PackageNameKey: state.resolvedRevisionMetadata.Package,
431-
labels.BundleVersionKey: state.resolvedRevisionMetadata.Version,
432-
labels.BundleReferenceKey: state.resolvedRevisionMetadata.Image,
482+
labels.BundleNameKey: state.resolvedRevisionMetadata.Name,
483+
labels.PackageNameKey: state.resolvedRevisionMetadata.Package,
484+
labels.BundleVersionKey: state.resolvedRevisionMetadata.Version,
485+
labels.BundleReferenceKey: state.resolvedRevisionMetadata.Image,
486+
labels.ResolutionDigestKey: state.resolvedRevisionMetadata.ResolutionDigest,
433487
}
434488
objLbls := map[string]string{
435489
labels.OwnerKindKey: ocv1.ClusterExtensionKind,

internal/operator-controller/labels/labels.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,4 +57,9 @@ const (
5757
// that were created during migration from Helm releases. This label is used
5858
// to distinguish migrated revisions from those created by normal Boxcutter operation.
5959
MigratedFromHelmKey = "olm.operatorframework.io/migrated-from-helm"
60+
61+
// ResolutionDigestKey stores a SHA256 hash of the catalog filter inputs
62+
// (version, channels, selector, upgradeConstraintPolicy) used during bundle resolution.
63+
// The controller compares this digest to detect spec changes that require re-resolution.
64+
ResolutionDigestKey = "olm.operatorframework.io/resolution-digest"
6065
)

0 commit comments

Comments
 (0)