Revert "Collapse not shared NSG rules with multiple source ranges"

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

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)

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)
-	if !rule1Found {
-		t.Fatalf("Expected security rule %q but it was not present", expectedRuleName11)
-	}
-
-	_, securityRule12, rule1Found := findSecurityRuleByName(*sg.SecurityRules, expectedRuleName12)
+	_, securityRule1, rule1Found := findSecurityRuleByName(*sg.SecurityRules, expectedRuleName1)
 	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?