Support combined image and tags syntax in WithImage extension method

[MadL1me]

In many cases of using docker, we usually specify image + tag in one continious string in a following format: "{image}:{tag}", for example like postgres:14.3. This notation is used with direct docker command, as well as on dockerhub or in Dockerfiles and Docker-compose files, and it might be a little bit better to be able just to copy one string and specify it in extension method.

But current extension method specifies tag = "latest" directly, so you can't just set "postgres:14.3" in image. For me that was a surprise, and I've got runtime error:

fail: Aspire.Hosting.Dcp.dcpctrl.ContainerReconciler[0]
      could not create the container    {"Container": {"name":"database"}, "Reconciliation": 3, "error": "CreateContainer command returned error code 1 Stdout: '' Stderr: 'invalid reference format\n'"}

In my opinion, change of extension method will add a little bit to a better developer experience:

Before:

var builder = DistributedApplication.CreateBuilder(args);

builder.AddPostgres("database")
    .WithImage("postgres", "16.2")

builder.Build().Run();

After:

var builder = DistributedApplication.CreateBuilder(args);

builder.AddPostgres("database")
    .WithImage("postgres:16.2")
    .WithPgAdmin();

builder.Build().Run();

[mitchdenny]

If you are only changing the tag couldn't you just use WithImageTag?

[davidfowl]

I think we should support this. We'd be willing to take a PR if there was a sane way to parse the registry, image name and tag. @danegsta do you know if there's a spec? Or is this just docker magic?

[danegsta]

I’ve never found a formal spec, but the implementations for image format are consistent and stable between Docker and Podman, so we were able to put together a regex that handles all image tag parts in the vscode-docker extension: https://github.com/microsoft/vscode-docker-extensibility/blob/759aefe83ed6af599db22fd6c4db963ff40c6323/packages/vscode-container-client/src/utils/parseDockerLikeImageName.ts#L22

Should be pretty straightforward to repurpose that same regex/logic for Aspire.

[robertcoltheart]

I'll also comment here that there are valid registries eg. artifactory that require you to pass a port as part of the image name.

For example:

.WithImage("artifactory.my.org:9956/redis", "7.2.4") or .WithImage("artifactory.my.org:9956/redis")

both fail with the above error.

The workaround is to use .WithImageRegistry("artifactory.my.org:9956") however the WithImage syntax runtime error came as a surprise, much like the OP.

[davidfowl]

Marking this as a good first issue that we'll take a contribution for. The work looks like taking that regex and setting the right state based on the capture groups.

[afscrome]

This seems like similar to #4218 - same underlying issue, although with different entrypoints (AddContainer vs WithImage)

[MadL1me]

@davidfowl I'll take the issue to contribute

Is the flow similar to ASP.NET Core contributions? Do we need to decide on the API signatures before moving forward? Or I can just jump straight into PR? (cause it's pretty straighforward - just add an additional method with just only one string param)

Also, @afscrome targeted similar thing, I think we should change this for other similar functions as well

[davidfowl]

Is the flow similar to ASP.NET Core contributions? Do we need to decide on the API signatures before moving forward? Or I can just jump straight into PR? (cause it's pretty straighforward - just add an additional method with just only one string param)

Do we need a method? I think WithImage just parses the string and sets the other properties if it can.

[MadL1me]

Do we need a method? I think WithImage just parses the string and sets the other properties if it can.

The current signature looks like this:

IResourceBuilder<T> WithImage<T>(this IResourceBuilder<T> builder, string image, string tag = "latest") where T : ContainerResource

thing I don't like, is that, from user perspective it is not very intuitive - even if you passed image with correct format, the default second param for image tag still would be "latest". This is why I though about second method, but without tag param at all

[davidfowl]

Can you update the issue with the new API using this template

[afscrome]

Here's a stab with the API Template

Background and Motivation

Most places in the container ecosystem treat the image reference as a single string. For example, the docker run command:

docker run redis:7.2.5

Or a Kubernetes container spec:

apiVersion: v1
kind: Pod
metadata:
  name: sql
spec:
  containers:
  - name: sql
    image: mcr.microsoft.com/mssql/server:2022-latest

However if you try that in Aspire as follows:

IDistributedApplicationBuilder.AddContainer("mcr.microsoft.com/mssql/server:2022-latest");
IResourceBuilder<T>.WithImage("mcr.microsoft.com/mssql/server:2022-latest")

Not only does it fail, but it fails with an error quite a distance away from where you provided the bad image, and with an error that's not immediately clear why the reference is invalid

fail: Aspire.Hosting.Dcp.dcpctrl.ContainerReconciler[0] could not create the container {"Container": {"name":"database"}, "Reconciliation": 3, "error": "CreateContainer command returned error code 1 Stdout: '' Stderr: 'invalid reference format\n'"}

Aspire instead expects you to do the following

IDistributedApplicationBuilder
    .AddContainer("mssql/server", "2022-latest")
    .WithImageRegistry("mcr.microsoft.com")

Proposed API

ContainerResourceBuilderExtensions.WithImage

We should accept users passing a full image reference to WithImage, and break it out into the constituent parts,

namespace Aspire.Hosting;
public static class ContainerResourceBuilderExtensions
{
-        public static IResourceBuilder<T> WithImage<T>(this IResourceBuilder<T> builder, string image, string tag = "latest") where T : ContainerResource
+        public static IResourceBuilder<T> WithImage<T>(this IResourceBuilder<T> builder, string image) where T : ContainerResource
+        public static IResourceBuilder<T> WithImage<T>(this IResourceBuilder<T> builder, string image, string tag) where T : ContainerResource
}

The WithImage(string) method parse the input string, break it up into registry, image, tag (or sha256), and then use those to build up the ContainerImageAnnotation added to the resource. Using the rules described at https://github.com/dotnet/aspire/issues/4355#issuecomment-2147862676

What the WithImage(string,string) does has a couple of wrinkles - in particular what happens with the following

builder.WithImage("redis:7.2.5", "latest")
builder.WithImage("redis@sha256:f5ef9e24a9ef3b7cc552ae0cbc3cbade4f2877502683496c5d775605ae071412", "7.2.5")

Those overload are ambiguous as to what image should be referenced. My first thought of a solution here would be to limit the parsing of the image reference to WithImage(string), and leave ContainerResourceBuilderExtensions.WithImage(string,string) behaving exactly as it does today. However that then means the following would continue to fail with a cryptic error, even though we could resolve that unambiguously.

.WithImage("mcr.microsoft.com/mssql/server", "2022-latest")

That leaves us with two options

1. Parse the image as an image reference, and throw an error if there's ambiguity due to two tags, or a tag and a digest
2. Do no parsing of the image, and fail later on if an image reference is passed

I'm inclined to go for option 1, although that's not a particularly strongly held view.

IDistributedApplicationBuilder.AddContainer

No API signature changes should be required, however the AddContainer(string) and AddContainer(string,string) methods should follow the same rules as described above for WithImage.

This shouldn't be too problematic as AddContainer ultimately calls WithImage, but a change may be needed to how the calls chain together. Currently it goes AddContainer(string) --> AddContainer(string,string) --> WithImage(string,string), but that may need to become AddContainer(string) --> WithContainer(string)

Usage Examples

The following will continue to behave exactly as it does today

builder.WithImage("redis")

The following fail today, but after this proposal should work as if an equalivant chain of WithImage, WithImageRegistry and/or WithImageSHA256 calls were used:

// image & tag
builder.WithImage("redis:latest")
builder.WithImage("redis:7.2.5")

// image & digest
builder.WithImage("redis@sha256:f5ef9e24a9ef3b7cc552ae0cbc3cbade4f2877502683496c5d775605ae071412")

// registry & image
builder.WithImage("quay.io/prometheus/prometheus")

// registry, image & tag
builder.WithImage("mcr.microsoft.com/mssql/server:2022-latest")
builder.WithImage("localhost:1235/mcr.microsoft.com/mssql/server:2022-latest")

// registry, image & digest
builder.WithImage("mcr.microsoft.com/mssql/server@sha256:c4369c38385eba011c10906dc8892425831275bb035d5ce69656da8e29de50d8")

For each of the above examples, IDistributedApplicationBuilder.AddContainer should behave the same as th equilivant IResourceBuilder<T>.WithImage method

Alternative Designs

Handle Errors better

One solution to this problem is to simply improve the errors. Rather than failing later on with a cryptic error, far away from where the image was provided, we could fail much earlier. E.g fail in WithImage so the debugger breaks on the line of code where the user provided the image reference with an error along the lines of:

Image references not supported - break your image reference up into separate image and tag fields in addition to using WithContainerRegistry for any registry components.

However that doesn't feel like a great user experience, especially since there's a good chance that adding a container reference is one of the first things people do with aspire, presenting a bad first impression.

Analysers & Code Fixes

We could build some analysers and code fixes to detect this cases, but that feels like a large amount of work just to provide a worse developer experience. There are also likely to be many edge cases that our analysers don't cover. (e.g. when the image reference is built up dynamically)

Risks

Making an optional parameter no longer optional is a breaking change, however I believe the addition of the single string overload would get resolved in all source breaking changes that would have previously broken making this an acceptable change. If that's not the case, the string tag parameter could remain as an optional parameter.

[davidfowl]

Does anyone want to take this on? This would be a really nice QOL improvement (Especially if we could make all of the APIs compose nicely!);

[afscrome]

I can give this a go.

[afscrome]

I've got part of an implementation by adding a WithImageReference() (e.g. WithImageReference("myorg.azurecr.io/docker.io/library/redis:tag"). I've wired that up to AddContainer which works, but introduced a breaking change. I also tried replacing WithImage with WithImageReference but hit a bunch of problems that I think are more trouble than they're worth.

AddContainer() Breaking Change

If someone in 8.x called AddContainer() and the first segment of the image name contained a . or was the string localhost, AND WithImageRegistry() is later called, there is a breaking change

.AddContainer("mcr.microsoft.com/dotnet/aspire-dashboard")
.WithImageRegistry("mycompany.azurecr.io");
// Previously - mycompany.azurecr.io/mcr.microsoft.com/dotnet/aspire-dashboard
// Now  - mycompany.azurecr.io/dotnet/aspire-dashboard

However after this change, the first segment will be treated as a registry, and so will be replaced rather than prepended to by the subsequent WithImageRegistry call.

I don't think this a common case, and if it does break it should break clearly with an image not found error so should be relatively easy to fix. But by allowing AddContainer to work with full image references, it does make aspire much easier when starting for the first time.

Replacing WithImage() with WithImageReference()

I did try to plumb WithImageReference into WithImage() reference with WithImageReference() but it got gnarly quickly,

1. How to deal with ambiguity with WithImage("image:tag") and WithImage("image:tag", "tag2), further complicated by tag being a default paramaeter with a value of latest`.
2. Breaks symmetry with WithImageTag, WithImageSha256 and WithImageContainerRegistry which only modify part of the
3. How to handle `WithImageRegistry(registry).WithImage("image:foo"). As the image reference contains no registry portion, should that overwrite the registry, or supplement it?
4. How to handle WithImageTag("mcr.microsoft.com/dotnet/aspire-dashboard") - should mcr.microsot.com(this is much the same issue as the breaking change inAddContainer`)

Individually all these are solveable, but but all together get very gnarly and more trouble than I think is worth.

An alternate solution

An alternate solution to making this work is to allow the whole image to be smuggled in through ContainerImageAnnotation.Image. This works in 8.2 for container registries but fails when the reference contains tags or digets (i.e. image:tag becomes image:tag:latest which is illegal). The only reson the image becomes illegal is because we forcably set the tag to "latest" in WithImage() and AddContainer(), rather than leaving the tag null. It also makes the smuggling behaviour consistent - today you can already smuggle the registry portion, just not the tag or digest - with this proposal you can smuggle all portions through the image.

The upside here is it avoids any breaking changes and makes hte expected paths work. However it has the downside that ContainerImageAnnotation.ContainerRegistry can no longer be relied on, nor will WithImageTag or WithImageSha256 correctly mutate the image as they'll append an extra tag/digest rather than mutate.

Options

Given this, I think we have a few options

1. Do Nothing and close this issue and the linked Pr.
2. Introduce WithImageReference() but don't change AddContainer() to use it
3. Option 2 + wire WithImageReference up to AddContainer() (leaving WithImage() alone) (What I've implemented in #5835)
4. Option 3 + spend more time tyring to merge WithImageReference() into WithImage()
5. Default image- tags to null rather than "latest" to smuggle the whole reference
6. Some kind of larger change to ContainerImageAnnotation and/or WithImage*() model due to ambiguity as to whether mcr.microsoft.com is part of the hostname or path.

[danegsta]

Fortunately, because Docker Hub doesn’t allow periods in registry names, you can get away with assuming that if your image name starts with a valid domain (such as mcr.microsoft.com), that you’re dealing with a custom repository domain and not part of the registry name.

Here’s the regex that the VSCode Docker extension uses to parse an image name into component parts. The same logic can be applied to parse a registry name to determine if it includes additional optional parts.

[afscrome]

@danegsta yup, I used that regex with a small addition to handle ipv6 addresses - https://github.com/dotnet/aspire/blob/ea985ec4fd70b9eb8cb2f8c1a567372f5475c89c/src/Aspire.Hosting/Utils/ContainerReferenceParser.cs#L40

[davidfowl]

Maybe we make ImageReference a type for add overloads that take it. Not as convenient but less breaking?

builder.AddContainer("app", new ImageReference("myorg.azurecr.io/docker.io/library/redis:tag"));

[afscrome]

To me the main reason to make this change is to help a user, possibly using aspire for the first time, who may try to do the following.

var builder = DistributedApplication.CreateBuilder(args);
var nginx = builder.AddContainer("nginx", "nginx:1.27"); 
await builder.Build().RunAsync();

Since you can do docker pull nginx:1.27 it seems perfectly reasonable that builder.AddContainer("nginx", "nginx:1.27") should be equalivent. Adding an additional overload would help those who spot it, it does leave a pit of failure for those who miss it, or are not aware the overload is there.

If we wanted to maintain backwards compatibility, we could modify the parsing in AddContainer() to not parse out the registry and leave that as part of the image like happens today. It may not be 100% correct but it's no more wrong than things are today for people who passed the registry as part of the image. (WithImageReference however would continue to break the registry out, but that's fine as it's a new method)

Another option could be to give up on auto fixing this, and instead just throwing if the image contains : or @ with a clear error suggesting the use of WithImageTag() or WithImageSha256() - all such cases today will fail to start, so at least it points the user as to how to fix the problem rather than letting the app start up and then fail with a cryptic invalid reference format error.

[mitchdenny]

Even users not using Aspire for the first time do this 😅

Personally I would be happier with AddContainer(...) throwing if the image name includes a : character. I want to see the constituent parts of the container image broken out as early as possible.

What we could do here is add an analyzer which detects that the image name has a tag in it and create a compiler error - and possibly a fixer.

[danegsta]

I'd argue for a breaking change of marking WithImageTag and WithImageRegistry as deprecated and having WithImage always replace the existing ContainerImageAnnotation (only preserving an existing SHA256 if one is set). Then WithImage can parse the image reference without worrying about ambiguous behavior.

The only scenario that would behave differently with that change is if a user were to chain a WithImage method AFTER a WithImageTag/WithImageRegistry call in their builder. My anecdotal evidence of that not being a big deal is that searching github code for WithImageTag and WithImageRegistry usage, the only example I could find where WithImage was chained after one of the other methods was when WithImageRegistry is explicitly being set to a null value.

That'd give this behavior:

Call	Registry	Name	Tag
AddContainer("nginx", "nginx:1.27")	N\A	nginx	1.27
AddContainer("nginx", "nginx")	N\A	nginx	latest
AddContainer("nginx", "mcr.microsoft.com/dotnet/sdk:9.0")	mcr.microsoft.com	dotnet/sdk	9.0
AddContainer("nginx", "mcr.microsoft.com/dotnet/sdk:9.0") .WithImage("nginx")	N\A	nginx	latest
AddContainer("nginx", "mcr.microsoft.com/dotnet/sdk:9.0") .WithImage("nginx:1.27")	N\A	nginx	1.27

[mitchdenny]

Throwing an exception when someone has an image tag where the image name isn't ambiguous :) We seem to have shifted from making an affordance in the API for folks that just want to paste image:tag to breaking an API surface. Seems drastic for such a minor inconvenience (if you could even call it that).

[danegsta]

I’m suggesting an exception if the user specifies something like WithImage(“nginx:1.27”, “latest”) where they’ve provided conflicting tags. The current APIs are inconsistent with how they treat image references, which is why I think there’d be value in cleaning up and simplifying the API.

We treat the registry as part of the image name, unless you use WithImageRegistry, in which case we don’t. In mcr.microsoft.com/dotnet/aspnet, for example, mcr.microsoft.com is the image registry, but we allow the user to specify it as part of the image name. But we also allow explicitly setting a registry that is stored in a separate property on the annotation.

Any image that isn’t stored in the Docker Hub registry requires a registry as part of the image name (ACR images, for example), so it’s fairly common to have to specify a registry. I’d just like to see us handle image references in a more consistent and predictable way where a user could copy an image reference and have it just work.

I don’t think you necessarily have to get rid of the WithImageRegistry or WithImageTag extensions so long as WithImage always replaces both the tag and registry values. The problem is, as @afscrome called out above, the OCI spec for images allows domain like image names, so you can’t necessarily differentiate a registry from a domain like image name if a registry is already set. If WithImage always overrides the registry in addition to the name and tag, that’s not really an issue anymore.

[afscrome]

WithImage method AFTER a WithImageTag/WithImageRegistry call in their builder

I was going to give registry mirrors as a counter point of a case where you'll want to call WithImage or WithImageRegistry after a WithImageTag call. However I then realized that as this is being done in a startup hook, you don't have access to the WithImage* methods, so had to manipulate ContainerImageAnnotation directly. Meaning the behavior of the WithImage* method is irrelevant here.

    public Task BeforeStartAsync(DistributedApplicationModel appModel, CancellationToken cancellationToken = default)
    {
        foreach (var container in appModel.GetContainerResources().SelectMany(x => x.Annotations.OfType<ContainerImageAnnotation>()))
        {
            var registry = container.Registry ?? "docker.io";

            if (!registry.EndsWith("azurecr.io", StringComparison.OrdinalIgnoreCase))
            {
                container.Registry = "myorg.azurecr.io";
                container.Image = $"{registry}/{container.Image}";;
            }
        }

        return Task.CompletedTask;
    }

[danegsta]

I was thinking about it a bit more and chatted some with @mitchdenny; if WithImage is updated to properly parse and set all the properties for a ContainerImageAnnotation (including parsing out and setting the Registry property or the tag if specified) it would be a solid improvement that should behave almost exactly the same except for a very small edge case (which I'd argue counts as a bug fix). Calling WithImage after WithImageRegistry would override the Registry value again, which doesn't happen currently.

WithImageRegistry should actually be more useful after the change since you could easily set a mirror registry for images that aren't hosted on Docker Hub. Currently if you did WithImage("myimage", "mcr.microsoft.com/dotnet/aspnet", "9.0").WithImageRegistry("myregistry.azurecr.io") we'd try to use an image like "myregistry.azurecr.io/mcr.microsoft.com/dotnet/aspnet:9.0" when it should be "myregistry.azurecr.io/dotnet/aspnet:9.0".

[afscrome]

Based on the feedback from #5835, trying to split out the image registry from the image is causing some complications leading to the PR being unmergeable.

I think I'm going to abandon that PR, and have another go at one that narrowly targets the case of .WithImage("postgres:16.2"), and have it parse out the tag (or digests) from that. I think that is the change that will bring the most value of simplifying the new developer experience, and it has minimal risk around breaking changes as right now including the tag leads to a guaranteed error.

We can then look at adding the image registry parsing on as a follow up issue / PR.

My goal is to make it such that .AddContainer("image:tag") and .WithImage("image:tag") behave exactly like .AddContainer("image:tag") and .WithImage("image:tag") respectively. Same for digests, although if it gets complicated I'd happly shed their support as most of hte value

I expect to be able to do that within the existing API surface, although there is the possibility of needing to split WithImage into two. I think I'll get away without that, but it may be needed.

namespace Aspire.Hosting;
public static class ContainerResourceBuilderExtensions
{
-        public static IResourceBuilder<T> WithImage<T>(this IResourceBuilder<T> builder, string image, string tag = "latest") where T : ContainerResource
+        public static IResourceBuilder<T> WithImage<T>(this IResourceBuilder<T> builder, string image) where T : ContainerResource
+        public static IResourceBuilder<T> WithImage<T>(this IResourceBuilder<T> builder, string image, string tag) where T : ContainerResource
}

[davidfowl]

Thanks @afscrome ! Lets see if there are any unexpected regressions!