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

HTTP2: Consider supporting h2c without AppContext switch #988

Closed
JamesNK opened this issue Oct 7, 2019 · 16 comments
Closed

HTTP2: Consider supporting h2c without AppContext switch #988

JamesNK opened this issue Oct 7, 2019 · 16 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Net.Http
Milestone

Comments

@JamesNK
Copy link
Member

JamesNK commented Oct 7, 2019

API proposal: https://github.com/dotnet/corefx/issues/41621#issuecomment-564280539

Today users must set an AppContext switch to be able to use h2c (HTTP/2 without TLS) with HttpClient. This switch was added in .NET Core 3.0 to make it easier to debug the content of HttpClient traffic by having it sent without TLS.

AppContext.SetSwitch("System.Net.Http.SocketsHttpHandler.Http2UnencryptedSupport", true);

This setting has turned out to be very useful, and has a lot of legitimate uses outside of just debugging. I think HttpClient should support h2c without setting this switch.

Although browsers require a web server to always encrypt HTTP/2 content, it is common for back end services to send HTTP/2 API calls without TLS, especially during development. Most development ecosystems don't have the TLS-by-default focus that .NET has.

For example, a gRPC service hosted written in Java/Go/C++ probably won't have a TLS cert setup in development, and .NET devs will need to jump through additional hoops to figure out how to successfully call it.

Another example of where h2c is required is ASP.NET Core gRPC on macOS. .NET Core ASP.NET on Mac doesn't support server TLS, so users have to develop with h2c - https://docs.microsoft.com/en-us/aspnet/core/grpc/troubleshoot?view=aspnetcore-3.0#unable-to-start-aspnet-core-grpc-app-on-macos

@shirhatti

@JamesNK JamesNK changed the title Consider supporting h2c without AppContext switch HTTP2: Consider supporting h2c without AppContext switch Oct 7, 2019
@geoffkizer
Copy link
Contributor

Do you want "real" h2c support, as defined in the RFC? Or do you just want a way to turn on Http2UnencryptedSupport without AppContext?

Real h2c support, as defined in the RFC, is a bit of a pain to support: https://http2.github.io/http2-spec/#discover-http

If all we really care about is enabling dev scenarios, then we could just add Http2UnencryptedSupport as a property on HttpClientHandler. We'd need to name this something explicit though.

@shirhatti
Copy link
Contributor

Just support for prior-knowledge unencrypted HTTP/2 which is what the context switch does today. https://http2.github.io/http2-spec/#rfc.section.3.4

@JamesNK
Copy link
Member Author

JamesNK commented Oct 7, 2019

Do you want "real" h2c support, as defined in the RFC? Or do you just want a way to turn on Http2UnencryptedSupport without AppContext?

My perspective is from working with gRPC, where the client will always have prior knowledge of HTTP/2.

@JamesNK
Copy link
Member Author

JamesNK commented Oct 7, 2019

If all we really care about is enabling dev scenarios, then we could just add Http2UnencryptedSupport as a property on HttpClientHandler. We'd need to name this something explicit though.

This would definitely be an improvement. A gRPC channel is usually just created with an address: GrpcChannel.ForAddress("http://localhost:5001"). When we create the HttpClient for the channel we can look at the address and set HttpClientHandler.Http2UnencryptedSupport to true for a http address.

People who pass an existing HttpClient to the channel will still need to think about this, but they probably power users with more complex scenarios. They can discover the setting by looking at documentation.

@geoffkizer
Copy link
Contributor

Just support for prior-knowledge unencrypted HTTP/2 which is what the context switch does today. https://http2.github.io/http2-spec/#rfc.section.3.4

Makes sense. That's easy enough to do. Mostly we just need a good name for the property...

@scalablecory
Copy link
Contributor

This would also help save some TLS overhead for internal microservices.

@stephentoub
Copy link
Member

stephentoub commented Oct 8, 2019

This would also help save some TLS overhead for internal microservices.

If the cost of accessing a [ThreadStatic] is showing up compared to the cost of establishing a TCP connection, we have bigger problems ;)

@RajeshAKumar
Copy link

Another production use case for supporting http/2 without TLS, is the service URL is secure, but TLS is terminated at load balancer and micro services only get the Http traffic.
The app servers hosting micro services have private ip in cloud and are on slim servers, hence the TLS overhead is borne by load balancer instead of loading app servers.

@JamesNK
Copy link
Member Author

JamesNK commented Dec 10, 2019

Proposal:

namespace System.Net.Http
{
    public partial class SocketsHttpHandler
    {
        public bool SupportPriorKnowledgeHttp2 { get; set; }
    }
}

When true: https://http2.github.io/http2-spec/#rfc.section.3.4 (this is what the AppContext switch does)

One question would be what happens with code that sets the AppContext switch today? Does it default this flag automatically, or is the switch completely removed, and apps in 5.0 has to be modified to continue to run correctly. I would prefer it defaults this flag.

@karelz I'd like this in 5.0. h2c is a common use case with gRPC and setting the AppContext switch is a source of user pain. With this SocketsHttpHandler.SupportPriorKnowledgeHttp2 flag the .NET Core gRPC client can detect when the user wants to call a non-TLS address (i.e. http://whatever) and create a SocketsHttpHandler for the user with this flag set for them.

Can you please triage and help get this into API review.

@scalablecory
Copy link
Contributor

scalablecory commented Dec 10, 2019

