kcp: Add additional validation of versioning

Signed-off-by: Naadir Jeewa <jeewan@vmware.com>

controlplane/kubeadm/api/v1alpha3/kubeadm_control_plane_types.go

@@ -43,7 +43,8 @@ type KubeadmControlPlaneSpec struct {
 	Replicas *int32 `json:"replicas,omitempty"`
 
 	// Version defines the desired Kubernetes version.
-	// +kubebuilder:validation:MinLength:=1
+	// +kubebuilder:validation:MinLength:=2
+	// +kubebuilder:validation:Pattern:=^v(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)([-0-9a-zA-Z_\.+]*)?$
 	Version string `json:"version"`
 
 	// InfrastructureTemplate is a required reference to a custom resource

controlplane/kubeadm/api/v1alpha3/kubeadm_control_plane_webhook.go

@@ -18,12 +18,14 @@ package v1alpha3
 
 import (
 	"encoding/json"
+	"fmt"
 
 	"github.com/blang/semver"
 	jsonpatch "github.com/evanphx/json-patch"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/validation/field"
+	"sigs.k8s.io/cluster-api/util"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/webhook"
 )
@@ -55,6 +57,7 @@ func (in *KubeadmControlPlane) Default() {
 // ValidateCreate implements webhook.Validator so a webhook will be registered for the type
 func (in *KubeadmControlPlane) ValidateCreate() error {
 	allErrs := in.validateCommon()
+	allErrs = append(allErrs, in.validateEtcd(nil)...)
 	if len(allErrs) > 0 {
 		return apierrors.NewInvalid(GroupVersion.WithKind("KubeadmControlPlane").GroupKind(), in.Name, allErrs)
 	}
@@ -88,8 +91,6 @@ func (in *KubeadmControlPlane) ValidateUpdate(old runtime.Object) error {
 
 	prev := old.(*KubeadmControlPlane)
 
-	allErrs = append(allErrs, in.validateEtcd(prev)...)
-
 	originalJSON, err := json.Marshal(prev)
 	if err != nil {
 		return apierrors.NewInternalError(err)
@@ -124,6 +125,8 @@ func (in *KubeadmControlPlane) ValidateUpdate(old runtime.Object) error {
 		}
 	}
 
+	allErrs = append(allErrs, in.validateEtcd(prev)...)
+
 	if len(allErrs) > 0 {
 		return apierrors.NewInvalid(GroupVersion.WithKind("KubeadmControlPlane").GroupKind(), in.Name, allErrs)
 	}
@@ -234,6 +237,25 @@ func (in *KubeadmControlPlane) validateCommon() (allErrs field.ErrorList) {
 		allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "version"), in.Spec.Version, "must be a valid semantic version"))
 	}
 
+	allErrs = append(allErrs, in.validateCoreDNS()...)
+
+	return allErrs
+}
+
+func (in *KubeadmControlPlane) validateCoreDNS() (allErrs field.ErrorList) {
+	if in.Spec.KubeadmConfigSpec.ClusterConfiguration == nil {
+		return allErrs
+	}
+	// TODO: Remove when kubeadm types include OpenAPI validation
+	if !util.ImageTagIsValid(in.Spec.KubeadmConfigSpec.ClusterConfiguration.DNS.ImageTag) {
+		allErrs = append(
+			allErrs,
+			field.Forbidden(
+				field.NewPath("spec", "kubeadmConfigSpec", "clusterConfiguration", "dns", "imageTag"),
+				fmt.Sprintf("tag %s is invalid", in.Spec.KubeadmConfigSpec.ClusterConfiguration.DNS.ImageTag),
+			),
+		)
+	}
 	return allErrs
 }
 
@@ -242,26 +264,50 @@ func (in *KubeadmControlPlane) validateEtcd(prev *KubeadmControlPlane) (allErrs
 		return allErrs
 	}
 
