From 549c5b94d929ea5c83ae6aa2eaaa791edf49b13e Mon Sep 17 00:00:00 2001 From: Eleanor Pratt <101330560+eleanorpratt@users.noreply.github.com> Date: Wed, 18 Sep 2024 16:57:36 +0100 Subject: [PATCH] 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 --- cmd/scheduler/cmd/root.go | 2 +- .../binoculars/configuration/validation.go | 8 ++++ internal/common/config/pulsar.go | 2 +- internal/common/config/validation.go | 29 ++++++++------ internal/common/grpc/configuration/types.go | 2 +- internal/common/startup.go | 7 +++- .../eventingester/configuration/validation.go | 10 +++++ internal/executor/configuration/validation.go | 8 ++++ .../configuration/validation.go | 8 ++++ .../lookoutv2/configuration/validation.go | 8 ++++ .../scheduler/configuration/configuration.go | 35 ---------------- .../scheduler/configuration/validation.go | 40 +++++++++++++++++++ ...nfiguration_test.go => validation_test.go} | 0 internal/scheduleringester/config.go | 7 ++++ internal/server/configuration/validation.go | 8 ++++ 15 files changed, 123 insertions(+), 51 deletions(-) create mode 100644 internal/binoculars/configuration/validation.go create mode 100644 internal/eventingester/configuration/validation.go create mode 100644 internal/executor/configuration/validation.go create mode 100644 internal/lookoutingesterv2/configuration/validation.go create mode 100644 internal/lookoutv2/configuration/validation.go create mode 100644 internal/scheduler/configuration/validation.go rename internal/scheduler/configuration/{configuration_test.go => validation_test.go} (100%) create mode 100644 internal/server/configuration/validation.go diff --git a/cmd/scheduler/cmd/root.go b/cmd/scheduler/cmd/root.go index 5b25c754d44..25f1aec73d0 100644 --- 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 new file mode 100644 index 00000000000..914f40053dd --- /dev/null +++ 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 index b2ec8eef984..478ca22d8c5 100644 --- 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 index 8b14cf77d48..55e8abde269 100644 --- 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 index 967485f348a..2101e873f65 100644 --- 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 index e8ee1825ab9..535fb628c5e 100644 --- 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 new file mode 100644 index 00000000000..cc797e3ad4a --- /dev/null +++ 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 new file mode 100644 index 00000000000..186bb9cd6e1 --- /dev/null +++ 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 new file mode 100644 index 00000000000..c78f1df1d19 --- /dev/null +++ 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 new file mode 100644 index 00000000000..e9228dbbef2 --- /dev/null +++ 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 index 36c1a0142c6..72f6f9648ec 100644 --- 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 new file mode 100644 index 00000000000..8d11541ae66 --- /dev/null +++ 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/internal/scheduler/configuration/configuration_test.go b/internal/scheduler/configuration/validation_test.go similarity index 100% rename from internal/scheduler/configuration/configuration_test.go rename to internal/scheduler/configuration/validation_test.go diff --git a/internal/scheduleringester/config.go b/internal/scheduleringester/config.go index fa38326e701..4ec84f5fb69 100644 --- 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 new file mode 100644 index 00000000000..cdfd46513c6 --- /dev/null +++ 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) +}