fix: reference params in default values, allow chained references

- parameter defaults can now reference other parameters in chain - validate if the default parameter references are correct - ensure that no referenced parameter goes unresolved, including the chains - ensure that there are no circular dependencies with param referencing - multipass replacement from default params to ensure both referenced and non reference default values are processed - add tests Signed-off-by: Vibhav Bobade <[email protected]>
tektoncd · Jan 28, 2025 · 46a7adc · 46a7adc
1 parent cef86d1
commit 46a7adc
Show file tree

Hide file tree

Showing 4 changed files with 336 additions and 14 deletions.
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,52 @@ 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: "valid param default value references defined param",
+		fields: fields{
+			Image: "myimage",
+			Params: []v1.ParamSpec{{
+				Name:    "param1",
+				Type:    v1.ParamTypeString,
+				Default: v1.NewStructuredValues("hello"),
+			}, {
+				Name:    "param2",
+				Type:    v1.ParamTypeString,
+				Default: v1.NewStructuredValues("$(params.param1) world"),
+			}},
+		},
+	}, {
+		name: "multiple param references in default value",
+		fields: fields{
+			Image: "myimage",
+			Params: []v1.ParamSpec{{
+				Name:    "param1",
+				Type:    v1.ParamTypeString,
+				Default: v1.NewStructuredValues("hello"),
+			}, {
+				Name:    "param2",
+				Type:    v1.ParamTypeString,
+				Default: v1.NewStructuredValues("hi"),
+			}, {
+				Name:    "param3",
+				Type:    v1.ParamTypeString,
+				Default: v1.NewStructuredValues("$(params.param1) $(params.param2)"),
+			}},
+		},
 	}, {
 		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,33 +67,149 @@ var (
 )
 
 // applyStepActionParameters applies the params from the task and the underlying step to the referenced stepaction.
-// substitution order:
+// Parameter substitution order:
 // 1. taskrun parameter values in step parameters
-// 2. set params from StepAction defaults
-// 3. set and overwrite params with the ones from the step
-// 4. set step result replacements last
+// 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)
 	}
-	// 2. set params from stepaction defaults
-	stringReplacements, arrayReplacements, _ := replacementsFromDefaultParams(defaults)
 
-	// 3. 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 substitution (2,3,4)
+	processedParams := make([]v1.Param, 0, len(defaults))
+	// keep track of parameters that need resolution and their references
+	paramsNeedingResolution := make(map[string]bool)
+	paramReferenceMap := make(map[string][]string) // maps param name to names of params it references
+
+	// collect parameter references and handle parameters without references
+	for _, p := range defaults {
+		// 2. step provided parameters
+		if value, exists := stepProvidedParams[p.Name]; exists {
+			// parameter provided by step, add it to processed
+			processedParams = append(processedParams, v1.Param{
+				Name:  p.Name,
+				Value: value,
+			})
+			continue
+		}
+
+		// 3. default params
+		if p.Default != nil {
+			if !strings.Contains(p.Default.StringVal, "$(params.") {
+				// parameter has no references, add it to processed
+				processedParams = append(processedParams, v1.Param{
+					Name:  p.Name,
+					Value: *p.Default,
+				})
+				continue
+			}
+
+			// parameter has references to other parameters, track them >:(
+			paramsNeedingResolution[p.Name] = true
+			matches, _ := substitution.ExtractVariableExpressions(p.Default.StringVal, "params")
+			referencedParams := make([]string, 0, len(matches))
+			for _, match := range matches {
+				paramName := strings.TrimSuffix(strings.TrimPrefix(match, "$(params."), ")")
+				referencedParams = append(referencedParams, paramName)
+			}
+			paramReferenceMap[p.Name] = referencedParams
+		}
+	}
+
+	// process parameters until no more can be resolved
+	for len(paramsNeedingResolution) > 0 {
+		paramWasResolved := false
+		for paramName := range paramsNeedingResolution {
+			canResolveParam := true
+			for _, referencedParam := range paramReferenceMap[paramName] {
+				// Check if referenced parameter is processed
+				isReferenceResolved := false
+				for _, pp := range processedParams {
+					if pp.Name == referencedParam {
+						isReferenceResolved = true
+						break
+					}
+				}
+				if !isReferenceResolved {
+					canResolveParam = false
+					break
+				}
+			}
+
+			if canResolveParam {
+				// process this parameter as all its references have been resolved
+				for _, p := range defaults {
+					if p.Name == paramName {
+						defaultValue := *p.Default
+						resolvedValue := defaultValue.StringVal
+						// hydrate parameter references
+						for _, referencedParam := range paramReferenceMap[paramName] {
+							for _, pp := range processedParams {
+								if pp.Name == referencedParam {
+									resolvedValue = strings.ReplaceAll(
+										resolvedValue,
+										fmt.Sprintf("$(params.%s)", referencedParam),
+										pp.Value.StringVal,
+									)
+									break
+								}
+							}
+						}
+						defaultValue.StringVal = resolvedValue
+						processedParams = append(processedParams, v1.Param{
+							Name:  paramName,
+							Value: defaultValue,
+						})
+						delete(paramsNeedingResolution, paramName)
+						paramWasResolved = true
+						break
+					}
+				}
+			}
+		}
+
+		// circular dependency detected
+		if !paramWasResolved {
+			return nil, fmt.Errorf("circular dependency detected in parameter references")
+		}
+	}
+
+	// apply the processed parameters and merge all replacements (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
 	}
+	for k, v := range procObjectReplacements {
+		if objectReplacements[k] == nil {
+			objectReplacements[k] = v
+		} else {
+			for key, val := range v {
+				objectReplacements[k][key] = val
+			}
+		}
+	}
 
-	// 4. set step result replacements last
+	// 5. set step result replacements last
 	if stepResultReplacements, err := replacementsFromStepResults(step, stepParams, defaults); err != nil {
 		return nil, err
 	} else {
+		// merge step result replacements into string replacements last
 		for k, v := range stepResultReplacements {
 			stringReplacements[k] = v
 		}
@@ -107,6 +223,7 @@ func applyStepActionParameters(step *v1.Step, spec *v1.TaskSpec, tr *v1.TaskRun,
 	}
 
 	container.ApplyStepReplacements(step, stringReplacements, arrayReplacements)
+
 	return step, nil
 }
 
@@ -190,6 +307,8 @@ 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:
+						// for array parameters
+
 						// with star notation, replace:
 						// $(params.p1[*]) with $(steps.step1.results.foo[*])
 						for _, pattern := range paramPatterns {
@@ -243,6 +362,7 @@ func getTaskParameters(spec *v1.TaskSpec, tr *v1.TaskRun, defaults ...v1.ParamSp
 			}
 		}
 	}
+
 	return stringReplacements, arrayReplacements, objectReplacements
 }
 
@@ -257,9 +377,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 {
@@ -284,6 +404,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
 }