Bug 2074304: generateRouteHostRegexp: Escape blanks #381

Miciah · 2022-04-11T23:01:59Z

Escape spaces, tabs, carriage returns, and linefeeds in the regular expression for a route's path in HAProxy map files in order to ensure that HAProxy can parse the resulting map files correctly.

Follow-up to #343.

pkg/router/template/util/haproxy/map_entry_test.go (TestGenerateHttpRedirectMapEntry): Add a test with a path with a space, a test with a path with a tab, a test with a path with a carriage return, and a test with a path with a linefeed.
pkg/router/template/util/util.go (GenerateRouteRegexp): Escape spaces, tabs, carriage returns, and linefeeds in the pattern for the route's path.

openshift-ci · 2022-04-11T23:02:02Z

@Miciah: An error was encountered searching for bug 2074304 on the Bugzilla server at https://bugzilla.redhat.com. No known errors were detected, please see the full error message for details.

Full error message.


code 102: You are not authorized to access bug #2074304. Most likely the bug has been restricted for internal development processes and we cannot grant access. If your role requires it then you may be able to use the self service Request Group Membership workflow to gain the permissions required to access this bug. If you are a Red Hat customer with an active subscription, please visit the Red Hat Customer Portal for assistance with your issue If you are a Fedora Project user and require assistance, please consider using one of the mailing lists we host for the Fedora Project.

Please contact an administrator to resolve this issue, then request a bug refresh with /bugzilla refresh.

In response to this:

Bug 2074304: generateRouteHostRegexp: Escape spaces and tabs

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Miciah · 2022-04-12T16:07:50Z

/assign @candita

candita · 2022-04-12T18:29:12Z

pkg/router/template/util/util.go

+	// HAProxy map files.  See
+	// <https://bugzilla.redhat.com/show_bug.cgi?id=2074304>.
+	pathRE = strings.ReplaceAll(pathRE, " ", "\\ ")
+	pathRE = strings.ReplaceAll(pathRE, "\t", "\\\t")


Do we also have to consider newlines?

Newlines don't seem to cause a problem:

% oc -n openshift-ingress-canary create route edge test-route-with-path-with-cr --service=ingress-canary --path=$'/foo\rbar' route.route.openshift.io/test-route-with-path-with-cr created % oc -n openshift-ingress logs -c router -l ingresscontroller.operator.openshift.io/deployment-ingresscontroller=default --tail=5 I0412 19:04:47.854197 1 router.go:618] template "msg"="router reloaded" "output"=" - Checking http://localhost:80 ...\n - Health check ok : 0 retry attempt(s).\n" I0412 19:05:02.484520 1 router.go:618] template "msg"="router reloaded" "output"=" - Checking http://localhost:80 ...\n - Health check ok : 0 retry attempt(s).\n" I0412 19:06:08.034955 1 router.go:618] template "msg"="router reloaded" "output"=" - Checking http://localhost:80 ...\n - Health check ok : 0 retry attempt(s).\n" I0412 19:07:23.523944 1 router.go:618] template "msg"="router reloaded" "output"=" - Checking http://localhost:80 ...\n - Health check ok : 0 retry attempt(s).\n" I0412 19:13:26.570714 1 router.go:618] template "msg"="router reloaded" "output"=" - Checking http://localhost:80 ...\n - Health check ok : 0 retry attempt(s).\n" W0412 19:04:57.246318 1 reflector.go:442] github.com/openshift/router/pkg/router/controller/factory/factory.go:125: watch of *v1.Route ended with: an error on the server ("unable to decode an event from the watch stream: stream error: stream ID 7; INTERNAL_ERROR") has prevented the request from succeeding I0412 19:05:02.477983 1 router.go:618] template "msg"="router reloaded" "output"=" - Checking http://localhost:80 ...\n - Health check ok : 0 retry attempt(s).\n" I0412 19:06:08.034481 1 router.go:618] template "msg"="router reloaded" "output"=" - Checking http://localhost:80 ...\n - Health check ok : 0 retry attempt(s).\n" I0412 19:07:23.513991 1 router.go:618] template "msg"="router reloaded" "output"=" - Checking http://localhost:80 ...\n - Health check ok : 0 retry attempt(s).\n" I0412 19:13:26.571833 1 router.go:618] template "msg"="router reloaded" "output"=" - Checking http://localhost:80 ...\n - Health check ok : 0 retry attempt(s).\n" % oc -n openshift-ingress-canary create route edge test-route-with-path-with-ln --service=ingress-canary --path=$'/foo\nbar' route.route.openshift.io/test-route-with-path-with-ln created % oc -n openshift-ingress logs -c router -l ingresscontroller.operator.openshift.io/deployment-ingresscontroller=default --tail=5 I0412 19:05:02.484520 1 router.go:618] template "msg"="router reloaded" "output"=" - Checking http://localhost:80 ...\n - Health check ok : 0 retry attempt(s).\n" I0412 19:06:08.034955 1 router.go:618] template "msg"="router reloaded" "output"=" - Checking http://localhost:80 ...\n - Health check ok : 0 retry attempt(s).\n" I0412 19:07:23.523944 1 router.go:618] template "msg"="router reloaded" "output"=" - Checking http://localhost:80 ...\n - Health check ok : 0 retry attempt(s).\n" I0412 19:13:26.570714 1 router.go:618] template "msg"="router reloaded" "output"=" - Checking http://localhost:80 ...\n - Health check ok : 0 retry attempt(s).\n" I0412 19:13:36.540289 1 router.go:618] template "msg"="router reloaded" "output"=" - Checking http://localhost:80 ...\n - Health check ok : 0 retry attempt(s).\n" I0412 19:05:02.477983 1 router.go:618] template "msg"="router reloaded" "output"=" - Checking http://localhost:80 ...\n - Health check ok : 0 retry attempt(s).\n" I0412 19:06:08.034481 1 router.go:618] template "msg"="router reloaded" "output"=" - Checking http://localhost:80 ...\n - Health check ok : 0 retry attempt(s).\n" I0412 19:07:23.513991 1 router.go:618] template "msg"="router reloaded" "output"=" - Checking http://localhost:80 ...\n - Health check ok : 0 retry attempt(s).\n" I0412 19:13:26.571833 1 router.go:618] template "msg"="router reloaded" "output"=" - Checking http://localhost:80 ...\n - Health check ok : 0 retry attempt(s).\n" I0412 19:13:36.542543 1 router.go:618] template "msg"="router reloaded" "output"=" - Checking http://localhost:80 ...\n - Health check ok : 0 retry attempt(s).\n"

