fix(config): validate assembled Config in LoadConfig to fail fast on invalid values

internal/config/config.go

@@ -4,17 +4,29 @@
 package config
 
 import (
+	"errors"
 	"fmt"
 	"os"
 	"path/filepath"
 	"strings"
 
 	"github.com/spf13/cobra"
 	"github.com/spf13/viper"
+	"k8s.io/apimachinery/pkg/api/resource"
 
 	"github.com/opdev/virtwork/internal/constants"
 )
 
+var (
+	ErrInvalidNamespace = errors.New("invalid config: namespace must not be empty")
+	ErrInvalidCpuCores  = errors.New("invalid config: cpu-cores must be at least 1, got")
+	ErrInvalidMemory    = errors.New("invalid config: not a valid value for memory")
+	ErrInvalidDiskSize  = errors.New("invalid config: not a valid value for disk-size")
+	ErrInvalidTimeout   = errors.New(
+		"invalid config: timeout must be at least 1 when wait-for-ready is enabled, got",
+	)
+)
+
 // WorkloadConfig holds per-workload configuration.
 type WorkloadConfig struct {
 	Enabled  *bool             `mapstructure:"enabled"`
@@ -24,6 +36,27 @@ type WorkloadConfig struct {
 	Params   map[string]string `mapstructure:"params"`
 }
 
+// Validate checks the assembled Config for semantic errors and returns
+// a clear message naming the invalid field, the value, and what is expected.
+func (c *Config) Validate() error {
+	if strings.TrimSpace(c.Namespace) == "" {
+		return ErrInvalidNamespace
+	}
+	if c.CPUCores < 1 {
+		return fmt.Errorf("%w %d", ErrInvalidCpuCores, c.CPUCores)
+	}
+	if _, err := resource.ParseQuantity(c.Memory); err != nil {
+		return fmt.Errorf("%w (%q): %w", ErrInvalidMemory, c.Memory, err)
+	}
+	if _, err := resource.ParseQuantity(c.DataDiskSize); err != nil {
+		return fmt.Errorf("%w (%q): %w", ErrInvalidDiskSize, c.DataDiskSize, err)
+	}
+	if c.WaitForReady && c.ReadyTimeoutSeconds < 1 {
+		return fmt.Errorf("%w %d", ErrInvalidTimeout, c.ReadyTimeoutSeconds)
+	}
+	return nil
+}
+
 // BoolPtr returns a pointer to the provided bool value.
 // Useful for setting WorkloadConfig.Enabled in tests and code.
 func BoolPtr(b bool) *bool {
@@ -197,6 +230,10 @@ func LoadConfig(cmd *cobra.Command) (*Config, error) {
 	}
 	cfg.Workloads = workloads
 
+	if err := cfg.Validate(); err != nil {
+		return nil, err
+	}
+
 	return cfg, nil
 }
 

internal/config/config_test.go

@@ -33,7 +33,7 @@ func newTestCommand() *cobra.Command {
 	root.AddCommand(cmd)
 
 	// Merge root's persistent flags into cmd (Cobra does this during Execute;
-	// tests call LoadConfig directly so we replicate the merge here).
+	// tests call LoadConfig directly, so we replicate the merge here).
 	root.PersistentFlags().VisitAll(func(f *pflag.Flag) {
 		cmd.Flags().AddFlag(f)
 	})
@@ -274,32 +274,32 @@ container-disk-image: quay.io/test/image:latest
 		})
 
 		It("should prefer --disk-size flag over env var", func() {
-			_ = os.Setenv("VIRTWORK_DISK_SIZE", "env-100Gi")
+			_ = os.Setenv("VIRTWORK_DISK_SIZE", "100Gi")
 			defer func() {
 				_ = os.Unsetenv("VIRTWORK_DISK_SIZE")
 			}()
 
-			err1 := cmd.Flags().Set("disk-size", "flag-50Gi")
+			err1 := cmd.Flags().Set("disk-size", "50Gi")
 			Expect(err1).NotTo(HaveOccurred())
 
 			cfg, err := config.LoadConfig(cmd)
 			Expect(err).NotTo(HaveOccurred())
-			Expect(cfg.DataDiskSize).To(Equal("flag-50Gi"))
+			Expect(cfg.DataDiskSize).To(Equal("50Gi"))
 		})
 
 		It("should prefer VIRTWORK_DISK_SIZE env over config file", func() {
-			path := writeConfigFile(tmpDir, `disk-size: file-30Gi`)
+			path := writeConfigFile(tmpDir, `disk-size: 30Gi`)
 			err1 := cmd.Flags().Set("config", path)
 			Expect(err1).NotTo(HaveOccurred())
 
-			_ = os.Setenv("VIRTWORK_DISK_SIZE", "env-60Gi")
+			_ = os.Setenv("VIRTWORK_DISK_SIZE", "60Gi")
 			defer func() {
 				_ = os.Unsetenv("VIRTWORK_DISK_SIZE")
 			}()
 
 			cfg, err := config.LoadConfig(cmd)
 			Expect(err).NotTo(HaveOccurred())
-			Expect(cfg.DataDiskSize).To(Equal("env-60Gi"))
+			Expect(cfg.DataDiskSize).To(Equal("60Gi"))
 		})
 	})
 
