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

[Release/6.0] Backport test fixes #68332

Merged
merged 5 commits into from
May 3, 2022

Conversation

rzikm
Copy link
Member

@rzikm rzikm commented Apr 21, 2022

Backports #67935, #68083, #65322, #65105, #65120

Fixes #67946

Description

Backport several test-only changes to fix frequent test failures on CI pipelines.
It was caused (intentionally) by Helix update (https://github.com/dotnet/core-eng/issues/15858)-- which re-enabled older TLS versions on all OS versions (so that we can get better test coverage - once we got exception from security folks). However, Win7 and Server 2022 currently don't support the full range of all TLS versions and we need to react accordingly in what the tests expect on these platforms.
Note: There will be likely follow up Helix changes removing further restrictions on Win7 and/or Server 2022. Those will need as well reactions from us when it happens.

Customer Impact

None, Test only change.

Regression

Yes, introduced by recent Helix changes -- see above.

Testing

See CI report for this PR

Risk

Very low, Test only change.

@ghost
Copy link

ghost commented Apr 21, 2022

Tagging subscribers to this area: @dotnet/ncl, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes Issue

main PR

Description

Customer Impact

Regression

Testing

Risk

Author: rzikm
Assignees: rzikm
Labels:

area-System.Net.Security

Milestone: -

@MihaZupan
Copy link
Member

Since we're backporting #65322, can we also include #65105 (same kind of change to avoid throwing on SkipTest)?

@karelz karelz added this to the 6.0.x milestone Apr 21, 2022
@karelz karelz marked this pull request as draft April 21, 2022 16:28
@karelz karelz requested a review from wfurt April 21, 2022 16:37
@rzikm rzikm marked this pull request as ready for review April 22, 2022 12:44
@rzikm rzikm added the Servicing-consider Issue for next servicing release review label Apr 22, 2022
@rzikm rzikm removed the Servicing-consider Issue for next servicing release review label Apr 24, 2022
@rzikm
Copy link
Member Author

rzikm commented Apr 24, 2022

There still seem to be some failures, something more needs to be ported:

    System.Net.Security.Tests.ServerAllowNoEncryptionTest.ServerAllowNoEncryption_ClientNoEncryption_ConnectWithNoEncryption [FAIL]
      System.Security.Authentication.AuthenticationException : Authentication failed, see inner exception.
      ---- System.ComponentModel.Win32Exception : The client and server cannot communicate, because they do not possess a common algorithm.
      Stack Trace:
        /_/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs(364,0): at System.Net.Security.SslStream.ForceAuthenticationAsync[TIOAdapter](TIOAdapter adapter, Boolean receiveFirst, Byte[] reAuthenticationData, Boolean isApm)
        /_/src/libraries/System.Net.Security/tests/FunctionalTests/ServerAllowNoEncryptionTest.cs(86,0): at System.Net.Security.Tests.ServerAllowNoEncryptionTest.ServerAllowNoEncryption_ClientNoEncryption_ConnectWithNoEncryption()
        --- End of stack trace from previous location ---
        ----- Inner Stack Trace -----
System.Net.Security.Tests.ServerNoEncryptionTest.ServerNoEncryption_ClientAllowNoEncryption_ConnectWithNoEncryption [FAIL]
      System.Security.Authentication.AuthenticationException : Authentication failed, see inner exception.
      ---- System.ComponentModel.Win32Exception : The function requested is not supported
      Stack Trace:
        /_/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs(418,0): at System.Net.Security.SslStream.ForceAuthenticationAsync[TIOAdapter](TIOAdapter adapter, Boolean receiveFirst, Byte[] reAuthenticationData, Boolean isApm)
        /_/src/libraries/System.Net.Security/tests/FunctionalTests/ServerNoEncryptionTest.cs(62,0): at System.Net.Security.Tests.ServerNoEncryptionTest.ServerNoEncryption_ClientAllowNoEncryption_ConnectWithNoEncryption()
        --- End of stack trace from previous location ---
        ----- Inner Stack Trace -----

Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

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

LGTM.
thanks for chasing the changes @rzikm

@rzikm
Copy link
Member Author

rzikm commented Apr 26, 2022

@karelz Looks like release/6.0 is using different queues than main, the failing tests are on the Windows.10.Amd64.Server20H2.Open queue, which has been updated on main in #64827. Should we port the queue update as well? Alternatively, we can disable those tests on 20H2 in this PR.

@wfurt
Copy link
Member

wfurt commented Apr 26, 2022

cc: @danmoseley. I'm not sure what is our vision for platform coverage and if there are good reasons why 6.0 tests different OSes than main. But it makes test stability more challenging.

@danmoseley
Copy link
Member

I'm not aware of any reason we would want to have different coverage on 6.0 vs main, unless it's adding/removing OS support.

@agocke thoughts on concern above?

@agocke
Copy link
Member

agocke commented Apr 27, 2022

Keeping the same test coverage as main makes sense to me, aside from differences in platform support (e.g., there was no support for ARM64 in 3.1)

@rzikm
Copy link
Member Author

rzikm commented Apr 27, 2022

looks like the queue update did not help (same errors as before), so there must be other issues

I found #65120, which changes how we detect support for NULL encryption which may fix the tests

* update SSL tests to deal better with disabled protocols

* Improve detection of Null encryption on Windows

* update expectation for Mismatched protocols

* update detection

* wrap win32 exception

* update ProtocolMismatchData sets

* remove debug print

* final cleanup

* generate mismatch data

* avoid SslProtocols.Default
@rzikm rzikm force-pushed the 67946-backport-test-fixes branch from 0471c3c to 032eaac Compare April 27, 2022 11:51
@danmoseley
Copy link
Member

BTW @rzikm, @wfurt may have mentioned, but there is no central approval required to backport necessary infra/test fixes. When things are green you simply have to make sure the branch is unlocked -- @carlossanlop owns this currently.

@rzikm
Copy link
Member Author

rzikm commented Apr 28, 2022

/azp run runtime

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rzikm
Copy link
Member Author

rzikm commented Apr 28, 2022

Test failures are unrelated (see mentions above this comment).

@carlossanlop can you merge this?

@carlossanlop
Copy link
Member

Branding was done today, so I can merge now.

@carlossanlop carlossanlop merged commit 59bbb1f into dotnet:release/6.0 May 3, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jun 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants