add azure_identity::create_credential(), SpecificAzureCredential, AppServiceManagedIdentityCredential, VirtualMachineManagedIdentityCredential #1532
Conversation
…viceManagedIdentityCredential, VirtualMachineManagedIdentityCredential
sdk/identity/src/token_credentials/client_certificate_credentials.rs
Outdated
Show resolved
Hide resolved
| ClientCertificateCredential { | ||
| tenant_id, | ||
| client_id, | ||
| client_certificate: client_certificate.into(), | ||
| client_certificate_pass: client_certificate_pass.into(), | ||
| http_client: new_http_client(), | ||
| options, | ||
| http_client: options.options().http_client().clone(), |
There was a problem hiding this comment.
options.options feels awkward.
There was a problem hiding this comment.
Yes, a little bit awkward. It still felt better than copying all the members.
| pub fn new( | ||
| options: impl Into<TokenCredentialOptions>, | ||
| endpoint: Url, | ||
| api_version: &str, | ||
| secret_header: HeaderName, | ||
| secret_env: &str, | ||
| id: ImdsId, |
There was a problem hiding this comment.
Can you discuss the intent for ImdsManagedIdentityCredential in the future?
If this is intended to be used by consumers of the create, I would expect users to not have to know these things, but it's reasonable enable users to change them as appropriate.
If this is intended to be the implementation wrappers like the app service managed identity & virtual machine managed identity, can the visibility be changed to only internal to the crate?
There was a problem hiding this comment.
Looking at both the golang and the dotnet implementations, both appear to allow the user to create a Managed Identity client directly and based on trying to initialize each of them in term, picks the underlying implementation that is appropriate.
In dotnet, this is performed here:
In golang, this is performed here:
https://github.com/Azure/azure-sdk-for-go/blob/2d7b90ad1183eb96d60458a8cef2afeec66f7c29/sdk/azidentity/managed_identity_client.go#L124-L151
To mirror the other SDKs, we will want a Managed Identity Credential that figures out the underlying implementation at initialization. The user should not need to know the endpoint, api version, environment variables, etc.
There was a problem hiding this comment.
Yes, we could add a ManagedIdentityCredential that contains all of the managed identities, but it is essentially just a subset of DefaultAzureCredential. I'm not sure how useful it is. Same with EnvironmentCredential now. Still, probably good to check off the compatibility box.
I expect ImdsManagedIdentityCredential to primarily be for internal use. Feature like AzureArcManagedIdentity #1503 will build off of it. I left it public, primarily for others to be able to build off of it for features we have not implemented.
There was a problem hiding this comment.
Adding ManagedIdentityCredential would be useful, as it aligns with the different SDK implementations.
There was a problem hiding this comment.
I agree and I will add it after this merges. I created #1536 to track.
There was a problem hiding this comment.
👆 Relevant for #1659 @johnbatty.
cataggar
changed the title
Co-authored-by: Heath Stewart <[email protected]>
Co-authored-by: Heath Stewart <[email protected]>
|
Testing on a Azure Virtual Machine is complete. I created a VM, az vm create -g $vm_name -n $vm_name --image $image --admin-username $env.USER --ssh-key-values $'($env.HOME)/.ssh/id_rsa.pub'I used the portal to add the identity, but it can be done via the CLI too: |
|
As we just discussed, I created #1581 as a placeholder for updating the README. This PR is ready to merge. |
heaths
force-pushed
the
SpecificAzureCredential2
branch
from
bd1104e to
71fac71
Compare
|
Opened a couple tracking issues for subsequent discussion / work, but nothing to hold up this PR. |
| /// | ||
| /// These sources will be tried in the order provided in the `TokenCredential` authentication flow. | ||
| pub fn with_sources(sources: Vec<DefaultAzureCredentialEnum>) -> Self { | ||
| fn with_sources(sources: Vec<DefaultAzureCredentialKind>) -> Self { |
There was a problem hiding this comment.
I removed pub here. It could be made public, but I'd like to know the use case.
Happy new year 2024!
This adds
azure_identity::new_credential(),SpecificAzureCredential,AppServiceManagedIdentityCredential, andVirtualMachineManagedIdentityCredential.azure_core::new_http_client(),azure_identity::create_credential()is a function that simplifies the most common use cases.SpecificAzureCredentialfixes add SpecificAzureCredential #1503. It provides an easy and fast way to specify a specific credential type at runtime using anAZURE_CREDENTIAL_KINDenvironment variable.AppServiceManagedIdentityCredentialandVirtualMachineManagedIdentityCredentialare split off of and make use ofImdsManagedIdentityCredential. This fixes split up ImdsManagedIdentityCredential #1495.Most all of the credentials have something similar to
create(options: impl into<TokenCredentialOptions>) -> Result<Self>now. This is similar to how C# hasTryCreateshown in #1495. They are able to check environment variables or files to see if they can be created, something milliseconds fast. The main exception isVirtualMachineManagedIdentityCredential, which does not have anything quick to check, so it is disabled by default. This is a breaking behavioral change that allows the default credentials to be faster by default. You can still opt in to using them by settingAZURE_CREDENTIAL_KINDtovirtualmachine. You can also include them using theDefaultAzureCredentialBuilder.Manual functional tests of the
AppServiceManagedIdentityCredentialandVirtualMachineManagedIdentityCredentialwas completed.