feat(iam): show what an API key can do, and take a tag off a resource - #267
feat(iam): show what an API key can do, and take a tag off a resource#267Denis-hamon wants to merge 22 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>
The grid scores access management 2/5, and its stated reason is half out of date: logout shipped in ovh#228, and the CLI pushes no token, it collects a key the operator made elsewhere. The real gap is narrower and sharper. Nothing showed what a credential could do. On the account this was measured against there are 66 of them: 17 never used, 44 that never expire, 2 restricted to an IP, and 38 that can call the whole API. That last number is why the listing has a scope column rather than a rule count -- 139 of the rules here are on "/*", and "4 rules" reads as narrow. --unused exists for the first number: a key never used, never expiring, usable from anywhere is the one to revoke first, and it could not even be listed. The plan for this said `iam resource edit --tag` replaces the whole tag map and silently drops the others. Measured, and it does not: common.EditResource GETs the resource and merges before it PUTs. The raw API does replace -- two POSTs then a PUT carrying one tag leaves one tag -- so the merge is real work done by the client across two calls, and an edit landing between them is lost. Worth knowing, but not the gap. The gap is that nothing can remove a tag at all. A shorter --tag list removes nothing, by design, and --tag key= writes {"key": ""} -- a tag that looks removed and is not. `tag set` and `tag remove` are the only way to take one off, and set posts one tag at a time without reading the others first. `iam check` is a POST that writes nothing and answers what a permission error does not: which actions are allowed, on which resource. Both directions were exercised. One unknown action name fails the whole batch and the API's 400 does not say so, so a typo in the fourth action reads as "the check is broken"; the refusal is translated instead. The action reference holds 9158 actions across 117 product families, so past 400 matches it says how to narrow rather than printing them. --type is sent to the API; --category and --search are applied to what comes back, because the API filters on neither. The credentials live in /me while the rest of IAM is v2. They are placed under iam because Scaleway, AWS and gcloud all keep API keys under their identity command, and because an operator asking what a key can do looks where the policies are. Nothing existing is moved. Left out: the identity directory beyond enable/disable, because exercising it means creating real users and groups on a production account. And what a write does to the ovh:-prefixed computed tags could not be exercised -- they exist on this account, on a billing account and on domains, but on no dedicated server. Signed-off-by: Denis <denis.hamon@ovhcloud.com>
3a76445 to
965cb5d
Compare
c8d6d2c to
2f14b4e
Compare
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
`credential list`, `reference actions` and `reference resource-types` registered the flag and rendered their rows directly. withFilterFlag only binds --filter to flags.GenericFilters; display.RenderTable does not read it, and ManageListRequest — which normally passes it to filters.FilterLines — is not on this path because the rows are assembled here. So the flag was accepted by cobra, written into doc/ by docgen, and inert. An operator reading a list they believe was narrowed is worse served than one who was never offered the flag. These go through common.RenderFilteredTable, added for the same defect earlier in the stack. The test asserts the row the filter EXCLUDES is absent, not merely that the kept one is present: the second passes just as well with the filtering removed. Found by a review pass over this branch, then confirmed by an audit of every withFilterFlag call site in the tree — twenty-one commands this series added carried the same defect. 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>
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>
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>
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 — Does not workO5 · You could not remove a tag
🛑 failed to read the tags of urn:v1:eu:resource:dedicatedServer:ns31633811: OVHcloud API error (status code 404): Client::NotFound::RESOURCE_NOT_FOUND: "resource not found" (X-OVH-Query-Id: EU.ext-5.6a8b854b.1090988.2b41961fca552531612672aa118a07fd) — Denis (Product Manager), through CLI sandbox review page |
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/iam-credentials-and-tags
Signed-off-by: Denis <denis.hamon@ovhcloud.com>
Human review — Does not workO5 · You could not remove a tag
🛑 failed to read the tags of : OVHcloud API error (status code 400): Client::BadRequest::INVALID_PARAMETER: "invalid resource identitifer" (X-OVH-Query-Id: EU.ext-4.6a8c69e1.541833.f7c7a07d11b58bf2ca1ca67851c023e0) — Denis (Product Manager), through CLI sandbox review page |
`iam resource edit --tag env=` is what somebody types to take a tag off. The
API accepts it, stores {"env": ""}, and the tag then looks gone while a policy
written against its KEY still matches it. Of the three ways this edit can
surprise, that is the worst, and it is the natural attempt.
The edit merges — the CLI GETs the resource, adds what was given and PUTs the
whole thing back — so a shorter --tag list removes nothing either. That was
never written down anywhere the operator would look. It is now in the flag's own
help, without backticks: cobra reads those as the argument TYPE, and the help
rendered as `--tag iam resource tag remove`.
There is now a command that removes a tag for real, so the empty value is
refused and points at it, before the GET rather than after it.
Tested both ways: the refusal reaches no endpoint at all, and a real value still
goes through to the PUT. Sabotaged the condition — the refusal test goes red.
Stacked on #266. The reviewable unit is the last commit.
The grid in the product note scores "Authentication and access management" 2/5,
the widest remaining gap against the panel. Its stated reason is half out of date —
logoutshipped in #228, and the CLI pushes no token, it collects an application keythe operator created elsewhere. The real gap is narrower and sharper: nothing in the
CLI shows what a key can do, and nothing can take a tag off a resource.
What the account looks like once you can see it
66 API credentials, none of them visible from the CLI before this patch:
That last row is why
scopeis a column rather than a rule count: 139 of the rules onthis account are on
/*, and "4 rules" would have read as narrow. A key that holdsGET POST PUT DELETEon/*is reported aswhole API: DELETE,GET,POST,PUT.--unusedexists because of the first row. A key that has never been used, neverexpires and works from anywhere is the one to revoke first, and the CLI could not
even list it.
Fine-grained tagging — the premise was wrong, and the real gap is worse
The plan for this lot said
iam resource edit --tagsends a PUT that replaces thewhole tag map, silently. Measured, and it does not.
common.EditResourceGETs theresource and merges before it PUTs, so the tags already there survive. That is the
eighth audit premise this series has had to correct with a measurement.
The raw API does replace — posting two tags then PUTting one leaves exactly one —
so the merge is real work done by the client, over two calls, with an edit landing
between them being lost. That is worth knowing but it is not the gap.
The gap is that nothing can remove a tag. Measured on a real resource:
edit --tag owner=xwith a shorter listedit --tag only=with an empty value{"only": ""}— a tag that looks removed and is nottag remove onlySo
tag setandtag removeare the only way to take a tag off a resource, andsetposts one tag at a time without reading the others first — no read-modify-write, no
window for a concurrent edit.
iam check, and why it is the most useful command hereIt is a POST that writes nothing, and it answers the question a permission error does
not: which of these actions are allowed, on which resource. Both directions verified
against the API:
One unknown action fails the whole batch, and the API's 400 does not say that. A
typo in the fourth action therefore reads as "the check does not work". The refusal is
translated: it says one bad name sank the rest, how many went unanswered, and where to
find the real names.
The action reference refuses to print itself
9158 actions across 117 product families —
dedicatedServeralone has 183. A tableof nine thousand rows is not an answer, so past 400 matches the command says how to
narrow instead.
--typeis sent to the API;--categoryand--searchare applied towhat comes back, because the API filters on neither.
Verification
Gate green on the five steps.
Three of the first eighteen sabotages did not do their job, and each was worth the
detour. One came back green: the dry-run of
credential deletewas tested and theguard was not — the half that actually stops a mistake. Two broke the build,
which proves nothing about a test's teeth; re-aimed so they compile, both bite. Fixing
the first also surfaced a defect in the product:
tag set --dry-runprinted the samePOSTline once per tag and never said which tag, becausecommon.Callcarries nobody on this stack. A preview that cannot be told from another preview is not a preview.
41 tests (21 unit, 20 cobra) and 19 sabotages, 19 red, with restoration verified by checksum.
Run against the real API: the credential listing and all three filters, the refusal of
an unknown status,
checkin both directions and with an unknown action, the referencewith and without a filter, and the full tagging cycle on a designated test server —
set,setagain, the oldedit --tagalongside it for comparison,remove, and therefusal of an absent key. The server's tags were read outside the CLI before and
after, and are back to
null.The credentials live in
/mewhile the rest of IAM is v2. They are placed underiambecause Scaleway, AWS and gcloud all keep API keys under their identity command, and
because an operator asking "what can this key do" looks where the policies are. Nothing
existing is moved.
Not in this patch
/me/identity/group(22 groups) and the rest of the identity directory: exercising thewrite paths means creating real users and groups on a production account, which the
tagging tests did not require. Its own lot.
Tags prefixed
ovh:are computed by OVHcloud. They exist on this account —ovh:defaulton a billing account,
ovh:whoisOwneron domains — but on no dedicated server, sowhat a write does to them could not be exercised within the authorised scope. Stated
rather than assumed.