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

Microsoft.Quic.MsQuicException on rolling build of runtime #69792

Closed
mkhamoyan opened this issue May 25, 2022 · 21 comments
Closed

Microsoft.Quic.MsQuicException on rolling build of runtime #69792

mkhamoyan opened this issue May 25, 2022 · 21 comments
Assignees
Milestone

Comments

@mkhamoyan
Copy link
Contributor

Description

Last build of runtime is failing with below exception on System.Net.Quic.Tests.MsQuicTests

System.AggregateException : One or more errors occurred. (Connection has been shutdown by transport: QUIC_STATUS_CONNECTION_REFUSED) ---- Microsoft.Quic.MsQuicException : Connection has been shutdown by transport: QUIC_STATUS_CONNECTION_REFUSED

Reproduction Steps

Check last rolling build of runtime

Expected behavior

System.Net.Quic.Tests.MsQuicTests should pass

Actual behavior

System.Net.Quic.Tests.MsQuicTests are failing

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

@ghost ghost added the untriaged New issue has not been triaged by the area owner label May 25, 2022
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Net.Quic and removed untriaged New issue has not been triaged by the area owner labels May 25, 2022
@ghost
Copy link

ghost commented May 25, 2022

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

Issue Details

Description

Last build of runtime is failing with below exception on System.Net.Quic.Tests.MsQuicTests

System.AggregateException : One or more errors occurred. (Connection has been shutdown by transport: QUIC_STATUS_CONNECTION_REFUSED) ---- Microsoft.Quic.MsQuicException : Connection has been shutdown by transport: QUIC_STATUS_CONNECTION_REFUSED

Reproduction Steps

Check last rolling build of runtime

Expected behavior

System.Net.Quic.Tests.MsQuicTests should pass

Actual behavior

System.Net.Quic.Tests.MsQuicTests are failing

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

Author: mkhamoyan
Assignees: -
Labels:

area-System.Net.Quic

Milestone: -

@mkhamoyan
Copy link
Contributor Author

@rzikm from commit log most probably it is related with #69603 , could you please take a look?

@rzikm
Copy link
Member

rzikm commented May 25, 2022

I am looking into it, hopefully will be fixed with #69789

@rzikm rzikm self-assigned this May 25, 2022
@rzikm
Copy link
Member

rzikm commented May 25, 2022

No such luck, but the test failures seem to be confined to Windows 2022 Server, so I will disable the tests there meanwhile

@ManickaP
Copy link
Member

Has the dotnet/msquic change been propagated via maestro already? When we update that repo, new version of windows version of libmsquic gets published into maestro subscription, which then creates PR in runtime in versions.xml, which then updates the msquic version.

Yeah, it wasn't consumed yet: #69709

@rzikm
Copy link
Member

rzikm commented May 25, 2022

I find it a bit weird that the tests didn't fail on #69603 then. But we can wait a bit and see if the maestro PR resolves that.

@ManickaP
Copy link
Member

Ha, Tomas is faster, yeah this was a trap 😄

@thhous-msft
Copy link

WS 2022 will not support client certificates using schannel. It was a feature added for Windows 11. The only way to get client certificate support on WS 2022 or earlier is to use OpenSSL.

@rzikm
Copy link
Member

rzikm commented May 25, 2022

WS 2022 will not support client certificates using schannel. It was a feature added for Windows 11. The only way to get client certificate support on WS 2022 or earlier is to use OpenSSL.

I see, I will update our tests to reflect this

@MattGal
Copy link
Member

MattGal commented May 25, 2022

It actually did: https://github.com/dotnet/runtime/pull/69603/checks?check_run_id=6578113266 and https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-69603-merge-e5a980b51f6847128e/System.Net.Quic.Functional.Tests/1/console.00373325.log?helixlogtype=result

But the test runs was marked as successful: cc @MattGal

