Skip to content

Conversation

@jpinsonneau
Copy link
Contributor

Description

Add namespace bar on top of the page when the user doesn't have the admin role.
The page will act as same as in the developer perspective in that case.

For admins, the selector is not showing.

Dependencies

based on #658

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).

@openshift-ci
Copy link

openshift-ci bot commented Feb 6, 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 ask for approval from jpinsonneau. 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

@memodi
Copy link
Member

memodi commented Mar 6, 2025

@jpinsonneau could you rebase here please?

@jpinsonneau
Copy link
Contributor Author

@jpinsonneau could you rebase here please?

sure, here we go !

@jpinsonneau
Copy link
Contributor Author

/retest

@jpinsonneau jpinsonneau force-pushed the unified-perspectives branch from b4534aa to ad75014 Compare March 7, 2025 13:49
@jotak jotak changed the title NETOBSERV-2098 Unified-perspectives NETOBSERV-2098: Unified-perspectives Mar 10, 2025
@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented Mar 10, 2025

@jpinsonneau: This pull request references NETOBSERV-2098 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set.

In response to this:

Description

Add namespace bar on top of the page when the user doesn't have the admin role.
The page will act as same as in the developer perspective in that case.

For admins, the selector is not showing.

Dependencies

based on #658

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).

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 openshift-eng/jira-lifecycle-plugin repository.

@memodi
Copy link
Member

memodi commented Mar 10, 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 Mar 10, 2025
@github-actions
Copy link

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

It will expire after two weeks.

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

USER=netobserv VERSION=8875510 make set-plugin-image

@jpinsonneau jpinsonneau force-pushed the unified-perspectives branch from b5126f7 to 3aeaa32 Compare March 11, 2025 09:09
@github-actions github-actions bot removed the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Mar 11, 2025
@memodi
Copy link
Member

memodi commented Mar 11, 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 Mar 11, 2025
@github-actions
Copy link

New image:
quay.io/netobserv/network-observability-console-plugin:042225a

It will expire after two weeks.

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

USER=netobserv VERSION=042225a make set-plugin-image

@memodi
Copy link
Member

memodi commented Mar 11, 2025

Add namespace bar on top of the page when the user doesn't have the admin role.

@jpinsonneau - I am curious why bar namespace filter was added here? I know in past we used project in URL, but I am wondering if there could be no differences in views whether user is admin or developer and each user would see the flows for the namespaces they have access to 🤔

@jpinsonneau
Copy link
Contributor Author

Add namespace bar on top of the page when the user doesn't have the admin role.

@jpinsonneau - I am curious why bar namespace filter was added here? I know in past we used project in URL, but I am wondering if there could be no differences in views whether user is admin or developer and each user would see the flows for the namespaces they have access to 🤔

Ideally yes. However when querying to prometheus, endpoints are differents between admin and developers:
https://github.com/netobserv/network-observability-operator/blob/aab62278e13f3b4a3c103204397ebb291fd33229/controllers/consoleplugin/consoleplugin_objects.go#L383-L384
The developer endpoint requires a namespace to be specified to run the query. Else it's rejected. There is no way to run a single query with multiple namespaces set.

We could run multiple queries in parallel and reconcile the results in the plugin to have a seamless experience but it could be heavy depending on how many namespaces the user have access to.

@memodi
Copy link
Member

memodi commented Mar 11, 2025

The developer endpoint requires a namespace to be specified to run the query. Else it's rejected. There is no way to run a single query with multiple namespaces set.

ah, yes, thanks for refreshing my memory there.

@memodi
Copy link
Member

memodi commented Mar 11, 2025

@jpinsonneau this may be unrelated, but could we add deterministic test-ids for "Advanced Configuration" toggles in the form menu:

<button aria-controls="expandable-section-content-1741722066486ix6dk872j2" id="expandable-section-toggle-1741722066486ut8wt7386iq" class="pf-v6-c-button pf-m-link" type="button" data-ouia-component-type="PF6/Button" data-ouia-safe="true" data-ouia-component-id="OUIA-Generated-Button-link-19" aria-expanded="false"><span class="pf-v6-c-button__icon pf-m-start"><span class="pf-v6-c-expandable-section__toggle-icon"><svg class="pf-v6-svg" viewBox="0 0 256 512" fill="currentColor" aria-hidden="true" role="img" width="1em" height="1em"><path d="M224.3 273l-136 136c-9.4 9.4-24.6 9.4-33.9 0l-22.6-22.6c-9.4-9.4-9.4-24.6 0-33.9l96.4-96.4-96.4-96.4c-9.4-9.4-9.4-24.6 0-33.9L54.3 103c9.4-9.4 24.6-9.4 33.9 0l136 136c9.5 9.4 9.5 24.6.1 34z"></path></svg></span></span><span class="pf-v6-c-button__text">Advanced configuration</span></button>

@jpinsonneau
Copy link
Contributor Author

