Skip to content

Refactor OwnerStrategy to RevisionMetadata #427

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mdbooth wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Refactor OwnerStrategy to RevisionMetadata #427

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
da25e2a
Refactor OwnerStrategy to RevisionMetadata
mdbooth Jan 20, 2026
bdf204b
Add generic RevisionImpl
mdbooth Feb 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 21 additions & 24 deletions boxcutter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,20 +3,26 @@ package boxcutter

import (
"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/client-go/discovery"
"sigs.k8s.io/controller-runtime/pkg/client"

"pkg.package-operator.run/boxcutter/machinery"
"pkg.package-operator.run/boxcutter/machinery/types"
"pkg.package-operator.run/boxcutter/ownerhandling"
"pkg.package-operator.run/boxcutter/validation"
)

// NewRevision creates a new Revision instance using type inference to determine the concrete type of the revision metadata.
func NewRevision[T types.RevisionMetadata](name string, metadata T, revision int64, phases []types.Phase) *RevisionImpl[T] {
return types.NewRevision(name, metadata, revision, phases)
}

// Revision represents multiple phases at a given point in time.
type Revision = types.Revision

// RevisionImpl is an implementation of the Revision interface whose revision metadata is of concrete type T.
type RevisionImpl[T types.RevisionMetadata] = types.RevisionImpl[T]

// Phase represents a collection of objects lifecycled together.
type Phase = types.Phase

Expand Down Expand Up @@ -92,15 +98,11 @@ const ProgressProbeType = types.ProgressProbeType
// RevisionEngine manages rollout and teardown of multiple phases.
type RevisionEngine = machinery.RevisionEngine

// OwnerStrategy interface needed for RevisionEngine.
type OwnerStrategy interface {
SetControllerReference(owner, obj metav1.Object) error
GetController(obj metav1.Object) (metav1.OwnerReference, bool)
IsController(owner, obj metav1.Object) bool
CopyOwnerReferences(objA, objB metav1.Object)
ReleaseController(obj metav1.Object)
RemoveOwner(owner, obj metav1.Object)
}
// RevisionMetadata is the interface for managing ownership metadata.
type RevisionMetadata = types.RevisionMetadata

// RevisionReference is the interface for revision reference information.
type RevisionReference = types.RevisionReference

// RevisionEngineOptions holds all configuration options for the RevisionEngine.
type RevisionEngineOptions struct {
Expand All @@ -114,7 +116,6 @@ type RevisionEngineOptions struct {

// Optional

OwnerStrategy OwnerStrategy
PhaseValidator *validation.PhaseValidator
}

Expand All @@ -124,20 +125,16 @@ func NewPhaseEngine(opts RevisionEngineOptions) (*machinery.PhaseEngine, error)
return nil, err
}

if opts.OwnerStrategy == nil {
opts.OwnerStrategy = ownerhandling.NewNative(opts.Scheme)
}

if opts.PhaseValidator == nil {
opts.PhaseValidator = validation.NewNamespacedPhaseValidator(opts.RestMapper, opts.Writer)
opts.PhaseValidator = validation.NewPhaseValidator(opts.RestMapper, opts.Writer)
}

comp := machinery.NewComparator(
opts.OwnerStrategy, opts.DiscoveryClient, opts.Scheme, opts.FieldOwner)
opts.DiscoveryClient, opts.Scheme, opts.FieldOwner)

oe := machinery.NewObjectEngine(
opts.Scheme, opts.Reader, opts.Writer,
opts.OwnerStrategy, comp, opts.FieldOwner, opts.SystemPrefix,
comp, opts.FieldOwner, opts.SystemPrefix,
)

return machinery.NewPhaseEngine(oe, opts.PhaseValidator), nil
Expand All @@ -149,19 +146,19 @@ func NewRevisionEngine(opts RevisionEngineOptions) (*RevisionEngine, error) {
return nil, err
}

if opts.OwnerStrategy == nil {
opts.OwnerStrategy = ownerhandling.NewNative(opts.Scheme)
pval := opts.PhaseValidator
if pval == nil {
pval = validation.NewPhaseValidator(opts.RestMapper, opts.Writer)
}

pval := validation.NewNamespacedPhaseValidator(opts.RestMapper, opts.Writer)
rval := validation.NewRevisionValidator()

comp := machinery.NewComparator(
opts.OwnerStrategy, opts.DiscoveryClient, opts.Scheme, opts.FieldOwner)
opts.DiscoveryClient, opts.Scheme, opts.FieldOwner)

