Missing validation when referencing unknown params with bracket notation #4787

chuangw6 · 2022-04-22T20:40:27Z

Expected Behavior

There are three ways to reference a parameter in task steps.

# dot notation
$(params.<name>)
# or bracket notation (wrapping <name> with either single or double quotes):
$(params['<name>'])
$(params["<name>"])

If task steps use an unknown <name> in any 3 reference forms (means the parameter name is not declared in params), the webhook should validate against it and report an error like the following when applying a taskrun.

Error from server (BadRequest): error when creating "validation-test.yaml": admission webhook "validation.webhook.pipeline.tekton.dev" denied the request: validation failed: non-existent variable in "set -e\necho $(params.fooo) | tee $(results.echo-output.path)\n": spec.taskSpec.steps[0].script

Actual Behavior

Only validate against $(params.<name>) dot notation reference
Not validate against $(params['<name>']) $(params["<name>"]) bracket notation reference. If I use wrong parameter name, the taskrun still succeeds, but the unknown param reference has some bad behaviour i.e. when echo to result.

Steps to Reproduce the Problem

Apply the following taskrun in which the step echo-params references an unknown parameter name fooo using the bracket notation.

apiVersion: tekton.dev/v1beta1
kind: TaskRun
metadata:
  generateName: validation-
spec:
  taskSpec:
    params:
      - name: foo
        type: string
        default: "bar"
    results:
      - name: echo-output
        description: "see echo output"
    steps:
      - name: echo-params
        image: bash
        script: |
          set -e
          echo $(params["fooo"]) | tee $(results.echo-output.path)

Then, you can see nothing is complaint and taskrun is applied successfully. But the expectation is that the taskrun should not be run and an error should be reported.
Get the taskrun yaml
kubectl get tr <TASKRUN_NAME> -o yaml
You can see the taskResults echo-output has unexpected content because the parameter fooo doesn't exits. And this should not happen.

...
  taskResults:
  - name: echo-output
    value: |2+

...

Additional Info

Normal behaviour when using unknown name in dot notation `$(params.<name>)`.

Change the echo command in step1's yaml to echo $(params.fooo) | tee $(results.echo-output.path)
apply that yaml file
you can see the admission webhook will complaint, which is the expected behaviour when using unknown parameter names.

Error from server (BadRequest): error when creating "validation-test.yaml": admission webhook "validation.webhook.pipeline.tekton.dev" denied the request: validation failed: non-existent variable in "set -e\necho $(params.fooo) | tee $(results.echo-output.path)\n": spec.taskSpec.steps[0].script

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

The text was updated successfully, but these errors were encountered:

chuangw6 · 2022-04-22T20:52:54Z

Proposed Solution

Modify the extractVariablesFromString function to enable it to extract variable names from bracket notation reference strings. Currently, it can only extract variable names from dot notation reference strings.

Example:

current functionality

It can on extract foo from string $(params.foo) and extract foo[*] from string $(params.foo[*]), but it cannot extract foo from string $(params["foo"]) or $(params['foo'])

expect functionality

Also be able to extract foo from string $(params["foo"]) or $(params['foo'])

chuangw6 · 2022-04-26T06:29:04Z

Just found an issue that the validation against unknown params using the dot notion reference $(params.<name>) doesn't even work as expected in some cases. reported in #4792

chitrangpatel · 2022-04-29T14:43:18Z

To allow bracket notation to extract the variable properly, I think the regex pattern needs to be updated to

const braceMatchingRegex = "(\\$(\\(%s(\\.(?P<var1>%s)|\\[\"(?P<var2>%s)\"\\]|\\['(?P<var3>%s)'\\])\\)))"

And line needs to be replaced by something like:

		for _, v := range []string{"var1", "var2", "var3"} {
			val := strings.SplitN(groups[v], ".", 2)[0]
			if strings.SplitN(groups[v], ".", 2)[0] != "" {
				vars[i] = val
				break
			}

		}

I'm happy to take a stab at this and add some unit tests if you are not working on it @chuangw6.

