Skip to content

test(notify): add tests to compare flow proto parsed from notify events #35059

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 3 commits into from
Oct 3, 2024
Merged

test(notify): add tests to compare flow proto parsed from notify events #35059

merged 3 commits into from
Oct 3, 2024

Conversation

sypakine
Copy link
Contributor

Related: #35057

Diffing the flow proto provides more clarity how a change to the notification or parsing has affected the output.

Note: no release note

@sypakine
test(notify): add drop notify flow diff test
1a0e4f5 
Diffing the flow proto provides more clarity how a change to the notification or parsing has affected the output.

Signed-off-by: Mark St John <markstjohn@google.com>
@sypakine
test(notify): add trace notify flow diff test
ee931bf 
Diffing the flow proto provides more clarity how a change to the notification or parsing has affected the output.

Signed-off-by: Mark St John <markstjohn@google.com>
@sypakine
test(notify): add policy verfict notify flow diff test
f82e361 
Diffing the flow proto provides more clarity how a change to the notification or parsing has affected the output.

Signed-off-by: Mark St John <markstjohn@google.com>
@sypakine sypakine requested a review from a team as a code owner September 26, 2024 20:56
@sypakine sypakine requested a review from rolinh September 26, 2024 20:56
@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
@sypakine sypakine mentioned this pull request Sep 26, 2024
Closed
@sypakine
Copy link
Contributor Author

/test

@rolinh rolinh added sig/hubble release-note/misc This PR makes changes that have no direct user impact. 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 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.

Sorry for the delay in reviewing your PR. Changes lgtm.

@rolinh rolinh enabled auto-merge October 3, 2024 08:01
@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
@rolinh rolinh added this pull request to the merge queue Oct 3, 2024
Merged via the queue into cilium:main with commit 4f00d67 Oct 3, 2024
68 of 69 checks passed
@jrajahalme
Copy link
Member

This broke main:

~/go/src/github.com/cilium/cilium$ cd pkg/hubble/parser/threefour/
~/go/src/github.com/cilium/cilium/pkg/hubble/parser/threefour$ go test ./...
--- FAIL: TestDecode_DropNotify (0.00s)
    --- FAIL: TestDecode_DropNotify/drop_unknown (0.00s)
        parser_test.go:1497: Unexpected diff (-want +got):
              (*flow.Flow)(Inverse(protocmp.Transform, protocmp.Message{
              	... // 5 identical entries
              	"ethernet":   protocmp.Message{"@type": s"flow.Ethernet", "destination": string("07:08:09:00:01:02"), "source": string("01:02:03:04:05:06")},
              	"event_type": protocmp.Message{"@type": s"flow.CiliumEventType", "type": int32(1)},
            + 	"file":       s`{name:"unknown(0)"}`,
              	"source":     protocmp.Message{"@type": s"flow.Endpoint", "ID": uint32(1234)},
              	"verdict":    s"DROPPED",
              }))
    --- FAIL: TestDecode_DropNotify/drop_egress (0.00s)
        parser_test.go:1497: Unexpected diff (-want +got):
              (*flow.Flow)(Inverse(protocmp.Transform, protocmp.Message{
              	... // 5 identical entries
              	"ethernet":          protocmp.Message{"@type": s"flow.Ethernet", "destination": string("07:08:09:00:01:02"), "source": string("01:02:03:04:05:06")},
              	"event_type":        protocmp.Message{"@type": s"flow.CiliumEventType", "type": int32(1)},
            + 	"file":              s`{name:"unknown(0)"}`,
              	"source":            protocmp.Message{"@type": s"flow.Endpoint", "ID": uint32(1234)},
              	"traffic_direction": s"EGRESS",
              	"verdict":           s"DROPPED",
              }))
    --- FAIL: TestDecode_DropNotify/drop_ingress (0.00s)
        parser_test.go:1497: Unexpected diff (-want +got):
              (*flow.Flow)(Inverse(protocmp.Transform, protocmp.Message{
              	... // 5 identical entries
              	"ethernet":          protocmp.Message{"@type": s"flow.Ethernet", "destination": string("07:08:09:00:01:02"), "source": string("01:02:03:04:05:06")},
              	"event_type":        protocmp.Message{"@type": s"flow.CiliumEventType", "type": int32(1)},
            + 	"file":              s`{name:"unknown(0)"}`,
              	"source":            protocmp.Message{"@type": s"flow.Endpoint"},
              	"traffic_direction": s"INGRESS",
              	"verdict":           s"DROPPED",
              }))
FAIL
FAIL	github.com/cilium/cilium/pkg/hubble/parser/threefour	0.030s
FAIL

@rolinh rolinh mentioned this pull request Oct 3, 2024
Merged
@rolinh
Copy link
Member

rolinh commented Oct 3, 2024

Thanks for reporting @jrajahalme, looks like there was a merge race. See #35196 for a fix.

jrajahalme added a commit to jrajahalme/cilium that referenced this pull request Oct 3, 2024
@jrajahalme
hubble: Fix parser test
650aa12 
Add missing expected file name.

Fixes: cilium#35059

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
@jrajahalme jrajahalme mentioned this pull request Oct 3, 2024
Closed
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

Assignees
No one assigned
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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sypakine @jrajahalme @rolinh