Skip to content

Default to HTTP/2 on dotnet5 #590

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Default to HTTP/2 on dotnet5 #590

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 20 additions & 13 deletions src/KubernetesClient/Kubernetes.ConfigInit.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,24 +170,22 @@ private void CreateHttpClient(DelegatingHandler[] handlers, KubernetesClientConf
{
FirstMessageHandler = HttpClientHandler = CreateRootHandler();


#if NET5_0
// https://github.com/kubernetes-client/csharp/issues/533
// net5 only
var sh = new SocketsHttpHandler
{
KeepAlivePingPolicy = HttpKeepAlivePingPolicy.WithActiveRequests,
KeepAlivePingDelay = new TimeSpan(0, 3, 0), // Send pings every three minutes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
KeepAlivePingDelay = new TimeSpan(0, 3, 0), // Send pings every three minutes
KeepAlivePingDelay = TimeSpan.FromMinutes(3), // Send pings every three minutes

KeepAlivePingTimeout = new TimeSpan(0, 0, 30), // Timeout pings after 30s of no response
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
KeepAlivePingTimeout = new TimeSpan(0, 0, 30), // Timeout pings after 30s of no response
KeepAlivePingTimeout = TimeSpan.FromSeconds(30), // Timeout pings after 30s of no response

};
// https://github.com/kubernetes-client/csharp/issues/587
// let user control if tcp keep alive until better fix
if (config.TcpKeepAlive)
{
// https://github.com/kubernetes-client/csharp/issues/533
// net5 only
// this is a temp fix to attach SocketsHttpHandler to HttpClient in order to set SO_KEEPALIVE
// https://tldp.org/HOWTO/html_single/TCP-Keepalive-HOWTO/
//
// _underlyingHandler is not a public accessible field
// src of net5 HttpClientHandler and _underlyingHandler field defined here
// https://github.com/dotnet/runtime/blob/79ae74f5ca5c8a6fe3a48935e85bd7374959c570/src/libraries/System.Net.Http/src/System/Net/Http/HttpClientHandler.cs#L22
//
// Should remove after better solution

var sh = new SocketsHttpHandler();
sh.ConnectCallback = async (context, token) =>
{
var socket = new Socket(SocketType.Stream, ProtocolType.Tcp)
Expand Down Expand Up @@ -215,10 +213,15 @@ private void CreateHttpClient(DelegatingHandler[] handlers, KubernetesClientConf
await socket.ConnectAsync(host, context.DnsEndPoint.Port, token).ConfigureAwait(false);
return new NetworkStream(socket, ownsSocket: true);
};

var p = HttpClientHandler.GetType().GetField("_underlyingHandler", BindingFlags.NonPublic | BindingFlags.Instance);
p.SetValue(HttpClientHandler, (sh));
}

// _underlyingHandler is not a public accessible field
// src of net5 HttpClientHandler and _underlyingHandler field defined here
// https://github.com/dotnet/runtime/blob/79ae74f5ca5c8a6fe3a48935e85bd7374959c570/src/libraries/System.Net.Http/src/System/Net/Http/HttpClientHandler.cs#L22
//
// Should remove after better solution
var p = HttpClientHandler.GetType().GetField("_underlyingHandler", BindingFlags.NonPublic | BindingFlags.Instance);
p.SetValue(HttpClientHandler, (sh));
#endif

if (handlers == null || handlers.Length == 0)
Expand All @@ -243,6 +246,10 @@ private void CreateHttpClient(DelegatingHandler[] handlers, KubernetesClientConf

AppendDelegatingHandler<WatcherDelegatingHandler>();
HttpClient = new HttpClient(FirstMessageHandler, false);

#if NET5_0
HttpClient.DefaultRequestVersion = HttpVersion.Version20;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does NOT work,
see https://docs.microsoft.com/en-us/dotnet/api/system.net.http.httpclient.defaultrequestversion?view=net-5.0

The DefaultRequestVersion property doesn't apply to the SendAsync method. The HttpRequestMessage parameter passed as the argument to the SendAsync method has its own Version property that controls the HTTP version used for the request.

Unfortunately, our generated code use SendAsync LOL

Here is my test (Should find a way to do automation)

  1. create a AKS (1.19)
  2. disable tcp keepalive
  3. start watch example

got a [R] from remote side after 5 min and then watch will not show any event from kubectl run

02:42:58.973029 IP 10.x.x.x.49746 > 52.x.x.x.443: Flags [.], ack 28897, win 492, options [nop,nop,TS val 255369207 ecr 3243887534], length 0
02:47:48.004159 IP 52.x.x.x.443 > 10.x.x.x.49746: Flags [R], seq 660228079, win 0, length 0

#endif
}

/// <summary>
Expand Down