Implement insecureEdgeTermination options for reencrypt and pasthrough routes #11953

JacobTanenbaum · 2016-11-17T14:55:38Z

reencrypt routes work the same as edge routes with Allow, Redirect, and
None

passthrough routes are only allowed Redirect or None

change the name of the haproxy acl's and map files to makes sense

map files
os_edge_http_redirect -> os_route_http_redirect
os_edge_http_expose -> os_route_http_expose

acls

edge_http_expose -> route_http_expose
wildcard_edge_http_expose -> wildcard_route_http_expose

git issue: 11547
bug: 1317159 Link
docs: PR3244

JacobTanenbaum · 2016-11-17T15:05:01Z

@knobunc

knobunc

LGTM. Are there tests we should extend to cover these new cases?

knobunc · 2016-11-17T15:47:50Z

@openshift/networking PTAL

knobunc · 2016-11-17T15:48:24Z

Also, I presume there are docs that need to be updated too?

smarterclayton · 2016-11-20T17:51:08Z

pkg/route/api/validation/validation.go

-	if _, ok := allowedValues[tls.InsecureEdgeTerminationPolicy]; !ok {
-		msg := fmt.Sprintf("invalid value for InsecureEdgeTerminationPolicy option, acceptable values are %s, %s, %s, or empty", routeapi.InsecureEdgeTerminationPolicyNone, routeapi.InsecureEdgeTerminationPolicyAllow, routeapi.InsecureEdgeTerminationPolicyRedirect)
-		return field.Invalid(fldPath, tls.InsecureEdgeTerminationPolicy, msg)
+	if routeapi.TLSTerminationEdge == tls.Termination || routeapi.TLSTerminationReencrypt == tls.Termination {


Style: Make this a switch statement on the termination field

marun · 2016-11-21T18:29:33Z

pkg/route/api/validation/validation.go

-	if _, ok := allowedValues[tls.InsecureEdgeTerminationPolicy]; !ok {
-		msg := fmt.Sprintf("invalid value for InsecureEdgeTerminationPolicy option, acceptable values are %s, %s, %s, or empty", routeapi.InsecureEdgeTerminationPolicyNone, routeapi.InsecureEdgeTerminationPolicyAllow, routeapi.InsecureEdgeTerminationPolicyRedirect)
-		return field.Invalid(fldPath, tls.InsecureEdgeTerminationPolicy, msg)
+	switch tls.Termination {


Consider updating the unit test for this function to ensure against regression.

@marun updated the unit test

marun

Minor nit about one of the test changes.

I see there is integration coverage for edge + insecure allowed in test/integration/router_test.go. Consider ensuring that reencrypt + insecure allowed has equivalent coverage so the haproxy changes get validated.

marun · 2016-11-23T18:57:18Z

pkg/route/api/validation/validation_test.go

@@ -1055,7 +1055,7 @@ func TestValidateTLS(t *testing.T) {
 	}
 }

-func TestValidateTLSInsecureEdgeTerminationPolicy(t *testing.T) {
+func TestValidatePassthroughInsecureEdgeTerminationPolicy(t *testing.T) {


I think your intent is reasonable here, but I think it is confusing to use a test table to define the single route used in each iteration of the test loop. Consider creating the route in the loop instead and getting ride of tests.

knobunc · 2016-11-29T19:59:18Z

And are there docs we need to update?

ramr · 2016-11-29T23:43:28Z

LGTM

knobunc · 2016-11-30T13:25:24Z

@marun are you good with the test case he added?

JacobTanenbaum · 2016-11-30T16:32:46Z

@marun did I implement the kind of testing you where looking for?

marun · 2016-11-30T17:39:52Z

LGTM [test]

routes reencrypt routes work the same as edge routes with Allow, Redirect, and None passthrough routes are only allowed Redirect or none change the name of the haproxy acl's and map files to makes sense map files os_edge_http_redirect -> os_route_http_redirect os_edge_http_expose -> os_route_http_expose acls edge_http_expose -> route_http_expose wildcard_edge_http_expose -> wildcard_route_http_expose

knobunc · 2016-11-30T19:30:14Z

[merge]

knobunc · 2016-11-30T21:17:59Z

[test] epel yum install failed

openshift-bot · 2016-11-30T21:22:08Z

Evaluated for origin test up to 38c0198

openshift-bot · 2016-11-30T22:55:06Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11834/) (Base Commit: 166273b)

openshift-bot · 2016-12-01T09:05:21Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11834/) (Image: devenv-rhel7_5458)

knobunc · 2016-12-01T15:09:13Z

[merge] Last flaked on #11004 (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11848/)

openshift-bot · 2016-12-01T15:13:20Z

Evaluated for origin merge up to 38c0198

JacobTanenbaum mentioned this pull request Nov 17, 2016

adding for docs for insecureEdgeTerminatePolicy in reencrypt and passthrough routes openshift/openshift-docs#3244

Merged

knobunc approved these changes Nov 17, 2016

View reviewed changes

knobunc self-assigned this Nov 17, 2016

knobunc mentioned this pull request Nov 18, 2016

Inital start to be able to redirect from http to https. #8258

Closed

smarterclayton reviewed Nov 20, 2016

View reviewed changes

JacobTanenbaum force-pushed the v2Reencrypt-redirect branch from 9f54841 to c68e954 Compare November 21, 2016 15:29

marun reviewed Nov 21, 2016

View reviewed changes

marun reviewed Nov 23, 2016

View reviewed changes

JacobTanenbaum force-pushed the v2Reencrypt-redirect branch from 5d02f39 to 38c0198 Compare November 30, 2016 19:28

openshift-bot merged commit 5d5c1e9 into openshift:master Dec 1, 2016

knobunc changed the title ~~Implement inscureEdgeTermination options for reencrypt and pasthrough routes~~ Implement insecureEdgeTermination options for reencrypt and pasthrough routes Dec 16, 2016

wshearn mentioned this pull request Apr 10, 2017

logging routes insecure edge termination policy defaults empty openshift/openshift-ansible#3802

Closed

knobunc mentioned this pull request Jun 26, 2017

Combination of http and passthrough routes bound to the same host confuses ha-proxy #5946

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement insecureEdgeTermination options for reencrypt and pasthrough routes #11953

Implement insecureEdgeTermination options for reencrypt and pasthrough routes #11953

JacobTanenbaum commented Nov 17, 2016 •

edited

Loading

JacobTanenbaum commented Nov 17, 2016

knobunc left a comment

knobunc commented Nov 17, 2016

knobunc commented Nov 17, 2016

smarterclayton Nov 20, 2016

marun Nov 21, 2016

JacobTanenbaum Nov 22, 2016

marun left a comment

marun Nov 23, 2016

knobunc commented Nov 29, 2016

ramr commented Nov 29, 2016

knobunc commented Nov 30, 2016

JacobTanenbaum commented Nov 30, 2016

marun commented Nov 30, 2016

knobunc commented Nov 30, 2016

knobunc commented Nov 30, 2016

openshift-bot commented Nov 30, 2016

openshift-bot commented Nov 30, 2016

openshift-bot commented Dec 1, 2016 •

edited

Loading

knobunc commented Dec 1, 2016 •

edited

Loading

openshift-bot commented Dec 1, 2016

Implement insecureEdgeTermination options for reencrypt and pasthrough routes #11953

Implement insecureEdgeTermination options for reencrypt and pasthrough routes #11953

Conversation

JacobTanenbaum commented Nov 17, 2016 • edited Loading

JacobTanenbaum commented Nov 17, 2016

knobunc left a comment

Choose a reason for hiding this comment

knobunc commented Nov 17, 2016

knobunc commented Nov 17, 2016

smarterclayton Nov 20, 2016

Choose a reason for hiding this comment

marun Nov 21, 2016

Choose a reason for hiding this comment

JacobTanenbaum Nov 22, 2016

Choose a reason for hiding this comment

marun left a comment

Choose a reason for hiding this comment

marun Nov 23, 2016

Choose a reason for hiding this comment

knobunc commented Nov 29, 2016

ramr commented Nov 29, 2016

knobunc commented Nov 30, 2016

JacobTanenbaum commented Nov 30, 2016

marun commented Nov 30, 2016

knobunc commented Nov 30, 2016

knobunc commented Nov 30, 2016

openshift-bot commented Nov 30, 2016

openshift-bot commented Nov 30, 2016

openshift-bot commented Dec 1, 2016 • edited Loading

knobunc commented Dec 1, 2016 • edited Loading

openshift-bot commented Dec 1, 2016

JacobTanenbaum commented Nov 17, 2016 •

edited

Loading

openshift-bot commented Dec 1, 2016 •

edited

Loading

knobunc commented Dec 1, 2016 •

edited

Loading