diff --git a/config/samples/providers-config.yaml b/config/samples/providers-config.yaml index d0d7743..b32cc5a 100644 --- a/config/samples/providers-config.yaml +++ b/config/samples/providers-config.yaml @@ -30,8 +30,9 @@ providers: knownHostsFile: /etc/proxy-operator/kvm/known_hosts # insecureIgnoreHostKey: true # skip host-key checks (not recommended) # diskSizeGb: 10 # per-VM overlay; cloud-init grows the root fs into it - # vendorData: | # extra vendor-data; the qemu-guest-agent bootstrap is merged - # #cloud-config # into each Proxy's cloud-config, not configured here + # installGuestAgent: false # skip the vendor-data script that installs qemu-guest-agent + # vendorData: | # extra vendor-data, carried beside the agent script as a + # #cloud-config # MIME multipart. The Proxy's own cloud-init is never rewritten. # ... images: # placement.image names a key here (or is an https:// URL directly). diff --git a/docs/kvm-provider.md b/docs/kvm-provider.md index 8e1f9a9..f58e4e5 100644 --- a/docs/kvm-provider.md +++ b/docs/kvm-provider.md @@ -27,13 +27,12 @@ Per host, once: `edk2-ovmf` on Fedora/RHEL. VMs boot UEFI by default (libvirt picks the OVMF binary itself); a domain start fails with "no firmware found" without it. Only `firmware: bios` images skip this. -6. Optionally, **outbound internet from the VMs**: the operator adds - `qemu-guest-agent` to every Proxy's cloud-config (`packages` plus a - `runcmd` that enables it), so it is installed from the distro mirrors - on first boot. Without it the VM still works — the operator falls back - to the host's ARP table to find the IP, which fills in once the guest - sends traffic — but note the ARP table only sees traffic to the host - itself, so on a plain bridge the agent is in practice required. +6. Optionally, **outbound internet from the VMs**: a vendor-data script + installs `qemu-guest-agent` from the distro mirrors on first boot. + Without it the VM still works — the operator falls back to the host's + ARP table to find the IP, which fills in once the guest sends traffic — + but the ARP table only sees traffic to the host itself, so on a plain + bridge the agent is in practice required. Check from the operator's point of view before wiring it in: @@ -136,7 +135,8 @@ ssh virt@nas1.home.lan ip link show br0 | `knownHostsFile` | unless `insecureIgnoreHostKey` | | OpenSSH `known_hosts` to verify host keys against | | `insecureIgnoreHostKey` | | `false` | skip host-key verification entirely | | `diskSizeGb` | | `10` | capacity of each VM's overlay disk; cloud-init's growpart expands the root fs into it | -| `vendorData` | | | extra cloud-init vendor-data placed on every seed next to the Proxy's user-data; cloud-init drops any top-level key here that the user-data also defines. The guest-agent bootstrap is not configured here, see *What lands on the host* | +| `vendorData` | | | extra cloud-init vendor-data placed on every seed next to the Proxy's user-data. Beware that cloud-init drops any top-level key of a vendor-data **cloud-config** that the user-data also defines. Shipped as one part of a MIME multipart alongside the agent script when `installGuestAgent` is on | +| `installGuestAgent` | | `true` | add the vendor-data script that installs and starts `qemu-guest-agent`. Turn off for images that ship it already or cannot reach a package mirror; IP discovery then depends on the host's ARP table | | `images.` | | | the key names the image in `placement.image`. Letters, digits, dots, dashes and underscores only — it becomes the cached volume name `base-.` | | `images..url` | yes per entry | | http(s) URL of a cloud image (qcow2 or raw) | | `images..sha256` | | | expected digest; verified after download, a change triggers a re-download | @@ -182,20 +182,27 @@ console, the guest-agent channel, a virtio RNG and no graphics. Its creation time) — that is how orphan GC recognises its own VMs, since libvirt has no labels. -The seed's `user-data` is the Proxy's `cloudInit` with the guest-agent -bootstrap **merged into the document**: `qemu-guest-agent` appended to -`packages` and `[systemctl, enable, --now, qemu-guest-agent]` to `runcmd` -(neither duplicated if already there; other keys untouched, though the -YAML is re-rendered). It is not shipped as vendor-data because cloud-init -merges vendor-data under user-data with its default mergers, which keep -the user-data's value for any key both define and never append lists — a -cloud-config with its own `packages:` would silently lose the agent and -the Proxy would sit in `Provisioning` forever. Only when the user-data is -not a cloud-config document (a shell script, `#include`, MIME archive) -does the bootstrap go to `vendor-data`, and then only if `vendorData` is -not configured. A cloud-config that does not parse as a YAML mapping, or -whose `packages`/`runcmd` is not a list, fails the Proxy permanently -rather than booting a VM without its config. +The seed's `user-data` is the Proxy's `cloudInit`, **byte for byte**. Any +format cloud-init accepts works, including a Jinja template, a MIME +archive, a shell script or a gzipped payload; the operator never parses or +re-renders it. + +The guest agent rides along in `vendor-data`, as a **shell script** rather +than a cloud-config. That is the only shape cloud-init cannot drop: +vendor-data cloud-config is merged under the user's own document with +mergers that keep the user-data's value for every key both define and never +append lists, so a Proxy carrying its own `packages:` silently lost the +agent and sat in `Provisioning` forever. The strategy cannot be overridden +from either document — `merge_how` in user-data governs only the first +source, and `merge_how` in vendor-data is consumed and discarded before the +document is stored. A vendor-data script skips all of it: cloud-init gives +vendor-data its own handler chain, writes scripts to a separate directory +and runs them from `cc_scripts_vendor`, after the user's packages are +installed and before the user's `runcmd`. The script is idempotent, picks +whichever of `apt-get`, `dnf`, `yum`, `zypper` or `apk` the image has, and +logs into `/var/log/cloud-init-output.log`. Set `installGuestAgent: false` +to leave it out. When `vendorData` is also configured, the two travel as +separate parts of a MIME multipart vendor-data. ## The image cache @@ -287,8 +294,10 @@ The provider logs through controller-runtime's logger, named `kvm`, with bridge with no DHCP never gets an address and stays `Provisioning` forever; check `virsh console`. The same symptom with a booted, addressed VM means the guest agent never came up: check `systemctl status - qemu-guest-agent` and `cloud-init status --long` in the guest (a VM - without outbound internet cannot install it). + qemu-guest-agent`, `cloud-init status --long` and the vendor script's + output in `/var/log/cloud-init-output.log` (a VM without outbound + internet cannot install it). A user-data that disables vendor-data + (`vendor_data: {enabled: false}`) also removes the agent, by design. - **Debian 13 cloud images hang in GRUB under BIOS on a headless VM** ([Debian bug #1111240](https://bugs.debian.org/1111240), open, in `grub2`): trixie's image ships `terminal_output gfxterm serial` in its diff --git a/docs/plans-executions/2026-09-10-2222-kvm-review-fixes.md b/docs/plans-executions/2026-09-10-2222-kvm-review-fixes.md index 00a5aa4..30143f5 100644 --- a/docs/plans-executions/2026-09-10-2222-kvm-review-fixes.md +++ b/docs/plans-executions/2026-09-10-2222-kvm-review-fixes.md @@ -5,7 +5,7 @@ Plan: [docs/plans/2026-09-10-2222-kvm-review-fixes.md](../plans/2026-09-10-2222- - [x] Step 1 — Unwedge deletion (finding 1) - [x] Step 2 — Validate names that become identifiers (finding 2) - [x] Step 3 — Read the SSH material lazily (finding 3) -- [ ] Step 4 — Guest agent via vendor-data script (findings 4 and 5) +- [x] Step 4 — Guest agent via vendor-data script (findings 4 and 5) - [ ] Step 5 — Validate, record, push ## Step 0 — plan committed @@ -98,6 +98,70 @@ a rotated Secret needs a manager restart. `TestNew_badKeyPathFailsFast` was inverted rather than deleted, so the old contract stays visible in the diff next to the new one. +## Step 4 — Guest agent via vendor-data script + +The review filed two findings against `withGuestAgent` — detection too +narrow, and YAML re-typing on the round-trip. Reading cloud-init 26.1's +sources while planning turned both into one finding: the operator should +not be editing the user's document at all. + +Vendor-data gets its own handler chain (`stages.py:_default_vendordata_handlers`), +with separate script and cloud-config paths. A vendor-data **script** is +therefore written to the vendor script directory and run by +`cc_scripts_vendor`, never touching the config merger that drops colliding +keys. On the final-stage module list it lands after +`package_update_upgrade_install` and before the user's `runcmd`, so the +agent now comes up *earlier* than the old `runcmd` append did. + +That also closed the door the previous approach had left ajar. The merge +cannot be rescued from inside either document: `merge_how` in user-data +governs only the first source, and `merge_how` in vendor-data is popped by +`dict_extract_mergers` and discarded before the document is written. There +was no fix that kept the merge. + +`withGuestAgent`, `isCloudConfig` and `appendUnique` are gone, along with +seed.go's `sigs.k8s.io/yaml` dependency. `installGuestAgent` (default true) +turns the script off. When `vendorData` is also configured the two ship as +a MIME multipart, with the user's document declared `text/plain` — a type +cloud-init re-sniffs from the content's own header line, so this code never +classifies what the user wrote. + +Before deleting the old path, its damage was captured rather than taken on +trust. Round-tripping a plausible cloud-config through it: + +```sh +# temporary test in the package, deleted afterwards +go test -run TestZZProbeRetyping -v ./internal/provider/kvm/ +# in: version: 1.10 +# out: version: 1.1 +``` + +`1.10` arriving as `1.1` is worse than the reviewer's `passwd` example, +since nothing about it looks wrong in the seed. Comments were dropped and +keys reordered on the same pass. + +The generated vendor-data was then checked against the cloud-init actually +installed on this machine, rather than against the docs: + +```sh +python3 -c 'from cloudinit import user_data, handlers; ...' +# vendor-script: multipart=False sniffed=text/x-shellscript +# vendor-multipart: multipart=True part 1 text/plain -> text/cloud-config +# part 2 text/x-shellscript +``` + +and the script itself through `sh -n`. + +Worth noting: the script has to choose a package manager where cloud-init's +`packages:` key was distro-agnostic. It covers apt, dnf, yum, zypper and +apk, and both systemd and OpenRC for the enable step, but an image outside +that set now needs `installGuestAgent: false` and the agent baked in. The +seed's user-data is byte-identical to the Proxy's `cloudInit` from here on, +which is what the new `TestBuildSeedISO_userDataIsNeverRewritten` pins +across seven formats. One consequence worth remembering: a user-data that +sets `vendor_data: {enabled: false}` disables the agent, and cloud-init +documents that as intended behaviour. + Worth noting: the review filed this as a KVM finding, but the fix is mostly in the controller, because KVM is the only provider that resolves a providerID segment against operator config at all. GCP and Kubernetes hand diff --git a/internal/provider/config_kvm.go b/internal/provider/config_kvm.go index d8010f1..1e672cf 100644 --- a/internal/provider/config_kvm.go +++ b/internal/provider/config_kvm.go @@ -23,10 +23,17 @@ type KVMConfig struct { // guest's cloud-init growpart expands the root filesystem into it. DiskSizeGB int64 `json:"diskSizeGb,omitempty"` // VendorData is optional cloud-init vendor-data placed on every seed next - // to the Proxy's own user-data. cloud-init drops any top-level key here - // that the user-data also defines. The qemu-guest-agent bootstrap does - // not live here: the provider merges it into each Proxy's cloud-config. + // to the Proxy's own user-data. Note that cloud-init drops any top-level + // key of a vendor-data cloud-config that the user-data also defines. + // When InstallGuestAgent is on, this document and the agent script are + // carried as separate parts of a MIME multipart vendor-data. VendorData string `json:"vendorData,omitempty"` + // InstallGuestAgent adds a vendor-data script that installs and starts + // qemu-guest-agent, which is how the provider learns a VM's address + // before the host's ARP table has seen traffic from it. Default true. + // Turn it off for images that already ship the agent, or that cannot + // reach a package mirror; discovery then depends on ARP alone. + InstallGuestAgent *bool `json:"installGuestAgent,omitempty"` // Images are named base images; placement.image names a key here (or is // an http(s) URL directly). The provider downloads each image into a // host's storage pool on first use and caches it there. diff --git a/internal/provider/kvm/kvm.go b/internal/provider/kvm/kvm.go index e0d51b4..41e394a 100644 --- a/internal/provider/kvm/kvm.go +++ b/internal/provider/kvm/kvm.go @@ -250,7 +250,7 @@ func (p *Provider) writeSeed(log logr.Logger, api libvirtAPI, pool libvirt.Stora if err := deleteIfExists(api, pool, seedName); err != nil { return libvirt.StorageVol{}, err } - userData, vendorData, err := withGuestAgent(userData, p.cfg.VendorData) + vendorData, err := buildVendorData(p.cfg.VendorData, p.cfg.InstallGuestAgent == nil || *p.cfg.InstallGuestAgent) if err != nil { return libvirt.StorageVol{}, fmt.Errorf("%w: %v", provider.ErrPermanent, err) } diff --git a/internal/provider/kvm/kvm_test.go b/internal/provider/kvm/kvm_test.go index f8c6cf3..6a39007 100644 --- a/internal/provider/kvm/kvm_test.go +++ b/internal/provider/kvm/kvm_test.go @@ -188,8 +188,11 @@ func assertCreated(t *testing.T, api *fakeAPI) { t.Errorf("seed = format %s, %d bytes, capacity %d; want raw with capacity == uploaded size", seed.format, len(seed.data), seed.capacity) } _, files := readSeed(t, seed.data) - if ud := files["user-data"]; !strings.Contains(ud, "squid") || !strings.Contains(ud, guestAgentPackage) { - t.Errorf("seed user-data = %q, want the Proxy's cloud-config with the guest agent merged in", ud) + if files["user-data"] != testCloudInit { + t.Errorf("seed user-data = %q, want the Proxy's cloud-init verbatim", files["user-data"]) + } + if !strings.Contains(files["vendor-data"], "qemu-guest-agent") { + t.Errorf("seed vendor-data = %q, want the guest-agent script", files["vendor-data"]) } } diff --git a/internal/provider/kvm/seed.go b/internal/provider/kvm/seed.go index e2f612d..7ba7e13 100644 --- a/internal/provider/kvm/seed.go +++ b/internal/provider/kvm/seed.go @@ -3,110 +3,104 @@ package kvm import ( "bytes" "fmt" - "reflect" + "io" + "mime/multipart" + "net/textproto" "strings" "github.com/kdomanski/iso9660" - "sigs.k8s.io/yaml" ) // seedLabel is the volume label cloud-init's NoCloud datasource looks for. const seedLabel = "cidata" -const ( - cloudConfigHeader = "#cloud-config" - guestAgentPackage = "qemu-guest-agent" -) - -// guestAgentEnable is the runcmd entry that starts the agent once it is -// installed, so the provider can read the VM's IP before ARP sees traffic. -var guestAgentEnable = []any{"systemctl", "enable", "--now", guestAgentPackage} - -// guestAgentVendorData is the same bootstrap as a standalone cloud-config. -// It is only used as vendor-data when the Proxy's user-data is not a -// cloud-config document (a shell script, a MIME archive, an #include), i.e. -// when there are no top-level keys for it to collide with. -const guestAgentVendorData = cloudConfigHeader + ` -packages: - - qemu-guest-agent -runcmd: - - [systemctl, enable, --now, qemu-guest-agent] +// guestAgentScript installs and starts the QEMU guest agent, which is how +// the provider learns a VM's address before the host's ARP table has seen +// any traffic from it. +// +// It ships as vendor-data, and as a *script* rather than a cloud-config, +// because that is the only shape cloud-init cannot drop. Vendor-data +// cloud-config is merged under the Proxy's own user-data with mergers that +// keep the user-data's value for every key both documents define and never +// append lists, so a Proxy carrying its own packages: silently lost the +// agent. The strategy cannot be overridden from either document: merge_how +// in user-data governs only the first source, and merge_how in vendor-data +// is consumed and discarded before the document is stored. A vendor-data +// script bypasses all of it — cloud-init gives vendor-data its own handler +// chain, writes scripts to a separate directory and runs them from +// cc_scripts_vendor, after the user's packages are installed and before the +// user's runcmd. +const guestAgentScript = `#!/bin/sh +# Added by egress-proxies-operator so the hypervisor can report this VM's +# address. Set installGuestAgent: false on the provider to leave it out. +set -e +if ! command -v qemu-ga >/dev/null 2>&1 && [ ! -x /usr/sbin/qemu-ga ]; then + if command -v apt-get >/dev/null 2>&1; then + apt-get update + DEBIAN_FRONTEND=noninteractive apt-get install -y qemu-guest-agent + elif command -v dnf >/dev/null 2>&1; then + dnf install -y qemu-guest-agent + elif command -v yum >/dev/null 2>&1; then + yum install -y qemu-guest-agent + elif command -v zypper >/dev/null 2>&1; then + zypper --non-interactive install qemu-guest-agent + elif command -v apk >/dev/null 2>&1; then + apk add qemu-guest-agent + else + echo "no known package manager; install qemu-guest-agent in the image" >&2 + exit 1 + fi +fi +if command -v systemctl >/dev/null 2>&1; then + systemctl enable --now qemu-guest-agent +elif command -v rc-update >/dev/null 2>&1; then + rc-update add qemu-guest-agent default + rc-service qemu-guest-agent start +fi ` -// withGuestAgent decides where the guest-agent bootstrap goes and returns -// the user-data and vendor-data to put on the seed. -// -// It cannot simply ride along as vendor-data: cloud-init merges vendor-data -// under user-data with its default mergers, which keep the user-data's -// value for any key both define and never append lists. A Proxy whose -// cloud-config has its own `packages:` (most do) would silently lose the -// agent and sit in Provisioning forever. So for a cloud-config user-data -// the bootstrap is merged into the document itself: `qemu-guest-agent` -// appended to `packages`, the enable command to `runcmd`, both skipped -// when already present. Anything the operator cannot parse as a -// cloud-config mapping is an error — cloud-init would reject it too, and a -// VM that boots without its config is harder to diagnose than a Proxy that -// fails fast. -func withGuestAgent(userData, vendorData string) (string, string, error) { - if !isCloudConfig(userData) { - if vendorData == "" { - vendorData = guestAgentVendorData - } - return userData, vendorData, nil - } - var cfg map[string]any - if err := yaml.Unmarshal([]byte(userData), &cfg); err != nil { - return "", "", fmt.Errorf("parsing cloud-config user-data: %w", err) - } - if cfg == nil { - cfg = map[string]any{} - } - pkgs, err := appendUnique(cfg, "packages", guestAgentPackage) - if err != nil { - return "", "", err - } - cfg["packages"] = pkgs - cmds, err := appendUnique(cfg, "runcmd", guestAgentEnable) - if err != nil { - return "", "", err - } - cfg["runcmd"] = cmds - out, err := yaml.Marshal(cfg) - if err != nil { - return "", "", fmt.Errorf("rendering cloud-config user-data: %w", err) - } - return cloudConfigHeader + "\n" + string(out), vendorData, nil -} - -// isCloudConfig reports whether userData is a cloud-config document by -// cloud-init's own rule: the first line is the #cloud-config header. Empty -// user-data counts, so a Proxy without cloud-init still gets the agent. -func isCloudConfig(userData string) bool { - if strings.TrimSpace(userData) == "" { - return true - } - first, _, _ := strings.Cut(userData, "\n") - return strings.TrimSpace(first) == cloudConfigHeader -} - -// appendUnique returns cfg[key] with item appended unless an equal entry -// is already there. A missing key is an empty list; anything but a list is -// an error, since cloud-init expects one there as well. -func appendUnique(cfg map[string]any, key string, item any) ([]any, error) { - var items []any - switch v := cfg[key].(type) { - case nil: - case []any: - items = v +// buildVendorData renders the seed's vendor-data. With nothing configured +// it is the guest-agent script alone; with both, a MIME multipart carrying +// the configured document and the script as separate parts, since +// vendor-data is a single file and the two must not be concatenated. +func buildVendorData(configured string, installAgent bool) (string, error) { + switch { + case !installAgent: + return configured, nil + case strings.TrimSpace(configured) == "": + return guestAgentScript, nil default: - return nil, fmt.Errorf("cloud-config %s: want a list, got %T", key, v) + return multipartVendorData(configured, guestAgentScript) } - for _, it := range items { - if reflect.DeepEqual(it, item) { - return items, nil +} + +// multipartVendorData packs several documents into one vendor-data blob. +// cloud-init decides a payload is multipart by finding "mime-version:" +// within its first 4096 bytes, so those headers lead. The configured +// document goes in as text/plain on purpose: cloud-init re-sniffs that type +// from the content's own header line, which keeps this code out of the +// business of classifying what the user wrote. +func multipartVendorData(configured, script string) (string, error) { + var body bytes.Buffer + w := multipart.NewWriter(&body) + parts := []struct{ contentType, content string }{ + {"text/plain", configured}, + {"text/x-shellscript", script}, + } + for _, p := range parts { + part, err := w.CreatePart(textproto.MIMEHeader{"Content-Type": {p.contentType}}) + if err != nil { + return "", fmt.Errorf("building vendor-data part: %w", err) + } + if _, err := io.WriteString(part, p.content); err != nil { + return "", fmt.Errorf("writing vendor-data part: %w", err) } } - return append(items, item), nil + if err := w.Close(); err != nil { + return "", fmt.Errorf("closing vendor-data: %w", err) + } + return fmt.Sprintf("MIME-Version: 1.0\r\nContent-Type: multipart/mixed; boundary=%q\r\n\r\n%s", + w.Boundary(), body.String()), nil } // buildSeedISO renders a NoCloud seed: user-data and vendor-data verbatim, diff --git a/internal/provider/kvm/seed_test.go b/internal/provider/kvm/seed_test.go index fc97464..66caa50 100644 --- a/internal/provider/kvm/seed_test.go +++ b/internal/provider/kvm/seed_test.go @@ -4,12 +4,12 @@ import ( "bytes" "io" "maps" - "reflect" + "mime" + "mime/multipart" "strings" "testing" "github.com/kdomanski/iso9660" - "sigs.k8s.io/yaml" ) func readSeed(t *testing.T, iso []byte) (label string, files map[string]string) { @@ -44,7 +44,7 @@ func readSeed(t *testing.T, iso []byte) (label string, files map[string]string) func TestBuildSeedISO_contents(t *testing.T) { t.Parallel() userData := "#cloud-config\npackages: [squid]\n" - iso, err := buildSeedISO("proxy-abc123def456ghij", userData, guestAgentVendorData) + iso, err := buildSeedISO("proxy-abc123def456ghij", userData, guestAgentScript) if err != nil { t.Fatalf("buildSeedISO() error = %v", err) } @@ -58,7 +58,7 @@ func TestBuildSeedISO_contents(t *testing.T) { if files["user-data"] != userData { t.Errorf("user-data = %q, want verbatim cloud-init", files["user-data"]) } - if files["vendor-data"] != guestAgentVendorData { + if files["vendor-data"] != guestAgentScript { t.Errorf("vendor-data = %q", files["vendor-data"]) } if got, want := files["meta-data"], "instance-id: proxy-abc123def456ghij\nlocal-hostname: proxy-abc123def456ghij\n"; got != want { @@ -91,7 +91,7 @@ func TestBuildSeedISO_deterministicContent(t *testing.T) { func TestBuildSeedISO_emptyUserData(t *testing.T) { t.Parallel() - iso, err := buildSeedISO("proxy-x", "", guestAgentVendorData) + iso, err := buildSeedISO("proxy-x", "", guestAgentScript) if err != nil { t.Fatalf("buildSeedISO() error = %v", err) } @@ -101,149 +101,133 @@ func TestBuildSeedISO_emptyUserData(t *testing.T) { } } -// parseCloudConfig is what cloud-init sees: the document under the header. -func parseCloudConfig(t *testing.T, doc string) map[string]any { - t.Helper() - if first, _, _ := strings.Cut(doc, "\n"); first != cloudConfigHeader { - t.Fatalf("first line = %q, want %q", first, cloudConfigHeader) - } - var cfg map[string]any - if err := yaml.Unmarshal([]byte(doc), &cfg); err != nil { - t.Fatalf("merged user-data is not YAML: %v\n%s", err, doc) - } - return cfg -} - -func TestWithGuestAgent_mergesIntoCloudConfig(t *testing.T) { - t.Parallel() - agentCmd := []any{"systemctl", "enable", "--now", "qemu-guest-agent"} - tests := []struct { - name string - userData string - wantPackages []any - wantRuncmd []any - }{ - { - name: "empty document", - userData: "#cloud-config\n", - wantPackages: []any{"qemu-guest-agent"}, - wantRuncmd: []any{agentCmd}, - }, - { - name: "no user-data at all", - userData: "", - wantPackages: []any{"qemu-guest-agent"}, - wantRuncmd: []any{agentCmd}, - }, - { - // The case that motivated this: cloud-init's default merger keeps - // the user-data's lists and drops vendor-data's, so the agent must - // be appended to the user's own packages and runcmd. - name: "user packages and runcmd are kept and extended", - userData: "#cloud-config\npackages:\n - squid\nruncmd:\n - systemctl restart squid\n" + - "write_files:\n - path: /etc/squid/conf.d/x.conf\n content: |\n http_port 3128\n via off\n", - wantPackages: []any{"squid", "qemu-guest-agent"}, - wantRuncmd: []any{"systemctl restart squid", agentCmd}, - }, - { - name: "agent already present is not duplicated", - userData: "#cloud-config\npackages: [qemu-guest-agent]\nruncmd:\n - [systemctl, enable, --now, qemu-guest-agent]\n", - wantPackages: []any{"qemu-guest-agent"}, - wantRuncmd: []any{agentCmd}, - }, - { - name: "header with trailing whitespace", - userData: "#cloud-config \r\npackages: [curl]\n", - wantPackages: []any{"curl", "qemu-guest-agent"}, - wantRuncmd: []any{agentCmd}, - }, - } - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - t.Parallel() - user, vendor, err := withGuestAgent(tc.userData, "") - if err != nil { - t.Fatalf("withGuestAgent() error = %v", err) - } - if vendor != "" { - t.Errorf("vendor-data = %q, want none when the agent is merged into the user-data", vendor) - } - cfg := parseCloudConfig(t, user) - if !reflect.DeepEqual(cfg["packages"], tc.wantPackages) { - t.Errorf("packages = %#v, want %#v", cfg["packages"], tc.wantPackages) - } - if !reflect.DeepEqual(cfg["runcmd"], tc.wantRuncmd) { - t.Errorf("runcmd = %#v, want %#v", cfg["runcmd"], tc.wantRuncmd) - } - }) - } -} - -func TestWithGuestAgent_keepsOtherKeysAndVendorData(t *testing.T) { - t.Parallel() - userData := "#cloud-config\nhostname: px\nwrite_files:\n - path: /etc/x\n content: |\n line one\n line two\n permissions: '0644'\nbootcmd: [echo hi]\n" - user, vendor, err := withGuestAgent(userData, "#cloud-config\nntp: {enabled: true}\n") - if err != nil { - t.Fatalf("withGuestAgent() error = %v", err) - } - if vendor != "#cloud-config\nntp: {enabled: true}\n" { - t.Errorf("vendor-data = %q, want the configured one verbatim", vendor) - } - got := parseCloudConfig(t, user) - delete(got, "packages") - delete(got, "runcmd") - want := parseCloudConfig(t, userData) - if !reflect.DeepEqual(got, want) { - t.Errorf("merged user-data changed the user's keys:\n got %#v\nwant %#v", got, want) - } -} - -func TestWithGuestAgent_nonCloudConfigFallsBackToVendorData(t *testing.T) { - t.Parallel() - tests := []struct { - name string - userData string - vendorData string - wantVendor string - }{ - {"shell script, no vendor-data", "#!/bin/sh\napt-get install -y squid\n", "", guestAgentVendorData}, - {"include, no vendor-data", "#include\nhttps://example/x\n", "", guestAgentVendorData}, - {"shell script, own vendor-data wins", "#!/bin/sh\n", "#cloud-config\nntp: {}\n", "#cloud-config\nntp: {}\n"}, - } - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - t.Parallel() - user, vendor, err := withGuestAgent(tc.userData, tc.vendorData) - if err != nil { - t.Fatalf("withGuestAgent() error = %v", err) - } - if user != tc.userData { - t.Errorf("user-data = %q, want it untouched", user) - } - if vendor != tc.wantVendor { - t.Errorf("vendor-data = %q, want %q", vendor, tc.wantVendor) - } - }) - } -} - -func TestWithGuestAgent_rejectsBrokenCloudConfig(t *testing.T) { +// The whole point of shipping the agent as vendor-data: whatever the Proxy +// wrote reaches the guest untouched, in any of cloud-init's formats. The +// previous implementation parsed and re-rendered cloud-config user-data, +// which dropped comments, reordered keys and re-typed unquoted scalars +// (version: 1.10 arrived as 1.1). +func TestBuildSeedISO_userDataIsNeverRewritten(t *testing.T) { t.Parallel() tests := []struct { name string userData string }{ - {"not yaml", "#cloud-config\npackages: [squid\n"}, - {"not a mapping", "#cloud-config\n- squid\n"}, - {"packages is not a list", "#cloud-config\npackages: squid\n"}, - {"runcmd is not a list", "#cloud-config\nruncmd:\n a: b\n"}, + {"cloud-config with its own packages", "#cloud-config\n# keep me\npackages:\n - squid\nversion: 1.10\nchpasswd: {list: 'u:0123456789'}\n"}, + {"jinja template", "## template: jinja\n#cloud-config\nhostname: {{ v1.local_hostname }}\n"}, + {"shell script", "#!/bin/sh\napt-get install -y squid\n"}, + {"include", "#include\nhttps://example.invalid/x\n"}, + {"mime multipart", "MIME-Version: 1.0\r\nContent-Type: multipart/mixed; boundary=\"x\"\r\n\r\n--x\r\nContent-Type: text/cloud-config\r\n\r\n#cloud-config\npackages: [squid]\n\r\n--x--\r\n"}, + {"gzip payload", "\x1f\x8b\x08\x00\x00\x00\x00\x00\x00\x03binary\x00bytes"}, + {"leading blank line", "\n\n#cloud-config\npackages: [squid]\n"}, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { t.Parallel() - if _, _, err := withGuestAgent(tc.userData, ""); err == nil { - t.Errorf("withGuestAgent() = nil error, want one for %q", tc.userData) + vendor, err := buildVendorData("", true) + if err != nil { + t.Fatalf("buildVendorData() error = %v", err) + } + iso, err := buildSeedISO("proxy-x", tc.userData, vendor) + if err != nil { + t.Fatalf("buildSeedISO() error = %v", err) + } + _, files := readSeed(t, iso) + if files["user-data"] != tc.userData { + t.Errorf("user-data was rewritten:\n got %q\nwant %q", files["user-data"], tc.userData) + } + if !strings.Contains(files["vendor-data"], "qemu-guest-agent") { + t.Errorf("vendor-data = %q, want the agent script", files["vendor-data"]) } }) } } + +func TestBuildVendorData_agentScriptAlone(t *testing.T) { + t.Parallel() + got, err := buildVendorData("", true) + if err != nil { + t.Fatalf("buildVendorData() error = %v", err) + } + if got != guestAgentScript { + t.Errorf("vendor-data = %q, want the bare script", got) + } + if !strings.HasPrefix(got, "#!") { + t.Error("vendor-data must start with a shebang or cloud-init treats it as something other than a script") + } +} + +func TestBuildVendorData_optOutKeepsConfiguredOnly(t *testing.T) { + t.Parallel() + configured := "#cloud-config\nntp: {enabled: true}\n" + tests := []struct { + name string + configured string + want string + }{ + {"nothing configured", "", ""}, + {"configured document passes through", configured, configured}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got, err := buildVendorData(tc.configured, false) + if err != nil { + t.Fatalf("buildVendorData() error = %v", err) + } + if got != tc.want { + t.Errorf("vendor-data = %q, want %q", got, tc.want) + } + if strings.Contains(got, "qemu-guest-agent") { + t.Error("agent script present although installGuestAgent is off") + } + }) + } +} + +// Both documents have to survive in one file, and cloud-init only looks for +// the multipart marker in the first 4096 bytes. +func TestBuildVendorData_multipartCarriesBoth(t *testing.T) { + t.Parallel() + configured := "#cloud-config\nntp:\n enabled: true\n" + got, err := buildVendorData(configured, true) + if err != nil { + t.Fatalf("buildVendorData() error = %v", err) + } + head := got + if len(head) > 4096 { + head = head[:4096] + } + if !strings.Contains(strings.ToLower(head), "mime-version:") { + t.Fatalf("no mime-version: in the first 4096 bytes; cloud-init would treat this as a single document:\n%s", head) + } + _, params, err := mime.ParseMediaType(strings.SplitN(got, "\r\n\r\n", 2)[0][strings.Index(got, "Content-Type:")+len("Content-Type: "):]) + if err != nil { + t.Fatalf("parsing the multipart content type: %v", err) + } + body := strings.SplitN(got, "\r\n\r\n", 2)[1] + r := multipart.NewReader(strings.NewReader(body), params["boundary"]) + var bodies, types []string + for { + part, err := r.NextPart() + if err == io.EOF { + break + } + if err != nil { + t.Fatalf("reading part: %v", err) + } + content, err := io.ReadAll(part) + if err != nil { + t.Fatalf("reading part body: %v", err) + } + bodies = append(bodies, string(content)) + types = append(types, part.Header.Get("Content-Type")) + } + if len(bodies) != 2 || bodies[0] != configured || bodies[1] != guestAgentScript { + t.Fatalf("parts = %q, want the configured document then the script", bodies) + } + // text/plain makes cloud-init re-sniff the user's document from its own + // header line, so this code never has to classify it. + if types[0] != "text/plain" || types[1] != "text/x-shellscript" { + t.Errorf("part types = %v, want [text/plain text/x-shellscript]", types) + } +}