Harmonize configs: refactor database config nesting#88
Conversation
WalkthroughThe changes restructure how database configuration is represented and accessed across the project. Database settings are moved from being nested within server configuration objects to being top-level fields in both code and configuration files. Struct definitions, configuration loading, and usage are updated accordingly, with no changes to logic or error handling. Changes
Sequence Diagram(s)sequenceDiagram
participant ConfigFile
participant AppConfig
participant Main
participant Database
ConfigFile->>AppConfig: Load config (database.dsn at top-level)
AppConfig->>Main: Provide AppConfig.Database.DSN
Main->>Database: Initialize with AppConfig.Database.DSN
Database-->>Main: Connection established
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)level=warning msg="[runner] Can't run linter goanalysis_metalinter: buildir: failed to load package session: could not load export data: no export data for "github.com/vultisig/go-wrappers/go-dkls/sessions"" 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
api/server_config.go (1)
11-15: 💡 Verification agent🧩 Analysis chain
Verify callers no longer expect
ServerConfig.DatabaseThe struct change is widespread; ensure no remaining references compile.
🏁 Script executed:
#!/bin/bash # Find legacy accesses to removed field rg --line-number '\.Database\.DSN' --glob '*.go'Length of output: 474
Fix remaining references to
cfg.Database.DSNThe
Databasefield was removed fromServerConfig, but these calls are still referencing it. Please update them to use the new configuration fields (e.g. a top-levelDSNor a renamed config struct).Files to update:
- cmd/dca/server/main.go:53
- cmd/payroll/server/main.go:54
- cmd/payroll/worker/main.go:62
Replace occurrences of
postgres.NewPostgresBackend(cfg.Database.DSN, …)with the correct new field, for example:
postgres.NewPostgresBackend(cfg.Postgres.DSN, …)(or whatever the updated config path is).
♻️ Duplicate comments (2)
cmd/payroll/server/main.go (1)
54-57: Same DSN-logging issue as in DCA serverThe DSN handed to
postgres.NewPostgresBackendis logged verbatim inside that function, leaking the DB password.Reuse the mitigation proposed in the DCA server comment (mask before logging or refactor the backend constructor).
config-verifier.yaml (1)
11-13: Duplicate: plain-text DB credentials committedSame issue and remediation as in
config-plugin.yaml.🧰 Tools
🪛 Checkov (3.2.334)
[MEDIUM] 12-13: Basic Auth Credentials
(CKV_SECRET_4)
🧹 Nitpick comments (3)
api/server_config.go (1)
4-10: Comments are stale after removingDatabasefieldDoc-comment lines 9–10 still describe the
Databasesubsection that no longer exists inServerConfig, which will confuse users and tooling that parse godoc.-// - Database: Configuration for the database connection, including: -// - DSN: The Data Source Name (DSN) for connecting to the database.Please delete or update the comment to reflect the current struct definition.
cmd/dca/server/config.go (2)
44-46: Tweak the error message wording
"fail to reading config file"feels unpolished and is inconsistent with the Payroll variant.- return nil, fmt.Errorf("fail to reading config file, %w", err) + return nil, fmt.Errorf("failed to read config file: %w", err)
28-35: Consider renamingGetConfigure()toGetConfig()The current name is slightly awkward and differs from the idiom used elsewhere (
ReadConfig). Renaming improves readability and discoverability.Refactor impact is local to the two call-sites found in
main.go; no public API break.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/migration-test.yml(1 hunks)api/server_config.go(1 hunks)cmd/dca/server/config.go(1 hunks)cmd/dca/server/main.go(1 hunks)cmd/payroll/server/config.go(1 hunks)cmd/payroll/server/main.go(1 hunks)config-plugin.yaml(1 hunks)config-verifier.yaml(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
cmd/dca/server/main.go (1)
storage/postgres/db.go (1)
NewPostgresBackend(28-52)
cmd/dca/server/config.go (2)
api/server.go (1)
Server(32-45)api/server_config.go (1)
ServerConfig(11-15)
cmd/payroll/server/main.go (1)
storage/postgres/db.go (1)
NewPostgresBackend(28-52)
cmd/payroll/server/config.go (1)
api/server_config.go (1)
ServerConfig(11-15)
🪛 Checkov (3.2.334)
config-plugin.yaml
[MEDIUM] 14-15: Basic Auth Credentials
(CKV_SECRET_4)
config-verifier.yaml
[MEDIUM] 12-13: Basic Auth Credentials
(CKV_SECRET_4)
| db, err := postgres.NewPostgresBackend(cfg.Database.DSN, nil) | ||
| if err != nil { | ||
| logger.Fatalf("Failed to connect to database: %v", err) |
There was a problem hiding this comment.
Avoid logging raw DSNs – credentials will leak to logs
postgres.NewPostgresBackend logs the full DSN (see storage/postgres/db.go:28). Because the DSN contains the DB password, every server start currently prints credentials to your log aggregator.
-db, err := postgres.NewPostgresBackend(cfg.Database.DSN, nil)
+// TODO: strip password before passing to NewPostgresBackend or change the
+// backend constructor to accept a masked string for logging purposes.
+db, err := postgres.NewPostgresBackend(cfg.Database.DSN, nil)Consider:
- Changing
NewPostgresBackendto log a redacted DSN (e.g.,postgres://****@host/db). - Passing the password via
PGPASSWORD/SSLKEYenv vars instead of embedding it in the DSN.
This is a high-risk credential-exposure vector.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| db, err := postgres.NewPostgresBackend(cfg.Database.DSN, nil) | |
| if err != nil { | |
| logger.Fatalf("Failed to connect to database: %v", err) | |
| // TODO: strip password before passing to NewPostgresBackend or change the | |
| // backend constructor to accept a masked string for logging purposes. | |
| db, err := postgres.NewPostgresBackend(cfg.Database.DSN, nil) | |
| if err != nil { | |
| logger.Fatalf("Failed to connect to database: %v", err) | |
| } |
🤖 Prompt for AI Agents
In cmd/dca/server/main.go around lines 53 to 55, the current code passes the
full DSN containing credentials to NewPostgresBackend, which logs the raw DSN
and exposes sensitive information. To fix this, modify NewPostgresBackend in
storage/postgres/db.go to redact sensitive parts of the DSN (like the password)
before logging, replacing them with placeholders such as "****". Additionally,
refactor the code to avoid embedding the password in the DSN string by passing
it through environment variables like PGPASSWORD or SSLKEY instead, ensuring
credentials are not exposed in logs.
| database: | ||
| dsn: postgres://myuser:mypassword@localhost:5432/vultisig-plugin?sslmode=disable | ||
|
|
There was a problem hiding this comment.
Plain-text DB credentials in committed config
The dsn embeds myuser:mypassword directly in a repo-tracked YAML file. Even for local/dev use, this normalises storing secrets in VCS.
Recommended:
-dsn: postgres://myuser:mypassword@localhost:5432/vultisig-plugin?sslmode=disable
+dsn: ${PLUGIN_DB_DSN:?set PLUGIN_DB_DSN in env/secret manager}Pair this with a .env, GitHub Actions secret, or HashiCorp Vault entry.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| database: | |
| dsn: postgres://myuser:mypassword@localhost:5432/vultisig-plugin?sslmode=disable | |
| database: | |
| - dsn: postgres://myuser:mypassword@localhost:5432/vultisig-plugin?sslmode=disable | |
| + dsn: ${PLUGIN_DB_DSN:?set PLUGIN_DB_DSN in env/secret manager} |
🧰 Tools
🪛 Checkov (3.2.334)
[MEDIUM] 14-15: Basic Auth Credentials
(CKV_SECRET_4)
🤖 Prompt for AI Agents
In config-plugin.yaml around lines 13 to 15, the database DSN contains
plain-text credentials directly in the version-controlled file, which is
insecure. Remove the hardcoded username and password from the DSN and instead
reference environment variables or external secret management solutions like a
.env file or Vault. Update the configuration to load these sensitive values
dynamically at runtime rather than storing them in the YAML file.
| Server api.ServerConfig `mapstructure:"server" json:"server"` | ||
| Database struct { | ||
| DSN string `mapstructure:"dsn" json:"dsn,omitempty"` | ||
| } `mapstructure:"database" json:"database,omitempty"` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Factor Database into a shared type to avoid copy-paste divergence
Database is now declared anonymously in both DCA and Payroll configs (and likely elsewhere).
Anonymous duplication makes future additions (e.g., max-open-conns) error-prone because every struct has to be edited.
+type DatabaseConfig struct {
+ DSN string `mapstructure:"dsn" json:"dsn,omitempty"`
+}
type DCAServerConfig struct {
Server api.ServerConfig `mapstructure:"server" json:"server"`
- Database struct {
- DSN string `mapstructure:"dsn" json:"dsn,omitempty"`
- } `mapstructure:"database" json:"database,omitempty"`
+ Database DatabaseConfig `mapstructure:"database" json:"database,omitempty"`
…
}Placing DatabaseConfig in plugin/api (or a small config package) keeps the definition single-sourced and import-friendly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Server api.ServerConfig `mapstructure:"server" json:"server"` | |
| Database struct { | |
| DSN string `mapstructure:"dsn" json:"dsn,omitempty"` | |
| } `mapstructure:"database" json:"database,omitempty"` | |
| // Extract a shared DatabaseConfig type (e.g. in plugin/api or a config package) | |
| type DatabaseConfig struct { | |
| DSN string `mapstructure:"dsn" json:"dsn,omitempty"` | |
| } | |
| type DCAServerConfig struct { | |
| Server api.ServerConfig `mapstructure:"server" json:"server"` | |
| Database DatabaseConfig `mapstructure:"database" json:"database,omitempty"` | |
| … | |
| } |
🤖 Prompt for AI Agents
In cmd/dca/server/config.go around lines 15 to 18, the Database struct is
declared anonymously, causing duplication across DCA and Payroll configs. To fix
this, define a shared DatabaseConfig struct in a common package like plugin/api
or a dedicated config package, then replace the anonymous Database struct with
this shared type in all relevant configs. This centralizes the definition and
prevents divergence when adding new fields.
| "jwt_secret": "test-secret" | ||
| }, | ||
| "database": { | ||
| "dsn": "postgres://myuser:mypassword@localhost:5432/vultisig-plugin?sslmode=disable" | ||
| }, |
There was a problem hiding this comment.
Config key mismatch: jwt_secret vs encryption_secret
api.ServerConfig exposes EncryptionSecret, but the JSON fixture still uses jwt_secret.
This field will be silently dropped by viper.Unmarshal, so authentication may break at runtime.
Change the snippet to:
- "jwt_secret": "test-secret"
+ "encryption_secret": "test-secret"(or add a mapstructure tag alias if you must retain the old name).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "jwt_secret": "test-secret" | |
| }, | |
| "database": { | |
| "dsn": "postgres://myuser:mypassword@localhost:5432/vultisig-plugin?sslmode=disable" | |
| }, | |
| "encryption_secret": "test-secret" | |
| }, | |
| "database": { | |
| "dsn": "postgres://myuser:mypassword@localhost:5432/vultisig-plugin?sslmode=disable" | |
| }, |
🧰 Tools
🪛 Checkov (3.2.334)
[MEDIUM] 58-59: Basic Auth Credentials
(CKV_SECRET_4)
🤖 Prompt for AI Agents
In .github/workflows/migration-test.yml around lines 55 to 59, the configuration
key "jwt_secret" should be renamed to "encryption_secret" to match the expected
field name in api.ServerConfig. Update the JSON fixture to use
"encryption_secret" instead of "jwt_secret" to ensure viper.Unmarshal correctly
maps the value and prevents runtime authentication issues.
2 participants
Database configuration is nested within the Server data structure. This is inconsistent with the configuration in the verifier repository. The database should be a dependency that the server uses, not a server property
Tested by running dca/payroll workers + servers locally. Database migrations and start up runs fine
Summary by CodeRabbit