Skip to content

fix(aws): payment-option/engine/term correctness gaps in AWS RI service clients #5

Description

@cristim

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.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    effort/mDaysimpact/manyAffects most userspr-createdA PR has been opened for this issue (dedup guard for the auto-PR loop)pr-mergedThe PR for this issue has been mergedpriority/p1Next up; this sprintseverity/highSignificant harmtriagedItem has been triagedtype/bugDefecturgency/this-sprintWithin the current sprint

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions