-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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 4 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 |
|---|---|---|
|
|
@@ -532,11 +532,22 @@ 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. | ||
| * @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. | ||
| */ | ||
| 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; | ||
| }; | ||
|
|
||
| } // 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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.