Image digest output for container_image rule #445

ash2k · 2018-07-05T06:50:57Z

This PR adds an additional predeclared output to the container_image rule which contains image digest of that image.

This PR depends on google/containerregistry#87 and needs to be updated with a version of containerregistry that contains those changes.

Fixes #271.

go_binary(
    name = "smith",
    embed = [":go_default_library"],
    pure = "on",
    visibility = ["//visibility:public"],
)

container_push(
    name = "smith.push_docker",
    format = "Docker",
    image = ":smith",
    registry = ...,
    repository = ...,
    stamp = True,
    tag = ...,
)

$ bazel build  --platforms=@io_bazel_rules_go//go/toolchain:linux_amd64  //cmd/smith:container.digest
INFO: Analysed target //cmd/smith:container.digest (0 packages loaded).
INFO: Found 1 target...
Target //cmd/smith:container.digest up-to-date:
  bazel-bin/cmd/smith/container.digest
INFO: Elapsed time: 1.026s, Critical Path: 0.61s
INFO: 1 process: 1 darwin-sandbox.
INFO: Build completed successfully, 2 total actions

$ cat bazel-bin/cmd/smith/container.digest
sha256:73122a2d81d57895f5740efa9030c25045a9a797cd118c8f9867c8de02d57425

$ bazel run  --platforms=@io_bazel_rules_go//go/toolchain:linux_amd64  //cmd/smith:smith.push_docker
INFO: Analysed target //cmd/smith:smith.push_docker (1 packages loaded).
INFO: Found 1 target...
Target //cmd/smith:smith.push_docker up-to-date:
  bazel-bin/cmd/smith/smith.push_docker
INFO: Elapsed time: 0.538s, Critical Path: 0.19s
INFO: 0 processes.
INFO: Build completed successfully, 1 total action
INFO: Build completed successfully, 1 total action
<bla-bla-bla-stamping> was resolved to <bla-bla-bla-stamping>/smith:e4cd67ec0-6ebc8057e7619aba8e3c63f4392f1e44
<bla-bla-bla-stamping>/smith:e4cd67ec0-6ebc8057e7619aba8e3c63f4392f1e44 was published with digest: sha256:73122a2d81d57895f5740efa9030c25045a9a797cd118c8f9867c8de02d57425

nlopezgi

This looks good overall. Thanks for sending this PR. I'll kick off presubmits to verify all tests pass, but we'll need to wait until the PR in container registry is in to approve. Please let me know one that is in to kick of presubmit again.

nlopezgi · 2018-07-05T13:32:53Z

container/image.bzl

