From 709d751176c863f231e60a27960a14b0bb480311 Mon Sep 17 00:00:00 2001 From: scttfrdmn <3011922+scttfrdmn@users.noreply.github.com> Date: Tue, 18 Aug 2026 22:20:46 -0700 Subject: [PATCH] fix: reject param-file keys that look like spawn settings (#526) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every unrecognised key fell through to the PARAM_* passthrough arm, so a misspelled or invented spawn setting was indistinguishable from a workload parameter: `ttl_hours: 4` and `on-complete: terminate` launched instances with no bound and no complaint. Passthrough stays — it is the feature. A key is now rejected when it cannot become an env var at all (the bootstrap writes `export PARAM_=""` into /etc/profile.d, so a hyphen is a shell error), when it is a recognised key misspelled by hyphen or case, or when it is on a curated list of CLI-only flags and near-misses. Each error names the right spelling; all bad keys are reported in one pass. An explicit `param::` prefix is the escape hatch, so a reserved name is a redirect rather than a dead end for a workload that really does have a `budget` or `time_limit` parameter. Ambiguous words are off the list: `timeout` is documented step vocabulary and TestBuildLaunchConfigFromParams_WorkflowStep caught the first draft denying it. The recognised-key registry is a duplicate of the switch it mirrors, so a go/ast test requires the two to be equal in both directions — drift in the dangerous direction would otherwise be silent. --- CHANGELOG.md | 25 ++ cmd/launch_sweep.go | 10 + cmd/sweep.go | 16 +- cmd/sweep_keys.go | 311 ++++++++++++++++++++ cmd/sweep_keys_test.go | 367 ++++++++++++++++++++++++ test/e2e/tier0_sweep_param_keys_test.go | 152 ++++++++++ 6 files changed, 879 insertions(+), 2 deletions(-) create mode 100644 cmd/sweep_keys.go create mode 100644 cmd/sweep_keys_test.go create mode 100644 test/e2e/tier0_sweep_param_keys_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 6017ef8..b623c71 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,31 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Fixed +- **A param-file key that looked like a spawn setting silently became a + `PARAM_*` env var and did nothing** (#526). Every unrecognised key fell through + to the passthrough arm, which is correct for a workload parameter and disastrous + for a misspelled setting: `ttl_hours: 4`, `on-complete: terminate`, `budget: 50` + and `max_concurrent: 3` all launched normally and bounded nothing. The first two + leave a running instance, which is the version of this bug that costs money. + + Passthrough is unchanged for real parameters. A key is now rejected before + anything is launched or priced when it is (a) not a valid shell identifier — the + bootstrap writes each parameter into `/etc/profile.d` as + `export PARAM_=""`, so `PARAM_on-complete` is a line the shell + refuses, (b) a recognised key written with hyphens or in the wrong case, or (c) + on a curated list of CLI-only flags and near-misses, each carrying the correct + spelling in the error. All offending keys are reported at once rather than one + per run, and the `PARAM_*` variables a sweep *will* set are now listed in the + sweep header so an unintended passthrough has somewhere to be noticed. + + Because a reserved name would otherwise be a dead end for anyone whose workload + genuinely has a parameter called `budget` or `time_limit`, an explicit + `param::` prefix passes any name straight through as `PARAM_`. + Ambiguous English words are deliberately absent from the list — `timeout` is + documented step vocabulary (`examples/workflow-ci-pipeline.yaml`), and `image`, + `type`, `count`, `steps`, `instance` and `runtime` are all plausible sweep + parameters. The same check runs at the config-merge seam too, so `spawn resume` + cannot rebuild an instance from a file the launch path would have refused. - **A parameter sweep silently discarded `--ttl`, `--idle-timeout` and `--cost-limit`, then logged `Using safeguards: ttl=...`** (#525). `buildLaunchConfigFromParams` starts from an empty config and the sweep diff --git a/cmd/launch_sweep.go b/cmd/launch_sweep.go index 23f5b5a..7eb0242 100644 --- a/cmd/launch_sweep.go +++ b/cmd/launch_sweep.go @@ -46,6 +46,15 @@ func launchParameterSweep(ctx context.Context, baseConfig *aws.LaunchConfig, pla return fmt.Errorf("either --param-file or --params must be specified for parameter sweep") } + // Reject keys that look like spawn settings before anything is launched or + // priced (#526). This runs before applyCLISpendControlsToSweep so the error + // names only what the user actually wrote, not the ttl/cost_limit that is + // about to be injected below. + if err := validateSweepParamKeys(paramFormat); err != nil { + fmt.Fprintf(os.Stderr, "\n❌ ERROR: %v\n\n", err) + return fmt.Errorf("unusable param-file keys") + } + // The CLI spend-control flags used to be dropped on the floor for a sweep // (#525). buildLaunchConfigFromParams "start[s] with an empty config" and the // dispatch below copies only Region/InstanceType/Name off baseConfig, so @@ -177,6 +186,7 @@ func launchParameterSweep(ctx context.Context, baseConfig *aws.LaunchConfig, pla fmt.Fprintf(os.Stderr, " From the command line: %s (each row's own value wins)\n", strings.Join(appliedControls, ", ")) } + reportPassthroughParams(paramFormat) fmt.Fprintf(os.Stderr, "\n") // --estimate-only is handled HERE, above the detached/foreground dispatch diff --git a/cmd/sweep.go b/cmd/sweep.go index 82a8fbb..e2a43d0 100644 --- a/cmd/sweep.go +++ b/cmd/sweep.go @@ -242,8 +242,20 @@ func buildLaunchConfigFromParams(defaults, params map[string]interface{}, sweepI config.Name = s } default: - // All unknown fields become parameters (PARAM_* env vars) - config.Parameters[key] = fmt.Sprintf("%v", val) + // Unknown fields become workload parameters (PARAM_* env vars) — the + // passthrough that makes a sweep useful. But a key that only LOOKS like + // a workload parameter is rejected here rather than exported into the + // void: see cmd/sweep_keys.go for the three rules and #526 for the + // table of misconfigurations this used to accept in silence. + // + // launchParameterSweep runs the same check up front, with a better + // message and all the bad keys at once. This copy is the one that + // covers resume and the quota preflight, which do not go through there. + name, err := resolveParamName(key) + if err != nil { + return config, err + } + config.Parameters[name] = fmt.Sprintf("%v", val) } } diff --git a/cmd/sweep_keys.go b/cmd/sweep_keys.go new file mode 100644 index 0000000..02118c8 --- /dev/null +++ b/cmd/sweep_keys.go @@ -0,0 +1,311 @@ +package cmd + +import ( + "fmt" + "os" + "regexp" + "sort" + "strings" +) + +// Param-file key validation for the sweep launch path (#526). +// +// buildLaunchConfigFromParams routes any key it does not recognise to +// config.Parameters, which becomes a spawn:param: tag and then a PARAM_ +// environment variable on the instance. That passthrough is the feature — it is +// how a sweep hands values to a workload — but it also means the parser could +// never say "that isn't a thing". A key the user *meant* as a spawn setting and a +// key they meant for their own program are indistinguishable, so `ttl_hours: 4` +// launched an instance with no TTL and looked completely healthy doing it. +// +// The fix keeps passthrough and adds three ways for a key to be rejected instead, +// in the order they are checked: +// +// A. it cannot become an environment variable at all (invalid shell identifier) +// B. it is a recognised spawn key misspelled — hyphens, or the wrong case +// C. it is on the curated list of names below: CLI-only flags and settings +// people reasonably assume exist +// +// Anything else passes through, and is now listed at launch so an intentional +// parameter is visible and an unintentional one has somewhere to be noticed. + +// recognizedRowKeys is every key buildLaunchConfigFromParams acts on, including +// aliases. It exists so the near-miss check has something to compare against. +// +// This list is a duplicate of that function's case labels, which is exactly the +// kind of duplication that rots — so TestRecognizedRowKeysMatchesSwitch parses +// the switch out of the source with go/ast and requires the two to be equal. +// Adding a case without adding it here fails that test. +var recognizedRowKeys = map[string]bool{ + "instance_type": true, + "region": true, + "az": true, + "availability_zone": true, + "ami": true, + "key_pair": true, + "key_name": true, + "spot": true, + "spot_max_price": true, + "hibernate": true, + "ttl": true, + "idle_timeout": true, + "cost_limit": true, + "hibernate_on_idle": true, + "session_timeout": true, + "on_complete": true, + "completion_file": true, + "completion_delay": true, + "dns": true, + "dns_name": true, + "step": true, + "command": true, + "user_command": true, + "user_data": true, + "iam_role": true, + "name": true, +} + +// reservedRowKeys maps a key that must not silently pass through to the guidance +// printed when it appears. Every entry is either a CLI-only flag or a control a +// user could reasonably believe exists; the ones that cost money are the point. +// +// Deliberately NOT on this list: plausible workload parameter names that merely +// resemble a spawn setting — `image` (a container image is a normal sweep +// parameter), `type`, `count`, `steps`, `instance` (an optimisation problem +// instance), `runtime` (a container runtime). Rejecting those would break the +// feature this check is meant to preserve. When in doubt the key passes through: a +// denylist that swallows legitimate parameters is a worse bug than the one it +// fixes. +// +// `timeout` is the load-bearing example, and it is not on this list because +// spawn's own examples/workflow-ci-pipeline.yaml sets `timeout: 10m` per step and +// pkg/queue.JobConfig has a Timeout field — it is documented vocabulary on the +// workflow path, not a near-miss. TestBuildLaunchConfigFromParams_WorkflowStep +// caught the first draft of this list denying it. +// +// The entries that remain are still English words someone could plausibly sweep +// over: `budget` is an optimiser's evaluation budget as often as it is a dollar +// figure, `time_limit` is a standard solver option. They stay because the failure +// they prevent costs money — and because the escape hatch below means a rejection +// is never a dead end: `param:budget: 50` passes through untouched. +var reservedRowKeys = map[string]string{ + // Bounds the user believes they set. These are the expensive ones. + "ttl_hours": "use ttl: with a duration, e.g. ttl: 4h", + "ttl_minutes": "use ttl: with a duration, e.g. ttl: 90m", + "ttl_seconds": "use ttl: with a duration, e.g. ttl: 600s", + "time_limit": "use ttl: with a duration, e.g. ttl: 4h", + "max_runtime": "use ttl: with a duration, e.g. ttl: 4h", + "walltime": "use ttl: with a duration, e.g. ttl: 4h", + "idle_time": "use idle_timeout: with a duration, e.g. idle_timeout: 30m", + "max_cost": "use cost_limit: with a number in USD, e.g. cost_limit: 8", + "cost_cap": "use cost_limit: with a number in USD, e.g. cost_limit: 8", + "spend_limit": "use cost_limit: with a number in USD, e.g. cost_limit: 8", + "budget": "spawn has no per-sweep budget key; bound each row with ttl: (worst case) and cost_limit: (spend cap)", + "on_completion": "use on_complete:, e.g. on_complete: terminate", + "oncomplete": "use on_complete:, e.g. on_complete: terminate", + "terminate_on_done": "use on_complete: terminate", + "terminate_on_complete": "use on_complete: terminate", + "no_timeout": "--no-timeout is a command-line flag; there is no param-file equivalent (omit ttl:/idle_timeout: instead — but a sweep row with no bound is how zombie instances happen)", + + // Sweep-level settings that are CLI flags, not per-row values. + "max_concurrent": "--max-concurrent is a command-line flag, not a param-file key (spawn schedule reads max_concurrent from defaults:, but the launch path does not)", + "launch_delay": "--launch-delay is a command-line flag, not a param-file key", + "sweep_name": "--name is a command-line flag (spawn schedule reads sweep_name from defaults:, but the launch path does not)", + "detach": "--detach/--no-detach are command-line flags", + "no_detach": "--detach/--no-detach are command-line flags", + "estimate_only": "--estimate-only is a command-line flag", + "dry_run": "--estimate-only is the command-line flag for pricing a sweep without launching it", + + // Shapes that do not mean what they look like. + "instance_types": "one instance_type: per row, or use grid: {instance_type: [...]} to expand a list into rows", + "image_id": "use ami:", + "ami_id": "use ami:", + "keypair": "use key_pair: (or key_name:)", + "ssh_key": "use key_pair: (or key_name:)", + "spot_price": "use spot_max_price:", + "iam_profile": "use iam_role:", + "instance_profile": "use iam_role:", + "iam_instance_profile": "use iam_role:", + "region_name": "use region:", + "availability_zones": "one az: per row, or use grid: {az: [...]}", + "disk_size": "--disk-size is a command-line flag, not a param-file key", + "volume_size": "--disk-size is a command-line flag, not a param-file key", +} + +// shellIdentifier is the set of names that can survive the trip to the instance. +// pkg/launcher/bootstrap.go writes each parameter into /etc/profile.d as +// +// export PARAM_="" +// +// so a key that is not a valid shell identifier produces a line the shell +// refuses — `export PARAM_on-complete="terminate"` is a "not a valid identifier" +// error in every login shell, and the parameter reaches the workload as nothing. +var shellIdentifier = regexp.MustCompile(`^[A-Za-z_][A-Za-z0-9_]*$`) + +// normalizeKey folds the two ways a recognised key gets misspelled: CLI-style +// hyphens (`on-complete`) and inherited capitalisation (`TTL`, `Instance_Type`). +func normalizeKey(key string) string { + return strings.ToLower(strings.ReplaceAll(key, "-", "_")) +} + +// explicitParamPrefix opts a key out of the near-miss and reserved-name checks: +// `param:budget: 50` means "I really do want PARAM_budget", and gets it. +// +// Without this, every reserved name is a dead end rather than a redirect — a user +// sweeping a solver's `time_limit` or an optimiser's `budget` would have no way to +// pass it at all, and the check would have broken the feature it is protecting. +// Rule A still applies to what follows the prefix, because that part becomes the +// environment variable name. +const explicitParamPrefix = "param:" + +// explicitParamKey returns the parameter name behind an explicit param: prefix. +func explicitParamKey(key string) (string, bool) { + if !strings.HasPrefix(key, explicitParamPrefix) { + return "", false + } + return strings.TrimPrefix(key, explicitParamPrefix), true +} + +// classifyRowKey reports why an unrecognised param-file key must be rejected, or +// returns nil if it is a legitimate passthrough parameter. Callers have already +// established that the key is not in recognizedRowKeys. +func classifyRowKey(key string) error { + _, err := resolveParamName(key) + return err +} + +// resolveParamName returns the PARAM_* name an unrecognised key becomes, or an +// error explaining why it cannot become one. It is the single place the three +// rejection rules live; classifyRowKey is the boolean-ish wrapper for validation +// passes that do not need the name. +func resolveParamName(key string) (string, error) { + if name, explicit := explicitParamKey(key); explicit { + if name == "" { + return "", fmt.Errorf("%q has nothing after %q — write param: with the "+ + "parameter name you want", key, explicitParamPrefix) + } + if !shellIdentifier.MatchString(name) { + return "", fmt.Errorf("%q cannot become an environment variable: %q is not a valid "+ + "shell identifier (letters, digits and underscores only, not starting with a digit)", + key, name) + } + return name, nil + } + + norm := normalizeKey(key) + + // B before A: a near-miss gets the specific message even when it is also an + // invalid identifier, because "did you mean on_complete:" is more use than + // "that cannot be an environment variable". + if norm != key && recognizedRowKeys[norm] { + return "", fmt.Errorf("unknown key %q — did you mean %q? (spawn keys are lowercase with underscores)", key, norm) + } + if hint, ok := reservedRowKeys[norm]; ok { + return "", fmt.Errorf("%q is not a spawn setting: %s. If you really do mean a parameter "+ + "for your workload, write %s%s and it will be passed through as PARAM_%s", + key, hint, explicitParamPrefix, norm, norm) + } + if !shellIdentifier.MatchString(key) { + return "", fmt.Errorf("unknown key %q cannot be passed to the workload: parameters become "+ + "PARAM_ environment variables, and %q is not a valid shell identifier "+ + "(letters, digits and underscores only, not starting with a digit)", key, key) + } + return key, nil +} + +// passthroughKeys returns the sorted parameter keys a merged param set would send +// to the workload as PARAM_* env vars. +func passthroughKeys(m map[string]interface{}) []string { + var out []string + for k := range m { + if !recognizedRowKeys[k] { + out = append(out, k) + } + } + sort.Strings(out) + return out +} + +// validateSweepParamKeys checks every key in defaults: and in each row before +// anything is launched, and returns one error naming ALL the bad keys rather than +// failing on the first. A 30-row sweep with three typos should be one round trip, +// not three. +// +// buildLaunchConfigFromParams rejects the same keys on its own (it is reached by +// resume and the quota preflight too), so this is the early, better-worded copy of +// a check that also exists at the seam. Deliberate duplication: this one runs +// before the first AWS call. +func validateSweepParamKeys(paramFormat *ParamFileFormat) error { + type problem struct { + where string + err error + } + var problems []problem + + check := func(where string, m map[string]interface{}) { + for _, k := range passthroughKeys(m) { + if err := classifyRowKey(k); err != nil { + problems = append(problems, problem{where: where, err: err}) + } + } + } + + check("defaults:", paramFormat.Defaults) + for i, row := range paramFormat.Params { + label := fmt.Sprintf("row %d", i) + if it, ok := row["instance_type"].(string); ok && it != "" { + label += " (" + it + ")" + } + check(label, row) + } + if len(problems) == 0 { + return nil + } + + var b strings.Builder + fmt.Fprintf(&b, "%d unusable key(s) in %s:", len(problems), paramFile) + for _, p := range problems { + fmt.Fprintf(&b, "\n %s: %v", p.where, p.err) + } + b.WriteString("\n\n Keys spawn does not recognise are passed to the workload as PARAM_* " + + "environment variables, so a misspelled setting cannot be told apart from one of your " + + "own parameters. The keys above are rejected because they look like spawn settings that " + + "would silently do nothing.") + return fmt.Errorf("%s", b.String()) +} + +// reportPassthroughParams lists the PARAM_* variables the sweep will set, so an +// intentional parameter is confirmed and an unintentional one is at least visible +// — option (3) of #526, which is the weak half of the fix on its own and the +// useful half once the dangerous names are hard errors. +func reportPassthroughParams(paramFormat *ParamFileFormat) { + seen := map[string]bool{} + var keys []string + for _, m := range append([]map[string]interface{}{paramFormat.Defaults}, paramFormat.Params...) { + for _, k := range passthroughKeys(m) { + if !seen[k] { + seen[k] = true + keys = append(keys, k) + } + } + } + if len(keys) == 0 { + return + } + sort.Strings(keys) + prefixed := make([]string, 0, len(keys)) + for _, k := range keys { + // Show the name the workload will actually see, which for an explicit + // param:budget is PARAM_budget. + if name, err := resolveParamName(k); err == nil { + prefixed = append(prefixed, "PARAM_"+name) + } + } + if len(prefixed) == 0 { + return + } + sort.Strings(prefixed) + fmt.Fprintf(os.Stderr, " Workload parameters: %s\n", strings.Join(prefixed, ", ")) + fmt.Fprintf(os.Stderr, " (any key spawn does not recognise is passed through as an env var — "+ + "check this list if a setting seems to have no effect)\n") +} diff --git a/cmd/sweep_keys_test.go b/cmd/sweep_keys_test.go new file mode 100644 index 0000000..103bc6a --- /dev/null +++ b/cmd/sweep_keys_test.go @@ -0,0 +1,367 @@ +package cmd + +import ( + "go/ast" + "go/parser" + "go/token" + "sort" + "strconv" + "strings" + "testing" +) + +// TestRecognizedRowKeysMatchesSwitch is the reason recognizedRowKeys is allowed to +// exist as a hand-written duplicate of buildLaunchConfigFromParams' case labels. +// It reads the switch out of cmd/sweep.go with go/ast and requires exact equality +// in both directions: +// +// - a case label missing from the map means a REAL spawn key is treated as a +// near-miss candidate and possibly rejected — a working param file breaks +// - a map entry with no case label means a key spawn ignores is advertised as +// recognised, which is the #526 bug wearing a different hat +// +// Without this test the map would drift the first time someone adds a field, and +// the drift would be silent in exactly the direction that costs money. +func TestRecognizedRowKeysMatchesSwitch(t *testing.T) { + fset := token.NewFileSet() + file, err := parser.ParseFile(fset, "sweep.go", nil, 0) + if err != nil { + t.Fatalf("parse sweep.go: %v", err) + } + + fn := findFuncDecl(file, "buildLaunchConfigFromParams") + if fn == nil { + t.Fatal("buildLaunchConfigFromParams not found in sweep.go — this test's premise is gone, " + + "not its subject: find where the param keys are switched on now and re-point it") + } + + labels := map[string]bool{} + ast.Inspect(fn, func(n ast.Node) bool { + sw, ok := n.(*ast.SwitchStmt) + if !ok { + return true + } + // Only the switch over the merged param key, not any inner switch. + if id, ok := sw.Tag.(*ast.Ident); !ok || id.Name != "key" { + return true + } + for _, stmt := range sw.Body.List { + cc, ok := stmt.(*ast.CaseClause) + if !ok { + continue + } + for _, expr := range cc.List { + lit, ok := expr.(*ast.BasicLit) + if !ok || lit.Kind != token.STRING { + t.Errorf("non-literal case in the param-key switch: %T — this test can only "+ + "see string literals", expr) + continue + } + s, err := strconv.Unquote(lit.Value) + if err != nil { + t.Errorf("unquote case %s: %v", lit.Value, err) + continue + } + labels[s] = true + } + } + return true + }) + + if len(labels) == 0 { + t.Fatal("found no case labels in the param-key switch — the AST walk is broken, and a " + + "broken walk here would make every assertion below pass vacuously") + } + + for k := range labels { + if !recognizedRowKeys[k] { + t.Errorf("case %q exists in buildLaunchConfigFromParams but is missing from "+ + "recognizedRowKeys — spawn acts on that key, so it must not be treated as an "+ + "unknown one", k) + } + } + for k := range recognizedRowKeys { + if !labels[k] { + t.Errorf("recognizedRowKeys has %q but buildLaunchConfigFromParams has no case for it "+ + "— that key is advertised as recognised and silently does nothing", k) + } + } +} + +func findFuncDecl(file *ast.File, name string) *ast.FuncDecl { + for _, decl := range file.Decls { + if fn, ok := decl.(*ast.FuncDecl); ok && fn.Name.Name == name { + return fn + } + } + return nil +} + +// TestClassifyRowKeyRejects covers each rule with the cases from the #526 table, +// and asserts on the guidance text rather than just "an error happened" — the +// whole value of this check is that the message tells the user the right spelling. +func TestClassifyRowKeyRejects(t *testing.T) { + tests := []struct { + key string + wantHint string // substring the message must contain + why string + }{ + {"ttl_hours", "ttl:", "a TTL the user believes they set — the expensive case"}, + {"ttl_minutes", "ttl:", "same, other unit"}, + {"max_runtime", "ttl:", "plausible name from other schedulers"}, + {"walltime", "ttl:", "the HPC spelling"}, + {"max_cost", "cost_limit:", "a dollar cap that capped nothing"}, + {"budget", "cost_limit:", "no such key; point at what does work"}, + {"max_concurrent", "command-line flag", "CLI-only"}, + {"launch_delay", "command-line flag", "CLI-only"}, + {"instance_types", "grid:", "plural means rows or a grid, not a list in one row"}, + {"image_id", "ami:", "wrong name for the same thing"}, + {"spot_price", "spot_max_price:", "near-miss on a real key"}, + {"on-complete", "on_complete", "hyphen instead of underscore — rule B"}, + {"instance-type", "instance_type", "hyphen on the key that decides what you pay for"}, + {"cost-limit", "cost_limit", "hyphen on the dollar cap"}, + {"TTL", "ttl", "wrong case — rule B"}, + {"Instance_Type", "instance_type", "inherited capitalisation"}, + {"my-workload-flag", "not a valid shell identifier", "not a spawn key, but cannot be an env var either — rule A"}, + {"2fast", "not a valid shell identifier", "leading digit"}, + {"has space", "not a valid shell identifier", "space"}, + } + for _, tc := range tests { + t.Run(tc.key, func(t *testing.T) { + if recognizedRowKeys[tc.key] { + t.Fatalf("%q is a recognised key, so classifyRowKey is never called on it — "+ + "this case is testing nothing", tc.key) + } + err := classifyRowKey(tc.key) + if err == nil { + t.Fatalf("classifyRowKey(%q) = nil, want an error (%s)", tc.key, tc.why) + } + if !strings.Contains(err.Error(), tc.wantHint) { + t.Errorf("classifyRowKey(%q) message does not contain %q, so it does not tell the "+ + "user what to write instead:\n %v", tc.key, tc.wantHint, err) + } + }) + } +} + +// TestClassifyRowKeyAllowsWorkloadParams is the half of this feature that must NOT +// regress. Every key here is a real parameter someone sweeps over, and several +// deliberately resemble a spawn setting — `image` is a container image, `steps` is +// an MD step count next to spawn's `step`. A denylist that eats these is a worse +// bug than the one it fixes, so they are pinned as allowed. +func TestClassifyRowKeyAllowsWorkloadParams(t *testing.T) { + allowed := []string{ + "alpha", "beta", "learning_rate", "batch_size", "epochs", "seed", + "image", "container", "type", "count", "steps", "nsteps", "threads", + "dataset", "optimizer", "temperature", "cutoff", "n_gpu", "mdp", + "TEMPERATURE", "N_REPLICAS", "_private", "x2", "gmx_bin", + // timeout is documented vocabulary on the workflow path + // (examples/workflow-ci-pipeline.yaml, pkg/queue.JobConfig.Timeout), not a + // near-miss. The first draft of the denylist rejected it and + // TestBuildLaunchConfigFromParams_WorkflowStep failed — that test is the + // reason this entry is pinned here. + "timeout", "runtime", "instance", "instances", "idle", + } + for _, key := range allowed { + if recognizedRowKeys[key] { + t.Errorf("%q is in recognizedRowKeys, so it is not a passthrough parameter at all — "+ + "this list is asserting the wrong thing about it", key) + continue + } + if err := classifyRowKey(key); err != nil { + t.Errorf("classifyRowKey(%q) rejected a legitimate workload parameter: %v", key, err) + } + } +} + +// TestNoRecognizedKeyIsAlsoReserved: an entry in both maps would be unreachable +// (the switch handles it before classifyRowKey is called), so it is dead guidance +// that reads as active. The overlap is easy to introduce by adding a real key +// whose name someone already listed as a near-miss. +func TestNoRecognizedKeyIsAlsoReserved(t *testing.T) { + for k := range reservedRowKeys { + if recognizedRowKeys[k] { + t.Errorf("%q is both recognised and reserved — the reserved entry can never fire", k) + } + } +} + +// TestReservedKeysAreNormalized: lookups happen on the normalized key, so a +// reserved entry written with a hyphen or a capital could never match. That would +// be a check that cannot fail, which is the failure mode this repo keeps hitting. +func TestReservedKeysAreNormalized(t *testing.T) { + for k := range reservedRowKeys { + if norm := normalizeKey(k); norm != k { + t.Errorf("reserved key %q is not in normalized form (%q), so it can never be matched", k, norm) + } + } +} + +func TestValidateSweepParamKeysReportsEveryProblem(t *testing.T) { + pf := &ParamFileFormat{ + Defaults: map[string]interface{}{ + "on_complete": "terminate", + "budget": 50, + }, + Params: []map[string]interface{}{ + {"instance_type": "c7g.16xlarge", "ttl_hours": 4}, + {"instance_type": "c6i.32xlarge", "alpha": 0.5}, + {"instance_type": "g5.xlarge", "max_cost": 20}, + }, + } + err := validateSweepParamKeys(pf) + if err != nil { + msg := err.Error() + // One error naming all three, not one round trip per typo. + for _, want := range []string{"budget", "ttl_hours", "max_cost", "defaults:", "c7g.16xlarge", "g5.xlarge"} { + if !strings.Contains(msg, want) { + t.Errorf("error does not mention %q:\n%s", want, msg) + } + } + if strings.Contains(msg, "alpha") { + t.Errorf("error names alpha, which is a legitimate workload parameter:\n%s", msg) + } + if strings.Contains(msg, "c6i.32xlarge") { + t.Errorf("error names the row whose only extra key is legitimate:\n%s", msg) + } + } else { + t.Error("validateSweepParamKeys accepted a file with budget:, ttl_hours: and max_cost:") + } +} + +func TestValidateSweepParamKeysAcceptsCleanFile(t *testing.T) { + pf := &ParamFileFormat{ + Defaults: map[string]interface{}{"ttl": "1h", "on_complete": "terminate", "cost_limit": 8}, + Params: []map[string]interface{}{ + {"instance_type": "c7g.16xlarge", "isa": "neon", "nsteps": 500000}, + {"instance_type": "c8g.16xlarge", "isa": "sve", "nsteps": 500000, "ttl": "30m"}, + }, + } + if err := validateSweepParamKeys(pf); err != nil { + t.Errorf("rejected a valid file: %v", err) + } +} + +// TestBuildLaunchConfigRejectsDangerousKeyAtTheSeam: the check also lives in +// buildLaunchConfigFromParams, because resume and the quota preflight call that +// directly and never pass through launchParameterSweep's early validation. +func TestBuildLaunchConfigRejectsDangerousKeyAtTheSeam(t *testing.T) { + _, err := buildLaunchConfigFromParams( + map[string]interface{}{"ttl_hours": 4}, + map[string]interface{}{"instance_type": "c5.large"}, + "sweep-1", "bench", 0, 1, + ) + if err == nil { + t.Fatal("buildLaunchConfigFromParams accepted ttl_hours: — resume would rebuild an " + + "unbounded instance from a file the launch path had rejected") + } + if !strings.Contains(err.Error(), "ttl") { + t.Errorf("error does not point at the right spelling: %v", err) + } +} + +func TestBuildLaunchConfigStillPassesThroughParams(t *testing.T) { + config, err := buildLaunchConfigFromParams( + map[string]interface{}{"ttl": "1h"}, + map[string]interface{}{"instance_type": "c5.large", "alpha": 0.1, "nsteps": 1000}, + "sweep-1", "bench", 0, 1, + ) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if config.Parameters["alpha"] != "0.1" { + t.Errorf("alpha = %q, want \"0.1\" — passthrough is the feature", config.Parameters["alpha"]) + } + if config.Parameters["nsteps"] != "1000" { + t.Errorf("nsteps = %q, want \"1000\"", config.Parameters["nsteps"]) + } + if config.TTL != "1h" { + t.Errorf("TTL = %q, want 1h", config.TTL) + } +} + +func TestPassthroughKeysExcludesRecognized(t *testing.T) { + got := passthroughKeys(map[string]interface{}{ + "instance_type": "c5.large", + "ttl": "1h", + "cost_limit": 8, + "beta": 2, + "alpha": 1, + }) + want := []string{"alpha", "beta"} + sort.Strings(got) + if strings.Join(got, ",") != strings.Join(want, ",") { + t.Errorf("passthroughKeys = %v, want %v", got, want) + } +} + +// TestExplicitParamPrefixEscapesTheDenylist: every reserved name must have a way +// through, or the check has replaced a silent misconfiguration with a hard block on +// a legitimate parameter — `budget` is an optimiser's evaluation budget as often as +// it is a dollar figure, and `time_limit` is a standard solver option. +func TestExplicitParamPrefixEscapesTheDenylist(t *testing.T) { + for _, key := range []string{"param:budget", "param:ttl_hours", "param:time_limit", "param:max_cost"} { + name, err := resolveParamName(key) + if err != nil { + t.Errorf("resolveParamName(%q) = %v, want it to pass through — a denylist with no "+ + "escape hatch blocks the feature it is protecting", key, err) + continue + } + want := strings.TrimPrefix(key, explicitParamPrefix) + if name != want { + t.Errorf("resolveParamName(%q) = %q, want %q (the prefix must not reach the env var)", + key, name, want) + } + } +} + +// TestExplicitParamPrefixStillEnforcesRuleA: the prefix opts out of the near-miss +// and reserved checks, not out of "this has to be a legal env var name". +func TestExplicitParamPrefixStillEnforcesRuleA(t *testing.T) { + for _, key := range []string{"param:on-complete", "param:2fast", "param:has space", "param:"} { + if _, err := resolveParamName(key); err == nil { + t.Errorf("resolveParamName(%q) = nil, want an error: the name after the prefix still "+ + "becomes PARAM_ in /etc/profile.d", key) + } + } +} + +// TestReservedKeyErrorNamesTheEscapeHatch: the message has to carry the remedy, +// otherwise the user's only option is to guess. +func TestReservedKeyErrorNamesTheEscapeHatch(t *testing.T) { + err := classifyRowKey("budget") + if err == nil { + t.Fatal("budget: was accepted") + } + for _, want := range []string{"param:budget", "PARAM_budget"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error does not mention %q, so the rejection is a dead end:\n %v", want, err) + } + } +} + +// TestExplicitParamPrefixReachesParameters is the end-to-end of the escape hatch +// through the real merge function: prefix stripped, value intact, and no spawn +// setting touched by a key that merely looked like one. +func TestExplicitParamPrefixReachesParameters(t *testing.T) { + config, err := buildLaunchConfigFromParams( + map[string]interface{}{"ttl": "1h"}, + map[string]interface{}{"instance_type": "c5.large", "param:budget": 50, "param:time_limit": 300}, + "sweep-1", "bench", 0, 1, + ) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if config.Parameters["budget"] != "50" { + t.Errorf("PARAM_budget = %q, want \"50\"", config.Parameters["budget"]) + } + if config.Parameters["time_limit"] != "300" { + t.Errorf("PARAM_time_limit = %q, want \"300\"", config.Parameters["time_limit"]) + } + if _, leaked := config.Parameters["param:budget"]; leaked { + t.Error("the param: prefix leaked into the parameter name") + } + if config.TTL != "1h" { + t.Errorf("TTL = %q, want 1h — an escaped key must not disturb the real settings", config.TTL) + } +} diff --git a/test/e2e/tier0_sweep_param_keys_test.go b/test/e2e/tier0_sweep_param_keys_test.go new file mode 100644 index 0000000..9dfd30b --- /dev/null +++ b/test/e2e/tier0_sweep_param_keys_test.go @@ -0,0 +1,152 @@ +//go:build e2e_tier0 + +package e2e + +import ( + "strings" + "testing" +) + +// Tier 0 regression coverage for #526: a param-file key that looks like a spawn +// setting must fail the launch, and a key that is genuinely a workload parameter +// must still reach the instance. +// +// The bug was the `default:` arm of buildLaunchConfigFromParams: every unrecognised +// key became a PARAM_ env var, so `ttl_hours: 4` and `on-complete: terminate` +// launched instances with no bound and no complaint. The table in #526 lists six +// such spellings; the two here are the ones that leave an instance running. +// +// Both halves are asserted deliberately. A test suite that only checked the +// rejections would be satisfied by a denylist that rejected everything, which +// would break the passthrough that makes a sweep useful — and passthrough is the +// half with no test to protect it before now. + +// TestTier0_SweepRejectsMisspelledBound: `ttl_hours: 4` is a bound the user +// believes they set. The CLI carries a real --ttl so the launch would otherwise +// succeed — without that, a non-zero exit would prove only that the sweep was +// unbounded, not that the key was rejected. +func TestTier0_SweepRejectsMisspelledBound(t *testing.T) { + env := startSpawnSubstrate(t) + file := writeSweepFile(t, `defaults: + on_complete: terminate +params: + - instance_type: c5.large + ttl_hours: 4 +`) + stdout, stderr, code := env.run( + "launch", "misspelled-bound", + "--param-file", file, + "--region", "us-east-1", + "--no-detach", + "--ttl", "1h", + "--wait-for-running=false", + "--wait-for-ssh=false", + "-y", + ) + if code == 0 { + t.Errorf("expected a non-zero exit for ttl_hours:, got 0 — pre-fix this launched with "+ + "PARAM_ttl_hours=4 and no TTL of its own\nstdout:\n%s\nstderr:\n%s", stdout, stderr) + } + if !strings.Contains(stderr, "ttl_hours") { + t.Errorf("the error does not name the offending key\nstderr:\n%s", stderr) + } + if !strings.Contains(stderr, "ttl:") { + t.Errorf("the error does not name the correct spelling, which is the whole point of "+ + "rejecting it\nstderr:\n%s", stderr) + } + env.requireNothingLaunched("a param file with ttl_hours:") +} + +// TestTier0_SweepRejectsHyphenatedKey: `on-complete:` is the hyphen/underscore +// slip. It is worth its own case because it fails twice over — it is not a spawn +// key, and `export PARAM_on-complete="terminate"` is not even a line the shell +// will accept in /etc/profile.d (pkg/launcher/bootstrap.go writes it verbatim). +func TestTier0_SweepRejectsHyphenatedKey(t *testing.T) { + env := startSpawnSubstrate(t) + file := writeSweepFile(t, `defaults: + ttl: 1h +params: + - instance_type: c5.large + on-complete: terminate +`) + stdout, stderr, code := env.run( + "launch", "hyphenated", + "--param-file", file, + "--region", "us-east-1", + "--no-detach", + "--wait-for-running=false", + "--wait-for-ssh=false", + "-y", + ) + if code == 0 { + t.Errorf("expected a non-zero exit for on-complete:, got 0\nstdout:\n%s\nstderr:\n%s", + stdout, stderr) + } + if !strings.Contains(stderr, "on_complete") { + t.Errorf("the error does not suggest the underscore spelling\nstderr:\n%s", stderr) + } + env.requireNothingLaunched("a param file with on-complete:") +} + +// TestTier0_SweepPassesThroughWorkloadParams is the half that must not regress: a +// real workload parameter still becomes a spawn:param:* tag (and thus a PARAM_* +// env var), and an explicit param: escape hatch strips its prefix on the way. +// `budget` is on the reserved list precisely because it is ambiguous, so this also +// pins that the escape hatch works on a name that is otherwise refused. +func TestTier0_SweepPassesThroughWorkloadParams(t *testing.T) { + env := startSpawnSubstrate(t) + file := writeSweepFile(t, `defaults: + on_complete: terminate + ttl: 1h +params: + - instance_type: c5.large + isa: neon + nsteps: 500000 + param:budget: 50 +`) + env.launchForegroundSweep(file) + + tags := env.tagsByInstanceType()["c5.large"] + requireTag(t, "workload param", tags, "spawn:param:isa", "neon") + requireTag(t, "workload param", tags, "spawn:param:nsteps", "500000") + requireTag(t, "escaped reserved name", tags, "spawn:param:budget", "50") + if v, ok := tags["spawn:param:param:budget"]; ok { + t.Errorf("the param: prefix leaked into the tag name: %q", v) + } + // The escape hatch must not touch the real setting it shares a name with. + requireTag(t, "real bound alongside an escaped param", tags, "spawn:ttl", "1h") + if v, ok := tags["spawn:cost-limit"]; ok { + t.Errorf("param:budget was mistaken for a cost limit: spawn:cost-limit=%q", v) + } +} + +// TestTier0_SweepListsPassthroughParams covers option (3) of #526: the PARAM_* +// variables are named at launch, so a key that is silently passing through has +// somewhere to be noticed. +func TestTier0_SweepListsPassthroughParams(t *testing.T) { + env := startSpawnSubstrate(t) + file := writeSweepFile(t, `defaults: + on_complete: terminate + ttl: 1h +params: + - instance_type: c5.large + isa: neon +`) + args := []string{ + "launch", "listed-params", + "--param-file", file, + "--region", "us-east-1", + "--no-detach", + "--wait-for-running=false", + "--wait-for-ssh=false", + "-y", + } + stdout, stderr, code := env.run(args...) + if code != 0 { + t.Fatalf("expected exit 0, got %d\nstdout:\n%s\nstderr:\n%s", code, stdout, stderr) + } + if !strings.Contains(stderr, "PARAM_isa") { + t.Errorf("the sweep header does not name the passthrough parameter it is about to set\n"+ + "stderr:\n%s", stderr) + } +}