Collapse source range in nsg

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 {
+	if wantLb && len(sourceAddressPrefixes) > 0 {
-		expectedSecurityRules = make([]network.SecurityRule, len(ports)*len(sourceAddressPrefixes))
+		for _, port := range ports {
-
-		for i, port := range ports {
 			_, securityProto, _, err := getProtocolsFromKubernetesProtocol(port.Protocol)
 			if err != nil {
 				return nil, err
 			}
-			for j := range sourceAddressPrefixes {
+			if useSharedSecurityRule(service) {
-				ix := i*len(sourceAddressPrefixes) + j
+				// When service is shared, a separate rule is created for each element in sourceAddressPrefixes
-				securityRuleName := az.getSecurityRuleName(service, port, sourceAddressPrefixes[j])
+				for j := range sourceAddressPrefixes {
-				expectedSecurityRules[ix] = network.SecurityRule{
+					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,
+			SourceAddressPrefix:        newRule.SourceAddressPrefix,
-			SourceAddressPrefixes:      existingRule.SourceAddressPrefixes,
+			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,
+// We intentionally do not compare SourceAddressPrefixes and 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.
+// 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)) {
+		if allowsConsolidation(existingRule) && allowsConsolidation(rule) {
-			continue
+			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

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)

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 getTestSecurityGroupBackwardCompat(az *Cloud, services ...v1.Service) *network.SecurityGroup {
-func getTestSecurityGroup(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}
 		}
 	}
-
+	sort.Strings(*ruleSource)
-	if !contains(*ruleSource, serviceSourceRange) {
+	ruleSourceRange := strings.Join(*ruleSource, ",")
-		return fmt.Errorf("Rule does not contain source %s", serviceSourceRange)
+	// 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 {
+			if len(sources) > 0 {
-				wantedRuleName := az.getSecurityRuleName(&svc, wantedRule, source)
+				if useSharedSecurityRule(&svc) {
-				seenRules[wantedRuleName] = wantedRuleName
+					for _, source := range sources {
-				foundRule := false
+						wantedRuleName := az.getSecurityRuleName(&svc, port, source)
-				for _, actualRule := range *securityGroup.SecurityRules {
+						seenRules[wantedRuleName] = wantedRuleName
-					if strings.EqualFold(*actualRule.Name, wantedRuleName) {
+						foundRule := false
-						err := securityRuleMatches(source, wantedRule, svc.Spec.LoadBalancerIP, actualRule)
+						for _, actualRule := range *securityGroup.SecurityRules {
-						if err != nil {
+							if strings.EqualFold(*actualRule.Name, wantedRuleName) {
-							t.Errorf("Found matching security rule %q but properties were incorrect: %v", wantedRuleName, err)
+								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
 					}
-				}
+				} else {
-				if !foundRule {
+					// not shared
-					t.Errorf("Expected security group rule but didn't find it: %q", wantedRuleName)
+					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
+	expectedDestinationIPCount11 := 1
-	if len(*securityRule1.DestinationAddressPrefixes) != expectedDestinationIPCount1 {
+	if len(*securityRule11.DestinationAddressPrefixes) != expectedDestinationIPCount11 {
-		t.Errorf("Shared rule %s should have had %d destination IP addresses but had %d", expectedRuleName1, expectedDestinationIPCount1, len(*securityRule1.DestinationAddressPrefixes))
+		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)
+	err = securityRuleMatches(svc1.Spec.LoadBalancerSourceRanges[1], v1.ServicePort{Port: 80}, "192.168.77.88", securityRule12)
-	if err == nil {
+	if err != nil {
-		t.Errorf("Shared rule %s matched wrong service's port and IP", expectedRuleName1)
+		t.Errorf("Shared rule %s did not match service: %v", expectedRuleName12, err)
-	}
-
-	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 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")
+	expectedRuleName4 := az.getSecurityRuleName(&svc4, v1.ServicePort{Port: 4444, Protocol: v1.ProtocolTCP}, "")
-	expectedRuleName5 := az.getSecurityRuleName(&svc5, v1.ServicePort{Port: 8888, Protocol: v1.ProtocolTCP}, "Internet")
+	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?