NotificationsYou must be signed in to change notification settings
Fork311
Star927

[Tests] Diagnose Windows cert creation failures#3237

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

paulmedynski merged 3 commits intomainfromdev/paul/issue-3223-pwsh-test-failure

Apr 8, 2025

Merged

[Tests] Diagnose Windows cert creation failures#3237

paulmedynski merged 3 commits intomainfromdev/paul/issue-3223-pwsh-test-failure

Apr 8, 2025

Conversation

Copy link

Contributor

paulmedynski commentedMar 24, 2025•
edited
Loading

Diagnostic changes for#3223. We're now emitting the PowerShell script content used to generate a self-signed cert and add it to the Windows system cert store. This may help figure out why the cert operations fail intermittently.

Created this draft PR to run the CI pipelines where the failures sometimes occur and acquire the PowerShell content for further investigation.

Copy link

Contributor

edwardneal commentedMar 24, 2025

I think I introduced this in#3034, which used PowerShell to generate certificates more frequently. Apologies if so!

The only difference in the PS invocation which I can see between my version and the preceding one is that we no longer load the user profile. Does settingProcessStartInfo.LoadUserProfile to true fix this?

Copy link

ContributorAuthor

paulmedynski commentedMar 24, 2025

I think I introduced this in#3034, which used PowerShell to generate certificates more frequently. Apologies if so!
The only difference in the PS invocation which I can see between my version and the preceding one is that we no longer load the user profile. Does settingProcessStartInfo.LoadUserProfile to true fix this?

LoadUserProfile is already being set to true, so likely not the issue. I've run some manual tests in a Windows Server container, and the PS script works fine. Since the failures are intermittent, I suspect a flaky test environment.

We're having some ADO CI pipeline issues that are preventing me from running CI reliably right now, so I will circle back once the overall CI stuff settles down.

Copy link

codecovbot commentedMar 24, 2025•
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.58%. Comparing base(1247ca4) to head(4dabe63).
Report is 15 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@##             main    #3237       +/-   ##===========================================+ Coverage   72.69%   92.58%   +19.88%===========================================  Files         303        6      -297       Lines       59718      310    -59408     ===========================================- Hits        43414      287    -43127+ Misses      16304       23    -16281

Flag	Coverage Δ
addons	`92.58% <ø> (ø)`
netcore	`?`
netfx	`?`

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.

🚀 New features to boost your workflow:

❄️Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

paulmedynski force-pushed thedev/paul/issue-3223-pwsh-test-failure branch from11d4009 tob1bb29fCompare

March 25, 2025 10:57

Issue 34304:#3223Fix Functional test failures in CI

5a29f5b

- Added powershell script content to test error output for disgnostics.- Replaced #if NET9_0 with #if NET9_0_OR_GREATER to future-proof our directives.- Removed unnecessary NETFRAMEWORK symbol.- Updated download URL for .NET x86 installer for tests pipelines.

paulmedynski force-pushed thedev/paul/issue-3223-pwsh-test-failure branch fromb65989c to5a29f5bCompare

March 31, 2025 16:34

Issue 34304:#3223Fix Functional test failures in CI

1336f73

- Added some retries to the PowerShell script execution to see if that helps avoid overall test failures.

Copy link

ContributorAuthor

paulmedynski commentedApr 1, 2025

The latest commit adds a retry within theCertificateFixtureBase.CreateCertificate that seems to work around theACCESS DENIED issue. We now see this in the test logs:

2025-03-31T17:22:11.0473456Z   PowerShell command failed with exit code 1 on attempt 1 of 3; retrying in 5 seconds......2025-03-31T17:22:19.7322237Z     Passed Microsoft.Data.SqlClient.Tests.AlwaysEncryptedTests.ExceptionsAlgorithmErrors.TestInvalidCipherText [44 ms]

There are no warnings or errors for the CI runs.

paulmedynski marked this pull request as ready for review

April 1, 2025 11:05

paulmedynski requested review fromCopilot anda team

April 1, 2025 11:06

CopilotAI reviewed

Apr 1, 2025

View reviewed changes

Copy link

Contributor

CopilotAI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates various preprocessor directives from NET9_0 to NET9_0_OR_GREATER and introduces enhanced diagnostics for Windows certificate creation failures via retry logic and additional output details. Key changes include updating build conditions in multiple certificate and SQL components, adding retry logic with delay and logging for PowerShell command execution in certificate creation, and updating a pipeline URL to a newer dotnet-install.ps1 script location.

Reviewed Changes

Copilot reviewed 9 out of 10 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
ColumnEncryptionCertificateFixture.cs	Updated preprocessor condition to reflect newer target frameworks
CertificateFixtureBase.cs	Replaced NET9_0 with NET9_0_OR_GREATER and added retry logic with delay and diagnostic output for certificate generation
CertificateTestWithTdsServer.cs	Updated preprocessor condition for certificate loading
CertificateUtility.cs	Updated preprocessor condition for certificate loading and RSA decryption/signature verification
SqlDbTypeExtensions.cs	Updated preprocessor condition for exposing the Json SqlDbType constant
VirtualSecureModeEnclaveProvider.cs	Updated preprocessor condition for loading a certificate from binary payload
SNICommon.cs	Updated preprocessor condition for validating SSL server certificates
ci-run-tests-job.yml	Updated the URL to the latest dotnet-install.ps1 script