chuangw6 · 2022-04-29T17:15:52Z

sgtm! Go for it! @chitrangpatel thanks for doing this.

chitrangpatel · 2022-04-29T18:28:04Z

/assign

chitrangpatel · 2022-04-29T19:28:41Z

After implementing the above fixes, I now get errors from the webhook if the names are inconsistent even with the bracket notation when I run this task:

apiVersion: tekton.dev/v1beta1
kind: TaskRun
metadata:
  generateName: validation-
spec:
  taskSpec:
    params:
      - name: foo
        type: string
        default: "bar"
    results:
      - name: echo-output
        description: "see echo output"
    steps:
      - name: echo-params
        image: bash
        script: |
          set -e
          echo $(params["fooo"]) | tee $(results.echo-output.path)

chitrang-macbookpro:pipeline chitrang$ ko create -f examples/v1beta1/chitrang/missing_brac.yaml
Error from server (BadRequest): error when creating "STDIN": admission webhook "validation.webhook.pipeline.tekton.dev" denied the request: validation failed: non-existent variable in "set -e\necho $(params.fooo) | tee $(results.echo-output.path)\n": spec.taskSpec.steps[0].script
Error: error executing 'kubectl create': exit status 1
2022/04/29 15:18:34 error during command execution:error executing 'kubectl create': exit status 1

chitrang-macbookpro:pipeline chitrang$ vim examples/v1beta1/chitrang/missing_brac.yaml
chitrang-macbookpro:pipeline chitrang$ ko create -f examples/v1beta1/chitrang/missing_brac.yaml
Error from server (BadRequest): error when creating "STDIN": admission webhook "validation.webhook.pipeline.tekton.dev" denied the request: validation failed: non-existent variable in "set -e\necho $(params['fooo']) | tee $(results.echo-output.path)\n": spec.taskSpec.steps[0].script
Error: error executing 'kubectl create': exit status 1
2022/04/29 15:19:21 error during command execution:error executing 'kubectl create': exit status 1

chitrang-macbookpro:pipeline chitrang$ vim examples/v1beta1/chitrang/missing_brac.yaml
chitrang-macbookpro:pipeline chitrang$ ko create -f examples/v1beta1/chitrang/missing_brac.yaml
Error from server (BadRequest): error when creating "STDIN": admission webhook "validation.webhook.pipeline.tekton.dev" denied the request: validation failed: non-existent variable in "set -e\necho $(params[\"fooo\"]) | tee $(results.echo-output.path)\n": spec.taskSpec.steps[0].script
Error: error executing 'kubectl create': exit status 1
2022/04/29 15:24:04 error during command execution:error executing 'kubectl create': exit status 1

chitrangpatel · 2022-04-29T20:10:27Z

There is a PR for this: #4811

imjasonh · 2022-05-14T14:38:32Z

The change in #4833 seems to have broken bracket notation with 's instead of "s, e.g., "$(params['org.blah.foo'])". Was this an intentional breakage? If not, can we get this working again with the new bracket notation validation logic? 🙏

chitrangpatel · 2022-05-14T14:59:45Z

@imjasonh, I don't think it was an intentional breakage. I think we only wanted to enable extraction of variables properly from the three different notations. The issue earlier was that it was not extracting any variable if we didn't use the dot notation.

My guess is that the extraction should return org.blah.foo but it's returning just org?
It should do the same with " as well, not just '. Let me know if the error is something else. I'll look into this ASAP.

imjasonh · 2022-05-14T19:17:59Z

@imjasonh, I don't think it was an intentional breakage. I think we only wanted to enable extraction of variables properly from the three different notations. The issue earlier was that it was not extracting any variable if we didn't use the dot notation.

My guess is that the extraction should return org.blah.foo but it's returning just org? It should do the same with " as well, not just '. Let me know if the error is something else. I'll look into this ASAP.

The error we get downstream is

Error from server (BadRequest): error when creating "blah/task.yaml": admission webhook "resource-validation.webhook.pipeline.tekton.dev" denied the request: validation failed: non-existent variable in "$(params['org.foo.blah'])": spec.steps[3].args[3]

