diff --git a/go.mod b/go.mod index b76711d03..f0d99deb8 100644 --- a/go.mod +++ b/go.mod @@ -5,6 +5,7 @@ go 1.20 require ( dario.cat/mergo v1.0.0 github.com/bufbuild/buf v1.26.1 + github.com/evanphx/json-patch v5.6.0+incompatible github.com/go-logr/logr v1.2.4 github.com/google/go-cmp v0.5.9 github.com/spf13/afero v1.9.5 diff --git a/go.sum b/go.sum index 3b6ec0f8f..1f7918422 100644 --- a/go.sum +++ b/go.sum @@ -96,6 +96,7 @@ github.com/envoyproxy/go-control-plane v0.9.7/go.mod h1:cwu0lG7PUMfa9snN8LXBig5y github.com/envoyproxy/go-control-plane v0.9.9-0.20201210154907-fd9021fe5dad/go.mod h1:cXg6YxExXjJnVBQHBLXeUAgxn2UodCpnH306RInaBQk= github.com/envoyproxy/protoc-gen-validate v0.1.0/go.mod h1:iSmxcyjqTsJpI2R4NaDN7+kN2VEUnK/pcBlmesArF7c= github.com/evanphx/json-patch v5.6.0+incompatible h1:jBYDEEiFBPxA0v50tFdvOzQQTCvpL6mnFh5mB2/l16U= +github.com/evanphx/json-patch v5.6.0+incompatible/go.mod h1:50XU6AFN0ol/bzJsmQLiYLvXMP4fmwYFNcr97nuDLSk= github.com/evanphx/json-patch/v5 v5.6.0 h1:b91NhWfaz02IuVxO9faSllyAtNXHMPkC5J8sJCLunww= github.com/evanphx/json-patch/v5 v5.6.0/go.mod h1:G79N1coSVB93tBe7j6PhzjmR3/2VvlbKOFpnXhI9Bw4= github.com/fatih/color v1.15.0 h1:kOqh6YHBtK8aywxGerMG2Eq3H6Qgoqeo13Bk2Mv/nBs= diff --git a/pkg/connection/store/kubernetes/store.go b/pkg/connection/store/kubernetes/store.go index 72623efb7..63033ebf6 100644 --- a/pkg/connection/store/kubernetes/store.go +++ b/pkg/connection/store/kubernetes/store.go @@ -66,7 +66,7 @@ func NewSecretStore(ctx context.Context, local client.Client, _ *tls.Config, cfg return &SecretStore{ client: resource.ClientApplicator{ Client: kube, - Applicator: resource.NewApplicatorWithRetry(resource.NewAPIPatchingApplicator(kube), resource.IsAPIErrorWrapped, nil), + Applicator: resource.NewApplicatorWithRetry(resource.NewAPIPatchingApplicator(kube), resource.IsNonConflictAPIErrorWrapped, nil), }, defaultNamespace: cfg.DefaultScope, }, nil diff --git a/pkg/reconciler/managed/api.go b/pkg/reconciler/managed/api.go index a374a9a11..77e5d9d04 100644 --- a/pkg/reconciler/managed/api.go +++ b/pkg/reconciler/managed/api.go @@ -74,7 +74,7 @@ func NewAPISecretPublisher(c client.Client, ot runtime.ObjectTyper) *APISecretPu // backward compatibility with the original API of this function. return &APISecretPublisher{ secret: resource.NewApplicatorWithRetry(resource.NewAPIPatchingApplicator(c), - resource.IsAPIErrorWrapped, nil), + resource.IsNonConflictAPIErrorWrapped, nil), typer: ot, } } diff --git a/pkg/resource/api.go b/pkg/resource/api.go index 8c60dd3e4..5ca91dfec 100644 --- a/pkg/resource/api.go +++ b/pkg/resource/api.go @@ -17,12 +17,14 @@ limitations under the License. package resource import ( + "bytes" "context" - "encoding/json" + jsonpatch "github.com/evanphx/json-patch" kerrors "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/runtime/serializer/json" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" @@ -33,6 +35,12 @@ 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 @@ -50,41 +58,97 @@ func NewAPIPatchingApplicator(c client.Client) *APIPatchingApplicator { // Apply changes to the supplied object. The object will be created if it does // not exist, or patched if it does. If the object does exist, it will only be // patched if the passed object has the same or an empty resource version. -func (a *APIPatchingApplicator) Apply(ctx context.Context, o client.Object, ao ...ApplyOption) error { - m, ok := o.(metav1.Object) - if !ok { - return errors.New("cannot access object metadata") - } - - if m.GetName() == "" && m.GetGenerateName() != "" { - return errors.Wrap(a.client.Create(ctx, o), "cannot create object") +func (a *APIPatchingApplicator) Apply(ctx context.Context, obj client.Object, ao ...ApplyOption) error { + if obj.GetName() == "" && obj.GetGenerateName() != "" { + return a.client.Create(ctx, obj) } - desired := o.DeepCopyObject() - - err := a.client.Get(ctx, types.NamespacedName{Name: m.GetName(), Namespace: m.GetNamespace()}, o) + 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, o), "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, o, desired); err != nil { + // Note: this check would ideally not be necessary if the Apply signature + // 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() { + gvr, err := groupResource(a.client, obj) + if err != nil { return err } + return kerrors.NewConflict(gvr, current.GetName(), errors.New(errOptimisticLock)) + } + + for _, fn := range ao { + if err := fn(ctx, current, obj); err != nil { + return errors.Wrapf(err, "apply option failed for %s", HumanReadableReference(a.client, obj)) + } } - // TODO(negz): Allow callers to override the kind of patch used. - return errors.Wrap(a.client.Patch(ctx, o, &patch{desired}), "cannot patch object") + return a.client.Patch(ctx, obj, client.MergeFromWithOptions(current, client.MergeFromWithOptimisticLock{})) } -type patch struct{ from runtime.Object } +func groupResource(c client.Client, o client.Object) (schema.GroupResource, error) { + gvk, err := c.GroupVersionKindFor(o) + if err != nil { + return schema.GroupResource{}, errors.Wrapf(err, "cannot determine group version kind of %T", o) + } + m, err := c.RESTMapper().RESTMapping(gvk.GroupKind(), gvk.Version) + if err != nil { + return schema.GroupResource{}, errors.Wrapf(err, "cannot determine group resource of %v", gvk) + } + return m.Resource.GroupResource(), nil +} -func (p *patch) Type() types.PatchType { return types.MergePatchType } -func (p *patch) Data(_ client.Object) ([]byte, error) { return json.Marshal(p.from) } +var emptyScheme = runtime.NewScheme() // no need to recognize any types +var jsonSerializer = json.NewSerializerWithOptions(json.DefaultMetaFactory, emptyScheme, emptyScheme, json.SerializerOptions{}) + +// AdditiveMergePatchApplyOption returns an ApplyOption that makes +// the Apply additive in the sense of a merge patch without null values. This is +// the old behavior of the APIPatchingApplicator. +// +// This only works with a desired object of type *unstructured.Unstructured. +// +// Deprecated: replace with Server Side Apply. +func AdditiveMergePatchApplyOption(_ context.Context, current, desired runtime.Object) error { + // set GVK uniformly to the desired object to make serializer happy + currentGVK, desiredGVK := current.GetObjectKind().GroupVersionKind(), desired.GetObjectKind().GroupVersionKind() + if !desiredGVK.Empty() && currentGVK != desiredGVK { + return errors.Errorf("cannot apply %v to %v", desired.GetObjectKind().GroupVersionKind(), current.GetObjectKind().GroupVersionKind()) + } + desired.GetObjectKind().SetGroupVersionKind(currentGVK) + + // merge `desired` additively with `current` + var currentBytes, desiredBytes bytes.Buffer + if err := jsonSerializer.Encode(current, ¤tBytes); err != nil { + return errors.Wrapf(err, "cannot marshal current %s", HumanReadableReference(nil, current)) + } + if err := jsonSerializer.Encode(desired, &desiredBytes); err != nil { + return errors.Wrapf(err, "cannot marshal desired %s", HumanReadableReference(nil, desired)) + } + mergedBytes, err := jsonpatch.MergePatch(currentBytes.Bytes(), desiredBytes.Bytes()) + if err != nil { + return errors.Wrapf(err, "cannot merge patch to %s", HumanReadableReference(nil, desired)) + } + + // write merged object back to `desired` + if _, _, err := jsonSerializer.Decode(mergedBytes, nil, desired); err != nil { + return errors.Wrapf(err, "cannot unmarshal merged patch to %s", HumanReadableReference(nil, desired)) + } + + // restore empty GVK for typed objects + if _, isUnstructured := desired.(runtime.Unstructured); !isUnstructured { + desired.GetObjectKind().SetGroupVersionKind(schema.GroupVersionKind{}) + } + + return nil +} // An APIUpdatingApplicator applies changes to an object by either creating or // updating it in a Kubernetes API server. @@ -94,43 +158,37 @@ type APIUpdatingApplicator struct { // NewAPIUpdatingApplicator returns an Applicator that applies changes to an // object by either creating or updating it in a Kubernetes API server. +// +// 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} } // Apply changes to the supplied object. The object will be created if it does // not exist, or updated if it does. -func (a *APIUpdatingApplicator) Apply(ctx context.Context, o client.Object, ao ...ApplyOption) error { - m, ok := o.(Object) - if !ok { - return errors.New("cannot access object metadata") +func (a *APIUpdatingApplicator) Apply(ctx context.Context, obj client.Object, ao ...ApplyOption) error { + if obj.GetName() == "" && obj.GetGenerateName() != "" { + return a.client.Create(ctx, obj) } - if m.GetName() == "" && m.GetGenerateName() != "" { - return errors.Wrap(a.client.Create(ctx, o), "cannot create object") - } - - current := o.DeepCopyObject().(client.Object) - - err := a.client.Get(ctx, types.NamespacedName{Name: m.GetName(), Namespace: m.GetNamespace()}, current) + 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, m), "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, m); err != nil { - return err + if err := fn(ctx, current, obj); err != nil { + return errors.Wrapf(err, "apply option failed for %s", HumanReadableReference(a.client, obj)) } } - // NOTE(hasheddan): we must set the resource version of the desired object - // to that of the current or the update will always fail. - m.SetResourceVersion(current.(metav1.Object).GetResourceVersion()) - return errors.Wrap(a.client.Update(ctx, m), "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..25ea99a1f 100644 --- a/pkg/resource/api_test.go +++ b/pkg/resource/api_test.go @@ -18,14 +18,20 @@ package resource import ( "context" + "encoding/json" "testing" + jsonpatch "github.com/evanphx/json-patch" "github.com/google/go-cmp/cmp" + corev1 "k8s.io/api/core/v1" 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/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/yaml" "github.com/crossplane/crossplane-runtime/pkg/errors" "github.com/crossplane/crossplane-runtime/pkg/resource/fake" @@ -34,8 +40,32 @@ 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) + + // for additive merge patch option test + currentYAML := ` +metadata: + resourceVersion: "42" +a: old +b: old +` type args struct { ctx context.Context @@ -56,53 +86,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 +159,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 +174,121 @@ 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)), + }, + }, + "AdditiveMergePatch": { + reason: "No error with the old additive behaviour if desired", + c: &test.MockClient{ + MockGet: test.NewMockGetFn(nil, func(o client.Object) error { + o.(*unstructured.Unstructured).Object = map[string]interface{}{} + return yaml.Unmarshal([]byte(currentYAML), &o.(*unstructured.Unstructured).Object) + }), + MockPatch: func(_ context.Context, o client.Object, patch client.Patch, _ ...client.PatchOption) error { + bs, err := patch.Data(o) + if err != nil { + return err + } + currentJSON, err := yaml.YAMLToJSON([]byte(currentYAML)) + if err != nil { + return err + } + patched, err := jsonpatch.MergePatch(currentJSON, bs) + if err != nil { + return err + } + o.(*unstructured.Unstructured).Object = map[string]interface{}{} + if err := json.Unmarshal(patched, &o.(*unstructured.Unstructured).Object); err != nil { + return err + } + o.SetResourceVersion("43") + return nil + }, + MockGroupVersionKindFor: test.NewMockGroupVersionKindForFn(nil, gvk), + MockRESTMapper: test.NewMockRESTMapperFn(fakeRESTMapper), + }, + args: args{ + o: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "kind": "Thing", + "metadata": map[string]interface{}{ + "resourceVersion": "42", + }, + "b": "changed", + "c": "added", + }, + }, + ao: []ApplyOption{AdditiveMergePatchApplyOption}, + }, + want: want{ + o: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "kind": "Thing", + "metadata": map[string]interface{}{ + "resourceVersion": "43", + }, + "a": "old", + "b": "changed", + "c": "added", + }, + }, }, }, } @@ -155,10 +309,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 +347,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), }, }, } @@ -396,3 +588,263 @@ func TestAPIFinalizerAdder(t *testing.T) { }) } } + +func TestAdditiveMergePatchApplyOption(t *testing.T) { + type args struct { + current runtime.Object + desired runtime.Object + } + type want struct { + err error + current runtime.Object + desired runtime.Object + } + tests := []struct { + name string + args args + want want + }{ + { + name: "equal unstructed", + args: args{ + current: &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": "example.com/v1", + "kind": "Thing", + "a": "foo", + }}, + desired: &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": "example.com/v1", + "kind": "Thing", + "a": "foo", + }}, + }, + want: want{ + current: &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": "example.com/v1", + "kind": "Thing", + "a": "foo", + }}, + desired: &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": "example.com/v1", + "kind": "Thing", + "a": "foo", + }}, + }, + }, + { + name: "overlapping unstructed", + args: args{ + current: &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": "example.com/v1", + "kind": "Thing", + "a": "foo", + "b": "foo", + "c": "foo", + }}, + desired: &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": "example.com/v1", + "kind": "Thing", + "a": "foo", + "b": "bar", + "d": "bar", + }}, + }, + want: want{ + current: &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": "example.com/v1", + "kind": "Thing", + "a": "foo", + "b": "foo", + "c": "foo", + }}, + desired: &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": "example.com/v1", + "kind": "Thing", + "a": "foo", + "b": "bar", + "c": "foo", + "d": "bar", + }}, + }, + }, + { + name: "equal typed", + args: args{ + current: &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{ + "a": "foo", + }}}, + desired: &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{ + "a": "foo", + }}}, + }, + want: want{ + current: &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{ + "a": "foo", + }}}, + desired: &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{ + "a": "foo", + }}}, + }, + }, + { + name: "overlapping typed", + args: args{ + current: &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{ + "a": "foo", + "b": "foo", + "c": "foo", + }}}, + desired: &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{ + "a": "foo", + "b": "bar", + "d": "bar", + }}}, + }, + want: want{ + current: &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{ + "a": "foo", + "b": "foo", + "c": "foo", + }}}, + desired: &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{ + "a": "foo", + "b": "bar", + "c": "foo", + "d": "bar", + }}}, + }, + }, + { + name: "equal mixed", + args: args{ + current: &unstructured.Unstructured{Object: map[string]interface{}{ + "kind": "Thing", + "metadata": map[string]interface{}{ + "labels": map[string]interface{}{ + "a": "foo", + }, + }, + }}, + desired: &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{ + "a": "foo", + }}}, + }, + want: want{ + current: &unstructured.Unstructured{Object: map[string]interface{}{ + "kind": "Thing", + "metadata": map[string]interface{}{ + "labels": map[string]interface{}{ + "a": "foo", + }, + }, + }}, + desired: &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{ + "a": "foo", + }}}, + }, + }, + { + name: "overlapping mixed", + args: args{ + current: &unstructured.Unstructured{Object: map[string]interface{}{ + "kind": "Thing", + "metadata": map[string]interface{}{ + "labels": map[string]interface{}{ + "a": "foo", + "b": "foo", + "c": "foo", + }, + }, + }}, + desired: &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{ + "a": "foo", + "b": "bar", + "d": "bar", + }}}, + }, + want: want{ + current: &unstructured.Unstructured{Object: map[string]interface{}{ + "kind": "Thing", + "metadata": map[string]interface{}{ + "labels": map[string]interface{}{ + "a": "foo", + "b": "foo", + "c": "foo", + }, + }, + }}, + desired: &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{ + "a": "foo", + "b": "bar", + "c": "foo", + "d": "bar", + }}}, + }, + }, + { + name: "incomplete desired", + args: args{ + current: &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": "example.com/v1", + "kind": "Thing", + "a": "foo", + }}, + desired: &unstructured.Unstructured{Object: map[string]interface{}{ + "a": "foo", + }}, + }, + want: want{ + current: &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": "example.com/v1", + "kind": "Thing", + "a": "foo", + }}, + desired: &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": "example.com/v1", + "kind": "Thing", + "a": "foo", + }}, + }, + }, + { + name: "different GVKs", + args: args{ + current: &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": "example.com/v1", + "kind": "Thing", + }}, + desired: &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": "example.com/v1", + "kind": "SomethingElse", + "a": "foo", + }}, + }, + want: want{ + err: errors.New("cannot apply example.com/v1, Kind=SomethingElse to example.com/v1, Kind=Thing"), + current: &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": "example.com/v1", + "kind": "Thing", + }}, + desired: &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": "example.com/v1", + "kind": "SomethingElse", + "a": "foo", + }}, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := AdditiveMergePatchApplyOption(context.Background(), tt.args.current, tt.args.desired) + if diff := cmp.Diff(tt.want.err, err, test.EquateErrors()); diff != "" { + t.Errorf("AdditiveMergePatchApplyOption() error = %v, wantErr %v", err, tt.want.err) + } + if diff := cmp.Diff(tt.want.current, tt.args.current); diff != "" { + t.Errorf("AdditiveMergePatchApplyOption()\ncurrent = %v\nwant = %v\ndiff = %s", tt.args.current, tt.want.current, diff) + } + if diff := cmp.Diff(tt.want.desired, tt.args.desired); diff != "" { + t.Errorf("AdditiveMergePatchApplyOption()\ncurrent = %v\nwant = %v\ndiff = %s", tt.args.desired, tt.want.desired, diff) + } + }) + } +} diff --git a/pkg/resource/resource.go b/pkg/resource/resource.go index 7c3f3b6b6..149cc83a2 100644 --- a/pkg/resource/resource.go +++ b/pkg/resource/resource.go @@ -18,6 +18,7 @@ package resource import ( "context" + "fmt" "strings" corev1 "k8s.io/api/core/v1" @@ -190,13 +191,24 @@ func IsAPIErrorWrapped(err error) bool { return IsAPIError(errors.Cause(err)) } +// IsNonConflictAPIErrorWrapped returns true if err is a non-conflict K8s API +// error, or recursively wraps a K8s API error +func IsNonConflictAPIErrorWrapped(err error) bool { + cause := errors.Cause(err) + return IsAPIError(cause) && !kerrors.IsConflict(cause) +} + // IsConditionTrue returns if condition status is true func IsConditionTrue(c xpv1.Condition) bool { return c.Status == corev1.ConditionTrue } -// An Applicator applies changes to an object. +// 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 } @@ -374,3 +386,35 @@ 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. +// +// The client is optional and can be nil. Then the kind is guessed from the +// object. +func HumanReadableReference(c client.Client, o runtime.Object) string { + gvk := o.GetObjectKind().GroupVersionKind() + if gvk.Kind == "" && c != nil { + gvk, _ = c.GroupVersionKindFor(o) + } + if gvk.Kind == "" { + gvk.Kind = fmt.Sprintf("%T", o) // best effort + } + + co, ok := o.(client.Object) + if !ok { + return gvk.Kind + } + + name := co.GetName() + infix := "" + if gn := co.GetGenerateName(); name == "" && gn != "" { + name = gn + infix = "with generated name " + } + if ns := co.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