libpod/image: Use ParseNormalizedNamed in RepoDigests #2106

wking · 2019-01-09T06:05:53Z

Avoid generating quay.io/openshift-release-dev/ocp-release@sha256@sha256:239... and similar when the image name is already digest-based. It's not clear exactly how we get into this state, but as shown by the unit tests, the new code handles this case correctly (while the previous code does not).

Fixes #2086

wking · 2019-01-09T06:08:21Z

libpod/image/image_test.go

+		{
+			name:     "digest",
+			names:    []string{"docker.io/library/busybox@sha256:7173b809ca12ec5dee4506cd86be934c4596dd234ee82c0662eac04a8c2c71dc"},
+			expected: []string{"docker.io/library/busybox@sha256:7173b809ca12ec5dee4506cd86be934c4596dd234ee82c0662eac04a8c2c71dc"},


This test fails with the doubled @sha256 unless we also include the ParseNormalizedNamed change this commit makes to RepoDigests().

wking · 2019-01-09T06:10:50Z

pkg/varlinkapi/images.go

@@ -41,13 +41,18 @@ func (i *LibpodAPI) ListImages(call iopodman.VarlinkCall) error {
 	for _, image := range images {
 		labels, _ := image.Labels(getContext())
 		containers, _ := image.Containers()
+		repoDigests, err := image.RepoDigests()
+		if err != nil {
+			return err


I dunno why everyone else is ignoring errors here, but it's been that way since 39a7a77 (#669). @baude, do you remember your motivation?

image.Labels(...) seems to ignore errors as well.

image.Labels(...) seems to ignore errors as well.

Yeah, I left the existing error-ignores alone, since I don't understand their motivation. I'm happy to make these all consistent though (one way or the other) if someone can explain the motivation.

It looks like we don't want to fatally fail when trying to get some information on an image, but it's not commented at all so I'm not sure. @baude this intentional?

vrothberg · 2019-01-09T10:51:34Z

/approve

openshift-ci-robot · 2019-01-09T10:51:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vrothberg

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~~ [vrothberg]

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

rhatdan · 2019-01-09T11:36:10Z

@baude @mtrmac @mheon PTAL

mheon · 2019-01-09T14:39:33Z

libpod/image/image_test.go

+		{
+			name:     "tagged",
+			names:    []string{"docker.io/library/busybox:latest"},
+			expected: []string{"docker.io/library/busybox@sha256:7173b809ca12ec5dee4506cd86be934c4596dd234ee82c0662eac04a8c2c71dc"},


How often does Busybox update (such that this would no longer pull the same image)?

Can we throw a comment on here that this needs updating when busybox bumps its image version?

How often does Busybox update...

This digest is hard-coded in the test itself; it is completely decoupled from any real-world BusyBox image. So no need to bump anything.

Oh, sorry, I meant the line above - busybox:latest

Oh, sorry, I meant the line above - busybox:latest

Still just a string I'm hard-coding for these unit tests with no relation to real-world images. The new unit tests should just be exercising vendored string-manipulation code; they aren't hitting the network or anything.

Alright, should be fine then, thanks

mheon · 2019-01-09T14:41:48Z

Few questions but code LGTM.

mheon · 2019-01-09T14:42:34Z

Tests are failing - looks like there's a :latest tag in there?

wking · 2019-01-09T15:05:54Z

Tests are failing - looks like there's a :latest tag in there?

Do you know what's going in there. The unit tests pass for me locally, and we vendor the parser, so I dunno how CI is failing.

rhatdan · 2019-01-09T15:08:40Z

Will attempt to rerun them to see if it is a flake.

wking · 2019-01-09T15:25:57Z

The unit tests pass for me locally...

Actually, I must have changed something and forgotten to run them again, since now I can reproduce locally (thank you, CI ;). Obviously reference.WithDigest is not doing what I think it should be doing, since it preserves the tag which is no longer needed once you have a digest. @mtrmac, what's the proper invocation to attach a digest to a name, stripping the tag from tagged names?

mtrmac · 2019-01-09T17:01:10Z

Obviously reference.WithDigest is not doing what I think it should be doing, since it preserves the tag which is no longer needed once you have a digest. @mtrmac, what's the proper invocation to attach a digest to a name, stripping the tag from tagged names?

reference.WithDigest(reference.TrimNamed(input), digest)

should work.

mtrmac · 2019-01-09T16:58:33Z

libpod/image/image.go

 	for _, name := range i.Names() {
-		repoDigests = append(repoDigests, strings.SplitN(name, ":", 2)[0]+"@"+i.Digest().String())
+		named, err := reference.ParseNormalizedNamed(name)
+		if err != nil {


FWIW c/image/storage silently ignores errors (= .Names entries not in the c/image/docker/reference format).

Names are not documented to be in any particular format, so hard failures here may not be appropriate — OTOH maybe we should tighten the definition of Names instead. I don’t know.

Names are not documented to be in any particular format, so hard failures here may not be appropriate...

I don't have an opinion on what ParseNormaliedNamed wants to consider an error, but if it returns an error, I think we should be passing it back up the chain. I certainly don't want different opinions on what names are considered valid living at different places in the stack.

I’m saying that the existing opinion/practice is that non-ParseNormalizedNamed names are 1) allowed and 2) silently ignored, so if you ”don’t want different opinions” (which I agree with) the right thing to do is to silently ignore such names as well.

I do support tightening the definition, but if that should happen, it needs be a separate PR set across c/storage (documenting the field as such)+c/image+libpod (using it) and maybe others.

wking · 2019-01-09T17:44:01Z

I've pushed 397ba9d -> f539fd2 adding TrimedName, which should green up CI (it works for me locally).

rhatdan · 2019-01-09T19:40:18Z

@wking Still broken :^(

mheon · 2019-01-09T19:42:07Z

@wking I think the issue with CI was introduced by the most recent merge (another user of RepoDigests appeared and needs to be changed to use the error return)

Avoid generating quay.io/openshift-release-dev/ocp-release@sha256@sha256:239... and similar when the image name is already digest-based [1]. It's not clear exactly how we get into this state, but as shown by the unit tests, the new code handles this case correctly (while the previous code does not). [1]: containers#2086 Signed-off-by: W. Trevor King <wking@tremily.us>

wking · 2019-01-09T21:03:15Z

Rebased around #2040 with f539fd2 -> a780521.

wking · 2019-01-09T21:57:49Z

All green :).

mheon · 2019-01-09T22:22:24Z

Code LGTM

rhatdan · 2019-01-09T22:29:13Z

@rh-atomic-bot r+

rh-atomic-bot · 2019-01-09T22:29:13Z

📌 Commit a780521 has been approved by rhatdan

rh-atomic-bot · 2019-01-09T22:29:21Z

⌛ Testing commit a780521 with merge 0f6535c...

rh-atomic-bot · 2019-01-09T23:02:45Z

☀️ Test successful - status-papr
Approved by: rhatdan
Pushing 0f6535c to master...

…-release:4.0.0-0.2 RHCOS 47.280 brings in, among other things, [1], which fixes a bug we see occasionally on the bootstrap machine. Clayton pushed 4.0.0-0.nightly-2019-01-25-205123 to quay.io/openshift-release-dev/ocp-release:4.0.0-0.2. Renaming OPENSHIFT_INSTALL_RELEASE_IMAGE_OVERRIDE gets us CI testing of the pinned release despite openshift/release@60007df2 (Use RELEASE_IMAGE_LATEST for CVO payload, 2018-10-03, openshift/release#1793). Also comment out regions which this particular RHCOS build wasn't pushed to, leaving only: $ curl -s https://releases-rhcos.svc.ci.openshift.org/storage/releases/maipo/47.280/meta.json | jq -r '.amis[] | .name' ap-northeast-1 ap-northeast-2 ap-south-1 ap-southeast-1 ap-southeast-2 ca-central-1 eu-central-1 eu-west-1 eu-west-2 eu-west-3 sa-east-1 us-east-1 us-east-2 us-west-1 us-west-2 I'd initially expected to export the pinning environment variables in release.sh, but I've put them in build.sh here because our continuous integration tests use build.sh directly and don't go through release.sh. [1]: containers/podman#2106

To get the more-robust handling from 0f6535c (libpod/image: Use ParseNormalizedNamed in RepoDigests, 2019-01-08, containers#2106) here too.

To get the more-robust handling from 0f6535c (libpod/image: Use ParseNormalizedNamed in RepoDigests, 2019-01-08, containers#2106) here too. Signed-off-by: W. Trevor King <wking@tremily.us>

…-release:4.0.0-0.3 The bump from RHCOS 47.280 to 47.297 brings in, among other things, v0.12 kubelets. Clayton pushed 4.0.0-0.nightly-2019-01-30-145955 to quay.io/openshift-release-dev/ocp-release:4.0.0-0.3. Renaming OPENSHIFT_INSTALL_RELEASE_IMAGE_OVERRIDE gets us CI testing of the pinned release despite openshift/release@60007df2 (Use RELEASE_IMAGE_LATEST for CVO payload, 2018-10-03, openshift/release#1793). Also comment out regions which this particular RHCOS build wasn't pushed to, leaving only: $ curl -s https://releases-rhcos.svc.ci.openshift.org/storage/releases/maipo/47.297/meta.json | jq -r '.amis[] | .name' ap-northeast-1 ap-northeast-2 ap-south-1 ap-southeast-1 ap-southeast-2 ca-central-1 eu-central-1 eu-west-1 eu-west-2 eu-west-3 sa-east-1 us-east-1 us-east-2 us-west-1 us-west-2 I'd initially expected to export the pinning environment variables in release.sh, but I've put them in build.sh here because our continuous integration tests use build.sh directly and don't go through release.sh. [1]: containers/podman#2106

To get the more-robust handling from 0f6535c (libpod/image: Use ParseNormalizedNamed in RepoDigests, 2019-01-08, containers#2106) here too. Signed-off-by: W. Trevor King <wking@tremily.us>

…-release:4.0.0-0.4 The bump from RHCOS 47.297 to 47.318 brings in, among other things, Podman 1.0.1. Clayton pushed 4.0.0-0.nightly-2019-02-17-024922 to quay.io/openshift-release-dev/ocp-release:4.0.0-0.4. Renaming OPENSHIFT_INSTALL_RELEASE_IMAGE_OVERRIDE gets us CI testing of the pinned release despite openshift/release@60007df2 (Use RELEASE_IMAGE_LATEST for CVO payload, 2018-10-03, openshift/release#1793). Also comment out regions which this particular RHCOS build wasn't pushed to, leaving only: $ curl -s https://releases-rhcos.svc.ci.openshift.org/storage/releases/maipo/47.318/meta.json | jq -r '.amis[] | .name' ap-northeast-1 ap-northeast-2 ap-south-1 ap-southeast-1 ap-southeast-2 ca-central-1 eu-central-1 eu-west-1 eu-west-2 eu-west-3 sa-east-1 us-east-1 us-east-2 us-west-1 us-west-2 I'd initially expected to export the pinning environment variables in release.sh, but I've put them in build.sh here because our continuous integration tests use build.sh directly and don't go through release.sh. [1]: containers/podman#2106

…-release:4.0.0-0.5 Clayton pushed 4.0.0-0.nightly-2019-02-26-125216 to quay.io/openshift-release-dev/ocp-release:4.0.0-0.5. Extracting the associated RHCOS build: $ oc adm release info --pullspecs quay.io/openshift-release-dev/ocp-release:4.0.0-0.5 | grep machine-os-content machine-os-content registry.svc.ci.openshift.org/ocp/4.0-art-latest-2019-02-26-125216@sha256:1262533e31a427917f94babeef2774c98373409897863ae742ff04120f32f79b $ oc image info registry.svc.ci.openshift.org/ocp/4.0-art-latest-2019-02-26-125216@sha256:1262533e31a427917f94babeef2774c98373409897863ae742ff04120f32f79b | grep version version=47.330 The bump from RHCOS 47.297 to 47.330 brings in, among other things, Podman 1.0.1. Renaming OPENSHIFT_INSTALL_RELEASE_IMAGE_OVERRIDE gets us CI testing of the pinned release despite openshift/release@60007df2 (Use RELEASE_IMAGE_LATEST for CVO payload, 2018-10-03, openshift/release#1793). Also comment out regions which this particular RHCOS build wasn't pushed to, leaving only: $ curl -s https://releases-rhcos.svc.ci.openshift.org/storage/releases/maipo/47.330/meta.json | jq -r '.amis[] | .name' ap-northeast-1 ap-northeast-2 ap-south-1 ap-southeast-1 ap-southeast-2 ca-central-1 eu-central-1 eu-west-1 eu-west-2 eu-west-3 sa-east-1 us-east-1 us-east-2 us-west-1 us-west-2 I'd initially expected to export the pinning environment variables in release.sh, but I've put them in build.sh here because our continuous integration tests use build.sh directly and don't go through release.sh. [1]: containers/podman#2106

openshift-ci-robot requested review from giuseppe and vrothberg January 9, 2019 06:05

openshift-ci-robot added the size/M label Jan 9, 2019

wking force-pushed the fix-repo-digests-name-parsing branch from c675435 to 397ba9d Compare January 9, 2019 06:06

wking mentioned this pull request Jan 9, 2019

@sha256@sha256 strikes again! (on 0.11.1.1) #2086

Closed

wking commented Jan 9, 2019

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 9, 2019

mheon reviewed Jan 9, 2019

View reviewed changes

wking mentioned this pull request Jan 9, 2019

installer not completing : incorrect ocp default registry format registry@sha@sha openshift/installer#933

Closed

mtrmac reviewed Jan 9, 2019

View reviewed changes

wking force-pushed the fix-repo-digests-name-parsing branch from 397ba9d to f539fd2 Compare January 9, 2019 17:43

wking force-pushed the fix-repo-digests-name-parsing branch from f539fd2 to a780521 Compare January 9, 2019 21:02

wking mentioned this pull request Jan 9, 2019

Remove the double @sha256 in case the image name is already digest base openshift/installer#1032

Merged

rh-atomic-bot closed this in 0f6535c Jan 9, 2019

wking mentioned this pull request Jan 15, 2019

[aws] pulling ocp-release images includes @sha256@sha256 on latest installer 0.9.1 causing installation break openshift/installer#1066

Closed

mtrmac mentioned this pull request Jan 17, 2019

(podman images) does not handle digested references correctly #2175

Closed

wking mentioned this pull request Jan 27, 2019

hack/build: Pin to RHCOS 47.280 and quay.io/openshift-release-dev/ocp-release:4.0.0-0.2 openshift/installer#1134

Closed

wking deleted the fix-repo-digests-name-parsing branch January 31, 2019 18:23

wking added a commit to wking/libpod that referenced this pull request Jan 31, 2019

libpod/image: Use RepoDigests() in Inspect()

9984c5a

To get the more-robust handling from 0f6535c (libpod/image: Use ParseNormalizedNamed in RepoDigests, 2019-01-08, containers#2106) here too.

wking mentioned this pull request Jan 31, 2019

libpod/image: Use RepoDigests() in Inspect() #2251

Merged

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 27, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

libpod/image: Use ParseNormalizedNamed in RepoDigests #2106

libpod/image: Use ParseNormalizedNamed in RepoDigests #2106

wking commented Jan 9, 2019

wking Jan 9, 2019

wking Jan 9, 2019

vrothberg Jan 9, 2019

wking Jan 9, 2019

mheon Jan 9, 2019

vrothberg commented Jan 9, 2019

openshift-ci-robot commented Jan 9, 2019

rhatdan commented Jan 9, 2019

mheon Jan 9, 2019

wking Jan 9, 2019

mheon Jan 9, 2019

wking Jan 9, 2019

mheon Jan 9, 2019

mheon commented Jan 9, 2019

mheon commented Jan 9, 2019

wking commented Jan 9, 2019

rhatdan commented Jan 9, 2019

wking commented Jan 9, 2019

mtrmac commented Jan 9, 2019

mtrmac Jan 9, 2019

wking Jan 9, 2019

mtrmac Jan 9, 2019

wking commented Jan 9, 2019

rhatdan commented Jan 9, 2019

mheon commented Jan 9, 2019

wking commented Jan 9, 2019

wking commented Jan 9, 2019

mheon commented Jan 9, 2019

rhatdan commented Jan 9, 2019

rh-atomic-bot commented Jan 9, 2019

rh-atomic-bot commented Jan 9, 2019

rh-atomic-bot commented Jan 9, 2019

libpod/image: Use ParseNormalizedNamed in RepoDigests #2106

libpod/image: Use ParseNormalizedNamed in RepoDigests #2106

Conversation

wking commented Jan 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vrothberg commented Jan 9, 2019

openshift-ci-robot commented Jan 9, 2019

rhatdan commented Jan 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mheon commented Jan 9, 2019

mheon commented Jan 9, 2019

wking commented Jan 9, 2019

rhatdan commented Jan 9, 2019

wking commented Jan 9, 2019

mtrmac commented Jan 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wking commented Jan 9, 2019

rhatdan commented Jan 9, 2019

mheon commented Jan 9, 2019

wking commented Jan 9, 2019

wking commented Jan 9, 2019

mheon commented Jan 9, 2019

rhatdan commented Jan 9, 2019

rh-atomic-bot commented Jan 9, 2019

rh-atomic-bot commented Jan 9, 2019

rh-atomic-bot commented Jan 9, 2019