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.

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

Update createSpec to use errdefs #38770

Open
wants to merge 1 commit into
base: master
from
Open
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -955,7 +955,7 @@ func (daemon *Daemon) getNetworkedContainer(containerID, connectedContainerID st
return nil, err
}
if containerID == nc.ID {
return nil, fmt.Errorf("cannot join own network")
return nil, errdefs.InvalidParameter(fmt.Errorf("cannot join own network"))
}
if !nc.IsRunning() {
err := fmt.Errorf("cannot join network of a non running container: %s", connectedContainerID)
@@ -26,6 +26,7 @@ import (
)

func (daemon *Daemon) setupLinkedContainers(container *container.Container) ([]string, error) {
// NOTE(dperny): this method has been fixed to return only errdefs errors.

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Feb 21, 2019

Member

Think we can remove this note (as it's not a "todo") 😅

var env []string
children := daemon.children(container)

@@ -36,12 +37,12 @@ func (daemon *Daemon) setupLinkedContainers(container *container.Container) ([]s

for linkAlias, child := range children {
if !child.IsRunning() {
return nil, fmt.Errorf("Cannot link to a non running container: %s AS %s", child.Name, linkAlias)
return nil, errdefs.InvalidParameter(fmt.Errorf("Cannot link to a non running container: %s AS %s", child.Name, linkAlias))

This comment has been minimized.

Copy link
@cpuguy83

cpuguy83 Feb 21, 2019

Contributor

Maybe this should be Conflict?

This comment has been minimized.

Copy link
@dperny

dperny Feb 21, 2019

Author Contributor

Agree, will fix.

}

childBridgeSettings := child.NetworkSettings.Networks[runconfig.DefaultDaemonNetworkMode().NetworkName()]
if childBridgeSettings == nil || childBridgeSettings.EndpointSettings == nil {
return nil, fmt.Errorf("container %s not attached to default bridge network", child.ID)
return nil, errdefs.InvalidParameter(fmt.Errorf("container %s not attached to default bridge network", child.ID))
}

link := links.NewLink(
@@ -72,10 +73,10 @@ func (daemon *Daemon) getIpcContainer(id string) (*container.Container, error) {
// Check the container ipc is shareable
if st, err := os.Stat(container.ShmPath); err != nil || !st.IsDir() {
if err == nil || os.IsNotExist(err) {
return nil, errors.New(errMsg + ": non-shareable IPC")
return nil, errdefs.InvalidParameter(errors.New(errMsg + ": non-shareable IPC"))
}
// stat() failed?
return nil, errors.Wrap(err, errMsg+": unexpected error from stat "+container.ShmPath)
return nil, errdefs.System(errors.Wrap(err, errMsg+": unexpected error from stat "+container.ShmPath))
}

return container, nil
@@ -13,6 +13,7 @@ import (
containertypes "github.com/docker/docker/api/types/container"
"github.com/docker/docker/container"
daemonconfig "github.com/docker/docker/daemon/config"
"github.com/docker/docker/errdefs"
"github.com/docker/docker/oci"
"github.com/docker/docker/oci/caps"
"github.com/docker/docker/pkg/idtools"
@@ -637,18 +638,24 @@ func setMounts(daemon *Daemon, s *specs.Spec, c *container.Container, mounts []c

func (daemon *Daemon) populateCommonSpec(s *specs.Spec, c *container.Container) error {
if c.BaseFS == nil {
return errors.New("populateCommonSpec: BaseFS of container " + c.ID + " is unexpectedly nil")
// Return System, because it's unlikely that the requester screwed
// something up to cause this
return errdefs.System(errors.New("populateCommonSpec: BaseFS of container " + c.ID + " is unexpectedly nil"))
}
linkedEnv, err := daemon.setupLinkedContainers(c)
if err != nil {
// all of the errors returned from setupLinkedContainers should already
// be of errdefs types, so we can safely return it raw.
return err
}
s.Root = &specs.Root{
Path: c.BaseFS.Path(),
Readonly: c.HostConfig.ReadonlyRootfs,
}
if err := c.SetupWorkingDirectory(daemon.idMapping.RootPair()); err != nil {
return err
// all of the errors in SetupWorkingDirectory are not really the
// requester's fault, so return System
return errdefs.System(err)
}
cwd := c.Config.WorkingDir
if len(cwd) == 0 {
@@ -667,7 +674,10 @@ func (daemon *Daemon) populateCommonSpec(s *specs.Spec, c *container.Container)
if path == "" {
path, err = exec.LookPath(daemonconfig.DefaultInitBinary)
if err != nil {
return err
// If we can't find the daemon's default init, then that's
// a System error, because the configuration of the daemon
// is invalid, but not the user's request.
return errdefs.System(err)
}
}
s.Mounts = append(s.Mounts, specs.Mount{
@@ -695,8 +705,13 @@ func (daemon *Daemon) populateCommonSpec(s *specs.Spec, c *container.Container)
}

func (daemon *Daemon) createSpec(c *container.Container) (retSpec *specs.Spec, err error) {
// NOTE(dperny): this method has been updated so that all errors returned
// are errdefs errors. most of the errors returned have been annotated with
// comments to explain the choice of error type.
s := oci.DefaultSpec()
if err := daemon.populateCommonSpec(&s, c); err != nil {
// all of the errors returned by populateCommonSpec are errdefs types,
// so we can safely return the error without wrapping it
return nil, err
}

@@ -723,7 +738,7 @@ func (daemon *Daemon) createSpec(c *container.Container) (retSpec *specs.Spec, e
s.Linux.CgroupsPath = cgroupsPath

if err := setResources(&s, c.HostConfig.Resources); err != nil {
return nil, fmt.Errorf("linux runtime spec resources: %v", err)
return nil, errdefs.InvalidParameter(fmt.Errorf("linux runtime spec resources: %v", err))

This comment has been minimized.

Copy link
@cpuguy83

cpuguy83 Feb 21, 2019

Contributor

Can you change this to errors.Wrap?

This comment has been minimized.

Copy link
@dperny

dperny Feb 21, 2019

Author Contributor

Yeah, I realize I discovered that errors.Wrap works with errdefs, but I forgot to go back and change the ones I'd already written.

This comment has been minimized.

Copy link
@dperny

dperny Feb 21, 2019

Author Contributor

To be clear, do you think this:

errdefs.InvalidParameter(errors.Wrap(err, "linux runtime spec resources:")

or this:

errors.Wrap(errdefs.InvalidParameter(err), "linux runtime spec resources:")

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Feb 21, 2019

Member

I think the first one would be clearer (and the InvalidParameter would not be nested deeper)

}
// We merge the sysctls injected above with the HostConfig (latter takes
// precedence for backwards-compatibility reasons).
@@ -733,13 +748,15 @@ func (daemon *Daemon) createSpec(c *container.Container) (retSpec *specs.Spec, e

p := s.Linux.CgroupsPath
if useSystemd {
// these calls don't rely on any user input, so if they fails, that's
// on us, and we should return a System error
initPath, err := cgroups.GetInitCgroup("cpu")
if err != nil {
return nil, err
return nil, errdefs.System(err)
}
_, err = cgroups.GetOwnCgroup("cpu")
if err != nil {
return nil, err
return nil, errdefs.System(err)
}
p = filepath.Join(initPath, s.Linux.CgroupsPath)
}
@@ -751,37 +768,56 @@ func (daemon *Daemon) createSpec(c *container.Container) (retSpec *specs.Spec, e
}

if err := daemon.initCgroupsPath(parentPath); err != nil {
return nil, fmt.Errorf("linux init cgroups path: %v", err)
// all of the errors in initCgroupsPath are things like failing to make
// a directory, or are otherwise the result of a function call with
// hard-coded arguments, so return a System error if it fails.
return nil, errdefs.System(fmt.Errorf("linux init cgroups path: %v", err))
}
if err := setDevices(&s, c); err != nil {
return nil, fmt.Errorf("linux runtime spec devices: %v", err)
// The errors resulting from devices are generally more of a
// user-configured-it-wrong error, so return InvalidParameter
return nil, errdefs.InvalidParameter(fmt.Errorf("linux runtime spec devices: %v", err))

This comment has been minimized.

Copy link
@cpuguy83

cpuguy83 Feb 21, 2019

Contributor

errors.Wrap

}
if err := daemon.setRlimits(&s, c); err != nil {
return nil, fmt.Errorf("linux runtime spec rlimits: %v", err)
// as of this writing, setRlimits doesn't return any errors at all, so
// we return error type Unknown if it happens.
return nil, errdefs.Unknown(fmt.Errorf("linux runtime spec rlimits: %v", err))

This comment has been minimized.

Copy link
@cpuguy83

cpuguy83 Feb 21, 2019

Contributor

errors.Wrap
But we should maybe not use errdefs here, in case setRLimits wants to use errors int he future? Or if it's really not returning errors, maybe the method definition should be updated.

This comment has been minimized.

Copy link
@dperny

dperny Feb 21, 2019

Author Contributor

I could do either. I suppose it would be the ideal to enforce that if setRLimits ever did return errors, they would have to be of errdefs types.

}
if err := setUser(&s, c); err != nil {
return nil, fmt.Errorf("linux spec user: %v", err)
// setUser fails if you specify a bad user, so it's an InvalidParameter
return nil, errdefs.InvalidParameter(fmt.Errorf("linux spec user: %v", err))

This comment has been minimized.

Copy link
@cpuguy83

cpuguy83 Feb 21, 2019

Contributor

errors.Wrap

}
if err := setNamespaces(daemon, &s, c); err != nil {
return nil, fmt.Errorf("linux spec namespaces: %v", err)
// setNamespaces has been fixed to return all errdefs errors, so we
// don't need to choose one here. additionally, the errdefs package is
// aware of the "causer" interface from github.com/pkg/errors, so we
// can safely wrap this error so that the actual error message still
// starts with "linux spec namespaces"
return nil, errors.Wrap(err, "linux spec namespaces")
}
capabilities, err := caps.TweakCapabilities(oci.DefaultCapabilities(), c.HostConfig.CapAdd, c.HostConfig.CapDrop, c.HostConfig.Capabilities, c.HostConfig.Privileged)
if err != nil {
return nil, fmt.Errorf("linux spec capabilities: %v", err)
// caps.TweakCapabilities also uses all errdefs errors
return nil, errors.Wrap(err, "linux spec capabilities")
}
if err := oci.SetCapabilities(&s, capabilities); err != nil {
return nil, fmt.Errorf("linux spec capabilities: %v", err)
// SetCapabilities returned no error in this revision, so if it fails,
// we don't know why
return nil, errdefs.Unknown(fmt.Errorf("linux spec capabilities: %v", err))

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Feb 21, 2019

Member

I see there's some more fmt.Errorf('s here (and below); should all ideally be changed to errors.Wrap so that the original error doesn't get lost.

}
if err := setSeccomp(daemon, &s, c); err != nil {
return nil, fmt.Errorf("linux seccomp: %v", err)
// if setting Seccomp fails, then it's probably your fault.
return nil, errdefs.InvalidParameter(fmt.Errorf("linux seccomp: %v", err))
}

if err := daemon.setupContainerMountsRoot(c); err != nil {
return nil, err
// it shouldn't be the user's fault if this fails.
return nil, errdefs.System(err)
}

if err := daemon.setupIpcDirs(c); err != nil {
return nil, err
// this will fail if the user configures it wrongly
return nil, errdefs.InvalidParameter(err)
}

defer func() {
@@ -790,13 +826,16 @@ func (daemon *Daemon) createSpec(c *container.Container) (retSpec *specs.Spec, e
}
}()

// the next several calls only return System errors, because their failures
// are not really caused by user input.

if err := daemon.setupSecretDir(c); err != nil {
return nil, err
return nil, errdefs.System(err)
}

ms, err := daemon.setupMounts(c)
if err != nil {
return nil, err
return nil, errdefs.System(err)
}

if !c.HostConfig.IpcMode.IsPrivate() && !c.HostConfig.IpcMode.IsEmpty() {
@@ -805,19 +844,19 @@ func (daemon *Daemon) createSpec(c *container.Container) (retSpec *specs.Spec, e

tmpfsMounts, err := c.TmpfsMounts()
if err != nil {
return nil, err
return nil, errdefs.System(err)
}
ms = append(ms, tmpfsMounts...)

secretMounts, err := c.SecretMounts()
if err != nil {
return nil, err
return nil, errdefs.System(err)
}
ms = append(ms, secretMounts...)

sort.Sort(mounts(ms))
if err := setMounts(daemon, &s, c, ms); err != nil {
return nil, fmt.Errorf("linux mounts: %v", err)
return nil, errdefs.System(fmt.Errorf("linux mounts: %v", err))
}

for _, ns := range s.Linux.Namespaces {
@@ -850,7 +889,7 @@ func (daemon *Daemon) createSpec(c *container.Container) (retSpec *specs.Spec, e
// sure that we keep the default profile enabled we dynamically
// reload it if necessary.
if err := ensureDefaultAppArmorProfile(); err != nil {
return nil, err
return nil, errdefs.System(err)
}
}

@@ -871,7 +910,9 @@ func (daemon *Daemon) createSpec(c *container.Container) (retSpec *specs.Spec, e

if daemon.configStore.Rootless {
if err := specconv.ToRootless(&s); err != nil {
return nil, err
// in the current revision, ToRootless doesn't return any known
// error.
return nil, errdefs.Unknown(err)
}
}
return &s, nil
Oops, something went wrong.
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.