feat(ip): say what is blocking an address, and what its filter lets through - #256
Draft
Denis-hamon wants to merge 19 commits into
Draft
feat(ip): say what is blocking an address, and what its filter lets through#256Denis-hamon wants to merge 19 commits into
Denis-hamon wants to merge 19 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>
…hrough
Three separate mechanisms can block an address — anti-hack, ARP and anti-spam —
each with its own list, its own detail route and its own release route. None
was reachable from the CLI, and neither was the anti-DDoS configuration under
them: mitigation, its auto-mitigation profiles, and the game filter with its
rules. 26 routes, none of them exposed.
`blocked` reads the three lists together, because an operator hit by one of
them has no way to know which, and what it prints is what `unblock` takes.
Three things the account had to be asked, on all 537 of its blocks:
The `time` field means the opposite thing on one of the three mechanisms. On
anti-hack and ARP it is the cooldown before a release is accepted; on spam it
is the length of the block itself. One column holding both under one header
would be wrong on a third of its rows and look right, so the number becomes a
sentence per mechanism — and `unblock` refuses locally while the cooldown runs,
because the API refuses it with "The requested object does not exist", which
reads as "unknown address" when it means "not yet".
`GET /ip/{ip}/phishing` answers 500 for every address hosted outside Europe:
ten network prefixes fail on 100% of their blocks, the 49 others answer 200 on
100% of theirs, not one prefix mixed — while the four sibling families answer
200 on those very same blocks. The failure is reported as a failure. An empty
table there would say "no phishing reported" about a block nobody could read.
The game protocols come from the address, not from the enum shipped with this
CLI. The two disagree: `arkSurvivalAscended` is reported by the API and used by
a live rule, and is absent from the embedded schema. The lists also differ
between addresses, 14 protocols against 20, as does the rule limit, 30 against
100 — so there was no single right list to validate against anyway.
And firewall mode only lets through UDP traffic matching a rule, so turning it
on with no rule drops every UDP packet to the address. One address measured sat
exactly there: mode off, zero rules. `game edit` refuses instead, and names the
command that adds a rule first.
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Denis-hamon
force-pushed
the
feat/ip-incident-surface
branch
from
August 19, 2026 13:59
a32d3ee to
5fd72b1
Compare
…s the same blackhole Seven defects found by an adversarial review of the previous commit, six of them real. Two would have shipped: A spam block carries `time` = "time while the IP will be blocked", where anti-hack and ARP carry "time remaining before you can request your IP to be unblocked". The previous commit knew that in one place and forgot it in the other: the path that works the mechanism out itself passed the sentence length on as a cooldown and refused the release, while `--reason spam` performed the same POST without blinking — and the table, on the same number, printed the correct "blocked for 1d". Three readings of one field, two of them wrong. Enabling firewall mode with no rule drops all UDP, and the previous commit guarded that. Deleting the last rule while firewall mode is on does exactly the same thing, and was not guarded at all — the outcome was one `game rule delete` away, on the command an operator runs while tidying up rather than while changing protection. And four smaller ones. `--reason` swallowed a 404, so it became the way around the refusal it exists to give, and reached the API for the opaque "The requested object does not exist" the auto-detection was written to avoid; a read failure is now told apart from an address this mechanism does not hold. The mechanism and the address now travel in the API's spelling rather than the one typed, because both end up in a path segment and in a comparison — a `--reason Spam` passed validation and then missed the spam exemption. The profile update treated every read failure as "no profile yet", sending a create where an update was due and making --dry-run name the wrong verb. And `--timeout 0`, one of the five values the API takes, announced "stays on for no delay", which names the setting and describes no behaviour. The termination helpers move to internal/services/common on the way: the survey enums are shared API types, and a second copy of a routine that withholds a credential does not expire — it stays wrong in silence while the original is fixed. The six baremetal termination tests are the regression net for the move and pass unchanged. Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 19, 2026
Denis-hamon
marked this pull request as draft
August 20, 2026 13:09
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>
The first sweep replaced only the exact spelling. A test that checks a name can be copied back without reproducing its case carried the uppercase form of a real hostname, which therefore survived — and, once its neighbour was replaced, made that test fail. A real value does not stop being one because one of its spellings was removed. 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> # Conflicts: # internal/services/ip/move_test.go
Denis-hamon
added a commit
to Denis-hamon/ovhcloud-cli
that referenced
this pull request
Aug 20, 2026
--period and --type were validated against two string slices written by hand in traffic.go. The values match dedicated.server.MrtgPeriodEnum and dedicated.server.MrtgTypeEnum today, which is precisely why nobody would notice them drifting apart: a copied list does not fail, it quietly refuses something the API accepts. This repository has already paid for that. ovh#256 shipped a game-protocol list copied the same way, and it refused arkSurvivalAscended — a protocol the API returns in supportedProtocols and that a live rule on a real account was already using. Both lists are now read from the embedded schema through openapi.GetComponentEnum behind sync.OnceValues, so the day the schema is refreshed the flags follow. The refusal still lists what is accepted, and now it lists what the schema says rather than what someone remembered. The shared helper for this, common.CheckEnumFlag, does not exist yet at this point of the stack — it arrives in a later branch. A local helper does the same job here and can be collapsed into the shared one when the stack lands. Found by a third-party review pass over this branch. 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>
…elay
Two defects on the same surface, both of which made the command state something
false rather than admit it did not know.
`spam-stats` turned every failure of GET /ip/{ip}/spam/{ip} into one sentence:
"is not flagged by the anti-spam system, so it has no statistics". That is a
statement of fact about the address, and any 403, 429 or 500 produced it. These
/ip/ routes are exactly the ones that answer 500 for every block hosted outside
Europe — 52 of the 537 on the account measured, ten network prefixes failing on
100% of their blocks, which the sibling phishing command documents ten lines
away. So the false negative was the likely case, not the exotic one. Only a 404
means "not flagged" now; isNotFound already existed in this package and the
404-versus-the-rest shape is already used elsewhere in this same file.
`mitigation-profile set --timeout` defaulted to 0, and 0 is one of the five
values the API accepts — "no delay", as the command's own refusal message says.
It was neither required nor distinguishable from an omission, and the handler
does not even receive the cobra command, so "was it typed" was unreachable. A
`set` run to change nothing else wrote "no delay" over a profile that may have
been at 1560. The flag is required now, which is what this repository already
does 39 times elsewhere for a value it cannot invent.
Three tests. Two compiling sabotages, each red on its own test.
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> # Conflicts: # internal/services/baremetal/catalog.go
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>
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/ip-incident-surface
Signed-off-by: Denis <denis.hamon@ovhcloud.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this adds
The
ipdomain exposes 15 of its 81 routes. This adds the incident surface: what blocks anaddress, how to release it, and how its anti-DDoS filter is configured.
26 routes, 22 commands. Stacked on #255; only the last commit belongs to this pull request.
What the account said
Every block of a real account, every family — 537 × 7 = 3 759 requests:
antihackarpspamphishingmitigationmitigationProfilesgameA 14-block sample taken earlier said all seven families were empty. They are not:
gameandmitigationcarry live configuration, and the reads below run against it. The zero came from thesample, not from the account.
Four things only the measurement showed
timemeans the opposite thing on one of the three mechanisms. Onantihackandarpit is"time remaining before you can request your IP to be unblocked"; on
spamit is "time whilethe IP will be blocked". One is a cooldown to wait out, the other is the sentence itself. A single
column holding both under one header would be wrong on a third of its rows and look right, so
blockedturns the number into a sentence per mechanism, andunblockrefuses locally while thecooldown runs — the API refuses it with "The requested object (ipBlocked = x) does not exist",
which reads as "unknown address" when it means "not yet".
GET /ip/{ip}/phishingreturns 500 for every address hosted outside Europe. Ten networkprefixes answer 500 on 100 % of their blocks —
139.99,148.113,149.56,15.235,158.69,51.161,51.222,51.79,2402:1f00,2607:5300. The 49 other prefixes answer 200 on 100 % oftheirs. Not one prefix is mixed, with and without the
stateandipOnAntiphishingfilters,and the four sibling families answer 200 on those very same blocks. The CLI cannot fix that, so it
reports the failure as a failure: an empty table there would say "no phishing reported" about a
block nobody managed to read. Worth a look by the route's owner.
The embedded schema is stale on game protocols.
ip.GameMitigationRuleProtocolEnumdoes notlist
arkSurvivalAscended, which the API reports insupportedProtocolsand which rule 11266524of a live address already uses. So
game rule addchecks--protocolagainst the address, notagainst the enum shipped with the CLI — validating against the schema would refuse a protocol
already in production. The lists also differ between addresses (14 protocols on one, 20 on
another), as does
maxRules(30 and 100), so there was no single right list to check againstanyway:
valheimis accepted on one address of this account and refused on another, both live.Firewall mode with no rule is a UDP blackhole. It only lets through UDP traffic matching a
rule, so turning it on with zero rules drops every UDP packet to the address — and one address of
the account measured sits in exactly that state: firewall mode off, zero rules, one flag away.
game edit --firewall-moderefuses instead, and names the command that adds a rule first.Verified against the API
Reads, on the addresses that carry configuration:
ip game list 145.239.23.144/32ip game getip game rule list123or7777-7778ip mitigation list 87.98.233.18/32--state removalPendingnarrows it to noneip mitigation-profile list 141.94.98.55/32ip phishing liston a European blockip phishing liston a Canadian blockip blockedEight refusals executed against the API: nothing to release, unknown
--reason, the UDPblackhole, an unsupported protocol, a reversed port range, a delay outside the five the API takes,
statistics for an address the anti-spam system does not know, and a
game geton an unprotectedaddress.
The writes are not demonstrable and this PR does not pretend otherwise. Releasing a block
needs an address that is blocked, and none is; the mitigation and game configuration belongs to
running services. So the eight write paths are proven by
--dry-run— each one printing themethod and path it would call and sending nothing — and the account state was read back afterwards
outside the CLI: 12 rules still,
permanentstill false, timeout still 360, firewall mode stilloff. Nothing was written.
Notes
--filteris applied after the readable columns are built, not before: filtering first makes theflag silently match nothing on the very columns the table shows, which reads as an answer.
mitigationkeeps its three verbs.POSTenrols an address,DELETEdrops it from the list,and
PUTchanges thepermanentflag of a row that stays. Collapsing them into two commandswould have made one of the three unreachable.
mitigation-profile setreads whether a profile exists and picksPOSTorPUTitself. Anoperator setting a delay does not know which of the two routes applies, and should not have to.
spam-statsdefaults its mandatory window to the date the address was blocked, read from thesibling route, and says where the bound came from. The API makes
fromandtorequired andgives no default, so forwarding them as-is makes the command unusable without two values the
operator cannot know.
Review round — second commit
An adversarial review of the first commit found seven defects, six real. Two would have shipped.
A spam block is a sentence, not a wait. The path that works the mechanism out itself passed the
block duration on as a release cooldown and refused, while
--reason spamperformed the same POSTunchanged — and the table, on the same number, printed the correct "blocked for 1d". Three readings
of one field, two of them wrong. This is the trap named at the top of this page, and the first
commit fell into it in its own code.
Deleting the last rule under firewall mode is the same UDP blackhole as enabling firewall mode
with no rule, and only the second was guarded. It was one
game rule deleteaway, on the commandan operator runs while tidying up rather than while changing protection.
Four smaller ones:
--reasonswallowed a 404 and so became the way around the refusal it exists togive; the mechanism and the address now travel in the API's spelling rather than the one typed,
because both end up in a path segment and in a comparison; the profile update treated every read
failure as "no profile yet", sending a create where an update was due and making
--dry-runnamethe wrong verb; and
--timeout 0, one of the five accepted values, announced "stays on for nodelay" — which names the setting and describes no behaviour.
One finding was rejected and measured:
averageSpamscoreis spelled exactly that way in thespecification.
Seven sabotages against the fixes, seven red.