SNAT routing #1083
SNAT routing #1083
Conversation
ch-iara
force-pushed
the
chiara/iptables-routing-snat-cgnat
branch
4 times, most recently
from
e25610a to
ab2b2e8
Compare
Routes work with pingability for snat exit integration test. In this mode clients are assigned an external ip that their traffic exits on.
Wherein we finish testing the teardown logic in the integration test. Routers offline for more than 5 days have their SNAT rules torn down so that the ip is freed up.
ch-iara
force-pushed
the
chiara/iptables-routing-snat-cgnat
branch
from
ab2b2e8 to
fbfd486
Compare
ch-iara
force-pushed
the
chiara/iptables-routing-snat-cgnat
branch
from
fbfd486 to
7c6628a
Compare
There was a problem hiding this comment.
Awesome work getting this working. It's an in depth change with a lot of moving parts.
I've left some comments on logic cleanup in a few places, and one edgecase that needs to be handled. Overall I like the structure you've used and the implementation is very clean.
My only flag is the significant use of functional chains to get a given value out of an array for example, these can be hard to read and confusing sometimes. You can usually just ask copilot to generate a utility function or an iterative version of the same check and get a great result.
| info!("Setting up iptables SNAT rules on exit for client: {client_external_ip} to {client_internal_ip}"); | ||
| add_iptables_rule( | ||
| "iptables", | ||
| &[ |
There was a problem hiding this comment.
where these actually ever tested since the test environment is nftables only?
I suppose we'd have to delete nftables in the container to try.
There was a problem hiding this comment.
if it turns out they don't work I'd rather drop iptables support for this entierly. We still want it form the routers becuase we have hundreds of devices out there without nftables installed, but for exit appliances since none currently exist we will never make an image without nftables in the first place. Not that this isn't still useful if someone tries to install an exit on an older machine for some reason in the future, but it won't be us.
There was a problem hiding this comment.
removed iptables rules since if we won't be using them I don't think we need to include them untested
The previous implementation of setup would have readded the same SNAT rules to nftables any time a new client registered to the exit, which would clog the tables and mess with teardown logic. In this fix we only add rules when a client is not in the external ip assignments list AND also online as defined by WG_INACTIVE_THRESHOLD. When a client is deemed offline they are now removed from external ip assignments list as they should have been on top of just having their snat rules torn down.
|
Multi Exit test currently spitting about a bunch of "Too many open files" error from all namespaces and panics when trying to spawn the thread registering to exit? |
Better handling for the 0 timestamp wg handshake, now clients have 1 min to handshake after connecting and receiving an ip before they can be auto torn down. + Extra cleanup
Integration test is still cooking, iptables rules must be translated into nftables as should parsing for the teardown code when nftables in use