fix(webhook): Handle nil in mutation webhooks

pkg/execution/mutation/mutation.go

@@ -62,10 +62,6 @@ func NewMutator(ctrlContext controllercontext.Context) *Mutator {
 func (m *Mutator) MutateJobConfig(rjc *v1alpha1.JobConfig) *webhook.Result {
 	result := webhook.NewResult()
 
-	// Specify default values for JobTemplate.
-	result.Merge(m.MutateJobTemplate(&rjc.Spec.Template,
-		field.NewPath("spec").Child("spec").Child("template")))
-
 	// Specify default values for OptionSpec.
 	if spec := rjc.Spec.Option; spec != nil {
 		rjc.Spec.Option = options.DefaultOptionSpec(spec)
@@ -128,9 +124,12 @@ func (m *Mutator) MutateJob(rj *v1alpha1.Job) *webhook.Result {
 	if rj.Spec.TTLSecondsAfterFinished == nil {
 		rj.Spec.TTLSecondsAfterFinished = cfg.DefaultTTLSecondsAfterFinished
 	}
-	if rj.Spec.Template.MaxAttempts == nil {
-		rj.Spec.Template.MaxAttempts = pointer.Int32(1)
+
+	// Specify default values for JobTemplateSpec.
+	if rj.Spec.Template == nil {
+		rj.Spec.Template = &v1alpha1.JobTemplateSpec{}
 	}
+	result.Merge(m.MutateJobTemplateSpec(rj.Spec.Template, field.NewPath("spec", "template")))
 
 	return result
 }
@@ -292,12 +291,27 @@ func (m *Mutator) evaluateOptionValues(rj *v1alpha1.Job, rjc *v1alpha1.JobConfig
 	return result
 }
 
-func (m *Mutator) MutateJobTemplate(spec *v1alpha1.JobTemplate, fldPath *field.Path) *webhook.Result {
+// MutateJobTemplateSpec mutates a JobTemplateSpec in-place.
+func (m *Mutator) MutateJobTemplateSpec(spec *v1alpha1.JobTemplateSpec, fldPath *field.Path) *webhook.Result {
+	result := webhook.NewResult()
+
+	// Add MaxAttempts if not specified.
+	if spec.MaxAttempts == nil {
+		spec.MaxAttempts = pointer.Int32(1)
+	}
+
+	// Mutate task's PodTemplateSpec.
+	m.MutatePodTemplateSpec(&spec.Task.Template, fldPath.Child("task", "template"))
+
+	return result
+}
+
+// MutatePodTemplateSpec mutates PodTemplateSpec in-place.
+func (m *Mutator) MutatePodTemplateSpec(spec *corev1.PodTemplateSpec, fldPath *field.Path) *webhook.Result {
 	result := webhook.NewResult()
 
-	// Specify default RestartPolicy.
-	if spec.Spec.Task.Template.Spec.RestartPolicy == "" {
-		spec.Spec.Task.Template.Spec.RestartPolicy = corev1.RestartPolicyNever
+	if spec.Spec.RestartPolicy == "" {
+		spec.Spec.RestartPolicy = corev1.RestartPolicyNever
 	}
 
 	return result

pkg/execution/mutation/mutation_test.go

@@ -43,6 +43,7 @@ import (
 	"github.com/furiko-io/furiko/pkg/execution/mutation"
 	"github.com/furiko-io/furiko/pkg/execution/tasks"
 	"github.com/furiko-io/furiko/pkg/execution/variablecontext"
+	"github.com/furiko-io/furiko/pkg/runtime/controllercontext"
 	"github.com/furiko-io/furiko/pkg/runtime/controllercontext/mock"
 	"github.com/furiko-io/furiko/pkg/utils/execution/jobconfig"
 	"github.com/furiko-io/furiko/pkg/utils/webhook"
@@ -228,7 +229,7 @@ func TestMutator_MutateJobConfig(t *testing.T) {
 			},
 		},
 		{
-			name: "default JobTemplate",
+			name: "don't need to default JobTemplate",
 			rjc: &v1alpha1.JobConfig{
 				ObjectMeta: objectMetaJobConfig,
 				Spec: v1alpha1.JobConfigSpec{
@@ -241,12 +242,6 @@ func TestMutator_MutateJobConfig(t *testing.T) {
 					},
 				},
 			},
-			want: &v1alpha1.JobConfig{
-				ObjectMeta: objectMetaJobConfig,
-				Spec: v1alpha1.JobConfigSpec{
-					Template: jobTemplateSpecBasic,
-				},
-			},
 		},
 		{
 			name: "default OptionSpec",
@@ -1253,10 +1248,8 @@ func (m *mockProvider) MakeVariablesFromTask(rj *v1alpha1.Job, task *tasks.TaskT
 	return nil
 }
 
-func setup(t *testing.T, cfgs map[configv1alpha1.ConfigName]runtime.Object, rjcs []*v1alpha1.JobConfig) *mutation.Mutator {
-	ctx, cancel := context.WithCancel(context.Background())
-	defer cancel()
-
+func setupContext(t *testing.T, cfgs map[configv1alpha1.ConfigName]runtime.Object, rjcs []*v1alpha1.JobConfig) controllercontext.Context {
+	ctx := context.Background()
 	ctrlContext := mock.NewContext()
 	ctrlContext.MockConfigs().SetConfigs(cfgs)
 	hasSynced := ctrlContext.Informers().Furiko().Execution().V1alpha1().JobConfigs().Informer().HasSynced
@@ -1279,6 +1272,11 @@ func setup(t *testing.T, cfgs map[configv1alpha1.ConfigName]runtime.Object, rjcs
 	// Replace provider
 	variablecontext.ContextProvider = &noopProvider{}
 
+	return ctrlContext
+}
+
+func setup(t *testing.T, cfgs map[configv1alpha1.ConfigName]runtime.Object, rjcs []*v1alpha1.JobConfig) *mutation.Mutator {
+	ctrlContext := setupContext(t, cfgs, rjcs)
 	return mutation.NewMutator(ctrlContext)
 }
 

pkg/execution/mutation/patcher_jobconfigs.go

@@ -0,0 +1,62 @@
+/*
+ * Copyright 2022 The Furiko Authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package mutation
+
+import (
+	admissionv1 "k8s.io/api/admission/v1"
+
+	execution "github.com/furiko-io/furiko/apis/execution/v1alpha1"
+	"github.com/furiko-io/furiko/pkg/runtime/controllercontext"
+	"github.com/furiko-io/furiko/pkg/utils/webhook"
+)
+
+// JobConfigPatcher encapsulates high-level patch methods for JobConfigs.
+type JobConfigPatcher struct {
+	mutator *Mutator
+}
+
+func NewJobConfigPatcher(ctrlContext controllercontext.Context) *JobConfigPatcher {
+	return &JobConfigPatcher{
+		mutator: NewMutator(ctrlContext),
+	}
+}
+
+func (p *JobConfigPatcher) Patch(operation admissionv1.Operation, oldRjc, rjc *execution.JobConfig) *webhook.Result {
+	switch operation {
+	case admissionv1.Create:
+		return p.patchCreate(oldRjc, rjc)
+	case admissionv1.Update:
+		return p.patchUpdate(oldRjc, rjc)
+	}
+
+	// Unhandled operation
+	return webhook.NewResult()
+}
+
+func (p *JobConfigPatcher) patchCreate(_, rjc *execution.JobConfig) *webhook.Result {
+	result := webhook.NewResult()
+	result.Merge(p.mutator.MutateCreateJobConfig(rjc))
+	result.Merge(p.mutator.MutateJobConfig(rjc))
+	return result
+}
+
+func (p *JobConfigPatcher) patchUpdate(oldRjc, rjc *execution.JobConfig) *webhook.Result {
+	result := webhook.NewResult()
+	result.Merge(p.mutator.MutateJobConfig(rjc))
+	result.Merge(p.mutator.MutateUpdateJobConfig(oldRjc, rjc))
+	return result
+}

pkg/execution/mutation/patcher_jobs.go

@@ -0,0 +1,61 @@
+/*
+ * Copyright 2022 The Furiko Authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package mutation
+
+import (
+	admissionv1 "k8s.io/api/admission/v1"
+
+	execution "github.com/furiko-io/furiko/apis/execution/v1alpha1"
+	"github.com/furiko-io/furiko/pkg/runtime/controllercontext"
+	"github.com/furiko-io/furiko/pkg/utils/webhook"
+)
+
+// JobPatcher encapsulates high-level patch methods for Jobs.
+type JobPatcher struct {
+	mutator *Mutator
+}
+
+func NewJobPatcher(ctrlContext controllercontext.Context) *JobPatcher {
+	return &JobPatcher{
+		mutator: NewMutator(ctrlContext),
+	}
+}
+
+func (p *JobPatcher) Patch(operation admissionv1.Operation, oldRj, rj *execution.Job) *webhook.Result {
+	switch operation {
+	case admissionv1.Create:
+		return p.patchCreate(rj)
+	case admissionv1.Update:
+		return p.patchUpdate(oldRj, rj)
+	}
+
+	// Unhandled operation
+	return webhook.NewResult()
+}
+
+func (p *JobPatcher) patchCreate(rj *execution.Job) *webhook.Result {
+	result := webhook.NewResult()
+	result.Merge(p.mutator.MutateCreateJob(rj))
+	result.Merge(p.mutator.MutateJob(rj))
+	return result
+}
+
+func (p *JobPatcher) patchUpdate(_, rj *execution.Job) *webhook.Result {
+	result := webhook.NewResult()
+	result.Merge(p.mutator.MutateJob(rj))
+	return result
+}

pkg/execution/mutation/patcher_jobs_test.go

@@ -0,0 +1,133 @@
+/*
+ * Copyright 2022 The Furiko Authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package mutation_test
+
+import (
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
+	admissionv1 "k8s.io/api/admission/v1"
+	corev1 "k8s.io/api/core/v1"
+	"k8s.io/utils/pointer"
+
+	"github.com/furiko-io/furiko/apis/execution/v1alpha1"
+	"github.com/furiko-io/furiko/pkg/config"
+	"github.com/furiko-io/furiko/pkg/execution/mutation"
+)
+
+func TestNewJobPatcher(t *testing.T) {
+	tests := []struct {
+		name         string
+		operation    admissionv1.Operation
+		oldRj        *v1alpha1.Job
+		rj           *v1alpha1.Job
+		want         *v1alpha1.Job
+		wantErrors   string
+		wantWarnings []string
+	}{
+		{
+			name:      "basic mutation for create with template",
+			operation: admissionv1.Create,
+			rj: &v1alpha1.Job{
+				ObjectMeta: objectMetaJob,
+				Spec: v1alpha1.JobSpec{
+					Template: &v1alpha1.JobTemplateSpec{
+						Task: v1alpha1.JobTaskSpec{
+							Template: corev1.PodTemplateSpec{
+								Spec: podTemplateSpecBare.Spec,
+							},
+						},
+					},
+				},
+			},
+			want: &v1alpha1.Job{
+				ObjectMeta: objectMetaJobWithFinalizer,
+				Spec: v1alpha1.JobSpec{
+					Type: v1alpha1.JobTypeAdhoc,
+					Template: &v1alpha1.JobTemplateSpec{
+						Task: v1alpha1.JobTaskSpec{
+							Template: corev1.PodTemplateSpec{
+								Spec: corev1.PodSpec{
+									Containers:    podTemplateSpecBare.Spec.Containers,
+									RestartPolicy: corev1.RestartPolicyNever,
+								},
+							},
+						},
+						MaxAttempts: pointer.Int32(1),
+					},
+					TTLSecondsAfterFinished: config.DefaultJobExecutionConfig.DefaultTTLSecondsAfterFinished,
+				},
+			},
+		},
+		{
+			name:      "basic mutation for create with ConfigName",
+			operation: admissionv1.Create,
+			rj: &v1alpha1.Job{
+				ObjectMeta: objectMetaJob,
+				Spec: v1alpha1.JobSpec{
+					ConfigName: objectMetaJobConfig.Name,
+				},
+			},
+			want: &v1alpha1.Job{
+				ObjectMeta: objectMetaJobWithAllReferences,
+				Spec: v1alpha1.JobSpec{
+					Type: v1alpha1.JobTypeAdhoc,
+					Template: &v1alpha1.JobTemplateSpec{
+						Task:        jobTemplateSpecBasic.Spec.Task,
+						MaxAttempts: pointer.Int32(1),
+					},
+					StartPolicy:             &startPolicyBasic,
+					TTLSecondsAfterFinished: config.DefaultJobExecutionConfig.DefaultTTLSecondsAfterFinished,
+				},
+			},
+		},
+	}
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			ctrlContext := setupContext(t, nil, []*v1alpha1.JobConfig{
+				{
+					ObjectMeta: objectMetaJobConfig,
+					Spec: v1alpha1.JobConfigSpec{
+						Template:    jobTemplateSpecBasic,
+						Concurrency: concurrencySpecBasic,
+					},
+				},
+			})
+			patcher := mutation.NewJobPatcher(ctrlContext)
+			newRj := tt.rj.DeepCopy()
+			resp := patcher.Patch(tt.operation, tt.oldRj, newRj)
+
+			if err := checkResult(resp, tt.wantErrors, tt.wantWarnings); err != "" {
+				t.Errorf("MutateJob() %v", err)
+			}
+			if tt.wantErrors != "" {
+				return
+			}
+
+			opts := []cmp.Option{cmpopts.EquateEmpty()}
+			if tt.want == nil {
+				if !cmp.Equal(newRj, tt.rj, opts...) {
+					t.Errorf("Patch() expected no change\ndiff = %v", cmp.Diff(tt.rj, newRj, opts...))
+				}
+			} else if !cmp.Equal(newRj, tt.want, opts...) {
+				t.Errorf("Patch() not equal\ndiff = %v", cmp.Diff(tt.want, newRj, opts...))
+			}
+		})
+	}
+}

pkg/execution/webhooks/jobconfigmutatingwebhook/webhook.go

@@ -154,16 +154,7 @@ func (w *Webhook) Handle(
 	return resp, nil
 }
 
+// Patch returns the result after mutating a JobConfig in-place.
 func (w *Webhook) Patch(req *admissionv1.AdmissionRequest, oldRjc, rjc *executionv1alpha1.JobConfig) *webhook.Result {
-	mutator := mutation.NewMutator(w)
-	result := mutator.MutateJobConfig(rjc)
-
-	switch req.Operation {
-	case admissionv1.Create:
-		result.Merge(mutator.MutateCreateJobConfig(rjc))
-	case admissionv1.Update:
-		result.Merge(mutator.MutateUpdateJobConfig(oldRjc, rjc))
-	}
-
-	return result
+	return mutation.NewJobConfigPatcher(w).Patch(req.Operation, oldRjc, rjc)
 }