Merge pull request #652 from freehan/bug-fix1

Cherrypick #650 into release 1.5 branch

pkg/utils/namer.go

@@ -69,6 +69,12 @@ const (
 	// avoid terminating on an invalid character ('-').
 	nameLenLimit = 62
 
+	// clusterNameEvalThreshold is the minimum required length of the clusterName section
+	// in the suffix of a GCE resource in order to qualify for evaluating if a given name
+	// belong to the current cluster. This is for minimizing chances of cluster name collision due
+	// matching uid prefix.
+	clusterNameEvalThreshold = 4
+
 	// maxNEGDescriptiveLabel is the max length for namespace, name and
 	// port for neg name.  63 - 5 (k8s and naming schema version prefix)
 	// - 8 (truncated cluster id) - 8 (suffix hash) - 4 (hyphen connector) = 38
@@ -226,7 +232,7 @@ func (n *Namer) ParseName(name string) *NameComponents {
 	if resource == urlMapPrefix {
 		// It is possible for the loadbalancer name to have dashes in it - so we
 		// join the remaining name parts.
-		lbName = strings.Join(c[2:len(c)], "-")
+		lbName = strings.Join(c[2:], "-")
 	}
 
 	return &NameComponents{
@@ -248,9 +254,34 @@ func (n *Namer) NameBelongsToCluster(name string) bool {
 	if !strings.HasPrefix(name, n.prefix+"-") {
 		return false
 	}
-	clusterName := n.UID()
+	fullClusterName := n.UID()
 	components := n.ParseName(name)
-	return components.ClusterName == clusterName
+	componentClusterName := components.ClusterName
+
+	// if exact match is found, then return true immediately
+	// otherwise, do best effort matching as follows
+	if componentClusterName == fullClusterName {
+		return true
+	}
+
+	// if the name is longer or equal to 63 charactors and the last character of the resource matches alphaNumericChar,
+	// it is likely that the name was truncated.
+	if len(name) > nameLenLimit && len(componentClusterName) > 0 && componentClusterName[len(componentClusterName)-1:] == alphaNumericChar {
+		componentClusterName = componentClusterName[0 : len(componentClusterName)-1]
+	}
+
+	// If the name is longer or equal to 63 characters and the length of the
+	// cluster name parsed from the resource name is too short, ignore the resource and do
+	// not consider the resource managed by this cluster. This is to prevent cluster A
+	// accidentally GC resources from cluster B due to both clusters share the same prefix
+	// uid.
+	// For example:
+	// Case 1: k8s-fws-test-sandbox-50a6f22a4cd34e91-ingress-1--16a1467191ad30
+	// The cluster name is 16a1467191ad30 which is longer than clusterNameEvalThreshold.
+	// Case 2: k8s-fws-test-sandbox-50a6f22a4cd34e91-ingress-1111111111111--10
+	// The cluster name is 10 which is shorter than clusterNameEvalThreshold.
+	return len(componentClusterName) > clusterNameEvalThreshold && strings.HasPrefix(fullClusterName, componentClusterName)
+
 }
 
 // IGBackend constructs the name for a backend service targeting instance groups.

pkg/utils/namer_test.go

@@ -119,24 +119,35 @@ func TestNamerParseName(t *testing.T) {
 }
 
 func TestNameBelongsToCluster(t *testing.T) {
-	const uid = "uid1"
+	const uid = "0123456789abcdef"
+	// string with 10 characters
+	longKey := "0123456789"
 	secretHash := fmt.Sprintf("%x", sha256.Sum256([]byte("test123")))[:16]
-
 	for _, prefix := range []string{defaultPrefix, "mci"} {
 		namer := NewNamerWithPrefix(prefix, uid, "fw1")
 		lbName := namer.LoadBalancer("key1")
+		// longLBName with 40 characters. Triggers truncation
+		longLBName := namer.LoadBalancer(strings.Repeat(longKey, 4))
 		// Positive cases.
 		for _, tc := range []string{
+			// short names
 			namer.IGBackend(80),
 			namer.InstanceGroup(),
 			namer.TargetProxy(lbName, HTTPProtocol),
 			namer.TargetProxy(lbName, HTTPSProtocol),
 			namer.SSLCertName("default/my-ing", secretHash),
-			namer.SSLCertName("default/my-ing", secretHash),
 			namer.ForwardingRule(lbName, HTTPProtocol),
 			namer.ForwardingRule(lbName, HTTPSProtocol),
 			namer.UrlMap(lbName),
 			namer.NEG("ns", "n", int32(80)),
+			// long names that are truncated
+			namer.TargetProxy(longLBName, HTTPProtocol),
+			namer.TargetProxy(longLBName, HTTPSProtocol),
+			namer.SSLCertName(longLBName, secretHash),
+			namer.ForwardingRule(longLBName, HTTPProtocol),
+			namer.ForwardingRule(longLBName, HTTPSProtocol),
+			namer.UrlMap(longLBName),
+			namer.NEG(strings.Repeat(longKey, 3), strings.Repeat(longKey, 3), int32(88888)),
 		} {
 			if !namer.NameBelongsToCluster(tc) {
 				t.Errorf("namer.NameBelongsToCluster(%q) = false, want true", tc)
@@ -146,7 +157,18 @@ func TestNameBelongsToCluster(t *testing.T) {
 
 	// Negative cases.
 	namer := NewNamer(uid, "fw1")
-	for _, tc := range []string{"", "invalid", "not--the-right-uid"} {
+	// longLBName with 60 characters. Triggers truncation to eliminate cluster name suffix
+	longLBName := namer.LoadBalancer(strings.Repeat(longKey, 6))
+	for _, tc := range []string{
+		"",
+		"invalid",
+		"not--the-right-uid",
+		namer.TargetProxy(longLBName, HTTPProtocol),
+		namer.TargetProxy(longLBName, HTTPSProtocol),
+		namer.ForwardingRule(longLBName, HTTPProtocol),
+		namer.ForwardingRule(longLBName, HTTPSProtocol),
+		namer.UrlMap(longLBName),
+	} {
 		if namer.NameBelongsToCluster(tc) {
 			t.Errorf("namer.NameBelongsToCluster(%q) = true, want false", tc)
 		}