Skip to content

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

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 3 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
61369de
Bug 1823143: Try user-given registry/repository during extract before…
sallyom Apr 20, 2020
36bb248
Try user-given registry/repository in 'oc adm release mirror' before …
sallyom May 9, 2020
396f3d7
condense verifyImage
sallyom May 18, 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
37 changes: 37 additions & 0 deletions pkg/cli/admin/release/helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
package release

import (
"context"

"k8s.io/klog"

"github.com/openshift/library-go/pkg/image/reference"
"github.com/openshift/library-go/pkg/image/registryclient"
"github.com/openshift/oc/pkg/cli/image/imagesource"
imagemanifest "github.com/openshift/oc/pkg/cli/image/manifest"
)

func verifyImageExists(fromContext *registryclient.Context, fileDir string, insecure bool, include imagemanifest.FilterFunc, ref reference.DockerImageReference) bool {
from := imagesource.TypedImageReference{Type: imagesource.DestinationRegistry, Ref: ref}
ctx := context.Background()
fromOptions := &imagesource.Options{
FileDir: fileDir,
Insecure: insecure,
RegistryContext: fromContext,
}

repo, err := fromOptions.Repository(ctx, from)
if err != nil {
klog.V(2).Infof("unable to connect to image repository %s: %v", from.String(), err)
return false
}
_, _, err = imagemanifest.FirstManifest(ctx, from.Ref, repo, include)
if err != nil {
if imagemanifest.IsImageNotFound(err) {
return false
}
klog.V(2).Infof("unable to read image %s: %v", from.String(), err)
return false
}
return true
}
26 changes: 26 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,32 @@ func (o *InfoOptions) LoadReleaseInfo(image string, retrieveImages bool) (*Relea
errs = append(errs, err)
return true, nil
}
// 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()
Copy link
Contributor

@smarterclayton smarterclayton May 6, 2020

Choose a reason for hiding this comment

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

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

Copy link
Member

@wking wking May 6, 2020

Choose a reason for hiding this comment

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

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?

Copy link
Member

@wking wking May 6, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@sallyom sallyom May 7, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@soltysh soltysh May 7, 2020

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@smarterclayton smarterclayton May 15, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@smarterclayton smarterclayton May 15, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@sallyom sallyom May 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@smarterclayton smarterclayton May 15, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@smarterclayton smarterclayton May 15, 2020

Choose a reason for hiding this comment

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

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

// Try the registry/repo passed from the user first. If image does not exist,
// proceed with the release info from the release image-references, from readReleaseImageReference above
for _, tag := range is.Spec.Tags {
// tagRef is the sha 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
tagRef, err := imagereference.Parse(tag.From.Name)
if err != nil {
continue
}
newRef.ID = tagRef.ID
// If the user-given registry/repo/name:digest exists, replace with that, if not keep the
// is.Spec.Tag from release image-reference
fromContext, err := opts.SecurityOptions.Context()
if err != nil {
return true, nil
}
if verifyImageExists(fromContext, opts.FileDir, o.SecurityOptions.Insecure, opts.FilterOptions.Include, newRef) {
tag.From.Name = newRef.String()
}
}

release.References = is
case "release-metadata":
data, err := ioutil.ReadAll(r)
Expand Down
23 changes: 22 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 @@ -579,6 +581,7 @@ func (o *MirrorOptions) Run() error {

repositories := make(map[string]struct{})

newRef := srcRef.Ref.AsRepository()
// build the mapping list for mirroring and rewrite if necessary
for i := range is.Spec.Tags {
tag := &is.Spec.Tags[i]
Expand All @@ -593,9 +596,27 @@ func (o *MirrorOptions) Run() error {
return fmt.Errorf("image-references should only contain pointers to images by digest: %s", tag.From.Name)
}

opts := mirror.NewMirrorImageOptions(genericclioptions.IOStreams{Out: o.Out, ErrOut: o.ErrOut})
opts.SecurityOptions = o.SecurityOptions
opts.ParallelOptions = o.ParallelOptions
opts.FileDir = o.ToDir
// Allow mirror refs to be sourced locally
srcMirrorRef := imagesource.TypedImageReference{Ref: from, Type: imagesource.DestinationRegistry}
srcMirrorRef = sourceFn(srcMirrorRef)
// Try the registry/repo passed from the user first. If image does not exist,
// proceed with the release info from the release image-references
// ID is the sha of each component in the release image-reference.
newRef.ID = from.ID
// If the user-given registry/repo/name:digest exists, replace with that, if not keep the
// is.Spec.Tag from release image-reference
fromContext, err := opts.SecurityOptions.Context()
if err != nil {
return err
}
if verifyImageExists(fromContext, opts.FileDir, opts.SecurityOptions.Insecure, opts.FilterOptions.Include, newRef) {
srcMirrorRef = imagesource.TypedImageReference{Ref: newRef, Type: imagesource.DestinationRegistry}
srcMirrorRef = sourceFn(srcMirrorRef)
}

// Create a unique map of repos as keys
currentRepo := from.AsRepository().String()
Expand Down