Skip to content

Commit

Permalink
Fixed validation for VSR exact & regex subroutes (#4744)
Browse files Browse the repository at this point in the history 
* Fixed validation for VSR exact & regex subroutes

Since a VSR may only contain a single subroute when using exact or
regex matching skipping route validation here is incorrect.

Correctly configured VSRs for regex or exact will be validated
(and terminate) earlier in function, ie. where VS path is regex +
only 1 subroute + subroute syntax check passes.

* Added Unit Tests

* Fix validation

Signed-off-by: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com>

* Add unit tests and update docs

Signed-off-by: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com>

* Adjusted Unit test messages.

---------

Signed-off-by: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com>
Co-authored-by: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com>
Co-authored-by: Venktesh Shivam Patel <ve.patel@f5.com>
  • Loading branch information
@jo-carter @haywoodsh @vepatel
3 people committed Feb 6, 2024
1 parent fb60fb6 commit 3787bb8
Show file tree
Hide file tree
Showing 3 changed files with 204 additions and 15 deletions.
2 changes: 1 addition & 1 deletion docs/content/configuration/virtualserver-and-virtualserverroute-resources.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,7 @@ action:
{{% table %}}
|Field | Description | Type | Required |
| ---| ---| ---| --- |
|``path`` | The path of the subroute. NGINX will match it against the URI of a request. Possible values are: a prefix ( ``/`` , ``/path`` ), an exact match ( ``=/exact/match`` ), a case insensitive regular expression ( ``~*^/Bar.*\.jpg`` ) or a case sensitive regular expression ( ``~^/foo.*\.jpg`` ). In the case of a prefix, the path must start with the same path as the path of the route of the VirtualServer that references this resource. In the case of an exact or regex match, the path must be the same as the path of the route of the VirtualServer that references this resource. In the case of a prefix or an exact match, the path must not include any whitespace characters, ``{`` , ``}`` or ``;``. In the case of the regex matches, all double quotes ``"`` must be escaped and the match can't end in an unescaped backslash ``\``. The path must be unique among the paths of all subroutes of the VirtualServerRoute. | ``string`` | Yes |
|``path`` | The path of the subroute. NGINX will match it against the URI of a request. Possible values are: a prefix ( ``/`` , ``/path`` ), an exact match ( ``=/exact/match`` ), a case insensitive regular expression ( ``~*^/Bar.*\.jpg`` ) or a case sensitive regular expression ( ``~^/foo.*\.jpg`` ). In the case of a prefix, the path must start with the same path as the path of the route of the VirtualServer that references this resource. In the case of an exact or regex match, the path must be the same as the path of the route of the VirtualServer that references this resource. A matching path of the route of the VirtualServer but in different type is not accepted, e.g. a regex path (`~/match`) cannot be used with a prefix path in VirtualServer (`/match`) In the case of a prefix or an exact match, the path must not include any whitespace characters, ``{`` , ``}`` or ``;``. In the case of the regex matches, all double quotes ``"`` must be escaped and the match can't end in an unescaped backslash ``\``. The path must be unique among the paths of all subroutes of the VirtualServerRoute. | ``string`` | Yes |
|``policies`` | A list of policies. The policies override *all* policies defined in the route of the VirtualServer that references this resource. The policies also override the policies of the same type defined in the ``spec`` of the VirtualServer. See [Applying Policies](/nginx-ingress-controller/configuration/policy-resource/#applying-policies) for more details. | [[]policy](#virtualserverpolicy) | No |
|``action`` | The default action to perform for a request. | [action](#action) | No |
|``dos`` | A reference to a DosProtectedResource, setting this enables DOS protection of the VirtualServerRoute subroute. | ``string`` | No |
Expand Down
4 changes: 2 additions & 2 deletions pkg/apis/configuration/validation/virtualserver.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1466,7 +1466,7 @@ func isValidMatchValue(value string) []string {

// ValidateVirtualServerRoute validates a VirtualServerRoute.
func (vsv *VirtualServerValidator) ValidateVirtualServerRoute(virtualServerRoute *v1.VirtualServerRoute) error {
allErrs := vsv.validateVirtualServerRouteSpec(&virtualServerRoute.Spec, field.NewPath("spec"), "", "/", virtualServerRoute.Namespace)
allErrs := vsv.validateVirtualServerRouteSpec(&virtualServerRoute.Spec, field.NewPath("spec"), "", "", virtualServerRoute.Namespace)
return allErrs.ToAggregate()
}

Expand Down Expand Up @@ -1527,7 +1527,7 @@ func (vsv *VirtualServerValidator) validateVirtualServerRouteSubroutes(routes []
isRouteFieldForbidden := true
routeErrs := vsv.validateRoute(r, idxPath, upstreamNames, isRouteFieldForbidden, namespace)

if vsPath != "" && !strings.HasPrefix(r.Path, vsPath) && !isRegexOrExactMatch(r.Path) {
if vsPath != "" && !strings.HasPrefix(r.Path, vsPath) {
msg := fmt.Sprintf("must start with '%s'", vsPath)
routeErrs = append(routeErrs, field.Invalid(idxPath, r.Path, msg))
}
Expand Down
213 changes: 201 additions & 12 deletions pkg/apis/configuration/validation/virtualserver_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2426,13 +2426,13 @@ func TestValidateVirtualServerRouteSubroutes(t *testing.T) {
tests := []struct {
routes []v1.Route
upstreamNames sets.Set[string]
pathPrefix string
vsPath string
msg string
}{
{
routes: []v1.Route{},
upstreamNames: sets.Set[string]{},
pathPrefix: "/",
vsPath: "/",
msg: "no routes",
},
{
Expand All @@ -2447,16 +2447,82 @@ func TestValidateVirtualServerRouteSubroutes(t *testing.T) {
upstreamNames: map[string]sets.Empty{
"test": {},
},
pathPrefix: "/",
msg: "valid route",
vsPath: "/",
msg: "valid prefix route",
},
{
routes: []v1.Route{
{
Path: "/",
Action: &v1.Action{
Pass: "test",
},
},
{
Path: "/test",
Action: &v1.Action{
Pass: "test",
},
},
},
upstreamNames: map[string]sets.Empty{
"test": {},
},
vsPath: "/",
msg: "valid route prefix with two paths",
},
{
routes: []v1.Route{
{
Path: "~/test",
Action: &v1.Action{
Pass: "test",
},
},
},
upstreamNames: map[string]sets.Empty{
"test": {},
},
vsPath: "~/test",
msg: "valid regex route",
},
{
routes: []v1.Route{
{
Path: "~ /regex1/?(.*)",
Action: &v1.Action{
Pass: "test",
},
},
},
upstreamNames: map[string]sets.Empty{
"test": {},
},
vsPath: "~ /regex1/?(.*)",
msg: "valid regex route",
},
{
routes: []v1.Route{
{
Path: "=/test",
Action: &v1.Action{
Pass: "test",
},
},
},
upstreamNames: map[string]sets.Empty{
"test": {},
},
vsPath: "=/test",
msg: "valid exact route",
},
}

vsv := &VirtualServerValidator{isPlus: false}

for _, test := range tests {
allErrs := vsv.validateVirtualServerRouteSubroutes(test.routes, field.NewPath("subroutes"), test.upstreamNames,
test.pathPrefix, "default")
test.vsPath, "default")
if len(allErrs) > 0 {
t.Errorf("validateVirtualServerRouteSubroutes() returned errors %v for valid input for the case of %s", allErrs, test.msg)
}
Expand All @@ -2468,7 +2534,7 @@ func TestValidateVirtualServerRouteSubroutesFails(t *testing.T) {
tests := []struct {
routes []v1.Route
upstreamNames sets.Set[string]
pathPrefix string
vsPath string
msg string
}{
{
Expand All @@ -2490,8 +2556,8 @@ func TestValidateVirtualServerRouteSubroutesFails(t *testing.T) {
"test-1": {},
"test-2": {},
},
pathPrefix: "/",
msg: "duplicated paths",
vsPath: "/",
msg: "duplicated paths",
},
{
routes: []v1.Route{
Expand All @@ -2501,7 +2567,7 @@ func TestValidateVirtualServerRouteSubroutesFails(t *testing.T) {
},
},
upstreamNames: map[string]sets.Empty{},
pathPrefix: "",
vsPath: "",
msg: "invalid route",
},
{
Expand All @@ -2516,16 +2582,139 @@ func TestValidateVirtualServerRouteSubroutesFails(t *testing.T) {
upstreamNames: map[string]sets.Empty{
"test-1": {},
},
pathPrefix: "/abc",
msg: "invalid prefix",
vsPath: "/abc",
msg: "invalid prefix",
},
{
routes: []v1.Route{
{
Path: "/abc",
Action: &v1.Action{
Pass: "test-1",
},
},
{
Path: "~^/test",
Action: &v1.Action{
Pass: "test-1",
},
},
},
upstreamNames: map[string]sets.Empty{
"test-1": {},
},
vsPath: "/abc",
msg: "prefix vs path with both matching prefix and mismatching regex subroute path",
},
{
routes: []v1.Route{
{
Path: "~/test",
Action: &v1.Action{
Pass: "test-1",
},
},
},
upstreamNames: map[string]sets.Empty{
"test-1": {},
},
vsPath: "/test",
msg: "prefix vs path with matching regex subroute path",
},
{
routes: []v1.Route{
{
Path: "=/test",
Action: &v1.Action{
Pass: "test-1",
},
},
},
upstreamNames: map[string]sets.Empty{
"test-1": {},
},
vsPath: "/test",
msg: "prefix vs path with matching exact subroute path",
},
{
routes: []v1.Route{
{
Path: "/test",
Action: &v1.Action{
Pass: "test-1",
},
},
},
upstreamNames: map[string]sets.Empty{
"test-1": {},
},
vsPath: "=/test",
msg: "exact vs path with prefix subroute path",
},
{
routes: []v1.Route{
{
Path: "=/test",
Action: &v1.Action{
Pass: "test-1",
},
},
},
upstreamNames: map[string]sets.Empty{
"test-1": {},
},
vsPath: "~/test",
msg: "regex vs path with exact subroute path",
},
{
routes: []v1.Route{
{
Path: "=/test",
Action: &v1.Action{
Pass: "test-1",
},
},
{
Path: "/abc",
Action: &v1.Action{
Pass: "test-1",
},
},
},
upstreamNames: map[string]sets.Empty{
"test-1": {},
},
vsPath: "/abc",
msg: "prefix vs path with both exact and matching prefix subroute path",
},
{
routes: []v1.Route{
{
Path: "~/abc",
Action: &v1.Action{
Pass: "test-1",
},
},
{
Path: "/test",
Action: &v1.Action{
Pass: "test-1",
},
},
},
upstreamNames: map[string]sets.Empty{
"test-1": {},
},
vsPath: "/test",
msg: "prefix vs path with both regex and matching prefix subroute path",
},
}

vsv := &VirtualServerValidator{isPlus: false}

for _, test := range tests {
allErrs := vsv.validateVirtualServerRouteSubroutes(test.routes, field.NewPath("subroutes"), test.upstreamNames,
test.pathPrefix, "default")
test.vsPath, "default")
if len(allErrs) == 0 {
t.Errorf("validateVirtualServerRouteSubroutes() returned no errors for the case of %s", test.msg)
}
Expand Down

0 comments on commit 3787bb8

Please sign in to comment.