From 39f4cfb79d9ea4d462b97ee73babca973d756cb5 Mon Sep 17 00:00:00 2001 From: Daniel J Walsh Date: Thu, 15 Oct 2020 05:16:50 -0400 Subject: [PATCH] Stop excessive wrapping Golang built in functions like os.Create and others print the name of the file system object when they fail. Wrapping them a second time with the file system object, makes the error message look like crap when reported to the user. Signed-off-by: Daniel J Walsh --- add.go | 6 +-- buildah.go | 8 ++-- commit.go | 2 +- image.go | 2 +- imagebuildah/build.go | 10 ++--- imagebuildah/chroot_symlink_linux.go | 12 +++--- imagebuildah/stage_executor.go | 28 ++++++------- imagebuildah/util.go | 6 +-- run_linux.go | 59 ++++++++++++++-------------- tests/bud.bats | 8 +++- util.go | 2 +- 11 files changed, 74 insertions(+), 69 deletions(-) diff --git a/add.go b/add.go index 45b5c6a94..80ee0d912 100644 --- a/add.go +++ b/add.go @@ -74,11 +74,11 @@ func sourceIsRemote(source string) bool { func getURL(src, mountpoint, renameTarget string, writer io.Writer) error { url, err := url.Parse(src) if err != nil { - return errors.Wrapf(err, "error parsing URL %q", url) + return err } response, err := http.Get(src) if err != nil { - return errors.Wrapf(err, "error parsing URL %q", url) + return err } defer response.Body.Close() // Figure out what to name the new content. @@ -93,7 +93,7 @@ func getURL(src, mountpoint, renameTarget string, writer io.Writer) error { if lastModified != "" { d, err := time.Parse(time.RFC1123, lastModified) if err != nil { - return errors.Wrapf(err, "error parsing last-modified time %q", lastModified) + return errors.Wrapf(err, "error parsing last-modified time") } date = d } diff --git a/buildah.go b/buildah.go index 369e4e665..90941878a 100644 --- a/buildah.go +++ b/buildah.go @@ -453,7 +453,7 @@ func OpenBuilder(store storage.Store, container string) (*Builder, error) { } buildstate, err := ioutil.ReadFile(filepath.Join(cdir, stateFile)) if err != nil { - return nil, errors.Wrapf(err, "error reading %q", filepath.Join(cdir, stateFile)) + return nil, err } b := &Builder{} if err = json.Unmarshal(buildstate, &b); err != nil { @@ -476,7 +476,7 @@ func OpenBuilderByPath(store storage.Store, path string) (*Builder, error) { } abs, err := filepath.Abs(path) if err != nil { - return nil, errors.Wrapf(err, "error turning %q into an absolute path", path) + return nil, err } builderMatchesPath := func(b *Builder, path string) bool { return (b.MountPoint == path) @@ -492,7 +492,7 @@ func OpenBuilderByPath(store storage.Store, path string) (*Builder, error) { logrus.Debugf("error reading %q: %v, ignoring container %q", filepath.Join(cdir, stateFile), err, container.ID) continue } - return nil, errors.Wrapf(err, "error reading %q", filepath.Join(cdir, stateFile)) + return nil, err } b := &Builder{} err = json.Unmarshal(buildstate, &b) @@ -528,7 +528,7 @@ func OpenAllBuilders(store storage.Store) (builders []*Builder, err error) { logrus.Debugf("error reading %q: %v, ignoring container %q", filepath.Join(cdir, stateFile), err, container.ID) continue } - return nil, errors.Wrapf(err, "error reading %q", filepath.Join(cdir, stateFile)) + return nil, err } b := &Builder{} err = json.Unmarshal(buildstate, &b) diff --git a/commit.go b/commit.go index 74db32f7e..7a57021aa 100644 --- a/commit.go +++ b/commit.go @@ -419,7 +419,7 @@ func (b *Builder) Commit(ctx context.Context, dest types.ImageReference, options } if options.IIDFile != "" { if err = ioutil.WriteFile(options.IIDFile, []byte(img.ID), 0644); err != nil { - return imgID, nil, "", errors.Wrapf(err, "failed to write image ID to file %q", options.IIDFile) + return imgID, nil, "", err } } } diff --git a/image.go b/image.go index b2c95fecd..154bc503f 100644 --- a/image.go +++ b/image.go @@ -620,7 +620,7 @@ func (i *containerImageRef) Transport() types.ImageTransport { func (i *containerImageSource) Close() error { err := os.RemoveAll(i.path) if err != nil { - return errors.Wrapf(err, "error removing layer blob directory %q", i.path) + return errors.Wrapf(err, "error removing layer blob directory") } return nil } diff --git a/imagebuildah/build.go b/imagebuildah/build.go index ba08a2c88..76dfeaf54 100644 --- a/imagebuildah/build.go +++ b/imagebuildah/build.go @@ -208,7 +208,7 @@ func BuildDockerfiles(ctx context.Context, store storage.Store, options BuildOpt logrus.Debugf("reading remote Dockerfile %q", dfile) resp, err := http.Get(dfile) if err != nil { - return "", nil, errors.Wrapf(err, "error getting %q", dfile) + return "", nil, err } if resp.ContentLength == 0 { resp.Body.Close() @@ -237,7 +237,7 @@ func BuildDockerfiles(ctx context.Context, store storage.Store, options BuildOpt logrus.Debugf("reading local Dockerfile %q", dfile) contents, err := os.Open(dfile) if err != nil { - return "", nil, errors.Wrapf(err, "error reading %q", dfile) + return "", nil, err } dinfo, err = contents.Stat() if err != nil { @@ -246,7 +246,7 @@ func BuildDockerfiles(ctx context.Context, store storage.Store, options BuildOpt } if dinfo.Mode().IsRegular() && dinfo.Size() == 0 { contents.Close() - return "", nil, errors.Wrapf(err, "no contents in %q", dfile) + return "", nil, errors.Errorf("no contents in %q", dfile) } data = contents } @@ -265,7 +265,7 @@ func BuildDockerfiles(ctx context.Context, store storage.Store, options BuildOpt mainNode, err := imagebuilder.ParseDockerfile(dockerfiles[0]) if err != nil { - return "", nil, errors.Wrapf(err, "error parsing main Dockerfile") + return "", nil, errors.Wrapf(err, "error parsing main Dockerfile: %s", dockerfiles[0]) } warnOnUnsetBuildArgs(mainNode, options.Args) @@ -273,7 +273,7 @@ func BuildDockerfiles(ctx context.Context, store storage.Store, options BuildOpt for _, d := range dockerfiles[1:] { additionalNode, err := imagebuilder.ParseDockerfile(d) if err != nil { - return "", nil, errors.Wrapf(err, "error parsing additional Dockerfile") + return "", nil, errors.Wrapf(err, "error parsing additional Dockerfile %s", d) } mainNode.Children = append(mainNode.Children, additionalNode.Children...) } diff --git a/imagebuildah/chroot_symlink_linux.go b/imagebuildah/chroot_symlink_linux.go index 2fb10ab83..4dd49130d 100644 --- a/imagebuildah/chroot_symlink_linux.go +++ b/imagebuildah/chroot_symlink_linux.go @@ -90,7 +90,7 @@ func getSymbolicLink(path string) (string, error) { symPath = filepath.Join(symPath, p) isSymlink, resolvedPath, err := hasSymlink(symPath) if err != nil { - return "", errors.Wrapf(err, "error checking symlink for %q", symPath) + return "", err } // if isSymlink is true, check if resolvedPath is potentially another symlink // keep doing this till resolvedPath is not a symlink and isSymlink is false @@ -102,7 +102,7 @@ func getSymbolicLink(path string) (string, error) { } isSymlink, resolvedPath, err = hasSymlink(resolvedPath) if err != nil { - return "", errors.Wrapf(err, "error checking symlink for %q", resolvedPath) + return "", err } symLinksResolved++ } @@ -121,14 +121,14 @@ func hasSymlink(path string) (bool, string, error) { if err != nil { if os.IsNotExist(err) { if err = os.MkdirAll(path, 0755); err != nil { - return false, "", errors.Wrapf(err, "error ensuring volume path %q exists", path) + return false, "", err } info, err = os.Lstat(path) if err != nil { - return false, "", errors.Wrapf(err, "error running lstat on %q", path) + return false, "", err } } else { - return false, path, errors.Wrapf(err, "error get stat of path %q", path) + return false, path, err } } @@ -140,7 +140,7 @@ func hasSymlink(path string) (bool, string, error) { // Read the symlink to get what it points to targetDir, err := os.Readlink(path) if err != nil { - return false, "", errors.Wrapf(err, "error reading link %q", path) + return false, "", err } // if the symlink points to a relative path, prepend the path till now to the resolved path if !filepath.IsAbs(targetDir) { diff --git a/imagebuildah/stage_executor.go b/imagebuildah/stage_executor.go index e13de36c4..e157bb1c8 100644 --- a/imagebuildah/stage_executor.go +++ b/imagebuildah/stage_executor.go @@ -81,7 +81,7 @@ func (s *StageExecutor) Preserve(path string) error { // except ensure that it exists. archivedPath := filepath.Join(s.mountPoint, path) if err := os.MkdirAll(archivedPath, 0755); err != nil { - return errors.Wrapf(err, "error ensuring volume path %q exists", archivedPath) + return errors.Wrapf(err, "error ensuring volume path exists") } if err := s.volumeCacheInvalidate(path); err != nil { return errors.Wrapf(err, "error ensuring volume path %q is preserved", archivedPath) @@ -110,13 +110,13 @@ func (s *StageExecutor) Preserve(path string) error { st, err := os.Stat(archivedPath) if os.IsNotExist(err) { if err = os.MkdirAll(archivedPath, 0755); err != nil { - return errors.Wrapf(err, "error ensuring volume path %q exists", archivedPath) + return errors.Wrapf(err, "error ensuring volume path exists") } st, err = os.Stat(archivedPath) } if err != nil { logrus.Debugf("error reading info about %q: %v", archivedPath, err) - return errors.Wrapf(err, "error reading info about volume path %q", archivedPath) + return err } s.volumeCacheInfo[path] = st if !s.volumes.Add(path) { @@ -152,7 +152,7 @@ func (s *StageExecutor) Preserve(path string) error { if os.IsNotExist(err) { continue } - return errors.Wrapf(err, "error removing %q", s.volumeCache[cachedPath]) + return err } delete(s.volumeCache, cachedPath) } @@ -173,7 +173,7 @@ func (s *StageExecutor) volumeCacheInvalidate(path string) error { if os.IsNotExist(err) { continue } - return errors.Wrapf(err, "error removing volume cache %q", s.volumeCache[cachedPath]) + return err } archivedPath := filepath.Join(s.mountPoint, cachedPath) logrus.Debugf("invalidated volume cache for %q from %q", archivedPath, s.volumeCache[cachedPath]) @@ -193,15 +193,15 @@ func (s *StageExecutor) volumeCacheSave() error { continue } if !os.IsNotExist(err) { - return errors.Wrapf(err, "error checking for cache of %q in %q", archivedPath, cacheFile) + return err } if err := os.MkdirAll(archivedPath, 0755); err != nil { - return errors.Wrapf(err, "error ensuring volume path %q exists", archivedPath) + return errors.Wrapf(err, "error ensuring volume path exists") } logrus.Debugf("caching contents of volume %q in %q", archivedPath, cacheFile) cache, err := os.Create(cacheFile) if err != nil { - return errors.Wrapf(err, "error creating archive at %q", cacheFile) + return err } defer cache.Close() rc, err := archive.Tar(archivedPath, archive.Uncompressed) @@ -224,14 +224,14 @@ func (s *StageExecutor) volumeCacheRestore() error { logrus.Debugf("restoring contents of volume %q from %q", archivedPath, cacheFile) cache, err := os.Open(cacheFile) if err != nil { - return errors.Wrapf(err, "error opening archive at %q", cacheFile) + return err } defer cache.Close() if err := os.RemoveAll(archivedPath); err != nil { - return errors.Wrapf(err, "error clearing volume path %q", archivedPath) + return err } if err := os.MkdirAll(archivedPath, 0755); err != nil { - return errors.Wrapf(err, "error recreating volume path %q", archivedPath) + return err } err = archive.Untar(cache, archivedPath, nil) if err != nil { @@ -239,7 +239,7 @@ func (s *StageExecutor) volumeCacheRestore() error { } if st, ok := s.volumeCacheInfo[cachedPath]; ok { if err := os.Chmod(archivedPath, st.Mode()); err != nil { - return errors.Wrapf(err, "error restoring permissions on %q", archivedPath) + return err } uid := 0 gid := 0 @@ -248,10 +248,10 @@ func (s *StageExecutor) volumeCacheRestore() error { gid = util.GID(st) } if err := os.Chown(archivedPath, uid, gid); err != nil { - return errors.Wrapf(err, "error setting ownership on %q", archivedPath) + return err } if err := os.Chtimes(archivedPath, st.ModTime(), st.ModTime()); err != nil { - return errors.Wrapf(err, "error restoring datestamps on %q", archivedPath) + return err } } } diff --git a/imagebuildah/util.go b/imagebuildah/util.go index 29cdf44d0..50da74b37 100644 --- a/imagebuildah/util.go +++ b/imagebuildah/util.go @@ -40,7 +40,7 @@ func downloadToDirectory(url, dir string) error { logrus.Debugf("extracting %q to %q", url, dir) resp, err := http.Get(url) if err != nil { - return errors.Wrapf(err, "error getting %q", url) + return err } defer resp.Body.Close() if resp.ContentLength == 0 { @@ -49,12 +49,12 @@ func downloadToDirectory(url, dir string) error { if err := chrootarchive.Untar(resp.Body, dir, nil); err != nil { resp1, err := http.Get(url) if err != nil { - return errors.Wrapf(err, "error getting %q", url) + return err } defer resp1.Body.Close() body, err := ioutil.ReadAll(resp1.Body) if err != nil { - return errors.Wrapf(err, "Failed to read %q", url) + return err } dockerfile := filepath.Join(dir, "Dockerfile") // Assume this is a Dockerfile diff --git a/run_linux.go b/run_linux.go index 0f9607748..3a07407b0 100644 --- a/run_linux.go +++ b/run_linux.go @@ -63,18 +63,18 @@ func setChildProcess() error { func (b *Builder) Run(command []string, options RunOptions) error { p, err := ioutil.TempDir("", Package) if err != nil { - return errors.Wrapf(err, "run: error creating temporary directory under %q", os.TempDir()) + return err } // On some hosts like AH, /tmp is a symlink and we need an // absolute path. path, err := filepath.EvalSymlinks(p) if err != nil { - return errors.Wrapf(err, "run: error evaluating %q for symbolic links", p) + return err } logrus.Debugf("using %q to hold bundle data", path) defer func() { if err2 := os.RemoveAll(path); err2 != nil { - logrus.Errorf("error removing %q: %v", path, err2) + logrus.Error(err2) } }() @@ -167,7 +167,7 @@ func (b *Builder) Run(command []string, options RunOptions) error { logrus.Debugf("ensuring working directory %q exists", filepath.Join(mountPoint, spec.Process.Cwd)) if err = os.MkdirAll(filepath.Join(mountPoint, spec.Process.Cwd), 0755); err != nil && !os.IsExist(err) { - return errors.Wrapf(err, "error ensuring working directory %q exists", spec.Process.Cwd) + return err } // Set the seccomp configuration using the specified profile name. Some syscalls are @@ -213,7 +213,7 @@ func (b *Builder) Run(command []string, options RunOptions) error { if _, ok := bindFiles["/run/.containerenv"]; !ok { // Empty string for now, but we may consider populating this later containerenvPath := filepath.Join(path, "/run/.containerenv") - if err = os.MkdirAll(filepath.Dir(containerenvPath), 0755); err != nil && !os.IsExist(err) { + if err = os.MkdirAll(filepath.Dir(containerenvPath), 0755); err != nil { return err } emptyFile, err := os.Create(containerenvPath) @@ -222,7 +222,7 @@ func (b *Builder) Run(command []string, options RunOptions) error { } emptyFile.Close() if err := label.Relabel(containerenvPath, b.MountLabel, false); err != nil { - return errors.Wrapf(err, "error relabeling %q in container %q", containerenvPath, b.ContainerID) + return err } bindFiles["/run/.containerenv"] = containerenvPath @@ -330,35 +330,35 @@ func runSetupBuiltinVolumes(mountLabel, mountPoint, containerDir string, builtin // If we need to, initialize the volume path's initial contents. if _, err := os.Stat(volumePath); err != nil { if !os.IsNotExist(err) { - return nil, errors.Wrapf(err, "failed to stat %q for volume %q", volumePath, volume) + return nil, err } logrus.Debugf("setting up built-in volume at %q", volumePath) if err = os.MkdirAll(volumePath, 0755); err != nil { - return nil, errors.Wrapf(err, "error creating directory %q for volume %q", volumePath, volume) + return nil, err } if err = label.Relabel(volumePath, mountLabel, false); err != nil { - return nil, errors.Wrapf(err, "error relabeling directory %q for volume %q", volumePath, volume) + return nil, err } initializeVolume = true } stat, err := os.Stat(srcPath) if err != nil { if !os.IsNotExist(err) { - return nil, errors.Wrapf(err, "failed to stat %q for volume %q", srcPath, volume) + return nil, err } if err = idtools.MkdirAllAndChownNew(srcPath, 0755, hostOwner); err != nil { - return nil, errors.Wrapf(err, "error creating directory %q for volume %q", srcPath, volume) + return nil, err } if stat, err = os.Stat(srcPath); err != nil { - return nil, errors.Wrapf(err, "failed to stat %q for volume %q", srcPath, volume) + return nil, err } } if initializeVolume { if err = os.Chmod(volumePath, stat.Mode().Perm()); err != nil { - return nil, errors.Wrapf(err, "failed to chmod %q for volume %q", volumePath, volume) + return nil, err } if err = os.Chown(volumePath, int(stat.Sys().(*syscall.Stat_t).Uid), int(stat.Sys().(*syscall.Stat_t).Gid)); err != nil { - return nil, errors.Wrapf(err, "error chowning directory %q for volume %q", volumePath, volume) + return nil, err } if err = extractWithTar(mountPoint, srcPath, volumePath); err != nil && !os.IsNotExist(errors.Cause(err)) { return nil, errors.Wrapf(err, "error populating directory %q for volume %q using contents of %q", volumePath, volume, srcPath) @@ -514,11 +514,11 @@ func (b *Builder) setupMounts(mountPoint string, spec *specs.Spec, bundlePath st func (b *Builder) addNetworkConfig(rdir, hostPath string, chownOpts *idtools.IDPair, dnsServers, dnsSearch, dnsOptions []string, namespaceOptions NamespaceOptions) (string, error) { stat, err := os.Stat(hostPath) if err != nil { - return "", errors.Wrapf(err, "error statting %q for container %q", hostPath, b.ContainerID) + return "", err } contents, err := ioutil.ReadFile(hostPath) if err != nil { - return "", errors.Wrapf(err, "unable to read %s", hostPath) + return "", err } search := resolvconf.GetSearchDomains(contents) @@ -572,13 +572,12 @@ func (b *Builder) addNetworkConfig(rdir, hostPath string, chownOpts *idtools.IDP gid = chownOpts.GID } if err = os.Chown(cfile, uid, gid); err != nil { - return "", errors.Wrapf(err, "error chowning file %q for container %q", cfile, b.ContainerID) + return "", err } if err := label.Relabel(cfile, b.MountLabel, false); err != nil { - return "", errors.Wrapf(err, "error relabeling %q in container %q", cfile, b.ContainerID) + return "", err } - return cfile, nil } @@ -587,13 +586,13 @@ func (b *Builder) generateHosts(rdir, hostname string, addHosts []string, chownO hostPath := "/etc/hosts" stat, err := os.Stat(hostPath) if err != nil { - return "", errors.Wrapf(err, "error statting %q for container %q", hostPath, b.ContainerID) + return "", err } hosts := bytes.NewBufferString("# Generated by Buildah\n") orig, err := ioutil.ReadFile(hostPath) if err != nil { - return "", errors.Wrapf(err, "unable to read %s", hostPath) + return "", err } hosts.Write(orig) for _, host := range addHosts { @@ -626,10 +625,10 @@ func (b *Builder) generateHosts(rdir, hostname string, addHosts []string, chownO gid = chownOpts.GID } if err = os.Chown(cfile, uid, gid); err != nil { - return "", errors.Wrapf(err, "error chowning file %q for container %q", cfile, b.ContainerID) + return "", err } if err := label.Relabel(cfile, b.MountLabel, false); err != nil { - return "", errors.Wrapf(err, "error relabeling %q in container %q", cfile, b.ContainerID) + return "", err } return cfile, nil @@ -678,7 +677,7 @@ func runUsingRuntime(isolation Isolation, options RunOptions, configureNetwork b return 1, errors.Wrapf(err, "error encoding configuration %#v as json", spec) } if err = ioutils.AtomicWriteFile(filepath.Join(bundlePath, "config.json"), specbytes, 0600); err != nil { - return 1, errors.Wrapf(err, "error storing runtime configuration in %q", filepath.Join(bundlePath, "config.json")) + return 1, errors.Wrapf(err, "error storing runtime configuration") } logrus.Debugf("config = %v", string(specbytes)) @@ -800,7 +799,7 @@ func runUsingRuntime(isolation Isolation, options RunOptions, configureNetwork b // Make sure we read the container's exit status when it exits. pidValue, err := ioutil.ReadFile(pidFile) if err != nil { - return 1, errors.Wrapf(err, "error reading pid from %q", pidFile) + return 1, err } pid, err := strconv.Atoi(strings.TrimSpace(string(pidValue))) if err != nil { @@ -951,7 +950,7 @@ func runCollectOutput(fds, closeBeforeReadingFds []int) string { func setupRootlessNetwork(pid int) (teardown func(), err error) { slirp4netns, err := exec.LookPath("slirp4netns") if err != nil { - return nil, errors.Wrapf(err, "cannot find slirp4netns") + return nil, err } rootlessSlirpSyncR, rootlessSlirpSyncW, err := os.Pipe() @@ -1568,7 +1567,7 @@ func setupNamespaces(g *generate.Generator, namespaceOptions NamespaceOptions, i p := filepath.Join("/proc/sys", strings.Replace(name, ".", "/", -1)) _, err := os.Stat(p) if err != nil && !os.IsNotExist(err) { - return false, nil, false, errors.Wrapf(err, "cannot stat %s", p) + return false, nil, false, err } if err == nil { g.AddLinuxSysctl(name, val) @@ -1706,12 +1705,12 @@ func (b *Builder) runSetupVolumeMounts(mountLabel string, volumeMounts []string, } if foundz { if err := label.Relabel(host, mountLabel, true); err != nil { - return specs.Mount{}, errors.Wrapf(err, "relabeling %q failed", host) + return specs.Mount{}, err } } if foundZ { if err := label.Relabel(host, mountLabel, false); err != nil { - return specs.Mount{}, errors.Wrapf(err, "relabeling %q failed", host) + return specs.Mount{}, err } } if foundO { @@ -1988,7 +1987,7 @@ func setupRootlessSpecChanges(spec *specs.Spec, bundleDir string, shmSize string emptyDir := filepath.Join(bundleDir, "empty") if err := os.Mkdir(emptyDir, 0); err != nil { - return errors.Wrapf(err, "error creating %q", emptyDir) + return err } // Replace /sys with a read-only bind mount. diff --git a/tests/bud.bats b/tests/bud.bats index 4cd16c595..dcca80223 100644 --- a/tests/bud.bats +++ b/tests/bud.bats @@ -1787,10 +1787,16 @@ _EOF @test "bud with specified context should succeed if context contains existing Dockerfile" { DIR=$(mktemp -d) - touch "$DIR"/Dockerfile + echo "FROM alpine" > "$DIR"/Dockerfile run_buildah 0 bud --signature-policy ${TESTSDIR}/policy.json "$DIR"/Dockerfile } +@test "bud with specified context should fail if context contains empty Dockerfile" { + DIR=$(mktemp -d) + touch "$DIR"/Dockerfile + run_buildah 125 bud --signature-policy ${TESTSDIR}/policy.json "$DIR"/Dockerfile +} + @test "bud-no-change" { _prefetch alpine parent=alpine diff --git a/util.go b/util.go index 4b5a00e44..47c9ac5cd 100644 --- a/util.go +++ b/util.go @@ -189,7 +189,7 @@ func IsContainer(id string, store storage.Store) (bool, error) { if os.IsNotExist(err) { return false, nil } - return false, errors.Wrapf(err, "error stating %q", filepath.Join(cdir, stateFile)) + return false, err } return true, nil }