From 422dac5d9be0efc59f8918293a282eb8c72163ca Mon Sep 17 00:00:00 2001 From: itowlson Date: Fri, 17 Nov 2017 14:05:51 +1300 Subject: [PATCH] Option to consolidate Azure NSG rules for services (#13) * Option to consolidate Azure NSG rules for services * Fixed panic checking for service on other Azure LB --- .../providers/azure/azure_loadbalancer.go | 336 ++++++- .../providers/azure/azure_test.go | 903 +++++++++++++++++- .../providers/azure/azure_util.go | 4 + 3 files changed, 1198 insertions(+), 45 deletions(-) diff --git a/pkg/cloudprovider/providers/azure/azure_loadbalancer.go b/pkg/cloudprovider/providers/azure/azure_loadbalancer.go index 41d44e9a7b5..a4eee6e7d6e 100644 --- a/pkg/cloudprovider/providers/azure/azure_loadbalancer.go +++ b/pkg/cloudprovider/providers/azure/azure_loadbalancer.go @@ -59,6 +59,14 @@ const ServiceAnnotationLoadBalancerAutoModeValue = "__auto__" // ServiceAnnotationDNSLabelName annotation speficying the DNS label name for the service. const ServiceAnnotationDNSLabelName = "service.beta.kubernetes.io/azure-dns-label-name" +// ServiceAnnotationSharedSecurityRule is the annotation used on the service +// to specify that the service should be exposed using an Azure security rule +// that may be shared with other service, trading specificity of rules for an +// increase in the number of services that can be exposed. This relies on the +// Azure "augmented security rules" feature which at the time of writing is in +// preview and available only in certain regions. +const ServiceAnnotationSharedSecurityRule = "service.beta.kubernetes.io/azure-shared-securityrule" + // GetLoadBalancer returns whether the specified load balancer exists, and // if so, what its status is. func (az *Cloud) GetLoadBalancer(clusterName string, service *v1.Service) (status *v1.LoadBalancerStatus, exists bool, err error) { @@ -107,7 +115,12 @@ func (az *Cloud) EnsureLoadBalancer(clusterName string, service *v1.Service, nod return nil, err } - if _, err := az.reconcileSecurityGroup(clusterName, service, lbStatus, true /* wantLb */); err != nil { + var serviceIP *string + if lbStatus != nil && len(lbStatus.Ingress) > 0 { + serviceIP = &lbStatus.Ingress[0].IP + } + glog.V(10).Infof("Calling reconcileSecurityGroup from EnsureLoadBalancer for %s with IP %s, wantLb = true", service.Name, logSafe(serviceIP)) + if _, err := az.reconcileSecurityGroup(clusterName, service, serviceIP, true /* wantLb */); err != nil { return nil, err } @@ -127,9 +140,17 @@ func (az *Cloud) UpdateLoadBalancer(clusterName string, service *v1.Service, nod // have multiple underlying components, meaning a Get could say that the LB // doesn't exist even if some part of it is still laying around. func (az *Cloud) EnsureLoadBalancerDeleted(clusterName string, service *v1.Service) error { + isInternal := requiresInternalLoadBalancer(service) serviceName := getServiceName(service) glog.V(5).Infof("delete(%s): START clusterName=%q", serviceName, clusterName) - if _, err := az.reconcileSecurityGroup(clusterName, service, nil, false /* wantLb */); err != nil { + + serviceIPToCleanup, err := az.findServiceIPAddress(clusterName, service, isInternal) + if err != nil { + return err + } + + glog.V(10).Infof("Calling reconcileSecurityGroup from EnsureLoadBalancerDeleted for %s with IP %s, wantLb = false", service.Name, serviceIPToCleanup) + if _, err := az.reconcileSecurityGroup(clusterName, service, &serviceIPToCleanup, false /* wantLb */); err != nil { return err } @@ -331,6 +352,9 @@ func (az *Cloud) determinePublicIPName(clusterName string, service *v1.Service) func flipServiceInternalAnnotation(service *v1.Service) *v1.Service { copyService := service.DeepCopy() + if copyService.Annotations == nil { + copyService.Annotations = map[string]string{} + } if v, ok := copyService.Annotations[ServiceAnnotationLoadBalancerInternal]; ok && v == "true" { // If it is internal now, we make it external by remove the annotation delete(copyService.Annotations, ServiceAnnotationLoadBalancerInternal) @@ -341,6 +365,25 @@ func flipServiceInternalAnnotation(service *v1.Service) *v1.Service { return copyService } +func (az *Cloud) findServiceIPAddress(clusterName string, service *v1.Service, isInternalLb bool) (string, error) { + if len(service.Spec.LoadBalancerIP) > 0 { + return service.Spec.LoadBalancerIP, nil + } + + lbStatus, existsLb, err := az.GetLoadBalancer(clusterName, service) + if err != nil { + return "", err + } + if !existsLb { + return "", fmt.Errorf("Expected to find an IP address for service %s but did not", service.Name) + } + if len(lbStatus.Ingress) < 1 { + return "", fmt.Errorf("Expected to find an IP address for service %s but it had no ingresses", service.Name) + } + + return lbStatus.Ingress[0].IP, nil +} + func (az *Cloud) ensurePublicIPExists(serviceName, pipName, domainNameLabel string) (*network.PublicIPAddress, error) { pip, existsPip, err := az.getPublicIPAddress(pipName) if err != nil { @@ -744,16 +787,19 @@ func (az *Cloud) reconcileLoadBalancer(clusterName string, service *v1.Service, // This reconciles the Network Security Group similar to how the LB is reconciled. // This entails adding required, missing SecurityRules and removing stale rules. -func (az *Cloud) reconcileSecurityGroup(clusterName string, service *v1.Service, lbStatus *v1.LoadBalancerStatus, wantLb bool) (*network.SecurityGroup, error) { +func (az *Cloud) reconcileSecurityGroup(clusterName string, service *v1.Service, lbIP *string, wantLb bool) (*network.SecurityGroup, error) { serviceName := getServiceName(service) - glog.V(5).Infof("ensure(%s): START clusterName=%q lbName=%q", serviceName, clusterName) + glog.V(5).Infof("reconcileSecurityGroup(%s): START clusterName=%q lbName=%q", serviceName, clusterName) - var ports []v1.ServicePort - if wantLb { - ports = service.Spec.Ports - } else { + ports := service.Spec.Ports + if ports == nil { + if useSharedSecurityRule(service) { + glog.V(2).Infof("Attempting to reconcile security group for service %s, but service uses shared rule and we don't know which port it's for", service.Name) + return nil, fmt.Errorf("No port info for reconciling shared rule for service %s", service.Name) + } ports = []v1.ServicePort{} } + az.operationPollRateLimiter.Accept() glog.V(10).Infof("SecurityGroupsClient.Get(%q): start", az.SecurityGroupName) sg, err := az.SecurityGroupsClient.Get(az.ResourceGroup, az.SecurityGroupName, "") @@ -763,12 +809,10 @@ func (az *Cloud) reconcileSecurityGroup(clusterName string, service *v1.Service, } destinationIPAddress := "" - if wantLb { - // Get lbIP since we make up NSG rules based on ingress IP - lbIP := &lbStatus.Ingress[0].IP - if lbIP == nil { - return nil, fmt.Errorf("No load balancer IP for setting up security rules for service %s", service.Name) - } + if wantLb && lbIP == nil { + return nil, fmt.Errorf("No load balancer IP for setting up security rules for service %s", service.Name) + } + if lbIP != nil { destinationIPAddress = *lbIP } if destinationIPAddress == "" { @@ -789,38 +833,52 @@ func (az *Cloud) reconcileSecurityGroup(clusterName string, service *v1.Service, sourceAddressPrefixes = append(sourceAddressPrefixes, ip.String()) } } - expectedSecurityRules := make([]network.SecurityRule, len(ports)*len(sourceAddressPrefixes)) + expectedSecurityRules := []network.SecurityRule{} - for i, port := range ports { - _, securityProto, _, err := getProtocolsFromKubernetesProtocol(port.Protocol) - if err != nil { - return nil, err - } - for j := range sourceAddressPrefixes { - ix := i*len(sourceAddressPrefixes) + j - securityRuleName := 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))), - SourceAddressPrefix: to.StringPtr(sourceAddressPrefixes[j]), - DestinationAddressPrefix: to.StringPtr(destinationIPAddress), - Access: network.SecurityRuleAccessAllow, - Direction: network.SecurityRuleDirectionInbound, - }, + 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 + } + for j := range sourceAddressPrefixes { + ix := i*len(sourceAddressPrefixes) + j + securityRuleName := 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))), + SourceAddressPrefix: to.StringPtr(sourceAddressPrefixes[j]), + DestinationAddressPrefix: to.StringPtr(destinationIPAddress), + Access: network.SecurityRuleAccessAllow, + Direction: network.SecurityRuleDirectionInbound, + }, + } } } } + for _, r := range expectedSecurityRules { + glog.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 dirtySg := false var updatedRules []network.SecurityRule if sg.SecurityRules != nil { updatedRules = *sg.SecurityRules } - // update security rules: remove unwanted + + for _, r := range updatedRules { + glog.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 + // to this service for i := len(updatedRules) - 1; i >= 0; i-- { existingRule := updatedRules[i] if serviceOwnsRule(service, *existingRule.Name) { @@ -837,6 +895,50 @@ func (az *Cloud) reconcileSecurityGroup(clusterName string, service *v1.Service, } } } + // update security rules: if the service uses a shared rule and is being deleted, + // then remove it from the shared rule + if useSharedSecurityRule(service) && !wantLb { + for _, port := range ports { + for _, sourceAddressPrefix := range sourceAddressPrefixes { + sharedRuleName := getSecurityRuleName(service, port, sourceAddressPrefix) + sharedIndex, sharedRule, sharedRuleFound := findSecurityRuleByName(updatedRules, sharedRuleName) + if !sharedRuleFound { + glog.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 { + glog.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 { + glog.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 { + updatedRules = append(updatedRules[:sharedIndex], updatedRules[sharedIndex+1:]...) + } else { + newDestinations := append(existingPrefixes[:addressIndex], existingPrefixes[addressIndex+1:]...) + sharedRule.DestinationAddressPrefixes = &newDestinations + updatedRules[sharedIndex] = sharedRule + } + dirtySg = true + } + } + } + + // update security rules: prepare rules for consolidation + for index, rule := range updatedRules { + if allowsConsolidation(rule) { + updatedRules[index] = makeConsolidatable(rule) + } + } + for index, rule := range expectedSecurityRules { + if allowsConsolidation(rule) { + expectedSecurityRules[index] = makeConsolidatable(rule) + } + } // update security rules: add needed for _, expectedRule := range expectedSecurityRules { foundRule := false @@ -844,6 +946,11 @@ func (az *Cloud) reconcileSecurityGroup(clusterName string, service *v1.Service, glog.V(10).Infof("reconcile(%s)(%t): sg rule(%s) - already exists", serviceName, wantLb, *expectedRule.Name) foundRule = true } + if foundRule && allowsConsolidation(expectedRule) { + index, _ := findConsolidationCandidate(updatedRules, expectedRule) + updatedRules[index] = consolidate(updatedRules[index], expectedRule) + dirtySg = true + } if !foundRule { glog.V(10).Infof("reconcile(%s)(%t): sg rule(%s) - adding", serviceName, wantLb, *expectedRule.Name) @@ -857,6 +964,11 @@ func (az *Cloud) reconcileSecurityGroup(clusterName string, service *v1.Service, dirtySg = true } } + + for _, r := range updatedRules { + glog.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 { sg.SecurityRules = &updatedRules glog.V(3).Infof("ensure(%s): sg(%s) - updating", serviceName, *sg.Name) @@ -865,6 +977,14 @@ func (az *Cloud) reconcileSecurityGroup(clusterName string, service *v1.Service, err := az.CreateOrUpdateSGWithRetry(sg) if err != nil { glog.V(2).Infof("ensure(%s) abort backoff: sg(%s) - updating", serviceName, *sg.Name) + // TODO (Nov 2017): remove when augmented security rules are out of preview + // we could try to parse the response but it's not worth it for bridging a preview + errorDescription := err.Error() + if strings.Contains(errorDescription, "SubscriptionNotRegisteredForFeature") && strings.Contains(errorDescription, "Microsoft.Network/AllowAccessRuleExtendedProperties") { + sharedRuleError := fmt.Errorf("Shared security rules are not available in this Azure region. Details: %v", errorDescription) + return nil, sharedRuleError + } + // END TODO return nil, err } glog.V(10).Infof("CreateOrUpdateSGWithRetry(%q): end", *sg.Name) @@ -872,6 +992,144 @@ func (az *Cloud) reconcileSecurityGroup(clusterName string, service *v1.Service, return &sg, nil } +func logSafe(s *string) string { + if s == nil { + return "(nil)" + } + return *s +} + +func logSafeCollection(s *string, strs *[]string) string { + if s == nil { + if strs == nil { + return "(nil)" + } + return "[" + strings.Join(*strs, ",") + "]" + } + return *s +} + +func findSecurityRuleByName(rules []network.SecurityRule, ruleName string) (int, network.SecurityRule, bool) { + for index, rule := range rules { + if rule.Name != nil && strings.EqualFold(*rule.Name, ruleName) { + return index, rule, true + } + } + return 0, network.SecurityRule{}, false +} + +func findIndex(strs []string, s string) (int, bool) { + for index, str := range strs { + if strings.EqualFold(str, s) { + return index, true + } + } + return 0, false +} + +func allowsConsolidation(rule network.SecurityRule) bool { + return strings.HasPrefix(*rule.Name, "shared") +} + +func findConsolidationCandidate(rules []network.SecurityRule, rule network.SecurityRule) (int, bool) { + for index, r := range rules { + if allowsConsolidation(r) { + if strings.EqualFold(*r.Name, *rule.Name) { + return index, true + } + } + } + + return 0, false +} + +func makeConsolidatable(rule network.SecurityRule) network.SecurityRule { + return network.SecurityRule{ + Name: rule.Name, + SecurityRulePropertiesFormat: &network.SecurityRulePropertiesFormat{ + Priority: rule.Priority, + Protocol: rule.Protocol, + SourcePortRange: rule.SourcePortRange, + SourcePortRanges: rule.SourcePortRanges, + DestinationPortRange: rule.DestinationPortRange, + DestinationPortRanges: rule.DestinationPortRanges, + SourceAddressPrefix: rule.SourceAddressPrefix, + SourceAddressPrefixes: rule.SourceAddressPrefixes, + DestinationAddressPrefixes: collectionOrSingle(rule.DestinationAddressPrefixes, rule.DestinationAddressPrefix), + Access: rule.Access, + Direction: rule.Direction, + }, + } +} + +func consolidate(existingRule network.SecurityRule, newRule network.SecurityRule) network.SecurityRule { + destinations := appendElements(existingRule.SecurityRulePropertiesFormat.DestinationAddressPrefixes, newRule.DestinationAddressPrefix, newRule.DestinationAddressPrefixes) + destinations = deduplicate(destinations) // there are transient conditions during controller startup where it tries to add a service that is already added + + return network.SecurityRule{ + Name: existingRule.Name, + SecurityRulePropertiesFormat: &network.SecurityRulePropertiesFormat{ + Priority: existingRule.Priority, + Protocol: existingRule.Protocol, + SourcePortRange: existingRule.SourcePortRange, + SourcePortRanges: existingRule.SourcePortRanges, + DestinationPortRange: existingRule.DestinationPortRange, + DestinationPortRanges: existingRule.DestinationPortRanges, + SourceAddressPrefix: existingRule.SourceAddressPrefix, + SourceAddressPrefixes: existingRule.SourceAddressPrefixes, + DestinationAddressPrefixes: destinations, + Access: existingRule.Access, + Direction: existingRule.Direction, + }, + } +} + +func collectionOrSingle(collection *[]string, s *string) *[]string { + if collection != nil && len(*collection) > 0 { + return collection + } + if s == nil { + return &[]string{} + } + return &[]string{*s} +} + +func appendElements(collection *[]string, appendString *string, appendStrings *[]string) *[]string { + newCollection := []string{} + + if collection != nil { + newCollection = append(newCollection, *collection...) + } + if appendString != nil { + newCollection = append(newCollection, *appendString) + } + if appendStrings != nil { + newCollection = append(newCollection, *appendStrings...) + } + + return &newCollection +} + +func deduplicate(collection *[]string) *[]string { + if collection == nil { + return nil + } + + seen := map[string]bool{} + result := make([]string, 0, len(*collection)) + + for _, v := range *collection { + if seen[v] == true { + // skip this element + } else { + seen[v] = true + result = append(result, v) + } + } + + return &result +} + // This reconciles the PublicIP resources similar to how the LB is reconciled. func (az *Cloud) reconcilePublicIP(clusterName string, service *v1.Service, wantLb bool) (*network.PublicIPAddress, error) { isInternal := requiresInternalLoadBalancer(service) @@ -1087,3 +1345,11 @@ func getServiceLoadBalancerMode(service *v1.Service) (hasMode bool, isAuto bool, return hasMode, isAuto, availabilitySetNames } + +func useSharedSecurityRule(service *v1.Service) bool { + if l, ok := service.Annotations[ServiceAnnotationSharedSecurityRule]; ok { + return l == "true" + } + + return false +} diff --git a/pkg/cloudprovider/providers/azure/azure_test.go b/pkg/cloudprovider/providers/azure/azure_test.go index 811cdec45cc..db6fac8d3f7 100644 --- a/pkg/cloudprovider/providers/azure/azure_test.go +++ b/pkg/cloudprovider/providers/azure/azure_test.go @@ -660,7 +660,7 @@ func TestReconcileSecurityGroupNewServiceAddsPort(t *testing.T) { lb, _ := az.reconcileLoadBalancer(testClusterName, &svc1, clusterResources.nodes, true) lbStatus, _ := az.getServiceLoadBalancerStatus(&svc1, lb) - sg, err := az.reconcileSecurityGroup(testClusterName, &svc1, lbStatus, true /* wantLb */) + sg, err := az.reconcileSecurityGroup(testClusterName, &svc1, &lbStatus.Ingress[0].IP, true /* wantLb */) if err != nil { t.Errorf("Unexpected error: %q", err) } @@ -677,7 +677,7 @@ func TestReconcileSecurityGroupNewInternalServiceAddsPort(t *testing.T) { lb, _ := az.reconcileLoadBalancer(testClusterName, &svc1, clusterResources.nodes, true) lbStatus, _ := az.getServiceLoadBalancerStatus(&svc1, lb) - sg, err := az.reconcileSecurityGroup(testClusterName, &svc1, lbStatus, true /* wantLb */) + sg, err := az.reconcileSecurityGroup(testClusterName, &svc1, &lbStatus.Ingress[0].IP, true /* wantLb */) if err != nil { t.Errorf("Unexpected error: %q", err) } @@ -699,7 +699,7 @@ func TestReconcileSecurityGroupRemoveService(t *testing.T) { sg := getTestSecurityGroup(az, service1, service2) validateSecurityGroup(t, sg, service1, service2) - sg, err := az.reconcileSecurityGroup(testClusterName, &service1, lbStatus, false /* wantLb */) + sg, err := az.reconcileSecurityGroup(testClusterName, &service1, &lbStatus.Ingress[0].IP, false /* wantLb */) if err != nil { t.Errorf("Unexpected error: %q", err) } @@ -717,7 +717,7 @@ func TestReconcileSecurityGroupRemoveServiceRemovesPort(t *testing.T) { lb, _ := az.reconcileLoadBalancer(testClusterName, &svc, clusterResources.nodes, true) lbStatus, _ := az.getServiceLoadBalancerStatus(&svc, lb) - sg, err := az.reconcileSecurityGroup(testClusterName, &svcUpdated, lbStatus, true /* wantLb */) + sg, err := az.reconcileSecurityGroup(testClusterName, &svcUpdated, &lbStatus.Ingress[0].IP, true /* wantLb */) if err != nil { t.Errorf("Unexpected error: %q", err) } @@ -738,7 +738,7 @@ func TestReconcileSecurityWithSourceRanges(t *testing.T) { lb, _ := az.reconcileLoadBalancer(testClusterName, &svc, clusterResources.nodes, true) lbStatus, _ := az.getServiceLoadBalancerStatus(&svc, lb) - sg, err := az.reconcileSecurityGroup(testClusterName, &svc, lbStatus, true /* wantLb */) + sg, err := az.reconcileSecurityGroup(testClusterName, &svc, &lbStatus.Ingress[0].IP, true /* wantLb */) if err != nil { t.Errorf("Unexpected error: %q", err) } @@ -1266,19 +1266,73 @@ func validatePublicIP(t *testing.T, publicIP *network.PublicIPAddress, service * // Becuase service properties are updated outside of cloudprovider code } +func contains(ruleValues []string, targetValue string) bool { + for _, ruleValue := range ruleValues { + if strings.EqualFold(ruleValue, targetValue) { + return true + } + } + return false +} + +func securityRuleMatches(serviceSourceRange string, servicePort v1.ServicePort, serviceIP string, securityRule network.SecurityRule) error { + ruleSource := securityRule.SourceAddressPrefixes + if ruleSource == nil || len(*ruleSource) == 0 { + if securityRule.SourceAddressPrefix == nil { + ruleSource = &[]string{} + } else { + ruleSource = &[]string{*securityRule.SourceAddressPrefix} + } + } + + rulePorts := securityRule.DestinationPortRanges + if rulePorts == nil || len(*rulePorts) == 0 { + if securityRule.DestinationPortRange == nil { + rulePorts = &[]string{} + } else { + rulePorts = &[]string{*securityRule.DestinationPortRange} + } + } + + ruleDestination := securityRule.DestinationAddressPrefixes + if ruleDestination == nil || len(*ruleDestination) == 0 { + if securityRule.DestinationAddressPrefix == nil { + ruleDestination = &[]string{} + } else { + ruleDestination = &[]string{*securityRule.DestinationAddressPrefix} + } + } + + if !contains(*ruleSource, serviceSourceRange) { + return fmt.Errorf("Rule does not contain source %s", serviceSourceRange) + } + + if !contains(*rulePorts, fmt.Sprintf("%d", servicePort.Port)) { + return fmt.Errorf("Rule does not contain port %d", servicePort.Port) + } + + if serviceIP != "" && !contains(*ruleDestination, serviceIP) { + return fmt.Errorf("Rule does not contain destination %s", serviceIP) + } + + return nil +} + func validateSecurityGroup(t *testing.T, securityGroup *network.SecurityGroup, services ...v1.Service) { - expectedRuleCount := 0 + seenRules := make(map[string]string) for _, svc := range services { for _, wantedRule := range svc.Spec.Ports { sources := getServiceSourceRanges(&svc) for _, source := range sources { wantedRuleName := getSecurityRuleName(&svc, wantedRule, source) - expectedRuleCount++ + seenRules[wantedRuleName] = wantedRuleName foundRule := false for _, actualRule := range *securityGroup.SecurityRules { - if strings.EqualFold(*actualRule.Name, wantedRuleName) && - *actualRule.SourceAddressPrefix == source && - *actualRule.DestinationPortRange == fmt.Sprintf("%d", wantedRule.Port) { + 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 } @@ -1291,6 +1345,7 @@ func validateSecurityGroup(t *testing.T, securityGroup *network.SecurityGroup, s } lenRules := len(*securityGroup.SecurityRules) + expectedRuleCount := len(seenRules) if lenRules != expectedRuleCount { t.Errorf("Expected the loadbalancer to have %d rules. Found %d.\n", expectedRuleCount, lenRules) } @@ -1698,3 +1753,831 @@ func addTestSubnet(t *testing.T, az *Cloud, svc *v1.Service) { } svc.Annotations[ServiceAnnotationLoadBalancerInternalSubnet] = subName } + +func TestIfServiceSpecifiesSharedRuleAndRuleDoesNotExistItIsCreated(t *testing.T) { + az := getTestCloud() + svc := getTestService("servicesr", v1.ProtocolTCP, 80) + svc.Spec.LoadBalancerIP = "192.168.77.88" + svc.Annotations[ServiceAnnotationSharedSecurityRule] = "true" + + sg := getTestSecurityGroup(az) + + sg, err := az.reconcileSecurityGroup(testClusterName, &svc, to.StringPtr(svc.Spec.LoadBalancerIP), true) + if err != nil { + t.Errorf("Unexpected error: %q", err) + } + + validateSecurityGroup(t, sg, svc) + + expectedRuleName := "shared-TCP-80-Internet" + _, securityRule, ruleFound := findSecurityRuleByName(*sg.SecurityRules, expectedRuleName) + if !ruleFound { + t.Fatalf("Expected security rule %q but it was not present", expectedRuleName) + } + + err = securityRuleMatches("Internet", v1.ServicePort{Port: 80}, "192.168.77.88", securityRule) + if err != nil { + t.Errorf("Shared rule was not updated with new service IP: %v", err) + } + + if securityRule.Priority == nil { + t.Errorf("Shared rule %s had no priority", expectedRuleName) + } + + if securityRule.Access != network.SecurityRuleAccessAllow { + t.Errorf("Shared rule %s did not have Allow access", expectedRuleName) + } + + if securityRule.Direction != network.SecurityRuleDirectionInbound { + t.Errorf("Shared rule %s did not have Inbound direction", expectedRuleName) + } +} + +func TestIfServiceSpecifiesSharedRuleAndRuleExistsThenTheServicesPortAndAddressAreAdded(t *testing.T) { + az := getTestCloud() + svc := getTestService("servicesr", v1.ProtocolTCP, 80) + svc.Spec.LoadBalancerIP = "192.168.77.88" + svc.Annotations[ServiceAnnotationSharedSecurityRule] = "true" + + expectedRuleName := "shared-TCP-80-Internet" + + sg := getTestSecurityGroup(az) + sg.SecurityRules = &[]network.SecurityRule{ + { + Name: &expectedRuleName, + SecurityRulePropertiesFormat: &network.SecurityRulePropertiesFormat{ + Protocol: network.SecurityRuleProtocolTCP, + SourcePortRange: to.StringPtr("*"), + SourceAddressPrefix: to.StringPtr("Internet"), + DestinationPortRange: to.StringPtr("80"), + DestinationAddressPrefix: to.StringPtr("192.168.33.44"), + Access: network.SecurityRuleAccessAllow, + Direction: network.SecurityRuleDirectionInbound, + }, + }, + } + + sg, err := az.reconcileSecurityGroup(testClusterName, &svc, to.StringPtr(svc.Spec.LoadBalancerIP), true) + if err != nil { + t.Errorf("Unexpected error: %q", err) + } + + validateSecurityGroup(t, sg, svc) + + _, securityRule, ruleFound := findSecurityRuleByName(*sg.SecurityRules, expectedRuleName) + if !ruleFound { + t.Fatalf("Expected security rule %q but it was not present", expectedRuleName) + } + + expectedDestinationIPCount := 2 + if len(*securityRule.DestinationAddressPrefixes) != expectedDestinationIPCount { + t.Errorf("Shared rule should have had %d destination IP addresses but had %d", expectedDestinationIPCount, len(*securityRule.DestinationAddressPrefixes)) + } + + err = securityRuleMatches("Internet", v1.ServicePort{Port: 80}, "192.168.33.44", securityRule) + if err != nil { + t.Errorf("Shared rule no longer matched other service IP: %v", err) + } + + err = securityRuleMatches("Internet", v1.ServicePort{Port: 80}, "192.168.77.88", securityRule) + if err != nil { + t.Errorf("Shared rule was not updated with new service IP: %v", err) + } +} + +func TestIfServicesSpecifySharedRuleButDifferentPortsThenSeparateRulesAreCreated(t *testing.T) { + az := getTestCloud() + + svc1 := getTestService("servicesr1", v1.ProtocolTCP, 4444) + svc1.Spec.LoadBalancerIP = "192.168.77.88" + svc1.Annotations[ServiceAnnotationSharedSecurityRule] = "true" + + svc2 := getTestService("servicesr2", v1.ProtocolTCP, 8888) + svc2.Spec.LoadBalancerIP = "192.168.33.44" + svc2.Annotations[ServiceAnnotationSharedSecurityRule] = "true" + + expectedRuleName1 := "shared-TCP-4444-Internet" + expectedRuleName2 := "shared-TCP-8888-Internet" + + 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) + } + + sg, err = az.reconcileSecurityGroup(testClusterName, &svc2, to.StringPtr(svc2.Spec.LoadBalancerIP), true) + if err != nil { + t.Errorf("Unexpected error adding svc2: %q", err) + } + + validateSecurityGroup(t, sg, svc1, svc2) + + _, securityRule1, rule1Found := findSecurityRuleByName(*sg.SecurityRules, expectedRuleName1) + if !rule1Found { + t.Fatalf("Expected security rule %q but it was not present", expectedRuleName1) + } + + _, securityRule2, rule2Found := findSecurityRuleByName(*sg.SecurityRules, expectedRuleName2) + if !rule2Found { + t.Fatalf("Expected security rule %q but it was not present", expectedRuleName2) + } + + expectedDestinationIPCount1 := 1 + if len(*securityRule1.DestinationAddressPrefixes) != expectedDestinationIPCount1 { + t.Errorf("Shared rule %s should have had %d destination IP addresses but had %d", expectedRuleName1, expectedDestinationIPCount1, len(*securityRule1.DestinationAddressPrefixes)) + } + + err = securityRuleMatches("Internet", v1.ServicePort{Port: 4444}, "192.168.77.88", securityRule1) + if err != nil { + t.Errorf("Shared rule %s did not match service IP: %v", expectedRuleName1, err) + } + + err = securityRuleMatches("Internet", v1.ServicePort{Port: 8888}, "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("Internet", v1.ServicePort{Port: 8888}, "192.168.33.44", securityRule2) + if err != nil { + t.Errorf("Shared rule %s did not match service IP: %v", expectedRuleName2, err) + } + + err = securityRuleMatches("Internet", v1.ServicePort{Port: 4444}, "192.168.77.88", securityRule2) + if err == nil { + t.Errorf("Shared rule %s matched wrong service's port and IP", expectedRuleName2) + } +} + +func TestIfServicesSpecifySharedRuleButDifferentProtocolsThenSeparateRulesAreCreated(t *testing.T) { + az := getTestCloud() + + svc1 := getTestService("servicesr1", v1.ProtocolTCP, 4444) + svc1.Spec.LoadBalancerIP = "192.168.77.88" + svc1.Annotations[ServiceAnnotationSharedSecurityRule] = "true" + + svc2 := getTestService("servicesr2", v1.ProtocolUDP, 4444) + svc2.Spec.LoadBalancerIP = "192.168.77.88" + svc2.Annotations[ServiceAnnotationSharedSecurityRule] = "true" + + expectedRuleName1 := "shared-TCP-4444-Internet" + expectedRuleName2 := "shared-UDP-4444-Internet" + + 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) + } + + sg, err = az.reconcileSecurityGroup(testClusterName, &svc2, to.StringPtr(svc2.Spec.LoadBalancerIP), true) + if err != nil { + t.Errorf("Unexpected error adding svc2: %q", err) + } + + validateSecurityGroup(t, sg, svc1, svc2) + + _, securityRule1, rule1Found := findSecurityRuleByName(*sg.SecurityRules, expectedRuleName1) + if !rule1Found { + t.Fatalf("Expected security rule %q but it was not present", expectedRuleName1) + } + + _, securityRule2, rule2Found := findSecurityRuleByName(*sg.SecurityRules, expectedRuleName2) + if !rule2Found { + t.Fatalf("Expected security rule %q but it was not present", expectedRuleName2) + } + + expectedDestinationIPCount1 := 1 + if len(*securityRule1.DestinationAddressPrefixes) != expectedDestinationIPCount1 { + t.Errorf("Shared rule %s should have had %d destination IP addresses but had %d", expectedRuleName1, expectedDestinationIPCount1, len(*securityRule1.DestinationAddressPrefixes)) + } + + err = securityRuleMatches("Internet", v1.ServicePort{Port: 4444}, "192.168.77.88", securityRule1) + if err != nil { + t.Errorf("Shared rule %s did not match service IP: %v", expectedRuleName1, err) + } + + if securityRule1.Protocol != network.SecurityRuleProtocolTCP { + t.Errorf("Shared rule %s should have been %s but was %s", expectedRuleName1, network.SecurityRuleProtocolTCP, securityRule1.Protocol) + } + + 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("Internet", v1.ServicePort{Port: 4444}, "192.168.77.88", securityRule2) + if err != nil { + t.Errorf("Shared rule %s did not match service IP: %v", expectedRuleName2, err) + } + + if securityRule2.Protocol != network.SecurityRuleProtocolUDP { + t.Errorf("Shared rule %s should have been %s but was %s", expectedRuleName2, network.SecurityRuleProtocolUDP, securityRule2.Protocol) + } +} + +func TestIfServicesSpecifySharedRuleButDifferentSourceAddressesThenSeparateRulesAreCreated(t *testing.T) { + az := getTestCloud() + + svc1 := getTestService("servicesr1", v1.ProtocolTCP, 80) + svc1.Spec.LoadBalancerIP = "192.168.77.88" + svc1.Spec.LoadBalancerSourceRanges = []string{"192.168.12.0/24"} + svc1.Annotations[ServiceAnnotationSharedSecurityRule] = "true" + + svc2 := getTestService("servicesr2", v1.ProtocolTCP, 80) + svc2.Spec.LoadBalancerIP = "192.168.33.44" + svc2.Spec.LoadBalancerSourceRanges = []string{"192.168.34.0/24"} + svc2.Annotations[ServiceAnnotationSharedSecurityRule] = "true" + + expectedRuleName1 := "shared-TCP-80-192.168.12.0_24" + expectedRuleName2 := "shared-TCP-80-192.168.34.0_24" + + 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) + } + + sg, err = az.reconcileSecurityGroup(testClusterName, &svc2, to.StringPtr(svc2.Spec.LoadBalancerIP), true) + if err != nil { + t.Errorf("Unexpected error adding svc2: %q", err) + } + + validateSecurityGroup(t, sg, svc1, svc2) + + _, securityRule1, rule1Found := findSecurityRuleByName(*sg.SecurityRules, expectedRuleName1) + if !rule1Found { + t.Fatalf("Expected security rule %q but it was not present", expectedRuleName1) + } + + _, securityRule2, rule2Found := findSecurityRuleByName(*sg.SecurityRules, expectedRuleName2) + if !rule2Found { + t.Fatalf("Expected security rule %q but it was not present", expectedRuleName2) + } + + expectedDestinationIPCount1 := 1 + if len(*securityRule1.DestinationAddressPrefixes) != expectedDestinationIPCount1 { + t.Errorf("Shared rule %s should have had %d destination IP addresses but had %d", expectedRuleName1, expectedDestinationIPCount1, len(*securityRule1.DestinationAddressPrefixes)) + } + + 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 IP: %v", expectedRuleName1, 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 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) { + az := getTestCloud() + + svc1 := getTestService("servicesr1", v1.ProtocolTCP, 4444) + svc1.Spec.LoadBalancerIP = "192.168.77.88" + svc1.Annotations[ServiceAnnotationSharedSecurityRule] = "true" + + svc2 := getTestService("servicesr2", v1.ProtocolTCP, 8888) + svc2.Spec.LoadBalancerIP = "192.168.33.44" + svc2.Annotations[ServiceAnnotationSharedSecurityRule] = "true" + + svc3 := getTestService("servicesr3", v1.ProtocolTCP, 4444) + svc3.Spec.LoadBalancerIP = "192.168.99.11" + svc3.Annotations[ServiceAnnotationSharedSecurityRule] = "true" + + expectedRuleName13 := "shared-TCP-4444-Internet" + expectedRuleName2 := "shared-TCP-8888-Internet" + + 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) + } + + sg, err = az.reconcileSecurityGroup(testClusterName, &svc2, to.StringPtr(svc2.Spec.LoadBalancerIP), true) + if err != nil { + t.Errorf("Unexpected error adding svc2: %q", err) + } + + sg, err = az.reconcileSecurityGroup(testClusterName, &svc3, to.StringPtr(svc3.Spec.LoadBalancerIP), true) + if err != nil { + t.Errorf("Unexpected error adding svc3: %q", err) + } + + 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) + } + + _, securityRule2, rule2Found := findSecurityRuleByName(*sg.SecurityRules, expectedRuleName2) + if !rule2Found { + t.Fatalf("Expected security rule %q but it was not present", expectedRuleName2) + } + + 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)) + } + + err = securityRuleMatches("Internet", v1.ServicePort{Port: 4444}, "192.168.77.88", securityRule13) + if err != nil { + t.Errorf("Shared rule %s did not match service IP: %v", expectedRuleName13, err) + } + + err = securityRuleMatches("Internet", v1.ServicePort{Port: 4444}, "192.168.99.11", securityRule13) + if err != nil { + t.Errorf("Shared rule %s did not match service IP: %v", expectedRuleName13, err) + } + + err = securityRuleMatches("Internet", v1.ServicePort{Port: 8888}, "192.168.33.44", securityRule13) + if err == nil { + t.Errorf("Shared rule %s matched wrong service's port and IP", expectedRuleName13) + } + + if securityRule13.Priority == nil { + t.Errorf("Shared rule %s had no priority", expectedRuleName13) + } + + if securityRule13.Access != network.SecurityRuleAccessAllow { + t.Errorf("Shared rule %s did not have Allow access", expectedRuleName13) + } + + if securityRule13.Direction != network.SecurityRuleDirectionInbound { + t.Errorf("Shared rule %s did not have Inbound direction", expectedRuleName13) + } + + 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("Internet", v1.ServicePort{Port: 8888}, "192.168.33.44", securityRule2) + if err != nil { + t.Errorf("Shared rule %s did not match service IP: %v", expectedRuleName2, err) + } + + err = securityRuleMatches("Internet", v1.ServicePort{Port: 4444}, "192.168.77.88", securityRule2) + if err == nil { + t.Errorf("Shared rule %s matched wrong service's port and IP", expectedRuleName2) + } + + err = securityRuleMatches("Internet", v1.ServicePort{Port: 4444}, "192.168.99.11", securityRule2) + if err == nil { + t.Errorf("Shared rule %s matched wrong service's port and IP", expectedRuleName2) + } +} + +func TestIfServiceSpecifiesSharedRuleAndServiceIsDeletedThenTheServicesPortAndAddressAreRemoved(t *testing.T) { + az := getTestCloud() + + svc1 := getTestService("servicesr1", v1.ProtocolTCP, 80) + svc1.Spec.LoadBalancerIP = "192.168.77.88" + svc1.Annotations[ServiceAnnotationSharedSecurityRule] = "true" + + svc2 := getTestService("servicesr2", v1.ProtocolTCP, 80) + svc2.Spec.LoadBalancerIP = "192.168.33.44" + svc2.Annotations[ServiceAnnotationSharedSecurityRule] = "true" + + expectedRuleName := "shared-TCP-80-Internet" + + 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) + } + + sg, err = az.reconcileSecurityGroup(testClusterName, &svc2, to.StringPtr(svc2.Spec.LoadBalancerIP), true) + if err != nil { + t.Errorf("Unexpected error adding svc2: %q", err) + } + + validateSecurityGroup(t, sg, svc1, svc2) + + sg, err = az.reconcileSecurityGroup(testClusterName, &svc1, to.StringPtr(svc1.Spec.LoadBalancerIP), false) + if err != nil { + t.Errorf("Unexpected error removing svc1: %q", err) + } + + validateSecurityGroup(t, sg, svc2) + + _, securityRule, ruleFound := findSecurityRuleByName(*sg.SecurityRules, expectedRuleName) + if !ruleFound { + t.Fatalf("Expected security rule %q but it was not present", expectedRuleName) + } + + expectedDestinationIPCount := 1 + if len(*securityRule.DestinationAddressPrefixes) != expectedDestinationIPCount { + t.Errorf("Shared rule should have had %d destination IP addresses but had %d", expectedDestinationIPCount, len(*securityRule.DestinationAddressPrefixes)) + } + + err = securityRuleMatches("Internet", v1.ServicePort{Port: 80}, "192.168.33.44", securityRule) + if err != nil { + t.Errorf("Shared rule no longer matched other service IP: %v", err) + } + + err = securityRuleMatches("Internet", v1.ServicePort{Port: 80}, "192.168.77.88", securityRule) + if err == nil { + t.Error("Shared rule was not updated to remove deleted service IP") + } +} + +func TestIfSomeServicesShareARuleAndOneIsDeletedItIsRemovedFromTheRightRule(t *testing.T) { + az := getTestCloud() + + svc1 := getTestService("servicesr1", v1.ProtocolTCP, 4444) + svc1.Spec.LoadBalancerIP = "192.168.77.88" + svc1.Annotations[ServiceAnnotationSharedSecurityRule] = "true" + + svc2 := getTestService("servicesr2", v1.ProtocolTCP, 8888) + svc2.Spec.LoadBalancerIP = "192.168.33.44" + svc2.Annotations[ServiceAnnotationSharedSecurityRule] = "true" + + svc3 := getTestService("servicesr3", v1.ProtocolTCP, 4444) + svc3.Spec.LoadBalancerIP = "192.168.99.11" + svc3.Annotations[ServiceAnnotationSharedSecurityRule] = "true" + + expectedRuleName13 := "shared-TCP-4444-Internet" + expectedRuleName2 := "shared-TCP-8888-Internet" + + 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) + } + + sg, err = az.reconcileSecurityGroup(testClusterName, &svc2, to.StringPtr(svc2.Spec.LoadBalancerIP), true) + if err != nil { + t.Errorf("Unexpected error adding svc2: %q", err) + } + + sg, err = az.reconcileSecurityGroup(testClusterName, &svc3, to.StringPtr(svc3.Spec.LoadBalancerIP), true) + if err != nil { + t.Errorf("Unexpected error adding svc3: %q", err) + } + + validateSecurityGroup(t, sg, svc1, svc2, svc3) + + sg, err = az.reconcileSecurityGroup(testClusterName, &svc1, to.StringPtr(svc1.Spec.LoadBalancerIP), false) + if err != nil { + t.Errorf("Unexpected error removing svc1: %q", err) + } + + validateSecurityGroup(t, sg, svc2, svc3) + + _, securityRule13, rule13Found := findSecurityRuleByName(*sg.SecurityRules, expectedRuleName13) + if !rule13Found { + t.Fatalf("Expected security rule %q but it was not present", expectedRuleName13) + } + + _, securityRule2, rule2Found := findSecurityRuleByName(*sg.SecurityRules, expectedRuleName2) + if !rule2Found { + t.Fatalf("Expected security rule %q but it was not present", expectedRuleName2) + } + + 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)) + } + + err = securityRuleMatches("Internet", v1.ServicePort{Port: 4444}, "192.168.77.88", securityRule13) + if err == nil { + t.Errorf("Shared rule %s should have had svc1 removed but did not", expectedRuleName13) + } + + err = securityRuleMatches("Internet", v1.ServicePort{Port: 4444}, "192.168.99.11", securityRule13) + if err != nil { + t.Errorf("Shared rule %s did not match service IP: %v", expectedRuleName13, err) + } + + err = securityRuleMatches("Internet", v1.ServicePort{Port: 8888}, "192.168.33.44", securityRule13) + if err == nil { + t.Errorf("Shared rule %s matched wrong service's port and IP", expectedRuleName13) + } + + if securityRule13.Priority == nil { + t.Errorf("Shared rule %s had no priority", expectedRuleName13) + } + + if securityRule13.Access != network.SecurityRuleAccessAllow { + t.Errorf("Shared rule %s did not have Allow access", expectedRuleName13) + } + + if securityRule13.Direction != network.SecurityRuleDirectionInbound { + t.Errorf("Shared rule %s did not have Inbound direction", expectedRuleName13) + } + + 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("Internet", v1.ServicePort{Port: 8888}, "192.168.33.44", securityRule2) + if err != nil { + t.Errorf("Shared rule %s did not match service IP: %v", expectedRuleName2, err) + } + + err = securityRuleMatches("Internet", v1.ServicePort{Port: 4444}, "192.168.77.88", securityRule2) + if err == nil { + t.Errorf("Shared rule %s matched wrong service's port and IP", expectedRuleName2) + } + + err = securityRuleMatches("Internet", v1.ServicePort{Port: 4444}, "192.168.99.11", securityRule2) + if err == nil { + t.Errorf("Shared rule %s matched wrong service's port and IP", expectedRuleName2) + } +} + +func TestIfServiceSpecifiesSharedRuleAndLastServiceIsDeletedThenRuleIsDeleted(t *testing.T) { + az := getTestCloud() + + svc1 := getTestService("servicesr1", v1.ProtocolTCP, 4444) + svc1.Spec.LoadBalancerIP = "192.168.77.88" + svc1.Annotations[ServiceAnnotationSharedSecurityRule] = "true" + + svc2 := getTestService("servicesr2", v1.ProtocolTCP, 8888) + svc2.Spec.LoadBalancerIP = "192.168.33.44" + svc2.Annotations[ServiceAnnotationSharedSecurityRule] = "true" + + svc3 := getTestService("servicesr3", v1.ProtocolTCP, 4444) + svc3.Spec.LoadBalancerIP = "192.168.99.11" + svc3.Annotations[ServiceAnnotationSharedSecurityRule] = "true" + + expectedRuleName13 := "shared-TCP-4444-Internet" + expectedRuleName2 := "shared-TCP-8888-Internet" + + 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) + } + + sg, err = az.reconcileSecurityGroup(testClusterName, &svc2, to.StringPtr(svc2.Spec.LoadBalancerIP), true) + if err != nil { + t.Errorf("Unexpected error adding svc2: %q", err) + } + + sg, err = az.reconcileSecurityGroup(testClusterName, &svc3, to.StringPtr(svc3.Spec.LoadBalancerIP), true) + if err != nil { + t.Errorf("Unexpected error adding svc3: %q", err) + } + + validateSecurityGroup(t, sg, svc1, svc2, svc3) + + sg, err = az.reconcileSecurityGroup(testClusterName, &svc1, to.StringPtr(svc1.Spec.LoadBalancerIP), false) + if err != nil { + t.Errorf("Unexpected error removing svc1: %q", err) + } + + sg, err = az.reconcileSecurityGroup(testClusterName, &svc3, to.StringPtr(svc3.Spec.LoadBalancerIP), false) + if err != nil { + t.Errorf("Unexpected error removing svc3: %q", err) + } + + validateSecurityGroup(t, sg, svc2) + + _, _, rule13Found := findSecurityRuleByName(*sg.SecurityRules, expectedRuleName13) + if rule13Found { + t.Fatalf("Expected security rule %q to have been deleted but it was still present", expectedRuleName13) + } + + _, securityRule2, rule2Found := findSecurityRuleByName(*sg.SecurityRules, expectedRuleName2) + if !rule2Found { + t.Fatalf("Expected security rule %q but it was not present", expectedRuleName2) + } + + 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("Internet", v1.ServicePort{Port: 8888}, "192.168.33.44", securityRule2) + if err != nil { + t.Errorf("Shared rule %s did not match service IP: %v", expectedRuleName2, err) + } + + err = securityRuleMatches("Internet", v1.ServicePort{Port: 4444}, "192.168.77.88", securityRule2) + if err == nil { + t.Errorf("Shared rule %s matched wrong service's port and IP", expectedRuleName2) + } + + err = securityRuleMatches("Internet", v1.ServicePort{Port: 4444}, "192.168.99.11", securityRule2) + if err == nil { + t.Errorf("Shared rule %s matched wrong service's port and IP", expectedRuleName2) + } +} + +func TestCanCombineSharedAndPrivateRulesInSameGroup(t *testing.T) { + az := getTestCloud() + + svc1 := getTestService("servicesr1", v1.ProtocolTCP, 4444) + svc1.Spec.LoadBalancerIP = "192.168.77.88" + svc1.Annotations[ServiceAnnotationSharedSecurityRule] = "true" + + svc2 := getTestService("servicesr2", v1.ProtocolTCP, 8888) + svc2.Spec.LoadBalancerIP = "192.168.33.44" + svc2.Annotations[ServiceAnnotationSharedSecurityRule] = "true" + + svc3 := getTestService("servicesr3", v1.ProtocolTCP, 4444) + svc3.Spec.LoadBalancerIP = "192.168.99.11" + svc3.Annotations[ServiceAnnotationSharedSecurityRule] = "true" + + svc4 := getTestService("servicesr4", v1.ProtocolTCP, 4444) + svc4.Spec.LoadBalancerIP = "192.168.22.33" + svc4.Annotations[ServiceAnnotationSharedSecurityRule] = "false" + + svc5 := getTestService("servicesr5", v1.ProtocolTCP, 8888) + svc5.Spec.LoadBalancerIP = "192.168.22.33" + svc5.Annotations[ServiceAnnotationSharedSecurityRule] = "false" + + expectedRuleName13 := "shared-TCP-4444-Internet" + expectedRuleName2 := "shared-TCP-8888-Internet" + expectedRuleName4 := getSecurityRuleName(&svc4, v1.ServicePort{Port: 4444, Protocol: v1.ProtocolTCP}, "Internet") + expectedRuleName5 := getSecurityRuleName(&svc5, v1.ServicePort{Port: 8888, Protocol: v1.ProtocolTCP}, "Internet") + + 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) + } + + sg, err = az.reconcileSecurityGroup(testClusterName, &svc2, to.StringPtr(svc2.Spec.LoadBalancerIP), true) + if err != nil { + t.Errorf("Unexpected error adding svc2: %q", err) + } + + sg, err = az.reconcileSecurityGroup(testClusterName, &svc3, to.StringPtr(svc3.Spec.LoadBalancerIP), true) + if err != nil { + t.Errorf("Unexpected error adding svc3: %q", err) + } + + sg, err = az.reconcileSecurityGroup(testClusterName, &svc4, to.StringPtr(svc4.Spec.LoadBalancerIP), true) + if err != nil { + t.Errorf("Unexpected error adding svc4: %q", err) + } + + sg, err = az.reconcileSecurityGroup(testClusterName, &svc5, to.StringPtr(svc5.Spec.LoadBalancerIP), true) + if err != nil { + t.Errorf("Unexpected error adding svc4: %q", err) + } + + validateSecurityGroup(t, sg, svc1, svc2, svc3, svc4, svc5) + + expectedRuleCount := 4 + if len(*sg.SecurityRules) != expectedRuleCount { + t.Errorf("Expected security group to have %d rules but it had %d", expectedRuleCount, len(*sg.SecurityRules)) + } + + _, securityRule13, rule13Found := findSecurityRuleByName(*sg.SecurityRules, expectedRuleName13) + if !rule13Found { + t.Fatalf("Expected security rule %q but it was not present", expectedRuleName13) + } + + _, securityRule2, rule2Found := findSecurityRuleByName(*sg.SecurityRules, expectedRuleName2) + if !rule2Found { + t.Fatalf("Expected security rule %q but it was not present", expectedRuleName2) + } + + _, securityRule4, rule4Found := findSecurityRuleByName(*sg.SecurityRules, expectedRuleName4) + if !rule4Found { + t.Fatalf("Expected security rule %q but it was not present", expectedRuleName4) + } + + _, securityRule5, rule5Found := findSecurityRuleByName(*sg.SecurityRules, expectedRuleName5) + if !rule5Found { + t.Fatalf("Expected security rule %q but it was not present", expectedRuleName5) + } + + 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)) + } + + err = securityRuleMatches("Internet", v1.ServicePort{Port: 4444}, "192.168.77.88", securityRule13) + if err != nil { + t.Errorf("Shared rule %s did not match service IP: %v", expectedRuleName13, err) + } + + err = securityRuleMatches("Internet", v1.ServicePort{Port: 4444}, "192.168.99.11", securityRule13) + if err != nil { + t.Errorf("Shared rule %s did not match service IP: %v", expectedRuleName13, err) + } + + err = securityRuleMatches("Internet", v1.ServicePort{Port: 4444}, "192.168.22.33", securityRule13) + if err == nil { + t.Errorf("Shared rule %s matched wrong (unshared) service's port and IP", expectedRuleName13) + } + + 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("Internet", v1.ServicePort{Port: 8888}, "192.168.33.44", securityRule2) + if err != nil { + t.Errorf("Shared rule %s did not match service IP: %v", expectedRuleName2, err) + } + + err = securityRuleMatches("Internet", v1.ServicePort{Port: 8888}, "192.168.22.33", securityRule2) + if err == nil { + t.Errorf("Shared rule %s matched wrong (unshared) service's port and IP", expectedRuleName2) + } + + if securityRule4.DestinationAddressPrefixes != nil { + t.Errorf("Expected unshared rule %s to use single destination IP address but used collection", expectedRuleName4) + } + + if securityRule4.DestinationAddressPrefix == nil { + t.Errorf("Expected unshared rule %s to have a destination IP address", expectedRuleName4) + } else { + if !strings.EqualFold(*securityRule4.DestinationAddressPrefix, svc4.Spec.LoadBalancerIP) { + t.Errorf("Expected unshared rule %s to have a destination %s but had %s", expectedRuleName4, svc4.Spec.LoadBalancerIP, *securityRule4.DestinationAddressPrefix) + } + } + + if securityRule5.DestinationAddressPrefixes != nil { + t.Errorf("Expected unshared rule %s to use single destination IP address but used collection", expectedRuleName5) + } + + if securityRule5.DestinationAddressPrefix == nil { + t.Errorf("Expected unshared rule %s to have a destination IP address", expectedRuleName5) + } else { + if !strings.EqualFold(*securityRule5.DestinationAddressPrefix, svc5.Spec.LoadBalancerIP) { + t.Errorf("Expected unshared rule %s to have a destination %s but had %s", expectedRuleName5, svc5.Spec.LoadBalancerIP, *securityRule5.DestinationAddressPrefix) + } + } + + sg, err = az.reconcileSecurityGroup(testClusterName, &svc1, to.StringPtr(svc1.Spec.LoadBalancerIP), false) + if err != nil { + t.Errorf("Unexpected error removing svc1: %q", err) + } + + sg, err = az.reconcileSecurityGroup(testClusterName, &svc5, to.StringPtr(svc5.Spec.LoadBalancerIP), false) + if err != nil { + t.Errorf("Unexpected error removing svc5: %q", err) + } + + _, securityRule13, rule13Found = findSecurityRuleByName(*sg.SecurityRules, expectedRuleName13) + if !rule13Found { + t.Fatalf("Expected security rule %q but it was not present", expectedRuleName13) + } + + _, securityRule2, rule2Found = findSecurityRuleByName(*sg.SecurityRules, expectedRuleName2) + if !rule2Found { + t.Fatalf("Expected security rule %q but it was not present", expectedRuleName2) + } + + _, securityRule4, rule4Found = findSecurityRuleByName(*sg.SecurityRules, expectedRuleName4) + if !rule4Found { + t.Fatalf("Expected security rule %q but it was not present", expectedRuleName4) + } + + _, _, rule5Found = findSecurityRuleByName(*sg.SecurityRules, expectedRuleName5) + if rule5Found { + t.Fatalf("Expected security rule %q to have been removed but it was not present", expectedRuleName5) + } + + 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)) + } +} + +// TODO: sanity check if the same IP address incorrectly gets put in twice? +// (shouldn't happen but...) + +// func TestIfServiceIsEditedFromOwnRuleToSharedRuleThenOwnRuleIsDeletedAndSharedRuleIsCreated(t *testing.T) { +// t.Error() +// } + +// func TestIfServiceIsEditedFromSharedRuleToOwnRuleThenItIsRemovedFromSharedRuleAndOwnRuleIsCreated(t *testing.T) { +// t.Error() +// } diff --git a/pkg/cloudprovider/providers/azure/azure_util.go b/pkg/cloudprovider/providers/azure/azure_util.go index 04ff821e76a..7d2aa565c4a 100644 --- a/pkg/cloudprovider/providers/azure/azure_util.go +++ b/pkg/cloudprovider/providers/azure/azure_util.go @@ -340,6 +340,10 @@ func getLoadBalancerRuleName(service *v1.Service, port v1.ServicePort, subnetNam } func getSecurityRuleName(service *v1.Service, port v1.ServicePort, sourceAddrPrefix string) string { + if useSharedSecurityRule(service) { + safePrefix := strings.Replace(sourceAddrPrefix, "/", "_", -1) + return fmt.Sprintf("shared-%s-%d-%s", port.Protocol, port.Port, safePrefix) + } safePrefix := strings.Replace(sourceAddrPrefix, "/", "_", -1) return fmt.Sprintf("%s-%s-%d-%s", getRulePrefix(service), port.Protocol, port.Port, safePrefix) }