Merge pull request #3744 from KnVerey/plugin-dir-error

Do no require exec/go plugin home to use fn plugins

api/internal/plugins/execplugin/execplugin_test.go

@@ -13,10 +13,10 @@ import (
 	. "sigs.k8s.io/kustomize/api/internal/plugins/execplugin"
 	pLdr "sigs.k8s.io/kustomize/api/internal/plugins/loader"
 	"sigs.k8s.io/kustomize/api/internal/plugins/utils"
-	"sigs.k8s.io/kustomize/api/konfig"
 	fLdr "sigs.k8s.io/kustomize/api/loader"
 	"sigs.k8s.io/kustomize/api/provider"
 	"sigs.k8s.io/kustomize/api/resmap"
+	"sigs.k8s.io/kustomize/api/types"
 )
 
 func TestExecPluginConfig(t *testing.T) {
@@ -45,10 +45,12 @@ s/$BAR/bar baz/g
 		})
 
 	pluginConfig.RemoveBuildAnnotations()
-	p := NewExecPlugin(
-		pLdr.AbsolutePluginPath(
-			konfig.DisabledPluginConfig(),
-			pluginConfig.OrgId()))
+	loader := pLdr.NewLoader(types.DisabledPluginConfig(), rf, fSys)
+	pluginPath, err := loader.AbsolutePluginPath(pluginConfig.OrgId())
+	if err != nil {
+		t.Fatalf("unexpected err: %v", err)
+	}
+	p := NewExecPlugin(pluginPath)
 	// Not checking to see if the plugin is executable,
 	// because this test does not run it.
 	// This tests only covers sending configuration

api/internal/plugins/loader/loader.go

