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.

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

bpf: lb: clean up IPv4 loopback handling #25456

Merged
merged 8 commits into from May 18, 2023
Merged

bpf: lb: clean up IPv4 loopback handling #25456

merged 8 commits into from May 18, 2023

Conversation

julianwiedmann
Copy link
Member

@julianwiedmann julianwiedmann commented May 15, 2023 •
edited

bpf_host, bpf_overlay and bpf_xdp are compiled by the agent with DISABLE_LOOPBACK_LB, to exclude the IPv4 loopback handling from their nodeport LB code paths.

Apply this fact across

  • the BPF complexity tests, so that we test the programs in their actual configuration
  • the BPF compile tests, so that we catch issues with/without DISABLE_LOOPBACK_LB
  • the LB and CT code, so that we include less code in the nodeport paths

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

/test

@julianwiedmann julianwiedmann added sig/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. labels May 15, 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 May 15, 2023
@julianwiedmann
Copy link
Member Author

/test

@julianwiedmann julianwiedmann force-pushed the 1.14-bpf-lb-cleanups branch 2 times, most recently from 0175730 to 5e8d1dc Compare May 15, 2023 15:54
@julianwiedmann
Copy link
Member Author

/test

@julianwiedmann
Copy link
Member Author

/ci-verifier

@julianwiedmann
Copy link
Member Author

/test

@julianwiedmann julianwiedmann changed the title bpf: lb: loopback cleanups bpf: lb: clean up IPv4 loopback handling May 16, 2023
@julianwiedmann julianwiedmann added the kind/complexity-issue Relates to BPF complexity or program size issues label May 16, 2023
@julianwiedmann julianwiedmann marked this pull request as ready for review May 16, 2023 12:10
@julianwiedmann julianwiedmann requested review from a team as code owners May 16, 2023 12:10
@julianwiedmann
Copy link
Member Author

Some random before/after observations:

=== RUN   TestVerifier/bpf_xdp_1
    ...
    verifier_test.go:202: tail_lb_ipv4: processed 23163 insns (limit 131072), stack depth 200
vs
    verifier_test.go:202: tail_lb_ipv4: processed 14288 insns (limit 131072), stack depth 200

=== RUN   TestVerifier/bpf_overlay_1
    ...
    verifier_test.go:202: tail_handle_ipv4: processed 19571 insns (limit 131072), stack depth 240
vs
    verifier_test.go:202: tail_handle_ipv4: processed 12838 insns (limit 131072), stack depth 224

=== RUN   TestVerifier/bpf_host_1
    ...
    verifier_test.go:202: tail_handle_ipv4_from_netdev: processed 40760 insns (limit 131072), stack depth 264
vs
    verifier_test.go:202: tail_handle_ipv4_from_netdev: processed 25143 insns (limit 131072), stack depth 232

ldelossa
ldelossa approved these changes May 17, 2023
View reviewed changes
Copy link
Contributor

@ldelossa ldelossa left a comment •
edited

Choose a reason for hiding this comment

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

Approved, contingent on a small nit with the conditional changing of structure field layout.

Discussed out of bound with Julian.

@julianwiedmann
bpf: complexity: set DISABLE_LOOPBACK_LB for host / overlay / xdp progs
db099c4 
Mirror how the agent compiles these programs, so that we can exclude the
IPv4 loopback code from the nodeport LB paths.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann
bpf: ct: remove loopback handling in ct_create6()
971d196 
There's no loopback support for IPv6, see lb6_local(). So this flag will
always be 0.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann
bpf: ct: wrap IPv4 loopback handling in DISABLE_LOOPBACK_LB
610b28e 
The relevant loopback code in lb4_local() already has this condition. So
also apply it to all the CT paths that would process an entry with the
.loopback flag set.

This excludes the CT loopback code sections from all nodeport paths.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann
bpf: ct: limit loopback case in ct_create4() to CT_EGRESS
f3bc77c 
The loopback case is only relevant after a successful service lookup in
bpf_lxc's from-container flow. Here the CT entry is created with
CT_EGRESS scope. Make this obvious in the code.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann
bpf: ct: remove ct_state->addr from DBG_CT_CREATED4 event
ee87103 
This hasn't aged well, so might as well remove it to make our life easier.

For nodeport connections we inherit the ct_state from the preceding
call to lb4_local(), and ct_state->addr is set to the backend address. We
can get the same information from the actual connection tuple (as the packet
has already been service-DNATed).

For service connections from bpf_lxc we typically don't restore state->addr
in lb4_ctx_restore_state(), so this field will just be 0. The expection
being a loopback connection, where it's hard-coded to IPV4_LOOPBACK.

In either case this has long stopped being the "lb address".

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann
bpf: lb: remove state->addr assignments in lb4_local()
59f5045 
Neither of the callers cares about this field.

In particular for the loopback case, lb4_ctx_restore_state() simply sets up
the "new" ct_state struct with IPV4_LOOPBACK when .loopback is set. So even
here it's not required to reflect the .saddr back to the caller from
lb4_local().

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann
bpf: lb: wrap IPv4 RevNAT loopback handling in DISABLE_LOOPBACK_LB
05c3786 
The loopback handling is only relevant for usage in bpf_lxc. Don't include
it from the nodeport paths.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann
bpf: ct: have DISABLE_LOOPBACK_LB exclude all loopback fields from ct…
4ed09d5 
…_state

Make it easier to catch stray users of the loopback-specific fields. And
improve the compile tests to actually cover this.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann
Copy link
Member Author

Approved, contingent on a small nit with the conditional changing of structure field layout.

Discussed out of bound with Julian.

-> pushed https://github.com/cilium/cilium/compare/980808a212338add90c7c1b34993a761b880930a..9a39a1c1731cc949e89a44946df7181b18701d89
(and rebased afterwards to pick up some CI improvements)

@julianwiedmann
Copy link
Member Author

julianwiedmann commented May 18, 2023 •
edited by maintainer-s-little-helper bot

/test

Job 'Cilium-PR-K8s-1.26-kernel-net-next' failed:

Click to show.

Test Name

K8sDatapathServicesTest Checks N/S loadbalancing With host policy Tests NodePort

Failure Output

FAIL: Can not connect to service "http://192.168.56.11:30428" from outside cluster (1/10)

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.26-kernel-net-next/2383/

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.26-kernel-net-next so I can create one.

Then please upload the Jenkins artifacts to that issue.

@julianwiedmann
Copy link
Member Author

/mlh new-flake Cilium-PR-K8s-1.26-kernel-net-next

@julianwiedmann
Copy link
Member Author

opened #25522 for the runtime flake

@julianwiedmann
Copy link
Member Author

/test-runtime

@julianwiedmann
Copy link
Member Author

Jumping in for mlh, opened #25524 for the net-next flake in https://jenkins.cilium.io/job/Cilium-PR-K8s-1.26-kernel-net-next/2383/testReport/Suite-k8s-1/26/K8sDatapathServicesTest_Checks_N_S_loadbalancing_With_host_policy_Tests_NodePort/

@julianwiedmann
Copy link
Member Author

julianwiedmann commented May 18, 2023 •
edited by maintainer-s-little-helper bot

/test-1.26-net-next

Job 'Cilium-PR-K8s-1.26-kernel-net-next' failed:

Click to show.

Test Name

K8sDatapathServicesTest Checks N/S loadbalancing With host policy Tests NodePort

Failure Output

FAIL: Can not connect to service "http://[fd04::11]:32027" from outside cluster (1/10)

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.26-kernel-net-next/2386/

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.26-kernel-net-next so I can create one.

Then please upload the Jenkins artifacts to that issue.

@julianwiedmann
Copy link
Member Author

Same thing on IPv6: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.26-kernel-net-next/2386/testReport/Suite-k8s-1/26/K8sDatapathServicesTest_Checks_N_S_loadbalancing_With_host_policy_Tests_NodePort/

@julianwiedmann
Copy link
Member Author

/test-1.26-net-next

@julianwiedmann julianwiedmann added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 18, 2023
@julianwiedmann julianwiedmann merged commit 455e0bc into cilium:main May 18, 2023
58 checks passed
@julianwiedmann julianwiedmann deleted the 1.14-bpf-lb-cleanups branch May 18, 2023 17:16
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

@ti-mo ti-mo Awaiting requested review from ti-mo ti-mo is a code owner automatically assigned from cilium/loader

Assignees
No one assigned
Labels
kind/complexity-issue Relates to BPF complexity or program size issues 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. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@julianwiedmann @ldelossa