Conversation
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR enables running the Percona PostgreSQL Operator without PGBouncer by making the Proxy specification optional. Previously, the operator always initialized Proxy and PGBouncer objects even when not configured. Now, clusters can run with direct connections to PostgreSQL pods instead of routing through PGBouncer.
Changes:
- Modified the Default() method to only initialize PGBouncer when Proxy is configured
- Updated getHost() to return the pod service FQDN when Proxy/PGBouncer is not configured
- Added nil check for Proxy before accessing PGBouncer.EnvFrom (partially - bug found)
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/apis/pgv2.percona.com/v2/perconapgcluster_types.go | Modified Default() to conditionally initialize PGBouncer only when Proxy is non-nil |
| percona/controller/pgcluster/status.go | Updated getHost() to fallback to ClusterPodService when Proxy/PGBouncer is nil, improving service name handling |
| percona/controller/pgcluster/controller.go | Added partial nil check for Proxy before accessing PGBouncer.EnvFrom (missing PGBouncer check) |
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
egegunes
approved these changes
Feb 18, 2026
gkech
approved these changes
Feb 18, 2026
|
|
||
| // 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 |
There was a problem hiding this comment.
- Problem: When proxy/pgBouncer is not configured,
getHostalways returns the in-cluster primary Service FQDN, ignoring whether the primary Service is exposed as a LoadBalancer. - Why it matters:
.status.hostis printed as the user-facing “Endpoint”; without pgBouncer, users exposing Postgres via.spec.expose.type=LoadBalancerwill still see an internal-only DNS name. - Fix: Mirror the existing LoadBalancer logic for the primary Service when proxy is nil: if
.spec.exposeis LoadBalancer, fetch the primary Service and prefer its ingress hostname/IP; otherwise return the Service FQDN.
Suggested change
| return svcFQDN(naming.ClusterPrimaryService(postgresCluster).Name, postgresCluster.Namespace), nil | |
| svcName := naming.ClusterPrimaryService(postgresCluster).Name | |
| // If the primary is exposed via LoadBalancer, prefer its external hostname/IP. | |
| if cr.Spec.Expose != nil && cr.Spec.Expose.Type == string(corev1.ServiceTypeLoadBalancer) { | |
| svc := &corev1.Service{} | |
| err := r.Client.Get(ctx, types.NamespacedName{Namespace: cr.Namespace, Name: svcName}, svc) | |
| if err != nil { | |
| return "", errors.Wrapf(err, "get %s service", svcName) | |
| } | |
| var host string | |
| for _, i := range svc.Status.LoadBalancer.Ingress { | |
| if len(i.Hostname) > 0 { | |
| host = i.Hostname | |
| } else if len(i.IP) > 0 { | |
| host = i.IP | |
| } | |
| } | |
| if host != "" { | |
| return host, nil | |
| } | |
| } | |
| return svcFQDN(svcName, postgresCluster.Namespace), nil |
Comment on lines
+388
to
419
| "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) | ||
| }, | ||
| }, |
There was a problem hiding this comment.
- Problem: The test suite covers
Proxy: nilbut does not cover the newly-supported case whereProxyis non-nil whileProxy.PGBounceris nil (e.g. YAMLproxy: {}orpgBouncer: null). - Why it matters: That is a realistic user configuration for disabling pgBouncer, and it would have caught the current
ToCrunchynil-dereference risk. - Fix: Add a
ToCrunchytable entry that setsSpec.Proxy = &PGProxySpec{PGBouncer:nil}(or an emptyPGProxySpec{}) and assertactual.Spec.Proxy == nil(and no panic/error).
| } | ||
|
|
||
| func (p *PGProxySpec) ToCrunchy(version string) *crunchyv1beta1.PostgresProxySpec { | ||
| if p == nil { |
There was a problem hiding this comment.
- Problem:
PGProxySpec.ToCrunchywill panic when.spec.proxyis present but.spec.proxy.pgBounceris nil, because it later dereferencesp.PGBouncerwithout a nil guard. - Why it matters: With this PR,
Default()can leaveProxynon-nil whilePGBounceris nil (e.g.proxy: {}), making this crash reachable duringPerconaPGCluster.ToCrunchy. - Fix: In
ToCrunchy, treatp == nil || p.PGBouncer == nilas “proxy disabled” and returnnil(or otherwise guard the dereference) before calling intop.PGBouncer.
Suggested change
| if p == nil { | |
| if p == nil || p.PGBouncer == nil { |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CHANGE DESCRIPTION
Problem:
Operator does not allow running a cluster without PG bouncer. Specifying
nilfor.spec.proxyor.spec.proxy.pgbouncerstill results in the default pgbouncer settings being applied.Cause:
Operator adds default pgbouncer settings when set to nil.
Solution:
Honor
nilsetting for.spec.proxy, and apply pgbouncer defaults only when.spec.proxyis not nilCHECKLIST
Jira
Needs Doc) and QA (Needs QA)?Tests
Config/Logging/Testability