diff --git a/pkg/operator/controller/ingress/load_balancer_service.go b/pkg/operator/controller/ingress/load_balancer_service.go index 2c4de654d0..98cd3bfa42 100644 --- a/pkg/operator/controller/ingress/load_balancer_service.go +++ b/pkg/operator/controller/ingress/load_balancer_service.go @@ -43,6 +43,9 @@ const ( // 5 and 300. awsLBHealthCheckIntervalAnnotation = "service.beta.kubernetes.io/aws-load-balancer-healthcheck-interval" awsLBHealthCheckIntervalDefault = "5" + // Network Load Balancers require a health check interval of 10 or 30. + // See https://docs.aws.amazon.com/elasticloadbalancing/latest/network/target-group-health-checks.html + awsLBHealthCheckIntervalNLB = "10" // awsLBHealthCheckTimeoutAnnotation is the amount of time, in seconds, during which no response // means a failed AWS load balancer health check. The value must be less than the value of @@ -141,16 +144,27 @@ func (r *reconciler) ensureLoadBalancerService(ci *operatorv1.IngressController, if err != nil { return false, nil, err } - if wantLBS && !haveLBS { - if err := r.client.Create(context.TODO(), desiredLBService); err != nil { - return false, nil, fmt.Errorf("failed to create load balancer service %s/%s: %v", desiredLBService.Namespace, desiredLBService.Name, err) + + switch { + case !wantLBS && !haveLBS: + return false, nil, nil + case !wantLBS && haveLBS: + // TODO: Delete the service. + return true, currentLBService, nil + case wantLBS && !haveLBS: + if err := r.createLoadBalancerService(desiredLBService); err != nil { + return false, nil, err + } + return r.currentLoadBalancerService(ci) + case wantLBS && haveLBS: + if updated, err := r.updateLoadBalancerService(currentLBService, desiredLBService, platform); err != nil { + return true, currentLBService, fmt.Errorf("failed to update load balancer service: %v", err) + } else if updated { + log.Info("updated load balancer service", "namespace", desiredLBService.Namespace, "name", desiredLBService.Name) + return r.currentLoadBalancerService(ci) } - log.Info("created load balancer service", "namespace", desiredLBService.Namespace, "name", desiredLBService.Name) - return true, desiredLBService, nil } - // return haveLBS instead of forcing true here since - // there is no guarantee that currentLBService != nil - return haveLBS, currentLBService, nil + return true, currentLBService, nil } // desiredLoadBalancerService returns the desired LB service for a @@ -201,9 +215,12 @@ func desiredLoadBalancerService(ci *operatorv1.IngressController, deploymentRef ci.Status.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS != nil && ci.Status.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.Type == operatorv1.AWSNetworkLoadBalancer { service.Annotations[AWSLBTypeAnnotation] = AWSNLBAnnotation + // NLBs require a different health check interval than CLBs + service.Annotations[awsLBHealthCheckIntervalAnnotation] = awsLBHealthCheckIntervalNLB + } else { + service.Annotations[awsLBHealthCheckIntervalAnnotation] = awsLBHealthCheckIntervalDefault } // Set the load balancer for AWS to be as aggressive as Azure (2 fail @ 5s interval, 2 healthy) - service.Annotations[awsLBHealthCheckIntervalAnnotation] = awsLBHealthCheckIntervalDefault service.Annotations[awsLBHealthCheckTimeoutAnnotation] = awsLBHealthCheckTimeoutDefault service.Annotations[awsLBHealthCheckUnhealthyThresholdAnnotation] = awsLBHealthCheckUnhealthyThresholdDefault service.Annotations[awsLBHealthCheckHealthyThresholdAnnotation] = awsLBHealthCheckHealthyThresholdDefault @@ -260,3 +277,47 @@ func (r *reconciler) finalizeLoadBalancerService(ci *operatorv1.IngressControlle log.Info("finalized load balancer service for ingress", "namespace", ci.Namespace, "name", ci.Name) return true, nil } + +// createLoadBalancerService creates a load balancer service. +func (r *reconciler) createLoadBalancerService(service *corev1.Service) error { + if err := r.client.Create(context.TODO(), service); err != nil { + return fmt.Errorf("failed to create load balancer service %s/%s: %v", service.Namespace, service.Name, err) + } + log.Info("created load balancer service", "namespace", service.Namespace, "name", service.Name) + return nil +} + +// updateLoadBalancerService updates a load balancer service. Returns a Boolean +// indicating whether the service was updated, and an error value. +func (r *reconciler) updateLoadBalancerService(current, desired *corev1.Service, platform *configv1.PlatformStatus) (bool, error) { + changed, updated := loadBalancerServiceChanged(current, desired) + if !changed { + return false, nil + } + if err := r.client.Update(context.TODO(), updated); err != nil { + return false, err + } + return true, nil +} + +// loadBalancerServiceChanged checks if the current load balancer service +// matches the expected and if not returns an updated one. +func loadBalancerServiceChanged(current, expected *corev1.Service) (bool, *corev1.Service) { + updated := current.DeepCopy() + + // Preserve everything but the AWS LB health check interval annotation + // (see ). + // Updating annotations and spec fields cannot be done unless the + // previous release blocks upgrades when the user has modified those + // fields (see ). + if updated.Annotations == nil { + updated.Annotations = map[string]string{} + } + if current.Annotations[awsLBHealthCheckIntervalAnnotation] == expected.Annotations[awsLBHealthCheckIntervalAnnotation] { + return false, nil + } + + updated.Annotations[awsLBHealthCheckIntervalAnnotation] = expected.Annotations[awsLBHealthCheckIntervalAnnotation] + + return true, updated +} diff --git a/pkg/operator/controller/ingress/load_balancer_service_test.go b/pkg/operator/controller/ingress/load_balancer_service_test.go index f23177a3e5..f13d16162e 100644 --- a/pkg/operator/controller/ingress/load_balancer_service_test.go +++ b/pkg/operator/controller/ingress/load_balancer_service_test.go @@ -9,6 +9,7 @@ import ( operatorv1 "github.com/openshift/api/operator/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/intstr" ) func TestDesiredLoadBalancerService(t *testing.T) { @@ -222,9 +223,6 @@ func TestDesiredLoadBalancerService(t *testing.T) { } } if tc.strategyType == operatorv1.LoadBalancerServiceStrategyType { - if err := checkServiceHasAnnotation(svc, awsLBHealthCheckIntervalAnnotation, true, awsLBHealthCheckIntervalDefault); err != nil { - t.Errorf("annotation check for test %q failed: %v", tc.description, err) - } if err := checkServiceHasAnnotation(svc, awsLBHealthCheckTimeoutAnnotation, true, awsLBHealthCheckTimeoutDefault); err != nil { t.Errorf("annotation check for test %q failed: %v", tc.description, err) } @@ -237,10 +235,16 @@ func TestDesiredLoadBalancerService(t *testing.T) { classicLB := tc.lbStrategy.ProviderParameters == nil || tc.lbStrategy.ProviderParameters.AWS.Type == operatorv1.AWSClassicLoadBalancer switch { case classicLB: + if err := checkServiceHasAnnotation(svc, awsLBHealthCheckIntervalAnnotation, true, awsLBHealthCheckIntervalDefault); err != nil { + t.Errorf("annotation check for test %q failed: %v", tc.description, err) + } if err := checkServiceHasAnnotation(svc, awsLBProxyProtocolAnnotation, true, "*"); err != nil { t.Errorf("annotation check for test %q failed: %v", tc.description, err) } case tc.lbStrategy.ProviderParameters.AWS.Type == operatorv1.AWSNetworkLoadBalancer: + if err := checkServiceHasAnnotation(svc, awsLBHealthCheckIntervalAnnotation, true, awsLBHealthCheckIntervalNLB); err != nil { + t.Errorf("annotation check for test %q failed: %v", tc.description, err) + } if err := checkServiceHasAnnotation(svc, AWSLBTypeAnnotation, true, AWSNLBAnnotation); err != nil { t.Errorf("annotation check for test %q failed: %v", tc.description, err) } @@ -329,3 +333,162 @@ func checkServiceHasAnnotation(svc *corev1.Service, name string, expectValue boo return nil } } + +func TestLoadBalancerServiceChanged(t *testing.T) { + testCases := []struct { + description string + mutate func(*corev1.Service) + expect bool + }{ + { + description: "if nothing changes", + mutate: func(_ *corev1.Service) {}, + expect: false, + }, + { + description: "if .uid changes", + mutate: func(svc *corev1.Service) { + svc.UID = "2" + }, + expect: false, + }, + { + description: "if .spec.clusterIP changes", + mutate: func(svc *corev1.Service) { + svc.Spec.ClusterIP = "2.3.4.5" + }, + expect: false, + }, + { + description: "if .spec.externalIPs changes", + mutate: func(svc *corev1.Service) { + svc.Spec.ExternalIPs = []string{"3.4.5.6"} + }, + expect: false, + }, + { + description: "if .spec.externalTrafficPolicy changes", + mutate: func(svc *corev1.Service) { + svc.Spec.ExternalTrafficPolicy = corev1.ServiceExternalTrafficPolicyTypeCluster + }, + expect: false, + }, + { + description: "if .spec.healthCheckNodePort changes", + mutate: func(svc *corev1.Service) { + svc.Spec.HealthCheckNodePort = int32(34566) + }, + expect: false, + }, + { + description: "if .spec.ports changes", + mutate: func(svc *corev1.Service) { + newPort := corev1.ServicePort{ + Name: "foo", + Protocol: corev1.ProtocolTCP, + Port: int32(8080), + TargetPort: intstr.FromString("foo"), + } + svc.Spec.Ports = append(svc.Spec.Ports, newPort) + }, + expect: false, + }, + { + description: "if .spec.ports[*].nodePort changes", + mutate: func(svc *corev1.Service) { + svc.Spec.Ports[0].NodePort = int32(33337) + svc.Spec.Ports[1].NodePort = int32(33338) + }, + expect: false, + }, + { + description: "if .spec.selector changes", + mutate: func(svc *corev1.Service) { + svc.Spec.Selector = nil + }, + expect: false, + }, + { + description: "if .spec.sessionAffinity is defaulted", + mutate: func(service *corev1.Service) { + service.Spec.SessionAffinity = corev1.ServiceAffinityNone + }, + expect: false, + }, + { + description: "if .spec.sessionAffinity is set to a non-default value", + mutate: func(service *corev1.Service) { + service.Spec.SessionAffinity = corev1.ServiceAffinityClientIP + }, + expect: false, + }, + { + description: "if .spec.type changes", + mutate: func(svc *corev1.Service) { + svc.Spec.Type = corev1.ServiceTypeNodePort + }, + expect: false, + }, + { + description: "if the service.beta.kubernetes.io/load-balancer-source-ranges annotation changes", + mutate: func(svc *corev1.Service) { + svc.Annotations["service.beta.kubernetes.io/load-balancer-source-ranges"] = "10.0.0.0/8" + }, + expect: false, + }, + { + description: "if the service.beta.kubernetes.io/aws-load-balancer-healthcheck-interval annotation changes", + mutate: func(svc *corev1.Service) { + svc.Annotations["service.beta.kubernetes.io/aws-load-balancer-healthcheck-interval"] = "10" + }, + expect: true, + }, + } + + for _, tc := range testCases { + original := corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "service.beta.kubernetes.io/aws-load-balancer-healthcheck-interval": "5", + }, + Namespace: "openshift-ingress", + Name: "router-original", + UID: "1", + }, + Spec: corev1.ServiceSpec{ + ClusterIP: "1.2.3.4", + ExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyTypeLocal, + HealthCheckNodePort: int32(33333), + Ports: []corev1.ServicePort{ + { + Name: "http", + NodePort: int32(33334), + Port: int32(80), + Protocol: corev1.ProtocolTCP, + TargetPort: intstr.FromString("http"), + }, + { + Name: "https", + NodePort: int32(33335), + Port: int32(443), + Protocol: corev1.ProtocolTCP, + TargetPort: intstr.FromString("https"), + }, + }, + Selector: map[string]string{ + "foo": "bar", + }, + Type: corev1.ServiceTypeLoadBalancer, + }, + } + mutated := original.DeepCopy() + tc.mutate(mutated) + if changed, updated := loadBalancerServiceChanged(&original, mutated); changed != tc.expect { + t.Errorf("%s, expect loadBalancerServiceChanged to be %t, got %t", tc.description, tc.expect, changed) + } else if changed { + if changedAgain, _ := loadBalancerServiceChanged(mutated, updated); changedAgain { + t.Errorf("%s, loadBalancerServiceChanged does not behave as a fixed point function", tc.description) + } + } + } +}