fix: Fix remote task params default-value substitution

Fixes #8637

When substituting Params in a StepAction was initially implemented, the
defaults were taken from the `TaskRun.Spec.TaskSpec.Params`. This works
when the Task is defined in place, however if the TaskRun does not
define the Task inline and instead references a remote Task from
`TaskRun.Spec.TaskRef`, then any param defaults failed to be applied.

When `GetStepActionFunc` is called from the TaskRun reconciler, it's
passed a TaskSpec which comes from either the `TaskRun.Spec.TaskSpec` or
a remotely-resolved TaskRef. By checking this TaskSpec for the Param
defaults, instead of `TaskRun.Spec.TaskSpec`, we can ensure the defaults
will always be applied when they're specified

Author: aThorp96

examples/v1/taskruns/default_task_params.yaml

@@ -0,0 +1,27 @@
+apiVersion: tekton.dev/v1
+kind: Task
+metadata:
+  # This has to be explicit instead of `generateName`, since it will be referenced
+  # by the TaskRun
+  name: example-default-task-param
+spec:
+  params:
+    - name: input
+      default: "No input provided, but that's okay!"
+  steps:
+    - name: echo-input
+      image: mirror.gcr.io/ubuntu
+      script: |
+        echo "$(params.input)"
+---
+apiVersion: tekton.dev/v1
+kind: TaskRun
+metadata:
+  generateName: default-task-params-run-
+spec:
+  taskRef:
+    name: example-default-task-param
+    # # Uncomment this block to override the default param value!
+    # params:
+    #   - name: input
+    #     value: "You can supply the param from the TaskRun if the default not what you want"

pkg/reconciler/taskrun/resources/taskref.go

@@ -138,15 +138,15 @@ func GetTaskFunc(ctx context.Context, k8s kubernetes.Interface, tekton clientset
 // It also requires a kubeclient, tektonclient, requester in case it needs to find that task in
 // cluster or authorize against an external repository. It will figure out whether it needs to look in the cluster or in
 // a remote location to fetch the reference.
-func GetStepActionFunc(tekton clientset.Interface, k8s kubernetes.Interface, requester remoteresource.Requester, tr *v1.TaskRun, step *v1.Step) GetStepAction {
+func GetStepActionFunc(tekton clientset.Interface, k8s kubernetes.Interface, requester remoteresource.Requester, tr *v1.TaskRun, taskSpec v1.TaskSpec, step *v1.Step) GetStepAction {
 	trName := tr.Name
 	namespace := tr.Namespace
 	if step.Ref != nil && step.Ref.Resolver != "" && requester != nil {
 		// Return an inline function that implements GetStepAction by calling Resolver.Get with the specified StepAction type and
 		// casting it to a StepAction.
 		return func(ctx context.Context, name string) (*v1beta1.StepAction, *v1.RefSource, error) {
 			// Perform params replacements for StepAction resolver params
-			ApplyParameterSubstitutionInResolverParams(tr, step)
+			ApplyParameterSubstitutionInResolverParams(tr, taskSpec, step)
 			resolverPayload := remoteresource.ResolverPayload{
 				Name:      trName,
 				Namespace: namespace,
@@ -167,14 +167,14 @@ func GetStepActionFunc(tekton clientset.Interface, k8s kubernetes.Interface, req
 }
 
 // ApplyParameterSubstitutionInResolverParams applies parameter substitutions in resolver params for Step Ref.
-func ApplyParameterSubstitutionInResolverParams(tr *v1.TaskRun, step *v1.Step) {
+func ApplyParameterSubstitutionInResolverParams(tr *v1.TaskRun, taskSpec v1.TaskSpec, step *v1.Step) {
 	stringReplacements := make(map[string]string)
 	arrayReplacements := make(map[string][]string)
 	objectReplacements := make(map[string]map[string]string)
-	if tr.Spec.TaskSpec != nil {
-		defaultSR, defaultAR, defaultOR := replacementsFromDefaultParams(tr.Spec.TaskSpec.Params)
-		stringReplacements, arrayReplacements, objectReplacements = extendReplacements(stringReplacements, arrayReplacements, objectReplacements, defaultSR, defaultAR, defaultOR)
-	}
+
+	defaultSR, defaultAR, defaultOR := replacementsFromDefaultParams(taskSpec.Params)
+	stringReplacements, arrayReplacements, objectReplacements = extendReplacements(stringReplacements, arrayReplacements, objectReplacements, defaultSR, defaultAR, defaultOR)
+
 	paramSR, paramAR, paramOR := replacementsFromParams(tr.Spec.Params)
 	stringReplacements, arrayReplacements, objectReplacements = extendReplacements(stringReplacements, arrayReplacements, objectReplacements, paramSR, paramAR, paramOR)
 	step.Ref.Params = step.Ref.Params.ReplaceVariables(stringReplacements, arrayReplacements, objectReplacements)

pkg/reconciler/taskrun/resources/taskref_test.go

@@ -347,6 +347,7 @@ func TestStepActionResolverParamReplacements(t *testing.T) {
 		name      string
 		namespace string
 		taskrun   *v1.TaskRun
+		taskSpec  *v1.TaskSpec
 		want      *v1.Step
 	}{{
 		name:      "default taskspec parms",
@@ -585,11 +586,53 @@ func TestStepActionResolverParamReplacements(t *testing.T) {
 				},
 			},
 		},
+	}, {
+		name:      "defaults from remote task",
+		namespace: "default",
+		taskrun: &v1.TaskRun{
+			ObjectMeta: metav1.ObjectMeta{Name: "some-tr"},
+			Spec: v1.TaskRunSpec{
+				TaskRef: &v1.TaskRef{
+					Name: "resolved-task-name",
+				},
+			},
+		},
+		taskSpec: &v1.TaskSpec{
+			Params: []v1.ParamSpec{{
+				Name:    "resolver-param",
+				Default: v1.NewStructuredValues("foo/bar"),
+			}},
+			Steps: []v1.Step{{
+				Ref: &v1.Ref{
+					ResolverRef: v1.ResolverRef{
+						Resolver: "git",
+						Params: []v1.Param{{
+							Name:  "pathInRepo",
+							Value: *v1.NewStructuredValues("$(params.resolver-param)"),
+						}},
+					},
+				},
+			}},
+		},
+		want: &v1.Step{
+			Ref: &v1.Ref{
+				ResolverRef: v1.ResolverRef{
+					Resolver: "git",
+					Params: []v1.Param{{
+						Name:  "pathInRepo",
+						Value: *v1.NewStructuredValues("foo/bar"),
+					}},
+				},
+			},
+		},
 	}}
 	for _, tc := range testcases {
 		t.Run(tc.name, func(t *testing.T) {
-			step := &tc.taskrun.Spec.TaskSpec.Steps[0]
-			resources.ApplyParameterSubstitutionInResolverParams(tc.taskrun, step)
+			if tc.taskSpec == nil {
+				tc.taskSpec = tc.taskrun.Spec.TaskSpec
+			}
+			step := &tc.taskSpec.Steps[0]
+			resources.ApplyParameterSubstitutionInResolverParams(tc.taskrun, *tc.taskSpec, step)
 			if d := cmp.Diff(tc.want, step); tc.want != nil && d != "" {
 				t.Error(diff.PrintWantGot(d))
 			}
@@ -861,7 +904,7 @@ func TestGetStepActionFunc_Local(t *testing.T) {
 	for _, tc := range testcases {
 		t.Run(tc.name, func(t *testing.T) {
 			tektonclient := fake.NewSimpleClientset(tc.localStepActions...)
-			fn := resources.GetStepActionFunc(tektonclient, nil, nil, tc.taskRun, &tc.taskRun.Spec.TaskSpec.Steps[0])
+			fn := resources.GetStepActionFunc(tektonclient, nil, nil, tc.taskRun, *tc.taskRun.Spec.TaskSpec, &tc.taskRun.Spec.TaskSpec.Steps[0])
 
 			stepAction, refSource, err := fn(ctx, tc.taskRun.Spec.TaskSpec.Steps[0].Ref.Name)
 			if err != nil {
@@ -922,7 +965,7 @@ func TestGetStepActionFunc_RemoteResolution_Success(t *testing.T) {
 				},
 			}
 			tektonclient := fake.NewSimpleClientset()
-			fn := resources.GetStepActionFunc(tektonclient, nil, requester, tr, &tr.Spec.TaskSpec.Steps[0])
+			fn := resources.GetStepActionFunc(tektonclient, nil, requester, tr, *tr.Spec.TaskSpec, &tr.Spec.TaskSpec.Steps[0])
 
 			resolvedStepAction, resolvedRefSource, err := fn(ctx, tr.Spec.TaskSpec.Steps[0].Ref.Name)
 			if tc.wantErr {
@@ -983,7 +1026,7 @@ func TestGetStepActionFunc_RemoteResolution_Error(t *testing.T) {
 				},
 			}
 			tektonclient := fake.NewSimpleClientset()
-			fn := resources.GetStepActionFunc(tektonclient, nil, requester, tr, &tr.Spec.TaskSpec.Steps[0])
+			fn := resources.GetStepActionFunc(tektonclient, nil, requester, tr, *tr.Spec.TaskSpec, &tr.Spec.TaskSpec.Steps[0])
 			if _, _, err := fn(ctx, tr.Spec.TaskSpec.Steps[0].Ref.Name); err == nil {
 				t.Fatalf("expected error due to invalid pipeline data but saw none")
 			}

pkg/reconciler/taskrun/resources/taskspec.go

@@ -107,7 +107,7 @@ func GetStepActionsData(ctx context.Context, taskSpec v1.TaskSpec, taskRun *v1.T
 	for i, step := range taskSpec.Steps {
 		s := step.DeepCopy()
 		if step.Ref != nil {
-			getStepAction := GetStepActionFunc(tekton, k8s, requester, taskRun, s)
+			getStepAction := GetStepActionFunc(tekton, k8s, requester, taskRun, taskSpec, s)
 			stepAction, source, err := getStepAction(ctx, s.Ref.Name)
 			if err != nil {
 				return nil, err

test/taskrun_test.go

@@ -604,3 +604,101 @@ spec:
 		t.Fatalf("expected 1 retry status, got %d", len(taskrun.Status.RetriesStatus))
 	}
 }
+
+func TestTaskRunResolveDefaultParameterSubstitutionOnStepAction(t *testing.T) {
+	ctx := context.Background()
+	ctx, cancel := context.WithCancel(ctx)
+	defer cancel()
+
+	c, namespace := setup(ctx, t, requireAllGates(requireEnableStepActionsGate))
+
+	knativetest.CleanupOnInterrupt(func() { tearDown(ctx, t, c, namespace) }, t.Logf)
+	defer tearDown(ctx, t, c, namespace)
+
+	t.Logf("Creating Task and TaskRun in namespace %s", namespace)
+	task := parse.MustParseV1Task(t, fmt.Sprintf(`
+metadata:
+  name: %s
+  namespace: %s
+spec:
+  params:
+    - name: repository
+      type: string
+      default: https://github.com/tektoncd/catalog.git
+    - name: revision
+      type: string
+      default: main
+  steps:
+    - name: clone
+      ref:
+        resolver: git
+        params:
+        - name: url
+          value: "$(params.repository)"
+        - name: pathInRepo
+          value: /stepaction/git-clone/0.1/git-clone.yaml
+        - name: revision
+          value: "$(params.revision)"
+      params:
+        - name: output-path
+          value: "/tmp"
+        - name: url
+          value: $(params.repository)
+        - name: revision
+          value: $(params.revision)
+`, helpers.ObjectNameForTest(t), namespace))
+	if _, err := c.V1TaskClient.Create(ctx, task, metav1.CreateOptions{}); err != nil {
+		t.Fatalf("Failed to create Task: %s", err)
+	}
+
+	taskRunName := helpers.ObjectNameForTest(t)
+	taskRun := parse.MustParseV1TaskRun(t, fmt.Sprintf(`
+metadata:
+  name: %s
+  namespace: %s
+spec:
+  taskRef:
+    name: %s
+  retries: 1
+`, taskRunName, namespace, task.Name))
+	if _, err := c.V1TaskRunClient.Create(ctx, taskRun, metav1.CreateOptions{}); err != nil {
+		t.Fatalf("Failed to create TaskRun: %s", err)
+	}
+
+	t.Logf("Waiting for TaskRun in namespace %s to complete", namespace)
+	if err := WaitForTaskRunState(ctx, c, taskRunName, TaskRunSucceed(taskRunName), "TaskRunSucceed", v1Version); err != nil {
+		t.Errorf("Error waiting for TaskRun to finish: %s", err)
+	}
+
+	taskrun, err := c.V1TaskRunClient.Get(ctx, taskRunName, metav1.GetOptions{})
+	if err != nil {
+		t.Fatalf("Couldn't get expected TaskRun %s: %s", taskRunName, err)
+	}
+
+	if !isSuccessful(t, taskrun.GetName(), taskrun.Status.Conditions) {
+		t.Fatalf("task should have succeeded")
+	}
+
+	expectedReason := "Succeeded"
+	actualReason := taskrun.Status.GetCondition(apis.ConditionSucceeded).GetReason()
+	if actualReason != expectedReason {
+		t.Fatalf("expected TaskRun to have failed reason %s, got %s", expectedReason, actualReason)
+	}
+
+	expectedStepState := []v1.StepState{{
+		ContainerState: corev1.ContainerState{
+			Terminated: &corev1.ContainerStateTerminated{
+				ExitCode: 0,
+				Reason:   "Completed",
+			},
+		},
+		TerminationReason: "Completed",
+		Name:              "clone",
+		Container:         "step-clone",
+	}}
+	ignoreTerminatedFields := cmpopts.IgnoreFields(corev1.ContainerStateTerminated{}, "StartedAt", "FinishedAt", "ContainerID", "Message")
+	ignoreStepFields := cmpopts.IgnoreFields(v1.StepState{}, "ImageID", "Results", "Provenance")
+	if d := cmp.Diff(taskrun.Status.Steps, expectedStepState, ignoreTerminatedFields, ignoreStepFields); d != "" {
+		t.Fatalf("-got, +want: %v", d)
+	}
+}