add enableConnectionPool to validation and some nior fixes

charts/postgres-operator/crds/operatorconfigurations.yaml

@@ -327,7 +327,7 @@ spec:
                 connection_pool_user:
                   type: string
                   #default: "pooler"
-                connection_pool_instances_number:
+                connection_pool_replicas:
                   type: integer
                   #default: 1
                 connection_pool_image:
@@ -354,7 +354,7 @@ spec:
                 connection_pool_default_memory_request:
                   type: string
                   pattern: '^(\d+(e\d+)?|\d+(\.\d+)?(e\d+)?[EPTGMK]i?)$'
-                  #default: "100m"
+                  #default: "100Mi"
         status:
           type: object
           additionalProperties:

charts/postgres-operator/crds/postgresqls.yaml

@@ -157,6 +157,8 @@ spec:
               # Note: usernames specified here as database owners must be declared in the users key of the spec key.
             dockerImage:
               type: string
+            enableConnectionPool:
+              type: boolean
             enableLogicalBackup:
               type: boolean
             enableMasterLoadBalancer:

docs/user.md

@@ -494,10 +494,10 @@ spec:
     resources:
       requests:
         cpu: "100m"
-        memory: "100M"
+        memory: "100Mi"
       limits:
         cpu: "100m"
-        memory: "100M"
+        memory: "100Mi"
 ```
 
 By default `pgbouncer` is used to create a connection pool. To find out about

e2e/tests/test_e2e.py

@@ -270,7 +270,16 @@ class EndToEndTestCase(unittest.TestCase):
         }
         k8s.update_config(patch_custom_service_annotations)
 
-        k8s.create_with_kubectl("manifests/postgres-manifest-with-service-annotations.yaml")
+        pg_patch_custom_annotations = {
+            "spec": {
+                "serviceAnnotations": {
+                    "annotation.key": "value"
+                }
+            }
+        }
+        k8s.api.custom_objects_api.patch_namespaced_custom_object(
+            "acid.zalan.do", "v1", "default", "postgresqls", "acid-minimal-cluster", pg_patch_custom_annotations)
+
         annotations = {
             "annotation.key": "value",
             "foo": "bar",

manifests/configmap.yaml

@@ -16,7 +16,7 @@ data:
   # connection_pool_default_memory_limit: 100m
   # connection_pool_default_memory_request: "100Mi"
   # connection_pool_image: ""
-  # connection_pool_instances_number: 1
+  # connection_pool_replicas: 1
   # connection_pool_mode: "transaction"
   # connection_pool_schema: "pooler"
   # connection_pool_user: "pooler"

manifests/minimal-postgres-manifest.yaml

@@ -17,5 +17,4 @@ spec:
     foo: zalando  # dbname: owner
   postgresql:
     version: "11"
-  connectionPool:
+  enableConnectionPool: true
-    type: "pgbouncer"

manifests/operatorconfiguration.crd.yaml

@@ -303,7 +303,7 @@ spec:
                 connection_pool_user:
                   type: string
                   #default: "pooler"
-                connection_pool_instances_number:
+                connection_pool_replicas:
                   type: integer
                   #default: 1
                 connection_pool_image:

manifests/postgres-manifest-with-service-annotations.yaml

@@ -1,20 +0,0 @@
-apiVersion: "acid.zalan.do/v1"
-kind: postgresql
-metadata:
-  name: acid-service-annotations
-spec:
-  teamId: "acid"
-  volume:
-    size: 1Gi
-  numberOfInstances: 2
-  users:
-    zalando:  # database owner
-    - superuser
-    - createdb
-    foo_user: []  # role for application foo
-  databases:
-    foo: zalando  # dbname: owner
-  postgresql:
-    version: "11"
-  serviceAnnotations:
-    annotation.key: value

manifests/postgresql-operator-default-configuration.yaml

@@ -127,7 +127,7 @@ configuration:
     connection_pool_default_memory_limit: 100m
     connection_pool_default_memory_request: "100Mi"
     # connection_pool_image: ""
-    connection_pool_instances_number: 1
+    connection_pool_replicas: 1
     connection_pool_mode: "transaction"
     # connection_pool_schema: "pooler"
     # connection_pool_user: "pooler"

manifests/postgresql.crd.yaml

@@ -121,6 +121,8 @@ spec:
               # Note: usernames specified here as database owners must be declared in the users key of the spec key.
             dockerImage:
               type: string
+            enableConnectionPool:
+              type: boolean
             enableLogicalBackup:
               type: boolean
             enableMasterLoadBalancer:

pkg/apis/acid.zalan.do/v1/crds.go

@@ -247,6 +247,9 @@ var PostgresCRDResourceValidation = apiextv1beta1.CustomResourceValidation{
 					"dockerImage": {
 						Type: "string",
 					},
+					"enableConnectionPool": {
+						Type: "boolean",
+					},
 					"enableLogicalBackup": {
 						Type: "boolean",
 					},
@@ -1118,7 +1121,7 @@ var OperatorConfigCRDResourceValidation = apiextv1beta1.CustomResourceValidation
 							"connection_pool_image": {
 								Type: "string",
 							},
-							"connection_pool_instances_number": {
+							"connection_pool_replicas": {
 								Type:    "integer",
 								Minimum: &min1,
 							},

pkg/util/k8sutil/k8sutil.go

@@ -9,7 +9,6 @@ import (
 	batchv1beta1 "k8s.io/api/batch/v1beta1"
 	clientbatchv1beta1 "k8s.io/client-go/kubernetes/typed/batch/v1beta1"
 
-	"github.com/zalando/postgres-operator/pkg/util/constants"
 	apiappsv1 "k8s.io/api/apps/v1"
 	v1 "k8s.io/api/core/v1"
 	policybeta1 "k8s.io/api/policy/v1beta1"
@@ -172,21 +171,37 @@ func SameService(cur, new *v1.Service) (match bool, reason string) {
 		}
 	}
 
-	oldDNSAnnotation := cur.Annotations[constants.ZalandoDNSNameAnnotation]
+	match = true
-	newDNSAnnotation := new.Annotations[constants.ZalandoDNSNameAnnotation]
-	oldELBAnnotation := cur.Annotations[constants.ElbTimeoutAnnotationName]
-	newELBAnnotation := new.Annotations[constants.ElbTimeoutAnnotationName]
 
-	if oldDNSAnnotation != newDNSAnnotation {
+	reasonPrefix := "new service's annotations doesn't match the current one:"
-		return false, fmt.Sprintf("new service's %q annotation value %q doesn't match the current one %q",
+	for ann := range cur.Annotations {
-			constants.ZalandoDNSNameAnnotation, newDNSAnnotation, oldDNSAnnotation)
+		if _, ok := new.Annotations[ann]; !ok {
-	}
+			match = false
-	if oldELBAnnotation != newELBAnnotation {
+			if len(reason) == 0 {
-		return false, fmt.Sprintf("new service's %q annotation value %q doesn't match the current one %q",
+				reason = reasonPrefix
-			constants.ElbTimeoutAnnotationName, oldELBAnnotation, newELBAnnotation)
+			}
+			reason += fmt.Sprintf(" Removed '%s'.", ann)
+		}
 	}
 
-	return true, ""
+	for ann := range new.Annotations {
+		v, ok := cur.Annotations[ann]
+		if !ok {
+			if len(reason) == 0 {
+				reason = reasonPrefix
+			}
+			reason += fmt.Sprintf(" Added '%s' with value '%s'.", ann, new.Annotations[ann])
+			match = false
+		} else if v != new.Annotations[ann] {
+			if len(reason) == 0 {
+				reason = reasonPrefix
+			}
+			reason += fmt.Sprintf(" '%s' changed from '%s' to '%s'.", ann, v, new.Annotations[ann])
+			match = false
+		}
+	}
+
+	return match, reason
 }
 
 // SamePDB compares the PodDisruptionBudgets

pkg/util/k8sutil/k8sutil_test.go

@@ -0,0 +1,310 @@
+package k8sutil
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/zalando/postgres-operator/pkg/util/constants"
+
+	v1 "k8s.io/api/core/v1"
+)
+
+func newsService(ann map[string]string, svcT v1.ServiceType, lbSr []string) *v1.Service {
+	svc := &v1.Service{
+		Spec: v1.ServiceSpec{
+			Type:                     svcT,
+			LoadBalancerSourceRanges: lbSr,
+		},
+	}
+	svc.Annotations = ann
+	return svc
+}
+
+func TestServiceAnnotations(t *testing.T) {
+	tests := []struct {
+		about   string
+		current *v1.Service
+		new     *v1.Service
+		reason  string
+		match   bool
+	}{
+		{
+			about: "two equal services",
+			current: newsService(
+				map[string]string{
+					constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do",
+					constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue,
+				},
+				v1.ServiceTypeClusterIP,
+				[]string{"128.141.0.0/16", "137.138.0.0/16"}),
+			new: newsService(
+				map[string]string{
+					constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do",
+					constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue,
+				},
+				v1.ServiceTypeClusterIP,
+				[]string{"128.141.0.0/16", "137.138.0.0/16"}),
+			match: true,
+		},
+		{
+			about: "services differ on service type",
+			current: newsService(
+				map[string]string{
+					constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do",
+					constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue,
+				},
+				v1.ServiceTypeClusterIP,
+				[]string{"128.141.0.0/16", "137.138.0.0/16"}),
+			new: newsService(
+				map[string]string{
+					constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do",
+					constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue,
+				},
+				v1.ServiceTypeLoadBalancer,
+				[]string{"128.141.0.0/16", "137.138.0.0/16"}),
+			match:  false,
+			reason: `new service's type "LoadBalancer" doesn't match the current one "ClusterIP"`,
+		},
+		{
+			about: "services differ on lb source ranges",
+			current: newsService(
+				map[string]string{
+					constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do",
+					constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue,
+				},
+				v1.ServiceTypeLoadBalancer,
+				[]string{"128.141.0.0/16", "137.138.0.0/16"}),
+			new: newsService(
+				map[string]string{
+					constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do",
+					constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue,
+				},
+				v1.ServiceTypeLoadBalancer,
+				[]string{"185.249.56.0/22"}),
+			match:  false,
+			reason: `new service's LoadBalancerSourceRange doesn't match the current one`,
+		},
+		{
+			about: "new service doesn't have lb source ranges",
+			current: newsService(
+				map[string]string{
+					constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do",
+					constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue,
+				},
+				v1.ServiceTypeLoadBalancer,
+				[]string{"128.141.0.0/16", "137.138.0.0/16"}),
+			new: newsService(
+				map[string]string{
+					constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do",
+					constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue,
+				},
+				v1.ServiceTypeLoadBalancer,
+				[]string{}),
+			match:  false,
+			reason: `new service's LoadBalancerSourceRange doesn't match the current one`,
+		},
+		{
+			about: "services differ on DNS annotation",
+			current: newsService(
+				map[string]string{
+					constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do",
+					constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue,
+				},
+				v1.ServiceTypeLoadBalancer,
+				[]string{"128.141.0.0/16", "137.138.0.0/16"}),
+			new: newsService(
+				map[string]string{
+					constants.ZalandoDNSNameAnnotation: "new_clstr.acid.zalan.do",
+					constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue,
+				},
+				v1.ServiceTypeLoadBalancer,
+				[]string{"128.141.0.0/16", "137.138.0.0/16"}),
+			match:  false,
+			reason: `new service's annotations doesn't match the current one: 'external-dns.alpha.kubernetes.io/hostname' changed from 'clstr.acid.zalan.do' to 'new_clstr.acid.zalan.do'.`,
+		},
+		{
+			about: "services differ on AWS ELB annotation",
+			current: newsService(
+				map[string]string{
+					constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do",
+					constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue,
+				},
+				v1.ServiceTypeLoadBalancer,
+				[]string{"128.141.0.0/16", "137.138.0.0/16"}),
+			new: newsService(
+				map[string]string{
+					constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do",
+					constants.ElbTimeoutAnnotationName: "1800",
+				},
+				v1.ServiceTypeLoadBalancer,
+				[]string{"128.141.0.0/16", "137.138.0.0/16"}),
+			match:  false,
+			reason: `new service's annotations doesn't match the current one: 'service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout' changed from '3600' to '1800'.`,
+		},
+		{
+			about: "service changes existing annotation",
+			current: newsService(
+				map[string]string{
+					constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do",
+					constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue,
+					"foo":                              "bar",
+				},
+				v1.ServiceTypeLoadBalancer,
+				[]string{"128.141.0.0/16", "137.138.0.0/16"}),
+			new: newsService(
+				map[string]string{
+					constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do",
+					constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue,
+					"foo":                              "baz",
+				},
+				v1.ServiceTypeLoadBalancer,
+				[]string{"128.141.0.0/16", "137.138.0.0/16"}),
+			match:  false,
+			reason: `new service's annotations doesn't match the current one: 'foo' changed from 'bar' to 'baz'.`,
+		},
+		{
+			about: "service changes multiple existing annotations",
+			current: newsService(
+				map[string]string{
+					constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do",
+					constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue,
+					"foo":                              "bar",
+					"bar":                              "foo",
+				},
+				v1.ServiceTypeLoadBalancer,
+				[]string{"128.141.0.0/16", "137.138.0.0/16"}),
+			new: newsService(
+				map[string]string{
+					constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do",
+					constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue,
+					"foo":                              "baz",
+					"bar":                              "fooz",
+				},
+				v1.ServiceTypeLoadBalancer,
+				[]string{"128.141.0.0/16", "137.138.0.0/16"}),
+			match: false,
+			// Test just the prefix to avoid flakiness and map sorting
+			reason: `new service's annotations doesn't match the current one:`,
+		},
+		{
+			about: "service adds a new custom annotation",
+			current: newsService(
+				map[string]string{
+					constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do",
+					constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue,
+				},
+				v1.ServiceTypeLoadBalancer,
+				[]string{"128.141.0.0/16", "137.138.0.0/16"}),
+			new: newsService(
+				map[string]string{
+					constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do",
+					constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue,
+					"foo":                              "bar",
+				},
+				v1.ServiceTypeLoadBalancer,
+				[]string{"128.141.0.0/16", "137.138.0.0/16"}),
+			match:  false,
+			reason: `new service's annotations doesn't match the current one: Added 'foo' with value 'bar'.`,
+		},
+		{
+			about: "service removes a custom annotation",
+			current: newsService(
+				map[string]string{
+					constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do",
+					constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue,
+					"foo":                              "bar",
+				},
+				v1.ServiceTypeLoadBalancer,
+				[]string{"128.141.0.0/16", "137.138.0.0/16"}),
+			new: newsService(
+				map[string]string{
+					constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do",
+					constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue,
+				},
+				v1.ServiceTypeLoadBalancer,
+				[]string{"128.141.0.0/16", "137.138.0.0/16"}),
+			match:  false,
+			reason: `new service's annotations doesn't match the current one: Removed 'foo'.`,
+		},
+		{
+			about: "service removes a custom annotation and adds a new one",
+			current: newsService(
+				map[string]string{
+					constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do",
+					constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue,
+					"foo":                              "bar",
+				},
+				v1.ServiceTypeLoadBalancer,
+				[]string{"128.141.0.0/16", "137.138.0.0/16"}),
+			new: newsService(
+				map[string]string{
+					constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do",
+					constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue,
+					"bar":                              "foo",
+				},
+				v1.ServiceTypeLoadBalancer,
+				[]string{"128.141.0.0/16", "137.138.0.0/16"}),
+			match:  false,
+			reason: `new service's annotations doesn't match the current one: Removed 'foo'. Added 'bar' with value 'foo'.`,
+		},
+		{
+			about: "service removes a custom annotation, adds a new one and change another",
+			current: newsService(
+				map[string]string{
+					constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do",
+					constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue,
+					"foo":                              "bar",
+					"zalan":                            "do",
+				},
+				v1.ServiceTypeLoadBalancer,
+				[]string{"128.141.0.0/16", "137.138.0.0/16"}),
+			new: newsService(
+				map[string]string{
+					constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do",
+					constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue,
+					"bar":                              "foo",
+					"zalan":                            "do.com",
+				},
+				v1.ServiceTypeLoadBalancer,
+				[]string{"128.141.0.0/16", "137.138.0.0/16"}),
+			match:  false,
+			reason: `new service's annotations doesn't match the current one: Removed 'foo'. Added 'bar' with value 'foo'. 'zalan' changed from 'do' to 'do.com'`,
+		},
+		{
+			about: "service add annotations",
+			current: newsService(
+				map[string]string{},
+				v1.ServiceTypeLoadBalancer,
+				[]string{"128.141.0.0/16", "137.138.0.0/16"}),
+			new: newsService(
+				map[string]string{
+					constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do",
+					constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue,
+				},
+				v1.ServiceTypeLoadBalancer,
+				[]string{"128.141.0.0/16", "137.138.0.0/16"}),
+			match: false,
+			// Test just the prefix to avoid flakiness and map sorting
+			reason: `new service's annotations doesn't match the current one: Added `,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.about, func(t *testing.T) {
+			match, reason := SameService(tt.current, tt.new)
+			if match && !tt.match {
+				t.Errorf("expected services to do not match: '%q' and '%q'", tt.current, tt.new)
+				return
+			}
+			if !match && tt.match {
+				t.Errorf("expected services to be the same: '%q' and '%q'", tt.current, tt.new)
+				return
+			}
+			if !match && !tt.match {
+				if !strings.HasPrefix(reason, tt.reason) {
+					t.Errorf("expected reason '%s', found '%s'", tt.reason, reason)
+					return
+				}
+			}
+		})
+	}
+}