[cristim]

Summary

#451, #455 and #456 made the EC2 RI exchange lookups check every offering AWS returns against the request, using offeringMismatch in providers/aws/services/ec2/offering_match.go. The purchase path did not. scanEC2OfferingPage and findOffering checked only the zone and the payment option of the offering they picked. PurchaseCommitment, ValidateOffering and GetOfferingDetails all use findOffering.

This PR applies the same rule to the purchase path.

• A new ec2OfferingQuery.want() maps the purchase query (instance type, product, tenancy, scope, duration, offering class, payment option, AZ) onto offeringWant.
• scanEC2OfferingPage now calls offeringMismatch(o, q.want()), which uses the tiered rule:
  • a zero want field is not checked;
  • class, tenancy, duration and payment option are strict, so an offering missing one of them is skipped;
  • instance type and scope are compared only when the offering sets them;
  • product is compared when both sides are set, ignoring a trailing " (Amazon VPC)".
• findOffering appends per-reason skip counts to its not-found and page-cap errors, e.g. "3 offerings skipped: class=2, tenancy=1". The existing error text is kept.
• The request (describeInputFromQuery) is unchanged. It already sends tenancy, class, duration, product, instance type, payment option and AZ as typed fields, so this is a second line of defence against an offering AWS returns that does not match. A test now pins that request shape.

Effect: a wrong-tenancy, wrong-class, wrong-duration or wrong-product offering is skipped instead of purchased. If no offering matches, the purchase fails loud with the skip counts.

Live gap: this has not been checked against live AWS, i.e. that it always returns OfferingClass, InstanceTenancy and Duration on offerings. If AWS omitted one, purchases would fail loud with "class=N" in the error rather than buy the wrong thing. No AWS call was made; this repo does not run cloud-backed checks unless they are explicitly intended. The exchange lookups already depend on the same fields.

ValidateOffering and GetOfferingDetails keep their documented convertible default. Their only change is that a mismatched offering now errors.

Closes #457 (follow-up to #233; refs #305).

Verification (macOS, Go 1.26.9)

• (cd providers/aws && GOWORK=off go test -race -short ./...) passes. make build, make vet pass. golangci-lint v2.10.1 (the CI pin) reports 0 issues. gofmt is clean.
• New tests in purchase_offering_match_test.go:
  • TestPurchaseCommitment_WrongOfferingIsNotPurchased: tenancy, class, duration and product subtests through PurchaseCommitment with a mock that returns only the wrong offering. Each asserts the error and AssertNotCalled on PurchaseReservedInstancesOffering.
  • TestFindOffering_SkipsWrongTenancyThenPicksMatch.
  • TestFindOffering_NotFoundReportsSkips and TestFindOffering_PaginationCapReportsSkips.
  • TestFindOffering_VPCSuffixedProductStillMatches.
  • TestScanEC2OfferingPage_CountsSkipsByReason.
  • TestEC2OfferingQueryWant, against independent literals.
  • TestDescribeInputFromQuery_SendsTenancyClassDurationProduct.
• Fixtures: about a dozen existing fixtures gained only the fields the rec already implies, mostly OfferingClass: convertible, with no assertion edited. TestFindOffering_WrongVariantRejected now also asserts offering-type=1. The duration-vs-term price test gained an idOnly column, because findOffering now rejects that offering first and the price guard in offeringprice.Price stays reachable through the by-id re-describe.
• Fail-before: on the original client.go, the four purchase subtests fail, as do the skip-count error tests, SkipsWrongTenancyThenPicksMatch and WrongVariantRejected.
• Mutation probes run by the independent reviewer in scratch copies all fail tests:
  • dropping the mismatch call;
  • making class or tenancy lenient;
  • dropping the skip note;
  • dropping the VPC-suffix stripping;
  • a nil skips map (panics).
• Review: two plan passes and two diff passes, all by the same reviewer, who is not the author. Diff pass 1 added the purchase-path VPC-suffix test and the rename. No actionable findings on 56c8786.

Fakes only; no AWS call or purchase.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 6 billable files and costs up to $1.50.}

• Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Or wait 2 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available. Your 68 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-go/.coderabbit.yaml
• Review profile: CHILL
• Plan: Essentials
• Run ID: 62adb76d-87ac-47cc-85f6-3507fc59c1dc

📥 Commits

Reviewing files that changed from the base of the PR and between 011be8f and f194876.

📒 Files selected for processing: 6

• providers/aws/services/ec2/client.go
• providers/aws/services/ec2/client_test.go
• providers/aws/services/ec2/offering_match.go
• providers/aws/services/ec2/offering_price_test.go
• providers/aws/services/ec2/purchase_offering_match_test.go
• providers/aws/services/ec2/purchase_retry_test.go

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[cristim]

