Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TEP-0144] Validate TaskRun for Param Enum #7326

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 12 additions & 0 deletions docs/taskruns.md
Original file line number Diff line number Diff line change
Expand Up @@ -402,6 +402,18 @@ case is when your CI system autogenerates `TaskRuns` and it has `Parameters` it
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.

#### Parameter Enums
QuanZhang-William marked this conversation as resolved.
Show resolved Hide resolved

> :seedling: **Specifying `enum` is an [alpha](additional-configs.md#alpha-features) feature.** The `enable-param-enum` feature flag must be set to `"true"` to enable this feature.

> :seedling: This feature is WIP and not yet supported/implemented. Documentation to be completed.

If a `Parameter` is guarded by `Enum` in the `Task`, you can only provide `Parameter` values in the `TaskRun` that are predefined in the `Param.Enum` in the `Task`. The `TaskRun` will fail with reason `InvalidParamValue` otherwise.

You can also specify `Enum` for [`TaskRun` with an embedded `Task`](#example-taskrun-with-an-embedded-task). The same param validation will be executed in this scenario.

See more details in [Param.Enum](./tasks.md#param-enum).

### Specifying `Resource` limits

Each Step in a Task can specify its resource requirements. See
Expand Down
23 changes: 22 additions & 1 deletion docs/tasks.md
Original file line number Diff line number Diff line change
Expand Up @@ -717,7 +717,28 @@ spec:

> :seedling: This feature is WIP and not yet supported/implemented. Documentation to be completed.

Parameter declarations can include `enum` which is a predefine set of valid values that can be accepted by the `Task`.
Parameter declarations can include `enum` which is a predefine set of valid values that can be accepted by the `Param`. For example, the valid/allowed values for `Param` "message" is bounded to `v1`, `v2` and `v3`:

``` yaml
apiVersion: tekton.dev/v1
kind: Task
metadata:
name: param-enum-demo
spec:
params:
- name: message
type: string
enum: ["v1", "v2", "v3"]
steps:
- name: build
image: bash:latest
script: |
echo "$(params.message)"
```

If the `Param` value passed in by `TaskRuns` is **NOT** in the predefined `enum` list, the `TaskRuns` will fail with reason `InvalidParamValue`.

See usage in this [example](../examples/v1/taskruns/alpha/param-enum.yaml)

### Specifying `Workspaces`

Expand Down
25 changes: 25 additions & 0 deletions examples/v1/taskruns/alpha/param-enum.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
apiVersion: tekton.dev/v1
kind: Task
metadata:
name: task-param-enum
spec:
params:
- name: message
enum: ["v1", "v2", "v3"]
default: "v1"
steps:
- name: build
image: bash:3.2
script: |
echo "$(params.message)"
---
apiVersion: tekton.dev/v1
kind: TaskRun
metadata:
name: taskrun-param-enum
spec:
taskRef:
name: task-param-enum
params:
- name: message
value: "v1"
6 changes: 6 additions & 0 deletions pkg/apis/pipeline/v1/param_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"github.com/tektoncd/pipeline/pkg/substitution"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/utils/strings/slices"
"knative.dev/pkg/apis"
)

Expand Down Expand Up @@ -161,6 +162,11 @@ func (ps ParamSpecs) validateParamEnums(ctx context.Context) *apis.FieldError {
for dup := range findDups(p.Enum) {
errs = errs.Also(apis.ErrGeneric(fmt.Sprintf("parameter enum value %v appears more than once", dup), "").ViaKey(p.Name))
}
if p.Default != nil && p.Default.StringVal != "" {
if !slices.Contains(p.Enum, p.Default.StringVal) {
errs = errs.Also(apis.ErrGeneric(fmt.Sprintf("param default value %v not in the enum list", p.Default.StringVal), "").ViaKey(p.Name))
}
}
}
return errs
}
Expand Down
15 changes: 15 additions & 0 deletions pkg/apis/pipeline/v1/task_validation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2300,6 +2300,21 @@ func TestParamEnum_Failure(t *testing.T) {
configMap map[string]string
expectedErr error
}{{
name: "param default val not in enum list - failure",
params: []v1.ParamSpec{{
Name: "param1",
Type: v1.ParamTypeString,
Default: &v1.ParamValue{
Type: v1.ParamTypeString,
StringVal: "v4",
},
Enum: []string{"v1", "v2"},
}},
configMap: map[string]string{
"enable-param-enum": "true",
},
expectedErr: fmt.Errorf("param default value v4 not in the enum list: params[param1]"),
}, {
name: "param enum with array type - failure",
params: []v1.ParamSpec{{
Name: "param1",
Expand Down
2 changes: 2 additions & 0 deletions pkg/apis/pipeline/v1/taskrun_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,8 @@ const (
TaskRunReasonResultLargerThanAllowedLimit TaskRunReason = "TaskRunResultLargerThanAllowedLimit"
// TaskRunReasonStopSidecarFailed indicates that the sidecar is not properly stopped.
TaskRunReasonStopSidecarFailed = "TaskRunStopSidecarFailed"
// TaskRunReasonInvalidParamValue indicates that the TaskRun Param input value is not allowed.
TaskRunReasonInvalidParamValue = "InvalidParamValue"
)

func (t TaskRunReason) String() string {
Expand Down
6 changes: 6 additions & 0 deletions pkg/apis/pipeline/v1beta1/param_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"github.com/tektoncd/pipeline/pkg/substitution"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/utils/strings/slices"
"knative.dev/pkg/apis"
)

Expand Down Expand Up @@ -152,6 +153,11 @@ func (ps ParamSpecs) validateParamEnums(ctx context.Context) *apis.FieldError {
for dup := range findDups(p.Enum) {
errs = errs.Also(apis.ErrGeneric(fmt.Sprintf("parameter enum value %v appears more than once", dup), "").ViaKey(p.Name))
}
if p.Default != nil && p.Default.StringVal != "" {
if !slices.Contains(p.Enum, p.Default.StringVal) {
errs = errs.Also(apis.ErrGeneric(fmt.Sprintf("param default value %v not in the enum list", p.Default.StringVal), "").ViaKey(p.Name))
}
}
}
return errs
}
Expand Down
15 changes: 15 additions & 0 deletions pkg/apis/pipeline/v1beta1/task_validation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2137,6 +2137,21 @@ func TestParamEnum_Failure(t *testing.T) {
configMap map[string]string
expectedErr error
}{{
name: "param default val not in enum list - failure",
params: []v1beta1.ParamSpec{{
Name: "param1",
Type: v1beta1.ParamTypeString,
Default: &v1beta1.ParamValue{
Type: v1beta1.ParamTypeString,
StringVal: "v4",
},
Enum: []string{"v1", "v2"},
}},
configMap: map[string]string{
"enable-param-enum": "true",
},
expectedErr: fmt.Errorf("param default value v4 not in the enum list: params[param1]"),
}, {
name: "param enum with array type - failure",
params: []v1beta1.ParamSpec{{
Name: "param1",
Expand Down
8 changes: 8 additions & 0 deletions pkg/reconciler/taskrun/taskrun.go
Original file line number Diff line number Diff line change
Expand Up @@ -404,6 +404,14 @@ func (c *Reconciler) prepare(ctx context.Context, tr *v1.TaskRun) (*v1.TaskSpec,
return nil, nil, controller.NewPermanentError(err)
}

if config.FromContextOrDefaults(ctx).FeatureFlags.EnableParamEnum {
if err := ValidateEnumParam(ctx, tr.Spec.Params, rtr.TaskSpec.Params); err != nil {
logger.Errorf("TaskRun %q Param Enum validation failed: %v", tr.Name, err)
tr.Status.MarkResourceFailed(v1.TaskRunReasonInvalidParamValue, err)
return nil, nil, controller.NewPermanentError(err)
}
}

if err := resources.ValidateParamArrayIndex(rtr.TaskSpec, tr.Spec.Params); err != nil {
logger.Errorf("TaskRun %q Param references are invalid: %v", tr.Name, err)
tr.Status.MarkResourceFailed(podconvert.ReasonFailedValidation, err)
Expand Down
108 changes: 108 additions & 0 deletions pkg/reconciler/taskrun/taskrun_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,24 @@ var (
Steps: []v1.Step{simpleStep},
},
}
simpleTaskWithParamEnum = &v1.Task{
ObjectMeta: objectMeta("test-task-param-enum", "foo"),
TypeMeta: metav1.TypeMeta{
APIVersion: "tekton.dev/v1",
Kind: "Task",
},
Spec: v1.TaskSpec{
Params: []v1.ParamSpec{{
Name: "param1",
Enum: []string{"v1", "v2"},
}, {
Name: "param2",
Enum: []string{"v1", "v2"},
Default: &v1.ParamValue{Type: v1.ParamTypeString, StringVal: "v1"},
}},
Steps: []v1.Step{simpleStep},
},
}
resultsTask = &v1.Task{
ObjectMeta: objectMeta("test-results-task", "foo"),
Spec: v1.TaskSpec{
Expand Down Expand Up @@ -4999,6 +5017,96 @@ status:
}
}

func TestReconcile_TaskRunWithParam_Enum_valid(t *testing.T) {
taskRunWithParamValid := parse.MustParseV1TaskRun(t, `
metadata:
name: test-taskrun-with-param-enum-valid
namespace: foo
spec:
params:
- name: param1
value: v1
taskRef:
name: test-task-param-enum
`)

d := test.Data{
TaskRuns: []*v1.TaskRun{taskRunWithParamValid},
Tasks: []*v1.Task{simpleTaskWithParamEnum},
ConfigMaps: []*corev1.ConfigMap{{
ObjectMeta: metav1.ObjectMeta{Namespace: system.Namespace(), Name: config.GetFeatureFlagsConfigName()},
Data: map[string]string{
"enable-param-enum": "true",
},
}},
}

testAssets, cancel := getTaskRunController(t, d)
defer cancel()
createServiceAccount(t, testAssets, taskRunWithParamValid.Spec.ServiceAccountName, taskRunWithParamValid.Namespace)

// Reconcile the TaskRun
if err := testAssets.Controller.Reconciler.Reconcile(testAssets.Ctx, getRunName(taskRunWithParamValid)); err == nil {
t.Error("wanted a wrapped requeue error, but got nil.")
} else if ok, _ := controller.IsRequeueKey(err); !ok {
t.Errorf("expected no error. Got error %v", err)
}

tr, err := testAssets.Clients.Pipeline.TektonV1().TaskRuns(taskRunWithParamValid.Namespace).Get(testAssets.Ctx, taskRunWithParamValid.Name, metav1.GetOptions{})
if err != nil {
t.Fatalf("getting updated taskrun: %v", err)
}
condition := tr.Status.GetCondition(apis.ConditionSucceeded)
if condition.Type != apis.ConditionSucceeded || condition.Reason == string(corev1.ConditionFalse) {
t.Errorf("Expected TaskRun to succeed but it did not. Final conditions were:\n%#v", tr.Status.Conditions)
}
}

func TestReconcile_TaskRunWithParam_Enum_invalid(t *testing.T) {
taskRunWithParamInvalid := parse.MustParseV1TaskRun(t, `
metadata:
name: test-taskrun-with-param-enum-invalid
namespace: foo
spec:
params:
- name: param1
value: invalid
taskRef:
name: test-task-param-enum
`)

d := test.Data{
TaskRuns: []*v1.TaskRun{taskRunWithParamInvalid},
Tasks: []*v1.Task{simpleTaskWithParamEnum},
ConfigMaps: []*corev1.ConfigMap{{
ObjectMeta: metav1.ObjectMeta{Namespace: system.Namespace(), Name: config.GetFeatureFlagsConfigName()},
Data: map[string]string{
"enable-param-enum": "true",
},
}},
}

expectedErr := fmt.Errorf("1 error occurred:\n\t* param `param1` value: invalid is not in the enum list")
expectedFailureReason := "InvalidParamValue"
testAssets, cancel := getTaskRunController(t, d)
defer cancel()
createServiceAccount(t, testAssets, taskRunWithParamInvalid.Spec.ServiceAccountName, taskRunWithParamInvalid.Namespace)

// Reconcile the TaskRun
err := testAssets.Controller.Reconciler.Reconcile(testAssets.Ctx, getRunName(taskRunWithParamInvalid))
if d := cmp.Diff(expectedErr.Error(), strings.TrimSuffix(err.Error(), "\n\n")); d != "" {
t.Errorf("Expected: %v, but Got: %v", expectedErr, err)
}
tr, err := testAssets.Clients.Pipeline.TektonV1().TaskRuns(taskRunWithParamInvalid.Namespace).Get(testAssets.Ctx, taskRunWithParamInvalid.Name, metav1.GetOptions{})
if err != nil {
t.Fatalf("Error getting updated taskrun: %v", err)
}
condition := tr.Status.GetCondition(apis.ConditionSucceeded)
if condition.Type != apis.ConditionSucceeded || condition.Status != corev1.ConditionFalse || condition.Reason != expectedFailureReason {
t.Errorf("Expected TaskRun to fail with reason \"%s\" but it did not. Final conditions were:\n%#v", expectedFailureReason, tr.Status.Conditions)
}
}

func TestReconcile_validateTaskRunResults_valid(t *testing.T) {
taskRunResultsTypeMatched := parse.MustParseV1TaskRun(t, `
metadata:
Expand Down
30 changes: 30 additions & 0 deletions pkg/reconciler/taskrun/validate_taskrun.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
"github.com/tektoncd/pipeline/pkg/reconciler/taskrun/resources"

"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/utils/strings/slices"
)

// validateParams validates that all Pipeline Task, Matrix.Params and Matrix.Include parameters all have values, match the specified
Expand Down Expand Up @@ -174,6 +175,35 @@ func ValidateResolvedTask(ctx context.Context, params []v1.Param, matrix *v1.Mat
return nil
}

// ValidateEnumParam validates the param values are in the defined enum list in the corresponding paramSpecs if provided.
// A validation error is returned otherwise.
func ValidateEnumParam(ctx context.Context, params []v1.Param, paramSpecs v1.ParamSpecs) error {
paramSpecNameToEnum := map[string][]string{}
for _, ps := range paramSpecs {
if len(ps.Enum) == 0 {
continue
}
paramSpecNameToEnum[ps.Name] = ps.Enum
}

for _, p := range params {
// skip validation for and non-string typed and optional params (using default value)
// the default value of param is validated at validation webhook dryrun
if p.Value.Type != v1.ParamTypeString || p.Value.StringVal == "" {
continue
}
// skip validation for paramSpec without enum
if _, ok := paramSpecNameToEnum[p.Name]; !ok {
continue
}

if !slices.Contains(paramSpecNameToEnum[p.Name], p.Value.StringVal) {
return fmt.Errorf("param `%s` value: %s is not in the enum list", p.Name, p.Value.StringVal)
}
}
return nil
}

func validateTaskSpecRequestResources(taskSpec *v1.TaskSpec) error {
if taskSpec != nil {
for _, step := range taskSpec.Steps {
Expand Down