Add validation for child jobs without ownerReference

Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>
kubernetes-sigs · Jun 17, 2023 · 0349b30 · 0349b30
1 parent 860bd12
commit 0349b30
Show file tree

Hide file tree

Showing 3 changed files with 49 additions and 9 deletions.
diff --git 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 value, exists := job.Object().GetAnnotations()[constants.ParentWorkloadAnnotation]; exists {
+		if job.Object().GetOwnerReferences() == nil {
+			allErrs = append(allErrs, field.Invalid(parentWorkloadKeyPath, value, "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
@@ -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
@@ -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.Invalid(parentWorkloadKeyPath, "parent-workload", "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"),