@jpinsonneau this may be unrelated, but could we add deterministic test-ids for "Advanced Configuration" toggles in the form menu:

<button aria-controls="expandable-section-content-1741722066486ix6dk872j2" id="expandable-section-toggle-1741722066486ut8wt7386iq" class="pf-v6-c-button pf-m-link" type="button" data-ouia-component-type="PF6/Button" data-ouia-safe="true" data-ouia-component-id="OUIA-Generated-Button-link-19" aria-expanded="false"><span class="pf-v6-c-button__icon pf-m-start"><span class="pf-v6-c-expandable-section__toggle-icon"><svg class="pf-v6-svg" viewBox="0 0 256 512" fill="currentColor" aria-hidden="true" role="img" width="1em" height="1em"><path d="M224.3 273l-136 136c-9.4 9.4-24.6 9.4-33.9 0l-22.6-22.6c-9.4-9.4-9.4-24.6 0-33.9l96.4-96.4-96.4-96.4c-9.4-9.4-9.4-24.6 0-33.9L54.3 103c9.4-9.4 24.6-9.4 33.9 0l136 136c9.5 9.4 9.5 24.6.1 34z"></path></svg></span></span><span class="pf-v6-c-button__text">Advanced configuration</span></button>

Sure ! is it the only one you need ?
Maybe we can open a dedicated PR and address all the ones you identified yet

@memodi
Copy link
Member

memodi commented Mar 12, 2025

Maybe we can open a dedicated PR and address all the ones you identified yet

I think there are 4 such toggles in flowcollector form, one for each plugin, processor and ebpf agent and last one for exporters.

image

feel free to add in this PR or create a separate whichever one you prefer.

@jpinsonneau
Copy link
Contributor Author

jpinsonneau commented Mar 12, 2025

Advanced Configuration

oh that's on the form view. It's not implemented on our side yet as we rely on the console implementation.
We'll need https://issues.redhat.com/browse/NETOBSERV-1942 & https://issues.redhat.com/browse/NETOBSERV-1943 first

.then(role => {
let namespace = this.state.namespace;
if (role === 'dev') {
namespace = window?.sessionStorage?.getItem('bridge/last-namespace-name') || 'default';
Copy link
Member

Choose a reason for hiding this comment

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

what's this 'bridge/last-namespace-name' ? Isn't it specific to running in dev mode with the bridge?

Copy link
Contributor Author

@jpinsonneau jpinsonneau Mar 13, 2025

Choose a reason for hiding this comment

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

@github-actions
Copy link

New image:
quay.io/netobserv/network-observability-console-plugin:8c5c793

It will expire after two weeks.

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

USER=netobserv VERSION=8c5c793 make set-plugin-image

@memodi
Copy link
Member

memodi commented Mar 28, 2025

/label qe-approved

@openshift-ci openshift-ci bot added the qe-approved QE has approved this pull request label Mar 28, 2025
@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented Mar 28, 2025

@jpinsonneau: This pull request references NETOBSERV-2098 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set.

In response to this:

Description

Add namespace bar on top of the page when the user doesn't have the admin role.
The page will act as same as in the developer perspective in that case.

For admins, the selector is not showing.

Dependencies

based on #658

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).

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 openshift-eng/jira-lifecycle-plugin repository.

@jpinsonneau jpinsonneau requested a review from jotak March 31, 2025 11:29
@jotak
Copy link
Member

jotak commented Apr 1, 2025

I'm finding a couple of issues with multi-tenancy, although I'm not sure if this is related to this PR or if it's regressions with 4.19 more generally.

  • When logging with a non-admin user and Monolith loki, I get an error mentioning this setup isn't for non-admin users. In fact I'm not even sure this is a regression - but we should be able to show the metrics in that case and not try anything with Loki
    - Still with a non-admin user, after disabling Loki, it should get data from prometheus. But it doesn't get anything. I'm wondering if that's a regression / breaking-change on the cluster-monitoring side.
    Edit: forget the 2d point .... I was misled by a message saying there was no namespace label, but it was another issue on my side ... fixed now

So I couldn't fully test this PR due to these issues....

I'll open a separate ticket for the first point

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 1, 2025
@jpinsonneau
Copy link
Contributor Author

/retest

@openshift-ci
Copy link

openshift-ci bot commented Apr 1, 2025

@jpinsonneau: The following test 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/qe-e2e-console-tests 3aeaa32 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.

@jpinsonneau
Copy link
Contributor Author

@memodi & @jotak is it safe to merge this ?

@memodi
Copy link
Member

memodi commented Apr 7, 2025

@memodi & @jotak is it safe to merge this ?

@jpinsonneau yes, fine by me.

@jpinsonneau jpinsonneau merged commit e659e3a into netobserv:main Apr 7, 2025
9 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

jira/valid-reference lgtm ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. qe-approved QE has approved this pull request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants