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

bpf: SRv6 fib on encap #26136

Merged
merged 4 commits into from Jun 15, 2023

Conversation

ldelossa
Copy link
Contributor

@ldelossa ldelossa commented Jun 12, 2023

This is the first PR in a set of two.

In this PR we introduce new functions in lib/fib.h to fix the issue described below

A follow up PR will be opened once this is merged which carries the non-backported refactor of fib/lib.h, removing the duplicate code introduced in this PR.

see #26048 (comment) for an explanation of why this occurs in two PRs.

Original SRv6 encapsulation flow:

 flowchart LR
p["pod"] 
v1["veth1"]
v2["veth2"]
hs["host stack"]
e["eth0"]
tx["tx"]

p-->|egress pkt|v1-->v2-->hs-->|fib lookup|e-->|SRv6 encap|tx

This results in the egress packet always choosing the output interface, source, and destination mac from the FIB lookup that occurred on the inner packet.

SRv6 encapsulation flow corrected:

 flowchart LR
p["pod"] 
v1["veth1"]
v2["veth2"]
hs["host stack"]
e["eth0"]
tx["tx"]

p-->|egress pkt|v1-->v2-->hs-->|fib lookup|e-->|SRv6 encap\n+\nfib lookup|tx

Now, we perform an additional fib lookup once the egress packet has been encapsulated, ensuring the encapsulated packet is forwarded correctly based on the host namespace's FIB.

Performing these actions independently is currently necessary for this SRv6 datapath fix.
This is because we perform the fib_lookup already at a egress interface and we may or may not need to redirect to a new interface for tx.

It is obvious to see that two fib lookups are occurring here.
It does seem to me that this can be reduced to one, however this involves changing where encapsulation occurs and will be a larger refactor, however I will also look into this.

Fixes an issue where SRv6 encapsulated packets are forwarded to the wrong layer 2 next hop.

@ldelossa ldelossa added release-note/bug This PR fixes an issue in a previous release of Cilium. feature/srv6 Impacts the SRv6 feature. needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch labels Jun 12, 2023
@ldelossa ldelossa self-assigned this Jun 12, 2023
@ldelossa ldelossa requested a review from a team as a code owner June 12, 2023 18:25
@ldelossa
Copy link
Contributor Author

ldelossa commented Jun 12, 2023

/test

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

Click to show.

Test Name

K8sUpdates Tests upgrade and downgrade from a Cilium stable image to master

Failure Output

FAIL: Unexpected missed tail call

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

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.

@ldelossa
Copy link
Contributor Author

/test-1.26-net-next

@nickolaev
Copy link
Contributor

nickolaev commented Jun 13, 2023

Is it worth considering a unit test?

Copy link
Member

@julianwiedmann julianwiedmann left a comment

Choose a reason for hiding this comment

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

Please apply one small change to the error reporting (and maybe pick up the cosmetic change while at it 😁). Otherwise all good, thanks!

bpf/lib/egress_policies.h Outdated Show resolved Hide resolved
bpf/lib/egress_policies.h Outdated Show resolved Hide resolved
@ldelossa ldelossa force-pushed the ldelossa/srv6-encap-fib-no-refactor branch from 4f87263 to 567c2b6 Compare June 13, 2023 19:00
@ldelossa
Copy link
Contributor Author

@julianwiedmann handled your feedback.

@nickolaev working on a test now, feel free to block until this test commit is present.

@ldelossa ldelossa force-pushed the ldelossa/srv6-encap-fib-no-refactor branch 3 times, most recently from 010c7fa to 48174e4 Compare June 13, 2023 23:12
@julianwiedmann julianwiedmann self-requested a review June 14, 2023 07:25
Copy link
Member

@julianwiedmann julianwiedmann left a comment

Choose a reason for hiding this comment

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

lgtm, thank you!

Two small clarifying questions, but no blocker unless you realize that something is amiss :)

bpf/lib/egress_policies.h Show resolved Hide resolved
bpf/lib/fib.h Outdated Show resolved Hide resolved
@ldelossa ldelossa force-pushed the ldelossa/srv6-encap-fib-no-refactor branch from 48174e4 to 7588f88 Compare June 14, 2023 19:37
@ldelossa
Copy link
Contributor Author

@jibi latest push moves the fib_lookup functions to take src/dst addresses instead of the full header

@ldelossa ldelossa force-pushed the ldelossa/srv6-encap-fib-no-refactor branch 2 times, most recently from d1da55f to 2f1d6f4 Compare June 15, 2023 17:58
This commit adds a new function, 'fib_do_redirect', to 'lib/fib.h'.

This function decouples the 'bpf_redirect' functionality from the
'bpf_fib_lookup' functionality while keeping the existing 'fib_redirect'
logic the same.

In other words, this function can pickup right after the 'fib_lookup' is
performed in 'fib_redirect' and carry out the same exact operations as
'fib_redirect'.

This will be used in a subsequent commit to decouple the `bpf_fib_lookup`
from the `bpf_redirect`, such that each can be performed independently.

This is an addition-only change and amounts to no functional
changes in the data path.

Signed-off-by: Louis DeLosSantos <louis.delos@isovalent.com>
Introduce functions for performing fib_lookups independent of any other
actions.

Signed-off-by: Louis DeLosSantos <louis.delos@isovalent.com>
@ldelossa ldelossa force-pushed the ldelossa/srv6-encap-fib-no-refactor branch from 2f1d6f4 to 4bce021 Compare June 15, 2023 18:02
@ldelossa
Copy link
Contributor Author

@nickolaev added tests: 4bce021

@ldelossa
Copy link
Contributor Author

/test

@ldelossa ldelossa force-pushed the ldelossa/srv6-encap-fib-no-refactor branch from 4bce021 to fb3f639 Compare June 15, 2023 18:50
@nebril nebril added this to Needs backport from main in 1.13.7 Aug 10, 2023
@nebril nebril removed this from Needs backport from main in 1.13.6 Aug 10, 2023
@michi-covalent michi-covalent added this to Needs backport from main in 1.13.8 Sep 9, 2023
@michi-covalent michi-covalent removed this from Needs backport from main in 1.13.7 Sep 9, 2023
@jrajahalme jrajahalme added this to Needs backport from main in 1.13.9 Oct 17, 2023
@jrajahalme jrajahalme removed this from Needs backport from main in 1.13.8 Oct 17, 2023
@nathanjsweet nathanjsweet removed this from Needs backport from main in 1.13.9 Nov 12, 2023
@nathanjsweet nathanjsweet added this to Needs backport from main in 1.13.10 Nov 12, 2023
@nebril nebril added this to Needs backport from main in 1.13.11 Dec 11, 2023
@nebril nebril removed this from Needs backport from main in 1.13.10 Dec 11, 2023
@gentoo-root gentoo-root added this to Needs backport from main in 1.13.12 Jan 17, 2024
@gentoo-root gentoo-root removed this from Needs backport from main in 1.13.11 Jan 17, 2024
@michi-covalent michi-covalent added this to Needs backport from main in 1.13.13 Feb 13, 2024
@michi-covalent michi-covalent removed this from Needs backport from main in 1.13.12 Feb 13, 2024
@thorn3r thorn3r removed this from Needs backport from main in 1.13.13 Mar 13, 2024
@thorn3r thorn3r added this to Needs backport from main in 1.13.13 Mar 13, 2024
@thorn3r thorn3r added this to Needs backport from main in 1.13.14 Mar 13, 2024
@thorn3r thorn3r removed this from Needs backport from main in 1.13.13 Mar 13, 2024
@thorn3r thorn3r added this to Needs backport from main in 1.13.15 Mar 26, 2024
@thorn3r thorn3r removed this from Needs backport from main in 1.13.14 Mar 26, 2024
@julianwiedmann julianwiedmann added affects/v1.13 This issue affects v1.13 branch and removed needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch labels Apr 2, 2024
@julianwiedmann
Copy link
Member

I believe there's no plan to backport this into v1.13 anymore.

@asauber asauber added this to Needs backport from main in 1.13.16 Apr 11, 2024
@asauber asauber removed this from Needs backport from main in 1.13.15 Apr 11, 2024
@nebril nebril added this to Needs backport from main in 1.13.17 May 10, 2024
@nebril nebril removed this from Needs backport from main in 1.13.16 May 10, 2024
@nebril nebril removed this from Needs backport from main in 1.13.17 May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects/v1.13 This issue affects v1.13 branch backport/author The backport will be carried out by the author of the PR. feature/srv6 Impacts the SRv6 feature. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants