diff --git a/cmd/wfctl/deploy_providers.go b/cmd/wfctl/deploy_providers.go index 69b238362..16d8c6210 100644 --- a/cmd/wfctl/deploy_providers.go +++ b/cmd/wfctl/deploy_providers.go @@ -19,6 +19,7 @@ import ( "github.com/GoCodeAlone/workflow/config" "github.com/GoCodeAlone/workflow/interfaces" + "github.com/GoCodeAlone/workflow/plugin" "github.com/GoCodeAlone/workflow/plugin/external" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" @@ -120,6 +121,31 @@ func findIaCPluginDir(pluginDir, providerName string) (name string, hasBinary bo return "", false, nil } +// loadIaCPlugin finds and loads the IaC plugin for the given provider, returning +// its name, module factories, and a closer that shuts down the plugin subprocess. +// Tests replace this var to inject fakes without touching the filesystem. +var loadIaCPlugin = defaultLoadIaCPlugin + +func defaultLoadIaCPlugin(pluginDir, providerName string) (pluginName string, factories map[string]plugin.ModuleFactory, closer io.Closer, err error) { + pName, hasBinary, findErr := findIaCPluginDir(pluginDir, providerName) + if findErr != nil { + return "", nil, nil, fmt.Errorf("resolve IaC provider %q: %w", providerName, findErr) + } + if pName == "" { + return "", nil, nil, fmt.Errorf("no plugin found for IaC provider %q in %s — run: wfctl plugin install ", providerName, pluginDir) + } + if !hasBinary { + return "", nil, nil, fmt.Errorf("plugin %q declares provider %q but binary is missing — run: wfctl plugin install %s", pName, providerName, pName) + } + mgr := external.NewExternalPluginManager(pluginDir, nil) + adapter, loadErr := mgr.LoadPlugin(pName) + if loadErr != nil { + mgr.Shutdown() + return "", nil, nil, fmt.Errorf("load plugin %q for provider %q: %w", pName, providerName, loadErr) + } + return pName, adapter.ModuleFactories(), closerFunc(func() error { mgr.Shutdown(); return nil }), nil +} + // discoverAndLoadIaCProvider implements the default resolveIaCProvider: it scans // the plugin directory for a plugin that declares iacProvider.name == providerName, // loads it via ExternalPluginManager, and returns the IaCProvider plus a Closer @@ -130,37 +156,25 @@ func discoverAndLoadIaCProvider(ctx context.Context, providerName string, cfg ma pluginDir = "./data/plugins" } - pluginName, hasBinary, err := findIaCPluginDir(pluginDir, providerName) + pluginName, factories, closer, err := loadIaCPlugin(pluginDir, providerName) if err != nil { - return nil, nil, fmt.Errorf("resolve IaC provider %q: %w", providerName, err) - } - if pluginName == "" { - return nil, nil, fmt.Errorf("no plugin found for IaC provider %q in %s — run: wfctl plugin install ", providerName, pluginDir) - } - if !hasBinary { - return nil, nil, fmt.Errorf("plugin %q declares provider %q but binary is missing — run: wfctl plugin install %s", pluginName, providerName, pluginName) - } - - mgr := external.NewExternalPluginManager(pluginDir, nil) - closer := closerFunc(func() error { mgr.Shutdown(); return nil }) - - adapter, loadErr := mgr.LoadPlugin(pluginName) - if loadErr != nil { - mgr.Shutdown() - return nil, nil, fmt.Errorf("load plugin %q for provider %q: %w", pluginName, providerName, loadErr) + return nil, nil, err } - factories := adapter.ModuleFactories() factory, ok := factories["iac.provider"] if !ok { - mgr.Shutdown() + _ = closer.Close() return nil, nil, fmt.Errorf("plugin %q does not expose an iac.provider module type — upgrade with: wfctl plugin update %s", pluginName, pluginName) } mod := factory("iac-provider", cfg) + if pluginErr := external.AsModuleError(mod); pluginErr != nil { + _ = closer.Close() + return nil, nil, fmt.Errorf("plugin %q iac.provider factory failed: %w", pluginName, pluginErr) + } if mod == nil { - mgr.Shutdown() - return nil, nil, fmt.Errorf("plugin %q iac.provider factory returned nil", pluginName) + _ = closer.Close() + return nil, nil, fmt.Errorf("plugin %q iac.provider factory returned nil (unexpected — file an issue)", pluginName) } // RemoteModule does not directly implement interfaces.IaCProvider; instead it @@ -168,7 +182,7 @@ func discoverAndLoadIaCProvider(ctx context.Context, providerName string, cfg ma // remoteIaCProvider that routes each IaCProvider call through InvokeService. invoker, ok := mod.(remoteServiceInvoker) if !ok { - mgr.Shutdown() + _ = closer.Close() return nil, nil, fmt.Errorf("plugin %q iac.provider module (%T) does not support service invocation — upgrade with: wfctl plugin update %s", pluginName, mod, pluginName) } @@ -176,7 +190,7 @@ func discoverAndLoadIaCProvider(ctx context.Context, providerName string, cfg ma // Notify the plugin that Initialize has been called (the plugin may treat // this as a no-op if it already ran Initialize inside CreateModule). if initErr := iacProvider.Initialize(ctx, cfg); initErr != nil { - mgr.Shutdown() + _ = closer.Close() return nil, nil, fmt.Errorf("initialize provider %q: %w", providerName, initErr) } return iacProvider, closer, nil diff --git a/cmd/wfctl/deploy_providers_test.go b/cmd/wfctl/deploy_providers_test.go index e60fa0be5..88f9aec06 100644 --- a/cmd/wfctl/deploy_providers_test.go +++ b/cmd/wfctl/deploy_providers_test.go @@ -2,13 +2,52 @@ package main import ( "context" + "errors" + "io" "os" "strings" "testing" + "github.com/GoCodeAlone/modular" "github.com/GoCodeAlone/workflow/config" + "github.com/GoCodeAlone/workflow/plugin" + "github.com/GoCodeAlone/workflow/plugin/external" ) +// noopCloser is an io.Closer that does nothing — used in test stubs. +type noopCloser struct{} + +func (noopCloser) Close() error { return nil } + +// ── discoverAndLoadIaCProvider — error propagation ──────────────────────────── + +// TestResolveIaCProviderSurfacesPluginError is the regression gate for the +// caller-side fix: discoverAndLoadIaCProvider must surface the error message +// from an *errorModule returned by the factory, not fall through to a generic +// "does not support service invocation" message. +func TestResolveIaCProviderSurfacesPluginError(t *testing.T) { + const pluginErrMsg = "digitalocean: missing required config key 'token'" + + oldLoader := loadIaCPlugin + loadIaCPlugin = func(_, _ string) (string, map[string]plugin.ModuleFactory, io.Closer, error) { + factory := func(name string, _ map[string]any) modular.Module { + return external.NewErrorModule(name, errors.New(pluginErrMsg)) + } + return "workflow-plugin-digitalocean", map[string]plugin.ModuleFactory{ + "iac.provider": factory, + }, noopCloser{}, nil + } + defer func() { loadIaCPlugin = oldLoader }() + + _, _, err := discoverAndLoadIaCProvider(context.Background(), "digitalocean", map[string]any{}) + if err == nil { + t.Fatal("expected error, got nil") + } + if !strings.Contains(err.Error(), pluginErrMsg) { + t.Errorf("expected plugin error message %q in returned error, got: %v", pluginErrMsg, err) + } +} + // ── newDeployProvider ───────────────────────────────────────────────────────── func TestNewDeployProvider_Kubernetes(t *testing.T) { diff --git a/plugin/external/adapter.go b/plugin/external/adapter.go index bddb20d03..cf037803c 100644 --- a/plugin/external/adapter.go +++ b/plugin/external/adapter.go @@ -61,6 +61,24 @@ func (m *errorModule) Init(modular.Application) error { return m.err } func (m *errorModule) Start(context.Context) error { return m.err } func (m *errorModule) Stop(context.Context) error { return nil } +// AsModuleError returns the wrapped error if m was produced by a failed +// CreateModule call (i.e. the factory returned an *errorModule), or nil if m +// is a successfully-created module. Callers outside this package use this to +// surface plugin-reported errors without depending on the unexported type. +func AsModuleError(m modular.Module) error { + if em, ok := m.(*errorModule); ok { + return em.err + } + return nil +} + +// NewErrorModule returns a Module that surfaces err from Init and Start. +// Exported for use in tests that need to simulate a plugin factory failure +// without importing the unexported errorModule type directly. +func NewErrorModule(name string, err error) modular.Module { + return &errorModule{name: name, err: err} +} + // NewExternalPluginAdapter creates an adapter from a connected plugin client. func NewExternalPluginAdapter(name string, client *PluginClient) (*ExternalPluginAdapter, error) { ctx := context.Background() @@ -326,8 +344,11 @@ func (a *ExternalPluginAdapter) ModuleFactories() map[string]plugin.ModuleFactor Config: config, TypedConfig: typedConfig, }) - if createErr != nil || createResp.Error != "" { - return nil + if createErr != nil { + return &errorModule{name: name, err: fmt.Errorf("create remote module %s: %w", tn, createErr)} + } + if createResp.Error != "" { + return &errorModule{name: name, err: fmt.Errorf("create remote module %s: plugin reported: %s", tn, createResp.Error)} } remote := NewRemoteModule(name, createResp.HandleId, a.client.client, remoteModuleContracts{ module: a.contracts.module(tn), diff --git a/plugin/external/adapter_test.go b/plugin/external/adapter_test.go index c2cc3d94d..480265e3b 100644 --- a/plugin/external/adapter_test.go +++ b/plugin/external/adapter_test.go @@ -212,6 +212,59 @@ func TestContractRegistry_UnimplementedUsesEmptyRegistry(t *testing.T) { } } +// errorOnCreateModuleClient overrides CreateModule to return a plugin-reported +// error in the response Error field (not as a gRPC error). +type errorOnCreateModuleClient struct { + adapterTestPluginServiceClient + createModuleError string +} + +func (c *errorOnCreateModuleClient) CreateModule(_ context.Context, req *pb.CreateModuleRequest, _ ...grpc.CallOption) (*pb.HandleResponse, error) { + c.lastCreateModReq = req + return &pb.HandleResponse{Error: c.createModuleError}, nil +} + +// TestModuleFactoriesPropagatesPluginError is a regression gate for the class +// invariant: when CreateModule returns a non-empty Error field, ModuleFactories +// must return an *errorModule wrapping the plugin's message — not bare nil. +// Previously the condition `if createErr != nil || createResp.Error != ""` fell +// through to `return nil`, silently discarding the plugin diagnostic. +func TestModuleFactoriesPropagatesPluginError(t *testing.T) { + const pluginErrMsg = "digitalocean: missing required config key 'token'" + client := &errorOnCreateModuleClient{ + adapterTestPluginServiceClient: adapterTestPluginServiceClient{ + manifest: &pb.Manifest{Name: "test-plugin"}, + moduleTypes: []string{"iac.provider"}, + }, + createModuleError: pluginErrMsg, + } + a, err := NewExternalPluginAdapter("test-plugin", &PluginClient{client: client}) + if err != nil { + t.Fatalf("NewExternalPluginAdapter: %v", err) + } + + factories := a.ModuleFactories() + factory, ok := factories["iac.provider"] + if !ok { + t.Fatal("expected iac.provider factory to be registered") + } + + mod := factory("test-provider", map[string]any{}) + if mod == nil { + t.Fatal("expected *errorModule, got nil — plugin error was swallowed") + } + errMod, ok := mod.(*errorModule) + if !ok { + t.Fatalf("expected *errorModule, got %T", mod) + } + if errMod.err == nil { + t.Fatal("errorModule has nil err") + } + if !strings.Contains(errMod.err.Error(), pluginErrMsg) { + t.Errorf("expected plugin error message %q in propagated error, got: %v", pluginErrMsg, errMod.err) + } +} + func TestExternalPluginAdapter_ServiceContractsAttachByModuleType(t *testing.T) { registry := &pb.ContractRegistry{ Contracts: []*pb.ContractDescriptor{