Skip to content

Commit 43b1b25

Browse files
wlynchtekton-robot
wlynch
authored and
tekton-robot
committed
Disable implicit param behavior for Pipeline objects.
We recently received feedback that the implicit param behavior implementation was not quite implemented as expected in the TEP - primarily, that the implicit behavior modifying Pipelines on admission was unexpected. As a short term measure, this change disables the implicit behavior for Pipelines, but keeps the PipelineRun and TaskRun behavior in place. This is implemented by introducing a new context variable, similar to the existing Alpha context behavior, that is only enabled on PipelineRun/TaskRun reconcile. This allows for minimially invasive changes while giving us the flexibility to revert this behavior or enable in other contexts such as during reconcile. On top of this: - Moves implicit param tests to their own file, adds test for Pipeline to cover the behavior in question. - Fixes bug with param context key using var struct{} instead of type struct{} -> using var associates the value to any key of struct{}, whereas type struct{} creates a new type that can be used by only that key. There's no indication that this was causing problems before.
1 parent 507d09d commit 43b1b25

File tree

9 files changed

+537
-271
lines changed

9 files changed

+537
-271
lines changed

‎pkg/apis/pipeline/v1beta1/implicit_params_test.go‎

Lines changed: 473 additions & 0 deletions
Large diffs are not rendered by default.

‎pkg/apis/pipeline/v1beta1/param_context.go‎

Lines changed: 36 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -17,14 +17,26 @@ package v1beta1
1717
import (
1818
"context"
1919
"fmt"
20-
21-
"github.com/tektoncd/pipeline/pkg/apis/config"
2220
)
2321

24-
// paramCtxKey is the unique identifier for referencing param information from
22+
// enableImplicitParamContextKeyType is the unique type for referencing whether
23+
// Implicit Param behavior is enabled for the given request context.
24+
// See [TEP-0023](https://github.com/tektoncd/community/blob/main/teps/0023-implicit-mapping.md).
25+
//
26+
// +k8s:openapi-gen=false
27+
type enableImplicitParamContextKeyType struct{}
28+
29+
// paramCtxKey is the unique type for referencing param information from
2530
// a context.Context. See [context.Context.Value](https://pkg.go.dev/context#Context)
2631
// for more details.
27-
var paramCtxKey struct{}
32+
//
33+
// +k8s:openapi-gen=false
34+
type paramCtxKeyType struct{}
35+
36+
var (
37+
enableImplicitParamContextKey = enableImplicitParamContextKeyType{}
38+
paramCtxKey = paramCtxKeyType{}
39+
)
2840

2941
// paramCtxVal is the data type stored in the param context.
3042
// This maps param names -> ParamSpec.
@@ -37,7 +49,7 @@ func addContextParams(ctx context.Context, in []Param) context.Context {
3749
return ctx
3850
}
3951

