Fix endpoint resolution for individual Host/Port properties in WithEnvironment (#10892)

* Initial plan

* Fix endpoint resolution for individual Host/Port properties in WithEnvironment

Co-authored-by: davidfowl <[email protected]>

* Remove unused preprocessing infrastructure from ExpressionResolver

Co-authored-by: davidfowl <[email protected]>

* Update test data for ExpressionResolver and fix expected PORT value in WithEnvironmentTests

* Update src/Aspire.Hosting/ApplicationModel/ExpressionResolver.cs

Co-authored-by: Copilot <[email protected]>

* Fix Kafka host port in run mode

---------

Co-authored-by: copilot-swe-agent[bot] <[email protected]>
Co-authored-by: davidfowl <[email protected]>
Co-authored-by: David Fowler <[email protected]>
Co-authored-by: Copilot <[email protected]>

Author: 3 people

src/Aspire.Hosting.Kafka/KafkaBuilderExtensions.cs

@@ -1,6 +1,7 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
+using System.Globalization;
 using Aspire.Hosting.ApplicationModel;
 using Confluent.Kafka;
 using HealthChecks.Kafka;
@@ -214,7 +215,7 @@ private static void ConfigureKafkaContainer(EnvironmentCallbackContext context,
         var advertisedListeners = context.ExecutionContext.IsRunMode
             // In run mode, PLAINTEXT_INTERNAL assumes kafka is being accessed over a default Aspire container network and hardcodes the resource address
             // This will need to be refactored once updated service discovery APIs are available
-            ? ReferenceExpression.Create($"PLAINTEXT://localhost:29092,PLAINTEXT_HOST://localhost:{primaryEndpoint.Property(EndpointProperty.Port)},PLAINTEXT_INTERNAL://{resource.Name}:{internalEndpoint.Property(EndpointProperty.TargetPort)}")
+            ? ReferenceExpression.Create($"PLAINTEXT://localhost:29092,PLAINTEXT_HOST://localhost:{primaryEndpoint.Port.ToString(CultureInfo.InvariantCulture)},PLAINTEXT_INTERNAL://{resource.Name}:{internalEndpoint.Property(EndpointProperty.TargetPort)}")
             : ReferenceExpression.Create($"PLAINTEXT://{primaryEndpoint.Property(EndpointProperty.Host)}:29092,PLAINTEXT_HOST://{primaryEndpoint.Property(EndpointProperty.HostAndPort)},PLAINTEXT_INTERNAL://{internalEndpoint.Property(EndpointProperty.HostAndPort)}");
 
         context.EnvironmentVariables["KAFKA_ADVERTISED_LISTENERS"] = advertisedListeners;

src/Aspire.Hosting/ApplicationModel/ExpressionResolver.cs

@@ -7,71 +7,25 @@ namespace Aspire.Hosting.ApplicationModel;
 
 internal class ExpressionResolver(string containerHostName, CancellationToken cancellationToken, bool sourceIsContainer)
 {
-    class HostAndPortPresence
-    {
-        public bool HasHost { get; set; }
-        public bool HasPort { get; set; }
-    }
-
-    // For each endpoint, keep track of whether host and port are in use
-    // The key is the unique name of the endpoint, which is the resource name and endpoint name
-    readonly Dictionary<string, HostAndPortPresence> _endpointUsage = [];
-    static string EndpointUniqueName(EndpointReference endpointReference) => $"{endpointReference.Resource.Name}/{endpointReference.EndpointName}";
-
-    // This marks whether we are in the preprocess phase or not
-    // Not thread-safe, but we doesn't matter, since this class is never used concurrently
-    bool Preprocess { get; set; }
 
     async Task<string?> EvalEndpointAsync(EndpointReference endpointReference, EndpointProperty property)
     {
-        var endpointUniqueName = EndpointUniqueName(endpointReference);
-
-        // In the preprocess phase, our only goal is to determine if the host and port properties are both used
-        // for each endpoint.
-        if (Preprocess)
-        {
-            if (!_endpointUsage.TryGetValue(endpointUniqueName, out var hostAndPortPresence))
-            {
-                hostAndPortPresence = new HostAndPortPresence();
-                _endpointUsage[endpointUniqueName] = hostAndPortPresence;
-            }
-
-            if (property is EndpointProperty.Host or EndpointProperty.IPV4Host)
-            {
-                hostAndPortPresence.HasHost = true;
-            }
-            else if (property == EndpointProperty.Port)
-            {
-                hostAndPortPresence.HasPort = true;
-            }
-            else if (property is EndpointProperty.Url or EndpointProperty.HostAndPort)
-            {
-                hostAndPortPresence.HasHost = hostAndPortPresence.HasPort = true;
-            }
-            return string.Empty;
-        }
         // We need to use the root resource, e.g. AzureStorageResource instead of AzureBlobResource
         // Otherwise, we get the wrong values for IsContainer and Name
         var target = endpointReference.Resource.GetRootResource();
 
-        bool HasBothHostAndPort() =>
-            _endpointUsage[endpointUniqueName].HasHost &&
-            _endpointUsage[endpointUniqueName].HasPort;
-
-        return (property, target.IsContainer(), HasBothHostAndPort()) switch
+        return (property, target.IsContainer()) switch
         {
             // If Container -> Container, we go directly to the container name and target port, bypassing the host
-            // But only do this if we have processed both the host and port properties for that same endpoint.
-            // This allows the host and port to be handled in a unified way.
-            (EndpointProperty.Host or EndpointProperty.IPV4Host, true, true) => target.Name,
-            (EndpointProperty.Port, true, true) => await endpointReference.Property(EndpointProperty.TargetPort).GetValueAsync(cancellationToken).ConfigureAwait(false),
+            (EndpointProperty.Host or EndpointProperty.IPV4Host, true) => target.Name,
+            (EndpointProperty.Port, true) => await endpointReference.Property(EndpointProperty.TargetPort).GetValueAsync(cancellationToken).ConfigureAwait(false),
             // If Container -> Exe, we need to go through the container host
-            (EndpointProperty.Host or EndpointProperty.IPV4Host, false, _) => containerHostName,
-            (EndpointProperty.Url, _, _) => string.Format(CultureInfo.InvariantCulture, "{0}://{1}:{2}",
+            (EndpointProperty.Host or EndpointProperty.IPV4Host, false) => containerHostName,
+            (EndpointProperty.Url, _) => string.Format(CultureInfo.InvariantCulture, "{0}://{1}:{2}",
                                             endpointReference.Scheme,
                                             await EvalEndpointAsync(endpointReference, EndpointProperty.Host).ConfigureAwait(false),
                                             await EvalEndpointAsync(endpointReference, EndpointProperty.Port).ConfigureAwait(false)),
-            (EndpointProperty.HostAndPort, _, _) => string.Format(CultureInfo.InvariantCulture, "{0}:{1}",
+            (EndpointProperty.HostAndPort, _) => string.Format(CultureInfo.InvariantCulture, "{0}:{1}",
                                             await EvalEndpointAsync(endpointReference, EndpointProperty.Host).ConfigureAwait(false),
                                             await EvalEndpointAsync(endpointReference, EndpointProperty.Port).ConfigureAwait(false)),
             _ => await endpointReference.Property(property).GetValueAsync(cancellationToken).ConfigureAwait(false)
@@ -159,8 +113,8 @@ async Task<ResolvedValue> ResolveConnectionStringReferenceAsync(ConnectionString
         // so we need to do the same here.
         var value = await ResolveInternalAsync(cs.Resource.ConnectionStringExpression).ConfigureAwait(false);
 
-        // While pre-processing the endpoints, we never throw
-        if (!Preprocess && string.IsNullOrEmpty(value.Value) && !cs.Optional)
+        // Throw if the connection string is required but not present
+        if (string.IsNullOrEmpty(value.Value) && !cs.Optional)
         {
             cs.ThrowConnectionStringUnavailableException();
         }
@@ -188,12 +142,6 @@ async ValueTask<ResolvedValue> ResolveInternalAsync(object? value)
     static async ValueTask<ResolvedValue> ResolveWithContainerSourceAsync(IValueProvider valueProvider, string containerHostName, bool sourceIsContainer, CancellationToken cancellationToken)
     {
         var resolver = new ExpressionResolver(containerHostName, cancellationToken, sourceIsContainer);
-
-        // Run the processing phase to know if the host and port properties are both used for each endpoint.
-        resolver.Preprocess = true;
-        await resolver.ResolveInternalAsync(valueProvider).ConfigureAwait(false);
-        resolver.Preprocess = false;
-
         return await resolver.ResolveInternalAsync(valueProvider).ConfigureAwait(false);
     }
 

tests/Aspire.Hosting.Tests/ExpressionResolverTests.cs

@@ -63,15 +63,15 @@ public record ExpressionResolverTestData(bool SourceIsContainer, IValueProvider
     [InlineData("Url2", true, false, "Url=http://ContainerHostName:12345;")]
     [InlineData("Url2", true, true, "Url=http://testresource:10000;")]
     [InlineData("OnlyHost", true, false, "Host=ContainerHostName;")]
-    [InlineData("OnlyHost", true, true, "Host=localhost;")] // host not replaced since no port
+    [InlineData("OnlyHost", true, true, "Host=testresource;")] // host now replaced to container name
     [InlineData("OnlyPort", true, false, "Port=12345;")]
-    [InlineData("OnlyPort", true, true, "Port=12345;")] // port not replaced since no host
+    [InlineData("OnlyPort", true, true, "Port=10000;")] // port now replaced with target port
     [InlineData("HostAndPort", true, false, "HostPort=ContainerHostName:12345")]
     [InlineData("HostAndPort", true, true, "HostPort=testresource:10000")] // host not replaced since no port
     [InlineData("PortBeforeHost", true, false, "Port=12345;Host=ContainerHostName;")]
     [InlineData("PortBeforeHost", true, true, "Port=10000;Host=testresource;")]
     [InlineData("FullAndPartial", true, false, "Test1=http://ContainerHostName:12345/;Test2=https://localhost:12346/;")]
-    [InlineData("FullAndPartial", true, true, "Test1=http://testresource:10000/;Test2=https://localhost:12346/;")] // Second port not replaced since host is hard coded
+    [InlineData("FullAndPartial", true, true, "Test1=http://testresource:10000/;Test2=https://localhost:10001/;")]
     public async Task ExpressionResolverGeneratesCorrectEndpointStrings(string exprName, bool sourceIsContainer, bool targetIsContainer, string expectedConnectionString)
     {
         var builder = DistributedApplication.CreateBuilder();

tests/Aspire.Hosting.Tests/WithEnvironmentTests.cs

@@ -242,7 +242,7 @@ public async Task EnvironmentVariableExpressions()
 
         Assert.Equal(4, config.Count);
         Assert.Equal($"http://container1:10005/foo", config["URL"]);
-        Assert.Equal("90", config["PORT"]);
+        Assert.Equal("10005", config["PORT"]);
         Assert.Equal("10005", config["TARGET_PORT"]);
         Assert.Equal("connectionString;name=1", config["HOST"]);