@@ -228,6 +250,7 @@ def _impl(
    tars: File list, overrides ctx.files.tars
    output_executable: File to use as output for script to load docker image
    output_tarball: File, overrides ctx.outputs.out
+    output_digest: File, overrides ctx.outputs.digest


Could you please document this new implicit output in https://github.com/bazelbuild/rules_docker#container_image-1

nlopezgi · 2018-07-05T13:45:55Z

container/image.bzl

+
+    ctx.actions.run(
+        outputs = [output_digest],
+        inputs = [image["config"]] + blobsums + blobs + ([image["legacy"]] if image.get("legacy") else []),


wrap line (or declare inputs above?)

ArthurRab · 2018-07-05T15:22:58Z

Hey Mikhail, the container image rule already produces a manifest.json in the tarball which contains the image's id. There is a helper script at extract_image_id.sh which extracts it for you!

ittaiz · 2018-07-05T17:43:26Z

Wow, this looks really promising!
We’re just trying to figure this out ourselves.
We need to compare the current rule digest to one in our registry to know if we need to push or not. Do you think we will be able to use this as is for this need or am I mixing things up?

ArthurRab · 2018-07-05T18:11:46Z

Sure, I don't see why not. The script I mentioned simply gives you the image's id from the tarball.

ash2k · 2018-07-05T22:04:47Z

@ArthurRab I see, I didn't know, thanks. However, I'd like to get this information as an additional output + expose that output in the ImageInfo provider. Should I rewrite this PR to use the script? What I don't like about the script, is that it "parses" json using cut. Might be fragile and also cut is a non-hermetic dependency.

ash2k · 2018-07-05T22:31:26Z

@ArthurRab hey, I can see you sent #448 - this is awesome, thanks. Could you add a command line flag to that script to output the digest into a file please? It is easier to program a rule if the command can output into a file, rather that just print the result. I'll be able to use that new script in this PR as a replacement for the python script in containerregistry repo. Btw, I filled bazelbuild/bazel#5511 a couple of days ago, asking for a way to capture stdout.

ash2k · 2018-07-05T23:03:05Z

Btw, I only see a single tar file + a tar.gz but I don't see any manifest.json files in them. Where is it?

ash2k · 2018-07-05T23:19:00Z

Ok, I found it. I need to request that .tar output explicitly and then I get the file. But that manifest.json does not contain the digest, produced by container_push and/or .digest output.

 $ bazel run --platforms=@io_bazel_rules_go//go/toolchain:linux_amd64 //cmd/smith:push_docker
INFO: Analysed target //cmd/smith:push_docker (0 packages loaded).
INFO: Found 1 target...
Target //cmd/smith:push_docker up-to-date:
  bazel-bin/cmd/smith/push_docker
INFO: Elapsed time: 0.342s, Critical Path: 0.10s
INFO: 0 processes.
INFO: Build completed successfully, 1 total action
INFO: Build completed successfully, 1 total action
index.docker.io/atlassianlabs/smith:{STABLE_BUILD_GIT_TAG}-{STABLE_BUILD_GIT_COMMIT} was resolved to index.docker.io/atlassianlabs/smith:v1.0.0-2cb13f4
index.docker.io/atlassianlabs/smith:v1.0.0-2cb13f4 was published with digest: sha256:6b74d7e6b4cf36aae4c54e723eb1bb59b6d84a7ceae2141e6804cae3c0ec3f4b

 $ bazel build --platforms=@io_bazel_rules_go//go/toolchain:linux_amd64 //cmd/smith:container.digest
INFO: Analysed target //cmd/smith:container.digest (0 packages loaded).
INFO: Found 1 target...
Target //cmd/smith:container.digest up-to-date:
  bazel-bin/cmd/smith/container.digest
INFO: Elapsed time: 0.258s, Critical Path: 0.04s
INFO: 0 processes.
INFO: Build completed successfully, 1 total action

 $ cat bazel-bin/cmd/smith/container.digest
sha256:6b74d7e6b4cf36aae4c54e723eb1bb59b6d84a7ceae2141e6804cae3c0ec3f4b%

 $ bazel build --platforms=@io_bazel_rules_go//go/toolchain:linux_amd64 //cmd/smith:container.tar   
INFO: Analysed target //cmd/smith:container.tar (0 packages loaded).
INFO: Found 1 target...
Target //cmd/smith:container.tar up-to-date:
  bazel-bin/cmd/smith/container.tar
INFO: Elapsed time: 0.241s, Critical Path: 0.04s
INFO: 0 processes.
INFO: Build completed successfully, 1 total action

 $ tar -xf bazel-bin/cmd/smith/container.tar "manifest.json"                                        
 
 $ cat manifest.json | grep 6b74d7e6b4cf36aae4c54e723eb1bb59b6d84a7ceae2141e6804cae3c0ec3f4b

 $ cat manifest.json
[{"Config": "07a57c5b5e1100d9ed139dc7641e84f19602a4501aa6c8df115023623c76d005.json", "Layers": ["b0dde63cbf25a3f5a23ca221181d9f00ae48be8cb125938dc388e93ac6ff6dfd/layer.tar", "a6fe265ee3668f5cc5fc0d5dee33593df6d199069ef4be0a9648d478b04ec13e/layer.tar"], "RepoTags": ["bazel/cmd/smith:container"]}]%

ArthurRab · 2018-07-06T13:59:58Z

Ah, my bad. I had the image id and digest confused, sorry about that. We don't have a script that gives you the digest so this pr is still valid. Go ahead and make the requested changes and we'll merge it in.

ash2k · 2018-07-06T22:31:54Z

No worries. I've made the requested changes, waiting on the containerregistry PR to get merged.

ittaiz · 2018-08-16T04:51:34Z

Hi,
Is this approved apart from the commit pointer of container registry? This has been pending for a long time (due to the PR there waiting) and I think it would be great to make sure this can be merged as soon as the other one gets merged

nlopezgi · 2018-08-16T19:00:18Z

Yes, iirc I think this can be merged once the upstream PR is in (and conflicts have been resolved). Let me know if you are having issues with that one to ping them.

ittaiz · 2018-08-16T19:50:40Z

Last update there was 11 days ago about finalizing code review. If you can ping them on Monday I’d appreciate it very much

…

On Thu, 16 Aug 2018 at 22:00 Nicolas Lopez ***@***.***> wrote: Yes, iirc I think this can be merged once the upstream PR is in (and conflicts have been resolved). Let me know if you are having issues with that one to ping them. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#445 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUIF5URHiqO_HI3_SRlavDUAJWtQVLLks5uRcFDgaJpZM4VDVYH> .

ash2k · 2018-08-27T15:33:55Z

This is ready for review and merge.

ittaiz · 2018-08-28T06:51:39Z

yay, everything is green. @nlopezgi waiting for your approval.

nlopezgi

Thanks again for sending this PR and good to hear the upstream change in containerregstry finally made it in.

This looks good overall, one comment about the digest being part of the cotnainer_parts.

Also, could you add a test to https://github.com/bazelbuild/rules_docker/blob/master/tests/docker/BUILD that verifies the digest output is produced with the right value?

nlopezgi · 2018-08-28T15:03:03Z

container/providers.bzl

@@ -20,7 +20,7 @@ BundleInfo = provider(fields = ["container_images", "stamp"])
 FlattenInfo = provider()

 # A provider containing information exposed by container_image rules
-ImageInfo = provider(fields = ["container_parts"])
+ImageInfo = provider(fields = ["container_parts", "digest"])


Would it make sense to make the digest part of the container_parts? If not, why?

Makes sense, done.

ash2k · 2018-08-30T10:26:26Z

Also, could you add a test to https://github.com/bazelbuild/rules_docker/blob/master/tests/docker/BUILD that verifies the digest output is produced with the right value?

I've just tried to figure out how to do that but could not. Any specific instructions on how to do that would be very useful.

xingao267 · 2018-08-30T15:11:33Z

@ash2k There is a file_test skylark rule which should work for you. Example usage: https://github.com/bazelbuild/rules_docker/blob/master/tests/contrib/BUILD#L140

You can have something like:

container_image(
    name = "my_test_image",
    ....
)

file_test(
    name = "test_container_image_output_digest",
    file = ":my_test_image.digest",
    regexp (or match depends on your need) = "<the digest>",
)

ash2k · 2018-08-30T18:12:47Z

@xingao267 thanks a lot, that was much easier than I expected. Ready for merge.

stevewolter · 2018-08-31T12:15:56Z

Danger: This change is missing support for the newly-introduced --manifest flag: google/containerregistry@2acf471. In the current form, this change produces different digests than the images pushed with docker_push.

In containerregistry's image_digest tool, manifest support has also been overlooked, so a containerregistry change like stevewolter/containerregistry@21496a1 is needed first.

ittaiz · 2018-08-31T13:33:16Z

Wow, this is indeed dangerous.

…

On Fri, 31 Aug 2018 at 15:15 Steve Wolter ***@***.***> wrote: Danger: This change is missing support for the newly-introduced --manifest flag: ***@***.*** <google/containerregistry@2acf471>. In the current form, this change produces different digests than the images pushed with docker_push. In containerregistry's image_digest tool, manifest support has also been overlooked, so a containerregistry change like ***@***.*** <stevewolter/containerregistry@21496a1> is needed first. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#445 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUIFwBnZr4iR1Ur-V4YpoIiI2OthmEEks5uWSj_gaJpZM4VDVYH> .

nlopezgi · 2018-08-31T13:36:17Z

Thanks for noticing this issue. I think we should wait until the fix @stevewolter proposes is in container registry, could you please send them a PR (and I will help get it submitted faster early next week, or later today if I can finish some other high priority tasks I have pending).

stevewolter · 2018-09-03T12:25:53Z

Thanks for the help, Nicolas. Sent google/containerregistry#104.

…

On Fri, Aug 31, 2018 at 3:36 PM Nicolas Lopez ***@***.***> wrote: Thanks for noticing this issue. I think we should wait until the fix @stevewolter <https://github.com/stevewolter> proposes is in container registry, could you please send them a PR (and I will help get it submitted faster early next week, or later today if I can finish some other high priority tasks I have pending). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#445 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFgJgNRyAdnLqCTCWUNKL6wSY5l_I8pAks5uWTvTgaJpZM4VDVYH> .

nlopezgi · 2018-09-07T19:08:32Z

@stevewolter let me know if I can help get google/containerregistry#104 submitted faster (e.g., by trying to ping owners internally)

nlopezgi · 2018-09-11T17:59:18Z

Thanks @stevewolter! the change to container registry is in github, @ash2k can you update the pin to containerregistry in this cl and let me know when its ready to review again.

ash2k · 2018-09-13T06:48:11Z

@nlopezgi updated

googlebot added the cla: yes label Jul 5, 2018

ash2k mentioned this pull request Jul 5, 2018

Tool to get image digest google/containerregistry#87

Closed

ash2k force-pushed the image_digest branch from 87a5d41 to 52cc8ea Compare July 5, 2018 11:38

nlopezgi self-requested a review July 5, 2018 13:55

nlopezgi reviewed Jul 5, 2018

View reviewed changes

ash2k force-pushed the image_digest branch from 52cc8ea to 841ad3a Compare July 5, 2018 23:28

ash2k force-pushed the image_digest branch from 841ad3a to 8d3b688 Compare August 16, 2018 09:51

ash2k force-pushed the image_digest branch 2 times, most recently from 36868b1 to 49395fc Compare August 27, 2018 10:44

nlopezgi reviewed Aug 28, 2018

View reviewed changes

ash2k force-pushed the image_digest branch from 49395fc to 129716e Compare August 30, 2018 10:23

ash2k force-pushed the image_digest branch from 129716e to 9d28b4a Compare August 30, 2018 10:27

ash2k force-pushed the image_digest branch from 9b7fe3c to dac564a Compare August 30, 2018 18:11

ash2k mentioned this pull request Sep 12, 2018

Fix bug in image digester google/containerregistry#107

Merged

Image digest output for container_image rule

e548ecb

ash2k force-pushed the image_digest branch from dac564a to e548ecb Compare September 13, 2018 06:47

nlopezgi approved these changes Sep 13, 2018

View reviewed changes

nlopezgi merged commit 9fd09b3 into bazelbuild:master Sep 13, 2018

ash2k deleted the image_digest branch September 13, 2018 17:45

nlopezgi mentioned this pull request Oct 17, 2018

Run update-deps.sh #549

Merged

nlopezgi mentioned this pull request Aug 9, 2019

[feature request] provide sha256 of image without pushing to registry #538

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Image digest output for container_image rule #445

Image digest output for container_image rule #445

ash2k commented Jul 5, 2018 •

edited

Loading

nlopezgi left a comment

nlopezgi Jul 5, 2018

nlopezgi Jul 5, 2018

ArthurRab commented Jul 5, 2018

ittaiz commented Jul 5, 2018

ArthurRab commented Jul 5, 2018

ash2k commented Jul 5, 2018

ash2k commented Jul 5, 2018

ash2k commented Jul 5, 2018

ash2k commented Jul 5, 2018

ArthurRab commented Jul 6, 2018

ash2k commented Jul 6, 2018

ittaiz commented Aug 16, 2018

nlopezgi commented Aug 16, 2018

ittaiz commented Aug 16, 2018 via email

ash2k commented Aug 27, 2018

ittaiz commented Aug 28, 2018

nlopezgi left a comment

nlopezgi Aug 28, 2018

ash2k Aug 30, 2018

ash2k commented Aug 30, 2018

xingao267 commented Aug 30, 2018 •

edited

Loading

ash2k commented Aug 30, 2018

stevewolter commented Aug 31, 2018

ittaiz commented Aug 31, 2018 via email

nlopezgi commented Aug 31, 2018

stevewolter commented Sep 3, 2018 via email

nlopezgi commented Sep 7, 2018

nlopezgi commented Sep 11, 2018

ash2k commented Sep 13, 2018

Image digest output for container_image rule #445

Image digest output for container_image rule #445

Conversation

ash2k commented Jul 5, 2018 • edited Loading

nlopezgi left a comment

Choose a reason for hiding this comment

nlopezgi Jul 5, 2018

Choose a reason for hiding this comment

nlopezgi Jul 5, 2018

Choose a reason for hiding this comment

ArthurRab commented Jul 5, 2018

ittaiz commented Jul 5, 2018

ArthurRab commented Jul 5, 2018

ash2k commented Jul 5, 2018

ash2k commented Jul 5, 2018

ash2k commented Jul 5, 2018

ash2k commented Jul 5, 2018

ArthurRab commented Jul 6, 2018

ash2k commented Jul 6, 2018

ittaiz commented Aug 16, 2018

nlopezgi commented Aug 16, 2018

ittaiz commented Aug 16, 2018 via email

ash2k commented Aug 27, 2018

ittaiz commented Aug 28, 2018

nlopezgi left a comment

Choose a reason for hiding this comment

nlopezgi Aug 28, 2018

Choose a reason for hiding this comment

ash2k Aug 30, 2018

Choose a reason for hiding this comment

ash2k commented Aug 30, 2018

xingao267 commented Aug 30, 2018 • edited Loading

ash2k commented Aug 30, 2018

stevewolter commented Aug 31, 2018

ittaiz commented Aug 31, 2018 via email

nlopezgi commented Aug 31, 2018

stevewolter commented Sep 3, 2018 via email

nlopezgi commented Sep 7, 2018

nlopezgi commented Sep 11, 2018

ash2k commented Sep 13, 2018

ash2k commented Jul 5, 2018 •

edited

Loading

xingao267 commented Aug 30, 2018 •

edited

Loading