diff --git a/percona/controller/pgcluster/controller.go b/percona/controller/pgcluster/controller.go index 1237ebf6dc..4598d95981 100644 --- a/percona/controller/pgcluster/controller.go +++ b/percona/controller/pgcluster/controller.go @@ -599,7 +599,7 @@ func (r *PGClusterReconciler) reconcileEnvFromSecrets(ctx context.Context, cr *v m[&set.EnvFrom] = set.Metadata } - if len(cr.Spec.Proxy.PGBouncer.EnvFrom) > 0 { + if cr.Spec.Proxy.IsSet() && len(cr.Spec.Proxy.PGBouncer.EnvFrom) > 0 { if cr.Spec.Proxy.PGBouncer.Metadata == nil { cr.Spec.Proxy.PGBouncer.Metadata = new(v1beta1.Metadata) } @@ -608,7 +608,7 @@ func (r *PGClusterReconciler) reconcileEnvFromSecrets(ctx context.Context, cr *v if len(cr.Spec.Backups.PGBackRest.EnvFrom) > 0 { if cr.Spec.Backups.PGBackRest.Metadata == nil { - cr.Spec.Proxy.PGBouncer.Metadata = new(v1beta1.Metadata) + cr.Spec.Backups.PGBackRest.Metadata = new(v1beta1.Metadata) } m[&cr.Spec.Backups.PGBackRest.EnvFrom] = cr.Spec.Backups.PGBackRest.Metadata } diff --git a/percona/controller/pgcluster/gethost_test.go b/percona/controller/pgcluster/gethost_test.go new file mode 100644 index 0000000000..4f6eea5406 --- /dev/null +++ b/percona/controller/pgcluster/gethost_test.go @@ -0,0 +1,188 @@ +package pgcluster + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + v2 "github.com/percona/percona-postgresql-operator/v2/pkg/apis/pgv2.percona.com/v2" + "github.com/percona/percona-postgresql-operator/v2/pkg/apis/postgres-operator.crunchydata.com/v1beta1" +) + +func TestGetHost(t *testing.T) { + ctx := context.Background() + + const ( + clusterName = "test-cluster" + ns = "test-ns" + ) + + tests := []struct { + name string + proxy *v2.PGProxySpec + svc *corev1.Service + expectedHost string + expectErr bool + }{ + { + name: "proxy is nil", + proxy: nil, + expectedHost: clusterName + "-primary." + ns + ".svc", + }, + { + name: "proxy set but PGBouncer is nil", + proxy: &v2.PGProxySpec{PGBouncer: nil}, + expectedHost: clusterName + "-primary." + ns + ".svc", + }, + { + name: "PGBouncer configured, no ServiceExpose", + proxy: &v2.PGProxySpec{PGBouncer: &v2.PGBouncerSpec{}}, + expectedHost: clusterName + "-pgbouncer." + ns + ".svc", + }, + { + name: "PGBouncer configured, ServiceExpose is ClusterIP", + proxy: &v2.PGProxySpec{PGBouncer: &v2.PGBouncerSpec{ + ServiceExpose: &v2.ServiceExpose{Type: string(corev1.ServiceTypeClusterIP)}, + }}, + expectedHost: clusterName + "-pgbouncer." + ns + ".svc", + }, + { + name: "PGBouncer configured, ServiceExpose is NodePort", + proxy: &v2.PGProxySpec{PGBouncer: &v2.PGBouncerSpec{ + ServiceExpose: &v2.ServiceExpose{Type: string(corev1.ServiceTypeNodePort)}, + }}, + expectedHost: clusterName + "-pgbouncer." + ns + ".svc", + }, + { + name: "PGBouncer LoadBalancer with IP", + proxy: &v2.PGProxySpec{PGBouncer: &v2.PGBouncerSpec{ + ServiceExpose: &v2.ServiceExpose{Type: string(corev1.ServiceTypeLoadBalancer)}, + }}, + svc: &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: clusterName + "-pgbouncer", + Namespace: ns, + }, + Status: corev1.ServiceStatus{ + LoadBalancer: corev1.LoadBalancerStatus{ + Ingress: []corev1.LoadBalancerIngress{ + {IP: "10.0.0.1"}, + }, + }, + }, + }, + expectedHost: "10.0.0.1", + }, + { + name: "PGBouncer LoadBalancer with hostname", + proxy: &v2.PGProxySpec{PGBouncer: &v2.PGBouncerSpec{ + ServiceExpose: &v2.ServiceExpose{Type: string(corev1.ServiceTypeLoadBalancer)}, + }}, + svc: &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: clusterName + "-pgbouncer", + Namespace: ns, + }, + Status: corev1.ServiceStatus{ + LoadBalancer: corev1.LoadBalancerStatus{ + Ingress: []corev1.LoadBalancerIngress{ + {Hostname: "my-lb.example.com"}, + }, + }, + }, + }, + expectedHost: "my-lb.example.com", + }, + { + name: "PGBouncer LoadBalancer with both IP and hostname prefers hostname", + proxy: &v2.PGProxySpec{PGBouncer: &v2.PGBouncerSpec{ + ServiceExpose: &v2.ServiceExpose{Type: string(corev1.ServiceTypeLoadBalancer)}, + }}, + svc: &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: clusterName + "-pgbouncer", + Namespace: ns, + }, + Status: corev1.ServiceStatus{ + LoadBalancer: corev1.LoadBalancerStatus{ + Ingress: []corev1.LoadBalancerIngress{ + {IP: "10.0.0.1", Hostname: "my-lb.example.com"}, + }, + }, + }, + }, + expectedHost: "my-lb.example.com", + }, + { + name: "PGBouncer LoadBalancer with no ingress returns empty host", + proxy: &v2.PGProxySpec{PGBouncer: &v2.PGBouncerSpec{ + ServiceExpose: &v2.ServiceExpose{Type: string(corev1.ServiceTypeLoadBalancer)}, + }}, + svc: &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: clusterName + "-pgbouncer", + Namespace: ns, + }, + }, + expectedHost: "", + }, + { + name: "PGBouncer LoadBalancer service not found", + proxy: &v2.PGProxySpec{PGBouncer: &v2.PGBouncerSpec{ + ServiceExpose: &v2.ServiceExpose{Type: string(corev1.ServiceTypeLoadBalancer)}, + }}, + expectErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cr := &v2.PerconaPGCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: clusterName, + Namespace: ns, + }, + Spec: v2.PerconaPGClusterSpec{ + Proxy: tt.proxy, + }, + } + + var objs []client.Object + if tt.svc != nil { + objs = append(objs, tt.svc) + } + + s := scheme.Scheme + err := v1beta1.AddToScheme(s) + require.NoError(t, err) + err = v2.AddToScheme(s) + require.NoError(t, err) + + cl := fake.NewClientBuilder(). + WithScheme(s). + WithObjects(objs...). + WithStatusSubresource(objs...). + Build() + + r := &PGClusterReconciler{ + Client: cl, + } + + host, err := r.getHost(ctx, cr) + if tt.expectErr { + require.Error(t, err) + return + } + + require.NoError(t, err) + assert.Equal(t, tt.expectedHost, host) + }) + } +} diff --git a/percona/controller/pgcluster/status.go b/percona/controller/pgcluster/status.go index 1d99d968a3..ac6efcb418 100644 --- a/percona/controller/pgcluster/status.go +++ b/percona/controller/pgcluster/status.go @@ -2,6 +2,7 @@ package pgcluster import ( "context" + "fmt" "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" @@ -11,18 +12,36 @@ import ( "k8s.io/client-go/util/retry" "github.com/percona/percona-postgresql-operator/v2/internal/controller/postgrescluster" + "github.com/percona/percona-postgresql-operator/v2/internal/naming" pNaming "github.com/percona/percona-postgresql-operator/v2/percona/naming" v2 "github.com/percona/percona-postgresql-operator/v2/pkg/apis/pgv2.percona.com/v2" "github.com/percona/percona-postgresql-operator/v2/pkg/apis/postgres-operator.crunchydata.com/v1beta1" ) func (r *PGClusterReconciler) getHost(ctx context.Context, cr *v2.PerconaPGCluster) (string, error) { - svcName := cr.Name + "-pgbouncer" + postgresCluster := &v1beta1.PostgresCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: cr.Name, + Namespace: cr.Namespace, + }, + } + + svcFQDN := func(svcName, ns string) string { + return fmt.Sprintf("%s.%s.svc", svcName, ns) + } + + // If proxy is not configured, use the primary service as host. + if cr.Spec.Proxy == nil || cr.Spec.Proxy.PGBouncer == nil { + return svcFQDN(naming.ClusterPrimaryService(postgresCluster).Name, postgresCluster.Namespace), nil + } + // Proxy is configured, but PGBouncer is not exposed, use the service name as host. + svcName := naming.ClusterPGBouncer(postgresCluster).Name if cr.Spec.Proxy.PGBouncer.ServiceExpose == nil || cr.Spec.Proxy.PGBouncer.ServiceExpose.Type != string(corev1.ServiceTypeLoadBalancer) { - return svcName + "." + cr.Namespace + ".svc", nil + return svcFQDN(svcName, postgresCluster.Namespace), nil } + // PGBouncer is exposed, find the IP/hostnames svc := &corev1.Service{} err := r.Client.Get(ctx, types.NamespacedName{Namespace: cr.Namespace, Name: svcName}, svc) if err != nil { @@ -36,7 +55,6 @@ func (r *PGClusterReconciler) getHost(ctx context.Context, cr *v2.PerconaPGClust host = i.Hostname } } - return host, nil } diff --git a/pkg/apis/pgv2.percona.com/v2/perconapgcluster_types.go b/pkg/apis/pgv2.percona.com/v2/perconapgcluster_types.go index 4384177529..1efa51fc2d 100644 --- a/pkg/apis/pgv2.percona.com/v2/perconapgcluster_types.go +++ b/pkg/apis/pgv2.percona.com/v2/perconapgcluster_types.go @@ -212,21 +212,23 @@ func (cr *PerconaPGCluster) Default() { cr.Spec.InstanceSets[i].Metadata.Labels[LabelOperatorVersion] = cr.Spec.CRVersion } - if cr.Spec.Proxy == nil { - cr.Spec.Proxy = new(PGProxySpec) - } + if cr.CompareVersion("2.9.0") < 0 || cr.Spec.Proxy.IsSet() { + if cr.Spec.Proxy == nil { + cr.Spec.Proxy = &PGProxySpec{} + } - if cr.Spec.Proxy.PGBouncer == nil { - cr.Spec.Proxy.PGBouncer = new(PGBouncerSpec) - } + if cr.Spec.Proxy.PGBouncer == nil { + cr.Spec.Proxy.PGBouncer = &PGBouncerSpec{} + } - if cr.Spec.Proxy.PGBouncer.Metadata == nil { - cr.Spec.Proxy.PGBouncer.Metadata = new(crunchyv1beta1.Metadata) - } - if cr.Spec.Proxy.PGBouncer.Metadata.Labels == nil { - cr.Spec.Proxy.PGBouncer.Metadata.Labels = make(map[string]string) + if cr.Spec.Proxy.PGBouncer.Metadata == nil { + cr.Spec.Proxy.PGBouncer.Metadata = &crunchyv1beta1.Metadata{} + } + if cr.Spec.Proxy.PGBouncer.Metadata.Labels == nil { + cr.Spec.Proxy.PGBouncer.Metadata.Labels = make(map[string]string) + } + cr.Spec.Proxy.PGBouncer.Metadata.Labels[LabelOperatorVersion] = cr.Spec.CRVersion } - cr.Spec.Proxy.PGBouncer.Metadata.Labels[LabelOperatorVersion] = cr.Spec.CRVersion t := true f := false @@ -1040,6 +1042,10 @@ type PGProxySpec struct { PGBouncer *PGBouncerSpec `json:"pgBouncer"` } +func (p *PGProxySpec) IsSet() bool { + return p != nil && p.PGBouncer != nil +} + func (p *PGProxySpec) ToCrunchy(version string) *crunchyv1beta1.PostgresProxySpec { if p == nil { return nil diff --git a/pkg/apis/pgv2.percona.com/v2/perconapgcluster_types_test.go b/pkg/apis/pgv2.percona.com/v2/perconapgcluster_types_test.go index 5f9cfd68be..7e663f3ad2 100644 --- a/pkg/apis/pgv2.percona.com/v2/perconapgcluster_types_test.go +++ b/pkg/apis/pgv2.percona.com/v2/perconapgcluster_types_test.go @@ -6,8 +6,8 @@ import ( "os" "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "gotest.tools/v3/assert" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -55,6 +55,38 @@ func TestPerconaPGCluster_BackupsEnabled(t *testing.T) { } } +func TestPerconaPGCluster_Proxy(t *testing.T) { + t.Run("Proxy is nil, CRVersion < 2.9.0", func(t *testing.T) { + cr := new(PerconaPGCluster) + cr.Spec.CRVersion = "2.8.0" + cr.Default() + assert.NotNil(t, cr.Spec.Proxy) + assert.NotNil(t, cr.Spec.Proxy.PGBouncer) + assert.NotNil(t, cr.Spec.Proxy.PGBouncer.Metadata) + assert.NotNil(t, cr.Spec.Proxy.PGBouncer.Metadata.Labels) + assert.Equal(t, cr.Spec.CRVersion, cr.Spec.Proxy.PGBouncer.Metadata.Labels[LabelOperatorVersion]) + }) + t.Run("Proxy is nil, CRVersion >= 2.9.0", func(t *testing.T) { + cr := new(PerconaPGCluster) + cr.Spec.CRVersion = "2.9.0" + cr.Default() + assert.Nil(t, cr.Spec.Proxy) + }) + t.Run("Proxy is not nil", func(t *testing.T) { + cr := new(PerconaPGCluster) + cr.Spec.Proxy = &PGProxySpec{ + PGBouncer: &PGBouncerSpec{}, + } + cr.Spec.CRVersion = "2.9.0" + cr.Default() + assert.NotNil(t, cr.Spec.Proxy) + assert.NotNil(t, cr.Spec.Proxy.PGBouncer) + assert.NotNil(t, cr.Spec.Proxy.PGBouncer.Metadata) + assert.NotNil(t, cr.Spec.Proxy.PGBouncer.Metadata.Labels) + assert.Equal(t, cr.Spec.CRVersion, cr.Spec.Proxy.PGBouncer.Metadata.Labels[LabelOperatorVersion]) + }) +} + func TestPerconaPGCluster_PostgresImage(t *testing.T) { cluster := new(PerconaPGCluster) cluster.Default() @@ -108,7 +140,7 @@ func TestPerconaPGCluster_PostgresImage(t *testing.T) { }() } - assert.Equal(t, cluster.PostgresImage(), tt.expectedImage) + assert.Equal(t, tt.expectedImage, cluster.PostgresImage()) }) } } @@ -167,23 +199,24 @@ func TestPerconaPGCluster_ToCrunchy(t *testing.T) { }, }, assertClusterFunc: func(t *testing.T, actual *crunchyv1beta1.PostgresCluster, expected *PerconaPGCluster) { - assert.Equal(t, actual.Name, expected.Name) - assert.Equal(t, actual.Namespace, expected.Namespace) - assert.DeepEqual(t, actual.Finalizers, []string{naming.Finalizer}) - assert.Equal(t, actual.Spec.PostgresVersion, expected.Spec.PostgresVersion) - assert.Equal(t, actual.Labels[LabelOperatorVersion], expected.Spec.CRVersion) - assert.Equal(t, len(actual.Spec.InstanceSets), 1) - assert.Equal(t, len(actual.Spec.InstanceSets), len(expected.Spec.InstanceSets)) - assert.Equal(t, actual.Spec.InstanceSets[0].Name, expected.Spec.InstanceSets[0].Name) - - assert.Equal(t, actual.Spec.Service.Type, expected.Spec.Expose.Type) - assert.Assert(t, actual.Spec.Service.LoadBalancerClass != nil) - assert.Equal(t, actual.Spec.Service.LoadBalancerClass, expected.Spec.Expose.LoadBalancerClass) - - assert.Equal(t, actual.Spec.ReplicaService.Type, expected.Spec.ExposeReplicas.Type) - assert.Assert(t, actual.Spec.ReplicaService.LoadBalancerClass != nil) - assert.Equal(t, actual.Spec.ReplicaService.LoadBalancerClass, expected.Spec.ExposeReplicas.LoadBalancerClass) - assert.Equal(t, *actual.Spec.Backups.TrackLatestRestorableTime, true) + assert.Equal(t, expected.Name, actual.Name) + assert.Equal(t, expected.Namespace, actual.Namespace) + assert.Equal(t, []string{naming.Finalizer}, actual.Finalizers) + assert.Equal(t, expected.Spec.PostgresVersion, actual.Spec.PostgresVersion) + assert.Equal(t, expected.Spec.CRVersion, actual.Labels[LabelOperatorVersion]) + assert.Len(t, actual.Spec.InstanceSets, 1) + assert.Len(t, expected.Spec.InstanceSets, len(actual.Spec.InstanceSets)) + assert.Equal(t, expected.Spec.InstanceSets[0].Name, actual.Spec.InstanceSets[0].Name) + + assert.Equal(t, expected.Spec.Expose.Type, actual.Spec.Service.Type) + assert.NotNil(t, actual.Spec.Service.LoadBalancerClass) + assert.Equal(t, expected.Spec.Expose.LoadBalancerClass, actual.Spec.Service.LoadBalancerClass) + + assert.Equal(t, expected.Spec.ExposeReplicas.Type, actual.Spec.ReplicaService.Type) + assert.NotNil(t, actual.Spec.ReplicaService.LoadBalancerClass) + assert.Equal(t, expected.Spec.ExposeReplicas.LoadBalancerClass, actual.Spec.ReplicaService.LoadBalancerClass) + assert.NotNil(t, actual.Spec.Backups.TrackLatestRestorableTime) + assert.True(t, *actual.Spec.Backups.TrackLatestRestorableTime) }, }, "updates existing PostgresCluster": { @@ -228,11 +261,11 @@ func TestPerconaPGCluster_ToCrunchy(t *testing.T) { }, }, assertClusterFunc: func(t *testing.T, actual *crunchyv1beta1.PostgresCluster, expected *PerconaPGCluster) { - assert.Equal(t, actual.Spec.PostgresVersion, expected.Spec.PostgresVersion) - assert.Equal(t, actual.Spec.Port, expected.Spec.Port) - assert.Equal(t, actual.Spec.TLSOnly, expected.Spec.TLSOnly) - assert.Equal(t, actual.Labels["test-label"], "test-value") - assert.Equal(t, actual.Labels[LabelOperatorVersion], expected.Spec.CRVersion) + assert.Equal(t, expected.Spec.PostgresVersion, actual.Spec.PostgresVersion) + assert.Equal(t, expected.Spec.Port, actual.Spec.Port) + assert.Equal(t, expected.Spec.TLSOnly, actual.Spec.TLSOnly) + assert.Equal(t, "test-value", actual.Labels["test-label"]) + assert.Equal(t, expected.Spec.CRVersion, actual.Labels[LabelOperatorVersion]) }, }, "handles PMM enabled scenario": { @@ -274,7 +307,7 @@ func TestPerconaPGCluster_ToCrunchy(t *testing.T) { break } } - assert.Equal(t, hasMonitoringUser, true) + assert.True(t, hasMonitoringUser) }, }, "handles AutoCreateUserSchema annotation": { @@ -306,7 +339,7 @@ func TestPerconaPGCluster_ToCrunchy(t *testing.T) { }, }, assertClusterFunc: func(t *testing.T, actual *crunchyv1beta1.PostgresCluster, _ *PerconaPGCluster) { - assert.Equal(t, actual.Annotations[naming.AutoCreateUserSchemaAnnotation], "true") + assert.Equal(t, "true", actual.Annotations[naming.AutoCreateUserSchemaAnnotation]) }, }, "filters out reserved monitoring user": { @@ -342,14 +375,46 @@ func TestPerconaPGCluster_ToCrunchy(t *testing.T) { }, }, assertClusterFunc: func(t *testing.T, result *crunchyv1beta1.PostgresCluster, original *PerconaPGCluster) { - assert.Equal(t, len(result.Spec.Users), 2) + assert.Len(t, result.Spec.Users, 2) userNames := make([]string, len(result.Spec.Users)) for i, user := range result.Spec.Users { userNames[i] = string(user.Name) } - assert.Assert(t, contains(userNames, "regular-user")) - assert.Assert(t, contains(userNames, "another-user")) - assert.Assert(t, !contains(userNames, UserMonitoring)) + assert.True(t, contains(userNames, "regular-user")) + assert.True(t, contains(userNames, "another-user")) + assert.False(t, contains(userNames, UserMonitoring)) + }, + }, + "handles nil proxy": { + expectedPerconaPGCluster: &PerconaPGCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cluster", + Namespace: "test-namespace", + }, + Spec: PerconaPGClusterSpec{ + CRVersion: "2.9.0", + PostgresVersion: 15, + Proxy: nil, + InstanceSets: PGInstanceSets{ + { + Name: "instance1", + Replicas: &[]int32{1}[0], + DataVolumeClaimSpec: corev1.PersistentVolumeClaimSpec{ + AccessModes: []corev1.PersistentVolumeAccessMode{corev1.ReadWriteOnce}, + }, + }, + }, + Backups: Backups{ + PGBackRest: PGBackRestArchive{ + Repos: []crunchyv1beta1.PGBackRestRepo{ + {Name: "repo1"}, + }, + }, + }, + }, + }, + assertClusterFunc: func(t *testing.T, actual *crunchyv1beta1.PostgresCluster, _ *PerconaPGCluster) { + assert.Nil(t, actual.Spec.Proxy) }, }, } @@ -361,12 +426,12 @@ func TestPerconaPGCluster_ToCrunchy(t *testing.T) { crunchyCluster, err := tt.expectedPerconaPGCluster.ToCrunchy(ctx, tt.inputPostgresCluster, scheme) if tt.expectedError { - assert.Assert(t, err != nil) + require.Error(t, err) return } - assert.NilError(t, err) - assert.Assert(t, crunchyCluster != nil) + require.NoError(t, err) + assert.NotNil(t, crunchyCluster) if tt.assertClusterFunc != nil { tt.assertClusterFunc(t, crunchyCluster, tt.expectedPerconaPGCluster)