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

NETOBSERV-204 source or destination match #83

Closed
wants to merge 2 commits into from

Conversation

jpinsonneau
Copy link
Contributor

@jpinsonneau jpinsonneau commented Feb 25, 2022

This PR add a query option to match all Source OR Destination fields

http://localhost:3100/loki/api/v1/query_range?query={app="netobserv-flowcollector"}|json|(DstNamespace=~".*open.*"+and+DstPod=~`.*api.*`)+or+(SrcNamespace=~".*open.*"+and+SrcPod=~`prom.*`)&start=1645797646&limit=100

image
image

I suggest to release that as is to unlock #81 and add an extra custom match filter in the future to unlock custom grouping.

We can imagine custom grouping using drag and drop on UI side and match argument sent to the backend containing group infos (fields, criteria between each value of group / between each group)

Or wait for PF query builder patternfly/pf-roadmap#17

@openshift-ci
Copy link

openshift-ci bot commented Feb 25, 2022

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci
Copy link

openshift-ci bot commented Feb 25, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please ask for approval from jpinsonneau after the PR has been reviewed.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jotak
Copy link
Member

jotak commented Feb 25, 2022

My initial thinking about this task was more to have new filters like "Pods" in the filters list, and that would match both for src and dest pods. So in terms of display you wouldn't have this large box showing Source and Destination and complex filters, you would just have the usual key-value "Pod: foo", and the underlying business logic of the filters would be hidden. WDYT?

@jpinsonneau
Copy link
Contributor Author

jpinsonneau commented Feb 28, 2022

My initial thinking about this task was more to have new filters like "Pods" in the filters list, and that would match both for src and dest pods. So in terms of display you wouldn't have this large box showing Source and Destination and complex filters, you would just have the usual key-value "Pod: foo", and the underlying business logic of the filters would be hidden. WDYT?

So this "Pods" filter would also be available as a column or just in the filters ?
How do you add "Namespaces" to the equation (SrcPod AND SrcNamespace) OR (DstPod AND DstNamespace) to avoid a match with a source pod and destination namespace ?

Maybe in that case we can have a "Match any" associated with only two new filters : Source / Destination that contains both Namespace.Pod ? #78 (comment)

@jpinsonneau
Copy link
Contributor Author

/hold

@jotak
Copy link
Member

jotak commented Feb 28, 2022

To me, the main use case we want to solve is to filter for a given pod / namespace (or whatever kube object), regardless it's source or destination. Is there another use case you wanted to address?

So this "Pods" filter would also be available as a column or just in the filters ?

It should be only a filter (in my initial vision at least)

How do you add "Namespaces" to the equation (SrcPod AND SrcNamespace) OR (DstPod AND DstNamespace) to avoid a match with a source pod and destination namespace ?

OK I get your point. I thought at first that we wanted something like (SrcPod=X OR DstPod=X) AND (SrcNamespace=Y OR DstNamespace=Y), but it catches too large (e.g. it catches the situation where a flow is: srcpod=X, srcns=other, dstpod=other, dstns=Y, which shouldn't be a match for pod=X, ns=Y)

So we want somehow, in the "match all" situation, end up with this:
(SrcPod=X AND SrcNamespace=Y) OR (DstPod=X AND DstNamespace=Y) instead of (SrcPod=X OR DstPod=X) AND (SrcNamespace=Y OR DstNamespace=Y) .. is that right, is that your point?

@jotak
Copy link
Member

jotak commented Feb 28, 2022

So we want somehow, in the "match all" situation, end up with this: (SrcPod=X AND SrcNamespace=Y) OR (DstPod=X AND DstNamespace=Y)

Let's imagine we want to build a more complex query with more filters, in "match all" mode, eg. with Pod, Namespace and Workload (src+dest), and Port (only dst). The resulting query must be:

(SrcPod=X AND SrcNamespace=Y AND SrcWorkload=Z AND DstPort=999) OR (DstPod=X AND DstNamespace=Y AND DstWorkload=Z AND DstPort=999)
Sounds correct or not? I think it is...

So, to extract the rule from there, looks like when a "Src+Dst" filter is in the game, we can run 2 queries: first with all "Src+Dst" fields on "Src", + the non-"Src+Dst" filters, all ANDed; then all "Src+Dst" fields on "Dst", + the non-"Src+Dst" filters, all ANDed

This is maybe not super clear (and must be challenged), we can have a call if you want

@jpinsonneau
Copy link
Contributor Author

Following our slack discussion, let's consider both way and test them.

(SrcPod=X AND SrcNamespace=Y AND SrcWorkload=Z AND DstPort=999) OR (DstPod=X AND DstNamespace=Y AND DstWorkload=Z AND DstPort=999) Sounds correct or not? I think it is...

That's currently working in this PR
image

If you feel groups takes too much room we can remove titles:
image
and optimize using show more / show less buttons if there is too many fields.

Let's try the super-columns source/destination way in parallel
Thanks for your feedback 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants