Allow NodePort Port to be Specified via the PostgresCluster Spec

This update allows a specific NodePort port to be specified for
the primary Postgres, pgBouncer and pgAdmin services via the
PostgresCluster spec.

Note this is used when type is NodePort or LoadBalancer only.
Setting this value when using the 'ClusterIP' type will result
in an error. The specified value must be also be in-range and
not currently in use or the operation will fail. If unspecified,
a port will be allocated if this Service requires one as before.

Resolves #3008
Issue: [sc-14918]

Author: tjmoore4

config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml

@@ -10426,6 +10426,14 @@ spec:
                       service:
                         description: Specification of the service that exposes PgBouncer.
                         properties:
+                          nodePort:
+                            description: The port on which this service is exposed
+                              when type is NodePort or LoadBalancer. Value must be
+                              in-range and not in use or the operation will fail.
+                              If unspecified, a port will be allocated if this Service
+                              requires one. - https://kubernetes.io/docs/concepts/services-networking/service/#type-nodeport
+                            format: int32
+                            type: integer
                           type:
                             description: 'More info: https://kubernetes.io/docs/concepts/services-networking/service/#publishing-services-service-types'
                             enum:
@@ -10633,6 +10641,13 @@ spec:
                 description: Specification of the service that exposes the PostgreSQL
                   primary instance.
                 properties:
+                  nodePort:
+                    description: The port on which this service is exposed when type
+                      is NodePort or LoadBalancer. Value must be in-range and not
+                      in use or the operation will fail. If unspecified, a port will
+                      be allocated if this Service requires one. - https://kubernetes.io/docs/concepts/services-networking/service/#type-nodeport
+                    format: int32
+                    type: integer
                   type:
                     description: 'More info: https://kubernetes.io/docs/concepts/services-networking/service/#publishing-services-service-types'
                     enum:
@@ -11795,6 +11810,14 @@ spec:
                       service:
                         description: Specification of the service that exposes pgAdmin.
                         properties:
+                          nodePort:
+                            description: The port on which this service is exposed
+                              when type is NodePort or LoadBalancer. Value must be
+                              in-range and not in use or the operation will fail.
+                              If unspecified, a port will be allocated if this Service
+                              requires one. - https://kubernetes.io/docs/concepts/services-networking/service/#type-nodeport
+                            format: int32
+                            type: integer
                           type:
                             description: 'More info: https://kubernetes.io/docs/concepts/services-networking/service/#publishing-services-service-types'
                             enum:

docs/content/references/crd.md

@@ -42,22 +42,28 @@ All connections are over TLS. PGO provides its own certificate authority (CA) to
 
 ### Modifying Service Type
 
