diff --git a/util/patch/options.go b/util/patch/options.go new file mode 100644 index 000000000000..7ce739d36744 --- /dev/null +++ b/util/patch/options.go @@ -0,0 +1,37 @@ +/* +Copyright 2020 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package patch + +// WithStatusObservedGeneration sets the status.observedGeneration field +// on the incoming object to match metadata.generation, only if there is a change. +type WithStatusObservedGeneration struct{} + +// ApplyToHelper applies this configuration to the given an List options. +func (w WithStatusObservedGeneration) ApplyToHelper(in *HelperOptions) { + in.IncludeStatusObservedGeneration = true +} + +// Option is some configuration that modifies options for a patch request. +type Option interface { + // ApplyToHelper applies this configuration to the given Helper options. + ApplyToHelper(*HelperOptions) +} + +// HelperOptions contains options for patch options. +type HelperOptions struct { + IncludeStatusObservedGeneration bool +} diff --git a/util/patch/patch.go b/util/patch/patch.go index 91550c39ea6d..05dd7a026fea 100644 --- a/util/patch/patch.go +++ b/util/patch/patch.go @@ -1,5 +1,5 @@ /* -Copyright 2017 The Kubernetes Authors. +Copyright 2020 The Kubernetes Authors. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -30,20 +30,28 @@ import ( // Helper is a utility for ensuring the proper Patching of resources // and their status type Helper struct { - client client.Client - before map[string]interface{} - hasStatus bool - beforeStatus interface{} - resourcePatch client.Patch - statusPatch client.Patch + opts *HelperOptions + + client client.Client + before map[string]interface{} + beforeHasStatus bool + beforeStatus interface{} + resourcePatch client.Patch + statusPatch client.Patch } // NewHelper returns an initialized Helper -func NewHelper(resource runtime.Object, crClient client.Client) (*Helper, error) { +func NewHelper(resource runtime.Object, crClient client.Client, opts ...Option) (*Helper, error) { if resource == nil { return nil, errors.Errorf("expected non-nil resource") } + // Calculate the options to pass to the helper. + options := &HelperOptions{} + for _, opt := range opts { + opt.ApplyToHelper(options) + } + // If the object is already unstructured, we need to perform a deepcopy first // because the `DefaultUnstructuredConverter.ToUnstructured` function returns // the underlying unstructured object map without making a copy. @@ -71,12 +79,13 @@ func NewHelper(resource runtime.Object, crClient client.Client) (*Helper, error) } return &Helper{ - client: crClient, - before: before, - beforeStatus: beforeStatus, - hasStatus: hasStatus, - resourcePatch: client.MergeFrom(resource.DeepCopyObject()), - statusPatch: client.MergeFrom(resource.DeepCopyObject()), + opts: options, + client: crClient, + before: before, + beforeStatus: beforeStatus, + beforeHasStatus: hasStatus, + resourcePatch: client.MergeFrom(resource.DeepCopyObject()), + statusPatch: client.MergeFrom(resource.DeepCopyObject()), }, nil } @@ -86,45 +95,54 @@ func (h *Helper) Patch(ctx context.Context, resource runtime.Object) error { return errors.Errorf("expected non-nil resource") } - // If the object is already unstructured, we need to perform a deepcopy first - // because the `DefaultUnstructuredConverter.ToUnstructured` function returns - // the underlying unstructured object map without making a copy. - if _, ok := resource.(runtime.Unstructured); ok { - resource = resource.DeepCopyObject() - } - // Convert the resource to unstructured to compare against our before copy. - after, err := runtime.DefaultUnstructuredConverter.ToUnstructured(resource) + raw, err := runtime.DefaultUnstructuredConverter.ToUnstructured(resource.DeepCopyObject()) if err != nil { return err } + unstructuredResource := &unstructured.Unstructured{Object: raw} + + // Determine if the object has status. + afterHasStatus := unstructuredHasStatus(unstructuredResource) + if afterHasStatus { + if h.opts.IncludeStatusObservedGeneration { + // Set status.observedGeneration if we're asked to do so. + if err := unstructured.SetNestedField(unstructuredResource.Object, unstructuredResource.GetGeneration(), "status", "observedGeneration"); err != nil { + return err + } + + // Restore the changes back to the original object. + if err := runtime.DefaultUnstructuredConverter.FromUnstructured(unstructuredResource.Object, resource); err != nil { + return err + } + } + } - hasStatus := false - // attempt to extract the status from the resource to compare against our - // beforeStatus copy + // Make a copy of the resource to compare with before/beforeStatus. + after := unstructuredResource.DeepCopy().Object + + // Attempt to extract the status. afterStatus, ok, err := unstructured.NestedFieldCopy(after, "status") if err != nil { return err } if ok { - hasStatus = true - // if the resource contains a status remove it from our unstructured copy + // If the resource contains a status remove it from our unstructured copy // to avoid uneccsary patching. unstructured.RemoveNestedField(after, "status") } var errs []error + // If the before/after map (containing only metadata & spec) is not the same, issue a patch. if !reflect.DeepEqual(h.before, after) { - // only issue a Patch if the before and after resources (minus status) differ if err := h.client.Patch(ctx, resource.DeepCopyObject(), h.resourcePatch); err != nil { errs = append(errs, err) } } - if (h.hasStatus || hasStatus) && !reflect.DeepEqual(h.beforeStatus, afterStatus) { - // only issue a Status Patch if the resource has a status and the beforeStatus - // and afterStatus copies differ + // If the beforeStatus/afterStatus map (containing only status fields) is not the same, issue patch. + if (h.beforeHasStatus || afterHasStatus) && !reflect.DeepEqual(h.beforeStatus, afterStatus) { if err := h.client.Status().Patch(ctx, resource.DeepCopyObject(), h.statusPatch); err != nil { errs = append(errs, err) } @@ -132,3 +150,8 @@ func (h *Helper) Patch(ctx context.Context, resource runtime.Object) error { return kerrors.NewAggregate(errs) } + +func unstructuredHasStatus(resource *unstructured.Unstructured) bool { + _, ok := resource.Object["status"] + return ok +} diff --git a/util/patch/patch_test.go b/util/patch/patch_test.go index 814ce15f54c8..f3468eb82443 100644 --- a/util/patch/patch_test.go +++ b/util/patch/patch_test.go @@ -1,5 +1,5 @@ /* -Copyright 2017 The Kubernetes Authors. +Copyright 2020 The Kubernetes Authors. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -27,6 +27,7 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/client-go/kubernetes/scheme" + "k8s.io/utils/pointer" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" @@ -246,3 +247,218 @@ func TestHelperPatch(t *testing.T) { }) } } + +func TestHelperPatchOptions(t *testing.T) { + + tests := []struct { + name string + opts []Option + before runtime.Object + patchInput runtime.Object + patchOutput runtime.Object + wantErr bool + }{ + { + name: "Only spec update, include observed generation update", + opts: []Option{ + WithStatusObservedGeneration{}, + }, + before: &clusterv1.MachineSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-ms", + Namespace: "test-namespace", + Generation: 100, + }, + }, + patchInput: &clusterv1.MachineSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-ms", + Namespace: "test-namespace", + Generation: 100, + }, + Spec: clusterv1.MachineSetSpec{ + Replicas: pointer.Int32Ptr(1), + }, + }, + patchOutput: &clusterv1.MachineSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-ms", + Namespace: "test-namespace", + Generation: 100, + }, + Spec: clusterv1.MachineSetSpec{ + Replicas: pointer.Int32Ptr(1), + }, + Status: clusterv1.MachineSetStatus{ + ObservedGeneration: 100, + }, + }, + }, + { + name: "Spec and Status update, include observed generation update", + opts: []Option{ + WithStatusObservedGeneration{}, + }, + before: &clusterv1.MachineSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-ms", + Namespace: "test-namespace", + Generation: 100, + }, + }, + patchInput: &clusterv1.MachineSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-ms", + Namespace: "test-namespace", + Generation: 100, + }, + Spec: clusterv1.MachineSetSpec{ + Replicas: pointer.Int32Ptr(1), + }, + Status: clusterv1.MachineSetStatus{ + ObservedGeneration: 90, + AvailableReplicas: 10, + }, + }, + patchOutput: &clusterv1.MachineSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-ms", + Namespace: "test-namespace", + Generation: 100, + }, + Spec: clusterv1.MachineSetSpec{ + Replicas: pointer.Int32Ptr(1), + }, + Status: clusterv1.MachineSetStatus{ + ObservedGeneration: 100, + AvailableReplicas: 10, + }, + }, + }, + { + name: "No changes, observed generation should not update", + opts: []Option{ + WithStatusObservedGeneration{}, + }, + before: &clusterv1.MachineSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-ms", + Namespace: "test-namespace", + Generation: 100, + }, + Status: clusterv1.MachineSetStatus{ + ObservedGeneration: 100, + AvailableReplicas: 10, + }, + }, + patchInput: &clusterv1.MachineSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-ms", + Namespace: "test-namespace", + Generation: 100, + }, + Status: clusterv1.MachineSetStatus{ + ObservedGeneration: 100, + AvailableReplicas: 10, + }, + }, + patchOutput: &clusterv1.MachineSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-ms", + Namespace: "test-namespace", + Generation: 100, + }, + Status: clusterv1.MachineSetStatus{ + ObservedGeneration: 100, + AvailableReplicas: 10, + }, + }, + }, + { + name: "Only add ownerref update to unstructured object, update observed generation", + opts: []Option{ + WithStatusObservedGeneration{}, + }, + before: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "kind": "BootstrapConfig", + "apiVersion": "bootstrap.cluster.x-k8s.io/v1alpha3", + "metadata": map[string]interface{}{ + "name": "test-bootstrap", + "namespace": "default", + "generation": int64(10), + }, + }, + }, + patchInput: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "kind": "BootstrapConfig", + "apiVersion": "bootstrap.cluster.x-k8s.io/v1alpha3", + "metadata": map[string]interface{}{ + "name": "test-bootstrap", + "namespace": "default", + "generation": int64(10), + "ownerReferences": []interface{}{ + map[string]interface{}{ + "kind": "TestOwner", + "apiVersion": "test.cluster.x-k8s.io/v1alpha3", + "name": "test", + }, + }, + }, + "status": map[string]interface{}{ + "test": "1", + }, + }, + }, + patchOutput: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "kind": "BootstrapConfig", + "apiVersion": "bootstrap.cluster.x-k8s.io/v1alpha3", + "metadata": map[string]interface{}{ + "name": "test-bootstrap", + "namespace": "default", + "generation": int64(10), + "ownerReferences": []interface{}{ + map[string]interface{}{ + "kind": "TestOwner", + "apiVersion": "test.cluster.x-k8s.io/v1alpha3", + "name": "test", + }, + }, + }, + "status": map[string]interface{}{ + "test": "1", + "observedGeneration": int64(10), + }, + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + g.Expect(clusterv1.AddToScheme(scheme.Scheme)).To(Succeed()) + + ctx := context.Background() + fakeClient := fake.NewFakeClientWithScheme(scheme.Scheme) + + beforeCopy := tt.before.DeepCopyObject() + g.Expect(fakeClient.Create(ctx, beforeCopy)).To(Succeed()) + + h, err := NewHelper(beforeCopy, fakeClient, tt.opts...) + g.Expect(err).NotTo(HaveOccurred()) + + patchInput := tt.patchInput.DeepCopyObject() + err = h.Patch(ctx, patchInput) + if tt.wantErr { + g.Expect(err).To(HaveOccurred()) + } else { + g.Expect(err).NotTo(HaveOccurred()) + } + + g.Expect(patchInput).To(Equal(tt.patchOutput)) + }) + } +}