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

Fix raw-filter flags by binding flags to viper only if command is run #1202

Merged
merged 1 commit into from
Sep 7, 2023

Conversation

glrf
Copy link
Contributor

@glrf glrf commented Sep 5, 2023

The raw-filter flags are broken since we bind them to viper twice with the same name, during initalization. Once for hubble observe and once for hubble observe flows. This means that only the flags for hubble observe flows work, as it is registered later and overwrites the binding for hubble observe

We fix this by only binding the flags of the command we actually run.

Fixes: #1201

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label PR is blocked until the release note is set label Sep 5, 2023
@glrf
Copy link
Contributor Author

glrf commented Sep 5, 2023

I'm not entirely sure this is the best way to fix this. It's probably the shortest, but it doesn't remove the original footgun that we can easily bind two flagsets to the same viper config.
Maybe someone has a better idea?

@glrf glrf marked this pull request as ready for review September 5, 2023 14:48
@glrf glrf requested a review from a team as a code owner September 5, 2023 14:48
@glrf glrf requested review from chancez and removed request for a team September 5, 2023 14:48
@chancez
Copy link
Contributor

chancez commented Sep 5, 2023

This approach seems reasonable. I'm sure there's probably a "better" way to do it, but I suspect it involves bigger refactors.

I do think what we need however is a test for this. Could you add a new test case into https://github.com/cilium/hubble/blob/main/cmd/cli_test.go where you call hubble observe --allowlist ... --print-raw-filters and verify the output contains the expected "raw filters"? I think that's probably the simplest. We should have a test case for both hubble observe and hubble observe flows to make sure they're both getting the raw filters.

@glrf glrf force-pushed the fix/raw-filters branch 2 times, most recently from 2d61ef7 to ce1d9f9 Compare September 6, 2023 09:07
The raw-filter flags are broken since we bind them to viper twice with
the same name, during initalization. Once for `hubble observe` and
once for `hubble observe flows`. This means that only the flags for
`hubble observe flows` work, as it is registered later and overwrites
the binding for `hubble observe`

We fix this by only binding the flags of the command we actually run.

Fixes: cilium#1201

Signed-off-by: Fabian Fischer <fabian.fischer@isovalent.com>
@glrf
Copy link
Contributor Author

glrf commented Sep 6, 2023

I do think what we need however is a test for this. Could you add a new test case into https://github.com/cilium/hubble/blob/main/cmd/cli_test.go where you call hubble observe --allowlist ... --print-raw-filters and verify the output contains the expected "raw filters"? I think that's probably the simplest. We should have a test case for both hubble observe and hubble observe flows to make sure they're both getting the raw filters.

Good point, I totally forgot about that. I added a test case for it. Had to do a very minor change to printing raw filters toactually use the output defined by the command, so that the test case actually works.

@kaworu kaworu added the release-note/bug This PR fixes an issue in a previous release of Hubble. label Sep 6, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label PR is blocked until the release note is set label Sep 6, 2023
@kaworu kaworu added kind/enhancement This would improve or streamline existing functionality. ⌨️ area/cli Impacts the command line interface of any command in the repository. dont-merge/needs-release-note-label PR is blocked until the release note is set labels Sep 6, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label PR is blocked until the release note is set label Sep 6, 2023
@chancez chancez requested a review from kaworu September 6, 2023 16:02
Copy link
Member

@kaworu kaworu left a comment

Choose a reason for hiding this comment

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

Nice catch!

@rolinh rolinh merged commit b09ba13 into cilium:main Sep 7, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⌨️ area/cli Impacts the command line interface of any command in the repository. kind/enhancement This would improve or streamline existing functionality. release-note/bug This PR fixes an issue in a previous release of Hubble.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Raw-Filters Flags are silently dropped for hubble observe
4 participants