ensure remote server HTTP2 tests are actually using HTTP2 #39343

geoffkizer · 2019-07-09T22:28:52Z

Fixes #39178
Contributes to #34460

Update many remote server tests to take a RemoteServer argument (instead of just server uri) and use CreateHttpClientForRemoteServer to set the HttpClient's DefaultRequestVersion property based on the definition of the RemoteServer.

Also, validate during request processing that we are sending the correct request version and receiving the expected response version.

Also, update some remote server tests to run against the HTTP2 remote server as well.

geoffkizer · 2019-07-09T22:33:01Z

/azp run corefx-outerloop-windows

azure-pipelines · 2019-07-09T22:33:10Z

Azure Pipelines successfully started running 1 pipeline(s).

geoffkizer · 2019-07-09T22:33:19Z

/azp run corefx-outerloop-linux

azure-pipelines · 2019-07-09T22:33:28Z

Azure Pipelines successfully started running 1 pipeline(s).

davidsh · 2019-07-09T23:00:04Z

src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.ServerCertificates.cs

+                        yield return new object[] {
+                            remoteServer,
+                            remoteServer.RedirectUriForDestinationUri(
+                                statusCode:302,


nit: add space after ":" for named parameters.

(comment applies to other places in the PR)

davidsh · 2019-07-10T00:12:17Z

Windows 7 CI runs are failing with the HTTP/2.0 check. That's probably because Windows 7 doesn't support ALPN.

geoffkizer · 2019-07-10T00:13:32Z

Yeah, makes sense. Will fix. Thanks.

geoffkizer · 2019-07-10T00:20:13Z

/azp run corefx-outerloop-windows

azure-pipelines · 2019-07-10T00:20:21Z

Azure Pipelines successfully started running 1 pipeline(s).

geoffkizer · 2019-07-10T00:20:23Z

/azp run corefx-outerloop-linux

azure-pipelines · 2019-07-10T00:20:31Z

Azure Pipelines successfully started running 1 pipeline(s).

geoffkizer · 2019-07-10T19:37:43Z

/azp run corefx-outerloop-windows

geoffkizer · 2019-07-10T19:37:51Z

/azp run corefx-outerloop-linux

azure-pipelines · 2019-07-10T19:37:52Z

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines · 2019-07-10T19:37:59Z

Azure Pipelines successfully started running 1 pipeline(s).

geoffkizer · 2019-07-10T19:38:23Z

@stephentoub Can you review?

stephentoub · 2019-07-10T19:45:38Z

src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.cs

        {
-            using (HttpClient client = CreateHttpClient())
-            using (HttpResponseMessage response = await client.GetAsync(remoteServer))
+            using (HttpClient client = CreateHttpClientForRemoteServer(remoteServer))


This HttpClientHandlerTest class has derivations for both 1.1:

corefx/src/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs

Line 944 in 290a6e5

public sealed class SocketsHttpHandler_HttpClientHandlerTest : HttpClientHandlerTest

and 2.0:

corefx/src/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs

Line 2232 in 290a6e5

public sealed class SocketsHttpHandlerTest_HttpClientHandlerTest_Http2 : HttpClientHandlerTest

In conjunction with this remote server approach, that then means we're running the same tests more than we should be? e.g. for both the 1.1 and 2.0 derivations, we'll run both 1.1 and 2.0 tests?

Yep. See #39345.

Note also that since a bunch of the tests in HttpClientHandlerTest are 1.1-loopback based (not generic loopback), we're also running these tests twice for 1.1, since in the "2.0" derivation they still run as 1.1.

The "2.0" derivation really means "use the 2.0 loopback server". It would be nice to clarify this, but it's not urgent.

src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTestBase.cs

stephentoub

Other than my two comments/questions, LGTM. Thanks.

geoffkizer · 2019-07-10T20:24:03Z

/azp run corefx-outerloop-windows

azure-pipelines · 2019-07-10T20:24:12Z

Azure Pipelines successfully started running 1 pipeline(s).

geoffkizer · 2019-07-10T20:24:18Z

/azp run corefx-outerloop-linux

azure-pipelines · 2019-07-10T20:24:27Z

Azure Pipelines successfully started running 1 pipeline(s).

geoffkizer · 2019-07-10T22:59:19Z

I'm seeing an assert in the certificate code on Linux. Seems to be happening for both CurlHandler and SocketsHttpHandler. @bartonjs any idea what's going on here?

Process terminated. Assertion failed.
Unrecognized X509VerifyStatusCode:44
   at Internal.Cryptography.Pal.OpenSslX509ChainProcessor.MapVerifyErrorToChainStatus(X509VerifyStatusCode code) in /_/src/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Unix/OpenSslX509ChainProcessor.cs:line 819
   at Internal.Cryptography.Pal.OpenSslX509ChainProcessor.AddElementStatus(X509VerifyStatusCode errorCode, List`1 elementStatus, List`1 overallStatus) in /_/src/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Unix/OpenSslX509ChainProcessor.cs:line 703
   at Internal.Cryptography.Pal.OpenSslX509ChainProcessor.AddElementStatus(ErrorCollection errorCodes, List`1 elementStatus, List`1 overallStatus) in /_/src/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Unix/OpenSslX509ChainProcessor.cs:line 694
   at Internal.Cryptography.Pal.OpenSslX509ChainProcessor.BuildChainElements(WorkingChain workingChain, List`1& overallStatus) in /_/src/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Unix/OpenSslX509ChainProcessor.cs:line 599
   at Internal.Cryptography.Pal.OpenSslX509ChainProcessor.Finish(OidCollection applicationPolicy, OidCollection certificatePolicy) in /_/src/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Unix/OpenSslX509ChainProcessor.cs:line 393
   at Internal.Cryptography.Pal.ChainPal.BuildChain(Boolean useMachineContext, ICertificatePal cert, X509Certificate2Collection extraStore, OidCollection applicationPolicy, OidCollection certificatePolicy, X509RevocationMode revocationMode, X509RevocationFlag revocationFlag, DateTime verificationTime, TimeSpan timeout) in /_/src/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Unix/ChainPal.cs:line 99
   at System.Security.Cryptography.X509Certificates.X509Chain.Build(X509Certificate2 certificate, Boolean throwOnException) in /_/src/System.Security.Cryptography.X509Certificates/src/System/Security/Cryptography/X509Certificates/X509Chain.cs:line 118
   at System.Security.Cryptography.X509Certificates.X509Chain.Build(X509Certificate2 certificate) in /_/src/System.Security.Cryptography.X509Certificates/src/System/Security/Cryptography/X509Certificates/X509Chain.cs:line 105
   at System.Net.Security.CertificateValidation.BuildChainAndVerifyProperties(X509Chain chain, X509Certificate2 remoteCertificate, Boolean checkCertName, String hostName) in /_/src/Common/src/System/Net/Security/CertificateValidation.Unix.cs:line 18
   at System.Net.CertificateValidationPal.VerifyCertificateProperties(SafeDeleteContext securityContext, X509Chain chain, X509Certificate2 remoteCertificate, Boolean checkCertName, Boolean isServer, String hostName) in /_/src/System.Net.Security/src/System/Net/CertificateValidationPal.Unix.cs:line 22
   at System.Net.Security.SecureChannel.VerifyRemoteCertificate(RemoteCertValidationCallback remoteCertValidationCallback, ProtocolToken& alertToken) in /_/src/System.Net.Security/src/System/Net/Security/SecureChannel.cs:line 1010
   at System.Net.Security.SslStream.CompleteHandshake(ProtocolToken& alertToken) in /_/src/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs:line 754

Unfortuately, it's not obvious which test is causing this. Likely it's an existing test that didn't run over SSL before, but does now because of HTTP2.

azure-pipelines · 2019-07-11T00:38:56Z

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines · 2019-07-11T00:39:03Z

Azure Pipelines successfully started running 1 pipeline(s).

geoffkizer · 2019-07-12T06:47:13Z

Interestingly, we're still hitting the same assert even with the code I added to try to skip cert validation on Linux for the HTTP2 endpoint.

I don't see anything wrong with the code I added, so this would seem to imply that we see this even with HTTP/1.1 over SSL. Not sure what to make of this. There must be a test that wasn't running over SSL before and now is, and it's hitting this assert. But all the tests are running against the same endpoint, so I have no idea why it would cause this assert now.

geoffkizer · 2019-07-13T21:50:51Z

/azp run corefx-outerloop-windows

azure-pipelines · 2019-07-13T21:50:56Z

Pull request contains merge conflicts.

…some remote server tests to run over HTTP2 as well as HTTP/1.1

…Server is used instead

geoffkizer · 2019-07-14T00:26:34Z

/azp run corefx-outerloop-windows

geoffkizer · 2019-07-14T00:26:41Z

/azp run corefx-outerloop-linux

azure-pipelines · 2019-07-14T00:26:42Z

Azure Pipelines successfully started running 1 pipeline(s).

geoffkizer · 2019-07-14T00:26:48Z

/azp run corefx-outerloop-osx

azure-pipelines · 2019-07-14T00:26:49Z

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines · 2019-07-14T00:26:56Z

Azure Pipelines successfully started running 1 pipeline(s).

geoffkizer · 2019-07-15T05:37:40Z

/azp run corefx-outerloop-linux

azure-pipelines · 2019-07-15T05:37:49Z

Azure Pipelines successfully started running 1 pipeline(s).

geoffkizer · 2019-07-16T19:18:50Z

/azp run

azure-pipelines · 2019-07-16T19:19:13Z

Azure Pipelines successfully started running 4 pipeline(s).

geoffkizer · 2019-07-16T22:09:50Z

Failures are unrelated

…ttp2 ensure remote server HTTP2 tests are actually using HTTP2

…stsforhttp2 ensure remote server HTTP2 tests are actually using HTTP2 Commit migrated from dotnet/corefx@6ec67c1

geoffkizer added this to the 3.0 milestone Jul 9, 2019

geoffkizer requested a review from a team July 9, 2019 22:29

geoffkizer added the area-System.Net.Http.SocketsHttpHandler label Jul 9, 2019

davidsh requested a review from stephentoub July 9, 2019 22:54

davidsh reviewed Jul 9, 2019

View reviewed changes

davidsh approved these changes Jul 9, 2019

View reviewed changes

stephentoub reviewed Jul 10, 2019

View reviewed changes

src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTestBase.cs Outdated Show resolved Hide resolved

stephentoub reviewed Jul 10, 2019

View reviewed changes

src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTestBase.cs Show resolved Hide resolved

stephentoub approved these changes Jul 10, 2019

View reviewed changes

Geoff Kizer added 7 commits July 13, 2019 17:17

ensure remote server HTTP2 tests are actually using HTTP2 and modify …

f013baa

…some remote server tests to run over HTTP2 as well as HTTP/1.1

fix NETFX and UWP build issues

b2216d6

disable version check on platforms that don't support ALPN

c11af55

convert a few more tests to also use HTTP2 remote server

1db9d56

revert change to CreateHttpClient, now that CreateHttpClientForRemote…

14f9968

…Server is used instead

disable cert validation for HTTP2 endpoint on Linux

d34a257

revert workarounds for 39372

0aea825

geoffkizer force-pushed the enableremotetestsforhttp2 branch from 4bb8441 to 0aea825 Compare July 14, 2019 00:20

disable two slow tests that time out in CI

3bf0256

geoffkizer merged commit 6ec67c1 into dotnet:master Jul 16, 2019

wtgodbe pushed a commit to dotnet-maestro-bot/corefx that referenced this pull request Jul 16, 2019

Merge pull request dotnet#39343 from geoffkizer/enableremotetestsforh…

549e812

…ttp2 ensure remote server HTTP2 tests are actually using HTTP2

geoffkizer mentioned this pull request Jan 9, 2020

HTTP2: Two remote server tests occasionally timing out in CI dotnet/runtime#1580

Closed

geoffkizer mentioned this pull request Feb 1, 2020

Assertion in certificate validation code when talking to HTTP2 remote server on Linux dotnet/runtime#30215

Closed

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/corefx#39343 from geoffkizer/enableremotete…

074a4f6

…stsforhttp2 ensure remote server HTTP2 tests are actually using HTTP2 Commit migrated from dotnet/corefx@6ec67c1

ensure remote server HTTP2 tests are actually using HTTP2 #39343

ensure remote server HTTP2 tests are actually using HTTP2 #39343

Uh oh!

Conversation

geoffkizer commented Jul 9, 2019

Uh oh!

geoffkizer commented Jul 9, 2019

Uh oh!

azure-pipelines bot commented Jul 9, 2019

Uh oh!

geoffkizer commented Jul 9, 2019

Uh oh!

azure-pipelines bot commented Jul 9, 2019

Uh oh!

davidsh Jul 9, 2019

Choose a reason for hiding this comment

Uh oh!

davidsh commented Jul 10, 2019

Uh oh!

geoffkizer commented Jul 10, 2019

Uh oh!

geoffkizer commented Jul 10, 2019

Uh oh!

azure-pipelines bot commented Jul 10, 2019

Uh oh!

geoffkizer commented Jul 10, 2019

Uh oh!

azure-pipelines bot commented Jul 10, 2019

Uh oh!

geoffkizer commented Jul 10, 2019

Uh oh!

geoffkizer commented Jul 10, 2019

Uh oh!

azure-pipelines bot commented Jul 10, 2019

Uh oh!

azure-pipelines bot commented Jul 10, 2019

Uh oh!

geoffkizer commented Jul 10, 2019

Uh oh!

stephentoub Jul 10, 2019

Choose a reason for hiding this comment

Uh oh!

geoffkizer Jul 10, 2019

Choose a reason for hiding this comment

Uh oh!

geoffkizer Jul 10, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

stephentoub left a comment

Choose a reason for hiding this comment

Uh oh!

geoffkizer commented Jul 10, 2019

Uh oh!

azure-pipelines bot commented Jul 10, 2019

Uh oh!

geoffkizer commented Jul 10, 2019

Uh oh!

azure-pipelines bot commented Jul 10, 2019

Uh oh!

geoffkizer commented Jul 10, 2019

Uh oh!

azure-pipelines bot commented Jul 11, 2019

Uh oh!

azure-pipelines bot commented Jul 11, 2019

Uh oh!

geoffkizer commented Jul 12, 2019

Uh oh!

geoffkizer commented Jul 13, 2019

Uh oh!

azure-pipelines bot commented Jul 13, 2019

Uh oh!

geoffkizer commented Jul 14, 2019

Uh oh!

geoffkizer commented Jul 14, 2019

Uh oh!

azure-pipelines bot commented Jul 14, 2019

Uh oh!

geoffkizer commented Jul 14, 2019