Skip to content
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

Fix pulling of images within buildah #1319

Closed
wants to merge 4 commits into from
Closed

Fix pulling of images within buildah #1319

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8125c1a
Fix pulling of images within buildah
rhatdan Feb 2, 2019
1854716
Support oci layout format
rhatdan Feb 19, 2019
525cb47
Make sure buildah pull --all-tags only works with docker transport
rhatdan Feb 19, 2019
ea5f858
Remove quiet option from pull options
rhatdan Feb 20, 2019
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
5 changes: 0 additions & 5 deletions buildah.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,11 +336,6 @@ type BuilderOptions struct {
// needs to be pulled and the image name alone can not be resolved to a
// reference to a source image. No separator is implicitly added.
Registry string
// Transport is a value which is prepended to the image's name, if it
// needs to be pulled and the image name alone, or the image name and
// the registry together, can not be resolved to a reference to a
// source image. No separator is implicitly added.
Transport string
Copy link
Member

Choose a reason for hiding this comment

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

Just wondering why you switched this to lower case? Also in pull.go. I understand the scoping, but none of the other variables in these structs are lowercase. In some of the code we have a number of variables named transport(s) being used and having this upper case helps it to stand out better.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't want callers to use this field. We use it to pass the information around withing buildah library, but specifying transport as well as imagename can cause conflicts.
Bottom line, I only want transport derived in one place and from ImageName. Before we had it being derived in different places,and not the same way.

// PullBlobDirectory is the name of a directory in which we'll attempt
// to store copies of layer blobs that we pull down, if any. It should
// already exist.
Expand Down
2 changes: 1 addition & 1 deletion cmd/buildah/commit.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ func commitCmd(c *cobra.Command, args []string, iopts commitInputOptions) error

dest, err := alltransports.ParseImageName(image)
if err != nil {
candidates, _, err := util.ResolveName(image, "", systemContext, store)
candidates, _, _, err := util.ResolveName(image, "", systemContext, store)
if err != nil {
return errors.Wrapf(err, "error parsing target image name %q", image)
}
Expand Down
10 changes: 0 additions & 10 deletions cmd/buildah/from.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import (
"github.com/containers/buildah"
buildahcli "github.com/containers/buildah/pkg/cli"
"github.com/containers/buildah/pkg/parse"
"github.com/containers/buildah/util"
"github.com/pkg/errors"
"github.com/sirupsen/logrus"
"github.com/spf13/cobra"
Expand Down Expand Up @@ -189,14 +188,6 @@ func fromCmd(c *cobra.Command, args []string, iopts fromReply) error {
return err
}

transport := util.DefaultTransport
arr := strings.SplitN(args[0], ":", 2)
if len(arr) == 2 {
if _, ok := util.Transports[arr[0]]; ok {
transport = arr[0]
}
}

mtrmac marked this conversation as resolved.
Show resolved Hide resolved
isolation, err := parse.IsolationOption(c)
if err != nil {
return err
Expand All @@ -219,7 +210,6 @@ func fromCmd(c *cobra.Command, args []string, iopts fromReply) error {

options := buildah.BuilderOptions{
FromImage: args[0],
Transport: transport,
Container: iopts.name,
PullPolicy: pullPolicy,
SignaturePolicyPath: signaturePolicy,
Expand Down
20 changes: 4 additions & 16 deletions cmd/buildah/pull.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,10 @@ package main

import (
"os"
"strings"

"github.com/containers/buildah"
buildahcli "github.com/containers/buildah/pkg/cli"
"github.com/containers/buildah/pkg/parse"
"github.com/containers/buildah/util"
"github.com/pkg/errors"
"github.com/spf13/cobra"
)
Expand Down Expand Up @@ -81,27 +79,17 @@ func pullCmd(c *cobra.Command, args []string, iopts pullResults) error {
return err
}

transport := util.DefaultTransport
arr := strings.SplitN(args[0], ":", 2)
if len(arr) == 2 {
if iopts.allTags {
return errors.Errorf("tag can't be used with --all-tags")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This check seems useful and worth preserving somewhere (though this implementation did not made little sense). AFAICS util.ResolveNames does not add the implicit :latest tag, so the check could be moved closer to the PullOptions.AllTags implementation .

Copy link
Member Author

Choose a reason for hiding this comment

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

This is tough to check, and seem fairly arbitrary. If I state I want to pull --alltags on alpine:latest, I don't see this as a big conflict.

if _, ok := util.Transports[arr[0]]; ok {
transport = arr[0]
}
}

options := buildah.PullOptions{
Transport: transport,
SignaturePolicyPath: iopts.signaturePolicy,
Store: store,
SystemContext: systemContext,
BlobDirectory: iopts.blobCache,
AllTags: iopts.allTags,
ReportWriter: os.Stderr,
}
if !iopts.quiet {
options.ReportWriter = os.Stderr

if iopts.quiet {
options.ReportWriter = nil // Turns off logging output
}

return buildah.Pull(getContext(), args[0], options)
Expand Down
3 changes: 3 additions & 0 deletions docs/buildah-from.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@ Multiple transports are supported:
**docker-daemon:**_docker-reference_
An image _docker-reference_ stored in the docker daemon's internal storage. _docker-reference_ must include either a tag or a digest. Alternatively, when reading images, the format can also be docker-daemon:algo:digest (an image ID).

**oci:**_path_**:**_tag_**
An image tag in a directory compliant with "Open Container Image Layout Specification" at _path_.

**oci-archive:**_path_**:**_tag_
An image _tag_ in a directory compliant with "Open Container Image Layout Specification" at _path_.

Expand Down
3 changes: 3 additions & 0 deletions docs/buildah-pull.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@ Multiple transports are supported:
**docker-daemon:**_docker-reference_
An image _docker-reference_ stored in the docker daemon's internal storage. _docker-reference_ must include either a tag or a digest. Alternatively, when reading images, the format can also be docker-daemon:algo:digest (an image ID).

**oci:**_path_**:**_tag_**
An image tag in a directory compliant with "Open Container Image Layout Specification" at _path_.

**oci-archive:**_path_**:**_tag_
An image _tag_ in a directory compliant with "Open Container Image Layout Specification" at _path_.

Expand Down
10 changes: 1 addition & 9 deletions imagebuildah/build.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,6 @@ type BuildOptions struct {
// needs to be pulled and the image name alone can not be resolved to a
// reference to a source image. No separator is implicitly added.
Registry string
// Transport is a value which is prepended to the image's name, if it
// needs to be pulled and the image name alone, or the image name and
// the registry together, can not be resolved to a reference to a
// source image. No separator is implicitly added.
Transport string
// IgnoreUnrecognizedInstructions tells us to just log instructions we
// don't recognize, and try to keep going.
IgnoreUnrecognizedInstructions bool
Expand Down Expand Up @@ -186,7 +181,6 @@ type Executor struct {
builder *buildah.Builder
pullPolicy buildah.PullPolicy
registry string
transport string
ignoreUnrecognizedInstructions bool
quiet bool
runtime string
Expand Down Expand Up @@ -582,7 +576,6 @@ func NewExecutor(store storage.Store, options BuildOptions) (*Executor, error) {
contextDir: options.ContextDirectory,
pullPolicy: options.PullPolicy,
registry: options.Registry,
transport: options.Transport,
mtrmac marked this conversation as resolved.
Show resolved Hide resolved
ignoreUnrecognizedInstructions: options.IgnoreUnrecognizedInstructions,
quiet: options.Quiet,
runtime: options.Runtime,
Expand Down Expand Up @@ -672,7 +665,6 @@ func (b *Executor) Prepare(ctx context.Context, stage imagebuilder.Stage, from s
FromImage: from,
PullPolicy: b.pullPolicy,
Registry: b.registry,
Transport: b.transport,
PullBlobDirectory: b.blobDirectory,
SignaturePolicyPath: b.signaturePolicyPath,
ReportWriter: b.reportWriter,
Expand Down Expand Up @@ -785,7 +777,7 @@ func (b *Executor) resolveNameToImageRef() (types.ImageReference, error) {
if b.output != "" {
imageRef, err = alltransports.ParseImageName(b.output)
if err != nil {
candidates, _, err := util.ResolveName(b.output, "", b.systemContext, b.store)
candidates, _, _, err := util.ResolveName(b.output, "", b.systemContext, b.store)
if err != nil {
return nil, errors.Wrapf(err, "error parsing target image name %q", b.output)
}
Expand Down
68 changes: 33 additions & 35 deletions new.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,14 @@ const (
minimumTruncatedIDLength = 3
)

func pullAndFindImage(ctx context.Context, store storage.Store, imageName string, options BuilderOptions, sc *types.SystemContext) (*storage.Image, types.ImageReference, error) {
func pullAndFindImage(ctx context.Context, store storage.Store, transport string, imageName string, options BuilderOptions, sc *types.SystemContext) (*storage.Image, types.ImageReference, error) {
pullOptions := PullOptions{
ReportWriter: options.ReportWriter,
Store: store,
SystemContext: options.SystemContext,
Transport: options.Transport,
BlobDirectory: options.PullBlobDirectory,
}
ref, err := pullImage(ctx, store, imageName, pullOptions, sc)
ref, err := pullImage(ctx, store, transport, imageName, pullOptions, sc)
if err != nil {
logrus.Debugf("error pulling image %q: %v", imageName, err)
return nil, nil, err
Expand Down Expand Up @@ -101,55 +100,56 @@ func newContainerIDMappingOptions(idmapOptions *IDMappingOptions) storage.IDMapp
return options
}

func resolveImage(ctx context.Context, systemContext *types.SystemContext, store storage.Store, options BuilderOptions) (types.ImageReference, *storage.Image, error) {
func resolveImage(ctx context.Context, systemContext *types.SystemContext, store storage.Store, options BuilderOptions) (types.ImageReference, string, *storage.Image, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

(Ultimately I’d like more of the code to deal with the source as a types.ImageReference instead of transport+name strings, like libpod’s pullRefPair, but this is still a big improvement over the previous state.)

type failure struct {
resolvedImageName string
err error
}

candidates, searchRegistriesWereUsedButEmpty, err := util.ResolveName(options.FromImage, options.Registry, systemContext, store)
candidates, transport, searchRegistriesWereUsedButEmpty, err := util.ResolveName(options.FromImage, options.Registry, systemContext, store)
if err != nil {
return nil, nil, errors.Wrapf(err, "error parsing reference to image %q", options.FromImage)
return nil, "", nil, errors.Wrapf(err, "error parsing reference to image %q", options.FromImage)
}

failures := []failure{}
for _, image := range candidates {
var err error
if len(image) >= minimumTruncatedIDLength {
if img, err := store.Image(image); err == nil && img != nil && strings.HasPrefix(img.ID, image) {
ref, err := is.Transport.ParseStoreReference(store, img.ID)
if err != nil {
return nil, nil, errors.Wrapf(err, "error parsing reference to image %q", img.ID)
return nil, "", nil, errors.Wrapf(err, "error parsing reference to image %q", img.ID)
}
return ref, img, nil
return ref, transport, img, nil
}
}

if options.PullPolicy == PullAlways {
pulledImg, pulledReference, err := pullAndFindImage(ctx, store, image, options, systemContext)
pulledImg, pulledReference, err := pullAndFindImage(ctx, store, transport, image, options, systemContext)
if err != nil {
logrus.Debugf("unable to pull and read image %q: %v", image, err)
failures = append(failures, failure{resolvedImageName: image, err: err})
continue
}
return pulledReference, pulledImg, nil
return pulledReference, transport, pulledImg, nil
}

srcRef, err := alltransports.ParseImageName(image)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don’t think we even need to try ParseImageName(image): if util.ResolveName detects a valid transport:name value, it now always splits it into (image, transport) values that need to be reassembled here.

Parsing only the name part as transport2:name2 could, in principle, misinterpret the input.

Just always use the if err != nil path below, combining transport and image back again.

(Yes, this looks convoluted. util.ResolveName would ideally return a types.ImageReference already, but that would require updating the code that converts the return value into a storage reference, expanding the scope of this PR even more. Let’s defer that, this is already a big improvement.)

(All of this is non-blocking, the ambiguity existed before this PR; this PR only makes the results of ResolveName consistent enough that it can now actually be fixed.)

if err != nil {
if options.Transport == "" {
if transport == "" {
logrus.Debugf("error parsing image name %q: %v", image, err)
failures = append(failures, failure{
resolvedImageName: image,
err: errors.Wrapf(err, "error parsing image name"),
})
continue
}
logrus.Debugf("error parsing image name %q as given, trying with transport %q: %v", image, options.Transport, err)
transport := options.Transport
logrus.Debugf("error parsing image name %q as given, trying with transport %q: %v", image, transport, err)

trans := transport
if transport != util.DefaultTransport {
transport = transport + ":"
trans = trans + ":"
}
srcRef2, err := alltransports.ParseImageName(transport + image)
srcRef2, err := alltransports.ParseImageName(trans + image)
if err != nil {
logrus.Debugf("error parsing image name %q: %v", transport+image, err)
failures = append(failures, failure{
Expand All @@ -163,19 +163,19 @@ func resolveImage(ctx context.Context, systemContext *types.SystemContext, store

destImage, err := localImageNameForReference(ctx, store, srcRef, options.FromImage)
if err != nil {
return nil, nil, errors.Wrapf(err, "error computing local image name for %q", transports.ImageName(srcRef))
return nil, "", nil, errors.Wrapf(err, "error computing local image name for %q", transports.ImageName(srcRef))
}
if destImage == "" {
return nil, nil, errors.Errorf("error computing local image name for %q", transports.ImageName(srcRef))
return nil, "", nil, errors.Errorf("error computing local image name for %q", transports.ImageName(srcRef))
}

ref, err := is.Transport.ParseStoreReference(store, destImage)
if err != nil {
return nil, nil, errors.Wrapf(err, "error parsing reference to image %q", destImage)
return nil, "", nil, errors.Wrapf(err, "error parsing reference to image %q", destImage)
}
img, err := is.Transport.GetStoreImage(store, ref)
if err == nil {
return ref, img, nil
return ref, transport, img, nil
}

if errors.Cause(err) == storage.ErrImageUnknown && options.PullPolicy != PullIfMissing {
Expand All @@ -187,26 +187,26 @@ func resolveImage(ctx context.Context, systemContext *types.SystemContext, store
continue
}

pulledImg, pulledReference, err := pullAndFindImage(ctx, store, image, options, systemContext)
pulledImg, pulledReference, err := pullAndFindImage(ctx, store, transport, image, options, systemContext)
if err != nil {
logrus.Debugf("unable to pull and read image %q: %v", image, err)
failures = append(failures, failure{resolvedImageName: image, err: err})
continue
}
return pulledReference, pulledImg, nil
return pulledReference, transport, pulledImg, nil
}

if len(failures) != len(candidates) {
return nil, nil, fmt.Errorf("internal error: %d candidates (%#v) vs. %d failures (%#v)", len(candidates), candidates, len(failures), failures)
return nil, "", nil, fmt.Errorf("internal error: %d candidates (%#v) vs. %d failures (%#v)", len(candidates), candidates, len(failures), failures)
}

registriesConfPath := sysregistries.RegistriesConfPath(systemContext)
switch len(failures) {
case 0:
if searchRegistriesWereUsedButEmpty {
return nil, nil, errors.Errorf("image name %q is a short name and no search registries are defined in %s.", options.FromImage, registriesConfPath)
return nil, "", nil, errors.Errorf("image name %q is a short name and no search registries are defined in %s.", options.FromImage, registriesConfPath)
}
return nil, nil, fmt.Errorf("internal error: no pull candidates were available for %q for an unknown reason", options.FromImage)
return nil, "", nil, fmt.Errorf("internal error: no pull candidates were available for %q for an unknown reason", options.FromImage)

case 1:
err := failures[0].err
Expand All @@ -216,15 +216,15 @@ func resolveImage(ctx context.Context, systemContext *types.SystemContext, store
if searchRegistriesWereUsedButEmpty {
err = errors.Wrapf(err, "(image name %q is a short name and no search registries are defined in %s)", options.FromImage, registriesConfPath)
}
return nil, nil, err
return nil, "", nil, err

default:
// NOTE: a multi-line error string:
e := fmt.Sprintf("The following failures happened while trying to pull image specified by %q based on search registries in %s:", options.FromImage, registriesConfPath)
for _, f := range failures {
e = e + fmt.Sprintf("\n* %q: %s", f.resolvedImageName, f.err.Error())
}
return nil, nil, errors.New(e)
return nil, "", nil, errors.New(e)
}
}

Expand All @@ -250,21 +250,19 @@ func findUnusedContainer(name string, containers []storage.Container) string {
}

func newBuilder(ctx context.Context, store storage.Store, options BuilderOptions) (*Builder, error) {
var ref types.ImageReference
var img *storage.Image
var err error

var (
ref types.ImageReference
img *storage.Image
err error
)
if options.FromImage == BaseImageFakeName {
options.FromImage = ""
}
if options.Transport == "" {
options.Transport = util.DefaultTransport
}

systemContext := getSystemContext(options.SystemContext, options.SignaturePolicyPath)

if options.FromImage != "" && options.FromImage != "scratch" {
ref, img, err = resolveImage(ctx, systemContext, store, options)
ref, _, img, err = resolveImage(ctx, systemContext, store, options)
if err != nil {
return nil, err
}
Expand Down
Loading