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

Kestrel and IIS set different Request.Protocol values for HTTP/2 #14139

Closed
JamesNK opened this issue Sep 19, 2019 · 17 comments · Fixed by #14644
Closed

Kestrel and IIS set different Request.Protocol values for HTTP/2 #14139

JamesNK opened this issue Sep 19, 2019 · 17 comments · Fixed by #14644
Assignees
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions bug This issue describes a behavior which is not expected - a bug. Done This issue has been fixed feature-iis Includes: IIS, ANCM

Comments

@JamesNK
Copy link
Member

JamesNK commented Sep 19, 2019

Kestrel sets HTTP/2 while IIS sets HTTP/2.0.

Is this expected behavior?

@Tratcher Tratcher added bug This issue describes a behavior which is not expected - a bug. feature-iis Includes: IIS, ANCM 3.1-candidate labels Sep 19, 2019
@Tratcher
Copy link
Member

Bug. The official value is HTTP/2. Is this messing with your gPRC logic?

@jkotalik
Copy link
Contributor

@Tratcher is there anything we can do if IIS sets it?

@jkotalik
Copy link
Contributor

I misunderstood the original issue. I see now.

@JamesNK
Copy link
Member Author

JamesNK commented Sep 19, 2019

Is this messing with your gPRC logic?

Kind of. I have a test for protocol, and IIS (specifically IIS express) is failing that test even though it is HTTP/2. It doesn't actually break anyone at the moment because IIS express can't support gRPC, but it gives people an odd error message.

grpc/grpc-dotnet#526 (comment)

Current:

 if (httpContext.Request.Protocol != "HTTP/2") 
 { 
     // Set error response on HttpResponse
     return Task.CompletedTask; 
 } 

I can change it to:

 if (httpContext.Request.Protocol != "HTTP/2" && httpContext.Request.Protocol != "HTTP/2.0") 
 { 
     // Set error response on HttpResponse
     return Task.CompletedTask; 
 } 

IIS will still fail but it will fail for a less odd reason.

@analogrelay analogrelay added this to the 5.0.0-preview1 milestone Sep 24, 2019
@analogrelay analogrelay added good first issue Good for newcomers. help wanted Up for grabs. We would accept a PR to help resolve this issue labels Sep 24, 2019
@analogrelay
Copy link
Contributor

So all we need to do is add a clause to the IISHttpContext.FeatureCollection code @Tratcher linked above to have it explicitly return HTTP/2. Sounds good. Good first issue for a new contributor!

@gfoidl
Copy link
Member

gfoidl commented Sep 25, 2019

@anure I'm going to do this.

@jkotalik
Copy link
Contributor

In @gfoidl we trust.

@analogrelay
Copy link
Contributor

That turnaround was awesome :). Nice work and thanks @gfoidl!

@tiago-quintanilha
Copy link

I've just finished a gRPC service in .NET Core 3.0 and I'd like to host it with IIS inprocess, but I got this 426 error. It was correctualy qualified as a bug, but will only be included in 5.0-preview? Shall I drop my intention to use IIS so?

@analogrelay
Copy link
Contributor

@tiago-quintanilha I don't think your question is related to this issue thread. Can you open a new issue please?

@JamesNK
Copy link
Member Author

JamesNK commented Sep 25, 2019

IIS doesn't support gRPC. It needs improvements to support HTTP/2 trailers and bi-directional streaming.

@tiago-quintanilha
Copy link

@anurse, I posted here because the error was exactly the same, since gRPC always uses http/2, and I noticed that '3.1-candidate' tag was exchanged for '5.0.0-preview1'.

@JamesNK, thanks for your feedback. I've searched for this very information but I could't find. Guess it is all too new. I'll have to use windows service then.

@analogrelay
Copy link
Contributor

I'd like to host it with IIS inprocess, but I got this 426 error.

The initial post was just about being more consistent with how we report HttpRequest.Protocol, with nothing about a client error so I'm still a little confused what is related between your issue and the initial report. As @JamesNK said, IIS doesn't have sufficient HTTP/2 features to support gRPC (we are working on it though), so that's probably the root of the issue you were seeing.

Anyway, I think we got to the bottom of your issue @tiago-quintanilha, but feel free to file a new issue if you need to! Even if your issue is related to this one, once this issue is closed it's better to just file a new one rather than posting on a closed issue (since we don't triage these anymore).

@JamesNK
Copy link
Member Author

JamesNK commented Sep 25, 2019

Re-opening because I think this should be unit tested to prevent regression. gRPC tests this string to verify the request is HTTP/2. It is important it is correct.

@JamesNK JamesNK reopened this Sep 25, 2019
@analogrelay analogrelay removed good first issue Good for newcomers. help wanted Up for grabs. We would accept a PR to help resolve this issue labels Sep 26, 2019
@JamesNK
Copy link
Member Author

JamesNK commented Sep 30, 2019

For testing, I think the most useful verification would be a couple of integration/functional tests that check Kestrel and IIS product the same Request.Protocol values in HTTP/2.

@jkotalik
Copy link
Contributor

I can do this. It's fairly non-trivial to verify Http/2+IIS.

@jkotalik jkotalik self-assigned this Sep 30, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Dec 2, 2019
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions bug This issue describes a behavior which is not expected - a bug. Done This issue has been fixed feature-iis Includes: IIS, ANCM
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants