Add support for using Digest for base image

[BeyondEvil]

Add support for using image digest (sha256) for the base image when using the PublishContainer target.

For example:
mcr.microsoft.com/dotnet/aspnet@sha256:6cec36412a215aad2a033cfe259890482be0a1dcb680e81fccc393b2d4069455

Closes: dotnet/sdk-container-builds#448

[BeyondEvil]

Open question:

Currently you can supply both Tag and Digest, and if you do, Digest takes precedence.

We can leave it as is, or warn when supplying both.

[BeyondEvil]

@dotnet-policy-service agree company="Cosafe Technology AB"

[BeyondEvil]

I don't fully understand the CI errors but I doubt it's related to my changes. Perhaps it's a permissions error because I'm working on a fork? 🤔 🤷‍♂️

[baronfel]

This is great so far - kudos for diving in to a new codebase and doing everything almost perfectly!

Open question:

Currently you can supply both Tag and Digest, and if you do, Digest takes precedence.

We can live it as is, or warn when supplying both.

I would prefer creating a warning. This warning should be raised in the ParseContainerProperties task and warn when providing both versions. The Warning should

• have a distinct Code so that users can NoWarn it individually
• Recommend that users only specify Digest if both are specified

When new messages/warnings of any kind are added, we need to create them using .NET Resource strings. To do this, you create a new entry in the Strings.resx file and then trigger a build via ./build.cmd - this should cause the build tooling to create the matching C# values in Strings.Designer.cs - from there you can detect the condition in the ParseContainerProperties task and use LogWarningWithCodeFromResources to actually create the MSBuild warning.

I don't fully understand the CI errors but I doubt it's related to my changes. Perhaps it's a permissions error because I'm working on a fork? 🤔 🤷‍♂️

This is indeed just flaky CI - rerunning those legs or pushing new commits will probably clear them up.

[BeyondEvil]

This is great so far - kudos for diving in to a new codebase and doing everything almost perfectly!

Thank you, but you're giving me too much credit. 😅 I basically just tried to follow the same pattern as Tag. 😊

Open question:
Currently you can supply both Tag and Digest, and if you do, Digest takes precedence.
We can live it as is, or warn when supplying both.

I would prefer creating a warning. This warning should be raised in the ParseContainerProperties task and warn when providing both versions. The Warning should

• have a distinct Code so that users can NoWarn it individually
• Recommend that users only specify Digest if both are specified

When new messages/warnings of any kind are added, we need to create them using .NET Resource strings. To do this, you create a new entry in the Strings.resx file and then trigger a build via ./build.cmd - this should cause the build tooling to create the matching C# values in Strings.Designer.cs - from there you can detect the condition in the ParseContainerProperties task and use LogWarningWithCodeFromResources to actually create the MSBuild warning.

Would you mind choosing a code for me to use (for the NoWarn)? 😅 🙏

I don't fully understand the CI errors but I doubt it's related to my changes. Perhaps it's a permissions error because I'm working on a fork? 🤔 🤷‍♂️

This is indeed just flaky CI - rerunning those legs or pushing new commits will probably clear them up.

Got it, thanks! :)

I'll add a test for the warning as well.

Any other test you think I should add?

[baronfel]

Would you mind choosing a code for me to use (for the NoWarn)? 😅 🙏

The parse-related and validation-related errors are the 2xxx-series, so maybe CONTAINER2031 would be my suggestion. That's coming from looking at the codes that are in the error messages in Strings.resx already and seeing what's next, nothing special here.

[baronfel]

Any other test you think I should add?

It would be great to verify that a base image could be used by digest - in the EndToEnd tests, we pre-populate a few images by Tag. If we could get the Digest values for one or more of those and run an end-to-end publishing test using the Digest value of one of the pre-populated images that would be a good test.

[BeyondEvil]

Any other test you think I should add?

It would be great to verify that a base image could be used by digest - in the EndToEnd tests, we pre-populate a few images by Tag. If we could get the Digest values for one or more of those and run an end-to-end publishing test using the Digest value of one of the pre-populated images that would be a good test.

Any pointers to code here would be greatly appreciated! 🙏

Are these images public? Meaning, can I query/pull them and get the SHA?

[baronfel]

Any pointers to code here would be greatly appreciated! 🙏

Are these images public? Meaning, can I query/pull them and get the SHA?

Sure - a test I would look at is EndToEnd_NoAPI_Console() - The gist of this test (once you get past the setup) is publishing a console app using a dotnet/runtime base image tag.. These tests run against a locally-running Docker registry that is prepopulated in this test setup fixture - in this setup we pull various .NET images from the public mcr.microsoft.com registry. During this process we could grab and store the Digest of the same image that's used in the EndToEnd_NoAPI_Console() test and run the same test but with a ContainerBaseImage that used the digest instead of the tag.

Does that make sense?

[tmds]

@BeyondEvil can you please add additional cases to

sdk/test/Microsoft.NET.Build.Containers.UnitTests/ContainerHelpersTests.cs

Line 43 in 88ef730

public void TryParseFullyQualifiedContainerName(string fullyQualifiedName, bool expectedReturn, string? expectedRegistry, string? expectedImage, string? expectedTag, bool expectedIsRegistrySpecified)

for:

• a name that includes a digest
• a name that includes a tag and a digest

[tmds]

I would prefer creating a warning. This warning should be raised in the ParseContainerProperties task and warn when providing both versions. The Warning should

@baronfel the format of including both a tag and a digest is not uncommon. This format enables to express what tag to follow while still being in control when to sync when the tag changes reference. It is used by tools like https://github.com/renovatebot/renovate.

podman (and I assume docker does the same) doesn't issue a warning and just ignores the tag.

For these reasons, I think the .NET SDK should simply ignore the tag and not issue a warning.

[baronfel]

@tmds thanks for bringing the details from other tools. This is enough to not do the warning, and not do the from tag/from digest work either.

[BeyondEvil]

So to summarize, I should only add the missing tests?

[BeyondEvil]

Any pointers to code here would be greatly appreciated! 🙏
Are these images public? Meaning, can I query/pull them and get the SHA?

Sure - a test I would look at is EndToEnd_NoAPI_Console() - The gist of this test (once you get past the setup) is publishing a console app using a dotnet/runtime base image tag.. These tests run against a locally-running Docker registry that is prepopulated in this test setup fixture - in this setup we pull various .NET images from the public mcr.microsoft.com registry. During this process we could grab and store the Digest of the same image that's used in the EndToEnd_NoAPI_Console() test and run the same test but with a ContainerBaseImage that used the digest instead of the tag.

Does that make sense?

Maybe 😅

I'll implement what I think you're after and we can take it from there. 😊

[BeyondEvil]

I'm guessing this is the reason why the test is failing: https://github.com/BeyondEvil/dotnet-sdk/blob/beyondevil/support-image-digest/src/Containers/Microsoft.NET.Build.Containers/Tasks/CreateNewImage.Interface.cs#L37

Can I just remove it?

Doesn't this conflict with the above: https://github.com/BeyondEvil/dotnet-sdk/blob/beyondevil/support-image-digest/src/Containers/Microsoft.NET.Build.Containers/ContainerHelpers.cs#L167

I should probably remove this comment: https://github.com/BeyondEvil/dotnet-sdk/blob/beyondevil/support-image-digest/src/Containers/Microsoft.NET.Build.Containers/ContainerHelpers.cs#L172

When I tried using this in my own project:

  <PropertyGroup>
    <ContainerBaseImage>mcr.microsoft.com/dotnet/aspnet</ContainerBaseImage>
  </PropertyGroup>

I got the same error as the test. So the tag is required.

Should I update the logic to require either tag or digest? If so, where would I put that? In the parser or CreateNewImage?

Also, I'm having some trouble running these tests locally. I'm on an M3 Mac.

Would appreciate some guidance on all of the above. 😅 🙏

[tmds]

Should I update the logic to require either tag or digest?

If neither are specified, we could default to a tag of latest. @baronfel wdyt?

[BeyondEvil]

Should I update the logic to require either tag or digest?

If neither are specified, we could default to a tag of latest. @baronfel wdyt?

In my, perhaps not-so-humble, opinion - the latest tag is an abomination. 😅 😆

It's fine if the resulting image, the output, is tagged with latest if an explicit tag is not provided. This is the default behaviour of Docker in most instances (like docker build).

However for inputs, as the base image, this will lead to non-deterministic builds where one day your base image is bumped to a major version with breaking changes leading to difficult to find issues.

Ask me how I know. 😅

With all that said, it seems like the command line argument defaults to latest: https://github.com/BeyondEvil/dotnet-sdk/blob/beyondevil/support-image-digest/src/Containers/containerize/ContainerizeCommand.cs#L35

[tmds]

Your argument is too use digests instead of tags for determinism. It isn't specific tolatest.

[BeyondEvil]

Your argument is too use digests instead of tags for determinism. It isn't specific tolatest.

Not sure what you mean 🤔

If I have tagged the base image with say 8.0 it's less likely upstream will re-tag 8.0 with breaking changes than the latest tag being moved from 8.0 to 9.0 (with possible breaking changes).

But maybe I misunderstood your point?

But you are correct. For guaranteed determinism, digests should be used. As recommended by docker: https://docs.docker.com/build/building/best-practices/#pin-base-image-versions

[tmds]

For guaranteed determinism, digests should be used. As recommended by docker: https://docs.docker.com/build/building/best-practices/#pin-base-image-versions

Yes, this is what I meant and we have the same opinion on this.

For UX of the SDK, I think good to follow the behavior of docker/podman. For base image references, they use latest when no tag is specified.

[BeyondEvil]

For guaranteed determinism, digests should be used. As recommended by docker: https://docs.docker.com/build/building/best-practices/#pin-base-image-versions

Yes, this is what I meant and we have the same opinion on this.

For UX of the SDK, I think good to follow the behavior of docker/podman. For base image references, they use latest when no tag is specified.

Fair enough.

Is this the correct placement of a default value? https://github.com/BeyondEvil/dotnet-sdk/blob/beyondevil/support-image-digest/src/Containers/Microsoft.NET.Build.Containers/Tasks/CreateNewImage.Interface.cs#L192

[tmds]

I'm sharing my opinion. @baronfel should make the the actual decision on the UX for the tag.

Is this the correct placement of a default value? https://github.com/BeyondEvil/dotnet-sdk/blob/beyondevil/support-image-digest/src/Containers/Microsoft.NET.Build.Containers/Tasks/CreateNewImage.Interface.cs#L192

Or perhaps in ParseContainerProperties.cs.

[baronfel]

@tmds is correct - if no Tag or Digest is specified for an image, we should default to latest. We also do this default when creating new images because of the expectation users have from docker and podman. I believe we currently do this defaulting in ParseContainerProperties as @tmds mentioned - that way CreateNewImage can assume that it has a valid input.

[BeyondEvil]

@tmds is correct - if no Tag or Digest is specified for an image, we should default to latest. We also do this default when creating new images because of the expectation users have from docker and podman. I believe we currently do this defaulting in ParseContainerProperties as @tmds mentioned - that way CreateNewImage can assume that it has a valid input.

Does that mean I should move the default from the interface to the parser?

Is there a ”props” way of doing it or should I do it in logic/code? E.g. if empty assign it?

[baronfel]

@BeyondEvil after a quick look it seems I was wrong - we don't default to latest for base images today, but we should and it should happen in the ParseContainerProperties. I think the simplest way to do this would be to change the following line from null-coalescing to "" to instead null-coalesce to "latest":

sdk/src/Containers/Microsoft.NET.Build.Containers/Tasks/ParseContainerProperties.cs

Line 165 in 3c685aa

ParsedContainerTag = outputTag ?? "";

This will ensure that after ParseContainerProperties we always have a Tag and/or a Digest.

Edit: hah, and it looks like you found the right place already.

[BeyondEvil]

I think since everything that has been discussed has been adressed and tests are passing, I'm setting the PR to Ready.

Should I also ping domestic-cat? 🤔

Thank you for all the help! 🙇

[baronfel]

No, domestic-cat is the SDK team's infrastructure rotation. For PRs like this you'd tag the feature area owners - in this case dotnet/sdk-container-builds-maintainers like you already have.

This is a great PR, thanks again for the initiative! I'll take a look and do PM approvals, but I'd still like some of the engineering team to take a look as well.

[BeyondEvil]

No, domestic-cat is the SDK team's infrastructure rotation. For PRs like this you'd tag the feature area owners - in this case dotnet/sdk-container-builds-maintainers like you already have.

I see 👍 😊

This is a great PR, thanks again for the initiative! I'll take a look and do PM approvals, but I'd still like some of the engineering team to take a look as well.

Sounds good, let me know if and when you need anything else from me. 🙌