From 53d4d7499c1ba41110ec0acd014589b6e377d75d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20Ma=C5=82ek?= Date: Tue, 2 Nov 2021 17:15:25 +0100 Subject: [PATCH 1/2] filestorageextension: fix panic when configured directory cannot be accessed --- extension/storage/filestorage/extension.go | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/extension/storage/filestorage/extension.go b/extension/storage/filestorage/extension.go index 5a3d151030d1..e00e6040d35c 100644 --- a/extension/storage/filestorage/extension.go +++ b/extension/storage/filestorage/extension.go @@ -17,6 +17,7 @@ package filestorage // import "github.com/open-telemetry/opentelemetry-collector import ( "context" "fmt" + "io/fs" "os" "path/filepath" "time" @@ -38,8 +39,20 @@ var _ storage.Extension = (*localFileStorage)(nil) func newLocalFileStorage(logger *zap.Logger, config *Config) (component.Extension, error) { info, err := os.Stat(config.Directory) - if (err != nil && os.IsNotExist(err)) || !info.IsDir() { - return nil, fmt.Errorf("directory must exist: %v", err) + if err != nil { + if os.IsNotExist(err) { + return nil, fmt.Errorf("directory must exist: %v", err) + } + if fsErr, ok := err.(*fs.PathError); ok { + return nil, fmt.Errorf( + "problem accessing configured directory: %s, err: %v", + config.Directory, fsErr, + ) + } + + } + if !info.IsDir() { + return nil, fmt.Errorf("%s is not a directory: %v", config.Directory, err) } return &localFileStorage{ From d45068b661213c85ed319682010da8caf40e8588 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20Ma=C5=82ek?= Date: Tue, 2 Nov 2021 19:55:57 +0100 Subject: [PATCH 2/2] filestorageextension: move validation to config.Validate() --- extension/storage/filestorage/config.go | 24 +++++++++++ extension/storage/filestorage/config_test.go | 41 +++++++++++++++++-- extension/storage/filestorage/extension.go | 19 --------- .../storage/filestorage/extension_test.go | 10 ----- extension/storage/filestorage/factory_test.go | 16 +------- .../storage/filestorage/testdata/config.yaml | 6 ++- 6 files changed, 68 insertions(+), 48 deletions(-) diff --git a/extension/storage/filestorage/config.go b/extension/storage/filestorage/config.go index a384583280dc..595791a86466 100644 --- a/extension/storage/filestorage/config.go +++ b/extension/storage/filestorage/config.go @@ -15,6 +15,9 @@ package filestorage // import "github.com/open-telemetry/opentelemetry-collector-contrib/extension/storage/filestorage" import ( + "fmt" + "io/fs" + "os" "time" "go.opentelemetry.io/collector/config" @@ -27,3 +30,24 @@ type Config struct { Directory string `mapstructure:"directory,omitempty"` Timeout time.Duration `mapstructure:"timeout,omitempty"` } + +func (cfg *Config) Validate() error { + info, err := os.Stat(cfg.Directory) + if err != nil { + if os.IsNotExist(err) { + return fmt.Errorf("directory must exist: %v", err) + } + if fsErr, ok := err.(*fs.PathError); ok { + return fmt.Errorf( + "problem accessing configured directory: %s, err: %v", + cfg.Directory, fsErr, + ) + } + + } + if !info.IsDir() { + return fmt.Errorf("%s is not a directory", cfg.Directory) + } + + return nil +} diff --git a/extension/storage/filestorage/config_test.go b/extension/storage/filestorage/config_test.go index d4606761e295..4366d4194a2a 100644 --- a/extension/storage/filestorage/config_test.go +++ b/extension/storage/filestorage/config_test.go @@ -15,6 +15,7 @@ package filestorage import ( + "os" "path" "testing" "time" @@ -34,20 +35,54 @@ func TestLoadConfig(t *testing.T) { factories.Extensions[typeStr] = factory cfg, err := configtest.LoadConfigAndValidate(path.Join(".", "testdata", "config.yaml"), factories) - require.Nil(t, err) + require.NoError(t, err) require.NotNil(t, cfg) require.Len(t, cfg.Extensions, 2) ext0 := cfg.Extensions[config.NewComponentID(typeStr)] - assert.Equal(t, factory.CreateDefaultConfig(), ext0) + defaultConfig := factory.CreateDefaultConfig() + defaultConfigFileStorage, ok := defaultConfig.(*Config) + require.True(t, ok) + // Specify a directory so that tests will pass because on some systems the + // default dir (e.g. on not windows: /var/lib/otelcol/file_storage) might not + // exist which will fail the test when config.Validate() will be called. + defaultConfigFileStorage.Directory = "." + assert.Equal(t, defaultConfig, ext0) ext1 := cfg.Extensions[config.NewComponentIDWithName(typeStr, "all_settings")] assert.Equal(t, &Config{ ExtensionSettings: config.NewExtensionSettings(config.NewComponentIDWithName(typeStr, "all_settings")), - Directory: "/var/lib/otelcol/mydir", + Directory: ".", Timeout: 2 * time.Second, }, ext1) } + +func TestHandleNonExistingDirectoryWithAnError(t *testing.T) { + f := NewFactory() + cfg := f.CreateDefaultConfig().(*Config) + cfg.Directory = "/not/a/dir" + + err := cfg.Validate() + require.Error(t, err) + require.EqualError(t, err, "directory must exist: stat /not/a/dir: no such file or directory") +} + +func TestHandleProvidingFilePathAsDirWithAnError(t *testing.T) { + f := NewFactory() + cfg := f.CreateDefaultConfig().(*Config) + + file, err := os.CreateTemp("", "") + require.NoError(t, err) + t.Cleanup(func() { + require.NoError(t, os.Remove(file.Name())) + }) + + cfg.Directory = file.Name() + + err = cfg.Validate() + require.Error(t, err) + require.EqualError(t, err, file.Name()+" is not a directory") +} diff --git a/extension/storage/filestorage/extension.go b/extension/storage/filestorage/extension.go index e00e6040d35c..afbff1797e0c 100644 --- a/extension/storage/filestorage/extension.go +++ b/extension/storage/filestorage/extension.go @@ -17,8 +17,6 @@ package filestorage // import "github.com/open-telemetry/opentelemetry-collector import ( "context" "fmt" - "io/fs" - "os" "path/filepath" "time" @@ -38,23 +36,6 @@ type localFileStorage struct { var _ storage.Extension = (*localFileStorage)(nil) func newLocalFileStorage(logger *zap.Logger, config *Config) (component.Extension, error) { - info, err := os.Stat(config.Directory) - if err != nil { - if os.IsNotExist(err) { - return nil, fmt.Errorf("directory must exist: %v", err) - } - if fsErr, ok := err.(*fs.PathError); ok { - return nil, fmt.Errorf( - "problem accessing configured directory: %s, err: %v", - config.Directory, fsErr, - ) - } - - } - if !info.IsDir() { - return nil, fmt.Errorf("%s is not a directory: %v", config.Directory, err) - } - return &localFileStorage{ directory: filepath.Clean(config.Directory), timeout: config.Timeout, diff --git a/extension/storage/filestorage/extension_test.go b/extension/storage/filestorage/extension_test.go index c386da475f8e..91c7600cdd5d 100644 --- a/extension/storage/filestorage/extension_test.go +++ b/extension/storage/filestorage/extension_test.go @@ -143,16 +143,6 @@ func TestClientHandlesSimpleCases(t *testing.T) { } -func TestNewExtensionErrorsOnMissingDirectory(t *testing.T) { - f := NewFactory() - cfg := f.CreateDefaultConfig().(*Config) - cfg.Directory = "/not/a/dir" - - extension, err := f.CreateExtension(context.Background(), componenttest.NewNopExtensionCreateSettings(), cfg) - require.Error(t, err) - require.Nil(t, extension) -} - func TestTwoClientsWithDifferentNames(t *testing.T) { ctx := context.Background() se := newTestExtension(t) diff --git a/extension/storage/filestorage/factory_test.go b/extension/storage/filestorage/factory_test.go index 97a09ca0eca1..f570406d558b 100644 --- a/extension/storage/filestorage/factory_test.go +++ b/extension/storage/filestorage/factory_test.go @@ -50,20 +50,6 @@ func TestFactory(t *testing.T) { wantErr bool wantErrMessage string }{ - { - name: "Default", - config: cfg, - wantErr: true, - wantErrMessage: "directory must exist", - }, - { - name: "Invalid directory", - config: &Config{ - Directory: "/not/very/likely/a/real/dir", - }, - wantErr: true, - wantErrMessage: "directory must exist", - }, { name: "Default", config: func() *Config { @@ -83,10 +69,10 @@ func TestFactory(t *testing.T) { test.config, ) if test.wantErr { + require.Error(t, err) if test.wantErrMessage != "" { require.True(t, strings.HasPrefix(err.Error(), test.wantErrMessage)) } - require.Error(t, err) require.Nil(t, e) } else { require.NoError(t, err) diff --git a/extension/storage/filestorage/testdata/config.yaml b/extension/storage/filestorage/testdata/config.yaml index 3f97797f8862..9e997042fac2 100644 --- a/extension/storage/filestorage/testdata/config.yaml +++ b/extension/storage/filestorage/testdata/config.yaml @@ -1,7 +1,11 @@ extensions: file_storage: + # Specify a directory so that tests will pass because on some systems the + # default dir (e.g. on not windows: /var/lib/otelcol/file_storage) might not + # exist which will fail the test when config.Validate() will be called. + directory: . file_storage/all_settings: - directory: /var/lib/otelcol/mydir + directory: . timeout: 2s service: