Skip to content

Commit

Permalink
Updated regex pattern to allow all three valid bracket notations.
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@chitrangpatel
chitrangpatel committed May 4, 2022
1 parent 5ee0b3e commit 65da44d
Show file tree
Hide file tree
Showing 2 changed files with 174 additions and 8 deletions.
67 changes: 62 additions & 5 deletions pkg/substitution/substitution.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,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 @@ -47,6 +46,33 @@ 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 := extractVariablesWithoutValidationFromString(value, prefix); present {
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{""},
}
}
}
}
if vs, present := extractVariablesFromString(value, prefix); present {
for _, v := range vs {
v = strings.TrimSuffix(v, "[*]")
Expand Down Expand Up @@ -137,7 +163,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 +173,32 @@ 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 {
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] != "" {
vars[i] = val
break
}
}
}
return vars, true
}

func extractVariablesWithoutValidationFromString(s, prefix string) ([]string, bool) {
parameterSub := `.*?(\[\*\])?`
pattern := fmt.Sprintf(braceMatchingRegex, prefix, parameterSub, parameterSub, parameterSub)
re := regexp.MustCompile(pattern)
matches := re.FindAllStringSubmatch(s, -1)
if len(matches) == 0 {
Expand All @@ -159,7 +210,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
115 changes: 112 additions & 3 deletions pkg/substitution/substitution_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ func TestValidateVariables(t *testing.T) {
}, {
name: "multiple variables",
args: args{
input: "--flag=$(inputs.params.baz) $(input.params.foo)",
input: "--flag=$(inputs.params.baz) $(inputs.params.foo)",
prefix: "inputs.params",
locationName: "step",
path: "taskspec.steps",
Expand Down Expand Up @@ -95,14 +95,14 @@ func TestValidateVariables(t *testing.T) {
}, {
name: "undefined variable and defined variable",
args: args{
input: "--flag=$(inputs.params.baz) $(input.params.foo)",
input: "--flag=$(inputs.params.baz) $(inputs.params.foo)",
prefix: "inputs.params",
locationName: "step",
path: "taskspec.steps",
vars: sets.NewString("foo"),
},
expectedError: &apis.FieldError{
Message: `non-existent variable in "--flag=$(inputs.params.baz) $(input.params.foo)" for step somefield`,
Message: `non-existent variable in "--flag=$(inputs.params.baz) $(inputs.params.foo)" for step somefield`,
Paths: []string{"taskspec.steps.somefield"},
},
}} {
Expand All @@ -116,6 +116,115 @@ func TestValidateVariables(t *testing.T) {
}
}

func TestValidateVariablePs(t *testing.T) {
type args struct {
input string
prefix string
vars sets.String
}
for _, tc := range []struct {
name string
args args
expectedError *apis.FieldError
}{{
name: "valid variable",
args: args{
input: "--flag=$(inputs.params.baz)",
prefix: "inputs.params",
vars: sets.NewString("baz"),
},
expectedError: nil,
}, {
name: "valid variable with double quote bracket",
args: args{
input: "--flag=$(inputs.params[\"baz\"])",
prefix: "inputs.params",
vars: sets.NewString("baz"),
},
expectedError: nil,
}, {
name: "valid variable with single quotebracket",
args: args{
input: "--flag=$(inputs.params['baz'])",
prefix: "inputs.params",
vars: sets.NewString("baz"),
},
expectedError: nil,
}, {
name: "Invalid variable wrong start char",
args: args{
input: "--flag=$(inputs.params['0baz'])",
prefix: "inputs.params",
vars: sets.NewString("baz"),
},
expectedError: &apis.FieldError{
Message: `Invalid variable name format in value: "--flag=$(inputs.params['0baz'])". Must not start with any character other than a-z, A-Z, 0-9, or _`,
Paths: []string{""},
},
}, {
name: "Invalid variable contains wrong char",
args: args{
input: "--flag=$(inputs.params['ba&z'])",
prefix: "inputs.params",
vars: sets.NewString("baz"),
},
expectedError: &apis.FieldError{
Message: `Invalid variable name format in value: "--flag=$(inputs.params['ba&z'])". Must not contain any character other than a-z, A-Z, 0-9, or . _ -`,
Paths: []string{""},
},
}, {
name: "valid variable contains diffetent chars",
args: args{
input: "--flag=$(inputs.params['ba-_9z'])",
prefix: "inputs.params",
vars: sets.NewString("ba-_9z"),
},
expectedError: nil,
}, {
name: "valid variable uid",
args: args{
input: "--flag=$(context.taskRun.uid)",
prefix: "context.taskRun",
vars: sets.NewString("uid"),
},
expectedError: nil,
}, {
name: "multiple variables",
args: args{
input: "--flag=$(inputs.params.baz) $(inputs.params.foo)",
prefix: "inputs.params",
vars: sets.NewString("baz", "foo"),
},
expectedError: nil,
}, {
name: "different context and prefix",
args: args{
input: "--flag=$(something.baz)",
prefix: "something",
vars: sets.NewString("baz"),
},
expectedError: nil,
}, {
name: "undefined variable",
args: args{
input: "--flag=$(inputs.params.baz)",
prefix: "inputs.params",
vars: sets.NewString("foo"),
},
expectedError: &apis.FieldError{
Message: `non-existent variable in "--flag=$(inputs.params.baz)"`,
Paths: []string{""},
},
}} {
t.Run(tc.name, func(t *testing.T) {
got := substitution.ValidateVariableP(tc.args.input, tc.args.prefix, tc.args.vars)

if d := cmp.Diff(got, tc.expectedError, cmp.AllowUnexported(apis.FieldError{})); d != "" {
t.Errorf("ValidateVariableP() error did not match expected error %s", diff.PrintWantGot(d))
}
})
}
}
func TestApplyReplacements(t *testing.T) {
type args struct {
input string
Expand Down

0 comments on commit 65da44d

Please sign in to comment.