diff --git a/go.mod b/go.mod index 451110066..b76711d03 100644 --- a/go.mod +++ b/go.mod @@ -5,7 +5,6 @@ go 1.20 require ( dario.cat/mergo v1.0.0 github.com/bufbuild/buf v1.26.1 - github.com/evanphx/json-patch/v5 v5.6.0 github.com/go-logr/logr v1.2.4 github.com/google/go-cmp v0.5.9 github.com/spf13/afero v1.9.5 @@ -40,6 +39,7 @@ require ( github.com/docker/go-connections v0.4.0 // indirect github.com/docker/go-units v0.5.0 // indirect github.com/emicklei/go-restful/v3 v3.10.2 // indirect + github.com/evanphx/json-patch/v5 v5.6.0 // indirect github.com/fatih/color v1.15.0 // indirect github.com/felixge/fgprof v0.9.3 // indirect github.com/fsnotify/fsnotify v1.6.0 // indirect diff --git a/pkg/logging/logging.go b/pkg/logging/logging.go index 898024d84..ded792a15 100644 --- a/pkg/logging/logging.go +++ b/pkg/logging/logging.go @@ -37,6 +37,8 @@ limitations under the License. package logging import ( + "fmt" + "github.com/go-logr/logr" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -98,6 +100,9 @@ func (l logrLogger) WithValues(keysAndValues ...any) Logger { func ForResource(object client.Object) []string { ret := make([]string, 0, 10) gvk := object.GetObjectKind().GroupVersionKind() + if gvk.Kind == "" { + gvk.Kind = fmt.Sprintf("%T", object) // best effort for native Go types + } ret = append(ret, "name", object.GetName(), "kind", gvk.Kind, diff --git a/pkg/resource/api.go b/pkg/resource/api.go index 5d903d493..3b20a5956 100644 --- a/pkg/resource/api.go +++ b/pkg/resource/api.go @@ -32,24 +32,30 @@ import ( // Error strings. const ( errUpdateObject = "cannot update object" + + // taken from k8s.io/apiserver. Not crucial to match, but for uniformity it + // better should. + // TODO(sttts): import from k8s.io/apiserver/pkg/registry/generic/registry when + // kube has updated otel dependencies post-1.28. + errOptimisticLock = "the object has been modified; please apply your changes to the latest version and try again" ) // An APIPatchingApplicator applies changes to an object by either creating or // patching it in a Kubernetes API server. type APIPatchingApplicator struct { - client client.Client - optionalLog logging.Logger // can be nil + client client.Client + log logging.Logger } // NewAPIPatchingApplicator returns an Applicator that applies changes to an // object by either creating or patching it in a Kubernetes API server. func NewAPIPatchingApplicator(c client.Client) *APIPatchingApplicator { - return &APIPatchingApplicator{client: c} + return &APIPatchingApplicator{client: c, log: logging.NewNopLogger()} } // WithLogger sets the logger on the APIPatchingApplicator. func (a *APIPatchingApplicator) WithLogger(l logging.Logger) *APIPatchingApplicator { - a.optionalLog = l + a.log = l return a } @@ -58,21 +64,23 @@ func (a *APIPatchingApplicator) WithLogger(l logging.Logger) *APIPatchingApplica // patched if the passed object has the same or an empty resource version. func (a *APIPatchingApplicator) Apply(ctx context.Context, obj client.Object, ao ...ApplyOption) error { //nolint:gocyclo // the logic here is crucial and deserves to stay in one method if obj.GetName() == "" && obj.GetGenerateName() != "" { - return errors.Wrap(a.client.Create(ctx, obj), "cannot create object") + log := a.log.WithValues(logging.ForResource(obj)) + log.Info("creating object") + return a.client.Create(ctx, obj) } current := obj.DeepCopyObject().(client.Object) err := a.client.Get(ctx, types.NamespacedName{Name: obj.GetName(), Namespace: obj.GetNamespace()}, current) if kerrors.IsNotFound(err) { // TODO(negz): Apply ApplyOptions here too? - return errors.Wrap(a.client.Create(ctx, obj), "cannot create object") + return a.client.Create(ctx, obj) } if err != nil { - return errors.Wrap(err, "cannot get object") + return err } // Note: this check would ideally not be necessary if the Apply signature - // had a current object that we could us for the diff. But we have no + // had a current object that we could use for the diff. But we have no // current and for consistency of the patch it matters that the object we // get above is the one that was originally used. if obj.GetResourceVersion() != "" && obj.GetResourceVersion() != current.GetResourceVersion() { @@ -80,21 +88,25 @@ func (a *APIPatchingApplicator) Apply(ctx context.Context, obj client.Object, ao if err != nil { return err } - return kerrors.NewConflict(gvr, current.GetName(), errors.New("resource version does not match")) + return kerrors.NewConflict(gvr, current.GetName(), errors.New(errOptimisticLock)) } for _, fn := range ao { if err := fn(ctx, current, obj); err != nil { - return err + return errors.Wrapf(err, "apply option failed for %s", HumanReadableReference(a.client, obj)) } } - if err := LogDiff(a.optionalLog, current, obj); err != nil { - return err + // log diff + patch := client.MergeFromWithOptions(current, client.MergeFromWithOptimisticLock{}) + patchBytes, err := patch.Data(obj) + if err != nil { + return errors.Wrapf(err, "failed to diff %s", HumanReadableReference(a.client, obj)) } + log := a.log.WithValues(logging.ForResource(obj)) + log.WithValues("diff", string(patchBytes)).Info("patching object") - // TODO(negz): Allow callers to override the kind of patch used. - return errors.Wrap(a.client.Patch(ctx, obj, client.MergeFromWithOptions(current, client.MergeFromWithOptimisticLock{})), "cannot patch object") + return a.client.Patch(ctx, obj, client.RawPatch(patch.Type(), patchBytes)) } func groupResource(c client.Client, o client.Object) (schema.GroupResource, error) { @@ -112,8 +124,8 @@ func groupResource(c client.Client, o client.Object) (schema.GroupResource, erro // An APIUpdatingApplicator applies changes to an object by either creating or // updating it in a Kubernetes API server. type APIUpdatingApplicator struct { - client client.Client - optionalLog logging.Logger // can be nil + client client.Client + log logging.Logger } // NewAPIUpdatingApplicator returns an Applicator that applies changes to an @@ -122,12 +134,12 @@ type APIUpdatingApplicator struct { // Deprecated: Use NewAPIPatchingApplicator instead. The updating applicator // can lead to data-loss if the Golang types in this process are not up-to-date. func NewAPIUpdatingApplicator(c client.Client) *APIUpdatingApplicator { - return &APIUpdatingApplicator{client: c} + return &APIUpdatingApplicator{client: c, log: logging.NewNopLogger()} } // WithLogger sets the logger on the APIUpdatingApplicator. func (a *APIUpdatingApplicator) WithLogger(l logging.Logger) *APIUpdatingApplicator { - a.optionalLog = l + a.log = l return a } @@ -135,30 +147,37 @@ func (a *APIUpdatingApplicator) WithLogger(l logging.Logger) *APIUpdatingApplica // not exist, or updated if it does. func (a *APIUpdatingApplicator) Apply(ctx context.Context, obj client.Object, ao ...ApplyOption) error { if obj.GetName() == "" && obj.GetGenerateName() != "" { - return errors.Wrap(a.client.Create(ctx, obj), "cannot create object") + log := a.log.WithValues(logging.ForResource(obj)) + log.Info("creating object") + return a.client.Create(ctx, obj) } current := obj.DeepCopyObject().(client.Object) err := a.client.Get(ctx, types.NamespacedName{Name: obj.GetName(), Namespace: obj.GetNamespace()}, current) if kerrors.IsNotFound(err) { // TODO(negz): Apply ApplyOptions here too? - return errors.Wrap(a.client.Create(ctx, obj), "cannot create object") + return a.client.Create(ctx, obj) } if err != nil { - return errors.Wrap(err, "cannot get object") + return err } for _, fn := range ao { if err := fn(ctx, current, obj); err != nil { - return err + return errors.Wrapf(err, "apply option failed for %s", HumanReadableReference(a.client, obj)) } } - if err := LogDiff(a.optionalLog, current, obj); err != nil { - return err + // log diff + patch := client.MergeFromWithOptions(current, client.MergeFromWithOptimisticLock{}) + patchBytes, err := patch.Data(obj) + if err != nil { + return errors.Wrapf(err, "failed to diff %s", HumanReadableReference(a.client, obj)) } + log := a.log.WithValues(logging.ForResource(obj)) + log.WithValues("diff", string(patchBytes)).Info("updating object") - return errors.Wrap(a.client.Update(ctx, obj), "cannot update object") + return a.client.Update(ctx, obj) } // An APIFinalizer adds and removes finalizers to and from a resource. diff --git a/pkg/resource/api_test.go b/pkg/resource/api_test.go index 731cee31e..0cf2dde34 100644 --- a/pkg/resource/api_test.go +++ b/pkg/resource/api_test.go @@ -22,6 +22,7 @@ import ( "github.com/google/go-cmp/cmp" kerrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" @@ -34,8 +35,24 @@ import ( func TestAPIPatchingApplicator(t *testing.T) { errBoom := errors.New("boom") - desired := &object{} - desired.SetName("desired") + + current := &object{Spec: "old"} + current.SetName("foo") + + desired := &object{Spec: "new"} + desired.SetName("foo") + + withRV := func(rv string, o client.Object) client.Object { + cpy := *(o.(*object)) + cpy.SetResourceVersion(rv) + return &cpy + } + + gvk := schema.GroupVersionKind{Group: "example.com", Version: "v1", Kind: "Thing"} + gvr := schema.GroupVersionResource{Group: "example.com", Version: "v1", Resource: "things"} + singular := schema.GroupVersionResource{Group: "example.com", Version: "v1", Resource: "thing"} + fakeRESTMapper := meta.NewDefaultRESTMapper([]schema.GroupVersion{gvk.GroupVersion()}) + fakeRESTMapper.AddSpecific(gvk, gvr, singular, meta.RESTScopeRoot) type args struct { ctx context.Context @@ -56,53 +73,71 @@ func TestAPIPatchingApplicator(t *testing.T) { }{ "GetError": { reason: "An error should be returned if we can't get the object", - c: &test.MockClient{MockGet: test.NewMockGetFn(errBoom)}, + c: &test.MockClient{ + MockGet: test.NewMockGetFn(errBoom), + MockGroupVersionKindFor: test.NewMockGroupVersionKindForFn(nil, gvk), + }, args: args{ - o: &object{}, + o: withRV("42", desired), }, want: want{ - o: &object{}, - err: errors.Wrap(errBoom, "cannot get object"), + o: withRV("42", desired), + // this is intentionally not a wrapped error because this comes from a client + err: errBoom, }, }, "CreateError": { reason: "No error should be returned if we successfully create a new object", c: &test.MockClient{ - MockGet: test.NewMockGetFn(kerrors.NewNotFound(schema.GroupResource{}, "")), - MockCreate: test.NewMockCreateFn(errBoom), + MockGet: test.NewMockGetFn(kerrors.NewNotFound(schema.GroupResource{}, "")), + MockCreate: test.NewMockCreateFn(errBoom), + MockGroupVersionKindFor: test.NewMockGroupVersionKindForFn(nil, gvk), }, args: args{ - o: &object{}, + o: desired, }, want: want{ - o: &object{}, - err: errors.Wrap(errBoom, "cannot create object"), + o: desired, + // this is intentionally not a wrapped error because this comes from a client + err: errBoom, }, }, "ApplyOptionError": { reason: "Any errors from an apply option should be returned", - c: &test.MockClient{MockGet: test.NewMockGetFn(nil)}, + c: &test.MockClient{ + MockGet: test.NewMockGetFn(nil, func(o client.Object) error { + *o.(*object) = *current + o.SetResourceVersion("42") + return nil + }), + MockGroupVersionKindFor: test.NewMockGroupVersionKindForFn(nil, gvk), + }, args: args{ - o: &object{}, + o: withRV("42", desired), ao: []ApplyOption{func(_ context.Context, _, _ runtime.Object) error { return errBoom }}, }, want: want{ - o: &object{}, - err: errBoom, + o: withRV("42", desired), + err: errors.Wrapf(errBoom, "apply option failed for thing \"foo\""), }, }, "PatchError": { reason: "An error should be returned if we can't patch the object", c: &test.MockClient{ - MockGet: test.NewMockGetFn(nil), - MockPatch: test.NewMockPatchFn(errBoom), + MockGet: test.NewMockGetFn(nil, func(o client.Object) error { + *o.(*object) = *current + o.SetResourceVersion("42") + return nil + }), + MockPatch: test.NewMockPatchFn(errBoom), + MockGroupVersionKindFor: test.NewMockGroupVersionKindForFn(nil, gvk), }, args: args{ - o: &object{}, + o: withRV("42", desired), }, want: want{ - o: &object{}, - err: errors.Wrap(errBoom, "cannot patch object"), + o: withRV("42", desired), + err: errBoom, // this is intentionally not a wrapped error because this comes from a client }, }, "Created": { @@ -111,8 +146,10 @@ func TestAPIPatchingApplicator(t *testing.T) { MockGet: test.NewMockGetFn(kerrors.NewNotFound(schema.GroupResource{}, "")), MockCreate: test.NewMockCreateFn(nil, func(o client.Object) error { *o.(*object) = *desired + o.SetResourceVersion("1") return nil }), + MockGroupVersionKindFor: test.NewMockGroupVersionKindForFn(nil, gvk), }, args: args{ o: desired, @@ -124,17 +161,64 @@ func TestAPIPatchingApplicator(t *testing.T) { "Patched": { reason: "No error should be returned if we successfully patch an existing object", c: &test.MockClient{ - MockGet: test.NewMockGetFn(nil), + MockGet: test.NewMockGetFn(nil, func(o client.Object) error { + *o.(*object) = *current + o.SetResourceVersion("42") + return nil + }), MockPatch: test.NewMockPatchFn(nil, func(o client.Object) error { *o.(*object) = *desired + o.SetResourceVersion("43") return nil }), + MockGroupVersionKindFor: test.NewMockGroupVersionKindForFn(nil, gvk), }, args: args{ - o: desired, + o: withRV("42", desired), }, want: want{ - o: desired, + o: withRV("43", desired), + }, + }, + "GetConflictError": { + reason: "No error should be returned if we successfully patch an existing object", + c: &test.MockClient{ + MockGet: test.NewMockGetFn(nil, func(o client.Object) error { + *o.(*object) = *current + o.SetResourceVersion("100") + return nil + }), + MockGroupVersionKindFor: test.NewMockGroupVersionKindForFn(nil, gvk), + MockRESTMapper: test.NewMockRESTMapperFn(fakeRESTMapper), + }, + args: args{ + o: withRV("42", desired), + }, + want: want{ + o: withRV("42", desired), + // this is intentionally not a wrapped error because this comes from a client + err: kerrors.NewConflict(schema.GroupResource{Group: "example.com", Resource: "things"}, current.GetName(), errors.New(errOptimisticLock)), + }, + }, + "PatchConflictError": { + reason: "No error should be returned if we successfully patch an existing object", + c: &test.MockClient{ + MockGet: test.NewMockGetFn(nil, func(o client.Object) error { + *o.(*object) = *current + o.SetResourceVersion("42") + return nil + }), + MockPatch: test.NewMockPatchFn(kerrors.NewConflict(schema.GroupResource{Group: "example.com", Resource: "things"}, "foo", errors.New(errOptimisticLock))), + MockGroupVersionKindFor: test.NewMockGroupVersionKindForFn(nil, gvk), + MockRESTMapper: test.NewMockRESTMapperFn(fakeRESTMapper), + }, + args: args{ + o: withRV("42", desired), + }, + want: want{ + o: withRV("42", desired), + // this is intentionally not a wrapped error because this comes from a client + err: kerrors.NewConflict(schema.GroupResource{Group: "example.com", Resource: "things"}, current.GetName(), errors.New(errOptimisticLock)), }, }, } @@ -155,10 +239,24 @@ func TestAPIPatchingApplicator(t *testing.T) { func TestAPIUpdatingApplicator(t *testing.T) { errBoom := errors.New("boom") - desired := &object{} - desired.SetName("desired") - current := &object{} - current.SetName("current") + + current := &object{Spec: "old"} + current.SetName("foo") + + desired := &object{Spec: "new"} + desired.SetName("foo") + + withRV := func(rv string, o client.Object) client.Object { + cpy := *(o.(*object)) + cpy.SetResourceVersion(rv) + return &cpy + } + + gvk := schema.GroupVersionKind{Group: "example.com", Version: "v1", Kind: "Thing"} + gvr := schema.GroupVersionResource{Group: "example.com", Version: "v1", Resource: "things"} + singular := schema.GroupVersionResource{Group: "example.com", Version: "v1", Resource: "thing"} + fakeRESTMapper := meta.NewDefaultRESTMapper([]schema.GroupVersion{gvk.GroupVersion()}) + fakeRESTMapper.AddSpecific(gvk, gvr, singular, meta.RESTScopeRoot) type args struct { ctx context.Context @@ -179,90 +277,114 @@ func TestAPIUpdatingApplicator(t *testing.T) { }{ "GetError": { reason: "An error should be returned if we can't get the object", - c: &test.MockClient{MockGet: test.NewMockGetFn(errBoom)}, + c: &test.MockClient{ + MockGet: test.NewMockGetFn(errBoom), + MockGroupVersionKindFor: test.NewMockGroupVersionKindForFn(nil, gvk), + }, args: args{ - o: &object{}, + o: withRV("42", desired), }, want: want{ - o: &object{}, - err: errors.Wrap(errBoom, "cannot get object"), + o: withRV("42", desired), + // this is intentionally not a wrapped error because this comes from a client + err: errBoom, }, }, "CreateError": { reason: "No error should be returned if we successfully create a new object", c: &test.MockClient{ - MockGet: test.NewMockGetFn(kerrors.NewNotFound(schema.GroupResource{}, "")), - MockCreate: test.NewMockCreateFn(errBoom), + MockGet: test.NewMockGetFn(kerrors.NewNotFound(gvr.GroupResource(), desired.GetName())), + MockCreate: test.NewMockCreateFn(errBoom), + MockGroupVersionKindFor: test.NewMockGroupVersionKindForFn(nil, gvk), }, args: args{ - o: &object{}, + o: desired, }, want: want{ - o: &object{}, - err: errors.Wrap(errBoom, "cannot create object"), + o: desired, + // this is intentionally not a wrapped error because this comes from a client + err: errBoom, }, }, "ApplyOptionError": { reason: "Any errors from an apply option should be returned", - c: &test.MockClient{MockGet: test.NewMockGetFn(nil)}, + c: &test.MockClient{ + MockGet: test.NewMockGetFn(nil, func(o client.Object) error { + *o.(*object) = *desired + o.SetResourceVersion("42") + return nil + }), + MockGroupVersionKindFor: test.NewMockGroupVersionKindForFn(nil, gvk), + }, args: args{ - o: &object{}, + o: withRV("42", desired), ao: []ApplyOption{func(_ context.Context, _, _ runtime.Object) error { return errBoom }}, }, want: want{ - o: &object{}, - err: errBoom, + o: withRV("42", desired), + err: errors.Wrapf(errBoom, "apply option failed for thing \"foo\""), }, }, "UpdateError": { reason: "An error should be returned if we can't update the object", c: &test.MockClient{ - MockGet: test.NewMockGetFn(nil), - MockUpdate: test.NewMockUpdateFn(errBoom), + MockGet: test.NewMockGetFn(nil, func(o client.Object) error { + *o.(*object) = *desired + o.SetResourceVersion("42") + return nil + }), + MockUpdate: test.NewMockUpdateFn(errBoom), + MockGroupVersionKindFor: test.NewMockGroupVersionKindForFn(nil, gvk), }, args: args{ - o: &object{}, + o: withRV("42", desired), }, want: want{ - o: &object{}, - err: errors.Wrap(errBoom, "cannot update object"), + o: withRV("42", desired), + // this is intentionally not a wrapped error because this comes from a client + err: errBoom, }, }, "Created": { reason: "No error should be returned if we successfully create a new object", c: &test.MockClient{ - MockGet: test.NewMockGetFn(kerrors.NewNotFound(schema.GroupResource{}, "")), + MockGet: test.NewMockGetFn(kerrors.NewNotFound(gvr.GroupResource(), desired.GetName())), MockCreate: test.NewMockCreateFn(nil, func(o client.Object) error { *o.(*object) = *desired + o.SetResourceVersion("1") return nil }), + MockGroupVersionKindFor: test.NewMockGroupVersionKindForFn(nil, gvk), }, args: args{ o: desired, }, want: want{ - o: desired, + o: withRV("1", desired), }, }, "Updated": { reason: "No error should be returned if we successfully update an existing object. If no ApplyOption is passed the existing should not be modified", c: &test.MockClient{ MockGet: test.NewMockGetFn(nil, func(o client.Object) error { - *o.(*object) = *current + *o.(*object) = *desired + o.SetResourceVersion("42") return nil }), MockUpdate: test.NewMockUpdateFn(nil, func(o client.Object) error { - if diff := cmp.Diff(*desired, *o.(*object)); diff != "" { + if diff := cmp.Diff(withRV("42", desired), o); diff != "" { t.Errorf("r: -want, +got:\n%s", diff) } + o.SetResourceVersion("43") return nil }), + MockGroupVersionKindFor: test.NewMockGroupVersionKindForFn(nil, gvk), }, args: args{ - o: desired, + o: withRV("42", desired), }, want: want{ - o: desired, + o: withRV("43", desired), }, }, } diff --git a/pkg/resource/resource.go b/pkg/resource/resource.go index da8c8bd7d..5c0aeec36 100644 --- a/pkg/resource/resource.go +++ b/pkg/resource/resource.go @@ -18,10 +18,9 @@ package resource import ( "context" - "encoding/json" + "fmt" "strings" - jsonpatch "github.com/evanphx/json-patch/v5" corev1 "k8s.io/api/core/v1" kerrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -34,7 +33,6 @@ import ( xpv1 "github.com/crossplane/crossplane-runtime/apis/common/v1" "github.com/crossplane/crossplane-runtime/pkg/errors" - "github.com/crossplane/crossplane-runtime/pkg/logging" "github.com/crossplane/crossplane-runtime/pkg/meta" ) @@ -208,6 +206,9 @@ func IsConditionTrue(c xpv1.Condition) bool { // An Applicator applies changes to an object. The passed object is expected to // be complete, i.e. not partial. type Applicator interface { + // Apply updates the given object to exactly the given state. It conflicts + // if the resource version stored on the apiserver does not match anymore + // the one in the given object. Apply(context.Context, client.Object, ...ApplyOption) error } @@ -273,30 +274,6 @@ func UpdateFn(fn func(current, desired runtime.Object)) ApplyOption { } } -// LogDiff logs the diff between current and desired object. -func LogDiff(log logging.Logger, current, desired runtime.Object) error { - if log == nil { - return nil - } - - oldData, err := json.Marshal(current) - if err != nil { - return errors.Wrapf(err, "failed to Marshal old data for %T", current) - } - newData, err := json.Marshal(desired) - if err != nil { - return errors.Wrapf(err, "failed to Marshal new data for %T", desired) - } - patchBytes, err := jsonpatch.CreateMergePatch(oldData, newData) - if err != nil { - return errors.Wrapf(err, "failed to create patch for %T", current) - } - - log.WithValues("diff", string(patchBytes)).Info("patching object") - - return nil -} - type errNotControllable struct{ error } func (e errNotControllable) NotControllable() bool { @@ -409,3 +386,24 @@ func GetExternalTags(mg Managed) map[string]string { } return tags } + +// HumanReadableReference returns a human readable object reference like +// "pod default/database", e.g. to be used in error strings. +func HumanReadableReference(c client.Client, o client.Object) string { + gvk, err := c.GroupVersionKindFor(o) + if err != nil { + gvk.Kind = fmt.Sprintf("%T", o) // best effort + } + + name := o.GetName() + infix := "" + if gn := o.GetGenerateName(); name == "" && gn != "" { + name = gn + infix = "with generated name " + } + if ns := o.GetNamespace(); ns != "" { + return fmt.Sprintf("%s %s%s/%s", strings.ToLower(gvk.Kind), infix, ns, name) + } + + return fmt.Sprintf("%s %s%q", strings.ToLower(gvk.Kind), infix, name) +} diff --git a/pkg/resource/resource_test.go b/pkg/resource/resource_test.go index 8a506dea8..e19a5a715 100644 --- a/pkg/resource/resource_test.go +++ b/pkg/resource/resource_test.go @@ -25,6 +25,7 @@ import ( corev1 "k8s.io/api/core/v1" kerrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" @@ -395,8 +396,9 @@ func TestIsConditionTrue(t *testing.T) { } type object struct { - runtime.Object - metav1.ObjectMeta + metav1.TypeMeta `json:",inline"` + metav1.ObjectMeta `json:"metadata"` + Spec string `json:"spec"` } func (o *object) DeepCopyObject() runtime.Object { @@ -876,3 +878,123 @@ func TestUpdate(t *testing.T) { }) } } + +func TestHumanReadableReference(t *testing.T) { + type args struct { + c client.Client + o client.Object + } + tests := []struct { + name string + args args + want string + }{ + { + name: "simple", + args: args{ + c: &test.MockClient{ + MockGroupVersionKindFor: func(r runtime.Object) (schema.GroupVersionKind, error) { + return schema.GroupVersionKind{Group: "", Version: "v1", Kind: "Pod"}, nil + }, + }, + o: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "default", + }, + }, + }, + want: "pod default/foo", + }, + { + name: "unstructured", + args: args{ + c: &test.MockClient{ + MockGroupVersionKindFor: func(r runtime.Object) (schema.GroupVersionKind, error) { + return schema.GroupVersionKind{Group: "", Version: "v1", Kind: "Pod"}, nil + }, + }, + o: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "Pod", + "metadata": map[string]interface{}{ + "name": "foo", + "namespace": "default", + }, + }, + }, + }, + want: "pod default/foo", + }, + { + name: "unknown", + args: args{ + c: &test.MockClient{ + MockGroupVersionKindFor: func(r runtime.Object) (schema.GroupVersionKind, error) { + return schema.GroupVersionKind{Group: "", Version: "v1", Kind: "Pod"}, nil + }, + }, + o: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "Pod", + "metadata": map[string]interface{}{ + "name": "foo", + "namespace": "default", + }, + }, + }, + }, + want: "pod default/foo", + }, + { + name: "no namespace", + args: args{ + c: &test.MockClient{ + MockGroupVersionKindFor: func(r runtime.Object) (schema.GroupVersionKind, error) { + return schema.GroupVersionKind{Group: "", Version: "v1", Kind: "Node"}, nil + }, + }, + o: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "Node", + "metadata": map[string]interface{}{ + "name": "foo", + }, + }, + }, + }, + want: "node \"foo\"", + }, + { + name: "generate name", + args: args{ + c: &test.MockClient{ + MockGroupVersionKindFor: func(r runtime.Object) (schema.GroupVersionKind, error) { + return schema.GroupVersionKind{Group: "", Version: "v1", Kind: "Pod"}, nil + }, + }, + o: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "Pod", + "metadata": map[string]interface{}{ + "generateName": "foo-", + "namespace": "default", + }, + }, + }, + }, + want: "pod with generated name default/foo-", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := HumanReadableReference(tt.args.c, tt.args.o); got != tt.want { + t.Errorf("HumanReadableReference() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/pkg/test/fake.go b/pkg/test/fake.go index c9f4b1c1b..908783ec4 100644 --- a/pkg/test/fake.go +++ b/pkg/test/fake.go @@ -63,6 +63,9 @@ type MockSubResourcePatchFn func(ctx context.Context, obj client.Object, patch c // A MockSchemeFn is used to mock client.Client's Scheme implementation. type MockSchemeFn func() *runtime.Scheme +// A MockRESTMapperFn is used to mock client.RESTMapper. +type MockRESTMapperFn func() meta.RESTMapper + // A MockGroupVersionKindForFn is used to mock client.Client's GroupVersionKindFor implementation. type MockGroupVersionKindForFn func(runtime.Object) (schema.GroupVersionKind, error) @@ -201,13 +204,20 @@ func NewMockSubResourcePatchFn(err error, ofn ...ObjectFn) MockSubResourcePatchF } } -// NewMockSchemeFn returns a MockSchemeFn that returns the scheme +// NewMockSchemeFn returns a MockSchemeFn that returns the scheme. func NewMockSchemeFn(scheme *runtime.Scheme) MockSchemeFn { return func() *runtime.Scheme { return scheme } } +// NewMockRESTMapperFn returns a MockRESTMapperFn that returns the RESTMapper. +func NewMockRESTMapperFn(mapper meta.RESTMapper) MockRESTMapperFn { + return func() meta.RESTMapper { + return mapper + } +} + // NewMockGroupVersionKindForFn returns a MockGroupVersionKindForFn that returns the supplied GVK and error. func NewMockGroupVersionKindForFn(err error, gvk schema.GroupVersionKind, rofn ...RuntimeObjectFn) MockGroupVersionKindForFn { return func(obj runtime.Object) (schema.GroupVersionKind, error) { @@ -257,6 +267,7 @@ type MockClient struct { MockScheme MockSchemeFn MockGroupVersionKindFor MockGroupVersionKindForFn MockIsObjectNamespaced MockIsObjectNamespacedFn + MockRESTMapper MockRESTMapperFn } // NewMockClient returns a MockClient that does nothing when its methods are @@ -277,6 +288,7 @@ func NewMockClient() *MockClient { MockScheme: NewMockSchemeFn(nil), MockGroupVersionKindFor: NewMockGroupVersionKindForFn(nil, schema.GroupVersionKind{}), MockIsObjectNamespaced: NewMockIsObjectNamespacedFn(nil, false), + MockRESTMapper: NewMockRESTMapperFn(nil), } } @@ -336,7 +348,7 @@ func (c *MockClient) SubResource(_ string) client.SubResourceClient { // RESTMapper returns the REST mapper. func (c *MockClient) RESTMapper() meta.RESTMapper { - return nil + return c.MockRESTMapper() } // Scheme calls MockClient's MockScheme function