40-
if config.FromContextOrDefaults(ctx).FeatureFlags.EnableAPIFields != "alpha" {
52+
if !GetImplicitParamsEnabled(ctx) {
4153
return ctx
4254
}
4355

@@ -73,7 +85,7 @@ func addContextParamSpec(ctx context.Context, in []ParamSpec) context.Context {
7385
return ctx
7486
}
7587

76-
if config.FromContextOrDefaults(ctx).FeatureFlags.EnableAPIFields != "alpha" {
88+
if !GetImplicitParamsEnabled(ctx) {
7789
return ctx
7890
}
7991

@@ -170,3 +182,21 @@ func getContextParamSpecs(ctx context.Context) []ParamSpec {
170182
}
171183
return out
172184
}
185+
186+
// WithImplicitParamsEnabled enables implicit parameter behavior for the
187+
// request context. If enabled, parameters will propagate down embedded request
188+
// objects (e.g. PipelineRun -> Pipeline -> Task, Pipeline -> Task,
189+
// TaskRun -> Task) during defaulting.
190+
func WithImplicitParamsEnabled(ctx context.Context, v bool) context.Context {
191+
return context.WithValue(ctx, enableImplicitParamContextKey, v)
192+
}
193+
194+
// GetImplicitParamsEnabled returns whether implicit parameters are enabled for
195+
// the given context.
196+
func GetImplicitParamsEnabled(ctx context.Context) bool {
197+
v := ctx.Value(enableImplicitParamContextKey)
198+
if v == nil {
199+
return false
200+
}
201+
return v.(bool)
202+
}

‎pkg/apis/pipeline/v1beta1/param_context_test.go‎

Lines changed: 8 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -20,23 +20,20 @@ import (
2020

2121
"github.com/google/go-cmp/cmp"
2222
"github.com/google/go-cmp/cmp/cmpopts"
23-
"github.com/tektoncd/pipeline/pkg/apis/config"
2423
)
2524

2625
func TestAddContextParams(t *testing.T) {
2726
ctx := context.Background()
2827

29-
t.Run("no-alpha", func(t *testing.T) {
28+
t.Run("not-enabled", func(t *testing.T) {
3029
ctx := addContextParams(ctx, []Param{{Name: "a"}})
3130
if v := ctx.Value(paramCtxKey); v != nil {
3231
t.Errorf("expected no param context values, got %v", v)
3332
}
3433
})
3534

3635
// Enable Alpha features.
37-
cfg := config.FromContextOrDefaults(ctx)
38-
cfg.FeatureFlags = &config.FeatureFlags{EnableAPIFields: "alpha"}
39-
ctx = config.ToContext(ctx, cfg)
36+
ctx = WithImplicitParamsEnabled(ctx, true)
4037

4138
// These test cases should run sequentially. Each step will modify the
4239
// above context.
@@ -120,17 +117,15 @@ func TestAddContextParams(t *testing.T) {
120117
func TestAddContextParamSpec(t *testing.T) {
121118
ctx := context.Background()
122119

123-
t.Run("no-alpha", func(t *testing.T) {
120+
t.Run("not-enabled", func(t *testing.T) {
124121
ctx := addContextParamSpec(ctx, []ParamSpec{{Name: "a"}})
125122
if v := ctx.Value(paramCtxKey); v != nil {
126123
t.Errorf("expected no param context values, got %v", v)
127124
}
128125
})
129126

130127
// Enable Alpha features.
131-
cfg := config.FromContextOrDefaults(ctx)
132-
cfg.FeatureFlags = &config.FeatureFlags{EnableAPIFields: "alpha"}
133-
ctx = config.ToContext(ctx, cfg)
128+
ctx = WithImplicitParamsEnabled(ctx, true)
134129

135130
// These test cases should run sequentially. Each step will modify the
136131
// above context.
@@ -213,17 +208,15 @@ func TestGetContextParams(t *testing.T) {
213208
Description: "racecar",
214209
},
215210
}
216-
t.Run("no-alpha", func(t *testing.T) {
211+
t.Run("not-enabled", func(t *testing.T) {
217212
ctx := addContextParamSpec(ctx, want)
218213
if v := getContextParamSpecs(ctx); v != nil {
219214
t.Errorf("expected no param context values, got %v", v)
220215
}
221216
})
222217

223218
// Enable Alpha features.
224-
cfg := config.FromContextOrDefaults(ctx)
225-
cfg.FeatureFlags = &config.FeatureFlags{EnableAPIFields: "alpha"}
226-
ctx = config.ToContext(ctx, cfg)
219+
ctx = WithImplicitParamsEnabled(ctx, true)
227220

228221
ctx = addContextParamSpec(ctx, want)
229222

@@ -294,17 +287,15 @@ func TestGetContextParamSpecs(t *testing.T) {
294287
Description: "racecar",
295288
},
296289
}
297-
t.Run("no-alpha", func(t *testing.T) {
290+
t.Run("not-enabled", func(t *testing.T) {
298291
ctx := addContextParamSpec(ctx, want)
299292
if v := getContextParamSpecs(ctx); v != nil {
300293
t.Errorf("expected no param context values, got %v", v)
301294
}
302295
})
303296

304297
// Enable Alpha features.
305-
cfg := config.FromContextOrDefaults(ctx)
306-
cfg.FeatureFlags = &config.FeatureFlags{EnableAPIFields: "alpha"}
307-
ctx = config.ToContext(ctx, cfg)
298+
ctx = WithImplicitParamsEnabled(ctx, true)
308299

309300
ctx = addContextParamSpec(ctx, want)
310301
got := getContextParamSpecs(ctx)

‎pkg/apis/pipeline/v1beta1/pipeline_defaults.go‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ package v1beta1
1919
import (
2020
"context"
2121

22-
"github.com/tektoncd/pipeline/pkg/apis/config"
2322
"knative.dev/pkg/apis"
2423
)
2524

@@ -35,7 +34,7 @@ func (ps *PipelineSpec) SetDefaults(ctx context.Context) {
3534
for i := range ps.Params {
3635
ps.Params[i].SetDefaults(ctx)
3736
}
38-
if config.FromContextOrDefaults(ctx).FeatureFlags.EnableAPIFields == "alpha" {
37+
if GetImplicitParamsEnabled(ctx) {
3938
ctx = addContextParamSpec(ctx, ps.Params)
4039
ps.Params = getContextParamSpecs(ctx)
4140
}
@@ -50,7 +49,7 @@ func (ps *PipelineSpec) SetDefaults(ctx context.Context) {
5049
if pt.TaskSpec != nil {
5150
// Only propagate param context to the spec - ref params should
5251
// still be explicitly set.
53-
if config.FromContextOrDefaults(ctx).FeatureFlags.EnableAPIFields == "alpha" {
52+
if GetImplicitParamsEnabled(ctx) {
5453
ctx = addContextParams(ctx, pt.Params)
5554
ps.Tasks[i].Params = getContextParams(ctx, pt.Params...)
5655
}
@@ -66,7 +65,7 @@ func (ps *PipelineSpec) SetDefaults(ctx context.Context) {
6665
}
6766
}
6867
if ft.TaskSpec != nil {
69-
if config.FromContextOrDefaults(ctx).FeatureFlags.EnableAPIFields == "alpha" {
68+
if GetImplicitParamsEnabled(ctx) {
7069
ctx = addContextParams(ctx, ft.Params)
7170
ps.Finally[i].Params = getContextParams(ctx, ft.Params...)
7271
}

‎pkg/apis/pipeline/v1beta1/pipelinerun_defaults.go‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,10 @@ func (pr *PipelineRun) SetDefaults(ctx context.Context) {
3434

3535
// SetDefaults implements apis.Defaultable
3636
func (prs *PipelineRunSpec) SetDefaults(ctx context.Context) {
37+
if config.FromContextOrDefaults(ctx).FeatureFlags.EnableAPIFields == "alpha" {
38+
ctx = WithImplicitParamsEnabled(ctx, true)
39+
}
40+
3741
cfg := config.FromContextOrDefaults(ctx)
3842
if prs.Timeout == nil && prs.Timeouts == nil {
3943
prs.Timeout = &metav1.Duration{Duration: time.Duration(cfg.Defaults.DefaultTimeoutMinutes) * time.Minute}
@@ -52,7 +56,7 @@ func (prs *PipelineRunSpec) SetDefaults(ctx context.Context) {
5256
prs.PodTemplate = mergePodTemplateWithDefault(prs.PodTemplate, defaultPodTemplate)
5357

5458
if prs.PipelineSpec != nil {
55-
if config.FromContextOrDefaults(ctx).FeatureFlags.EnableAPIFields == "alpha" {
59+
if GetImplicitParamsEnabled(ctx) {
5660
ctx = addContextParams(ctx, prs.Params)
5761
}
5862
prs.PipelineSpec.SetDefaults(ctx)

‎pkg/apis/pipeline/v1beta1/pipelinerun_defaults_test.go‎

Lines changed: 3 additions & 167 deletions
Original file line numberDiff line numberDiff line change
@@ -34,10 +34,9 @@ import (
3434

3535
func TestPipelineRunSpec_SetDefaults(t *testing.T) {
3636
cases := []struct {
37-
desc string
38-
prs *v1beta1.PipelineRunSpec
39-
want *v1beta1.PipelineRunSpec
40-
ctxFn func(context.Context) context.Context
37+
desc string
38+
prs *v1beta1.PipelineRunSpec
39+
want *v1beta1.PipelineRunSpec
4140
}{
4241
{
4342
desc: "timeout is nil",
@@ -84,173 +83,10 @@ func TestPipelineRunSpec_SetDefaults(t *testing.T) {
8483
},
8584
},
8685
},
87-
{
88-
desc: "implicit params",
89-
ctxFn: func(ctx context.Context) context.Context {
90-
cfg := config.FromContextOrDefaults(ctx)
91-
cfg.FeatureFlags = &config.FeatureFlags{EnableAPIFields: "alpha"}
92-
return config.ToContext(ctx, cfg)
93-
},
94-
prs: &v1beta1.PipelineRunSpec{
95-
Params: []v1beta1.Param{
96-
{
97-
Name: "foo",
98-
Value: v1beta1.ArrayOrString{
99-
StringVal: "a",
100-
},
101-
},
102-
{
103-
Name: "bar",
104-
Value: v1beta1.ArrayOrString{
105-
ArrayVal: []string{"b"},
106-
},
107-
},
108-
},
109-
PipelineSpec: &v1beta1.PipelineSpec{
110-
Tasks: []v1beta1.PipelineTask{
111-
{
112-
TaskSpec: &v1beta1.EmbeddedTask{
113-
TaskSpec: v1beta1.TaskSpec{},
114-
},
115-
},
116-
{
117-
TaskRef: &v1beta1.TaskRef{
118-
Name: "baz",
119-
},
120-
},
121-
},
122-
Finally: []v1beta1.PipelineTask{
123-
{
124-
TaskSpec: &v1beta1.EmbeddedTask{
125-
TaskSpec: v1beta1.TaskSpec{},
126-
},
127-
},
128-
{
129-
TaskRef: &v1beta1.TaskRef{
130-
Name: "baz",
131-
},
132-
},
133-
},
134-
},
135-
},
136-
want: &v1beta1.PipelineRunSpec{
137-
ServiceAccountName: config.DefaultServiceAccountValue,
138-
Timeout: &metav1.Duration{Duration: config.DefaultTimeoutMinutes * time.Minute},
139-
Params: []v1beta1.Param{
140-
{
141-
Name: "foo",
142-
Value: v1beta1.ArrayOrString{
143-
StringVal: "a",
144-
},
145-
},
146-
{
147-
Name: "bar",
148-
Value: v1beta1.ArrayOrString{
149-
ArrayVal: []string{"b"},
150-
},
151-
},
152-
},
153-
PipelineSpec: &v1beta1.PipelineSpec{
154-
Tasks: []v1beta1.PipelineTask{
155-
{
156-
TaskSpec: &v1beta1.EmbeddedTask{
157-
TaskSpec: v1beta1.TaskSpec{
158-
Params: []v1beta1.ParamSpec{
159-
{
160-
Name: "foo",
161-
Type: v1beta1.ParamTypeString,
162-
},
163-
{
164-
Name: "bar",
165-
Type: v1beta1.ParamTypeArray,
166-
},
167-
},
168-
},
169-
},
170-
Params: []v1beta1.Param{
171-
{
172-
Name: "foo",
173-
Value: v1beta1.ArrayOrString{
174-
Type: v1beta1.ParamTypeString,
175-
StringVal: "$(params.foo)",
176-
},
177-
},
178-
{
179-
Name: "bar",
180-
Value: v1beta1.ArrayOrString{
181-
Type: v1beta1.ParamTypeArray,
182-
ArrayVal: []string{"$(params.bar[*])"},
183-
},
184-
},
185-
},
186-
},
187-
{
188-
TaskRef: &v1beta1.TaskRef{
189-
Name: "baz",
190-
Kind: v1beta1.NamespacedTaskKind,
191-
},
192-
},
193-
},
194-
Finally: []v1beta1.PipelineTask{
195-
{
196-
TaskSpec: &v1beta1.EmbeddedTask{
197-
TaskSpec: v1beta1.TaskSpec{
198-
Params: []v1beta1.ParamSpec{
199-
{
200-
Name: "foo",
201-
Type: v1beta1.ParamTypeString,
202-
},
203-
{
204-
Name: "bar",
205-
Type: v1beta1.ParamTypeArray,
206-
},
207-
},
208-
},
209-
},
210-
Params: []v1beta1.Param{
211-
{
212-
Name: "foo",
213-
Value: v1beta1.ArrayOrString{
214-
Type: v1beta1.ParamTypeString,
215-
StringVal: "$(params.foo)",
216-
},
217-
},
218-
{
219-
Name: "bar",
220-
Value: v1beta1.ArrayOrString{
221-
Type: v1beta1.ParamTypeArray,
222-
ArrayVal: []string{"$(params.bar[*])"},
223-
},
224-
},
225-
},
226-
},
227-
{
228-
TaskRef: &v1beta1.TaskRef{
229-
Name: "baz",
230-
Kind: v1beta1.NamespacedTaskKind,
231-
},
232-
},
233-
},
234-
Params: []v1beta1.ParamSpec{
235-
{
236-
Name: "foo",
237-
Type: v1beta1.ParamTypeString,
238-
},
239-
{
240-
Name: "bar",
241-
Type: v1beta1.ParamTypeArray,
242-
},
243-
},
244-
},
245-
},
246-
},
24786
}
24887
for _, tc := range cases {
24988
t.Run(tc.desc, func(t *testing.T) {
25089
ctx := context.Background()
251-
if tc.ctxFn != nil {
252-
ctx = tc.ctxFn(ctx)
253-
}
25490
tc.prs.SetDefaults(ctx)
25591

25692
sortParamSpecs := func(x, y v1beta1.ParamSpec) bool {

0 commit comments

Comments
 (0)