Browse code

Fix error handling for bind mount spec parser.

Errors were being ignored and always telling the user that the path
doesn't exist even if it was some other problem, such as a permission
error.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
(cherry picked from commit ebcef288343698dd86ff307f5b9c58aa52ce9fdd)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Brian Goff authored on 2019/05/22 07:34:57
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
 }