Skip to content
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

Support both HTTP2 and HTTP1 for upstreams #6521

Closed
guydc opened this issue Jun 2, 2022 · 5 comments
Closed

Support both HTTP2 and HTTP1 for upstreams #6521

guydc opened this issue Jun 2, 2022 · 5 comments
Assignees
Labels
Good First Issue Good issue for newbies Type: Enhancement New feature or request
Milestone

Comments

@guydc
Copy link

guydc commented Jun 2, 2022

Version

1.11.x (latest stable)

Is your feature request related to a problem? Please describe.

Gloo supports configuring either HTTP1 or HTTP2 options per upstream. If options for both protocols are provided, the Gloo translation fails.

Describe the solution you'd like

Envoy supports configuring both http_protocol_options and http2_protocol_options on the same cluster, as long as the non-default protocol_selection: USE_DOWNSTREAM_PROTOCOL option is used.

It should be possible to configure both HTTP1 and HTTP2 options for an upstream and define the upstream protocol selection policy that supports this configuration.

Gloo edge validating webhooks should not admit upstreams that configure both options, if appropriate protocol selection is not defined.

Describe alternatives you've considered

No response

Additional Context

No response

@guydc guydc added the Type: Enhancement New feature or request label Jun 2, 2022
@chrisgaun chrisgaun added this to the Q2-2 milestone Jun 2, 2022
@chrisgaun chrisgaun added the Good First Issue Good issue for newbies label Jun 2, 2022
@ianmacclancy
Copy link
Contributor

ianmacclancy commented Jun 17, 2022

Most of this task I understand and am implementing

"Gloo edge validating webhooks should not admit upstreams that configure both options if appropriate protocol selection is not defined."

This line does not make sense to me. It is valid to pass both http1 and http2 on the same upstream and that is what this change is enabling. We currently allow users to pass both settings when useHttp2 is enabled

If this line of the description means you cannot pass both protocol selection enums, then that is doable

@guydc
Copy link
Author

guydc commented Jun 18, 2022

Hi @ianmacclancy. Currently, it's possible to specify both options for upstreams in Gloo Edge. However, this configuration is rejected by envoy (error message: cluster: Both HTTP1 and HTTP2 options may only be configured with non-default 'protocol_selection' values), leading to inconsistent behavior (quietly breaking the configuration from being applied, without any indication on the upstream or proxy status). Gloo Edge should protect from this sort of inconsistency.

Protection can be implemented with a webhook that explicitly forbids using both options without specifying an appropriate protocol selection policy. Alternatively, Gloo Edge can implicitly set protocol_selection: USE_DOWNSTREAM_PROTOCOL if both HTTP1 and HTTP2 options are specified on an upstream. However, this implicit behavior will not be future proof, as envoy also supports an ALPN-based upstream protocol selection in the newer Upstream HTTP Protocol Options Extension (Currently not used by Gloo). So, the policy should probably be provided by the user.

At the very least, the Gloo Edge docs should be clear about this behavior: if both options are required, protocol selection must also be configured.

@ianmacclancy
Copy link
Contributor

For now, we won't make the decision to automatically recognize that multiple connection settings are passed and will reject the resource if both HTTP1 and HTTP2 are passed.

As part of the DOD for this task I am proposing a restructuring of the Upstream CRD to support the current envoy behavior and the deprecation of the cluster config protocol selection - ref: envoyproxy/envoy@7554d61

I agree that a better experience would be abstracting the protocol selection and working out end-user intentions based on what they pass rather than shifting the more complex logic to the end-user.

@ianmacclancy
Copy link
Contributor

ianmacclancy commented Jun 22, 2022

Final PR - this will update gloo ee with the changes - https://github.com/solo-io/solo-projects/pull/3788/files

@nfuden
Copy link
Contributor

nfuden commented Jun 22, 2022

Closing per #6521 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Issue Good issue for newbies Type: Enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants