-
Notifications
You must be signed in to change notification settings - Fork 310
[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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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 setting |
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. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
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
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:
|
11d4009
to
b1bb29f
Compare
- 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.
b65989c
to
5a29f5b
Compare
- Added some retries to the PowerShell script execution to see if that helps avoid overall test failures.
The latest commit adds a retry within the
There are no warnings or errors for the CI runs. |
There was a problem hiding this 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)
There was a problem hiding this 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.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This avoids Invoke-WebRequest
failures due to some dead IPs in the DNS results for dot.net
, and is the proper canonical DNS name for the dotnet scripts.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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 is builds.dotnet.microsoft.com
.
https://learn.microsoft.com/en-us/dotnet/core/install/windows#install-with-powershell
Says the URL is https://dot.net/v1/dotnet-install.ps1
Those DNS names resolve to different IPs:
;; ANSWER SECTION:
dot.net. 143 IN A 20.112.250.133
dot.net. 143 IN A 20.236.44.162
dot.net. 143 IN A 20.70.246.20
dot.net. 143 IN A 20.76.201.171
dot.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.168
a441.dscd.akamai.net. 20 IN A 24.222.184.138
I can revert it back, but dead IPs in the dot.net
records were definitely causing job failures earlier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use builds.dotnet.microsoft.com
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh, the dot.net
hosts just HTTP redirect to the other URL:
$ curl -I https://dot.net/v1/dotnet-install.ps1
HTTP/2 301
date: Fri, 04 Apr 2025 19:15:47 GMT
server: Kestrel
location: https://builds.dotnet.microsoft.com/dotnet/scripts/v1/dotnet-install.ps1
strict-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.cs
Show resolved
Hide resolved
...Client/tests/tools/Microsoft.Data.SqlClient.TestUtilities/Fixtures/CertificateFixtureBase.cs
Show resolved
Hide resolved
There was a problem hiding this 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
Outdated
Show resolved
Hide resolved
@@ -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 |
There was a problem hiding this comment.
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...
- Addressed review comments.
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.