Skip to content

Commit

Permalink
Disable implicit param behavior for Pipeline objects.
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@wlynch
wlynch committed Jan 24, 2022
1 parent f97df42 commit e2af920
Show file tree
Hide file tree
Showing 9 changed files with 533 additions and 271 deletions.
473 changes: 473 additions & 0 deletions pkg/apis/pipeline/v1beta1/implicit_params_test.go
View file

Large diffs are not rendered by default.

38 changes: 32 additions & 6 deletions pkg/apis/pipeline/v1beta1/param_context.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,22 @@ package v1beta1
import (
"context"
"fmt"

"github.com/tektoncd/pipeline/pkg/apis/config"
)

// paramCtxKey is the unique identifier for referencing param information from
// enableImplicitParamContextKeyType is the unique type for referencing whether
// Implicit Param behavior is enabled for the given request context.
// See [TEP-0023](https://github.com/tektoncd/community/blob/main/teps/0023-implicit-mapping.md).
type enableImplicitParamContextKeyType struct{}

// paramCtxKey is the unique type for referencing param information from
// a context.Context. See [context.Context.Value](https://pkg.go.dev/context#Context)
// for more details.
var paramCtxKey struct{}
type paramCtxKeyType struct{}

var (
enableImplicitParamContextKey = enableImplicitParamContextKeyType{}
paramCtxKey = paramCtxKeyType{}
)

// paramCtxVal is the data type stored in the param context.
// This maps param names -> ParamSpec.
Expand All @@ -37,7 +45,7 @@ func addContextParams(ctx context.Context, in []Param) context.Context {
return ctx
}

if config.FromContextOrDefaults(ctx).FeatureFlags.EnableAPIFields != "alpha" {
if !GetImplicitParamsEnabled(ctx) {
return ctx
}

Expand Down Expand Up @@ -73,7 +81,7 @@ func addContextParamSpec(ctx context.Context, in []ParamSpec) context.Context {
return ctx
}

if config.FromContextOrDefaults(ctx).FeatureFlags.EnableAPIFields != "alpha" {
if !GetImplicitParamsEnabled(ctx) {
return ctx
}

Expand Down Expand Up @@ -170,3 +178,21 @@ func getContextParamSpecs(ctx context.Context) []ParamSpec {
}
return out
}

// WithImplicitParamsEnabled enables implicit parameter behavior for the
// request context. If enabled, parameters will propagate down embedded request
// objects (e.g. PipelineRun -> Pipeline -> Task, Pipeline -> Task,
// TaskRun -> Task) during defaulting.
func WithImplicitParamsEnabled(ctx context.Context, v bool) context.Context {
return context.WithValue(ctx, enableImplicitParamContextKey, v)
}

// GetImplicitParamsEnabled returns whether implicit parameters are enabled for
// the given context.
func GetImplicitParamsEnabled(ctx context.Context) bool {
v := ctx.Value(enableImplicitParamContextKey)
if v == nil {
return false
}
return v.(bool)
}
25 changes: 8 additions & 17 deletions pkg/apis/pipeline/v1beta1/param_context_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,23 +20,20 @@ import (

"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) {
t.Run("not-enabled", 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)
ctx = WithImplicitParamsEnabled(ctx, true)

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

t.Run("no-alpha", func(t *testing.T) {
t.Run("not-enabled", 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)
ctx = WithImplicitParamsEnabled(ctx, true)

// These test cases should run sequentially. Each step will modify the
// above context.
Expand Down Expand Up @@ -213,17 +208,15 @@ func TestGetContextParams(t *testing.T) {
Description: "racecar",
},
}
t.Run("no-alpha", func(t *testing.T) {
t.Run("not-enabled", 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 = WithImplicitParamsEnabled(ctx, true)

ctx = addContextParamSpec(ctx, want)

Expand Down Expand Up @@ -294,17 +287,15 @@ func TestGetContextParamSpecs(t *testing.T) {
Description: "racecar",
},
}
t.Run("no-alpha", func(t *testing.T) {
t.Run("not-enabled", 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 = WithImplicitParamsEnabled(ctx, true)

ctx = addContextParamSpec(ctx, want)
got := getContextParamSpecs(ctx)
Expand Down
7 changes: 3 additions & 4 deletions pkg/apis/pipeline/v1beta1/pipeline_defaults.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ package v1beta1
import (
"context"

"github.com/tektoncd/pipeline/pkg/apis/config"
"knative.dev/pkg/apis"
)

Expand All @@ -35,7 +34,7 @@ func (ps *PipelineSpec) SetDefaults(ctx context.Context) {
for i := range ps.Params {
ps.Params[i].SetDefaults(ctx)
}
if config.FromContextOrDefaults(ctx).FeatureFlags.EnableAPIFields == "alpha" {
if GetImplicitParamsEnabled(ctx) {
ctx = addContextParamSpec(ctx, ps.Params)
ps.Params = getContextParamSpecs(ctx)
}
Expand All @@ -50,7 +49,7 @@ func (ps *PipelineSpec) SetDefaults(ctx context.Context) {
if pt.TaskSpec != nil {
// Only propagate param context to the spec - ref params should
// still be explicitly set.
if config.FromContextOrDefaults(ctx).FeatureFlags.EnableAPIFields == "alpha" {
if GetImplicitParamsEnabled(ctx) {
ctx = addContextParams(ctx, pt.Params)
ps.Tasks[i].Params = getContextParams(ctx, pt.Params...)
}
Expand All @@ -66,7 +65,7 @@ func (ps *PipelineSpec) SetDefaults(ctx context.Context) {
}
}
if ft.TaskSpec != nil {
if config.FromContextOrDefaults(ctx).FeatureFlags.EnableAPIFields == "alpha" {
if GetImplicitParamsEnabled(ctx) {
ctx = addContextParams(ctx, ft.Params)
ps.Finally[i].Params = getContextParams(ctx, ft.Params...)
}
Expand Down
6 changes: 5 additions & 1 deletion pkg/apis/pipeline/v1beta1/pipelinerun_defaults.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@ func (pr *PipelineRun) SetDefaults(ctx context.Context) {

// SetDefaults implements apis.Defaultable
func (prs *PipelineRunSpec) SetDefaults(ctx context.Context) {
if config.FromContextOrDefaults(ctx).FeatureFlags.EnableAPIFields == "alpha" {
ctx = WithImplicitParamsEnabled(ctx, true)
}

cfg := config.FromContextOrDefaults(ctx)
if prs.Timeout == nil && prs.Timeouts == nil {
prs.Timeout = &metav1.Duration{Duration: time.Duration(cfg.Defaults.DefaultTimeoutMinutes) * time.Minute}
Expand All @@ -52,7 +56,7 @@ func (prs *PipelineRunSpec) SetDefaults(ctx context.Context) {
prs.PodTemplate = mergePodTemplateWithDefault(prs.PodTemplate, defaultPodTemplate)

if prs.PipelineSpec != nil {
if config.FromContextOrDefaults(ctx).FeatureFlags.EnableAPIFields == "alpha" {
if GetImplicitParamsEnabled(ctx) {
ctx = addContextParams(ctx, prs.Params)
}
prs.PipelineSpec.SetDefaults(ctx)
Expand Down
170 changes: 3 additions & 167 deletions pkg/apis/pipeline/v1beta1/pipelinerun_defaults_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,9 @@ import (

func TestPipelineRunSpec_SetDefaults(t *testing.T) {
cases := []struct {
desc string
prs *v1beta1.PipelineRunSpec
want *v1beta1.PipelineRunSpec
ctxFn func(context.Context) context.Context
desc string
prs *v1beta1.PipelineRunSpec
want *v1beta1.PipelineRunSpec
}{
{
desc: "timeout is nil",
Expand Down Expand Up @@ -84,173 +83,10 @@ 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{},
},
},
{
TaskRef: &v1beta1.TaskRef{
Name: "baz",
},
},
},
Finally: []v1beta1.PipelineTask{
{
TaskSpec: &v1beta1.EmbeddedTask{
TaskSpec: v1beta1.TaskSpec{},
},
},
{
TaskRef: &v1beta1.TaskRef{
Name: "baz",
},
},
},
},
},
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[*])"},
},
},
},
},
{
TaskRef: &v1beta1.TaskRef{
Name: "baz",
Kind: v1beta1.NamespacedTaskKind,
},
},
},
Finally: []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[*])"},
},
},
},
},
{
TaskRef: &v1beta1.TaskRef{
Name: "baz",
Kind: v1beta1.NamespacedTaskKind,
},
},
},
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)

sortParamSpecs := func(x, y v1beta1.ParamSpec) bool {
Expand Down
Loading

0 comments on commit e2af920

Please sign in to comment.