Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ipmasq: fix nftables backend #1120

Merged
merged 1 commit into from
Nov 21, 2024

Conversation

champtar
Copy link
Contributor

@champtar champtar commented Nov 11, 2024

Rename
SetupIPMasqForNetwork -> SetupIPMasqForNetworks
TeardownIPMasqForNetwork -> TeardownIPMasqForNetworks
and have them take []*net.IPNet instead of *net.IPNet.

This allow the nftables backend to cleanup stale rules and recreate all needed rules in a single transaction, where previously the stale rules cleanup was breaking all but the last IPNet.

Fixes 61d0786
Fixes #1118

Comments for reviewer:
I kept SetupIPMasqForNetwork (without the s) but it's broken for nftables if you call it in loop, so might be better to just break the API and get rid of it, as it was just released in 1.6.0.

@champtar
Copy link
Contributor Author

Tested with

{
  "type": "ptp",
  "ipMasq": true,
  "ipMasqBackend": "nftables",
  "ipam": {
    "type": "host-local",
    "ranges": [
      [{"subnet": "198.18.0.0/17"}],
      [{"subnet": "fd61:7465:6d65:1000::/112"}]
    ],
    "routes": [
      { "dst": "0.0.0.0/0" },
      { "dst": "198.18.128.0/17" },
      { "dst": "198.19.254.254/32" },
      { "dst": "::/0" },
      { "dst": "fd61:7465:6d65:2000::/112" },
      { "dst": "fd61:7465:6d65:ffff::/128" }
    ]
  }
},

@champtar
Copy link
Contributor Author

@danwinship if you can review this one

@danwinship
Copy link
Contributor

I kept SetupIPMasqForNetwork (without the s) but it's broken for nftables if you call it in loop, so might be better to just break the API and get rid of it, as it was just released in 1.6.0.

I don't like keeping the broken function, but I don't know what @squeed will think about removing it, API-compatibility-wise.

It's also possible to avoid needing a new function, by including the CIDR in the comment hash so each one will get a different comment.

@danwinship
Copy link
Contributor

needs a unit test that fails with the old code and passes with the new code

@champtar
Copy link
Contributor Author

I kept SetupIPMasqForNetwork (without the s) but it's broken for nftables if you call it in loop, so might be better to just break the API and get rid of it, as it was just released in 1.6.0.

I don't like keeping the broken function, but I don't know what @squeed will think about removing it, API-compatibility-wise.

It's also possible to avoid needing a new function, by including the CIDR in the comment hash so each one will get a different comment.

Right now teardownIPMasqNFTablesWithInterface completely ignores the IPNets and clean everything, which makes a lot of sense to me

needs a unit test that fails with the old code and passes with the new code

will do

@danwinship
Copy link
Contributor

OK, Casey says containernetworking/plugins has no API guarantees, so you can just drop the old function.

But you should still add a unit test

@champtar champtar force-pushed the ipmasq-fix-nftables branch 2 times, most recently from 1b46fa1 to 1f31269 Compare November 18, 2024 21:21
@champtar
Copy link
Contributor Author

champtar commented Nov 18, 2024

@danwinship unit test modified to test the fix (and more exotic setup), we should be good to go

Rename
SetupIPMasqForNetwork -> SetupIPMasqForNetworks
TeardownIPMasqForNetwork -> TeardownIPMasqForNetworks
and have them take []*net.IPNet instead of *net.IPNet.

This allow the nftables backend to cleanup stale rules and recreate all
needed rules in a single transaction, where previously the stale rules
cleanup was breaking all but the last IPNet.

Fixes 61d0786

Signed-off-by: Etienne Champetier <[email protected]>
Copy link
Contributor

@danwinship danwinship left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@danwinship
Copy link
Contributor

@squeed this one should be ready to merge too

@squeed squeed merged commit 6de8a98 into containernetworking:main Nov 21, 2024
6 checks passed
@champtar champtar deleted the ipmasq-fix-nftables branch November 21, 2024 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ipmasq nftables doesn't support multiple ips (dual stack)
3 participants