Hm, but I do see the following in the map file:

^test-route-with-path-with-ln-openshift-ingress-canary\.apps\.ci-ln-kliwjz2-72292\.origin-ci-int-gce\.dev\.rhcloud\.com\.?(:[0-9]+)?/foo bar(/.*)?$ 0 bar(/.*)?$ 0with-path-with-cr-openshift-ingress-canary\.apps\.ci-ln-kliwjz2-72292\.origin-ci-int-gce\.dev\.rhcloud\.com\.?(:[0-9]+)?/foo

I'll add a check for \n.

frobware · 2022-04-12T18:32:44Z

/lgtm

frobware · 2022-04-12T18:34:11Z

I saw @candita had some questions, so...

/hold

candita · 2022-04-12T18:38:30Z

pkg/router/template/util/util.go

@@ -51,6 +51,12 @@ func GenerateRouteRegexp(hostname, path string, wildcard bool) string {
 		subpathRE = "(/.*)?"
 	}

+	// The path could contain space characters, which must be escaped in


Could the host also contain space characters?

No, the API would reject it:

% oc -n openshift-ingress-canary create route edge test-route-with-host-with-space --service=ingress-canary --hostname='www.foo bar.com' The Route "test-route-with-host-with-space" is invalid: * spec.host: Invalid value: "www.foo bar.com": host must conform to DNS 952 subdomain conventions * spec.host: Invalid value: "www.foo bar.com": a lowercase RFC 1123 label must consist of lower case alphanumeric characters or '-', and must start and end with an alphanumeric character (e.g. 'my-name', or '123-abc', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?') zsh: exit 1 oc -n openshift-ingress-canary create route edge --service=ingress-canary

candita · 2022-04-12T20:03:31Z

/unhold
/lgtm

Escape spaces, tabs, carriage returns, and linefeeds in the regular expression for a route's path in HAProxy map files in order to ensure that HAProxy can parse the resulting map files correctly. Follow-up to commit 019c5ac. This commit fixes bug 2074304. https://bugzilla.redhat.com/show_bug.cgi?id=2074304 * pkg/router/template/util/haproxy/map_entry_test.go (TestGenerateHttpRedirectMapEntry): Add a test with a path with a space, a test with a path with a tab, a test with a path with a carriage return, and a test with a path with a linefeed. * pkg/router/template/util/util.go (GenerateRouteRegexp): Escape spaces, tabs, carriage returns, and linefeeds in the pattern for the route's path.

Miciah · 2022-04-12T20:24:03Z

Turns out \ didn't entirely fix the problem, so I have changed it to \x20.

Miciah · 2022-04-12T21:11:03Z

Here are the results of my testing with the current iteration of this PR:

% oc -n openshift-ingress-canary create route edge test-route-with-path-with-space --service=ingress-canary --path='/foo bar' 
route.route.openshift.io/test-route-with-path-with-space created
% oc -n openshift-ingress-canary create route edge test-route-with-path-with-cr --service=ingress-canary --path=$'/foo\rbar'  
route.route.openshift.io/test-route-with-path-with-cr created
% oc -n openshift-ingress-canary create route edge test-route-with-path-with-lf --service=ingress-canary --path=$'/foo\nbar'
route.route.openshift.io/test-route-with-path-with-lf created
% oc -n openshift-ingress logs -c router -l ingresscontroller.operator.openshift.io/deployment-ingresscontroller=default --tail=5 
I0412 20:55:46.668322       1 router.go:618] template "msg"="router reloaded"  "output"=" - Checking http://localhost:80 ...\n - Health check ok : 0 retry attempt(s).\n"
I0412 20:56:49.327631       1 router.go:618] template "msg"="router reloaded"  "output"=" - Checking http://localhost:80 ...\n - Health check ok : 0 retry attempt(s).\n"
I0412 21:08:53.635017       1 router.go:618] template "msg"="router reloaded"  "output"=" - Checking http://localhost:80 ...\n - Health check ok : 0 retry attempt(s).\n"
I0412 21:08:58.633818       1 router.go:618] template "msg"="router reloaded"  "output"=" - Checking http://localhost:80 ...\n - Health check ok : 0 retry attempt(s).\n"
I0412 21:09:03.631756       1 router.go:618] template "msg"="router reloaded"  "output"=" - Checking http://localhost:80 ...\n - Health check ok : 0 retry attempt(s).\n"
I0412 20:55:46.663206       1 router.go:618] template "msg"="router reloaded"  "output"=" - Checking http://localhost:80 ...\n - Health check ok : 0 retry attempt(s).\n"
I0412 20:56:49.327559       1 router.go:618] template "msg"="router reloaded"  "output"=" - Checking http://localhost:80 ...\n - Health check ok : 0 retry attempt(s).\n"
I0412 21:08:53.628625       1 router.go:618] template "msg"="router reloaded"  "output"=" - Checking http://localhost:80 ...\n - Health check ok : 0 retry attempt(s).\n"
I0412 21:08:58.626378       1 router.go:618] template "msg"="router reloaded"  "output"=" - Checking http://localhost:80 ...\n - Health check ok : 0 retry attempt(s).\n"
I0412 21:09:03.629454       1 router.go:618] template "msg"="router reloaded"  "output"=" - Checking http://localhost:80 ...\n - Health check ok : 0 retry attempt(s).\n"
% for pod in $(oc -n openshift-ingress get pods -l ingresscontroller.operator.openshift.io/deployment-ingresscontroller=default -o name); do echo "=== $pod ==="; oc -n openshift-ingress rsh -c router $pod cat os_route_http_redirect.map; echo; done
=== pod/router-default-7b66dc6696-985qz ===
^thanos-querier-openshift-monitoring\.apps\.ci-ln-55lv57b-72292\.origin-ci-int-gce\.dev\.rhcloud\.com\.?(:[0-9]+)?/api(/.*)?$ 1
^test-route-with-path-with-space-openshift-ingress-canary\.apps\.ci-ln-55lv57b-72292\.origin-ci-int-gce\.dev\.rhcloud\.com\.?(:[0-9]+)?/foo\x20bar(/.*)?$ 0
^test-route-with-path-with-lf-openshift-ingress-canary\.apps\.ci-ln-55lv57b-72292\.origin-ci-int-gce\.dev\.rhcloud\.com\.?(:[0-9]+)?/foo\nbar(/.*)?$ 0
^test-route-with-path-with-cr-openshift-ingress-canary\.apps\.ci-ln-55lv57b-72292\.origin-ci-int-gce\.dev\.rhcloud\.com\.?(:[0-9]+)?/foo\rbar(/.*)?$ 0
^prometheus-k8s-openshift-monitoring\.apps\.ci-ln-55lv57b-72292\.origin-ci-int-gce\.dev\.rhcloud\.com\.?(:[0-9]+)?/api(/.*)?$ 1
^prometheus-k8s-federate-openshift-monitoring\.apps\.ci-ln-55lv57b-72292\.origin-ci-int-gce\.dev\.rhcloud\.com\.?(:[0-9]+)?/federate(/.*)?$ 1
^oauth-openshift\.apps\.ci-ln-55lv57b-72292\.origin-ci-int-gce\.dev\.rhcloud\.com\.?(:[0-9]+)?(/.*)?$ 1
^downloads-openshift-console\.apps\.ci-ln-55lv57b-72292\.origin-ci-int-gce\.dev\.rhcloud\.com\.?(:[0-9]+)?(/.*)?$ 1
^console-openshift-console\.apps\.ci-ln-55lv57b-72292\.origin-ci-int-gce\.dev\.rhcloud\.com\.?(:[0-9]+)?(/.*)?$ 1
^canary-openshift-ingress-canary\.apps\.ci-ln-55lv57b-72292\.origin-ci-int-gce\.dev\.rhcloud\.com\.?(:[0-9]+)?(/.*)?$ 1
^alertmanager-main-openshift-monitoring\.apps\.ci-ln-55lv57b-72292\.origin-ci-int-gce\.dev\.rhcloud\.com\.?(:[0-9]+)?/api(/.*)?$ 1

=== pod/router-default-7b66dc6696-dmlbh ===
^thanos-querier-openshift-monitoring\.apps\.ci-ln-55lv57b-72292\.origin-ci-int-gce\.dev\.rhcloud\.com\.?(:[0-9]+)?/api(/.*)?$ 1
^test-route-with-path-with-space-openshift-ingress-canary\.apps\.ci-ln-55lv57b-72292\.origin-ci-int-gce\.dev\.rhcloud\.com\.?(:[0-9]+)?/foo\x20bar(/.*)?$ 0
^test-route-with-path-with-lf-openshift-ingress-canary\.apps\.ci-ln-55lv57b-72292\.origin-ci-int-gce\.dev\.rhcloud\.com\.?(:[0-9]+)?/foo\nbar(/.*)?$ 0
^test-route-with-path-with-cr-openshift-ingress-canary\.apps\.ci-ln-55lv57b-72292\.origin-ci-int-gce\.dev\.rhcloud\.com\.?(:[0-9]+)?/foo\rbar(/.*)?$ 0
^prometheus-k8s-openshift-monitoring\.apps\.ci-ln-55lv57b-72292\.origin-ci-int-gce\.dev\.rhcloud\.com\.?(:[0-9]+)?/api(/.*)?$ 1
^prometheus-k8s-federate-openshift-monitoring\.apps\.ci-ln-55lv57b-72292\.origin-ci-int-gce\.dev\.rhcloud\.com\.?(:[0-9]+)?/federate(/.*)?$ 1
^oauth-openshift\.apps\.ci-ln-55lv57b-72292\.origin-ci-int-gce\.dev\.rhcloud\.com\.?(:[0-9]+)?(/.*)?$ 1
^downloads-openshift-console\.apps\.ci-ln-55lv57b-72292\.origin-ci-int-gce\.dev\.rhcloud\.com\.?(:[0-9]+)?(/.*)?$ 1
^console-openshift-console\.apps\.ci-ln-55lv57b-72292\.origin-ci-int-gce\.dev\.rhcloud\.com\.?(:[0-9]+)?(/.*)?$ 1
^canary-openshift-ingress-canary\.apps\.ci-ln-55lv57b-72292\.origin-ci-int-gce\.dev\.rhcloud\.com\.?(:[0-9]+)?(/.*)?$ 1
^alertmanager-main-openshift-monitoring\.apps\.ci-ln-55lv57b-72292\.origin-ci-int-gce\.dev\.rhcloud\.com\.?(:[0-9]+)?/api(/.*)?$ 1

No errors logged, and the map files look reasonable.

candita · 2022-04-13T13:45:02Z

/retest

openshift-ci · 2022-04-13T15:47:15Z

@Miciah: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-metal-ipi-ovn-ipv6	`30dd5b7`	link	false	`/test e2e-metal-ipi-ovn-ipv6`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

candita · 2022-04-13T16:55:40Z

/lgtm

openshift-bot · 2022-04-16T17:41:22Z

/retest-required