Refactor OwnerStrategy to RevisionMetadata#427
Refactor OwnerStrategy to RevisionMetadata#427mdbooth wants to merge 1 commit intopackage-operator:mainfrom
Conversation
WalkthroughReplaces owner-reference APIs with a revision-metadata abstraction (types.RevisionMetadata / RevisionReference). Public OwnerStrategy and owner-based fields removed; code now accepts RevisionMetadata across comparator, object/phase engines, validators, CLI, and tests. Adds native and annotation-backed RevisionMetadata implementations and updates plumbing and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as RevisionEngine
participant Meta as RevisionMetadata
participant Phase as PhaseEngine
participant Obj as ObjectEngine
participant Comp as Comparator
participant K8s as Kubernetes API
CLI->>Meta: rev.GetMetadata()
CLI->>Phase: PhaseEngine.Reconcile(ctx, metadata, revision, phase, opts...)
Phase->>Obj: ObjectEngine.Reconcile(ctx, metadata, revision, desiredObject, opts...)
Obj->>Comp: Comparator.Compare(metadata, desiredObject, actualObject)
Comp->>Meta: metadata.SetCurrent(desiredObject)
Obj->>K8s: apply/update/delete objects via client.Writer/Reader
K8s-->>Obj: API responses
Obj-->>Phase: ObjectResult
Phase-->>CLI: PhaseResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.Metadatais not a*NativeRevisionMetadata. While the comment acknowledges this, consider using a safe type assertion with anokcheck to provide a more informative error message, especially since this is called in a loop inprevJSON.💡 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,
NewAnnotationHandleris 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_IsControllerduplicatesTestIsController.This test (lines 659-705) is essentially identical to
TestIsController(lines 266-312). Both test theisController()method with the same scenarios. Consider removing one to avoid maintenance burden.validation/object.go (1)
106-141: Remove unusedvalidateNamespacefunction.The
validateNamespacefunction is no longer used in production code and has been replaced bymetadata.IsNamespaceAllowed(obj)in theValidatemethod (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 thepreviousMetadataslice.Since the length of
previousUnstris known, pre-allocatingpreviousMetadataavoids 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)) }
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/revision_engine_basic_test.go (1)
26-61:⚠️ Potential issue | 🟠 MajorRemove manually-set UID and CreationTimestamp; build metadata after Create with actual server-assigned values.
Kubernetes server-manages these fields and ignores them on create. The real issue is that metadata is built from
revOwnerbefore the Create call at line 86, so it captures the fake UID ("12345-678") instead of the actual server-assigned UID. Create the owner first, verify UID is populated, then build the metadata.Suggested fix
revOwner := &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: "rev-test", Namespace: "default", - UID: "12345-678", - CreationTimestamp: metav1.Now(), }, } obj1Probe := &stubProbe{status: probing.StatusFalse, messages: []string{"nope"}} obj2Probe := &stubProbe{status: probing.StatusTrue} obj1 := &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "v1", "kind": "ConfigMap", "metadata": map[string]interface{}{ "name": "test-rev-obj-1", "namespace": "default", }, }, } obj2 := &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "v1", "kind": "ConfigMap", "metadata": map[string]interface{}{ "name": "test-rev-obj-2", "namespace": "default", }, }, } - revOwnerMetadata := ownerhandling.NewNativeRevisionMetadata(revOwner, Scheme) - 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}, - }, - }, - } comp := machinery.NewComparator(DiscoveryClient, Scheme, fieldOwner) oe := machinery.NewObjectEngine( Scheme, Client, Client, comp, fieldOwner, systemPrefix, ) pval := validation.NewPhaseValidator(Client.RESTMapper(), Client) pe := machinery.NewPhaseEngine(oe, pval) rval := validation.NewRevisionValidator() re := machinery.NewRevisionEngine(pe, rval, Client) ctx := t.Context() // 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) + 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}, + }, + }, + }
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmd/reference/internal/deploy.go (1)
405-425:⚠️ Potential issue | 🟠 MajorGuard against panic when previous revisions lack UID.
NewNativeRevisionMetadatapanics on empty UID; previous revisions loaded fromcmPreviousKeymay omit UID (e.g., older data or manual edits). Consider validating and returning a controlled error (or skipping) instead of panicking.💡 Suggested guard to avoid panic
- 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 previousUnstr[i].GetUID() == "" { + return nil, nil, nil, fmt.Errorf("previous revision %q is missing UID", previousUnstr[i].GetName()) + } + previousMetadata = append( + previousMetadata, + ownerhandling.NewNativeRevisionMetadata(&previousUnstr[i], c.scheme), + ) + previous = append(previous, &previousUnstr[i]) + }machinery/objects.go (1)
71-99:⚠️ Potential issue | 🟠 MajorOrphan shortcut can panic on typed objects when called directly or if object types change.
The ObjectEngine.Teardown signature accepts the generic
Objectinterface, but line 93 contains an unsafe type assertion to*unstructured.Unstructuredin the orphan path:err := removeBoxcutterManagedLabel(ctx, e.writer, desiredObject.(*unstructured.Unstructured))This will panic if a typed object is passed. While Phase.Objects currently contains only unstructured objects, direct callers to ObjectEngine.Teardown or future changes to support typed objects would expose this. The removeBoxcutterManagedLabel function (line 559) also only accepts
*unstructured.Unstructured.Update removeBoxcutterManagedLabel to accept the generic
Objectinterface:🛠️ Safer orphan path
-func removeBoxcutterManagedLabel( - ctx context.Context, w client.Writer, obj *unstructured.Unstructured, -) error { - updated := obj.DeepCopy() +func removeBoxcutterManagedLabel( + ctx context.Context, w client.Writer, obj Object, +) error { + updated := obj.DeepCopyObject().(Object)Then update the call site at line 93 to remove the type assertion:
- err := removeBoxcutterManagedLabel(ctx, e.writer, desiredObject.(*unstructured.Unstructured)) + err := removeBoxcutterManagedLabel(ctx, e.writer, desiredObject)Also applies to: 559-571
🤖 Fix all issues with AI agents
In `@machinery/revision.go`:
- Around line 182-185: The code calls rev.Metadata.GetReconcileOptions() which
can panic if rev.Metadata is nil; change to use the protobuf getter
(rev.GetMetadata().GetReconcileOptions()) or otherwise guard rev.Metadata before
calling to ensure a non-nil metadata is used, then apply options into
types.RevisionReconcileOptions as before; make the analogous change for the
teardown path that uses GetTeardownOptions (the block around lines 318-320) so
both reconcile and teardown use rev.GetMetadata() (or an explicit nil-check) and
avoid panics.
🧹 Nitpick comments (3)
validation/object_test.go (1)
119-138: Test case name doesn't match the actual test behavior.The test case
"valid object with cross-namespace allowed"usesNativeRevisionMetadatawhich restricts objects to the owner's namespace (unless cluster-scoped). The object is in"default"namespace matching the owner's namespace, so this isn't actually testing cross-namespace behavior. Consider renaming to"valid object in owner namespace"for clarity, or useAnnotationStrategyif cross-namespace is the intended test scenario.ownerhandling/annotation.go (1)
224-227: Consider returning an error instead of panicking on malformed annotation.The
panic(err)on JSON unmarshal failure could cause unexpected controller crashes if an annotation is malformed (e.g., by manual editing or a bug). Returning an error or an empty slice with logging would be more resilient.♻️ Suggested approach
var ownerReferences []annotationOwnerRef if err := json.Unmarshal([]byte(annotations[annotationKey]), &ownerReferences); err != nil { - panic(err) + // Log warning and return empty slice to avoid crashing on malformed data + return nil }Alternatively, you could return
([]annotationOwnerRef, error)and propagate the error to callers.test/revision_engine_colision_protection_test.go (1)
129-141: Type assertion workaround is necessary but fragile.The code correctly handles the interface change where
ConflictingOwner()now returnstypes.RevisionReferenceinstead of*metav1.OwnerReference. The type assertion to*metav1.OwnerReferenceworks becauseNativeRevisionMetadata.GetCurrent()returns this type.The inline comment about the type assertion issue is helpful. However, this pattern relies on implementation details of
NativeRevisionMetadata. If a differentRevisionMetadataimplementation returns a different concrete type forRevisionReference, this test would fail.Consider adding a comment noting this coupling, or using the
String()method onRevisionReferenceif only display/identification is needed.
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.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
machinery/phases_test.go (1)
129-177:⚠️ Potential issue | 🟡 MinorTeardown mock expectation likely mismatches actual call signature.
objectEngineMock.Teardownpasses the variadicoptsslice as a single argument tom.Called(...). The test currently expects two option arguments, which will not match and can fail.🔧 Minimal fix (match the current mock behavior)
- oe.On("Teardown", mock.Anything, mock.Anything, revision, obj, mock.Anything, mock.Anything). + oe.On("Teardown", mock.Anything, mock.Anything, revision, obj, mock.Anything).
🤖 Fix all issues with AI agents
In `@cmd/reference/internal/deploy.go`:
- Around line 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.
In `@test/revision_engine_basic_test.go`:
- Around line 62-63: Remove the unused nolint directive before the test
assertion: delete the "//nolint:usetesting" comment that precedes the
require.NoError call invoking Client.Delete with revOwner; leave the
require.NoError(t, Client.Delete(context.Background(), revOwner)) line intact
and run golangci-lint to confirm no remaining nolint warnings.
In `@validation/object.go`:
- Around line 50-61: The code calls metadata.IsNamespaceAllowed(obj) and will
panic if the bctypes.RevisionMetadata interface value is nil; add a nil guard
before invoking IsNamespaceAllowed: if metadata == nil { errs = append(errs, /*
create a descriptive error e.g. MissingRevisionMetadataError or a
ValidationError capturing that metadata is nil */) ; return
NewObjectValidationError(bctypes.ToObjectRef(obj), errs...) } otherwise proceed
to call metadata.IsNamespaceAllowed(obj) as before; update any error type/name
you add to match project conventions and reference validateObjectMetadata,
metadata.IsNamespaceAllowed, and NewObjectValidationError when making the
change.
| previousMetadata := make([]boxcutter.RevisionMetadata, len(previousUnstr)) | ||
| for i := range previousUnstr { | ||
| previousMetadata[i] = ownerhandling.NewNativeRevisionMetadata(&previousUnstr[i], c.scheme) | ||
| previous = append(previous, &previousUnstr[i]) | ||
| } |
There was a problem hiding this comment.
🧩 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 25Repository: package-operator/boxcutter
Length of output: 724
🏁 Script executed:
#!/bin/bash
# Search for ownerhandling.NewNativeRevisionMetadata to understand what it does
rg -n "NewNativeRevisionMetadata" -C5Repository: package-operator/boxcutter
Length of output: 26530
🏁 Script executed:
#!/bin/bash
# Search for cmPreviousKey to understand where previousUnstr comes from
rg -n "cmPreviousKey" -C5Repository: 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 10Repository: 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 5Repository: 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.goRepository: 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.goRepository: 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 2Repository: 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 10Repository: 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 10Repository: 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 3Repository: 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 2Repository: 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.goRepository: 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 10Repository: 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.goRepository: 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 2Repository: 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.goRepository: 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 3Repository: 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 5Repository: 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/internalRepository: 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.goRepository: 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 15Repository: 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.goRepository: 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 2Repository: 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.goRepository: 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 -20Repository: 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.
| 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.
| //nolint:usetesting | ||
| require.NoError(t, Client.Delete(context.Background(), revOwner)) |
There was a problem hiding this comment.
Remove unused //nolint:usetesting directive.
golangci-lint reports this nolint is unused; drop it.
🧹 Proposed cleanup
- //nolint:usetesting
require.NoError(t, Client.Delete(context.Background(), revOwner))📝 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.
| //nolint:usetesting | |
| require.NoError(t, Client.Delete(context.Background(), revOwner)) | |
| require.NoError(t, Client.Delete(context.Background(), revOwner)) |
🧰 Tools
🪛 golangci-lint (2.5.0)
[error] 62-62: directive //nolint:usetesting is unused for linter "usetesting"
(nolintlint)
🤖 Prompt for AI Agents
In `@test/revision_engine_basic_test.go` around lines 62 - 63, Remove the unused
nolint directive before the test assertion: delete the "//nolint:usetesting"
comment that precedes the require.NoError call invoking Client.Delete with
revOwner; leave the require.NoError(t, Client.Delete(context.Background(),
revOwner)) line intact and run golangci-lint to confirm no remaining nolint
warnings.
| ctx context.Context, metadata bctypes.RevisionMetadata, | ||
| obj *unstructured.Unstructured, | ||
| ) error { | ||
| // Static metadata validation. | ||
| errs := validateObjectMetadata(obj) | ||
|
|
||
| if !d.allowNamespaceEscalation { | ||
| // Ensure we are not leaving the namespace we are operating in. | ||
| if err := validateNamespace( | ||
| d.restMapper, owner.GetNamespace(), obj, | ||
| ); err != nil { | ||
| errs = append(errs, err) | ||
| // we don't want to do a dry-run when this already fails. | ||
| return NewObjectValidationError(bctypes.ToObjectRef(obj), errs...) | ||
| } | ||
| // Check if namespace is allowed by the metadata. | ||
| if !metadata.IsNamespaceAllowed(obj) { | ||
| errs = append(errs, NamespaceNotAllowedError{Namespace: obj.GetNamespace()}) | ||
| // we don't want to do a dry-run when this already fails. | ||
| return NewObjectValidationError(bctypes.ToObjectRef(obj), errs...) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -n "*.go" | head -20Repository: package-operator/boxcutter
Length of output: 300
🏁 Script executed:
# Find RevisionMetadata type definition
rg -n "type RevisionMetadata" -g'*.go' -A 5
# Find IsNamespaceAllowed method
rg -n "func.*IsNamespaceAllowed" -g'*.go' -A 10
# Check if metadata is pointer or value type in the Validate signature
rg -n "func.*Validate.*RevisionMetadata" -g'*.go' -B 2 -A 2Repository: package-operator/boxcutter
Length of output: 5370
🏁 Script executed:
# Search for Validate call sites to see how metadata is passed
rg -n "\.Validate\(" -g'*.go' -B 2 -A 2Repository: package-operator/boxcutter
Length of output: 2222
🏁 Script executed:
# Check the actual Validate function at lines 50-61
cat -n validation/object.go | sed -n '45,70p'Repository: package-operator/boxcutter
Length of output: 1291
🏁 Script executed:
# Check if there's explicit contract/documentation for non-nil metadata
rg -n "metadata.*must\|metadata.*nil\|metadata.*required" -g'*.go' -B 2 -A 2
# Check the main entry points that call Validate with metadata
cat -n validation/phase.go | sed -n '30,60p'Repository: package-operator/boxcutter
Length of output: 941
🏁 Script executed:
# Check how metadata parameter is obtained/validated in phase validator
cat -n validation/phase.go | sed -n '1,50p'Repository: package-operator/boxcutter
Length of output: 1553
Add nil guard for RevisionMetadata to prevent runtime panic.
metadata.IsNamespaceAllowed(obj) will panic if the interface value is nil. Although current call sites appear to pass non-nil metadata, a defensive check prevents this potential runtime error from caller mistakes.
Suggested nil guard
func (d *ObjectValidator) Validate(
ctx context.Context, metadata bctypes.RevisionMetadata,
obj *unstructured.Unstructured,
) error {
+ if metadata == nil {
+ return fmt.Errorf("revision metadata must not be nil")
+ }
// Static metadata validation.
errs := validateObjectMetadata(obj)📝 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.
| ctx context.Context, metadata bctypes.RevisionMetadata, | |
| obj *unstructured.Unstructured, | |
| ) error { | |
| // Static metadata validation. | |
| errs := validateObjectMetadata(obj) | |
| if !d.allowNamespaceEscalation { | |
| // Ensure we are not leaving the namespace we are operating in. | |
| if err := validateNamespace( | |
| d.restMapper, owner.GetNamespace(), obj, | |
| ); err != nil { | |
| errs = append(errs, err) | |
| // we don't want to do a dry-run when this already fails. | |
| return NewObjectValidationError(bctypes.ToObjectRef(obj), errs...) | |
| } | |
| // Check if namespace is allowed by the metadata. | |
| if !metadata.IsNamespaceAllowed(obj) { | |
| errs = append(errs, NamespaceNotAllowedError{Namespace: obj.GetNamespace()}) | |
| // we don't want to do a dry-run when this already fails. | |
| return NewObjectValidationError(bctypes.ToObjectRef(obj), errs...) | |
| } | |
| ctx context.Context, metadata bctypes.RevisionMetadata, | |
| obj *unstructured.Unstructured, | |
| ) error { | |
| if metadata == nil { | |
| return fmt.Errorf("revision metadata must not be nil") | |
| } | |
| // Static metadata validation. | |
| errs := validateObjectMetadata(obj) | |
| // Check if namespace is allowed by the metadata. | |
| if !metadata.IsNamespaceAllowed(obj) { | |
| errs = append(errs, NamespaceNotAllowedError{Namespace: obj.GetNamespace()}) | |
| // we don't want to do a dry-run when this already fails. | |
| return NewObjectValidationError(bctypes.ToObjectRef(obj), errs...) | |
| } |
🤖 Prompt for AI Agents
In `@validation/object.go` around lines 50 - 61, The code calls
metadata.IsNamespaceAllowed(obj) and will panic if the bctypes.RevisionMetadata
interface value is nil; add a nil guard before invoking IsNamespaceAllowed: if
metadata == nil { errs = append(errs, /* create a descriptive error e.g.
MissingRevisionMetadataError or a ValidationError capturing that metadata is nil
*/) ; return NewObjectValidationError(bctypes.ToObjectRef(obj), errs...) }
otherwise proceed to call metadata.IsNamespaceAllowed(obj) as before; update any
error type/name you add to match project conventions and reference
validateObjectMetadata, metadata.IsNamespaceAllowed, and
NewObjectValidationError when making the change.
1 participant
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.
RevisionMetadata therefore has a similar set of methods to the OwnerStrategy it replaces. The existing Native and Annotation strategies are updated to use the new interface. In both cases, the Owner object is passed at construction, and is therefore no longer part of the RevisionMetadata interface.
The differences are summarised as:
Renamed functions
RevisionReferenceinstead ofmetav1.OwnerReference. The reason is that we can't return an OwnerReference in an implementation which has no owner object. The return value is only used in log messages, so in fact that only requirement was that it has aString()method. The new interface simply codifies the existing usage.owneris no longer an argument.Functions removed without replacement
NOTE: I can add these back if there are any out of tree callers of these methods, but I preferred to err on the side of deleting potentially dead code in the first instance.
New methods
Teardown(). The previous implementation checked the owner object direction, which the new implementation cannot do as there may not be an owner object. Instead we allow the RevisionMetadata to add options to Reconcile() or Teardown(). The only current use is that we automatically addWithOrphanto Teardown when using the Native handler (this doesn't make sense for Annotation) if theorphanfinalizer is observed on the owner object.Event handlers
Both previous OwnerStrategies also implemented an
EnqueueRequestForOwnermethod, although this was not part of theOwnerStrategyinterface. In both cases a direct replacement is provided with identical behaviour to the previous event handler, although they are not implemented as methods of the RevisionMetadata implementation object.Change Type
Breaking Change
Check List Before Merging
Additional Information