-
Notifications
You must be signed in to change notification settings - Fork 5.5k
http: adding configuration for new style upgrades #3685
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
Changes from 2 commits
c681bcd
fed384e
7a583d9
7e596a3
72a6f63
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,7 +19,7 @@ import "gogoproto/gogo.proto"; | |
| // [#protodoc-title: HTTP connection manager] | ||
| // HTTP connection manager :ref:`configuration overview <config_http_conn_man>`. | ||
|
|
||
| // [#comment:next free field: 23] | ||
| // [#comment:next free field: 24] | ||
| message HttpConnectionManager { | ||
| enum CodecType { | ||
| option (gogoproto.goproto_enum_prefix) = false; | ||
|
|
@@ -270,6 +270,29 @@ message HttpConnectionManager { | |
| // <config_http_conn_man_runtime_represent_ipv4_remote_address_as_ipv4_mapped_ipv6>` for runtime | ||
| // control. | ||
| bool represent_ipv4_remote_address_as_ipv4_mapped_ipv6 = 20; | ||
|
|
||
| // [#not-implemented-hide:] | ||
| // The configuration for HTTP upgrades. | ||
| // For each upgrade type desired, an UpgradeConfig must be added. | ||
| // | ||
| // .. warning:: | ||
| // | ||
| // The current implementation of upgrade headers does not handle | ||
| // multi-valued upgrade headers. Support for multi-valued headers may be | ||
| // added in future if needed. | ||
| message UpgradeConfig { | ||
| // The case-insensitive name of this upgrade, e.g. "websocket". | ||
| // For each upgrade type present in upgrade_configs, requests with | ||
| // Upgrade: [upgrade_type] | ||
| // will be proxied upstream. | ||
| string upgrade_type = 1; | ||
| // If present, this represents the filter chain which will be created for | ||
| // this type of upgrade. If no filters are present, the filter chain for | ||
| // HTTP connections will be used for this upgrade type. | ||
| repeated HttpFilter filters = 5; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be more clear from a config perspective to make this a oneof which can either be a bool (use primary filter chain) or a repeated filter list with in size 1? Just throwing it out there as I think it might be a bit more clear from a config perspective.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, initially I thought that'd add clarity but on second thought I think it's sort of weird, because the one-of boolean could really only have one value of true. Also it's a bit more complicated since I believe oneof can't be repeated so we'd have to have a sub-message for "repeated filter chain". I'm up for doing the change if you still think it's worth it - let me know!
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't feel strongly about it if you think the current way is better. |
||
| }; | ||
| // [#not-implemented-hide:] | ||
| repeated UpgradeConfig upgrade_configs = 23; | ||
| } | ||
|
|
||
| message Rds { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -532,11 +532,27 @@ class FilterChainFactory { | |
| virtual ~FilterChainFactory() {} | ||
|
|
||
| /** | ||
| * Called when a new stream is created on the connection. | ||
| * Called when a new HTTP stream is created on the connection. | ||
| * @param callbacks supplies the "sink" that is used for actually creating the filter chain. @see | ||
| * FilterChainFactoryCallbacks. | ||
| */ | ||
| virtual void createFilterChain(FilterChainFactoryCallbacks& callbacks) PURE; | ||
|
|
||
| /** | ||
| * Called when a new upgrade stream is created on the connection. | ||
| * @param upgrade supplies the upgrade header from downstream | ||
| * @param callbacks supplies the "sink" that is used for actually creating the filter chain. @see | ||
| * FilterChainFactoryCallbacks. | ||
| */ | ||
| virtual bool createUpgradeFilterChain(absl::string_view upgrade, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry to be dense (obviously this is going to be wired up in a forthcoming PR) but what will the behavior be if no upgrade filter chain matches? Do we create the normal one? Would it be worth it to doc that here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So if there's no upgrade filter chain, upgrades are not supported and the HCM will auto-reply with a failure. Per #3599 we no longer need to create a filter chain for early local-only replies, so I opted to not create the filter chain in this case. We could create the default and return false if we think that's better |
||
| FilterChainFactoryCallbacks& callbacks) PURE; | ||
|
|
||
| /** | ||
| * Called to determine if this type of upgrade is supported. | ||
| * @param the upgrade type requested. | ||
| * @return true if upgrades of this type are allowed, false otherwise. | ||
| */ | ||
| virtual bool upgradeSupported(absl::string_view upgrade) const PURE; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where is the called from in prod code? Is the intention that a filter say at runtime that it doesn't support an upgrade? That scares me a little bit just from a config stability perspective, but mainly wondering how this is intended to work. I assume this is going to be used in a follow up? Maybe more docs here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's currently used in the HCM where we decide if we're accepting or rejecting upgrade headers. That said we call createFilterChain before we make that go no-go call, so I could latch the return value of createUpgradeFilterChain, return that from createFilterChain and use that for the reject instead of calling isUpgradeSupported master...alyssawilk:websocket_delay#diff-d70ea2d9706e0246700148b2e2bb63ceR569 (doesn't have error handling yet) |
||
| }; | ||
|
|
||
| } // namespace Http | ||
|
|
||
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.
nit: s/in/in the