Skip to content

Conversation

@mdbooth
Copy link

@mdbooth mdbooth commented Jan 26, 2026

Summary

The primary purpose of this refactor is to remove the hard-coded
association between a Revision and a client.Object. It allows the
creation of a RevisionMetadata implementation which does not require a
separate kubernetes object to exist for each Revision.

Change Type

Breaking Change

Check List Before Merging

  • This PR passes all pre-commit hook validations.
  • This PR is fully tested and regression tests are included.
  • Relevant documentation has been updated.

Additional Information

The primary purpose of this refactor is to remove the hard-coded
association between a Revision and a client.Object. It allows the
creation of a RevisionMetadata implementation which does not require a
separate kubernetes object to exist for each Revision.
@coderabbitai
Copy link

coderabbitai bot commented Jan 26, 2026

Walkthrough

This pull request refactors the ownership handling system from an owner-based strategy pattern to a metadata-based abstraction. New RevisionMetadata and RevisionReference interfaces replace the OwnerStrategy pattern across comparators, engines, validators, and supporting code. Method signatures throughout the machinery, validation, and command packages are updated to accept metadata instead of direct owner objects. The public API surface in boxcutter removes OwnerStrategy and introduces type aliases for the new metadata types.

Changes

Cohort / File(s) Summary
Metadata Interface Definitions
machinery/types/metadata.go, machinery/types/metadata_test.go
Introduces public RevisionMetadata interface with methods SetCurrent, IsCurrent, RemoveFrom, IsNamespaceAllowed, CopyReferences, GetCurrent, and public RevisionReference interface with String() method. Adds test mocks for interface validation.
Native Ownership Implementation
ownerhandling/native.go, ownerhandling/native_test.go
Replaces OwnerStrategyNative with public NativeRevisionMetadata type. Introduces NewNativeRevisionMetadata(owner, scheme, allowCrossNamespace) constructor. Implements metadata interface methods and adds NativeEnqueueRequestForOwner handler function.
Annotation-based Ownership
ownerhandling/annotation.go, ownerhandling/annotation_test.go
Refactors annotation-based ownership to use new metadata model via annotationRevisionMetadata. Introduces NewAnnotationHandler for managing annotation-based metadata. Replaces direct owner reference operations with metadata interface methods.
Ownership Infrastructure Cleanup
ownerhandling/common.go, ownerhandling/common_test.go
Removes obsolete ownerStrategy interface and generic remove helper function; deletes associated tests. Marks end of owner-strategy pattern.
Comparator Updates
machinery/comparator.go, machinery/comparator_test.go
Removes ownerStrategy parameter from NewComparator constructor. Updates Compare method signature from owner client.Object to metadata types.RevisionMetadata. Replaces SetControllerReference with metadata.SetCurrent.
Object Engine Refactoring
machinery/objects.go, machinery/objects_test.go
Removes ownerStrategy parameter from NewObjectEngine. Updates all method signatures (Reconcile, Teardown, objectUpdateHandling, detectOwner) to use types.RevisionMetadata. Replaces owner-based operations with metadata interface calls.
Phase Engine and Validation Integration
machinery/phases.go, machinery/phases_test.go, validation/phase.go, validation/phase_test.go
Updates PhaseEngine methods (Reconcile, Teardown) and phaseValidator interface to accept metadata instead of owner. Consolidates phase validator constructors from cluster/namespaced variants to single NewPhaseValidator.
Result Types Update
machinery/results.go
Changes ObjectResultCollision.conflictingOwner from *metav1.OwnerReference to types.RevisionReference. Updates ConflictingOwner() accessor return type accordingly.
Revision Types and Engine
machinery/types/types.go, machinery/types/types_test.go, machinery/revision.go, machinery/revision_test.go
Replaces Revision.Owner field with Revision.Metadata field of type RevisionMetadata. Updates GetOwner() method to GetMetadata(). Updates RevisionEngine to pass metadata to phase engine methods.
Type Options
machinery/types/options.go, machinery/types/options_test.go
Changes ObjectReconcileOptions.PreviousOwners from []client.Object to []RevisionMetadata. Updates WithPreviousOwners option wrapper type accordingly.
Object Validation
validation/object.go, validation/object_test.go
Consolidates validators from NewClusterObjectValidator / NewNamespacedObjectValidator to single NewObjectValidator. Updates Validate signature from owner client.Object to metadata types.RevisionMetadata. Replaces namespace validation with metadata.IsNamespaceAllowed() check. Introduces new NamespaceNotAllowedError type.
Public API Surface
boxcutter/boxcutter.go
Removes public OwnerStrategy interface and field from RevisionEngineOptions. Adds type aliases RevisionMetadata and RevisionReference mapped to types package. Updates validator construction from NewNamespacedPhaseValidator to NewPhaseValidator.
Deployment Command Usage
cmd/reference/internal/deploy.go, cmd/reference/internal/util.go
Replaces Revision.Owner with Revision.Metadata using ownerhandling.NewNativeRevisionMetadata. Updates WithPreviousOwners to accept metadata objects. Introduces getNativeOwner() helper to extract owner from native metadata.
Integration Tests
test/comparator_test.go, test/objects_test.go, test/revision_engine_*.go
Updates test constructors and invocations to remove owner-strategy parameters. Introduces metadata construction via ownerhandling.NewNativeRevisionMetadata(). Updates all Reconcile, Teardown, and Compare calls to pass metadata instead of owner objects. Updates conflict detection to handle new RevisionReference type.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

