Skip to content

NETOBSERV-1905 Query UDNs without flows #688

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jpinsonneau
Copy link
Contributor

@jpinsonneau jpinsonneau commented Jan 20, 2025

Description

  • Query (C)UDNs using K8S API
  • Display empty (C)UDNs in the topology view

image

Dependencies

n/a

Checklist

If you are not familiar with our processes or don't know what to answer in the list below, let us know in a comment: the maintainers will take care of that.

  • Is this PR backed with a JIRA ticket? If so, make sure it is written as a title prefix (in general, PRs affecting the NetObserv/Network Observability product should be backed with a JIRA ticket - especially if they bring user facing changes).
  • Does this PR require product documentation?
    • If so, make sure the JIRA epic is labelled with "documentation" and provides a description relevant for doc writers, such as use cases or scenarios. Any required step to activate or configure the feature should be documented there, such as new CRD knobs.
  • Does this PR require a product release notes entry?
    • If so, fill in "Release Note Text" in the JIRA.
  • Is there anything else the QE team should know before testing? E.g: configuration changes, environment setup, etc.
    • If so, make sure it is described in the JIRA ticket.
  • QE requirements (check 1 from the list):
    • Standard QE validation, with pre-merge tests unless stated otherwise.
    • Regression tests only (e.g. refactoring with no user-facing change).
    • No QE (e.g. trivial change with high reviewer's confidence, or per agreement with the QE team).

Copy link

openshift-ci bot commented Jan 20, 2025

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

@jpinsonneau
Copy link
Contributor Author

jpinsonneau commented Jan 20, 2025

@jotak & @msherif1234 any thoughts about the approach ?
I still need to find a way to highlight empty UDNs in the topology view.

That approach could be useful for other scenarios such as Nodes / Namespaces especially since we are introducing capture filters. The UI could show we don't capture everything in these cases

v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/client-go/dynamic"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Relying on dynamic client here allows us to read the CRDs without importing the whole API.

Comment on lines 22 to 42
cudns, err := resources.List(ctx, token, schema.GroupVersionResource{
Group: "k8s.ovn.org",
Version: "v1",
Resource: "clusteruserdefinednetworks",
})
if err != nil {
writeError(w, http.StatusInternalServerError, err.Error())
}

udns, err := resources.List(ctx, token, schema.GroupVersionResource{
Group: "k8s.ovn.org",
Version: "v1",
Resource: "userdefinednetworks",
})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We only need to maintain the version per group / resource

Copy link

codecov bot commented Jan 20, 2025

Codecov Report

Attention: Patch coverage is 34.61538% with 17 lines in your changes missing coverage. Please review.

Project coverage is 58.99%. Comparing base (7fd6c46) to head (f5f2edd).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
web/src/components/netflow-traffic.tsx 0.00% 8 Missing ⚠️
web/src/model/topology.ts 0.00% 6 Missing ⚠️
...omponents/drawer/element/element-panel-content.tsx 80.00% 1 Missing ⚠️
...ents/tabs/netflow-topology/2d/styles/styleNode.tsx 0.00% 1 Missing ⚠️
...ponents/tabs/netflow-topology/netflow-topology.tsx 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #688      +/-   ##
==========================================
+ Coverage   56.62%   58.99%   +2.36%     
==========================================
  Files         197      159      -38     
  Lines       10210     6974    -3236     
  Branches     1196     1199       +3     
==========================================
- Hits         5781     4114    -1667     
+ Misses       4059     2658    -1401     
+ Partials      370      202     -168     
Flag Coverage Δ
uitests 58.99% <34.61%> (-0.10%) ⬇️
unittests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...eb/src/components/drawer/element/element-panel.tsx 73.33% <100.00%> (+0.91%) ⬆️
...b/src/components/drawer/netflow-traffic-drawer.tsx 45.54% <ø> (ø)
...ents/tabs/netflow-topology/2d/topology-content.tsx 29.23% <ø> (ø)
web/src/model/netflow-traffic.ts 86.90% <100.00%> (+0.15%) ⬆️
...omponents/drawer/element/element-panel-content.tsx 67.34% <80.00%> (+1.43%) ⬆️
...ents/tabs/netflow-topology/2d/styles/styleNode.tsx 29.16% <0.00%> (-0.63%) ⬇️
...ponents/tabs/netflow-topology/netflow-topology.tsx 36.26% <66.66%> (+1.03%) ⬆️
web/src/model/topology.ts 25.83% <0.00%> (-0.77%) ⬇️
web/src/components/netflow-traffic.tsx 65.25% <0.00%> (-1.51%) ⬇️

... and 38 files with indirect coverage changes

values := []string{}
for _, cudn := range cudns {
md := cudn.Object["metadata"].(map[string]interface{})
values = append(values, fmt.Sprintf("%s", md["name"]))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
values = append(values, fmt.Sprintf("%s", md["name"]))
values = append(values, md["name"])

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I commited this but I will revert it as the metadata can contains other things than strings such as boolean

Copy link

openshift-ci bot commented Apr 7, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign ronensc for approval. For more information see the Code Review Process.

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

Resource: "clusteruserdefinednetworks",
})
if err != nil {
writeError(w, http.StatusInternalServerError, err.Error())
Copy link
Member

Choose a reason for hiding this comment

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

It would make sense I think to propagate the k8s error code, such as 401/403 ?
I never tried that but I guess it should work:

	kerr "k8s.io/apimachinery/pkg/api/errors"
//...
		if err != nil {
			var k8sErr *kerr.StatusError
			if errors.As(err, &k8sErr) {
				writeError(w, int(k8sErr.ErrStatus.Code), err.Error())
			} else {
				writeError(w, http.StatusInternalServerError, err.Error())
			}
		}

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in 96a8ce4

Copy link
Member

@jotak jotak left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Apr 7, 2025
@Amoghrd
Copy link
Contributor

Amoghrd commented Apr 25, 2025

/ok-to-test

@openshift-ci openshift-ci bot added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Apr 25, 2025
Copy link

New image:
quay.io/netobserv/network-observability-console-plugin:9b2cd3a

It will expire after two weeks.

To deploy this build, run from the operator repo, assuming the operator is running:

USER=netobserv VERSION=9b2cd3a make set-plugin-image

Copy link

openshift-ci bot commented Apr 25, 2025

@jpinsonneau: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/plugin-cypress 96a8ce4 link true /test plugin-cypress
ci/prow/qe-e2e-console-tests 96a8ce4 link false /test qe-e2e-console-tests

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm ok-to-test To set manually when a PR is safe to test. Triggers image build on PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants