Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support custom ImdsId for AppServiceManagedIdentityCredential. #2284

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Support custom ImdsId for AppServiceManagedIdentityCredential. #2284

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
93d9fc8
Allow specifying custom ImdsIds for AppServiceCredential.
tot0 Mar 5, 2025
c646422
Expose TokenCache for external use as it's nice to have when implemen…
tot0 Mar 5, 2025
30d9258
Revert "Expose TokenCache for external use as it's nice to have when …
tot0 Mar 5, 2025
b354206
Address PR comments.
tot0 Mar 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 11 additions & 0 deletions sdk/identity/azure_identity/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,16 @@
# Release History

## 0.23.0 (?)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
## 0.23.0 (?)
## 0.23.0 (Unreleased)

@hallipr can we get the bot hooked up to make these changes automatically soon?


### Features Added

- Added `ImdsId` param to `AppServiceManagedIdentityCredential::new`
- Expose `azure_identity::credentials::TokenCache` for external `TokenCredential` implementations to leverage

### Breaking Changes

- Added `ImdsId` param to `AppServiceManagedIdentityCredential::new`

## 0.22.0 (2025-02-18)

### Features Added
Expand Down
2 changes: 1 addition & 1 deletion sdk/identity/azure_identity/examples/specific_credential.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ impl SpecificAzureCredential {
// case insensitive and allow spaces
match credential_type.replace(' ', "").to_lowercase().as_str() {
azure_credential_kinds::APP_SERVICE => {
AppServiceManagedIdentityCredential::new(options)
AppServiceManagedIdentityCredential::new(ImdsId::SystemAssigned, options)
.map(SpecificAzureCredentialKind::AppService)
.with_context(ErrorKind::Credential, || {
format!(
Expand Down
7 changes: 5 additions & 2 deletions sdk/identity/azure_identity/src/credentials/app_service_managed_identity_credential.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,10 @@ pub struct AppServiceManagedIdentityCredential {
}

impl AppServiceManagedIdentityCredential {
pub fn new(options: impl Into<TokenCredentialOptions>) -> azure_core::Result<Arc<Self>> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chlowell @scottaddie should this type even be exposed publicly? I'm close to refactoring azure_identity and I remember talking about how only ManagedIdentityCredential should be exposed and all the "implementation-specific" credentials should be internal. That would significantly affect the need for this PR and force consideration for how else to pass the IMDS ID. Comparing with Go's azidentity, I see it's just an implementation detail. I'm guessing this equates to ManagedIdentityCredentialOptions.ManagedIDKind? I'm not sure, though, since the only implementations I see are all user-assigned variants.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A unified ManagedIdentityCredential credential makes sense to me. When to call the IMDS static IP endpiont and when to call a environment specific endpoint I guess switches on the presence of IDENTITY_ENDPOINT in env?

For any Managed Identity credential it is important to be somehow able to specify whether to use a specific ClientId or just use the endpoints default.

pub fn new(
id: ImdsId,
options: impl Into<TokenCredentialOptions>,
) -> azure_core::Result<Arc<Self>> {
let options = options.into();
let env = options.env();
let endpoint = &env
Expand All @@ -43,7 +46,7 @@ impl AppServiceManagedIdentityCredential {
API_VERSION,
SECRET_HEADER,
SECRET_ENV,
ImdsId::SystemAssigned,
id,
),
}))
}
Expand Down
14 changes: 10 additions & 4 deletions sdk/identity/azure_identity/src/credentials/cache.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,20 +8,20 @@ use std::collections::HashMap;
use tracing::trace;

#[derive(Debug)]
pub(crate) struct TokenCache(RwLock<HashMap<Vec<String>, AccessToken>>);
pub struct TokenCache(RwLock<HashMap<Vec<String>, AccessToken>>);

impl TokenCache {
pub(crate) fn new() -> Self {
pub fn new() -> Self {
Self(RwLock::new(HashMap::new()))
}

pub(crate) async fn clear(&self) -> azure_core::Result<()> {
pub async fn clear(&self) -> azure_core::Result<()> {
let mut token_cache = self.0.write().await;
token_cache.clear();
Ok(())
}

pub(crate) async fn get_token(
pub async fn get_token(
&self,
scopes: &[&str],
callback: impl Future<Output = azure_core::Result<AccessToken>>,
Expand Down Expand Up @@ -61,6 +61,12 @@ impl TokenCache {
}
}

impl Default for TokenCache {
fn default() -> Self {
Self::new()
}
}

#[cfg(test)]
mod tests {
use super::*;
Expand Down
1 change: 1 addition & 0 deletions sdk/identity/azure_identity/src/credentials/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ mod workload_identity_credentials;
pub use app_service_managed_identity_credential::*;
#[cfg(not(target_arch = "wasm32"))]
pub use azure_cli_credentials::*;
pub use cache::*;
pub use client_assertion_credentials::*;
#[cfg(feature = "client_certificate")]
pub use client_certificate_credentials::*;
Expand Down