Merge branch 'main' into K8SPG-938_fix_conditions_update

nmarukovich · web-flow · commit 28981776f3d6 · 2026-02-13T12:57:38.000+01:00
diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md
@@ -0,0 +1,75 @@
+# GitHub Copilot Code Review Instructions
+
+## Review Philosophy
+
+- Only comment when you have HIGH CONFIDENCE (>80%) that an issue exists
+- Be concise
+- Focus on actionable feedback, not observations
+- If you're uncertain, stay silent—false positives reduce trust
+
+## Project Context
+
+Kubernetes Operator for PostgreSQL (Operator SDK, controller-runtime). Go + YAML. Key paths: `internal/`, `percona/`, `pkg/apis` and `e2e-tests/`.
+
+## Priority Areas
+
+### Security
+
+- Hardcoded secrets, credentials, or API keys
+- SQL injection—use parameterized queries, never string concatenation
+- Missing or overly broad RBAC (`+kubebuilder:rbac` on reconcile functions)
+- Logging of secrets or sensitive data
+- Unvalidated user input before DB operations
+
+### Correctness
+
+- Logic errors that could cause panics or incorrect behavior
+- Race conditions, resource leaks (files, connections, memory)
+- Incorrect or missing error propagation
+- Error wrapping that doesn't add useful context
+- Redundant comments that restate what the code shows
+
+### Imports and Dependencies
+
+- Use standard import aliases: `corev1`, `appsv1`, `metav1`, `apierrors`, etc. (per `.golangci.yaml`)
+- Import order: standard, default, `github.com/percona` prefix
+
+### Controller / Reconcile Logic
+
+- Add `+kubebuilder:rbac` above reconcile functions that create/update K8s resources
+- Set controller/owner references for owned resources
+- Idempotent reconcile; handle `apierrors.IsConflict` with requeue
+
+### Logging
+
+- Prefer `logging.FromContext(ctx)` for loggers
+- Use structured fields: `log.Info("message", "key", value)`
+- Add logging for important operator actions (reconcile steps, errors, retries)
+
+### Testing
+
+- New features: expect unit tests and/or E2E (KUTTL) where appropriate
+- Unit tests should use `assert` and `require` from `github.com/stretchr/testify` wherever applicable
+- Utilize table driven tests when possible
+- Test names should describe the scenario
+
+## Response Format
+
+When you identify an issue:
+
+1. **Problem** (1 sentence)
+2. **Why it matters** (1 sentence, only if not obvious)
+3. **Fix** (concrete suggestion or code snippet)
+
+Example:
+```
+1. **Problem**: This map access can panic if the map is nil.
+2. **Why it matters**: A panic can crash the operator and disrupt reconciliation.
+3. **Fix**: Initialize the map before use, e.g. `m := make(map[string]string)` before assigning or reading.
+```
+
+## When to Stay Silent
+
+- You're uncertain whether something is an issue
+- The concern is stylistic and the code is acceptable
+- The fix would be a matter of preference, not correctness or security
diff --git a/build/crd/crunchy/generated/postgres-operator.crunchydata.com_postgresclusters.yaml b/build/crd/crunchy/generated/postgres-operator.crunchydata.com_postgresclusters.yaml
@@ -6965,6 +6965,9 @@ spec:
                     description: Enable tracking latest restorable time
                     type: boolean
                 type: object
+              clusterServiceDNSSuffix:
+                description: K8SPG-694
+                type: string
               config:
                 properties:
                   files:
diff --git a/build/crd/percona/generated/pgv2.percona.com_perconapgclusters.yaml b/build/crd/percona/generated/pgv2.percona.com_perconapgclusters.yaml
@@ -7116,6 +7116,8 @@ spec:
                     are enabled
                   rule: (has(self.enabled) && self.enabled == false) || (has(self.pgbackrest.repos)
                     && size(self.pgbackrest.repos) > 0)
+              clusterServiceDNSSuffix:
+                type: string
               crVersion:
                 description: |-
                   Version of the operator. Update this to new version after operator
diff --git a/config/crd/bases/pgv2.percona.com_perconapgclusters.yaml b/config/crd/bases/pgv2.percona.com_perconapgclusters.yaml
@@ -7521,6 +7521,8 @@ spec:
                     are enabled
                   rule: (has(self.enabled) && self.enabled == false) || (has(self.pgbackrest.repos)
                     && size(self.pgbackrest.repos) > 0)
+              clusterServiceDNSSuffix:
+                type: string
               crVersion:
                 description: |-
                   Version of the operator. Update this to new version after operator
diff --git a/config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml b/config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml
@@ -6955,6 +6955,9 @@ spec:
                     description: Enable tracking latest restorable time
                     type: boolean
                 type: object
+              clusterServiceDNSSuffix:
+                description: K8SPG-694
+                type: string
               config:
                 properties:
                   files:
diff --git a/deploy/bundle.yaml b/deploy/bundle.yaml
@@ -7818,6 +7818,8 @@ spec:
                     are enabled
                   rule: (has(self.enabled) && self.enabled == false) || (has(self.pgbackrest.repos)
                     && size(self.pgbackrest.repos) > 0)
+              clusterServiceDNSSuffix:
+                type: string
               crVersion:
                 description: |-
                   Version of the operator. Update this to new version after operator
@@ -36871,6 +36873,9 @@ spec:
                     description: Enable tracking latest restorable time
                     type: boolean
                 type: object
+              clusterServiceDNSSuffix:
+                description: K8SPG-694
+                type: string
               config:
                 properties:
                   files:
diff --git a/deploy/cr.yaml b/deploy/cr.yaml
@@ -10,6 +10,7 @@ metadata:
 #  - percona.com/delete-backups
 spec:
   crVersion: 2.9.0
+#  clusterServiceDNSSuffix: cluster.local
 #  initContainer:
 #    image: docker.io/perconalab/percona-postgresql-operator:main
 #    resources:
diff --git a/deploy/crd.yaml b/deploy/crd.yaml
@@ -7818,6 +7818,8 @@ spec:
                     are enabled
                   rule: (has(self.enabled) && self.enabled == false) || (has(self.pgbackrest.repos)
                     && size(self.pgbackrest.repos) > 0)
+              clusterServiceDNSSuffix:
+                type: string
               crVersion:
                 description: |-
                   Version of the operator. Update this to new version after operator
@@ -36871,6 +36873,9 @@ spec:
                     description: Enable tracking latest restorable time
                     type: boolean
                 type: object
+              clusterServiceDNSSuffix:
+                description: K8SPG-694
+                type: string
               config:
                 properties:
                   files:
diff --git a/deploy/cw-bundle.yaml b/deploy/cw-bundle.yaml
@@ -7818,6 +7818,8 @@ spec:
                     are enabled
                   rule: (has(self.enabled) && self.enabled == false) || (has(self.pgbackrest.repos)
                     && size(self.pgbackrest.repos) > 0)
+              clusterServiceDNSSuffix:
+                type: string
               crVersion:
                 description: |-
                   Version of the operator. Update this to new version after operator
@@ -36871,6 +36873,9 @@ spec:
                     description: Enable tracking latest restorable time
                     type: boolean
                 type: object
+              clusterServiceDNSSuffix:
+                description: K8SPG-694
+                type: string
               config:
                 properties:
                   files:
diff --git a/internal/controller/postgrescluster/instance.go b/internal/controller/postgrescluster/instance.go
@@ -1498,7 +1498,7 @@ func (r *Reconciler) reconcileInstanceCertificates(
 	var leafCert *pki.LeafCertificate
 
 	if err == nil {
-		leafCert, err = r.instanceCertificate(ctx, instance, existing, instanceCerts, root)
+		leafCert, err = r.instanceCertificate(ctx, instance, existing, instanceCerts, root, cluster.Spec.ClusterServiceDNSSuffix)
 	}
 	if err == nil {
 		err = patroni.InstanceCertificates(ctx,
diff --git a/internal/controller/postgrescluster/pki.go b/internal/controller/postgrescluster/pki.go
@@ -176,12 +176,12 @@ func (r *Reconciler) reconcileClusterCertificate(
 		r.Client.Get(ctx, client.ObjectKeyFromObject(existing), existing)))
 
 	leaf := &pki.LeafCertificate{}
-	primaryServiceDNSNames, err := naming.ServiceDNSNames(ctx, primaryService)
+	primaryServiceDNSNames, err := naming.ServiceDNSNames(ctx, primaryService, cluster.Spec.ClusterServiceDNSSuffix)
 	if err != nil {
 		return nil, errors.Wrap(err, "get primary service dns names")
 	}
 
-	replicaServiceDNSNames, err := naming.ServiceDNSNames(ctx, replicaService)
+	replicaServiceDNSNames, err := naming.ServiceDNSNames(ctx, replicaService, cluster.Spec.ClusterServiceDNSSuffix)
 	if err != nil {
 		return nil, errors.Wrap(err, "get replica service dns names")
 	}
@@ -256,7 +256,7 @@ func (r *Reconciler) reconcileClusterCertificate(
 // using the current root certificate
 func (*Reconciler) instanceCertificate(
 	ctx context.Context, instance *appsv1.StatefulSet,
-	existing, intent *corev1.Secret, root *pki.RootCertificateAuthority,
+	existing, intent *corev1.Secret, root *pki.RootCertificateAuthority, dnsSuffix string,
 ) (
 	*pki.LeafCertificate, error,
 ) {
@@ -267,7 +267,7 @@ func (*Reconciler) instanceCertificate(
 
 	// RFC 2818 states that the certificate DNS names must be used to verify
 	// HTTPS identity.
-	dnsNames := naming.InstancePodDNSNames(ctx, instance)
+	dnsNames := naming.InstancePodDNSNames(ctx, instance, dnsSuffix)
 	dnsFQDN := dnsNames[0]
 
 	if err == nil {
diff --git a/internal/controller/postgrescluster/pki_test.go b/internal/controller/postgrescluster/pki_test.go
@@ -188,7 +188,7 @@ func TestReconcileCerts(t *testing.T) {
 			existing := &corev1.Secret{Data: make(map[string][]byte)}
 			intent := &corev1.Secret{Data: make(map[string][]byte)}
 
-			initialLeafCert, err := r.instanceCertificate(ctx, instance, existing, intent, initialRoot)
+			initialLeafCert, err := r.instanceCertificate(ctx, instance, existing, intent, initialRoot, "")
 			assert.NilError(t, err)
 
 			fromSecret := &pki.LeafCertificate{}
@@ -215,25 +215,23 @@ func TestReconcileCerts(t *testing.T) {
 			existing := &corev1.Secret{Data: make(map[string][]byte)}
 			intent := &corev1.Secret{Data: make(map[string][]byte)}
 
-			initialLeaf, err := r.instanceCertificate(ctx, instance, existing, intent, initialRoot)
+			initialLeaf, err := r.instanceCertificate(ctx, instance, existing, intent, initialRoot, "")
 			assert.NilError(t, err)
 
 			// reconcile the certificate
-			newLeaf, err := r.instanceCertificate(ctx, instance, existing, intent, newRootCert)
+			newLeaf, err := r.instanceCertificate(ctx, instance, existing, intent, newRootCert, "")
 			assert.NilError(t, err)
 
 			// assert old leaf cert does not match the newly reconciled one
 			assert.Assert(t, !initialLeaf.Certificate.Equal(newLeaf.Certificate))
 
 			// 'reconcile' the certificate when the secret does not change. The returned leaf certificate should not change
-			newLeaf2, err := r.instanceCertificate(ctx, instance, intent, intent, newRootCert)
+			newLeaf2, err := r.instanceCertificate(ctx, instance, intent, intent, newRootCert, "")
 			assert.NilError(t, err)
 
 			// check that the leaf cert did not change after another reconciliation
 			assert.DeepEqual(t, newLeaf2, newLeaf)
-
 		})
-
 	})
 
 	t.Run("check cluster certificate secret reconciliation", func(t *testing.T) {
diff --git a/internal/naming/dns.go b/internal/naming/dns.go
@@ -16,9 +16,9 @@ import (
 
 // InstancePodDNSNames returns the possible DNS names for instance. The first
 // name is the fully qualified domain name (FQDN).
-func InstancePodDNSNames(ctx context.Context, instance *appsv1.StatefulSet) []string {
+func InstancePodDNSNames(ctx context.Context, instance *appsv1.StatefulSet, dnsSuffix string) []string {
 	var (
-		domain    = KubernetesClusterDomain(ctx)
+		domain    = KubernetesClusterDomain(ctx, dnsSuffix)
 		namespace = instance.Namespace
 		name      = instance.Name + "-0." + instance.Spec.ServiceName
 	)
@@ -36,7 +36,7 @@ func InstancePodDNSNames(ctx context.Context, instance *appsv1.StatefulSet) []st
 
 // RepoHostPodDNSNames returns the possible DNS names for a pgBackRest repository host Pod.
 // The first name is the fully qualified domain name (FQDN).
-func RepoHostPodDNSNames(ctx context.Context, repoHost *appsv1.StatefulSet) ([]string, error) {
+func RepoHostPodDNSNames(ctx context.Context, repoHost *appsv1.StatefulSet, dnsSuffix string) ([]string, error) {
 	if repoHost.Namespace == "" {
 		return nil, errors.New("repoHost.Namespace is empty")
 	}
@@ -48,7 +48,7 @@ func RepoHostPodDNSNames(ctx context.Context, repoHost *appsv1.StatefulSet) ([]s
 	}
 
 	var (
-		domain    = KubernetesClusterDomain(ctx)
+		domain    = KubernetesClusterDomain(ctx, dnsSuffix)
 		namespace = repoHost.Namespace
 		name      = repoHost.Name + "-0." + repoHost.Spec.ServiceName
 	)
@@ -66,7 +66,7 @@ func RepoHostPodDNSNames(ctx context.Context, repoHost *appsv1.StatefulSet) ([]s
 
 // ServiceDNSNames returns the possible DNS names for service. The first name
 // is the fully qualified domain name (FQDN).
-func ServiceDNSNames(ctx context.Context, service *corev1.Service) ([]string, error) {
+func ServiceDNSNames(ctx context.Context, service *corev1.Service, dnsSuffix string) ([]string, error) {
 	if service.Name == "" {
 		return nil, errors.New("service.Name is empty")
 	}
@@ -75,7 +75,7 @@ func ServiceDNSNames(ctx context.Context, service *corev1.Service) ([]string, er
 		return nil, errors.New("service.Namespace is empty")
 	}
 
-	domain := KubernetesClusterDomain(ctx)
+	domain := KubernetesClusterDomain(ctx, dnsSuffix)
 
 	return []string{
 		service.Name + "." + service.Namespace + ".svc." + domain,
@@ -86,7 +86,13 @@ func ServiceDNSNames(ctx context.Context, service *corev1.Service) ([]string, er
 }
 
 // KubernetesClusterDomain looks up the Kubernetes cluster domain name.
-func KubernetesClusterDomain(ctx context.Context) string {
+// K8SPG-694: If the override parameter is provided, it is returned without performing any operations
+func KubernetesClusterDomain(ctx context.Context, override string) string {
+	// K8SPG-694
+	if override != "" {
+		return override
+	}
+
 	ctx, span := tracer.Start(ctx, "kubernetes-domain-lookup")
 	defer span.End()
 
diff --git a/internal/naming/dns_test.go b/internal/naming/dns_test.go
@@ -16,15 +16,15 @@ import (
 )
 
 func TestInstancePodDNSNames(t *testing.T) {
-	ctx, cancel := context.WithTimeout(context.Background(), time.Second)
+	ctx, cancel := context.WithTimeout(t.Context(), time.Second)
 	defer cancel()
 
 	instance := &appsv1.StatefulSet{}
 	instance.Namespace = "some-place"
 	instance.Name = "cluster-name-id"
 	instance.Spec.ServiceName = "cluster-pods"
 
-	names := InstancePodDNSNames(ctx, instance)
+	names := InstancePodDNSNames(ctx, instance, "")
 	assert.Assert(t, len(names) > 0)
 
 	assert.DeepEqual(t, names[1:], []string{
@@ -36,17 +36,31 @@ func TestInstancePodDNSNames(t *testing.T) {
 	assert.Assert(t, len(names[0]) > len(names[1]), "expected FQDN first, got %q", names[0])
 	assert.Assert(t, strings.HasPrefix(names[0], names[1]+"."), "wrong FQDN: %q", names[0])
 	assert.Assert(t, !strings.HasSuffix(names[0], "."), "not expected root, got %q", names[0])
+
+	names = InstancePodDNSNames(ctx, instance, "override.cluster.local")
+	assert.Assert(t, len(names) > 0)
+
+	assert.DeepEqual(t, names, []string{
+		"cluster-name-id-0.cluster-pods.some-place.svc.override.cluster.local",
+		"cluster-name-id-0.cluster-pods.some-place.svc",
+		"cluster-name-id-0.cluster-pods.some-place",
+		"cluster-name-id-0.cluster-pods",
+	})
+
+	assert.Assert(t, len(names[0]) > len(names[1]), "expected FQDN first, got %q", names[0])
+	assert.Assert(t, strings.HasPrefix(names[0], names[1]+"."), "wrong FQDN: %q", names[0])
+	assert.Assert(t, !strings.HasSuffix(names[0], "."), "not expected root, got %q", names[0])
 }
 
 func TestServiceDNSNames(t *testing.T) {
-	ctx, cancel := context.WithTimeout(context.Background(), time.Second)
+	ctx, cancel := context.WithTimeout(t.Context(), time.Second)
 	defer cancel()
 
 	service := &corev1.Service{}
 	service.Namespace = "baltia"
 	service.Name = "the-primary"
 
-	names, err := ServiceDNSNames(ctx, service)
+	names, err := ServiceDNSNames(ctx, service, "")
 	assert.NilError(t, err)
 	assert.Assert(t, len(names) > 0)
 
@@ -59,4 +73,19 @@ func TestServiceDNSNames(t *testing.T) {
 	assert.Assert(t, len(names[0]) > len(names[1]), "expected FQDN first, got %q", names[0])
 	assert.Assert(t, strings.HasPrefix(names[0], names[1]+"."), "wrong FQDN: %q", names[0])
 	assert.Assert(t, !strings.HasSuffix(names[0], "."), "not expected root, got %q", names[0])
+
+	names, err = ServiceDNSNames(ctx, service, "override.cluster.local")
+	assert.NilError(t, err)
+	assert.Assert(t, len(names) > 0)
+
+	assert.DeepEqual(t, names, []string{
+		"the-primary.baltia.svc.override.cluster.local",
+		"the-primary.baltia.svc",
+		"the-primary.baltia",
+		"the-primary",
+	})
+
+	assert.Assert(t, len(names[0]) > len(names[1]), "expected FQDN first, got %q", names[0])
+	assert.Assert(t, strings.HasPrefix(names[0], names[1]+"."), "wrong FQDN: %q", names[0])
+	assert.Assert(t, !strings.HasSuffix(names[0], "."), "not expected root, got %q", names[0])
 }
diff --git a/internal/pgbackrest/config.go b/internal/pgbackrest/config.go
diff --git a/internal/pgbackrest/config_test.go b/internal/pgbackrest/config_test.go
diff --git a/internal/pgbackrest/reconcile.go b/internal/pgbackrest/reconcile.go
diff --git a/internal/pgbouncer/reconcile.go b/internal/pgbouncer/reconcile.go
diff --git a/pkg/apis/pgv2.percona.com/v2/perconapgcluster_types.go b/pkg/apis/pgv2.percona.com/v2/perconapgcluster_types.go
diff --git a/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgrescluster_types.go b/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgrescluster_types.go

Original file line number	Diff line number	Diff line change
`@@ -1498,7 +1498,7 @@ func (r *Reconciler) reconcileInstanceCertificates(`
`1498`	`1498`	`var leafCert *pki.LeafCertificate`
`1499`	`1499`
`1500`	`1500`	`if err == nil {`
`1501`		`- leafCert, err = r.instanceCertificate(ctx, instance, existing, instanceCerts, root)`
	`1501`	`+ leafCert, err = r.instanceCertificate(ctx, instance, existing, instanceCerts, root, cluster.Spec.ClusterServiceDNSSuffix)`
`1502`	`1502`	`}`
`1503`	`1503`	`if err == nil {`
`1504`	`1504`	`err = patroni.InstanceCertificates(ctx,`