oe := machinery.NewObjectEngine(
opts.Scheme, opts.Reader, opts.Writer,
opts.OwnerStrategy, comp, opts.FieldOwner, opts.SystemPrefix,
comp, opts.FieldOwner, opts.SystemPrefix,
)
pe := machinery.NewPhaseEngine(oe, pval)

Expand Down
44 changes: 26 additions & 18 deletions cmd/reference/internal/deploy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (

"pkg.package-operator.run/boxcutter"
"pkg.package-operator.run/boxcutter/managedcache"
"pkg.package-operator.run/boxcutter/ownerhandling"
"pkg.package-operator.run/boxcutter/probing"
"pkg.package-operator.run/boxcutter/util"
)
Expand All @@ -43,6 +44,10 @@ const (
revisionHistoryLimit = 5
)

// revisionT is a type alias for a Revision using NativeRevisionMetadata for metadata.
// It is the revision type used by the reconciler.
type revisionT = boxcutter.RevisionImpl[*ownerhandling.NativeRevisionMetadata]

type Reconciler struct {
client client.Client
discoveryClient *discovery.DiscoveryClient
Expand Down Expand Up @@ -122,15 +127,15 @@ func (c *Reconciler) handleDeployment(ctx context.Context, cm *corev1.ConfigMap)
return res, fmt.Errorf("listing revisions: %w", err)
}

existingRevisions := make([]boxcutter.Revision, 0, len(existingRevisionsRaw.Items))
existingRevisions := make([]*revisionT, 0, len(existingRevisionsRaw.Items))

for _, rev := range existingRevisionsRaw.Items {
r, _, _, err := c.toRevision(cm.Name, &rev)
if err != nil {
return res, fmt.Errorf("to revision: %w", err)
}

existingRevisions = append(existingRevisions, *r)
existingRevisions = append(existingRevisions, r)
}

sort.Sort(revisionAscending(existingRevisions))
Expand All @@ -139,18 +144,18 @@ func (c *Reconciler) handleDeployment(ctx context.Context, cm *corev1.ConfigMap)

// Sort into current and previous revisions.
var (
currentRevision *boxcutter.Revision
prevRevisions []boxcutter.Revision
currentRevision *revisionT
prevRevisions []*revisionT
)

if len(existingRevisions) > 0 {
maybeCurrentObjectSet := existingRevisions[len(existingRevisions)-1]

annotations := maybeCurrentObjectSet.GetOwner().GetAnnotations()
annotations := getNativeOwner(maybeCurrentObjectSet).GetAnnotations()
if annotations != nil {
if hash, ok := annotations[hashAnnotation]; ok &&
hash == currentHash {
currentRevision = &maybeCurrentObjectSet
currentRevision = maybeCurrentObjectSet
prevRevisions = existingRevisions[0 : len(existingRevisions)-1] // previous is everything excluding current
}
}
Expand Down Expand Up @@ -197,7 +202,7 @@ func (c *Reconciler) handleDeployment(ctx context.Context, cm *corev1.ConfigMap)
break
}

