Allow certificates config context to access raw arguments and environment before processing #12995
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 12995Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 12995" |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors certificate trust configuration to provide access to previously defined environment variables and arguments before processing. This enables callbacks to append or update existing configuration instead of wholesale replacement. The change is particularly useful for Node.js applications where certificate trust now uses the NODE_OPTIONS environment variable to ensure the flag is passed even when Node.js isn't directly invoked.
Key changes include:
- Introduction of
Gather*methods that collect unprocessed arguments and environment variables, allowing multiple callbacks to contribute before resolution - Refactoring of certificate trust configuration to receive and modify existing arguments/environment collections
- Update to Node.js certificate trust to use
NODE_OPTIONSenvironment variable with intelligent appending logic
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/Aspire.Hosting.Tests/DistributedApplicationTests.cs | Adds comprehensive test verifying environment variables are accessible in certificate trust configuration callbacks |
| src/Aspire.Hosting/Dcp/DcpExecutor.cs | Refactors executable and container creation to use new gather/process pattern for arguments and environment variables; updates certificate trust configuration integration |
| src/Aspire.Hosting/ApplicationModel/ResourceExtensions.cs | Introduces Gather* methods to collect unprocessed configuration and ProcessGathered* methods to resolve them; updates certificate trust processing to work with pre-gathered collections |
| src/Aspire.Hosting.JavaScript/JavaScriptHostingExtensions.cs | Changes Node.js certificate trust from command-line argument to NODE_OPTIONS environment variable with logic to append to existing values |
| this IResource resource, | ||
| DistributedApplicationExecutionContext executionContext, | ||
| // (unprocessed, processed, exception, isSensitive) | ||
| Action<object?, string?, Exception?, bool> processValue, |
There was a problem hiding this comment.
Maybe we should pass a context here to solve tis overload problem.
There was a problem hiding this comment.
Main headache is that ProcessArgumentValuesAsync is public so I didn't want to break it. I could also just stick with calling the ProcessXValuesAsync methods in DcpExecutor and build the argument and environment contexts for certificate config out of the processed values.
There was a problem hiding this comment.
As long as the logic is shared that's fine. The main purpose was to avoid multiple implementations that had diverged from what tests were using.
There was a problem hiding this comment.
I'm going to update this to take advantage of a feature I added in DCP around simplifying public certificate config for both exes and containers and give a single unified method that'll hopefully make it easier to integrate this with deploy in the future.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
| if (OperatingSystem.IsMacOS()) | ||
| { | ||
| // Disable default developer certificate features in MacOS due to test performance issues | ||
| EnvVars["ASPIRE_DEVELOPER_CERTIFICATE_DEFAULT_TRUST"] = "false"; |
There was a problem hiding this comment.
@radical I'm disabling the new dev cert features in MacOS template tests due to what seem to likely be Mac keychain related issues trying to retrieve the private key for the dev cert. I'm not sure if it's just slowing the tests down so much that they time out or if there's some interactive mode it's trying to trigger for approval.
There was a problem hiding this comment.
I think we'll need to explicitly unlock the keychain in our mac testing runs using security unlock-keychain; there's examples in arcade and dotnet/runtime using the command.
2 participants
Previously certificate arguments and environment had no access to the previously defined environment or arguments, meaning the only option was to append to arguments or replace environment variables. This updates how values are processed to allow access to the previously configured environment and arguments with the ability to append or update values instead of replacing wholesale.
In addition, this PR updates the way system certificate trust is enabled for Node.JS based apps to use the NODE_OPTIONS environment variable to pass the config to node even if it isn't the direct invocation target. This is what prompted the change to allow appending config as it's possible a user may try to pass their own arguments via NODE_OPTIONS that we should try to preserve.