-	if in.Spec.KubeadmConfigSpec.ClusterConfiguration.Etcd.External != nil && prev.Spec.KubeadmConfigSpec.ClusterConfiguration.Etcd.Local != nil {
+	// TODO: Remove when kubeadm types include OpenAPI validation
+	if in.Spec.KubeadmConfigSpec.ClusterConfiguration.Etcd.Local != nil && !util.ImageTagIsValid(in.Spec.KubeadmConfigSpec.ClusterConfiguration.Etcd.Local.ImageTag) {
 		allErrs = append(
 			allErrs,
 			field.Forbidden(
-				field.NewPath("spec", "kubeadmConfigSpec", "initConfiguration", "etcd", "external"),
-				"cannot have both local and external etcd at the same time",
+				field.NewPath("spec", "kubeadmConfigSpec", "clusterConfiguration", "etcd", "local", "imageTag"),
+				fmt.Sprintf("tag %s is invalid", in.Spec.KubeadmConfigSpec.ClusterConfiguration.Etcd.Local.ImageTag),
 			),
 		)
 	}
 
-	if in.Spec.KubeadmConfigSpec.ClusterConfiguration.Etcd.Local != nil && prev.Spec.KubeadmConfigSpec.ClusterConfiguration.Etcd.External != nil {
+	if in.Spec.KubeadmConfigSpec.ClusterConfiguration.Etcd.Local != nil && in.Spec.KubeadmConfigSpec.ClusterConfiguration.Etcd.External != nil {
 		allErrs = append(
 			allErrs,
 			field.Forbidden(
-				field.NewPath("spec", "kubeadmConfigSpec", "initConfiguration", "etcd", "local"),
-				"cannot have both local and external etcd at the same time",
+				field.NewPath("spec", "kubeadmConfigSpec", "clusterConfiguration", "etcd", "local"),
+				"cannot have both external and local etcd",
 			),
 		)
 	}
 
+	// update validations
+	if prev != nil {
+		if in.Spec.KubeadmConfigSpec.ClusterConfiguration.Etcd.External != nil && prev.Spec.KubeadmConfigSpec.ClusterConfiguration.Etcd.Local != nil {
+			allErrs = append(
+				allErrs,
+				field.Forbidden(
+					field.NewPath("spec", "kubeadmConfigSpec", "clusterConfiguration", "etcd", "external"),
+					"cannot change between external and local etcd",
+				),
+			)
+		}
+
+		if in.Spec.KubeadmConfigSpec.ClusterConfiguration.Etcd.Local != nil && prev.Spec.KubeadmConfigSpec.ClusterConfiguration.Etcd.External != nil {
+			allErrs = append(
+				allErrs,
+				field.Forbidden(
+					field.NewPath("spec", "kubeadmConfigSpec", "clusterConfiguration", "etcd", "local"),
+					"cannot change between external and local etcd",
+				),
+			)
+		}
+	}
+
 	return allErrs
 }
 

controlplane/kubeadm/api/v1alpha3/kubeadm_control_plane_webhook_test.go

@@ -221,6 +221,21 @@ func TestKubeadmControlPlaneValidateUpdate(t *testing.T) {
 			ImageTag: "v9.1.1",
 		},
 	}
+
+	etcdLocalImageBuildTag := before.DeepCopy()
+	etcdLocalImageBuildTag.Spec.KubeadmConfigSpec.ClusterConfiguration.Etcd.Local = &kubeadmv1beta1.LocalEtcd{
+		ImageMeta: kubeadmv1beta1.ImageMeta{
+			ImageTag: "v9.1.1_validBuild1",
+		},
+	}
+
+	etcdLocalImageInvalidTag := before.DeepCopy()
+	etcdLocalImageInvalidTag.Spec.KubeadmConfigSpec.ClusterConfiguration.Etcd.Local = &kubeadmv1beta1.LocalEtcd{
+		ImageMeta: kubeadmv1beta1.ImageMeta{
+			ImageTag: "v9.1.1+invalidBuild1",
+		},
+	}
+
 	unsetEtcd := etcdLocalImageTag.DeepCopy()
 	unsetEtcd.Spec.KubeadmConfigSpec.ClusterConfiguration.Etcd.Local = nil
 
