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

blob/azureblob: Adds CLI auth #3160

Closed
wants to merge 1 commit into from

Conversation

ekini
Copy link
Contributor

@ekini ekini commented Aug 11, 2022

Contrary to the closed #3012 the cli auth didn't work for me so I've taken a look a made a working version.

However! Here https://github.com/google/go-cloud/blob/master/blob/azureblob/azureblob.go#L42 it says it should fall back to NewDefaultAzureCredentials but I don't see it happening in the code below.

But then again after looking at https://github.com/Azure/azure-sdk-for-go/blob/sdk/azidentity/v1.1.0/sdk/azidentity/default_azure_credential.go#L45-L93 it seems it does something similar as code here https://github.com/google/go-cloud/blob/master/blob/azureblob/azureblob.go#L320-L331 so maybe we should just use NewDefaultAzureCredentials straight away?

Hence, I'm creating this PR as a draft to discuss the approach first.

@codecov
Copy link

codecov bot commented Aug 11, 2022

Codecov Report

Merging #3160 (bdf004a) into master (bb5165b) will decrease coverage by 0.06%.
The diff coverage is 10.00%.

@@            Coverage Diff             @@
##           master    #3160      +/-   ##
==========================================
- Coverage   72.79%   72.73%   -0.07%     
==========================================
  Files         113      113              
  Lines       14385    14393       +8     
==========================================
- Hits        10472    10469       -3     
- Misses       3187     3197      +10     
- Partials      726      727       +1     
Impacted Files Coverage Δ
blob/azureblob/azureblob.go 82.54% <10.00%> (-1.52%) ⬇️
pubsub/rabbitpubsub/rabbit.go 80.56% <0.00%> (-0.63%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@vangent
Copy link
Contributor

vangent commented Aug 11, 2022

Thanks for trying it out and your suggestions!

You are right, the comment says it defaults to NewDefaultAzureCredential while the code uses NewEnvironmentCredential. The former seems better because it tries the latter and then also tries other things.

So, can you update the PR to rename credTypeIdentityFromEnv to credTypeIdentityDefault and have it call NewDefaultAzureCredential? Then see if that works for CLI credentials; based on https://github.com/Azure/azure-sdk-for-go/blob/sdk/azidentity/v1.1.0/sdk/azidentity/default_azure_credential.go#L74 it seems like it should.

I'm less confident that NewDefaultAzureCredential handles the shared key, connection string, and SAS token approaches in the same way as azureblob does, and so I think it makes sense to detect and handle them explicitly the way it's done now, for backwards compatibility if nothing else.

@vangent
Copy link
Contributor

vangent commented Aug 11, 2022

I'm doing this in #3161, PTAL and let me know if it doesn't work.

@vangent vangent closed this Aug 11, 2022
@ekini
Copy link
Contributor Author

ekini commented Aug 11, 2022

Thank you, that worked!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

blob/azureblob: add a way to use CLI auth
2 participants