feat(clp-credential-manager): Create `clp-credential-manager` Rust service skeleton. #1668

Eden-D-Zhang · 2025-11-26T02:54:02Z

Description

This PR sets the foundation of the clp-credential-manager service.

Added:

New component: clp-credential-manager Rust Crate.
Dependencies, including those needed in the future (AWS, JWT, SQLx, etc.)
Service configuration structs
Error enumeration considering planned functionality
Basic health route for service API
Barebones service initialization

Next up:

Implement credential DB CRUD operations
Add DB operation auditing

Checklist

The PR satisfies the contribution guidelines.
This is a breaking change and that has been indicated in the PR title, OR this isn't a
breaking change.
Necessary docs have been updated, OR no docs need to be updated.

Validation performed

Cargo fmt, check, clippy

coderabbitai · 2025-11-26T02:54:08Z

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

hoophalab

LGTM in general. There are some comments

components/clp-credential-manager/src/config.rs

components/clp-credential-manager/src/error.rs

components/clp-credential-manager/src/service.rs

hoophalab · 2025-12-01T17:22:47Z

components/clp-credential-manager/src/main.rs

+fn init_tracing() {
+    let env_filter = EnvFilter::try_from_default_env().unwrap_or_else(|_| EnvFilter::new("info"));
+
+    let _ = tracing_subscriber::fmt()
+        .with_env_filter(env_filter)
+        .with_target(false)
+        .json()
+        .try_init();
+}


The tracing_subscriber::fmt()...try_init() call returns a Result which is being dropped, meaning any initialization failure will be ignored, leading to silent bugs.

Possible implementations: we could use anyhow::Result<()>, or define a crate-wide Error type and use Result<(), Error>

Suggested change

fn init_tracing() {

let env_filter = EnvFilter::try_from_default_env().unwrap_or_else(|_| EnvFilter::new("info"));

let _ = tracing_subscriber::fmt()

.with_env_filter(env_filter)

.with_target(false)

.json()

.try_init();

}

fn init_tracing() -> Result<(), Error>{

let env_filter = EnvFilter::try_from_default_env().unwrap_or_else(|_| EnvFilter::new("info"));

tracing_subscriber::fmt()

.with_env_filter(env_filter)

.with_target(false)

.json()

.try_init()?;

}

Zhihao will probably suggest you implementing log file rotation in code.
https://github.com/y-scope/clp/blob/main/components/api-server/src/bin/api_server.rs#L45C1-L66C2

components/clp-credential-manager/src/routes/mod.rs

components/clp-credential-manager/src/error.rs

…s` directly

… into cred-mgr-pr1

hoophalab

A few comments to make the code more rusty, otherwise all good. @LinZhihao-723 you can start having another review.

components/clp-credential-manager/src/config.rs

hoophalab · 2025-12-01T23:30:38Z

components/clp-credential-manager/src/main.rs

+        .try_init()
+        .map_err(|err| anyhow::Error::msg(err.to_string()))


Suggested change

.try_init()

.map_err(|err| anyhow::Error::msg(err.to_string()))

.try_init()?

This change isn't working for me:

"? couldn't convert the error: dyn StdError + Send + Sync: Sized is not satisfied"

Co-authored-by: hoophalab <200652805+hoophalab@users.noreply.github.com>

LinZhihao-723

In general a great job, but still some things to fix. @hoophalab please also check my comments so that you can help catch more later.
I think there are two lessons learned in this PR:

As your first PR to Rust this is still too large. Personally, I would say this PR can be split into three PRs: one to set up the error handling system, one to set up the server skeleton like #1715, and last one for the rest. In that way, you get more chance to learn step by step and save more time on the later review process.
When contributing to sth you're not familiar with, you can check what other components are doing. We had similar set up for the API server and clp-rust-utils a month ago, which can be references to you to pick up.

components/clp-credential-manager/Cargo.toml

components/clp-credential-manager/src/routes/mod.rs

LinZhihao-723 · 2025-12-03T00:01:55Z

components/clp-credential-manager/src/routes/mod.rs

+pub fn build_router() -> Router<AppState> {
+    Router::new()
+        .route("/health", get(health::health_check))
+        .layer(TraceLayer::new_for_http())
+}


Can u pass the state into this method to be consistent with the API server?

clp/components/api-server/src/routes.rs

Line 38 in 2534e39

.with_state(client)

I had trouble getting the above suggestion to work, plus it seems the current implementation is actually recommended.

https://docs.rs/axum/0.8.7/axum/routing/struct.Router.html#returning-routers-with-states-from-functions

kk thx for referring to the doc. Let me check.

components/clp-credential-manager/src/routes/health.rs

LinZhihao-723 · 2025-12-03T00:46:33Z

components/clp-credential-manager/src/config.rs

+/// Supplies the default `MySQL` server port (`3306`).
+const fn default_mysql_port() -> u16 {
+    3306
+}
+
+/// Supplies the default maximum connection count for the `MySQL` pool.
+const fn default_max_connections() -> u32 {
+    DEFAULT_MAX_CONNECTIONS
+}


We should group all "default" wrappers together.

Not sure but I guess Eden might don't want to provide default value for every field? So he cannot use impl Default for ...

Let's wait for @junhaoliao to check whether we should share the DB credential config schema with CLP DB config. If they're shareable then we can get rid of this entirely.

LinZhihao-723 · 2025-12-03T00:46:53Z

components/clp-credential-manager/src/config.rs

+
+/// Connection options for the `MySQL` backend that stores credentials.
+#[derive(Debug, Clone, Deserialize)]
+pub struct DatabaseConfig {


pub should comes before private.

This might be irrelevant pending the option of using the clp_rust_utils config instead, right?

Yes. But it's a heads up for the proper ordering.

components/clp-credential-manager/src/config.rs

LinZhihao-723 · 2025-12-03T00:52:07Z

components/clp-credential-manager/src/main.rs

+
+    init_tracing()?;
+
+    let config = AppConfig::from_file(&args.config)?;


Back to the previous comment: if you want to make the errors traceable, you should log the error before return. Otherwise when you start this thing inside docker compose I'm not sure how that error will be reflected.

I'm not sure I understand what you mean here; can you explain what I need to add here?

Co-authored-by: Lin Zhihao <59785146+LinZhihao-723@users.noreply.github.com>

Eden-D-Zhang added 3 commits November 24, 2025 19:12

Setup clp-credential-manager skeleton

1251447

Remove unused code

56b163f

Update docstrings

b8c0177

Eden-D-Zhang requested a review from a team as a code owner November 26, 2025 02:54

Lint

e0d0e25

hoophalab self-requested a review December 1, 2025 15:49

hoophalab reviewed Dec 1, 2025

View reviewed changes

Eden-D-Zhang added 5 commits December 1, 2025 14:36

Remove health::router() and reference health check in `routes/mod.r…

c0e6567

…s` directly

Address review

74ac860

Remove ApiError, use only ServiceError instead

3183b97

Merge branch 's3-credentials-manager' of https://github.com/y-scope/clp…

2706f94

… into cred-mgr-pr1

Make status_code function const

202603d

hoophalab previously approved these changes Dec 1, 2025

View reviewed changes

Eden-D-Zhang dismissed hoophalab’s stale review via c2ab9c4 December 1, 2025 23:44

Eden-D-Zhang and others added 2 commits December 1, 2025 18:44

Apply suggestions from code review

c2ab9c4

Co-authored-by: hoophalab <200652805+hoophalab@users.noreply.github.com>

Fix indent

f9fd34f

LinZhihao-723 requested changes Dec 3, 2025

View reviewed changes

Eden-D-Zhang and others added 2 commits December 4, 2025 11:57

Apply suggestions from code review

165981e

Co-authored-by: Lin Zhihao <59785146+LinZhihao-723@users.noreply.github.com>

Address code review

581a9bc

		.try_init()
		.map_err(\|err\| anyhow::Error::msg(err.to_string()))

	.try_init()
	.map_err(\|err\| anyhow::Error::msg(err.to_string()))
	.try_init()?


		init_tracing()?;

		let config = AppConfig::from_file(&args.config)?;

feat(clp-credential-manager): Create clp-credential-manager Rust service skeleton. #1668

Are you sure you want to change the base?

feat(clp-credential-manager): Create clp-credential-manager Rust service skeleton. #1668

Uh oh!

Conversation

Eden-D-Zhang commented Nov 26, 2025

Description

Checklist

Validation performed

Uh oh!

coderabbitai bot commented Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

hoophalab left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hoophalab left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LinZhihao-723 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

feat(clp-credential-manager): Create `clp-credential-manager` Rust service skeleton. #1668

feat(clp-credential-manager): Create `clp-credential-manager` Rust service skeleton. #1668

coderabbitai bot commented Nov 26, 2025 •

edited

Loading