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

http: allow ignoring unknown upgrade requests #37150

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ggreenway
Copy link
Contributor

Fixes #36305

Add configuration to ignore either all unknown upgrade requests, or specific upgrade requests. This is allowed in https://datatracker.ietf.org/doc/html/rfc7230#section-6.7:

A server MAY ignore a received Upgrade
header field if it wishes to continue using the current protocol on
that connection.

Risk Level: Medium
Testing: TODO
Docs Changes:
Release Notes: TODO

Copy link

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #37150 was opened by ggreenway.

see: more, trace.

Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @abeyad
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #37150 was opened by ggreenway.

see: more, trace.

@ggreenway
Copy link
Contributor Author

Initial questions:

  1. Is this the right way to ignore upgrades?
  2. Is it better to default the global-ignore to true or false?
  3. Do we need to handle h2/h3 specially? My understanding is that the Upgrade header doesn't apply to them at all (https://www.rfc-editor.org/rfc/rfc9113#name-the-upgrade-header-field).

@alyssawilk
Copy link
Contributor

cc @RyanTheOptimist for spec thoughts

Copy link
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with this conceptually but as commented would like @RyanTheOptimist (or @DavidSchinazi but he's less likely to respond) thoughts from a spec perspective
and tests, obviously but it's still a draft so can wait for those =P

// If enabled, Envoy will ignore upgrade requests of this type. It is invalid to set both this option and
// :ref:`enabled <envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.UpgradeConfig.enabled>`
// to true.
google.protobuf.BoolValue ignore = 4;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually follow-up on that, do we have or need a way to ignore for all types?

Copy link
Contributor Author

@ggreenway ggreenway Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by "ignore for all types"? If you mean "ignore all upgrade requests", that's https://github.com/envoyproxy/envoy/pull/37150/files#diff-4b0f0f4bf391293548732ba0dac7ea417d196e9e5bb87ad4041850f453a982f5R802. If that's not what you meant, please clarify the question.

@@ -100,17 +100,24 @@ class FilterChainFactory {
createFilterChain(FilterChainManager& manager, bool only_create_if_configured = false,
const FilterChainOptions& options = EmptyFilterChainOptions{}) const PURE;

enum class UpgradeAction {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think move the comments below up here in case the enum gets used multiple places

@RyanTheOptimist
Copy link
Contributor

Yes, Upgrade is HTTP/1 only. However Extended CONNECT provides similar capabilities for H2 and H3. Do we have a similar configuration method for this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HTTP/1.1 TLS Upgrade (RFC-2817) causes upgrade_failed
4 participants