diff --git a/examples/v1/taskruns/beta/stepaction-param-references.yaml b/examples/v1/taskruns/beta/stepaction-param-references.yaml new file mode 100644 index 00000000000..cad25b462de --- /dev/null +++ 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" \ No newline at end of file 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..8a42f5d2a8d 100644 --- 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 index e6036faf832..10985846006 100644 --- 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 } diff --git a/pkg/reconciler/taskrun/resources/apply_test.go b/pkg/reconciler/taskrun/resources/apply_test.go index 9b1d9f1be1b..f6e20791bb1 100644 --- a/pkg/reconciler/taskrun/resources/apply_test.go +++ b/pkg/reconciler/taskrun/resources/apply_test.go @@ -18,6 +18,7 @@ package resources_test import ( "context" + "reflect" "testing" "github.com/google/go-cmp/cmp" @@ -2049,3 +2050,307 @@ func TestArtifacts(t *testing.T) { t.Errorf("ApplyArtifacts() got diff %s", diff.PrintWantGot(d)) } } + +func TestApplyStepActionParameters(t *testing.T) { + tests := []struct { + name string + step *v1.Step + taskSpec *v1.TaskSpec + taskRun *v1.TaskRun + stepParams v1.Params + defaultParams []v1.ParamSpec + want *v1.Step + wantErr bool + }{ + { + name: "default value references another parameter", + step: &v1.Step{ + Name: "test-step", + Image: "test-image", + Script: `echo "param1: $(params.param1)" +echo "param2: $(params.param2)"`, + }, + taskSpec: &v1.TaskSpec{}, + taskRun: &v1.TaskRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-taskrun", + Namespace: "test-ns", + }, + }, + stepParams: v1.Params{{ + Name: "param1", + Value: *v1.NewStructuredValues("value-1"), + }}, + defaultParams: []v1.ParamSpec{ + { + Name: "param1", + Type: v1.ParamTypeString, + }, + { + Name: "param2", + Type: v1.ParamTypeString, + Default: v1.NewStructuredValues("$(params.param1)"), + }, + }, + want: &v1.Step{ + Name: "test-step", + Image: "test-image", + Script: `echo "param1: value-1" +echo "param2: value-1"`, + }, + wantErr: false, + }, + { + name: "override default value that references another parameter", + step: &v1.Step{ + Name: "test-step", + Image: "test-image", + Script: `echo "param1: $(params.param1)" +echo "param2: $(params.param2)"`, + }, + taskSpec: &v1.TaskSpec{}, + taskRun: &v1.TaskRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-taskrun", + Namespace: "test-ns", + }, + }, + stepParams: v1.Params{ + { + Name: "param1", + Value: *v1.NewStructuredValues("value-1"), + }, + { + Name: "param2", + Value: *v1.NewStructuredValues("override-value"), + }, + }, + defaultParams: []v1.ParamSpec{ + { + Name: "param1", + Type: v1.ParamTypeString, + }, + { + Name: "param2", + Type: v1.ParamTypeString, + Default: v1.NewStructuredValues("$(params.param1)"), + }, + }, + want: &v1.Step{ + Name: "test-step", + Image: "test-image", + Script: `echo "param1: value-1" +echo "param2: override-value"`, + }, + wantErr: false, + }, + { + name: "reference undefined parameter in default value", + step: &v1.Step{ + Name: "test-step", + Image: "test-image", + }, + taskSpec: &v1.TaskSpec{}, + taskRun: &v1.TaskRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-taskrun", + Namespace: "test-ns", + }, + }, + stepParams: v1.Params{}, + defaultParams: []v1.ParamSpec{ + { + Name: "param2", + Type: v1.ParamTypeString, + Default: v1.NewStructuredValues("$(params.param1)"), + }, + { + Name: "param1", + Type: v1.ParamTypeString, + }, + }, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := applyStepActionParameters(tt.step, tt.taskSpec, tt.taskRun, tt.stepParams, tt.defaultParams) + if (err != nil) != tt.wantErr { + t.Errorf("applyStepActionParameters() error = %v, wantErr %v", err, tt.wantErr) + return + } + if !tt.wantErr && !reflect.DeepEqual(got, tt.want) { + t.Errorf("applyStepActionParameters() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestApplyStepActionStepResults(t *testing.T) { + tests := []struct { + name string + step *v1.Step + taskSpec *v1.TaskSpec + taskRun *v1.TaskRun + stepParams v1.Params + defaultParams []v1.ParamSpec + want *v1.Step + wantErr bool + }{ + { + name: "string parameter referencing step result", + step: &v1.Step{ + Name: "test-step", + Image: "test-image", + Script: `echo "param1: $(params.param1)" +echo "param2: $(params.param2)"`, + }, + taskSpec: &v1.TaskSpec{}, + taskRun: &v1.TaskRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-taskrun", + Namespace: "test-ns", + }, + }, + stepParams: v1.Params{ + { + Name: "param1", + Value: *v1.NewStructuredValues("value-1"), + }, + { + Name: "param2", + Value: *v1.NewStructuredValues("$(steps.previous-step.results.result1)"), + }, + }, + defaultParams: []v1.ParamSpec{ + { + Name: "param1", + Type: v1.ParamTypeString, + }, + { + Name: "param2", + Type: v1.ParamTypeString, + }, + }, + want: &v1.Step{ + Name: "test-step", + Image: "test-image", + Script: `echo "param1: value-1" +echo "param2: $(steps.previous-step.results.result1)"`, + }, + wantErr: false, + }, + { + name: "array parameter referencing step result", + step: &v1.Step{ + Name: "test-step", + Image: "test-image", + Script: `echo "array param: $(params.array-param[*])" +echo "array param index: $(params.array-param[0])"`, + }, + taskSpec: &v1.TaskSpec{}, + taskRun: &v1.TaskRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-taskrun", + Namespace: "test-ns", + }, + }, + stepParams: v1.Params{ + { + Name: "array-param", + Value: *v1.NewStructuredValues("$(steps.previous-step.results.array-result)"), + }, + }, + defaultParams: []v1.ParamSpec{ + { + Name: "array-param", + Type: v1.ParamTypeArray, + }, + }, + want: &v1.Step{ + Name: "test-step", + Image: "test-image", + Script: `echo "array param: $(steps.previous-step.results.array-result[*])" +echo "array param index: $(steps.previous-step.results.array-result[0])"`, + }, + wantErr: false, + }, + { + name: "object parameter referencing step result", + step: &v1.Step{ + Name: "test-step", + Image: "test-image", + Script: `echo "object param: $(params.object-param.key1)"`, + }, + taskSpec: &v1.TaskSpec{}, + taskRun: &v1.TaskRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-taskrun", + Namespace: "test-ns", + }, + }, + stepParams: v1.Params{ + { + Name: "object-param", + Value: *v1.NewStructuredValues("$(steps.previous-step.results.object-result)"), + }, + }, + defaultParams: []v1.ParamSpec{ + { + Name: "object-param", + Type: v1.ParamTypeObject, + Properties: map[string]v1.PropertySpec{ + "key1": {Type: "string"}, + }, + }, + }, + want: &v1.Step{ + Name: "test-step", + Image: "test-image", + Script: `echo "object param: $(steps.previous-step.results.object-result.key1)"`, + }, + wantErr: false, + }, + { + name: "invalid step result reference", + step: &v1.Step{ + Name: "test-step", + Image: "test-image", + }, + taskSpec: &v1.TaskSpec{}, + taskRun: &v1.TaskRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-taskrun", + Namespace: "test-ns", + }, + }, + stepParams: v1.Params{ + { + Name: "param1", + Value: *v1.NewStructuredValues("$(steps.invalid.results)"), // Missing result name + }, + }, + defaultParams: []v1.ParamSpec{ + { + Name: "param1", + Type: v1.ParamTypeString, + }, + }, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := applyStepActionParameters(tt.step, tt.taskSpec, tt.taskRun, tt.stepParams, tt.defaultParams) + if (err != nil) != tt.wantErr { + t.Errorf("applyStepActionParameters() error = %v, wantErr %v", err, tt.wantErr) + return + } + if !tt.wantErr && !reflect.DeepEqual(got, tt.want) { + t.Errorf("applyStepActionParameters() = %v, want %v", got, tt.want) + } + }) + } +}