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

Add --cluster flag for filtering by cluster #1309

Merged
merged 1 commit into from Dec 6, 2023

Conversation

chancez
Copy link
Contributor

@chancez chancez commented Dec 4, 2023

This is just a short-cut to --node-name <cluster-name>/ but should make it more easier for users to filter by cluster, since many users are not aware the cluster name is part of the node name field on a flow.

@chancez chancez added ⌨️ area/cli Impacts the command line interface of any command in the repository. 🌟 kind/feature This introduces new functionality. labels Dec 4, 2023
@chancez chancez added this to the Hubble CLI 1.0 milestone Dec 4, 2023
@chancez chancez self-assigned this Dec 4, 2023
@chancez chancez requested a review from a team as a code owner December 4, 2023 18:04
@chancez chancez requested review from kaworu and removed request for a team December 4, 2023 18:04
@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-release-note-label PR is blocked until the release note is set labels Dec 4, 2023
@kaworu kaworu added the release-note/minor This PR introduces functionality that users may find relevant to operating Hubble. label Dec 5, 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 Dec 5, 2023
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.

Patch LGTM, requesting change for a test func to be added for this new flag.

Also there is a potential incompatibility with the node-name flag, e.g.

hubble observe --cluster foo --node-name bar/cilium

Ideally we would check for this in the same fashion we do for e.g. the --from-namespace --from-pod since the "format" is the same (i.e. slash separated), but IMO non-blocking as --node-name is less useful than --{from,to}-pod and likewise for --cluster vs --{from,to}-namespace.

Copy link
Contributor

@glrf glrf left a comment

Choose a reason for hiding this comment

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

As is I think the following does something unexpected

hubble observe --cluster foo --node-name bar

I'd expect it to show me all flows related to node bar on cluster foo, but it'll show all flows on cluster foo as well as flows on node bar in any cluster.

So, I agree we should probably make --cluster incompatible with --node-name or implement the same logic I added for namespaces in #1279. But I think just making the flags incompatible should be good enough.

@chancez
Copy link
Contributor Author

chancez commented Dec 5, 2023

Hmm, good point. I had considered making them conflict, but found they seemed fine when using both flags to filter by cluster, but I didn't think about filtering by cluster + node, so I can see the possible confusing behavior. I'll update the PR.

This is just a short-cut to `--node-name <cluster-name>/` but should
make it more easier for users to filter by cluster, since many users are
not aware the cluster name is part of the node name field on a flow.

Signed-off-by: Chance Zibolski <chance.zibolski@gmail.com>
@chancez chancez force-pushed the pr/chancez/main/cluster_name branch from 5dd20c6 to d5e5338 Compare December 5, 2023 17:04
@chancez chancez requested review from kaworu and glrf December 5, 2023 17:16
@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 Dec 6, 2023
@kaworu kaworu merged commit 3d7ace3 into main Dec 6, 2023
5 checks passed
@kaworu kaworu deleted the pr/chancez/main/cluster_name branch December 6, 2023 15:36
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/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 introduces functionality that users may find relevant to operating Hubble.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants