Refactor translator.ToURLMap to not re-fetch backend services

kubernetes · Apr 12, 2018 · a3c7bc7 · a3c7bc7
1 parent 9a0fc14
commit a3c7bc7
Show file tree

Hide file tree

Showing 11 changed files with 118 additions and 101 deletions.
diff --git a/cmd/glbc/app/namer.go b/cmd/glbc/app/namer.go
@@ -41,7 +41,7 @@ const (
 )
 
 // NewNamer returns a new naming policy given the state of the cluster.
-func NewNamer(kubeClient kubernetes.Interface, clusterName string, fwName string) (*utils.Namer, error) {
+func NewNamer(kubeClient kubernetes.Interface, clusterName, fwName string) (*utils.Namer, error) {
 	name, err := getClusterUID(kubeClient, clusterName)
 	if err != nil {
 		return nil, err

diff --git a/pkg/controller/cluster_manager.go b/pkg/controller/cluster_manager.go
@@ -179,15 +179,6 @@ func (c *ClusterManager) GC(lbNames []string, nodePorts []backends.ServicePort)
 	return nil
 }
 
-func (c *ClusterManager) BackendServiceForPort(port int64) (*compute.BackendService, error) {
-	bs, err := c.backendPool.Get(port, false)
-	return bs.Ga, err
-}
-
-func (c *ClusterManager) DefaultBackendNodePort() *backends.ServicePort {
-	return &c.defaultBackendNodePort
-}
-
 // NewClusterManager creates a cluster manager for shared resources.
 // - namer: is the namer used to tag cluster wide shared resources.
 // - defaultBackendNodePort: is the node port of glbc's default backend. This is

diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go
@@ -167,7 +167,7 @@ func NewLoadBalancerController(kubeClient kubernetes.Interface, stopCh chan stru
 	if ctx.EndpointInformer != nil {
 		endpointIndexer = ctx.EndpointInformer.GetIndexer()
 	}
-	lbc.Translator = translator.New(lbc.ctx, lbc.CloudClusterManager,
+	lbc.Translator = translator.New(lbc.ctx, lbc.CloudClusterManager.ClusterNamer,
 		ctx.ServiceInformer.GetIndexer(),
 		ctx.NodeInformer.GetIndexer(),
 		ctx.PodInformer.GetIndexer(),

diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go
@@ -166,7 +166,8 @@ func (p *nodePortManager) toNodePortSvcNames(inputMap map[string]utils.FakeIngre
 	for host, rules := range inputMap {
 		ruleMap := utils.FakeIngressRuleValueMap{}
 		for path, svc := range rules {
-			ruleMap[path] = p.namer.Backend(int64(p.portMap[svc]))
+			beName := p.namer.Backend(int64(p.portMap[svc]))
+			ruleMap[path] = utils.BackendServiceRelativeResourcePath(beName)
 		}
 		expectedMap[host] = ruleMap
 	}
@@ -320,7 +321,7 @@ func TestLbFaultyUpdate(t *testing.T) {
 	// make sure the controller corrects it.
 	l7.UpdateUrlMap(utils.GCEURLMap{
 		"foo.example.com": {
-			"/foo1": &compute.BackendService{SelfLink: "foo2svc"},
+			"/foo1": "foo2svc",
 		},
 	})
 

diff --git a/pkg/controller/translator/translator.go b/pkg/controller/translator/translator.go
@@ -23,8 +23,6 @@ import (
 
 	"github.com/golang/glog"
 
-	compute "google.golang.org/api/compute/v1"
-
 	api_v1 "k8s.io/api/core/v1"
 	extensions "k8s.io/api/extensions/v1beta1"
 	"k8s.io/apimachinery/pkg/api/meta"
@@ -44,21 +42,15 @@ import (
 	"k8s.io/ingress-gce/pkg/utils"
 )
 
-// BackendInfo is an interface to return information about the backends.
-type BackendInfo interface {
-	BackendServiceForPort(port int64) (*compute.BackendService, error)
-	DefaultBackendNodePort() *backends.ServicePort
-}
-
 type recorderSource interface {
 	Recorder(ns string) record.EventRecorder
 }
 
 // New returns a new ControllerContext.
-func New(recorders recorderSource, bi BackendInfo, svcLister cache.Indexer, nodeLister cache.Indexer, podLister cache.Indexer, endpointLister cache.Indexer, negEnabled bool) *GCE {
+func New(recorders recorderSource, namer *utils.Namer, svcLister cache.Indexer, nodeLister cache.Indexer, podLister cache.Indexer, endpointLister cache.Indexer, negEnabled bool) *GCE {
 	return &GCE{
 		recorders,
-		bi,
+		namer,
 		svcLister,
 		nodeLister,
 		podLister,
@@ -71,7 +63,7 @@ func New(recorders recorderSource, bi BackendInfo, svcLister cache.Indexer, node
 type GCE struct {
 	recorders recorderSource
 
-	bi             BackendInfo
+	namer          *utils.Namer
 	svcLister      cache.Indexer
 	nodeLister     cache.Indexer
 	podLister      cache.Indexer
@@ -87,9 +79,9 @@ func (t *GCE) ToURLMap(ing *extensions.Ingress) (utils.GCEURLMap, error) {
 			glog.Errorf("Ignoring non http Ingress rule")
 			continue
 		}
-		pathToBackend := map[string]*compute.BackendService{}
+		pathToBackend := map[string]string{}
 		for _, p := range rule.HTTP.Paths {
-			backend, err := t.toGCEBackend(&p.Backend, ing.Namespace)
+			backendName, err := t.toGCEBackendName(&p.Backend, ing.Namespace)
 			if err != nil {
 				// If a service doesn't have a nodeport we can still forward traffic
 				// to all other services under the assumption that the user will
@@ -111,7 +103,7 @@ func (t *GCE) ToURLMap(ing *extensions.Ingress) (utils.GCEURLMap, error) {
 			if path == "" {
 				path = loadbalancers.DefaultPath
 			}
-			pathToBackend[path] = backend
+			pathToBackend[path] = backendName
 		}
 		// If multiple hostless rule sets are specified, last one wins
 		host := rule.Host
@@ -120,41 +112,39 @@ func (t *GCE) ToURLMap(ing *extensions.Ingress) (utils.GCEURLMap, error) {
 		}
 		hostPathBackend[host] = pathToBackend
 	}
-	var defaultBackend *compute.BackendService
+	var defaultBackendName string
 	if ing.Spec.Backend != nil {
 		var err error
-		defaultBackend, err = t.toGCEBackend(ing.Spec.Backend, ing.Namespace)
+		defaultBackendName, err = t.toGCEBackendName(ing.Spec.Backend, ing.Namespace)
 		if err != nil {
 			msg := fmt.Sprintf("%v", err)
 			if _, ok := err.(errors.ErrNodePortNotFound); ok {
 				msg = fmt.Sprintf("couldn't find nodeport for %v/%v", ing.Namespace, ing.Spec.Backend.ServiceName)
 			}
 			msg = fmt.Sprintf("failed to identify user specified default backend, %v, using system default", msg)
 			t.recorders.Recorder(ing.Namespace).Eventf(ing, api_v1.EventTypeWarning, "Service", msg)
-		} else if defaultBackend != nil {
-			msg := fmt.Sprintf("default backend set to %v:%v", ing.Spec.Backend.ServiceName, defaultBackend.Port)
+		} else if defaultBackendName != "" {
+			port, _ := t.namer.BackendPort(defaultBackendName)
+			msg := fmt.Sprintf("default backend set to %v:%v", ing.Spec.Backend.ServiceName, port)
 			t.recorders.Recorder(ing.Namespace).Eventf(ing, api_v1.EventTypeNormal, "Service", msg)
 		}
 	} else {
 		t.recorders.Recorder(ing.Namespace).Eventf(ing, api_v1.EventTypeNormal, "Service", "no user specified default backend, using system default")
 	}
-	hostPathBackend.PutDefaultBackend(defaultBackend)
+	hostPathBackend.PutDefaultBackendName(defaultBackendName)
 	return hostPathBackend, nil
 }
 
-func (t *GCE) toGCEBackend(be *extensions.IngressBackend, ns string) (*compute.BackendService, error) {
+func (t *GCE) toGCEBackendName(be *extensions.IngressBackend, ns string) (string, error) {
 	if be == nil {
-		return nil, nil
+		return "", nil
 	}
 	port, err := t.getServiceNodePort(*be, ns)
 	if err != nil {
-		return nil, err
-	}
-	backend, err := t.bi.BackendServiceForPort(port.NodePort)
-	if err != nil {
-		return nil, fmt.Errorf("no GCE backend exists for port %v, kube backend %+v", port, be)
+		return "", err
 	}
-	return backend, nil
+	backendName := t.namer.Backend(port.NodePort)
+	return backendName, nil
 }
 
 // getServiceNodePort looks in the svc store for a matching service:port,

diff --git a/pkg/controller/translator/translator_test.go b/pkg/controller/translator/translator_test.go
@@ -22,7 +22,6 @@ import (
 	"time"
 
 	"github.com/golang/glog"
-	compute "google.golang.org/api/compute/v1"
 
 	apiv1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -36,23 +35,13 @@ import (
 	"k8s.io/ingress-gce/pkg/annotations"
 	"k8s.io/ingress-gce/pkg/backends"
 	"k8s.io/ingress-gce/pkg/context"
+	"k8s.io/ingress-gce/pkg/utils"
 )
 
 var (
 	firstPodCreationTime = time.Date(2006, 01, 02, 15, 04, 05, 0, time.UTC)
 )
 
-type fakeBackendInfo struct {
-}
-
-func (bi *fakeBackendInfo) BackendServiceForPort(port int64) (*compute.BackendService, error) {
-	panic(fmt.Errorf("should not be used"))
-}
-
-func (bi *fakeBackendInfo) DefaultBackendNodePort() *backends.ServicePort {
-	return &backends.ServicePort{NodePort: 30000, Protocol: annotations.ProtocolHTTP}
-}
-
 func gceForTest(negEnabled bool) *GCE {
 	client := fake.NewSimpleClientset()
 	broadcaster := record.NewBroadcaster()
@@ -61,10 +50,12 @@ func gceForTest(negEnabled bool) *GCE {
 		Interface: client.Core().Events(""),
 	})
 
+	namer := utils.NewNamer("uid1", "fw1")
+
 	ctx := context.NewControllerContext(client, apiv1.NamespaceAll, 1*time.Second, negEnabled)
 	gce := &GCE{
 		recorders:  ctx,
-		bi:         &fakeBackendInfo{},
+		namer:      namer,
 		svcLister:  ctx.ServiceInformer.GetIndexer(),
 		nodeLister: ctx.NodeInformer.GetIndexer(),
 		podLister:  ctx.PodInformer.GetIndexer(),

diff --git a/pkg/loadbalancers/l7.go b/pkg/loadbalancers/l7.go
@@ -711,9 +711,9 @@ func (l *L7) UpdateUrlMap(ingressRules utils.GCEURLMap) error {
 	// backend, it applies to all host rules as well as to the urlmap itself.
 	// If it doesn't the urlmap might have a stale default, so replace it with
 	// glbc's default backend.
-	defaultBackend := ingressRules.GetDefaultBackend()
-	if defaultBackend != nil {
-		l.um.DefaultService = defaultBackend.SelfLink
+	defaultBackendName := ingressRules.GetDefaultBackendName()
+	if defaultBackendName != "" {
+		l.um.DefaultService = utils.BackendServiceRelativeResourcePath(defaultBackendName)
 	} else {
 		l.um.DefaultService = l.glbcDefaultBackend.SelfLink
 	}
@@ -726,7 +726,7 @@ func (l *L7) UpdateUrlMap(ingressRules utils.GCEURLMap) error {
 	l.um.HostRules = []*compute.HostRule{}
 	l.um.PathMatchers = []*compute.PathMatcher{}
 
-	for hostname, urlToBackend := range ingressRules {
+	for hostname, urlToBackendName := range ingressRules {
 		// Create a host rule
 		// Create a path matcher
 		// Add all given endpoint:backends to pathRules in path matcher
@@ -744,9 +744,10 @@ func (l *L7) UpdateUrlMap(ingressRules utils.GCEURLMap) error {
 
 		// Longest prefix wins. For equal rules, first hit wins, i.e the second
 		// /foo rule when the first is deleted.
-		for expr, be := range urlToBackend {
+		for expr, beName := range urlToBackendName {
+			beLink := utils.BackendServiceRelativeResourcePath(beName)
 			pathMatcher.PathRules = append(
-				pathMatcher.PathRules, &compute.PathRule{Paths: []string{expr}, Service: be.SelfLink})
+				pathMatcher.PathRules, &compute.PathRule{Paths: []string{expr}, Service: beLink})
 		}
 		l.um.PathMatchers = append(l.um.PathMatchers, pathMatcher)
 	}
@@ -771,7 +772,7 @@ func (l *L7) UpdateUrlMap(ingressRules utils.GCEURLMap) error {
 }
 
 func mapsEqual(a, b *compute.UrlMap) bool {
-	if a.DefaultService != b.DefaultService {
+	if utils.BackendServiceComparablePath(a.DefaultService) != utils.BackendServiceComparablePath(b.DefaultService) {
 		return false
 	}
 	if len(a.HostRules) != len(b.HostRules) {
@@ -801,7 +802,7 @@ func mapsEqual(a, b *compute.UrlMap) bool {
 	for i := range a.PathMatchers {
 		a := a.PathMatchers[i]
 		b := b.PathMatchers[i]
-		if a.DefaultService != b.DefaultService {
+		if utils.BackendServiceComparablePath(a.DefaultService) != utils.BackendServiceComparablePath(b.DefaultService) {
 			return false
 		}
 		if a.Description != b.Description {
@@ -824,7 +825,9 @@ func mapsEqual(a, b *compute.UrlMap) bool {
 					return false
 				}
 			}
-			if a.Service != b.Service {
+			// Trim down the url's for a.Service and b.Service to a comparable structure
+			// We do this because we update the UrlMap with relative links (not full) to backends.
+			if utils.BackendServiceComparablePath(a.Service) != utils.BackendServiceComparablePath(b.Service) {
 				return false
 			}
 		}

diff --git a/pkg/loadbalancers/loadbalancers_test.go b/pkg/loadbalancers/loadbalancers_test.go
@@ -615,19 +615,19 @@ func TestCreateBothLoadBalancers(t *testing.T) {
 func TestUpdateUrlMap(t *testing.T) {
 	um1 := utils.GCEURLMap{
 		"bar.example.com": {
-			"/bar2": &compute.BackendService{SelfLink: "bar2svc"},
+			"/bar2": "bar2svc",
 		},
 	}
 	um2 := utils.GCEURLMap{
 		"foo.example.com": {
-			"/foo1": &compute.BackendService{SelfLink: "foo1svc"},
-			"/foo2": &compute.BackendService{SelfLink: "foo2svc"},
+			"/foo1": "foo1svc",
+			"/foo2": "foo2svc",
 		},
 		"bar.example.com": {
-			"/bar1": &compute.BackendService{SelfLink: "bar1svc"},
+			"/bar1": "bar1svc",
 		},
 	}
-	um2.PutDefaultBackend(&compute.BackendService{SelfLink: "default"})
+	um2.PutDefaultBackendName("default")
 
 	namer := utils.NewNamer("uid1", "fw1")
 	lbInfo := &L7RuntimeInfo{Name: namer.LoadBalancer("test"), AllowHTTP: true}
@@ -647,14 +647,14 @@ func TestUpdateUrlMap(t *testing.T) {
 	// The final map doesn't contain /bar2
 	expectedMap := map[string]utils.FakeIngressRuleValueMap{
 		utils.DefaultBackendKey: {
-			utils.DefaultBackendKey: "default",
+			utils.DefaultBackendKey: utils.BackendServiceRelativeResourcePath("default"),
 		},
 		"foo.example.com": {
-			"/foo1": "foo1svc",
-			"/foo2": "foo2svc",
+			"/foo1": utils.BackendServiceRelativeResourcePath("foo1svc"),
+			"/foo2": utils.BackendServiceRelativeResourcePath("foo2svc"),
 		},
 		"bar.example.com": {
-			"/bar1": "bar1svc",
+			"/bar1": utils.BackendServiceRelativeResourcePath("bar1svc"),
 		},
 	}
 	if err := f.CheckURLMap(l7, expectedMap); err != nil {
@@ -665,24 +665,24 @@ func TestUpdateUrlMap(t *testing.T) {
 func TestUpdateUrlMapNoChanges(t *testing.T) {
 	um1 := utils.GCEURLMap{
 		"foo.example.com": {
-			"/foo1": &compute.BackendService{SelfLink: "foo1svc"},
-			"/foo2": &compute.BackendService{SelfLink: "foo2svc"},
+			"/foo1": "foo1svc",
+			"/foo2": "foo2svc",
 		},
 		"bar.example.com": {
-			"/bar1": &compute.BackendService{SelfLink: "bar1svc"},
+			"/bar1": "bar1svc",
 		},
 	}
-	um1.PutDefaultBackend(&compute.BackendService{SelfLink: "default"})
+	um1.PutDefaultBackendName("default")
 	um2 := utils.GCEURLMap{
 		"foo.example.com": {
-			"/foo1": &compute.BackendService{SelfLink: "foo1svc"},
-			"/foo2": &compute.BackendService{SelfLink: "foo2svc"},
+			"/foo1": "foo1svc",
+			"/foo2": "foo2svc",
 		},
 		"bar.example.com": {
-			"/bar1": &compute.BackendService{SelfLink: "bar1svc"},
+			"/bar1": "bar1svc",
 		},
 	}
-	um2.PutDefaultBackend(&compute.BackendService{SelfLink: "default"})
+	um2.PutDefaultBackendName("default")
 
 	namer := utils.NewNamer("uid1", "fw1")
 	lbInfo := &L7RuntimeInfo{Name: namer.LoadBalancer("test"), AllowHTTP: true}
@@ -797,7 +797,6 @@ func TestInvalidClusterNameChange(t *testing.T) {
 			t.Fatalf("Expected %q got %q", testCase.expected, got)
 		}
 	}
-
 }
 
 func createCert(key string, contents string, name string) *TLSCerts {