google / go-cloud

The Go Cloud Development Kit (Go CDK): A library and tools for open cloud development in Go.
https://gocloud.dev/
Apache License 2.0
9.45k stars 799 forks source link

azblob: change in behaviour for falling back to `az login` credentials #3414

Closed tgummerer closed 2 months ago

tgummerer commented 2 months ago

Describe the bug

Sometime between v0.27 and v0.36 there seems to be a change in behaviour for what happens when two alternative login methods to azblob backends are set. In v0.27, when AZURE_KEYVAULT_AUTH_VIA_CLI=true is set, and the user is logged in to azure using the az login CLI, it seems like gocloud.dev falls back to using those credentials, even when AZURE_CLIENT_ID and AZURE_CLIENT_SECRET are set, but don't have permissions for the particular backend (or are plain wrong).

In v0.36 the login will simply fail if the AZURE_CLIENT_* environment variables do not allow the login.

To Reproduce

Run az login and set AZURE_KEYVAULT_AUTH_VIA_CLI=true. Also set AZURE_CLIENT_ID and AZURE_CLIENT_SECRET to something that doesn't have access to an azblob container. Then try to access the container. It will succeed with 0.27, but fail with 0.36

Expected behavior

The login should fall back to the credentials obtained from az login.

Version

0.36

Additional context

This came to my attention in the investigation in https://github.com/pulumi/pulumi/issues/15786#issuecomment-2051667094.

vangent commented 2 months ago

It's not obvious to me that the old/fallback behavior is better than the new one. It seems like an explicit failure is better...? Otherwise the fallback could lead to quite surprising results.

tgummerer commented 2 months ago

I'm a bit torn on the issue tbh. It's something that did work before, so the change in behaviour is surprising. E.g. in the linked issue the user is logged in, so accessing the azblob storage should work, even if they are using different credentials for setting up other resources. But I can see how the explicit failure is better in some cases.

vangent commented 2 months ago

Yea, I think this new behavior is more clear (although it wasn't on purpose). Given that nobody else has complained, the change in behavior doesn't appear to have been too disruptive, let's stick with it.