Add SessionCatalog trait - #2836
Conversation
|
I literally just proposed this idea last week in the rust community sync, and very happy to see some actual work already! There are some existing work on AuthManager/Session, maybe we should merge the effort? #2815 |
|
@CTTY What a coincidence! I was planning to attend myself to push for it, so thanks for covering the topic! 🙏 On #2815, this is perfect timing too. I was about to start some work on the AuthManager (according to #2774 (comment)) but that doesn't seem necessary anymore 👏 They're 100% related, but it already looks like a reasonable split to contribute them sequentially. |
6507c6c to
7e6df7b
Compare
| /// Note that the session_id may be used for caching session-scoped state | ||
| /// and re-use of a session_id with different session context may result in | ||
| /// unexpected behavior. | ||
| session_id: String, |
There was a problem hiding this comment.
nit: since reusing a session_id across different contexts is documented as hazardous, maybe default it in the builder (#[builder(default = Uuid::new_v4().to_string())]) so the safe thing is also the default?
There was a problem hiding this comment.
This is a great idea! Will do 🙇
| /// | ||
| /// It is redacted from logs and automatically zeroized. | ||
| #[derive(Clone)] | ||
| pub struct Credential(Zeroizing<String>); |
There was a problem hiding this comment.
Yes, this is dual use! Sounds good to me. If you have a preference on the naming, SensitiveString is definitely the more generally plausible name. Credential works better in the current state where we're only dealing with credentials.
I'll take a look through the Java codebase to see whether there's other places where sensitive strings could be handled.
CTTY
left a comment
CTTY
left a comment
There was a problem hiding this comment.
LGTM in general! Just one question
| } | ||
|
|
||
| /// Common interface for all session catalog builders. | ||
| pub trait SessionCatalogBuilder: Default + Debug + Send + Sync { |
There was a problem hiding this comment.
Is the trait actually needed?
We currently have trait CatalogBuilder because we want users to be able to load catalogs using a type string. But for session catalog I don't see such use case for now
There was a problem hiding this comment.
Oh maybe not. I was mostly following the Catalog example. Let me remove it then, we can always add it back 👍
Signed-off-by: Jannik Steinmann <jannik.steinmann@datadoghq.com> Re-generate public API Signed-off-by: Jannik Steinmann <jannik.steinmann@datadoghq.com>
3 participants
Which issue does this PR close?
What changes are included in this PR?
This PR introduces the session-aware catalog API:
SessionCatalogandSessionCatalogBuilderSessionContext, including identity, properties, and credentialsCredential, which redacts secrets from debug output and zeroizes themThe REST implementation and catalog refactor have been split into a follow-up PR.
Are these changes tested?
Added some unit tests, but this PR adds very little logic, only new interfaces which the follow-up PR is validating.