But the param is defined:

apiVersion: tekton.dev/v1beta1
kind: Task
metadata:
  name: blah
spec:
  params:
    - name: org.foo.blah

This worked prior to picking up the change in #4833.

imjasonh · 2022-05-16T16:38:40Z

It seems that #4833 may have broken param names that include dots, using bracket notation with either "s or 's. I've added a simple test in main...imjasonh:dots which fails with the new test cases:

--- FAIL: TestValidateVariablePs (0.00s)
    --- FAIL: TestValidateVariablePs/valid_variable_with_double_quote_bracket_and_dots (0.00s)
        substitution_test.go:217: ValidateVariableP() error did not match expected error (-want, +got):   (*apis.FieldError)(
            - 	nil,
            + 	e`non-existent variable in "--flag=$(inputs.params[\"foo.bar.baz\"])": `,
              )
    --- FAIL: TestValidateVariablePs/valid_variable_with_single_quote_bracket_and_dots (0.00s)
        substitution_test.go:217: ValidateVariableP() error did not match expected error (-want, +got):   (*apis.FieldError)(
            - 	nil,
            + 	e`non-existent variable in "--flag=$(inputs.params['foo.bar.baz'])": `,
              )
FAIL
FAIL	github.com/tektoncd/pipeline/pkg/substitution	0.151s
FAIL

Should we revert that change to undo this regression?

chitrangpatel · 2022-05-16T18:03:22Z

Hi @imjasonh I dug into this and I think I found the issue. Let me summarize here:
Looking at the lines that were changed, The original braceMatchingRegex only matched variables that were written using the dot notation and could not extract anything from the bracket notation. The reason your test case passed earlier was a result of this. When there is no match, the function ValidateVariableP returned nil and so a validation error was not raised. Now, if you accidentally made a typo when referencing org.foo.blah as for eg. org.foo.blahh, this would not raise a validation error either (when it should have) and replace it with a dummy value or error out silently later. The bug fix was trying to address this issue.

However, if you see the code before specifically Line 162 was splitting the extracted variable on . so org.foo.blah would only return org. I'm not sure if this was always intended. (May be that's the correct way of extraction if the parameter was accessed via the . notation instead of brackets?). The logic was kept the same in the bug fix but that separation now raised this error that you are seeing. If I don't split it by a . and return the entire string then that bug is resolved.

There is clearly a bug that has surfaced since #4833 (but it always existed, just not noticed 🙂 ). I think we should revert this change and implement the fix that resolves the .s in the brackets.

chitrangpatel · 2022-05-17T02:29:27Z

There is a PR that addresses these bugs @imjasonh.
As an example, the following task run file will now work as expected.

apiVersion: tekton.dev/v1beta1 
kind: TaskRun                  
metadata:
  generateName: validation-
spec:
  taskSpec:                    
    params:                    
      - name: org.foo.double.blah     
        type: string           
        default: "double quotes"        
      - name: org.foo.single.blah     
        type: string           
        default: "single quotes"        
    steps:
      - name: echo-params      
        image: bash            
        script: |
          set -e
          echo $(params["org.foo.double.blah"])
      - name: echo-params2     
        image: bash            
        script: |              
          set -e               
          echo $(params['org.foo.single.blah'])

If you just use the dot notation with dots then it will throw an error. dots are now only allowed with the bracket notation.

Let me know if I missed anything.

chuangw6 · 2022-05-20T03:13:47Z

I think #4880 shouldn't regard anything like $(params.foo.bar) as invalid and return error message, because TEP75 proposes to add object type params that can have multiple keys. And the way to access individual key is like $(params.objectName.keyName) - a valid reference that can appear in a task step.

@chitrangpatel IIUC, by the following line, you mean when you see anything like $(params.foo.bar) or $(params['foo.bar']) or $(params["foo.bar"]), you extract out foo.bar in all three cases without throwing errors?
If so, this sounds like a solution.

