From e162f8508b503d20feb9b31fd0b27d91e58f2c2f Mon Sep 17 00:00:00 2001 From: Hiroyuki Moriya <41197469+Gekko0114@users.noreply.github.com> Date: Wed, 4 Oct 2023 23:45:38 +0900 Subject: [PATCH] use validateImmutableField (#1179) --- pkg/controller/jobframework/validation.go | 17 +++------ pkg/controller/jobs/job/job_webhook_test.go | 36 +++++++++++-------- .../jobs/rayjob/rayjob_webhook_test.go | 2 +- 3 files changed, 27 insertions(+), 28 deletions(-) diff --git a/pkg/controller/jobframework/validation.go b/pkg/controller/jobframework/validation.go index ed7ab9777993..caf29d5cc334 100644 --- 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 index b65205af9502..86953d500ce2 100644 --- 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 index 81fdcd5ad18d..049f1de705ca 100644 --- 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": {