Skip to content

bpf: nodeport: re-introduce Ingress HostFW between RevSNAT and RevDNAT #28960

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Nov 7, 2023
Merged

bpf: nodeport: re-introduce Ingress HostFW between RevSNAT and RevDNAT #28960

merged 2 commits into from
Nov 7, 2023

Conversation

julianwiedmann
Copy link
Member

@julianwiedmann julianwiedmann commented Nov 3, 2023 •
edited
Loading

5e2202a ("bpf: nodeport: make Ingress RevDNAT tail-call conditional")
changed the sequence in which RevNAT and Ingress HostFW are
applied on modern kernels. Where previously we would apply in them in the
order of
RevSNAT / Ingress policy / RevDNAT,
the new sequence is
RevSNAT / RevDNAT / Ingress policy

as we now inline nodeport_rev_dnat_ingress_ipv*(), and then only on the
recircle pass through the Ingress policy in bpf_host's handle_ipv*().
With the subtle difference that

  1. we now apply policy after RevDNAT, and thus the .saddr is the VIP
    instead of the backendIP, and
  2. after RevDNAT, we might redirect to a different interface
    (and thus not recircle, skipping Ingress policy entirely).

Restore the old sequence, by shuffling the relevant HostFW policy code back
into place between RevSNAT and RevDNAT. Having it there was the plan all
along :).

@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 Nov 3, 2023
@julianwiedmann
Copy link
Member Author

/test

@julianwiedmann julianwiedmann force-pushed the 1.15-bpf-nodeport-hostfw-fix branch from f2f4724 to 99aa244 Compare November 3, 2023 06:02
@julianwiedmann julianwiedmann added kind/bug This is a bug in the Cilium logic. area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/misc This PR makes changes that have no direct user impact. area/host-firewall Impacts the host firewall or the host endpoint. area/loadbalancing Impacts load-balancing and Kubernetes service implementations labels Nov 3, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Nov 3, 2023
@julianwiedmann julianwiedmann changed the title 1.15 bpf nodeport hostfw fix bpf: nodeport: re-introduce Ingress HostFW between RevSNAT and RevDNAT Nov 3, 2023
@julianwiedmann
Copy link
Member Author

/test

1 similar comment
@julianwiedmann
Copy link
Member Author

/test

@julianwiedmann
bpf: nodeport: re-introduce Ingress HostFW between RevSNAT and RevDNAT
9460a07 
5e2202a ("bpf: nodeport: make Ingress RevDNAT tail-call conditional")
changed the sequence in which RevNAT and Ingress HostFW are
applied on modern kernels. Where previously we would apply in them in the
order of
    RevSNAT / Ingress policy / RevDNAT,
the new sequence is
    RevSNAT / RevDNAT / Ingress policy

as we now inline nodeport_rev_dnat_ingress_ipv*(), and then only on the
recircle pass through the Ingress policy in bpf_host's handle_ipv*().
With the subtle difference that
1. we now apply policy *after* RevDNAT, and thus the .saddr is the VIP
   instead of the backendIP, and
1. after RevDNAT, we might redirect to a different interface
   (and thus *not* recircle, skipping Ingress policy entirely).

Restore the old sequence, by shuffling the relevant HostFW policy code back
into place between RevSNAT and RevDNAT. Having it there was the plan all
along :).

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann
bpf: nodeport: fix up indentation in ingress path
83a10f6 
5e2202a ("bpf: nodeport: make Ingress RevDNAT tail-call conditional")
introduced some mis-indentation, fix this up again. While at it also clean
up some tiny whitespace damage for two jump labels.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann julianwiedmann force-pushed the 1.15-bpf-nodeport-hostfw-fix branch from 99aa244 to 83a10f6 Compare November 4, 2023 06:12
@julianwiedmann
Copy link
Member Author

/test

@julianwiedmann julianwiedmann marked this pull request as ready for review November 4, 2023 08:05
@julianwiedmann julianwiedmann requested a review from a team as a code owner November 4, 2023 08:05
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Nov 6, 2023
@julianwiedmann julianwiedmann merged commit 2685d16 into cilium:main Nov 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ldelossa ldelossa ldelossa approved these changes

Assignees
No one assigned
Labels
area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. area/host-firewall Impacts the host firewall or the host endpoint. area/loadbalancing Impacts load-balancing and Kubernetes service implementations kind/bug This is a bug in the Cilium logic. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@julianwiedmann @ldelossa