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-135 super columns + NETOBSERV-204 filters #90

Closed

Conversation

jpinsonneau
Copy link
Contributor

@jpinsonneau jpinsonneau commented Mar 3, 2022

This PR is an alternative to #83 using FQDN fields

Full Qualified Name fields can either contains ip:port or namespace.pod
image

Filtering on SrcFQDN + DstFQDN with match any option will group query by (SrcNamespace AND SrcPod) OR (DstNamespace AND DstPod) so it solves NETOBSERV-204

image

http://localhost:3100/loki/api/v1/query_range?query={app="netobserv-flowcollector",FlowDirection="0"}|json|(DstNamespace=~".*openshift-monitoring.*"+or+DstPod=~`.*prometheus-k8s-1.*`)+or+(SrcNamespace=~".*openshift-apiserver.*"+or+SrcPod=~`.*apiserver-f65cb946b-7mckt.*`)&limit=100&start=1646324861

Also added the ability to filter on Source / Destination / Both for Common fields using an accordion in filter dropdown
image
image

These new filters appears in the table as fields for PoC.

@openshift-ci
Copy link

openshift-ci bot commented Mar 3, 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 Mar 3, 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

@jpinsonneau jpinsonneau force-pushed the grouped_columns_filters branch 2 times, most recently from 20c3869 to e5b0252 Compare March 3, 2022 16:55
@jpinsonneau jpinsonneau marked this pull request as ready for review March 3, 2022 17:05
pkg/loki/query.go Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot added lgtm and removed lgtm labels Mar 4, 2022
@openshift-ci
Copy link

openshift-ci bot commented Mar 4, 2022

New changes are detected. LGTM label has been removed.

prefix = "Dst"
}

for _, value := range values {
Copy link
Member

@jotak jotak Mar 4, 2022

Choose a reason for hiding this comment

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

It could be risky to handle so many different cases under the "FQDN" case, no? For instance, what if we're doing a partial matches? could "10.9" be seen as namespace + pod, whereas we're looking for an IP with partial match?

I just wonder if you're not trying to do too much here, beyond the main use case of a FQDN which would just be "namespace.pod" ?
by the way, I think it should be reversed, "pod.namespace" rather than "namespace.pod", it more on page with the actual kube domain naming: https://kubernetes.io/docs/concepts/services-networking/dns-pod-service/#pod-s-hostname-and-subdomain-fields

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes there are a lot of cases but I guess we can improve front end input to split them correctly. As a user I feel it's easier to type namespace first.
What if we separate ip:port from namespace.pod cases in two filters and implement autocompletes for namespaces & pods ?
It could show a first AC for namespaces then a second one for pods (filtered by selected namespace).
Then the concatenation will be done by the code and added to filters to avoid mistakes.

Copy link
Member

Choose a reason for hiding this comment

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

yes, if the user is presented specific fields for namespace and pod, it solves the FQDN order problem (but it's more work to do, especially with auto-completion).
+1 also to separate ip/port to a different filter type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure I can handle it 👍
Thanks for the feedback

jpinsonneau and others added 2 commits March 7, 2022 13:19
Co-authored-by: Mario Macias <mmaciasl@redhat.com>
@jpinsonneau
Copy link
Contributor Author

  • Spitted FQDN field to Namespace & Pod and Address & Port filters
  • Added auto completion for Namespace / Pod / Namespace & Pod filters
    • Namespaces are get only when user select a namespace filter type
    • Pods are get only when at least one filter already exists on a specific namespace
    • Namespace & Pod shows namespaces at first, then pods from selected namespace.
      User can't filter if namespace + pod is not found (only for this field)
    • Address & Port manage IPv4 / Range / CIDR only since we use : to split

image
image
image
image

@jpinsonneau jpinsonneau requested a review from jotak March 7, 2022 12:38
@jpinsonneau
Copy link
Contributor Author

I have played a bit around a more generic Kubernetes Object filter for Service / Deployment / StatefulSet / DaemonSet / Job / CronJob.

We can either wait for FLP merge or go ahead with it and do the proper modification in processKubeObjectFilter function when FLP is ready

image
image

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

Successfully merging this pull request may close these issues.

None yet

3 participants