From 593c91d67530591b7536ca38a2f8457d891fb395 Mon Sep 17 00:00:00 2001 From: killianmuldoon Date: Thu, 8 Dec 2022 19:30:39 +0000 Subject: [PATCH] Add explicit length check for cluster and md names Signed-off-by: killianmuldoon --- api/v1beta1/machinedeployment_webhook.go | 14 ++++++++ api/v1beta1/machinedeployment_webhook_test.go | 36 ++++++++++++++++++- internal/webhooks/cluster.go | 14 ++++++++ internal/webhooks/cluster_test.go | 30 ++++++++++++++++ 4 files changed, 93 insertions(+), 1 deletion(-) diff --git a/api/v1beta1/machinedeployment_webhook.go b/api/v1beta1/machinedeployment_webhook.go index 65fa45f4858b..6a5d6646c8e0 100644 --- a/api/v1beta1/machinedeployment_webhook.go +++ b/api/v1beta1/machinedeployment_webhook.go @@ -25,6 +25,7 @@ import ( "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/intstr" + "k8s.io/apimachinery/pkg/util/validation" "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/utils/pointer" ctrl "sigs.k8s.io/controller-runtime" @@ -76,6 +77,19 @@ func (m *MachineDeployment) ValidateDelete() error { func (m *MachineDeployment) validate(old *MachineDeployment) error { var allErrs field.ErrorList + // The MachineDeployment name is used as a label value. This check ensures names which are not be valid label values are rejected. + if errs := validation.IsValidLabelValue(m.Name); len(errs) != 0 { + for _, err := range errs { + allErrs = append( + allErrs, + field.Invalid( + field.NewPath("metadata", "name"), + m.Name, + fmt.Sprintf("must be a valid label value: %s", err), + ), + ) + } + } specPath := field.NewPath("spec") selector, err := metav1.LabelSelectorAsSelector(&m.Spec.Selector) if err != nil { diff --git a/api/v1beta1/machinedeployment_webhook_test.go b/api/v1beta1/machinedeployment_webhook_test.go index 4ab87a471896..c7ccc4800cd5 100644 --- a/api/v1beta1/machinedeployment_webhook_test.go +++ b/api/v1beta1/machinedeployment_webhook_test.go @@ -67,14 +67,45 @@ func TestMachineDeploymentValidation(t *testing.T) { goodMaxSurgeInt := intstr.FromInt(1) goodMaxUnavailableInt := intstr.FromInt(0) - tests := []struct { name string + md MachineDeployment + mdName string selectors map[string]string labels map[string]string strategy MachineDeploymentStrategy expectErr bool }{ + { + name: "pass with name of under 63 characters", + mdName: "short-name", + expectErr: false, + }, + { + name: "pass with _, -, . characters in name", + mdName: "thisNameContains.A_Non-Alphanumeric", + expectErr: false, + }, + { + name: "error with name of more than 63 characters", + mdName: "thisNameIsReallyMuchLongerThanTheMaximumLengthOfSixtyThreeCharacters", + expectErr: true, + }, + { + name: "error when name starts with NonAlphanumeric character", + mdName: "-thisNameStartsWithANonAlphanumeric", + expectErr: true, + }, + { + name: "error when name ends with NonAlphanumeric character", + mdName: "thisNameEndsWithANonAlphanumeric.", + expectErr: true, + }, + { + name: "error when name contains invalid NonAlphanumeric character", + mdName: "thisNameContainsInvalid!@NonAlphanumerics", + expectErr: true, + }, { name: "should return error on mismatch", selectors: map[string]string{"foo": "bar"}, @@ -163,6 +194,9 @@ func TestMachineDeploymentValidation(t *testing.T) { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) md := &MachineDeployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: tt.mdName, + }, Spec: MachineDeploymentSpec{ Strategy: &tt.strategy, Selector: metav1.LabelSelector{ diff --git a/internal/webhooks/cluster.go b/internal/webhooks/cluster.go index ef6a6d870be7..1948abfa44ba 100644 --- a/internal/webhooks/cluster.go +++ b/internal/webhooks/cluster.go @@ -26,6 +26,7 @@ import ( "github.com/pkg/errors" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/validation" "k8s.io/apimachinery/pkg/util/validation/field" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -149,6 +150,19 @@ func (webhook *Cluster) ValidateDelete(_ context.Context, _ runtime.Object) erro func (webhook *Cluster) validate(ctx context.Context, oldCluster, newCluster *clusterv1.Cluster) error { var allErrs field.ErrorList + // The Cluster name is used as a label value. This check ensures that names which are not valid label values are rejected. + if errs := validation.IsValidLabelValue(newCluster.Name); len(errs) != 0 { + for _, err := range errs { + allErrs = append( + allErrs, + field.Invalid( + field.NewPath("metadata", "name"), + newCluster.Name, + fmt.Sprintf("must be a valid label value %s", err), + ), + ) + } + } specPath := field.NewPath("spec") if newCluster.Spec.InfrastructureRef != nil && newCluster.Spec.InfrastructureRef.Namespace != newCluster.Namespace { allErrs = append( diff --git a/internal/webhooks/cluster_test.go b/internal/webhooks/cluster_test.go index d403f8dc0ae6..2d12487d128b 100644 --- a/internal/webhooks/cluster_test.go +++ b/internal/webhooks/cluster_test.go @@ -518,6 +518,36 @@ func TestClusterValidation(t *testing.T) { CIDRBlocks: []string{"10.10.10.10", "11.11.11.11"}}}). Build(), }, + { + name: "pass with name of under 63 characters", + expectErr: false, + in: builder.Cluster("fooNamespace", "short-name").Build(), + }, + { + name: "pass with _, -, . characters in name", + in: builder.Cluster("fooNamespace", "thisNameContains.A_Non-Alphanumeric").Build(), + expectErr: false, + }, + { + name: "fails if cluster name is longer than 63 characters", + in: builder.Cluster("fooNamespace", "thisNameIsReallyMuchLongerThanTheMaximumLengthOfSixtyThreeCharacters").Build(), + expectErr: true, + }, + { + name: "error when name starts with NonAlphanumeric character", + in: builder.Cluster("fooNamespace", "-thisNameStartsWithANonAlphanumeric").Build(), + expectErr: true, + }, + { + name: "error when name ends with NonAlphanumeric character", + in: builder.Cluster("fooNamespace", "thisNameEndsWithANonAlphanumeric.").Build(), + expectErr: true, + }, + { + name: "error when name contains invalid NonAlphanumeric character", + in: builder.Cluster("fooNamespace", "thisNameContainsInvalid!@NonAlphanumerics").Build(), + expectErr: true, + }, } ) for _, tt := range tests {