Add config validation for all components (#3952)

* Add config validation to all armada components * Make invalid config error and remove required tag from unrequired fields --------- Co-authored-by: Eleanor Pratt <[email protected]>
armadaproject · Sep 18, 2024 · 549c5b9 · 549c5b9
1 parent f45aefa
commit 549c5b9
Show file tree

Hide file tree

Showing 15 changed files with 123 additions and 51 deletions.
diff --git a/cmd/scheduler/cmd/root.go b/cmd/scheduler/cmd/root.go
@@ -42,7 +42,7 @@ func loadConfig() (schedulerconfig.Configuration, error) {
 	common.LoadConfig(&config, "./config/scheduler", viper.GetStringSlice(CustomConfigLocation))
 	err := config.Validate()
 	if err != nil {
-		commonconfig.LogValidationErrors(err)
+		return config, commonconfig.FormatValidationErrors(err)
 	}
 	return config, err
 }
diff --git a/internal/binoculars/configuration/validation.go b/internal/binoculars/configuration/validation.go
@@ -0,0 +1,8 @@
+package configuration
+
+import "github.com/go-playground/validator/v10"
+
+func (c BinocularsConfig) Validate() error {
+	validate := validator.New()
+	return validate.Struct(c)
+}
diff --git a/internal/common/config/pulsar.go b/internal/common/config/pulsar.go
@@ -34,7 +34,7 @@ type PulsarConfig struct {
 	// Maximum allowed message size in bytes
 	MaxAllowedMessageSize uint
 	// Timeout when sending messages asynchronously
-	SendTimeout time.Duration `validate:"required"`
+	SendTimeout time.Duration
 	// Backoff from polling when Pulsar returns an error
 	BackoffTime time.Duration
 	// Number of pulsar messages that will be queued by the pulsar consumer.

diff --git a/internal/common/config/validation.go b/internal/common/config/validation.go
@@ -1,25 +1,30 @@
 package config
 
 import (
+	"errors"
+	"fmt"
 	"strings"
 
 	"github.com/go-playground/validator/v10"
-	log "github.com/sirupsen/logrus"
 )
 
-func LogValidationErrors(err error) {
-	if err != nil {
-		for _, err := range err.(validator.ValidationErrors) {
-			fieldName := stripPrefix(err.Namespace())
-			tag := err.Tag()
-			switch tag {
-			case "required":
-				log.Errorf("ConfigError: Field %s is required but was not found", fieldName)
-			default:
-				log.Errorf("ConfigError: Field %s has invalid value %s: %s", fieldName, err.Value(), tag)
-			}
+type Config interface {
+	Validate() error
+}
+
+func FormatValidationErrors(err error) error {
+	var validationErrors error
+	for _, err := range err.(validator.ValidationErrors) {
+		fieldName := stripPrefix(err.Namespace())
+		tag := err.Tag()
+		switch tag {
+		case "required":
+			validationErrors = errors.Join(validationErrors, fmt.Errorf("ConfigError: Field %s is required but was not found", fieldName))
+		default:
+			validationErrors = errors.Join(validationErrors, fmt.Errorf("ConfigError: Field %s has invalid value %s: %s", fieldName, err.Value(), tag))
 		}
 	}
+	return validationErrors
 }
 
 func stripPrefix(s string) string {

diff --git a/internal/common/grpc/configuration/types.go b/internal/common/grpc/configuration/types.go
@@ -5,7 +5,7 @@ import (
 )
 
 type GrpcConfig struct {
-	Port                       int `validate:"required"`
+	Port                       int
 	KeepaliveParams            keepalive.ServerParameters
 	KeepaliveEnforcementPolicy keepalive.EnforcementPolicy
 	Tls                        TlsConfig

diff --git a/internal/common/startup.go b/internal/common/startup.go
@@ -42,7 +42,7 @@ func BindCommandlineArguments() {
 }
 
 // TODO Move code relating to config out of common into a new package internal/serverconfig
-func LoadConfig(config any, defaultPath string, overrideConfigs []string) *viper.Viper {
+func LoadConfig(config commonconfig.Config, defaultPath string, overrideConfigs []string) *viper.Viper {
 	v := viper.NewWithOptions(viper.KeyDelimiter("::"))
 	v.SetConfigName(baseConfigFileName)
 	v.AddConfigPath(defaultPath)
@@ -89,6 +89,11 @@ func LoadConfig(config any, defaultPath string, overrideConfigs []string) *viper
 		log.Debugf("Unset keys: %v", metadata.Unset)
 	}
 
+	if err := config.Validate(); err != nil {
+		log.Error(commonconfig.FormatValidationErrors(err))
+		os.Exit(-1)
+	}
+
 	return v
 }
 

diff --git a/internal/eventingester/configuration/validation.go b/internal/eventingester/configuration/validation.go
@@ -0,0 +1,10 @@
+package configuration
+
+import (
+	"github.com/go-playground/validator/v10"
+)
+
+func (c EventIngesterConfiguration) Validate() error {
+	validate := validator.New()
+	return validate.Struct(c)
+}
diff --git a/internal/executor/configuration/validation.go b/internal/executor/configuration/validation.go
@@ -0,0 +1,8 @@
+package configuration
+
+import "github.com/go-playground/validator/v10"
+
+func (c ExecutorConfiguration) Validate() error {
+	validate := validator.New()
+	return validate.Struct(c)
+}
diff --git a/internal/lookoutingesterv2/configuration/validation.go b/internal/lookoutingesterv2/configuration/validation.go
@@ -0,0 +1,8 @@
+package configuration
+
+import "github.com/go-playground/validator/v10"
+
+func (c LookoutIngesterV2Configuration) Validate() error {
+	validate := validator.New()
+	return validate.Struct(c)
+}
diff --git a/internal/lookoutv2/configuration/validation.go b/internal/lookoutv2/configuration/validation.go
@@ -0,0 +1,8 @@
+package configuration
+
+import "github.com/go-playground/validator/v10"
+
+func (c LookoutV2Config) Validate() error {
+	validate := validator.New()
+	return validate.Struct(c)
+}
diff --git a/internal/scheduler/configuration/configuration.go b/internal/scheduler/configuration/configuration.go
@@ -1,10 +1,8 @@
 package configuration
 
 import (
-	"fmt"
 	"time"
 
-	"github.com/go-playground/validator/v10"
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
 
@@ -59,12 +57,6 @@ type Configuration struct {
 	QueueRefreshPeriod time.Duration `validate:"required"`
 }
 
-func (c Configuration) Validate() error {
-	validate := validator.New()
-	validate.RegisterStructValidation(SchedulingConfigValidation, SchedulingConfig{})
-	return validate.Struct(c)
-}
-
 type LeaderConfig struct {
 	// Valid modes are "standalone" or "kubernetes"
 	Mode string `validate:"required"`
@@ -251,33 +243,6 @@ const (
 	UnknownWellKnownNodeTypeErrorMessage       = "priority class refers to unknown well-known node type"
 )
 
-func SchedulingConfigValidation(sl validator.StructLevel) {
-	c := sl.Current().Interface().(SchedulingConfig)
-
-	wellKnownNodeTypes := make(map[string]bool)
-	for i, wellKnownNodeType := range c.WellKnownNodeTypes {
-		if wellKnownNodeTypes[wellKnownNodeType.Name] {
-			fieldName := fmt.Sprintf("WellKnownNodeTypes[%d].Name", i)
-			sl.ReportError(wellKnownNodeType.Name, fieldName, "", DuplicateWellKnownNodeTypeErrorMessage, "")
-		}
-		wellKnownNodeTypes[wellKnownNodeType.Name] = true
-	}
-
-	for priorityClassName, priorityClass := range c.PriorityClasses {
-		if len(priorityClass.AwayNodeTypes) > 0 && !priorityClass.Preemptible {
-			fieldName := fmt.Sprintf("Preemption.PriorityClasses[%s].Preemptible", priorityClassName)
-			sl.ReportError(priorityClass.Preemptible, fieldName, "", AwayNodeTypesWithoutPreemptionErrorMessage, "")
-		}
-
-		for i, awayNodeType := range priorityClass.AwayNodeTypes {
-			if !wellKnownNodeTypes[awayNodeType.WellKnownNodeTypeName] {
-				fieldName := fmt.Sprintf("Preemption.PriorityClasses[%s].AwayNodeTypes[%d].WellKnownNodeTypeName", priorityClassName, i)
-				sl.ReportError(awayNodeType.WellKnownNodeTypeName, fieldName, "", UnknownWellKnownNodeTypeErrorMessage, "")
-			}
-		}
-	}
-}
-
 // ResourceType represents a resource the scheduler indexes for efficient lookup.
 type ResourceType struct {
 	// Resource name, e.g., "cpu", "memory", or "nvidia.com/gpu".

diff --git a/internal/scheduler/configuration/validation.go b/internal/scheduler/configuration/validation.go
@@ -0,0 +1,40 @@
+package configuration
+
+import (
+	"fmt"
+
+	"github.com/go-playground/validator/v10"
+)
+
+func (c Configuration) Validate() error {
+	validate := validator.New()
+	validate.RegisterStructValidation(SchedulingConfigValidation, SchedulingConfig{})
+	return validate.Struct(c)
+}
+
+func SchedulingConfigValidation(sl validator.StructLevel) {
+	c := sl.Current().Interface().(SchedulingConfig)
+
+	wellKnownNodeTypes := make(map[string]bool)
+	for i, wellKnownNodeType := range c.WellKnownNodeTypes {
+		if wellKnownNodeTypes[wellKnownNodeType.Name] {
+			fieldName := fmt.Sprintf("WellKnownNodeTypes[%d].Name", i)
+			sl.ReportError(wellKnownNodeType.Name, fieldName, "", DuplicateWellKnownNodeTypeErrorMessage, "")
+		}
+		wellKnownNodeTypes[wellKnownNodeType.Name] = true
+	}
+
+	for priorityClassName, priorityClass := range c.PriorityClasses {
+		if len(priorityClass.AwayNodeTypes) > 0 && !priorityClass.Preemptible {
+			fieldName := fmt.Sprintf("Preemption.PriorityClasses[%s].Preemptible", priorityClassName)
+			sl.ReportError(priorityClass.Preemptible, fieldName, "", AwayNodeTypesWithoutPreemptionErrorMessage, "")
+		}
+
+		for i, awayNodeType := range priorityClass.AwayNodeTypes {
+			if !wellKnownNodeTypes[awayNodeType.WellKnownNodeTypeName] {
+				fieldName := fmt.Sprintf("Preemption.PriorityClasses[%s].AwayNodeTypes[%d].WellKnownNodeTypeName", priorityClassName, i)
+				sl.ReportError(awayNodeType.WellKnownNodeTypeName, fieldName, "", UnknownWellKnownNodeTypeErrorMessage, "")
+			}
+		}
+	}
+}
diff --git a/...duler/configuration/configuration_test.go → ...cheduler/configuration/validation_test.go b/...duler/configuration/configuration_test.go → ...cheduler/configuration/validation_test.go
diff --git a/internal/scheduleringester/config.go b/internal/scheduleringester/config.go
@@ -3,6 +3,8 @@ package scheduleringester
 import (
 	"time"
 
+	"github.com/go-playground/validator/v10"
+
 	commonconfig "github.com/armadaproject/armada/internal/common/config"
 	profilingconfig "github.com/armadaproject/armada/internal/common/profiling/configuration"
 	"github.com/armadaproject/armada/internal/server/configuration"
@@ -24,3 +26,8 @@ type Configuration struct {
 	// If non-nil, configures pprof profiling
 	Profiling *profilingconfig.ProfilingConfig
 }
+
+func (c Configuration) Validate() error {
+	validate := validator.New()
+	return validate.Struct(c)
+}
diff --git a/internal/server/configuration/validation.go b/internal/server/configuration/validation.go
@@ -0,0 +1,8 @@
+package configuration
+
+import "github.com/go-playground/validator/v10"
+
+func (c ArmadaConfig) Validate() error {
+	validate := validator.New()
+	return validate.Struct(c)
+}