🚥 Pre-merge checks | ✅ 1 | ❌ 2
❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 22.54% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ❓ Inconclusive The description provides a clear summary of the refactoring purpose and identifies it as a Breaking Change, but critical items on the pre-merge checklist (validations, tests, documentation) remain unchecked. Complete the checklist items: confirm pre-commit validations pass, verify regression tests are included, and ensure documentation updates are complete or add a note if not required.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main refactoring work—replacing OwnerStrategy with RevisionMetadata—which aligns with the core change across all modified files.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

Comment @coderabbitai help to get the list of available commands and usage tips.

@mdbooth
Copy link
Author

mdbooth commented Jan 27, 2026

@coderabbitai review

@coderabbitai
Copy link

coderabbitai bot commented Jan 27, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 6

🤖 Fix all issues with AI agents
In `@boxcutter.go`:
- Around line 141-150: The code unconditionally constructs a new PhaseValidator
via validation.NewPhaseValidator, ignoring any custom PhaseValidator provided on
RevisionEngineOptions; update the pval initialization to use the supplied
opts.RevisionEngineOptions.PhaseValidator when non-nil and only call
validation.NewPhaseValidator(opts.RestMapper, opts.Writer) as a fallback so
RevisionEngine honors injected validators (adjust the pval assignment near the
NewPhaseValidator call and ensure the variable name pval is used by
machinery.NewComparator/machinery.NewObjectEngine as before).

In `@ownerhandling/annotation_test.go`:
- Around line 83-85: The test uses require.Error(t, err) then calls
assert.ErrorAs(t, err, &alreadyOwnedErr) which can continue on failure; replace
the assertion with require.ErrorAs(t, err, &alreadyOwnedErr) so the test fails
fast and avoids nil-pointer issues—update the call to require.ErrorAs
referencing the same variable alreadyOwnedErr and error value err in the
ownerhandling/annotation_test.go test.

In `@ownerhandling/annotation.go`:
- Around line 166-179: GetCurrent currently converts an annotationOwnerRef to
metav1.OwnerReference via ToMetaV1OwnerRef which drops Namespace; instead
preserve namespace by returning a RevisionReference that carries namespace
information — either return the original annotationOwnerRef (from
getOwnerReferences) or construct/return a wrapper implementing
bctypes.RevisionReference that embeds the metav1.OwnerReference plus a Namespace
field. Update GetCurrent in annotationRevisionMetadata and, if needed, adjust
ToMetaV1OwnerRef or add a new conversion method so the returned object from
GetCurrent still exposes Namespace (refer to GetCurrent, getOwnerReferences,
annotationOwnerRef and ToMetaV1OwnerRef).

