Skip to content

bpf: lxc: fix ifindex in TO_ENDPOINT trace notification#33085

Merged
julianwiedmann merged 2 commits intocilium:mainfrom
julianwiedmann:1.16-bpf-lxc-trace
Jun 19, 2024
Merged

bpf: lxc: fix ifindex in TO_ENDPOINT trace notification#33085
julianwiedmann merged 2 commits intocilium:mainfrom
julianwiedmann:1.16-bpf-lxc-trace

Conversation

@julianwiedmann
Copy link
Copy Markdown
Member

@julianwiedmann julianwiedmann commented Jun 12, 2024 •
edited
Loading

Insert the ifindex of an endpoint into the bpf_lxc program, so that we can use it for trace notifications.

@julianwiedmann julianwiedmann added area/loader Impacts the loading of BPF programs into the kernel. area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. area/monitor Impacts monitoring, access logging, flow logging, visibility of datapath traffic. release-note/misc This PR makes changes that have no direct user impact. labels Jun 12, 2024
@julianwiedmann
Copy link
Copy Markdown
Member Author

julianwiedmann commented Jun 12, 2024

/test

@julianwiedmann julianwiedmann force-pushed the 1.16-bpf-lxc-trace branch 2 times, most recently from 86ba0c5 to fe8684b Compare June 12, 2024 08:44
@julianwiedmann
Copy link
Copy Markdown
Member Author

julianwiedmann commented Jun 12, 2024

/test

@julianwiedmann
Copy link
Copy Markdown
Member Author

julianwiedmann commented Jun 12, 2024

/ci-runtime

@julianwiedmann
Copy link
Copy Markdown
Member Author

julianwiedmann commented Jun 12, 2024

/test

@julianwiedmann julianwiedmann changed the title 1.16 bpf lxc trace bpf: lxc: fix ifindex in TO_ENDPOINT trace notification Jun 12, 2024
@julianwiedmann julianwiedmann added the dont-merge/preview-only Only for preview or testing, don't merge it. label Jun 12, 2024
@julianwiedmann julianwiedmann marked this pull request as ready for review June 12, 2024 17:31
@julianwiedmann julianwiedmann requested review from a team as code owners June 12, 2024 17:31
@julianwiedmann
Copy link
Copy Markdown
Member Author

julianwiedmann commented Jun 12, 2024

Mind you that I have little clue about all the templating dance in the loader. So this might be totally bogus :).

lmb
lmb suggested changes Jun 13, 2024
View reviewed changes
@julianwiedmann
Copy link
Copy Markdown
Member Author

julianwiedmann commented Jun 13, 2024

/test

lmb
lmb approved these changes Jun 14, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@lmb lmb left a comment

Choose a reason for hiding this comment

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

LGTM

@julianwiedmann julianwiedmann removed the dont-merge/preview-only Only for preview or testing, don't merge it. label Jun 14, 2024
@julianwiedmann
Copy link
Copy Markdown
Member Author

julianwiedmann commented Jun 14, 2024

/test

@julianwiedmann
Copy link
Copy Markdown
Member Author

julianwiedmann commented Jun 14, 2024

(quick rebase to pick up healthy CI)

@julianwiedmann
datapath: template the interface index into bpf_lxc
7d4aed4 
Make the ifindex of an endpoint available to the attached BPF program, so
that it can be used by a subsequent patch.

We can most likely unify this later with NATIVE_DEV_IFINDEX, which is only
provided for native devices.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann
bpf: lxc: report ifindex in ingress trace notifications
4201e60 
ipv*_policy() takes an `ifindex` parameter, and exclusively uses it to
fill trace notifications. But for some cases the provided ifindex is
currently 0 (for instance in a configuration with per-EP routing, when
calling from to-container).

Just provide the actual interface index instead.

Reported-by: Tomasz Tarczyński <tomasz.tarczynski@isovalent.com>
Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann
Copy link
Copy Markdown
Member Author

julianwiedmann commented Jun 14, 2024

/test

@julianwiedmann
Copy link
Copy Markdown
Member Author

julianwiedmann commented Jun 14, 2024

(and one more to resolve a conflict in bpf_lxc)

@julianwiedmann julianwiedmann enabled auto-merge June 14, 2024 14:10
@ysksuzuki
Copy link
Copy Markdown
Member

ysksuzuki commented Jun 19, 2024

We don't need to pass CB_IFINDEX from l3_local_delivery anymore with this PR?

cilium/bpf/lib/l3.h

Line 112 in 572bca4

ctx_store_meta(ctx, CB_IFINDEX, ep->ifindex);

@julianwiedmann julianwiedmann added this pull request to the merge queue Jun 19, 2024
Merged via the queue into cilium:main with commit 3c8ff5b Jun 19, 2024
@julianwiedmann julianwiedmann deleted the 1.16-bpf-lxc-trace branch June 19, 2024 08:23
@julianwiedmann
Copy link
Copy Markdown
Member Author

julianwiedmann commented Jun 19, 2024

We don't need to pass CB_IFINDEX from l3_local_delivery anymore with this PR?

cilium/bpf/lib/l3.h

Line 112 in 572bca4

ctx_store_meta(ctx, CB_IFINDEX, ep->ifindex);

I think we still need it to indicate whether the redirect should happen. But ack, it can be a boolean flag now.

@joestringer joestringer mentioned this pull request Jun 28, 2024
Merged
@julianwiedmann
Copy link
Copy Markdown
Member Author

julianwiedmann commented Jul 2, 2024

We don't need to pass CB_IFINDEX from l3_local_delivery anymore with this PR?

cilium/bpf/lib/l3.h

Line 112 in 572bca4

ctx_store_meta(ctx, CB_IFINDEX, ep->ifindex);

I think we still need it to indicate whether the redirect should happen. But ack, it can be a boolean flag now.

Addressed here: #33524

@julianwiedmann julianwiedmann mentioned this pull request Jul 4, 2024
Merged
2 tasks
@github-actions github-actions bot added backport-done/1.15 The backport for Cilium 1.15.x for this PR is done. and removed backport-pending/1.15 labels Jul 9, 2024
This was referenced Jul 10, 2024
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ti-mo ti-mo ti-mo left review comments

@nebril nebril nebril approved these changes

@ysksuzuki ysksuzuki ysksuzuki approved these changes

@gentoo-root gentoo-root Awaiting requested review from gentoo-root gentoo-root is a code owner automatically assigned from cilium/sig-datapath

+1 more reviewer

@lmb lmb lmb approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. area/loader Impacts the loading of BPF programs into the kernel. area/monitor Impacts monitoring, access logging, flow logging, visibility of datapath traffic. backport-done/1.15 The backport for Cilium 1.15.x for this PR is done. 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.

5 participants

@julianwiedmann @ysksuzuki @lmb @nebril @ti-mo