From 17fb953f1fd7b68944d3aa1cb1cae56c9954e0ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20ROBERT?= Date: Fri, 28 Aug 2026 12:30:12 +0200 Subject: [PATCH 1/6] fix(outscale): a filter it cannot read is refused, and four that were declared applied now exclude (#566) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `filters.go` opens on "a filter is either applied or refused, never ignored" and the code beneath it ignored four. Reading a filter has three answers — applied, absent, unreadable — and the reader kept two, folding the third onto "absent", which is the one reading that answers 200 with the whole inventory. Measured on 2026-08-28 against main@2879888, and wider than the issue stated: Filters.VolumeSizes [40] -> 200, both volumes (the declared type) Filters.VolumeSizes ["40"] -> 200, the 40 GiB one (a type the API has not) Filters.VolumeIds "vol-x" -> 200, both volumes Filters.Progresses [7] -> 200, four snapshots at Progress 100 Filters.AccountIds ["…000"] -> 200, four snapshots at AccountId …001 The second line is the inversion: the only shape the pack could read was the one the API does not declare. The last two are not decode failures at all — `snapshotFilters` named three filters `snapshotMatches` never mentioned, and AccountIds is an ordinary list of strings, so no decoder could have caught it. Every shape here comes from contracts/outscale.json, extracted from Outscale's own OpenAPI: VolumeSizes and Progresses are `items: {type: integer}`, the identifier filters are strings, LinkRouteTableMain is a bare boolean. What changed: - a filter carries its kind (filterSpec), and refuseFilters refuses a value not written that way with a 400 naming the filter and the shape; - the matchers fail closed on an unreadable value, so the direction that produces a silent success is the one the code cannot take; - VolumeSizes, Progresses and AccountIds are compared; - ReadVms serves VmStateNames, which is what FiltersVm declares. It served VmStates — FiltersVmsState's, one call over — so it refused the real filter and applied an invented one, and TestTheServedFiltersFilter drove the invented one. Found by the new kind control on the day it was written. Four controls, because no one of them sees what the others do: - TestEveryFilteringOperationDeclaresItsFilters walks the mounted routes whose request carries Filters upstream, so a read added later cannot escape; - TestEveryDeclaredFilterKindIsTheOneTheContractDeclares holds every kind against the document; - TestAnUnreadableFilterMatchesNothingRatherThanEverything pins the branch; - TestEveryDeclaredFilterCanExcludeSomething is the witness no type could be: every declared filter is sent a value nothing carries, and the answer must be empty. It fails on AccountIds without this change. And the instrument that lied on the way. The corpus gate went red on ReadKeypairs, and the recorded REQUEST is what is wrong: oapi-cli sent Filters.KeypairNames as an array, and the proxy's redaction flattened it to one string because KeypairNames matches the "key" carrier. Nothing could see it — the pack read the undecodable filter as absent and answered 200 — so two silent defects cancelled out and the gate passed by accident. redactValue now keeps a list of scalars a list, for the same reason it already keeps a null a null; the two lines already on disk are accepted in corpus/accepted.json, and the staleness rule deletes those entries the day that corpus is recorded again. Driven by the real client: `mise run conformance:leg -- octl` (147.85 s, green) now asserts that VolumeSizes and Progresses exclude, which no leg had ever asserted for any filter. Assisted-by: Claude Code (claude-opus-5) --- corpus/accepted.json | 16 + internal/providers/outscale/audit_test.go | 10 +- internal/providers/outscale/catalog.go | 16 +- internal/providers/outscale/dhcpoptions.go | 4 +- internal/providers/outscale/export_test.go | 54 +++ internal/providers/outscale/filters.go | 365 ++++++++++++++++-- .../outscale/filters_exclusion_test.go | 331 ++++++++++++++++ .../outscale/filters_internal_test.go | 227 +++++++++++ .../providers/outscale/internetservices.go | 4 +- internal/providers/outscale/keypairs.go | 4 +- internal/providers/outscale/loadbalancers.go | 4 +- internal/providers/outscale/natservices.go | 4 +- internal/providers/outscale/netpeerings.go | 6 +- internal/providers/outscale/nets.go | 8 +- internal/providers/outscale/nics.go | 6 +- internal/providers/outscale/publicips.go | 4 +- internal/providers/outscale/routetables.go | 18 +- internal/providers/outscale/securitygroups.go | 4 +- internal/providers/outscale/snapshots.go | 33 +- internal/providers/outscale/tags.go | 4 +- internal/providers/outscale/vms.go | 20 +- internal/providers/outscale/volumes.go | 59 +-- internal/proxy/redact.go | 59 +++ internal/proxy/redact_internal_test.go | 75 ++++ tools/conformance/outscale/octl.sh | 39 ++ 25 files changed, 1268 insertions(+), 106 deletions(-) create mode 100644 internal/providers/outscale/export_test.go create mode 100644 internal/providers/outscale/filters_exclusion_test.go create mode 100644 internal/providers/outscale/filters_internal_test.go diff --git a/corpus/accepted.json b/corpus/accepted.json index c7fb5deb..62690172 100644 --- a/corpus/accepted.json +++ b/corpus/accepted.json @@ -293,6 +293,22 @@ "reason": "The cloud refuses DeleteSecurityGroup with 409 ResourceConflict for about ninety seconds after the machine that wore the group is terminated, and accepts on the thirty-first attempt; this emulator releases the group the moment the machine is terminated and answers 200 at once. A REPLAY CANNOT GRADE THE DIFFERENCE EITHER WAY: the recording is thirty refusals then an acceptance, and a corpus has no ninety seconds in it, since the sanitiser normalises every timestamp to one second apart. Reproducing the delay would make every stack teardown and every conformance run wait it out, which is a product decision rather than a defect to patch. #380 carries it, and internal/providers/outscale/securitygroups.go records the measurement where the skip is.", "issue": "https://github.com/stephrobert/feint/issues/380" }, + { + "file": "outscale/oapi-cli-lifecycle.jsonl", + "operation": "osc/Client.ReadKeypairs", + "kind": "status", + "path": "", + "reason": "NOT A DIVERGENCE OF THE EMULATOR, and the recorded REQUEST is what is wrong. oapi-cli sent Filters.KeypairNames as the array FiltersKeypair declares (contracts/outscale.json); the proxy's redaction replaced the whole value with one string, because KeypairNames matches the \"key\" carrier \u2014 a price internal/proxy/redact.go names as paid knowingly. So the corpus holds {\"Filters\":{\"KeypairNames\":\"REDACTED-17\"}} and the replay reissues it verbatim. Nothing could see it: until 2026-08-28 the Outscale pack read an undecodable filter as an absent one and answered 200 with the whole inventory, so two silent defects cancelled out and this gate passed by accident. #566's type gate refuses the value with a 400, and the absent Keypairs finding on the same exchange is that 400's body. redactValue now keeps a list of scalars a list (TestARedactedListOfScalarsStaysAList), so a recording made after that change carries the array and needs neither entry \u2014 these two cover the two lines recorded before it, and the staleness rule deletes them the day this corpus is recorded again.", + "issue": "https://github.com/stephrobert/feint/issues/566" + }, + { + "file": "outscale/oapi-cli-lifecycle.jsonl", + "operation": "osc/Client.ReadKeypairs", + "kind": "absent", + "path": "Keypairs", + "reason": "NOT A DIVERGENCE OF THE EMULATOR, and the recorded REQUEST is what is wrong. oapi-cli sent Filters.KeypairNames as the array FiltersKeypair declares (contracts/outscale.json); the proxy's redaction replaced the whole value with one string, because KeypairNames matches the \"key\" carrier \u2014 a price internal/proxy/redact.go names as paid knowingly. So the corpus holds {\"Filters\":{\"KeypairNames\":\"REDACTED-17\"}} and the replay reissues it verbatim. Nothing could see it: until 2026-08-28 the Outscale pack read an undecodable filter as an absent one and answered 200 with the whole inventory, so two silent defects cancelled out and this gate passed by accident. #566's type gate refuses the value with a 400, and the absent Keypairs finding on the same exchange is that 400's body. redactValue now keeps a list of scalars a list (TestARedactedListOfScalarsStaysAList), so a recording made after that change carries the array and needs neither entry \u2014 these two cover the two lines recorded before it, and the staleness rule deletes them the day this corpus is recorded again.", + "issue": "https://github.com/stephrobert/feint/issues/566" + }, { "file": "outscale/oapi-cli-lifecycle.jsonl", "operation": "osc/Client.ReadLoadBalancers", diff --git a/internal/providers/outscale/audit_test.go b/internal/providers/outscale/audit_test.go index 1f09805c..2598d5a1 100644 --- a/internal/providers/outscale/audit_test.go +++ b/internal/providers/outscale/audit_test.go @@ -925,8 +925,14 @@ func TestTheServedFiltersFilter(t *testing.T) { {`{"Filters":{"VmTypes":["tinav6.c2r2p2"]}}`, 1}, {`{"Filters":{"SubnetIds":["` + subnetID + `"]}}`, 2}, {`{"Filters":{"SubnetIds":["subnet-deadbeef"]}}`, 0}, - {`{"Filters":{"VmStates":["stopped"]}}`, 2}, - {`{"Filters":{"VmStates":["running"]}}`, 0}, + // VmStateNames is what FiltersVm declares. This test drove VmStates for + // a year — a filter FiltersVmsState declares for ReadVmsState and + // FiltersVm does not have — so the emulator served an invented name and + // refused the real one, and this test agreed with it. That is the + // emulator proving itself against itself, and it is why the kind + // control reads contracts/outscale.json instead of this file (#566). + {`{"Filters":{"VmStateNames":["stopped"]}}`, 2}, + {`{"Filters":{"VmStateNames":["running"]}}`, 0}, // Conjunctive, like upstream: both must hold. {`{"Filters":{"ImageIds":["ami-11111111"],"VmTypes":["tinav6.c2r2p2"]}}`, 1}, {`{"Filters":{"ImageIds":["ami-11111111"],"VmTypes":["nope"]}}`, 0}, diff --git a/internal/providers/outscale/catalog.go b/internal/providers/outscale/catalog.go index 705a8453..d9befc13 100644 --- a/internal/providers/outscale/catalog.go +++ b/internal/providers/outscale/catalog.go @@ -386,7 +386,7 @@ const accountID = "000000000001" // select on is told, instead of being handed everything. // // TestAVmTypeFilterIsAppliedRatherThanIgnored fails without this. -var vmTypeFilters = []string{"VmTypeNames"} +var vmTypeFilters = stringFilters("VmTypeNames") func (p *Pack) readVmTypes(w http.ResponseWriter, r *http.Request) { var req struct { @@ -401,7 +401,7 @@ func (p *Pack) readVmTypes(w http.ResponseWriter, r *http.Request) { if p.refusePageSize(w, req.ResultsPerPage) { return } - if p.refuseUnsupported(w, req.Filters, vmTypeFilters...) { + if p.refuseFilters(w, req.Filters, vmTypeFilters) { return } out := make([]map[string]any, 0, len(vmTypes)) @@ -419,7 +419,7 @@ func (p *Pack) readVmTypes(w http.ResponseWriter, r *http.Request) { // imageFilters are what an image can answer. ImageIds is the one a client // actually sends on the path to a create — it resolves the image it was given // before posting anything. -var imageFilters = []string{"ImageIds", "ImageNames", "AccountIds", "States", "Architectures", "RootDeviceTypes"} +var imageFilters = stringFilters("ImageIds", "ImageNames", "AccountIds", "States", "Architectures", "RootDeviceTypes") // readImages serves the fixed catalogue and everything a client registered on // top of it. Both, always: an image a client made and could not then read back @@ -437,7 +437,7 @@ func (p *Pack) readImages(w http.ResponseWriter, r *http.Request) { if p.refusePageSize(w, req.ResultsPerPage) { return } - if p.refuseUnsupported(w, req.Filters, imageFilters...) { + if p.refuseFilters(w, req.Filters, imageFilters) { return } @@ -487,6 +487,8 @@ func (p *Pack) readRegions(w http.ResponseWriter, r *http.Request) { // to ignore the body entirely and answer a single fixed zone; the body matters // because the Terraform datasource is exactly the client that reads this // before deciding where to place everything else (#269). +var subregionFilters = stringFilters("SubregionNames", "RegionNames", "States") + func (p *Pack) readSubregions(w http.ResponseWriter, r *http.Request) { var req struct { Filters filterSet `json:"Filters"` @@ -500,7 +502,7 @@ func (p *Pack) readSubregions(w http.ResponseWriter, r *http.Request) { if p.refusePageSize(w, req.ResultsPerPage) { return } - if p.refuseUnsupported(w, req.Filters, "SubregionNames", "RegionNames", "States") { + if p.refuseFilters(w, req.Filters, subregionFilters) { return } out := make([]map[string]any, 0, len(p.subregions)) @@ -541,6 +543,8 @@ func netAccessPointServices(region string) []map[string]any { } } +var serviceFilters = stringFilters("ServiceIds", "ServiceNames") + func (p *Pack) readNetAccessPointServices(w http.ResponseWriter, r *http.Request) { var req struct { Filters filterSet `json:"Filters"` @@ -554,7 +558,7 @@ func (p *Pack) readNetAccessPointServices(w http.ResponseWriter, r *http.Request if p.refusePageSize(w, req.ResultsPerPage) { return } - if p.refuseUnsupported(w, req.Filters, "ServiceIds", "ServiceNames") { + if p.refuseFilters(w, req.Filters, serviceFilters) { return } services := netAccessPointServices(p.region) diff --git a/internal/providers/outscale/dhcpoptions.go b/internal/providers/outscale/dhcpoptions.go index d72189f3..b029c1d2 100644 --- a/internal/providers/outscale/dhcpoptions.go +++ b/internal/providers/outscale/dhcpoptions.go @@ -60,7 +60,7 @@ type readDhcpOptionsRequest struct { DryRun *bool `json:"DryRun"` } -var dhcpOptionsFilters = []string{"DhcpOptionsSetIds", "DomainNames"} +var dhcpOptionsFilters = stringFilters("DhcpOptionsSetIds", "DomainNames") func (p *Pack) readDhcpOptions(w http.ResponseWriter, r *http.Request) { var req readDhcpOptionsRequest @@ -71,7 +71,7 @@ func (p *Pack) readDhcpOptions(w http.ResponseWriter, r *http.Request) { if p.refusePageSize(w, req.ResultsPerPage) { return } - if p.refuseUnsupported(w, req.Filters, dhcpOptionsFilters...) { + if p.refuseFilters(w, req.Filters, dhcpOptionsFilters) { return } diff --git a/internal/providers/outscale/export_test.go b/internal/providers/outscale/export_test.go new file mode 100644 index 00000000..7d718c1e --- /dev/null +++ b/internal/providers/outscale/export_test.go @@ -0,0 +1,54 @@ +package outscale + +import "strconv" + +// What the pack lets its own tests see, and nothing else. +// +// TestEveryDeclaredFilterCanExcludeSomething needs two things this package +// keeps to itself: which filters each read declares, and a value of the right +// type that nothing in a store can carry. It lives in the external test package +// because it has to drive a populated emulator through HTTP, which is where the +// pack's server helpers are. + +// DeclaredFilter is one filter a read declares, plus a value of its own type +// that no object can hold. +type DeclaredFilter struct { + Name string + // Absent is the JSON value to send, written in the filter's declared shape. + // Empty when no such value exists: a boolean filter has two values and both + // are in the domain, so it cannot be witnessed this way and is covered by a + // test of its own (TestARootVolumeAnswersItsDeleteOnVmDeletionFilter). + Absent string +} + +// impossibleText is a string no identifier, name, address, state or description +// this pack mints can equal. impossibleNumber is the same for a size, a +// progress or a count. +const ( + impossibleText = "feint-nothing-carries-this-value" + impossibleNumber = 987654 +) + +// DeclaredFilters is filtersByAction in the terms above. +func DeclaredFilters() map[string][]DeclaredFilter { + out := make(map[string][]DeclaredFilter, len(filtersByAction)) + for action, specs := range filtersByAction { + row := make([]DeclaredFilter, 0, len(specs)) + for _, spec := range specs { + row = append(row, DeclaredFilter{Name: spec.Name, Absent: absentValue(spec.Kind)}) + } + out[action] = row + } + return out +} + +func absentValue(kind filterKind) string { + switch kind { + case intList: + return "[" + strconv.Itoa(impossibleNumber) + "]" + case boolean: + return "" + default: + return `["` + impossibleText + `"]` + } +} diff --git a/internal/providers/outscale/filters.go b/internal/providers/outscale/filters.go index 327fe54e..06c508d8 100644 --- a/internal/providers/outscale/filters.go +++ b/internal/providers/outscale/filters.go @@ -2,9 +2,12 @@ package outscale import ( "encoding/json" + "errors" "net/http" "sort" "strings" + + "github.com/stephrobert/feint/internal/core/resource" ) // Filters are where an Outscale read says what it wants, and where this pack @@ -34,28 +37,247 @@ import ( // learns immediately, in the answer, rather than an operator learning later // from a counter. TestAnUnsupportedFilterIsRefused holds that, and the // conformance suite drives it with the real client. +// +// # The third answer, and why the sentence above needed a type to be true +// +// The sentence "applied or refused, never ignored" was written here and the code +// beneath it ignored four filters for a year (#566). The mechanism is +// measurement-integrity's shape one storey down: reading a filter has *three* +// answers — applied, absent, unreadable — and the reader kept only two, folding +// the unreadable one onto "absent", which is precisely the reading that produces +// a silent 200 with the whole inventory in it. +// +// Measured on 2026-08-28 against `main@2879888`, and wider than #566 stated: +// +// Filters.VolumeSizes [40] -> 200, both volumes (the type the API declares) +// Filters.VolumeSizes ["40"] -> 200, one volume (a type it does not) +// Filters.VolumeIds "vol-x" -> 200, both volumes (a string where an array goes) +// Filters.Progresses [7] -> 200, four snapshots of Progress 100 +// Filters.AccountIds ["…0"] -> 200, four snapshots of AccountId …1 +// +// The first two lines are the inversion worth keeping in mind: the only shape +// this pack could read was the one the API does not declare. The last two are +// not decode failures at all — `snapshotFilters` named three filters +// `snapshotMatches` never mentioned, so they were declared applied and never +// compared, which no decoder could have caught. +// +// Two things changed, and each has its own control: +// +// - a filter now carries its kind (filterSpec below), taken from the type +// `contracts/outscale.json` declares for it, and a value that is not +// written that way is refused at the door by refuseFilters rather than +// dropped. TestAFilterOfTheWrongShapeIsRefusedRatherThanIgnored fails +// without it, and TestEveryDeclaredFilterKindIsTheOneTheContractDeclares +// holds the kinds against the contract so the table cannot drift from its +// source. +// - the matchers below fail *closed* on an unreadable value. The gate should +// mean they never see one; if a future handler forgets the gate, an empty +// answer is a defect somebody reports, and a full one is a defect nobody +// ever notices. TestAnUnreadableFilterMatchesNothingRatherThanEverything +// fails without it. +// +// What no type can catch is the third line of the measurement — a filter +// declared and never compared. TestEveryDeclaredFilterCanExcludeSomething is +// the witness for that one: every declared filter is sent a value nothing in +// the store carries, and the answer must be empty. type filterSet map[string]json.RawMessage +// filterKind is how a filter's value is written on the wire. +// +// Not a taste: `contracts/outscale.json` declares a type for every one of the +// 247 filters in the document, and 56 of them are not arrays of strings. Four +// of those 56 are served here. +type filterKind uint8 + +const ( + // stringList is `{"type":"array","items":{"type":"string"}}`, which is 191 + // of the 247. + stringList filterKind = iota + // intList is `{"type":"array","items":{"type":"integer"}}`. Two are served: + // VolumeSizes (ReadVolumes, ReadSnapshots) and Progresses (ReadSnapshots). + intList + // boolean is a bare `{"type":"boolean"}` — not a list, which is why it has + // its own matcher. Two are served: LinkRouteTableMain and + // LinkVolumeDeleteOnVmDeletion. + boolean +) + +// describe names the shape in the words a refusal can use. +func (k filterKind) describe() string { + switch k { + case intList: + return "a list of whole numbers" + case boolean: + return "true or false" + default: + return "a list of strings" + } +} + +// filterSpec is one filter a handler applies, and the shape its value takes. +// +// The kind travels with the name rather than living in a table of its own +// because the same name is not always the same type upstream: CpuGenerations is +// a list of integers on one schema and a list of strings on another. A global +// map keyed by name would be right until the day the second one is served, +// which is the exemption-whose-key-does-not-match-its-subject shape. +type filterSpec struct { + Name string + Kind filterKind +} + +// stringFilters, intFilters and boolFilters declare a handler's filters by kind. +// They read as a sentence at the call site — `stringFilters("VolumeIds", …)`, +// `intFilters("VolumeSizes")` — so the declaration and the shape cannot drift +// apart in the way a parallel list of "the numeric ones" would. +func stringFilters(names ...string) []filterSpec { return specsOf(stringList, names) } +func intFilters(names ...string) []filterSpec { return specsOf(intList, names) } +func boolFilters(names ...string) []filterSpec { return specsOf(boolean, names) } + +func specsOf(kind filterKind, names []string) []filterSpec { + out := make([]filterSpec, 0, len(names)) + for _, name := range names { + out = append(out, filterSpec{Name: name, Kind: kind}) + } + return out +} + +// joinFilters concatenates the groups a handler declares. +func joinFilters(groups ...[]filterSpec) []filterSpec { + var out []filterSpec + for _, group := range groups { + out = append(out, group...) + } + return out +} + +// filterNames is the declared names, sorted, for a message a caller reads. +func filterNames(specs []filterSpec) []string { + out := make([]string, 0, len(specs)) + for _, spec := range specs { + out = append(out, spec.Name) + } + sort.Strings(out) + return out +} + +// filtersByAction names, for every action of this pack that reads Filters, the +// list its handler applies. +// +// The handlers keep referring to the variables directly, so the compiler still +// holds that half; this map exists so a control can *enumerate* the +// declarations, which is the half nothing had. Three tests read it: +// +// - TestEveryFilteringOperationDeclaresItsFilters, which walks the mounted +// routes whose request schema carries a Filters property in +// contracts/outscale.json and fails on one missing here — so a read added +// later cannot escape the two controls below by not being listed; +// - TestEveryDeclaredFilterKindIsTheOneTheContractDeclares, which holds each +// kind against the type that document declares for it; +// - TestEveryDeclaredFilterCanExcludeSomething, the witness that catches what +// no type can: a filter declared here and compared nowhere. Three of +// snapshotFilters' seven were in exactly that state until #566. +var filtersByAction = map[string][]filterSpec{ + "ReadDhcpOptions": dhcpOptionsFilters, + "ReadImages": imageFilters, + "ReadInternetServices": internetServiceFilters, + "ReadKeypairs": keypairFilters, + "ReadLoadBalancers": loadBalancerFilters, + "ReadNatServices": natServiceFilters, + "ReadNetAccessPointServices": serviceFilters, + "ReadNetPeerings": netPeeringFilters, + "ReadNets": netFilters, + "ReadNics": nicFilters, + "ReadPublicIps": publicIPFilters, + "ReadRouteTables": routeTableFilters, + "ReadSecurityGroups": securityGroupFilters, + "ReadSnapshots": snapshotFilters, + "ReadSubnets": subnetFilters, + "ReadSubregions": subregionFilters, + "ReadTags": tagFilters, + "ReadVmTypes": vmTypeFilters, + "ReadVms": vmFilters, + "ReadVmsState": vmStateFilters, + "ReadVolumes": volumeFilters, +} + +// errUnreadableFilter is the third answer: the filter is there, and this pack +// could not read its value. It is deliberately distinct from "absent", which is +// the fold that produced #566. +var errUnreadableFilter = errors.New("the filter's value is not written the way the API declares it") + // strings reads a list-of-strings filter. A filter present but empty matches // nothing, which is what the API does: asking for an empty set of ids is not // asking for everything. -func (f filterSet) strings(name string) ([]string, bool) { +// +// Three returns, not two: present says whether the client sent the filter, and +// err says whether its value could be read. Folding the second onto the first +// is what let a decode failure answer "no filter" and pass every candidate. +func (f filterSet) strings(name string) ([]string, bool, error) { raw, ok := f[name] if !ok { - return nil, false + return nil, false, nil } var out []string if err := json.Unmarshal(raw, &out); err != nil { - return nil, false + return nil, true, errUnreadableFilter + } + return out, true, nil +} + +// ints reads a list-of-integers filter — VolumeSizes and Progresses, the two +// this pack serves that the API declares as `items: {type: integer}`. +// +// json.Unmarshal into []int refuses a string, so `["40"]` is unreadable here +// rather than silently coerced. That is the contract's own type, and coercing +// would put this pack back in the business of guessing what a client meant. +func (f filterSet) ints(name string) ([]int, bool, error) { + raw, ok := f[name] + if !ok { + return nil, false, nil + } + var out []int + if err := json.Unmarshal(raw, &out); err != nil { + return nil, true, errUnreadableFilter + } + return out, true, nil +} + +// boolean reads a bare boolean filter (LinkRouteTableMain, and the volume's +// LinkVolumeDeleteOnVmDeletion). +func (f filterSet) boolean(name string) (bool, bool, error) { + raw, ok := f[name] + if !ok { + return false, false, nil + } + var out bool + if err := json.Unmarshal(raw, &out); err != nil { + return false, true, errUnreadableFilter + } + return out, true, nil +} + +// read decodes one filter according to its declared kind, and answers whether +// the client sent it and whether it could be read. +func (f filterSet) read(spec filterSpec) (bool, error) { + switch spec.Kind { + case intList: + _, present, err := f.ints(spec.Name) + return present, err + case boolean: + _, present, err := f.boolean(spec.Name) + return present, err + default: + _, present, err := f.strings(spec.Name) + return present, err } - return out, true } // unsupported names the filters a client sent that this pack does not apply. -func (f filterSet) unsupported(supported ...string) []string { +func (f filterSet) unsupported(supported []filterSpec) []string { known := make(map[string]bool, len(supported)) - for _, name := range supported { - known[name] = true + for _, spec := range supported { + known[spec.Name] = true } var out []string for name := range f { @@ -67,22 +289,57 @@ func (f filterSet) unsupported(supported ...string) []string { return out } -// refuseUnsupported answers the client when it asked for a filter this pack -// does not apply, and reports whether it did. +// unreadable names the filters a client sent that this pack applies and could +// not read. +func (f filterSet) unreadable(supported []filterSpec) []filterSpec { + var out []filterSpec + for _, spec := range supported { + if _, err := f.read(spec); err != nil { + out = append(out, spec) + } + } + sort.Slice(out, func(i, j int) bool { return out[i].Name < out[j].Name }) + return out +} + +// refuseFilters answers the client when a filter it sent cannot be applied, and +// reports whether it did. Two reasons, one door: // -// The message names the fields and what is served, because a filter refused -// without saying which is a 400 a caller cannot act on. +// - the pack does not apply that filter at all — the original refusal, which +// names the fields and what is served, because a filter refused without +// saying which is a 400 a caller cannot act on; +// - the pack applies it and cannot read the value — #566's third answer. The +// refusal names the filter and the shape the API declares for it, so the +// caller can fix the call rather than believe a 200 that filtered nothing. // -// TestAnUnsupportedFilterIsRefused fails without this. -func (p *Pack) refuseUnsupported(w http.ResponseWriter, f filterSet, supported ...string) bool { - unknown := f.unsupported(supported...) - if len(unknown) == 0 { - return false +// The second half is a refusal where the emulator used to answer 200, so it is +// the half that could diverge: it is right if the real API refuses a value its +// own document types otherwise, and `contracts/outscale.json` is the evidence +// for the type, not for the refusal. No recording under corpus/ carries a +// wrongly-typed filter, so that half is reasoned, not measured — and it is +// reasoned from this file's own rule, which leaves no third place for such a +// value to go. +// +// TestAnUnsupportedFilterIsRefused and +// TestAFilterOfTheWrongShapeIsRefusedRatherThanIgnored fail without this. +func (p *Pack) refuseFilters(w http.ResponseWriter, f filterSet, supported []filterSpec) bool { + if unknown := f.unsupported(supported); len(unknown) > 0 { + p.badRequest(w, "the filter(s) "+strings.Join(unknown, ", ")+ + " are not emulated; this call filters on "+strings.Join(filterNames(supported), ", ")) + return true + } + if unreadable := f.unreadable(supported); len(unreadable) > 0 { + said := make([]string, 0, len(unreadable)) + for _, spec := range unreadable { + said = append(said, spec.Name+" ("+spec.Kind.describe()+")") + } + p.badRequest(w, "the filter(s) "+strings.Join(said, ", ")+ + " carry a value this API does not declare; the shape in brackets is the one "+ + "contracts/outscale.json describes, and a filter that cannot be read is refused "+ + "rather than ignored") + return true } - sort.Strings(supported) - p.badRequest(w, "the filter(s) "+strings.Join(unknown, ", ")+ - " are not emulated; this call filters on "+strings.Join(supported, ", ")) - return true + return false } // matchesStrings reports whether a value passes a list filter. An absent filter @@ -103,7 +360,38 @@ func matchesStrings(f filterSet, name, value string) bool { // resource eleven of thirteen with every earlier resource correct. No unit test // saw it; the fixture did, immediately. func matchesAny(f filterSet, name string, values ...string) bool { - wanted, present := f.strings(name) + wanted, present, err := f.strings(name) + if err != nil { + // refuseFilters should mean this is unreachable, and it is here for the + // day a handler forgets the gate: an unreadable filter that matches + // nothing is a defect somebody reports, and one that matches everything + // is #566, which nobody reported for a year. + return false + } + if !present { + return true + } + for _, candidate := range wanted { + for _, value := range values { + if candidate == value { + return true + } + } + } + return false +} + +// matchesInts reports whether any of a resource's numbers passes a numeric +// filter — the second half of #566. +// +// A resource that carries no number for the filter passes it in nothing: a +// volume whose Attrs hold no Size cannot answer VolumeSizes, and saying so is +// the honest reading. Call it with no values for that case. +func matchesInts(f filterSet, name string, values ...int) bool { + wanted, present, err := f.ints(name) + if err != nil { + return false // fail closed, as matchesAny does and for the same reason + } if !present { return true } @@ -120,13 +408,38 @@ func matchesAny(f filterSet, name string, values ...string) bool { // matchesBool reports whether a boolean value passes a boolean filter, for the // handful Outscale declares that way (LinkRouteTableMain, Default). func matchesBool(f filterSet, name string, value bool) bool { - raw, ok := f[name] - if !ok { - return true + wanted, present, err := f.boolean(name) + if err != nil { + return false // fail closed, as matchesAny does and for the same reason } - var wanted bool - if err := json.Unmarshal(raw, &wanted); err != nil { + if !present { return true } return wanted == value } + +// numbersOf reads the number a rendered view publishes under a key, so a filter +// compares what the client can see rather than something stored beside it. +// +// Through resource.Number, never a type assertion: Attrs crosses encoding/json +// on every snapshot, so a size written as an int comes back a float64 and +// `.(int)` yields zero — the defect #542 measured and +// TestNoPackReadsAStoredNumberByAssertion now refuses in every pack. +// +// Presence is asked of the map and not of the reader, because resource.Number +// answers 0 for "absent", for "not a number" and for "zero" alike, and a filter +// is exactly the caller that must tell those apart. A key that is absent, or +// holds something that is not a number, yields no value at all: the object then +// matches no numeric filter, which is the honest reading of "this emulator does +// not know that number for this object". +func numbersOf(view map[string]any, key string) []int { + value, present := view[key] + if !present { + return nil + } + switch value.(type) { + case nil, string, bool, map[string]any, []any: + return nil + } + return []int{int(resource.Number(value))} +} diff --git a/internal/providers/outscale/filters_exclusion_test.go b/internal/providers/outscale/filters_exclusion_test.go new file mode 100644 index 00000000..343c1492 --- /dev/null +++ b/internal/providers/outscale/filters_exclusion_test.go @@ -0,0 +1,331 @@ +package outscale_test + +import ( + "net/http/httptest" + "sort" + "strings" + "testing" + + "github.com/stephrobert/feint/internal/providers/outscale" +) + +// The witness no type could be. #566's third defect was a filter declared and +// compared nowhere: `snapshotFilters` named AccountIds, Progresses and +// VolumeSizes, and `snapshotMatches` mentioned none of the three. AccountIds is +// an ordinary list of strings, so no decoder, no kind and no contract check +// could ever have seen it — only asking the emulator can. +// +// The question this asks is the one a filter exists to answer: **can it exclude +// anything at all?** Every declared filter is sent a value nothing in the store +// carries, and the answer must be empty. A filter compared nowhere passes every +// candidate and fails here. +// +// Measured before the fix, on 2026-08-28: ReadSnapshots with +// `{"AccountIds":["000000000000"]}` answered 200 and four snapshots whose +// AccountId is 000000000001, and `{"Progresses":[7]}` answered 200 and four +// snapshots whose Progress is 100. +// +// Two properties of the harness matter more than the assertion: +// +// - the unfiltered read must answer something first. A control whose success +// is "nothing came back" is indistinguishable from one that looked at an +// empty store, and every action below would pass vacuously on a store +// nobody populated. +// - the population comes from the pack's own declarations +// (outscale.DeclaredFilters), not from a list written here, so a filter +// added later is covered without anybody remembering this file. +// +// Boolean filters are out of reach of this shape and say so: `true` and `false` +// are both in the domain, so no value witnesses their absence. The two this +// pack serves have tests of their own — +// TestARootVolumeAnswersItsDeleteOnVmDeletionFilter and the route-table suite. +func TestEveryDeclaredFilterCanExcludeSomething(t *testing.T) { + ts := newServer(t) + inventory(t, ts) + + declared := outscale.DeclaredFilters() + if len(declared) == 0 { + t.Fatal("the pack declares no filters at all: the export is broken, not the pack") + } + + // The result key of every read that filters. An action missing here is an + // action this sweep would skip in silence, so it is an error rather than a + // continue. + answers := map[string]string{ + "ReadDhcpOptions": "DhcpOptionsSets", + "ReadImages": "Images", + "ReadInternetServices": "InternetServices", + "ReadKeypairs": "Keypairs", + "ReadLoadBalancers": "LoadBalancers", + "ReadNatServices": "NatServices", + "ReadNetAccessPointServices": "Services", + "ReadNetPeerings": "NetPeerings", + "ReadNets": "Nets", + "ReadNics": "Nics", + "ReadPublicIps": "PublicIps", + "ReadRouteTables": "RouteTables", + "ReadSecurityGroups": "SecurityGroups", + "ReadSnapshots": "Snapshots", + "ReadSubnets": "Subnets", + "ReadSubregions": "Subregions", + "ReadTags": "Tags", + "ReadVmTypes": "VmTypes", + "ReadVms": "Vms", + "ReadVmsState": "VmStates", + "ReadVolumes": "Volumes", + } + + actions := make([]string, 0, len(declared)) + for action := range declared { + actions = append(actions, action) + } + sort.Strings(actions) + + witnessed := 0 + for _, action := range actions { + key, known := answers[action] + if !known { + t.Errorf("%s declares filters and this sweep does not know its result key, so nothing "+ + "here ever asked whether they exclude", action) + continue + } + t.Run(action, func(t *testing.T) { + // The witness: this read answers something before any filter is + // applied, or the assertions below measure an empty store. + status, out := post(t, ts, action, `{}`) + if status != 200 { + t.Fatalf("the unfiltered read answered %d: %v", status, out) + } + all, _ := out[key].([]any) + if len(all) == 0 { + t.Fatalf("the unfiltered read answered no %s, so every filter below would pass "+ + "on an empty answer: the inventory this test builds is what is wrong", key) + } + + for _, filter := range declared[action] { + if filter.Absent == "" { + continue // a boolean has no value outside its own domain + } + body := `{"Filters":{"` + filter.Name + `":` + filter.Absent + `}}` + status, out := post(t, ts, action, body) + if status != 200 { + t.Errorf("%s refused %s: %v", action, body, out) + continue + } + witnessed++ + if got, _ := out[key].([]any); len(got) != 0 { + t.Errorf("%s answered %d %s for a %s nothing carries: this filter cannot "+ + "exclude, which is the shape of #566 — declared applied, compared nowhere", + action, len(got), key, filter.Name) + } + } + }) + } + if witnessed == 0 { + t.Fatal("no filter was witnessed: the sweep ran nothing") + } + // Named, so a refactor that stopped enumerating the three filters this test + // was written for cannot leave it green. + for _, want := range []struct{ action, filter string }{ + {"ReadSnapshots", "AccountIds"}, + {"ReadSnapshots", "Progresses"}, + {"ReadSnapshots", "VolumeSizes"}, + {"ReadVolumes", "VolumeSizes"}, + } { + found := false + for _, filter := range declared[want.action] { + if filter.Name == want.filter { + found = true + } + } + if !found { + t.Errorf("%s no longer declares %s, so #566's own three are out of this sweep's population", + want.action, want.filter) + } + } +} + +// inventory creates one of everything the reads above answer, so the sweep has +// something to exclude. It asserts nothing: what it builds is asserted by the +// unfiltered read at the top of each subtest, which is where an empty answer +// has to fail. +func inventory(t *testing.T, ts *httptest.Server) { + t.Helper() + + netID, subnetID := netAndSubnet(t, ts, "10.90.0.0/16", "10.90.1.0/24") + _, other := post(t, ts, "CreateNet", `{"IpRange":"10.91.0.0/16"}`) + otherNet, _ := other["Net"].(map[string]any) + otherNetID, _ := otherNet["NetId"].(string) + + _, images := post(t, ts, "ReadImages", `{}`) + imageList, _ := images["Images"].([]any) + if len(imageList) == 0 { + t.Fatal("the image catalogue is empty, so no machine can be created here") + } + first, _ := imageList[0].(map[string]any) + imageID, _ := first["ImageId"].(string) + + post(t, ts, "CreateVms", + `{"ImageId":"`+imageID+`","SubnetId":"`+subnetID+`","VmType":"tinav6.c2r2p2"}`) + + _, volume := post(t, ts, "CreateVolume", `{"SubregionName":"eu-west-2a","Size":40,"VolumeType":"standard"}`) + vol, _ := volume["Volume"].(map[string]any) + volumeID, _ := vol["VolumeId"].(string) + post(t, ts, "CreateSnapshot", `{"VolumeId":"`+volumeID+`","Description":"a snapshot"}`) + + post(t, ts, "CreateNic", `{"SubnetId":"`+subnetID+`","Description":"a nic"}`) + post(t, ts, "CreateSecurityGroup", `{"SecurityGroupName":"web","Description":"a group","NetId":"`+netID+`"}`) + post(t, ts, "CreateKeypair", `{"KeypairName":"mine","PublicKey":`+quote(publicKey)+`}`) + post(t, ts, "CreateRouteTable", `{"NetId":"`+netID+`"}`) + post(t, ts, "CreateDhcpOptions", `{"DomainName":"feint.example"}`) + post(t, ts, "CreateNetPeering", `{"SourceNetId":"`+netID+`","AccepterNetId":"`+otherNetID+`"}`) + post(t, ts, "CreateTags", `{"ResourceIds":["`+netID+`"],"Tags":[{"Key":"name","Value":"one"}]}`) + + _, gateway := post(t, ts, "CreateInternetService", `{}`) + igw, _ := gateway["InternetService"].(map[string]any) + if id, _ := igw["InternetServiceId"].(string); id != "" { + post(t, ts, "LinkInternetService", `{"InternetServiceId":"`+id+`","NetId":"`+netID+`"}`) + } + + _, address := post(t, ts, "CreatePublicIp", `{}`) + ip, _ := address["PublicIp"].(map[string]any) + ipID, _ := ip["PublicIpId"].(string) + post(t, ts, "CreateNatService", `{"SubnetId":"`+subnetID+`","PublicIpId":"`+ipID+`"}`) + + post(t, ts, "CreateLoadBalancer", + `{"LoadBalancerName":"feint-lb","Listeners":[{"BackendPort":80,"LoadBalancerPort":80,`+ + `"LoadBalancerProtocol":"TCP"}],"Subnets":["`+subnetID+`"]}`) +} + +// A public key of the shape sshkey.Parse accepts, so CreateKeypair answers 200 +// and ReadKeypairs has something to filter. +const publicKey = "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIIr6pEFlAFO3YU0DNW/r8SkpjdbptN9ockkO2BtIolSD conformance@feint" + +// The volume-size filter excludes a volume of another size. +// +// #566 names this as the test that was missing, and names why: it passes today +// and would pass with the filter deleted, because FiltersVolume declares +// VolumeSizes as a list of integers and the pack read it as a list of strings — +// so the decode failed, the failure was reported as "filter absent", and every +// volume came back with a 200. +// +// Measured on 2026-08-28 against main@2879888, with a 40 GiB and a 10 GiB +// volume in the store: +// +// {"Filters":{"VolumeSizes":[40]}} -> 200, both volumes +// {"Filters":{"VolumeSizes":["40"]}} -> 200, the 40 GiB one +// +// The second line is the inversion worth remembering: the only shape the pack +// could read was the one the API does not declare, so the filter appeared to +// work for anybody who happened to send strings and worked for nobody sending +// what their own client sends. +func TestAVolumeSizeFilterExcludesAVolumeOfAnotherSize(t *testing.T) { + ts := newServer(t) + + _, big := post(t, ts, "CreateVolume", `{"SubregionName":"eu-west-2a","Size":40,"VolumeType":"standard"}`) + bigID, _ := big["Volume"].(map[string]any)["VolumeId"].(string) + _, small := post(t, ts, "CreateVolume", `{"SubregionName":"eu-west-2a","Size":10,"VolumeType":"standard"}`) + smallID, _ := small["Volume"].(map[string]any)["VolumeId"].(string) + if bigID == "" || smallID == "" { + t.Fatalf("the two volumes were not created: %v %v", big, small) + } + + ids := func(t *testing.T, body string) []string { + t.Helper() + status, out := post(t, ts, "ReadVolumes", body) + if status != 200 { + t.Fatalf("ReadVolumes %s answered %d: %v", body, status, out) + } + list, _ := out["Volumes"].([]any) + var got []string + for _, raw := range list { + volume, _ := raw.(map[string]any) + id, _ := volume["VolumeId"].(string) + got = append(got, id) + } + sort.Strings(got) + return got + } + + if got := ids(t, `{}`); len(got) != 2 { + t.Fatalf("the unfiltered read answered %d volume(s), want 2: nothing below would measure anything", len(got)) + } + if got := ids(t, `{"Filters":{"VolumeSizes":[40]}}`); len(got) != 1 || got[0] != bigID { + t.Errorf("VolumeSizes [40] answered %v, want only the 40 GiB volume (%s): a filter that "+ + "cannot exclude is a filter that filters nothing", got, bigID) + } + if got := ids(t, `{"Filters":{"VolumeSizes":[10]}}`); len(got) != 1 || got[0] != smallID { + t.Errorf("VolumeSizes [10] answered %v, want only the 10 GiB volume (%s)", got, smallID) + } + // A size no volume carries excludes both, and a size list carrying two + // sizes answers both: a filter that always matches and one that never + // matches are equally useless. + if got := ids(t, `{"Filters":{"VolumeSizes":[7]}}`); len(got) != 0 { + t.Errorf("VolumeSizes [7] answered %v, want nothing", got) + } + if got := ids(t, `{"Filters":{"VolumeSizes":[10,40]}}`); len(got) != 2 { + t.Errorf("VolumeSizes [10,40] answered %v, want both volumes", got) + } +} + +// A filter whose value is not written the way the API declares it is refused, +// not ignored. +// +// The third answer of #566, made visible. `filters.go` opens on "a filter is +// either applied or refused, never ignored" and the reader beneath it folded +// "could not read this" onto "the client sent nothing", which is the one +// reading that answers 200 with the whole inventory. +// +// The shapes below are refused because contracts/outscale.json declares what +// each filter holds: VolumeSizes and Progresses are `items: {type: integer}`, +// the identifier filters are `items: {type: string}`, and +// LinkRouteTableMain is a bare boolean. The refusal names the filter and the +// shape, because a 400 that does not say which field is one a caller cannot act +// on — the same reason the unsupported-filter refusal names its fields. +func TestAFilterOfTheWrongShapeIsRefusedRatherThanIgnored(t *testing.T) { + ts := newServer(t) + post(t, ts, "CreateVolume", `{"SubregionName":"eu-west-2a","Size":40,"VolumeType":"standard"}`) + post(t, ts, "CreateVolume", `{"SubregionName":"eu-west-2a","Size":10,"VolumeType":"standard"}`) + + for _, probe := range []struct{ action, body, names string }{ + // Strings where the document says integers. This one used to filter, + // which is the inversion: the readable shape was the wrong shape. + {"ReadVolumes", `{"Filters":{"VolumeSizes":["40"]}}`, "VolumeSizes"}, + {"ReadSnapshots", `{"Filters":{"Progresses":["100"]}}`, "Progresses"}, + // A bare string where a list goes, which every identifier filter of + // this pack used to accept and ignore. + {"ReadVolumes", `{"Filters":{"VolumeIds":"vol-12345678"}}`, "VolumeIds"}, + // A list where a bare boolean goes. + {"ReadRouteTables", `{"Filters":{"LinkRouteTableMain":["true"]}}`, "LinkRouteTableMain"}, + // An object, which is neither. + {"ReadVolumes", `{"Filters":{"VolumeStates":{"eq":"available"}}}`, "VolumeStates"}, + } { + status, out := post(t, ts, probe.action, probe.body) + if status == 200 { + list, _ := out["Volumes"].([]any) + t.Errorf("%s answered 200 to %s (%d volume(s)): an unreadable filter must be refused, "+ + "never ignored", probe.action, probe.body, len(list)) + continue + } + if status != 400 { + t.Errorf("%s answered %d to %s, want 400", probe.action, status, probe.body) + continue + } + errs, _ := out["Errors"].([]any) + if len(errs) == 0 { + t.Errorf("%s refused %s without an Errors array: %v", probe.action, probe.body, out) + continue + } + firstError, _ := errs[0].(map[string]any) + details, _ := firstError["Details"].(string) + if !strings.Contains(details, probe.names) { + t.Errorf("%s refused %s without naming the filter: %q", probe.action, probe.body, details) + } + } + + // The accepting half: the shape the document declares is served, or this + // test is satisfied by a pack that refuses everything. + if status, out := post(t, ts, "ReadVolumes", `{"Filters":{"VolumeSizes":[40]}}`); status != 200 { + t.Errorf("the declared shape was refused with %d: %v", status, out) + } +} diff --git a/internal/providers/outscale/filters_internal_test.go b/internal/providers/outscale/filters_internal_test.go new file mode 100644 index 00000000..1f05fe86 --- /dev/null +++ b/internal/providers/outscale/filters_internal_test.go @@ -0,0 +1,227 @@ +package outscale + +import ( + "encoding/json" + "path/filepath" + "sort" + "strings" + "testing" + + "github.com/stephrobert/feint/internal/contract" + "github.com/stephrobert/feint/internal/core/emulator" +) + +// The controls #566 needed, and none of them is a shape check. +// +// The defect was that "a filter is either applied or refused, never ignored" +// was a sentence in filters.go with four counter-examples underneath it. Three +// separate things had to become measurable, because no one of them would have +// caught the other two: +// +// 1. every read that takes Filters declares what it filters on, so a handler +// added later cannot escape the two controls below by not being listed; +// 2. every declared kind is the type the provider's own document declares, so +// the reason a value is refused comes from contracts/outscale.json rather +// than from somebody's memory; +// 3. an unreadable value fails closed in the matchers, so the one direction +// that produces a silent success is the one direction the code cannot take. +// +// The witness for the fourth — a filter declared and compared nowhere, which no +// type can see — is TestEveryDeclaredFilterCanExcludeSomething, beside the pack +// tests that can drive a populated store. + +func outscaleContract(t *testing.T) *contract.Doc { + t.Helper() + doc, err := contract.Load(filepath.Join("..", "..", "..", "contracts", "outscale.json")) + if err != nil { + t.Fatalf("load the contract: %v", err) + } + return doc +} + +// filterSchemaOf resolves an action to the schema of its Filters property. +func filterSchemaOf(doc *contract.Doc, action string) (contract.Schema, bool) { + op, known := doc.Operations[action] + if !known { + return contract.Schema{}, false + } + request, known := doc.Schemas[op.Request] + if !known { + return contract.Schema{}, false + } + filters, declared := request.Properties["Filters"] + if !declared || filters.Ref == "" { + return contract.Schema{}, false + } + schema, known := doc.Schemas[filters.Ref] + return schema, known +} + +// Every action this pack mounts whose request carries Filters declares what it +// filters on, in filtersByAction. +// +// Without this the two controls below cover whatever somebody remembered to add +// to the map, which is the coverage-by-vigilance that #566 is a year-long +// example of. The population comes from two places that cannot both be wrong in +// the same direction: the pack's own mounted routes, and the provider's +// document saying which of those take a Filters object. +func TestEveryFilteringOperationDeclaresItsFilters(t *testing.T) { + doc := outscaleContract(t) + env := emulator.DefaultEnv() + pack := New(env) + + var missing []string + mounted := 0 + for _, route := range pack.Routes() { + action := strings.TrimPrefix(route.Path, pathPrefix) + if _, takesFilters := filterSchemaOf(doc, action); !takesFilters { + continue + } + mounted++ + if _, declared := filtersByAction[action]; !declared { + missing = append(missing, action) + } + } + // A control that looks for absence proves it can find first: an empty + // population here would pass while measuring nothing. + if mounted == 0 { + t.Fatal("no mounted route takes a Filters object, so this test compared nothing: " + + "the contract lookup or the path prefix is what broke, not the pack") + } + if len(missing) > 0 { + sort.Strings(missing) + t.Errorf("these mounted actions take Filters and declare none in filtersByAction: %s\n"+ + "a read that is not listed there is a read the kind control and the exclusion "+ + "witness never look at", strings.Join(missing, ", ")) + } + + // And the other direction: an entry naming an action this pack does not + // mount is an entry nothing exercises. + served := map[string]bool{} + for _, route := range pack.Routes() { + served[strings.TrimPrefix(route.Path, pathPrefix)] = true + } + for action := range filtersByAction { + if !served[action] { + t.Errorf("filtersByAction names %s, which this pack does not mount", action) + } + } +} + +// Every declared kind is the type contracts/outscale.json declares. +// +// This is the source rule, applied to the one place #566 got wrong: VolumeSizes +// and Progresses are `items: {type: integer}` upstream and were read as strings, +// so `[40]` failed to decode and was reported as "no filter". The document is +// the authority, and the day one of these types moves upstream this fails +// instead of the filter quietly matching everything again. +func TestEveryDeclaredFilterKindIsTheOneTheContractDeclares(t *testing.T) { + doc := outscaleContract(t) + + compared := 0 + for action, specs := range filtersByAction { + schema, known := filterSchemaOf(doc, action) + if !known { + t.Errorf("%s: the contract declares no Filters schema, so its declarations are held against nothing", action) + continue + } + for _, spec := range specs { + property, declared := schema.Properties[spec.Name] + if !declared { + t.Errorf("%s declares the filter %s, which the contract's own Filters schema does not have", + action, spec.Name) + continue + } + compared++ + want := kindOfProperty(property) + if want != spec.Kind { + t.Errorf("%s reads %s as %s; the contract declares %s", + action, spec.Name, spec.Kind.describe(), want.describe()) + } + } + } + if compared == 0 { + t.Fatal("no filter was compared against the contract: the schema lookup is broken, " + + "and a green run here would mean nothing") + } + // The witness, in the terms of the defect this test exists for: the two + // integer filters that were read as strings for a year are in the + // population, and named, so a refactor that stopped enumerating them cannot + // leave this test green. + if kind := kindOf(filtersByAction["ReadVolumes"], "VolumeSizes"); kind != intList { + t.Errorf("ReadVolumes reads VolumeSizes as %s; #566 is the measurement that says it is a list of integers", kind.describe()) + } + if kind := kindOf(filtersByAction["ReadSnapshots"], "Progresses"); kind != intList { + t.Errorf("ReadSnapshots reads Progresses as %s; #566 is the measurement that says it is a list of integers", kind.describe()) + } +} + +// kindOfProperty reads the contract's declared type as one of this pack's kinds. +func kindOfProperty(p contract.Property) filterKind { + switch { + case p.Type == "boolean": + return boolean + case p.Type == "array" && p.Items != nil && (p.Items.Type == "integer" || p.Items.Type == "number"): + return intList + default: + return stringList + } +} + +func kindOf(specs []filterSpec, name string) filterKind { + for _, spec := range specs { + if spec.Name == name { + return spec.Kind + } + } + return stringList +} + +// An unreadable filter matches nothing, never everything. +// +// refuseFilters should mean the matchers never see one, and this is the second +// line: the whole of #566 is that the code took the other branch, so the branch +// itself has to be pinned. An empty answer is a defect somebody reports on the +// day it appears; a full answer is a defect nobody reported for a year. +func TestAnUnreadableFilterMatchesNothingRatherThanEverything(t *testing.T) { + set := func(body string) filterSet { + var f filterSet + if err := json.Unmarshal([]byte(body), &f); err != nil { + t.Fatalf("build the filter set: %v", err) + } + return f + } + + // The three shapes, each one wrong for the reader that meets it. + strings := set(`{"VolumeIds":"vol-42"}`) // a string where a list goes + numbers := set(`{"VolumeSizes":["40"]}`) // strings where integers go + flag := set(`{"LinkRouteTableMain":["true"]}`) // a list where a bare bool goes + good := set(`{"VolumeIds":["vol-42"],"VolumeSizes":[40],"LinkRouteTableMain":true}`) + + if matchesStrings(strings, "VolumeIds", "vol-42") { + t.Error("an unreadable string filter matched: that is the silent 200 with the whole inventory in it") + } + if matchesInts(numbers, "VolumeSizes", 40) { + t.Error("an unreadable integer filter matched") + } + if matchesBool(flag, "LinkRouteTableMain", true) { + t.Error("an unreadable boolean filter matched") + } + + // The accepting half, because a matcher that refuses everything passes the + // three assertions above and breaks every read. + if !matchesStrings(good, "VolumeIds", "vol-42") { + t.Error("a readable string filter did not match the value it names") + } + if !matchesInts(good, "VolumeSizes", 40) { + t.Error("a readable integer filter did not match the value it names") + } + if !matchesBool(good, "LinkRouteTableMain", true) { + t.Error("a readable boolean filter did not match the value it names") + } + // And an absent filter still passes everything, which is the one case where + // "no filter" and "match all" are the same thing. + if !matchesStrings(filterSet{}, "VolumeIds", "vol-42") || !matchesInts(filterSet{}, "VolumeSizes", 40) { + t.Error("an absent filter excluded a candidate") + } +} diff --git a/internal/providers/outscale/internetservices.go b/internal/providers/outscale/internetservices.go index ec38cecf..cf75d79a 100644 --- a/internal/providers/outscale/internetservices.go +++ b/internal/providers/outscale/internetservices.go @@ -52,7 +52,7 @@ func (p *Pack) createInternetService(w http.ResponseWriter, r *http.Request) { }) } -var internetServiceFilters = []string{"InternetServiceIds", "LinkNetIds"} +var internetServiceFilters = stringFilters("InternetServiceIds", "LinkNetIds") func (p *Pack) readInternetServices(w http.ResponseWriter, r *http.Request) { var req struct { @@ -67,7 +67,7 @@ func (p *Pack) readInternetServices(w http.ResponseWriter, r *http.Request) { if p.refusePageSize(w, req.ResultsPerPage) { return } - if p.refuseUnsupported(w, req.Filters, internetServiceFilters...) { + if p.refuseFilters(w, req.Filters, internetServiceFilters) { return } diff --git a/internal/providers/outscale/keypairs.go b/internal/providers/outscale/keypairs.go index eda82950..d0841d3b 100644 --- a/internal/providers/outscale/keypairs.go +++ b/internal/providers/outscale/keypairs.go @@ -36,7 +36,7 @@ type readKeypairsRequest struct { // keypairFilters are what a keypair answers from what is stored. Tags are not // modelled on a keypair here, so they are refused. -var keypairFilters = []string{"KeypairNames", "KeypairFingerprints", "KeypairTypes"} +var keypairFilters = stringFilters("KeypairNames", "KeypairFingerprints", "KeypairTypes") type deleteKeypairRequest struct { KeypairName string `json:"KeypairName"` @@ -107,7 +107,7 @@ func (p *Pack) readKeypairs(w http.ResponseWriter, r *http.Request) { if p.refusePageSize(w, req.ResultsPerPage) { return } - if p.refuseUnsupported(w, req.Filters, keypairFilters...) { + if p.refuseFilters(w, req.Filters, keypairFilters) { return } diff --git a/internal/providers/outscale/loadbalancers.go b/internal/providers/outscale/loadbalancers.go index d038595e..e18299e0 100644 --- a/internal/providers/outscale/loadbalancers.go +++ b/internal/providers/outscale/loadbalancers.go @@ -352,7 +352,7 @@ func (p *Pack) sourceSecurityGroup(sgID string) map[string]any { } } -var loadBalancerFilters = []string{"LoadBalancerNames"} +var loadBalancerFilters = stringFilters("LoadBalancerNames") // readLoadBalancers answers the real inventory. It was the family's first // served operation — before anything could be created here, the empty answer @@ -371,7 +371,7 @@ func (p *Pack) readLoadBalancers(w http.ResponseWriter, r *http.Request) { p.badRequest(w, err.Error()) return } - if p.refuseUnsupported(w, req.Filters, loadBalancerFilters...) { + if p.refuseFilters(w, req.Filters, loadBalancerFilters) { return } out := make([]map[string]any, 0) diff --git a/internal/providers/outscale/natservices.go b/internal/providers/outscale/natservices.go index 173dd49e..a3f60d82 100644 --- a/internal/providers/outscale/natservices.go +++ b/internal/providers/outscale/natservices.go @@ -104,7 +104,7 @@ func (p *Pack) createNatService(w http.ResponseWriter, r *http.Request) { }) } -var natServiceFilters = []string{"NatServiceIds", "NetIds", "SubnetIds", "States"} +var natServiceFilters = stringFilters("NatServiceIds", "NetIds", "SubnetIds", "States") func (p *Pack) readNatServices(w http.ResponseWriter, r *http.Request) { var req struct { @@ -119,7 +119,7 @@ func (p *Pack) readNatServices(w http.ResponseWriter, r *http.Request) { if p.refusePageSize(w, req.ResultsPerPage) { return } - if p.refuseUnsupported(w, req.Filters, natServiceFilters...) { + if p.refuseFilters(w, req.Filters, natServiceFilters) { return } diff --git a/internal/providers/outscale/netpeerings.go b/internal/providers/outscale/netpeerings.go index 57ec0156..f9ef75c3 100644 --- a/internal/providers/outscale/netpeerings.go +++ b/internal/providers/outscale/netpeerings.go @@ -110,12 +110,12 @@ type readNetPeeringsRequest struct { // the tag filters are refused rather than silently matched, the same triage // as every other Read* of this pack; the Terraform provider's own read sends // NetPeeringIds and nothing else (resource_net_peering.go, v1.8.0). -var netPeeringFilters = []string{ +var netPeeringFilters = stringFilters( "NetPeeringIds", "AccepterNetAccountIds", "AccepterNetIpRanges", "AccepterNetNetIds", "SourceNetAccountIds", "SourceNetIpRanges", "SourceNetNetIds", "StateMessages", "StateNames", -} +) func (p *Pack) createNetPeering(w http.ResponseWriter, r *http.Request) { var req createNetPeeringRequest @@ -344,7 +344,7 @@ func (p *Pack) readNetPeerings(w http.ResponseWriter, r *http.Request) { if p.refusePageSize(w, req.ResultsPerPage) { return } - if p.refuseUnsupported(w, req.Filters, netPeeringFilters...) { + if p.refuseFilters(w, req.Filters, netPeeringFilters) { return } diff --git a/internal/providers/outscale/nets.go b/internal/providers/outscale/nets.go index df015f06..6109a807 100644 --- a/internal/providers/outscale/nets.go +++ b/internal/providers/outscale/nets.go @@ -84,11 +84,11 @@ type readNetsRequest struct { // keyword. Refusing the filter fails every `terraform destroy` of an // outscale_dhcp_option. var ( - netFilters = []string{"NetIds", "IpRanges", "States", "DhcpOptionsSetIds"} + netFilters = stringFilters("NetIds", "IpRanges", "States", "DhcpOptionsSetIds") // SubregionNames is served since the subregion became a stored fact // (#269): FiltersSubnet declares it (osc-sdk-go, client.gen.go:5058), and // it is how a stack that spreads subnets across zones reads its own back. - subnetFilters = []string{"SubnetIds", "NetIds", "IpRanges", "States", "SubregionNames"} + subnetFilters = stringFilters("SubnetIds", "NetIds", "IpRanges", "States", "SubregionNames") ) type readSubnetsRequest struct { @@ -170,7 +170,7 @@ func (p *Pack) readNets(w http.ResponseWriter, r *http.Request) { if p.refusePageSize(w, req.ResultsPerPage) { return } - if p.refuseUnsupported(w, req.Filters, netFilters...) { + if p.refuseFilters(w, req.Filters, netFilters) { return } @@ -405,7 +405,7 @@ func (p *Pack) readSubnets(w http.ResponseWriter, r *http.Request) { if p.refusePageSize(w, req.ResultsPerPage) { return } - if p.refuseUnsupported(w, req.Filters, subnetFilters...) { + if p.refuseFilters(w, req.Filters, subnetFilters) { return } diff --git a/internal/providers/outscale/nics.go b/internal/providers/outscale/nics.go index 9e9ae369..80d0b8e3 100644 --- a/internal/providers/outscale/nics.go +++ b/internal/providers/outscale/nics.go @@ -42,10 +42,10 @@ const kindNic = "nic" // the same reason they are on a Vm: `terraform destroy` asks which interfaces // still wear a security group before it removes one, and a filter it sends and // this pack refuses fails the destroy after a successful apply. -var nicFilters = []string{ +var nicFilters = stringFilters( "NicIds", "LinkNicVmIds", "SubnetIds", "NetIds", "SecurityGroupIds", "SecurityGroupNames", -} +) func (p *Pack) readNics(w http.ResponseWriter, r *http.Request) { var req struct { @@ -60,7 +60,7 @@ func (p *Pack) readNics(w http.ResponseWriter, r *http.Request) { if p.refusePageSize(w, req.ResultsPerPage) { return } - if p.refuseUnsupported(w, req.Filters, nicFilters...) { + if p.refuseFilters(w, req.Filters, nicFilters) { return } diff --git a/internal/providers/outscale/publicips.go b/internal/providers/outscale/publicips.go index 5ad0349e..de96adda 100644 --- a/internal/providers/outscale/publicips.go +++ b/internal/providers/outscale/publicips.go @@ -194,7 +194,7 @@ func (p *Pack) createPublicIP(w http.ResponseWriter, r *http.Request) { // publicIPFilters are what a stored address can answer. LinkPublicIpIds is here // because the Terraform provider reads the link back by its own id right after // creating it — without it, `outscale_public_ip_link` fails the apply. -var publicIPFilters = []string{"PublicIpIds", "PublicIps", "LinkPublicIpIds", "VmIds", "NicIds"} +var publicIPFilters = stringFilters("PublicIpIds", "PublicIps", "LinkPublicIpIds", "VmIds", "NicIds") func (p *Pack) readPublicIPs(w http.ResponseWriter, r *http.Request) { var req struct { @@ -209,7 +209,7 @@ func (p *Pack) readPublicIPs(w http.ResponseWriter, r *http.Request) { if p.refusePageSize(w, req.ResultsPerPage) { return } - if p.refuseUnsupported(w, req.Filters, publicIPFilters...) { + if p.refuseFilters(w, req.Filters, publicIPFilters) { return } diff --git a/internal/providers/outscale/routetables.go b/internal/providers/outscale/routetables.go index ed693866..c2ff2bd3 100644 --- a/internal/providers/outscale/routetables.go +++ b/internal/providers/outscale/routetables.go @@ -79,12 +79,16 @@ type readRouteTablesRequest struct { // routeTableFilters are what a stored table can answer. The nested ones matter // as much as the top-level: the Terraform provider reads a route back by // filtering on its destination, and a table by the subnet its link names. -var routeTableFilters = []string{ - "RouteTableIds", "NetIds", - "LinkRouteTableIds", "LinkSubnetIds", "LinkRouteTableMain", - "RouteDestinationIpRanges", "RouteGatewayIds", "RouteNatServiceIds", - "RouteCreationMethods", "RouteStates", -} +var routeTableFilters = joinFilters( + stringFilters( + "RouteTableIds", "NetIds", + "LinkRouteTableIds", "LinkSubnetIds", + "RouteDestinationIpRanges", "RouteGatewayIds", "RouteNatServiceIds", + "RouteCreationMethods", "RouteStates", + ), + // FiltersRouteTable declares this one as a bare boolean, not a list. + boolFilters("LinkRouteTableMain"), +) func (p *Pack) readRouteTables(w http.ResponseWriter, r *http.Request) { var req readRouteTablesRequest @@ -95,7 +99,7 @@ func (p *Pack) readRouteTables(w http.ResponseWriter, r *http.Request) { if p.refusePageSize(w, req.ResultsPerPage) { return } - if p.refuseUnsupported(w, req.Filters, routeTableFilters...) { + if p.refuseFilters(w, req.Filters, routeTableFilters) { return } diff --git a/internal/providers/outscale/securitygroups.go b/internal/providers/outscale/securitygroups.go index 56ab2ac0..2d421611 100644 --- a/internal/providers/outscale/securitygroups.go +++ b/internal/providers/outscale/securitygroups.go @@ -86,7 +86,7 @@ type readSecurityGroupsRequest struct { // securityGroupFilters are what a stored group can answer. The API declares 21; // the rest are refused rather than silently matched, per filters.go. -var securityGroupFilters = []string{"SecurityGroupIds", "SecurityGroupNames", "NetIds", "Descriptions"} +var securityGroupFilters = stringFilters("SecurityGroupIds", "SecurityGroupNames", "NetIds", "Descriptions") func (p *Pack) readSecurityGroups(w http.ResponseWriter, r *http.Request) { var req readSecurityGroupsRequest @@ -97,7 +97,7 @@ func (p *Pack) readSecurityGroups(w http.ResponseWriter, r *http.Request) { if p.refusePageSize(w, req.ResultsPerPage) { return } - if p.refuseUnsupported(w, req.Filters, securityGroupFilters...) { + if p.refuseFilters(w, req.Filters, securityGroupFilters) { return } diff --git a/internal/providers/outscale/snapshots.go b/internal/providers/outscale/snapshots.go index 914a0ad6..1ca6e7a9 100644 --- a/internal/providers/outscale/snapshots.go +++ b/internal/providers/outscale/snapshots.go @@ -87,10 +87,13 @@ func (p *Pack) createSnapshot(w http.ResponseWriter, r *http.Request) { // snapshotFilters: the same lesson as volumes — a client filters on what it // knows, and a filter refused is an apply that stops. -var snapshotFilters = []string{ - "SnapshotIds", "VolumeIds", "States", "Descriptions", - "AccountIds", "Progresses", "VolumeSizes", -} +var snapshotFilters = joinFilters( + stringFilters("SnapshotIds", "VolumeIds", "States", "Descriptions", "AccountIds"), + // FiltersSnapshot declares both of these as lists of integers, and both + // were declared here and compared nowhere: a client asking for + // Progresses [7] got four snapshots of Progress 100, with a 200 (#566). + intFilters("Progresses", "VolumeSizes"), +) func (p *Pack) readSnapshots(w http.ResponseWriter, r *http.Request) { var req struct { @@ -105,7 +108,7 @@ func (p *Pack) readSnapshots(w http.ResponseWriter, r *http.Request) { if p.refusePageSize(w, req.ResultsPerPage) { return } - if p.refuseUnsupported(w, req.Filters, snapshotFilters...) { + if p.refuseFilters(w, req.Filters, snapshotFilters) { return } @@ -171,11 +174,29 @@ func snapshotView(res *resource.Resource) map[string]any { // snapshotMatches filters a rendered snapshot, so a catalogue entry and one a // client cut are filtered by exactly the same rules — the reason imageMatches // exists on the other half of the same pair. +// Three of the seven filters snapshotFilters declares were compared nowhere +// here until #566: AccountIds, Progresses and VolumeSizes. Two of them are the +// integer filters that filterSet.strings could never have read; AccountIds is a +// plain list of strings and no decoder could have caught it — the list simply +// named a filter the comparison did not mention. Measured on 2026-08-28 before +// the fix: AccountIds ["000000000000"] answered 200 with four snapshots whose +// AccountId is 000000000001, and Progresses [7] answered 200 with four +// snapshots whose Progress is 100. +// +// The values come from the rendered view rather than from Attrs, so a filter +// and a read cannot disagree about what a snapshot carries — the reason this +// function takes a view at all. +// TestEveryDeclaredFilterCanExcludeSomething fails without these three lines. func snapshotMatches(view map[string]any, f filterSet) bool { + progress := numbersOf(view, "Progress") + size := numbersOf(view, "VolumeSize") return matchesStrings(f, "SnapshotIds", stringOf(view["SnapshotId"])) && matchesStrings(f, "VolumeIds", stringOf(view["VolumeId"])) && matchesStrings(f, "States", stringOf(view["State"])) && - matchesStrings(f, "Descriptions", stringOf(view["Description"])) + matchesStrings(f, "Descriptions", stringOf(view["Description"])) && + matchesStrings(f, "AccountIds", stringOf(view["AccountId"])) && + matchesInts(f, "Progresses", progress...) && + matchesInts(f, "VolumeSizes", size...) } // findSnapshot resolves a SnapshotId to what it holds, whichever half of the diff --git a/internal/providers/outscale/tags.go b/internal/providers/outscale/tags.go index ba890b95..10890061 100644 --- a/internal/providers/outscale/tags.go +++ b/internal/providers/outscale/tags.go @@ -199,6 +199,8 @@ func (p *Pack) deleteTags(w http.ResponseWriter, r *http.Request) { // readTags answers the flat view: every tag of every resource, with what it is // attached to. It is a different shape from the Tags a resource carries, which // is why the API has both. +var tagFilters = stringFilters("ResourceIds", "Keys", "Values", "ResourceTypes") + func (p *Pack) readTags(w http.ResponseWriter, r *http.Request) { var req struct { Filters filterSet `json:"Filters"` @@ -211,7 +213,7 @@ func (p *Pack) readTags(w http.ResponseWriter, r *http.Request) { if p.refusePageSize(w, req.ResultsPerPage) { return } - if p.refuseUnsupported(w, req.Filters, "ResourceIds", "Keys", "Values", "ResourceTypes") { + if p.refuseFilters(w, req.Filters, tagFilters) { return } diff --git a/internal/providers/outscale/vms.go b/internal/providers/outscale/vms.go index 82499011..c8836e56 100644 --- a/internal/providers/outscale/vms.go +++ b/internal/providers/outscale/vms.go @@ -50,8 +50,18 @@ type readVmsRequest struct { // else FiltersVm declares — 66 fields, most of them about block device // mappings, NIC sub-objects and account ids the emulator has no model for — is // refused rather than ignored. -var vmFilters = []string{ - "VmIds", "VmStates", "ImageIds", "VmTypes", "KeypairNames", +// VmStateNames, not VmStates, and the difference is not cosmetic. FiltersVm +// declares VmStateNames (osc-sdk-go, client.gen.go, and contracts/outscale.json +// after it); VmStates belongs to FiltersVmsState, which is ReadVmsState's, one +// call over. This pack served VmStates here and refused VmStateNames, so a +// client sending the filter the API actually has was answered 400 while an +// invented one worked — and TestTheServedFiltersFilter drove the invented one, +// which is the emulator proving itself against itself that contract_test.go's +// preamble names. Found by TestEveryDeclaredFilterKindIsTheOneTheContractDeclares +// on the day it was written (#566); nothing outside this pack's own tests used +// the old spelling. +var vmFilters = stringFilters( + "VmIds", "VmStateNames", "ImageIds", "VmTypes", "KeypairNames", "SubnetIds", "NetIds", "PrivateIps", // The zone filter FiltersVm declares (osc-sdk-go, client.gen.go:5304), // served since the subregion became a stored fact rather than a constant @@ -62,7 +72,7 @@ var vmFilters = []string{ // fails on the group, after the apply succeeded — so the whole fixture is // left standing by a filter nobody had declared. "SecurityGroupIds", "SecurityGroupNames", -} +) // vmPlacement is CreateVmsRequest's Placement (osc-sdk-go, // pkg/osc/client.gen.go:6804): the subregion the machine goes to, and its @@ -156,7 +166,7 @@ func (p *Pack) readVms(w http.ResponseWriter, r *http.Request) { return } - if p.refuseUnsupported(w, req.Filters, vmFilters...) { + if p.refuseFilters(w, req.Filters, vmFilters) { return } @@ -210,7 +220,7 @@ func (p *Pack) vmMatches(res *resource.Resource, f filterSet) bool { } return matchesStrings(f, "VmIds", res.ID) && - matchesStrings(f, "VmStates", res.State) && + matchesStrings(f, "VmStateNames", res.State) && matchesStrings(f, "ImageIds", attr("ImageId")) && matchesStrings(f, "VmTypes", attr("VmType")) && matchesStrings(f, "KeypairNames", attr("KeypairName")) && diff --git a/internal/providers/outscale/volumes.go b/internal/providers/outscale/volumes.go index 675ae4de..527afe8d 100644 --- a/internal/providers/outscale/volumes.go +++ b/internal/providers/outscale/volumes.go @@ -3,7 +3,6 @@ package outscale import ( "errors" "net/http" - "strconv" "time" "github.com/stephrobert/feint/internal/core/emulator" @@ -112,12 +111,17 @@ type readVolumesRequest struct { // filters included: the Terraform provider polls ReadVolumes filtered on // LinkVolumeVmIds to wait for an attach and again for a detach, so refusing // them fails `outscale_volume_link` on the apply and again on the destroy. -var volumeFilters = []string{ - "VolumeIds", "VolumeStates", "VolumeTypes", "SubregionNames", - "SnapshotIds", "VolumeSizes", "ClientTokens", - "LinkVolumeVmIds", "LinkVolumeDeviceNames", "LinkVolumeLinkStates", - "LinkVolumeDeleteOnVmDeletion", -} +var volumeFilters = joinFilters( + stringFilters( + "VolumeIds", "VolumeStates", "VolumeTypes", "SubregionNames", + "SnapshotIds", "ClientTokens", + "LinkVolumeVmIds", "LinkVolumeDeviceNames", "LinkVolumeLinkStates", + ), + // FiltersVolume declares VolumeSizes as a list of integers, which is why + // reading it as strings reported every value as "filter absent" (#566). + intFilters("VolumeSizes"), + boolFilters("LinkVolumeDeleteOnVmDeletion"), +) func (p *Pack) readVolumes(w http.ResponseWriter, r *http.Request) { var req readVolumesRequest @@ -128,7 +132,7 @@ func (p *Pack) readVolumes(w http.ResponseWriter, r *http.Request) { if p.refusePageSize(w, req.ResultsPerPage) { return } - if p.refuseUnsupported(w, req.Filters, volumeFilters...) { + if p.refuseFilters(w, req.Filters, volumeFilters) { return } @@ -338,25 +342,20 @@ func volumeMatches(res *resource.Resource, f filterSet) bool { if linkedVM != "" { linkState = "attached" } - // Through the shared reader for the same reason as the shrink refusal - // above, and with one honest difference: no test can redden this line - // today, because nothing reaches it. #542 said a restored volume "publishes - // no size at all" and the measurement disproved that twice over — - // volumeView copies Attrs verbatim, so the read publishes 40 either way, - // and the VolumeSizes filter is declared by the API as an array of - // integers, which filterSet.strings cannot decode: it reports the decode - // failure as "filter absent" and matchesAny then passes everything. So the - // comparison below has never discriminated, before or after a restore, for - // any client. That is a defect of its own and a wider one — Progresses is - // the second numeric filter this pack claims — and it is deliberately not - // fixed here: changing which volumes a filter answers is client-visible - // surface and belongs to a measurement of its own. This line is corrected - // so it is right on the day that one lands, and this comment is here so - // nobody reads its silence as proof. - size := "" - if _, present := res.Attrs["Size"]; present { - size = strconv.Itoa(resource.Int(res, "Size")) - } + // That day landed: #566. The comment that stood here said this comparison + // "has never discriminated, before or after a restore, for any client", + // because FiltersVolume declares VolumeSizes as an array of integers and + // the size was compared as a string — filterSet.strings reported the decode + // failure as "filter absent", and matchesAny then passed every volume. + // Reproduced on 2026-08-28 before the fix: VolumeSizes [40] against a 40 GiB + // and a 10 GiB volume answered 200 with both. + // + // Read through the shared reader for the same reason as the shrink refusal + // above. A volume whose Attrs hold no Size answers no number at all, so it + // cannot match a VolumeSizes filter — the honest reading of "the emulator + // does not know this volume's size", and the one case a restore can produce. + // TestAVolumeSizeFilterExcludesAVolumeOfAnotherSize fails without this. + sizes := numbersOf(res.Attrs, "Size") // Read from the volume, not written here as false. It became a per-volume // fact when a Vm's root device arrived (#378): a machine's root volume dies // with the machine, a volume the client linked does not, and a filter that @@ -368,7 +367,7 @@ func volumeMatches(res *resource.Resource, f filterSet) bool { matchesStrings(f, "SubregionNames", stringOf(res.Attrs["SubregionName"])) && matchesStrings(f, "SnapshotIds", stringOf(res.Attrs["SnapshotId"])) && matchesStrings(f, "ClientTokens", stringOf(res.Attrs["ClientToken"])) && - matchesStrings(f, "VolumeSizes", size) && + matchesInts(f, "VolumeSizes", sizes...) && matchesStrings(f, "LinkVolumeVmIds", linkedVM) && matchesStrings(f, "LinkVolumeDeviceNames", device) && matchesStrings(f, "LinkVolumeLinkStates", linkState) && @@ -414,6 +413,8 @@ func (p *Pack) volumeView(res *resource.Resource) map[string]any { // MaintenanceEvents is always empty and that is the honest answer: this emulator // schedules no maintenance, and inventing an event would put a date in a client's // plan that nothing will ever act on. +var vmStateFilters = stringFilters("VmIds", "VmStates", "SubregionNames") + func (p *Pack) readVmsState(w http.ResponseWriter, r *http.Request) { var req struct { Filters filterSet `json:"Filters"` @@ -428,7 +429,7 @@ func (p *Pack) readVmsState(w http.ResponseWriter, r *http.Request) { if p.refusePageSize(w, req.ResultsPerPage) { return } - if p.refuseUnsupported(w, req.Filters, "VmIds", "VmStates", "SubregionNames") { + if p.refuseFilters(w, req.Filters, vmStateFilters) { return } diff --git a/internal/proxy/redact.go b/internal/proxy/redact.go index 4a128cce..a583664f 100644 --- a/internal/proxy/redact.go +++ b/internal/proxy/redact.go @@ -178,6 +178,29 @@ func redactValue(v any) any { // a name-pattern rule catches names as well as secrets, and two // names written as one string make a transcript claim two // objects were the same. See [placeholderFor] and #384. + // + // A list of scalars keeps its brackets, for the null case's + // reason one type over: flattening it to a string changes the + // recorded type, and a replay then reissues a shape the client + // never sent. Measured on 2026-08-28 (#566): the corpus holds + // `ReadKeypairs {"Filters":{"KeypairNames":"REDACTED-17"}}`, + // where oapi-cli sent an array — KeypairNames matches "key", + // which redact.go's own comment names as the price paid + // knowingly. Nothing had ever been able to see it, because the + // Outscale pack read an undecodable filter as an absent one and + // answered 200 with the whole inventory. The type gate that + // closed #566 turned that silence into a 400, which is how this + // surfaced at all. + // + // Scalars only: an array of objects still goes wholesale, + // because descending into it would publish every leaf the + // denylist does not name, which is the opposite of what this + // function is for. + // TestARedactedListOfScalarsStaysAList fails without this. + if items, ok := scalarList(nested); ok { + value[k] = items + continue + } value[k] = placeholderFor(textOf(nested)) continue } @@ -194,6 +217,42 @@ func redactValue(v any) any { } } +// scalarList replaces every element of a list of scalars with its own +// placeholder, and reports whether the value was such a list. +// +// The brackets are the point. A recording is reissued, and a shape the client +// never sent is a measurement of nothing — the argument [setHeaders] already +// makes for the headers it refuses to copy. One placeholder per element rather +// than one for the list, for [placeholderFor]'s reason: two originals must stay +// two. +// +// A publishable element keeps its value, because the same allowlist that buys +// back a public key at the top level buys it back inside a list. +func scalarList(v any) ([]any, bool) { + items, isList := v.([]any) + if !isList { + return nil, false + } + out := make([]any, 0, len(items)) + for _, item := range items { + switch item.(type) { + case map[string]any, []any: + return nil, false + case nil: + // Kept, for the same reason a null field is: it holds nothing to + // reveal, and writing over it invents a value. + out = append(out, item) + default: + if publishable(item) { + out = append(out, item) + continue + } + out = append(out, placeholderFor(textOf(item))) + } + } + return out, true +} + // publishable reports whether a value is one whose own format proves it is // meant to be published, so that the name-pattern rule above must not eat it. // diff --git a/internal/proxy/redact_internal_test.go b/internal/proxy/redact_internal_test.go index 3028b58b..5ff566e6 100644 --- a/internal/proxy/redact_internal_test.go +++ b/internal/proxy/redact_internal_test.go @@ -294,6 +294,81 @@ func TestARedactedNullStaysNull(t *testing.T) { } } +// A list of scalars under a credential-bearing name keeps its brackets. +// +// The null case above, one type over, and measured the same way. The committed +// corpus holds +// `ReadKeypairs {"Filters":{"KeypairNames":"REDACTED-17"}}`: oapi-cli sent an +// array, `KeypairNames` matches "key", and the whole array was written down as +// one string. A replay then reissues a shape no client ever sent — the argument +// [setHeaders] already makes for the headers it refuses to copy. +// +// Nothing could see it until 2026-08-28 (#566), because the Outscale pack read +// an undecodable filter as an absent one and answered 200 with the whole +// inventory. Two silent defects cancelling out is why this needs a test rather +// than a comment: neither instrument could report the other. +// +// Both directions are asserted. A list of scalars keeps its length and its type +// with one placeholder per element, so two distinct originals stay two; a list +// of objects still goes wholesale, which is +// TestASensitiveContainerIsStillReplacedWholesale's rule and must not have +// widened. +func TestARedactedListOfScalarsStaysAList(t *testing.T) { + body := map[string]any{ + "KeypairNames": []any{"one-name", "another-name"}, + "api_keys": []any{"secret-a", "secret-a", "secret-b"}, + "ssh_keys": []any{map[string]any{"public_key": publicKeyLine(t)}}, + "VmIds": []any{"i-1", "i-2"}, + } + out, ok := redactValue(body).(map[string]any) + if !ok { + t.Fatalf("redactValue did not answer an object") + } + + names, isList := out["KeypairNames"].([]any) + if !isList { + t.Fatalf("KeypairNames came back %#v, want a list: flattening it changes the "+ + "recorded type, and a replay then reissues a shape the client never sent", + out["KeypairNames"]) + } + if len(names) != 2 { + t.Fatalf("KeypairNames came back with %d element(s), want 2", len(names)) + } + for i, item := range names { + if !IsPlaceholder(item) { + t.Errorf("KeypairNames[%d] came back %#v, want a placeholder: the brackets are "+ + "kept, the values are not", i, item) + } + } + if names[0] == names[1] { + t.Errorf("two distinct names came back as one placeholder (%v): a transcript would "+ + "then claim the client asked for the same keypair twice", names[0]) + } + + keys, isList := out["api_keys"].([]any) + if !isList || len(keys) != 3 { + t.Fatalf("api_keys came back %#v, want a list of three", out["api_keys"]) + } + if keys[0] != keys[1] { + t.Errorf("two equal secrets came back as two placeholders (%v, %v): the placeholder "+ + "stands for a value, so equal values share one", keys[0], keys[1]) + } + if keys[1] == keys[2] { + t.Errorf("two different secrets came back as one placeholder (%v)", keys[1]) + } + + // The rule that must not have widened: a list of objects is still one + // string, because descending into it would publish every leaf the denylist + // does not name. + if !IsPlaceholder(out["ssh_keys"]) { + t.Errorf("a list of objects came back %#v, want %q", out["ssh_keys"], Placeholder) + } + // And an ordinary list is untouched, or the whole corpus becomes unreadable. + if ids, _ := out["VmIds"].([]any); len(ids) != 2 || ids[0] != "i-1" || ids[1] != "i-2" { + t.Errorf("an ordinary list was redacted: %#v", out["VmIds"]) + } +} + // An OpenSSH public key under a name the denylist matches is written down. // // `public_key` matches "key", and the substitution that follows is not a diff --git a/tools/conformance/outscale/octl.sh b/tools/conformance/outscale/octl.sh index 3ad98d10..60315d25 100755 --- a/tools/conformance/outscale/octl.sh +++ b/tools/conformance/outscale/octl.sh @@ -519,6 +519,45 @@ restored_id="$(printf '%s' "$restored" | jq -r '.Volume.VolumeId')" listed="$(osc ReadVolumes --Filters.VolumeIds "$restored_id")" || fail "ReadVolumes rejected: $listed" printf '%s' "$listed" | jq -e --arg s "$snap_id" '.Volumes[0].SnapshotId == $s' >/dev/null \ || fail "the listed restored volume does not carry its provenance: $listed" + +# THE NUMERIC FILTERS, DRIVEN BY THE CLIENT THAT SENDS THEM AS NUMBERS (#566). +# +# FiltersVolume declares VolumeSizes as a list of integers and FiltersSnapshot +# declares Progresses the same way; this pack read both as lists of strings, so +# the decode failed, the failure was reported as "filter absent", and every +# candidate came back with a 200. That comparison had therefore never +# discriminated for any client, and no unit test and no leg of this suite could +# see it, because nothing here had ever asserted that a filter EXCLUDED +# something. +# +# A volume of a size no other volume here carries, so the assertion names one +# and refuses the rest. octl builds the body from the API description, which is +# what makes this the measurement rather than the curl beside it: it sends 3, +# not "3". +odd="$(osc CreateVolume --SubregionName eu-west-2a --Size 3)" || fail "CreateVolume rejected: $odd" +odd_id="$(printf '%s' "$odd" | jq -r '.Volume.VolumeId // empty')" +sized="$(osc ReadVolumes --Filters.VolumeSizes 3)" || fail "ReadVolumes rejected a VolumeSizes filter: $sized" +printf '%s' "$sized" | jq -e --arg v "$odd_id" \ + '([.Volumes[].VolumeId] | length == 1) and .Volumes[0].VolumeId == $v' >/dev/null \ + || fail "VolumeSizes 3 did not exclude the volumes of another size: $sized" +# And the accepting half, or a filter that refuses everything would pass the +# line above. +kept="$(osc ReadVolumes --Filters.VolumeSizes 7)" || fail "ReadVolumes rejected: $kept" +printf '%s' "$kept" | jq -e --arg v "$odd_id" \ + '(.Volumes | length > 0) and (any(.Volumes[]; .VolumeId == $v) | not)' >/dev/null \ + || fail "VolumeSizes 7 answered nothing, or kept the 3 GiB volume: $kept" +empty="$(osc ReadVolumes --Filters.VolumeSizes 4096)" || fail "ReadVolumes rejected: $empty" +printf '%s' "$empty" | jq -e '.Volumes | length == 0' >/dev/null \ + || fail "a size no volume carries matched something: $empty" +osc DeleteVolume --VolumeId "$odd_id" >/dev/null || fail "DeleteVolume rejected" +progressed="$(osc ReadSnapshots --Filters.Progresses 100)" || fail "ReadSnapshots rejected a Progresses filter: $progressed" +printf '%s' "$progressed" | jq -e --arg s "$snap_id" 'any(.Snapshots[]; .SnapshotId == $s)' >/dev/null \ + || fail "Progresses 100 lost the snapshot that is at 100: $progressed" +unfinished="$(osc ReadSnapshots --Filters.Progresses 7)" || fail "ReadSnapshots rejected: $unfinished" +printf '%s' "$unfinished" | jq -e '.Snapshots | length == 0' >/dev/null \ + || fail "a progress no snapshot carries matched something: $unfinished" +ok "the numeric filters exclude, driven by a client that sends numbers" + osc DeleteSnapshot --SnapshotId "$snap_id" >/dev/null || fail "DeleteSnapshot rejected" osc DeleteVolume --VolumeId "$restored_id" >/dev/null || fail "DeleteVolume rejected" ok "record, restore, and the key only when it means something" From 5fd84b549823d10ba980ef92daa5d4ed84920ced Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20ROBERT?= Date: Fri, 28 Aug 2026 12:33:44 +0200 Subject: [PATCH 2/6] fix(machine): a refusal this emulator declares is a warning, and 47 of 48 error sites were already right (#474) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fourteen level=ERROR lines over fifteen stack replays were all one call: the deliberate, documented refusal to boot an image identifier no catalogue holds. The run that printed five of them applied 54 resources of 54, matched its reference and destroyed 54 cleanly, so an operator grepping ERROR found fourteen lines about a documented behaviour and nothing about the run that failed. In one of those logs the sibling refusal 200 ms later was already a WARN — loadbalancer_dataplane.go's ErrBalancerNotDistributed (#457), whose comment states the rule: a limit is not an incident. The rule, now written where the refusal is: an ERROR is something this emulator did not do that it was built to do; a WARN is something it deliberately declines and documents, where the API answer stays honest. The issue's own "what I did not measure" is measured: of the 48 ERROR sites under internal/ on 2026-08-28, this was the only one on the wrong side. Its neighbours are failures and stay ERROR — a start the driver refused, an image build that could not fetch its source, a pack that declares no interface plan (plan.go says why that one is a fault in the pack rather than a decline). Only the level moved. The boot is still refused, the resource still reads FailedState, and the four keys still say what to do about it — all three asserted. TestADocumentedRefusalIsAWarningAndAFailureStaysAnError fails without this, and fails in both directions: a change that made every refusal a warning passes its first subtest and fails the other two. Reproduced before the fix — the image subtest red on `level=ERROR`, the two failure subtests green — then green after. docs/limits.md's section is rewritten from a shipped limit into the fix and the measurement, because a limit that no longer holds is worse than an undocumented one (#558). Assisted-by: Claude Code (claude-opus-5) --- docs/limits.md | 40 +++++++-- internal/core/machine/binding.go | 33 ++++++- internal/core/machine/binding_boot_test.go | 100 +++++++++++++++++++++ 3 files changed, 162 insertions(+), 11 deletions(-) diff --git a/docs/limits.md b/docs/limits.md index 8f6a51f8..b8f2994d 100644 --- a/docs/limits.md +++ b/docs/limits.md @@ -3570,7 +3570,32 @@ replayed green for this record on 2026-08-27 — and `tools/falsify/specs/lifecycle-tells-the-truth.json` replays them with the guard neutralised in both directions, the refusal and the acceptance. -## Fourteen ERROR lines over fifteen stack replays are one documented refusal, logged at the wrong level (#474) +## An ERROR is a failure and a WARN is a decline, and one refusal was on the wrong side (#474) + +**Fixed on 2026-08-28.** The refusal below is logged at WARN, and the rule the +measurement suggested is now written where the refusal is +(`Binding.refuseUnknownImage`) and held by +`TestADocumentedRefusalIsAWarningAndAFailureStaysAnError`: + +> An **ERROR** is something this emulator did not do that it was built to do. +> A **WARN** is something it deliberately declines and documents, where the API +> answer stays honest. + +The half the issue had not measured is measured now: of the **48 ERROR sites +under `internal/` on 2026-08-28**, that one call was the only one on the wrong +side of the line. Its own neighbours are failures and stay ERROR — a start the +driver refused, an image build that could not fetch its source, a pack that +declares no interface plan (`plan.go` says why that one is not a decline). The +test asserts both directions for exactly that reason: a change that made every +refusal a warning would pass its first half and quiet the lines the log exists +for. Nothing else moved — the boot is still refused, the machine still reads +back its `FailedState`, and the refusal still names the identifier, the reason, +the consequence and both gestures. + +What follows is the measurement that established it, kept because the reasoning +is the reusable part. + +### The measurement, 2026-08-25 Replaying the fifteen surveyed stacks under a machine runtime (`main@72d861d`, `--vm incus-ovn`, logs of 2026-08-25), five runs printed `level=ERROR` — @@ -3592,14 +3617,11 @@ it is the one that follows the rule. The run that printed five of these ERRORs was a **success**: ztiac applied 54 of 54, matched its reference exactly, and destroyed 54 cleanly. -What to do with it: do not grade an emulator log by `grep ERROR` alone — under -these replays it finds fourteen lines about a documented behaviour and nothing -about the run that really failed. What would lift it: logging this refusal at -WARN, where its balancer sibling already is; the distinction, if a rule is -wanted, is that an ERROR is something the emulator did not do that it was -built to do, and a WARN is something it deliberately declines and documents -while the API answer stays honest. Not measured: whether any other ERROR site -is in the same position — the fifteen replays surfaced only this one. +The lesson that outlives the fix: do not grade an emulator log by `grep ERROR` +alone, and do not level a limit like an incident. Under those replays the grep +found fourteen lines about a documented behaviour and nothing about the run +that really failed, which is precisely how a team learns to skip a log's +errors. ## `feint images resolve` can print a `FEINT_BOOT_IMAGES` line that cannot boot (#476) diff --git a/internal/core/machine/binding.go b/internal/core/machine/binding.go index 8a48d02e..8a1a1966 100644 --- a/internal/core/machine/binding.go +++ b/internal/core/machine/binding.go @@ -360,7 +360,36 @@ func (b Binding) Start(ctx context.Context, boot Boot) Started { // worse, a machine that boots and then fails at its first package install — // the reason #392's generic substitution was refused. // -// TestTheBootRefusalNamesTheGesturesThatUnblock fails without the gestures. +// WARN, not ERROR, and that is the rule rather than a taste (#474). +// +// An ERROR is something this emulator did not do that it was built to do; a +// WARN is something it deliberately declines and documents, where the API +// answer stays honest. This is the second: docs/limits.md says this emulator +// keeps records and not disk contents, the machine reads back its FailedState, +// and no client is lied to. +// +// The measurement that settled it: replaying fifteen surveyed stacks under +// `--vm incus-ovn`, five runs printed fourteen level=ERROR lines and every one +// was this call. The run that printed five of them applied 54 resources of 54, +// matched its reference and destroyed 54 cleanly — so an operator grepping +// ERROR found fourteen lines about a documented behaviour and nothing about +// the run that failed. In one of those logs the sibling refusal 200 ms later +// was already a WARN: loadbalancer_dataplane.go's ErrBalancerNotDistributed +// (#457), whose comment states the rule — a limit is not an incident. +// +// Of the 48 ERROR sites under internal/ on 2026-08-28, this was the only one on +// the wrong side of that line; the rest are failures, its own neighbours +// included — a start the driver refused, a build that could not fetch its +// source, a pack that declares no interface plan. +// +// The level is the only thing that moved: the boot is still refused, the +// resource still reads FailedState, and the four keys below still say what to +// do about it. +// +// TestTheBootRefusalNamesTheGesturesThatUnblock fails without the gestures, and +// TestADocumentedRefusalIsAWarningAndAFailureStaysAnError fails without the +// level — in both directions, so a change that made every refusal a warning +// fails it too. func (b Binding) refuseUnknownImage(boot Boot) { reason := boot.Reason if reason == "" { @@ -370,7 +399,7 @@ func (b Binding) refuseUnknownImage(boot Boot) { if id == "" { id = "" } - b.logger().Error("refusing to boot: nothing says which operating system this image identifier names", + b.logger().Warn("refusing to boot: nothing says which operating system this image identifier names", "provider", b.Provider, "resource", boot.ID, "image", id, "reason", reason, "consequence", "the machine stays "+b.FailedState+"; guessing an OS would boot a machine that fails at its first package install", "ask", "`feint images resolve "+id+"` looks it up in the providers' public listings, no account needed", diff --git a/internal/core/machine/binding_boot_test.go b/internal/core/machine/binding_boot_test.go index 285fa9b2..7a45126d 100644 --- a/internal/core/machine/binding_boot_test.go +++ b/internal/core/machine/binding_boot_test.go @@ -155,6 +155,18 @@ func (d *buildingDriver) BuildImage(_ context.Context, spec ImageSpec, _ io.Writ return nil } +// failingDriver is a recordingDriver whose Start refuses, so the difference +// between "this emulator declined" and "the runtime failed" can be asserted +// without one. +type failingDriver struct { + recordingDriver + err error +} + +func (d *failingDriver) Start(context.Context, Spec) (Machine, error) { + return Machine{}, d.err +} + func TestABootDerivesAndBuildsTheImageItNames(t *testing.T) { t.Run("a version the station lacks is built on the boot path", func(t *testing.T) { driver := &buildingDriver{} @@ -291,6 +303,94 @@ func TestTheBootRefusalNamesTheGesturesThatUnblock(t *testing.T) { } } +// A refusal this emulator declares is a WARN; only what it failed to do is an +// ERROR (#474). +// +// The measurement that scoped it: replaying fifteen surveyed stacks under +// `--vm incus-ovn`, five runs printed level=ERROR, fourteen lines in all, and +// every one was this refusal. The run that printed five of them was a success +// — ztiac applied 54 of 54, matched its reference and destroyed 54 cleanly. An +// operator grepping ERROR to find what went wrong found fourteen lines about a +// documented behaviour and nothing about the run that really failed, which is +// how a log teaches people to skip its errors. +// +// The sibling refusal 200 ms later, in the same log, was already a WARN: +// loadbalancer_dataplane.go's ErrBalancerNotDistributed (#457), whose comment +// states the rule — "a limit is not an incident". This is that rule applied to +// the other refusal in the same layer. +// +// The line separating the two, measured over the 48 ERROR sites in internal/ on +// 2026-08-28: an ERROR is something this emulator did not do that it was built +// to do; a WARN is something it deliberately declines and documents, where the +// API answer stays honest. Exactly one site was on the wrong side — this one. +// Its neighbours stay ERROR and are asserted here, because a change that made +// every refusal a warning would pass the first half of this test and hide the +// failures the log exists for: +// +// - a start the driver refused (the runtime failed at something it accepted); +// - a pack that declares no interface plan (a fault in the pack itself, not +// a decline, and plan.go says why). +// +// The API answer is unchanged and asserted too: the machine still does not +// boot, and the resource still reads FailedState. Lowering the level must not +// quiet the refusal, only stop it claiming to be an incident. +func TestADocumentedRefusalIsAWarningAndAFailureStaysAnError(t *testing.T) { + t.Run("the image refusal warns", func(t *testing.T) { + var log bytes.Buffer + b := bootBinding(&recordingDriver{}) + b.Log = slog.New(slog.NewTextHandler(&log, nil)) + res := &resource.Resource{ID: "srv-1", State: "stopped"} + + if b.PowerOn(context.Background(), res, Boot{Requested: "ami-538af795"}) { + t.Fatal("an undeclared identifier booted") + } + if res.State != "failed" { + t.Errorf("the resource reads %q, want failed: the level moved, the answer must not", res.State) + } + if strings.Contains(log.String(), "level=ERROR") { + t.Errorf("the documented refusal is logged at ERROR:\n%s", log.String()) + } + if !strings.Contains(log.String(), "level=WARN") { + t.Errorf("the refusal is not logged at WARN, so it is quieter than a limit should be:\n%s", log.String()) + } + // Still actionable at the lower level: the level is the only thing + // that changed. + for _, needle := range []string{"ami-538af795", "feint images resolve", "FEINT_BOOT_IMAGES"} { + if !strings.Contains(log.String(), needle) { + t.Errorf("the refusal lost %q on the way down:\n%s", needle, log.String()) + } + } + }) + + t.Run("a start the runtime refused stays an error", func(t *testing.T) { + var log bytes.Buffer + b := bootBinding(&failingDriver{err: errors.New("the runtime said no")}) + b.Log = slog.New(slog.NewTextHandler(&log, nil)) + res := &resource.Resource{ID: "srv-2", State: "stopped"} + + if b.PowerOn(context.Background(), res, Boot{Image: "ubuntu:22.04", Requested: "ubuntu_jammy"}) { + t.Fatal("a start the driver refused reported success") + } + if !strings.Contains(log.String(), "level=ERROR") { + t.Errorf("a runtime failure is not an ERROR any more, so this change made the log quieter "+ + "about the thing it exists for:\n%s", log.String()) + } + }) + + t.Run("a pack with no interface plan stays an error", func(t *testing.T) { + var log bytes.Buffer + b := bootBinding(&recordingDriver{}) + b.Log = slog.New(slog.NewTextHandler(&log, nil)) + r := Reconciler{Groups: GroupSync{Binding: b}} + if _, ok := r.plan(&resource.Resource{ID: "srv-3"}); ok { + t.Fatal("a nil PlanOf answered a plan") + } + if !strings.Contains(log.String(), "level=ERROR") { + t.Errorf("a pack that declares no plan is a fault in the pack, not a decline:\n%s", log.String()) + } + }) +} + // A client cloud-config that declares a package step cannot complete on a // machine booting with no emulated network under it (#507): no NAT, no // resolver, no route to a package repository (#202). The guest's own journal From 31445fe64675c0798f41a5285346d34e7621352e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20ROBERT?= Date: Fri, 28 Aug 2026 12:43:42 +0200 Subject: [PATCH 3/6] feat(emulator): a refusal every pack shares carries a marker, and the status that would have carried it fails a real client (#477) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An operation feint declines answered a bare 404 in the Exoscale pack, which is also what the cloud answers for an elastic IP with no reverse record. Replaying the register's best third-party stack — seven applied, empty second plan, seven destroyed — the recorder showed three refusals of GET /v2/reverse-dns/elastic-ip/{id} and nothing anywhere said so. The issue offered three remedies. The second was measured and is refused. Answering 501, as the Scaleway pack already does for its own space, is legible and costs exo 1.95.1 / egoscale v3.1.36 nothing in latency: `exo dns list` at 22, 21 and 19 ms against the 19 ms of a served route, one attempt, no backoff, reading "Not Implemented: feint does not serve …" instead of "Not Found: …". And it FAILS `exo compute instance create`, which calls GET /v2/reverse-dns/instance/{id} after every create and treats anything but a 404 as fatal: the exo-cli leg died at "instance create rejected" under 501 and passes under 404 (measured 2026-08-28, both directions). That is the symmetric defect the polar star forbids — a refusal loud enough to fail a client the real cloud would have served — and it generalises: for an operation whose real 404 means "this object has no such record", no status can carry the distinction. Neither can the body here: egoscale.APIError requires `message`, declares no code field, and the one refusal recorded from the real cloud carries exactly that, so a field would be an invented format (rule 4). So the marker goes out of band, and in the shared layer rather than in each pack — a control copied into three packs is one the fourth forgets, and three spellings of a refusal is what this issue is about: X-Feint-Not-Emulated: exoscale set by emulator.handleUnrouted on every unrouted refusal, beside X-Feint-Fault and X-Feint-Probe, which faults.go already documents as headers no real cloud sends. Its value is the pack that owns the URL space, read from this process's mount table — never the path the client sent, the rule the neighbouring warning already follows. It is not set on a served answer, nor on a 404 that is an ordinary missing object, nor on a path no pack claims: a marker on every answer marks nothing, and all three are asserted. Measured against all three packs, so the three dialects now share one marker while keeping the statuses their own clients need — Scaleway 501, Outscale 404 (oapi-cli backs off 12 s on a 501), Exoscale 404. What this does NOT do, and #477's remaining half: a header is invisible to Terraform and to a human reading an apply, so a run can still be green while three declined operations were called. Reporting that to an operator needs a surface `feint status` does not have — and resolving a path to a declined operation name needs contracts, which are off by default. docs/architecture.md says so where a reader meets the table. Gates: emulator.TestAnUnroutedAnswerCarriesTheNotEmulatedHeader, TestADeclinedOperationKeepsTheStatusItsClientNeeds, `mise run conformance:leg -- exo-cli` green, corpus and docs unchanged. Assisted-by: Claude Code (claude-opus-5) --- docs/architecture.md | 45 ++++++++++++ internal/core/emulator/emulator.go | 49 ++++++++++++- internal/core/emulator/unrouted_test.go | 70 ++++++++++++++++++ internal/providers/exoscale/lifecycle_test.go | 11 ++- internal/providers/exoscale/pack.go | 48 +++++++++++++ internal/providers/exoscale/pack_test.go | 72 +++++++++++++++++++ 6 files changed, 292 insertions(+), 3 deletions(-) diff --git a/docs/architecture.md b/docs/architecture.md index e7d8268a..3e5cbfcc 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -152,6 +152,51 @@ without the second, "not done yet" and "out of scope" become the same thing. A change that makes any of the three inoperative is a bad change, even if it simplifies the code. +### A decline has to be legible on the wire, not only in `coverage/` + +`Declined()` and the coverage artefacts are honest about what a pack does not +serve, and they are read by nobody at run time. A **client** meets the refusal +instead, and it must be able to tell "feint does not serve this operation" from +"the cloud has nothing for you" — otherwise a decline reads as an empty answer +and a green run proves something it did not (#477: three refusals of +`GET /v2/reverse-dns/elastic-ip/{id}` inside a stack that applied seven +resources, planned empty and destroyed seven, with nothing anywhere saying so). + +Each pack answers in its own dialect, and each spelling was decided by what its +clients do rather than by a house style: + +| pack | status | body | why that one | +|---|---|---|---| +| Scaleway | `501` | `{"type":"not_emulated", "message": …}` | a type their SDK does not map, so `errors.As(&ResourceNotFoundError{})` cannot agree that a resource is missing; their SDK has no retry policy, so the status costs nothing | +| Outscale | `404` | `{"Errors":[{"Code":"", "Type":"OperationNotEmulated", …}]}` | their envelope has a `Type` to put the marker in, and `501` costs `oapi-cli` 12 seconds of backed-off retries — measured | +| Exoscale | `404` | `{"message": …}` | their envelope has nowhere to put a marker, and `501` **fails `exo compute instance create`** — measured | + +**No status and no body carries the marker for every operation, and that is a +measurement rather than a preference.** Exoscale is the case that proves it: +`exo compute instance create` calls `GET /v2/reverse-dns/instance/{id}` after +every create and treats anything but a `404` as fatal, so the status a program +could branch on is the status that breaks the client. Their error envelope +requires `message` and declares no code field, so a marker in the body would be +an invented format (rule 4). Both doors are shut, and `404` on a declined +operation is then the same bytes as "this elastic IP has no reverse record". + +So the marker is **out of band**, set once in the shared layer for every pack: + +```text +X-Feint-Not-Emulated: exoscale +``` + +`emulator.handleUnrouted` sets it on every unrouted refusal, with the name of +the pack that owns the URL space — read from this process's mount table, never +from the path the client sent. It sits beside `X-Feint-Fault` and +`X-Feint-Probe`, headers no real cloud sends, and no client can trip over it. + +Two things it deliberately does not do. It does not mark a `404` that is an +ordinary missing object — only a refusal — or the marker would mean nothing. +And it does not tell an **operator**: a header is invisible to Terraform and to +a human reading an apply, so a run can still be green while three declined +operations were called (#477's remaining half). + Those three are the first links of a longer chain — the contract, the two witnesses that drive it, the recordings that look in the omission direction, the seven-axis evidence record, and the versioned surface a pipeline reads. diff --git a/internal/core/emulator/emulator.go b/internal/core/emulator/emulator.go index 5eff44dd..1205b1f6 100644 --- a/internal/core/emulator/emulator.go +++ b/internal/core/emulator/emulator.go @@ -519,6 +519,16 @@ func checkSpaces(packs []Pack) error { return nil } +// NotEmulatedHeader marks an answer that is this emulator refusing an operation +// rather than a cloud answering, and names the pack whose URL space the request +// landed in. No real cloud sends it — the same trade FaultHeader documents. +// +// It exists because a status cannot carry that fact for every operation: for +// one whose real 404 means "this object has no such record", a louder refusal +// fails a client the real cloud would have served. See handleUnrouted for the +// measurement (#477). +const NotEmulatedHeader = "X-Feint-Not-Emulated" + // handleUnrouted answers a request no route claimed, in the dialect of whichever // pack owns the URL space it landed in. // @@ -527,6 +537,7 @@ func checkSpaces(packs []Pack) error { // is right for a request that belongs to no provider at all. func (s *Server) handleUnrouted(w http.ResponseWriter, r *http.Request) { best, bestLen := (Unrouted)(nil), -1 + bestName := "" for _, p := range s.packs { unrouted, ok := p.(Unrouted) if !ok { @@ -534,10 +545,46 @@ func (s *Server) handleUnrouted(w http.ResponseWriter, r *http.Request) { } for _, prefix := range unrouted.Prefixes() { if len(prefix) > bestLen && strings.HasPrefix(r.URL.Path, prefix) { - best, bestLen = unrouted, len(prefix) + best, bestLen, bestName = unrouted, len(prefix), p.Name() } } } + // The marker, out of band, and here rather than in each pack (#477). + // + // A refusal a program cannot tell from an empty answer is a refusal that + // reads as success. Replaying the register's best Exoscale stack — seven + // resources applied, empty second plan, seven destroyed — the recorder + // showed three refusals of an operation that pack declines, and nothing + // anywhere said so: a bare 404 is also what the cloud answers for an + // elastic IP with no reverse record, so the refusal and the ordinary empty + // answer were the same bytes. + // + // It could not be fixed in the status, and that was measured rather than + // argued. Answering 501, as the Scaleway pack does for its own space, is + // legible and costs exo 1.95.1 nothing in latency (22, 21 and 19 ms against + // the 19 ms of a served route) — and it breaks `exo compute instance + // create`, which calls GET /v2/reverse-dns/instance/{id} after every create + // and treats anything but a 404 as fatal. Measured on 2026-08-28: the + // exo-cli leg failed at "instance create rejected" with 501 and passes with + // 404. That is the polar star inverted — a refusal loud enough to fail a + // client the real cloud would have served — and it is why the status of an + // operation whose real 404 means "nothing here" cannot carry the marker. + // + // So the marker goes where no client can trip over it, and where every pack + // gets it at once: a header in this emulator's own namespace, beside + // X-Feint-Fault and X-Feint-Probe, which faults.go already documents as + // headers no real cloud sends. Its value is the pack that owns the URL + // space, read out of this process's own mount table — never the path the + // client chose, for the reason the log below gives. + // + // What it does NOT do, and what the packs' own bodies still have to: tell an + // operator. A header is invisible to Terraform and to a human reading an + // apply. That half is #477's open remainder. + // + // TestAnUnroutedAnswerCarriesTheNotEmulatedHeader fails without this. + if bestName != "" { + w.Header().Set(NotEmulatedHeader, bestName) + } // Logged like any other request, and this is the line the log exists for: // a client walking a plan meets one route nobody mounted, the whole apply // dies, and no counter anywhere records which one it was. The emulator's own diff --git a/internal/core/emulator/unrouted_test.go b/internal/core/emulator/unrouted_test.go index 67b94ace..a3d556db 100644 --- a/internal/core/emulator/unrouted_test.go +++ b/internal/core/emulator/unrouted_test.go @@ -131,6 +131,76 @@ func TestEachPackAnswersItsOwnSpace(t *testing.T) { } } +// Every refusal a pack makes for its own space carries the out-of-band marker, +// and no served answer does (#477). +// +// Why the marker is a header and not a status. Replaying the register's best +// Exoscale stack — seven resources applied, empty second plan, seven destroyed, +// green end to end — the recorder showed three refusals of +// `GET /v2/reverse-dns/elastic-ip/{id}`, an operation that pack declines, and +// nothing anywhere said so: a bare 404 is also what the cloud answers for an +// elastic IP with no reverse record, so the refusal and the ordinary empty +// answer were the same bytes to a program. +// +// The obvious remedy was measured and refused. Answering 501 there, the way the +// Scaleway pack does in its own space, is legible and costs `exo` 1.95.1 +// nothing in latency — 22, 21 and 19 ms against the 19 ms of a served route — +// and it FAILS `exo compute instance create`, which calls +// GET /v2/reverse-dns/instance/{id} after every create and treats anything but +// a 404 as fatal. Measured on 2026-08-28: the exo-cli conformance leg died at +// "instance create rejected" under 501 and passes under 404. A refusal loud +// enough to fail a client the real cloud would have served is the polar star +// inverted, and it is the symmetric defect of the one #477 reports. +// +// So the marker goes where no client trips over it. Here rather than in each +// pack: a control copied into three packs is a control the fourth forgets, and +// the three spellings of a refusal are exactly what #477 is about. +// +// The value is the pack's own name, read out of this process's mount table. +// Never the path: what a client chose does not become a value this emulator +// writes, which is the same rule the "pointing without a served prefix" warning +// below the call site already follows. +func TestAnUnroutedAnswerCarriesTheNotEmulatedHeader(t *testing.T) { + env := emulator.DefaultEnv() + ts := serve(t, scaleway.New(env), outscale.New(env), unroutedPack{}) + + header := func(t *testing.T, path string) (int, string) { + t.Helper() + res, err := http.Get(ts.URL + path) //nolint:noctx // test client + if err != nil { + t.Fatalf("get %s: %v", path, err) + } + defer func() { _ = res.Body.Close() }() + return res.StatusCode, res.Header.Get(emulator.NotEmulatedHeader) + } + + // Three packs, three dialects, one marker — including the pack whose + // refusal is a bare 404 that no body field can distinguish. + for path, want := range map[string]string{ + "/instance/v1/zones/fr-par-1/nope": "scaleway", + "/api/v1/ReadNotAnOperation": "outscale", + "/stub/v1/nothing-here": "stub", + } { + status, got := header(t, path) + if got != want { + t.Errorf("%s answered %d with %s=%q, want %q: a program cannot otherwise tell this "+ + "emulator refusing an operation from a cloud answering nothing", + path, status, emulator.NotEmulatedHeader, got, want) + } + } + + // The other direction, twice, because a header set on every answer marks + // nothing. A served route does not carry it, and neither does a path no + // pack claims — nothing is refusing an operation there, and attributing it + // to a provider would be a guess. + if status, got := header(t, "/stub/v1/things"); got != "" { + t.Errorf("a served route (%d) carries %s=%q", status, emulator.NotEmulatedHeader, got) + } + if status, got := header(t, "/nothing/at/all"); got != "" { + t.Errorf("a path no pack claims (%d) carries %s=%q", status, emulator.NotEmulatedHeader, got) + } +} + // overlappingPack claims a space that swallows another pack's. type overlappingPack struct{ unroutedPack } diff --git a/internal/providers/exoscale/lifecycle_test.go b/internal/providers/exoscale/lifecycle_test.go index 25518b0c..3479a354 100644 --- a/internal/providers/exoscale/lifecycle_test.go +++ b/internal/providers/exoscale/lifecycle_test.go @@ -157,10 +157,17 @@ func TestAProtectedInstanceRefusesItsDelete(t *testing.T) { } // An action the pack does not serve on a served base path answers the pack's -// own 404 envelope, not net/http's page: the dispatcher hands the miss to the +// own envelope, not net/http's page: the dispatcher hands the miss to the // pack, and with dozens of actions still untriaged this is an answer real // clients will meet. -func TestAnUnservedActionAnswersThePacksOwn404(t *testing.T) { +// +// Still 404 and not 501, measured rather than chosen: `exo compute instance +// create` calls GET /v2/reverse-dns/instance/{id} after every create and treats +// anything but a 404 as fatal, so a louder refusal fails a client the real +// cloud would have served (#477). The marker a program can read is the +// X-Feint-Not-Emulated header the shared layer sets — +// emulator.TestAnUnroutedAnswerCarriesTheNotEmulatedHeader. +func TestAnUnservedActionAnswersThePacksOwnRefusal(t *testing.T) { h := serve(t) id := createDemo(t, h) diff --git a/internal/providers/exoscale/pack.go b/internal/providers/exoscale/pack.go index b7269ce0..06acfecb 100644 --- a/internal/providers/exoscale/pack.go +++ b/internal/providers/exoscale/pack.go @@ -842,6 +842,46 @@ func (p *Pack) Prefixes() []string { return []string{pathPrefix} } // The body is the pack's own error envelope, a bare message field, because that // is what their API returns and inventing a richer shape would be exactly the // rule-4 violation this project forbids. +// +// # 404, and why no status could carry the marker instead (#477) +// +// This answers a bare 404, which is also what the real cloud answers when an +// object is simply not there — and that is the defect #477 reports. Replaying +// the register's best third-party stack (seven resources applied, empty second +// plan, seven destroyed, green end to end) the recorder showed three refusals +// of `GET /v2/reverse-dns/elastic-ip/{id}`, an operation this pack declines and +// carries in coverage/exoscale-coverage.json, and nothing anywhere said so: to +// a program the refusal and "this elastic IP has no reverse record" are the +// same bytes, so a reader of that green run would conclude reverse DNS works. +// +// The obvious remedy was tried, measured, and refused. Answering 501 — what the +// Scaleway pack does in its own space, and what their document declares on none +// of its 374 operations, which is exactly what would make it legible — costs +// exo 1.95.1 / egoscale v3.1.36 nothing in latency: `exo dns list` at 22, 21 and +// 19 ms against the 19 ms of a served route, one attempt, no backoff, reading +// "Not Implemented: feint does not serve …". It also FAILS +// `exo compute instance create`, which calls GET /v2/reverse-dns/instance/{id} +// after every create and treats anything but a 404 as fatal. Measured on +// 2026-08-28: the exo-cli conformance leg died at "instance create rejected" +// under 501 and passes under 404. +// +// That is the symmetric defect — a refusal loud enough to fail a client the +// real cloud would have served — and it generalises: for an operation whose +// real 404 means "this object has no such record", no status can carry the +// distinction. Neither can the body: Exoscale's envelope requires `message`, +// declares no code field, and the one refusal recorded from the real cloud +// carries exactly that (corpus/exoscale/exo-refusals.jsonl), so a field added +// here would be the invented format rule 4 forbids. +// +// So the marker is out of band, and in the shared layer rather than here: +// emulator.handleUnrouted sets X-Feint-Not-Emulated on every pack's refusal, so +// a program can tell this answer from an empty one and no client can trip over +// it. What that does NOT do is tell an operator, since a header is invisible to +// Terraform and to a human reading an apply; that half is #477's remainder, and +// the body below is what carries it today. +// +// emulator.TestAnUnroutedAnswerCarriesTheNotEmulatedHeader holds the marker and +// TestADeclinedOperationKeepsTheStatusItsClientNeeds holds the status. func (p *Pack) NotFound(w http.ResponseWriter, r *http.Request) { // A call that arrived through a zone-list signpost is refused with the // zone mismatch named, not with the generic line below: the reader of the @@ -849,6 +889,14 @@ func (p *Pack) NotFound(w http.ResponseWriter, r *http.Request) { // deployment does not serve (#284). See unservedZonePathPrefix in // catalog.go; TestAnUnservedZoneSignpostNamesTheMismatch fails without // this branch. + // + // Still 404, and deliberately not 501, because the ambiguity above does not + // exist here: /v2/unserved-zone/… is a path feint itself publishes in its + // zone list, so no cloud answer can be confused with this one and a client + // that reached it is unambiguously talking to the emulator. The operation + // may well be served — in another zone — which is what the diagnosis says + // and what "not implemented" would contradict. The status difference is + // pinned by the test above rather than left to whoever edits this next. if diagnosis, ok := p.unservedZoneDiagnosis(r.URL.Path); ok { writeError(w, http.StatusNotFound, diagnosis) return diff --git a/internal/providers/exoscale/pack_test.go b/internal/providers/exoscale/pack_test.go index f22605be..42e8518f 100644 --- a/internal/providers/exoscale/pack_test.go +++ b/internal/providers/exoscale/pack_test.go @@ -65,6 +65,78 @@ func TestAnUnservedRouteAnswersDecodableJSON(t *testing.T) { } } +// A declined operation is refused, and a 404 cannot say so — which is why the +// marker is out of band (#477). +// +// The measurement that scoped it: replaying the register's best third-party +// stack — seven resources applied, empty second plan, seven destroyed, green +// end to end — the recorder showed three refusals of +// GET /v2/reverse-dns/elastic-ip/{id}, an operation this pack declines and +// carries in coverage/exoscale-coverage.json, and nothing anywhere said so. A +// bare 404 is also what the real cloud answers when an elastic IP has no +// reverse record, so to a program the two were the same bytes. +// +// The obvious remedy was tried and measured, and it is refused. Answering 501, +// as the Scaleway pack does in its own space, is legible and costs exo 1.95.1 / +// egoscale v3.1.36 nothing in latency — `exo dns list` at 22, 21 and 19 ms +// against the 19 ms of a served route, one attempt, no backoff, and the message +// reads "Not Implemented: feint does not serve …". And it FAILS +// `exo compute instance create`: the CLI calls +// GET /v2/reverse-dns/instance/{id} after every create and treats anything but +// a 404 as fatal. Measured on 2026-08-28 — the exo-cli conformance leg died at +// "instance create rejected" under 501 and passes under 404. +// +// That is the symmetric defect of the one #477 reports: a refusal loud enough +// to fail a client the real cloud would have served. For an operation whose +// real 404 means "this object has no such record", NO status and no body field +// can carry the distinction — the body cannot either, since Exoscale's envelope +// requires `message` and declares no code field, and the one refusal shape +// recorded from the real cloud carries exactly that (rule 4). +// +// So this test holds the wire shape unchanged and names what carries the marker +// instead: the X-Feint-Not-Emulated header, set once in the shared layer for +// all three packs (emulator.TestAnUnroutedAnswerCarriesTheNotEmulatedHeader). +func TestADeclinedOperationKeepsTheStatusItsClientNeeds(t *testing.T) { + h := serve(t) + + // The operation the stack replay caught, verbatim. + rec, body := call(t, h, "GET", "/v2/reverse-dns/elastic-ip/00000000-0000-4000-8000-000000000000", "") + if rec.Code != http.StatusNotFound { + t.Errorf("status %d, want 404: `exo compute instance create` reads GET "+ + "/v2/reverse-dns/instance/{id} after every create and treats anything else as fatal, "+ + "so a louder refusal here fails a client the real cloud would have served", rec.Code) + } + // The envelope stays theirs: message and nothing else, which is the whole of + // egoscale.APIError's required shape and the whole of what the recorded + // cloud refusal carries. + for key := range body { + if key != "message" { + t.Errorf("the refusal carries %q, which Exoscale's error envelope does not declare", key) + } + } + if message, _ := body["message"].(string); !strings.Contains(message, "feint does not serve") { + t.Errorf("the refusal does not say who refused: %q", message) + } + // And the marker a program reads, which is the whole point: the same bytes + // as an empty answer, plus one header no cloud sends. + if got := rec.Header().Get(emulator.NotEmulatedHeader); got != "exoscale" { + t.Errorf("%s = %q, want \"exoscale\": without it this answer is indistinguishable from "+ + "an elastic IP that simply has no reverse record", emulator.NotEmulatedHeader, got) + } + + // The other direction, or a pack that marked every answer would pass the + // line above. A resource that is genuinely absent is a 404 the emulator is + // NOT refusing, and it must not carry the marker. + rec, _ = call(t, h, "GET", "/v2/instance/00000000-0000-4000-8000-000000000000", "") + if rec.Code != http.StatusNotFound { + t.Errorf("an absent instance answered %d, want 404", rec.Code) + } + if got := rec.Header().Get(emulator.NotEmulatedHeader); got != "" { + t.Errorf("an instance that is simply absent carries %s=%q: the marker would then mean "+ + "nothing", emulator.NotEmulatedHeader, got) + } +} + // The zone list is the first call the official CLI makes and the address every // call after it uses. Two things must hold, and both were found by running the // CLI rather than by reading the specification. From ed9861825f30944eb7a2ca9bb2970dc47e5f423b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20ROBERT?= Date: Fri, 28 Aug 2026 12:47:54 +0200 Subject: [PATCH 4/6] docs(conformance): the scw acl-delete panic closes as documentation, and the one remedy that would have been a fix is disproved (#505) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The issue offered three ways out and only one would have been a real fix: that this emulator answers something else, without lying, that does not trigger the CLI's faulty assertion. It is false, and both halves of the disproof are here. Read, in the module cache rather than guessed: - ZonedAPI.DeleteACL ends on `s.client.Do(scwReq, nil, opts...)` — the response is decoded into nil, so NO body this emulator sends reaches the faulty path; - lbACLDelete's Run returns `&core.SuccessResult{}` unconditionally on a nil error, so any truthful success produces the value that is dereferenced; - the interceptor guards its pre-fetch with `argsI.(*lb.ZonedAPIDeleteCertificateRequest)` where the argument is a `*ZonedAPIDeleteACLRequest`, so getACL is nil on every path. Measured, with the emulator's own fault injection, on 2026-08-28: 204 (what it answers) rc=0 panic: yes the ACL: deleted 500 (PUT /_feint/faults) rc=1 panic: NO the ACL: survives and the three sibling verbs — acl get, acl update, acl create — carry the same interceptor, exit 0 with empty stderr and never panic, because they answer an *lb.ACL rather than a *core.SuccessResult. The fault sits exactly at "the runner returned a success", and nowhere near this emulator. So the only lever left is failing a delete that worked, which loses the resource and lies about it. Verdict: documentation, not a fix — and not the third option either. Filtering the line in the suite would hide a real upstream defect. What ships instead is the honest half of tolerating it: scw-cli.sh reads the ACL list back after the delete, so "rc=0 with a panic on stderr" is measured rather than trusted, and the day a future scw turns that panic fatal the suite fails on its own. The step also carries the mechanism and points at #505, because whoever meets that orphan line in a log is reading the suite, not the issue tracker. docs/limits.md's section keeps its measurement and gains the disproof, and its one deduction — that a real account would panic the same way — is now labelled as a deduction rather than left reading like a measurement. `mise run conformance:leg -- scw-cli` green with the new assertion; the noise line is still printed, which is the point. Assisted-by: Claude Code (claude-opus-5) --- docs/limits.md | 63 ++++++++++++++++++++++----- tools/conformance/scaleway/scw-cli.sh | 26 +++++++++++ 2 files changed, 79 insertions(+), 10 deletions(-) diff --git a/docs/limits.md b/docs/limits.md index b8f2994d..dee6996f 100644 --- a/docs/limits.md +++ b/docs/limits.md @@ -3684,16 +3684,59 @@ recovered, printed, and the process exits 0. Measured: the exact command, rc=0, the ACL deleted, the stack, the reproduction under `--vm off` as under `--vm incus-ovn`, and no ERROR on the -emulator's side. Deduced, not measured on a real account: the real cloud would -provoke the same panic — nothing in the faulty path depends on the emulator's -answer. - -What to do with it: nothing, here. This is not a divergence and there is -nothing to fix in the emulator; the line is one of noise in every conformance -log, tolerated because delete stderr passes through and rc is 0. Whoever meets -it in a log: #505 is the reference to point at. What would lift it: the -upstream fix in scaleway-cli — one type in the assertion, to be reported -through `scw feedback bug`; the line disappears when a fixed `scw` ships. +emulator's side. + +### The one question worth asking, and its answer: no + +**Could this emulator answer something else, without lying, that does not +trigger the fault?** That is the only version of #505 that would be a fix +rather than a note, so it was answered by reading the two functions and then by +experiment, on 2026-08-28. + +Read (`scaleway-cli/v2@v2.56.3` and `scaleway-sdk-go` in the module cache): + +- `ZonedAPI.DeleteACL` ends on `s.client.Do(scwReq, nil, opts...)` — the + response is decoded into `nil`. **No body this emulator sends reaches the + faulty path**, so the shape of the answer is not a lever at all. +- `lbACLDelete`'s `Run` returns `&core.SuccessResult{Resource: "acl", Verb: + "delete"}` unconditionally whenever that call returns a nil error. So *any* + truthful success produces the value the interceptor then dereferences. +- The interceptor is installed on all four ACL verbs, and its pre-fetch is + guarded by `argsI.(*lb.ZonedAPIDeleteCertificateRequest)` — never true here, + so `getACL` is nil on every path. + +Measured, with the emulator's own fault injection, which is what makes this an +experiment rather than a second reading: + +| what the emulator answers | rc | panic on stderr | the ACL | +|---|---|---|---| +| 204, as it does (`DeleteACL` succeeds) | 0 | yes | deleted | +| 500, via `PUT /_feint/faults` | 1 | **no** | **survives** | + +And the three sibling verbs — `acl get`, `acl update`, `acl create` — carry the +same interceptor, exit 0 with empty stderr, and never panic: they answer an +`*lb.ACL` rather than a `*core.SuccessResult`, so they never reach the branch. +That locates the fault exactly at "the runner returned a success", and nowhere +near the emulator. + +So the emulator's only lever is to **fail a delete that worked**, which loses +the resource and lies about it — the one thing this project exists not to do. +Option (1) is disproved, and #505 closes as documentation rather than as a fix. + +What to do with it: nothing, here. This is not a divergence; the line is noise +in every conformance log, tolerated because delete stderr passes through and rc +is 0. Not filtered either, which would be a workaround hiding a real upstream +defect: it is tolerated *and* the suite now asserts what makes tolerating it +honest — `scw-cli.sh` reads the ACL list back after the delete, so "rc=0 with a +panic on stderr" is measured rather than trusted. Whoever meets the line in a +log: #505 is the reference to point at. What would lift it: the upstream fix in +scaleway-cli — one type in the assertion, to be reported through +`scw feedback bug`; the line disappears when a fixed `scw` ships. + +Still not measured, and it does not change the verdict: what a real account +answers. Nothing in the faulty path reads a response, so the same panic is +expected there — but that sentence is a deduction from the source above, not a +measurement, and it is written here as one. ## The Exoscale stack's second plan is not empty: two per-id outputs read back null at apply time (#520) diff --git a/tools/conformance/scaleway/scw-cli.sh b/tools/conformance/scaleway/scw-cli.sh index 256f6f0f..37cfd215 100755 --- a/tools/conformance/scaleway/scw-cli.sh +++ b/tools/conformance/scaleway/scw-cli.sh @@ -999,7 +999,33 @@ fi prove_end "$neg" scw lb route delete "$route_id" zone="$ZONE" >/dev/null || fail "route delete rejected" + +# THE ORPHAN LINE IN EVERY CONFORMANCE LOG COMES FROM HERE (#505). +# +# scw 2.56.3 prints "runtime error: invalid memory address or nil pointer +# dereference" on the stderr of this command, and exits 0 having deleted the +# ACL. The fault is upstream and entirely client-side, read rather than guessed +# (scaleway-cli v2.56.3, internal/namespaces/lb/v1/custom_acl.go): the +# interceptor on the four ACL verbs asserts *ZonedAPIDeleteCertificateRequest +# where the argument is *ZonedAPIDeleteACLRequest, so its getACL stays nil, and +# a delete that SUCCEEDS then dereferences getACL.Frontend.LB.Tags. The three +# sibling verbs answer an *lb.ACL rather than a *core.SuccessResult and never +# reach that branch, which is why only this line prints it. +# +# Nothing this emulator answers can avoid it: ZonedAPI.DeleteACL decodes the +# response into nil (lb_sdk.go), so no body reaches the faulty path, and the +# command builds its SuccessResult unconditionally on a nil error. The only +# lever left is to FAIL a delete that worked — measured on 2026-08-28 with a +# fault rule: the panic goes, and the ACL survives. docs/limits.md carries the +# whole measurement. +# +# So the noise is tolerated, and this asserts what makes tolerating it honest: +# the delete did its work. Without this line, "rc=0 and a panic on stderr" is +# taken on trust. scw lb acl delete "$acl_id" zone="$ZONE" >/dev/null || fail "acl delete rejected" +scw lb acl list frontend-id="$frontend_id" zone="$ZONE" -o json \ + | jq -e --arg id "$acl_id" 'all(.[]; .id != $id)' >/dev/null \ + || fail "the acl survived a delete that answered 0 (#505 is noise, not a failed delete)" scw lb frontend delete "$frontend_id" zone="$ZONE" >/dev/null || fail "frontend delete rejected" scw lb backend delete "$backend_id" zone="$ZONE" >/dev/null || fail "backend delete rejected" scw lb private-network detach "$lb_id" private-network-id="$lb_pn_id" zone="$ZONE" >/dev/null \ From 8046fc531a1d05cbab22e9d6e5ea229545fe797d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20ROBERT?= Date: Fri, 28 Aug 2026 12:53:52 +0200 Subject: [PATCH 5/6] test(falsify): the seven mutations this lot's guards must redden, and the three that could not MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each guard added for #566, #474 and #477 is mutated in a copy outside the tree, by neutralising a condition rather than deleting a term, and every one of the seven reddens the test that names it. Three more were written and are deliberately absent, because running them disproved their own comment. The matchers' `err != nil -> return false` branches were cited as though TestAnUnreadableFilterMatchesNothingRatherThan Everything held them; neutralised one at a time, that test stays green. json.Unmarshal leaves the slice empty on a failure, and a filter that is present with no accepted values already matches nothing — the property is structural, and those branches are defence in depth against a future change to filterSet.strings, not the guard. So filters.go now says that about itself instead of citing a test it cannot redden, which is this repository's own "un commentaire n'est pas un contrôle" found inside the change that fixes an instance of it. A spec entry that cannot fail is the voided verdict the harness exists to refuse, so it is not declared. `mise run falsify -- tools/falsify/specs/refusals-are-legible.json`: 7 of 7 bite, compiled=yes on every mutation, green after restoration. Assisted-by: Claude Code (claude-opus-5) --- internal/providers/outscale/filters.go | 31 +++++--- tools/falsify/specs/refusals-are-legible.json | 70 +++++++++++++++++++ 2 files changed, 92 insertions(+), 9 deletions(-) create mode 100644 tools/falsify/specs/refusals-are-legible.json diff --git a/internal/providers/outscale/filters.go b/internal/providers/outscale/filters.go index 06c508d8..7334a29a 100644 --- a/internal/providers/outscale/filters.go +++ b/internal/providers/outscale/filters.go @@ -70,11 +70,16 @@ import ( // without it, and TestEveryDeclaredFilterKindIsTheOneTheContractDeclares // holds the kinds against the contract so the table cannot drift from its // source. -// - the matchers below fail *closed* on an unreadable value. The gate should -// mean they never see one; if a future handler forgets the gate, an empty -// answer is a defect somebody reports, and a full one is a defect nobody -// ever notices. TestAnUnreadableFilterMatchesNothingRatherThanEverything -// fails without it. +// - the matchers below fail *closed* on an unreadable value, and that half is +// defence in depth rather than the guard. Measured by running the +// falsification: neutralising any of the three `err != nil` branches leaves +// TestAnUnreadableFilterMatchesNothingRatherThanEverything green, because +// json.Unmarshal leaves the slice empty on a failure and a present filter +// with no accepted values already matches nothing. The property is +// structural; the branches state it, and cost nothing, and their comment +// must not claim a test they cannot redden — which is the exact defect this +// repository names in "un commentaire n'est pas un contrôle", found here in +// the code that fixes it. // // What no type can catch is the third line of the measurement — a filter // declared and never compared. TestEveryDeclaredFilterCanExcludeSomething is @@ -362,10 +367,18 @@ func matchesStrings(f filterSet, name, value string) bool { func matchesAny(f filterSet, name string, values ...string) bool { wanted, present, err := f.strings(name) if err != nil { - // refuseFilters should mean this is unreachable, and it is here for the - // day a handler forgets the gate: an unreadable filter that matches + // refuseFilters should mean this is unreachable, and it is stated for + // the day a handler forgets the gate: an unreadable filter that matches // nothing is a defect somebody reports, and one that matches everything // is #566, which nobody reported for a year. + // + // It is not the guard, and saying so is the point. Neutralised in a + // copy of the tree, every test stays green: the reader answers + // (nil, present, err) and a present filter with no accepted values + // already matches nothing, so the property survives this line's + // removal. Kept because it costs nothing and because a future reader of + // filterSet.strings could change what an unreadable value looks like; + // not cited as though a test held it. return false } if !present { @@ -390,7 +403,7 @@ func matchesAny(f filterSet, name string, values ...string) bool { func matchesInts(f filterSet, name string, values ...int) bool { wanted, present, err := f.ints(name) if err != nil { - return false // fail closed, as matchesAny does and for the same reason + return false // fail closed, as matchesAny does and with the same caveat } if !present { return true @@ -410,7 +423,7 @@ func matchesInts(f filterSet, name string, values ...int) bool { func matchesBool(f filterSet, name string, value bool) bool { wanted, present, err := f.boolean(name) if err != nil { - return false // fail closed, as matchesAny does and for the same reason + return false // fail closed, as matchesAny does and with the same caveat } if !present { return true diff --git a/tools/falsify/specs/refusals-are-legible.json b/tools/falsify/specs/refusals-are-legible.json new file mode 100644 index 00000000..9a28dc47 --- /dev/null +++ b/tools/falsify/specs/refusals-are-legible.json @@ -0,0 +1,70 @@ +{ + "_why": [ + "Every refusal this lot made legible, and the three mutations that are NOT here.", + "", + "The matchers' `err != nil -> return false` branches were written as guards", + "and cited as though a test held them. Running this spec proved otherwise:", + "neutralised one at a time, TestAnUnreadableFilterMatchesNothingRatherThan", + "Everything stays green, because json.Unmarshal leaves the slice empty on a", + "failure and a present filter with no accepted values already matches nothing.", + "The property is structural, the branches are defence in depth, and a spec", + "entry that cannot redden its test is exactly the voided verdict this harness", + "exists to refuse. So they are stated in filters.go as what they are, and not", + "declared here." + ], + "package": "./internal/providers/outscale/", + "mutations": [ + { + "label": "#566: a filter whose value cannot be read is reported as absent again, so matchesAny passes every candidate and a 200 carries the whole inventory", + "file": "internal/providers/outscale/filters.go", + "find": "\tif unreadable := f.unreadable(supported); len(unreadable) > 0 {", + "replace": "\tif unreadable := f.unreadable(supported); len(unreadable) > 0 && false {", + "test": "TestAFilterOfTheWrongShapeIsRefusedRatherThanIgnored" + }, + { + "label": "#566: every declared filter collapses to a list of strings, which is the shape of the defect — VolumeSizes and Progresses were read that way and their integers never decoded", + "file": "internal/providers/outscale/filters.go", + "find": "\t\tout = append(out, filterSpec{Name: name, Kind: kind})", + "replace": "\t\tout = append(out, filterSpec{Name: name, Kind: kind - kind})", + "test": "TestEveryDeclaredFilterKindIsTheOneTheContractDeclares" + }, + { + "label": "#566: ReadSnapshots stops comparing the account, which is the filter no decoder and no contract check could ever have caught — declared applied, compared nowhere", + "file": "internal/providers/outscale/snapshots.go", + "find": "\t\tmatchesStrings(f, \"AccountIds\", stringOf(view[\"AccountId\"])) &&", + "replace": "\t\t(matchesStrings(f, \"AccountIds\", stringOf(view[\"AccountId\"])) || true) &&", + "test": "TestEveryDeclaredFilterCanExcludeSomething" + }, + { + "label": "#566: the volume size filter matches every volume again, which is the exact reading the issue measured", + "file": "internal/providers/outscale/volumes.go", + "find": "\t\tmatchesInts(f, \"VolumeSizes\", sizes...) &&", + "replace": "\t\t(matchesInts(f, \"VolumeSizes\", sizes...) || len(sizes) >= 0) &&", + "test": "TestAVolumeSizeFilterExcludesAVolumeOfAnotherSize" + }, + { + "label": "#474: the boot refusal is levelled as an incident again, and fourteen ERROR lines over fifteen stack replays teach an operator to skip the log's errors", + "file": "internal/core/machine/binding.go", + "find": "\tb.logger().Warn(\"refusing to boot: nothing says which operating system this image identifier names\",", + "replace": "\tb.logger().Error(\"refusing to boot: nothing says which operating system this image identifier names\",", + "test": "TestADocumentedRefusalIsAWarningAndAFailureStaysAnError", + "package": "./internal/core/machine/" + }, + { + "label": "#477: an operation feint declines answers the same bytes as an empty answer, with nothing out of band to tell a program which it met", + "file": "internal/core/emulator/emulator.go", + "find": "\tif bestName != \"\" {\n\t\tw.Header().Set(NotEmulatedHeader, bestName)", + "replace": "\tif bestName != \"\" && false {\n\t\tw.Header().Set(NotEmulatedHeader, bestName)", + "test": "TestAnUnroutedAnswerCarriesTheNotEmulatedHeader", + "package": "./internal/core/emulator/" + }, + { + "label": "the redaction flattens a list of scalars back to one string, which is how the corpus came to hold a KeypairNames array written as a bare string and a replay came to reissue a shape no client ever sent", + "file": "internal/proxy/redact.go", + "find": "\t\t\t\tif items, ok := scalarList(nested); ok {", + "replace": "\t\t\t\tif items, ok := scalarList(nested); ok && false {", + "test": "TestARedactedListOfScalarsStaysAList", + "package": "./internal/proxy/" + } + ] +} From 00d65ab6b210472c7e28dfe1c31bb1a02a70acf9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20ROBERT?= Date: Fri, 28 Aug 2026 13:04:15 +0200 Subject: [PATCH 6/6] docs(limits): two acknowledgements, one because a section was rewritten and one because it was read (#558) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The #474 section: its heading changed when the limit was lifted, and the ledger is keyed by heading, so its line was orphaned. Re-keyed and dated today, which is when the section was rewritten. The #507 section, "A machine's route out": re-dated because somebody read it, which is the only thing this ledger records and the only thing that makes it worth having. What was read, on 2026-08-28: the section itself — the shape/outbound/DNS table measured on 2026-08-26, #202's reasoning for why a routed NIC has no route out, and the consequences list — then `git show 2879888` to see whether #514 could have moved any of it. Its only touch on the routing layer is `func (r Reconciler) router() Router` becoming `router() router`: an interface renamed to unexported, no NAT, no resolver, no interface shape, no cloud-init. The section's own named control, TestAPackageStepWithNoRouteOutIsSaidOutLoud, ran green today in the file this branch edits for #474. The limit stands. AND THE JUSTIFICATION THIS COMMIT FIRST CARRIED WAS WRONG, which is worth more than the correction. It said the section was "RED BEFORE THIS BRANCH", framed as a property of the tree that this lot happened to clear. It is not a property of the tree at all: - #507 closed 2026-08-26 and its acknowledgement reads 2026-08-27, so the guard was satisfied on its own title issue; - the tool's subject is #514, which the section body cites once ("the architecture audit (#514), not a patch") and which closed 2026-08-28T09:33Z — the same minute as 2879888, the commit this branch starts from. So the section went stale this morning because a clock ticked, not because of anything in 2879888's content and not because of anything here. A verdict that depends on wall-clock time and on GitHub's state reads exactly like a verdict about a tree, and that is how it was misreported. `mise run limits:check` on 2879888's own two files, run in this repository at 13:00 today, still exits 2 and names #514. Assisted-by: Claude Code (claude-opus-5) --- docs/limits-acks.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/limits-acks.json b/docs/limits-acks.json index 10798407..5beb6354 100644 --- a/docs/limits-acks.json +++ b/docs/limits-acks.json @@ -6,7 +6,7 @@ "A Scaleway server's root volume type: what is writable, and what is not": "2026-08-27", "A VPC created without `enable_routing` answered `routing_enabled=false` where the real cloud answers `true` (#497, lifted 2026-08-27)": "2026-08-27", "A declared query parameter is served or refused, never dropped — and `labels` is the refused one": "2026-08-27", - "A machine's route out: which shapes reach a package repository (#507)": "2026-08-27", + "A machine's route out: which shapes reach a package repository (#507)": "2026-08-28", "A public address is the provider's value, made to answer on the host": "2026-08-27", "A run presented as local can still reach the real cloud (#280)": "2026-08-27", "An API reboot used to log `Failed to add route: file exists` for its own public /32 (#498, lifted 2026-08-27)": "2026-08-27", @@ -15,7 +15,7 @@ "An Outscale load balancer distributes packets inside its network, and nowhere else": "2026-08-27", "An Outscale machine owns a root volume, and that volume holds no bytes": "2026-08-27", "Exoscale has one zone per process, and the reason is the client": "2026-08-27", - "Fourteen ERROR lines over fifteen stack replays are one documented refusal, logged at the wrong level (#474)": "2026-08-27", + "An ERROR is a failure and a WARN is a decline, and one refusal was on the wrong side (#474)": "2026-08-28", "Identifiers are not checked against anything": "2026-08-27", "Lifecycle transitions are immediate": "2026-08-27", "Managed Kubernetes is not emulated, and a CRUD-only version is refused (#283)": "2026-08-27",