Bug 1823143: Try user-given registry/repository for oc adm release extract|info|mirror before defaulting to referenced image #395

sallyom · 2020-04-22T04:30:29Z

When working with mirrored release payloads, a release from a mirrored registry,
mylocalregistry/ocp/release:4.5.0-0.nightly-2020-04-18-093630 mirrored from
registry.svc.ci.openshift.org/ocp/release:4.5.0-0.nightly-2020-04-18-093630 -
In both, the tags reference quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:2eb0a51....
In oc adm release extract, always try extracting from the user-given release first. If this fails, then fall back to the release image-references. In disconnected environments, currently, oc adm release extract fails when the given release is mirrored from a connected registry.

Different from mirrored, when working with a tagged release that was uploaded to another repository, such as quay.io/sallyom/release:new, since that release wasn't mirrored, the underlying images still point to the original tag. In this case, oc adm release extract --command openshift-install will try the sallyom/release image but since that image does not exist, proceed to use the underlying tagged installer image - that will succeed as long as the user has access to the original release image. Note many devs build release w/ substitutions for one or a few component images, and this is the way they sometimes push images around.

To test this PR, do something like this:

$ podman run -p 5000:5000 -d registry:2
$ oc adm release mirror --insecure=true -a ~/pull-secret --from=registry.svc.ci.openshift.org/ocp/release:4.5.0-0.nightly-2020-04-29-144201 --to localhost:5000/ocp/release
$ oc adm release --insecure=true extract --command openshift-install localhost:5000/ocp/release:4.5.0-0.nightly-2020-04-29-144201 -v=2

you'll find the failure from master, fixed w /this PR

Scenarios i tested:

build new release w/ substituting some component images from a ci/nightly/whatever, then extract or info from that
extract from release that was mirrored from ci or nightly and is now in disconnected registry - verify this image is extracted from disconnected registry with -v=2
pull down a release from ci, tag it as mine, push to my quay, extract from that
extract or info from a ci image (no openshift-release-dev/ocp-v4.0-art-dev reference) - if you can access registry.svc.ci images this succeeds, if not, you'll get unauthorized
mirror a release from local file or from localregistry to remote registry

Bugs that are resolved with these changes (or the equivalent):

openshift-ci-robot · 2020-04-22T04:30:36Z

@sallyom: This pull request references Bugzilla bug 1823143, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.5.0) matches configured target release for branch (4.5.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Details

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot · 2020-04-22T04:30:52Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sallyom
To complete the pull request process, please assign mfojtik
You can assign the PR to them by writing /assign @mfojtik in a comment when ready.

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

Details

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

openshift-ci-robot · 2020-04-22T04:49:01Z

@sallyom: This pull request references Bugzilla bug 1823143, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.5.0) matches configured target release for branch (4.5.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Details

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot · 2020-04-22T04:49:27Z

@sallyom: This pull request references Bugzilla bug 1823143, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.5.0) matches configured target release for branch (4.5.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Details

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot · 2020-04-22T04:50:05Z

@sallyom: This pull request references Bugzilla bug 1823143, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.5.0) matches configured target release for branch (4.5.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Details

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot · 2020-04-22T05:41:17Z

@sallyom: An error was encountered searching for bug 1823143 on the Bugzilla server at https://bugzilla.redhat.com:

Details

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot · 2020-04-22T05:52:28Z

@sallyom: An error was encountered searching for bug 1823143 on the Bugzilla server at https://bugzilla.redhat.com:

Details

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

pkg/cli/admin/release/extract_tools.go

openshift-ci-robot · 2020-04-22T16:26:39Z

@sallyom: This pull request references Bugzilla bug 1823143, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.5.0) matches configured target release for branch (4.5.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Details

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot · 2020-04-22T16:33:44Z

@sallyom: This pull request references Bugzilla bug 1823143, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.5.0) matches configured target release for branch (4.5.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Details

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

sallyom · 2020-04-22T18:51:34Z

/test e2e-cmd

openshift-ci-robot · 2020-04-23T14:47:36Z

@sallyom: This pull request references Bugzilla bug 1823143, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.5.0) matches configured target release for branch (4.5.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Details

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

sallyom · 2020-04-23T14:49:31Z

/retest

sallyom · 2020-04-23T22:22:24Z

/hold

This PR uses a bump in library-go for a not-yet-merged PR: openshift/library-go#785

sallyom · 2020-04-24T04:17:43Z

/retest

pkg/cli/admin/release/extract_tools.go

openshift-ci-robot · 2020-04-24T14:03:38Z

@sallyom: This pull request references Bugzilla bug 1823143, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.5.0) matches configured target release for branch (4.5.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Details

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

sallyom · 2020-05-01T21:44:41Z

@soltysh I've reworked this PR, moved all changes to pkg/cli/admin/release/info.go rather than release/extract_tools.go - this is more clear. ptal, thanks.
~~If this direction looks better, ptal here, also, so I can remove the fake bump: openshift/library-go#785~~ thanks for showing me that i do not need that change doh

soltysh · 2020-05-04T11:23:44Z

pkg/cli/admin/release/info.go

+				if err != nil {
+					errs = append(errs, err)
+					return true, nil
+				}


This could be outside of the loop.

Actually looking inside ParseReference invoked at the beginning of the method that one already has that invocation see

oc/pkg/cli/image/imagesource/reference.go

Line 96 in 29e9a33

func ParseReference(ref string) (TypedImageReference, error) {

Iow. you could just check ref.Type == DestinationRegistry and drop this above code entirely.

thanks, i've updated. much cleaner.
userGivenImage.AsRepository() == ref.Ref.AsRepository() :) so i can drop the userGivenImage nonsense altogether

soltysh · 2020-05-04T11:33:21Z

pkg/cli/admin/release/info.go

+				// try to verify user-passed image tag first, in case of mirrored images this will
+				// exist, and if so, use this instead of the release image-reference. If this doesn't
+				// exist, proceed with tag from release image-reference from the readReleaseImageReference above
+				givenTag := fmt.Sprintf("%s@%s", userGivenImage.AsRepository(), digest)


tag is confusing, since this is more imageID, imageDigest, right?

I'm a bit confused, you're creating a new reference from user-passed registry and release's image SHA, right? And then you parse it once again in verifyImageExists. If this works you replace the tag contents, correct?

I still don't understand why you can't just use the DockerImageReference returned from userGivenImage.AsRepository() invocation and set digest there. Iow.

newRef := userGivenImage.AsRepository() // this will clear both Tag and ID fields newRef.ID = digest if verifyImageExists(opts, newRef)...

This way verifyImageExists will work with already passed reference, so there's no need for double parsing.

thanks! updated, now passing imagereference.DockerImageReference to verifyImageExists, and if found, tag.From.Name = newRef.String()

soltysh · 2020-05-04T11:37:21Z

pkg/cli/admin/release/info.go

+	if err != nil {
+		if imagemanifest.IsImageNotFound(err) {
+			return false
+		}


This condition is not necessary, since in both cases you just return false.

kind of, but if imageNotFound, no logging (this means it's not a mirrored image) but for any other reason should log the error, this would indicate there was an error reading the mirrored image (it exists, but there is a problem). I'm fine not logging, too, but this was my reasoning.

openshift-ci-robot · 2020-05-04T16:19:56Z

@sallyom: This pull request references Bugzilla bug 1823143, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.5.0) matches configured target release for branch (4.5.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Details

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

sallyom · 2020-05-04T17:31:35Z

/hold cancel

fake bump removed

sallyom · 2020-05-04T21:15:56Z

/retest

smarterclayton · 2020-05-06T22:14:15Z

pkg/cli/admin/release/info.go

+			// try to verify user-passed image first, in case of mirrored images this will
+			// exist, and if so, use this instead of the release image-reference. If this doesn't
+			// exist, proceed with release image-reference from the readReleaseImageReference above
+			newRef := ref.Ref.AsRepository()


Assuming that the release image and the target share the same repository is a really ugly hack. I don't think it's appropriate without the user explicitly telling us where it is (perhaps via a flag, perhaps by providing an ICSP).

So is this "pull down any ICSPs from the cluster and teach the local oc image resolvers how to process them"? That would be great, but seems like a lot of work without vendoring more of containers/image. Or does the native oc image-resolution code already support mirrors and we'd just need to feed the ICSPs in?

... and there is no guarantee that ICSPs that make sense for the cluster also make sense for wherever the user is running oc. Would be nice if we had image-streams for every image referenced from the release image or some such, so we could pull from there without having to reach out and hit some external registry.

so ICS is printed when mirroring image like so:

imageContentSources: - mirrors: - quay.io/sallyom/release-test source: quay.io/openshift-release-dev/ocp-v4.0-art-dev - mirrors: - quay.io/sallyom/release-test source: registry.svc.ci.openshift.org/ocp/release

so if an image is mirrored pass a flag to provide that info? i don't have details for how to use this info atm, i'll take a look.

OK, that's fair and w/o that information it's ok to not to have properly working disconnected mirrors working, exactly like described in the bug. The second question is I can't seem to find which piece of containers/image is responsible for that, can you point to it?

It would be more appropriate right now to add a flag that is "--mirror-repository=foo/bar" (possibly that supports multiple flags via StringSlice) than to add magic lookup. In the future, reeading from an ICSP file or ICSP on the cluster might be reasonable. Right now, don't do magic.

Adding error path when image can't be found to suggest that if this is a mirrored repository to provide that flag, and to add a paragraph to help that describes how to work with mirrored repositories, would also be required to make this clear to users.

but how will that solve 'I can't oc adm release extract in disconnected environment'?
As/is, the cmd does that-extracts from the mirrored repository (you don't have to pass it), and succeeds if you are connected and have the correct pull secret for that repo but fails otherwise.

That flag would add additional search locations (like ICSP) if the first fetch fails. All locations that look at an image would support the alternates.

You would be designing in (vs adding hacks) a mechanism that says "if i get an image, if i can't get it at location A, try B, C, and D by digest". That mechanism probably should be in the lowest level client (the registry fetcher) which would transparently support this for all commands (i.e. putting code into mirror is probably wrong).

openshift-ci-robot · 2020-05-09T02:50:31Z

@sallyom: This pull request references Bugzilla bug 1823143, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.5.0) matches configured target release for branch (4.5.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Details

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot · 2020-05-09T02:55:21Z

@sallyom: This pull request references Bugzilla bug 1823143, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.5.0) matches configured target release for branch (4.5.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Details

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

pkg/cli/admin/release/mirror.go

… defaulting to image reference When working with mirrored release payloads, a release from a mirrored registry, mylocalregistry/ocp/release:4.5.0-0.nightly-2020-04-18-093630 mirrored from registry.svc.ci.openshift.org/ocp/release:4.5.0-0.nightly-2020-04-18-093630 - Both reference 'quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:2eb0a51...'. When extracting from the mirrored registry, try extracting from the user-given registry/repo/name If this fails, then try the release image-references. In disconnected environments, currently, oc adm release extract fails. Different from mirrored, when working with a tagged release that was uploaded to another repository, such as `quay.io/sallyom/release:new`, since that release wasn't mirrored, the underlying images still point to the original tag. In this case, `oc adm release extract --command openshift-install` will try the sallyom/release image but will fail, issue an info message, and then proceed to use the underlying tagged installer image - that will succeed as long as the user has access to the original release image.

…defaulting to image reference

openshift-ci-robot · 2020-05-18T22:48:29Z

@sallyom: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/images	`396f3d7`	link	`/test images`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

sallyom · 2020-05-19T15:54:31Z

closing in favor of #427 , it's less ugly, but maybe still too ugly

sallyom · 2020-06-01T19:55:54Z

Please see #439 and openshift/enhancements#334 for progress

openshift-ci-robot added bugzilla/medium bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Apr 22, 2020

openshift-ci-robot requested review from deads2k and mfojtik April 22, 2020 04:30

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 22, 2020

sallyom force-pushed the bz1819546 branch from 0e43116 to 9e5fde6 Compare April 22, 2020 05:50

soltysh suggested changes Apr 22, 2020

View reviewed changes

pkg/cli/admin/release/extract_tools.go Outdated Show resolved Hide resolved

sallyom force-pushed the bz1819546 branch from 9e5fde6 to a161097 Compare April 22, 2020 16:24

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 22, 2020

sallyom mentioned this pull request Apr 23, 2020

return an image digest from DockerImageReference string openshift/library-go#785

Closed

sallyom force-pushed the bz1819546 branch from a161097 to 82955f3 Compare April 23, 2020 22:20

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 23, 2020

sallyom force-pushed the bz1819546 branch from 82955f3 to f6df1af Compare April 23, 2020 22:25

sallyom commented Apr 24, 2020

View reviewed changes

pkg/cli/admin/release/extract_tools.go Outdated Show resolved Hide resolved

sallyom force-pushed the bz1819546 branch from f6df1af to a44e31e Compare April 24, 2020 13:57

sallyom force-pushed the bz1819546 branch 3 times, most recently from 329df97 to d72ae52 Compare May 2, 2020 16:28

soltysh suggested changes May 4, 2020

View reviewed changes

sallyom force-pushed the bz1819546 branch 2 times, most recently from 09f91c9 to b2d9989 Compare May 4, 2020 17:27

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 4, 2020

smarterclayton reviewed May 6, 2020

View reviewed changes

sallyom force-pushed the bz1819546 branch from b2d9989 to d176f12 Compare May 9, 2020 02:45

sallyom changed the title ~~Bug 1823143: Try user-given registry/repository for extract before defaulting to referenced image~~ Bug 1823143: Try user-given registry/repository for oc adm releaese extract|info|mirror before defaulting to referenced image May 9, 2020

sallyom commented May 15, 2020

View reviewed changes

pkg/cli/admin/release/mirror.go Show resolved Hide resolved

sallyom changed the title ~~Bug 1823143: Try user-given registry/repository for oc adm releaese extract|info|mirror before defaulting to referenced image~~ Bug 1823143: Try user-given registry/repository for oc adm release extract|info|mirror before defaulting to referenced image May 15, 2020

sallyom added 3 commits May 18, 2020 15:24

Try user-given registry/repository in 'oc adm release mirror' before …

36bb248

…defaulting to image reference

condense verifyImage

396f3d7

sallyom force-pushed the bz1819546 branch from d176f12 to 396f3d7 Compare May 18, 2020 22:33

sallyom mentioned this pull request May 19, 2020

Bug 1823143: oc adm release mirror|extract allow user to force from image prefix #427

Closed

sallyom closed this May 19, 2020

sallyom mentioned this pull request Jun 1, 2020

Bug 1823143: Implement enhancement for "Add ImageContentSource awareness to oc" #439

Closed

openshift-ci bot mentioned this pull request Apr 7, 2022

Bug 1823143: wire ICSP lookups to oc image info #829

Merged

openshift-ci bot mentioned this pull request Jun 15, 2022

Bug 1823143: add --icsp-file option to adm release subcommands #1169

Merged

Bug 1823143: Try user-given registry/repository for oc adm release extract|info|mirror before defaulting to referenced image #395

Bug 1823143: Try user-given registry/repository for oc adm release extract|info|mirror before defaulting to referenced image #395

Uh oh!

Conversation

sallyom commented Apr 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Apr 22, 2020

Uh oh!

openshift-ci-robot commented Apr 22, 2020

Uh oh!

openshift-ci-robot commented Apr 22, 2020

Uh oh!

openshift-ci-robot commented Apr 22, 2020

Uh oh!

openshift-ci-robot commented Apr 22, 2020

Uh oh!

openshift-ci-robot commented Apr 22, 2020

Uh oh!

openshift-ci-robot commented Apr 22, 2020

Uh oh!

Uh oh!

openshift-ci-robot commented Apr 22, 2020

Uh oh!

openshift-ci-robot commented Apr 22, 2020

Uh oh!

sallyom commented Apr 22, 2020

Uh oh!

openshift-ci-robot commented Apr 23, 2020

Uh oh!

sallyom commented Apr 23, 2020

Uh oh!

sallyom commented Apr 23, 2020

Uh oh!

sallyom commented Apr 24, 2020

Uh oh!

Uh oh!

openshift-ci-robot commented Apr 24, 2020

Uh oh!

sallyom commented May 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented May 4, 2020

Uh oh!

sallyom commented May 4, 2020

Uh oh!

sallyom commented May 4, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sallyom commented Apr 22, 2020 •

edited

Loading

sallyom commented May 1, 2020 •

edited

Loading

sallyom May 15, 2020 •

edited

Loading