libnetwork: add new function to better deal with pasta's resolv.conf/hosts file setup in podman #1905
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Luap99 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 |
Luap99
force-pushed
the
pasta-result
branch
5 times, most recently
from
89dd9da
to
ab46f7e
Compare
|
@mheon PTAL I have a test PR (containers/podman#22043) to wire this into podman. cc @sbrivio-rh |
mheon
reviewed
Mar 14, 2024
libnetwork/pasta/pasta_linux.go
Outdated
| for _, addr := range addrs { | ||
| if ipnet, ok := addr.(*net.IPNet); ok && ipnet.IP.IsGlobalUnicast() { | ||
| // make sure to skip localhost and other special addresses | ||
| if ipnet.IP.IsGlobalUnicast() { |
There was a problem hiding this comment.
ah yes, well drop it on the next push
|
It all makes sense to me, but I can't say I have thoroughly reviewed it (I'm not familiar with the libnetwork part). |
Currently both callers in podman and buildah join and inspect the netns to get the local ip configured by pasta in order to add it to /etc/hosts. So instead of doing this in two places let's just do it here once and return the result to the caller. In order to not cause vendoring issues I decided against breaking the API and added a new Setup2 function instead. I will then update podman and buildah to make use of it. Also I plan on adding more fields in the result, i.e. dns address. Because this now depends on linux only functionality make sure to only build it on linux, pasta only works on linux anyway so this is not a problem. Signed-off-by: Paul Holzinger <[email protected]>
This reverts commit 92784a2. I plan on using --dns-forward now so we do not want to disable dns by default, see [1]. [1] containers/podman#19213 Signed-off-by: Paul Holzinger <[email protected]>
By default set 169.254.0.1 as nameserver in the container, right now we do not do special dns handling which means if a user has only localhost resolver or the same nameserver ip as the host ip used by pasta then dns will most likely fail. pasta allows us to remap one ipv4 for dns which will then automatically get remapped to the host dns server from resolv.conf. For that we must use --dns-forward, now the choice of which ip is arbitrary but using the local link address 169.254.0.1 is unlikely to be used so it should avoid conflicts. Also return the ip in the result together with a ipv6 bool so that podman can create a correct resolv.conf with that ip for the container. Signed-off-by: Paul Holzinger <[email protected]>
This tells us if ipv6 is supported. And we should use the forward ips as dns servers to make sure we can read localhost resolvers on the host. Signed-off-by: Paul Holzinger <[email protected]>
For some cases it may be required to get a local ip while excluding a certain ip. This will be used to set a better host.containers.internal name for pasta networks as it is likely that host eth0 ip == container ip so in this case we must use different host ip. Signed-off-by: Paul Holzinger <[email protected]>
For callers like podman we might have to exclude certain ips for the use of host.containers.internal as they might conflict with the inside container ip otherwise. Signed-off-by: Paul Holzinger <[email protected]>
My plan is to directly use the result in podman on the Container struct, this is shared with freebsd even though slirp4netns will never be used there. Thus we can simply move the type definition to the file that is shared to not cause build problems. Signed-off-by: Paul Holzinger <[email protected]>
mheon
reviewed
Mar 15, 2024
| // The pasta binary is looked up in the HelperBinariesDir and $PATH. | ||
| // Note that there is no need for any special cleanup logic, the pasta | ||
| // process will automatically exit when the netns path is deleted. | ||
| func Setup2(opts *SetupOptions) (*SetupResult, error) { |
There was a problem hiding this comment.
Maybe rename to SetupWithResult or NewSetup or something that makes it more clear this is the better, generally preferred option?
There was a problem hiding this comment.
Once I switched the callers in podman and buildah my plan is to rename Setup2 back to Setup()
|
Code LGTM |
|
Ok this should be good to go, buildah PR seems to work as well: containers/buildah#5409 |
|
/lgtm |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
see commits
draft until I implemented the podman parts and know this works as I like.
The basic idea here is to use --dns-forward with pasta to allow proxy dns request to localhost resolvers, it also gives access to all pasta ips so we can then skip them when choosing a ip for host.containers.internal. It will still not work when you only have a single ip on the host but at least it should make it work on more environments and it will no longer set an incorrect ip, rather it will not set the entry at all if there is no suitable host ip.