Looks like a weird GitHub blip; all the Helix components and even most of the GitHub AzDO components logged "the right thing". If it ever happens again please work with the @dnceng first responders Teams channel to reach out to GitHub; it's unlikely to be investigated while still a one-off.

@ManickaP
Copy link
Member

WS 2022 will not support client certificates using schannel. It was a feature added for Windows 11. The only way to get client certificate support on WS 2022 or earlier is to use OpenSSL.

I see, I will update our tests to reflect this

Should we then rather increase our minimum required Windows version in S.N.Quic? Since without it, the feature doesn't work.
I'm not sure how impactful the feature is and how confusing/debuggable this might be if someone used it on unsupported Win. @wfurt @CarnaViire @rzikm wdyt?

@wfurt
Copy link
Member

wfurt commented May 25, 2022

WS 2022 will not support client certificates using schannel. It was a feature added for Windows 11. The only way to get client certificate support on WS 2022 or earlier is to use OpenSSL.

I see, I will update our tests to reflect this

I'm wondering if we should detect this in the product and throw PlatformNotSupportedException.

@rzikm
Copy link
Member

rzikm commented May 25, 2022

Should we then rather increase our minimum required Windows version in S.N.Quic?

We still need to disable the tests for that OS to unblock CI. Then we can think about following up with some more changes.

I'm wondering if we should detect this in the product and throw PlatformNotSupportedException.

Detecting it sounds reasonable. It would mean OS platform and version check in MsQuicListener, which is doable (although we generally don't have version checks in the product). I would like to investigate this further because we are only supposed to get QUIC_STATUS_CONNECTION_REFUSED when the server is overloaded so it was misleading in this case.

@thhous-msft
Copy link

thhous-msft commented May 25, 2022

Should we then rather increase our minimum required Windows version in S.N.Quic?

This would only be for the specific client certificate feature, and not the entirety of S.N.Quic, right? If the whole S.N.Quic namespace needs higher then WS 2022, then that would mean that no server os Microsoft ships would be supported by S.N.Quic, which would not be ideal.

we are only supposed to get QUIC_STATUS_CONNECTION_REFUSED when the server is overloaded

CONNECTION_REFUSED can happen for a lot more reasons then this. The server could explicitly choose to fail the handshake, which would result in refused as well iirc.

@wfurt
Copy link
Member

wfurt commented May 25, 2022

Yes, I think this check should be only for client certificates ... that is not common IMHO.
Do you know @ThadHouse if the S2022 cannot send the cert or receive it? (or both)
(e.g. would win11 client work agains server 2022?)

@thhous-msft
Copy link

Its server side can't receive it. The functionality to enable requesting client certificates does not work.

#45456

Its the above bug, just exposed from our end.

@rzikm
Copy link
Member

rzikm commented May 25, 2022

CONNECTION_REFUSED can happen for a lot more reasons then this. The server could explicitly choose to fail the handshake, which would result in refused as well iirc.

Yes, that is the second instance I know of where server will refuse the connection, but we don't do that for the failed tests in question.

@jakobbotsch
Copy link
Member

@agocke
Copy link
Member

agocke commented May 26, 2022

Sorry, read the comments but not sure I followed everything -- do we expect this is fixed?

@rzikm
Copy link
Member

rzikm commented May 26, 2022

I hoped #69709 would get in soon (it would fix the test failure), but since there are some test failures on that PR I will merge my other PR (#69843) which will disable the test and will have to follow up on that later.

@ericstj ericstj added the blocking-clean-ci Blocking PR or rolling runs of 'runtime' or 'runtime-extra-platforms' label May 27, 2022
@rzikm rzikm removed the blocking-clean-ci Blocking PR or rolling runs of 'runtime' or 'runtime-extra-platforms' label Jun 7, 2022
@rzikm rzikm closed this as completed Jun 7, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jul 7, 2022
@karelz karelz added this to the 7.0.0 milestone Jul 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

10 participants