@@ -13,6 +13,7 @@ import (
 	"strings"
 
 	"github.com/pkg/errors"
+	"sigs.k8s.io/kustomize/api/filesys"
 	"sigs.k8s.io/kustomize/api/ifc"
 	"sigs.k8s.io/kustomize/api/internal/plugins/builtinhelpers"
 	"sigs.k8s.io/kustomize/api/internal/plugins/execplugin"
@@ -29,11 +30,16 @@ import (
 type Loader struct {
 	pc *types.PluginConfig
 	rf *resmap.Factory
+	fs filesys.FileSystem
+
+	// absolutePluginHome caches the location of a valid plugin root directory.
+	// It should only be set once the directory's existence has been confirmed.
+	absolutePluginHome string
 }
 
 func NewLoader(
-	pc *types.PluginConfig, rf *resmap.Factory) *Loader {
-	return &Loader{pc: pc, rf: rf}
+	pc *types.PluginConfig, rf *resmap.Factory, fs filesys.FileSystem) *Loader {
+	return &Loader{pc: pc, rf: rf, fs: fs}
 }
 
 func (l *Loader) LoadGenerators(
@@ -95,13 +101,47 @@ func relativePluginPath(id resid.ResId) string {
 		strings.ToLower(id.Kind))
 }
 
-func AbsolutePluginPath(pc *types.PluginConfig, id resid.ResId) string {
-	return filepath.Join(
-		pc.AbsPluginHome, relativePluginPath(id), id.Kind)
+func (l *Loader) AbsolutePluginPath(id resid.ResId) (string, error) {
+	pluginHome, err := l.absPluginHome()
+	if err != nil {
+		return "", err
+	}
+	return filepath.Join(pluginHome, relativePluginPath(id), id.Kind), nil
 }
 
-func (l *Loader) absolutePluginPath(id resid.ResId) string {
-	return AbsolutePluginPath(l.pc, id)
+// absPluginHome is the home of kustomize Exec and Go plugins.
+// Kustomize plugin configuration files are k8s-style objects
+// containing the fields 'apiVersion' and 'kind', e.g.
+//   apiVersion: apps/v1
+//   kind: Deployment
+// kustomize reads plugin configuration data from a file path
+// specified in the 'generators:' or 'transformers:' field of a
+// kustomization file.  For Exec and Go plugins, kustomize
+// uses this data to both locate the plugin and configure it.
+// Each Exec or Go plugin (its code, its tests, its supporting data
+// files, etc.) must be housed in its own directory at
+//   ${absPluginHome}/${pluginApiVersion}/LOWERCASE(${pluginKind})
+// where
+//   - ${absPluginHome} is an absolute path, defined below.
+//   - ${pluginApiVersion} is taken from the plugin config file.
+//   - ${pluginKind} is taken from the plugin config file.
+func (l *Loader) absPluginHome() (string, error) {
+	// External plugins are disabled--return the dummy plugin root.
+	if l.pc.PluginRestrictions != types.PluginRestrictionsNone {
+		return konfig.NoPluginHomeSentinal, nil
+	}
+	// We've already determined plugin home--use the cached value.
+	if l.absolutePluginHome != "" {
+		return l.absolutePluginHome, nil
+	}
+
+	// Check default locations for a valid plugin root, and cache it if found.
+	dir, err := konfig.DefaultAbsPluginHome(l.fs)
+	if err != nil {
+		return "", err
+	}
+	l.absolutePluginHome = dir
+	return l.absolutePluginHome, nil
 }
 
 func isBuiltinPlugin(res *resource.Resource) bool {
@@ -176,10 +216,13 @@ func (l *Loader) loadPlugin(res *resource.Resource) (resmap.Configurable, error)
 }
 
 func (l *Loader) loadExecOrGoPlugin(resId resid.ResId) (resmap.Configurable, error) {
+	absPluginPath, err := l.AbsolutePluginPath(resId)
+	if err != nil {
+		return nil, err
+	}
 	// First try to load the plugin as an executable.
-	p := execplugin.NewExecPlugin(l.absolutePluginPath(resId))
-	err := p.ErrIfNotExecutable()
-	if err == nil {
+	p := execplugin.NewExecPlugin(absPluginPath)
+	if err = p.ErrIfNotExecutable(); err == nil {
 		return p, nil
 	}
 	if !os.IsNotExist(err) {
@@ -193,7 +236,7 @@ func (l *Loader) loadExecOrGoPlugin(resId resid.ResId) (resmap.Configurable, err
 		return nil, err
 	}
 	// Failing the above, try loading it as a Go plugin.
-	c, err := l.loadGoPlugin(resId)
+	c, err := l.loadGoPlugin(resId, absPluginPath+".so")
 	if err != nil {
 		return nil, err
 	}
@@ -208,12 +251,11 @@ func (l *Loader) loadExecOrGoPlugin(resId resid.ResId) (resmap.Configurable, err
 // as a Loader instance variable.  So make it a package variable.
 var registry = make(map[string]resmap.Configurable)
 
-func (l *Loader) loadGoPlugin(id resid.ResId) (resmap.Configurable, error) {
+func (l *Loader) loadGoPlugin(id resid.ResId, absPath string) (resmap.Configurable, error) {
 	regId := relativePluginPath(id)
 	if c, ok := registry[regId]; ok {
 		return copyPlugin(c), nil
 	}
-	absPath := l.absolutePluginPath(id) + ".so"
 	if !utils.FileExists(absPath) {
 		return nil, fmt.Errorf(
 			"expected file with Go object code at: %s", absPath)

api/internal/plugins/loader/loader_test.go

@@ -8,7 +8,6 @@ import (
 
 	"sigs.k8s.io/kustomize/api/filesys"
 	. "sigs.k8s.io/kustomize/api/internal/plugins/loader"
-	"sigs.k8s.io/kustomize/api/konfig"
 	"sigs.k8s.io/kustomize/api/loader"
 	"sigs.k8s.io/kustomize/api/provider"
 	"sigs.k8s.io/kustomize/api/resmap"
@@ -52,9 +51,10 @@ func TestLoader(t *testing.T) {
 	defer th.Reset()
 	p := provider.NewDefaultDepProvider()
 	rmF := resmap.NewFactory(p.GetResourceFactory())
+	fsys := filesys.MakeFsInMemory()
 	fLdr, err := loader.NewLoader(
 		loader.RestrictionRootOnly,
-		filesys.Separator, filesys.MakeFsInMemory())
+		filesys.Separator, fsys)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -66,11 +66,8 @@ func TestLoader(t *testing.T) {
 	for _, behavior := range []types.BuiltinPluginLoadingOptions{
 		/* types.BploUseStaticallyLinked,
 		types.BploLoadFromFileSys */} {
-		c, err := konfig.EnabledPluginConfig(behavior)
-		if err != nil {
-			t.Fatal(err)
-		}
-		pLdr := NewLoader(c, rmF)
+		c := types.EnabledPluginConfig(behavior)
+		pLdr := NewLoader(c, rmF, fsys)
 		if pLdr == nil {
 			t.Fatal("expect non-nil loader")
 		}

api/internal/plugins/utils/utils.go

@@ -34,7 +34,7 @@ func GoBin() string {
 // has her ${g}/${v}/$lower(${k})/${k}.go files.
 func DeterminePluginSrcRoot(fSys filesys.FileSystem) (string, error) {
 	return konfig.FirstDirThatExistsElseError(
-		"source directory", fSys, []konfig.NotedFunc{
+		"plugin src root", fSys, []konfig.NotedFunc{
 			{
 				Note: "relative to unit test",
 				F: func() string {

api/internal/target/maker_test.go

@@ -9,11 +9,11 @@ import (
 	"sigs.k8s.io/kustomize/api/filesys"
 	pLdr "sigs.k8s.io/kustomize/api/internal/plugins/loader"
 	"sigs.k8s.io/kustomize/api/internal/target"
-	"sigs.k8s.io/kustomize/api/konfig"
 	fLdr "sigs.k8s.io/kustomize/api/loader"
 	"sigs.k8s.io/kustomize/api/provider"
 	"sigs.k8s.io/kustomize/api/resmap"
 	valtest_test "sigs.k8s.io/kustomize/api/testutils/valtest"
+	"sigs.k8s.io/kustomize/api/types"
 )
 
 func makeAndLoadKustTarget(
@@ -37,10 +37,10 @@ func makeKustTargetWithRf(
 		t.Fatal(err)
 	}
 	rf := resmap.NewFactory(pvd.GetResourceFactory())
-	pc := konfig.DisabledPluginConfig()
+	pc := types.DisabledPluginConfig()
 	return target.NewKustTarget(
 		ldr,
 		valtest_test.MakeFakeValidator(),
 		rf,
-		pLdr.NewLoader(pc, rf))
+		pLdr.NewLoader(pc, rf, fSys))
 }

api/konfig/plugins.go

@@ -41,32 +41,6 @@ const (
 	NoPluginHomeSentinal = "/No/non-builtin/plugins!"
 )
 
-func EnabledPluginConfig(b types.BuiltinPluginLoadingOptions) (*types.PluginConfig, error) {
-	dir, err := DefaultAbsPluginHome(filesys.MakeFsOnDisk())
-	if err != nil {
-		return nil, err
-	}
-	return MakePluginConfig(types.PluginRestrictionsNone, b, dir), nil
-}
-
-func DisabledPluginConfig() *types.PluginConfig {
-	return MakePluginConfig(
-		types.PluginRestrictionsBuiltinsOnly,
-		types.BploUseStaticallyLinked,
-		NoPluginHomeSentinal)
-}
-
-func MakePluginConfig(
-	pr types.PluginRestrictions,
-	b types.BuiltinPluginLoadingOptions,
-	home string) *types.PluginConfig {
-	return &types.PluginConfig{
-		PluginRestrictions: pr,
-		AbsPluginHome:      home,
-		BpLoadingOptions:   b,
-	}
-}
-
 type NotedFunc struct {
 	Note string
 	F    func() string
@@ -77,7 +51,7 @@ type NotedFunc struct {
 // the home of kustomize plugins.
 func DefaultAbsPluginHome(fSys filesys.FileSystem) (string, error) {
 	return FirstDirThatExistsElseError(
-		"plugin home directory", fSys, []NotedFunc{
+		"plugin root", fSys, []NotedFunc{
 			{
 				Note: "homed in $" + KustomizePluginHomeEnv,
 				F: func() string {
@@ -87,9 +61,11 @@ func DefaultAbsPluginHome(fSys filesys.FileSystem) (string, error) {
 			{
 				Note: "homed in $" + XdgConfigHomeEnv,
 				F: func() string {
-					return filepath.Join(
-						os.Getenv(XdgConfigHomeEnv),
-						ProgramName, RelPluginHome)
+					if root := os.Getenv(XdgConfigHomeEnv); root != "" {
+						return filepath.Join(root, ProgramName, RelPluginHome)
+					}
+					// do not look in "kustomize/plugin" if XdgConfigHomeEnv is unset
+					return ""
 				},
 			},
 			{
@@ -118,11 +94,14 @@ func FirstDirThatExistsElseError(
 	pathFuncs []NotedFunc) (string, error) {
 	var nope []types.Pair
 	for _, dt := range pathFuncs {
-		dir := dt.F()
-		if fSys.Exists(dir) {
-			return dir, nil
+		if dir := dt.F(); dir != "" {
+			if fSys.Exists(dir) {
+				return dir, nil
+			}
+			nope = append(nope, types.Pair{Key: dt.Note, Value: dir})
+		} else {
+			nope = append(nope, types.Pair{Key: dt.Note, Value: "<no value>"})
 		}
-		nope = append(nope, types.Pair{Key: dt.Note, Value: dir})
 	}
 	return "", types.NewErrUnableToFind(what, nope)
 }

api/konfig/plugins_test.go

@@ -8,6 +8,8 @@ import (
 	"path/filepath"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
+
 	"sigs.k8s.io/kustomize/api/filesys"
 	"sigs.k8s.io/kustomize/api/types"
 )
@@ -28,6 +30,34 @@ func TestDefaultAbsPluginHome_NoKustomizePluginHomeEnv(t *testing.T) {
 	if !types.IsErrUnableToFind(err) {
 		t.Fatalf("unexpected err: %v", err)
 	}
+	for _, expectedMsg := range []string{
+		"unable to find plugin root - tried:",
+		"('<no value>'; homed in $KUSTOMIZE_PLUGIN_HOME)",
+		"; homed in $XDG_CONFIG_HOME)",
+		"/.config/kustomize/plugin'; homed in default value of $XDG_CONFIG_HOME)",
+		"/kustomize/plugin'; homed in home directory)",
+	} {
+		assert.Contains(t, err.Error(), expectedMsg)
+	}
+}
+
+func TestDefaultAbsPluginHome_EmptyKustomizePluginHomeEnv(t *testing.T) {
+	keep, isSet := os.LookupEnv(KustomizePluginHomeEnv)
+	os.Setenv(KustomizePluginHomeEnv, "")
+
+	_, err := DefaultAbsPluginHome(filesys.MakeFsInMemory())
+	if !isSet {
+		_ = os.Unsetenv(KustomizePluginHomeEnv)
+	} else {
+		_ = os.Setenv(KustomizePluginHomeEnv, keep)
+	}
+	if err == nil {
+		t.Fatalf("expected err")
+	}
+	if !types.IsErrUnableToFind(err) {
+		t.Fatalf("unexpected err: %v", err)
+	}
+	assert.Contains(t, err.Error(), "('<no value>'; homed in $KUSTOMIZE_PLUGIN_HOME)")
 }
 
 func TestDefaultAbsPluginHome_WithKustomizePluginHomeEnv(t *testing.T) {
@@ -89,6 +119,25 @@ func TestDefaultAbsPluginHomeNoConfig(t *testing.T) {
 	}
 }
 
+func TestDefaultAbsPluginHomeEmptyXdgConfig(t *testing.T) {
+	keep, isSet := os.LookupEnv(XdgConfigHomeEnv)
+	os.Setenv(XdgConfigHomeEnv, "")
+	if isSet {
+		_ = os.Unsetenv(XdgConfigHomeEnv)
+	}
+	_, err := DefaultAbsPluginHome(filesys.MakeFsInMemory())
+	if isSet {
+		os.Setenv(XdgConfigHomeEnv, keep)
+	}
+	if err == nil {
+		t.Fatalf("expected err")
+	}
+	if !types.IsErrUnableToFind(err) {
+		t.Fatalf("unexpected err: %v", err)
+	}
+	assert.Contains(t, err.Error(), "('<no value>'; homed in $XDG_CONFIG_HOME)")
+}
+
 func TestDefaultAbsPluginHomeNoXdgWithDotConfig(t *testing.T) {
 	fSys := filesys.MakeFsInMemory()
 	configDir := filepath.Join(

api/krusty/fnplugin_test.go

@@ -8,8 +8,8 @@ import (
 )
 
 func TestFnExecGenerator(t *testing.T) {
-	th := kusttest_test.MakeEnhancedHarness(t)
-	defer th.Reset()
+	// Function plugins should not need the env setup done by MakeEnhancedHarness
+	th := kusttest_test.MakeHarness(t)
 
 	th.WriteK(".", `
 resources:
@@ -92,8 +92,8 @@ func skipIfNoDocker(t *testing.T) {
 func TestFnContainerGenerator(t *testing.T) {
 	skipIfNoDocker(t)
 
-	th := kusttest_test.MakeEnhancedHarness(t)
-	defer th.Reset()
+	// Function plugins should not need the env setup done by MakeEnhancedHarness
+	th := kusttest_test.MakeHarness(t)
 
 	th.WriteK(".", `
 resources:
@@ -308,8 +308,8 @@ spec:
 func TestFnContainerTransformer(t *testing.T) {
 	skipIfNoDocker(t)
 
-	th := kusttest_test.MakeEnhancedHarness(t)
-	defer th.Reset()
+	// Function plugins should not need the env setup done by MakeEnhancedHarness
+	th := kusttest_test.MakeHarness(t)
 
 	th.WriteK(".", `
 resources:
@@ -408,8 +408,8 @@ spec:
 func TestFnContainerTransformerWithConfig(t *testing.T) {
 	skipIfNoDocker(t)
 
-	th := kusttest_test.MakeEnhancedHarness(t)
-	defer th.Reset()
+	// Function plugins should not need the env setup done by MakeEnhancedHarness
+	th := kusttest_test.MakeHarness(t)
 
 	th.WriteK(".", `
 resources:
@@ -468,8 +468,8 @@ metadata:
 func TestFnContainerEnvVars(t *testing.T) {
 	skipIfNoDocker(t)
 
-	th := kusttest_test.MakeEnhancedHarness(t)
-	defer th.Reset()
+	// Function plugins should not need the env setup done by MakeEnhancedHarness
+	th := kusttest_test.MakeHarness(t)
 
 	th.WriteK(".", `
 generators:

api/krusty/kustomizer.go

@@ -66,7 +66,8 @@ func (b *Kustomizer) Run(
 		ldr,
 		b.depProvider.GetFieldValidator(),
 		resmapFactory,
-		pLdr.NewLoader(b.options.PluginConfig, resmapFactory),
+		// The plugin configs are always located on disk, regardless of the fSys passed in
+		pLdr.NewLoader(b.options.PluginConfig, resmapFactory, filesys.MakeFsOnDisk()),
 	)
 	err = kt.Load()
 	if err != nil {

api/krusty/options.go

@@ -5,7 +5,6 @@ package krusty
 
 import (
 	"sigs.k8s.io/kustomize/api/internal/plugins/builtinhelpers"
-	"sigs.k8s.io/kustomize/api/konfig"
 	"sigs.k8s.io/kustomize/api/types"
 )
 
@@ -42,7 +41,7 @@ func MakeDefaultOptions() *Options {
 		AddManagedbyLabel:    false,
 		LoadRestrictions:     types.LoadRestrictionsRootOnly,
 		DoPrune:              false,
-		PluginConfig:         konfig.DisabledPluginConfig(),
+		PluginConfig:         types.DisabledPluginConfig(),
 	}
 }
 

api/testutils/kusttest/harness.go

@@ -5,7 +5,6 @@ package kusttest_test
 
 import (
 	"path/filepath"
-	"strings"
 	"testing"
 
 	"sigs.k8s.io/kustomize/api/filesys"
@@ -77,15 +76,7 @@ func (th Harness) MakeOptionsPluginsDisabled() krusty.Options {
 
 // Enables use of non-builtin plugins.
 func (th Harness) MakeOptionsPluginsEnabled() krusty.Options {
-	pc, err := konfig.EnabledPluginConfig(types.BploLoadFromFileSys)
-	if err != nil {
-		if strings.Contains(err.Error(), "unable to find plugin root") {
-			th.t.Log(
-				"Tests that want to run with plugins enabled must be " +
-					"bookended by calls to MakeEnhancedHarness(), Reset().")
-		}
-		th.t.Fatal(err)
-	}
+	pc := types.EnabledPluginConfig(types.BploLoadFromFileSys)
 	o := *krusty.MakeDefaultOptions()
 	o.PluginConfig = pc
 	return o

api/testutils/kusttest/harnessenhanced.go

@@ -40,11 +40,8 @@ type HarnessEnhanced struct {
 func MakeEnhancedHarness(t *testing.T) *HarnessEnhanced {
 	pte := newPluginTestEnv(t).set()
 
-	pc, err := konfig.EnabledPluginConfig(types.BploLoadFromFileSys)
+	pc := types.EnabledPluginConfig(types.BploLoadFromFileSys)
 	pc.FnpLoadingOptions.EnableStar = true
-	if err != nil {
-		t.Fatal(err)
-	}
 	p := provider.NewDefaultDepProvider()
 	resourceFactory := p.GetResourceFactory()
 	resmapFactory := resmap.NewFactory(resourceFactory)
@@ -53,7 +50,8 @@ func MakeEnhancedHarness(t *testing.T) *HarnessEnhanced {
 		Harness: MakeHarness(t),
 		pte:     pte,
 		rf:      resmapFactory,
-		pl:      pLdr.NewLoader(pc, resmapFactory)}
+		// The plugin configs are always located on disk, regardless of the test harness's FS
+		pl: pLdr.NewLoader(pc, resmapFactory, filesys.MakeFsOnDisk())}
 
 	// Point the file loader to the root ('/') of the in-memory file system.
 	result.ResetLoaderRoot(filesys.Separator)

api/types/errunabletofind.go

@@ -23,7 +23,7 @@ func (e *errUnableToFind) Error() string {
 		m = append(m, "('"+p.Value+"'; "+p.Key+")")
 	}
 	return fmt.Sprintf(
-		"unable to find plugin root - tried: %s", strings.Join(m, ", "))
+		"unable to find %s - tried: %s", e.what, strings.Join(m, ", "))
 }
 
 func NewErrUnableToFind(w string, a []Pair) *errUnableToFind {

api/types/pluginconfig.go

@@ -5,25 +5,6 @@ package types
 
 // PluginConfig holds plugin configuration.
 type PluginConfig struct {
-	// AbsPluginHome is the home of kustomize plugins.
-	// Kustomize plugin configuration files are k8s-style objects
-	// containing the fields 'apiVersion' and 'kind', e.g.
-	//   apiVersion: apps/v1
-	//   kind: Deployment
-	// kustomize reads plugin configuration data from a file path
-	// specified in the 'generators:' or 'transformers:' field of a
-	// kustomization file.  kustomize must then use this data to both
-	// locate the plugin and configure it.
-	// Every kustomize plugin (its code, its tests, its supporting data
-	// files, etc.) must be housed in its own directory at
-	//   ${AbsPluginHome}/${pluginApiVersion}/LOWERCASE(${pluginKind})
-	// where
-	//   - ${AbsPluginHome} is an absolute path, defined below.
-	//   - ${pluginApiVersion} is taken from the plugin config file.
-	//   - ${pluginKind} is taken from the plugin config file.
-	// The value of AbsPluginHome can be any absolute path.
-	AbsPluginHome string
-
 	// PluginRestrictions distinguishes plugin restrictions.
 	PluginRestrictions PluginRestrictions
 
@@ -33,3 +14,21 @@ type PluginConfig struct {
 	// FnpLoadingOptions sets the way function-based plugin behaviors.
 	FnpLoadingOptions FnPluginLoadingOptions
 }
+
+func EnabledPluginConfig(b BuiltinPluginLoadingOptions) *PluginConfig {
+	return MakePluginConfig(PluginRestrictionsNone, b)
+}
+
+func DisabledPluginConfig() *PluginConfig {
+	return MakePluginConfig(
+		PluginRestrictionsBuiltinsOnly,
+		BploUseStaticallyLinked)
+}
+
+func MakePluginConfig(pr PluginRestrictions,
+	b BuiltinPluginLoadingOptions) *PluginConfig {
+	return &PluginConfig{
+		PluginRestrictions: pr,
+		BpLoadingOptions:   b,
+	}
+}