Skip to content

Commit

Permalink
[IMPROVED] Subject transform validation and error reporting (#4202)
Browse files Browse the repository at this point in the history 
 - [X] Tests added
- [X] Branch rebased on top of current main (`git pull --rebase origin
main`)
- [X] Changes squashed to a single commit (described
[here](http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html))
 - [ ] Build is green in Travis CI
- [X] You have certified that the contribution is your original work and
that you license the work to the project under the [Apache 2
license](https://github.com/nats-io/nats-server/blob/main/LICENSE)

### Changes proposed in this pull request:
1: Improves error reporting for weighted mappings:
As it was, any error in a weighted mapping would return a very
unhelpfull error message.

e.g. `nats-server: mappingtest.cfg:38:39: interface conversion:
interface {} is []interface {}, not string`

This was because the line `err := &configErr{tk, fmt.Sprintf("Error
adding mapping for %q to %q : %v", subj, v.(string), err)}` would panic
on the `v.(string)` since in weighted mapping that interface{} is
actually a map[string]interface{} (since there's can be more than one
mapping in weighted mappings).

Now returns the actual error:

e.g. `nats-server: mappingtest.cfg:40:3: Error adding mapping for "bla"
: invalid mapping destination: wildcard index out of range in
{{wildcard(1)}}`

2: improves subject transform checking and catches if the destination is
using a mapping function and there are no partial wildcards in the
source.
  • Loading branch information
@derekcollison
derekcollison committed Jun 1, 2023
2 parents 30fa427 + 0bd3fa4 commit c3234dc
Show file tree
Hide file tree
Showing 4 changed files with 47 additions and 28 deletions.
20 changes: 10 additions & 10 deletions server/errors.go
View file
Expand Up @@ -208,20 +208,20 @@ var (
// ErrUnknownMappingDestinationFunction is returned when a subject mapping destination contains an unknown mustache-escaped mapping function.
ErrUnknownMappingDestinationFunction = fmt.Errorf("%w: unknown function", ErrInvalidMappingDestination)

// ErrorMappingDestinationFunctionWildcardIndexOutOfRange is returned when the mapping destination function is passed an out of range wildcard index value for one of it's arguments
ErrorMappingDestinationFunctionWildcardIndexOutOfRange = fmt.Errorf("%w: wildcard index out of range", ErrInvalidMappingDestination)
// ErrMappingDestinationIndexOutOfRange is returned when the mapping destination function is passed an out of range wildcard index value for one of it's arguments
ErrMappingDestinationIndexOutOfRange = fmt.Errorf("%w: wildcard index out of range", ErrInvalidMappingDestination)

// ErrorMappingDestinationFunctionNotEnoughArguments is returned when the mapping destination function is not passed enough arguments
ErrorMappingDestinationFunctionNotEnoughArguments = fmt.Errorf("%w: not enough arguments passed to the function", ErrInvalidMappingDestination)
// ErrMappingDestinationNotEnoughArgs is returned when the mapping destination function is not passed enough arguments
ErrMappingDestinationNotEnoughArgs = fmt.Errorf("%w: not enough arguments passed to the function", ErrInvalidMappingDestination)

// ErrorMappingDestinationFunctionInvalidArgument is returned when the mapping destination function is passed and invalid argument
ErrorMappingDestinationFunctionInvalidArgument = fmt.Errorf("%w: function argument is invalid or in the wrong format", ErrInvalidMappingDestination)
// ErrMappingDestinationInvalidArg is returned when the mapping destination function is passed and invalid argument
ErrMappingDestinationInvalidArg = fmt.Errorf("%w: function argument is invalid or in the wrong format", ErrInvalidMappingDestination)

// ErrorMappingDestinationFunctionTooManyArguments is returned when the mapping destination function is passed too many arguments
ErrorMappingDestinationFunctionTooManyArguments = fmt.Errorf("%w: too many arguments passed to the function", ErrInvalidMappingDestination)
// ErrMappingDestinationTooManyArgs is returned when the mapping destination function is passed too many arguments
ErrMappingDestinationTooManyArgs = fmt.Errorf("%w: too many arguments passed to the function", ErrInvalidMappingDestination)

// ErrorMappingDestinationFunctionNotSupportedForImport is returned when you try to use a mapping function other than wildcard in a transform that needs to be reversible (i.e. an import)
ErrorMappingDestinationFunctionNotSupportedForImport = fmt.Errorf("%w: the only mapping function allowed for import transforms is {{Wildcard()}}", ErrInvalidMappingDestination)
// ErrMappingDestinationNotSupportedForImport is returned when you try to use a mapping function other than wildcard in a transform that needs to be reversible (i.e. an import)
ErrMappingDestinationNotSupportedForImport = fmt.Errorf("%w: the only mapping function allowed for import transforms is {{Wildcard()}}", ErrInvalidMappingDestination)
)

// mappingDestinationErr is a type of subject mapping destination error
Expand Down
4 changes: 2 additions & 2 deletions server/opts.go
View file
Expand Up @@ -2690,7 +2690,7 @@ func parseAccountMappings(v interface{}, acc *Account, errors *[]error, warnings

// Now add them in..
if err := acc.AddWeightedMappings(subj, mappings...); err != nil {
err := &configErr{tk, fmt.Sprintf("Error adding mapping for %q to %q : %v", subj, v.(string), err)}
err := &configErr{tk, fmt.Sprintf("Error adding mapping for %q : %v", subj, err)}
*errors = append(*errors, err)
continue
}
Expand All @@ -2702,7 +2702,7 @@ func parseAccountMappings(v interface{}, acc *Account, errors *[]error, warnings
}
// Now add it in..
if err := acc.AddWeightedMappings(subj, mdest); err != nil {
err := &configErr{tk, fmt.Sprintf("Error adding mapping for %q to %q : %v", subj, v.(string), err)}
err := &configErr{tk, fmt.Sprintf("Error adding mapping for %q : %v", subj, err)}
*errors = append(*errors, err)
continue
}
Expand Down
50 changes: 34 additions & 16 deletions server/subject_transform.go
View file
Expand Up @@ -110,7 +110,13 @@ func NewSubjectTransformWithStrict(src, dest string, strict bool) (*subjectTrans

if strict {
if tranformType != NoTransform && tranformType != Wildcard {
return nil, &mappingDestinationErr{token, ErrorMappingDestinationFunctionNotSupportedForImport}
return nil, &mappingDestinationErr{token, ErrMappingDestinationNotSupportedForImport}
}
}

if npwcs == 0 {
if tranformType != NoTransform {
return nil, &mappingDestinationErr{token, ErrMappingDestinationIndexOutOfRange}
}
}

Expand All @@ -125,7 +131,7 @@ func NewSubjectTransformWithStrict(src, dest string, strict bool) (*subjectTrans
var stis []int
for _, wildcardIndex := range transformArgWildcardIndexes {
if wildcardIndex > npwcs {
return nil, &mappingDestinationErr{fmt.Sprintf("%s: [%d]", token, wildcardIndex), ErrorMappingDestinationFunctionWildcardIndexOutOfRange}
return nil, &mappingDestinationErr{fmt.Sprintf("%s: [%d]", token, wildcardIndex), ErrMappingDestinationIndexOutOfRange}
}
stis = append(stis, sti[wildcardIndex])
}
Expand All @@ -140,6 +146,18 @@ func NewSubjectTransformWithStrict(src, dest string, strict bool) (*subjectTrans
// not all wildcards are being used in the destination
return nil, &mappingDestinationErr{dest, ErrMappingDestinationNotUsingAllWildcards}
}
} else {
// no wildcards used in the source: check that no transform functions are used in the destination
for _, token := range dtokens {
tranformType, _, _, _, err := indexPlaceHolders(token)
if err != nil {
return nil, err
}

if tranformType != NoTransform {
return nil, &mappingDestinationErr{token, ErrMappingDestinationIndexOutOfRange}
}
}
}

return &subjectTransform{
Expand Down Expand Up @@ -173,18 +191,18 @@ func getMappingFunctionArgs(functionRegEx *regexp.Regexp, token string) []string
// Helper for mapping functions that take a wildcard index and an integer as arguments
func transformIndexIntArgsHelper(token string, args []string, transformType int16) (int16, []int, int32, string, error) {
if len(args) < 2 {
return BadTransform, []int{}, -1, _EMPTY_, &mappingDestinationErr{token, ErrorMappingDestinationFunctionNotEnoughArguments}
return BadTransform, []int{}, -1, _EMPTY_, &mappingDestinationErr{token, ErrMappingDestinationNotEnoughArgs}
}
if len(args) > 2 {
return BadTransform, []int{}, -1, _EMPTY_, &mappingDestinationErr{token, ErrorMappingDestinationFunctionTooManyArguments}
return BadTransform, []int{}, -1, _EMPTY_, &mappingDestinationErr{token, ErrMappingDestinationTooManyArgs}
}
i, err := strconv.Atoi(strings.Trim(args[0], " "))
if err != nil {
return BadTransform, []int{}, -1, _EMPTY_, &mappingDestinationErr{token, ErrorMappingDestinationFunctionInvalidArgument}
return BadTransform, []int{}, -1, _EMPTY_, &mappingDestinationErr{token, ErrMappingDestinationInvalidArg}
}
mappingFunctionIntArg, err := strconv.Atoi(strings.Trim(args[1], " "))
if err != nil {
return BadTransform, []int{}, -1, _EMPTY_, &mappingDestinationErr{token, ErrorMappingDestinationFunctionInvalidArgument}
return BadTransform, []int{}, -1, _EMPTY_, &mappingDestinationErr{token, ErrMappingDestinationInvalidArg}
}

return transformType, []int{i}, int32(mappingFunctionIntArg), _EMPTY_, nil
Expand Down Expand Up @@ -212,36 +230,36 @@ func indexPlaceHolders(token string) (int16, []int, int32, string, error) {
args := getMappingFunctionArgs(wildcardMappingFunctionRegEx, token)
if args != nil {
if len(args) == 1 && args[0] == _EMPTY_ {
return BadTransform, []int{}, -1, _EMPTY_, &mappingDestinationErr{token, ErrorMappingDestinationFunctionNotEnoughArguments}
return BadTransform, []int{}, -1, _EMPTY_, &mappingDestinationErr{token, ErrMappingDestinationNotEnoughArgs}
}
if len(args) == 1 {
tokenIndex, err := strconv.Atoi(strings.Trim(args[0], " "))
if err != nil {
return BadTransform, []int{}, -1, _EMPTY_, &mappingDestinationErr{token, ErrorMappingDestinationFunctionInvalidArgument}
return BadTransform, []int{}, -1, _EMPTY_, &mappingDestinationErr{token, ErrMappingDestinationInvalidArg}
}
return Wildcard, []int{tokenIndex}, -1, _EMPTY_, nil
} else {
return BadTransform, []int{}, -1, _EMPTY_, &mappingDestinationErr{token, ErrorMappingDestinationFunctionTooManyArguments}
return BadTransform, []int{}, -1, _EMPTY_, &mappingDestinationErr{token, ErrMappingDestinationTooManyArgs}
}
}

// partition(number of partitions, token1, token2, ...)
args = getMappingFunctionArgs(partitionMappingFunctionRegEx, token)
if args != nil {
if len(args) < 2 {
return BadTransform, []int{}, -1, _EMPTY_, &mappingDestinationErr{token, ErrorMappingDestinationFunctionNotEnoughArguments}
return BadTransform, []int{}, -1, _EMPTY_, &mappingDestinationErr{token, ErrMappingDestinationNotEnoughArgs}
}
if len(args) >= 2 {
mappingFunctionIntArg, err := strconv.Atoi(strings.Trim(args[0], " "))
if err != nil {
return BadTransform, []int{}, -1, _EMPTY_, &mappingDestinationErr{token, ErrorMappingDestinationFunctionInvalidArgument}
return BadTransform, []int{}, -1, _EMPTY_, &mappingDestinationErr{token, ErrMappingDestinationInvalidArg}
}
var numPositions = len(args[1:])
tokenIndexes := make([]int, numPositions)
for ti, t := range args[1:] {
i, err := strconv.Atoi(strings.Trim(t, " "))
if err != nil {
return BadTransform, []int{}, -1, _EMPTY_, &mappingDestinationErr{token, ErrorMappingDestinationFunctionInvalidArgument}
return BadTransform, []int{}, -1, _EMPTY_, &mappingDestinationErr{token, ErrMappingDestinationInvalidArg}
}
tokenIndexes[ti] = i
}
Expand Down Expand Up @@ -278,17 +296,17 @@ func indexPlaceHolders(token string) (int16, []int, int32, string, error) {
args = getMappingFunctionArgs(splitMappingFunctionRegEx, token)
if args != nil {
if len(args) < 2 {
return BadTransform, []int{}, -1, _EMPTY_, &mappingDestinationErr{token, ErrorMappingDestinationFunctionNotEnoughArguments}
return BadTransform, []int{}, -1, _EMPTY_, &mappingDestinationErr{token, ErrMappingDestinationNotEnoughArgs}
}
if len(args) > 2 {
return BadTransform, []int{}, -1, _EMPTY_, &mappingDestinationErr{token, ErrorMappingDestinationFunctionTooManyArguments}
return BadTransform, []int{}, -1, _EMPTY_, &mappingDestinationErr{token, ErrMappingDestinationTooManyArgs}
}
i, err := strconv.Atoi(strings.Trim(args[0], " "))
if err != nil {
return BadTransform, []int{}, -1, _EMPTY_, &mappingDestinationErr{token, ErrorMappingDestinationFunctionInvalidArgument}
return BadTransform, []int{}, -1, _EMPTY_, &mappingDestinationErr{token, ErrMappingDestinationInvalidArg}
}
if strings.Contains(args[1], " ") || strings.Contains(args[1], tsep) {
return BadTransform, []int{}, -1, _EMPTY_, &mappingDestinationErr{token: token, err: ErrorMappingDestinationFunctionInvalidArgument}
return BadTransform, []int{}, -1, _EMPTY_, &mappingDestinationErr{token: token, err: ErrMappingDestinationInvalidArg}
}

return Split, []int{i}, -1, args[1], nil
Expand Down
1 change: 1 addition & 0 deletions server/subject_transform_test.go
View file
Expand Up @@ -149,6 +149,7 @@ func TestSubjectTransforms(t *testing.T) {
shouldErr("foo.*", "foo.{{wildcard(1,2)}}", false) // Too many arguments passed to the mapping function
shouldErr("foo.*", "foo.{{ wildcard5) }}", false) // Bad mapping function
shouldErr("foo.*", "foo.{{splitLeft(2,2}}", false) // arg out of range
shouldErr("foo", "bla.{{wildcard(1)}}", false) // arg out of range with no wildcard in the source

shouldBeOK := func(src, dest string, strict bool) *subjectTransform {
t.Helper()
Expand Down

0 comments on commit c3234dc

Please sign in to comment.