If I don't split it by a . and return the entire string then that bug is resolved.

cc @imjasonh

jerop · 2022-05-26T19:55:41Z

fixed in #4833 and #4880 - thanks @chitrangpatel!

chuangw6 added the kind/bug Categorizes issue or PR as related to a bug. label Apr 22, 2022

chuangw6 changed the title ~~Validation when referencing prams with bracket notation~~ Validation when referencing params with bracket notation Apr 22, 2022

chuangw6 changed the title ~~Validation when referencing params with bracket notation~~ Validation when referencing unknown params with bracket notation Apr 26, 2022

chuangw6 changed the title ~~Validation when referencing unknown params with bracket notation~~ Missing validation when referencing unknown params with bracket notation Apr 26, 2022

tekton-robot assigned chitrangpatel Apr 29, 2022

chitrangpatel mentioned this issue Apr 29, 2022

Updated regex pattern to allow all three valid bracket notations. #4811

Closed

5 tasks

chitrangpatel mentioned this issue May 4, 2022

Added validation for parameters with bracket notation #4833

Merged

5 tasks

jerop closed this as completed May 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Missing validation when referencing unknown params with bracket notation #4787

Missing validation when referencing unknown params with bracket notation #4787

chuangw6 commented Apr 22, 2022 •

edited

Loading

chuangw6 commented Apr 22, 2022 •

edited

Loading

chuangw6 commented Apr 26, 2022 •

edited

Loading

chitrangpatel commented Apr 29, 2022 •

edited

Loading

chuangw6 commented Apr 29, 2022

chitrangpatel commented Apr 29, 2022

chitrangpatel commented Apr 29, 2022 •

edited

Loading

chitrangpatel commented Apr 29, 2022

imjasonh commented May 14, 2022

chitrangpatel commented May 14, 2022 •

edited

Loading

imjasonh commented May 14, 2022

imjasonh commented May 16, 2022

chitrangpatel commented May 16, 2022 •

edited

Loading

chitrangpatel commented May 17, 2022 •

edited

Loading

chuangw6 commented May 20, 2022 •

edited

Loading

jerop commented May 26, 2022

Missing validation when referencing unknown params with bracket notation #4787

Missing validation when referencing unknown params with bracket notation #4787

Comments

chuangw6 commented Apr 22, 2022 • edited Loading

Expected Behavior

Actual Behavior

Steps to Reproduce the Problem

Additional Info

Normal behaviour when using unknown name in dot notation $(params.<name>).

Kubernetes version:

Tekton Pipeline version:

chuangw6 commented Apr 22, 2022 • edited Loading

Proposed Solution

Example:

current functionality

expect functionality

chuangw6 commented Apr 26, 2022 • edited Loading

chitrangpatel commented Apr 29, 2022 • edited Loading

chuangw6 commented Apr 29, 2022

chitrangpatel commented Apr 29, 2022

chitrangpatel commented Apr 29, 2022 • edited Loading

chitrangpatel commented Apr 29, 2022

imjasonh commented May 14, 2022

chitrangpatel commented May 14, 2022 • edited Loading

imjasonh commented May 14, 2022

imjasonh commented May 16, 2022

chitrangpatel commented May 16, 2022 • edited Loading

chitrangpatel commented May 17, 2022 • edited Loading

chuangw6 commented May 20, 2022 • edited Loading

jerop commented May 26, 2022

chuangw6 commented Apr 22, 2022 •

edited

Loading

Normal behaviour when using unknown name in dot notation `$(params.<name>)`.

chuangw6 commented Apr 22, 2022 •

edited

Loading

chuangw6 commented Apr 26, 2022 •

edited

Loading

chitrangpatel commented Apr 29, 2022 •

edited

Loading

chitrangpatel commented Apr 29, 2022 •

edited

Loading

chitrangpatel commented May 14, 2022 •

edited

Loading

chitrangpatel commented May 16, 2022 •

edited

Loading

chitrangpatel commented May 17, 2022 •

edited

Loading

chuangw6 commented May 20, 2022 •

edited

Loading