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

fix git http_proxy ignored #5613

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

fix git http_proxy ignored #5613

wants to merge 1 commit into from

Conversation

WnP
Copy link

@WnP WnP commented Dec 27, 2024

fix #5329

Git commands are executed using os/exec package and therefore do not share environment variables with the main process, That's why ADD doesn't honor proxy related environment variables in this context.

That said, in a pure HTTP context, like:

ADD https://example.com/archive.zip /usr/src/things/

ADD already respects proxy-related environment variables because it uses net/http package internally.

Signed-off-by: Steeve Chailloux <[email protected]>
Copy link
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

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

I guess this is ok for matching HTTP although I'm not sure if HTTP behavior is intentional. I think it would make more sense if these were to be passed from the client (as they are with RUN statements via build-arg).

Maybe we can merge this but then follow up with:

  • update HTTP and Git to both use the proxy settings configured from client
  • deprecate passthrough of proxy settings from daemon process and remove a couple of releases after.

@thaJeztah
Copy link
Member

The classic builder used the proxies configured in the daemon environment, not those passed from the CLI; #5329 (comment)

Which would match behavior for (say) ADD <image> which I assume would also use the daemon's proxy settings?

@tonistiigi
Copy link
Member

FROM image I think can be considered internal behavior (mirror, tls etc. config is also on the daemon side). I don't understand why RUN curl <URL> and ADD <URL> would use a different method for proxy configuration though. There is no guarantee of what that implementation is (eg. there is a completely different Git implementation in #1048 ).

@thaJeztah
Copy link
Member

Yeah, there's a wide range of possible combinations possible.

My concern here is that there's situations where the daemon runs in a MITM proxy environment (we're currently looking into some of those situations). In such cases, the proxy should be mostly transparent to the user for operations performed by Docker (and BuildKit) itself, but may not be desired automatically for tools running inside containers (including build containers).

In such cases using the proxy from the host would work (daemon configured to use a that proxy, and custom root-CA's present on the host), but we (currently at least) have no good options to specify those options from the client; first of all, because the client may not have access to relevant root-CAs, but also because there's no options to send those over or otherwise configure which ones to use (and where / how they must be installed).

Perhaps we need more options for this;

  • Default (could be configurable) to inherit proxy settings from the host / daemon
  • And/or an option for --proxy=inherit (or something) to explicitly use the proxies configured for the daemon
  • TBD whether such options should also be configurable as a default for RUN, as it's not always desirable for software running inside the container to have networking access through those proxies.

@thaJeztah
Copy link
Member

cc @neersighted @thompson-shaun

@@ -189,6 +189,12 @@ func (cli *GitCLI) Run(ctx context.Context, args ...string) (_ []byte, err error
"GIT_CONFIG_NOSYSTEM=1", // Disable reading from system gitconfig.
"HOME=/dev/null", // Disable reading from user gitconfig.
"LC_ALL=C", // Ensure consistent output.
"HTTP_PROXY=" + os.Getenv("HTTP_PROXY"),
Copy link
Member

Choose a reason for hiding this comment

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

Afaics this should define new variables with empty string values if the env value is not defined. os.LookupEnv handles the case of detecting if env is defined or not.

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

Successfully merging this pull request may close these issues.

http_proxy, https_proxy not honored by Dockerfile ADD instruction?
3 participants