feat(operator): enforce RFC 1035 validation for TrainJob name (#2767)

* feat(webhook): enforce RFC 1035 validation for TrainJob name

Signed-off-by: Junie <[email protected]>

* feat(operator): enforce RFC 1035 validation using CEL rules for TrainJob name

Signed-off-by: Junie <[email protected]>

* test(integration): added integration test for TrainJob name validation

Signed-off-by: Junie <[email protected]>

---------

Signed-off-by: Junie <[email protected]>

charts/kubeflow-trainer/crds/trainer.kubeflow.org_trainjobs.yaml

@@ -3081,6 +3081,11 @@ spec:
                 x-kubernetes-list-type: map
             type: object
         type: object
+        x-kubernetes-validations:
+        - message: metadata.name must match RFC 1035 DNS label format
+          rule: self.metadata.name.matches('^[a-z]([-a-z0-9]*[a-z0-9])?$')
+        - message: metadata.name must be no more than 63 characters
+          rule: size(self.metadata.name) <= 63
     served: true
     storage: true
     subresources:

manifests/base/crds/trainer.kubeflow.org_trainjobs.yaml

@@ -3081,6 +3081,11 @@ spec:
                 x-kubernetes-list-type: map
             type: object
         type: object
+        x-kubernetes-validations:
+        - message: metadata.name must match RFC 1035 DNS label format
+          rule: self.metadata.name.matches('^[a-z]([-a-z0-9]*[a-z0-9])?$')
+        - message: metadata.name must be no more than 63 characters
+          rule: size(self.metadata.name) <= 63
     served: true
     storage: true
     subresources:

pkg/apis/trainer/v1alpha1/trainjob_types.go

@@ -34,6 +34,8 @@ const (
 // +kubebuilder:storageversion
 // +kubebuilder:printcolumn:name="State",type=string,JSONPath=`.status.conditions[-1:].type`
 // +kubebuilder:printcolumn:name="Age",type=date,JSONPath=`.metadata.creationTimestamp`
+// +kubebuilder:validation:XValidation:rule="self.metadata.name.matches('^[a-z]([-a-z0-9]*[a-z0-9])?$')", message="metadata.name must match RFC 1035 DNS label format"
+// +kubebuilder:validation:XValidation:rule="size(self.metadata.name) <= 63", message="metadata.name must be no more than 63 characters"
 
 // TrainJob represents configuration of a training job.
 type TrainJob struct {

pkg/webhooks/trainjob_webhook.go

@@ -21,8 +21,6 @@ import (
 	"fmt"
 
 	apiruntime "k8s.io/apimachinery/pkg/runtime"
-	"k8s.io/apimachinery/pkg/util/validation"
-	"k8s.io/apimachinery/pkg/util/validation/field"
 	"k8s.io/klog/v2"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/webhook"
@@ -52,20 +50,13 @@ func (w *TrainJobWebhook) ValidateCreate(ctx context.Context, obj apiruntime.Obj
 	log := ctrl.LoggerFrom(ctx).WithName("trainJob-webhook")
 	log.V(5).Info("Validating create", "TrainJob", klog.KObj(trainJob))
 
-	allErrs := field.ErrorList{}
-
-	// Check RFC 1035 name validation errors
-	for _, err := range validation.IsDNS1035Label(trainJob.Name) {
-		allErrs = append(allErrs, field.Invalid(field.NewPath("metadata", "name"), trainJob.Name, err))
-	}
-
 	runtimeRefGK := runtime.RuntimeRefToRuntimeRegistryKey(trainJob.Spec.RuntimeRef)
 	runtime, ok := w.runtimes[runtimeRefGK]
 	if !ok {
 		return nil, fmt.Errorf("unsupported runtime: %s", runtimeRefGK)
 	}
 	warnings, errors := runtime.ValidateObjects(ctx, nil, trainJob)
-	return warnings, append(allErrs, errors...).ToAggregate()
+	return warnings, errors.ToAggregate()
 }
 
 func (w *TrainJobWebhook) ValidateUpdate(ctx context.Context, oldObj apiruntime.Object, newObj apiruntime.Object) (admission.Warnings, error) {

pkg/webhooks/trainjob_webhook_test.go

@@ -45,19 +45,6 @@ func TestValidateCreate(t *testing.T) {
 			wantError:    nil,
 			wantWarnings: nil,
 		},
-		"invalid trainjob name with uppercase letters": {
-			obj: testingutil.MakeTrainJobWrapper("default", "Invalid-job-name").
-				RuntimeRef(trainer.SchemeGroupVersion.WithKind(trainer.ClusterTrainingRuntimeKind), "test-runtime").
-				Obj(),
-			wantError: field.ErrorList{
-				&field.Error{
-					Type:     field.ErrorTypeInvalid,
-					Field:    "metadata.name",
-					BadValue: "Invalid-job-name",
-				},
-			},
-			wantWarnings: nil,
-		},
 		"unsupported runtime": {
 			obj: testingutil.MakeTrainJobWrapper("default", "valid-job-name").
 				RuntimeRef(trainer.SchemeGroupVersion.WithKind(trainer.ClusterTrainingRuntimeKind), "unsupported-runtime").

test/integration/webhooks/trainjob_test.go

@@ -218,6 +218,54 @@ var _ = ginkgo.Describe("TrainJob Webhook", ginkgo.Ordered, func() {
 				},
 				testingutil.BeForbiddenError()),
 		)
+		ginkgo.DescribeTable("RFC1035-compliant TrainJob name validation", func(trainJob func() *trainer.TrainJob, errorMatcher gomega.OmegaMatcher) {
+			gomega.Expect(k8sClient.Create(ctx, trainJob())).Should(errorMatcher)
+		},
+			ginkgo.Entry("Should succeed to create TrainJob with valid RFC1035-compliant name",
+				func() *trainer.TrainJob {
+					return testingutil.MakeTrainJobWrapper(ns.Name, "valid-job-name").
+						RuntimeRef(trainer.SchemeGroupVersion.WithKind(trainer.ClusterTrainingRuntimeKind), runtimeName).
+						Obj()
+				},
+				gomega.Succeed()),
+			ginkgo.Entry("Should succeed to create TrainJob with name exactly 63 characters",
+				func() *trainer.TrainJob {
+					return testingutil.MakeTrainJobWrapper(ns.Name,
+						"abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijk"). // 63 chars
+						RuntimeRef(trainer.SchemeGroupVersion.WithKind(trainer.ClusterTrainingRuntimeKind), runtimeName).
+						Obj()
+				},
+				gomega.Succeed()),
+			ginkgo.Entry("Should fail to create TrainJob with uppercase letters in name",
+				func() *trainer.TrainJob {
+					return testingutil.MakeTrainJobWrapper(ns.Name, "Invalid-job-name").
+						RuntimeRef(trainer.SchemeGroupVersion.WithKind(trainer.ClusterTrainingRuntimeKind), runtimeName).
+						Obj()
+				},
+				testingutil.BeInvalidError()),
+			ginkgo.Entry("Should fail to create TrainJob starting with a digit",
+				func() *trainer.TrainJob {
+					return testingutil.MakeTrainJobWrapper(ns.Name, "1jobname").
+						RuntimeRef(trainer.SchemeGroupVersion.WithKind(trainer.ClusterTrainingRuntimeKind), runtimeName).
+						Obj()
+				},
+				testingutil.BeInvalidError()),
+			ginkgo.Entry("Should fail to create TrainJob ending with a hyphen",
+				func() *trainer.TrainJob {
+					return testingutil.MakeTrainJobWrapper(ns.Name, "jobname-").
+						RuntimeRef(trainer.SchemeGroupVersion.WithKind(trainer.ClusterTrainingRuntimeKind), runtimeName).
+						Obj()
+				},
+				testingutil.BeInvalidError()),
+			ginkgo.Entry("Should fail to create TrainJob with name exceeding 63 characters",
+				func() *trainer.TrainJob {
+					return testingutil.MakeTrainJobWrapper(ns.Name,
+						"this-name-is-way-too-long-for-a-rfc1035-label-and-should-fail-validation").
+						RuntimeRef(trainer.SchemeGroupVersion.WithKind(trainer.ClusterTrainingRuntimeKind), runtimeName).
+						Obj()
+				},
+				testingutil.BeInvalidError()),
+		)
 	})
 })