From edc19b8072755e195f423f687c615e37d90fb04e Mon Sep 17 00:00:00 2001 From: Pengfei Ni Date: Mon, 16 Aug 2021 05:22:22 +0000 Subject: [PATCH] fix: skip case sensitivity when checking Azure NSG rules --- .../azure/azure_loadbalancer.go | 18 +-- .../azure/azure_loadbalancer_test.go | 30 ++++ .../azure/azure_test.go | 153 ++++++++++++++++++ 3 files changed, 192 insertions(+), 9 deletions(-) diff --git a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer.go b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer.go index 3f3556639bb..40f38fb9f49 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer.go +++ b/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) - return nil, fmt.Errorf("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) + 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) - return nil, fmt.Errorf("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) + 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) - 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) + klog.V(4).Infof("Didn't find destination address %v in shared rule %s for service %s", 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 diff --git a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer_test.go b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer_test.go index 54b984b15ea..5ca7250c303 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer_test.go +++ b/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 { diff --git a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_test.go b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_test.go index 42fb037751e..8981cf5f932 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_test.go +++ b/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) + } +}