From d9d6a4481fa7b27046972ee666390a06e21ca297 Mon Sep 17 00:00:00 2001 From: Victor Cinaglia Date: Mon, 10 Dec 2018 16:25:02 -0500 Subject: [PATCH 1/4] snapshots: Add external_delete_url column --- pkg/services/sqlstore/migrations/dashboard_snapshot_mig.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pkg/services/sqlstore/migrations/dashboard_snapshot_mig.go b/pkg/services/sqlstore/migrations/dashboard_snapshot_mig.go index b880497cd23..be0bc80134c 100644 --- a/pkg/services/sqlstore/migrations/dashboard_snapshot_mig.go +++ b/pkg/services/sqlstore/migrations/dashboard_snapshot_mig.go @@ -60,4 +60,8 @@ func addDashboardSnapshotMigrations(mg *Migrator) { {Name: "external_url", Type: DB_NVarchar, Length: 255, Nullable: false}, {Name: "dashboard", Type: DB_MediumText, Nullable: false}, })) + + mg.AddMigration("Add column external_delete_url to dashboard_snapshots table", NewAddColumnMigration(snapshotV5, &Column{ + Name: "external_delete_url", Type: DB_NVarchar, Length: 255, Nullable: true, + })) } From 9d6da10e82ff254c8877f8ea2405934493b09541 Mon Sep 17 00:00:00 2001 From: Victor Cinaglia Date: Mon, 10 Dec 2018 16:36:32 -0500 Subject: [PATCH 2/4] snapshots: Move external snapshot creation to backend --- pkg/api/dashboard_snapshot.go | 82 +++++++++++++++++-- pkg/models/dashboard_snapshot.go | 22 +++-- pkg/services/sqlstore/dashboard_snapshot.go | 22 ++--- .../features/dashboard/share_snapshot_ctrl.ts | 33 +------- 4 files changed, 103 insertions(+), 56 deletions(-) diff --git a/pkg/api/dashboard_snapshot.go b/pkg/api/dashboard_snapshot.go index e4e9c9d040f..6c3ee7b69c6 100644 --- a/pkg/api/dashboard_snapshot.go +++ b/pkg/api/dashboard_snapshot.go @@ -1,10 +1,15 @@ package api import ( + "bytes" + "encoding/json" + "fmt" + "net/http" "time" "github.com/grafana/grafana/pkg/api/dtos" "github.com/grafana/grafana/pkg/bus" + "github.com/grafana/grafana/pkg/components/simplejson" "github.com/grafana/grafana/pkg/metrics" m "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/guardian" @@ -12,6 +17,11 @@ import ( "github.com/grafana/grafana/pkg/util" ) +var client = &http.Client{ + Timeout: time.Second * 5, + Transport: &http.Transport{Proxy: http.ProxyFromEnvironment}, +} + func GetSharingOptions(c *m.ReqContext) { c.JSON(200, util.DynMap{ "externalSnapshotURL": setting.ExternalSnapshotUrl, @@ -20,26 +30,82 @@ func GetSharingOptions(c *m.ReqContext) { }) } +type CreateExternalSnapshotResponse struct { + Key string `json:"key"` + DeleteKey string `json:"deleteKey"` + Url string `json:"url"` + DeleteUrl string `json:"deleteUrl"` +} + +func createExternalDashboardSnapshot(cmd m.CreateDashboardSnapshotCommand) (*CreateExternalSnapshotResponse, error) { + var createSnapshotResponse CreateExternalSnapshotResponse + message := map[string]interface{}{ + "name": cmd.Name, + "expires": cmd.Expires, + "dashboard": cmd.Dashboard, + } + + messageBytes, err := simplejson.NewFromAny(message).Encode() + if err != nil { + return nil, err + } + + response, err := client.Post(setting.ExternalSnapshotUrl+"/api/snapshots", "application/json", bytes.NewBuffer(messageBytes)) + if response != nil { + defer response.Body.Close() + } + + if err != nil { + return nil, err + } + + if response.StatusCode != 200 { + return nil, fmt.Errorf("Create external snapshot response status code %d", response.StatusCode) + } + + if err := json.NewDecoder(response.Body).Decode(&createSnapshotResponse); err != nil { + return nil, err + } + + return &createSnapshotResponse, nil +} + +// POST /api/snapshots func CreateDashboardSnapshot(c *m.ReqContext, cmd m.CreateDashboardSnapshotCommand) { if cmd.Name == "" { cmd.Name = "Unnamed snapshot" } + var url string + cmd.ExternalUrl = "" + cmd.OrgId = c.OrgId + cmd.UserId = c.UserId + if cmd.External { - // external snapshot ref requires key and delete key - if cmd.Key == "" || cmd.DeleteKey == "" { - c.JsonApiErr(400, "Missing key and delete key for external snapshot", nil) + if !setting.ExternalEnabled { + c.JsonApiErr(403, "External dashboard creation is disabled", nil) return } - cmd.OrgId = -1 - cmd.UserId = -1 + response, err := createExternalDashboardSnapshot(cmd) + if err != nil { + c.JsonApiErr(500, "Failed to create external snaphost", err) + return + } + + url = response.Url + cmd.Key = response.Key + cmd.DeleteKey = response.DeleteKey + cmd.ExternalUrl = response.Url + cmd.ExternalDeleteUrl = response.DeleteUrl + cmd.Dashboard = simplejson.New() + metrics.M_Api_Dashboard_Snapshot_External.Inc() } else { cmd.Key = util.GetRandomString(32) cmd.DeleteKey = util.GetRandomString(32) - cmd.OrgId = c.OrgId - cmd.UserId = c.UserId + url = setting.ToAbsUrl("dashboard/snapshot/" + cmd.Key) + metrics.M_Api_Dashboard_Snapshot_Create.Inc() } @@ -51,7 +117,7 @@ func CreateDashboardSnapshot(c *m.ReqContext, cmd m.CreateDashboardSnapshotComma c.JSON(200, util.DynMap{ "key": cmd.Key, "deleteKey": cmd.DeleteKey, - "url": setting.ToAbsUrl("dashboard/snapshot/" + cmd.Key), + "url": url, "deleteUrl": setting.ToAbsUrl("api/snapshots-delete/" + cmd.DeleteKey), }) } diff --git a/pkg/models/dashboard_snapshot.go b/pkg/models/dashboard_snapshot.go index 3024ba94122..e8db0372758 100644 --- a/pkg/models/dashboard_snapshot.go +++ b/pkg/models/dashboard_snapshot.go @@ -8,14 +8,15 @@ import ( // DashboardSnapshot model type DashboardSnapshot struct { - Id int64 - Name string - Key string - DeleteKey string - OrgId int64 - UserId int64 - External bool - ExternalUrl string + Id int64 + Name string + Key string + DeleteKey string + OrgId int64 + UserId int64 + External bool + ExternalUrl string + ExternalDeleteUrl string Expires time.Time Created time.Time @@ -48,7 +49,10 @@ type CreateDashboardSnapshotCommand struct { Expires int64 `json:"expires"` // these are passed when storing an external snapshot ref - External bool `json:"external"` + External bool `json:"external"` + ExternalUrl string `json:"-"` + ExternalDeleteUrl string `json:"-"` + Key string `json:"key"` DeleteKey string `json:"deleteKey"` diff --git a/pkg/services/sqlstore/dashboard_snapshot.go b/pkg/services/sqlstore/dashboard_snapshot.go index 2e2ea8a4783..d0af676a305 100644 --- a/pkg/services/sqlstore/dashboard_snapshot.go +++ b/pkg/services/sqlstore/dashboard_snapshot.go @@ -47,16 +47,18 @@ func CreateDashboardSnapshot(cmd *m.CreateDashboardSnapshotCommand) error { } snapshot := &m.DashboardSnapshot{ - Name: cmd.Name, - Key: cmd.Key, - DeleteKey: cmd.DeleteKey, - OrgId: cmd.OrgId, - UserId: cmd.UserId, - External: cmd.External, - Dashboard: cmd.Dashboard, - Expires: expires, - Created: time.Now(), - Updated: time.Now(), + Name: cmd.Name, + Key: cmd.Key, + DeleteKey: cmd.DeleteKey, + OrgId: cmd.OrgId, + UserId: cmd.UserId, + External: cmd.External, + ExternalUrl: cmd.ExternalUrl, + ExternalDeleteUrl: cmd.ExternalDeleteUrl, + Dashboard: cmd.Dashboard, + Expires: expires, + Created: time.Now(), + Updated: time.Now(), } _, err := sess.Insert(snapshot) diff --git a/public/app/features/dashboard/share_snapshot_ctrl.ts b/public/app/features/dashboard/share_snapshot_ctrl.ts index ac09d63054d..7dcf0469e77 100644 --- a/public/app/features/dashboard/share_snapshot_ctrl.ts +++ b/public/app/features/dashboard/share_snapshot_ctrl.ts @@ -27,7 +27,6 @@ export class ShareSnapshotCtrl { $scope.init = () => { backendSrv.get('/api/snapshot/shared-options').then(options => { - $scope.externalUrl = options['externalSnapshotURL']; $scope.sharingButtonText = options['externalSnapshotName']; $scope.externalEnabled = options['externalEnabled']; }); @@ -61,30 +60,14 @@ export class ShareSnapshotCtrl { dashboard: dash, name: dash.title, expires: $scope.snapshot.expires, + external: external, }; - const postUrl = external ? $scope.externalUrl + $scope.apiUrl : $scope.apiUrl; - - backendSrv.post(postUrl, cmdData).then( + backendSrv.post($scope.apiUrl, cmdData).then( results => { $scope.loading = false; - - if (external) { - $scope.deleteUrl = results.deleteUrl; - $scope.snapshotUrl = results.url; - $scope.saveExternalSnapshotRef(cmdData, results); - } else { - const url = $location.url(); - let baseUrl = $location.absUrl(); - - if (url !== '/') { - baseUrl = baseUrl.replace(url, '') + '/'; - } - - $scope.snapshotUrl = baseUrl + 'dashboard/snapshot/' + results.key; - $scope.deleteUrl = baseUrl + 'api/snapshots-delete/' + results.deleteKey; - } - + $scope.deleteUrl = results.deleteUrl; + $scope.snapshotUrl = results.url; $scope.step = 2; }, () => { @@ -161,14 +144,6 @@ export class ShareSnapshotCtrl { $scope.step = 3; }); }; - - $scope.saveExternalSnapshotRef = (cmdData, results) => { - // save external in local instance as well - cmdData.external = true; - cmdData.key = results.key; - cmdData.deleteKey = results.deleteKey; - backendSrv.post('/api/snapshots/', cmdData); - }; } } From 411d67cae76f49a9925e3663426c4d1a3dbe00b4 Mon Sep 17 00:00:00 2001 From: Victor Cinaglia Date: Mon, 10 Dec 2018 16:40:26 -0500 Subject: [PATCH 3/4] snapshots: Add support for deleting external snapshots --- pkg/api/dashboard_snapshot.go | 45 ++++++++++ pkg/api/dashboard_snapshot_test.go | 87 +++++++++++++++++++ .../manage-dashboards/SnapshotListCtrl.ts | 8 +- .../partials/snapshot_list.html | 10 ++- 4 files changed, 145 insertions(+), 5 deletions(-) diff --git a/pkg/api/dashboard_snapshot.go b/pkg/api/dashboard_snapshot.go index 6c3ee7b69c6..af818be99b0 100644 --- a/pkg/api/dashboard_snapshot.go +++ b/pkg/api/dashboard_snapshot.go @@ -157,6 +157,37 @@ func GetDashboardSnapshot(c *m.ReqContext) { c.JSON(200, dto) } +func deleteExternalDashboardSnapshot(externalUrl string) error { + response, err := client.Get(externalUrl) + + if response != nil { + defer response.Body.Close() + } + + if err != nil { + return err + } + + if response.StatusCode == 200 { + return nil + } + + // Gracefully ignore "snapshot not found" errors as they could have already + // been removed either via the cleanup script or by request. + if response.StatusCode == 500 { + var respJson map[string]interface{} + if err := json.NewDecoder(response.Body).Decode(&respJson); err != nil { + return err + } + + if respJson["message"] == "Failed to get dashboard snapshot" { + return nil + } + } + + return fmt.Errorf("Unexpected response when deleting external snapshot. Status code: %d", response.StatusCode) +} + // GET /api/snapshots-delete/:deleteKey func DeleteDashboardSnapshotByDeleteKey(c *m.ReqContext) Response { key := c.Params(":deleteKey") @@ -168,6 +199,13 @@ func DeleteDashboardSnapshotByDeleteKey(c *m.ReqContext) Response { return Error(500, "Failed to get dashboard snapshot", err) } + if query.Result.External { + err := deleteExternalDashboardSnapshot(query.Result.ExternalDeleteUrl) + if err != nil { + return Error(500, "Failed to delete external dashboard", err) + } + } + cmd := &m.DeleteDashboardSnapshotCommand{DeleteKey: query.Result.DeleteKey} if err := bus.Dispatch(cmd); err != nil { @@ -204,6 +242,13 @@ func DeleteDashboardSnapshot(c *m.ReqContext) Response { return Error(403, "Access denied to this snapshot", nil) } + if query.Result.External { + err := deleteExternalDashboardSnapshot(query.Result.ExternalDeleteUrl) + if err != nil { + return Error(500, "Failed to delete external dashboard", err) + } + } + cmd := &m.DeleteDashboardSnapshotCommand{DeleteKey: query.Result.DeleteKey} if err := bus.Dispatch(cmd); err != nil { diff --git a/pkg/api/dashboard_snapshot_test.go b/pkg/api/dashboard_snapshot_test.go index e58f2c4712d..a24d0f38d85 100644 --- a/pkg/api/dashboard_snapshot_test.go +++ b/pkg/api/dashboard_snapshot_test.go @@ -1,6 +1,9 @@ package api import ( + "fmt" + "net/http" + "net/http/httptest" "testing" "time" @@ -13,13 +16,17 @@ import ( func TestDashboardSnapshotApiEndpoint(t *testing.T) { Convey("Given a single snapshot", t, func() { + var externalRequest *http.Request jsonModel, _ := simplejson.NewJson([]byte(`{"id":100}`)) mockSnapshotResult := &m.DashboardSnapshot{ Id: 1, + Key: "12345", + DeleteKey: "54321", Dashboard: jsonModel, Expires: time.Now().Add(time.Duration(1000) * time.Second), UserId: 999999, + External: true, } bus.AddHandler("test", func(query *m.GetDashboardSnapshotQuery) error { @@ -45,13 +52,25 @@ func TestDashboardSnapshotApiEndpoint(t *testing.T) { return nil }) + setupRemoteServer := func(fn func(http.ResponseWriter, *http.Request)) *httptest.Server { + return httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + fn(rw, r) + })) + } + Convey("When user has editor role and is not in the ACL", func() { Convey("Should not be able to delete snapshot", func() { loggedInUserScenarioWithRole("When calling DELETE on", "DELETE", "/api/snapshots/12345", "/api/snapshots/:key", m.ROLE_EDITOR, func(sc *scenarioContext) { + ts := setupRemoteServer(func(rw http.ResponseWriter, req *http.Request) { + externalRequest = req + }) + + mockSnapshotResult.ExternalDeleteUrl = ts.URL sc.handlerFunc = DeleteDashboardSnapshot sc.fakeReqWithParams("DELETE", sc.url, map[string]string{"key": "12345"}).exec() So(sc.resp.Code, ShouldEqual, 403) + So(externalRequest, ShouldBeNil) }) }) }) @@ -59,6 +78,12 @@ func TestDashboardSnapshotApiEndpoint(t *testing.T) { Convey("When user is anonymous", func() { Convey("Should be able to delete snapshot by deleteKey", func() { anonymousUserScenario("When calling GET on", "GET", "/api/snapshots-delete/12345", "/api/snapshots-delete/:deleteKey", func(sc *scenarioContext) { + ts := setupRemoteServer(func(rw http.ResponseWriter, req *http.Request) { + rw.WriteHeader(200) + externalRequest = req + }) + + mockSnapshotResult.ExternalDeleteUrl = ts.URL sc.handlerFunc = DeleteDashboardSnapshotByDeleteKey sc.fakeReqWithParams("GET", sc.url, map[string]string{"deleteKey": "12345"}).exec() @@ -67,6 +92,10 @@ func TestDashboardSnapshotApiEndpoint(t *testing.T) { So(err, ShouldBeNil) So(respJSON.Get("message").MustString(), ShouldStartWith, "Snapshot deleted") + + So(externalRequest.Method, ShouldEqual, http.MethodGet) + So(fmt.Sprintf("http://%s", externalRequest.Host), ShouldEqual, ts.URL) + So(externalRequest.URL.EscapedPath(), ShouldEqual, "/") }) }) }) @@ -79,6 +108,12 @@ func TestDashboardSnapshotApiEndpoint(t *testing.T) { Convey("Should be able to delete a snapshot", func() { loggedInUserScenarioWithRole("When calling DELETE on", "DELETE", "/api/snapshots/12345", "/api/snapshots/:key", m.ROLE_EDITOR, func(sc *scenarioContext) { + ts := setupRemoteServer(func(rw http.ResponseWriter, req *http.Request) { + rw.WriteHeader(200) + externalRequest = req + }) + + mockSnapshotResult.ExternalDeleteUrl = ts.URL sc.handlerFunc = DeleteDashboardSnapshot sc.fakeReqWithParams("DELETE", sc.url, map[string]string{"key": "12345"}).exec() @@ -87,6 +122,8 @@ func TestDashboardSnapshotApiEndpoint(t *testing.T) { So(err, ShouldBeNil) So(respJSON.Get("message").MustString(), ShouldStartWith, "Snapshot deleted") + So(fmt.Sprintf("http://%s", externalRequest.Host), ShouldEqual, ts.URL) + So(externalRequest.URL.EscapedPath(), ShouldEqual, "/") }) }) }) @@ -94,6 +131,7 @@ func TestDashboardSnapshotApiEndpoint(t *testing.T) { Convey("When user is editor and is the creator of the snapshot", func() { aclMockResp = []*m.DashboardAclInfoDTO{} mockSnapshotResult.UserId = TestUserID + mockSnapshotResult.External = false Convey("Should be able to delete a snapshot", func() { loggedInUserScenarioWithRole("When calling DELETE on", "DELETE", "/api/snapshots/12345", "/api/snapshots/:key", m.ROLE_EDITOR, func(sc *scenarioContext) { @@ -108,5 +146,54 @@ func TestDashboardSnapshotApiEndpoint(t *testing.T) { }) }) }) + + Convey("When deleting an external snapshot", func() { + aclMockResp = []*m.DashboardAclInfoDTO{} + mockSnapshotResult.UserId = TestUserID + + Convey("Should gracefully delete local snapshot when remote snapshot has already been removed", func() { + loggedInUserScenarioWithRole("When calling DELETE on", "DELETE", "/api/snapshots/12345", "/api/snapshots/:key", m.ROLE_EDITOR, func(sc *scenarioContext) { + ts := setupRemoteServer(func(rw http.ResponseWriter, req *http.Request) { + rw.Write([]byte(`{"message":"Failed to get dashboard snapshot"}`)) + rw.WriteHeader(500) + }) + + mockSnapshotResult.ExternalDeleteUrl = ts.URL + sc.handlerFunc = DeleteDashboardSnapshot + sc.fakeReqWithParams("DELETE", sc.url, map[string]string{"key": "12345"}).exec() + + So(sc.resp.Code, ShouldEqual, 200) + }) + }) + + Convey("Should fail to delete local snapshot when an unexpected 500 error occurs", func() { + loggedInUserScenarioWithRole("When calling DELETE on", "DELETE", "/api/snapshots/12345", "/api/snapshots/:key", m.ROLE_EDITOR, func(sc *scenarioContext) { + ts := setupRemoteServer(func(rw http.ResponseWriter, req *http.Request) { + rw.WriteHeader(500) + rw.Write([]byte(`{"message":"Unexpected"}`)) + }) + + mockSnapshotResult.ExternalDeleteUrl = ts.URL + sc.handlerFunc = DeleteDashboardSnapshot + sc.fakeReqWithParams("DELETE", sc.url, map[string]string{"key": "12345"}).exec() + + So(sc.resp.Code, ShouldEqual, 500) + }) + }) + + Convey("Should fail to delete local snapshot when an unexpected remote error occurs", func() { + loggedInUserScenarioWithRole("When calling DELETE on", "DELETE", "/api/snapshots/12345", "/api/snapshots/:key", m.ROLE_EDITOR, func(sc *scenarioContext) { + ts := setupRemoteServer(func(rw http.ResponseWriter, req *http.Request) { + rw.WriteHeader(404) + }) + + mockSnapshotResult.ExternalDeleteUrl = ts.URL + sc.handlerFunc = DeleteDashboardSnapshot + sc.fakeReqWithParams("DELETE", sc.url, map[string]string{"key": "12345"}).exec() + + So(sc.resp.Code, ShouldEqual, 500) + }) + }) + }) }) } diff --git a/public/app/features/manage-dashboards/SnapshotListCtrl.ts b/public/app/features/manage-dashboards/SnapshotListCtrl.ts index 2ff53e7aed5..4d6dc006d47 100644 --- a/public/app/features/manage-dashboards/SnapshotListCtrl.ts +++ b/public/app/features/manage-dashboards/SnapshotListCtrl.ts @@ -5,10 +5,14 @@ export class SnapshotListCtrl { snapshots: any; /** @ngInject */ - constructor(private $rootScope, private backendSrv, navModelSrv) { + constructor(private $rootScope, private backendSrv, navModelSrv, private $location) { this.navModel = navModelSrv.getNav('dashboards', 'snapshots', 0); this.backendSrv.get('/api/dashboard/snapshots').then(result => { - this.snapshots = result; + const baseUrl = this.$location.absUrl().replace($location.url(), ''); + this.snapshots = result.map(snapshot => ({ + ...snapshot, + url: snapshot.externalUrl || `${baseUrl}/dashboard/snapshot/${snapshot.key}`, + })); }); } diff --git a/public/app/features/manage-dashboards/partials/snapshot_list.html b/public/app/features/manage-dashboards/partials/snapshot_list.html index 8775b527ae1..f646194088d 100644 --- a/public/app/features/manage-dashboards/partials/snapshot_list.html +++ b/public/app/features/manage-dashboards/partials/snapshot_list.html @@ -6,17 +6,21 @@ Name Snapshot url + - {{snapshot.name}} + {{snapshot.name}} - dashboard/snapshot/{{snapshot.key}} + {{snapshot.url}} + + + External - + View From 48fe92a9457210a5a9551a8cd5b22d82a880ec11 Mon Sep 17 00:00:00 2001 From: Victor Cinaglia Date: Tue, 18 Dec 2018 08:32:49 -0500 Subject: [PATCH 4/4] snapshots: Close response body after error check --- pkg/api/dashboard_snapshot.go | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/pkg/api/dashboard_snapshot.go b/pkg/api/dashboard_snapshot.go index af818be99b0..ca7e78a58cd 100644 --- a/pkg/api/dashboard_snapshot.go +++ b/pkg/api/dashboard_snapshot.go @@ -51,13 +51,10 @@ func createExternalDashboardSnapshot(cmd m.CreateDashboardSnapshotCommand) (*Cre } response, err := client.Post(setting.ExternalSnapshotUrl+"/api/snapshots", "application/json", bytes.NewBuffer(messageBytes)) - if response != nil { - defer response.Body.Close() - } - if err != nil { return nil, err } + defer response.Body.Close() if response.StatusCode != 200 { return nil, fmt.Errorf("Create external snapshot response status code %d", response.StatusCode) @@ -159,14 +156,10 @@ func GetDashboardSnapshot(c *m.ReqContext) { func deleteExternalDashboardSnapshot(externalUrl string) error { response, err := client.Get(externalUrl) - - if response != nil { - defer response.Body.Close() - } - if err != nil { return err } + defer response.Body.Close() if response.StatusCode == 200 { return nil