Add NodePortService endpoint publishing strategy

This commit resolves NE-260. https://issues.redhat.com/browse/NE-260 * README.md: Document NodePortService. * docs/images/endpoint-publishing-nodeportservice.png: New file. * pkg/operator/controller/ingress/controller.go (setDefaultPublishingStrategy): Add NodePortServiceStrategyType. (ensureIngressController): Call ensureNodePortService. * pkg/operator/controller/ingress/nodeport_service.go: New file. (ensureNodePortService): New method. Ensure the appropriate NodePort service exists for the given ingress controller, if one is desired. Get the current one using currentNodePortService. If none exists and one is needed, create it using the desiredNodePortService function. If one already exists, update if it necessary using updateNodePortService, or delete it if none is desired. (desiredNodePortService): New function. (currentNodePortService): New method. (updateNodePortService): New method. Use nodePortServiceChanged to determine whether an update is needed. (nodePortServiceChanged): New function. * pkg/operator/controller/ingress/nodeport_service_test.go: New file. (TestDesiredNodePortService): New test. (TestNodePortServiceChanged): New test. * pkg/operator/controller/names.go (NodePortServiceName): New function. * test/e2e/operator_test.go (TestNodePortServiceChanged): New test. (newNodePortController): New function.
Miciah · Jan 4, 2020 · 3d9a5d3 · 3d9a5d3
1 parent 7e0d9b6
commit 3d9a5d3
Show file tree

Hide file tree

Showing 7 changed files with 405 additions and 0 deletions.
diff --git a/README.md b/README.md
@@ -84,6 +84,16 @@ and on some platforms offers managed wildcard DNS.
 
 ![Image of LoadBalancerService](docs/images/endpoint-publishing-loadbalancerservice.png)
 
+#### NodePortService
+
+The `NodePortService` strategy publishes an ingress controller using a
+Kubernetes [NodePort
+Service](https://kubernetes.io/docs/concepts/services-networking/service/#nodeport).
+With this strategy, the administrator is responsible for configuring
+any external DNS or load balancer.
+
+![Image of NodePortService](docs/images/endpoint-publishing-nodeportservice.png)
+
 #### HostNetwork
 
 The `HostNetwork` strategy uses host networking to publish the ingress

diff --git a/docs/images/endpoint-publishing-nodeportservice.png b/docs/images/endpoint-publishing-nodeportservice.png
diff --git a/pkg/operator/controller/ingress/controller.go b/pkg/operator/controller/ingress/controller.go
@@ -316,6 +316,8 @@ func setDefaultPublishingStrategy(ic *operatorv1.IngressController, infraConfig
 				Scope: operatorv1.ExternalLoadBalancer,
 			}
 		}
+	case operatorv1.NodePortServiceStrategyType:
+		// No parameters.
 	case operatorv1.HostNetworkStrategyType:
 		// No parameters.
 	case operatorv1.PrivateStrategyType:
@@ -595,6 +597,10 @@ func (r *reconciler) ensureIngressController(ci *operatorv1.IngressController, d
 		}
 	}
 
