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

Rewrite section on configuring proxies for containers #13786

Merged
merged 1 commit into from
May 5, 2023

Conversation

thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Nov 4, 2021

Proposed changes

Unreleased project version (optional)

Related issues (optional)

@thaJeztah
Copy link
Member Author

Just pushing this as a draft; this is very unfinished (need to rewrite the last section and also mention upper/lowercase etc)

@netlify
Copy link

netlify bot commented Nov 4, 2021

✔️ Deploy Preview for docsdocker ready!

🔨 Explore the source changes: c07a1dc

🔍 Inspect the deploy log: https://app.netlify.com/sites/docsdocker/deploys/6183bb41ca9524000751e914

😎 Browse the preview: https://deploy-preview-13786--docsdocker.netlify.app

network/proxy.md Outdated
"httpProxy": "http://192.168.1.12:3128",
"httpsProxy": "http://192.168.1.12:3128",
"noProxy": "*.test.example.com,.example2.com,127.0.0.0/8"
"httpsProxy": "https://192.168.1.12:3129",
Copy link
Member

Choose a reason for hiding this comment

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

I think it is supposed to be http here, quoting @djs55:

Confusingly a HTTPS proxy is a HTTP server which accepts CONNECT host:port HTTP requests and establishes a tunnel, which the client then sends TLS back and forth but the proxy doesn't understand (in some ways this is easier than in the HTTP case where the proxy has a bunch of rules about what to do with headers)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think both should work, although it could depend on the implementation.

It's really a "fuzzy" area, without any (good) standards. e.g. Golang also made a silly (and breaking) change and started to ignore HTTP_PROXY for https connections; https://docs.docker.com/engine/release-notes/#201010

Copy link
Contributor

Choose a reason for hiding this comment

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

I think

  • the httpProxy (HTTP_PROXY) will be used to proxy http:// connections
  • the httpsProxy (HTTPS_PROXY) will be used to proxy https:// connections

where the connection to the proxy can either be plaintext http://proxy:3128 or itself using TLS https://proxy:3128. So there can be 0, 1 or 2 layers of TLS.

If the underlying connection is TLS, then the only reason to use TLS to talk to the proxy too is to protect the authorization header which could include a base64-encoded password. The destination server is sent in the clear even with TLS (as a Server Name Indication) so the HTTP CONNECT <server:443> part is not secret.

So I think the examples are ok. My main problem with the docs as they are is that there are a confusing number of places where proxies can be configured. I think this PR makes an improvement though!

| Variable | Dockerfile example | `docker run` example |
|:--------------|:--------------------------------------------------|:----------------------------------------------------|
| `HTTP_PROXY` | `ENV HTTP_PROXY="http://192.168.1.12:3128"` | `--env HTTP_PROXY="http://192.168.1.12:3128"` |
| `HTTPS_PROXY` | `ENV HTTPS_PROXY="https://192.168.1.12:3128"` | `--env HTTPS_PROXY="https://192.168.1.12:3128"` |
Copy link
Member

Choose a reason for hiding this comment

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

Ah that one had https though… 🤔

@thaJeztah
Copy link
Member Author

I need to collect more changes and put them together, e.g. #8352 and #12463 (which also isn't correct, as "it depends")

And describe that handling of NO_PROXY is quite different (depending on what software uses it), same as for upper/lowercase cURL, wget, and Golang don't agree on those.

@craig-osterhout craig-osterhout added the area/configuration Relates to configuring containers label Aug 4, 2022
@usha-mandya
Copy link
Member

@thaJeztah Is this PR still relevant? Thanks.

@docker-robott
Copy link
Collaborator

Thanks for the pull request. We'd like to make our product docs better, but haven’t been able to review all the suggestions.
As our docs have also diverged, we do not have the bandwidth to review and rebase old pull requests.

If the updates are still relevant, review our contribution guidelines and rebase your pull request against the latest version of the docs, then mark it as fresh with a /remove-lifecycle stale comment.
If not, this pull request will be closed in 30 days. This helps our maintainers focus on the active pull requests.

Prevent pull requests from auto-closing with a /lifecycle frozen comment.

/lifecycle stale

@thaJeztah
Copy link
Member Author

Reopening for now (still in draft), but still something to finish.

@netlify
Copy link

netlify bot commented Apr 13, 2023

Deploy Preview for docsdocker ready!

Name Link
🔨 Latest commit 073cd2f
🔍 Latest deploy log https://app.netlify.com/sites/docsdocker/deploys/6452a1be955eb400084b5bab
😎 Deploy Preview https://deploy-preview-13786--docsdocker.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

config/daemon/systemd.md Outdated Show resolved Hide resolved
@dvdksn dvdksn self-assigned this Apr 13, 2023
@dvdksn dvdksn force-pushed the rewrite_proxy_section branch 3 times, most recently from 7e81ad5 to d1d76c7 Compare April 25, 2023 14:26
@dvdksn dvdksn marked this pull request as ready for review April 27, 2023 13:43
@dvdksn dvdksn self-requested a review as a code owner April 27, 2023 13:43
@dvdksn
Copy link
Contributor

dvdksn commented Apr 27, 2023

@thaJeztah I continued on some of the changes you started in this PR. PTAL and let me know if there's more stuff that we should add/change in the scope of this

@dvdksn dvdksn changed the title WIP rewrite section on configuring proxies for containers Rewrite section on configuring proxies for containers May 2, 2023
@thaJeztah
Copy link
Member Author

Oh! Thanks, sorry forgot about this one; gonna give this one a look Today.

I see there's one link failure; perhaps we need a custom id / anchor somewhere;

