diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index 12739507b8..530f941e8d 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -316,11 +316,13 @@ func (lbc *LoadBalancerController) ensureIngress(ing *extensions.Ingress, nodeNa } ingSvcPorts := urlMap.AllServicePorts() + igs, err := lbc.ensureInstanceGroupsAndPorts(ingSvcPorts, nodeNames) if err != nil { return err } + // TODO: Remove this after deprecation if utils.IsGCEMultiClusterIngress(ing) { // Add instance group names as annotation on the ingress and return. if ing.Annotations == nil { @@ -480,12 +482,13 @@ func (lbc *LoadBalancerController) ToSvcPorts(ings *extensions.IngressList) []ut return knownPorts } +// ensureInstanceGroupsAndPorts creates instance group if necessary +// if all service ports have NEG enabled, then instance group creation will be skipped and return nil. func (lbc *LoadBalancerController) ensureInstanceGroupsAndPorts(svcPorts []utils.ServicePort, nodeNames []string) ([]*compute.InstanceGroup, error) { - ports := []int64{} - for _, p := range uniq(svcPorts) { - if !p.NEGEnabled { - ports = append(ports, p.NodePort) - } + ports := nodePorts(svcPorts) + if len(ports) == 0 { + glog.V(2).Infof("Skip ensuring instance groups as all backend(s) have NEG enabled.") + return nil, nil } // Create instance groups and set named ports. @@ -507,7 +510,7 @@ func (lbc *LoadBalancerController) ensureInstanceGroupsAndPorts(svcPorts []utils // - nodePorts are the ports for which we want BackendServies. BackendServices // for ports not in this list are deleted. // This method ignores googleapi 404 errors (StatusNotFound). -func (lbc *LoadBalancerController) gc(lbNames []string, nodePorts []utils.ServicePort) error { +func (lbc *LoadBalancerController) gc(lbNames []string, svcPorts []utils.ServicePort) error { // On GC: // * Loadbalancers need to get deleted before backends. // * Backends are refcounted in a shared pool. @@ -519,7 +522,7 @@ func (lbc *LoadBalancerController) gc(lbNames []string, nodePorts []utils.Servic // happen when an Ingress is updated, if we don't GC after the update // we'll leak the backend. lbErr := lbc.l7Pool.GC(lbNames) - beErr := lbc.backendSyncer.GC(nodePorts) + beErr := lbc.backendSyncer.GC(svcPorts) if lbErr != nil { return lbErr } @@ -528,7 +531,7 @@ func (lbc *LoadBalancerController) gc(lbNames []string, nodePorts []utils.Servic } // TODO(ingress#120): Move this to the backend pool so it mirrors creation - if len(lbNames) == 0 { + if len(lbNames) == 0 || len(nodePorts(svcPorts)) == 0 { igName := lbc.ctx.ClusterNamer.InstanceGroup() glog.Infof("Deleting instance group %v", igName) if err := lbc.instancePool.DeleteInstanceGroup(igName); err != err { diff --git a/pkg/controller/utils.go b/pkg/controller/utils.go index 35ec41b2d2..83e3d01870 100644 --- a/pkg/controller/utils.go +++ b/pkg/controller/utils.go @@ -86,3 +86,15 @@ func convert(ings []*extensions.Ingress) (retVal []interface{}) { } return } + +// nodePorts returns the list of uniq NodePort from the input ServicePorts. +// Only NonNEG service backend need NodePort. +func nodePorts(svcPorts []utils.ServicePort) []int64 { + ports := []int64{} + for _, p := range uniq(svcPorts) { + if !p.NEGEnabled { + ports = append(ports, p.NodePort) + } + } + return ports +} diff --git a/pkg/controller/utils_test.go b/pkg/controller/utils_test.go index d745fd95f1..de1646e60d 100644 --- a/pkg/controller/utils_test.go +++ b/pkg/controller/utils_test.go @@ -20,7 +20,7 @@ import ( "testing" "time" - compute "google.golang.org/api/compute/v1" + "google.golang.org/api/compute/v1" api_v1 "k8s.io/api/core/v1" meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -30,6 +30,7 @@ import ( "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/ingress-gce/pkg/annotations" "k8s.io/ingress-gce/pkg/utils" + "reflect" ) // Pods created in loops start from this time, for routines that @@ -216,68 +217,68 @@ func TestUniq(t *testing.T) { { "Two service ports", []utils.ServicePort{ - testServicePort("ns", "name", "80", 80, 30080), - testServicePort("ns", "name", "443", 443, 30443), + testServicePort("ns", "name", "80", 80, 30080, true), + testServicePort("ns", "name", "443", 443, 30443, true), }, []utils.ServicePort{ - testServicePort("ns", "name", "80", 80, 30080), - testServicePort("ns", "name", "443", 443, 30443), + testServicePort("ns", "name", "80", 80, 30080, true), + testServicePort("ns", "name", "443", 443, 30443, true), }, }, { "Two service ports with different names", []utils.ServicePort{ - testServicePort("ns", "name1", "80", 80, 30080), - testServicePort("ns", "name2", "80", 80, 30880), + testServicePort("ns", "name1", "80", 80, 30080, true), + testServicePort("ns", "name2", "80", 80, 30880, true), }, []utils.ServicePort{ - testServicePort("ns", "name1", "80", 80, 30080), - testServicePort("ns", "name2", "80", 80, 30880), + testServicePort("ns", "name1", "80", 80, 30080, true), + testServicePort("ns", "name2", "80", 80, 30880, true), }, }, { "Two duplicate service ports", []utils.ServicePort{ - testServicePort("ns", "name", "80", 80, 30080), - testServicePort("ns", "name", "80", 80, 30080), + testServicePort("ns", "name", "80", 80, 30080, true), + testServicePort("ns", "name", "80", 80, 30080, true), }, []utils.ServicePort{ - testServicePort("ns", "name", "80", 80, 30080), + testServicePort("ns", "name", "80", 80, 30080, true), }, }, { "Two services without nodeports", []utils.ServicePort{ - testServicePort("ns", "name", "80", 80, 0), - testServicePort("ns", "name", "443", 443, 0), + testServicePort("ns", "name", "80", 80, 0, true), + testServicePort("ns", "name", "443", 443, 0, true), }, []utils.ServicePort{ - testServicePort("ns", "name", "80", 80, 0), - testServicePort("ns", "name", "443", 443, 0), + testServicePort("ns", "name", "80", 80, 0, true), + testServicePort("ns", "name", "443", 443, 0, true), }, }, { "2 out of 3 are duplicates", []utils.ServicePort{ - testServicePort("ns", "name", "80", 80, 0), - testServicePort("ns", "name", "443", 443, 0), - testServicePort("ns", "name", "443", 443, 0), + testServicePort("ns", "name", "80", 80, 0, true), + testServicePort("ns", "name", "443", 443, 0, true), + testServicePort("ns", "name", "443", 443, 0, true), }, []utils.ServicePort{ - testServicePort("ns", "name", "80", 80, 0), - testServicePort("ns", "name", "443", 443, 0), + testServicePort("ns", "name", "80", 80, 0, true), + testServicePort("ns", "name", "443", 443, 0, true), }, }, { "mix of named port and port number references", []utils.ServicePort{ - testServicePort("ns", "name", "http", 80, 0), - testServicePort("ns", "name", "https", 443, 0), - testServicePort("ns", "name", "443", 443, 0), + testServicePort("ns", "name", "http", 80, 0, true), + testServicePort("ns", "name", "https", 443, 0, true), + testServicePort("ns", "name", "443", 443, 0, true), }, []utils.ServicePort{ - testServicePort("ns", "name", "http", 80, 0), - testServicePort("ns", "name", "443", 443, 0), + testServicePort("ns", "name", "http", 80, 0, true), + testServicePort("ns", "name", "443", 443, 0, true), }, }, } @@ -302,6 +303,61 @@ func TestUniq(t *testing.T) { } +func TestGetNodePortsUsedByIngress(t *testing.T) { + testCases := []struct { + desc string + svcPorts []utils.ServicePort + expectPorts []int64 + }{ + { + "empty input", + []utils.ServicePort{}, + []int64{}, + }, + { + " all NEG enabled", + []utils.ServicePort{ + testServicePort("ns", "name", "80", 80, 30080, true), + testServicePort("ns", "name", "443", 443, 30443, true), + }, + []int64{}, + }, + { + " all nonNEG enabled", + []utils.ServicePort{ + testServicePort("ns", "name", "80", 80, 30080, false), + testServicePort("ns", "name", "443", 443, 30443, false), + }, + []int64{30080, 30443}, + }, + { + " mixed SvcPorts", + []utils.ServicePort{ + testServicePort("ns", "name", "80", 80, 30080, false), + testServicePort("ns", "name", "443", 443, 30443, true), + }, + []int64{30080}, + }, + { + " mixed SvcPorts with duplicates", + []utils.ServicePort{ + testServicePort("ns", "name", "80", 80, 30080, false), + testServicePort("ns", "name", "80", 80, 30080, false), + testServicePort("ns", "name", "443", 443, 30443, false), + }, + []int64{30080, 30443}, + }, + } + + for _, tc := range testCases { + res := nodePorts(tc.svcPorts) + if !reflect.DeepEqual(res, tc.expectPorts) { + t.Errorf("For case %q, expect %v, but got %v", tc.desc, tc.expectPorts, res) + } + } + +} + func testNode() *api_v1.Node { return &api_v1.Node{ ObjectMeta: meta_v1.ObjectMeta{ @@ -327,7 +383,7 @@ func testNode() *api_v1.Node { } } -func testServicePort(namespace, name, port string, servicePort, nodePort int) utils.ServicePort { +func testServicePort(namespace, name, port string, servicePort, nodePort int, enableNEG bool) utils.ServicePort { return utils.ServicePort{ ID: utils.ServicePortID{ Service: types.NamespacedName{ @@ -336,7 +392,8 @@ func testServicePort(namespace, name, port string, servicePort, nodePort int) ut }, Port: intstr.FromString(port), }, - Port: int32(servicePort), - NodePort: int64(nodePort), + Port: int32(servicePort), + NodePort: int64(nodePort), + NEGEnabled: enableNEG, } }