Skip to content

fix: hubble exporter filter test with clashing filters #35058

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 3, 2024
Merged

fix: hubble exporter filter test with clashing filters #35058

merged 1 commit into from
Oct 3, 2024

Conversation

rectified95
Copy link
Contributor

@rectified95 rectified95 commented Sep 26, 2024 •
edited
Loading

Fixes bug in test where a flow matching both the white- and blacklist was not getting filtered out.
Fix suggested by @chancez and @glibsm

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Sep 26, 2024
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Sep 26, 2024
@rectified95 rectified95 changed the title fix: Hubble exporter filter test case matching both white- and blackl… fix: hubble exporter filter test with clashing filters Sep 26, 2024
@rectified95 rectified95 marked this pull request as ready for review September 26, 2024 20:18
@rectified95 rectified95 requested a review from a team as a code owner September 26, 2024 20:18
@rectified95 rectified95 requested a review from rolinh September 26, 2024 20:18
chancez
chancez requested changes Sep 26, 2024
View reviewed changes
Copy link
Contributor

@chancez chancez left a comment

Choose a reason for hiding this comment

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

Can you update the other events in the list to also use the source endpoint field?

@rectified95 rectified95 force-pushed the rectified/fix_exporter_test branch from 076284f to 470479c Compare September 26, 2024 20:43
chancez
chancez requested changes Sep 26, 2024
View reviewed changes
Copy link
Contributor

@chancez chancez left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Sorry, actually I think the test cases are now a bit confusing as to what is expected to be returned since now only one of the allow-pods is being returned. Perhaps we should change the namespace: "allow-pod" so that it's not so confusing? It could be more arbitrary just like "namespace-a" and "namespace-b" and then update the filters to pick one of them and edit the results as expected.

@rectified95 rectified95 force-pushed the rectified/fix_exporter_test branch from 470479c to da3592c Compare September 27, 2024 17:06
@rectified95
fix: Hubble exporter filter test case matching both white- and blackl…
1aaa714 
…ist.

Signed-off-by: Igor Klemenski <igor.klemenski@microsoft.com>
@rectified95 rectified95 force-pushed the rectified/fix_exporter_test branch from da3592c to 1aaa714 Compare September 27, 2024 17:08
@rectified95
Copy link
Contributor Author

Sorry, actually I think the test cases are now a bit confusing as to what is expected to be returned since now only one of the allow-pods is being returned. Perhaps we should change the namespace: "allow-pod" so that it's not so confusing? It could be more arbitrary just like "namespace-a" and "namespace-b" and then update the filters to pick one of them and edit the results as expected.

For sure, the namespaces we now filter on are called namespace-a and namespace-b. I added comments on each test case explaining the filtering behavior for clarity.

@rectified95
Copy link
Contributor Author

@rolinh Mind taking a quick look? :)

@rectified95 rectified95 mentioned this pull request Oct 2, 2024
Merged
@rolinh rolinh added sig/hubble release-note/bug This PR fixes an issue in a previous release of Cilium. labels Oct 3, 2024
@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 Oct 3, 2024
@rolinh rolinh added release-note/misc This PR makes changes that have no direct user impact. and removed release-note/bug This PR fixes an issue in a previous release of Cilium. labels Oct 3, 2024
rolinh
rolinh approved these changes Oct 3, 2024
View reviewed changes
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.

lgtm, thanks!

@rolinh
Copy link
Member

rolinh commented Oct 3, 2024

/test

@rolinh rolinh enabled auto-merge October 3, 2024 07:48
@rolinh rolinh added this pull request to the merge queue Oct 3, 2024
@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 Oct 3, 2024
Merged via the queue into cilium:main with commit f0782e1 Oct 3, 2024
63 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rolinh rolinh rolinh approved these changes

@chancez chancez chancez approved these changes

Assignees
No one assigned
Labels
kind/community-contribution This was a contribution made by a community member. 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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rectified95 @rolinh @chancez