diff --git a/CHANGELOG.fr.md b/CHANGELOG.fr.md index e5e224a6..c7a9a3ed 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,65 @@ 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. + + 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 a8ec37e6..890d9cfd 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,62 @@ 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. + + 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 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/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..e125ee6c 100644 --- a/internal/cli/driver_surface_test.go +++ b/internal/cli/driver_surface_test.go @@ -719,17 +719,18 @@ 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.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 + "Binding.Start", "Binding.Stop", "Binding.Remove", "Binding.Addresses", + "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 + // 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..80540e07 --- /dev/null +++ b/internal/core/machine/address_kind_test.go @@ -0,0 +1,168 @@ +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) + } +} + +// 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 8a1a1966..418cb031 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,52 @@ 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 +} + +// 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 } @@ -580,11 +631,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..3242ed48 100644 --- a/internal/core/machine/incus_address.go +++ b/internal/core/machine/incus_address.go @@ -46,39 +46,57 @@ 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 != "" { + // 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 + } } // 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 +144,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 +157,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) } - for _, cfg := range devices.own { + names := make([]string, 0, len(devices.own)) + for name := range devices.own { + names = append(names, name) + } + 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..0883fb27 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,207 @@ 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/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 + + 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 _, 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) + } + } + }) + } +} + +// 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.Fatalf("replaying a delivered address must be a no-op, got: %v", err) + }); err == nil { + t.Fatal("the migration was accepted through a network 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")) - } + 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 +// 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..50a55cfd 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). @@ -150,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] { @@ -159,8 +233,17 @@ 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 + // 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 } @@ -168,11 +251,11 @@ 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") } - 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) } 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..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 @@ -258,8 +275,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 +291,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 +312,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 +327,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 +372,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..59ff426e 100644 --- a/tools/falsify/specs/interface-plan.json +++ b/tools/falsify/specs/interface-plan.json @@ -4,15 +4,15 @@ { "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" }, { "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..129db6ab 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 != \"\" {", + "replace": "\tif routed != \"\" && false {", + "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..eeff83d9 --- /dev/null +++ b/tools/falsify/specs/public-address-migration.json @@ -0,0 +1,77 @@ +{ + "_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": "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", + "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" + }, + { + "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" + } + ] +} 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" }, {