-
Notifications
You must be signed in to change notification settings - Fork 115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bug 1826225: Support edge-terminated h2 connections #328
Bug 1826225: Support edge-terminated h2 connections #328
Conversation
* pkg/router/template/plugin_test.go (TestHandleTCPEndpoints): * pkg/router/template/router.go (createServiceUnitInternal): Fix typos in the godoc for these functions.
@Miciah: This pull request references Bugzilla bug 1826225, which is invalid:
Comment In response to this:
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. |
94577cb
to
67867a3
Compare
/bugzilla refresh |
@Miciah: This pull request references Bugzilla bug 1826225, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
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 it would be worthwhile creating a PR that removes the restriction here https://github.com/openshift/origin/blob/master/test/extended/router/grpc-interop.go#L142 and verifying the behaviour. |
/bugzilla refresh The requirements for Bugzilla bugs have changed, recalculating validity. |
@openshift-merge-robot: This pull request references Bugzilla bug 1826225, which is invalid:
Comment In response to this:
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. |
|
/bugzilla refresh |
@Miciah: An error was encountered querying GitHub for users with public email (aiyengar@redhat.com) for bug 1826225 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.
Post "http://ghproxy/graphql": dial tcp 172.30.229.2:80: i/o timeout
Please contact an administrator to resolve this issue, then request a bug refresh with In response to this:
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'm open to suggestions for alternatives to using "h2c" for |
@@ -620,7 +620,8 @@ backend {{ genBackendNamePrefix $cfg.TLSTermination }}:{{ $cfgIdx }} | |||
{{- range $idx, $endpoint := processEndpointsForAlias $cfg $serviceUnit (env "ROUTER_BACKEND_PROCESS_ENDPOINTS" "") }} | |||
server {{ $endpoint.ID }} {{ $endpoint.IP }}:{{ $endpoint.Port }} cookie {{ $endpoint.IdHash }} weight {{ $weight }} | |||
{{- if (eq $cfg.TLSTermination "reencrypt") }} ssl | |||
{{- if not (isTrue $router_disable_http2) }} alpn h2,http/1.1 | |||
{{- if eq $endpoint.AppProtocol "h2c" }} proto h2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this work (or does this work) if HTTP/2 is disabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding proto h2
to the server
stanza affects the protocol used for the connection from HAProxy to the backend and doesn't affect the protocol used for the connection from the client to the frontend, which is governed by the sslbindconf
in the certificate map.
A quick search gave me:
|
It's "established" and codified for use in TLS and HTTP headers by RFC 7540. However, the API godoc for |
67867a3
to
c1b46fb
Compare
@Miciah: This pull request references Bugzilla bug 1826225, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
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. |
Can we extend the tests that we have to show that this would have failed and is now correct? |
Check the appProtocol field on ports in Kubernetes Endpoints objects. If a non-TLS or edge-terminated route is associated with an endpoint with appProtocol: h2c, configure HAProxy to use HTTP/2 for the endpoint. Add checks in the dynamic config manager to force a full reload when a new endpoint specifies appProtocol: h2c or an existing endpoint is modified to change it to or from specifying appProtocol: h2c on a port. Dynamically configuring such an endpoint would require a way to set proto on the HAProxy server, and HAProxy's management interface does not provide any way to do so. This commit fixes bug 1826225. https://bugzilla.redhat.com/show_bug.cgi?id=1826225 * images/router/haproxy/conf/haproxy-config.template: Specify "proto h2" for the server corresponding to an endpoint associated with a non-TLS or edge-terminated route if the endpoint specifies the "h2c" application protocol. * pkg/router/template/configmanager/haproxy/backend.go (UpdateServerInfo): Add appProtocol parameter. If appProtocol is "h2c", return an error. * pkg/router/template/configmanager/haproxy/manager.go (ReplaceRouteEndpoints): Return an error if an endpoint's AppProtocol is changed to or from "h2c". Log AppProtocol and pass it to UpdateServerInfo. * pkg/router/template/plugin.go (createRouterEndpoints): Copy the AppProtocol field from the Kubernetes Endpoints object to the internal Endpoints object. * pkg/router/template/types.go (Endpoint): Add AppProtocol field.
c1b46fb
to
9197ac5
Compare
Latest push drops the logic for reencrypt routes, per recent discussions. I tested the latest version using cluster-bot, and job test e2e openshift/router#328,openshift/origin#26456 succeeded. |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: frobware, Miciah The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
18 similar comments
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
@Miciah: Some pull requests linked via external trackers have merged: The following pull requests linked via external trackers have not merged:
These pull request must merge or be unlinked from the Bugzilla bug in order for it to move to the next state. Once unlinked, request a bug refresh with Bugzilla bug 1826225 has not been moved to the MODIFIED state. In response to this:
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. |
Fix a couple of godoc typos
pkg/router/template/plugin_test.go
(TestHandleTCPEndpoints
):pkg/router/template/router.go
(createServiceUnitInternal
): Fix typos in the godoc for these functions.Use HTTP/2 if endpoint's AppProtocol is "h2" or "h2c"
Check the
appProtocol
field on ports in KubernetesEndpoints
objects. If the route is a reencrypt route withappProtocol
"h2" or a non-TLS or edge-terminated route withappProtocol
"h2c", configure HAProxy to use HTTP/2 for the endpoint.Add checks in the dynamic config manager to force a full reload when a new endpoint specifies
appProtocol: h2
orappProtocol: h2c
or an existing endpoint is modified to change it to or from specifying one of these appProtocol values on a port. Dynamically configuring such an endpoint would require a way to set proto on the HAProxy server, and HAProxy's management interface does not provide any way to do so.images/router/haproxy/conf/haproxy-config.template
: Specifyproto h2
for the server corresponding to an endpoint associated with a reencrypt route if the endpoint specifies the "h2" application protocol, and likewise for an endpoint associated with a non-TLS or edge-terminated route if the endpoint specifies the "h2c" application protocol.pkg/router/template/configmanager/haproxy/backend.go
(UpdateServerInfo
): AddappProtocol
parameter. IfappProtocol
is "h2" or "h2c", return an error.pkg/router/template/configmanager/haproxy/manager.go
(ReplaceRouteEndpoints
): Return an error if an endpoint'sAppProtocol
is changed to or from "h2" or "h2c". LogAppProtocol
and pass it toUpdateServerInfo
.pkg/router/template/plugin.go
(createRouterEndpoints
): Copy theAppProtocol
field from the KubernetesEndpoints
object to the internalEndpoints
object.pkg/router/template/types.go
(Endpoint
): AddAppProtocol
field.