From 5104ed975eb04e5f78b56593e47dd9677fadb203 Mon Sep 17 00:00:00 2001 From: maya-ots Date: Thu, 22 Oct 2026 08:46:24 +0100 Subject: [PATCH 1/5] feat(plugin): support required plugin failures --- internal/plugin/config.go | 11 ++++--- internal/plugin/manager.go | 36 ++++++++++++++++++--- internal/plugin/plugin_test.go | 41 ++++++++++++++++++++++-- internal/schemas/mctf-config.schema.json | 5 +++ schema/mctf-config.schema.json | 5 +++ 5 files changed, 86 insertions(+), 12 deletions(-) diff --git a/internal/plugin/config.go b/internal/plugin/config.go index 52f2bcc..3624689 100644 --- a/internal/plugin/config.go +++ b/internal/plugin/config.go @@ -7,11 +7,12 @@ import ( // Config is one plugin entry from mctf.config.yml `plugins:`. type Config struct { - Name string `json:"name"` - Command string `json:"command"` - Args []string `json:"args,omitempty"` - Enabled *bool `json:"enabled,omitempty"` // nil => enabled - Config map[string]any `json:"config,omitempty"` // opaque, passed to the plugin + Name string `json:"name"` + Command string `json:"command"` + Args []string `json:"args,omitempty"` + Enabled *bool `json:"enabled,omitempty"` // nil => enabled + Required bool `json:"required,omitempty"` + Config map[string]any `json:"config,omitempty"` // opaque, passed to the plugin } func (c Config) isEnabled() bool { return c.Enabled == nil || *c.Enabled } diff --git a/internal/plugin/manager.go b/internal/plugin/manager.go index 24614da..3459ca6 100644 --- a/internal/plugin/manager.go +++ b/internal/plugin/manager.go @@ -30,6 +30,7 @@ type Manager struct { opts Options log io.Writer procCancel context.CancelFunc // cancels every plugin's process on Close + loadErr error } // Load parses raw plugin config, starts each enabled plugin, and runs the @@ -69,6 +70,9 @@ func Load(ctx context.Context, raw []map[string]any, opts Options) *Manager { } if err := p.start(procCtx, opts.Dir); err != nil { fmt.Fprintf(log, "warning: plugin %q failed to start: %v\n", c.Name, err) + if c.Required && m.loadErr == nil { + m.loadErr = fmt.Errorf("required plugin %q failed to start: %w", c.Name, err) + } continue } hctx, cancel := context.WithTimeout(ctx, opts.StartTimeout) @@ -77,11 +81,14 @@ func Load(ctx context.Context, raw []map[string]any, opts Options) *Manager { if err != nil { fmt.Fprintf(log, "warning: plugin %q handshake failed: %v\n", c.Name, err) p.markDeadAndKill() + if c.Required && m.loadErr == nil { + m.loadErr = fmt.Errorf("required plugin %q handshake failed: %w", c.Name, err) + } continue } m.plugins = append(m.plugins, p) } - if len(m.plugins) == 0 { + if len(m.plugins) == 0 && m.loadErr == nil { procCancel() return nil } @@ -91,22 +98,43 @@ func Load(ctx context.Context, raw []map[string]any, opts Options) *Manager { // Dispatch sends a hook to every subscribing plugin, sequentially, each under // its own DispatchTimeout. A plugin that errors, crashes, or times out is // logged and dropped from future dispatch. It never returns an error. -func (m *Manager) Dispatch(ctx context.Context, hook string, payload any) { +// Dispatch sends a hook to every subscribing plugin, sequentially, each under +// its own DispatchTimeout. Optional plugin failures are logged and dropped. +// Failures from required plugins are also returned so blocking hooks can veto +// the host operation. +func (m *Manager) Dispatch(ctx context.Context, hook string, payload any) error { if m == nil { - return + return nil } + + if m.loadErr != nil { + return m.loadErr + } + + var requiredErr error live := m.plugins[:0] + for _, p := range m.plugins { dctx, cancel := context.WithTimeout(ctx, m.opts.DispatchTimeout) err := p.dispatch(dctx, hook, payload) cancel() + if err != nil { fmt.Fprintf(m.log, "warning: plugin %q hook %q: %v\n", p.name, hook, err) - continue // drop it + if p.cfg.Required && requiredErr == nil { + requiredErr = fmt.Errorf( + "required plugin %q failed on hook %q: %w", + p.name, hook, err, + ) + } + continue } + live = append(live, p) } + m.plugins = live + return requiredErr } // Close shuts every plugin down and reaps it. Safe to call on a nil Manager. diff --git a/internal/plugin/plugin_test.go b/internal/plugin/plugin_test.go index 7688015..04c349f 100644 --- a/internal/plugin/plugin_test.go +++ b/internal/plugin/plugin_test.go @@ -63,6 +63,20 @@ func TestHelperProcess(t *testing.T) { os.Exit(1) case "timeout": time.Sleep(30 * time.Second) + case "veto": + if req.Method == HookBeforeDeploy { + fmt.Fprintf(os.Stdout, + "{\"jsonrpc\":\"2.0\",\"id\":%d,\"error\":{\"code\":-32000,\"message\":\"deployment rejected by policy\"}}\n", + req.ID, + ) + continue + } + if outfile != "" { + f, _ := os.OpenFile(outfile, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o644) + fmt.Fprintf(f, "%s %s\n", req.Method, strings.TrimSpace(string(req.Params))) + _ = f.Close() + } + reply(req.ID, map[string]any{"ok": true}) default: if outfile != "" { f, _ := os.OpenFile(outfile, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o644) @@ -91,9 +105,10 @@ func helperManager(t *testing.T, mode, outfile string) *Manager { return cmd } raw := []map[string]any{{ - "name": "helper", - "command": os.Args[0], - "args": []any{"-test.run=TestHelperProcess"}, + "name": "helper", + "command": os.Args[0], + "args": []any{"-test.run=TestHelperProcess"}, + "required": mode == "veto", }} return Load(context.Background(), raw, Options{ HostVersion: "test", @@ -287,3 +302,23 @@ func TestManager_NilIsSafe(t *testing.T) { m.Dispatch(context.Background(), HookBeforeDeploy, nil) // must not panic m.Close() } + +func TestManager_RequiredPluginCanVetoDeploy(t *testing.T) { + m := helperManager(t, "veto", "") + if m == nil { + t.Fatal("expected manager for required plugin") + } + defer m.Close() + + err := m.Dispatch( + context.Background(), + HookBeforeDeploy, + DeployPayload{Phase: "before"}, + ) + if err == nil { + t.Fatal("expected required plugin to veto deployment") + } + if !strings.Contains(err.Error(), "deployment rejected by policy") { + t.Fatalf("unexpected error: %v", err) + } +} diff --git a/internal/schemas/mctf-config.schema.json b/internal/schemas/mctf-config.schema.json index bf22126..23b40c2 100644 --- a/internal/schemas/mctf-config.schema.json +++ b/internal/schemas/mctf-config.schema.json @@ -239,6 +239,11 @@ "command": { "type": "string", "minLength": 1 }, "args": { "type": "array", "items": { "type": "string" } }, "enabled": { "type": "boolean", "default": true }, + "required": { + "type": "boolean", + "default": false, + "description": "If true, failures in required plugin startup or blocking hooks can abort the command." + }, "config": { "type": "object" } } } diff --git a/schema/mctf-config.schema.json b/schema/mctf-config.schema.json index bf22126..23b40c2 100644 --- a/schema/mctf-config.schema.json +++ b/schema/mctf-config.schema.json @@ -239,6 +239,11 @@ "command": { "type": "string", "minLength": 1 }, "args": { "type": "array", "items": { "type": "string" } }, "enabled": { "type": "boolean", "default": true }, + "required": { + "type": "boolean", + "default": false, + "description": "If true, failures in required plugin startup or blocking hooks can abort the command." + }, "config": { "type": "object" } } } From 8277602c613b76d55eda4870b185bd5eef6debc8 Mon Sep 17 00:00:00 2001 From: maya-ots Date: Fri, 23 Oct 2026 00:52:23 +0100 Subject: [PATCH 2/5] feat(plugin): dispatch teardown and Kubernetes hooks --- cmd/deploy.go | 112 ++++++++++++++++++++++++++++----- docs/spec/plugins.md | 47 +++++++++----- internal/plugin/plugin_test.go | 42 ++++++++++++- internal/plugin/protocol.go | 4 +- 4 files changed, 172 insertions(+), 33 deletions(-) diff --git a/cmd/deploy.go b/cmd/deploy.go index 95d75d4..6d67ba2 100644 --- a/cmd/deploy.go +++ b/cmd/deploy.go @@ -188,7 +188,10 @@ var deployCmd = &cobra.Command{ // Lifecycle plugins (best-effort; a no-op without a `plugins:` block). pm := plugin.Load(ctx, cfg.Plugins, plugin.Options{HostVersion: version, Dir: flagDir}) defer pm.Close() - pm.Dispatch(ctx, plugin.HookBeforeDeploy, deployHookPayload(cfg, mode, "before", flagDeployDryRun, plan, "", nil)) + if err := pm.Dispatch(ctx, plugin.HookBeforeDeploy, + deployHookPayload(cfg, mode, "before", flagDeployDryRun, plan, "", nil)); err != nil { + return fmt.Errorf("deployment blocked by plugin: %w", err) + } if err := deploy.ApplyUp(ctx, runner, plan); err != nil { pm.Dispatch(ctx, plugin.HookAfterDeploy, deployHookPayload(cfg, mode, "after", flagDeployDryRun, plan, "failed", err)) @@ -261,6 +264,32 @@ func deployHookPayload(cfg *config.Config, mode, phase string, dryRun bool, plan return p } +func kubeDeployHookPayload( + cfg *config.Config, + phase string, + dryRun bool, + plan []kubernetes.KubeUnit, + outcome string, + err error, +) plugin.DeployPayload { + p := plugin.DeployPayload{ + Event: cfg.Event.Slug, + Phase: phase, + Mode: "kubernetes", + DryRun: dryRun, + Outcome: outcome, + } + if err != nil { + p.Error = err.Error() + } + for _, u := range plan { + p.Units = append(p.Units, plugin.DeployUnit{ + Challenge: u.Challenge, + }) + } + return p +} + func loadPlanAndRunner(dir, sha string, dryRun bool) ([]deploy.Unit, deploy.Runner, *config.Config, error) { cfg, err := config.Load(dir) if err != nil { @@ -368,11 +397,29 @@ var downCmd = &cobra.Command{ } } } + pm := plugin.Load(ctx, cfg.Plugins, plugin.Options{ + HostVersion: version, + Dir: flagDir, + }) + defer pm.Close() + + if err := pm.Dispatch(ctx, plugin.HookBeforeDown, + deployHookPayload(cfg, mode, "before", flagDownDryRun, plan, "", nil)); err != nil { + return fmt.Errorf("teardown blocked by plugin: %w", err) + } + if err := deploy.ApplyDown(ctx, runner, plan, flagDownVolumes); err != nil { + pm.Dispatch(ctx, plugin.HookAfterDown, + deployHookPayload(cfg, mode, "after", flagDownDryRun, plan, "failed", err)) notifyDiscord(ctx, "teardown failed", "error", fmt.Sprintf("%s: %v", planSlugs(plan), err)) return err } + + if !flagDownDryRun { + pm.Dispatch(ctx, plugin.HookAfterDown, + deployHookPayload(cfg, mode, "after", false, plan, "success", nil)) + } fmt.Println("\ndown complete") if !flagDownDryRun { notifyDiscord(ctx, "teardown complete", "info", @@ -470,6 +517,26 @@ func applyKube(ctx context.Context, cfg *config.Config, challs []*challenge.Chal } printKubePlan(verb, plan) + pm := plugin.Load(ctx, cfg.Plugins, plugin.Options{ + HostVersion: version, + Dir: flagDir, + }) + defer pm.Close() + + beforeHook := plugin.HookBeforeDeploy + afterHook := plugin.HookAfterDeploy + operation := "deployment" + + if verb == "down" { + beforeHook = plugin.HookBeforeDown + afterHook = plugin.HookAfterDown + operation = "teardown" + } + + if err := pm.Dispatch(ctx, beforeHook, + kubeDeployHookPayload(cfg, "before", dry, plan, "", nil)); err != nil { + return fmt.Errorf("%s blocked by plugin: %w", operation, err) + } kubeconfig := "" if cfg.Deploy.Kubernetes != nil { @@ -482,27 +549,44 @@ func applyKube(ctx context.Context, cfg *config.Config, challs []*challenge.Chal runner = kubernetes.NewKubeRunner(kubeconfig) } + var applyErr error if verb == "up" { - if err := kubernetes.ApplyUp(ctx, runner, plan); err != nil { - notifyDiscord(ctx, "deploy failed", "error", err.Error()) - return err + applyErr = kubernetes.ApplyUp(ctx, runner, plan) + } else { + applyErr = kubernetes.ApplyDown(ctx, runner, plan) + } + + if applyErr != nil { + pm.Dispatch(ctx, afterHook, + kubeDeployHookPayload(cfg, "after", dry, plan, "failed", applyErr)) + + if verb == "up" { + notifyDiscord(ctx, "deploy failed", "error", applyErr.Error()) + } else { + notifyDiscord(ctx, "teardown failed", "error", applyErr.Error()) } - fmt.Println("\ndeploy complete") - if !dry { + return applyErr + } + + if !dry { + pm.Dispatch(ctx, afterHook, + kubeDeployHookPayload(cfg, "after", false, plan, "success", nil)) + + if verb == "up" { + fmt.Println("\ndeploy complete") notifyDiscord(ctx, "deploy succeeded", "success", fmt.Sprintf("%d challenge(s) up [kubernetes]", len(plan))) - } - } else { - if err := kubernetes.ApplyDown(ctx, runner, plan); err != nil { - notifyDiscord(ctx, "teardown failed", "error", err.Error()) - return err - } - fmt.Println("\ndown complete") - if !dry { + } else { + fmt.Println("\ndown complete") notifyDiscord(ctx, "teardown complete", "info", fmt.Sprintf("%d challenge(s) down [kubernetes]", len(plan))) } + } else if verb == "up" { + fmt.Println("\ndeploy complete") + } else { + fmt.Println("\ndown complete") } + return nil } diff --git a/docs/spec/plugins.md b/docs/spec/plugins.md index 0df9a43..9f1c704 100644 --- a/docs/spec/plugins.md +++ b/docs/spec/plugins.md @@ -5,8 +5,7 @@ can run arbitrary logic (notifications, policy checks, custom provisioning, …) without being compiled into the CLI. They are language-agnostic: any executable that speaks the protocol below works. -> **Status:** foundation + the `before_deploy` / `after_deploy` hooks ship now. -> The remaining hooks are planned — see [Hook roster](#hook-roster). +> **Status:** The plugin foundation, deploy hooks (before_deploy / after_deploy), teardown hooks (before_down / after_down), build hooks, and instance-spawn hook are implemented. Required plugins can block commands when a required plugin fails during startup or a blocking before_* hook. ## Trust model @@ -24,6 +23,7 @@ plugins: command: ./plugins/logger.sh # executable; relative paths resolve to the event root args: ["--verbose"] # optional enabled: true # optional (default true) + required: false # optional; defaults to false config: # optional, opaque — passed to the plugin at initialize file: ./audit.log ``` @@ -64,23 +64,39 @@ name and whose `params` is the hook payload; the plugin replies with any result The host sends `{"method":"shutdown"}` and closes stdin; the plugin exits. -## Semantics: best-effort & crash-isolated +## Semantics: best-effort and required plugins + +Plugins are best-effort by default. A plugin with `required: false` (the default) +that fails to start, fails its handshake, returns an error, crashes, or times out +is logged and dropped; the command continues. + +Setting `required: true` opts a plugin into blocking behavior: + +- If a required plugin fails to start or complete its initialization handshake, + the command is blocked when the plugin manager reports that failure. +- If a required plugin returns an error or fails during a blocking `before_*` + hook, the command is aborted before the corresponding operation runs. +- Failures in `after_*` hooks do not undo an operation that has already completed. +- Non-required plugins remain best-effort, and their failures do not block the + command. + +Each hook call is bounded by a timeout. A hung plugin is terminated. Only +opt-in required-plugin failures can block commands; ordinary plugin failures +remain isolated. -Plugins never break an MCTF command. A plugin that fails to start, botches the -handshake, errors, crashes, or hangs is **logged to stderr and dropped** — the -command continues. Each hook call is bounded by a timeout; a hung plugin is -killed. (This mirrors the `notify` package.) ## Hook roster | Hook | Payload | Status | | --- | --- | --- | -| `before_deploy` | deploy payload (event, mode, units, dry_run) | **shipped** | -| `after_deploy` | deploy payload + `outcome` (`success`/`failed`) + `error` | **shipped** | -| `before_build` | build payload (event, sha, dry_run, images) | **shipped** | -| `after_build` | build payload + `outcome` (`success`/`failed`) + `error` | **shipped** | -| `on_instance_spawn` | instance spawn payload (event, instance_id, challenge_slug, team_id, ports, flag, containers, expires_at) | **shipped** | -| `on_health_failure` | — | planned (needs a health-monitoring loop) | +| `before_deploy` | Deploy payload (event, mode, units, dry_run) | **Shipped** | +| `after_deploy` | Deploy payload plus `outcome` (`success` / `failed`) and `error` | **Shipped** | +| `before_down` | Teardown payload (event, mode, units, dry_run) | **Shipped** | +| `after_down` | Teardown payload plus `outcome` (`success` / `failed`) and `error` | **Shipped** | +| `before_build` | Build payload (event, sha, dry_run, images) | **Shipped** | +| `after_build` | Build payload plus `outcome` (`success` / `failed`) and `error` | **Shipped** | +| `on_instance_spawn` | Instance spawn payload (event, instance_id, challenge_slug, team_id, ports, flag, containers, expires_at) | **Shipped** | +| `on_health_failure` | — | Planned (needs a health-monitoring loop) | ### Deploy payload @@ -88,7 +104,7 @@ killed. (This mirrors the `notify` package.) { "event": "myctf", "phase": "before", // "before" | "after" - "mode": "docker", // "docker" | "ssh" + "mode": "docker", // "docker" | "ssh" | "kubernetes" "dry_run": false, "units": [ {"challenge":"web/echo","category":"web","slug":"echo", @@ -99,8 +115,7 @@ killed. (This mirrors the `notify` package.) } ``` -`after_deploy(success)` fires only on a real (non-dry-run) deploy; -`after_deploy(failed)` fires whenever the deploy errors. +`after_deploy(success)` fires only after a real, non-dry-run deploy. `after_deploy(failed)` fires when the deploy operation fails. Teardown uses the same payload structure with `before_down` and `after_down`. The `mode` field identifies the execution mode: `docker`, `ssh`, or `kubernetes`. Successful `after_*` hooks fire only after a real, non-dry-run operation; failure hooks fire when the operation fails. ### Build payload diff --git a/internal/plugin/plugin_test.go b/internal/plugin/plugin_test.go index 04c349f..92a6e6b 100644 --- a/internal/plugin/plugin_test.go +++ b/internal/plugin/plugin_test.go @@ -50,14 +50,14 @@ func TestHelperProcess(t *testing.T) { fmt.Fprintln(os.Stdout, "this is not json") // desync the stream continue } - hooks := []string{"before_deploy", "after_deploy", "before_build", "after_build", "on_instance_spawn"} + hooks := []string{"before_deploy", "after_deploy", "before_down", "after_down", "before_build", "after_build", "on_instance_spawn"} if mode == "nohooks" { hooks = nil } reply(req.ID, map[string]any{"name": "helper-" + mode, "hooks": hooks}) case "shutdown": os.Exit(0) - case "before_deploy", "after_deploy", "before_build", "after_build", "on_instance_spawn": + case "before_deploy", "after_deploy", "before_down", "after_down", "before_build", "after_build", "on_instance_spawn": switch mode { case "crash": os.Exit(1) @@ -322,3 +322,41 @@ func TestManager_RequiredPluginCanVetoDeploy(t *testing.T) { t.Fatalf("unexpected error: %v", err) } } + +func TestManager_DownHooksDelivered(t *testing.T) { + out := filepath.Join(t.TempDir(), "down_hooks.log") + m := helperManager(t, "echo", out) + if m == nil { + t.Fatal("manager should have one live plugin") + } + defer m.Close() + + ctx := context.Background() + m.Dispatch(ctx, HookBeforeDown, DeployPayload{ + Event: "test-event", + Phase: "before", + Mode: "docker", + }) + m.Dispatch(ctx, HookAfterDown, DeployPayload{ + Event: "test-event", + Phase: "after", + Mode: "docker", + Outcome: "success", + }) + + data, err := os.ReadFile(out) + if err != nil { + t.Fatalf("read hooks log: %v", err) + } + + s := string(data) + if !strings.Contains(s, "before_down") { + t.Errorf("before_down hook not delivered:\n%s", s) + } + if !strings.Contains(s, "after_down") { + t.Errorf("after_down hook not delivered:\n%s", s) + } + if !strings.Contains(s, `"outcome":"success"`) { + t.Errorf("after_down outcome not delivered:\n%s", s) + } +} diff --git a/internal/plugin/protocol.go b/internal/plugin/protocol.go index baf26fb..7a0271a 100644 --- a/internal/plugin/protocol.go +++ b/internal/plugin/protocol.go @@ -66,6 +66,8 @@ type initResult struct { const ( HookBeforeDeploy = "before_deploy" HookAfterDeploy = "after_deploy" + HookBeforeDown = "before_down" + HookAfterDown = "after_down" HookBeforeBuild = "before_build" HookAfterBuild = "after_build" HookOnInstanceSpawn = "on_instance_spawn" @@ -75,7 +77,7 @@ const ( type DeployPayload struct { Event string `json:"event"` Phase string `json:"phase"` // "before" | "after" - Mode string `json:"mode"` // "docker" | "ssh" + Mode string `json:"mode"` // "docker" | "ssh" | "kubernetes" DryRun bool `json:"dry_run"` Units []DeployUnit `json:"units"` Outcome string `json:"outcome,omitempty"` // after: "success" | "failed" From 9076af527185ec4f2e474c16b29e00da905e472b Mon Sep 17 00:00:00 2001 From: maya-ots Date: Fri, 23 Oct 2026 01:20:00 +0100 Subject: [PATCH 3/5] fix(plugin): handle dispatch errors --- cmd/build.go | 14 +++++++++----- cmd/deploy.go | 12 ++++++------ cmd/instance.go | 2 +- internal/instancer/server.go | 2 +- 4 files changed, 17 insertions(+), 13 deletions(-) diff --git a/cmd/build.go b/cmd/build.go index c4bbe2c..d866b6c 100644 --- a/cmd/build.go +++ b/cmd/build.go @@ -91,18 +91,20 @@ var buildCmd = &cobra.Command{ // Lifecycle plugins (best-effort; a no-op without a `plugins:` block). pm := plugin.Load(ctx, cfg.Plugins, plugin.Options{HostVersion: version, Dir: flagDir}) defer pm.Close() - pm.Dispatch(ctx, plugin.HookBeforeBuild, buildHookPayload(cfg, opts, "before", flagBuildDryRun, plan, "", nil)) + if err := pm.Dispatch(ctx, plugin.HookBeforeBuild, buildHookPayload(cfg, opts, "before", flagBuildDryRun, plan, "", nil)); err != nil { + return fmt.Errorf("build blocked by plugin: %w", err) + } buildOpts := opts buildOpts.Push = false if err := build.Apply(ctx, runner, plan, buildOpts); err != nil { - pm.Dispatch(ctx, plugin.HookAfterBuild, buildHookPayload(cfg, opts, "after", flagBuildDryRun, plan, "failed", err)) + _ = pm.Dispatch(ctx, plugin.HookAfterBuild, buildHookPayload(cfg, opts, "after", flagBuildDryRun, plan, "failed", err)) return err } if !flagBuildDryRun && flagBuildScan { if err := scanBuiltImages(ctx, plan); err != nil { - pm.Dispatch(ctx, plugin.HookAfterBuild, buildHookPayload(cfg, opts, "after", flagBuildDryRun, plan, "failed", err)) + _ = pm.Dispatch(ctx, plugin.HookAfterBuild, buildHookPayload(cfg, opts, "after", flagBuildDryRun, plan, "failed", err)) return err } } @@ -110,12 +112,14 @@ var buildCmd = &cobra.Command{ if flagBuildPush { fmt.Printf("\npushing %d image(s) to registry...\n", len(plan)) if err := build.ApplyPush(ctx, runner, plan, opts); err != nil { - pm.Dispatch(ctx, plugin.HookAfterBuild, buildHookPayload(cfg, opts, "after", flagBuildDryRun, plan, "failed", err)) + _ = pm.Dispatch(ctx, plugin.HookAfterBuild, buildHookPayload(cfg, opts, "after", flagBuildDryRun, plan, "failed", err)) return err } } - pm.Dispatch(ctx, plugin.HookAfterBuild, buildHookPayload(cfg, opts, "after", flagBuildDryRun, plan, "success", nil)) + if err := pm.Dispatch(ctx, plugin.HookAfterBuild, buildHookPayload(cfg, opts, "after", flagBuildDryRun, plan, "success", nil)); err != nil { + return fmt.Errorf("build completed, but plugin hook failed: %w", err) + } fmt.Println("\nbuild complete") return nil }, diff --git a/cmd/deploy.go b/cmd/deploy.go index 6d67ba2..75f2bd0 100644 --- a/cmd/deploy.go +++ b/cmd/deploy.go @@ -194,7 +194,7 @@ var deployCmd = &cobra.Command{ } if err := deploy.ApplyUp(ctx, runner, plan); err != nil { - pm.Dispatch(ctx, plugin.HookAfterDeploy, deployHookPayload(cfg, mode, "after", flagDeployDryRun, plan, "failed", err)) + _ = pm.Dispatch(ctx, plugin.HookAfterDeploy, deployHookPayload(cfg, mode, "after", flagDeployDryRun, plan, "failed", err)) notifyDiscord(ctx, "deploy failed", "error", fmt.Sprintf("%s: %v", planSlugs(plan), err)) return err } @@ -233,7 +233,7 @@ var deployCmd = &cobra.Command{ fmt.Println("\ndeploy complete") if !flagDeployDryRun { - pm.Dispatch(ctx, plugin.HookAfterDeploy, deployHookPayload(cfg, mode, "after", false, plan, "success", nil)) + _ = pm.Dispatch(ctx, plugin.HookAfterDeploy, deployHookPayload(cfg, mode, "after", false, plan, "success", nil)) notifyDiscord(ctx, "deploy succeeded", "success", fmt.Sprintf("%d challenge(s) up:\n%s", len(plan), planSlugs(plan))) } return nil @@ -409,7 +409,7 @@ var downCmd = &cobra.Command{ } if err := deploy.ApplyDown(ctx, runner, plan, flagDownVolumes); err != nil { - pm.Dispatch(ctx, plugin.HookAfterDown, + _ = pm.Dispatch(ctx, plugin.HookAfterDown, deployHookPayload(cfg, mode, "after", flagDownDryRun, plan, "failed", err)) notifyDiscord(ctx, "teardown failed", "error", fmt.Sprintf("%s: %v", planSlugs(plan), err)) @@ -417,7 +417,7 @@ var downCmd = &cobra.Command{ } if !flagDownDryRun { - pm.Dispatch(ctx, plugin.HookAfterDown, + _ = pm.Dispatch(ctx, plugin.HookAfterDown, deployHookPayload(cfg, mode, "after", false, plan, "success", nil)) } fmt.Println("\ndown complete") @@ -557,7 +557,7 @@ func applyKube(ctx context.Context, cfg *config.Config, challs []*challenge.Chal } if applyErr != nil { - pm.Dispatch(ctx, afterHook, + _ = pm.Dispatch(ctx, afterHook, kubeDeployHookPayload(cfg, "after", dry, plan, "failed", applyErr)) if verb == "up" { @@ -569,7 +569,7 @@ func applyKube(ctx context.Context, cfg *config.Config, challs []*challenge.Chal } if !dry { - pm.Dispatch(ctx, afterHook, + _ = pm.Dispatch(ctx, afterHook, kubeDeployHookPayload(cfg, "after", false, plan, "success", nil)) if verb == "up" { diff --git a/cmd/instance.go b/cmd/instance.go index 71cded5..3412c91 100644 --- a/cmd/instance.go +++ b/cmd/instance.go @@ -116,7 +116,7 @@ var instanceSpawnCmd = &cobra.Command{ // Lifecycle plugins (best-effort; a no-op without a `plugins:` block). pm := plugin.Load(ctx, cfg.Plugins, plugin.Options{HostVersion: version, Dir: flagDir}) defer pm.Close() - pm.Dispatch(ctx, plugin.HookOnInstanceSpawn, instanceSpawnPayload(cfg, inst)) + _ = pm.Dispatch(ctx, plugin.HookOnInstanceSpawn, instanceSpawnPayload(cfg, inst)) fmt.Printf("\nspawn complete (expires %s)\n", inst.ExpiresAt.Format(time.RFC3339)) return nil diff --git a/internal/instancer/server.go b/internal/instancer/server.go index d7e672b..b199863 100644 --- a/internal/instancer/server.go +++ b/internal/instancer/server.go @@ -192,7 +192,7 @@ func (s *Server) spawn(w http.ResponseWriter, r *http.Request) { if s.cfg != nil { eventSlug = s.cfg.Event.Slug } - s.pm.Dispatch(r.Context(), plugin.HookOnInstanceSpawn, plugin.InstanceSpawnPayload{ + _ = s.pm.Dispatch(r.Context(), plugin.HookOnInstanceSpawn, plugin.InstanceSpawnPayload{ Event: eventSlug, InstanceID: inst.ID, ChallengeSlug: inst.ChallengeSlug, From 1203a67f1268b7993b131eda2f4a6f6e3ad368c3 Mon Sep 17 00:00:00 2001 From: maya-ots Date: Sat, 31 Oct 2026 02:10:52 +0100 Subject: [PATCH 4/5] fix(plugin): propagate required hook failures --- cmd/deploy.go | 32 ++++++++++++++++++++++++-------- docs/spec/plugins.md | 2 +- internal/plugin/manager.go | 24 ++++++++++++++++++------ 3 files changed, 43 insertions(+), 15 deletions(-) diff --git a/cmd/deploy.go b/cmd/deploy.go index 75f2bd0..e703e55 100644 --- a/cmd/deploy.go +++ b/cmd/deploy.go @@ -233,7 +233,9 @@ var deployCmd = &cobra.Command{ fmt.Println("\ndeploy complete") if !flagDeployDryRun { - _ = pm.Dispatch(ctx, plugin.HookAfterDeploy, deployHookPayload(cfg, mode, "after", false, plan, "success", nil)) + if err := pm.Dispatch(ctx, plugin.HookAfterDeploy, deployHookPayload(cfg, mode, "after", false, plan, "success", nil)); err != nil { + return fmt.Errorf("after_deploy hook failed: %w", err) + } notifyDiscord(ctx, "deploy succeeded", "success", fmt.Sprintf("%d challenge(s) up:\n%s", len(plan), planSlugs(plan))) } return nil @@ -284,7 +286,11 @@ func kubeDeployHookPayload( } for _, u := range plan { p.Units = append(p.Units, plugin.DeployUnit{ - Challenge: u.Challenge, + Challenge: u.Challenge, + Category: u.Category, + Slug: u.Slug, + ProjectName: u.Name, + ImageTag: u.Image, }) } return p @@ -409,16 +415,21 @@ var downCmd = &cobra.Command{ } if err := deploy.ApplyDown(ctx, runner, plan, flagDownVolumes); err != nil { - _ = pm.Dispatch(ctx, plugin.HookAfterDown, + hookErr := pm.Dispatch(ctx, plugin.HookAfterDown, deployHookPayload(cfg, mode, "after", flagDownDryRun, plan, "failed", err)) notifyDiscord(ctx, "teardown failed", "error", fmt.Sprintf("%s: %v", planSlugs(plan), err)) + if hookErr != nil { + return fmt.Errorf("after_down hook failed: %w (teardown also failed: %v)", hookErr, err) + } return err } if !flagDownDryRun { - _ = pm.Dispatch(ctx, plugin.HookAfterDown, - deployHookPayload(cfg, mode, "after", false, plan, "success", nil)) + if err := pm.Dispatch(ctx, plugin.HookAfterDown, + deployHookPayload(cfg, mode, "after", false, plan, "success", nil)); err != nil { + return fmt.Errorf("after_down hook failed: %w", err) + } } fmt.Println("\ndown complete") if !flagDownDryRun { @@ -557,7 +568,7 @@ func applyKube(ctx context.Context, cfg *config.Config, challs []*challenge.Chal } if applyErr != nil { - _ = pm.Dispatch(ctx, afterHook, + hookErr := pm.Dispatch(ctx, afterHook, kubeDeployHookPayload(cfg, "after", dry, plan, "failed", applyErr)) if verb == "up" { @@ -565,12 +576,17 @@ func applyKube(ctx context.Context, cfg *config.Config, challs []*challenge.Chal } else { notifyDiscord(ctx, "teardown failed", "error", applyErr.Error()) } + if hookErr != nil { + return fmt.Errorf("after_%s hook failed: %w (operation also failed: %v)", verb, hookErr, applyErr) + } return applyErr } if !dry { - _ = pm.Dispatch(ctx, afterHook, - kubeDeployHookPayload(cfg, "after", false, plan, "success", nil)) + if err := pm.Dispatch(ctx, afterHook, + kubeDeployHookPayload(cfg, "after", false, plan, "success", nil)); err != nil { + return fmt.Errorf("after_%s hook failed: %w", verb, err) + } if verb == "up" { fmt.Println("\ndeploy complete") diff --git a/docs/spec/plugins.md b/docs/spec/plugins.md index 9f1c704..af93a7b 100644 --- a/docs/spec/plugins.md +++ b/docs/spec/plugins.md @@ -76,7 +76,7 @@ Setting `required: true` opts a plugin into blocking behavior: the command is blocked when the plugin manager reports that failure. - If a required plugin returns an error or fails during a blocking `before_*` hook, the command is aborted before the corresponding operation runs. -- Failures in `after_*` hooks do not undo an operation that has already completed. +- Failures in after_* hooks do not undo an operation that has already completed. However, failures from required plugins are returned to the CLI, allowing CI pipelines to detect them. - Non-required plugins remain best-effort, and their failures do not block the command. diff --git a/internal/plugin/manager.go b/internal/plugin/manager.go index 3459ca6..5cfac16 100644 --- a/internal/plugin/manager.go +++ b/internal/plugin/manager.go @@ -34,9 +34,10 @@ type Manager struct { } // Load parses raw plugin config, starts each enabled plugin, and runs the -// handshake. It is best-effort and crash-isolated: a plugin that fails to -// parse, start, or handshake is logged and excluded, never aborting the host -// command. Returns nil when no plugin is live. +// handshake. Optional plugin failures are logged and excluded without +// aborting the host command. Required plugin failures are retained and +// returned by Dispatch so they can block the host operation. +// Returns nil when no plugin is live and no required plugin has failed. func Load(ctx context.Context, raw []map[string]any, opts Options) *Manager { log := opts.Log if log == nil { @@ -52,6 +53,20 @@ func Load(ctx context.Context, raw []map[string]any, opts Options) *Manager { cfgs, err := ParseConfigs(raw) if err != nil { fmt.Fprintf(log, "warning: plugins disabled: %v\n", err) + + // Preserve required-plugin configuration errors so Dispatch can + // prevent the host operation from proceeding. + for _, entry := range raw { + required, _ := entry["required"].(bool) + if required { + return &Manager{ + opts: opts, + log: log, + loadErr: fmt.Errorf("required plugin configuration is invalid: %w", err), + } + } + } + return nil } @@ -95,9 +110,6 @@ func Load(ctx context.Context, raw []map[string]any, opts Options) *Manager { return m } -// Dispatch sends a hook to every subscribing plugin, sequentially, each under -// its own DispatchTimeout. A plugin that errors, crashes, or times out is -// logged and dropped from future dispatch. It never returns an error. // Dispatch sends a hook to every subscribing plugin, sequentially, each under // its own DispatchTimeout. Optional plugin failures are logged and dropped. // Failures from required plugins are also returned so blocking hooks can veto From c429935207c48c1af4ea7017d6ab0546d18151c4 Mon Sep 17 00:00:00 2001 From: maya-ots Date: Sat, 31 Oct 2026 11:37:40 +0100 Subject: [PATCH 5/5] test(plugin): cover required plugin failures --- cmd/deploy_mode_test.go | 57 ++++++++++++++++++++++++ internal/plugin/plugin_test.go | 81 +++++++++++++++++++++++++++++++++- 2 files changed, 137 insertions(+), 1 deletion(-) diff --git a/cmd/deploy_mode_test.go b/cmd/deploy_mode_test.go index c5cd415..28bd755 100644 --- a/cmd/deploy_mode_test.go +++ b/cmd/deploy_mode_test.go @@ -8,6 +8,7 @@ import ( "testing" "github.com/MicroClub-USTHB/mctf-cli/internal/config" + "github.com/MicroClub-USTHB/mctf-cli/internal/kubernetes" ) func cfgWithMode(mode string) *config.Config { @@ -100,3 +101,59 @@ func TestApplyKube_DoesNotMutateConfig(t *testing.T) { t.Errorf("config mutated: Deploy = %+v, want nil", nilDeploy.Deploy) } } + +func TestKubeDeployHookPayload_MapsAllUnitFields(t *testing.T) { + cfg := &config.Config{ + Event: config.Event{Slug: "test-event"}, + } + + plan := []kubernetes.KubeUnit{ + { + Challenge: "web/my-challenge", + Category: "web", + Slug: "my-challenge", + Name: "mctf-test-web-my-challenge", + Image: "registry.example.com/my-challenge:v1", + }, + } + + payload := kubeDeployHookPayload( + cfg, + "after", + false, + plan, + "success", + nil, + ) + + if payload.Event != "test-event" { + t.Errorf("Event = %q, want %q", payload.Event, "test-event") + } + if payload.Phase != "after" { + t.Errorf("Phase = %q, want %q", payload.Phase, "after") + } + if payload.Mode != "kubernetes" { + t.Errorf("Mode = %q, want %q", payload.Mode, "kubernetes") + } + if len(payload.Units) != 1 { + t.Fatalf("got %d units, want 1", len(payload.Units)) + } + + unit := payload.Units[0] + + if unit.Challenge != "web/my-challenge" { + t.Errorf("Challenge = %q, want %q", unit.Challenge, "web/my-challenge") + } + if unit.Category != "web" { + t.Errorf("Category = %q, want %q", unit.Category, "web") + } + if unit.Slug != "my-challenge" { + t.Errorf("Slug = %q, want %q", unit.Slug, "my-challenge") + } + if unit.ProjectName != "mctf-test-web-my-challenge" { + t.Errorf("ProjectName = %q, want %q", unit.ProjectName, "mctf-test-web-my-challenge") + } + if unit.ImageTag != "registry.example.com/my-challenge:v1" { + t.Errorf("ImageTag = %q, want %q", unit.ImageTag, "registry.example.com/my-challenge:v1") + } +} diff --git a/internal/plugin/plugin_test.go b/internal/plugin/plugin_test.go index 92a6e6b..b8e3b9f 100644 --- a/internal/plugin/plugin_test.go +++ b/internal/plugin/plugin_test.go @@ -94,7 +94,12 @@ func TestHelperProcess(t *testing.T) { // helperManager starts a Manager whose single plugin is the test binary // running as TestHelperProcess in the given mode. func helperManager(t *testing.T, mode, outfile string) *Manager { + return helperManagerWithRequired(t, mode, outfile, mode == "veto") +} + +func helperManagerWithRequired(t *testing.T, mode, outfile string, required bool) *Manager { t.Helper() + newCmd := func(ctx context.Context, name string, args ...string) *exec.Cmd { cmd := exec.CommandContext(ctx, name, args...) cmd.Env = append(os.Environ(), @@ -104,12 +109,14 @@ func helperManager(t *testing.T, mode, outfile string) *Manager { ) return cmd } + raw := []map[string]any{{ "name": "helper", "command": os.Args[0], "args": []any{"-test.run=TestHelperProcess"}, - "required": mode == "veto", + "required": required, }} + return Load(context.Background(), raw, Options{ HostVersion: "test", DispatchTimeout: 2 * time.Second, @@ -360,3 +367,75 @@ func TestManager_DownHooksDelivered(t *testing.T) { t.Errorf("after_down outcome not delivered:\n%s", s) } } + +func TestManager_RequiredPluginStartupFailureBlocksDispatch(t *testing.T) { + raw := []map[string]any{{ + "name": "missing", + "command": filepath.Join(t.TempDir(), "missing-plugin.exe"), + "required": true, + }} + + m := Load(context.Background(), raw, Options{Log: os.Stderr}) + if m == nil { + t.Fatal("expected manager when required plugin fails to start") + } + defer m.Close() + + err := m.Dispatch( + context.Background(), + HookBeforeDeploy, + DeployPayload{Phase: "before"}, + ) + if err == nil { + t.Fatal("expected required plugin startup failure to block dispatch") + } + if !strings.Contains(err.Error(), "failed to start") { + t.Fatalf("unexpected error: %v", err) + } +} + +func TestManager_RequiredPluginHandshakeFailureBlocksDispatch(t *testing.T) { + m := helperManagerWithRequired(t, "badhandshake", "", true) + if m == nil { + t.Fatal("expected manager when required plugin handshake fails") + } + defer m.Close() + + err := m.Dispatch( + context.Background(), + HookBeforeDeploy, + DeployPayload{Phase: "before"}, + ) + if err == nil { + t.Fatal("expected required plugin handshake failure to block dispatch") + } + if !strings.Contains(err.Error(), "handshake failed") { + t.Fatalf("unexpected error: %v", err) + } +} + +func TestManager_RequiredPluginInvalidConfigBlocksDispatch(t *testing.T) { + raw := []map[string]any{{ + "name": "invalid", + "required": true, + // Deliberately missing the required command field. + }} + + m := Load(context.Background(), raw, Options{Log: os.Stderr}) + if m == nil { + t.Fatal("expected manager for invalid required plugin configuration") + } + defer m.Close() + + err := m.Dispatch( + context.Background(), + HookBeforeDeploy, + DeployPayload{Phase: "before"}, + ) + if err == nil { + t.Fatal("expected invalid required plugin configuration to block dispatch") + } + if !strings.Contains(err.Error(), "configuration is invalid") { + t.Fatalf("unexpected error: %v", err) + } +}