Fix | Linux SPN port number using named instance and Kerberos authentication does not return port# #2240

arellegue · 2023-11-23T04:11:29Z

Fixes Linux SPN to return port number in managed SNI when using named instance and Kerberos authentication. #2187

… domain.

H-Yeo

Just general comments or a follow-up.

src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/InstanceNameTest/InstanceNameTest.cs

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIProxy.cs

…ity.IsUsingManagedSNI.

codecov · 2023-11-23T21:08:01Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7f203dd) 71.23% compared to head (030d8f7) 72.51%.
Report is 9 commits behind head on main.

❗ Current head 030d8f7 differs from pull request most recent head 2c8e139. Consider uploading reports for the commit 2c8e139 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2240      +/-   ##
==========================================
+ Coverage   71.23%   72.51%   +1.28%     
==========================================
  Files         305      309       +4     
  Lines       61854    61960     +106     
==========================================
+ Hits        44061    44931     +870     
+ Misses      17793    17029     -764

Flag	Coverage Δ
addons	`92.88% <ø> (ø)`
netcore	`76.72% <100.00%> (+2.96%)`	⬆️
netfx	`69.90% <ø> (-0.14%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/InstanceNameTest/InstanceNameTest.cs

src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs

Fixed checks for localhost. Use object initializer for array of types declarations.

… per Javad.

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/InstanceNameTest/InstanceNameTest.cs

src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/InstanceNameTest/InstanceNameTest.cs

src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/InstanceNameTest/InstanceNameTest.cs

…ceNameTest class.

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/InstanceNameTest/InstanceNameTest.cs

… on ManagedSNI, we expect the same behavior on native SNI.".

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/InstanceNameTest/InstanceNameTest.cs

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIProxy.cs

DavoudEshtehari · 2023-12-07T00:12:24Z

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/InstanceNameTest/InstanceNameTest.cs

+
+ SqlConnectionStringBuilder builder = new(connStr);
+
+ Assert.True(DataTestUtility.ParseDataSource(builder.DataSource, out string hostname, out _, out string instanceName));


With specifying instance name and port number, this method call always returns false!

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/InstanceNameTest/InstanceNameTest.cs

arellegue added 6 commits November 16, 2023 10:36

Push unit test to continue development in VM.

e8b6a5f

Added unit test for SPN in managed SNI and tested in VMs in sqldrv.ad…

ce2481d

… domain.

Save to my branch all changes

60f061f

Implemented Unit Test using Reflections.

e77bfb6

Change TimeourTimer from 100000 to just 30 seconds.

ed15e37

Removed and sort usings.

d671acd

arellegue requested review from David-Engel, DavoudEshtehari and JRahnama November 23, 2023 04:12

Fix backwards DataTestUtility.IsNotLocalhost logic.

d62c9e1

arellegue changed the title ~~Fix | Fix Linux SPN port number in managed SNI, using named instance and Kerberos authentication~~ Fix | Linux SPN port number using named instance and Kerberos authentication does not return port# Nov 23, 2023

H-Yeo reviewed Nov 23, 2023

View reviewed changes

Removed DataTestUtility.IsManagedSNI as there is already DataTestUtil…

8de6014

…ity.IsUsingManagedSNI.

DavoudEshtehari reviewed Nov 23, 2023

View reviewed changes

DavoudEshtehari added this to the 5.2.0-preview4 milestone Nov 23, 2023

DavoudEshtehari added the Ⓜ️ Managed SNI Use this label if the issue/PR relates to issues in Managed SNI label Nov 23, 2023

arellegue added 3 commits November 23, 2023 16:53

Add Type suffix to reflected types.

2d3ae85

Fixed checks for localhost. Use object initializer for array of types declarations.

Use meaningful variable names per Davoud.

5907497

Use object type instead of var type to make code easier to understand…

589d5bd

… per Javad.

David-Engel reviewed Nov 28, 2023

View reviewed changes

arellegue added 2 commits November 28, 2023 16:47

Implement suggestions from PR Review.

a7bd4ed

Removed all instance of top secret domain name.

94457f9

David-Engel reviewed Nov 30, 2023

View reviewed changes

src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs Outdated Show resolved Hide resolved

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/InstanceNameTest/InstanceNameTest.cs Outdated Show resolved Hide resolved

Moved IsManagedKerberos function from DataTestUtility class to Instan…

6c6f6dc

…ceNameTest class.

arellegue requested a review from David-Engel December 4, 2023 19:24

David-Engel approved these changes Dec 4, 2023

View reviewed changes

arellegue requested review from DavoudEshtehari and H-Yeo December 5, 2023 22:47

arellegue linked an issue Dec 5, 2023 that may be closed by this pull request

MDS on Linux sends SPN with instance name instead of port #2187

Closed

DavoudEshtehari reviewed Dec 6, 2023

View reviewed changes

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/InstanceNameTest/InstanceNameTest.cs Outdated Show resolved Hide resolved

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/InstanceNameTest/InstanceNameTest.cs Outdated Show resolved Hide resolved

Applied suggestions by Davoud "Even though this PR suggests an update…

030d8f7

… on ManagedSNI, we expect the same behavior on native SNI.".

arellegue requested a review from DavoudEshtehari December 7, 2023 00:14

DavoudEshtehari reviewed Dec 7, 2023

View reviewed changes

Disable unit test for Linux SPN Port# issue using ActiveIssue 27824.

4a92bc7

David-Engel reviewed Dec 7, 2023

View reviewed changes

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/InstanceNameTest/InstanceNameTest.cs Outdated Show resolved Hide resolved

Changed comment for ActiveIssue 27824.

2c8e139

JRahnama approved these changes Dec 7, 2023

View reviewed changes

JRahnama merged commit 59080c8 into dotnet:main Dec 7, 2023
138 of 147 checks passed

JRahnama mentioned this pull request Mar 12, 2024

Regression | Revert PR #2281 SNIProxy code change #2395

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix | Linux SPN port number using named instance and Kerberos authentication does not return port# #2240

Fix | Linux SPN port number using named instance and Kerberos authentication does not return port# #2240

arellegue commented Nov 23, 2023 •

edited

Loading

H-Yeo left a comment

codecov bot commented Nov 23, 2023 •

edited

Loading

DavoudEshtehari Dec 7, 2023


		SqlConnectionStringBuilder builder = new(connStr);

		Assert.True(DataTestUtility.ParseDataSource(builder.DataSource, out string hostname, out _, out string instanceName));

Fix | Linux SPN port number using named instance and Kerberos authentication does not return port# #2240

Fix | Linux SPN port number using named instance and Kerberos authentication does not return port# #2240

Conversation

arellegue commented Nov 23, 2023 • edited Loading

H-Yeo left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 23, 2023 • edited Loading

Codecov Report

DavoudEshtehari Dec 7, 2023

Choose a reason for hiding this comment

arellegue commented Nov 23, 2023 •

edited

Loading

codecov bot commented Nov 23, 2023 •

edited

Loading