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: Use protobuf GetType() helper in v1.FlowProtocol() to avoid possible panic #27889

Merged

Conversation

chancez
Copy link
Contributor

@chancez chancez commented Sep 1, 2023

If the event type is unset, or the flow is nil, it's possible that FlowProtocol could panic. To avoid this, use the GetType() helper method which is nil safe.

hubble: Use protobuf GetType() helper in v1.FlowProtocol() to avoid possible panic

@chancez chancez added kind/cleanup This includes no functional changes. area/metrics Impacts statistics / metrics gathering, eg via Prometheus. sig/hubble Impacts hubble server or relay labels Sep 1, 2023
@chancez chancez requested a review from a team as a code owner September 1, 2023 15:39
@chancez chancez self-assigned this Sep 1, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Sep 1, 2023
Copy link
Contributor

@michi-covalent michi-covalent left a comment

Choose a reason for hiding this comment

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

😇

@michi-covalent michi-covalent added the release-note/misc This PR makes changes that have no direct user impact. label Sep 1, 2023
@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 Sep 1, 2023
@rolinh
Copy link
Member

rolinh commented Sep 1, 2023

I guess this affects v1.14 and should be backported, correct?

@chancez
Copy link
Contributor Author

chancez commented Sep 1, 2023

@rolinh Sort of. It's not an actual bug in practice, just a possible bug, so I don't know if we need to backport it. I only noticed when I was writing some new tests in another PR, and the tests were triggering a panic here because the code used this function.

@rolinh rolinh added the needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch label Sep 1, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.14.2 Sep 1, 2023
…anic

If the event type is unset, or the flow is nil, it's possible that
FlowProtocol could panic. To avoid this, use the GetType() helper method
which is nil safe.

Signed-off-by: Chance Zibolski <chance.zibolski@gmail.com>
@chancez chancez force-pushed the pr/chancez/hubble_fix_possible_panic_in_metrics branch from cf3299f to 06d94b9 Compare September 5, 2023 20:44
@chancez
Copy link
Contributor Author

chancez commented Sep 5, 2023

Rebased, and then I shortened the commit message summary to satisfy checkpatch.

@michi-covalent
Copy link
Contributor

/test

@michi-covalent michi-covalent merged commit 2f6a2ed into main Sep 5, 2023
203 checks passed
@michi-covalent michi-covalent deleted the pr/chancez/hubble_fix_possible_panic_in_metrics branch September 5, 2023 23:40
@maintainer-s-little-helper maintainer-s-little-helper bot added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Sep 5, 2023
@michi-covalent michi-covalent added this to Needs backport from main in 1.14.3 Sep 9, 2023
@michi-covalent michi-covalent removed this from Needs backport from main in 1.14.2 Sep 9, 2023
@gandro gandro mentioned this pull request Sep 12, 2023
15 tasks
@gandro gandro added backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. and removed needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Sep 12, 2023
@gandro gandro added backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. and removed backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. labels Sep 25, 2023
@jrajahalme jrajahalme moved this from Needs backport from main to Backport done to v1.14 in 1.14.3 Oct 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/metrics Impacts statistics / metrics gathering, eg via Prometheus. backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. kind/cleanup This includes no functional changes. 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. sig/hubble Impacts hubble server or relay
Projects
No open projects
1.14.3
Backport done to v1.14
Development

Successfully merging this pull request may close these issues.

None yet

5 participants