Merge pull request #3352 from lzhecheng/cp-3289-25

[release-1.25] Fix how getSecurityRuleName() handles IPv6 addr prefix
kubernetes-sigs · Feb 15, 2023 · c522c0e · c522c0e
2 parents 5eb5de4 + 45dc363
commit c522c0e
Show file tree

Hide file tree

Showing 3 changed files with 68 additions and 3 deletions.
diff --git a/pkg/provider/azure_standard.go b/pkg/provider/azure_standard.go
@@ -301,11 +301,11 @@ func (az *Cloud) getloadbalancerHAmodeRuleName(service *v1.Service) string {
 }
 
 func (az *Cloud) getSecurityRuleName(service *v1.Service, port v1.ServicePort, sourceAddrPrefix string) string {
+	safePrefix := strings.Replace(sourceAddrPrefix, "/", "_", -1)
+	safePrefix = strings.Replace(safePrefix, ":", ".", -1) // Consider IPv6 address
 	if useSharedSecurityRule(service) {
-		safePrefix := strings.Replace(sourceAddrPrefix, "/", "_", -1)
 		return fmt.Sprintf("shared-%s-%d-%s", port.Protocol, port.Port, safePrefix)
 	}
-	safePrefix := strings.Replace(sourceAddrPrefix, "/", "_", -1)
 	rulePrefix := az.getRulePrefix(service)
 	return fmt.Sprintf("%s-%s-%d-%s", rulePrefix, port.Protocol, port.Port, safePrefix)
 }

diff --git a/pkg/provider/azure_standard_test.go b/pkg/provider/azure_standard_test.go
@@ -2180,3 +2180,67 @@ func TestGetNodeVMSetName(t *testing.T) {
 		assert.Equal(t, tc.expectedVMSetName, vmSetName, tc.description)
 	}
 }
+
+func TestGetSecurityRuleName(t *testing.T) {
+	testcases := []struct {
+		desc             string
+		svc              *v1.Service
+		port             v1.ServicePort
+		sourceAddrPrefix string
+		expectedRuleName string
+	}{
+		{
+			"IPv4",
+			&v1.Service{
+				ObjectMeta: meta.ObjectMeta{
+					UID: "257b9655-5137-4ad2-b091-ef3f07043ad3",
+				},
+			},
+			v1.ServicePort{
+				Protocol: v1.ProtocolTCP,
+				Port:     80,
+			},
+			"10.0.0.1/24",
+			"a257b965551374ad2b091ef3f07043ad-TCP-80-10.0.0.1_24",
+		},
+		{
+			"IPv4-shared",
+			&v1.Service{
+				ObjectMeta: meta.ObjectMeta{
+					UID:         "257b9655-5137-4ad2-b091-ef3f07043ad3",
+					Annotations: map[string]string{consts.ServiceAnnotationSharedSecurityRule: "true"},
+				},
+			},
+			v1.ServicePort{
+				Protocol: v1.ProtocolTCP,
+				Port:     80,
+			},
+			"10.0.0.1/24",
+			"shared-TCP-80-10.0.0.1_24",
+		},
+		{
+			"IPv6",
+			&v1.Service{
+				ObjectMeta: meta.ObjectMeta{
+					UID: "257b9655-5137-4ad2-b091-ef3f07043ad3",
+				},
+			},
+			v1.ServicePort{
+				Protocol: v1.ProtocolTCP,
+				Port:     80,
+			},
+			"2001:0:0::1/64",
+			"a257b965551374ad2b091ef3f07043ad-TCP-80-2001.0.0..1_64",
+		},
+	}
+
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+	az := GetTestCloud(ctrl)
+	for _, tc := range testcases {
+		t.Run(tc.desc, func(t *testing.T) {
+			ruleName := az.getSecurityRuleName(tc.svc, tc.port, tc.sourceAddrPrefix)
+			assert.Equal(t, tc.expectedRuleName, ruleName)
+		})
+	}
+}
diff --git a/tests/e2e/network/network_security_group.go b/tests/e2e/network/network_security_group.go
@@ -280,7 +280,8 @@ var _ = Describe("Network security group", Label(utils.TestSuiteLabelNSG), func(
 		By("Checking if there is a LoadBalancerSourceRanges rule")
 		nsgs, err = tc.GetClusterSecurityGroups()
 		Expect(err).NotTo(HaveOccurred())
-		found = validateLoadBalancerSourceRangesRuleExists(nsgs, internalIP, allowCIDR, fmt.Sprintf("%s_%d", hostExecPodIP, mask))
+		ipRangesSuffix := strings.Replace(fmt.Sprintf("%s_%d", hostExecPodIP, mask), ":", ".", -1) // Handled in pkg/provider/getSecurityRuleName()
+		found = validateLoadBalancerSourceRangesRuleExists(nsgs, internalIP, allowCIDR, ipRangesSuffix)
 		Expect(found).To(BeTrue())
 
 		By("Checking if there is a deny_all rule")