diff --git a/pkg/api/http_server.go b/pkg/api/http_server.go index 11927f139adeb..1911f258f1c6e 100644 --- a/pkg/api/http_server.go +++ b/pkg/api/http_server.go @@ -133,6 +133,7 @@ type HTTPServer struct { pluginClient plugins.Client pluginStore plugins.Store pluginInstaller plugins.Installer + pluginFileStore plugins.FileStore pluginDashboardService plugindashboards.Service pluginStaticRouteResolver plugins.StaticRouteResolver pluginErrorResolver plugins.ErrorResolver @@ -230,7 +231,7 @@ func ProvideHTTPServer(opts ServerOptions, cfg *setting.Cfg, routeRegister routi quotaService quota.Service, socialService social.Service, tracer tracing.Tracer, encryptionService encryption.Internal, grafanaUpdateChecker *updatechecker.GrafanaService, pluginsUpdateChecker *updatechecker.PluginsService, searchUsersService searchusers.Service, - dataSourcesService datasources.DataSourceService, queryDataService query.Service, + dataSourcesService datasources.DataSourceService, queryDataService query.Service, pluginFileStore plugins.FileStore, teamGuardian teamguardian.TeamGuardian, serviceaccountsService serviceaccounts.Service, authInfoService login.AuthInfoService, storageService store.StorageService, httpEntityStore httpentitystore.HTTPEntityStore, notificationService *notifications.NotificationService, dashboardService dashboards.DashboardService, @@ -271,6 +272,7 @@ func ProvideHTTPServer(opts ServerOptions, cfg *setting.Cfg, routeRegister routi pluginStaticRouteResolver: pluginStaticRouteResolver, pluginDashboardService: pluginDashboardService, pluginErrorResolver: pluginErrorResolver, + pluginFileStore: pluginFileStore, grafanaUpdateChecker: grafanaUpdateChecker, pluginsUpdateChecker: pluginsUpdateChecker, SettingsProvider: settingsProvider, diff --git a/pkg/api/plugins.go b/pkg/api/plugins.go index 3cf46428fe8ad..b5313141a2ce3 100644 --- a/pkg/api/plugins.go +++ b/pkg/api/plugins.go @@ -6,7 +6,6 @@ import ( "encoding/json" "errors" "fmt" - "io" "net/http" "path" "path/filepath" @@ -350,7 +349,7 @@ func (hs *HTTPServer) getPluginAssets(c *contextmodel.ReqContext) { // serveLocalPluginAsset returns the content of a plugin asset file from the local filesystem to the http client. func (hs *HTTPServer) serveLocalPluginAsset(c *contextmodel.ReqContext, plugin plugins.PluginDTO, assetPath string) { - f, err := plugin.File(assetPath) + f, err := hs.pluginFileStore.File(c.Req.Context(), plugin.ID, assetPath) if err != nil { if errors.Is(err, plugins.ErrFileNotExist) { c.JsonApiErr(404, "Plugin file not found", nil) @@ -359,17 +358,6 @@ func (hs *HTTPServer) serveLocalPluginAsset(c *contextmodel.ReqContext, plugin p c.JsonApiErr(500, "Could not open plugin file", err) return } - defer func() { - if err = f.Close(); err != nil { - hs.log.Error("Failed to close plugin file", "err", err) - } - }() - - fi, err := f.Stat() - if err != nil { - c.JsonApiErr(500, "Plugin file exists but could not open", err) - return - } if hs.Cfg.Env == setting.Dev { c.Resp.Header().Set("Cache-Control", "max-age=0, must-revalidate, no-cache") @@ -377,17 +365,7 @@ func (hs *HTTPServer) serveLocalPluginAsset(c *contextmodel.ReqContext, plugin p c.Resp.Header().Set("Cache-Control", "public, max-age=3600") } - if rs, ok := f.(io.ReadSeeker); ok { - http.ServeContent(c.Resp, c.Req, assetPath, fi.ModTime(), rs) - return - } - - b, err := io.ReadAll(f) - if err != nil { - c.JsonApiErr(500, "Plugin file exists but could not read", err) - return - } - http.ServeContent(c.Resp, c.Req, assetPath, fi.ModTime(), bytes.NewReader(b)) + http.ServeContent(c.Resp, c.Req, assetPath, f.ModTime, bytes.NewReader(f.Content)) } // redirectCDNPluginAsset redirects the http request to specified asset path on the configured plugins CDN. @@ -538,34 +516,24 @@ func translatePluginRequestErrorToAPIError(err error) response.Response { return response.Error(500, "Plugin request failed", err) } -func (hs *HTTPServer) pluginMarkdown(ctx context.Context, pluginId string, name string) ([]byte, error) { - plugin, exists := hs.pluginStore.Plugin(ctx, pluginId) - if !exists { - return make([]byte, 0), plugins.NotFoundError{PluginID: pluginId} - } - +func (hs *HTTPServer) pluginMarkdown(ctx context.Context, pluginID string, name string) ([]byte, error) { file, err := mdFilepath(strings.ToUpper(name)) if err != nil { return make([]byte, 0), err } - md, err := plugin.File(file) + md, err := hs.pluginFileStore.File(ctx, pluginID, file) if err != nil { - md, err = plugin.File(strings.ToLower(file)) + if errors.Is(err, plugins.ErrPluginNotInstalled) { + return make([]byte, 0), plugins.NotFoundError{PluginID: pluginID} + } + + md, err = hs.pluginFileStore.File(ctx, pluginID, strings.ToLower(file)) if err != nil { return make([]byte, 0), nil } } - defer func() { - if err = md.Close(); err != nil { - hs.log.Error("Failed to close plugin markdown file", "err", err) - } - }() - d, err := io.ReadAll(md) - if err != nil { - return make([]byte, 0), nil - } - return d, nil + return md.Content, nil } func mdFilepath(mdFilename string) (string, error) { diff --git a/pkg/api/plugins_test.go b/pkg/api/plugins_test.go index e644751f3f41b..9de3a41e4ccea 100644 --- a/pkg/api/plugins_test.go +++ b/pkg/api/plugins_test.go @@ -1,13 +1,11 @@ package api import ( - "bytes" "context" "encoding/json" "errors" "fmt" "io" - "io/fs" "net/http" "net/http/httptest" "os" @@ -27,6 +25,9 @@ import ( "github.com/grafana/grafana/pkg/plugins" "github.com/grafana/grafana/pkg/plugins/config" "github.com/grafana/grafana/pkg/plugins/manager/fakes" + "github.com/grafana/grafana/pkg/plugins/manager/filestore" + "github.com/grafana/grafana/pkg/plugins/manager/registry" + "github.com/grafana/grafana/pkg/plugins/manager/store" "github.com/grafana/grafana/pkg/plugins/pluginscdn" ac "github.com/grafana/grafana/pkg/services/accesscontrol" contextmodel "github.com/grafana/grafana/pkg/services/contexthandler/model" @@ -140,6 +141,7 @@ func Test_PluginsInstallAndUninstall_AccessControl(t *testing.T) { PluginAdminExternalManageEnabled: tc.pluginAdminExternalManageEnabled} hs.orgService = &orgtest.FakeOrgService{ExpectedOrg: &org.Org{}} hs.pluginInstaller = NewFakePluginInstaller() + hs.pluginFileStore = &fakes.FakePluginFileStore{} }) t.Run(testName("Install", tc), func(t *testing.T) { @@ -172,10 +174,10 @@ func Test_GetPluginAssetCDNRedirect(t *testing.T) { nonCdnPlugin := &plugins.Plugin{ JSONData: plugins.JSONData{ID: nonCDNPluginID, Info: plugins.Info{Version: "2.0.0"}}, } - service := &plugins.FakePluginStore{ - PluginList: []plugins.PluginDTO{ - cdnPlugin.ToDTO(), - nonCdnPlugin.ToDTO(), + registry := &fakes.FakePluginRegistry{ + Store: map[string]*plugins.Plugin{ + cdnPluginID: cdnPlugin, + nonCDNPluginID: nonCdnPlugin, }, } cfg := setting.NewCfg() @@ -200,7 +202,7 @@ func Test_GetPluginAssetCDNRedirect(t *testing.T) { "When calling GET for a CDN plugin on", fmt.Sprintf("/public/plugins/%s/%s", cdnPluginID, cas.assetURL), "/public/plugins/:pluginId/*", - cfg, service, func(sc *scenarioContext) { + cfg, registry, func(sc *scenarioContext) { // Get the prometheus metric (to test that the handler is instrumented correctly) counter := pluginsCDNFallbackRedirectRequests.With(prometheus.Labels{ "plugin_id": cdnPluginID, @@ -230,7 +232,7 @@ func Test_GetPluginAssetCDNRedirect(t *testing.T) { "When calling GET for a non-CDN plugin on", fmt.Sprintf("/public/plugins/%s/%s", nonCDNPluginID, "module.js"), "/public/plugins/:pluginId/*", - cfg, service, func(sc *scenarioContext) { + cfg, registry, func(sc *scenarioContext) { // Here the metric should not increment var m dto.Metric counter := pluginsCDNFallbackRedirectRequests.With(prometheus.Labels{ @@ -275,14 +277,16 @@ func Test_GetPluginAssets(t *testing.T) { requestedFile := filepath.Clean(tmpFile.Name()) t.Run("Given a request for an existing plugin file", func(t *testing.T) { - p := createPluginDTO(plugins.JSONData{ID: pluginID}, plugins.External, plugins.NewLocalFS(map[string]struct{}{requestedFile: {}}, filepath.Dir(requestedFile))) - service := &plugins.FakePluginStore{ - PluginList: []plugins.PluginDTO{p}, + p := createPlugin(plugins.JSONData{ID: pluginID}, plugins.External, plugins.NewLocalFS(map[string]struct{}{requestedFile: {}}, filepath.Dir(requestedFile))) + pluginRegistry := &fakes.FakePluginRegistry{ + Store: map[string]*plugins.Plugin{ + p.ID: p, + }, } url := fmt.Sprintf("/public/plugins/%s/%s", pluginID, requestedFile) pluginAssetScenario(t, "When calling GET on", url, "/public/plugins/:pluginId/*", - setting.NewCfg(), service, func(sc *scenarioContext) { + setting.NewCfg(), pluginRegistry, func(sc *scenarioContext) { callGetPluginAsset(sc) require.Equal(t, 200, sc.resp.Code) @@ -291,14 +295,16 @@ func Test_GetPluginAssets(t *testing.T) { }) t.Run("Given a request for a relative path", func(t *testing.T) { - p := createPluginDTO(plugins.JSONData{ID: pluginID}, plugins.External, plugins.NewLocalFS(map[string]struct{}{}, "")) - service := &plugins.FakePluginStore{ - PluginList: []plugins.PluginDTO{p}, + p := createPlugin(plugins.JSONData{ID: pluginID}, plugins.External, plugins.NewLocalFS(map[string]struct{}{}, "")) + pluginRegistry := &fakes.FakePluginRegistry{ + Store: map[string]*plugins.Plugin{ + p.ID: p, + }, } url := fmt.Sprintf("/public/plugins/%s/%s", pluginID, tmpFileInParentDir.Name()) pluginAssetScenario(t, "When calling GET on", url, "/public/plugins/:pluginId/*", - setting.NewCfg(), service, func(sc *scenarioContext) { + setting.NewCfg(), pluginRegistry, func(sc *scenarioContext) { callGetPluginAsset(sc) require.Equal(t, 404, sc.resp.Code) @@ -306,16 +312,18 @@ func Test_GetPluginAssets(t *testing.T) { }) t.Run("Given a request for an existing plugin file that is not listed as a signature covered file", func(t *testing.T) { - p := createPluginDTO(plugins.JSONData{ID: pluginID}, plugins.Core, plugins.NewLocalFS(map[string]struct{}{ + p := createPlugin(plugins.JSONData{ID: pluginID}, plugins.Core, plugins.NewLocalFS(map[string]struct{}{ requestedFile: {}, }, "")) - service := &plugins.FakePluginStore{ - PluginList: []plugins.PluginDTO{p}, + pluginRegistry := &fakes.FakePluginRegistry{ + Store: map[string]*plugins.Plugin{ + p.ID: p, + }, } url := fmt.Sprintf("/public/plugins/%s/%s", pluginID, requestedFile) pluginAssetScenario(t, "When calling GET on", url, "/public/plugins/:pluginId/*", - setting.NewCfg(), service, func(sc *scenarioContext) { + setting.NewCfg(), pluginRegistry, func(sc *scenarioContext) { callGetPluginAsset(sc) require.Equal(t, 200, sc.resp.Code) @@ -324,9 +332,11 @@ func Test_GetPluginAssets(t *testing.T) { }) t.Run("Given a request for an non-existing plugin file", func(t *testing.T) { - p := createPluginDTO(plugins.JSONData{ID: pluginID}, plugins.External, plugins.NewLocalFS(map[string]struct{}{}, "")) - service := &plugins.FakePluginStore{ - PluginList: []plugins.PluginDTO{p}, + p := createPlugin(plugins.JSONData{ID: pluginID}, plugins.External, plugins.NewLocalFS(map[string]struct{}{}, "")) + service := &fakes.FakePluginRegistry{ + Store: map[string]*plugins.Plugin{ + p.ID: p, + }, } requestedFile := "nonExistent" @@ -344,14 +354,10 @@ func Test_GetPluginAssets(t *testing.T) { }) t.Run("Given a request for an non-existing plugin", func(t *testing.T) { - service := &plugins.FakePluginStore{ - PluginList: []plugins.PluginDTO{}, - } - requestedFile := "nonExistent" url := fmt.Sprintf("/public/plugins/%s/%s", pluginID, requestedFile) pluginAssetScenario(t, "When calling GET on", url, "/public/plugins/:pluginId/*", - setting.NewCfg(), service, func(sc *scenarioContext) { + setting.NewCfg(), fakes.NewFakePluginRegistry(), func(sc *scenarioContext) { callGetPluginAsset(sc) var respJson map[string]interface{} @@ -471,11 +477,12 @@ func TestMakePluginResourceRequestContentTypeEmpty(t *testing.T) { func TestPluginMarkdown(t *testing.T) { t.Run("Plugin not installed returns error", func(t *testing.T) { - hs := HTTPServer{ - pluginStore: &plugins.FakePluginStore{ - PluginList: []plugins.PluginDTO{}, + pluginFileStore := &fakes.FakePluginFileStore{ + FileFunc: func(ctx context.Context, pluginID, filename string) (*plugins.File, error) { + return nil, plugins.ErrPluginNotInstalled }, } + hs := HTTPServer{pluginFileStore: pluginFileStore} pluginID := "test-datasource" md, err := hs.pluginMarkdown(context.Background(), pluginID, "test") @@ -485,20 +492,16 @@ func TestPluginMarkdown(t *testing.T) { t.Run("File fetch will be retried using different casing if error occurs", func(t *testing.T) { var requestedFiles []string - pluginFiles := &fakes.FakePluginFiles{ - OpenFunc: func(name string) (fs.File, error) { - requestedFiles = append(requestedFiles, name) + pluginFileStore := &fakes.FakePluginFileStore{ + FileFunc: func(ctx context.Context, pluginID, filename string) (*plugins.File, error) { + requestedFiles = append(requestedFiles, filename) return nil, errors.New("some error") }, } - p := createPluginDTO(plugins.JSONData{ID: "test-app"}, plugins.External, pluginFiles) - - hs := HTTPServer{ - pluginStore: &plugins.FakePluginStore{PluginList: []plugins.PluginDTO{p}}, - } + hs := HTTPServer{pluginFileStore: pluginFileStore} - md, err := hs.pluginMarkdown(context.Background(), p.ID, "reAdMe") + md, err := hs.pluginMarkdown(context.Background(), "", "reAdMe") require.NoError(t, err) require.Equal(t, []byte{}, md) require.Equal(t, []string{"README.md", "readme.md"}, requestedFiles) @@ -524,54 +527,44 @@ func TestPluginMarkdown(t *testing.T) { } for _, tc := range tcs { + data := []byte{123} var requestedFiles []string - pluginFiles := &fakes.FakePluginFiles{ - OpenFunc: func(name string) (fs.File, error) { - requestedFiles = append(requestedFiles, name) - return &FakeFile{data: bytes.NewReader(nil)}, nil + pluginFileStore := &fakes.FakePluginFileStore{ + FileFunc: func(ctx context.Context, pluginID, filename string) (*plugins.File, error) { + requestedFiles = append(requestedFiles, filename) + return &plugins.File{Content: data}, nil }, } - p := createPluginDTO(plugins.JSONData{ID: "test-app"}, plugins.External, pluginFiles) - - hs := HTTPServer{ - pluginStore: &plugins.FakePluginStore{PluginList: []plugins.PluginDTO{p}}, - } + hs := HTTPServer{pluginFileStore: pluginFileStore} - md, err := hs.pluginMarkdown(context.Background(), p.ID, tc.filePath) + md, err := hs.pluginMarkdown(context.Background(), "test-datasource", tc.filePath) require.NoError(t, err) - require.Equal(t, []byte{}, md) + require.Equal(t, data, md) require.Equal(t, tc.expected, requestedFiles) } }) t.Run("Non markdown file request returns an error", func(t *testing.T) { - p := createPluginDTO(plugins.JSONData{ID: "test-app"}, plugins.External, nil) - hs := HTTPServer{ - pluginStore: &plugins.FakePluginStore{PluginList: []plugins.PluginDTO{p}}, - } + hs := HTTPServer{pluginFileStore: &fakes.FakePluginFileStore{}} - md, err := hs.pluginMarkdown(context.Background(), p.ID, "test.json") + md, err := hs.pluginMarkdown(context.Background(), "", "test.json") require.ErrorIs(t, err, ErrUnexpectedFileExtension) require.Equal(t, []byte{}, md) }) t.Run("Happy path", func(t *testing.T) { data := []byte{1, 2, 3} - fakeFile := &FakeFile{data: bytes.NewReader(data)} - pluginFiles := &fakes.FakePluginFiles{ - OpenFunc: func(name string) (fs.File, error) { - return fakeFile, nil + pluginFileStore := &fakes.FakePluginFileStore{ + FileFunc: func(ctx context.Context, pluginID, filename string) (*plugins.File, error) { + return &plugins.File{Content: data}, nil }, } - p := createPluginDTO(plugins.JSONData{ID: "test-app"}, plugins.External, pluginFiles) - hs := HTTPServer{ - pluginStore: &plugins.FakePluginStore{PluginList: []plugins.PluginDTO{p}}, - } + hs := HTTPServer{pluginFileStore: pluginFileStore} - md, err := hs.pluginMarkdown(context.Background(), p.ID, "someFile") + md, err := hs.pluginMarkdown(context.Background(), "", "someFile") require.NoError(t, err) require.Equal(t, data, md) }) @@ -582,13 +575,14 @@ func callGetPluginAsset(sc *scenarioContext) { } func pluginAssetScenario(t *testing.T, desc string, url string, urlPattern string, - cfg *setting.Cfg, pluginStore plugins.Store, fn scenarioFunc) { + cfg *setting.Cfg, pluginRegistry registry.Service, fn scenarioFunc) { t.Run(fmt.Sprintf("%s %s", desc, url), func(t *testing.T) { cfg.IsFeatureToggleEnabled = func(_ string) bool { return false } hs := HTTPServer{ - Cfg: cfg, - pluginStore: pluginStore, - log: log.NewNopLogger(), + Cfg: cfg, + pluginStore: store.New(pluginRegistry), + pluginFileStore: filestore.ProvideService(pluginRegistry), + log: log.NewNopLogger(), pluginsCDNService: pluginscdn.ProvideService(&config.Cfg{ PluginsCDNURLTemplate: cfg.PluginsCDNURLTemplate, PluginSettings: cfg.PluginSettings, @@ -648,19 +642,24 @@ func (c *fakePluginClient) QueryData(ctx context.Context, req *backend.QueryData } func Test_PluginsList_AccessControl(t *testing.T) { - p1 := createPluginDTO(plugins.JSONData{ + p1 := createPlugin(plugins.JSONData{ ID: "test-app", Type: "app", Name: "test-app", Info: plugins.Info{ Version: "1.0.0", }}, plugins.External, plugins.NewLocalFS(map[string]struct{}{}, "")) - p2 := createPluginDTO( + p2 := createPlugin( plugins.JSONData{ID: "mysql", Type: "datasource", Name: "MySQL", Info: plugins.Info{ Author: plugins.InfoLink{Name: "Grafana Labs", URL: "https://grafana.com"}, Description: "Data source for MySQL databases", }}, plugins.Core, plugins.NewLocalFS(map[string]struct{}{}, "")) - pluginStore := plugins.FakePluginStore{PluginList: []plugins.PluginDTO{p1, p2}} + pluginRegistry := &fakes.FakePluginRegistry{ + Store: map[string]*plugins.Plugin{ + p1.ID: p1, + p2.ID: p2, + }, + } pluginSettings := pluginsettings.FakePluginSettings{Plugins: map[string]*pluginsettings.DTO{ "test-app": {ID: 0, OrgID: 1, PluginID: "test-app", PluginVersion: "1.0.0", Enabled: true}, @@ -693,9 +692,10 @@ func Test_PluginsList_AccessControl(t *testing.T) { server := SetupAPITestServer(t, func(hs *HTTPServer) { hs.Cfg = setting.NewCfg() hs.PluginSettings = &pluginSettings - hs.pluginStore = pluginStore + hs.pluginStore = store.New(pluginRegistry) + hs.pluginFileStore = filestore.ProvideService(pluginRegistry) var err error - hs.pluginsUpdateChecker, err = updatechecker.ProvidePluginsService(hs.Cfg, pluginStore, tracing.InitializeTracerForTest()) + hs.pluginsUpdateChecker, err = updatechecker.ProvidePluginsService(hs.Cfg, nil, tracing.InitializeTracerForTest()) require.NoError(t, err) }) @@ -713,30 +713,10 @@ func Test_PluginsList_AccessControl(t *testing.T) { } } -func createPluginDTO(jd plugins.JSONData, class plugins.Class, files plugins.FS) plugins.PluginDTO { - p := &plugins.Plugin{ +func createPlugin(jd plugins.JSONData, class plugins.Class, files plugins.FS) *plugins.Plugin { + return &plugins.Plugin{ JSONData: jd, Class: class, FS: files, } - - return p.ToDTO() -} - -var _ fs.File = (*FakeFile)(nil) - -type FakeFile struct { - data io.Reader -} - -func (f *FakeFile) Stat() (fs.FileInfo, error) { - return nil, nil -} - -func (f *FakeFile) Read(bytes []byte) (int, error) { - return f.data.Read(bytes) -} - -func (f *FakeFile) Close() error { - return nil } diff --git a/pkg/plugins/ifaces.go b/pkg/plugins/ifaces.go index a9786c9c80db0..8c1ae78f34b9e 100644 --- a/pkg/plugins/ifaces.go +++ b/pkg/plugins/ifaces.go @@ -3,6 +3,7 @@ package plugins import ( "context" "io/fs" + "time" "github.com/grafana/grafana-plugin-sdk-go/backend" @@ -30,6 +31,16 @@ type PluginSource interface { DefaultSignature(ctx context.Context) (Signature, bool) } +type FileStore interface { + // File retrieves a plugin file. + File(ctx context.Context, pluginID, filename string) (*File, error) +} + +type File struct { + Content []byte + ModTime time.Time +} + type CompatOpts struct { GrafanaVersion string OS string diff --git a/pkg/plugins/manager/dashboards/filestore.go b/pkg/plugins/manager/dashboards/filestore.go index 80796cd448bab..4bd34faa38b63 100644 --- a/pkg/plugins/manager/dashboards/filestore.go +++ b/pkg/plugins/manager/dashboards/filestore.go @@ -3,7 +3,6 @@ package dashboards import ( "context" "fmt" - "io/fs" "strings" "github.com/grafana/grafana/pkg/plugins" @@ -13,17 +12,24 @@ import ( var _ FileStore = (*FileStoreManager)(nil) type FileStoreManager struct { - pluginStore plugins.Store + pluginStore plugins.Store + pluginFileStore plugins.FileStore } -func ProvideFileStoreManager(pluginStore plugins.Store) *FileStoreManager { +func ProvideFileStoreManager(pluginStore plugins.Store, pluginFileStore plugins.FileStore) *FileStoreManager { return &FileStoreManager{ - pluginStore: pluginStore, + pluginStore: pluginStore, + pluginFileStore: pluginFileStore, } } -var openDashboardFile = func(p plugins.PluginDTO, name string) (fs.File, error) { - return p.File(name) +var openDashboardFile = func(ctx context.Context, pluginFileStore plugins.FileStore, pluginID, name string) (*plugins.File, error) { + f, err := pluginFileStore.File(ctx, pluginID, name) + if err != nil { + return &plugins.File{}, err + } + + return f, nil } func (m *FileStoreManager) ListPluginDashboardFiles(ctx context.Context, args *ListPluginDashboardFilesArgs) (*ListPluginDashboardFilesResult, error) { @@ -86,12 +92,12 @@ func (m *FileStoreManager) GetPluginDashboardFileContents(ctx context.Context, a return nil, err } - file, err := openDashboardFile(plugin, cleanPath) + file, err := openDashboardFile(ctx, m.pluginFileStore, plugin.ID, cleanPath) if err != nil { return nil, err } return &GetPluginDashboardFileContentsResult{ - Content: file, + Content: file.Content, }, nil } diff --git a/pkg/plugins/manager/dashboards/filestore_test.go b/pkg/plugins/manager/dashboards/filestore_test.go index 4ab977e418770..d1aeb2ce28bfc 100644 --- a/pkg/plugins/manager/dashboards/filestore_test.go +++ b/pkg/plugins/manager/dashboards/filestore_test.go @@ -3,7 +3,6 @@ package dashboards import ( "context" "io" - "io/fs" "testing" "testing/fstest" @@ -131,10 +130,13 @@ func TestDashboardFileStore(t *testing.T) { Data: []byte("dash2"), }, } - openDashboardFile = func(p plugins.PluginDTO, name string) (fs.File, error) { + openDashboardFile = func(ctx context.Context, pluginFiles plugins.FileStore, pluginID, name string) (*plugins.File, error) { f, err := mapFs.Open(name) require.NoError(t, err) - return f, nil + + b, err := io.ReadAll(f) + require.NoError(t, err) + return &plugins.File{Content: b}, nil } t.Cleanup(func() { openDashboardFile = origOpenDashboardFile @@ -157,10 +159,7 @@ func TestDashboardFileStore(t *testing.T) { }) require.NoError(t, err) require.NotNil(t, res) - require.NotNil(t, res.Content) - b, err := io.ReadAll(res.Content) - require.NoError(t, err) - require.Equal(t, "dash1", string(b)) + require.Equal(t, "dash1", string(res.Content)) }) t.Run("Should return file content for dashboards/dash2.json", func(t *testing.T) { @@ -170,10 +169,7 @@ func TestDashboardFileStore(t *testing.T) { }) require.NoError(t, err) require.NotNil(t, res) - require.NotNil(t, res.Content) - b, err := io.ReadAll(res.Content) - require.NoError(t, err) - require.Equal(t, "dash2", string(b)) + require.Equal(t, "dash2", string(res.Content)) }) t.Run("Should return error when trying to read relative file", func(t *testing.T) { diff --git a/pkg/plugins/manager/dashboards/ifaces.go b/pkg/plugins/manager/dashboards/ifaces.go index a8cd5a11278cb..aaadd622f952a 100644 --- a/pkg/plugins/manager/dashboards/ifaces.go +++ b/pkg/plugins/manager/dashboards/ifaces.go @@ -2,7 +2,6 @@ package dashboards import ( "context" - "io" ) // FileStore is the interface for plugin dashboard file storage. @@ -31,5 +30,5 @@ type GetPluginDashboardFileContentsArgs struct { // GetPluginDashboardFileContentsResult get plugin dashboard file content result model. type GetPluginDashboardFileContentsResult struct { - Content io.ReadCloser + Content []byte } diff --git a/pkg/plugins/manager/fakes/fakes.go b/pkg/plugins/manager/fakes/fakes.go index a4a9ad1a4d096..55c8f8d595afb 100644 --- a/pkg/plugins/manager/fakes/fakes.go +++ b/pkg/plugins/manager/fakes/fakes.go @@ -421,3 +421,14 @@ func (s *FakePluginSource) DefaultSignature(ctx context.Context) (plugins.Signat } return plugins.Signature{}, false } + +type FakePluginFileStore struct { + FileFunc func(ctx context.Context, pluginID, filename string) (*plugins.File, error) +} + +func (f *FakePluginFileStore) File(ctx context.Context, pluginID, filename string) (*plugins.File, error) { + if f.FileFunc != nil { + return f.FileFunc(ctx, pluginID, filename) + } + return nil, nil +} diff --git a/pkg/plugins/manager/filestore/fs.go b/pkg/plugins/manager/filestore/fs.go new file mode 100644 index 0000000000000..335daf49d9ffe --- /dev/null +++ b/pkg/plugins/manager/filestore/fs.go @@ -0,0 +1,54 @@ +package filestore + +import ( + "context" + "io" + + "github.com/grafana/grafana/pkg/plugins" + "github.com/grafana/grafana/pkg/plugins/log" + "github.com/grafana/grafana/pkg/plugins/manager/registry" +) + +type Service struct { + pluginRegistry registry.Service + log log.Logger +} + +func ProvideService(pluginRegistry registry.Service) *Service { + return &Service{ + pluginRegistry: pluginRegistry, + log: log.New("plugin.fs"), + } +} + +func (s *Service) File(ctx context.Context, pluginID, filename string) (*plugins.File, error) { + if p, exists := s.pluginRegistry.Plugin(ctx, pluginID); exists { + f, err := p.File(filename) + if err != nil { + return nil, err + } + defer func() { + err = f.Close() + if err != nil { + s.log.Error("Could not close plugin file", "pluginID", p.ID, "file", filename) + } + }() + + b, err := io.ReadAll(f) + if err != nil { + return nil, err + } + + fi, err := f.Stat() + if err != nil { + return nil, err + } + + return &plugins.File{ + Content: b, + ModTime: fi.ModTime(), + }, nil + } else { + return nil, plugins.ErrPluginNotInstalled + } +} diff --git a/pkg/plugins/plugins.go b/pkg/plugins/plugins.go index 495128b349a3f..8fc2af80a028a 100644 --- a/pkg/plugins/plugins.go +++ b/pkg/plugins/plugins.go @@ -95,25 +95,6 @@ func (p PluginDTO) IsCorePlugin() bool { return p.Class == Core } -func (p PluginDTO) File(name string) (fs.File, error) { - cleanPath, err := util.CleanRelativePath(name) - if err != nil { - // CleanRelativePath should clean and make the path relative so this is not expected to fail - return nil, err - } - - if p.fs == nil { - return nil, ErrFileNotExist - } - - f, err := p.fs.Open(cleanPath) - if err != nil { - return nil, err - } - - return f, nil -} - // JSONData represents the plugin's plugin.json type JSONData struct { // Common settings @@ -325,6 +306,25 @@ func (p *Plugin) RunStream(ctx context.Context, req *backend.RunStreamRequest, s return pluginClient.RunStream(ctx, req, sender) } +func (p *Plugin) File(name string) (fs.File, error) { + cleanPath, err := util.CleanRelativePath(name) + if err != nil { + // CleanRelativePath should clean and make the path relative so this is not expected to fail + return nil, err + } + + if p.FS == nil { + return nil, ErrFileNotExist + } + + f, err := p.FS.Open(cleanPath) + if err != nil { + return nil, err + } + + return f, nil +} + func (p *Plugin) RegisterClient(c backendplugin.Plugin) { p.client = c } diff --git a/pkg/services/plugindashboards/service/service.go b/pkg/services/plugindashboards/service/service.go index 48dd814bd20c7..861fedc838218 100644 --- a/pkg/services/plugindashboards/service/service.go +++ b/pkg/services/plugindashboards/service/service.go @@ -5,6 +5,7 @@ import ( "fmt" "github.com/grafana/grafana/pkg/components/simplejson" + "github.com/grafana/grafana/pkg/infra/log" pluginDashboardsManager "github.com/grafana/grafana/pkg/plugins/manager/dashboards" "github.com/grafana/grafana/pkg/services/dashboards" @@ -115,13 +116,7 @@ func (s Service) LoadPluginDashboard(ctx context.Context, req *plugindashboards. return nil, err } - defer func() { - if err = resp.Content.Close(); err != nil { - s.logger.Warn("Failed to close plugin dashboard file", "reference", req.Reference, "err", err) - } - }() - - data, err := simplejson.NewFromReader(resp.Content) + data, err := simplejson.NewJson(resp.Content) if err != nil { return nil, err } diff --git a/pkg/services/plugindashboards/service/service_test.go b/pkg/services/plugindashboards/service/service_test.go index 6cfed39d71fd1..891b1ebcb0762 100644 --- a/pkg/services/plugindashboards/service/service_test.go +++ b/pkg/services/plugindashboards/service/service_test.go @@ -1,10 +1,8 @@ package service import ( - "bytes" "context" "fmt" - "io" "sort" "testing" @@ -193,7 +191,7 @@ func (m pluginDashboardStoreMock) GetPluginDashboardFileContents(ctx context.Con if dashboardFiles, exists := m.pluginDashboardFiles[args.PluginID]; exists { if content, exists := dashboardFiles[args.FileReference]; exists { return &dashboards.GetPluginDashboardFileContentsResult{ - Content: io.NopCloser(bytes.NewReader(content)), + Content: content, }, nil } } else if !exists { diff --git a/pkg/services/pluginsintegration/pluginsintegration.go b/pkg/services/pluginsintegration/pluginsintegration.go index cb114e7c9767b..535c5e419139a 100644 --- a/pkg/services/pluginsintegration/pluginsintegration.go +++ b/pkg/services/pluginsintegration/pluginsintegration.go @@ -10,6 +10,7 @@ import ( "github.com/grafana/grafana/pkg/plugins/config" "github.com/grafana/grafana/pkg/plugins/manager" "github.com/grafana/grafana/pkg/plugins/manager/client" + "github.com/grafana/grafana/pkg/plugins/manager/filestore" "github.com/grafana/grafana/pkg/plugins/manager/loader" "github.com/grafana/grafana/pkg/plugins/manager/loader/assetpath" "github.com/grafana/grafana/pkg/plugins/manager/loader/finder" @@ -60,6 +61,8 @@ var WireSet = wire.NewSet( sources.ProvideService, pluginSettings.ProvideService, wire.Bind(new(pluginsettings.Service), new(*pluginSettings.Service)), + filestore.ProvideService, + wire.Bind(new(plugins.FileStore), new(*filestore.Service)), ) // WireExtensionSet provides a wire.ProviderSet of plugin providers that can be