From eeebe248b11c4d37def435b802e2b556899591ce Mon Sep 17 00:00:00 2001 From: Billy Lynch Date: Thu, 25 Mar 2021 12:15:29 -0400 Subject: [PATCH] Implement implicit parameter resolution. This is the implementation of [TEP-0023](https://github.com/tektoncd/community/blob/main/teps/0023-implicit-mapping.md). This adds information to the defaulting context to allow parameters to be passed down the stack during evaluation. This functionality is gated behind the alpha feature flag. Additionally, Tasks are now allowed to accept more params than are actually used. This should generally be safe, since extra params that are provided should not affect the behavior of the Task itself. --- config/200-role.yaml | 2 +- config/webhook.yaml | 2 + docs/pipelineruns.md | 69 ++++ docs/taskruns.md | 63 ++++ .../alpha/pipelinerun-implicit-params.yaml | 18 + .../taskruns/alpha/implicit-params.yaml | 15 + pkg/apis/pipeline/v1beta1/param_context.go | 172 ++++++++++ .../pipeline/v1beta1/param_context_test.go | 314 ++++++++++++++++++ .../pipeline/v1beta1/pipeline_defaults.go | 26 +- .../pipeline/v1beta1/pipelinerun_defaults.go | 3 + .../v1beta1/pipelinerun_defaults_test.go | 113 ++++++- pkg/apis/pipeline/v1beta1/task_defaults.go | 5 + pkg/apis/pipeline/v1beta1/taskrun_defaults.go | 3 + .../pipeline/v1beta1/taskrun_defaults_test.go | 76 ++++- pkg/reconciler/pipelinerun/pipelinerun.go | 2 +- pkg/reconciler/taskrun/taskrun.go | 2 +- pkg/reconciler/taskrun/validate_resources.go | 26 +- .../taskrun/validate_resources_test.go | 37 ++- 18 files changed, 910 insertions(+), 38 deletions(-) create mode 100644 examples/v1beta1/pipelineruns/alpha/pipelinerun-implicit-params.yaml create mode 100644 examples/v1beta1/taskruns/alpha/implicit-params.yaml create mode 100644 pkg/apis/pipeline/v1beta1/param_context.go create mode 100644 pkg/apis/pipeline/v1beta1/param_context_test.go diff --git a/config/200-role.yaml b/config/200-role.yaml index 98041e7e064..d52f7149c99 100644 --- a/config/200-role.yaml +++ b/config/200-role.yaml @@ -52,7 +52,7 @@ rules: - apiGroups: [""] resources: ["configmaps"] verbs: ["get"] - resourceNames: ["config-logging", "config-observability", "config-leader-election"] + resourceNames: ["config-logging", "config-observability", "config-leader-election", "feature-flags"] - apiGroups: [""] resources: ["secrets"] verbs: ["list", "watch"] diff --git a/config/webhook.yaml b/config/webhook.yaml index 739a12d75b9..53f1fb9c079 100644 --- a/config/webhook.yaml +++ b/config/webhook.yaml @@ -100,6 +100,8 @@ spec: value: config-observability - name: CONFIG_LEADERELECTION_NAME value: config-leader-election + - name: CONFIG_FEATURE_FLAGS_NAME + value: feature-flags - name: WEBHOOK_SERVICE_NAME value: tekton-pipelines-webhook - name: WEBHOOK_SECRET_NAME diff --git a/docs/pipelineruns.md b/docs/pipelineruns.md index d919cb51d42..50c54e34cfa 100644 --- a/docs/pipelineruns.md +++ b/docs/pipelineruns.md @@ -246,6 +246,75 @@ case is when your CI system autogenerates `PipelineRuns` and it has `Parameters` provide to all `PipelineRuns`. Because you can pass in extra `Parameters`, you don't have to go through the complexity of checking each `Pipeline` and providing only the required params. +#### Implicit Parameters + +**([alpha only](https://github.com/tektoncd/pipeline/blob/main/docs/install.md#alpha-features))** + +When using an inlined spec, parameters from the parent `PipelineRun` will be +available to any inlined specs without needing to be explicitly defined. This +allows authors to simplify specs by automatically propagating top-level +parameters down to other inlined resources. + +```yaml +apiVersion: tekton.dev/v1beta1 +kind: PipelineRun +metadata: + generateName: echo- +spec: + params: + - name: MESSAGE + value: "Good Morning!" + pipelineSpec: + tasks: + - name: echo-message + taskSpec: + steps: + - name: echo + image: ubuntu + script: | + #!/usr/bin/env bash + echo "$(params.MESSAGE)" +``` + +On creation, this will resolve to a fully-formed spec and will be returned back +to clients to avoid ambiguity: + +```yaml +apiVersion: tekton.dev/v1beta1 +kind: PipelineRun +metadata: + generateName: echo- +spec: + params: + - name: MESSAGE + value: Good Morning! + pipelineSpec: + params: + - name: MESSAGE + type: string + tasks: + - name: echo-message + params: + - name: MESSAGE + value: $(params.MESSAGE) + taskSpec: + params: + - name: MESSAGE + type: string + spec: null + steps: + - name: echo + image: ubuntu + script: | + #!/usr/bin/env bash + echo "$(params.MESSAGE)" +``` + +Note that all implicit Parameters will be passed through to inlined resources +(i.e. PipelineRun -> Pipeline -> Tasks) even if they are not used. +Extra parameters passed this way should generally be safe (since they aren't +actually used), but may result in more verbose specs being returned by the API. + ### Specifying custom `ServiceAccount` credentials You can execute the `Pipeline` in your `PipelineRun` with a specific set of credentials by diff --git a/docs/taskruns.md b/docs/taskruns.md index 867cfb12d6d..0f37925a1bc 100644 --- a/docs/taskruns.md +++ b/docs/taskruns.md @@ -163,6 +163,69 @@ spec: **Note:** If a parameter does not have an implicit default value, you must explicitly set its value. +#### Implicit Parameters + +**([alpha only](https://github.com/tektoncd/pipeline/blob/main/docs/install.md#alpha-features))** + +When using an inlined `taskSpec`, parameters from the parent `TaskRun` will be +available to the `Task` without needing to be explicitly defined. + +```yaml +apiVersion: tekton.dev/v1beta1 +kind: TaskRun +metadata: + generateName: hello- +spec: + params: + - name: message + value: "hello world!" + taskSpec: + # There are no explicit params defined here. + # They are derived from the TaskRun params above. + steps: + - name: default + image: ubuntu + script: | + echo $(params.message) +``` + +On creation, this will resolve to a fully-formed spec and will be returned back +to clients to avoid ambiguity: + +```yaml +apiVersion: tekton.dev/v1beta1 +kind: TaskRun +metadata: + generateName: hello- +spec: + params: + - name: message + value: "hello world!" + taskSpec: + params: + - name: message + type: string + steps: + - name: default + image: ubuntu + script: | + echo $(params.message) +``` + +Note that all implicit Parameters will be passed through to inlined resource, +even if they are not used. Extra parameters passed this way should generally +be safe (since they aren't actually used), but may result in more verbose specs +being returned by the API. + +#### Extra Parameters + +**([alpha only](https://github.com/tektoncd/pipeline/blob/main/docs/install.md#alpha-features))** + +You can pass in extra `Parameters` if needed depending on your use cases. An example use +case is when your CI system autogenerates `TaskRuns` and it has `Parameters` it wants to +provide to all `TaskRuns`. Because you can pass in extra `Parameters`, you don't have to +go through the complexity of checking each `Task` and providing only the required params. + ### Specifying `Resources` If a `Task` requires [`Resources`](tasks.md#specifying-resources) (that is, `inputs` and `outputs`) you must diff --git a/examples/v1beta1/pipelineruns/alpha/pipelinerun-implicit-params.yaml b/examples/v1beta1/pipelineruns/alpha/pipelinerun-implicit-params.yaml new file mode 100644 index 00000000000..69ec95650cc --- /dev/null +++ b/examples/v1beta1/pipelineruns/alpha/pipelinerun-implicit-params.yaml @@ -0,0 +1,18 @@ +apiVersion: tekton.dev/v1beta1 +kind: PipelineRun +metadata: + generateName: echo- +spec: + pipelineSpec: + tasks: + - name: echo-message + taskSpec: + steps: + - name: echo + image: ubuntu + script: | + #!/usr/bin/env bash + echo "$(params.MESSAGE)" + params: + - name: MESSAGE + value: "Good Morning!" \ No newline at end of file diff --git a/examples/v1beta1/taskruns/alpha/implicit-params.yaml b/examples/v1beta1/taskruns/alpha/implicit-params.yaml new file mode 100644 index 00000000000..d0a47a8eacc --- /dev/null +++ b/examples/v1beta1/taskruns/alpha/implicit-params.yaml @@ -0,0 +1,15 @@ +apiVersion: tekton.dev/v1beta1 +kind: TaskRun +metadata: + generateName: hello- +spec: + params: + - name: message + value: "hello world!" + taskSpec: + # There are no explicit params defined here. They are derived from the TaskRun. + steps: + - name: default + image: ubuntu + script: | + echo $(params.message) diff --git a/pkg/apis/pipeline/v1beta1/param_context.go b/pkg/apis/pipeline/v1beta1/param_context.go new file mode 100644 index 00000000000..59c737ef130 --- /dev/null +++ b/pkg/apis/pipeline/v1beta1/param_context.go @@ -0,0 +1,172 @@ +// Copyright 2021 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 v1beta1 + +import ( + "context" + "fmt" + + "github.com/tektoncd/pipeline/pkg/apis/config" +) + +// paramCtxKey is the unique identifier for referencing param information from +// a context.Context. See [context.Context.Value](https://pkg.go.dev/context#Context) +// for more details. +var paramCtxKey struct{} + +// paramCtxVal is the data type stored in the param context. +// This maps param names -> ParamSpec. +type paramCtxVal map[string]ParamSpec + +// AddContextParams adds the given Params to the param context. This only +// preserves the fields included in ParamSpec - Name and Type. +func AddContextParams(ctx context.Context, in []Param) context.Context { + if in == nil { + return ctx + } + + if config.FromContextOrDefaults(ctx).FeatureFlags.EnableAPIFields != "alpha" { + return ctx + } + + out := paramCtxVal{} + // Copy map to ensure that contexts are unique. + v := ctx.Value(paramCtxKey) + if v != nil { + for n, cps := range v.(paramCtxVal) { + out[n] = cps + } + } + for _, p := range in { + // The user may have omitted type data. Fill this in in to normalize data. + if v := p.Value; v.Type == "" { + if len(v.ArrayVal) > 0 { + p.Value.Type = ParamTypeArray + } + if v.StringVal != "" { + p.Value.Type = ParamTypeString + } + } + out[p.Name] = ParamSpec{ + Name: p.Name, + Type: p.Value.Type, + } + } + return context.WithValue(ctx, paramCtxKey, out) +} + +// AddContextParamSpec adds the given ParamSpecs to the param context. +func AddContextParamSpec(ctx context.Context, in []ParamSpec) context.Context { + if in == nil { + return ctx + } + + if config.FromContextOrDefaults(ctx).FeatureFlags.EnableAPIFields != "alpha" { + return ctx + } + + out := paramCtxVal{} + // Copy map to ensure that contexts are unique. + v := ctx.Value(paramCtxKey) + if v != nil { + for n, ps := range v.(paramCtxVal) { + out[n] = ps + } + } + for _, p := range in { + cps := ParamSpec{ + Name: p.Name, + Type: p.Type, + Description: p.Description, + Default: p.Default, + } + out[p.Name] = cps + } + return context.WithValue(ctx, paramCtxKey, out) +} + +// GetContextParams returns the current context parameters overlayed with a +// given set of params. Overrides should generally be the current layer you +// are trying to evaluate. Any context params not in the overrides will default +// to a generic pass-through param of the given type (i.e. $(params.name) or +// $(params.name[*])). +func GetContextParams(ctx context.Context, overlays ...Param) []Param { + pv := paramCtxVal{} + v := ctx.Value(paramCtxKey) + if v == nil && len(overlays) == 0 { + return nil + } + if v != nil { + pv = v.(paramCtxVal) + } + out := make([]Param, 0, len(pv)) + + // Overlays take precedence over any context params. Keep track of + // these and automatically add them to the output. + overrideSet := make(map[string]Param, len(overlays)) + for _, p := range overlays { + overrideSet[p.Name] = p + out = append(out, p) + } + + // Include the rest of the context params. + for _, ps := range pv { + // Don't do anything for any overlay params - these are already + // included. + if _, ok := overrideSet[ps.Name]; ok { + continue + } + + // If there is no overlay, pass through the param to the next level. + // e.g. for strings $(params.name), for arrays $(params.name[*]). + p := Param{ + Name: ps.Name, + } + if ps.Type == ParamTypeString { + p.Value = ArrayOrString{ + Type: ParamTypeString, + StringVal: fmt.Sprintf("$(params.%s)", ps.Name), + } + } else { + p.Value = ArrayOrString{ + Type: ParamTypeArray, + ArrayVal: []string{fmt.Sprintf("$(params.%s[*])", ps.Name)}, + } + } + out = append(out, p) + } + + return out +} + +// GetContextParamSpecs returns the current context ParamSpecs. +func GetContextParamSpecs(ctx context.Context) []ParamSpec { + v := ctx.Value(paramCtxKey) + if v == nil { + return nil + } + + pv := v.(paramCtxVal) + out := make([]ParamSpec, 0, len(pv)) + for _, ps := range pv { + out = append(out, ParamSpec{ + Name: ps.Name, + Type: ps.Type, + Description: ps.Description, + Default: ps.Default, + }) + } + return out +} diff --git a/pkg/apis/pipeline/v1beta1/param_context_test.go b/pkg/apis/pipeline/v1beta1/param_context_test.go new file mode 100644 index 00000000000..c1dd6f6ec44 --- /dev/null +++ b/pkg/apis/pipeline/v1beta1/param_context_test.go @@ -0,0 +1,314 @@ +// Copyright 2021 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 v1beta1 + +import ( + "context" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + "github.com/tektoncd/pipeline/pkg/apis/config" +) + +func TestAddContextParams(t *testing.T) { + ctx := context.Background() + + t.Run("no-alpha", func(t *testing.T) { + ctx := AddContextParams(ctx, []Param{{Name: "a"}}) + if v := ctx.Value(paramCtxKey); v != nil { + t.Errorf("expected no param context values, got %v", v) + } + }) + + // Enable Alpha features. + cfg := config.FromContextOrDefaults(ctx) + cfg.FeatureFlags = &config.FeatureFlags{EnableAPIFields: "alpha"} + ctx = config.ToContext(ctx, cfg) + + // These test cases should run sequentially. Each step will modify the + // above context. + for _, tc := range []struct { + name string + params []Param + want paramCtxVal + }{ + { + name: "add-string-param", + params: []Param{{Name: "a", Value: *NewArrayOrString("foo")}}, + want: paramCtxVal{ + "a": ParamSpec{ + Name: "a", + Type: ParamTypeString, + }, + }, + }, + { + name: "add-array-param", + params: []Param{{Name: "b", Value: *NewArrayOrString("bar", "baz")}}, + want: paramCtxVal{ + "a": ParamSpec{ + Name: "a", + Type: ParamTypeString, + }, + "b": ParamSpec{ + Name: "b", + Type: ParamTypeArray, + }, + }, + }, + { + name: "existing-param", + params: []Param{ + {Name: "a", Value: *NewArrayOrString("foo1")}, + {Name: "b", Value: *NewArrayOrString("bar1", "baz1")}, + }, + want: paramCtxVal{ + "a": ParamSpec{ + Name: "a", + Type: ParamTypeString, + }, + "b": ParamSpec{ + Name: "b", + Type: ParamTypeArray, + }, + }, + }, + { + // This test case doesn't really make sense for typical use-cases, + // but exists to document the behavior of how this would be + // handled. The param context is simply responsible for propagating + // the param values through, regardless of their underlying value. + // Later validation should make sure these values make sense. + name: "empty-param", + params: []Param{{}}, + want: paramCtxVal{ + "": ParamSpec{}, + "a": ParamSpec{ + Name: "a", + Type: ParamTypeString, + }, + "b": ParamSpec{ + Name: "b", + Type: ParamTypeArray, + }, + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + ctx = AddContextParams(ctx, tc.params) + got := ctx.Value(paramCtxKey) + if diff := cmp.Diff(tc.want, got); diff != "" { + t.Errorf("-want,+got: %s", diff) + } + }) + } +} + +func TestAddContextParamSpec(t *testing.T) { + ctx := context.Background() + + t.Run("no-alpha", func(t *testing.T) { + ctx := AddContextParamSpec(ctx, []ParamSpec{{Name: "a"}}) + if v := ctx.Value(paramCtxKey); v != nil { + t.Errorf("expected no param context values, got %v", v) + } + }) + + // Enable Alpha features. + cfg := config.FromContextOrDefaults(ctx) + cfg.FeatureFlags = &config.FeatureFlags{EnableAPIFields: "alpha"} + ctx = config.ToContext(ctx, cfg) + + // These test cases should run sequentially. Each step will modify the + // above context. + for _, tc := range []struct { + name string + params []ParamSpec + want paramCtxVal + }{ + { + name: "add-paramspec", + params: []ParamSpec{{ + Name: "a", + }}, + want: paramCtxVal{ + "a": ParamSpec{ + Name: "a", + }, + }, + }, + { + name: "edit-paramspec", + params: []ParamSpec{{ + Name: "a", + Type: ParamTypeArray, + Default: NewArrayOrString("foo", "bar"), + Description: "tacocat", + }}, + want: paramCtxVal{ + "a": ParamSpec{ + Name: "a", + Type: ParamTypeArray, + Default: NewArrayOrString("foo", "bar"), + Description: "tacocat", + }, + }, + }, + { + // This test case doesn't really make sense for typical use-cases, + // but exists to document the behavior of how this would be + // handled. The param context is simply responsible for propagating + // the ParamSpec values through, regardless of their underlying + // value. Later validation should make sure these values make + // sense. + name: "empty-param", + params: []ParamSpec{{}}, + want: paramCtxVal{ + "": ParamSpec{}, + "a": ParamSpec{ + Name: "a", + Type: ParamTypeArray, + Default: NewArrayOrString("foo", "bar"), + Description: "tacocat", + }, + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + ctx = AddContextParamSpec(ctx, tc.params) + got := ctx.Value(paramCtxKey) + if diff := cmp.Diff(tc.want, got); diff != "" { + t.Errorf("-want,+got: %s", diff) + } + }) + } +} + +func TestGetContextParams(t *testing.T) { + ctx := context.Background() + want := []ParamSpec{ + { + Name: "a", + Type: ParamTypeString, + Default: NewArrayOrString("foo"), + Description: "tacocat", + }, + { + Name: "b", + Type: ParamTypeArray, + Default: NewArrayOrString("bar"), + Description: "racecar", + }, + } + t.Run("no-alpha", func(t *testing.T) { + ctx := AddContextParamSpec(ctx, want) + if v := GetContextParamSpecs(ctx); v != nil { + t.Errorf("expected no param context values, got %v", v) + } + }) + + // Enable Alpha features. + cfg := config.FromContextOrDefaults(ctx) + cfg.FeatureFlags = &config.FeatureFlags{EnableAPIFields: "alpha"} + ctx = config.ToContext(ctx, cfg) + + ctx = AddContextParamSpec(ctx, want) + + for _, tc := range []struct { + name string + overlay []Param + want []Param + }{ + { + name: "from-context", + want: []Param{ + { + Name: "a", + Value: *NewArrayOrString("$(params.a)"), + }, + { + Name: "b", + Value: ArrayOrString{ + Type: ParamTypeArray, + ArrayVal: []string{"$(params.b[*])"}, + }, + }, + }, + }, + { + name: "with-overlay", + overlay: []Param{{ + Name: "a", + Value: *NewArrayOrString("tacocat"), + }}, + want: []Param{ + { + Name: "a", + Value: *NewArrayOrString("tacocat"), + }, + { + Name: "b", + Value: ArrayOrString{ + Type: ParamTypeArray, + ArrayVal: []string{"$(params.b[*])"}, + }, + }, + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + got := GetContextParams(ctx, tc.overlay...) + if diff := cmp.Diff(tc.want, got, cmpopts.SortSlices(func(x, y Param) bool { return x.Name < y.Name })); diff != "" { + t.Errorf("-want,+got: %s", diff) + } + }) + } +} + +func TestGetContextParamSpecs(t *testing.T) { + ctx := context.Background() + want := []ParamSpec{ + { + Name: "a", + Type: ParamTypeString, + Default: NewArrayOrString("foo"), + Description: "tacocat", + }, + { + Name: "b", + Type: ParamTypeArray, + Default: NewArrayOrString("bar"), + Description: "racecar", + }, + } + t.Run("no-alpha", func(t *testing.T) { + ctx := AddContextParamSpec(ctx, want) + if v := GetContextParamSpecs(ctx); v != nil { + t.Errorf("expected no param context values, got %v", v) + } + }) + + // Enable Alpha features. + cfg := config.FromContextOrDefaults(ctx) + cfg.FeatureFlags = &config.FeatureFlags{EnableAPIFields: "alpha"} + ctx = config.ToContext(ctx, cfg) + + ctx = AddContextParamSpec(ctx, want) + got := GetContextParamSpecs(ctx) + if diff := cmp.Diff(want, got, cmpopts.SortSlices(func(x, y ParamSpec) bool { return x.Name < y.Name })); diff != "" { + t.Errorf("-want,+got: %s", diff) + } +} diff --git a/pkg/apis/pipeline/v1beta1/pipeline_defaults.go b/pkg/apis/pipeline/v1beta1/pipeline_defaults.go index b4cbb8fec11..6eb9698e372 100644 --- a/pkg/apis/pipeline/v1beta1/pipeline_defaults.go +++ b/pkg/apis/pipeline/v1beta1/pipeline_defaults.go @@ -19,6 +19,7 @@ package v1beta1 import ( "context" + "github.com/tektoncd/pipeline/pkg/apis/config" "knative.dev/pkg/apis" ) @@ -29,7 +30,19 @@ func (p *Pipeline) SetDefaults(ctx context.Context) { } func (ps *PipelineSpec) SetDefaults(ctx context.Context) { - for _, pt := range ps.Tasks { + for i := range ps.Params { + ps.Params[i].SetDefaults(ctx) + } + if config.FromContextOrDefaults(ctx).FeatureFlags.EnableAPIFields == "alpha" { + ctx = AddContextParamSpec(ctx, ps.Params) + ps.Params = GetContextParamSpecs(ctx) + } + for i, pt := range ps.Tasks { + ctx := ctx // Ensure local scoping per Task + if config.FromContextOrDefaults(ctx).FeatureFlags.EnableAPIFields == "alpha" { + ctx = AddContextParams(ctx, pt.Params) + ps.Tasks[i].Params = GetContextParams(ctx, pt.Params...) + } if pt.TaskRef != nil { if pt.TaskRef.Kind == "" { pt.TaskRef.Kind = NamespacedTaskKind @@ -39,10 +52,13 @@ func (ps *PipelineSpec) SetDefaults(ctx context.Context) { pt.TaskSpec.SetDefaults(ctx) } } - for i := range ps.Params { - ps.Params[i].SetDefaults(ctx) - } - for _, ft := range ps.Finally { + + for i, ft := range ps.Finally { + ctx := ctx // Ensure local scoping per Task + if config.FromContextOrDefaults(ctx).FeatureFlags.EnableAPIFields == "alpha" { + ctx = AddContextParams(ctx, ft.Params) + ps.Finally[i].Params = GetContextParams(ctx, ft.Params...) + } if ft.TaskRef != nil { if ft.TaskRef.Kind == "" { ft.TaskRef.Kind = NamespacedTaskKind diff --git a/pkg/apis/pipeline/v1beta1/pipelinerun_defaults.go b/pkg/apis/pipeline/v1beta1/pipelinerun_defaults.go index 703b1ba362b..b0858bd2ace 100644 --- a/pkg/apis/pipeline/v1beta1/pipelinerun_defaults.go +++ b/pkg/apis/pipeline/v1beta1/pipelinerun_defaults.go @@ -50,6 +50,9 @@ func (prs *PipelineRunSpec) SetDefaults(ctx context.Context) { prs.PodTemplate = mergePodTemplateWithDefault(prs.PodTemplate, defaultPodTemplate) if prs.PipelineSpec != nil { + if config.FromContextOrDefaults(ctx).FeatureFlags.EnableAPIFields == "alpha" { + ctx = AddContextParams(ctx, prs.Params) + } prs.PipelineSpec.SetDefaults(ctx) } } diff --git a/pkg/apis/pipeline/v1beta1/pipelinerun_defaults_test.go b/pkg/apis/pipeline/v1beta1/pipelinerun_defaults_test.go index d9cfbb5b60b..2e40cfe34a1 100644 --- a/pkg/apis/pipeline/v1beta1/pipelinerun_defaults_test.go +++ b/pkg/apis/pipeline/v1beta1/pipelinerun_defaults_test.go @@ -22,6 +22,7 @@ import ( "time" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "github.com/tektoncd/pipeline/pkg/apis/config" "github.com/tektoncd/pipeline/pkg/apis/pipeline/pod" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" @@ -34,9 +35,10 @@ import ( func TestPipelineRunSpec_SetDefaults(t *testing.T) { cases := []struct { - desc string - prs *v1beta1.PipelineRunSpec - want *v1beta1.PipelineRunSpec + desc string + prs *v1beta1.PipelineRunSpec + want *v1beta1.PipelineRunSpec + ctxFn func(context.Context) context.Context }{ { desc: "timeout is nil", @@ -83,18 +85,119 @@ func TestPipelineRunSpec_SetDefaults(t *testing.T) { }, }, }, + { + desc: "implicit params", + ctxFn: func(ctx context.Context) context.Context { + cfg := config.FromContextOrDefaults(ctx) + cfg.FeatureFlags = &config.FeatureFlags{EnableAPIFields: "alpha"} + return config.ToContext(ctx, cfg) + }, + prs: &v1beta1.PipelineRunSpec{ + Params: []v1beta1.Param{ + { + Name: "foo", + Value: v1beta1.ArrayOrString{ + StringVal: "a", + }, + }, + { + Name: "bar", + Value: v1beta1.ArrayOrString{ + ArrayVal: []string{"b"}, + }, + }, + }, + PipelineSpec: &v1beta1.PipelineSpec{ + Tasks: []v1beta1.PipelineTask{{ + TaskSpec: &v1beta1.EmbeddedTask{ + TaskSpec: v1beta1.TaskSpec{}, + }, + }}, + }, + }, + want: &v1beta1.PipelineRunSpec{ + ServiceAccountName: config.DefaultServiceAccountValue, + Timeout: &metav1.Duration{Duration: config.DefaultTimeoutMinutes * time.Minute}, + Params: []v1beta1.Param{ + { + Name: "foo", + Value: v1beta1.ArrayOrString{ + StringVal: "a", + }, + }, + { + Name: "bar", + Value: v1beta1.ArrayOrString{ + ArrayVal: []string{"b"}, + }, + }, + }, + PipelineSpec: &v1beta1.PipelineSpec{ + Tasks: []v1beta1.PipelineTask{{ + TaskSpec: &v1beta1.EmbeddedTask{ + TaskSpec: v1beta1.TaskSpec{ + Params: []v1beta1.ParamSpec{ + { + Name: "foo", + Type: v1beta1.ParamTypeString, + }, + { + Name: "bar", + Type: v1beta1.ParamTypeArray, + }, + }, + }, + }, + Params: []v1beta1.Param{ + { + Name: "foo", + Value: v1beta1.ArrayOrString{ + Type: v1beta1.ParamTypeString, + StringVal: "$(params.foo)", + }, + }, + { + Name: "bar", + Value: v1beta1.ArrayOrString{ + Type: v1beta1.ParamTypeArray, + ArrayVal: []string{"$(params.bar[*])"}, + }, + }, + }, + }}, + Params: []v1beta1.ParamSpec{ + { + Name: "foo", + Type: v1beta1.ParamTypeString, + }, + { + Name: "bar", + Type: v1beta1.ParamTypeArray, + }, + }, + }, + }, + }, } for _, tc := range cases { t.Run(tc.desc, func(t *testing.T) { ctx := context.Background() + if tc.ctxFn != nil { + ctx = tc.ctxFn(ctx) + } tc.prs.SetDefaults(ctx) - if d := cmp.Diff(tc.want, tc.prs); d != "" { + sortPS := func(x, y v1beta1.ParamSpec) bool { + return x.Name < y.Name + } + sortP := func(x, y v1beta1.Param) bool { + return x.Name < y.Name + } + if d := cmp.Diff(tc.want, tc.prs, cmpopts.SortSlices(sortPS), cmpopts.SortSlices(sortP)); d != "" { t.Errorf("Mismatch of PipelineRunSpec %s", diff.PrintWantGot(d)) } }) } - } func TestPipelineRunDefaulting(t *testing.T) { diff --git a/pkg/apis/pipeline/v1beta1/task_defaults.go b/pkg/apis/pipeline/v1beta1/task_defaults.go index c0399f34418..a7ed4aa02d4 100644 --- a/pkg/apis/pipeline/v1beta1/task_defaults.go +++ b/pkg/apis/pipeline/v1beta1/task_defaults.go @@ -19,6 +19,7 @@ package v1beta1 import ( "context" + "github.com/tektoncd/pipeline/pkg/apis/config" "knative.dev/pkg/apis" ) @@ -33,4 +34,8 @@ func (ts *TaskSpec) SetDefaults(ctx context.Context) { for i := range ts.Params { ts.Params[i].SetDefaults(ctx) } + if config.FromContextOrDefaults(ctx).FeatureFlags.EnableAPIFields == "alpha" { + ctx = AddContextParamSpec(ctx, ts.Params) + ts.Params = GetContextParamSpecs(ctx) + } } diff --git a/pkg/apis/pipeline/v1beta1/taskrun_defaults.go b/pkg/apis/pipeline/v1beta1/taskrun_defaults.go index 8fbcd3fd1c2..2a7e92be829 100644 --- a/pkg/apis/pipeline/v1beta1/taskrun_defaults.go +++ b/pkg/apis/pipeline/v1beta1/taskrun_defaults.go @@ -64,6 +64,9 @@ func (trs *TaskRunSpec) SetDefaults(ctx context.Context) { // If this taskrun has an embedded task, apply the usual task defaults if trs.TaskSpec != nil { + if config.FromContextOrDefaults(ctx).FeatureFlags.EnableAPIFields == "alpha" { + ctx = AddContextParams(ctx, trs.Params) + } trs.TaskSpec.SetDefaults(ctx) } } diff --git a/pkg/apis/pipeline/v1beta1/taskrun_defaults_test.go b/pkg/apis/pipeline/v1beta1/taskrun_defaults_test.go index afd736d6805..ad779e99170 100644 --- a/pkg/apis/pipeline/v1beta1/taskrun_defaults_test.go +++ b/pkg/apis/pipeline/v1beta1/taskrun_defaults_test.go @@ -39,9 +39,10 @@ var ( func TestTaskRunSpec_SetDefaults(t *testing.T) { cases := []struct { - desc string - trs *v1beta1.TaskRunSpec - want *v1beta1.TaskRunSpec + desc string + trs *v1beta1.TaskRunSpec + want *v1beta1.TaskRunSpec + ctxFn func(context.Context) context.Context }{{ desc: "taskref is nil", trs: &v1beta1.TaskRunSpec{ @@ -118,10 +119,79 @@ func TestTaskRunSpec_SetDefaults(t *testing.T) { ServiceAccountName: config.DefaultServiceAccountValue, Timeout: &metav1.Duration{Duration: config.DefaultTimeoutMinutes * time.Minute}, }, + }, { + desc: "implicit string param", + ctxFn: func(ctx context.Context) context.Context { + cfg := config.FromContextOrDefaults(ctx) + cfg.FeatureFlags = &config.FeatureFlags{EnableAPIFields: "alpha"} + return config.ToContext(ctx, cfg) + }, + trs: &v1beta1.TaskRunSpec{ + TaskSpec: &v1beta1.TaskSpec{}, + Params: []v1beta1.Param{{ + Name: "param-name", + Value: v1beta1.ArrayOrString{ + StringVal: "a", + }, + }}, + }, + want: &v1beta1.TaskRunSpec{ + TaskSpec: &v1beta1.TaskSpec{ + Params: []v1beta1.ParamSpec{{ + Name: "param-name", + Type: v1beta1.ParamTypeString, + }}, + }, + Params: []v1beta1.Param{{ + Name: "param-name", + Value: v1beta1.ArrayOrString{ + StringVal: "a", + }, + }}, + ServiceAccountName: config.DefaultServiceAccountValue, + Timeout: &metav1.Duration{Duration: config.DefaultTimeoutMinutes * time.Minute}, + }, + }, { + desc: "implicit array param", + ctxFn: func(ctx context.Context) context.Context { + cfg := config.FromContextOrDefaults(ctx) + cfg.FeatureFlags = &config.FeatureFlags{EnableAPIFields: "alpha"} + return config.ToContext(ctx, cfg) + }, + trs: &v1beta1.TaskRunSpec{ + TaskSpec: &v1beta1.TaskSpec{}, + Params: []v1beta1.Param{{ + Name: "param-name", + Value: v1beta1.ArrayOrString{ + Type: v1beta1.ParamTypeArray, + ArrayVal: []string{"a"}, + }, + }}, + }, + want: &v1beta1.TaskRunSpec{ + TaskSpec: &v1beta1.TaskSpec{ + Params: []v1beta1.ParamSpec{{ + Name: "param-name", + Type: v1beta1.ParamTypeArray, + }}, + }, + Params: []v1beta1.Param{{ + Name: "param-name", + Value: v1beta1.ArrayOrString{ + Type: v1beta1.ParamTypeArray, + ArrayVal: []string{"a"}, + }, + }}, + ServiceAccountName: config.DefaultServiceAccountValue, + Timeout: &metav1.Duration{Duration: config.DefaultTimeoutMinutes * time.Minute}, + }, }} for _, tc := range cases { t.Run(tc.desc, func(t *testing.T) { ctx := context.Background() + if tc.ctxFn != nil { + ctx = tc.ctxFn(ctx) + } tc.trs.SetDefaults(ctx) if d := cmp.Diff(tc.want, tc.trs); d != "" { diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index 092fb74314b..1fb6eb30a5d 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -496,7 +496,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun, get for _, rprt := range pipelineRunFacts.State { if !rprt.IsCustomTask() { - err := taskrun.ValidateResolvedTaskResources(rprt.PipelineTask.Params, rprt.ResolvedTaskResources) + err := taskrun.ValidateResolvedTaskResources(ctx, rprt.PipelineTask.Params, rprt.ResolvedTaskResources) if err != nil { logger.Errorf("Failed to validate pipelinerun %q with error %v", pr.Name, err) pr.Status.MarkFailed(ReasonFailedValidation, err.Error()) diff --git a/pkg/reconciler/taskrun/taskrun.go b/pkg/reconciler/taskrun/taskrun.go index 7261062cd26..20cd8691693 100644 --- a/pkg/reconciler/taskrun/taskrun.go +++ b/pkg/reconciler/taskrun/taskrun.go @@ -339,7 +339,7 @@ func (c *Reconciler) prepare(ctx context.Context, tr *v1beta1.TaskRun) (*v1beta1 return nil, nil, controller.NewPermanentError(err) } - if err := ValidateResolvedTaskResources(tr.Spec.Params, rtr); err != nil { + if err := ValidateResolvedTaskResources(ctx, tr.Spec.Params, rtr); err != nil { logger.Errorf("TaskRun %q resources are invalid: %v", tr.Name, err) tr.Status.MarkResourceFailed(podconvert.ReasonFailedValidation, err) return nil, nil, controller.NewPermanentError(err) diff --git a/pkg/reconciler/taskrun/validate_resources.go b/pkg/reconciler/taskrun/validate_resources.go index 370b2e32168..d7b38472971 100644 --- a/pkg/reconciler/taskrun/validate_resources.go +++ b/pkg/reconciler/taskrun/validate_resources.go @@ -17,8 +17,10 @@ limitations under the License. package taskrun import ( + "context" "fmt" + "github.com/tektoncd/pipeline/pkg/apis/config" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" resourcev1alpha1 "github.com/tektoncd/pipeline/pkg/apis/resource/v1alpha1" "github.com/tektoncd/pipeline/pkg/list" @@ -64,7 +66,7 @@ func validateResources(requiredResources []v1beta1.TaskResource, providedResourc return nil } -func validateParams(paramSpecs []v1beta1.ParamSpec, params []v1beta1.Param) error { +func validateParams(ctx context.Context, paramSpecs []v1beta1.ParamSpec, params []v1beta1.Param) error { var neededParams []string paramTypes := make(map[string]v1beta1.ParamType) neededParams = make([]string, 0, len(paramSpecs)) @@ -88,15 +90,27 @@ func validateParams(paramSpecs []v1beta1.ParamSpec, params []v1beta1.Param) erro if len(missingParamsNoDefaults) > 0 { return fmt.Errorf("missing values for these params which have no default values: %s", missingParamsNoDefaults) } - extraParams := list.DiffLeft(providedParams, neededParams) - if len(extraParams) != 0 { - return fmt.Errorf("didn't need these params but they were provided anyway: %s", extraParams) + // If alpha features are enabled, disable checking for extra params. + // Extra params are needed to support + // https://github.com/tektoncd/community/blob/main/teps/0023-implicit-mapping.md + // So that parent params can be passed down to subresources (even if they + // are not explicitly used). + if config.FromContextOrDefaults(ctx).FeatureFlags.EnableAPIFields != "alpha" { + extraParams := list.DiffLeft(providedParams, neededParams) + if len(extraParams) != 0 { + return fmt.Errorf("didn't need these params but they were provided anyway: %s", extraParams) + } } // Now that we have checked against missing/extra params, make sure each param's actual type matches // the user-specified type. var wrongTypeParamNames []string for _, param := range params { + if _, ok := paramTypes[param.Name]; !ok { + // Ignore any missing params - this happens when extra params were + // passed to the task that aren't being used. + continue + } if param.Value.Type != paramTypes[param.Name] { wrongTypeParamNames = append(wrongTypeParamNames, param.Name) } @@ -109,8 +123,8 @@ func validateParams(paramSpecs []v1beta1.ParamSpec, params []v1beta1.Param) erro } // ValidateResolvedTaskResources validates task inputs, params and output matches taskrun -func ValidateResolvedTaskResources(params []v1beta1.Param, rtr *resources.ResolvedTaskResources) error { - if err := validateParams(rtr.TaskSpec.Params, params); err != nil { +func ValidateResolvedTaskResources(ctx context.Context, params []v1beta1.Param, rtr *resources.ResolvedTaskResources) error { + if err := validateParams(ctx, rtr.TaskSpec.Params, params); err != nil { return fmt.Errorf("invalid input params for task %s: %w", rtr.TaskName, err) } inputs := []v1beta1.TaskResource{} diff --git a/pkg/reconciler/taskrun/validate_resources_test.go b/pkg/reconciler/taskrun/validate_resources_test.go index 2ca9a917202..251b347172a 100644 --- a/pkg/reconciler/taskrun/validate_resources_test.go +++ b/pkg/reconciler/taskrun/validate_resources_test.go @@ -17,9 +17,11 @@ limitations under the License. package taskrun_test import ( + "context" "testing" tb "github.com/tektoncd/pipeline/internal/builder/v1beta1" + "github.com/tektoncd/pipeline/pkg/apis/config" resourcev1alpha1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" "github.com/tektoncd/pipeline/pkg/reconciler/taskrun" @@ -27,6 +29,7 @@ import ( ) func TestValidateResolvedTaskResources_ValidResources(t *testing.T) { + ctx := context.Background() task := tb.Task("foo", tb.TaskSpec( tb.Step("myimage", tb.StepCommand("mycmd")), tb.TaskResources( @@ -55,12 +58,13 @@ func TestValidateResolvedTaskResources_ValidResources(t *testing.T) { tb.PipelineResourceSpec(resourcev1alpha1.PipelineResourceTypeImage)), }, } - if err := taskrun.ValidateResolvedTaskResources([]v1beta1.Param{}, rtr); err != nil { + if err := taskrun.ValidateResolvedTaskResources(ctx, []v1beta1.Param{}, rtr); err != nil { t.Fatalf("Did not expect to see error when validating valid resolved TaskRun but saw %v", err) } } func TestValidateResolvedTaskResources_ValidParams(t *testing.T) { + ctx := context.Background() task := tb.Task("foo", tb.TaskSpec( tb.Step("myimage", tb.StepCommand("mycmd")), tb.TaskParam("foo", v1beta1.ParamTypeString), @@ -76,12 +80,24 @@ func TestValidateResolvedTaskResources_ValidParams(t *testing.T) { Name: "bar", Value: *v1beta1.NewArrayOrString("somethinggood"), }} - if err := taskrun.ValidateResolvedTaskResources(p, rtr); err != nil { + if err := taskrun.ValidateResolvedTaskResources(ctx, p, rtr); err != nil { t.Fatalf("Did not expect to see error when validating TaskRun with correct params but saw %v", err) } + + t.Run("alpha-extra-params", func(t *testing.T) { + ctx := config.ToContext(ctx, &config.Config{FeatureFlags: &config.FeatureFlags{EnableAPIFields: "alpha"}}) + extra := v1beta1.Param{ + Name: "extra", + Value: *v1beta1.NewArrayOrString("i am an extra param"), + } + if err := taskrun.ValidateResolvedTaskResources(ctx, append(p, extra), rtr); err != nil { + t.Fatalf("Did not expect to see error when validating TaskRun with correct params but saw %v", err) + } + }) } func TestValidateResolvedTaskResources_InvalidParams(t *testing.T) { + ctx := context.Background() task := tb.Task("foo", tb.TaskSpec( tb.Step("myimage", tb.StepCommand("mycmd")), tb.TaskParam("foo", v1beta1.ParamTypeString), @@ -99,22 +115,10 @@ func TestValidateResolvedTaskResources_InvalidParams(t *testing.T) { Name: "foobar", Value: *v1beta1.NewArrayOrString("somethingfun"), }}, - }, { - name: "missing-params", - rtr: &resources.ResolvedTaskResources{ - TaskSpec: &task.Spec, - }, - params: []v1beta1.Param{{ - Name: "foo", - Value: *v1beta1.NewArrayOrString("i am a real param"), - }, { - Name: "extra", - Value: *v1beta1.NewArrayOrString("i am an extra param"), - }}, }} for _, tc := range tcs { t.Run(tc.name, func(t *testing.T) { - if err := taskrun.ValidateResolvedTaskResources(tc.params, tc.rtr); err == nil { + if err := taskrun.ValidateResolvedTaskResources(ctx, tc.params, tc.rtr); err == nil { t.Errorf("Expected to see error when validating invalid resolved TaskRun with wrong params but saw none") } }) @@ -122,6 +126,7 @@ func TestValidateResolvedTaskResources_InvalidParams(t *testing.T) { } func TestValidateResolvedTaskResources_InvalidResources(t *testing.T) { + ctx := context.Background() r := tb.PipelineResource("git-test-resource", tb.PipelineResourceSpec( resourcev1alpha1.PipelineResourceTypeGit, tb.PipelineResourceSpecParam("foo", "bar"), @@ -245,7 +250,7 @@ func TestValidateResolvedTaskResources_InvalidResources(t *testing.T) { for _, tc := range tcs { t.Run(tc.name, func(t *testing.T) { - if err := taskrun.ValidateResolvedTaskResources([]v1beta1.Param{}, tc.rtr); err == nil { + if err := taskrun.ValidateResolvedTaskResources(ctx, []v1beta1.Param{}, tc.rtr); err == nil { t.Errorf("Expected to see error when validating invalid resolved TaskRun but saw none") } })