Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix ValidateStepResultsVariables to validate stepResults only #8264

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+85 −36
Open

fix ValidateStepResultsVariables to validate stepResults only #8264

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions examples/v1/taskruns/alpha/task-stepaction-results.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
apiVersion: tekton.dev/v1alpha1
kind: StepAction
metadata:
name: step-action
spec:
params:
- name: message
type: string
env:
- name: message
value: $(params.message)
image: mirror.gcr.io/bash
script: |
#!/usr/bin/env bash
echo ${message}
---
apiVersion: tekton.dev/v1
kind: TaskRun
metadata:
generateName: reference-step-result-in-step-
spec:
taskSpec:
description: |
A simple task to demonstrate how to reference a step result in another step when used alongside with task result
results:
- name: resultsDir
type: string
steps:
- name: collect
image: mirror.gcr.io/bash
results:
- name: message
type: string
script: |
#!/usr/bin/env sh
set -x
message="scott"

echo -n "${message}" > $(step.results.message.path)
echo -n "tom" > $(results.resultsDir.path)
- name: reduce
params:
- name: message
value: $(steps.collect.results.message)
ref:
name: step-action
24 changes: 19 additions & 5 deletions pkg/apis/pipeline/v1/task_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ func (ts *TaskSpec) Validate(ctx context.Context) (errs *apis.FieldError) {
})
}

errs = errs.Also(validateSteps(ctx, mergedSteps).ViaField("steps"))
errs = errs.Also(validateSteps(ctx, mergedSteps, ts.Results).ViaField("steps"))
errs = errs.Also(validateSidecars(ts.Sidecars).ViaField("sidecars"))
errs = errs.Also(ValidateParameterTypes(ctx, ts.Params).ViaField("params"))
errs = errs.Also(ValidateParameterVariables(ctx, ts.Steps, ts.Params))
Expand Down Expand Up @@ -251,13 +251,13 @@ func ValidateVolumes(volumes []corev1.Volume) (errs *apis.FieldError) {
return errs
}

func validateSteps(ctx context.Context, steps []Step) (errs *apis.FieldError) {
func validateSteps(ctx context.Context, steps []Step, taskResults []TaskResult) (errs *apis.FieldError) {
// Task must not have duplicate step names.
names := sets.NewString()
for idx, s := range steps {
errs = errs.Also(validateStep(ctx, s, names).ViaIndex(idx))
if s.Results != nil {
errs = errs.Also(ValidateStepResultsVariables(ctx, s.Results, s.Script).ViaIndex(idx))
errs = errs.Also(ValidateStepResultsVariables(ctx, s.Results, taskResults, s.Script).ViaIndex(idx))
errs = errs.Also(ValidateStepResults(ctx, s.Results).ViaIndex(idx).ViaField("results"))
}
if len(s.When) > 0 {
Expand Down Expand Up @@ -853,12 +853,26 @@ func ValidateStepResults(ctx context.Context, results []StepResult) (errs *apis.
}

// ValidateStepResultsVariables validates if the StepResults referenced in step script are defined in step's results.
func ValidateStepResultsVariables(ctx context.Context, results []StepResult, script string) (errs *apis.FieldError) {
func ValidateStepResultsVariables(ctx context.Context, results []StepResult, taskResults []TaskResult, script string) (errs *apis.FieldError) {
resultsNames := sets.NewString()
taskResultsNames := sets.NewString()
for _, r := range results {
resultsNames.Insert(r.Name)
}
for _, r := range taskResults {
taskResultsNames.Insert(r.Name)
}
errs = errs.Also(substitution.ValidateNoReferencesToUnknownVariables(script, "step.results", resultsNames).ViaField("script"))
errs = errs.Also(substitution.ValidateNoReferencesToUnknownVariables(script, "results", taskResultsNames).ViaField("script"))
return errs
}

// ValidateStepActionResultsVariables validates if the StepResults referenced in step script are defined in step's results.
func ValidateStepActionResultsVariables(ctx context.Context, results []StepResult, script string) (errs *apis.FieldError) {
resultsNames := sets.NewString()
for _, r := range results {
resultsNames.Insert(r.Name)
}
errs = errs.Also(substitution.ValidateNoReferencesToUnknownVariables(script, "step.results", resultsNames).ViaField("script"))
errs = errs.Also(substitution.ValidateNoReferencesToUnknownVariables(script, "results", resultsNames).ViaField("script"))
return errs
}
2 changes: 1 addition & 1 deletion pkg/apis/pipeline/v1alpha1/stepaction_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ func (ss *StepActionSpec) Validate(ctx context.Context) (errs *apis.FieldError)
errs = errs.Also(validateUsageOfDeclaredParameters(ctx, *ss))
errs = errs.Also(v1.ValidateParameterTypes(ctx, ss.Params).ViaField("params"))
errs = errs.Also(validateParameterVariables(ctx, *ss, ss.Params))
errs = errs.Also(v1.ValidateStepResultsVariables(ctx, ss.Results, ss.Script))
errs = errs.Also(v1.ValidateStepActionResultsVariables(ctx, ss.Results, ss.Script))
errs = errs.Also(v1.ValidateStepResults(ctx, ss.Results).ViaField("results"))
errs = errs.Also(validateVolumeMounts(ss.VolumeMounts, ss.Params).ViaField("volumeMounts"))
return errs
Expand Down
13 changes: 0 additions & 13 deletions pkg/apis/pipeline/v1alpha1/stepaction_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -613,19 +613,6 @@ func TestStepActionSpecValidateError(t *testing.T) {
Message: `windows script support requires "enable-api-fields" feature gate to be "alpha" but it is "beta"`,
Paths: []string{},
},
}, {
name: "step script refers to nonexistent result",
fields: fields{
Image: "my-image",
Script: `
#!/usr/bin/env bash
date | tee $(results.non-exist.path)`,
Results: []v1.StepResult{{Name: "a-result"}},
},
expectedError: apis.FieldError{
Message: `non-existent variable in "\n\t\t\t#!/usr/bin/env bash\n\t\t\tdate | tee $(results.non-exist.path)"`,
Paths: []string{"script"},
},
Comment on lines -616 to -628
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact that you have to remove this test here highlights another problem in the current codebase.
You want to fix an issue in how we validate an inline step in a Task definition, and this has a side effect on the validation of a StepAction, which should not happen.

In the task-stepaction-results.yaml example that you provided, an inline step references to a Task result, which is valid and should work, and doesn't work today, which is the problem that needs to be fixed.
This stems from the fact that ValidateStepResultsVariables does not have enough context to validate Task level references, which are validated instead by ValidateTaskResultsVariables as you mentioned.

The change you proposed, however, modifies code used for validation of StepActions, which is not ok, as exemplified by the test you removed, which should continue to pass. The author of a StepAction has no knowledge about the Tasks that may use it, and should not reference any variable outside of the known context.

A correct fix for the issue should include, on top of the fix already done, a change in the stepaction_validation code, which, instead of using ValidateStepResultsVariables from the task validation, should define its own ValidateStepActionResultsVariables, which should look like what ValidateStepResultsVariables looks today.

Unit tests should not be removed and we should make sure we have a unit tests that validates ValidateTaskResultsVariables (which we most likely already have).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@afrittoli I think you're right to add ValidateStepActionResultsVariables that validates step results only because it has only the step results context and hence i removed the snippet of tests there (we dont have the task result context when using stepAction.Validate)
I am also fixing ValidateStepResultsVariables to add task results in the function args so we can validate the step script against both step and task result because we have them in context as this method is now only called in task validation context

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@afrittoli Can you have a look to my comment pleae

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jkhelil Sorry about the late reply.

The proposed fix is not ok. Quoting my previous comment:

The change you proposed modifies code used for validation of StepActions, which is not ok

The validation of StepActions should remain the same and its tests should also remain the same as they are correct. The problem stems from the fact that the Tasks validation function is also used to validate StepActions and it enforces validation that is only required for StepActions.

A correct fix would be to have two different validation functions:

  • ValidateStepActionResultsVariables, which should look like what ValidateStepResultsVariables looks today (before this PR)
  • ValidateStepResultsVariables, which should be fixed, like you have done in this PR, to enforce validation that is appropriate for Tasks

If you did that, then you would not need to remove any unit tests.

}, {
name: "step script refers to nonexistent stepresult",
fields: fields{
Expand Down
2 changes: 1 addition & 1 deletion pkg/apis/pipeline/v1beta1/stepaction_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ func (ss *StepActionSpec) Validate(ctx context.Context) (errs *apis.FieldError)
errs = errs.Also(validateUsageOfDeclaredParameters(ctx, *ss))
errs = errs.Also(v1.ValidateParameterTypes(ctx, ss.Params).ViaField("params"))
errs = errs.Also(validateParameterVariables(ctx, *ss, ss.Params))
errs = errs.Also(v1.ValidateStepResultsVariables(ctx, ss.Results, ss.Script))
errs = errs.Also(v1.ValidateStepActionResultsVariables(ctx, ss.Results, ss.Script))
errs = errs.Also(v1.ValidateStepResults(ctx, ss.Results).ViaField("results"))
errs = errs.Also(validateVolumeMounts(ss.VolumeMounts, ss.Params).ViaField("volumeMounts"))
return errs
Expand Down
13 changes: 0 additions & 13 deletions pkg/apis/pipeline/v1beta1/stepaction_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -613,19 +613,6 @@ func TestStepActionSpecValidateError(t *testing.T) {
Message: `windows script support requires "enable-api-fields" feature gate to be "alpha" but it is "beta"`,
Paths: []string{},
},
}, {
name: "step script refers to nonexistent result",
fields: fields{
Image: "my-image",
Script: `
#!/usr/bin/env bash
date | tee $(results.non-exist.path)`,
Results: []v1.StepResult{{Name: "a-result"}},
},
expectedError: apis.FieldError{
Message: `non-existent variable in "\n\t\t\t#!/usr/bin/env bash\n\t\t\tdate | tee $(results.non-exist.path)"`,
Paths: []string{"script"},
},
}, {
name: "step script refers to nonexistent stepresult",
fields: fields{
Expand Down
21 changes: 18 additions & 3 deletions pkg/apis/pipeline/v1beta1/task_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ func (ts *TaskSpec) Validate(ctx context.Context) (errs *apis.FieldError) {
})
}

errs = errs.Also(validateSteps(ctx, mergedSteps).ViaField("steps"))
errs = errs.Also(validateSteps(ctx, mergedSteps, ts.Results).ViaField("steps"))
errs = errs.Also(validateSidecarNames(ts.Sidecars))
errs = errs.Also(ValidateParameterTypes(ctx, ts.Params).ViaField("params"))
errs = errs.Also(ValidateParameterVariables(ctx, ts.Steps, ts.Params))
Expand Down Expand Up @@ -240,13 +240,13 @@ func ValidateVolumes(volumes []corev1.Volume) (errs *apis.FieldError) {
return errs
}

func validateSteps(ctx context.Context, steps []Step) (errs *apis.FieldError) {
func validateSteps(ctx context.Context, steps []Step, taskResults []TaskResult) (errs *apis.FieldError) {
// Task must not have duplicate step names.
names := sets.NewString()
for idx, s := range steps {
errs = errs.Also(validateStep(ctx, s, names).ViaIndex(idx))
if s.Results != nil {
errs = errs.Also(v1.ValidateStepResultsVariables(ctx, s.Results, s.Script).ViaIndex(idx))
errs = errs.Also(ValidateStepResultsVariables(ctx, s.Results, taskResults, s.Script).ViaIndex(idx))
errs = errs.Also(v1.ValidateStepResults(ctx, s.Results).ViaIndex(idx).ViaField("results"))
}
if len(s.When) > 0 {
Expand All @@ -256,6 +256,21 @@ func validateSteps(ctx context.Context, steps []Step) (errs *apis.FieldError) {
return errs
}

// ValidateStepResultsVariables validates if the StepResults referenced in step script are defined in step's results or in task's results
func ValidateStepResultsVariables(ctx context.Context, results []v1.StepResult, taskResults []TaskResult, script string) (errs *apis.FieldError) {
resultsNames := sets.NewString()
taskResultsNames := sets.NewString()
for _, r := range results {
resultsNames.Insert(r.Name)
}
for _, r := range taskResults {
taskResultsNames.Insert(r.Name)
}
errs = errs.Also(substitution.ValidateNoReferencesToUnknownVariables(script, "step.results", resultsNames).ViaField("script"))
errs = errs.Also(substitution.ValidateNoReferencesToUnknownVariables(script, "results", taskResultsNames).ViaField("script"))
return errs
}

// isCreateOrUpdateAndDiverged checks if the webhook event was create or update
// if create, it returns true.
// if update, it checks if the step results have diverged and returns if diverged.
Expand Down