Merge pull request #2428 from twz123/enhance-config-validation

Enhanced config CRD validation

inttest/basic/basic_test.go

@@ -190,10 +190,10 @@ func (s *BasicSuite) verifyContainerdDefaultConfig() {
 		return
 	}
 
-	s.Equal(v1beta1.ImageSpec{
+	s.Equal((&v1beta1.ImageSpec{
 		Image:   constant.KubePauseContainerImage,
 		Version: constant.KubePauseContainerImageVersion,
-	}.URI(), parsedConfig.Plugins.CRI.SandboxImage)
+	}).URI(), parsedConfig.Plugins.CRI.SandboxImage)
 }
 
 func TestBasicSuite(t *testing.T) {

pkg/apis/k0s.k0sproject.io/v1beta1/api.go

@@ -20,9 +20,12 @@ import (
 	"fmt"
 	"net"
 
-	"github.com/asaskevich/govalidator"
 	"github.com/k0sproject/k0s/internal/pkg/iface"
 	"github.com/k0sproject/k0s/internal/pkg/stringslice"
+
+	"k8s.io/apimachinery/pkg/util/validation/field"
+
+	"github.com/asaskevich/govalidator"
 )
 
 var _ Validateable = (*APISpec)(nil)
@@ -114,20 +117,30 @@ func (a *APISpec) Sans() []string {
 
 // Validate validates APISpec struct
 func (a *APISpec) Validate() []error {
+	if a == nil {
+		return nil
+	}
+
 	var errors []error
 
-	for _, a := range a.Sans() {
-		if govalidator.IsIP(a) {
-			continue
-		}
-		if govalidator.IsDNSName(a) {
-			continue
+	if !govalidator.IsIP(a.Address) {
+		errors = append(errors, field.Invalid(field.NewPath("address"), a.Address, "invalid IP address"))
+	}
+
+	validateIPAddressOrDNSName := func(path *field.Path, san string) {
+		if govalidator.IsIP(san) || govalidator.IsDNSName(san) {
+			return
 		}
-		errors = append(errors, fmt.Errorf("%s is not a valid address for sans", a))
+		errors = append(errors, field.Invalid(path, san, "invalid IP address / DNS name"))
 	}
 
-	if !govalidator.IsIP(a.Address) {
-		errors = append(errors, fmt.Errorf("spec.api.address: %q is not IP address", a.Address))
+	sansPath := field.NewPath("sans")
+	for idx, san := range a.SANs {
+		validateIPAddressOrDNSName(sansPath.Index(idx), san)
+	}
+
+	if a.ExternalAddress != "" {
+		validateIPAddressOrDNSName(field.NewPath("externalAddress"), a.ExternalAddress)
 	}
 	if a.TunneledNetworkingMode && a.Port == defaultKasPort {
 		errors = append(errors, fmt.Errorf("can't use default kubeapi port if TunneledNetworkingMode is enabled"))

pkg/apis/k0s.k0sproject.io/v1beta1/api_test.go

@@ -44,27 +44,29 @@ func (s *APISuite) TestValidation() {
 
 	s.T().Run("invalid_api_address", func(t *testing.T) {
 		a := APISpec{
-			Address: "somehting.that.is.not.valid//(())",
+			Address: "something.that.is.not.valid//(())",
 		}
 
 		errors := a.Validate()
 		s.NotNil(errors)
-		s.Len(errors, 2)
-		s.Contains(errors[0].Error(), "is not a valid address for sans")
+		if s.Len(errors, 1) {
+			s.ErrorContains(errors[0], `address: Invalid value: "something.that.is.not.valid//(())": invalid IP address`)
+		}
 	})
 
 	s.T().Run("invalid_sans_address", func(t *testing.T) {
 		a := APISpec{
 			Address: "1.2.3.4",
 			SANs: []string{
-				"somehting.that.is.not.valid//(())",
+				"something.that.is.not.valid//(())",
 			},
 		}
 
 		errors := a.Validate()
 		s.NotNil(errors)
-		s.Len(errors, 1)
-		s.Contains(errors[0].Error(), "is not a valid address for sans")
+		if s.Len(errors, 1) {
+			s.ErrorContains(errors[0], `sans[0]: Invalid value: "something.that.is.not.valid//(())": invalid IP address / DNS name`)
+		}
 	})
 	s.T().Run("TunneledNetworkingMode_and_default_kas_port_is_invalid", func(t *testing.T) {
 		a := DefaultAPISpec()

pkg/apis/k0s.k0sproject.io/v1beta1/clusterconfig_types.go

@@ -19,6 +19,7 @@ package v1beta1
 import (
 	"bytes"
 	"encoding/json"
+	"fmt"
 	"io"
 	"reflect"
 
@@ -107,18 +108,26 @@ type InstallSpec struct {
 	SystemUsers *SystemUser `json:"users,omitempty"`
 }
 
+var _ Validateable = (*InstallSpec)(nil)
+
+func (*InstallSpec) Validate() []error { return nil }
+
 // ControllerManagerSpec defines the fields for the ControllerManager
 type ControllerManagerSpec struct {
 	// Map of key-values (strings) for any extra arguments you want to pass down to the Kubernetes controller manager process
 	ExtraArgs map[string]string `json:"extraArgs,omitempty"`
 }
 
+var _ Validateable = (*ControllerManagerSpec)(nil)
+
 func DefaultControllerManagerSpec() *ControllerManagerSpec {
 	return &ControllerManagerSpec{
 		ExtraArgs: make(map[string]string),
 	}
 }
 
+func (c *ControllerManagerSpec) Validate() []error { return nil }
+
 // SchedulerSpec defines the fields for the Scheduler
 type SchedulerSpec struct {
 	// Map of key-values (strings) for any extra arguments you want to pass down to Kubernetes scheduler process
@@ -131,6 +140,10 @@ func DefaultSchedulerSpec() *SchedulerSpec {
 	}
 }
 
+var _ Validateable = (*SchedulerSpec)(nil)
+
+func (*SchedulerSpec) Validate() []error { return nil }
+
 // +kubebuilder:object:root=true
 // +genclient
 // +genclient:onlyVerbs=create
@@ -277,45 +290,48 @@ func DefaultClusterSpec(defaultStorage ...*StorageSpec) *ClusterSpec {
 	}
 }
 
-func (c *ControllerManagerSpec) Validate() []error {
-	return nil
-}
-
-var _ Validateable = (*SchedulerSpec)(nil)
-
-func (s *SchedulerSpec) Validate() []error {
-	return nil
-}
-
-var _ Validateable = (*InstallSpec)(nil)
-
-// Validate stub for Validateable interface
-func (i *InstallSpec) Validate() []error {
-	return nil
-}
-
 // Validateable interface to ensure that all config components implement Validate function
 // +k8s:deepcopy-gen=false
 type Validateable interface {
 	Validate() []error
 }
 
+func (s *ClusterSpec) Validate() (errs []error) {
+	if s == nil {
+		return
+	}
+
+	for name, field := range map[string]Validateable{
+		"api":               s.API,
+		"controllerManager": s.ControllerManager,
+		"scheduler":         s.Scheduler,
+		"storage":           s.Storage,
+		"network":           s.Network,
+		"workerProfiles":    s.WorkerProfiles,
+		"telemetry":         s.Telemetry,
+		"install":           s.Install,
+		"extensions":        s.Extensions,
+		"konnectivity":      s.Konnectivity,
+	} {
+		for _, err := range field.Validate() {
+			errs = append(errs, fmt.Errorf("%s: %w", name, err))
+		}
+	}
+
+	return
+}
+
 // Validate validates cluster config
-func (c *ClusterConfig) Validate() []error {
-	var errors []error
-
-	errors = append(errors, validateSpecs(c.Spec.API)...)
-	errors = append(errors, validateSpecs(c.Spec.ControllerManager)...)
-	errors = append(errors, validateSpecs(c.Spec.Scheduler)...)
-	errors = append(errors, validateSpecs(c.Spec.Storage)...)
-	errors = append(errors, validateSpecs(c.Spec.Network)...)
-	errors = append(errors, validateSpecs(c.Spec.WorkerProfiles)...)
-	errors = append(errors, validateSpecs(c.Spec.Telemetry)...)
-	errors = append(errors, validateSpecs(c.Spec.Install)...)
-	errors = append(errors, validateSpecs(c.Spec.Extensions)...)
-	errors = append(errors, validateSpecs(c.Spec.Konnectivity)...)
-
-	return errors
+func (c *ClusterConfig) Validate() (errs []error) {
+	if c == nil {
+		return nil
+	}
+
+	for _, err := range c.Spec.Validate() {
+		errs = append(errs, fmt.Errorf("spec: %w", err))
+	}
+
+	return errs
 }
 
 // GetBootstrappingConfig returns a ClusterConfig object stripped of Cluster-Wide Settings
@@ -386,8 +402,3 @@ func (c *ClusterConfig) CRValidator() *ClusterConfig {
 
 	return copy
 }
-
-// validateSpecs invokes validator Validate function
-func validateSpecs(v Validateable) []error {
-	return v.Validate()
-}

pkg/apis/k0s.k0sproject.io/v1beta1/clusterconfig_types_test.go

@@ -55,6 +55,15 @@ metadata:
 	assert.Equal(t, addr, c.Spec.Storage.Etcd.PeerAddress)
 }
 
+func TestEmptyClusterSpec(t *testing.T) {
+	underTest := ClusterConfig{
+		Spec: &ClusterSpec{},
+	}
+
+	errs := underTest.Validate()
+	assert.Nil(t, errs)
+}
+
 func TestEtcdDefaults(t *testing.T) {
 	yamlData := `
 apiVersion: k0s.k0sproject.io/v1beta1
@@ -128,8 +137,9 @@ spec:
 	c, err := ConfigFromString(yamlData)
 	assert.NoError(t, err)
 	errors := c.Validate()
-	assert.Equal(t, 1, len(errors))
-	assert.Equal(t, "unsupported network provider: invalidProvider", errors[0].Error())
+	if assert.Len(t, errors, 1) {
+		assert.ErrorContains(t, errors[0], `spec: network: provider: Unsupported value: "invalidProvider": supported values: "kuberouter", "calico", "custom"`)
+	}
 }
 
 func TestApiExternalAddress(t *testing.T) {

pkg/apis/k0s.k0sproject.io/v1beta1/images.go

@@ -19,9 +19,14 @@ package v1beta1
 import (
 	"encoding/json"
 	"fmt"
+	"regexp"
 	"strings"
 
 	"github.com/k0sproject/k0s/pkg/constant"
+
+	"k8s.io/apimachinery/pkg/util/validation/field"
+
+	"github.com/containerd/containerd/reference/docker"
 )
 
 // ImageSpec container image settings
@@ -30,9 +35,29 @@ type ImageSpec struct {
 	Version string `json:"version"`
 }
 
+func (s *ImageSpec) Validate(path *field.Path) (errs field.ErrorList) {
+	if s == nil {
+		return
+	}
+
+	imageLen := len(s.Image)
+	if imageLen == 0 {
+		errs = append(errs, field.Required(path.Child("image"), ""))
+	} else if imageLen != len(strings.TrimSpace(s.Image)) {
+		errs = append(errs, field.Invalid(path.Child("image"), s.Image, "must not have leading or trailing whitespace"))
+	}
+
+	versionRe := regexp.MustCompile(`^` + docker.TagRegexp.String() + `$`)
+	if !versionRe.MatchString(s.Version) {
+		errs = append(errs, field.Invalid(path.Child("version"), s.Version, "must match regular expression: "+versionRe.String()))
+	}
+
+	return
+}
+
 // URI build image uri
-func (is ImageSpec) URI() string {
-	return fmt.Sprintf("%s:%s", is.Image, is.Version)
+func (s *ImageSpec) URI() string {
+	return fmt.Sprintf("%s:%s", s.Image, s.Version)
 }
 
 // ClusterImages sets docker images for addon components

pkg/apis/k0s.k0sproject.io/v1beta1/konnectivity.go

@@ -16,14 +16,28 @@ limitations under the License.
 
 package v1beta1
 
+import (
+	"k8s.io/apimachinery/pkg/util/validation"
+	"k8s.io/apimachinery/pkg/util/validation/field"
+)
+
 var _ Validateable = (*KonnectivitySpec)(nil)
 
 // KonnectivitySpec defines the requested state for Konnectivity
 type KonnectivitySpec struct {
-	// agent port to listen on (default 8132)
-	AgentPort int64 `json:"agentPort,omitempty"`
 	// admin port to listen on (default 8133)
-	AdminPort int64 `json:"adminPort,omitempty"`
+	// +kubebuilder:validation:Minimum=1
+	// +kubebuilder:validation:Maximum=65535
+	// +kubebuilder:default=8133
+	// +optional
+	AdminPort int32 `json:"adminPort,omitempty"`
+
+	// agent port to listen on (default 8132)
+	// +kubebuilder:validation:Minimum=1
+	// +kubebuilder:validation:Maximum=65535
+	// +kubebuilder:default=8132
+	// +optional
+	AgentPort int32 `json:"agentPort,omitempty"`
 }
 
 // DefaultKonnectivitySpec builds default KonnectivitySpec
@@ -34,7 +48,19 @@ func DefaultKonnectivitySpec() *KonnectivitySpec {
 	}
 }
 
-// Validate stub for Validateable interface
-func (k *KonnectivitySpec) Validate() []error {
-	return nil
+// Validate implements [Validateable].
+func (k *KonnectivitySpec) Validate() (errs []error) {
+	if k == nil {
+		return nil
+	}
+
+	for _, msg := range validation.IsValidPortNum(int(k.AdminPort)) {
+		errs = append(errs, field.Invalid(field.NewPath("adminPort"), k.AdminPort, msg))
+	}
+
+	for _, msg := range validation.IsValidPortNum(int(k.AgentPort)) {
+		errs = append(errs, field.Invalid(field.NewPath("agentPort"), k.AgentPort, msg))
+	}
+
+	return errs
 }