feat(nimbus): Grafana proxy view#15433
Conversation
yashikakhurana
requested review from
freshstrangemusic,
jaredlockhart and
mikewilli
as code owners
jaredlockhart
left a comment
jaredlockhart
left a comment
There was a problem hiding this comment.
Oh that's a clever idea! I think there's a bunch to talk through on this so let's chat about it on Monday.
|
Blocking this now as we discussed to do a security review first before adding this |
|
cleared from the security review, got approval |
…y security - Embed the Grafana monitoring iframe directly in the Feature Health Dashboard (features.html) instead of a dedicated separate page - Remove NimbusFeatureMonitoringView and feature_monitoring.html since the monitoring now lives on the feature page as originally intended - Fix security issue: GrafanaProxyView now accepts ?slug= for dashboard requests, validates it against the DB, and constructs the upstream URL internally so user input never controls the proxy target path - Asset requests (JS/CSS) continue to use path-based proxy as before
…t URL from settings Addresses security review: removes path parameter injection risk by validating slug against DB and building the Grafana URL entirely from settings constants.
yashikakhurana
force-pushed
the
iframe_grafana_test
branch
from
e39bc28 to
1017c28
Compare
…ep only TestGrafanaProxyView
|
@jaredlockhart this is ready for review now |
yashikakhurana
force-pushed
the
iframe_grafana_test
branch
from
72d590b to
4004ea5
Compare
jaredlockhart
left a comment
jaredlockhart
left a comment
There was a problem hiding this comment.
Alright let's give it a shot let's gooooo 🎉 🔥 🎉 🔥 🎉 🔥 🎉 🔥
| class="btn btn-outline-primary btn-sm"> | ||
| <i class="fa-solid fa-arrow-up-right-from-square me-1"></i> | ||
| Open in Grafana | ||
| </a> |
There was a problem hiding this comment.
Yeah exactly I think it's better to link front and centre out to external dashboards, people will miss the links if they're in the sidebar.
| def get(self, request): | ||
| slug = request.GET.get("slug") | ||
| if not slug: | ||
| return HttpResponse("Missing slug parameter", status=400) | ||
|
|
||
| feature_config = get_object_or_404(NimbusFeatureConfig, slug=slug) |
There was a problem hiding this comment.
Yeah so we can do this like this, where we just receive GET and then naively unpack the slug field and do get_object_or_404 ourselves. But you can also mix in something like the SingleObjectMixin that does all that for you, and then it'll catch like extra cases and raise additional codes where necessary etc, it just does a lot of the plumbing for you. This is totally fine to just leave as is I just wanted to highlight ways that Django gives you pieces that does things like this for you that you can easily drop in.
| "timezone": "utc", | ||
| "var-application": application, | ||
| "var-feature": feature_config.slug, | ||
| "kiosk": "", |
There was a problem hiding this comment.
Are we gonna have to pass through the light/dark mode theme? 🤔
| </div> | ||
| <div class="card shadow-sm border-0 rounded-3 mb-3"> | ||
| <div class="card-body p-0"> | ||
| <iframe src="/nimbus/grafana-proxy/?slug={{ selected_feature_config.slug }}" |
There was a problem hiding this comment.
So yeah let's land this as a prototype just to show that the graph renders etc. But next we'll probably have to
- Discover the list of metrics for a feature (which we can discover by pulling in teh feature metric config files that are in metric hub now, they might already even be pulled in via the config update? 🤔 )
- Loop over them and create one iframe per metric
- Pass that metric in through the url
But we an do that as a followup
Or if you can 'select all' by default so we get all the metrics for the feature automatically try that 🙏
…n default time range to 30d
2 participants
Because
-Grafana is behind Google IAP, which sets session cookies on
yardstick.mozilla.org.Browsers block these as third-party cookies when the dashboard is embedded in an iframe on a different origin, causing it to fail to load.This commit
GRAFANA_INTERNAL_URLandGRAFANA_SERVICE_ACCOUNT_TOKENsettings for the internal endpoint and service account authUpdates the feature monitoring iframe to use
/nimbus/grafana-proxy/instead of the direct Grafana URLFixes #15342
https://github.com/mozilla/webservices-infra/pull/10728