From bb1155d855615849fbb9479137efbb6b425b3c79 Mon Sep 17 00:00:00 2001 From: Khurram Baig Date: Tue, 17 Oct 2023 21:03:34 +0530 Subject: [PATCH 1/7] [TEP-0104] Task-level Resource Requirements to beta Setting of Task-level Resource Requirements was introduced in v0.39.0. This is a crucial feature required by our customers when running Pipelines at large scale. This promotes TEP-0104 to beta such that these features can be used by the task authors and pipeline authors in a cluster when enable-api-fields is either set to beta or alpha. --- docs/additional-configs.md | 17 +++++++++-------- docs/compute-resources.md | 2 +- pkg/apis/pipeline/v1/pipelinerun_validation.go | 2 +- .../pipeline/v1/pipelinerun_validation_test.go | 4 ++-- pkg/apis/pipeline/v1/taskrun_validation.go | 2 +- pkg/apis/pipeline/v1/taskrun_validation_test.go | 6 +++--- .../pipeline/v1beta1/pipelinerun_validation.go | 2 +- .../v1beta1/pipelinerun_validation_test.go | 16 ++++++++++++++-- pkg/apis/pipeline/v1beta1/taskrun_validation.go | 2 +- .../pipeline/v1beta1/taskrun_validation_test.go | 4 ++-- pkg/pod/pod.go | 2 +- 11 files changed, 36 insertions(+), 23 deletions(-) diff --git a/docs/additional-configs.md b/docs/additional-configs.md index 1f923812010..a4a5150e88e 100644 --- a/docs/additional-configs.md +++ b/docs/additional-configs.md @@ -311,7 +311,6 @@ Features currently in "alpha" are: | [Windows Scripts](./tasks.md#windows-scripts) | [TEP-0057](https://github.com/tektoncd/community/blob/main/teps/0057-windows-support.md) | [v0.28.0](https://github.com/tektoncd/pipeline/releases/tag/v0.28.0) | | | [Debug](./debug.md) | [TEP-0042](https://github.com/tektoncd/community/blob/main/teps/0042-taskrun-breakpoint-on-failure.md) | [v0.26.0](https://github.com/tektoncd/pipeline/releases/tag/v0.26.0) | | | [Step and Sidecar Overrides](./taskruns.md#overriding-task-steps-and-sidecars) | [TEP-0094](https://github.com/tektoncd/community/blob/main/teps/0094-specifying-resource-requirements-at-runtime.md) | [v0.34.0](https://github.com/tektoncd/pipeline/releases/tag/v0.34.0) | | -| [Task-level Resource Requirements](compute-resources.md#task-level-compute-resources-configuration) | [TEP-0104](https://github.com/tektoncd/community/blob/main/teps/0104-tasklevel-resource-requirements.md) | [v0.39.0](https://github.com/tektoncd/pipeline/releases/tag/v0.39.0) | | | [Trusted Resources](./trusted-resources.md) | [TEP-0091](https://github.com/tektoncd/community/blob/main/teps/0091-trusted-resources.md) | N/A | `trusted-resources-verification-no-match-policy` | | [Larger Results via Sidecar Logs](#enabling-larger-results-using-sidecar-logs) | [TEP-0127](https://github.com/tektoncd/community/blob/main/teps/0127-larger-results-via-sidecar-logs.md) | [v0.43.0](https://github.com/tektoncd/pipeline/releases/tag/v0.43.0) | `results-from` | | [Configure Default Resolver](./resolution.md#configuring-built-in-resolvers) | [TEP-0133](https://github.com/tektoncd/community/blob/main/teps/0133-configure-default-resolver.md) | N/A | | @@ -331,14 +330,16 @@ except where otherwise noted. Features currently in "beta" are: -| Feature | Proposal | Alpha Release | Beta Release | Individual Flag | `enable-api-fields=beta` required for `v1beta1` | -|:-------------------------------------------------------------------|:------------------------------------------------------------------------------------------------|:---------------------------------------------------------------------|:---------------------------------------------------------------------------------------------------|:---------------------------------------------------------------------|:------------------------------------------------| -| [Array Results and Array Indexing](pipelineruns.md#specifying-parameters) | [TEP-0076](https://github.com/tektoncd/community/blob/main/teps/0076-array-result-types.md) | [v0.38.0](https://github.com/tektoncd/pipeline/releases/tag/v0.38.0) | [v0.45.0](https://github.com/tektoncd/pipeline/releases/tag/v0.45.0) | | No | -| [Object Parameters and Results](pipelineruns.md#specifying-parameters) | [TEP-0075](https://github.com/tektoncd/community/blob/main/teps/0075-object-param-and-result-types.md) | | [v0.46.0](https://github.com/tektoncd/pipeline/releases/tag/v0.46.0) | | No | -| [Remote Tasks](./taskruns.md#remote-tasks) and [Remote Pipelines](./pipelineruns.md#remote-pipelines) | [TEP-0060](https://github.com/tektoncd/community/blob/main/teps/0060-remote-resolution.md) | | [v0.41.0](https://github.com/tektoncd/pipeline/releases/tag/v0.41.0) | | No | -| [`Provenance` field in Status](pipeline-api.md#provenance)| [issue#5550](https://github.com/tektoncd/pipeline/issues/5550)| [v0.41.0](https://github.com/tektoncd/pipeline/releases/tag/v0.41.0)| [v0.48.0](https://github.com/tektoncd/pipeline/releases/tag/v0.48.0) | `enable-provenance-in-status` | No | -| [Isolated `Step` & `Sidecar` `Workspaces`](./workspaces.md#isolated-workspaces) | [TEP-0029](https://github.com/tektoncd/community/blob/main/teps/0029-step-workspaces.md) | [v0.24.0](https://github.com/tektoncd/pipeline/releases/tag/v0.24.0) | [v0.50.0](https://github.com/tektoncd/pipeline/releases/tag/v0.50.0) | | Yes | +| Feature | Proposal | Alpha Release | Beta Release | Individual Flag | `enable-api-fields=beta` required for `v1beta1` | +|:-------------------------------------------------------------------|:------------------------------------------------------------------------------------------------|:---------------------------------------------------------------------|:---------------------------------------------------------------------|:----------------|:---| +| [Array Results and Array Indexing](pipelineruns.md#specifying-parameters) | [TEP-0076](https://github.com/tektoncd/community/blob/main/teps/0076-array-result-types.md) | [v0.38.0](https://github.com/tektoncd/pipeline/releases/tag/v0.38.0) | [v0.45.0](https://github.com/tektoncd/pipeline/releases/tag/v0.45.0) | | No | +| [Object Parameters and Results](pipelineruns.md#specifying-parameters) | [TEP-0075](https://github.com/tektoncd/community/blob/main/teps/0075-object-param-and-result-types.md) | | [v0.46.0](https://github.com/tektoncd/pipeline/releases/tag/v0.46.0) | | No | +| [Remote Tasks](./taskruns.md#remote-tasks) and [Remote Pipelines](./pipelineruns.md#remote-pipelines) | [TEP-0060](https://github.com/tektoncd/community/blob/main/teps/0060-remote-resolution.md) | | [v0.41.0](https://github.com/tektoncd/pipeline/releases/tag/v0.41.0) | | No | +| [`Provenance` field in Status](pipeline-api.md#provenance)| [issue#5550](https://github.com/tektoncd/pipeline/issues/5550)| [v0.41.0](https://github.com/tektoncd/pipeline/releases/tag/v0.41.0)| [v0.48.0](https://github.com/tektoncd/pipeline/releases/tag/v0.48.0) | `enable-provenance-in-status`| No | +| [Isolated `Step` & `Sidecar` `Workspaces`](./workspaces.md#isolated-workspaces) | [TEP-0029](https://github.com/tektoncd/community/blob/main/teps/0029-step-workspaces.md) | [v0.24.0](https://github.com/tektoncd/pipeline/releases/tag/v0.24.0) | [v0.50.0](https://github.com/tektoncd/pipeline/releases/tag/v0.50.0) | | Yes | | [Matrix](./matrix.md) | [TEP-0090](https://github.com/tektoncd/community/blob/main/teps/0090-matrix.md) | [v0.38.0](https://github.com/tektoncd/pipeline/releases/tag/v0.38.0) | [v0.53.0](https://github.com/tektoncd/pipeline/releases/tag/v0.53.0) | | No | +| [Task-level Resource Requirements](compute-resources.md#task-level-compute-resources-configuration) | [TEP-0104](https://github.com/tektoncd/community/blob/main/teps/0104-tasklevel-resource-requirements.md) | [v0.39.0](https://github.com/tektoncd/pipeline/releases/tag/v0.39.0) | | + ## Enabling larger results using sidecar logs diff --git a/docs/compute-resources.md b/docs/compute-resources.md index ca9cf65e3ec..40de3fe70d4 100644 --- a/docs/compute-resources.md +++ b/docs/compute-resources.md @@ -44,7 +44,7 @@ Therefore, the pod will have no effective CPU limit. ## Task-level Compute Resources Configuration -**([alpha only](https://github.com/tektoncd/pipeline/blob/main/docs/additional-configs.md#alpha-features))** +**([beta](https://github.com/tektoncd/pipeline/blob/main/docs/additional-configs.md#beta-features))** Tekton allows users to specify resource requirements of [`Steps`](./tasks.md#defining-steps), which run sequentially. However, the pod's effective resource requirements are still the diff --git a/pkg/apis/pipeline/v1/pipelinerun_validation.go b/pkg/apis/pipeline/v1/pipelinerun_validation.go index ee7a6dc2c53..e554263705a 100644 --- a/pkg/apis/pipeline/v1/pipelinerun_validation.go +++ b/pkg/apis/pipeline/v1/pipelinerun_validation.go @@ -290,7 +290,7 @@ func validateTaskRunSpec(ctx context.Context, trs PipelineTaskRunSpec) (errs *ap errs = errs.Also(validateSidecarSpecs(trs.SidecarSpecs).ViaField("sidecarSpecs")) } if trs.ComputeResources != nil { - errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "computeResources", config.AlphaAPIFields).ViaField("computeResources")) + errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "computeResources", config.BetaAPIFields).ViaField("computeResources")) errs = errs.Also(validateTaskRunComputeResources(trs.ComputeResources, trs.StepSpecs)) } if trs.PodTemplate != nil { diff --git a/pkg/apis/pipeline/v1/pipelinerun_validation_test.go b/pkg/apis/pipeline/v1/pipelinerun_validation_test.go index a0748fa7590..6ebff9a9bba 100644 --- a/pkg/apis/pipeline/v1/pipelinerun_validation_test.go +++ b/pkg/apis/pipeline/v1/pipelinerun_validation_test.go @@ -1040,7 +1040,7 @@ func TestPipelineRunSpec_Invalidate(t *testing.T) { ), withContext: cfgtesting.EnableAlphaAPIFields, }, { - name: "computeResources disallowed without alpha feature gate", + name: "computeResources disallowed without beta feature gate", spec: v1.PipelineRunSpec{ PipelineRef: &v1.PipelineRef{Name: "foo"}, TaskRunSpecs: []v1.PipelineTaskRunSpec{ @@ -1053,7 +1053,7 @@ func TestPipelineRunSpec_Invalidate(t *testing.T) { }, }, withContext: cfgtesting.EnableStableAPIFields, - wantErr: apis.ErrGeneric("computeResources requires \"enable-api-fields\" feature gate to be \"alpha\" but it is \"stable\"").ViaIndex(0).ViaField("taskRunSpecs"), + wantErr: apis.ErrGeneric("computeResources requires \"enable-api-fields\" feature gate to be \"alpha\" or \"beta\" but it is \"stable\"").ViaIndex(0).ViaField("taskRunSpecs"), }} for _, ps := range tests { diff --git a/pkg/apis/pipeline/v1/taskrun_validation.go b/pkg/apis/pipeline/v1/taskrun_validation.go index d3915dc5d87..87294c4cdc1 100644 --- a/pkg/apis/pipeline/v1/taskrun_validation.go +++ b/pkg/apis/pipeline/v1/taskrun_validation.go @@ -87,7 +87,7 @@ func (ts *TaskRunSpec) Validate(ctx context.Context) (errs *apis.FieldError) { errs = errs.Also(validateSidecarSpecs(ts.SidecarSpecs).ViaField("sidecarSpecs")) } if ts.ComputeResources != nil { - errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "computeResources", config.AlphaAPIFields).ViaField("computeResources")) + errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "computeResources", config.BetaAPIFields).ViaField("computeResources")) errs = errs.Also(validateTaskRunComputeResources(ts.ComputeResources, ts.StepSpecs)) } diff --git a/pkg/apis/pipeline/v1/taskrun_validation_test.go b/pkg/apis/pipeline/v1/taskrun_validation_test.go index 8dae7195b2f..c895cbe5834 100644 --- a/pkg/apis/pipeline/v1/taskrun_validation_test.go +++ b/pkg/apis/pipeline/v1/taskrun_validation_test.go @@ -773,7 +773,7 @@ func TestTaskRunSpec_Invalidate(t *testing.T) { ), wc: cfgtesting.EnableAlphaAPIFields, }, { - name: "computeResources disallowed without alpha feature gate", + name: "computeResources disallowed without beta feature gate", spec: v1.TaskRunSpec{ TaskRef: &v1.TaskRef{ Name: "foo", @@ -785,7 +785,7 @@ func TestTaskRunSpec_Invalidate(t *testing.T) { }, }, wc: cfgtesting.EnableStableAPIFields, - wantErr: apis.ErrGeneric("computeResources requires \"enable-api-fields\" feature gate to be \"alpha\" but it is \"stable\""), + wantErr: apis.ErrGeneric("computeResources requires \"enable-api-fields\" feature gate to be \"alpha\" or \"beta\" but it is \"stable\""), }} for _, ts := range tests { @@ -864,7 +864,7 @@ func TestTaskRunSpec_Validate(t *testing.T) { }, }, }, - wc: cfgtesting.EnableAlphaAPIFields, + wc: cfgtesting.EnableBetaAPIFields, }, { name: "valid sidecar and task-level (spec.resources) resource requirements", spec: v1.TaskRunSpec{ diff --git a/pkg/apis/pipeline/v1beta1/pipelinerun_validation.go b/pkg/apis/pipeline/v1beta1/pipelinerun_validation.go index ad6d5cbbdb4..56d44138805 100644 --- a/pkg/apis/pipeline/v1beta1/pipelinerun_validation.go +++ b/pkg/apis/pipeline/v1beta1/pipelinerun_validation.go @@ -334,7 +334,7 @@ func validateTaskRunSpec(ctx context.Context, trs PipelineTaskRunSpec) (errs *ap errs = errs.Also(validateSidecarOverrides(trs.SidecarOverrides).ViaField("sidecarOverrides")) } if trs.ComputeResources != nil { - errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "computeResources", config.AlphaAPIFields).ViaField("computeResources")) + errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "computeResources", config.BetaAPIFields).ViaField("computeResources")) errs = errs.Also(validateTaskRunComputeResources(trs.ComputeResources, trs.StepOverrides)) } if trs.TaskPodTemplate != nil { diff --git a/pkg/apis/pipeline/v1beta1/pipelinerun_validation_test.go b/pkg/apis/pipeline/v1beta1/pipelinerun_validation_test.go index 9f76c1825f4..b21439ef949 100644 --- a/pkg/apis/pipeline/v1beta1/pipelinerun_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/pipelinerun_validation_test.go @@ -1159,7 +1159,7 @@ func TestPipelineRunSpec_Invalidate(t *testing.T) { ), withContext: cfgtesting.EnableAlphaAPIFields, }, { - name: "computeResources disallowed without alpha feature gate", + name: "computeResources disallowed without beta feature gate", spec: v1beta1.PipelineRunSpec{ PipelineRef: &v1beta1.PipelineRef{Name: "foo"}, TaskRunSpecs: []v1beta1.PipelineTaskRunSpec{ @@ -1172,7 +1172,7 @@ func TestPipelineRunSpec_Invalidate(t *testing.T) { }, }, withContext: cfgtesting.EnableStableAPIFields, - wantErr: apis.ErrGeneric("computeResources requires \"enable-api-fields\" feature gate to be \"alpha\" but it is \"stable\"").ViaIndex(0).ViaField("taskRunSpecs"), + wantErr: apis.ErrGeneric("computeResources requires \"enable-api-fields\" feature gate to be \"alpha\" or \"beta\" but it is \"stable\"").ViaIndex(0).ViaField("taskRunSpecs"), }} for _, ps := range tests { @@ -1208,6 +1208,18 @@ func TestPipelineRunSpec_Validate(t *testing.T) { }, }, { name: "valid task-level (taskRunSpecs.resources) resource requirements configured", + spec: v1beta1.PipelineRunSpec{ + PipelineRef: &v1beta1.PipelineRef{Name: "pipeline"}, + TaskRunSpecs: []v1beta1.PipelineTaskRunSpec{{ + PipelineTaskName: "pipelineTask", + ComputeResources: &corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: corev1resources.MustParse("2Gi")}, + }, + }}, + }, + withContext: cfgtesting.EnableBetaAPIFields, + }, { + name: "valid task-level (taskRunSpecs.resources) resource requirements configured with stepOverride", spec: v1beta1.PipelineRunSpec{ PipelineRef: &v1beta1.PipelineRef{Name: "pipeline"}, TaskRunSpecs: []v1beta1.PipelineTaskRunSpec{{ diff --git a/pkg/apis/pipeline/v1beta1/taskrun_validation.go b/pkg/apis/pipeline/v1beta1/taskrun_validation.go index e49856d2c50..e5108a6464c 100644 --- a/pkg/apis/pipeline/v1beta1/taskrun_validation.go +++ b/pkg/apis/pipeline/v1beta1/taskrun_validation.go @@ -82,7 +82,7 @@ func (ts *TaskRunSpec) Validate(ctx context.Context) (errs *apis.FieldError) { errs = errs.Also(validateSidecarOverrides(ts.SidecarOverrides).ViaField("sidecarOverrides")) } if ts.ComputeResources != nil { - errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "computeResources", config.AlphaAPIFields).ViaField("computeResources")) + errs = errs.Also(config.ValidateEnabledAPIFields(ctx, "computeResources", config.BetaAPIFields).ViaField("computeResources")) errs = errs.Also(validateTaskRunComputeResources(ts.ComputeResources, ts.StepOverrides)) } diff --git a/pkg/apis/pipeline/v1beta1/taskrun_validation_test.go b/pkg/apis/pipeline/v1beta1/taskrun_validation_test.go index 1aaa409741c..6e604fb6e36 100644 --- a/pkg/apis/pipeline/v1beta1/taskrun_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/taskrun_validation_test.go @@ -761,7 +761,7 @@ func TestTaskRunSpec_Invalidate(t *testing.T) { ), wc: cfgtesting.EnableAlphaAPIFields, }, { - name: "computeResources disallowed without alpha feature gate", + name: "computeResources disallowed without beta feature gate", spec: v1beta1.TaskRunSpec{ TaskRef: &v1beta1.TaskRef{ Name: "foo", @@ -773,7 +773,7 @@ func TestTaskRunSpec_Invalidate(t *testing.T) { }, }, wc: cfgtesting.EnableStableAPIFields, - wantErr: apis.ErrGeneric("computeResources requires \"enable-api-fields\" feature gate to be \"alpha\" but it is \"stable\""), + wantErr: apis.ErrGeneric("computeResources requires \"enable-api-fields\" feature gate to be \"alpha\" or \"beta\" but it is \"stable\""), }, { name: "uses resources", spec: v1beta1.TaskRunSpec{ diff --git a/pkg/pod/pod.go b/pkg/pod/pod.go index e99ee7e2c62..cfc7d69d683 100644 --- a/pkg/pod/pod.go +++ b/pkg/pod/pod.go @@ -184,7 +184,7 @@ func (b *Builder) Build(ctx context.Context, taskRun *v1.TaskRun, taskSpec v1.Ta if err != nil { return nil, err } - if alphaAPIEnabled && taskRun.Spec.ComputeResources != nil { + if taskRun.Spec.ComputeResources != nil { tasklevel.ApplyTaskLevelComputeResources(steps, taskRun.Spec.ComputeResources) } windows := usesWindows(taskRun) From a43037e06531288dfe225a8d89a6a0219333ef35 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Thu, 19 Oct 2023 20:01:48 +0000 Subject: [PATCH 2/7] Bump github.com/sigstore/sigstore from 1.7.3 to 1.7.4 Bumps [github.com/sigstore/sigstore](https://github.com/sigstore/sigstore) from 1.7.3 to 1.7.4. - [Release notes](https://github.com/sigstore/sigstore/releases) - [Commits](https://github.com/sigstore/sigstore/compare/v1.7.3...v1.7.4) --- updated-dependencies: - dependency-name: github.com/sigstore/sigstore dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] --- go.mod | 2 +- go.sum | 4 ++-- .../sigstore/sigstore/pkg/signature/payload/payload.go | 2 +- vendor/modules.txt | 4 ++-- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/go.mod b/go.mod index eda56305cf8..822aa0bea1c 100644 --- a/go.mod +++ b/go.mod @@ -18,7 +18,7 @@ require ( github.com/mitchellh/go-homedir v1.1.0 github.com/opencontainers/image-spec v1.1.0-rc5 github.com/pkg/errors v0.9.1 - github.com/sigstore/sigstore v1.7.3 + github.com/sigstore/sigstore v1.7.4 github.com/spiffe/go-spiffe/v2 v2.1.5 github.com/spiffe/spire-api-sdk v1.8.1 github.com/tektoncd/plumbing v0.0.0-20220817140952-3da8ce01aeeb diff --git a/go.sum b/go.sum index 18b40c92b5f..06b648b0c04 100644 --- a/go.sum +++ b/go.sum @@ -1027,8 +1027,8 @@ github.com/shurcooL/githubv4 v0.0.0-20190718010115-4ba037080260/go.mod h1:hAF0iL github.com/shurcooL/graphql v0.0.0-20181231061246-d48a9a75455f h1:tygelZueB1EtXkPI6mQ4o9DQ0+FKW41hTbunoXZCTqk= github.com/shurcooL/graphql v0.0.0-20181231061246-d48a9a75455f/go.mod h1:AuYgA5Kyo4c7HfUmvRGs/6rGlMMV/6B1bVnB9JxJEEg= github.com/shurcooL/sanitized_anchor_name v1.0.0/go.mod h1:1NzhyTcUVG4SuEtjjoZeVRXNmyL/1OwPU0+IJeTBvfc= -github.com/sigstore/sigstore v1.7.3 h1:HVVTfrMezJeLyl2xhJ8edzkrEGBa4KxjQZB4FlQ4JLU= -github.com/sigstore/sigstore v1.7.3/go.mod h1:cl0c7Dtg3MM3c13L8pqqrfrmBa0eM3POcdtBepjylmw= +github.com/sigstore/sigstore v1.7.4 h1:Fyqn6OKOVsYnV0Vs6JhG5t+q0u7Gj6R5dJ52kUVteLs= +github.com/sigstore/sigstore v1.7.4/go.mod h1:5MxR9PrWYGk5I3sXgdnrMUOLbwFPuAUNtWPm3VwOjkc= github.com/sigstore/sigstore/pkg/signature/kms/aws v1.7.4 h1:zIqB/IB8qVJBjazd+fjI6XZlE9f8s5HxnOllHAeTTew= github.com/sigstore/sigstore/pkg/signature/kms/aws v1.7.4/go.mod h1:eorFqzSo/RDsBfl97Ui/mKXw0YQ9qLDT1RhMYiuvPf8= github.com/sigstore/sigstore/pkg/signature/kms/azure v1.7.4 h1:2xI6GX+tQMF0L+DlQq09U4fH8PlFhuz0wSYkiHY8fgo= diff --git a/vendor/github.com/sigstore/sigstore/pkg/signature/payload/payload.go b/vendor/github.com/sigstore/sigstore/pkg/signature/payload/payload.go index 2764b4b3153..cab6f5b98a7 100644 --- a/vendor/github.com/sigstore/sigstore/pkg/signature/payload/payload.go +++ b/vendor/github.com/sigstore/sigstore/pkg/signature/payload/payload.go @@ -27,7 +27,7 @@ import ( const CosignSignatureType = "cosign container image signature" // SimpleContainerImage describes the structure of a basic container image signature payload, as defined at: -// https://github.com/containers/image/blob/master/docs/containers-signature.5.md#json-data-format +// https://github.com/containers/image/blob/main/docs/containers-signature.5.md#json-data-format type SimpleContainerImage struct { Critical Critical `json:"critical"` // Critical data critical to correctly evaluating the validity of the signature Optional map[string]interface{} `json:"optional"` // Optional optional metadata about the image diff --git a/vendor/modules.txt b/vendor/modules.txt index 9b9a32d508b..6af4a033530 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -828,8 +828,8 @@ github.com/shurcooL/githubv4 github.com/shurcooL/graphql github.com/shurcooL/graphql/ident github.com/shurcooL/graphql/internal/jsonutil -# github.com/sigstore/sigstore v1.7.3 -## explicit; go 1.19 +# github.com/sigstore/sigstore v1.7.4 +## explicit; go 1.20 github.com/sigstore/sigstore/pkg/cryptoutils github.com/sigstore/sigstore/pkg/signature github.com/sigstore/sigstore/pkg/signature/kms From 5066b40c0e5cd699bea33eb2ed5444bf4e06d7b9 Mon Sep 17 00:00:00 2001 From: joey Date: Mon, 11 Sep 2023 16:13:23 +0800 Subject: [PATCH 3/7] 1. add container type substitution expresions to pipeline task result reference 2. propagate results to embedded task spec Part of work on issue #7086 Signed-off-by: chengjoey --- docs/pipelineruns.md | 86 +++ ...ropagating_results_implicit_resultref.yaml | 34 ++ pkg/apis/pipeline/v1/container_types.go | 60 ++ pkg/apis/pipeline/v1/container_types_test.go | 122 ++++ pkg/apis/pipeline/v1/pipeline_validation.go | 16 + pkg/apis/pipeline/v1/resultref.go | 4 + pkg/apis/pipeline/v1/resultref_test.go | 254 ++++++++ pkg/reconciler/pipelinerun/pipelinerun.go | 3 + .../pipelinerun/pipelinerun_test.go | 542 ++++++++++++++++++ pkg/reconciler/pipelinerun/resources/apply.go | 24 + .../pipelinerun/resources/apply_test.go | 320 +++++++++++ test/propagated_results_test.go | 482 ++++++++++++++++ 12 files changed, 1947 insertions(+) create mode 100644 examples/v1/pipelineruns/propagating_results_implicit_resultref.yaml create mode 100644 pkg/apis/pipeline/v1/container_types_test.go create mode 100644 test/propagated_results_test.go diff --git a/docs/pipelineruns.md b/docs/pipelineruns.md index 482c9e40913..a8ab9cf2187 100644 --- a/docs/pipelineruns.md +++ b/docs/pipelineruns.md @@ -744,6 +744,92 @@ spec: then `test-task` will execute using the `sa-1` account while `build-task` will execute with `sa-for-build`. +#### Propagated Results + +When using an embedded spec, `Results` from the parent `PipelineRun` will be +propagated to any inlined specs without needing to be explicitly defined. This +allows authors to simplify specs by automatically propagating top-level +results down to other inlined resources. +**`Result` substitutions will only be made for `name`, `commands`, `args`, `env` and `script` fields of `steps`, `sidecars`.** + +```yaml +apiVersion: tekton.dev/v1 +kind: PipelineRun +metadata: + name: uid-pipeline-run +spec: + pipelineSpec: + tasks: + - name: add-uid + taskSpec: + results: + - name: uid + type: string + steps: + - name: add-uid + image: busybox + command: ["/bin/sh", "-c"] + args: + - echo "1001" | tee $(results.uid.path) + - name: show-uid + # params: + # - name: uid + # value: $(tasks.add-uid.results.uid) + taskSpec: + steps: + - name: show-uid + image: busybox + command: ["/bin/sh", "-c"] + args: + - echo + # - $(params.uid) + - $(tasks.add-uid.results.uid) +``` + +On executing the `PipelineRun`, the `Results` will be interpolated during resolution. + +```yaml +name: uid-pipeline-run-show-uid +apiVersion: tekton.dev/v1 +kind: TaskRun +metadata: + ... +spec: + taskSpec: + steps: + args: + echo + 1001 + command: + - /bin/sh + - -c + image: busybox + name: show-uid +status: + completionTime: 2023-09-11T07:34:28Z + conditions: + lastTransitionTime: 2023-09-11T07:34:28Z + message: All Steps have completed executing + reason: Succeeded + status: True + type: Succeeded + podName: uid-pipeline-run-show-uid-pod + steps: + container: step-show-uid + name: show-uid + taskSpec: + steps: + args: + echo + 1001 + command: + /bin/sh + -c + computeResources: + image: busybox + name: show-uid +``` + ### Specifying a `Pod` template You can specify a [`Pod` template](podtemplates.md) configuration that will serve as the configuration starting diff --git a/examples/v1/pipelineruns/propagating_results_implicit_resultref.yaml b/examples/v1/pipelineruns/propagating_results_implicit_resultref.yaml new file mode 100644 index 00000000000..35c086393c6 --- /dev/null +++ b/examples/v1/pipelineruns/propagating_results_implicit_resultref.yaml @@ -0,0 +1,34 @@ +apiVersion: tekton.dev/v1 +kind: Task +metadata: + name: uid-task +spec: + results: + - name: uid + type: string + steps: + - name: uid + image: busybox + command: ["/bin/sh", "-c"] + args: + - echo "1001" | tee $(results.uid.path) +--- +apiVersion: tekton.dev/v1 +kind: PipelineRun +metadata: + name: uid-pipeline-run +spec: + pipelineSpec: + tasks: + - name: add-uid + taskRef: + name: uid-task + - name: show-uid + taskSpec: + steps: + - name: show-uid + image: busybox + command: ["/bin/sh", "-c"] + args: + - echo + - $(tasks.add-uid.results.uid) \ No newline at end of file diff --git a/pkg/apis/pipeline/v1/container_types.go b/pkg/apis/pipeline/v1/container_types.go index 79c9922f462..ccef95cf767 100644 --- a/pkg/apis/pipeline/v1/container_types.go +++ b/pkg/apis/pipeline/v1/container_types.go @@ -188,6 +188,36 @@ func (s *Step) SetContainerFields(c corev1.Container) { s.SecurityContext = c.SecurityContext } +// GetVarSubstitutionExpressions walks all the places a substitution reference can be used +func (s *Step) GetVarSubstitutionExpressions() []string { + var allExpressions []string + allExpressions = append(allExpressions, validateString(s.Name)...) + allExpressions = append(allExpressions, validateString(s.Image)...) + allExpressions = append(allExpressions, validateString(string(s.ImagePullPolicy))...) + allExpressions = append(allExpressions, validateString(s.Script)...) + allExpressions = append(allExpressions, validateString(s.WorkingDir)...) + for _, cmd := range s.Command { + allExpressions = append(allExpressions, validateString(cmd)...) + } + for _, arg := range s.Args { + allExpressions = append(allExpressions, validateString(arg)...) + } + for _, env := range s.Env { + allExpressions = append(allExpressions, validateString(env.Value)...) + if env.ValueFrom != nil { + if env.ValueFrom.SecretKeyRef != nil { + allExpressions = append(allExpressions, validateString(env.ValueFrom.SecretKeyRef.Key)...) + allExpressions = append(allExpressions, validateString(env.ValueFrom.SecretKeyRef.LocalObjectReference.Name)...) + } + if env.ValueFrom.ConfigMapKeyRef != nil { + allExpressions = append(allExpressions, validateString(env.ValueFrom.ConfigMapKeyRef.Key)...) + allExpressions = append(allExpressions, validateString(env.ValueFrom.ConfigMapKeyRef.LocalObjectReference.Name)...) + } + } + } + return allExpressions +} + // StepTemplate is a template for a Step type StepTemplate struct { // Image reference name. @@ -541,3 +571,33 @@ func (s *Sidecar) SetContainerFields(c corev1.Container) { s.StdinOnce = c.StdinOnce s.TTY = c.TTY } + +// GetVarSubstitutionExpressions walks all the places a substitution reference can be used +func (s *Sidecar) GetVarSubstitutionExpressions() []string { + var allExpressions []string + allExpressions = append(allExpressions, validateString(s.Name)...) + allExpressions = append(allExpressions, validateString(s.Image)...) + allExpressions = append(allExpressions, validateString(string(s.ImagePullPolicy))...) + allExpressions = append(allExpressions, validateString(s.Script)...) + allExpressions = append(allExpressions, validateString(s.WorkingDir)...) + for _, cmd := range s.Command { + allExpressions = append(allExpressions, validateString(cmd)...) + } + for _, arg := range s.Args { + allExpressions = append(allExpressions, validateString(arg)...) + } + for _, env := range s.Env { + allExpressions = append(allExpressions, validateString(env.Value)...) + if env.ValueFrom != nil { + if env.ValueFrom.SecretKeyRef != nil { + allExpressions = append(allExpressions, validateString(env.ValueFrom.SecretKeyRef.Key)...) + allExpressions = append(allExpressions, validateString(env.ValueFrom.SecretKeyRef.LocalObjectReference.Name)...) + } + if env.ValueFrom.ConfigMapKeyRef != nil { + allExpressions = append(allExpressions, validateString(env.ValueFrom.ConfigMapKeyRef.Key)...) + allExpressions = append(allExpressions, validateString(env.ValueFrom.ConfigMapKeyRef.LocalObjectReference.Name)...) + } + } + } + return allExpressions +} diff --git a/pkg/apis/pipeline/v1/container_types_test.go b/pkg/apis/pipeline/v1/container_types_test.go new file mode 100644 index 00000000000..06948f9acaa --- /dev/null +++ b/pkg/apis/pipeline/v1/container_types_test.go @@ -0,0 +1,122 @@ +/* + * + * Copyright 2019 The Tekton Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * / + */ + +package v1 + +import ( + "testing" + + "github.com/google/go-cmp/cmp" + corev1 "k8s.io/api/core/v1" +) + +func TestStepGetVarSubstitutionExpressions(t *testing.T) { + s := Step{ + Name: "$(tasks.task1.results.stepName)", + Image: "$(tasks.task1.results.imageResult)", + ImagePullPolicy: corev1.PullPolicy("$(tasks.task1.results.imagePullPolicy)"), + Script: "substitution within string $(tasks.task1.results.scriptResult)", + WorkingDir: "$(tasks.task1.results.workingDir)", + Command: []string{ + "$(tasks.task2.results.command[*])", + }, + Args: []string{ + "$(tasks.task2.results.args[*])", + }, + Env: []corev1.EnvVar{ + { + Name: "env1", + Value: "$(tasks.task2.results.env1)", + ValueFrom: &corev1.EnvVarSource{ + SecretKeyRef: &corev1.SecretKeySelector{ + Key: "$(tasks.task2.results.secretKeyRef)", + LocalObjectReference: corev1.LocalObjectReference{ + Name: "$(tasks.task2.results.secretNameRef)", + }, + }, + ConfigMapKeyRef: &corev1.ConfigMapKeySelector{ + Key: "$(tasks.task2.results.configMapKeyRef)", + LocalObjectReference: corev1.LocalObjectReference{ + Name: "$(tasks.task2.results.configMapNameRef)", + }, + }, + }, + }, + }, + } + subRefExpressions := s.GetVarSubstitutionExpressions() + wantRefExpressions := []string{ + "tasks.task1.results.stepName", + "tasks.task1.results.imageResult", + "tasks.task1.results.imagePullPolicy", + "tasks.task1.results.scriptResult", + "tasks.task1.results.workingDir", + "tasks.task2.results.command[*]", + "tasks.task2.results.args[*]", + "tasks.task2.results.env1", + "tasks.task2.results.secretKeyRef", + "tasks.task2.results.secretNameRef", + "tasks.task2.results.configMapKeyRef", + "tasks.task2.results.configMapNameRef", + } + if d := cmp.Diff(wantRefExpressions, subRefExpressions); d != "" { + t.Fatalf("Unexpected result (-want, +got): %s", d) + } +} + +func TestSidecarGetVarSubstitutionExpressions(t *testing.T) { + s := Sidecar{ + Name: "$(tasks.task1.results.sidecarName)", + Image: "$(tasks.task1.results.sidecarImage)", + ImagePullPolicy: corev1.PullPolicy("$(tasks.task1.results.sidecarImagePullPolicy)"), + Env: []corev1.EnvVar{ + { + Name: "env1", + Value: "$(tasks.task2.results.sidecarEnv1)", + ValueFrom: &corev1.EnvVarSource{ + SecretKeyRef: &corev1.SecretKeySelector{ + Key: "$(tasks.task2.results.sidecarSecretKeyRef)", + LocalObjectReference: corev1.LocalObjectReference{ + Name: "$(tasks.task2.results.sidecarSecretNameRef)", + }, + }, + ConfigMapKeyRef: &corev1.ConfigMapKeySelector{ + Key: "$(tasks.task2.results.sidecarConfigMapKeyRef)", + LocalObjectReference: corev1.LocalObjectReference{ + Name: "$(tasks.task2.results.sidecarConfigMapNameRef)", + }, + }, + }, + }, + }, + } + subRefExpressions := s.GetVarSubstitutionExpressions() + wantRefExpressions := []string{ + "tasks.task1.results.sidecarName", + "tasks.task1.results.sidecarImage", + "tasks.task1.results.sidecarImagePullPolicy", + "tasks.task2.results.sidecarEnv1", + "tasks.task2.results.sidecarSecretKeyRef", + "tasks.task2.results.sidecarSecretNameRef", + "tasks.task2.results.sidecarConfigMapKeyRef", + "tasks.task2.results.sidecarConfigMapNameRef", + } + if d := cmp.Diff(wantRefExpressions, subRefExpressions); d != "" { + t.Fatalf("Unexpected result (-want, +got): %s", d) + } +} diff --git a/pkg/apis/pipeline/v1/pipeline_validation.go b/pkg/apis/pipeline/v1/pipeline_validation.go index 7b2de9f77ff..a5cea7d8760 100644 --- a/pkg/apis/pipeline/v1/pipeline_validation.go +++ b/pkg/apis/pipeline/v1/pipeline_validation.go @@ -497,6 +497,22 @@ func (pt *PipelineTask) extractAllParams() Params { return allParams } +// GetVarSubstitutionExpressions extract all values between the parameters "$(" and ")" of steps and sidecars +func (pt *PipelineTask) GetVarSubstitutionExpressions() []string { + var allExpressions []string + if pt.TaskSpec != nil { + for _, step := range pt.TaskSpec.Steps { + stepExpressions := step.GetVarSubstitutionExpressions() + allExpressions = append(allExpressions, stepExpressions...) + } + for _, sidecar := range pt.TaskSpec.Sidecars { + sidecarExpressions := sidecar.GetVarSubstitutionExpressions() + allExpressions = append(allExpressions, sidecarExpressions...) + } + } + return allExpressions +} + func containsExecutionStatusRef(p string) bool { if strings.HasPrefix(p, "tasks.") && strings.HasSuffix(p, ".status") { return true diff --git a/pkg/apis/pipeline/v1/resultref.go b/pkg/apis/pipeline/v1/resultref.go index 14b0de17fdb..2de6bb80804 100644 --- a/pkg/apis/pipeline/v1/resultref.go +++ b/pkg/apis/pipeline/v1/resultref.go @@ -172,6 +172,8 @@ func ParseResultName(resultName string) (string, string) { // in a PipelineTask and returns a list of any references that are found. func PipelineTaskResultRefs(pt *PipelineTask) []*ResultRef { refs := []*ResultRef{} + // TODO move the whenExpression.GetVarSubstitutionExpressions() and GetVarSubstitutionExpressionsForParam(p) as well + // separate cleanup, reference https://github.com/tektoncd/pipeline/pull/7121 for _, p := range pt.extractAllParams() { expressions, _ := p.GetVarSubstitutionExpressions() refs = append(refs, NewResultRefs(expressions)...) @@ -180,5 +182,7 @@ func PipelineTaskResultRefs(pt *PipelineTask) []*ResultRef { expressions, _ := whenExpression.GetVarSubstitutionExpressions() refs = append(refs, NewResultRefs(expressions)...) } + taskSubExpressions := pt.GetVarSubstitutionExpressions() + refs = append(refs, NewResultRefs(taskSubExpressions)...) return refs } diff --git a/pkg/apis/pipeline/v1/resultref_test.go b/pkg/apis/pipeline/v1/resultref_test.go index a471418edec..0ec0c37a79c 100644 --- a/pkg/apis/pipeline/v1/resultref_test.go +++ b/pkg/apis/pipeline/v1/resultref_test.go @@ -23,6 +23,7 @@ import ( "github.com/google/go-cmp/cmp/cmpopts" v1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1" "github.com/tektoncd/pipeline/test/diff" + corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/selection" ) @@ -472,6 +473,224 @@ func TestNewResultReferenceWhenExpressions(t *testing.T) { } } +func TestNewResultReferenceTaskSpec(t *testing.T) { + for _, tt := range []struct { + name string + pt *v1.PipelineTask + wantRef []*v1.ResultRef + }{ + { + name: "Test step expression references", + pt: &v1.PipelineTask{ + TaskSpec: &v1.EmbeddedTask{ + TaskSpec: v1.TaskSpec{ + Steps: []v1.Step{ + { + Name: "$(tasks.task1.results.stepName)", + Image: "$(tasks.task1.results.imageResult)", + ImagePullPolicy: corev1.PullPolicy("$(tasks.task1.results.imagePullPolicy)"), + Script: "substitution within string $(tasks.task1.results.scriptResult)", + WorkingDir: "$(tasks.task1.results.workingDir)", + Command: []string{ + "$(tasks.task2.results.command[*])", + }, + Args: []string{ + "$(tasks.task2.results.args[*])", + }, + Env: []corev1.EnvVar{ + { + Name: "env1", + Value: "$(tasks.task2.results.env1)", + ValueFrom: &corev1.EnvVarSource{ + SecretKeyRef: &corev1.SecretKeySelector{ + Key: "$(tasks.task2.results.secretKeyRef)", + LocalObjectReference: corev1.LocalObjectReference{ + Name: "$(tasks.task2.results.secretNameRef)", + }, + }, + ConfigMapKeyRef: &corev1.ConfigMapKeySelector{ + Key: "$(tasks.task2.results.configMapKeyRef)", + LocalObjectReference: corev1.LocalObjectReference{ + Name: "$(tasks.task2.results.configMapNameRef)", + }, + }, + }, + }, + }, + }, + }, + }, + }, + }, + wantRef: []*v1.ResultRef{{ + PipelineTask: "task1", + Result: "stepName", + }, { + PipelineTask: "task1", + Result: "imageResult", + }, { + PipelineTask: "task1", + Result: "imagePullPolicy", + }, { + PipelineTask: "task1", + Result: "scriptResult", + }, { + PipelineTask: "task1", + Result: "workingDir", + }, { + PipelineTask: "task2", + Result: "command", + }, { + PipelineTask: "task2", + Result: "args", + }, { + PipelineTask: "task2", + Result: "env1", + }, { + PipelineTask: "task2", + Result: "secretKeyRef", + }, { + PipelineTask: "task2", + Result: "secretNameRef", + }, { + PipelineTask: "task2", + Result: "configMapKeyRef", + }, { + PipelineTask: "task2", + Result: "configMapNameRef", + }}, + }, { + name: "Test sidecar expression references", + pt: &v1.PipelineTask{ + TaskSpec: &v1.EmbeddedTask{ + TaskSpec: v1.TaskSpec{ + Sidecars: []v1.Sidecar{ + { + Name: "$(tasks.task1.results.stepName)", + Image: "$(tasks.task1.results.imageResult)", + ImagePullPolicy: corev1.PullPolicy("$(tasks.task1.results.imagePullPolicy)"), + Script: "substitution within string $(tasks.task1.results.scriptResult)", + WorkingDir: "$(tasks.task1.results.workingDir)", + Command: []string{ + "$(tasks.task2.results.command[*])", + }, + Args: []string{ + "$(tasks.task2.results.args[*])", + }, + Env: []corev1.EnvVar{ + { + Name: "env1", + Value: "$(tasks.task2.results.env1)", + ValueFrom: &corev1.EnvVarSource{ + SecretKeyRef: &corev1.SecretKeySelector{ + Key: "$(tasks.task2.results.secretKeyRef)", + LocalObjectReference: corev1.LocalObjectReference{ + Name: "$(tasks.task2.results.secretNameRef)", + }, + }, + ConfigMapKeyRef: &corev1.ConfigMapKeySelector{ + Key: "$(tasks.task2.results.configMapKeyRef)", + LocalObjectReference: corev1.LocalObjectReference{ + Name: "$(tasks.task2.results.configMapNameRef)", + }, + }, + }, + }, + }, + }, + }, + }, + }, + }, + wantRef: []*v1.ResultRef{{ + PipelineTask: "task1", + Result: "stepName", + }, { + PipelineTask: "task1", + Result: "imageResult", + }, { + PipelineTask: "task1", + Result: "imagePullPolicy", + }, { + PipelineTask: "task1", + Result: "scriptResult", + }, { + PipelineTask: "task1", + Result: "workingDir", + }, { + PipelineTask: "task2", + Result: "command", + }, { + PipelineTask: "task2", + Result: "args", + }, { + PipelineTask: "task2", + Result: "env1", + }, { + PipelineTask: "task2", + Result: "secretKeyRef", + }, { + PipelineTask: "task2", + Result: "secretNameRef", + }, { + PipelineTask: "task2", + Result: "configMapKeyRef", + }, { + PipelineTask: "task2", + Result: "configMapNameRef", + }}, + }, { + name: "Test both step and sidecar expression references", + pt: &v1.PipelineTask{ + TaskSpec: &v1.EmbeddedTask{ + TaskSpec: v1.TaskSpec{ + Steps: []v1.Step{ + { + Name: "$(tasks.task1.results.stepName)", + Image: "$(tasks.task1.results.stepImage)", + ImagePullPolicy: corev1.PullPolicy("$(tasks.task1.results.stepImagePullPolicy)"), + }, + }, + Sidecars: []v1.Sidecar{ + { + Name: "$(tasks.task2.results.sidecarName)", + Image: "$(tasks.task2.results.sidecarImage)", + ImagePullPolicy: corev1.PullPolicy("$(tasks.task2.results.sidecarImagePullPolicy)"), + }, + }, + }, + }, + }, + wantRef: []*v1.ResultRef{{ + PipelineTask: "task1", + Result: "stepName", + }, { + PipelineTask: "task1", + Result: "stepImage", + }, { + PipelineTask: "task1", + Result: "stepImagePullPolicy", + }, { + PipelineTask: "task2", + Result: "sidecarName", + }, { + PipelineTask: "task2", + Result: "sidecarImage", + }, { + PipelineTask: "task2", + Result: "sidecarImagePullPolicy", + }}, + }, + } { + t.Run(tt.name, func(t *testing.T) { + got := v1.PipelineTaskResultRefs(tt.pt) + if d := cmp.Diff(tt.wantRef, got); d != "" { + t.Error(diff.PrintWantGot(d)) + } + }) + } +} + func TestHasResultReferenceWhenExpression(t *testing.T) { for _, tt := range []struct { name string @@ -650,6 +869,20 @@ func TestPipelineTaskResultRefs(t *testing.T) { }, { Value: *v1.NewStructuredValues("$(tasks.pt7.results.r7)", "$(tasks.pt8.results.r8)"), }}}, + TaskSpec: &v1.EmbeddedTask{ + TaskSpec: v1.TaskSpec{ + Steps: []v1.Step{ + { + Name: "$(tasks.pt8.results.r8)", + Image: "$(tasks.pt9.results.r9)", + ImagePullPolicy: corev1.PullPolicy("$(tasks.pt10.results.r10)"), + Command: []string{"$(tasks.pt11.results.r11)"}, + Args: []string{"$(tasks.pt12.results.r12)", "$(tasks.pt13.results.r13)"}, + Script: "$(tasks.pt14.results.r14)", + }, + }, + }, + }, } refs := v1.PipelineTaskResultRefs(&pt) expectedRefs := []*v1.ResultRef{{ @@ -679,6 +912,27 @@ func TestPipelineTaskResultRefs(t *testing.T) { }, { PipelineTask: "pt9", Result: "r9", + }, { + PipelineTask: "pt8", + Result: "r8", + }, { + PipelineTask: "pt9", + Result: "r9", + }, { + PipelineTask: "pt10", + Result: "r10", + }, { + PipelineTask: "pt11", + Result: "r11", + }, { + PipelineTask: "pt12", + Result: "r12", + }, { + PipelineTask: "pt13", + Result: "r13", + }, { + PipelineTask: "pt14", + Result: "r14", }} if d := cmp.Diff(refs, expectedRefs, cmpopts.SortSlices(lessResultRef)); d != "" { t.Errorf("%v", d) diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index b06da51b21e..90a61b73420 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -808,6 +808,9 @@ func (c *Reconciler) runNextSchedulableTask(ctx context.Context, pr *v1.Pipeline continue } + // propagate previous task results + resources.PropagateResults(rpt, pipelineRunFacts.State) + // Validate parameter types in matrix after apply substitutions from Task Results if rpt.PipelineTask.IsMatrixed() { if err := resources.ValidateParameterTypesInMatrix(pipelineRunFacts.State); err != nil { diff --git a/pkg/reconciler/pipelinerun/pipelinerun_test.go b/pkg/reconciler/pipelinerun/pipelinerun_test.go index 57cdebd4965..e118173020c 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun_test.go +++ b/pkg/reconciler/pipelinerun/pipelinerun_test.go @@ -43,11 +43,14 @@ import ( resolutionutil "github.com/tektoncd/pipeline/pkg/internal/resolution" "github.com/tektoncd/pipeline/pkg/reconciler/events/cloudevent" "github.com/tektoncd/pipeline/pkg/reconciler/events/k8sevent" + "github.com/tektoncd/pipeline/pkg/reconciler/pipeline/dag" "github.com/tektoncd/pipeline/pkg/reconciler/pipelinerun/resources" + taskresources "github.com/tektoncd/pipeline/pkg/reconciler/taskrun/resources" ttesting "github.com/tektoncd/pipeline/pkg/reconciler/testing" "github.com/tektoncd/pipeline/pkg/reconciler/volumeclaim" resolutioncommon "github.com/tektoncd/pipeline/pkg/resolution/common" remoteresource "github.com/tektoncd/pipeline/pkg/resolution/resource" + "github.com/tektoncd/pipeline/pkg/tracing" "github.com/tektoncd/pipeline/pkg/trustedresources" "github.com/tektoncd/pipeline/pkg/trustedresources/verifier" "github.com/tektoncd/pipeline/test" @@ -15485,6 +15488,545 @@ spec: } } +func Test_runNextSchedulableTask(t *testing.T) { + for _, tc := range []struct { + name string + pr *v1.PipelineRun + pipelineRunFacts *resources.PipelineRunFacts + want *resources.PipelineRunFacts + wantErr bool + }{ + { + name: "propagate strings, array, object results to next task", + pr: &v1.PipelineRun{ + Status: v1.PipelineRunStatus{ + Status: duckv1.Status{ + Conditions: duckv1.Conditions{ + { + Type: apis.ConditionSucceeded, + Status: corev1.ConditionUnknown, + }, + }, + }, + }, + Spec: v1.PipelineRunSpec{ + Status: v1.PipelineRunSpecStatusPending, + PipelineSpec: &v1.PipelineSpec{ + Tasks: []v1.PipelineTask{ + { + Name: "task1", + }, + { + Name: "task2", + }, + }, + }, + }, + }, + pipelineRunFacts: &resources.PipelineRunFacts{ + State: resources.PipelineRunState{ + { + TaskRunNames: []string{"task1"}, + ResolvedTask: &taskresources.ResolvedTask{ + TaskName: "task1", + TaskSpec: &v1.TaskSpec{ + Steps: []v1.Step{ + { + Name: "step1", + }, + }, + }, + }, + PipelineTask: &v1.PipelineTask{ + Name: "task1", + }, + TaskRuns: []*v1.TaskRun{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "task1", + }, + Status: v1.TaskRunStatus{ + Status: duckv1.Status{ + Conditions: duckv1.Conditions{ + { + Type: apis.ConditionSucceeded, + Status: corev1.ConditionTrue, + }, + }, + }, + TaskRunStatusFields: v1.TaskRunStatusFields{ + // string, array, object results generated by task1 + Results: []v1.TaskRunResult{ + { + Name: "foo", + Type: v1.ResultsTypeString, + Value: v1.ResultValue{ + StringVal: "bar", + }, + }, + { + Name: "commands", + Type: v1.ResultsTypeArray, + Value: v1.ResultValue{ + ArrayVal: []string{"bash", "-c"}, + }, + }, + { + Name: "objects", + Type: v1.ResultsTypeObject, + Value: v1.ResultValue{ + ObjectVal: map[string]string{ + "image": "alpine:latest", + }, + }, + }, + }, + }, + }, + Spec: v1.TaskRunSpec{TaskSpec: &v1.TaskSpec{}}, + }, + }, + }, + { + TaskRunNames: []string{"task2"}, + ResolvedTask: &taskresources.ResolvedTask{ + TaskName: "task2", + TaskSpec: &v1.TaskSpec{ + Steps: []v1.Step{ + { + // The results from task1 should be propagated to task2 + Name: "$(tasks.task1.results.foo)", + Command: []string{"$(tasks.task1.results.commands[*])"}, + Image: "$(tasks.task1.results.objects.image)", + }, + }, + }, + }, + PipelineTask: &v1.PipelineTask{ + Name: "task2", + }, + }, + }, + TasksGraph: &dag.Graph{ + Nodes: map[string]*dag.Node{ + "task1": { + Key: "task1", + Next: []*dag.Node{ + { + Key: "task2", + }, + }, + }, + "task2": { + Key: "task2", + Prev: []*dag.Node{ + { + Key: "task1", + }, + }, + }, + }, + }, + FinalTasksGraph: &dag.Graph{ + Nodes: map[string]*dag.Node{}, + }, + }, + want: &resources.PipelineRunFacts{ + State: resources.PipelineRunState{ + { + TaskRunNames: []string{"task1"}, + ResolvedTask: &taskresources.ResolvedTask{ + TaskName: "task1", + TaskSpec: &v1.TaskSpec{ + Steps: []v1.Step{{Name: "step1"}}, + }, + }, + PipelineTask: &v1.PipelineTask{ + Name: "task1", + }, + TaskRuns: []*v1.TaskRun{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "task1", + }, + Status: v1.TaskRunStatus{ + Status: duckv1.Status{ + Conditions: duckv1.Conditions{ + { + Type: apis.ConditionSucceeded, + Status: corev1.ConditionTrue, + }, + }, + }, + TaskRunStatusFields: v1.TaskRunStatusFields{ + Results: []v1.TaskRunResult{ + { + Name: "foo", + Type: v1.ResultsTypeString, + Value: v1.ResultValue{ + StringVal: "bar", + }, + }, + { + Name: "commands", + Type: v1.ResultsTypeArray, + Value: v1.ResultValue{ + ArrayVal: []string{"bash", "-c"}, + }, + }, + { + Name: "objects", + Type: v1.ResultsTypeObject, + Value: v1.ResultValue{ + ObjectVal: map[string]string{ + "image": "alpine:latest", + }, + }, + }, + }, + }, + }, + Spec: v1.TaskRunSpec{TaskSpec: &v1.TaskSpec{}}, + }, + }, + }, + { + TaskRunNames: []string{"task2"}, + ResolvedTask: &taskresources.ResolvedTask{ + TaskName: "task2", + TaskSpec: &v1.TaskSpec{ + Steps: []v1.Step{ + { + // here we expect the results from task1 to be propagated to task2 + Name: "bar", + Command: []string{"bash", "-c"}, + Image: "alpine:latest", + }, + }, + }, + }, + PipelineTask: &v1.PipelineTask{ + Name: "task2", + }, + TaskRuns: []*v1.TaskRun{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "task2", + ResourceVersion: "00002", + Labels: map[string]string{"tekton.dev/pipelineRun": "", "tekton.dev/pipelineTask": "task2"}, + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: "tekton.dev/v1", + Kind: "PipelineRun", + Controller: &[]bool{true}[0], + BlockOwnerDeletion: &[]bool{true}[0], + }, + }, + Annotations: map[string]string{}, + }, + Status: v1.TaskRunStatus{ + Status: duckv1.Status{}, + }, + Spec: v1.TaskRunSpec{}, + }, + }, + }, + }, + TasksGraph: &dag.Graph{ + Nodes: map[string]*dag.Node{ + "task1": { + Key: "task1", + Next: []*dag.Node{ + { + Key: "task2", + }, + }, + }, + "task2": { + Key: "task2", + Prev: []*dag.Node{ + { + Key: "task1", + }, + }, + }, + }, + }, + FinalTasksGraph: &dag.Graph{ + Nodes: map[string]*dag.Node{}, + }, + SkipCache: map[string]resources.TaskSkipStatus{ + "task1": {SkippingReason: "None"}, + "task2": {SkippingReason: "None"}, + }, + }, + wantErr: false, + }, + { + name: "don't propagate results to next task if task is done", + pr: &v1.PipelineRun{ + Status: v1.PipelineRunStatus{ + Status: duckv1.Status{ + Conditions: duckv1.Conditions{ + { + Type: apis.ConditionSucceeded, + Status: corev1.ConditionUnknown, + }, + }, + }, + }, + Spec: v1.PipelineRunSpec{ + Status: v1.PipelineRunSpecStatusPending, + PipelineSpec: &v1.PipelineSpec{ + Tasks: []v1.PipelineTask{ + { + Name: "task1", + }, + { + Name: "task2", + }, + }, + }, + }, + }, + pipelineRunFacts: &resources.PipelineRunFacts{ + State: resources.PipelineRunState{ + { + TaskRunNames: []string{"task1"}, + ResolvedTask: &taskresources.ResolvedTask{ + TaskName: "task1", + TaskSpec: &v1.TaskSpec{ + Steps: []v1.Step{ + { + Name: "step1", + }, + }, + }, + }, + PipelineTask: &v1.PipelineTask{ + Name: "task1", + }, + TaskRuns: []*v1.TaskRun{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "task1", + }, + Status: v1.TaskRunStatus{ + Status: duckv1.Status{ + Conditions: duckv1.Conditions{ + { + Type: apis.ConditionSucceeded, + Status: corev1.ConditionTrue, + }, + }, + }, + TaskRunStatusFields: v1.TaskRunStatusFields{ + Results: []v1.TaskRunResult{ + { + Name: "foo", + Type: v1.ResultsTypeString, + Value: v1.ResultValue{ + StringVal: "bar", + }, + }, + }, + }, + }, + Spec: v1.TaskRunSpec{TaskSpec: &v1.TaskSpec{}}, + }, + }, + }, + { + TaskRunNames: []string{"task2"}, + ResolvedTask: &taskresources.ResolvedTask{ + TaskName: "task2", + TaskSpec: &v1.TaskSpec{ + Steps: []v1.Step{ + { + Name: "$(tasks.task1.results.foo)", + }, + }, + }, + }, + PipelineTask: &v1.PipelineTask{ + Name: "task2", + }, + TaskRuns: []*v1.TaskRun{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "task2", + }, + Status: v1.TaskRunStatus{ + Status: duckv1.Status{ + Conditions: duckv1.Conditions{ + { + Type: apis.ConditionSucceeded, + Status: corev1.ConditionFalse, + }, + }, + }, + }, + Spec: v1.TaskRunSpec{TaskSpec: &v1.TaskSpec{}}, + }, + }, + }, + }, + TasksGraph: &dag.Graph{ + Nodes: map[string]*dag.Node{ + "task1": { + Key: "task1", + Next: []*dag.Node{ + { + Key: "task2", + }, + }, + }, + "task2": { + Key: "task2", + Prev: []*dag.Node{ + { + Key: "task1", + }, + }, + }, + }, + }, + FinalTasksGraph: &dag.Graph{ + Nodes: map[string]*dag.Node{}, + }, + }, + want: &resources.PipelineRunFacts{ + State: resources.PipelineRunState{ + { + TaskRunNames: []string{"task1"}, + ResolvedTask: &taskresources.ResolvedTask{ + TaskName: "task1", + TaskSpec: &v1.TaskSpec{ + Steps: []v1.Step{{Name: "step1"}}, + }, + }, + PipelineTask: &v1.PipelineTask{ + Name: "task1", + }, + TaskRuns: []*v1.TaskRun{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "task1", + }, + Status: v1.TaskRunStatus{ + Status: duckv1.Status{ + Conditions: duckv1.Conditions{ + { + Type: apis.ConditionSucceeded, + Status: corev1.ConditionTrue, + }, + }, + }, + TaskRunStatusFields: v1.TaskRunStatusFields{ + Results: []v1.TaskRunResult{ + { + Name: "foo", + Type: v1.ResultsTypeString, + Value: v1.ResultValue{ + StringVal: "bar", + }, + }, + }, + }, + }, + Spec: v1.TaskRunSpec{TaskSpec: &v1.TaskSpec{}}, + }, + }, + }, + { + TaskRunNames: []string{"task2"}, + ResolvedTask: &taskresources.ResolvedTask{ + TaskName: "task2", + TaskSpec: &v1.TaskSpec{ + Steps: []v1.Step{ + { + Name: "$(tasks.task1.results.foo)", + }, + }, + }, + }, + PipelineTask: &v1.PipelineTask{ + Name: "task2", + }, + TaskRuns: []*v1.TaskRun{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "task2", + }, + Status: v1.TaskRunStatus{ + Status: duckv1.Status{ + Conditions: duckv1.Conditions{ + { + Type: apis.ConditionSucceeded, + Status: corev1.ConditionFalse, + }, + }, + }, + }, + Spec: v1.TaskRunSpec{TaskSpec: &v1.TaskSpec{}}, + }, + }, + }, + }, + TasksGraph: &dag.Graph{ + Nodes: map[string]*dag.Node{ + "task1": { + Key: "task1", + Next: []*dag.Node{ + { + Key: "task2", + }, + }, + }, + "task2": { + Key: "task2", + Prev: []*dag.Node{ + { + Key: "task1", + }, + }, + }, + }, + }, + FinalTasksGraph: &dag.Graph{ + Nodes: map[string]*dag.Node{}, + }, + SkipCache: map[string]resources.TaskSkipStatus{ + "task1": {SkippingReason: "None"}, + "task2": {SkippingReason: "None"}, + }, + }, + wantErr: false, + }, + } { + t.Run(tc.name, func(t *testing.T) { + ctx, _ := ttesting.SetupFakeContext(t) + testAssets, cancel := getPipelineRunController(t, test.Data{ + PipelineRuns: []*v1.PipelineRun{tc.pr}, + }) + defer cancel() + c := &Reconciler{ + Clock: clock.NewFakePassiveClock(time.Now()), + KubeClientSet: testAssets.Clients.Kube, + PipelineClientSet: testAssets.Clients.Pipeline, + tracerProvider: tracing.New("pipelinerun"), + } + err := c.runNextSchedulableTask(ctx, tc.pr, tc.pipelineRunFacts) + if (err != nil) != tc.wantErr { + t.Errorf("runNextSchedulableTask() error = %v, wantErr %v", err, tc.wantErr) + } + if diff := cmp.Diff(tc.want, tc.pipelineRunFacts); diff != "" { + t.Errorf("runNextSchedulableTask() got unexpected pipelineRunFacts diff %s", diff) + } + }) + } +} + func getSignedV1Pipeline(unsigned *pipelinev1.Pipeline, signer signature.Signer, name string) (*pipelinev1.Pipeline, error) { signed := unsigned.DeepCopy() signed.Name = name diff --git a/pkg/reconciler/pipelinerun/resources/apply.go b/pkg/reconciler/pipelinerun/resources/apply.go index 7f1a9057900..936a28939be 100644 --- a/pkg/reconciler/pipelinerun/resources/apply.go +++ b/pkg/reconciler/pipelinerun/resources/apply.go @@ -369,6 +369,30 @@ func propagateParams(t v1.PipelineTask, stringReplacements map[string]string, ar return t } +// PropagateResults propagate the result of the completed task to the unfinished task that is not explicitly specify in the params +func PropagateResults(rpt *ResolvedPipelineTask, runStates PipelineRunState) { + if rpt.ResolvedTask == nil || rpt.ResolvedTask.TaskSpec == nil { + return + } + stringReplacements := map[string]string{} + arrayReplacements := map[string][]string{} + for taskName, taskResults := range runStates.GetTaskRunsResults() { + for _, res := range taskResults { + switch res.Type { + case v1.ResultsTypeString: + stringReplacements[fmt.Sprintf("tasks.%s.results.%s", taskName, res.Name)] = res.Value.StringVal + case v1.ResultsTypeArray: + arrayReplacements[fmt.Sprintf("tasks.%s.results.%s", taskName, res.Name)] = res.Value.ArrayVal + case v1.ResultsTypeObject: + for k, v := range res.Value.ObjectVal { + stringReplacements[fmt.Sprintf("tasks.%s.results.%s.%s", taskName, res.Name, k)] = v + } + } + } + } + rpt.ResolvedTask.TaskSpec = resources.ApplyReplacements(rpt.ResolvedTask.TaskSpec, stringReplacements, arrayReplacements) +} + // ApplyTaskResultsToPipelineResults applies the results of completed TasksRuns and Runs to a Pipeline's // list of PipelineResults, returning the computed set of PipelineRunResults. References to // non-existent TaskResults or failed TaskRuns or Runs result in a PipelineResult being considered invalid diff --git a/pkg/reconciler/pipelinerun/resources/apply_test.go b/pkg/reconciler/pipelinerun/resources/apply_test.go index 2e0d57dc62a..995404fd01b 100644 --- a/pkg/reconciler/pipelinerun/resources/apply_test.go +++ b/pkg/reconciler/pipelinerun/resources/apply_test.go @@ -26,9 +26,13 @@ import ( v1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" resources "github.com/tektoncd/pipeline/pkg/reconciler/pipelinerun/resources" + taskresources "github.com/tektoncd/pipeline/pkg/reconciler/taskrun/resources" "github.com/tektoncd/pipeline/test/diff" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/selection" + "knative.dev/pkg/apis" + duckv1 "knative.dev/pkg/apis/duck/v1" ) func TestApplyParameters(t *testing.T) { @@ -4251,3 +4255,319 @@ func TestApplyTaskRunContext(t *testing.T) { t.Fatalf("ApplyTaskRunContext() %s", diff.PrintWantGot(d)) } } + +func TestPropagateResults(t *testing.T) { + for _, tt := range []struct { + name string + resolvedTask *resources.ResolvedPipelineTask + runStates resources.PipelineRunState + expectedResolvedTask *resources.ResolvedPipelineTask + }{ + { + name: "propagate string result", + resolvedTask: &resources.ResolvedPipelineTask{ + ResolvedTask: &taskresources.ResolvedTask{ + TaskSpec: &v1.TaskSpec{ + Steps: []v1.Step{ + { + Name: "$(tasks.pt1.results.r1)", + Command: []string{"$(tasks.pt1.results.r2)"}, + Args: []string{"$(tasks.pt2.results.r1)"}, + }, + }, + }, + }, + }, + runStates: resources.PipelineRunState{ + { + PipelineTask: &v1.PipelineTask{ + Name: "pt1", + }, + TaskRuns: []*v1.TaskRun{ + { + Status: v1.TaskRunStatus{ + Status: duckv1.Status{ + Conditions: duckv1.Conditions{ + { + Type: apis.ConditionSucceeded, + Status: corev1.ConditionTrue, + }, + }, + }, + TaskRunStatusFields: v1.TaskRunStatusFields{ + Results: []v1.TaskRunResult{ + { + Name: "r1", + Type: v1.ResultsTypeString, + Value: v1.ResultValue{ + StringVal: "step1", + }, + }, + { + Name: "r2", + Type: v1.ResultsTypeString, + Value: v1.ResultValue{ + StringVal: "echo", + }, + }, + }, + }, + }, + }, + }, + }, { + PipelineTask: &v1.PipelineTask{ + Name: "pt2", + }, + TaskRuns: []*v1.TaskRun{ + { + Status: v1.TaskRunStatus{ + Status: duckv1.Status{ + Conditions: duckv1.Conditions{ + { + Type: apis.ConditionSucceeded, + Status: corev1.ConditionTrue, + }, + }, + }, + TaskRunStatusFields: v1.TaskRunStatusFields{ + Results: []v1.TaskRunResult{ + { + Name: "r1", + Type: v1.ResultsTypeString, + Value: v1.ResultValue{ + StringVal: "arg1", + }, + }, + }, + }, + }, + }, + }, + }, + }, + expectedResolvedTask: &resources.ResolvedPipelineTask{ + ResolvedTask: &taskresources.ResolvedTask{ + TaskSpec: &v1.TaskSpec{ + Steps: []v1.Step{ + { + Name: "step1", + Command: []string{"echo"}, + Args: []string{"arg1"}, + }, + }, + }, + }, + }, + }, { + name: "propagate array result", + resolvedTask: &resources.ResolvedPipelineTask{ + ResolvedTask: &taskresources.ResolvedTask{ + TaskSpec: &v1.TaskSpec{ + Steps: []v1.Step{ + { + Command: []string{"$(tasks.pt1.results.r1[*])"}, + Args: []string{"$(tasks.pt2.results.r1[*])"}, + }, + }, + }, + }, + }, + runStates: resources.PipelineRunState{ + { + PipelineTask: &v1.PipelineTask{ + Name: "pt1", + }, + TaskRuns: []*v1.TaskRun{ + { + Status: v1.TaskRunStatus{ + Status: duckv1.Status{ + Conditions: duckv1.Conditions{ + { + Type: apis.ConditionSucceeded, + Status: corev1.ConditionTrue, + }, + }, + }, + TaskRunStatusFields: v1.TaskRunStatusFields{ + Results: []v1.TaskRunResult{ + { + Name: "r1", + Type: v1.ResultsTypeArray, + Value: v1.ResultValue{ + ArrayVal: []string{"bash", "-c"}, + }, + }, + }, + }, + }, + }, + }, + }, { + PipelineTask: &v1.PipelineTask{ + Name: "pt2", + }, + TaskRuns: []*v1.TaskRun{ + { + Status: v1.TaskRunStatus{ + Status: duckv1.Status{ + Conditions: duckv1.Conditions{ + { + Type: apis.ConditionSucceeded, + Status: corev1.ConditionTrue, + }, + }, + }, + TaskRunStatusFields: v1.TaskRunStatusFields{ + Results: []v1.TaskRunResult{ + { + Name: "r1", + Type: v1.ResultsTypeArray, + Value: v1.ResultValue{ + ArrayVal: []string{"echo", "arg1"}, + }, + }, + }, + }, + }, + }, + }, + }, + }, + expectedResolvedTask: &resources.ResolvedPipelineTask{ + ResolvedTask: &taskresources.ResolvedTask{ + TaskSpec: &v1.TaskSpec{ + Steps: []v1.Step{ + { + Command: []string{"bash", "-c"}, + Args: []string{"echo", "arg1"}, + }, + }, + }, + }, + }, + }, { + name: "propagate object result", + resolvedTask: &resources.ResolvedPipelineTask{ + ResolvedTask: &taskresources.ResolvedTask{ + TaskSpec: &v1.TaskSpec{ + Steps: []v1.Step{ + { + Command: []string{"$(tasks.pt1.results.r1.command1)", "$(tasks.pt1.results.r1.command2)"}, + Args: []string{"$(tasks.pt2.results.r1.arg1)", "$(tasks.pt2.results.r1.arg2)"}, + }, + }, + }, + }, + }, + runStates: resources.PipelineRunState{ + { + PipelineTask: &v1.PipelineTask{ + Name: "pt1", + }, + TaskRuns: []*v1.TaskRun{ + { + Status: v1.TaskRunStatus{ + Status: duckv1.Status{ + Conditions: duckv1.Conditions{ + { + Type: apis.ConditionSucceeded, + Status: corev1.ConditionTrue, + }, + }, + }, + TaskRunStatusFields: v1.TaskRunStatusFields{ + Results: []v1.TaskRunResult{ + { + Name: "r1", + Type: v1.ResultsTypeObject, + Value: v1.ResultValue{ + ObjectVal: map[string]string{ + "command1": "bash", + "command2": "-c", + }, + }, + }, + }, + }, + }, + }, + }, + }, { + PipelineTask: &v1.PipelineTask{ + Name: "pt2", + }, + TaskRuns: []*v1.TaskRun{ + { + Status: v1.TaskRunStatus{ + Status: duckv1.Status{ + Conditions: duckv1.Conditions{ + { + Type: apis.ConditionSucceeded, + Status: corev1.ConditionTrue, + }, + }, + }, + TaskRunStatusFields: v1.TaskRunStatusFields{ + Results: []v1.TaskRunResult{ + { + Name: "r1", + Type: v1.ResultsTypeObject, + Value: v1.ResultValue{ + ObjectVal: map[string]string{ + "arg1": "echo", + "arg2": "arg1", + }, + }, + }, + }, + }, + }, + }, + }, + }, + }, + expectedResolvedTask: &resources.ResolvedPipelineTask{ + ResolvedTask: &taskresources.ResolvedTask{ + TaskSpec: &v1.TaskSpec{ + Steps: []v1.Step{ + { + Command: []string{"bash", "-c"}, + Args: []string{"echo", "arg1"}, + }, + }, + }, + }, + }, + }, { + name: "not propagate result when resolved task is nil", + resolvedTask: &resources.ResolvedPipelineTask{ + ResolvedTask: nil, + }, + runStates: resources.PipelineRunState{}, + expectedResolvedTask: &resources.ResolvedPipelineTask{ + ResolvedTask: nil, + }, + }, { + name: "not propagate result when taskSpec is nil", + resolvedTask: &resources.ResolvedPipelineTask{ + ResolvedTask: &taskresources.ResolvedTask{ + TaskSpec: nil, + }, + }, + runStates: resources.PipelineRunState{}, + expectedResolvedTask: &resources.ResolvedPipelineTask{ + ResolvedTask: &taskresources.ResolvedTask{ + TaskSpec: nil, + }, + }, + }, + } { + t.Run(tt.name, func(t *testing.T) { + resources.PropagateResults(tt.resolvedTask, tt.runStates) + if d := cmp.Diff(tt.expectedResolvedTask, tt.resolvedTask); d != "" { + t.Fatalf("PropagateResults() %s", diff.PrintWantGot(d)) + } + }) + } +} diff --git a/test/propagated_results_test.go b/test/propagated_results_test.go new file mode 100644 index 00000000000..6af07542851 --- /dev/null +++ b/test/propagated_results_test.go @@ -0,0 +1,482 @@ +//go:build e2e +// +build e2e + +/* +Copyright 2023 The Tekton Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package test + +import ( + "context" + "fmt" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + v1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1" + "github.com/tektoncd/pipeline/test/parse" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + knativetest "knative.dev/pkg/test" +) + +func TestPropagatedResults(t *testing.T) { + t.Parallel() + + ignorePipelineRunStatusFields := cmpopts.IgnoreFields(v1.PipelineRunStatusFields{}, "Provenance") + ignoreTaskRunStatus := cmpopts.IgnoreFields(v1.TaskRunStatusFields{}, "StartTime", "CompletionTime", "Sidecars", "Provenance") + requireAlphaFeatureFlag = requireAnyGate(map[string]string{ + "enable-api-fields": "alpha"}) + type tests struct { + name string + pipelineName string + pipelineRunFunc func(*testing.T, string) (*v1.PipelineRun, *v1.PipelineRun, []*v1.TaskRun) + } + + tds := []tests{{ + name: "propagated all type results", + pipelineName: "propagated-all-type-results", + pipelineRunFunc: getPropagatedResultPipelineRun, + }} + + for _, td := range tds { + td := td + t.Run(td.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + ctx, cancel := context.WithCancel(ctx) + defer cancel() + + c, namespace := setup(ctx, t, requireAlphaFeatureFlag) + + knativetest.CleanupOnInterrupt(func() { tearDown(ctx, t, c, namespace) }, t.Logf) + defer tearDown(ctx, t, c, namespace) + + t.Logf("Setting up test resources for %q test in namespace %s", td.name, namespace) + pipelineRun, expectedResolvedPipelineRun, expectedTaskRuns := td.pipelineRunFunc(t, namespace) + + prName := pipelineRun.Name + _, err := c.V1PipelineRunClient.Create(ctx, pipelineRun, metav1.CreateOptions{}) + if err != nil { + t.Fatalf("Failed to create PipelineRun `%s`: %s", prName, err) + } + + t.Logf("Waiting for PipelineRun %s in namespace %s to complete", prName, namespace) + if err := WaitForPipelineRunState(ctx, c, prName, timeout, PipelineRunSucceed(prName), "PipelineRunSuccess", v1Version); err != nil { + t.Fatalf("Error waiting for PipelineRun %s to finish: %s", prName, err) + } + cl, _ := c.V1PipelineRunClient.Get(ctx, prName, metav1.GetOptions{}) + d := cmp.Diff(expectedResolvedPipelineRun, cl, + ignoreTypeMeta, + ignoreObjectMeta, + ignoreCondition, + ignorePipelineRunStatus, + ignoreTaskRunStatus, + ignoreConditions, + ignoreContainerStates, + ignoreStepState, + ignoreSAPipelineRunSpec, + ignorePipelineRunStatusFields, + ) + if d != "" { + t.Fatalf(`The resolved spec does not match the expected spec. Here is the diff: %v`, d) + } + for _, tr := range expectedTaskRuns { + t.Logf("Checking Taskrun %s", tr.Name) + taskrun, _ := c.V1TaskRunClient.Get(ctx, tr.Name, metav1.GetOptions{}) + d = cmp.Diff(tr, taskrun, + ignoreTypeMeta, + ignoreObjectMeta, + ignoreCondition, + ignoreTaskRunStatus, + ignoreConditions, + ignoreContainerStates, + ignoreStepState, + ignoreSATaskRunSpec, + ) + if d != "" { + t.Fatalf(`The expected taskrun does not match created taskrun. Here is the diff: %v`, d) + } + } + t.Logf("Successfully finished test %q", td.name) + }) + } +} + +func getPropagatedResultPipelineRun(t *testing.T, namespace string) (*v1.PipelineRun, *v1.PipelineRun, []*v1.TaskRun) { + t.Helper() + pipelineRun := parse.MustParseV1PipelineRun(t, fmt.Sprintf(` +metadata: + name: propagated-all-type-results + namespace: %s +spec: + pipelineSpec: + tasks: + - name: make-uid + taskSpec: + results: + - name: strUid + type: string + - name: arrayUid + type: array + - name: mapUid + type: object + properties: + uid: { + type: string + } + steps: + - name: add-str-uid + image: busybox + command: ["/bin/sh", "-c"] + args: + - echo "1001" | tee $(results.strUid.path) + - name: add-array-uid + image: busybox + command: ["/bin/sh", "-c"] + args: + - echo "[\"1002\", \"1003\"]" | tee $(results.arrayUid.path) + - name: add-map-uid + image: busybox + command: ["/bin/sh", "-c"] + args: + - echo -n "{\"uid\":\"1004\"}" | tee $(results.mapUid.path) + - name: show-uid + taskSpec: + steps: + - name: show-str-uid + image: busybox + command: ["/bin/sh", "-c"] + args: + - echo + - $(tasks.make-uid.results.strUid) + - name: show-array-uid + image: busybox + command: ["/bin/sh", "-c"] + args: + - echo + - $(tasks.make-uid.results.arrayUid[*]) + - name: show-map-uid + image: busybox + command: ["/bin/sh", "-c"] + args: + - echo + - $(tasks.make-uid.results.mapUid.uid) +`, namespace)) + expectedPipelineRun := parse.MustParseV1PipelineRun(t, fmt.Sprintf(` +metadata: + name: propagated-all-type-results + namespace: %s +spec: + pipelineSpec: + tasks: + - name: make-uid + taskSpec: + results: + - name: strUid + type: string + - name: arrayUid + type: array + - name: mapUid + properties: + uid: + type: string + type: object + steps: + - args: + - echo "1001" | tee $(results.strUid.path) + command: + - /bin/sh + - -c + image: busybox + name: add-str-uid + - args: + - echo "[\"1002\", \"1003\"]" | tee $(results.arrayUid.path) + command: + - /bin/sh + - -c + image: busybox + name: add-array-uid + - args: + - echo -n "{\"uid\":\"1004\"}" | tee $(results.mapUid.path) + command: + - /bin/sh + - -c + image: busybox + name: add-map-uid + - name: show-uid + taskSpec: + steps: + - args: + - echo + - $(tasks.make-uid.results.strUid) + command: + - /bin/sh + - -c + image: busybox + name: show-str-uid + - args: + - echo + - $(tasks.make-uid.results.arrayUid[*]) + command: + - /bin/sh + - -c + image: busybox + name: show-array-uid + - args: + - echo + - $(tasks.make-uid.results.mapUid.uid) + command: + - /bin/sh + - -c + image: busybox + name: show-map-uid + timeouts: + pipeline: 1h0m0s +status: + pipelineSpec: + tasks: + - name: make-uid + taskSpec: + results: + - name: strUid + type: string + - name: arrayUid + type: array + - name: mapUid + properties: + uid: + type: string + type: object + steps: + - args: + - echo "1001" | tee $(results.strUid.path) + command: + - /bin/sh + - -c + image: busybox + name: add-str-uid + - args: + - echo "[\"1002\", \"1003\"]" | tee $(results.arrayUid.path) + command: + - /bin/sh + - -c + image: busybox + name: add-array-uid + - args: + - echo -n "{\"uid\":\"1004\"}" | tee $(results.mapUid.path) + command: + - /bin/sh + - -c + image: busybox + name: add-map-uid + - name: show-uid + taskSpec: + steps: + - args: + - echo + - $(tasks.make-uid.results.strUid) + command: + - /bin/sh + - -c + image: busybox + name: show-str-uid + - args: + - echo + - $(tasks.make-uid.results.arrayUid[*]) + command: + - /bin/sh + - -c + image: busybox + name: show-array-uid + - args: + - echo + - $(tasks.make-uid.results.mapUid.uid) + command: + - /bin/sh + - -c + image: busybox + name: show-map-uid +`, namespace)) + makeUidTaskRun := parse.MustParseV1TaskRun(t, fmt.Sprintf(` +metadata: + name: propagated-all-type-results-make-uid + namespace: %s +spec: + taskSpec: + results: + - name: strUid + type: string + - name: arrayUid + type: array + - name: mapUid + properties: + uid: + type: string + type: object + steps: + - args: + - echo "1001" | tee $(results.strUid.path) + command: + - /bin/sh + - -c + image: busybox + name: add-str-uid + - args: + - echo "[\"1002\", \"1003\"]" | tee $(results.arrayUid.path) + command: + - /bin/sh + - -c + image: busybox + name: add-array-uid + - args: + - echo -n "{\"uid\":\"1004\"}" | tee $(results.mapUid.path) + command: + - /bin/sh + - -c + image: busybox + name: add-map-uid + timeout: 1h0m0s +status: + podName: propagated-all-type-results-make-uid-pod + results: + - name: strUid + type: string + value: | + 1001 + - name: arrayUid + type: array + value: + - "1002" + - "1003" + - name: mapUid + type: object + value: + uid: "1004" + steps: + - container: step-add-str-uid + name: add-str-uid + - container: step-add-array-uid + name: add-array-uid + - container: step-add-map-uid + name: add-map-uid + taskSpec: + results: + - name: strUid + type: string + - name: arrayUid + type: array + - name: mapUid + properties: + uid: + type: string + type: object + steps: + - args: + - echo "1001" | tee /tekton/results/strUid + command: + - /bin/sh + - -c + image: busybox + name: add-str-uid + - args: + - echo "[\"1002\", \"1003\"]" | tee /tekton/results/arrayUid + command: + - /bin/sh + - -c + image: busybox + name: add-array-uid + - args: + - echo -n "{\"uid\":\"1004\"}" | tee /tekton/results/mapUid + command: + - /bin/sh + - -c + image: busybox + name: add-map-uid +`, namespace)) + showUidTaskRun := parse.MustParseV1TaskRun(t, fmt.Sprintf(` +metadata: + name: propagated-all-type-results-show-uid + namespace: %s +spec: + taskSpec: + steps: + - args: + - echo + - | + 1001 + command: + - /bin/sh + - -c + image: busybox + name: show-str-uid + - args: + - echo + - "1002" + - "1003" + command: + - /bin/sh + - -c + image: busybox + name: show-array-uid + - args: + - echo + - "1004" + command: + - /bin/sh + - -c + image: busybox + name: show-map-uid + timeout: 1h0m0s +status: + podName: propagated-all-type-results-show-uid-pod + steps: + - container: step-show-str-uid + name: show-str-uid + - container: step-show-array-uid + name: show-array-uid + - container: step-show-map-uid + name: show-map-uid + taskSpec: + steps: + - args: + - echo + - | + 1001 + command: + - /bin/sh + - -c + image: busybox + name: show-str-uid + - args: + - echo + - "1002" + - "1003" + command: + - /bin/sh + - -c + image: busybox + name: show-array-uid + - args: + - echo + - "1004" + command: + - /bin/sh + - -c + image: busybox + name: show-map-uid +`, namespace)) + return pipelineRun, expectedPipelineRun, []*v1.TaskRun{makeUidTaskRun, showUidTaskRun} +} From 40268768b68cefca6682d9e8bb6439d9d511337e Mon Sep 17 00:00:00 2001 From: Yongxuan Zhang Date: Thu, 19 Oct 2023 19:14:14 +0000 Subject: [PATCH 4/7] [TEP-0145] Add CEL field to WhenExpression, and feature flag to guard the field add cel to the WhenExpression, a feature flag enable-cel-in-whenexpression to guard thie new api field. Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com --- config/config-feature-flags.yaml | 3 + docs/additional-configs.md | 1 + docs/pipeline-api.md | 28 ++++++++ docs/pipelines.md | 10 ++- pkg/apis/config/feature_flags.go | 9 ++- pkg/apis/config/feature_flags_test.go | 4 ++ .../testdata/feature-flags-all-flags-set.yaml | 1 + ...-invalid-enable-cel-in-whenexpression.yaml | 21 ++++++ pkg/apis/pipeline/v1/openapi_generated.go | 10 ++- pkg/apis/pipeline/v1/pipeline_validation.go | 8 +-- pkg/apis/pipeline/v1/swagger.json | 15 ++--- pkg/apis/pipeline/v1/when_types.go | 12 +++- pkg/apis/pipeline/v1/when_validation.go | 22 +++++-- pkg/apis/pipeline/v1/when_validation_test.go | 65 ++++++++++++++++++- .../pipeline/v1beta1/openapi_generated.go | 10 ++- .../pipeline/v1beta1/pipeline_conversion.go | 2 + .../v1beta1/pipeline_conversion_test.go | 3 + .../pipeline/v1beta1/pipeline_validation.go | 8 +-- pkg/apis/pipeline/v1beta1/swagger.json | 15 ++--- pkg/apis/pipeline/v1beta1/when_types.go | 12 +++- pkg/apis/pipeline/v1beta1/when_validation.go | 22 +++++-- .../pipeline/v1beta1/when_validation_test.go | 65 ++++++++++++++++++- 22 files changed, 292 insertions(+), 54 deletions(-) create mode 100644 pkg/apis/config/testdata/feature-flags-invalid-enable-cel-in-whenexpression.yaml diff --git a/config/config-feature-flags.yaml b/config/config-feature-flags.yaml index af73d4a005a..50eaa68519b 100644 --- a/config/config-feature-flags.yaml +++ b/config/config-feature-flags.yaml @@ -121,3 +121,6 @@ data: # Setting this flag to "true" will keep pod on cancellation # allowing examination of the logs on the pods from cancelled taskruns keep-pod-on-cancel: "false" + # Setting this flag to "true" will enable the CEL evaluation in WhenExpression + # This feature is in preview mode and not implemented yet. Please check #7244 for the updates. + enable-cel-in-whenexpression: "false" diff --git a/docs/additional-configs.md b/docs/additional-configs.md index a4a5150e88e..a5a5a796d94 100644 --- a/docs/additional-configs.md +++ b/docs/additional-configs.md @@ -316,6 +316,7 @@ Features currently in "alpha" are: | [Configure Default Resolver](./resolution.md#configuring-built-in-resolvers) | [TEP-0133](https://github.com/tektoncd/community/blob/main/teps/0133-configure-default-resolver.md) | N/A | | | [Coschedule](./affinityassistants.md) | [TEP-0135](https://github.com/tektoncd/community/blob/main/teps/0135-coscheduling-pipelinerun-pods.md) | N/A |`coschedule` | | [keep pod on cancel](./taskruns.md#cancelling-a-taskrun) | N/A | v0.52 | keep-pod-on-cancel | +| [CEL in WhenExpression](./taskruns.md#cancelling-a-taskrun) | [TEP-0145](https://github.com/tektoncd/community/blob/main/teps/0145-cel-in-whenexpression.md) | N/A | enable-cel-in-whenexpression | ### Beta Features diff --git a/docs/pipeline-api.md b/docs/pipeline-api.md index 573745f4a61..9b73b9e7e40 100644 --- a/docs/pipeline-api.md +++ b/docs/pipeline-api.md @@ -5782,6 +5782,20 @@ k8s.io/apimachinery/pkg/selection.Operator It must be non-empty

+ + +cel
+ +string + + + +(Optional) +

CEL is a string of Common Language Expression, which can be used to conditionally execute +the task based on the result of the expression evaluation +More info about CEL syntax: https://github.com/google/cel-spec/blob/master/doc/langdef.md

+ +

WhenExpressions @@ -14549,6 +14563,20 @@ k8s.io/apimachinery/pkg/selection.Operator It must be non-empty

+ + +cel
+ +string + + + +(Optional) +

CEL is a string of Common Language Expression, which can be used to conditionally execute +the task based on the result of the expression evaluation +More info about CEL syntax: https://github.com/google/cel-spec/blob/master/doc/langdef.md

+ +

WhenExpressions diff --git a/docs/pipelines.md b/docs/pipelines.md index da68e774781..300835e9ddf 100644 --- a/docs/pipelines.md +++ b/docs/pipelines.md @@ -705,7 +705,7 @@ If the result is **NOT** initialized before failing, and there is a `PipelineTas ``` - If the consuming `PipelineTask` has `OnError:stopAndFail`, the `PipelineRun` will fail with `InvalidTaskResultReference`. -- If the consuming `PipelineTask` has `OnError:continue`, the consuming `PipelineTask` will be skipped with reason `Results were missing`, +- If the consuming `PipelineTask` has `OnError:continue`, the consuming `PipelineTask` will be skipped with reason `Results were missing`, and the `PipelineRun` will continue to execute. ### Guard `Task` execution using `when` expressions @@ -775,6 +775,14 @@ There are a lot of scenarios where `when` expressions can be really useful. Some - Checking if the name of a CI job matches - Checking if an optional Workspace has been provided +#### Use CEL expression in WhenExpression + +> :seedling: **`CEL in WhenExpression` is an [alpha](install.md#alpha-features) feature.** +> The `enable-cel-in-whenexpression` feature flag must be set to `"true"` to enable the use of `CEL` in `WhenExpression`. +> +> :warning: This feature is in a preview mode. +> It is still in a very early stage of development and is not yet fully functional + #### Guarding a `Task` and its dependent `Tasks` To guard a `Task` and its dependent Tasks: diff --git a/pkg/apis/config/feature_flags.go b/pkg/apis/config/feature_flags.go index 5ad99f228ee..6b4940c0880 100644 --- a/pkg/apis/config/feature_flags.go +++ b/pkg/apis/config/feature_flags.go @@ -92,6 +92,10 @@ const ( KeepPodOnCancel = "keep-pod-on-cancel" // DefaultEnableKeepPodOnCancel is the default value for "keep-pod-on-cancel" DefaultEnableKeepPodOnCancel = false + // EnableCELInWhenExpression is the flag to enabled CEL in WhenExpression + EnableCELInWhenExpression = "enable-cel-in-whenexpression" + // DefaultEnableCELInWhenExpression is the default value for EnableCELInWhenExpression + DefaultEnableCELInWhenExpression = false disableAffinityAssistantKey = "disable-affinity-assistant" disableCredsInitKey = "disable-creds-init" @@ -140,6 +144,7 @@ type FeatureFlags struct { MaxResultSize int SetSecurityContext bool Coschedule string + EnableCELInWhenExpression bool } // GetFeatureFlagsConfigName returns the name of the configmap containing all @@ -209,10 +214,12 @@ func NewFeatureFlagsFromMap(cfgMap map[string]string) (*FeatureFlags, error) { if err := setFeature(setSecurityContextKey, DefaultSetSecurityContext, &tc.SetSecurityContext); err != nil { return nil, err } - if err := setCoschedule(cfgMap, DefaultCoschedule, tc.DisableAffinityAssistant, &tc.Coschedule); err != nil { return nil, err } + if err := setFeature(EnableCELInWhenExpression, DefaultEnableCELInWhenExpression, &tc.EnableCELInWhenExpression); err != nil { + return nil, err + } // Given that they are alpha features, Tekton Bundles and Custom Tasks should be switched on if // enable-api-fields is "alpha". If enable-api-fields is not "alpha" then fall back to the value of // each feature's individual flag. diff --git a/pkg/apis/config/feature_flags_test.go b/pkg/apis/config/feature_flags_test.go index a6f4e60bb1a..65756040698 100644 --- a/pkg/apis/config/feature_flags_test.go +++ b/pkg/apis/config/feature_flags_test.go @@ -73,6 +73,7 @@ func TestNewFeatureFlagsFromConfigMap(t *testing.T) { MaxResultSize: 4096, SetSecurityContext: true, Coschedule: config.CoscheduleDisabled, + EnableCELInWhenExpression: true, }, fileName: "feature-flags-all-flags-set", }, @@ -269,6 +270,9 @@ func TestNewFeatureFlagsConfigMapErrors(t *testing.T) { }, { fileName: "feature-flags-invalid-disable-affinity-assistant", want: `failed parsing feature flags config "truee": strconv.ParseBool: parsing "truee": invalid syntax`, + }, { + fileName: "feature-flags-invalid-enable-cel-in-whenexpression", + want: `failed parsing feature flags config "invalid": strconv.ParseBool: parsing "invalid": invalid syntax`, }} { t.Run(tc.fileName, func(t *testing.T) { cm := test.ConfigMapFromTestFile(t, tc.fileName) diff --git a/pkg/apis/config/testdata/feature-flags-all-flags-set.yaml b/pkg/apis/config/testdata/feature-flags-all-flags-set.yaml index 9e01abd1944..63015ec07c1 100644 --- a/pkg/apis/config/testdata/feature-flags-all-flags-set.yaml +++ b/pkg/apis/config/testdata/feature-flags-all-flags-set.yaml @@ -32,3 +32,4 @@ data: enable-provenance-in-status: "false" set-security-context: "true" keep-pod-on-cancel: "true" + enable-cel-in-whenexpression: "true" diff --git a/pkg/apis/config/testdata/feature-flags-invalid-enable-cel-in-whenexpression.yaml b/pkg/apis/config/testdata/feature-flags-invalid-enable-cel-in-whenexpression.yaml new file mode 100644 index 00000000000..fca3049351f --- /dev/null +++ b/pkg/apis/config/testdata/feature-flags-invalid-enable-cel-in-whenexpression.yaml @@ -0,0 +1,21 @@ +# Copyright 2023 The Tekton Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# https://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +apiVersion: v1 +kind: ConfigMap +metadata: + name: feature-flags + namespace: tekton-pipelines +data: + enable-cel-in-whenexpression: "invalid" diff --git a/pkg/apis/pipeline/v1/openapi_generated.go b/pkg/apis/pipeline/v1/openapi_generated.go index b5a14a692f1..6425ec1de3b 100644 --- a/pkg/apis/pipeline/v1/openapi_generated.go +++ b/pkg/apis/pipeline/v1/openapi_generated.go @@ -4289,7 +4289,6 @@ func schema_pkg_apis_pipeline_v1_WhenExpression(ref common.ReferenceCallback) co "input": { SchemaProps: spec.SchemaProps{ Description: "Input is the string for guard checking which can be a static input or an output from a parent Task", - Default: "", Type: []string{"string"}, Format: "", }, @@ -4297,7 +4296,6 @@ func schema_pkg_apis_pipeline_v1_WhenExpression(ref common.ReferenceCallback) co "operator": { SchemaProps: spec.SchemaProps{ Description: "Operator that represents an Input's relationship to the values", - Default: "", Type: []string{"string"}, Format: "", }, @@ -4322,8 +4320,14 @@ func schema_pkg_apis_pipeline_v1_WhenExpression(ref common.ReferenceCallback) co }, }, }, + "cel": { + SchemaProps: spec.SchemaProps{ + Description: "CEL is a string of Common Language Expression, which can be used to conditionally execute the task based on the result of the expression evaluation More info about CEL syntax: https://github.com/google/cel-spec/blob/master/doc/langdef.md", + Type: []string{"string"}, + Format: "", + }, + }, }, - Required: []string{"input", "operator", "values"}, }, }, } diff --git a/pkg/apis/pipeline/v1/pipeline_validation.go b/pkg/apis/pipeline/v1/pipeline_validation.go index a5cea7d8760..3067303e87c 100644 --- a/pkg/apis/pipeline/v1/pipeline_validation.go +++ b/pkg/apis/pipeline/v1/pipeline_validation.go @@ -88,7 +88,7 @@ func (ps *PipelineSpec) Validate(ctx context.Context) (errs *apis.FieldError) { errs = errs.Also(validatePipelineResults(ps.Results, ps.Tasks, ps.Finally)) errs = errs.Also(validateTasksAndFinallySection(ps)) errs = errs.Also(validateFinalTasks(ps.Tasks, ps.Finally)) - errs = errs.Also(validateWhenExpressions(ps.Tasks, ps.Finally)) + errs = errs.Also(validateWhenExpressions(ctx, ps.Tasks, ps.Finally)) errs = errs.Also(validateMatrix(ctx, ps.Tasks).ViaField("tasks")) errs = errs.Also(validateMatrix(ctx, ps.Finally).ViaField("finally")) return errs @@ -761,12 +761,12 @@ func validateResultsVariablesExpressionsInFinally(expressions []string, pipeline return errs } -func validateWhenExpressions(tasks []PipelineTask, finalTasks []PipelineTask) (errs *apis.FieldError) { +func validateWhenExpressions(ctx context.Context, tasks []PipelineTask, finalTasks []PipelineTask) (errs *apis.FieldError) { for i, t := range tasks { - errs = errs.Also(t.When.validate().ViaFieldIndex("tasks", i)) + errs = errs.Also(t.When.validate(ctx).ViaFieldIndex("tasks", i)) } for i, t := range finalTasks { - errs = errs.Also(t.When.validate().ViaFieldIndex("finally", i)) + errs = errs.Also(t.When.validate(ctx).ViaFieldIndex("finally", i)) } return errs } diff --git a/pkg/apis/pipeline/v1/swagger.json b/pkg/apis/pipeline/v1/swagger.json index bb031c72730..76d325bfff0 100644 --- a/pkg/apis/pipeline/v1/swagger.json +++ b/pkg/apis/pipeline/v1/swagger.json @@ -2220,21 +2220,18 @@ "v1.WhenExpression": { "description": "WhenExpression allows a PipelineTask to declare expressions to be evaluated before the Task is run to determine whether the Task should be executed or skipped", "type": "object", - "required": [ - "input", - "operator", - "values" - ], "properties": { + "cel": { + "description": "CEL is a string of Common Language Expression, which can be used to conditionally execute the task based on the result of the expression evaluation More info about CEL syntax: https://github.com/google/cel-spec/blob/master/doc/langdef.md", + "type": "string" + }, "input": { "description": "Input is the string for guard checking which can be a static input or an output from a parent Task", - "type": "string", - "default": "" + "type": "string" }, "operator": { "description": "Operator that represents an Input's relationship to the values", - "type": "string", - "default": "" + "type": "string" }, "values": { "description": "Values is an array of strings, which is compared against the input, for guard checking It must be non-empty", diff --git a/pkg/apis/pipeline/v1/when_types.go b/pkg/apis/pipeline/v1/when_types.go index 58af7273d2b..45a8bdbd98c 100644 --- a/pkg/apis/pipeline/v1/when_types.go +++ b/pkg/apis/pipeline/v1/when_types.go @@ -27,15 +27,21 @@ import ( // to determine whether the Task should be executed or skipped type WhenExpression struct { // Input is the string for guard checking which can be a static input or an output from a parent Task - Input string `json:"input"` + Input string `json:"input,omitempty"` // Operator that represents an Input's relationship to the values - Operator selection.Operator `json:"operator"` + Operator selection.Operator `json:"operator,omitempty"` // Values is an array of strings, which is compared against the input, for guard checking // It must be non-empty // +listType=atomic - Values []string `json:"values"` + Values []string `json:"values,omitempty"` + + // CEL is a string of Common Language Expression, which can be used to conditionally execute + // the task based on the result of the expression evaluation + // More info about CEL syntax: https://github.com/google/cel-spec/blob/master/doc/langdef.md + // +optional + CEL string `json:"cel,omitempty"` } func (we *WhenExpression) isInputInValues() bool { diff --git a/pkg/apis/pipeline/v1/when_validation.go b/pkg/apis/pipeline/v1/when_validation.go index f445d13ed6a..fb8a92487d9 100644 --- a/pkg/apis/pipeline/v1/when_validation.go +++ b/pkg/apis/pipeline/v1/when_validation.go @@ -17,11 +17,13 @@ limitations under the License. package v1 import ( + "context" "fmt" "strings" // TODO(#7244): Pull the cel-go library for now, the following PR will use the library. _ "github.com/google/cel-go/cel" + "github.com/tektoncd/pipeline/pkg/apis/config" "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/selection" "k8s.io/apimachinery/pkg/util/sets" @@ -33,18 +35,28 @@ var validWhenOperators = []string{ string(selection.NotIn), } -func (wes WhenExpressions) validate() *apis.FieldError { - return wes.validateWhenExpressionsFields().ViaField("when") +func (wes WhenExpressions) validate(ctx context.Context) *apis.FieldError { + return wes.validateWhenExpressionsFields(ctx).ViaField("when") } -func (wes WhenExpressions) validateWhenExpressionsFields() (errs *apis.FieldError) { +func (wes WhenExpressions) validateWhenExpressionsFields(ctx context.Context) (errs *apis.FieldError) { for idx, we := range wes { - errs = errs.Also(we.validateWhenExpressionFields().ViaIndex(idx)) + errs = errs.Also(we.validateWhenExpressionFields(ctx).ViaIndex(idx)) } return errs } -func (we *WhenExpression) validateWhenExpressionFields() *apis.FieldError { +func (we *WhenExpression) validateWhenExpressionFields(ctx context.Context) *apis.FieldError { + if we.CEL != "" { + if !config.FromContextOrDefaults(ctx).FeatureFlags.EnableCELInWhenExpression { + return apis.ErrGeneric("feature flag %s should be set to true to use CEL: %s in WhenExpression", config.EnableCELInWhenExpression, we.CEL) + } + if we.Input != "" || we.Operator != "" || len(we.Values) != 0 { + return apis.ErrGeneric(fmt.Sprintf("cel and input+operator+values cannot be set in one WhenExpression: %v", we)) + } + return nil + } + if equality.Semantic.DeepEqual(we, &WhenExpression{}) || we == nil { return apis.ErrMissingField(apis.CurrentField) } diff --git a/pkg/apis/pipeline/v1/when_validation_test.go b/pkg/apis/pipeline/v1/when_validation_test.go index 8cb2a891356..f3dbeb962d6 100644 --- a/pkg/apis/pipeline/v1/when_validation_test.go +++ b/pkg/apis/pipeline/v1/when_validation_test.go @@ -17,8 +17,10 @@ limitations under the License. package v1 import ( + "context" "testing" + "github.com/tektoncd/pipeline/pkg/apis/config" "k8s.io/apimachinery/pkg/selection" ) @@ -54,7 +56,7 @@ func TestWhenExpressions_Valid(t *testing.T) { }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if err := tt.wes.validate(); err != nil { + if err := tt.wes.validate(context.Background()); err != nil { t.Errorf("WhenExpressions.validate() returned an error for valid when expressions: %s", tt.wes) } }) @@ -97,9 +99,68 @@ func TestWhenExpressions_Invalid(t *testing.T) { }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if err := tt.wes.validate(); err == nil { + if err := tt.wes.validate(context.Background()); err == nil { t.Errorf("WhenExpressions.validate() did not return error for invalid when expressions: %s, %s", tt.wes, err) } }) } } + +func TestCELinWhenExpressions_Valid(t *testing.T) { + ctx := config.ToContext(context.Background(), &config.Config{ + FeatureFlags: &config.FeatureFlags{ + EnableCELInWhenExpression: true, + }, + }) + tests := []struct { + name string + wes WhenExpressions + }{{ + name: "valid cel", + wes: []WhenExpression{{ + CEL: " 'foo' == 'foo' ", + }}, + }} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if err := tt.wes.validate(ctx); err != nil { + t.Errorf("WhenExpressions.validate() returned an error: %s for valid when expressions: %s", err, tt.wes) + } + }) + } +} + +func TestCELWhenExpressions_Invalid(t *testing.T) { + tests := []struct { + name string + wes WhenExpressions + enableCELInWhenExpression bool + }{{ + name: "feature flag not set", + wes: []WhenExpression{{ + CEL: " 'foo' == 'foo' ", + }}, + enableCELInWhenExpression: false, + }, { + name: "CEL should not coexist with input+operator+values", + wes: []WhenExpression{{ + CEL: "'foo' != 'foo'", + Input: "foo", + Operator: selection.In, + Values: []string{"foo"}, + }}, + enableCELInWhenExpression: true, + }} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := config.ToContext(context.Background(), &config.Config{ + FeatureFlags: &config.FeatureFlags{ + EnableCELInWhenExpression: tt.enableCELInWhenExpression, + }, + }) + if err := tt.wes.validate(ctx); err == nil { + t.Errorf("WhenExpressions.validate() did not return error for invalid when expressions: %s", tt.wes) + } + }) + } +} diff --git a/pkg/apis/pipeline/v1beta1/openapi_generated.go b/pkg/apis/pipeline/v1beta1/openapi_generated.go index 74a6be3b64f..eba5a872419 100644 --- a/pkg/apis/pipeline/v1beta1/openapi_generated.go +++ b/pkg/apis/pipeline/v1beta1/openapi_generated.go @@ -5693,7 +5693,6 @@ func schema_pkg_apis_pipeline_v1beta1_WhenExpression(ref common.ReferenceCallbac "input": { SchemaProps: spec.SchemaProps{ Description: "Input is the string for guard checking which can be a static input or an output from a parent Task", - Default: "", Type: []string{"string"}, Format: "", }, @@ -5701,7 +5700,6 @@ func schema_pkg_apis_pipeline_v1beta1_WhenExpression(ref common.ReferenceCallbac "operator": { SchemaProps: spec.SchemaProps{ Description: "Operator that represents an Input's relationship to the values", - Default: "", Type: []string{"string"}, Format: "", }, @@ -5726,8 +5724,14 @@ func schema_pkg_apis_pipeline_v1beta1_WhenExpression(ref common.ReferenceCallbac }, }, }, + "cel": { + SchemaProps: spec.SchemaProps{ + Description: "CEL is a string of Common Language Expression, which can be used to conditionally execute the task based on the result of the expression evaluation More info about CEL syntax: https://github.com/google/cel-spec/blob/master/doc/langdef.md", + Type: []string{"string"}, + Format: "", + }, + }, }, - Required: []string{"input", "operator", "values"}, }, }, } diff --git a/pkg/apis/pipeline/v1beta1/pipeline_conversion.go b/pkg/apis/pipeline/v1beta1/pipeline_conversion.go index 9f5c75a839f..b80b2ad74b7 100644 --- a/pkg/apis/pipeline/v1beta1/pipeline_conversion.go +++ b/pkg/apis/pipeline/v1beta1/pipeline_conversion.go @@ -262,12 +262,14 @@ func (we WhenExpression) convertTo(ctx context.Context, sink *v1.WhenExpression) sink.Input = we.Input sink.Operator = we.Operator sink.Values = we.Values + sink.CEL = we.CEL } func (we *WhenExpression) convertFrom(ctx context.Context, source v1.WhenExpression) { we.Input = source.Input we.Operator = source.Operator we.Values = source.Values + we.CEL = source.CEL } func (m *Matrix) convertTo(ctx context.Context, sink *v1.Matrix) { diff --git a/pkg/apis/pipeline/v1beta1/pipeline_conversion_test.go b/pkg/apis/pipeline/v1beta1/pipeline_conversion_test.go index e9ba9cc9032..3b5939ccce3 100644 --- a/pkg/apis/pipeline/v1beta1/pipeline_conversion_test.go +++ b/pkg/apis/pipeline/v1beta1/pipeline_conversion_test.go @@ -62,6 +62,9 @@ func TestPipelineConversion(t *testing.T) { Name: "foo", OnError: v1beta1.PipelineTaskContinue, TaskRef: &v1beta1.TaskRef{Name: "example.com/my-foo-task"}, + WhenExpressions: v1beta1.WhenExpressions{{ + CEL: "'$(params.param-1)'=='foo'", + }}, }}, Params: []v1beta1.ParamSpec{{ Name: "param-1", diff --git a/pkg/apis/pipeline/v1beta1/pipeline_validation.go b/pkg/apis/pipeline/v1beta1/pipeline_validation.go index 2a0cbd6d2b6..118fff45a15 100644 --- a/pkg/apis/pipeline/v1beta1/pipeline_validation.go +++ b/pkg/apis/pipeline/v1beta1/pipeline_validation.go @@ -86,7 +86,7 @@ func (ps *PipelineSpec) Validate(ctx context.Context) (errs *apis.FieldError) { errs = errs.Also(validatePipelineResults(ps.Results, ps.Tasks, ps.Finally)) errs = errs.Also(validateTasksAndFinallySection(ps)) errs = errs.Also(validateFinalTasks(ps.Tasks, ps.Finally)) - errs = errs.Also(validateWhenExpressions(ps.Tasks, ps.Finally)) + errs = errs.Also(validateWhenExpressions(ctx, ps.Tasks, ps.Finally)) errs = errs.Also(validateMatrix(ctx, ps.Tasks).ViaField("tasks")) errs = errs.Also(validateMatrix(ctx, ps.Finally).ViaField("finally")) return errs @@ -707,12 +707,12 @@ func validateResultsVariablesExpressionsInFinally(expressions []string, pipeline return errs } -func validateWhenExpressions(tasks []PipelineTask, finalTasks []PipelineTask) (errs *apis.FieldError) { +func validateWhenExpressions(ctx context.Context, tasks []PipelineTask, finalTasks []PipelineTask) (errs *apis.FieldError) { for i, t := range tasks { - errs = errs.Also(t.WhenExpressions.validate().ViaFieldIndex("tasks", i)) + errs = errs.Also(t.WhenExpressions.validate(ctx).ViaFieldIndex("tasks", i)) } for i, t := range finalTasks { - errs = errs.Also(t.WhenExpressions.validate().ViaFieldIndex("finally", i)) + errs = errs.Also(t.WhenExpressions.validate(ctx).ViaFieldIndex("finally", i)) } return errs } diff --git a/pkg/apis/pipeline/v1beta1/swagger.json b/pkg/apis/pipeline/v1beta1/swagger.json index 97f789dbcbf..4ff7d3d43ab 100644 --- a/pkg/apis/pipeline/v1beta1/swagger.json +++ b/pkg/apis/pipeline/v1beta1/swagger.json @@ -3111,21 +3111,18 @@ "v1beta1.WhenExpression": { "description": "WhenExpression allows a PipelineTask to declare expressions to be evaluated before the Task is run to determine whether the Task should be executed or skipped", "type": "object", - "required": [ - "input", - "operator", - "values" - ], "properties": { + "cel": { + "description": "CEL is a string of Common Language Expression, which can be used to conditionally execute the task based on the result of the expression evaluation More info about CEL syntax: https://github.com/google/cel-spec/blob/master/doc/langdef.md", + "type": "string" + }, "input": { "description": "Input is the string for guard checking which can be a static input or an output from a parent Task", - "type": "string", - "default": "" + "type": "string" }, "operator": { "description": "Operator that represents an Input's relationship to the values", - "type": "string", - "default": "" + "type": "string" }, "values": { "description": "Values is an array of strings, which is compared against the input, for guard checking It must be non-empty", diff --git a/pkg/apis/pipeline/v1beta1/when_types.go b/pkg/apis/pipeline/v1beta1/when_types.go index e98eff147c7..76a78ea0ea1 100644 --- a/pkg/apis/pipeline/v1beta1/when_types.go +++ b/pkg/apis/pipeline/v1beta1/when_types.go @@ -27,15 +27,21 @@ import ( // to determine whether the Task should be executed or skipped type WhenExpression struct { // Input is the string for guard checking which can be a static input or an output from a parent Task - Input string `json:"input"` + Input string `json:"input,omitempty"` // Operator that represents an Input's relationship to the values - Operator selection.Operator `json:"operator"` + Operator selection.Operator `json:"operator,omitempty"` // Values is an array of strings, which is compared against the input, for guard checking // It must be non-empty // +listType=atomic - Values []string `json:"values"` + Values []string `json:"values,omitempty"` + + // CEL is a string of Common Language Expression, which can be used to conditionally execute + // the task based on the result of the expression evaluation + // More info about CEL syntax: https://github.com/google/cel-spec/blob/master/doc/langdef.md + // +optional + CEL string `json:"cel,omitempty"` } func (we *WhenExpression) isInputInValues() bool { diff --git a/pkg/apis/pipeline/v1beta1/when_validation.go b/pkg/apis/pipeline/v1beta1/when_validation.go index 17bb55c56cf..8279950afab 100644 --- a/pkg/apis/pipeline/v1beta1/when_validation.go +++ b/pkg/apis/pipeline/v1beta1/when_validation.go @@ -17,9 +17,11 @@ limitations under the License. package v1beta1 import ( + "context" "fmt" "strings" + "github.com/tektoncd/pipeline/pkg/apis/config" "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/selection" "k8s.io/apimachinery/pkg/util/sets" @@ -31,18 +33,28 @@ var validWhenOperators = []string{ string(selection.NotIn), } -func (wes WhenExpressions) validate() *apis.FieldError { - return wes.validateWhenExpressionsFields().ViaField("when") +func (wes WhenExpressions) validate(ctx context.Context) *apis.FieldError { + return wes.validateWhenExpressionsFields(ctx).ViaField("when") } -func (wes WhenExpressions) validateWhenExpressionsFields() (errs *apis.FieldError) { +func (wes WhenExpressions) validateWhenExpressionsFields(ctx context.Context) (errs *apis.FieldError) { for idx, we := range wes { - errs = errs.Also(we.validateWhenExpressionFields().ViaIndex(idx)) + errs = errs.Also(we.validateWhenExpressionFields(ctx).ViaIndex(idx)) } return errs } -func (we *WhenExpression) validateWhenExpressionFields() *apis.FieldError { +func (we *WhenExpression) validateWhenExpressionFields(ctx context.Context) *apis.FieldError { + if we.CEL != "" { + if !config.FromContextOrDefaults(ctx).FeatureFlags.EnableCELInWhenExpression { + return apis.ErrGeneric("feature flag %s should be set to true to use CEL: %s in WhenExpression", config.EnableCELInWhenExpression, we.CEL) + } + if we.Input != "" || we.Operator != "" || len(we.Values) != 0 { + return apis.ErrGeneric(fmt.Sprintf("cel and input+operator+values cannot be set in one WhenExpression: %v", we)) + } + return nil + } + if equality.Semantic.DeepEqual(we, &WhenExpression{}) || we == nil { return apis.ErrMissingField(apis.CurrentField) } diff --git a/pkg/apis/pipeline/v1beta1/when_validation_test.go b/pkg/apis/pipeline/v1beta1/when_validation_test.go index ac23b41d402..16ec85b52fd 100644 --- a/pkg/apis/pipeline/v1beta1/when_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/when_validation_test.go @@ -17,8 +17,10 @@ limitations under the License. package v1beta1 import ( + "context" "testing" + "github.com/tektoncd/pipeline/pkg/apis/config" "k8s.io/apimachinery/pkg/selection" ) @@ -54,7 +56,7 @@ func TestWhenExpressions_Valid(t *testing.T) { }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if err := tt.wes.validate(); err != nil { + if err := tt.wes.validate(context.Background()); err != nil { t.Errorf("WhenExpressions.validate() returned an error for valid when expressions: %s", tt.wes) } }) @@ -97,9 +99,68 @@ func TestWhenExpressions_Invalid(t *testing.T) { }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if err := tt.wes.validate(); err == nil { + if err := tt.wes.validate(context.Background()); err == nil { t.Errorf("WhenExpressions.validate() did not return error for invalid when expressions: %s, %s", tt.wes, err) } }) } } + +func TestCELinWhenExpressions_Valid(t *testing.T) { + ctx := config.ToContext(context.Background(), &config.Config{ + FeatureFlags: &config.FeatureFlags{ + EnableCELInWhenExpression: true, + }, + }) + tests := []struct { + name string + wes WhenExpressions + }{{ + name: "valid cel", + wes: []WhenExpression{{ + CEL: " 'foo' == 'foo' ", + }}, + }} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if err := tt.wes.validate(ctx); err != nil { + t.Errorf("WhenExpressions.validate() returned an error: %s for valid when expressions: %s", err, tt.wes) + } + }) + } +} + +func TestCELWhenExpressions_Invalid(t *testing.T) { + tests := []struct { + name string + wes WhenExpressions + enableCELInWhenExpression bool + }{{ + name: "feature flag not set", + wes: []WhenExpression{{ + CEL: " 'foo' == 'foo' ", + }}, + enableCELInWhenExpression: false, + }, { + name: "CEL should not coexist with input+operator+values", + wes: []WhenExpression{{ + CEL: "'foo' != 'foo'", + Input: "foo", + Operator: selection.In, + Values: []string{"foo"}, + }}, + enableCELInWhenExpression: true, + }} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := config.ToContext(context.Background(), &config.Config{ + FeatureFlags: &config.FeatureFlags{ + EnableCELInWhenExpression: tt.enableCELInWhenExpression, + }, + }) + if err := tt.wes.validate(ctx); err == nil { + t.Errorf("WhenExpressions.validate() did not return error for invalid when expressions: %s", tt.wes) + } + }) + } +} From 0111021091a3f8490ab02e1598e79b0d132c320c Mon Sep 17 00:00:00 2001 From: Chitrang Patel Date: Fri, 20 Oct 2023 16:30:53 -0400 Subject: [PATCH 5/7] Flake Test fix: Sort TaskRunResults PR https://github.com/tektoncd/pipeline/pull/7100 introduced a flaky test. When comparing the pipelineRuns and TaskRuns, the results were not sorted. As a result, `cmp.Diff` throws an error sometimes when the order does not match. This PR sorts the TaskRunResults by providing a sort function to `cmp.Diff`. After the fix, running this 20 times consecutively, no flakes were found. --- test/propagated_results_test.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/test/propagated_results_test.go b/test/propagated_results_test.go index 6af07542851..57fee1be165 100644 --- a/test/propagated_results_test.go +++ b/test/propagated_results_test.go @@ -35,10 +35,16 @@ import ( func TestPropagatedResults(t *testing.T) { t.Parallel() + // Ignore the Results when comparing the Runs directly. Instead, check the results separately. + // This is because the order of resutls changes and makes the test very flaky. + ignoreTaskRunStatusFields := cmpopts.IgnoreFields(v1.TaskRunStatusFields{}, "Results") + sortTaskRunResults := cmpopts.SortSlices(func(x, y v1.TaskRunResult) bool { return x.Name < y.Name }) + ignorePipelineRunStatusFields := cmpopts.IgnoreFields(v1.PipelineRunStatusFields{}, "Provenance") ignoreTaskRunStatus := cmpopts.IgnoreFields(v1.TaskRunStatusFields{}, "StartTime", "CompletionTime", "Sidecars", "Provenance") requireAlphaFeatureFlag = requireAnyGate(map[string]string{ "enable-api-fields": "alpha"}) + type tests struct { name string pipelineName string @@ -89,6 +95,7 @@ func TestPropagatedResults(t *testing.T) { ignoreStepState, ignoreSAPipelineRunSpec, ignorePipelineRunStatusFields, + ignoreTaskRunStatusFields, ) if d != "" { t.Fatalf(`The resolved spec does not match the expected spec. Here is the diff: %v`, d) @@ -105,10 +112,15 @@ func TestPropagatedResults(t *testing.T) { ignoreContainerStates, ignoreStepState, ignoreSATaskRunSpec, + ignoreTaskRunStatusFields, ) if d != "" { t.Fatalf(`The expected taskrun does not match created taskrun. Here is the diff: %v`, d) } + d = cmp.Diff(tr.Status.TaskRunStatusFields.Results, taskrun.Status.TaskRunStatusFields.Results, sortTaskRunResults) + if d != "" { + t.Fatalf(`The expected TaskRunResults does not what was received. Here is the diff: %v`, d) + } } t.Logf("Successfully finished test %q", td.name) }) From c29e9780456a3afb28fbc3ee694a6f042df09593 Mon Sep 17 00:00:00 2001 From: Yongxuan Zhang Date: Fri, 20 Oct 2023 17:26:31 +0000 Subject: [PATCH 6/7] [TEP-0145] Add sanity check for CEL expression This commit adds sanity check for CEL expression by pre-compiling the CEL expression at admission webhook without evluation. This can help fail fast if the CEL expression is invalid. And also disallow the usage of variable references without wrapper with single quotes and prevent CEL injection from variables. Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com --- docs/pipelines.md | 13 ++++++- pkg/apis/pipeline/v1/when_validation.go | 15 ++++++- pkg/apis/pipeline/v1/when_validation_test.go | 39 ++++++++++++++++++- pkg/apis/pipeline/v1beta1/when_validation.go | 12 ++++++ .../pipeline/v1beta1/when_validation_test.go | 39 ++++++++++++++++++- 5 files changed, 113 insertions(+), 5 deletions(-) diff --git a/docs/pipelines.md b/docs/pipelines.md index 300835e9ddf..44071ab5a3a 100644 --- a/docs/pipelines.md +++ b/docs/pipelines.md @@ -777,12 +777,23 @@ There are a lot of scenarios where `when` expressions can be really useful. Some #### Use CEL expression in WhenExpression -> :seedling: **`CEL in WhenExpression` is an [alpha](install.md#alpha-features) feature.** +> :seedling: **`CEL in WhenExpression` is an [alpha](additional-configs.md#alpha-features) feature.** > The `enable-cel-in-whenexpression` feature flag must be set to `"true"` to enable the use of `CEL` in `WhenExpression`. > > :warning: This feature is in a preview mode. > It is still in a very early stage of development and is not yet fully functional +`CEL` expression is validated at admission webhook and a validation error will be returned if the expression is invalid. + +**Note:** To use Tekton's [variable substitution](variables.md), you need to wrap the reference with single quotes. This also means that if you pass another CEL expression via `params` or `results`, it won't be executed. Therefore CEL injection is disallowed. + +For example: +``` +This is valid: '$(params.foo)' == 'foo' +This is invalid: $(params.foo) == 'foo' +CEL's variable substitution is not supported yet and thus invalid: params.foo == 'foo' +``` + #### Guarding a `Task` and its dependent `Tasks` To guard a `Task` and its dependent Tasks: diff --git a/pkg/apis/pipeline/v1/when_validation.go b/pkg/apis/pipeline/v1/when_validation.go index fb8a92487d9..2a058299a7d 100644 --- a/pkg/apis/pipeline/v1/when_validation.go +++ b/pkg/apis/pipeline/v1/when_validation.go @@ -21,8 +21,7 @@ import ( "fmt" "strings" - // TODO(#7244): Pull the cel-go library for now, the following PR will use the library. - _ "github.com/google/cel-go/cel" + "github.com/google/cel-go/cel" "github.com/tektoncd/pipeline/pkg/apis/config" "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/selection" @@ -54,6 +53,18 @@ func (we *WhenExpression) validateWhenExpressionFields(ctx context.Context) *api if we.Input != "" || we.Operator != "" || len(we.Values) != 0 { return apis.ErrGeneric(fmt.Sprintf("cel and input+operator+values cannot be set in one WhenExpression: %v", we)) } + + // We need to compile the CEL expression and check if it is a valid expression + // note that at the validation webhook, Tekton's variables are not substituted, + // so they need to be wrapped with single quotes. + // e.g. This is a valid CEL expression: '$(params.foo)' == 'foo'; + // But this is not a valid expression since CEL cannot recognize: $(params.foo) == 'foo'; + // This is not valid since we don't pass params to CEL's environment: params.foo == 'foo'; + env, _ := cel.NewEnv() + _, iss := env.Compile(we.CEL) + if iss.Err() != nil { + return apis.ErrGeneric("invalid cel expression: %s with err: %s", we.CEL, iss.Err().Error()) + } return nil } diff --git a/pkg/apis/pipeline/v1/when_validation_test.go b/pkg/apis/pipeline/v1/when_validation_test.go index f3dbeb962d6..eb4c615bcf3 100644 --- a/pkg/apis/pipeline/v1/when_validation_test.go +++ b/pkg/apis/pipeline/v1/when_validation_test.go @@ -116,10 +116,35 @@ func TestCELinWhenExpressions_Valid(t *testing.T) { name string wes WhenExpressions }{{ - name: "valid cel", + name: "valid operator - Equal", wes: []WhenExpression{{ CEL: " 'foo' == 'foo' ", }}, + }, { + name: "valid operator - NotEqual", + wes: []WhenExpression{{ + CEL: " 'foo' != 'foo' ", + }}, + }, { + name: "valid operator - In", + wes: []WhenExpression{{ + CEL: "'foo' in ['foo', 'bar']", + }}, + }, { + name: "valid operator - NotIn", + wes: []WhenExpression{{ + CEL: "!('foo' in ['foo', 'bar'])", + }}, + }, { + name: "valid regex expression", + wes: []WhenExpression{{ + CEL: "'release/v1'.matches('release/.*')", + }}, + }, { + name: "valid variable reference syntax", + wes: []WhenExpression{{ + CEL: "'$(params.foo)' in ['foo', 'bar']", + }}, }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -141,6 +166,18 @@ func TestCELWhenExpressions_Invalid(t *testing.T) { CEL: " 'foo' == 'foo' ", }}, enableCELInWhenExpression: false, + }, { + name: "variable reference should be wrapped with single quotes", + wes: []WhenExpression{{ + CEL: " $(params.foo) == 'foo' ", + }}, + enableCELInWhenExpression: true, + }, { + name: "variables not declared in environment", + wes: []WhenExpression{{ + CEL: " params.foo == 'foo' ", + }}, + enableCELInWhenExpression: true, }, { name: "CEL should not coexist with input+operator+values", wes: []WhenExpression{{ diff --git a/pkg/apis/pipeline/v1beta1/when_validation.go b/pkg/apis/pipeline/v1beta1/when_validation.go index 8279950afab..33855040b2b 100644 --- a/pkg/apis/pipeline/v1beta1/when_validation.go +++ b/pkg/apis/pipeline/v1beta1/when_validation.go @@ -21,6 +21,7 @@ import ( "fmt" "strings" + "github.com/google/cel-go/cel" "github.com/tektoncd/pipeline/pkg/apis/config" "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/selection" @@ -52,6 +53,17 @@ func (we *WhenExpression) validateWhenExpressionFields(ctx context.Context) *api if we.Input != "" || we.Operator != "" || len(we.Values) != 0 { return apis.ErrGeneric(fmt.Sprintf("cel and input+operator+values cannot be set in one WhenExpression: %v", we)) } + // We need to compile the CEL expression and check if it is a valid expression + // note that at the validation webhook, Tekton's variables are not substituted, + // so they need to be wrapped with single quotes. + // e.g. This is a valid CEL expression: '$(params.foo)' == 'foo'; + // But this is not a valid expression since CEL cannot recognize: $(params.foo) == 'foo'; + // This is not valid since we don't pass params to CEL's environment: params.foo == 'foo'; + env, _ := cel.NewEnv() + _, iss := env.Compile(we.CEL) + if iss.Err() != nil { + return apis.ErrGeneric("invalid cel expression: %s with err: %s", we.CEL, iss.Err().Error()) + } return nil } diff --git a/pkg/apis/pipeline/v1beta1/when_validation_test.go b/pkg/apis/pipeline/v1beta1/when_validation_test.go index 16ec85b52fd..0393cb3c288 100644 --- a/pkg/apis/pipeline/v1beta1/when_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/when_validation_test.go @@ -116,10 +116,35 @@ func TestCELinWhenExpressions_Valid(t *testing.T) { name string wes WhenExpressions }{{ - name: "valid cel", + name: "valid operator - Equal", wes: []WhenExpression{{ CEL: " 'foo' == 'foo' ", }}, + }, { + name: "valid operator - NotEqual", + wes: []WhenExpression{{ + CEL: " 'foo' != 'foo' ", + }}, + }, { + name: "valid operator - In", + wes: []WhenExpression{{ + CEL: "'foo' in ['foo', 'bar']", + }}, + }, { + name: "valid operator - NotIn", + wes: []WhenExpression{{ + CEL: "!('foo' in ['foo', 'bar'])", + }}, + }, { + name: "valid regex expression", + wes: []WhenExpression{{ + CEL: "'release/v1'.matches('release/.*')", + }}, + }, { + name: "valid variable reference syntax", + wes: []WhenExpression{{ + CEL: "'$(params.foo)' in ['foo', 'bar']", + }}, }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -141,6 +166,18 @@ func TestCELWhenExpressions_Invalid(t *testing.T) { CEL: " 'foo' == 'foo' ", }}, enableCELInWhenExpression: false, + }, { + name: "variable reference should be wrapped with single quotes", + wes: []WhenExpression{{ + CEL: " $(params.foo) == 'foo' ", + }}, + enableCELInWhenExpression: true, + }, { + name: "variables not declared in environment", + wes: []WhenExpression{{ + CEL: " params.foo == 'foo' ", + }}, + enableCELInWhenExpression: true, }, { name: "CEL should not coexist with input+operator+values", wes: []WhenExpression{{ From dcd34c1d4a617583233960fb5c11d423161e908d Mon Sep 17 00:00:00 2001 From: Chmouel Boudjnah Date: Mon, 23 Oct 2023 11:53:19 +0200 Subject: [PATCH 7/7] Set public Tekton Hub API as default catalog Public tekton hub API was not set by default in the hub resolver, i understand that ArtifactHUB is the future but until then makes it more easier to use the tekton hub Signed-off-by: Chmouel Boudjnah --- config/resolvers/resolvers-deployment.yaml | 2 ++ docs/hub-resolver.md | 5 ++++- go.mod | 2 +- 3 files changed, 7 insertions(+), 2 deletions(-) diff --git a/config/resolvers/resolvers-deployment.yaml b/config/resolvers/resolvers-deployment.yaml index 87d1e7c8213..48c48d43604 100644 --- a/config/resolvers/resolvers-deployment.yaml +++ b/config/resolvers/resolvers-deployment.yaml @@ -102,6 +102,8 @@ spec: # Override this env var to set a private hub api endpoint - name: ARTIFACT_HUB_API value: "https://artifacthub.io/" + - name: TEKTON_HUB_API + value: "https://api.hub.tekton.dev/" securityContext: allowPrivilegeEscalation: false readOnlyRootFilesystem: true diff --git a/docs/hub-resolver.md b/docs/hub-resolver.md index 37d23cf3f58..b7cc7c64fe6 100644 --- a/docs/hub-resolver.md +++ b/docs/hub-resolver.md @@ -64,7 +64,10 @@ env value: "https://artifacthub.io/" ``` -When setting the `type` field to `tekton`, you **must** configure your own instance of the Tekton Hub by setting the `TEKTON_HUB_API` environment variable in +When setting the `type` field to `tekton`, the resolver will hit the public +tekton catalog api at https://api.hub.tekton.dev by default but you can configure +your own instance of the Tekton Hub by setting the `TEKTON_HUB_API` environment +variable in [`../config/resolvers/resolvers-deployment.yaml`](../config/resolvers/resolvers-deployment.yaml). Example: ```yaml diff --git a/go.mod b/go.mod index 822aa0bea1c..0d9cc3d4be2 100644 --- a/go.mod +++ b/go.mod @@ -190,7 +190,7 @@ require ( github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect github.com/golang/protobuf v1.5.3 // indirect github.com/google/gofuzz v1.2.0 // indirect - github.com/hashicorp/go-version v1.6.0 // indirect + github.com/hashicorp/go-version v1.6.0 github.com/imdario/mergo v0.3.13 // indirect github.com/jmespath/go-jmespath v0.4.0 // indirect github.com/josharian/intern v1.0.0 // indirect