Prohibit passthrough route with path #6037

ramr · 2015-11-24T01:02:48Z

Newer version of PR #4282 - fixed up for new Spec.* fields.

It is impossible to support paths with passthrough termination because the router cannot determine the path in the request without decrypting the connection.

…nd fix gofmt errors.

liggitt · 2015-11-24T01:41:53Z

pkg/route/api/validation/validation.go

@@ -32,6 +32,11 @@ func ValidateRoute(route *routeapi.Route) fielderrors.ValidationErrorList {
 		result = append(result, fielderrors.NewFieldInvalid("path", route.Spec.Path, "path must begin with /"))
 	}

+	if len(route.Spec.Path) > 0 && route.Spec.TLS != nil &&
+		route.Spec.TLS.Termination == routeapi.TLSTerminationPassthrough {


What will the system do with existing routes that don't validate?

Will that break route status update?

@liggitt Not sure if I understood you correctly, there should be no changes vis-a-vis the fields - they should be converted from the old to the new schema.

If you mean for passthrough routes with paths, well those would have been redundant anyway - would have never worked ("cruft" in the datastore / etcd) as they would have just generated unused config. So this is actually indicating that those routes are invalid and it would log errors (and wouldn't try to add a route to the router).

And re: route status update - no, this is not called in the path via ValidateRouteStatusUpdate, so it would not affect status update.

We have to be careful with tightening validation on things "that would never have worked"… we can do it as long as we're sure that no automated systems will be hot looping or logging thousands of events/validation failures if a previously "valid" persisted object suddenly doesn't pass validation after an upgrade

Well, that one's a doozy - can we ever be sure of that? What we don't know of could always exist on a different plane of existence (aka schrodinger's cat). There could well be a script out there that expects things that worked in v1beta1 to work now or in the future and are we absolutely certain that we can ever have a 100% temporal guarantee on that. IMHO, within a reasonable scheme/limit is good. And in this specific case, this is something that would not have worked in the first place. Just me tuppence.

I'm referring to things we have allowed through validation in supported releases

That argument could be made for v1 to v2 to v3 as well, no. My point is, if someone was using this - it would have never worked + the docs do mention it as well ref: https://docs.openshift.org/latest/architecture/core_concepts/routes.html#path-based-routes
We could argue the reverse direction to say hey someone could well do that but that's would cover a lot of area (the different planes of existence above) we don't know about.

updated link

Not saying we can't tighten this, I'm just saying we have to be very careful, and we need to audit how existing routes (especially route status) are being updated/validated:

we can do it as long as we're sure that no automated systems will be hot looping or logging thousands of events/validation failures if a previously "valid" persisted object suddenly doesn't pass validation after an upgrade

We ran into this when validation around pod spec ports changed, and invalid replication controller data caused hundreds of thousands of pod creation failures and logged events.

Miciah · 2015-11-24T14:30:06Z

pkg/route/api/validation/validation.go

@@ -32,6 +32,11 @@ func ValidateRoute(route *routeapi.Route) fielderrors.ValidationErrorList {
 		result = append(result, fielderrors.NewFieldInvalid("path", route.Spec.Path, "path must begin with /"))
 	}

+	if len(route.Spec.Path) > 0 && route.Spec.TLS != nil &&
+		route.Spec.TLS.Termination == routeapi.TLSTerminationPassthrough {
+		result = append(result, fielderrors.NewFieldInvalid("path", route.Spec.Path, "paththrough termination does not support paths"))


s/paththrough/passthrough/

ramr · 2015-11-24T18:03:14Z

@Miciah fixed the typo - good catch.

@liggitt fair enough, something to definitely to do (+ keep in mind).

ramr · 2015-12-03T03:14:54Z

@rajatchopra /bump Thx

0xmichalis · 2016-01-12T15:03:08Z

LGTM

If passthrough path-based routes are already not working, then this should be ok.
@liggitt @deads2k

deads2k · 2016-01-12T15:32:10Z

If passthrough path-based routes are already not working, then this should be ok.

Agreed. If it never worked before and the controller won't fail on status updates because of this, I think its ok to tighten.

I see this as another reason to check resource validity in oc status. It's not perfect, but I think helps.

@jwforres For your client-side validation.

0xmichalis · 2016-01-12T15:46:03Z

@deads2k once this gets merged, we wont be able to test oc status against broken fixtures due to tighter validation (similar to what happened in #6076). I am not sure if this is a separate issue for oc status.

jwforres · 2016-01-12T15:52:54Z

CC @spadgett

0xmichalis · 2016-01-12T16:15:40Z

@deads2k once this gets merged, we wont be able to test oc status against broken fixtures due to tighter validation (similar to what happened in #6076). I am not sure if this is a separate issue for oc status.

Nevermind, the error in 6076 wasn't due to validation. I pulled this branch, rebased it on top of latest, cherry-picked the changes from #6092 and the unit tests work fine. Also couldn't find any validation in BuildGraph.

0xmichalis · 2016-01-20T12:51:51Z

[merge]

openshift-bot · 2016-01-20T12:55:25Z

[Test]ing while waiting on the merge queue

openshift-bot · 2016-01-20T13:00:34Z

Evaluated for origin test up to f174dce

openshift-bot · 2016-01-20T14:18:24Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/179/)

openshift-bot · 2016-01-20T14:20:33Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/4675/) (Image: devenv-rhel7_3192)

0xmichalis · 2016-01-20T14:26:53Z

#6738 flake

[merge]

openshift-bot · 2016-01-20T14:30:34Z

Evaluated for origin merge up to f174dce

Merged by openshift-bot

Prohibit passthrough route with path

6a24244

It is impossible to support paths with passthrough termination because the router cannot determine the path in the request without decrypting the connection.

ramr mentioned this pull request Nov 24, 2015

Prohibit passthrough route with path #4282

Closed

Fix old PR openshift#4282 code and tests to use new layout (Spec.*) a…

52742df

…nd fix gofmt errors.

ramr force-pushed the prohibit-passthrough-route-with-path branch from c36893c to 52742df Compare November 24, 2015 01:36

liggitt reviewed Nov 24, 2015
View reviewed changes

liggitt added component/restapi component/routing area/compatibility labels Nov 24, 2015

Miciah reviewed Nov 24, 2015
View reviewed changes

Fix typo as per review comments from @Miciah

f174dce

liggitt mentioned this pull request Nov 25, 2015

Require tls termination in route tls configuration #6076

Merged

0xmichalis mentioned this pull request Nov 25, 2015

status: Report path-based passthrough terminated routes #6092

Merged

openshift-bot pushed a commit that referenced this pull request Jan 20, 2016

Merge pull request #6037 from ramr/prohibit-passthrough-route-with-path

a0f9bba

Merged by openshift-bot

openshift-bot merged commit a0f9bba into openshift:master Jan 20, 2016

ramr deleted the prohibit-passthrough-route-with-path branch March 15, 2016 18:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prohibit passthrough route with path #6037

Prohibit passthrough route with path #6037

ramr commented Nov 24, 2015

liggitt Nov 24, 2015

liggitt Nov 24, 2015

ramr Nov 24, 2015

liggitt Nov 24, 2015

ramr Nov 24, 2015

liggitt Nov 24, 2015

ramr Nov 24, 2015

liggitt Nov 24, 2015

Miciah Nov 24, 2015

ramr commented Nov 24, 2015

ramr commented Dec 3, 2015

0xmichalis commented Jan 12, 2016

deads2k commented Jan 12, 2016

0xmichalis commented Jan 12, 2016

jwforres commented Jan 12, 2016

0xmichalis commented Jan 12, 2016

0xmichalis commented Jan 20, 2016

openshift-bot commented Jan 20, 2016

openshift-bot commented Jan 20, 2016

openshift-bot commented Jan 20, 2016

openshift-bot commented Jan 20, 2016

0xmichalis commented Jan 20, 2016

openshift-bot commented Jan 20, 2016

Prohibit passthrough route with path #6037

Prohibit passthrough route with path #6037

Conversation

ramr commented Nov 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramr commented Nov 24, 2015

ramr commented Dec 3, 2015

0xmichalis commented Jan 12, 2016

deads2k commented Jan 12, 2016

0xmichalis commented Jan 12, 2016

jwforres commented Jan 12, 2016

0xmichalis commented Jan 12, 2016

0xmichalis commented Jan 20, 2016

openshift-bot commented Jan 20, 2016

openshift-bot commented Jan 20, 2016

openshift-bot commented Jan 20, 2016

openshift-bot commented Jan 20, 2016

0xmichalis commented Jan 20, 2016

openshift-bot commented Jan 20, 2016