feat(databases): pass through arbitrary PostgreSQL query params#415
Merged
feat(databases): pass through arbitrary PostgreSQL query params#415
Conversation
Previously, GetPostgresEnvVars() reconstructed POSTGRES_URI from scratch and only supported two internal query params (disableSSLMode and secret). All other query parameters were silently dropped, blocking deployments on managed PostgreSQL providers like Google Cloud SQL that require params such as sslmode=require or tcpKeepAlive=true. Now all query parameters from the Settings URI are preserved in the final POSTGRES_URI, except for the operator-internal ones (disableSSLMode and secret) which are still consumed and filtered out. Backward compatibility is fully maintained: disableSSLMode=true still produces sslmode=disable.
Contributor
WalkthroughThis change centralizes PostgreSQL URI query parameter handling by introducing a helper function that filters internal parameters while preserving custom ones. Documentation is updated to explain parameter forwarding and internal parameter handling behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/09-Configuration` reference/01-Settings.md:
- Around line 98-100: Add a fenced-code language specifier to the block
containing the PostgreSQL connection URL (the
``postgresql://user:pass@host:5432?sslmode=require&tcpKeepAlive=true`` example)
so markdownlint MD040 is satisfied; replace the opening ``` with a language
token such as ```text (or another appropriate language) to explicitly mark the
block's language.
In `@internal/resources/databases/mock_test.go`:
- Around line 30-55: The file has formatting drift causing pre-commit/CI
failures; run gofmt (or `gofmt -w`) on internal/resources/databases/mock_test.go
to normalize formatting and then re-stage the file; ensure the changes cover the
mockManager method block (e.g., GetClient, GetScheme, GetAPIReader, Elected,
Start, etc.) so that the entire file is properly formatted before committing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 21ec4392-7768-4363-af84-d0d30620f712
📒 Files selected for processing (5)
docs/05-Infrastructure services/01-PostgreSQL.mddocs/09-Configuration reference/01-Settings.mdinternal/resources/databases/env.gointernal/resources/databases/env_test.gointernal/resources/databases/mock_test.go
Comment on lines
+30
to
+55
| func (m *mockManager) GetClient() client.Client { return m.client } | ||
| func (m *mockManager) GetScheme() *runtime.Scheme { return m.scheme } | ||
| func (m *mockManager) GetAPIReader() client.Reader { return m.client } | ||
| func (m *mockManager) GetCache() cache.Cache { return nil } | ||
| func (m *mockManager) GetConfig() *rest.Config { return nil } | ||
| func (m *mockManager) GetEventRecorderFor(string) record.EventRecorder { return nil } | ||
| func (m *mockManager) GetLogger() logr.Logger { return logr.Discard() } | ||
| func (m *mockManager) GetRESTMapper() meta.RESTMapper { return nil } | ||
| func (m *mockManager) GetFieldIndexer() client.FieldIndexer { return nil } | ||
| func (m *mockManager) GetHTTPClient() *http.Client { return nil } | ||
| func (m *mockManager) GetWebhookServer() webhook.Server { return nil } | ||
| func (m *mockManager) GetMetricsServer() server.Server { return nil } | ||
| func (m *mockManager) GetControllerOptions() config.Controller { return config.Controller{} } | ||
| func (m *mockManager) GetPlatform() core.Platform { return core.Platform{} } | ||
| func (m *mockManager) Add(manager.Runnable) error { return nil } | ||
| func (m *mockManager) Elected() <-chan struct{} { | ||
| ch := make(chan struct{}) | ||
| close(ch) | ||
| return ch | ||
| } | ||
| func (m *mockManager) SetFields(interface{}) error { return nil } | ||
| func (m *mockManager) AddMetricsExtraHandler(string, interface{}) error { return nil } | ||
| func (m *mockManager) AddMetricsServerExtraHandler(string, http.Handler) error { return nil } | ||
| func (m *mockManager) AddHealthzCheck(string, healthz.Checker) error { return nil } | ||
| func (m *mockManager) AddReadyzCheck(string, healthz.Checker) error { return nil } | ||
| func (m *mockManager) Start(context.Context) error { return nil } |
Contributor
There was a problem hiding this comment.
Pre-commit failure points to formatting drift in this file.
CI reports unstaged changes for internal/resources/databases/mock_test.go; please run gofmt on this file and re-stage to unblock the pipeline.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/resources/databases/mock_test.go` around lines 30 - 55, The file has
formatting drift causing pre-commit/CI failures; run gofmt (or `gofmt -w`) on
internal/resources/databases/mock_test.go to normalize formatting and then
re-stage the file; ensure the changes cover the mockManager method block (e.g.,
GetClient, GetScheme, GetAPIReader, Elected, Start, etc.) so that the entire
file is properly formatted before committing.
Dav-14
previously approved these changes
Mar 4, 2026
Dav-14
reviewed
Mar 4, 2026
Contributor
Dav-14
left a comment
Dav-14
left a comment
There was a problem hiding this comment.
why do you need to mock all the client ?. Can you just export the business part we need and use the existing integration test ?
Address PR #415 review feedback: - Extract URI query param logic into exported BuildPostgresQueryString() - Simplify tests to call pure function directly (no fake k8s client) - Remove mock_test.go (no longer needed) - Fix markdown lint: add language identifier to fenced code block
gfyrag
approved these changes
Mar 6, 2026
Contributor
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 `@internal/resources/databases/env.go`:
- Around line 63-64: The formatted string `f` built for the Postgres URI can
contain percent signs from BuildPostgresQueryString(database.Status.URI.Query())
which will be interpreted as format directives when later used with
core.ComputeEnvVar (which calls fmt.Sprintf); fix by escaping percent signs in
the encoded query (replace "%" with "%%") before concatenating into `f` or by
moving the query portion out of the format template passed to ComputeEnvVar so
the query is appended after fmt.Sprintf; update the logic around the `f`
construction (where `f += "?" + encoded`) and ensure ComputeEnvVar is called
with a safe format string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 55b36659-9a55-4815-be2a-2246f97edcc5
📒 Files selected for processing (3)
docs/09-Configuration reference/01-Settings.mdinternal/resources/databases/env.gointernal/resources/databases/env_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/09-Configuration reference/01-Settings.md
- internal/resources/databases/env_test.go
Comment on lines
+63
to
+64
| if encoded := BuildPostgresQueryString(database.Status.URI.Query()); encoded != "" { | ||
| f += "?" + encoded |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== ComputeEnvVar definition ==\n'
rg -n -A20 -B2 'func ComputeEnvVar\('
printf '\n== Formatting calls near ComputeEnvVar ==\n'
rg -n -C4 'fmt\.(Sprintf|Errorf|Fprintf)\('
printf '\n== POSTGRES_URI assembly site ==\n'
rg -n -A6 -B4 'BuildPostgresQueryString|POSTGRES_URI|ComputeEnvVar\(' internal/resources/databases/env.goRepository: formancehq/operator
Length of output: 50375
Escape percent signs in encoded query strings before passing to fmt.Sprintf.
At lines 63-64, the encoded query string is appended to f which is then passed to core.ComputeEnvVar. Since ComputeEnvVar uses fmt.Sprintf(format, ...) with f as the format string, any percent-encoded characters from url.Values.Encode() (e.g., %20, %40) will be misinterpreted as format directives, causing POSTGRES_URI corruption or runtime errors. Either escape % to %% in the encoded query string before concatenating, or append the query string outside the format template passed to ComputeEnvVar.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/resources/databases/env.go` around lines 63 - 64, The formatted
string `f` built for the Postgres URI can contain percent signs from
BuildPostgresQueryString(database.Status.URI.Query()) which will be interpreted
as format directives when later used with core.ComputeEnvVar (which calls
fmt.Sprintf); fix by escaping percent signs in the encoded query (replace "%"
with "%%") before concatenating into `f` or by moving the query portion out of
the format template passed to ComputeEnvVar so the query is appended after
fmt.Sprintf; update the logic around the `f` construction (where `f += "?" +
encoded`) and ensure ComputeEnvVar is called with a safe format string.
Dav-14
approved these changes
Mar 6, 2026
flemzord
added a commit
that referenced
this pull request
Mar 13, 2026
The awsRole query parameter is an internal operator parameter (legacy mechanism for IAM role configuration) that should not be forwarded to PostgreSQL. Since #415 introduced arbitrary query param pass-through, awsRole was being sent to PostgreSQL which rejects it with "unrecognized configuration parameter".
2 tasks
flemzord
added a commit
that referenced
this pull request
Mar 13, 2026
…ugh (#424) The awsRole query parameter is an internal operator parameter (legacy mechanism for IAM role configuration) that should not be forwarded to PostgreSQL. Since #415 introduced arbitrary query param pass-through, awsRole was being sent to PostgreSQL which rejects it with "unrecognized configuration parameter".
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
POSTGRES_URIenvironment variable. Previously onlydisableSSLModeandsecretwere handled; all other params were silently dropped.sslmode=requireortcpKeepAlive=true.disableSSLMode=truestill producessslmode=disable,secretis still consumed for credentials and filtered out.Changes
internal/resources/databases/env.godisableSSLModeandsecretinternal/resources/databases/env_test.goGetPostgresEnvVarsinternal/resources/databases/mock_test.godocs/09-Configuration reference/01-Settings.mddocs/05-Infrastructure services/01-PostgreSQL.mdTest plan
go test ./internal/resources/databases/...— all 7 new unit tests passgo vet ./...— no issuesgo build ./...— compiles cleanly