Add endpoint workload filters

[chancez]

This adds support to Hubble CLI for filtering against endpoints workloads
The server side of this was implemented in cilium/cilium#21296

[chancez]

Need to also figure out if this works correctly with --workload "". For "any workload". I special cased *, but I have a feeling the default is "" so it's always selecting pods with workloads currently.

[kaworu]

CI complains seems legit (I tested it and it's pulling LICENSE.txt files):

please run 'go mod tidy && go mod vendor', and submit your changes

[chancez]

Apparently my global gitignore ignores license.txt and that's how that happened 😓

[chancez]

So far I think the flag properly handles the difference between --workload "" and not specifying the flag at all, so my remaining question is should I keep * as a synonym for --workload "" or just use one or the other? My thinking with * was that it might be a bit more obvious than "" meaning "any flow with workloads".

[kaworu]

@chancez I feel conflicted about *. On one hand it make sense and on the other hand it's inconsistent with other * wildcard that can be used by the Hubble CLI. For example, in both the FQDN and namespace filters * can be used for partial matches (e.g. *.cilium.io and k8s*, respectively) while we can't do that for workload (as the filter is a strict match on the server side).

Now we could change the workload filter on the server side to support partial matching but I don't think it makes a lot of sense for workloads, and also we could still do that later if need be (it would be a backward compatible change).

If we elect to support * then I think we should consider supporting both */foo-deploy (same behaviour as foo-deploy) and Deployment/* (same behaviour as Deployment/).

cc @rolinh @gandro

[chancez]

Then maybe to start we remove * and support just "" for the use-case and we can see if we want to do partial matching in the future.

[chancez]

i updated cilium in hubble and now this fails:

15
    flows_test.go:37: 
16
        	Error Trace:	/home/runner/work/hubble/hubble/cmd/observe/flows_test.go:37
17
        	Error:      	"[capture drop l7 policy-verdict trace]" should have 6 item(s), but has 5
18
        	Test:       	TestEventTypes

This is because my update to cilium adds a new event type

+       // MessageTypeTraceSock is a BPF datapath notification carrying a TraceNotifySock
+       // which corresponds to trace_sock_notify defined in bpf/lib/trace_sock.h
+       MessageTypeTraceSock

do we need this as an option in the CLI event types?

[gandro]

I agree with just supporting "" for now. It allows us to extend the expression logic in the future if we ever decide we want to support "foo*" serverside.

[rolinh]

i updated cilium in hubble and now this fails:

15
    flows_test.go:37: 
16
        	Error Trace:	/home/runner/work/hubble/hubble/cmd/observe/flows_test.go:37
17
        	Error:      	"[capture drop l7 policy-verdict trace]" should have 6 item(s), but has 5
18
        	Test:       	TestEventTypes

This is because my update to cilium adds a new event type

+       // MessageTypeTraceSock is a BPF datapath notification carrying a TraceNotifySock
+       // which corresponds to trace_sock_notify defined in bpf/lib/trace_sock.h
+       MessageTypeTraceSock

do we need this as an option in the CLI event types?

Ah, CI working as expected 🙂 I'm not sure about what level of support is expected for MessageTypeTraceSock from the Hubble CLI. cc @aditighag who authored cilium/cilium#20492 and is working on the follow-ups (cilium/cilium#21510).

[gandro]

Yes, we want to add the trace sock events as well, but the server-side code is still missing. The Hubble portion is on my TODO list at the moment.

[rolinh]

@gandro So to answer Chance's question (and have CI happy), the fix would be to add monitorAPI.MessageTypeTraceSock to flowEventTypes in cmd/observe/flows.go it seems.

[chancez]

@gandro So to answer Chance's question (and have CI happy), the fix would be to add monitorAPI.MessageTypeTraceSock to flowEventTypes in cmd/observe/flows.go it seems.

Even if it isn't supported on the server? The other option is to just adjust the test.

[gandro]

@gandro So to answer Chance's question (and have CI happy), the fix would be to add monitorAPI.MessageTypeTraceSock to flowEventTypes in cmd/observe/flows.go it seems.

Even if it isn't supported on the server? The other option is to just adjust the test.

Ah, I thought the error was due to the list of possible values is being pulled in via vendor (which I think we used to). But I was wrong, we maintain our own list here and ensure it's kept up to date via test. In that case, yes, we should adjust the test, and add TraceSock to the test's exclude list.

Sorry about that, I should have read the failing test first.

[aditighag]

@chancez @gandro I don't have context on the Hubble side much. Based on Sebastian's last comment, I presume this PR needs to be extended to exclude TraceSock? @rolinh If that's the case, do you still need me assigned to this PR?
Thanks Sebastian for chiming in!

[chancez]

Updated tests and I removed the usage of *.

[kaworu]

Thanks @chancez LGTM. One more nit on the way: about the first commit ("go.{mod,sum},vendor: Update cilium") can we have the cilium version updated to in the commit message please? Make it a bit easier to track without having to look at the diff.

[nbusseneau]

ci-structure was pinged due to a trivial change in workflow files, acking without looking further.