From 54664d08dd66e401f1fcde4df79e09a985930dff Mon Sep 17 00:00:00 2001 From: Dong Liu Date: Wed, 22 Mar 2017 13:27:33 +0800 Subject: [PATCH] Update reconcileSecurityGroup logic for Azure, add tests. --- .../providers/azure/azure_loadbalancer.go | 50 ++++++---- .../providers/azure/azure_test.go | 93 +++++++++++++++++-- 2 files changed, 117 insertions(+), 26 deletions(-) diff --git a/pkg/cloudprovider/providers/azure/azure_loadbalancer.go b/pkg/cloudprovider/providers/azure/azure_loadbalancer.go index 2d4ae7d0d4d..5ee913a504a 100644 --- a/pkg/cloudprovider/providers/azure/azure_loadbalancer.go +++ b/pkg/cloudprovider/providers/azure/azure_loadbalancer.go @@ -38,7 +38,7 @@ const ServiceAnnotationLoadBalancerInternal = "service.beta.kubernetes.io/azure- // 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) { - isInternal := isLoadBalancerInternal(service) + isInternal := requiresInternalLoadBalancer(service) lbName := getLoadBalancerName(clusterName, isInternal) serviceName := getServiceName(service) @@ -113,7 +113,7 @@ func (az *Cloud) getPublicIPName(clusterName string, service *v1.Service) (strin // EnsureLoadBalancer creates a new load balancer 'name', or updates the existing one. Returns the status of the balancer func (az *Cloud) EnsureLoadBalancer(clusterName string, service *v1.Service, nodes []*v1.Node) (*v1.LoadBalancerStatus, error) { - isInternal := isLoadBalancerInternal(service) + isInternal := requiresInternalLoadBalancer(service) lbName := getLoadBalancerName(clusterName, isInternal) // When a client updates the internal load balancer annotation, @@ -128,7 +128,7 @@ func (az *Cloud) EnsureLoadBalancer(clusterName string, service *v1.Service, nod if err != nil { return nil, err } - sg, sgNeedsUpdate, err := az.reconcileSecurityGroup(sg, clusterName, service) + sg, sgNeedsUpdate, err := az.reconcileSecurityGroup(sg, clusterName, service, true /* wantLb */) if err != nil { return nil, err } @@ -266,7 +266,7 @@ 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 := isLoadBalancerInternal(service) + isInternal := requiresInternalLoadBalancer(service) lbName := getLoadBalancerName(clusterName, isInternal) serviceName := getServiceName(service) @@ -279,7 +279,7 @@ func (az *Cloud) EnsureLoadBalancerDeleted(clusterName string, service *v1.Servi return err } if existsSg { - reconciledSg, sgNeedsUpdate, reconcileErr := az.reconcileSecurityGroup(sg, clusterName, service) + reconciledSg, sgNeedsUpdate, reconcileErr := az.reconcileSecurityGroup(sg, clusterName, service, false /* wantLb */) if reconcileErr != nil { return reconcileErr } @@ -306,9 +306,6 @@ func (az *Cloud) ensureLoadBalancerDeleted(clusterName string, service *v1.Servi glog.V(10).Infof("ensure lb deleted: clusterName=%q, serviceName=%s, lbName=%q", clusterName, serviceName, lbName) - // reconcile logic is capable of fully reconcile, so we can use this to delete - service.Spec.Ports = []v1.ServicePort{} - lb, existsLb, err := az.getAzureLoadBalancer(lbName) if err != nil { return err @@ -395,7 +392,7 @@ func (az *Cloud) ensurePublicIPDeleted(serviceName, pipName string) error { // This also reconciles the Service's Ports with the LoadBalancer config. // This entails adding rules/probes for expected Ports and removing stale rules/ports. func (az *Cloud) reconcileLoadBalancer(lb network.LoadBalancer, fipConfigurationProperties *network.FrontendIPConfigurationPropertiesFormat, clusterName string, service *v1.Service, nodes []*v1.Node) (network.LoadBalancer, bool, error) { - isInternal := isLoadBalancerInternal(service) + isInternal := requiresInternalLoadBalancer(service) lbName := getLoadBalancerName(clusterName, isInternal) serviceName := getServiceName(service) lbFrontendIPConfigName := getFrontendIPConfigName(service) @@ -403,7 +400,7 @@ func (az *Cloud) reconcileLoadBalancer(lb network.LoadBalancer, fipConfiguration lbBackendPoolName := getBackendPoolName(clusterName) lbBackendPoolID := az.getBackendPoolID(lbName, lbBackendPoolName) - wantLb := len(service.Spec.Ports) > 0 + wantLb := fipConfigurationProperties != nil dirtyLb := false // Ensure LoadBalancer's Backend Pool Configuration @@ -473,9 +470,15 @@ func (az *Cloud) reconcileLoadBalancer(lb network.LoadBalancer, fipConfiguration } // update probes/rules - expectedProbes := make([]network.Probe, len(service.Spec.Ports)) - expectedRules := make([]network.LoadBalancingRule, len(service.Spec.Ports)) - for i, port := range service.Spec.Ports { + var ports []v1.ServicePort + if wantLb { + ports = service.Spec.Ports + } else { + ports = []v1.ServicePort{} + } + expectedProbes := make([]network.Probe, len(ports)) + expectedRules := make([]network.LoadBalancingRule, len(ports)) + for i, port := range ports { lbRuleName := getRuleName(service, port) transportProto, _, probeProto, err := getProtocolsFromKubernetesProtocol(port.Protocol) @@ -614,9 +617,14 @@ func (az *Cloud) reconcileLoadBalancer(lb network.LoadBalancer, fipConfiguration // 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(sg network.SecurityGroup, clusterName string, service *v1.Service) (network.SecurityGroup, bool, error) { +func (az *Cloud) reconcileSecurityGroup(sg network.SecurityGroup, clusterName string, service *v1.Service, wantLb bool) (network.SecurityGroup, bool, error) { serviceName := getServiceName(service) - wantLb := len(service.Spec.Ports) > 0 + var ports []v1.ServicePort + if wantLb { + ports = service.Spec.Ports + } else { + ports = []v1.ServicePort{} + } sourceRanges, err := serviceapi.GetLoadBalancerSourceRanges(service) if err != nil { @@ -624,15 +632,17 @@ func (az *Cloud) reconcileSecurityGroup(sg network.SecurityGroup, clusterName st } var sourceAddressPrefixes []string if sourceRanges == nil || serviceapi.IsAllowAll(sourceRanges) { - sourceAddressPrefixes = []string{"Internet"} + if !requiresInternalLoadBalancer(service) { + sourceAddressPrefixes = []string{"Internet"} + } } else { for _, ip := range sourceRanges { sourceAddressPrefixes = append(sourceAddressPrefixes, ip.String()) } } - expectedSecurityRules := make([]network.SecurityRule, len(service.Spec.Ports)*len(sourceAddressPrefixes)) + expectedSecurityRules := make([]network.SecurityRule, len(ports)*len(sourceAddressPrefixes)) - for i, port := range service.Spec.Ports { + for i, port := range ports { securityRuleName := getRuleName(service, port) _, securityProto, _, err := getProtocolsFromKubernetesProtocol(port.Protocol) if err != nil { @@ -799,8 +809,8 @@ func (az *Cloud) ensureHostInPool(serviceName string, nodeName types.NodeName, b return nil } -// Check if service requests an internal load balancer. -func isLoadBalancerInternal(service *v1.Service) bool { +// Check if service requires an internal load balancer. +func requiresInternalLoadBalancer(service *v1.Service) bool { if l, ok := service.Annotations[ServiceAnnotationLoadBalancerInternal]; ok { return l == "true" } diff --git a/pkg/cloudprovider/providers/azure/azure_test.go b/pkg/cloudprovider/providers/azure/azure_test.go index c0452d00642..af9bd96e20c 100644 --- a/pkg/cloudprovider/providers/azure/azure_test.go +++ b/pkg/cloudprovider/providers/azure/azure_test.go @@ -87,6 +87,37 @@ func TestReconcileLoadBalancerNodeHealth(t *testing.T) { } // Test removing all services results in removing the frontend ip configuration +func TestReconcileLoadBalancerRemoveService(t *testing.T) { + az := getTestCloud() + svc := getTestService("servicea", 80, 443) + lb := getTestLoadBalancer() + configProperties := getTestPublicFipConfigurationProperties() + nodes := []*v1.Node{} + + lb, updated, err := az.reconcileLoadBalancer(lb, &configProperties, testClusterName, &svc, nodes) + if err != nil { + t.Errorf("Unexpected error: %q", err) + } + validateLoadBalancer(t, lb, svc) + + lb, updated, err = az.reconcileLoadBalancer(lb, nil, testClusterName, &svc, nodes) + if err != nil { + t.Errorf("Unexpected error: %q", err) + } + + if !updated { + t.Error("Expected the loadbalancer to need an update") + } + + // ensure we abandoned the frontend ip configuration + if len(*lb.FrontendIPConfigurations) != 0 { + t.Error("Expected the loadbalancer to have no frontend ip configuration") + } + + validateLoadBalancer(t, lb) +} + +// Test removing all service ports results in removing the frontend ip configuration func TestReconcileLoadBalancerRemoveAllPortsRemovesFrontendConfig(t *testing.T) { az := getTestCloud() svc := getTestService("servicea", 80) @@ -98,6 +129,7 @@ func TestReconcileLoadBalancerRemoveAllPortsRemovesFrontendConfig(t *testing.T) if err != nil { t.Errorf("Unexpected error: %q", err) } + validateLoadBalancer(t, lb, svc) svcUpdated := getTestService("servicea") lb, updated, err = az.reconcileLoadBalancer(lb, nil, testClusterName, &svcUpdated, nodes) @@ -164,7 +196,7 @@ func TestReconcileSecurityGroupNewServiceAddsPort(t *testing.T) { sg := getTestSecurityGroup() - sg, _, err := az.reconcileSecurityGroup(sg, testClusterName, &svc1) + sg, _, err := az.reconcileSecurityGroup(sg, testClusterName, &svc1, true) if err != nil { t.Errorf("Unexpected error: %q", err) } @@ -172,6 +204,36 @@ func TestReconcileSecurityGroupNewServiceAddsPort(t *testing.T) { validateSecurityGroup(t, sg, svc1) } +func TestReconcileSecurityGroupNewInternalServiceAddsPort(t *testing.T) { + az := getTestCloud() + svc1 := getInternalTestService("serviceea", 80) + + sg := getTestSecurityGroup() + + sg, _, err := az.reconcileSecurityGroup(sg, testClusterName, &svc1, true) + if err != nil { + t.Errorf("Unexpected error: %q", err) + } + + validateSecurityGroup(t, sg, svc1) +} + +func TestReconcileSecurityGroupRemoveService(t *testing.T) { + service1 := getTestService("servicea", 81) + service2 := getTestService("serviceb", 82) + + sg := getTestSecurityGroup(service1, service2) + + validateSecurityGroup(t, sg, service1, service2) + az := getTestCloud() + sg, _, err := az.reconcileSecurityGroup(sg, testClusterName, &service1, false) + if err != nil { + t.Errorf("Unexpected error: %q", err) + } + + validateSecurityGroup(t, sg, service2) +} + func TestReconcileSecurityGroupRemoveServiceRemovesPort(t *testing.T) { az := getTestCloud() svc := getTestService("servicea", 80, 443) @@ -179,7 +241,7 @@ func TestReconcileSecurityGroupRemoveServiceRemovesPort(t *testing.T) { sg := getTestSecurityGroup(svc) svcUpdated := getTestService("servicea", 80) - sg, _, err := az.reconcileSecurityGroup(sg, testClusterName, &svcUpdated) + sg, _, err := az.reconcileSecurityGroup(sg, testClusterName, &svcUpdated, true) if err != nil { t.Errorf("Unexpected error: %q", err) } @@ -196,7 +258,7 @@ func TestReconcileSecurityWithSourceRanges(t *testing.T) { } sg := getTestSecurityGroup(svc) - sg, _, err := az.reconcileSecurityGroup(sg, testClusterName, &svc) + sg, _, err := az.reconcileSecurityGroup(sg, testClusterName, &svc, true) if err != nil { t.Errorf("Unexpected error: %q", err) } @@ -249,6 +311,14 @@ func getTestService(identifier string, requestedPorts ...int32) v1.Service { svc.Name = identifier svc.Namespace = "default" svc.UID = types.UID(identifier) + svc.Annotations = make(map[string]string) + + return svc +} + +func getInternalTestService(identifier string, requestedPorts ...int32) v1.Service { + svc := getTestService(identifier, requestedPorts...) + svc.Annotations[ServiceAnnotationLoadBalancerInternal] = "true" return svc } @@ -288,8 +358,11 @@ func getTestLoadBalancer(services ...v1.Service) network.LoadBalancer { func getServiceSourceRanges(service *v1.Service) []string { if len(service.Spec.LoadBalancerSourceRanges) == 0 { - return []string{"Internet"} + if !requiresInternalLoadBalancer(service) { + return []string{"Internet"} + } } + return service.Spec.LoadBalancerSourceRanges } @@ -324,7 +397,11 @@ func getTestSecurityGroup(services ...v1.Service) network.SecurityGroup { func validateLoadBalancer(t *testing.T, loadBalancer network.LoadBalancer, services ...v1.Service) { expectedRuleCount := 0 + expectedFrontendIPCount := 0 for _, svc := range services { + if len(svc.Spec.Ports) > 0 { + expectedFrontendIPCount++ + } for _, wantedRule := range svc.Spec.Ports { expectedRuleCount++ wantedRuleName := getRuleName(&svc, wantedRule) @@ -371,6 +448,11 @@ func validateLoadBalancer(t *testing.T, loadBalancer network.LoadBalancer, servi } } + frontendIPCount := len(*loadBalancer.FrontendIPConfigurations) + if frontendIPCount != expectedFrontendIPCount { + t.Errorf("Expected the loadbalancer to have %d frontend IPs. Found %d.\n%v", expectedFrontendIPCount, frontendIPCount, loadBalancer.FrontendIPConfigurations) + } + lenRules := len(*loadBalancer.LoadBalancingRules) if lenRules != expectedRuleCount { t.Errorf("Expected the loadbalancer to have %d rules. Found %d.\n%v", expectedRuleCount, lenRules, loadBalancer.LoadBalancingRules) @@ -386,10 +468,9 @@ func validateSecurityGroup(t *testing.T, securityGroup network.SecurityGroup, se for _, svc := range services { for _, wantedRule := range svc.Spec.Ports { sources := getServiceSourceRanges(&svc) - + wantedRuleName := getRuleName(&svc, wantedRule) for _, source := range sources { expectedRuleCount++ - wantedRuleName := getRuleName(&svc, wantedRule) foundRule := false for _, actualRule := range *securityGroup.SecurityRules { if strings.EqualFold(*actualRule.Name, wantedRuleName) &&