Merge pull request #1942 from cezarygerard/catch-1.20-negs

[Cherry pick #1926] Remove Services.Get() from syncNegStatusAnnotation()
kubernetes · Feb 13, 2023 · 9cea723 · 9cea723
2 parents 2bdfaed + 0d19cff
commit 9cea723
Show file tree

Hide file tree

Showing 2 changed files with 20 additions and 6 deletions.
diff --git a/pkg/neg/controller.go b/pkg/neg/controller.go
@@ -694,24 +694,30 @@ func (c *Controller) syncNegStatusAnnotation(namespace, name string, portMap neg
 	if err != nil {
 		return err
 	}
-	coreClient := c.client.CoreV1()
-	service, err := coreClient.Services(namespace).Get(context.TODO(), name, metav1.GetOptions{})
+	obj, exists, err := c.serviceLister.GetByKey(getServiceKey(namespace, name).Key())
 	if err != nil {
 		return err
 	}
+	if !exists {
+		// Service no longer exists so doesn't require any update.
+		return nil
+	}
+	service, ok := obj.(*apiv1.Service)
+	if !ok {
+		return fmt.Errorf("cannot convert obj to Service; obj=%T", obj)
+	}
 
 	// Remove NEG Status Annotation when no NEG is needed
 	if len(portMap) == 0 {
 		if _, ok := service.Annotations[annotations.NEGStatusKey]; ok {
 			newSvcObjectMeta := service.ObjectMeta.DeepCopy()
 			delete(newSvcObjectMeta.Annotations, annotations.NEGStatusKey)
 			c.logger.V(2).Info("Removing NEG status annotation from service", "service", klog.KRef(namespace, name))
-			return patch.PatchServiceObjectMetadata(coreClient, service, *newSvcObjectMeta)
+			return patch.PatchServiceObjectMetadata(c.client.CoreV1(), service, *newSvcObjectMeta)
 		}
 		// service doesn't have the expose NEG annotation and doesn't need update
 		return nil
 	}
-
 	negStatus := annotations.NewNegStatus(zones, portMap.ToPortNegMap())
 	annotation, err := negStatus.Marshal()
 	if err != nil {
@@ -728,7 +734,7 @@ func (c *Controller) syncNegStatusAnnotation(namespace, name string, portMap neg
 	}
 	newSvcObjectMeta.Annotations[annotations.NEGStatusKey] = annotation
 	c.logger.V(2).Info("Updating NEG visibility annotation on service", "annotation", annotation, "service", klog.KRef(namespace, name))
-	return patch.PatchServiceObjectMetadata(coreClient, service, *newSvcObjectMeta)
+	return patch.PatchServiceObjectMetadata(c.client.CoreV1(), service, *newSvcObjectMeta)
 }
 
 // syncDestinationRuleNegStatusAnnotation syncs the destinationrule related neg status annotation

diff --git a/pkg/neg/controller_test.go b/pkg/neg/controller_test.go
@@ -685,7 +685,7 @@ func TestSyncNegAnnotation(t *testing.T) {
 	controller := newTestController(fake.NewSimpleClientset())
 	defer controller.stop()
 	svcClient := controller.client.CoreV1().Services(testServiceNamespace)
-	newTestService(controller, false, []int32{})
+	svc := newTestService(controller, false, []int32{})
 	namespace := testServiceNamespace
 	name := testServiceName
 	namer := controller.namer
@@ -734,6 +734,7 @@ func TestSyncNegAnnotation(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Run(tc.desc, func(t *testing.T) {
+			controller.serviceLister.Add(svc)
 			controller.syncNegStatusAnnotation(namespace, name, tc.previousPortMap)
 			svc, _ := svcClient.Get(context.TODO(), name, metav1.GetOptions{})
 
@@ -743,6 +744,10 @@ func TestSyncNegAnnotation(t *testing.T) {
 			}
 			validateServiceStateAnnotation(t, svc, oldSvcPorts, controller.namer)
 
+			// Mimic any Update calls to the API Server by manually updating the
+			// informer cache.
+			controller.serviceLister.Update(svc)
+
 			controller.syncNegStatusAnnotation(namespace, name, tc.portMap)
 			svc, _ = svcClient.Get(context.TODO(), name, metav1.GetOptions{})
 
@@ -751,6 +756,9 @@ func TestSyncNegAnnotation(t *testing.T) {
 				svcPorts = append(svcPorts, port.ServicePort)
 			}
 			validateServiceStateAnnotation(t, svc, svcPorts, controller.namer)
+
+			// Reset state of controller for the next test run.
+			controller.serviceLister.Delete(svc)
 		})
 	}
 }