From 61fbba74e55b2fb5405096ad18442e3a7cfa0676 Mon Sep 17 00:00:00 2001 From: Rita Zhang Date: Mon, 19 Nov 2018 17:07:43 -0800 Subject: [PATCH] Collapse source range in nsg --- .../providers/azure/azure_loadbalancer.go | 104 +++++-- .../providers/azure/azure_standard.go | 5 + .../providers/azure/azure_test.go | 275 ++++++++++++++---- 3 files changed, 308 insertions(+), 76 deletions(-) diff --git a/pkg/cloudprovider/providers/azure/azure_loadbalancer.go b/pkg/cloudprovider/providers/azure/azure_loadbalancer.go index 77bfdd20de..3eb0984a56 100644 --- a/pkg/cloudprovider/providers/azure/azure_loadbalancer.go +++ b/pkg/cloudprovider/providers/azure/azure_loadbalancer.go @@ -991,7 +991,7 @@ func (az *Cloud) reconcileSecurityGroup(clusterName string, service *v1.Service, if err != nil { return nil, err } - var sourceAddressPrefixes []string + sourceAddressPrefixes := []string{} if (sourceRanges == nil || serviceapi.IsAllowAll(sourceRanges)) && len(serviceTags) == 0 { if !requiresInternalLoadBalancer(service) { sourceAddressPrefixes = []string{"Internet"} @@ -1006,35 +1006,51 @@ func (az *Cloud) reconcileSecurityGroup(clusterName string, service *v1.Service, } expectedSecurityRules := []network.SecurityRule{} - if wantLb { - expectedSecurityRules = make([]network.SecurityRule, len(ports)*len(sourceAddressPrefixes)) - - for i, port := range ports { + if wantLb && len(sourceAddressPrefixes) > 0 { + for _, port := range ports { _, securityProto, _, err := getProtocolsFromKubernetesProtocol(port.Protocol) if err != nil { return nil, err } - for j := range sourceAddressPrefixes { - ix := i*len(sourceAddressPrefixes) + j - securityRuleName := az.getSecurityRuleName(service, port, sourceAddressPrefixes[j]) - expectedSecurityRules[ix] = network.SecurityRule{ + if useSharedSecurityRule(service) { + // When service is shared, a separate rule is created for each element in sourceAddressPrefixes + for j := range sourceAddressPrefixes { + securityRuleName := az.getSecurityRuleName(service, port, sourceAddressPrefixes[j]) + expectedSecurityRules = append(expectedSecurityRules, network.SecurityRule{ + Name: to.StringPtr(securityRuleName), + SecurityRulePropertiesFormat: &network.SecurityRulePropertiesFormat{ + Protocol: *securityProto, + SourcePortRange: to.StringPtr("*"), + DestinationPortRange: to.StringPtr(strconv.Itoa(int(port.Port))), + SourceAddressPrefix: to.StringPtr(sourceAddressPrefixes[j]), + DestinationAddressPrefix: to.StringPtr(destinationIPAddress), + Access: network.SecurityRuleAccessAllow, + Direction: network.SecurityRuleDirectionInbound, + }, + }) + } + } else { + // When service is not shared, a single rule UUID-PROTOCOL-PORT is created + securityRuleName := az.getSecurityRuleName(service, port, "") + expectedSecurityRules = append(expectedSecurityRules, network.SecurityRule{ Name: to.StringPtr(securityRuleName), SecurityRulePropertiesFormat: &network.SecurityRulePropertiesFormat{ Protocol: *securityProto, SourcePortRange: to.StringPtr("*"), DestinationPortRange: to.StringPtr(strconv.Itoa(int(port.Port))), - SourceAddressPrefix: to.StringPtr(sourceAddressPrefixes[j]), + SourceAddressPrefixes: &sourceAddressPrefixes, DestinationAddressPrefix: to.StringPtr(destinationIPAddress), Access: network.SecurityRuleAccessAllow, Direction: network.SecurityRuleDirectionInbound, }, - } + }) } + } } for _, r := range expectedSecurityRules { - klog.V(10).Infof("Expecting security rule for %s: %s:%s -> %s:%s", service.Name, *r.SourceAddressPrefix, *r.SourcePortRange, *r.DestinationAddressPrefix, *r.DestinationPortRange) + klog.V(10).Infof("Expecting security rule for %s: %s:%s -> %s:%s", service.Name, logSafeCollection(r.SourceAddressPrefix, r.SourceAddressPrefixes), *r.SourcePortRange, *r.DestinationAddressPrefix, *r.DestinationPortRange) } // update security rules @@ -1045,7 +1061,7 @@ func (az *Cloud) reconcileSecurityGroup(clusterName string, service *v1.Service, } for _, r := range updatedRules { - klog.V(10).Infof("Existing security rule while processing %s: %s:%s -> %s:%s", service.Name, logSafe(r.SourceAddressPrefix), logSafe(r.SourcePortRange), logSafeCollection(r.DestinationAddressPrefix, r.DestinationAddressPrefixes), logSafe(r.DestinationPortRange)) + klog.V(10).Infof("Existing security rule while processing %s: %s:%s -> %s:%s", service.Name, logSafeCollection(r.SourceAddressPrefix, r.SourceAddressPrefixes), logSafe(r.SourcePortRange), logSafeCollection(r.DestinationAddressPrefix, r.DestinationAddressPrefixes), logSafe(r.DestinationPortRange)) } // update security rules: remove unwanted rules that belong privately @@ -1074,17 +1090,14 @@ 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) } 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) } 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) } if len(existingPrefixes) == 1 { @@ -1118,10 +1131,26 @@ func (az *Cloud) reconcileSecurityGroup(clusterName string, service *v1.Service, foundRule = true } if foundRule && allowsConsolidation(expectedRule) { + klog.V(2).Infof("reconcile(%s)(%t): sg rule(%s) - allowConsolidation", serviceName, wantLb, *expectedRule.Name) index, _ := findConsolidationCandidate(updatedRules, expectedRule) updatedRules[index] = consolidate(updatedRules[index], expectedRule) dirtySg = true } + if foundRule && !allowsConsolidation(expectedRule) { + // For backward compatibility, for not shared rules, + // if existing rule(s)'s name contains or equals to UUID-PROTOCOL-PORT, + // then replace the first matching rule with the new rule with the new naming convention. + // Keep the same priority as the existing rule. + // Remove rest of the matching rules as the new rule already includes all the SourceAddressPrefixes. + // This also takes care of service updates. + klog.V(2).Infof("reconcile(%s)(%t): sg rule(%s) - not allowConsolidation", serviceName, wantLb, *expectedRule.Name) + updated := false + updatedRules, updated = updateMatchingRules(updatedRules, expectedRule) + if !updated { + return nil, fmt.Errorf("Expected to update rules containing %s for service %s, but did not", *expectedRule.Name, service.Name) + } + dirtySg = true + } if !foundRule { klog.V(10).Infof("reconcile(%s)(%t): sg rule(%s) - adding", serviceName, wantLb, *expectedRule.Name) @@ -1137,7 +1166,7 @@ func (az *Cloud) reconcileSecurityGroup(clusterName string, service *v1.Service, } for _, r := range updatedRules { - klog.V(10).Infof("Updated security rule while processing %s: %s:%s -> %s:%s", service.Name, logSafe(r.SourceAddressPrefix), logSafe(r.SourcePortRange), logSafeCollection(r.DestinationAddressPrefix, r.DestinationAddressPrefixes), logSafe(r.DestinationPortRange)) + klog.V(10).Infof("Updated security rule while processing %s: %s:%s -> %s:%s", service.Name, logSafeCollection(r.SourceAddressPrefix, r.SourceAddressPrefixes), logSafe(r.SourcePortRange), logSafeCollection(r.DestinationAddressPrefix, r.DestinationAddressPrefixes), logSafe(r.DestinationPortRange)) } if dirtySg { @@ -1213,6 +1242,25 @@ func findConsolidationCandidate(rules []network.SecurityRule, rule network.Secur return 0, false } +func updateMatchingRules(existingRules []network.SecurityRule, updatedRule network.SecurityRule) ([]network.SecurityRule, bool) { + updatedRules := []network.SecurityRule{} + updated := false + for _, existingRule := range existingRules { + if strings.Contains(to.String(existingRule.Name), to.String(updatedRule.Name)) { + if !updated { + updated = true + updatedRule.Priority = existingRule.Priority + updatedRules = append(updatedRules, updatedRule) + } + // skip if another matching rule is found after an existing rule has been updated + } else { + updatedRules = append(updatedRules, existingRule) + } + } + + return updatedRules, updated +} + func makeConsolidatable(rule network.SecurityRule) network.SecurityRule { return network.SecurityRule{ Name: rule.Name, @@ -1245,8 +1293,8 @@ func consolidate(existingRule network.SecurityRule, newRule network.SecurityRule SourcePortRanges: existingRule.SourcePortRanges, DestinationPortRange: existingRule.DestinationPortRange, DestinationPortRanges: existingRule.DestinationPortRanges, - SourceAddressPrefix: existingRule.SourceAddressPrefix, - SourceAddressPrefixes: existingRule.SourceAddressPrefixes, + SourceAddressPrefix: newRule.SourceAddressPrefix, + SourceAddressPrefixes: newRule.SourceAddressPrefixes, DestinationAddressPrefixes: destinations, Access: existingRule.Access, Direction: existingRule.Direction, @@ -1473,12 +1521,19 @@ func equalLoadBalancingRulePropertiesFormat(s, t *network.LoadBalancingRulePrope // 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. +// We intentionally do not compare SourceAddressPrefixes and DestinationAddressPrefixes in consolidated case because reconcileSecurityRule has to consider the two rules equal, +// despite different SourceAddressPrefixes and 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(to.String(existingRule.Name), to.String(rule.Name)) { - continue + if allowsConsolidation(existingRule) && allowsConsolidation(rule) { + if !strings.EqualFold(to.String(existingRule.Name), to.String(rule.Name)) { + continue + } + } else { + // for backward compatibiiity as existing rule name UUID-PROTOCOL-PORT-SOURCEIP should contain new naming pattern UUID-PROTOCOL-PORT + if !strings.Contains(to.String(existingRule.Name), to.String(rule.Name)) { + continue + } } if existingRule.Protocol != rule.Protocol { continue @@ -1489,9 +1544,6 @@ func findSecurityRule(rules []network.SecurityRule, rule network.SecurityRule) b if !strings.EqualFold(to.String(existingRule.DestinationPortRange), to.String(rule.DestinationPortRange)) { continue } - if !strings.EqualFold(to.String(existingRule.SourceAddressPrefix), to.String(rule.SourceAddressPrefix)) { - continue - } if !allowsConsolidation(existingRule) && !allowsConsolidation(rule) { if !strings.EqualFold(to.String(existingRule.DestinationAddressPrefix), to.String(rule.DestinationAddressPrefix)) { continue diff --git a/pkg/cloudprovider/providers/azure/azure_standard.go b/pkg/cloudprovider/providers/azure/azure_standard.go index 95126f58b0..a33c6d57e6 100644 --- a/pkg/cloudprovider/providers/azure/azure_standard.go +++ b/pkg/cloudprovider/providers/azure/azure_standard.go @@ -236,6 +236,11 @@ func (az *Cloud) getSecurityRuleName(service *v1.Service, port v1.ServicePort, s safePrefix := strings.Replace(sourceAddrPrefix, "/", "_", -1) return fmt.Sprintf("shared-%s-%d-%s", port.Protocol, port.Port, safePrefix) } + if sourceAddrPrefix == "" { + rulePrefix := az.getRulePrefix(service) + return fmt.Sprintf("%s-%s-%d", rulePrefix, port.Protocol, port.Port) + } + // ensure backward compatibility safePrefix := strings.Replace(sourceAddrPrefix, "/", "_", -1) rulePrefix := az.getRulePrefix(service) return fmt.Sprintf("%s-%s-%d-%s", rulePrefix, port.Protocol, port.Port, safePrefix) diff --git a/pkg/cloudprovider/providers/azure/azure_test.go b/pkg/cloudprovider/providers/azure/azure_test.go index 0369d59a53..4a54512145 100644 --- a/pkg/cloudprovider/providers/azure/azure_test.go +++ b/pkg/cloudprovider/providers/azure/azure_test.go @@ -23,6 +23,7 @@ import ( "math" "net" "net/http" + "sort" "strings" "testing" @@ -852,6 +853,26 @@ func TestReconcileSecurityWithSourceRanges(t *testing.T) { } validateSecurityGroup(t, sg, svc) + + expectedRuleCount := 2 + if len(*sg.SecurityRules) != expectedRuleCount { + t.Errorf("Expected security group to have %d rules but it had %d", expectedRuleCount, len(*sg.SecurityRules)) + } + expectedRuleName := "aservicea-TCP-80" + _, securityRule, ruleFound := findSecurityRuleByName(*sg.SecurityRules, expectedRuleName) + if !ruleFound { + t.Fatalf("Expected security rule %q but it was not present", expectedRuleName) + } + + expectedSourceAddressPrefixesCount := 2 + if securityRule.SourceAddressPrefixes == nil || len(*securityRule.SourceAddressPrefixes) != expectedSourceAddressPrefixesCount { + t.Errorf("Shared rule %s should have had %d source addresses but had %d", expectedRuleName, expectedSourceAddressPrefixesCount, len(*securityRule.SourceAddressPrefixes)) + } + + err = securityRuleMatches("10.0.0.0/32,192.168.0.0/24", v1.ServicePort{Port: 80}, lbStatus.Ingress[0].IP, securityRule) + if err != nil { + t.Errorf("Shared rule %s did not match service IP: %v", expectedRuleName, err) + } } func TestReconcilePublicIPWithNewService(t *testing.T) { @@ -1155,8 +1176,7 @@ func getServiceSourceRanges(service *v1.Service) []string { return service.Spec.LoadBalancerSourceRanges } - -func getTestSecurityGroup(az *Cloud, services ...v1.Service) *network.SecurityGroup { +func getTestSecurityGroupBackwardCompat(az *Cloud, services ...v1.Service) *network.SecurityGroup { rules := []network.SecurityRule{} for _, service := range services { @@ -1193,6 +1213,55 @@ func getTestSecurityGroup(az *Cloud, services ...v1.Service) *network.SecurityGr return &sg } +func getTestSecurityGroup(az *Cloud, services ...v1.Service) *network.SecurityGroup { + rules := []network.SecurityRule{} + + for _, service := range services { + for _, port := range service.Spec.Ports { + sources := getServiceSourceRanges(&service) + if useSharedSecurityRule(&service) { + for _, src := range sources { + ruleName := az.getSecurityRuleName(&service, port, src) + rules = append(rules, network.SecurityRule{ + Name: to.StringPtr(ruleName), + SecurityRulePropertiesFormat: &network.SecurityRulePropertiesFormat{ + SourceAddressPrefix: to.StringPtr(src), + DestinationPortRange: to.StringPtr(fmt.Sprintf("%d", port.Port)), + }, + }) + } + + } else { + ruleName := az.getSecurityRuleName(&service, port, "") + rules = append(rules, network.SecurityRule{ + Name: to.StringPtr(ruleName), + SecurityRulePropertiesFormat: &network.SecurityRulePropertiesFormat{ + SourceAddressPrefixes: &sources, + DestinationPortRange: to.StringPtr(fmt.Sprintf("%d", port.Port)), + }, + }) + } + } + } + + sg := network.SecurityGroup{ + Name: &az.SecurityGroupName, + SecurityGroupPropertiesFormat: &network.SecurityGroupPropertiesFormat{ + SecurityRules: &rules, + }, + } + + ctx, cancel := getContextWithCancel() + defer cancel() + az.SecurityGroupsClient.CreateOrUpdate( + ctx, + az.ResourceGroup, + az.SecurityGroupName, + sg) + + return &sg +} + func validateLoadBalancer(t *testing.T, loadBalancer *network.LoadBalancer, services ...v1.Service) { az := getTestCloud() expectedRuleCount := 0 @@ -1366,7 +1435,6 @@ func securityRuleMatches(serviceSourceRange string, servicePort v1.ServicePort, ruleSource = &[]string{*securityRule.SourceAddressPrefix} } } - rulePorts := securityRule.DestinationPortRanges if rulePorts == nil || len(*rulePorts) == 0 { if securityRule.DestinationPortRange == nil { @@ -1384,9 +1452,11 @@ func securityRuleMatches(serviceSourceRange string, servicePort v1.ServicePort, ruleDestination = &[]string{*securityRule.DestinationAddressPrefix} } } - - if !contains(*ruleSource, serviceSourceRange) { - return fmt.Errorf("Rule does not contain source %s", serviceSourceRange) + sort.Strings(*ruleSource) + ruleSourceRange := strings.Join(*ruleSource, ",") + // both serviceSourceRange and ruleSourceRange should have been sorted in ascending order + if ruleSourceRange != serviceSourceRange { + return fmt.Errorf("Rule %s does not equal to service %s", ruleSourceRange, serviceSourceRange) } if !contains(*rulePorts, fmt.Sprintf("%d", servicePort.Port)) { @@ -1404,24 +1474,48 @@ func validateSecurityGroup(t *testing.T, securityGroup *network.SecurityGroup, s az := getTestCloud() seenRules := make(map[string]string) for _, svc := range services { - for _, wantedRule := range svc.Spec.Ports { + for _, port := range svc.Spec.Ports { sources := getServiceSourceRanges(&svc) - for _, source := range sources { - wantedRuleName := az.getSecurityRuleName(&svc, wantedRule, source) - seenRules[wantedRuleName] = wantedRuleName - foundRule := false - for _, actualRule := range *securityGroup.SecurityRules { - if strings.EqualFold(*actualRule.Name, wantedRuleName) { - err := securityRuleMatches(source, wantedRule, svc.Spec.LoadBalancerIP, actualRule) - if err != nil { - t.Errorf("Found matching security rule %q but properties were incorrect: %v", wantedRuleName, err) + if len(sources) > 0 { + if useSharedSecurityRule(&svc) { + for _, source := range sources { + wantedRuleName := az.getSecurityRuleName(&svc, port, source) + seenRules[wantedRuleName] = wantedRuleName + foundRule := false + for _, actualRule := range *securityGroup.SecurityRules { + if strings.EqualFold(*actualRule.Name, wantedRuleName) { + err := securityRuleMatches(source, port, svc.Spec.LoadBalancerIP, actualRule) + if err != nil { + t.Errorf("Found matching security rule %q but properties were incorrect: %v", wantedRuleName, err) + } + foundRule = true + break + } + } + if !foundRule { + t.Errorf("Expected security group rule but didn't find it: %q", wantedRuleName) } - foundRule = true - break } - } - if !foundRule { - t.Errorf("Expected security group rule but didn't find it: %q", wantedRuleName) + } else { + // not shared + wantedRuleName := az.getSecurityRuleName(&svc, port, "") + seenRules[wantedRuleName] = wantedRuleName + foundRule := false + for _, actualRule := range *securityGroup.SecurityRules { + if strings.EqualFold(*actualRule.Name, wantedRuleName) { + sort.Strings(sources) + svcSources := strings.Join(sources, ",") + err := securityRuleMatches(svcSources, port, svc.Spec.LoadBalancerIP, actualRule) + if err != nil { + t.Errorf("Found matching security rule %q but properties were incorrect: %v", wantedRuleName, err) + } + foundRule = true + break + } + } + if !foundRule { + t.Errorf("Expected security group rule but didn't find it: %q", wantedRuleName) + } } } } @@ -1872,7 +1966,7 @@ func TestIfServiceSpecifiesSharedRuleAndRuleExistsThenTheServicesPortAndAddressA SecurityRulePropertiesFormat: &network.SecurityRulePropertiesFormat{ Protocol: network.SecurityRuleProtocolTCP, SourcePortRange: to.StringPtr("*"), - SourceAddressPrefix: to.StringPtr("Internet"), + SourceAddressPrefixes: &[]string{"Internet"}, DestinationPortRange: to.StringPtr("80"), DestinationAddressPrefix: to.StringPtr("192.168.33.44"), Access: network.SecurityRuleAccessAllow, @@ -2050,7 +2144,7 @@ func TestIfServicesSpecifySharedRuleButDifferentSourceAddressesThenSeparateRules svc1 := getTestService("servicesr1", v1.ProtocolTCP, 80) svc1.Spec.LoadBalancerIP = "192.168.77.88" - svc1.Spec.LoadBalancerSourceRanges = []string{"192.168.12.0/24"} + svc1.Spec.LoadBalancerSourceRanges = []string{"192.168.12.0/24", "192.10.10.0/24"} svc1.Annotations[ServiceAnnotationSharedSecurityRule] = "true" svc2 := getTestService("servicesr2", v1.ProtocolTCP, 80) @@ -2058,7 +2152,10 @@ func TestIfServicesSpecifySharedRuleButDifferentSourceAddressesThenSeparateRules svc2.Spec.LoadBalancerSourceRanges = []string{"192.168.34.0/24"} svc2.Annotations[ServiceAnnotationSharedSecurityRule] = "true" - expectedRuleName1 := "shared-TCP-80-192.168.12.0_24" + // shared service should create separate rules for each source address + expectedRuleName11 := "shared-TCP-80-192.168.12.0_24" + expectedRuleName12 := "shared-TCP-80-192.10.10.0_24" + // only share outbound when source range match expectedRuleName2 := "shared-TCP-80-192.168.34.0_24" sg := getTestSecurityGroup(az) @@ -2075,9 +2172,14 @@ func TestIfServicesSpecifySharedRuleButDifferentSourceAddressesThenSeparateRules validateSecurityGroup(t, sg, svc1, svc2) - _, securityRule1, rule1Found := findSecurityRuleByName(*sg.SecurityRules, expectedRuleName1) + _, securityRule11, rule1Found := findSecurityRuleByName(*sg.SecurityRules, expectedRuleName11) if !rule1Found { - t.Fatalf("Expected security rule %q but it was not present", expectedRuleName1) + t.Fatalf("Expected security rule %q but it was not present", expectedRuleName11) + } + + _, securityRule12, rule1Found := findSecurityRuleByName(*sg.SecurityRules, expectedRuleName12) + if !rule1Found { + t.Fatalf("Expected security rule %q but it was not present", expectedRuleName12) } _, securityRule2, rule2Found := findSecurityRuleByName(*sg.SecurityRules, expectedRuleName2) @@ -2085,35 +2187,26 @@ func TestIfServicesSpecifySharedRuleButDifferentSourceAddressesThenSeparateRules t.Fatalf("Expected security rule %q but it was not present", expectedRuleName2) } - expectedDestinationIPCount1 := 1 - if len(*securityRule1.DestinationAddressPrefixes) != expectedDestinationIPCount1 { - t.Errorf("Shared rule %s should have had %d destination IP addresses but had %d", expectedRuleName1, expectedDestinationIPCount1, len(*securityRule1.DestinationAddressPrefixes)) + expectedDestinationIPCount11 := 1 + if len(*securityRule11.DestinationAddressPrefixes) != expectedDestinationIPCount11 { + t.Errorf("Shared rule %s should have had %d destination IP addresses but had %d", expectedRuleName11, expectedDestinationIPCount11, len(*securityRule11.DestinationAddressPrefixes)) } - err = securityRuleMatches(svc1.Spec.LoadBalancerSourceRanges[0], v1.ServicePort{Port: 80}, "192.168.77.88", securityRule1) + err = securityRuleMatches(svc1.Spec.LoadBalancerSourceRanges[0], v1.ServicePort{Port: 80}, "192.168.77.88", securityRule11) if err != nil { - t.Errorf("Shared rule %s did not match service IP: %v", expectedRuleName1, err) + t.Errorf("Shared rule %s did not match service: %v", expectedRuleName11, err) } - err = securityRuleMatches(svc2.Spec.LoadBalancerSourceRanges[0], v1.ServicePort{Port: 80}, "192.168.33.44", securityRule1) - if err == nil { - t.Errorf("Shared rule %s matched wrong service's port and IP", expectedRuleName1) - } - - expectedDestinationIPCount2 := 1 - if len(*securityRule2.DestinationAddressPrefixes) != expectedDestinationIPCount2 { - t.Errorf("Shared rule %s should have had %d destination IP addresses but had %d", expectedRuleName2, expectedDestinationIPCount2, len(*securityRule2.DestinationAddressPrefixes)) + err = securityRuleMatches(svc1.Spec.LoadBalancerSourceRanges[1], v1.ServicePort{Port: 80}, "192.168.77.88", securityRule12) + if err != nil { + t.Errorf("Shared rule %s did not match service: %v", expectedRuleName12, err) } err = securityRuleMatches(svc2.Spec.LoadBalancerSourceRanges[0], v1.ServicePort{Port: 80}, "192.168.33.44", securityRule2) if err != nil { - t.Errorf("Shared rule %s did not match service IP: %v", expectedRuleName2, err) + t.Errorf("Shared rule %s did not match service: %v", expectedRuleName12, err) } - err = securityRuleMatches(svc1.Spec.LoadBalancerSourceRanges[0], v1.ServicePort{Port: 80}, "192.168.77.88", securityRule2) - if err == nil { - t.Errorf("Shared rule %s matched wrong service's port and IP", expectedRuleName2) - } } func TestIfServicesSpecifySharedRuleButSomeAreOnDifferentPortsThenRulesAreSeparatedOrConsoliatedByPort(t *testing.T) { @@ -2308,6 +2401,16 @@ func TestIfSomeServicesShareARuleAndOneIsDeletedItIsRemovedFromTheRightRule(t *t validateSecurityGroup(t, sg, svc1, svc2, svc3) + _, securityRule13, rule13Found := findSecurityRuleByName(*sg.SecurityRules, expectedRuleName13) + if !rule13Found { + t.Fatalf("Expected security rule %q but it was not present", expectedRuleName13) + } + + expectedDestinationIPCount13 := 2 + if len(*securityRule13.DestinationAddressPrefixes) != expectedDestinationIPCount13 { + t.Errorf("Shared rule %s should have had %d destination IP addresses but had %d", expectedRuleName13, expectedDestinationIPCount13, len(*securityRule13.DestinationAddressPrefixes)) + } + sg, err = az.reconcileSecurityGroup(testClusterName, &svc1, to.StringPtr(svc1.Spec.LoadBalancerIP), false) if err != nil { t.Errorf("Unexpected error removing svc1: %q", err) @@ -2315,7 +2418,7 @@ func TestIfSomeServicesShareARuleAndOneIsDeletedItIsRemovedFromTheRightRule(t *t validateSecurityGroup(t, sg, svc2, svc3) - _, securityRule13, rule13Found := findSecurityRuleByName(*sg.SecurityRules, expectedRuleName13) + _, securityRule13, rule13Found = findSecurityRuleByName(*sg.SecurityRules, expectedRuleName13) if !rule13Found { t.Fatalf("Expected security rule %q but it was not present", expectedRuleName13) } @@ -2325,7 +2428,7 @@ func TestIfSomeServicesShareARuleAndOneIsDeletedItIsRemovedFromTheRightRule(t *t t.Fatalf("Expected security rule %q but it was not present", expectedRuleName2) } - expectedDestinationIPCount13 := 1 + expectedDestinationIPCount13 = 1 if len(*securityRule13.DestinationAddressPrefixes) != expectedDestinationIPCount13 { t.Errorf("Shared rule %s should have had %d destination IP addresses but had %d", expectedRuleName13, expectedDestinationIPCount13, len(*securityRule13.DestinationAddressPrefixes)) } @@ -2475,16 +2578,17 @@ func TestCanCombineSharedAndPrivateRulesInSameGroup(t *testing.T) { svc4 := getTestService("servicesr4", v1.ProtocolTCP, 4444) svc4.Spec.LoadBalancerIP = "192.168.22.33" + svc4.Spec.LoadBalancerSourceRanges = []string{"192.168.12.0/24", "192.10.10.0/24"} svc4.Annotations[ServiceAnnotationSharedSecurityRule] = "false" svc5 := getTestService("servicesr5", v1.ProtocolTCP, 8888) - svc5.Spec.LoadBalancerIP = "192.168.22.33" + svc5.Spec.LoadBalancerIP = "192.168.22.55" svc5.Annotations[ServiceAnnotationSharedSecurityRule] = "false" expectedRuleName13 := "shared-TCP-4444-Internet" expectedRuleName2 := "shared-TCP-8888-Internet" - expectedRuleName4 := az.getSecurityRuleName(&svc4, v1.ServicePort{Port: 4444, Protocol: v1.ProtocolTCP}, "Internet") - expectedRuleName5 := az.getSecurityRuleName(&svc5, v1.ServicePort{Port: 8888, Protocol: v1.ProtocolTCP}, "Internet") + expectedRuleName4 := az.getSecurityRuleName(&svc4, v1.ServicePort{Port: 4444, Protocol: v1.ProtocolTCP}, "") + expectedRuleName5 := az.getSecurityRuleName(&svc5, v1.ServicePort{Port: 8888, Protocol: v1.ProtocolTCP}, "") sg := getTestSecurityGroup(az) @@ -2510,7 +2614,7 @@ func TestCanCombineSharedAndPrivateRulesInSameGroup(t *testing.T) { sg, err = az.reconcileSecurityGroup(testClusterName, &svc5, to.StringPtr(svc5.Spec.LoadBalancerIP), true) if err != nil { - t.Errorf("Unexpected error adding svc4: %q", err) + t.Errorf("Unexpected error adding svc5: %q", err) } validateSecurityGroup(t, sg, svc1, svc2, svc3, svc4, svc5) @@ -2575,6 +2679,11 @@ func TestCanCombineSharedAndPrivateRulesInSameGroup(t *testing.T) { t.Errorf("Shared rule %s matched wrong (unshared) service's port and IP", expectedRuleName2) } + expectedSourceIPCount4 := 2 + if len(*securityRule4.SourceAddressPrefixes) != expectedSourceIPCount4 { + t.Errorf("Shared rule %s should have had %d destination IP addresses but had %d", expectedRuleName4, expectedSourceIPCount4, len(*securityRule4.SourceAddressPrefixes)) + } + if securityRule4.DestinationAddressPrefixes != nil { t.Errorf("Expected unshared rule %s to use single destination IP address but used collection", expectedRuleName4) } @@ -2587,6 +2696,11 @@ func TestCanCombineSharedAndPrivateRulesInSameGroup(t *testing.T) { } } + expectedSourceAddressCount4 := 2 + if len(*securityRule4.SourceAddressPrefixes) != expectedSourceAddressCount4 { + t.Errorf("Expected rule %s should have had %d source IP addresses but had %d", expectedRuleName4, expectedSourceAddressCount4, len(*securityRule4.SourceAddressPrefixes)) + } + if securityRule5.DestinationAddressPrefixes != nil { t.Errorf("Expected unshared rule %s to use single destination IP address but used collection", expectedRuleName5) } @@ -2633,6 +2747,67 @@ func TestCanCombineSharedAndPrivateRulesInSameGroup(t *testing.T) { if len(*securityRule13.DestinationAddressPrefixes) != expectedDestinationIPCount13 { t.Errorf("Shared rule %s should have had %d destination IP addresses but had %d", expectedRuleName13, expectedDestinationIPCount13, len(*securityRule13.DestinationAddressPrefixes)) } + +} + +func TestNotSharedServiceUpdate(t *testing.T) { + az := getTestCloud() + + svc1 := getTestService("servicesr1", v1.ProtocolTCP, 9000) + svc1.Spec.LoadBalancerIP = "192.168.77.88" + svc1.Spec.LoadBalancerSourceRanges = []string{"192.168.12.0/24", "192.10.10.0/24"} + svc1.Annotations[ServiceAnnotationSharedSecurityRule] = "false" + + expectedRuleName1 := az.getSecurityRuleName(&svc1, v1.ServicePort{Port: 9000, Protocol: v1.ProtocolTCP}, "") + + sg := getTestSecurityGroup(az) + + sg, err := az.reconcileSecurityGroup(testClusterName, &svc1, to.StringPtr(svc1.Spec.LoadBalancerIP), true) + if err != nil { + t.Errorf("Unexpected error adding svc1: %q", err) + } + + validateSecurityGroup(t, sg, svc1) + + expectedRuleCount := 1 + if len(*sg.SecurityRules) != expectedRuleCount { + t.Errorf("Expected security group to have %d rules but it had %d", expectedRuleCount, len(*sg.SecurityRules)) + } + _, securityRule1, rule1Found := findSecurityRuleByName(*sg.SecurityRules, expectedRuleName1) + if !rule1Found { + t.Fatalf("Expected security rule %q but it was not present", expectedRuleName1) + } + if securityRule1.DestinationAddressPrefixes != nil { + t.Errorf("Expected unshared rule %s to use single destination IP address but used collection", expectedRuleName1) + } + expectedSourceAddressCount := 2 + if len(*securityRule1.SourceAddressPrefixes) != expectedSourceAddressCount { + t.Errorf("Expected unshared rule %s to have %d source addresses but it has %d", expectedRuleName1, expectedSourceAddressCount, len(*securityRule1.SourceAddressPrefixes)) + } + + existingPriority := securityRule1.Priority + + svc1.Spec.LoadBalancerSourceRanges = []string{"192.168.12.0/24"} + + sg, err = az.reconcileSecurityGroup(testClusterName, &svc1, to.StringPtr(svc1.Spec.LoadBalancerIP), true) + if err != nil { + t.Errorf("Unexpected error adding svc1: %q", err) + } + + validateSecurityGroup(t, sg, svc1) + + _, securityRule1, rule1Found = findSecurityRuleByName(*sg.SecurityRules, expectedRuleName1) + if !rule1Found { + t.Fatalf("Expected security rule %q but it was not present", expectedRuleName1) + } + expectedSourceAddressCount = 1 + if len(*securityRule1.SourceAddressPrefixes) != expectedSourceAddressCount { + t.Errorf("Expected unshared rule %s to have %d source addresses but it has %d", expectedRuleName1, expectedSourceAddressCount, len(*securityRule1.SourceAddressPrefixes)) + } + + if securityRule1.Priority != existingPriority { + t.Errorf("Expected unshared rule %s to have priority %d but it has %d", expectedRuleName1, existingPriority, securityRule1.Priority) + } } // TODO: sanity check if the same IP address incorrectly gets put in twice?