From 0f4a76c9de9c2f3d8835689c58af1cd9f76e8fd3 Mon Sep 17 00:00:00 2001 From: "Grot (@grafanabot)" <43478413+grafanabot@users.noreply.github.com> Date: Thu, 13 Apr 2023 12:41:26 +0200 Subject: [PATCH] [v9.5.x] Plugins: Fix module.js file not being closed when loading plugins (#66435) * Plugins: Fix module.js file not being closed when loading plugins (#66288) * Plugins: Loader: Fix module.js file not being closed * Plugins: LocalFS: Add comments, ensure same Read() behaviour as os.File's * Changed comment for Close() * Add tests for LocalFS * "Fix" linter error * "Fix" linter error again (cherry picked from commit 1c3ad81826fa023b39cce8414ea5a62771731cc3) * trigger ci --------- Co-authored-by: Giuseppe Guerra Co-authored-by: Giuseppe Guerra --- pkg/plugins/localfiles.go | 36 ++++++- pkg/plugins/localfiles_test.go | 143 +++++++++++++++++++++++++++ pkg/plugins/manager/loader/loader.go | 6 +- 3 files changed, 181 insertions(+), 4 deletions(-) create mode 100644 pkg/plugins/localfiles_test.go diff --git a/pkg/plugins/localfiles.go b/pkg/plugins/localfiles.go index a805ae5d43f..92708796a90 100644 --- a/pkg/plugins/localfiles.go +++ b/pkg/plugins/localfiles.go @@ -12,11 +12,18 @@ import ( var _ fs.FS = (*LocalFS)(nil) +// LocalFS is a plugins.FS that allows accessing files on the local file system. type LocalFS struct { - m map[string]*LocalFile + // m is a map of relative file paths that can be accessed on the local filesystem. + // The path separator must be os-specific. + m map[string]*LocalFile + + // basePath is the basePath that will be prepended to all the files (in m map) before accessing them. basePath string } +// NewLocalFS returns a new LocalFS that can access the specified files in the specified base path. +// Both the map keys and basePath should use the os-specific path separator for Open() to work properly. func NewLocalFS(m map[string]struct{}, basePath string) LocalFS { pfs := make(map[string]*LocalFile, len(m)) for k := range m { @@ -31,6 +38,8 @@ func NewLocalFS(m map[string]struct{}, basePath string) LocalFS { } } +// Open opens the specified file on the local filesystem, and returns the corresponding fs.File. +// If a nil error is returned, the caller should take care of closing the returned file. func (f LocalFS) Open(name string) (fs.File, error) { cleanPath, err := util.CleanRelativePath(name) if err != nil { @@ -53,10 +62,13 @@ func (f LocalFS) Open(name string) (fs.File, error) { return nil, ErrFileNotExist } +// Base returns the base path for the LocalFS. func (f LocalFS) Base() string { return f.basePath } +// Files returns a slice of all the file paths in the LocalFS relative to the base path. +// The returned strings use the same path separator as the func (f LocalFS) Files() []string { var files []string for p := range f.m { @@ -72,11 +84,14 @@ func (f LocalFS) Files() []string { var _ fs.File = (*LocalFile)(nil) +// LocalFile implements a fs.File for accessing the local filesystem. type LocalFile struct { f *os.File path string } +// Stat returns a FileInfo describing the named file. +// It returns ErrFileNotExist if the file does not exist, or ErrPluginFileRead if another error occurs. func (p *LocalFile) Stat() (fs.FileInfo, error) { fi, err := os.Stat(p.path) if err != nil { @@ -88,7 +103,19 @@ func (p *LocalFile) Stat() (fs.FileInfo, error) { return fi, nil } -func (p *LocalFile) Read(bytes []byte) (int, error) { +// Read reads up to len(b) bytes from the File and stores them in b. +// It returns the number of bytes read and any error encountered. +// At end of file, Read returns 0, io.EOF. +// If the file is already open, it is opened again, without closing it first. +// The file is not closed at the end of the read operation. If a non-nil error is returned, it +// must be manually closed by the caller by calling Close(). +func (p *LocalFile) Read(b []byte) (int, error) { + if p.f != nil { + // File is already open, Read() can be called more than once. + // io.EOF is returned if the file has been read entirely. + return p.f.Read(b) + } + var err error p.f, err = os.Open(p.path) if err != nil { @@ -97,9 +124,12 @@ func (p *LocalFile) Read(bytes []byte) (int, error) { } return 0, ErrPluginFileRead } - return p.f.Read(bytes) + return p.f.Read(b) } +// Close closes the file. +// If the file was never open, nil is returned. +// If the file is already closed, an error is returned. func (p *LocalFile) Close() error { if p.f != nil { return p.f.Close() diff --git a/pkg/plugins/localfiles_test.go b/pkg/plugins/localfiles_test.go new file mode 100644 index 00000000000..ed66190c048 --- /dev/null +++ b/pkg/plugins/localfiles_test.go @@ -0,0 +1,143 @@ +package plugins + +import ( + "io" + "os" + "testing" + + "github.com/stretchr/testify/require" +) + +type tempFileScenario struct { + filePath string +} + +func (s tempFileScenario) cleanup() error { + return os.Remove(s.filePath) +} + +func (s tempFileScenario) newLocalFile() LocalFile { + return LocalFile{path: s.filePath} +} + +func newTempFileScenario() (tempFileScenario, error) { + tf, err := os.CreateTemp(os.TempDir(), "*") + if err != nil { + return tempFileScenario{}, err + } + defer tf.Close() //nolint + if _, err := tf.Write([]byte("hello\n")); err != nil { + return tempFileScenario{}, err + } + return tempFileScenario{ + filePath: tf.Name(), + }, nil +} + +func newTempFileScenarioForTest(t *testing.T) tempFileScenario { + s, err := newTempFileScenario() + require.NoError(t, err) + t.Cleanup(func() { + require.NoError(t, s.cleanup()) + }) + return s +} + +func TestLocalFile_Read(t *testing.T) { + t.Run("not exists", func(t *testing.T) { + var out []byte + f := LocalFile{path: "does not exist"} + n, err := f.Read(out) + require.Zero(t, n) + require.Equal(t, ErrFileNotExist, err) + }) + + t.Run("read", func(t *testing.T) { + t.Run("extra", func(t *testing.T) { + s := newTempFileScenarioForTest(t) + f := s.newLocalFile() + + const bufSize = 512 + out := make([]byte, bufSize) + n, err := f.Read(out) + require.NoError(t, err) + require.NoError(t, f.Close()) + const exp = "hello\n" + require.Equal(t, len(exp), n) + require.Equal(t, []byte(exp), out[:len(exp)]) + require.Equal(t, make([]byte, bufSize-len(exp)), out[len(exp):]) + }) + + t.Run("empty", func(t *testing.T) { + s := newTempFileScenarioForTest(t) + f := s.newLocalFile() + + var out []byte + n, err := f.Read(out) + require.NoError(t, err) + require.NoError(t, f.Close()) + require.Zero(t, n) + require.Empty(t, out) + }) + + t.Run("multiple", func(t *testing.T) { + s := newTempFileScenarioForTest(t) + f := s.newLocalFile() + + a := make([]byte, 2) + b := make([]byte, 3) + c := make([]byte, 2) + + t.Cleanup(func() { + require.NoError(t, f.Close()) + }) + + n, err := f.Read(a) + require.NoError(t, err) + require.Equal(t, 2, n) + require.Equal(t, []byte("he"), a) + + n, err = f.Read(b) + require.NoError(t, err) + require.Equal(t, 3, n) + require.Equal(t, []byte("llo"), b) + + n, err = f.Read(c) + require.NoError(t, err) + require.Equal(t, 1, n) + require.Equal(t, []byte{'\n', 0}, c) + + n, err = f.Read(c) + require.Zero(t, n) + require.Equal(t, io.EOF, err) + }) + }) +} + +func TestLocalFile_Close(t *testing.T) { + t.Run("once after read", func(t *testing.T) { + s := newTempFileScenarioForTest(t) + f := s.newLocalFile() + + _, err := f.Read(nil) + require.NoError(t, err) + require.NoError(t, f.Close()) + }) + + t.Run("never opened", func(t *testing.T) { + s := newTempFileScenarioForTest(t) + f := s.newLocalFile() + + require.NoError(t, f.Close()) + }) + + t.Run("twice", func(t *testing.T) { + s := newTempFileScenarioForTest(t) + f := s.newLocalFile() + + _, err := f.Read(nil) + require.NoError(t, err) + require.NoError(t, f.Close()) + require.Error(t, f.Close()) + }) +} diff --git a/pkg/plugins/manager/loader/loader.go b/pkg/plugins/manager/loader/loader.go index 558d0cc6efb..700381f9b59 100644 --- a/pkg/plugins/manager/loader/loader.go +++ b/pkg/plugins/manager/loader/loader.go @@ -152,12 +152,16 @@ func (l *Loader) loadPlugins(ctx context.Context, src plugins.PluginSource, foun // verify module.js exists for SystemJS to load. // CDN plugins can be loaded with plugin.json only, so do not warn for those. if !plugin.IsRenderer() && !plugin.IsCorePlugin() { - _, err := plugin.FS.Open("module.js") + f, err := plugin.FS.Open("module.js") if err != nil { if errors.Is(err, plugins.ErrFileNotExist) && !l.pluginsCDN.PluginSupported(plugin.ID) { l.log.Warn("Plugin missing module.js", "pluginID", plugin.ID, "warning", "Missing module.js, If you loaded this plugin from git, make sure to compile it.") } + } else if f != nil { + if err := f.Close(); err != nil { + l.log.Warn("Could not close module.js", "pluginID", plugin.ID, "err", err) + } } }