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: parser: Set Encrypted bit correctly #14677

Merged
merged 1 commit into from Jan 22, 2021
Merged

Conversation

tgraf
Copy link
Member

@tgraf tgraf commented Jan 20, 2021

The Encrypted flag has not been decoded correctly and inherited to the
Hubble flow. Do so and add unit test.

@tgraf tgraf added kind/bug This is a bug in the Cilium logic. release-note/bug This PR fixes an issue in a previous release of Cilium. sig/hubble Impacts hubble server or relay needs-backport/1.8 labels Jan 20, 2021
@tgraf tgraf requested a review from a team January 20, 2021 23:15
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.10.0 Jan 20, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.8.7 Jan 20, 2021
@tgraf tgraf requested a review from glibsm January 20, 2021 23:15
The Encrypted flag has not been decoded correctly and inherited to the
Hubble flow. Do so and add unit test.

Signed-off-by: Thomas Graf <thomas@cilium.io>
@tgraf tgraf force-pushed the pr/tgraf/hubble-encrypt-bit branch from 4eaa562 to 7cea301 Compare January 20, 2021 23:34
@tgraf
Copy link
Member Author

tgraf commented Jan 20, 2021

test-me-please

Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

The fix itself looks good to me. I have one inline question if maybe more work is needed to fully fix this.

if ip != nil {
ip.Source = srcIP.String()
ip.Encrypted = (tn.Reason & monitor.TraceReasonEncryptMask) != 0
Copy link
Member

@gandro gandro Jan 21, 2021

Choose a reason for hiding this comment

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

Something to keep in mind here is that not all trace points set tn.Reason. So we need to be careful not confuse users with false negatives (we had a similar issue with the "is_reply" field).

A quick grep in the bpf sources seems to indicate that only the from-network and from-overlay trace points set TRACE_REASON_ENCRYPTED. I have not tested it, but does this mean that to-network will always report Encrypted as false?

Is this a problem? If so, we should either try to address that in the datapath or have a "Encrypted: unknown" state in Hubble.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. It's something we need to fix one layer below though. We need to be consistent. It must be possible to trust the flag.

@tgraf
Copy link
Member Author

tgraf commented Jan 21, 2021

retest-4.9

@tgraf
Copy link
Member Author

tgraf commented Jan 21, 2021

retest-runtime

@tgraf
Copy link
Member Author

tgraf commented Jan 21, 2021

retest-net-next

@tgraf
Copy link
Member Author

tgraf commented Jan 21, 2021

retest-4.9

@tgraf tgraf merged commit c338bc6 into master Jan 22, 2021
@tgraf tgraf deleted the pr/tgraf/hubble-encrypt-bit branch January 22, 2021 08:43
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.8 in 1.8.7 Jan 26, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.8 in 1.8.7 Jan 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug This is a bug in the Cilium logic. release-note/bug This PR fixes an issue in a previous release of Cilium. sig/hubble Impacts hubble server or relay
Projects
No open projects
1.8.7
Backport done to v1.8
Development

Successfully merging this pull request may close these issues.

None yet

7 participants