Missing validation against parameter name format

[chuangw6]

Expected Behavior

This doc mentioned that the parameter names should follow the following rules:

• Must only contain alphanumeric characters, hyphens (-), underscores (_), and dots (.).
• Must begin with a letter or an underscore (_).

For example, foo.Is-Bar_ is a valid parameter name, but barIsBa$ or 0banana are not.

Therefore, there should be existing validation against parameter names that users specified. For example, the following parameter names are invalid, which should be caught and reported by the admission webhook when the taskrun yaml is applied.

apiVersion: tekton.dev/v1beta1
kind: TaskRun
metadata:
  generateName: validation-
spec:
  taskSpec:
    params:
      - name: 0foo
        default: "bar1"
      - name: a b
        default: "bar2"
      - name: $x
        default: "bar3"
    results:
      - name: echo-output
        description: "successful echo"
      - name: bad-echo
        description: "bad echo behaviour"
    steps:
      - name: echo-params
        image: bash
        script: |
          set -e
          echo $(params.0foo) $(params.a b) $(params.$x) | tee $(results.echo-output.path)
          echo $(params.0fo) | tee $(results.bad-echo.path)

Actual Behavior

taskResults output

  taskResults:
  - name: bad-echo
    value: |2+

  - name: echo-output
    value: |
      bar1 bar2 bar3

1. When you apply the above taskrun yaml, it can be successfully applied, which should've been reported because of invalid param name format instead.
2. From taskrun result, echo-output is populated with values bar1 bar2 bar3, which should not happen because the parameter names are all invalid i.e. 0foo starts with 0, a b has a space in the middle, $x starts with $
3. In addition, the taskrun result bad-echo is shown in the taskResults list under status, but has some confusing value +2. (See the reason why this happens here)

Steps to Reproduce the Problem

1. Save above yaml code into a file, and apply it
2. see the taskrun
   kubectl get tr <TASKRUN_NAME> -o yaml
3. find status -> taskResults

Additional Info

Kubernetes version:

Output of kubectl version:

Client Version: version.Info{Major:"1", Minor:"23", GitVersion:"v1.23.1", GitCommit:"86ec240af8cbd1b60bcc4c03c20da9b98005b92e", GitTreeState:"clean", BuildDate:"2021-12-16T11:41:01Z", GoVersion:"go1.17.5", Compiler:"gc", Platform:"linux/amd64"}
Server Version: version.Info{Major:"1", Minor:"21", GitVersion:"v1.21.6-gke.1503", GitCommit:"2c7bbda09a9b7ca78db230e099cf90fe901d3df8", GitTreeState:"clean", BuildDate:"2022-02-18T03:17:45Z", GoVersion:"go1.16.9b7", Compiler:"gc", Platform:"linux/amd64"}
WARNING: version difference between client (1.23) and server (1.21) exceeds the supported minor version skew of +/-1

Tekton Pipeline version:

Output of tkn version or kubectl get pods -n tekton-pipelines -l app=tekton-pipelines-controller -o=jsonpath='{.items[0].metadata.labels.version}'

Client version: 0.21.0
Pipeline version: devel

Also found that a param without a name also works, which is weird

apiVersion: tekton.dev/v1beta1
kind: TaskRun
metadata:
  generateName: validation-
spec:
  taskSpec:
    params:
      - name:
        default: "bar1"
    results:
      - name: echo-output
        description: "successful echo"
    steps:
      - name: echo-params
        image: bash
        script: |
          set -e
          echo $(params.) | tee $(results.echo-output.path)

[chuangw6]

Thoughts

• The purpose of the function ValidateVariableP called in task_validation.go is to use extractVariablesFromString to parse the name part from the string $(params.<NAME>) and check if the name is one of param names in vars - a set of all param names in task spec.
• extractVariablesFromString seems to extract param names that follow the two rules mentioned at the beginning of the issue using the pattern it composed at the function's beginning.
• The bug is that when the param name is invalid format, extractVariablesFromString will return empty, and ValidateVariableP will return nil, which means it cannot catch the invalid param name. When the validation is done without any error, the taskrun is actually executed and when 0fo is actually referenced in echo $(params.0fo) | tee $(results.bad-echo.path), it's found that oh it doesn't exist in task's params list and the following unexpected behaviour appears.

    taskResults:
    - name: bad-echo
      value: |2+
  
  

[afrittoli]

@chuangw6 thank you for this report, it sounds like validation there can use some improvement and unit tests :)
Do you plan to work on a fix for this or shall we mark it as help wanted?

[chuangw6]

Hi @afrittoli ,
Yeah, I am happy to look into this.
Just wonder why two rules for param name format are in place. Are we restricting this in order to avoid something or is it just a convention for variable names? Can you help me understand this?
Thank you!

[chuangw6]

Hi @afrittoli @dibyom ,
I just realized that we might need to disallow the parameters whose name contains dot(.) because of reasons mentioned in #4797 .

Do you have any thoughts on this? Fee free to follow up here or in #4797.
Thanks!
Chuang

[chuangw6]

/assign

[chuangw6]

Result name and Param name

The interpretation of this regex seems that result name format shares same rules with param name except that a rule (not mentioned for param names) which is result name should only end with alphanumeric chars and not end with -, _ or ..

We should make sure result name and param name follow same rules and enforce that in param names as well (currently, the enforcement for param name format is missing).