@@ -517,3 +517,127 @@ workloads:
 		})
 	})
 })
+
+var _ = Describe("Config Validation", func() {
+	var cmd *cobra.Command
+
+	BeforeEach(func() {
+		for _, env := range os.Environ() {
+			if strings.HasPrefix(env, "VIRTWORK_") {
+				_ = os.Unsetenv(strings.Split(env, "=")[0])
+			}
+		}
+		cmd = newTestCommand()
+	})
+
+	Context("namespace", func() {
+		It("should reject an empty namespace", func() {
+			Expect(cmd.Flags().Set("namespace", "")).To(Succeed())
+			_, err := config.LoadConfig(cmd)
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("namespace"))
+		})
+
+		It("should accept a non-empty namespace", func() {
+			Expect(cmd.Flags().Set("namespace", "valid-ns")).To(Succeed())
+			cfg, err := config.LoadConfig(cmd)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(cfg.Namespace).To(Equal("valid-ns"))
+		})
+	})
+
+	//nolint:dupl
+	Context("cpu-cores", func() {
+		It("should reject zero CPU cores", func() {
+			Expect(cmd.Flags().Set("cpu-cores", "0")).To(Succeed())
+			_, err := config.LoadConfig(cmd)
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("cpu-cores"))
+		})
+
+		It("should reject negative CPU cores", func() {
+			Expect(cmd.Flags().Set("cpu-cores", "-1")).To(Succeed())
+			_, err := config.LoadConfig(cmd)
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("cpu-cores"))
+		})
+
+		It("should accept positive CPU cores", func() {
+			Expect(cmd.Flags().Set("cpu-cores", "4")).To(Succeed())
+			cfg, err := config.LoadConfig(cmd)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(cfg.CPUCores).To(Equal(4))
+		})
+	})
+
+	//nolint:dupl
+	Context("memory", func() {
+		It("should reject an empty memory value", func() {
+			Expect(cmd.Flags().Set("memory", "")).To(Succeed())
+			_, err := config.LoadConfig(cmd)
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("memory"))
+		})
+
+		It("should reject an invalid memory quantity", func() {
+			Expect(cmd.Flags().Set("memory", "not-a-quantity")).To(Succeed())
+			_, err := config.LoadConfig(cmd)
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("memory"))
+		})
+
+		It("should accept a valid memory quantity", func() {
+			Expect(cmd.Flags().Set("memory", "4Gi")).To(Succeed())
+			cfg, err := config.LoadConfig(cmd)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(cfg.Memory).To(Equal("4Gi"))
+		})
+	})
+
+	Context("disk-size", func() {
+		It("should reject an invalid disk-size quantity", func() {
+			Expect(cmd.Flags().Set("disk-size", "bad-size")).To(Succeed())
+			_, err := config.LoadConfig(cmd)
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("disk-size"))
+		})
+
+		It("should accept a valid disk-size quantity", func() {
+			Expect(cmd.Flags().Set("disk-size", "20Gi")).To(Succeed())
+			cfg, err := config.LoadConfig(cmd)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(cfg.DataDiskSize).To(Equal("20Gi"))
+		})
+	})
+
+	Context("timeout", func() {
+		It("should reject zero timeout when wait-for-ready is enabled", func() {
+			Expect(cmd.Flags().Set("timeout", "0")).To(Succeed())
+			_, err := config.LoadConfig(cmd)
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("timeout"))
+		})
+
+		It("should reject negative timeout when wait-for-ready is enabled", func() {
+			Expect(cmd.Flags().Set("timeout", "-5")).To(Succeed())
+			_, err := config.LoadConfig(cmd)
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("timeout"))
+		})
+
+		It("should accept positive timeout", func() {
+			Expect(cmd.Flags().Set("timeout", "300")).To(Succeed())
+			cfg, err := config.LoadConfig(cmd)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(cfg.ReadyTimeoutSeconds).To(Equal(300))
+		})
+
+		It("should skip timeout validation when wait-for-ready is disabled", func() {
+			Expect(cmd.Flags().Set("no-wait", "true")).To(Succeed())
+			Expect(cmd.Flags().Set("timeout", "0")).To(Succeed())
+			cfg, err := config.LoadConfig(cmd)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(cfg.WaitForReady).To(BeFalse())
+		})
+	})
+})