Merge pull request #466 from Miciah/desiredNodePortService-add-port-1936

Bug 1881210: desiredNodePortService: Add port 1936

pkg/operator/controller/ingress/deployment.go

@@ -480,8 +480,8 @@ func desiredRouterDeployment(ci *operatorv1.IngressController, ingressController
 	deployment.Spec.Template.Spec.DNSPolicy = corev1.DNSClusterFirst
 
 	if ci.Status.EndpointPublishingStrategy.Type == operatorv1.HostNetworkStrategyType {
-		// Expose ports 80 and 443 on the host to provide endpoints for
-		// the user's HA solution.
+		// Expose ports 80, 443, and 1936 on the host to provide
+		// endpoints for the user's HA solution.
 		deployment.Spec.Template.Spec.HostNetwork = true
 
 		// With container networking, probes default to using the pod IP

pkg/operator/controller/ingress/nodeport_service.go

@@ -24,13 +24,30 @@ import (
 // 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
 	}
 
+	// For compatibility, omit the "metrics" port iff the service already
+	// exists and doesn't have a "metrics" port.  This serves two purposes:
+	// (1) It avoids exhausting the nodeport range on upgrades if the
+	// cluster has many nodeport services and few available nodeports.
+	// (2) It enables the cluster administrator to remove the metrics port
+	// from an existing nodeport service to avoid exposing the port.
+	wantMetricsPort := false
+	if !haveService {
+		wantMetricsPort = true
+	} else {
+		for _, port := range current.Spec.Ports {
+			if port.Name == "metrics" {
+				wantMetricsPort = true
+			}
+		}
+	}
+
+	wantService, desired := desiredNodePortService(ic, deploymentRef, wantMetricsPort)
+
 	switch {
 	case !wantService && !haveService:
 		return false, nil, nil
@@ -62,7 +79,7 @@ func (r *reconciler) ensureNodePortService(ic *operatorv1.IngressController, dep
 
 // 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) {
+func desiredNodePortService(ic *operatorv1.IngressController, deploymentRef metav1.OwnerReference, wantMetricsPort bool) (bool, *corev1.Service) {
 	if ic.Status.EndpointPublishingStrategy.Type != operatorv1.NodePortServiceStrategyType {
 		return false, nil
 	}
@@ -94,11 +111,20 @@ func desiredNodePortService(ic *operatorv1.IngressController, deploymentRef meta
 					Port:       int32(443),
 					TargetPort: intstr.FromString("https"),
 				},
+				{
+					Name:       "metrics",
+					Protocol:   corev1.ProtocolTCP,
+					Port:       int32(1936),
+					TargetPort: intstr.FromString("metrics"),
+				},
 			},
 			Selector: controller.IngressControllerDeploymentPodSelector(ic).MatchLabels,
 			Type:     corev1.ServiceTypeNodePort,
 		},
 	}
+	if !wantMetricsPort {
+		service.Spec.Ports = service.Spec.Ports[0:2]
+	}
 
 	return true, service
 }

pkg/operator/controller/ingress/nodeport_service_test.go

 package ingress
 
 import (
+	"reflect"
 	"testing"
 
 	operatorv1 "github.com/openshift/api/operator/v1"
@@ -12,9 +13,20 @@ import (
 )
 
 func TestDesiredNodePortService(t *testing.T) {
+	trueVar := true
+	deploymentRef := metav1.OwnerReference{
+		APIVersion: "apps/v1",
+		Kind:       "Deployment",
+		Name:       "router-default",
+		UID:        "1",
+		Controller: &trueVar,
+	}
+
 	testCases := []struct {
-		strategyType operatorv1.EndpointPublishingStrategyType
-		expect       bool
+		strategyType    operatorv1.EndpointPublishingStrategyType
+		wantMetricsPort bool
+		expect          bool
+		expectService   corev1.Service
 	}{
 		{
 			strategyType: operatorv1.LoadBalancerServiceStrategyType,
@@ -23,6 +35,83 @@ func TestDesiredNodePortService(t *testing.T) {
 		{
 			strategyType: operatorv1.NodePortServiceStrategyType,
 			expect:       true,
+			expectService: corev1.Service{
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace: "openshift-ingress",
+					Name:      "router-nodeport-default",
+					Labels: map[string]string{
+						"app":    "router",
+						"router": "router-nodeport-default",
+						"ingresscontroller.operator.openshift.io/owning-ingresscontroller": "default",
+					},
+					OwnerReferences: []metav1.OwnerReference{deploymentRef},
+				},
+				Spec: corev1.ServiceSpec{
+					ExternalTrafficPolicy: "Local",
+					Ports: []corev1.ServicePort{
+						{
+							Name:       "http",
+							Protocol:   "TCP",
+							Port:       int32(80),
+							TargetPort: intstr.FromString("http"),
+						},
+						{
+							Name:       "https",
+							Protocol:   "TCP",
+							Port:       int32(443),
+							TargetPort: intstr.FromString("https"),
+						},
+					},
+					Selector: map[string]string{
+						"ingresscontroller.operator.openshift.io/deployment-ingresscontroller": "default",
+					},
+					Type: "NodePort",
+				},
+			},
+		},
+		{
+			strategyType:    operatorv1.NodePortServiceStrategyType,
+			wantMetricsPort: true,
+			expect:          true,
+			expectService: corev1.Service{
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace: "openshift-ingress",
+					Name:      "router-nodeport-default",
+					Labels: map[string]string{
+						"app":    "router",
+						"router": "router-nodeport-default",
+						"ingresscontroller.operator.openshift.io/owning-ingresscontroller": "default",
+					},
+					OwnerReferences: []metav1.OwnerReference{deploymentRef},
+				},
+				Spec: corev1.ServiceSpec{
+					ExternalTrafficPolicy: "Local",
+					Ports: []corev1.ServicePort{
+						{
+							Name:       "http",
+							Protocol:   "TCP",
+							Port:       int32(80),
+							TargetPort: intstr.FromString("http"),
+						},
+						{
+							Name:       "https",
+							Protocol:   "TCP",
+							Port:       int32(443),
+							TargetPort: intstr.FromString("https"),
+						},
+						{
+							Name:       "metrics",
+							Protocol:   "TCP",
+							Port:       int32(1936),
+							TargetPort: intstr.FromString("metrics"),
+						},
+					},
+					Selector: map[string]string{
+						"ingresscontroller.operator.openshift.io/deployment-ingresscontroller": "default",
+					},
+					Type: "NodePort",
+				},
+			},
 		},
 	}
 
@@ -37,18 +126,11 @@ func TestDesiredNodePortService(t *testing.T) {
 				},
 			},
 		}
-		trueVar := true
-		deploymentRef := metav1.OwnerReference{
-			APIVersion: "apps/v1",
-			Kind:       "Deployment",
-			Name:       "router-default",
-			UID:        "1",
-			Controller: &trueVar,
-		}
-
-		want, svc := desiredNodePortService(ic, deploymentRef)
+		want, svc := desiredNodePortService(ic, deploymentRef, tc.wantMetricsPort)
 		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)
+		} else if tc.expect && !reflect.DeepEqual(svc, &tc.expectService) {
+			t.Errorf("expected desiredNodePortService to return %#v, got %#v", &tc.expectService, svc)
 		}
 	}
 }
@@ -177,6 +259,13 @@ func TestNodePortServiceChanged(t *testing.T) {
 						Protocol:   corev1.ProtocolTCP,
 						TargetPort: intstr.FromString("https"),
 					},
+					{
+						Name:       "metrics",
+						NodePort:   int32(33336),
+						Port:       int32(1936),
+						Protocol:   corev1.ProtocolTCP,
+						TargetPort: intstr.FromString("metrics"),
+					},
 				},
 				Selector: map[string]string{
 					"foo": "bar",

test/e2e/operator_test.go

@@ -883,7 +883,12 @@ func TestInternalLoadBalancerGlobalAccessGCP(t *testing.T) {
 
 // TestNodePortServiceEndpointPublishingStrategy creates an ingresscontroller
 // with the "NodePortService" endpoint publishing strategy type and verifies
-// that the operator creates a router and that the router becomes available.
+// that the operator creates a router, that the router becomes available, and
+// that the operator creates the expected NodePort-type service.
+//
+// The test then removes the "metrics" port from the NodePort-type service and
+// verifies that the operator does not add the port back.  See
+// <https://bugzilla.redhat.com/show_bug.cgi?id=1881210>.
 func TestNodePortServiceEndpointPublishingStrategy(t *testing.T) {
 	name := types.NamespacedName{Namespace: operatorNamespace, Name: "nodeport"}
 	ing := newNodePortController(name, name.Name+"."+dnsConfig.Spec.BaseDomain)
@@ -902,6 +907,54 @@ func TestNodePortServiceEndpointPublishingStrategy(t *testing.T) {
 	if err != nil {
 		t.Errorf("failed to observe expected conditions: %v", err)
 	}
+
+	// Make sure the ingresscontroller has a nodeport service
+	// with the expected ports.
+	svcName := controller.NodePortServiceName(ing)
+	service := &corev1.Service{}
+	err = wait.PollImmediate(1*time.Second, 1*time.Minute, func() (bool, error) {
+		if err := kclient.Get(context.TODO(), svcName, service); err != nil {
+			t.Logf("failed to get service %q: %v", svcName, err)
+			return false, nil
+		}
+		return true, nil
+	})
+	ports := sets.String{}
+	for _, port := range service.Spec.Ports {
+		ports.Insert(port.Name)
+	}
+	expectedPorts := sets.NewString("http", "https", "metrics")
+	if !ports.Equal(expectedPorts) {
+		t.Fatalf("expected service %q to have ports %v, got %v", svcName, expectedPorts.List(), ports.List())
+	}
+
+	// Delete the "metrics" port and verify that the operator
+	// does not restore it.
+	for i, port := range service.Spec.Ports {
+		if port.Name == "metrics" {
+			ports := service.Spec.Ports
+			ports = append(ports[:i], ports[i+1:]...)
+			service.Spec.Ports = ports
+		}
+	}
+	if err := kclient.Update(context.TODO(), service); err != nil {
+		t.Fatalf("failed to update service %q: %v", svcName, err)
+	}
+	expectedPorts = sets.NewString("http", "https")
+	wait.PollImmediate(1*time.Second, 1*time.Minute, func() (bool, error) {
+		if err := kclient.Get(context.TODO(), svcName, service); err != nil {
+			t.Logf("failed to get service %q: %v", svcName, err)
+			return false, nil
+		}
+		ports = sets.String{}
+		for _, port := range service.Spec.Ports {
+			ports.Insert(port.Name)
+		}
+		return ports.Equal(expectedPorts), nil
+	})
+	if !ports.Equal(expectedPorts) {
+		t.Fatalf("after deleting the \"metrics\" port, expected service %q to have ports %v, got %v", svcName, expectedPorts.List(), ports.List())
+	}
 }
 
 // TestTLSSecurityProfile creates an ingresscontroller with no explicit TLS