[remotecache] Add Azure Blob Storage support

[vangarp]

This adds experimental support for Azure Blob Storage based remote cache to buildkit. For usage instructions please refer to the updated Readme. We have tried to keep it similar to the S3 based implementation while using equivalent Azure Blob Storage specific semantics where appropriate.
This also adds end-to-end tests to exercise the Azure Blob Storage based cache using the Azurite emulator.

Co-authored-by: Amr Mahdi amrh@microsoft.com
Co-authored-by: Pranav Pandit pranavp@microsoft.com

[thaJeztah]

Had a quick peek at the new dependencies, and left some comments/questions.

fwiw; I'm not a maintainer on this repository (but always keeping a bit of an eye on the dependency tree as it may affect other projects as well that use BuildKit as a module)

[thaJeztah]

This module does a lot of reflection etc, but looks to be used only to provide a "pretty" verbose error; https://github.com/AzureAD/microsoft-authentication-library-for-go/blob/751ec246d696ada22aee7a7c3bfd7a097b248250/apps/errors/errors.go#L77

I'm not very familiar with this package, but it looks to be managed by a single-maintainer, and not very widely used (edit: ignore me; maybe it is; GitHub didn't show things correctly), and it lists github.com/google/go-cmp and github.com/davecgh/go-spew as possible alternatives (both of which are already used, but even those only for tests).

Happy to hear input on that from others, but given that the functionality of that package (from the looks of it) doesn't seem to be essential, I'm wondering if there would be ways to remove it (could be through a build-tag perhaps).

[vangarp]

The package definitely looks like it is widely used including several very widely used projects like prometheus-operator, istio etc.. The other packages mentioned (gp-cmp, go-spew) also don't look like they are maintained much which is understandable considering how light the functionality they provide is. I really don't see this as a problem unless someone has a strong opinion on why this particular package is bad enough to make an exception by allowing it to be skipped somehow

[thaJeztah]

A difference here is that most of those don't use this in production code, only in tests; e.g. https://cs.github.com/prometheus-operator/prometheus-operator?q=github.com%2Fkylelemons%2Fgodebug
https://cs.github.com/istio/istio?q=github.com%2Fkylelemons%2Fgodebug

Not a strong blocker for me, but let me "unresolved" the conversation to keep the converesation visible for future reference

[tonistiigi]

Separate the golint fixes. Squash the rest.

[vangarp]

@tonistiigi PTAL. I am still waiting for either #3022 or #3023 to complete to fix the panic during lint, but everything else should be good

[crazy-max]

Can you also rebase please?

[thaJeztah]

I want to do a quick test (draft PR) to see if the additional dependencies cause issues in combination with the (already complex) dependency tree in moby/moby when vendored. These "SDK" dependencies in particular can cause a lot of havoc (40k+ added lines of code definitely is a lot as well for this). Do we want build-tags to allow making specific drivers to be opt-in/opt-out?

I still have some concerns about #3010 (comment)

This dependency sounds a bit scary (opening a browser with an URL); looks like it can be called from https://github.com/Azure/azure-sdk-for-go/blob/48dd0f41119aa74cbcaa91fa4bac178c1fd09a70/sdk/azidentity/interactive_browser_credential.go#L76-L77

Do you know if that can ever be hit?

yeah that guy looks like it is only used for InteractiveBrowserCredential but we are not using it

