feat(spider-storage)!: Read database credentials via environment variables.#397
Merged
LinZhihao-723 merged 6 commits intoJul 15, 2026
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughChangesDatabase configuration now uses nested credentials loaded from Database credential loading
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant TestTask
participant DatabaseConfig
participant DatabaseCredentials
participant MariaDbStorageConnector
TestTask->>DatabaseConfig: deserialize configuration
DatabaseConfig->>DatabaseCredentials: resolve credentials
DatabaseCredentials-->>DatabaseConfig: username and password
DatabaseConfig->>MariaDbStorageConnector: provide credentials
MariaDbStorageConnector->>MariaDbStorageConnector: configure MySQL authentication
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
20001020ycx
changed the title
20001020ycx
force-pushed
the
feat/2026-07-14-storage-db-password-env
branch
from
0c16a54 to
ff83d3d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
components/spider-storage/src/config.rs (1)
38-77: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider deriving
DefaultforDatabaseCredentials.Since
Stringandsecrecy::SecretStringboth implement theDefaulttrait, you can simplify the code by derivingDefaultautomatically and removing the manual implementation block.♻️ Proposed refactor
-#[derive(Clone, Debug)] +#[derive(Clone, Debug, Default)] pub struct DatabaseCredentials { pub username: String, pub password: SecretString, } impl DatabaseCredentials { /// Reads the database credentials from the [`DB_USERNAME_ENV`] and [`DB_PASSWORD_ENV`] /// environment variables. /// /// # Returns /// /// The credentials on success. /// /// # Errors /// /// Returns an error if: /// /// * [`CredentialsError::MissingEnvVar`] if either environment variable is unset. pub fn from_env() -> Result<Self, CredentialsError> { let username = std::env::var(DB_USERNAME_ENV) .map_err(|_| CredentialsError::MissingEnvVar(DB_USERNAME_ENV))?; let password = std::env::var(DB_PASSWORD_ENV) .map_err(|_| CredentialsError::MissingEnvVar(DB_PASSWORD_ENV))?; Ok(Self { username, password: SecretString::from(password), }) } } - -impl Default for DatabaseCredentials { - fn default() -> Self { - Self { - username: String::new(), - password: SecretString::from(String::new()), - } - } -}🤖 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 `@components/spider-storage/src/config.rs` around lines 38 - 77, Derive Default on DatabaseCredentials alongside its existing Clone and Debug derives, then remove the manual impl Default for DatabaseCredentials block. Preserve from_env and all field types unchanged.
🤖 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 `@components/spider-storage/src/config.rs`:
- Around line 38-77: Derive Default on DatabaseCredentials alongside its
existing Clone and Debug derives, then remove the manual impl Default for
DatabaseCredentials block. Preserve from_env and all field types unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 57b10cc5-b7ec-48ee-8acd-c54e2a8382a0
📒 Files selected for processing (5)
components/spider-storage/src/bin/grpc_server.rscomponents/spider-storage/src/config.rscomponents/spider-storage/src/db/mariadb.rscomponents/spider-storage/src/lib.rscomponents/spider-storage/tests/mariadb_infra.rs
Storage read the database username and password from its config file, which the Helm chart renders into a Kubernetes ConfigMap -- leaving the password in plaintext. It now reads them from the `SPIDER_STORAGE_DB_USERNAME` and `SPIDER_STORAGE_DB_PASSWORD` environment variables (both required) via a dedicated `DatabaseCredentials` type, so they can instead be injected from a Secret and kept out of the ConfigMap. BREAKING CHANGE: the storage service now requires the database credentials to be provided via environment variables; they are no longer read from the config file.
20001020ycx
force-pushed
the
feat/2026-07-14-storage-db-password-env
branch
from
ff83d3d to
d4e4473
Compare
3 tasks
LinZhihao-723
left a comment
LinZhihao-723
left a comment
Member
There was a problem hiding this comment.
Updated to make use of serde to implement the following behavior:
- The credentails can be explicitly given in the config, but it is optional.
- When it's optional, the config is read from the environment variable.
- No need to explicitly read from env. This is automatically resolved by
serde.
- No need to explicitly read from env. This is automatically resolved by
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@components/spider-storage/src/config.rs`:
- Around line 34-115: Update DatabaseConfig deserialization to pass an absent
credentials field through DatabaseCredentials::try_from, rather than requiring
the field before that conversion runs. Use a raw intermediate configuration or
equivalent with credentials represented as optional, ensuring both omitted and
null values fall back to DatabaseCredentials::from_env while explicitly provided
credentials remain unchanged.
In `@taskfiles/test.yaml`:
- Around line 220-231: Wrap the long vars list in the requires block across
multiple lines while preserving all four required variable names and the
existing YAML structure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c053c20e-48eb-42ea-a00b-36f65715095a
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
components/spider-storage/Cargo.tomlcomponents/spider-storage/src/config.rscomponents/spider-storage/src/db/mariadb.rscomponents/spider-storage/src/lib.rscomponents/spider-storage/tests/mariadb_infra.rstaskfiles/test.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- components/spider-storage/tests/mariadb_infra.rs
- components/spider-storage/src/db/mariadb.rs
Contributor
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)
components/spider-storage/src/config.rs (1)
106-114: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winContradiction with PR objective regarding configuration file support.
Based on the PR objectives, providing credentials through the configuration file is no longer supported. However, this
try_fromimplementation explicitly processes provided credentials viaSome(RawCredentials { ... }), and the testcredentials_come_from_config_when_presentat line 130 verifies this functionality.If configuration-based credentials are truly meant to be unsupported, consider removing the
Somebranch, marking thecredentialsfield with#[serde(skip)], and relying entirely onfrom_env(). Otherwise, the PR objective might need an update to reflect that config-based overrides are still permitted.🤖 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 `@components/spider-storage/src/config.rs` around lines 106 - 114, Remove configuration-file credential handling from TryFrom for the credentials type by eliminating the Some(RawCredentials { ... }) path and always using from_env(). Mark the credentials field with #[serde(skip)] and update or remove credentials_come_from_config_when_present so tests no longer expect configuration-based overrides.
🤖 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.
Inline comments:
In `@components/spider-storage/src/config.rs`:
- Around line 147-165: Update credentials_resolve_from_env_when_omitted and the
underlying DatabaseCredentials resolver to use an injectable environment source
instead of std::env::set_var/remove_var. Keep the test’s both-set and
missing-variable assertions, while avoiding process-wide environment mutation;
do not rely on making the entire test binary single-threaded unless that is the
established project-wide approach.
---
Outside diff comments:
In `@components/spider-storage/src/config.rs`:
- Around line 106-114: Remove configuration-file credential handling from
TryFrom for the credentials type by eliminating the Some(RawCredentials { ... })
path and always using from_env(). Mark the credentials field with #[serde(skip)]
and update or remove credentials_come_from_config_when_present so tests no
longer expect configuration-based overrides.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7f2c925c-951c-41aa-b83b-28f0363095ee
📒 Files selected for processing (2)
components/spider-storage/src/config.rstaskfiles/test.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- taskfiles/test.yaml
…ightly clippy ICE. `#[test]` followed by a proc-macro attribute (`#[serial]`) trips a nightly rustc ICE (`attribute is missing tokens ... rustc_test_entrypoint_marker`) when compiling the lib-test target, failing `lint-rust`. Expanding `#[serial]` before the built-in `#[test]` marker exists avoids it, leaving the tests otherwise unchanged.
20001020ycx
changed the title
20001020ycx
force-pushed
the
feat/2026-07-14-storage-db-password-env
branch
from
411665a to
5c30201
Compare
Comment on lines
+47
to
+56
| /// The credentials used to connect to the database. | ||
| /// | ||
| /// By default, Spider expects the database credentials to be supplied through the following | ||
| /// environment variables: | ||
| /// | ||
| /// * Database username: `SPIDER_STORAGE_DB_USERNAME` | ||
| /// * Database password: `SPIDER_STORAGE_DB_PASSWORD` | ||
| /// | ||
| /// These can be overridden by explicitly supplying the `credentials` field in the | ||
| /// configuration file. |
20001020ycx
changed the title
LinZhihao-723
approved these changes
Jul 15, 2026
LinZhihao-723
changed the title
sitaowang1998
approved these changes
Jul 15, 2026
20001020ycx
added a commit
to 20001020ycx/spider
that referenced
this pull request
Jul 15, 2026
…ironment variables. Stop rendering the database username/password into the storage ConfigMap. Storage now reads them from `SPIDER_STORAGE_DB_USERNAME`/`SPIDER_STORAGE_DB_PASSWORD` (spider-storage y-scope#397), injected via `secretKeyRef` from the database Secret. The Secret now renders in both bundled and external modes (`root_password` stays bundled-only) so the credentials are available regardless of deployment mode.
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.
Description
This PR extends how database credentials are passed from storage to the database: the username and password can now be supplied through environment variables, not just the config file — addressing the concern of storing them in plaintext (e.g. in a Kubernetes ConfigMap).
Credentials are read from the config file's
credentialsblock when present, and from the environment variables when the block is omitted. We carefully enumerate all possible scenarios and define their behavior:Note
BREAKING CHANGE:
Username and password are optional in config rather mandatory.
Checklist
Validation performed
Added a unit test covering two scenarios: it reads both variables, and it errors when one is unset.
All CI passes
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores