Skip to content

Conversation

paulmedynski
Copy link
Contributor

@paulmedynski paulmedynski commented Jul 17, 2025

Port of #3482 from main to release/5.1

  • Added 4 new trusted AKV URLs.
  • Fixed existing manual tests and added unit tests.
  • Addressed incompatible .NET syntax that isn't supported by C# 9.0.

@paulmedynski paulmedynski added this to the 5.1.8 milestone Jul 17, 2025
@Copilot Copilot AI review requested due to automatic review settings July 17, 2025 11:00
Copy link
Contributor

@Copilot Copilot AI 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

Port of #3482 from main to release/5.1, adding new Azure Key Vault endpoints for French and German sovereign clouds and updating tests.

  • Extended AzureKeyVaultPublicDomainNames with additional public and HSM vault domains.
  • Refactored InvalidAKVUrlTrustedEndpoints to return ArgumentException and centralized error message generation.
  • Updated manual test project to include a new TrustedUrlsTest and refactored exception assertions in ExceptionTestAKVStore.

Reviewed Changes

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

Show a summary per file
File Description
src/Microsoft.Data.SqlClient/add-ons/AzureKeyVaultProvider/Constants.cs Added new trusted AKV endpoint domains for French and German sovereign clouds.
src/Microsoft.Data.SqlClient/add-ons/AzureKeyVaultProvider/Utils.cs Changed InvalidAKVUrlTrustedEndpoints return type from Exception to ArgumentException.
src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/TrustedUrlsTest.cs Introduced a manual test class to validate both valid and invalid trusted URLs.
src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/ExceptionTestAKVStore.cs Refactored exception assertions to use TrustedUrlsTest.MakeInvalidVaultErrorMessage.
src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTesting.Tests.csproj Updated compile includes: added TrustedUrlsTest.cs and reordered AKV setup files.
Comments suppressed due to low confidence (3)

src/Microsoft.Data.SqlClient/add-ons/AzureKeyVaultProvider/Constants.cs:20

  • The addition of new trusted AKV domain names in Constants.AzureKeyVaultPublicDomainNames warrants unit tests to verify their acceptance by the validation logic, ensuring these domains are correctly allowed in automated (non-manual) tests.
            "vault.cloudapi.microsoft.scloud", // USSec

src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/TrustedUrlsTest.cs:35

  • [nitpick] Duplicating the error message formatting logic in MakeInvalidVaultErrorMessage can lead to maintenance overhead if the production template changes; consider loading the actual template from the production resource (Strings.InvalidAkvKeyPathTrustedTemplate) or centralizing this logic.
        public static string MakeInvalidVaultErrorMessage(string url)

src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/TrustedUrlsTest.cs:24

  • Using reflection to invoke the private ValidateNonEmptyAKVPath method is brittle and may break on internal refactorings; consider exposing a test-friendly public helper or validating through the public Encrypt/DecryptColumnEncryptionKey methods instead.
            _method = clazz.GetMethod(

Copy link

codecov bot commented Jul 17, 2025

Codecov Report

Attention: Patch coverage is 0% with 18 lines in your changes missing coverage. Please review.

Project coverage is 69.29%. Comparing base (6af24bc) to head (0d77937).

Files with missing lines Patch % Lines
...lClient/add-ons/AzureKeyVaultProvider/Constants.cs 0.00% 18 Missing ⚠️
Additional details and impacted files
@@               Coverage Diff               @@
##           release/5.1    #3483      +/-   ##
===============================================
- Coverage        71.81%   69.29%   -2.53%     
===============================================
  Files              293      293              
  Lines            61647    61656       +9     
===============================================
- Hits             44272    42723    -1549     
- Misses           17375    18933    +1558     
Flag Coverage Δ
addons 0.00% <0.00%> (-92.39%) ⬇️
netcore 69.90% <ø> (-5.99%) ⬇️
netfx 69.49% <ø> (-0.06%) ⬇️

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.

@cheenamalhotra cheenamalhotra marked this pull request as draft July 22, 2025 19:35
@paulmedynski paulmedynski deleted the dev/paul/release/5.1-akv-urls branch July 23, 2025 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant