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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 4 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2b55a4a
Added a unit test for pod status.
chitrangpatel Apr 25, 2022
dba064d
Updated regex pattern to allow all three valid bracket notations.
chitrangpatel Apr 29, 2022
3bc5be8
Merge branch 'tektoncd:main' into main
chitrangpatel Apr 29, 2022
cb4f19b
merged origin main
chitrangpatel Apr 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 28 additions & 4 deletions pkg/substitution/substitution.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import (

const parameterSubstitution = `[_a-zA-Z][_a-zA-Z0-9.-]*(\[\*\])?`

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

// ValidateVariable makes sure all variables in the provided string are known
func ValidateVariable(name, value, prefix, locationName, path string, vars sets.String) *apis.FieldError {
Expand All @@ -41,6 +41,15 @@ func ValidateVariable(name, value, prefix, locationName, path string, vars sets.
}
}
}
} else {
// if expected variables but cannot be extracted from the string.
if len(vars) > 0 {
return &apis.FieldError{
Message: fmt.Sprintf("Expected to find variables but none found in %q", value),
Paths: []string{path + "." + name},
}

}
}
return nil
}
Expand All @@ -58,6 +67,15 @@ func ValidateVariableP(value, prefix string, vars sets.String) *apis.FieldError
}
}
}
} else {
// if expected variables but cannot be extracted from the string.
if len(vars) > 0 {
return &apis.FieldError{
Message: fmt.Sprintf("Expected to find variables but none found in %q", value),
// Empty path is required to make the `ViaField`, … work
Paths: []string{""},
}
}
Comment on lines +70 to +78
Copy link
Member

@chuangw6 chuangw6 Apr 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding this else will not work because different values can be passed to ValidateVariableP function other than just param names i.e. image name, step name.

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

Try this example, when you apply, you'll see

Error from server (BadRequest): error when creating "validation.yaml": admission webhook "validation.webhook.pipeline.tekton.dev" denied the request: validation failed: Expected to find variables but none found in "": spec.taskSpec.steps[0].workingDir
Expected to find variables but none found in "bash": spec.taskSpec.steps[0].image
Expected to find variables but none found in "echo-params": spec.taskSpec.steps[0].name
Expected to find variables but none found in "set -e\necho $(params.foo) | tee $(results.echo-output.path)\n": spec.taskSpec.steps[0].script

}
return nil
}
Expand Down Expand Up @@ -137,7 +155,7 @@ func ValidateVariableIsolatedP(value, prefix string, vars sets.String) *apis.Fie
// Extract a the first full string expressions found (e.g "$(input.params.foo)"). Return
// "" and false if nothing is found.
func extractExpressionFromString(s, prefix string) (string, bool) {
pattern := fmt.Sprintf(braceMatchingRegex, prefix, parameterSubstitution)
pattern := fmt.Sprintf(braceMatchingRegex, prefix, parameterSubstitution, parameterSubstitution, parameterSubstitution)
re := regexp.MustCompile(pattern)
match := re.FindStringSubmatch(s)
if match == nil {
Expand All @@ -147,7 +165,7 @@ func extractExpressionFromString(s, prefix string) (string, bool) {
}

func extractVariablesFromString(s, prefix string) ([]string, bool) {
pattern := fmt.Sprintf(braceMatchingRegex, prefix, parameterSubstitution)
pattern := fmt.Sprintf(braceMatchingRegex, prefix, parameterSubstitution, parameterSubstitution, parameterSubstitution)
re := regexp.MustCompile(pattern)
matches := re.FindAllStringSubmatch(s, -1)
if len(matches) == 0 {
Expand All @@ -159,7 +177,13 @@ func extractVariablesFromString(s, prefix string) ([]string, bool) {
// foo -> foo
// foo.bar -> foo
// foo.bar.baz -> foo
vars[i] = strings.SplitN(groups["var"], ".", 2)[0]
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
}
}
}
return vars, true
}
Expand Down
30 changes: 30 additions & 0 deletions pkg/substitution/substitution_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,26 @@ func TestValidateVariables(t *testing.T) {
vars: sets.NewString("baz"),
},
expectedError: nil,
}, {
name: "valid variable single quote bracket",
args: args{
input: "--flag=$(inputs.params['baz'])",
prefix: "inputs.params",
locationName: "step",
path: "taskspec.steps",
vars: sets.NewString("baz"),
},
expectedError: nil,
}, {
name: "valid variable single double bracket",
args: args{
input: "--flag=$(inputs.params[\"baz\"])",
prefix: "inputs.params",
locationName: "step",
path: "taskspec.steps",
vars: sets.NewString("baz"),
},
expectedError: nil,
}, {
name: "valid variable uid",
args: args{
Expand All @@ -69,6 +89,16 @@ func TestValidateVariables(t *testing.T) {
vars: sets.NewString("baz", "foo"),
},
expectedError: nil,
}, {
name: "multiple variables with brackets",
args: args{
input: "--flag=$(inputs.params[\"baz\"]) $(input.params['foo'])",
prefix: "inputs.params",
locationName: "step",
path: "taskspec.steps",
vars: sets.NewString("baz", "foo"),
},
expectedError: nil,
}, {
name: "different context and prefix",
args: args{
Expand Down