Use Patch instead of Update for k8s Service client

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 {

pkg/neg/controller.go

@@ -585,20 +585,19 @@ 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
 	}
 
 	// 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

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)
 }

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
+}

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"},
+				},
+			},
+		},
+	}
+}