Bug 1908758: Update AWS LB health check interval annotation

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 <https://bugzilla.redhat.com/show_bug.cgi?id=1908758>).
+	// Updating annotations and spec fields cannot be done unless the
+	// previous release blocks upgrades when the user has modified those
+	// fields (see <https://bugzilla.redhat.com/show_bug.cgi?id=1905490>).
+	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
+}

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)
+			}
+		}
+	}
+}