diff --git a/boxcutter.go b/boxcutter.go index a2133178..e88aee41 100644 --- a/boxcutter.go +++ b/boxcutter.go @@ -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 @@ -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 { @@ -114,7 +116,6 @@ type RevisionEngineOptions struct { // Optional - OwnerStrategy OwnerStrategy PhaseValidator *validation.PhaseValidator } @@ -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 @@ -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) diff --git a/cmd/reference/internal/deploy.go b/cmd/reference/internal/deploy.go index 0cbd5cd4..f0996b1c 100644 --- a/cmd/reference/internal/deploy.go +++ b/cmd/reference/internal/deploy.go @@ -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" ) @@ -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 @@ -122,7 +127,7 @@ 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) @@ -130,7 +135,7 @@ func (c *Reconciler) handleDeployment(ctx context.Context, cm *corev1.ConfigMap) return res, fmt.Errorf("to revision: %w", err) } - existingRevisions = append(existingRevisions, *r) + existingRevisions = append(existingRevisions, r) } sort.Sort(revisionAscending(existingRevisions)) @@ -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 } } @@ -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) } @@ -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) } @@ -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) } @@ -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 @@ -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]) } for _, phase := range phases { @@ -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 { diff --git a/cmd/reference/internal/util.go b/cmd/reference/internal/util.go index 0e77af16..3ea69e10 100644 --- a/cmd/reference/internal/util.go +++ b/cmd/reference/internal/util.go @@ -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] } @@ -21,7 +19,7 @@ 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 } @@ -29,11 +27,15 @@ func latestRevisionNumber(prevRevisions []bctypes.Revision) int64 { 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()) diff --git a/machinery/comparator.go b/machinery/comparator.go index 13069932..f0c4c0d7 100644 --- a/machinery/comparator.go +++ b/machinery/comparator.go @@ -21,6 +21,8 @@ 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 @@ -28,7 +30,6 @@ import ( // 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 @@ -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(), }, @@ -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 { @@ -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 } diff --git a/machinery/comparator_test.go b/machinery/comparator_test.go index a958fdc1..88e1fc2f 100644 --- a/machinery/comparator_test.go +++ b/machinery/comparator_test.go @@ -32,10 +32,9 @@ func TestComparator_Unstructured(t *testing.T) { a := &dummyOpenAPIAccessor{ openAPI: oapi, } - n := ownerhandling.NewNative(scheme.Scheme) d := &Comparator{ - ownerStrategy: n, openAPIAccessor: a, + scheme: scheme.Scheme, fieldOwner: testFieldOwner, } @@ -46,6 +45,7 @@ func TestComparator_Unstructured(t *testing.T) { Namespace: "test", }, } + ownerMetadata := ownerhandling.NewNativeRevisionMetadata(owner, scheme.Scheme) // Test Case 1 // Another actor has updated .data.test and the field owner has changed. @@ -96,7 +96,7 @@ func TestComparator_Unstructured(t *testing.T) { }, }, } - err = n.SetControllerReference(owner, actualNewFieldOwner) + err = ownerMetadata.SetCurrent(actualNewFieldOwner) require.NoError(t, err) // Test Case 2 @@ -172,7 +172,7 @@ func TestComparator_Unstructured(t *testing.T) { }, }, } - err = n.SetControllerReference(owner, pod) + err = ownerMetadata.SetCurrent(pod) require.NoError(t, err) tests := []struct { @@ -202,7 +202,7 @@ func TestComparator_Unstructured(t *testing.T) { t.Run(test.name, func(t *testing.T) { t.Parallel() - res, err := d.Compare(owner, test.desired, test.actual) + res, err := d.Compare(ownerMetadata, test.desired, test.actual) require.NoError(t, err) assert.Equal(t, test.expectedReport, res.String()) @@ -256,10 +256,10 @@ func TestComparator_Unstructured(t *testing.T) { }, }, } - err = n.SetControllerReference(owner, actualValueChange) + err = ownerMetadata.SetCurrent(actualValueChange) require.NoError(t, err) - res, err := d.Compare(owner, desiredValueChange, actualValueChange) + res, err := d.Compare(ownerMetadata, desiredValueChange, actualValueChange) require.NoError(t, err) // no conflicts assert.Empty(t, res.ConflictingMangers) @@ -281,10 +281,9 @@ func TestComparator_Structured(t *testing.T) { a := &dummyOpenAPIAccessor{ openAPI: oapi, } - n := ownerhandling.NewNative(scheme.Scheme) d := &Comparator{ - ownerStrategy: n, openAPIAccessor: a, + scheme: scheme.Scheme, fieldOwner: testFieldOwner, } @@ -297,6 +296,7 @@ func TestComparator_Structured(t *testing.T) { CreationTimestamp: now, }, } + ownerMetadata := ownerhandling.NewNativeRevisionMetadata(owner, scheme.Scheme) // Test Case 1 // Another actor has updated .data.test and the field owner has changed. @@ -351,7 +351,7 @@ func TestComparator_Structured(t *testing.T) { "test": []byte("test123"), }, } - err = n.SetControllerReference(owner, actualNewFieldOwner) + err = ownerMetadata.SetCurrent(actualNewFieldOwner) require.NoError(t, err) // Test Case 2 @@ -461,7 +461,7 @@ func TestComparator_Structured(t *testing.T) { }, }, } - err = n.SetControllerReference(owner, actualPod) + err = ownerMetadata.SetCurrent(actualPod) require.NoError(t, err) tests := []struct { @@ -491,7 +491,7 @@ func TestComparator_Structured(t *testing.T) { t.Run(test.name, func(t *testing.T) { t.Parallel() - res, err := d.Compare(owner, test.desired, test.actual) + res, err := d.Compare(ownerMetadata, test.desired, test.actual) require.NoError(t, err) if res.Comparison != nil { @@ -543,10 +543,10 @@ func TestComparator_Structured(t *testing.T) { }, }, } - err = n.SetControllerReference(owner, actualValueChange) + err = ownerMetadata.SetCurrent(actualValueChange) require.NoError(t, err) - res, err := d.Compare(owner, desiredValueChange, actualValueChange) + res, err := d.Compare(ownerMetadata, desiredValueChange, actualValueChange) require.NoError(t, err) // no conflicts assert.Empty(t, res.ConflictingMangers) diff --git a/machinery/objects.go b/machinery/objects.go index 85727208..d13c4ef1 100644 --- a/machinery/objects.go +++ b/machinery/objects.go @@ -8,7 +8,6 @@ import ( "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" machinerytypes "k8s.io/apimachinery/pkg/types" @@ -16,18 +15,16 @@ import ( "k8s.io/client-go/util/csaupgrade" "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "pkg.package-operator.run/boxcutter/machinery/types" ) // ObjectEngine reconciles individual objects. type ObjectEngine struct { - scheme *runtime.Scheme - cache client.Reader - writer client.Writer - ownerStrategy objectEngineOwnerStrategy - comparator comparator + scheme *runtime.Scheme + cache client.Reader + writer client.Writer + comparator comparator fieldOwner string systemPrefix string @@ -38,18 +35,16 @@ func NewObjectEngine( scheme *runtime.Scheme, cache client.Reader, writer client.Writer, - ownerStrategy objectEngineOwnerStrategy, comparator comparator, fieldOwner string, systemPrefix string, ) *ObjectEngine { return &ObjectEngine{ - scheme: scheme, - cache: cache, - writer: writer, - ownerStrategy: ownerStrategy, - comparator: comparator, + scheme: scheme, + cache: cache, + writer: writer, + comparator: comparator, fieldOwner: fieldOwner, systemPrefix: systemPrefix, @@ -62,18 +57,9 @@ type Object interface { runtime.Object } -type objectEngineOwnerStrategy 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) -} - type comparator interface { Compare( - owner client.Object, + metadata types.RevisionMetadata, desiredObject, actualObject Object, ) (res CompareResult, err error) } @@ -85,7 +71,7 @@ const ( // Teardown ensures the given object is safely removed from the cluster. func (e *ObjectEngine) Teardown( ctx context.Context, - owner client.Object, // Owner of the object. + metadata types.RevisionMetadata, // Metadata for the owner. revision int64, // Revision number, must start at 1. desiredObject Object, opts ...types.ObjectTeardownOption, @@ -102,14 +88,8 @@ func (e *ObjectEngine) Teardown( panic("owner revision must be set and start at 1") } - if len(owner.GetUID()) == 0 { - panic("owner must be persisted to cluster, empty UID") - } - - // Shortcut when Owner is orphaning its dependents. - // If we don't check this, we might be too quick and start deleting - // dependents that should be kept on the cluster! - if controllerutil.ContainsFinalizer(owner, "orphan") || options.Orphan { + // Shortcut when orphaning dependents. + if options.Orphan { err := removeBoxcutterManagedLabel(ctx, e.writer, desiredObject.(*unstructured.Unstructured)) if err != nil { return false, err @@ -145,11 +125,11 @@ func (e *ObjectEngine) Teardown( return false, fmt.Errorf("getting object revision: %w", err) } - ctrlSit, _ := e.detectOwner(owner, actualObject, nil) + ctrlSit, _ := e.detectOwner(metadata, actualObject, nil) if actualRevision != revision || ctrlSit != ctrlSituationIsController { // Remove us from owners list: patch := actualObject.DeepCopyObject().(Object) - e.ownerStrategy.RemoveOwner(owner, patch) + metadata.RemoveFrom(patch) return true, e.writer.Patch(ctx, patch, client.MergeFrom(actualObject)) } @@ -178,7 +158,7 @@ func (e *ObjectEngine) Teardown( // Reconcile runs actions to bring actual state closer to desired. func (e *ObjectEngine) Reconcile( ctx context.Context, - owner client.Object, // Owner of the object. + metadata types.RevisionMetadata, // Metadata for the owner. revision int64, // Revision number, must start at 1. desiredObject Object, opts ...types.ObjectReconcileOption, @@ -203,10 +183,6 @@ func (e *ObjectEngine) Reconcile( panic("owner revision must be set and start at 1") } - if len(owner.GetUID()) == 0 { - panic("owner must be persistet to cluster, empty UID") - } - if err := ensureGVKIsSet(desiredObject, e.scheme); err != nil { return nil, err } @@ -215,9 +191,7 @@ func (e *ObjectEngine) Reconcile( desiredObject = desiredObject.DeepCopyObject().(Object) e.setObjectRevision(desiredObject, revision) - if err := e.ownerStrategy.SetControllerReference( - owner, desiredObject, - ); err != nil { + if err := metadata.SetCurrent(desiredObject); err != nil { return nil, fmt.Errorf("set controller reference: %w", err) } @@ -261,14 +235,14 @@ func (e *ObjectEngine) Reconcile( } return e.objectUpdateHandling( - ctx, owner, revision, + ctx, metadata, revision, desiredObject, actualObject, options, ) } func (e *ObjectEngine) objectUpdateHandling( ctx context.Context, - owner client.Object, + metadata types.RevisionMetadata, revision int64, desiredObject Object, actualObject Object, @@ -277,9 +251,9 @@ func (e *ObjectEngine) objectUpdateHandling( // An object already exists on the cluster. // Before doing anything else, we have to figure out // who owns and controls the object. - ctrlSit, actualOwner := e.detectOwner(owner, actualObject, options.PreviousOwners) + ctrlSit, actualOwner := e.detectOwner(metadata, actualObject, options.PreviousOwners) - compareRes, err := e.comparator.Compare(owner, desiredObject, actualObject) + compareRes, err := e.comparator.Compare(metadata, desiredObject, actualObject) if err != nil { return nil, fmt.Errorf("diverge check: %w", err) } @@ -412,12 +386,9 @@ func (e *ObjectEngine) objectUpdateHandling( // ObjectResult ModifiedFields does not contain ownerReference changes // introduced here, this may lead to Updated Actions without modifications. e.setObjectRevision(desiredObject, revision) - e.ownerStrategy.CopyOwnerReferences(actualObject, desiredObject) - e.ownerStrategy.ReleaseController(desiredObject) + metadata.CopyReferences(actualObject, desiredObject) - if err := e.ownerStrategy.SetControllerReference( - owner, desiredObject, - ); err != nil { + if err := metadata.SetCurrent(desiredObject); err != nil { return nil, fmt.Errorf("set controller reference: %w", err) } @@ -513,33 +484,33 @@ const ( ) func (e *ObjectEngine) detectOwner( - owner client.Object, + metadata types.RevisionMetadata, actualObject Object, - previousOwners []client.Object, -) (ctrlSituation, *metav1.OwnerReference) { - // e.ownerStrategy may either work on .metadata.ownerReferences or + previousOwners []types.RevisionMetadata, +) (ctrlSituation, types.RevisionReference) { + // metadata.GetCurrent may either work on .metadata.ownerReferences or // on an annotation to allow cross-namespace and cross-cluster refs. - ownerRef, ok := e.ownerStrategy.GetController(actualObject) - if !ok { + ownerRef := metadata.GetCurrent(actualObject) + if ownerRef == nil { return ctrlSituationNoController, nil } // Are we already controller? - if e.ownerStrategy.IsController(owner, actualObject) { - return ctrlSituationIsController, &ownerRef + if metadata.IsCurrent(actualObject) { + return ctrlSituationIsController, ownerRef } // Check if previous owner is controller. for _, previousOwner := range previousOwners { - if e.ownerStrategy.IsController(previousOwner, actualObject) { - return ctrlSituationPreviousIsController, &ownerRef + if previousOwner.IsCurrent(actualObject) { + return ctrlSituationPreviousIsController, ownerRef } } // Anyone else controller? // This statement can only resolve to true if annotations // are used for owner reference tracking. - return ctrlSituationUnknownController, &ownerRef + return ctrlSituationUnknownController, ownerRef } // Stores the revision number in a well-known annotation on the given object. diff --git a/machinery/objects_test.go b/machinery/objects_test.go index 7aa4e1ae..5b8e0472 100644 --- a/machinery/objects_test.go +++ b/machinery/objects_test.go @@ -109,7 +109,7 @@ func TestObjectEngine(t *testing.T) { }). Return(nil) ddm. - On("Compare", owner, mock.Anything, mock.Anything). + On("Compare", mock.Anything, mock.Anything, mock.Anything). Return(CompareResult{}, nil) writer. @@ -175,7 +175,7 @@ func TestObjectEngine(t *testing.T) { ). Return(apierrors.NewNotFound(schema.GroupResource{}, "")) ddm. - On("Compare", owner, mock.Anything, mock.Anything). + On("Compare", mock.Anything, mock.Anything, mock.Anything). Return(CompareResult{}, nil) writer. @@ -257,7 +257,7 @@ func TestObjectEngine(t *testing.T) { }). Return(nil) ddm. - On("Compare", owner, mock.Anything, mock.Anything). + On("Compare", mock.Anything, mock.Anything, mock.Anything). Return(CompareResult{}, nil) writer. @@ -336,7 +336,7 @@ func TestObjectEngine(t *testing.T) { }). Return(nil) ddm. - On("Compare", owner, mock.Anything, mock.Anything). + On("Compare", mock.Anything, mock.Anything, mock.Anything). Return(CompareResult{}, nil) writer. @@ -427,7 +427,7 @@ func TestObjectEngine(t *testing.T) { fs := &fieldpath.Set{} fs.Insert(fieldpath.MakePathOrDie("spec", "banana")) ddm. - On("Compare", owner, mock.Anything, mock.Anything). + On("Compare", mock.Anything, mock.Anything, mock.Anything). Return(CompareResult{ Comparison: &typed.Comparison{ Added: &fieldpath.Set{}, @@ -527,7 +527,7 @@ func TestObjectEngine(t *testing.T) { fs := &fieldpath.Set{} fs.Insert(fieldpath.MakePathOrDie("spec", "banana")) ddm. - On("Compare", owner, mock.Anything, mock.Anything). + On("Compare", mock.Anything, mock.Anything, mock.Anything). Return(CompareResult{ ConflictingMangers: []CompareResultManagedFields{ {Manager: "xxx"}, @@ -625,7 +625,7 @@ func TestObjectEngine(t *testing.T) { fs := &fieldpath.Set{} fs.Insert(fieldpath.MakePathOrDie("spec", "banana")) ddm. - On("Compare", owner, mock.Anything, mock.Anything). + On("Compare", mock.Anything, mock.Anything, mock.Anything). Return(CompareResult{}, nil) writer. @@ -672,7 +672,7 @@ func TestObjectEngine(t *testing.T) { }, }, opts: []types.ObjectReconcileOption{ - types.WithPreviousOwners{oldOwner}, + types.WithPreviousOwners{ownerhandling.NewNativeRevisionMetadata(oldOwner, scheme.Scheme)}, }, mockSetup: func( @@ -718,7 +718,7 @@ func TestObjectEngine(t *testing.T) { fs := &fieldpath.Set{} fs.Insert(fieldpath.MakePathOrDie("spec", "banana")) ddm. - On("Compare", owner, mock.Anything, mock.Anything). + On("Compare", mock.Anything, mock.Anything, mock.Anything). Return(CompareResult{}, nil) writer. @@ -809,7 +809,7 @@ func TestObjectEngine(t *testing.T) { fs := &fieldpath.Set{} fs.Insert(fieldpath.MakePathOrDie("spec", "banana")) ddm. - On("Compare", owner, mock.Anything, mock.Anything). + On("Compare", mock.Anything, mock.Anything, mock.Anything). Return(CompareResult{}, nil) writer. @@ -839,13 +839,12 @@ func TestObjectEngine(t *testing.T) { cache := &cacheMock{} writer := testutil.NewClient() - ownerStrategy := ownerhandling.NewNative(scheme.Scheme) divergeDetector := &comparatorMock{} oe := NewObjectEngine( scheme.Scheme, cache, writer, - ownerStrategy, divergeDetector, + divergeDetector, testFieldOwner, testSystemPrefix, ) @@ -854,8 +853,9 @@ func TestObjectEngine(t *testing.T) { //nolint:usetesting ctx := context.Background() + ownerMeta := ownerhandling.NewNativeRevisionMetadata(owner, scheme.Scheme) res, err := oe.Reconcile( - ctx, owner, 1, test.desiredObject, + ctx, ownerMeta, 1, test.desiredObject, test.opts..., ) require.NoError(t, err) @@ -882,20 +882,25 @@ func TestObjectEngine_Reconcile_SanityChecks(t *testing.T) { t.Parallel() oe := &ObjectEngine{} - owner := &unstructured.Unstructured{} desired := &unstructured.Unstructured{} t.Run("missing revision", func(t *testing.T) { t.Parallel() + + mockMeta := &mockRevisionMetadata{} + assert.PanicsWithValue(t, "owner revision must be set and start at 1", func() { - _, _ = oe.Reconcile(t.Context(), owner, 0, desired) + _, _ = oe.Reconcile(t.Context(), mockMeta, 0, desired) }) }) - t.Run("missing owner.UID", func(t *testing.T) { + t.Run("missing owner.UID in NewNativeRevisionMetadata", func(t *testing.T) { t.Parallel() - assert.PanicsWithValue(t, "owner must be persistet to cluster, empty UID", func() { - _, _ = oe.Reconcile(t.Context(), owner, 1, desired) + + owner := &unstructured.Unstructured{} + + assert.PanicsWithValue(t, "owner must be persisted to cluster, empty UID", func() { + _ = ownerhandling.NewNativeRevisionMetadata(owner, scheme.Scheme) }) }) } @@ -916,13 +921,13 @@ func TestObjectEngine_Reconcile_UnsupportedTypedObject(t *testing.T) { cache := &cacheMock{} writer := testutil.NewClient() - ownerStrategy := ownerhandling.NewNative(scheme.Scheme) + revisionMetadata := ownerhandling.NewNativeRevisionMetadata(owner, scheme.Scheme) divergeDetector := &comparatorMock{} oe := NewObjectEngine( scheme.Scheme, cache, writer, - ownerStrategy, divergeDetector, + divergeDetector, testFieldOwner, testSystemPrefix, ) @@ -972,7 +977,7 @@ func TestObjectEngine_Reconcile_UnsupportedTypedObject(t *testing.T) { fs := &fieldpath.Set{} fs.Insert(fieldpath.MakePathOrDie("data", "key")) divergeDetector. - On("Compare", owner, mock.Anything, mock.Anything). + On("Compare", revisionMetadata, mock.Anything, mock.Anything). Return(CompareResult{ Comparison: &typed.Comparison{ Added: &fieldpath.Set{}, @@ -983,7 +988,7 @@ func TestObjectEngine_Reconcile_UnsupportedTypedObject(t *testing.T) { //nolint:usetesting ctx := context.Background() - res, err := oe.Reconcile(ctx, owner, 1, desiredObject) + res, err := oe.Reconcile(ctx, revisionMetadata, 1, desiredObject) // Should return UnsupportedApplyConfigurationError require.Error(t, err) @@ -1061,7 +1066,7 @@ func TestObjectEngine_TeardownWithTeardownWriter(t *testing.T) { cache := &cacheMock{} engineWriter := testutil.NewClient() // default engine writer teardownWriter := testutil.NewClient() // used during tearDown - ownerStrategy := ownerhandling.NewNative(scheme.Scheme) + revisionMetadata := ownerhandling.NewNativeRevisionMetadata(owner, scheme.Scheme) divergeDetector := &comparatorMock{} cache. @@ -1090,12 +1095,12 @@ func TestObjectEngine_TeardownWithTeardownWriter(t *testing.T) { oe := NewObjectEngine( scheme.Scheme, cache, engineWriter, - ownerStrategy, divergeDetector, + divergeDetector, testFieldOwner, testSystemPrefix, ) - result, err := oe.Teardown(t.Context(), owner, 1, obj, types.WithTeardownWriter(teardownWriter)) + result, err := oe.Teardown(t.Context(), revisionMetadata, 1, obj, types.WithTeardownWriter(teardownWriter)) if test.expectedError != nil { assert.EqualError(t, err, test.expectedError.Error()) } else { @@ -1283,7 +1288,6 @@ func TestObjectEngine_Teardown(t *testing.T) { cache := &cacheMock{} writer := testutil.NewClient() - ownerStrategy := ownerhandling.NewNative(scheme.Scheme) divergeDetector := &comparatorMock{} cache. @@ -1296,12 +1300,14 @@ func TestObjectEngine_Teardown(t *testing.T) { oe := NewObjectEngine( scheme.Scheme, cache, writer, - ownerStrategy, divergeDetector, + divergeDetector, testFieldOwner, testSystemPrefix, ) - deleted, err := oe.Teardown(t.Context(), owner, 1, obj) + ownerMeta := ownerhandling.NewNativeRevisionMetadata(owner, scheme.Scheme) + + deleted, err := oe.Teardown(t.Context(), ownerMeta, 1, obj) if test.expectedError != nil { assert.EqualError(t, err, test.expectedError.Error()) } else { @@ -1336,7 +1342,6 @@ func TestObjectEngine_Teardown_Orphan(t *testing.T) { cache := &cacheMock{} writer := testutil.NewClient() - ownerStrategy := ownerhandling.NewNative(scheme.Scheme) divergeDetector := &comparatorMock{} cache. @@ -1347,11 +1352,12 @@ func TestObjectEngine_Teardown_Orphan(t *testing.T) { oe := NewObjectEngine( scheme.Scheme, cache, writer, - ownerStrategy, divergeDetector, + divergeDetector, testFieldOwner, testSystemPrefix, ) - deleted, err := oe.Teardown(t.Context(), owner, 1, obj, types.WithOrphan()) + ownerMeta := ownerhandling.NewNativeRevisionMetadata(owner, scheme.Scheme) + deleted, err := oe.Teardown(t.Context(), ownerMeta, 1, obj, types.WithOrphan()) require.NoError(t, err) assert.True(t, deleted) @@ -1361,20 +1367,25 @@ func TestObjectEngine_Teardown_SanityChecks(t *testing.T) { t.Parallel() oe := &ObjectEngine{} - owner := &unstructured.Unstructured{} desired := &unstructured.Unstructured{} t.Run("missing revision", func(t *testing.T) { t.Parallel() + + mockMeta := &mockRevisionMetadata{} + assert.PanicsWithValue(t, "owner revision must be set and start at 1", func() { - _, _ = oe.Teardown(t.Context(), owner, 0, desired) + _, _ = oe.Teardown(t.Context(), mockMeta, 0, desired) }) }) - t.Run("missing owner.UID", func(t *testing.T) { + t.Run("missing owner.UID in NewNativeRevisionMetadata", func(t *testing.T) { t.Parallel() + + owner := &unstructured.Unstructured{} + assert.PanicsWithValue(t, "owner must be persisted to cluster, empty UID", func() { - _, _ = oe.Teardown(t.Context(), owner, 1, desired) + _ = ownerhandling.NewNativeRevisionMetadata(owner, scheme.Scheme) }) }) } @@ -1388,10 +1399,25 @@ type comparatorMock struct { } func (m *comparatorMock) Compare( - owner client.Object, + metadata types.RevisionMetadata, desiredObject, actualObject Object, ) (CompareResult, error) { - args := m.Called(owner, desiredObject, actualObject) + args := m.Called(metadata, desiredObject, actualObject) return args.Get(0).(CompareResult), args.Error(1) } + +type mockRevisionMetadata struct{} + +func (m *mockRevisionMetadata) GetReconcileOptions() []types.RevisionReconcileOption { return nil } +func (m *mockRevisionMetadata) GetTeardownOptions() []types.RevisionTeardownOption { return nil } +func (m *mockRevisionMetadata) SetCurrent(_ metav1.Object, _ ...types.SetCurrentOption) error { + return nil +} +func (m *mockRevisionMetadata) IsCurrent(_ metav1.Object) bool { return false } +func (m *mockRevisionMetadata) RemoveFrom(_ metav1.Object) {} +func (m *mockRevisionMetadata) IsNamespaceAllowed(_ metav1.Object) bool { return true } +func (m *mockRevisionMetadata) CopyReferences(_, _ metav1.Object) {} +func (m *mockRevisionMetadata) GetCurrent(_ metav1.Object) types.RevisionReference { return nil } + +var _ types.RevisionMetadata = &mockRevisionMetadata{} diff --git a/machinery/phases.go b/machinery/phases.go index 18eb2b1a..235af6cb 100644 --- a/machinery/phases.go +++ b/machinery/phases.go @@ -7,7 +7,6 @@ import ( "strings" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "sigs.k8s.io/controller-runtime/pkg/client" "pkg.package-operator.run/boxcutter/machinery/types" "pkg.package-operator.run/boxcutter/validation" @@ -35,7 +34,7 @@ func NewPhaseEngine( type phaseValidator interface { Validate( ctx context.Context, - owner client.Object, + metadata types.RevisionMetadata, phase types.Phase, ) error } @@ -43,14 +42,14 @@ type phaseValidator interface { type objectEngine interface { Reconcile( ctx context.Context, - owner client.Object, // Owner of the object. + metadata types.RevisionMetadata, // Metadata for the owner. revision int64, // Revision number, must start at 1. desiredObject Object, opts ...types.ObjectReconcileOption, ) (ObjectResult, error) Teardown( ctx context.Context, - owner client.Object, // Owner of the object. + metadata types.RevisionMetadata, // Metadata for the owner. revision int64, // Revision number, must start at 1. desiredObject Object, opts ...types.ObjectTeardownOption, @@ -132,7 +131,7 @@ func (r *phaseTeardownResult) Waiting() []types.ObjectRef { // Teardown ensures the given phase is safely removed from the cluster. func (e *PhaseEngine) Teardown( ctx context.Context, - owner client.Object, + metadata types.RevisionMetadata, revision int64, phase types.Phase, opts ...types.PhaseTeardownOption, @@ -148,7 +147,7 @@ func (e *PhaseEngine) Teardown( obj := &o gone, err := e.objectEngine.Teardown( - ctx, owner, revision, obj, options.ForObject(obj)...) + ctx, metadata, revision, obj, options.ForObject(obj)...) if err != nil { return res, fmt.Errorf("teardown object: %w", err) } @@ -166,7 +165,7 @@ func (e *PhaseEngine) Teardown( // Reconcile runs actions to bring actual state closer to desired. func (e *PhaseEngine) Reconcile( ctx context.Context, - owner client.Object, + metadata types.RevisionMetadata, revision int64, phase types.Phase, opts ...types.PhaseReconcileOption, @@ -181,7 +180,7 @@ func (e *PhaseEngine) Reconcile( } // Preflight - err := e.phaseValidator.Validate(ctx, owner, phase) + err := e.phaseValidator.Validate(ctx, metadata, phase) if err != nil { var perr validation.PhaseValidationError if errors.As(err, &perr) { @@ -198,7 +197,7 @@ func (e *PhaseEngine) Reconcile( obj := &o ores, err := e.objectEngine.Reconcile( - ctx, owner, revision, obj, options.ForObject(obj)...) + ctx, metadata, revision, obj, options.ForObject(obj)...) if err != nil { return pres, fmt.Errorf("reconciling object: %w", err) } diff --git a/machinery/phases_test.go b/machinery/phases_test.go index b687538e..e44de79a 100644 --- a/machinery/phases_test.go +++ b/machinery/phases_test.go @@ -11,10 +11,12 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "sigs.k8s.io/controller-runtime/pkg/client" "pkg.package-operator.run/boxcutter/machinery/types" + "pkg.package-operator.run/boxcutter/ownerhandling" "pkg.package-operator.run/boxcutter/validation" ) @@ -27,6 +29,11 @@ func TestPhaseEngine_Reconcile(t *testing.T) { pv := &phaseValidatorMock{} pe := NewPhaseEngine(oe, pv) + scheme := runtime.NewScheme() + if err := corev1.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + owner := &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ UID: "12345-678", @@ -35,6 +42,8 @@ func TestPhaseEngine_Reconcile(t *testing.T) { }, } + metadata := ownerhandling.NewNativeRevisionMetadata(owner, scheme) + obj := &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "v1", @@ -51,10 +60,10 @@ func TestPhaseEngine_Reconcile(t *testing.T) { pv. On("Validate", mock.Anything, mock.Anything, mock.Anything). Return(nil) - oe.On("Reconcile", mock.Anything, owner, revision, obj, mock.Anything). + oe.On("Reconcile", mock.Anything, mock.Anything, revision, obj, mock.Anything). Return(newObjectResultCreated(obj, types.ObjectReconcileOptions{}), nil) - _, err := pe.Reconcile(t.Context(), owner, revision, types.Phase{ + _, err := pe.Reconcile(t.Context(), metadata, revision, types.Phase{ Name: "test", Objects: []unstructured.Unstructured{ *obj, @@ -70,6 +79,11 @@ func TestPhaseEngine_Reconcile_PreflightViolation(t *testing.T) { pv := &phaseValidatorMock{} pe := NewPhaseEngine(oe, pv) + scheme := runtime.NewScheme() + if err := corev1.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + owner := &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ UID: "12345-678", @@ -78,6 +92,8 @@ func TestPhaseEngine_Reconcile_PreflightViolation(t *testing.T) { }, } + metadata := ownerhandling.NewNativeRevisionMetadata(owner, scheme) + obj := &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "v1", @@ -94,10 +110,10 @@ func TestPhaseEngine_Reconcile_PreflightViolation(t *testing.T) { pv. On("Validate", mock.Anything, mock.Anything, mock.Anything). Return(validation.PhaseValidationError{}) - oe.On("Reconcile", mock.Anything, owner, revision, obj, mock.Anything). + oe.On("Reconcile", mock.Anything, mock.Anything, revision, obj, mock.Anything). Return(newObjectResultCreated(obj, types.ObjectReconcileOptions{}), nil) - _, err := pe.Reconcile(t.Context(), owner, revision, types.Phase{ + _, err := pe.Reconcile(t.Context(), metadata, revision, types.Phase{ Name: "test", Objects: []unstructured.Unstructured{ *obj, @@ -117,6 +133,11 @@ func TestPhaseEngine_Teardown(t *testing.T) { pv := &phaseValidatorMock{} pe := NewPhaseEngine(oe, pv) + scheme := runtime.NewScheme() + if err := corev1.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + owner := &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ UID: "12345-678", @@ -125,6 +146,8 @@ func TestPhaseEngine_Teardown(t *testing.T) { }, } + metadata := ownerhandling.NewNativeRevisionMetadata(owner, scheme) + obj := &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "v1", @@ -138,10 +161,10 @@ func TestPhaseEngine_Teardown(t *testing.T) { var revision int64 = 1 - oe.On("Teardown", mock.Anything, owner, revision, obj, mock.Anything, mock.Anything). + oe.On("Teardown", mock.Anything, mock.Anything, revision, obj, mock.Anything, mock.Anything). Return(true, nil) - deleted, err := pe.Teardown(t.Context(), owner, revision, types.Phase{ + deleted, err := pe.Teardown(t.Context(), metadata, revision, types.Phase{ Name: "test", Objects: []unstructured.Unstructured{ *obj, *obj, @@ -159,24 +182,24 @@ type objectEngineMock struct { func (m *objectEngineMock) Reconcile( ctx context.Context, - owner client.Object, + metadata types.RevisionMetadata, revision int64, desiredObject Object, opts ...types.ObjectReconcileOption, ) (ObjectResult, error) { - args := m.Called(ctx, owner, revision, desiredObject, opts) + args := m.Called(ctx, metadata, revision, desiredObject, opts) return args.Get(0).(ObjectResult), args.Error(1) } func (m *objectEngineMock) Teardown( ctx context.Context, - owner client.Object, + metadata types.RevisionMetadata, revision int64, desiredObject Object, opts ...types.ObjectTeardownOption, ) (objectDeleted bool, err error) { - args := m.Called(ctx, owner, revision, desiredObject, opts) + args := m.Called(ctx, metadata, revision, desiredObject, opts) return args.Bool(0), args.Error(1) } @@ -187,10 +210,10 @@ type phaseValidatorMock struct { func (m *phaseValidatorMock) Validate( ctx context.Context, - owner client.Object, + metadata types.RevisionMetadata, phase types.Phase, ) error { - args := m.Called(ctx, owner, phase) + args := m.Called(ctx, metadata, phase) return args.Error(0) } diff --git a/machinery/results.go b/machinery/results.go index 97395d12..8ae7a3a3 100644 --- a/machinery/results.go +++ b/machinery/results.go @@ -4,7 +4,6 @@ import ( "fmt" "sort" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes/scheme" "pkg.package-operator.run/boxcutter/machinery/types" @@ -236,11 +235,11 @@ func (r normalResult) String() string { type ObjectResultCollision struct { normalResult // conflictingOwner is provided when Refusing due to Collision. - conflictingOwner *metav1.OwnerReference + conflictingOwner types.RevisionReference } // ConflictingOwner Conflicting owner if Action == RefusingConflict. -func (r ObjectResultCollision) ConflictingOwner() (*metav1.OwnerReference, bool) { +func (r ObjectResultCollision) ConflictingOwner() (types.RevisionReference, bool) { return r.conflictingOwner, r.conflictingOwner != nil } @@ -262,7 +261,7 @@ func (r ObjectResultCollision) String() string { func newObjectResultConflict( obj Object, diverged CompareResult, - conflictingOwner *metav1.OwnerReference, + conflictingOwner types.RevisionReference, options types.ObjectReconcileOptions, ) ObjectResult { return ObjectResultCollision{ diff --git a/machinery/revision.go b/machinery/revision.go index 84dfc854..eb7c3c6a 100644 --- a/machinery/revision.go +++ b/machinery/revision.go @@ -40,14 +40,14 @@ type revisionValidator interface { type phaseEngine interface { Reconcile( ctx context.Context, - owner client.Object, + metadata types.RevisionMetadata, revision int64, phase types.Phase, opts ...types.PhaseReconcileOption, ) (PhaseResult, error) Teardown( ctx context.Context, - owner client.Object, + metadata types.RevisionMetadata, revision int64, phase types.Phase, opts ...types.PhaseTeardownOption, @@ -180,7 +180,7 @@ func (re *RevisionEngine) Reconcile( opts ...types.RevisionReconcileOption, ) (RevisionResult, error) { var options types.RevisionReconcileOptions - for _, opt := range opts { + for _, opt := range slices.Concat(rev.GetMetadata().GetReconcileOptions(), opts) { opt.ApplyToRevisionReconcileOptions(&options) } @@ -204,7 +204,7 @@ func (re *RevisionEngine) Reconcile( // Reconcile for _, phase := range rev.GetPhases() { pres, err := re.phaseEngine.Reconcile( - ctx, rev.GetOwner(), rev.GetRevisionNumber(), + ctx, rev.GetMetadata(), rev.GetRevisionNumber(), phase, options.ForPhase(phase.GetName())...) if err != nil { return rres, fmt.Errorf("reconciling object: %w", err) @@ -315,7 +315,7 @@ func (re *RevisionEngine) Teardown( opts ...types.RevisionTeardownOption, ) (RevisionTeardownResult, error) { var options types.RevisionTeardownOptions - for _, opt := range opts { + for _, opt := range slices.Concat(rev.GetMetadata().GetTeardownOptions(), opts) { opt.ApplyToRevisionTeardownOptions(&options) } @@ -336,7 +336,7 @@ func (re *RevisionEngine) Teardown( res.active = p.GetName() pres, err := re.phaseEngine.Teardown( - ctx, rev.GetOwner(), rev.GetRevisionNumber(), + ctx, rev.GetMetadata(), rev.GetRevisionNumber(), p, options.ForPhase(p.GetName())...) if err != nil { return nil, fmt.Errorf("teardown phase: %w", err) diff --git a/machinery/revision_test.go b/machinery/revision_test.go index 55814de0..7e322903 100644 --- a/machinery/revision_test.go +++ b/machinery/revision_test.go @@ -10,10 +10,11 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "sigs.k8s.io/controller-runtime/pkg/client" + "k8s.io/apimachinery/pkg/runtime" "pkg.package-operator.run/boxcutter/internal/testutil" "pkg.package-operator.run/boxcutter/machinery/types" + "pkg.package-operator.run/boxcutter/ownerhandling" "pkg.package-operator.run/boxcutter/validation" ) @@ -26,6 +27,11 @@ func TestRevisionEngine_Teardown(t *testing.T) { re := NewRevisionEngine(pe, rv, c) + scheme := runtime.NewScheme() + if err := corev1.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + owner := &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ UID: "12345-678", @@ -34,18 +40,20 @@ func TestRevisionEngine_Teardown(t *testing.T) { }, } - rev := types.Revision{ - Owner: owner, - Revision: 3, - Phases: []types.Phase{ + metadata := ownerhandling.NewNativeRevisionMetadata(owner, scheme) + + rev := types.NewRevision( + "test", + metadata, + 3, []types.Phase{ {Name: "phase-1"}, {Name: "phase-2"}, {Name: "phase-3"}, }, - } + ) pe. - On("Teardown", mock.Anything, owner, mock.Anything, mock.Anything, mock.Anything). + On("Teardown", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything). Return(&phaseTeardownResult{}, nil) res, err := re.Teardown(t.Context(), rev) @@ -72,6 +80,11 @@ func TestRevisionEngine_Teardown_delayed(t *testing.T) { re := NewRevisionEngine(pe, rv, c) + scheme := runtime.NewScheme() + if err := corev1.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + owner := &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ UID: "12345-678", @@ -80,23 +93,25 @@ func TestRevisionEngine_Teardown_delayed(t *testing.T) { }, } - rev := types.Revision{ - Owner: owner, - Revision: 3, - Phases: []types.Phase{ + metadata := ownerhandling.NewNativeRevisionMetadata(owner, scheme) + + rev := types.NewRevision( + "test", + metadata, + 3, []types.Phase{ {Name: "phase-1"}, {Name: "phase-2"}, {Name: "phase-3"}, {Name: "phase-4"}, }, - } + ) pe. - On("Teardown", mock.Anything, owner, mock.Anything, mock.Anything, mock.Anything). + On("Teardown", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything). Twice(). Return(&phaseTeardownResult{}, nil) pe. - On("Teardown", mock.Anything, owner, mock.Anything, mock.Anything, mock.Anything). + On("Teardown", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything). Return(&phaseTeardownResult{waiting: []types.ObjectRef{ {}, }}, nil) @@ -120,24 +135,24 @@ type phaseEngineMock struct { func (m *phaseEngineMock) Reconcile( ctx context.Context, - owner client.Object, + metadata types.RevisionMetadata, revision int64, phase types.Phase, opts ...types.PhaseReconcileOption, ) (PhaseResult, error) { - args := m.Called(ctx, owner, revision, phase, opts) + args := m.Called(ctx, metadata, revision, phase, opts) return args.Get(0).(PhaseResult), args.Error(1) } func (m *phaseEngineMock) Teardown( ctx context.Context, - owner client.Object, + metadata types.RevisionMetadata, revision int64, phase types.Phase, opts ...types.PhaseTeardownOption, ) (PhaseTeardownResult, error) { - args := m.Called(ctx, owner, revision, phase, opts) + args := m.Called(ctx, metadata, revision, phase, opts) return args.Get(0).(PhaseTeardownResult), args.Error(1) } diff --git a/machinery/types/metadata.go b/machinery/types/metadata.go new file mode 100644 index 00000000..c512d26a --- /dev/null +++ b/machinery/types/metadata.go @@ -0,0 +1,61 @@ +package types + +import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +// RevisionMetadata manages revision ownership metadata of objects. +// Implementations may store ownership information in various ways +// (native ownerReferences, annotations, or external systems). +type RevisionMetadata interface { + // GetReconcileOptions returns a set of options that will added to any + // revision reconciliation options. + GetReconcileOptions() []RevisionReconcileOption + + // GetTeardownOptions returns a set of options that will added to any + // revision teardown options. + GetTeardownOptions() []RevisionTeardownOption + + // SetCurrent updates obj to mark this RevisionMetadata as the current (controlling) revision. + // Returns an error if the object already has a different current revision. + SetCurrent(obj metav1.Object, opts ...SetCurrentOption) error + + // IsCurrent returns true if this RevisionMetadata is the current (controlling) revision of obj. + IsCurrent(obj metav1.Object) bool + + // RemoveFrom removes this RevisionMetadata from obj, whether it is the current revision or otherwise. + RemoveFrom(obj metav1.Object) + + // IsNamespaceAllowed returns true if objects may be created/managed in the namespace of obj. + // The behavior is determined by the constructor configuration (e.g., same-namespace only vs cross-namespace). + IsNamespaceAllowed(obj metav1.Object) bool + + // 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. + CopyReferences(objA, objB metav1.Object) + + // GetCurrent returns a RevisionReference describing the current revision of obj. + // Returns nil if there is no current revision set. + GetCurrent(obj metav1.Object) RevisionReference +} + +// RevisionReference provides information about a revision owner. +// Implementations should return the underlying type when possible +// (e.g., metav1.OwnerReference for Native/Annotation strategies). +type RevisionReference interface { + String() string +} + +// SetCurrentOption is an option which can be passed to SetCurrent. +type SetCurrentOption func(*SetCurrentOptions) + +// SetCurrentOptions is a set of options used internally by SetCurrent. +type SetCurrentOptions struct { + AllowUpdate bool +} + +// WithAllowUpdate permits SetCurrent to update the current revision without +// returning an error if the object already has a different current revision. +func WithAllowUpdate(o *SetCurrentOptions) { + o.AllowUpdate = true +} diff --git a/machinery/types/metadata_test.go b/machinery/types/metadata_test.go new file mode 100644 index 00000000..84407cee --- /dev/null +++ b/machinery/types/metadata_test.go @@ -0,0 +1,22 @@ +package types + +import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +// mockRevisionMetadata is a test mock for RevisionMetadata. +// Not tested directly here, but used in other tests. +type mockRevisionMetadata struct { + name string +} + +func (m *mockRevisionMetadata) GetReconcileOptions() []RevisionReconcileOption { return nil } +func (m *mockRevisionMetadata) GetTeardownOptions() []RevisionTeardownOption { return nil } +func (m *mockRevisionMetadata) SetCurrent(metav1.Object, ...SetCurrentOption) error { return nil } +func (m *mockRevisionMetadata) IsCurrent(metav1.Object) bool { return false } +func (m *mockRevisionMetadata) RemoveFrom(metav1.Object) {} +func (m *mockRevisionMetadata) IsNamespaceAllowed(metav1.Object) bool { return true } +func (m *mockRevisionMetadata) CopyReferences(metav1.Object, metav1.Object) {} +func (m *mockRevisionMetadata) GetCurrent(metav1.Object) RevisionReference { return nil } + +var _ RevisionMetadata = &mockRevisionMetadata{} diff --git a/machinery/types/options.go b/machinery/types/options.go index 8b2647b9..e8eb50e7 100644 --- a/machinery/types/options.go +++ b/machinery/types/options.go @@ -101,7 +101,7 @@ type PhaseTeardownOption interface { // ObjectReconcileOptions holds configuration options changing object reconciliation. type ObjectReconcileOptions struct { CollisionProtection CollisionProtection - PreviousOwners []client.Object + PreviousOwners []RevisionMetadata Paused bool Probes map[string]Prober } @@ -180,9 +180,9 @@ func (p WithCollisionProtection) ApplyToRevisionReconcileOptions(opts *RevisionR opts.DefaultPhaseOptions = append(opts.DefaultPhaseOptions, p) } -// WithPreviousOwners is a list of known objects allowed to take ownership from. +// WithPreviousOwners is a list of known revision metadata allowed to take ownership from. // Objects from this list will not trigger collision detection and prevention. -type WithPreviousOwners []client.Object +type WithPreviousOwners []RevisionMetadata // ApplyToObjectReconcileOptions implements ObjectReconcileOption. func (p WithPreviousOwners) ApplyToObjectReconcileOptions(opts *ObjectReconcileOptions) { diff --git a/machinery/types/options_test.go b/machinery/types/options_test.go index 7bf54fa1..1304f769 100644 --- a/machinery/types/options_test.go +++ b/machinery/types/options_test.go @@ -256,27 +256,17 @@ func TestWithCollisionProtection(t *testing.T) { func TestWithPreviousOwners(t *testing.T) { t.Parallel() - owner1 := &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: "owner1", - Namespace: "test", - }, - } - owner2 := &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: "owner2", - Namespace: "test", - }, - } + meta1 := &mockRevisionMetadata{name: "owner1"} + meta2 := &mockRevisionMetadata{name: "owner2"} - previousOwners := WithPreviousOwners{owner1, owner2} + previousOwners := WithPreviousOwners{meta1, meta2} t.Run("applies to object reconcile options", func(t *testing.T) { t.Parallel() opts := &ObjectReconcileOptions{} previousOwners.ApplyToObjectReconcileOptions(opts) - assert.Equal(t, []client.Object{owner1, owner2}, opts.PreviousOwners) + assert.Equal(t, []RevisionMetadata{meta1, meta2}, opts.PreviousOwners) }) t.Run("applies to phase reconcile options", func(t *testing.T) { diff --git a/machinery/types/types.go b/machinery/types/types.go index 175a4cc9..cecba828 100644 --- a/machinery/types/types.go +++ b/machinery/types/types.go @@ -9,6 +9,8 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" ) +// Note: RevisionMetadata and RevisionReference interfaces are defined in metadata.go + // ObjectRef holds information to identify an object. type ObjectRef struct { schema.GroupVersionKind @@ -46,13 +48,22 @@ func (p *Phase) GetObjects() []unstructured.Unstructured { return p.Objects } -// Revision represents the version of a content collection consisting of phases. -type Revision struct { +// NewRevision creates a new Revision instance. +// It is primarily a convenience function to take advantage of type inference. +func NewRevision[T RevisionMetadata](name string, metadata T, revision int64, phases []Phase) *RevisionImpl[T] { + return &RevisionImpl[T]{ + Name: name, + Metadata: metadata, + Revision: revision, + Phases: phases, + } +} + +type RevisionImpl[T RevisionMetadata] struct { // Name of the Revision. Name string - // Owner object will be added as OwnerReference - // to all objects managed by this revision. - Owner client.Object + // Metadata manages revision ownership metadata of objects. + Metadata T // Revision number. Revision int64 // Ordered list of phases. @@ -60,21 +71,38 @@ type Revision struct { } // GetName returns the name of the revision. -func (r *Revision) GetName() string { +func (r *RevisionImpl[_]) GetName() string { return r.Name } -// GetOwner returns the owning object. -func (r *Revision) GetOwner() client.Object { - return r.Owner +// GetMetadata returns the revision metadata handler. +func (r *RevisionImpl[_]) GetMetadata() RevisionMetadata { + return r.Metadata } // GetRevisionNumber returns the current revision number. -func (r *Revision) GetRevisionNumber() int64 { +func (r *RevisionImpl[_]) GetRevisionNumber() int64 { return r.Revision } // GetPhases returns the phases a revision is made up of. -func (r *Revision) GetPhases() []Phase { +func (r *RevisionImpl[_]) GetPhases() []Phase { return r.Phases } + +var _ Revision = &RevisionImpl[RevisionMetadata]{} + +// Revision represents the version of a content collection consisting of phases. +type Revision interface { + // GetName returns the name of the revision. + GetName() string + + // GetMetadata returns the revision metadata handler. + GetMetadata() RevisionMetadata + + // GetRevisionNumber returns the current revision number. + GetRevisionNumber() int64 + + // GetPhases returns the phases a revision is made up of. + GetPhases() []Phase +} diff --git a/machinery/types/types_test.go b/machinery/types/types_test.go index 861e0ce2..31c39504 100644 --- a/machinery/types/types_test.go +++ b/machinery/types/types_test.go @@ -201,34 +201,29 @@ func TestPhase_GetObjects(t *testing.T) { func TestRevision_GetName(t *testing.T) { t.Parallel() - revision := &Revision{ + revision := &RevisionImpl[RevisionMetadata]{ Name: "test-revision", } assert.Equal(t, "test-revision", revision.GetName()) } -func TestRevision_GetOwner(t *testing.T) { +func TestRevision_GetMetadata(t *testing.T) { t.Parallel() - owner := &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: "owner", - Namespace: "test", - }, - } + metadata := &mockRevisionMetadata{name: "test"} - revision := &Revision{ - Owner: owner, + revision := &RevisionImpl[RevisionMetadata]{ + Metadata: metadata, } - assert.Equal(t, owner, revision.GetOwner()) + assert.Equal(t, metadata, revision.GetMetadata()) } func TestRevision_GetRevisionNumber(t *testing.T) { t.Parallel() - revision := &Revision{ + revision := &RevisionImpl[RevisionMetadata]{ Revision: 42, } @@ -269,7 +264,7 @@ func TestRevision_GetPhases(t *testing.T) { }, } - revision := &Revision{ + revision := &RevisionImpl[RevisionMetadata]{ Phases: phases, } diff --git a/ownerhandling/annotation.go b/ownerhandling/annotation.go index f786e532..00afb063 100644 --- a/ownerhandling/annotation.go +++ b/ownerhandling/annotation.go @@ -5,12 +5,11 @@ import ( "encoding/json" "errors" "fmt" + "slices" - "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" - "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/util/workqueue" "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" @@ -19,158 +18,133 @@ import ( "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/reconcile" + + bctypes "pkg.package-operator.run/boxcutter/machinery/types" ) -var _ ownerStrategy = (*OwnerStrategyAnnotation)(nil) +// AnnotationStrategy can be used to create a RevisionMetadata which uses an +// annotation key to store owner references, and an event handler that enqueues +// reconcile requests for the object referenced by the annotation. The +// AnnotationStrategy itself specifies the annotation key to use. +type AnnotationStrategy string + +// NewAnnotationStrategy creates an AnnotationStrategy which uses the given +// annotation key. +func NewAnnotationStrategy(annotationKey string) AnnotationStrategy { + return AnnotationStrategy(annotationKey) +} -// OwnerStrategyAnnotation handling strategy uses .metadata.annotations. -// Allows cross-namespace owner references. -type OwnerStrategyAnnotation struct { +// Ensure AnnotationRevisionMetadata implements RevisionMetadata. +var _ bctypes.RevisionMetadata = (*AnnotationRevisionMetadata)(nil) + +// AnnotationRevisionMetadata uses annotations for cross-namespace ownership tracking. +// Cross-namespace is always allowed (this is the primary purpose of annotation-based ownership). +type AnnotationRevisionMetadata struct { + owner client.Object scheme *runtime.Scheme annotationKey string } -// NewAnnotation returns a new OwnerStrategyAnnotation instance. -func NewAnnotation(scheme *runtime.Scheme, annotationKey string) *OwnerStrategyAnnotation { - return &OwnerStrategyAnnotation{ - scheme: scheme, - annotationKey: annotationKey, +// NewRevisionMetadata creates a RevisionMetadata using annotation-based ownership. +// IsNamespaceAllowed() always returns true since cross-namespace support is the primary +// purpose of annotation-based ownership. +// Panics if owner has an empty UID (not persisted to cluster). +func (h AnnotationStrategy) NewRevisionMetadata( + owner client.Object, + scheme *runtime.Scheme, +) *AnnotationRevisionMetadata { + if len(owner.GetUID()) == 0 { + panic("owner must be persisted to cluster, empty UID") } -} -// GetController returns the OwnerReference with Controller==true, if one exist. -func (s *OwnerStrategyAnnotation) GetController(obj metav1.Object) ( - metav1.OwnerReference, bool, -) { - for _, ref := range s.getOwnerReferences(obj) { - if ref.Controller != nil && *ref.Controller { - return ref.ToMetaV1OwnerRef(), true - } + return &AnnotationRevisionMetadata{ + owner: owner, + scheme: scheme, + annotationKey: string(h), } - - return metav1.OwnerReference{}, false } -// CopyOwnerReferences copies all OwnerReferences from objA to objB, -// overriding any existing OwnerReferences on objB. -func (s *OwnerStrategyAnnotation) CopyOwnerReferences(objA, objB metav1.Object) { - s.setOwnerReferences(objB, s.getOwnerReferences(objA)) +// GetReconcileOptions returns a set of options that will added to any +// revision reconciliation options. +// For annotation-based ownership, there are no default reconciliation options. +func (m *AnnotationRevisionMetadata) GetReconcileOptions() []bctypes.RevisionReconcileOption { + return nil } -// EnqueueRequestForOwner returns a EventHandler to enqueue the owner. -func (s *OwnerStrategyAnnotation) EnqueueRequestForOwner( - ownerType client.Object, _ meta.RESTMapper, isController bool, -) handler.EventHandler { - a := &AnnotationEnqueueRequestForOwner{ - OwnerType: ownerType, - IsController: isController, - ownerStrategy: s, - } - if err := a.parseOwnerTypeGroupKind(s.scheme); err != nil { - // This (passing a type that is not in the scheme) HAS to be a - // programmer error and can't be recovered at runtime anyways. - panic(err) - } - - return a +// GetTeardownOptions returns a set of options that will added to any +// revision teardown options. +// For annotation-based ownership, there are no default teardown options. +func (m *AnnotationRevisionMetadata) GetTeardownOptions() []bctypes.RevisionTeardownOption { + return nil } -// SetOwnerReference adds owner as OwnerReference to obj, with Controller set to false. -func (s *OwnerStrategyAnnotation) SetOwnerReference(owner, obj metav1.Object) error { - ownerRefs := s.getOwnerReferences(obj) - - gvk, err := apiutil.GVKForObject(owner.(runtime.Object), s.scheme) - if err != nil { - return err - } - - ownerRef := annotationOwnerRef{ - APIVersion: gvk.GroupVersion().String(), - Kind: gvk.Kind, - UID: owner.GetUID(), - Name: owner.GetName(), - Namespace: owner.GetNamespace(), - } - - ownerIndex := s.indexOf(ownerRefs, ownerRef) - if ownerIndex != -1 { - ownerRefs[ownerIndex] = ownerRef - } else { - ownerRefs = append(ownerRefs, ownerRef) +// SetCurrent updates obj to mark this RevisionMetadata as the current (controlling) revision. +// Returns an error if the object already has a different current revision. +func (m *AnnotationRevisionMetadata) SetCurrent(obj metav1.Object, opts ...bctypes.SetCurrentOption) error { + options := &bctypes.SetCurrentOptions{} + for _, opt := range opts { + opt(options) } - s.setOwnerReferences(obj, ownerRefs) - - return nil -} - -// SetControllerReference adds owner as OwnerReference to obj, with Controller set to true. -func (s *OwnerStrategyAnnotation) SetControllerReference(owner, obj metav1.Object) error { - ownerRefComp := s.ownerRefForCompare(owner) - ownerRefs := s.getOwnerReferences(obj) + ownerRefComp := annotationOwnerRefForCompare(m.owner, m.scheme) + ownerRefs := m.getOwnerReferences(obj) // Ensure that there is no controller already. - for _, ownerRef := range ownerRefs { - if !s.referSameObject(ownerRefComp, ownerRef) && - ownerRef.Controller != nil && *ownerRef.Controller { - return &controllerutil.AlreadyOwnedError{ - Object: obj, - Owner: metav1.OwnerReference{ - APIVersion: ownerRef.APIVersion, - Kind: ownerRef.Kind, - Name: ownerRef.Name, - Controller: ownerRef.Controller, - UID: ownerRef.UID, - }, + for i := range ownerRefs { + ownerRef := &ownerRefs[i] + if !referSameObject(&ownerRefComp, ownerRef) && ownerRef.Controller != nil && *ownerRef.Controller { + if !options.AllowUpdate { + return &controllerutil.AlreadyOwnedError{ + Object: obj, + Owner: metav1.OwnerReference{ + APIVersion: ownerRef.APIVersion, + Kind: ownerRef.Kind, + Name: ownerRef.Name, + Controller: ownerRef.Controller, + UID: ownerRef.UID, + }, + } } + + ownerRef.Controller = nil } } - gvk, err := apiutil.GVKForObject(owner.(runtime.Object), s.scheme) + gvk, err := apiutil.GVKForObject(m.owner.(runtime.Object), m.scheme) if err != nil { return err } ownerRef := annotationOwnerRef{ - APIVersion: gvk.GroupVersion().String(), - Kind: gvk.Kind, - UID: owner.GetUID(), - Name: owner.GetName(), - Namespace: owner.GetNamespace(), - Controller: ptr.To(true), - } - - ownerIndex := s.indexOf(ownerRefs, ownerRef) + OwnerReference: metav1.OwnerReference{ + APIVersion: gvk.GroupVersion().String(), + Kind: gvk.Kind, + UID: m.owner.GetUID(), + Name: m.owner.GetName(), + Controller: ptr.To(true), + }, + Namespace: m.owner.GetNamespace(), + } + + ownerIndex := slices.IndexFunc(ownerRefs, func(ref annotationOwnerRef) bool { + return referSameObject(&ownerRef, &ref) + }) if ownerIndex != -1 { ownerRefs[ownerIndex] = ownerRef } else { ownerRefs = append(ownerRefs, ownerRef) } - s.setOwnerReferences(obj, ownerRefs) + m.setOwnerReferences(obj, ownerRefs) return nil } -// IsOwner returns true if owner is contained in object OwnerReference list. -func (s *OwnerStrategyAnnotation) IsOwner(owner, obj metav1.Object) bool { - ownerRefComp := s.ownerRefForCompare(owner) - for _, ownerRef := range s.getOwnerReferences(obj) { - if s.referSameObject(ownerRefComp, ownerRef) { - return true - } - } - - return false -} - -// IsController returns true if the given owner is the controller of obj. -func (s *OwnerStrategyAnnotation) IsController( - owner, obj metav1.Object, -) bool { - ownerRefComp := s.ownerRefForCompare(owner) - for _, ownerRef := range s.getOwnerReferences(obj) { - if s.referSameObject(ownerRefComp, ownerRef) && +// IsCurrent returns true if this RevisionMetadata is the current (controlling) revision of obj. +func (m *AnnotationRevisionMetadata) IsCurrent(obj metav1.Object) bool { + ownerRefComp := annotationOwnerRefForCompare(m.owner, m.scheme) + for _, ownerRef := range m.getOwnerReferences(obj) { + if referSameObject(&ownerRefComp, &ownerRef) && ownerRef.Controller != nil && *ownerRef.Controller { return true @@ -180,15 +154,14 @@ func (s *OwnerStrategyAnnotation) IsController( return false } -// RemoveOwner removes the owner from objs OwnerReference list. -func (s *OwnerStrategyAnnotation) RemoveOwner(owner, obj metav1.Object) { - ownerRefComp := s.ownerRefForCompare(owner) - ownerRefs := s.getOwnerReferences(obj) +// RemoveFrom removes this RevisionMetadata from obj, whether it is the current revision or otherwise. +func (m *AnnotationRevisionMetadata) RemoveFrom(obj metav1.Object) { + ownerRefComp := annotationOwnerRefForCompare(m.owner, m.scheme) + ownerRefs := m.getOwnerReferences(obj) foundIndex := -1 for i, ownerRef := range ownerRefs { - if s.referSameObject(ownerRefComp, ownerRef) { - // remove owner + if referSameObject(&ownerRefComp, &ownerRef) { foundIndex = i break @@ -196,39 +169,67 @@ func (s *OwnerStrategyAnnotation) RemoveOwner(owner, obj metav1.Object) { } if foundIndex != -1 { - s.setOwnerReferences(obj, remove(ownerRefs, foundIndex)) + m.setOwnerReferences(obj, slices.Delete(ownerRefs, foundIndex, foundIndex+1)) } } -// ReleaseController sets all OwnerReferences Controller to false. -func (s *OwnerStrategyAnnotation) ReleaseController(obj metav1.Object) { - ownerRefs := s.getOwnerReferences(obj) +// IsNamespaceAllowed returns true if objects may be created/managed in the namespace of obj. +// For annotation-based ownership, cross-namespace is always allowed. +func (m *AnnotationRevisionMetadata) IsNamespaceAllowed(_ metav1.Object) bool { + return true +} + +// 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 *AnnotationRevisionMetadata) CopyReferences(objA, objB metav1.Object) { + // Copy owner references from A to B. + ownerRefs := m.getOwnerReferences(objA) + // Release controller (set all Controller fields to nil/false). for i := range ownerRefs { ownerRefs[i].Controller = nil } - s.setOwnerReferences(obj, ownerRefs) + m.setOwnerReferences(objB, ownerRefs) +} + +// 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 { + // The returned value is only used in log messages. We return the + // owner reference for continuity with the original implementation. + return &ref.OwnerReference + } + } + + return nil +} + +func (m *AnnotationRevisionMetadata) getOwnerReferences(obj metav1.Object) []annotationOwnerRef { + return getAnnotationOwnerReferences(obj, m.annotationKey) } -func (s *OwnerStrategyAnnotation) getOwnerReferences(obj metav1.Object) []annotationOwnerRef { +// getAnnotationOwnerReferences returns the owner references stored in the given annotation key. +func getAnnotationOwnerReferences(obj metav1.Object, annotationKey string) []annotationOwnerRef { annotations := obj.GetAnnotations() if annotations == nil { return nil } - if len(annotations[s.annotationKey]) == 0 { + if len(annotations[annotationKey]) == 0 { return nil } var ownerReferences []annotationOwnerRef - if err := json.Unmarshal([]byte(annotations[s.annotationKey]), &ownerReferences); err != nil { + if err := json.Unmarshal([]byte(annotations[annotationKey]), &ownerReferences); err != nil { panic(err) } return ownerReferences } -func (s *OwnerStrategyAnnotation) setOwnerReferences(obj metav1.Object, owners []annotationOwnerRef) { +func (m *AnnotationRevisionMetadata) setOwnerReferences(obj metav1.Object, owners []annotationOwnerRef) { annotations := obj.GetAnnotations() if annotations == nil { annotations = map[string]string{} @@ -239,45 +240,31 @@ func (s *OwnerStrategyAnnotation) setOwnerReferences(obj metav1.Object, owners [ panic(err) } - annotations[s.annotationKey] = string(j) + annotations[m.annotationKey] = string(j) obj.SetAnnotations(annotations) } -func (s *OwnerStrategyAnnotation) indexOf(ownerRefs []annotationOwnerRef, ownerRef annotationOwnerRef) int { - for i := range ownerRefs { - if s.referSameObject(ownerRef, ownerRefs[i]) { - return i - } - } - - return -1 -} - -func (s *OwnerStrategyAnnotation) ownerRefForCompare(owner metav1.Object) annotationOwnerRef { - // Validate the owner. - ro, ok := owner.(runtime.Object) - if !ok { - panic(fmt.Sprintf("%T is not a runtime.Object, cannot call SetOwnerReference", owner)) - } - +func annotationOwnerRefForCompare(obj client.Object, scheme *runtime.Scheme) annotationOwnerRef { // Create a new owner ref. - gvk, err := apiutil.GVKForObject(ro, s.scheme) + gvk, err := apiutil.GVKForObject(obj, scheme) if err != nil { panic(err) } ref := annotationOwnerRef{ - APIVersion: gvk.GroupVersion().String(), - Kind: gvk.Kind, - UID: owner.GetUID(), - Name: owner.GetName(), + OwnerReference: metav1.OwnerReference{ + APIVersion: gvk.GroupVersion().String(), + Kind: gvk.Kind, + UID: obj.GetUID(), + Name: obj.GetName(), + }, + Namespace: obj.GetNamespace(), } return ref } -// Returns true if a and b point to the same object. -func (s *OwnerStrategyAnnotation) referSameObject(a, b annotationOwnerRef) bool { +func referSameObject(a, b *annotationOwnerRef) bool { aGV, err := schema.ParseGroupVersion(a.APIVersion) if err != nil { return false @@ -291,57 +278,58 @@ func (s *OwnerStrategyAnnotation) referSameObject(a, b annotationOwnerRef) bool return aGV.Group == bGV.Group && a.Kind == b.Kind && a.Name == b.Name && a.UID == b.UID } +// annotationOwnerRef represents an owner reference stored in annotations. +// This is used for cross-namespace ownership tracking where native ownerReferences cannot be used. type annotationOwnerRef struct { - // API version of the referent. - APIVersion string `json:"apiVersion"` - // Kind of the referent. - // More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds - Kind string `json:"kind"` - // Name of the referent. - // More info: http://kubernetes.io/docs/user-guide/identifiers#names - Name string `json:"name"` - // Name of the referent. + metav1.OwnerReference + + // Namespace of the referent. // More info: http://kubernetes.io/docs/user-guide/identifiers#namespaces Namespace string `json:"namespace"` - // UID of the referent. - // More info: http://kubernetes.io/docs/user-guide/identifiers#uids - UID types.UID `json:"uid"` - // If true, this reference struct points to the managing controller. - // +optional - Controller *bool `json:"controller,omitempty"` } func (r *annotationOwnerRef) isController() bool { return r.Controller != nil && *r.Controller } -func (r *annotationOwnerRef) ToMetaV1OwnerRef() metav1.OwnerReference { - return metav1.OwnerReference{ - APIVersion: r.APIVersion, - Kind: r.Kind, - Name: r.Name, - UID: r.UID, - Controller: r.Controller, - BlockOwnerDeletion: ptr.To(true), +// EnqueueRequestForOwner returns an EventHandler that enqueues reconcile requests +// for the owner of the object that triggered the event, using annotation-based owner references. +func (h AnnotationStrategy) EnqueueRequestForOwner( + scheme *runtime.Scheme, + ownerType client.Object, + isController bool, +) handler.EventHandler { + e := &annotationEnqueueRequestForOwner{ + ownerType: ownerType, + isController: isController, + annotationKey: string(h), } + if err := e.parseOwnerTypeGroupKind(scheme); err != nil { + panic(err) + } + + return e } -// AnnotationEnqueueRequestForOwner implements an EventHandler using the annotation ownerStrategy. -type AnnotationEnqueueRequestForOwner struct { - // OwnerType is the type of the Owner object to look for in OwnerReferences. Only Group and Kind are compared. - OwnerType client.Object +// annotationEnqueueRequestForOwner implements an EventHandler using annotation-based owner references. +type annotationEnqueueRequestForOwner struct { + // ownerType is the type of the Owner object to look for in OwnerReferences. Only Group and Kind are compared. + ownerType client.Object - // IsController if set will only look at the first OwnerReference with Controller: true. - IsController bool + // isController if set will only look at the first OwnerReference with Controller: true. + isController bool - // OwnerType is the type of the Owner object to look for in OwnerReferences. Only Group and Kind are compared. - ownerGK schema.GroupKind + // annotationKey is the annotation key used to store owner references. + annotationKey string - ownerStrategy *OwnerStrategyAnnotation + // ownerGK is the cached Group and Kind for the OwnerType. + ownerGK schema.GroupKind } +var _ handler.EventHandler = (*annotationEnqueueRequestForOwner)(nil) + // Create implements EventHandler. -func (e *AnnotationEnqueueRequestForOwner) Create( +func (e *annotationEnqueueRequestForOwner) Create( _ context.Context, evt event.CreateEvent, q workqueue.TypedRateLimitingInterface[reconcile.Request], ) { for _, req := range e.getOwnerReconcileRequest(evt.Object) { @@ -350,7 +338,7 @@ func (e *AnnotationEnqueueRequestForOwner) Create( } // Update implements EventHandler. -func (e *AnnotationEnqueueRequestForOwner) Update( +func (e *annotationEnqueueRequestForOwner) Update( _ context.Context, evt event.UpdateEvent, q workqueue.TypedRateLimitingInterface[reconcile.Request], ) { for _, req := range e.getOwnerReconcileRequest(evt.ObjectOld) { @@ -363,7 +351,7 @@ func (e *AnnotationEnqueueRequestForOwner) Update( } // Delete implements EventHandler. -func (e *AnnotationEnqueueRequestForOwner) Delete( +func (e *annotationEnqueueRequestForOwner) Delete( _ context.Context, evt event.DeleteEvent, q workqueue.TypedRateLimitingInterface[reconcile.Request], ) { for _, req := range e.getOwnerReconcileRequest(evt.Object) { @@ -372,7 +360,7 @@ func (e *AnnotationEnqueueRequestForOwner) Delete( } // Generic implements EventHandler. -func (e *AnnotationEnqueueRequestForOwner) Generic( +func (e *annotationEnqueueRequestForOwner) Generic( _ context.Context, evt event.GenericEvent, q workqueue.TypedRateLimitingInterface[reconcile.Request], ) { for _, req := range e.getOwnerReconcileRequest(evt.Object) { @@ -380,8 +368,8 @@ func (e *AnnotationEnqueueRequestForOwner) Generic( } } -func (e *AnnotationEnqueueRequestForOwner) getOwnerReconcileRequest(object metav1.Object) []reconcile.Request { - ownerReferences := e.ownerStrategy.getOwnerReferences(object) +func (e *annotationEnqueueRequestForOwner) getOwnerReconcileRequest(object metav1.Object) []reconcile.Request { + ownerReferences := getAnnotationOwnerReferences(object, e.annotationKey) requests := make([]reconcile.Request, 0, len(ownerReferences)) for _, ownerRef := range ownerReferences { @@ -395,7 +383,7 @@ func (e *AnnotationEnqueueRequestForOwner) getOwnerReconcileRequest(object metav continue } - if e.IsController && !ownerRef.isController() { + if e.isController && !ownerRef.isController() { continue } @@ -414,15 +402,15 @@ func (e *AnnotationEnqueueRequestForOwner) getOwnerReconcileRequest(object metav var ErrMultipleKinds = errors.New("multiple kinds error: expected exactly one kind") // parseOwnerTypeGroupKind parses the OwnerType into a Group and Kind and caches the result. -func (e *AnnotationEnqueueRequestForOwner) parseOwnerTypeGroupKind(scheme *runtime.Scheme) error { +func (e *annotationEnqueueRequestForOwner) parseOwnerTypeGroupKind(scheme *runtime.Scheme) error { // Get the kinds of the type - kinds, _, err := scheme.ObjectKinds(e.OwnerType) + kinds, _, err := scheme.ObjectKinds(e.ownerType) if err != nil { return err } // Expect only 1 kind. If there is more than one kind this is probably an edge case such as ListOptions. if len(kinds) != 1 { - return fmt.Errorf("%w. For ownerType %T, found %s kinds", ErrMultipleKinds, e.OwnerType, kinds) + return fmt.Errorf("%w. For ownerType %T, found %s kinds", ErrMultipleKinds, e.ownerType, kinds) } // Cache the Group and Kind for the OwnerType e.ownerGK = schema.GroupKind{Group: kinds[0].Group, Kind: kinds[0].Kind} diff --git a/ownerhandling/annotation_test.go b/ownerhandling/annotation_test.go index 600876eb..7a688cfc 100644 --- a/ownerhandling/annotation_test.go +++ b/ownerhandling/annotation_test.go @@ -5,6 +5,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -15,11 +16,13 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/reconcile" + + bctypes "pkg.package-operator.run/boxcutter/machinery/types" ) const testAnnotationKey = "xyz/owner" -func TestOwnerStrategyAnnotation_RemoveOwner(t *testing.T) { +func TestAnnotationRevisionMetadata_RemoveFrom(t *testing.T) { t.Parallel() obj := &corev1.ConfigMap{ @@ -28,7 +31,7 @@ func TestOwnerStrategyAnnotation_RemoveOwner(t *testing.T) { Namespace: "test", UID: types.UID("1234"), Annotations: map[string]string{ - testAnnotationKey: `[{"uid":"123456", "kind":"ConfigMap", "name":"cm1"}]`, + testAnnotationKey: `[{"uid":"123456", "kind":"ConfigMap", "name":"cm1", "apiVersion":"v1"}]`, }, }, } @@ -40,49 +43,16 @@ func TestOwnerStrategyAnnotation_RemoveOwner(t *testing.T) { }, } - s := NewAnnotation(testScheme, testAnnotationKey) - s.RemoveOwner(owner, obj) + h := NewAnnotationStrategy(testAnnotationKey) + m := h.NewRevisionMetadata(owner, testScheme) + m.RemoveFrom(obj) assert.Equal(t, `[]`, obj.Annotations[testAnnotationKey]) } -func TestOwnerStrategyAnnotation_SetOwnerReference(t *testing.T) { - t.Parallel() - - s := NewAnnotation(testScheme, testAnnotationKey) - cm1 := &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: "cm1", - Namespace: "cmtestns", - UID: types.UID("1234"), - }, - } - obj := &corev1.Secret{} - - require.NoError(t, s.SetOwnerReference(cm1, obj)) - - ownerRefs := s.getOwnerReferences(obj) - if assert.Len(t, ownerRefs, 1) { - assert.Equal(t, cm1.Name, ownerRefs[0].Name) - assert.Equal(t, cm1.Namespace, ownerRefs[0].Namespace) - assert.Equal(t, "ConfigMap", ownerRefs[0].Kind) - assert.Nil(t, ownerRefs[0].Controller) - } - - cm2 := &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: "cm2", - Namespace: "cmtestns", - UID: types.UID("56789"), - }, - } - require.NoError(t, s.SetOwnerReference(cm2, obj)) -} - -func TestOwnerStrategyAnnotation_SetControllerReference(t *testing.T) { +func TestAnnotationRevisionMetadata_SetCurrent(t *testing.T) { t.Parallel() - s := NewAnnotation(testScheme, testAnnotationKey) cm1 := &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: "cm1", @@ -92,17 +62,16 @@ func TestOwnerStrategyAnnotation_SetControllerReference(t *testing.T) { } obj := &corev1.Secret{} - err := s.SetControllerReference(cm1, obj) + h := NewAnnotationStrategy(testAnnotationKey) + m := h.NewRevisionMetadata(cm1, testScheme) + err := m.SetCurrent(obj) require.NoError(t, err) - ownerRefs := s.getOwnerReferences(obj) - if assert.Len(t, ownerRefs, 1) { - assert.Equal(t, cm1.Name, ownerRefs[0].Name) - assert.Equal(t, cm1.Namespace, ownerRefs[0].Namespace) - assert.Equal(t, "ConfigMap", ownerRefs[0].Kind) - assert.True(t, *ownerRefs[0].Controller) - } + // Verify the annotation was set + assert.NotEmpty(t, obj.Annotations[testAnnotationKey]) + assert.True(t, m.IsCurrent(obj)) + // Setting a second controller should fail cm2 := &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: "cm2", @@ -110,151 +79,24 @@ func TestOwnerStrategyAnnotation_SetControllerReference(t *testing.T) { UID: types.UID("56789"), }, } - err = s.SetControllerReference(cm2, obj) - require.Error(t, err, controllerutil.AlreadyOwnedError{}) - s.ReleaseController(obj) + m2 := h.NewRevisionMetadata(cm2, testScheme) + err = m2.SetCurrent(obj) + require.Error(t, err) - err = s.SetControllerReference(cm2, obj) - require.NoError(t, err) - assert.True(t, s.IsOwner(cm1, obj)) - assert.True(t, s.IsOwner(cm2, obj)) -} + var alreadyOwnedErr *controllerutil.AlreadyOwnedError -func TestOwnerStrategyAnnotation_ReleaseController(t *testing.T) { - t.Parallel() + require.ErrorAs(t, err, &alreadyOwnedErr) - s := NewAnnotation(testScheme, testAnnotationKey) - owner := &corev1.ConfigMap{} - obj := &corev1.Secret{} + // Verify cm1 is still the controller + require.True(t, m.IsCurrent(obj)) + require.False(t, m2.IsCurrent(obj)) - err := s.SetControllerReference(owner, obj) + // We should be able to set a new controller using WithAllowUpdate + err = m2.SetCurrent(obj, bctypes.WithAllowUpdate) require.NoError(t, err) - - ownerRefs := s.getOwnerReferences(obj) - if assert.Len(t, ownerRefs, 1) { - assert.NotNil(t, ownerRefs[0].Controller) - } - - s.ReleaseController(obj) - ownerRefs = s.getOwnerReferences(obj) - - if assert.Len(t, ownerRefs, 1) { - assert.Nil(t, ownerRefs[0].Controller) - } -} - -func TestOwnerStrategyAnnotation_IndexOf(t *testing.T) { - t.Parallel() - - testCases := []struct { - name string - ownerRef annotationOwnerRef - ownerRefs []annotationOwnerRef - expectedIndex int - }{ - { - name: "owner references are not defined", - ownerRef: annotationOwnerRef{ - APIVersion: "test/v1", - Kind: "Testi", - Name: "cm1___3245", - }, - ownerRefs: []annotationOwnerRef{}, - expectedIndex: -1, - }, - { - name: "owner reference is not defined", - ownerRef: annotationOwnerRef{}, - ownerRefs: []annotationOwnerRef{ - { - APIVersion: "test/v1", - Kind: "Testi", - Name: "cm1___3245", - }, - }, - expectedIndex: -1, - }, - { - name: "owner reference and references are not defined", - ownerRef: annotationOwnerRef{}, - ownerRefs: []annotationOwnerRef{}, - expectedIndex: -1, - }, - { - name: "owner reference is not present in references", - ownerRef: annotationOwnerRef{ - APIVersion: "test/v1", - Kind: "Testi", - Name: "cm1___3245", - }, - ownerRefs: []annotationOwnerRef{ - { - APIVersion: "test/v1", - Kind: "Testi", - Name: "cm1___3265", - }, - { - APIVersion: "test/v1", - Kind: "Testi", - Name: "cm1___3456", - }, - }, - expectedIndex: -1, - }, - { - name: "owner reference is present in references", - ownerRef: annotationOwnerRef{ - APIVersion: "test/v1", - Kind: "Testi", - Name: "cm1___3245", - }, - ownerRefs: []annotationOwnerRef{ - { - APIVersion: "test/v1", - Kind: "Testi", - Name: "cm1___3265", - }, - { - APIVersion: "test/v1", - Kind: "Testi", - Name: "cm1___3456", - }, - { - APIVersion: "test/v1", - Kind: "Testi", - Name: "cm1___3245", - }, - }, - expectedIndex: 2, - }, - } - - for i := range testCases { - tc := testCases[i] - t.Run(tc.name, func(t *testing.T) { - t.Parallel() - - s := OwnerStrategyAnnotation{} - resultIndex := s.indexOf(tc.ownerRefs, tc.ownerRef) - assert.Equal(t, tc.expectedIndex, resultIndex) - }) - } -} - -func TestOwnerStrategyAnnotation_setOwnerReferences(t *testing.T) { - t.Parallel() - - ownerRef := newConfigMapAnnotationOwnerRef() - obj := &corev1.Secret{} - - s := NewAnnotation(testScheme, testAnnotationKey) - s.setOwnerReferences(obj, []annotationOwnerRef{ownerRef}) - gottenOwnerRefs := s.getOwnerReferences(obj) - - if assert.Len(t, gottenOwnerRefs, 1) { - assert.Equal(t, gottenOwnerRefs[0], ownerRef) - } + require.True(t, m2.IsCurrent(obj)) + require.False(t, m.IsCurrent(obj)) } func TestAnnotationEnqueueOwnerHandler_GetOwnerReconcileRequest(t *testing.T) { @@ -263,102 +105,117 @@ func TestAnnotationEnqueueOwnerHandler_GetOwnerReconcileRequest(t *testing.T) { tests := []struct { name string isOwnerController *bool - enqueue AnnotationEnqueueRequestForOwner + ownerType client.Object + isController bool requestExpected bool }{ { name: "owner is controller, enqueue is controller, types match", isOwnerController: ptr.To(true), - enqueue: AnnotationEnqueueRequestForOwner{ - OwnerType: &corev1.ConfigMap{}, - IsController: true, - }, - requestExpected: true, + ownerType: &corev1.ConfigMap{}, + isController: true, + requestExpected: true, }, { name: "owner is not controller, enqueue is controller, types match", isOwnerController: ptr.To(false), - enqueue: AnnotationEnqueueRequestForOwner{ - OwnerType: &corev1.ConfigMap{}, - IsController: true, - }, - requestExpected: false, + ownerType: &corev1.ConfigMap{}, + isController: true, + requestExpected: false, }, { name: "owner is controller, enqueue is not controller, types match", isOwnerController: ptr.To(true), - enqueue: AnnotationEnqueueRequestForOwner{ - OwnerType: &corev1.ConfigMap{}, - IsController: false, - }, - requestExpected: true, + ownerType: &corev1.ConfigMap{}, + isController: false, + requestExpected: true, }, { name: "owner is not controller, enqueue is not controller, types match", isOwnerController: ptr.To(false), - enqueue: AnnotationEnqueueRequestForOwner{ - OwnerType: &corev1.ConfigMap{}, - IsController: false, - }, - requestExpected: true, + ownerType: &corev1.ConfigMap{}, + isController: false, + requestExpected: true, }, { name: "owner controller is nil, enqueue is controller, types match", isOwnerController: nil, - enqueue: AnnotationEnqueueRequestForOwner{ - OwnerType: &corev1.ConfigMap{}, - IsController: true, - }, - requestExpected: false, + ownerType: &corev1.ConfigMap{}, + isController: true, + requestExpected: false, }, { name: "owner controller is nil, enqueue is not controller, types match", isOwnerController: nil, - enqueue: AnnotationEnqueueRequestForOwner{ - OwnerType: &corev1.ConfigMap{}, - IsController: false, - }, - requestExpected: true, + ownerType: &corev1.ConfigMap{}, + isController: false, + requestExpected: true, }, { name: "owner is controller, enqueue is controller, types do not match", isOwnerController: ptr.To(true), - enqueue: AnnotationEnqueueRequestForOwner{ - OwnerType: &appsv1.Deployment{}, - IsController: true, - }, - requestExpected: false, + ownerType: &appsv1.Deployment{}, + isController: true, + requestExpected: false, + }, + } + + owner := &corev1.ConfigMap{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "v1", + Kind: "ConfigMap", + }, + ObjectMeta: metav1.ObjectMeta{ + UID: types.UID("cm1___3245"), + Name: "cm1", + Namespace: "cm1namespace", + }, + } + + newOwner := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + UID: types.UID("cm2___3245"), + Name: "cm2", + Namespace: "cm2namespace", }, } + h := NewAnnotationStrategy(testAnnotationKey) + revisionMetadata := h.NewRevisionMetadata(owner, testScheme) + newRevisionMetadata := h.NewRevisionMetadata(newOwner, testScheme) + for i := range tests { test := tests[i] t.Run(test.name, func(t *testing.T) { t.Parallel() - ownerRef := newConfigMapAnnotationOwnerRef() - ownerRef.Controller = test.isOwnerController - s := NewAnnotation(testScheme, testAnnotationKey) - test.enqueue.ownerStrategy = s obj := &corev1.Secret{} - s.setOwnerReferences(obj, []annotationOwnerRef{ownerRef}) - - err := test.enqueue.parseOwnerTypeGroupKind(testScheme) + err := revisionMetadata.SetCurrent(obj) require.NoError(t, err) - r := test.enqueue.getOwnerReconcileRequest(obj) + if test.isOwnerController == nil || !*test.isOwnerController { + // New revision will take over ownership, so 'owner' is no longer the controller + err = newRevisionMetadata.SetCurrent(obj, bctypes.WithAllowUpdate) + require.NoError(t, err) + + newRevisionMetadata.RemoveFrom(obj) + } + + enqueue := h.EnqueueRequestForOwner(testScheme, test.ownerType, test.isController).(*annotationEnqueueRequestForOwner) + req := enqueue.getOwnerReconcileRequest(obj) + if test.requestExpected { assert.Equal(t, []reconcile.Request{ { NamespacedName: client.ObjectKey{ - Name: ownerRef.Name, - Namespace: ownerRef.Namespace, + Name: owner.Name, + Namespace: owner.Namespace, }, }, - }, r) + }, req) } else { - assert.Empty(t, r) + assert.Empty(t, req) } }) } @@ -367,9 +224,9 @@ func TestAnnotationEnqueueOwnerHandler_GetOwnerReconcileRequest(t *testing.T) { func TestAnnotationEnqueueOwnerHandler_ParseOwnerTypeGroupKind(t *testing.T) { t.Parallel() - h := &AnnotationEnqueueRequestForOwner{ - OwnerType: &appsv1.Deployment{}, - IsController: true, + h := &annotationEnqueueRequestForOwner{ + ownerType: &appsv1.Deployment{}, + isController: true, } scheme := runtime.NewScheme() @@ -384,23 +241,9 @@ func TestAnnotationEnqueueOwnerHandler_ParseOwnerTypeGroupKind(t *testing.T) { assert.Equal(t, expectedGK, h.ownerGK) } -func newConfigMapAnnotationOwnerRef() annotationOwnerRef { - ownerRef1 := annotationOwnerRef{ - APIVersion: "v1", - Kind: "ConfigMap", - UID: types.UID("cm1___3245"), - Name: "cm1", - Namespace: "cm1namespace", - Controller: ptr.To(false), - } - - return ownerRef1 -} - -func TestOwnerStrategyAnnotation_IsController(t *testing.T) { +func TestAnnotationRevisionMetadata_IsCurrent(t *testing.T) { t.Parallel() - s := NewAnnotation(testScheme, testAnnotationKey) obj := &corev1.Secret{} cm1 := &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ @@ -410,7 +253,9 @@ func TestOwnerStrategyAnnotation_IsController(t *testing.T) { }, } - err := s.SetControllerReference(cm1, obj) + h := NewAnnotationStrategy(testAnnotationKey) + m1 := h.NewRevisionMetadata(cm1, testScheme) + err := m1.SetCurrent(obj) require.NoError(t, err) cm2 := &corev1.ConfigMap{ @@ -420,206 +265,107 @@ func TestOwnerStrategyAnnotation_IsController(t *testing.T) { UID: types.UID("56789"), }, } + m2 := h.NewRevisionMetadata(cm2, testScheme) - assert.True(t, s.IsController(cm1, obj)) - assert.False(t, s.IsController(cm2, obj)) + assert.True(t, m1.IsCurrent(obj)) + assert.False(t, m2.IsCurrent(obj)) } -func TestIsOwner(t *testing.T) { - t.Parallel() - - testCases := []struct { - name string - owner *corev1.ConfigMap - obj *corev1.Secret - expectedOwner bool - }{ - { - name: "owner reference is not present", - owner: &corev1.ConfigMap{}, - obj: &corev1.Secret{}, - expectedOwner: false, - }, - { - name: "owner reference is present", - owner: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: "cm", - Namespace: "cmtestns", - }, - }, - obj: &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "cm", - Namespace: "cmtestns", - UID: types.UID("asdfjkl"), - Annotations: map[string]string{ - testAnnotationKey: `[{"kind":"ConfigMap", "apiVersion":"v1", "name":"cm","namespace":"cmtestns"}]`, - }, - }, - }, - expectedOwner: true, - }, - } - - for i := range testCases { - tc := testCases[i] - t.Run(tc.name, func(t *testing.T) { - t.Parallel() - - s := NewAnnotation(testScheme, testAnnotationKey) - resultOwner := s.IsOwner(tc.owner, tc.obj) - assert.Equal(t, tc.expectedOwner, resultOwner) - }) - } -} - -func TestIsController(t *testing.T) { +func TestAnnotationRevisionMetadata_GetCurrent(t *testing.T) { t.Parallel() testCases := []struct { name string - annOwnerRef annotationOwnerRef - expectedController bool - }{ - { - name: "annotation owner ref not defined", - annOwnerRef: annotationOwnerRef{}, - expectedController: false, - }, - { - name: "controller is null", - annOwnerRef: annotationOwnerRef{ - Controller: nil, - }, - expectedController: false, - }, - { - name: "controller is false", - annOwnerRef: annotationOwnerRef{ - Controller: ptr.To(false), - }, - expectedController: false, - }, - { - name: "conroller is defined and true", - annOwnerRef: annotationOwnerRef{ - Controller: ptr.To(true), - }, - expectedController: true, - }, - } - - for i := range testCases { - tc := testCases[i] - t.Run(tc.name, func(t *testing.T) { - t.Parallel() - - annOwnerRef := tc.annOwnerRef - resultController := annOwnerRef.isController() - assert.Equal(t, tc.expectedController, resultController) - }) - } -} - -func TestOwnerStrategyAnnotation_GetController(t *testing.T) { - t.Parallel() - - testCases := []struct { - name string - ownerRefs []annotationOwnerRef + annotation string expectedController metav1.OwnerReference expectedFound bool }{ { name: "no owner references", - ownerRefs: []annotationOwnerRef{}, + annotation: "", expectedController: metav1.OwnerReference{}, expectedFound: false, }, { - name: "no controller owner reference", - ownerRefs: []annotationOwnerRef{ - { - APIVersion: "v1", - Kind: "ConfigMap", - Name: "cm1", - Namespace: "test", - UID: types.UID("1234"), - Controller: nil, - }, - { - APIVersion: "v1", - Kind: "Secret", - Name: "secret1", - Namespace: "test", - UID: types.UID("5678"), - Controller: ptr.To(false), - }, - }, + name: "empty owner references array", + annotation: "[]", expectedController: metav1.OwnerReference{}, expectedFound: false, }, { - name: "has controller owner reference", - ownerRefs: []annotationOwnerRef{ - { - APIVersion: "v1", - Kind: "ConfigMap", - Name: "cm1", - Namespace: "test", - UID: types.UID("1234"), - Controller: ptr.To(false), - }, - { - APIVersion: "v1", - Kind: "Secret", - Name: "secret1", - Namespace: "test", - UID: types.UID("5678"), - Controller: ptr.To(true), - }, - }, + name: "no controller owner reference", + annotation: `[{"apiVersion":"v1","kind":"ConfigMap","name":"cm1","namespace":"test","uid":"1234"},{"apiVersion":"v1","kind":"Secret","name":"secret1","namespace":"test","uid":"5678","controller":false}]`, + expectedController: metav1.OwnerReference{}, + expectedFound: false, + }, + { + name: "has controller owner reference", + annotation: `[{"apiVersion":"v1","kind":"ConfigMap","name":"cm1","namespace":"test","uid":"1234","controller":false},{"apiVersion":"v1","kind":"Secret","name":"secret1","namespace":"test","uid":"5678","controller":true}]`, expectedController: metav1.OwnerReference{ - APIVersion: "v1", - Kind: "Secret", - Name: "secret1", - UID: types.UID("5678"), - Controller: ptr.To(true), - BlockOwnerDeletion: ptr.To(true), + APIVersion: "v1", + Kind: "Secret", + Name: "secret1", + UID: types.UID("5678"), + Controller: ptr.To(true), }, expectedFound: true, }, } + // Create a dummy owner for creating the metadata (required for constructor) + dummyOwner := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "dummy", + UID: types.UID("dummy-uid"), + }, + } + for i := range testCases { tc := testCases[i] t.Run(tc.name, func(t *testing.T) { t.Parallel() - s := NewAnnotation(testScheme, testAnnotationKey) - obj := &corev1.Secret{} - s.setOwnerReferences(obj, tc.ownerRefs) + h := NewAnnotationStrategy(testAnnotationKey) + m := h.NewRevisionMetadata(dummyOwner, testScheme) - controller, found := s.GetController(obj) - assert.Equal(t, tc.expectedFound, found) + obj := &corev1.Secret{} + if tc.annotation != "" { + obj.Annotations = map[string]string{testAnnotationKey: tc.annotation} + } + controller := m.GetCurrent(obj) if tc.expectedFound { - assert.Equal(t, tc.expectedController, controller) + require.NotNil(t, controller) + // Type assert to get the OwnerReference + ownerRef, ok := controller.(*metav1.OwnerReference) + require.True(t, ok) + assert.Equal(t, tc.expectedController, *ownerRef) + } else { + assert.Nil(t, controller) } }) } } -func TestOwnerStrategyAnnotation_CopyOwnerReferences(t *testing.T) { +func TestAnnotationRevisionMetadata_CopyReferences(t *testing.T) { t.Parallel() - s := NewAnnotation(testScheme, testAnnotationKey) + dummyOwner := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "dummy", + UID: types.UID("dummy-uid"), + }, + } + h := NewAnnotationStrategy(testAnnotationKey) + m := h.NewRevisionMetadata(dummyOwner, testScheme) objA := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: "secret-a", Namespace: "test", + Annotations: map[string]string{ + testAnnotationKey: `[{"apiVersion":"v1","kind":"Pod","name":"pod1","namespace":"test","uid":"1234","controller":true},{"apiVersion":"apps/v1","kind":"Deployment","name":"deploy1","namespace":"test","uid":"5678","controller":false}]`, + }, }, } objB := &corev1.ConfigMap{ @@ -629,104 +375,61 @@ func TestOwnerStrategyAnnotation_CopyOwnerReferences(t *testing.T) { }, } - ownerRefsA := []annotationOwnerRef{ - { - APIVersion: "v1", - Kind: "Pod", - Name: "pod1", - Namespace: "test", - UID: types.UID("1234"), - Controller: ptr.To(true), - }, - { - APIVersion: "apps/v1", - Kind: "Deployment", - Name: "deploy1", - Namespace: "test", - UID: types.UID("5678"), - Controller: ptr.To(false), - }, - } - - s.setOwnerReferences(objA, ownerRefsA) + m.CopyReferences(objA, objB) - s.CopyOwnerReferences(objA, objB) - - ownerRefsB := s.getOwnerReferences(objB) - assert.Equal(t, ownerRefsA, ownerRefsB) -} - -func TestOwnerStrategyAnnotation_ToMetaV1OwnerRef(t *testing.T) { - t.Parallel() - - testCases := []struct { - name string - ownerRef annotationOwnerRef - expectedOwner metav1.OwnerReference - }{ + // Verify objB has annotations set + expectedOwnerRefs := []annotationOwnerRef{ { - name: "basic owner reference", - ownerRef: annotationOwnerRef{ + OwnerReference: metav1.OwnerReference{ APIVersion: "v1", - Kind: "ConfigMap", - Name: "cm1", - Namespace: "test", + Kind: "Pod", + Name: "pod1", UID: types.UID("1234"), - Controller: nil, - }, - expectedOwner: metav1.OwnerReference{ - APIVersion: "v1", - Kind: "ConfigMap", - Name: "cm1", - UID: types.UID("1234"), - Controller: nil, - BlockOwnerDeletion: ptr.To(true), + // Controller is released (set to nil) }, + Namespace: "test", }, { - name: "controller owner reference", - ownerRef: annotationOwnerRef{ + OwnerReference: metav1.OwnerReference{ APIVersion: "apps/v1", Kind: "Deployment", Name: "deploy1", - Namespace: "test", UID: types.UID("5678"), - Controller: ptr.To(true), - }, - expectedOwner: metav1.OwnerReference{ - APIVersion: "apps/v1", - Kind: "Deployment", - Name: "deploy1", - UID: types.UID("5678"), - Controller: ptr.To(true), - BlockOwnerDeletion: ptr.To(true), + // Controller is released (set to nil) }, + Namespace: "test", }, } - for i := range testCases { - tc := testCases[i] - t.Run(tc.name, func(t *testing.T) { - t.Parallel() + require.NotEmpty(t, objB.Annotations[testAnnotationKey]) + ownerRefs := m.getOwnerReferences(objB) + require.ElementsMatch(t, expectedOwnerRefs, ownerRefs) - result := tc.ownerRef.ToMetaV1OwnerRef() - assert.Equal(t, tc.expectedOwner, result) - }) - } + // Verify that controller is released (set to nil) + // GetCurrent should return nil since all controllers are released + controller := m.GetCurrent(objB) + assert.Nil(t, controller, "CopyReferences should release all controllers") } -func TestOwnerStrategyAnnotation_GetOwnerReferencesEdgeCases(t *testing.T) { +func TestAnnotationRevisionMetadata_GetOwnerReferencesEdgeCases(t *testing.T) { t.Parallel() + dummyOwner := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "dummy", + UID: types.UID("dummy-uid"), + }, + } + testCases := []struct { - name string - obj metav1.Object - expectedLen int + name string + obj metav1.Object + expectedNilRef bool }{ { - name: "no annotations", - obj: &corev1.Secret{}, - expectedLen: 0, + name: "no annotations", + obj: &corev1.Secret{}, + expectedNilRef: true, }, { name: "empty annotation key", @@ -737,7 +440,7 @@ func TestOwnerStrategyAnnotation_GetOwnerReferencesEdgeCases(t *testing.T) { }, }, }, - expectedLen: 0, + expectedNilRef: true, }, { name: "empty owner references array", @@ -748,7 +451,7 @@ func TestOwnerStrategyAnnotation_GetOwnerReferencesEdgeCases(t *testing.T) { }, }, }, - expectedLen: 0, + expectedNilRef: true, }, } @@ -757,149 +460,71 @@ func TestOwnerStrategyAnnotation_GetOwnerReferencesEdgeCases(t *testing.T) { t.Run(tc.name, func(t *testing.T) { t.Parallel() - s := NewAnnotation(testScheme, testAnnotationKey) - ownerRefs := s.getOwnerReferences(tc.obj) - assert.Len(t, ownerRefs, tc.expectedLen) + h := NewAnnotationStrategy(testAnnotationKey) + m := h.NewRevisionMetadata(dummyOwner, testScheme) + + controller := m.GetCurrent(tc.obj) + if tc.expectedNilRef { + assert.Nil(t, controller) + } }) } } -func TestOwnerStrategyAnnotation_ReferSameObjectEdgeCases(t *testing.T) { +func TestAnnotationRevisionMetadata_ReferSameObjectEdgeCases(t *testing.T) { t.Parallel() + // Test via IsCurrent with various owner reference configurations. + // Note: The owner is always a ConfigMap, so its GVK is always v1/ConfigMap (core group). + // We test referSameObject by varying the annotation ref's GVK fields. testCases := []struct { - name string - a annotationOwnerRef - b annotationOwnerRef - expected bool + name string + ownerName string + ownerUID types.UID + refAnnotation string + expectedMatch bool }{ { - name: "same objects", - a: annotationOwnerRef{ - APIVersion: "v1", - Kind: "ConfigMap", - Name: "cm1", - UID: types.UID("1234"), - }, - b: annotationOwnerRef{ - APIVersion: "v1", - Kind: "ConfigMap", - Name: "cm1", - UID: types.UID("1234"), - }, - expected: true, - }, - { - name: "different groups same version", - a: annotationOwnerRef{ - APIVersion: "apps/v1", - Kind: "Deployment", - Name: "deploy1", - UID: types.UID("1234"), - }, - b: annotationOwnerRef{ - APIVersion: "v1", - Kind: "Deployment", - Name: "deploy1", - UID: types.UID("1234"), - }, - expected: false, + name: "same objects", + ownerName: "cm1", + ownerUID: types.UID("1234"), + refAnnotation: `[{"apiVersion":"v1","kind":"ConfigMap","name":"cm1","uid":"1234","controller":true}]`, + expectedMatch: true, }, { - name: "same group different versions", - a: annotationOwnerRef{ - APIVersion: "apps/v1", - Kind: "Deployment", - Name: "deploy1", - UID: types.UID("1234"), - }, - b: annotationOwnerRef{ - APIVersion: "apps/v1beta1", - Kind: "Deployment", - Name: "deploy1", - UID: types.UID("1234"), - }, - expected: true, - }, - { - name: "different kinds", - a: annotationOwnerRef{ - APIVersion: "v1", - Kind: "ConfigMap", - Name: "cm1", - UID: types.UID("1234"), - }, - b: annotationOwnerRef{ - APIVersion: "v1", - Kind: "Secret", - Name: "cm1", - UID: types.UID("1234"), - }, - expected: false, + name: "different groups - core vs apps", + ownerName: "cm1", + ownerUID: types.UID("1234"), + refAnnotation: `[{"apiVersion":"apps/v1","kind":"ConfigMap","name":"cm1","uid":"1234","controller":true}]`, + expectedMatch: false, // Owner is core group (v1), ref is apps group }, { - name: "different names", - a: annotationOwnerRef{ - APIVersion: "v1", - Kind: "ConfigMap", - Name: "cm1", - UID: types.UID("1234"), - }, - b: annotationOwnerRef{ - APIVersion: "v1", - Kind: "ConfigMap", - Name: "cm2", - UID: types.UID("1234"), - }, - expected: false, + name: "same group different versions - both core", + ownerName: "cm1", + ownerUID: types.UID("1234"), + refAnnotation: `[{"apiVersion":"v1beta1","kind":"ConfigMap","name":"cm1","uid":"1234","controller":true}]`, + expectedMatch: true, // Same group (core), different versions should match }, { - name: "different UIDs", - a: annotationOwnerRef{ - APIVersion: "v1", - Kind: "ConfigMap", - Name: "cm1", - UID: types.UID("1234"), - }, - b: annotationOwnerRef{ - APIVersion: "v1", - Kind: "ConfigMap", - Name: "cm1", - UID: types.UID("5678"), - }, - expected: false, + name: "different kinds", + ownerName: "cm1", + ownerUID: types.UID("1234"), + refAnnotation: `[{"apiVersion":"v1","kind":"Secret","name":"cm1","uid":"1234","controller":true}]`, + expectedMatch: false, }, { - name: "different groups - core vs invalid", - a: annotationOwnerRef{ - APIVersion: "invalid-group/v1", - Kind: "ConfigMap", - Name: "cm1", - UID: types.UID("1234"), - }, - b: annotationOwnerRef{ - APIVersion: "v1", - Kind: "ConfigMap", - Name: "cm1", - UID: types.UID("1234"), - }, - expected: false, + name: "different names", + ownerName: "cm1", + ownerUID: types.UID("1234"), + refAnnotation: `[{"apiVersion":"v1","kind":"ConfigMap","name":"cm2","uid":"1234","controller":true}]`, + expectedMatch: false, }, { - name: "different groups - core vs apps", - a: annotationOwnerRef{ - APIVersion: "v1", - Kind: "ConfigMap", - Name: "cm1", - UID: types.UID("1234"), - }, - b: annotationOwnerRef{ - APIVersion: "apps/v1", - Kind: "ConfigMap", - Name: "cm1", - UID: types.UID("1234"), - }, - expected: false, + name: "different UIDs", + ownerName: "cm1", + ownerUID: types.UID("1234"), + refAnnotation: `[{"apiVersion":"v1","kind":"ConfigMap","name":"cm1","uid":"5678","controller":true}]`, + expectedMatch: false, }, } @@ -908,17 +533,31 @@ func TestOwnerStrategyAnnotation_ReferSameObjectEdgeCases(t *testing.T) { t.Run(tc.name, func(t *testing.T) { t.Parallel() - s := NewAnnotation(testScheme, testAnnotationKey) - result := s.referSameObject(tc.a, tc.b) - assert.Equal(t, tc.expected, result) + owner := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: tc.ownerName, + UID: tc.ownerUID, + }, + } + obj := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + testAnnotationKey: tc.refAnnotation, + }, + }, + } + + h := NewAnnotationStrategy(testAnnotationKey) + m := h.NewRevisionMetadata(owner, testScheme) + result := m.IsCurrent(obj) + assert.Equal(t, tc.expectedMatch, result) }) } } -func TestOwnerStrategyAnnotation_RemoveOwnerNotFound(t *testing.T) { +func TestAnnotationRevisionMetadata_RemoveFromNotFound(t *testing.T) { t.Parallel() - s := NewAnnotation(testScheme, testAnnotationKey) obj := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ @@ -933,9 +572,137 @@ func TestOwnerStrategyAnnotation_RemoveOwnerNotFound(t *testing.T) { }, } - initialOwnerRefs := s.getOwnerReferences(obj) - s.RemoveOwner(owner, obj) - finalOwnerRefs := s.getOwnerReferences(obj) + h := NewAnnotationStrategy(testAnnotationKey) + m := h.NewRevisionMetadata(owner, testScheme) + initialAnnotation := obj.Annotations[testAnnotationKey] + m.RemoveFrom(obj) + finalAnnotation := obj.Annotations[testAnnotationKey] + + assert.Equal(t, initialAnnotation, finalAnnotation) +} + +// Tests for annotationOwnerRef struct methods + +func TestAnnotationOwnerRef_IsController(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + annOwnerRef annotationOwnerRef + expectedController bool + }{ + { + name: "annotation owner ref not defined", + annOwnerRef: annotationOwnerRef{}, + expectedController: false, + }, + { + name: "controller is null", + annOwnerRef: annotationOwnerRef{ + OwnerReference: metav1.OwnerReference{ + Controller: nil, + }, + }, + expectedController: false, + }, + { + name: "controller is false", + annOwnerRef: annotationOwnerRef{ + OwnerReference: metav1.OwnerReference{ + Controller: ptr.To(false), + }, + }, + expectedController: false, + }, + { + name: "controller is defined and true", + annOwnerRef: annotationOwnerRef{ + OwnerReference: metav1.OwnerReference{ + Controller: ptr.To(true), + }, + }, + expectedController: true, + }, + } + + for i := range testCases { + tc := testCases[i] + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + annOwnerRef := tc.annOwnerRef + resultController := annOwnerRef.isController() + assert.Equal(t, tc.expectedController, resultController) + }) + } +} + +func TestAnnotationRevisionMetadata_IsNamespaceAllowed(t *testing.T) { + t.Parallel() + + // For annotation-based ownership, cross-namespace is always allowed + testCases := []struct { + name string + ownerNamespace string + objNamespace string + }{ + { + name: "same namespace", + ownerNamespace: "test", + objNamespace: "test", + }, + { + name: "different namespace", + ownerNamespace: "test", + objNamespace: "other", + }, + { + name: "cluster-scoped owner", + ownerNamespace: "", + objNamespace: "any-namespace", + }, + } + + for i := range testCases { + tc := testCases[i] + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + owner := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "owner", + Namespace: tc.ownerNamespace, + UID: types.UID("owner-uid"), + }, + } + obj := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "obj", + Namespace: tc.objNamespace, + }, + } + + h := NewAnnotationStrategy(testAnnotationKey) + m := h.NewRevisionMetadata(owner, testScheme) + // Annotation-based ownership always allows cross-namespace + assert.True(t, m.IsNamespaceAllowed(obj)) + }) + } +} + +func TestAnnotationRevisionMetadata_PanicsOnEmptyUID(t *testing.T) { + t.Parallel() + + owner := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "owner-without-uid", + Namespace: "test", + // UID is empty - not persisted to cluster + }, + } - assert.Equal(t, initialOwnerRefs, finalOwnerRefs) + assert.Panics(t, func() { + h := NewAnnotationStrategy(testAnnotationKey) + _ = h.NewRevisionMetadata(owner, testScheme) + }) } diff --git a/ownerhandling/common.go b/ownerhandling/common.go deleted file mode 100644 index f81abe0c..00000000 --- a/ownerhandling/common.go +++ /dev/null @@ -1,38 +0,0 @@ -package ownerhandling - -import ( - "k8s.io/apimachinery/pkg/api/meta" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/handler" -) - -type ownerStrategy interface { - // SetOwnerReference adds owner as OwnerReference to obj, with Controller set to false. - SetOwnerReference(owner, obj metav1.Object) error - // SetControllerReference adds owner as OwnerReference to obj, with Controller set to true. - SetControllerReference(owner, obj metav1.Object) error - // GetController returns the OwnerReference with Controller==true, if one exist. - GetController(obj metav1.Object) (metav1.OwnerReference, bool) - // IsController returns true if the given owner is the controller of obj. - IsController(owner, obj metav1.Object) bool - // CopyOwnerReferences copies all OwnerReferences from objA to objB, - // overriding any existing OwnerReferences on objB. - CopyOwnerReferences(objA, objB metav1.Object) - // EnqueueRequestForOwner returns a EventHandler to enqueue the owner. - EnqueueRequestForOwner(ownerType client.Object, mapper meta.RESTMapper, isController bool) handler.EventHandler - // ReleaseController sets all OwnerReferences Controller to false. - ReleaseController(obj metav1.Object) - // RemoveOwner removes the owner from objs OwnerReference list. - RemoveOwner(owner, obj metav1.Object) - // IsOwner returns true if owner is contained in object OwnerReference list. - IsOwner(owner, obj metav1.Object) bool -} - -// Removes the given index from the slice. -// does not perform an out-of-bounds check. -func remove[T any](s []T, i int) []T { - s[i] = s[len(s)-1] - - return s[:len(s)-1] -} diff --git a/ownerhandling/common_test.go b/ownerhandling/common_test.go deleted file mode 100644 index 2764ccf2..00000000 --- a/ownerhandling/common_test.go +++ /dev/null @@ -1,74 +0,0 @@ -package ownerhandling - -import ( - "testing" - - "github.com/stretchr/testify/assert" -) - -func TestRemoveUtility(t *testing.T) { - t.Parallel() - - testCases := []struct { - name string - slice []string - index int - expected []string - }{ - { - name: "remove from middle", - slice: []string{"a", "b", "c", "d"}, - index: 1, - expected: []string{"a", "d", "c"}, - }, - { - name: "remove first element", - slice: []string{"a", "b", "c"}, - index: 0, - expected: []string{"c", "b"}, - }, - { - name: "remove last element", - slice: []string{"a", "b", "c"}, - index: 2, - expected: []string{"a", "b"}, - }, - { - name: "remove from single element slice", - slice: []string{"a"}, - index: 0, - expected: []string{}, - }, - { - name: "remove from two element slice - first", - slice: []string{"a", "b"}, - index: 0, - expected: []string{"b"}, - }, - { - name: "remove from two element slice - second", - slice: []string{"a", "b"}, - index: 1, - expected: []string{"a"}, - }, - } - - for i := range testCases { - tc := testCases[i] - t.Run(tc.name, func(t *testing.T) { - t.Parallel() - - result := remove(tc.slice, tc.index) - assert.Equal(t, tc.expected, result) - }) - } -} - -func TestRemoveUtilityWithInts(t *testing.T) { - t.Parallel() - - slice := []int{10, 20, 30, 40, 50} - result := remove(slice, 2) - expected := []int{10, 20, 50, 40} - assert.Equal(t, expected, result) -} diff --git a/ownerhandling/native.go b/ownerhandling/native.go index 6f619300..9adcf1ca 100644 --- a/ownerhandling/native.go +++ b/ownerhandling/native.go @@ -1,7 +1,7 @@ package ownerhandling import ( - "fmt" + "slices" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -12,60 +12,103 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client/apiutil" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/handler" + + "pkg.package-operator.run/boxcutter/machinery/types" ) -var _ ownerStrategy = (*OwnerStrategyNative)(nil) +// Ensure NativeRevisionMetadata implements RevisionMetadata. +var _ types.RevisionMetadata = (*NativeRevisionMetadata)(nil) -// OwnerStrategyNative handling strategy uses .metadata.ownerReferences. -type OwnerStrategyNative struct { +// 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{ +// 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, +) *NativeRevisionMetadata { + if len(owner.GetUID()) == 0 { + panic("owner must be persisted to cluster, empty UID") + } + + return &NativeRevisionMetadata{ + owner: owner, scheme: scheme, } } -// GetController returns the OwnerReference with Controller==true, if one exist. -func (s *OwnerStrategyNative) GetController(obj metav1.Object) ( - metav1.OwnerReference, bool, -) { - for _, ref := range obj.GetOwnerReferences() { - if ref.Controller != nil && *ref.Controller { - return ref, true - } +// GetReconcileOptions returns a set of options that will added to any +// revision reconciliation options. +// For native ownership, there are no default reconciliation options. +func (m *NativeRevisionMetadata) GetReconcileOptions() []types.RevisionReconcileOption { + return nil +} + +// GetTeardownOptions returns a set of options that will added to any +// revision teardown options. +func (m *NativeRevisionMetadata) GetTeardownOptions() []types.RevisionTeardownOption { + var opts []types.RevisionTeardownOption + + // The revision owner is currently being deleted with "cascade=orphan". This + // option was requested by the user at the time the object was deleted. It + // was added by kube-apiserver, and is handled by the kubernetes garbage + // collector. The garbage collector will remove owner references to this + // object from all dependents before removing this finalizer. + // + // We want to honour the user's intent to orphan the dependents of this + // object during teardown if we find ourselves racing with the garbage + // collector. If the GC has already removed the owner reference from an + // object, existing checks in object teardown will already ensure we ignore + // that object. To ensure correct handling of objects which have not yet + // been orphaned by the GC, we add the WithOrphan option to the teardown + // options. + if controllerutil.ContainsFinalizer(m.owner, "orphan") { + opts = append(opts, types.WithOrphan()) } - return metav1.OwnerReference{}, false + return opts } -// CopyOwnerReferences copies all OwnerReferences from objA to objB, -// overriding any existing OwnerReferences on objB. -func (s *OwnerStrategyNative) CopyOwnerReferences(objA, objB metav1.Object) { - objB.SetOwnerReferences(objA.GetOwnerReferences()) +// GetOwner returns the owner object used to create this metadata. +// This method is not part of the RevisionMetadata interface, but is currently +// used by the reference controller. +func (m *NativeRevisionMetadata) GetOwner() client.Object { + return m.owner } -// IsOwner returns true if owner is contained in object OwnerReference list. -func (s *OwnerStrategyNative) IsOwner(owner, obj metav1.Object) bool { - ownerRefComp := s.ownerRefForCompare(owner) - for _, ownerRef := range obj.GetOwnerReferences() { - if s.referSameObject(ownerRefComp, ownerRef) { - return true +// SetCurrent updates obj to mark this RevisionMetadata as the current (controlling) revision. +// Returns an error if the object already has a different current revision +// unless the WithAllowUpdate option is given. +func (m *NativeRevisionMetadata) SetCurrent(obj metav1.Object, opts ...types.SetCurrentOption) error { + options := &types.SetCurrentOptions{} + for _, opt := range opts { + opt(options) + } + + if options.AllowUpdate { + ownerRefs := obj.GetOwnerReferences() + for i := range ownerRefs { + if ownerRefs[i].Controller != nil && *ownerRefs[i].Controller { + ownerRefs[i].Controller = nil + } } + + obj.SetOwnerReferences(ownerRefs) } - return false + return controllerutil.SetControllerReference(m.owner, obj, m.scheme) } -// IsController returns true if the given owner is the controller of obj. -func (s *OwnerStrategyNative) IsController( - owner, obj metav1.Object, -) bool { - ownerRefComp := s.ownerRefForCompare(owner) +// IsCurrent returns true if this RevisionMetadata is the current (controlling) revision of obj. +func (m *NativeRevisionMetadata) IsCurrent(obj metav1.Object) bool { + ownerRefComp := nativeOwnerRefForCompare(m.owner, m.scheme) for _, ownerRef := range obj.GetOwnerReferences() { - if s.referSameObject(ownerRefComp, ownerRef) && + if m.referSameObject(ownerRefComp, ownerRef) && ownerRef.Controller != nil && *ownerRef.Controller { return true @@ -75,14 +118,14 @@ func (s *OwnerStrategyNative) IsController( return false } -// RemoveOwner removes the owner from objs OwnerReference list. -func (s *OwnerStrategyNative) RemoveOwner(owner, obj metav1.Object) { - ownerRefComp := s.ownerRefForCompare(owner) +// RemoveFrom removes this RevisionMetadata from obj, whether it is the current revision or otherwise. +func (m *NativeRevisionMetadata) RemoveFrom(obj metav1.Object) { + ownerRefComp := nativeOwnerRefForCompare(m.owner, m.scheme) ownerRefs := obj.GetOwnerReferences() foundIndex := -1 for i, ownerRef := range ownerRefs { - if s.referSameObject(ownerRefComp, ownerRef) { + if m.referSameObject(ownerRefComp, ownerRef) { foundIndex = i break @@ -90,50 +133,56 @@ func (s *OwnerStrategyNative) RemoveOwner(owner, obj metav1.Object) { } if foundIndex != -1 { - obj.SetOwnerReferences(remove(ownerRefs, foundIndex)) + obj.SetOwnerReferences(slices.Delete(ownerRefs, foundIndex, foundIndex+1)) } } -// ReleaseController sets all OwnerReferences Controller to false. -func (s *OwnerStrategyNative) ReleaseController(obj metav1.Object) { - ownerRefs := obj.GetOwnerReferences() - for i := range ownerRefs { - ownerRefs[i].Controller = ptr.To(false) +// IsNamespaceAllowed returns true if objects may be created/managed in the namespace of obj. +func (m *NativeRevisionMetadata) IsNamespaceAllowed(obj metav1.Object) bool { + ownerNs := m.owner.GetNamespace() + // If owner is cluster-scoped, all namespaces are allowed. + if len(ownerNs) == 0 { + return true } - obj.SetOwnerReferences(ownerRefs) + // For namespaced owners, object must be in the same namespace. + return obj.GetNamespace() == ownerNs } -// SetOwnerReference adds owner as OwnerReference to obj, with Controller set to false. -func (s *OwnerStrategyNative) SetOwnerReference(owner, obj metav1.Object) error { - return controllerutil.SetOwnerReference(owner, obj, s.scheme) -} +// 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(oldObj, newObj metav1.Object) { + // Copy owner references from A to B. + oldOwnerRefs := slices.Clone(oldObj.GetOwnerReferences()) + newObj.SetOwnerReferences(oldOwnerRefs) + + // Release controller on B (set all Controller fields to false). + newOwnerRefs := newObj.GetOwnerReferences() + for i := range newOwnerRefs { + newOwnerRefs[i].Controller = ptr.To(false) + } -// SetControllerReference adds owner as OwnerReference to obj, with Controller set to true. -func (s *OwnerStrategyNative) SetControllerReference(owner, obj metav1.Object) error { - return controllerutil.SetControllerReference(owner, obj, s.scheme) + newObj.SetOwnerReferences(newOwnerRefs) } -// EnqueueRequestForOwner returns a EventHandler to enqueue the owner. -func (s *OwnerStrategyNative) EnqueueRequestForOwner( - ownerType client.Object, mapper meta.RESTMapper, isController bool, -) handler.EventHandler { - if isController { - return handler.EnqueueRequestForOwner(s.scheme, mapper, ownerType, handler.OnlyControllerOwner()) +// GetCurrent returns a RevisionReference describing the current revision of obj. +// Returns nil if there is no current revision set. +func (m *NativeRevisionMetadata) GetCurrent(obj metav1.Object) types.RevisionReference { + for _, ref := range obj.GetOwnerReferences() { + if ref.Controller != nil && *ref.Controller { + // Return a copy to avoid mutation. + refCopy := ref + + return &refCopy + } } - return handler.EnqueueRequestForOwner(s.scheme, mapper, ownerType) + return nil } -func (s *OwnerStrategyNative) ownerRefForCompare(owner metav1.Object) metav1.OwnerReference { - // Validate the owner. - ro, ok := owner.(runtime.Object) - if !ok { - panic(fmt.Sprintf("%T is not a runtime.Object, cannot call SetOwnerReference", owner)) - } - +func nativeOwnerRefForCompare(obj client.Object, scheme *runtime.Scheme) metav1.OwnerReference { // Create a new owner ref. - gvk, err := apiutil.GVKForObject(ro, s.scheme) + gvk, err := apiutil.GVKForObject(obj, scheme) if err != nil { panic(err) } @@ -141,15 +190,14 @@ func (s *OwnerStrategyNative) ownerRefForCompare(owner metav1.Object) metav1.Own ref := metav1.OwnerReference{ APIVersion: gvk.GroupVersion().String(), Kind: gvk.Kind, - UID: owner.GetUID(), - Name: owner.GetName(), + UID: obj.GetUID(), + Name: obj.GetName(), } return ref } -// Returns true if a and b point to the same object. -func (s *OwnerStrategyNative) referSameObject(a, b metav1.OwnerReference) bool { +func (m *NativeRevisionMetadata) referSameObject(a, b metav1.OwnerReference) bool { aGV, err := schema.ParseGroupVersion(a.APIVersion) if err != nil { panic(err) @@ -162,3 +210,18 @@ func (s *OwnerStrategyNative) referSameObject(a, b metav1.OwnerReference) bool { return aGV.Group == bGV.Group && a.Kind == b.Kind && a.Name == b.Name && a.UID == b.UID } + +// NativeEnqueueRequestForOwner returns an EventHandler that enqueues reconcile requests +// for the owner of the object that triggered the event. +func NativeEnqueueRequestForOwner( + scheme *runtime.Scheme, + mapper meta.RESTMapper, + ownerType client.Object, + isController bool, +) handler.EventHandler { + if isController { + return handler.EnqueueRequestForOwner(scheme, mapper, ownerType, handler.OnlyControllerOwner()) + } + + return handler.EnqueueRequestForOwner(scheme, mapper, ownerType) +} diff --git a/ownerhandling/native_test.go b/ownerhandling/native_test.go index 0e08bb6c..eea76a3a 100644 --- a/ownerhandling/native_test.go +++ b/ownerhandling/native_test.go @@ -11,11 +11,13 @@ import ( "k8s.io/client-go/kubernetes/scheme" "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + + bctypes "pkg.package-operator.run/boxcutter/machinery/types" ) var testScheme = scheme.Scheme -func TestOwnerStrategyNative_RemoveOwner(t *testing.T) { +func TestNativeRevisionMetadata_RemoveFrom(t *testing.T) { t.Parallel() obj := &corev1.ConfigMap{ @@ -36,118 +38,88 @@ func TestOwnerStrategyNative_RemoveOwner(t *testing.T) { }, } - s := NewNative(testScheme) - s.RemoveOwner(owner, obj) + m := NewNativeRevisionMetadata(owner, testScheme) + m.RemoveFrom(obj) assert.Equal(t, []metav1.OwnerReference{}, obj.GetOwnerReferences()) } -func TestOwnerStrategyNative_SetOwnerReference(t *testing.T) { +func TestNativeRevisionMetadata_SetCurrent(t *testing.T) { t.Parallel() - s := NewNative(testScheme) - obj := &corev1.Secret{} cm1 := &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: "cm1", - Namespace: obj.Namespace, + Namespace: "test", UID: types.UID("1234"), }, } - - require.NoError(t, s.SetOwnerReference(cm1, obj)) - - ownerRefs := obj.GetOwnerReferences() - if assert.Len(t, ownerRefs, 1) { - assert.Equal(t, cm1.Name, ownerRefs[0].Name) - assert.Equal(t, "ConfigMap", ownerRefs[0].Kind) - assert.Nil(t, ownerRefs[0].Controller) - } - - cm2 := &corev1.ConfigMap{ + obj := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: "cm2", - Namespace: obj.Namespace, - UID: types.UID("56789"), + Namespace: "test", }, } - require.NoError(t, s.SetControllerReference(cm2, obj)) -} - -func TestOwnerStrategyNative_SetControllerReference(t *testing.T) { - t.Parallel() - - s := NewNative(testScheme) - obj := &corev1.Secret{} - cm1 := &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: "cm1", - Namespace: obj.Namespace, - UID: types.UID("1234"), - }, + m := NewNativeRevisionMetadata(cm1, testScheme) + err := m.SetCurrent(obj) + require.NoError(t, err) + require.True(t, m.IsCurrent(obj)) + + toOwnerRef := func(obj metav1.Object) metav1.OwnerReference { + return metav1.OwnerReference{ + APIVersion: "v1", + Kind: "ConfigMap", + Name: obj.GetName(), + UID: obj.GetUID(), + BlockOwnerDeletion: ptr.To(true), + Controller: ptr.To(true), + } } - err := s.SetControllerReference(cm1, obj) - require.NoError(t, err) + cm1Ref := toOwnerRef(cm1) ownerRefs := obj.GetOwnerReferences() - if assert.Len(t, ownerRefs, 1) { - assert.Equal(t, cm1.Name, ownerRefs[0].Name) - assert.Equal(t, "ConfigMap", ownerRefs[0].Kind) - assert.True(t, *ownerRefs[0].Controller) - } + require.ElementsMatch(t, ownerRefs, []metav1.OwnerReference{cm1Ref}) + // Setting a second controller should fail cm2 := &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: "cm2", - Namespace: obj.Namespace, + Namespace: "test", UID: types.UID("56789"), }, } - err = s.SetControllerReference(cm2, obj) - require.Error(t, err, controllerutil.AlreadyOwnedError{}) + cm2Ref := toOwnerRef(cm2) - s.ReleaseController(obj) + m2 := NewNativeRevisionMetadata(cm2, testScheme) + err = m2.SetCurrent(obj) + require.Error(t, err) - err = s.SetControllerReference(cm2, obj) - require.NoError(t, err) - assert.True(t, s.IsOwner(cm1, obj)) - assert.True(t, s.IsOwner(cm2, obj)) -} + var alreadyOwnedErr *controllerutil.AlreadyOwnedError -func TestOwnerStrategyNative_IsController(t *testing.T) { - t.Parallel() + require.ErrorAs(t, err, &alreadyOwnedErr) + require.True(t, m.IsCurrent(obj)) + require.False(t, m2.IsCurrent(obj)) - s := NewNative(testScheme) - obj := &corev1.Secret{} - cm1 := &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: "cm1", - Namespace: obj.Namespace, - UID: types.UID("1234"), - }, - } - err := s.SetControllerReference(cm1, obj) + // We should be able to set a new controller using WithAllowUpdate + err = m2.SetCurrent(obj, bctypes.WithAllowUpdate) require.NoError(t, err) + require.True(t, m2.IsCurrent(obj)) + require.False(t, m.IsCurrent(obj)) - cm2 := &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: "cm2", - Namespace: obj.Namespace, - UID: types.UID("56789"), - }, - } - - assert.True(t, s.IsController(cm1, obj)) - assert.False(t, s.IsController(cm2, obj)) + ownerRefs = obj.GetOwnerReferences() + cm1Ref.Controller = nil + require.ElementsMatch(t, ownerRefs, []metav1.OwnerReference{cm1Ref, cm2Ref}) } -func TestOwnerStrategyNative_IsOwner(t *testing.T) { +func TestNativeRevisionMetadata_IsCurrent(t *testing.T) { t.Parallel() - s := NewNative(testScheme) - obj := &corev1.Secret{} + obj := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + }, + } cm1 := &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: "cm1", @@ -156,7 +128,8 @@ func TestOwnerStrategyNative_IsOwner(t *testing.T) { }, } - err := s.SetControllerReference(cm1, obj) + m1 := NewNativeRevisionMetadata(cm1, testScheme) + err := m1.SetCurrent(obj) require.NoError(t, err) cm2 := &corev1.ConfigMap{ @@ -166,36 +139,13 @@ func TestOwnerStrategyNative_IsOwner(t *testing.T) { UID: types.UID("56789"), }, } + m2 := NewNativeRevisionMetadata(cm2, testScheme) - assert.True(t, s.IsOwner(cm1, obj)) - assert.False(t, s.IsOwner(cm2, obj)) + assert.True(t, m1.IsCurrent(obj)) + assert.False(t, m2.IsCurrent(obj)) } -func TestOwnerStrategyNative_ReleaseController(t *testing.T) { - t.Parallel() - - s := NewNative(testScheme) - obj := &corev1.Secret{} - owner := &corev1.ConfigMap{} - owner.Namespace = obj.Namespace - - err := s.SetControllerReference(owner, obj) - require.NoError(t, err) - - ownerRefs := obj.GetOwnerReferences() - if assert.Len(t, ownerRefs, 1) { - assert.NotNil(t, ownerRefs[0].Controller) - } - - s.ReleaseController(obj) - ownerRefs = obj.GetOwnerReferences() - - if assert.Len(t, ownerRefs, 1) && assert.NotNil(t, ownerRefs[0].Controller) { - assert.False(t, *ownerRefs[0].Controller) - } -} - -func TestOwnerStrategyNative_GetController(t *testing.T) { +func TestNativeRevisionMetadata_GetCurrent(t *testing.T) { t.Parallel() testCases := []struct { @@ -260,29 +210,47 @@ func TestOwnerStrategyNative_GetController(t *testing.T) { }, } + // Create a dummy owner for creating the metadata (required for constructor) + dummyOwner := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "dummy", + UID: types.UID("dummy-uid"), + }, + } + for i := range testCases { tc := testCases[i] t.Run(tc.name, func(t *testing.T) { t.Parallel() - s := NewNative(testScheme) + m := NewNativeRevisionMetadata(dummyOwner, testScheme) obj := &corev1.Secret{} obj.SetOwnerReferences(tc.ownerRefs) - controller, found := s.GetController(obj) - assert.Equal(t, tc.expectedFound, found) - + controller := m.GetCurrent(obj) if tc.expectedFound { - assert.Equal(t, tc.expectedController, controller) + require.NotNil(t, controller) + // Type assert to get the OwnerReference + ownerRef, ok := controller.(*metav1.OwnerReference) + require.True(t, ok) + assert.Equal(t, tc.expectedController, *ownerRef) + } else { + assert.Nil(t, controller) } }) } } -func TestOwnerStrategyNative_CopyOwnerReferences(t *testing.T) { +func TestNativeRevisionMetadata_CopyReferences(t *testing.T) { t.Parallel() - s := NewNative(testScheme) + dummyOwner := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "dummy", + UID: types.UID("dummy-uid"), + }, + } + m := NewNativeRevisionMetadata(dummyOwner, testScheme) objA := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ @@ -313,118 +281,104 @@ func TestOwnerStrategyNative_CopyOwnerReferences(t *testing.T) { }, } - expectedOwnerRefs := objA.GetOwnerReferences() - - s.CopyOwnerReferences(objA, objB) + m.CopyReferences(objA, objB) actualOwnerRefs := objB.GetOwnerReferences() + // CopyReferences should copy all refs and set Controller to false + expectedOwnerRefs := []metav1.OwnerReference{ + { + APIVersion: "v1", + Kind: "Pod", + Name: "pod1", + UID: types.UID("1234"), + Controller: ptr.To(false), // Changed from true to false + }, + { + APIVersion: "apps/v1", + Kind: "Deployment", + Name: "deploy1", + UID: types.UID("5678"), + Controller: ptr.To(false), + }, + } assert.Equal(t, expectedOwnerRefs, actualOwnerRefs) } -func TestOwnerStrategyNative_ReferSameObjectEdgeCases(t *testing.T) { +func TestNativeRevisionMetadata_ReferSameObjectEdgeCases(t *testing.T) { t.Parallel() + // Test via IsCurrent with various owner reference configurations. + // Note: The owner is always a ConfigMap, so its GVK is always v1/ConfigMap (core group). + // We test referSameObject by varying the ref's GVK fields. testCases := []struct { - name string - a metav1.OwnerReference - b metav1.OwnerReference - expected bool + name string + ownerName string + ownerUID types.UID + refAPIVer string + refKind string + refName string + refUID types.UID + expectedMatch bool }{ { - name: "same objects", - a: metav1.OwnerReference{ - APIVersion: "v1", - Kind: "ConfigMap", - Name: "cm1", - UID: types.UID("1234"), - }, - b: metav1.OwnerReference{ - APIVersion: "v1", - Kind: "ConfigMap", - Name: "cm1", - UID: types.UID("1234"), - }, - expected: true, + name: "same objects", + ownerName: "cm1", + ownerUID: types.UID("1234"), + refAPIVer: "v1", + refKind: "ConfigMap", + refName: "cm1", + refUID: types.UID("1234"), + expectedMatch: true, }, { - name: "different groups same version", - a: metav1.OwnerReference{ - APIVersion: "apps/v1", - Kind: "Deployment", - Name: "deploy1", - UID: types.UID("1234"), - }, - b: metav1.OwnerReference{ - APIVersion: "v1", - Kind: "Deployment", - Name: "deploy1", - UID: types.UID("1234"), - }, - expected: false, + name: "different groups - core vs apps", + ownerName: "cm1", + ownerUID: types.UID("1234"), + refAPIVer: "apps/v1", + refKind: "ConfigMap", + refName: "cm1", + refUID: types.UID("1234"), + expectedMatch: false, // core group vs apps group }, { - name: "same group different versions", - a: metav1.OwnerReference{ - APIVersion: "apps/v1", - Kind: "Deployment", - Name: "deploy1", - UID: types.UID("1234"), - }, - b: metav1.OwnerReference{ - APIVersion: "apps/v1beta1", - Kind: "Deployment", - Name: "deploy1", - UID: types.UID("1234"), - }, - expected: true, + name: "same group different versions - both core", + ownerName: "cm1", + ownerUID: types.UID("1234"), + refAPIVer: "v1beta1", // core group with different version + refKind: "ConfigMap", + refName: "cm1", + refUID: types.UID("1234"), + expectedMatch: true, // Same group (core), different versions should match }, { - name: "different kinds", - a: metav1.OwnerReference{ - APIVersion: "v1", - Kind: "ConfigMap", - Name: "cm1", - UID: types.UID("1234"), - }, - b: metav1.OwnerReference{ - APIVersion: "v1", - Kind: "Secret", - Name: "cm1", - UID: types.UID("1234"), - }, - expected: false, + name: "different kinds", + ownerName: "cm1", + ownerUID: types.UID("1234"), + refAPIVer: "v1", + refKind: "Secret", + refName: "cm1", + refUID: types.UID("1234"), + expectedMatch: false, }, { - name: "different names", - a: metav1.OwnerReference{ - APIVersion: "v1", - Kind: "ConfigMap", - Name: "cm1", - UID: types.UID("1234"), - }, - b: metav1.OwnerReference{ - APIVersion: "v1", - Kind: "ConfigMap", - Name: "cm2", - UID: types.UID("1234"), - }, - expected: false, + name: "different names", + ownerName: "cm1", + ownerUID: types.UID("1234"), + refAPIVer: "v1", + refKind: "ConfigMap", + refName: "cm2", + refUID: types.UID("1234"), + expectedMatch: false, }, { - name: "different UIDs", - a: metav1.OwnerReference{ - APIVersion: "v1", - Kind: "ConfigMap", - Name: "cm1", - UID: types.UID("1234"), - }, - b: metav1.OwnerReference{ - APIVersion: "v1", - Kind: "ConfigMap", - Name: "cm1", - UID: types.UID("5678"), - }, - expected: false, + name: "different UIDs", + ownerName: "cm1", + ownerUID: types.UID("1234"), + refAPIVer: "v1", + refKind: "ConfigMap", + refName: "cm1", + refUID: types.UID("5678"), + expectedMatch: false, }, } @@ -433,17 +387,36 @@ func TestOwnerStrategyNative_ReferSameObjectEdgeCases(t *testing.T) { t.Run(tc.name, func(t *testing.T) { t.Parallel() - s := NewNative(testScheme) - result := s.referSameObject(tc.a, tc.b) - assert.Equal(t, tc.expected, result) + owner := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: tc.ownerName, + UID: tc.ownerUID, + }, + } + obj := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: tc.refAPIVer, + Kind: tc.refKind, + Name: tc.refName, + UID: tc.refUID, + Controller: ptr.To(true), + }, + }, + }, + } + + m := NewNativeRevisionMetadata(owner, testScheme) + result := m.IsCurrent(obj) + assert.Equal(t, tc.expectedMatch, result) }) } } -func TestOwnerStrategyNative_RemoveOwnerNotFound(t *testing.T) { +func TestNativeRevisionMetadata_RemoveFromNotFound(t *testing.T) { t.Parallel() - s := NewNative(testScheme) obj := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ OwnerReferences: []metav1.OwnerReference{ @@ -459,8 +432,95 @@ func TestOwnerStrategyNative_RemoveOwnerNotFound(t *testing.T) { } initialOwnerRefs := obj.GetOwnerReferences() - s.RemoveOwner(owner, obj) + m := NewNativeRevisionMetadata(owner, testScheme) + m.RemoveFrom(obj) finalOwnerRefs := obj.GetOwnerReferences() assert.Equal(t, initialOwnerRefs, finalOwnerRefs) } + +func TestNativeRevisionMetadata_IsNamespaceAllowed(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + ownerNamespace string + objNamespace string + expected bool + }{ + { + name: "same namespace", + ownerNamespace: "test", + objNamespace: "test", + expected: true, + }, + { + name: "different namespace", + ownerNamespace: "test", + objNamespace: "other", + expected: false, + }, + { + name: "cluster-scoped owner", + ownerNamespace: "", // cluster-scoped + objNamespace: "any-namespace", + expected: true, // cluster-scoped owners allow any namespace + }, + } + + for i := range testCases { + tc := testCases[i] + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + owner := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "owner", + Namespace: tc.ownerNamespace, + UID: types.UID("owner-uid"), + }, + } + obj := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "obj", + Namespace: tc.objNamespace, + }, + } + + m := NewNativeRevisionMetadata(owner, testScheme) + result := m.IsNamespaceAllowed(obj) + assert.Equal(t, tc.expected, result) + }) + } +} + +func TestNativeRevisionMetadata_GetOwner(t *testing.T) { + t.Parallel() + + owner := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-owner", + Namespace: "test", + UID: types.UID("owner-uid"), + }, + } + + m := NewNativeRevisionMetadata(owner, testScheme) + assert.Equal(t, owner, m.GetOwner()) +} + +func TestNativeRevisionMetadata_PanicsOnEmptyUID(t *testing.T) { + t.Parallel() + + owner := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "owner-without-uid", + Namespace: "test", + // UID is empty - not persisted to cluster + }, + } + + assert.Panics(t, func() { + _ = NewNativeRevisionMetadata(owner, testScheme) + }) +} diff --git a/test/comparator_test.go b/test/comparator_test.go index e03974ed..fccffa72 100644 --- a/test/comparator_test.go +++ b/test/comparator_test.go @@ -34,9 +34,8 @@ func sanitizeCompareResult(r *machinery.CompareResult) { //nolint:maintidx func TestComparator(t *testing.T) { - os := ownerhandling.NewNative(Scheme) comp := machinery.NewComparator( - os, DiscoveryClient, Scheme, fieldOwner) + DiscoveryClient, Scheme, fieldOwner) ctx := t.Context() owner := &corev1.ConfigMap{ @@ -53,6 +52,8 @@ func TestComparator(t *testing.T) { } }) + ownerMetadata := ownerhandling.NewNativeRevisionMetadata(owner, Scheme) + // ConfigMap unstructured actualConfigMap := &unstructured.Unstructured{ Object: map[string]interface{}{ @@ -391,7 +392,7 @@ Comparison: require.NoError(t, err) } - r, err := comp.Compare(owner, test.desiredObj, test.actualObj) + r, err := comp.Compare(ownerMetadata, test.desiredObj, test.actualObj) require.NoError(t, err) sanitizeCompareResult(&r) diff --git a/test/objects_test.go b/test/objects_test.go index c6f28d1a..eff65da7 100644 --- a/test/objects_test.go +++ b/test/objects_test.go @@ -19,10 +19,9 @@ import ( ) func TestObjectEngine(t *testing.T) { - os := ownerhandling.NewNative(Scheme) - comp := machinery.NewComparator(os, DiscoveryClient, Scheme, fieldOwner) + comp := machinery.NewComparator(DiscoveryClient, Scheme, fieldOwner) oe := machinery.NewObjectEngine( - Scheme, Client, Client, os, comp, fieldOwner, systemPrefix, + Scheme, Client, Client, comp, fieldOwner, systemPrefix, ) ctx := t.Context() @@ -44,6 +43,8 @@ func TestObjectEngine(t *testing.T) { } }) + ownerMetadata := ownerhandling.NewNativeRevisionMetadata(owner, Scheme) + configMap := &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "v1", @@ -60,7 +61,7 @@ func TestObjectEngine(t *testing.T) { } // Creation - res, err := oe.Reconcile(ctx, owner, 1, configMap) + res, err := oe.Reconcile(ctx, ownerMetadata, 1, configMap) require.NoError(t, err) assert.Equal(t, `Object ConfigMap.v1 default/oe-test Action: "Created" @@ -70,7 +71,7 @@ Action: "Created" assert.False(t, res.IsPaused(), "IsPaused") // Idle - res, err = oe.Reconcile(ctx, owner, 1, configMap) + res, err = oe.Reconcile(ctx, ownerMetadata, 1, configMap) require.NoError(t, err) assert.Equal(t, `Object ConfigMap.v1 default/oe-test Action: "Idle" @@ -89,7 +90,7 @@ Action: "Idle" require.NoError(t, err) // Idle with other participant. - res, err = oe.Reconcile(ctx, owner, 1, configMap) + res, err = oe.Reconcile(ctx, ownerMetadata, 1, configMap) require.NoError(t, err) assert.Equal(t, `Object ConfigMap.v1 default/oe-test Action: "Idle" @@ -114,7 +115,7 @@ Comparison: }, "data") require.NoError(t, err) - res, err = oe.Reconcile(ctx, owner, 1, configMap) + res, err = oe.Reconcile(ctx, ownerMetadata, 1, configMap) require.NoError(t, err) assert.Equal(t, `Object ConfigMap.v1 default/oe-test Action: "Updated" @@ -135,20 +136,19 @@ Comparison: assert.False(t, res.IsPaused(), "IsPaused") // Teardown is a two step process at the moment. - gone, err := oe.Teardown(ctx, owner, 1, configMap) + gone, err := oe.Teardown(ctx, ownerMetadata, 1, configMap) require.NoError(t, err) assert.False(t, gone) - gone, err = oe.Teardown(ctx, owner, 1, configMap) + gone, err = oe.Teardown(ctx, ownerMetadata, 1, configMap) require.NoError(t, err) assert.True(t, gone) } func TestObjectEnginePaused(t *testing.T) { - os := ownerhandling.NewNative(Scheme) - comp := machinery.NewComparator(os, DiscoveryClient, Scheme, fieldOwner) + comp := machinery.NewComparator(DiscoveryClient, Scheme, fieldOwner) oe := machinery.NewObjectEngine( - Scheme, Client, Client, os, comp, fieldOwner, systemPrefix, + Scheme, Client, Client, comp, fieldOwner, systemPrefix, ) ctx := t.Context() @@ -170,6 +170,8 @@ func TestObjectEnginePaused(t *testing.T) { } }) + ownerMetadata := ownerhandling.NewNativeRevisionMetadata(owner, Scheme) + configMap := &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "v1", @@ -187,7 +189,7 @@ func TestObjectEnginePaused(t *testing.T) { originalConfigMap := configMap.DeepCopy() // Creation Paused - res, err := oe.Reconcile(ctx, owner, 1, configMap, types.WithPaused{}) + res, err := oe.Reconcile(ctx, ownerMetadata, 1, configMap, types.WithPaused{}) require.NoError(t, err) assert.Equal(t, `Object ConfigMap.v1 default/oe-test-paused Action (PAUSED): "Created" @@ -201,7 +203,7 @@ Action (PAUSED): "Created" require.True(t, apierrors.IsNotFound(err), "Object should not exist after paused create action") // Creation Not Paused - res, err = oe.Reconcile(ctx, owner, 1, configMap) + res, err = oe.Reconcile(ctx, ownerMetadata, 1, configMap) require.NoError(t, err) assert.Equal(t, `Object ConfigMap.v1 default/oe-test-paused Action: "Created" @@ -210,7 +212,7 @@ Action: "Created" assert.False(t, res.IsPaused(), "IsPaused") // Idle Paused - res, err = oe.Reconcile(ctx, owner, 1, configMap, types.WithPaused{}) + res, err = oe.Reconcile(ctx, ownerMetadata, 1, configMap, types.WithPaused{}) require.NoError(t, err) assert.Equal(t, `Object ConfigMap.v1 default/oe-test-paused Action (PAUSED): "Idle" @@ -229,7 +231,7 @@ Action (PAUSED): "Idle" }, "data") require.NoError(t, err) - res, err = oe.Reconcile(ctx, owner, 1, configMap, types.WithPaused{}) + res, err = oe.Reconcile(ctx, ownerMetadata, 1, configMap, types.WithPaused{}) require.NoError(t, err) assert.Equal(t, `Object ConfigMap.v1 default/oe-test-paused Action (PAUSED): "Updated" @@ -256,20 +258,19 @@ Comparison: assert.Equal(t, originalConfigMap.GetAnnotations()["my-annotation"], cmNotUpdated.GetAnnotations()["my-annotation"]) // Teardown is a two step process at the moment. - gone, err := oe.Teardown(ctx, owner, 1, configMap) + gone, err := oe.Teardown(ctx, ownerMetadata, 1, configMap) require.NoError(t, err) assert.False(t, gone) - gone, err = oe.Teardown(ctx, owner, 1, configMap) + gone, err = oe.Teardown(ctx, ownerMetadata, 1, configMap) require.NoError(t, err) assert.True(t, gone) } func TestObjectEngineProbing(t *testing.T) { - os := ownerhandling.NewNative(Scheme) - comp := machinery.NewComparator(os, DiscoveryClient, Scheme, fieldOwner) + comp := machinery.NewComparator(DiscoveryClient, Scheme, fieldOwner) oe := machinery.NewObjectEngine( - Scheme, Client, Client, os, comp, fieldOwner, systemPrefix, + Scheme, Client, Client, comp, fieldOwner, systemPrefix, ) ctx := t.Context() @@ -291,6 +292,8 @@ func TestObjectEngineProbing(t *testing.T) { } }) + ownerMetadata := ownerhandling.NewNativeRevisionMetadata(owner, Scheme) + probeSuccess := &stubProbe{status: types.ProbeStatusTrue, messages: []string{"works!"}} probeFailed := &stubProbe{status: types.ProbeStatusFalse, messages: []string{"does not work!"}} probeUnknown := &stubProbe{status: types.ProbeStatusUnknown, messages: []string{"no clue!"}} @@ -311,7 +314,7 @@ func TestObjectEngineProbing(t *testing.T) { } // Creation progress probe fails res, err := oe.Reconcile( - ctx, owner, 1, configMap, + ctx, ownerMetadata, 1, configMap, types.WithProbe(types.ProgressProbeType, probeFailed), types.WithProbe("other", probeSuccess), ) @@ -329,7 +332,7 @@ Probes: // Idle progress probe unknown res, err = oe.Reconcile( - ctx, owner, 1, configMap, + ctx, ownerMetadata, 1, configMap, types.WithProbe(types.ProgressProbeType, probeUnknown), types.WithProbe("other", probeSuccess), ) @@ -347,7 +350,7 @@ Probes: // Idle progress probe success res, err = oe.Reconcile( - ctx, owner, 1, configMap, + ctx, ownerMetadata, 1, configMap, types.WithProbe(types.ProgressProbeType, probeSuccess), types.WithProbe("other", probeSuccess), ) @@ -364,11 +367,11 @@ Probes: assert.False(t, res.IsPaused(), "IsPaused") // Teardown is a two step process at the moment. - gone, err := oe.Teardown(ctx, owner, 1, configMap) + gone, err := oe.Teardown(ctx, ownerMetadata, 1, configMap) require.NoError(t, err) assert.False(t, gone) - gone, err = oe.Teardown(ctx, owner, 1, configMap) + gone, err = oe.Teardown(ctx, ownerMetadata, 1, configMap) require.NoError(t, err) assert.True(t, gone) } diff --git a/test/revision_engine_basic_test.go b/test/revision_engine_basic_test.go index 84625e08..2a6f5dc8 100644 --- a/test/revision_engine_basic_test.go +++ b/test/revision_engine_basic_test.go @@ -23,6 +23,8 @@ import ( ) func TestRevisionEngine(t *testing.T) { + ctx := t.Context() + revOwner := &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: "rev-test", @@ -52,11 +54,21 @@ func TestRevisionEngine(t *testing.T) { }, }, } - rev := boxcutter.Revision{ - Name: "rev-1", - Owner: revOwner, - Revision: 1, - Phases: []boxcutter.Phase{ + + // Owner has to be there first: + err := Client.Create(ctx, revOwner) + require.NoError(t, err) + t.Cleanup(func() { + //nolint:usetesting + require.NoError(t, Client.Delete(context.Background(), revOwner)) + }) + + revOwnerMetadata := ownerhandling.NewNativeRevisionMetadata(revOwner, Scheme) + rev := boxcutter.NewRevision( + "rev-1", + revOwnerMetadata, + 1, + []boxcutter.Phase{ { Name: "phase-1", Objects: []unstructured.Unstructured{*obj1}, @@ -66,28 +78,17 @@ func TestRevisionEngine(t *testing.T) { Objects: []unstructured.Unstructured{*obj2}, }, }, - } + ) - os := ownerhandling.NewNative(Scheme) - comp := machinery.NewComparator(os, DiscoveryClient, Scheme, fieldOwner) + comp := machinery.NewComparator(DiscoveryClient, Scheme, fieldOwner) oe := machinery.NewObjectEngine( - Scheme, Client, Client, os, comp, fieldOwner, systemPrefix, + Scheme, Client, Client, comp, fieldOwner, systemPrefix, ) - pval := validation.NewNamespacedPhaseValidator(Client.RESTMapper(), Client) + 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) - t.Cleanup(func() { - //nolint:usetesting - require.NoError(t, Client.Delete(context.Background(), revOwner)) - }) - // Test execution // -------------- diff --git a/test/revision_engine_colision_protection_test.go b/test/revision_engine_colision_protection_test.go index f07c8b38..4421e274 100644 --- a/test/revision_engine_colision_protection_test.go +++ b/test/revision_engine_colision_protection_test.go @@ -18,6 +18,7 @@ import ( "pkg.package-operator.run/boxcutter/machinery" "pkg.package-operator.run/boxcutter/machinery/types" + "pkg.package-operator.run/boxcutter/ownerhandling" ) func TestCollisionProtectionPreventUnowned(t *testing.T) { @@ -37,12 +38,14 @@ func TestCollisionProtectionPreventUnowned(t *testing.T) { colliding.Data["apple"] = "pie" + ownerMetadata := ownerhandling.NewNativeRevisionMetadata(owner, Scheme) + re := newTestRevisionEngine() - res, err := re.Reconcile(ctx, types.Revision{ - Name: "test-collision-prevention-prevent-unowned-cm", - Revision: 1, - Owner: owner, - Phases: []types.Phase{ + res, err := re.Reconcile(ctx, types.NewRevision( + "test-collision-prevention-prevent-unowned-cm", + ownerMetadata, + 1, + []types.Phase{ { Name: "simple", Objects: []unstructured.Unstructured{ @@ -50,7 +53,7 @@ func TestCollisionProtectionPreventUnowned(t *testing.T) { }, }, }, - }) + )) require.NoError(t, err) assert.False(t, res.IsComplete()) assert.True(t, res.InTransition()) @@ -89,12 +92,14 @@ func TestCollisionProtectionPreventOwned(t *testing.T) { require.NoError(t, Client.Create(ctx, owner)) cleanupOnSuccess(t, owner) + ownerMetadata := ownerhandling.NewNativeRevisionMetadata(owner, Scheme) + re := newTestRevisionEngine() - res, err := re.Reconcile(ctx, types.Revision{ - Name: "test-collision-prevention-prevent-owned-cm", - Revision: 1, - Owner: owner, - Phases: []types.Phase{ + res, err := re.Reconcile(ctx, types.NewRevision( + "test-collision-prevention-prevent-owned-cm", + ownerMetadata, + 1, + []types.Phase{ { Name: "simple", Objects: []unstructured.Unstructured{ @@ -102,7 +107,7 @@ func TestCollisionProtectionPreventOwned(t *testing.T) { }, }, }, - }) + )) require.NoError(t, err) assert.False(t, res.IsComplete()) assert.True(t, res.InTransition()) @@ -122,13 +127,16 @@ func TestCollisionProtectionPreventOwned(t *testing.T) { // hco, ok := object.(*machinery.ObjectResultCollision) // Crude workaround: type hasConflictingOwner interface { - ConflictingOwner() (*metav1.OwnerReference, bool) + ConflictingOwner() (types.RevisionReference, bool) } hco, ok := object.(hasConflictingOwner) require.True(t, ok) conflictingOwner, ok := hco.ConflictingOwner() require.True(t, ok) - assert.Equal(t, existingOwner.GetUID(), conflictingOwner.UID) + // RevisionReference is an interface - type assert to get the OwnerReference + ownerRef, ok := conflictingOwner.(*metav1.OwnerReference) + require.True(t, ok) + assert.Equal(t, existingOwner.GetUID(), ownerRef.UID) } } diff --git a/test/revision_engine_preflight_validation_test.go b/test/revision_engine_preflight_validation_test.go index 2c9899d8..522d1088 100644 --- a/test/revision_engine_preflight_validation_test.go +++ b/test/revision_engine_preflight_validation_test.go @@ -15,6 +15,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "pkg.package-operator.run/boxcutter/machinery/types" + "pkg.package-operator.run/boxcutter/ownerhandling" "pkg.package-operator.run/boxcutter/validation" ) @@ -46,12 +47,14 @@ func TestWithOwnerReference(t *testing.T) { }, } + ownerMetadata := ownerhandling.NewNativeRevisionMetadata(owner, Scheme) + re := newTestRevisionEngine() - res, err := re.Reconcile(ctx, types.Revision{ - Name: "test-collision-prevention-invalid-set", - Revision: 1, - Owner: owner, - Phases: []types.Phase{ + res, err := re.Reconcile(ctx, types.NewRevision( + "test-collision-prevention-invalid-set", + ownerMetadata, + 1, + []types.Phase{ { Name: "simple", Objects: []unstructured.Unstructured{ @@ -59,7 +62,7 @@ func TestWithOwnerReference(t *testing.T) { }, }, }, - }) + )) require.NoError(t, err) assert.False(t, res.IsComplete()) diff --git a/validation/object.go b/validation/object.go index fea52f47..f73b203e 100644 --- a/validation/object.go +++ b/validation/object.go @@ -29,26 +29,10 @@ type restMapper interface { type ObjectValidator struct { restMapper restMapper writer client.Writer - - // Allows creating objects in namespaces different to Owner. - allowNamespaceEscalation bool -} - -// NewClusterObjectValidator returns an ObjectValidator for cross-cluster deployments. -func NewClusterObjectValidator( - restMapper restMapper, - writer client.Writer, -) *ObjectValidator { - return &ObjectValidator{ - restMapper: restMapper, - writer: writer, - - allowNamespaceEscalation: true, - } } -// NewNamespacedObjectValidator returns an ObjecctValidator for single-namespace deployments. -func NewNamespacedObjectValidator( +// NewObjectValidator returns an ObjectValidator. +func NewObjectValidator( restMapper restMapper, writer client.Writer, ) *ObjectValidator { @@ -63,21 +47,17 @@ func NewNamespacedObjectValidator( // It returns an ObjectValidationError when it was successfully able to validate the Object. // It returns a different error when unable to validate the object. func (d *ObjectValidator) Validate( - ctx context.Context, owner client.Object, + 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...) } // Dry run against API server to catch any other surprises. @@ -93,6 +73,17 @@ func (d *ObjectValidator) Validate( return err } +// NamespaceNotAllowedError is returned when an object is in a namespace +// not allowed by the RevisionMetadata. +type NamespaceNotAllowedError struct { + Namespace string +} + +// Error implements the error interface. +func (e NamespaceNotAllowedError) Error() string { + return fmt.Sprintf("namespace %q is not allowed by the revision metadata", e.Namespace) +} + // MustBeNamespaceScopedResourceError is returned when a cluster-scoped // resource is used in a namespaced context. type MustBeNamespaceScopedResourceError struct{} diff --git a/validation/object_test.go b/validation/object_test.go index fc04525b..fd728e3d 100644 --- a/validation/object_test.go +++ b/validation/object_test.go @@ -8,13 +8,17 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" apimachineryerrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" + + "pkg.package-operator.run/boxcutter/ownerhandling" ) type mockRestMapper struct { @@ -70,49 +74,52 @@ func (m *mockWriter) DeleteAllOf(ctx context.Context, obj client.Object, opts .. return args.Error(0) } -func TestNewClusterObjectValidator(t *testing.T) { +func TestNewObjectValidator(t *testing.T) { t.Parallel() restMapper := &mockRestMapper{} writer := &mockWriter{} - validator := NewClusterObjectValidator(restMapper, writer) + validator := NewObjectValidator(restMapper, writer) assert.NotNil(t, validator) - assert.True(t, validator.allowNamespaceEscalation) - assert.Equal(t, restMapper, validator.restMapper) - assert.Equal(t, writer, validator.writer) } -func TestNewNamespacedObjectValidator(t *testing.T) { - t.Parallel() - - restMapper := &mockRestMapper{} - writer := &mockWriter{} +// testScheme returns a scheme with corev1 types registered for testing. +func testScheme() *runtime.Scheme { + scheme := runtime.NewScheme() + _ = corev1.AddToScheme(scheme) - validator := NewNamespacedObjectValidator(restMapper, writer) + return scheme +} - assert.NotNil(t, validator) - assert.False(t, validator.allowNamespaceEscalation) - assert.Equal(t, restMapper, validator.restMapper) - assert.Equal(t, writer, validator.writer) +// testOwner creates a ConfigMap owner with a UID for testing. +func testOwner(namespace string) *corev1.ConfigMap { + return &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-owner", + Namespace: namespace, + UID: types.UID("test-owner-uid"), + }, + } } func TestObjectValidator_Validate(t *testing.T) { t.Parallel() + scheme := testScheme() + tests := []struct { - name string - allowNamespaceEscalation bool - obj *unstructured.Unstructured - owner client.Object - mockSetup func(*mockRestMapper, *mockWriter) - expectError bool - expectValidationError bool + name string + ownerNamespace string + obj *unstructured.Unstructured + mockSetup func(*mockRestMapper, *mockWriter) + expectError bool + expectValidationError bool }{ { - name: "valid object with cluster validator", - allowNamespaceEscalation: true, + name: "valid object with cross-namespace allowed", + ownerNamespace: "default", obj: &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "v1", @@ -123,13 +130,6 @@ func TestObjectValidator_Validate(t *testing.T) { }, }, }, - owner: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "namespace": "default", - }, - }, - }, mockSetup: func(_ *mockRestMapper, writer *mockWriter) { writer.On("Patch", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil) }, @@ -137,8 +137,8 @@ func TestObjectValidator_Validate(t *testing.T) { expectValidationError: false, }, { - name: "valid object with namespaced validator", - allowNamespaceEscalation: false, + name: "valid object in same namespace", + ownerNamespace: "default", obj: &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "v1", @@ -149,24 +149,15 @@ func TestObjectValidator_Validate(t *testing.T) { }, }, }, - owner: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "namespace": "default", - }, - }, - }, - mockSetup: func(restMapper *mockRestMapper, writer *mockWriter) { - restMapper.On("RESTMapping", schema.GroupKind{Group: "", Kind: "ConfigMap"}, []string{"v1"}).Return( - &meta.RESTMapping{Scope: meta.RESTScopeNamespace}, nil) + mockSetup: func(_ *mockRestMapper, writer *mockWriter) { writer.On("Patch", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil) }, expectError: false, expectValidationError: false, }, { - name: "namespace validation fails", - allowNamespaceEscalation: false, + name: "namespace validation fails - cross-namespace not allowed", + ownerNamespace: "default", obj: &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "v1", @@ -177,23 +168,13 @@ func TestObjectValidator_Validate(t *testing.T) { }, }, }, - owner: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "namespace": "default", - }, - }, - }, - mockSetup: func(restMapper *mockRestMapper, _ *mockWriter) { - restMapper.On("RESTMapping", schema.GroupKind{Group: "", Kind: "ConfigMap"}, []string{"v1"}).Return( - &meta.RESTMapping{Scope: meta.RESTScopeNamespace}, nil) - }, + mockSetup: func(_ *mockRestMapper, _ *mockWriter) {}, expectError: true, expectValidationError: true, }, { - name: "dry run validation fails", - allowNamespaceEscalation: true, + name: "dry run validation fails", + ownerNamespace: "default", obj: &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "v1", @@ -204,13 +185,6 @@ func TestObjectValidator_Validate(t *testing.T) { }, }, }, - owner: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "namespace": "default", - }, - }, - }, mockSetup: func(_ *mockRestMapper, writer *mockWriter) { statusErr := &apimachineryerrors.StatusError{ ErrStatus: metav1.Status{ @@ -232,13 +206,12 @@ func TestObjectValidator_Validate(t *testing.T) { writer := &mockWriter{} test.mockSetup(restMapper, writer) - validator := &ObjectValidator{ - restMapper: restMapper, - writer: writer, - allowNamespaceEscalation: test.allowNamespaceEscalation, - } + validator := NewObjectValidator(restMapper, writer) + + owner := testOwner(test.ownerNamespace) + metadata := ownerhandling.NewNativeRevisionMetadata(owner, scheme) - err := validator.Validate(t.Context(), test.owner, test.obj) + err := validator.Validate(t.Context(), metadata, test.obj) if test.expectError { require.Error(t, err) diff --git a/validation/phase.go b/validation/phase.go index 8045324c..3248558c 100644 --- a/validation/phase.go +++ b/validation/phase.go @@ -19,29 +19,19 @@ type PhaseValidator struct { *ObjectValidator } -// NewClusterPhaseValidator returns an PhaseValidator for cross-cluster deployments. -func NewClusterPhaseValidator( +// NewPhaseValidator returns a PhaseValidator. +func NewPhaseValidator( restMapper restMapper, writer client.Writer, ) *PhaseValidator { return &PhaseValidator{ - ObjectValidator: NewClusterObjectValidator(restMapper, writer), - } -} - -// NewNamespacedPhaseValidator returns an ObjecctValidator for single-namespace deployments. -func NewNamespacedPhaseValidator( - restMapper restMapper, - writer client.Writer, -) *PhaseValidator { - return &PhaseValidator{ - ObjectValidator: NewNamespacedObjectValidator(restMapper, writer), + ObjectValidator: NewObjectValidator(restMapper, writer), } } // Validate runs validation of the phase and its objects. func (v *PhaseValidator) Validate( - ctx context.Context, owner client.Object, phase types.Phase, + ctx context.Context, metadata types.RevisionMetadata, phase types.Phase, ) error { phaseError := validatePhaseName(phase) @@ -53,7 +43,7 @@ func (v *PhaseValidator) Validate( for _, o := range phase.GetObjects() { obj := &o - err := v.ObjectValidator.Validate(ctx, owner, obj) + err := v.ObjectValidator.Validate(ctx, metadata, obj) if err == nil { continue } diff --git a/validation/phase_test.go b/validation/phase_test.go index 8ce3e48e..361c4a4f 100644 --- a/validation/phase_test.go +++ b/validation/phase_test.go @@ -13,14 +13,15 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "pkg.package-operator.run/boxcutter/machinery/types" + "pkg.package-operator.run/boxcutter/ownerhandling" ) type mockObjectValidator struct { mock.Mock } -func (m *mockObjectValidator) Validate(ctx context.Context, owner client.Object, obj *unstructured.Unstructured) error { - args := m.Called(ctx, owner, obj) +func (m *mockObjectValidator) Validate(ctx context.Context, metadata types.RevisionMetadata, obj *unstructured.Unstructured) error { + args := m.Called(ctx, metadata, obj) return args.Error(0) } @@ -30,7 +31,7 @@ type testablePhaseValidator struct { mockObjValidator *mockObjectValidator } -func (v *testablePhaseValidator) Validate(ctx context.Context, owner client.Object, phase types.Phase) error { +func (v *testablePhaseValidator) Validate(ctx context.Context, metadata types.RevisionMetadata, phase types.Phase) error { phaseError := validatePhaseName(phase) var ( @@ -41,7 +42,7 @@ func (v *testablePhaseValidator) Validate(ctx context.Context, owner client.Obje for _, o := range phase.GetObjects() { obj := &o - err := v.mockObjValidator.Validate(ctx, owner, obj) + err := v.mockObjValidator.Validate(ctx, metadata, obj) if err == nil { continue } @@ -66,39 +67,27 @@ func (v *testablePhaseValidator) Validate(ctx context.Context, owner client.Obje return result } -func TestNewClusterPhaseValidator(t *testing.T) { +func TestNewPhaseValidator(t *testing.T) { t.Parallel() restMapper := &mockRestMapper{} writer := &mockWriter{} - validator := NewClusterPhaseValidator(restMapper, writer) + validator := NewPhaseValidator(restMapper, writer) assert.NotNil(t, validator) assert.NotNil(t, validator.ObjectValidator) - assert.True(t, validator.allowNamespaceEscalation) -} - -func TestNewNamespacedPhaseValidator(t *testing.T) { - t.Parallel() - - restMapper := &mockRestMapper{} - writer := &mockWriter{} - - validator := NewNamespacedPhaseValidator(restMapper, writer) - - assert.NotNil(t, validator) - assert.NotNil(t, validator.ObjectValidator) - assert.False(t, validator.allowNamespaceEscalation) } func TestPhaseValidator_Validate(t *testing.T) { t.Parallel() + scheme := testScheme() + tests := []struct { name string phase types.Phase - owner client.Object + ownerNamespace string mockSetup func(*mockObjectValidator) expectError bool expectPhaseValidationErr bool @@ -121,13 +110,7 @@ func TestPhaseValidator_Validate(t *testing.T) { }, }, }, - owner: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "namespace": "default", - }, - }, - }, + ownerNamespace: "default", mockSetup: func(objValidator *mockObjectValidator) { objValidator.On("Validate", mock.Anything, mock.Anything, mock.Anything).Return(nil) }, @@ -150,13 +133,7 @@ func TestPhaseValidator_Validate(t *testing.T) { }, }, }, - owner: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "namespace": "default", - }, - }, - }, + ownerNamespace: "default", mockSetup: func(objValidator *mockObjectValidator) { objValidator.On("Validate", mock.Anything, mock.Anything, mock.Anything).Return(nil) }, @@ -180,13 +157,7 @@ func TestPhaseValidator_Validate(t *testing.T) { }, }, }, - owner: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "namespace": "default", - }, - }, - }, + ownerNamespace: "default", mockSetup: func(objValidator *mockObjectValidator) { objErr := &ObjectValidationError{ ObjectRef: testObjRef, @@ -215,13 +186,7 @@ func TestPhaseValidator_Validate(t *testing.T) { }, }, }, - owner: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "namespace": "default", - }, - }, - }, + ownerNamespace: "default", mockSetup: func(objValidator *mockObjectValidator) { objValidator.On("Validate", mock.Anything, mock.Anything, mock.Anything).Return( errors.New("unknown error")) @@ -256,13 +221,7 @@ func TestPhaseValidator_Validate(t *testing.T) { }, }, }, - owner: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "namespace": "default", - }, - }, - }, + ownerNamespace: "default", mockSetup: func(objValidator *mockObjectValidator) { objValidator.On("Validate", mock.Anything, mock.Anything, mock.Anything).Return(nil) }, @@ -279,6 +238,9 @@ func TestPhaseValidator_Validate(t *testing.T) { var objValidator *mockObjectValidator + owner := testOwner(test.ownerNamespace) + metadata := ownerhandling.NewNativeRevisionMetadata(owner, scheme) + if test.useRealValidator { restMapper := &mockRestMapper{} writer := &mockWriter{} @@ -287,8 +249,8 @@ func TestPhaseValidator_Validate(t *testing.T) { &meta.RESTMapping{Scope: meta.RESTScopeNamespace}, nil) writer.On("Patch", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil) - realValidator := NewClusterPhaseValidator(restMapper, writer) - err = realValidator.Validate(t.Context(), test.owner, test.phase) + realValidator := NewPhaseValidator(restMapper, writer) + err = realValidator.Validate(t.Context(), metadata, test.phase) } else { objValidator = &mockObjectValidator{} test.mockSetup(objValidator) @@ -298,7 +260,7 @@ func TestPhaseValidator_Validate(t *testing.T) { mockObjValidator: objValidator, } - err = validator.Validate(t.Context(), test.owner, test.phase) + err = validator.Validate(t.Context(), metadata, test.phase) } if test.expectError { diff --git a/validation/revision_test.go b/validation/revision_test.go index 4cfc1b2e..619fd007 100644 --- a/validation/revision_test.go +++ b/validation/revision_test.go @@ -29,10 +29,11 @@ func TestRevisionValidator_Validate(t *testing.T) { }{ { name: "valid revision", - revision: types.Revision{ - Name: "test-revision", - Revision: 1, - Phases: []types.Phase{ + revision: types.NewRevision[types.RevisionMetadata]( + "test-revision", + nil, + 1, + []types.Phase{ { Name: "phase1", Objects: []unstructured.Unstructured{ @@ -64,15 +65,16 @@ func TestRevisionValidator_Validate(t *testing.T) { }, }, }, - }, + ), expectError: false, }, { name: "revision with invalid phase name", - revision: types.Revision{ - Name: "test-revision", - Revision: 1, - Phases: []types.Phase{ + revision: types.NewRevision[types.RevisionMetadata]( + "test-revision", + nil, + 1, + []types.Phase{ { Name: "Invalid_Phase_Name", Objects: []unstructured.Unstructured{ @@ -89,16 +91,17 @@ func TestRevisionValidator_Validate(t *testing.T) { }, }, }, - }, + ), expectError: true, expectRevisionValidationErr: true, }, { name: "revision with metadata validation errors", - revision: types.Revision{ - Name: "test-revision", - Revision: 1, - Phases: []types.Phase{ + revision: types.NewRevision[types.RevisionMetadata]( + "test-revision", + nil, + 1, + []types.Phase{ { Name: "phase1", Objects: []unstructured.Unstructured{ @@ -114,16 +117,17 @@ func TestRevisionValidator_Validate(t *testing.T) { }, }, }, - }, + ), expectError: true, expectRevisionValidationErr: true, }, { name: "revision with duplicate objects across phases", - revision: types.Revision{ - Name: "test-revision", - Revision: 1, - Phases: []types.Phase{ + revision: types.NewRevision[types.RevisionMetadata]( + "test-revision", + nil, + 1, + []types.Phase{ { Name: "phase1", Objects: []unstructured.Unstructured{ @@ -155,18 +159,19 @@ func TestRevisionValidator_Validate(t *testing.T) { }, }, }, - }, + ), // Duplicate detection now works properly expectError: true, expectRevisionValidationErr: true, }, { name: "empty revision", - revision: types.Revision{ - Name: "test-revision", - Revision: 1, - Phases: []types.Phase{}, - }, + revision: types.NewRevision[types.RevisionMetadata]( + "test-revision", + nil, + 1, + []types.Phase{}, + ), expectError: false, }, }