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..7c4c501 100644 --- a/README.md +++ b/README.md @@ -543,6 +543,41 @@ 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, 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 +> 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 new file mode 100644 index 0000000..9a70fd4 --- /dev/null +++ b/docs/proposals/user-data-parity.md @@ -0,0 +1,235 @@ +# `--user-data` parity with upstream `openstack` + +**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 +→ "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. 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/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/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.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..746aee8 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) } @@ -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)