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 nftables doesn't support multiple ips (dual stack) #1118

Closed
champtar opened this issue Nov 6, 2024 · 3 comments · Fixed by #1120
Closed

ipmasq nftables doesn't support multiple ips (dual stack) #1118

champtar opened this issue Nov 6, 2024 · 3 comments · Fixed by #1120

Comments

@champtar
Copy link
Contributor

champtar commented Nov 6, 2024

dual stack setup

{
  "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": "::/0" },
    ]
  }
},

Looking at nft list ruleset, only the ip6 rules are present in cni_plugins_masquerade table
Looking at nft monitor rules, we see that the ip rules are added then deleted

func setupIPMasqNFTablesWithInterface(nft knftables.Interface, ipn *net.IPNet, network, ifname, containerID string) error {
staleRules, err := findRules(nft, hashForInstance(network, ifname, containerID))

In setupIPMasqNFTablesWithInterface the stale rule logic is incorrect

@danwinship
Copy link
Contributor

right... probably should have separate ip and ip6 tables rather than a single inet one... I think I had originally tried to use inet for the portmap plugin too, but eventually decided it worked better with separate tables.

@champtar
Copy link
Contributor Author

champtar commented Nov 6, 2024

There is no limit on the number of ranges, you could have 2 IPv4 and 5 IPv6, so having separate ip/ip6 would only fix the dual stack case.
We need the list of all IPs in setupIPMasqNFTablesWithInterface to be able to cleanup and recreate all new rules at once.
Bonus point it'll do only 1 nft call instead of 1 per range.

@danwinship
Copy link
Contributor

ah. yes, either that or you could include the range in the comment

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 a pull request may close this issue.

2 participants