http: adding configuration for new style upgrades #3685
Conversation
|
@mattklein123 / @ggreenway I think either of you two are logical reviewers but if you're overloaded I can tag someone on my side to take a look, given this is 90% refactor. Just let me know. If the context would be helpful, this is pulled out of |
15927c2 to
e4f0799
Compare
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
e4f0799 to
c681bcd
Compare
|
Sorry for the delay will review tomorrow. |
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
| // | ||
| // 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. |
| // 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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
I don't feel strongly about it if you think the current way is better.
include/envoy/http/filter.h
Outdated
| * @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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
include/envoy/http/filter.h
Outdated
| * FilterChainFactoryCallbacks. | ||
| * @return true if upgrades of this type are allowed and the filter chain has been created. | ||
| */ | ||
| virtual bool createUpgradeFilterChain(absl::string_view upgrade, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
include/envoy/http/filter.h
Outdated
| * @param callbacks supplies the "sink" that is used for actually creating the filter chain. @see | ||
| * FilterChainFactoryCallbacks. | ||
| * @return true if upgrades of this type are allowed and the filter chain has been created. | ||
| * returns false if this upgrade type is not configured, and no filte chain is created. |
There was a problem hiding this comment.
typo "filte".
P.S., I realize I'm being pedantic here and part of the issue is not seeing the full change this is part of, but it's still a little confusing to me what the overall flow is going to be when an upgrade is requested but there is no filter chain. I guess there are two cases, an upgrade uses the default chain (empty upgrade list, but upgrade allowed), and upgrade is fully specified with a dedicated filter chain.
So I guess the error case is an upgrade is called for, but there is no upgrade specified in the config, whether with default or dedicated chain. In that case the request will be failed by HCM?
There was a problem hiding this comment.
Yeah, exactly. If an upgrade is requested and there's no UpgradeConfig, the HCM will reject and sendLocalReply just as it does for upgrades today.
Adding the (hidden) option to configure a HTTP filter chain for any upgrade type.
Also tossing in some minor renames and status code as prep work for #3301
Risk Level: Low (refactors and adding config guarded code)
Testing: Unit tests of new code.
Docs Changes: docs inline with protos.
Release Notes: n/a (will add with PR which uses/unhides new config)