fix(sdk-dotnet): provide default AWS region in SecretProviderFactory#147
fix(sdk-dotnet): provide default AWS region in SecretProviderFactory#147
Conversation
Implement the Envilder .NET SDK targeting netstandard2.0 with hexagonal architecture (Domain/Application/Infrastructure). Includes map-file parsing, secret resolution, IConfiguration and IServiceCollection integration, plus unit and acceptance tests using LocalStack and Lowkey Vault via Testcontainers.
Implement the Envilder .NET SDK targeting netstandard2.0 with hexagonal architecture (Domain/Application/Infrastructure). Includes map-file parsing, secret resolution, IConfiguration and IServiceCollection integration, plus unit and acceptance tests using LocalStack and Lowkey Vault via Testcontainers.
…nvilder into macalbert/sdk-dotnet
Replaces string provider selection with SecretProviderType enum for strong typing and clarity. Refactors SecretProviderFactory and parsing logic accordingly. Updates tests to use the enum and improves LocalStack test reliability with health checks and embedded secrets.
Add comprehensive XML documentation for all public APIs to improve discoverability and maintainability. Introduce end-to-end tests using LocalStack and Lowkey Vault to verify integration with AWS SSM and Azure Key Vault, including configuration and error handling scenarios. Update test dependencies to Microsoft.Extensions.* v10 for .NET 8 support.
Allow MapFileConfig and ParsedMapFile properties to be set after construction. Remove IsExternalInit.cs as it is no longer needed.
Enable slash-delimited keys in map file for hierarchical config binding (e.g., Database/ConnectionString). Refactor ParsedMapFile to use immutable properties. Add validation and error handling to AddEnvilder. Remove unused Region option. Update and add tests for section binding and error cases.
Add explicit null and argument validation to constructors and methods for improved robustness and fail-fast behavior. Update tests to cover new error cases. Update package versions and improve resource handling and code clarity.
Ensure integration tests run in CI by falling back to AWS when the configured secret provider (e.g., Azure Key Vault) is unavailable.
Add workflow_dispatch trigger for manual execution
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughSecretProviderFactory now resolves and supplies an explicit AWS region (with a USEast1 fallback) when constructing SSM clients. Tests/fixtures expanded exception handling and added a test for factory behavior with no AWS region env vars. Documentation and README entries for the .NET SDK were added. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The AWS SDK for .NET requires a RegionEndpoint or ServiceURL when creating an SSM client. Without one, AmazonClientException is thrown. This works locally when ~/.aws/config exists but fails in CI where no AWS config is present. - Add RegionEndpoint.USEast1 as default in CreateAwsSecretProvider - Broaden LocalStackFixture.ResolveClient catch to handle any exception - Exclude EndToEnd tests from publish-nuget workflow (need Docker + creds) - Add tests-dotnet-sdk.yml CI workflow for PR validation - Add regression test Should_SelectAwsProvider_When_NoAwsRegionConfigured
a2376d0 to
1a88c54
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/sdks/dotnet/Infrastructure/SecretProviderFactory.cs (1)
19-19: Hardcoded default region may cause issues for users with secrets in other regions.While this fix resolves the CI issue, hardcoding
USEast1means users with AWS SSM parameters in other regions (e.g.,eu-west-1,ap-southeast-1) will silently connect to the wrong region and fail to retrieve their secrets.Consider making the region configurable via
MapFileConfigand/orEnvilderOptions, falling back toUSEast1only when no region is specified anywhere:♻️ Proposed approach to make region configurable
public static class SecretProviderFactory { - private static readonly Amazon.RegionEndpoint DefaultRegion = Amazon.RegionEndpoint.USEast1; + private static readonly Amazon.RegionEndpoint FallbackRegion = Amazon.RegionEndpoint.USEast1;Then in
CreateAwsSecretProvider:+ var regionName = options?.Region ?? config.Region; + var region = string.IsNullOrWhiteSpace(regionName) + ? FallbackRegion + : Amazon.RegionEndpoint.GetBySystemName(regionName); + if (!string.IsNullOrWhiteSpace(profile)) { // ... - return new(new AmazonSimpleSystemsManagementClient(credentials, DefaultRegion)); + return new(new AmazonSimpleSystemsManagementClient(credentials, region)); } return new(new AmazonSimpleSystemsManagementClient(new AmazonSimpleSystemsManagementConfig { - RegionEndpoint = DefaultRegion, + RegionEndpoint = region, }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sdks/dotnet/Infrastructure/SecretProviderFactory.cs` at line 19, The DefaultRegion constant in SecretProviderFactory is hardcoded to USEast1 which can cause wrong-region lookups; update SecretProviderFactory to accept a configurable region (use MapFileConfig and/or EnvilderOptions as the source) and have CreateAwsSecretProvider use that configured region, falling back to Amazon.RegionEndpoint.USEast1 only if no region is provided; reference the DefaultRegion symbol to remove/replace it, and wire the region through the factory constructor or method parameters so callers can specify region via MapFileConfig/EnvilderOptions.tests/sdks/dotnet/Fixtures/LocalStackFixture.cs (1)
94-97: Consider catching specific exception types instead of bareException.Catching all exceptions is overly broad and may mask unexpected errors (e.g.,
OutOfMemoryException,ArgumentNullExceptionfrom a bug). The PR description mentions AWS SDK throwsAmazonClientException, so consider catching that alongsideInvalidOperationException:♻️ Proposed fix to catch specific exceptions
+using Amazon.Runtime; + // ... private static EnvilderClient ResolveClient(ParsedMapFile mapFile) { try { return new(SecretProviderFactory.Create(mapFile.Config)); } - catch (Exception) + catch (Exception ex) when (ex is InvalidOperationException or AmazonServiceException) { return new(SecretProviderFactory.Create(new() { Provider = SecretProviderType.Aws })); } }This uses an exception filter to catch only the expected failure types while letting truly unexpected exceptions propagate for investigation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/sdks/dotnet/Fixtures/LocalStackFixture.cs` around lines 94 - 97, Replace the broad catch(Exception) in LocalStackFixture.cs with a specific exception filter that only handles the expected AWS/client errors; catch AmazonClientException and InvalidOperationException (e.g., use "catch (Exception ex) when (ex is AmazonClientException || ex is InvalidOperationException)") and then return the fallback SecretProviderFactory.Create(new() { Provider = SecretProviderType.Aws }); ensure the AmazonClientException type is available via the appropriate AWS SDK namespace import.tests/sdks/dotnet/Infrastructure/SecretProviderFactoryTests.cs (1)
102-145: Good regression test, but environment manipulation may cause test flakiness with parallel execution.This test correctly validates the fix for missing AWS region configuration. However, modifying process-wide environment variables can cause flaky behavior if xUnit runs tests in parallel (which it does by default for different test classes).
Consider adding
[Collection("Sequential")]or disabling parallelization for this test class to prevent interference:♻️ Suggested improvements
- Prevent parallel execution issues:
[Collection("EnvironmentTests")] public class SecretProviderFactoryTests
- Enhance assertion to verify the returned type:
// Assert - act.Should().NotThrow(); + var provider = act.Should().NotThrow().Subject; + provider.Should().BeOfType<AwsSsmSecretProvider>();The
Subjectproperty from AwesomeAssertions (FluentAssertions fork) allows chaining assertions on the actual result, providing stronger verification that the factory returns the expected provider type.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/sdks/dotnet/Infrastructure/SecretProviderFactoryTests.cs` around lines 102 - 145, The test Should_SelectAwsProvider_When_NoAwsRegionConfigured manipulates process-wide environment variables and can flake when xUnit runs tests in parallel; to fix it, mark the test class SecretProviderFactoryTests with a collection attribute that forces sequential execution (e.g., add a [Collection("EnvironmentTests")] or equivalent to the class) or disable parallelization for this test class, and strengthen the assertion by asserting the actual returned provider type from SecretProviderFactory.Create(config) (use FluentAssertions/Subject chaining to verify the concrete provider type) so the test both avoids parallel-env interference and validates the returned type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/sdks/dotnet/Infrastructure/SecretProviderFactory.cs`:
- Line 19: The DefaultRegion constant in SecretProviderFactory is hardcoded to
USEast1 which can cause wrong-region lookups; update SecretProviderFactory to
accept a configurable region (use MapFileConfig and/or EnvilderOptions as the
source) and have CreateAwsSecretProvider use that configured region, falling
back to Amazon.RegionEndpoint.USEast1 only if no region is provided; reference
the DefaultRegion symbol to remove/replace it, and wire the region through the
factory constructor or method parameters so callers can specify region via
MapFileConfig/EnvilderOptions.
In `@tests/sdks/dotnet/Fixtures/LocalStackFixture.cs`:
- Around line 94-97: Replace the broad catch(Exception) in LocalStackFixture.cs
with a specific exception filter that only handles the expected AWS/client
errors; catch AmazonClientException and InvalidOperationException (e.g., use
"catch (Exception ex) when (ex is AmazonClientException || ex is
InvalidOperationException)") and then return the fallback
SecretProviderFactory.Create(new() { Provider = SecretProviderType.Aws });
ensure the AmazonClientException type is available via the appropriate AWS SDK
namespace import.
In `@tests/sdks/dotnet/Infrastructure/SecretProviderFactoryTests.cs`:
- Around line 102-145: The test
Should_SelectAwsProvider_When_NoAwsRegionConfigured manipulates process-wide
environment variables and can flake when xUnit runs tests in parallel; to fix
it, mark the test class SecretProviderFactoryTests with a collection attribute
that forces sequential execution (e.g., add a [Collection("EnvironmentTests")]
or equivalent to the class) or disable parallelization for this test class, and
strengthen the assertion by asserting the actual returned provider type from
SecretProviderFactory.Create(config) (use FluentAssertions/Subject chaining to
verify the concrete provider type) so the test both avoids parallel-env
interference and validates the returned type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 27471fc5-92a6-4ff4-a3fe-67a1bcf8c1cc
⛔ Files ignored due to path filters (2)
.github/workflows/publish-nuget.ymlis excluded by none and included by none.github/workflows/tests-dotnet-sdk.ymlis excluded by none and included by none
📒 Files selected for processing (3)
src/sdks/dotnet/Infrastructure/SecretProviderFactory.cstests/sdks/dotnet/Fixtures/LocalStackFixture.cstests/sdks/dotnet/Infrastructure/SecretProviderFactoryTests.cs
- Added .NET SDK installation instructions and usage examples to README - Updated CHANGELOG with .NET SDK details and CI workflow - Enhanced documentation for SDK architecture and prerequisites
Switch SecretProviderFactoryTests to use ContainersCollection for better alignment with shared test fixture setup.
Summary
Introduces the .NET SDK for Envilder with AWS SSM and Azure Key Vault support, along with CI pipeline fixes for AWS region resolution. The SDK follows the same map-file contract as the TypeScript core and supports hierarchical config binding, runtime provider overrides, and end-to-end testing via TestContainers.
Changes
.NET SDK (
src/sdks/dotnet/)ISecretProviderport,EnvilderClient, andMapFileParserAwsSsmSecretProviderandAzureKeyVaultSecretProvideradaptersSecretProviderFactorywith dynamic AWS region resolution from profile config and env vars$configsection)SecretProviderType) for provider type selectionParsedMapFileand improve secret resolution logicDocumentation
CI / Workflows
.NET SDK testworkflow with caching and status reportingLOCALSTACK_AUTH_TOKENin CIactions/cache,actions/upload-artifact, andactions/checkoutversionstests/directory in NuGet publish workflowTests
ContainersCollectionusingforLocalStackHealthCheckTesting
dotnet build src/sdks/dotnet/Envilder.slndotnet test tests/sdks/dotnet/pnpm testpasses (TypeScript core unaffected)pnpm lintpassesRelated
Builds on the .NET SDK branch (
macalbert/sdk-dotnet).