8000 Update createSpec to use errdefs · moby/moby@59c7083 · GitHub
[go: up one dir, main page]
Skip to content

Commit 59c7083

Browse files
dpernydperny
committed
Update createSpec to use errdefs
Updates the createSpec method, for both Windows and Linux systems, to return all errors in terms of errdefs. Makes easy changes in dependencies of those methods as well, if bringing them to use all errdefs errors was trivial. Includes comments annotating why each errdef type was chosen for each particular error. Signed-off-by: Drew Erny <drew.erny@docker.com>
1 parent cbb885b commit 59c7083

File tree

7 files changed

+210
-58
lines changed

7 files changed

+210
-58
lines changed

‎daemon/container_operations.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -955,7 +955,7 @@ func (daemon *Daemon) getNetworkedContainer(containerID, connectedContainerID st
955955
return nil, err
956956
}
957957
if containerID == nc.ID {
958-
return nil, fmt.Errorf("cannot join own network")
958+
return nil, errdefs.InvalidParameter(fmt.Errorf("cannot join own network"))
959959
}
960960
if !nc.IsRunning() {
961961
err := fmt.Errorf("cannot join network of a non running container: %s", connectedContainerID)

‎daemon/container_operations_unix.go‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ import (
2626
)
2727

2828
func (daemon *Daemon) setupLinkedContainers(container *container.Container) ([]string, error) {
29+
// NOTE(dperny): this method has been fixed to return only errdefs errors.
2930
var env []string
3031
children := daemon.children(container)
3132

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

3738
for linkAlias, child := range children {
3839
if !child.IsRunning() {
39-
return nil, fmt.Errorf("Cannot link to a non running container: %s AS %s", child.Name, linkAlias)
40+
return nil, errdefs.InvalidParameter(fmt.Errorf("Cannot link to a non running container: %s AS %s", child.Name, linkAlias))
4041
}
4142

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

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

8182
return container, nil

‎daemon/oci_linux.go‎

Lines changed: 64 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import (
1313
containertypes "github.com/docker/docker/api/types/container"
1414
"github.com/docker/docker/container"
1515
daemonconfig "github.com/docker/docker/daemon/config"
16+
"github.com/docker/docker/errdefs"
1617
"github.com/docker/docker/oci"
1718
"github.com/docker/docker/oci/caps"
1819
"github.com/docker/docker/pkg/idtools"
@@ -637,18 +638,24 @@ func setMounts(daemon *Daemon, s *specs.Spec, c *container.Container, mounts []c
637638

638639
func (daemon *Daemon) populateCommonSpec(s *specs.Spec, c *container.Container) error {
639640
if c.BaseFS == nil {
640-
return errors.New("populateCommonSpec: BaseFS of container " + c.ID + " is unexpectedly nil")
641+
// Return System, because it's unlikely that the requester screwed
642+
// something up to cause this
643+
return errdefs.System(errors.New("populateCommonSpec: BaseFS of container " + c.ID + " is unexpectedly nil"))
641644
}
642645
linkedEnv, err := daemon.setupLinkedContainers(c)
643646
if err != nil {
647+
// all of the errors returned from setupLinkedContainers should already
648+
// be of errdefs types, so we can safely return it raw.
644649
return err
645650
}
646651
s.Root = &specs.Root{
647652
Path: c.BaseFS.Path(),
648653
Readonly: c.HostConfig.ReadonlyRootfs,
649654
}
650655
if err := c.SetupWorkingDirectory(daemon.idMapping.RootPair()); err != nil {
651-
return err
656+
// all of the errors in SetupWorkingDirectory are not really the
657+
// requester's fault, so return System
658+
return errdefs.System(err)
652659
}
653660
cwd := c.Config.WorkingDir
654661
if len(cwd) == 0 {
@@ -667,7 +674,10 @@ func (daemon *Daemon) populateCommonSpec(s *specs.Spec, c *container.Container)
667674
if path == "" {
668675
path, err = exec.LookPath(daemonconfig.DefaultInitBinary)
669676
if err != nil {
670-
return err
677+
// If we can't find the daemon's default init, then that's
678+
// a System error, because the configuration of the daemon
679+
// is invalid, but not the user's request.
680+
return errdefs.System(err)
671681
}
672682
}
673683
s.Mounts = append(s.Mounts, specs.Mount{
@@ -695,8 +705,13 @@ func (daemon *Daemon) populateCommonSpec(s *specs.Spec, c *container.Container)
695705
}
696706

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

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

725740
if err := setResources(&s, c.HostConfig.Resources); err != nil {
726-
return nil, fmt.Errorf("linux runtime spec resources: %v", err)
741+
return nil, errdefs.InvalidParameter(fmt.Errorf("linux runtime spec resources: %v", err))
727742
}
728743
// We merge the sysctls injected above with the HostConfig (latter takes
729744
// precedence for backwards-compatibility reasons).
@@ -733,13 +748,15 @@ func (daemon *Daemon) createSpec(c *container.Container) (retSpec *specs.Spec, e
733748

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

753770
if err := daemon.initCgroupsPath(parentPath); err != nil {
754-
return nil, fmt.Errorf("linux init cgroups path: %v", err)
771+
// all of the errors in initCgroupsPath are things like failing to make
772+
// a directory, or are otherwise the result of a function call with
773+
// hard-coded arguments, so return a System error if it fails.
774+
return nil, errdefs.System(fmt.Errorf("linux init cgroups path: %v", err))
755775
}
756776
if err := setDevices(&s, c); err != nil {
757-
return nil, fmt.Errorf("linux runtime spec devices: %v", err)
777+
// The errors resulting from devices are generally more of a
778+
// user-configured-it-wrong error, so return InvalidParameter
779+
return nil, errdefs.InvalidParameter(fmt.Errorf("linux runtime spec devices: %v", err))
758780
}
759781
if err := daemon.setRlimits(&s, c); err != nil {
760-
return nil, fmt.Errorf("linux runtime spec rlimits: %v", err)
782+
// as of this writing, setRlimits doesn't return any errors at all, so
783+
// we return error type Unknown if it happens.
784+
return nil, errdefs.Unknown(fmt.Errorf("linux runtime spec rlimits: %v", err))
761785
}
762786
if err := setUser(&s, c); err != nil {
763-
return nil, fmt.Errorf("linux spec user: %v", err)
787+
// setUser fails if you specify a bad user, so it's an InvalidParameter
788+
return nil, errdefs.InvalidParameter(fmt.Errorf("linux spec user: %v", err))
764789
}
765790
if err := setNamespaces(daemon, &s, c); err != nil {
766-
return nil, fmt.Errorf("linux spec namespaces: %v", err)
791+
// setNamespaces has been fixed to return all errdefs errors, so we
792+
// don't need to choose one here. additionally, the errdefs package is
793+
// aware of the "causer" interface from github.com/pkg/errors, so we
794+
// can safely wrap this error so that the actual error message still
795+
// starts with "linux spec namespaces"
796+
return nil, errors.Wrap(err, "linux spec namespaces")
767797
}
768798
capabilities, err := caps.TweakCapabilities(oci.DefaultCapabilities(), c.HostConfig.CapAdd, c.HostConfig.CapDrop, c.HostConfig.Capabilities, c.HostConfig.Privileged)
769799
if err != nil {
770-
return nil, fmt.Errorf("linux spec capabilities: %v", err)
800+
// caps.TweakCapabilities also uses all errdefs errors
801+
return nil, errors.Wrap(err, "linux spec capabilities")
771802
}
772803
if err := oci.SetCapabilities(&s, capabilities); err != nil {
773-
return nil, fmt.Errorf("linux spec capabilities: %v", err)
804+
// SetCapabilities returned no error in this revision, so if it fails,
805+
// we don't know why
806+
return nil, errdefs.Unknown(fmt.Errorf("linux spec capabilities: %v", err))
774807
}
775808
if err := setSeccomp(daemon, &s, c); err != nil {
776-
return nil, fmt.Errorf("linux seccomp: %v", err)
809+
// if setting Seccomp fails, then it's probably your fault.
810+
return nil, errdefs.InvalidParameter(fmt.Errorf("linux seccomp: %v", err))
777811
}
778812

779813
if err := daemon.setupContainerMountsRoot(c); err != nil {
780-
return nil, err
814+
// it shouldn't be the user's fault if this fails.
815+
return nil, errdefs.System(err)
781816
}
782817

783818
if err := daemon.setupIpcDirs(c); err != nil {
784-
return nil, err
819+
// this will fail if the user configures it wrongly
820+
return nil, errdefs.InvalidParameter(err)
785821
}
786822

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

829+
// the next several calls only return System errors, because their failures
830+
// are not really caused by user input.
831+
793832
if err := daemon.setupSecretDir(c); err != nil {
794-
return nil, err
833+
return nil, errdefs.System(err)
795834
}
796835

797836
ms, err := daemon.setupMounts(c)
798837
if err != nil {
799-
return nil, err
838+
return nil, errdefs.System(err)
800839
}
801840

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

806845
tmpfsMounts, err := c.TmpfsMounts()
807846
if err != nil {
808-
return nil, err
847+
return nil, errdefs.System(err)
809848
}
810849
ms = append(ms, tmpfsMounts...)
811850

812851
secretMounts, err := c.SecretMounts()
813852
if err != nil {
814-
return nil, err
853+
return nil, errdefs.System(err)
815854
}
816855
ms = append(ms, secretMounts...)
817856

818857
sort.Sort(mounts(ms))
819858
if err := setMounts(daemon, &s, c, ms); err != nil {
820-
return nil, fmt.Errorf("linux mounts: %v", err)
859+
return nil, errdefs.System(fmt.Errorf("linux mounts: %v", err))
821860
}
822861

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

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

872911
if daemon.configStore.Rootless {
873912
if err := specconv.ToRootless(&s); err != nil {
874-
return nil, err
913+
// in the current revision, ToRootless doesn't return any known
914+
// error.
915+
return nil, errdefs.Unknown(err)
875916
}
876917
}
877918
return &s, nil

0 commit comments

Comments
 (0)
0