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/threefour: ignore gopacket errors on unsupported layers #14418

Merged
merged 1 commit into from
Dec 17, 2020

Conversation

tklauser
Copy link
Member

Errors returned by gopacket.DecodingLayerParser.DecodeLayers are already
ignored by the Hubble L3/L4 parser. However, this is currently achieved
by matching the error message of the returned err. Avoid this by setting
gopacket.DecodingLayerParser.IgnoreUnsupported in threefour.New such
that gopacket.DecodingLayerParser.DecodeLayers returns a nil error when
it encounters a layer it doesn't have a parser for.

Errors returned by gopacket.DecodingLayerParser.DecodeLayers are already
ignored by the Hubble L3/L4 parser. However, this is currently achieved
by matching the error message of the returned err. Avoid this by setting
gopacket.DecodingLayerParser.IgnoreUnsupported in threefour.New such
that gopacket.DecodingLayerParser.DecodeLayers returns a nil error when
it encounters a layer it doesn't have a parser for.

Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
@tklauser tklauser added release-note/misc This PR makes changes that have no direct user impact. sig/hubble Impacts hubble server or relay labels Dec 15, 2020
@tklauser tklauser requested review from a team and kaworu December 15, 2020 17:24
@tklauser
Copy link
Member Author

test-me-please

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.

That's much better than string matching on the error message which could break at any gopacket update. Thanks !

@tklauser
Copy link
Member Author

tklauser commented Dec 15, 2020

retest-runtime

failed on known flake #14125 https://jenkins.cilium.io/job/Cilium-PR-Runtime-4.9/3049/

@tklauser
Copy link
Member Author

tklauser commented Dec 16, 2020

retest-gke

Scaling cluster failed https://jenkins.cilium.io/job/Cilium-PR-K8s-GKE/3686/

@tklauser tklauser added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Dec 16, 2020
@tklauser
Copy link
Member Author

Got two reviews from @cilium/hubble (@kaworu is OOO today) and CI passed. Marked as ready to merge.

@tklauser
Copy link
Member Author

retest-gke

Copy link
Member

@glibsm glibsm left a comment

Choose a reason for hiding this comment

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

this doesn't affect any unit tests? 🤔

@maintainer-s-little-helper maintainer-s-little-helper bot removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Dec 16, 2020
@tklauser
Copy link
Member Author

this doesn't affect any unit tests? 🤔

It shouldn't. The behavior is expected to be the same, this only changes the way we treat unsupported layers from having them ignored in gopacket already vs. matching the error string (which can be brittle).

@tklauser tklauser added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Dec 17, 2020
@gandro gandro merged commit 919066d into master Dec 17, 2020
@gandro gandro deleted the pr/tklauser/hubble-parser-gopacket-unsupported branch December 17, 2020 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Development

Successfully merging this pull request may close these issues.

None yet

6 participants