Merge pull request #3853 from k8s-infra-cherrypick-robot/cherry-pick-…

…3823-to-release-1.27

[release-1.27] [DualStack] IPv6 PIP uses suffix only when DualStack

pkg/provider/azure_loadbalancer.go

@@ -869,7 +869,7 @@ func (az *Cloud) determinePublicIPName(clusterName string, service *v1.Service,
 
 	pipResourceGroup := az.getPublicIPAddressResourceGroup(service)
 	if id := getServicePIPPrefixID(service, isIPv6); id != "" {
-		pipName, err := az.getPublicIPName(clusterName, service, pipResourceGroup, isIPv6)
+		pipName, err := az.getPublicIPName(clusterName, service, isIPv6)
 		return pipName, false, err
 	}
 
@@ -878,7 +878,7 @@ func (az *Cloud) determinePublicIPName(clusterName string, service *v1.Service,
 	// Assume that the service without loadBalancerIP set is a primary service.
 	// If a secondary service doesn't set the loadBalancerIP, it is not allowed to share the IP.
 	if len(loadBalancerIP) == 0 {
-		pipName, err := az.getPublicIPName(clusterName, service, pipResourceGroup, isIPv6)
+		pipName, err := az.getPublicIPName(clusterName, service, isIPv6)
 		return pipName, false, err
 	}
 
@@ -896,24 +896,6 @@ func (az *Cloud) determinePublicIPName(clusterName string, service *v1.Service,
 	return "", false, fmt.Errorf("user supplied IP Address %s was not found in resource group %s", loadBalancerIP, pipResourceGroup)
 }
 
-// findMatchedPIPByIPFamilyAndServiceName is for IPv6. It tries to find a matching PIP whose name doesn't have IPv6 suffix.
-// Such PIP comes from old CCM without dual-stack support.
-func (az *Cloud) findMatchedPIPByIPFamilyAndServiceName(clusterName string, service *v1.Service, pipResourceGroup string) (*network.PublicIPAddress, error) {
-	pips, err := az.listPIP(pipResourceGroup)
-	if err != nil {
-		return nil, fmt.Errorf("findMatchedPIPByIPFamilyAndServiceName: failed to listPIP: %w", err)
-	}
-	baseName := az.GetLoadBalancerName(context.TODO(), clusterName, service)
-	for _, pip := range pips {
-		pip := pip
-		if pointer.StringDeref(pip.Name, "") == baseName && pip.PublicIPAddressPropertiesFormat != nil &&
-			pip.PublicIPAddressPropertiesFormat.PublicIPAddressVersion == network.IPv6 {
-			return &pip, nil
-		}
-	}
-	return nil, nil
-}
-
 func (az *Cloud) findMatchedPIPByLoadBalancerIP(service *v1.Service, loadBalancerIP, pipResourceGroup string) (*network.PublicIPAddress, error) {
 	pips, err := az.listPIP(pipResourceGroup)
 	if err != nil {
@@ -2984,6 +2966,8 @@ func shouldReleaseExistingOwnedPublicIP(existingPip *network.PublicIPAddress, lb
 		// #2 - If the load balancer is internal, and thus doesn't require public exposure
 		lbIsInternal ||
 		// #3 - If the name of this public ip does not match the desired name,
+		// NOTICE: For IPv6 Service created with CCM v1.27.1, the created PIP has IPv6 suffix.
+		// We need to recreate such PIP and current logic to delete needs no change.
 		(pipName != desiredPipName) ||
 		// #4 If the service annotations have specified the ip tags that the public ip must have, but they do not match the ip tags of the existing instance
 		(ipTagRequest.IPTagsRequestedByAnnotation && !areIPTagsEquivalent(currentIPTags, ipTagRequest.IPTags))

pkg/provider/azure_loadbalancer_test.go

@@ -100,7 +100,7 @@ func TestExistsPip(t *testing.T) {
 			func(client *mockpublicipclient.MockInterface) {
 				pips := []network.PublicIPAddress{
 					{
-						Name: pointer.String("testCluster-aservice-IPv6"),
+						Name: pointer.String("testCluster-aservice"),
 						PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
 							IPAddress: pointer.String("fe::1"),
 						},
@@ -110,6 +110,22 @@ func TestExistsPip(t *testing.T) {
 			},
 			true,
 		},
+		{
+			"IPv6 not exists - should not have suffix",
+			getTestService("service", v1.ProtocolTCP, nil, true, 80),
+			func(client *mockpublicipclient.MockInterface) {
+				pips := []network.PublicIPAddress{
+					{
+						Name: pointer.String("testCluster-aservice-IPv6"),
+						PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
+							IPAddress: pointer.String("fe::1"),
+						},
+					},
+				}
+				client.EXPECT().List(gomock.Any(), "rg").Return(pips, nil).MaxTimes(2)
+			},
+			false,
+		},
 		{
 			"DualStack exists",
 			getTestServiceDualStack("service", v1.ProtocolTCP, nil, 80),
@@ -1129,6 +1145,8 @@ func TestShouldReleaseExistingOwnedPublicIP(t *testing.T) {
 			},
 		},
 	}
+	existingPipWithTagIPv6Suffix := existingPipWithTag
+	existingPipWithTagIPv6Suffix.Name = pointer.String("testPIP-IPv6")
 
 	existingPipWithNoPublicIPAddressFormatProperties := network.PublicIPAddress{
 		ID:                              pointer.String("/subscriptions/subscription/resourceGroups/rg/providers/Microsoft.Network/publicIPAddresses/testPIP"),
@@ -1237,6 +1255,25 @@ func TestShouldReleaseExistingOwnedPublicIP(t *testing.T) {
 			},
 			expectedShouldRelease: true,
 		},
+		{
+			// This test is for IPv6 PIP created with CCM v1.27.1 and CCM gets upgraded.
+			// Such PIP should be recreated.
+			desc:           "matching except PIP name (with IPv6 suffix), expect release",
+			existingPip:    existingPipWithTagIPv6Suffix,
+			lbShouldExist:  true,
+			lbIsInternal:   false,
+			desiredPipName: *existingPipWithTag.Name,
+			ipTagRequest: serviceIPTagRequest{
+				IPTagsRequestedByAnnotation: true,
+				IPTags: &[]network.IPTag{
+					{
+						IPTagType: pointer.String("tag1"),
+						Tag:       pointer.String("tag1value"),
+					},
+				},
+			},
+			expectedShouldRelease: true,
+		},
 		{
 			desc:           "should delete orphaned managed public IP",
 			existingPip:    existingPipWithTag,
@@ -1277,13 +1314,15 @@ func TestShouldReleaseExistingOwnedPublicIP(t *testing.T) {
 		},
 	}
 
-	for i, c := range tests {
-		if c.tags != nil {
-			c.existingPip.Tags = c.tags
-		}
-		existingPip := c.existingPip
-		actualShouldRelease := shouldReleaseExistingOwnedPublicIP(&existingPip, c.lbShouldExist, c.lbIsInternal, c.isUserAssignedPIP, c.desiredPipName, c.ipTagRequest)
-		assert.Equal(t, c.expectedShouldRelease, actualShouldRelease, "TestCase[%d]: %s", i, c.desc)
+	for _, c := range tests {
+		t.Run(c.desc, func(t *testing.T) {
+			if c.tags != nil {
+				c.existingPip.Tags = c.tags
+			}
+			existingPip := c.existingPip
+			actualShouldRelease := shouldReleaseExistingOwnedPublicIP(&existingPip, c.lbShouldExist, c.lbIsInternal, c.isUserAssignedPIP, c.desiredPipName, c.ipTagRequest)
+			assert.Equal(t, c.expectedShouldRelease, actualShouldRelease)
+		})
 	}
 }
 

pkg/provider/azure_standard.go

@@ -326,26 +326,16 @@ func (az *Cloud) getRulePrefix(service *v1.Service) string {
 	return az.GetLoadBalancerName(context.TODO(), "", service)
 }
 
-func (az *Cloud) getPublicIPName(clusterName string, service *v1.Service, pipResourceGroup string, isIPv6 bool) (string, error) {
-	// For IPv6, the PIP may not have IPv6 suffix because it may be one created before dual-stack support.
-	if isIPv6 {
-		pip, err := az.findMatchedPIPByIPFamilyAndServiceName(clusterName, service, pipResourceGroup)
-		if err != nil {
-			return "", err
-		}
-		if pip != nil {
-			return pointer.StringDeref(pip.Name, ""), nil
-		}
-	}
-
+func (az *Cloud) getPublicIPName(clusterName string, service *v1.Service, isIPv6 bool) (string, error) {
+	isDualStack := isServiceDualStack(service)
 	pipName := fmt.Sprintf("%s-%s", clusterName, az.GetLoadBalancerName(context.TODO(), clusterName, service))
 	if id := getServicePIPPrefixID(service, isIPv6); id != "" {
 		id, err := getLastSegment(id, "/")
 		if err == nil {
 			pipName = fmt.Sprintf("%s-%s", pipName, id)
 		}
 	}
-	return getResourceByIPFamily(pipName, isIPv6), nil
+	return getResourceByIPFamily(pipName, isDualStack, isIPv6), nil
 }
 
 // TODO: UT

pkg/provider/azure_standard_test.go

@@ -2378,7 +2378,6 @@ func TestGetPublicIPName(t *testing.T) {
 					IPFamilies: []v1.IPFamily{v1.IPv4Protocol, v1.IPv6Protocol},
 				},
 			},
-			pips:            []network.PublicIPAddress{},
 			isIPv6:          true,
 			expectedPIPName: "azure-auid-prefix-id-ipv6-IPv6",
 		},
@@ -2392,30 +2391,30 @@ func TestGetPublicIPName(t *testing.T) {
 					IPFamilies: []v1.IPFamily{v1.IPv6Protocol},
 				},
 			},
-			pips: []network.PublicIPAddress{
-				{
-					Name: pointer.String("auid"),
-					PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
-						PublicIPAddressVersion: network.IPv6,
-					},
+			isIPv6:          true,
+			expectedPIPName: "azure-auid",
+		},
+		{
+			desc: "Service PIP IPv6 only with existing PIP - dualstack",
+			svc: &v1.Service{
+				ObjectMeta: meta.ObjectMeta{
+					UID: types.UID("uid"),
+				},
+				Spec: v1.ServiceSpec{
+					IPFamilies: []v1.IPFamily{v1.IPv4Protocol, v1.IPv6Protocol},
 				},
 			},
 			isIPv6:          true,
-			expectedPIPName: "auid",
+			expectedPIPName: "azure-auid-IPv6",
 		},
 	}
 
 	ctrl := gomock.NewController(t)
 	defer ctrl.Finish()
-	rg := "rg0"
 	for _, tc := range testcases {
 		t.Run(tc.desc, func(t *testing.T) {
 			az := GetTestCloud(ctrl)
-			if tc.isIPv6 {
-				mockPIPsClient := az.PublicIPAddressesClient.(*mockpublicipclient.MockInterface)
-				mockPIPsClient.EXPECT().List(gomock.Any(), rg).Return(tc.pips, nil)
-			}
-			name, err := az.getPublicIPName("azure", tc.svc, rg, tc.isIPv6)
+			name, err := az.getPublicIPName("azure", tc.svc, tc.isIPv6)
 			assert.Nil(t, err)
 			assert.Equal(t, tc.expectedPIPName, name)
 		})

pkg/provider/azure_utils.go

@@ -435,8 +435,12 @@ func getServicePIPPrefixID(service *v1.Service, isIPv6 bool) string {
 	return service.Annotations[consts.ServiceAnnotationPIPPrefixIDDualStack[isIPv6]]
 }
 
-func getResourceByIPFamily(resource string, isIPv6 bool) string {
-	if isIPv6 {
+// getResourceByIPFamily returns the resource name of with IPv6 suffix when
+// it is a dual-stack Service and the resource is of IPv6.
+// NOTICE: For PIPs of IPv6 Services created with CCM v1.27.1, after the CCM is upgraded,
+// the old PIPs will be recreated.
+func getResourceByIPFamily(resource string, isDualStack, isIPv6 bool) string {
+	if isDualStack && isIPv6 {
 		return fmt.Sprintf("%s-%s", resource, v6Suffix)
 	}
 	return resource

pkg/provider/azure_utils_test.go

@@ -856,24 +856,41 @@ func TestGetResourceByIPFamily(t *testing.T) {
 		desc             string
 		resource         string
 		isIPv6           bool
+		isDualStack      bool
 		expectedResource string
 	}{
 		{
-			"IPv4",
+			"DualStack - IPv4",
 			"resource0",
 			false,
+			true,
 			"resource0",
 		},
 		{
-			"IPv6",
+			"DualStack - IPv6",
 			"resource0",
 			true,
+			true,
 			"resource0-IPv6",
 		},
+		{
+			"SingleStack - IPv4",
+			"resource0",
+			false,
+			false,
+			"resource0",
+		},
+		{
+			"SingleStack - IPv6",
+			"resource0",
+			true,
+			false,
+			"resource0",
+		},
 	}
 	for _, tc := range testcases {
 		t.Run(tc.desc, func(t *testing.T) {
-			resource := getResourceByIPFamily(tc.resource, tc.isIPv6)
+			resource := getResourceByIPFamily(tc.resource, tc.isDualStack, tc.isIPv6)
 			assert.Equal(t, tc.expectedResource, resource)
 		})
 	}