[management] Allow activityStore DSN via env when using postgres#6668
[management] Allow activityStore DSN via env when using postgres#6668Tarasusrus wants to merge 3 commits into
Conversation
initializeConfig() rejected a postgres activityStore.engine whenever activityStore.dsn was empty in config.yaml, even though the store layer already reads NB_ACTIVITY_EVENT_POSTGRES_DSN as a fallback. This blocked env-only/K8s-style deployments from configuring the activity store. Fixes netbirdio#5976. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesActivity Store DSN Env Fallback
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
combined/cmd/root_test.go (1)
18-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOnly the "green" success path is covered; the "red" regression case is missing.
The PR description states this adds a "red/green" test reproducing the original error and verifying the fix, but only the success path (env var set) is present here. Without a companion test asserting
initializeConfig()still errors whenactivityStore.engine: postgresand neitherdsnnor the env var is set, a future refactor could silently reintroduce the original bug (e.g., accidentally checking the wrong env var name) without failing any test.✅ Suggested companion test
func TestInitializeConfig_ActivityStorePostgresMissingDSN(t *testing.T) { dir := t.TempDir() cfgFile := filepath.Join(dir, "config.yaml") yamlContent := ` server: exposedAddress: "https://netbird.example.com:443" dataDir: "` + dir + `" authSecret: "test-secret" activityStore: engine: postgres ` require.NoError(t, os.WriteFile(cfgFile, []byte(yamlContent), 0o600)) oldConfigPath := configPath oldConfig := config t.Cleanup(func() { configPath = oldConfigPath config = oldConfig }) configPath = cfgFile err := initializeConfig() require.Error(t, err, "initializeConfig should reject missing activityStore DSN when no env var is set") }🤖 Prompt for 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. In `@combined/cmd/root_test.go` around lines 18 - 44, The current test only covers the successful env-var path in TestInitializeConfig_ActivityStorePostgresDSNFromEnv; add a companion regression test around initializeConfig that uses the same postgres activityStore config but does not set NB_ACTIVITY_EVENT_POSTGRES_DSN and asserts an error is returned. Keep the setup aligned with the existing test helpers and configPath/config cleanup so the pair of tests covers both the green and red cases.combined/cmd/root.go (1)
152-163: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPresence check doesn't guard against an explicitly-empty env var.
os.LookupEnvonly confirms the variable is set, not that it has a non-empty value. IfNB_ACTIVITY_EVENT_POSTGRES_DSNis set to""(common in some deployment tooling, e.g. templated secrets that resolve to empty), this check passes but the DSN is effectively still missing, silently deferring to a less informative failure deep in the store layer.🐛 Proposed fix
if engineLower == "postgres" && config.Server.ActivityStore.DSN == "" { - if _, ok := os.LookupEnv("NB_ACTIVITY_EVENT_POSTGRES_DSN"); !ok { + if envDSN := os.Getenv("NB_ACTIVITY_EVENT_POSTGRES_DSN"); envDSN == "" { return fmt.Errorf("activityStore.dsn is required when activityStore.engine is postgres (or set NB_ACTIVITY_EVENT_POSTGRES_DSN)") } }🤖 Prompt for 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. In `@combined/cmd/root.go` around lines 152 - 163, The activity store DSN validation in the root command only checks whether NB_ACTIVITY_EVENT_POSTGRES_DSN is present, so an explicitly empty value can still slip through and fail later. Update the validation around config.Server.ActivityStore.Engine in root handling to treat a blank environment value the same as missing by checking the retrieved DSN value for emptiness, not just os.LookupEnv presence. Keep the existing postgres/DSN guard in place, but ensure the error is returned when either config.Server.ActivityStore.DSN or NB_ACTIVITY_EVENT_POSTGRES_DSN is empty, and keep the os.Setenv calls using the normalized engine and resolved DSN as they are.
🤖 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.
Nitpick comments:
In `@combined/cmd/root_test.go`:
- Around line 18-44: The current test only covers the successful env-var path in
TestInitializeConfig_ActivityStorePostgresDSNFromEnv; add a companion regression
test around initializeConfig that uses the same postgres activityStore config
but does not set NB_ACTIVITY_EVENT_POSTGRES_DSN and asserts an error is
returned. Keep the setup aligned with the existing test helpers and
configPath/config cleanup so the pair of tests covers both the green and red
cases.
In `@combined/cmd/root.go`:
- Around line 152-163: The activity store DSN validation in the root command
only checks whether NB_ACTIVITY_EVENT_POSTGRES_DSN is present, so an explicitly
empty value can still slip through and fail later. Update the validation around
config.Server.ActivityStore.Engine in root handling to treat a blank environment
value the same as missing by checking the retrieved DSN value for emptiness, not
just os.LookupEnv presence. Keep the existing postgres/DSN guard in place, but
ensure the error is returned when either config.Server.ActivityStore.DSN or
NB_ACTIVITY_EVENT_POSTGRES_DSN is empty, and keep the os.Setenv calls using the
normalized engine and resolved DSN as they are.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e02aa0e7-5397-4209-b229-67f1fdb4c0f9
📒 Files selected for processing (2)
combined/cmd/root.gocombined/cmd/root_test.go
… test - Treat NB_ACTIVITY_EVENT_POSTGRES_DSN="" the same as unset (os.Getenv instead of os.LookupEnv), so templated-but-empty secrets don't silently pass validation. - Add companion test covering both env unset and env set to "", ensuring initializeConfig still rejects a postgres activityStore with no DSN. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
combined/cmd/root_test.go (2)
72-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
os.Unsetenvdoesn't restore prior env state.Unlike the sibling subtest which uses
t.Setenv(auto-restored), this subtest callsos.Unsetenvdirectly with no save/restore. IfNB_ACTIVITY_EVENT_POSTGRES_DSNhappens to be set in the ambient environment when tests run (plausible given this PR's own use case of env-injected DSNs in CI/K8s), this permanently clears it for the remainder of the test binary process, potentially affecting other tests in the package.🧪 Proposed fix to save/restore env state
t.Run("env var unset", func(t *testing.T) { - os.Unsetenv("NB_ACTIVITY_EVENT_POSTGRES_DSN") + orig, wasSet := os.LookupEnv("NB_ACTIVITY_EVENT_POSTGRES_DSN") + os.Unsetenv("NB_ACTIVITY_EVENT_POSTGRES_DSN") + if wasSet { + t.Cleanup(func() { os.Setenv("NB_ACTIVITY_EVENT_POSTGRES_DSN", orig) }) + } err := initializeConfig() require.Error(t, err, "initializeConfig should reject missing activityStore DSN when no env var is set") })🤖 Prompt for 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. In `@combined/cmd/root_test.go` around lines 72 - 76, The "env var unset" subtest in cmd/root_test.go is mutating NB_ACTIVITY_EVENT_POSTGRES_DSN with os.Unsetenv without restoring the prior value. Update this test to use t.Setenv or explicitly save and restore the original environment state around initializeConfig so the change is isolated to the subtest. Keep the fix near the existing subtests for initializeConfig and preserve the current assertion behavior.
46-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate temp-config/configPath-swap boilerplate across tests.
This setup (write temp
config.yaml, save/swap/restoreconfigPathandconfigglobals) mirrorsTestInitializeConfig_ActivityStorePostgresDSNFromEnv(lines 11-44 per the change summary). Consider extracting a small test helper (e.g.,withTempConfig(t, yamlContent) func()) to reduce duplication across both tests.🤖 Prompt for 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. In `@combined/cmd/root_test.go` around lines 46 - 70, The temp config setup in TestInitializeConfig_ActivityStorePostgresMissingDSN duplicates the same configPath/config swap boilerplate used by TestInitializeConfig_ActivityStorePostgresDSNFromEnv. Extract a small shared test helper around writing the temporary config.yaml and saving/restoring the configPath and config globals, then use it in both tests to remove duplication while keeping the same setup behavior.
🤖 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.
Nitpick comments:
In `@combined/cmd/root_test.go`:
- Around line 72-76: The "env var unset" subtest in cmd/root_test.go is mutating
NB_ACTIVITY_EVENT_POSTGRES_DSN with os.Unsetenv without restoring the prior
value. Update this test to use t.Setenv or explicitly save and restore the
original environment state around initializeConfig so the change is isolated to
the subtest. Keep the fix near the existing subtests for initializeConfig and
preserve the current assertion behavior.
- Around line 46-70: The temp config setup in
TestInitializeConfig_ActivityStorePostgresMissingDSN duplicates the same
configPath/config swap boilerplate used by
TestInitializeConfig_ActivityStorePostgresDSNFromEnv. Extract a small shared
test helper around writing the temporary config.yaml and saving/restoring the
configPath and config globals, then use it in both tests to remove duplication
while keeping the same setup behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 467dbb89-aad6-4d1c-8886-207bf3f5f26c
📒 Files selected for processing (2)
combined/cmd/root.gocombined/cmd/root_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- combined/cmd/root.go
os.Unsetenv had no save/restore, so if NB_ACTIVITY_EVENT_POSTGRES_DSN was already set in the ambient test environment, it stayed cleared for the rest of the test binary process. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
2 participants
Summary
initializeConfig()rejectedserver.activityStore.engine: postgreswheneverserver.activityStore.dsnwas empty inconfig.yaml, even though the store layer (management/server/activity/store/sql_store.go) already readsNB_ACTIVITY_EVENT_POSTGRES_DSNas a fallback.combined/cmd/root.gonow also accepts the DSN viaNB_ACTIVITY_EVENT_POSTGRES_DSNbefore failing.Fixes #5976.
Test plan
combined/cmd/root_test.go) reproducing the exact reported error, now passing after the fixgo test ./combined/cmd/...passesgolangci-lint run ./combined/cmd/...cleangofmtcleanNote for maintainers
While investigating this, I found the same class of problem is worse for
authStore(embedded IdP storage):combined/cmd/config.go:576requiresauthStore.dsninconfig.yamlwith no environment variable fallback at all anywhere in the codebase (unlike activityStore/main store). Kept out of scope here since it's a separate code path; happy to open a follow-up issue/PR if useful.🤖 Generated with Claude Code
Summary by CodeRabbit
server.activityStore.engineis set topostgresand the DSN is missing, the required DSN is now taken from theNB_ACTIVITY_EVENT_POSTGRES_DSNenvironment variable.