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

Refactor proxy handling and improve monitor messages #10906

Merged
merged 8 commits into from
May 5, 2020
Merged

Refactor proxy handling and improve monitor messages #10906

merged 8 commits into from
May 5, 2020

Conversation

joestringer
Copy link
Member

@joestringer joestringer commented Apr 9, 2020 •
edited
Loading

Suggested review commit-by-commit.

Intended functional changes:

  • Improved output of trace messages in cases where there were no trace messages before, eg when delivering a packet to the stack from the bpf_hostdev_ingress program, or when unknown protos short-circuit packet handling logic leading to delivery to the stack.
  • Avoid always printing the "to proxy port" messages. Use debug messages instead.
  • Split bpf_netdev programs into clear "from_netdev" vs. "from_host" to apply at TC ingress / TC egress.
  • Defer the functional preparation of the skb to deliver to proxy so it doesn't happen at the original BPF program (eg bpf_netdev attached at cilium_host) but is instead prepared in the bpf_hostdev_ingress program.

These changes are intermediate refactors to simplify the upcoming BPF tproxy PR, also to help bisect any potential regressions.

Related: #9921

@joestringer joestringer 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 Apr 9, 2020
@joestringer
Copy link
Member Author

test-me-please

@coveralls
Copy link

coveralls commented Apr 9, 2020 •
edited
Loading

Coverage Status

Coverage increased (+0.02%) to 44.631% when pulling bccf7ce379e939a3e7c77711792de818777581a1 on joestringer:submit/defer-proxy-to-hostdev-ingress into 462da1d on cilium:master.

@joestringer joestringer force-pushed the submit/defer-proxy-to-hostdev-ingress branch from 65b7a66 to d7cefa1 Compare April 9, 2020 02:31
@joestringer
Copy link
Member Author

test-me-please

@joestringer
Copy link
Member Author

Also seeing lots of messages like this now which aren't overly informative, I wonder if we should avoid logging them:

-> 0: 10.56.1.169:53554 -> 172.217.11.170:443 tcp ACK
-> 0: 10.56.1.169:53554 -> 172.217.11.170:443 tcp ACK
-> 0: 10.56.1.169:53554 -> 172.217.11.170:443 tcp ACK
-> 0: 10.56.1.169:53554 -> 172.217.11.170:443 tcp ACK

(Just noticed while testing; don't know for sure it's this PR yet but I will confirm before setting this for further review)

@joestringer joestringer force-pushed the submit/defer-proxy-to-hostdev-ingress branch from d7cefa1 to 95dae1e Compare April 28, 2020 05:25
@joestringer
Copy link
Member Author

test-me-please

@joestringer
Copy link
Member Author

Hit #10763 .

Otherwise should be ready for review.

@joestringer joestringer marked this pull request as ready for review April 28, 2020 16:19
@joestringer joestringer requested a review from a team April 28, 2020 16:19
@joestringer joestringer requested a review from a team as a code owner April 28, 2020 16:19
@joestringer joestringer requested a review from pchaigno April 28, 2020 16:19
@joestringer
Copy link
Member Author

@pchaigno this will conflict with your host firewall PR, so let's co-ordinate on how to rebase. This one is a lot smaller so it may make sense for me to just rebase on top of your changes. The logic here is using at_tc_ingress rather than from_host as the boolean for where the traffic comes from, which unfortunately have the opposite values. I'm inclined to swap over to using the from_host nomenclature as that seems like a tidier name.

@joestringer joestringer force-pushed the submit/defer-proxy-to-hostdev-ingress branch from 95dae1e to 826ed7f Compare April 28, 2020 21:35
@joestringer
Copy link
Member Author

test-me-please

qmonnet
qmonnet reviewed Apr 30, 2020
View reviewed changes
Copy link
Member

@qmonnet qmonnet left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Not marked as pending review, but here are a few nits anyway.
Also there's a conflict on bpf/lib/l3.h. Plus I guess the conflicts with Paul's PR to solve somehow.

bpf/lib/proxy.h Outdated Show resolved Hide resolved
bpf/lib/proxy.h Outdated Show resolved Hide resolved
@joestringer joestringer force-pushed the submit/defer-proxy-to-hostdev-ingress branch from 826ed7f to bccf7ce Compare April 30, 2020 20:00
@joestringer
Copy link
Member Author

test-me-please

@joestringer joestringer mentioned this pull request May 1, 2020
Merged
7 tasks
@joestringer
Copy link
Member Author

I pinged @pchaigno offline and we agreed that I should swap the tc_ingress flag in the current version of this PR over to from_host, which will require reversing all of the boolean logic. I'll work on that.

@joestringer joestringer marked this pull request as draft May 1, 2020 16:25
joestringer added 3 commits May 1, 2020 14:54
@joestringer
bpf_netdev: Capture unknown proto passed to stack
e7cc01c 
Signed-off-by: Joe Stringer <[email protected]>
@joestringer
bpf: Refactor proxy handling to separate header
e2d1919 
This makes it easier to ensure that only the relevant changes are
imported into headers in other upcoming changes.

No functional changes.

Signed-off-by: Joe Stringer <[email protected]>
@joestringer
bpf: Make proxy / encrypt CB use more explicit
ab96fad 
Depending on the path through the datapath, skb->cb[0] may be used
either for proxy magic port state transfer or encryption. Make these a
bit more explicit using macros and enums.

No functional changes.

Signed-off-by: Joe Stringer <[email protected]>
joestringer added 5 commits May 1, 2020 14:54
@joestringer
bpf: Trace decrypted or non-encrypt traffic to stack
42462e4 
Send a trace notification when traffic that should be encrypted, or
traffic with no encrypt/proxy logic, passes through the hostdev prog.

Signed-off-by: Joe Stringer <[email protected]>
@joestringer
bpf: Refactor proxy redirect logic into proxy.h
86f71b9 
This logic was configuring the mark from the bpf_hostdev_ingress
program. Future commits will plan to replace this logic with socket
assign logic, so factor it out into the proxy.h header so that all
proxy redirect logic will be in one header.

Signed-off-by: Joe Stringer <[email protected]>
@joestringer
bpf: Don't use capture messages for proxy debug
ee3ba7c 
These messages would always show up in monitor output, sometimes
multiple times, because we were capturing the full content of the
packet. Reduce this to a standard debug message.

Signed-off-by: Joe Stringer <[email protected]>
@joestringer
bpf: Split bpf_netdev prog into ingress/egress
20b7b98 
This will make a subsequent change easier to manage.

Signed-off-by: Joe Stringer <[email protected]>
@joestringer
bpf: Defer proxy preparation to bpf_hostdev_ingress
518cc59 
In the case where we will already return CTX_ACT_OK to pass traffic
through from the TC egress of the cilium_host device onto the TC ingress
(and hence bpf_hostdev_ingress) prog of the cilium_net device, defer the
logic for configuring the mark appropriately until the hostdev program.

This way, future code that refactors the implementation (which relies on
executing in tc ingress) may be easily written on top.

Signed-off-by: Joe Stringer <[email protected]>
@joestringer joestringer force-pushed the submit/defer-proxy-to-hostdev-ingress branch from bccf7ce to 518cc59 Compare May 1, 2020 22:41
@joestringer
Copy link
Member Author

test-me-please

@joestringer joestringer marked this pull request as ready for review May 2, 2020 00:19
@joestringer joestringer requested a review from qmonnet May 4, 2020 19:32
@joestringer joestringer merged commit f604fdf into cilium:master May 5, 2020
@joestringer joestringer deleted the submit/defer-proxy-to-hostdev-ingress branch May 5, 2020 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qmonnet qmonnet qmonnet approved these changes

@jrajahalme jrajahalme jrajahalme approved these changes

@pchaigno pchaigno Awaiting requested review from pchaigno

Assignees
No one assigned
Labels
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.
4 participants
@joestringer @coveralls @jrajahalme @qmonnet