feat: add collector name validation in doctor (#2229)

* feat: add collector name validation in doctor

* feat: address review comments

* feat: address review comments

* feat: address review comments

cmd/poller/poller.go

@@ -76,21 +76,13 @@ import (
 
 // default params
 var (
-	pollerSchedule   = "60s"
-	logFileName      = ""
-	logMaxMegaBytes  = logging.DefaultLogMaxMegaBytes
-	logMaxBackups    = logging.DefaultLogMaxBackups
-	logMaxAge        = logging.DefaultLogMaxAge
-	asupSchedule     = "24h" // send every 24 hours
-	asupFirstWrite   = "4m"  // after this time, write 1st autosupport payload (for testing)
-	isOntapCollector = map[string]struct{}{
-		"ZapiPerf":    {},
-		"Zapi":        {},
-		"Rest":        {},
-		"RestPerf":    {},
-		"Ems":         {},
-		"StorageGrid": {},
-	}
+	pollerSchedule  = "60s"
+	logFileName     = ""
+	logMaxMegaBytes = logging.DefaultLogMaxMegaBytes
+	logMaxBackups   = logging.DefaultLogMaxBackups
+	logMaxAge       = logging.DefaultLogMaxAge
+	asupSchedule    = "24h" // send every 24 hours
+	asupFirstWrite  = "4m"  // after this time, write 1st autosupport payload (for testing)
 )
 
 const (
@@ -302,6 +294,11 @@ func (p *Poller) Init() error {
 
 	objectsToCollectors := make(map[string][]objectCollector)
 	for _, c := range filteredCollectors {
+		_, ok := util.IsCollector[c.Name]
+		if !ok {
+			logger.Error().Str("Detected invalid collector", c.Name).Msgf("Valid collectors are: %v", util.GetCollectorSlice())
+			continue
+		}
 		objects, err := p.readObjects(c)
 		if err != nil {
 			logger.Error().Err(err).
@@ -966,7 +963,7 @@ var pollerCmd = &cobra.Command{
 
 func (p *Poller) targetIsOntap() bool {
 	for _, c := range p.collectors {
-		_, ok := isOntapCollector[c.GetName()]
+		_, ok := util.IsCollector[c.GetName()]
 		if ok {
 			return true
 		}

cmd/tools/doctor/doctor.go

@@ -8,6 +8,7 @@ import (
 	"github.com/netapp/harvest/v2/pkg/conf"
 	"github.com/netapp/harvest/v2/pkg/tree"
 	harvestyaml "github.com/netapp/harvest/v2/pkg/tree/yaml"
+	"github.com/netapp/harvest/v2/pkg/util"
 	"github.com/spf13/cobra"
 	"gopkg.in/yaml.v3"
 	"io/fs"
@@ -124,6 +125,7 @@ func checkAll(path string, contents []byte) {
 	anyFailed = !checkPollersExportToUniquePromPorts(*harvestConfig).isValid || anyFailed
 	anyFailed = !checkExporterTypes(*harvestConfig).isValid || anyFailed
 	anyFailed = !checkCustomYaml("").isValid || anyFailed
+	anyFailed = !checkCollectorName(*harvestConfig).isValid || anyFailed
 
 	if anyFailed {
 		os.Exit(1)
@@ -132,6 +134,52 @@ func checkAll(path string, contents []byte) {
 	}
 }
 
+// checkCollectorName checks if the collector names in the config struct are valid
+func checkCollectorName(config conf.HarvestConfig) validation {
+	valid := validation{isValid: true}
+
+	var isDefaultCollectorExist bool
+	// Verify default collectors
+	if config.Defaults != nil {
+		defaultCollectors := config.Defaults.Collectors
+		for _, c := range defaultCollectors {
+			isDefaultCollectorExist = true
+			// Check if the collector name is valid
+			_, ok := util.IsCollector[c.Name]
+			if !ok {
+				fmt.Printf("Default Section uses an invalid collector %s \n", color.Colorize(c.Name, color.Red))
+				valid.isValid = false
+			}
+		}
+	}
+
+	var isPollerCollectorExist bool
+	// Verify poller collectors
+	for k, v := range config.Pollers {
+		for _, c := range v.Collectors {
+			isPollerCollectorExist = true
+			// Check if the collector name is valid
+			_, ok := util.IsCollector[c.Name]
+			if !ok {
+				fmt.Printf("Poller [%s] uses an invalid collector [%s] \n", color.Colorize(k, color.Yellow), color.Colorize(c.Name, color.Red))
+				valid.isValid = false
+			}
+		}
+	}
+
+	// if no collector is configured in default and poller
+	if !isDefaultCollectorExist && !isPollerCollectorExist {
+		valid.isValid = false
+	}
+
+	// Print the valid collector names if there are invalid collector names
+	if !valid.isValid {
+		fmt.Printf("Valid collector names %v \n", color.Colorize(util.GetCollectorSlice(), color.Green))
+	}
+
+	return valid
+}
+
 func checkCustomYaml(confParent string) validation {
 	valid := validation{isValid: true}
 	confDir := path.Join(conf.GetHarvestHomePath(), "conf")

cmd/tools/doctor/doctor_test.go

@@ -29,7 +29,7 @@ func assertRedacted(t *testing.T, input, redacted string) {
 }
 
 func TestConfigToStruct(t *testing.T) {
-	loadTestConfig()
+	conf.TestLoadHarvestConfig("testdata/testConfig.yml")
 	if conf.Config.Defaults.Password != "123#abc" {
 		t.Fatalf(`expected harvestConfig.Defaults.Password to be 123#abc, actual=[%+v]`,
 			conf.Config.Defaults.Addr)
@@ -74,7 +74,7 @@ func TestConfigToStruct(t *testing.T) {
 }
 
 func TestUniquePromPorts(t *testing.T) {
-	loadTestConfig()
+	conf.TestLoadHarvestConfig("testdata/testConfig.yml")
 	valid := checkUniquePromPorts(conf.Config)
 	if valid.isValid {
 		t.Fatal(`expected isValid to be false since there are duplicate prom ports, actual was isValid=true`)
@@ -85,7 +85,7 @@ func TestUniquePromPorts(t *testing.T) {
 }
 
 func TestExporterTypesAreValid(t *testing.T) {
-	loadTestConfig()
+	conf.TestLoadHarvestConfig("testdata/testConfig.yml")
 	valid := checkExporterTypes(conf.Config)
 	if valid.isValid {
 		t.Fatalf(`expected isValid to be false since there are invalid exporter types, actual was %+v`, valid)
@@ -95,14 +95,6 @@ func TestExporterTypesAreValid(t *testing.T) {
 	}
 }
 
-func loadTestConfig() {
-	path := "testdata/testConfig.yml"
-	err := conf.LoadHarvestConfig(path)
-	if err != nil {
-		panic(err)
-	}
-}
-
 func TestCustomYamlIsValid(t *testing.T) {
 	type test struct {
 		path        string
@@ -144,3 +136,42 @@ func TestCustomYamlIsValid(t *testing.T) {
 		})
 	}
 }
+
+func TestCheckCollectorName(t *testing.T) {
+	type test struct {
+		path string
+		want bool
+	}
+
+	tests := []test{
+		{
+			path: "testdata/collector/conf1.yml",
+			want: false,
+		},
+		{
+			path: "testdata/collector/conf2.yml",
+			want: false,
+		},
+		{
+			path: "testdata/collector/conf3.yml",
+			want: false,
+		},
+		{
+			path: "testdata/collector/conf4.yml",
+			want: true,
+		},
+		{
+			path: "testdata/collector/conf5.yml",
+			want: false,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.path, func(t *testing.T) {
+			conf.TestLoadHarvestConfig(tt.path)
+			valid := checkCollectorName(conf.Config)
+			if valid.isValid != tt.want {
+				t.Errorf("want isValid=%t, got %t", tt.want, valid.isValid)
+			}
+		})
+	}
+}

cmd/tools/doctor/testdata/collector/conf1.yml

@@ -0,0 +1,24 @@
+Exporters:
+  prometheus:
+    exporter: Prometheus
+    port_range: 12990-14000
+    local_http_addr: localhost
+    add_meta_tags: false
+
+Defaults:
+  use_insecure_tls: true
+  prefer_zapi: true
+  collectors:
+    - rest
+
+Pollers:
+
+  u2:
+    datacenter: u2
+    addr: 1.1.1.1
+    username: username
+    password: password
+    collectors:
+      - Zapi
+    exporters:
+      - prometheus

cmd/tools/doctor/testdata/collector/conf2.yml

@@ -0,0 +1,24 @@
+Exporters:
+  prometheus:
+    exporter: Prometheus
+    port_range: 12990-14000
+    local_http_addr: localhost
+    add_meta_tags: false
+
+Defaults:
+  use_insecure_tls: true
+  prefer_zapi: true
+  collectors:
+    - Rest
+
+Pollers:
+
+  u2:
+    datacenter: u2
+    addr: 1.1.1.1
+    username: username
+    password: password
+    collectors:
+      - zapi
+    exporters:
+      - prometheus

cmd/tools/doctor/testdata/collector/conf3.yml

@@ -0,0 +1,24 @@
+Exporters:
+  prometheus:
+    exporter: Prometheus
+    port_range: 12990-14000
+    local_http_addr: localhost
+    add_meta_tags: false
+
+Defaults:
+  use_insecure_tls: true
+  prefer_zapi: true
+  collectors:
+    - rest
+
+Pollers:
+
+  u2:
+    datacenter: u2
+    addr: 1.1.1.1
+    username: username
+    password: password
+    collectors:
+      - zapi
+    exporters:
+      - prometheus

cmd/tools/doctor/testdata/collector/conf4.yml

@@ -0,0 +1,24 @@
+Exporters:
+  prometheus:
+    exporter: Prometheus
+    port_range: 12990-14000
+    local_http_addr: localhost
+    add_meta_tags: false
+
+Defaults:
+  use_insecure_tls: true
+  prefer_zapi: true
+  collectors:
+    - Rest
+
+Pollers:
+
+  u2:
+    datacenter: u2
+    addr: 1.1.1.1
+    username: username
+    password: password
+    collectors:
+      - Zapi
+    exporters:
+      - prometheus

cmd/tools/doctor/testdata/collector/conf5.yml

@@ -0,0 +1,20 @@
+Exporters:
+  prometheus:
+    exporter: Prometheus
+    port_range: 12990-14000
+    local_http_addr: localhost
+    add_meta_tags: false
+
+Defaults:
+  use_insecure_tls: true
+  prefer_zapi: true
+
+Pollers:
+
+  u2:
+    datacenter: u2
+    addr: 1.1.1.1
+    username: username
+    password: password
+    exporters:
+      - prometheus

pkg/conf/conf.go

@@ -34,6 +34,7 @@ const (
 // TestLoadHarvestConfig is used by testing code to reload a new config
 func TestLoadHarvestConfig(configPath string) {
 	configRead = false
+	Config = HarvestConfig{}
 	promPortRangeMapping = make(map[string]PortMap)
 	err := LoadHarvestConfig(configPath)
 	if err != nil {

pkg/util/util.go

@@ -20,6 +20,25 @@ import (
 	"strings"
 )
 
+var IsCollector = map[string]struct{}{
+	"ZapiPerf":    {},
+	"Zapi":        {},
+	"Rest":        {},
+	"RestPerf":    {},
+	"Ems":         {},
+	"StorageGrid": {},
+	"Unix":        {},
+	"Simple":      {},
+}
+
+func GetCollectorSlice() []string {
+	keys := make([]string, 0, len(IsCollector))
+	for k := range IsCollector {
+		keys = append(keys, k)
+	}
+	return keys
+}
+
 func MinLen(elements [][]string) int {
 	var min, i int
 	min = len(elements[0])