Repository navigation
fix(postgres): accept integer values for postgresql.parameters in schema - #2687
Conversation
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 relaxes the schema validation for PostgreSQL parameters in the Helm chart. By allowing both string and integer types, it aligns the schema validation with the existing template logic that already handles type conversion, ensuring that users can provide numeric values without triggering validation failures. Highlights
New Features🧠 You can now enable Memory (public preview) 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 the 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 counterproductive. 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
|
|
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:
📝 WalkthroughWalkthroughThis PR broadens postgresql.parameters to accept integers or strings (integers coerced to strings) and aligns types/schemas/docs across values.schema.json, values.yaml, the CRD, and ApplicationDefinition. The Helm template's dangerous-parameter list is expanded to block archive_cleanup_command and recovery_end_command. ChangesPostgreSQL Parameters Migration
Sequence Diagram(s)sequenceDiagram
participant User as Chart Values
participant Helm as Helm Template (db.yaml)
participant Validator as Template Guard (dangerousParams)
User->>Helm: supplies `.Values.postgresql.parameters` (string|int)
Helm->>Validator: check parameters map
Validator-->>Helm: fail if blocked param present
Helm-->>User: render/abort based on validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
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 |
There was a problem hiding this comment.
Code Review
This pull request modifies the PostgreSQL application schema to allow integer values for server parameters in addition to strings. However, the feedback points out that manual edits to values.schema.json are discouraged as it is a generated file, and such changes will be overwritten. Additionally, there is a mismatch with the underlying Go types which would lead to runtime unmarshaling errors if an integer is provided. The reviewer suggests updating the type annotations in values.yaml and regenerating the schema to ensure consistency.
| "description": "PostgreSQL server parameters. String values are preferred (quote numbers: \"100\"); integer values are also accepted and converted to strings automatically. BLOCKED (enable arbitrary code execution): archive_command, restore_command, ssl_passphrase_command, dynamic_library_path, local_preload_libraries, session_preload_libraries, shared_preload_libraries. Do NOT override CloudNativePG-managed parameters: archive_mode, primary_conninfo, wal_level, max_replication_slots.", | ||
| "type": "object", | ||
| "default": { | ||
| "max_connections": "100" | ||
| }, | ||
| "additionalProperties": { | ||
| "type": "string" | ||
| "anyOf": [ | ||
| {"type": "string"}, | ||
| {"type": "integer"} | ||
| ] | ||
| } |
There was a problem hiding this comment.
While the intent to allow integer values for PostgreSQL parameters is correct, modifying values.schema.json directly is problematic for several reasons:
- Generated File: According to the repository style guide (line 72),
values.schema.jsonis a generated file that is overwritten when runningmake generate. Any manual changes, including the updated description and theanyOflogic, will be lost in the next build. - Go Type Mismatch: The corresponding Go type for this field in
api/apps/v1alpha1/postgresql/types.gois currentlymap[string]string(line 132). If the schema is relaxed to allow integers but the Go struct remains strictly typed to strings, the Cozystack controller will fail to unmarshal thePostgresresource whenever an integer is provided, leading to a runtime error (e.g.,json: cannot unmarshal number into Go struct field PostgreSQL.parameters of type string). - Metadata Stripping: The style guide also notes that
descriptionfields are stripped from the schema during regeneration.
Recommendation:
You should instead update the type annotations in packages/apps/postgres/values.yaml (specifically the ## @field annotation for parameters at line 92) to a type that supports both strings and integers (such as map[string]any or a union type if supported by the generator) and then run make generate. This will ensure that both the JSON schema and the Go types are updated consistently and persist through future builds.
References
- The style guide states that
values.schema.jsonis a generated file and that manual changes to descriptions or custom annotations will be stripped or overwritten. (link)
f445b11 to
a5040e4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/system/postgres-rd/cozyrds/postgres.yaml`:
- Line 11: Generated CRD/chart schema in
packages/system/postgres-rd/cozyrds/postgres.yaml is out-of-date; run the
project generation step to update artifacts, then commit the regenerated
file(s). Specifically, run the repository generation command (make generate) to
refresh the generated schema that feeds postgres.yaml, verify the resulting
postgres.yaml changes are included, and re-run the pre-commit/checks to ensure
no further diffs remain before pushing the commit.
- Line 11: The description for postgresql.parameters is out of sync with the
schema: additionalProperties.anyOf now allows integers in addition to strings
but the text still says "All values must be strings (quote numbers: \"100\")".
Update the postgresql.parameters description to reflect that parameter values
may be strings or integers (remove the "quote numbers" instruction) and keep the
security/block list text unchanged; ensure the description specifically
references postgresql.parameters and additionalProperties.anyOf so reviewers can
verify the wording matches the schema.
🪄 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: bf58e731-079e-454c-b3f2-8975dffb849b
📒 Files selected for processing (4)
packages/apps/postgres/templates/db.yamlpackages/apps/postgres/values.schema.jsonpackages/apps/postgres/values.yamlpackages/system/postgres-rd/cozyrds/postgres.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/apps/postgres/values.yaml
- packages/apps/postgres/values.schema.json
|
Repro: after running This is the same regeneration trap that #2597 flags about #2466's original Fix has to land in the generator first: cozystack/cozyvalues-gen#25 (reopened) adds an The |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/apps/postgres/values.yaml`:
- Line 92: Update the CI step that installs cozyvalues-gen (referenced in
.github/workflows/pre-commit.yml) to a release that supports intOrString (so it
recognizes "intOrString"/x-kubernetes-int-or-string/intstr.IntOrString), then
locally run `make generate` for the packages/apps/postgres target and commit the
regenerated artifacts (this will update values.yaml-derived generated files to
match the new generator). Ensure the workflow change pins the newer
cozyvalues-gen version and the committed outputs reflect the regenerated files
for the postgres package.
🪄 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: dcbdef5e-d3b9-43e2-b296-73277cee7496
📒 Files selected for processing (5)
api/apps/v1alpha1/postgresql/types.gopackages/apps/postgres/README.mdpackages/apps/postgres/values.schema.jsonpackages/apps/postgres/values.yamlpackages/system/postgres-rd/cozyrds/postgres.yaml
✅ Files skipped from review due to trivial changes (1)
- packages/apps/postgres/README.md
|
Picked this up — the path you outlined worked cleanly. cozyvalues-gen v1.4.0 shipped today with the Bonus: the regenerated Verified end-to-end:
|
The template already coerces all parameter values to strings via
'toString | quote', but the JSON schema had 'additionalProperties: {type: string}'
which caused Helm schema validation to fail before the template ran
when users passed integer values (e.g. max_connections: 100).
Accept both string and integer types in additionalProperties so that
existing configurations with unquoted numbers continue to work.
Signed-off-by: IvanHunters <[email protected]>
Add archive_cleanup_command and recovery_end_command to the dangerous parameters blocklist in db.yaml. Both are PostgreSQL shell-exec hooks identical in threat model to archive_command/restore_command and must not be settable by tenants. Also update the BLOCKED list in values.schema.json, values.yaml, and the embedded openAPISchema in cozyrds/postgres.yaml to reflect the two new blocked parameters. Additionally update the embedded openAPISchema in cozyrds/postgres.yaml to allow integer values for postgresql.parameters (anyOf string|integer), matching the fix already applied to values.schema.json. Signed-off-by: IvanHunters <[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]>
v1.4.0 adds the `intOrString` type alias used by the postgres package's
`postgresql.parameters` schema in this PR. CI must install v1.4.0 so the
"Verify generated code is up to date" check regenerates the same schema
shape (anyOf + x-kubernetes-int-or-string: true) rather than reverting
to {type: string}.
Assisted-By: Claude <[email protected]>
Signed-off-by: Myasnikov Daniil <[email protected]>
Companion regen for the type change in the previous commit. The deepcopy generator emits a fresh import for intstr and updates the inner make() call to match the new map value type. Caught by the codegen-drift check on CI. Generated by `make generate` at repo root (hack/update-codegen.sh). Assisted-By: Claude <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
2cbb87f to
26db323
Compare
|
Successfully created backport PR for |
What this PR does
PostgreSQL server parameters like
max_connectionsare natural integers,but the chart's schema declared them as
{type: string}only. When userssupply bare integers (e.g.
max_connections: 100) — either directly inApplication 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.jsonand the embeddedopenAPISchemain
postgres-rd/cozyrds/postgres.yaml) are generated fromvalues.yamlannotations via
cozyvalues-gen. Hand-editing them does not survivemake generate— same regeneration trap that caught #2466 (issue #2597).cozyvalues-genv1.4.0 (cozystack/cozyvalues-gen#25) adds anintOrStringtype alias for map values that emitsadditionalProperties.anyOf: [integer, string]plus the Kubernetesx-kubernetes-int-or-string: truemarker.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.
values.schema.json, embeddedopenAPISchemain
packages/system/postgres-rd/cozyrds/postgres.yaml,packages/apps/postgres/README.md, andapi/apps/v1alpha1/postgresql/types.go(Parameters is now
map[string]intstr.IntOrString).packages/apps/postgres/templates/db.yaml:$dangerousParamslistextended with
archive_cleanup_commandandrecovery_end_command(same RCE surface as
archive_command/restore_command).Verification
helm templatewithmax_connections: 200(int) andmax_connections: "200"(string) produce identical render output.
archive_cleanup_commandandrecovery_end_commandwith the expectedSECURITY:error.make testinpackages/apps/postgres/: 13/13 pass.make generateis idempotent — schema files don't drift on regen.Release note
Summary by CodeRabbit
New Features
Security
Documentation