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.

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

Validate dependencies between resolved resources in a PipelineRun #3711

Merged
merged 1 commit into from Jan 28, 2021
Merged

Validate dependencies between resolved resources in a PipelineRun #3711

merged 1 commit into from Jan 28, 2021

Conversation

ghost
Copy link

@ghost ghost commented Jan 22, 2021

Changes

Fixes #3499

When a PipelineRun is started there are several error conditions that can be
hit due to invalid Pipeline configuration. These errors have previously
only surfaced halfway through execution of the PipelineRun because validating
them requires a number of different resources to be resolved.

This commit performs validation on resolved dependencies of a PipelineRun before
it is allowed to start:

  • All result variables used in the Pipeline are checked to be pointing at valid
    Tasks and TaskResults. This requires looking at resolved TaskSpecs.
  • Workspaces marked optional by the Pipeline are confirmed to also be Optional
    in the Tasks they're passed to. This also requires looking at resolved TaskSpecs.

This validation required searching for result variables in a PipelineTask in a very
similar way we do in several other places in our codebase. I've refactored all of these
to use a common func called PipelineTaskResultRefs().

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

  • Includes tests (if functionality changed/added)
  • Commit messages follow commit message best practices
  • Release notes block has been filled in or deleted (only if no user facing changes)

Release Notes

Added extra validations before PipelineRun can start: all result variables in the Pipeline must be valid and optional workspaces from a pipeline can only be passed to tasks expecting optional workspaces.

@ghost ghost added the kind/misc Categorizes issue or PR as a miscellaneuous one. label Jan 22, 2021
@tekton-robot tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jan 22, 2021
@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 22, 2021
@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1beta1/pipeline_types.go 74.2% 82.8% 8.6
pkg/reconciler/pipelinerun/pipelinerun.go 83.1% 82.7% -0.4
pkg/reconciler/pipelinerun/resources/resultrefresolution.go 91.6% 76.8% -14.8
pkg/reconciler/pipelinerun/resources/validate_dependencies.go Do not exist 80.0%

@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1beta1/pipeline_types.go 74.2% 82.8% 8.6
pkg/reconciler/pipelinerun/pipelinerun.go 83.1% 81.6% -1.5
pkg/reconciler/pipelinerun/resources/resultrefresolution.go 91.6% 76.8% -14.8
pkg/reconciler/pipelinerun/resources/validate_dependencies.go Do not exist 80.0%

@ghost
Copy link
Author

ghost commented Jan 22, 2021

/hold

There's quite a bit of test coverage I need to add before this goes in.

@tekton-robot tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 22, 2021
@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1beta1/pipeline_types.go 74.2% 82.8% 8.6
pkg/reconciler/pipelinerun/pipelinerun.go 83.1% 81.6% -1.5
pkg/reconciler/pipelinerun/resources/resultrefresolution.go 91.6% 95.1% 3.5
pkg/reconciler/pipelinerun/resources/validate_dependencies.go Do not exist 80.0%

@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1beta1/pipeline_types.go 74.2% 82.8% 8.6
pkg/reconciler/pipelinerun/pipelinerun.go 83.1% 81.6% -1.5
pkg/reconciler/pipelinerun/resources/resultrefresolution.go 91.6% 93.8% 2.2
pkg/reconciler/pipelinerun/resources/validate_dependencies.go Do not exist 80.0%

@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1beta1/pipeline_types.go 74.2% 82.8% 8.6
pkg/reconciler/pipelinerun/pipelinerun.go 83.1% 81.6% -1.5
pkg/reconciler/pipelinerun/resources/resultrefresolution.go 91.6% 93.8% 2.2
pkg/reconciler/pipelinerun/resources/validate_dependencies.go Do not exist 80.0%

@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1beta1/pipeline_types.go 74.2% 82.8% 8.6
pkg/reconciler/pipelinerun/pipelinerun.go 83.1% 81.6% -1.5
pkg/reconciler/pipelinerun/resources/resultrefresolution.go 91.6% 93.8% 2.2
pkg/reconciler/pipelinerun/resources/validate_dependencies.go Do not exist 80.0%

@tekton-robot tekton-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 25, 2021
@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1beta1/pipeline_types.go 74.2% 82.8% 8.6
pkg/reconciler/pipelinerun/pipelinerun.go 83.1% 81.6% -1.5
pkg/reconciler/pipelinerun/resources/resultrefresolution.go 91.6% 93.8% 2.2
pkg/reconciler/pipelinerun/resources/validate_dependencies.go Do not exist 95.0%

@ghost ghost added this to the Pipelines 0.21 milestone Jan 25, 2021
@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1beta1/pipeline_types.go 74.2% 82.8% 8.6
pkg/reconciler/pipelinerun/pipelinerun.go 83.1% 81.6% -1.5
pkg/reconciler/pipelinerun/resources/resultrefresolution.go 91.6% 93.8% 2.2
pkg/reconciler/pipelinerun/resources/validate_dependencies.go Do not exist 100.0%

@ghost
Copy link
Author

ghost commented Jan 25, 2021

/test pull-tekton-pipeline-integration-tests

@ghost
Copy link
Author

ghost commented Jan 25, 2021

/hold cancel

Improved coverage.

@tekton-robot tekton-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jan 25, 2021
@ghost ghost removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 25, 2021
@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1beta1/pipeline_types.go 74.2% 82.8% 8.6
pkg/reconciler/pipelinerun/pipelinerun.go 83.1% 83.5% 0.4
pkg/reconciler/pipelinerun/resources/resultrefresolution.go 91.6% 93.8% 2.2
pkg/reconciler/pipelinerun/resources/validate_dependencies.go Do not exist 100.0%

@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1beta1/pipeline_types.go 74.2% 82.6% 8.4
pkg/reconciler/pipelinerun/pipelinerun.go 83.1% 83.5% 0.4
pkg/reconciler/pipelinerun/resources/resultrefresolution.go 91.6% 93.8% 2.2
pkg/reconciler/pipelinerun/resources/validate_dependencies.go Do not exist 100.0%

@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1beta1/pipeline_types.go 74.2% 82.6% 8.4
pkg/reconciler/pipelinerun/pipelinerun.go 83.1% 83.5% 0.4
pkg/reconciler/pipelinerun/resources/resultrefresolution.go 91.6% 93.8% 2.2
pkg/reconciler/pipelinerun/resources/validate_dependencies.go Do not exist 100.0%

Comment on lines +93 to +115
func ValidateOptionalWorkspaces(pipelineWorkspaces []v1beta1.PipelineWorkspaceDeclaration, state PipelineRunState) error {
optionalWorkspaces := sets.NewString()
for _, ws := range pipelineWorkspaces {
if ws.Optional {
optionalWorkspaces.Insert(ws.Name)
}
}

for _, rprt := range state {
for _, pws := range rprt.PipelineTask.Workspaces {
if optionalWorkspaces.Has(pws.Workspace) {
for _, tws := range rprt.ResolvedTaskResources.TaskSpec.Workspaces {
if tws.Name == pws.Name {
if !tws.Optional {
return fmt.Errorf("pipeline workspace %q is marked optional but pipeline task %q requires it be provided", pws.Workspace, rprt.PipelineTask.Name)
}
}
}
}
}
}
return nil
}
Copy link
Member

Choose a reason for hiding this comment

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

this makes me think that it may be clearer to have the reason above as ReasonRequiredWorkspaceMarkedOptional instead of ReasonOptionalWorkspaceNotSupportedByTask

// ReasonOptionalWorkspaceNotSupportedByTask indicates an optional workspace
// has been passed to a Task that is expecting a non-optional workspace
ReasonOptionalWorkspaceNotSupportedByTask = "OptionalWorkspaceNotSupportedByTask"

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for reviewing, I've made this change.

@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1beta1/pipeline_types.go 74.2% 82.6% 8.4
pkg/reconciler/pipelinerun/pipelinerun.go 83.1% 83.5% 0.4
pkg/reconciler/pipelinerun/resources/resultrefresolution.go 91.6% 93.8% 2.2
pkg/reconciler/pipelinerun/resources/validate_dependencies.go Do not exist 100.0%

When a PipelineRun is started there are several error conditions that can be
hit due to invalid Pipeline configuration. These errors have previously
only surfaced halfway through execution of the PipelineRun because validating
them requires a number of different resources to be resolved.

This commit performs more extensive runtime validation of a PipelineRun before
it is allowed to start:
- All result variables used in the Pipeline are checked to be pointing at valid
Tasks and TaskResults.
-  Workspaces marked optional by the Pipeline are confirmed to also be Optional
in the Tasks they're passed to.

This validation required searching for result variables in a PipelineTask in a very
similar way we do in several other places in our codebase. I've refactored all of these
to use a common func `PipelineTaskResultRefs()`.
@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1beta1/pipeline_types.go 74.2% 82.6% 8.4
pkg/reconciler/pipelinerun/pipelinerun.go 83.1% 83.5% 0.4
pkg/reconciler/pipelinerun/resources/resultrefresolution.go 91.6% 93.8% 2.2
pkg/reconciler/pipelinerun/resources/validate_dependencies.go Do not exist 100.0%

@ghost
Copy link
Author

ghost commented Jan 26, 2021

/test pull-tekton-pipeline-integration-tests

Copy link
Member

@jerop jerop left a comment

Choose a reason for hiding this comment

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

thanks @sbwsg for the added validation and making using result references cleaner!

@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jerop

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 27, 2021
@ghost
Copy link
Author

ghost commented Jan 28, 2021

/test check-pr-has-kind-label

@pritidesai
Copy link
Member

pritidesai commented Jan 28, 2021

Thanks @sbwsg, this PR has a lot of simplifications around results and I appreciate these changes 👍

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 28, 2021
@tekton-robot tekton-robot merged commit 08177fa into tektoncd:master Jan 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/misc Categorizes issue or PR as a miscellaneuous one. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add earlier validation of pipeline result references
3 participants