Merge pull request #1790 from panslava/l4namer-getsuffixname

Refactor L4 Namer, add getSuffixedName, getTrimmedNamespacedName function

pkg/l4lb/l4controller_test.go

@@ -106,7 +106,7 @@ func deleteILBService(l4c *L4Controller, svc *api_v1.Service) {
 
 func addNEG(l4c *L4Controller, svc *api_v1.Service) {
 	// Also create a fake NEG for this service since the sync code will try to link the backend service to NEG
-	negName, _ := l4c.namer.L4Backend(svc.Namespace, svc.Name)
+	negName := l4c.namer.L4Backend(svc.Namespace, svc.Name)
 	neg := &composite.NetworkEndpointGroup{Name: negName}
 	key := meta.ZonalKey(negName, testGCEZone)
 	composite.CreateNetworkEndpointGroup(l4c.ctx.Cloud, key, neg)

pkg/l4lb/l4netlbcontroller_test.go

@@ -201,7 +201,7 @@ func updateRegionBackendServiceWithLockHook(ctx context.Context, key *meta.Key,
 }
 
 func getBackend(l4netController *L4NetLBController, svc *v1.Service) (string, *composite.BackendService, error) {
-	backendServiceName, _ := l4netController.namer.L4Backend(svc.Namespace, svc.Name)
+	backendServiceName := l4netController.namer.L4Backend(svc.Namespace, svc.Name)
 	key := meta.RegionalKey(backendServiceName, l4netController.ctx.Cloud.Region())
 	backendServiceLink := cloud.SelfLink(meta.VersionGA, l4netController.ctx.Cloud.ProjectID(), "backendServices", key)
 	bs, err := composite.GetBackendService(l4netController.ctx.Cloud, key, meta.VersionGA)
@@ -729,7 +729,7 @@ func TestProcessServiceUpdate(t *testing.T) {
 				if len(svc.Spec.Ports) == 0 {
 					return fmt.Errorf("No Ports in service")
 				}
-				name, _ := (l4netController.namer.(namer.BackendNamer)).L4Backend(svc.Namespace, svc.Name)
+				name := (l4netController.namer.(namer.BackendNamer)).L4Backend(svc.Namespace, svc.Name)
 				fw, err := l4netController.ctx.Cloud.GetFirewall(name)
 				if err != nil {
 					return fmt.Errorf("Failed to fetch firewall service: %v", err)

pkg/loadbalancers/l4.go

@@ -17,7 +17,6 @@ limitations under the License.
 package loadbalancers
 
 import (
-	"fmt"
 	"strings"
 	"time"
 
@@ -110,11 +109,7 @@ func (l4 *L4) EnsureInternalLoadBalancerDeleted(svc *corev1.Service) *L4ILBSyncR
 	klog.V(2).Infof("EnsureInternalLoadBalancerDeleted(%s): attempting delete of load balancer resources", l4.NamespacedName.String())
 	result := &L4ILBSyncResult{SyncType: SyncTypeDelete, StartTime: time.Now()}
 	// All resources use the L4Backend Name, except forwarding rule.
-	name, ok := l4.namer.L4Backend(svc.Namespace, svc.Name)
-	if !ok {
-		result.Error = fmt.Errorf("Namer does not support L4 Backends")
-		return result
-	}
+	name := l4.namer.L4Backend(svc.Namespace, svc.Name)
 	frName := l4.GetFRName()
 	key, err := l4.CreateKey(frName)
 	if err != nil {
@@ -206,11 +201,7 @@ func (l4 *L4) EnsureInternalLoadBalancer(nodeNames []string, svc *corev1.Service
 
 	l4.Service = svc
 	// All resources use the L4Backend name, except forwarding rule.
-	name, ok := l4.namer.L4Backend(l4.Service.Namespace, l4.Service.Name)
-	if !ok {
-		result.Error = fmt.Errorf("Namer does not support L4 VMIPNEGs")
-		return result
-	}
+	name := l4.namer.L4Backend(l4.Service.Namespace, l4.Service.Name)
 	options := getILBOptions(l4.Service)
 
 	// create healthcheck

pkg/loadbalancers/l4_test.go

@@ -77,7 +77,7 @@ func TestEnsureInternalBackendServiceUpdates(t *testing.T) {
 	l := NewL4Handler(l4ilbParams)
 	l.l4HealthChecks = healthchecks.FakeL4(fakeGCE, &test.FakeRecorderSource{})
 
-	bsName, _ := l.namer.L4Backend(l.Service.Namespace, l.Service.Name)
+	bsName := l.namer.L4Backend(l.Service.Namespace, l.Service.Name)
 	_, err := l.backendPool.EnsureL4BackendService(bsName, "", "TCP", string(svc.Spec.SessionAffinity), string(cloud.SchemeInternal), l.NamespacedName, meta.VersionGA)
 	if err != nil {
 		t.Errorf("Failed to ensure backend service  %s - err %v", bsName, err)
@@ -147,7 +147,7 @@ func TestEnsureInternalLoadBalancer(t *testing.T) {
 	}
 	assertInternalLbResources(t, svc, l, nodeNames, result.Annotations)
 
-	backendServiceName, _ := l.namer.L4Backend(l.Service.Namespace, l.Service.Name)
+	backendServiceName := l.namer.L4Backend(l.Service.Namespace, l.Service.Name)
 	key := meta.RegionalKey(backendServiceName, l.cloud.Region())
 	bs, err := composite.GetBackendService(l.cloud, key, meta.VersionGA)
 	if err != nil {
@@ -241,7 +241,7 @@ func TestEnsureInternalLoadBalancerWithExistingResources(t *testing.T) {
 		t.Errorf("Unexpected error when adding nodes %v", err)
 	}
 
-	lbName, _ := l.namer.L4Backend(svc.Namespace, svc.Name)
+	lbName := l.namer.L4Backend(svc.Namespace, svc.Name)
 
 	// Create the expected resources necessary for an Internal Load Balancer
 	sharedHC := !servicehelper.RequestsOnlyLocalTraffic(svc)
@@ -291,7 +291,7 @@ func TestEnsureInternalLoadBalancerClearPreviousResources(t *testing.T) {
 		t.Errorf("Unexpected error when adding nodes %v", err)
 	}
 
-	lbName, _ := l.namer.L4Backend(svc.Namespace, svc.Name)
+	lbName := l.namer.L4Backend(svc.Namespace, svc.Name)
 	frName := l.GetFRName()
 	key, err := composite.CreateKey(l.cloud, frName, meta.Regional)
 	if err != nil {
@@ -417,7 +417,7 @@ func TestUpdateResourceLinks(t *testing.T) {
 		t.Errorf("Unexpected error when adding nodes %v", err)
 	}
 
-	lbName, _ := l.namer.L4Backend(svc.Namespace, svc.Name)
+	lbName := l.namer.L4Backend(svc.Namespace, svc.Name)
 	key, err := composite.CreateKey(l.cloud, lbName, meta.Regional)
 	if err != nil {
 		t.Errorf("Unexpected error when creating key - %v", err)
@@ -500,7 +500,7 @@ func TestEnsureInternalLoadBalancerHealthCheckConfigurable(t *testing.T) {
 	if err != nil {
 		t.Errorf("Unexpected error when adding nodes %v", err)
 	}
-	lbName, _ := l.namer.L4Backend(svc.Namespace, svc.Name)
+	lbName := l.namer.L4Backend(svc.Namespace, svc.Name)
 	key, err := composite.CreateKey(l.cloud, lbName, meta.Regional)
 	if err != nil {
 		t.Errorf("Unexpected error when creating key - %v", err)
@@ -756,7 +756,7 @@ func TestEnsureInternalLoadBalancerWithSpecialHealthCheck(t *testing.T) {
 	}
 	assertInternalLbResources(t, svc, l, nodeNames, result.Annotations)
 
-	lbName, _ := l.namer.L4Backend(svc.Namespace, svc.Name)
+	lbName := l.namer.L4Backend(svc.Namespace, svc.Name)
 	key, err := composite.CreateKey(l.cloud, lbName, meta.Global)
 	if err != nil {
 		t.Errorf("Unexpected error when creating key - %v", err)
@@ -1131,7 +1131,7 @@ func TestEnsureInternalFirewallPortRanges(t *testing.T) {
 	l := NewL4Handler(l4ilbParams)
 	l.l4HealthChecks = healthchecks.FakeL4(fakeGCE, &test.FakeRecorderSource{})
 
-	fwName, _ := l.namer.L4Backend(l.Service.Namespace, l.Service.Name)
+	fwName := l.namer.L4Backend(l.Service.Namespace, l.Service.Name)
 	tc := struct {
 		Input  []int
 		Result []string
@@ -1377,7 +1377,7 @@ func TestEnsureInternalLoadBalancerAllPorts(t *testing.T) {
 func assertInternalLbResources(t *testing.T, apiService *v1.Service, l *L4, nodeNames []string, resourceAnnotations map[string]string) {
 	// Check that Firewalls are created for the LoadBalancer and the HealthCheck
 	sharedHC := !servicehelper.RequestsOnlyLocalTraffic(apiService)
-	resourceName, _ := l.namer.L4Backend(l.Service.Namespace, l.Service.Name)
+	resourceName := l.namer.L4Backend(l.Service.Namespace, l.Service.Name)
 	resourceDesc, err := utils.MakeL4LBServiceDescription(utils.ServiceKeyFunc(apiService.Namespace, apiService.Name), "", meta.VersionGA, false, utils.ILB)
 
 	if err != nil {
@@ -1505,7 +1505,7 @@ func assertInternalLbResources(t *testing.T, apiService *v1.Service, l *L4, node
 
 func assertInternalLbResourcesDeleted(t *testing.T, apiService *v1.Service, firewallsDeleted bool, l *L4) {
 	frName := l.GetFRName()
-	resourceName, _ := l.namer.L4Backend(l.Service.Namespace, l.Service.Name)
+	resourceName := l.namer.L4Backend(l.Service.Namespace, l.Service.Name)
 	hcNameShared := l.namer.L4HealthCheck(l.Service.Namespace, l.Service.Name, true)
 	hcFwNameShared := l.namer.L4HealthCheckFirewall(l.Service.Namespace, l.Service.Name, true)
 	hcNameNonShared := l.namer.L4HealthCheck(l.Service.Namespace, l.Service.Name, false)

pkg/loadbalancers/l4netlb_test.go

@@ -408,7 +408,7 @@ func TestEnsureNetLBFirewallDestinations(t *testing.T) {
 	}
 
 	flags.F.EnablePinhole = true
-	fwName, _ := l4netlb.namer.L4Backend(l4netlb.Service.Namespace, l4netlb.Service.Name)
+	fwName := l4netlb.namer.L4Backend(l4netlb.Service.Namespace, l4netlb.Service.Name)
 
 	fwrParams := firewalls.FirewallParams{
 		Name:              fwName,

pkg/neg/types/types.go

@@ -178,7 +178,7 @@ func NewPortInfoMapForVMIPNEG(namespace, name string, namer namer.L4ResourcesNam
 		if local {
 			mode = L4LocalMode
 		}
-		negName, _ := namer.L4Backend(namespace, name)
+		negName := namer.L4Backend(namespace, name)
 		ret[PortInfoMapKey{svcPortTuple.Port, ""}] = PortInfo{
 			PortTuple:        svcPortTuple,
 			NegName:          negName,

pkg/utils/namer/interfaces.go

@@ -60,7 +60,7 @@ type BackendNamer interface {
 	// L4Backend returns the name for L4 LB backend resources, based on the service namespace and name.
 	// It supports ILB with subsetting enabled (VM_IP_NEGs) and NetLB with RBS enabled.
 	// The second output parameter indicates if the namer is supported.
-	L4Backend(namespace, name string) (string, bool)
+	L4Backend(namespace, name string) string
 	// InstanceGroup constructs the name for an Instance Group.
 	InstanceGroup() string
 	// NamedPort returns the name for a named port.

pkg/utils/namer/l4_namer.go

@@ -51,12 +51,13 @@ func NewL4Namer(kubeSystemUID string, namer *Namer) *L4Namer {
 //	k8s2-{uid}-{ns}-{name}-{suffix}
 //
 // Output name is at most 63 characters.
-func (namer *L4Namer) L4Backend(namespace, name string) (string, bool) {
-	truncFields := TrimFieldsEvenly(maximumL4CombinedLength, namespace, name)
-	truncNamespace := truncFields[0]
-	truncName := truncFields[1]
-	return strings.Join([]string{namer.v2Prefix, namer.v2ClusterUID, truncNamespace, truncName, namer.suffix(namespace, name)}, "-"), true
-
+func (namer *L4Namer) L4Backend(namespace, name string) string {
+	return strings.Join([]string{
+		namer.v2Prefix,
+		namer.v2ClusterUID,
+		getTrimmedNamespacedName(namespace, name, maximumL4CombinedLength),
+		namer.getClusterSuffix(namespace, name),
+	}, "-")
 }
 
 // L4ForwardingRule returns the name of the L4 forwarding rule name based on the service namespace, name and protocol.
@@ -68,25 +69,28 @@ func (namer *L4Namer) L4Backend(namespace, name string) (string, bool) {
 func (namer *L4Namer) L4ForwardingRule(namespace, name, protocol string) string {
 	// add 1 for hyphen
 	protoLen := len(protocol) + 1
-	truncFields := TrimFieldsEvenly(maximumL4CombinedLength-protoLen, namespace, name)
-	truncNamespace := truncFields[0]
-	truncName := truncFields[1]
-	return strings.Join([]string{namer.v2Prefix, protocol, namer.v2ClusterUID, truncNamespace, truncName, namer.suffix(namespace, name)}, "-")
+	return strings.Join([]string{
+		namer.v2Prefix,
+		protocol,
+		namer.v2ClusterUID,
+		getTrimmedNamespacedName(namespace, name, maximumL4CombinedLength-protoLen),
+		namer.getClusterSuffix(namespace, name),
+	}, "-")
 }
 
 // L4HealthCheck returns the name of the L4 LB Healthcheck
 func (namer *L4Namer) L4HealthCheck(namespace, name string, shared bool) string {
 	if shared {
 		return strings.Join([]string{namer.v2Prefix, namer.v2ClusterUID, sharedHcSuffix}, "-")
 	}
-	l4Name, _ := namer.L4Backend(namespace, name)
+	l4Name := namer.L4Backend(namespace, name)
 	return l4Name
 }
 
 // L4HealthCheckFirewall returns the name of the L4 LB Healthcheck Firewall
 func (namer *L4Namer) L4HealthCheckFirewall(namespace, name string, shared bool) string {
 	if !shared {
-		l4Name, _ := namer.L4Backend(namespace, name)
+		l4Name := namer.L4Backend(namespace, name)
 		return namer.hcFirewallName(l4Name)
 	}
 	return strings.Join([]string{namer.v2Prefix, namer.v2ClusterUID, sharedFirewallHcSuffix}, "-")
@@ -97,19 +101,32 @@ func (namer *L4Namer) IsNEG(name string) bool {
 	return strings.HasPrefix(name, namer.v2Prefix+"-"+namer.v2ClusterUID)
 }
 
-// suffix returns hash string of length 8 of a concatenated string generated from
+// getClusterSuffix returns hash string of length 8 of a concatenated string generated from
 // kube-system uid, namespace and name. These fields in combination define an l4 load-balancer uniquely.
-func (n *L4Namer) suffix(namespace, name string) string {
+func (n *L4Namer) getClusterSuffix(namespace, name string) string {
 	lbString := strings.Join([]string{n.v2ClusterUID, namespace, name}, ";")
 	return common.ContentHash(lbString, 8)
 }
 
 // hcFirewallName generates the firewall name for the given healthcheck.
-// It ensures that the name is atmost 63 chars long.
+// It ensures that the name is at most 63 chars long.
 func (n *L4Namer) hcFirewallName(hcName string) string {
-	maxHcNameLen := maxResourceNameLength - len(firewallHcSuffix)
-	if len(hcName) > maxHcNameLen {
-		hcName = hcName[:maxHcNameLen]
+	return getSuffixedName(hcName, firewallHcSuffix)
+}
+
+func getSuffixedName(name string, suffix string) string {
+	trimmedName := ensureSpaceForSuffix(name, suffix)
+	return trimmedName + suffix
+}
+
+func ensureSpaceForSuffix(name string, suffix string) string {
+	maxRealNameLen := maxResourceNameLength - len(suffix)
+	if len(name) > maxRealNameLen {
+		name = name[:maxRealNameLen]
 	}
-	return hcName + firewallHcSuffix
+	return name
+}
+
+func getTrimmedNamespacedName(namespace, name string, maxLength int) string {
+	return strings.Join(TrimFieldsEvenly(maxLength, namespace, name), "-")
 }

pkg/utils/namer/l4_namer_test.go

@@ -69,12 +69,9 @@ func TestL4Namer(t *testing.T) {
 	newNamer := NewL4Namer(kubeSystemUID, nil)
 	for _, tc := range testCases {
 		frName := newNamer.L4ForwardingRule(tc.namespace, tc.name, strings.ToLower(tc.proto))
-		negName, ok := newNamer.L4Backend(tc.namespace, tc.name)
+		negName := newNamer.L4Backend(tc.namespace, tc.name)
 		hcName := newNamer.L4HealthCheck(tc.namespace, tc.name, tc.sharedHC)
 		hcFwName := newNamer.L4HealthCheckFirewall(tc.namespace, tc.name, tc.sharedHC)
-		if !ok {
-			t.Errorf("Namer does not support VMIPNEG")
-		}
 		if len(frName) > maxResourceNameLength || len(negName) > maxResourceNameLength || len(hcName) > maxResourceNameLength || len(hcFwName) > maxResourceNameLength {
 			t.Errorf("%s: got len(frName) == %v, len(negName) == %v, len(hcName) == %v, len(hcFwName) == %v want <= 63", tc.desc, len(frName), len(negName), len(hcName), len(hcFwName))
 		}

pkg/utils/namer/namer.go

@@ -479,8 +479,8 @@ func (n *Namer) IsNEG(name string) bool {
 }
 
 // L4Backend is only supported by L4Namer.
-func (namer *Namer) L4Backend(namespace, name string) (string, bool) {
-	return "", false
+func (namer *Namer) L4Backend(namespace, name string) string {
+	return ""
 }
 
 func (n *Namer) negPrefix() string {

pkg/utils/serviceport.go

@@ -74,8 +74,7 @@ func (sp *ServicePort) BackendName() string {
 		return sp.BackendNamer.NEG(sp.ID.Service.Namespace, sp.ID.Service.Name, sp.Port)
 	} else if sp.VMIPNEGEnabled || sp.L4RBSEnabled {
 		// Use L4 Backend name for both Internal and External LoadBalancers
-		backendName, _ := sp.BackendNamer.L4Backend(sp.ID.Service.Namespace, sp.ID.Service.Name)
-		return backendName
+		return sp.BackendNamer.L4Backend(sp.ID.Service.Namespace, sp.ID.Service.Name)
 	}
 	return sp.BackendNamer.IGBackend(sp.NodePort)
 }