Bug 1842875: Remove the checks CAN_GET_NS and prometheusBaseURL for monitoring Dashboard and Metrics tabs in devconsole #5661

vikram-raj · 2020-06-03T13:27:26Z

Fixes:
https://issues.redhat.com/browse/ODC-4082

Analysis / Root cause:
Basic user redirects to a blank page after clicking on the metrics chart on utilization card or topology metrics chart.

Solution Description:

Checks for only window.SERVER_FLAGS.prometheusBaseURL in devconsole to hide the monitoring dashboard and metrics.
Remove the link from the metrics chart if monitoring is not enabled.

Browser conformance:

Chrome
Firefox
Safari
Edge

vikram-raj · 2020-06-03T17:27:58Z

/cc @kyoto @christianvogt

spadgett · 2020-06-03T18:07:57Z

frontend/public/components/graphs/prometheus-graph.tsx

-    canAccessMonitoring && perspective === 'admin'
+    perspective === 'admin'


Is this right? Can't normal users see monitoring for workloads in their own namespace even if they don't have the get namespace permission?

I thought that if canAccessNS === false we cannot redirect the user to /monitoring otherwise they get a bunch of graphs with Forbidden errors.

My understanding is that the admin query browser uses prometheusBaseURL, which will return errors for a normal user. Dev console uses prometheusTenancyBaseURL, which will work for a normal user. That's why we're linking to the developer view when the user doesn't have get namespace permission.

Regardless, I wouldn't expect to ever have a blank page. Is there a runtime error?

It looks like we're reverting fix #3333 for https://bugzilla.redhat.com/show_bug.cgi?id=1770808

Updated the PR. Now, wrapping the chart with PrometheusGraphLink only if prometheusBaseURL is present.

As @spadgett mentioned, the dev console uses prometheusTenancyBaseURL rather than prometheusBaseURL.

So I think the logic should be like.

if (perspective === 'admin' && canAccessNS && window.SERVER_FLAGS.prometheusBaseURL) { url = `/monitoring/query-browser?${params.toString()}`; } else if (window.SERVER_FLAGS.prometheusTenancyBaseURL) { url = `/dev-monitoring/ns/${namespace}/metrics?${params.toString()}`; }

Actually, if the canAccess check is being removed from PageContents, I think we can leave PrometheusGraphLink unchanged.

I added check for prometheusTenancyBaseURL on Link so that if prometheusTenancyBaseURL is not present then we should not redirect the user to metrics page after clicking on the chart.

@vikram-raj Adding the check isn't bad, but prometheusBaseURL and prometheusTenancyBaseURL will always be set in an OpenShift cluster. The only times those aren't set are when running against native k8s or running off-cluster, which is for development only. So it's unclear to me what bug that fixes.

spadgett · 2020-06-03T18:09:38Z

frontend/public/components/graphs/prometheus-graph.tsx

@@ -39,19 +38,22 @@ export const PrometheusGraphLink_: React.FC<PrometheusGraphLinkProps> = ({
  if (!query) {
    return <>{children}</>;
  }
-
+  const prometheusBaseURLPresent = !!window.SERVER_FLAGS.prometheusBaseURL;
+  const canAccessMonitoring = (canAccessNS && prometheusBaseURLPresent) || prometheusBaseURLPresent;


With the ||, isn't this logically the same as...?

Suggested change

const canAccessMonitoring = (canAccessNS && prometheusBaseURLPresent) || prometheusBaseURLPresent;

const canAccessMonitoring = prometheusBaseURLPresent;

I'm unclear on the intent here.

kyoto · 2020-06-04T05:07:03Z

frontend/packages/dev-console/src/components/monitoring/MonitoringPage.tsx

  const activeNamespace = match.params.ns;
-  const canAccessPrometheus = canAccess && !!window.SERVER_FLAGS.prometheusBaseURL;
-
+  const canAccessPrometheus = !!window.SERVER_FLAGS.prometheusBaseURL;


Should this be prometheusTenancyBaseURL?

window.SERVER_FLAGS.prometheusBaseURL / window.SERVER_FLAGS.prometheusTenancyBaseURL is constant, so there should be no need for the React.useMemo below now.

Maybe it's easier to move the definition of pages outside the component now?

Updated it.

spadgett · 2020-06-04T13:46:41Z

@vikram-raj Can you describe exactly the bug you're seeing? The JIRA issue, Bugzilla, and PR description are vague, so I'm not sure what we're fixing.

Adding the checks for the Prometheus URL will not have an effects for an in-cluster console. Are you seeing the problem only in your development environment?

vikram-raj · 2020-06-04T14:24:29Z

@vikram-raj Can you describe exactly the bug you're seeing? The JIRA issue, Bugzilla, and PR description are vague, so I'm not sure what we're fixing.

Adding the checks for the Prometheus URL will not have an effects for an in-cluster console. Are you seeing the problem only in your development environment?

Currently, there is an issue for basic users. As we have checks for CAN_GET_NS and !!window.SERVER_FLAGS.prometheusBaseURL to hide the monitoring Dashboard and Metrics tabs. So, as basic-users has no access to CAN_GET_NS user navigate to a blank page when the user clicks on charts in Utilization card.

spadgett · 2020-06-04T14:44:36Z

Currently, there is an issue for basic users. As we have checks for CAN_GET_NS and !!window.SERVER_FLAGS.prometheusBaseURL to hide the monitoring Dashboard and Metrics tabs. So, as basic-users has no access to CAN_GET_NS user navigate to a blank page when the user clicks on charts in Utilization card.

This page should work for basic users. You don't need CAN_GET_NS permission for the dev console metrics view. Instead of disabling the link, we should figure out why the page is broken.

When you say blank page, do you mean a completely white screen? What exactly is the error? Is there a stack trace in the JS console?

Does this happen only in a development environment or can you reproduce with an in-cluster console?

vikram-raj · 2020-06-04T15:15:57Z

This page should work for basic users. You don't need CAN_GET_NS permission for the dev console metrics view. Instead of disabling the link, we should figure out why the page is broken.

Yes, this PR removes the check CAN_GET_NS. which is being there to hide the dashboard and metrics page.

When you say blank page, do you mean a completely white screen? What exactly is the error? Is there a stack trace in the JS console?

Not completely white screen. and there is no runtime error.

Does this happen only in a development environment or can you reproduce with an in-cluster console?

It happens in both the environment development as well as in cluster.

This is the scenario

vikram-raj · 2020-06-09T10:59:00Z

/retitle Bug 1842875: Hide metrics section from topology sidepanel if user has no access to monitoring

openshift-ci-robot · 2020-06-09T10:59:17Z

@vikram-raj: This pull request references Bugzilla bug 1842875, which is invalid:

expected the bug to target the "4.6.0" release, but it targets "4.5.0" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 1842875: Hide metrics section from topology sidepanel if user has no access to monitoring

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/test-infra repository.

christianvogt · 2020-06-09T20:34:36Z

frontend/packages/dev-console/src/components/monitoring/overview/MonitoringOverview.tsx

@@ -33,7 +33,7 @@ type MonitoringOverviewProps = {
 const MonitoringOverview: React.FC<MonitoringOverviewProps> = (props) => {
  const { resource, pods, resourceEvents } = props;
  const [expanded, setExpanded] = React.useState(['metrics']);
-
+  const canAccessMonitoring = !!window.SERVER_FLAGS.prometheusBaseURL;


Should this be window.SERVER_FLAGS. prometheusTenancyBaseURL ?

I don't see what the changes in this file are achieving. Could you please explain the intention here?

Undo the changes. As prometheusTenancyBaseURL is always available I removed the checks for it that I added.

kyoto · 2020-06-10T01:35:24Z

...ackages/dev-console/src/components/monitoring/overview/__tests__/MonitoringOverview.spec.tsx

@@ -52,4 +56,10 @@ describe('Monitoring Metric Section', () => {
    const component = shallow(<MonitoringOverview {...monitoringOverviewProps} />);
    expect(component.find(Badge).props().children).toBe(2);
  });
+
+  it('should not show metrics accordian if monitoring si disable', () => {


typo: si -> is

kyoto · 2020-06-10T01:38:08Z

frontend/public/components/graphs/prometheus-graph.tsx

@@ -42,16 +42,18 @@ export const PrometheusGraphLink_: React.FC<PrometheusGraphLinkProps> = ({

  const params = new URLSearchParams();
  params.set('query0', query);
-
+  const prometheusTenancyBaseURLPresent = !!window.SERVER_FLAGS.prometheusTenancyBaseURL;


Per @spadgett's #5661 (comment), this would always be true. So I don't think we should be making any change in this file.

…hboard and Metrics tabs in devconsole

vikram-raj · 2020-06-10T02:38:12Z

/bugzilla refresh

openshift-ci-robot · 2020-06-10T02:38:18Z

@vikram-raj: This pull request references Bugzilla bug 1842875, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.6.0) matches configured target release for branch (4.6.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

/bugzilla refresh

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/test-infra repository.

christianvogt · 2020-06-10T13:55:38Z

Looks to me like we are simply reverting #5602 which introduced these checks for a usecase that's considered invalid because monitoring will always be available for the dev user?

vikram-raj · 2020-06-10T14:41:12Z

Looks to me like we are simply reverting #5602 which introduced these checks for a usecase that's considered invalid because monitoring will always be available for the dev user?

Yes Christian, this PR removing the checks that have been added for hiding the Dashboard and Metrics tabs in this PR #5602.

christianvogt · 2020-06-10T16:39:55Z

/lgtm
/approve

openshift-ci-robot · 2020-06-10T16:40:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, vikram-raj

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~frontend/packages/dev-console/OWNERS~~ [christianvogt]

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

openshift-ci-robot · 2020-06-10T18:33:32Z

@vikram-raj: All pull requests linked via external trackers have merged: openshift/console#5661. Bugzilla bug 1842875 has been moved to the MODIFIED state.

In response to this:

Bug 1842875: Remove the checks CAN_GET_NS and prometheusBaseURL for monitoring Dashboard and Metrics tabs in devconsole

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/test-infra repository.

vikram-raj · 2020-06-11T14:16:43Z

/cherry-pick release-4.5

openshift-cherrypick-robot · 2020-06-11T14:16:51Z

@vikram-raj: new pull request created: #5725

In response to this:

/cherry-pick release-4.5

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/test-infra repository.

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. component/core Related to console core functionality labels Jun 3, 2020

openshift-ci-robot requested review from dtaylor113 and sahil143 June 3, 2020 13:27

openshift-ci-robot added the component/dev-console Related to dev-console label Jun 3, 2020

vikram-raj force-pushed the odc-4082 branch from 3bbe938 to 5787cc9 Compare June 3, 2020 17:19

vikram-raj changed the title ~~[WIP]: hide metrics section from topology sidepanel if user has no access to monitoring~~ Hide metrics section from topology sidepanel if user has no access to monitoring Jun 3, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 3, 2020

openshift-ci-robot requested review from christianvogt and kyoto June 3, 2020 17:27

spadgett reviewed Jun 3, 2020

View reviewed changes

vikram-raj force-pushed the odc-4082 branch from 5787cc9 to 177be67 Compare June 4, 2020 04:11

vikram-raj requested a review from spadgett June 4, 2020 04:24

kyoto reviewed Jun 4, 2020

View reviewed changes

vikram-raj force-pushed the odc-4082 branch 3 times, most recently from 6742d18 to 3136392 Compare June 4, 2020 07:19

openshift-ci-robot changed the title ~~Hide metrics section from topology sidepanel if user has no access to monitoring~~ Bug 1842875: Hide metrics section from topology sidepanel if user has no access to monitoring Jun 9, 2020

openshift-ci-robot added bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Jun 9, 2020

christianvogt reviewed Jun 9, 2020

View reviewed changes

vikram-raj force-pushed the odc-4082 branch from 3136392 to a7ab364 Compare June 10, 2020 01:23

vikram-raj requested a review from christianvogt June 10, 2020 01:23

kyoto reviewed Jun 10, 2020

View reviewed changes

vikram-raj force-pushed the odc-4082 branch 2 times, most recently from 25e5ef4 to ef55f0a Compare June 10, 2020 02:01

vikram-raj requested a review from kyoto June 10, 2020 02:01

vikram-raj force-pushed the odc-4082 branch 2 times, most recently from a693d3a to 22cf7af Compare June 10, 2020 02:32

Remove the checks CAN_GET_NS and prometheusBaseURL for monitoring Das…

29740cd

…hboard and Metrics tabs in devconsole

vikram-raj force-pushed the odc-4082 branch from 22cf7af to 29740cd Compare June 10, 2020 02:37

vikram-raj changed the title ~~Bug 1842875: Hide metrics section from topology sidepanel if user has no access to monitoring~~ Bug 1842875: Remove the checks CAN_GET_NS and prometheusBaseURL for monitoring Dashboard and Metrics tabs in devconsole Jun 10, 2020

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Jun 10, 2020

openshift-ci-robot removed the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Jun 10, 2020

openshift-ci-robot assigned christianvogt Jun 10, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 10, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 10, 2020

openshift-merge-robot merged commit af0c33a into openshift:master Jun 10, 2020

vikram-raj deleted the odc-4082 branch June 10, 2020 18:42

openshift-cherrypick-robot mentioned this pull request Jun 11, 2020

[release-4.5.z] Bug 1846409: Remove the checks CAN_GET_NS and prometheusBaseURL for monitoring Dashboard and Metrics tabs in devconsole #5725

Merged

spadgett added this to the v4.6 milestone Jun 15, 2020

		canAccessMonitoring && perspective === 'admin'
		perspective === 'admin'

	const canAccessMonitoring = (canAccessNS && prometheusBaseURLPresent) \|\| prometheusBaseURLPresent;
	const canAccessMonitoring = prometheusBaseURLPresent;

Bug 1842875: Remove the checks CAN_GET_NS and prometheusBaseURL for monitoring Dashboard and Metrics tabs in devconsole #5661

Bug 1842875: Remove the checks CAN_GET_NS and prometheusBaseURL for monitoring Dashboard and Metrics tabs in devconsole #5661

Conversation

vikram-raj commented Jun 3, 2020 • edited

vikram-raj commented Jun 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spadgett Jun 3, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyoto Jun 4, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spadgett commented Jun 4, 2020

vikram-raj commented Jun 4, 2020

spadgett commented Jun 4, 2020 • edited

vikram-raj commented Jun 4, 2020

vikram-raj commented Jun 9, 2020

openshift-ci-robot commented Jun 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vikram-raj commented Jun 10, 2020

openshift-ci-robot commented Jun 10, 2020

christianvogt commented Jun 10, 2020

vikram-raj commented Jun 10, 2020

christianvogt commented Jun 10, 2020

openshift-ci-robot commented Jun 10, 2020

openshift-ci-robot commented Jun 10, 2020

vikram-raj commented Jun 11, 2020

openshift-cherrypick-robot commented Jun 11, 2020

vikram-raj commented Jun 3, 2020 •

edited

spadgett Jun 3, 2020 •

edited

kyoto Jun 4, 2020 •

edited

spadgett commented Jun 4, 2020 •

edited