Encode connection properties and endpoints env var names #13009
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 13009Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 13009" |
sebastienros
requested review from
eerhardt
and removed request for
Copilot,
jfversluis and
mitchdenny
| // Also remove the {RESOURCENAME}_{ENDPOINTNAME} format variable (e.g., MAUIAPP-OTLP_OTLP) | ||
| // The resource name keeps its case/dashes, endpoint name is uppercased | ||
| var directEndpointKey = $"{tunnelConfig.OtlpStub.Name.ToUpperInvariant()}_OTLP"; | ||
| var directEndpointKey = $"{EnvironmentVariableNameEncoder.Encode(tunnelConfig.OtlpStub.Name).ToUpperInvariant()}_OTLP"; |
There was a problem hiding this comment.
When the ENV is added for the endpoint it is encoded. The code here is trying to remove this value. So the logic needs to match. It's not "just" uppercase now, it's encoding and uppercase.
There was a problem hiding this comment.
Updated the comment and referenced where this is defined.
|
@radical any known issue with macos legs? Keeps failing. |
| { | ||
| context.EnvironmentVariables[$"{serviceName.ToUpperInvariant()}_{endpointName.ToUpperInvariant()}"] = port.TunnelEndpoint; | ||
| var endpointKey = $"{encodedServiceName.ToUpperInvariant()}_{encodedEndpointName.ToUpperInvariant()}"; | ||
| context.EnvironmentVariables[endpointKey] = port.TunnelEndpoint; |
There was a problem hiding this comment.
Is it ever valid to have a non alphanumeric/underscore in an env var name? The problem with requiring the user to use EnvironmentVariableNameEncoder is they'll inevitably not know encoding is required or forget.
I wonder if the encoding should happen at a lower level. Why the current approach?
There was a problem hiding this comment.
I just spoke to Seb about that.
There was a problem hiding this comment.
My concern is that we feed ENVs to dotnet configuration that is looking for entries using resource names too, for instance in connection strings. If we encode these then we are breaking the integrations (service discovery and connection strings) and focused only on endpoints and connection properties.
If we wanted to handle other cases we'd need to refactor all the integrations. Hopefully there is a better option or I am wrong.
There was a problem hiding this comment.
Pull Request Overview
This PR introduces environment variable name encoding to ensure that resource and service names containing dashes or other invalid characters are properly converted to valid environment variable names by replacing invalid characters with underscores. This addresses compatibility issues with Linux environment variables and usage in languages like TypeScript.
Key changes include:
- Added
EnvironmentVariableNameEncoderutility class to encode names for environment variable usage - Applied encoding to endpoint environment variables and connection properties
- Service discovery keys deliberately remain unencoded to maintain compatibility with .NET configuration system
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Shared/EnvironmentVariableNameEncoder.cs | New utility class that encodes strings to be safe for environment variable names by replacing invalid characters with underscores |
| src/Aspire.Hosting/ResourceBuilderExtensions.cs | Updated to encode service/resource names and schemes when creating endpoint and connection property environment variables |
| src/Aspire.Hosting/Aspire.Hosting.csproj | Added reference to the new EnvironmentVariableNameEncoder shared file |
| src/Aspire.Hosting.DevTunnels/DevTunnelResourceBuilderExtensions.cs | Applied encoding to service and endpoint names for dev tunnel environment variables |
| src/Aspire.Hosting.DevTunnels/Aspire.Hosting.DevTunnels.csproj | Added reference to the new EnvironmentVariableNameEncoder shared file |
| src/Aspire.Hosting.Maui/MauiOtlpExtensions.cs | Updated to use encoded resource name when removing intermediate environment variables |
| src/Aspire.Hosting.Maui/Aspire.Hosting.Maui.csproj | Added reference to the new EnvironmentVariableNameEncoder shared file |
| tests/Aspire.Hosting.Tests/Utils/EnvironmentVariableNameEncoderTests.cs | Comprehensive unit tests for the EnvironmentVariableNameEncoder class |
| tests/Aspire.Hosting.Tests/WithReferenceTests.cs | Integration tests verifying encoded environment variables for resources and connection properties with dashes |
| tests/Aspire.Hosting.Maui.Tests/MauiPlatformExtensionsTests.cs | Test verifying OTLP dev tunnel cleanup uses encoded names; commented out non-Windows platform tests |
sebastienros
force-pushed
the
sebros/encodeenvs
branch
from
cb89f8d to
7bc0584
Compare
| private static string CreateTempProjectFile(string content) | ||
| { | ||
| var tempFile = Path.Combine(Path.GetTempPath(), $"test_{Guid.NewGuid()}.csproj"); | ||
| var tempFolder = Directory.CreateTempSubdirectory(); |
There was a problem hiding this comment.
Putting the csproj directly in the temp folder would make each build take minutes if the temp folder has lots of files in it. Could also be a security concern.
Description
ENV in Linux are not supposed to contain dashes (
-) as they may not work when used in bash. Another example is when these are used in typescript as property names.This PR encodes the ENVs that are not meant to be used in dotnet configuration where the naming needs to be kept. So connection properties and endpoints are encoded, while connection strings and service discovery keys are not.
Fixes #12974
Checklist
<remarks />and<code />elements on your triple slash comments?