Merge | Remove OS-specific TdsParser files #3469
Conversation
This was lingering from earlier work to change the SSPI abstractions
The managed SNI doesn't use this, so will always return SNI_SUCCESS_IO_PENDING.
The handling of this method is unusual. Only Windows actually waits for the SSL handshake.
Also mop up the unused SNIErrorDetails structure
This exposes a behavioural difference in TdsParser between Unix and Windows: downlevel TLS warnings only appear on Windows.
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
@edwardneal I wanted to resolve the conflicts for you, but there's one with TdsParserStateObject.PostReadAsyncForMars that I'm not sure the right way to resolve. Seems to me we shouldn't be checking for managed SNI in the native state object, but I'm not an expert here. I'll give the overall change a review, but let me know when the conflict is resolved and we can get a CI run going. |
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObjectNative.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObjectNative.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs
Show resolved
Hide resolved
Feed the correct SslProtocols type through the SNI layers
|
Thanks @benrr101 - I've handled the remaining code review feedback. |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3469 +/- ##
==========================================
- Coverage 69.90% 61.82% -8.08%
==========================================
Files 276 268 -8
Lines 62414 62148 -266
==========================================
- Hits 43629 38423 -5206
- Misses 18785 23725 +4940
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Description
This picks up where #3434 left off. The earlier PR handled the simple case of merging OS-specific functionality (which was actually just different between the native and managed SNIs.) This one expands the principle, moving
PostReadAsyncForMarsandWaitForSSLHandShakeToCompletefromTdsParsertoTdsParserStateObject's child classes. This removes TdsParser.Unix.cs and TdsParser.Windows.cs.This has also involved modifying the call site for the method within TdsParser to reference TdsParserStateObject. This is particularly noticeable in EnableSsl.
Finally, while it's not a new gap, this process has highlighted that the existing functionality of warning when down-level SSL is in use only functions on Windows. I've maintained this gap and made it a little more explicit.
Issues
Contributes to #1261 and #2953.
Testing
Testing with SSL- and MARS-enabled connection strings works, I'd appreciate a CI run.