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

Skip iptables masquerading for packets destined to remote nodes #16603

Merged
merged 5 commits into from Sep 29, 2021

Conversation

pchaigno
Copy link
Member

@pchaigno pchaigno commented Jun 21, 2021

This pull requests updates the iptables rules installed by Cilium to skip masquerading for traffic destined to cluster nodes. As a summary of commits:

  1. Add the ipset binary to the Cilium agent image.
  2. Update cilium-builder and cilium-runtime.
  3. Store node IP addresses in an ipset and use it to skip masquerading.
  4. Collect ipsets in the bugtool reports.
  5. Workaround for VMs where Cilium runs as a service, until provision: Install ipset binary packer-ci-build#278 is merged.

Fixes: #15403.

@pchaigno pchaigno added the sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. label Jun 21, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jun 21, 2021
@pchaigno pchaigno added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Jun 21, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jun 21, 2021
@pchaigno pchaigno force-pushed the skip-node-iptables-masquerading branch from f6d6135 to f466b6b Compare June 21, 2021 12:48
@pchaigno pchaigno temporarily deployed to release-base-images June 21, 2021 12:48 Inactive
@pchaigno pchaigno force-pushed the skip-node-iptables-masquerading branch from f466b6b to 9904711 Compare June 21, 2021 13:31
@pchaigno pchaigno temporarily deployed to release-base-images June 21, 2021 13:31 Inactive
@pchaigno pchaigno force-pushed the skip-node-iptables-masquerading branch from 9904711 to 10ad360 Compare June 21, 2021 13:44
@pchaigno pchaigno temporarily deployed to release-base-images June 21, 2021 13:44 Inactive
@pchaigno pchaigno force-pushed the skip-node-iptables-masquerading branch from 10ad360 to 4945a11 Compare June 21, 2021 13:57
@pchaigno pchaigno force-pushed the skip-node-iptables-masquerading branch from 4945a11 to d8bb856 Compare June 21, 2021 13:58
@pchaigno pchaigno temporarily deployed to release-base-images June 21, 2021 13:59 Inactive
@pchaigno pchaigno force-pushed the skip-node-iptables-masquerading branch from d8bb856 to dc63785 Compare June 21, 2021 14:15
@pchaigno pchaigno temporarily deployed to release-base-images June 21, 2021 14:16 Inactive
@pchaigno pchaigno temporarily deployed to release-base-images June 21, 2021 14:29 Inactive
@pchaigno pchaigno temporarily deployed to release-base-images June 21, 2021 14:29 Inactive
@pchaigno pchaigno temporarily deployed to release-base-images June 21, 2021 14:44 Inactive
@pchaigno pchaigno temporarily deployed to release-base-images June 21, 2021 14:44 Inactive
@pchaigno pchaigno force-pushed the skip-node-iptables-masquerading branch from 1a7534d to 70f2fc8 Compare June 23, 2021 14:31
@pchaigno pchaigno temporarily deployed to release-base-images June 23, 2021 14:31 Inactive
@borkmann borkmann added the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Sep 28, 2021
@borkmann
Copy link
Member

@pchaigno have a chance to rebase?

We will need the ipset binary in subsequent commits to implement an
ipset containing all node IPs (we need to skip packet destined to such
IPs when doing iptables masquerading).

Signed-off-by: Paul Chaignon <paul@cilium.io>
@pchaigno pchaigno force-pushed the skip-node-iptables-masquerading branch from c163507 to f05288d Compare September 29, 2021 10:08
@pchaigno pchaigno temporarily deployed to release-base-images September 29, 2021 10:08 Inactive
@pchaigno pchaigno temporarily deployed to release-base-images September 29, 2021 10:16 Inactive
@pchaigno pchaigno temporarily deployed to release-base-images September 29, 2021 10:16 Inactive
@pchaigno pchaigno force-pushed the skip-node-iptables-masquerading branch from f05288d to 43afb7a Compare September 29, 2021 10:43
@pchaigno pchaigno temporarily deployed to release-base-images September 29, 2021 10:43 Inactive
@pchaigno pchaigno temporarily deployed to release-base-images September 29, 2021 10:43 Inactive
Signed-off-by: Paul Chaignon <paul@cilium.io>
When using BPF masquerading, we don't masquerade traffic destined to
cluster nodes in native routing mode. We detect those destination
using the security identity.

When using iptables masquerading, we cannot implement the exact same
because we can't match on security identities. Instead, we need to
maintain an ipset of IP addresses belonging to cluster nodes and skip
the iptables masquerading rules when a packet is destined to an IP in
the ipset.

Signed-off-by: Paul Chaignon <paul@cilium.io>
We are now using ipsets when iptables masquerading is enabled, to skip
masquerading for traffic to remote nodes. We should therefore collect
ipsets in the bugtool reports, to enable debugging.

Signed-off-by: Paul Chaignon <paul@cilium.io>
This temporary fix is needed until [1] is merged.

1 - cilium/packer-ci-build#278
Signed-off-by: Paul Chaignon <paul@cilium.io>
@pchaigno pchaigno force-pushed the skip-node-iptables-masquerading branch from 43afb7a to 40f813c Compare September 29, 2021 10:56
@pchaigno pchaigno temporarily deployed to release-base-images September 29, 2021 10:56 Inactive
@pchaigno pchaigno temporarily deployed to release-base-images September 29, 2021 10:56 Inactive
@pchaigno pchaigno removed the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Sep 29, 2021
@pchaigno
Copy link
Member Author

/test

@pchaigno
Copy link
Member Author

The checkpatch failure can be ignored (one empty commit description) and the ConformanceAKS failure is expected, the workflow has been temporarily disabled. The ConformanceAKS workflow was also passing before I rebased.

All team reviews are covered except for cilium/cli. That team's review is only required because of the trivial change to the bugtool so I think it's safe to ignore.

Marking ready to merge.

@pchaigno pchaigno added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Sep 29, 2021
pchaigno added a commit to cilium/packer-ci-build that referenced this pull request Sep 29, 2021
The ipset binary is required in the VMs because of
cilium/cilium#16603 when Cilium is running as a
service (e.g., for development VMs and for Runtime tests).

Signed-off-by: Paul Chaignon <paul@cilium.io>
@glibsm glibsm merged commit 1c42ae9 into cilium:master Sep 29, 2021
@pchaigno pchaigno deleted the skip-node-iptables-masquerading branch September 29, 2021 22:44
pchaigno added a commit to cilium/packer-ci-build that referenced this pull request Oct 4, 2021
The ipset binary is required in the VMs because of
cilium/cilium#16603 when Cilium is running as a
service (e.g., for development VMs and for Runtime tests).

Signed-off-by: Paul Chaignon <paul@cilium.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unnecessary iptables masquerade from pod to remote host in direct routing mode
7 participants