Skip to content

bpf: nodeport: handle revDNAT for local backends at to-netdev/to-overlay#22756

Merged
ldelossa merged 6 commits intocilium:masterfrom
julianwiedmann:nodeport-hostns-fixes
Jan 19, 2023
Merged

bpf: nodeport: handle revDNAT for local backends at to-netdev/to-overlay#22756
ldelossa merged 6 commits intocilium:masterfrom
julianwiedmann:nodeport-hostns-fixes

Conversation

@julianwiedmann
Copy link
Copy Markdown
Member

@julianwiedmann julianwiedmann commented Dec 15, 2022 •
edited
Loading

Replies by local service backends currently get their revDNAT processing in
from-container. There's two problems with that:

  1. we have cases where the packet doesn't reach this part of the
    from-container program (ie. redirect to host proxy), and
  2. we're not handling replies by hostns backends at all.

So add an additional check for RevDNAT in handle_nat_fwd() at to-netdev,
before such an untranslated reply leaves the node. The same is needed for
to-overlay, see
3a83623 ("bpf: add support for local NodePort via tunnel").

Fixes: #22659
Fixes: #22838
Fixes: #21955

@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 Dec 15, 2022
@julianwiedmann
Copy link
Copy Markdown
Member Author

julianwiedmann commented Dec 15, 2022

/test

@julianwiedmann julianwiedmann force-pushed the nodeport-hostns-fixes branch 2 times, most recently from cc956cf to a3144b5 Compare December 16, 2022 15:52
@julianwiedmann
Copy link
Copy Markdown
Member Author

julianwiedmann commented Dec 16, 2022

/test

1 similar comment
@julianwiedmann
Copy link
Copy Markdown
Member Author

julianwiedmann commented Dec 16, 2022

/test

@julianwiedmann
Copy link
Copy Markdown
Member Author

julianwiedmann commented Dec 19, 2022

/test-1.26-net-next

@julianwiedmann
Copy link
Copy Markdown
Member Author

julianwiedmann commented Dec 19, 2022

/test-1.26-net-next

@julianwiedmann julianwiedmann added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Jan 5, 2023
@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 Jan 5, 2023
@julianwiedmann
Copy link
Copy Markdown
Member Author

julianwiedmann commented Jan 5, 2023

/test

@julianwiedmann julianwiedmann force-pushed the nodeport-hostns-fixes branch 2 times, most recently from 43940aa to ac195fd Compare January 13, 2023 10:53
@julianwiedmann
Copy link
Copy Markdown
Member Author

julianwiedmann commented Jan 13, 2023

/test

@julianwiedmann julianwiedmann added area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. needs-backport/1.12 labels Jan 13, 2023
@julianwiedmann
Copy link
Copy Markdown
Member Author

julianwiedmann commented Jan 13, 2023

/test

@julianwiedmann julianwiedmann changed the title WIP hostns / local backend fixes bpf: nodeport: handle revDNAT for local backends at to-netdev/to-overlay Jan 13, 2023
@julianwiedmann julianwiedmann marked this pull request as ready for review January 13, 2023 14:16
@julianwiedmann julianwiedmann requested review from a team as code owners January 13, 2023 14:16
@julianwiedmann julianwiedmann added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jan 19, 2023
@ldelossa ldelossa merged commit 72b1979 into cilium:master Jan 19, 2023
@julianwiedmann julianwiedmann mentioned this pull request Jan 19, 2023
Closed
@julianwiedmann julianwiedmann deleted the nodeport-hostns-fixes branch January 19, 2023 13:54
@liuyuan10
Copy link
Copy Markdown
Contributor

liuyuan10 commented Jan 21, 2023

I see it's going to be backported to v1.12 and v1.13. Are there plan for v1.11?

@aanm aanm mentioned this pull request Jan 23, 2023
Merged
19 tasks
@julianwiedmann julianwiedmann mentioned this pull request Jan 23, 2023
Merged
@aanm aanm added backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. and removed backport-pending/1.13 labels Jan 23, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jan 23, 2023
@joestringer joestringer mentioned this pull request Jan 24, 2023
Merged
@julianwiedmann
Copy link
Copy Markdown
Member Author

julianwiedmann commented Jan 24, 2023

I see it's going to be backported to v1.12 and v1.13. Are there plan for v1.11?

No objection from my side :). Let's see how smooth the v1.12 backport goes...

I take it you already tested on mainline that this fixes your issue?

@julianwiedmann julianwiedmann added the backport/author The backport will be carried out by the author of the PR. label Jan 24, 2023
@liuyuan10
Copy link
Copy Markdown
Contributor

liuyuan10 commented Jan 25, 2023

We just made the decision to update to 1.12 so we don't need this fix in v1.11. I haven't tested it but we will see.

@aanm Regarding v1.12 backport, is there ETA for that. ( I see 1.13 is already done).

@aanm
Copy link
Copy Markdown
Member

aanm commented Jan 25, 2023

@liuyuan10 No ETA for it.

@julianwiedmann julianwiedmann mentioned this pull request Jan 31, 2023
Closed
@julianwiedmann julianwiedmann mentioned this pull request Feb 28, 2023
Open
@tklauser tklauser added the affects/v1.12 This issue affects v1.12 branch label May 22, 2023
julianwiedmann added a commit to julianwiedmann/cilium that referenced this pull request Aug 1, 2023
@julianwiedmann
bpf: nodeport: phase out revDNAT support for local backends in bpf_lxc
b85067a 
We recently added support for Service RevDNAT in to-netdev / to-overlay
(cilium#22756). This is currently only used
as fall-back, for backends that are either in hostNetwork or bypass the
revDNAT step in from-container.

With this patch we switch all new connections for local backends to the new
RevDNAT mechanism. The code change in itself is simple: once to-container
doesn't set the .node_port flag for a pod-level CT entry, from-container
also doesn't apply revDNAT for this connection.

For new connections, replies by a local backend will now pass through
the whole from-container path without being revDNATed. They either get
redirected to the tunnel interface, get redirected to an external
interface (redirect_direct_*()) or pass to the stack for routing to the
external interface. Either way, they eventually reach the revDNAT stage
in handle_nat_fwd().

To avoid any cause for disruption, old connections continue to receive
their RevDNAT in from-container as before. We can pursue their transition
in a subsequent patch.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
julianwiedmann added a commit to julianwiedmann/cilium that referenced this pull request Nov 24, 2023
@julianwiedmann
bpf: nodeport: phase out revDNAT support for local backends in bpf_lxc
c1eb211 
We recently added support for Service RevDNAT in to-netdev / to-overlay
(cilium#22756). This is currently only used
as fall-back, for backends that are either in hostNetwork or bypass the
revDNAT step in from-container.

With this patch we switch all new connections for local backends to the new
RevDNAT mechanism. The code change in itself is simple: once to-container
doesn't set the .node_port flag for a pod-level CT entry, from-container
also doesn't apply revDNAT for this connection.

For new connections, replies by a local backend will now pass through
the whole from-container path without being revDNATed. They eventually
reach the revDNAT stage in handle_nat_fwd(), and get handled there.

To avoid any cause for disruption, old connections continue to receive
their RevDNAT in from-container as before (unless they get reopened, which
is a good point to stop their handling in bpf_lxc). We
can pursue their transition in a subsequent patch.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann julianwiedmann mentioned this pull request Nov 24, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dylandreimerink dylandreimerink dylandreimerink approved these changes

@aspsk aspsk aspsk approved these changes

@aanm aanm aanm approved these changes

@qmonnet qmonnet qmonnet approved these changes

@ti-mo ti-mo Awaiting requested review from ti-mo ti-mo was automatically assigned from cilium/loader

@brb brb Awaiting requested review from brb

Assignees

No one assigned

Labels

affects/v1.12 This issue affects v1.12 branch area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. backport/author The backport will be carried out by the author of the PR. backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. kind/bug This is a bug in the Cilium logic. release-note/bug This PR fixes an issue in a previous release of Cilium.

Projects

None yet

Milestone

No milestone