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

sallyom · 2020-05-19T15:36:43Z

@smarterclayton while thinking about the flag suggestion from #395 I quickly modified that PR to use a flag instead. I'm still looking at how this can happen at a lower level, but this is less ugly of a hack - still too ugly, though?

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-05-18-131223 --to localhost:5000/ocp/release
$ oc adm release --insecure=true extract --force-prefix=true --command openshift-install localhost:5000/ocp/release:4.5.0-0.nightly-2020-05-18-131223 -v=2

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

openshift-ci-robot · 2020-05-19T15:37:01Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sallyom
To complete the pull request process, please assign tnozicka
You can assign the PR to them by writing /assign @tnozicka 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-05-19T15:55:01Z

@sallyom: This pull request references Bugzilla bug 1823143, which is valid. 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 POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Details

In response to this:

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

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-19T15:56: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:

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

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.

wking · 2020-05-19T18:20:22Z

To test this PR, do something like this...

But then your suggestion doesn't mention --force-prefix?

wking · 2020-05-19T18:20:47Z

pkg/cli/admin/release/info.go

 				return true, nil
 			}
+			for _, tag := range is.Spec.Tags {
+				// If ForcePrefix true, use user-provided image rather than it's mirrored source


nit: "it's" -> "its"

To test this PR, do something like this...

But then your suggestion doesn't mention --force-prefix?

oops, fixed.. plus, I am going to rework this to use ICSP, but wanted to push this in the meantime bc it's simpler than the other PR.

nit: "it's" -> "its"

thanks, fixed, that one always gets me

wking · 2020-05-19T18:22:50Z

pkg/cli/admin/release/info.go

+				// If ForcePrefix true, use user-provided image rather than it's mirrored source
+				// imagereference.Parse returns the digest ID of each component in the release image-reference.
+				// If can't get digest ID, skip this tag, this happens when user has built a payload by
+				// replacing component images in the release with a new image


Is this "user builds a release replacing a component image with a by-tag pullspec"? Can we forbid that? I would hope users replacing component images would use by-digest pullspecs, and expect the new release image to include digests for those images too.

When I create a release payload w/ a substituted image I pass something like cli=quay.io/sallyom/cli:test I think that's what most devs would do? That's the case I added that skip for. 🤷 I figured the best thing to do there is ignore this tag(component image) and if the user has access to that, all good, but if not, that's on them anyways

pkg/cli/admin/release/mirror.go

openshift-ci-robot · 2020-05-19T18:25: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:

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

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.

wking · 2020-05-19T18:27:17Z

pkg/cli/image/manifest/manifest.go


 func (o *SecurityOptions) Bind(flags *pflag.FlagSet) {
 	flags.StringVarP(&o.RegistryConfig, "registry-config", "a", o.RegistryConfig, "Path to your registry credentials (defaults to ~/.docker/config.json)")
+	flags.BoolVar(&o.ForcePrefix, "force-prefix", o.ForcePrefix, "Force lookup of named prefix (registry/repository/name) for an image source")


It feels strange to me to have this flag down on oc image info ... and such but have handling only up under oc adm release ....

yes, wasn't sure where to place the flag. I was going to take suggestions. I'm reworking this to use the ImageContentSourcePolicy information, though, and possibly remove the need for a user to pass a flag at all - working on that now

…ory/name instead of defaulting to mirrored source from image reference When extracting from the mirrored registry, pass "force-prefix=true" to force lookup of user-given registry/repo/name instead of mirrored source image reference. In disconnected environments, currently, oc adm release extract fails. 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...'. In case of disconnected, pass 'force-prefix=true' to use prefix 'mylocalregistry/ocp/release' instead of 'quay.io/openshift-release-dev/ocp-v4.0-art-dev'

sallyom · 2020-05-19T18:34:31Z

To test this PR, do something like this...

But then your suggestion doesn't mention --force-prefix?

fixed it, added to the description, thanks, and oops

openshift-ci-robot · 2020-05-19T20:28:00Z

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

Test name	Commit	Details	Rerun command
ci/prow/e2e-cmd	`16d09ea`	link	`/test e2e-cmd`
ci/prow/e2e-aws	`16d09ea`	link	`/test e2e-aws`

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.

soltysh · 2020-05-21T13:37:15Z

/hold
this is being discussed in openshift/enhancements#334

sallyom · 2020-06-01T04:49:16Z

Direction of this PR has changed, closing this in favor of #439 and openshift/enhancements#334

openshift-ci-robot · 2020-06-01T04:49:19Z

@sallyom: This pull request references Bugzilla bug 1823143. The bug has been updated to no longer refer to the pull request using the external bug tracker.

Details

In response to this:

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

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 requested review from deads2k and mfojtik May 19, 2020 15:37

sallyom mentioned this pull request May 19, 2020

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

Closed

sallyom changed the title ~~oc adm release mirror|extract allow user to force from image prefix~~ Bug 1823143: oc adm release mirror|extract allow user to force from image prefix May 19, 2020

openshift-ci-robot added bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels May 19, 2020

wking reviewed May 19, 2020

View reviewed changes

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

wking reviewed May 19, 2020

View reviewed changes

sallyom added 2 commits May 19, 2020 14:28

generated completions: force-prefix

16d09ea

sallyom force-pushed the bug1823143 branch from 34835d9 to 16d09ea Compare May 19, 2020 18:28

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

sallyom mentioned this pull request May 26, 2020

Improve oc awareness of ImageContentSourcePolicy openshift/enhancements#334

Merged

sallyom closed this Jun 1, 2020

sallyom mentioned this pull request Jun 1, 2020

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

Closed

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

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

Uh oh!

Conversation

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

Uh oh!

Uh oh!

openshift-ci-robot commented May 19, 2020

Uh oh!

openshift-ci-robot commented May 19, 2020

Uh oh!

openshift-ci-robot commented May 19, 2020

Uh oh!

wking commented May 19, 2020

Uh oh!

wking May 19, 2020

Choose a reason for hiding this comment

Uh oh!

sallyom May 19, 2020

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wking May 19, 2020

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

openshift-ci-robot commented May 19, 2020

Uh oh!

wking May 19, 2020

Choose a reason for hiding this comment

Uh oh!

sallyom May 19, 2020

Choose a reason for hiding this comment

Uh oh!

sallyom commented May 19, 2020

Uh oh!

openshift-ci-robot commented May 19, 2020

Uh oh!

soltysh commented May 21, 2020

Uh oh!

sallyom commented Jun 1, 2020

Uh oh!

openshift-ci-robot commented Jun 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sallyom commented May 19, 2020 •

edited

Loading

sallyom May 19, 2020 •

edited

Loading

sallyom May 19, 2020 •

edited

Loading