-By default, PGO deploys Services with the `ClusterIP` Service type. Based on how you want to expose your database, you may want to modify the Services to use a different [Service type](https://kubernetes.io/docs/concepts/services-networking/service/#publishing-services-service-types).
+By default, PGO deploys Services with the `ClusterIP` Service type. Based on how you want to expose your database,
+you may want to modify the Services to use a different
+[Service type](https://kubernetes.io/docs/concepts/services-networking/service/#publishing-services-service-types)
+and [NodePort value](https://kubernetes.io/docs/concepts/services-networking/service/#type-nodeport).
 
 You can modify the Services that PGO manages from the following attributes:
 
 - `spec.service` - this manages the Service for connecting to a Postgres primary.
 - `spec.proxy.pgBouncer.service` - this manages the Service for connecting to the PgBouncer connection pooler.
+- `spec.userInterface.pgAdmin.service` - this manages the Service for connecting to the pgAdmin management tool.
 
-For example, to set the Postgres primary to use a `NodePort` service, you would add the following to your manifest:
+For example, to set the Postgres primary to use a `NodePort` service and specific `nodePort` value, you would add the
+following to your manifest:
 
 ```yaml
 spec:
   service:
     type: NodePort
+    nodePort: 32000
 ```
 
-For our `hippo` cluster, you would see the Service type modification in the . For example:
+For our `hippo` cluster, you would see the Service type and nodePort modification. For example:
 
 ```
 kubectl -n postgres-operator get svc --selector=postgres-operator.crunchydata.com/cluster=hippo
@@ -66,15 +72,18 @@ kubectl -n postgres-operator get svc --selector=postgres-operator.crunchydata.co
 will yield something similar to:
 
 ```
-NAME              TYPE        CLUSTER-IP     EXTERNAL-IP   PORT(S)          AGE
-hippo-ha          NodePort    10.96.17.210   <none>        5432:32751/TCP   2m37s
-hippo-ha-config   ClusterIP   None           <none>        <none>           2m37s
-hippo-pods        ClusterIP   None           <none>        <none>           2m37s
-hippo-primary     ClusterIP   None           <none>        5432/TCP         2m37s
-hippo-replicas    ClusterIP   10.96.151.53   <none>        5432/TCP         2m37s
+NAME              TYPE        CLUSTER-IP      EXTERNAL-IP   PORT(S)          AGE
+hippo-ha          NodePort    10.105.57.191   <none>        5432:32000/TCP   48s
+hippo-ha-config   ClusterIP   None            <none>        <none>           48s
+hippo-pods        ClusterIP   None            <none>        <none>           48s
+hippo-primary     ClusterIP   None            <none>        5432/TCP         48s
+hippo-replicas    ClusterIP   10.106.18.99    <none>        5432/TCP         48s
 ```
 
-(Note that if you are exposing your Services externally and are relying on TLS verification, you will need to use the [custom TLS]({{< relref "tutorial/customize-cluster.md" >}}#customize-tls) features of PGO).
+Note that setting the `nodePort` value is not allowed when using the `ClusterIP` type, and it must be in-range and
+not otherwise in use or the operation will fail. Also, if you are exposing your Services externally and are relying on TLS
+verification, you will need to use the [custom TLS]({{< relref "tutorial/customize-cluster.md" >}}#customize-tls)
+features of PGO).
 
 ## Connect an Application
 

docs/content/tutorial/connect-cluster.md

@@ -17,6 +17,7 @@ package postgrescluster
 
 import (
 	"context"
+	"fmt"
 	"io"
 	"time"
 
@@ -246,21 +247,37 @@ func (r *Reconciler) generatePatroniLeaderLeaseService(
 	// Patroni will ensure that they always route to the elected leader.
 	// - https://docs.k8s.io/concepts/services-networking/service/#services-without-selectors
 	service.Spec.Selector = nil
-	if cluster.Spec.Service != nil {
-		service.Spec.Type = corev1.ServiceType(cluster.Spec.Service.Type)
-	} else {
-		service.Spec.Type = corev1.ServiceTypeClusterIP
-	}
 
 	// The TargetPort must be the name (not the number) of the PostgreSQL
 	// ContainerPort. This name allows the port number to differ between
 	// instances, which can happen during a rolling update.
-	service.Spec.Ports = []corev1.ServicePort{{
+	servicePort := corev1.ServicePort{
 		Name:       naming.PortPostgreSQL,
 		Port:       *cluster.Spec.Port,
 		Protocol:   corev1.ProtocolTCP,
 		TargetPort: intstr.FromString(naming.PortPostgreSQL),
-	}}
+	}
+
+	if spec := cluster.Spec.Service; spec == nil {
+		service.Spec.Type = corev1.ServiceTypeClusterIP
+	} else {
+		service.Spec.Type = corev1.ServiceType(spec.Type)
+		if spec.NodePort != nil {
+			if service.Spec.Type == corev1.ServiceTypeClusterIP {
+				// The NodePort can only be set when the Service type is NodePort or
+				// LoadBalancer. However, due to a known issue prior to Kubernetes
+				// 1.20, we clear these errors during our apply. To preserve the
+				// appropriate behavior, we log an Event and return an error.
+				// TODO(tjmoore4): Once Validation Rules are available, this check
+				// and event could potentially be removed in favor of that validation
+				r.Recorder.Eventf(cluster, corev1.EventTypeWarning, "MisconfiguredClusterIP",
+					"NodePort cannot be set with type ClusterIP on Service %q", service.Name)
+				return nil, fmt.Errorf("NodePort cannot be set with type ClusterIP on Service %q", service.Name)
+			}
+			servicePort.NodePort = *spec.NodePort
+		}
+	}
+	service.Spec.Ports = []corev1.ServicePort{servicePort}
 
 	err := errors.WithStack(r.setControllerReference(cluster, service))
 	return service, err

internal/controller/postgrescluster/patroni.go

@@ -35,6 +35,7 @@ import (
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/client-go/tools/record"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
@@ -48,7 +49,10 @@ func TestGeneratePatroniLeaderLeaseService(t *testing.T) {
 	_, cc := setupKubernetes(t)
 	require.ParallelCapacity(t, 0)
 
-	reconciler := &Reconciler{Client: cc}
+	reconciler := &Reconciler{
+		Client:   cc,
+		Recorder: new(record.FakeRecorder),
+	}
 
 	cluster := &v1beta1.PostgresCluster{}
 	cluster.Namespace = "ns1"
@@ -75,12 +79,6 @@ ownerReferences:
   name: pg2
   uid: ""
 		`))
-		assert.Assert(t, marshalMatches(service.Spec.Ports, `
-- name: postgres
-  port: 9876
-  protocol: TCP
-  targetPort: postgres
-		`))
 
 		// Always gets a ClusterIP (never None).
 		assert.Equal(t, service.Spec.ClusterIP, "")
@@ -92,9 +90,14 @@ ownerReferences:
 		service, err := reconciler.generatePatroniLeaderLeaseService(cluster)
 		assert.NilError(t, err)
 		alwaysExpect(t, service)
-
 		// Defaults to ClusterIP.
 		assert.Equal(t, service.Spec.Type, corev1.ServiceTypeClusterIP)
+		assert.Assert(t, marshalMatches(service.Spec.Ports, `
+- name: postgres
+  port: 9876
+  protocol: TCP
+  targetPort: postgres
+		`))
 	})
 
 	t.Run("AnnotationsLabels", func(t *testing.T) {
@@ -148,6 +151,61 @@ ownerReferences:
 			assert.NilError(t, err)
 			alwaysExpect(t, service)
 			test.Expect(t, service)
+			assert.Assert(t, marshalMatches(service.Spec.Ports, `
+- name: postgres
+  port: 9876
+  protocol: TCP
+  targetPort: postgres
+		`))
+		})
+	}
+
+	typesAndPort := []struct {
+		Description string
+		Type        string
+		NodePort    *int32
+		Expect      func(testing.TB, *corev1.Service, error)
+	}{
+		{Description: "ClusterIP with Port 32000", Type: "ClusterIP",
+			NodePort: initialize.Int32(32000), Expect: func(t testing.TB, service *corev1.Service, err error) {
+				assert.ErrorContains(t, err, "NodePort cannot be set with type ClusterIP on Service \"pg2-ha\"")
+				assert.Assert(t, service == nil)
+			}},
+		{Description: "NodePort with Port 32001", Type: "NodePort",
+			NodePort: initialize.Int32(32001), Expect: func(t testing.TB, service *corev1.Service, err error) {
+				assert.NilError(t, err)
+				alwaysExpect(t, service)
+				assert.Equal(t, service.Spec.Type, corev1.ServiceTypeNodePort)
+				assert.Assert(t, marshalMatches(service.Spec.Ports, `
+- name: postgres
+  nodePort: 32001
+  port: 9876
+  protocol: TCP
+  targetPort: postgres
+`))
+			}},
+		{Description: "LoadBalancer with Port 32002", Type: "LoadBalancer",
+			NodePort: initialize.Int32(32002), Expect: func(t testing.TB, service *corev1.Service, err error) {
+				assert.Equal(t, service.Spec.Type, corev1.ServiceTypeLoadBalancer)
+				assert.NilError(t, err)
+				alwaysExpect(t, service)
+				assert.Assert(t, marshalMatches(service.Spec.Ports, `
+- name: postgres
+  nodePort: 32002
+  port: 9876
+  protocol: TCP
+  targetPort: postgres
+`))
+			}},
+	}
+
+	for _, test := range typesAndPort {
+		t.Run(test.Description, func(t *testing.T) {
+			cluster := cluster.DeepCopy()
+			cluster.Spec.Service = &v1beta1.ServiceSpec{Type: test.Type, NodePort: test.NodePort}
+
+			service, err := reconciler.generatePatroniLeaderLeaseService(cluster)
+			test.Expect(t, service, err)
 		})
 	}
 }

internal/controller/postgrescluster/patroni_test.go

@@ -149,24 +149,40 @@ func (r *Reconciler) generatePGAdminService(
 		naming.LabelCluster: cluster.Name,
 		naming.LabelRole:    naming.RolePGAdmin,
 	}
-	if spec := cluster.Spec.UserInterface.PGAdmin.Service; spec != nil {
-		service.Spec.Type = corev1.ServiceType(spec.Type)
-	} else {
-		service.Spec.Type = corev1.ServiceTypeClusterIP
-	}
 
 	// The TargetPort must be the name (not the number) of the pgAdmin
 	// ContainerPort. This name allows the port number to differ between Pods,
 	// which can happen during a rolling update.
 	//
 	// TODO(tjmoore4): A custom service port is not currently supported as this
 	// requires updates to the pgAdmin service configuration.
-	service.Spec.Ports = []corev1.ServicePort{{
+	servicePort := corev1.ServicePort{
 		Name:       naming.PortPGAdmin,
 		Port:       *initialize.Int32(5050),
 		Protocol:   corev1.ProtocolTCP,
 		TargetPort: intstr.FromString(naming.PortPGAdmin),
-	}}
+	}
+
+	if spec := cluster.Spec.UserInterface.PGAdmin.Service; spec == nil {
+		service.Spec.Type = corev1.ServiceTypeClusterIP
+	} else {
+		service.Spec.Type = corev1.ServiceType(spec.Type)
+		if spec.NodePort != nil {
+			if service.Spec.Type == corev1.ServiceTypeClusterIP {
+				// The NodePort can only be set when the Service type is NodePort or
+				// LoadBalancer. However, due to a known issue prior to Kubernetes
+				// 1.20, we clear these errors during our apply. To preserve the
+				// appropriate behavior, we log an Event and return an error.
+				// TODO(tjmoore4): Once Validation Rules are available, this check
+				// and event could potentially be removed in favor of that validation
+				r.Recorder.Eventf(cluster, corev1.EventTypeWarning, "MisconfiguredClusterIP",
+					"NodePort cannot be set with type ClusterIP on Service %q", service.Name)
+				return nil, true, fmt.Errorf("NodePort cannot be set with type ClusterIP on Service %q", service.Name)
+			}
+			servicePort.NodePort = *spec.NodePort
+		}
+	}
+	service.Spec.Ports = []corev1.ServicePort{servicePort}
 
 	err := errors.WithStack(r.setControllerReference(cluster, service))