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

add system proxy config support for cli requests #1487

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tonistiigi
Copy link
Member

This adds support for system HTTP_PROXY config for the requests that are made directly by the CLI. When previously user needed to call HTTPS_PROXY=x docker buildx build then now they can define these variables in ~/.docker/buildx/proxy.json where they are loaded automatically.

Note that this is different from the proxy config in the Docker CLI config that buildx also loads. That config is per host and forwarded to the VM side, so it can be a completely different configuration.

@djs55 @thaJeztah

Signed-off-by: Tonis Tiigi tonistiigi@gmail.com

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

WHYY???

So now;

  • docker buildx build will use ~/,docker/buidx/proxy.json
  • docker build will use HTTPS_PROXY or HTTP_PROXY
  • docker compose build will use????
  • docker manifest inspect will use ???
  • docker scan will use ???

Great. Unified experience.

@tonistiigi
Copy link
Member Author

docker build will use HTTPS_PROXY or HTTP_PROXY

No, docker build forwards to buildx. Also, this doesn't change anything about HTTPS_PROXY or HTTP_PROXY envs.

docker compose build will use????

Compose build should also forward to buildx (proxy matters only if they do requests).

docker manifest inspect will use ???

Should forward to imagetools inspect (or remove)

docker scan will use ???

That is for docker scan authors to decide.

WHYY???

So that people who have this issue atm have a solution.

@thaJeztah
Copy link
Member

So that people who have this issue atm have a solution.

What is the issue?

My issue is that buildx should not have this config; if we do want proxy settings for the CLIs to be configurable in a configuration file, it should not be a buildx config, but a CLI configuration.

@tonistiigi
Copy link
Member Author

What is the issue?

The system proxy can be configured for dockerd and Docker Desktop. People who rely on this(and for example can't dial to hosts without the proxy settings) with buildx hit an issue with their builds because buildkit does not send the registry credentials as plaintext to the daemon for security reasons, but client will ask for a short-lived token directly instead. To make this work user needs to always manually set HTTP_PROXY when calling buildx, or add it to .bashrc but then when it changes it can easily break. This allows that when such user modifies the proxy settings in desktop settings, the builds do not break. There are some other HTTP requests that buildx can do in advanced cases, but these are much less important.

if we do want proxy settings for the CLIs to be configurable in a configuration file, it should not be a buildx config, but a CLI configuration.

If Docker CLI ever adds a similar feature defined in some other config file I see no issue with buildx also looking up that config.

@tonistiigi
Copy link
Member Author

We still don't have any solution for this.

@tonistiigi tonistiigi added this to the v0.11.0 milestone May 8, 2023
@jedevc jedevc added the kind/enhancement New feature or request label May 18, 2023
@jedevc
Copy link
Collaborator

jedevc commented Jun 8, 2023

@thaJeztah we're not blocked on this, and can do the release without it - but this in the milestone. Is there anything we should do to move this forward, or should we try and do this upstream in cli?

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

Successfully merging this pull request may close these issues.

5 participants