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

mgr/dashboard: display helpfull message when the iframe-embedded Grafana dashboard failed to load #45012

Merged
merged 1 commit into from
Jun 20, 2022

Conversation

nSedrickm
Copy link
Contributor

@nSedrickm nSedrickm commented Feb 14, 2022

This commit checks if there is an error while loading a grafana panel and informs the user

Fixes: https://tracker.ceph.com/issues/54206
Signed-off-by: Ngwa Sedrick Meh nsedrick101@gmail.com

Checklist

  • Tracker (select at least one)
    • References tracker ticket
    • Very recent bug; references commit where it was introduced
    • New feature (ticket optional)
    • Doc update (no ticket needed)
    • Code cleanup (no ticket needed)
  • Component impact
    • Affects Dashboard, opened tracker ticket
    • Affects Orchestrator, opened tracker ticket
    • No impact that needs to be tracked
  • Documentation (select at least one)
    • Updates relevant documentation
    • No doc update is appropriate
  • Tests (select at least one)
Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test dashboard cephadm
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox

@nSedrickm nSedrickm requested a review from a team as a code owner February 14, 2022 05:50
@nSedrickm nSedrickm requested review from alfonsomthd and sunilangadi2 and removed request for a team February 14, 2022 05:50
Copy link
Contributor

@avanthakkar avanthakkar left a comment

Choose a reason for hiding this comment

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

@nSedrickm Can you add screenshot for the message shown and also unit-test for checking 500 error msg? Thanks!

@github-actions github-actions bot added this to In progress in Dashboard Feb 17, 2022
@nSedrickm
Copy link
Contributor Author

Hello @avanthakkar here's a screenshot

image

The test is done in the grafana components getFrame method

@nSedrickm nSedrickm force-pushed the grafana-iframe-ux-update branch 2 times, most recently from 09956fc to 17e28b3 Compare February 21, 2022 04:03
@nSedrickm
Copy link
Contributor Author

jenkins retest this please

@epuertat
Copy link
Member

@nSedrickm have you tested if the error warning badge disappears when the user accepts the invalid certificate? At least in my case it didn't work and the reason is that what the validateGrafanaDashboardUrl() service does is launching a connection to Grafana... from the Dashboard back-end:

image

There you see 2 simultaneous failures: the first one is a 500 coming form the Dashboard back-end when trying to access Grafana from the backend, and the second one is from the front-end, when the browser tries to embed Grafana inside the iframe.

That's basically because you accept the Grafana self-signed certificate in your browser, but the Dashboard back-end doesn't know anything about browsers. It uses the python-requests package for the HTTP connections, and in fact you'll see this traceback in the ceph-mgr logs:

Traceback (most recent call last):
...
  File "/ceph/src/pybind/mgr/dashboard/controllers/grafana.py", line 39, in validation
    response = grafana.url_validation(method, url)
  File "/ceph/src/pybind/mgr/dashboard/grafana.py", line 23, in url_validation
    verify=Settings.GRAFANA_API_SSL_VERIFY)
  File "/usr/lib/python3.6/site-packages/requests/api.py", line 60, in request
    return session.request(method=method, url=url, **kwargs)
  File "/usr/lib/python3.6/site-packages/requests/sessions.py", line 533, in request
    resp = self.send(prep, **send_kwargs)
  File "/usr/lib/python3.6/site-packages/requests/sessions.py", line 646, in send
    r = adapter.send(request, **kwargs)
  File "/usr/lib/python3.6/site-packages/requests/adapters.py", line 514, in send
    raise SSLError(e, request=request)
requests.exceptions.SSLError: HTTPSConnectionPool(host='172.20.0.6', port=3000): Max retries exceeded with url: /api/dashboards/uid/z99hzWtmk (Caused by SSLError(SSLError(1, '[SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed (_ssl.c:897)'),))

Therefore you have to modify that part of the back-end code to launch a request(..., verify=False). In fact, that part of the code is already fixed, and by setting ceph dashboard set-grafana-api-ssl-verify false will do exactly this.

So the problem is still there! Lesson learnt: you cannot rely on the back-end for this. You have to do your magic with just the front-end.

