Merge pull request #77630 from feiskyer/cluster-name-tag

Fix public IPs issues when multiple clusters are sharing the same resource group
kubernetes · May 9, 2019 · e9af72c · e9af72c
2 parents ed239ce + 1ea5a69
commit e9af72c
Show file tree

Hide file tree

Showing 3 changed files with 126 additions and 11 deletions.
diff --git a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer.go b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer.go
@@ -83,6 +83,11 @@ const (
 	// ServiceAnnotationLoadBalancerMixedProtocols is the annotation used on the service
 	// to create both TCP and UDP protocols when creating load balancer rules.
 	ServiceAnnotationLoadBalancerMixedProtocols = "service.beta.kubernetes.io/azure-load-balancer-mixed-protocols"
+
+	// serviceTagKey is the service key applied for public IP tags.
+	serviceTagKey = "service"
+	// clusterNameKey is the cluster name key applied for public IP tags.
+	clusterNameKey = "kubernetes-cluster-name"
 )
 
 var (
@@ -465,7 +470,7 @@ func (az *Cloud) findServiceIPAddress(ctx context.Context, clusterName string, s
 	return lbStatus.Ingress[0].IP, nil
 }
 
-func (az *Cloud) ensurePublicIPExists(service *v1.Service, pipName string, domainNameLabel string) (*network.PublicIPAddress, error) {
+func (az *Cloud) ensurePublicIPExists(service *v1.Service, pipName string, domainNameLabel, clusterName string) (*network.PublicIPAddress, error) {
 	pipResourceGroup := az.getPublicIPAddressResourceGroup(service)
 	pip, existsPip, err := az.getPublicIPAddress(pipResourceGroup, pipName)
 	if err != nil {
@@ -486,7 +491,10 @@ func (az *Cloud) ensurePublicIPExists(service *v1.Service, pipName string, domai
 			DomainNameLabel: &domainNameLabel,
 		}
 	}
-	pip.Tags = map[string]*string{"service": &serviceName}
+	pip.Tags = map[string]*string{
+		serviceTagKey:  &serviceName,
+		clusterNameKey: &clusterName,
+	}
 	if az.useStandardLoadBalancer() {
 		pip.Sku = &network.PublicIPAddressSku{
 			Name: network.PublicIPAddressSkuNameStandard,
@@ -711,7 +719,7 @@ func (az *Cloud) reconcileLoadBalancer(clusterName string, service *v1.Service,
 					return nil, err
 				}
 				domainNameLabel := getPublicIPDomainNameLabel(service)
-				pip, err := az.ensurePublicIPExists(service, pipName, domainNameLabel)
+				pip, err := az.ensurePublicIPExists(service, pipName, domainNameLabel, clusterName)
 				if err != nil {
 					return nil, err
 				}
@@ -1344,9 +1352,7 @@ func (az *Cloud) reconcilePublicIP(clusterName string, service *v1.Service, lb *
 
 	for i := range pips {
 		pip := pips[i]
-		if pip.Tags != nil &&
-			(pip.Tags)["service"] != nil &&
-			*(pip.Tags)["service"] == serviceName {
+		if serviceOwnsPublicIP(&pip, clusterName, serviceName) {
 			// We need to process for pips belong to this service
 			pipName := *pip.Name
 			if wantLb && !isInternal && pipName == desiredPipName {
@@ -1369,7 +1375,7 @@ func (az *Cloud) reconcilePublicIP(clusterName string, service *v1.Service, lb *
 		// Confirm desired public ip resource exists
 		var pip *network.PublicIPAddress
 		domainNameLabel := getPublicIPDomainNameLabel(service)
-		if pip, err = az.ensurePublicIPExists(service, desiredPipName, domainNameLabel); err != nil {
+		if pip, err = az.ensurePublicIPExists(service, desiredPipName, domainNameLabel, clusterName); err != nil {
 			return nil, err
 		}
 		return pip, nil
@@ -1612,3 +1618,24 @@ func getServiceTags(service *v1.Service) ([]string, error) {
 
 	return nil, nil
 }
+
+func serviceOwnsPublicIP(pip *network.PublicIPAddress, clusterName, serviceName string) bool {
+	if pip != nil && pip.Tags != nil {
+		serviceTag := pip.Tags[serviceTagKey]
+		clusterTag := pip.Tags[clusterNameKey]
+
+		if serviceTag != nil && *serviceTag == serviceName {
+			// Backward compatible for clusters upgraded from old releases.
+			// In such case, only "service" tag is set.
+			if clusterTag == nil {
+				return true
+			}
+
+			// If cluster name tag is set, then return true if it matches.
+			if *clusterTag == clusterName {
+				return true
+			}
+		}
+	}
+	return false
+}
diff --git a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer_test.go b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer_test.go
@@ -368,3 +368,82 @@ func TestEnsureLoadBalancerDeleted(t *testing.T) {
 		assert.Equal(t, len(result), 0, "TestCase[%d]: %s", i, c.desc)
 	}
 }
+
+func TestServiceOwnsPublicIP(t *testing.T) {
+	tests := []struct {
+		desc        string
+		pip         *network.PublicIPAddress
+		clusterName string
+		serviceName string
+		expected    bool
+	}{
+		{
+			desc:        "false should be returned when pip is nil",
+			clusterName: "kubernetes",
+			serviceName: "nginx",
+			expected:    false,
+		},
+		{
+			desc: "false should be returned when service name tag doesn't match",
+			pip: &network.PublicIPAddress{
+				Tags: map[string]*string{
+					serviceTagKey: to.StringPtr("nginx"),
+				},
+			},
+			serviceName: "web",
+			expected:    false,
+		},
+		{
+			desc: "true should be returned when service name tag matches and cluster name tag is not set",
+			pip: &network.PublicIPAddress{
+				Tags: map[string]*string{
+					serviceTagKey: to.StringPtr("nginx"),
+				},
+			},
+			clusterName: "kubernetes",
+			serviceName: "nginx",
+			expected:    true,
+		},
+		{
+			desc: "false should be returned when cluster name doesn't match",
+			pip: &network.PublicIPAddress{
+				Tags: map[string]*string{
+					serviceTagKey:  to.StringPtr("nginx"),
+					clusterNameKey: to.StringPtr("kubernetes"),
+				},
+			},
+			clusterName: "k8s",
+			serviceName: "nginx",
+			expected:    false,
+		},
+		{
+			desc: "false should be returned when cluster name matches while service name doesn't match",
+			pip: &network.PublicIPAddress{
+				Tags: map[string]*string{
+					serviceTagKey:  to.StringPtr("web"),
+					clusterNameKey: to.StringPtr("kubernetes"),
+				},
+			},
+			clusterName: "kubernetes",
+			serviceName: "nginx",
+			expected:    false,
+		},
+		{
+			desc: "true should be returned when both service name tag and cluster name match",
+			pip: &network.PublicIPAddress{
+				Tags: map[string]*string{
+					serviceTagKey:  to.StringPtr("nginx"),
+					clusterNameKey: to.StringPtr("kubernetes"),
+				},
+			},
+			clusterName: "kubernetes",
+			serviceName: "nginx",
+			expected:    true,
+		},
+	}
+
+	for i, c := range tests {
+		owns := serviceOwnsPublicIP(c.pip, c.clusterName, c.serviceName)
+		assert.Equal(t, owns, c.expected, "TestCase[%d]: %s", i, c.desc)
+	}
+}
diff --git a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_test.go b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_test.go
@@ -1367,14 +1367,23 @@ func validatePublicIP(t *testing.T, publicIP *network.PublicIPAddress, service *
 		t.Errorf("Expected publicIP resource exists, when it is not an internal service")
 	}
 
-	if publicIP.Tags == nil || publicIP.Tags["service"] == nil {
-		t.Errorf("Expected publicIP resource has tags[service]")
+	if publicIP.Tags == nil || publicIP.Tags[serviceTagKey] == nil {
+		t.Errorf("Expected publicIP resource has tags[%s]", serviceTagKey)
 	}
 
 	serviceName := getServiceName(service)
-	if serviceName != *(publicIP.Tags["service"]) {
-		t.Errorf("Expected publicIP resource has matching tags[service]")
+	if serviceName != *(publicIP.Tags[serviceTagKey]) {
+		t.Errorf("Expected publicIP resource has matching tags[%s]", serviceTagKey)
 	}
+
+	if publicIP.Tags[clusterNameKey] == nil {
+		t.Errorf("Expected publicIP resource has tags[%s]", clusterNameKey)
+	}
+
+	if *(publicIP.Tags[clusterNameKey]) != testClusterName {
+		t.Errorf("Expected publicIP resource has matching tags[%s]", clusterNameKey)
+	}
+
 	// We cannot use service.Spec.LoadBalancerIP to compare with
 	// Public IP's IPAddress
 	// Because service properties are updated outside of cloudprovider code