if err := client.IgnoreNotFound(c.client.Delete(ctx, prevRev.GetOwner())); err != nil {
if err := client.IgnoreNotFound(c.client.Delete(ctx, getNativeOwner(prevRev))); err != nil {
return res, fmt.Errorf("failed to delete revision (history limit): %w", err)
}

Expand Down Expand Up @@ -243,7 +248,7 @@ func (c *Reconciler) handleRevision(

if !revisionCM.DeletionTimestamp.IsZero() ||
revisionCM.Data[cmStateKey] == "Archived" {
tres, err := re.Teardown(ctx, *revision)
tres, err := re.Teardown(ctx, revision)
if err != nil {
return res, fmt.Errorf("revision teardown: %w", err)
}
Expand All @@ -270,7 +275,7 @@ func (c *Reconciler) handleRevision(
return res, err
}

rres, err := re.Reconcile(ctx, *revision, opts...)
rres, err := re.Reconcile(ctx, revision, opts...)
if err != nil {
return res, fmt.Errorf("revision reconcile: %w", err)
}
Expand Down Expand Up @@ -330,7 +335,7 @@ func (e RevisionNumberNotSetError) Error() string {
}

func (c *Reconciler) toRevision(deployName string, cm *corev1.ConfigMap) (
r *boxcutter.Revision, opts []boxcutter.RevisionReconcileOption, previous []client.Object, err error,
r *revisionT, opts []boxcutter.RevisionReconcileOption, previous []client.Object, err error,
) {
var (
phases []string
Expand Down Expand Up @@ -399,14 +404,17 @@ func (c *Reconciler) toRevision(deployName string, cm *corev1.ConfigMap) (
return nil, nil, nil, RevisionNumberNotSetError{msg: "revision not set"}
}

rev := &boxcutter.Revision{
Name: cm.Name,
Owner: cm,
Revision: revision,
}
rev := boxcutter.NewRevision(
cm.Name,
ownerhandling.NewNativeRevisionMetadata(cm, c.scheme),
revision,
nil,
)

for _, obj := range previousUnstr {
previous = append(previous, &obj)
previousMetadata := make([]boxcutter.RevisionMetadata, len(previousUnstr))
for i := range previousUnstr {
previousMetadata[i] = ownerhandling.NewNativeRevisionMetadata(&previousUnstr[i], c.scheme)
previous = append(previous, &previousUnstr[i])
}
Comment on lines +414 to 418
Copy link

@coderabbitai coderabbitai bot Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# First, let's locate the file and understand the context around lines 409-413
head -n 420 cmd/reference/internal/deploy.go | tail -n 25

Repository: package-operator/boxcutter

Length of output: 724

🏁 Script executed:

#!/bin/bash
# Search for ownerhandling.NewNativeRevisionMetadata to understand what it does
rg -n "NewNativeRevisionMetadata" -C5

Repository: package-operator/boxcutter

Length of output: 26530

🏁 Script executed:

#!/bin/bash
# Search for cmPreviousKey to understand where previousUnstr comes from
rg -n "cmPreviousKey" -C5

Repository: package-operator/boxcutter

Length of output: 2094

🏁 Script executed:

#!/bin/bash
# Search for the implementation of NewNativeRevisionMetadata in the ownerhandling package
fd -t f "ownerhandling" --type f -x grep -l "NewNativeRevisionMetadata" {}

Repository: package-operator/boxcutter

Length of output: 52

🏁 Script executed:

#!/bin/bash
# Find the prevJSON function to see how revisions are serialized
rg -n "func prevJSON" -A 10

Repository: package-operator/boxcutter

Length of output: 873

🏁 Script executed:

#!/bin/bash
# Also search for how previousUnstr is defined/typed
rg -n "previousUnstr" -B 5 -A 5

Repository: package-operator/boxcutter

Length of output: 2397

🏁 Script executed:

#!/bin/bash
# Look at the full context around line 365 where previousUnstr is unmarshalled
sed -n '355,375p' cmd/reference/internal/deploy.go

Repository: package-operator/boxcutter

Length of output: 462

🏁 Script executed:

#!/bin/bash
# Let's see the complete prevJSON function
sed -n '39,60p' cmd/reference/internal/util.go

Repository: package-operator/boxcutter

Length of output: 667

🏁 Script executed:

#!/bin/bash
# Check the unstructured.Unstructured marshaling/unmarshaling behavior
# and see if there are any tests for prevJSON
rg -n "prevJSON" -B 2 -A 2

Repository: package-operator/boxcutter

Length of output: 790

🏁 Script executed:

#!/bin/bash
# Look at getNativeOwner to understand what's being serialized
rg -n "func getNativeOwner" -A 10

Repository: package-operator/boxcutter

Length of output: 871

🏁 Script executed:

#!/bin/bash
# Let's search for tests related to prevJSON or the round-trip serialization
rg -n "prevJSON" -B 5 -A 10

Repository: package-operator/boxcutter

Length of output: 2287

🏁 Script executed:

#!/bin/bash
# Check if there are any tests that verify the previous revisions deserialization
rg -n "cmPreviousKey\|previousUnstr" test/ -B 3 -A 3

Repository: package-operator/boxcutter

Length of output: 52

🏁 Script executed:

#!/bin/bash
# Let's understand how unstructured.Unstructured serializes metadata by checking the marshal behavior
# Search for any tests or code that deals with UID in unstructured objects
rg -n "SetUID\|GetUID" cmd/reference/internal/ -B 2 -A 2

Repository: package-operator/boxcutter

Length of output: 52

🏁 Script executed:

#!/bin/bash
# Let me check how unstructured.Unstructured JSON marshals/unmarshals
# by looking at what fields are included when serializing
cat > /tmp/test_unstructured.go << 'EOF'
package main

import (
	"encoding/json"
	"fmt"
	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
	"k8s.io/apimachinery/pkg/types"
)

func main() {
	// Create an unstructured object with UID
	u := &unstructured.Unstructured{}
	u.SetUID(types.UID("test-uid-123"))
	u.SetName("test-name")
	u.SetNamespace("test-ns")
	
	// Marshal it
	data, _ := json.MarshalIndent(u, "", "  ")
	fmt.Println("Marshalled unstructured with UID:")
	fmt.Println(string(data))
	
	// Now unmarshal without UID in JSON
	jsonNoUID := `{"metadata":{"name":"test","namespace":"ns"}}`
	u2 := &unstructured.Unstructured{}
	json.Unmarshal([]byte(jsonNoUID), u2)
	fmt.Println("\nUnmarshalled without UID, GetUID() returns:")
	fmt.Printf("%q\n", u2.GetUID())
}
EOF
go run /tmp/test_unstructured.go

Repository: package-operator/boxcutter

Length of output: 1259

🏁 Script executed:

#!/bin/bash
# Check if there are tests for the toRevision function
rg -n "toRevision" test/ -B 3 -A 10

Repository: package-operator/boxcutter

Length of output: 52

🏁 Script executed:

#!/bin/bash
# Look at the actual deployment flow to see if previousUnstr can come from untrusted sources
sed -n '160,200p' cmd/reference/internal/deploy.go

Repository: package-operator/boxcutter

Length of output: 1264

🏁 Script executed:

#!/bin/bash
# Let me verify that the prevJSON function is the only place where 
# cmPreviousKey is written to understand the full data flow
rg -n "cmPreviousKey.*=" -B 2 -A 2

Repository: package-operator/boxcutter

Length of output: 805

🏁 Script executed:

#!/bin/bash
# Check if there are any version migrations or data transformations
# that might affect UID preservation
fd -type f -name "*.go" cmd/reference/internal/ -x grep -l "migrate\|version\|upgrade"

Repository: package-operator/boxcutter

Length of output: 240

🏁 Script executed:

#!/bin/bash
# Let me check for tests of the Reconciler.toRevision function
find cmd/reference/internal -name "*test.go" -exec grep -l "toRevision" {} \;

Repository: package-operator/boxcutter

Length of output: 52

🏁 Script executed:

#!/bin/bash
# Also check if there are any integration tests or test fixtures that might reveal this issue
fd -t f "test" cmd/reference/internal/ -name "*.go"

Repository: package-operator/boxcutter

Length of output: 300

🏁 Script executed:

#!/bin/bash
# Look for any existing tests in the deploy package
find . -path "*/cmd/reference/internal/*test*.go" -o -path "*/cmd/reference/*test*.go"

Repository: package-operator/boxcutter

Length of output: 52

🏁 Script executed:

#!/bin/bash
# Let's look at the complete toRevision function to understand all paths where previousUnstr could be empty
sed -n '333,450p' cmd/reference/internal/deploy.go

Repository: package-operator/boxcutter

Length of output: 3000

🏁 Script executed:

#!/bin/bash
# Let me verify the complete flow one more time by checking if there are any 
# code paths where previousUnstr could be created without UID
rg -n "previousUnstr\s*=" -B 3 -A 3

Repository: package-operator/boxcutter

Length of output: 52

🏁 Script executed:

#!/bin/bash
# Check if there's any version-related logic or data migration
rg -n "Data\[cmPreviousKey" -B 5 -A 5

Repository: package-operator/boxcutter

Length of output: 799

🏁 Script executed:

#!/bin/bash
# Look for any admission webhooks, validation rules, or safeguards on the ConfigMap
fd -type f "webhook\|validation" cmd/reference/internal

Repository: package-operator/boxcutter

Length of output: 240

🏁 Script executed:

#!/bin/bash
# Let me search for any validation of previousUnstr after unmarshalling
sed -n '364,415p' cmd/reference/internal/deploy.go

Repository: package-operator/boxcutter

Length of output: 1351

🏁 Script executed:

#!/bin/bash
# Check if there's any error handling after the json.Unmarshal of previousUnstr
rg -n "json.Unmarshal.*previousUnstr" -A 15

Repository: package-operator/boxcutter

Length of output: 1020

🏁 Script executed:

#!/bin/bash
# Let me check if there are any other places besides prevJSON that could write to cmPreviousKey
rg -n "cmPreviousKey\|Data\[\"previous\"\]"

Repository: package-operator/boxcutter

Length of output: 52

🏁 Script executed:

#!/bin/bash
# Also check if there are any conversion/migration functions
rg -n "func.*convert\|func.*migrate" cmd/reference/internal/

Repository: package-operator/boxcutter

Length of output: 52

🏁 Script executed:

#!/bin/bash
# Let me check if the ConfigMap could come from external sources (users, specs, etc)
sed -n '100,160p' cmd/reference/internal/deploy.go

Repository: package-operator/boxcutter

Length of output: 1744

🏁 Script executed:

#!/bin/bash
# Let me trace the complete flow - see how ConfigMaps get to toRevision
# and understand if there's any scenario where previous data could be corrupted
rg -n "toRevision" -B 5 -A 2

Repository: package-operator/boxcutter

Length of output: 1772

🏁 Script executed:

#!/bin/bash
# Check if there's any documentation about this or TODOs
rg -n "TODO\|FIXME\|BUG\|panic" cmd/reference/internal/deploy.go

Repository: package-operator/boxcutter

Length of output: 52

🏁 Script executed:

#!/bin/bash
# Let me verify if previous revisions are ONLY set by prevJSON or if there's any other source
rg -n "previous.*=" cmd/reference/internal/ | head -20

Repository: package-operator/boxcutter

Length of output: 613

Guard against missing UID in previous revision metadata.

NewNativeRevisionMetadata panics when UID is empty. If the previous field in ConfigMap data lacks a UID in any entry (e.g., from manual creation or external edits), reconciliation will crash. Add validation before constructing metadata.

🔧 Suggested guard to avoid panic and return a clear error 
-previousMetadata := make([]boxcutter.RevisionMetadata, len(previousUnstr))
-for i := range previousUnstr {
-	previousMetadata[i] = ownerhandling.NewNativeRevisionMetadata(&previousUnstr[i], c.scheme)
-	previous = append(previous, &previousUnstr[i])
-}
+previousMetadata := make([]boxcutter.RevisionMetadata, 0, len(previousUnstr))
+for i := range previousUnstr {
+	if len(previousUnstr[i].GetUID()) == 0 {
+		return nil, nil, nil, fmt.Errorf("previous revision %q missing UID", previousUnstr[i].GetName())
+	}
+	previousMetadata = append(previousMetadata, ownerhandling.NewNativeRevisionMetadata(&previousUnstr[i], c.scheme))
+	previous = append(previous, &previousUnstr[i])
+}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
previousMetadata := make([]boxcutter.RevisionMetadata, len(previousUnstr))
for i := range previousUnstr {
previousMetadata[i] = ownerhandling.NewNativeRevisionMetadata(&previousUnstr[i], c.scheme)
previous = append(previous, &previousUnstr[i])
}
previousMetadata := make([]boxcutter.RevisionMetadata, 0, len(previousUnstr))
for i := range previousUnstr {
if len(previousUnstr[i].GetUID()) == 0 {
return nil, nil, nil, fmt.Errorf("previous revision %q missing UID", previousUnstr[i].GetName())
}
previousMetadata = append(previousMetadata, ownerhandling.NewNativeRevisionMetadata(&previousUnstr[i], c.scheme))
previous = append(previous, &previousUnstr[i])
}
🤖 Prompt for AI Agents 
In `@cmd/reference/internal/deploy.go` around lines 409 - 413, The code calls
ownerhandling.NewNativeRevisionMetadata(&previousUnstr[i], c.scheme) which
panics if the Revision UID is empty; before constructing previousMetadata (and
before appending to previous), validate that previousUnstr[i].GetUID() (or the
UID field on the unstructured) is non-empty, and if empty return a clear error
explaining "missing UID in previous revision metadata" (or skip with a logged
warning if desired). Update the block manipulating previousUnstr,
previousMetadata, and previous to perform this guard and error return so
NewNativeRevisionMetadata is never invoked with an empty UID.


for _, phase := range phases {
Expand All @@ -419,7 +427,7 @@ func (c *Reconciler) toRevision(deployName string, cm *corev1.ConfigMap) (
}

opts = []boxcutter.RevisionReconcileOption{
boxcutter.WithPreviousOwners(previous),
boxcutter.WithPreviousOwners(previousMetadata),
boxcutter.WithProbe(
boxcutter.ProgressProbeType,
boxcutter.ProbeFunc(func(obj client.Object) probing.Result {
Expand Down
14 changes: 8 additions & 6 deletions cmd/reference/internal/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,9 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"sigs.k8s.io/controller-runtime/pkg/client"

bctypes "pkg.package-operator.run/boxcutter/machinery/types"
)

type revisionAscending []bctypes.Revision
type revisionAscending []*revisionT

func (a revisionAscending) Len() int { return len(a) }
func (a revisionAscending) Swap(i, j int) { a[i], a[j] = a[j], a[i] }
Expand All @@ -21,19 +19,23 @@ func (a revisionAscending) Less(i, j int) bool {
return iObj.GetRevisionNumber() < jObj.GetRevisionNumber()
}

func latestRevisionNumber(prevRevisions []bctypes.Revision) int64 {
func latestRevisionNumber(prevRevisions []*revisionT) int64 {
if len(prevRevisions) == 0 {
return 0
}

return prevRevisions[len(prevRevisions)-1].GetRevisionNumber()
}

func prevJSON(prevRevisions []bctypes.Revision) string {
func getNativeOwner(revision *revisionT) client.Object {
return revision.Metadata.GetOwner()
}

func prevJSON(prevRevisions []*revisionT) string {
data := make([]unstructured.Unstructured, 0, len(prevRevisions))

for _, rev := range prevRevisions {
refObj := rev.GetOwner()
refObj := getNativeOwner(rev)
ref := unstructured.Unstructured{}
ref.SetGroupVersionKind(refObj.GetObjectKind().GroupVersionKind())
ref.SetName(refObj.GetName())
Expand Down
13 changes: 4 additions & 9 deletions machinery/comparator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,15 @@ import (
"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
"sigs.k8s.io/structured-merge-diff/v6/fieldpath"
"sigs.k8s.io/structured-merge-diff/v6/typed"

"pkg.package-operator.run/boxcutter/machinery/types"
)

// Comparator detects divergent state between desired and actual
// by comparing managed field ownerships.
// If not all fields from desired are owned by the same field owner in actual,
// we know that the object has been updated by another actor.
type Comparator struct {
ownerStrategy divergeDetectorOwnerStrategy
openAPIAccessor openAPIAccessor
scheme *runtime.Scheme
fieldOwner string
Expand All @@ -38,23 +39,17 @@ type discoveryClient interface {
OpenAPIV3() openapi.Client
}

type divergeDetectorOwnerStrategy interface {
SetControllerReference(owner, obj metav1.Object) error
}

type openAPIAccessor interface {
Get(gv schema.GroupVersion) (*spec3.OpenAPI, error)
}

// NewComparator returns a new Comparator instance.
func NewComparator(
ownerStrategy divergeDetectorOwnerStrategy,
discoveryClient discoveryClient,
scheme *runtime.Scheme,
fieldOwner string,
) *Comparator {
return &Comparator{
ownerStrategy: ownerStrategy,
openAPIAccessor: &defaultOpenAPIAccessor{
c: discoveryClient.OpenAPIV3(),
},
Expand Down Expand Up @@ -178,7 +173,7 @@ func (d CompareResult) Modified() []string {

// Compare checks if a resource has been changed from desired.
func (d *Comparator) Compare(
owner client.Object,
metadata types.RevisionMetadata,
desiredObject, actualObject Object,
) (res CompareResult, err error) {
if err := ensureGVKIsSet(desiredObject, d.scheme); err != nil {
Expand Down Expand Up @@ -227,7 +222,7 @@ func (d *Comparator) Compare(

// Extrapolate a field set from desired.
desiredObject = desiredObject.DeepCopyObject().(Object)
if err := d.ownerStrategy.SetControllerReference(owner, desiredObject); err != nil {
if err := metadata.SetCurrent(desiredObject); err != nil {
return res, err
}

Expand Down
Loading