Let's explore the alternatives:

  • I tested the onload and onerror and they don't seem to work.
  • An srcdoc="The embedded Grafana Dashboard was blocked due to a certificate error, please click {{baseUrl}}" field works (it's not as fancy as the banner you created, but still works):
    image
  • Exploring existing Angular packages that do iframe management for you?

@nSedrickm
Copy link
Contributor Author

@epuertat thanks for the review. I noticed the backend still throws even if the user has accepted the certificate but wasn't sure if it was ok to edit the backend code. I will make changes focusing on the front-end only

@nSedrickm nSedrickm force-pushed the grafana-iframe-ux-update branch 2 times, most recently from bb1adf8 to 749c26b Compare March 2, 2022 03:23
@nSedrickm
Copy link
Contributor Author

@epuertat warning badge now disappears after user accepts certificate and reloads page. I found the onload event fires when the iframe successfully loads even with the server exception which helped. I tried using the srcdoc attribute but the message just replaced the dashboard after the user accepts the certificate

Copy link
Member

@nizamial09 nizamial09 left a comment

Choose a reason for hiding this comment

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

Sorry for the late review here @nSedrickm

When we show the new error information, can we hide the Grafana Timepicker and the iframe thing so that only the error is there and user don't have to worry about the blank frame below?
Screenshot from 2022-03-15 13-29-53

Also, once we accept the privacy concern we don't need to show the error again. Right now, even if we accept the privacy the message appears. So could you hide it when the user accepts that privacy message?

Screenshot from 2022-03-15 13-31-58

Dashboard automation moved this from In progress to Review in progress Mar 15, 2022
@nizamial09
Copy link
Member

I just noticed that its working good in firefox but when you try with chrome something is getting messed up. I am not sure if its a local issue of mine so an additional local testing would be good here. @ceph/dashboard

@nSedrickm
Copy link
Contributor Author

Hi @nizamial09, thanks for the review. I will test some more with chrome

@epuertat
Copy link
Member

@nSedrickm this is still the old approach, right? Can you plz update the PR with the front-end only check? Thanks!

@nSedrickm
Copy link
Contributor Author

Hi! @epuertat, yes, this is the old approach. I only made changes in the grafana html and ts file

@djgalloway djgalloway changed the base branch from master to main May 25, 2022 20:03
@github-actions
Copy link

github-actions bot commented Jun 3, 2022

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

@nSedrickm
Copy link
Contributor Author

Screenshots taken from chrome

Alert hidden
image

Alert displayed
image

Copy link
Member

@epuertat epuertat left a comment

Choose a reason for hiding this comment

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

LGTM. Some suggestions over there.

…when the iframe-embedded Grafana dashboard failed to load

This commit adds checks for successful grafana panel loads before displaying dashboards and informs the user if the request is blocked by the browser
Fixes: https://tracker.ceph.com/issues/54206
Signed-off-by: Ngwa Sedrick Meh <nsedrick101@gmail.com>
@epuertat
Copy link
Member

epuertat commented Jun 7, 2022

jenkins test api

Dashboard automation moved this from Review in progress to Reviewer approved Jun 17, 2022
Copy link

@sunilangadi2 sunilangadi2 left a comment

Choose a reason for hiding this comment

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

LGTM.

@ljflores
Copy link
Contributor

I am running some teuthology tests for this branch here:
http://pulpito.front.sepia.ceph.com/lflores-2022-06-17_20:48:59-rados:dashboard-grafana-iframe-ux-update-distro-default-smithi/

@nSedrickm you may not be able to access this link if you don't have access to the Sepia VPN, but I will summarize the results here once the tests finish running.

@nSedrickm
Copy link
Contributor Author

Hi @ljflores thanks, I can't access the link

@epuertat epuertat moved this from Reviewer approved to Ready-to-merge in Dashboard Jun 20, 2022
@epuertat epuertat merged commit e47dfb0 into ceph:main Jun 20, 2022
Dashboard automation moved this from Ready-to-merge to Done Jun 20, 2022
@ljflores
Copy link
Contributor

@nSedrickm you'd need access to the VPN. No worries though, the results were all good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Dashboard
  
Done
8 participants