Merge pull request #1724 from monopole/removeErrorFromConstructor

Improve error messaging around plugin loading problems.

api/krusty/kustomizer.go

@@ -28,11 +28,6 @@ type Kustomizer struct {
 	options *Options
 }
 
-// MakeDefaultKustomizer returns a Kustomizer with default configuration.
-func MakeDefaultKustomizer() *Kustomizer {
-	return MakeKustomizer(filesys.MakeFsOnDisk(), MakeDefaultOptions())
-}
-
 // MakeKustomizer returns an instance of Kustomizer.
 func MakeKustomizer(fSys filesys.FileSystem, o *Options) *Kustomizer {
 	return &Kustomizer{fSys: fSys, options: o}

api/krusty/options.go

@@ -35,6 +35,6 @@ func MakeDefaultOptions() *Options {
 		DoLegacyResourceSort: true,
 		LoadRestrictions:     types.LoadRestrictionsRootOnly,
 		DoPrune:              false,
-		PluginConfig:         pgmconfig.DefaultPluginConfig(),
+		PluginConfig:         pgmconfig.DisabledPluginConfig(),
 	}
 }

api/pgmconfig/pluginconfig.go

@@ -36,19 +36,27 @@ const (
 	DomainName = "sigs.k8s.io"
 )
 