+	if _, _, err := r.ensureNodePortService(ci, deploymentRef); err != nil {
+		errs = append(errs, err)
+	}
+
 	if internalSvc, err := r.ensureInternalIngressControllerService(ci, deploymentRef); err != nil {
 		errs = append(errs, fmt.Errorf("failed to create internal router service for ingresscontroller %s: %v", ci.Name, err))
 	} else if err := r.ensureMetricsIntegration(ci, internalSvc, deploymentRef); err != nil {

diff --git a/pkg/operator/controller/ingress/nodeport_service.go b/pkg/operator/controller/ingress/nodeport_service.go
@@ -0,0 +1,163 @@
+package ingress
+
+import (
+	"context"
+	"fmt"
+
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
+
+	operatorv1 "github.com/openshift/api/operator/v1"
+	"github.com/openshift/cluster-ingress-operator/pkg/manifests"
+	"github.com/openshift/cluster-ingress-operator/pkg/operator/controller"
+
+	corev1 "k8s.io/api/core/v1"
+
+	"k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+	"k8s.io/apimachinery/pkg/util/intstr"
+)
+
+// ensureNodePortService ensures a NodePort service exists for a given
+// ingresscontroller, if and only if one is desired.  Returns a Boolean
+// indicating whether the NodePort service exists, the current NodePort service
+// if it does exist, and an error value.
+func (r *reconciler) ensureNodePortService(ic *operatorv1.IngressController, deploymentRef metav1.OwnerReference) (bool, *corev1.Service, error) {
+	wantService, desired := desiredNodePortService(ic, deploymentRef)
+
+	haveService, current, err := r.currentNodePortService(ic)
+	if err != nil {
+		return false, nil, err
+	}
+
+	switch {
+	case !wantService && !haveService:
+		return false, nil, nil
+	case !wantService && haveService:
+		if err := r.client.Delete(context.TODO(), current); err != nil {
+			if errors.IsNotFound(err) {
+				return false, nil, nil
+			}
+			return true, current, fmt.Errorf("failed to delete NodePort service: %v", err)
+		}
+		log.Info("deleted NodePort service", "service", current)
+	case wantService && !haveService:
+		if err := r.client.Create(context.TODO(), desired); err != nil {
+			return false, nil, fmt.Errorf("failed to create NodePort service: %v", err)
+		}
+		log.Info("created NodePort service", "service", desired)
+	case wantService && haveService:
+		if updated, err := r.updateNodePortService(current, desired); err != nil {
+			return true, nil, fmt.Errorf("failed to update NodePort service: %v", err)
+		} else if updated {
+			log.Info("updated NodePort service", "service", desired)
+		}
+	}
+
+	return r.currentNodePortService(ic)
+}
+
+// desiredNodePortService returns a Boolean indicating whether a NodePort
+// service is desired, as well as the NodePort service if one is desired.
+func desiredNodePortService(ic *operatorv1.IngressController, deploymentRef metav1.OwnerReference) (bool, *corev1.Service) {
+	if ic.Status.EndpointPublishingStrategy.Type != operatorv1.NodePortServiceStrategyType {
+		return false, nil
+	}
+
+	name := controller.NodePortServiceName(ic)
+	service := &corev1.Service{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: name.Namespace,
+			Name:      name.Name,
+			Labels: map[string]string{
+				"app":                                  "router",
+				"router":                               name.Name,
+				manifests.OwningIngressControllerLabel: ic.Name,
+			},
+			OwnerReferences: []metav1.OwnerReference{deploymentRef},
+		},
+		Spec: corev1.ServiceSpec{
+			ExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyTypeLocal,
+			Ports: []corev1.ServicePort{
+				{
+					Name:       "http",
+					Protocol:   corev1.ProtocolTCP,
+					Port:       int32(80),
+					TargetPort: intstr.FromString("http"),
+				},
+				{
+					Name:       "https",
+					Protocol:   corev1.ProtocolTCP,
+					Port:       int32(443),
+					TargetPort: intstr.FromString("https"),
+				},
+			},
+			Selector: controller.IngressControllerDeploymentPodSelector(ic).MatchLabels,
+			Type:     corev1.ServiceTypeNodePort,
+		},
+	}
+
+	return true, service
+}
+
+// currentNodePortService returns a Boolean indicating whether a NodePort
+// service exists for the given ingresscontroller, as well as the NodePort
+// service if it does exist and an error value.
+func (r *reconciler) currentNodePortService(ic *operatorv1.IngressController) (bool, *corev1.Service, error) {
+	service := &corev1.Service{}
+	if err := r.client.Get(context.TODO(), controller.NodePortServiceName(ic), service); err != nil {
+		if errors.IsNotFound(err) {
+			return false, nil, nil
+		}
+		return false, nil, err
+	}
+	return true, service, nil
+}
+
+// updateNodePortService updates a NodePort service.  Returns a Boolean
+// indicating whether the service was updated, and an error value.
+func (r *reconciler) updateNodePortService(current, desired *corev1.Service) (bool, error) {
+	changed, updated := nodePortServiceChanged(current, desired)
+	if !changed {
+		return false, nil
+	}
+
+	if err := r.client.Update(context.TODO(), updated); err != nil {
+		return false, err
+	}
+	return true, nil
+}
+
+// nodePortServiceChanged checks if the current NodePort service spec matches
+// the expected spec and if not returns an updated one.
+func nodePortServiceChanged(current, expected *corev1.Service) (bool, *corev1.Service) {
+	serviceCmpOpts := []cmp.Option{
+		// Ignore fields that the API, other controllers, or user may
+		// have modified.
+		cmpopts.IgnoreFields(corev1.ServicePort{}, "NodePort"),
+		cmpopts.IgnoreFields(corev1.ServiceSpec{}, "ClusterIP", "ExternalIPs", "HealthCheckNodePort"),
+		cmpopts.EquateEmpty(),
+	}
+	if cmp.Equal(current.Spec, expected.Spec, serviceCmpOpts...) {
+		return false, nil
+	}
+
+	updated := current.DeepCopy()
+	updated.Spec = expected.Spec
+
+	// Preserve fields that the API, other controllers, or user may have
+	// modified.
+	updated.Spec.ClusterIP = current.Spec.ClusterIP
+	updated.Spec.ExternalIPs = current.Spec.ExternalIPs
+	updated.Spec.HealthCheckNodePort = current.Spec.HealthCheckNodePort
+	for i, updatedPort := range updated.Spec.Ports {
+		for _, currentPort := range current.Spec.Ports {
+			if currentPort.Name == updatedPort.Name {
+				updated.Spec.Ports[i].NodePort = currentPort.NodePort
+			}
+		}
+	}
+
+	return true, updated
+}
diff --git a/pkg/operator/controller/ingress/nodeport_service_test.go b/pkg/operator/controller/ingress/nodeport_service_test.go
@@ -0,0 +1,182 @@
+package ingress
+
+import (
+	"testing"
+
+	operatorv1 "github.com/openshift/api/operator/v1"
+
+	corev1 "k8s.io/api/core/v1"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/intstr"
+)
+
+func TestDesiredNodePortService(t *testing.T) {
+	testCases := []struct {
+		strategyType operatorv1.EndpointPublishingStrategyType
+		expect       bool
+	}{
+		{
+			strategyType: operatorv1.LoadBalancerServiceStrategyType,
+			expect:       false,
+		},
+		{
+			strategyType: operatorv1.NodePortServiceStrategyType,
+			expect:       true,
+		},
+	}
+
+	for _, tc := range testCases {
+		ic := &operatorv1.IngressController{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: "default",
+			},
+			Status: operatorv1.IngressControllerStatus{
+				EndpointPublishingStrategy: &operatorv1.EndpointPublishingStrategy{
+					Type: tc.strategyType,
+				},
+			},
+		}
+		trueVar := true
+		deploymentRef := metav1.OwnerReference{
+			APIVersion: "apps/v1",
+			Kind:       "Deployment",
+			Name:       "router-default",
+			UID:        "1",
+			Controller: &trueVar,
+		}
+
+		want, svc := desiredNodePortService(ic, deploymentRef)
+		if want != tc.expect {
+			t.Errorf("expected desiredNodePortService to return %t for endpoint publishing strategy type %v, got %t, with service %#v", tc.expect, tc.strategyType, want, svc)
+		}
+	}
+}
+
+func TestNodePortServiceChanged(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: true,
+		},
+		{
+			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: true,
+		},
+		{
+			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: true,
+		},
+		{
+			description: "if .spec.type changes",
+			mutate: func(svc *corev1.Service) {
+				svc.Spec.Type = corev1.ServiceTypeLoadBalancer
+			},
+			expect: true,
+		},
+	}
+
+	for _, tc := range testCases {
+		original := corev1.Service{
+			ObjectMeta: metav1.ObjectMeta{
+				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.ServiceTypeNodePort,
+			},
+		}
+		mutated := original.DeepCopy()
+		tc.mutate(mutated)
+		if changed, updated := nodePortServiceChanged(&original, mutated); changed != tc.expect {
+			t.Errorf("%s, expect nodePortServiceChanged to be %t, got %t", tc.description, tc.expect, changed)
+		} else if changed {
+			if changedAgain, _ := nodePortServiceChanged(mutated, updated); changedAgain {
+				t.Errorf("%s, nodePortServiceChanged does not behave as a fixed point function", tc.description)
+			}
+		}
+	}
+}
diff --git a/pkg/operator/controller/names.go b/pkg/operator/controller/names.go
@@ -147,6 +147,10 @@ func LoadBalancerServiceName(ic *operatorv1.IngressController) types.NamespacedN
 	return types.NamespacedName{Namespace: "openshift-ingress", Name: "router-" + ic.Name}
 }
 
+func NodePortServiceName(ic *operatorv1.IngressController) types.NamespacedName {
+	return types.NamespacedName{Namespace: "openshift-ingress", Name: "router-nodeport-" + ic.Name}
+}
+
 func WildcardDNSRecordName(ic *operatorv1.IngressController) types.NamespacedName {
 	return types.NamespacedName{
 		Namespace: ic.Namespace,