Browse code

daemon: use strings.Cut() and cleanup error messages

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Sebastiaan van Stijn authored on 2022/11/01 20:52:44
Showing 7 changed files
... ...
@@ -38,16 +38,16 @@ func merge(userConf, imageConf *containertypes.Config) error {
38 38
 	} else {
39 39
 		for _, imageEnv := range imageConf.Env {
40 40
 			found := false
41
-			imageEnvKey := strings.Split(imageEnv, "=")[0]
41
+			imageEnvKey, _, _ := strings.Cut(imageEnv, "=")
42 42
 			for _, userEnv := range userConf.Env {
43
-				userEnvKey := strings.Split(userEnv, "=")[0]
43
+				userEnvKey, _, _ := strings.Cut(userEnv, "=")
44 44
 				if isWindows {
45 45
 					// Case insensitive environment variables on Windows
46
-					imageEnvKey = strings.ToUpper(imageEnvKey)
47
-					userEnvKey = strings.ToUpper(userEnvKey)
46
+					found = strings.EqualFold(imageEnvKey, userEnvKey)
47
+				} else {
48
+					found = imageEnvKey == userEnvKey
48 49
 				}
49
-				if imageEnvKey == userEnvKey {
50
-					found = true
50
+				if found {
51 51
 					break
52 52
 				}
53 53
 			}
... ...
@@ -107,18 +107,18 @@ func (daemon *Daemon) buildSandboxOptions(container *container.Container) ([]lib
107 107
 		if _, err := opts.ValidateExtraHost(extraHost); err != nil {
108 108
 			return nil, err
109 109
 		}
110
-		parts := strings.SplitN(extraHost, ":", 2)
110
+		host, ip, _ := strings.Cut(extraHost, ":")
111 111
 		// If the IP Address is a string called "host-gateway", replace this
112 112
 		// value with the IP address stored in the daemon level HostGatewayIP
113 113
 		// config variable
114
-		if parts[1] == opts.HostGatewayName {
114
+		if ip == opts.HostGatewayName {
115 115
 			gateway := daemon.configStore.HostGatewayIP.String()
116 116
 			if gateway == "" {
117 117
 				return nil, fmt.Errorf("unable to derive the IP value for host-gateway")
118 118
 			}
119
-			parts[1] = gateway
119
+			ip = gateway
120 120
 		}
121
-		sboxOptions = append(sboxOptions, libnetwork.OptionExtraHost(parts[0], parts[1]))
121
+		sboxOptions = append(sboxOptions, libnetwork.OptionExtraHost(host, ip))
122 122
 	}
123 123
 
124 124
 	if container.HostConfig.PortBindings != nil {
... ...
@@ -215,26 +215,27 @@ func parseSecurityOpt(container *container.Container, config *containertypes.Hos
215 215
 			continue
216 216
 		}
217 217
 
218
-		var con []string
218
+		var k, v string
219
+		var ok bool
219 220
 		if strings.Contains(opt, "=") {
220
-			con = strings.SplitN(opt, "=", 2)
221
+			k, v, ok = strings.Cut(opt, "=")
221 222
 		} else if strings.Contains(opt, ":") {
222
-			con = strings.SplitN(opt, ":", 2)
223
+			k, v, ok = strings.Cut(opt, ":")
223 224
 			logrus.Warn("Security options with `:` as a separator are deprecated and will be completely unsupported in 17.04, use `=` instead.")
224 225
 		}
225
-		if len(con) != 2 {
226
+		if !ok {
226 227
 			return fmt.Errorf("invalid --security-opt 1: %q", opt)
227 228
 		}
228 229
 
229
-		switch con[0] {
230
+		switch k {
230 231
 		case "label":
231
-			labelOpts = append(labelOpts, con[1])
232
+			labelOpts = append(labelOpts, v)
232 233
 		case "apparmor":
233
-			container.AppArmorProfile = con[1]
234
+			container.AppArmorProfile = v
234 235
 		case "seccomp":
235
-			container.SeccompProfile = con[1]
236
+			container.SeccompProfile = v
236 237
 		case "no-new-privileges":
237
-			noNewPrivileges, err := strconv.ParseBool(con[1])
238
+			noNewPrivileges, err := strconv.ParseBool(v)
238 239
 			if err != nil {
239 240
 				return fmt.Errorf("invalid --security-opt 2: %q", opt)
240 241
 			}
... ...
@@ -1360,8 +1361,8 @@ func (daemon *Daemon) registerLinks(container *container.Container, hostConfig *
1360 1360
 			return errors.Wrapf(err, "could not get container for %s", name)
1361 1361
 		}
1362 1362
 		for child.HostConfig.NetworkMode.IsContainer() {
1363
-			parts := strings.SplitN(string(child.HostConfig.NetworkMode), ":", 2)
1364
-			child, err = daemon.GetContainer(parts[1])
1363
+			cid := child.HostConfig.NetworkMode.ConnectedContainer()
1364
+			child, err = daemon.GetContainer(cid)
1365 1365
 			if err != nil {
1366 1366
 				if errdefs.IsNotFound(err) {
1367 1367
 					// Trying to link to a non-existing container is not valid, and
... ...
@@ -1370,7 +1371,7 @@ func (daemon *Daemon) registerLinks(container *container.Container, hostConfig *
1370 1370
 					// image could not be found (see moby/moby#39823)
1371 1371
 					err = errdefs.InvalidParameter(err)
1372 1372
 				}
1373
-				return errors.Wrapf(err, "Could not get container for %s", parts[1])
1373
+				return errors.Wrapf(err, "could not get container for %s", cid)
1374 1374
 			}
1375 1375
 		}
1376 1376
 		if child.HostConfig.NetworkMode.IsHost() {
... ...
@@ -206,9 +206,7 @@ func (daemon *Daemon) fillAPIInfo(v *types.Info) {
206 206
 	cfg := daemon.configStore
207 207
 	for _, host := range cfg.Hosts {
208 208
 		// cnf.Hosts is normalized during startup, so should always have a scheme/proto
209
-		h := strings.SplitN(host, "://", 2)
210
-		proto := h[0]
211
-		addr := h[1]
209
+		proto, addr, _ := strings.Cut(host, "://")
212 210
 		if proto != "tcp" {
213 211
 			continue
214 212
 		}
... ...
@@ -241,8 +241,7 @@ func WithNamespaces(daemon *Daemon, c *container.Container) coci.SpecOpts {
241 241
 		// network
242 242
 		if !c.Config.NetworkDisabled {
243 243
 			ns := specs.LinuxNamespace{Type: "network"}
244
-			parts := strings.SplitN(string(c.HostConfig.NetworkMode), ":", 2)
245
-			if parts[0] == "container" {
244
+			if c.HostConfig.NetworkMode.IsContainer() {
246 245
 				nc, err := daemon.getNetworkedContainer(c.ID, c.HostConfig.NetworkMode.ConnectedContainer())
247 246
 				if err != nil {
248 247
 					return err
... ...
@@ -279,29 +279,31 @@ func (daemon *Daemon) setWindowsCredentialSpec(c *container.Container, s *specs.
279 279
 	// this doesn't seem like a great idea?
280 280
 	credentialSpec := ""
281 281
 
282
+	// TODO(thaJeztah): extract validating and parsing SecurityOpt to a reusable function.
282 283
 	for _, secOpt := range c.HostConfig.SecurityOpt {
283
-		optSplits := strings.SplitN(secOpt, "=", 2)
284
-		if len(optSplits) != 2 {
284
+		k, v, ok := strings.Cut(secOpt, "=")
285
+		if !ok {
285 286
 			return errdefs.InvalidParameter(fmt.Errorf("invalid security option: no equals sign in supplied value %s", secOpt))
286 287
 		}
287
-		if !strings.EqualFold(optSplits[0], "credentialspec") {
288
-			return errdefs.InvalidParameter(fmt.Errorf("security option not supported: %s", optSplits[0]))
288
+		// FIXME(thaJeztah): options should not be case-insensitive
289
+		if !strings.EqualFold(k, "credentialspec") {
290
+			return errdefs.InvalidParameter(fmt.Errorf("security option not supported: %s", k))
289 291
 		}
290 292
 
291
-		credSpecSplits := strings.SplitN(optSplits[1], "://", 2)
292
-		if len(credSpecSplits) != 2 || credSpecSplits[1] == "" {
293
+		scheme, value, ok := strings.Cut(v, "://")
294
+		if !ok || value == "" {
293 295
 			return errInvalidCredentialSpecSecOpt
294 296
 		}
295
-		value := credSpecSplits[1]
296
-
297 297
 		var err error
298
-		switch strings.ToLower(credSpecSplits[0]) {
298
+		switch strings.ToLower(scheme) {
299 299
 		case "file":
300
-			if credentialSpec, err = readCredentialSpecFile(c.ID, daemon.root, filepath.Clean(value)); err != nil {
300
+			credentialSpec, err = readCredentialSpecFile(c.ID, daemon.root, filepath.Clean(value))
301
+			if err != nil {
301 302
 				return errdefs.InvalidParameter(err)
302 303
 			}
303 304
 		case "registry":
304
-			if credentialSpec, err = readCredentialSpecRegistry(c.ID, value); err != nil {
305
+			credentialSpec, err = readCredentialSpecRegistry(c.ID, value)
306
+			if err != nil {
305 307
 				return errdefs.InvalidParameter(err)
306 308
 			}
307 309
 		case "config":
... ...
@@ -439,44 +441,41 @@ func readCredentialSpecRegistry(id, name string) (string, error) {
439 439
 // This allows for staging on machines which do not have the necessary components.
440 440
 func readCredentialSpecFile(id, root, location string) (string, error) {
441 441
 	if filepath.IsAbs(location) {
442
-		return "", fmt.Errorf("invalid credential spec - file:// path cannot be absolute")
442
+		return "", fmt.Errorf("invalid credential spec: file:// path cannot be absolute")
443 443
 	}
444 444
 	base := filepath.Join(root, credentialSpecFileLocation)
445 445
 	full := filepath.Join(base, location)
446 446
 	if !strings.HasPrefix(full, base) {
447
-		return "", fmt.Errorf("invalid credential spec - file:// path must be under %s", base)
447
+		return "", fmt.Errorf("invalid credential spec: file:// path must be under %s", base)
448 448
 	}
449 449
 	bcontents, err := os.ReadFile(full)
450 450
 	if err != nil {
451
-		return "", errors.Wrapf(err, "credential spec for container %s could not be read from file %q", id, full)
451
+		return "", errors.Wrapf(err, "failed to load credential spec for container %s", id)
452 452
 	}
453 453
 	return string(bcontents[:]), nil
454 454
 }
455 455
 
456 456
 func setupWindowsDevices(devices []containertypes.DeviceMapping) (specDevices []specs.WindowsDevice, err error) {
457
-	if len(devices) == 0 {
458
-		return
459
-	}
460
-
461 457
 	for _, deviceMapping := range devices {
462
-		devicePath := deviceMapping.PathOnHost
463
-		if strings.HasPrefix(devicePath, "class/") {
464
-			devicePath = strings.Replace(devicePath, "class/", "class://", 1)
465
-		}
466
-
467
-		srcParts := strings.SplitN(devicePath, "://", 2)
468
-		if len(srcParts) != 2 {
469
-			return nil, errors.Errorf("invalid device assignment path: '%s', must be 'class/ID' or 'IDType://ID'", deviceMapping.PathOnHost)
470
-		}
471
-		if srcParts[0] == "" {
472
-			return nil, errors.Errorf("invalid device assignment path: '%s', IDType cannot be empty", deviceMapping.PathOnHost)
473
-		}
474
-		wd := specs.WindowsDevice{
475
-			ID:     srcParts[1],
476
-			IDType: srcParts[0],
458
+		if strings.HasPrefix(deviceMapping.PathOnHost, "class/") {
459
+			specDevices = append(specDevices, specs.WindowsDevice{
460
+				ID:     strings.TrimPrefix(deviceMapping.PathOnHost, "class/"),
461
+				IDType: "class",
462
+			})
463
+		} else {
464
+			idType, id, ok := strings.Cut(deviceMapping.PathOnHost, "://")
465
+			if !ok {
466
+				return nil, errors.Errorf("invalid device assignment path: '%s', must be 'class/ID' or 'IDType://ID'", deviceMapping.PathOnHost)
467
+			}
468
+			if idType == "" {
469
+				return nil, errors.Errorf("invalid device assignment path: '%s', IDType cannot be empty", deviceMapping.PathOnHost)
470
+			}
471
+			specDevices = append(specDevices, specs.WindowsDevice{
472
+				ID:     id,
473
+				IDType: idType,
474
+			})
477 475
 		}
478
-		specDevices = append(specDevices, wd)
479 476
 	}
480 477
 
481
-	return
478
+	return specDevices, nil
482 479
 }
... ...
@@ -7,6 +7,7 @@ import (
7 7
 	"strings"
8 8
 	"testing"
9 9
 
10
+	is "gotest.tools/v3/assert/cmp"
10 11
 	"gotest.tools/v3/fs"
11 12
 
12 13
 	containertypes "github.com/docker/docker/api/types/container"
... ...
@@ -87,7 +88,7 @@ func TestSetWindowsCredentialSpecInSpec(t *testing.T) {
87 87
 		spec := &specs.Spec{}
88 88
 
89 89
 		err := daemon.setWindowsCredentialSpec(containerFactory(`file://C:\path\to\my\credspec.json`), spec)
90
-		assert.ErrorContains(t, err, "invalid credential spec - file:// path cannot be absolute")
90
+		assert.ErrorContains(t, err, "invalid credential spec: file:// path cannot be absolute")
91 91
 
92 92
 		assert.Check(t, spec.Windows == nil)
93 93
 	})
... ...
@@ -96,7 +97,7 @@ func TestSetWindowsCredentialSpecInSpec(t *testing.T) {
96 96
 		spec := &specs.Spec{}
97 97
 
98 98
 		err := daemon.setWindowsCredentialSpec(containerFactory(`file://..\credspec.json`), spec)
99
-		assert.ErrorContains(t, err, fmt.Sprintf("invalid credential spec - file:// path must be under %s", credSpecsDir))
99
+		assert.ErrorContains(t, err, fmt.Sprintf("invalid credential spec: file:// path must be under %s", credSpecsDir))
100 100
 
101 101
 		assert.Check(t, spec.Windows == nil)
102 102
 	})
... ...
@@ -105,9 +106,8 @@ func TestSetWindowsCredentialSpecInSpec(t *testing.T) {
105 105
 		spec := &specs.Spec{}
106 106
 
107 107
 		err := daemon.setWindowsCredentialSpec(containerFactory("file://i-dont-exist.json"), spec)
108
-		assert.ErrorContains(t, err, fmt.Sprintf("credential spec for container %s could not be read from file", dummyContainerID))
109
-		assert.ErrorContains(t, err, "The system cannot find")
110
-
108
+		assert.Check(t, is.ErrorContains(err, fmt.Sprintf("failed to load credential spec for container %s", dummyContainerID)))
109
+		assert.Check(t, is.ErrorIs(err, os.ErrNotExist))
111 110
 		assert.Check(t, spec.Windows == nil)
112 111
 	})
113 112