From 6145628fff2fe33b8649f8a8fcc1e191c4550741 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 1 Jul 2021 10:26:31 -0700 Subject: [PATCH 1/2] configs/validate: audit all returned errors All the errors returned from Validate should tell about a configuration error. Some were lacking a context, so add it. While at it, fix abusing fmt.Errorf and logrus.Warnf where the argument do not contain %-style formatting. Signed-off-by: Kir Kolyshkin --- libcontainer/configs/validate/validator.go | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/libcontainer/configs/validate/validator.go b/libcontainer/configs/validate/validator.go index c2500a63052..1ca91ba7478 100644 --- a/libcontainer/configs/validate/validator.go +++ b/libcontainer/configs/validate/validator.go @@ -52,7 +52,7 @@ func (v *ConfigValidator) Validate(config *configs.Config) error { } for _, c := range warns { if err := c(config); err != nil { - logrus.WithError(err).Warnf("invalid configuration") + logrus.WithError(err).Warn("invalid configuration") } } return nil @@ -62,20 +62,17 @@ func (v *ConfigValidator) Validate(config *configs.Config) error { // to the container's root filesystem. func (v *ConfigValidator) rootfs(config *configs.Config) error { if _, err := os.Stat(config.Rootfs); err != nil { - if os.IsNotExist(err) { - return fmt.Errorf("rootfs (%s) does not exist", config.Rootfs) - } - return err + return fmt.Errorf("invalid rootfs: %w", err) } cleaned, err := filepath.Abs(config.Rootfs) if err != nil { - return err + return fmt.Errorf("invalid rootfs: %w", err) } if cleaned, err = filepath.EvalSymlinks(cleaned); err != nil { - return err + return fmt.Errorf("invalid rootfs: %w", err) } if filepath.Clean(config.Rootfs) != cleaned { - return fmt.Errorf("%s is not an absolute path or is a symlink", config.Rootfs) + return errors.New("invalid rootfs: not an absolute path, or a symlink") } return nil } @@ -176,7 +173,7 @@ func (v *ConfigValidator) sysctl(config *configs.Config) error { hostnet, hostnetErr = isHostNetNS(path) }) if hostnetErr != nil { - return hostnetErr + return fmt.Errorf("invalid netns path: %w", hostnetErr) } if hostnet { return fmt.Errorf("sysctl %q not allowed in host network namespace", s) From 538ba846dd63b6fd1d78e83a395b83688d4c591c Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 1 Jul 2021 10:54:58 -0700 Subject: [PATCH 2/2] libct/error.go: rm ConfigError ConfigError was added by commit e918d021399e62, while removing runc own error system, to preserve a way for a libcontainer user to distinguish between a configuration error and something else. The way ConfigError is implemented requires a different type of check (compared to all other errors defined by error.go). An attempt was made to rectify this, but the resulting code became even more complicated. As no one is using this functionality (of differentiating a "bad config" type of error from other errors), let's just drop the ConfigError type. Signed-off-by: Kir Kolyshkin --- libcontainer/container_linux.go | 2 +- libcontainer/error.go | 8 -------- libcontainer/factory_linux.go | 6 +++--- 3 files changed, 4 insertions(+), 12 deletions(-) diff --git a/libcontainer/container_linux.go b/libcontainer/container_linux.go index 21e77932526..3c080f7040a 100644 --- a/libcontainer/container_linux.go +++ b/libcontainer/container_linux.go @@ -231,7 +231,7 @@ func (c *linuxContainer) Start(process *Process) error { c.m.Lock() defer c.m.Unlock() if c.config.Cgroups.Resources.SkipDevices { - return &ConfigError{"can't start container with SkipDevices set"} + return errors.New("can't start container with SkipDevices set") } if process.Init { if err := c.createExecFifo(); err != nil { diff --git a/libcontainer/error.go b/libcontainer/error.go index bd5ad1f8bf7..510c072264f 100644 --- a/libcontainer/error.go +++ b/libcontainer/error.go @@ -11,11 +11,3 @@ var ( ErrNotRunning = errors.New("container not running") ErrNotPaused = errors.New("container not paused") ) - -type ConfigError struct { - details string -} - -func (e *ConfigError) Error() string { - return "invalid configuration: " + e.details -} diff --git a/libcontainer/factory_linux.go b/libcontainer/factory_linux.go index 63cf57b099b..6c1b48b9334 100644 --- a/libcontainer/factory_linux.go +++ b/libcontainer/factory_linux.go @@ -251,13 +251,13 @@ type LinuxFactory struct { func (l *LinuxFactory) Create(id string, config *configs.Config) (Container, error) { if l.Root == "" { - return nil, &ConfigError{"invalid root"} + return nil, errors.New("root not set") } if err := l.validateID(id); err != nil { return nil, err } if err := l.Validator.Validate(config); err != nil { - return nil, &ConfigError{err.Error()} + return nil, err } containerRoot, err := securejoin.SecureJoin(l.Root, id) if err != nil { @@ -294,7 +294,7 @@ func (l *LinuxFactory) Create(id string, config *configs.Config) (Container, err func (l *LinuxFactory) Load(id string) (Container, error) { if l.Root == "" { - return nil, &ConfigError{"invalid root"} + return nil, errors.New("root not set") } // when load, we need to check id is valid or not. if err := l.validateID(id); err != nil {