Add missing envs for external pg db #1689
Conversation
WalkthroughAdds POSTGRES_CONNECTION_STRING_PATH to internal deployment and statefulset manifests and updates their embedded SHA256s; setDesiredCoreEnv now assigns that env var only when ExternalPgSecret is present; 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 to POSTGRES_CONNECTION_STRING_PATH
setDesiredCoreEnv-->>Reconciler: POSTGRES_CONNECTION_STRING_PATH="/etc/postgres-secret/db_url"
else ExternalPgSecret absent
note right of setDesiredCoreEnv #FDEBD0: leave POSTGRES_CONNECTION_STRING_PATH unset
setDesiredCoreEnv-->>Reconciler: POSTGRES_CONNECTION_STRING_PATH not assigned
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
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 |
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
from
0d2e1a3 to
706506e
Compare
Signed-off-by: Anisur Rahman <[email protected]>
anisurrahman75
force-pushed
the
add-missing-env
branch
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 👍 |
7 participants
Testing Instructions:
noobaa install --postgres-url=''Summary by CodeRabbit
New Features
Bug Fixes / Chores