filestorageextension: fix panic when configured directory cannot be a…

…ccessed (#6103)

* filestorageextension: fix panic when configured directory cannot be accessed

* filestorageextension: move validation to config.Validate()

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
+}

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")
+}

extension/storage/filestorage/extension.go

@@ -17,7 +17,6 @@ package filestorage // import "github.com/open-telemetry/opentelemetry-collector
 import (
 	"context"
 	"fmt"
-	"os"
 	"path/filepath"
 	"time"
 
@@ -37,11 +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 && os.IsNotExist(err)) || !info.IsDir() {
-		return nil, fmt.Errorf("directory must exist: %v", err)
-	}
-
 	return &localFileStorage{
 		directory: filepath.Clean(config.Directory),
 		timeout:   config.Timeout,

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)

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)

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: