Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds pg_tde (Transparent Data Encryption) support for PostgreSQL clusters, integrating with HashiCorp Vault for key management. The implementation follows the existing extension pattern in the codebase and includes API changes, reconciliation logic, and CRD updates.
Changes:
- Added pg_tde extension support with Vault integration for managing encryption keys
- Introduced new API types (PGTDESpec, PGTDEVaultSpec, PGTDESecretObjectReference) for configuring pg_tde
- Refactored extension configuration to use individual extension structs instead of the deprecated builtin structure
- Added reconciliation logic for configuring pg_tde providers and managing encryption keys
Reviewed changes
Copilot reviewed 20 out of 22 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgrescluster_types.go | Added PGTDESpec, PGTDEVaultSpec, and PGTDESecretObjectReference types for pg_tde configuration |
| pkg/apis/postgres-operator.crunchydata.com/v1beta1/zz_generated.deepcopy.go | Auto-generated DeepCopy methods for new pg_tde types |
| pkg/apis/pgv2.percona.com/v2/perconapgcluster_types.go | Added BuiltInExtensionSpec and pg_tde support; refactored extension defaults handling |
| pkg/apis/pgv2.percona.com/v2/zz_generated.deepcopy.go | Auto-generated DeepCopy methods for BuiltInExtensionSpec |
| internal/pgtde/postgres.go | New package implementing pg_tde enable/disable and Vault provider configuration |
| internal/postgres/reconcile.go | Added pg_tde volume and volume mount configuration for PostgreSQL instance pods |
| internal/controller/postgrescluster/postgres.go | Added reconcilePGTDEProviders function to configure pg_tde Vault providers |
| internal/controller/postgrescluster/controller.go | Integrated pg_tde parameters and provider reconciliation into main reconciliation flow |
| internal/controller/postgrescluster/instance.go | Added TDEInstalledAnnotation to instance pods when pg_tde is enabled |
| internal/naming/names.go | Added constants for pg_tde volume names, mount paths, and provider identifiers |
| internal/naming/annotations.go | Added TDEInstalledAnnotation constant |
| internal/pgvector/postgres.go | Fixed comments to correctly reference pgvector instead of pgAudit |
| percona/controller/pgbackup/controller.go | Added pg.Default() call to ensure extension defaults before backup |
| deploy/.yaml and config/crd/bases/.yaml | Updated CRD definitions with pg_tde specifications |
| deploy/cr.yaml | Added example pg_tde configuration with Vault settings |
| pkg/apis/pgv2.percona.com/v2/perconapgbackup_types_test.go | Replaced custom ptr() function with k8s.io/utils/ptr.To |
Comments suppressed due to low confidence (1)
pkg/apis/pgv2.percona.com/v2/perconapgcluster_types.go:436
- Inconsistent usage of extension enabled flags. Line 432 uses cr.Spec.Extensions.PGStatMonitor.Enabled (the new field structure), while lines 433-436 still use cr.Spec.Extensions.BuiltIn.* (the deprecated field structure). All lines should be updated to use the new field structure consistently for maintainability.
postgresCluster.Spec.Extensions.PGStatMonitor = *cr.Spec.Extensions.PGStatMonitor.Enabled
postgresCluster.Spec.Extensions.PGStatStatements = *cr.Spec.Extensions.BuiltIn.PGStatStatements
postgresCluster.Spec.Extensions.PGAudit = *cr.Spec.Extensions.BuiltIn.PGAudit
postgresCluster.Spec.Extensions.PGVector = *cr.Spec.Extensions.BuiltIn.PGVector
postgresCluster.Spec.Extensions.PGRepack = *cr.Spec.Extensions.BuiltIn.PGRepack
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/postgres/reconcile.go
Outdated
| pgTDECASecret := &corev1.SecretProjection{ | ||
| LocalObjectReference: corev1.LocalObjectReference{ | ||
| Name: inCluster.Spec.Extensions.PGTDE.Vault.CASecret.Name, | ||
| }, | ||
| } |
There was a problem hiding this comment.
The SecretProjection is missing the Items field to specify which key from the secret should be mounted. Without Items, all keys in the secret will be mounted. Based on the pattern used elsewhere in the codebase (e.g., internal/patroni/certificates.go), you should add an Items field to specify the exact key to mount from the secret.
| stdout, stderr, err := exec.Exec(ctx, | ||
| strings.NewReader(strings.Join([]string{ | ||
| // Quiet NOTICE messages from IF NOT EXISTS statements. | ||
| // - https://www.postgresql.org/docs/current/runtime-config-client.html | ||
| `SET client_min_messages = WARNING;`, | ||
| fmt.Sprintf(`SELECT pg_tde_change_global_key_provider_vault_v2( | ||
| 'vault-provider', '%s', '%s', '%s', '%s' | ||
| );`, | ||
| vault.Host, | ||
| vault.MountPath, | ||
| naming.PGTDEMountPath+"/"+vault.TokenSecret.Key, | ||
| naming.PGTDEMountPath+"/"+vault.CASecret.Key, |
There was a problem hiding this comment.
The function doesn't check if vault.CASecret.Key is empty before using it in the SQL statement. When CASecret.Key is empty, this will result in an invalid file path being passed to the pg_tde function. The function should handle the empty case similar to AddVaultProvider which uses "NULL" when CASecret.Key is empty.
| stdout, stderr, err := exec.Exec(ctx, | |
| strings.NewReader(strings.Join([]string{ | |
| // Quiet NOTICE messages from IF NOT EXISTS statements. | |
| // - https://www.postgresql.org/docs/current/runtime-config-client.html | |
| `SET client_min_messages = WARNING;`, | |
| fmt.Sprintf(`SELECT pg_tde_change_global_key_provider_vault_v2( | |
| 'vault-provider', '%s', '%s', '%s', '%s' | |
| );`, | |
| vault.Host, | |
| vault.MountPath, | |
| naming.PGTDEMountPath+"/"+vault.TokenSecret.Key, | |
| naming.PGTDEMountPath+"/"+vault.CASecret.Key, | |
| // Handle optional CA secret key similarly to AddVaultProvider: use NULL when empty. | |
| caPath := "NULL" | |
| if vault.CASecret.Key != "" { | |
| caPath = fmt.Sprintf("'%s'", naming.PGTDEMountPath+"/"+vault.CASecret.Key) | |
| } | |
| stdout, stderr, err := exec.Exec(ctx, | |
| strings.NewReader(strings.Join([]string{ | |
| // Quiet NOTICE messages from IF NOT EXISTS statements. | |
| // - https://www.postgresql.org/docs/current/runtime-config-client.html | |
| `SET client_min_messages = WARNING;`, | |
| fmt.Sprintf(`SELECT pg_tde_change_global_key_provider_vault_v2( | |
| 'vault-provider', '%s', '%s', '%s', %s | |
| );`, | |
| vault.Host, | |
| vault.MountPath, | |
| naming.PGTDEMountPath+"/"+vault.TokenSecret.Key, | |
| caPath, |
| 'vault-provider', '%s', '%s', '%s', '%s' | ||
| );`, |
There was a problem hiding this comment.
The provider name is hard-coded as 'vault-provider' instead of using the naming.PGTDEVaultProvider constant. This inconsistency could lead to configuration issues since AddVaultProvider uses the constant. Both functions should use the same constant for consistency.
| 'vault-provider', '%s', '%s', '%s', '%s' | |
| );`, | |
| '%s', '%s', '%s', '%s', '%s' | |
| );`, | |
| naming.PGTDEVaultProvider, |
| configure := func(ctx context.Context, exec postgres.Executor) error { | ||
| if cluster.Status.PGTDERevision == "" { | ||
| return pgtde.AddVaultProvider(ctx, exec, cluster.Spec.Extensions.PGTDE.Vault) | ||
| } | ||
| return pgtde.ChangeVaultProvider(ctx, exec, cluster.Spec.Extensions.PGTDE.Vault) | ||
| } |
There was a problem hiding this comment.
There's no validation to ensure cluster.Spec.Extensions.PGTDE.Vault is not nil before calling AddVaultProvider or ChangeVaultProvider. If pg_tde is enabled but the Vault configuration is not provided, this will result in a nil pointer dereference. Add a nil check for Vault configuration before using it.
| fmt.Sprintf(`SELECT pg_tde_change_global_key_provider_vault_v2( | ||
| 'vault-provider', '%s', '%s', '%s', '%s' | ||
| );`, | ||
| vault.Host, | ||
| vault.MountPath, | ||
| naming.PGTDEMountPath+"/"+vault.TokenSecret.Key, | ||
| naming.PGTDEMountPath+"/"+vault.CASecret.Key, | ||
| ), |
There was a problem hiding this comment.
The SQL statement construction is vulnerable to SQL injection as user-provided values from vault.Host, vault.MountPath, vault.TokenSecret.Key, and vault.CASecret.Key are directly interpolated into the SQL string using fmt.Sprintf without any sanitization or parameterization. Although these values come from the cluster spec, they should still be properly escaped or parameterized to prevent SQL injection attacks and handle special characters correctly. Consider using proper SQL parameterization or at minimum sanitizing these inputs.
internal/postgres/reconcile.go
Outdated
| pgTDETokenSecret := &corev1.SecretProjection{ | ||
| LocalObjectReference: corev1.LocalObjectReference{ | ||
| Name: inCluster.Spec.Extensions.PGTDE.Vault.TokenSecret.Name, | ||
| }, | ||
| } |
There was a problem hiding this comment.
The SecretProjection is missing the Items field to specify which key from the secret should be mounted. Without Items, all keys in the secret will be mounted, which may not be the intended behavior. Based on the pattern used elsewhere in the codebase (e.g., internal/patroni/certificates.go), you should add an Items field to specify the exact key to mount from the secret.
| crunchyv1beta1 "github.com/percona/percona-postgresql-operator/v2/pkg/apis/postgres-operator.crunchydata.com/v1beta1" | ||
| ) | ||
|
|
||
| // EnableInPostgreSQL installs pgvector triggers into every database. |
There was a problem hiding this comment.
The comment incorrectly says "installs pgvector triggers" but this function installs pg_tde extension, not pgvector. This should be updated to accurately describe the function's purpose.
| // EnableInPostgreSQL installs pgvector triggers into every database. | |
| // EnableInPostgreSQL installs and updates the pg_tde extension in every database. |
internal/postgres/reconcile.go
Outdated
| pgTDEVolumeMount := corev1.VolumeMount{ | ||
| Name: naming.PGTDEVolume, | ||
| MountPath: naming.PGTDEMountPath, | ||
| ReadOnly: true, | ||
| } | ||
| pgTDETokenSecret := &corev1.SecretProjection{ | ||
| LocalObjectReference: corev1.LocalObjectReference{ | ||
| Name: inCluster.Spec.Extensions.PGTDE.Vault.TokenSecret.Name, | ||
| }, | ||
| } | ||
| pgTDEVolume := corev1.Volume{ | ||
| Name: pgTDEVolumeMount.Name, | ||
| VolumeSource: corev1.VolumeSource{ | ||
| Projected: &corev1.ProjectedVolumeSource{ | ||
| DefaultMode: initialize.Int32(0o600), | ||
| Sources: []corev1.VolumeProjection{ | ||
| {Secret: pgTDETokenSecret}, | ||
| }, | ||
| }, | ||
| }, | ||
| } | ||
| if inCluster.Spec.Extensions.PGTDE.Vault.CASecret.Name != "" { | ||
| pgTDECASecret := &corev1.SecretProjection{ | ||
| LocalObjectReference: corev1.LocalObjectReference{ | ||
| Name: inCluster.Spec.Extensions.PGTDE.Vault.CASecret.Name, | ||
| }, | ||
| } | ||
| pgTDEVolume.VolumeSource.Projected.Sources = append( | ||
| pgTDEVolume.VolumeSource.Projected.Sources, corev1.VolumeProjection{ | ||
| Secret: pgTDECASecret, | ||
| }) | ||
| } |
There was a problem hiding this comment.
The pg_tde volume and volume mount are created unconditionally at the beginning of the function, even when PGTDE is not enabled. This creates unused volume objects that are only conditionally added to the pod later. Consider moving this logic inside the conditional check at lines 193-195 and 275-277 to avoid creating unnecessary objects.
| fmt.Sprintf(`SELECT pg_tde_add_global_key_provider_vault_v2( | ||
| '%s', '%s', '%s', '%s', %s | ||
| );`, | ||
| naming.PGTDEVaultProvider, | ||
| vault.Host, | ||
| vault.MountPath, | ||
| naming.PGTDEMountPath+"/"+vault.TokenSecret.Key, | ||
| caSecretPath, | ||
| ), |
There was a problem hiding this comment.
The SQL statement construction is vulnerable to SQL injection as user-provided values from vault.Host, vault.MountPath, vault.TokenSecret.Key, and vault.CASecret.Key are directly interpolated into the SQL string using fmt.Sprintf without any sanitization or parameterization. Although these values come from the cluster spec, they should still be properly escaped or parameterized to prevent SQL injection attacks and handle special characters correctly. Consider using proper SQL parameterization or at minimum sanitizing these inputs.
| package pgtde | ||
|
|
||
| import ( | ||
| "context" | ||
| "fmt" | ||
| "strings" | ||
|
|
||
| "github.com/percona/percona-postgresql-operator/v2/internal/logging" | ||
| "github.com/percona/percona-postgresql-operator/v2/internal/naming" | ||
| "github.com/percona/percona-postgresql-operator/v2/internal/postgres" | ||
| crunchyv1beta1 "github.com/percona/percona-postgresql-operator/v2/pkg/apis/postgres-operator.crunchydata.com/v1beta1" | ||
| ) | ||
|
|
||
| // EnableInPostgreSQL installs pgvector triggers into every database. | ||
| func EnableInPostgreSQL(ctx context.Context, exec postgres.Executor) error { | ||
| log := logging.FromContext(ctx) | ||
|
|
||
| stdout, stderr, err := exec.ExecInAllDatabases(ctx, | ||
| `SET client_min_messages = WARNING; CREATE EXTENSION IF NOT EXISTS pg_tde; ALTER EXTENSION pg_tde UPDATE;`, | ||
| map[string]string{ | ||
| "ON_ERROR_STOP": "on", // Abort when any one command fails. | ||
| "QUIET": "on", // Do not print successful commands to stdout. | ||
| }) | ||
|
|
||
| log.V(1).Info("enabled pg_tde", "stdout", stdout, "stderr", stderr) | ||
|
|
||
| return err | ||
| } | ||
|
|
||
| func DisableInPostgreSQL(ctx context.Context, exec postgres.Executor) error { | ||
| log := logging.FromContext(ctx) | ||
|
|
||
| stdout, stderr, err := exec.ExecInAllDatabases(ctx, | ||
| `SET client_min_messages = WARNING; DROP EXTENSION IF EXISTS pg_tde;`, | ||
| map[string]string{ | ||
| "ON_ERROR_STOP": "on", // Abort when any one command fails. | ||
| "QUIET": "on", // Do not print successful commands to stdout. | ||
| }) | ||
|
|
||
| log.V(1).Info("disabled pg_tde", "stdout", stdout, "stderr", stderr) | ||
|
|
||
| return err | ||
| } | ||
|
|
||
| func PostgreSQLParameters(outParameters *postgres.Parameters) { | ||
| outParameters.Mandatory.AppendToList("shared_preload_libraries", "pg_tde") | ||
| } | ||
|
|
||
| func AddVaultProvider(ctx context.Context, exec postgres.Executor, vault *crunchyv1beta1.PGTDEVaultSpec) error { | ||
| log := logging.FromContext(ctx) | ||
|
|
||
| caSecretPath := "NULL" | ||
| if vault.CASecret.Key != "" { | ||
| caSecretPath = fmt.Sprintf("'%s'", naming.PGTDEMountPath+"/"+vault.CASecret.Key) | ||
| } | ||
|
|
||
| stdout, stderr, err := exec.Exec(ctx, | ||
| strings.NewReader(strings.Join([]string{ | ||
| // Quiet NOTICE messages from IF NOT EXISTS statements. | ||
| // - https://www.postgresql.org/docs/current/runtime-config-client.html | ||
| `SET client_min_messages = WARNING;`, | ||
| fmt.Sprintf(`SELECT pg_tde_add_global_key_provider_vault_v2( | ||
| '%s', '%s', '%s', '%s', %s | ||
| );`, | ||
| naming.PGTDEVaultProvider, | ||
| vault.Host, | ||
| vault.MountPath, | ||
| naming.PGTDEMountPath+"/"+vault.TokenSecret.Key, | ||
| caSecretPath, | ||
| ), | ||
| fmt.Sprintf("SELECT pg_tde_create_key_using_global_key_provider('%s', '%s');", | ||
| naming.PGTDEGlobalKey, | ||
| naming.PGTDEVaultProvider, | ||
| ), | ||
| fmt.Sprintf("SELECT pg_tde_set_default_key_using_global_key_provider('%s', '%s');", | ||
| naming.PGTDEGlobalKey, | ||
| naming.PGTDEVaultProvider, | ||
| ), | ||
| }, "\n")), | ||
| map[string]string{ | ||
| "ON_ERROR_STOP": "on", // Abort when any one statement fails. | ||
| "QUIET": "on", // Do not print successful statements to stdout. | ||
| }, nil) | ||
|
|
||
| if err != nil { | ||
| log.Info("failed to add pg_tde vault provider", "stdout", stdout, "stderr", stderr) | ||
| } else { | ||
| log.Info("added pg_tde vault provider", "stdout", stdout, "stderr", stderr) | ||
| } | ||
|
|
||
| return err | ||
| } | ||
|
|
||
| func ChangeVaultProvider(ctx context.Context, exec postgres.Executor, vault *crunchyv1beta1.PGTDEVaultSpec) error { | ||
| log := logging.FromContext(ctx) | ||
|
|
||
| stdout, stderr, err := exec.Exec(ctx, | ||
| strings.NewReader(strings.Join([]string{ | ||
| // Quiet NOTICE messages from IF NOT EXISTS statements. | ||
| // - https://www.postgresql.org/docs/current/runtime-config-client.html | ||
| `SET client_min_messages = WARNING;`, | ||
| fmt.Sprintf(`SELECT pg_tde_change_global_key_provider_vault_v2( | ||
| 'vault-provider', '%s', '%s', '%s', '%s' | ||
| );`, | ||
| vault.Host, | ||
| vault.MountPath, | ||
| naming.PGTDEMountPath+"/"+vault.TokenSecret.Key, | ||
| naming.PGTDEMountPath+"/"+vault.CASecret.Key, | ||
| ), | ||
| }, "\n")), | ||
| map[string]string{ | ||
| "ON_ERROR_STOP": "on", // Abort when any one statement fails. | ||
| "QUIET": "on", // Do not print successful statements to stdout. | ||
| }, nil) | ||
|
|
||
| if err != nil { | ||
| log.Info("failed to change pg_tde vault provider", "stdout", stdout, "stderr", stderr) | ||
| } else { | ||
| log.Info("changed pg_tde vault provider", "stdout", stdout, "stderr", stderr) | ||
| } | ||
|
|
||
| return err | ||
| } |
There was a problem hiding this comment.
The internal/pgtde package lacks test coverage. Similar extension packages in the codebase (e.g., internal/pgvector/postgres_test.go, internal/pgaudit/postgres_test.go) have corresponding test files. Consider adding tests for the pgtde package functions, particularly for EnableInPostgreSQL, DisableInPostgreSQL, AddVaultProvider, and ChangeVaultProvider.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| local command=${1} | ||
| local uri=${2} | ||
| local driver=${3:-postgres} | ||
|
|
There was a problem hiding this comment.
[shfmt] reported by reviewdog 🐶
There was a problem hiding this comment.
[shfmt] reported by reviewdog 🐶
percona-postgresql-operator/e2e-tests/functions
Lines 1637 to 1643 in 0de3e00
| r.Recorder.Event(cluster, corev1.EventTypeWarning, "pgTdeDisabled", | ||
| "Unable to install pg_tde") | ||
| } | ||
| } else { | ||
| if pgTdeOK = pgtde.DisableInPostgreSQL(ctx, exec) == nil; !pgTdeOK { | ||
| r.Recorder.Event(cluster, corev1.EventTypeWarning, "pgTdeEnabled", |
There was a problem hiding this comment.
- Problem: The event reason strings for pg_tde failures are reversed (install failure emits
pgTdeDisabled, disable failure emitspgTdeEnabled). - Why it matters: Alerting and troubleshooting based on event reasons will be misleading.
- Fix: Swap the reason strings so the "install" failure uses an "Enabled"-style reason and the "disable" failure uses a "Disabled"-style reason (consistent with the other extension events in this function).
| r.Recorder.Event(cluster, corev1.EventTypeWarning, "pgTdeDisabled", | |
| "Unable to install pg_tde") | |
| } | |
| } else { | |
| if pgTdeOK = pgtde.DisableInPostgreSQL(ctx, exec) == nil; !pgTdeOK { | |
| r.Recorder.Event(cluster, corev1.EventTypeWarning, "pgTdeEnabled", | |
| r.Recorder.Event(cluster, corev1.EventTypeWarning, "pgTdeEnabled", | |
| "Unable to install pg_tde") | |
| } | |
| } else { | |
| if pgTdeOK = pgtde.DisableInPostgreSQL(ctx, exec) == nil; !pgTdeOK { | |
| r.Recorder.Event(cluster, corev1.EventTypeWarning, "pgTdeDisabled", |
| if cluster.Status.PGTDERevision == "" { | ||
| return pgtde.AddVaultProvider(ctx, exec, cluster.Spec.Extensions.PGTDE.Vault) | ||
| } | ||
| return pgtde.ChangeVaultProvider(ctx, exec, cluster.Spec.Extensions.PGTDE.Vault) |
There was a problem hiding this comment.
- Problem:
reconcilePGTDEProviderscan panic whenspec.extensions.pg_tde.enabledis true butspec.extensions.pg_tde.vaultis nil;configure()passes a nil vault intopgtde.AddVaultProvider/ChangeVaultProvider. - Why it matters: A panic will crash the operator and disrupt reconciliation for all clusters.
- Fix: Validate
cluster.Spec.Extensions.PGTDE.Vault != nil(and required fields) before calling intopgtde.*, returning a clear error/event when pg_tde is enabled without a provider configuration.
| if cluster.Status.PGTDERevision == "" { | |
| return pgtde.AddVaultProvider(ctx, exec, cluster.Spec.Extensions.PGTDE.Vault) | |
| } | |
| return pgtde.ChangeVaultProvider(ctx, exec, cluster.Spec.Extensions.PGTDE.Vault) | |
| pgtdeSpec := cluster.Spec.Extensions.PGTDE | |
| if pgtdeSpec == nil || pgtdeSpec.Vault == nil { | |
| err := errors.New("pg_tde is enabled but spec.extensions.pg_tde.vault is not configured") | |
| log.Error(err, "pg_tde vault provider configuration is missing") | |
| return err | |
| } | |
| if cluster.Status.PGTDERevision == "" { | |
| return pgtde.AddVaultProvider(ctx, exec, pgtdeSpec.Vault) | |
| } | |
| return pgtde.ChangeVaultProvider(ctx, exec, pgtdeSpec.Vault) |
| // EnableInPostgreSQL installs pgvector triggers into every database. | ||
| func EnableInPostgreSQL(ctx context.Context, exec postgres.Executor) error { | ||
| log := logging.FromContext(ctx) |
There was a problem hiding this comment.
- Problem: The file-level comment for
EnableInPostgreSQLmentions pgvector triggers, but the function enablespg_tde. - Why it matters: Incorrect comments slow down reviews/debugging and can mislead future changes.
- Fix: Update the comment to describe pg_tde (e.g., installing/updating the
pg_tdeextension in all databases).
| caSecretPath := "NULL" | ||
| if vault.CASecret.Key != "" { | ||
| caSecretPath = fmt.Sprintf("'%s'", naming.PGTDEMountPath+"/"+vault.CASecret.Key) | ||
| } | ||
|
|
||
| stdout, stderr, err := exec.Exec(ctx, | ||
| strings.NewReader(strings.Join([]string{ | ||
| // Quiet NOTICE messages from IF NOT EXISTS statements. | ||
| // - https://www.postgresql.org/docs/current/runtime-config-client.html | ||
| `SET client_min_messages = WARNING;`, | ||
| fmt.Sprintf(`SELECT pg_tde_add_global_key_provider_vault_v2( | ||
| '%s', '%s', '%s', '%s', %s | ||
| );`, | ||
| naming.PGTDEVaultProvider, | ||
| vault.Host, | ||
| vault.MountPath, | ||
| naming.PGTDEMountPath+"/"+vault.TokenSecret.Key, | ||
| caSecretPath, | ||
| ), | ||
| fmt.Sprintf("SELECT pg_tde_create_key_using_global_key_provider('%s', '%s');", | ||
| naming.PGTDEGlobalKey, | ||
| naming.PGTDEVaultProvider, | ||
| ), | ||
| fmt.Sprintf("SELECT pg_tde_set_default_key_using_global_key_provider('%s', '%s');", | ||
| naming.PGTDEGlobalKey, | ||
| naming.PGTDEVaultProvider, | ||
| ), | ||
| }, "\n")), | ||
| map[string]string{ | ||
| "ON_ERROR_STOP": "on", // Abort when any one statement fails. | ||
| "QUIET": "on", // Do not print successful statements to stdout. | ||
| }, nil) |
There was a problem hiding this comment.
- Problem: SQL for configuring Vault is built via
fmt.Sprintfwith unescaped user-provided values (host, mountPath, secret key paths), which can break SQL quoting or allow SQL injection via CR input. - Why it matters: The operator executes this SQL as a privileged user; malformed/hostile input could lead to unexpected SQL execution.
- Fix: Use the existing
internal/postgres.QuoteLiteralhelper (or equivalent) for every SQL string literal, and avoid embedding raw values directly into SQL.
| `SET client_min_messages = WARNING;`, | ||
| fmt.Sprintf(`SELECT pg_tde_change_global_key_provider_vault_v2( | ||
| 'vault-provider', '%s', '%s', '%s', '%s' | ||
| );`, | ||
| vault.Host, | ||
| vault.MountPath, | ||
| naming.PGTDEMountPath+"/"+vault.TokenSecret.Key, | ||
| naming.PGTDEMountPath+"/"+vault.CASecret.Key, | ||
| ), | ||
| }, "\n")), |
There was a problem hiding this comment.
- Problem:
ChangeVaultProviderhardcodes the provider name ('vault-provider') and always passes a CA path even whenCASecretis unset/optional. - Why it matters: This makes the provider name harder to change consistently and can cause reconciliation to fail indefinitely for configurations that omit
caSecret. - Fix: Use
naming.PGTDEVaultProviderinstead of a string literal, and mirrorAddVaultProviderby passingNULL(or equivalent) when no CA secret is configured.
| // The specification of extensions. | ||
| // +operator-sdk:csv:customresourcedefinitions:type=spec | ||
| // +optional | ||
| Extensions ExtensionsSpec `json:"extensions,omitempty"` | ||
| Extensions ExtensionsSpec `json:"extensions"` | ||
|
|
There was a problem hiding this comment.
- Problem:
Extensionswas changed fromjson:"extensions,omitempty"tojson:"extensions", which will make.spec.extensionsrequired in the generated CRD schema. - Why it matters: This is a breaking API change; existing clusters created without
spec.extensionsmay fail validation on update after the CRD is upgraded. - Fix: Keep
omitempty(or otherwise ensure the CRD does not require this field) and rely on defaulting/webhook logic to populate subfields as needed.
| run_psql_command() { | ||
| local command=${1} | ||
| local uri=${2} | ||
| local driver=${3:-postgres} | ||
|
|
||
| kubectl -n ${NAMESPACE} exec $(get_client_pod) -- \ | ||
| psql -v ON_ERROR_STOP=1 -t -q "${uri}" -c "${command}" | ||
| } |
There was a problem hiding this comment.
- Problem:
run_psql_commanddefines adrivervariable but never uses it. - Why it matters: Dead code in test helpers makes scripts harder to maintain and can confuse future edits.
- Fix: Remove the unused
driverparameter/variable, or incorporate it into the connection invocation (consistent withrun_psql_local).
1 participant
CHANGE DESCRIPTION
Problem:
Short explanation of the problem.
Cause:
Short explanation of the root cause of the issue if applicable.
Solution:
Short explanation of the solution we are providing with this PR.
CHECKLIST
Jira
Needs Doc) and QA (Needs QA)?Tests
Config/Logging/Testability