From 0357c8d020b88c47a752ab5c104e7e4dfd736406 Mon Sep 17 00:00:00 2001 From: Naadir Jeewa Date: Mon, 16 Mar 2020 16:59:09 +0000 Subject: [PATCH] kcp: Add additional validation of versioning Signed-off-by: Naadir Jeewa --- .../v1alpha3/kubeadm_control_plane_types.go | 3 +- .../v1alpha3/kubeadm_control_plane_webhook.go | 62 ++++++++++++++++--- .../kubeadm_control_plane_webhook_test.go | 57 ++++++++++++++++- ...cluster.x-k8s.io_kubeadmcontrolplanes.yaml | 3 +- test/infrastructure/docker/e2e/docker_test.go | 2 +- 5 files changed, 115 insertions(+), 12 deletions(-) diff --git a/controlplane/kubeadm/api/v1alpha3/kubeadm_control_plane_types.go b/controlplane/kubeadm/api/v1alpha3/kubeadm_control_plane_types.go index fc59ff28121f..06115291fe0c 100644 --- a/controlplane/kubeadm/api/v1alpha3/kubeadm_control_plane_types.go +++ b/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 diff --git a/controlplane/kubeadm/api/v1alpha3/kubeadm_control_plane_webhook.go b/controlplane/kubeadm/api/v1alpha3/kubeadm_control_plane_webhook.go index c5136e3b57d3..8cbbb94ee0db 100644 --- a/controlplane/kubeadm/api/v1alpha3/kubeadm_control_plane_webhook.go +++ b/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 } diff --git a/controlplane/kubeadm/api/v1alpha3/kubeadm_control_plane_webhook_test.go b/controlplane/kubeadm/api/v1alpha3/kubeadm_control_plane_webhook_test.go index e9aa250f18de..9c2a8ae8b651 100644 --- a/controlplane/kubeadm/api/v1alpha3/kubeadm_control_plane_webhook_test.go +++ b/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, diff --git a/controlplane/kubeadm/config/crd/bases/controlplane.cluster.x-k8s.io_kubeadmcontrolplanes.yaml b/controlplane/kubeadm/config/crd/bases/controlplane.cluster.x-k8s.io_kubeadmcontrolplanes.yaml index 23e1028d6c7f..4a521d87f5be 100644 --- a/controlplane/kubeadm/config/crd/bases/controlplane.cluster.x-k8s.io_kubeadmcontrolplanes.yaml +++ b/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 diff --git a/test/infrastructure/docker/e2e/docker_test.go b/test/infrastructure/docker/e2e/docker_test.go index a6bb5f017e97..e710c4ad21e2 100644 --- a/test/infrastructure/docker/e2e/docker_test.go +++ b/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{