container_pull: return the resolved digest to bazel and support overriding platform

[ixdy]

This potentially fixes #543.

It's roughly modeled after the fix for git_repository in bazelbuild/bazel@b42734f.

It depends on google/containerregistry#113 (and a corresponding update of that dependency in rules_docker), so this should not be merged as-is.

[nlopezgi]

Thanks for sending this PR. It looks good overall. Lets get the change in containerregistry submitted and then return to this.

[fejta]

testing 1 2 3

[fejta]

gubernator confirmed - https://k8s-gubernator.appspot.com/pr/ixdy

[fejta]

/assign @erain

[fejta]

/unassign @erain
Prow commands confirmed!

[ixdy]

google/containerregistry update now included. this should be good to go.

[nlopezgi]

Hi @ixdy ,
thanks again for sending this PR.
Let's try to add a test for it:

• add a new container_pull target in the WORKSPACE for a multi-platform image (and pick something other than linux / amd64?)
• add a container_test to https://github.com/bazelbuild/rules_docker/blob/master/tests/docker/BUILD that uses the image you just pulled. Test can have a metadata test and checl some property of the container that you would expect only be present for the given platform/os, (or you could use a file check that verifies the digest, if that is expected to be different for different platform/os)

Testing the resolved digest:

• Not sure how to do this one, I think we might need to expose the digest_result in a file produced by the container_pull rule, this would allow us to then add a file test (like https://github.com/bazelbuild/rules_docker/blob/master/tests/docker/BUILD#L168) that checks the file exists.

Please let me know if you have questions about how to add these tests.

[nlopezgi]

hi @ixdy , do you think you can try to set up tests for this PR?

[ixdy]

Yeah, I was starting to work on tests, then ran into google/containerregistry#128, and then I got busy with some other project work. I hope to get back to this by the end of the week, though we'll need a new containerregistry release, too.

[ixdy]

er, wrong issue, I meant google/containerregistry#127.

[ixdy]

(though google/containerregistry#128 means that windows images aren't supported currently)

[nlopezgi]

right, forgot about google/containerregistry#127, sgtm, if you need any help with extra reviews for containerregistry changes let me know. And bummer about google/containerregistry#128, but imo its not a priority to get windows working. (I'm not in a hurry to see this in either, just wanted to check up on status, so take your time)

[ixdy]

I've updated to containerregistry v0.0.34 and added some tests on multi-platform images. I'm still working on testing the resolved digest.

[ixdy]

(I also still need to update the README)

[ixdy]

I added some partial testing around the digests by checking that the digest file written by puller.par matches expectations. I'm not sure if there's a good way to test the return value of the container_pull rule itself.

I've also updated the README, so I think this is now ready for another review pass.

[ixdy]

I'm not particularly happy about the name of this attribute, but features is a reserved attribute.

The docker manifest list spec refers to this as CPU features, but the OCI image index spec just says it's a reserved field.

[ixdy]

I haven't found any images that actually use features yet.

[nlopezgi]

thanks for adding tests! I dont have any strong feelings about using platform_features (i.e., ifs fine by me). Couple of minor comments about sha's in container pull.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ixdy, nlopezgi
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approvers:

If they are not already assigned, you can assign the PR to them by writing /assign in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment