From f3662cc68688d52070d8e2dfbc0f6db2a79804c2 Mon Sep 17 00:00:00 2001 From: isaac Date: Thu, 6 Sep 2018 11:37:20 +0100 Subject: [PATCH 1/2] Custom comparison for ServicePort slice We only want to sync ingresses for service changes if particular properties of the service are changed. We'll ignore others. DeepEqual was causing us to sync ingresses when it was not necessary. --- internal/handlers/service.go | 49 +++++++++- internal/handlers/service_test.go | 156 ++++++++++++++++++++++++++++++ 2 files changed, 200 insertions(+), 5 deletions(-) create mode 100644 internal/handlers/service_test.go 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..81a9a07fbf --- /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{{ + Name: "foo", + }, { + Port: 80, + }}, + false, + "Some names some ports", + }, + } + + for _, c := range cases { + if c.result != hasServicePortChanges(c.a, c.b) { + t.Error(c.reason) + } + } +} From 3a785f807aa8e89b894be71309915f15d635683f Mon Sep 17 00:00:00 2001 From: isaac Date: Fri, 7 Sep 2018 14:57:01 +0100 Subject: [PATCH 2/2] Improve service_test error messages --- internal/handlers/service_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/handlers/service_test.go b/internal/handlers/service_test.go index 81a9a07fbf..63444c3c46 100644 --- a/internal/handlers/service_test.go +++ b/internal/handlers/service_test.go @@ -139,9 +139,9 @@ func TestHasServicePortChanges(t *testing.T) { Port: 80, }}, []v1.ServicePort{{ - Name: "foo", - }, { Port: 80, + }, { + Name: "foo", }}, false, "Some names some ports", @@ -150,7 +150,7 @@ func TestHasServicePortChanges(t *testing.T) { for _, c := range cases { if c.result != hasServicePortChanges(c.a, c.b) { - t.Error(c.reason) + t.Errorf("hasServicePortChanges returned %v, but expected %v for %q case", c.result, !c.result, c.reason) } } }