k8s first pass

[prom3theu5]

no ingress support directly yet

Checklist

• Is this feature complete?
  • Yes. Ready to ship.
  • No. Follow-up changes expected.
• Are you including unit tests for the changes and scenario tests if relevant?
  • Yes
  • No
• Did you add public API?
  • Yes
    • If yes, did you have an API Review for it?
      • Yes
      • No
    • Did you add <remarks /> and <code /> elements on your triple slash comments?
      • Yes
      • No
  • No
• Does the change make any security assumptions or guarantees?
  • Yes
    • If yes, have you done a threat model and had a security review?
      • Yes
      • No
  • No
• Does the change require an update in our Aspire docs?
  • Yes
    • Is this introducing a breaking change?
      • Yes
        • Link to aspire-docs issue (please use this breaking-change template):
      • No
        • Link to aspire-docs issue (please use this doc-idea template):
  • No

[davidfowl]

Should this be the name of the application instead of aspire?

[prom3theu5]

yeah! I just defaulted it to aspire, I thought the cli would probably pass it through

[prom3theu5]

Gonna have to rework the unit tests - they work in xunit in my ide (Rider), but fail through dotnet test ran from cli. Strange...
I'll remove the embedded resources tomorrow, and extract them into string constants like the compose test does.
Will also remove the reusable method and just test each file directly, as its too hard to see which file actually fails there

[davidfowl]

This is looking really promising @prom3theu5 !

[prom3theu5]

@davidfowl you ok with the dual context? (One for publisher, one for the state of a processed resource) There were so many cross cutting concerns in the single context felt better to split them into two and isolate responsibilities

[davidfowl]

Yep! it looks much cleaner.

[davidfowl]

What port is this supposed to be? The internal port? I think you can only choose a default for project resources. Otherwise, this has to throw right? The container needs to be configured to listen on this port.

[prom3theu5]

The target port is returned, if something like sqlServer for example was configured, it should have its target port set as 1433 shouldn't it?
This was just the fall back for something that doesn't have a target port set, which probably means endpoint annotations on the resource are wrong
We can choose to only apply this for project resources, or else "explode" with an exception and end publishing?

[prom3theu5]

something like

 foreach (var endpoint in endpoints)
        {
            var internalPort = endpoint.TargetPort;

            if (resource is ProjectResource _)
            {
                internalPort ??= endpoint.UriScheme switch
                {
                    "http" => 8080,
                    "https" => 8443,
                    _ => throw new NotSupportedException($"Unsupported scheme: {endpoint.UriScheme}"),
                };
            }

            if (internalPort is null)
            {
                throw new InvalidOperationException("TargetPort for endpoint must be set");
            }

            EndpointMappings[endpoint.Name] = new(endpoint.UriScheme, resource.Name, internalPort.Value, internalPort.Value, endpoint.Name);
        }
        ```

[davidfowl]

Yea, either this or it becomes a parameter.

[prom3theu5]

there's also "TargetPortEnvironmentVariable" to take into account, It's internal atm - but we'd probably want to make it public here so we can see if a value has been set too?

[davidfowl]

That shouldn't be required. When it is set the correct data is expressed in the model as an environment variable:

aspire/src/Aspire.Hosting/ResourceBuilderExtensions.cs

Lines 566 to 569 in e5014c7

	builder.WithAnnotation(new EnvironmentCallbackAnnotation(context =>
	{
	context.EnvironmentVariables[env] = endpointReference.Property(EndpointProperty.TargetPort);
	}));

All we need to do is model endpoints appropriately and it should all hang together.

The answer here might be to avoid doing anything clever and using a parameter if the value is not set.

[prom3theu5]

I've just done something along those lines in this: ce22f8a

However - if you see the message in the commit, I'm having a little issue with separation of env vars.
As you linked above, if we set WithEndpoint(env: "Port"), this forces an EV callback to populate an EnvVar called Port.
But i don't see a way without having access to

to know that this env var value that has been specified in the AppHost is meant to be referenced by the endpoint. When its populated back into the ev - this "link" is lost if you are just interrogating the annotations

[prom3theu5]

Might just be tired and missing something obvious lol

[davidfowl]

Yea, you shouldn't need to do anything special to make it work. If you do:

builder.AddNpmApp("fe", "../app").WithHttpEndpoint(env: "PORT");

The "PORT" environment variable slot will have a reference expression to the target endpoint value.

I realize now that in the docker compose case we use the port allocator:

aspire/src/Aspire.Hosting.Docker/DockerComposePublishingContext.cs

Line 272 in b3cc78f

var internalPort = endpoint.TargetPort ?? composePublishingContext.PortAllocator.AllocatePort();

We could do the same here, or we could decide to both use the port allocator and to parameterize to set a default value.

aspire/src/Aspire.Hosting/Publishing/ManifestPublishingContext.cs

Lines 415 to 429 in e5014c7

	int? targetPort = (resource, endpoint.UriScheme, endpoint.TargetPort, endpoint.Port) switch
	{
	// The port was specified so use it
	(_, _, int target, _) => target,
	// Container resources get their default listening port from the exposed port.
	(ContainerResource, _, null, int port) => port,
	// Check whether the project view this endpoint as Default (for its scheme).
	// If so, we don't specify the target port, as it will get one from the deployment tool.
	(ProjectResource project, string uriScheme, null, _) when IsHttpScheme(uriScheme) && !httpSchemesEncountered.Contains(uriScheme) => null,
	// Allocate a dynamic port
	_ => PortAllocator.AllocatePort()
	};

We might need to revisit this logic in multiple places but lets treat this like a follow up.

[prom3theu5]

Didn't realise the EnvVar had a reference expression, missed that yesterday ^^
Definitely will be a follow up on this some time this week

I opted to not use port allocator here purely because ClusterIP this wouldn't be an issue - its only if we had a specific NodePort binding to the same port we'd run into an issue, but two pods in two separate deployments can use the same port no problem

[davidfowl]

DU! 😄 As a follow up I would just use object if this was internal storage.

[davidfowl]

Lots of follow ups but great start!