From 46a7adc0dc6d3475d6f9574ee340916ae58a5ec7 Mon Sep 17 00:00:00 2001 From: Vibhav Bobade Date: Tue, 28 Jan 2025 18:12:59 +0530 Subject: [PATCH] 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 --- .../v1alpha1/stepaction_validation.go | 29 ++- .../v1alpha1/stepaction_validation_test.go | 46 +++++ pkg/reconciler/taskrun/resources/apply.go | 172 ++++++++++++++++-- .../taskrun/resources/taskspec_test.go | 103 +++++++++++ 4 files changed, 336 insertions(+), 14 deletions(-) diff --git a/pkg/apis/pipeline/v1alpha1/stepaction_validation.go b/pkg/apis/pipeline/v1alpha1/stepaction_validation.go index f61d137978a..069b4a03607 100644 --- 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 index e1b3c0f3b8d..8a9719c2df7 100644 --- 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 index a14ecaddc19..5d55008a174 100644 --- 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 } diff --git a/pkg/reconciler/taskrun/resources/taskspec_test.go b/pkg/reconciler/taskrun/resources/taskspec_test.go index 56f7473e7b8..a53749bef6b 100644 --- a/pkg/reconciler/taskrun/resources/taskspec_test.go +++ b/pkg/reconciler/taskrun/resources/taskspec_test.go @@ -1470,7 +1470,110 @@ func TestGetStepActionsData(t *testing.T) { Image: "myimage", Args: []string{"$(steps.step1.results.config.key1)", "$(steps.step1.results.config.key2)"}, }}, + }, { + name: "chained parameter references in defaults", + tr: &v1.TaskRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "mytaskrun", + Namespace: "default", + }, + Spec: v1.TaskRunSpec{ + TaskSpec: &v1.TaskSpec{ + Steps: []v1.Step{{ + Ref: &v1.Ref{ + Name: "stepAction", + }, + }}, + }, + }, + }, + stepAction: &v1beta1.StepAction{ + ObjectMeta: metav1.ObjectMeta{ + Name: "stepAction", + Namespace: "default", + }, + Spec: v1beta1.StepActionSpec{ + Image: "myimage", + Args: []string{"$(params.param1)", "$(params.param2)", "$(params.param3)"}, + Params: v1.ParamSpecs{{ + Name: "param1", + Type: v1.ParamTypeString, + Default: &v1.ParamValue{ + Type: v1.ParamTypeString, + StringVal: "hello", + }, + }, { + Name: "param2", + Type: v1.ParamTypeString, + Default: &v1.ParamValue{ + Type: v1.ParamTypeString, + StringVal: "$(params.param1) world", + }, + }, { + Name: "param3", + Type: v1.ParamTypeString, + Default: &v1.ParamValue{ + Type: v1.ParamTypeString, + StringVal: "$(params.param2)!", + }, + }}, + }, + }, + want: []v1.Step{{ + Image: "myimage", + Args: []string{"hello", "hello world", "hello world!"}, + }}, + }, { + name: "parameter substitution with task param reference", + tr: &v1.TaskRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "mytaskrun", + Namespace: "default", + }, + Spec: v1.TaskRunSpec{ + Params: v1.Params{{ + Name: "task-param", + Value: *v1.NewStructuredValues("task-override"), + }}, + TaskSpec: &v1.TaskSpec{ + Params: []v1.ParamSpec{{ + Name: "task-param", + Type: v1.ParamTypeString, + Default: v1.NewStructuredValues("task-default"), + }}, + Steps: []v1.Step{{ + Ref: &v1.Ref{ + Name: "stepAction", + }, + Params: v1.Params{{ + Name: "message", + Value: *v1.NewStructuredValues("override"), + }}, + }}, + }, + }, + }, + stepAction: &v1beta1.StepAction{ + ObjectMeta: metav1.ObjectMeta{ + Name: "stepAction", + Namespace: "default", + }, + Spec: v1beta1.StepActionSpec{ + Image: "myimage", + Args: []string{"$(params.message)"}, + Params: v1.ParamSpecs{{ + Name: "message", + Type: v1.ParamTypeString, + Default: v1.NewStructuredValues("$(params.task-param)"), + }}, + }, + }, + want: []v1.Step{{ + Image: "myimage", + Args: []string{"override"}, + }}, }} + for _, tt := range tests { ctx := context.Background() tektonclient := fake.NewSimpleClientset(tt.stepAction)