Files not reviewed (1)

src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj: Language not supported

Comments suppressed due to low confidence (1)

src/Microsoft.Data.SqlClient/tests/tools/Microsoft.Data.SqlClient.TestUtilities/Fixtures/CertificateFixtureBase.cs:173

Consider adding tests to verify the retry logic and ensure correct behavior on repeated PowerShell failures.

for (int attempt = 1; attempt <= retries; ++attempt)

paulmedynski commented

Apr 1, 2025

View reviewed changes

Copy link

ContributorAuthor

paulmedynski left a comment

Choose a reason for hiding this comment

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

Ready for review, with my comments added for context.

eng/pipelines/common/templates/jobs/ci-run-tests-job.yml

		@@ -236,7 +236,7 @@ jobs:
		- powershell: \|
		dotnet --info

		Invoke-WebRequest https://dot.net/v1/dotnet-install.ps1 -OutFile dotnet-install.ps1
		Invoke-WebRequest https://builds.dotnet.microsoft.com/dotnet/scripts/v1/dotnet-install.ps1 -OutFile dotnet-install.ps1

Copy link

ContributorAuthor

paulmedynskiApr 1, 2025

Choose a reason for hiding this comment

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

This avoidsInvoke-WebRequest failures due to some dead IPs in the DNS results fordot.net, and is the proper canonical DNS name for the dotnet scripts.

Copy link

Contributor

benrr101Apr 3, 2025

Choose a reason for hiding this comment

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

This feels a bit fishy since we have the UseDotNet task we could run before this...

Copy link

ContributorAuthor

paulmedynskiApr 4, 2025

Choose a reason for hiding this comment

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

Agreed, I'm getting conflicting information:

https://devblogs.microsoft.com/dotnet/critical-dotnet-install-links-are-changing/

Says that the canonical CDN isbuilds.dotnet.microsoft.com.

https://learn.microsoft.com/en-us/dotnet/core/install/windows#install-with-powershell

Says the URL ishttps://dot.net/v1/dotnet-install.ps1

Those DNS names resolve to different IPs:

;; ANSWER SECTION:dot.net.                143     IN      A       20.112.250.133dot.net.                143     IN      A       20.236.44.162dot.net.                143     IN      A       20.70.246.20dot.net.                143     IN      A       20.76.201.171dot.net.                143     IN      A       20.231.239.246

;; ANSWER SECTION:builds.dotnet.microsoft.com. 2922 IN    CNAME   dotnetcli.trafficmanager.net.dotnetcli.trafficmanager.net. 60 IN     CNAME   builds.dotnet.microsoft.com.edgesuite.net.builds.dotnet.microsoft.com.edgesuite.net. 9469 IN CNAME a441.dscd.akamai.net.a441.dscd.akamai.net.   20      IN      A       24.222.184.168a441.dscd.akamai.net.   20      IN      A       24.222.184.138

I can revert it back, but dead IPs in thedot.net records were definitely causing job failures earlier.

Copy link

Contributor

ErikEJApr 4, 2025

Choose a reason for hiding this comment

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

Usebuilds.dotnet.microsoft.com

Copy link

ContributorAuthor

paulmedynskiApr 4, 2025

Choose a reason for hiding this comment

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

Heh, thedot.net hosts just HTTP redirect to the other URL:

$ curl -I https://dot.net/v1/dotnet-install.ps1HTTP/2 301date: Fri, 04 Apr 2025 19:15:47 GMTserver: Kestrellocation: https://builds.dotnet.microsoft.com/dotnet/scripts/v1/dotnet-install.ps1strict-transport-security: max-age=31536000

I guess that one of them is/was dead. I think I'll keep it as-is.

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNICommon.csShow resolvedHide resolved

src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csprojShow resolvedHide resolved

...Client/tests/tools/Microsoft.Data.SqlClient.TestUtilities/Fixtures/CertificateFixtureBase.csShow resolvedHide resolved

paulmedynski added this to the6.1-preview1 milestone

Apr 2, 2025

mdaigle self-assigned this

Apr 2, 2025

benrr101 approved these changes

Apr 3, 2025

View reviewed changes

Copy link

Contributor

benrr101 left a comment

Choose a reason for hiding this comment

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

Overall pretty good - if it works, I won't complain too hard.

...Client/tests/tools/Microsoft.Data.SqlClient.TestUtilities/Fixtures/CertificateFixtureBase.cs OutdatedShow resolvedHide resolved

eng/pipelines/common/templates/jobs/ci-run-tests-job.yml

		@@ -236,7 +236,7 @@ jobs:
		- powershell: \|
		dotnet --info

		Invoke-WebRequest https://dot.net/v1/dotnet-install.ps1 -OutFile dotnet-install.ps1
		Invoke-WebRequest https://builds.dotnet.microsoft.com/dotnet/scripts/v1/dotnet-install.ps1 -OutFile dotnet-install.ps1