From d8a854e1c0071e13cff8f2a6848439540c6fbb8b Mon Sep 17 00:00:00 2001 From: Yuki Iwai Date: Sat, 17 Jun 2023 06:56:48 +0900 Subject: [PATCH] Add validation for child jobs without ownerReference Signed-off-by: Yuki Iwai --- pkg/controller/jobframework/validation.go | 10 +++++ pkg/controller/jobs/job/job_webhook.go | 1 + pkg/controller/jobs/job/job_webhook_test.go | 47 +++++++++++++++++---- 3 files changed, 49 insertions(+), 9 deletions(-) diff --git a/pkg/controller/jobframework/validation.go b/pkg/controller/jobframework/validation.go index 8ae012dd9a..0107c51c51 100644 --- a/pkg/controller/jobframework/validation.go +++ b/pkg/controller/jobframework/validation.go @@ -57,6 +57,16 @@ func validateLabelAsCRDName(job GenericJob, crdNameLabel string) field.ErrorList return allErrs } +func ValidateCreateForParentWorkload(job GenericJob) field.ErrorList { + var allErrs field.ErrorList + if _, exists := job.Object().GetAnnotations()[constants.ParentWorkloadAnnotation]; exists { + if job.Object().GetOwnerReferences() == nil { + allErrs = append(allErrs, field.Forbidden(parentWorkloadKeyPath, "must not add a parent workload annotation to job without OwnerReference")) + } + } + return allErrs +} + func ValidateUpdateForQueueName(oldJob, newJob GenericJob) field.ErrorList { var allErrs field.ErrorList if !newJob.IsSuspended() && (QueueName(oldJob) != QueueName(newJob)) { diff --git a/pkg/controller/jobs/job/job_webhook.go b/pkg/controller/jobs/job/job_webhook.go index 7dbcefbb3e..45e28b9b94 100644 --- a/pkg/controller/jobs/job/job_webhook.go +++ b/pkg/controller/jobs/job/job_webhook.go @@ -100,6 +100,7 @@ func validateCreate(job *Job) field.ErrorList { allErrs = append(allErrs, jobframework.ValidateAnnotationAsCRDName(job, constants.ParentWorkloadAnnotation)...) allErrs = append(allErrs, jobframework.ValidateCreateForQueueName(job)...) allErrs = append(allErrs, validatePartialAdmissionCreate(job)...) + allErrs = append(allErrs, jobframework.ValidateCreateForParentWorkload(job)...) return allErrs } diff --git a/pkg/controller/jobs/job/job_webhook_test.go b/pkg/controller/jobs/job/job_webhook_test.go index e76e4e923f..2d51c8a82e 100644 --- a/pkg/controller/jobs/job/job_webhook_test.go +++ b/pkg/controller/jobs/job/job_webhook_test.go @@ -58,15 +58,33 @@ func TestValidateCreate(t *testing.T) { wantErr: nil, }, { - name: "valid parent-workload annotation", - job: testingutil.MakeJob("job", "default").ParentWorkload("parent-workload-name").Queue("queue").Obj(), + name: "valid parent-workload annotation", + job: testingutil.MakeJob("job", "default"). + ParentWorkload("parent-workload-name"). + Queue("queue"). + OwnerReference("parent-workload-name", kubeflow.SchemeGroupVersionKind). + Obj(), wantErr: nil, }, { - name: "invalid parent-workload annotation", - job: testingutil.MakeJob("job", "default").ParentWorkload("parent workload name").Queue("queue").Obj(), + name: "invalid parent-workload annotation", + job: testingutil.MakeJob("job", "default"). + ParentWorkload("parent workload name"). + OwnerReference("parent workload name", kubeflow.SchemeGroupVersionKind). + Queue("queue"). + Obj(), wantErr: field.ErrorList{field.Invalid(parentWorkloadKeyPath, "parent workload name", invalidRFC1123Message)}, }, + { + name: "invalid parent-workload annotation (owner is missing)", + job: testingutil.MakeJob("job", "default"). + ParentWorkload("parent-workload"). + Queue("queue"). + Obj(), + wantErr: field.ErrorList{ + field.Forbidden(parentWorkloadKeyPath, "must not add a parent workload annotation to job without OwnerReference"), + }, + }, { name: "invalid queue-name label", job: testingutil.MakeJob("job", "default").Queue("queue name").Obj(), @@ -79,7 +97,11 @@ func TestValidateCreate(t *testing.T) { }, { name: "invalid queue-name and parent-workload annotation", - job: testingutil.MakeJob("job", "default").Queue("queue name").ParentWorkload("parent workload name").Obj(), + job: testingutil.MakeJob("job", "default"). + Queue("queue name"). + ParentWorkload("parent workload name"). + OwnerReference("parent workload name", kubeflow.SchemeGroupVersionKind). + Obj(), wantErr: field.ErrorList{ field.Invalid(parentWorkloadKeyPath, "parent workload name", invalidRFC1123Message), field.Invalid(queueNameLabelPath, "queue name", invalidRFC1123Message), @@ -173,9 +195,12 @@ func TestValidateUpdate(t *testing.T) { wantErr: field.ErrorList{field.Invalid(queueNameLabelPath, "queue name", invalidRFC1123Message)}, }, { - name: "update the nil parent workload to non-empty", - oldJob: testingutil.MakeJob("job", "default").Obj(), - newJob: testingutil.MakeJob("job", "default").ParentWorkload("parent").Obj(), + name: "update the nil parent workload to non-empty", + oldJob: testingutil.MakeJob("job", "default").Obj(), + newJob: testingutil.MakeJob("job", "default"). + ParentWorkload("parent"). + OwnerReference("parent", kubeflow.SchemeGroupVersionKind). + Obj(), wantErr: field.ErrorList{field.Forbidden(parentWorkloadKeyPath, "this annotation is immutable")}, }, { @@ -187,7 +212,11 @@ func TestValidateUpdate(t *testing.T) { { name: "invalid queue name and immutable parent", oldJob: testingutil.MakeJob("job", "default").Obj(), - newJob: testingutil.MakeJob("job", "default").Queue("queue name").ParentWorkload("parent").Obj(), + newJob: testingutil.MakeJob("job", "default"). + Queue("queue name"). + ParentWorkload("parent"). + OwnerReference("parent", kubeflow.SchemeGroupVersionKind). + Obj(), wantErr: field.ErrorList{ field.Invalid(queueNameLabelPath, "queue name", invalidRFC1123Message), field.Forbidden(parentWorkloadKeyPath, "this annotation is immutable"),