Skip to content

Conversation

rectified95
Copy link
Contributor

@rectified95 rectified95 commented Sep 26, 2024

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
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
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.

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
…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? :)

@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
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
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
Development

Successfully merging this pull request may close these issues.

3 participants