Browse code

Merge pull request #235 from thaJeztah/19.03_backport_make_sure_to_hydrate_when_eating_pretzels

[19.03 backport] Fix error handling for bind mount spec parser.

Sebastiaan van Stijn authored on 2019/05/23 19:09:08
Showing 2 changed files
... ...
@@ -82,7 +82,10 @@ func (p *linuxParser) validateMountConfigImpl(mnt *mount.Mount, validateBindSour
82 82
 		}
83 83
 
84 84
 		if validateBindSourceExists {
85
-			exists, _, _ := currentFileInfoProvider.fileInfo(mnt.Source)
85
+			exists, _, err := currentFileInfoProvider.fileInfo(mnt.Source)
86
+			if err != nil {
87
+				return &errMountConfig{mnt, err}
88
+			}
86 89
 			if !exists {
87 90
 				return &errMountConfig{mnt, errBindSourceDoesNotExist(mnt.Source)}
88 91
 			}
... ...
@@ -1,6 +1,7 @@
1 1
 package mounts // import "github.com/docker/docker/volume/mounts"
2 2
 
3 3
 import (
4
+	"errors"
4 5
 	"io/ioutil"
5 6
 	"os"
6 7
 	"runtime"
... ...
@@ -8,6 +9,8 @@ import (
8 8
 	"testing"
9 9
 
10 10
 	"github.com/docker/docker/api/types/mount"
11
+	"gotest.tools/assert"
12
+	"gotest.tools/assert/cmp"
11 13
 )
12 14
 
13 15
 type parseMountRawTestSet struct {
... ...
@@ -477,4 +480,51 @@ func TestParseMountSpec(t *testing.T) {
477 477
 			t.Errorf("Expected mount copy data to match. Expected: '%v', Actual: '%v'", c.expected.CopyData, mp.CopyData)
478 478
 		}
479 479
 	}
480
+
481
+}
482
+
483
+// always returns the configured error
484
+// this is used to test error handling
485
+type mockFiProviderWithError struct{ err error }
486
+
487
+func (m mockFiProviderWithError) fileInfo(path string) (bool, bool, error) {
488
+	return false, false, m.err
489
+}
490
+
491
+// TestParseMountSpecBindWithFileinfoError makes sure that the parser returns
492
+// the error produced by the fileinfo provider.
493
+//
494
+// Some extra context for the future in case of changes and possible wtf are we
495
+// testing this for:
496
+//
497
+// Currently this "fileInfoProvider" returns (bool, bool, error)
498
+// The 1st bool is "does this path exist"
499
+// The 2nd bool is "is this path a dir"
500
+// Then of course the error is an error.
501
+//
502
+// The issue is the parser was ignoring the error and only looking at the
503
+// "does this path exist" boolean, which is always false if there is an error.
504
+// Then the error returned to the caller was a (slightly, maybe) friendlier
505
+// error string than what comes from `os.Stat`
506
+// So ...the caller was always getting an error saying the path doesn't exist
507
+// even if it does exist but got some other error (like a permission error).
508
+// This is confusing to users.
509
+func TestParseMountSpecBindWithFileinfoError(t *testing.T) {
510
+	previousProvider := currentFileInfoProvider
511
+	defer func() { currentFileInfoProvider = previousProvider }()
512
+
513
+	testErr := errors.New("some crazy error")
514
+	currentFileInfoProvider = &mockFiProviderWithError{err: testErr}
515
+
516
+	p := "/bananas"
517
+	if runtime.GOOS == "windows" {
518
+		p = `c:\bananas`
519
+	}
520
+	m := mount.Mount{Type: mount.TypeBind, Source: p, Target: p}
521
+
522
+	parser := NewParser(runtime.GOOS)
523
+
524
+	_, err := parser.ParseMountSpec(m)
525
+	assert.Assert(t, err != nil)
526
+	assert.Assert(t, cmp.Contains(err.Error(), "some crazy error"))
480 527
 }