Merge pull request #749 from MikaelSmith/hide-fs-on-exec-fail

Hide fs on exec fail

plugin/aws/ec2Instance.go

@@ -217,7 +217,7 @@ func (inst *ec2Instance) List(ctx context.Context) ([]plugin.Entry, error) {
 
 	// Include a view of the remote filesystem using volume.FS. Use a small maxdepth because
 	// VMs can have lots of files and SSH is fast.
-	entries = append(entries, volume.NewFS("fs", inst, 3))
+	entries = append(entries, volume.NewFS(ctx, "fs", inst, 3))
 
 	return entries, nil
 }

plugin/docker/container.go

@@ -91,7 +91,7 @@ func (c *container) List(ctx context.Context) ([]plugin.Entry, error) {
 
 	// Include a view of the remote filesystem using volume.FS. Use a small maxdepth because
 	// VMs can have lots of files and Exec is fast.
-	return []plugin.Entry{clf, cm, vol.NewFS("fs", c, 3)}, nil
+	return []plugin.Entry{clf, cm, vol.NewFS(ctx, "fs", c, 3)}, nil
 }
 
 func (c *container) Delete(ctx context.Context) (bool, error) {

plugin/entryBase.go

@@ -142,7 +142,7 @@ func (e *EntryBase) SetPartialMetadata(obj interface{}) *EntryBase {
 // MarkInaccessible sets the inaccessible attribute and logs a message about why the entry is
 // inaccessible.
 func (e *EntryBase) MarkInaccessible(ctx context.Context, err error) {
-	activity.Record(ctx, "Omitting %v: %v", e.id, err)
+	activity.Warnf(ctx, "Omitting %v: %v", e.id, err)
 	e.isInaccessible = true
 }
 

plugin/external/coreEntries.go

@@ -1,6 +1,7 @@
 package external
 
 import (
+	"context"
 	"encoding/json"
 	"fmt"
 
@@ -9,7 +10,7 @@ import (
 )
 
 type coreEntry interface {
-	createInstance(parent *pluginEntry, decodedEntry decodedExternalPluginEntry) (plugin.Entry, error)
+	createInstance(ctx context.Context, parent *pluginEntry, decodedEntry decodedExternalPluginEntry) (plugin.Entry, error)
 	template() plugin.Entry
 }
 
@@ -19,7 +20,7 @@ var coreEntries = map[string]coreEntry{
 
 type volumeFS struct{}
 
-func (volumeFS) createInstance(parent *pluginEntry, e decodedExternalPluginEntry) (plugin.Entry, error) {
+func (volumeFS) createInstance(ctx context.Context, parent *pluginEntry, e decodedExternalPluginEntry) (plugin.Entry, error) {
 	var opts struct{ Maxdepth uint }
 	// Use a default of 3 if unspecified.
 	opts.Maxdepth = 3
@@ -28,7 +29,7 @@ func (volumeFS) createInstance(parent *pluginEntry, e decodedExternalPluginEntry
 		return nil, fmt.Errorf("volume filesystem options invalid: %v", err)
 	}
 
-	return volume.NewFS(e.Name, parent, int(opts.Maxdepth)), nil
+	return volume.NewFS(ctx, e.Name, parent, int(opts.Maxdepth)), nil
 }
 
 func (volumeFS) template() plugin.Entry {

plugin/external/pluginEntry.go

@@ -408,7 +408,7 @@ func (e *pluginEntry) List(ctx context.Context) ([]plugin.Entry, error) {
 	entries := make([]plugin.Entry, len(decodedEntries))
 	for i, decodedExternalPluginEntry := range decodedEntries {
 		if coreEnt, ok := coreEntries[decodedExternalPluginEntry.TypeID]; ok {
-			entry, err := coreEnt.createInstance(e, decodedExternalPluginEntry)
+			entry, err := coreEnt.createInstance(ctx, e, decodedExternalPluginEntry)
 			if err != nil {
 				return nil, err
 			}

plugin/external/pluginEntry_test.go

@@ -13,6 +13,7 @@ import (
 	"time"
 
 	"github.com/emirpasic/gods/maps/linkedhashmap"
+	"github.com/puppetlabs/wash/datastore"
 	"github.com/puppetlabs/wash/plugin"
 	"github.com/puppetlabs/wash/transport"
 	"github.com/puppetlabs/wash/volume"
@@ -667,21 +668,28 @@ func (suite *ExternalPluginEntryTestSuite) TestListReadWithMethodResults() {
 }
 
 func (suite *ExternalPluginEntryTestSuite) TestListWithCoreEntry() {
+	// Provide a test cache because listing FS invokes `plugin.List`.
+	ctx := plugin.SetTestCache(datastore.NewMemCache())
+	defer plugin.UnsetTestCache()
+
 	mockScript := &mockPluginScript{path: "plugin_script"}
 	entry := &pluginEntry{
 		EntryBase: plugin.NewEntry("foo"),
 		script:    mockScript,
 	}
 	entry.SetTestID("/foo")
 
-	ctx := context.Background()
 	stdout := []byte(`
 [
 	{"name": "bar", "methods": ["list"]},
 	{"type_id": "__volume::fs__", "name": "fs1", "state": "{\"maxdepth\": 2}"}
 ]`)
 	mockScript.OnInvokeAndWait(ctx, "list", entry).Return(mockInvocation(stdout), nil).Once()
 
+	mockInv := &mockedInvocation{Command: NewCommand(ctx, "")}
+	mockInv.MockExec(nil, nil, 0)
+	mockScript.On("NewInvocation", mock.Anything, "exec", entry, mock.Anything).Return(mockInv).Once()
+
 	entries, err := entry.List(ctx)
 	if suite.NoError(err) {
 		suite.Equal(2, len(entries))
@@ -810,6 +818,12 @@ func (m *mockedInvocation) Stderr() *bytes.Buffer {
 	return m.Called().Get(0).(*bytes.Buffer)
 }
 
+func (m *mockedInvocation) MockExec(startErr, waitErr error, exitCode int) {
+	m.On("Start").Return(startErr).Once()
+	m.On("Wait").Return(waitErr).Once()
+	m.On("ExitCode").Return(exitCode).Once()
+}
+
 var _ = invocation(&mockedInvocation{})
 
 func (suite *ExternalPluginEntryTestSuite) TestWrite() {
@@ -942,9 +956,7 @@ func (suite *ExternalPluginEntryTestSuite) TestExec() {
 		mockInv := &mockedInvocation{Command: NewCommand(ctx, "")}
 		args := []interface{}{ctx, "exec", entry, append([]string{`{"tty":false,"elevate":false,"stdin":false}`}, cmd...)}
 		mockScript.On("NewInvocation", args...).Return(mockInv).Once()
-		mockInv.On("Start").Return(startErr).Once()
-		mockInv.On("Wait").Return(waitErr).Once()
-		mockInv.On("ExitCode").Return(exitCode).Once()
+		mockInv.MockExec(startErr, waitErr, exitCode)
 	}
 
 	// Test that if Start errors, then Exec returns its error

plugin/gcp/computeInst.go

@@ -60,7 +60,7 @@ func (c *computeInstance) List(ctx context.Context) ([]plugin.Entry, error) {
 		metadataJSONFile,
 		// Include a view of the remote filesystem using volume.FS. Use a small maxdepth because
 		// VMs can have lots of files and SSH is fast.
-		volume.NewFS("fs", c, 3),
+		volume.NewFS(ctx, "fs", c, 3),
 	}, nil
 }
 

volume/fs.go

@@ -20,13 +20,18 @@ type FS struct {
 
 // NewFS creates a new FS entry with the given name, using the supplied executor to satisfy volume
 // operations.
-func NewFS(name string, executor plugin.Execable, maxdepth int) *FS {
+func NewFS(ctx context.Context, name string, executor plugin.Execable, maxdepth int) *FS {
 	fs := &FS{
 		EntryBase: plugin.NewEntry(name),
 	}
 	fs.executor = executor
 	fs.maxdepth = maxdepth
 	fs.SetTTLOf(plugin.ListOp, ListTTL)
+
+	if _, err := plugin.List(ctx, fs); err != nil {
+		fs.MarkInaccessible(ctx, err)
+	}
+
 	return fs
 }
 

volume/fs_test.go

@@ -3,7 +3,6 @@ package volume
 import (
 	"context"
 	"fmt"
-	"io"
 	"sort"
 	"strings"
 	"testing"
@@ -74,7 +73,6 @@ type fsTestSuite struct {
 	cancelFunc                context.CancelFunc
 	loginShell                plugin.Shell
 	statCmd                   func(path string, maxdepth int) []string
-	parseOutput               func(output io.Reader, base string, start string, maxdepth int) (DirMap, error)
 	outputFixture             string
 	outputDepth               int
 	shortFixture, deepFixture string
@@ -83,8 +81,8 @@ type fsTestSuite struct {
 
 func (suite *fsTestSuite) SetupTest() {
 	// Use a different cache each test because we may re-use some but not all of the same structure.
-	plugin.SetTestCache(datastore.NewMemCache())
-	suite.ctx, suite.cancelFunc = context.WithCancel(context.Background())
+	ctx := plugin.SetTestCache(datastore.NewMemCache())
+	suite.ctx, suite.cancelFunc = context.WithCancel(ctx)
 }
 
 func (suite *fsTestSuite) TearDownTest() {
@@ -141,12 +139,10 @@ func (suite *fsTestSuite) TestFSList() {
 	exec := suite.createExec()
 	exec.onExec(suite.statCmd("/", suite.outputDepth), suite.createResult(suite.outputFixture))
 
-	fs := NewFS("fs", exec, suite.outputDepth)
-	// ID would normally be set when listing FS within the parent instance.
-	fs.SetTestID("/instance/fs")
+	fs := NewFS(suite.ctx, "fs", exec, suite.outputDepth)
 
 	entry := suite.find(fs, "var/log").(plugin.Parent)
-	entries, err := entry.List(context.Background())
+	entries, err := entry.List(suite.ctx)
 	if !suite.NoError(err) {
 		suite.FailNow("Listing entries failed")
 	}
@@ -165,15 +161,15 @@ func (suite *fsTestSuite) TestFSList() {
 		}
 	}
 
-	entries1, err := entries[1].(plugin.Parent).List(context.Background())
+	entries1, err := entries[1].(plugin.Parent).List(suite.ctx)
 	if suite.NoError(err) {
 		suite.Equal(1, len(entries1))
 		suite.Equal("a file", plugin.Name(entries1[0]))
 		_, ok := entries1[0].(plugin.Readable)
 		suite.True(ok)
 	}
 
-	entries2, err := entries[2].(plugin.Parent).List(context.Background())
+	entries2, err := entries[2].(plugin.Parent).List(suite.ctx)
 	if suite.NoError(err) {
 		suite.Equal(1, len(entries2))
 		suite.Equal("dir", plugin.Name(entries2[0]))
@@ -189,9 +185,7 @@ func (suite *fsTestSuite) TestFSListTwice() {
 	exec.onExec(suite.statCmd("/", depth), suite.createResult(suite.shortFixture))
 	exec.onExec(suite.statCmd("/var/log/path", depth), suite.createResult(suite.deepFixture))
 
-	fs := NewFS("fs", exec, depth)
-	// ID would normally be set when listing FS within the parent instance.
-	fs.SetTestID("/instance/fs")
+	fs := NewFS(suite.ctx, "fs", exec, depth)
 
 	entry := suite.find(fs, "var/log").(plugin.Parent)
 	entries, err := plugin.List(suite.ctx, entry)
@@ -219,9 +213,7 @@ func (suite *fsTestSuite) TestFSListExpiredCache() {
 	exec := suite.createExec()
 	exec.onExec(suite.statCmd("/", depth), mockExecCmd{suite.shortFixture}).Twice()
 
-	fs := NewFS("fs", exec, depth)
-	// ID would normally be set when listing FS within the parent instance.
-	fs.SetTestID("/instance/fs")
+	fs := NewFS(suite.ctx, "fs", exec, depth)
 
 	entry := suite.find(fs, "var/log").(plugin.Parent)
 	entries, err := plugin.List(suite.ctx, entry)
@@ -231,8 +223,8 @@ func (suite *fsTestSuite) TestFSListExpiredCache() {
 	suite.Equal(1, entries.Len())
 	suite.Contains(entries.Map(), "path")
 
-	cleared := plugin.ClearCacheFor("/instance/fs", false)
-	suite.Equal([]string{"List::/instance/fs"}, cleared)
+	cleared := plugin.ClearCacheFor("/fs", false)
+	suite.Equal([]string{"List::/fs"}, cleared)
 	entries, err = plugin.List(suite.ctx, entry)
 	if suite.NoError(err) {
 		suite.Equal(1, entries.Len())
@@ -245,28 +237,25 @@ func (suite *fsTestSuite) TestFSRead() {
 	exec := suite.createExec()
 	exec.onExec(suite.statCmd("/", suite.outputDepth), suite.createResult(suite.outputFixture))
 
-	fs := NewFS("fs", exec, suite.outputDepth)
-	// ID would normally be set when listing FS within the parent instance.
-	fs.SetTestID("/instance/fs")
+	fs := NewFS(suite.ctx, "fs", exec, suite.outputDepth)
 
 	entry := suite.find(fs, "var/log/path1/a file")
 	suite.Equal("a file", plugin.Name(entry))
 	exec.onExec(suite.readCmdFn("/var/log/path1/a file"), suite.createResult("hello"))
 
-	content, err := entry.(plugin.Readable).Read(context.Background())
+	content, err := entry.(plugin.Readable).Read(suite.ctx)
 	suite.NoError(err)
 	suite.Equal([]byte("hello"), content)
 	exec.AssertExpectations(suite.T())
 }
 
 func (suite *fsTestSuite) TestVolumeDelete() {
 	exec := suite.createExec()
-	fs := NewFS("fs", exec, suite.outputDepth)
-	// ID would normally be set when listing FS within the parent instance.
-	fs.SetTestID("/instance/fs")
+	exec.onExec(suite.statCmd("/", suite.outputDepth), suite.createResult(suite.outputFixture))
+	fs := NewFS(suite.ctx, "fs", exec, suite.outputDepth)
 
 	exec.onExec(suite.deleteCmdFn("/var/log/path1/a file"), suite.createResult("deleted"))
-	deleted, err := fs.VolumeDelete(context.Background(), "/var/log/path1/a file")
+	deleted, err := fs.VolumeDelete(suite.ctx, "/var/log/path1/a file")
 	suite.NoError(err)
 	suite.True(deleted)
 	exec.AssertExpectations(suite.T())
@@ -276,7 +265,6 @@ func TestPOSIXFS(t *testing.T) {
 	suite.Run(t, &fsTestSuite{
 		loginShell:    plugin.POSIXShell,
 		statCmd:       StatCmdPOSIX,
-		parseOutput:   ParseStatPOSIX,
 		outputFixture: posixFixture,
 		outputDepth:   fixtureDepth,
 		shortFixture:  posixFixtureShort,
@@ -290,7 +278,6 @@ func TestPowershellFS(t *testing.T) {
 	suite.Run(t, &fsTestSuite{
 		loginShell:    plugin.PowerShell,
 		statCmd:       StatCmdPowershell,
-		parseOutput:   ParseStatPowershell,
 		outputFixture: powershellFixture,
 		outputDepth:   fixtureDepth,
 		shortFixture:  powershellFixtureShort,
@@ -316,8 +303,7 @@ func (m *mockExecutor) Schema() *plugin.EntrySchema {
 }
 
 func (m *mockExecutor) onExec(cmd []string, result plugin.ExecCommand) *mock.Call {
-	return m.On("Exec", mock.Anything, cmd[0], cmd[1:], plugin.ExecOptions{Elevate: true}).
-		Return(result, nil)
+	return m.On("Exec", mock.Anything, cmd[0], cmd[1:], mock.Anything).Return(result, nil)
 }
 
 // Mock ExecCommand that can be used repeatedly when mocking a repeated call.

volume/statPosix_test.go

@@ -223,3 +223,11 @@ func TestNumPathSegments(t *testing.T) {
 	assert.Equal(t, 1, numPathSegments("/foo"))
 	assert.Equal(t, 2, numPathSegments("/foo/bar"))
 }
+
+func TestNormalErrorPOSIX(t *testing.T) {
+	assert.False(t, NormalErrorPOSIX(""))
+	assert.False(t, NormalErrorPOSIX("anything"))
+
+	assert.True(t, NormalErrorPOSIX("find: a mistake"))
+	assert.True(t, NormalErrorPOSIX("stat: something absent"))
+}

volume/statPowershell_test.go

@@ -96,3 +96,8 @@ func TestParseStatPowershell_Empty(t *testing.T) {
 	assert.Len(t, dmap, 1)
 	assert.Empty(t, dmap[RootPath])
 }
+
+func TestNormalErrorPowerShell(t *testing.T) {
+	assert.False(t, NormalErrorPowerShell(""))
+	assert.False(t, NormalErrorPowerShell("anything"))
+}