🐛 controlplane: Normalise image name for kube-proxy, and validate Kubernetes versions

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

controlplane/kubeadm/internal/workload_cluster.go

@@ -40,12 +40,12 @@ import (
 	etcdutil "sigs.k8s.io/cluster-api/controlplane/kubeadm/internal/etcd/util"
 	"sigs.k8s.io/cluster-api/util"
 	"sigs.k8s.io/cluster-api/util/certs"
-	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/cluster-api/util/patch"
 	ctrlclient "sigs.k8s.io/controller-runtime/pkg/client"
 )
 
 const (
-	kubeProxyDaemonSetName = "kube-proxy"
+	kubeProxyKey = "kube-proxy"
 )
 
 var (
@@ -578,28 +578,50 @@ func firstNodeNotMatchingName(name string, nodes []corev1.Node) *corev1.Node {
 func (w *Workload) UpdateKubeProxyImageInfo(ctx context.Context, kcp *controlplanev1.KubeadmControlPlane) error {
 	ds := &appsv1.DaemonSet{}
 
-	if err := w.Client.Get(ctx, ctrlclient.ObjectKey{Name: kubeProxyDaemonSetName, Namespace: metav1.NamespaceSystem}, ds); err != nil {
+	if err := w.Client.Get(ctx, ctrlclient.ObjectKey{Name: kubeProxyKey, Namespace: metav1.NamespaceSystem}, ds); err != nil {
 		if apierrors.IsNotFound(err) {
 			// if kube-proxy is missing, return without errors
 			return nil
 		}
-		return errors.Wrapf(err, "failed to determine if %s daemonset already exists", kubeProxyDaemonSetName)
+		return errors.Wrapf(err, "failed to determine if %s daemonset already exists", kubeProxyKey)
 	}
 
-	if len(ds.Spec.Template.Spec.Containers) == 0 {
+	container := findKubeProxyContainer(ds)
+	if container == nil {
 		return nil
 	}
-	newImageName, err := util.ModifyImageTag(ds.Spec.Template.Spec.Containers[0].Image, kcp.Spec.Version)
+
+	newImageName, err := util.ModifyImageTag(container.Image, kcp.Spec.Version)
 	if err != nil {
 		return err
 	}
 
-	if ds.Spec.Template.Spec.Containers[0].Image != newImageName {
-		patch := client.MergeFrom(ds.DeepCopy())
-		ds.Spec.Template.Spec.Containers[0].Image = newImageName
-		if err := w.Client.Patch(ctx, ds, patch); err != nil {
-			return errors.Wrap(err, "error patching kube-proxy DaemonSet")
+	if container.Image != newImageName {
+		helper, err := patch.NewHelper(ds, w.Client)
+		if err != nil {
+			return err
 		}
+		patchKubeProxyImage(ds, newImageName)
+		return helper.Patch(ctx, ds)
 	}
 	return nil
 }
+
+func findKubeProxyContainer(ds *appsv1.DaemonSet) *corev1.Container {
+	containers := ds.Spec.Template.Spec.Containers
+	for idx := range containers {
+		if containers[idx].Name == kubeProxyKey {
+			return &containers[idx]
+		}
+	}
+	return nil
+}
+
+func patchKubeProxyImage(ds *appsv1.DaemonSet, image string) {
+	containers := ds.Spec.Template.Spec.Containers
+	for idx := range containers {
+		if containers[idx].Name == kubeProxyKey {
+			containers[idx].Image = image
+		}
+	}
+}