Repository: furiko-io/furiko (go)

Issue:
JobMutationWebhook nil pointer crash
Encountered panic in nil pointer exception:

```
2022/04/17 15:42:29 http: panic serving 10.244.0.1:8270: runtime error: invalid memory address or nil pointer dereference
goroutine 29143 [running]:
net/http.(*conn).serve.func1()
        /opt/hostedtoolcache/go/1.17.8/x64/src/net/http/server.go:1802 +0xb9
panic({0x17b32c0, 0x299acd0})
        /opt/hostedtoolcache/go/1.17.8/x64/src/runtime/panic.go:1047 +0x266
github.com/furiko-io/furiko/pkg/execution/mutation.(*Mutator).MutateJob(0xc000837630, 0xc00000a780)
        /home/runner/work/furiko/furiko/pkg/execution/mutation/mutation.go:131 +0xd8
github.com/furiko-io/furiko/pkg/execution/webhooks/jobmutatingwebhook.(*Webhook).Patch(0xc00000a5a0, 0xc000479040, 0x7fe542fa45b8)
        /home/runner/work/furiko/furiko/pkg/execution/webhooks/jobmutatingwebhook/webhook.go:176 +0x57
github.com/furiko-io/furiko/pkg/execution/webhooks/jobmutatingwebhook.(*Webhook).Handle(0x1c40778, {0xc00045bf40, 0x1c1e0b8}, 0xc000479040)
        /home/runner/work/furiko/furiko/pkg/execution/webhooks/jobmutatingwebhook/webhook.go:140 +0x26c
github.com/furiko-io/furiko/pkg/runtime/httphandler.HandleAdmissionWebhook.func1({0x1c1e0b8, 0xc00011c8c0}, 0xc00052ef00)
        /home/runner/work/furiko/furiko/pkg/runtime/httphandler/webhooks.go:67 +0x123
net/http.HandlerFunc.ServeHTTP(0xc00013ad00, {0x1c1e0b8, 0xc00011c8c0}, 0xc00011c8c0)
        /opt/hostedtoolcache/go/1.17.8/x64/src/net/http/server.go:2047 +0x2f
net/http.(*ServeMux).ServeHTTP(0xc0001a5928, {0x1c1e0b8, 0xc00011c8c0}, 0xc00052ef00)
        /opt/hostedtoolcache/go/1.17.8/x64/src/net/http/server.go:2425 +0x149
net/http.serverHandler.ServeHTTP({0x1c109d0}, {0x1c1e0b8, 0xc00011c8c0}, 0xc00052ef00)
        /opt/hostedtoolcache/go/1.17.8/x64/src/net/http/server.go:2879 +0x43b
net/http.(*conn).serve(0xc00037c960, {0x1c22878, 0xc000600720})
        /opt/hostedtoolcache/go/1.17.8/x64/src/net/http/server.go:1930 +0xb08
created by net/http.(*Server).Serve
        /opt/hostedtoolcache/go/1.17.8/x64/src/net/http/server.go:3034 +0x4e8
```

Pull request:
fix(webhook): Handle nil in mutation webhooks
Fixes #59.

- Extract JobPatcher and JobConfigPatcher from Mutator. There are many hidden assumptions in the `MutateX` methods, as we have seen that not only does it make the tests brittle, it also does not fully catch edge cases that may fall through the cracks.
- The `Patcher` types will simply just compose multiple `MutateX` methods in the desired order, and we will write tests based on the Operation and objects to be patched.
- Other fixes:
  - PodTemplateSpec should only add RestartPolicy on Job creates/updates, not JobConfig.
  - Handle nil for JobTemplate to add MaxAttempts.


Patch:
diff --git a/pkg/execution/mutation/mutation.go b/pkg/execution/mutation/mutation.go
index 4bc26b5..27aedb2 100644
--- a/pkg/execution/mutation/mutation.go
+++ b/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
diff --git a/pkg/execution/mutation/patcher_jobconfigs.go b/pkg/execution/mutation/patcher_jobconfigs.go
new file mode 100644
index 0000000..738678a
--- /dev/null
+++ b/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
+}
diff --git a/pkg/execution/mutation/patcher_jobs.go b/pkg/execution/mutation/patcher_jobs.go
new file mode 100644
index 0000000..22dfa89
--- /dev/null
+++ b/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
+}
diff --git a/pkg/execution/webhooks/jobconfigmutatingwebhook/webhook.go b/pkg/execution/webhooks/jobconfigmutatingwebhook/webhook.go
index f32e960..795794e 100644
--- a/pkg/execution/webhooks/jobconfigmutatingwebhook/webhook.go
+++ b/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)
 }
diff --git a/pkg/execution/webhooks/jobmutatingwebhook/webhook.go b/pkg/execution/webhooks/jobmutatingwebhook/webhook.go
index 46717ea..9593497 100644
--- a/pkg/execution/webhooks/jobmutatingwebhook/webhook.go
+++ b/pkg/execution/webhooks/jobmutatingwebhook/webhook.go
@@ -122,9 +122,17 @@ func (w *Webhook) Handle(
 	}
 
 	rj := &executionv1alpha1.Job{}
+	var existing *executionv1alpha1.Job
 
 	switch req.Operation {
-	case admissionv1.Update, admissionv1.Create:
+	case admissionv1.Update:
+		existing = &executionv1alpha1.Job{}
+		if err := json.Unmarshal(req.OldObject.Raw, existing); err != nil {
+			return nil, errors.Wrapf(err, "cannot decode object as executionv1alpha1.Job")
+		}
+		fallthrough
+
+	case admissionv1.Create:
 		if err := json.Unmarshal(req.Object.Raw, rj); err != nil {
 			return nil, errors.Wrapf(err, "cannot decode object as executionv1alpha1.Job")
 		}
@@ -137,7 +145,7 @@ func (w *Webhook) Handle(
 
 	// Patch the Job.
 	newRj := rj.DeepCopy()
-	result := w.Patch(req, newRj)
+	result := w.Patch(req, existing, newRj)
 
 	// Encountered validation error.
 	if len(result.Errors) > 0 {
@@ -171,14 +179,6 @@ func (w *Webhook) Handle(
 }
 
 // Patch returns the result after mutating a Job in-place.
-func (w *Webhook) Patch(req *admissionv1.AdmissionRequest, rj *executionv1alpha1.Job) *webhook.Result {
-	mutator := mutation.NewMutator(w)
-	result := mutator.MutateJob(rj)
-
-	switch req.Operation {
-	case admissionv1.Create:
-		result.Merge(mutator.MutateCreateJob(rj))
-	}
-
-	return result
+func (w *Webhook) Patch(req *admissionv1.AdmissionRequest, oldRj, rj *executionv1alpha1.Job) *webhook.Result {
+	return mutation.NewJobPatcher(w).Patch(req.Operation, oldRj, rj)
 }


Test patch:
diff --git a/pkg/execution/mutation/mutation_test.go b/pkg/execution/mutation/mutation_test.go
index ef63a25..903a687 100644
--- a/pkg/execution/mutation/mutation_test.go
+++ b/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)
 }
 
diff --git a/pkg/execution/mutation/patcher_jobs_test.go b/pkg/execution/mutation/patcher_jobs_test.go
new file mode 100644
index 0000000..b01ba20
--- /dev/null
+++ b/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...))
+			}
+		})
+	}
+}