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
Added validation for parameters with bracket notation #4833
Conversation
Hi @chitrangpatel. Thanks for your PR. I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@chitrangpatel: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
pkg/substitution/substitution.go
Outdated
for _, v := range vs { | ||
v = strings.TrimSuffix(v, "[*]") | ||
|
||
// contains any character except "_a-zA-Z0-9.-"? | ||
re := regexp.MustCompile(`[^_a-zA-Z0-9.-]`) | ||
matches := re.FindAllStringSubmatch(v, -1) | ||
if len(matches) > 0 { | ||
return &apis.FieldError{ | ||
Message: fmt.Sprintf("Invalid variable name format in value: %q. Must not contain any character other than a-z, A-Z, 0-9, or . _ -", value), | ||
// Empty path is required to make the `ViaField`, … work | ||
Paths: []string{""}, | ||
} | ||
} | ||
|
||
// starts with anything except _, a-z, A-Z? | ||
re = regexp.MustCompile(`^[^_a-zA-Z].*`) | ||
matches = re.FindAllStringSubmatch(v, -1) | ||
if len(matches) > 0 { | ||
return &apis.FieldError{ | ||
Message: fmt.Sprintf("Invalid variable name format in value: %q. Must not start with any character other than a-z, A-Z, 0-9, or _", value), | ||
// Empty path is required to make the `ViaField`, … work | ||
Paths: []string{""}, | ||
} | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the three steps we discussed (pasted below), in step2, I think we don't need to do any validation here inside ValidateVariableP
. Because validation against the param names specified in ParamSpec
should happen inside task_validation.go
's validateVariables
function, which is step 1.
Step1: validate the format of param names at the beginning of validateVariables in
pkg/apis/pipeline/v1beta1/task_validation.go
(Implemented here #4799)
Step2: change regex to extract
NAME
for$(params.<NAME>)
andNAME
for$(params["<NAME>"])
without validating anything
Step3: Check if the extracted string is one of the param names declared in param spec. (exists already)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be ok now @chuangw6 . Let me know otherwise.
/ok-to-test |
Perhaps its worth adding a note in the release notes? |
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
@afrittoli Since there were no user facing changes, I skipped it as suggested by the |
/retest |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vdemeester The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@chitrangpatel would you mind changing the PR title to something more descriptive? |
Done! |
@chitrangpatel would you mind also changing the commit message title and PR title to be "Add validation for parameters with bracket notation"? other than that looks good :) |
Done! |
don't forget the commit message title! |
Prior to this, when supplied parameter name was not the same as that called in the step, webhook raised an error when using the dot notation as expected. However, it did not when using the bracket notation. Updated the unit tests to also check for valid bracket notations.
Done now. (I accidentally changed the PR's first line thinking it was the title of the commit message). |
The following is the coverage report on the affected files.
|
/retest |
/lgtm |
Added validation for parameters with bracket notation.
Prior to this, when supplied parameter name was not the same as that
called in the step, webhook raised an error when using the dot notation
as expected. However, it did not when using the bracket notation.
Changes
Updated the unit tests to also check for valid bracket notations.
This PR addresses issue #4787
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
functionality, content, code)
(if there are no user facing changes, use release note "NONE")
Release Notes