-func ActivePluginConfig() *types.PluginConfig {
-	pc := DefaultPluginConfig()
-	pc.PluginRestrictions = types.PluginRestrictionsNone
-	return pc
+func EnabledPluginConfig() *types.PluginConfig {
+	return MakePluginConfig(
+		types.PluginRestrictionsNone, DefaultAbsPluginHome())
 }
 
-func DefaultPluginConfig() *types.PluginConfig {
+func DisabledPluginConfig() *types.PluginConfig {
+	return MakePluginConfig(
+		types.PluginRestrictionsBuiltinsOnly, NoPluginHomeSentinal)
+}
+
+func MakePluginConfig(
+	pr types.PluginRestrictions, home string) *types.PluginConfig {
 	return &types.PluginConfig{
-		PluginRestrictions: types.PluginRestrictionsBuiltinsOnly,
-		AbsPluginHome:      DefaultAbsPluginHome(),
+		PluginRestrictions: pr,
+		AbsPluginHome:      home,
 	}
 }
 
+// Use an obviously erroneous path, in case it's accidentally used.
+const NoPluginHomeSentinal = "/no/non-builtin/plugins!"
+
 func DefaultAbsPluginHome() string {
 	return filepath.Join(configRoot(), RelPluginHome)
 }

api/plugins/compiler/compiler.go

@@ -41,7 +41,7 @@ func DefaultSrcRoot() (string, error) {
 	}
 	nope = append(nope, root)
 
-	root = pgmconfig.DefaultPluginConfig().AbsPluginHome
+	root = pgmconfig.DefaultAbsPluginHome()
 	if FileExists(root) {
 		return root, nil
 	}

api/plugins/execplugin/execplugin.go

@@ -44,15 +44,19 @@ type ExecPlugin struct {
 	h *resmap.PluginHelpers
 }
 
-func NewExecPlugin(p string) (*ExecPlugin, error) {
-	f, err := os.Stat(p)
+func NewExecPlugin(p string) *ExecPlugin {
+	return &ExecPlugin{path: p}
+}
+
+func (p *ExecPlugin) ErrIfNotExecutable() error {
+	f, err := os.Stat(p.path)
 	if err != nil {
-		return nil, err
+		return err
 	}
 	if f.Mode()&0111 == 0000 {
-		return nil, fmt.Errorf("unable to execute plugin on path: %s", p)
+		return fmt.Errorf("unexecutable plugin at: %s", p.path)
 	}
-	return &ExecPlugin{path: p}, nil
+	return nil
 }
 
 func (p *ExecPlugin) Path() string {

api/plugins/execplugin/execplugin_test.go

@@ -5,7 +5,6 @@ package execplugin_test
 
 import (
 	"fmt"
-	"os"
 	"strings"
 	"testing"
 
@@ -44,13 +43,15 @@ s/$BAR/bar/g
  \ \ \ 
 `))
 
-	p, err := NewExecPlugin(
+	p := NewExecPlugin(
 		loader.AbsolutePluginPath(
-			pgmconfig.DefaultPluginConfig(),
+			pgmconfig.DisabledPluginConfig(),
 			pluginConfig.OrgId()))
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err.Error())
-	}
+	// Not checking to see if the plugin is executable,
+	// because this test does not run it.
+	// This tests only covers sending configuration
+	// to the plugin wrapper object and confirming
+	// that it's properly prepared for execution.
 
 	yaml, err := pluginConfig.AsYAML()
 	if err != nil {
@@ -58,7 +59,7 @@ s/$BAR/bar/g
 	}
 	p.Config(resmap.NewPluginHelpers(ldr, v, rf), yaml)
 
-	expected := "/kustomize/plugin/someteam.example.com/v1/sedtransformer/SedTransformer"
+	expected := "someteam.example.com/v1/sedtransformer/SedTransformer"
 	if !strings.HasSuffix(p.Path(), expected) {
 		t.Fatalf("expected suffix '%s', got '%s'", expected, p.Path())
 	}
@@ -118,9 +119,9 @@ func strptr(s string) *string {
 }
 
 func TestUpdateResourceOptions(t *testing.T) {
-	p, err := NewExecPlugin("")
-	if !os.IsNotExist(err) {
-		t.Fatalf("unexpected error: %v", err.Error())
+	p := NewExecPlugin("")
+	if err := p.ErrIfNotExecutable(); err == nil {
+		t.Fatalf("expected unexecutable error")
 	}
 	rf := resource.NewFactory(kunstruct.NewKunstructuredFactoryImpl())
 	in := resmap.New()
@@ -166,9 +167,9 @@ func TestUpdateResourceOptions(t *testing.T) {
 }
 
 func TestUpdateResourceOptionsWithInvalidHashAnnotationValues(t *testing.T) {
-	p, err := NewExecPlugin("")
-	if !os.IsNotExist(err) {
-		t.Fatalf("unexpected error: %v", err.Error())
+	p := NewExecPlugin("")
+	if err := p.ErrIfNotExecutable(); err == nil {
+		t.Fatalf("expected unexecutable error")
 	}
 	rf := resource.NewFactory(kunstruct.NewKunstructuredFactoryImpl())
 	cases := []string{

api/plugins/loader/loader.go

@@ -146,13 +146,23 @@ func (l *Loader) makeBuiltinPlugin(r resid.Gvk) (resmap.Configurable, error) {
 }
 
 func (l *Loader) loadPlugin(resId resid.ResId) (resmap.Configurable, error) {
-	p, err := execplugin.NewExecPlugin(l.absolutePluginPath(resId))
+	// First try to load the plugin as an executable.
+	p := execplugin.NewExecPlugin(l.absolutePluginPath(resId))
+	err := p.ErrIfNotExecutable()
 	if err == nil {
 		return p, nil
 	}
 	if !os.IsNotExist(err) {
+		// The file exists, but something else is wrong,
+		// likely it's not executable.
+		// Assume the user forgot to set the exec bit,
+		// and return an error, rather than adding ".so"
+		// to the name and attempting to load it as a Go
+		// plugin, which will likely fail and result
+		// in an obscure message.
 		return nil, err
 	}
+	// Failing the above, try loading it as a Go plugin.
 	c, err := l.loadGoPlugin(resId)
 	if err != nil {
 		return nil, err
@@ -186,7 +196,7 @@ func (l *Loader) loadGoPlugin(id resid.ResId) (resmap.Configurable, error) {
 	}
 	c, ok := symbol.(resmap.Configurable)
 	if !ok {
-		return nil, fmt.Errorf("plugin %s not configurable", regId)
+		return nil, fmt.Errorf("plugin '%s' not configurable", regId)
 	}
 	registry[regId] = c
 	return copyPlugin(c), nil

api/plugins/loader/loader_test.go

@@ -57,7 +57,7 @@ func TestLoader(t *testing.T) {
 
 	ldr := loadertest.NewFakeLoader("/foo")
 
-	pLdr := NewLoader(pgmconfig.ActivePluginConfig(), rmF)
+	pLdr := NewLoader(pgmconfig.EnabledPluginConfig(), rmF)
 	if pLdr == nil {
 		t.Fatal("expect non-nil loader")
 	}

api/target/baseandoverlaysmall_test.go

@@ -300,7 +300,7 @@ spec:
 func TestSharedPatchDisAllowed(t *testing.T) {
 	th := kusttest_test.NewKustTestHarnessFull(
 		t, "/app/overlay",
-		loader.RestrictionRootOnly, pgmconfig.DefaultPluginConfig())
+		loader.RestrictionRootOnly, pgmconfig.DisabledPluginConfig())
 	writeSmallBase(th)
 	th.WriteK("/app/overlay", `
 commonLabels:
@@ -332,7 +332,7 @@ spec:
 func TestSharedPatchAllowed(t *testing.T) {
 	th := kusttest_test.NewKustTestHarnessFull(
 		t, "/app/overlay",
-		loader.RestrictionNone, pgmconfig.DefaultPluginConfig())
+		loader.RestrictionNone, pgmconfig.DisabledPluginConfig())
 	writeSmallBase(th)
 	th.WriteK("/app/overlay", `
 commonLabels:

api/target/plugindir_test.go

@@ -65,7 +65,7 @@ metadata:
 	rf := resmap.NewFactory(resource.NewFactory(
 		kunstruct.NewKunstructuredFactoryImpl()), nil)
 
-	pl := pLdr.NewLoader(pgmconfig.ActivePluginConfig(), rf)
+	pl := pLdr.NewLoader(pgmconfig.EnabledPluginConfig(), rf)
 	tg, err := target.NewKustTarget(
 		ldr, valtest_test.MakeFakeValidator(), rf, transformer.NewFactoryImpl(), pl)
 	if err != nil {

api/testutils/kusttest/kusttestharness.go

@@ -36,17 +36,17 @@ type KustTestHarness struct {
 
 func NewKustTestHarness(t *testing.T, path string) *KustTestHarness {
 	return NewKustTestHarnessFull(
-		t, path, fLdr.RestrictionRootOnly, pgmconfig.DefaultPluginConfig())
+		t, path, fLdr.RestrictionRootOnly, pgmconfig.DisabledPluginConfig())
 }
 
 func NewKustTestHarnessAllowPlugins(t *testing.T, path string) *KustTestHarness {
 	return NewKustTestHarnessFull(
-		t, path, fLdr.RestrictionRootOnly, pgmconfig.ActivePluginConfig())
+		t, path, fLdr.RestrictionRootOnly, pgmconfig.EnabledPluginConfig())
 }
 
 func NewKustTestHarnessNoLoadRestrictor(t *testing.T, path string) *KustTestHarness {
 	return NewKustTestHarnessFull(
-		t, path, fLdr.RestrictionNone, pgmconfig.DefaultPluginConfig())
+		t, path, fLdr.RestrictionNone, pgmconfig.DisabledPluginConfig())
 }
 
 func NewKustTestHarnessFull(

kustomize/internal/commands/build/build.go

@@ -15,7 +15,6 @@ import (
 	"sigs.k8s.io/kustomize/api/pgmconfig"
 	"sigs.k8s.io/kustomize/api/resmap"
 	"sigs.k8s.io/kustomize/api/resource"
-	"sigs.k8s.io/kustomize/api/types"
 	"sigs.k8s.io/yaml"
 )
 
@@ -103,12 +102,15 @@ func (o *Options) Validate(args []string) (err error) {
 }
 
 func (o *Options) makeOptions() *krusty.Options {
-	opts := krusty.MakeDefaultOptions()
-	opts.LoadRestrictions = getFlagLoadRestrictorValue()
-	opts.DoLegacyResourceSort = o.outOrder == legacy
-	opts.PluginConfig.PluginRestrictions = types.PluginRestrictionsBuiltinsOnly
+	opts := &krusty.Options{
+		DoLegacyResourceSort: o.outOrder == legacy,
+		LoadRestrictions:     getFlagLoadRestrictorValue(),
+		DoPrune:              false,
+	}
 	if isFlagEnablePluginsSet() {
-		opts.PluginConfig.PluginRestrictions = types.PluginRestrictionsNone
+		opts.PluginConfig = pgmconfig.EnabledPluginConfig()
+	} else {
+		opts.PluginConfig = pgmconfig.DisabledPluginConfig()
 	}
 	return opts
 }