Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for using Digest for base image #44461

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

BeyondEvil
Copy link

@BeyondEvil BeyondEvil commented Oct 26, 2024

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

@dotnet-issue-labeler 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 BeyondEvil force-pushed the beyondevil/support-image-digest branch from 2842ab1 to 7f70916 Compare October 26, 2024 11:59
@BeyondEvil
Copy link
Author

BeyondEvil commented Oct 26, 2024

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
Copy link
Author

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

@BeyondEvil
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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
Copy link
Member

tmds commented Oct 26, 2024

@BeyondEvil can you please add additional cases to

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
Copy link
Member

tmds commented Oct 26, 2024

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
Copy link
Member

@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
Copy link
Author

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

@BeyondEvil
Copy link
Author

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 BeyondEvil force-pushed the beyondevil/support-image-digest branch from 7f70916 to 9f6970f Compare October 26, 2024 22:44
@BeyondEvil
Copy link
Author

BeyondEvil commented Oct 27, 2024

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
Copy link
Member

tmds commented Oct 27, 2024

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
Copy link
Author

BeyondEvil commented Oct 27, 2024

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
Copy link
Member

tmds commented Oct 27, 2024

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

@BeyondEvil
Copy link
Author

BeyondEvil commented Oct 27, 2024

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
Copy link
Member

tmds commented Oct 27, 2024

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 BeyondEvil force-pushed the beyondevil/support-image-digest branch from 9f6970f to 83e5984 Compare October 27, 2024 12:03
@BeyondEvil
Copy link
Author

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

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 BeyondEvil force-pushed the beyondevil/support-image-digest branch from 83e5984 to 0a0a8b8 Compare October 27, 2024 12:08
@tmds
Copy link
Member

tmds commented Oct 27, 2024

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
Copy link
Member

@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
Copy link
Author

@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
Copy link
Member

baronfel commented Oct 27, 2024

@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":

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 BeyondEvil marked this pull request as ready for review October 27, 2024 18:38
@BeyondEvil BeyondEvil requested a review from a team as a code owner October 27, 2024 18:38
@BeyondEvil
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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. 🙌

@tmds
Copy link
Member

tmds commented Oct 29, 2024

@BeyondEvil thanks for contributing this! This is a very useful feature! And, nice work on the PR!

@BeyondEvil
Copy link
Author

@BeyondEvil thanks for contributing this! This is a very useful feature! And, nice work on the PR!

Thank you, it was my pleasure!

And thank you for all the guidance! 🙇

@tmds
Copy link
Member

tmds commented Nov 13, 2024

@baronfel we didn't have to extend the parser in this PR to accept the digest. This means that existing SDKs allow a digest but then ignore it. We should clearly document what SDK versions are taking into account the digest and which aren't.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Containers Related to dotnet SDK containers functionality untriaged Request triage from a team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support explicitly specifying an image by SHA digest, not just tag
3 participants