Skip to content

Commit e166460

Browse files
jo-carterhaywoodshvepatel
authored and
pdabelf5
committed
Fixed validation for VSR exact & regex subroutes (#4744)
* 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>
1 parent 8acba14 commit e166460

File tree

3 files changed

+204
-15
lines changed

3 files changed

+204
-15
lines changed

‎docs/content/configuration/virtualserver-and-virtualserverroute-resources.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -301,7 +301,7 @@ action:
301301
{{% table %}}
302302
|Field | Description | Type | Required |
303303
| ---| ---| ---| --- |
304-
|``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 |
304+
|``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 |
305305
|``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 |
306306
|``action`` | The default action to perform for a request. | [action](#action) | No |
307307
|``dos`` | A reference to a DosProtectedResource, setting this enables DOS protection of the VirtualServerRoute subroute. | ``string`` | No |

‎pkg/apis/configuration/validation/virtualserver.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1466,7 +1466,7 @@ func isValidMatchValue(value string) []string {
14661466

14671467
// ValidateVirtualServerRoute validates a VirtualServerRoute.
14681468
func (vsv *VirtualServerValidator) ValidateVirtualServerRoute(virtualServerRoute *v1.VirtualServerRoute) error {
1469-
allErrs := vsv.validateVirtualServerRouteSpec(&virtualServerRoute.Spec, field.NewPath("spec"), "", "/", virtualServerRoute.Namespace)
1469+
allErrs := vsv.validateVirtualServerRouteSpec(&virtualServerRoute.Spec, field.NewPath("spec"), "", "", virtualServerRoute.Namespace)
14701470
return allErrs.ToAggregate()
14711471
}
14721472

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

1530-
if vsPath != "" && !strings.HasPrefix(r.Path, vsPath) && !isRegexOrExactMatch(r.Path) {
1530+
if vsPath != "" && !strings.HasPrefix(r.Path, vsPath) {
15311531
msg := fmt.Sprintf("must start with '%s'", vsPath)
15321532
routeErrs = append(routeErrs, field.Invalid(idxPath, r.Path, msg))
15331533
}

‎pkg/apis/configuration/validation/virtualserver_test.go‎

Lines changed: 201 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2426,13 +2426,13 @@ func TestValidateVirtualServerRouteSubroutes(t *testing.T) {
24262426
tests := []struct {
24272427
routes []v1.Route
24282428
upstreamNames sets.Set[string]
2429-
pathPrefix string
2429+
vsPath string
24302430
msg string
24312431
}{
24322432
{
24332433
routes: []v1.Route{},
24342434
upstreamNames: sets.Set[string]{},
2435-
pathPrefix: "/",
2435+
vsPath: "/",
24362436
msg: "no routes",
24372437
},
24382438
{
@@ -2447,16 +2447,82 @@ func TestValidateVirtualServerRouteSubroutes(t *testing.T) {
24472447
upstreamNames: map[string]sets.Empty{
24482448
"test": {},
24492449
},
2450-
pathPrefix: "/",
2451-
msg: "valid route",
2450+
vsPath: "/",
2451+
msg: "valid prefix route",
2452+
},
2453+
{
2454+
routes: []v1.Route{
2455+
{
2456+
Path: "/",
2457+
Action: &v1.Action{
2458+
Pass: "test",
2459+
},
2460+
},
2461+
{
2462+
Path: "/test",
2463+
Action: &v1.Action{
2464+
Pass: "test",
2465+
},
2466+
},
2467+
},
2468+
upstreamNames: map[string]sets.Empty{
2469+
"test": {},
2470+
},
2471+
vsPath: "/",
2472+
msg: "valid route prefix with two paths",
2473+
},
2474+
{
2475+
routes: []v1.Route{
2476+
{
2477+
Path: "~/test",
2478+
Action: &v1.Action{
2479+
Pass: "test",
2480+
},
2481+
},
2482+
},
2483+
upstreamNames: map[string]sets.Empty{
2484+
"test": {},
2485+
},
2486+
vsPath: "~/test",
2487+
msg: "valid regex route",
2488+
},
2489+
{
2490+
routes: []v1.Route{
2491+
{
2492+
Path: "~ /regex1/?(.*)",
2493+
Action: &v1.Action{
2494+
Pass: "test",
2495+
},
2496+
},
2497+
},
2498+
upstreamNames: map[string]sets.Empty{
2499+
"test": {},
2500+
},
2501+
vsPath: "~ /regex1/?(.*)",
2502+
msg: "valid regex route",
2503+
},
2504+
{
2505+
routes: []v1.Route{
2506+
{
2507+
Path: "=/test",
2508+
Action: &v1.Action{
2509+
Pass: "test",
2510+
},
2511+
},
2512+
},
2513+
upstreamNames: map[string]sets.Empty{
2514+
"test": {},
2515+
},
2516+
vsPath: "=/test",
2517+
msg: "valid exact route",
24522518
},
24532519
}
24542520

24552521
vsv := &VirtualServerValidator{isPlus: false}
24562522

24572523
for _, test := range tests {
24582524
allErrs := vsv.validateVirtualServerRouteSubroutes(test.routes, field.NewPath("subroutes"), test.upstreamNames,
2459-
test.pathPrefix, "default")
2525+
test.vsPath, "default")
24602526
if len(allErrs) > 0 {
24612527
t.Errorf("validateVirtualServerRouteSubroutes() returned errors %v for valid input for the case of %s", allErrs, test.msg)
24622528
}
@@ -2468,7 +2534,7 @@ func TestValidateVirtualServerRouteSubroutesFails(t *testing.T) {
24682534
tests := []struct {
24692535
routes []v1.Route
24702536
upstreamNames sets.Set[string]
2471-
pathPrefix string
2537+
vsPath string
24722538
msg string
24732539
}{
24742540
{
@@ -2490,8 +2556,8 @@ func TestValidateVirtualServerRouteSubroutesFails(t *testing.T) {
24902556
"test-1": {},
24912557
"test-2": {},
24922558
},
2493-
pathPrefix: "/",
2494-
msg: "duplicated paths",
2559+
vsPath: "/",
2560+
msg: "duplicated paths",
24952561
},
24962562
{
24972563
routes: []v1.Route{
@@ -2501,7 +2567,7 @@ func TestValidateVirtualServerRouteSubroutesFails(t *testing.T) {
25012567
},
25022568
},
25032569
upstreamNames: map[string]sets.Empty{},
2504-
pathPrefix: "",
2570+
vsPath: "",
25052571
msg: "invalid route",
25062572
},
25072573
{
@@ -2516,16 +2582,139 @@ func TestValidateVirtualServerRouteSubroutesFails(t *testing.T) {
25162582
upstreamNames: map[string]sets.Empty{
25172583
"test-1": {},
25182584
},
2519-
pathPrefix: "/abc",
2520-
msg: "invalid prefix",
2585+
vsPath: "/abc",
2586+
msg: "invalid prefix",
2587+
},
2588+
{
2589+
routes: []v1.Route{
2590+
{
2591+
Path: "/abc",
2592+
Action: &v1.Action{
2593+
Pass: "test-1",
2594+
},
2595+
},
2596+
{
2597+
Path: "~^/test",
2598+
Action: &v1.Action{
2599+
Pass: "test-1",
2600+
},
2601+
},
2602+
},
2603+
upstreamNames: map[string]sets.Empty{
2604+
"test-1": {},
2605+
},
2606+
vsPath: "/abc",
2607+
msg: "prefix vs path with both matching prefix and mismatching regex subroute path",
2608+
},
2609+
{
2610+
routes: []v1.Route{
2611+
{
2612+
Path: "~/test",
2613+
Action: &v1.Action{
2614+
Pass: "test-1",
2615+
},
2616+
},
2617+
},
2618+
upstreamNames: map[string]sets.Empty{
2619+
"test-1": {},
2620+
},
2621+
vsPath: "/test",
2622+
msg: "prefix vs path with matching regex subroute path",
2623+
},
2624+
{
2625+
routes: []v1.Route{
2626+
{
2627+
Path: "=/test",
2628+
Action: &v1.Action{
2629+
Pass: "test-1",
2630+
},
2631+
},
2632+
},
2633+
upstreamNames: map[string]sets.Empty{
2634+
"test-1": {},
2635+
},
2636+
vsPath: "/test",
2637+
msg: "prefix vs path with matching exact subroute path",
2638+
},
2639+
{
2640+
routes: []v1.Route{
2641+
{
2642+
Path: "/test",
2643+
Action: &v1.Action{
2644+
Pass: "test-1",
2645+
},
2646+
},
2647+
},
2648+
upstreamNames: map[string]sets.Empty{
2649+
"test-1": {},
2650+
},
2651+
vsPath: "=/test",
2652+
msg: "exact vs path with prefix subroute path",
2653+
},
2654+
{
2655+
routes: []v1.Route{
2656+
{
2657+
Path: "=/test",
2658+
Action: &v1.Action{
2659+
Pass: "test-1",
2660+
},
2661+
},
2662+
},
2663+
upstreamNames: map[string]sets.Empty{
2664+
"test-1": {},
2665+
},
2666+
vsPath: "~/test",
2667+
msg: "regex vs path with exact subroute path",
2668+
},
2669+
{
2670+
routes: []v1.Route{
2671+
{
2672+
Path: "=/test",
2673+
Action: &v1.Action{
2674+
Pass: "test-1",
2675+
},
2676+
},
2677+
{
2678+
Path: "/abc",
2679+
Action: &v1.Action{
2680+
Pass: "test-1",
2681+
},
2682+
},
2683+
},
2684+
upstreamNames: map[string]sets.Empty{
2685+
"test-1": {},
2686+
},
2687+
vsPath: "/abc",
2688+
msg: "prefix vs path with both exact and matching prefix subroute path",
2689+
},
2690+
{
2691+
routes: []v1.Route{
2692+
{
2693+
Path: "~/abc",
2694+
Action: &v1.Action{
2695+
Pass: "test-1",
2696+
},
2697+
},
2698+
{
2699+
Path: "/test",
2700+
Action: &v1.Action{
2701+
Pass: "test-1",
2702+
},
2703+
},
2704+
},
2705+
upstreamNames: map[string]sets.Empty{
2706+
"test-1": {},
2707+
},
2708+
vsPath: "/test",
2709+
msg: "prefix vs path with both regex and matching prefix subroute path",
25212710
},
25222711
}
25232712

25242713
vsv := &VirtualServerValidator{isPlus: false}
25252714

25262715
for _, test := range tests {
25272716
allErrs := vsv.validateVirtualServerRouteSubroutes(test.routes, field.NewPath("subroutes"), test.upstreamNames,
2528-
test.pathPrefix, "default")
2717+
test.vsPath, "default")
25292718
if len(allErrs) == 0 {
25302719
t.Errorf("validateVirtualServerRouteSubroutes() returned no errors for the case of %s", test.msg)
25312720
}

0 commit comments

Comments
 (0)