And would like to have more eyes on that (also from a security perspective) just to be sure. While BuildKit may not (currently) use the functionality, I want to be sure that code-path cannot be hit. I'd like to be sure that there's no code that "heuristically" determines if interactive authentication is needed, and that might trigger an interactive login (open <scheme://> in default application). Such a code-path may be a backdoor waiting to happen; taking into account BuildKit may be running as root. My ideal would be for that dependency to not be included unless code actually requires it (which could be "interactive" being a separate module and/or gated by a build-tag).

Same applies, but to a lesser extend, to #3010 (comment). Not a blocker for me, but (as you work at Microsoft) if you have any influence on the github.com/AzureAD/microsoft-authentication-library-for-go package, I'd love to see that dependency disappear from non-test code, as it mostly looks like a "nice to have", not essential.

[thaJeztah]

I want to do a quick test (draft PR) to see if the additional dependencies cause issues in combination with the (already complex) dependency tree in moby/moby when vendored.

Looks like we're good from that perspective; the new dependencies don't appear to be affecting versions of the Azure / Microsoft dependencies in moby/moby; moby/moby#44031

[vangarp]

I want to do a quick test (draft PR) to see if the additional dependencies cause issues in combination with the (already complex) dependency tree in moby/moby when vendored. These "SDK" dependencies in particular can cause a lot of havoc (40k+ added lines of code definitely is a lot as well for this). Do we want build-tags to allow making specific drivers to be opt-in/opt-out?

I still have some concerns about #3010 (comment)

This dependency sounds a bit scary (opening a browser with an URL); looks like it can be called from https://github.com/Azure/azure-sdk-for-go/blob/48dd0f41119aa74cbcaa91fa4bac178c1fd09a70/sdk/azidentity/interactive_browser_credential.go#L76-L77
Do you know if that can ever be hit?

yeah that guy looks like it is only used for InteractiveBrowserCredential but we are not using it

And would like to have more eyes on that (also from a security perspective) just to be sure. While BuildKit may not (currently) use the functionality, I want to be sure that code-path cannot be hit. I'd like to be sure that there's no code that "heuristically" determines if interactive authentication is needed, and that might trigger an interactive login (open <scheme://> in default application). Such a code-path may be a backdoor waiting to happen; taking into account BuildKit may be running as root. My ideal would be for that dependency to not be included unless code actually requires it (which could be "interactive" being a separate module and/or gated by a build-tag).

Same applies, but to a lesser extend, to #3010 (comment). Not a blocker for me, but (as you work at Microsoft) if you have any influence on the github.com/AzureAD/microsoft-authentication-library-for-go package, I'd love to see that dependency disappear from non-test code, as it mostly looks like a "nice to have", not essential.

I know ideal would be to not have that dependency and I will create an issue to see if there is a possibility of that, but for now what I can do is go over the code to show why the browser path is not being hit in this case. If you look at the place where we are using the credentials then you can see that we use either the NewSharedKeyCredential or NewDefaultAzureCredential. Shared key credential just uses the account name and key directly. DefaultAzureCredential code is also fairly clear, in that it is meant to make it easy for servers to auth easily using 1 of the combinations of environment variables or logged in credentials as mentioned here. You can see in the code and also in the documentation that it does not use the InteractiveBrowserCredential anywhere in there. I hope this helps somewhat alleviate the doubts?

[thaJeztah]

I know ideal would be to not have that dependency and I will create an issue to see if there is a possibility of that

Thank you; appreciated!

I know ideal would be to not have that dependency and I will create an issue to see if there is a possibility of that, but for now what I can do is go over the code to show why the browser path is not being hit in this case.

Yes, I had a look at the code earlier, and indeed it won't be hit currently. My (perhaps too paranoid) look at having the dependency was that, with the pkg/browser dependency already in place, it would be less apparent if an (indirect) dependency would start to consume it; with BuildKit (and/or dockerd) running as root, that's a risk. We do use vendoring to help us to audit/review changes in dependencies, but of course things can be missed, so where possible I try to veer on the safe side.

I guess a "dirty" fix would be to replace it with a stubbed out version, but perhaps that's taking it too far for now.

So I'm fine with the current state if maintainers are ok with it, but would indeed like to see it gone in future (if possible).

[thaJeztah]

I guess a "dirty" fix would be to replace it with a stubbed out version, but perhaps that's taking it too far for now.

Just for fun; tried if that would work, and looks like it does. Quite a dirty trick though; https://github.com/thajeztah/buildkit/tree/nobrowser

thaJeztah@38ac4be

[vangarp]

I guess a "dirty" fix would be to replace it with a stubbed out version, but perhaps that's taking it too far for now.

Just for fun; tried if that would work, and looks like it does. Quite a dirty trick though; https://github.com/thajeztah/buildkit/tree/nobrowser

thaJeztah@38ac4be

haha it is an interesting way of addressing it indeed!

@crazy-max @tonistiigi what do you think? are you ok with the browser dependency remaining as it is given that the path which uses it is not being used (and I am going to create an issue to see if it can be removed somehow), or would you rather have the "trick" that @thaJeztah suggested?

[tonistiigi]

or would you rather have the "trick"

No, that 250 lines is not worth it.