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

hubble: add trace reason support in hubble flows #31226

Merged

Conversation

kaworu
Copy link
Member

@kaworu kaworu commented Mar 7, 2024

Reviewer note: this PR is a draft as the patch are on top of #31211, and I wish to wait for #31073 to be merged and handle conflicts in this PR.

See commits, closes #31202.

@kaworu kaworu added kind/feature This introduces new functionality. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. sig/hubble Impacts hubble server or relay labels Mar 7, 2024
@kaworu kaworu self-assigned this Mar 7, 2024
@kaworu kaworu force-pushed the pr/kaworu/trace-reason-support-in-hubble-flows branch from 51a4fca to 58b278f Compare March 27, 2024 16:01
@kaworu kaworu marked this pull request as ready for review March 27, 2024 16:01
@kaworu kaworu requested review from a team as code owners March 27, 2024 16:01
@kaworu kaworu force-pushed the pr/kaworu/trace-reason-support-in-hubble-flows branch from 58b278f to 083fa5f Compare March 27, 2024 16:07
Copy link
Member

@bimmlerd bimmlerd left a comment

Choose a reason for hiding this comment

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

CODEOWNERS lgtm

Copy link
Member

@rolinh rolinh left a comment

Choose a reason for hiding this comment

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

Looks solid

@kaworu
Copy link
Member Author

kaworu commented Mar 28, 2024

/test

Copy link
Contributor

@lambdanis lambdanis left a comment

Choose a reason for hiding this comment

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

Looks good, left one question

api/v1/flow/flow.proto Show resolved Hide resolved
@kaworu kaworu force-pushed the pr/kaworu/trace-reason-support-in-hubble-flows branch from 083fa5f to 879801c Compare April 10, 2024 09:49
@kaworu
Copy link
Member Author

kaworu commented Apr 10, 2024

@lambdanis addressed your feedback and now setting TRACE_REASON_UNKNOWN = 0 at the protobuf level. The decoding logic is consequently a tad more complex, please take another look 🙏

@chancez chancez requested a review from lambdanis April 17, 2024 15:13
Copy link
Contributor

@lambdanis lambdanis left a comment

Choose a reason for hiding this comment

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

Makes sense, thanks @kaworu!

I see there is a conflict, but apart from that looks good to me.

@kaworu kaworu force-pushed the pr/kaworu/trace-reason-support-in-hubble-flows branch from 879801c to 973db16 Compare April 19, 2024 10:32
@kaworu
Copy link
Member Author

kaworu commented Apr 19, 2024

/test

@kaworu kaworu force-pushed the pr/kaworu/trace-reason-support-in-hubble-flows branch from 973db16 to 7bd6a83 Compare April 19, 2024 14:34
pkg/hubble/parser/threefour/parser.go Outdated Show resolved Hide resolved
@kaworu kaworu force-pushed the pr/kaworu/trace-reason-support-in-hubble-flows branch from 7bd6a83 to 9f33194 Compare April 22, 2024 08:48
@kaworu
Copy link
Member Author

kaworu commented Apr 22, 2024

/test

@kaworu kaworu force-pushed the pr/kaworu/trace-reason-support-in-hubble-flows branch from 9f33194 to bdff831 Compare April 23, 2024 13:27
@kaworu
Copy link
Member Author

kaworu commented Apr 23, 2024

/test

1 similar comment
@kaworu
Copy link
Member Author

kaworu commented Apr 24, 2024

/test

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 26, 2024
@kaworu kaworu force-pushed the pr/kaworu/trace-reason-support-in-hubble-flows branch from bdff831 to e18c3d2 Compare April 26, 2024 09:16
@kaworu kaworu removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 26, 2024
@kaworu
Copy link
Member Author

kaworu commented Apr 26, 2024

/test

cilium#30154 and
cilium#31073 introduced new datapath
trace reasons and had an impact on Hubble, but the sig-hubble team
doesn't get automatically pulled in for review.

This patch adds the sig-hubble team to review datapath_trace.go changes.

Signed-off-by: Alexandre Perrin <alex@isovalent.com>
Before this patch, both the monitor package and Hubble's "threefour"
parser would access the TraceNotify.Reason field directly. However, it
is easy to miss that the Reason field contains the "encrypted" bit and
has to be masked to retrieve the actual trace reason (e.g.
TraceReasonCtReply), as shown by
9939fa2.

This commit introduces several TraceNotify helpers around trace reason
and encryption status, so that both the monitor code and Hubble
"threefour" parser don't have to access the Reason field anymore.

Signed-off-by: Alexandre Perrin <alex@isovalent.com>
Before this patch, TraceReasonEncryptOverlay traces would result in
flows with ingress traffic direction.

Since the flow source is the local host and destination a remote node,
egress arguably make more sense to expose at a high level. Thus, this
patch set the traffic direction to egress consistently for
TraceReasonEncryptOverlay hubble flows.

Signed-off-by: Alexandre Perrin <alex@isovalent.com>
Before this patch, the datapath trace reason was not exposed in Hubble
flows.

In Hubble, the trace reason is used to infer the traffic direction and
reply status. Before a6bfb79 all trace
reasons were CT related, so the information was "converted" by Hubble
into higher level concept / terminology.

Since a6bfb79 there are now non-CT
trace reason that don't map with Hubble's traffic direction and/or reply
status, and thus it make sense to start exposing the underlying trace
reason.

Signed-off-by: Alexandre Perrin <alex@isovalent.com>
@kaworu kaworu force-pushed the pr/kaworu/trace-reason-support-in-hubble-flows branch from e18c3d2 to 8aa0551 Compare April 30, 2024 13:17
@kaworu
Copy link
Member Author

kaworu commented Apr 30, 2024

/test

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 30, 2024
@kaworu kaworu added this pull request to the merge queue May 1, 2024
Merged via the queue into cilium:main with commit 1b38beb May 1, 2024
64 checks passed
@kaworu kaworu deleted the pr/kaworu/trace-reason-support-in-hubble-flows branch May 1, 2024 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature This introduces new functionality. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. sig/hubble Impacts hubble server or relay
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hubble: expose the trace reason field
6 participants