fix: remove shared security rule when svc with shared-nsg annotation …

…is deleted Signed-off-by: MartinForReal <fanshangxiang@gmail.com> (cherry picked from commit a894df4)
kubernetes-sigs · Feb 22, 2023 · 20a5575 · 20a5575
1 parent 5707267
commit 20a5575
Show file tree

Hide file tree

Showing 3 changed files with 53 additions and 14 deletions.
diff --git a/pkg/provider/azure_loadbalancer.go b/pkg/provider/azure_loadbalancer.go
@@ -2638,6 +2638,7 @@ func (az *Cloud) reconcileSecurityRules(sg network.SecurityGroup, service *v1.Se
 				}
 				if sharedRule.DestinationAddressPrefixes == nil {
 					klog.V(4).Infof("Didn't find DestinationAddressPrefixes in shared rule for service %s", service.Name)
+					updatedRules = append(updatedRules[:sharedIndex], updatedRules[sharedIndex+1:]...)
 					continue
 				}
 				existingPrefixes := *sharedRule.DestinationAddressPrefixes
@@ -2681,10 +2682,14 @@ func (az *Cloud) reconcileSecurityRules(sg network.SecurityGroup, service *v1.Se
 		}
 		if foundRule && allowsConsolidation(expectedRule) {
 			index, _ := findConsolidationCandidate(updatedRules, expectedRule)
-			updatedRules[index] = consolidate(updatedRules[index], expectedRule)
+			if updatedRules[index].DestinationAddressPrefixes != nil {
+				updatedRules[index] = consolidate(updatedRules[index], expectedRule)
+			} else {
+				updatedRules = append(updatedRules[:index], updatedRules[index+1:]...)
+			}
 			dirtySg = true
 		}
-		if !foundRule {
+		if !foundRule && wantLb {
 			klog.V(10).Infof("reconcile(%s)(%t): sg rule(%s) - adding", serviceName, wantLb, *expectedRule.Name)
 
 			nextAvailablePriority, err := getNextAvailablePriority(updatedRules)

diff --git a/pkg/provider/azure_loadbalancer_test.go b/pkg/provider/azure_loadbalancer_test.go
@@ -3637,6 +3637,38 @@ func TestReconcileSecurityGroup(t *testing.T) {
 				},
 			},
 		},
+		{
+			desc:    "reconcileSecurityGroup shall delete shared sgs for service with azure-shared-securityrule annotations",
+			service: getTestService("test1", v1.ProtocolTCP, map[string]string{consts.ServiceAnnotationSharedSecurityRule: "true"}, true, 80),
+			existingSgs: map[string]network.SecurityGroup{"nsg": {
+				Name: pointer.String("nsg"),
+				SecurityGroupPropertiesFormat: &network.SecurityGroupPropertiesFormat{
+					SecurityRules: &[]network.SecurityRule{
+						{
+							Name: pointer.String("shared-TCP-80-Internet"),
+							SecurityRulePropertiesFormat: &network.SecurityRulePropertiesFormat{
+								Protocol:                   network.SecurityRuleProtocol("Tcp"),
+								SourcePortRange:            pointer.String("*"),
+								DestinationPortRange:       pointer.String("80"),
+								SourceAddressPrefix:        pointer.String("Internet"),
+								DestinationAddressPrefixes: &([]string{"1.2.3.4"}),
+								Access:                     network.SecurityRuleAccess("Allow"),
+								Priority:                   pointer.Int32(500),
+								Direction:                  network.SecurityRuleDirection("Inbound"),
+							},
+						},
+					},
+				},
+			}},
+			lbIP:   pointer.String("1.2.3.4"),
+			wantLb: false,
+			expectedSg: &network.SecurityGroup{
+				Name: pointer.String("nsg"),
+				SecurityGroupPropertiesFormat: &network.SecurityGroupPropertiesFormat{
+					SecurityRules: &[]network.SecurityRule{},
+				},
+			},
+		},
 		{
 			desc:    "reconcileSecurityGroup shall create sgs with floating IP disabled",
 			service: getTestService("test1", v1.ProtocolTCP, map[string]string{consts.ServiceAnnotationDisableLoadBalancerFloatingIP: "true"}, false, 80),

diff --git a/tests/e2e/network/network_security_group.go b/tests/e2e/network/network_security_group.go
@@ -135,7 +135,7 @@ var _ = Describe("Network security group", Label(utils.TestSuiteLabelNSG), func(
 		Expect(err).To(BeNil(), "Fail to automatically delete the rule")
 	})
 
-	It("should support service annotation `service.beta.kubernetes.io/azure-shared-securityrule`", func() {
+	It("should support service annotation `service.beta.kubernetes.io/azure-shared-securityrule`", func(ctx SpecContext) {
 		By("Exposing two services with shared security rule")
 		annotation := map[string]string{
 			consts.ServiceAnnotationSharedSecurityRule: "true",
@@ -166,21 +166,23 @@ var _ = Describe("Network security group", Label(utils.TestSuiteLabelNSG), func(
 		By("Validate automatically adjust or delete the rule, when service is deleted")
 		Expect(utils.DeleteService(cs, ns.Name, serviceName)).NotTo(HaveOccurred())
 		ipList = []string{ip2}
-		Expect(validateSharedSecurityRuleExists(nsgs, ipList, port)).To(BeTrue(), "Security rule should be modified to only contain service %s", serviceName2)
+
+		Eventually(func() (bool, error) {
+			nsgs, err = tc.GetClusterSecurityGroups()
+			if err != nil {
+				return false, err
+			}
+			return validateSharedSecurityRuleExists(nsgs, []string{ip2}, port) && !validateSharedSecurityRuleExists(nsgs, []string{ip1}, port), nil
+		}).WithContext(ctx).Should(BeTrue(), "Security rule should be modified to only contain service %s", serviceName2)
 
 		Expect(utils.DeleteService(cs, ns.Name, serviceName2)).NotTo(HaveOccurred())
-		isDeleted := false
-		for i := 1; i <= 30; i++ {
+		Eventually(func() (bool, error) {
 			nsgs, err := tc.GetClusterSecurityGroups()
-			Expect(err).NotTo(HaveOccurred())
-			if !validateSharedSecurityRuleExists(nsgs, ipList, port) {
-				utils.Logf("Target rule successfully deleted")
-				isDeleted = true
-				break
+			if err != nil {
+				return false, err
 			}
-			time.Sleep(20 * time.Second)
-		}
-		Expect(isDeleted).To(BeTrue(), "Fail to automatically delete the shared security rule")
+			return validateSharedSecurityRuleExists(nsgs, ipList, port), nil
+		}).WithContext(ctx).Should(BeFalse(), "Fail to automatically delete the shared security rule")
 	})
 
 	It("can set source IP prefixes automatically according to corresponding service tag", func() {