Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
sallyom wants to merge 2 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a398ec1
Bug 1823143: "force-prefix" flag to force user-given registry/reposit…
sallyom Apr 20, 2020
16d09ea
generated completions: force-prefix
sallyom May 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions contrib/completions/bash/oc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -505,6 +505,8 @@ _oc_adm_catalog_build()
flags+=("--filter-by-os=")
two_word_flags+=("--filter-by-os")
local_nonpersistent_flags+=("--filter-by-os=")
flags+=("--force-prefix")
local_nonpersistent_flags+=("--force-prefix")
flags+=("--from=")
two_word_flags+=("--from")
local_nonpersistent_flags+=("--from=")
Expand Down Expand Up @@ -597,6 +599,8 @@ _oc_adm_catalog_mirror()
flags+=("--filter-by-os=")
two_word_flags+=("--filter-by-os")
local_nonpersistent_flags+=("--filter-by-os=")
flags+=("--force-prefix")
local_nonpersistent_flags+=("--force-prefix")
flags+=("--from-dir=")
two_word_flags+=("--from-dir")
local_nonpersistent_flags+=("--from-dir=")
Expand Down Expand Up @@ -5417,6 +5421,8 @@ _oc_adm_release_extract()
flags+=("--file=")
two_word_flags+=("--file")
local_nonpersistent_flags+=("--file=")
flags+=("--force-prefix")
local_nonpersistent_flags+=("--force-prefix")
flags+=("--from=")
two_word_flags+=("--from")
local_nonpersistent_flags+=("--from=")
Expand Down Expand Up @@ -5524,6 +5530,8 @@ _oc_adm_release_info()
flags+=("--dir=")
two_word_flags+=("--dir")
local_nonpersistent_flags+=("--dir=")
flags+=("--force-prefix")
local_nonpersistent_flags+=("--force-prefix")
flags+=("--image-for=")
two_word_flags+=("--image-for")
local_nonpersistent_flags+=("--image-for=")
Expand Down Expand Up @@ -5617,6 +5625,8 @@ _oc_adm_release_mirror()
local_nonpersistent_flags+=("--apply-release-image-signature")
flags+=("--dry-run")
local_nonpersistent_flags+=("--dry-run")
flags+=("--force-prefix")
local_nonpersistent_flags+=("--force-prefix")
flags+=("--from=")
two_word_flags+=("--from")
local_nonpersistent_flags+=("--from=")
Expand Down Expand Up @@ -5729,6 +5739,8 @@ _oc_adm_release_new()
flags+=("--exclude=")
two_word_flags+=("--exclude")
local_nonpersistent_flags+=("--exclude=")
flags+=("--force-prefix")
local_nonpersistent_flags+=("--force-prefix")
flags+=("--from-dir=")
two_word_flags+=("--from-dir")
local_nonpersistent_flags+=("--from-dir=")
Expand Down Expand Up @@ -13450,6 +13462,8 @@ _oc_image_append()
local_nonpersistent_flags+=("--filter-by-os=")
flags+=("--force")
local_nonpersistent_flags+=("--force")
flags+=("--force-prefix")
local_nonpersistent_flags+=("--force-prefix")
flags+=("--from=")
two_word_flags+=("--from")
local_nonpersistent_flags+=("--from=")
Expand Down Expand Up @@ -13549,6 +13563,8 @@ _oc_image_extract()
flags+=("--filter-by-os=")
two_word_flags+=("--filter-by-os")
local_nonpersistent_flags+=("--filter-by-os=")
flags+=("--force-prefix")
local_nonpersistent_flags+=("--force-prefix")
flags+=("--insecure")
local_nonpersistent_flags+=("--insecure")
flags+=("--only-files")
Expand Down Expand Up @@ -13629,6 +13645,8 @@ _oc_image_info()
flags+=("--filter-by-os=")
two_word_flags+=("--filter-by-os")
local_nonpersistent_flags+=("--filter-by-os=")
flags+=("--force-prefix")
local_nonpersistent_flags+=("--force-prefix")
flags+=("--insecure")
local_nonpersistent_flags+=("--insecure")
flags+=("--output=")
Expand Down Expand Up @@ -13715,6 +13733,8 @@ _oc_image_mirror()
local_nonpersistent_flags+=("--filter-by-os=")
flags+=("--force")
local_nonpersistent_flags+=("--force")
flags+=("--force-prefix")
local_nonpersistent_flags+=("--force-prefix")
flags+=("--from-dir=")
two_word_flags+=("--from-dir")
local_nonpersistent_flags+=("--from-dir=")
Expand Down
20 changes: 20 additions & 0 deletions contrib/completions/zsh/oc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -647,6 +647,8 @@ _oc_adm_catalog_build()
flags+=("--filter-by-os=")
two_word_flags+=("--filter-by-os")
local_nonpersistent_flags+=("--filter-by-os=")
flags+=("--force-prefix")
local_nonpersistent_flags+=("--force-prefix")
flags+=("--from=")
two_word_flags+=("--from")
local_nonpersistent_flags+=("--from=")
Expand Down Expand Up @@ -739,6 +741,8 @@ _oc_adm_catalog_mirror()
flags+=("--filter-by-os=")
two_word_flags+=("--filter-by-os")
local_nonpersistent_flags+=("--filter-by-os=")
flags+=("--force-prefix")
local_nonpersistent_flags+=("--force-prefix")
flags+=("--from-dir=")
two_word_flags+=("--from-dir")
local_nonpersistent_flags+=("--from-dir=")
Expand Down Expand Up @@ -5559,6 +5563,8 @@ _oc_adm_release_extract()
flags+=("--file=")
two_word_flags+=("--file")
local_nonpersistent_flags+=("--file=")
flags+=("--force-prefix")
local_nonpersistent_flags+=("--force-prefix")
flags+=("--from=")
two_word_flags+=("--from")
local_nonpersistent_flags+=("--from=")
Expand Down Expand Up @@ -5666,6 +5672,8 @@ _oc_adm_release_info()
flags+=("--dir=")
two_word_flags+=("--dir")
local_nonpersistent_flags+=("--dir=")
flags+=("--force-prefix")
local_nonpersistent_flags+=("--force-prefix")
flags+=("--image-for=")
two_word_flags+=("--image-for")
local_nonpersistent_flags+=("--image-for=")
Expand Down Expand Up @@ -5759,6 +5767,8 @@ _oc_adm_release_mirror()
local_nonpersistent_flags+=("--apply-release-image-signature")
flags+=("--dry-run")
local_nonpersistent_flags+=("--dry-run")
flags+=("--force-prefix")
local_nonpersistent_flags+=("--force-prefix")
flags+=("--from=")
two_word_flags+=("--from")
local_nonpersistent_flags+=("--from=")
Expand Down Expand Up @@ -5871,6 +5881,8 @@ _oc_adm_release_new()
flags+=("--exclude=")
two_word_flags+=("--exclude")
local_nonpersistent_flags+=("--exclude=")
flags+=("--force-prefix")
local_nonpersistent_flags+=("--force-prefix")
flags+=("--from-dir=")
two_word_flags+=("--from-dir")
local_nonpersistent_flags+=("--from-dir=")
Expand Down Expand Up @@ -13592,6 +13604,8 @@ _oc_image_append()
local_nonpersistent_flags+=("--filter-by-os=")
flags+=("--force")
local_nonpersistent_flags+=("--force")
flags+=("--force-prefix")
local_nonpersistent_flags+=("--force-prefix")
flags+=("--from=")
two_word_flags+=("--from")
local_nonpersistent_flags+=("--from=")
Expand Down Expand Up @@ -13691,6 +13705,8 @@ _oc_image_extract()
flags+=("--filter-by-os=")
two_word_flags+=("--filter-by-os")
local_nonpersistent_flags+=("--filter-by-os=")
flags+=("--force-prefix")
local_nonpersistent_flags+=("--force-prefix")
flags+=("--insecure")
local_nonpersistent_flags+=("--insecure")
flags+=("--only-files")
Expand Down Expand Up @@ -13771,6 +13787,8 @@ _oc_image_info()
flags+=("--filter-by-os=")
two_word_flags+=("--filter-by-os")
local_nonpersistent_flags+=("--filter-by-os=")
flags+=("--force-prefix")
local_nonpersistent_flags+=("--force-prefix")
flags+=("--insecure")
local_nonpersistent_flags+=("--insecure")
flags+=("--output=")
Expand Down Expand Up @@ -13857,6 +13875,8 @@ _oc_image_mirror()
local_nonpersistent_flags+=("--filter-by-os=")
flags+=("--force")
local_nonpersistent_flags+=("--force")
flags+=("--force-prefix")
local_nonpersistent_flags+=("--force-prefix")
flags+=("--from-dir=")
two_word_flags+=("--from-dir")
local_nonpersistent_flags+=("--from-dir=")
Expand Down
14 changes: 14 additions & 0 deletions pkg/cli/admin/release/info.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -775,6 +775,20 @@ func (o *InfoOptions) LoadReleaseInfo(image string, retrieveImages bool) (*Relea
errs = append(errs, err)
return true, nil
}
for _, tag := range is.Spec.Tags {
// If ForcePrefix true, use user-provided image rather than its 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
Copy link
Member

@wking wking May 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@sallyom sallyom May 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if opts.SecurityOptions.ForcePrefix {
forcePrefix := ref.Ref.AsRepository().String()
_, err := imagereference.Parse(tag.From.Name)
if err == nil {
tag.From.Name = forcePrefix
}
}
}

release.References = is
case "release-metadata":
data, err := ioutil.ReadAll(r)
Expand Down
10 changes: 9 additions & 1 deletion pkg/cli/admin/release/mirror.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -517,10 +517,12 @@ func (o *MirrorOptions) Run() error {
if err := imageVerifier.Verify(ctx, releaseDigest); err != nil {
fmt.Fprintf(o.ErrOut, "warning: An image was retrieved that failed verification: %v\n", err)
}
var srcRef imagesource.TypedImageReference
var mappings []mirror.Mapping
if len(o.From) > 0 {
var err error
src := o.From
srcRef, err := imagesource.ParseReference(src)
srcRef, err = imagesource.ParseReference(src)
if err != nil {
return fmt.Errorf("invalid --from: %v", err)
}
Expand Down Expand Up @@ -593,6 +595,12 @@ func (o *MirrorOptions) Run() error {
return fmt.Errorf("image-references should only contain pointers to images by digest: %s", tag.From.Name)
}

// if ForcePrefix set, set from to be user-provided image rather than it's mirrored source
if o.SecurityOptions.ForcePrefix {
forcePrefix := srcRef.Ref.AsRepository()
forcePrefix.ID = from.ID
from = forcePrefix
}
// Allow mirror refs to be sourced locally
srcMirrorRef := imagesource.TypedImageReference{Ref: from, Type: imagesource.DestinationRegistry}
srcMirrorRef = sourceFn(srcMirrorRef)
Expand Down
2 changes: 2 additions & 0 deletions pkg/cli/image/manifest/manifest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,14 @@ type SecurityOptions struct {
RegistryConfig string
Insecure bool
SkipVerification bool
ForcePrefix bool

CachedContext *registryclient.Context
}

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")
Copy link
Member

@wking wking May 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ....

Copy link
Contributor Author

@sallyom sallyom May 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

flags.BoolVar(&o.Insecure, "insecure", o.Insecure, "Allow push and pull operations to registries to be made over HTTP")
flags.BoolVar(&o.SkipVerification, "skip-verification", o.SkipVerification, "Skip verifying the integrity of the retrieved content. This is not recommended, but may be necessary when importing images from older image registries. Only bypass verification if the registry is known to be trustworthy.")
}
Expand Down