Review changes v2

Author: chrischdi

api/core/v1beta2/common_types.go

@@ -412,15 +412,22 @@ func (r *ContractVersionedObjectReference) GroupKind() schema.GroupKind {
 // MachineTaint defines a taint equivalent to corev1.Taint, but additionally having a propagation field.
 type MachineTaint struct {
 	// key is the taint key to be applied to a node.
+	// Must be a valid qualified name of maximum size 63 characters
+	// with an optional subdomain prefix of maximum size 253 characters,
+	// separated by a `/`.
 	// +required
 	// +kubebuilder:validation:MinLength=1
-	// +kubebuilder:validation:MaxLength=253
+	// +kubebuilder:validation:MaxLength=317
+	// +kubebuilder:validation:Pattern=^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*\/)?([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]$
+	// +kubebuilder:validation:XValidation:rule="self.contains('/') ? ( self.split('/') [0].size() <= 253 && self.split('/') [1].size() <= 63 && self.split('/').size() == 2 ) : self.size() <= 63",message="key must be a valid qualified name of max size 63 characters with an optional subdomain prefix of max size 253 characters"
 	Key string `json:"key,omitempty"`
 
 	// value is the taint value corresponding to the taint key.
+	// It must be a valid label value of maximum size 63 characters.
 	// +optional
 	// +kubebuilder:validation:MinLength=1
 	// +kubebuilder:validation:MaxLength=63
+	// +kubebuilder:validation:Pattern=^(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?$
 	Value string `json:"value,omitempty"`
 
 	// effect is the effect for the taint. Valid values are NoSchedule, PreferNoSchedule and NoExecute.

internal/controllers/machineset/machineset_controller_test.go

@@ -63,10 +63,10 @@ var _ reconcile.Reconciler = &Reconciler{}
 
 func TestMachineSetReconciler(t *testing.T) {
 	setup := func(t *testing.T, g *WithT) (*corev1.Namespace, *clusterv1.Cluster) {
-		utilfeature.SetFeatureGateDuringTest(t, feature.Gates, feature.MachineTaintPropagation, true)
-
 		t.Helper()
 
+		utilfeature.SetFeatureGateDuringTest(t, feature.Gates, feature.MachineTaintPropagation, true)
+
 		t.Log("Creating the namespace")
 		ns, err := env.CreateNamespace(ctx, "test-machine-set-reconciler")
 		g.Expect(err).ToNot(HaveOccurred())

internal/webhooks/machine.go

@@ -22,9 +22,7 @@ import (
 	"strings"
 
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
-	metav1validation "k8s.io/apimachinery/pkg/apis/meta/v1/validation"
 	"k8s.io/apimachinery/pkg/runtime"
-	"k8s.io/apimachinery/pkg/util/validation"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	"k8s.io/utils/ptr"
 	ctrl "sigs.k8s.io/controller-runtime"
@@ -160,16 +158,6 @@ func validateMachineTaints(taints []clusterv1.MachineTaint, taintsPath *field.Pa
 	for i, taint := range taints {
 		idxPath := taintsPath.Index(i)
 
-		// Validate key syntax.
-		if errs := metav1validation.ValidateLabelName(taint.Key, idxPath.Child("key")); len(errs) > 0 {
-			allErrs = append(allErrs, errs...)
-		}
-
-		// Validate value syntax.
-		if errs := validation.IsValidLabelValue(taint.Value); len(errs) > 0 {
-			allErrs = append(allErrs, field.Invalid(idxPath.Child("value"), taint.Value, strings.Join(errs, ";")))
-		}
-
 		// The following validations uses a switch statement, because if one of them matches, then the others won't.
 
 		switch {

internal/webhooks/machine_test.go

@@ -20,15 +20,11 @@ import (
 	"testing"
 
 	. "github.com/onsi/gomega"
-	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	utilfeature "k8s.io/component-base/featuregate/testing"
 	"k8s.io/utils/ptr"
 
 	clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2"
-	"sigs.k8s.io/cluster-api/feature"
 	"sigs.k8s.io/cluster-api/internal/webhooks/util"
-	"sigs.k8s.io/cluster-api/util/test/builder"
 )
 
 func TestMachineDefault(t *testing.T) {
@@ -229,119 +225,3 @@ func TestMachineVersionValidation(t *testing.T) {
 		})
 	}
 }
-
-func TestMachineTaintValidation(t *testing.T) {
-	m := builder.Machine("default", "machine1").
-		WithBootstrapTemplate(builder.BootstrapTemplate("default", "bootstrap-template").Build())
-	webhook := &Machine{}
-
-	tests := []struct {
-		name           string
-		machine        *clusterv1.Machine
-		featureEnabled bool
-		expectErr      bool
-	}{
-		{
-			name:           "should allow empty taints with feature gate disabled",
-			featureEnabled: false,
-			machine:        m.DeepCopy().Build(),
-			expectErr:      false,
-		},
-		{
-			name:           "should allow empty taints with feature gate enabled",
-			featureEnabled: true,
-			machine:        m.DeepCopy().Build(),
-			expectErr:      false,
-		},
-		{
-			name:           "should block taint key node.cluster.x-k8s.io/uninitialized",
-			featureEnabled: true,
-			machine: m.DeepCopy().WithTaints(clusterv1.MachineTaint{
-				Key: "node.cluster.x-k8s.io/uninitialized", Effect: corev1.TaintEffectNoSchedule,
-			}).Build(),
-			expectErr: true,
-		},
-		{
-			name:           "should block taint key node.cluster.x-k8s.io/outdated-revision",
-			featureEnabled: true,
-			machine: m.DeepCopy().WithTaints(clusterv1.MachineTaint{
-				Key: "node.cluster.x-k8s.io/outdated-revision", Effect: corev1.TaintEffectNoSchedule,
-			}).Build(),
-			expectErr: true,
-		},
-		{
-			name:           "should block taint with key prefix node.kubernetes.io/, which is not `out-of-service`",
-			featureEnabled: true,
-			machine: m.DeepCopy().WithTaints(clusterv1.MachineTaint{
-				Key: "node.kubernetes.io/some-taint", Effect: corev1.TaintEffectNoSchedule,
-			}).Build(),
-			expectErr: true,
-		},
-		{
-			name:           "should allow taint node.kubernetes.io/out-of-service",
-			featureEnabled: true,
-			machine: m.DeepCopy().WithTaints(clusterv1.MachineTaint{
-				Key: "node.kubernetes.io/out-of-service", Effect: corev1.TaintEffectNoSchedule,
-			}).Build(),
-			expectErr: false,
-		},
-		{
-			name:           "should block taint with key prefix node.cloudprovider.kubernetes.io/",
-			featureEnabled: true,
-			machine: m.DeepCopy().WithTaints(clusterv1.MachineTaint{
-				Key: "node.cloudprovider.kubernetes.io/some-taint", Effect: corev1.TaintEffectNoSchedule,
-			}).Build(),
-			expectErr: true,
-		},
-		{
-			name:           "should block taint key node-role.kubernetes.io/master",
-			featureEnabled: true,
-			machine: m.DeepCopy().WithTaints(clusterv1.MachineTaint{
-				Key: "node-role.kubernetes.io/master", Effect: corev1.TaintEffectNoSchedule,
-			}).Build(),
-			expectErr: true,
-		},
-		{
-			name:           "should allow taint key node-role.kubernetes.io/control-plane for control-plane nodes",
-			featureEnabled: true,
-			machine: m.DeepCopy().
-				WithLabels(map[string]string{clusterv1.MachineControlPlaneLabel: "true"}).
-				WithTaints(clusterv1.MachineTaint{
-					Key: "node-role.kubernetes.io/control-plane", Effect: corev1.TaintEffectNoSchedule,
-				}).Build(),
-			expectErr: false,
-		},
-		{
-			name:           "should block taint key node-role.kubernetes.io/control-plane for worker nodes",
-			featureEnabled: true,
-			machine: m.DeepCopy().WithTaints(clusterv1.MachineTaint{
-				Key: "node-role.kubernetes.io/control-plane", Effect: corev1.TaintEffectNoSchedule,
-			}).Build(),
-			expectErr: true,
-		},
-	}
-	for i := range tests {
-		tt := tests[i]
-		t.Run(tt.name, func(t *testing.T) {
-			g := NewWithT(t)
-
-			utilfeature.SetFeatureGateDuringTest(t, feature.Gates, feature.MachineTaintPropagation, tt.featureEnabled)
-
-			warnings, err := webhook.ValidateCreate(ctx, tt.machine)
-			if tt.expectErr {
-				g.Expect(err).To(HaveOccurred())
-			} else {
-				g.Expect(err).ToNot(HaveOccurred())
-			}
-			g.Expect(warnings).To(BeEmpty())
-
-			warnings, err = webhook.ValidateUpdate(ctx, tt.machine, tt.machine)
-			if tt.expectErr {
-				g.Expect(err).To(HaveOccurred())
-			} else {
-				g.Expect(err).ToNot(HaveOccurred())
-			}
-			g.Expect(warnings).To(BeEmpty())
-		})
-	}
-}