Issue-6823 Conversion Webhook panic fix

Prior to this, the conversion of task spec from v1beta1 was panicing when the step template was nil since it could not access the underlying deprecated fields. This PR fixes that bug. Related issue: tektoncd#6823
JeromeJu · Jun 13, 2023 · fb292df · fb292df
1 parent 1bf65c5
commit fb292df
Show file tree

Hide file tree

Showing 2 changed files with 42 additions and 16 deletions.
diff --git a/pkg/apis/pipeline/v1beta1/task_conversion.go b/pkg/apis/pipeline/v1beta1/task_conversion.go
@@ -20,7 +20,6 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
-	"reflect"
 
 	"github.com/tektoncd/pipeline/pkg/apis/version"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -293,21 +292,21 @@ func retrieveTaskDeprecation(spec *TaskSpec) *taskDeprecation {
 			DeprecatedTTY:                      s.DeprecatedTTY,
 		})
 	}
-	dst := &StepTemplate{
-		DeprecatedName:                     spec.StepTemplate.DeprecatedName,
-		DeprecatedPorts:                    spec.StepTemplate.DeprecatedPorts,
-		DeprecatedLivenessProbe:            spec.StepTemplate.DeprecatedLivenessProbe,
-		DeprecatedReadinessProbe:           spec.StepTemplate.DeprecatedReadinessProbe,
-		DeprecatedStartupProbe:             spec.StepTemplate.DeprecatedStartupProbe,
-		DeprecatedLifecycle:                spec.StepTemplate.DeprecatedLifecycle,
-		DeprecatedTerminationMessagePath:   spec.StepTemplate.DeprecatedTerminationMessagePath,
-		DeprecatedTerminationMessagePolicy: spec.StepTemplate.DeprecatedTerminationMessagePolicy,
-		DeprecatedStdin:                    spec.StepTemplate.DeprecatedStdin,
-		DeprecatedStdinOnce:                spec.StepTemplate.DeprecatedStdinOnce,
-		DeprecatedTTY:                      spec.StepTemplate.DeprecatedTTY,
-	}
-	if reflect.DeepEqual(dst, &StepTemplate{}) {
-		dst = nil
+	var dst *StepTemplate
+	if spec.StepTemplate != nil {
+		dst = &StepTemplate{
+			DeprecatedName:                     spec.StepTemplate.DeprecatedName,
+			DeprecatedPorts:                    spec.StepTemplate.DeprecatedPorts,
+			DeprecatedLivenessProbe:            spec.StepTemplate.DeprecatedLivenessProbe,
+			DeprecatedReadinessProbe:           spec.StepTemplate.DeprecatedReadinessProbe,
+			DeprecatedStartupProbe:             spec.StepTemplate.DeprecatedStartupProbe,
+			DeprecatedLifecycle:                spec.StepTemplate.DeprecatedLifecycle,
+			DeprecatedTerminationMessagePath:   spec.StepTemplate.DeprecatedTerminationMessagePath,
+			DeprecatedTerminationMessagePolicy: spec.StepTemplate.DeprecatedTerminationMessagePolicy,
+			DeprecatedStdin:                    spec.StepTemplate.DeprecatedStdin,
+			DeprecatedStdinOnce:                spec.StepTemplate.DeprecatedStdinOnce,
+			DeprecatedTTY:                      spec.StepTemplate.DeprecatedTTY,
+		}
 	}
 	return &taskDeprecation{
 		DeprecatedSteps:        ds,

diff --git a/pkg/apis/pipeline/v1beta1/task_conversion_test.go b/pkg/apis/pipeline/v1beta1/task_conversion_test.go
@@ -235,6 +235,24 @@ spec:
   - name: step-1
   stepTemplate:
     image: foo
+`
+	taskWithoutStepTemplateYAML := `
+metadata:
+  name: foo
+  namespace: bar
+  generation: 1
+spec:
+  steps:
+  - image: alpine
+    name: echo
+    readinessProbe:
+      exec:
+        command:
+        - cat
+        - /tmp/healthy
+    resources: {}
+    script: |
+      echo "Good Morning!"
 `
 	simpleTaskV1beta1 := parse.MustParseV1beta1Task(t, simpleTaskYAML)
 	simpleTaskV1 := parse.MustParseV1Task(t, simpleTaskYAML)
@@ -252,6 +270,11 @@ spec:
 			`[{"name":"","ports":[{"name":"port","containerPort":0}],"resources":{},"livenessProbe":{"initialDelaySeconds":1},"readinessProbe":{"initialDelaySeconds":2},"startupProbe":{"initialDelaySeconds":3},"lifecycle":{"postStart":{"exec":{"command":["lifecycle command"]}}},"terminationMessagePath":"path","terminationMessagePolicy":"policy","stdin":true,"stdinOnce":true,"tty":true}],` +
 			`"deprecatedStepTemplate":{"name":"","ports":[{"name":"port","containerPort":0}],"resources":{},"livenessProbe":{"initialDelaySeconds":1},"readinessProbe":{"initialDelaySeconds":2},"startupProbe":{"initialDelaySeconds":3},"lifecycle":{"postStart":{"exec":{"command":["lifecycle command"]}}},"terminationMessagePath":"path","terminationMessagePolicy":"policy","stdin":true,"stdinOnce":true,"tty":true}}}`,
 	}
+	taskWithoutStepTemplateYAMLV1beta1 := parse.MustParseV1beta1Task(t, taskWithoutStepTemplateYAML)
+	taskWithoutStepTemplateYAMLV1 := parse.MustParseV1Task(t, taskWithoutStepTemplateYAML)
+	taskWithoutStepTemplateYAMLV1.ObjectMeta.Annotations = map[string]string{
+		v1beta1.TaskDeprecationsAnnotationKey: `{"foo":{"deprecatedSteps":[{"name":"","resources":{},"readinessProbe":{"exec":{"command":["cat","/tmp/healthy"]}}}]}}`,
+	}
 
 	tests := []struct {
 		name        string
@@ -273,6 +296,10 @@ spec:
 		name:        "task conversion deprecated fields",
 		v1beta1Task: taskWithDeprecatedFieldsV1beta1,
 		v1Task:      taskWithDeprecatedFieldsV1,
+	}, {
+		name:        "task conversion deprecated step template fields panic check",
+		v1beta1Task: taskWithoutStepTemplateYAMLV1beta1,
+		v1Task:      taskWithoutStepTemplateYAMLV1,
 	},
 	}
 	var ignoreTypeMeta = cmpopts.IgnoreFields(metav1.TypeMeta{}, "Kind", "APIVersion")