Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: skip case sensitivity when checking Azure NSG rules #104384

Merged
merged 1 commit into from
Aug 19, 2021
Merged

fix: skip case sensitivity when checking Azure NSG rules #104384

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 9 additions & 9 deletions staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:]...)
Expand Down Expand Up @@ -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)) {
Expand All @@ -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
Expand Down
30 changes: 30 additions & 0 deletions staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
153 changes: 153 additions & 0 deletions staging/src/k8s.io/legacy-cloud-providers/azure/azure_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}