Merge pull request #3415 from lzhecheng/dualstack-pip-prerequisite

Prerequisite for dualstack public IP support

pkg/consts/consts.go

@@ -201,21 +201,20 @@ var (
 		false: "service.beta.kubernetes.io/azure-load-balancer-ipv4",
 		true:  "service.beta.kubernetes.io/azure-load-balancer-ipv6",
 	}
+	// ServiceAnnotationPIPName specifies the pip that will be applied to load balancer
 	ServiceAnnotationPIPNameDualStack = map[bool]string{
-		false: "service.beta.kubernetes.io/azure-pip-name-ipv4",
+		false: "service.beta.kubernetes.io/azure-pip-name",
 		true:  "service.beta.kubernetes.io/azure-pip-name-ipv6",
 	}
+	// ServiceAnnotationPIPPrefixID specifies the pip prefix that will be applied to the load balancer.
 	ServiceAnnotationPIPPrefixIDDualStack = map[bool]string{
-		false: "service.beta.kubernetes.io/azure-pip-prefix-id-ipv4",
+		false: "service.beta.kubernetes.io/azure-pip-prefix-id",
 		true:  "service.beta.kubernetes.io/azure-pip-prefix-id-ipv6",
 	}
 )
 
 // load balancer
 const (
-	// TODO: After dual-stack is supported, all references should be updated and this variable is not needed.
-	DualstackSupported = false
-
 	// PreConfiguredBackendPoolLoadBalancerTypesInternal means that the `internal` load balancers are pre-configured
 	PreConfiguredBackendPoolLoadBalancerTypesInternal = "internal"
 	// PreConfiguredBackendPoolLoadBalancerTypesExternal means that the `external` load balancers are pre-configured
@@ -269,12 +268,6 @@ const (
 	// to specify the resource group of load balancer objects that are not in the same resource group as the cluster.
 	ServiceAnnotationLoadBalancerResourceGroup = "service.beta.kubernetes.io/azure-load-balancer-resource-group"
 
-	// ServiceAnnotationPIPName specifies the pip that will be applied to load balancer
-	ServiceAnnotationPIPName = "service.beta.kubernetes.io/azure-pip-name"
-
-	// ServiceAnnotationPIPPrefixID specifies the pip prefix that will be applied to the load balancer.
-	ServiceAnnotationPIPPrefixID = "service.beta.kubernetes.io/azure-pip-prefix-id"
-
 	// ServiceAnnotationIPTagsForPublicIP specifies the iptags used when dynamically creating a public ip
 	ServiceAnnotationIPTagsForPublicIP = "service.beta.kubernetes.io/azure-pip-ip-tags"
 

pkg/provider/azure_loadbalancer.go

@@ -859,11 +859,11 @@ func (az *Cloud) getServiceLoadBalancerStatus(service *v1.Service, lb *network.L
 
 // pips: a non-nil pointer to a slice of existing PIPs, if the slice being pointed to is nil, listPIP would be called when needed and the slice would be filled
 func (az *Cloud) determinePublicIPName(clusterName string, service *v1.Service, pips *[]network.PublicIPAddress) (string, bool, error) {
-	if name, found := service.Annotations[consts.ServiceAnnotationPIPName]; found && name != "" {
+	if name, found := service.Annotations[consts.ServiceAnnotationPIPNameDualStack[false]]; found && name != "" {
 		return name, true, nil
 	}
 	isIPv6 := utilnet.IsIPv6String(service.Spec.ClusterIP)
-	if ipPrefix, ok := service.Annotations[consts.ServiceAnnotationPIPPrefixID]; ok && ipPrefix != "" {
+	if ipPrefix, ok := service.Annotations[consts.ServiceAnnotationPIPPrefixIDDualStack[false]]; ok && ipPrefix != "" {
 		return az.getPublicIPName(clusterName, service, isIPv6), false, nil
 	}
 
@@ -1059,7 +1059,7 @@ func (az *Cloud) ensurePublicIPExists(service *v1.Service, pipName string, domai
 			pip.Sku = &network.PublicIPAddressSku{
 				Name: network.PublicIPAddressSkuNameStandard,
 			}
-			if pipPrefixName, ok := service.Annotations[consts.ServiceAnnotationPIPPrefixID]; ok && pipPrefixName != "" {
+			if pipPrefixName, ok := service.Annotations[consts.ServiceAnnotationPIPPrefixIDDualStack[false]]; ok && pipPrefixName != "" {
 				pip.PublicIPPrefix = &network.SubResource{ID: pointer.String(pipPrefixName)}
 			}
 

pkg/provider/azure_loadbalancer_test.go

@@ -3944,7 +3944,7 @@ func TestReconcilePublicIP(t *testing.T) {
 		{
 			desc:        "reconcilePublicIP shall report error if the given PIP name doesn't exist in the resource group",
 			wantLb:      true,
-			annotations: map[string]string{consts.ServiceAnnotationPIPName: "testPIP"},
+			annotations: map[string]string{consts.ServiceAnnotationPIPNameDualStack[false]: "testPIP"},
 			existingPIPs: []network.PublicIPAddress{
 				{
 					Name: pointer.String("pip1"),
@@ -3962,7 +3962,7 @@ func TestReconcilePublicIP(t *testing.T) {
 		{
 			desc:        "reconcilePublicIP shall delete unwanted PIP when given the name of desired PIP",
 			wantLb:      true,
-			annotations: map[string]string{consts.ServiceAnnotationPIPName: "testPIP"},
+			annotations: map[string]string{consts.ServiceAnnotationPIPNameDualStack[false]: "testPIP"},
 			existingPIPs: []network.PublicIPAddress{
 				{
 					Name: pointer.String("pip1"),
@@ -4001,7 +4001,7 @@ func TestReconcilePublicIP(t *testing.T) {
 		{
 			desc:        "reconcilePublicIP shall not delete unwanted PIP when there are other service references",
 			wantLb:      true,
-			annotations: map[string]string{consts.ServiceAnnotationPIPName: "testPIP"},
+			annotations: map[string]string{consts.ServiceAnnotationPIPNameDualStack[false]: "testPIP"},
 			existingPIPs: []network.PublicIPAddress{
 				{
 					Name: pointer.String("pip1"),
@@ -4041,8 +4041,8 @@ func TestReconcilePublicIP(t *testing.T) {
 			desc:   "reconcilePublicIP shall delete unwanted pips and existing pips, when the existing pips IP tags do not match",
 			wantLb: true,
 			annotations: map[string]string{
-				consts.ServiceAnnotationPIPName:           "testPIP",
-				consts.ServiceAnnotationIPTagsForPublicIP: "tag1=tag1value",
+				consts.ServiceAnnotationPIPNameDualStack[false]: "testPIP",
+				consts.ServiceAnnotationIPTagsForPublicIP:       "tag1=tag1value",
 			},
 			existingPIPs: []network.PublicIPAddress{
 				{
@@ -4089,8 +4089,8 @@ func TestReconcilePublicIP(t *testing.T) {
 			desc:   "reconcilePublicIP shall preserve existing pips, when the existing pips IP tags do match",
 			wantLb: true,
 			annotations: map[string]string{
-				consts.ServiceAnnotationPIPName:           "testPIP",
-				consts.ServiceAnnotationIPTagsForPublicIP: "tag1=tag1value",
+				consts.ServiceAnnotationPIPNameDualStack[false]: "testPIP",
+				consts.ServiceAnnotationIPTagsForPublicIP:       "tag1=tag1value",
 			},
 			existingPIPs: []network.PublicIPAddress{
 				{
@@ -4131,7 +4131,7 @@ func TestReconcilePublicIP(t *testing.T) {
 		{
 			desc:        "reconcilePublicIP shall find the PIP by given name and shall not delete the PIP which is not owned by service",
 			wantLb:      true,
-			annotations: map[string]string{consts.ServiceAnnotationPIPName: "testPIP"},
+			annotations: map[string]string{consts.ServiceAnnotationPIPNameDualStack[false]: "testPIP"},
 			existingPIPs: []network.PublicIPAddress{
 				{
 					Name: pointer.String("pip1"),

pkg/provider/azure_standard.go

@@ -56,7 +56,6 @@ var (
 )
 
 const (
-	v4Suffix = "IPv4"
 	v6Suffix = "IPv6"
 )
 
@@ -285,7 +284,8 @@ func (az *Cloud) getLoadBalancerRuleName(service *v1.Service, protocol v1.Protoc
 	ruleName := fmt.Sprintf("%s-%s-%d", prefix, protocol, port)
 	subnet := subnet(service)
 	if subnet == nil {
-		return getResourceByIPFamily(ruleName, isIPv6)
+		// TODO: Use getResourceByIPFamily()
+		return ruleName
 	}
 
 	// Load balancer rule name must be less or equal to 80 characters, so excluding the hyphen two segments cannot exceed 79
@@ -295,7 +295,8 @@ func (az *Cloud) getLoadBalancerRuleName(service *v1.Service, protocol v1.Protoc
 		subnetSegment = subnetSegment[:maxLength-len(ruleName)-1]
 	}
 
-	return getResourceByIPFamily(fmt.Sprintf("%s-%s-%s-%d", prefix, subnetSegment, protocol, port), isIPv6)
+	// TODO: Use getResourceByIPFamily()
+	return fmt.Sprintf("%s-%s-%s-%d", prefix, subnetSegment, protocol, port)
 }
 
 func (az *Cloud) getloadbalancerHAmodeRuleName(service *v1.Service, isIPv6 bool) string {
@@ -310,7 +311,8 @@ func (az *Cloud) getSecurityRuleName(service *v1.Service, port v1.ServicePort, s
 	}
 	rulePrefix := az.getRulePrefix(service)
 	name := fmt.Sprintf("%s-%s-%d-%s", rulePrefix, port.Protocol, port.Port, safePrefix)
-	return getResourceByIPFamily(name, isIPv6)
+	// TODO: Use getResourceByIPFamily
+	return name
 }
 
 // This returns a human-readable version of the Service used to tag some resources.
@@ -326,17 +328,16 @@ func (az *Cloud) getRulePrefix(service *v1.Service) string {
 
 func (az *Cloud) getPublicIPName(clusterName string, service *v1.Service, isIPv6 bool) string {
 	pipName := fmt.Sprintf("%s-%s", clusterName, az.GetLoadBalancerName(context.TODO(), clusterName, service))
-	pipName = getResourceByIPFamily(pipName, isIPv6)
 	if id := getServicePIPPrefixID(service, isIPv6); id != "" {
 		id, err := getLastSegment(id, "/")
-		if err != nil {
-			return pipName
+		if err == nil {
+			pipName = fmt.Sprintf("%s-%s", pipName, id)
 		}
-		pipName = fmt.Sprintf("%s-%s", pipName, id)
 	}
-	return pipName
+	return getResourceByIPFamily(pipName, isIPv6)
 }
 
+// TODO: UT
 func (az *Cloud) serviceOwnsRule(service *v1.Service, rule string) bool {
 	prefix := az.getRulePrefix(service)
 	return strings.HasPrefix(strings.ToUpper(rule), strings.ToUpper(prefix))

pkg/provider/azure_standard_test.go

@@ -2305,3 +2305,85 @@ func TestGetSecurityRuleName(t *testing.T) {
 		})
 	}
 }
+
+func TestGetPublicIPName(t *testing.T) {
+	testcases := []struct {
+		desc            string
+		svc             *v1.Service
+		isIPv6          bool
+		expectedPIPName string
+	}{
+		{
+			desc: "common",
+			svc: &v1.Service{
+				ObjectMeta: meta.ObjectMeta{UID: types.UID("uid")},
+				Spec: v1.ServiceSpec{
+					IPFamilies: []v1.IPFamily{v1.IPv4Protocol},
+				},
+			},
+			isIPv6:          false,
+			expectedPIPName: "azure-auid",
+		},
+		{
+			desc: "Service PIP prefix id",
+			svc: &v1.Service{
+				ObjectMeta: meta.ObjectMeta{
+					UID: types.UID("uid"),
+					Annotations: map[string]string{
+						consts.ServiceAnnotationPIPPrefixIDDualStack[false]: "prefix-id",
+					},
+				},
+				Spec: v1.ServiceSpec{
+					IPFamilies: []v1.IPFamily{v1.IPv4Protocol},
+				},
+			},
+			isIPv6:          false,
+			expectedPIPName: "azure-auid-prefix-id",
+		},
+		{
+			desc: "Service PIP prefix id dualstack IPv4",
+			svc: &v1.Service{
+				ObjectMeta: meta.ObjectMeta{
+					UID: types.UID("uid"),
+					Annotations: map[string]string{
+						consts.ServiceAnnotationPIPPrefixIDDualStack[false]: "prefix-id",
+						consts.ServiceAnnotationPIPPrefixIDDualStack[true]:  "prefix-id-ipv6",
+					},
+				},
+				Spec: v1.ServiceSpec{
+					IPFamilies: []v1.IPFamily{v1.IPv4Protocol, v1.IPv6Protocol},
+				},
+			},
+			isIPv6:          false,
+			expectedPIPName: "azure-auid-prefix-id",
+		},
+		{
+			desc: "Service PIP prefix id dualstack IPv6",
+			svc: &v1.Service{
+				ObjectMeta: meta.ObjectMeta{
+					UID: types.UID("uid"),
+					Annotations: map[string]string{
+						consts.ServiceAnnotationPIPPrefixIDDualStack[false]: "prefix-id",
+						consts.ServiceAnnotationPIPPrefixIDDualStack[true]:  "prefix-id-ipv6",
+					},
+				},
+				Spec: v1.ServiceSpec{
+					IPFamilies: []v1.IPFamily{v1.IPv4Protocol, v1.IPv6Protocol},
+				},
+			},
+			isIPv6:          true,
+			expectedPIPName: "azure-auid-prefix-id-ipv6-IPv6",
+		},
+	}
+
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+	az := GetTestCloud(ctrl)
+	for _, tc := range testcases {
+		t.Run(tc.desc, func(t *testing.T) {
+			name := az.getPublicIPName("azure", tc.svc, tc.isIPv6)
+			assert.Equal(t, tc.expectedPIPName, name)
+		})
+	}
+
+}

pkg/provider/azure_utils.go

@@ -342,6 +342,11 @@ func extractVmssVMName(name string) (string, string, error) {
 	return ssName, instanceID, nil
 }
 
+// isServiceDualStack checks if a Service is dual-stack or not.
+func isServiceDualStack(svc *v1.Service) bool {
+	return len(svc.Spec.IPFamilies) == 2
+}
+
 // getIPFamiliesEnabled checks if IPv4, IPv6 are enabled according to svc.Spec.IPFamilies.
 func getIPFamiliesEnabled(svc *v1.Service) (v4Enabled bool, v6Enabled bool) {
 	for _, ipFamily := range svc.Spec.IPFamilies {
@@ -411,36 +416,30 @@ func getServicePIPName(service *v1.Service, isIPv6 bool) string {
 		return ""
 	}
 
-	if consts.DualstackSupported {
-		if name, ok := service.Annotations[consts.ServiceAnnotationPIPNameDualStack[isIPv6]]; ok && name != "" {
-			return name
-		}
+	if !isServiceDualStack(service) {
+		return service.Annotations[consts.ServiceAnnotationPIPNameDualStack[false]]
 	}
-	return service.Annotations[consts.ServiceAnnotationPIPName]
+
+	return service.Annotations[consts.ServiceAnnotationPIPNameDualStack[isIPv6]]
 }
 
 func getServicePIPPrefixID(service *v1.Service, isIPv6 bool) string {
 	if service == nil {
 		return ""
 	}
 
-	if consts.DualstackSupported {
-		if name, ok := service.Annotations[consts.ServiceAnnotationPIPPrefixIDDualStack[isIPv6]]; ok && name != "" {
-			return name
-		}
+	if !isServiceDualStack(service) {
+		return service.Annotations[consts.ServiceAnnotationPIPPrefixIDDualStack[false]]
 	}
-	return service.Annotations[consts.ServiceAnnotationPIPPrefixID]
+
+	return service.Annotations[consts.ServiceAnnotationPIPPrefixIDDualStack[isIPv6]]
 }
 
 func getResourceByIPFamily(resource string, isIPv6 bool) string {
-	if !consts.DualstackSupported {
-		return resource
-	}
-
 	if isIPv6 {
 		return fmt.Sprintf("%s-%s", resource, v6Suffix)
 	}
-	return fmt.Sprintf("%s-%s", resource, v4Suffix)
+	return resource
 }
 
 // isFIPIPv6 checks if the frontend IP configuration is of IPv6.