diff --git a/internal/handlers/service.go b/internal/handlers/service.go index 1e43e9890e..152e6af519 100644 --- a/internal/handlers/service.go +++ b/internal/handlers/service.go @@ -2,6 +2,7 @@ package handlers import ( "reflect" + "sort" "github.com/golang/glog" "github.com/nginxinc/kubernetes-ingress/internal/controller" @@ -47,14 +48,52 @@ func CreateServiceHandlers(lbc *controller.LoadBalancerController) cache.Resourc }, UpdateFunc: func(old, cur interface{}) { if !reflect.DeepEqual(old, cur) { - svc := cur.(*api_v1.Service) - if lbc.IsExternalServiceForStatus(svc) { - lbc.AddSyncQueue(svc) + curSvc := cur.(*api_v1.Service) + if lbc.IsExternalServiceForStatus(curSvc) { + lbc.AddSyncQueue(curSvc) return } - glog.V(3).Infof("Service %v changed, syncing", svc.Name) - lbc.EnqueueIngressForService(svc) + oldSvc := old.(*api_v1.Service) + if hasServicePortChanges(oldSvc.Spec.Ports, curSvc.Spec.Ports) { + glog.V(3).Infof("Service %v changed, syncing", curSvc.Name) + lbc.EnqueueIngressForService(curSvc) + } } }, } } + +type portSort []api_v1.ServicePort + +func (a portSort) Len() int { + return len(a) +} + +func (a portSort) Swap(i, j int) { + a[i], a[j] = a[j], a[i] +} + +func (a portSort) Less(i, j int) bool { + if a[i].Name == a[j].Name { + return a[i].Port < a[j].Port + } + return a[i].Name < a[j].Name +} + +// hasServicePortChanges only compares ServicePort.Name and .Port. +func hasServicePortChanges(oldServicePorts []api_v1.ServicePort, curServicePorts []api_v1.ServicePort) bool { + if len(oldServicePorts) != len(curServicePorts) { + return true + } + + sort.Sort(portSort(oldServicePorts)) + sort.Sort(portSort(curServicePorts)) + + for i := range oldServicePorts { + if oldServicePorts[i].Port != curServicePorts[i].Port || + oldServicePorts[i].Name != curServicePorts[i].Name { + return true + } + } + return false +} diff --git a/internal/handlers/service_test.go b/internal/handlers/service_test.go new file mode 100644 index 0000000000..63444c3c46 --- /dev/null +++ b/internal/handlers/service_test.go @@ -0,0 +1,156 @@ +package handlers + +import ( + "testing" + + "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/util/intstr" +) + +func TestHasServicePortChanges(t *testing.T) { + cases := []struct { + a []v1.ServicePort + b []v1.ServicePort + result bool + reason string + }{ + { + []v1.ServicePort{}, + []v1.ServicePort{}, + false, + "Empty should report no changes", + }, + { + []v1.ServicePort{{ + Port: 80, + }}, + []v1.ServicePort{{ + Port: 8080, + }}, + true, + "Different Ports", + }, + { + []v1.ServicePort{{ + Port: 80, + }}, + []v1.ServicePort{{ + Port: 80, + }}, + false, + "Same Ports", + }, + { + []v1.ServicePort{{ + Name: "asdf", + Port: 80, + }}, + []v1.ServicePort{{ + Name: "asdf", + Port: 80, + }}, + false, + "Same Port and Name", + }, + { + []v1.ServicePort{{ + Name: "foo", + Port: 80, + }}, + []v1.ServicePort{{ + Name: "bar", + Port: 80, + }}, + true, + "Different Name same Port", + }, + { + []v1.ServicePort{{ + Name: "foo", + Port: 8080, + }}, + []v1.ServicePort{{ + Name: "bar", + Port: 80, + }}, + true, + "Different Name different Port", + }, + { + []v1.ServicePort{{ + Name: "foo", + }}, + []v1.ServicePort{{ + Name: "fooo", + }}, + true, + "Very similar Name", + }, + { + []v1.ServicePort{{ + Name: "asdf", + Port: 80, + TargetPort: intstr.IntOrString{ + IntVal: 80, + }, + }}, + []v1.ServicePort{{ + Name: "asdf", + Port: 80, + TargetPort: intstr.IntOrString{ + IntVal: 8080, + }, + }}, + false, + "TargetPort should be ignored", + }, + { + []v1.ServicePort{{ + Name: "foo", + }, { + Name: "bar", + }}, + []v1.ServicePort{{ + Name: "foo", + }, { + Name: "bar", + }}, + false, + "Multiple same names", + }, + { + []v1.ServicePort{{ + Name: "foo", + }, { + Name: "bar", + }}, + []v1.ServicePort{{ + Name: "foo", + }, { + Name: "bars", + }}, + true, + "Multiple different names", + }, + { + []v1.ServicePort{{ + Name: "foo", + }, { + Port: 80, + }}, + []v1.ServicePort{{ + Port: 80, + }, { + Name: "foo", + }}, + false, + "Some names some ports", + }, + } + + for _, c := range cases { + if c.result != hasServicePortChanges(c.a, c.b) { + t.Errorf("hasServicePortChanges returned %v, but expected %v for %q case", c.result, !c.result, c.reason) + } + } +}