Only process relevant updates in l4 controller

Addressed review comments

pkg/annotations/service.go

@@ -179,6 +179,25 @@ func WantsL4ILB(service *v1.Service) (bool, string) {
 	return false, fmt.Sprintf("Type : %s, LBType : %s", service.Spec.Type, ltype)
 }
 
+// OnlyNEGStatusChanged returns true if the only annotation change between the 2 services is the NEG status annotation.
+// This will be true if neg annotation was added or removed in the new service.
+func OnlyNEGStatusChanged(oldService, newService *v1.Service) bool {
+	return onlyNEGStatusChanged(oldService, newService) && onlyNEGStatusChanged(newService, oldService)
+}
+
+// onlyNEGStatusChanged returns true if the NEG Status annotation is the only extra annotation present in the new
+// service but not in the old service.
+func onlyNEGStatusChanged(oldService, newService *v1.Service) bool {
+	for key, _ := range newService.ObjectMeta.Annotations {
+		if _, ok := oldService.ObjectMeta.Annotations[key]; !ok {
+			if key != NEGStatusKey {
+				return false
+			}
+		}
+	}
+	return true
+}
+
 // ApplicationProtocols returns a map of port (name or number) to the protocol
 // on the port.
 func (svc *Service) ApplicationProtocols() (map[string]AppProtocol, error) {

pkg/backends/backends.go

@@ -235,6 +235,9 @@ func (b *Backends) List(key *meta.Key, version meta.Version) ([]*composite.Backe
 func (b *Backends) EnsureL4BackendService(name, hcLink, protocol, sessionAffinity, scheme string, nm types.NamespacedName, version meta.Version) (*composite.BackendService, error) {
 	klog.V(2).Infof("EnsureL4BackendService(%v, %v, %v): checking existing backend service", name, scheme, protocol)
 	key, err := composite.CreateKey(b.cloud, name, meta.Regional)
+	if err != nil {
+		return nil, err
+	}
 	bs, err := composite.GetBackendService(b.cloud, key, meta.VersionGA)
 	if err != nil && !utils.IsNotFoundError(err) {
 		return nil, err

pkg/controller/service_controller.go

@@ -98,10 +98,15 @@ func NewL4Controller(ctx *context.ControllerContext, stopCh chan struct{}) *L4Co
 			svcKey := utils.ServiceKeyFunc(curSvc.Namespace, curSvc.Name)
 			if common.HasGivenFinalizer(curSvc.ObjectMeta, common.ILBFinalizerV2) {
 				if reflect.DeepEqual(old, cur) {
-					// TODO when will this be true?
+					// this will happen when informers run a resync on all the existing services even when the object is
+					// not modified.
 					klog.V(3).Infof("Periodic enqueueing of %v", svcKey)
 				} else {
-					klog.V(3).Infof("Service %v changed, enqueuing", svcKey)
+					oldSvc := old.(*v1.Service)
+					svcKey := utils.ServiceKeyFunc(curSvc.Namespace, curSvc.Name)
+					if l4c.needsUpdate(curSvc, oldSvc) {
+						klog.V(3).Infof("Service %v changed, enqueuing", svcKey)
+					}
 				}
 				l4c.svcQueue.Enqueue(curSvc)
 			}
@@ -260,3 +265,120 @@ func (l4c *L4Controller) updateServiceStatus(svc *v1.Service, newStatus *v1.Load
 	}
 	return nil
 }
+
+// needsUpdate checks if load balancer needs to be updated due to change in attributes.
+func (l4c *L4Controller) needsUpdate(oldService *v1.Service, newService *v1.Service) bool {
+	oldSvcWantsILB, _ := annotations.WantsL4ILB(oldService)
+	newSvcWantsILB, _ := annotations.WantsL4ILB(newService)
+	recorder := l4c.ctx.Recorder(oldService.Namespace)
+	if oldSvcWantsILB != newSvcWantsILB {
+		recorder.Eventf(newService, v1.EventTypeNormal, "Type", "%v -> %v", oldService.Spec.Type, newService.Spec.Type)
+		return true
+	}
+
+	if !reflect.DeepEqual(oldService.Spec.LoadBalancerSourceRanges, newService.Spec.LoadBalancerSourceRanges) {
+		recorder.Eventf(newService, v1.EventTypeNormal, "LoadBalancerSourceRanges", "%v -> %v",
+			oldService.Spec.LoadBalancerSourceRanges, newService.Spec.LoadBalancerSourceRanges)
+		return true
+	}
+
+	if !portsEqualForLB(oldService, newService) || oldService.Spec.SessionAffinity != newService.Spec.SessionAffinity {
+		return true
+	}
+
+	if !reflect.DeepEqual(oldService.Spec.SessionAffinityConfig, newService.Spec.SessionAffinityConfig) {
+		return true
+	}
+	if oldService.Spec.LoadBalancerIP != newService.Spec.LoadBalancerIP {
+		recorder.Eventf(newService, v1.EventTypeNormal, "LoadbalancerIP", "%v -> %v",
+			oldService.Spec.LoadBalancerIP, newService.Spec.LoadBalancerIP)
+		return true
+	}
+	if len(oldService.Spec.ExternalIPs) != len(newService.Spec.ExternalIPs) {
+		recorder.Eventf(newService, v1.EventTypeNormal, "ExternalIP", "Count: %v -> %v",
+			len(oldService.Spec.ExternalIPs), len(newService.Spec.ExternalIPs))
+		return true
+	}
+	for i := range oldService.Spec.ExternalIPs {
+		if oldService.Spec.ExternalIPs[i] != newService.Spec.ExternalIPs[i] {
+			recorder.Eventf(newService, v1.EventTypeNormal, "ExternalIP", "Added: %v",
+				newService.Spec.ExternalIPs[i])
+			return true
+		}
+	}
+	if !reflect.DeepEqual(oldService.Annotations, newService.Annotations) {
+		// Ignore update if only neg annotation changed, this is added by the neg controller.
+		if !annotations.OnlyNEGStatusChanged(oldService, newService) {
+			return true
+		}
+	}
+	if oldService.UID != newService.UID {
+		recorder.Eventf(newService, v1.EventTypeNormal, "UID", "%v -> %v",
+			oldService.UID, newService.UID)
+		return true
+	}
+	if oldService.Spec.ExternalTrafficPolicy != newService.Spec.ExternalTrafficPolicy {
+		recorder.Eventf(newService, v1.EventTypeNormal, "ExternalTrafficPolicy", "%v -> %v",
+			oldService.Spec.ExternalTrafficPolicy, newService.Spec.ExternalTrafficPolicy)
+		return true
+	}
+	if oldService.Spec.HealthCheckNodePort != newService.Spec.HealthCheckNodePort {
+		recorder.Eventf(newService, v1.EventTypeNormal, "HealthCheckNodePort", "%v -> %v",
+			oldService.Spec.HealthCheckNodePort, newService.Spec.HealthCheckNodePort)
+		return true
+	}
+	return false
+}
+
+func getPortsForLB(service *v1.Service) []*v1.ServicePort {
+	ports := []*v1.ServicePort{}
+	for i := range service.Spec.Ports {
+		sp := &service.Spec.Ports[i]
+		ports = append(ports, sp)
+	}
+	return ports
+}
+
+func portsEqualForLB(x, y *v1.Service) bool {
+	xPorts := getPortsForLB(x)
+	yPorts := getPortsForLB(y)
+	return portSlicesEqualForLB(xPorts, yPorts)
+}
+
+func portSlicesEqualForLB(x, y []*v1.ServicePort) bool {
+	if len(x) != len(y) {
+		return false
+	}
+
+	for i := range x {
+		if !portEqualForLB(x[i], y[i]) {
+			return false
+		}
+	}
+	return true
+}
+
+func portEqualForLB(x, y *v1.ServicePort) bool {
+	// TODO: Should we check name?  (In theory, an LB could expose it)
+	if x.Name != y.Name {
+		return false
+	}
+
+	if x.Protocol != y.Protocol {
+		return false
+	}
+
+	if x.Port != y.Port {
+		return false
+	}
+
+	if x.NodePort != y.NodePort {
+		return false
+	}
+
+	if x.TargetPort != y.TargetPort {
+		return false
+	}
+
+	return true
+}

pkg/controller/translator/translator.go

@@ -276,10 +276,10 @@ func (t *Translator) GetZoneForNode(name string) (string, error) {
 // ListZones returns a list of zones this Kubernetes cluster spans.
 func (t *Translator) ListZones() ([]string, error) {
 	nodeLister := t.ctx.NodeInformer.GetIndexer()
-	return t.ListZonesWithLister(listers.NewNodeLister(nodeLister))
+	return t.listZonesWithLister(listers.NewNodeLister(nodeLister))
 }
 
-func (t *Translator) ListZonesWithLister(lister listers.NodeLister) ([]string, error) {
+func (t *Translator) listZonesWithLister(lister listers.NodeLister) ([]string, error) {
 	zones := sets.String{}
 	readyNodes, err := lister.ListWithPredicate(utils.GetNodeConditionPredicate())
 	if err != nil {