Add missing envs for external pg db #1689
Conversation
WalkthroughAdds a new POSTGRES_CONNECTION_STRING_PATH env var to internal deployment and statefulset manifests, removes the hard-coded POSTGRES_HOST value in the statefulset, updates embedded manifests' SHA256s, conditions setting of the env var in runtime only when ExternalPgSecret is present, and appends Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Reconciler
participant setDesiredCoreEnv
participant NooBaaSpec
Reconciler->>setDesiredCoreEnv: request CORE env setup
setDesiredCoreEnv->>NooBaaSpec: check ExternalPgSecret
alt ExternalPgSecret present
note right of setDesiredCoreEnv `#D6EAF8`: assign DB URL path to POSTGRES_CONNECTION_STRING_PATH
setDesiredCoreEnv-->>Reconciler: env includes POSTGRES_CONNECTION_STRING_PATH="/etc/postgres-secret/db_url"
else ExternalPgSecret absent
note right of setDesiredCoreEnv `#FDEBD0`: do not set POSTGRES_CONNECTION_STRING_PATH
setDesiredCoreEnv-->>Reconciler: env omits POSTGRES_CONNECTION_STRING_PATH
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-07-02T04:34:47.006ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (4)
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}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/bundle/deploy.go (1)
4115-4121: Missing runtime consumption for POSTGRES_CONNECTION_STRING_PATHI’ve verified that the operator now injects
POSTGRES_CONNECTION_STRING_PATHconsistently in:
- pkg/system/phase2_creating.go (case “POSTGRES_CONNECTION_STRING_PATH” → sets
…/db_url)- pkg/bundle/deploy.go (both endpoint and core specs)
- deploy/internal/statefulset-core.yaml
- deploy/internal/deployment-endpoint.yaml
However, a repository‐wide grep for any runtime references to
POSTGRES_CONNECTION_STRING_PATH(e.g. viaos.Getenv,os.LookupEnv,ioutil.ReadFile, or similar) returned no hits. This means the container won’t actually read the connection-string file path you’re mounting.Please add (or confirm existing) code in the endpoint/core service to:
- Read
os.Getenv("POSTGRES_CONNECTION_STRING_PATH")(or equivalent) at startup- Load the file at that path (e.g.
ioutil.ReadFile)- Use its contents as the Postgres connection string
Without this, the new env var is declared but never consumed at runtime.
🧹 Nitpick comments (2)
deploy/internal/statefulset-core.yaml (1)
129-134: Optional: keep env ordering consistent with the embedded bundle to reduce noisy diffsHere it’s placed after POSTGRES_PORT_PATH. AI summary indicates deploy.go may place it after POSTGRES_DBNAME_PATH. Functionally equivalent, but inconsistent ordering can cause churn in generated bundles and tests.
Use the script above to compare the relative order in pkg/bundle/deploy.go and align if they differ.
deploy/internal/deployment-endpoint.yaml (1)
120-137: Optional: add an e2e check for external PG via connection string pathGiven the PR objective (“Add missing envs for external pg db”), consider adding/adjusting an e2e test that:
- Mounts a Secret containing a connection string file,
- Sets POSTGRES_CONNECTION_STRING_PATH to that file path,
- Verifies successful startup and DB connectivity.
I can sketch the test plan or a small harness to validate this path-based configuration in CI.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
deploy/internal/deployment-endpoint.yaml(1 hunks)deploy/internal/statefulset-core.yaml(1 hunks)pkg/bundle/deploy.go(2 hunks)
🔇 Additional comments (2)
deploy/internal/deployment-endpoint.yaml (1)
129-129: Confirmed: POSTGRES_CONNECTION_STRING_PATH is in sync
ThePOSTGRES_CONNECTION_STRING_PATHentry appears at line 129 indeploy/internal/deployment-endpoint.yamland is also present at line 4119 in the embedded bundle withinpkg/bundle/deploy.go. No further updates are needed here—bundle and manifest are aligned.pkg/bundle/deploy.go (1)
5229-5234: Verification of POSTGRES_CONNECTION_STRING_PATH usage in manifestsThe
rgoutput confirms that bothstatefulset-core.yamlanddeployment-endpoint.yamlinclude the newPOSTGRES_CONNECTION_STRING_PATHentry alongside the otherPOSTGRES_*_PATHvariables. No other manifests underdeploy/contain this pattern, so parity is maintained across embedded manifests.Next steps:
- Confirm that the application code (or operator) actually reads
POSTGRES_CONNECTION_STRING_PATH, loads the file it points to, and falls back appropriately toPOSTGRES_CONNECTION_STRINGif the path variant is unset.- If the code does not support this fallback, update it or ensure the operator always sets exactly one of the two variables.
Please verify the wiring in the core service’s configuration loader to ensure end-to-end support of the path-based override.
|
@alphaprinz @jackyalbo Please review |
jackyalbo
left a comment
jackyalbo
left a comment
There was a problem hiding this comment.
LGTM, @dannyzaken was this just something we missed?
jackyalbo
left a comment
jackyalbo
left a comment
There was a problem hiding this comment.
@anisurrahman75 Can you run make gen and make gen-api and attach the changes to this PR as it doesn't pass the testes currently
Also, |
anisurrahman75
force-pushed
the
add-missing-env
branch
from
393aed7 to
4973097
Compare
|
@anisurrahman75 could you squash the commits? |
anisurrahman75
force-pushed
the
add-missing-env
branch
from
4973097 to
a3e5df5
Compare
liranmauda
force-pushed
the
add-missing-env
branch
from
a3e5df5 to
bfdad56
Compare
anisurrahman75
force-pushed
the
add-missing-env
branch
from
bfdad56 to
6a76bb7
Compare
|
@liranmauda brother, I've rebased with the latest changes. Can you merge this PR? |
|
The tests are failing. Need to figure out why |
|
Hi |
dannyzaken
left a comment
dannyzaken
left a comment
There was a problem hiding this comment.
@anisurrahman75, I mistakenly approved before.
It is correct to add POSTGRES_CONNECTION_STRING_PATH into the core and endpoint yamls.
In addition, here, we are also missing checking if we are using an external PG before setting a value for that env.
if r.NooBaa.Spec.ExternalPgSecret != nil {
c.Env[j].Value = postgresSecretMountPath + "/db_url"
}
Please ignore this comment. Thanks! |
anisurrahman75
force-pushed
the
add-missing-env
branch
from
6a76bb7 to
6fcbfa7
Compare
anisurrahman75
force-pushed
the
add-missing-env
branch
from
6fcbfa7 to
550e6ad
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pkg/system/phase2_creating.go (1)
557-563: Remove POSTGRES_CONNECTION_STRING_PATH from the endpoint manifest
Drop thePOSTGRES_CONNECTION_STRING_PATHenv var entry fromdeploy/internal/deployment-endpoint.yaml(around line 130); the endpoint doesn’t need DB connectivity by default. Only inject this variable whenExternalPgSecretis set—mirroring the handling instatefulset-core.yaml.pkg/bundle/deploy.go (2)
4099-4115: Add missing POSTGRES_CONNECTION_STRING_PATH to endpoint env.The PR goal mentions adding POSTGRES_CONNECTION_STRING_PATH, but the noobaa-endpoint Deployment still lacks this placeholder. Without it, downstream logic that expects this key (or tooling that checks for parity across POSTGRES_*_PATH vars) will miss it.
Apply this diff right after POSTGRES_PORT_PATH:
- name: POSTGRES_HOST_PATH - name: POSTGRES_USER_PATH - name: POSTGRES_PASSWORD_PATH - name: POSTGRES_DBNAME_PATH - name: POSTGRES_PORT_PATH + - name: POSTGRES_CONNECTION_STRING_PATH
5217-5225: Add missing POSTGRES_CONNECTION_STRING_PATH to core env.Same gap exists in the noobaa-core StatefulSet. Add the placeholder so the operator can populate it when ExternalPgSecret is present.
- name: POSTGRES_HOST_PATH - name: POSTGRES_USER_PATH - name: POSTGRES_PASSWORD_PATH - name: POSTGRES_DBNAME_PATH - name: POSTGRES_PORT_PATH + - name: POSTGRES_CONNECTION_STRING_PATH
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
.gitignore(1 hunks)deploy/internal/deployment-endpoint.yaml(1 hunks)deploy/internal/statefulset-core.yaml(1 hunks)pkg/bundle/deploy.go(4 hunks)pkg/system/phase2_creating.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (2)
- deploy/internal/deployment-endpoint.yaml
- deploy/internal/statefulset-core.yaml
🔇 Additional comments (3)
pkg/bundle/deploy.go (3)
3984-4165: Confirm endpoint actually needs the connection string path.If only core consumes POSTGRES_CONNECTION_STRING_PATH, keep the endpoint manifest lean and drop it there; otherwise, ensure the operator also populates it for endpoints.
3982-3984: Regenerate bundle SHAs after YAML edits
Makefile targets weren’t found in this environment; manually runmake gen && make gen-apilocally and commit the updated Sha256_* constants so CI integrity checks pass.
5091-5296: Operator correctly guards POSTGRES_CONNECTION_STRING_PATH assignmentsetDesiredCoreEnv only injects POSTGRES_CONNECTION_STRING_PATH when ExternalPgSecret is non-nil; please verify the core code path uses a safe file-read (e.g. fs_utils.try_read_file_sync) so an empty or missing path won’t crash the pod.
anisurrahman75
force-pushed
the
add-missing-env
branch
from
550e6ad to
3e15da0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
pkg/system/phase2_creating.go (2)
516-525: Also remove the variable entirely after the loopFilter c.Env to eliminate POSTGRES_CONNECTION_STRING_PATH unless ExternalPgSecret is in use. This guarantees the var is not visible to core when using the internal DB or CNPG.
} + // Drop POSTGRES_CONNECTION_STRING_PATH entirely unless an external PG secret is in use. + if r.NooBaa.Spec.ExternalPgSecret == nil { + filtered := make([]corev1.EnvVar, 0, len(c.Env)) + for _, e := range c.Env { + if e.Name != "POSTGRES_CONNECTION_STRING_PATH" { + filtered = append(filtered, e) + } + } + c.Env = filtered + } + if r.NooBaa.Spec.BucketNotifications.Enabled {Run to verify no empty POSTGRES_CONNECTION_STRING_PATH remains in rendered specs:
#!/bin/bash # Check operator code and embedded manifests for unconditional uses rg -nP "POSTGRES_CONNECTION_STRING_PATH|POSTGRES_PORT_PATH|POSTGRES_\\w+_PATH" -C2 deploy pkg | sed -n '1,200p'
490-492: Prevent core crash: drop POSTGRES_CONNECTION_STRING_PATH when ExternalPgSecret is not setLeaving this env var present (empty or pointing to a non-mounted path) makes core read it and crash (fs.readFileSync on empty/missing path). Set it only when ExternalPgSecret is provided; otherwise clear it here and remove it after the loop (next comment).
Apply this diff:
case "POSTGRES_CONNECTION_STRING_PATH": - if r.NooBaa.Spec.ExternalPgSecret != nil { - c.Env[j].Value = postgresSecretMountPath + "/db_url" - } + if r.NooBaa.Spec.ExternalPgSecret != nil { + c.Env[j].Value = postgresSecretMountPath + "/db_url" + } else { + // ensure no accidental reads by core + c.Env[j].Value = "" + c.Env[j].ValueFrom = nil + }
🧹 Nitpick comments (1)
pkg/system/phase2_creating.go (1)
479-489: Gate the other POSTGRES_*_PATH envs to secret-mounted cases (defensive)Optional hardening: only set file-path envs when CNPG or ExternalPgSecret is used; otherwise clear them to avoid accidental file reads from non-existent mounts.
- case "POSTGRES_DBNAME_PATH": - c.Env[j].Value = postgresSecretMountPath + "/dbname" + case "POSTGRES_DBNAME_PATH": + if r.shouldReconcileCNPGCluster() || r.NooBaa.Spec.ExternalPgSecret != nil { + c.Env[j].Value = postgresSecretMountPath + "/dbname" + } else { + c.Env[j].Value = "" + c.Env[j].ValueFrom = nil + } - case "POSTGRES_PASSWORD_PATH": - c.Env[j].Value = postgresSecretMountPath + "/password" + case "POSTGRES_PASSWORD_PATH": + if r.shouldReconcileCNPGCluster() || r.NooBaa.Spec.ExternalPgSecret != nil { + c.Env[j].Value = postgresSecretMountPath + "/password" + } else { + c.Env[j].Value = "" + c.Env[j].ValueFrom = nil + } - case "POSTGRES_PORT_PATH": - c.Env[j].Value = postgresSecretMountPath + "/port" + case "POSTGRES_PORT_PATH": + if r.shouldReconcileCNPGCluster() || r.NooBaa.Spec.ExternalPgSecret != nil { + c.Env[j].Value = postgresSecretMountPath + "/port" + } else { + c.Env[j].Value = "" + c.Env[j].ValueFrom = nil + } - case "POSTGRES_USER_PATH": - c.Env[j].Value = postgresSecretMountPath + "/username" + case "POSTGRES_USER_PATH": + if r.shouldReconcileCNPGCluster() || r.NooBaa.Spec.ExternalPgSecret != nil { + c.Env[j].Value = postgresSecretMountPath + "/username" + } else { + c.Env[j].Value = "" + c.Env[j].ValueFrom = nil + } - case "POSTGRES_HOST_PATH": - c.Env[j].Value = postgresSecretMountPath + "/host" + case "POSTGRES_HOST_PATH": + if r.shouldReconcileCNPGCluster() || r.NooBaa.Spec.ExternalPgSecret != nil { + c.Env[j].Value = postgresSecretMountPath + "/host" + } else { + c.Env[j].Value = "" + c.Env[j].ValueFrom = nil + }Also applies to: 481-486
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
.gitignore(1 hunks)deploy/internal/deployment-endpoint.yaml(1 hunks)deploy/internal/statefulset-core.yaml(1 hunks)pkg/bundle/deploy.go(4 hunks)pkg/system/phase2_creating.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- .gitignore
- deploy/internal/statefulset-core.yaml
- deploy/internal/deployment-endpoint.yaml
- pkg/bundle/deploy.go
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/system/phase2_creating.go (1)
pkg/apis/noobaa/v1alpha1/noobaa_types.go (1)
NooBaa(40-55)
🔇 Additional comments (1)
pkg/system/phase2_creating.go (1)
557-562: Add conditional removal of POSTGRES_CONNECTION_STRING_PATH for endpoint container
POSTGRES_CONNECTION_STRING_PATHis embedded unconditionally in bothstatefulset-core.yamlanddeployment-endpoint.yaml. Confirm thatr.setDesiredCoreEnv(c)strips this var whenExternalPgSecretis disabled—and implement equivalent stripping in the endpoint creation path (e.g. via asetDesiredEndpointEnvcall).
|
https://github.com/noobaa/noobaa-operator/blob/master/pkg/bundle/deploy.go#L5213 - name: POSTGRES_HOST
value: "noobaa-db-pg-0.noobaa-db-pg"@dannyzaken, I guess, we've to keep this field empty. |
anisurrahman75
force-pushed
the
add-missing-env
branch
from
3e15da0 to
0d2e1a3
Compare
anisurrahman75
force-pushed
the
add-missing-env
branch
2 times, most recently
from
706506e to
b4851d8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/bundle/deploy.go (2)
3982-4166: Add POSTGRES_CONNECTION_STRING_PATH to endpoint envs looks correct; verify population logic.The new POSTGRES_CONNECTION_STRING_PATH entry aligns with supporting an external PG connection string. Please verify:
- The operator populates this env var for the endpoint pod whenever ExternalPgSecret is present (same as for core), and leaves it unset otherwise.
- E2E: with an external PG Secret, endpoint pod spec contains POSTGRES_CONNECTION_STRING_PATH with the expected value.
If both connection string and discrete POSTGRES_* vars can be set, prefer one source of truth and log a warning when both are present.
5092-5300: Core StatefulSet: connection-string path added; ensure safe read and internal-DB defaults remain intact.Good addition of POSTGRES_CONNECTION_STRING_PATH. Two verifications to avoid regressions:
- Core read path: NooBaa core previously used fs.readFileSync() on POSTGRES_CONNECTION_STRING_PATH and could throw when undefined. Confirm the core now guards this (e.g., try_read_file_sync) or only reads it when the env var is set; otherwise, pods without ExternalPgSecret may still crash.
- Internal DB flow: POSTGRES_HOST hard-coded value was removed. Ensure the operator still sets POSTGRES_HOST (and other POSTGRES_* envs) for the managed/internal DB case so core continues to boot without an external secret.
Optionally, add a sanity check: when POSTGRES_CONNECTION_STRING_PATH is set, ignore discrete POSTGRES_* envs and emit a clear log to avoid ambiguity.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.gitignore(1 hunks)deploy/internal/deployment-endpoint.yaml(1 hunks)deploy/internal/statefulset-core.yaml(1 hunks)pkg/bundle/deploy.go(4 hunks)pkg/system/phase2_creating.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- .gitignore
- pkg/system/phase2_creating.go
- deploy/internal/deployment-endpoint.yaml
- deploy/internal/statefulset-core.yaml
yes. for external DB we only use the connection string |
|
I can confirm the related issue is a blocker for a clean installation with an external database Thanks for your work, hopefully will be merged soon 👍 |
|
@dannyzaken @aayushchouhan09 @anisurrahman75 Anything here that we are pending on? |
Signed-off-by: Anisur Rahman <[email protected]>
anisurrahman75
force-pushed
the
add-missing-env
branch
from
2cea237 to
43cbff2
Compare
For External PostgreSQL, I guess this PR is needed. @dannyzaken brother |
There was a problem hiding this comment.
Pull request overview
This PR adds support for external PostgreSQL databases by introducing the missing POSTGRES_CONNECTION_STRING_PATH environment variable for both core and endpoint pods. The key changes ensure that when an external PostgreSQL secret is provided (ExternalPgSecret), the appropriate environment variable is set to point to the connection string file.
Key Changes:
- Conditionalizes
POSTGRES_CONNECTION_STRING_PATHenvironment variable to only be set when using external PostgreSQL - Removes hard-coded
POSTGRES_HOSTdefault value to allow dynamic configuration - Updates both core statefulset and endpoint deployment manifests
- Adds
.idea/to.gitignorefor IDE support
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/system/phase2_creating.go | Conditionalizes POSTGRES_CONNECTION_STRING_PATH environment variable based on ExternalPgSecret |
| deploy/internal/statefulset-core.yaml | Removes hard-coded POSTGRES_HOST value and adds POSTGRES_CONNECTION_STRING_PATH env var |
| deploy/internal/deployment-endpoint.yaml | Adds POSTGRES_CONNECTION_STRING_PATH environment variable |
| pkg/bundle/deploy.go | Updates bundled deployment manifests with new SHA256 checksums and environment variables |
| .gitignore | Adds .idea/ directory for IDE support |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
7 participants
Testing Instructions:
noobaa install --postgres-url=''Summary by CodeRabbit
New Features
Bug Fixes / Chores
✏️ Tip: You can customize this high-level summary in your review settings.