feat(baremetal): the backup space nobody could see, and who was allowed on it - #258
feat(baremetal): the backup space nobody could see, and who was allowed on it#258Denis-hamon wants to merge 24 commits into
Conversation
…to change
`vps service-info edit myvps --renew-period 12` used to send this:
{"renew":{"automatic":false,"deleteAtExpiration":false,"forced":false,
"manualPayment":false,"period":12}}
The renewal settings are booleans bound to a shared struct carrying no
`omitempty`, so every one of them was marshalled at its zero value and won the
merge against the fetched resource. Changing the renewal period therefore also
switched automatic renewal off — on a service that had been renewing itself for
years, without a word in the output saying so.
Webhosting already built its payload from `cmd.Flags().Changed`, and did not
have the defect. This promotes that builder into `common`, so `vps` gets it too
and the next `service-info edit` cannot reintroduce the bug by reaching for the
struct.
Reading `Changed` rather than the values keeps `--renew-automatic=false`
working: pflag records a flag as changed whatever value it was given, so an
explicit false is still sent while an absent flag stays absent. Both cases are
covered by a test, and each test was checked against the failure it exists to
catch.
The shared mutable `ServiceInfoSpec` goes away with the last thing that read
it, and the five flag registrations repeated across four commands become one
call, which also settles the two spellings of the period's help text.
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
…ayer Declaring cobra flags is the command layer's job, and internal/services/common was the only service package doing it — the shared flag helpers all live in internal/cmd. Raised in review of this PR. The registration moves; the table does not. Both halves need the flag name — one to declare it, the other to read whether the operator set it — and that name is the only thing tying them together. Splitting it into two copies would mean a rename could touch one side and leave the other silently no longer sending a setting, which is the exact failure this PR exists to fix. So common.ServiceInfoRenewFlags becomes the exported description, internal/cmd registers from it, and the payload builder keeps reading it. Checked by renaming an entry in that table and watching the service-info tests fall: the two halves still move together. Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ed on it A dedicated server comes with a Backup FTP space, and the CLI could neither show it nor say who was allowed to reach it. Nine routes, none of them wired: the space itself, the access list that guards it, the two passwords, the cloud backup beside it and the capacities that can be ordered. Measured on a real account, on all thirty-five servers: seven have a Backup FTP space, none has a cloud backup, thirty-one can order extra storage and four cannot. So the reads are demonstrable and the writes had to be run. Running them found a defect in this commit's own --wait. The space answers 200 after seven minutes with an empty ftpBackupName, while its creation task is still going — and the API then refuses every write against it: "You have a backupFTP create task pending, you cannot release your backupFtp until it's done." Waiting for the object to exist therefore handed back a space nothing could be done with, eight minutes early. It waits for the name now, which is what says the space has been placed on a storage server. Measured end to end: sixteen minutes and twenty-seven seconds to create, one minute nine to delete. The two directions of that wait are not the same question either. A space halfway through its creation is not ready, but it is very much still there, so a deletion that reused the readiness predicate would have reported "gone" about a space being provisioned. Three more things the account had to be asked. An access rule takes three booleans and the API accepts all three false: a rule that appears in the list, says an IP block is allowed, and lets it reach nothing — worse than no rule, because it reads as protection. The blocks a space accepts are per server and regional: 115 on the European servers, 5 on the Canadian ones, so the rule is checked against the server rather than sent to be refused. And the cloud backup password reset answers four credentials in the response body, unlike the Backup FTP one which the API mails — they are withheld behind --reveal, on the object so that -o json is covered by the same decision as the table. A server the cloud backup offer does not cover answers HTTP 403 — thirty-four of the thirty-five. That is a business answer, not a permission problem, and it is reported as one. Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A hostname, an address, an order id and a colleague's name read off a live account had been used as fixtures and as examples in comments. This repository is public: what goes in stays in. The values are replaced with synthetic ones of the same shape — RFC 5737 documentation addresses, hostnames built on them, identifiers of the same length — so the tests keep exercising the same parsing. This is the fourth time in this series that live account data reached a commit, after a live IP migration token, a test server's address, and a real invoice id. The first three were fixed one at a time as they were noticed; this is the sweep that should have followed the first one. Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
A hostname, an address, an order id and a colleague's name read off a live account had been used as fixtures and as examples in comments. This repository is public: what goes in stays in. The values are replaced with synthetic ones of the same shape — RFC 5737 documentation addresses, hostnames built on them, identifiers of the same length — so the tests keep exercising the same parsing. This is the fourth time in this series that live account data reached a commit, after a live IP migration token, a test server's address, and a real invoice id. The first three were fixed one at a time as they were noticed; this is the sweep that should have followed the first one. Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
… elsewhere `backup cloud password` withholds the four credentials it resets, on the reasoning that a password reset is exactly the command somebody runs with the output on screen or in a build log. The same four are in the object, one level down: archive and storage each carry an sftp and a swift block, and each of those a password — format: password in the schema, all four of them. `show` and `create` printed that object whole. Masking on one command and printing on the two others is not a policy, it is an oversight, and `show` is the likelier leak of the three: it is a read, so it can be run again and again, including by a script that logs what it reads. Both now go through the same withholding, and both gained --reveal for the operator who actually needs the credential. The walk keys on the field name rather than on the four known paths: this object is a tree the API grows, and a third container — or a third protocol under an existing one — would otherwise be printed in the clear by a masker that still looked like it was working. It copies rather than edits in place, so its correctness does not depend on the order in which it is called. Three tests. Two sabotages, both compiling: one that returns the object untouched, one that masks only the top level — where none of the four are. Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Regenerated by make doc. The flag added to `backup cloud show` and `backup cloud create` in the previous commit changes their pages, and the CI gate compares the tree against what docgen produces. Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`backup cloud offer` read every HTTP 403 as "no cloud backup offer covers this
server". Thirty-four of the thirty-five servers measured do answer 403 for
exactly that reason, so the shape was right — but an API key that simply lacks
the grant on this route answers 403 too, and it was told a fact about the
catalogue instead of about itself. Under -o json it was handed offered:false,
which a script has no way to doubt.
The business answer is now matched on what it says. Those 34 servers returned
{"message":"Not available for this server"}; anything else with a 403 is reported
as a refusal, naming the grant to check.
The existing test carried an invented body — "not offered" — which is precisely
how a status-only check passed while the command could not tell the two apart.
It now uses the message the API really returned, and a second test covers the
refusal.
One compiling sabotage, back to matching on the status: red on the new test,
green on the old one, which is the asymmetry worth having.
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Promoting `isNotFound` to `common.IsNotFound` updated two of its three call sites. The third, in the anti-spam statistics path, kept calling the function that had just been deleted, so the `ip` package stopped compiling — and with it the whole binary and every branch stacked above. Two tests already pin both sides of that exact line: a 500 must surface as an error, a 404 must still read as "not flagged". Neither could fail, because a package that does not compile runs no tests at all. A build failure is the finding, never the noise to look past. Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The wait for a deletion decided on `err == nil`. Every failure to read the space — a 500, a rate limit, a dropped connection — therefore counted as "not there", and "not there" is precisely the answer the deletion is waiting for. One bad poll ended the wait and printed "✅ deleted" over a space that was still there, which is the worst shape this class of bug takes: the operator is told the thing is gone. There are three outcomes, not two: the space answered, the API says it is not there, and the read did not happen. Only the second is an answer about the space. A sentinel error carries that distinction — a sentinel rather than a boolean beside the error, so a caller cannot forget to look at it — and the loop now logs an unreadable poll and keeps waiting. The same distinction holds the other way: a 404 is what the route answers for most of the sixteen minutes a creation takes, so it must not end that wait either. Both directions are tested. Five tests, three sabotages, three reds — the first of the three first broke the build, which proves nothing about a test's teeth, and was rewritten to reintroduce the original defect while still compiling. Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
ovh#243 moved the Cobra registration of the renewal flags out of internal/services/common and into the command layer, and exported the descriptor table so both halves keep reading one list. That commit landed after this branch had already taken its parent, so this branch still carried the version where a service package declares flags — and being the more recent side of the merge, it would have won and quietly undone the refactor. Merged rather than rebased: nothing is rewritten, so the review threads on this PR stay attached to their lines. Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Human review — DoubtH1 ·
🔍 Dry run: nothing was sent. This would have been called: — Denis (Product Manager), through CLI sandbox review page |
Human review — Does not workH2 · An ACL that opens no protocol is refused
🛑 an access rule has to open at least one protocol; give --ftp, --nfs or --cifs — Denis (Product Manager), through CLI sandbox review page |
Human review — Does not workH3 · Four passwords inside one object
🛑 ns31633811.ip-162-19-234.eu has no cloud backup: OVHcloud API error (status code 404): "The requested object (backupCloud) does not exist" (X-OVH-Query-Id: EU.ext-1.6a8b4514.1195793.4a626d6fe3a0a86dedde4c1134f83d7e) — Denis (Product Manager), through CLI sandbox review page |
|
Follow-up on H1, H2 and H3. Two of the three are false negatives, and the third is an absence worth recording. H2 — the refusal is the pass. H3 — no server on this account has a Cloud Backup. Measured on all 30 reachable servers on 24/08: zero. The 404 was not the command misbehaving, it was the sandbox firing at an object that exists nowhere. The masking of the four passwords therefore cannot be exercised live at all; the check is now announced as replay-only, with that reason spelled out. H1 — not observable from the button either. |
A --dry-run already carries the parameters in its message, and a
log.Println sitting just above the branch repeated the same JSON behind
a Go timestamp no other command in this CLI emits:
🔍 Dry run: nothing was sent. This would have been posted to …
{ "operatingSystem": "debian12_64" }
2026/08/23 23:47:22 Final parameters:
{ "operatingSystem": "debian12_64" }
The log line moves below the branch. A real run still logs what it is
about to send, which is what it was for; a dry run logs nothing, because
it sends nothing.
The line goes to stderr, so no assertion on stdout could ever have seen
it — which is why it survived every green run. Two tests now redirect the
logger: one that a dry run does not log, and its positive control that a
real run still does, so deleting the line outright would not pass.
Signed-off-by: Denis <denis.hamon@ovhcloud.com>
Mechanical, not a change of behaviour. Placing them just after TestBaremetalReinstallDryRun put them in the one region of baremetal_test.go that 23 downstream branches also append to, and in the import block they all touch as well. Merging the parent into those branches failed 23 times out of 23, every one of them on adjacency rather than on a disagreement. baremetal_test.go goes back to what it was before the previous commit, so this branch now leaves that file untouched. A new file can only clash with a file of the same name, and nothing else carries this one. Signed-off-by: Denis <denis.hamon@ovhcloud.com>
…' into feat/baremetal-backup
Signed-off-by: Denis <denis.hamon@ovhcloud.com>
Human review — Does not workH3 · Four passwords inside one object
🛑 ns31633811.ip-162-19-234.eu has no cloud backup: OVHcloud API error (status code 404): "The requested object (backupCloud) does not exist" (X-OVH-Query-Id: EU.ext-1.6a8b4514.1195793.4a626d6fe3a0a86dedde4c1134f83d7e) — Denis (Product Manager), through CLI sandbox review page |
Human review — Does not workH1 ·
🔍 Dry run: nothing was sent. This would have been called: — Denis (Product Manager), through CLI sandbox review page |
Human review — Does not workH3 · Four passwords inside one object
🛑 ns0000001.ip-203-0-113.eu has no cloud backup: OVHcloud API error (status code 404): Client::NotFound: "This service does not exist" (X-OVH-Query-Id: EU.ext-4.6a8c73b0.523112.3787859a28588c2145a7641cb7714202) — Denis (Product Manager), through CLI sandbox review page |
Human review — Does not workH3 · Four passwords inside one object
🛑 ns0000001.ip-203-0-113.eu has no cloud backup: OVHcloud API error (status code 404): Client::NotFound: "This service does not exist" (X-OVH-Query-Id: EU.ext-3.6a8c8172.1295079.161d2e072f609a195626b187e8c8aa9a) — Denis (Product Manager), through CLI sandbox review page |
Human review — Does not workH1 ·
🔍 Dry run: nothing was sent. This would have been called: — Denis (Product Manager), through CLI sandbox review page |
Human review — Does not workH3 · Four passwords inside one object
🛑 ns0000001.ip-203-0-113.eu has no cloud backup: OVHcloud API error (status code 404): Client::NotFound: "This service does not exist" (X-OVH-Query-Id: EU.ext-3.6a8c8172.1295079.161d2e072f609a195626b187e8c8aa9a) — Denis (Product Manager), through CLI sandbox review page |
1 similar comment
Human review — Does not workH3 · Four passwords inside one object
🛑 ns0000001.ip-203-0-113.eu has no cloud backup: OVHcloud API error (status code 404): Client::NotFound: "This service does not exist" (X-OVH-Query-Id: EU.ext-3.6a8c8172.1295079.161d2e072f609a195626b187e8c8aa9a) — Denis (Product Manager), through CLI sandbox review page |
Human review — Does not workH2 · An ACL that opens no protocol is refused
🛑 an access rule has to open at least one protocol; give --ftp, --nfs or --cifs — Denis (Product Manager), through CLI sandbox review page |
Human review — Does not workH3 · Four passwords inside one object
🛑 ns0000001.ip-203-0-113.eu has no cloud backup: OVHcloud API error (status code 404): Client::NotFound: "This service does not exist" (X-OVH-Query-Id: EU.ext-3.6a8c8172.1295079.161d2e072f609a195626b187e8c8aa9a) — Denis (Product Manager), through CLI sandbox review page |
Human review — Does not workH2 · An ACL that opens no protocol is refused
🛑 an access rule has to open at least one protocol; give --ftp, --nfs or --cifs — Denis (Product Manager), through CLI sandbox review page |
1 participant
What this adds
A dedicated server comes with a Backup FTP space. The CLI could neither show it nor say who was
allowed to reach it — nine routes, none of them wired.
9 routes, 15 commands. Stacked on #257; only the last commit belongs to this pull request.
What the account said
All thirty-five servers, every backup route:
features/backupFTPfeatures/backupFTP/accessfeatures/backupFTP/authorizableBlocksfeatures/backupCloudbackupCloudOfferDetailsorderable/backupStorageThe
--waitthis commit shipped first was wrong, and only running it showed thatA real creation on an authorised server: the space answered 200 after seven minutes with an empty
ftpBackupName, while itscreateBackupFTPtask was stilldoing. Every write against it wasthen refused:
So waiting for the object to exist handed back a space nothing could be done with, eight minutes
early. It waits for the name now — the field that says the space has been placed on a storage
server.
POST features/backupFTP→ object answers 200ftpBackupNameempty)ftpBackupNamefilleddonebackup ftp delete --wait404— back where it startedThe name and the task completion fell in the same 30-second sample, so the fixed wait is right to
within that interval; the object-exists check was wrong by nearly nine minutes.
The two directions of that wait are not the same question either. A space halfway through its
creation is not ready, but it is very much still there — a deletion that reused the readiness
predicate would have reported "gone" about a space being provisioned. They are separate now, and a
sabotage that merges them turns the test red.
Three more things the account had to be asked
An access rule can open nothing. The API takes three booleans and accepts all three false: a
rule that appears in the access list, says an IP block is allowed, and lets it reach nothing. That
is worse than no rule, because it reads as protection that is in place. It is refused.
The blocks a space accepts are per server and regional — 115 on the European servers, 5 on the
Canadian ones — so
acl addchecks the block against the server rather than sending it to berefused. The refusal counts them and names the command that lists them: printing 115 lines in an
error message answers worse.
The cloud backup password reset answers four credentials in the response body, unlike the
Backup FTP one which the API mails. They are withheld behind
--reveal, and the substitutionhappens on the object so
-o jsonis covered by the same decision as the table — a password resetis exactly the command run inside a pipeline whose output is kept.
Verified against the API
Reads, on the servers that have a space:
backup ftp show(quota, storage server, type),acl list(the rules with their protocols spelled out rather than three booleans to cross-read),authorizable-blocks,backup orderableon both an eligible and an ineligible server, andbackup cloud offeron the one server it covers and on one of the thirty-four it does not — wherethe 403 is reported as an answer, not as a failure.
Writes, run end to end on an authorised server and then undone: create, the auto-created ACL the
provisioning leaves behind,
acl set,acl delete, the FTP password change, and the deletion. Theserver is back to answering 404, read outside the CLI.
Five refusals executed against the API: no space, no space so no access list, a block the server
does not accept, a rule opening no protocol, and no cloud backup.
Notes
IsNotFoundmoves tointernal/services/common: it was written twice already in this stack, anda second copy of a routine that decides between "this does not exist" and "the API failed to
answer" does not expire — it stays wrong in silence while the original is fixed.
backup ftp deletetakes the strongest guard the CLI has, in the API's own words: "Terminateyour Backup FTP service, ALL DATA WILL BE PERMANENTLY DELETED".
backup cloud deletetakes theweaker one, also on the API's own statement: "This does not delete container data."