We need to have a larger discussion re: prior knowledge, version upgrade/downgrade -- h2c, h3, and alt-svc are all affected by this.

H2C (Upgrade header) is different from prior knowledge (send the HTTP2 preface immediately, no HTTP/1 request involved). I think the 1st is important to support, the 2nd would be nice to have if it's trivial to implement but I wouldn't make it a priority.

@JamesNK
Copy link
Member Author

JamesNK commented Dec 10, 2019

Speaking only for myself and my requirements:

gRPC is always HTTP/2, so there is no need to negotiate. gRPC doesn't need the h2c upgrade header, and I don't believe Kestrel supports it // @Tratcher

@Tratcher
Copy link
Member

H2C (Upgrade header) is different from prior knowledge (send the HTTP2 preface immediately, no HTTP/1 request involved). I think the 1st is important to support, the 2nd would be nice to have if it's trivial to implement but I wouldn't make it a priority.

I'm not aware of any implementations that support or require the upgrade header flow. The prior knowledge flow is supported by Kestrel and others, and that's the one gRPC wants.

Proposal: Elsewhere we've discussed having a min and a max http version for requests. Under the following conditions the client could assume prior knowledge:

  • Min and max version are both set to Http/2
  • The request scheme is "http".

@scalablecory
Copy link
Contributor

Another option to chew on... sentinel Version instance.

public static class HttpVersion
{
   public Version Version20Plaintext { get; }
}

...

if(ReferenceEquals(request.Version, HttpVersion.Version20Plaintext))
{
   // prenegotiated h2c.
}

@scalablecory
Copy link
Contributor

Proposal: Elsewhere we've discussed having a min and a max http version for requests.

My ideal API would use the HttpRequestMessage.Version as a starting point and then allow users to ask "upgrade to whatever .NET happens to support". Explicit min/max versions have minimal utility beyond this feature:

public class SocketsHttpHandler
{
   public bool AllowRequestVersionUpgrade { get; set; } = true;
   public bool AllowRequestVersionDowngrade { get; set; } = true;
}

@Tratcher
Copy link
Member

Let's not re-hash the details of min/max version here. I only bring it up in this context as the request version is a strong indicator of prior knowledge.

Another option to chew on... sentinel Version instance.

Please no, sentinels are a very problematic pattern.

@scalablecory scalablecory transferred this issue from dotnet/corefx Dec 17, 2019
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Net.Http untriaged New issue has not been triaged by the area owner labels Dec 17, 2019
@scalablecory scalablecory added this to the 5.0 milestone Dec 17, 2019
@scalablecory scalablecory added api-suggestion Early API idea and discussion, it is NOT ready for implementation and removed untriaged New issue has not been triaged by the area owner labels Dec 17, 2019
@scalablecory
Copy link
Contributor

Closing as duplicate of #987

bpkroth added a commit to bpkroth/MLOS that referenced this issue Sep 26, 2020
…nections

This overrode and made the following line useless:

AppContext.SetSwitch("System.Net.Http.SocketsHttpHandler.Http2UnencryptedSupport", true);

The resulting error was:

Unhandled exception. Grpc.Core.RpcException: Status(StatusCode=Internal, Detail="Bad gRPC response. Response protocol downgraded to HTTP/1.1.")

Further details in:

#4
grpc/grpc-dotnet#654
dotnet/runtime#988
bpkroth added a commit to bpkroth/MLOS that referenced this issue Sep 26, 2020
…nections

This overrode and made the following line useless:

AppContext.SetSwitch("System.Net.Http.SocketsHttpHandler.Http2UnencryptedSupport", true);

The resulting error was:

Unhandled exception. Grpc.Core.RpcException: Status(StatusCode=Internal, Detail="Bad gRPC response. Response protocol downgraded to HTTP/1.1.")

Further details in:

#4
grpc/grpc-dotnet#654
dotnet/runtime#988
bpkroth added a commit to microsoft/MLOS that referenced this issue Sep 28, 2020
* improve and add debugging targets

* add a 'dotnet-build-quick' target for easier access to faster rebuilds

* spelling

* remove an environment variable that was breaking unencrypted grpc connections

This overrode and made the following line useless:

AppContext.SetSwitch("System.Net.Http.SocketsHttpHandler.Http2UnencryptedSupport", true);

The resulting error was:

Unhandled exception. Grpc.Core.RpcException: Status(StatusCode=Internal, Detail="Bad gRPC response. Response protocol downgraded to HTTP/1.1.")

Further details in:

bpkroth#4
grpc/grpc-dotnet#654
dotnet/runtime#988

* make one of the unit tests also invoke the optimizer

* build the mlos.agent.server when building projects that need it for unit tests

This makes it easier to run "make all test" in one project directory.

* need to make sure that the mlos modules is already installed and available
@ghost ghost locked as resolved and limited conversation to collaborators Dec 12, 2020
MichalStrehovsky pushed a commit to MichalStrehovsky/runtime that referenced this issue Jul 1, 2021
* Fixed wrong assert when getting stackTrace with unhandled exception on the stack
fixes dotnet#988

* Skip own frames when getting current StackTrace/Frame

* Changed arguments of RhpSfiNext to pointers

* Changed stackTrace/Frame to have a predictable stack depth,

* Always start stackTrace from the caller of RhGetCurrentThreadStackTrace(IntPtr[])
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Net.Http
Projects
None yet
Development

No branches or pull requests

8 participants