Allow alternate schemes in Kestrel requests #34013
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
added
the
JamesNK
added
the
api-ready-for-review
|
Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:
|
Tratcher
added
api-approved
blowdart
approved these changes
Jul 6, 2021
davidfowl
reviewed
Jul 6, 2021
| var str = CoreStrings.FormatHttp3StreamErrorSchemeMismatch(RequestHeaders[HeaderNames.Scheme], Scheme); | ||
| Abort(new ConnectionAbortedException(str), Http3ErrorCode.ProtocolError); | ||
| return false; | ||
| if (!ServerOptions.AllowAlternateSchemes || !Uri.CheckSchemeName(headerScheme)) |
There was a problem hiding this comment.
CheckSchemeName is a new check, perf?
There was a problem hiding this comment.
Note this new check only happens after the standard match checks have failed and you've opted into AllowAlternateSchemes, there should be no impact on the normal scenario.
I tried using Http2ConnectionHeadersBenchmark to measure this but the results between runs are wildly inconsistent.
Http2ConnectionHeadersBenchmark.MakeRequest:
AllowAlternateSchemes = false (default)
| Method | HeadersCount | HeadersChange | Mean | Error | StdDev | Median | Op/s | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|---|---|
| MakeRequest | 1 | False | 47.663 us | 0.9468 us | 2.3226 us | 47.941 us | 20,980.6 | - | - | - | 416 B |
| MakeRequest | 1 | True | 10.594 us | 0.6478 us | 1.7950 us | 10.339 us | 94,390.8 | - | - | - | 416 B |
| MakeRequest | 4 | False | 8.633 us | 0.1712 us | 0.1601 us | 8.620 us | 115,840.4 | - | - | - | 416 B |
| MakeRequest | 4 | True | 8.509 us | 0.1538 us | 0.2206 us | 8.510 us | 117,517.1 | - | - | - | 416 B |
| MakeRequest | 32 | False | 12.754 us | 0.3164 us | 0.9179 us | 12.351 us | 78,404.7 | - | - | - | 416 B |
| MakeRequest | 32 | True | 14.856 us | 0.2850 us | 0.2526 us | 14.779 us | 67,312.7 | - | - | - | 416 B |
2nd run
| Method | HeadersCount | HeadersChange | Mean | Error | StdDev | Median | Op/s | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|---|---|
| MakeRequest | 1 | False | 36.428 us | 2.2939 us | 6.7635 us | 35.743 us | 27,451.4 | - | - | - | 426 B |
| MakeRequest | 1 | True | 37.094 us | 2.4211 us | 7.1387 us | 35.420 us | 26,958.4 | - | - | - | 424 B |
| MakeRequest | 4 | False | 32.664 us | 3.2156 us | 9.3291 us | 35.267 us | 30,614.4 | - | - | - | 416 B |
| MakeRequest | 4 | True | 8.444 us | 0.1659 us | 0.2679 us | 8.353 us | 118,424.5 | - | - | - | 416 B |
| MakeRequest | 32 | False | 11.609 us | 0.2135 us | 0.1893 us | 11.601 us | 86,139.0 | - | - | - | 416 B |
| MakeRequest | 32 | True | 14.142 us | 0.2702 us | 0.2528 us | 14.023 us | 70,712.6 | - | - | - | 416 B |
AllowAlternateSchemes = false and scheme == "https" (same as the gRPC TLS termination scenario)
| Method | HeadersCount | HeadersChange | Mean | Error | StdDev | Op/s | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|---|
| MakeRequest | 1 | False | 36.66 us | 2.118 us | 6.246 us | 27,276.3 | - | - | - | 423 B |
| MakeRequest | 1 | True | 34.40 us | 2.340 us | 6.900 us | 29,070.2 | - | - | - | 425 B |
| MakeRequest | 4 | False | 34.19 us | 2.033 us | 5.995 us | 29,249.5 | - | - | - | 423 B |
| MakeRequest | 4 | True | 38.31 us | 1.672 us | 4.929 us | 26,106.1 | - | - | - | 422 B |
| MakeRequest | 32 | False | 12.15 us | 0.243 us | 0.532 us | 82,297.0 | - | - | - | 416 B |
| MakeRequest | 32 | True | 14.02 us | 0.267 us | 0.307 us | 71,317.8 | - | - | - | 416 B |
2nd run
| Method | HeadersCount | HeadersChange | Mean | Error | StdDev | Op/s | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|---|
| MakeRequest | 1 | False | 7.744 us | 0.1548 us | 0.1372 us | 129,124.5 | - | - | - | 416 B |
| MakeRequest | 1 | True | 8.157 us | 0.1626 us | 0.3133 us | 122,594.2 | - | - | - | 416 B |
| MakeRequest | 4 | False | 37.638 us | 1.3081 us | 3.8364 us | 26,568.7 | - | - | - | 427 B |
| MakeRequest | 4 | True | 8.455 us | 0.1690 us | 0.3451 us | 118,279.9 | - | - | - | 416 B |
| MakeRequest | 32 | False | 11.770 us | 0.2220 us | 0.1968 us | 84,961.1 | - | - | - | 416 B |
| MakeRequest | 32 | True | 14.149 us | 0.2154 us | 0.1909 us | 70,677.3 | - | - | - | 416 B |
davidfowl
reviewed
Jul 6, 2021
amcasey
added
area-networking
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #30532. Multiple HTTP/2 proxies for gRPC have been taking advantage of the
:schemeheader to forward the client's original scheme instead of usingx-forwarded-protoheaders. This change allows the developer to opt into mismatched schemes. Note the scheme must still be a valid format.