========================================================================
engine/reference/commandline/cli/index.html
  hash does not exist --- engine/reference/commandline/cli/index.html --> /config/daemon/systemd/#httphttps-proxy
engine/reference/commandline/dockerd/index.html
  hash does not exist --- engine/reference/commandline/dockerd/index.html --> /config/daemon/systemd/#httphttps-proxy
========================================================================

@dvdksn
Copy link
Contributor

dvdksn commented May 3, 2023

Fixed the broken link upstream in docker/cli#4249 - will need to backport that one

Copy link
Member Author

@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.

Might need to give this another pass later, but let me post what I had written down, before I forget to submit 😂

config/daemon/systemd.md Outdated Show resolved Hide resolved
config/daemon/systemd.md Outdated Show resolved Hide resolved
config/daemon/systemd.md Outdated Show resolved Hide resolved
config/daemon/systemd.md Outdated Show resolved Hide resolved
engine/release-notes/20.10.md Outdated Show resolved Hide resolved
network/proxy.md Outdated
```dockerfile
FROM alpine
ARG HTTPS_PROXY
RUN apk update # uses httpsProxy from ~/.docker/config.json
Copy link
Member Author

Choose a reason for hiding this comment

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

It might be worth also showing an example somewhere where;

  • the proxy is set manually during build (--build-arg)
  • the proxy is set manually during "run" (--env)

perhaps also an example where the proxy is taken from the current environment (this could be a follow-up). Use-case here would be that the host is configured with a proxy; by default, (build-)containers do not inherit those environment variables (by design), but it may be that you need them to have the same proxy configured.

# proxy configuration on my host
env | grep _PROXY
HTTPS_PROXY=https://my-hosts-proxy.example
HTTP_PROXY=http://my-hosts-proxy.example

# run a container that inherits the values from those by setting the name of the env-var, but no value (no `=`);
docker run -e HTTPS_PROXY -e HTTP_PROXY --rm alpine sh -c 'env | grep _PROXY'
HTTPS_PROXY=https://my-hosts-proxy.example
HTTP_PROXY=http://my-hosts-proxy.example

docker build --no-cache --progress=plain --build-arg HTTPS_PROXY --build-arg HTTP_PROXY -<<'EOF'
FROM alpine
RUN env | grep -i _PROXY
EOF

#5 [2/2] RUN env | grep -i _PROXY
#0 0.106 HTTPS_PROXY=https://my-proxy.example
#0 0.106 https_proxy=https://my-proxy.example
#0 0.106 http_proxy=http://my-proxy.example
#0 0.106 HTTP_PROXY=http://my-proxy.example
#5 DONE 0.1s

network/proxy.md Outdated
```json
{
"proxies": {
"https://docker-daemon1.example.com": {
Copy link
Member Author

Choose a reason for hiding this comment

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

I think this one needs to be tcp://docker-daemon1.example.com (but would have to verify that).

Do you think (for a follow-up?) it would be useful to show an example as well where both "default" and "other is configured?

cat ~/.docker/config.json
{
	"proxies": {
		"default": {
			"httpProxy": "http://192.168.1.12:1111"
		},
		"unix:///Users/thajeztah/.docker/run/docker.sock": {
			"httpProxy": "http://192.168.1.12:2222"
		}
	}
}

And we could show examples, including how it interacts with docker context;

docker context ls
NAME                TYPE                DESCRIPTION                               DOCKER ENDPOINT                                   KUBERNETES ENDPOINT   ORCHESTRATOR
default *           moby                Current DOCKER_HOST based configuration   unix:///var/run/docker.sock
desktop-linux       moby                                                          unix:///Users/thajeztah/.docker/run/docker.sock


# uses "default" (if set)
docker run --rm  alpine sh -c 'env | grep _PROXY'
HTTP_PROXY=http://192.168.1.12:1111

# uses proxy for "unix:///Users/thajeztah/.docker/run/docker.sock"
docker --host unix:///Users/thajeztah/.docker/run/docker.sock run --rm  alpine sh -c 'env | grep _PROXY'
HTTP_PROXY=http://192.168.1.12:2222

# also uses proxy for "unix:///Users/thajeztah/.docker/run/docker.sock"
docker --context=desktop-linux run --rm  alpine sh -c 'env | grep _PROXY'
HTTP_PROXY=http://192.168.1.12:2222

(and, yes, we should integrate that with docker context itself, and have it be part of the context's options)

☝️ I should mention that it looks like there's currently a bug in the CLI, and setting a "per-host" proxy for a ssh:// connection doesn't work (it tries to look up using our "magic" http://docker.example.com hostname, which is used internally.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a cool example. Let's add it in a follow-up - for now I can try to clarify the confusion with the key names, and maybe add both a default and a "named" daemon.

TBH, I find contexts a confusing topic overall. Maybe that's an area worth spending some time trying to clarify.

network/proxy.md Outdated Show resolved Hide resolved
network/proxy.md Outdated Show resolved Hide resolved
network/proxy.md Outdated Show resolved Hide resolved
Signed-off-by: David Karlsson <david.karlsson@docker.com>

Co-authored-by: Sebastiaan van Stijn <github@gone.nl>
Copy link
Member Author

@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.

LGTM ❤️

(I can't approve the PR because I opened it, but if you can LGTM it @dvdksn, then we can merge)

@dvdksn dvdksn merged commit 8c8d304 into docker:main May 5, 2023
@thaJeztah thaJeztah deleted the rewrite_proxy_section branch May 5, 2023 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/configuration Relates to configuring containers lifecycle/frozen
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants