Add support for using Digest for base image #44461

BeyondEvil · 2024-10-26T11:46:53Z

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 · 2024-10-26T12:03:54Z

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 · 2024-10-26T17:27:03Z

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

BeyondEvil · 2024-10-26T17:31:20Z

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? 🤔 🤷‍♂️

src/Containers/Microsoft.NET.Build.Containers/SourceImageReference.cs

baronfel · 2024-10-26T17:59:36Z

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 · 2024-10-26T18:18:41Z

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 · 2024-10-26T18:22:49Z

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 · 2024-10-26T18:32:10Z

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 · 2024-10-26T18:36:31Z

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 · 2024-10-26T18:42:31Z

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 · 2024-10-26T18:55:55Z

@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 · 2024-10-26T19:20:53Z

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 · 2024-10-26T19:24:57Z

@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 · 2024-10-26T20:30:38Z

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

BeyondEvil · 2024-10-26T20:32:16Z

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 · 2024-10-27T00:00:10Z

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 · 2024-10-27T06:17:41Z

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 · 2024-10-27T10:21:13Z

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 · 2024-10-27T10:32:54Z

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

BeyondEvil · 2024-10-27T10:36:04Z

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 · 2024-10-27T11:03:36Z

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 · 2024-10-27T12:05:03Z

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

baronfel · 2024-12-19T15:50:39Z

Looks like there was just one place missing a Digest. I've added that, let's see what CI says.

baronfel · 2024-12-19T22:03:46Z

@marcpopMSFT mind merging on red for the arm64 macos leg?

marcpopMSFT · 2024-12-20T00:20:39Z

@marcpopMSFT mind merging on red for the arm64 macos leg?

@baronfel arm64 leg should no longer be required. There are merge conflicts though so can't be merged.

BeyondEvil · 2024-12-20T00:30:52Z

Rebased again 👍

baronfel · 2024-12-20T00:48:16Z

@BeyondEvil looks like a bad merge on the EndToEndTests, can you take a look?

BeyondEvil · 2024-12-20T00:53:03Z

Sorry about that, it slipped by me somehow 🤔

Should be fixed now.

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`

BeyondEvil · 2024-12-20T09:01:12Z

Not sure what the failures are @baronfel

[xUnit.net 00:00:09.52] Microsoft.NET.Build.Containers.IntegrationTests: ❌Unable to find image 'registry:2' locally
[xUnit.net 00:00:10.50] Microsoft.NET.Build.Containers.IntegrationTests: ❌docker: Error response from daemon: toomanyrequests: You have reached your pull rate limit. You may increase the limit by authenticating and upgrading: https://www.docker.com/increase-rate-limit.
[xUnit.net 00:00:10.50] Microsoft.NET.Build.Containers.IntegrationTests: ❌See 'docker run --help'.

Is the rate limit a red herring?

baronfel · 2024-12-20T15:03:37Z

@BeyondEvil yeah - that particular one has nothing to do with your change, we have a backlog item to solve it permanently. Just trying to get through some more arm64 flakiness now...

baronfel · 2024-12-20T16:15:33Z

/backport to release/9.0.2xx

github-actions · 2024-12-20T16:15:45Z

Started backporting to release/9.0.2xx: https://github.com/dotnet/sdk/actions/runs/12434749671

github-actions · 2024-12-20T16:16:05Z

@baronfel backporting to release/9.0.2xx failed, the patch most likely resulted in conflicts:

$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch

Applying: Add support for using Digest for base image
Using index info to reconstruct a base tree...
A	src/Containers/Microsoft.NET.Build.Containers/PublicAPI/net10.0/PublicAPI.Unshipped.txt
M	test/Microsoft.NET.Build.Containers.IntegrationTests/CreateNewImageTests.cs
M	test/Microsoft.NET.Build.Containers.IntegrationTests/EndToEndTests.cs
Falling back to patching base and 3-way merge...
Auto-merging test/Microsoft.NET.Build.Containers.IntegrationTests/EndToEndTests.cs
CONFLICT (content): Merge conflict in test/Microsoft.NET.Build.Containers.IntegrationTests/EndToEndTests.cs
Auto-merging test/Microsoft.NET.Build.Containers.IntegrationTests/CreateNewImageTests.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 Add support for using Digest for base image
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

…et#44461) Co-authored-by: Chet Husk <chusk3@gmail.com>

dotnet-issue-labeler bot added Area-Containers Related to dotnet SDK containers functionality untriaged Request triage from a team member labels Oct 26, 2024

BeyondEvil force-pushed the beyondevil/support-image-digest branch from 2842ab1 to 7f70916 Compare October 26, 2024 11:59

BeyondEvil mentioned this pull request Oct 26, 2024

Support explicitly specifying an image by SHA digest, not just tag dotnet/sdk-container-builds#448

Closed

build-analysis bot mentioned this pull request Oct 26, 2024

error MSB3231: Unable to remove directory #42963

Closed

baronfel reviewed Oct 26, 2024

View reviewed changes

src/Containers/Microsoft.NET.Build.Containers/SourceImageReference.cs Outdated Show resolved Hide resolved

BeyondEvil force-pushed the beyondevil/support-image-digest branch from 7f70916 to 9f6970f Compare October 26, 2024 22:44

BeyondEvil force-pushed the beyondevil/support-image-digest branch from 9f6970f to 83e5984 Compare October 27, 2024 12:03

BeyondEvil force-pushed the beyondevil/support-image-digest branch from 83e5984 to 0a0a8b8 Compare October 27, 2024 12:08

baronfel approved these changes Dec 19, 2024

View reviewed changes

build-analysis bot mentioned this pull request Dec 19, 2024

Failure on Helix tests - Tests timing out #40074

Open

BeyondEvil force-pushed the beyondevil/support-image-digest branch from b407d79 to 13c7cc1 Compare December 20, 2024 00:27

BeyondEvil force-pushed the beyondevil/support-image-digest branch from 13c7cc1 to 03cb156 Compare December 20, 2024 00:51

BeyondEvil added 3 commits December 20, 2024 01:53

Add support for using Digest for base image

d0917b8

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`

Alternative location for default tag

c8cbf63

Refactor SourceImageReference

0324adb

BeyondEvil force-pushed the beyondevil/support-image-digest branch 2 times, most recently from cbb0830 to 21d5505 Compare December 20, 2024 01:14

add Digest null value to test scenario that was missed

e95d9ee

BeyondEvil force-pushed the beyondevil/support-image-digest branch from 21d5505 to e95d9ee Compare December 20, 2024 01:17

This was referenced Dec 20, 2024

The active test run was aborted. Reason: Test host process crashed dotnet/dnceng#451

Open

Microsoft.NET.Build.Containers.IntegrationTests are pulling from Docker Hub #42026

Open

baronfel merged commit 0ee30b8 into dotnet:main Dec 20, 2024
34 of 37 checks passed

baronfel added a commit to baronfel/sdk that referenced this pull request Dec 20, 2024

Add support for using a pinned Digest for Container base images (dotn…

6acc5b3

…et#44461) Co-authored-by: Chet Husk <chusk3@gmail.com>

baronfel mentioned this pull request Dec 20, 2024

Backport #44461 to release/9.0.2xx #45594

Open

BeyondEvil deleted the beyondevil/support-image-digest branch December 20, 2024 16:34

This was referenced Dec 20, 2024

slow macOS - "##[error]The job running on agent Azure Pipelines 9 ran longer than the maximum time of 60 minutes." dotnet/dnceng#1883

Open

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for using Digest for base image #44461

Add support for using Digest for base image #44461

BeyondEvil commented Oct 26, 2024 •

edited

Loading

BeyondEvil commented Oct 26, 2024 •

edited

Loading

BeyondEvil commented Oct 26, 2024

BeyondEvil commented Oct 26, 2024

baronfel commented Oct 26, 2024

BeyondEvil commented Oct 26, 2024

baronfel commented Oct 26, 2024

baronfel commented Oct 26, 2024

BeyondEvil commented Oct 26, 2024

baronfel commented Oct 26, 2024

tmds commented Oct 26, 2024

tmds commented Oct 26, 2024

baronfel commented Oct 26, 2024

BeyondEvil commented Oct 26, 2024

BeyondEvil commented Oct 26, 2024

BeyondEvil commented Oct 27, 2024 •

edited

Loading

tmds commented Oct 27, 2024

BeyondEvil commented Oct 27, 2024 •

edited

Loading

tmds commented Oct 27, 2024

BeyondEvil commented Oct 27, 2024 •

edited

Loading

tmds commented Oct 27, 2024

BeyondEvil commented Oct 27, 2024

baronfel commented Dec 19, 2024

baronfel commented Dec 19, 2024

marcpopMSFT commented Dec 20, 2024

BeyondEvil commented Dec 20, 2024

baronfel commented Dec 20, 2024

BeyondEvil commented Dec 20, 2024

BeyondEvil commented Dec 20, 2024

baronfel commented Dec 20, 2024

baronfel commented Dec 20, 2024

github-actions bot commented Dec 20, 2024

github-actions bot commented Dec 20, 2024

Add support for using Digest for base image #44461

Add support for using Digest for base image #44461

Conversation

BeyondEvil commented Oct 26, 2024 • edited Loading

BeyondEvil commented Oct 26, 2024 • edited Loading

BeyondEvil commented Oct 26, 2024

BeyondEvil commented Oct 26, 2024

baronfel commented Oct 26, 2024

BeyondEvil commented Oct 26, 2024

baronfel commented Oct 26, 2024

baronfel commented Oct 26, 2024

BeyondEvil commented Oct 26, 2024

baronfel commented Oct 26, 2024

tmds commented Oct 26, 2024

tmds commented Oct 26, 2024

baronfel commented Oct 26, 2024

BeyondEvil commented Oct 26, 2024

BeyondEvil commented Oct 26, 2024

BeyondEvil commented Oct 27, 2024 • edited Loading

tmds commented Oct 27, 2024

BeyondEvil commented Oct 27, 2024 • edited Loading

tmds commented Oct 27, 2024

BeyondEvil commented Oct 27, 2024 • edited Loading

tmds commented Oct 27, 2024

BeyondEvil commented Oct 27, 2024

baronfel commented Dec 19, 2024

baronfel commented Dec 19, 2024

marcpopMSFT commented Dec 20, 2024

BeyondEvil commented Dec 20, 2024

baronfel commented Dec 20, 2024

BeyondEvil commented Dec 20, 2024

BeyondEvil commented Dec 20, 2024

baronfel commented Dec 20, 2024

baronfel commented Dec 20, 2024

github-actions bot commented Dec 20, 2024

github-actions bot commented Dec 20, 2024

BeyondEvil commented Oct 26, 2024 •

edited

Loading

BeyondEvil commented Oct 26, 2024 •

edited

Loading

BeyondEvil commented Oct 27, 2024 •

edited

Loading

BeyondEvil commented Oct 27, 2024 •

edited

Loading

BeyondEvil commented Oct 27, 2024 •

edited

Loading