Repository navigation
Fix: Refuse a non-USD unit on a pricing block for every endpoint - #1242
Conversation
After rossoctl#1153 a unit is host-wide: Table.unitFor gives an endpoint the unit of its best-ranked row, and only rows in that unit may price its traffic. A block with a unit and no hosts, or with "*" among them, covers every endpoint, so every endpoint no more specific block names took that unit. The bundled dollar table then priced nothing there, which is loud: unpricedBy names every pair. The other half was silent: a charge any of those endpoints reported itself was labelled in the unit. Measured with hosts ["*"], unit credits and the bundled table on, api.anthropic.com/claude-opus-4-1 went from unit=USD prov=bundled to unit=credits prov=none. Config.entries now refuses that block at startup, beside the multiplier-only refusal, naming the block. A unit is a gateway's, so its hosts can be named. Any spelling of USD is still accepted, as it is no unit claim. TestConfig_BlocksForOneHostMustAgreeOnTheUnit loses its "any-endpoint spelled two ways" case: a catch-all can no longer carry a unit, so two spellings of it always agree, and the new refusal fires first. docs/pricing.md lists the refusal beside the other two. Fixes rossoctl#1190. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 48 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mrsabath
left a comment
There was a problem hiding this comment.
Verified the mechanism this guards against: Table.unitFor ranks provenance first, so a configured catch-all row with a unit does outrank the bundled USD rows and would relabel every endpoint no more specific block names — the failure in #1190 is real, not theoretical.
The check is correctly placed after the len(ep.Models) == 0 branch, so it cannot collide with the multiplier-only refusal (that path continues first). slices.ContainsFunc over the normalised hosts catches both the hostless case and "*" anywhere in the list, and EqualFold against CurrencyUSD after normaliseUnit means no spelling of the default is treated as a unit claim. Echoing ep.Unit rather than the normalised value in the error is the right call — it shows the operator what they typed.
Confirmed the "unaffected" claim: cmd/cortex/local.go and cmd_config_migrate_pricing.go both name api.us-east.bob.ibm.com, and the only unit: example in docs/pricing.md names a concrete host — so nothing shipped or documented regresses. The removed "any-endpoint spelled two ways" case is genuinely subsumed, and it left a pointer to the new test behind rather than vanishing. The mutation check in the test plan (a len(ep.Hosts) == 0-only predicate failing the ["*"] cases) is exactly the right thing to verify for this predicate.
No must-fix issues, and nothing I would raise as a suggestion either.
Areas reviewed: Go (core/cost/pricing), tests, docs
Commits: 1, signed off
CI status: passing (28 checks, Spellcheck skipped)
Summary
Fixes #1190. A pricing block with a non-USD
unit:and nohosts:(or"*"among them) now fails startup, beside the existing refusal for a unit on a multiplier-only block.Why: after #1153 a unit is host-wide.
Table.unitForgives an endpoint the unit of its best-ranked row, and only rows in that unit may price its traffic. So a catch-all block with a unit puts every endpoint that no more specific block names into that unit:unpricedBynames every pair;Measured in the issue at 80348bf with
hosts: ["*"], unit: creditsand the bundled table on:api.anthropic.com claude-opus-4-1went fromunit=USD prov=bundledtounit=credits prov=none.A unit belongs to one gateway, so that gateway's hosts can be named. The error names the block:
Changes
core/cost/pricing/config.go:Config.entriesrefuses a block whose unit is not USD and whose hosts include a catch-all (anyHost:""or"*"). The check covers a catch-all anywhere in the list, since["gw.bob", "*"]still covers every endpoint. Any spelling of USD ("",usd) is accepted, because it is no unit claim. This matches the multiplier-only refusal.core/cost/pricing/config_test.go:TestConfig_AUnitOnACatchAllBlockIsRefused, a sibling ofTestConfig_AUnitOnAMultiplierOnlyBlockIsRefused:["*"],["gw.bob", "*"];""/usd,["*"]withUSD, a named gateway withcredits."any-endpoint spelled two ways"case fromTestConfig_BlocksForOneHostMustAgreeOnTheUnit. It paired a hostless credits block with a"*"USD one. Since a catch-all can no longer carry a unit, two spellings of it always agree, and the new refusal fires first. A comment on the test now points at the new one.docs/pricing.md: the Billing units section lists this refusal beside the other two.Unaffected: the built-in local config's Bob entry (
cmd/cortex/local.go) andagentop config migrate-pricingboth nameapi.us-east.bob.ibm.com.Test plan
len(ep.Hosts) == 0) fails the["*"]and["gw.bob", "*"]cases.go test ./...incore;GOWORK=off go test ./...incmd/cortexandcmd/agentop(withSSL_CERT_FILEunset).go vet ./cost/...andgofmt -l ./costclean.Assisted-By: Claude (Anthropic AI) noreply@anthropic.com