@coderabbitai review

[coderabbitai]

⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

Independent adversarial review: CLEAN at 56c8786a3c0d20ccc0698f3cb93bd3110f2c4366

Reviewer: an independent agent that did not write this change (Sonnet, high effort), cold brief, full diff against main, with the review focused on the money path. The PR body was treated as a claim.

Blocking findings: none.

What was attacked:

• Zero-value hazard. Traced from PurchaseCommitment (client.go:164) through buildEC2QueryFromRec (client.go:489-509) to findOffering. Tenancy, scope, platform and payment option error when empty or invalid. The term maps to a non-zero duration, and the offering class is resolved at client.go:533. On the real path, want() leaves no checked field zero.
• SDK v2 types.ReservedInstancesOffering (types.go:16675-16728): a nil Duration reads as 0 and mismatches. An unset InstanceTenancy, OfferingClass or OfferingType mismatches under the strict checks. A missing AZ is rejected for zonal queries.
• Product. The (Amazon VPC) suffix is trimmed on both sides. Distinct products (Windows vs Windows with SQL Server, RHEL vs RHEL with HA) still mismatch.
• Callers and error text. Every path through ValidateOffering and GetOfferingDetails uses findOffering. No in-repo code matches on these error strings, and the skip note is a suffix.
• Fixtures. They only gained the fields the rec implies, and no assertion was weakened. The idOnly price case still reaches the "does not match" guard.
• Mutation probes (scratch copy, reverted). Each of these made tests fail:
  • neutralizing the offeringMismatch call;
  • making tenancy lenient;
  • dropping duration from want().

Non-blocking notes (none can cause a wrong purchase on the real path):

1. Instance type, scope and product are skipped when the offering leaves them empty. The request filters on instance type and scope, and the SDK populates both.
2. offeringMismatch never checks Marketplace. Third-party offerings are excluded by IncludeMarketplace=false on the request (client.go, about line 472).
3. ValidateOffering and GetOfferingDetails hard-code the convertible class (client.go:615, 626). This predates the PR.

Local verification (macOS, Go 1.27.2, exact SHA 56c8786)

Fixture and mock evidence (mocked EC2 API; no AWS call, no purchase):

• Fail-before. In a scratch copy, the only change was scanEC2OfferingPage matching against an empty offeringWant{}, which restores the old no-check behaviour. These fail:

  • TestPurchaseCommitment_WrongOfferingIsNotPurchased, subtests tenancy, duration, product and class. Each asserts that PurchaseReservedInstancesOffering is not called.
  • TestFindOffering_SkipsWrongTenancyThenPicksMatch.

  The reviewer separately restored main's client.go and offering_match.go, and the purchase and skip-count tests failed there too.

• Pass-after. (cd providers/aws && GOWORK=off go test -race -short ./...) exits 0 with 14 packages ok. make build and make vet pass.

• Real-account gap. This is not checked against live AWS, in particular whether AWS always returns OfferingClass, InstanceTenancy and Duration on offerings. If it did not, a purchase would fail loud with skip counts, not buy the wrong offering. A live check is not required to merge per the review gate.

Labels mirror #457. CodeRabbit is rate-limited on this PR ("Review limit reached"; an on-demand review is billed, so it was not run). If that still holds at merge: CR waived: quota, adversarial review + local verification + green CI.

[cristim]

CI is green on head 56c8786: pre-commit 38968918398 and Build & Test 38968918479.

[cristim]

Rebased onto main 011be8f (#467 and #468, both Azure-only). New head f194876c782f1dcfbe1254674e0f2c49bcf6c3cd.

git range-diff 4453ddd..56c8786 origin/main..f194876:

1:  56c8786 = 1:  f194876 fix(aws): verify the offering the EC2 RI purchase path picks

The commit is identical, so the CLEAN verdict for 56c8786 carries over to f194876. CI had passed on 56c8786 (runs 38968918398, 38968918479). On the new head, make build passes and the ec2 tests are ok, and CI is re-running. CR waived: quota, adversarial review + local verification + green CI.

[cristim]

Merged by squash at reviewed head f194876c782f1dcfbe1254674e0f2c49bcf6c3cd as 01f1c76. Gates on that SHA:

• Independent adversarial review CLEAN for 56c8786, carried to f194876 by an identical git range-diff.
• Local mock verification with fail-before (all four wrong-offering purchase subtests) and pass-after.
• CI green: pre-commit 38971127272, Build & Test 38971127507.
• Labels mirrored from fix(aws): verify the offering the EC2 RI purchase path picks, as the exchange lookups now do #457. No unresolved threads.

CR waived: quota, adversarial review + local verification + green CI. Live-AWS gap noted in the verdict. Retrospective CodeRabbit review to run when quota returns.