Problem (money path)
Adversarial review of providers/aws/ (report docs/code-review/08-provider-aws.md)
surfaced three HIGH-severity correctness gaps in the AWS RI service clients that
sit directly on the purchase / money-moving path. None were captured by the
existing tracking issues (LeanerCloud/cloud-commitments-cli#1012–LeanerCloud/cloud-commitments-cli#1035 only filed 08-C1 and 08-H1).
08-H2 — Redshift offering selection ignores the requested payment option
providers/aws/services/redshift/client.go (matchesOfferingType, findOfferingID)
matchesOfferingType returns true for any offering whose
ReservedNodeOfferingType is Regular/Upgradable and discards the
paymentOption argument entirely. So a no-upfront recommendation can be
filled with an all-upfront offering (or vice-versa) — a different price and
cash-flow profile than the operator approved. The OnDemand/savings math in the
UI no longer matches what was bought.
- Fix: select the offering whose price shape matches the requested payment option
(Redshift encodes payment via FixedPrice + RecurringCharges, not the
Regular/Upgradable enum): all-upfront = upfront>0 & recurring==0;
no-upfront = upfront==0 & recurring>0; partial-upfront = upfront>0 &
recurring>0. Surface the chosen payment option in GetOfferingDetails.
08-H3 — ElastiCache silently defaults an unknown payment option to Partial Upfront
providers/aws/services/elasticache/client.go (convertPaymentOption)
- The
default branch returns "Partial Upfront" (no error, no log). An empty
or malformed rec.PaymentOption therefore buys a Partial-Upfront reservation
rather than failing. RDS's equivalent (services/rds/client.go) correctly
returns an error for unknown options.
- Fix: mirror RDS — return
(string, error) and propagate the error up through
findOfferingID / PurchaseCommitment.
08-H4 — Per-service throttle can swallow a fully-throttled service as "no recs"
providers/aws/recommendations/client.go (GetRecommendationsForService,
mergeServiceResults, GetAllRecommendations)
- The (term × payment) sweep tolerates per-combo errors and only fails when
successCount == 0. A sustained throttle makes every combo fail; the wrapped
lastErr then reaches mergeServiceResults, which logs it at WARN and drops
that whole service's recommendations, returning the others as if the run
succeeded. A throttled run is indistinguishable from "no savings available".
- Fix: distinguish "no recommendations" from "all attempts errored" at the
GetAllRecommendations level so the caller can tell a fully-failed service
from an empty one.
Files
providers/aws/services/redshift/client.go
providers/aws/services/elasticache/client.go
providers/aws/recommendations/client.go
Findings
08-H2, 08-H3, 08-H4 (the untracked HIGH set from report 08).
Scope note: report 08 also lists Medium/Low/Nit AWS findings (08-M2..M7,
08-L2/L4/L5, 08-N1/N5). Those are deferred to a follow-up PR to keep this one
on the four-High money-path fixes and under the ~400-line review budget, per
the split guidance in 16-remaining-findings-plan.md.
Problem (money path)
Adversarial review of
providers/aws/(reportdocs/code-review/08-provider-aws.md)surfaced three HIGH-severity correctness gaps in the AWS RI service clients that
sit directly on the purchase / money-moving path. None were captured by the
existing tracking issues (LeanerCloud/cloud-commitments-cli#1012–LeanerCloud/cloud-commitments-cli#1035 only filed 08-C1 and 08-H1).
08-H2 — Redshift offering selection ignores the requested payment option
providers/aws/services/redshift/client.go(matchesOfferingType,findOfferingID)matchesOfferingTypereturnstruefor any offering whoseReservedNodeOfferingTypeisRegular/Upgradableand discards thepaymentOptionargument entirely. So ano-upfrontrecommendation can befilled with an
all-upfrontoffering (or vice-versa) — a different price andcash-flow profile than the operator approved. The OnDemand/savings math in the
UI no longer matches what was bought.
(Redshift encodes payment via
FixedPrice+RecurringCharges, not theRegular/Upgradableenum): all-upfront = upfront>0 & recurring==0;no-upfront = upfront==0 & recurring>0; partial-upfront = upfront>0 &
recurring>0. Surface the chosen payment option in
GetOfferingDetails.08-H3 — ElastiCache silently defaults an unknown payment option to Partial Upfront
providers/aws/services/elasticache/client.go(convertPaymentOption)defaultbranch returns"Partial Upfront"(no error, no log). An emptyor malformed
rec.PaymentOptiontherefore buys a Partial-Upfront reservationrather than failing. RDS's equivalent (
services/rds/client.go) correctlyreturns an error for unknown options.
(string, error)and propagate the error up throughfindOfferingID/PurchaseCommitment.08-H4 — Per-service throttle can swallow a fully-throttled service as "no recs"
providers/aws/recommendations/client.go(GetRecommendationsForService,mergeServiceResults,GetAllRecommendations)successCount == 0. A sustained throttle makes every combo fail; the wrappedlastErrthen reachesmergeServiceResults, which logs it at WARN and dropsthat whole service's recommendations, returning the others as if the run
succeeded. A throttled run is indistinguishable from "no savings available".
GetAllRecommendationslevel so the caller can tell a fully-failed servicefrom an empty one.
Files
providers/aws/services/redshift/client.goproviders/aws/services/elasticache/client.goproviders/aws/recommendations/client.goFindings
08-H2, 08-H3, 08-H4 (the untracked HIGH set from report 08).