From 48a48126de0b4dd3b75ebaec0c78b18902a301af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20ROBERT?= Date: Fri, 28 Aug 2026 20:05:34 +0200 Subject: [PATCH 1/6] fix(machine): the published address moves onto the interface that wears the rule sets, and what a machine answers on stops being a guess (#548) A Scaleway server created *with* its flexible IP boots carrying only that address, so the driver gives it a routed NIC (#202), and Incus accepts no security option on one at all (#337). The private NIC arrived afterwards, took the rule set, and left the published address on the bare interface. Measured 2026-08-27 on examples/stacks/scaleway, and reproduced from the API alone on 2026-08-28 in both driver modes: a port the group's drop default never opened answered from the station, with a listener proved inside the machine and a bare port as the negative control. RouteAddress releases the address from the routed device without removing the device, which is the only one of three remedies Incus 7.2 accepts on a running instance, then hands it to the interface the pack named. Read on the NIC after the move, in both modes: eth0 routed and addressless, eth1 carrying the address and security.acls. From the station: 443 open, the port no rule names closed with its listener still running, the bare port as the control. Two things had to be settled first, and neither is a cost: - Inspect answered one address, the first of the lowest-named interface, and the layer above then decided its *kind* from the pack's declared block. The two agreed only while a routed NIC sorted before a managed one. After the move both share eth1, so the old reading would have published the private address where it published the public one, for three packs at once. The driver reports every address and settles nothing; PublicAddressOf and PrivateAddressOf pick out of that set by the block. - A NIC attached to a running machine is configured inside the guest by this driver, and nothing in the guest remembers that across a reboot: the machine came back with no address on it, the ninety-second wait gave up on a lease nobody offers, and the published address stopped answering. The start path restores what the device reserves, in both modes, before it waits. tools/conformance/functional.sh stops skipping the public half by naming capabilities.firewall_public_only and this issue: it asserts the same pair the firewall family does, on the address the API publishes, gated on the runtime declaring capabilities.firewall_public_when_joined (health schema 7). Both example stacks gained a listener on a port no rule of their web group names, which is what a closed half needs to be told from a dead service. Assisted-by: Claude Code (claude-opus-5) --- CHANGELOG.fr.md | 65 +++++ CHANGELOG.md | 62 +++++ docs/limits.md | 228 ++++++++++-------- examples/stacks/outscale/main.tf | 19 ++ examples/stacks/outscale/proof.json | 13 + examples/stacks/scaleway/main.tf | 19 ++ examples/stacks/scaleway/proof.json | 16 ++ internal/cli/driver_surface_test.go | 16 +- internal/cli/testdata/frozen/health.json | 103 ++++++++ internal/core/emulator/schema.go | 15 +- internal/core/machine/address_kind_test.go | 121 ++++++++++ internal/core/machine/binding.go | 98 ++++++-- internal/core/machine/binding_boot_test.go | 2 +- internal/core/machine/capabilities.go | 56 ++++- internal/core/machine/incus.go | 65 +++-- internal/core/machine/incus_address.go | 129 +++++++--- .../core/machine/incus_address_replay_test.go | 184 ++++++++++++-- internal/core/machine/incus_firewall.go | 11 +- internal/core/machine/incus_firewall_test.go | 50 ++++ internal/core/machine/incus_ovn.go | 78 +++++- internal/core/machine/incus_restart_test.go | 76 +++++- internal/core/machine/machine.go | 22 +- internal/core/machine/ownership_test.go | 2 +- internal/core/machine/plan.go | 57 +++-- internal/core/machine/plan_test.go | 39 +++ internal/core/machine/recorder.go | 7 +- internal/core/machine/recorder_test.go | 4 +- internal/core/machine/verify.go | 6 + .../exoscale/elasticip_routing_test.go | 2 +- .../exoscale/machines_internal_test.go | 2 +- .../exoscale/pools_machines_internal_test.go | 2 +- internal/providers/outscale/audit_test.go | 2 +- .../outscale/machines_internal_test.go | 2 +- .../outscale/publicip_routing_test.go | 4 +- internal/providers/scaleway/barrage_test.go | 4 +- .../providers/scaleway/concurrency_test.go | 4 +- tools/conformance/functional.sh | 47 +++- tools/falsify/specs/address-kind.json | 8 +- tools/falsify/specs/balancer-dataplane.json | 4 +- tools/falsify/specs/interface-plan.json | 4 +- .../specs/lifecycle-tells-the-truth.json | 28 +-- .../falsify/specs/pack-firewall-handoff.json | 4 +- .../specs/public-address-migration.json | 56 +++++ tools/falsify/specs/routed-nic.json | 4 +- 44 files changed, 1457 insertions(+), 283 deletions(-) create mode 100644 internal/core/machine/address_kind_test.go create mode 100644 tools/falsify/specs/public-address-migration.json diff --git a/CHANGELOG.fr.md b/CHANGELOG.fr.md index e5e224a6..8059443e 100644 --- a/CHANGELOG.fr.md +++ b/CHANGELOG.fr.md @@ -19,6 +19,21 @@ change ni l'un ni l'autre a sa place dans `git log`. ### Ajouté +- **Un mot pour la forme que le vocabulaire ne savait pas décrire : + `capabilities.firewall_public_when_joined`, et `/_feint/health` passe au + schéma 7 (#548).** Un consommateur qui lisait `capabilities.firewall: true`, + `enforced.firewall` et `firewall_public_only: false` en concluait que le seul + cas non couvert était la machine sans réseau privé. C'était faux, et le + correctif ci-dessous est la raison d'être du champ : une machine qui rejoint + un réseau émulé se retrouve désormais avec son adresse publiée sur + l'interface qui porte ses jeux de règles, et c'est une affirmation + différente de celle que la version 5 avait ajoutée. Les deux sont vraies en + même temps et les deux sont nécessaires : l'ancien champ reste `false`, parce + qu'une machine dont la seule interface est routée n'a nulle part où déplacer + une adresse. Le nouveau parle du runtime et non de chaque pack : un pack qui + ne nomme aucun réseau émulé pour ses adresses publiques les garde sur une NIC + routée, ce que fait Exoscale à dessein. + - **Les stacks d'exemple sont appliquées à de vraies machines chaque nuit, et la porte qui le fait dit combien de fois (#504).** `conformance:functional` est la seule chose ici qui applique `examples/stacks/` contre un runtime, et @@ -38,6 +53,56 @@ change ni l'un ni l'autre a sa place dans `git log`. ### Corrigé +- **Un serveur créé avec son IP publique ne garde plus une interface non + filtrée à côté de sa filtrée (#548).** Créé *avec* un `ip_id`, un serveur + Scaleway démarre en ne portant que cette adresse : le pilote lui donne donc + une NIC routée (#202), une interface sur laquelle Incus refuse toute option + de sécurité (#337). La NIC privée arrivait ensuite, prenait le jeu de règles, + et laissait l'adresse publiée sur l'interface nue. Mesuré le 2026-08-27 sur + `examples/stacks/scaleway`, puis reproduit depuis la seule API le 2026-08-28 + dans **les deux** modes du pilote : un port que la politique `drop` du groupe + n'ouvre jamais répondait depuis la station, avec un écouteur prouvé à + l'intérieur de la machine et un port nu comme contrôle négatif. + + L'adresse est déplacée désormais. `RouteAddress` la retire du device routé + sans retirer le device (les deux autres remèdes ont été essayés et refusés, + et `docs/limits.md` conserve les deux : l'uplink ne peut pas prendre le `/32` + tant que la NIC routée détient la route hôte, et retirer le device démasque + l'`eth0` du profil sur le pont de l'opérateur), puis la confie à l'interface + que le pack a nommée, celle qui porte les jeux de règles. Après le + déplacement, dans les deux modes : 443 ouvert parce qu'une règle l'ouvre, le + port qu'aucune règle ne nomme fermé alors que son écouteur tourne toujours, + et le port nu comme contrôle. Le device routé reste sur l'instance sans + adresse, et une NIC routée qui ne porte rien n'est plus signalée comme une + échappée. + +- **Ce sur quoi une machine répond se lit, ne se devine pas, et une machine + redémarrée retrouve son interface (#548).** `Inspect` répondait une adresse, + la première de l'interface au nom le plus bas, et la couche du dessus + décidait ensuite de quelle *espèce* elle était d'après le bloc public déclaré + par le pack. Les deux ne coïncidaient que tant qu'une NIC routée triait avant + une NIC managée, ce à quoi le déplacement ci-dessus met fin : après lui, le + runtime répond `{"eth0":[],"eth1":["10.199.0.2","203.0.113.2"]}`, et + l'ancienne lecture aurait publié l'adresse privée là où elle publiait la + publique, pour les trois packs d'un coup puisque `Binding` est la couche + partagée et que le pack Exoscale la lit. Le pilote rapporte toutes les + adresses qu'il a vues et ne tranche rien ; `Reconciler.PublicAddressOf` et + `PrivateAddressOf` choisissent dans cet ensemble d'après le bloc, qui est la + déclaration du pack. Rien ne change pour un client, et une entrée qu'un + snapshot restauré aurait posée là sans être une adresse est écartée au lieu + d'être publiée : seule la moitié publique passait jusqu'ici par un parseur. + + La moitié « redémarrage » a été mesurée le même jour et devait être corrigée + pour que le déplacement survive à un reboot : une NIC attachée à une machine + qui tourne est configurée dans l'invité par le pilote, rien dans l'invité ne + s'en souvient au redémarrage, et la machine revenait sans aucune adresse sur + cette interface, l'attente de quatre-vingt-dix secondes du pilote abandonnant + sur un bail que personne n'offre, dans les deux modes. Tant que l'adresse + publiée voyageait sur sa propre NIC routée, cela ne coûtait que les routes + vers les sous-réseaux appairés (#549) ; dès qu'elle vit sur cette interface, + cela coûte l'adresse elle-même. Le chemin de démarrage restitue donc ce que + le device réserve avant d'attendre. + - **Un arrêt propre rend toute la plomberie qu'aucun client ne peut supprimer, et l'exécution qui fuit est celle qui rougit (#521).** La jambe incus-ovn de `runtime-proof.yml` a échoué au portillon de l'étape suivante sur un runner diff --git a/CHANGELOG.md b/CHANGELOG.md index a8ec37e6..40228ad7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,20 @@ what this project is judged on: **a response shape a client can observe**, and ### Added +- **A word for the shape the vocabulary could not describe: + `capabilities.firewall_public_when_joined`, and `/_feint/health` moves to + schema 7 (#548).** A consumer holding `capabilities.firewall: true`, + `enforced.firewall` and `firewall_public_only: false` concluded that the one + uncovered case was the machine with no private network. It was not, and the + fix below is why the field exists: a machine that *does* join an emulated + network now ends up with its published address on the interface that wears + its rule sets, and that is a different claim from the one version 5 added. + Both are true at once, and both are needed: the older field is still `false`, + because a machine whose only interface is routed has nowhere to move an + address to. The new one is a claim about the runtime and not about every + pack — a pack that names no emulated network for its public addresses keeps + them on a routed NIC, which Exoscale does on purpose. + - **The example stacks are applied to real machines every night, and the gate that does it says how many times (#504).** `conformance:functional` is the only thing here that applies `examples/stacks/` against a runtime, and @@ -53,6 +67,54 @@ what this project is judged on: **a response shape a client can observe**, and ### Fixed +- **A server created with its public IP no longer keeps an unfiltered + interface beside its filtered one (#548).** Created *with* an `ip_id`, a + Scaleway server boots carrying only that address, so the driver gives it a + routed NIC (#202) — an interface Incus accepts no security option on at all + (#337). The private NIC arrived afterwards, took the rule set, and left the + published address on the bare one: measured 2026-08-27 on + `examples/stacks/scaleway` and reproduced from the API alone on 2026-08-28 in + **both** driver modes, a port the group's `drop` default never opened + answered from the station, with a listener proved inside the machine and a + bare port as the negative control. + + The address moves now. `RouteAddress` releases it from the routed device + without removing the device — the two other remedies were tried and refused, + and `docs/limits.md` keeps both: the uplink cannot take the `/32` while the + routed NIC owns the host route, and removing the device unmasks the + profile's `eth0` on the operator's own bridge — then hands it to the + interface the pack named, which wears the rule sets. After it, in both modes: + 443 open because a rule opens it, the port no rule names closed with its + listener still running, and the bare port as the control. The routed device + stays on the instance with no address, and a routed NIC that carries nothing + is no longer reported as an escape. + +- **What a machine answers on is read, not guessed, and a restarted machine + gets its interface back (#548).** `Inspect` used to answer one address — the + first of the lowest-named interface — and the layer above then decided what + *kind* of address that was from the pack's declared public block. The two + agreed only while a routed NIC sorted before a managed one, which is exactly + what the move above ends: after it the runtime answers + `{"eth0":[],"eth1":["10.199.0.2","203.0.113.2"]}`, and the old reading would + have published the private address where it published the public one — for + three packs at once, since `Binding` is the shared layer and the Exoscale + pack reads it. The driver reports every address it saw and settles nothing; + `Reconciler.PublicAddressOf` and `PrivateAddressOf` pick out of that set by + the block, which is the pack's own declaration. Nothing a client sees + changes, and an entry a restored snapshot put there that is not an address is + now dropped rather than published — only the public half ever went through a + parser. + + The restart half was measured the same day and had to be fixed for the move + to survive one: a NIC attached to a running machine is configured inside the + guest by the driver, nothing in the guest remembers that across a reboot, and + the machine came back with no address on that interface at all — the driver's + own ninety-second wait giving up on a lease nobody offers, in both modes. + While the published address rode a routed NIC of its own that cost only the + routes to the peered subnets (#549); once it lives on that interface it costs + the address itself. The start path restores what the device reserves before + it waits. + - **A graceful exit gives back every piece of host plumbing no client can delete, and a run that leaks is the run that goes red (#521).** The incus-ovn leg of `runtime-proof.yml` failed at the doorstep of the next step on a diff --git a/docs/limits.md b/docs/limits.md index 89e41afb..a55776bb 100644 --- a/docs/limits.md +++ b/docs/limits.md @@ -1398,15 +1398,19 @@ to everything but the foreign subnets; the bridge-mode isolation set keeps its catch-all, because there the network ACL filters at the bridge-host boundary and would otherwise reject the station itself. -**One leg of that pass does not hold, and it is the Scaleway one from the -station (#548).** Re-measured on 2026-08-27, on the same stack under the same -runtime: the `platform-web` group opens 443 and 22-from-10.40.1.0/24 and names -no port 80, a service was proved listening on 80 inside the machine, and the -station reached `203.0.113.3:80` and `203.0.113.2:80`. The negative control is -in the same pass — the bastion, the same shape of machine, refuses 80 because -nothing listens there — so those two are completed connections rather than a -misread probe. The cause is #548's routed NIC, which carries the published -address and no rule set. +**One leg of that pass did not hold, and it is the Scaleway one from the +station (#548). It holds since 2026-08-28.** Measured on 2026-08-27, on the +same stack under the same runtime: the `platform-web` group opens 443 and +22-from-10.40.1.0/24 and names no port 80, a service was proved listening on +80 inside the machine, and the station reached `203.0.113.3:80` and +`203.0.113.2:80`. The negative control is in the same pass — the bastion, the +same shape of machine, refuses 80 because nothing listens there — so those two +were completed connections rather than a misread probe. The cause was #548's +routed NIC, which carried the published address and no rule set; the driver +now moves that address onto the filtered NIC as soon as the machine has one, +and the same probe refuses. The measurement of the remedy, in both driver +modes, is under "Two migrations were tried and refused, and a third one +works" below. Measured: the three probes and the device dump in #548. Deduced, and worth stating as a deduction: `9999` is a port nothing listens on, so its refusal @@ -1467,19 +1471,15 @@ of "filters nothing" and why an ordinary `scw instance server create` raises no alarm at all. A server *with* a private network keeps the claim **on its emulated -interfaces**, and whether its public address is covered depends on the order -in which the two arrived. That sentence used to read "keeps the full claim … -and a flexible IP, routed through the filtered NIC, stays covered", and a -measurement on 2026-08-27 contradicted it for the order the example stack -produces (#548): +interfaces and on its public address**, whichever order the two arrived in. +That took two corrections, and both are worth keeping in view. -| order | what the machine carries | the public address | -|---|---|---| -| private NIC first, address attached afterwards | one managed NIC, the address on its `ipv4.routes` | **covered** — the paragraph below measures it, fifteen runs both ways | -| address at creation, private NIC afterwards | `eth0` `nictype=routed` **and** `eth1` on a managed network | **not covered**: `eth0` accepts no security option at all (#337), so it carries no rule set | - -The second row is what `examples/stacks/scaleway` builds, since a server -created with an `ip_id` boots carrying only its public address: +The sentence used to read "keeps the full claim … and a flexible IP, routed +through the filtered NIC, stays covered", and a measurement on 2026-08-27 +contradicted it for the order the example stack produces (#548): a server +created with an `ip_id` boots carrying only its public address, so the driver +gives it a routed NIC, and the private NIC that arrives afterwards did not +take the address with it. ```console $ incus query /1.0/instances/feint-scw-936e816e-… | jq -c '.expanded_devices | with_entries(select(.value.type=="nic"))' @@ -1492,11 +1492,20 @@ $ incus query /1.0/instances/feint-scw-936e816e-… | jq -c '.expanded_devices | platform-bastion 203.0.113.4:80 closed # nothing listens: the negative control ``` -So `capabilities.firewall_public_only: false` is broader than the sentence it -was written for: it is not only the server whose *only* interface is routed, -it is any address living on a routed NIC. `tools/conformance/functional.sh` -keys its skip on that capability for exactly this reason and probes the -firewall pair on the emulated network instead. +| order | what the machine carries | the public address | +|---|---|---| +| private NIC first, address attached afterwards | one managed NIC, the address on its `ipv4.routes` | **covered** — fifteen runs both ways, the paragraph below | +| address at creation, private NIC afterwards | `eth0` `nictype=routed` addressless, `eth1` on a managed network carrying the address | **covered since 2026-08-28**: the driver moves the address onto the filtered NIC when the machine joins one (#548) | +| address at creation, no private network ever | `eth0` `nictype=routed` carrying the address | **not covered**: a routed NIC accepts no security option at all (#337), and there is no other interface to move to | + +So the two capabilities say two different things, and a consumer needs both. +`capabilities.firewall_public_only: false` is about the *interface*: an address +living on a routed NIC is covered by nothing, and that is the third row. +`capabilities.firewall_public_when_joined: true` is the second row — a machine +that has an emulated interface ends up with its public address on it. +`tools/conformance/functional.sh` keyed a skip on the first of those two for as +long as the second was missing; it asserts the public pair now, on the address +the API publishes, and gates it on the second capability. The group covers a **flexible IP**. The address is routed through the NIC device's `ipv4.routes` (`nic_bridged.go` at v7.2.0 lists it among the device's @@ -3253,17 +3262,20 @@ mean the same thing to whoever is about to open a socket. **The two bounds, both measured.** First, an interface the runtime declares unenforceable stays unenforceable: a routed NIC accepts no security option (#337, `capabilities.firewall_public_only: false`), and that is the *primary* -interface of every Exoscale instance, of every Scaleway server whose only -address is public, **and of every Scaleway server created with its address, -whose private NIC arrives afterwards and does not take the address with it** -(#548, measured 2026-08-27 on `examples/stacks/scaleway`: `eth0` routed and -bare beside `eth1` on a managed network carrying the rule set). The pack hands -the set over, the driver refuses with the typed error, and the log names the -declaring capability instead of crying wolf. The capability is therefore -broader than the machine it was written for: it is about the *interface*, not -about a machine with only one — and `machine.Capabilities.FirewallPublicOnly` -says so in its own words since #548, because a declaration whose subject is -wrong reads like proof. +interface of every Exoscale instance and of every Scaleway server whose only +address is public. The pack hands the set over, the driver refuses with the +typed error, and the log names the declaring capability instead of crying +wolf. The capability is about the *interface*, not about a machine with only +one — and `machine.Capabilities.FirewallPublicOnly` says so in its own words +since #548, because a declaration whose subject is wrong reads like proof. + +Until 2026-08-28 that list had a third member, and it is the one #548 was +filed on: a Scaleway server created *with* its address, whose private NIC +arrives afterwards and did not take the address with it (measured 2026-08-27 +on `examples/stacks/scaleway`: `eth0` routed and bare beside `eth1` on a +managed network carrying the rule set). That machine is covered now — the +driver moves the address onto the filtered NIC — and the paragraph after the +next one carries the before-and-after in both driver modes. Reproduced from the API alone on 2026-08-27, without the stack, under `--vm incus-ovn`: a group whose inbound default is `drop` with one rule @@ -3284,11 +3296,11 @@ $ incus query /1.0/instances/feint-scw-d3eaa40c-… | jq -c '.expanded_devices | packet reached the guest and was refused by it, where a covered interface would have dropped it. -**Two migrations were tried and refused, and a third one works.** #548 left one -thing untried — whether the driver could move the address onto the managed NIC -once that one arrives, which is the shape the other creation order already -produces and the one the rule set covers. Three attempts, and the third is the -reason this paragraph changed on 2026-08-28. +**Two migrations were tried and refused, and a third one works — and ships.** +#548 left one thing untried — whether the driver could move the address onto +the managed NIC once that one arrives, which is the shape the other creation +order already produces and the one the rule set covers. Three attempts, and +the third is what `RouteAddress` does today. The first two, by hand on 2026-08-27: @@ -3304,76 +3316,100 @@ The uplink cannot be given the `/32` while the routed NIC still owns the host route for it — the collision #498 documents, met from the other side — and removing the routed device unmasks the profile's `eth0` on `incusbr0`, the operator's own default bridge, which this emulator refuses to put anything on. +**Neither is a remedy, and neither should be proposed again.** Both come from one place: the host route. **The third attempt takes the address off the device without taking the device off the instance**, which is neither of -the two, and Incus 7.2 accepts it on a running instance. Measured on 2026-08-28, -under `--vm incus-ovn`, on a server created with its flexible IP whose private -NIC arrived afterwards — the exact shape above: +the two, and Incus 7.2 accepts it on a running instance: ```console -$ ip route show | grep 203.0.113.2 -203.0.113.2 dev veth030d08f6 scope link -$ incus config device set feint-scw-21c968ca-… eth0 ipv4.address= # 1 -$ ip route show | grep 203.0.113.2 # gone -$ incus network set feint-uplink ipv4.routes "10.199.0.0/24,203.0.113.2/32" # 2, no longer refused -$ incus config device set feint-scw-21c968ca-… eth1 \ - ipv4.routes.external=203.0.113.2/32 # 3 -$ incus query /1.0/instances/feint-scw-21c968ca-… | jq -c '…' -{"eth0":{"ipv4.host_address":"169.254.0.1","nictype":"routed","type":"nic"}, - "eth1":{"ipv4.address":"10.199.0.2","ipv4.routes.external":"203.0.113.2/32", - "network":"fnt-0da7a7bda1e","security.acls":"scw-892dbc3d91e","type":"nic"}} +$ incus config device set eth0 ipv4.address= # 1, the host route goes +$ incus network set feint-uplink ipv4.routes "…,203.0.113.2/32" # 2, no longer refused +$ incus config device set eth1 \ + ipv4.routes.external=203.0.113.2/32 # 3, routeAddressOVN's own gesture ``` Step 1 releases the address, step 2 is the first refusal now unblocked, step 3 -is `routeAddressOVN`'s own gesture, and the guest is then repaired the way that -function already repairs one. The device stays, so `incusbr0` is never unmasked. +is the address arriving on the interface that wears the rule sets. The device +stays, so `incusbr0` is never unmasked. In the bridge mode the third step is +`ipv4.routes` on the managed device instead, which is that mode's own way of +carrying a public address, and the first two are unchanged. -**The coverage is real, and the probe tells the two answers apart.** The group's -inbound default is `drop` with one rule allowing 443, and a listener sits on both -ports, so a refusal cannot be mistaken for an empty port: +**Before and after, in both modes, read on the NIC and from the station.** The +shape is #548's, reproduced from the API alone: a group whose inbound default +is `drop` with one rule allowing 443, a server created with its flexible IP, +its private NIC attached afterwards, a listener on 443 and one on 80, and 8080 +bare as the negative control. Measured 2026-08-28. ```console - 203.0.113.2:443 connect_ex=0 OPEN # a rule opens it, the listener answers - 203.0.113.2:80 connect_ex=111 refused # a listener is there, no rule is - 203.0.113.2:8080 connect_ex=111 refused # no listener: the negative control +--vm incus-ovn, before --vm incus-ovn, after +eth0 routed ipv4.address=203.0.113.2 eth0 routed (no address) + no security.acls no security.acls, and nothing to cover +eth1 network=fnt-… eth1 network=fnt-… + ipv4.address=10.199.0.2 ipv4.address=10.199.0.2 + security.acls=scw-… ipv4.routes.external=203.0.113.2/32 + security.acls=scw-… + + 203.0.113.2:443 OPEN 203.0.113.2:443 OPEN a rule opens it + 203.0.113.2:80 OPEN ← the escape 203.0.113.2:80 refused a listener is there, no rule is + 203.0.113.2:8080 refused 203.0.113.2:8080 refused no listener: the negative control + +--vm incus, before --vm incus, after +eth0 routed ipv4.address=203.0.113.2 eth0 routed (no address) +eth1 network=fnt-… security.acls=scw-… eth1 network=fnt-… ipv4.routes=203.0.113.2/32 + security.acls=scw-… + + 203.0.113.2:443 OPEN 203.0.113.2:443 OPEN + 203.0.113.2:80 OPEN ← the escape 203.0.113.2:80 timed out (the bridge default is drop) + 203.0.113.2:8080 refused 203.0.113.2:8080 timed out ``` -Before the migration, on the same machine, 80 was **OPEN** — that is the escape -this section describes. And the machine keeps its way out: `ipv4.nat=true` on the -OVN network, `ping 1.1.1.1` answers from inside, with the station as the control. - -**So this is a remedy, not an impossibility — and it is still not shipped, for -two reasons that are measurements rather than reluctance.** - -- **What a machine "answers on" moves.** `Incus.Inspect` reports the first - global IPv4 of the lowest-named interface, and after the migration the public - and private addresses share `eth1`: the runtime answers - `{"eth0":[],"eth1":["10.199.0.2","203.0.113.2"]}`, stable across three reads, - so `Binding.Address` and `Started.Address` would report the private address - where they used to report the public one. The Scaleway API is unaffected — - it publishes the flexible IP from its own store, and it still answered - `public_ip: 203.0.113.2` throughout — but `Binding.Address` is the shared - layer, and the Exoscale pack reads it (`machines.go`, the membership - attachment). A fix has to say which of a machine's addresses is the one it - answers on, for three packs at once. -- **Only OVN was measured.** The bridge mode delivers a public address through - `ipv4.routes` on the device rather than `ipv4.routes.external`, and nothing - above was run under `--vm incus`. - -Until those two are settled the bound in the table stands, and -`tools/conformance/functional.sh` goes on skipping the public half by naming -`capabilities.firewall_public_only` and #548. - -What #548 delivered instead is the naming: the refusal now carries every -routed interface that escapes *and the addresses it delivers* +Two readings of that table are worth spelling out. The verdict is taken on the +**NIC** and not only on the connection: `security.acls` is on the interface +that carries the address in the "after" column, which is what tells this apart +from a connection that happens to fail. And the bridge column's refusals are +timeouts rather than resets, because a bridged NIC's default action drops where +OVN's isolation set rejects — so on that mode the negative control cannot tell +"no listener" from "dropped", and the pair that carries the verdict is 443 open +beside 80 closed with **both** proved listening inside the machine. + +**What a restart costs, and what had to be fixed for the move to survive one.** +The address now lives on the interface a hot attach created, and nothing inside +a guest remembers what this driver configured on it: measured on 2026-08-28, +a machine rebooted through the API came back with no address at all on `eth1`, +the driver's own wait ran its ninety seconds and gave up with `it carries no +IPv4 address`, and the published address stopped answering — 443 timed out +where it had been open, in both modes. The cause was named by repairing it by +hand, in three steps, until the symptom went: restoring the interface's private +address alone did not bring 443 back, and a route towards the station's block +did. So the restart path restores the address a NIC device reserves (both +modes) before it waits for a lease nobody offers, and the routes towards the +peered subnets follow as before (#549). Re-measured after that: 443 open, 80 +refused, 8080 refused, identical before and after the restart. + +**What is still not delivered on this shape.** The machine has no way out: a +routed NIC has no NAT, its default route points at the link-local host address, +and `ping 1.1.1.1` from inside answers nothing — before the migration and after +it, with the station as the control in both passes, so the move neither gave +nor took that away. A guest that needs outbound access wants a default route +through its emulated network, which this driver deliberately does not invent +(see `repairGuestInterface`: inventing one would route a machine the control +plane declared isolated). The `#507` bound therefore stands unchanged. + +**What #548 delivered before the remedy, and keeps.** The refusal a routed NIC +still earns names every escaping interface *and the addresses it delivers* (`eth0 (203.0.113.2)`), read from both `ipv4.address` and `ipv4.routes` so an -address attached after the boot is named too, and the warning no longer calls -the machine public-only. -`TestTheUnenforceableRefusalNamesTheAddressThatEscapes`, +address attached after the boot is named too, and the warning does not call the +machine public-only. That is what an operator reads on the one shape the +remedy cannot reach — a machine with no emulated network to move the address +onto. `TestTheUnenforceableRefusalNamesTheAddressThatEscapes`, `TestTheUnenforceableRefusalNamesAnAddressRoutedAfterTheLaunch` and `TestTheUnenforceableWarningDoesNotCallTheMachinePublicOnly` fail without it, -and `tools/falsify/specs/uncovered-interface.json` replays all three. +and `tools/falsify/specs/uncovered-interface.json` replays all three. A routed +NIC that carries *nothing* is not named at all, and that half is +`TestARoutedNICThatCarriesNothingIsNotAnEscape`: after the move the device +stays on the instance with no address, and reporting it would be describing an +escape that had been closed. Second, between two machines of one subnet the sender's permissive egress still wins over the receiver's ingress default (the single-pipeline diff --git a/examples/stacks/outscale/main.tf b/examples/stacks/outscale/main.tf index 71dccc91..9c8b8d7f 100644 --- a/examples/stacks/outscale/main.tf +++ b/examples/stacks/outscale/main.tf @@ -368,6 +368,13 @@ resource "outscale_vm" "web" { # # No `packages:`: the same #507 bound the Scaleway stack documents. python3 # is guaranteed wherever cloud-init runs, since cloud-init is python. + # The second listener is the public path's closed half (#548): 80 is the port + # `outscale_security_group_rule.web_http` opens, 9090 is a metrics port no + # rule of this group names, and both live on the machine whose PublicIp the + # station dials. This cloud reaches the assertion by another road than + # Scaleway — a Vm is born on its Subnet, so its public address rides a + # managed NIC from the first boot and never needed moving — which is what + # makes it the control population for that change. user_data = base64encode(<<-EOT #cloud-config write_files: @@ -382,11 +389,23 @@ resource "outscale_vm" "web" { Restart=always [Install] WantedBy=multi-user.target + - path: /etc/systemd/system/platform-web-metrics.service + content: | + [Unit] + Description=platform web metrics, private to the tier + After=network-online.target + [Service] + WorkingDirectory=/var/www/html + ExecStart=/usr/bin/python3 -m http.server 9090 + Restart=always + [Install] + WantedBy=multi-user.target runcmd: - [mkdir, -p, /var/www/html] - [sh, -c, "hostname > /var/www/html/index.html"] - [systemctl, daemon-reload] - [systemctl, enable, --now, platform-web.service] + - [systemctl, enable, --now, platform-web-metrics.service] EOT ) diff --git a/examples/stacks/outscale/proof.json b/examples/stacks/outscale/proof.json index 7d49419c..3ad41a9b 100644 --- a/examples/stacks/outscale/proof.json +++ b/examples/stacks/outscale/proof.json @@ -21,6 +21,19 @@ ], "unit": "platform-web.service", "port": 80, + + "_closed_port": [ + "The public path's closed half (#548), the same shape the Scaleway stack", + "declares: 80 is opened by outscale_security_group_rule.web_http, 9090 is", + "named by no rule, both listen on the machine whose PublicIp the station", + "dials.", + "", + "This cloud is the control population for that change: a Vm is born on its", + "Subnet, so its public address has ridden a managed NIC since the first", + "boot and no migration was ever needed here. If this half went red while", + "Scaleway's passed, the cause would be the assertion and not the remedy." + ], + "closed_port": 9090, "machines": ["platform-web-a", "platform-web-b"], "restart": "platform-web-a", "restart_reaches": { diff --git a/examples/stacks/scaleway/main.tf b/examples/stacks/scaleway/main.tf index db71ddce..19754c7c 100644 --- a/examples/stacks/scaleway/main.tf +++ b/examples/stacks/scaleway/main.tf @@ -399,6 +399,13 @@ resource "scaleway_instance_server" "web" { # the emulator recorded in `Runtime.machine`: an answer therefore says which # machine served it, which is what a balancer assertion needs and what a # fixed string cannot give. + # The second listener is the app tier's pair, moved onto the public path + # (#548): 443 is the port this tier's own group opens, 9090 is a metrics port + # no rule of `scaleway_instance_security_group.web` names. Both on the + # machine that carries a flexible IP, so the closed half can be probed on the + # address a client actually dials — which is the half tools/conformance/ + # functional.sh skipped for as long as that address lived on a routed NIC + # nothing could filter. cloud_init = <<-EOT #cloud-config write_files: @@ -413,11 +420,23 @@ resource "scaleway_instance_server" "web" { Restart=always [Install] WantedBy=multi-user.target + - path: /etc/systemd/system/platform-web-metrics.service + content: | + [Unit] + Description=platform web metrics, private to the tier + After=network-online.target + [Service] + WorkingDirectory=/var/www/html + ExecStart=/usr/bin/python3 -m http.server 9090 + Restart=always + [Install] + WantedBy=multi-user.target runcmd: - [mkdir, -p, /var/www/html] - [sh, -c, "hostname > /var/www/html/index.html"] - [systemctl, daemon-reload] - [systemctl, enable, --now, platform-web.service] + - [systemctl, enable, --now, platform-web-metrics.service] EOT } diff --git a/examples/stacks/scaleway/proof.json b/examples/stacks/scaleway/proof.json index b9cbf6f4..3979cadb 100644 --- a/examples/stacks/scaleway/proof.json +++ b/examples/stacks/scaleway/proof.json @@ -57,6 +57,22 @@ ], "unit": "platform-web.service", "port": 443, + + "_closed_port": [ + "The public path's other half (#548). 443 is opened by", + "scaleway_instance_security_group.web and 9090 is named by no rule of it,", + "both are listening on the same machine, and the probe is the station", + "dialling the flexible IP — so the only thing that differs between the two", + "verdicts is the rule.", + "", + "It could not be asserted while the address rode a routed NIC: that", + "interface takes no security option at all, so 9090 answered from the", + "station under a group whose inbound default is drop (measured 2026-08-27", + "and again 2026-08-28, in both driver modes). The driver moves the address", + "onto the filtered NIC now, and the harness gates this on the runtime", + "saying so — capabilities.firewall_public_when_joined." + ], + "closed_port": 9090, "machines": ["platform-web-0", "platform-web-1"], "restart": "platform-web-0", "restart_reaches": { diff --git a/internal/cli/driver_surface_test.go b/internal/cli/driver_surface_test.go index 18d79c35..ab5df406 100644 --- a/internal/cli/driver_surface_test.go +++ b/internal/cli/driver_surface_test.go @@ -719,17 +719,17 @@ var mustStayOutside = []string{ // the surface on purpose: the plan's order — addresses, memberships, // firewall last — is a property of the runtime, and a pack that starts a // machine through the binding skips it. - "Binding.Start", "Binding.Stop", "Binding.Remove", "Binding.Address", + "Binding.Start", "Binding.Stop", "Binding.Remove", "Binding.Addresses", "Binding.Name", "Binding.PowerOn", "Binding.Refresh", "Binding.ForgetPlacements", - // The unkinded address reader (#541). It was in the surface until an - // Exoscale instance with no public IP published its private-network - // address as `public-ip`: the binding records whatever the runtime - // answered and says nothing about what kind of address it is, so every - // pack republishing it under a field whose name asserts one was asserting - // what nobody had checked. Reconciler.PublicAddressOf and + // The unkinded address reader (#541, plural since #548). It was in the + // surface until an Exoscale instance with no public IP published its + // private-network address as `public-ip`: the binding records whatever + // the runtime answered and says nothing about what kind of address it is, + // so every pack republishing it under a field whose name asserts one was + // asserting what nobody had checked. Reconciler.PublicAddressOf and // PrivateAddressOf are the doors, and this line is what stops the old one // from being quietly reopened. - "Binding.AddressOf", + "Binding.AddressesOf", "Binding.RouteAddress", "Binding.UnrouteAddress", "Binding.SyncRuleSet", "Binding.ApplyRuleSets", "Binding.DropRuleSet", "Binding.WithRuntime", diff --git a/internal/cli/testdata/frozen/health.json b/internal/cli/testdata/frozen/health.json index 1f7f7885..c892b5b9 100644 --- a/internal/cli/testdata/frozen/health.json +++ b/internal/cli/testdata/frozen/health.json @@ -489,6 +489,109 @@ } ] } + }, + { + "schema_version": 7, + "content": { + "fields": [ + { + "path": "capabilities", + "type": "object" + }, + { + "path": "capabilities.addresses", + "type": "bool" + }, + { + "path": "capabilities.balancing", + "type": "bool" + }, + { + "path": "capabilities.firewall", + "type": "bool" + }, + { + "path": "capabilities.firewall_public_only", + "type": "bool" + }, + { + "path": "capabilities.firewall_public_when_joined", + "type": "bool" + }, + { + "path": "capabilities.isolation", + "type": "bool" + }, + { + "path": "capabilities.machines", + "type": "bool" + }, + { + "path": "capabilities.own_kernel", + "type": "bool" + }, + { + "path": "capabilities.private_from_host", + "type": "bool" + }, + { + "path": "enforced", + "type": "object" + }, + { + "path": "enforced.balancing", + "type": "array" + }, + { + "path": "enforced.balancing[]", + "type": "string" + }, + { + "path": "enforced.firewall", + "type": "array" + }, + { + "path": "enforced.firewall[]", + "type": "string" + }, + { + "path": "instance", + "type": "object" + }, + { + "path": "instance.pid", + "type": "number" + }, + { + "path": "instance.started_at", + "type": "string" + }, + { + "path": "machines", + "type": "string" + }, + { + "path": "providers", + "type": "array" + }, + { + "path": "providers[]", + "type": "string" + }, + { + "path": "resources", + "type": "number" + }, + { + "path": "schema_version", + "type": "number" + }, + { + "path": "status", + "type": "string" + } + ] + } } ] } diff --git a/internal/core/emulator/schema.go b/internal/core/emulator/schema.go index 850c149e..e731a44f 100644 --- a/internal/core/emulator/schema.go +++ b/internal/core/emulator/schema.go @@ -63,7 +63,20 @@ const ( // suite that wants to assert distribution keys on the conjunction of the // two halves; a build that cannot answer the per-pack question is what the // version bump makes visible. - HealthSchemaVersion = 6 + // + // 7 since #548: `capabilities` gained `firewall_public_when_joined` — a + // public address is enforced once its machine also joins an emulated + // network, because the driver moves the address onto the filtered + // interface. Additive, and it closes a reading of version 5 that was + // measured wrong: a consumer holding `firewall_public_only: false` took + // the uncovered case to be the machine with no private network, and a + // Scaleway server created with its flexible IP is neither that machine nor + // covered — its address stayed on a routed NIC beside a filtered one, and + // a port its group never opened answered from the station in both driver + // modes on 2026-08-28. Two shapes, two words: this one is true, the older + // one is still false, and a build that cannot answer the second question + // is what the bump makes visible. + HealthSchemaVersion = 7 // RoutesSchemaVersion is the shape of GET /_feint/routes. // // This one is not on the wire: the endpoint answers a bare JSON array — the diff --git a/internal/core/machine/address_kind_test.go b/internal/core/machine/address_kind_test.go new file mode 100644 index 00000000..c294d4a0 --- /dev/null +++ b/internal/core/machine/address_kind_test.go @@ -0,0 +1,121 @@ +package machine + +import ( + "context" + "net/netip" + "testing" + + "github.com/stephrobert/feint/internal/core/resource" +) + +// Which address a machine "answers on" was a guess until #548, and the guess +// lived in two places at once: the driver picked one address off the +// lowest-named interface, and the layer above then decided what kind it was +// from the pack's declared public block. The two agreed only while a routed +// NIC sorted before a managed one — which is exactly the arrangement the +// address migration ends, since it puts both addresses on one interface. +// +// The measurement that settles it, 2026-08-28 under `--vm incus-ovn`, on a +// Scaleway server created with its flexible IP whose private NIC arrived +// afterwards and whose address was then migrated onto it: +// +// {"eth0":[],"eth1":["10.199.0.2","203.0.113.2"]} stable over three reads +// +// The old reading answered 10.199.0.2 — the private address — where it had +// answered 203.0.113.2, for three packs at once, and Exoscale publishes that +// value as `public-ip`. + +// twoAddressesOnOneInterface is that machine, as `incus list --format json` +// describes it: nothing on the routed NIC any more, both addresses on the +// managed one, the private one listed first. +const twoAddressesOnOneInterface = `[{ + "name": "feint-scw-mig", + "status": "Running", + "state": {"network": { + "lo": {"addresses": [{"family": "inet", "address": "127.0.0.1", "scope": "local"}]}, + "eth0": {"addresses": []}, + "eth1": {"addresses": [ + {"family": "inet", "address": "10.199.0.2", "scope": "global"}, + {"family": "inet", "address": "203.0.113.2", "scope": "global"}, + {"family": "inet6", "address": "fd42::2", "scope": "global"} + ]} + }} +}]` + +// TestInspectReportsEveryAddressTheMachineCarries: the driver answers the set +// and chooses nothing, so no reader downstream can inherit a choice nobody +// made. +func TestInspectReportsEveryAddressTheMachineCarries(t *testing.T) { + f := &fakeRuntime{answers: map[string]string{ + "list feint-scw-mig": twoAddressesOnOneInterface, + }} + d := newFakeDriver(f) + + m, ok, err := d.Inspect(context.Background(), "feint-scw-mig") + if err != nil || !ok { + t.Fatalf("inspect: ok=%v err=%v", ok, err) + } + if len(m.Addresses) != 2 { + t.Fatalf("addresses = %v, want both globals of the interface", m.Addresses) + } + // Both, in the order the runtime listed them within an interface, and the + // interfaces themselves in name order: a map has none, and an answer that + // changed between two reads of one machine would be worse than a guess. + if m.Addresses[0] != "10.199.0.2" || m.Addresses[1] != "203.0.113.2" { + t.Errorf("addresses = %v, want [10.199.0.2 203.0.113.2]", m.Addresses) + } + if !m.Running { + t.Errorf("the machine reads as not running") + } +} + +// And the layer picks out of that set by the pack's own declaration, which is +// the half that must not depend on which address came first. +func TestTheKindOfAnAddressIsTheBlocksAnswerNotTheOrders(t *testing.T) { + block := netip.MustParsePrefix("203.0.113.0/24") + for name, recorded := range map[string]string{ + "private first": "10.199.0.2,203.0.113.2", + "public first": "203.0.113.2,10.199.0.2", + } { + t.Run(name, func(t *testing.T) { + r := Reconciler{ + Groups: GroupSync{Binding: Binding{AddressKey: "address"}}, + PlanOf: func(*resource.Resource) Plan { return Plan{} }, + PublicBlock: block, + } + res := &resource.Resource{ID: "srv", Runtime: map[string]string{"address": recorded}} + + if got := r.PublicAddressOf(res); got != "203.0.113.2" { + t.Errorf("public = %q, want 203.0.113.2", got) + } + if got := r.PrivateAddressOf(res); got != "10.199.0.2" { + t.Errorf("private = %q, want 10.199.0.2", got) + } + }) + } +} + +// TestARestoredAddressThatIsNotAnAddressIsNotPublished: the recorded value +// comes back from Resource.Runtime, which a restored snapshot controls +// verbatim, and both readers above hand what they find to a client. Only the +// public half ever went through a parser, so a crafted entry reached +// Outscale's PrivateIp untouched. +func TestARestoredAddressThatIsNotAnAddressIsNotPublished(t *testing.T) { + r := Reconciler{ + Groups: GroupSync{Binding: Binding{AddressKey: "address"}}, + PlanOf: func(*resource.Resource) Plan { return Plan{} }, + PublicBlock: netip.MustParsePrefix("203.0.113.0/24"), + } + res := &resource.Resource{ID: "srv", Runtime: map[string]string{ + "address": ",not-an-address,10.199.0.2", + }} + + if got := r.PublicAddressOf(res); got != "" { + t.Errorf("public = %q, want nothing: no entry is inside the block", got) + } + // The one entry that parses is the one that is published, and the two that + // do not never reach a field a client reads. + if got := r.PrivateAddressOf(res); got != "10.199.0.2" { + t.Errorf("private = %q, want 10.199.0.2", got) + } +} diff --git a/internal/core/machine/binding.go b/internal/core/machine/binding.go index 8a1a1966..0a98b914 100644 --- a/internal/core/machine/binding.go +++ b/internal/core/machine/binding.go @@ -3,6 +3,7 @@ package machine import ( "context" "log/slog" + "net/netip" "strings" "github.com/stephrobert/feint/internal/core/cloudinit" @@ -60,7 +61,16 @@ type Binding struct { // belongs in Runtime and never in Attrs, so it cannot leak into an API // response, and the binding writes it rather than every pack remembering to. RuntimeKey string - // AddressKey is where the machine's address is kept, in Runtime. + // AddressKey is where the addresses the machine answers on are kept, in + // Runtime — every one of them, comma-separated, read back through + // storedAddresses. + // + // All of them since #548, and the plural is the whole point: one recorded + // address forced somebody to choose which, and the driver was choosing by + // interface order while the pack's own declaration — its emulated public + // block — already settled the kind. A machine whose public and private + // addresses share an interface makes the two answers differ, which is + // exactly the shape the address migration produces. // // Runtime and not Attrs, which was a mistake worth stating. The binding used // to write the address straight into an API field, which forced every pack @@ -208,8 +218,10 @@ type Boot struct { type Started struct { // Machine is the runtime name, for the pack to keep out of reach of the API. Machine string - // Address is what the machine answers on, empty when nothing started. - Address string + // Addresses are what the machine answers on, empty when nothing started. + // Every one of them, with no kind attached: which is public is settled by + // the pack's declared block, one layer up (Reconciler.PublicAddressOf). + Addresses []string } // Start powers a machine on. It never fails the caller: with no runtime the @@ -348,7 +360,7 @@ func (b Binding) Start(ctx context.Context, boot Boot) Started { "machine", name, "image", boot.Image, "error", err) return Started{} } - return Started{Machine: name, Address: m.IP} + return Started{Machine: name, Addresses: m.Addresses} } // refuseUnknownImage is the actionable half of the boot refusal: it names the @@ -465,14 +477,15 @@ func (b Binding) Remove(ctx context.Context, id, machine string) { } } -// Address asks the runtime what a machine answers on. +// Addresses asks the runtime what a machine answers on — all of it, with no +// kind attached. // // A container has its address immediately; a virtual machine gets one tens of // seconds later, once it has booted and DHCP has answered. Start therefore never // waits, and a pack calls this on the read a client is making anyway. -func (b Binding) Address(ctx context.Context, id, machine string) (string, bool) { +func (b Binding) Addresses(ctx context.Context, id, machine string) ([]string, bool) { if b.driver == nil || machine == "" { - return "", false + return nil, false } // Read-only, but still checked: inspecting an arbitrary instance of the host // would publish its address as if it were the emulated server's. @@ -481,12 +494,12 @@ func (b Binding) Address(ctx context.Context, id, machine string) (string, bool) if err != nil { b.logger().Error("could not inspect the backing machine", "provider", b.Provider, "resource", id, "machine", machine, "error", err) - return "", false + return nil, false } - if !ok || m.IP == "" { - return "", false + if !ok || len(m.Addresses) == 0 { + return nil, false } - return m.IP, true + return m.Addresses, true } func (b Binding) logger() *slog.Logger { @@ -528,8 +541,8 @@ func (b Binding) PowerOn(ctx context.Context, res *resource.Resource, boot Boot) res.Runtime = map[string]string{} } res.Runtime[b.RuntimeKey] = started.Machine - if started.Address != "" { - res.Runtime[b.AddressKey] = started.Address + if joined := joinAddresses(started.Addresses); joined != "" { + res.Runtime[b.AddressKey] = joined } res.State = b.RunningState return true @@ -562,14 +575,18 @@ func (b Binding) Refresh(ctx context.Context, res *resource.Resource) bool { if res.Runtime[b.AddressKey] != "" { return false } - address, found := b.Address(ctx, res.ID, res.Runtime[b.RuntimeKey]) + addresses, found := b.Addresses(ctx, res.ID, res.Runtime[b.RuntimeKey]) if !found { return false } + joined := joinAddresses(addresses) + if joined == "" { + return false + } if res.Runtime == nil { res.Runtime = map[string]string{} } - res.Runtime[b.AddressKey] = address + res.Runtime[b.AddressKey] = joined return true } @@ -580,11 +597,52 @@ func (b Binding) RefreshIfRunning(ctx context.Context, res *resource.Resource) b return res.State == b.RunningState && b.Refresh(ctx, res) } -// AddressOf is what the machine answers on, empty when nothing is running. A -// pack calls it to fill the field its own API declares for the address — or -// does not call it at all, when its API declares none. -func (b Binding) AddressOf(res *resource.Resource) string { - return res.Runtime[b.AddressKey] +// AddressesOf is what the machine answers on, empty when nothing is running. +// The layer above turns it into the field a pack's API declares, by kind +// (Reconciler.PublicAddressOf and PrivateAddressOf); no pack reads this. +// +// Every entry is parsed, and one that is not an address is dropped. That is a +// control and not tidiness: this value comes back from Resource.Runtime, which +// `PUT /_feint/state` and `feint snapshot load` restore verbatim — the format +// is documented as meant to outlive its instance and be loaded into another +// one — and both readers above hand what they find to a client. Until this, +// an entry no parser had ever looked at reached Outscale's PrivateIp field +// unchanged, because only the *public* half went through netip.ParseAddr. +// TestARestoredAddressThatIsNotAnAddressIsNotPublished fails without it. +func (b Binding) AddressesOf(res *resource.Resource) []string { + return storedAddresses(res.Runtime[b.AddressKey]) +} + +// storedAddresses reads the recorded list back, keeping only what parses as an +// IP address. +func storedAddresses(stored string) []string { + if stored == "" { + return nil + } + out := make([]string, 0, 2) + for _, entry := range strings.Split(stored, ",") { + entry = strings.TrimSpace(entry) + addr, err := netip.ParseAddr(entry) + if err != nil { + continue + } + out = append(out, addr.String()) + } + return out +} + +// joinAddresses is the write half of storedAddresses: the same filter, so +// nothing unparseable is ever recorded either. +func joinAddresses(addresses []string) string { + kept := make([]string, 0, len(addresses)) + for _, address := range addresses { + addr, err := netip.ParseAddr(strings.TrimSpace(address)) + if err != nil { + continue + } + kept = append(kept, addr.String()) + } + return strings.Join(kept, ",") } // declaresPackageStep reports whether a client's user data asks cloud-init for diff --git a/internal/core/machine/binding_boot_test.go b/internal/core/machine/binding_boot_test.go index 7a45126d..3e75f617 100644 --- a/internal/core/machine/binding_boot_test.go +++ b/internal/core/machine/binding_boot_test.go @@ -32,7 +32,7 @@ func (d *recordingDriver) Name() string { return "recording" } func (d *recordingDriver) Available(context.Context) bool { return true } func (d *recordingDriver) Start(_ context.Context, spec Spec) (Machine, error) { d.specs = append(d.specs, spec) - return Machine{Name: spec.Name, IP: "10.42.0.9", Running: true}, nil + return Machine{Name: spec.Name, Addresses: []string{"10.42.0.9"}, Running: true}, nil } func (d *recordingDriver) Stop(context.Context, string) error { return nil } func (d *recordingDriver) Remove(context.Context, string) error { return nil } diff --git a/internal/core/machine/capabilities.go b/internal/core/machine/capabilities.go index a0426527..d4752e3b 100644 --- a/internal/core/machine/capabilities.go +++ b/internal/core/machine/capabilities.go @@ -51,6 +51,44 @@ type Capabilities struct { // address published over a routed NIC gates on this claim, never on // capabilities.firewall. FirewallPublicOnly bool `json:"firewall_public_only"` + // FirewallPublicWhenJoined: a public address is enforced when the machine + // also joins an emulated network, because the driver moves the address off + // the routed NIC and onto the filtered one (#548). + // + // It is the word the vocabulary was missing, and the gap was measured + // before it was named. A consumer holding capabilities.firewall (true), + // enforced.firewall (the pack) and firewall_public_only (false) concluded + // that the uncovered case was the machine with no private network. It was + // not: a Scaleway server created *with* its flexible IP keeps that address + // on a routed eth0 beside a filtered eth1, and on 2026-08-28 a port its + // group never opened answered from the station in both driver modes. The + // same family as #481, where the missing word was enforced.balancing. + // + // What it claims, exactly, and the bound is as load-bearing as the claim. + // When a pack routes a public address *through one of its emulated + // networks* — Plan.RouteVia — the machine ends up with that address on + // that network's interface, wearing the machine's rule sets, even though + // the address first arrived on a routed NIC. It claims nothing about a + // machine whose only interface is routed: that is FirewallPublicOnly, + // still false and still measured, and such a machine has nowhere to move + // the address to (the two refused remedies are in docs/limits.md). + // + // So it is a claim about the runtime, and reading it as a claim about + // every published address of every pack would be the over-reading #481 and + // this issue are both about. A pack that names no network for its public + // addresses keeps them on a routed NIC and is covered by the sentence + // above, not by this one — Exoscale is that pack today, deliberately: its + // `public-ip` is the primary interface's address, and its private + // networks are the one place its own upstream says security groups do not + // apply (#574), so moving the address there would put it on an interface + // that wears no rule set by design. + // + // True in both Incus modes, measured before and after in each: the bridge + // carries the address as ipv4.routes on the managed device, OVN as + // ipv4.routes.external with the /32 delegated on the uplink. It follows + // the firewall claim rather than the mode, and a firewall write the host + // refuses withdraws it with its parent below. + FirewallPublicWhenJoined bool `json:"firewall_public_when_joined"` // Isolation: two networks of two different VPCs cannot reach each other. // // This is the one that separates the modes. Managed bridges on one host are @@ -183,6 +221,11 @@ func (d *Incus) Capabilities() Capabilities { if d.firewallDenied.Load() { caps.Firewall = false caps.FirewallPublicOnly = false + // The public half goes with it: it is the same rule sets on the same + // NICs, one interface over. A process that publishes "your address is + // covered" after the host refused a rule set is the lying 200 this + // project exists to refuse. + caps.FirewallPublicWhenJoined = false } // And the same rule for the balancer (#457). A load balancer the daemon // rejected after this driver had accepted it is the host saying this process @@ -214,9 +257,14 @@ func (d *Incus) declaredCapabilities() Capabilities { // instead of pretending. The claim goes true the day a mechanism // exists and is measured, not before. FirewallPublicOnly: false, - Isolation: d.OVN, - Balancing: d.OVN, - OwnKernel: d.VM, - PrivateFromHost: !d.OVN, + // True in both modes since #548, and it is the mechanism the line + // above says does not exist for a machine with nowhere to move the + // address: RouteAddress releases the address from the routed NIC and + // hands it to the interface the pack named, which wears the rule sets. + FirewallPublicWhenJoined: true, + Isolation: d.OVN, + Balancing: d.OVN, + OwnKernel: d.VM, + PrivateFromHost: !d.OVN, } } diff --git a/internal/core/machine/incus.go b/internal/core/machine/incus.go index dc3913c6..2a00babc 100644 --- a/internal/core/machine/incus.go +++ b/internal/core/machine/incus.go @@ -97,7 +97,7 @@ type Incus struct { busyPoll time.Duration busyBudget time.Duration - // routePoll and routeBudget override how restoreGuestRoutes waits for a + // routePoll and routeBudget override how restoreGuestNetwork waits for a // restarted guest to have configured its interface again. Only a test sets // them, for the same reason busyPoll exists; zero means the defaults in // waitForGuestInterface. @@ -441,20 +441,22 @@ func (d *Incus) Start(ctx context.Context, spec Spec) (Machine, error) { } // A machine that comes back from a stop has a brand new guest network: // the kernel booted again, DHCP configured the interface again, and - // nothing inside it remembers the routes this driver laid by hand the - // first time. Restoring them belongs here rather than in any pack, - // because it is a property of what starting a machine does, and every - // pack's poweron arrives through this one door (#549). + // nothing inside it remembers what this driver configured by hand the + // first time — neither the address a hot-attached NIC reserves (#548) + // nor the routes towards the peered subnets (#549). Restoring them + // belongs here rather than in any pack, because it is a property of + // what starting a machine does, and every pack's poweron arrives + // through this one door. // // Not fatal: the machine is up, and reporting a failed start would // publish FailedState for a machine the runtime is running — the lie // in the other direction. The log names what the machine cannot do, // and tools/conformance/functional.sh asserts the reachability this // restores. - if err := d.restoreGuestRoutes(ctx, spec.Name); err != nil { - d.logger().Error("a restarted machine did not get back its routes to the peered subnets", + if err := d.restoreGuestNetwork(ctx, spec.Name); err != nil { + d.logger().Error("a restarted machine did not get its guest network back", "machine", spec.Name, "error", err, - "consequence", "the machine is running and reaches its own subnet, but not the subnets its network is peered with (#549)") + "consequence", "the machine is running, and reaches neither the subnets its network is peered with (#549) nor — if the address it was given was reserved on a NIC attached while it ran — anything at all on its own interface (#548)") } return d.inspectOrFail(ctx, spec.Name) } @@ -1747,7 +1749,7 @@ func (d *Incus) WaitRunning(ctx context.Context, name string) (Machine, error) { switch { case err != nil: return Machine{}, err - case ok && machine.Running && machine.IP != "": + case ok && machine.Running && len(machine.Addresses) > 0: return machine, nil } @@ -1798,11 +1800,10 @@ func (d *Incus) Inspect(ctx context.Context, name string) (Machine, bool, error) if in.Name != name { continue } - // Interfaces come back as a map, and a map has no order: publishing the - // first one Go happens to visit means publishing a different address - // between two reads of the same machine. Sorted, eth1 (the first private - // NIC) wins over eth0 only if named so; what matters is that the answer - // never changes on its own. + // Interfaces come back as a map, and a map has no order: reporting them + // in the order Go happens to visit means answering differently between + // two reads of the same machine. Sorted, so the list below never + // changes on its own. names := make([]string, 0, len(in.State.Network)) for iface := range in.State.Network { if iface != "lo" { @@ -1811,23 +1812,37 @@ func (d *Incus) Inspect(ctx context.Context, name string) (Machine, bool, error) } slices.Sort(names) - ip := "" + // Every global IPv4 the machine carries, and no choice between them + // (#548). This used to answer one address — the first of the + // lowest-named interface — and that is a guess about *kind* the layer + // above had already settled: which address is public is the pack's + // declaration, held by Reconciler.PublicBlock since #541. The guess + // agreed with it only because a routed NIC sorts before a managed one, + // and it stops agreeing the moment an address moves: measured on + // 2026-08-28 under `--vm incus-ovn`, a server whose public address had + // been migrated onto its filtered NIC answered + // {"eth0":[],"eth1":["10.199.0.2","203.0.113.2"]}, so the old reading + // would have published the private address where it published the + // public one. + // + // So the driver reports what it saw and settles nothing. The one + // caller that never needed a kind — WaitRunning's readiness — asks + // whether the list is empty. + // TestInspectReportsEveryAddressTheMachineCarries fails without this. + addresses := make([]string, 0, 2) for _, iface := range names { for _, addr := range in.State.Network[iface].Addresses { - if addr.Family == "inet" && addr.Scope == "global" { - ip = addr.Address - break + if addr.Family == "inet" && addr.Scope == "global" && + !slices.Contains(addresses, addr.Address) { + addresses = append(addresses, addr.Address) } } - if ip != "" { - break - } } return Machine{ - Name: in.Name, - ID: in.Name, - IP: ip, - Running: strings.EqualFold(in.Status, "Running"), + Name: in.Name, + ID: in.Name, + Addresses: addresses, + Running: strings.EqualFold(in.Status, "Running"), }, true, nil } return Machine{}, false, nil diff --git a/internal/core/machine/incus_address.go b/internal/core/machine/incus_address.go index b1dfc552..9732a0bc 100644 --- a/internal/core/machine/incus_address.go +++ b/internal/core/machine/incus_address.go @@ -46,39 +46,44 @@ func (d *Incus) RouteAddress(ctx context.Context, spec AddressSpec) error { } return d.routeOntoRoutedNIC(ctx, spec.Machine, device, spec.Address) } - // The machine already answers on this address through a routed NIC, and - // there is nothing left to install: the launch put the address in the - // device's ipv4.address list and the runtime created the host route with - // the device. + // The machine answers on this address through a routed NIC, and the pack + // names a managed network for it: the address moves onto that interface + // (#548). // - // Routing it a second time through the network the pack names is not a - // no-op, it is a collision. A Scaleway server is created before its private - // NIC exists, so its public address rides a routed NIC (#202); by the time - // the NIC is there, Plan.RouteVia names the private network, and the replay - // a poweron or a reboot runs sends the very same /32 at the OVN uplink — - // where the delegation meets the host route the routed NIC already owns. - // Measured on 2026-08-27, twice in one run, on a host that ended up - // perfectly correct: + // Why it must move. A routed NIC accepts no security option at all (#337), + // so a Scaleway server created *with* its flexible IP — created before its + // private NIC exists, hence a routed NIC (#202) — kept the published + // address on an interface no rule set could ever cover, beside a private + // NIC wearing the group. Measured 2026-08-27 and again 2026-08-28 in both + // driver modes: eth0 routed and bare carrying 203.0.113.2, eth1 on a + // managed network with security.acls, and a port the group's drop default + // never opened answering from the station. // - // ERROR could not route the public address to the machine address=203.0.113.4 - // error="set routes of uplink feint-uplink: incus network: Error: Failed - // to add route {… Dst: 203.0.113.4/32 …}: file exists" + // Why this is where it happens. Until #548 this branch returned nil, and + // that was the right answer to a different question: routing the same /32 + // at the OVN uplink while the routed NIC still owned the host route died + // on `Failed to add route {… Dst: 203.0.113.4/32 …}: file exists` — an + // ERROR over a perfectly correct host, twice in one run (#498). Releasing + // the address from the device first is what unblocks that same call, so + // the collision is resolved rather than avoided, and the replay stays + // idempotent: once migrated, no routed NIC carries the address and this + // branch is not taken again. // - // An ERROR shouted over a correct host is exactly the noise that makes the - // next real one unreadable (#498). The replay is documented as idempotent; - // this is the door where it was not. The routed half already answered the - // same question for itself — routeOntoRoutedNIC returns nil for an address - // that rode the launch — and this is that answer, asked before the network - // the pack names decides which interface is edited. + // The device stays on the instance, and that is the whole trick: removing + // it unmasks the profile's own eth0 on incusbr0, the operator's default + // bridge, which this emulator must never put anything on (the second + // refused remedy in docs/limits.md). // - // TestReRoutingAnAddressARoutedNICAlreadyCarriesTouchesNothing fails - // without this. - carried, err := d.routedNICCarries(ctx, spec.Machine, spec.Address) + // TestAPublicAddressMovesOntoTheFilteredNIC and its bridge-mode twin fail + // without this, and TestMigratingIsIdempotent holds the second call. + routed, err := d.routedNICCarrying(ctx, spec.Machine, spec.Address) if err != nil { return err } - if carried { - return nil + if routed != "" { + if err := d.releaseFromRoutedNIC(ctx, spec.Machine, routed, spec.Address); err != nil { + return err + } } // OVN NICs take no live route edits, so the address travels as a network // forward instead; see routeAddressOVN for the measurements behind it. @@ -126,8 +131,8 @@ func addressAlreadyThere(err error) bool { strings.Contains(said, "address already assigned") } -// routedNICCarries reports whether one of the machine's own routed NICs already -// delivers this address. +// routedNICCarrying names the machine's own routed NIC that delivers this +// address, empty when none does. // // ipv4.address and nothing else, because that is the key the launch writes and // the key the host route follows: routedDevice builds the device from the @@ -139,20 +144,78 @@ func addressAlreadyThere(err error) bool { // Devices the instance owns, never the expanded set: a NIC inherited from a // profile belongs to the profile, and an address on it is not one this emulator // promised. -func (d *Incus) routedNICCarries(ctx context.Context, machine, address string) (bool, error) { +// +// Sorted, so a machine with two routed NICs carrying one address — which +// nothing produces today — names the same one on every read rather than +// whichever the map handed over. +func (d *Incus) routedNICCarrying(ctx context.Context, machine, address string) (string, error) { devices, err := d.instanceDevices(ctx, machine) if err != nil { - return false, fmt.Errorf("inspect %s: %w", machine, err) + return "", fmt.Errorf("inspect %s: %w", machine, err) + } + names := make([]string, 0, len(devices.own)) + for name := range devices.own { + names = append(names, name) } - for _, cfg := range devices.own { + slices.Sort(names) + for _, name := range names { + cfg := devices.own[name] if cfg["type"] != "nic" || cfg["nictype"] != "routed" { continue } if routeListContains(cfg["ipv4.address"], address) { - return true, nil + return name, nil + } + } + return "", nil +} + +// releaseFromRoutedNIC takes one address off a routed NIC without taking the +// NIC off the instance — step one of the migration RouteAddress performs, and +// the only one of three attempted remedies that Incus 7.2 accepts on a running +// instance (docs/limits.md carries the two refusals). +// +// What it costs and what it restores. Setting ipv4.address on a live routed +// NIC re-plugs the device, exactly as an ipv4.routes edit does, so the guest +// interface comes back down and bare; repairRoutedInterface puts back what the +// device still declares, which is every address but the one being moved. The +// explicit delete afterwards covers the case where the edit did *not* re-plug +// — a stopped machine, a runtime that updates in place — because an address +// left inside the guest on an interface the host no longer routes is a machine +// answering ARP for something nothing delivers. +// +// Ownership before shape, and this call is why: safeName has said the name +// could be a command argument, never that the instance is ours, and this +// reconfigures an instance's devices from a name a restored snapshot controls. +// RouteAddress's routed branch asks the same question for the same reason; the +// managed branch below asks it of the network instead, which says nothing +// about the instance whose device this edits. +// TestMigrationRefusesAnInstanceTheEmulatorDidNotCreate fails without it. +func (d *Incus) releaseFromRoutedNIC(ctx context.Context, machine, device, address string) error { + if err := d.mustOwnInstance(ctx, machine); err != nil { + return err + } + devices, err := d.instanceDevices(ctx, machine) + if err != nil { + return fmt.Errorf("inspect %s: %w", machine, err) + } + kept := make([]string, 0, 2) + for _, entry := range splitList(devices.own[device]["ipv4.address"]) { + if entry != address { + kept = append(kept, entry) } } - return false, nil + if _, err := d.run(ctx, "config", "device", "set", machine, device, + "ipv4.address="+strings.Join(kept, ",")); err != nil { + return fmt.Errorf("release %s from %s/%s: %w", address, machine, device, err) + } + if err := d.repairRoutedInterface(ctx, machine, device); err != nil { + return err + } + // Tolerant on purpose: the re-plug usually took it, a stopped machine + // holds no live address, and "cannot find" is the outcome asked for. + _, _ = d.run(ctx, "exec", machine, "--", "ip", "address", "del", address+"/32", "dev", device) + return nil } // mustOwn refuses to touch a network the emulator did not create. The label is diff --git a/internal/core/machine/incus_address_replay_test.go b/internal/core/machine/incus_address_replay_test.go index 40b69b36..fb4940b3 100644 --- a/internal/core/machine/incus_address_replay_test.go +++ b/internal/core/machine/incus_address_replay_test.go @@ -20,6 +20,15 @@ import ( // route {… Dst: 203.0.113.4/32 …}: file exists // // The host route was the routed NIC's own, installed with the device. +// +// #498 answered that by leaving the address where it was, and #548 measured +// what that cost: the routed NIC takes no security option, so the published +// address answered on an interface no group covered. The address moves now, +// and the property this file holds moved with it — from "a delivered address +// is left alone" to "a delivered address is moved once, and every replay after +// that writes nothing". The collision is what the move resolves: releasing the +// address from the device withdraws the host route, which is what the uplink +// write needed. // routedAndPrivate is that machine: a routed NIC carrying the public address, // and one OVN NIC on a network the emulator owns. @@ -36,38 +45,177 @@ const routedAndPrivate = `{ } }` -// TestReRoutingAnAddressARoutedNICAlreadyCarriesTouchesNothing: the address is -// delivered, so the replay emits nothing at all — and in particular never asks -// the uplink to carry a /32 the host already routes. -func TestReRoutingAnAddressARoutedNICAlreadyCarriesTouchesNothing(t *testing.T) { +// TestAPublicAddressMovesOntoTheFilteredNIC: the migration, in the order the +// runtime requires — release first, so the host route the routed device owns +// is gone before the uplink is asked to carry the same /32. +func TestAPublicAddressMovesOntoTheFilteredNIC(t *testing.T) { + f := &fakeRuntime{answers: map[string]string{ + "/1.0/instances/srv": routedAndPrivate, + "network get fnt-368798629f8 user." + LabelKey: "feint\n", + "network get fnt-368798629f8 ipv4.address": "10.30.1.1/24\n", + "network get " + DefaultUplinkName + " ipv4.routes": "", + }} + d := newFakeDriver(f) + d.OVN = true + + if err := d.RouteAddress(context.Background(), AddressSpec{ + Machine: "srv", Address: "203.0.113.4", Network: "fnt-368798629f8", + }); err != nil { + t.Fatalf("migrate: %v", err) + } + + release := step(f, "config device set srv eth0 ipv4.address=") + uplink := step(f, "network set "+DefaultUplinkName+" ipv4.routes=203.0.113.4/32") + external := step(f, "config device set srv eth1 ipv4.routes.external=203.0.113.4/32") + switch { + case release < 0: + t.Fatalf("the address was never released from the routed NIC:\n%s", strings.Join(f.commands(), "\n")) + case uplink < 0: + t.Fatalf("the uplink was never given the route:\n%s", strings.Join(f.commands(), "\n")) + case external < 0: + t.Fatalf("the filtered NIC never got the address:\n%s", strings.Join(f.commands(), "\n")) + case release > uplink: + t.Errorf("the uplink was asked for the /32 while the routed NIC still owned the host route, "+ + "which is the `file exists` of #498:\n%s", strings.Join(f.commands(), "\n")) + case uplink > external: + t.Errorf("the NIC's external route was set before the uplink carried it, and the runtime "+ + "validates the one against the other:\n%s", strings.Join(f.commands(), "\n")) + } + + // The device stays. Removing it is the second refused remedy: it unmasks + // the profile's own eth0 on incusbr0, the operator's default bridge. + if got := f.matching("config device remove"); len(got) != 0 { + t.Errorf("the routed device was removed, which unmasks the profile NIC on the host bridge:\n%s", + strings.Join(got, "\n")) + } +} + +// The same migration in bridge mode, where the address travels as ipv4.routes +// on the managed device instead. The mode was never measured on this shape +// until #548, and it is the poorer population of the two. +func TestAPublicAddressMovesOntoTheFilteredNICInBridgeMode(t *testing.T) { + f := &fakeRuntime{answers: map[string]string{ + "/1.0/instances/srv": routedAndPrivate, + "network get fnt-368798629f8 user." + LabelKey: "feint\n", + "config device get srv eth1 ipv4.routes": "", + }} + d := newFakeDriver(f) + + if err := d.RouteAddress(context.Background(), AddressSpec{ + Machine: "srv", Address: "203.0.113.4", Network: "fnt-368798629f8", + }); err != nil { + t.Fatalf("migrate: %v", err) + } + + release := step(f, "config device set srv eth0 ipv4.address=") + routes := step(f, "config device set srv eth1 ipv4.routes=203.0.113.4/32") + switch { + case release < 0: + t.Fatalf("the address was never released from the routed NIC:\n%s", strings.Join(f.commands(), "\n")) + case routes < 0: + t.Fatalf("the filtered NIC never got the route:\n%s", strings.Join(f.commands(), "\n")) + case release > routes: + t.Errorf("the bridged NIC was given the /32 while the routed NIC still owned the host route:\n%s", + strings.Join(f.commands(), "\n")) + } + if len(f.matching("ip address add 203.0.113.4/32 dev eth1")) == 0 { + t.Errorf("the guest was never given the address on its filtered interface:\n%s", + strings.Join(f.commands(), "\n")) + } +} + +// TestMigratingIsIdempotent is #498's property, restated for a machine that has +// already moved: the routed NIC carries nothing, the filtered one carries +// everything, and a replay writes not one key. Without it the poweron and the +// reboot would each re-plug a live NIC for nothing. +func TestMigratingIsIdempotent(t *testing.T) { + // The same machine after the migration: eth0 routed and addressless, eth1 + // carrying the external route. + const migrated = `{ + "devices": { + "eth0": {"type": "nic", "nictype": "routed", "ipv4.host_address": "169.254.0.1"}, + "eth1": {"type": "nic", "network": "fnt-368798629f8", "ipv4.address": "10.30.1.10", + "ipv4.routes.external": "203.0.113.4/32"} + }, + "expanded_devices": { + "eth0": {"type": "nic", "nictype": "routed", "ipv4.host_address": "169.254.0.1"}, + "eth1": {"type": "nic", "network": "fnt-368798629f8", "ipv4.address": "10.30.1.10", + "ipv4.routes.external": "203.0.113.4/32"} + } + }` + f := &fakeRuntime{answers: map[string]string{ + "/1.0/instances/srv": migrated, + "network get fnt-368798629f8 user." + LabelKey: "feint\n", + "network get fnt-368798629f8 ipv4.address": "10.30.1.1/24\n", + "network get " + DefaultUplinkName + " ipv4.routes": "203.0.113.4/32\n", + }} + d := newFakeDriver(f) + d.OVN = true + + if err := d.RouteAddress(context.Background(), AddressSpec{ + Machine: "srv", Address: "203.0.113.4", Network: "fnt-368798629f8", + }); err != nil { + t.Fatalf("replay: %v", err) + } + for _, forbidden := range []string{ + "network set " + DefaultUplinkName, + "config device set srv eth0", + "config device set srv eth1", + } { + if got := f.matching(forbidden); len(got) != 0 { + t.Errorf("the replay reconfigured a machine that already answers on the address:\n%s", + strings.Join(got, "\n")) + } + } +} + +// TestMigrationRefusesAnInstanceTheEmulatorDidNotCreate: the name comes from +// Resource.Runtime, which a restored snapshot controls verbatim, and this path +// reconfigures the instance's devices. safeName says the name is well formed, +// never that the instance is ours; the label the emulator itself wrote says +// that, and nothing is issued against an instance that does not carry it. +func TestMigrationRefusesAnInstanceTheEmulatorDidNotCreate(t *testing.T) { for name, ovn := range map[string]bool{"bridge mode": false, "ovn mode": true} { t.Run(name, func(t *testing.T) { f := &fakeRuntime{answers: map[string]string{ - "/1.0/instances/srv": routedAndPrivate, + "/1.0/instances/production-database": routedAndPrivate, + "network get fnt-368798629f8 user." + LabelKey: "feint\n", + // The instance carries no label of ours, which is what the + // fake's default answer would otherwise supply. + "config get production-database user." + LabelKey: "\n", }} d := newFakeDriver(f) d.OVN = ovn - if err := d.RouteAddress(context.Background(), AddressSpec{ - Machine: "srv", Address: "203.0.113.4", Network: "fnt-368798629f8", - }); err != nil { - t.Fatalf("replaying a delivered address must be a no-op, got: %v", err) + err := d.RouteAddress(context.Background(), AddressSpec{ + Machine: "production-database", Address: "203.0.113.4", Network: "fnt-368798629f8", + }) + if err == nil { + t.Fatalf("the migration was accepted on an instance the emulator did not create") } - - for _, forbidden := range []string{ - "network set " + DefaultUplinkName, - "ipv4.routes.external", - "config device set srv eth1", - } { - if got := f.matching(forbidden); len(got) != 0 { - t.Errorf("the replay reconfigured a machine that already answers on the address:\n%s", - strings.Join(got, "\n")) + for _, cmd := range f.commands() { + if strings.HasPrefix(cmd, "config device set production-database") || + strings.HasPrefix(cmd, "exec production-database") { + t.Errorf("the driver reconfigured the operator's own instance: %q", cmd) } } }) } } +// step is the position of the first command containing substr, -1 when none +// does: the migration's verdict is an order, not a set. Its neighbour indexOf +// compares whole commands, which cannot express "the device set, whatever it +// set the key to". +func step(f *fakeRuntime, substr string) int { + for i, cmd := range f.commands() { + if strings.Contains(cmd, substr) { + return i + } + } + return -1 +} + // The accepting half, without which the guard above would be a refusal rather // than an idempotence: an address no routed NIC carries still travels the OVN // path in full — the uplink first, because Incus validates ipv4.routes.external diff --git a/internal/core/machine/incus_firewall.go b/internal/core/machine/incus_firewall.go index 6db3b839..3891261d 100644 --- a/internal/core/machine/incus_firewall.go +++ b/internal/core/machine/incus_firewall.go @@ -330,8 +330,17 @@ func (d *Incus) ApplyFirewall(ctx context.Context, machine string, binding Firew // TestApplyFirewallDetachIgnoresARoutedNIC fail without this, and // TestTheUnenforceableRefusalNamesTheAddressThatEscapes fails without // the addresses. + // + // A routed NIC that carries nothing escapes nothing, and that half is + // #548's remedy landing here: once the public address has moved onto + // the filtered NIC, the device stays on the instance — removing it + // would unmask the profile's eth0 on the operator's own bridge — with + // no address and no host route. Refusing the whole binding for it + // would report an escape that no longer exists, and would keep the + // machine's other interfaces from ever being covered. + // TestARoutedNICThatCarriesNothingIsNotAnEscape fails without it. if device.routed { - if attached != "" { + if attached != "" && len(device.addresses) > 0 { escaped = append(escaped, device.describe()) } continue diff --git a/internal/core/machine/incus_firewall_test.go b/internal/core/machine/incus_firewall_test.go index f6ab4607..bf778ccd 100644 --- a/internal/core/machine/incus_firewall_test.go +++ b/internal/core/machine/incus_firewall_test.go @@ -530,6 +530,56 @@ func TestApplyFirewallRefusesAGroupOnARoutedNIC(t *testing.T) { } } +// TestARoutedNICThatCarriesNothingIsNotAnEscape: the machine of #548 once its +// public address has moved onto the filtered NIC. The routed device stays — +// removing it unmasks the profile's eth0 on the operator's own bridge — with +// no address and no host route, so it delivers nothing and there is nothing +// for a rule set to fail to cover. +// +// Without this the refusal would outlive the escape it names: every apply on +// every migrated machine would come back ErrFirewallUnenforceable, the pack +// would log a warning naming an interface that carries nothing, and the +// declared capability would go on describing a gap that had been closed. +func TestARoutedNICThatCarriesNothingIsNotAnEscape(t *testing.T) { + // eth0 routed and addressless, eth1 carrying both the private address and + // the migrated public one. + const migrated = `{ + "expanded_devices": { + "eth0": {"type": "nic", "nictype": "routed", "ipv4.host_address": "169.254.0.1"}, + "eth1": {"type": "nic", "network": "scw-abc", "ipv4.routes.external": "203.0.113.7/32"} + }, + "devices": { + "eth0": {"type": "nic", "nictype": "routed", "ipv4.host_address": "169.254.0.1"}, + "eth1": {"type": "nic", "network": "scw-abc", "ipv4.routes.external": "203.0.113.7/32"} + } + }` + f := &fakeRuntime{answers: map[string]string{ + "/1.0/instances/srv": migrated, + "/1.0/networks/": `{"type": "bridge"}`, + }} + d := newFakeDriver(f) + + if err := d.ApplyFirewall(context.Background(), "srv", FirewallBinding{ + Names: []string{"sg-one"}, + DefaultIngress: "drop", + DefaultEgress: "allow", + }); err != nil { + t.Fatalf("a routed NIC carrying no address escapes nothing, got %v", err) + } + if got := f.matching("config device set srv eth1 security.acls=sg-one"); len(got) != 1 { + t.Errorf("the interface that now carries the public address must wear the rule set:\n%s", + strings.Join(f.commands(), "\n")) + } + // And the refusal is still there for a routed NIC that does carry one, + // which TestApplyFirewallRefusesAGroupOnARoutedNIC holds: this test is the + // accepting half, not a licence. + for _, cmd := range f.matching("security.acls") { + if strings.Contains(cmd, " eth0 ") { + t.Errorf("a security option was sent to the routed NIC anyway: %q", cmd) + } + } +} + // TestApplyFirewallDetachIgnoresARoutedNIC holds the other direction: no rule // set has ever been attached to a routed NIC, so an empty binding — the // detach-all every permissive group now becomes — has nothing to take back diff --git a/internal/core/machine/incus_ovn.go b/internal/core/machine/incus_ovn.go index 377c5af8..34f225db 100644 --- a/internal/core/machine/incus_ovn.go +++ b/internal/core/machine/incus_ovn.go @@ -1006,8 +1006,9 @@ const ( guestRouteWait = 90 * time.Second ) -// restoreGuestRoutes puts back, inside a machine that has just been started -// again, the routes towards the peered subnets that a boot does not restore. +// restoreGuestNetwork puts back, inside a machine that has just been started +// again, what a boot does not restore: the address its NIC device reserves, +// and the routes towards the peered subnets. // // The measurement is #549, and it is the exact shape of "a machine the control // plane describes correctly and that does not work". A Scaleway server was @@ -1027,14 +1028,19 @@ const ( // would restore the wrong thing — or nothing, which is what a Scaleway server // whose NICs ride the launch would have got. // -// TestARestartedMachineGetsItsPeeredRoutesBack fails without this. -func (d *Incus) restoreGuestRoutes(ctx context.Context, machine string) error { - if !d.OVN { - // The aggregates exist because an OVN network's peers are only - // reachable through its router. A managed bridge has no router of its - // own and no peerings, so there is nothing here to put back. - return nil - } +// The two halves differ by mode and the difference is the modes' own: the +// address is owed back under a bridge exactly as under OVN — measured +// 2026-08-28, a bridge-mode machine came back with 203.0.113.2 on two +// interfaces and no address on its own subnet, and its published address +// stopped answering — while the aggregates exist only because an OVN +// network's peers are reachable through its router alone. A managed bridge +// has no router of its own and no peerings, so there is nothing of that kind +// to put back there. +// +// TestARestartedMachineGetsItsPeeredRoutesBack and +// TestARestartedMachineGetsItsPinnedAddressBack fail without this, the second +// in both modes. +func (d *Incus) restoreGuestNetwork(ctx context.Context, machine string) error { devices, err := d.instanceDevices(ctx, machine) if err != nil { return fmt.Errorf("inspect %s to restore its routes: %w", machine, err) @@ -1059,10 +1065,34 @@ func (d *Incus) restoreGuestRoutes(ctx context.Context, machine string) error { } slices.Sort(names) for _, device := range names { + // The address first, when the device pins one (#548). A NIC attached + // to a running machine is configured inside the guest by this driver, + // never by DHCP — the address is reserved on the device — and nothing + // inside the guest remembers it across a boot. Measured 2026-08-28 + // under `--vm incus-ovn` on a Scaleway server whose private NIC + // arrived hot: after a reboot the guest carried nothing on eth1, this + // function's wait ran its full ninety seconds and gave up, and the + // machine came back with neither its private address nor the routes + // below. + // + // That was survivable while the public address rode a routed NIC of + // its own; it stops being survivable the moment the address lives on + // this interface, because the reply to a station that dialled it is + // routed by the aggregates the failed wait never laid. So the restart + // path restores what it reserved instead of waiting for a lease + // nobody offers. + // TestARestartedMachineGetsItsPinnedAddressBack fails without this. + if err := d.restorePinnedAddress(ctx, machine, device, devices.own[device]); err != nil { + return err + } + if !d.OVN { + continue + } // The wait is the ordering, not politeness: `ip route add … via // dev ethN` is refused while the interface carries no address of that // subnet, and a route laid before DHCP finished is a route DHCP - // replaces. + // replaces. It still runs for a device that pins nothing, which is the + // DHCP case it was written for. if err := d.waitForGuestInterface(ctx, machine, device); err != nil { return err } @@ -1073,6 +1103,32 @@ func (d *Incus) restoreGuestRoutes(ctx context.Context, machine string) error { return nil } +// restorePinnedAddress gives the guest back the address its NIC device +// reserves, when the device reserves one. A device that pins nothing is left +// to DHCP, which is what the wait beside this call exists for. +// +// The mask comes from the network rather than from the caller: the reservation +// on the device carries no prefix length, and an address configured as a /32 +// inside the guest has no connected route to its own subnet. +func (d *Incus) restorePinnedAddress(ctx context.Context, machine, device string, cfg map[string]string) error { + address := cfg["ipv4.address"] + if address == "" { + return nil + } + gateway, err := d.networkGateway(ctx, cfg["network"]) + if err != nil { + return err + } + // Wrapped so the failure names the interface: the caller's own report is + // what an operator reads when a machine comes back unreachable, and + // "which one" is the first thing they need. + if err := d.configureGuestAddress(ctx, machine, device, + fmt.Sprintf("%s/%d", address, gateway.Bits())); err != nil { + return fmt.Errorf("restore the address of %s/%s: %w", machine, device, err) + } + return nil +} + // waitForGuestInterface blocks until the guest carries an IPv4 address on the // interface behind a device, which is what tells a boot that has finished // configuring the interface from one still doing it. diff --git a/internal/core/machine/incus_restart_test.go b/internal/core/machine/incus_restart_test.go index 824e55b6..9867e3bc 100644 --- a/internal/core/machine/incus_restart_test.go +++ b/internal/core/machine/incus_restart_test.go @@ -62,7 +62,7 @@ const routedPlusPrivate = `{ }` // TestARestartedMachineGetsItsPeeredRoutesBack is the test #549 names: without -// restoreGuestRoutes on the already-exists branch of Start, not one of these +// restoreGuestNetwork on the already-exists branch of Start, not one of these // commands is emitted and the guest comes back reaching its own subnet alone. func TestARestartedMachineGetsItsPeeredRoutesBack(t *testing.T) { f := &fakeRuntime{} @@ -82,6 +82,72 @@ func TestARestartedMachineGetsItsPeeredRoutesBack(t *testing.T) { } } +// TestARestartedMachineGetsItsPinnedAddressBack is the other half of the same +// boot (#548). The address of a NIC attached to a running machine is configured +// inside the guest by this driver — the device reserves it and no DHCP client +// watches that interface — and nothing inside the guest remembers it across a +// reboot. +// +// Measured 2026-08-28 under `--vm incus-ovn`, on a Scaleway server whose +// private NIC arrived hot: after a reboot the guest carried nothing on eth1, +// the wait beside this call ran its full ninety seconds and gave up with +// `it carries no IPv4 address`, and the routes above were never laid. That was +// survivable while the public address rode a routed NIC of its own; once the +// address lives on this interface, the reply to a station that dialled it has +// no route at all. +func TestARestartedMachineGetsItsPinnedAddressBack(t *testing.T) { + f := &fakeRuntime{} + restartedInstance(f, routedPlusPrivate) + d := ovnDriver(f) + + if _, err := d.Start(context.Background(), Spec{Name: "srv", Image: "ubuntu:22.04"}); err != nil { + t.Fatalf("start an existing machine: %v", err) + } + + // The mask is the network's, read off the runtime: an address configured + // as a /32 inside the guest has no connected route to its own subnet. + if len(f.matching("exec srv -- ip address add 10.30.1.10/24 dev eth1")) == 0 { + t.Errorf("a restarted machine was not given back the address its device reserves:\n%s", + strings.Join(f.commands(), "\n")) + } + // And the routed NIC is not one of these: it has no network to read a mask + // from, its address is the launch's own, and the guest's boot-time config + // declares it. + if got := f.matching("ip address add 203.0.113.4"); len(got) != 0 { + t.Errorf("the restart path configured the routed NIC's address inside the guest:\n%s", + strings.Join(got, "\n")) + } +} + +// TestARestartedMachineWaitsForAnAddressNobodyPinned is the accepting half of +// the branch above: a device that reserves nothing is a device DHCP owns, and +// the wait — not a made-up address — is what that case gets. +func TestARestartedMachineWaitsForAnAddressNobodyPinned(t *testing.T) { + const leased = `{ + "devices": { + "eth1": {"type": "nic", "network": "fnt-368798629f8"} + }, + "expanded_devices": { + "eth1": {"type": "nic", "network": "fnt-368798629f8"} + } + }` + f := &fakeRuntime{} + restartedInstance(f, leased) + d := ovnDriver(f) + + if _, err := d.Start(context.Background(), Spec{Name: "srv", Image: "ubuntu:22.04"}); err != nil { + t.Fatalf("start an existing machine: %v", err) + } + if got := f.matching("ip address add"); len(got) != 0 { + t.Errorf("the restart path invented an address for a NIC that pins none:\n%s", + strings.Join(got, "\n")) + } + // The routes still arrive: the wait saw the guest configured by its lease. + if len(f.matching("ip route add 10.0.0.0/8 via 10.30.1.1 dev eth1")) == 0 { + t.Errorf("a leased interface got no routes back:\n%s", strings.Join(f.commands(), "\n")) + } +} + // A machine that never went down must not pay for the repair either: the same // commands are what a poweron on an already-running machine emits, and they are // idempotent by construction ("file exists" is a previous call's work standing). @@ -159,6 +225,12 @@ func TestAGuestThatNeverConfiguresItsInterfaceIsReported(t *testing.T) { if strings.HasSuffix(args[len(args)-1], "/1.0/instances/srv") { return []byte(routedPlusPrivate), nil, true } + case "network": + // The block the pinned address is restored with, which this + // fixture needs since the restore runs before the wait (#548). + if len(args) > 3 && args[1] == "get" && args[3] == "ipv4.address" { + return []byte("10.30.1.1/24\n"), nil, true + } case "exec": return nil, errors.New("incus: Error: Command not found"), true } @@ -168,7 +240,7 @@ func TestAGuestThatNeverConfiguresItsInterfaceIsReported(t *testing.T) { d.routePoll = time.Millisecond d.routeBudget = 5 * time.Millisecond - err := d.restoreGuestRoutes(context.Background(), "srv") + err := d.restoreGuestNetwork(context.Background(), "srv") if err == nil { t.Fatal("a guest that never configured its interface was reported as repaired") } diff --git a/internal/core/machine/machine.go b/internal/core/machine/machine.go index 16374131..5ecee594 100644 --- a/internal/core/machine/machine.go +++ b/internal/core/machine/machine.go @@ -128,10 +128,24 @@ type Spec struct { // Machine is a running (or stopped) backing machine. type Machine struct { - ID string - Name string - IP string - Running bool + ID string + Name string + // Addresses is every global IPv4 the machine carries, in a deterministic + // order. + // + // The list, and no single address beside it, which is #548. This struct + // carried an IP field until then and a driver filling it had to decide + // *which* address that was; the only rules available to a driver are + // conventions — interface name order, the order the runtime happened to + // list. Which address is public is a fact the layer above already holds: + // the pack declares its emulated block, Reconciler.PublicBlock guards + // every address on its way to the driver since #541, and + // PublicAddressOf/PrivateAddressOf answer the kind from it. Two answers to + // one question agreed only while a routed NIC sorted before a managed one, + // and the field is gone rather than deprecated so no future reader can + // take the coincidence for an answer. + Addresses []string + Running bool } // driver runs machines and the networks they sit on. Implementations must be diff --git a/internal/core/machine/ownership_test.go b/internal/core/machine/ownership_test.go index fbc2f289..c26922e2 100644 --- a/internal/core/machine/ownership_test.go +++ b/internal/core/machine/ownership_test.go @@ -75,7 +75,7 @@ func TestAddressIgnoresAMachineNameTheEmulatorCouldNotHaveCreated(t *testing.T) // Read-only, but publishing another instance's address as the emulated // server's is still wrong. - b.Address(context.Background(), "srv-1", "production-database") + b.Addresses(context.Background(), "srv-1", "production-database") if mentions(f, "production-database") { t.Fatalf("the driver inspected the operator's instance: %v", f.calls) diff --git a/internal/core/machine/plan.go b/internal/core/machine/plan.go index 3c161bc7..d09cbd1b 100644 --- a/internal/core/machine/plan.go +++ b/internal/core/machine/plan.go @@ -258,8 +258,7 @@ func (r Reconciler) Unroute(ctx context.Context, machine, address string) { // PublicAddressOf is what the machine answers on, when that address is one // this pack could have handed out as a public one — and nothing otherwise. // -// The binding records one address per machine and gives it no kind: it is -// whatever the runtime answered, read off the first interface in name order. +// The binding records what the machine answers on and gives it no kind. // Each pack then republishes it under a field whose *name asserts a kind* — // Exoscale as `public-ip`, Outscale as `PrivateIp` — and neither asked whether // the recorded address was of that kind. Measured on 2026-08-27 under @@ -275,9 +274,20 @@ func (r Reconciler) Unroute(ctx context.Context, machine, address string) { // address passes on its way to the driver. So the layer answers the question // too, once, instead of each pack writing half of it: a pack that publishes // an address as public asks here, and a pack that publishes one as private -// asks PrivateAddressOf. Binding.AddressOf is out of PackSurface for that +// asks PrivateAddressOf. Binding.AddressesOf is out of PackSurface for that // reason — the layer no longer hands a pack an address with no kind on it. // +// It reads *every* address the machine carries since #548, and that is what +// makes the answer a reading rather than a coincidence. The binding used to +// record one — the driver's first-interface-in-name-order pick — so this +// function was only ever asked about an address somebody else had already +// chosen. It agreed with the block while the public address rode a routed +// NIC, which sorts before a managed one, and a machine whose two addresses +// share an interface breaks that agreement: measured 2026-08-28, +// {"eth0":[],"eth1":["10.199.0.2","203.0.113.2"]}. Now the driver reports the +// set and the block picks out of it, which is the same answer on every shape +// measured before, and the right one on the shape that moved. +// // A pack that declares no PublicBlock gets nothing here, which is the safe // direction: no block means nothing can be shown to be public. // @@ -285,11 +295,12 @@ func (r Reconciler) Unroute(ctx context.Context, machine, address string) { // fails without this, and its TestAnInstanceIsGivenAPublicAddressAtCreation // holds the accepting half. func (r Reconciler) PublicAddressOf(res *resource.Resource) string { - address := r.binding().AddressOf(res) - if !r.emulated(address) { - return "" + for _, address := range r.binding().AddressesOf(res) { + if r.emulated(address) { + return address + } } - return address + return "" } // PrivateAddressOf is the mirror: what the machine answers on, when that @@ -299,20 +310,26 @@ func (r Reconciler) PublicAddressOf(res *resource.Resource) string { // other half was live too. An Outscale Vm publishes the recorded address as // PrivateIp, and its plan carries promised public addresses onto the launch: // the guest then holds two global addresses on one interface, and Inspect -// answers with whichever the runtime lists first. A restart that came back -// with the public one would have published 198.51.100.x as PrivateIp and as -// PublicIp at once. Nothing measured that happening — the pin in +// used to answer with whichever the runtime listed first. A boot that came +// back with the public one would have published 198.51.100.x as PrivateIp and +// as PublicIp at once. Nothing measured that happening — the pin in // rememberAddress hides it after the first boot — which is exactly why the // control belongs in the layer rather than in a pack's memory. // +// Since #548 that ordering cannot decide anything at all: the driver reports +// every address, and the two halves here read the same set from opposite +// ends. The Vm above publishes its subnet address as PrivateIp whichever +// order the runtime listed the two in. +// // internal/providers/outscale's TestAVmPublishesNoPublicAddressAsItsPrivateOne // fails without this. func (r Reconciler) PrivateAddressOf(res *resource.Resource) string { - address := r.binding().AddressOf(res) - if address == "" || r.emulated(address) { - return "" + for _, address := range r.binding().AddressesOf(res) { + if !r.emulated(address) { + return address + } } - return address + return "" } // emulated reports whether an address is one this pack can have handed out: @@ -338,8 +355,20 @@ func (r Reconciler) emulated(address string) bool { // // The attach error is returned for the pack that surfaces it on its resource; // degrading quietly stays the caller's choice, as everywhere. +// +// The addresses are replayed between the two, and that order is #548's other +// half. A machine that gains its first managed interface is a machine whose +// promised public addresses can stop living on an interface no rule set +// covers: the replay is what moves them, the driver decides whether anything +// has to move, and the firewall step then sees the interface that carries +// them. Running it before the attach would find no interface to move onto; +// running it after the firewall would leave the set written over the shape it +// no longer describes. Idempotent for every machine that needs no move, which +// is every machine of the two packs whose addresses never rode a routed NIC. +// TestJoinReplaysTheAddressesBeforeTheFirewall fails without it. func (r Reconciler) Join(ctx context.Context, res *resource.Resource, att Attachment) error { err := r.attach(ctx, res, att) + r.ReplayAddresses(ctx, res) r.Groups.AfterBoot(ctx, res) return err } diff --git a/internal/core/machine/plan_test.go b/internal/core/machine/plan_test.go index 7838f6c5..f23e99e0 100644 --- a/internal/core/machine/plan_test.go +++ b/internal/core/machine/plan_test.go @@ -60,6 +60,45 @@ func TestTheReplayRoutesThenJoinsThenAppliesTheFirewall(t *testing.T) { } } +// TestJoinReplaysTheAddressesBeforeTheFirewall is the hot half of the same +// order (#548). A machine that gains its first managed interface is a machine +// whose promised public addresses can stop living on an interface no rule set +// covers — the Scaleway shape exactly: the server is created with its flexible +// IP, so the address rides a routed NIC, and the private NIC arrives on a +// running server through this door and no other. +// +// Attach, then the addresses, then the firewall. Before the attach there is no +// interface to move onto; after the firewall the rule sets would have been +// written over the shape they no longer describe. +func TestJoinReplaysTheAddressesBeforeTheFirewall(t *testing.T) { + b := newGroupSyncBench() + b.group("g", "") + vm := b.machine("m", "", "g") + vm.Runtime["machine"] = "feint-bench-m" + + r := reconcilerBench(b, Plan{Publics: []string{"203.0.113.9"}}) + if err := r.Join(context.Background(), vm, Attachment{Network: "fnt-bench-net"}); err != nil { + t.Fatalf("join: %v", err) + } + + position := map[string]int{} + for i, kind := range b.rec.Sequence() { + if _, seen := position[kind]; !seen { + position[kind] = i + } + } + for _, kind := range []string{"Attach", "RouteAddress", "ApplyFirewall"} { + if _, seen := position[kind]; !seen { + t.Fatalf("the hot attach never emitted %s; sequence: %v", kind, b.rec.Sequence()) + } + } + ordered := position["Attach"] < position["RouteAddress"] && + position["RouteAddress"] < position["ApplyFirewall"] + if !ordered { + t.Fatalf("the order is not attach, then the addresses, then the firewall: %v", b.rec.Sequence()) + } +} + // TestAPoisonedStoredAddressIsNeverRoutedByTheLayer holds the authorisation // half once for the three packs: a stored address is untrusted input — a // restored snapshot carries it verbatim — and routing an arbitrary value would diff --git a/internal/core/machine/recorder.go b/internal/core/machine/recorder.go index 1a79a6b1..69a0b3b5 100644 --- a/internal/core/machine/recorder.go +++ b/internal/core/machine/recorder.go @@ -182,7 +182,7 @@ func (r *Recorder) Start(_ context.Context, spec Spec) (Machine, error) { r.machines[spec.Name] = recordedMachine{ip: ip, running: true} r.mu.Unlock() r.Record(Gesture{Kind: "Start", Resource: spec.Name, Args: spec}) - return Machine{Name: spec.Name, IP: ip, Running: true}, nil + return Machine{Name: spec.Name, Addresses: []string{ip}, Running: true}, nil } // Stop implements driver. @@ -215,7 +215,10 @@ func (r *Recorder) Inspect(_ context.Context, name string) (Machine, bool, error if !found { return Machine{}, false, nil } - return Machine{Name: name, IP: m.ip, Running: m.running}, true, nil + // One address, which is every address this double knows about: it records + // what a pack asked for and runs nothing, so it has no second address to + // report and must not invent one. + return Machine{Name: name, Addresses: []string{m.ip}, Running: m.running}, true, nil } // EnsureNetwork implements driver. diff --git a/internal/core/machine/recorder_test.go b/internal/core/machine/recorder_test.go index 4ce85e4a..4c482c63 100644 --- a/internal/core/machine/recorder_test.go +++ b/internal/core/machine/recorder_test.go @@ -85,8 +85,8 @@ func TestARecorderReplaysTheLifecycleInCallOrder(t *testing.T) { if err != nil { t.Fatalf("start: %v", err) } - if m.IP != "10.230.0.7" { - t.Fatalf("the machine does not carry its attachment's address: %q", m.IP) + if len(m.Addresses) != 1 || m.Addresses[0] != "10.230.0.7" { + t.Fatalf("the machine does not carry its attachment's address: %v", m.Addresses) } if got, found, _ := r.Inspect(ctx, "feint-x-1"); !found || !got.Running { t.Fatalf("a started machine inspects as %+v, found=%v", got, found) diff --git a/internal/core/machine/verify.go b/internal/core/machine/verify.go index f8e735e5..75217342 100644 --- a/internal/core/machine/verify.go +++ b/internal/core/machine/verify.go @@ -111,6 +111,12 @@ func (d *Incus) Verify(ctx context.Context) (Capabilities, []string) { if declared.Firewall { if ok, why := d.firewallAge(ctx); !ok { verified.Firewall = false + // And the public half with it (#548): moving an address onto the + // filtered NIC covers it only if that NIC can wear a rule set at + // all, so a runtime too old for NIC-level ACLs delivers neither. + // A capability that outlived the one it rests on would be the + // declaration saying more than the host does. + verified.FirewallPublicWhenJoined = false unmet = append(unmet, "firewall: "+why) } } diff --git a/internal/providers/exoscale/elasticip_routing_test.go b/internal/providers/exoscale/elasticip_routing_test.go index 96c686af..4a12ab62 100644 --- a/internal/providers/exoscale/elasticip_routing_test.go +++ b/internal/providers/exoscale/elasticip_routing_test.go @@ -27,7 +27,7 @@ type routedRuntime struct { func (r *routedRuntime) Inspect(_ context.Context, n string) (machine.Machine, bool, error) { // Running, with an address: the packs replay routes on machines that // exist, and the base recording runtime reports every machine absent. - return machine.Machine{Name: n, IP: "10.209.84.9", Running: true}, true, nil + return machine.Machine{Name: n, Addresses: []string{"10.209.84.9"}, Running: true}, true, nil } func (r *routedRuntime) RouteAddress(_ context.Context, spec machine.AddressSpec) error { diff --git a/internal/providers/exoscale/machines_internal_test.go b/internal/providers/exoscale/machines_internal_test.go index 620ff6ce..7704ca5a 100644 --- a/internal/providers/exoscale/machines_internal_test.go +++ b/internal/providers/exoscale/machines_internal_test.go @@ -26,7 +26,7 @@ func (d *recordingDriver) Name() string { return "recording" } func (d *recordingDriver) Available(context.Context) bool { return true } func (d *recordingDriver) Start(_ context.Context, spec machine.Spec) (machine.Machine, error) { d.specs = append(d.specs, spec) - return machine.Machine{Name: spec.Name, IP: "10.42.0.9", Running: true}, nil + return machine.Machine{Name: spec.Name, Addresses: []string{"10.42.0.9"}, Running: true}, nil } func (d *recordingDriver) Stop(context.Context, string) error { return nil } func (d *recordingDriver) Remove(context.Context, string) error { return nil } diff --git a/internal/providers/exoscale/pools_machines_internal_test.go b/internal/providers/exoscale/pools_machines_internal_test.go index 89d4bd4c..b5cd2302 100644 --- a/internal/providers/exoscale/pools_machines_internal_test.go +++ b/internal/providers/exoscale/pools_machines_internal_test.go @@ -43,7 +43,7 @@ func (d *gatedDriver) Start(_ context.Context, spec machine.Spec) (machine.Machi d.mu.Lock() d.specs = append(d.specs, spec) d.mu.Unlock() - return machine.Machine{Name: spec.Name, IP: "10.42.0.9", Running: true}, nil + return machine.Machine{Name: spec.Name, Addresses: []string{"10.42.0.9"}, Running: true}, nil } func (d *gatedDriver) Stop(context.Context, string) error { return nil } func (d *gatedDriver) Remove(context.Context, string) error { return nil } diff --git a/internal/providers/outscale/audit_test.go b/internal/providers/outscale/audit_test.go index 2598d5a1..1afd218c 100644 --- a/internal/providers/outscale/audit_test.go +++ b/internal/providers/outscale/audit_test.go @@ -718,7 +718,7 @@ func (f *countingRuntime) Inspect(_ context.Context, n string) (machine.Machine, if f.counts[n] > 0 { // With an address, the way a real runtime answers once the machine is // up. Without one, TestAStoppedVmKeepsItsPrivateAddress skipped itself. - return machine.Machine{Name: n, Running: true, IP: "10.99.0.7"}, true, nil + return machine.Machine{Name: n, Running: true, Addresses: []string{"10.99.0.7"}}, true, nil } return machine.Machine{}, false, nil } diff --git a/internal/providers/outscale/machines_internal_test.go b/internal/providers/outscale/machines_internal_test.go index dd01349f..b326259b 100644 --- a/internal/providers/outscale/machines_internal_test.go +++ b/internal/providers/outscale/machines_internal_test.go @@ -25,7 +25,7 @@ func (d *recordingDriver) Name() string { return "recording" } func (d *recordingDriver) Available(context.Context) bool { return true } func (d *recordingDriver) Start(_ context.Context, spec machine.Spec) (machine.Machine, error) { d.specs = append(d.specs, spec) - return machine.Machine{Name: spec.Name, IP: "10.42.0.9", Running: true}, nil + return machine.Machine{Name: spec.Name, Addresses: []string{"10.42.0.9"}, Running: true}, nil } func (d *recordingDriver) Stop(context.Context, string) error { return nil } func (d *recordingDriver) Remove(context.Context, string) error { return nil } diff --git a/internal/providers/outscale/publicip_routing_test.go b/internal/providers/outscale/publicip_routing_test.go index e6cf83fb..b0fbbf33 100644 --- a/internal/providers/outscale/publicip_routing_test.go +++ b/internal/providers/outscale/publicip_routing_test.go @@ -38,7 +38,7 @@ func (r *routedRuntime) Start(_ context.Context, spec machine.Spec) (machine.Mac defer r.mu.Unlock() r.specs = append(r.specs, spec) r.machines[spec.Name] = true - return machine.Machine{Name: spec.Name, IP: "10.209.84.9", Running: true}, nil + return machine.Machine{Name: spec.Name, Addresses: []string{"10.209.84.9"}, Running: true}, nil } func (r *routedRuntime) Stop(_ context.Context, n string) error { @@ -62,7 +62,7 @@ func (r *routedRuntime) Inspect(_ context.Context, n string) (machine.Machine, b if !found { return machine.Machine{}, false, nil } - return machine.Machine{Name: n, IP: "10.209.84.9", Running: running}, true, nil + return machine.Machine{Name: n, Addresses: []string{"10.209.84.9"}, Running: running}, true, nil } func (r *routedRuntime) EnsureNetwork(context.Context, machine.NetworkSpec) error { return nil } diff --git a/internal/providers/scaleway/barrage_test.go b/internal/providers/scaleway/barrage_test.go index afe8b9b2..0396d2c9 100644 --- a/internal/providers/scaleway/barrage_test.go +++ b/internal/providers/scaleway/barrage_test.go @@ -88,7 +88,7 @@ func (b *barrageRuntime) Start(_ context.Context, spec machine.Spec) (machine.Ma return machine.Machine{}, errors.New(`Failed creating instance record: Add instance info to the database: This "instances" entry already exists`) } b.machines[spec.Name] = true - return machine.Machine{Name: spec.Name, IP: "10.42.0.7", Running: true}, nil + return machine.Machine{Name: spec.Name, Addresses: []string{"10.42.0.7"}, Running: true}, nil } func (b *barrageRuntime) Stop(_ context.Context, n string) error { @@ -112,7 +112,7 @@ func (b *barrageRuntime) Inspect(_ context.Context, n string) (machine.Machine, if !known { return machine.Machine{}, false, nil } - return machine.Machine{Name: n, IP: "10.42.0.7", Running: running}, true, nil + return machine.Machine{Name: n, Addresses: []string{"10.42.0.7"}, Running: running}, true, nil } func (b *barrageRuntime) EnsureNetwork(context.Context, machine.NetworkSpec) error { return nil } diff --git a/internal/providers/scaleway/concurrency_test.go b/internal/providers/scaleway/concurrency_test.go index 1151fabc..67590596 100644 --- a/internal/providers/scaleway/concurrency_test.go +++ b/internal/providers/scaleway/concurrency_test.go @@ -81,7 +81,7 @@ func (f *fakeRuntime) Start(_ context.Context, spec machine.Spec) (machine.Machi return machine.Machine{}, errors.New(`Failed creating instance record: Add instance info to the database: This "instances" entry already exists`) } f.machines[spec.Name] = true - return machine.Machine{Name: spec.Name, IP: "10.42.0.7", Running: true}, nil + return machine.Machine{Name: spec.Name, Addresses: []string{"10.42.0.7"}, Running: true}, nil } func (f *fakeRuntime) Inspect(_ context.Context, n string) (machine.Machine, bool, error) { @@ -91,7 +91,7 @@ func (f *fakeRuntime) Inspect(_ context.Context, n string) (machine.Machine, boo if !ok { return machine.Machine{}, false, nil } - return machine.Machine{Name: n, IP: "10.42.0.7", Running: running}, true, nil + return machine.Machine{Name: n, Addresses: []string{"10.42.0.7"}, Running: running}, true, nil } func (f *fakeRuntime) EnsureNetwork(context.Context, machine.NetworkSpec) error { return nil } diff --git a/tools/conformance/functional.sh b/tools/conformance/functional.sh index 5d77bfd7..4fd6de99 100755 --- a/tools/conformance/functional.sh +++ b/tools/conformance/functional.sh @@ -253,6 +253,19 @@ live_probe() { # machine address port return 1 } +# station_probe is live_probe's shape from where the operator stands: same three +# outcomes, same rule that the verdict is the exit code and never the text — a +# connection the firewall drops is killed by `timeout` without a word, and its +# output is exactly a successful one's. +# +# The console is not used, and the argument stays so this and live_probe are +# interchangeable wherever a probe is passed by name (fnl_firewall_pair). +station_probe() { # console address port + command -v timeout >/dev/null 2>&1 || return 2 + timeout 6 bash -c "exec 3<>/dev/tcp/$2/$3" >/dev/null 2>&1 && return 0 + return 1 +} + # live_fetch prints the body one machine gets from another, or nothing. live_fetch() { # machine address port incus exec "$1" -- timeout 6 python3 -c \ @@ -655,9 +668,18 @@ run_stack() { # name unit="$(printf '%s' "$service" | jq -r '.unit')" restart="$(printf '%s' "$service" | jq -r '.restart // ""')" echo "- $name: the declared service listens inside, and answers over the published address" - if [ "$(printf '%s' "$health" | jq -r '.capabilities.firewall_public_only // false')" != "true" ]; then - skip "$name: this runtime declares capabilities.firewall_public_only false (#337), so an address published on a routed NIC carries no rule set (#548). The station leg below therefore proves the service is reachable and asserts nothing about the firewall; the firewall pair runs on the emulated network." - fi + # The public half of the firewall used to be skipped here, naming + # capabilities.firewall_public_only and #548 together. Both halves of + # that citation stopped being true at once: the driver moves a public + # address onto the filtered NIC when the machine joins an emulated + # network, so the group covers it, and the runtime declares that as + # capabilities.firewall_public_when_joined. What is asserted below is + # what the skip described as missing — the same pair as the firewall + # family, on the published address, from the station. + local closed + closed="$(printf '%s' "$service" | jq -c 'if has("closed_port") then .closed_port else "MISSING" end')" + [ "$closed" != '"MISSING"' ] \ + || fail "$name: the service family declares no closed_port; the public path needs a port the machines listen on and no rule opens, or the reason there is none" for target in $(printf '%s' "$service" | jq -r '.machines[]'); do need_machine "$target" smachine="$MACHINE" @@ -671,6 +693,25 @@ run_stack() { # name fi body="$(station_fetch "$address" "$port")" fnl_service_answers "$name" "$target" "$smachine" "$address" "$port" "$body" + + # The public pair, on the address a client dials (#548). The open + # half is the service port a rule opens, the closed half is a port + # the same machine is listening on that no rule names, and + # fnl_firewall_pair proves both listeners from inside before it + # reads either verdict — so a refusal cannot be a dead service. + reason="$(skip_reason "$closed")" + if [ -n "$reason" ]; then + skip "$name service.closed_port: $reason" + elif [ "$(printf '%s' "$health" | jq -r '.capabilities.firewall // false')" != "true" ]; then + skip "$name: this runtime does not declare capabilities.firewall; nothing was promised on the public path either" + elif [ "$(printf '%s' "$health" | jq -r '.capabilities.firewall_public_when_joined // false')" != "true" ]; then + skip "$name: this runtime does not declare capabilities.firewall_public_when_joined, so a published address may still live on a NIC no rule set can reach (#337, #548) — asserting a closure nobody promised is what this gate exists to avoid" + elif ! printf '%s' "$health" | witness_enforced "$provider" firewall; then + skip "$name: $provider does not declare enforced.firewall — a property it never promised is not demanded of it (#481)" + else + fnl_firewall_pair "$name (public path)" "the station" "" "$target" "$address" \ + "$port" "$(printf '%s' "$closed" | jq -r '.')" "$LISTEN" station_probe + fi done # Handed to the end of the run rather than asserted here: see # assert_restart above for why the order is a measurement (#549). diff --git a/tools/falsify/specs/address-kind.json b/tools/falsify/specs/address-kind.json index 2833acab..89811438 100644 --- a/tools/falsify/specs/address-kind.json +++ b/tools/falsify/specs/address-kind.json @@ -4,15 +4,15 @@ { "label": "an address the machine answers on is published as public without being one", "file": "internal/core/machine/plan.go", - "find": "\taddress := r.binding().AddressOf(res)\n\tif !r.emulated(address) {\n\t\treturn \"\"\n\t}\n\treturn address", - "replace": "\taddress := r.binding().AddressOf(res)\n\tif !r.emulated(address) && false {\n\t\treturn \"\"\n\t}\n\treturn address", + "find": "\t\tif r.emulated(address) {\n\t\t\treturn address\n\t\t}", + "replace": "\t\tif r.emulated(address) || true {\n\t\t\treturn address\n\t\t}", "test": "TestAnInstanceWithNoPublicIPPublishesNone" }, { "label": "an emulated public address is published as the machine's private one", "file": "internal/core/machine/plan.go", - "find": "\tif address == \"\" || r.emulated(address) {\n\t\treturn \"\"\n\t}\n\treturn address", - "replace": "\tif address == \"\" || (r.emulated(address) && false) {\n\t\treturn \"\"\n\t}\n\treturn address", + "find": "\t\tif !r.emulated(address) {\n\t\t\treturn address\n\t\t}", + "replace": "\t\tif !r.emulated(address) || true {\n\t\t\treturn address\n\t\t}", "test": "TestAVmPublishesNoPublicAddressAsItsPrivateOne" } ] diff --git a/tools/falsify/specs/balancer-dataplane.json b/tools/falsify/specs/balancer-dataplane.json index 3e4872f1..e1f9e42d 100644 --- a/tools/falsify/specs/balancer-dataplane.json +++ b/tools/falsify/specs/balancer-dataplane.json @@ -39,8 +39,8 @@ { "label": "every mode declares balancing, including the one with no load balancer primitive", "file": "internal/core/machine/capabilities.go", - "find": "\t\tBalancing: d.OVN,", - "replace": "\t\tBalancing: d.OVN || true,", + "find": "\t\tBalancing: d.OVN,", + "replace": "\t\tBalancing: d.OVN || true,", "test": "TestABridgeBackedRunHasNoBalancer" }, { diff --git a/tools/falsify/specs/interface-plan.json b/tools/falsify/specs/interface-plan.json index 678155be..a8a51ecb 100644 --- a/tools/falsify/specs/interface-plan.json +++ b/tools/falsify/specs/interface-plan.json @@ -11,8 +11,8 @@ { "label": "a hot join resyncs the firewall before the interface it must cover exists (#510)", "file": "internal/core/machine/plan.go", - "find": "\terr := r.attach(ctx, res, att)\n\tr.Groups.AfterBoot(ctx, res)\n\treturn err", - "replace": "\tr.Groups.AfterBoot(ctx, res)\n\terr := r.attach(ctx, res, att)\n\treturn err", + "find": "\terr := r.attach(ctx, res, att)\n\tr.ReplayAddresses(ctx, res)\n\tr.Groups.AfterBoot(ctx, res)\n\treturn err", + "replace": "\tr.Groups.AfterBoot(ctx, res)\n\terr := r.attach(ctx, res, att)\n\tr.ReplayAddresses(ctx, res)\n\treturn err", "test": "TestJoinRunsTheFirewallAfterTheAttach" }, { diff --git a/tools/falsify/specs/lifecycle-tells-the-truth.json b/tools/falsify/specs/lifecycle-tells-the-truth.json index 015e7c0c..192111e2 100644 --- a/tools/falsify/specs/lifecycle-tells-the-truth.json +++ b/tools/falsify/specs/lifecycle-tells-the-truth.json @@ -11,7 +11,7 @@ "package": "./internal/core/machine/", "mutations": [ { - "label": "#547: a reboot stops taking the machine down, so the action answers success and the runtime — which refuses to relaunch a name it has already served — does nothing at all", + "label": "#547: a reboot stops taking the machine down, so the action answers success and the runtime \u2014 which refuses to relaunch a name it has already served \u2014 does nothing at all", "file": "internal/core/machine/plan.go", "find": "\tif res.State == r.binding().RunningState {\n\t\tr.binding().PowerOff(ctx, res)\n\t}", "replace": "\tif res.State == r.binding().RunningState && false {\n\t\tr.binding().PowerOff(ctx, res)\n\t}", @@ -25,7 +25,7 @@ "test": "TestARebootOfAStoppedMachineDoesNotStopItAgain" }, { - "label": "#547 across the packs: Scaleway's reboot calls the start alone again, which is the defect verbatim — the three other packs go on asking the runtime for a stop and the sequences diverge", + "label": "#547 across the packs: Scaleway's reboot calls the start alone again, which is the defect verbatim \u2014 the three other packs go on asking the runtime for a stop and the sequences diverge", "file": "internal/providers/scaleway/servers.go", "package": "./internal/providers/", "find": "\t\tp.rebootMachine(r.Context(), res)", @@ -35,8 +35,8 @@ { "label": "#549: a restarted guest stops being given back the routes to its peered subnets, so it comes back running, on its address, reaching its own subnet and nothing one router away", "file": "internal/core/machine/incus_ovn.go", - "find": "\tif !d.OVN {\n\t\t// The aggregates exist because an OVN network's peers are only", - "replace": "\tif !d.OVN || true {\n\t\t// The aggregates exist because an OVN network's peers are only", + "find": "\t\tif !d.OVN {\n\t\t\tcontinue\n\t\t}", + "replace": "\t\tif !d.OVN || true {\n\t\t\tcontinue\n\t\t}", "test": "TestARestartedMachineGetsItsPeeredRoutesBack" }, { @@ -49,8 +49,8 @@ { "label": "#549, the mode half: the bridge mode starts laying OVN aggregates, pointing a guest's private traffic at an address no router answers on", "file": "internal/core/machine/incus_ovn.go", - "find": "\tif !d.OVN {\n\t\t// The aggregates exist because an OVN network's peers are only", - "replace": "\tif !d.OVN && false {\n\t\t// The aggregates exist because an OVN network's peers are only", + "find": "\t\tif !d.OVN {\n\t\t\tcontinue\n\t\t}", + "replace": "\t\tif !d.OVN && false {\n\t\t\tcontinue\n\t\t}", "test": "TestABridgeModeRestartLaysNoAggregates" }, { @@ -61,18 +61,18 @@ "test": "TestAGuestThatNeverConfiguresItsInterfaceIsReported" }, { - "label": "#498: the replay stops noticing that a routed NIC already delivers the address, so it delegates the same /32 to the uplink where the routed NIC's own host route already sits, and shouts an ERROR over a correct host", + "label": "#498, since #548: the address is no longer released from the routed NIC before the uplink is asked for the same /32, which is the `file exists` that shouted an ERROR over a correct host \u2014 and the published address stays where no rule set reaches it", "file": "internal/core/machine/incus_address.go", - "find": "\tif carried {\n\t\treturn nil\n\t}", - "replace": "\tif carried && false {\n\t\treturn nil\n\t}", - "test": "TestReRoutingAnAddressARoutedNICAlreadyCarriesTouchesNothing" + "find": "\tif routed != \"\" {\n\t\tif err := d.releaseFromRoutedNIC(ctx, spec.Machine, routed, spec.Address); err != nil {\n\t\t\treturn err\n\t\t}\n\t}", + "replace": "\tif routed != \"\" && false {\n\t\tif err := d.releaseFromRoutedNIC(ctx, spec.Machine, routed, spec.Address); err != nil {\n\t\t\treturn err\n\t\t}\n\t}", + "test": "TestAPublicAddressMovesOntoTheFilteredNIC" }, { - "label": "#498, the accepting half: every address reads as already delivered, so an address that genuinely needs the OVN path never gets it and the machine answers on nothing", + "label": "#498, the accepting half: every routed NIC reads as carrying the address, so a machine that has already been migrated is re-plugged at every replay", "file": "internal/core/machine/incus_address.go", - "find": "\t\tif routeListContains(cfg[\"ipv4.address\"], address) {\n\t\t\treturn true, nil\n\t\t}", - "replace": "\t\tif routeListContains(cfg[\"ipv4.address\"], address) || true {\n\t\t\treturn true, nil\n\t\t}", - "test": "TestAnAddressNoRoutedNICCarriesStillTravelsTheOVNPath" + "find": "\t\tif routeListContains(cfg[\"ipv4.address\"], address) {\n\t\t\treturn name, nil\n\t\t}", + "replace": "\t\tif routeListContains(cfg[\"ipv4.address\"], address) || true {\n\t\t\treturn name, nil\n\t\t}", + "test": "TestMigratingIsIdempotent" } ] } diff --git a/tools/falsify/specs/pack-firewall-handoff.json b/tools/falsify/specs/pack-firewall-handoff.json index c72304e4..3ae2d52c 100644 --- a/tools/falsify/specs/pack-firewall-handoff.json +++ b/tools/falsify/specs/pack-firewall-handoff.json @@ -35,8 +35,8 @@ "label": "a network attached after the boot leaves its interface without the instance's rule sets", "package": "./internal/providers/exoscale/", "file": "internal/core/machine/plan.go", - "find": "\terr := r.attach(ctx, res, att)\n\tr.Groups.AfterBoot(ctx, res)\n\treturn err", - "replace": "\terr := r.attach(ctx, res, att)\n\tif false {\n\t\tr.Groups.AfterBoot(ctx, res)\n\t}\n\treturn err", + "find": "\terr := r.attach(ctx, res, att)\n\tr.ReplayAddresses(ctx, res)\n\tr.Groups.AfterBoot(ctx, res)\n\treturn err", + "replace": "\terr := r.attach(ctx, res, att)\n\tr.ReplayAddresses(ctx, res)\n\tif false {\n\t\tr.Groups.AfterBoot(ctx, res)\n\t}\n\treturn err", "test": "TestALateNetworkAttachCarriesTheRuleSets" }, { diff --git a/tools/falsify/specs/public-address-migration.json b/tools/falsify/specs/public-address-migration.json new file mode 100644 index 00000000..348032d8 --- /dev/null +++ b/tools/falsify/specs/public-address-migration.json @@ -0,0 +1,56 @@ +{ + "_comment": [ + "#548: a public address is moved onto the filtered NIC, and every guard", + "that makes the move safe or honest is falsified here.", + "", + "The migration itself is falsified by lifecycle-tells-the-truth.json,", + "which owned #498's half of the same branch before it: releasing the", + "address from the routed NIC is what unblocks the uplink write that used", + "to fail with `file exists`, so one mutation answers for both." + ], + "package": "./internal/core/machine/", + "mutations": [ + { + "label": "the migration reconfigures an instance without asking whether the emulator created it", + "file": "internal/core/machine/incus_address.go", + "find": "func (d *Incus) releaseFromRoutedNIC(ctx context.Context, machine, device, address string) error {\n\tif err := d.mustOwnInstance(ctx, machine); err != nil {\n\t\treturn err\n\t}", + "replace": "func (d *Incus) releaseFromRoutedNIC(ctx context.Context, machine, device, address string) error {\n\tif err := d.mustOwnInstance(ctx, machine); err != nil && false {\n\t\treturn err\n\t}", + "test": "TestMigrationRefusesAnInstanceTheEmulatorDidNotCreate" + }, + { + "label": "a routed NIC that carries nothing is reported as an escape again, so every migrated machine fails its firewall apply", + "file": "internal/core/machine/incus_firewall.go", + "find": "\t\t\tif attached != \"\" && len(device.addresses) > 0 {", + "replace": "\t\t\tif attached != \"\" && (len(device.addresses) > 0 || true) {", + "test": "TestARoutedNICThatCarriesNothingIsNotAnEscape" + }, + { + "label": "the hot attach stops replaying the addresses, so the interface that just arrived never takes the published address", + "file": "internal/core/machine/plan.go", + "find": "\terr := r.attach(ctx, res, att)\n\tr.ReplayAddresses(ctx, res)", + "replace": "\terr := r.attach(ctx, res, att)\n\tif false {\n\t\tr.ReplayAddresses(ctx, res)\n\t}", + "test": "TestJoinReplaysTheAddressesBeforeTheFirewall" + }, + { + "label": "the driver goes back to answering one address, the first of the lowest-named interface", + "file": "internal/core/machine/incus.go", + "find": "\t\t\t\tif addr.Family == \"inet\" && addr.Scope == \"global\" &&\n\t\t\t\t\t!slices.Contains(addresses, addr.Address) {", + "replace": "\t\t\t\tif addr.Family == \"inet\" && addr.Scope == \"global\" &&\n\t\t\t\t\t!slices.Contains(addresses, addr.Address) && len(addresses) == 0 {", + "test": "TestInspectReportsEveryAddressTheMachineCarries" + }, + { + "label": "a restored value that is not an address is published to a client anyway", + "file": "internal/core/machine/binding.go", + "find": "\t\taddr, err := netip.ParseAddr(entry)\n\t\tif err != nil {\n\t\t\tcontinue\n\t\t}", + "replace": "\t\taddr, err := netip.ParseAddr(entry)\n\t\tif err != nil {\n\t\t\tout = append(out, entry)\n\t\t\tcontinue\n\t\t}", + "test": "TestARestoredAddressThatIsNotAnAddressIsNotPublished" + }, + { + "label": "a restarted machine is left waiting for a lease nobody offers, so it comes back with neither its private address nor its routes", + "file": "internal/core/machine/incus_ovn.go", + "find": "\t\tif err := d.restorePinnedAddress(ctx, machine, device, devices.own[device]); err != nil {\n\t\t\treturn err\n\t\t}", + "replace": "\t\tif false {\n\t\t\tif err := d.restorePinnedAddress(ctx, machine, device, devices.own[device]); err != nil {\n\t\t\t\treturn err\n\t\t\t}\n\t\t}", + "test": "TestARestartedMachineGetsItsPinnedAddressBack" + } + ] +} diff --git a/tools/falsify/specs/routed-nic.json b/tools/falsify/specs/routed-nic.json index f469602c..bbf45be2 100644 --- a/tools/falsify/specs/routed-nic.json +++ b/tools/falsify/specs/routed-nic.json @@ -46,8 +46,8 @@ { "label": "a rule set on a routed NIC is silently skipped instead of refused", "file": "internal/core/machine/incus_firewall.go", - "find": "\t\tif device.routed {\n\t\t\tif attached != \"\" {", - "replace": "\t\tif device.routed {\n\t\t\tif attached != \"\" && false {", + "find": "\t\tif device.routed {\n\t\t\tif attached != \"\" && len(device.addresses) > 0 {", + "replace": "\t\tif device.routed {\n\t\t\tif attached != \"\" && len(device.addresses) > 0 && false {", "test": "TestApplyFirewallRefusesAGroupOnARoutedNIC" }, { From 6ef2b5d96abce405104c80de0889ad542ae8fb0b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20ROBERT?= Date: Fri, 28 Aug 2026 20:25:12 +0200 Subject: [PATCH 2/6] fix(machine): the network is asked about before the address leaves the routed NIC (#548) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The move is a real change to one of our own machines, and the mode branches below it refuse a network the emulator did not create. Releasing first and refusing after would leave the machine having lost the address it answered on, for a route it never got — and the network name reaching here comes from the pack's Plan.RouteVia, built from stored values a restored snapshot controls. TestAMigrationIsNotStartedForANetworkTheEmulatorDoesNotOwn fails without it, in both driver modes, and asserts that no device edit was issued before the refusal. Also re-dates the two docs/limits.md sections this lot audited: the firewall bounds and the packs' hand-off. Only those two — `tools/docs/limits-acks.py refresh` re-dates all fifty, and nobody looked at the other forty-eight. Assisted-by: Claude Code (claude-opus-5) --- docs/limits-acks.json | 4 +-- internal/core/machine/incus_address.go | 13 ++++++++ .../core/machine/incus_address_replay_test.go | 30 +++++++++++++++++++ 3 files changed, 45 insertions(+), 2 deletions(-) diff --git a/docs/limits-acks.json b/docs/limits-acks.json index 800c35a5..392c1ee4 100644 --- a/docs/limits-acks.json +++ b/docs/limits-acks.json @@ -28,10 +28,10 @@ "The catalogue is a whitelist, and its values are measured": "2026-08-27", "The contracts do not guarantee the same thing": "2026-08-27", "The cost of DNS/TLS interception, measured (#76)": "2026-08-27", - "The firewall enforces, within stated bounds": "2026-08-27", + "The firewall enforces, within stated bounds": "2026-08-28", "The per-parameter half: 18 Scaleway list operations, 72 parameters, each served or refused (#277)": "2026-08-27", "The station reaches an OVN private address only via the network's router, and the posted uplink routes do not go there (#496)": "2026-08-27", - "The three packs hand their security groups to the runtime, within two measured bounds": "2026-08-27", + "The three packs hand their security groups to the runtime, within two measured bounds": "2026-08-28", "Two Outscale filters reach the API only as a payload, and that is `octl`'s gap": "2026-08-27", "What survives a dead emulator, in one table": "2026-08-27", "`feint images resolve` can print a `FEINT_BOOT_IMAGES` line that cannot boot (#476)": "2026-08-27", diff --git a/internal/core/machine/incus_address.go b/internal/core/machine/incus_address.go index 9732a0bc..3242ed48 100644 --- a/internal/core/machine/incus_address.go +++ b/internal/core/machine/incus_address.go @@ -81,6 +81,19 @@ func (d *Incus) RouteAddress(ctx context.Context, spec AddressSpec) error { return err } if routed != "" { + // The network is asked about before the address moves, and that order + // is the answer to what a failure would cost. The release is a real + // change to one of our own machines; refusing afterwards — which is + // what the mode branches below do for a network the emulator did not + // create — would leave the machine having lost the address it + // answered on, for a route it never got. A network name reaching here + // comes from the pack's Plan.RouteVia, which is built from stored + // values a restored snapshot controls. + // TestAMigrationIsNotStartedForANetworkTheEmulatorDoesNotOwn fails + // without this. + if err := d.mustOwn(ctx, network); err != nil { + return err + } if err := d.releaseFromRoutedNIC(ctx, spec.Machine, routed, spec.Address); err != nil { return err } diff --git a/internal/core/machine/incus_address_replay_test.go b/internal/core/machine/incus_address_replay_test.go index fb4940b3..0883fb27 100644 --- a/internal/core/machine/incus_address_replay_test.go +++ b/internal/core/machine/incus_address_replay_test.go @@ -203,6 +203,36 @@ func TestMigrationRefusesAnInstanceTheEmulatorDidNotCreate(t *testing.T) { } } +// TestAMigrationIsNotStartedForANetworkTheEmulatorDoesNotOwn: the move is a +// real change to one of our own machines, and the network it moves the address +// *to* is named by the pack's plan, which is built from stored values a +// restored snapshot controls. Refusing after the release — which is what the +// route itself does for a foreign network — would leave the machine having +// lost the address it answered on, for a route it never got. +func TestAMigrationIsNotStartedForANetworkTheEmulatorDoesNotOwn(t *testing.T) { + for name, ovn := range map[string]bool{"bridge mode": false, "ovn mode": true} { + t.Run(name, func(t *testing.T) { + f := &fakeRuntime{answers: map[string]string{ + "/1.0/instances/srv": routedAndPrivate, + // The network carries no label of ours. + "network get fnt-368798629f8 user." + LabelKey: "\n", + }} + d := newFakeDriver(f) + d.OVN = ovn + + if err := d.RouteAddress(context.Background(), AddressSpec{ + Machine: "srv", Address: "203.0.113.4", Network: "fnt-368798629f8", + }); err == nil { + t.Fatal("the migration was accepted through a network the emulator did not create") + } + if got := f.matching("config device set srv eth0"); len(got) != 0 { + t.Errorf("the address was taken off the machine before the refusal:\n%s", + strings.Join(got, "\n")) + } + }) + } +} + // step is the position of the first command containing substr, -1 when none // does: the migration's verdict is an order, not a set. Its neighbour indexOf // compares whole commands, which cannot express "the device set, whatever it From d680d1c9ea0d5447227f6962528363e03d473b37 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20ROBERT?= Date: Fri, 28 Aug 2026 20:48:19 +0200 Subject: [PATCH 3/6] test(falsify): the migration's own two mutations name the guards this lot added (#548) The first mutation of lifecycle-tells-the-truth.json was retargeted at the release when #548 moved that branch, and pointed at the error handling rather than at the call: a mutation that only swallows the error still emits the release, so the test it names would have stayed green and the spec would have measured nothing. It skips the branch now, keeping `routed` evaluated. And the migration spec gains the mutation for the network's ownership check, which the guard earned when it was added: without it the address is taken off the machine before anyone asks whether the network it is moving to is ours. Assisted-by: Claude Code (claude-opus-5) --- tools/falsify/specs/lifecycle-tells-the-truth.json | 4 ++-- tools/falsify/specs/public-address-migration.json | 7 +++++++ 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/tools/falsify/specs/lifecycle-tells-the-truth.json b/tools/falsify/specs/lifecycle-tells-the-truth.json index 192111e2..129db6ab 100644 --- a/tools/falsify/specs/lifecycle-tells-the-truth.json +++ b/tools/falsify/specs/lifecycle-tells-the-truth.json @@ -63,8 +63,8 @@ { "label": "#498, since #548: the address is no longer released from the routed NIC before the uplink is asked for the same /32, which is the `file exists` that shouted an ERROR over a correct host \u2014 and the published address stays where no rule set reaches it", "file": "internal/core/machine/incus_address.go", - "find": "\tif routed != \"\" {\n\t\tif err := d.releaseFromRoutedNIC(ctx, spec.Machine, routed, spec.Address); err != nil {\n\t\t\treturn err\n\t\t}\n\t}", - "replace": "\tif routed != \"\" && false {\n\t\tif err := d.releaseFromRoutedNIC(ctx, spec.Machine, routed, spec.Address); err != nil {\n\t\t\treturn err\n\t\t}\n\t}", + "find": "\tif routed != \"\" {", + "replace": "\tif routed != \"\" && false {", "test": "TestAPublicAddressMovesOntoTheFilteredNIC" }, { diff --git a/tools/falsify/specs/public-address-migration.json b/tools/falsify/specs/public-address-migration.json index 348032d8..2472c969 100644 --- a/tools/falsify/specs/public-address-migration.json +++ b/tools/falsify/specs/public-address-migration.json @@ -17,6 +17,13 @@ "replace": "func (d *Incus) releaseFromRoutedNIC(ctx context.Context, machine, device, address string) error {\n\tif err := d.mustOwnInstance(ctx, machine); err != nil && false {\n\t\treturn err\n\t}", "test": "TestMigrationRefusesAnInstanceTheEmulatorDidNotCreate" }, + { + "label": "the address is taken off the machine before anyone asks whether the network it is moving to is ours", + "file": "internal/core/machine/incus_address.go", + "find": "\t\tif err := d.mustOwn(ctx, network); err != nil {\n\t\t\treturn err\n\t\t}\n\t\tif err := d.releaseFromRoutedNIC(ctx, spec.Machine, routed, spec.Address); err != nil {", + "replace": "\t\tif err := d.mustOwn(ctx, network); err != nil && false {\n\t\t\treturn err\n\t\t}\n\t\tif err := d.releaseFromRoutedNIC(ctx, spec.Machine, routed, spec.Address); err != nil {", + "test": "TestAMigrationIsNotStartedForANetworkTheEmulatorDoesNotOwn" + }, { "label": "a routed NIC that carries nothing is reported as an escape again, so every migrated machine fails its firewall apply", "file": "internal/core/machine/incus_firewall.go", From 65c0e13b386472fc0514f8b88b4b08c076da418b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20ROBERT?= Date: Fri, 28 Aug 2026 20:52:37 +0200 Subject: [PATCH 4/6] test(machine): the wait's own report is measured again, on a guest that answers and carries nothing (#548) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Moving the pinned-address restore in front of the wait changed what TestAGuestThatNeverConfiguresItsInterfaceIsReported measures without changing its verdict: its fixture refuses every `exec`, so the restore failed first and the error the test read was no longer the wait's. A falsification found it — neutralising the wait's error return left the test green, which is the shape this repository calls a comment rather than a control. The fixture now takes every command and reports no address, so the restore succeeds and only the wait can fail. The assertion names the wait's own wording as well as the machine and the device. Assisted-by: Claude Code (claude-opus-5) --- internal/core/machine/incus_restart_test.go | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/internal/core/machine/incus_restart_test.go b/internal/core/machine/incus_restart_test.go index 9867e3bc..50a55cfd 100644 --- a/internal/core/machine/incus_restart_test.go +++ b/internal/core/machine/incus_restart_test.go @@ -216,6 +216,14 @@ func TestABridgeModeRestartLaysNoAggregates(t *testing.T) { // — an operator reading "the machine is running" and finding it unreachable is // the state #549 was. func TestAGuestThatNeverConfiguresItsInterfaceIsReported(t *testing.T) { + // The guest takes every command and reports no address, which is the one + // shape that isolates the wait: the restore that now runs before it (#548) + // succeeds here, so a failure can only come from the wait itself. + // + // Written that way after a falsification measured the alternative: with an + // `exec` that fails outright, neutralising the wait's own error return left + // this test green — the restore was failing first, and the test had + // silently changed subject. f := &fakeRuntime{} f.hook = func(_ int, args []string) ([]byte, error, bool) { switch args[0] { @@ -232,7 +240,10 @@ func TestAGuestThatNeverConfiguresItsInterfaceIsReported(t *testing.T) { return []byte("10.30.1.1/24\n"), nil, true } case "exec": - return nil, errors.New("incus: Error: Command not found"), true + // The guest answers everything, and carries nothing: `ip address + // add` and `ip link set` succeed, and the read the wait makes + // comes back empty for ever. + return nil, nil, true } return nil, nil, false } @@ -244,7 +255,7 @@ func TestAGuestThatNeverConfiguresItsInterfaceIsReported(t *testing.T) { if err == nil { t.Fatal("a guest that never configured its interface was reported as repaired") } - for _, want := range []string{"srv", "eth1"} { + for _, want := range []string{"srv", "eth1", "carries no IPv4 address"} { if !strings.Contains(err.Error(), want) { t.Errorf("the failure does not name %q: %v", want, err) } From 4c0d5dea6d124a058f8f2bef849caf5df4b15b4d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20ROBERT?= Date: Fri, 28 Aug 2026 21:00:07 +0200 Subject: [PATCH 5/6] fix(machine): a boot's record is re-read once the replay has installed what the plan promised (#548) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The record a boot leaves is written when Start answers, and the addresses the plan promised are installed after that, by the replay. So the record was older than the machine by exactly those: measured 2026-08-28 under `--vm incus-ovn`, a server rebooted through the API once its public address had moved onto its private NIC came back recorded as `10.199.0.2` alone, while the station reached 203.0.113.2 on it in the same pass. No pack reads that combination today — Scaleway declares the route and publishes from its own store, Exoscale reads the layer and declares no route — but it is the same lie #541 closed, waiting for a fourth pack. Reconciler.PowerOn re-reads at the end of the replay, and Binding.Rescan overwrites only when the runtime has something to say, so a virtual machine still booting never erases what an earlier read learned. The pack's Settle hook moved with it, from straight after the start to after that read. It was too early by exactly the same addresses: the one pack that implements it copies the machine's private address into an API field, and internal/providers/outscale's TestAStoppedVmKeepsItsPrivateAddress went red the day the re-read was added and the hook stayed where it was. Assisted-by: Claude Code (claude-opus-5) --- internal/cli/driver_surface_test.go | 3 +- internal/core/machine/address_kind_test.go | 47 +++++++++++++++++++ internal/core/machine/binding.go | 34 ++++++++++++++ internal/core/machine/plan.go | 33 +++++++++---- tools/falsify/specs/interface-plan.json | 4 +- .../specs/public-address-migration.json | 14 ++++++ 6 files changed, 124 insertions(+), 11 deletions(-) diff --git a/internal/cli/driver_surface_test.go b/internal/cli/driver_surface_test.go index ab5df406..e125ee6c 100644 --- a/internal/cli/driver_surface_test.go +++ b/internal/cli/driver_surface_test.go @@ -720,7 +720,8 @@ var mustStayOutside = []string{ // firewall last — is a property of the runtime, and a pack that starts a // machine through the binding skips it. "Binding.Start", "Binding.Stop", "Binding.Remove", "Binding.Addresses", - "Binding.Name", "Binding.PowerOn", "Binding.Refresh", "Binding.ForgetPlacements", + "Binding.Name", "Binding.PowerOn", "Binding.Refresh", "Binding.Rescan", + "Binding.ForgetPlacements", // The unkinded address reader (#541, plural since #548). It was in the // surface until an Exoscale instance with no public IP published its // private-network address as `public-ip`: the binding records whatever diff --git a/internal/core/machine/address_kind_test.go b/internal/core/machine/address_kind_test.go index c294d4a0..80540e07 100644 --- a/internal/core/machine/address_kind_test.go +++ b/internal/core/machine/address_kind_test.go @@ -119,3 +119,50 @@ func TestARestoredAddressThatIsNotAnAddressIsNotPublished(t *testing.T) { t.Errorf("private = %q, want 10.199.0.2", got) } } + +// TestABootRecordsEveryAddressTheReplayInstalled: the record a boot leaves is +// written before the replay installs the addresses the plan promised, so it is +// older than the machine by exactly those. Measured 2026-08-28 under +// `--vm incus-ovn`, on a server rebooted through the API once its public +// address had moved onto its private NIC: the store came back `10.199.0.2` +// alone while the station reached 203.0.113.2 on that machine in the same +// pass, and a pack asking the layer for a public address would have been told +// there was none. +// +// What the recorder can hold is the read itself and its two directions, since +// the double answers one address per machine and cannot stage a set that grows +// mid-boot. The runtime half is the measurement above, re-read after the fix: +// `10.199.0.2,203.0.113.2`. +func TestABootRecordsEveryAddressTheReplayInstalled(t *testing.T) { + b := newGroupSyncBench() + vm := b.machine("m", "") + r := reconcilerBench(b, Plan{Publics: []string{"203.0.113.9"}}) + + if !r.PowerOn(context.Background(), vm, Boot{Image: "ubuntu:24.04"}) { + t.Fatalf("the boot did not start; sequence: %v", b.rec.Sequence()) + } + if got := vm.Runtime["address"]; got == "" { + t.Fatalf("the boot recorded no address at all: %v", vm.Runtime) + } + + // A record older than the machine is replaced by what the runtime holds. + vm.Runtime["address"] = "10.0.0.254" + if !r.binding().Rescan(context.Background(), vm) { + t.Fatalf("a stale record was not re-read: %v", vm.Runtime) + } + if got := vm.Runtime["address"]; got == "10.0.0.254" { + t.Errorf("the record still holds what nobody answered: %q", got) + } + + // And a runtime with nothing to say leaves the record alone, which is what + // a virtual machine still booting looks like: overwriting there would + // erase what an earlier read had learned. + before := vm.Runtime["address"] + vm.Runtime["machine"] = "" + if r.binding().Rescan(context.Background(), vm) { + t.Errorf("a machine the runtime cannot answer for had its record overwritten") + } + if vm.Runtime["address"] != before { + t.Errorf("the record moved to %q, want %q kept", vm.Runtime["address"], before) + } +} diff --git a/internal/core/machine/binding.go b/internal/core/machine/binding.go index 0a98b914..418cb031 100644 --- a/internal/core/machine/binding.go +++ b/internal/core/machine/binding.go @@ -590,6 +590,40 @@ func (b Binding) Refresh(ctx context.Context, res *resource.Resource) bool { return true } +// Rescan re-reads what the machine answers on and records the whole of it, +// where Refresh only fills a blank. +// +// It exists because a boot records too early to be complete (#548). Start +// answers as soon as the instance is up, and the addresses the plan promised +// are installed *after* that — the replay routes them — so the set recorded at +// the boot is whatever the guest happened to carry in between. Measured +// 2026-08-28 under `--vm incus-ovn`, on a server rebooted through the API once +// its public address had moved onto its private NIC: the record came back +// `10.199.0.2` alone while the machine answered on 203.0.113.2 as well, so a +// pack asking the layer for a public address would have been told there was +// none. +// +// Overwriting is the difference, and it is safe in the one direction that +// matters: a runtime with nothing to say leaves the record alone, so a virtual +// machine still booting never erases what an earlier read learned. +// +// TestABootRecordsEveryAddressTheReplayInstalled fails without this. +func (b Binding) Rescan(ctx context.Context, res *resource.Resource) bool { + addresses, found := b.Addresses(ctx, res.ID, res.Runtime[b.RuntimeKey]) + if !found { + return false + } + joined := joinAddresses(addresses) + if joined == "" || joined == res.Runtime[b.AddressKey] { + return false + } + if res.Runtime == nil { + res.Runtime = map[string]string{} + } + res.Runtime[b.AddressKey] = joined + return true +} + // RefreshIfRunning fills the address in when the resource is running and has // none yet. It was written out in all three packs, running-state comparison // included, which is exactly the line the binding exists to hold. diff --git a/internal/core/machine/plan.go b/internal/core/machine/plan.go index d09cbd1b..d36c0d81 100644 --- a/internal/core/machine/plan.go +++ b/internal/core/machine/plan.go @@ -59,11 +59,15 @@ type Reconciler struct { // PlanOf builds the machine's declared interface shape from the resource — // the pack's own field walks, nothing else. PlanOf func(res *resource.Resource) Plan - // Settle, optional, is the pack's bookkeeping between the start and the - // replay: what the boot produced, recorded in the pack's own attributes - // before the firewall expansion reads them. One pack keeps the machine's - // private address in an API field the others do not declare; forcing that - // into the shared sequence would invent a field, so it is a hook. + // Settle, optional, is the pack's bookkeeping at the end of the replay: + // what the boot produced, recorded in the pack's own attributes before the + // firewall expansion reads them. One pack keeps the machine's private + // address in an API field the others do not declare; forcing that into the + // shared sequence would invent a field, so it is a hook. + // + // After the replay and not before it (#548): the addresses the plan + // promised are installed by the replay, so a hook that ran first read a + // record that did not have them yet. Settle func(res *resource.Resource) // PublicBlock is the emulated public range this pack may route. It guards // every address on its way to the driver, routing and unrouting alike: a @@ -129,9 +133,6 @@ func (r Reconciler) PowerOn(ctx context.Context, res *resource.Resource, boot Bo if !r.binding().PowerOn(ctx, res, boot) { return false } - if r.Settle != nil { - r.Settle(res) - } // The launch installed the host half of every public route; this hands // the guest its addresses, and repairs a machine that already existed. // Idempotent, like everything in the replay. @@ -146,6 +147,22 @@ func (r Reconciler) PowerOn(ctx context.Context, res *resource.Resource, boot Bo for _, m := range plan.Memberships { _ = r.attach(ctx, res, m) } + // What the two loops above delivered is read back before the firewall, + // because the record written at the boot is older than they are (#548): + // the addresses the plan promised are installed here, not there. The + // expansion that follows then sees the same set a client will. + r.binding().Rescan(ctx, res) + // The pack's own bookkeeping, after that read and before the expansion. + // It used to run straight after the start, and that was too early by + // exactly the addresses this replay installs: the one pack that + // implements it copies the machine's private address into an API field, + // and a boot whose record was still empty left that field unset for the + // life of the resource — measured by internal/providers/outscale's + // TestAStoppedVmKeepsItsPrivateAddress, which went red the day the read + // above was added and the hook stayed where it was. + if r.Settle != nil { + r.Settle(res) + } // The firewall last, so the expansion sees every address and every // interface the two loops above delivered. This line is the order the // packs kept in comments; TestTheReplayRoutesThenJoinsThenAppliesTheFirewall diff --git a/tools/falsify/specs/interface-plan.json b/tools/falsify/specs/interface-plan.json index a8a51ecb..59ff426e 100644 --- a/tools/falsify/specs/interface-plan.json +++ b/tools/falsify/specs/interface-plan.json @@ -4,8 +4,8 @@ { "label": "the boot replay applies the firewall before joining the memberships, the order three packs kept only in comments (#510)", "file": "internal/core/machine/plan.go", - "find": "\tfor _, m := range plan.Memberships {\n\t\t_ = r.attach(ctx, res, m)\n\t}\n\t// The firewall last, so the expansion sees every address and every\n\t// interface the two loops above delivered. This line is the order the\n\t// packs kept in comments; TestTheReplayRoutesThenJoinsThenAppliesTheFirewall\n\t// is what holds it now.\n\tr.Groups.AfterBoot(ctx, res)", - "replace": "\t// The firewall last, so the expansion sees every address and every\n\t// interface the two loops above delivered. This line is the order the\n\t// packs kept in comments; TestTheReplayRoutesThenJoinsThenAppliesTheFirewall\n\t// is what holds it now.\n\tr.Groups.AfterBoot(ctx, res)\n\tfor _, m := range plan.Memberships {\n\t\t_ = r.attach(ctx, res, m)\n\t}", + "find": "\tfor _, m := range plan.Memberships {\n\t\t_ = r.attach(ctx, res, m)\n\t}", + "replace": "\tr.Groups.AfterBoot(ctx, res)\n\tfor _, m := range plan.Memberships {\n\t\t_ = r.attach(ctx, res, m)\n\t}", "test": "TestTheReplayRoutesThenJoinsThenAppliesTheFirewall" }, { diff --git a/tools/falsify/specs/public-address-migration.json b/tools/falsify/specs/public-address-migration.json index 2472c969..eeff83d9 100644 --- a/tools/falsify/specs/public-address-migration.json +++ b/tools/falsify/specs/public-address-migration.json @@ -58,6 +58,20 @@ "find": "\t\tif err := d.restorePinnedAddress(ctx, machine, device, devices.own[device]); err != nil {\n\t\t\treturn err\n\t\t}", "replace": "\t\tif false {\n\t\t\tif err := d.restorePinnedAddress(ctx, machine, device, devices.own[device]); err != nil {\n\t\t\t\treturn err\n\t\t\t}\n\t\t}", "test": "TestARestartedMachineGetsItsPinnedAddressBack" + }, + { + "label": "the boot's own record is never re-read, so it stays older than the machine by the addresses the replay installed", + "file": "internal/core/machine/binding.go", + "find": "\tif joined == \"\" || joined == res.Runtime[b.AddressKey] {\n\t\treturn false\n\t}", + "replace": "\tif joined == \"\" || joined == res.Runtime[b.AddressKey] || true {\n\t\treturn false\n\t}", + "test": "TestABootRecordsEveryAddressTheReplayInstalled" + }, + { + "label": "a runtime with nothing to say overwrites the record anyway, erasing what an earlier read had learned", + "file": "internal/core/machine/binding.go", + "find": "\taddresses, found := b.Addresses(ctx, res.ID, res.Runtime[b.RuntimeKey])\n\tif !found {\n\t\treturn false\n\t}\n\tjoined := joinAddresses(addresses)\n\tif joined == \"\" || joined == res.Runtime[b.AddressKey] {", + "replace": "\taddresses, found := b.Addresses(ctx, res.ID, res.Runtime[b.RuntimeKey])\n\tif !found && false {\n\t\treturn false\n\t}\n\tjoined := joinAddresses(addresses)\n\tif joined == res.Runtime[b.AddressKey] {", + "test": "TestABootRecordsEveryAddressTheReplayInstalled" } ] } From fa9c2083cac8664e723496da1e6e3e5976b877a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20ROBERT?= Date: Fri, 28 Aug 2026 21:27:40 +0200 Subject: [PATCH 6/6] docs(changelog): the re-read and the hook that moved with it (#548) Assisted-by: Claude Code (claude-opus-5) --- CHANGELOG.fr.md | 9 +++++++++ CHANGELOG.md | 8 ++++++++ 2 files changed, 17 insertions(+) diff --git a/CHANGELOG.fr.md b/CHANGELOG.fr.md index 8059443e..c7a9a3ed 100644 --- a/CHANGELOG.fr.md +++ b/CHANGELOG.fr.md @@ -103,6 +103,15 @@ change ni l'un ni l'autre a sa place dans `git log`. cela coûte l'adresse elle-même. Le chemin de démarrage restitue donc ce que le device réserve avant d'attendre. + Et l'enregistrement laissé par un démarrage est relu à la fin du rejeu, parce + qu'il était écrit avant que le rejeu n'installe les adresses promises par le + plan : la même machine redémarrée était enregistrée avec `10.199.0.2` seule, + alors que la station atteignait 203.0.113.2 dessus dans la même passe. Le + crochet de comptabilité du pack a suivi cette relecture, de juste après le + démarrage à juste après elle : il arrivait trop tôt d'exactement ces + adresses, et c'est le test Outscale sur l'adresse privée d'une Vm arrêtée qui + l'a mesuré. + - **Un arrêt propre rend toute la plomberie qu'aucun client ne peut supprimer, et l'exécution qui fuit est celle qui rougit (#521).** La jambe incus-ovn de `runtime-proof.yml` a échoué au portillon de l'étape suivante sur un runner diff --git a/CHANGELOG.md b/CHANGELOG.md index 40228ad7..890d9cfd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -115,6 +115,14 @@ what this project is judged on: **a response shape a client can observe**, and the address itself. The start path restores what the device reserves before it waits. + And the record a boot leaves is re-read at the end of the replay, because it + was written before the replay installed the addresses the plan promised: the + same rebooted machine recorded `10.199.0.2` alone while the station reached + 203.0.113.2 on it in the same pass. The pack's own bookkeeping hook moved + with that read, from straight after the start to just after it — it ran early + by exactly those addresses, and the Outscale test that keeps a stopped Vm's + private address is what measured it. + - **A graceful exit gives back every piece of host plumbing no client can delete, and a run that leaks is the run that goes red (#521).** The incus-ovn leg of `runtime-proof.yml` failed at the doorstep of the next step on a