From e62fea54ea5127ce4a14561d6b5058976bc82316 Mon Sep 17 00:00:00 2001 From: Scott Andrews Date: Sat, 16 Apr 2022 11:56:19 -0400 Subject: [PATCH] Support client patch and delete collection requests for tests (#202) Reconcilers and sub-reconcilers can now make use of patch and delete collection operations and test them like any other client request. Unlike for operation that use the full object, type aware comparisons are not available. The exact patch type and bytes called on the client must match the expected values. Signed-off-by: Scott Andrews --- testing/client.go | 59 +++++++++++++++++---- testing/reconciler.go | 108 +++++++++++++++++++++++++++++++++++++++ testing/subreconciler.go | 48 +++++++++++++++++ 3 files changed, 206 insertions(+), 9 deletions(-) diff --git a/testing/client.go b/testing/client.go index 455f7d6..55869a8 100644 --- a/testing/client.go +++ b/testing/client.go @@ -22,9 +22,11 @@ type clientWrapper struct { client client.Client CreateActions []objectAction UpdateActions []objectAction + PatchActions []PatchAction DeleteActions []DeleteAction DeleteCollectionActions []DeleteCollectionAction StatusUpdateActions []objectAction + StatusPatchActions []PatchAction genCount int reactionChain []Reactor } @@ -37,13 +39,16 @@ func NewFakeClient(scheme *runtime.Scheme, objs ...client.Object) *clientWrapper o[i] = objs[i] } client := &clientWrapper{ - client: fakeclient.NewFakeClientWithScheme(scheme, o...), - CreateActions: []objectAction{}, - UpdateActions: []objectAction{}, - DeleteActions: []DeleteAction{}, - StatusUpdateActions: []objectAction{}, - genCount: 0, - reactionChain: []Reactor{}, + client: fakeclient.NewFakeClientWithScheme(scheme, o...), + CreateActions: []objectAction{}, + UpdateActions: []objectAction{}, + PatchActions: []PatchAction{}, + DeleteActions: []DeleteAction{}, + DeleteCollectionActions: []DeleteCollectionAction{}, + StatusUpdateActions: []objectAction{}, + StatusPatchActions: []PatchAction{}, + genCount: 0, + reactionChain: []Reactor{}, } // generate names on create client.AddReactor("create", "*", func(action Action) (bool, runtime.Object, error) { @@ -211,7 +216,25 @@ func (w *clientWrapper) Update(ctx context.Context, obj client.Object, opts ...c } func (w *clientWrapper) Patch(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { - panic(fmt.Errorf("Patch() is not implemented")) + gvr, _, _, err := w.objmeta(obj) + if err != nil { + return err + } + b, err := patch.Data(obj) + if err != nil { + return err + } + + // capture action + w.PatchActions = append(w.PatchActions, clientgotesting.NewPatchAction(gvr, obj.GetNamespace(), obj.GetName(), patch.Type(), b)) + + // call reactor chain + err = w.react(clientgotesting.NewPatchAction(gvr, obj.GetNamespace(), obj.GetName(), patch.Type(), b)) + if err != nil { + return err + } + + return w.client.Patch(ctx, obj, patch, opts...) } func (w *clientWrapper) DeleteAllOf(ctx context.Context, obj client.Object, opts ...client.DeleteAllOfOption) error { @@ -269,7 +292,25 @@ func (w *statusWriterWrapper) Update(ctx context.Context, obj client.Object, opt } func (w *statusWriterWrapper) Patch(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { - panic(fmt.Errorf("Patch() is not implemented")) + gvr, _, _, err := w.clientWrapper.objmeta(obj) + if err != nil { + return err + } + b, err := patch.Data(obj) + if err != nil { + return err + } + + // capture action + w.clientWrapper.StatusPatchActions = append(w.clientWrapper.StatusPatchActions, clientgotesting.NewPatchSubresourceAction(gvr, obj.GetNamespace(), obj.GetName(), patch.Type(), b, "status")) + + // call reactor chain + err = w.clientWrapper.react(clientgotesting.NewPatchSubresourceAction(gvr, obj.GetNamespace(), obj.GetName(), patch.Type(), b, "status")) + if err != nil { + return err + } + + return w.statusWriter.Patch(ctx, obj, patch, opts...) } // InduceFailure is used in conjunction with reconciler test's WithReactors field. diff --git a/testing/reconciler.go b/testing/reconciler.go index 4302835..7dc8075 100644 --- a/testing/reconciler.go +++ b/testing/reconciler.go @@ -15,6 +15,8 @@ import ( "github.com/google/go-cmp/cmp/cmpopts" "github.com/vmware-labs/reconciler-runtime/reconcilers" "k8s.io/apimachinery/pkg/api/resource" + "k8s.io/apimachinery/pkg/fields" + "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" controllerruntime "sigs.k8s.io/controller-runtime" @@ -56,10 +58,16 @@ type ReconcilerTestCase struct { ExpectCreates []client.Object // ExpectUpdates builds the ordered list of objects expected to be updated during reconciliation ExpectUpdates []client.Object + // ExpectPatches builds the ordered list of objects expected to be patched during reconciliation + ExpectPatches []PatchRef // ExpectDeletes holds the ordered list of objects expected to be deleted during reconciliation ExpectDeletes []DeleteRef + // ExpectDeleteCollections holds the ordered list of collections expected to be deleted during reconciliation + ExpectDeleteCollections []DeleteCollectionRef // ExpectStatusUpdates builds the ordered list of objects whose status is updated during reconciliation ExpectStatusUpdates []client.Object + // ExpectStatusPatches builds the ordered list of objects whose status is patched during reconciliation + ExpectStatusPatches []PatchRef // outputs @@ -169,6 +177,7 @@ func (tc *ReconcilerTestCase) Run(t *testing.T, scheme *runtime.Scheme, factory tc.Verify(t, result, err) } + // compare tracks actualTracks := tracker.getTrackRequests() for i, exp := range tc.ExpectTracks { if i >= len(actualTracks) { @@ -186,6 +195,7 @@ func (tc *ReconcilerTestCase) Run(t *testing.T, scheme *runtime.Scheme, factory } } + // compare events actualEvents := recorder.events for i, exp := range tc.ExpectEvents { if i >= len(actualEvents) { @@ -203,9 +213,31 @@ func (tc *ReconcilerTestCase) Run(t *testing.T, scheme *runtime.Scheme, factory } } + // compare create CompareActions(t, "create", tc.ExpectCreates, clientWrapper.CreateActions, IgnoreLastTransitionTime, SafeDeployDiff, IgnoreTypeMeta, IgnoreResourceVersion, cmpopts.EquateEmpty()) + + // compare update CompareActions(t, "update", tc.ExpectUpdates, clientWrapper.UpdateActions, IgnoreLastTransitionTime, SafeDeployDiff, IgnoreTypeMeta, IgnoreResourceVersion, cmpopts.EquateEmpty()) + // compare patches + for i, exp := range tc.ExpectPatches { + if i >= len(clientWrapper.PatchActions) { + t.Errorf("Missing patch: %#v", exp) + continue + } + actual := NewPatchRef(clientWrapper.PatchActions[i]) + + if diff := cmp.Diff(exp, actual); diff != "" { + t.Errorf("Unexpected patch (-expected, +actual): %s", diff) + } + } + if actual, expected := len(clientWrapper.PatchActions), len(tc.ExpectPatches); actual > expected { + for _, extra := range clientWrapper.PatchActions[expected:] { + t.Errorf("Extra patch: %#v", extra) + } + } + + // compare deletes for i, exp := range tc.ExpectDeletes { if i >= len(clientWrapper.DeleteActions) { t.Errorf("Missing delete: %#v", exp) @@ -223,8 +255,45 @@ func (tc *ReconcilerTestCase) Run(t *testing.T, scheme *runtime.Scheme, factory } } + // compare delete collections + for i, exp := range tc.ExpectDeleteCollections { + if i >= len(clientWrapper.DeleteCollectionActions) { + t.Errorf("Missing delete collection: %#v", exp) + continue + } + actual := NewDeleteCollectionRef(clientWrapper.DeleteCollectionActions[i]) + + if diff := cmp.Diff(exp, actual); diff != "" { + t.Errorf("Unexpected delete collection (-expected, +actual): %s", diff) + } + } + if actual, expected := len(clientWrapper.DeleteCollectionActions), len(tc.ExpectDeleteCollections); actual > expected { + for _, extra := range clientWrapper.DeleteCollectionActions[expected:] { + t.Errorf("Extra delete collection: %#v", extra) + } + } + + // compare status update CompareActions(t, "status update", tc.ExpectStatusUpdates, clientWrapper.StatusUpdateActions, statusSubresourceOnly, IgnoreLastTransitionTime, SafeDeployDiff, cmpopts.EquateEmpty()) + // status patch + for i, exp := range tc.ExpectStatusPatches { + if i >= len(clientWrapper.StatusPatchActions) { + t.Errorf("Missing status patch: %#v", exp) + continue + } + actual := NewPatchRef(clientWrapper.StatusPatchActions[i]) + + if diff := cmp.Diff(exp, actual); diff != "" { + t.Errorf("Unexpected status patch (-expected, +actual): %s", diff) + } + } + if actual, expected := len(clientWrapper.StatusPatchActions), len(tc.ExpectStatusPatches); actual > expected { + for _, extra := range clientWrapper.StatusPatchActions[expected:] { + t.Errorf("Extra status patch: %#v", extra) + } + } + // Validate the given objects are not mutated by reconciliation if diff := cmp.Diff(originalGivenObjects, givenObjects, SafeDeployDiff, IgnoreResourceVersion, cmpopts.EquateEmpty()); diff != "" { t.Errorf("Given objects mutated by test %q (-expected, +actual): %v", tc.Name, diff) @@ -240,6 +309,7 @@ func normalizeResult(result controllerruntime.Result) controllerruntime.Result { } func CompareActions(t *testing.T, actionName string, expectedActionFactories []client.Object, actualActions []objectAction, diffOptions ...cmp.Option) { + // TODO(scothis) this could be a really good place to play with generics t.Helper() for i, exp := range expectedActionFactories { if i >= len(actualActions) { @@ -317,6 +387,26 @@ func (ts ReconcilerTestSuite) Run(t *testing.T, scheme *runtime.Scheme, factory // and FakeStatsReporter to capture stats. type ReconcilerFactory func(t *testing.T, rtc *ReconcilerTestCase, c reconcilers.Config) reconcile.Reconciler +type PatchRef struct { + Group string + Kind string + Namespace string + Name string + PatchType types.PatchType + Patch []byte +} + +func NewPatchRef(action PatchAction) PatchRef { + return PatchRef{ + Group: action.GetResource().Group, + Kind: action.GetResource().Resource, + Namespace: action.GetNamespace(), + Name: action.GetName(), + PatchType: action.GetPatchType(), + Patch: action.GetPatch(), + } +} + type DeleteRef struct { Group string Kind string @@ -332,3 +422,21 @@ func NewDeleteRef(action DeleteAction) DeleteRef { Name: action.GetName(), } } + +type DeleteCollectionRef struct { + Group string + Kind string + Namespace string + Labels labels.Selector + Fields fields.Selector +} + +func NewDeleteCollectionRef(action DeleteCollectionAction) DeleteCollectionRef { + return DeleteCollectionRef{ + Group: action.GetResource().Group, + Kind: action.GetResource().Resource, + Namespace: action.GetNamespace(), + Labels: action.GetListRestrictions().Labels, + Fields: action.GetListRestrictions().Fields, + } +} diff --git a/testing/subreconciler.go b/testing/subreconciler.go index 072a49f..21c6002 100644 --- a/testing/subreconciler.go +++ b/testing/subreconciler.go @@ -59,8 +59,12 @@ type SubReconcilerTestCase struct { ExpectCreates []client.Object // ExpectUpdates builds the ordered list of objects expected to be updated during reconciliation ExpectUpdates []client.Object + // ExpectPatches builds the ordered list of objects expected to be patched during reconciliation + ExpectPatches []PatchRef // ExpectDeletes holds the ordered list of objects expected to be deleted during reconciliation ExpectDeletes []DeleteRef + // ExpectDeleteCollections holds the ordered list of collections expected to be deleted during reconciliation + ExpectDeleteCollections []DeleteCollectionRef // outputs @@ -188,6 +192,7 @@ func (tc *SubReconcilerTestCase) Run(t *testing.T, scheme *runtime.Scheme, facto tc.Verify(t, result, err) } + // compare parent expectedParent := tc.Parent.DeepCopyObject().(client.Object) if tc.ExpectParent != nil { expectedParent = tc.ExpectParent.DeepCopyObject().(client.Object) @@ -196,6 +201,7 @@ func (tc *SubReconcilerTestCase) Run(t *testing.T, scheme *runtime.Scheme, facto t.Errorf("Unexpected parent mutations(-expected, +actual): %s", diff) } + // compare stashed for key, expected := range tc.ExpectStashedValues { if f, ok := expected.(runtime.Object); ok { expected = f.DeepCopyObject() @@ -206,6 +212,7 @@ func (tc *SubReconcilerTestCase) Run(t *testing.T, scheme *runtime.Scheme, facto } } + // compare tracks actualTracks := tracker.getTrackRequests() for i, exp := range tc.ExpectTracks { if i >= len(actualTracks) { @@ -223,6 +230,7 @@ func (tc *SubReconcilerTestCase) Run(t *testing.T, scheme *runtime.Scheme, facto } } + // compare events actualEvents := recorder.events for i, exp := range tc.ExpectEvents { if i >= len(actualEvents) { @@ -240,9 +248,31 @@ func (tc *SubReconcilerTestCase) Run(t *testing.T, scheme *runtime.Scheme, facto } } + // compare create CompareActions(t, "create", tc.ExpectCreates, clientWrapper.CreateActions, IgnoreLastTransitionTime, SafeDeployDiff, IgnoreTypeMeta, IgnoreResourceVersion, cmpopts.EquateEmpty()) + + // compare update CompareActions(t, "update", tc.ExpectUpdates, clientWrapper.UpdateActions, IgnoreLastTransitionTime, SafeDeployDiff, IgnoreTypeMeta, IgnoreResourceVersion, cmpopts.EquateEmpty()) + // compare patches + for i, exp := range tc.ExpectPatches { + if i >= len(clientWrapper.PatchActions) { + t.Errorf("Missing patch: %#v", exp) + continue + } + actual := NewPatchRef(clientWrapper.PatchActions[i]) + + if diff := cmp.Diff(exp, actual); diff != "" { + t.Errorf("Unexpected patch (-expected, +actual): %s", diff) + } + } + if actual, expected := len(clientWrapper.PatchActions), len(tc.ExpectPatches); actual > expected { + for _, extra := range clientWrapper.PatchActions[expected:] { + t.Errorf("Extra patch: %#v", extra) + } + } + + // compare deletes for i, exp := range tc.ExpectDeletes { if i >= len(clientWrapper.DeleteActions) { t.Errorf("Missing delete: %#v", exp) @@ -260,6 +290,24 @@ func (tc *SubReconcilerTestCase) Run(t *testing.T, scheme *runtime.Scheme, facto } } + // compare delete collections + for i, exp := range tc.ExpectDeleteCollections { + if i >= len(clientWrapper.DeleteCollectionActions) { + t.Errorf("Missing delete collection: %#v", exp) + continue + } + actual := NewDeleteCollectionRef(clientWrapper.DeleteCollectionActions[i]) + + if diff := cmp.Diff(exp, actual); diff != "" { + t.Errorf("Unexpected delete collection (-expected, +actual): %s", diff) + } + } + if actual, expected := len(clientWrapper.DeleteCollectionActions), len(tc.ExpectDeleteCollections); actual > expected { + for _, extra := range clientWrapper.DeleteCollectionActions[expected:] { + t.Errorf("Extra delete collection: %#v", extra) + } + } + // Validate the given objects are not mutated by reconciliation if diff := cmp.Diff(originalGivenObjects, givenObjects, SafeDeployDiff, IgnoreResourceVersion, cmpopts.EquateEmpty()); diff != "" { t.Errorf("Given objects mutated by test %q (-expected, +actual): %v", tc.Name, diff)