Merge pull request #55752 from kevinkim9264/fix-azure-loadbalancer

Automatic merge from submit-queue (batch tested with PRs 55812, 55752, 55447, 55848, 50984). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Azure Load Balancer reconciliation should consider all Kubernetes-controlled properties of a LB NSG

**What this PR does / why we need it**:
This PR refers to issue #55733 
With this PR, Kubernetes will update Azure nsg rules based on not just name, but also based on other properties such as destination port range and destination ip address.
We need it because right now Kubernetes will detect the difference and update only if there is difference in Name of nsg rule. It's been working fine for changing destination port range and source IP address because these two are part of the Name. (which external users should not assume) Basically right now, Kubernetes won't detect the difference if I go ahead and change any part of nsg rule using port UI. 
This PR will let Kubernetes detect the difference and always try to reconcile nsg rules with service definition.

**Which issue(s) this PR fixes** :
Fixes #55733 

**Special notes for your reviewer**: None

**Release note**:

```release-note
Kubernetes update Azure nsg rules based on not just difference in Name, but also in Protocol, SourcePortRange, DestinationPortRange, SourceAddressPrefix, DestinationAddressPrefix, Access, and Direction.
```

pkg/cloudprovider/providers/azure/azure_loadbalancer.go

@@ -1205,11 +1205,39 @@ func findRule(rules []network.LoadBalancingRule, rule network.LoadBalancingRule)
 	return false
 }
 
+// This compares rule's Name, Protocol, SourcePortRange, DestinationPortRange, SourceAddressPrefix, Access, and Direction.
+// Note that it compares rule's DestinationAddressPrefix only when it's not consolidated rule as such rule does not have DestinationAddressPrefix defined.
+// We intentionally do not compare DestinationAddressPrefixes in consolidated case because reconcileSecurityRule has to consider the two rules equal,
+// despite different DestinationAddressPrefixes, in order to give it a chance to consolidate the two rules.
 func findSecurityRule(rules []network.SecurityRule, rule network.SecurityRule) bool {
 	for _, existingRule := range rules {
-		if strings.EqualFold(*existingRule.Name, *rule.Name) {
-			return true
+		if !strings.EqualFold(*existingRule.Name, *rule.Name) {
+			continue
 		}
+		if existingRule.Protocol != rule.Protocol {
+			continue
+		}
+		if !strings.EqualFold(*existingRule.SourcePortRange, *rule.SourcePortRange) {
+			continue
+		}
+		if !strings.EqualFold(*existingRule.DestinationPortRange, *rule.DestinationPortRange) {
+			continue
+		}
+		if !strings.EqualFold(*existingRule.SourceAddressPrefix, *rule.SourceAddressPrefix) {
+			continue
+		}
+		if !allowsConsolidation(existingRule) && !allowsConsolidation(rule) {
+			if !strings.EqualFold(*existingRule.DestinationAddressPrefix, *rule.DestinationAddressPrefix) {
+				continue
+			}
+		}
+		if existingRule.Access != rule.Access {
+			continue
+		}
+		if existingRule.Direction != rule.Direction {
+			continue
+		}
+		return true
 	}
 	return false
 }

pkg/cloudprovider/providers/azure/azure_test.go

@@ -392,6 +392,36 @@ func TestReconcileLoadBalancerAddServiceOnInternalSubnet(t *testing.T) {
 	validateLoadBalancer(t, lb, svc)
 }
 
+func TestReconcileSecurityGroupFromAnyDestinationAddressPrefixToLoadBalancerIP(t *testing.T) {
+	az := getTestCloud()
+	svc1 := getTestService("serviceea", v1.ProtocolTCP, 80)
+	svc1.Spec.LoadBalancerIP = "192.168.0.0"
+	sg := getTestSecurityGroup(az)
+	// Simulate a pre-Kubernetes 1.8 NSG, where we do not specify the destination address prefix
+	sg, err := az.reconcileSecurityGroup(testClusterName, &svc1, to.StringPtr(""), true)
+	if err != nil {
+		t.Errorf("Unexpected error: %q", err)
+	}
+	sg, err = az.reconcileSecurityGroup(testClusterName, &svc1, to.StringPtr(svc1.Spec.LoadBalancerIP), true)
+	if err != nil {
+		t.Errorf("Unexpected error: %q", err)
+	}
+	validateSecurityGroup(t, sg, svc1)
+}
+
+func TestReconcileSecurityGroupDynamicLoadBalancerIP(t *testing.T) {
+	az := getTestCloud()
+	svc1 := getTestService("servicea", v1.ProtocolTCP, 80)
+	svc1.Spec.LoadBalancerIP = ""
+	sg := getTestSecurityGroup(az)
+	dynamicallyAssignedIP := "192.168.0.0"
+	sg, err := az.reconcileSecurityGroup(testClusterName, &svc1, to.StringPtr(dynamicallyAssignedIP), true)
+	if err != nil {
+		t.Errorf("unexpected error: %q", err)
+	}
+	validateSecurityGroup(t, sg, svc1)
+}
+
 // Test addition of services on an internal LB using both default and explicit subnets.
 func TestReconcileLoadBalancerAddServicesOnMultipleSubnets(t *testing.T) {
 	az := getTestCloud()