Repository navigation
test(purchase): pin the Azure savings-plans alias to the re-drive token through the real client - #1305
Draft
IAmJSD wants to merge 1 commit into
Draft
test(purchase): pin the Azure savings-plans alias to the re-drive token through the real client#1305IAmJSD wants to merge 1 commit into
IAmJSD wants to merge 1 commit into
Conversation
…en through the real client The LeanerCloud#838 matrix (written against cloud-commitments-go ad67030387e4) says the Azure savings-plans client names its order alias from time.Now().UnixNano(), so the platform's derived idempotency token never reaches Azure. The module pinned in go.mod now (providers/azure 3def3f898617) derives the alias from the token via common.OrderIDFromToken, but nothing on the platform side pinned that: TestRedriveReusesDerivedTokensForEveryProvider stops at a mock service client and does not cover azure savings-plans. Run the platform purchase fan-out against the real pinned savings-plans client with only the order-alias API faked, and assert that each recommendation PUTs OrderIDFromToken(DeriveIdempotencyToken(lineage, index)), that a re-drive PUTs the identical names, that recommendations and lineages do not collide, and that the platform still refuses Azure savings-plans re-drive (whether Azure collapses a repeat alias PUT is server behavior no fake can prove). Tests only; no production change. Refs LeanerCloud#838 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Comment |
This branch has not been deployed
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 #838 asked, and what the pinned module actually does now
#838's matrix was written against cloud-commitments-go
ad67030387e4. Its open gap was that the Azure savings-plans client names its order alias fromtime.Now().UnixNano(), so the platform'sDeriveIdempotencyToken(lineage, index)never reaches Azure.I re-checked that against the module go.mod pins now (
providers/azure v0.0.0-20261017134002-3def3f898617). That row is out of date.services/savingsplans/client.go:376now does:So with a token (the platform's purchase path), the alias is the deterministic GUID derived from the token. The timestamp name is used only with an empty token (the CLI path), and a malformed token fails before any PUT. The module tests this with a mock in
TestPurchaseCommitment_DeterministicAliasNameFromToken.The gap in the platform: nothing here pinned it.
TestRedriveReusesDerivedTokensForEveryProviderstops at a mock service client and does not cover azure savings-plans at all. A go.mod bump that brought the timestamp alias back would pass every platform test.Change (tests only, no production change)
New file:
internal/purchase/azure_savings_plans_alias_proof_test.go, withTestAzureSavingsPlansAliasIsDerivedFromTheRedriveToken. It runsprocessPurchaseRecommendations(the platform's real per-rec fan-out and token derivation) against the real pinnedsavingsplans.Client. Only the Azure order-alias API (SetOrderAliasClient) is faked. For both dispatchable slugs (savings-plansandsavingsplans) it asserts:OrderIDFromToken(DeriveIdempotencyToken(lineage, index));cudly-<nanos>timestamp fallback;common.ServiceSavingsPlansAll, the only service type Azure routes to this client;recRedriveRefusalReasonstill refuses Azure savings-plans re-drive (see below).Why the platform keeps refusing Azure savings-plans re-drive
A deterministic alias is necessary but not sufficient. Whether Azure answers a second
PUT savingsPlanOrderAliases/{name}by returning the first order is server behavior. That could mean neither a second plan nor an error, and no fake can prove it. Neither repo has a test or a cited provider doc for it. So this PR keeps option (a) from the issue: refuse automatic re-drive and Retry for Azure savings plans. That coversallRecsSafeToRedrive, the #1668 Retry gate and the #1718armedRedriveRefusalbackstop, all of which go throughrecRedriveRefusalReason. The new test fails if someone relaxes that as a side effect.Remaining exposure, unchanged: a first attempt whose response is lost. The row fails, and a human who then starts a new purchase gets a new lineage, so a new alias and a second plan. That is a legitimate new buy from the platform's point of view.
Proposed cloud-commitments-go change (not made here; go.mod untouched)
This would let the owner choose option (b) safely. In
providers/azure/services/savingsplans/client.goPurchaseCommitment, whenopts.IdempotencyToken != "", first callGET savingsPlanOrderAliases/{aliasName}. That isarmbillingbenefits.SavingsPlanOrderAliasClient.Get, which exists in the pinnedarmbillingbenefits v1.0.0; addGetto theSavingsPlanOrderAliasAPIinterface andrealOrderAliasClient.DoIdempotentPurchaseTwoSteplookup.BeginCreate.Add module tests for each branch, including "existing alias, so BeginCreate is never called". After that lands and go.mod is bumped, the platform can drop the
azure/savingsplanscase inrecRedriveRefusalReason. It would update the existing guard tests (TestRedriveGuardRefusalSetIsExactlyTheDispatchableSpellings,TestRedriveRefusedWhereNoProviderGuardExists) and the last assertion of the new test.Matrix re-verified at the current pins
The pins are
providers/aws aeb8a6f7ff51,providers/azureandproviders/gcp 3def3f898617, andpkg 541e05e5fd29. I read the module source in the Go module cache.CreateSavingsPlanInput.ClientToken(savingsplans/client.go:250)TestClient_PurchaseCommitment_SetsClientTokenForIdempotencyIdempotentReservationID+idempotencyGuard/recoverAlreadyExists(e.g. rds/client.go:150,173)*Idempotent*(5 tests), pluspurchase_empty_token_test.goper servicefindRIByIdempotencyTokenbefore buying (ec2/client.go:145,226)guardAgainstRepurchasematches an owned node on offering ID, node count and start time (redshift/client.go:113-139)*Idempotent*reservations.DoIdempotentPurchaseTwoStep(internal/reservations/purchase.go:679)TestFindReservationOrderByIdempotencyToken_*etc.OrderIDFromToken(token)(savingsplans/client.go:376); server-side collapse of a repeat PUT unprovenTestPurchaseCommitment_DeterministicAliasNameFromToken(mock); now also pinned hereRequestIdfromOrderIDFromToken(token)+ deterministic name (computeengine/client.go:829,873-878)TestComputeEngineClient_PurchaseCommitment_IdempotentReDrivePurchaseCommitmentreturns an error (no purchase API)Owner decisions left open (why this is Refs, not Closes)
CreateTagsafter the buy, so a crash between the buy and the tag still leaves an untagged RI. That argues for keeping the refusal there too.Deploy
None needed. This PR is tests only, and the platform behavior is unchanged. The Azure/GCP deploy pause since 2026-10-20 does not affect it.
Coordination with in-flight work
This PR adds one new test file and touches no production lines. Our
fix/issue-243branch already rewrites the stale "UnixNano" comments ininternal/purchase/manager.go(theRedriveRefusalReason/recRedriveRefusalReason/safeFaildoc comments), so I deliberately left them alone here to avoid a conflict. It does not overlap #1111, #1245 or fix/issue-532/-54/-361.Refs #838
Verification
All of the following is unit, mock or fixture evidence. Nothing calls Azure or makes a purchase. The Azure order-alias API is a fake recorder.
Env:
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true, Go 1.26.9, Linux.go build ./...,go vet ./...,go vet -tags integration ./...: OKgo test ./internal/purchase/ -count=1: ok (32.6s)go test ./internal/mocks/ -count=1: okgolangci-lint run --allow-parallel-runners ./internal/purchase/(v2.10.1): 0 issuesgofmt -l internal/purchase/: clean.GOWORK=off go mod tidy -diff: clean (go.mod/go.sum unchanged;armbillingbenefitswas already a direct require).Fail-before and pass-after, by mutation. The test pins existing behavior, so "before" means a regression of each link in the chain:
cudly-<UnixNano>alias, wired in throughGOWORK=off go test -modfile=<tmp>/go.modwith areplace; the repo's go.mod is untouched): FAIL. Expectedc505bf94-f62f-1682-6300-54342dcab999, gotcudly-1792744476476032937; the re-drive alias differs; "alias … is the timestamp fallback". Both subtests fail.execution.goderives the token fromexec.ExecutionIDinstead ofidempotencyLineageKey(exec)): FAIL, "each purchase PUTs the alias derived from DeriveIdempotencyToken(lineage, index)".recRedriveRefusalReasonazure savings-plans case disabled): FAIL, "Azure savings-plans re-drive must stay refused until a repeat alias PUT is proven to dedupe".go test ./internal/purchase/ -count=1 -run TestAzureSavingsPlansAliasIsDerivedFromTheRedriveToken -v: PASS (both subtests).Combined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree:
queue/platform@b56afb2b= origin/maina14dc8f2plus these heads, merged in this order:fix/issue-324andfix/issue-204both edit.github/workflows/ci.ymlnext to theazure-kv-access-policyjob. The tree kept #324's new "Run Function URL reader self-tests" step and #204's updated job comment; whichever PR merges second takes that same resolution.Linux, Go 1.26.9,
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true; Docker Postgres via testcontainers. The full suite ran oned0c7174, which differs fromb56afb2bonly by one.gitallowedline on fix/issue-324:go build ./...,go vet ./...,go vet -tags integration ./...and the fullgo test -race -short -count=1 ./....--build-tags integration.gocyclo -over 10is silent on the changed non-test files.-tags integration -count=1, 60-minute local timeout):internal/api,internal/purchase,internal/schedulerandinternal/serverall pass..gitallowedline. Re-run atb56afb2b: git-secrets, using the repo'sscripts/setup-git-secrets.shpatterns in a fresh repo with an isolated HOME, found nothing in the changed files, and a planted fake key was flagged.scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.Out of scope
Get(above) belongs in cloud-commitments-go.Maintainer: please mirror #838's triage labels onto this PR:
priority/p2,severity/medium,type/chore,triaged,urgency/this-quarter,impact/few,effort/m.🤖 Generated with Claude Code