use validateImmutableField (kubernetes#1179)

yaroslava-serdiuk · Oct 4, 2023 · e162f85 · e162f85
1 parent f3bb6f2
commit e162f85
Showing 3 changed files with 27 additions and 28 deletions.
diff --git a/pkg/controller/jobframework/validation.go b/pkg/controller/jobframework/validation.go
@@ -70,27 +70,18 @@ func ValidateCreateForParentWorkload(job GenericJob) field.ErrorList {
 
 func ValidateUpdateForQueueName(oldJob, newJob GenericJob) field.ErrorList {
 	var allErrs field.ErrorList
-	if !newJob.IsSuspended() && (QueueName(oldJob) != QueueName(newJob)) {
-		allErrs = append(allErrs, field.Forbidden(queueNameLabelPath, "must not update queue name when job is unsuspend"))
+	if !newJob.IsSuspended() {
+		allErrs = append(allErrs, apivalidation.ValidateImmutableField(QueueName(oldJob), QueueName(newJob), queueNameLabelPath)...)
 	}
 	return allErrs
 }
 
 func ValidateUpdateForParentWorkload(oldJob, newJob GenericJob) field.ErrorList {
-	var allErrs field.ErrorList
-	if errList := apivalidation.ValidateImmutableField(ParentWorkloadName(newJob),
-		ParentWorkloadName(oldJob), parentWorkloadKeyPath); len(errList) > 0 {
-		allErrs = append(allErrs, field.Forbidden(parentWorkloadKeyPath, "this annotation is immutable"))
-	}
+	allErrs := apivalidation.ValidateImmutableField(ParentWorkloadName(newJob), ParentWorkloadName(oldJob), parentWorkloadKeyPath)
 	return allErrs
 }
 
 func ValidateUpdateForWorkloadPriorityClassName(oldJob, newJob GenericJob) field.ErrorList {
-	var allErrs field.ErrorList
-	oldName := workloadPriorityClassName(oldJob)
-	newName := workloadPriorityClassName(newJob)
-	if oldName != newName {
-		allErrs = append(allErrs, apivalidation.ValidateImmutableField(oldName, newName, workloadPriorityClassNamePath)...)
-	}
+	allErrs := apivalidation.ValidateImmutableField(workloadPriorityClassName(oldJob), workloadPriorityClassName(newJob), workloadPriorityClassNamePath)
 	return allErrs
 }
diff --git a/pkg/controller/jobs/job/job_webhook_test.go b/pkg/controller/jobs/job/job_webhook_test.go
@@ -268,10 +268,12 @@ func TestValidateUpdate(t *testing.T) {
 			wantErr: nil,
 		},
 		{
-			name:    "add queue name with suspend is false",
-			oldJob:  testingutil.MakeJob("job", "default").Obj(),
-			newJob:  testingutil.MakeJob("job", "default").Queue("queue").Suspend(false).Obj(),
-			wantErr: field.ErrorList{field.Forbidden(queueNameLabelPath, "must not update queue name when job is unsuspend")},
+			name:   "add queue name with suspend is false",
+			oldJob: testingutil.MakeJob("job", "default").Obj(),
+			newJob: testingutil.MakeJob("job", "default").Queue("queue").Suspend(false).Obj(),
+			wantErr: field.ErrorList{
+				field.Invalid(queueNameLabelPath, "", apivalidation.FieldImmutableErrorMsg),
+			},
 		},
 		{
 			name:    "add queue name with suspend is true",
@@ -280,10 +282,12 @@ func TestValidateUpdate(t *testing.T) {
 			wantErr: nil,
 		},
 		{
-			name:    "change queue name with suspend is false",
-			oldJob:  testingutil.MakeJob("job", "default").Queue("queue").Obj(),
-			newJob:  testingutil.MakeJob("job", "default").Queue("queue2").Suspend(false).Obj(),
-			wantErr: field.ErrorList{field.Forbidden(queueNameLabelPath, "must not update queue name when job is unsuspend")},
+			name:   "change queue name with suspend is false",
+			oldJob: testingutil.MakeJob("job", "default").Queue("queue").Obj(),
+			newJob: testingutil.MakeJob("job", "default").Queue("queue2").Suspend(false).Obj(),
+			wantErr: field.ErrorList{
+				field.Invalid(queueNameLabelPath, "queue", apivalidation.FieldImmutableErrorMsg),
+			},
 		},
 		{
 			name:    "change queue name with suspend is true",
@@ -304,13 +308,17 @@ func TestValidateUpdate(t *testing.T) {
 				ParentWorkload("parent").
 				OwnerReference("parent", kubeflow.SchemeGroupVersionKind).
 				Obj(),
-			wantErr: field.ErrorList{field.Forbidden(parentWorkloadKeyPath, "this annotation is immutable")},
+			wantErr: field.ErrorList{
+				field.Invalid(parentWorkloadKeyPath, "parent", apivalidation.FieldImmutableErrorMsg),
+			},
 		},
 		{
-			name:    "update the non-empty parent workload to nil",
-			oldJob:  testingutil.MakeJob("job", "default").ParentWorkload("parent").Obj(),
-			newJob:  testingutil.MakeJob("job", "default").Obj(),
-			wantErr: field.ErrorList{field.Forbidden(parentWorkloadKeyPath, "this annotation is immutable")},
+			name:   "update the non-empty parent workload to nil",
+			oldJob: testingutil.MakeJob("job", "default").ParentWorkload("parent").Obj(),
+			newJob: testingutil.MakeJob("job", "default").Obj(),
+			wantErr: field.ErrorList{
+				field.Invalid(parentWorkloadKeyPath, "", apivalidation.FieldImmutableErrorMsg),
+			},
 		},
 		{
 			name:   "invalid queue name and immutable parent",
@@ -322,7 +330,7 @@ func TestValidateUpdate(t *testing.T) {
 				Obj(),
 			wantErr: field.ErrorList{
 				field.Invalid(queueNameLabelPath, "queue name", invalidRFC1123Message),
-				field.Forbidden(parentWorkloadKeyPath, "this annotation is immutable"),
+				field.Invalid(parentWorkloadKeyPath, "parent", apivalidation.FieldImmutableErrorMsg),
 			},
 		},
 		{

diff --git a/pkg/controller/jobs/rayjob/rayjob_webhook_test.go b/pkg/controller/jobs/rayjob/rayjob_webhook_test.go
@@ -223,7 +223,7 @@ func TestValidateUpdate(t *testing.T) {
 				ShutdownAfterJobFinishes(true).
 				Obj(),
 			wantErr: field.ErrorList{
-				field.Forbidden(field.NewPath("metadata", "labels").Key(constants.QueueLabel), "must not update queue name when job is unsuspend"),
+				field.Invalid(field.NewPath("metadata", "labels").Key(constants.QueueLabel), "queue", apivalidation.FieldImmutableErrorMsg),
 			}.ToAggregate(),
 		},
 		"managed - queue name can change while suspended": {