Merge pull request #7739 from k8s-infra-cherrypick-robot/cherry-pick-…

…7712-to-release-1.3 [release-1.3] 🌱 Add explicit length check for cluster and md names
kubernetes-sigs · Dec 13, 2022 · 86937cc · 86937cc
2 parents 3b886d7 + 593c91d
commit 86937cc
Show file tree

Hide file tree

Showing 4 changed files with 93 additions and 1 deletion.
diff --git 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
@@ -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
@@ -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
@@ -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 {