[CI:DOCS] Update documentation of CLI options affecting /etc/hosts
#24043
Conversation
|
One nit, overall LGTM |
openshift-ci
bot
added
the
approved
PhrozenByte
force-pushed
the
docs-add-host-gateway
branch
from
4462101 to
045f5c2
Compare
| @@ -2,10 +2,14 @@ | |||
| ####> podman build, create, farm build, pod create, run | |||
| ####> If file is edited, make sure the changes | |||
| ####> are applicable to all of those. | |||
| #### **--add-host**=*host:ip* | |||
| #### **--add-host**=*hostname[,hostname[,…]]*:*ip* | *hostname[,hostname[,…]]*:*host-gateway* | |||
There was a problem hiding this comment.
I don't think the host-gateway need to specified here, makes things more confusing IMO. If you think it is important it should properly be ...=*hostname[;hostname[;…]]*:*<ip>|host-gateway*
There was a problem hiding this comment.
I was thinking about a similar syntax, too. However, I was unsure and thus checked other Podman options and couldn't find any other Podman option using a simliar syntax (doesn't mean that I might have missed it though). Personally I'd write it as *hostname[;hostname[;…]]*:*ip*|"host-gateway" to indicate that ip is a param and "host-gateway" a fixed string (I also know <> to indicate required params, but that isn't used in Podman either?).
Keeping this consistent is more important though. The problem is that I'm not entirely sure what the most consistent solution is 😅 I'm fine with any variant, just tell me which variant to use, I'll add it then.
About "host-gateway": I feel like that it should be mentioned in the format line, because I believe that the format line should include all possibilities of the option and allow the user to fully understand the option without reading the description. But I'm fine with not mentioning it, too. Basically I'm fine with anything 😄 I've removed it for now.
There was a problem hiding this comment.
I am not to sure either, yes the syntax is not very consistent across all our man pages.
If you like to keep host-gateway in the format line that is fine for me.
Although I guess the description could also need some more explaining what host-gateway is, i.e. a ip address that can be used to connect from the container to the host.
There was a problem hiding this comment.
What do you think about the suggested *hostname[;hostname[;…]]*:*ip*|"host-gateway"? If that syntax is okay, I'd suggest using that.
Not sure how to further elaborate on host-gateway though. Right now we have this:
Instead of an IP address the special flag host-gateway can be given that resolves to an IP address the container can use to connect to the host. Podman will automatically add the
host.containers.internalandhost.docker.internalhostnames using the host-gateway address, unless the --no-hosts option is given.
We could add more details about how Podman chooses that IP address, but this fundamentally depends on the network mode used and even some other options as well, like if IPv4 is disabled (I assume that host-gateway resolves to an IPv6 address then?). I'm not entirely sure whether such details add much clarification.
I just checked Docker's docs on this, see https://docs.docker.com/reference/cli/docker/container/run/#add-host. They describe the option as follows:
The --add-host flag supports a special host-gateway value that resolves to the internal IP address of the host. This is useful when you want containers to connect to services running on the host machine.
It's conventional to use host.docker.internal as the hostname referring to host-gateway. Docker Desktop automatically resolves this hostname, see Explore networking features.
I feel like this is all mentioned already. Suggestions?
There was a problem hiding this comment.
What do you think about the suggested hostname[;hostname[;…]]:ip|"host-gateway"? If that syntax is okay, I'd suggest using that.
Works for me.
For the other point it is rather complicated, first of all there is no guarantee that host.containers.internal will be added. Depending on your host setup we might not be able to find a free host ip for this so I feel like it is important to mention that the entry might not be set.
With podman machine it is even more complciated as we do not add it to /etc/hosts and let the gvproxy dns resolver resolve the real host ip as podman would use the VM ip otherwise.
And this should likely point to host_containers_internal_ip option from containers.conf because users can overwrite the behavior there or set a fixed ip if they like.
So overall I think something like this:
Unless disabled in via *host_containers_internal_ip* option in *containers.conf* Podman will add the `host.containers.internal` and `host.docker.internal` name by default with an ip pointing to the host. If Podman cannot figure out a suitable ip address it may not add this entry. In order to use a different name it is possible to use `--add-host <name>:host-gateway` and then it will use the same ip in place of host-gateway automatically, however if it failed to find a suitable ip in that case Podman will return an error instead and not silently skip the entry.
There was a problem hiding this comment.
Got it. See updated suggestion.
I skipped mentioning that one can force Podman to add the internal hostnames using --add-host. I rather added a general note that one can use --add-host to overwrite any automatically added entries (I actually use that for --hostname), therefore implying that this is also true for these internal hostnames, even though not specifically mentioning it because I assume this is very rarely used for that.
Since this yields some cross references I decided to not just update --add-host, but --no-hosts, --hostname, and --name as well. This should unify them and hopefully constitute a definitive documentation of Podman's /etc/hosts file. Did I miss something?
I assumed that host_containers_internal_ip also overwrites host-gateway of --add-host, not just the internal hostnames. Is that correct?
Open question: I checked the docs of the host_containers_internal_ip config and one can apparently set it to 'none' to disable adding the internal hostnames by default. Does setting host_containers_internal_ip="none" also affect host-gateway of --add-host? If yes, how? I assumed that it should have no effect, but is that actually true?
There was a problem hiding this comment.
I assumed that host_containers_internal_ip also overwrites host-gateway of --add-host, not just the internal hostnames. Is that correct?
Yes
Does setting host_containers_internal_ip="none" also affect host-gateway of --add-host? If yes, how? I assumed that it should have no effect, but is that actually true?
It will cause an error and not work but this is more of a how the code works today and I am not opposed to changing this in the future if anyone needs that.
PhrozenByte
force-pushed
the
docs-add-host-gateway
branch
2 times, most recently
from
9cb3633 to
1eb7ba6
Compare
PhrozenByte
changed the title
--add-host CLI option/etc/hosts
|
Since we've changed a lot since your initial review @mheon, you might wanna take another look. Thank you! 👍 |
There was a problem hiding this comment.
Man page check complains about
values must be space-separated: '=*hostname[;hostname[;…]]*:*ip*|"host-gateway"'
@edsantiago Do you know hoe the format string should be changed here to make the check happy?
Document the special *host-gateway* flag introduced with containers#19152, mention the special `host.containers.internal` and `host.docker.internal` hostnames, and clarify the option's usage in general. Signed-off-by: Daniel Rudolf <github.com@daniel-rudolf.de>
…options Update the docs to properly cross-reference the `--add-host` option. Signed-off-by: Daniel Rudolf <github.com@daniel-rudolf.de>
PhrozenByte
force-pushed
the
docs-add-host-gateway
branch
from
1eb7ba6 to
6e4ef2c
Compare
|
Ephemeral COPR build failed. @containers/packit-build please check. |
|
Thank you @edsantiago, all fixed 👍 Looks like that the docs complaint is gone, but CI is failing for some other reason now 🙈 No idea what's going on there… Can't even find the logs 😕
Too bad that we can't include it, but I agree that repeating the whole option just for
I was a little confused about that request at first because Unicode ellipsis are used all over the parsed docs, but I just noticed that they are apparently added later when parsing the source files… 😏 |
|
CI is fine! The blue-robot things are always (well, very often) red. We ignore those. The CI that matters is green. Re-reviewing now. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: edsantiago, Luap99, mheon, PhrozenByte The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
At the risk of beating a dead horse, one way to do that might be: LGTM, will leave final approval to @mheon per above request. |
|
/lgtm |
openshift-merge-bot
bot
merged commit 5cef143
into
containers:main
Follow-up to containers#24043 Signed-off-by: Daniel Rudolf <github.com@daniel-rudolf.de>
stale-locking-app
bot
added
the
locked - please file new issue/PR
Document the special host-gateway flag of the
--add-hostoption introduced with #19152, mention the specialhost.containers.internalandhost.docker.internalhostnames, and clarify the option's usage in general. Also update resp. cross-reference the related--no-hosts,--hostname, and--nameoptions.See #19152 and #22653 (comment)
Does this PR introduce a user-facing change?
Cc: @Luap99