solver: correctly set the content selector with multiple bind mounts …

…references

Correctly set the content based selector when multiple bind mounts refer
to the same source. Previously, a selector that referred to the root
filesystem would be ignored. This is because a blank selector refers to
the root filesystem.

When two bind mounts referred to the same dependency, one mount would
add a selector while the other would be skipped. This caused the cache
key to be only computed based on the more narrow filesystem which caused
erroneous cache hits.

Now, the creation of the selector includes the root filesystem for
consideration. It fills in `/` as the selector and then removes it later
so that we don't narrow the selection in an invalid way.

Signed-off-by: Jonathan A. Sternberg <[email protected]>

solver/llbsolver/ops/exec.go

@@ -227,16 +227,28 @@ func (e *ExecOp) getMountDeps() ([]dep, error) {
 			return nil, errors.Errorf("invalid mountinput %v", m)
 		}
 
-		sel := m.Selector
-		if sel != "" {
-			sel = path.Join("/", sel)
-			deps[m.Input].Selectors = append(deps[m.Input].Selectors, sel)
-		}
+		// Mark the selector path as used. In this section, we need to
+		// record root selectors so the selection criteria isn't narrowed
+		// erroneously.
+		sel := path.Join("/", m.Selector)
+		deps[m.Input].Selectors = append(deps[m.Input].Selectors, sel)
 
 		if (!m.Readonly || m.Dest == pb.RootMount) && m.Output != -1 { // exclude read-only rootfs && read-write mounts
 			deps[m.Input].NoContentBasedHash = true
 		}
 	}
+
+	// Remove extraneous selectors that may have been generated from above.
+	for i, dep := range deps {
+		for _, sel := range dep.Selectors {
+			// If the root path is included in the list of selectors,
+			// this is the same as if no selector was used. Zero out this field.
+			if sel == "/" {
+				deps[i].Selectors = nil
+				break
+			}
+		}
+	}
 	return deps, nil
 }
 

solver/llbsolver/ops/exec_test.go

@@ -3,6 +3,9 @@ package ops
 import (
 	"testing"
 
+	"github.com/moby/buildkit/solver"
+	"github.com/moby/buildkit/solver/pb"
+	digest "github.com/opencontainers/go-digest"
 	"github.com/stretchr/testify/require"
 )
 
@@ -25,3 +28,72 @@ func TestDedupPaths(t *testing.T) {
 	res = dedupePaths([]string{"foo/bar/baz", "foo/bara", "foo/bar/bax", "foo/bar"})
 	require.Equal(t, []string{"foo/bar", "foo/bara"}, res)
 }
+
+func TestExecOp_getMountDeps(t *testing.T) {
+	v1 := &vertex{name: "local://context"}
+	v2 := &vertex{
+		name: "foo",
+		inputs: []solver.Edge{
+			{Vertex: v1, Index: 0},
+		},
+	}
+	op2, err := NewExecOp(v2, &pb.Op_Exec{
+		Exec: &pb.ExecOp{
+			Meta: &pb.Meta{
+				Args: []string{"/bin/bash", "-l"},
+			},
+			Mounts: []*pb.Mount{
+				{
+					Input:    pb.Empty,
+					Dest:     "/",
+					Readonly: true,
+					Output:   pb.SkipOutput,
+				},
+				{
+					Input:    pb.InputIndex(0),
+					Selector: "b.txt",
+					Dest:     "/test/b.txt",
+					Output:   pb.SkipOutput,
+				},
+				{
+					Input:  pb.InputIndex(0),
+					Dest:   "/test/data",
+					Output: pb.SkipOutput,
+				},
+			},
+		},
+	}, nil, nil, nil, nil, nil, nil)
+	require.NoError(t, err)
+
+	deps, err := op2.getMountDeps()
+	require.NoError(t, err)
+
+	require.Len(t, deps, 1)
+	require.Len(t, deps[0].Selectors, 0)
+	require.False(t, deps[0].NoContentBasedHash)
+}
+
+type vertex struct {
+	name   string
+	inputs []solver.Edge
+}
+
+func (v *vertex) Digest() digest.Digest {
+	return digest.FromString(v.name)
+}
+
+func (v *vertex) Sys() interface{} {
+	return v
+}
+
+func (v *vertex) Options() solver.VertexOptions {
+	return solver.VertexOptions{}
+}
+
+func (v *vertex) Inputs() []solver.Edge {
+	return v.inputs
+}
+
+func (v *vertex) Name() string {
+	return v.name
+}