fix: ensure folders do not get loaded more than once (#10551)

internal/snmp/testdata/loadMibsFromPath/root/symlink

@@ -0,0 +1 @@
+../linkTarget/

internal/snmp/translate.go

@@ -2,6 +2,7 @@ package snmp
 
 import (
 	"fmt"
+	"io/ioutil"
 	"os"
 	"path/filepath"
 	"strings"
@@ -18,14 +19,21 @@ var m sync.Mutex
 var once sync.Once
 var cache = make(map[string]bool)
 
-func appendPath(path string) {
+type MibLoader interface {
+	loadModule(path string) error
+	appendPath(path string)
+}
+
+type GosmiMibLoader struct{}
+
+func (*GosmiMibLoader) appendPath(path string) {
 	m.Lock()
 	defer m.Unlock()
 
 	gosmi.AppendPath(path)
 }
 
-func loadModule(path string) error {
+func (*GosmiMibLoader) loadModule(path string) error {
 	m.Lock()
 	defer m.Unlock()
 
@@ -37,13 +45,51 @@ func ClearCache() {
 	cache = make(map[string]bool)
 }
 
-func LoadMibsFromPath(paths []string, log telegraf.Logger) error {
+//will give all found folders to gosmi and load in all modules found in the folders
+func LoadMibsFromPath(paths []string, log telegraf.Logger, loader MibLoader) error {
+	folders, err := walkPaths(paths, log)
+	if err != nil {
+		return err
+	}
+	for _, path := range folders {
+		loader.appendPath(path)
+		modules, err := ioutil.ReadDir(path)
+		if err != nil {
+			log.Warnf("Can't read directory %v", modules)
+		}
+
+		for _, info := range modules {
+			if info.Mode()&os.ModeSymlink != 0 {
+				target, err := filepath.EvalSymlinks(path)
+				if err != nil {
+					log.Warnf("Bad symbolic link %v", target)
+					continue
+				}
+				info, err = os.Lstat(filepath.Join(path, target))
+				if err != nil {
+					log.Warnf("Couldn't stat target %v", target)
+					continue
+				}
+				path = target
+			}
+			if info.Mode().IsRegular() {
+				err := loader.loadModule(info.Name())
+				if err != nil {
+					log.Warnf("module %v could not be loaded", info.Name())
+					continue
+				}
+			}
+		}
+	}
+	return nil
+}
+
+//should walk the paths given and find all folders
+func walkPaths(paths []string, log telegraf.Logger) ([]string, error) {
 	once.Do(gosmi.Init)
-	modules := []string{}
+	folders := []string{}
 
 	for _, mibPath := range paths {
-		folders := []string{}
-
 		// Check if we loaded that path already and skip it if so
 		m.Lock()
 		cached := cache[mibPath]
@@ -53,7 +99,6 @@ func LoadMibsFromPath(paths []string, log telegraf.Logger) error {
 			continue
 		}
 
-		appendPath(mibPath)
 		err := filepath.Walk(mibPath, func(path string, info os.FileInfo, err error) error {
 			if info == nil {
 				log.Warnf("No mibs found")
@@ -64,44 +109,29 @@ func LoadMibsFromPath(paths []string, log telegraf.Logger) error {
 				}
 				return nil
 			}
-			folders = append(folders, mibPath)
-			// symlinks are files so we need to double check if any of them are folders
-			// Will check file vs directory later on
+
 			if info.Mode()&os.ModeSymlink != 0 {
-				link, err := os.Readlink(path)
+				target, err := filepath.EvalSymlinks(path)
 				if err != nil {
-					log.Warnf("Bad symbolic link %v", link)
+					log.Warnf("Could not evaluate link %v", target)
 				}
-				folders = append(folders, link)
+				info, err = os.Lstat(target)
+				if err != nil {
+					log.Warnf("Couldn't stat target %v", path)
+				}
+				path = target
 			}
+			if info.IsDir() {
+				folders = append(folders, path)
+			}
+
 			return nil
 		})
 		if err != nil {
-			return fmt.Errorf("Filepath %q could not be walked: %v", mibPath, err)
-		}
-
-		for _, folder := range folders {
-			err := filepath.Walk(folder, func(path string, info os.FileInfo, err error) error {
-				// checks if file or directory
-				if info.IsDir() {
-					appendPath(path)
-				} else if info.Mode()&os.ModeSymlink == 0 {
-					modules = append(modules, info.Name())
-				}
-				return nil
-			})
-			if err != nil {
-				return fmt.Errorf("Filepath could not be walked: %v", err)
-			}
+			return folders, fmt.Errorf("Filepath %q could not be walked: %v", mibPath, err)
 		}
 	}
-	for _, module := range modules {
-		err := loadModule(module)
-		if err != nil {
-			log.Warnf("module %v could not be loaded", module)
-		}
-	}
-	return nil
+	return folders, nil
 }
 
 // The following is for snmp_trap

internal/snmp/translate_test.go

@@ -1,6 +1,8 @@
 package snmp
 
 import (
+	"path/filepath"
+	"runtime"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -46,7 +48,7 @@ func TestTrapLookup(t *testing.T) {
 	}
 
 	// Load the MIBs
-	require.NoError(t, LoadMibsFromPath([]string{"testdata/mibs"}, testutil.Logger{}))
+	require.NoError(t, LoadMibsFromPath([]string{"testdata/mibs"}, testutil.Logger{}, &GosmiMibLoader{}))
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -77,7 +79,7 @@ func TestTrapLookupFail(t *testing.T) {
 	}
 
 	// Load the MIBs
-	require.NoError(t, LoadMibsFromPath([]string{"testdata/mibs"}, testutil.Logger{}))
+	require.NoError(t, LoadMibsFromPath([]string{"testdata/mibs"}, testutil.Logger{}, &GosmiMibLoader{}))
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -87,3 +89,61 @@ func TestTrapLookupFail(t *testing.T) {
 		})
 	}
 }
+
+type TestingMibLoader struct {
+	folders []string
+	files   []string
+}
+
+func (t *TestingMibLoader) appendPath(path string) {
+	t.folders = append(t.folders, path)
+}
+
+func (t *TestingMibLoader) loadModule(path string) error {
+	t.files = append(t.files, path)
+	return nil
+}
+func TestFolderLookup(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("Skipping on windows")
+	}
+	var folders []string
+	var givenPath []string
+
+	tests := []struct {
+		name    string
+		mibPath [][]string
+		paths   [][]string
+		files   []string
+	}{
+		{
+			name:    "loading folders",
+			mibPath: [][]string{{"testdata", "loadMibsFromPath", "root"}},
+			paths: [][]string{
+				{"testdata", "loadMibsFromPath", "root"},
+				{"testdata", "loadMibsFromPath", "root", "dirOne"},
+				{"testdata", "loadMibsFromPath", "root", "dirOne", "dirTwo"},
+				{"testdata", "loadMibsFromPath", "linkTarget"},
+			},
+			files: []string{"empty", "emptyFile"},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			loader := TestingMibLoader{}
+			for _, paths := range tt.mibPath {
+				rootPath := filepath.Join(paths...)
+				givenPath = append(givenPath, rootPath)
+			}
+			err := LoadMibsFromPath(givenPath, testutil.Logger{}, &loader)
+			require.NoError(t, err)
+			for _, pathSlice := range tt.paths {
+				path := filepath.Join(pathSlice...)
+				folders = append(folders, path)
+			}
+			require.Equal(t, folders, loader.folders)
+			require.Equal(t, tt.files, loader.files)
+		})
+	}
+}

plugins/inputs/snmp/snmp.go

@@ -100,7 +100,7 @@ type Snmp struct {
 }
 
 func (s *Snmp) Init() error {
-	err := snmp.LoadMibsFromPath(s.Path, s.Log)
+	err := snmp.LoadMibsFromPath(s.Path, s.Log, &snmp.GosmiMibLoader{})
 	if err != nil {
 		return err
 	}

plugins/inputs/snmp/snmp_test.go

@@ -129,6 +129,7 @@ func TestFieldInit(t *testing.T) {
 		ClientConfig: snmp.ClientConfig{
 			Path: []string{testDataPath},
 		},
+		Log: &testutil.Logger{},
 	}
 
 	err = s.Init()
@@ -1313,7 +1314,7 @@ func BenchmarkMibLoading(b *testing.B) {
 	log := testutil.Logger{}
 	path := []string{"testdata"}
 	for i := 0; i < b.N; i++ {
-		err := snmp.LoadMibsFromPath(path, log)
+		err := snmp.LoadMibsFromPath(path, log, &snmp.GosmiMibLoader{})
 		require.NoError(b, err)
 	}
 }
@@ -1332,6 +1333,6 @@ func TestCanNotParse(t *testing.T) {
 func TestMissingMibPath(t *testing.T) {
 	log := testutil.Logger{}
 	path := []string{"non-existing-directory"}
-	err := snmp.LoadMibsFromPath(path, log)
+	err := snmp.LoadMibsFromPath(path, log, &snmp.GosmiMibLoader{})
 	require.NoError(t, err)
 }

plugins/inputs/snmp_trap/snmp_trap.go

@@ -99,7 +99,7 @@ func init() {
 }
 
 func (s *SnmpTrap) Init() error {
-	err := snmp.LoadMibsFromPath(s.Path, s.Log)
+	err := snmp.LoadMibsFromPath(s.Path, s.Log, &snmp.GosmiMibLoader{})
 	if err != nil {
 		s.Log.Errorf("Could not get path %v", err)
 	}