fix stepaction param default value referencing other param

- multipass default value substitution: one for non referenced default values, another one for parameter references in default values - add validation to ensure parameters referenced in default values are defined before use - substitute default values of parameters with referenced parameter values if necessary - update replacementsFromStepResults to only process step result references - add comments which highlight order of hydration of parameters - add tests for replacementsFromStepResults, validateDefaultParameterReferences - add example for parameter references in default values of step action parameters Signed-off-by: Vibhav Bobade <[email protected]>
tektoncd · Jan 22, 2025 · ba4a742 · ba4a742
1 parent 0d698a7
commit ba4a742
Show file tree

Hide file tree

Showing 5 changed files with 542 additions and 23 deletions.
diff --git a/examples/v1/taskruns/beta/stepaction-param-references.yaml b/examples/v1/taskruns/beta/stepaction-param-references.yaml
@@ -0,0 +1,72 @@
+apiVersion: tekton.dev/v1beta1
+kind: StepAction
+metadata:
+  name: step-action
+spec:
+  params:
+    - name: message
+      default: "hello"
+    - name: greeting
+      default: "$(params.message) world"
+    - name: values
+      type: array
+      default:
+        - "first"
+        - "second"
+        - "third"
+    - name: config
+      properties:
+        key1:
+          type: string
+        key2:
+          type: string
+      default:
+        key1: "value1"
+        key2: "value2"
+  image: mirror.gcr.io/ubuntu
+  env:
+    - name: MESSAGE
+      value: $(params.message)
+    - name: GREETING
+      value: $(params.greeting)
+    - name: VALUE_1
+      value: $(params.values[0])
+    - name: VALUE_2
+      value: $(params.values[1])
+    - name: VALUE_3
+      value: $(params.values[2])
+    - name: CONFIG_KEY1
+      value: $(params.config.key1)
+    - name: CONFIG_KEY2
+      value: $(params.config.key2)
+  script: |
+    #!/usr/bin/env bash
+    echo "Message: ${MESSAGE}"
+    echo "Greeting: ${GREETING}"
+    echo "Values: ${VALUE_1}, ${VALUE_2}, ${VALUE_3}"
+    echo "Config key1: ${CONFIG_KEY1}"
+    echo "Config key2: ${CONFIG_KEY2}"
+---
+apiVersion: tekton.dev/v1
+kind: TaskRun
+metadata:
+  generateName: param-reference-example-
+spec:
+  taskSpec:
+    steps:
+      - name: show-params
+        ref:
+          name: step-action
+        params:
+          - name: message
+            value: "hi"
+          # greeting will use default with substituted message: "hi world"
+          - name: values
+            value:
+              - "one"
+              - "two"
+              - "three"
+          - name: config
+            value:
+              key1: "custom1"
+              key2: "custom2" 
diff --git a/pkg/apis/pipeline/v1alpha1/stepaction_validation.go b/pkg/apis/pipeline/v1alpha1/stepaction_validation.go
@@ -15,6 +15,7 @@ package v1alpha1
 
 import (
 	"context"
+	"fmt"
 	"strings"
 
 	"github.com/tektoncd/pipeline/pkg/apis/config"
@@ -128,7 +129,33 @@ func validateParameterVariables(ctx context.Context, sas StepActionSpec, params
 	stringParameterNames := sets.NewString(stringParams.GetNames()...)
 	arrayParameterNames := sets.NewString(arrayParams.GetNames()...)
 	errs = errs.Also(v1.ValidateNameFormat(stringParameterNames.Insert(arrayParameterNames.List()...), objectParams))
-	return errs.Also(validateStepActionArrayUsage(sas, "params", arrayParameterNames))
+	errs = errs.Also(validateStepActionArrayUsage(sas, "params", arrayParameterNames))
+	return errs.Also(validateDefaultParameterReferences(params))
+}
+
+// validateDefaultParameterReferences ensures that parameters referenced in default values are defined
+func validateDefaultParameterReferences(params v1.ParamSpecs) *apis.FieldError {
+	var errs *apis.FieldError
+	allParams := sets.NewString(params.GetNames()...)
+
+	for _, p := range params {
+		if p.Default != nil {
+			// check if default value references any parameters
+			matches, _ := substitution.ExtractVariableExpressions(p.Default.StringVal, "params")
+			// validate each referenced parameter exists
+			for _, match := range matches {
+				paramName := strings.TrimSuffix(strings.TrimPrefix(match, "$(params."), ")")
+				if !allParams.Has(paramName) {
+					errs = errs.Also(&apis.FieldError{
+						Message: fmt.Sprintf("param %q default value references param %q which is not defined", p.Name, paramName),
+						Paths:   []string{"params"},
+					})
+				}
+			}
+		}
+	}
+
+	return errs
 }
 
 // validateObjectUsage validates the usage of individual attributes of an object param and the usage of the entire object

diff --git a/pkg/apis/pipeline/v1alpha1/stepaction_validation_test.go b/pkg/apis/pipeline/v1alpha1/stepaction_validation_test.go
@@ -592,6 +592,20 @@ func TestStepActionSpecValidateError(t *testing.T) {
 			Message: `missing field(s)`,
 			Paths:   []string{"Image"},
 		},
+	}, {
+		name: "param default value references undefined param",
+		fields: fields{
+			Image: "myimage",
+			Params: []v1.ParamSpec{{
+				Name:    "param2",
+				Type:    v1.ParamTypeString,
+				Default: v1.NewStructuredValues("$(params.param1)"),
+			}},
+		},
+		expectedError: apis.FieldError{
+			Message: `param "param2" default value references param "param1" which is not defined`,
+			Paths:   []string{"params"},
+		},
 	}, {
 		name: "command and script both used.",
 		fields: fields{

diff --git a/pkg/reconciler/taskrun/resources/apply.go b/pkg/reconciler/taskrun/resources/apply.go
@@ -67,37 +67,103 @@ var (
 )
 
 // applyStepActionParameters applies the params from the Task and the underlying Step to the referenced StepAction.
+// substitution order:
+// 1. taskrun parameter values
+// 2. step-provided parameter values
+// 3. default values that reference other parameters
+// 4. simple default values
+// 5. step result references
 func applyStepActionParameters(step *v1.Step, spec *v1.TaskSpec, tr *v1.TaskRun, stepParams v1.Params, defaults []v1.ParamSpec) (*v1.Step, error) {
+	// 1. taskrun parameter substitutions to step parameters
 	if stepParams != nil {
 		stringR, arrayR, objectR := getTaskParameters(spec, tr, spec.Params...)
 		stepParams = stepParams.ReplaceVariables(stringR, arrayR, objectR)
 	}
-	// Set params from StepAction defaults
-	stringReplacements, arrayReplacements, _ := replacementsFromDefaultParams(defaults)
-
-	// Set and overwrite params with the ones from the Step
-	stepStrings, stepArrays, _ := replacementsFromParams(stepParams)
-	for k, v := range stepStrings {
+	// 2. step provided parameters
+	stepProvidedParams := make(map[string]v1.ParamValue)
+	for _, sp := range stepParams {
+		stepProvidedParams[sp.Name] = sp.Value
+	}
+	// 3,4. get replacements from default params (both referenced and simple)
+	stringReplacements, arrayReplacements, objectReplacements := replacementsFromDefaultParams(defaults)
+	// process parameter values in order of precedence (2,3,4)
+	processedParams := make([]v1.Param, 0, len(defaults))
+	for _, p := range defaults {
+		// 2. if parameter is provided by the step, use that value
+		if value, exists := stepProvidedParams[p.Name]; exists {
+			processedParams = append(processedParams, v1.Param{
+				Name:  p.Name,
+				Value: value,
+			})
+			continue
+		}
+		// 3,4. if parameter has a default value, process it
+		if p.Default != nil {
+			defaultValue := *p.Default
+			// 3. if default value references other parameters, substitute them
+			if strings.Contains(defaultValue.StringVal, "$(params.") {
+				matches, _ := substitution.ExtractVariableExpressions(defaultValue.StringVal, "params")
+				resolvedValue := defaultValue.StringVal
+
+				for _, match := range matches {
+					paramName := strings.TrimSuffix(strings.TrimPrefix(match, "$(params."), ")")
+
+					for _, processed := range processedParams {
+						if processed.Name == paramName {
+							resolvedValue = strings.ReplaceAll(resolvedValue, match, processed.Value.StringVal)
+							break
+						}
+					}
+				}
+				defaultValue.StringVal = resolvedValue
+			}
+			// 4. apply simple default value
+			processedParams = append(processedParams, v1.Param{
+				Name:  p.Name,
+				Value: defaultValue,
+			})
+		}
+	}
+	// apply the processed parameters (2,3,4)
+	procStringReplacements, procArrayReplacements, procObjectReplacements := replacementsFromParams(processedParams)
+	// merge replacements from defaults and processed params
+	for k, v := range procStringReplacements {
 		stringReplacements[k] = v
 	}
-	for k, v := range stepArrays {
+	for k, v := range procArrayReplacements {
 		arrayReplacements[k] = v
 	}
-
-	stepResultReplacements, _ := replacementsFromStepResults(step, stepParams, defaults)
-	for k, v := range stepResultReplacements {
-		stringReplacements[k] = v
+	for k, v := range procObjectReplacements {
+		if objectReplacements[k] == nil {
+			objectReplacements[k] = v
+		} else {
+			for key, val := range v {
+				objectReplacements[k][key] = val
+			}
+		}
 	}
 
 	// Check if there are duplicate keys in the replacements
 	// If the same key is present in both stringReplacements and arrayReplacements, it means
 	// that the default value and the passed value have different types.
-	err := checkForDuplicateKeys(stringReplacements, arrayReplacements)
+	if err := checkForDuplicateKeys(stringReplacements, arrayReplacements); err != nil {
+		return nil, err
+	}
+
+	// 5. handle step result references in parameters last
+	stepResultReplacements, err := replacementsFromStepResults(step, stepParams, defaults)
 	if err != nil {
 		return nil, err
 	}
 
+	// 5. merge step result replacements into string replacements last
+	for k, v := range stepResultReplacements {
+		stringReplacements[k] = v
+	}
+
+	// apply all replacements at once
 	container.ApplyStepReplacements(step, stringReplacements, arrayReplacements)
+
 	return step, nil
 }
 
@@ -165,8 +231,9 @@ func replacementsArrayIdxStepResults(step *v1.Step, paramName string, stepName s
 func replacementsFromStepResults(step *v1.Step, stepParams v1.Params, defaults []v1.ParamSpec) (map[string]string, error) {
 	stringReplacements := map[string]string{}
 	for _, sp := range stepParams {
-		if sp.Value.StringVal != "" {
-			//  $(params.p1) --> $(steps.step1.results.foo) (normal substitution)
+		if sp.Value.StringVal != "" && strings.HasPrefix(sp.Value.StringVal, "$(steps.") {
+			// eg: when parameter p1 references a step result, replace:
+			// $(params.p1) with $(steps.step1.results.foo)
 			value := strings.TrimSuffix(strings.TrimPrefix(sp.Value.StringVal, "$("), ")")
 			pr, err := resultref.ParseStepExpression(value)
 			if err != nil {
@@ -180,19 +247,26 @@ func replacementsFromStepResults(step *v1.Step, stepParams v1.Params, defaults [
 							stringReplacements[fmt.Sprintf("params.%s.%s", d.Name, k)] = fmt.Sprintf("$(steps.%s.results.%s.%s)", pr.ResourceName, pr.ResultName, k)
 						}
 					case v1.ParamTypeArray:
-						//  $(params.p1[*]) --> $(steps.step1.results.foo)
+						// for array parameters
+
+						// with star notation, replace:
+						// $(params.p1[*]) with $(steps.step1.results.foo[*])
 						for _, pattern := range paramPatterns {
 							stringReplacements[fmt.Sprintf(pattern+"[*]", d.Name)] = fmt.Sprintf("$(steps.%s.results.%s[*])", pr.ResourceName, pr.ResultName)
 						}
-						//  $(params.p1[idx]) --> $(steps.step1.results.foo[idx])
+						// with index notation, replace:
+						// $(params.p1[idx]) with $(steps.step1.results.foo[idx])
 						for k, v := range replacementsArrayIdxStepResults(step, d.Name, pr.ResourceName, pr.ResultName) {
 							stringReplacements[k] = v
 						}
-					// This is handled by normal param substitution.
-					// $(params.p1.key) --> $(steps.step1.results.foo)
-					case v1.ParamTypeString:
-					// Since String is the default, This is handled by normal param substitution.
 					default:
+						// for string parameters and default case,
+						// replace any reference to the parameter with the step result reference
+						// since both use simple value substitution
+						// eg: replace $(params.p1) with $(steps.step1.results.foo)
+						for _, pattern := range paramPatterns {
+							stringReplacements[fmt.Sprintf(pattern, d.Name)] = sp.Value.StringVal
+						}
 					}
 				}
 			}
@@ -226,6 +300,7 @@ func getTaskParameters(spec *v1.TaskSpec, tr *v1.TaskRun, defaults ...v1.ParamSp
 			}
 		}
 	}
+
 	return stringReplacements, arrayReplacements, objectReplacements
 }
 
@@ -240,9 +315,9 @@ func replacementsFromDefaultParams(defaults v1.ParamSpecs) (map[string]string, m
 	arrayReplacements := map[string][]string{}
 	objectReplacements := map[string]map[string]string{}
 
-	// Set all the default stringReplacements
+	// First pass: collect all non-reference default values
 	for _, p := range defaults {
-		if p.Default != nil {
+		if p.Default != nil && !strings.Contains(p.Default.StringVal, "$(params.") {
 			switch p.Default.Type {
 			case v1.ParamTypeArray:
 				for _, pattern := range paramPatterns {
@@ -267,6 +342,32 @@ func replacementsFromDefaultParams(defaults v1.ParamSpecs) (map[string]string, m
 			}
 		}
 	}
+
+	// Second pass: handle parameter references in default values
+	for _, p := range defaults {
+		if p.Default != nil && strings.Contains(p.Default.StringVal, "$(params.") {
+			// extract referenced parameter name
+			matches, _ := substitution.ExtractVariableExpressions(p.Default.StringVal, "params")
+			for _, match := range matches {
+				paramName := strings.TrimPrefix(match, "$(params.")
+				paramName = strings.TrimSuffix(paramName, ")")
+
+				// find referenced parameter value
+				for _, pattern := range paramPatterns {
+					key := fmt.Sprintf(pattern, paramName)
+					if value, exists := stringReplacements[key]; exists {
+						// Apply the value to this parameter's default
+						resolvedValue := strings.ReplaceAll(p.Default.StringVal, match, value)
+						for _, pattern := range paramPatterns {
+							stringReplacements[fmt.Sprintf(pattern, p.Name)] = resolvedValue
+						}
+						break
+					}
+				}
+			}
+		}
+	}
+
 	return stringReplacements, arrayReplacements, objectReplacements
 }