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

Enable deletion of public snapshot #14109

Closed
DanCech opened this Issue Nov 16, 2018 · 4 comments

Comments

Projects
None yet
4 participants
@DanCech
Copy link
Member

DanCech commented Nov 16, 2018

Currently the way that Grafana handles external snapshots has a few problems that make it difficult to manage external snapshots, as they do not show up in the snapshot list and cannot easily be deleted.

Firstly, when external snapshots are recorded in the local database, they have the OrgId & UserId set to -1 instead of the correct values, which means that they will never show up in the snapshot lists. These should be set to the correct values, and the ExternalUrl should be set to the correct setting so that it can be used later to assemble the correct links to view and delete the snapshot.

The snapshot_list.html partial should be updated to handle external snapshots by indicating that they are external and providing the correct link to view the snapshot.

When deleting a snapshot, the delete controls offered on the snapshot list and on the confirmation screen of the share modal should both call the local API to remove the snapshot from the local database.

The biggest question in my mind is whether the request to the external snapshot server should come from the Grafana server or be done from the front-end. Currently at creation time the browser makes the request to the external server directly, then makes a separate request to the local API to register the external snapshot. The advantage of doing things that way is that it's possible to publish snapshots to external servers that require browser authentication, which wouldn't be possible if the request was made by the Grafana server.

The problem arises with handling deletion, in order to delete directly from the external snapshot server the client would need the deleteKey, which isn't exposed in the list because it can be used to delete the snapshot without any permission checks.

The cleanest way to handle this would be for the user to make a request directly to the local API, which would then make the delete request to the external snapshot server (using the deleteKey stored in the local database) before deleting the local record.

An alternative would be to provide a dedicated endpoint that the frontend could use to get the deleteKey which would perform the permission check before returning it. The frontend could then make the request directly to the external server to delete the snapshot before calling the local API to remove the external snapshot record.

At this point I'm tending toward just having the Grafana server make the request to the external snapshot server in both create and delete cases, as it makes things much simpler than having to coordinate via the frontend, and would also supporting deleting external snapshots via the API without having to make 1 call to get the deleteKey, a second to delete from the external server, then a 3rd to delete the record from the local server. This would also mean that when the local snapshot record for external snapshots is expired, Grafana would be able to ensure that it has been removed from the external server.

If we went that way, at creation time the frontend would no longer have to deal with the external server and could just call the local API specifying external=true and letting it take care of making the request to the external server, then creating the corresponding local record.

The biggest advantage of these changes would be that a user who creates an external snapshot would be able to delete it from within their Grafana instance, rather than having to ask the maintainer of the external snapshot server to remove it for them, and an administrator could easily see which external snapshots have been created, etc.

It's also worth noting that currently the internal record for an external snapshot includes the complete snapshot data, which could be a problem for instances with a lot of external snapshots that never expire as they will never be removed from the local database. It may be worth considering not storing the snapshot data for external snapshots in the local database to save space.

@bergquist

This comment has been minimized.

Copy link
Contributor

bergquist commented Nov 19, 2018

👍 Would prefer if this was solved in smaller independent pull requests.

@cinaglia

This comment has been minimized.

Copy link
Contributor

cinaglia commented Nov 20, 2018

This would also mean that when the local snapshot record for external snapshots is expired, Grafana would be able to ensure that it has been removed from the external server.

I'm wondering how important this is given that the remote server itself is also periodically running the cleanup script.

The advantage of doing things that way is that it's possible to publish snapshots to external servers that require browser authentication, which wouldn't be possible if the request was made by the Grafana server.

I'm also of the opinion that it makes the most sense to have the local Grafana server make the requests. However, do you know if this browser authentication behavior is something a lot of users rely upon? I'm not familiar enough with the codebase. Is this done via SSO?

@DanCech

This comment has been minimized.

Copy link
Member Author

DanCech commented Nov 20, 2018

This would also mean that when the local snapshot record for external snapshots is expired, Grafana would be able to ensure that it has been removed from the external server.

I'm wondering how important this is given that the remote server itself is also periodically running the cleanup script.

Yes, in theory at least the remote server should have removed it.

The advantage of doing things that way is that it's possible to publish snapshots to external servers that require browser authentication, which wouldn't be possible if the request was made by the Grafana server.

I'm also of the opinion that it makes the most sense to have the local Grafana server make the requests. However, do you know if this browser authentication behavior is something a lot of users rely upon? I'm not familiar enough with the codebase. Is this done via SSO?

Right now the publish request is made directly from the user's browser, so it's more of a theoretical possibility that a snapshot server might have some sort of authentication like SSO. In practice I doubt that it would be an issue as those cases would be able to just use local snapshots.

@bergquist

This comment has been minimized.

Copy link
Contributor

bergquist commented Dec 18, 2018

Closed by #14441

@bergquist bergquist closed this Dec 18, 2018

@bergquist bergquist changed the title Improve External Snapshots Enable deletion of public snapshot Dec 18, 2018

bergquist added a commit that referenced this issue Dec 18, 2018

ryantxu added a commit to NatelEnergy/grafana that referenced this issue Dec 19, 2018

Merge remote-tracking branch 'grafana/master'
* grafana/master:
  changelog: adds note about closing grafana#14109
  snapshots: Close response body after error check
  snapshots: Add support for deleting external snapshots
  snapshots: Move external snapshot creation to backend
  snapshots: Add external_delete_url column

ryantxu added a commit to ryantxu/grafana that referenced this issue Dec 19, 2018

Merge remote-tracking branch 'grafana/master'
* grafana/master: (41 commits)
  Fixes undefined issue with angular panels and editorTabs
  changelog: adds note about closing grafana#14562
  Update field name
  Add documentation
  Rename the setting and add description
  export init notifier func
  Increase recent and starred limit in search and home dashboard, closes grafana#13950
  changelog: adds note about closing grafana#14486
  Panel help view fixes
  Add min/max height when resizing and replace debounce with throttle
  changelog: adds note about closing grafana#14546
  Adding tests for auth proxy CIDR support
  changelog: adds note about closing grafana#14109
  fix signed in user for orgId=0 result should return active org id
  Another take on resizing the panel, now using react-draggable
  Raise datasources number to 5000
  copy props to state to make it visible in the view
  refactor to not crash when no links
  updating snaps
  renaming component
  ...

ryantxu added a commit to ryantxu/grafana that referenced this issue Dec 19, 2018

Merge remote-tracking branch 'grafana/master' into timepicker-tooltip
* grafana/master: (2079 commits)
  Fixes undefined issue with angular panels and editorTabs
  changelog: adds note about closing grafana#14562
  Update field name
  Add documentation
  Rename the setting and add description
  export init notifier func
  Increase recent and starred limit in search and home dashboard, closes grafana#13950
  changelog: adds note about closing grafana#14486
  Panel help view fixes
  Add min/max height when resizing and replace debounce with throttle
  changelog: adds note about closing grafana#14546
  Adding tests for auth proxy CIDR support
  changelog: adds note about closing grafana#14109
  fix signed in user for orgId=0 result should return active org id
  Another take on resizing the panel, now using react-draggable
  Raise datasources number to 5000
  copy props to state to make it visible in the view
  refactor to not crash when no links
  updating snaps
  renaming component
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment