pesto: target address mapping and netavark integration#1004
Conversation
c17020c to
5b996c5
Compare
5b996c5 to
1b2918e
Compare
| logrus.Errorf("pesto: reading addr state: %v", err) | ||
| return |
There was a problem hiding this comment.
I feel like it would be better if return an actual error in this function and then in the end just let the caller log it once, that way we can skip the pesto prefix from each log here and just have the prefix once on the caller side.
| // firstIPsWithNetwork picks the first IPv4 and IPv6 from the network IPs | ||
| // map (sorted by network name), returning the IP and the owning network. | ||
| func firstIPsWithNetwork(netIPs map[string]pestoNetworkIPs) (ipv4, ipv4Net, ipv6, ipv6Net string) { | ||
| for _, name := range sortedNetworkNames(netIPs) { |
There was a problem hiding this comment.
we have a network order more or less given via the input network that are sorted I think we should respect that order and not sort based on the names.
| // pestoAddrState tracks which container IPs are currently used for pesto | ||
| // port forwarding. Stored as a per-container JSON file in the rootless | ||
| // netns directory so connect/disconnect operations can detect whether | ||
| // ports are already published and remap when necessary. | ||
| type pestoAddrState struct { |
There was a problem hiding this comment.
so I am thinking if we can avoid yet another extra way to keep the state here via the josn file, the podman db already carries the network result as state so I feel like adding onto that seems better as we then only store it in one location.
Of course it means we then need to pass the state from the podman db back into the teardown/setup functions but I feel like this could be a bit cleaner?
cc @mheon
| if state == nil { | ||
| // Migration fallback: no state file means this container was set up | ||
| // before state tracking existed. Best-effort delete using IPAM IPs. | ||
| ipv4, ipv6 := containerIPsFromNetOpts(opts) |
There was a problem hiding this comment.
I don't think this will gain us anything? The port format has changed so pasta would not consider them to be the same I think?
I guess there is not much do be done, we marked it as experimental so I am fine telling users they need to stop all containers after the update and only then start the new one which should kill and restart pasta to clean out the stale ports. (OR well just let them reboot)
There was a problem hiding this comment.
Yes, that is leftover after my mistake. I will drop that.
| // In pesto mode, pasta handles the full forwarding path including target | ||
| // address mapping, so netavark doesn't need DNAT rules at all. Clear port | ||
| // mappings from the payload to avoid unnecessary iptables/nftables rules. | ||
| if n.rootlessPortForwarder == config.RootlessPortForwarderPasta && n.networkRootless { | ||
| opts.PortMappings = nil |
There was a problem hiding this comment.
I would not drop the ports here, a user could still use podman unshare --rootless-netns curl 127.0.0.1:80 for example and that shoudl work. With nftables we batch update all rules anyway so this will not gain us anything noticeably
| strings.Contains(string(out), "unrecognized option '"+mapGuestAddrOpt) && | ||
| len(mapGuestAddrIPs) == 1 && mapGuestAddrIPs[0] == mapGuestAddrIpv4 { | ||
| // we did add the default --map-guest-addr option, if users set something different we want | ||
| len(mapGuestAddrIPs) == 2 && mapGuestAddrIPs[0] == mapGuestAddrIpv4 && mapGuestAddrIPs[1] == mapGuestAddrIpv6 { |
There was a problem hiding this comment.
I think pasta has supported --map-guest-addr for long enough now that we can hard depend on it. I would first simplify this by removing the retry code path. And then we do not need to special case this new argument here
abc89b3 to
8aa86af
Compare
Fixes: podman-container-tools/podman#28770 Signed-off-by: Jan Rodák <hony.com@seznam.cz>
Fixes: podman-container-tools/podman#28771 Signed-off-by: Jan Rodák <hony.com@seznam.cz>
…ate tracking Fixes: podman-container-tools/podman#28769 Signed-off-by: Jan Rodák <hony.com@seznam.cz>
8aa86af to
c7b86fe
Compare
|
PTAL @Luap99 |
2 participants
portMappingsToPestoArgsnow generates-t hostIP/hostPort:containerIP/containerPort--map-guest-addrfor forwardingPestoAddPorts/PestoDeletePortsfrom netavarkSetup/TeardownFixes: podman-container-tools/podman#28769
Fixes: podman-container-tools/podman#28770
Fixes: podman-container-tools/podman#28771