Skip to content

Commit

Permalink
Allow proper extraction when using dots in dot notations.
Browse files Browse the repository at this point in the history 
Prior to this, when using `$params["<NAME>"]` or `$params['<NAME>']`,
if `NAME` contained `dots(.)` (eg. `org.foo.blah`), only `org` would be
extracted, instead of `org.foo.blah`. This PR now properly fetches the
entire string and throws an error if using `dots (.)` without `single`
or `double` quotes.

The following references show whats valid and whats not:

`$params.org.foo.blah` : Not Valid: the webhook will throw a validation error.
`$params["org.foo.blah"]`: Valid
`$params['org.foo.blah']`: Valid
  • Loading branch information
@chitrangpatel
chitrangpatel committed May 17, 2022
1 parent 155179b commit 01290d8
Show file tree
Hide file tree
Showing 3 changed files with 107 additions and 16 deletions.
4 changes: 3 additions & 1 deletion docs/tasks.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -833,8 +833,10 @@ variable values as follows:
- To reference a parameter in a `Task`, use the following syntax, where `<name>` is the name of the parameter:
```shell
# dot notation
$(params.<name>)
# Here, the name cannot contain dots (eg. foo.bar is not allowed). If the name contains `dots`, it can only be accessed via the bracket notation.
$(params.<name> )
# or bracket notation (wrapping <name> with either single or double quotes):
# Here, the name can contain dots (eg. foo.bar is allowed).
$(params['<name>'])
$(params["<name>"])
```
Expand Down
73 changes: 58 additions & 15 deletions pkg/substitution/substitution.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,12 @@ import (
)

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

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 {
if vs, present := extractVariablesFromString(value, prefix); present {
if vs, present, _ := extractVariablesFromString(value, prefix); present {
for _, v := range vs {
v = strings.TrimSuffix(v, "[*]")
if !vars.Has(v) {
Expand All @@ -46,7 +47,14 @@ func ValidateVariable(name, value, prefix, locationName, path string, vars sets.

// ValidateVariableP makes sure all variables for a parameter in the provided string are known
func ValidateVariableP(value, prefix string, vars sets.String) *apis.FieldError {
if vs, present := extractVariablesFromString(value, prefix); present {
if vs, present, errString := extractVariablesFromString(value, prefix); present {
if errString != "" {
return &apis.FieldError{
Message: errString,
Paths: []string{""},
}

}
for _, v := range vs {
v = strings.TrimSuffix(v, "[*]")
if !vars.Has(v) {
Expand All @@ -63,7 +71,7 @@ func ValidateVariableP(value, prefix string, vars sets.String) *apis.FieldError

// ValidateVariableProhibited verifies that variables matching the relevant string expressions do not reference any of the names present in vars.
func ValidateVariableProhibited(name, value, prefix, locationName, path string, vars sets.String) *apis.FieldError {
if vs, present := extractVariablesFromString(value, prefix); present {
if vs, present, _ := extractVariablesFromString(value, prefix); present {
for _, v := range vs {
v = strings.TrimSuffix(v, "[*]")
if vars.Has(v) {
Expand All @@ -79,7 +87,14 @@ func ValidateVariableProhibited(name, value, prefix, locationName, path string,

// ValidateVariableProhibitedP verifies that variables for a parameter matching the relevant string expressions do not reference any of the names present in vars.
func ValidateVariableProhibitedP(value, prefix string, vars sets.String) *apis.FieldError {
if vs, present := extractVariablesFromString(value, prefix); present {
if vs, present, errString := extractVariablesFromString(value, prefix); present {
if errString != "" {
return &apis.FieldError{
Message: errString,
Paths: []string{""},
}

}
for _, v := range vs {
v = strings.TrimSuffix(v, "[*]")
if vars.Has(v) {
Expand All @@ -96,7 +111,7 @@ func ValidateVariableProhibitedP(value, prefix string, vars sets.String) *apis.F

// ValidateVariableIsolated verifies that variables matching the relevant string expressions are completely isolated if present.
func ValidateVariableIsolated(name, value, prefix, locationName, path string, vars sets.String) *apis.FieldError {
if vs, present := extractVariablesFromString(value, prefix); present {
if vs, present, _ := extractVariablesFromString(value, prefix); present {
firstMatch, _ := extractExpressionFromString(value, prefix)
for _, v := range vs {
v = strings.TrimSuffix(v, "[*]")
Expand All @@ -115,7 +130,14 @@ func ValidateVariableIsolated(name, value, prefix, locationName, path string, va

// ValidateVariableIsolatedP verifies that variables matching the relevant string expressions are completely isolated if present.
func ValidateVariableIsolatedP(value, prefix string, vars sets.String) *apis.FieldError {
if vs, present := extractVariablesFromString(value, prefix); present {
if vs, present, errString := extractVariablesFromString(value, prefix); present {
if errString != "" {
return &apis.FieldError{
Message: errString,
Paths: []string{""},
}

}
firstMatch, _ := extractExpressionFromString(value, prefix)
for _, v := range vs {
v = strings.TrimSuffix(v, "[*]")
Expand Down Expand Up @@ -145,28 +167,49 @@ func extractExpressionFromString(s, prefix string) (string, bool) {
return match[0], true
}

func extractVariablesFromString(s, prefix string) ([]string, bool) {
func extractVariablesFromString(s, prefix string) ([]string, bool, string) {
pattern := fmt.Sprintf(braceMatchingRegex, prefix, parameterSubstitution, parameterSubstitution, parameterSubstitution)
re := regexp.MustCompile(pattern)
matches := re.FindAllStringSubmatch(s, -1)
errString := ""
if len(matches) == 0 {
return []string{}, false
return []string{}, false, ""
}
vars := make([]string, len(matches))
for i, match := range matches {
groups := matchGroups(match, re)
// foo -> foo
// foo.bar -> foo
// foo.bar.baz -> foo
for _, v := range []string{"var1", "var2", "var3"} {
val := strings.SplitN(groups[v], ".", 2)[0]
if strings.SplitN(groups[v], ".", 2)[0] != "" {
for j, v := range []string{"var1", "var2", "var3"} {
val := groups[v]
// if using the dot notation
if j == 0 && strings.Contains(val, ".") {
if prefix == "params" {
// params can only have a maximum of two components in the dot notation otherwise it needs to use the bracket notation.
if len(strings.Split(val, ".")) > 0 {
errString = fmt.Sprintf(`Invalid referencing of parameters in %s !!! You can only use the dots inside single or double quotes. eg. $(params["org.foo.blah"]) or $(params['org.foo.blah']) are valid references but NOT $params.org.foo.blah.`, s)
return vars, true, errString
}
} else if prefix == "resources.(?:inputs|outputs)" {
// resources can only have a maximum of 4 components.
if len(strings.Split(val, ".")) > 2 {
errString = fmt.Sprintf(`Invalid referencing of parameters in %s !!! resources.* can only have 4 components (eg. resources.inputs.foo.bar). Found more than 4 components.`, s)
} else {
vars[i] = strings.SplitN(val, ".", 2)[0]
}
return vars, true, errString
} else {
// for backwards compatibality
vars[i] = strings.SplitN(val, ".", 2)[0]
return vars, true, errString
}
}
if groups[v] != "" {
vars[i] = val
break
}

}
}
return vars, true
return vars, true, errString
}

func matchGroups(matches []string, pattern *regexp.Regexp) map[string]string {
Expand Down
46 changes: 46 additions & 0 deletions pkg/substitution/substitution_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,52 @@ func TestValidateVariablePs(t *testing.T) {
vars: sets.NewString("baz"),
},
expectedError: nil,
}, {
name: "valid variable with double quote bracket and dots",
args: args{
input: "--flag=$(params[\"foo.bar.baz\"])",
prefix: "params",
vars: sets.NewString("foo.bar.baz"),
},
expectedError: nil,
}, {
name: "valid variable with single quote bracket and dots",
args: args{
input: "--flag=$(params['foo.bar.baz'])",
prefix: "params",
vars: sets.NewString("foo.bar.baz"),
},
expectedError: nil,
}, {
name: "invalid variable with only dots referencing parameters",
args: args{
input: "--flag=$(params.foo.bar.baz)",
prefix: "params",
vars: sets.NewString("foo.bar.baz"),
},
expectedError: &apis.FieldError{
Message: `Invalid referencing of parameters in --flag=$(params.foo.bar.baz) !!! You can only use the dots inside single or double quotes. eg. $(params["org.foo.blah"]) or $(params['org.foo.blah']) are valid references but NOT $params.org.foo.blah.`,
Paths: []string{""},
},
}, {
name: "valid variable with dots referencing resources",
args: args{
input: "--flag=$(resources.inputs.foo.bar)",
prefix: "resources.(?:inputs|outputs)",
vars: sets.NewString("foo"),
},
expectedError: nil,
}, {
name: "invalid variable with dots referencing resources",
args: args{
input: "--flag=$(resources.inputs.foo.bar.baz)",
prefix: "resources.(?:inputs|outputs)",
vars: sets.NewString("foo.bar"),
},
expectedError: &apis.FieldError{
Message: `Invalid referencing of parameters in --flag=$(resources.inputs.foo.bar.baz) !!! resources.* can only have 4 components (eg. resources.inputs.foo.bar). Found more than 4 components.`,
Paths: []string{""},
},
}, {
name: "valid variable contains diffetent chars",
args: args{
Expand Down

0 comments on commit 01290d8

Please sign in to comment.