Add outbound_addr to allow for SNAT instead of MASQ #1180
Conversation
There was a problem hiding this comment.
please add proper integration test in test/ for both iptables and nftables
I have not yet looked into the rules that are added and if they are correct
Also I think this would need to be added to the firewalld code as well cc @mheon
|
let me know if the modifications are ok |
|
Note I do not have time to properly review the rules right now. However please make sure your commits make sense as individual unit. Currently they are pretty meaningless so I suggest you squash everything into one |
|
Ephemeral COPR build failed. @containers/packit-build please check. |
There was a problem hiding this comment.
Sorry for the delay, looking at this closer a single outbound_addr option that only accept ipv4 doesn't seem to logically if we still require ipv6 masquerading.
Should we not have two options then one for ipv4_outbound address and one for ipv6_outbound address instead then?
@mheon I assume this would have to be implemented in the firewalld driver as well?
| network_setup.isolation, | ||
| network_setup.dns_port, | ||
| ); | ||
| network_hash_name: network_setup.network_hash_name.clone(), |
There was a problem hiding this comment.
this is cloning the network hash now which should not be necessary AFAICS. It is always good to avoid extra clones where possible. You should keep the hash &str defined (yes that means defining a lifetime one the struct)
src/network/bridge.rs
Outdated
| outbound_addr: parse_option::<IpAddr>( | ||
| &self.info.network.options, | ||
| OPTION_OUTBOUND_ADDR, | ||
| )?, |
There was a problem hiding this comment.
options should be parsed as first thing in validate() to return errors on incorrect values early and not once we already created the interfaces as they would have to be cleaned up again.
|
For firewalld, this would force all of our NAT to be through rich rules, as the standard port-forwarding rules don't allow for this. Would be a pretty significant change if we wanted to make it. |
I guess we should throw an error if firewalld is used with this option. Given the likely low usage of firewalld users I don't think we need to implement this right now for this PR then. |
I was trying to mimic some of the docker behavior, i use ipv4. it looks like their bridge supports ipv6...or not, documentation is kinda conflicting let me know if you want it and what names to use for it |
|
I think for consistency it makes sense to have it assuming kernel wise is basically the same rule with ipv6? If it turns out to be more complicated then I am fine with leaving it. Naming wise I think |
lto-dev
force-pushed
the
main
branch
2 times, most recently
from
6d0c2ae to
4f27a97
Compare
Signed-off-by: lto-dev <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lto-dev, 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 |
Added outbound_addr to the bridge driver.
I created a bridge-snat plugin to use this feature and do SNAT instead of MASQUERADE on certain networks since i have multiple public ip's on same machine and i needed the traffic to go out on them and not on the gateway.
I can share the plugin as well if anyone wants it.