From 4de7c6c92f74abe3409ab99d78f284ee78f1a740 Mon Sep 17 00:00:00 2001 From: Satish Matti Date: Tue, 2 Jun 2020 17:17:24 -0700 Subject: [PATCH] Use Patch instead of Update for k8s Service client --- pkg/l4/l4controller.go | 7 +- pkg/neg/controller.go | 21 ++-- pkg/utils/common/finalizer.go | 47 ++------- pkg/utils/patch/patch.go | 58 ++++-------- pkg/utils/patch/patch_test.go | 174 ++++++++++++++++++++++++++++++++++ 5 files changed, 216 insertions(+), 91 deletions(-) diff --git a/pkg/l4/l4controller.go b/pkg/l4/l4controller.go index 2d31584b22..57bc31fb1a 100644 --- a/pkg/l4/l4controller.go +++ b/pkg/l4/l4controller.go @@ -230,12 +230,7 @@ func (l4c *L4Controller) updateServiceStatus(svc *v1.Service, newStatus *v1.Load if helper.LoadBalancerStatusEqual(&svc.Status.LoadBalancer, newStatus) { return nil } - updated := svc.DeepCopy() - updated.Status.LoadBalancer = *newStatus - if _, err := patch.PatchService(l4c.ctx.KubeClient.CoreV1(), svc, updated); err != nil { - return err - } - return nil + return patch.PatchServiceLoadBalancerStatus(l4c.ctx.KubeClient.CoreV1(), svc, *newStatus) } func needsDeletion(svc *v1.Service) bool { diff --git a/pkg/neg/controller.go b/pkg/neg/controller.go index 54d3aea0b8..6bf5a03438 100644 --- a/pkg/neg/controller.go +++ b/pkg/neg/controller.go @@ -585,8 +585,8 @@ func (c *Controller) syncNegStatusAnnotation(namespace, name string, portMap neg if err != nil { return err } - svcClient := c.client.CoreV1().Services(namespace) - service, err := svcClient.Get(context2.TODO(), name, metav1.GetOptions{}) + coreClient := c.client.CoreV1() + service, err := coreClient.Services(namespace).Get(context2.TODO(), name, metav1.GetOptions{}) if err != nil { return err } @@ -594,11 +594,10 @@ func (c *Controller) syncNegStatusAnnotation(namespace, name string, portMap neg // Remove NEG Status Annotation when no NEG is needed if len(portMap) == 0 { if _, ok := service.Annotations[annotations.NEGStatusKey]; ok { - // TODO: use PATCH to remove annotation - delete(service.Annotations, annotations.NEGStatusKey) + newSvcObjectMeta := service.ObjectMeta.DeepCopy() + delete(newSvcObjectMeta.Annotations, annotations.NEGStatusKey) klog.V(2).Infof("Removing NEG status annotation from service: %s/%s", namespace, name) - _, err = svcClient.Update(context2.TODO(), service, metav1.UpdateOptions{}) - return err + return patch.PatchServiceObjectMetadata(coreClient, service, *newSvcObjectMeta) } // service doesn't have the expose NEG annotation and doesn't need update return nil @@ -613,14 +612,14 @@ func (c *Controller) syncNegStatusAnnotation(namespace, name string, portMap neg if ok && existingAnnotation == annotation { return nil } + newSvcObjectMeta := service.ObjectMeta.DeepCopy() // If enableCSM=true, it's possible a service having nil Annotations. - if service.Annotations == nil { - service.Annotations = make(map[string]string) + if newSvcObjectMeta.Annotations == nil { + newSvcObjectMeta.Annotations = make(map[string]string) } - service.Annotations[annotations.NEGStatusKey] = annotation + newSvcObjectMeta.Annotations[annotations.NEGStatusKey] = annotation klog.V(2).Infof("Updating NEG visibility annotation %q on service %s/%s.", annotation, namespace, name) - _, err = svcClient.Update(context2.TODO(), service, metav1.UpdateOptions{}) - return err + return patch.PatchServiceObjectMetadata(coreClient, service, *newSvcObjectMeta) } // syncDestinationRuleNegStatusAnnotation syncs the destinationrule related neg status annotation diff --git a/pkg/utils/common/finalizer.go b/pkg/utils/common/finalizer.go index 7c2e0e8ea0..b4cee1cfef 100644 --- a/pkg/utils/common/finalizer.go +++ b/pkg/utils/common/finalizer.go @@ -14,17 +14,12 @@ limitations under the License. package common import ( - "context" - "encoding/json" "fmt" corev1 "k8s.io/api/core/v1" "k8s.io/api/networking/v1beta1" meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/types" - "k8s.io/apimachinery/pkg/util/strategicpatch" "k8s.io/client-go/kubernetes" - coreclient "k8s.io/client-go/kubernetes/typed/core/v1" client "k8s.io/client-go/kubernetes/typed/networking/v1beta1" "k8s.io/ingress-gce/pkg/utils/patch" "k8s.io/klog" @@ -101,12 +96,12 @@ func EnsureServiceFinalizer(service *corev1.Service, key string, kubeClient kube return nil } - // Make a copy so we don't mutate the shared informer cache. - updated := service.DeepCopy() - updated.ObjectMeta.Finalizers = append(updated.ObjectMeta.Finalizers, key) + // Make a copy of object metadata so we don't mutate the shared informer cache. + updatedObjectMeta := service.ObjectMeta.DeepCopy() + updatedObjectMeta.Finalizers = append(updatedObjectMeta.Finalizers, key) - klog.V(2).Infof("Adding finalizer %s to service %s/%s", key, updated.Namespace, updated.Name) - return patchServiceFinalizer(kubeClient.CoreV1().Services(updated.Namespace), service, updated) + klog.V(2).Infof("Adding finalizer %s to service %s/%s", key, service.Namespace, service.Name) + return patch.PatchServiceObjectMetadata(kubeClient.CoreV1(), service, *updatedObjectMeta) } // removeFinalizer patches the service to remove finalizer. @@ -115,32 +110,10 @@ func EnsureDeleteServiceFinalizer(service *corev1.Service, key string, kubeClien return nil } - // Make a copy so we don't mutate the shared informer cache. - updated := service.DeepCopy() - updated.ObjectMeta.Finalizers = slice.RemoveString(updated.ObjectMeta.Finalizers, key, nil) + // Make a copy of object metadata so we don't mutate the shared informer cache. + updatedObjectMeta := service.ObjectMeta.DeepCopy() + updatedObjectMeta.Finalizers = slice.RemoveString(updatedObjectMeta.Finalizers, key, nil) - klog.V(2).Infof("Removing finalizer from service %s/%s", updated.Namespace, updated.Name) - return patchServiceFinalizer(kubeClient.CoreV1().Services(updated.Namespace), service, updated) -} - -func patchServiceFinalizer(sc coreclient.ServiceInterface, oldSvc, newSvc *corev1.Service) error { - svcKey := fmt.Sprintf("%s/%s", oldSvc.Namespace, oldSvc.Name) - oldData, err := json.Marshal(oldSvc) - if err != nil { - return fmt.Errorf("failed to Marshal oldData for service %s: %v", svcKey, err) - } - - newData, err := json.Marshal(newSvc) - if err != nil { - return fmt.Errorf("failed to Marshal newData for service %s: %v", svcKey, err) - } - - patchBytes, err := strategicpatch.CreateTwoWayMergePatch(oldData, newData, corev1.Service{}) - if err != nil { - return fmt.Errorf("failed to create TwoWayMergePatch for service %s: %v", svcKey, err) - } - - klog.V(3).Infof("Patch bytes for service %s: %s", svcKey, patchBytes) - _, err = sc.Patch(context.TODO(), oldSvc.Name, types.StrategicMergePatchType, patchBytes, meta_v1.PatchOptions{}, "status") - return err + klog.V(2).Infof("Removing finalizer from service %s/%s", service.Namespace, service.Name) + return patch.PatchServiceObjectMetadata(kubeClient.CoreV1(), service, *updatedObjectMeta) } diff --git a/pkg/utils/patch/patch.go b/pkg/utils/patch/patch.go index 24a18a5aa8..3a272432d7 100644 --- a/pkg/utils/patch/patch.go +++ b/pkg/utils/patch/patch.go @@ -18,13 +18,14 @@ import ( "encoding/json" "fmt" - "k8s.io/api/core/v1" + corev1 "k8s.io/api/core/v1" "k8s.io/api/networking/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/strategicpatch" - corev1 "k8s.io/client-go/kubernetes/typed/core/v1" + coreclient "k8s.io/client-go/kubernetes/typed/core/v1" client "k8s.io/client-go/kubernetes/typed/networking/v1beta1" + svchelpers "k8s.io/cloud-provider/service/helpers" "k8s.io/klog" ) @@ -49,41 +50,6 @@ func StrategicMergePatchBytes(old, cur, refStruct interface{}) ([]byte, error) { return patchBytes, nil } -// TODO remove these after picking up https://github.com/kubernetes/kubernetes/pull/87217 -// PatchService patches the given service's Status or ObjectMeta based on the original and -// updated ones. Change to spec will be ignored. -func PatchService(c corev1.CoreV1Interface, oldSvc, newSvc *v1.Service) (*v1.Service, error) { - // Reset spec to make sure only patch for Status or ObjectMeta. - newSvc.Spec = oldSvc.Spec - - patchBytes, err := getPatchBytes(oldSvc, newSvc) - if err != nil { - return nil, err - } - - return c.Services(oldSvc.Namespace).Patch(context.TODO(), oldSvc.Name, types.MergePatchType, patchBytes, metav1.PatchOptions{}, "status") - -} - -func getPatchBytes(oldSvc, newSvc *v1.Service) ([]byte, error) { - oldData, err := json.Marshal(oldSvc) - if err != nil { - return nil, fmt.Errorf("failed to Marshal oldData for svc %s/%s: %v", oldSvc.Namespace, oldSvc.Name, err) - } - - newData, err := json.Marshal(newSvc) - if err != nil { - return nil, fmt.Errorf("failed to Marshal newData for svc %s/%s: %v", newSvc.Namespace, newSvc.Name, err) - } - - patchBytes, err := strategicpatch.CreateTwoWayMergePatch(oldData, newData, v1.Service{}) - if err != nil { - return nil, fmt.Errorf("failed to CreateTwoWayMergePatch for svc %s/%s: %v", oldSvc.Namespace, oldSvc.Name, err) - } - return patchBytes, nil - -} - // PatchIngressObjectMetadata patches the given ingress's metadata based on new // ingress metadata. func PatchIngressObjectMetadata(ic client.IngressInterface, ing *v1beta1.Ingress, newObjectMetadata metav1.ObjectMeta) (*v1beta1.Ingress, error) { @@ -124,3 +90,21 @@ func patchIngress(ic client.IngressInterface, oldIngress, newIngress *v1beta1.In klog.V(4).Infof("Patch bytes for ingress %s: %s", ingKey, patchBytes) return ic.Patch(context.TODO(), oldIngress.Name, types.StrategicMergePatchType, patchBytes, metav1.PatchOptions{}, "status") } + +// PatchServiceObjectMetadata patches the given service's metadata based on new +// service metadata. +func PatchServiceObjectMetadata(client coreclient.CoreV1Interface, svc *corev1.Service, newObjectMetadata metav1.ObjectMeta) error { + newSvc := svc.DeepCopy() + newSvc.ObjectMeta = newObjectMetadata + _, err := svchelpers.PatchService(client, svc, newSvc) + return err +} + +// PatchServiceLoadBalancerStatus patches the given service's LoadBalancerStatus +// based on new service's load-balancer status. +func PatchServiceLoadBalancerStatus(client coreclient.CoreV1Interface, svc *corev1.Service, newStatus corev1.LoadBalancerStatus) error { + newSvc := svc.DeepCopy() + newSvc.Status.LoadBalancer = newStatus + _, err := svchelpers.PatchService(client, svc, newSvc) + return err +} diff --git a/pkg/utils/patch/patch_test.go b/pkg/utils/patch/patch_test.go index 07c31cb2c9..4ad70a8da0 100644 --- a/pkg/utils/patch/patch_test.go +++ b/pkg/utils/patch/patch_test.go @@ -224,3 +224,177 @@ func newTestIngress(namespace, name string) *v1beta1.Ingress { }, } } + +func TestPatchServiceObjectMetadata(t *testing.T) { + for _, tc := range []struct { + desc string + svc *apiv1.Service + newMetaFunc func(*apiv1.Service) *apiv1.Service + }{ + { + desc: "add annotation", + svc: newTestService("ns1", "add-annotation-svc"), + newMetaFunc: func(svc *apiv1.Service) *apiv1.Service { + ret := svc.DeepCopy() + ret.Annotations["test-annotation-key3"] = "test-value3" + return ret + }, + }, + { + desc: "delete annotation", + svc: newTestService("ns2", "delete-annotation-svc"), + newMetaFunc: func(svc *apiv1.Service) *apiv1.Service { + ret := svc.DeepCopy() + delete(ret.Annotations, testAnnotationKey) + return ret + }, + }, + { + desc: "delete all annotations", + svc: newTestService("ns3", "delete-all-annotations-svc"), + newMetaFunc: func(svc *apiv1.Service) *apiv1.Service { + ret := svc.DeepCopy() + ret.Annotations = nil + return ret + }, + }, + { + desc: "add finalizer", + svc: newTestService("ns4", "add-finalizer-svc"), + newMetaFunc: func(svc *apiv1.Service) *apiv1.Service { + ret := svc.DeepCopy() + ret.Finalizers = append(ret.Finalizers, "new-test-finalizer") + return ret + }, + }, + { + desc: "delete finalizer", + svc: newTestService("ns5", "delete-finalizer-svc"), + newMetaFunc: func(svc *apiv1.Service) *apiv1.Service { + ret := svc.DeepCopy() + ret.Finalizers = slice.RemoveString(ret.Finalizers, testFinalizer, nil) + return ret + }, + }, + { + desc: "delete all finalizers", + svc: newTestService("ns6", "delete-all-finalizers-svc"), + newMetaFunc: func(svc *apiv1.Service) *apiv1.Service { + ret := svc.DeepCopy() + ret.Finalizers = nil + return ret + }, + }, + { + desc: "delete both annotation and finalizer", + svc: newTestService("ns7", "delete-annotation-and-finalizer-svc"), + newMetaFunc: func(svc *apiv1.Service) *apiv1.Service { + ret := svc.DeepCopy() + ret.Finalizers = slice.RemoveString(ret.Finalizers, testFinalizer, nil) + delete(ret.Annotations, testAnnotationKey) + return ret + }, + }, + } { + t.Run(tc.desc, func(t *testing.T) { + svcKey := fmt.Sprintf("%s/%s", tc.svc.Namespace, tc.svc.Name) + coreClient := fake.NewSimpleClientset().CoreV1() + if _, err := coreClient.Services(tc.svc.Namespace).Create(context.TODO(), tc.svc, metav1.CreateOptions{}); err != nil { + t.Fatalf("Create(%s) = %v, want nil", svcKey, err) + } + expectSvc := tc.newMetaFunc(tc.svc) + err := PatchServiceObjectMetadata(coreClient, tc.svc, expectSvc.ObjectMeta) + if err != nil { + t.Fatalf("PatchServiceObjectMetadata(%s) = %v, want nil", svcKey, err) + } + + gotSvc, err := coreClient.Services(tc.svc.Namespace).Get(context.TODO(), tc.svc.Name, metav1.GetOptions{}) + if err != nil { + t.Fatalf("Get(%s) = %v, want nil", svcKey, err) + } + if diff := cmp.Diff(expectSvc, gotSvc); diff != "" { + t.Errorf("Got mismatch for Service (-want +got):\n%s", diff) + } + }) + } +} + +func TestPatchServiceLoadBalancerStatus(t *testing.T) { + for _, tc := range []struct { + desc string + svc *apiv1.Service + newMetaFunc func(*apiv1.Service) *apiv1.Service + }{ + { + desc: "update status", + svc: newTestService("ns1", "update-status-svc"), + newMetaFunc: func(svc *apiv1.Service) *apiv1.Service { + ret := svc.DeepCopy() + ret.Status = apiv1.ServiceStatus{ + LoadBalancer: apiv1.LoadBalancerStatus{ + Ingress: []apiv1.LoadBalancerIngress{ + {IP: "10.0.0.1"}, + }, + }, + } + return ret + }, + }, + { + desc: "delete status", + svc: newTestService("ns2", "delete-status-svc"), + newMetaFunc: func(svc *apiv1.Service) *apiv1.Service { + ret := svc.DeepCopy() + ret.Status = apiv1.ServiceStatus{} + return ret + }, + }, + } { + t.Run(tc.desc, func(t *testing.T) { + svcKey := fmt.Sprintf("%s/%s", tc.svc.Namespace, tc.svc.Name) + coreClient := fake.NewSimpleClientset().CoreV1() + if _, err := coreClient.Services(tc.svc.Namespace).Create(context.TODO(), tc.svc, metav1.CreateOptions{}); err != nil { + t.Fatalf("Create(%s) = %v, want nil", svcKey, err) + } + expectSvc := tc.newMetaFunc(tc.svc) + err := PatchServiceLoadBalancerStatus(coreClient, tc.svc, expectSvc.Status.LoadBalancer) + if err != nil { + t.Fatalf("PatchServiceLoadBalancerStatus(%s) = %v, want nil", svcKey, err) + } + + gotSvc, err := coreClient.Services(tc.svc.Namespace).Get(context.TODO(), tc.svc.Name, metav1.GetOptions{}) + if err != nil { + t.Fatalf("Get(%s) = %v, want nil", svcKey, err) + } + if diff := cmp.Diff(expectSvc, gotSvc); diff != "" { + t.Errorf("Got mismatch for Service (-want +got):\n%s", diff) + } + }) + } +} + +func newTestService(namespace, name string) *apiv1.Service { + return &apiv1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + Annotations: map[string]string{ + testAnnotationKey: "test-value1", + "test-annotations-key2": "test-value2", + }, + Finalizers: []string{testFinalizer}, + }, + Spec: apiv1.ServiceSpec{ + Ports: []apiv1.ServicePort{ + {Name: "http-80"}, + }, + }, + Status: apiv1.ServiceStatus{ + LoadBalancer: apiv1.LoadBalancerStatus{ + Ingress: []apiv1.LoadBalancerIngress{ + {IP: "127.0.0.1"}, + }, + }, + }, + } +}