diff --git a/boxcutter.go b/boxcutter.go index a2133178..2810b30a 100644 --- a/boxcutter.go +++ b/boxcutter.go @@ -3,23 +3,40 @@ 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" ) // Revision represents multiple phases at a given point in time. type Revision = types.Revision +// RevisionBuilder is a Revision with methods to attach options. +type RevisionBuilder = types.RevisionBuilder + +// NewRevision creates a new RevisionBuilder with the given name, rev and phases. +var NewRevision = types.NewRevision + +// NewRevisionWithOwner creates a new RevisionBuilder +// with the given name, rev, phases and owner. +var NewRevisionWithOwner = types.NewRevisionWithOwner + // Phase represents a collection of objects lifecycled together. type Phase = types.Phase +// PhaseBuilder is a Phase with methods to attach options. +type PhaseBuilder = types.PhaseBuilder + +// NewPhase creates a new PhaseBuilder with the given name and objects. +var NewPhase = types.NewPhase + +// NewPhaseWithOwner creates a new PhaseBuilder with the given name, objects and owner. +var NewPhaseWithOwner = types.NewPhaseWithOwner + // ObjectReconcileOption is the common interface for object reconciliation options. type ObjectReconcileOption = types.ObjectReconcileOption @@ -86,6 +103,11 @@ var WithPhaseReconcileOptions = types.WithPhaseReconcileOptions // WithPhaseTeardownOptions applies the given options only to the given Phase. var WithPhaseTeardownOptions = types.WithPhaseTeardownOptions +// WithOwner sets an owning object and the strategy to use with it. +// Ensures controller-refs are set to track the owner and +// enables handover between owners. +var WithOwner = types.WithOwner + // ProgressProbeType is a well-known probe type used to guard phase progression. const ProgressProbeType = types.ProgressProbeType @@ -93,14 +115,7 @@ const ProgressProbeType = types.ProgressProbeType 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) -} +type OwnerStrategy = types.OwnerStrategy // RevisionEngineOptions holds all configuration options for the RevisionEngine. type RevisionEngineOptions struct { @@ -114,7 +129,6 @@ type RevisionEngineOptions struct { // Optional - OwnerStrategy OwnerStrategy PhaseValidator *validation.PhaseValidator } @@ -124,20 +138,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) } 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 +159,15 @@ func NewRevisionEngine(opts RevisionEngineOptions) (*RevisionEngine, error) { return nil, err } - if opts.OwnerStrategy == nil { - opts.OwnerStrategy = ownerhandling.NewNative(opts.Scheme) - } - 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 d085a17f..e0a9ed74 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" ) @@ -48,8 +49,9 @@ type Reconciler struct { discoveryClient *discovery.DiscoveryClient restMapper meta.RESTMapper - cache managedcache.ObjectBoundAccessManager[*corev1.ConfigMap] - scheme *runtime.Scheme + cache managedcache.ObjectBoundAccessManager[*corev1.ConfigMap] + scheme *runtime.Scheme + ownerStrategy boxcutter.OwnerStrategy } func NewReconciler( @@ -65,6 +67,7 @@ func NewReconciler( restMapper: restMapper, cache: cache, scheme: scheme, + ownerStrategy: ownerhandling.NewNative(scheme), } } @@ -130,7 +133,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)) @@ -146,7 +149,7 @@ func (c *Reconciler) handleDeployment(ctx context.Context, cm *corev1.ConfigMap) if len(existingRevisions) > 0 { maybeCurrentObjectSet := existingRevisions[len(existingRevisions)-1] - annotations := maybeCurrentObjectSet.GetOwner().GetAnnotations() + annotations := getOwnerFromRev(maybeCurrentObjectSet).GetAnnotations() if annotations != nil { if hash, ok := annotations[hashAnnotation]; ok && hash == currentHash { @@ -197,7 +200,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, getOwnerFromRev(prevRev))); err != nil { return res, fmt.Errorf("failed to delete revision (history limit): %w", err) } @@ -218,9 +221,7 @@ func (c *Reconciler) handleRevision( var objects []client.Object for _, phase := range revision.GetPhases() { - for _, pobj := range phase.GetObjects() { - objects = append(objects, &pobj) - } + objects = append(objects, phase.GetObjects()...) } accessor, err := c.cache.GetWithUser(ctx, deploy, revisionCM, objects) @@ -243,7 +244,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 +271,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,19 +331,19 @@ 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 boxcutter.Revision, opts []boxcutter.RevisionReconcileOption, previous []client.Object, err error, ) { var ( - phases []string + phaseNames []string previousUnstr []unstructured.Unstructured revision int64 ) - objects := map[string][]unstructured.Unstructured{} + objects := map[string][]client.Object{} for k, v := range cm.Data { if k == cmPhasesKey { - if err := json.Unmarshal([]byte(v), &phases); err != nil { + if err := json.Unmarshal([]byte(v), &phaseNames); err != nil { return nil, nil, nil, fmt.Errorf("json unmarshal key %s: %w", k, err) } @@ -375,8 +376,8 @@ func (c *Reconciler) toRevision(deployName string, cm *corev1.ConfigMap) ( phase := parts[0] - obj := unstructured.Unstructured{} - if err := json.Unmarshal([]byte(v), &obj); err != nil { + obj := &unstructured.Unstructured{} + if err := json.Unmarshal([]byte(v), obj); err != nil { return nil, nil, nil, fmt.Errorf("json unmarshal key %s: %w", k, err) } @@ -399,25 +400,25 @@ 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, - } - for _, obj := range previousUnstr { previous = append(previous, &obj) } - for _, phase := range phases { - p := boxcutter.Phase{ - Name: phase, - Objects: objects[phase], - } + phases := make([]boxcutter.Phase, 0, len(phaseNames)) + for _, phaseName := range phaseNames { + p := boxcutter.NewPhase( + phaseName, + objects[phaseName], + ) - rev.Phases = append(rev.Phases, p) + phases = append(phases, p) } + rev := boxcutter.NewRevisionWithOwner( + cm.Name, revision, phases, + cm, c.ownerStrategy, + ) + opts = []boxcutter.RevisionReconcileOption{ boxcutter.WithPreviousOwners(previous), boxcutter.WithProbe( diff --git a/cmd/reference/internal/util.go b/cmd/reference/internal/util.go index 0e77af16..ae62d44c 100644 --- a/cmd/reference/internal/util.go +++ b/cmd/reference/internal/util.go @@ -7,6 +7,7 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "sigs.k8s.io/controller-runtime/pkg/client" + "pkg.package-operator.run/boxcutter" bctypes "pkg.package-operator.run/boxcutter/machinery/types" ) @@ -33,7 +34,7 @@ func prevJSON(prevRevisions []bctypes.Revision) string { data := make([]unstructured.Unstructured, 0, len(prevRevisions)) for _, rev := range prevRevisions { - refObj := rev.GetOwner() + refObj := getOwnerFromRev(rev) ref := unstructured.Unstructured{} ref.SetGroupVersionKind(refObj.GetObjectKind().GroupVersionKind()) ref.SetName(refObj.GetName()) @@ -59,3 +60,12 @@ func getOwner(obj client.Object) (metav1.OwnerReference, bool) { return metav1.OwnerReference{}, false } + +func getOwnerFromRev(rev boxcutter.Revision) client.Object { + var options bctypes.RevisionReconcileOptions + for _, opt := range rev.GetReconcileOptions() { + opt.ApplyToRevisionReconcileOptions(&options) + } + + return options.GetOwner() +} diff --git a/machinery/comparator.go b/machinery/comparator.go index 13069932..7027f43b 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,9 +173,14 @@ func (d CompareResult) Modified() []string { // Compare checks if a resource has been changed from desired. func (d *Comparator) Compare( - owner client.Object, desiredObject, actualObject Object, + opts ...types.ComparatorOption, ) (res CompareResult, err error) { + var options types.ComparatorOptions + for _, opt := range opts { + opt.ApplyToComparatorOptions(&options) + } + if err := ensureGVKIsSet(desiredObject, d.scheme); err != nil { return res, err } @@ -227,8 +227,12 @@ func (d *Comparator) Compare( // Extrapolate a field set from desired. desiredObject = desiredObject.DeepCopyObject().(Object) - if err := d.ownerStrategy.SetControllerReference(owner, desiredObject); err != nil { - return res, err + if options.Owner != nil { + if err := options.OwnerStrategy.SetControllerReference( + options.Owner, desiredObject, + ); err != nil { + return res, err + } } tName, err := openAPICanonicalName(desiredObject) @@ -435,7 +439,7 @@ func ensureGVKIsSet(obj client.Object, scheme *runtime.Scheme) error { const statusSubresourceSuffix = "{name}/status" // Determines if the schema has a Status subresource defined. -// If so the comperator has to ignore .status, because the API server will also ignore these fields. +// If so the Comparator has to ignore .status, because the API server will also ignore these fields. func hasStatusSubresource(openAPISchema *spec3.OpenAPI) bool { if openAPISchema.Paths == nil { return false diff --git a/machinery/comparator_test.go b/machinery/comparator_test.go index a958fdc1..a31d9953 100644 --- a/machinery/comparator_test.go +++ b/machinery/comparator_test.go @@ -5,6 +5,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -12,8 +13,10 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/kubernetes/scheme" + "k8s.io/client-go/openapi" "k8s.io/kube-openapi/pkg/spec3" + bctypes "pkg.package-operator.run/boxcutter/machinery/types" "pkg.package-operator.run/boxcutter/ownerhandling" ) @@ -29,12 +32,11 @@ func TestComparator_Unstructured(t *testing.T) { oapi := &spec3.OpenAPI{} require.NoError(t, oapi.UnmarshalJSON(testOAPISchema)) - a := &dummyOpenAPIAccessor{ - openAPI: oapi, - } + a := &dummyOpenAPIAccessor{} + a.On("Get", mock.Anything).Return(oapi, nil) + n := ownerhandling.NewNative(scheme.Scheme) d := &Comparator{ - ownerStrategy: n, openAPIAccessor: a, fieldOwner: testFieldOwner, } @@ -179,6 +181,7 @@ func TestComparator_Unstructured(t *testing.T) { name string desired *unstructured.Unstructured actual *unstructured.Unstructured + opts []bctypes.ComparatorOption expectedReport string }{ @@ -186,15 +189,43 @@ func TestComparator_Unstructured(t *testing.T) { name: "Hans updated .data.test", desired: desiredNewFieldOwner, actual: actualNewFieldOwner, + opts: []bctypes.ComparatorOption{ + bctypes.WithOwner(owner, n), + }, + + expectedReport: `Conflicts: +- "Hans" + .data.test +`, + }, + { + name: "Hans updated .data.test - no owner", + desired: desiredNewFieldOwner, + actual: actualNewFieldOwner, + expectedReport: `Conflicts: - "Hans" .data.test +Comparison: +- Added: + .metadata.ownerReferences[uid="12345-678"] `, }, { - name: "Pod Compare", - desired: pod.DeepCopy(), - actual: pod.DeepCopy(), + name: "Pod Compare", + desired: pod.DeepCopy(), + actual: pod.DeepCopy(), + opts: []bctypes.ComparatorOption{ + bctypes.WithOwner(owner, n), + }, + + expectedReport: ``, + }, + { + name: "Pod Compare - no owner", + desired: pod.DeepCopy(), + actual: pod.DeepCopy(), + expectedReport: ``, }, } @@ -202,14 +233,10 @@ 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(test.desired, test.actual, test.opts...) require.NoError(t, err) assert.Equal(t, test.expectedReport, res.String()) - - if res.Comparison != nil { - assert.True(t, res.Comparison.IsSame(), res.Comparison.String()) - } }) } @@ -259,7 +286,7 @@ func TestComparator_Unstructured(t *testing.T) { err = n.SetControllerReference(owner, actualValueChange) require.NoError(t, err) - res, err := d.Compare(owner, desiredValueChange, actualValueChange) + res, err := d.Compare(desiredValueChange, actualValueChange, bctypes.WithOwner(owner, n)) require.NoError(t, err) // no conflicts assert.Empty(t, res.ConflictingMangers) @@ -278,12 +305,11 @@ func TestComparator_Structured(t *testing.T) { oapi := &spec3.OpenAPI{} require.NoError(t, oapi.UnmarshalJSON(testOAPISchema)) - a := &dummyOpenAPIAccessor{ - openAPI: oapi, - } + a := &dummyOpenAPIAccessor{} + a.On("Get", mock.Anything).Return(oapi, nil) + n := ownerhandling.NewNative(scheme.Scheme) d := &Comparator{ - ownerStrategy: n, openAPIAccessor: a, fieldOwner: testFieldOwner, } @@ -491,7 +517,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(test.desired, test.actual, bctypes.WithOwner(owner, n)) require.NoError(t, err) if res.Comparison != nil { @@ -546,7 +572,7 @@ func TestComparator_Structured(t *testing.T) { err = n.SetControllerReference(owner, actualValueChange) require.NoError(t, err) - res, err := d.Compare(owner, desiredValueChange, actualValueChange) + res, err := d.Compare(desiredValueChange, actualValueChange, bctypes.WithOwner(owner, n)) require.NoError(t, err) // no conflicts assert.Empty(t, res.ConflictingMangers) @@ -555,12 +581,78 @@ func TestComparator_Structured(t *testing.T) { }) } +func TestNewComparator(t *testing.T) { + t.Parallel() + + // Use a simple mock discovery client + oapiClient := &simpleOpenAPIClientMock{} + oapiClient.On("Paths").Return(nil, nil) + oapiClient.On("GV", mock.Anything).Return(&spec3.OpenAPI{}, nil) + + discoveryClient := &simpleDiscoveryMock{} + discoveryClient.On("OpenAPIV3").Return(oapiClient) + + c := NewComparator(discoveryClient, scheme.Scheme, "test-owner") + + assert.NotNil(t, c) + assert.Equal(t, "test-owner", c.fieldOwner) + assert.NotNil(t, c.openAPIAccessor) + assert.Equal(t, scheme.Scheme, c.scheme) +} + +func TestCompareResult_Modified(t *testing.T) { + t.Parallel() + + // Test the nil comparison case which has 0% coverage + result := CompareResult{Comparison: nil} + modified := result.Modified() + + assert.Nil(t, modified) +} + +type simpleDiscoveryMock struct { + mock.Mock +} + +func (m *simpleDiscoveryMock) OpenAPIV3() openapi.Client { + args := m.Called() + + return args.Get(0).(openapi.Client) +} + +type simpleOpenAPIClientMock struct { + mock.Mock +} + +func (m *simpleOpenAPIClientMock) Paths() (map[string]openapi.GroupVersion, error) { + args := m.Called() + if args.Get(0) == nil { + return nil, args.Error(1) + } + + return args.Get(0).(map[string]openapi.GroupVersion), args.Error(1) +} + +func (m *simpleOpenAPIClientMock) GV(path string) (*spec3.OpenAPI, error) { + args := m.Called(path) + if args.Get(0) == nil { + return nil, args.Error(1) + } + + return args.Get(0).(*spec3.OpenAPI), args.Error(1) +} + type dummyOpenAPIAccessor struct { - openAPI *spec3.OpenAPI + mock.Mock } -func (a *dummyOpenAPIAccessor) Get(_ schema.GroupVersion) (*spec3.OpenAPI, error) { - return a.openAPI, nil +func (a *dummyOpenAPIAccessor) Get(gv schema.GroupVersion) (*spec3.OpenAPI, error) { + args := a.Called(gv) + if args.Get(0) == nil { + return nil, args.Error(1) + } + + return args.Get(0).(*spec3.OpenAPI), args.Error(1) } func Test_openAPICanonicalName(t *testing.T) { diff --git a/machinery/objects.go b/machinery/objects.go index 9d92a927..d4e3d0cf 100644 --- a/machinery/objects.go +++ b/machinery/objects.go @@ -4,7 +4,6 @@ import ( "context" "fmt" "strconv" - "strings" "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" @@ -23,11 +22,10 @@ import ( // 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 +36,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, @@ -73,19 +69,20 @@ type objectEngineOwnerStrategy interface { type comparator interface { Compare( - owner client.Object, desiredObject, actualObject Object, + opts ...types.ComparatorOption, ) (res CompareResult, err error) } const ( + managedByLabel string = "app.kubernetes.io/managed-by" + managedByLabelValue string = "boxcutter" boxcutterManagedLabel string = "boxcutter-managed" ) // 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. revision int64, // Revision number, must start at 1. desiredObject Object, opts ...types.ObjectTeardownOption, @@ -102,15 +99,12 @@ 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 { - err := removeBoxcutterManagedLabel(ctx, e.writer, desiredObject.(*unstructured.Unstructured)) + // The "orphan" finalizer on the owner object indicates that the Owner + // is being deleted and orphaning its dependents. This finalizer is + // managed by KCM's gc controller. If we observe it, we are racing with + // the gc controller, and should not delete dependent objects. + if options.Orphan || (options.Owner != nil && controllerutil.ContainsFinalizer(options.Owner, "orphan")) { + err := e.removeBoxcutterManagedLabelsAndAnnotations(ctx, e.writer, desiredObject) if err != nil { return false, err } @@ -145,13 +139,21 @@ func (e *ObjectEngine) Teardown( return false, fmt.Errorf("getting object revision: %w", err) } - ctrlSit, _ := e.detectOwner(owner, actualObject, nil) - if actualRevision != revision || ctrlSit != ctrlSituationIsController { - // Remove us from owners list: - patch := actualObject.DeepCopyObject().(Object) - e.ownerStrategy.RemoveOwner(owner, patch) + // Object is not owned by this revision + if actualRevision != revision { + if options.Owner == nil { + // No Owner to remove from the object, return. + return true, nil + } + + ctrlSit, _ := e.detectOwner(options.Owner, options.OwnerStrategy, actualObject, nil) + if ctrlSit != ctrlSituationIsController { + // Remove us from owners list: + patch := actualObject.DeepCopyObject().(Object) + options.OwnerStrategy.RemoveOwner(options.Owner, patch) - return true, e.writer.Patch(ctx, patch, client.MergeFrom(actualObject)) + return true, e.writer.Patch(ctx, patch, client.MergeFrom(actualObject)) + } } // Actually delete the object. @@ -178,7 +180,6 @@ 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. revision int64, // Revision number, must start at 1. desiredObject Object, opts ...types.ObjectReconcileOption, @@ -193,7 +194,7 @@ func (e *ObjectEngine) Reconcile( labels = map[string]string{} } - labels[boxcutterManagedLabel] = "True" + labels[managedByLabel] = managedByLabelValue desiredObject.SetLabels(labels) options.Default() @@ -203,10 +204,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,10 +212,12 @@ func (e *ObjectEngine) Reconcile( desiredObject = desiredObject.DeepCopyObject().(Object) e.setObjectRevision(desiredObject, revision) - if err := e.ownerStrategy.SetControllerReference( - owner, desiredObject, - ); err != nil { - return nil, fmt.Errorf("set controller reference: %w", err) + if options.Owner != nil { + if err := options.OwnerStrategy.SetControllerReference( + options.Owner, desiredObject, + ); err != nil { + return nil, fmt.Errorf("set controller reference: %w", err) + } } // Lookup actual object state on cluster. @@ -261,27 +260,66 @@ func (e *ObjectEngine) Reconcile( } return e.objectUpdateHandling( - ctx, owner, revision, - desiredObject, actualObject, options, + ctx, revision, desiredObject, + actualObject, options, ) } -func (e *ObjectEngine) objectUpdateHandling( - ctx context.Context, - owner client.Object, - revision int64, +func (e *ObjectEngine) checkSituation( desiredObject Object, actualObject Object, options types.ObjectReconcileOptions, -) (ObjectResult, error) { +) ( + ctrlSit ctrlSituation, + compareRes CompareResult, + actualOwner *metav1.OwnerReference, + err error, +) { + var compareOpts []types.ComparatorOption + + if options.Owner != nil { + ctrlSit, actualOwner = e.detectOwner( + options.Owner, options.OwnerStrategy, actualObject, options.PreviousOwners) + + compareOpts = append(compareOpts, types.WithOwner(options.Owner, options.OwnerStrategy)) + } else { + if e.isBoxcutterManaged(actualObject) { + ctrlSit = ctrlSituationIsController + } else { + ctrlSit = ctrlSituationNoController + } + } + // 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) + compareRes, err = e.comparator.Compare(desiredObject, actualObject, compareOpts...) + if err != nil { + err = fmt.Errorf("diverge check: %w", err) - compareRes, err := e.comparator.Compare(owner, desiredObject, actualObject) + return ctrlSit, + compareRes, + actualOwner, + err + } + + return ctrlSit, + compareRes, + actualOwner, + err +} + +func (e *ObjectEngine) objectUpdateHandling( + ctx context.Context, + revision int64, + desiredObject Object, + actualObject Object, + options types.ObjectReconcileOptions, +) (ObjectResult, error) { + ctrlSit, compareRes, actualOwner, err := e.checkSituation( + desiredObject, actualObject, options) if err != nil { - return nil, fmt.Errorf("diverge check: %w", err) + return nil, err } // Ensure revision linearity. @@ -412,13 +450,16 @@ 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) - if err := e.ownerStrategy.SetControllerReference( - owner, desiredObject, - ); err != nil { - return nil, fmt.Errorf("set controller reference: %w", err) + if options.Owner != nil { + options.OwnerStrategy.CopyOwnerReferences(actualObject, desiredObject) + options.OwnerStrategy.ReleaseController(desiredObject) + + if err := options.OwnerStrategy.SetControllerReference( + options.Owner, desiredObject, + ); err != nil { + return nil, fmt.Errorf("set controller reference: %w", err) + } } // Write changes. @@ -443,11 +484,16 @@ func (e *ObjectEngine) objectUpdateHandling( ), nil } +// isBoxcutterManaged is used to detect if we have managed this object at some point. +// It's only purpose is to prevent boxcutter immediately re-adopting objects when +// resources get orphaned by the GC. func (e *ObjectEngine) isBoxcutterManaged(obj client.Object) bool { - for k := range obj.GetLabels() { - if strings.HasPrefix(k, boxcutterManagedLabel) { - return true - } + labels := obj.GetLabels() + annotations := obj.GetAnnotations() + + _, hasRevisionAnnotation := annotations[e.revisionAnnotation()] + if labels[managedByLabel] == managedByLabelValue && hasRevisionAnnotation { + return true } return false @@ -514,24 +560,25 @@ const ( func (e *ObjectEngine) detectOwner( owner client.Object, + ownerStrategy objectEngineOwnerStrategy, actualObject Object, previousOwners []client.Object, ) (ctrlSituation, *metav1.OwnerReference) { // e.ownerStrategy may either work on .metadata.ownerReferences or // on an annotation to allow cross-namespace and cross-cluster refs. - ownerRef, ok := e.ownerStrategy.GetController(actualObject) + ownerRef, ok := ownerStrategy.GetController(actualObject) if !ok { return ctrlSituationNoController, nil } // Are we already controller? - if e.ownerStrategy.IsController(owner, actualObject) { + if ownerStrategy.IsController(owner, actualObject) { return ctrlSituationIsController, &ownerRef } // Check if previous owner is controller. for _, previousOwner := range previousOwners { - if e.ownerStrategy.IsController(previousOwner, actualObject) { + if ownerStrategy.IsController(previousOwner, actualObject) { return ctrlSituationPreviousIsController, &ownerRef } } @@ -596,14 +643,20 @@ func (e *ObjectEngine) revisionAnnotation() string { return e.systemPrefix + "/revision" } -func removeBoxcutterManagedLabel( - ctx context.Context, w client.Writer, obj *unstructured.Unstructured, +func (e *ObjectEngine) removeBoxcutterManagedLabelsAndAnnotations( + ctx context.Context, w client.Writer, obj Object, ) error { - updated := obj.DeepCopy() + updated := obj.DeepCopyObject().(Object) + + annotations := obj.GetAnnotations() + delete(annotations, e.revisionAnnotation()) + obj.SetAnnotations(annotations) labels := updated.GetLabels() + if l, ok := labels[managedByLabel]; ok && l == managedByLabelValue { + delete(labels, managedByLabel) + } - delete(labels, boxcutterManagedLabel) updated.SetLabels(labels) if err := w.Patch(ctx, updated, client.MergeFrom(obj)); err != nil { diff --git a/machinery/objects_test.go b/machinery/objects_test.go index c5623274..e3d6c0b9 100644 --- a/machinery/objects_test.go +++ b/machinery/objects_test.go @@ -3,6 +3,7 @@ package machinery import ( "context" "errors" + "slices" "testing" "github.com/stretchr/testify/assert" @@ -10,13 +11,13 @@ import ( "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" apierrors "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/schema" "k8s.io/client-go/kubernetes/scheme" "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/structured-merge-diff/v6/fieldpath" "sigs.k8s.io/structured-merge-diff/v6/typed" @@ -29,11 +30,9 @@ const ( testSystemPrefix = "testtest.xxx" ) -//nolint:maintidx,dupl -func TestObjectEngine(t *testing.T) { - t.Parallel() - - owner := &corev1.ConfigMap{ +var ( + testOwnerStrategy = ownerhandling.NewNative(scheme.Scheme) + testOwner = &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ UID: "12345-678", Name: "owner", @@ -41,384 +40,237 @@ func TestObjectEngine(t *testing.T) { }, } - oldOwner := &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - UID: "6789", - Name: "old-owner", - Namespace: "test", + withNativeOwnerMode = nativeOwnerMode("with owner", testOwner) + + withoutOwnerMode = ownerMode{ + name: "without owner", + reconcileOpts: func() []types.ObjectReconcileOption { return nil }, + teardownOpts: func() []types.ObjectTeardownOption { return nil }, + setManaged: func(obj *unstructured.Unstructured) { + setBoxcutterManagedLabel(obj) }, } +) - tests := []struct { - name string - revision int64 - desiredObject *unstructured.Unstructured - opts []types.ObjectReconcileOption +// ownerMode captures the per-mode differences for shared reconcile and teardown test scenarios. +type ownerMode struct { + name string + reconcileOpts func() []types.ObjectReconcileOption + teardownOpts func() []types.ObjectTeardownOption + setManaged func(*unstructured.Unstructured) +} + +// nativeOwnerMode creates an ownerMode for the given owner using native owner references. +func nativeOwnerMode(name string, owner client.Object) ownerMode { + gvks, _, _ := scheme.Scheme.ObjectKinds(owner) + apiVersion, kind := gvks[0].ToAPIVersionAndKind() + + return ownerMode{ + name: name, + reconcileOpts: func() []types.ObjectReconcileOption { + return []types.ObjectReconcileOption{types.WithOwner(owner, testOwnerStrategy)} + }, + teardownOpts: func() []types.ObjectTeardownOption { + return []types.ObjectTeardownOption{types.WithOwner(owner, testOwnerStrategy)} + }, + setManaged: func(obj *unstructured.Unstructured) { + withOwnerRef(apiVersion, kind, owner.GetName(), string(owner.GetUID()), true)(obj, nil) + setBoxcutterManagedLabel(obj) + }, + } +} + +// objOption is a functional option for modifying an unstructured test object. +type objOption func(obj *unstructured.Unstructured, r *ownerMode) + +type objBuilder func(*ownerMode) *unstructured.Unstructured + +// buildObj returns a builder which creates a Secret unstructured object with the given name/namespace and options applied. +func buildObj(name, namespace string, opts ...objOption) objBuilder { //nolint:unparam + return func(mode *ownerMode) *unstructured.Unstructured { + obj := &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "Secret", + "metadata": map[string]interface{}{ + "name": name, + "namespace": namespace, + }, + }, + } + for _, opt := range opts { + opt(obj, mode) + } + + return obj + } +} + +func withRevision(rev string) objOption { + return func(obj *unstructured.Unstructured, _ *ownerMode) { + annotations := obj.GetAnnotations() + if annotations == nil { + annotations = map[string]string{} + } + + annotations[testSystemPrefix+"/revision"] = rev + obj.SetAnnotations(annotations) + } +} +func setBoxcutterManagedLabel(obj *unstructured.Unstructured) { + labels := obj.GetLabels() + if labels == nil { + labels = map[string]string{} + } + + labels[managedByLabel] = managedByLabelValue + obj.SetLabels(labels) +} + +func withManaged(obj *unstructured.Unstructured, r *ownerMode) { + r.setManaged(obj) +} + +func withOwnerRef(apiVersion, kind, name, uid string, controller bool) objOption { + return func(obj *unstructured.Unstructured, _ *ownerMode) { + md := obj.Object["metadata"].(map[string]interface{}) + refs, _ := md["ownerReferences"].([]interface{}) + refs = append(refs, map[string]interface{}{ + "apiVersion": apiVersion, + "kind": kind, + "controller": controller, + "name": name, + "uid": uid, + "blockOwnerDeletion": true, + }) + md["ownerReferences"] = refs + } +} + +func withBoxcutterManagedLabel(obj *unstructured.Unstructured, _ *ownerMode) { + setBoxcutterManagedLabel(obj) +} + +func withResourceVersion(v string) objOption { + return func(obj *unstructured.Unstructured, _ *ownerMode) { + obj.SetResourceVersion(v) + } +} + +//nolint:maintidx +func TestObjectEngine(t *testing.T) { + t.Parallel() + + defaultModes := []ownerMode{ + withNativeOwnerMode, + withoutOwnerMode, + } + + type sharedTestCase struct { + name string + revision int64 + + // desiredObject is the bare desired object (no ownerRefs, no labels). + desiredObject objBuilder + + // actualObject is the on-cluster object + // nil means NotFound (create path). + actualObject objBuilder + + // expectedObject is what we expect back + expectedObject objBuilder + + // opts are additional reconcile options to apply to the test. + opts []types.ObjectReconcileOption + + // mockSetup configures mocks. actualObject is the mode-transformed actual (nil for create). mockSetup func( - *cacheMock, - *testutil.CtrlClient, - *comparatorMock, + cache *cacheMock, + writer *testutil.CtrlClient, + ddm *comparatorMock, + actualObject *unstructured.Unstructured, ) expectedAction Action - expectedObject *unstructured.Unstructured - }{ - { - name: "Updated noController CollisionProtectionIfNoController", - revision: 1, - desiredObject: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "apiVersion": "v1", - "kind": "Secret", - "metadata": map[string]interface{}{ - "name": "testi", - "namespace": "test", - }, - }, - }, - opts: []types.ObjectReconcileOption{ - types.WithCollisionProtection(types.CollisionProtectionIfNoController), - }, - mockSetup: func( - cache *cacheMock, writer *testutil.CtrlClient, - ddm *comparatorMock, - ) { - actualObject := &unstructured.Unstructured{ - Object: map[string]interface{}{ - "apiVersion": "v1", - "kind": "Secret", - "metadata": map[string]interface{}{ - "name": "testi", - "namespace": "test", - }, - }, - } + modes []ownerMode // nil = both default modes - // Mock setup - cache. - On( - "Get", mock.Anything, - client.ObjectKeyFromObject(actualObject), - mock.Anything, mock.Anything, - ). - Run(func(args mock.Arguments) { - obj := args.Get(2).(*unstructured.Unstructured) - *obj = *actualObject - }). - Return(nil) - ddm. - On("Compare", owner, mock.Anything, mock.Anything). - Return(CompareResult{}, nil) + // expectedError, if set, means Reconcile should return an error containing this substring. + // expectedAction and expectedObject are ignored when set. + expectedError string - writer. - On("Apply", mock.Anything, mock.Anything, mock.Anything, mock.Anything). - Return(nil) - }, + // afterAssert, if set, runs additional assertions after the standard checks. + afterAssert func(t *testing.T, writer *testutil.CtrlClient) + } - expectedAction: ActionUpdated, - expectedObject: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "apiVersion": "v1", - "kind": "Secret", - "metadata": map[string]interface{}{ - "name": "testi", - "namespace": "test", - "ownerReferences": []interface{}{ - map[string]interface{}{ - "apiVersion": "v1", - "kind": "ConfigMap", - "controller": true, - "name": "owner", - "uid": "12345-678", - "blockOwnerDeletion": true, - }, - }, - "annotations": map[string]interface{}{ - "testtest.xxx/revision": "1", - }, - "labels": map[string]interface{}{ - "boxcutter-managed": "True", - }, - }, - }, - }, - }, + sharedTests := []sharedTestCase{ { - name: "Created", - revision: 1, - desiredObject: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "apiVersion": "v1", - "kind": "Secret", - "metadata": map[string]interface{}{ - "name": "testi", - "namespace": "test", - }, - }, - }, - + name: "Created", + revision: 1, + desiredObject: buildObj("testi", "test"), + actualObject: nil, // NotFound + expectedObject: buildObj("testi", "test", withRevision("1"), withManaged), mockSetup: func( cache *cacheMock, writer *testutil.CtrlClient, - ddm *comparatorMock, + ddm *comparatorMock, _ *unstructured.Unstructured, ) { - // Mock setup cache. - On( - "Get", mock.Anything, - client.ObjectKey{ - Name: "testi", - Namespace: "test", - }, - mock.Anything, mock.Anything, - ). + On("Get", mock.Anything, + client.ObjectKey{Name: "testi", Namespace: "test"}, + mock.Anything, mock.Anything). 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. On("Create", mock.Anything, mock.Anything, mock.Anything). Return(nil) }, - expectedAction: ActionCreated, - expectedObject: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "apiVersion": "v1", - "kind": "Secret", - "metadata": map[string]interface{}{ - "name": "testi", - "namespace": "test", - "ownerReferences": []interface{}{ - map[string]interface{}{ - "apiVersion": "v1", - "kind": "ConfigMap", - "controller": true, - "name": "owner", - "uid": "12345-678", - "blockOwnerDeletion": true, - }, - }, - "annotations": map[string]interface{}{ - "testtest.xxx/revision": "1", - }, - "labels": map[string]interface{}{ - "boxcutter-managed": "True", - }, - }, - }, - }, - }, - { - name: "Progressed", - revision: 1, - desiredObject: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "apiVersion": "v1", - "kind": "Secret", - "metadata": map[string]interface{}{ - "name": "testi", - "namespace": "test", - }, - }, - }, - opts: []types.ObjectReconcileOption{}, - - mockSetup: func( - cache *cacheMock, writer *testutil.CtrlClient, - ddm *comparatorMock, - ) { - actualObject := &unstructured.Unstructured{ - Object: map[string]interface{}{ - "apiVersion": "v1", - "kind": "Secret", - "metadata": map[string]interface{}{ - "name": "testi", - "namespace": "test", - "annotations": map[string]interface{}{ - "testtest.xxx/revision": "4", - }, - }, - }, - } - - // Mock setup - cache. - On( - "Get", mock.Anything, - client.ObjectKeyFromObject(actualObject), - mock.Anything, mock.Anything, - ). - Run(func(args mock.Arguments) { - obj := args.Get(2).(*unstructured.Unstructured) - *obj = *actualObject - }). - Return(nil) - ddm. - On("Compare", owner, mock.Anything, mock.Anything). - Return(CompareResult{}, nil) - - writer. - On("Patch", mock.Anything, mock.Anything, mock.Anything, mock.Anything). - Return(nil) - }, - - expectedAction: ActionProgressed, - expectedObject: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "apiVersion": "v1", - "kind": "Secret", - "metadata": map[string]interface{}{ - "name": "testi", - "namespace": "test", - "annotations": map[string]interface{}{ - "testtest.xxx/revision": "4", - }, - }, - }, - }, }, - { - name: "Idle", - revision: 1, - desiredObject: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "apiVersion": "v1", - "kind": "Secret", - "metadata": map[string]interface{}{ - "name": "testi", - "namespace": "test", - }, - }, - }, - opts: []types.ObjectReconcileOption{}, - + { //nolint:dupl + name: "Idle", + revision: 1, + desiredObject: buildObj("testi", "test"), + actualObject: buildObj("testi", "test", withRevision("1"), withManaged), + expectedObject: buildObj("testi", "test", withRevision("1"), withManaged), mockSetup: func( cache *cacheMock, writer *testutil.CtrlClient, - ddm *comparatorMock, + ddm *comparatorMock, actualObject *unstructured.Unstructured, ) { - actualObject := &unstructured.Unstructured{ - Object: map[string]interface{}{ - "apiVersion": "v1", - "kind": "Secret", - "metadata": map[string]interface{}{ - "name": "testi", - "namespace": "test", - "annotations": map[string]interface{}{ - "testtest.xxx/revision": "1", - }, - "ownerReferences": []interface{}{ - map[string]interface{}{ - "apiVersion": "v1", - "kind": "ConfigMap", - "controller": true, - "name": "owner", - "uid": "12345-678", - "blockOwnerDeletion": true, - }, - }, - }, - }, - } - - // Mock setup cache. - On( - "Get", mock.Anything, + On("Get", mock.Anything, client.ObjectKeyFromObject(actualObject), - mock.Anything, mock.Anything, - ). + mock.Anything, mock.Anything). Run(func(args mock.Arguments) { obj := args.Get(2).(*unstructured.Unstructured) *obj = *actualObject }). Return(nil) ddm. - On("Compare", owner, mock.Anything, mock.Anything). + On("Compare", mock.Anything, mock.Anything, mock.Anything). Return(CompareResult{}, nil) - writer. On("Patch", mock.Anything, mock.Anything, mock.Anything, mock.Anything). Return(nil) }, - expectedAction: ActionIdle, - expectedObject: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "apiVersion": "v1", - "kind": "Secret", - "metadata": map[string]interface{}{ - "name": "testi", - "namespace": "test", - "annotations": map[string]interface{}{ - "testtest.xxx/revision": "1", - }, - "ownerReferences": []interface{}{ - map[string]interface{}{ - "apiVersion": "v1", - "kind": "ConfigMap", - "controller": true, - "name": "owner", - "uid": "12345-678", - "blockOwnerDeletion": true, - }, - }, - }, - }, - }, }, { - name: "Updated - modified", - revision: 1, - desiredObject: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "apiVersion": "v1", - "kind": "Secret", - "metadata": map[string]interface{}{ - "name": "testi", - "namespace": "test", - }, - }, - }, - opts: []types.ObjectReconcileOption{}, - + name: "Updated - modified", + revision: 1, + desiredObject: buildObj("testi", "test"), + actualObject: buildObj("testi", "test", withRevision("1"), withManaged), + expectedObject: buildObj("testi", "test", withRevision("1"), withManaged), mockSetup: func( cache *cacheMock, writer *testutil.CtrlClient, - ddm *comparatorMock, + ddm *comparatorMock, actualObject *unstructured.Unstructured, ) { - actualObject := &unstructured.Unstructured{ - Object: map[string]interface{}{ - "apiVersion": "v1", - "kind": "Secret", - "metadata": map[string]interface{}{ - "name": "testi", - "namespace": "test", - "annotations": map[string]interface{}{ - "testtest.xxx/revision": "1", - }, - "ownerReferences": []interface{}{ - map[string]interface{}{ - "apiVersion": "v1", - "kind": "ConfigMap", - "controller": true, - "name": "owner", - "uid": "12345-678", - "blockOwnerDeletion": true, - }, - }, - }, - }, - } - - // Mock setup cache. - On( - "Get", mock.Anything, + On("Get", mock.Anything, client.ObjectKeyFromObject(actualObject), - mock.Anything, mock.Anything, - ). + mock.Anything, mock.Anything). Run(func(args mock.Arguments) { obj := args.Get(2).(*unstructured.Unstructured) *obj = *actualObject @@ -428,7 +280,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{}, @@ -436,451 +288,533 @@ func TestObjectEngine(t *testing.T) { Modified: fs, }, }, nil) - writer. On("Apply", mock.Anything, mock.Anything, mock.Anything, mock.Anything). Return(nil) }, - expectedAction: ActionUpdated, - expectedObject: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "apiVersion": "v1", - "kind": "Secret", - "metadata": map[string]interface{}{ - "name": "testi", - "namespace": "test", - "annotations": map[string]interface{}{ - "testtest.xxx/revision": "1", - }, - "labels": map[string]interface{}{ - "boxcutter-managed": "True", - }, - "ownerReferences": []interface{}{ - map[string]interface{}{ - "apiVersion": "v1", - "kind": "ConfigMap", - "controller": true, - "name": "owner", - "uid": "12345-678", - "blockOwnerDeletion": true, - }, - }, - }, - }, - }, }, { - name: "Recovered", - revision: 1, - desiredObject: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "apiVersion": "v1", - "kind": "Secret", - "metadata": map[string]interface{}{ - "name": "testi", - "namespace": "test", - }, - }, - }, - opts: []types.ObjectReconcileOption{}, - + name: "Recovered", + revision: 1, + desiredObject: buildObj("testi", "test"), + actualObject: buildObj("testi", "test", withRevision("1"), withManaged), + expectedObject: buildObj("testi", "test", withRevision("1"), withManaged), mockSetup: func( cache *cacheMock, writer *testutil.CtrlClient, - ddm *comparatorMock, + ddm *comparatorMock, actualObject *unstructured.Unstructured, ) { - actualObject := &unstructured.Unstructured{ - Object: map[string]interface{}{ - "apiVersion": "v1", - "kind": "Secret", - "metadata": map[string]interface{}{ - "name": "testi", - "namespace": "test", - "annotations": map[string]interface{}{ - "testtest.xxx/revision": "1", - }, - "ownerReferences": []interface{}{ - map[string]interface{}{ - "apiVersion": "v1", - "kind": "ConfigMap", - "controller": true, - "name": "owner", - "uid": "12345-678", - "blockOwnerDeletion": true, - }, - }, - }, - }, - } - - // Mock setup cache. - On( - "Get", mock.Anything, + On("Get", mock.Anything, client.ObjectKeyFromObject(actualObject), - mock.Anything, mock.Anything, - ). + mock.Anything, mock.Anything). Run(func(args mock.Arguments) { obj := args.Get(2).(*unstructured.Unstructured) *obj = *actualObject }). Return(nil) - - 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"}, }, }, nil) - writer. On("Apply", mock.Anything, mock.Anything, mock.Anything, mock.Anything). Return(nil) }, - expectedAction: ActionRecovered, - expectedObject: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "apiVersion": "v1", - "kind": "Secret", - "metadata": map[string]interface{}{ - "name": "testi", - "namespace": "test", - "annotations": map[string]interface{}{ - "testtest.xxx/revision": "1", - }, - "labels": map[string]interface{}{ - "boxcutter-managed": "True", - }, - "ownerReferences": []interface{}{ - map[string]interface{}{ - "apiVersion": "v1", - "kind": "ConfigMap", - "controller": true, - "name": "owner", - "uid": "12345-678", - "blockOwnerDeletion": true, - }, - }, - }, - }, - }, }, - { - name: "Collision - unknown controller", - revision: 1, - desiredObject: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "apiVersion": "v1", - "kind": "Secret", - "metadata": map[string]interface{}{ - "name": "testi", - "namespace": "test", - }, - }, - }, - opts: []types.ObjectReconcileOption{}, - + { //nolint:dupl + name: "Progressed", + revision: 1, + desiredObject: buildObj("testi", "test"), + actualObject: buildObj("testi", "test", withRevision("4"), withManaged), + expectedObject: buildObj("testi", "test", withRevision("4"), withManaged), mockSetup: func( cache *cacheMock, writer *testutil.CtrlClient, - ddm *comparatorMock, + ddm *comparatorMock, actualObject *unstructured.Unstructured, ) { - actualObject := &unstructured.Unstructured{ - Object: map[string]interface{}{ - "apiVersion": "v1", - "kind": "Secret", - "metadata": map[string]interface{}{ - "name": "testi", - "namespace": "test", - "annotations": map[string]interface{}{ - "testtest.xxx/revision": "1", - }, - "ownerReferences": []interface{}{ - map[string]interface{}{ - "apiVersion": "v1", - "kind": "Node", - "controller": true, - "name": "node1", - "uid": "xxxx", - "blockOwnerDeletion": true, - }, - }, - }, - }, - } - - // Mock setup cache. - On( - "Get", mock.Anything, + On("Get", mock.Anything, client.ObjectKeyFromObject(actualObject), - mock.Anything, mock.Anything, - ). + mock.Anything, mock.Anything). Run(func(args mock.Arguments) { obj := args.Get(2).(*unstructured.Unstructured) *obj = *actualObject }). Return(nil) - - 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. On("Patch", mock.Anything, mock.Anything, mock.Anything, mock.Anything). Return(nil) }, - - expectedAction: ActionCollision, - expectedObject: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "apiVersion": "v1", - "kind": "Secret", - "metadata": map[string]interface{}{ - "name": "testi", - "namespace": "test", - "annotations": map[string]interface{}{ - "testtest.xxx/revision": "1", - }, - "ownerReferences": []interface{}{ - map[string]interface{}{ - "apiVersion": "v1", - "kind": "Node", - "controller": true, - "name": "node1", - "uid": "xxxx", - "blockOwnerDeletion": true, - }, - }, - }, - }, - }, + expectedAction: ActionProgressed, }, { - name: "Updated takeover from previousOwner", - revision: 1, - desiredObject: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "apiVersion": "v1", - "kind": "Secret", - "metadata": map[string]interface{}{ - "name": "testi", - "namespace": "test", - }, - }, - }, + name: "Updated noController CollisionProtectionIfNoController", + revision: 1, + desiredObject: buildObj("testi", "test"), + actualObject: buildObj("testi", "test"), + expectedObject: buildObj("testi", "test", withRevision("1"), withManaged), opts: []types.ObjectReconcileOption{ - types.WithPreviousOwners{oldOwner}, + types.WithCollisionProtection(types.CollisionProtectionIfNoController), }, - mockSetup: func( cache *cacheMock, writer *testutil.CtrlClient, - ddm *comparatorMock, + ddm *comparatorMock, actualObject *unstructured.Unstructured, ) { - actualObject := &unstructured.Unstructured{ - Object: map[string]interface{}{ - "apiVersion": "v1", - "kind": "Secret", - "metadata": map[string]interface{}{ - "name": "testi", - "namespace": "test", - "annotations": map[string]interface{}{ - "testtest.xxx/revision": "1", - }, - "ownerReferences": []interface{}{ - map[string]interface{}{ - "apiVersion": "v1", - "kind": "ConfigMap", - "controller": true, - "name": "old-owner", - "uid": "6789", - "blockOwnerDeletion": true, - }, - }, - }, - }, - } - - // Mock setup cache. - On( - "Get", mock.Anything, + On("Get", mock.Anything, client.ObjectKeyFromObject(actualObject), - mock.Anything, mock.Anything, - ). + mock.Anything, mock.Anything). Run(func(args mock.Arguments) { obj := args.Get(2).(*unstructured.Unstructured) *obj = *actualObject }). Return(nil) - - 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. On("Apply", mock.Anything, mock.Anything, mock.Anything, mock.Anything). Return(nil) }, - expectedAction: ActionUpdated, - expectedObject: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "apiVersion": "v1", - "kind": "Secret", - "metadata": map[string]interface{}{ - "name": "testi", - "namespace": "test", - "annotations": map[string]interface{}{ - "testtest.xxx/revision": "1", - }, - "labels": map[string]interface{}{ - "boxcutter-managed": "True", - }, - "ownerReferences": []interface{}{ - map[string]interface{}{ - "apiVersion": "v1", - "kind": "ConfigMap", - "controller": false, - "name": "old-owner", - "uid": "6789", - "blockOwnerDeletion": true, - }, - map[string]interface{}{ - "apiVersion": "v1", - "kind": "ConfigMap", - "controller": true, - "name": "owner", - "uid": "12345-678", - "blockOwnerDeletion": true, - }, - }, - }, - }, - }, }, { - name: "Collision - no controller", - revision: 1, - desiredObject: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "apiVersion": "v1", - "kind": "Secret", - "metadata": map[string]interface{}{ - "name": "testi", - "namespace": "test", - }, - }, - }, - + name: "Collision - no controller", + revision: 1, + desiredObject: buildObj("testi", "test"), + actualObject: buildObj("testi", "test"), + expectedObject: buildObj("testi", "test"), mockSetup: func( cache *cacheMock, writer *testutil.CtrlClient, - ddm *comparatorMock, + ddm *comparatorMock, actualObject *unstructured.Unstructured, ) { - actualObject := &unstructured.Unstructured{ - Object: map[string]interface{}{ - "apiVersion": "v1", - "kind": "Secret", - "metadata": map[string]interface{}{ - "name": "testi", - "namespace": "test", - "annotations": map[string]interface{}{ - "testtest.xxx/revision": "1", - }, - }, - }, - } - - // Mock setup cache. - On( - "Get", mock.Anything, + On("Get", mock.Anything, client.ObjectKeyFromObject(actualObject), - mock.Anything, mock.Anything, - ). + mock.Anything, mock.Anything). Run(func(args mock.Arguments) { obj := args.Get(2).(*unstructured.Unstructured) *obj = *actualObject }). Return(nil) - - 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. On("Patch", mock.Anything, mock.Anything, mock.Anything, mock.Anything). Return(nil) }, - expectedAction: ActionCollision, - expectedObject: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "apiVersion": "v1", - "kind": "Secret", - "metadata": map[string]interface{}{ - "name": "testi", - "namespace": "test", - "annotations": map[string]interface{}{ - "testtest.xxx/revision": "1", - }, - }, - }, - }, }, - } - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - t.Parallel() - - cache := &cacheMock{} - writer := testutil.NewClient() - ownerStrategy := ownerhandling.NewNative(scheme.Scheme) - divergeDetector := &comparatorMock{} - - oe := NewObjectEngine( - scheme.Scheme, - cache, writer, - ownerStrategy, divergeDetector, - testFieldOwner, - testSystemPrefix, - ) - - test.mockSetup(cache, writer, divergeDetector) - - ctx := context.Background() - res, err := oe.Reconcile( - ctx, owner, 1, test.desiredObject, - test.opts..., - ) - require.NoError(t, err) - - switch r := res.(type) { - case ObjectResultCreated: - assert.Equal(t, test.expectedObject, r.Object()) - case ObjectResultUpdated: - assert.Equal(t, test.expectedObject, r.Object()) - case ObjectResultIdle: - assert.Equal(t, test.expectedObject, r.Object()) - case ObjectResultProgressed: - assert.Equal(t, test.expectedObject, r.Object()) - case ObjectResultRecovered: - assert.Equal(t, test.expectedObject, r.Object()) - } - - assert.Equal(t, test.expectedAction, res.Action()) - }) - } -} + { + name: "comparison error", + revision: 1, + desiredObject: buildObj("testi", "test"), + actualObject: buildObj("testi", "test"), + mockSetup: func( + cache *cacheMock, _ *testutil.CtrlClient, + ddm *comparatorMock, actualObject *unstructured.Unstructured, + ) { + cache. + On("Get", mock.Anything, + client.ObjectKeyFromObject(actualObject), + mock.Anything, mock.Anything). + Run(func(args mock.Arguments) { + obj := args.Get(2).(*unstructured.Unstructured) + *obj = *actualObject + }). + Return(nil) + ddm. + On("Compare", mock.Anything, mock.Anything, mock.Anything). + Return(CompareResult{}, errors.New("comparison failed")) + }, + expectedError: "diverge check", + }, + { + name: "Updated takeover from previousOwner", + revision: 1, + desiredObject: buildObj("testi", "test"), + actualObject: buildObj("testi", "test", withRevision("1"), + withOwnerRef("v1", "ConfigMap", "old-owner", "6789", true)), + expectedObject: buildObj("testi", "test", withRevision("1"), + withOwnerRef("v1", "ConfigMap", "old-owner", "6789", false), withManaged), + modes: []ownerMode{withNativeOwnerMode}, + opts: []types.ObjectReconcileOption{ + types.WithPreviousOwners{&corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + UID: "6789", Name: "old-owner", Namespace: "test", + }, + }}, + }, + mockSetup: func( + cache *cacheMock, writer *testutil.CtrlClient, + ddm *comparatorMock, actualObject *unstructured.Unstructured, + ) { + cache. + On("Get", mock.Anything, + client.ObjectKeyFromObject(actualObject), + mock.Anything, mock.Anything). + Run(func(args mock.Arguments) { + obj := args.Get(2).(*unstructured.Unstructured) + *obj = *actualObject + }). + Return(nil) + ddm. + On("Compare", mock.Anything, mock.Anything, mock.Anything). + Return(CompareResult{}, nil) + writer. + On("Apply", mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Return(nil) + }, + expectedAction: ActionUpdated, + }, + { + name: "Collision - unknown controller", + revision: 1, + desiredObject: buildObj("testi", "test"), + actualObject: buildObj("testi", "test", withRevision("1"), + withOwnerRef("v1", "Node", "node1", "xxxx", true)), + expectedObject: buildObj("testi", "test", withRevision("1"), + withOwnerRef("v1", "Node", "node1", "xxxx", true)), + modes: []ownerMode{withNativeOwnerMode}, + mockSetup: func( + cache *cacheMock, writer *testutil.CtrlClient, + ddm *comparatorMock, actualObject *unstructured.Unstructured, + ) { + cache. + On("Get", mock.Anything, + client.ObjectKeyFromObject(actualObject), + mock.Anything, mock.Anything). + Run(func(args mock.Arguments) { + obj := args.Get(2).(*unstructured.Unstructured) + *obj = *actualObject + }). + Return(nil) + ddm. + On("Compare", mock.Anything, mock.Anything, mock.Anything). + Return(CompareResult{}, nil) + writer. + On("Patch", mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Return(nil) + }, + expectedAction: ActionCollision, + }, + { + name: "Get error", + revision: 1, + desiredObject: buildObj("testi", "test"), + mockSetup: func( + cache *cacheMock, _ *testutil.CtrlClient, + _ *comparatorMock, _ *unstructured.Unstructured, + ) { + cache. + On("Get", mock.Anything, + client.ObjectKey{Name: "testi", Namespace: "test"}, + mock.Anything, mock.Anything). + Return(errors.New("cache error")) + }, + expectedError: "getting object", + }, + { + name: "Updated, CollisionProtectionNone, unknown controller", + revision: 1, + desiredObject: buildObj("testi", "test"), + actualObject: buildObj("testi", "test", withRevision("1"), + withOwnerRef("v1", "Node", "node1", "xxxx", true)), + modes: []ownerMode{withNativeOwnerMode}, + opts: []types.ObjectReconcileOption{ + types.WithCollisionProtection(types.CollisionProtectionNone), + }, + mockSetup: func( + cache *cacheMock, writer *testutil.CtrlClient, + ddm *comparatorMock, actualObject *unstructured.Unstructured, + ) { + cache. + On("Get", mock.Anything, + client.ObjectKeyFromObject(actualObject), + mock.Anything, mock.Anything). + Run(func(args mock.Arguments) { + obj := args.Get(2).(*unstructured.Unstructured) + *obj = *actualObject + }). + Return(nil) + ddm. + On("Compare", mock.Anything, mock.Anything, mock.Anything). + Return(CompareResult{}, nil) + writer. + On("Apply", mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Return(nil) + }, + expectedAction: ActionUpdated, + }, + { + name: "Collision, CollisionProtectionNone, boxcutter managed", + revision: 1, + desiredObject: buildObj("testi", "test"), + actualObject: buildObj("testi", "test", withBoxcutterManagedLabel, withRevision("1")), + modes: []ownerMode{withNativeOwnerMode}, + opts: []types.ObjectReconcileOption{ + types.WithCollisionProtection(types.CollisionProtectionNone), + }, + mockSetup: func( + cache *cacheMock, _ *testutil.CtrlClient, + ddm *comparatorMock, actualObject *unstructured.Unstructured, + ) { + cache. + On("Get", mock.Anything, + client.ObjectKeyFromObject(actualObject), + mock.Anything, mock.Anything). + Run(func(args mock.Arguments) { + obj := args.Get(2).(*unstructured.Unstructured) + *obj = *actualObject + }). + Return(nil) + ddm. + On("Compare", mock.Anything, mock.Anything, mock.Anything). + Return(CompareResult{}, nil) + }, + expectedAction: ActionCollision, + }, + { + name: "Updated, CollisionProtectionNone", + revision: 1, + desiredObject: buildObj("testi", "test"), + actualObject: buildObj("testi", "test", withRevision("1")), + expectedObject: buildObj("testi", "test", withRevision("1"), withManaged), + opts: []types.ObjectReconcileOption{ + types.WithCollisionProtection(types.CollisionProtectionNone), + }, + mockSetup: func( + cache *cacheMock, writer *testutil.CtrlClient, + ddm *comparatorMock, actualObject *unstructured.Unstructured, + ) { + cache. + On("Get", mock.Anything, + client.ObjectKeyFromObject(actualObject), + mock.Anything, mock.Anything). + Run(func(args mock.Arguments) { + obj := args.Get(2).(*unstructured.Unstructured) + *obj = *actualObject + }). + Return(nil) + ddm. + On("Compare", mock.Anything, mock.Anything, mock.Anything). + Return(CompareResult{}, nil) + writer. + On("Apply", mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Return(nil) + }, + expectedAction: ActionUpdated, + }, + { + name: "Paused, create skipped", + revision: 1, + desiredObject: buildObj("testi", "test"), + opts: []types.ObjectReconcileOption{ + types.WithPaused{}, + }, + mockSetup: func( + cache *cacheMock, _ *testutil.CtrlClient, + _ *comparatorMock, _ *unstructured.Unstructured, + ) { + cache. + On("Get", mock.Anything, + client.ObjectKey{Name: "testi", Namespace: "test"}, + mock.Anything, mock.Anything). + Return(apierrors.NewNotFound(schema.GroupResource{}, "")) + }, + expectedAction: ActionCreated, + afterAssert: func(t *testing.T, writer *testutil.CtrlClient) { + t.Helper() + writer.AssertNotCalled(t, "Create") + }, + }, + { + name: "Paused, apply skipped", + revision: 1, + desiredObject: buildObj("testi", "test"), + actualObject: buildObj("testi", "test", withRevision("1"), withManaged), + opts: []types.ObjectReconcileOption{ + types.WithPaused{}, + }, + mockSetup: func( + cache *cacheMock, _ *testutil.CtrlClient, + ddm *comparatorMock, actualObject *unstructured.Unstructured, + ) { + cache. + On("Get", mock.Anything, + client.ObjectKeyFromObject(actualObject), + mock.Anything, mock.Anything). + Run(func(args mock.Arguments) { + obj := args.Get(2).(*unstructured.Unstructured) + *obj = *actualObject + }). + Return(nil) + + fs := &fieldpath.Set{} + fs.Insert(fieldpath.MakePathOrDie("spec", "data")) + ddm. + On("Compare", mock.Anything, mock.Anything, mock.Anything). + Return(CompareResult{ + Comparison: &typed.Comparison{ + Added: &fieldpath.Set{}, + Removed: &fieldpath.Set{}, + Modified: fs, + }, + }, nil) + }, + expectedAction: ActionUpdated, + afterAssert: func(t *testing.T, writer *testutil.CtrlClient) { + t.Helper() + writer.AssertNotCalled(t, "Apply") + }, + }, + { + name: "Paused, recovered returns actual", + revision: 1, + desiredObject: buildObj("testi", "test"), + actualObject: buildObj("testi", "test", withRevision("1"), + withResourceVersion("999"), withManaged), + expectedObject: buildObj("testi", "test", withRevision("1"), + withResourceVersion("999"), withManaged), + opts: []types.ObjectReconcileOption{ + types.WithPaused{}, + }, + mockSetup: func( + cache *cacheMock, _ *testutil.CtrlClient, + ddm *comparatorMock, actualObject *unstructured.Unstructured, + ) { + cache. + On("Get", mock.Anything, + client.ObjectKeyFromObject(actualObject), + mock.Anything, mock.Anything). + Run(func(args mock.Arguments) { + obj := args.Get(2).(*unstructured.Unstructured) + *obj = *actualObject + }). + Return(nil) + ddm. + On("Compare", mock.Anything, mock.Anything, mock.Anything). + Return(CompareResult{ + ConflictingMangers: []CompareResultManagedFields{ + {Manager: "some-other-manager"}, + }, + }, nil) + }, + expectedAction: ActionRecovered, + afterAssert: func(t *testing.T, writer *testutil.CtrlClient) { + t.Helper() + writer.AssertNotCalled(t, "Apply") + }, + }, + { + name: "Paused, takeover returns actual", + revision: 1, + desiredObject: buildObj("testi", "test"), + actualObject: buildObj("testi", "test", withRevision("1"), + withResourceVersion("888"), + withOwnerRef("v1", "ConfigMap", "old-owner", "6789", true)), + expectedObject: buildObj("testi", "test", withRevision("1"), + withResourceVersion("888"), + withOwnerRef("v1", "ConfigMap", "old-owner", "6789", true)), + modes: []ownerMode{withNativeOwnerMode}, + opts: []types.ObjectReconcileOption{ + types.WithPaused{}, + types.WithPreviousOwners{&corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + UID: "6789", Name: "old-owner", Namespace: "test", + }, + }}, + }, + mockSetup: func( + cache *cacheMock, _ *testutil.CtrlClient, + ddm *comparatorMock, actualObject *unstructured.Unstructured, + ) { + cache. + On("Get", mock.Anything, + client.ObjectKeyFromObject(actualObject), + mock.Anything, mock.Anything). + Run(func(args mock.Arguments) { + obj := args.Get(2).(*unstructured.Unstructured) + *obj = *actualObject + }). + Return(nil) + ddm. + On("Compare", mock.Anything, mock.Anything, mock.Anything). + Return(CompareResult{}, nil) + }, + expectedAction: ActionUpdated, + afterAssert: func(t *testing.T, writer *testutil.CtrlClient) { + t.Helper() + writer.AssertNotCalled(t, "Apply") + }, + }, + } + + for _, tc := range sharedTests { + modes := tc.modes + if len(modes) == 0 { + modes = defaultModes + } + + for _, mode := range modes { + t.Run(tc.name+"/"+mode.name, func(t *testing.T) { + t.Parallel() + + cache := &cacheMock{} + writer := testutil.NewClient() + divergeDetector := &comparatorMock{} + + oe := NewObjectEngine( + scheme.Scheme, + cache, writer, + divergeDetector, + testFieldOwner, + testSystemPrefix, + ) + + desiredObject := tc.desiredObject(&mode) + + var actualObject *unstructured.Unstructured + if tc.actualObject != nil { + actualObject = tc.actualObject(&mode) + } + + tc.mockSetup(cache, writer, divergeDetector, actualObject) + + res, err := oe.Reconcile( + t.Context(), tc.revision, desiredObject, + slices.Concat(mode.reconcileOpts(), tc.opts)..., + ) + + if tc.expectedObject != nil { + expectedObject := tc.expectedObject(&mode) + assert.Equal(t, expectedObject, res.Object()) + } + + if tc.expectedError == "" { + require.NoError(t, err) + assert.Equal(t, tc.expectedAction, res.Action()) + } else { + require.Error(t, err) + assert.Contains(t, err.Error(), tc.expectedError) + assert.Nil(t, res) + } + + if tc.afterAssert != nil { + tc.afterAssert(t, writer) + } + }) + } + } +} func TestObjectEngine_Reconcile_SanityChecks(t *testing.T) { t.Parallel() @@ -892,14 +826,14 @@ func TestObjectEngine_Reconcile_SanityChecks(t *testing.T) { t.Run("missing revision", func(t *testing.T) { t.Parallel() assert.PanicsWithValue(t, "owner revision must be set and start at 1", func() { - _, _ = oe.Reconcile(t.Context(), owner, 0, desired) + _, _ = oe.Reconcile(t.Context(), 0, desired) }) }) t.Run("missing owner.UID", 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) + assert.PanicsWithValue(t, "owner must be persisted to cluster, empty UID", func() { + _, _ = oe.Reconcile(t.Context(), 1, desired, types.WithOwner(owner, nil)) }) }) } @@ -926,7 +860,7 @@ func TestObjectEngine_Reconcile_UnsupportedTypedObject(t *testing.T) { oe := NewObjectEngine( scheme.Scheme, cache, writer, - ownerStrategy, divergeDetector, + divergeDetector, testFieldOwner, testSystemPrefix, ) @@ -976,7 +910,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", mock.Anything, mock.Anything, mock.Anything). Return(CompareResult{ Comparison: &typed.Comparison{ Added: &fieldpath.Set{}, @@ -986,7 +920,7 @@ func TestObjectEngine_Reconcile_UnsupportedTypedObject(t *testing.T) { }, nil) ctx := context.Background() - res, err := oe.Reconcile(ctx, owner, 1, desiredObject) + res, err := oe.Reconcile(ctx, 1, desiredObject, types.WithOwner(owner, ownerStrategy)) // Should return UnsupportedApplyConfigurationError require.Error(t, err) @@ -999,387 +933,515 @@ func TestObjectEngine_Reconcile_UnsupportedTypedObject(t *testing.T) { }) } -func TestObjectEngine_TeardownWithTeardownWriter(t *testing.T) { +//nolint:maintidx +func TestObjectEngine_Teardown(t *testing.T) { t.Parallel() - tests := []struct { - name string + defaultModes := []ownerMode{ + withNativeOwnerMode, + withoutOwnerMode, + } - mockSetup func( - *testutil.CtrlClient, + sharedTests := []struct { + name string + revision int64 + desiredObject objBuilder + actualObject objBuilder // nil means object does not exist on cluster + opts func() []types.ObjectTeardownOption + mockSetup func( + cache *cacheMock, + writer *testutil.CtrlClient, + actualObject *unstructured.Unstructured, ) - - expectedResult bool - expectedError error + modes []ownerMode // nil = both default modes + expectedGone bool + expectedError string }{ { - name: "deletes with teardown client", - mockSetup: func(teardownWriter *testutil.CtrlClient) { - teardownWriter. - On("Delete", mock.Anything, mock.Anything, mock.Anything, mock.Anything). + name: "Orphan", + revision: 1, + desiredObject: buildObj("testi", "test"), + actualObject: buildObj("testi", "test", withRevision("1"), withManaged), + opts: func() []types.ObjectTeardownOption { + return []types.ObjectTeardownOption{types.WithOrphan()} + }, + mockSetup: func( + cache *cacheMock, writer *testutil.CtrlClient, + actualObject *unstructured.Unstructured, + ) { + writer.On("Patch", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil) + cache. + On("Get", mock.Anything, + client.ObjectKey{Name: "testi", Namespace: "test"}, + mock.Anything, mock.Anything). + Run(func(args mock.Arguments) { + obj := args.Get(2).(*unstructured.Unstructured) + *obj = *actualObject + }). Return(nil) }, - expectedResult: false, + expectedGone: true, }, { - name: "returns teardown client errors", - mockSetup: func(teardownWriter *testutil.CtrlClient) { - teardownWriter. - On("Delete", mock.Anything, mock.Anything, mock.Anything, mock.Anything). - Return(errors.New("test error")) + name: "NotFound on Get", + revision: 1, + desiredObject: buildObj("testi", "test"), + mockSetup: func( + cache *cacheMock, _ *testutil.CtrlClient, + _ *unstructured.Unstructured, + ) { + cache. + On("Get", mock.Anything, + client.ObjectKey{Name: "testi", Namespace: "test"}, + mock.Anything, mock.Anything). + Return(apierrors.NewNotFound(schema.GroupResource{}, "")) }, - expectedError: errors.New("deleting object: test error"), - expectedResult: false, + expectedGone: true, }, - } - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - t.Parallel() - - owner := &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - UID: "12345-678-91011", - Name: "some-owner", - Namespace: "test-namespace", - }, - } - - obj := &unstructured.Unstructured{ - Object: map[string]interface{}{ - "apiVersion": "v1", - "kind": "Secret", - "metadata": map[string]interface{}{ - "name": "test-package", - "namespace": "test-namespace", - "annotations": map[string]interface{}{ - "testtest.xxx/revision": "1", + { + name: "NoMatchError on Get", + revision: 1, + desiredObject: buildObj("testi", "test"), + mockSetup: func( + cache *cacheMock, _ *testutil.CtrlClient, + _ *unstructured.Unstructured, + ) { + cache. + On("Get", mock.Anything, + client.ObjectKey{Name: "testi", Namespace: "test"}, + mock.Anything, mock.Anything). + Return(&meta.NoResourceMatchError{ + PartialResource: schema.GroupVersionResource{ + Group: "", Version: "v1", Resource: "secrets", }, - }, - }, - } - err := controllerutil.SetControllerReference(owner, obj, scheme.Scheme) - require.NoError(t, err) - - cache := &cacheMock{} - engineWriter := testutil.NewClient() // default engine writer - teardownWriter := testutil.NewClient() // used during tearDown - ownerStrategy := ownerhandling.NewNative(scheme.Scheme) - divergeDetector := &comparatorMock{} - - cache. - On("Watch", mock.Anything, mock.Anything, mock.Anything). - Return(nil) - - cache. - On( - "Get", mock.Anything, - client.ObjectKeyFromObject(obj), - mock.Anything, mock.Anything, - ). - Run(func(args mock.Arguments) { - objArg := args.Get(2).(*unstructured.Unstructured) - *objArg = *obj - }). - Return(nil) - - engineWriter.On("Patch", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil) - - engineWriter.On("Delete", mock.Anything, mock.Anything, mock.Anything). - Panic("Delete should not be called on the engine writer when WithTeardownWriter options is used") - - test.mockSetup(teardownWriter) - - oe := NewObjectEngine( - scheme.Scheme, - cache, engineWriter, - ownerStrategy, divergeDetector, - testFieldOwner, - testSystemPrefix, - ) - - result, err := oe.Teardown(t.Context(), owner, 1, obj, types.WithTeardownWriter(teardownWriter)) - if test.expectedError != nil { - assert.EqualError(t, err, test.expectedError.Error()) - } else { - require.NoError(t, err) - assert.Equal(t, test.expectedResult, result) - } - }) - } -} - -func TestObjectEngine_Teardown(t *testing.T) { - t.Parallel() - - owner := &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - UID: "12345-678", - Name: "owner", - Namespace: "test", + }) + }, + expectedGone: true, }, - } - - obj := &unstructured.Unstructured{ - Object: map[string]interface{}{ - "apiVersion": "v1", - "kind": "Secret", - "metadata": map[string]interface{}{ - "name": "testi", - "namespace": "test", + { + name: "Get error", + revision: 1, + desiredObject: buildObj("testi", "test"), + mockSetup: func( + cache *cacheMock, _ *testutil.CtrlClient, + _ *unstructured.Unstructured, + ) { + cache. + On("Get", mock.Anything, + client.ObjectKey{Name: "testi", Namespace: "test"}, + mock.Anything, mock.Anything). + Return(errors.New("cache error")) }, + expectedError: "getting object before deletion", }, - } - err := controllerutil.SetOwnerReference(owner, obj, scheme.Scheme) - require.NoError(t, err) - - tests := []struct { - name string - revision int64 - - mockSetup func( - *cacheMock, - *testutil.CtrlClient, - ) - - expectedResult bool - expectedError error - }{ { - name: "deletes", - revision: 1, - + name: "Deletes", + revision: 1, + desiredObject: buildObj("testi", "test"), + actualObject: buildObj("testi", "test", withRevision("1"), withManaged), mockSetup: func( cache *cacheMock, writer *testutil.CtrlClient, + actualObject *unstructured.Unstructured, ) { - actualObject := &unstructured.Unstructured{ - Object: map[string]interface{}{ - "apiVersion": "v1", - "kind": "Secret", - "metadata": map[string]interface{}{ - "name": "testi", - "namespace": "test", - "annotations": map[string]interface{}{ - "testtest.xxx/revision": "1", - }, - "ownerReferences": []interface{}{ - map[string]interface{}{ - "apiVersion": "v1", - "kind": "ConfigMap", - "controller": true, - "name": "owner", - "uid": "12345-678", - "blockOwnerDeletion": true, - }, - }, - }, - }, - } - - // Mock setup cache. - On( - "Get", mock.Anything, + On("Get", mock.Anything, client.ObjectKeyFromObject(actualObject), - mock.Anything, mock.Anything, - ). + mock.Anything, mock.Anything). Run(func(args mock.Arguments) { obj := args.Get(2).(*unstructured.Unstructured) *obj = *actualObject }). Return(nil) - writer. On("Delete", mock.Anything, mock.Anything, mock.Anything, mock.Anything). Return(nil) }, + expectedGone: false, }, { - name: "revision error", - revision: 1, - + name: "Delete NotFound", + revision: 1, + desiredObject: buildObj("testi", "test"), + actualObject: buildObj("testi", "test", withRevision("1"), withManaged), mockSetup: func( - cache *cacheMock, _ *testutil.CtrlClient, + cache *cacheMock, writer *testutil.CtrlClient, + actualObject *unstructured.Unstructured, ) { - actualObject := &unstructured.Unstructured{ - Object: map[string]interface{}{ - "apiVersion": "v1", - "kind": "Secret", - "metadata": map[string]interface{}{ - "name": "testi", - "namespace": "test", - "annotations": map[string]interface{}{ - "testtest.xxx/revision": "4", - }, - "ownerReferences": []interface{}{ - map[string]interface{}{ - "apiVersion": "v1", - "kind": "ConfigMap", - "controller": true, - "name": "owner", - "uid": "12345-678", - "blockOwnerDeletion": true, - }, - }, - }, - }, - } - - // Mock setup cache. - On( - "Get", mock.Anything, + On("Get", mock.Anything, client.ObjectKeyFromObject(actualObject), - mock.Anything, mock.Anything, - ). + mock.Anything, mock.Anything). Run(func(args mock.Arguments) { obj := args.Get(2).(*unstructured.Unstructured) *obj = *actualObject }). Return(nil) + writer. + On("Delete", mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Return(apierrors.NewNotFound(schema.GroupResource{}, "")) }, - expectedResult: true, + expectedGone: true, }, { - name: "owner error", - revision: 1, - + name: "Delete error", + revision: 1, + desiredObject: buildObj("testi", "test"), + actualObject: buildObj("testi", "test", withRevision("1"), withManaged), mockSetup: func( cache *cacheMock, writer *testutil.CtrlClient, + actualObject *unstructured.Unstructured, ) { - actualObject := &unstructured.Unstructured{ - Object: map[string]interface{}{ - "apiVersion": "v1", - "kind": "Secret", - "metadata": map[string]interface{}{ - "name": "testi", - "namespace": "test", - "annotations": map[string]interface{}{ - "testtest.xxx/revision": "1", - }, - }, - }, - } + cache. + On("Get", mock.Anything, + client.ObjectKeyFromObject(actualObject), + mock.Anything, mock.Anything). + Run(func(args mock.Arguments) { + obj := args.Get(2).(*unstructured.Unstructured) + *obj = *actualObject + }). + Return(nil) + writer. + On("Delete", mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Return(errors.New("delete failed")) + }, + expectedError: "deleting object", + }, + { + name: "TeardownWriter deletes", + revision: 1, + desiredObject: buildObj("testi", "test"), + actualObject: buildObj("testi", "test", withRevision("1"), withManaged), + opts: func() []types.ObjectTeardownOption { + teardownWriter := testutil.NewClient() + teardownWriter. + On("Delete", mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Return(nil) - // Mock setup + return []types.ObjectTeardownOption{types.WithTeardownWriter(teardownWriter)} + }, + mockSetup: func( + cache *cacheMock, writer *testutil.CtrlClient, + actualObject *unstructured.Unstructured, + ) { cache. - On( - "Get", mock.Anything, + On("Get", mock.Anything, client.ObjectKeyFromObject(actualObject), - mock.Anything, mock.Anything, - ). + mock.Anything, mock.Anything). Run(func(args mock.Arguments) { obj := args.Get(2).(*unstructured.Unstructured) *obj = *actualObject }). Return(nil) + writer.On("Delete", mock.Anything, mock.Anything, mock.Anything). + Panic("Delete should not be called on the engine writer") + }, + expectedGone: false, + }, + { + name: "TeardownWriter error", + revision: 1, + desiredObject: buildObj("testi", "test"), + actualObject: buildObj("testi", "test", withRevision("1"), withManaged), + opts: func() []types.ObjectTeardownOption { + teardownWriter := testutil.NewClient() + teardownWriter. + On("Delete", mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Return(errors.New("teardown delete failed")) + return []types.ObjectTeardownOption{types.WithTeardownWriter(teardownWriter)} + }, + mockSetup: func( + cache *cacheMock, writer *testutil.CtrlClient, + actualObject *unstructured.Unstructured, + ) { + cache. + On("Get", mock.Anything, + client.ObjectKeyFromObject(actualObject), + mock.Anything, mock.Anything). + Run(func(args mock.Arguments) { + obj := args.Get(2).(*unstructured.Unstructured) + *obj = *actualObject + }). + Return(nil) + writer.On("Delete", mock.Anything, mock.Anything, mock.Anything). + Panic("Delete should not be called on the engine writer") + }, + expectedError: "deleting object: teardown delete failed", + }, + { + name: "Revision mismatch", + revision: 1, + desiredObject: buildObj("testi", "test"), + actualObject: buildObj("testi", "test", withRevision("4")), + mockSetup: func( + cache *cacheMock, writer *testutil.CtrlClient, + actualObject *unstructured.Unstructured, + ) { + cache. + On("Get", mock.Anything, + client.ObjectKeyFromObject(actualObject), + mock.Anything, mock.Anything). + Run(func(args mock.Arguments) { + obj := args.Get(2).(*unstructured.Unstructured) + *obj = *actualObject + }). + Return(nil) + // Patch may be called in with-owner mode to remove owner ref. writer.On("Patch", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil) }, - expectedResult: true, + expectedGone: true, + }, + { + name: "Revision mismatch, unknown controller", + revision: 1, + desiredObject: buildObj("testi", "test"), + actualObject: buildObj("testi", "test", withRevision("4"), + withOwnerRef("v1", "ConfigMap", "other-owner", "12345-678", true)), + modes: []ownerMode{withNativeOwnerMode}, + mockSetup: func( + cache *cacheMock, writer *testutil.CtrlClient, + actualObject *unstructured.Unstructured, + ) { + cache. + On("Get", mock.Anything, + client.ObjectKeyFromObject(actualObject), + mock.Anything, mock.Anything). + Run(func(args mock.Arguments) { + obj := args.Get(2).(*unstructured.Unstructured) + *obj = *actualObject + }). + Return(nil) + writer.On("Patch", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil) + }, + expectedGone: true, + }, + { + name: "Revision mismatch, no controller", + revision: 1, + desiredObject: buildObj("testi", "test"), + actualObject: buildObj("testi", "test", withRevision("2")), + mockSetup: func( + cache *cacheMock, writer *testutil.CtrlClient, + actualObject *unstructured.Unstructured, + ) { + cache. + On("Get", mock.Anything, + client.ObjectKeyFromObject(actualObject), + mock.Anything, mock.Anything). + Run(func(args mock.Arguments) { + obj := args.Get(2).(*unstructured.Unstructured) + *obj = *actualObject + }). + Return(nil) + writer.On("Patch", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil) + }, + expectedGone: true, + }, + { + name: "OrphanFinalizer", + revision: 1, + desiredObject: buildObj("testi", "test"), + actualObject: buildObj("testi", "test", withManaged), + modes: []ownerMode{nativeOwnerMode("with orphan finalizer owner", &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + UID: "12345-678", Name: "owner", Namespace: "test", + Finalizers: []string{"orphan"}, + }, + })}, + mockSetup: func( + _ *cacheMock, writer *testutil.CtrlClient, + _ *unstructured.Unstructured, + ) { + writer.On("Patch", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil) + }, + expectedGone: true, }, } - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - t.Parallel() + for _, tc := range sharedTests { + modes := tc.modes + if len(modes) == 0 { + modes = defaultModes + } - cache := &cacheMock{} - writer := testutil.NewClient() - ownerStrategy := ownerhandling.NewNative(scheme.Scheme) - divergeDetector := &comparatorMock{} - - cache. - On("Watch", mock.Anything, mock.Anything, mock.Anything). - Return(nil) - - writer.On("Patch", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil) - - test.mockSetup(cache, writer) - oe := NewObjectEngine( - scheme.Scheme, - cache, writer, - ownerStrategy, divergeDetector, - testFieldOwner, - testSystemPrefix, - ) - - deleted, err := oe.Teardown(t.Context(), owner, 1, obj) - if test.expectedError != nil { - assert.EqualError(t, err, test.expectedError.Error()) - } else { - require.NoError(t, err) - assert.Equal(t, test.expectedResult, deleted) - } - }) + for _, mode := range modes { + t.Run(tc.name+"/"+mode.name, func(t *testing.T) { + t.Parallel() + + cache := &cacheMock{} + writer := testutil.NewClient() + divergeDetector := &comparatorMock{} + + oe := NewObjectEngine( + scheme.Scheme, + cache, writer, + divergeDetector, + testFieldOwner, + testSystemPrefix, + ) + + desiredObject := tc.desiredObject(&mode) + + var actualObject *unstructured.Unstructured + if tc.actualObject != nil { + actualObject = tc.actualObject(&mode) + } + + tc.mockSetup(cache, writer, actualObject) + + opts := mode.teardownOpts() + if tc.opts != nil { + opts = append(opts, tc.opts()...) + } + + gone, err := oe.Teardown( + t.Context(), tc.revision, desiredObject, + opts..., + ) + + if tc.expectedError == "" { + require.NoError(t, err) + assert.Equal(t, tc.expectedGone, gone) + } else { + require.Error(t, err) + assert.Contains(t, err.Error(), tc.expectedError) + } + }) + } } } -func TestObjectEngine_Teardown_Orphan(t *testing.T) { +func TestObjectEngine_Teardown_SanityChecks(t *testing.T) { t.Parallel() - owner := &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - UID: "12345-678", - Name: "owner", - Namespace: "test", + oe := &ObjectEngine{} + owner := &unstructured.Unstructured{} + desired := &unstructured.Unstructured{} + + t.Run("missing revision", func(t *testing.T) { + t.Parallel() + assert.PanicsWithValue(t, "owner revision must be set and start at 1", func() { + _, _ = oe.Teardown(t.Context(), 0, desired) + }) + }) + + t.Run("missing owner.UID", func(t *testing.T) { + t.Parallel() + assert.PanicsWithValue(t, "owner must be persisted to cluster, empty UID", func() { + _, _ = oe.Teardown(t.Context(), 1, desired, types.WithOwner(owner, nil)) + }) + }) +} + +func TestObjectEngine_IsBoxcutterManaged_FalseCase(t *testing.T) { + t.Parallel() + + engine := NewObjectEngine( + scheme.Scheme, + testutil.NewClient(), + testutil.NewClient(), + &comparatorMock{}, + "test-owner", + "test-prefix", + ) + + tests := []struct { + name string + labels map[string]string + }{ + { + name: "no labels", + labels: nil, + }, + { + name: "empty labels", + labels: map[string]string{}, }, + { + name: "other labels only", + labels: map[string]string{ + "app": "myapp", + "env": "prod", + }, + }, + { + name: "contains but doesn't start with prefix", + labels: map[string]string{ + "my-test-prefix-managed": "true", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + obj := &unstructured.Unstructured{} + obj.SetLabels(tt.labels) + + assert.False(t, engine.isBoxcutterManaged(obj)) + }) } +} + +func TestObjectEngine_GetObjectRevision_Error(t *testing.T) { + t.Parallel() + + oe := NewObjectEngine( + scheme.Scheme, + testutil.NewClient(), + testutil.NewClient(), + &comparatorMock{}, + testFieldOwner, + testSystemPrefix, + ) obj := &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "v1", "kind": "Secret", "metadata": map[string]interface{}{ - "name": "testi", + "name": "test", "namespace": "test", + "annotations": map[string]interface{}{ + testSystemPrefix + "/revision": "not-a-number", + }, }, }, } - cache := &cacheMock{} - writer := testutil.NewClient() - ownerStrategy := ownerhandling.NewNative(scheme.Scheme) - divergeDetector := &comparatorMock{} + _, err := oe.getObjectRevision(obj) + require.Error(t, err) +} - cache. - On("Watch", mock.Anything, mock.Anything, mock.Anything). - Return(nil) +func TestObjectEngine_MigrateFieldManagersToSSA_NoPatch(t *testing.T) { + t.Parallel() - writer.On("Patch", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil) + writer := testutil.NewClient() oe := NewObjectEngine( scheme.Scheme, - cache, writer, - ownerStrategy, divergeDetector, + testutil.NewClient(), + writer, + &comparatorMock{}, testFieldOwner, testSystemPrefix, ) - deleted, err := oe.Teardown(t.Context(), owner, 1, obj, types.WithOrphan()) - require.NoError(t, err) - - assert.True(t, deleted) -} - -func TestObjectEngine_Teardown_SanityChecks(t *testing.T) { - t.Parallel() - oe := &ObjectEngine{} - owner := &unstructured.Unstructured{} - desired := &unstructured.Unstructured{} + obj := &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "Secret", + "metadata": map[string]interface{}{ + "name": "test", + "namespace": "test", + }, + }, + } - t.Run("missing revision", func(t *testing.T) { - t.Parallel() - assert.PanicsWithValue(t, "owner revision must be set and start at 1", func() { - _, _ = oe.Teardown(t.Context(), owner, 0, desired) - }) - }) + err := oe.migrateFieldManagersToSSA(context.Background(), obj) + require.NoError(t, err) - t.Run("missing owner.UID", func(t *testing.T) { - t.Parallel() - assert.PanicsWithValue(t, "owner must be persisted to cluster, empty UID", func() { - _, _ = oe.Teardown(t.Context(), owner, 1, desired) - }) - }) + writer.AssertNotCalled(t, "Patch") } type cacheMock struct { @@ -1391,10 +1453,10 @@ type comparatorMock struct { } func (m *comparatorMock) Compare( - owner client.Object, desiredObject, actualObject Object, + opts ...types.ComparatorOption, ) (CompareResult, error) { - args := m.Called(owner, desiredObject, actualObject) + args := m.Called(desiredObject, actualObject, opts) return args.Get(0).(CompareResult), args.Error(1) } diff --git a/machinery/phases.go b/machinery/phases.go index bf61a2c3..9d4d68d4 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,22 +34,20 @@ func NewPhaseEngine( type phaseValidator interface { Validate( ctx context.Context, - owner client.Object, phase types.Phase, + opts ...types.PhaseReconcileOption, ) error } type objectEngine interface { Reconcile( ctx context.Context, - owner client.Object, // Owner of the object. 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. revision int64, // Revision number, must start at 1. desiredObject Object, opts ...types.ObjectTeardownOption, @@ -135,11 +132,12 @@ 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, revision int64, phase types.Phase, opts ...types.PhaseTeardownOption, ) (PhaseTeardownResult, error) { + opts = append(opts, phase.GetTeardownOptions()...) + var options types.PhaseTeardownOptions for _, opt := range opts { opt.ApplyToPhaseTeardownOptions(&options) @@ -147,11 +145,9 @@ func (e *PhaseEngine) Teardown( res := &phaseTeardownResult{name: phase.GetName()} - for _, o := range phase.GetObjects() { - obj := &o - + for _, obj := range phase.GetObjects() { gone, err := e.objectEngine.Teardown( - ctx, owner, revision, obj, options.ForObject(obj)...) + ctx, revision, obj, options.ForObject(obj)...) if err != nil { return res, fmt.Errorf("teardown object: %w", err) } @@ -169,11 +165,12 @@ func (e *PhaseEngine) Teardown( // Reconcile runs actions to bring actual state closer to desired. func (e *PhaseEngine) Reconcile( ctx context.Context, - owner client.Object, revision int64, phase types.Phase, opts ...types.PhaseReconcileOption, ) (PhaseResult, error) { + opts = append(opts, phase.GetReconcileOptions()...) + var options types.PhaseReconcileOptions for _, opt := range opts { opt.ApplyToPhaseReconcileOptions(&options) @@ -184,7 +181,7 @@ func (e *PhaseEngine) Reconcile( } // Preflight - err := e.phaseValidator.Validate(ctx, owner, phase) + err := e.phaseValidator.Validate(ctx, phase, opts...) if err != nil { var perr validation.PhaseValidationError if errors.As(err, &perr) { @@ -197,11 +194,9 @@ func (e *PhaseEngine) Reconcile( } // Reconcile - for _, o := range phase.GetObjects() { - obj := &o - + for _, obj := range phase.GetObjects() { ores, err := e.objectEngine.Reconcile( - ctx, owner, revision, obj, options.ForObject(obj)...) + ctx, 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..ba808001 100644 --- a/machinery/phases_test.go +++ b/machinery/phases_test.go @@ -51,15 +51,15 @@ 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, revision, obj, mock.Anything). Return(newObjectResultCreated(obj, types.ObjectReconcileOptions{}), nil) - _, err := pe.Reconcile(t.Context(), owner, revision, types.Phase{ - Name: "test", - Objects: []unstructured.Unstructured{ - *obj, + _, err := pe.Reconcile(t.Context(), revision, types.NewPhase( + "test", + []client.Object{ + obj, }, - }) + ), types.WithOwner(owner, nil)) require.NoError(t, err) } @@ -97,12 +97,12 @@ func TestPhaseEngine_Reconcile_PreflightViolation(t *testing.T) { oe.On("Reconcile", mock.Anything, owner, revision, obj, mock.Anything). Return(newObjectResultCreated(obj, types.ObjectReconcileOptions{}), nil) - _, err := pe.Reconcile(t.Context(), owner, revision, types.Phase{ - Name: "test", - Objects: []unstructured.Unstructured{ - *obj, + _, err := pe.Reconcile(t.Context(), revision, types.NewPhase( + "test", + []client.Object{ + obj, }, - }) + ), types.WithOwner(owner, nil)) require.NoError(t, err) oe.AssertNotCalled( t, "Reconcile", mock.Anything, mock.Anything, @@ -138,15 +138,14 @@ 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, revision, obj, mock.Anything, mock.Anything). Return(true, nil) - deleted, err := pe.Teardown(t.Context(), owner, revision, types.Phase{ - Name: "test", - Objects: []unstructured.Unstructured{ - *obj, *obj, + deleted, err := pe.Teardown(t.Context(), revision, types.NewPhase( + "test", []client.Object{ + obj, obj, }, - }) + ), types.WithOwner(owner, nil)) require.NoError(t, err) assert.True(t, deleted.IsComplete()) assert.Empty(t, deleted.Waiting()) @@ -159,24 +158,22 @@ type objectEngineMock struct { func (m *objectEngineMock) Reconcile( ctx context.Context, - owner client.Object, revision int64, desiredObject Object, opts ...types.ObjectReconcileOption, ) (ObjectResult, error) { - args := m.Called(ctx, owner, revision, desiredObject, opts) + args := m.Called(ctx, revision, desiredObject, opts) return args.Get(0).(ObjectResult), args.Error(1) } func (m *objectEngineMock) Teardown( ctx context.Context, - owner client.Object, revision int64, desiredObject Object, opts ...types.ObjectTeardownOption, ) (objectDeleted bool, err error) { - args := m.Called(ctx, owner, revision, desiredObject, opts) + args := m.Called(ctx, revision, desiredObject, opts) return args.Bool(0), args.Error(1) } @@ -187,10 +184,10 @@ type phaseValidatorMock struct { func (m *phaseValidatorMock) Validate( ctx context.Context, - owner client.Object, phase types.Phase, + opts ...types.PhaseReconcileOption, ) error { - args := m.Called(ctx, owner, phase) + args := m.Called(ctx, phase, opts) return args.Error(0) } diff --git a/machinery/results_test.go b/machinery/results_test.go index d6f53d2f..2ebc4c21 100644 --- a/machinery/results_test.go +++ b/machinery/results_test.go @@ -399,3 +399,42 @@ func Test_isComplete(t *testing.T) { }) } } + +func TestObjectResultCollision_Success(t *testing.T) { + t.Parallel() + + ownerRef := &metav1.OwnerReference{ + APIVersion: "v1", + Kind: "ConfigMap", + Name: "conflicting-owner", + UID: "uid-123", + } + + collision := newObjectResultConflict( + resultExampleObj, + CompareResult{}, + ownerRef, + types.ObjectReconcileOptions{}, + ) + + // Cast to access Success() method + collisionResult, ok := collision.(ObjectResultCollision) + assert.True(t, ok) + + // Collisions always report as not successful + assert.False(t, collisionResult.Success()) +} + +func TestNewNormalObjectResult_PanicOnCreated(t *testing.T) { + t.Parallel() + + // newNormalObjectResult should panic when ActionCreated is passed + assert.Panics(t, func() { + newNormalObjectResult( + ActionCreated, + resultExampleObj, + CompareResult{}, + types.ObjectReconcileOptions{}, + ) + }) +} diff --git a/machinery/revision.go b/machinery/revision.go index 380ceb32..4280071e 100644 --- a/machinery/revision.go +++ b/machinery/revision.go @@ -40,14 +40,12 @@ type revisionValidator interface { type phaseEngine interface { Reconcile( ctx context.Context, - owner client.Object, revision int64, phase types.Phase, opts ...types.PhaseReconcileOption, ) (PhaseResult, error) Teardown( ctx context.Context, - owner client.Object, revision int64, phase types.Phase, opts ...types.PhaseTeardownOption, @@ -183,7 +181,7 @@ func (re *RevisionEngine) Reconcile( opts ...types.RevisionReconcileOption, ) (RevisionResult, error) { var options types.RevisionReconcileOptions - for _, opt := range opts { + for _, opt := range append(opts, rev.GetReconcileOptions()...) { opt.ApplyToRevisionReconcileOptions(&options) } @@ -207,7 +205,7 @@ func (re *RevisionEngine) Reconcile( // Reconcile for _, phase := range rev.GetPhases() { pres, err := re.phaseEngine.Reconcile( - ctx, rev.GetOwner(), rev.GetRevisionNumber(), + ctx, rev.GetRevisionNumber(), phase, options.ForPhase(phase.GetName())...) if err != nil { return rres, fmt.Errorf("reconciling object: %w", err) @@ -323,7 +321,7 @@ func (re *RevisionEngine) Teardown( opts ...types.RevisionTeardownOption, ) (RevisionTeardownResult, error) { var options types.RevisionTeardownOptions - for _, opt := range opts { + for _, opt := range append(opts, rev.GetTeardownOptions()...) { opt.ApplyToRevisionTeardownOptions(&options) } @@ -344,7 +342,7 @@ func (re *RevisionEngine) Teardown( res.active = p.GetName() pres, err := re.phaseEngine.Teardown( - ctx, rev.GetOwner(), rev.GetRevisionNumber(), + ctx, 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 34035631..7b15eb8c 100644 --- a/machinery/revision_test.go +++ b/machinery/revision_test.go @@ -35,21 +35,20 @@ func TestRevisionEngine_Teardown(t *testing.T) { }, } - rev := types.Revision{ - Owner: owner, - Revision: 3, - Phases: []types.Phase{ - {Name: "phase-1"}, - {Name: "phase-2"}, - {Name: "phase-3"}, + rev := types.NewRevision( + "test", 3, + []types.Phase{ + types.NewPhase("phase-1", nil), + types.NewPhase("phase-2", nil), + types.NewPhase("phase-3", nil), }, - } + ) pe. - On("Teardown", mock.Anything, owner, mock.Anything, mock.Anything, mock.Anything). + On("Teardown", mock.Anything, mock.Anything, mock.Anything, mock.Anything). Return(&phaseTeardownResult{}, nil) - res, err := re.Teardown(t.Context(), rev) + res, err := re.Teardown(t.Context(), rev, types.WithOwner(owner, nil)) require.NoError(t, err) assert.True(t, res.IsComplete()) @@ -81,28 +80,27 @@ func TestRevisionEngine_Teardown_delayed(t *testing.T) { }, } - rev := types.Revision{ - Owner: owner, - Revision: 3, - Phases: []types.Phase{ - {Name: "phase-1"}, - {Name: "phase-2"}, - {Name: "phase-3"}, - {Name: "phase-4"}, + rev := types.NewRevision( + "test", 3, + []types.Phase{ + types.NewPhase("phase-1", nil), + types.NewPhase("phase-2", nil), + types.NewPhase("phase-3", nil), + types.NewPhase("phase-4", nil), }, - } + ) pe. - On("Teardown", mock.Anything, owner, mock.Anything, mock.Anything, mock.Anything). + On("Teardown", 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). Return(&phaseTeardownResult{waiting: []types.ObjectRef{ {}, }}, nil) - res, err := re.Teardown(t.Context(), rev) + res, err := re.Teardown(t.Context(), rev, types.WithOwner(owner, nil)) require.NoError(t, err) assert.False(t, res.IsComplete()) @@ -115,30 +113,566 @@ func TestRevisionEngine_Teardown_delayed(t *testing.T) { assert.Equal(t, []string{"phase-1"}, res.GetWaitingPhaseNames()) } +func TestRevisionEngine_Reconcile_Success_AllPhasesComplete(t *testing.T) { + t.Parallel() + + pe := &phaseEngineMock{} + rv := &revisionValidatorMock{} + c := testutil.NewClient() + + re := NewRevisionEngine(pe, rv, c) + + owner := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + UID: "12345", + Name: "owner", + Namespace: "test", + }, + } + + obj1 := &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": map[string]interface{}{ + "name": "obj1", + "namespace": "test", + }, + }, + } + + obj2 := &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "Secret", + "metadata": map[string]interface{}{ + "name": "obj2", + "namespace": "test", + }, + }, + } + + obj3 := &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "Service", + "metadata": map[string]interface{}{ + "name": "obj3", + "namespace": "test", + }, + }, + } + + // Create revision with 3 phases + rev := types.NewRevision("test", 3, []types.Phase{ + types.NewPhase("phase-1", []client.Object{obj1}), + types.NewPhase("phase-2", []client.Object{obj2}), + types.NewPhase("phase-3", []client.Object{obj3}), + }) + + // Mock: validation passes + rv.On("Validate", mock.Anything, rev).Return(nil) + + // Mock: all phases complete successfully + pe.On("Reconcile", mock.Anything, int64(3), mock.Anything, mock.Anything). + Return(&testPhaseResult{complete: true, progressed: true}, nil). + Times(3) + + // Execute + res, err := re.Reconcile(context.Background(), rev, types.WithOwner(owner, nil)) + + // Assert + require.NoError(t, err) + assert.True(t, res.IsComplete()) + assert.False(t, res.InTransition()) + assert.Len(t, res.GetPhases(), 3) + assert.Nil(t, res.GetValidationError()) + pe.AssertExpectations(t) + rv.AssertExpectations(t) +} + +func TestRevisionEngine_Reconcile_WaitsOnIncompletePhase(t *testing.T) { + t.Parallel() + + pe := &phaseEngineMock{} + rv := &revisionValidatorMock{} + c := testutil.NewClient() + + re := NewRevisionEngine(pe, rv, c) + + owner := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + UID: "12345", + Name: "owner", + Namespace: "test", + }, + } + + obj1 := &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": map[string]interface{}{ + "name": "obj1", + "namespace": "test", + }, + }, + } + + rev := types.NewRevision("test", 3, []types.Phase{ + types.NewPhase("phase-1", []client.Object{obj1}), + types.NewPhase("phase-2", nil), + types.NewPhase("phase-3", nil), + }) + + // Mock: validation passes + rv.On("Validate", mock.Anything, rev).Return(nil) + + // First phase incomplete, second should never be called + pe.On("Reconcile", mock.Anything, int64(3), mock.Anything, mock.Anything). + Return(&testPhaseResult{complete: false}, nil). + Once() + + res, err := re.Reconcile(context.Background(), rev, types.WithOwner(owner, nil)) + + require.NoError(t, err) + assert.False(t, res.IsComplete()) + assert.True(t, res.InTransition()) + assert.Len(t, res.GetPhases(), 1) // Only first phase + + // Verify second phase was never reconciled + pe.AssertNumberOfCalls(t, "Reconcile", 1) + rv.AssertExpectations(t) +} + +func TestRevisionEngine_Reconcile_ValidationError(t *testing.T) { + t.Parallel() + + pe := &phaseEngineMock{} + rv := &revisionValidatorMock{} + c := testutil.NewClient() + + re := NewRevisionEngine(pe, rv, c) + + rev := types.NewRevision("test", 3, []types.Phase{ + types.NewPhase("phase-1", nil), + }) + + // Mock: validation returns RevisionValidationError + validationErr := &validation.RevisionValidationError{ + RevisionName: "test", + RevisionNumber: 3, + Phases: []validation.PhaseValidationError{ + { + PhaseError: errTest, + }, + }, + } + rv.On("Validate", mock.Anything, rev).Return(validationErr) + + res, err := re.Reconcile(context.Background(), rev) + + // Should NOT error out, but capture validation error in result + require.NoError(t, err) + assert.NotNil(t, res.GetValidationError()) + assert.Equal(t, validationErr, res.GetValidationError()) + assert.False(t, res.InTransition()) // Validation errors block transition + assert.Empty(t, res.GetPhases()) // No phases reconciled + + // Verify no phase reconciliation attempted + pe.AssertNotCalled(t, "Reconcile") + rv.AssertExpectations(t) +} + +func TestRevisionEngine_Reconcile_ValidationFailure(t *testing.T) { + t.Parallel() + + pe := &phaseEngineMock{} + rv := &revisionValidatorMock{} + c := testutil.NewClient() + + re := NewRevisionEngine(pe, rv, c) + + rev := types.NewRevision("test", 3, []types.Phase{ + types.NewPhase("phase-1", nil), + }) + + // Mock: validation returns non-RevisionValidationError + rv.On("Validate", mock.Anything, rev). + Return(errTest) + + res, err := re.Reconcile(context.Background(), rev) + + // Should error out + require.Error(t, err) + assert.Contains(t, err.Error(), "validating") + + // Result still returned but phases not reconciled + assert.Empty(t, res.GetPhases()) + + // Verify no phase reconciliation attempted + pe.AssertNotCalled(t, "Reconcile") + rv.AssertExpectations(t) +} + +func TestRevisionEngine_Reconcile_PhaseReconcileError(t *testing.T) { + t.Parallel() + + pe := &phaseEngineMock{} + rv := &revisionValidatorMock{} + c := testutil.NewClient() + + re := NewRevisionEngine(pe, rv, c) + + rev := types.NewRevision("test", 3, []types.Phase{ + types.NewPhase("phase-1", nil), + types.NewPhase("phase-2", nil), + types.NewPhase("phase-3", nil), + }) + + // Mock: validation passes + rv.On("Validate", mock.Anything, rev).Return(nil) + + // First phase succeeds + pe.On("Reconcile", mock.Anything, int64(3), mock.Anything, mock.Anything). + Return(&testPhaseResult{complete: true}, nil). + Once() + + // Second phase errors + pe.On("Reconcile", mock.Anything, int64(3), mock.Anything, mock.Anything). + Return(&testPhaseResult{}, errTest). + Once() + + res, err := re.Reconcile(context.Background(), rev) + + require.Error(t, err) + assert.Contains(t, err.Error(), "reconciling object") + assert.Len(t, res.GetPhases(), 1) // Only first phase in result + + pe.AssertExpectations(t) + rv.AssertExpectations(t) +} + +func TestRevisionEngine_Reconcile_WithRevisionOptions(t *testing.T) { + t.Parallel() + + pe := &phaseEngineMock{} + rv := &revisionValidatorMock{} + c := testutil.NewClient() + + re := NewRevisionEngine(pe, rv, c) + + owner := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + UID: "12345", + Name: "owner", + Namespace: "test", + }, + } + + testObj := &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": map[string]interface{}{ + "name": "test-obj", + "namespace": "test", + }, + }, + } + + // Create a simple prober for testing + testProber := types.ProbeFunc(func(client.Object) types.ProbeResult { + return types.ProbeResult{Status: types.ProbeStatusTrue} + }) + + // Create revision with phase-level options + rev := types.NewRevision("test", 1, []types.Phase{ + types.NewPhase("phase-1", []client.Object{testObj}). + WithReconcileOptions(types.WithProbe("test", testProber)), + }) + + // Mock: validation passes + rv.On("Validate", mock.Anything, rev).Return(nil) + + // Verify options are merged and forwarded + pe.On("Reconcile", mock.Anything, int64(1), mock.Anything, + mock.MatchedBy(func(opts []types.PhaseReconcileOption) bool { + // Verify options were passed (includes phase options + owner) + return len(opts) > 0 + })). + Return(&testPhaseResult{complete: true}, nil) + + res, err := re.Reconcile(context.Background(), rev, types.WithOwner(owner, nil)) + + require.NoError(t, err) + assert.True(t, res.IsComplete()) + pe.AssertExpectations(t) + rv.AssertExpectations(t) +} + +func TestRevisionResult_HasProgressed(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + phases []string + results []PhaseResult + expected bool + }{ + { + name: "all phases progressed", + phases: []string{"p1", "p2", "p3"}, + results: []PhaseResult{ + &testPhaseResult{progressed: true}, + &testPhaseResult{progressed: true}, + &testPhaseResult{progressed: true}, + }, + expected: true, + }, + { + name: "some phases progressed", + phases: []string{"p1", "p2", "p3"}, + results: []PhaseResult{ + &testPhaseResult{progressed: true}, + &testPhaseResult{progressed: false}, + &testPhaseResult{progressed: true}, + }, + expected: false, + }, + { + name: "no phases progressed", + phases: []string{"p1", "p2"}, + results: []PhaseResult{ + &testPhaseResult{progressed: false}, + &testPhaseResult{progressed: false}, + }, + expected: false, + }, + { + name: "empty phases", + phases: []string{}, + results: []PhaseResult{}, + expected: true, // 0 == 0 is true + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + r := &revisionResult{ + phases: tt.phases, + phasesResults: tt.results, + } + assert.Equal(t, tt.expected, r.HasProgressed()) + }) + } +} + +func TestRevisionResult_IsComplete_EdgeCases(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + phases []string + results []PhaseResult + expected bool + }{ + { + name: "not all phases acted on", + phases: []string{"p1", "p2", "p3"}, + results: []PhaseResult{ + &testPhaseResult{complete: true}, + &testPhaseResult{complete: true}, + // Missing p3 + }, + expected: false, // Line 124-127 + }, + { + name: "all phases acted but some incomplete", + phases: []string{"p1", "p2"}, + results: []PhaseResult{ + &testPhaseResult{complete: true}, + &testPhaseResult{complete: false}, + }, + expected: false, // Line 129-132 + }, + { + name: "all phases complete", + phases: []string{"p1", "p2"}, + results: []PhaseResult{ + &testPhaseResult{complete: true}, + &testPhaseResult{complete: true}, + }, + expected: true, // Line 135 + }, + { + name: "empty phases list", + phases: []string{}, + results: []PhaseResult{}, + expected: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + r := &revisionResult{ + phases: tt.phases, + phasesResults: tt.results, + } + assert.Equal(t, tt.expected, r.IsComplete()) + }) + } +} + +func TestRevisionResult_InTransition_EdgeCases(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + phases []string + results []PhaseResult + validationError *validation.RevisionValidationError + expected bool + }{ + { + name: "phase in transition", + phases: []string{"p1", "p2"}, + results: []PhaseResult{ + &testPhaseResult{inTransition: true}, + &testPhaseResult{inTransition: false}, + }, + expected: true, // Line 90-93 + }, + { + name: "validation error present", + phases: []string{"p1"}, + results: []PhaseResult{}, + validationError: &validation.RevisionValidationError{ + RevisionName: "test", + RevisionNumber: 1, + }, + expected: false, // Line 96-98 + }, + { + name: "not all phases acted on", + phases: []string{"p1", "p2", "p3"}, + results: []PhaseResult{ + &testPhaseResult{inTransition: false}, + }, + expected: true, // Line 100-103 + }, + { + name: "all complete no transition", + phases: []string{"p1", "p2"}, + results: []PhaseResult{ + &testPhaseResult{inTransition: false}, + &testPhaseResult{inTransition: false}, + }, + expected: false, // Line 105 + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + r := &revisionResult{ + phases: tt.phases, + phasesResults: tt.results, + validationError: tt.validationError, + } + assert.Equal(t, tt.expected, r.InTransition()) + }) + } +} + +func TestRevisionResult_GetPhases(t *testing.T) { + t.Parallel() + + t.Run("returns phase results", func(t *testing.T) { + t.Parallel() + + results := []PhaseResult{ + &testPhaseResult{name: "p1"}, + &testPhaseResult{name: "p2"}, + } + r := &revisionResult{phasesResults: results} + + assert.Equal(t, results, r.GetPhases()) + }) + + t.Run("empty list", func(t *testing.T) { + t.Parallel() + + r := &revisionResult{} + assert.Empty(t, r.GetPhases()) + }) +} + +// testPhaseResult is a simple test implementation of PhaseResult. +type testPhaseResult struct { + complete bool + progressed bool + inTransition bool + name string + validationErr *validation.PhaseValidationError + objects []ObjectResult +} + +func (r *testPhaseResult) GetName() string { + return r.name +} + +func (r *testPhaseResult) GetValidationError() *validation.PhaseValidationError { + return r.validationErr +} + +func (r *testPhaseResult) GetObjects() []ObjectResult { + return r.objects +} + +func (r *testPhaseResult) InTransition() bool { + return r.inTransition +} + +func (r *testPhaseResult) IsComplete() bool { + return r.complete +} + +func (r *testPhaseResult) HasProgressed() bool { + return r.progressed +} + +func (r *testPhaseResult) String() string { + return "" +} + type phaseEngineMock struct { mock.Mock } func (m *phaseEngineMock) Reconcile( ctx context.Context, - owner client.Object, revision int64, phase types.Phase, opts ...types.PhaseReconcileOption, ) (PhaseResult, error) { - args := m.Called(ctx, owner, revision, phase, opts) + args := m.Called(ctx, revision, phase, opts) return args.Get(0).(PhaseResult), args.Error(1) } func (m *phaseEngineMock) Teardown( ctx context.Context, - owner client.Object, revision int64, phase types.Phase, opts ...types.PhaseTeardownOption, ) (PhaseTeardownResult, error) { - args := m.Called(ctx, owner, revision, phase, opts) + args := m.Called(ctx, revision, phase, opts) return args.Get(0).(PhaseTeardownResult), args.Error(1) } diff --git a/machinery/types/options.go b/machinery/types/options.go index 6b51c3bc..a80aba9c 100644 --- a/machinery/types/options.go +++ b/machinery/types/options.go @@ -1,6 +1,7 @@ package types import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -21,6 +22,20 @@ func (rropts RevisionReconcileOptions) ForPhase(phaseName string) []PhaseReconci return opts } +func (rropts RevisionReconcileOptions) GetOwner() client.Object { + var phaseOptions PhaseReconcileOptions + for _, opt := range rropts.DefaultPhaseOptions { + opt.ApplyToPhaseReconcileOptions(&phaseOptions) + } + + var objectOptions ObjectReconcileOptions + for _, opt := range phaseOptions.DefaultObjectOptions { + opt.ApplyToObjectReconcileOptions(&objectOptions) + } + + return objectOptions.Owner +} + // RevisionReconcileOption is the common interface for revision reconciliation options. type RevisionReconcileOption interface { ApplyToRevisionReconcileOptions(opts *RevisionReconcileOptions) @@ -102,12 +117,18 @@ type PhaseTeardownOption interface { type ObjectReconcileOptions struct { CollisionProtection CollisionProtection PreviousOwners []client.Object + Owner client.Object + OwnerStrategy OwnerStrategy Paused bool Probes map[string]Prober } // Default sets empty Option fields to their default value. func (opts *ObjectReconcileOptions) Default() { + if opts.Owner != nil && opts.OwnerStrategy == nil { + panic("Owner without ownerStrategy set") + } + if len(opts.CollisionProtection) == 0 { opts.CollisionProtection = CollisionProtectionPrevent } @@ -131,10 +152,16 @@ var ( type ObjectTeardownOptions struct { Orphan bool TeardownWriter client.Writer + Owner client.Object + OwnerStrategy OwnerStrategy } // Default sets empty Option fields to their default value. -func (opts *ObjectTeardownOptions) Default() {} +func (opts *ObjectTeardownOptions) Default() { + if opts.Owner != nil && opts.OwnerStrategy == nil { + panic("Owner without ownerStrategy set") + } +} // ObjectTeardownOption is the common interface for object teardown options. type ObjectTeardownOption interface { @@ -349,6 +376,67 @@ func (p *withPhaseTeardownOptions) ApplyToRevisionTeardownOptions(opts *Revision opts.PhaseOptions[p.phaseName] = p.opts } +// 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) +} + +// WithOwner sets an owning object and the strategy to use with it. +// Ensures controller-refs are set to track the owner and +// enables handover between owners. +func WithOwner(obj client.Object, start OwnerStrategy) interface { + ComparatorOption + ObjectReconcileOption + ObjectTeardownOption +} { + if len(obj.GetUID()) == 0 { + panic("owner must be persisted to cluster, empty UID") + } + + return &combinedOpts{ + fn: func(opts *ComparatorOptions) { + opts.Owner = obj + opts.OwnerStrategy = start + }, + optionFn: optionFn{ + fn: func(opts *ObjectReconcileOptions) { + opts.Owner = obj + opts.OwnerStrategy = start + }, + }, + teardownOptionFn: teardownOptionFn{ + fn: func(opts *ObjectTeardownOptions) { + opts.Owner = obj + opts.OwnerStrategy = start + }, + }, + } +} + +type combinedOpts struct { + optionFn + teardownOptionFn + fn func(opts *ComparatorOptions) +} + +func (copt *combinedOpts) ApplyToComparatorOptions(opts *ComparatorOptions) { + copt.fn(opts) +} + +type ComparatorOptions struct { + Owner client.Object + OwnerStrategy OwnerStrategy +} + +type ComparatorOption interface { + ApplyToComparatorOptions(opts *ComparatorOptions) +} + type optionFn struct { fn func(opts *ObjectReconcileOptions) } diff --git a/machinery/types/options_test.go b/machinery/types/options_test.go index 7bf54fa1..1cda382c 100644 --- a/machinery/types/options_test.go +++ b/machinery/types/options_test.go @@ -554,3 +554,226 @@ func TestInterfaceImplementations(t *testing.T) { var _ RevisionReconcileOption = WithPreviousOwners{} } + +func TestWithOrphan(t *testing.T) { + t.Parallel() + + orphanOpt := WithOrphan() + + t.Run("applies to object teardown options", func(t *testing.T) { + t.Parallel() + + opts := &ObjectTeardownOptions{} + orphanOpt.ApplyToObjectTeardownOptions(opts) + assert.True(t, opts.Orphan) + }) + + t.Run("applies to phase teardown options", func(t *testing.T) { + t.Parallel() + + opts := &PhaseTeardownOptions{} + orphanOpt.ApplyToPhaseTeardownOptions(opts) + require.Len(t, opts.DefaultObjectOptions, 1) + }) + + t.Run("applies to revision teardown options", func(t *testing.T) { + t.Parallel() + + opts := &RevisionTeardownOptions{} + orphanOpt.ApplyToRevisionTeardownOptions(opts) + require.Len(t, opts.DefaultPhaseOptions, 1) + }) +} + +func TestWithTeardownWriter(t *testing.T) { + t.Parallel() + + // Use nil writer for testing - we're only testing that the option sets the field + var writer client.Writer + + writerOpt := WithTeardownWriter(writer) + + t.Run("applies to object teardown options", func(t *testing.T) { + t.Parallel() + + opts := &ObjectTeardownOptions{} + writerOpt.ApplyToObjectTeardownOptions(opts) + assert.Equal(t, writer, opts.TeardownWriter) + }) + + t.Run("applies to phase teardown options", func(t *testing.T) { + t.Parallel() + + opts := &PhaseTeardownOptions{} + writerOpt.ApplyToPhaseTeardownOptions(opts) + require.Len(t, opts.DefaultObjectOptions, 1) + }) + + t.Run("applies to revision teardown options", func(t *testing.T) { + t.Parallel() + + opts := &RevisionTeardownOptions{} + writerOpt.ApplyToRevisionTeardownOptions(opts) + require.Len(t, opts.DefaultPhaseOptions, 1) + }) +} + +func TestRevisionReconcileOptions_GetOwner(t *testing.T) { + t.Parallel() + + t.Run("returns owner from default phase options", func(t *testing.T) { + t.Parallel() + + owner := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "owner", + Namespace: "test-ns", + UID: "test-uid", + }, + } + + mockStrat := &mockOwnerStrategy{} + ownerOpt := WithOwner(owner, mockStrat) + + opts := RevisionReconcileOptions{ + DefaultPhaseOptions: []PhaseReconcileOption{ownerOpt.(PhaseReconcileOption)}, + } + + gotOwner := opts.GetOwner() + assert.Equal(t, owner, gotOwner) + }) + + t.Run("returns nil when no owner is set", func(t *testing.T) { + t.Parallel() + + opts := RevisionReconcileOptions{} + + gotOwner := opts.GetOwner() + assert.Nil(t, gotOwner) + }) +} + +func TestWithOwner(t *testing.T) { + t.Parallel() + + owner := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "owner", + Namespace: "test-ns", + UID: "test-uid", + }, + } + + mockStrat := &mockOwnerStrategy{} + + t.Run("applies to object reconcile options", func(t *testing.T) { + t.Parallel() + + opts := &ObjectReconcileOptions{} + ownerOpt := WithOwner(owner, mockStrat) + ownerOpt.(ObjectReconcileOption).ApplyToObjectReconcileOptions(opts) + + assert.Equal(t, owner, opts.Owner) + assert.Equal(t, mockStrat, opts.OwnerStrategy) + }) + + t.Run("applies to object teardown options", func(t *testing.T) { + t.Parallel() + + opts := &ObjectTeardownOptions{} + ownerOpt := WithOwner(owner, mockStrat) + ownerOpt.(ObjectTeardownOption).ApplyToObjectTeardownOptions(opts) + + assert.Equal(t, owner, opts.Owner) + assert.Equal(t, mockStrat, opts.OwnerStrategy) + }) + + t.Run("applies to Comparator options", func(t *testing.T) { + t.Parallel() + + opts := &ComparatorOptions{} + ownerOpt := WithOwner(owner, mockStrat) + ownerOpt.(ComparatorOption).ApplyToComparatorOptions(opts) + + assert.Equal(t, owner, opts.Owner) + assert.Equal(t, mockStrat, opts.OwnerStrategy) + }) + + t.Run("applies to phase reconcile options via optionFn", func(t *testing.T) { + t.Parallel() + + opts := &PhaseReconcileOptions{} + ownerOpt := WithOwner(owner, mockStrat) + ownerOpt.(PhaseReconcileOption).ApplyToPhaseReconcileOptions(opts) + + require.Len(t, opts.DefaultObjectOptions, 1) + }) + + t.Run("applies to revision reconcile options via optionFn", func(t *testing.T) { + t.Parallel() + + opts := &RevisionReconcileOptions{} + ownerOpt := WithOwner(owner, mockStrat) + ownerOpt.(RevisionReconcileOption).ApplyToRevisionReconcileOptions(opts) + + require.Len(t, opts.DefaultPhaseOptions, 1) + }) + + t.Run("panics when owner has empty UID", func(t *testing.T) { + t.Parallel() + + invalidOwner := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "owner", + Namespace: "test-ns", + }, + } + + assert.Panics(t, func() { + WithOwner(invalidOwner, mockStrat) + }) + }) +} + +func TestObjectReconcileOptions_Default_Panic(t *testing.T) { + t.Parallel() + + t.Run("panics when owner is set without strategy", func(t *testing.T) { + t.Parallel() + + opts := &ObjectReconcileOptions{ + Owner: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "owner", + UID: "test-uid", + }, + }, + OwnerStrategy: nil, + } + + assert.Panics(t, func() { + opts.Default() + }) + }) +} + +// mockOwnerStrategy is a mock implementation of ownerStrategy for testing. +type mockOwnerStrategy struct{} + +func (m *mockOwnerStrategy) SetControllerReference(owner, obj metav1.Object) error { + return nil +} + +func (m *mockOwnerStrategy) GetController(obj metav1.Object) (metav1.OwnerReference, bool) { + return metav1.OwnerReference{}, false +} + +func (m *mockOwnerStrategy) IsController(owner, obj metav1.Object) bool { + return false +} + +func (m *mockOwnerStrategy) CopyOwnerReferences(objA, objB metav1.Object) {} + +func (m *mockOwnerStrategy) ReleaseController(obj metav1.Object) {} + +func (m *mockOwnerStrategy) RemoveOwner(owner, obj metav1.Object) {} diff --git a/machinery/types/types.go b/machinery/types/types.go index 175a4cc9..b2d54573 100644 --- a/machinery/types/types.go +++ b/machinery/types/types.go @@ -4,7 +4,6 @@ package types import ( "fmt" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/schema" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -17,6 +16,10 @@ type ObjectRef struct { // ToObjectRef returns an ObjectRef object from unstructured. func ToObjectRef(obj client.Object) ObjectRef { + if obj == nil { + return ObjectRef{} + } + return ObjectRef{ GroupVersionKind: obj.GetObjectKind().GroupVersionKind(), ObjectKey: client.ObjectKeyFromObject(obj), @@ -29,52 +32,196 @@ func (oid ObjectRef) String() string { } // Phase represents a named collection of objects. -type Phase struct { +type Phase interface { + // GetName returns the name of the phase. + GetName() string + // GetObjects returns the objects contained in the phase. + GetObjects() []client.Object + // GetReconcileOptions returns options for reconciling this phase. + GetReconcileOptions() []PhaseReconcileOption + // GetTeardownOptions returns options for tearing down this phase. + GetTeardownOptions() []PhaseTeardownOption +} + +// PhaseBuilder is a Phase with methods to attach options. +type PhaseBuilder interface { + Phase + // WithReconcileOptions sets PhaseReconcileOptions on this phase. + WithReconcileOptions(opts ...PhaseReconcileOption) PhaseBuilder + // WithTeardownOptions sets PhaseTeardownOption on this phase. + WithTeardownOptions(opts ...PhaseTeardownOption) PhaseBuilder +} + +// NewPhase creates a new PhaseBuilder with the given name and objects. +func NewPhase(name string, objects []client.Object) PhaseBuilder { + return &phase{ + Name: name, + Objects: objects, + } +} + +// NewPhaseWithOwner creates a new PhaseBuilder with the given name, objects and owner. +func NewPhaseWithOwner( + name string, objects []client.Object, + owner client.Object, ownerStrat OwnerStrategy, +) PhaseBuilder { + oo := WithOwner(owner, ownerStrat) + p := &phase{ + Name: name, + Objects: objects, + } + + return p.WithReconcileOptions(oo).WithTeardownOptions(oo) +} + +// phase represents a named collection of objects. +type phase struct { // Name of the Phase. Name string // Objects contained in the phase. - Objects []unstructured.Unstructured + Objects []client.Object + + ReconcileOptions []PhaseReconcileOption + TeardownOptions []PhaseTeardownOption } // GetName returns the name of the phase. -func (p *Phase) GetName() string { +func (p *phase) GetName() string { return p.Name } // GetObjects returns the objects contained in the phase. -func (p *Phase) GetObjects() []unstructured.Unstructured { +func (p *phase) GetObjects() []client.Object { return p.Objects } +// GetReconcileOptions returns options for reconciling this phase. +func (p *phase) GetReconcileOptions() []PhaseReconcileOption { + return p.ReconcileOptions +} + +// GetTeardownOptions returns options for tearing down this phase. +func (p *phase) GetTeardownOptions() []PhaseTeardownOption { + return p.TeardownOptions +} + +// WithReconcileOptions sets PhaseReconcileOptions on this phase. +func (p *phase) WithReconcileOptions(opts ...PhaseReconcileOption) PhaseBuilder { + p.ReconcileOptions = append(p.ReconcileOptions, opts...) + + return p +} + +// WithTeardownOptions sets PhaseTeardownOption on this phase. +func (p *phase) WithTeardownOptions(opts ...PhaseTeardownOption) PhaseBuilder { + p.TeardownOptions = append(p.TeardownOptions, opts...) + + return p +} + // Revision represents the version of a content collection consisting of phases. -type Revision struct { +type Revision interface { + // GetName returns the name of the revision. + GetName() string + // GetRevisionNumber returns the current revision number. + GetRevisionNumber() int64 + // GetPhases returns the phases a revision is made up of. + GetPhases() []Phase + // GetReconcileOptions returns options for reconciling this revision. + GetReconcileOptions() []RevisionReconcileOption + // GetTeardownOptions returns options for tearing down this revision. + GetTeardownOptions() []RevisionTeardownOption +} + +// RevisionBuilder is a Revision with methods to attach options. +type RevisionBuilder interface { + Revision + // WithReconcileOptions sets RevisionReconcileOptions on this revision. + WithReconcileOptions(opts ...RevisionReconcileOption) RevisionBuilder + // WithTeardownOptions sets RevisionTeardownOption on this revision. + WithTeardownOptions(opts ...RevisionTeardownOption) RevisionBuilder +} + +// NewRevision creates a new RevisionBuilder with +// the given name, rev and phases. +func NewRevision( + name string, + revNumber int64, + phases []Phase, +) RevisionBuilder { + return &revision{ + Name: name, + Revision: revNumber, + Phases: phases, + } +} + +// NewRevisionWithOwner creates a new RevisionBuilder +// with the given name, rev, phases and owner. +func NewRevisionWithOwner( + name string, + revNumber int64, + phases []Phase, + owner client.Object, ownerStrat OwnerStrategy, +) RevisionBuilder { + oo := WithOwner(owner, ownerStrat) + r := &revision{ + Name: name, + Revision: revNumber, + Phases: phases, + } + + return r.WithReconcileOptions(oo).WithTeardownOptions(oo) +} + +// revision represents the version of a content collection consisting of phases. +type revision struct { // Name of the Revision. Name string - // Owner object will be added as OwnerReference - // to all objects managed by this revision. - Owner client.Object // Revision number. Revision int64 // Ordered list of phases. Phases []Phase + + ReconcileOptions []RevisionReconcileOption + TeardownOptions []RevisionTeardownOption } // GetName returns the name of the revision. -func (r *Revision) GetName() string { +func (r *revision) GetName() string { return r.Name } -// GetOwner returns the owning object. -func (r *Revision) GetOwner() client.Object { - return r.Owner -} - // GetRevisionNumber returns the current revision number. -func (r *Revision) GetRevisionNumber() int64 { +func (r *revision) GetRevisionNumber() int64 { return r.Revision } // GetPhases returns the phases a revision is made up of. -func (r *Revision) GetPhases() []Phase { +func (r *revision) GetPhases() []Phase { return r.Phases } + +// GetReconcileOptions returns options for reconciling this revision. +func (r *revision) GetReconcileOptions() []RevisionReconcileOption { + return r.ReconcileOptions +} + +// GetTeardownOptions returns options for tearing down this revision. +func (r *revision) GetTeardownOptions() []RevisionTeardownOption { + return r.TeardownOptions +} + +// WithReconcileOptions sets RevisionReconcileOptions on this revision. +func (r *revision) WithReconcileOptions(opts ...RevisionReconcileOption) RevisionBuilder { + r.ReconcileOptions = opts + + return r +} + +// WithTeardownOptions sets RevisionTeardownOption on this revision. +func (r *revision) WithTeardownOptions(opts ...RevisionTeardownOption) RevisionBuilder { + r.TeardownOptions = opts + + return r +} diff --git a/machinery/types/types_test.go b/machinery/types/types_test.go index 861e0ce2..86ab3c34 100644 --- a/machinery/types/types_test.go +++ b/machinery/types/types_test.go @@ -67,6 +67,11 @@ func TestToObjectRef(t *testing.T) { }, }, }, + { + name: "nil object", + obj: nil, + want: ObjectRef{}, + }, } for _, tt := range tests { @@ -146,20 +151,17 @@ func TestObjectRef_String(t *testing.T) { func TestPhase_GetName(t *testing.T) { t.Parallel() - phase := &Phase{ - Name: "test-phase", - Objects: []unstructured.Unstructured{ - { - Object: map[string]interface{}{ - "apiVersion": "v1", - "kind": "ConfigMap", - "metadata": map[string]interface{}{ - "name": "test-cm", - }, + phase := NewPhase("test-phase", []client.Object{ + &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": map[string]interface{}{ + "name": "test-cm", }, }, }, - } + }) assert.Equal(t, "test-phase", phase.GetName()) } @@ -167,8 +169,8 @@ func TestPhase_GetName(t *testing.T) { func TestPhase_GetObjects(t *testing.T) { t.Parallel() - objects := []unstructured.Unstructured{ - { + objects := []client.Object{ + &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "v1", "kind": "ConfigMap", @@ -177,7 +179,7 @@ func TestPhase_GetObjects(t *testing.T) { }, }, }, - { + &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "v1", "kind": "Secret", @@ -188,10 +190,9 @@ func TestPhase_GetObjects(t *testing.T) { }, } - phase := &Phase{ - Name: "test-phase", - Objects: objects, - } + phase := NewPhase( + "test-phase", objects, + ) got := phase.GetObjects() assert.Equal(t, objects, got) @@ -201,36 +202,17 @@ func TestPhase_GetObjects(t *testing.T) { func TestRevision_GetName(t *testing.T) { t.Parallel() - revision := &Revision{ - Name: "test-revision", - } + revision := NewRevision( + "test-revision", 1, nil, + ) assert.Equal(t, "test-revision", revision.GetName()) } -func TestRevision_GetOwner(t *testing.T) { - t.Parallel() - - owner := &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: "owner", - Namespace: "test", - }, - } - - revision := &Revision{ - Owner: owner, - } - - assert.Equal(t, owner, revision.GetOwner()) -} - func TestRevision_GetRevisionNumber(t *testing.T) { t.Parallel() - revision := &Revision{ - Revision: 42, - } + revision := NewRevision("test-revision", 42, nil) assert.Equal(t, int64(42), revision.GetRevisionNumber()) } @@ -239,10 +221,10 @@ func TestRevision_GetPhases(t *testing.T) { t.Parallel() phases := []Phase{ - { + &phase{ Name: "phase1", - Objects: []unstructured.Unstructured{ - { + Objects: []client.Object{ + &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "v1", "kind": "ConfigMap", @@ -253,10 +235,10 @@ func TestRevision_GetPhases(t *testing.T) { }, }, }, - { + &phase{ Name: "phase2", - Objects: []unstructured.Unstructured{ - { + Objects: []client.Object{ + &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "v1", "kind": "Secret", @@ -269,11 +251,197 @@ func TestRevision_GetPhases(t *testing.T) { }, } - revision := &Revision{ - Phases: phases, - } + revision := NewRevision( + "test", 2, phases, + ) got := revision.GetPhases() assert.Equal(t, phases, got) assert.Len(t, got, 2) } + +func TestPhase_WithReconcileOptions(t *testing.T) { + t.Parallel() + + phase := NewPhase("test-phase", []client.Object{}) + + opts := []PhaseReconcileOption{ + WithCollisionProtection(CollisionProtectionNone), + } + + result := phase.WithReconcileOptions(opts...) + + assert.Equal(t, phase, result, "should return same phase for chaining") + assert.Equal(t, opts, phase.GetReconcileOptions()) +} + +func TestPhase_WithTeardownOptions(t *testing.T) { + t.Parallel() + + phase := NewPhase("test-phase", []client.Object{}) + + opts := []PhaseTeardownOption{} + + result := phase.WithTeardownOptions(opts...) + + assert.Equal(t, phase, result, "should return same phase for chaining") + assert.Empty(t, phase.GetTeardownOptions()) + + chainedOpts := []PhaseTeardownOption{ + WithTeardownWriter(nil), + } + + result = phase.WithTeardownOptions(chainedOpts...) + assert.Equal(t, chainedOpts, result.GetTeardownOptions()) +} + +func TestPhase_GetReconcileOptions(t *testing.T) { + t.Parallel() + + opts := []PhaseReconcileOption{ + WithCollisionProtection(CollisionProtectionNone), + } + + phase := NewPhase("test-phase", []client.Object{}). + WithReconcileOptions(opts...) + + assert.Equal(t, opts, phase.GetReconcileOptions()) +} + +func TestPhase_GetTeardownOptions(t *testing.T) { + t.Parallel() + + opts := []PhaseTeardownOption{} + + phase := NewPhase("test-phase", []client.Object{}). + WithTeardownOptions(opts...) + + assert.Empty(t, phase.GetTeardownOptions()) +} + +func TestNewPhaseWithOwner(t *testing.T) { + t.Parallel() + + owner := &corev1.ConfigMap{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "v1", + Kind: "ConfigMap", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "owner", + Namespace: "test-ns", + UID: "test-uid", + }, + } + + objects := []client.Object{ + &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": map[string]interface{}{ + "name": "test-cm", + }, + }, + }, + } + + mockStrat := &mockOwnerStrategy{} + + phase := NewPhaseWithOwner("test-phase", objects, owner, mockStrat) + + assert.Equal(t, "test-phase", phase.GetName()) + assert.Equal(t, objects, phase.GetObjects()) + + reconcileOpts := phase.GetReconcileOptions() + assert.NotEmpty(t, reconcileOpts) + + teardownOpts := phase.GetTeardownOptions() + assert.NotEmpty(t, teardownOpts) +} + +func TestRevision_WithReconcileOptions(t *testing.T) { + t.Parallel() + + revision := NewRevision("test-revision", 1, nil) + + opts := []RevisionReconcileOption{ + WithCollisionProtection(CollisionProtectionNone), + } + + result := revision.WithReconcileOptions(opts...) + + assert.Equal(t, revision, result, "should return same revision for chaining") + assert.Equal(t, opts, revision.GetReconcileOptions()) +} + +func TestRevision_WithTeardownOptions(t *testing.T) { + t.Parallel() + + revision := NewRevision("test-revision", 1, nil) + + opts := []RevisionTeardownOption{} + + result := revision.WithTeardownOptions(opts...) + + assert.Equal(t, revision, result, "should return same revision for chaining") + assert.Equal(t, opts, revision.GetTeardownOptions()) +} + +func TestRevision_GetReconcileOptions(t *testing.T) { + t.Parallel() + + opts := []RevisionReconcileOption{ + WithCollisionProtection(CollisionProtectionNone), + } + + revision := NewRevision("test-revision", 1, nil). + WithReconcileOptions(opts...) + + assert.Equal(t, opts, revision.GetReconcileOptions()) +} + +func TestRevision_GetTeardownOptions(t *testing.T) { + t.Parallel() + + opts := []RevisionTeardownOption{} + + revision := NewRevision("test-revision", 1, nil). + WithTeardownOptions(opts...) + + assert.Equal(t, opts, revision.GetTeardownOptions()) +} + +func TestNewRevisionWithOwner(t *testing.T) { + t.Parallel() + + owner := &corev1.ConfigMap{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "v1", + Kind: "ConfigMap", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "owner", + Namespace: "test-ns", + UID: "test-uid", + }, + } + + phases := []Phase{ + NewPhase("phase1", []client.Object{}), + } + + mockStrat := &mockOwnerStrategy{} + + revision := NewRevisionWithOwner("test-revision", 1, phases, owner, mockStrat) + + assert.Equal(t, "test-revision", revision.GetName()) + assert.Equal(t, int64(1), revision.GetRevisionNumber()) + assert.Equal(t, phases, revision.GetPhases()) + + reconcileOpts := revision.GetReconcileOptions() + assert.NotEmpty(t, reconcileOpts) + + teardownOpts := revision.GetTeardownOptions() + assert.NotEmpty(t, teardownOpts) +} diff --git a/test/comparator_test.go b/test/comparator_test.go index 60757c5b..4dbfb03f 100644 --- a/test/comparator_test.go +++ b/test/comparator_test.go @@ -17,6 +17,7 @@ import ( "sigs.k8s.io/structured-merge-diff/v6/fieldpath" "pkg.package-operator.run/boxcutter/machinery" + bctypes "pkg.package-operator.run/boxcutter/machinery/types" "pkg.package-operator.run/boxcutter/ownerhandling" ) @@ -36,7 +37,7 @@ func sanitizeCompareResult(r *machinery.CompareResult) { 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{ @@ -390,7 +391,7 @@ Comparison: require.NoError(t, err) } - r, err := comp.Compare(owner, test.desiredObj, test.actualObj) + r, err := comp.Compare(test.desiredObj, test.actualObj, bctypes.WithOwner(owner, os)) require.NoError(t, err) sanitizeCompareResult(&r) diff --git a/test/objects_test.go b/test/objects_test.go index cd611822..b49004bd 100644 --- a/test/objects_test.go +++ b/test/objects_test.go @@ -20,9 +20,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() @@ -59,7 +59,7 @@ func TestObjectEngine(t *testing.T) { } // Creation - res, err := oe.Reconcile(ctx, owner, 1, configMap) + res, err := oe.Reconcile(ctx, 1, configMap, types.WithOwner(owner, os)) require.NoError(t, err) assert.Equal(t, `Object ConfigMap.v1 default/oe-test Action: "Created" @@ -69,7 +69,7 @@ Action: "Created" assert.False(t, res.IsPaused(), "IsPaused") // Idle - res, err = oe.Reconcile(ctx, owner, 1, configMap) + res, err = oe.Reconcile(ctx, 1, configMap, types.WithOwner(owner, os)) require.NoError(t, err) assert.Equal(t, `Object ConfigMap.v1 default/oe-test Action: "Idle" @@ -88,7 +88,7 @@ Action: "Idle" require.NoError(t, err) // Idle with other participant. - res, err = oe.Reconcile(ctx, owner, 1, configMap) + res, err = oe.Reconcile(ctx, 1, configMap, types.WithOwner(owner, os)) require.NoError(t, err) assert.Equal(t, `Object ConfigMap.v1 default/oe-test Action: "Idle" @@ -113,7 +113,7 @@ Comparison: }, "data") require.NoError(t, err) - res, err = oe.Reconcile(ctx, owner, 1, configMap) + res, err = oe.Reconcile(ctx, 1, configMap, types.WithOwner(owner, os)) require.NoError(t, err) assert.Equal(t, `Object ConfigMap.v1 default/oe-test Action: "Updated" @@ -134,20 +134,20 @@ 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, 1, configMap, types.WithOwner(owner, os)) require.NoError(t, err) assert.False(t, gone) - gone, err = oe.Teardown(ctx, owner, 1, configMap) + gone, err = oe.Teardown(ctx, 1, configMap, types.WithOwner(owner, os)) 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() @@ -185,7 +185,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, 1, configMap, types.WithPaused{}, types.WithOwner(owner, os)) require.NoError(t, err) assert.Equal(t, `Object ConfigMap.v1 default/oe-test-paused Action (PAUSED): "Created" @@ -199,7 +199,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, 1, configMap, types.WithOwner(owner, os)) require.NoError(t, err) assert.Equal(t, `Object ConfigMap.v1 default/oe-test-paused Action: "Created" @@ -208,7 +208,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, 1, configMap, types.WithPaused{}, types.WithOwner(owner, os)) require.NoError(t, err) assert.Equal(t, `Object ConfigMap.v1 default/oe-test-paused Action (PAUSED): "Idle" @@ -227,7 +227,7 @@ Action (PAUSED): "Idle" }, "data") require.NoError(t, err) - res, err = oe.Reconcile(ctx, owner, 1, configMap, types.WithPaused{}) + res, err = oe.Reconcile(ctx, 1, configMap, types.WithPaused{}, types.WithOwner(owner, os)) require.NoError(t, err) assert.Equal(t, `Object ConfigMap.v1 default/oe-test-paused Action (PAUSED): "Updated" @@ -254,20 +254,20 @@ 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, 1, configMap, types.WithOwner(owner, os)) require.NoError(t, err) assert.False(t, gone) - gone, err = oe.Teardown(ctx, owner, 1, configMap) + gone, err = oe.Teardown(ctx, 1, configMap, types.WithOwner(owner, os)) 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() @@ -308,9 +308,10 @@ func TestObjectEngineProbing(t *testing.T) { } // Creation progress probe fails res, err := oe.Reconcile( - ctx, owner, 1, configMap, + ctx, 1, configMap, types.WithProbe(types.ProgressProbeType, probeFailed), types.WithProbe("other", probeSuccess), + types.WithOwner(owner, os), ) require.NoError(t, err) assert.Equal(t, `Object ConfigMap.v1 default/oe-test-probing @@ -326,9 +327,10 @@ Probes: // Idle progress probe unknown res, err = oe.Reconcile( - ctx, owner, 1, configMap, + ctx, 1, configMap, types.WithProbe(types.ProgressProbeType, probeUnknown), types.WithProbe("other", probeSuccess), + types.WithOwner(owner, os), ) require.NoError(t, err) assert.Equal(t, `Object ConfigMap.v1 default/oe-test-probing @@ -344,9 +346,10 @@ Probes: // Idle progress probe success res, err = oe.Reconcile( - ctx, owner, 1, configMap, + ctx, 1, configMap, types.WithProbe(types.ProgressProbeType, probeSuccess), types.WithProbe("other", probeSuccess), + types.WithOwner(owner, os), ) require.NoError(t, err) assert.Equal(t, `Object ConfigMap.v1 default/oe-test-probing @@ -361,11 +364,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, 1, configMap, types.WithOwner(owner, os)) require.NoError(t, err) assert.False(t, gone) - gone, err = oe.Teardown(ctx, owner, 1, configMap) + gone, err = oe.Teardown(ctx, 1, configMap, types.WithOwner(owner, os)) 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 17259578..284b8949 100644 --- a/test/revision_engine_basic_test.go +++ b/test/revision_engine_basic_test.go @@ -52,26 +52,10 @@ func TestRevisionEngine(t *testing.T) { }, }, } - rev := boxcutter.Revision{ - Name: "rev-1", - Owner: revOwner, - Revision: 1, - Phases: []boxcutter.Phase{ - { - Name: "phase-1", - Objects: []unstructured.Unstructured{*obj1}, - }, - { - Name: "phase-2", - 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) pe := machinery.NewPhaseEngine(oe, pval) @@ -87,6 +71,22 @@ func TestRevisionEngine(t *testing.T) { require.NoError(t, Client.Delete(context.Background(), revOwner)) }) + os := ownerhandling.NewNative(Scheme) + rev := boxcutter.NewRevisionWithOwner( + "rev-1", 1, + []boxcutter.Phase{ + boxcutter.NewPhase( + "phase-1", + []client.Object{obj1}, + ), + boxcutter.NewPhase( + "phase-2", + []client.Object{obj2}, + ), + }, + revOwner, os, + ) + // Test execution // -------------- diff --git a/test/revision_engine_colision_protection_test.go b/test/revision_engine_colision_protection_test.go index f07c8b38..870e1d10 100644 --- a/test/revision_engine_colision_protection_test.go +++ b/test/revision_engine_colision_protection_test.go @@ -11,13 +11,13 @@ import ( "github.com/stretchr/testify/require" 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/schema" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "pkg.package-operator.run/boxcutter" "pkg.package-operator.run/boxcutter/machinery" - "pkg.package-operator.run/boxcutter/machinery/types" + "pkg.package-operator.run/boxcutter/ownerhandling" ) func TestCollisionProtectionPreventUnowned(t *testing.T) { @@ -38,19 +38,18 @@ func TestCollisionProtectionPreventUnowned(t *testing.T) { colliding.Data["apple"] = "pie" re := newTestRevisionEngine() - res, err := re.Reconcile(ctx, types.Revision{ - Name: "test-collision-prevention-prevent-unowned-cm", - Revision: 1, - Owner: owner, - Phases: []types.Phase{ - { - Name: "simple", - Objects: []unstructured.Unstructured{ + res, err := re.Reconcile(ctx, boxcutter.NewRevisionWithOwner( + "test-collision-prevention-prevent-unowned-cm", 1, + []boxcutter.Phase{ + boxcutter.NewPhase( + "simple", + []client.Object{ toUns(colliding), }, - }, + ), }, - }) + owner, ownerhandling.NewNative(Scheme), + )) require.NoError(t, err) assert.False(t, res.IsComplete()) assert.True(t, res.InTransition()) @@ -90,19 +89,18 @@ func TestCollisionProtectionPreventOwned(t *testing.T) { cleanupOnSuccess(t, owner) re := newTestRevisionEngine() - res, err := re.Reconcile(ctx, types.Revision{ - Name: "test-collision-prevention-prevent-owned-cm", - Revision: 1, - Owner: owner, - Phases: []types.Phase{ - { - Name: "simple", - Objects: []unstructured.Unstructured{ + res, err := re.Reconcile(ctx, boxcutter.NewRevisionWithOwner( + "test-collision-prevention-prevent-owned-cm", 1, + []boxcutter.Phase{ + boxcutter.NewPhase( + "simple", + []client.Object{ toUns(colliding), }, - }, + ), }, - }) + owner, ownerhandling.NewNative(Scheme), + )) require.NoError(t, err) assert.False(t, res.IsComplete()) assert.True(t, res.InTransition()) diff --git a/test/revision_engine_preflight_validation_test.go b/test/revision_engine_preflight_validation_test.go index 2c9899d8..8f48d2ab 100644 --- a/test/revision_engine_preflight_validation_test.go +++ b/test/revision_engine_preflight_validation_test.go @@ -10,11 +10,12 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" + "pkg.package-operator.run/boxcutter" "pkg.package-operator.run/boxcutter/machinery/types" + "pkg.package-operator.run/boxcutter/ownerhandling" "pkg.package-operator.run/boxcutter/validation" ) @@ -47,19 +48,18 @@ func TestWithOwnerReference(t *testing.T) { } re := newTestRevisionEngine() - res, err := re.Reconcile(ctx, types.Revision{ - Name: "test-collision-prevention-invalid-set", - Revision: 1, - Owner: owner, - Phases: []types.Phase{ - { - Name: "simple", - Objects: []unstructured.Unstructured{ + res, err := re.Reconcile(ctx, boxcutter.NewRevisionWithOwner( + "test-collision-prevention-invalid-set", 1, + []boxcutter.Phase{ + boxcutter.NewPhase( + "simple", + []client.Object{ toUns(invalid), }, - }, + ), }, - }) + owner, ownerhandling.NewNative(Scheme), + )) require.NoError(t, err) assert.False(t, res.IsComplete()) diff --git a/test/test_util.go b/test/test_util.go index 9448650a..20625d5c 100644 --- a/test/test_util.go +++ b/test/test_util.go @@ -47,12 +47,12 @@ func mustGVKForObject(obj client.Object) schema.GroupVersionKind { // Converts the given client.Object to an unstructured object. // Panics if this fails. -func toUns(obj client.Object) unstructured.Unstructured { +func toUns(obj client.Object) *unstructured.Unstructured { must(setTypeMeta(obj, Scheme)) raw, err := runtime.DefaultUnstructuredConverter.ToUnstructured(obj) must(err) - return unstructured.Unstructured{ + return &unstructured.Unstructured{ Object: raw, } } diff --git a/validation/metadata.go b/validation/metadata.go index 9fb4367a..4393c12a 100644 --- a/validation/metadata.go +++ b/validation/metadata.go @@ -1,15 +1,15 @@ package validation import ( - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/util/validation/field" + "sigs.k8s.io/controller-runtime/pkg/client" ) -func validateObjectMetadata(obj *unstructured.Unstructured) []error { +func validateObjectMetadata(obj client.Object) []error { var errs []error // Type Meta - if len(obj.GetAPIVersion()) == 0 { + if len(obj.GetObjectKind().GroupVersionKind().Version) == 0 { errs = append(errs, field.Required( field.NewPath("apiVersion"), @@ -17,7 +17,7 @@ func validateObjectMetadata(obj *unstructured.Unstructured) []error { )) } - if len(obj.GetKind()) == 0 { + if len(obj.GetObjectKind().GroupVersionKind().Kind) == 0 { errs = append(errs, field.Required( field.NewPath("kind"), diff --git a/validation/object.go b/validation/object.go index fea52f47..2c068a66 100644 --- a/validation/object.go +++ b/validation/object.go @@ -11,7 +11,6 @@ import ( 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/schema" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" @@ -63,16 +62,22 @@ 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, - obj *unstructured.Unstructured, + ctx context.Context, + obj client.Object, + opts ...bctypes.ObjectReconcileOption, ) error { + var options bctypes.ObjectReconcileOptions + for _, opt := range opts { + opt.ApplyToObjectReconcileOptions(&options) + } + // Static metadata validation. errs := validateObjectMetadata(obj) - if !d.allowNamespaceEscalation { + if options.Owner != nil && !d.allowNamespaceEscalation { // Ensure we are not leaving the namespace we are operating in. if err := validateNamespace( - d.restMapper, owner.GetNamespace(), obj, + d.restMapper, options.Owner.GetNamespace(), obj, ); err != nil { errs = append(errs, err) // we don't want to do a dry-run when this already fails. @@ -116,7 +121,7 @@ func (e MustBeInNamespaceError) Error() string { func validateNamespace( restMapper restMapper, namespace string, - obj *unstructured.Unstructured, + obj client.Object, ) error { // shortcut if Namespaces are not limited. if len(namespace) == 0 { @@ -167,7 +172,7 @@ func (e DryRunValidationError) Unwrap() error { func validateDryRun( ctx context.Context, w client.Writer, - obj *unstructured.Unstructured, + obj client.Object, ) error { objectPatch, mErr := json.Marshal(obj) if mErr != nil { @@ -175,7 +180,7 @@ func validateDryRun( } patch := client.RawPatch(types.ApplyPatchType, objectPatch) - dst := obj.DeepCopyObject().(*unstructured.Unstructured) + dst := obj.DeepCopyObject().(client.Object) err := w.Patch(ctx, dst, patch, client.FieldOwner("dummy"), client.ForceOwnership, client.DryRunAll) if apimachineryerrors.IsNotFound(err) { diff --git a/validation/object_test.go b/validation/object_test.go index fc04525b..17aa3066 100644 --- a/validation/object_test.go +++ b/validation/object_test.go @@ -15,6 +15,8 @@ import ( "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" ) type mockRestMapper struct { @@ -127,6 +129,7 @@ func TestObjectValidator_Validate(t *testing.T) { Object: map[string]interface{}{ "metadata": map[string]interface{}{ "namespace": "default", + "uid": "234", }, }, }, @@ -153,6 +156,7 @@ func TestObjectValidator_Validate(t *testing.T) { Object: map[string]interface{}{ "metadata": map[string]interface{}{ "namespace": "default", + "uid": "234", }, }, }, @@ -181,6 +185,7 @@ func TestObjectValidator_Validate(t *testing.T) { Object: map[string]interface{}{ "metadata": map[string]interface{}{ "namespace": "default", + "uid": "234", }, }, }, @@ -208,6 +213,7 @@ func TestObjectValidator_Validate(t *testing.T) { Object: map[string]interface{}{ "metadata": map[string]interface{}{ "namespace": "default", + "uid": "234", }, }, }, @@ -238,7 +244,7 @@ func TestObjectValidator_Validate(t *testing.T) { allowNamespaceEscalation: test.allowNamespaceEscalation, } - err := validator.Validate(t.Context(), test.owner, test.obj) + err := validator.Validate(t.Context(), test.obj, types.WithOwner(test.owner, nil)) if test.expectError { require.Error(t, err) diff --git a/validation/phase.go b/validation/phase.go index 8c447ecf..06c385a8 100644 --- a/validation/phase.go +++ b/validation/phase.go @@ -41,8 +41,13 @@ func NewNamespacedPhaseValidator( // 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, phase types.Phase, opts ...types.PhaseReconcileOption, ) error { + var options types.PhaseReconcileOptions + for _, opt := range opts { + opt.ApplyToPhaseReconcileOptions(&options) + } + phaseError := validatePhaseName(phase) var ( @@ -50,10 +55,8 @@ func (v *PhaseValidator) Validate( errs []error ) - for _, o := range phase.GetObjects() { - obj := &o - - err := v.ObjectValidator.Validate(ctx, owner, obj) + for _, obj := range phase.GetObjects() { + err := v.ObjectValidator.Validate(ctx, obj, options.ForObject(obj)...) if err == nil { continue } @@ -121,13 +124,12 @@ func checkForObjectDuplicates(phases ...types.Phase) []ObjectValidationError { conflicts := map[types.ObjectRef]map[string]struct{}{} for _, phase := range phases { - for _, o := range phase.GetObjects() { - obj := &o + for _, obj := range phase.GetObjects() { ref := types.ToObjectRef(obj) otherPhase, ok := uniqueObjectsInPhase[ref] if !ok { - uniqueObjectsInPhase[ref] = phase.Name + uniqueObjectsInPhase[ref] = phase.GetName() continue } diff --git a/validation/phase_test.go b/validation/phase_test.go index 8ce3e48e..b218cf8b 100644 --- a/validation/phase_test.go +++ b/validation/phase_test.go @@ -19,7 +19,7 @@ type mockObjectValidator struct { mock.Mock } -func (m *mockObjectValidator) Validate(ctx context.Context, owner client.Object, obj *unstructured.Unstructured) error { +func (m *mockObjectValidator) Validate(ctx context.Context, owner client.Object, obj client.Object) error { args := m.Called(ctx, owner, obj) return args.Error(0) @@ -38,9 +38,7 @@ func (v *testablePhaseValidator) Validate(ctx context.Context, owner client.Obje errs []error ) - for _, o := range phase.GetObjects() { - obj := &o - + for _, obj := range phase.GetObjects() { err := v.mockObjValidator.Validate(ctx, owner, obj) if err == nil { continue @@ -106,10 +104,10 @@ func TestPhaseValidator_Validate(t *testing.T) { }{ { name: "valid phase", - phase: types.Phase{ - Name: "valid-phase", - Objects: []unstructured.Unstructured{ - { + phase: types.NewPhase( + "valid-phase", + []client.Object{ + &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "v1", "kind": "ConfigMap", @@ -120,7 +118,7 @@ func TestPhaseValidator_Validate(t *testing.T) { }, }, }, - }, + ), owner: &unstructured.Unstructured{ Object: map[string]interface{}{ "metadata": map[string]interface{}{ @@ -135,10 +133,10 @@ func TestPhaseValidator_Validate(t *testing.T) { }, { name: "invalid phase name", - phase: types.Phase{ - Name: "Invalid_Phase_Name", - Objects: []unstructured.Unstructured{ - { + phase: types.NewPhase( + "Invalid_Phase_Name", + []client.Object{ + &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "v1", "kind": "ConfigMap", @@ -149,7 +147,7 @@ func TestPhaseValidator_Validate(t *testing.T) { }, }, }, - }, + ), owner: &unstructured.Unstructured{ Object: map[string]interface{}{ "metadata": map[string]interface{}{ @@ -165,10 +163,10 @@ func TestPhaseValidator_Validate(t *testing.T) { }, { name: "object validation error from mock", - phase: types.Phase{ - Name: "valid-phase", - Objects: []unstructured.Unstructured{ - { + phase: types.NewPhase( + "valid-phase", + []client.Object{ + &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "v1", "kind": "ConfigMap", @@ -179,7 +177,7 @@ func TestPhaseValidator_Validate(t *testing.T) { }, }, }, - }, + ), owner: &unstructured.Unstructured{ Object: map[string]interface{}{ "metadata": map[string]interface{}{ @@ -200,10 +198,10 @@ func TestPhaseValidator_Validate(t *testing.T) { }, { name: "unknown error during object validation", - phase: types.Phase{ - Name: "valid-phase", - Objects: []unstructured.Unstructured{ - { + phase: types.NewPhase( + "valid-phase", + []client.Object{ + &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "v1", "kind": "ConfigMap", @@ -214,7 +212,7 @@ func TestPhaseValidator_Validate(t *testing.T) { }, }, }, - }, + ), owner: &unstructured.Unstructured{ Object: map[string]interface{}{ "metadata": map[string]interface{}{ @@ -231,10 +229,10 @@ func TestPhaseValidator_Validate(t *testing.T) { }, { name: "duplicate objects in same phase", - phase: types.Phase{ - Name: "valid-phase", - Objects: []unstructured.Unstructured{ - { + phase: types.NewPhase( + "valid-phase", + []client.Object{ + &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "v1", "kind": "ConfigMap", @@ -244,7 +242,7 @@ func TestPhaseValidator_Validate(t *testing.T) { }, }, }, - { + &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "v1", "kind": "ConfigMap", @@ -255,7 +253,7 @@ func TestPhaseValidator_Validate(t *testing.T) { }, }, }, - }, + ), owner: &unstructured.Unstructured{ Object: map[string]interface{}{ "metadata": map[string]interface{}{ @@ -288,7 +286,7 @@ func TestPhaseValidator_Validate(t *testing.T) { 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) + err = realValidator.Validate(t.Context(), test.phase, types.WithOwner(test.owner, nil)) } else { objValidator = &mockObjectValidator{} test.mockSetup(objValidator) @@ -340,38 +338,28 @@ func TestValidatePhaseName(t *testing.T) { expectError bool }{ { - name: "valid phase name", - phase: types.Phase{ - Name: "valid-phase-name", - }, + name: "valid phase name", + phase: types.NewPhase("valid-phase-name", nil), expectError: false, }, { - name: "invalid phase name with uppercase", - phase: types.Phase{ - Name: "Invalid-Phase-Name", - }, + name: "invalid phase name with uppercase", + phase: types.NewPhase("Invalid-Phase-Name", nil), expectError: true, }, { - name: "invalid phase name with underscores", - phase: types.Phase{ - Name: "invalid_phase_name", - }, + name: "invalid phase name with underscores", + phase: types.NewPhase("invalid_phase_name", nil), expectError: true, }, { - name: "invalid phase name too long", - phase: types.Phase{ - Name: "this-is-a-very-long-phase-name-that-exceeds-the-dns1035-label-length-limit-of-63-characters", - }, + name: "invalid phase name too long", + phase: types.NewPhase("this-is-a-very-long-phase-name-that-exceeds-the-dns1035-label-length-limit-of-63-characters", nil), expectError: true, }, { - name: "empty phase name", - phase: types.Phase{ - Name: "", - }, + name: "empty phase name", + phase: types.NewPhase("", nil), expectError: true, }, } @@ -483,52 +471,31 @@ func TestCheckForObjectDuplicates(t *testing.T) { { name: "no duplicates", phases: []types.Phase{ - { - Name: "phase1", - Objects: []unstructured.Unstructured{obj1}, - }, - { - Name: "phase2", - Objects: []unstructured.Unstructured{obj2}, - }, + types.NewPhase("phase1", []client.Object{&obj1}), + types.NewPhase("phase2", []client.Object{&obj2}), }, expectedConflicts: 0, }, { name: "duplicate across phases", phases: []types.Phase{ - { - Name: "phase1", - Objects: []unstructured.Unstructured{obj1}, - }, - { - Name: "phase2", - Objects: []unstructured.Unstructured{obj1}, - }, + types.NewPhase("phase1", []client.Object{&obj1}), + types.NewPhase("phase2", []client.Object{&obj1}), }, expectedConflicts: 1, }, { name: "multiple duplicates", phases: []types.Phase{ - { - Name: "phase1", - Objects: []unstructured.Unstructured{obj1, obj2}, - }, - { - Name: "phase2", - Objects: []unstructured.Unstructured{obj1, obj2}, - }, + types.NewPhase("phase1", []client.Object{&obj1, &obj2}), + types.NewPhase("phase2", []client.Object{&obj1, &obj2}), }, expectedConflicts: 2, }, { name: "duplicate in same phase", phases: []types.Phase{ - { - Name: "phase1", - Objects: []unstructured.Unstructured{obj1, obj1}, - }, + types.NewPhase("phase1", []client.Object{&obj1, &obj1}), }, expectedConflicts: 1, }, diff --git a/validation/revision.go b/validation/revision.go index 96c0dc2b..9dd180b9 100644 --- a/validation/revision.go +++ b/validation/revision.go @@ -39,10 +39,10 @@ func staticValidateMultiplePhases(phases ...types.Phase) []PhaseValidationError for _, phase := range phases { var objectErrors []ObjectValidationError - for _, obj := range phase.Objects { + for _, obj := range phase.GetObjects() { oe := NewObjectValidationError( - types.ToObjectRef(&obj), - validateObjectMetadata(&obj)..., + types.ToObjectRef(obj), + validateObjectMetadata(obj)..., ) if oe == nil { continue diff --git a/validation/revision_test.go b/validation/revision_test.go index 4cfc1b2e..2ff4c55e 100644 --- a/validation/revision_test.go +++ b/validation/revision_test.go @@ -6,6 +6,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "sigs.k8s.io/controller-runtime/pkg/client" "pkg.package-operator.run/boxcutter/machinery/types" ) @@ -29,14 +30,13 @@ func TestRevisionValidator_Validate(t *testing.T) { }{ { name: "valid revision", - revision: types.Revision{ - Name: "test-revision", - Revision: 1, - Phases: []types.Phase{ - { - Name: "phase1", - Objects: []unstructured.Unstructured{ - { + revision: types.NewRevision( + "test-revision", 1, + []types.Phase{ + types.NewPhase( + "phase1", + []client.Object{ + &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "v1", "kind": "ConfigMap", @@ -47,11 +47,11 @@ func TestRevisionValidator_Validate(t *testing.T) { }, }, }, - }, - { - Name: "phase2", - Objects: []unstructured.Unstructured{ - { + ), + types.NewPhase( + "phase2", + []client.Object{ + &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "v1", "kind": "ConfigMap", @@ -62,21 +62,20 @@ 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{ - { - Name: "Invalid_Phase_Name", - Objects: []unstructured.Unstructured{ - { + revision: types.NewRevision( + "test-revision", 1, + []types.Phase{ + types.NewPhase( + "Invalid_Phase_Name", + []client.Object{ + &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "v1", "kind": "ConfigMap", @@ -87,22 +86,21 @@ 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{ - { - Name: "phase1", - Objects: []unstructured.Unstructured{ - { + revision: types.NewRevision( + "test-revision", 1, + []types.Phase{ + types.NewPhase( + "phase1", + []client.Object{ + &unstructured.Unstructured{ Object: map[string]interface{}{ "kind": "ConfigMap", "metadata": map[string]interface{}{ @@ -112,22 +110,21 @@ 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{ - { - Name: "phase1", - Objects: []unstructured.Unstructured{ - { + revision: types.NewRevision( + "test-revision", 1, + []types.Phase{ + types.NewPhase( + "phase1", + []client.Object{ + &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "v1", "kind": "ConfigMap", @@ -138,11 +135,11 @@ func TestRevisionValidator_Validate(t *testing.T) { }, }, }, - }, - { - Name: "phase2", - Objects: []unstructured.Unstructured{ - { + ), + types.NewPhase( + "phase2", + []client.Object{ + &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "v1", "kind": "ConfigMap", @@ -153,20 +150,16 @@ 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{}, - }, + name: "empty revision", + revision: types.NewRevision("test-revision", 1, []types.Phase{}), expectError: false, }, } @@ -205,10 +198,10 @@ func TestStaticValidateMultiplePhases(t *testing.T) { { name: "valid phases", phases: []types.Phase{ - { - Name: "phase1", - Objects: []unstructured.Unstructured{ - { + types.NewPhase( + "phase1", + []client.Object{ + &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "v1", "kind": "ConfigMap", @@ -219,11 +212,11 @@ func TestStaticValidateMultiplePhases(t *testing.T) { }, }, }, - }, - { - Name: "phase2", - Objects: []unstructured.Unstructured{ - { + ), + types.NewPhase( + "phase2", + []client.Object{ + &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "v1", "kind": "ConfigMap", @@ -234,17 +227,17 @@ func TestStaticValidateMultiplePhases(t *testing.T) { }, }, }, - }, + ), }, expectedPhaseErrorCount: 0, }, { name: "phase with invalid name", phases: []types.Phase{ - { - Name: "Invalid_Phase_Name", - Objects: []unstructured.Unstructured{ - { + types.NewPhase( + "Invalid_Phase_Name", + []client.Object{ + &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "v1", "kind": "ConfigMap", @@ -255,17 +248,17 @@ func TestStaticValidateMultiplePhases(t *testing.T) { }, }, }, - }, + ), }, expectedPhaseErrorCount: 1, }, { name: "phase with metadata validation errors", phases: []types.Phase{ - { - Name: "valid-phase", - Objects: []unstructured.Unstructured{ - { + types.NewPhase( + "valid-phase", + []client.Object{ + &unstructured.Unstructured{ Object: map[string]interface{}{ "kind": "ConfigMap", "metadata": map[string]interface{}{ @@ -275,17 +268,17 @@ func TestStaticValidateMultiplePhases(t *testing.T) { }, }, }, - }, + ), }, expectedPhaseErrorCount: 1, }, { name: "phases with duplicate objects", phases: []types.Phase{ - { - Name: "phase1", - Objects: []unstructured.Unstructured{ - { + types.NewPhase( + "phase1", + []client.Object{ + &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "v1", "kind": "ConfigMap", @@ -296,11 +289,11 @@ func TestStaticValidateMultiplePhases(t *testing.T) { }, }, }, - }, - { - Name: "phase2", - Objects: []unstructured.Unstructured{ - { + ), + types.NewPhase( + "phase2", + []client.Object{ + &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "v1", "kind": "ConfigMap", @@ -311,17 +304,17 @@ func TestStaticValidateMultiplePhases(t *testing.T) { }, }, }, - }, + ), }, expectedPhaseErrorCount: 2, }, { name: "multiple issues", phases: []types.Phase{ - { - Name: "Invalid_Phase_Name", - Objects: []unstructured.Unstructured{ - { + types.NewPhase( + "Invalid_Phase_Name", + []client.Object{ + &unstructured.Unstructured{ Object: map[string]interface{}{ "kind": "ConfigMap", "metadata": map[string]interface{}{ @@ -331,11 +324,11 @@ func TestStaticValidateMultiplePhases(t *testing.T) { }, }, }, - }, - { - Name: "phase2", - Objects: []unstructured.Unstructured{ - { + ), + types.NewPhase( + "phase2", + []client.Object{ + &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "v1", "kind": "ConfigMap", @@ -347,7 +340,7 @@ func TestStaticValidateMultiplePhases(t *testing.T) { }, }, }, - }, + ), }, expectedPhaseErrorCount: 2, }, @@ -372,7 +365,7 @@ func TestStaticValidateMultiplePhases(t *testing.T) { func TestStaticValidateMultiplePhases_DuplicateHandling(t *testing.T) { t.Parallel() - obj1 := unstructured.Unstructured{ + obj1 := &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "v1", "kind": "ConfigMap", @@ -384,14 +377,8 @@ func TestStaticValidateMultiplePhases_DuplicateHandling(t *testing.T) { } phases := []types.Phase{ - { - Name: "phase1", - Objects: []unstructured.Unstructured{obj1}, - }, - { - Name: "phase2", - Objects: []unstructured.Unstructured{obj1}, - }, + types.NewPhase("phase1", []client.Object{obj1}), + types.NewPhase("phase2", []client.Object{obj1}), } phaseErrors := staticValidateMultiplePhases(phases...) @@ -410,10 +397,10 @@ func TestStaticValidateMultiplePhases_EmptyPhases(t *testing.T) { func TestStaticValidateMultiplePhases_PhaseWithoutErrors(t *testing.T) { t.Parallel() - validPhase := types.Phase{ - Name: "valid-phase", - Objects: []unstructured.Unstructured{ - { + validPhase := types.NewPhase( + "valid-phase", + []client.Object{ + &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "v1", "kind": "ConfigMap", @@ -424,7 +411,7 @@ func TestStaticValidateMultiplePhases_PhaseWithoutErrors(t *testing.T) { }, }, }, - } + ) phaseErrors := staticValidateMultiplePhases(validPhase)