From 70fb6540ae5b869e32203224b6c454633ffdadf5 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 22 Sep 2026 14:48:21 +0000 Subject: [PATCH 1/3] docs(compute): propose --user-data parity with upstream OSC MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Measured `koc server create --user-data` against python-openstackclient 8.2.0 and nova 26.3.0 (the Zed floor). The headline finding is a bug, not a missing flag: koc hands the file's raw bytes to gophercloud's servers.CreateOpts.UserData, which base64-encodes them only if they do not already decode as base64. Go's decoder ignores newlines, so an ordinary file of alphanumerics whose length is a multiple of four ("runcmd\nls\n", "hostname\n") takes the pass-through branch. Nova's base64 format checker is lenient (oslo_serialization's b64decode discards non-alphabet characters), so it accepts the value, and the guest is served the decoded garbage. Nothing errors at any layer. Upstream OSC unconditionally b64encodes the file. Also recorded: an empty file is a hard error here and a silent no-op upstream; `server rebuild` is missing --user-data/--no-user-data (nova 2.57 — note OSC's own gate says 2.54, which its help text contradicts and nova's schema rejects); and a bad path is only reported after two API calls. Proposal is docs-only; no behaviour changes in this commit. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01EkWzWSEyrjJ1gd1v5zJNtE --- docs/proposals/user-data-parity.md | 229 +++++++++++++++++++++++++++++ 1 file changed, 229 insertions(+) create mode 100644 docs/proposals/user-data-parity.md diff --git a/docs/proposals/user-data-parity.md b/docs/proposals/user-data-parity.md new file mode 100644 index 0000000..5d8712c --- /dev/null +++ b/docs/proposals/user-data-parity.md @@ -0,0 +1,229 @@ +# `--user-data` parity with upstream `openstack` + +Scope: how a user-data file reaches nova on `server create`, measured against +`python-openstackclient` 8.2.0 and nova 26.3.0 (Zed, the floor from AGENTS.md +→ "Minimum supported cloud"). One finding is a silent data-corruption bug, not a +missing flag. + +## Summary + +| # | Divergence | Severity | Fix | +| --- | --- | --- | --- | +| 1 | koc sends the file **unencoded** when its bytes happen to parse as base64; nova then base64-*decodes* it and the guest boots with garbage user-data. No error anywhere. | **bug** | encode explicitly, stop relying on gophercloud's heuristic | +| 2 | An empty `--user-data` file is a hard error in koc; OSC silently sends no user-data | parity | warn on stderr, omit the field (OSC's wire behaviour, but not OSC's silence) | +| 3 | `server rebuild` has no `--user-data` / `--no-user-data` (nova 2.57) | missing feature | add both, gated at 2.57 | +| 4 | A missing/unreadable file is reported only after two API calls | polish | read the file during validation, before any network I/O | + +1 and 3 are worth doing. 2 and 4 are small and ride along. + +## What upstream does + +`openstackclient/compute/v2/server.py` (OSC 8.2.0): + +- `--user-data `, help `"User data file to serve from the metadata + server"` (`:1381-1385`). One string, no stdin convention — `-` is just a + filename and `open()` fails on it. +- The file is read binary and **unconditionally** base64-encoded + (`:1656-1666`): + + ```python + with open(parsed_args.user_data, 'rb') as fh: + # TODO(stephenfin): SDK should do this for us + user_data = base64.b64encode(fh.read()).decode('utf-8') + ``` + + No content inspection, no size check, no charset assumption. An `OSError` + becomes `Can't open '': `. +- The result is attached only if truthy (`:2041-2042`): `if user_data: + kwargs['user_data'] = user_data`. An **empty file** encodes to `''`, which is + falsy, so OSC drops the key and creates the server with no user-data at all — + silently. The SDK passes the string through verbatim (hence the TODO above), + so what OSC computes is exactly what goes on the wire. + +`server rebuild` has the same reader plus a mutually exclusive +`--no-user-data` (`:3498-3516`, `:3656-3683`), which sends `user_data: null` to +clear it. + +> Upstream bug worth not copying: both rebuild paths gate on +> `supports_microversion(compute_client, '2.54')` while their own help text says +> 2.57. Nova added `user_data` to rebuild at **2.57** +> (`nova/api/openstack/compute/schemas/servers.py:430-440`, `rebuild_v257`); +> 2.54 is `key_name`. Between 2.54 and 2.56 OSC sends a field nova's schema +> rejects with `additionalProperties`. koc should gate on 2.57. + +## What nova accepts + +`nova/api/openstack/compute/schemas/servers.py:212-216` (create): + +```python +'user_data': {'type': 'string', 'format': 'base64', 'maxLength': 65535} +``` + +- Not nullable on create (only the 2.0 variant, `create_v20`, allows `null`); + nullable on rebuild from 2.57, which is what clears it. +- `maxLength` applies to the **encoded** string, so the largest file that can be + sent is 49 149 bytes. +- The `base64` format checker (`nova/api/validation/validators.py:56-67`) calls + `oslo_serialization.base64.decode_as_bytes`, which is plain + `base64.b64decode(...)` with no `validate=True` + (`oslo_serialization/base64.py:57-73`). Python's decoder **discards** + characters outside the base64 alphabet, so nova's validation is lenient: it + accepts far more than a strict decoder would, and never complains about the + payload in divergence 1 below. + +## What koc does today + +`internal/cli/server/server.go:682` registers the flag, `:759-772` reads it, +`:805-807` assigns it to `servers.CreateOpts.UserData`. The struct's field is +`[]byte` and gophercloud decides the encoding for us +(`vendor/.../compute/v2/servers/requests.go:528-536`): + +```go +if opts.UserData != nil { + var userData string + if _, err := base64.StdEncoding.DecodeString(string(opts.UserData)); err != nil { + userData = base64.StdEncoding.EncodeToString(opts.UserData) + } else { + userData = string(opts.UserData) // <- pass-through + } + b["user_data"] = &userData +} +``` + +### Divergence 1 — the heuristic corrupts ordinary files + +"Already base64?" is decided by *trying to decode the file*. Go's base64 decoder +ignores `\r` and `\n`, so any file whose remaining bytes are all in +`[A-Za-z0-9+/]` with a length divisible by four takes the pass-through branch. +Observed against the vendored gophercloud: + +| `--user-data` file | what koc sends | what OSC sends | +| --- | --- | --- | +| `#cloud-config\npackages: [fio]\n` | `I2Nsb3VkLWNvbmZpZwpwYWNrYWdlczogW2Zpb10K` | same ✅ | +| `runcmd\nls\n` | `runcmd\nls\n` ❌ | `cnVuY21kCmxzCg==` | +| `hostname\n` | `hostname\n` ❌ | `aG9zdG5hbWUK` | +| `deadbeef` | `deadbeef` ❌ | `ZGVhZGJlZWY=` | + +Nova accepts the pass-through values (lenient decoder, above), stores them, and +the metadata service serves the *decoded* bytes: `runcmd\nls\n` reaches the guest +as the six bytes of `b64decode("runcmdls")`. Nothing fails. The operator gets an +ACTIVE instance that silently did not run its cloud-init, and `koc server show +--user-data` decodes the same garbage, so the client agrees with itself. + +Files with `#`, `:`, `-`, `=` or a space are safe, which covers most real +cloud-configs and is why this has not bitten yet. Short scripts and generated +one-liners are not safe. The failure is silent, data-dependent and only +reproducible with the exact file, which is the worst combination to debug. + +Note also that the heuristic's *intended* case is wrong for a drop-in +replacement: a file that is already base64 is passed through by koc and +double-encoded by OSC. The file's bytes are the user-data — that is the contract, +and koc should not second-guess it. + +### Divergence 2 — empty file + +`readUserData` (`:767-770`) rejects an empty file. OSC drops the key and +proceeds. An automation template that renders empty when there is nothing to +configure works under `openstack` and fails under `koc` — a real drop-in +regression, even though the resulting instance is identical either way. + +### Divergence 3 — rebuild + +`newServerRebuildCommand` (`internal/cli/server/actions.go:506-531`) registers +only `--image` and `--name`. gophercloud's `RebuildOpts` (`requests.go:759-785`) +has no `UserData` field at all — it still carries `Personality`, which nova +removed at 2.57 — so this needs a koc-owned builder, the same shape as +`serverCreateOptsExt`. + +### Divergence 4 — ordering + +`runServerCreate` validates, resolves the flavor (API call), parses properties, +builds scheduler hints (possibly another API call), and only then reads the +user-data file (`:805`). A typo'd path costs two round-trips before the error. + +## Proposal + +### P1 — encode explicitly (fixes 1, 2, 4) + +`readUserData` returns the base64 text rather than the raw bytes: + +```go +// readUserData loads the --user-data file and returns it base64-encoded, the +// way nova's schema wants it. Encoding here rather than leaving it to +// gophercloud's servers.CreateOpts is deliberate: that code base64-encodes the +// bytes only if they do not already decode as base64, and Go's decoder ignores +// newlines, so an ordinary file of alphanumerics whose length is a multiple of +// four (e.g. "runcmd\nls\n") takes the pass-through branch and reaches the +// guest as the decoded garbage instead. Handing gophercloud text that is +// already valid base64 pins the pass-through branch, so what is sent is exactly +// what upstream OSC sends (openstackclient/compute/v2/server.py, which +// unconditionally b64encodes the file). +``` + +- Empty file: warn on stderr — `warning: --user-data file %q is empty; creating + the server without user data` — and leave `opts.UserData` nil so the key is + omitted, matching OSC's request byte for byte. Precedent for the warning is + the `--disk-overcommit` one at `actions.go:260`; OSC's silence here is not + worth copying. +- Move the read into `validateServerCreate` (or immediately after it) so a bad + path fails before the first API call. +- Add a bullet to AGENTS.md → "gophercloud v2 gotchas": `CreateOpts.UserData` + guesses at the encoding, so pre-encode. + +The fix depends on gophercloud keeping the pass-through branch. That is pinned +by a test asserting the exact `user_data` value on the wire, so a vendor bump +that changed it would fail loudly rather than start double-encoding. If that +ever happens, the alternative is to set `user_data` from `serverCreateOptsExt` +and leave `CreateOpts.UserData` unset, which removes the dependency entirely. + +### P2 — `server rebuild --user-data` / `--no-user-data` + +- Mutually exclusive (cobra's `MarkFlagsMutuallyExclusive`), matching OSC's + argparse group. +- Gate both on `computeSupportsMicroversion(client, "2.57")` — nova's number, + not OSC's 2.54 — and say `(nova 2.57 or later)` in the flag help, the way + `--host` states 2.74. Zed's cap is 2.93, so this reaches the whole fleet. +- `--user-data` reuses P1's reader. `--no-user-data` sends JSON `null`. +- gophercloud's `RebuildOpts` cannot express the field, so add a + `serverRebuildOptsExt` wrapping `servers.RebuildOptsBuilder` and splicing + `user_data` into the body — same pattern and same comment style as + `serverCreateOptsExt` (`create_blockdevice.go:219-231`). + +Command-surface unchanged (no new leaf), so `docs/coverage.md` needs no edit; +this is flag-level parity, which that document does not count. + +### P3 — optional, not recommended on its own + +A client-side size check against nova's 49 149-byte raw ceiling would turn an +opaque 400 into a clear message. It is also a place to be wrong: a cloud with a +patched `maxLength` would be rejected by koc for a request it would accept. +If it lands at all it should decorate the error from a failed create rather than +pre-empt the request. + +## Tests + +Against the existing seam (`runServerCreate`, `runServerRebuild`) and the mock +endpoint, per AGENTS.md → "Testing": + +1. **Regression for divergence 1** — table of the four files above, asserting + the exact `user_data` string in the request body. `runcmd\nls\n` must arrive + as `cnVuY21kCmxzCg==`. This is the test that pins gophercloud's behaviour. +2. Binary user-data (a gzip blob) round-trips to the same base64 Python's + `b64encode` produces. +3. Empty file: no `user_data` key in the body, warning on stderr, exit 0. +4. Missing file: error mentions the path, and **no request reaches the mock**. +5. Rebuild: `--user-data` sends the encoded string; `--no-user-data` sends JSON + `null`; both together are rejected by cobra; below 2.57 both fail with the + microversion message and send nothing. + +`create_blockdevice_test.go:165` already asserts a correct `user_data` value on +the happy path — extend that table rather than adding a parallel one. + +## Not proposed + +- `-` for stdin. OSC has no such convention and a file path is unambiguous. +- Decoding or validating the file's content (cloud-config lint, MIME + multipart). The file's bytes are the user-data; nova and cloud-init own the + rest. +- `server show --user-data` (koc-native, decodes what OSC prints raw) stays as + it is. From 5756d22ef963b6ac0ada738a52cc4a4862398a94 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 22 Sep 2026 15:03:14 +0000 Subject: [PATCH 2/3] fix(server): base64-encode --user-data instead of letting the SDK guess server create handed the file's raw bytes to gophercloud's servers.CreateOpts.UserData, which base64-encodes them only when they do not already decode as base64. Go's decoder ignores newlines, so an ordinary file whose remaining bytes all fall in the base64 alphabet with a length divisible by four -- "runcmd\nls\n", "hostname\n", "deadbeef" -- took the pass-through branch and was sent verbatim. Nothing caught it downstream. Nova's "format": "base64" check is oslo_serialization's b64decode, which discards characters outside the alphabet rather than rejecting them, so the request succeeded and the guest was served the decoded garbage instead of the file. The instance reached ACTIVE having run nothing, and `server show --user-data` decoded the same garbage, so the client agreed with itself. Encode in readUserData instead. Handing gophercloud text that is already valid base64 pins the pass-through branch, so the request body now matches upstream OSC, which encodes unconditionally. The regression test asserts the exact user_data string on the wire for each payload, which also pins gophercloud's behaviour across a vendor bump. Two smaller divergences from OSC go with it. An empty file is no longer rejected: upstream's `if user_data:` drops the field from the request, so koc sends the same body -- but warns on stderr rather than doing it silently, since a template that rendered to nothing is usually a mistake. And the file is read before the flavor and scheduler-hint lookups, so a mistyped path costs no round-trip. Affected releases are v0.28.0 through v0.32.1; README "User data" carries the pre-encoding workaround for them. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01EkWzWSEyrjJ1gd1v5zJNtE --- AGENTS.md | 10 + README.md | 33 ++++ docs/proposals/user-data-parity.md | 5 + .../cli/server/create_blockdevice_test.go | 25 +-- internal/cli/server/create_hints_test.go | 9 +- internal/cli/server/create_userdata_test.go | 178 ++++++++++++++++++ internal/cli/server/server.go | 72 +++++-- internal/cli/server/server_more_test.go | 8 +- 8 files changed, 297 insertions(+), 43 deletions(-) create mode 100644 internal/cli/server/create_userdata_test.go diff --git a/AGENTS.md b/AGENTS.md index f271dd5..d1626e0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -392,6 +392,16 @@ separate entry. floating-IP actions, glance activate/deactivate). Fall back to raw `ServiceClient.Get/Post/Put/Delete` with the correct microversion, **isolated behind a small helper**, and note it in a comment so it's easy to replace. +- **`servers.CreateOpts.UserData` guesses at the encoding.** It base64-encodes + the bytes only when they do not already decode as base64, and Go's decoder + ignores newlines — so an ordinary file whose remaining bytes are all in the + base64 alphabet with a length divisible by four (`runcmd\nls\n`, + `hostname\n`) is sent verbatim, nova's lenient `format: base64` check accepts + it, and the guest is served the decoded garbage. **Encode before handing the + value over** (`readUserData` in `server/server.go`), which pins the + pass-through branch and matches upstream OSC. `RebuildOpts` has no `UserData` + field at all — it still models the personality files nova removed at 2.57 — so + rebuild splices the field in via `serverRebuildOptsExt`. - **Provision-state / async transitions** (ironic): after deploy/manage/inspect, `--wait` polls `provision_state` keyed off `target_provision_state` clearing — see `baremetal/node_provision.go`. diff --git a/README.md b/README.md index d4369f8..533ab0f 100644 --- a/README.md +++ b/README.md @@ -543,6 +543,39 @@ Nova is the exception worth knowing: `server list --name` is a server-side `volume list`, `network list`, `port list` and `subnet list` are exact-match with no `--name-contains` yet — pipe through `grep` there. +### User data (`--user-data`) + +`koc server create --user-data ` injects a cloud-init payload. The +**file's bytes are the payload**: koc base64-encodes them for nova and does not +inspect or transform the content, matching `openstack`. A path is the only +accepted form — `-` is a filename, not stdin. `koc server show +--user-data` prints the payload back, decoded. + +> **Releases v0.28.0 through v0.32.1 can corrupt the payload.** Those versions +> left the encoding to the SDK, which sent the file unencoded whenever its bytes +> happened to parse as base64 — a file with no `#`, `:`, `-`, `=` or space whose +> length is a multiple of four (`runcmd\nls\n`, `hostname\n`) would take that +> branch. Nova accepts it, so nothing fails: the instance boots ACTIVE having +> run whatever the payload decoded to. On those versions, encode the file +> yourself and pass the encoded text, which the affected code passes through +> unchanged: +> +> ```sh +> base64 -w0 cloud-init.yaml > cloud-init.b64 # a file +> printf '%s' "$USER_DATA" | base64 -w0 > cloud-init.b64 # a shell variable +> koc server create --user-data cloud-init.b64 … # v0.28.0 - v0.32.1 only +> ``` +> +> `base64 -w0` matters: without it GNU coreutils wraps at 76 columns, and while +> both nova and the affected code tolerate the newlines, the unwrapped form is +> what the encoded file is meant to be. On macOS the flag is `-b0`, or pipe +> through `tr -d '\n'`. +> +> **Remove the workaround when you upgrade.** From v0.33.0 the file is encoded +> unconditionally, so a pre-encoded file is encoded a second time and the guest +> receives the base64 text instead of the payload. After upgrading, pass the +> plain file. + ### Microversions Each service client sets its own microversion; defaults negotiate the latest the diff --git a/docs/proposals/user-data-parity.md b/docs/proposals/user-data-parity.md index 5d8712c..3d730b6 100644 --- a/docs/proposals/user-data-parity.md +++ b/docs/proposals/user-data-parity.md @@ -1,5 +1,10 @@ # `--user-data` parity with upstream `openstack` +**Status: P1 implemented.** The encoding fix, the empty-file handling and the +early read landed with the tests in "Tests" below. The analysis is kept as the +record of why the behaviour is what it is — the workaround for the affected +releases (v0.28.0 - v0.32.1) is in README "User data (`--user-data`)". + Scope: how a user-data file reaches nova on `server create`, measured against `python-openstackclient` 8.2.0 and nova 26.3.0 (Zed, the floor from AGENTS.md → "Minimum supported cloud"). One finding is a silent data-corruption bug, not a diff --git a/internal/cli/server/create_blockdevice_test.go b/internal/cli/server/create_blockdevice_test.go index 10af123..727708b 100644 --- a/internal/cli/server/create_blockdevice_test.go +++ b/internal/cli/server/create_blockdevice_test.go @@ -3,6 +3,7 @@ package server import ( "bytes" "context" + "io" "net/http" "os" "path/filepath" @@ -196,7 +197,7 @@ func TestRunServerCreate_PlacementBlockDevicesAndUserData(t *testing.T) { } o := &output.Options{Format: output.FormatTable} var buf bytes.Buffer - if err := runServerCreate(context.Background(), computeClient(fakeServer, "2.93"), o, "io-writer", f, &buf); err != nil { + if err := runServerCreate(context.Background(), computeClient(fakeServer, "2.93"), o, "io-writer", f, &buf, io.Discard); err != nil { t.Fatalf("runServerCreate: %v", err) } @@ -287,7 +288,7 @@ func TestRunServerCreate_HostAndBootIndexZero(t *testing.T) { } o := &output.Options{Format: output.FormatTable} var buf bytes.Buffer - if err := runServerCreate(context.Background(), computeClient(fakeServer, "2.93"), o, "pinned", f, &buf); err != nil { + if err := runServerCreate(context.Background(), computeClient(fakeServer, "2.93"), o, "pinned", f, &buf, io.Discard); err != nil { t.Fatalf("runServerCreate: %v", err) } if got := gotServer["host"]; got != "compute-7" { @@ -325,24 +326,6 @@ func TestValidateServerCreate_HostConflict(t *testing.T) { } } -// TestReadUserData covers the two ways the flag can be wrong. An empty file is -// rejected rather than sent: nova accepts it and the guest then boots with no -// cloud-init payload at all, which is the failure this flag exists to avoid. -func TestReadUserData(t *testing.T) { - dir := t.TempDir() - empty := filepath.Join(dir, "empty") - if err := os.WriteFile(empty, nil, 0o600); err != nil { - t.Fatalf("writing fixture: %v", err) - } - if _, err := readUserData(empty); err == nil || !strings.Contains(err.Error(), "is empty") { - t.Errorf("readUserData(empty) err = %v, want the empty-file rejection", err) - } - if _, err := readUserData(filepath.Join(dir, "absent")); err == nil || - !strings.Contains(err.Error(), "reading --user-data") { - t.Errorf("readUserData(absent) err = %v, want a read failure", err) - } -} - // TestRunServerCreate_Wait asserts --wait polls until nova reports ACTIVE and // renders the settled status, and that it fails the command when the build ends // in ERROR — the case a caller without --wait discovers much later. @@ -386,7 +369,7 @@ func TestRunServerCreate_Wait(t *testing.T) { f := &serverCreateFlags{flavor: "m1.small", image: "img", wait: true, waitTimeout: 5 * time.Second} o := &output.Options{Format: output.FormatTable} var buf bytes.Buffer - err := runServerCreate(context.Background(), computeClient(fakeServer, "2.93"), o, "waited", f, &buf) + err := runServerCreate(context.Background(), computeClient(fakeServer, "2.93"), o, "waited", f, &buf, io.Discard) if tc.wantErr != "" { if err == nil || !strings.Contains(err.Error(), tc.wantErr) { t.Fatalf("err = %v, want containing %q", err, tc.wantErr) diff --git a/internal/cli/server/create_hints_test.go b/internal/cli/server/create_hints_test.go index 6dddbab..8ab3b33 100644 --- a/internal/cli/server/create_hints_test.go +++ b/internal/cli/server/create_hints_test.go @@ -3,6 +3,7 @@ package server import ( "bytes" "context" + "io" "net/http" "reflect" "strings" @@ -129,7 +130,7 @@ func TestRunServerCreate_HintsRideBesideTheServerObject(t *testing.T) { } o := &output.Options{Format: output.FormatTable} var buf bytes.Buffer - if err := runServerCreate(context.Background(), computeClient(fakeServer, "2.93"), o, "db-2", f, &buf); err != nil { + if err := runServerCreate(context.Background(), computeClient(fakeServer, "2.93"), o, "db-2", f, &buf, io.Discard); err != nil { t.Fatalf("runServerCreate: %v", err) } @@ -171,7 +172,7 @@ func TestRunServerCreate_MalformedHintFailsBeforeRequest(t *testing.T) { f := &serverCreateFlags{image: "img-uuid", flavor: "m1.small", hints: []string{"group"}} var buf bytes.Buffer err := runServerCreate(context.Background(), computeClient(fakeServer, "2.93"), - &output.Options{Format: output.FormatTable}, "db-2", f, &buf) + &output.Options{Format: output.FormatTable}, "db-2", f, &buf, io.Discard) if err == nil || !strings.Contains(err.Error(), "--hint") { t.Fatalf("error = %v, want it to name --hint", err) } @@ -270,7 +271,7 @@ func TestRunServerCreate_ServerGroupResolvesAndWins(t *testing.T) { } o := &output.Options{Format: output.FormatTable} var buf bytes.Buffer - if err := runServerCreate(context.Background(), computeClient(fakeServer, "2.93"), o, "web-9", f, &buf); err != nil { + if err := runServerCreate(context.Background(), computeClient(fakeServer, "2.93"), o, "web-9", f, &buf, io.Discard); err != nil { t.Fatalf("runServerCreate: %v", err) } @@ -317,7 +318,7 @@ func TestRunServerCreate_ServerGroupByIDNeedsNoLookup(t *testing.T) { f := &serverCreateFlags{image: "img-uuid", flavor: "m1.small", serverGroup: serverGroupID} var buf bytes.Buffer if err := runServerCreate(context.Background(), computeClient(fakeServer, "2.93"), - &output.Options{Format: output.FormatTable}, "web-9", f, &buf); err != nil { + &output.Options{Format: output.FormatTable}, "web-9", f, &buf, io.Discard); err != nil { t.Fatalf("runServerCreate: %v", err) } if listed { diff --git a/internal/cli/server/create_userdata_test.go b/internal/cli/server/create_userdata_test.go new file mode 100644 index 0000000..4476757 --- /dev/null +++ b/internal/cli/server/create_userdata_test.go @@ -0,0 +1,178 @@ +package server + +import ( + "bytes" + "context" + "io" + "net/http" + "os" + "path/filepath" + "strings" + "testing" + + th "github.com/gophercloud/gophercloud/v2/testhelper" + + "github.com/ftarasenko/go-openstackclient/internal/output" +) + +// userDataFixture writes body to a file under t.TempDir() and returns its path, +// so every case below exercises the real read rather than a pre-loaded buffer. +func userDataFixture(t *testing.T, name string, body []byte) string { + t.Helper() + path := filepath.Join(t.TempDir(), name) + if err := os.WriteFile(path, body, 0o600); err != nil { + t.Fatalf("writing user-data fixture %s: %v", name, err) + } + return path +} + +// userDataCases are the payloads that decide whether the file survives the trip. +// The last three are the regression: each is valid base64 once Go's decoder +// drops the newlines, so handing the raw bytes to gophercloud's +// servers.CreateOpts sends them verbatim, nova's lenient base64 check accepts +// them, and the guest is served the decoded garbage. Every "want" here is what +// Python's base64.b64encode produces, which is what upstream OSC sends. +var userDataCases = []struct { + name string + body []byte + want string +}{ + {"cloud-config", []byte("#cloud-config\npackages: [fio]\n"), "I2Nsb3VkLWNvbmZpZwpwYWNrYWdlczogW2Zpb10K"}, + {"shell script", []byte("#!/bin/sh\necho hi\n"), "IyEvYmluL3NoCmVjaG8gaGkK"}, + {"alphanumeric, length divisible by four", []byte("runcmd\nls\n"), "cnVuY21kCmxzCg=="}, + {"single alphanumeric line", []byte("hostname\n"), "aG9zdG5hbWUK"}, + {"no newline at all", []byte("deadbeef"), "ZGVhZGJlZWY="}, + {"binary (gzip magic)", []byte{0x1f, 0x8b, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00}, "H4sIAAAAAAA="}, + {"already base64", []byte("IyEvYmluL3NoCg=="), "SXlFdlltbHVMM05vQ2c9PQ=="}, +} + +// TestReadUserData asserts the file is loaded from disk and encoded the way +// nova's schema wants it, including for the payloads that used to be passed +// through unencoded. +func TestReadUserData(t *testing.T) { + for _, tc := range userDataCases { + t.Run(tc.name, func(t *testing.T) { + got, err := readUserData(userDataFixture(t, "user-data", tc.body)) + if err != nil { + t.Fatalf("readUserData: %v", err) + } + if got != tc.want { + t.Errorf("readUserData = %q, want %q", got, tc.want) + } + }) + } + + // An empty file is not an error here: it encodes to "", and the caller + // decides what that means (create warns and omits, rebuild refuses). + if got, err := readUserData(userDataFixture(t, "empty", nil)); err != nil || got != "" { + t.Errorf("readUserData(empty) = %q, %v; want \"\", nil", got, err) + } + if _, err := readUserData(filepath.Join(t.TempDir(), "absent")); err == nil || + !strings.Contains(err.Error(), "reading --user-data") { + t.Errorf("readUserData(absent) err = %v, want a read failure naming the flag", err) + } +} + +// newUserDataCreateServer stands up a mock nova that records the created +// server's body, so a test can assert the exact user_data string on the wire. +func newUserDataCreateServer(t *testing.T, got *map[string]any) th.FakeServer { + t.Helper() + fakeServer := th.SetupHTTP() + t.Cleanup(fakeServer.Teardown) + fakeServer.Mux.HandleFunc("/flavors/detail", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"flavors":[{"id":"2","name":"m1.small"}]}`)) + }) + fakeServer.Mux.HandleFunc("/servers", func(w http.ResponseWriter, r *http.Request) { + *got, _ = decodeBody(t, r)["server"].(map[string]any) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusAccepted) + _, _ = w.Write([]byte(`{"server":{"id":"new-id","adminPass":"pw"}}`)) + }) + fakeServer.Mux.HandleFunc("/servers/new-id", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"server":{"id":"new-id","name":"boot","status":"ACTIVE"}}`)) + }) + return fakeServer +} + +// TestRunServerCreate_UserDataEncoding is the regression test for the +// pass-through bug: every payload must reach nova base64-encoded, whatever its +// bytes happen to look like. It is driven from a real file so the whole path — +// open, read, encode, serialise — is under test. +func TestRunServerCreate_UserDataEncoding(t *testing.T) { + for _, tc := range userDataCases { + t.Run(tc.name, func(t *testing.T) { + var gotServer map[string]any + fakeServer := newUserDataCreateServer(t, &gotServer) + + f := &serverCreateFlags{ + image: "img-uuid", + flavor: "m1.small", + userData: userDataFixture(t, "user-data", tc.body), + } + var buf, warn bytes.Buffer + o := &output.Options{Format: output.FormatTable} + if err := runServerCreate(context.Background(), computeClient(fakeServer, "2.93"), + o, "boot", f, &buf, &warn); err != nil { + t.Fatalf("runServerCreate: %v", err) + } + if got := gotServer["user_data"]; got != tc.want { + t.Errorf("user_data = %v, want %v", got, tc.want) + } + if warn.Len() != 0 { + t.Errorf("unexpected warning: %q", warn.String()) + } + }) + } +} + +// TestRunServerCreate_UserDataEmptyFile pins the upstream behaviour for an empty +// file: the field is left out of the request rather than sent as "", the create +// still succeeds, and koc says so on stderr instead of doing it silently the way +// OSC does. +func TestRunServerCreate_UserDataEmptyFile(t *testing.T) { + var gotServer map[string]any + fakeServer := newUserDataCreateServer(t, &gotServer) + + empty := userDataFixture(t, "empty", nil) + f := &serverCreateFlags{image: "img-uuid", flavor: "m1.small", userData: empty} + var buf, warn bytes.Buffer + o := &output.Options{Format: output.FormatTable} + if err := runServerCreate(context.Background(), computeClient(fakeServer, "2.93"), + o, "boot", f, &buf, &warn); err != nil { + t.Fatalf("runServerCreate: %v", err) + } + if _, present := gotServer["user_data"]; present { + t.Errorf("an empty --user-data file must be left out of the body, got %v", gotServer["user_data"]) + } + if !strings.Contains(warn.String(), "is empty") || !strings.Contains(warn.String(), empty) { + t.Errorf("warning = %q, want it to name the empty file", warn.String()) + } +} + +// TestRunServerCreate_UserDataMissingFile asserts the typo is caught before any +// request goes out — the flavor lookup and the scheduler-hint resolution used to +// happen first, so a wrong path cost two round-trips. +func TestRunServerCreate_UserDataMissingFile(t *testing.T) { + fakeServer := th.SetupHTTP() + defer fakeServer.Teardown() + + var called bool + fakeServer.Mux.HandleFunc("/", func(w http.ResponseWriter, _ *http.Request) { + called = true + w.WriteHeader(http.StatusInternalServerError) + }) + + absent := filepath.Join(t.TempDir(), "absent") + f := &serverCreateFlags{image: "img-uuid", flavor: "m1.small", userData: absent} + var buf bytes.Buffer + err := runServerCreate(context.Background(), computeClient(fakeServer, "2.93"), + &output.Options{Format: output.FormatTable}, "boot", f, &buf, io.Discard) + if err == nil || !strings.Contains(err.Error(), absent) { + t.Fatalf("error = %v, want it to name %s", err, absent) + } + if called { + t.Error("a missing --user-data file must fail before anything is sent") + } +} diff --git a/internal/cli/server/server.go b/internal/cli/server/server.go index 9cae30e..4cea3b0 100644 --- a/internal/cli/server/server.go +++ b/internal/cli/server/server.go @@ -641,7 +641,7 @@ func newServerCreateCommand(a *auth.Options, o *output.Options) *cobra.Command { if err := resolveServerCreateRefs(ctx, s.auth, f); err != nil { return err } - return runServerCreate(ctx, s.client, o, args[0], f, cmd.OutOrStdout()) + return runServerCreate(ctx, s.client, o, args[0], f, cmd.OutOrStdout(), cmd.ErrOrStderr()) }, } fl := cmd.Flags() @@ -756,25 +756,71 @@ func serverCreateBlockDevices(f *serverCreateFlags) []map[string]any { return append(bdms, f.bdmSpecs...) } -// readUserData loads the --user-data file. gophercloud base64-encodes the bytes -// for nova unless they already decode as base64, so a pre-encoded file is -// passed through rather than double-encoded. -func readUserData(path string) ([]byte, error) { +// readUserData loads the --user-data file and returns its bytes base64-encoded, +// which is the form nova's schema wants ("format": "base64", maxLength 65535 in +// nova/api/openstack/compute/schemas/servers.py). +// +// Encoding here rather than handing gophercloud the raw bytes is deliberate. +// servers.CreateOpts base64-encodes UserData only when it does not already +// decode as base64, and Go's decoder ignores newlines, so an ordinary file +// whose remaining bytes all fall in the base64 alphabet with a length divisible +// by four ("runcmd\nls\n", "hostname\n") takes the pass-through branch and is +// sent verbatim. Nova does not catch that either — its base64 format checker is +// oslo_serialization's b64decode, which discards characters outside the +// alphabet instead of rejecting them — so the request succeeds and the guest is +// served the decoded garbage rather than the file. Text that is already valid +// base64 pins the pass-through branch, so what goes on the wire matches +// upstream OSC, which encodes unconditionally +// (openstackclient/compute/v2/server.py). +// +// An empty file encodes to "", which the caller reports and drops from the +// request: upstream's `if user_data:` is false for that value, so OSC leaves +// the field out entirely. +func readUserData(path string) (string, error) { //nolint:gosec // G304: operator-supplied user-data path, the point of the flag data, err := os.ReadFile(path) if err != nil { - return nil, fmt.Errorf("reading --user-data %q: %w", path, err) + return "", fmt.Errorf("reading --user-data %q: %w", path, err) } - if len(data) == 0 { - return nil, fmt.Errorf("--user-data file %q is empty", path) + return base64.StdEncoding.EncodeToString(data), nil +} + +// serverCreateUserData resolves --user-data to the base64 payload the request +// carries, or "" when the flag is unset or names an empty file. An empty file +// is not an error — it produces the same server upstream OSC would, which drops +// the field rather than sending it — but it is announced, because a template +// that rendered to nothing is more often a mistake than an intent. +func serverCreateUserData(f *serverCreateFlags, warn io.Writer) (string, error) { + if f.userData == "" { + return "", nil + } + userData, err := readUserData(f.userData) + if err != nil { + return "", err } - return data, nil + if userData == "" { + if _, err := fmt.Fprintf(warn, + "warning: --user-data file %q is empty; creating the server without user data\n", + f.userData); err != nil { + return "", err + } + } + return userData, nil } -func runServerCreate(ctx context.Context, client *gophercloud.ServiceClient, o *output.Options, name string, f *serverCreateFlags, w io.Writer) error { +func runServerCreate(ctx context.Context, client *gophercloud.ServiceClient, o *output.Options, name string, + f *serverCreateFlags, w, warn io.Writer, +) error { if err := validateServerCreate(f); err != nil { return err } + // The user-data file is read before anything is sent. A bad path is an + // operator typo, and reporting it only after the flavor and scheduler-hint + // lookups have gone out makes the typo cost two round-trips. + userData, err := serverCreateUserData(f, warn) + if err != nil { + return err + } flavorRef, err := resolveFlavorRef(ctx, client, f.flavor) if err != nil { return err @@ -802,10 +848,8 @@ func runServerCreate(ctx context.Context, client *gophercloud.ServiceClient, o * AvailabilityZone: f.availabilityZone, HypervisorHostname: f.hypervisorHostname, } - if f.userData != "" { - if opts.UserData, err = readUserData(f.userData); err != nil { - return err - } + if userData != "" { + opts.UserData = []byte(userData) } if f.configDriveSet { cd := f.configDrive diff --git a/internal/cli/server/server_more_test.go b/internal/cli/server/server_more_test.go index 0667ec9..629b83e 100644 --- a/internal/cli/server/server_more_test.go +++ b/internal/cli/server/server_more_test.go @@ -265,7 +265,7 @@ func TestRunServerCreate_RequestBodyAndOutput(t *testing.T) { f := &serverCreateFlags{image: "img-uuid", flavor: "m1.small"} var buf bytes.Buffer - if err := runServerCreate(context.Background(), client, o, "web-3", f, &buf); err != nil { + if err := runServerCreate(context.Background(), client, o, "web-3", f, &buf, io.Discard); err != nil { t.Fatalf("runServerCreate: %v", err) } if gotMethod != http.MethodPost { @@ -294,7 +294,7 @@ func TestRunServerCreate_FlavorRequired(t *testing.T) { client := computeClient(fakeServer, "2.79") o := &output.Options{Format: output.FormatTable} var buf bytes.Buffer - err := runServerCreate(context.Background(), client, o, "web-3", &serverCreateFlags{}, &buf) + err := runServerCreate(context.Background(), client, o, "web-3", &serverCreateFlags{}, &buf, io.Discard) if err == nil || !strings.Contains(err.Error(), "--flavor is required") { t.Fatalf("err = %v, want --flavor is required", err) } @@ -338,7 +338,7 @@ func TestRunServerCreate_BootFromVolume(t *testing.T) { } var buf bytes.Buffer - if err := runServerCreate(context.Background(), client, o, "koc", f, &buf); err != nil { + if err := runServerCreate(context.Background(), client, o, "koc", f, &buf, io.Discard); err != nil { t.Fatalf("runServerCreate: %v", err) } if v, ok := gotServer["imageRef"]; ok && v != "" { @@ -382,7 +382,7 @@ func TestRunServerCreate_BootFromVolumeValidation(t *testing.T) { for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { var buf bytes.Buffer - err := runServerCreate(context.Background(), nil, o, "koc", tc.f, &buf) + err := runServerCreate(context.Background(), nil, o, "koc", tc.f, &buf, io.Discard) if err == nil || !strings.Contains(err.Error(), tc.want) { t.Fatalf("err = %v, want containing %q", err, tc.want) } From 9ebf211c72fb34dacf63d15e1d5157cec33765f3 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 22 Sep 2026 15:03:53 +0000 Subject: [PATCH 3/3] feat(server): add rebuild --user-data and --no-user-data Nova added user_data to the rebuild request at microversion 2.57, as a nullable string: a base64 payload replaces the server's user data, JSON null clears it (rebuild_v257 in nova/api/openstack/compute/schemas/servers.py). koc rebuild carried only --image and --name, so re-provisioning a guest meant deleting and recreating it to change the cloud-init payload. The gate is nova's 2.57, not upstream OSC's 2.54. OSC checks 2.54 while its own help text says 2.57; 2.54 is the microversion that added key_name to rebuild, and nova's schema rejects user_data below 2.57 as an unexpected property. Zed's nova caps at 2.93, so the flags reach the whole supported fleet. gophercloud's servers.RebuildOpts has no field for this -- it predates the change and still models the personality files 2.57 removed -- so serverRebuildOptsExt splices the field into the body, the way serverCreateOptsExt splices nova 2.74's host into a create. The file is read through the same encoder as create, so the payloads that used to be passed through unencoded are correct here from the start. An empty file is refused rather than sent as "": clearing is what --no-user-data spells. Flag surface only; no new leaf command, so docs/coverage.md is unchanged. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01EkWzWSEyrjJ1gd1v5zJNtE --- README.md | 12 +- docs/proposals/user-data-parity.md | 9 +- internal/cli/server/actions.go | 117 ++++++++++++-- internal/cli/server/rebuild_userdata_test.go | 162 +++++++++++++++++++ internal/cli/server/server_more_test.go | 6 +- 5 files changed, 286 insertions(+), 20 deletions(-) create mode 100644 internal/cli/server/rebuild_userdata_test.go diff --git a/README.md b/README.md index 533ab0f..7c4c501 100644 --- a/README.md +++ b/README.md @@ -545,11 +545,13 @@ no `--name-contains` yet — pipe through `grep` there. ### User data (`--user-data`) -`koc server create --user-data ` injects a cloud-init payload. The -**file's bytes are the payload**: koc base64-encodes them for nova and does not -inspect or transform the content, matching `openstack`. A path is the only -accepted form — `-` is a filename, not stdin. `koc server show ---user-data` prints the payload back, decoded. +`koc server create --user-data ` injects a cloud-init payload, and +`koc server rebuild --user-data ` / `--no-user-data` replace or clear the +one a server already has (nova microversion 2.57 or later, so every supported +cloud). In all three the **file's bytes are the payload**: koc base64-encodes +them for nova and does not inspect or transform the content, matching +`openstack`. A path is the only accepted form — `-` is a filename, not stdin. +`koc server show --user-data` prints the payload back, decoded. > **Releases v0.28.0 through v0.32.1 can corrupt the payload.** Those versions > left the encoding to the SDK, which sent the file unencoded whenever its bytes diff --git a/docs/proposals/user-data-parity.md b/docs/proposals/user-data-parity.md index 3d730b6..9a70fd4 100644 --- a/docs/proposals/user-data-parity.md +++ b/docs/proposals/user-data-parity.md @@ -1,9 +1,10 @@ # `--user-data` parity with upstream `openstack` -**Status: P1 implemented.** The encoding fix, the empty-file handling and the -early read landed with the tests in "Tests" below. The analysis is kept as the -record of why the behaviour is what it is — the workaround for the affected -releases (v0.28.0 - v0.32.1) is in README "User data (`--user-data`)". +**Status: P1 and P2 implemented.** The encoding fix, the empty-file handling, +the early read and the two `server rebuild` flags landed with the tests in +"Tests" below; P3 was not taken. The analysis is kept as the record of why the +behaviour is what it is — the workaround for the affected releases (v0.28.0 - +v0.32.1) is in README "User data (`--user-data`)". Scope: how a user-data file reaches nova on `server create`, measured against `python-openstackclient` 8.2.0 and nova 26.3.0 (Zed, the floor from AGENTS.md diff --git a/internal/cli/server/actions.go b/internal/cli/server/actions.go index 105c020..c55821a 100644 --- a/internal/cli/server/actions.go +++ b/internal/cli/server/actions.go @@ -504,8 +504,31 @@ func runServerResize(ctx context.Context, client *gophercloud.ServiceClient, ref // rebuild ---------------------------------------------------------------------- +// rebuildUserDataMicroversion is the nova microversion that added user_data to +// the rebuild request, as a nullable string: a base64 payload replaces the +// server's user data, JSON null clears it (rebuild_v257 in +// nova/api/openstack/compute/schemas/servers.py). Zed's nova caps at 2.93, so +// this reaches the whole supported fleet. +// +// Upstream OSC gates its own --user-data/--no-user-data on 2.54 while its help +// text says 2.57; 2.54 is the microversion that added key_name, and nova's +// schema rejects user_data below 2.57 as an unexpected property. koc follows +// nova. +const rebuildUserDataMicroversion = "2.57" + +type serverRebuildFlags struct { + image string + name string + + // userData is a path, like "server create --user-data"; noUserData is + // upstream's spelling of "clear whatever the server has". cobra keeps them + // mutually exclusive, mirroring OSC's argparse group. + userData string + noUserData bool +} + func newServerRebuildCommand(a *auth.Options, o *output.Options) *cobra.Command { - var image, name string + f := &serverRebuildFlags{} cmd := &cobra.Command{ Use: "rebuild ", Short: "Rebuild a server from an image", @@ -514,7 +537,7 @@ func newServerRebuildCommand(a *auth.Options, o *output.Options) *cobra.Command if err := o.Validate(); err != nil { return err } - if image == "" { + if f.image == "" { return fmt.Errorf("--image is required") } ctx := cmd.Context() @@ -522,25 +545,101 @@ func newServerRebuildCommand(a *auth.Options, o *output.Options) *cobra.Command if err != nil { return err } - return runServerRebuild(ctx, client, o, args[0], image, name, cmd.OutOrStdout()) + return runServerRebuild(ctx, client, o, args[0], f, cmd.OutOrStdout()) }, } fl := cmd.Flags() - fl.StringVar(&image, "image", "", "image ID to rebuild from (required; pass an ID)") - fl.StringVar(&name, "name", "", "rename the server as part of the rebuild") + fl.StringVar(&f.image, "image", "", "image ID to rebuild from (required; pass an ID)") + fl.StringVar(&f.name, "name", "", "rename the server as part of the rebuild") + fl.StringVar(&f.userData, "user-data", "", + "path to a cloud-init/user-data file to replace the server's own (nova 2.57 or later)") + fl.BoolVar(&f.noUserData, "no-user-data", false, + "clear the server's existing user data (nova 2.57 or later)") + cmd.MarkFlagsMutuallyExclusive("user-data", "no-user-data") return cmd } +// serverRebuildOptsExt carries the one rebuild field gophercloud's +// servers.RebuildOpts cannot express: user_data. That struct predates +// microversion 2.57 — it still models the personality files 2.57 removed — so +// the field is spliced into the body here, the way serverCreateOptsExt splices +// nova 2.74's host into a create. +type serverRebuildOptsExt struct { + servers.RebuildOptsBuilder + + // UserData is the base64 payload to set; nil leaves user_data out of the + // request, which is what keeps the server's current value. ClearUserData + // sends JSON null instead, nova's reset. + UserData *string + ClearUserData bool +} + +func (opts serverRebuildOptsExt) ToServerRebuildMap() (map[string]any, error) { + body, err := opts.RebuildOptsBuilder.ToServerRebuildMap() + if err != nil { + return nil, err + } + rebuild, ok := body["rebuild"].(map[string]any) + if !ok { + return nil, fmt.Errorf("unexpected rebuild request body: %T", body["rebuild"]) + } + switch { + case opts.ClearUserData: + rebuild["user_data"] = nil + case opts.UserData != nil: + rebuild["user_data"] = *opts.UserData + } + return body, nil +} + +// newServerRebuildOpts builds the rebuild body, refusing the user-data flags +// when the client is pinned below the microversion that added the field. The +// file is read here, before the server reference is resolved, so a bad path +// costs no round-trip. +func newServerRebuildOpts(client *gophercloud.ServiceClient, f *serverRebuildFlags) (servers.RebuildOptsBuilder, error) { + // RebuildOpts.Name is tagged omitempty, so an unset --name leaves the body + // exactly as it was before that flag existed and nova keeps the current name. + base := servers.RebuildOpts{ImageRef: f.image, Name: f.name} + if f.userData == "" && !f.noUserData { + return base, nil + } + flag := "--user-data" + if f.noUserData { + flag = "--no-user-data" + } + if !computeSupportsMicroversion(client, rebuildUserDataMicroversion) { + return nil, fmt.Errorf("%s requires nova microversion %s or later; this client is pinned to %s", + flag, rebuildUserDataMicroversion, client.Microversion) + } + if f.noUserData { + return serverRebuildOptsExt{RebuildOptsBuilder: base, ClearUserData: true}, nil + } + userData, err := readUserData(f.userData) + if err != nil { + return nil, err + } + if userData == "" { + // Unlike create, an empty file here is unambiguous: the operator asked + // for the server's user data to be replaced, and replacing it with + // nothing is what --no-user-data spells. Sending "" would leave the + // server with an empty-but-present value instead. + return nil, fmt.Errorf("--user-data file %q is empty; use --no-user-data to clear the server's user data", f.userData) + } + return serverRebuildOptsExt{RebuildOptsBuilder: base, UserData: &userData}, nil +} + func runServerRebuild(ctx context.Context, client *gophercloud.ServiceClient, o *output.Options, - ref, image, name string, w io.Writer, + ref string, f *serverRebuildFlags, w io.Writer, ) error { + opts, err := newServerRebuildOpts(client, f) + if err != nil { + return err + } id, err := resolveServerID(ctx, client, ref) if err != nil { return err } - // RebuildOpts.Name is tagged omitempty, so an unset --name leaves the body - // exactly as it was before this flag existed and nova keeps the current name. - s, err := servers.Rebuild(ctx, client, id, servers.RebuildOpts{ImageRef: image, Name: name}).Extract() + s, err := servers.Rebuild(ctx, client, id, opts).Extract() if err != nil { return fmt.Errorf("rebuilding server %q: %w", ref, err) } diff --git a/internal/cli/server/rebuild_userdata_test.go b/internal/cli/server/rebuild_userdata_test.go new file mode 100644 index 0000000..0c0d253 --- /dev/null +++ b/internal/cli/server/rebuild_userdata_test.go @@ -0,0 +1,162 @@ +package server + +import ( + "bytes" + "context" + "encoding/json" + "io" + "net/http" + "strings" + "testing" + + th "github.com/gophercloud/gophercloud/v2/testhelper" + + "github.com/ftarasenko/go-openstackclient/internal/auth" + "github.com/ftarasenko/go-openstackclient/internal/output" +) + +// rebuildBody runs a rebuild against a mock and returns the decoded "rebuild" +// object plus the raw request body, which is what distinguishes an absent +// user_data from an explicit JSON null. +func rebuildBody(t *testing.T, microversion string, f *serverRebuildFlags) (map[string]any, string, error) { + t.Helper() + fakeServer := th.SetupHTTP() + t.Cleanup(fakeServer.Teardown) + + var raw string + fakeServer.Mux.HandleFunc("/servers/"+serverUUID+"/action", func(w http.ResponseWriter, r *http.Request) { + body, err := io.ReadAll(r.Body) + if err != nil { + t.Fatalf("reading rebuild body: %v", err) + } + raw = string(body) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusAccepted) + _, _ = w.Write([]byte(`{"server":{"id":"` + serverUUID + `","name":"web-1","status":"REBUILD"}}`)) + }) + + var buf bytes.Buffer + err := runServerRebuild(context.Background(), computeClient(fakeServer, microversion), + &output.Options{Format: output.FormatTable}, serverUUID, f, &buf) + if err != nil { + return nil, raw, err + } + var decoded map[string]any + if raw != "" { + var body map[string]any + if err := json.Unmarshal([]byte(raw), &body); err != nil { + t.Fatalf("decoding rebuild body %q: %v", raw, err) + } + decoded, _ = body["rebuild"].(map[string]any) + } + return decoded, raw, nil +} + +// TestRunServerRebuild_UserData covers nova 2.57's addition: a file replaces the +// server's user data, --no-user-data clears it with a JSON null, and an +// untouched rebuild still omits the field so the server keeps what it has. +func TestRunServerRebuild_UserData(t *testing.T) { + t.Run("file replaces it", func(t *testing.T) { + path := userDataFixture(t, "user-data", []byte("runcmd\nls\n")) + rebuild, _, err := rebuildBody(t, "2.93", &serverRebuildFlags{image: "img-new", userData: path}) + if err != nil { + t.Fatalf("runServerRebuild: %v", err) + } + // The same encoding regression as create: this payload is valid base64 + // once the newlines are dropped. + if got, want := rebuild["user_data"], "cnVuY21kCmxzCg=="; got != want { + t.Errorf("user_data = %v, want %v", got, want) + } + }) + + t.Run("no-user-data clears it", func(t *testing.T) { + rebuild, raw, err := rebuildBody(t, "2.93", &serverRebuildFlags{image: "img-new", noUserData: true}) + if err != nil { + t.Fatalf("runServerRebuild: %v", err) + } + value, present := rebuild["user_data"] + if !present || value != nil { + t.Errorf("user_data = %v (present %v), want an explicit null", value, present) + } + // Belt and braces: a decoded nil is indistinguishable from a missing + // key in some shapes, so assert the literal null reached the wire. + if !strings.Contains(raw, `"user_data":null`) { + t.Errorf("request body = %s, want an explicit \"user_data\":null", raw) + } + }) + + t.Run("untouched keeps it", func(t *testing.T) { + rebuild, _, err := rebuildBody(t, "2.93", &serverRebuildFlags{image: "img-new"}) + if err != nil { + t.Fatalf("runServerRebuild: %v", err) + } + if _, present := rebuild["user_data"]; present { + t.Error("a rebuild without the user-data flags must omit the field") + } + }) + + t.Run("empty file is refused", func(t *testing.T) { + path := userDataFixture(t, "empty", nil) + _, raw, err := rebuildBody(t, "2.93", &serverRebuildFlags{image: "img-new", userData: path}) + if err == nil || !strings.Contains(err.Error(), "--no-user-data") { + t.Fatalf("error = %v, want it to point at --no-user-data", err) + } + if raw != "" { + t.Errorf("nothing should have been sent, got %s", raw) + } + }) +} + +// TestRunServerRebuild_UserDataMicroversion pins the gate to nova's 2.57 rather +// than OSC's 2.54, which its own help text contradicts and nova's schema +// rejects. Below the gate nothing is sent at all. +func TestRunServerRebuild_UserDataMicroversion(t *testing.T) { + path := userDataFixture(t, "user-data", []byte("#cloud-config\n")) + for _, tc := range []struct { + name string + f *serverRebuildFlags + flag string + }{ + {"user-data", &serverRebuildFlags{image: "img-new", userData: path}, "--user-data"}, + {"no-user-data", &serverRebuildFlags{image: "img-new", noUserData: true}, "--no-user-data"}, + } { + t.Run(tc.name, func(t *testing.T) { + _, raw, err := rebuildBody(t, "2.56", tc.f) + if err == nil || !strings.Contains(err.Error(), tc.flag) || + !strings.Contains(err.Error(), rebuildUserDataMicroversion) { + t.Fatalf("error = %v, want it to name %s and %s", err, tc.flag, rebuildUserDataMicroversion) + } + if raw != "" { + t.Errorf("nothing should have been sent below the microversion, got %s", raw) + } + // 2.57 exactly is enough. + if _, _, err := rebuildBody(t, rebuildUserDataMicroversion, tc.f); err != nil { + t.Errorf("runServerRebuild at %s: %v", rebuildUserDataMicroversion, err) + } + }) + } +} + +// TestServerRebuild_UserDataFlagParity pins the option surface against OSC's +// RebuildServer parser, where the two are one mutually exclusive group. +func TestServerRebuild_UserDataFlagParity(t *testing.T) { + root := NewCommand(&auth.Options{}, &output.Options{}) + leaf, _, err := root.Find([]string{"rebuild"}) + if err != nil || leaf == nil { + t.Fatalf("server rebuild: not found: %v", err) + } + for _, name := range []string{"user-data", "no-user-data"} { + if leaf.Flags().Lookup(name) == nil { + t.Fatalf("koc server rebuild: missing --%s", name) + } + } + // The refusal comes from cobra's flag-group validation, which runs before + // RunE, so this never reaches auth. + root.SetOut(io.Discard) + root.SetErr(io.Discard) + root.SetArgs([]string{"rebuild", "--image", "img-new", "--user-data", "/dev/null", "--no-user-data", serverUUID}) + if err := root.Execute(); err == nil || + !strings.Contains(err.Error(), "none of the others can be") { + t.Errorf("error = %v, want cobra's mutual-exclusion refusal", err) + } +} diff --git a/internal/cli/server/server_more_test.go b/internal/cli/server/server_more_test.go index 629b83e..746aee8 100644 --- a/internal/cli/server/server_more_test.go +++ b/internal/cli/server/server_more_test.go @@ -711,7 +711,8 @@ func TestRunServerRebuild_RequestAndOutput(t *testing.T) { client := computeClient(fakeServer, "2.79") o := &output.Options{Format: output.FormatTable} var buf bytes.Buffer - if err := runServerRebuild(context.Background(), client, o, serverUUID, "img-new", "", &buf); err != nil { + f := &serverRebuildFlags{image: "img-new"} + if err := runServerRebuild(context.Background(), client, o, serverUUID, f, &buf); err != nil { t.Fatalf("runServerRebuild: %v", err) } if gotMethod != http.MethodPost { @@ -735,7 +736,8 @@ func TestRunServerRebuild_RequestAndOutput(t *testing.T) { // ... and a given --name must reach nova in the same action body. var renamed bytes.Buffer - if err := runServerRebuild(context.Background(), client, o, serverUUID, "img-new", "web-2", &renamed); err != nil { + renameFlags := &serverRebuildFlags{image: "img-new", name: "web-2"} + if err := runServerRebuild(context.Background(), client, o, serverUUID, renameFlags, &renamed); err != nil { t.Fatalf("runServerRebuild with --name: %v", err) } rebuild, _ = gotBody["rebuild"].(map[string]any)