From 3e1725f9acbda92bdcb9fb2dfa86d1346a4275cf Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Thu, 14 May 2026 01:27:24 -0400 Subject: [PATCH] fix(wfctl): handle plugin flags and adoption secrets Accept plugin install flags after positional plugin names. Route sensitive outputs discovered during resource adoption through the configured secrets provider so infra_output can consume them in the same apply. --- cmd/wfctl/infra_apply.go | 73 ++++++++++++++++++++---- cmd/wfctl/infra_apply_test.go | 95 +++++++++++++++++++++++++++++++- cmd/wfctl/plugin_install.go | 57 +++++++++++++++++-- cmd/wfctl/plugin_install_test.go | 61 +++++++++++++++++++- 4 files changed, 268 insertions(+), 18 deletions(-) diff --git a/cmd/wfctl/infra_apply.go b/cmd/wfctl/infra_apply.go index e1243c0c..aefef0aa 100644 --- a/cmd/wfctl/infra_apply.go +++ b/cmd/wfctl/infra_apply.go @@ -410,7 +410,7 @@ func applyWithProviderAndStore(ctx context.Context, provider interfaces.IaCProvi // group; applyInfraModules does that before invoking this helper. var err error - current, err = adoptExistingResources(ctx, provider, providerType, specs, current, store) + current, err = adoptExistingResources(ctx, provider, providerType, specs, current, store, secretsProvider, hydratedOut) if err != nil { return err } @@ -768,7 +768,7 @@ func normalizeAppliedOutputIdentity(spec interfaces.ResourceSpec, out interfaces return out, nil } -func adoptExistingResources(ctx context.Context, provider interfaces.IaCProvider, providerType string, specs []interfaces.ResourceSpec, current []interfaces.ResourceState, store infraStateStore) ([]interfaces.ResourceState, error) { +func adoptExistingResources(ctx context.Context, provider interfaces.IaCProvider, providerType string, specs []interfaces.ResourceSpec, current []interfaces.ResourceState, store infraStateStore, secretsProvider secrets.Provider, hydratedOut map[string]string) ([]interfaces.ResourceState, error) { if len(specs) == 0 { return current, nil } @@ -842,15 +842,25 @@ func adoptExistingResources(ctx context.Context, provider interfaces.IaCProvider if err := validateStateProviderID(provider, providerType, state); err != nil { return nil, err } - // Sanitize-only via persistResourceWithSecretRouting in read mode: - // drivers may declare Sensitive on Read but we MUST NOT call - // provider.Set from a Read path (cache-pollution risk per design §4.4). - // Provider passed nil; helper is nil-safe in read mode. - // Cached-prior variant: avoids per-resource ListResources scans - // when adoption pass touches many resources. - if _, err := persistResourceWithSecretRoutingCachedPrior(ctx, store, nil, driver, state, *live, persistModeRead, priorByName); err != nil { + mode := persistModeRead + routeProvider := secrets.Provider(nil) + if secretsProvider != nil && hasSensitiveOutputs(live) { + // Adoption is a live cloud read, but when a secrets provider is + // configured the same apply process must route newly discovered + // sensitive outputs. Otherwise infra_output generators cannot + // consume write-only stores like GitHub Actions secrets. + mode = persistModeAdoptRoute + routeProvider = secretsProvider + } + hydrated, err := persistResourceWithSecretRoutingCachedPrior(ctx, store, routeProvider, driver, state, *live, mode, priorByName) + if err != nil { return nil, fmt.Errorf("%s/%s: persist adopted state: %w", spec.Type, spec.Name, err) } + if hydratedOut != nil { + for k, v := range hydrated { + hydratedOut[k] = v + } + } fmt.Printf(" Adopted existing %s %q (id=%s)\n", spec.Type, spec.Name, state.ProviderID) current = append(current, state) currentByName[spec.Name] = struct{}{} @@ -988,6 +998,7 @@ const ( persistModeApply persistMode = iota persistModeRead persistModeApplyNoCompensate + persistModeAdoptRoute ) // persistResourceWithSecretRouting builds rs.Outputs from out (routing @@ -1024,6 +1035,8 @@ func persistResourceWithSecretRouting( return persistApplyMode(ctx, store, provider, driver, rs, out, true) case persistModeApplyNoCompensate: return persistApplyMode(ctx, store, provider, driver, rs, out, false) + case persistModeAdoptRoute: + return persistAdoptRouteMode(ctx, store, provider, rs, out) case persistModeRead: return nil, persistReadMode(ctx, store, rs, out, nil) default: @@ -1054,6 +1067,8 @@ func persistResourceWithSecretRoutingCachedPrior( return persistApplyMode(ctx, store, provider, driver, rs, out, true) case persistModeApplyNoCompensate: return persistApplyMode(ctx, store, provider, driver, rs, out, false) + case persistModeAdoptRoute: + return persistAdoptRouteMode(ctx, store, provider, rs, out) case persistModeRead: return nil, persistReadMode(ctx, store, rs, out, priorByName) default: @@ -1098,6 +1113,30 @@ func persistApplyMode( return hydrated, nil } +func persistAdoptRouteMode( + ctx context.Context, + store infraStateStore, + provider secrets.Provider, + rs interfaces.ResourceState, + out interfaces.ResourceOutput, +) (map[string]string, error) { + sanitized, hydrated, err := sensitive.Route(ctx, provider, rs.Name, &out) + if err != nil { + if compErr := cleanupRoutedSecrets(provider, hydrated); compErr != nil { + return nil, fmt.Errorf("%s/%s: route sensitive outputs: %w (routed-secret cleanup failed: %v)", rs.Type, rs.Name, err, compErr) + } + return nil, fmt.Errorf("%s/%s: route sensitive outputs: %w", rs.Type, rs.Name, err) + } + rs.Outputs = sanitized + if saveErr := store.SaveResource(ctx, rs); saveErr != nil { + if compErr := cleanupRoutedSecrets(provider, hydrated); compErr != nil { + return nil, fmt.Errorf("%s/%s: persist adopted state: %w (routed-secret cleanup failed: %v)", rs.Type, rs.Name, saveErr, compErr) + } + return nil, fmt.Errorf("%s/%s: persist adopted state: %w", rs.Type, rs.Name, saveErr) + } + return hydrated, nil +} + func persistReadMode( ctx context.Context, store infraStateStore, @@ -1151,6 +1190,21 @@ func persistReadMode( return nil } +func cleanupRoutedSecrets(provider secrets.Provider, hydrated map[string]string) error { + if provider == nil || len(hydrated) == 0 { + return nil + } + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + var errs []error + for secretName := range hydrated { + if delErr := provider.Delete(ctx, secretName); delErr != nil && !errors.Is(delErr, secrets.ErrNotFound) { + errs = append(errs, fmt.Errorf("provider.Delete(%s): %w", secretName, delErr)) + } + } + return errors.Join(errs...) +} + // compensateAfterSaveFailure rolls back routed secrets and the underlying // cloud resource after an apply-mode failure where the just-mutated resource is // known to be newly created or replacement-created. Uses a fresh 30-second @@ -1182,7 +1236,6 @@ func compensateAfterSaveFailure( } } if provider != nil { - // Delete each routed secret by its full provider key. for secretName := range hydrated { if delErr := provider.Delete(ctx, secretName); delErr != nil && !errors.Is(delErr, secrets.ErrNotFound) { errs = append(errs, fmt.Errorf("provider.Delete(%s): %w", secretName, delErr)) diff --git a/cmd/wfctl/infra_apply_test.go b/cmd/wfctl/infra_apply_test.go index c8e29b69..6842a286 100644 --- a/cmd/wfctl/infra_apply_test.go +++ b/cmd/wfctl/infra_apply_test.go @@ -12,6 +12,7 @@ import ( "sync" "testing" + "github.com/GoCodeAlone/workflow/iac/sensitive" "github.com/GoCodeAlone/workflow/interfaces" ) @@ -70,6 +71,7 @@ type readDriver struct { readOut *interfaces.ResourceOutput readErr error reads []interfaces.ResourceRef + deletes []interfaces.ResourceRef expectedProviderID string format interfaces.ProviderIDFormat } @@ -90,7 +92,10 @@ func (d *readDriver) Update(_ context.Context, ref interfaces.ResourceRef, spec return &interfaces.ResourceOutput{Name: spec.Name, Type: spec.Type, ProviderID: ref.ProviderID}, nil } -func (d *readDriver) Delete(_ context.Context, _ interfaces.ResourceRef) error { return nil } +func (d *readDriver) Delete(_ context.Context, ref interfaces.ResourceRef) error { + d.deletes = append(d.deletes, ref) + return nil +} func (d *readDriver) Diff(_ context.Context, _ interfaces.ResourceSpec, _ *interfaces.ResourceOutput) (*interfaces.DiffResult, error) { // W-3b ComputePlan dispatches Diff per resource. The adoption tests @@ -804,6 +809,94 @@ func TestApplyWithProvider_AdoptsExistingDNSBeforeComputePlan(t *testing.T) { } } +func TestApplyWithProvider_AdoptionRoutesNewSensitiveOutputs(t *testing.T) { + const rawURI = "postgres://doadmin:secret@example.com:25060/defaultdb" + spec := interfaces.ResourceSpec{ + Name: "adopted-db", + Type: "infra.database", + Config: map[string]any{"adopt_existing": true}, + } + driver := &readDriver{ + readOut: &interfaces.ResourceOutput{ + Name: "adopted-db", + Type: "infra.database", + ProviderID: "db-123", + Outputs: map[string]any{ + "uri": rawURI, + "config": map[string]any{ + "engine": "pg", + }, + }, + Sensitive: map[string]bool{"uri": true}, + }, + } + provider := &readBackedProvider{driver: driver} + store := &fakeStateStore{} + cfgPath := filepath.Join(t.TempDir(), "workflow.yaml") + if err := os.WriteFile(cfgPath, []byte("secrets:\n provider: env\n"), 0o600); err != nil { + t.Fatalf("write config: %v", err) + } + secretName := sensitive.SecretKey("adopted-db", "uri") + t.Setenv(secretName, "") + hydrated := map[string]string{} + origCompute := computeInfraPlan + computeInfraPlan = func(context.Context, interfaces.IaCProvider, []interfaces.ResourceSpec, []interfaces.ResourceState) (interfaces.IaCPlan, error) { + return interfaces.IaCPlan{}, nil + } + t.Cleanup(func() { computeInfraPlan = origCompute }) + + if err := applyWithProviderAndStore(t.Context(), provider, "digitalocean", []interfaces.ResourceSpec{spec}, nil, store, io.Discard, "", cfgPath, hydrated); err != nil { + t.Fatalf("applyWithProviderAndStore: %v", err) + } + + store.mu.Lock() + defer store.mu.Unlock() + if len(store.saved) != 1 { + t.Fatalf("saved states = %d, want adopted state", len(store.saved)) + } + if got := store.saved[0].Outputs["uri"]; got != sensitive.Placeholder("adopted-db", "uri") { + t.Fatalf("adopted uri output = %#v, want routed placeholder", got) + } + if got := os.Getenv(strings.ToUpper(secretName)); got != rawURI { + t.Fatalf("routed env secret = %q, want raw URI", got) + } + if got := hydrated[secretName]; got != rawURI { + t.Fatalf("hydrated handoff = %q, want raw URI", got) + } +} + +func TestAdoptExistingResources_AdoptionRoutingSaveFailureCleansSecretsOnly(t *testing.T) { + const rawURI = "postgres://doadmin:secret@example.com:25060/defaultdb" + spec := interfaces.ResourceSpec{ + Name: "adopted-db", + Type: "infra.database", + Config: map[string]any{"adopt_existing": true}, + } + driver := &readDriver{ + readOut: &interfaces.ResourceOutput{ + Name: "adopted-db", + Type: "infra.database", + ProviderID: "db-123", + Outputs: map[string]any{"uri": rawURI}, + Sensitive: map[string]bool{"uri": true}, + }, + } + provider := &readBackedProvider{driver: driver} + store := &fakeStateStore{saveErr: errors.New("state unavailable")} + secretsProvider := newEnvTestProvider() + + _, err := adoptExistingResources(t.Context(), provider, "digitalocean", []interfaces.ResourceSpec{spec}, nil, store, secretsProvider, map[string]string{}) + if err == nil { + t.Fatalf("adoptExistingResources succeeded, want state persistence error") + } + if len(secretsProvider.values) != 0 { + t.Fatalf("routed secrets after failed adoption = %#v, want cleanup", secretsProvider.values) + } + if len(driver.deletes) != 0 { + t.Fatalf("driver deletes = %#v, want no cloud resource delete for adoption", driver.deletes) + } +} + func TestApplyWithProvider_DNSAdoptionFailedUpdateKeepsLiveAppliedConfig(t *testing.T) { desiredConfig := map[string]any{ "provider": "do-provider", diff --git a/cmd/wfctl/plugin_install.go b/cmd/wfctl/plugin_install.go index b16a3683..b7db65ed 100644 --- a/cmd/wfctl/plugin_install.go +++ b/cmd/wfctl/plugin_install.go @@ -87,7 +87,11 @@ func runPluginInstall(args []string) error { fmt.Fprintf(fs.Output(), "Usage: wfctl plugin install [options] [[@]]\n\nInstall a plugin from the registry, a URL, a local directory, or from the lockfile.\n\n wfctl plugin install Install latest from registry\n wfctl plugin install @v1.0.0 Install specific version\n wfctl plugin install --url Install from a direct download URL\n wfctl plugin install --local Install from a local build directory\n wfctl plugin install --from-config Install all requires.plugins[] from workflow config\n wfctl plugin install Install all plugins from .wfctl-lock.yaml\n\nOptions:\n") fs.PrintDefaults() } - if err := fs.Parse(args); err != nil { + parsedArgs, err := interspersedPluginInstallArgs(fs, args) + if err != nil { + return err + } + if err := fs.Parse(parsedArgs); err != nil { return err } // Validate flag combinations before doing anything else. @@ -251,6 +255,49 @@ func runPluginInstall(args []string) error { return nil } +type boolFlag interface { + IsBoolFlag() bool +} + +func interspersedPluginInstallArgs(fs *flag.FlagSet, args []string) ([]string, error) { + if len(args) == 0 { + return args, nil + } + flags := make([]string, 0, len(args)) + positionals := make([]string, 0, len(args)) + for i := 0; i < len(args); i++ { + arg := args[i] + if arg == "--" { + positionals = append(positionals, args[i:]...) + break + } + if !strings.HasPrefix(arg, "-") || arg == "-" { + positionals = append(positionals, arg) + continue + } + flags = append(flags, arg) + name := strings.TrimLeft(arg, "-") + if idx := strings.IndexByte(name, '='); idx >= 0 { + name = name[:idx] + } + f := fs.Lookup(name) + if f == nil || strings.Contains(arg, "=") { + continue + } + if bf, ok := f.Value.(boolFlag); ok && bf.IsBoolFlag() { + continue + } + remaining := args[i+1:] + if len(remaining) == 0 { + return nil, fmt.Errorf("flag needs an argument: -%s", name) + } + value := remaining[0] + i++ + flags = append(flags, value) + } + return append(flags, positionals...), nil +} + // installPluginFromManifest downloads, extracts, and installs a plugin using the // provided registry manifest. It is shared by runPluginInstall and runPluginUpdate. // The plugin.json is always written/updated from the manifest to keep version tracking correct. @@ -1431,10 +1478,10 @@ func preparePluginStagingDir(destDir string) (stagingDir string, cleanup func(), // renamed to a trash location on the same filesystem. Only after the new // directory is successfully renamed into place is the trash removed. // -// 1. Rename destDir → destDir+".uninstalling" (no-op if destDir absent) -// 2. Rename stagingDir → destDir -// 3. On step-2 failure: restore destDir+".uninstalling" → destDir -// 4. On step-2 success: remove destDir+".uninstalling" +// 1. Rename destDir → destDir+".uninstalling" (no-op if destDir absent) +// 2. Rename stagingDir → destDir +// 3. On step-2 failure: restore destDir+".uninstalling" → destDir +// 4. On step-2 success: remove destDir+".uninstalling" // // On success stagingDir no longer exists on disk; the deferred cleanup // returned by preparePluginStagingDir becomes a harmless no-op. diff --git a/cmd/wfctl/plugin_install_test.go b/cmd/wfctl/plugin_install_test.go index 16e686da..8a4cd1ff 100644 --- a/cmd/wfctl/plugin_install_test.go +++ b/cmd/wfctl/plugin_install_test.go @@ -7,6 +7,7 @@ import ( "crypto/sha256" "encoding/hex" "encoding/json" + "flag" "fmt" "net/http" "net/http/httptest" @@ -749,8 +750,6 @@ func sha256sum(data []byte) string { return hex.EncodeToString(h[:]) } - - func TestVerifyChecksum_MismatchFormat(t *testing.T) { err := verifyChecksum([]byte("data"), strings.Repeat("0", 64)) if err == nil { @@ -976,6 +975,64 @@ func TestRunPluginInstallCompatSkipsNewerKnownFail(t *testing.T) { } } +func TestRunPluginInstallHonorsTrailingPluginDirFlag(t *testing.T) { + reg := newCompatInstallRegistry(t, "test", "v0.2.0", []compatInstallVersion{ + {Version: "v0.2.0", Status: PluginCompatibilityStatusPass}, + }) + pluginDir := t.TempDir() + if err := runPluginInstall([]string{ + "test", + "--config", reg.ConfigPath, + "--plugin-dir", pluginDir, + "--engine-version", "v0.51.2", + }); err != nil { + t.Fatalf("runPluginInstall: %v", err) + } + if got := readInstalledVersion(filepath.Join(pluginDir, "test")); got != "v0.2.0" { + t.Fatalf("installed version in trailing --plugin-dir = %q, want v0.2.0", got) + } +} + +func TestRunPluginInstallTrailingFlagMissingValueErrors(t *testing.T) { + err := runPluginInstall([]string{"test", "--config"}) + if err == nil { + t.Fatal("expected missing trailing --config value to error") + } + if !strings.Contains(err.Error(), "flag needs an argument") || !strings.Contains(err.Error(), "config") { + t.Fatalf("error = %v, want missing --config value", err) + } +} + +func TestInterspersedPluginInstallArgsReordersSupportedForms(t *testing.T) { + fs := flag.NewFlagSet("plugin install", flag.ContinueOnError) + fs.String("config", "", "") + fs.String("plugin-dir", "", "") + fs.Bool("skip-checksum", false, "") + + got, err := interspersedPluginInstallArgs(fs, []string{ + "test", + "--config=registry.yaml", + "--skip-checksum", + "--plugin-dir", "plugins", + "--", + "--not-a-flag", + }) + if err != nil { + t.Fatalf("interspersedPluginInstallArgs: %v", err) + } + want := []string{ + "--config=registry.yaml", + "--skip-checksum", + "--plugin-dir", "plugins", + "test", + "--", + "--not-a-flag", + } + if strings.Join(got, "\x00") != strings.Join(want, "\x00") { + t.Fatalf("args = %#v, want %#v", got, want) + } +} + func TestRunPluginInstallCompatRequestedFailErrorsAndWarnPermits(t *testing.T) { reg := newCompatInstallRegistry(t, "test", "v0.2.0", []compatInstallVersion{ {Version: "v0.2.0", Status: PluginCompatibilityStatusFail},