Validate that MPIJob produces valid hostnames (#385)

Hostnames must be valid DNS labels. This includes checking for invalid characters and a maximum length
kubeflow · Jul 27, 2021 · 84604c8 · 84604c8
1 parent 7b6c1bf
commit 84604c8
Show file tree

Hide file tree

Showing 2 changed files with 75 additions and 1 deletion.
diff --git a/v2/pkg/apis/kubeflow/validation/validation.go b/v2/pkg/apis/kubeflow/validation/validation.go
@@ -16,9 +16,11 @@ package validation
 
 import (
 	"fmt"
+	"strings"
 
 	apivalidation "k8s.io/apimachinery/pkg/api/validation"
 	"k8s.io/apimachinery/pkg/util/sets"
+	apimachineryvalidation "k8s.io/apimachinery/pkg/util/validation"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 
 	common "github.com/kubeflow/common/pkg/apis/common/v1"
@@ -31,7 +33,24 @@ var validCleanPolicies = sets.NewString(
 	string(common.CleanPodPolicyAll))
 
 func ValidateMPIJob(job *kubeflow.MPIJob) field.ErrorList {
-	return validateMPIJobSpec(&job.Spec, field.NewPath("spec"))
+	errs := validateMPIJobName(job)
+	errs = append(errs, validateMPIJobSpec(&job.Spec, field.NewPath("spec"))...)
+	return errs
+}
+
+func validateMPIJobName(job *kubeflow.MPIJob) field.ErrorList {
+	var allErrs field.ErrorList
+	var replicas int32 = 1
+	if workerSpec := job.Spec.MPIReplicaSpecs[kubeflow.MPIReplicaTypeWorker]; workerSpec != nil {
+		if workerSpec.Replicas != nil && *workerSpec.Replicas > 0 {
+			replicas = *workerSpec.Replicas
+		}
+	}
+	maximumPodHostname := fmt.Sprintf("%s-worker-%d", job.Name, replicas-1)
+	if errs := apimachineryvalidation.IsDNS1123Label(maximumPodHostname); len(errs) > 0 {
+		allErrs = append(allErrs, field.Invalid(field.NewPath("metadata").Child("name"), job.ObjectMeta.Name, fmt.Sprintf("will not able to create pod with invalid DNS label %q: %s", maximumPodHostname, strings.Join(errs, ", "))))
+	}
+	return allErrs
 }
 
 func validateMPIJobSpec(spec *kubeflow.MPIJobSpec, path *field.Path) field.ErrorList {

diff --git a/v2/pkg/apis/kubeflow/validation/validation_test.go b/v2/pkg/apis/kubeflow/validation/validation_test.go
@@ -22,6 +22,7 @@ import (
 	common "github.com/kubeflow/common/pkg/apis/common/v1"
 	"github.com/kubeflow/mpi-operator/v2/pkg/apis/kubeflow/v2beta1"
 	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 )
 
@@ -32,6 +33,9 @@ func TestValidateMPIJob(t *testing.T) {
 	}{
 		"valid": {
 			job: v2beta1.MPIJob{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "foo",
+				},
 				Spec: v2beta1.MPIJobSpec{
 					SlotsPerWorker: newInt32(2),
 					CleanPodPolicy: newCleanPodPolicy(common.CleanPodPolicyRunning),
@@ -50,6 +54,9 @@ func TestValidateMPIJob(t *testing.T) {
 		},
 		"valid with worker": {
 			job: v2beta1.MPIJob{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "foo",
+				},
 				Spec: v2beta1.MPIJobSpec{
 					SlotsPerWorker: newInt32(2),
 					CleanPodPolicy: newCleanPodPolicy(common.CleanPodPolicyRunning),
@@ -76,6 +83,10 @@ func TestValidateMPIJob(t *testing.T) {
 		},
 		"empty job": {
 			wantErrs: field.ErrorList{
+				&field.Error{
+					Type:  field.ErrorTypeInvalid,
+					Field: "metadata.name",
+				},
 				&field.Error{
 					Type:  field.ErrorTypeRequired,
 					Field: "spec.mpiReplicaSpecs",
@@ -90,8 +101,46 @@ func TestValidateMPIJob(t *testing.T) {
 				},
 			},
 		},
+		"invalid name": {
+			job: v2beta1.MPIJob{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "this-name-is-waaaaaaaay-too-long-for-a-worker-hostname",
+				},
+				Spec: v2beta1.MPIJobSpec{
+					SlotsPerWorker: newInt32(2),
+					CleanPodPolicy: newCleanPodPolicy(common.CleanPodPolicyRunning),
+					MPIReplicaSpecs: map[v2beta1.MPIReplicaType]*common.ReplicaSpec{
+						v2beta1.MPIReplicaTypeLauncher: {
+							Replicas: newInt32(1),
+							Template: corev1.PodTemplateSpec{
+								Spec: corev1.PodSpec{
+									Containers: []corev1.Container{{}},
+								},
+							},
+						},
+						v2beta1.MPIReplicaTypeWorker: {
+							Replicas: newInt32(1000),
+							Template: corev1.PodTemplateSpec{
+								Spec: corev1.PodSpec{
+									Containers: []corev1.Container{{}},
+								},
+							},
+						},
+					},
+				},
+			},
+			wantErrs: field.ErrorList{
+				{
+					Type:  field.ErrorTypeInvalid,
+					Field: "metadata.name",
+				},
+			},
+		},
 		"empty replica specs": {
 			job: v2beta1.MPIJob{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "foo",
+				},
 				Spec: v2beta1.MPIJobSpec{
 					SlotsPerWorker:  newInt32(2),
 					CleanPodPolicy:  newCleanPodPolicy(common.CleanPodPolicyRunning),
@@ -107,6 +156,9 @@ func TestValidateMPIJob(t *testing.T) {
 		},
 		"missing replica spec fields": {
 			job: v2beta1.MPIJob{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "foo",
+				},
 				Spec: v2beta1.MPIJobSpec{
 					SlotsPerWorker: newInt32(2),
 					CleanPodPolicy: newCleanPodPolicy(common.CleanPodPolicyRunning),
@@ -137,6 +189,9 @@ func TestValidateMPIJob(t *testing.T) {
 		},
 		"invalid replica counts": {
 			job: v2beta1.MPIJob{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "foo",
+				},
 				Spec: v2beta1.MPIJobSpec{
 					SlotsPerWorker: newInt32(2),
 					CleanPodPolicy: newCleanPodPolicy(common.CleanPodPolicyRunning),