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

Ldelossa/srv6 encap fib #26048

Closed
wants to merge 5 commits into from
Closed

Conversation

ldelossa
Copy link
Contributor

@ldelossa ldelossa commented Jun 8, 2023

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.

To accomplish this a refactor of lib/fib.h was necessary.
The fib library can now be used to perform a v4/v6 fib lookup and a redirect independently, where before the actions were coupled together.

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.
This refactor does not change the semantics of the existing functions, they work exactly how they used to, sans removing the iif field from the fib_redirect_v[4|6] functions, as we can extract them from the passed ctx making the arguments redundant.

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 requested a review from a team as a code owner June 8, 2023 18:28
@ldelossa ldelossa requested a review from aspsk June 8, 2023 18:28
@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 8, 2023
@ldelossa ldelossa added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Jun 8, 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 Jun 8, 2023
@ldelossa ldelossa added the needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch label Jun 8, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.13.4 Jun 8, 2023
@ldelossa ldelossa force-pushed the ldelossa/srv6-encap-fib branch 2 times, most recently from 1ba630c to 5bb84c4 Compare June 8, 2023 20:03
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>
Refactor 'fib_redirect' to utilize the new 'fib_do_redirect' to perform
the actions after a call to 'fib_lookup'.

Signed-off-by: Louis DeLosSantos <louis.delos@isovalent.com>
@ldelossa ldelossa force-pushed the ldelossa/srv6-encap-fib branch 2 times, most recently from f68b223 to 8acb3ea Compare June 8, 2023 20:09
@ldelossa ldelossa added the feature/srv6 Impacts the SRv6 feature. label Jun 8, 2023
@ldelossa
Copy link
Contributor Author

ldelossa commented Jun 8, 2023

/test

@ldelossa
Copy link
Contributor Author

ldelossa commented Jun 8, 2023

/ci-verifier

@ldelossa
Copy link
Contributor Author

ldelossa commented Jun 8, 2023

/test

Introduce functions for performing fib_lookups independent of any other
actions.

Signed-off-by: Louis DeLosSantos <louis.delos@isovalent.com>
@ldelossa
Copy link
Contributor Author

ldelossa commented Jun 8, 2023

/test

This commit updates the fib_redirect_v4/6 functions to utilized the
newly decoupled 'fib_lookup_*' and 'fib_do_redirect' functions.

Two tests were also fixed as they assumed fib_params were aligned on 16
byte boundary.

Signed-off-by: Louis DeLosSantos <louis.delos@isovalent.com>
An additional FIB lookup must be performed after encapsulation to correctly
forward the encapsulated SRv6 packet to the next hop.

Introduce a new function `srv6_refib` which performs an additional FIB lookup
after the encapsulation has been performed.

This function may redirect the egress packet to another interface, rewrite the
current DMAC, or simply let the packet transmit on the current native-dev,
depending on the result of the subsequent FIB lookup.

Signed-off-by: Louis DeLosSantos <louis.delos@isovalent.com>
@ldelossa
Copy link
Contributor Author

ldelossa commented Jun 9, 2023

/test

@YutaroHayakawa YutaroHayakawa self-requested a review June 9, 2023 06:00
@qmonnet qmonnet added this to Needs backport from main in 1.13.5 Jun 9, 2023
@qmonnet qmonnet removed this from Needs backport from main in 1.13.4 Jun 9, 2023
@ldelossa
Copy link
Contributor Author

ldelossa commented Jun 9, 2023

Tests are all checking out:

 Conformance ginkgo (ci-ginkgo) — Gingko test failed 

Is not required, and is failing for all PRs regularly.

@julianwiedmann
Copy link
Member

Ah, I hadn't realized this also needs to go into v1.13. That FIB code looks very different 😬.

Could we then just do what's needed for the fix (to have a backportable PR), and leave the refactor for a follow-on PR?

I peeked at just the following applied, and that lgtm so far 👍.

7007d6761a bpf,srv6: perform fib lookup after encap
75e06d8be4 bpf,fib: introduce fib_lookup_v4/6 functions
92a5475f63 bpf,fib: introduce fib_do_redirect function

@ldelossa
Copy link
Contributor Author

ldelossa commented Jun 12, 2023

I think I see what you're saying.

The SRv6 code only needs the fib_lookup* and the fib_do_redirect functions to exist for this fix.

I'll split this out like you're saying, just introducing the the pieces necessary for SRv6 fix, and backport this.

Then open another PR with the refactor, which wont need to be back ported.

Sound aitttte?

@ldelossa
Copy link
Contributor Author

Closing in favor of opening a PR which splits the SRv6 fixes and the refactor.

@ldelossa ldelossa closed this Jun 12, 2023
@ldelossa
Copy link
Contributor Author

@julianwiedmann new PR here: #26136

@gentoo-root gentoo-root added this to Needs backport from main in 1.13.6 Jul 26, 2023
@gentoo-root gentoo-root removed this from Needs backport from main in 1.13.5 Jul 26, 2023
@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
@jrajahalme jrajahalme removed this from Needs backport from main in 1.13.9 Oct 17, 2023
@jrajahalme jrajahalme removed the needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch label Oct 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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

3 participants