Convert TLS connection adapter to connection middleware by jkotalik · Pull Request #11109 · dotnet/aspnetcore

jkotalik · 2019-06-11T20:19:25Z

Handles part of #4623

Pretty much is a direct port of aspnet/KestrelHttpServer#2849 to 3.0. Honestly confused why we didn't merge it before, things overall looked good with it.

Follow up:

Implement SslDuplexPipe : Stream, IDuplexPipe to remove allocations of two pipes and the RawStream.
Any breaking changes to remove all adapters

Drafting to write some follow-up questions to make sure design wise everything looks good.

halter73

This is good except I'd remove the cross-cancellation

jkotalik · 2019-06-12T00:36:36Z

@aspnet/build thoughts?

  Results will be available from https://mc.dot.net/#/user/aspnetcore/pr~2Faspnet~2Faspnetcore/ci/20190611.42.1
  Sent Helix Job 6fb69598-c097-4168-87bc-5020cd2b7ee9
F:\workspace\_work\1\s\.nuget\packages\microsoft.dotnet.helix.sdk\2.0.0-beta.19302.2\tools\Microsoft.DotNet.Helix.Sdk.MonoQueue.targets(51,5): error : ArgumentException: Provided Job List Uri is not accessible [F:\workspace\_work\1\s\eng\helix\helix.proj]
F:\workspace\_work\1\s\.nuget\packages\microsoft.dotnet.helix.sdk\2.0.0-beta.19302.2\tools\Microsoft.DotNet.Helix.Sdk.MonoQueue.targets(51,5): error :    at Microsoft.DotNet.Helix.Client.HelixApi.HandleFailedRequest(RestApiException ex) in /_/src/Microsoft.DotNet.Helix/Client/CSharp/HelixApi.cs:line 29 [F:\workspace\_work\1\s\eng\helix\helix.proj]
F:\workspace\_work\1\s\.nuget\packages\microsoft.dotnet.helix.sdk\2.0.0-beta.19302.2\tools\Microsoft.DotNet.Helix.Sdk.MonoQueue.targets(51,5): error :    at Microsoft.DotNet.Helix.Client.HelixApi.OnFailedRequest(RestApiException ex) in /_/src/Microsoft.DotNet.Helix/Client/CSharp/generated-code/HelixApi.cs:line 116 [F:\workspace\_work\1\s\eng\helix\helix.proj]
F:\workspace\_work\1\s\.nuget\packages\microsoft.dotnet.helix.sdk\2.0.0-beta.19302.2\tools\Microsoft.DotNet.Helix.Sdk.MonoQueue.targets(51,5): error :    at Microsoft.DotNet.Helix.Client.Job.<NewInternalAsync>d__10.MoveNext() in /_/src/Microsoft.DotNet.Helix/Client/CSharp/generated-code/Job.cs:line 273 [F:\workspace\_work\1\s\eng\helix\helix.proj]
F:\workspace\_work\1\s\.nuget\packages\microsoft.dotnet.helix.sdk\2.0.0-beta.19302.2\tools\Microsoft.DotNet.Helix.Sdk.MonoQueue.targets(51,5): error : --- End of stack trace from previous location where exception was thrown --- [F:\workspace\_work\1\s\eng\helix\helix.proj]
F:\workspace\_work\1\s\.nuget\packages\microsoft.dotnet.helix.sdk\2.0.0-beta.19302.2\tools\Microsoft.DotNet.Helix.Sdk.MonoQueue.targets(51,5): error :    at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) [F:\workspace\_work\1\s\eng\helix\helix.proj]
F:\workspace\_work\1\s\.nuget\packages\microsoft.dotnet.helix.sdk\2.0.0-beta.19302.2\tools\Microsoft.DotNet.Helix.Sdk.MonoQueue.targets(51,5): error :    at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) [F:\workspace\_work\1\s\eng\helix\helix.proj]
F:\workspace\_work\1\s\.nuget\packages\microsoft.dotnet.helix.sdk\2.0.0-beta.19302.2\tools\Microsoft.DotNet.Helix.Sdk.MonoQueue.targets(51,5): error :    at Microsoft.DotNet.Helix.Client.Job.<NewAsync>d__9.MoveNext() in /_/src/Microsoft.DotNet.Helix/Client/CSharp/generated-code/Job.cs:line 190 [F:\workspace\_work\1\s\eng\helix\helix.proj]
F:\workspace\_work\1\s\.nuget\packages\microsoft.dotnet.helix.sdk\2.0.0-beta.19302.2\tools\Microsoft.DotNet.Helix.Sdk.MonoQueue.targets(51,5): error : --- End of stack trace from previous location where exception was thrown --- [F:\workspace\_work\1\s\eng\helix\helix.proj]
F:\workspace\_work\1\s\.nuget\packages\microsoft.dotnet.helix.sdk\2.0.0-beta.19302.2\tools\Microsoft.DotNet.Helix.Sdk.MonoQueue.targets(51,5): error :    at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) [F:\workspace\_work\1\s\eng\helix\helix.proj]
F:\workspace\_work\1\s\.nuget\packages\microsoft.dotnet.helix.sdk\2.0.0-beta.19302.2\tools\Microsoft.DotNet.Helix.Sdk.MonoQueue.targets(51,5): error :    at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) [F:\workspace\_work\1\s\eng\helix\helix.proj]
F:\workspace\_work\1\s\.nuget\packages\microsoft.dotnet.helix.sdk\2.0.0-beta.19302.2\tools\Microsoft.DotNet.Helix.Sdk.MonoQueue.targets(51,5): error :    at Microsoft.DotNet.Helix.Client.HelixApi.<RetryAsync>d__69`1.MoveNext() [F:\workspace\_work\1\s\eng\helix\helix.proj]
F:\workspace\_work\1\s\.nuget\packages\microsoft.dotnet.helix.sdk\2.0.0-beta.19302.2\tools\Microsoft.DotNet.Helix.Sdk.MonoQueue.targets(51,5): error : --- End of stack trace from previous location where exception was thrown --- [F:\workspace\_work\1\s\eng\helix\helix.proj]
F:\workspace\_work\1\s\.nuget\packages\microsoft.dotnet.helix.sdk\2.0.0-beta.19302.2\tools\Microsoft.DotNet.Helix.Sdk.MonoQueue.targets(51,5): error :    at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) [F:\workspace\_work\1\s\eng\helix\helix.proj]
F:\workspace\_work\1\s\.nuget\packages\microsoft.dotnet.helix.sdk\2.0.0-beta.19302.2\tools\Microsoft.DotNet.Helix.Sdk.MonoQueue.targets(51,5): error :    at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) [F:\workspace\_work\1\s\eng\helix\helix.proj]
F:\workspace\_work\1\s\.nuget\packages\microsoft.dotnet.helix.sdk\2.0.0-beta.19302.2\tools\Microsoft.DotNet.Helix.Sdk.MonoQueue.targets(51,5): error :    at System.Runtime.CompilerServices.TaskAwaiter.ValidateEnd(Task task) [F:\workspace\_work\1\s\eng\helix\helix.proj]
F:\workspace\_work\1\s\.nuget\packages\microsoft.dotnet.helix.sdk\2.0.0-beta.19302.2\tools\Microsoft.DotNet.Helix.Sdk.MonoQueue.targets(51,5): error :    at Microsoft.DotNet.Helix.Client.JobDefinition.<SendAsync>d__81.MoveNext() in /_/src/Microsoft.DotNet.Helix/JobSender/JobDefinition.cs:line 225 [F:\workspace\_work\1\s\eng\helix\helix.proj]
F:\workspace\_work\1\s\.nuget\packages\microsoft.dotnet.helix.sdk\2.0.0-beta.19302.2\tools\Microsoft.DotNet.Helix.Sdk.MonoQueue.targets(51,5): error : --- End of stack trace from previous location where exception was thrown --- [F:\workspace\_work\1\s\eng\helix\helix.proj]
F:\workspace\_work\1\s\.nuget\packages\microsoft.dotnet.helix.sdk\2.0.0-beta.19302.2\tools\Microsoft.DotNet.Helix.Sdk.MonoQueue.targets(51,5): error :    at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) [F:\workspace\_work\1\s\eng\helix\helix.proj]
F:\workspace\_work\1\s\.nuget\packages\microsoft.dotnet.helix.sdk\2.0.0-beta.19302.2\tools\Microsoft.DotNet.Helix.Sdk.MonoQueue.targets(51,5): error :    at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) [F:\workspace\_work\1\s\eng\helix\helix.proj]
F:\workspace\_work\1\s\.nuget\packages\microsoft.dotnet.helix.sdk\2.0.0-beta.19302.2\tools\Microsoft.DotNet.Helix.Sdk.MonoQueue.targets(51,5): error :    at System.Runtime.CompilerServices.TaskAwaiter.ValidateEnd(Task task) [F:\workspace\_work\1\s\eng\helix\helix.proj]
F:\workspace\_work\1\s\.nuget\packages\microsoft.dotnet.helix.sdk\2.0.0-beta.19302.2\tools\Microsoft.DotNet.Helix.Sdk.MonoQueue.targets(51,5): error :    at Microsoft.DotNet.Helix.Sdk.SendHelixJob.<ExecuteCore>d__65.MoveNext() in /_/src/Microsoft.DotNet.Helix/Sdk/SendHelixJob.cs:line 234 [F:\workspace\_work\1\s\eng\helix\helix.proj]
F:\workspace\_work\1\s\.nuget\packages\microsoft.dotnet.helix.sdk\2.0.0-beta.19302.2\tools\Microsoft.DotNet.Helix.Sdk.MonoQueue.targets(51,5): error : --- End of stack trace from previous location where exception was thrown --- [F:\workspace\_work\1\s\eng\helix\helix.proj]
F:\workspace\_work\1\s\.nuget\packages\microsoft.dotnet.helix.sdk\2.0.0-beta.19302.2\tools\Microsoft.DotNet.Helix.Sdk.MonoQueue.targets(51,5): error :    at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) [F:\workspace\_work\1\s\eng\helix\helix.proj]
F:\workspace\_work\1\s\.nuget\packages\microsoft.dotnet.helix.sdk\2.0.0-beta.19302.2\tools\Microsoft.DotNet.Helix.Sdk.MonoQueue.targets(51,5): error :    at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) [F:\workspace\_work\1\s\eng\helix\helix.proj]
F:\workspace\_work\1\s\.nuget\packages\microsoft.dotnet.helix.sdk\2.0.0-beta.19302.2\tools\Microsoft.DotNet.Helix.Sdk.MonoQueue.targets(51,5): error :    at Microsoft.DotNet.Helix.Sdk.HelixTask.Execute() in /_/src/Microsoft.DotNet.Helix/Sdk/HelixTask.cs:line 45 [F:\workspace\_work\1\s\eng\helix\helix.proj]
F:\workspace\_work\1\s\.nuget\packages\microsoft.dotnet.helix.sdk\2.0.0-beta.19302.2\tools\Microsoft.DotNet.Helix.Sdk.MonoQueue.targets(51,5): error : RestApiException: The response contained an invalid status code 400 Bad Request [F:\workspace\_work\1\s\eng\helix\helix.proj]
F:\workspace\_work\1\s\.nuget\packages\microsoft.dotnet.helix.sdk\2.0.0-beta.19302.2\tools\Microsoft.DotNet.Helix.Sdk.MonoQueue.targets(51,5): error :  [F:\workspace\_work\1\s\eng\helix\helix.proj]
F:\workspace\_work\1\s\.nuget\packages\microsoft.dotnet.helix.sdk\2.0.0-beta.19302.2\tools\Microsoft.DotNet.Helix.Sdk.MonoQueue.targets(51,5): error : Body: {"Message":"Provided Job List Uri is not accessible"} [F:\workspace\_work\1\s\eng\helix\helix.proj]
F:\workspace\_work\1\s\.nuget\packages\microsoft.dotnet.helix.sdk\2.0.0-beta.19302.2\tools\Microsoft.DotNet.Helix.Sdk.MonoQueue.targets(51,5): error :  [F:\workspace\_work\1\s\eng\helix\helix.proj]
F:\workspace\_work\1\s\.nuget\packages\microsoft.dotnet.helix.sdk\2.0.0-beta.19302.2\tools\Microsoft.DotNet.Helix.Sdk.MonoQueue.targets(51,5): error :  [F:\workspace\_work\1\s\eng\helix\helix.proj]

natemcmaster · 2019-06-12T00:45:20Z

Can you open an issue on https://github.com/dotnet/core-eng/? Looks like an outage or error within Helix

The code has changed a bunch

halter73 · 2019-06-19T23:26:37Z

            _options = options;
-            _logger = loggerFactory?.CreateLogger<HttpsConnectionAdapter>() ?? (ILogger)NullLogger.Instance;
+            _logger = loggerFactory?.CreateLogger<HttpsConnectionMiddleware>();
        }


nit: newline

halter73 · 2019-06-19T23:28:17Z

            context.Features.Set<ITlsConnectionFeature>(feature);
            context.Features.Set<ITlsHandshakeFeature>(feature);

+            // TODO: Handle the cases where this can be null


Nit: remove TODO or do it.

I'll tackle them across the code base in a separate PR.

PS: https://github.com/aspnet/AspNetCore/blob/0648edf7c2c481309272697c0e0b60e47696f9f7/src/Servers/Kestrel/Transport.Libuv/src/Internal/Networking/UvStreamHandle.cs#L47

I'm aware there are TODO comments in Kestrel. That one was checked in five years ago when Kestrel was still a test server. I bet you could find a bunch more recent examples than that, but I don't really see the point. I'm glad you don't plan to just leave this there though.

halter73 · 2019-06-19T23:29:04Z

+                minimumSegmentSize: memoryPoolFeature.MemoryPool.GetMinimumSegmentSize()
+            );
+
+            // TODO: eventually make SslDuplexStream : Stream, IDuplexPipe to avoid RawStream allocation and pipe allocations


This TODO on the other hand is fine. This seems like a fair bit of work.

davidfowl · 2019-06-20T01:37:24Z

@aspnet-hello triage https://dev.azure.com/dnceng/public/_build/results?buildId=231397

davidfowl · 2019-06-20T01:38:02Z

/azp run AspNetCore-ci

azure-pipelines · 2019-06-20T01:38:12Z

Azure Pipelines successfully started running 1 pipeline(s).

aspnet-hello · 2019-06-20T01:41:53Z

This comment was made automatically. If there is a problem contact aspnetcore-build@microsoft.com.

I've triaged the above build. I've created/commented on the following issue(s)
https://github.com/aspnet/AspNetCore-Internal/issues/2410
https://github.com/aspnet/AspNetCore-Internal/issues/2623
https://github.com/aspnet/AspNetCore-Internal/issues/2415

jkotalik · 2019-06-20T02:55:11Z


        private void StopProcessingNextRequest()
        {
+            ProtocolSelectionState previousState;


Do we still need these changes?

jkotalik

I approve, but this is my PR 😃

jkotalik · 2019-06-20T03:01:05Z

I'm going to go ahead and merge this. I'm OOF friday so I'd like to be able to revert if something is failing due to this change.

jkotalik commented Jun 11, 2019

View reviewed changes

Comment thread src/Servers/Kestrel/Core/src/Internal/HttpConnection.cs

jkotalik commented Jun 11, 2019

View reviewed changes

Comment thread src/Servers/Kestrel/Core/src/KestrelServer.cs Outdated

jkotalik requested review from davidfowl and halter73 June 11, 2019 20:28

jkotalik added this to the 3.0.0-preview7 milestone Jun 11, 2019

jkotalik added the area-servers label Jun 11, 2019

davidfowl reviewed Jun 11, 2019

View reviewed changes

Comment thread src/Servers/Kestrel/Core/src/Internal/HttpsConnectionMiddleware.cs Outdated

davidfowl reviewed Jun 11, 2019

View reviewed changes

Comment thread src/Servers/Kestrel/Core/src/Internal/HttpsConnectionMiddleware.cs Outdated

davidfowl reviewed Jun 11, 2019

View reviewed changes

Comment thread src/Servers/Kestrel/Core/src/Internal/HttpsConnectionMiddleware.cs Outdated

jkotalik marked this pull request as ready for review June 11, 2019 22:07

jkotalik requested review from Tratcher and analogrelay as code owners June 11, 2019 22:07

halter73 previously approved these changes Jun 11, 2019

View reviewed changes

jkotalik mentioned this pull request Jun 12, 2019

Remove dependency on pipe events in HttpConnection #11132

Merged

halter73 reviewed Jun 13, 2019

View reviewed changes

Comment thread src/Servers/Kestrel/Core/src/Internal/HttpConnection.cs Outdated

davidfowl mentioned this pull request Jun 19, 2019

Kestrel outputs to console an http url for non-http connection handlers #8565

Open

Move the HTTPS connection adapter to connection middleware

90a4b81

davidfowl force-pushed the jkotalik/connectionMiddleware branch from cc695ae to 90a4b81 Compare June 19, 2019 17:33

davidfowl added 4 commits June 20, 2019 01:09

Fix the number of expected middleware

2835913

Fixed comment

ef7555f

Sort usings

5117b18

Add check that got removed in merge

86a17b9

davidfowl requested a review from halter73 June 19, 2019 23:17

halter73 reviewed Jun 19, 2019

View reviewed changes

halter73 approved these changes Jun 19, 2019

View reviewed changes

jkotalik commented Jun 20, 2019

View reviewed changes

jkotalik merged commit 689b052 into master Jun 20, 2019

ghost deleted the jkotalik/connectionMiddleware branch June 20, 2019 03:02

ReubenBond mentioned this pull request Aug 1, 2019

Non-HTTP TLS connection middleware #12809

Open

amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023

Conversation

jkotalik commented Jun 11, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

halter73 left a comment

Choose a reason for hiding this comment

Uh oh!

jkotalik commented Jun 12, 2019

Uh oh!

natemcmaster commented Jun 12, 2019 via email

Uh oh!

Uh oh!

halter73 Jun 19, 2019

Choose a reason for hiding this comment

Uh oh!

halter73 Jun 19, 2019

Choose a reason for hiding this comment

Uh oh!

davidfowl Jun 19, 2019

Choose a reason for hiding this comment

Uh oh!

halter73 Jun 20, 2019

Choose a reason for hiding this comment

Uh oh!

halter73 Jun 19, 2019

Choose a reason for hiding this comment

Uh oh!

davidfowl commented Jun 20, 2019

Uh oh!

davidfowl commented Jun 20, 2019

Uh oh!

azure-pipelines Bot commented Jun 20, 2019

Uh oh!

aspnet-hello commented Jun 20, 2019

Uh oh!

jkotalik Jun 20, 2019

Choose a reason for hiding this comment

Uh oh!

jkotalik left a comment

Choose a reason for hiding this comment

Uh oh!

jkotalik commented Jun 20, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants