diff --git a/pkg/cloudprovider/providers/azure/azure_loadbalancer.go b/pkg/cloudprovider/providers/azure/azure_loadbalancer.go index 3eb0984a56a..77bfdd20de2 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 } - sourceAddressPrefixes := []string{} + var sourceAddressPrefixes []string if (sourceRanges == nil || serviceapi.IsAllowAll(sourceRanges)) && len(serviceTags) == 0 { if !requiresInternalLoadBalancer(service) { sourceAddressPrefixes = []string{"Internet"} @@ -1006,51 +1006,35 @@ func (az *Cloud) reconcileSecurityGroup(clusterName string, service *v1.Service, } expectedSecurityRules := []network.SecurityRule{} - if wantLb && len(sourceAddressPrefixes) > 0 { - for _, port := range ports { + if wantLb { + expectedSecurityRules = make([]network.SecurityRule, len(ports)*len(sourceAddressPrefixes)) + + for i, port := range ports { _, securityProto, _, err := getProtocolsFromKubernetesProtocol(port.Protocol) if err != nil { return nil, err } - 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{ + for j := range sourceAddressPrefixes { + ix := i*len(sourceAddressPrefixes) + j + securityRuleName := az.getSecurityRuleName(service, port, sourceAddressPrefixes[j]) + expectedSecurityRules[ix] = network.SecurityRule{ Name: to.StringPtr(securityRuleName), SecurityRulePropertiesFormat: &network.SecurityRulePropertiesFormat{ Protocol: *securityProto, SourcePortRange: to.StringPtr("*"), DestinationPortRange: to.StringPtr(strconv.Itoa(int(port.Port))), - SourceAddressPrefixes: &sourceAddressPrefixes, + SourceAddressPrefix: to.StringPtr(sourceAddressPrefixes[j]), 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, logSafeCollection(r.SourceAddressPrefix, r.SourceAddressPrefixes), *r.SourcePortRange, *r.DestinationAddressPrefix, *r.DestinationPortRange) + klog.V(10).Infof("Expecting security rule for %s: %s:%s -> %s:%s", service.Name, *r.SourceAddressPrefix, *r.SourcePortRange, *r.DestinationAddressPrefix, *r.DestinationPortRange) } // update security rules @@ -1061,7 +1045,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, logSafeCollection(r.SourceAddressPrefix, r.SourceAddressPrefixes), 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, logSafe(r.SourceAddressPrefix), logSafe(r.SourcePortRange), logSafeCollection(r.DestinationAddressPrefix, r.DestinationAddressPrefixes), logSafe(r.DestinationPortRange)) } // update security rules: remove unwanted rules that belong privately @@ -1090,14 +1074,17 @@ 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 { @@ -1131,26 +1118,10 @@ 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) @@ -1166,7 +1137,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, logSafeCollection(r.SourceAddressPrefix, r.SourceAddressPrefixes), 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, logSafe(r.SourceAddressPrefix), logSafe(r.SourcePortRange), logSafeCollection(r.DestinationAddressPrefix, r.DestinationAddressPrefixes), logSafe(r.DestinationPortRange)) } if dirtySg { @@ -1242,25 +1213,6 @@ 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, @@ -1293,8 +1245,8 @@ func consolidate(existingRule network.SecurityRule, newRule network.SecurityRule SourcePortRanges: existingRule.SourcePortRanges, DestinationPortRange: existingRule.DestinationPortRange, DestinationPortRanges: existingRule.DestinationPortRanges, - SourceAddressPrefix: newRule.SourceAddressPrefix, - SourceAddressPrefixes: newRule.SourceAddressPrefixes, + SourceAddressPrefix: existingRule.SourceAddressPrefix, + SourceAddressPrefixes: existingRule.SourceAddressPrefixes, DestinationAddressPrefixes: destinations, Access: existingRule.Access, Direction: existingRule.Direction, @@ -1521,19 +1473,12 @@ 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 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. +// 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 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 !strings.EqualFold(to.String(existingRule.Name), to.String(rule.Name)) { + continue } if existingRule.Protocol != rule.Protocol { continue @@ -1544,6 +1489,9 @@ 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 a33c6d57e6e..95126f58b0f 100644 --- a/pkg/cloudprovider/providers/azure/azure_standard.go +++ b/pkg/cloudprovider/providers/azure/azure_standard.go @@ -236,11 +236,6 @@ 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 4a54512145d..0369d59a53c 100644 --- a/pkg/cloudprovider/providers/azure/azure_test.go +++ b/pkg/cloudprovider/providers/azure/azure_test.go @@ -23,7 +23,6 @@ import ( "math" "net" "net/http" - "sort" "strings" "testing" @@ -853,26 +852,6 @@ 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) { @@ -1176,7 +1155,8 @@ func getServiceSourceRanges(service *v1.Service) []string { return service.Spec.LoadBalancerSourceRanges } -func getTestSecurityGroupBackwardCompat(az *Cloud, services ...v1.Service) *network.SecurityGroup { + +func getTestSecurityGroup(az *Cloud, services ...v1.Service) *network.SecurityGroup { rules := []network.SecurityRule{} for _, service := range services { @@ -1213,55 +1193,6 @@ func getTestSecurityGroupBackwardCompat(az *Cloud, services ...v1.Service) *netw 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 @@ -1435,6 +1366,7 @@ func securityRuleMatches(serviceSourceRange string, servicePort v1.ServicePort, ruleSource = &[]string{*securityRule.SourceAddressPrefix} } } + rulePorts := securityRule.DestinationPortRanges if rulePorts == nil || len(*rulePorts) == 0 { if securityRule.DestinationPortRange == nil { @@ -1452,11 +1384,9 @@ func securityRuleMatches(serviceSourceRange string, servicePort v1.ServicePort, ruleDestination = &[]string{*securityRule.DestinationAddressPrefix} } } - 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(*ruleSource, serviceSourceRange) { + return fmt.Errorf("Rule does not contain source %s", serviceSourceRange) } if !contains(*rulePorts, fmt.Sprintf("%d", servicePort.Port)) { @@ -1474,48 +1404,24 @@ func validateSecurityGroup(t *testing.T, securityGroup *network.SecurityGroup, s az := getTestCloud() seenRules := make(map[string]string) for _, svc := range services { - for _, port := range svc.Spec.Ports { + for _, wantedRule := range svc.Spec.Ports { sources := getServiceSourceRanges(&svc) - 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) + 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) } + foundRule = true + break } - } 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) - } + } + if !foundRule { + t.Errorf("Expected security group rule but didn't find it: %q", wantedRuleName) } } } @@ -1966,7 +1872,7 @@ func TestIfServiceSpecifiesSharedRuleAndRuleExistsThenTheServicesPortAndAddressA SecurityRulePropertiesFormat: &network.SecurityRulePropertiesFormat{ Protocol: network.SecurityRuleProtocolTCP, SourcePortRange: to.StringPtr("*"), - SourceAddressPrefixes: &[]string{"Internet"}, + SourceAddressPrefix: to.StringPtr("Internet"), DestinationPortRange: to.StringPtr("80"), DestinationAddressPrefix: to.StringPtr("192.168.33.44"), Access: network.SecurityRuleAccessAllow, @@ -2144,7 +2050,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", "192.10.10.0/24"} + svc1.Spec.LoadBalancerSourceRanges = []string{"192.168.12.0/24"} svc1.Annotations[ServiceAnnotationSharedSecurityRule] = "true" svc2 := getTestService("servicesr2", v1.ProtocolTCP, 80) @@ -2152,10 +2058,7 @@ func TestIfServicesSpecifySharedRuleButDifferentSourceAddressesThenSeparateRules svc2.Spec.LoadBalancerSourceRanges = []string{"192.168.34.0/24"} svc2.Annotations[ServiceAnnotationSharedSecurityRule] = "true" - // 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 + expectedRuleName1 := "shared-TCP-80-192.168.12.0_24" expectedRuleName2 := "shared-TCP-80-192.168.34.0_24" sg := getTestSecurityGroup(az) @@ -2172,14 +2075,9 @@ func TestIfServicesSpecifySharedRuleButDifferentSourceAddressesThenSeparateRules validateSecurityGroup(t, sg, svc1, svc2) - _, securityRule11, rule1Found := findSecurityRuleByName(*sg.SecurityRules, expectedRuleName11) + _, securityRule1, rule1Found := findSecurityRuleByName(*sg.SecurityRules, expectedRuleName1) if !rule1Found { - 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) + t.Fatalf("Expected security rule %q but it was not present", expectedRuleName1) } _, securityRule2, rule2Found := findSecurityRuleByName(*sg.SecurityRules, expectedRuleName2) @@ -2187,26 +2085,35 @@ func TestIfServicesSpecifySharedRuleButDifferentSourceAddressesThenSeparateRules t.Fatalf("Expected security rule %q but it was not present", expectedRuleName2) } - 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)) + 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)) } - err = securityRuleMatches(svc1.Spec.LoadBalancerSourceRanges[0], v1.ServicePort{Port: 80}, "192.168.77.88", securityRule11) + err = securityRuleMatches(svc1.Spec.LoadBalancerSourceRanges[0], v1.ServicePort{Port: 80}, "192.168.77.88", securityRule1) if err != nil { - t.Errorf("Shared rule %s did not match service: %v", expectedRuleName11, err) + t.Errorf("Shared rule %s did not match service IP: %v", expectedRuleName1, err) } - 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", 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(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: %v", expectedRuleName12, err) + t.Errorf("Shared rule %s did not match service IP: %v", expectedRuleName2, 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) { @@ -2401,16 +2308,6 @@ 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) @@ -2418,7 +2315,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) } @@ -2428,7 +2325,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)) } @@ -2578,17 +2475,16 @@ 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.55" + svc5.Spec.LoadBalancerIP = "192.168.22.33" 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}, "") - expectedRuleName5 := az.getSecurityRuleName(&svc5, v1.ServicePort{Port: 8888, Protocol: v1.ProtocolTCP}, "") + expectedRuleName4 := az.getSecurityRuleName(&svc4, v1.ServicePort{Port: 4444, Protocol: v1.ProtocolTCP}, "Internet") + expectedRuleName5 := az.getSecurityRuleName(&svc5, v1.ServicePort{Port: 8888, Protocol: v1.ProtocolTCP}, "Internet") sg := getTestSecurityGroup(az) @@ -2614,7 +2510,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 svc5: %q", err) + t.Errorf("Unexpected error adding svc4: %q", err) } validateSecurityGroup(t, sg, svc1, svc2, svc3, svc4, svc5) @@ -2679,11 +2575,6 @@ 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) } @@ -2696,11 +2587,6 @@ 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) } @@ -2747,67 +2633,6 @@ 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?