pasta: use new --map-guest-addr option #2136
Merged
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
changed the title
edsantiago
reviewed
Aug 28, 2024
libnetwork/pasta/pasta_linux.go
Outdated
| if err != nil { | ||
| exitErr := &exec.ExitError{} | ||
| if errors.As(err, &exitErr) { | ||
| // special backwards compat check, --map-guest-addr was just added recently so we cannot hard require it yet |
There was a problem hiding this comment.
"recently" is problematic in comments. Would you consider adding a YMD, with a hint to future maintainers that this code can be removed once pasta version 20240814 has propagated?
Luap99
force-pushed
the
pasta-map-guest-addr
branch
from
3081b0d to
e335652
Compare
|
@mheon PTAL, podman PR: containers/podman#23791 |
|
LGTM on my end |
Per feedback[1] the 169.254.0.0/24 range is reserved for future use in RFC 3927. As such we should not use it here as it might break in the future if the range gets assigned a new meaning. Switch to 169.254.1.1. [1] containers/podman#23791 (comment) Signed-off-by: Paul Holzinger <[email protected]>
The --map-guest-addr option allows us to sepcify a ip that is remapped to the actual host ip that was used by pasta. This is done to fix the problem where connecting to the host ip was not possible as the same ip was used in the netns. We now set --map-guest-addr 169.254.1.2 which follows the same idea we already used for the --dns-forward option. With that podman can use this ip to set it for host.containers.internal which should the case where there was no second host ip available, see containers/podman#19213 Signed-off-by: Paul Holzinger <[email protected]>
--map-guest-addr was just added in 20240814, we cannot yet hard require this option to be present. This means we must deal with the case where the option is not working. Both a version check or checking --help would add extra overhead in the good case. To avoid this we try first with the new option and if this fails check the error message for the right error. If it didn't know about the new option we remove it and try to exec pasta again. Signed-off-by: Paul Holzinger <[email protected]>
I already switch all user from the old Setup over to Setup2(), so no we can again reuse the Setup() name. As such alias Setup and Setup for the same function and then once I migrated all callers in podman and buildah I will remove Setup2() here. Signed-off-by: Paul Holzinger <[email protected]>
When using the rootless netns (bridge mode) so far podman ignored the proper pasta or slirp4netns dns sever for networks without aardvark-dns. This is not good. We should try to use them by default, and with the new MapGuestAddr option we need to use that as well for host.containers.internal. The problem is that becuase we only know what options we uses when we started the process later container starts from a new podman process do not really see these options if we just cache the result in memory. So in order to make all following podman process aware we serialize this info struct as json and later processes read it when needed. It also means we do not have to lookup the netns ip evey time so I removed that code. Signed-off-by: Paul Holzinger <[email protected]>
Use %s as %q just quotes/escapes everything which makes it harder to read and trim of the last newline and spaces as well. Also update the warnings comment, we still see warnings by default on our debian VMs in podman CI so this cannot be on the warning level yet. Signed-off-by: Paul Holzinger <[email protected]>
Luap99
force-pushed
the
pasta-map-guest-addr
branch
from
e335652 to
6e7b17b
Compare
GetHostContainersInternalIP() is no longer called in podman or buildah as they use GetHostContainersInternalIPExcluding(). I need to add a new option so chnage the function to accept the parameters as struct so we do not have to break the API every time we add a new parameter. Signed-off-by: Paul Holzinger <[email protected]>
For the pasta network mode we now use --map-guest-addr which means we have a specific ip that we want to use as host.containers.internal address. I first thought we could handle it in podman but that doesn't work as the contianers.conf option must have a higher priority. Signed-off-by: Paul Holzinger <[email protected]>
|
@mheon PTAL again as I added some more changes Passes podman CI containers/podman#23791 so this is good to merge I would say |
|
LGTM |
|
/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, I still need to test and integrate this into podman