Merge pull request #104384 from feiskyer/fix-nsg-case

fix: skip case sensitivity when checking Azure NSG rules

staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer.go

@@ -1858,18 +1858,18 @@ func (az *Cloud) reconcileSecurityGroup(clusterName string, service *v1.Service,
 				sharedRuleName := az.getSecurityRuleName(service, port, sourceAddressPrefix)
 				sharedIndex, sharedRule, sharedRuleFound := findSecurityRuleByName(updatedRules, sharedRuleName)
 				if !sharedRuleFound {
-					klog.V(4).Infof("Expected to find shared rule %s for service %s being deleted, but did not", sharedRuleName, service.Name)
+					klog.V(4).Infof("Didn't find shared rule %s for service %s", sharedRuleName, service.Name)
-					return nil, fmt.Errorf("expected to find shared rule %s for service %s being deleted, but did not", sharedRuleName, service.Name)
+					continue
 				}
 				if sharedRule.DestinationAddressPrefixes == nil {
-					klog.V(4).Infof("Expected to have array of destinations in shared rule for service %s being deleted, but did not", service.Name)
+					klog.V(4).Infof("Didn't find DestinationAddressPrefixes in shared rule for service %s", service.Name)
-					return nil, fmt.Errorf("expected to have array of destinations in shared rule for service %s being deleted, but did not", service.Name)
+					continue
 				}
 				existingPrefixes := *sharedRule.DestinationAddressPrefixes
 				addressIndex, found := findIndex(existingPrefixes, destinationIPAddress)
 				if !found {
-					klog.V(4).Infof("Expected to find destination address %s in shared rule %s for service %s being deleted, but did not", destinationIPAddress, sharedRuleName, service.Name)
+					klog.V(4).Infof("Didn't find destination address %v in shared rule %s for service %s", destinationIPAddress, sharedRuleName, service.Name)
-					return nil, fmt.Errorf("expected to find destination address %s in shared rule %s for service %s being deleted, but did not", destinationIPAddress, sharedRuleName, service.Name)
+					continue
 				}
 				if len(existingPrefixes) == 1 {
 					updatedRules = append(updatedRules[:sharedIndex], updatedRules[sharedIndex+1:]...)
@@ -2426,7 +2426,7 @@ func findSecurityRule(rules []network.SecurityRule, rule network.SecurityRule) b
 		if !strings.EqualFold(to.String(existingRule.Name), to.String(rule.Name)) {
 			continue
 		}
-		if existingRule.Protocol != rule.Protocol {
+		if !strings.EqualFold(string(existingRule.Protocol), string(rule.Protocol)) {
 			continue
 		}
 		if !strings.EqualFold(to.String(existingRule.SourcePortRange), to.String(rule.SourcePortRange)) {
@@ -2443,10 +2443,10 @@ func findSecurityRule(rules []network.SecurityRule, rule network.SecurityRule) b
 				continue
 			}
 		}
-		if existingRule.Access != rule.Access {
+		if !strings.EqualFold(string(existingRule.Access), string(rule.Access)) {
 			continue
 		}
-		if existingRule.Direction != rule.Direction {
+		if !strings.EqualFold(string(existingRule.Direction), string(rule.Direction)) {
 			continue
 		}
 		return true

staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer_test.go

@@ -2585,6 +2585,36 @@ func TestReconcileSecurityGroup(t *testing.T) {
 				},
 			},
 		},
+		{
+			desc:    "reconcileSecurityGroup shall create shared sgs for service with azure-shared-securityrule annotations",
+			service: getTestService("test1", v1.ProtocolTCP, map[string]string{ServiceAnnotationSharedSecurityRule: "true"}, true, 80),
+			existingSgs: map[string]network.SecurityGroup{"nsg": {
+				Name:                          to.StringPtr("nsg"),
+				SecurityGroupPropertiesFormat: &network.SecurityGroupPropertiesFormat{},
+			}},
+			lbIP:   to.StringPtr("1.2.3.4"),
+			wantLb: true,
+			expectedSg: &network.SecurityGroup{
+				Name: to.StringPtr("nsg"),
+				SecurityGroupPropertiesFormat: &network.SecurityGroupPropertiesFormat{
+					SecurityRules: &[]network.SecurityRule{
+						{
+							Name: to.StringPtr("shared-TCP-80-Internet"),
+							SecurityRulePropertiesFormat: &network.SecurityRulePropertiesFormat{
+								Protocol:                   network.SecurityRuleProtocol("Tcp"),
+								SourcePortRange:            to.StringPtr("*"),
+								DestinationPortRange:       to.StringPtr("80"),
+								SourceAddressPrefix:        to.StringPtr("Internet"),
+								DestinationAddressPrefixes: to.StringSlicePtr([]string{"1.2.3.4"}),
+								Access:                     network.SecurityRuleAccess("Allow"),
+								Priority:                   to.Int32Ptr(500),
+								Direction:                  network.SecurityRuleDirection("Inbound"),
+							},
+						},
+					},
+				},
+			},
+		},
 	}
 
 	for i, test := range testCases {

staging/src/k8s.io/legacy-cloud-providers/azure/azure_test.go

@@ -3336,3 +3336,156 @@ func TestInitializeCloudFromConfig(t *testing.T) {
 	expectedErr = fmt.Errorf("useInstanceMetadata must be enabled without Azure credentials")
 	assert.Equal(t, expectedErr, err)
 }
+
+func TestFindSecurityRule(t *testing.T) {
+	testRuleName := "test-rule"
+	testIP1 := "192.168.192.168"
+	sg := network.SecurityRule{
+		Name: to.StringPtr(testRuleName),
+		SecurityRulePropertiesFormat: &network.SecurityRulePropertiesFormat{
+			Protocol:                 network.SecurityRuleProtocolTCP,
+			SourcePortRange:          to.StringPtr("*"),
+			SourceAddressPrefix:      to.StringPtr("Internet"),
+			DestinationPortRange:     to.StringPtr("80"),
+			DestinationAddressPrefix: to.StringPtr(testIP1),
+			Access:                   network.SecurityRuleAccessAllow,
+			Direction:                network.SecurityRuleDirectionInbound,
+		},
+	}
+	testCases := []struct {
+		desc     string
+		testRule network.SecurityRule
+		expected bool
+	}{
+		{
+			desc:     "false should be returned for an empty rule",
+			testRule: network.SecurityRule{},
+			expected: false,
+		},
+		{
+			desc: "false should be returned when rule name doesn't match",
+			testRule: network.SecurityRule{
+				Name: to.StringPtr("not-the-right-name"),
+			},
+			expected: false,
+		},
+		{
+			desc: "false should be returned when protocol doesn't match",
+			testRule: network.SecurityRule{
+				Name: to.StringPtr(testRuleName),
+				SecurityRulePropertiesFormat: &network.SecurityRulePropertiesFormat{
+					Protocol: network.SecurityRuleProtocolUDP,
+				},
+			},
+			expected: false,
+		},
+		{
+			desc: "false should be returned when SourcePortRange doesn't match",
+			testRule: network.SecurityRule{
+				Name: to.StringPtr(testRuleName),
+				SecurityRulePropertiesFormat: &network.SecurityRulePropertiesFormat{
+					Protocol:        network.SecurityRuleProtocolUDP,
+					SourcePortRange: to.StringPtr("1.2.3.4/32"),
+				},
+			},
+			expected: false,
+		},
+		{
+			desc: "false should be returned when SourceAddressPrefix doesn't match",
+			testRule: network.SecurityRule{
+				Name: to.StringPtr(testRuleName),
+				SecurityRulePropertiesFormat: &network.SecurityRulePropertiesFormat{
+					Protocol:            network.SecurityRuleProtocolUDP,
+					SourcePortRange:     to.StringPtr("*"),
+					SourceAddressPrefix: to.StringPtr("2.3.4.0/24"),
+				},
+			},
+			expected: false,
+		},
+		{
+			desc: "false should be returned when DestinationPortRange doesn't match",
+			testRule: network.SecurityRule{
+				Name: to.StringPtr(testRuleName),
+				SecurityRulePropertiesFormat: &network.SecurityRulePropertiesFormat{
+					Protocol:             network.SecurityRuleProtocolUDP,
+					SourcePortRange:      to.StringPtr("*"),
+					SourceAddressPrefix:  to.StringPtr("Internet"),
+					DestinationPortRange: to.StringPtr("443"),
+				},
+			},
+			expected: false,
+		},
+		{
+			desc: "false should be returned when DestinationAddressPrefix doesn't match",
+			testRule: network.SecurityRule{
+				Name: to.StringPtr(testRuleName),
+				SecurityRulePropertiesFormat: &network.SecurityRulePropertiesFormat{
+					Protocol:                 network.SecurityRuleProtocolUDP,
+					SourcePortRange:          to.StringPtr("*"),
+					SourceAddressPrefix:      to.StringPtr("Internet"),
+					DestinationPortRange:     to.StringPtr("80"),
+					DestinationAddressPrefix: to.StringPtr("192.168.0.3"),
+				},
+			},
+			expected: false,
+		},
+		{
+			desc: "false should be returned when Access doesn't match",
+			testRule: network.SecurityRule{
+				Name: to.StringPtr(testRuleName),
+				SecurityRulePropertiesFormat: &network.SecurityRulePropertiesFormat{
+					Protocol:                 network.SecurityRuleProtocolUDP,
+					SourcePortRange:          to.StringPtr("*"),
+					SourceAddressPrefix:      to.StringPtr("Internet"),
+					DestinationPortRange:     to.StringPtr("80"),
+					DestinationAddressPrefix: to.StringPtr(testIP1),
+					Access:                   network.SecurityRuleAccessDeny,
+					// Direction:                network.SecurityRuleDirectionInbound,
+				},
+			},
+			expected: false,
+		},
+		{
+			desc: "false should be returned when Direction doesn't match",
+			testRule: network.SecurityRule{
+				Name: to.StringPtr(testRuleName),
+				SecurityRulePropertiesFormat: &network.SecurityRulePropertiesFormat{
+					Protocol:                 network.SecurityRuleProtocolUDP,
+					SourcePortRange:          to.StringPtr("*"),
+					SourceAddressPrefix:      to.StringPtr("Internet"),
+					DestinationPortRange:     to.StringPtr("80"),
+					DestinationAddressPrefix: to.StringPtr(testIP1),
+					Access:                   network.SecurityRuleAccessAllow,
+					Direction:                network.SecurityRuleDirectionOutbound,
+				},
+			},
+			expected: false,
+		},
+		{
+			desc: "true should be returned when everything matches but protocol is in different case",
+			testRule: network.SecurityRule{
+				Name: to.StringPtr(testRuleName),
+				SecurityRulePropertiesFormat: &network.SecurityRulePropertiesFormat{
+					Protocol:                 network.SecurityRuleProtocol("TCP"),
+					SourcePortRange:          to.StringPtr("*"),
+					SourceAddressPrefix:      to.StringPtr("Internet"),
+					DestinationPortRange:     to.StringPtr("80"),
+					DestinationAddressPrefix: to.StringPtr(testIP1),
+					Access:                   network.SecurityRuleAccessAllow,
+					Direction:                network.SecurityRuleDirectionInbound,
+				},
+			},
+			expected: true,
+		},
+		{
+			desc:     "true should be returned when everything matches",
+			testRule: sg,
+			expected: true,
+		},
+	}
+
+	for i := range testCases {
+		found := findSecurityRule([]network.SecurityRule{sg}, testCases[i].testRule)
+		assert.Equal(t, testCases[i].expected, found, testCases[i].desc)
+	}
+}