In `@ownerhandling/native.go`:
- Around line 23-36: The constructor NewNativeRevisionMetadata currently ignores
the allowCrossNamespace parameter and IsNamespaceAllowed always enforces
same-namespace; store allowCrossNamespace on the NativeRevisionMetadata struct
(add a field like allowCrossNamespace bool), set it in
NewNativeRevisionMetadata, and update the IsNamespaceAllowed method to return
true when allowCrossNamespace is true (or when the owner is cluster-scoped)
otherwise enforce that target namespace equals owner.GetNamespace(); reference
NativeRevisionMetadata, NewNativeRevisionMetadata, and IsNamespaceAllowed when
making these changes.
- Around line 103-114: The CopyReferences implementation on
NativeRevisionMetadata is mutating the same OwnerReferences slice returned from
objA by calling objB.SetOwnerReferences(objA.GetOwnerReferences()) then changing
ownerRefs[i].Controller, which can mutate objA; instead, create a deep copy of
the owner references before mutating: obtain refs := objA.GetOwnerReferences(),
allocate a new slice of the same length, copy each metav1.OwnerReference into
the new slice (or append a struct literal per element) and set Controller =
ptr.To(false) on the copied elements, then call objB.SetOwnerReferences with
that new slice so objA is never modified.

In `@test/revision_engine_basic_test.go`:
- Around line 55-59: NewNativeRevisionMetadata is being called before the owner
UID exists which causes a panic when UID is empty; move creation of
revOwnerMetadata and the boxcutter.Revision rev until after you create the owner
(i.e., after calling Client.Create on revOwner) or re-fetch the owner to obtain
its UID, then construct revOwnerMetadata with
ownerhandling.NewNativeRevisionMetadata and build rev using that metadata so UID
is non-empty.
🧹 Nitpick comments (5)
cmd/reference/internal/util.go (1)

34-37: Type assertion without safe check could cause runtime panics.

The unchecked type assertion on line 36 will panic if revision.Metadata is not a *NativeRevisionMetadata. While the comment acknowledges this, consider using a safe type assertion with an ok check to provide a more informative error message, especially since this is called in a loop in prevJSON.

💡 Suggested improvement
 func getNativeOwner(revision boxcutter.Revision) client.Object {
-	// NOTE: Will panic if not a NativeRevisionMetadata
-	return revision.Metadata.(*ownerhandling.NativeRevisionMetadata).GetOwner()
+	// NOTE: Will panic if not a NativeRevisionMetadata
+	nativeMeta, ok := revision.Metadata.(*ownerhandling.NativeRevisionMetadata)
+	if !ok {
+		panic(fmt.Sprintf("expected *NativeRevisionMetadata, got %T", revision.Metadata))
+	}
+	return nativeMeta.GetOwner()
 }
ownerhandling/annotation.go (1)

29-31: Add documentation comment for exported function.

Per static analysis, NewAnnotationHandler is exported but lacks a documentation comment. This is a public API that should be documented.

📝 Suggested fix
+// NewAnnotationHandler creates a handler for annotation-based revision metadata operations.
+// The annotationKey specifies which annotation to use for storing owner references.
 func NewAnnotationHandler(annotationKey string) annotationHandler {
 	return annotationHandler{annotationKey: annotationKey}
 }
ownerhandling/annotation_test.go (1)

659-705: Duplicate test: TestAnnotationOwnerRef_IsController duplicates TestIsController.

This test (lines 659-705) is essentially identical to TestIsController (lines 266-312). Both test the isController() method with the same scenarios. Consider removing one to avoid maintenance burden.

validation/object.go (1)

106-141: Remove unused validateNamespace function.

The validateNamespace function is no longer used in production code and has been replaced by metadata.IsNamespaceAllowed(obj) in the Validate method (line 57). It only appears in test code (validation/object_test.go:370). Removing this function and its corresponding test will reduce code maintenance burden.

cmd/reference/internal/deploy.go (1)

403-425: Consider pre-allocating the previousMetadata slice.

Since the length of previousUnstr is known, pre-allocating previousMetadata avoids repeated slice growth allocations, as flagged by static analysis.

♻️ Suggested fix
-	var previousMetadata []boxcutter.RevisionMetadata
-	for i := range previousUnstr {
+	previousMetadata := make([]boxcutter.RevisionMetadata, 0, len(previousUnstr))
+
+	for i := range previousUnstr {
 		previous = append(previous, &previousUnstr[i])
 		previousMetadata = append(previousMetadata, ownerhandling.NewNativeRevisionMetadata(&previousUnstr[i], c.scheme, false))
 	}

Comment on lines +141 to 150
pval := validation.NewPhaseValidator(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,
)
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Respect RevisionEngineOptions.PhaseValidator when provided.

Line 141 always constructs a new PhaseValidator, which ignores a custom validator supplied in RevisionEngineOptions. This breaks configurability and differs from NewPhaseEngine.

🔧 Suggested fix
-	pval := validation.NewPhaseValidator(opts.RestMapper, opts.Writer)
+	pval := opts.PhaseValidator
+	if pval == nil {
+		pval = validation.NewPhaseValidator(opts.RestMapper, opts.Writer)
+	}
📝 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
pval := validation.NewPhaseValidator(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,
)
pval := opts.PhaseValidator
if pval == nil {
pval = validation.NewPhaseValidator(opts.RestMapper, opts.Writer)
}
rval := validation.NewRevisionValidator()
comp := machinery.NewComparator(
opts.DiscoveryClient, opts.Scheme, opts.FieldOwner)
oe := machinery.NewObjectEngine(
opts.Scheme, opts.Reader, opts.Writer,
comp, opts.FieldOwner, opts.SystemPrefix,
)
🤖 Prompt for AI Agents
In `@boxcutter.go` around lines 141 - 150, The code unconditionally constructs a
new PhaseValidator via validation.NewPhaseValidator, ignoring any custom
PhaseValidator provided on RevisionEngineOptions; update the pval initialization
to use the supplied opts.RevisionEngineOptions.PhaseValidator when non-nil and
only call validation.NewPhaseValidator(opts.RestMapper, opts.Writer) as a
fallback so RevisionEngine honors injected validators (adjust the pval
assignment near the NewPhaseValidator call and ensure the variable name pval is
used by machinery.NewComparator/machinery.NewObjectEngine as before).

Comment on lines +83 to +85
require.Error(t, err)
var alreadyOwnedErr *controllerutil.AlreadyOwnedError
assert.ErrorAs(t, err, &alreadyOwnedErr)
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Use require.ErrorAs instead of assert.ErrorAs after require.Error.

Per static analysis, error assertions should use require to fail fast and avoid nil pointer issues in subsequent assertions.

🔧 Suggested fix
 	err = m2.SetCurrent(obj)
 	require.Error(t, err)
 	var alreadyOwnedErr *controllerutil.AlreadyOwnedError
-	assert.ErrorAs(t, err, &alreadyOwnedErr)
+	require.ErrorAs(t, err, &alreadyOwnedErr)
📝 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
require.Error(t, err)
var alreadyOwnedErr *controllerutil.AlreadyOwnedError
assert.ErrorAs(t, err, &alreadyOwnedErr)
require.Error(t, err)
var alreadyOwnedErr *controllerutil.AlreadyOwnedError
require.ErrorAs(t, err, &alreadyOwnedErr)
🧰 Tools
🪛 golangci-lint (2.5.0)

[error] 85-85: require-error: for error assertions use require

(testifylint)

🤖 Prompt for AI Agents
In `@ownerhandling/annotation_test.go` around lines 83 - 85, The test uses
require.Error(t, err) then calls assert.ErrorAs(t, err, &alreadyOwnedErr) which
can continue on failure; replace the assertion with require.ErrorAs(t, err,
&alreadyOwnedErr) so the test fails fast and avoids nil-pointer issues—update
the call to require.ErrorAs referencing the same variable alreadyOwnedErr and
error value err in the ownerhandling/annotation_test.go test.

Comment on lines +166 to +179
// GetCurrent returns a RevisionReference describing the current revision of obj.
// Returns nil if there is no current revision set.
func (m *annotationRevisionMetadata) GetCurrent(obj metav1.Object) bctypes.RevisionReference {
for _, ref := range m.getOwnerReferences(obj) {
if ref.Controller != nil && *ref.Controller {
// Convert to metav1.OwnerReference for consistent return type.
ownerRef := ref.ToMetaV1OwnerRef()

return &ownerRef
}
}

s.setOwnerReferences(obj, ownerRefs)
return nil
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Search for GetCurrent usage across the codebase
echo "=== GetCurrent usage ==="
rg -n -B2 -A5 'GetCurrent\(' --type=go

echo -e "\n=== RevisionReference interface definition ==="
rg -n -B2 -A10 'type RevisionReference' --type=go

Repository: package-operator/boxcutter

Length of output: 7421


🏁 Script executed:

#!/bin/bash
# Check if metav1.OwnerReference has a Namespace field
echo "=== Checking metav1.OwnerReference fields ==="
# First find where it's imported from
rg -n 'metav1\.OwnerReference' --type=go | head -20

echo -e "\n=== Check annotationOwnerRef structure ==="
rg -n -B2 -A15 'type annotationOwnerRef' --type=go

echo -e "\n=== Check ToMetaV1OwnerRef method ==="
rg -n -B2 -A15 'func.*ToMetaV1OwnerRef' --type=go

Repository: package-operator/boxcutter

Length of output: 5624


🏁 Script executed:

#!/bin/bash
# Check how GetCurrent return value is used - what happens after the call
echo "=== GetCurrent usage in machinery/objects.go ==="
sed -n '475,495p' machinery/objects.go

echo -e "\n=== Search for namespace access after GetCurrent calls ==="
rg -n -B3 -A10 'GetCurrent\(' ownerhandling/annotation.go machinery/objects.go | head -60

Repository: package-operator/boxcutter

Length of output: 2573


🏁 Script executed:

#!/bin/bash
# Check if metav1.OwnerReference can have/store namespace
echo "=== Search for Namespace in metav1.OwnerReference usage ==="
rg -n 'OwnerReference.*Namespace' --type=go

echo -e "\n=== Check if annotationOwnerRef namespace is used elsewhere ==="
rg -n '\.Namespace' ownerhandling/annotation.go | head -20

Repository: package-operator/boxcutter

Length of output: 233


🏁 Script executed:

#!/bin/bash
# Check the test for ToMetaV1OwnerRef to see if namespace is tested
echo "=== Test case expectations for ToMetaV1OwnerRef conversion ==="
sed -n '429,475p' ownerhandling/annotation_test.go

Repository: package-operator/boxcutter

Length of output: 1361


Namespace information is lost in GetCurrent() return value.

ToMetaV1OwnerRef() deliberately omits the Namespace field from annotationOwnerRef when converting to metav1.OwnerReference, despite the comment at line 475-476 in machinery/objects.go stating the annotation approach is meant to "allow cross-namespace and cross-cluster refs." Since metav1.OwnerReference (a standard Kubernetes type for same-namespace references) doesn't have a namespace field, consumers of GetCurrent() cannot access namespace information, which contradicts the cross-namespace support goal of annotation-based ownership.

🤖 Prompt for AI Agents
In `@ownerhandling/annotation.go` around lines 166 - 179, GetCurrent currently
converts an annotationOwnerRef to metav1.OwnerReference via ToMetaV1OwnerRef
which drops Namespace; instead preserve namespace by returning a
RevisionReference that carries namespace information — either return the
original annotationOwnerRef (from getOwnerReferences) or construct/return a
wrapper implementing bctypes.RevisionReference that embeds the
metav1.OwnerReference plus a Namespace field. Update GetCurrent in
annotationRevisionMetadata and, if needed, adjust ToMetaV1OwnerRef or add a new
conversion method so the returned object from GetCurrent still exposes Namespace
(refer to GetCurrent, getOwnerReferences, annotationOwnerRef and
ToMetaV1OwnerRef).

Comment on lines +23 to +36
// NativeRevisionMetadata uses .metadata.ownerReferences for ownership tracking.
type NativeRevisionMetadata struct {
owner client.Object
scheme *runtime.Scheme
}

// NewNative returns a new OwnerStrategyNative instance.
func NewNative(scheme *runtime.Scheme) *OwnerStrategyNative {
return &OwnerStrategyNative{
scheme: scheme,
// NewNativeRevisionMetadata creates a RevisionMetadata using native ownerReferences.
// If allowCrossNamespace is false, only objects in owner.GetNamespace() are allowed.
// Panics if owner has an empty UID (not persisted to cluster).
func NewNativeRevisionMetadata(
owner client.Object,
scheme *runtime.Scheme,
allowCrossNamespace bool,
) *NativeRevisionMetadata {
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Persist and honor allowCrossNamespace (currently ignored).

allowCrossNamespace (Line 33) is never stored or checked, and IsNamespaceAllowed (Line 92) always enforces same-namespace for namespaced owners. This breaks cross-namespace mode and matches the unused-parameter lint warning.

🛠️ Proposed fix
 type NativeRevisionMetadata struct {
 	owner  client.Object
 	scheme *runtime.Scheme
+	allowCrossNamespace bool
 }
@@
 	return &NativeRevisionMetadata{
 		owner:  owner,
 		scheme: scheme,
+		allowCrossNamespace: allowCrossNamespace,
 	}
 }
@@
 	// If owner is cluster-scoped, all namespaces are allowed.
 	if len(ownerNs) == 0 {
 		return true
 	}
+	if m.allowCrossNamespace {
+		return true
+	}
 
 	// For namespaced owners, object must be in the same namespace.
 	return obj.GetNamespace() == ownerNs
 }

Also applies to: 91-101

🧰 Tools
🪛 golangci-lint (2.5.0)

[warning] 35-35: unused-parameter: parameter 'allowCrossNamespace' seems to be unused, consider removing or renaming it as _

(revive)

🤖 Prompt for AI Agents
In `@ownerhandling/native.go` around lines 23 - 36, The constructor
NewNativeRevisionMetadata currently ignores the allowCrossNamespace parameter
and IsNamespaceAllowed always enforces same-namespace; store allowCrossNamespace
on the NativeRevisionMetadata struct (add a field like allowCrossNamespace
bool), set it in NewNativeRevisionMetadata, and update the IsNamespaceAllowed
method to return true when allowCrossNamespace is true (or when the owner is
cluster-scoped) otherwise enforce that target namespace equals
owner.GetNamespace(); reference NativeRevisionMetadata,
NewNativeRevisionMetadata, and IsNamespaceAllowed when making these changes.

Comment on lines +103 to 114
// CopyReferences copies all revision metadata from objA to objB except the current revision marker.
// This is used when taking over control from a previous owner while preserving their watch references.
func (m *NativeRevisionMetadata) CopyReferences(objA, objB metav1.Object) {
// Copy owner references from A to B.
objB.SetOwnerReferences(objA.GetOwnerReferences())
// Release controller on B (set all Controller fields to false).
ownerRefs := objB.GetOwnerReferences()
for i := range ownerRefs {
ownerRefs[i].Controller = ptr.To(false)
}
objB.SetOwnerReferences(ownerRefs)
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Avoid mutating objA when clearing controller flags.

objB.SetOwnerReferences(objA.GetOwnerReferences()) reuses the same backing slice. Mutating ownerRefs[i].Controller can inadvertently clear controller flags on objA. Clone before mutation.

🛠️ Proposed fix
-	// Copy owner references from A to B.
-	objB.SetOwnerReferences(objA.GetOwnerReferences())
-	// Release controller on B (set all Controller fields to false).
-	ownerRefs := objB.GetOwnerReferences()
+	// Copy owner references from A to B (clone to avoid aliasing).
+	ownerRefs := slices.Clone(objA.GetOwnerReferences())
+	// Release controller on B (set all Controller fields to false).
 	for i := range ownerRefs {
 		ownerRefs[i].Controller = ptr.To(false)
 	}
 	objB.SetOwnerReferences(ownerRefs)
🤖 Prompt for AI Agents
In `@ownerhandling/native.go` around lines 103 - 114, The CopyReferences
implementation on NativeRevisionMetadata is mutating the same OwnerReferences
slice returned from objA by calling
objB.SetOwnerReferences(objA.GetOwnerReferences()) then changing
ownerRefs[i].Controller, which can mutate objA; instead, create a deep copy of
the owner references before mutating: obtain refs := objA.GetOwnerReferences(),
allocate a new slice of the same length, copy each metav1.OwnerReference into
the new slice (or append a struct literal per element) and set Controller =
ptr.To(false) on the copied elements, then call objB.SetOwnerReferences with
that new slice so objA is never modified.

Comment on lines +55 to 59
revOwnerMetadata := ownerhandling.NewNativeRevisionMetadata(revOwner, Scheme, false)
rev := boxcutter.Revision{
Name: "rev-1",
Owner: revOwner,
Metadata: revOwnerMetadata,
Revision: 1,
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Create metadata only after the owner UID exists.

Line 55 builds NativeRevisionMetadata before the owner is created. NewNativeRevisionMetadata panics if the UID is empty, so this will fail in integration runs. Move metadata (and revision construction) after Client.Create, or re-fetch to ensure UID is set.

✅ Suggested fix (move metadata creation after owner creation)
-	revOwnerMetadata := ownerhandling.NewNativeRevisionMetadata(revOwner, Scheme, false)
-	rev := boxcutter.Revision{
-		Name:     "rev-1",
-		Metadata: revOwnerMetadata,
-		Revision: 1,
-		Phases: []boxcutter.Phase{
-			{
-				Name:    "phase-1",
-				Objects: []unstructured.Unstructured{*obj1},
-			},
-			{
-				Name:    "phase-2",
-				Objects: []unstructured.Unstructured{*obj2},
-			},
-		},
-	}
-
-	// Owner has to be there first:
-	err := Client.Create(ctx, revOwner)
+	// Owner has to be there first:
+	err := Client.Create(ctx, revOwner)
 	require.NoError(t, err)
+	require.NotEmpty(t, revOwner.GetUID())
+
+	revOwnerMetadata := ownerhandling.NewNativeRevisionMetadata(revOwner, Scheme, false)
+	rev := boxcutter.Revision{
+		Name:     "rev-1",
+		Metadata: revOwnerMetadata,
+		Revision: 1,
+		Phases: []boxcutter.Phase{
+			{
+				Name:    "phase-1",
+				Objects: []unstructured.Unstructured{*obj1},
+			},
+			{
+				Name:    "phase-2",
+				Objects: []unstructured.Unstructured{*obj2},
+			},
+		},
+	}
🤖 Prompt for AI Agents
In `@test/revision_engine_basic_test.go` around lines 55 - 59,
NewNativeRevisionMetadata is being called before the owner UID exists which
causes a panic when UID is empty; move creation of revOwnerMetadata and the
boxcutter.Revision rev until after you create the owner (i.e., after calling
Client.Create on revOwner) or re-fetch the owner to obtain its UID, then
construct revOwnerMetadata with ownerhandling.NewNativeRevisionMetadata and
build rev using that metadata so UID is non-empty.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant