Mbe 776 decouple credentialstore from kubers and add more complex #3388

BigLullu · 2025-06-25T05:21:57Z

decoupled Client from kube.rs in CredentialsStore and Credentials and
added unit test leveraging on achieved decoupling

…or certificate retrieval; replace in-method logic with reusable implementation in `cluster_api.rs`.

…et_or_init` in `CredentialStore`

Razz4780 · 2025-06-25T13:10:19Z

Skimmed through the PR:

Need to make sure that unit tests use fresh tempdirs, not ~/.mirrord
Need a test for scenario:
- Folder empty, no credentials file
- mirrord session - operator fingerprint F1, operator subscription id S1 (should request user cert)
- mirrord session - operator fingerprint F1, operator subscription id S1 (should reuse user cert)
- mirrord session - operator fingerprint F2, operator subscription id S1 (should request user cert, but reuse private key)
- mirrord session - operator fingerprint F2, operator subscription id S1 (should reuse user cert)
Need a test for scenario:
- Folder empty, no credentials file
- mirrord session - operator fingerprint F1, operator subscription id S1 (should request user cert)
- mirrord session - operator fingerprint F1, operator subscription id S1 (should reuse user cert)
- mirrord session - operator fingerprint F2, operator subscription id S1 (should request user cert, with new private key)
- mirrord session - operator fingerprint F2, operator subscription id S1 (should reuse user cert)

Each simulated mirrord session should go through the whole flow, starting from loading the credentials file.

@meowjesty

meowjesty

Few questions, have not looked into the tests yet.

meowjesty · 2025-06-25T15:07:03Z

mirrord/auth/src/lib.rs

@@ -27,3 +27,5 @@ pub mod credentials;
 pub mod error;
 /// Public/Private key abstraction for serialization and deserialization
 pub mod key_pair;
+
+pub use cluster_api::AuthClient;


nit: both cluster_api and AuthClient are already pub right? so why re-export?

I'd hide cluster_api here n reexport AuthClient since is a pub trait, do you agree ?

Sorry, somehow missed this comment.

Sure.

meowjesty · 2025-06-25T15:10:51Z

mirrord/auth/src/credentials.rs


+    //use kube::{api::PostParams, Api, Client, Resource};


Can be removed.

meowjesty · 2025-06-25T15:40:44Z

mirrord/auth/src/cluster_api.rs

+        &self,
+        key_pair: &KeyPair,
+        common_name: &str,
+    ) -> impl Future<Output = Result<Certificate, CredentialStoreError>> + Send;


Don't we have async fn in traits?

Here based on what I've seen in intproxy the code seems to be aligned with the project, that part at least (though I personally have some reservations about async in traits).
I completely agree with the point about complexity. In this case, I'd say the complexity isn’t particularly high, but we don’t really have a way to measure it objectively, so… xD.
It does tend to hide complexity, though: one for all, the fact that two futures cannot share the same type to the compiler, can only belong to the same family(like closures).
Just sharing thoughts, of course :)

I think it's like that in the intproxy mostly due to it being a more unstable thing at the time, and we've been trying to avoid adding features so one day we can get back on stable rust (a bit of mirrord project background/backstory here).

About complexity, yeah it's not high, but the implementation we have wouldn't it look more like a normal function with async? Like the error handling could be done with just ? instead of match, and the async move part be dropped completely?

let request = certificate_request(common_name, key_pair)?; let certificate = request.encode_pem()?; let api: Api<R> = Api::all(self.clone()); Ok(api.create_subresource("certificate", "operator", &PostParams::default(), certificate_request.into()).await?)

I wouldn't say this is a true blocker, we have not talked about it yet, so if you think impl Future would be better, then maybe we can bring it up in the next sync and come to an agreement with the team?

This is the correct syntax. You could write

trait Whatever { async fn something(); }

But then you can implement this trait it in a way that makes the future not Send, and clippy complains (because the trait is mostly unusable in async code).
While when you use the fn name() -> impl Future<...> + ... syntax, you can add trait bounds on the returned future.

Can't we use https://crates.io/crates/async-trait for this then?

meowjesty · 2025-06-25T15:41:57Z

mirrord/auth/src/cluster_api.rs

+    R: for<'de> Deserialize<'de> + Resource + Clone + Debug,
+    <R as Resource>::DynamicType: Default,
+{
+    fn obtain_certificate(


I think async would simplify this impl, or am I misunderstanding something here?

meowjesty · 2025-06-25T15:47:46Z

mirrord/auth/src/cluster_api.rs

+
+use crate::{certificate::Certificate, error::CredentialStoreError, key_pair::KeyPair};
+
+pub trait AuthClient<R> {


This trait is part of our default-features, while the Client impl is part of the client feature, right? If I'm understanding this correctly, then I think AuthClient should be moved up (to some upper mod, which I guess it's lib.rs), and we could feature gate this whole mod with client?

I don't want to complicate things though (not a big fan of having features like this), so do we care @Razz4780 ?

If it's used only by code hidden behind the client feature, it should also be behind this feature

…-and-add-more-complex

BigLullu added 5 commits June 25, 2025 05:13

Refactor init and refresh methods to use new AuthClient trait f…

63610f5

…or certificate retrieval; replace in-method logic with reusable implementation in `cluster_api.rs`.

fmt

e560490

Add ClientMock for testing AuthClient trait and unit tests for `g…

d8ddcee

…et_or_init` in `CredentialStore`

Add towncrier file

f1ebc84

Remove redundant reference to key_pair in certificate_request

76c1e3f

Razz4780 requested a review from meowjesty June 25, 2025 12:41

meowjesty requested changes Jun 25, 2025

View reviewed changes

BigLullu added 5 commits July 1, 2025 04:17

Merge branch 'main' into mbe-776-decouple-credentialstore-from-kubers…

cb5b977

…-and-add-more-complex

various little fix plus test

b143e2b

Merge branch 'main' into mbe-776-decouple-credentialstore-from-kubers…

25a22f7

…-and-add-more-complex

Merge branch 'main' into mbe-776-decouple-credentialstore-from-kubers…

1c0d407

…-and-add-more-complex

Merge branch 'main' into mbe-776-decouple-credentialstore-from-kubers…

c1b4f0c

…-and-add-more-complex


		use crate::{certificate::Certificate, error::CredentialStoreError, key_pair::KeyPair};

		pub trait AuthClient<R> {

Mbe 776 decouple credentialstore from kubers and add more complex #3388

Are you sure you want to change the base?

Mbe 776 decouple credentialstore from kubers and add more complex #3388

Uh oh!

Conversation

BigLullu commented Jun 25, 2025

Uh oh!

Razz4780 commented Jun 25, 2025

Uh oh!

meowjesty left a comment

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!

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!

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!