diff --git a/docs/stepactions.md b/docs/stepactions.md index 120794278be..062a2e39d94 100644 --- a/docs/stepactions.md +++ b/docs/stepactions.md @@ -141,14 +141,15 @@ spec: When applying parameters to a StepAction, the substitutions are applied in the following order: 1. TaskRun parameter values in step parameters -2. Parameters from StepAction defaults -3. Parameters from the step (overwriting any defaults) -4. Step result replacements +2. Step-provided parameter values +3. Default values that reference other parameters +4. Simple default values +5. Step result references This order ensures that: -- Step parameters can reference TaskRun parameters -- StepAction defaults provide fallback values -- Step-specific parameters take precedence over defaults +- TaskRun parameters are available for step parameter substitution +- Step-provided values take precedence over defaults +- Parameter references in defaults are resolved before simple defaults - Step result references are resolved last to allow referencing results from previous steps ### Emitting Results diff --git a/pkg/apis/pipeline/v1alpha1/stepaction_validation.go b/pkg/apis/pipeline/v1alpha1/stepaction_validation.go index f61d137978a..66b8daab06e 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,80 @@ 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()...) + + // First pass: collect all parameters that have no references in their default values + resolvedParams := sets.NewString() + paramsNeedingResolution := make(map[string][]string) + + for _, p := range params { + if p.Default != nil { + matches, _ := substitution.ExtractVariableExpressions(p.Default.StringVal, "params") + if len(matches) == 0 { + resolvedParams.Insert(p.Name) + } else { + // Track which parameters this parameter depends on + referencedParams := make([]string, 0, len(matches)) + hasUndefinedParam := false + for _, match := range matches { + paramName := strings.TrimSuffix(strings.TrimPrefix(match, "$(params."), ")") + if !allParams.Has(paramName) { + hasUndefinedParam = true + 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"}, + }) + } + referencedParams = append(referencedParams, paramName) + } + // Only track dependencies if all referenced parameters exist + if !hasUndefinedParam { + paramsNeedingResolution[p.Name] = referencedParams + } + } + } else { + resolvedParams.Insert(p.Name) + } + } + + // Second pass: iteratively resolve parameters whose dependencies are satisfied + for len(paramsNeedingResolution) > 0 { + paramWasResolved := false + for paramName, referencedParams := range paramsNeedingResolution { + canResolveParam := true + for _, referencedParam := range referencedParams { + if !resolvedParams.Has(referencedParam) { + canResolveParam = false + break + } + } + if canResolveParam { + resolvedParams.Insert(paramName) + delete(paramsNeedingResolution, paramName) + paramWasResolved = true + } + } + if !paramWasResolved { + // If we couldn't resolve any parameters in this iteration, + // we have a circular dependency + for paramName := range paramsNeedingResolution { + errs = errs.Also(&apis.FieldError{ + Message: fmt.Sprintf("param %q default value has a circular dependency", paramName), + Paths: []string{"params"}, + }) + } + break + } + } + + 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..98583cc4917 100644 --- a/pkg/apis/pipeline/v1alpha1/stepaction_validation_test.go +++ b/pkg/apis/pipeline/v1alpha1/stepaction_validation_test.go @@ -80,6 +80,38 @@ func TestStepActionSpecValidate(t *testing.T) { Command: []string{"ls"}, Args: []string{"-lh"}, }, + }, { + 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: "step action with script", fields: fields{ @@ -540,6 +572,69 @@ func TestStepActionValidateError(t *testing.T) { Message: `non-existent variable in "$(params.gitrepo.foo)"`, Paths: []string{"spec.volumeMounts[0]"}, }, + }, { + name: "circular dependency in param default values", + fields: fields{ + Image: "myimage", + Params: []v1.ParamSpec{{ + Name: "param1", + Type: v1.ParamTypeString, + Default: v1.NewStructuredValues("$(params.param2)"), + }, { + Name: "param2", + Type: v1.ParamTypeString, + Default: v1.NewStructuredValues("$(params.param1)"), + }}, + }, + expectedError: *((&apis.FieldError{ + Message: `param "param1" default value has a circular dependency`, + Paths: []string{"spec.params"}, + }).Also(&apis.FieldError{ + Message: `param "param2" default value has a circular dependency`, + Paths: []string{"spec.params"}, + })), + }, { + name: "complex circular dependency in param default values", + fields: fields{ + Image: "myimage", + Params: []v1.ParamSpec{{ + Name: "param1", + Type: v1.ParamTypeString, + Default: v1.NewStructuredValues("$(params.param2)"), + }, { + Name: "param2", + Type: v1.ParamTypeString, + Default: v1.NewStructuredValues("$(params.param3)"), + }, { + Name: "param3", + Type: v1.ParamTypeString, + Default: v1.NewStructuredValues("$(params.param1)"), + }}, + }, + expectedError: *((&apis.FieldError{ + Message: `param "param1" default value has a circular dependency`, + Paths: []string{"spec.params"}, + }).Also(&apis.FieldError{ + Message: `param "param2" default value has a circular dependency`, + Paths: []string{"spec.params"}, + }).Also(&apis.FieldError{ + Message: `param "param3" default value has a circular dependency`, + Paths: []string{"spec.params"}, + })), + }, { + name: "self-referential param default value", + fields: fields{ + Image: "myimage", + Params: []v1.ParamSpec{{ + Name: "param1", + Type: v1.ParamTypeString, + Default: v1.NewStructuredValues("$(params.param1)"), + }}, + }, + expectedError: apis.FieldError{ + Message: `param "param1" default value has a circular dependency`, + Paths: []string{"spec.params"}, + }, }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -562,6 +657,7 @@ func TestStepActionValidateError(t *testing.T) { if err == nil { t.Fatalf("Expected an error, got nothing for %v", sa) } + t.Logf("Actual error: %v", err) if d := cmp.Diff(tt.expectedError.Error(), err.Error(), cmpopts.IgnoreUnexported(apis.FieldError{})); d != "" { t.Errorf("StepActionSpec.Validate() errors diff %s", diff.PrintWantGot(d)) } @@ -592,6 +688,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{ @@ -967,6 +1077,7 @@ func TestStepActionSpecValidateError(t *testing.T) { if err == nil { t.Fatalf("Expected an error, got nothing for %v", sa) } + t.Logf("Actual error: %v", err) if d := cmp.Diff(tt.expectedError.Error(), err.Error(), cmpopts.IgnoreUnexported(apis.FieldError{})); d != "" { t.Errorf("StepActionSpec.Validate() errors diff %s", diff.PrintWantGot(d)) } diff --git a/pkg/reconciler/taskrun/resources/apply.go b/pkg/reconciler/taskrun/resources/apply.go index a14ecaddc19..18174faba46 100644 --- a/pkg/reconciler/taskrun/resources/apply.go +++ b/pkg/reconciler/taskrun/resources/apply.go @@ -18,6 +18,7 @@ package resources import ( "context" + "errors" "fmt" "path/filepath" "regexp" @@ -69,31 +70,168 @@ var ( // applyStepActionParameters applies the params from the task and the underlying step to the referenced stepaction. // 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 + // track unresolved params and their references + unresolvedParams := make(map[string][]string) + + 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 + unresolvedParams[paramName] = append(unresolvedParams[paramName], referencedParam) + 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 + } + } + } + } + + // unresolvable parameters or circular dependencies + if !paramWasResolved { + // check parameter references to a non-existent parameter + for param, unresolvedRefs := range unresolvedParams { + // check referenced parameters in defaults + for _, ref := range unresolvedRefs { + exists := false + for _, p := range defaults { + if p.Name == ref { + exists = true + break + } + } + if !exists { + return nil, fmt.Errorf("parameter %q references non-existent parameter %q", param, ref) + } + } + // parameters exist but can't be resolved hence it's a circular dependency + return nil, errors.New("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 +245,7 @@ func applyStepActionParameters(step *v1.Step, spec *v1.TaskSpec, tr *v1.TaskRun, } container.ApplyStepReplacements(step, stringReplacements, arrayReplacements) + return step, nil } @@ -190,6 +329,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 +384,7 @@ func getTaskParameters(spec *v1.TaskSpec, tr *v1.TaskRun, defaults ...v1.ParamSp } } } + return stringReplacements, arrayReplacements, objectReplacements } @@ -257,9 +399,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 +426,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..962e4355ad2 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) @@ -1492,6 +1595,92 @@ func TestGetStepActionsData_Error(t *testing.T) { stepAction *v1beta1.StepAction expectedError error }{{ + name: "unresolvable parameter reference", + 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: v1.ParamSpecs{{ + Name: "param1", + Type: v1.ParamTypeString, + Default: &v1.ParamValue{ + Type: v1.ParamTypeString, + StringVal: "$(params.nonexistent)", + }, + }}, + }, + }, + expectedError: errors.New(`parameter "param1" references non-existent parameter "nonexistent"`), + }, { + name: "circular dependency in params", + 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: "$(params.param3)", + }, + }, { + Name: "param2", + Type: v1.ParamTypeString, + Default: &v1.ParamValue{ + Type: v1.ParamTypeString, + StringVal: "$(params.param1)", + }, + }, { + Name: "param3", + Type: v1.ParamTypeString, + Default: &v1.ParamValue{ + Type: v1.ParamTypeString, + StringVal: "$(params.param2)", + }, + }}, + }, + }, + expectedError: errors.New("circular dependency detected in parameter references"), + }, { name: "namespace missing error", tr: &v1.TaskRun{ ObjectMeta: metav1.ObjectMeta{