Skip to content

bpf_host: emit '-> network' traces for egress packets#16082

Merged
aanm merged 2 commits intocilium:masterfrom
navarrothiago:pr/host-firewall-monitor/12561
May 28, 2021
Merged

bpf_host: emit '-> network' traces for egress packets#16082
aanm merged 2 commits intocilium:masterfrom
navarrothiago:pr/host-firewall-monitor/12561

Conversation

@navarrothiago
Copy link
Copy Markdown

@navarrothiago navarrothiago commented May 11, 2021

bpf_host: emit '-> network' traces for egress packets

See the issue below for the full context. The caveat is, this program is only attached to the native device egress if the host firewall is enabled, so there still won't be any TRACE_TO_NETWORK visibility if that's not the case.

I've also added handle_to_netdev_ipv4() to clean up to_netdev() a bit, to bring it on par with the ipv6 handling.

bpf_host: emit '-> network' traces for packets leaving the node when Host Firewall are enabled

Based on: #13485
Fixes: #12561

Co-authored-by: Thiago Navarro navarro@accuknox.com
Signed-off-by: Timo Beckers timo@isovalent.com
Signed-off-by: Thiago Navarro navarro@accuknox.com

@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 11, 2021
@navarrothiago navarrothiago force-pushed the pr/host-firewall-monitor/12561 branch 5 times, most recently from 74da421 to 6e02e2b Compare May 11, 2021 18:12
@navarrothiago navarrothiago marked this pull request as ready for review May 13, 2021 23:18
@navarrothiago navarrothiago requested review from a team and jrfastab May 13, 2021 23:18
@navarrothiago navarrothiago force-pushed the pr/host-firewall-monitor/12561 branch 2 times, most recently from 5d03a1c to a58c86c Compare May 17, 2021 13:36
Copy link
Copy Markdown
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

This is a bit hard to review as is. Could you separate in multiple logical commits? For example, the changes to take monitor into account in existing traces (what #12561 is about) could be one commit, new traces would be another commit, and general cleanups (e.g., new comments) would be a last commit.

Comment thread bpf/lib/common.h Outdated
Comment thread bpf/lib/trace.h Outdated
@pchaigno pchaigno added area/misc Impacts miscellaneous areas of the code not otherwise owned by another area. 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. area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. and removed area/misc Impacts miscellaneous areas of the code not otherwise owned by another area. labels May 21, 2021
@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 21, 2021
@navarrothiago navarrothiago marked this pull request as draft May 24, 2021 14:57
@navarrothiago navarrothiago force-pushed the pr/host-firewall-monitor/12561 branch from a58c86c to e5dbe84 Compare May 24, 2021 20:47
@navarrothiago
Copy link
Copy Markdown
Author

navarrothiago commented May 24, 2021

new traces would be another commit

Thank you for you comments. Sorry, but I did not understand you. What do you mean by new traces? AFAIU, the issue is related to this comment, right? So, new trace would be the issue. Let me know if I got your point.

UPDATE: I believe I got your point now. You refer send_trace_notify(ctx, TRACE_TO_NETWORK, 0, 0, 0, 0, ret, monitor); in a separate commit, right? I will do that.

UPDATE: Done! Sorry about the misunderstood. Let me know if I am aligned with what you have suggested!

@navarrothiago navarrothiago marked this pull request as ready for review May 24, 2021 21:04
@navarrothiago navarrothiago requested a review from pchaigno May 24, 2021 21:04
@navarrothiago navarrothiago marked this pull request as draft May 25, 2021 15:30
@navarrothiago navarrothiago force-pushed the pr/host-firewall-monitor/12561 branch 2 times, most recently from c74e442 to 72cad26 Compare May 25, 2021 18:55
@navarrothiago navarrothiago marked this pull request as ready for review May 25, 2021 19:56
Copy link
Copy Markdown
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

I think we should exclude the second commit for now. The other two commits look 👍 to me.

Please don't forget to add Timo has a coauthor of the last commit (via Signed-off-by, Co-authored-by, or both).

Comment thread bpf/lib/nodeport.h Outdated
@navarrothiago navarrothiago force-pushed the pr/host-firewall-monitor/12561 branch from 72cad26 to 6eba8ab Compare May 26, 2021 13:04
@pchaigno
Copy link
Copy Markdown
Member

@navarrothiago Could you also fix the second commit's title? Right now, I doesn't provide any useful/actionable information.

@navarrothiago navarrothiago force-pushed the pr/host-firewall-monitor/12561 branch from 6eba8ab to 5a1b385 Compare May 26, 2021 14:19
See the issue below for the full context. The caveat is, this program is
only attached to the native device egress if the host firewall is
enabled, so there still won't be any TRACE_TO_NETWORK visibility if
that's not the case.

bpf_host: emit '-> network' traces for packets leaving the node when
Host Firewall are enabled

Based on: cilium#13485
Fixes: cilium#12561

Co-authored-by: Thiago Navarro <navarro@accuknox.com>
Signed-off-by: Timo Beckers <timo@isovalent.com>
Signed-off-by: Thiago Navarro <navarro@accuknox.com>
@navarrothiago navarrothiago force-pushed the pr/host-firewall-monitor/12561 branch from 5a1b385 to 9758957 Compare May 26, 2021 14:26
Besides, cleans up with some comments and applies code style

Co-authored-by: Thiago Navarro <navarro@accuknox.com>
Signed-off-by: Timo Beckers <timo@isovalent.com>
Signed-off-by: Thiago Navarro <navarro@accuknox.com>
@navarrothiago navarrothiago force-pushed the pr/host-firewall-monitor/12561 branch from 9758957 to 84f7f14 Compare May 26, 2021 14:28
@navarrothiago navarrothiago requested a review from pchaigno May 26, 2021 14:29
@pchaigno
Copy link
Copy Markdown
Member

pchaigno commented May 26, 2021

test-me-please

The checkpatch failure can be ignored.

@navarrothiago
Copy link
Copy Markdown
Author

@pchaigno, should we re-trigger the jobs?

@pchaigno
Copy link
Copy Markdown
Member

test-runtime

@pchaigno pchaigno added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 27, 2021
@pchaigno
Copy link
Copy Markdown
Member

Team reviews are covered and tests are passing. Marking as ready to merge.

@aanm aanm merged commit 4e7ea3b into cilium:master May 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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. 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Monitor Aggregation for connections does not work with host firewall

5 participants