@@ -256,6 +271,22 @@ func TestKubeadmControlPlaneValidateUpdate(t *testing.T) {
 		},
 	}
 
+	dnsBuildTag := before.DeepCopy()
+	dnsBuildTag.Spec.KubeadmConfigSpec.ClusterConfiguration.DNS = kubeadmv1beta1.DNS{
+		ImageMeta: kubeadmv1beta1.ImageMeta{
+			ImageRepository: "gcr.io/capi-test",
+			ImageTag:        "v0.20.0_build1",
+		},
+	}
+
+	dnsInvalidTag := before.DeepCopy()
+	dnsInvalidTag.Spec.KubeadmConfigSpec.ClusterConfiguration.DNS = kubeadmv1beta1.DNS{
+		ImageMeta: kubeadmv1beta1.ImageMeta{
+			ImageRepository: "gcr.io/capi-test",
+			ImageTag:        "v0.20.0+invalidBuild1",
+		},
+	}
+
 	certificatesDir := before.DeepCopy()
 	certificatesDir.Spec.KubeadmConfigSpec.ClusterConfiguration.CertificatesDir = "a new certificates directory"
 
@@ -392,6 +423,18 @@ func TestKubeadmControlPlaneValidateUpdate(t *testing.T) {
 			before:    before,
 			kcp:       etcdLocalImageTag,
 		},
+		{
+			name:      "should succeed when making a change to the local etcd image tag",
+			expectErr: false,
+			before:    before,
+			kcp:       etcdLocalImageBuildTag,
+		},
+		{
+			name:      "should fail when using an invalid etcd image tag",
+			expectErr: true,
+			before:    before,
+			kcp:       etcdLocalImageInvalidTag,
+		},
 		{
 			name:      "should fail when making a change to the cluster config's networking struct",
 			expectErr: true,
@@ -429,11 +472,23 @@ func TestKubeadmControlPlaneValidateUpdate(t *testing.T) {
 			kcp:       scheduler,
 		},
 		{
-			name:      "should fail when making a change to the cluster config's dns",
+			name:      "should succeed when making a change to the cluster config's dns",
 			expectErr: false,
 			before:    before,
 			kcp:       dns,
 		},
+		{
+			name:      "should succeed when using an valid DNS build",
+			expectErr: false,
+			before:    before,
+			kcp:       dnsBuildTag,
+		},
+		{
+			name:      "should fail when using an invalid DNS build",
+			expectErr: true,
+			before:    before,
+			kcp:       dnsInvalidTag,
+		},
 		{
 			name:      "should fail when making a change to the cluster config's certificatesDir",
 			expectErr: true,

controlplane/kubeadm/config/crd/bases/controlplane.cluster.x-k8s.io_kubeadmcontrolplanes.yaml

@@ -908,7 +908,8 @@ spec:
                 type: string
               version:
                 description: Version defines the desired Kubernetes version.
-                minLength: 1
+                minLength: 2
+                pattern: ^v(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)([-0-9a-zA-Z_\.+]*)?$
                 type: string
             required:
             - infrastructureTemplate

test/infrastructure/docker/e2e/docker_test.go

@@ -311,7 +311,7 @@ type ClusterGenerator struct {
 func (c *ClusterGenerator) GenerateCluster(namespace string, replicas int32) (*clusterv1.Cluster, *infrav1.DockerCluster, *controlplanev1.KubeadmControlPlane, *infrav1.DockerMachineTemplate) {
 	generatedName := fmt.Sprintf("test-%d", c.counter)
 	c.counter++
-	version := "1.16.3"
+	version := "v1.16.3"
 
 	infraCluster := &infrav1.DockerCluster{
 		ObjectMeta: metav1.ObjectMeta{