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.

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

Conversation

tot0
Copy link

@tot0 tot0 commented Mar 5, 2025

This matches the interface of VirtualMachineManagedIdentityCredential, it enables specifying a client_id to use for Identities being served not via the VM local Imds endpoint.

Copy link
Member

@heaths heaths left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution, but I don't think we can expose much of this publicly and probably already have too much public for the next beta release where we'll better align with other languages' versions of azure_identity. That's not to say we can't have some idiomatic rust-specifics, but it seems the AppServiceManagedIdentityCredential shouldn't even be public.

For that alone I'd still merge this, but then the whole reason for exposing the IMDS type evaporates. We also won't expose the cache. It's not like people can't write their own if needed. We don't want to expose types unless necessary or highly requested becuase we'll have to support them as-is in perpetuity. Instead, we will probably have a more robust cache that can be securely persisted a la https://github.com/Azure/azure-sdk-for-go/blob/main/sdk/azidentity/cache/cache.go. So we wouldn't expose this simple in-memory cache as-is.

@@ -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?

@@ -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.

@tot0
Copy link
Author

tot0 commented Mar 5, 2025

Thanks for the contribution, but I don't think we can expose much of this publicly and probably already have too much public for the next beta release where we'll better align with other languages' versions of azure_identity. That's not to say we can't have some idiomatic rust-specifics, but it seems the AppServiceManagedIdentityCredential shouldn't even be public.

For that alone I'd still merge this, but then the whole reason for exposing the IMDS type evaporates. We also won't expose the cache. It's not like people can't write their own if needed. We don't want to expose types unless necessary or highly requested becuase we'll have to support them as-is in perpetuity. Instead, we will probably have a more robust cache that can be securely persisted a la https://github.com/Azure/azure-sdk-for-go/blob/main/sdk/azidentity/cache/cache.go. So we wouldn't expose this simple in-memory cache as-is.

Understandable on both fronts.
If I understand correctly, if I remove the TokenCache visibility changes you would accept the AppService changes? The breaking isn't Ideal, but I just want to reuse the IDENTITY_ENDPOINT and Secret logic, while being able to specify a ClientId to pass.

@heaths
Copy link
Member

heaths commented Mar 5, 2025

Possibly. We're discussing in the Identity v-team. But I can say you won't be able to use these changes for long, and even then would have to use a patched git reference to azure_identity. By the time the next beta releases, azure_identity won't expose all these other MSI credential types - only ManagedIdentityCredential.

@tot0
Copy link
Author

tot0 commented Mar 5, 2025

Possibly. We're discussing in the Identity v-team. But I can say you won't be able to use these changes for long, and even then would have to use a patched git reference to azure_identity. By the time the next beta releases, azure_identity won't expose all these other MSI credential types - only ManagedIdentityCredential.

Happy to not actually PR this then. For now I am referencing my fork for usage of azure_core, azure_identity and azure_messaging_eventhub. I can keep it that way for a month or so if there's a refactor coming soob.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Azure.Identity The azure_identity crate
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

2 participants