fix(aws): payment-option/engine/term correctness gaps in AWS RI service clients - #1075
Merged
cristim merged 2 commits intoJun 7, 2026
Merged
Conversation
…hape (08-H2) Redshift offering selection matched only node type and duration and accepted any Regular/Upgradable offering, discarding the requested payment option. A no-upfront recommendation could therefore be filled with an all-upfront offering (or vice versa), buying on payment terms the operator never approved and breaking the OnDemand/savings math shown in the UI. Redshift does not encode the payment option in the Regular/Upgradable ReservedNodeOfferingType enum; it is expressed through the offering's FixedPrice (upfront) and recurring charge. scanRedshiftOfferingPage now matches the offering's price shape against the requested option (all-upfront = upfront only, no-upfront = recurring only, partial-upfront = both) and skips offerings that do not match. An empty/unknown requested option matches nothing, so a malformed recommendation errors with "no offerings found" instead of buying arbitrarily. GetOfferingDetails now reports the derived payment option (the price shape) instead of the Regular/Upgradable enum, so callers can reconcile the bought terms. matchesOfferingType drops its ignored payment-option parameter (the enum-validity check is now orthogonal to payment matching). Regression tests: TestFindOfferingID_PaymentOptionMustMatch (picks the offering matching the requested option, errors when none matches; fails pre-fix by returning the wrong-payment offering), TestMatchesPaymentOption, TestDerivePaymentOption. Existing fixtures updated to carry the price shape matching their declared payment option.
…(08-H4) GetAllRecommendations tolerated per-service failures and merged the survivors, but when EVERY service errored (e.g. a sustained Cost Explorer throttle that exhausts each service's per-combo retries) mergeServiceResults logged each failure at WARN and returned an empty slice with a nil error. A fully-throttled run was then indistinguishable from "no savings available", which an operator can misread as "nothing to buy". mergeServiceResults now returns ([]Recommendation, error) and yields a wrapped error only when all services failed, while still tolerating partial failure (nil error plus the surviving services' recs). The signature of the exported GetAllRecommendations is unchanged, so the pkg/provider interface and the scheduler callers are unaffected; the scheduler already propagates a non-nil error, so a totally-failed run now aborts loudly instead of recording an empty result as success. Regression test TestMergeServiceResults_AllFailIsError pins the new contract (all-fail -> error, partial-fail -> survivors with nil error, no-fail empty -> nil error). Tested at the merge function directly rather than through GetAllRecommendations because the six concurrent goroutines share one *RateLimiter (the out-of-scope 08-C1 race).
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR fixes two HIGH-severity money-path correctness gaps in AWS RI clients. The recommendations client now distinguishes "fully failed services" from "no recommendations available" instead of silently dropping all results when throttled. Redshift offering selection validates payment options by matching offering price shapes (FixedPrice and RecurringCharges) instead of discarding the payment-option parameter, and surfaces the matched payment option in GetOfferingDetails. ChangesRecommendations client: error propagation for fully-failed services
Redshift: payment option matching by price shape instead of offering type
Possibly related PRs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
cristim
added a commit
that referenced
this pull request
Jun 7, 2026
…ilent defaults (#1085) * fix(aws/savingsplans): error on unknown payment-option/term, no silent default convertPaymentOption and convertTermToSeconds previously logged a warning and silently substituted All Upfront (largest cash outlay) or 1 year when given an unrecognized or empty value, risking a mis-purchase on any typo or new/renamed AWS enum value. Both functions now return an explicit error on unrecognized input so the purchase and offering-lookup paths fail loud instead of buying the wrong product. Mirrors the pattern already used by the RDS convertPaymentOption. Regression tests for H1 (unknown/empty payment option) and H2 (unknown/empty term) are added to TestClient_FindOfferingID_AllPaymentOptions and TestClient_FindOfferingID_TermVariations; the previously-passing "default term" case is updated to assert the error path. Fixes H1 and H2 from docs/code-review/19-hardcoded-fallbacks-aws.md (PR #1039). * fix(aws): fail loud on unknown payment-option/term/engine Fixes H3, H4, M1-M6, L1, L2 from the 19-hardcoded-fallbacks-aws.md audit plus H1/H2 from the now-merged #1039 branch (savingsplans). H3 (converters.go): add convertPaymentOptionE/convertTermInYearsE/ convertLookbackPeriodE that error on unrecognized input; wire into the SP recommendation path (parser_sp.go). The legacy non-erroring wrappers are retained for the RI path in client.go (owned by #865/#1075) with a deprecation comment and a follow-up reference. H4 (elasticache/client.go): convertPaymentOption now returns (string, error) mirroring the RDS sibling that already errors correctly. H1/H2 (savingsplans/client.go): convertPaymentOption and convertTermToSeconds now return (type, error); cherry-picked from the merged #1039 branch where the fix was staged. M4 (parser_services.go, rds/client.go): parseRDSDetails no longer silently defaults AZConfig to "single-az" when CE omits DeploymentOption; findOfferingID rejects empty AZConfig with an explicit error. M5 (parser_services.go): resolveEC2Tenancy now maps only the known CE values ("shared"/nil -> default, "dedicated" -> dedicated); any other value (e.g. "host" for Dedicated Hosts) returns an error rather than silently collapsing to default tenancy. M6 (rds/client.go): normalizeEngineName now returns (string, error) and errors for ambiguous Oracle ("oracle*"), SQL Server ("sqlserver*"/"sql-server*"), and bare Aurora inputs; unambiguous engines (mysql, postgresql, mariadb, aurora-mysql, aurora-postgresql) pass through unchanged. M1/M2/M3 (ec2/client.go): replace raw "convertible" literals with types.OfferingClassTypeConvertible; buildEC2OfferingQuery errors on empty Platform (purchase path); centralize "Linux/UNIX" literal into defaultEC2Platform constant (exchange-path helpers retain their safe fallback; the purchase path does not). Regression tests added for H3, H1/H2, H4, M2, M4, M5, M6, L1, L2; each fails pre-fix and passes post-fix. Closes #1084 * test(aws/rds): clarify ambiguous-engine regression test Simplify TestNormalizeEngineName_AmbiguousErrors: remove the confusing oracle-se2 case (strings.Contains("oracle-se2","oracle") is true, so it errors like all other oracle inputs -- no special case needed) and remove the misleading inline comment. All remaining cases assert that any Oracle, SQL Server, or bare Aurora input errors, which is the invariant under test. * refactor(aws): extract helpers to fix gocyclo violations in rds and savingsplans gocyclo -over 10 fails on two functions added by the fail-loud refactor: - savingsplans.(*Client).findOfferingID (complexity 12): extract resolveSPPlanType and buildSPOfferingsInput helpers - rds.(*Client).paginateRDSOfferings (complexity 11): extract fetchRDSOfferingPage to isolate the per-page API call and scan Also apply gofmt to rds/client.go (trailing whitespace / brace style). Behaviour and error paths are unchanged; all rds and savingsplans tests still pass. * fix(aws/rds): tighten AZ-config and engine-name validation (CR #1085) Three related fail-loud fixes in the AWS RDS purchase path: 1. parseRDSDetails: unknown DeploymentOption now errors instead of silently folding into "single-az". Only "Multi-AZ" and "Single-AZ" are accepted; any other non-nil value (e.g. "Multi-AZ-Readable-Standbys") returns an explicit error so the bad token surfaces rather than driving findOfferingID to the wrong RI class. 2. findOfferingID: AZConfig validation now uses a switch with an explicit default error case rather than only checking for empty string. A non-empty but invalid value (typo, future AWS enum addition) would previously fall through to multiAZ==false in paginateRDSOfferings, silently treating the bad input as single-AZ. 3. normalizeEngineName: Oracle and SQL Server checks changed from strings.Contains to == so that edition-qualified tokens (oracle-se2, oracle-ee, sqlserver-se, sqlserver-web, etc.) pass through as valid RDS ProductDescription values. The previous Contains check contradicted the error message itself, which told callers to supply those exact edition tokens. Regression tests added for all three: each new test was verified to fail against the pre-fix code and pass after.
This was referenced Jul 27, 2026
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
1 participant
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
Fixes two untracked HIGH-severity correctness gaps on the AWS RI purchase /
money-moving path, from the adversarial review in
docs/code-review/08-provider-aws.md. These were not captured by the existingtracking issues (#1012-#1035 filed only 08-C1 and 08-H1).
08-H2 — Redshift offering selection ignored the requested payment option
providers/aws/services/redshift/client.goscanRedshiftOfferingPagematched only node type + duration and accepted anyRegular/Upgradableoffering, discarding the requested payment option. Ano-upfrontrecommendation could be filled with anall-upfrontoffering (orvice versa), committing the buyer to terms they never approved and breaking the
OnDemand/savings math in the UI.
Redshift encodes the payment option through the offering's
FixedPrice(upfront)and recurring charge, not the
Regular/Upgradableenum. The scan now matchesthe offering's price shape against the requested option:
An empty/unknown option matches nothing, so a malformed recommendation errors
with "no offerings found" rather than buying on an arbitrary option.
GetOfferingDetailsnow reports the derived payment option so callers canreconcile the bought terms.
08-H4 — Per-service throttle could swallow a fully-throttled service as "no recs"
providers/aws/recommendations/client.goGetAllRecommendationstolerated per-service failures and merged survivors, butwhen EVERY service errored (a sustained Cost Explorer throttle exhausting each
service's per-combo retries)
mergeServiceResultslogged each failure at WARNand returned an empty slice with a nil error. A throttled run was then
indistinguishable from "no savings available".
mergeServiceResultsnow returns([]Recommendation, error)and yields anerror only when all services failed, still tolerating partial failure. The
exported
GetAllRecommendationssignature is unchanged (nopkg/providerinterface or scheduler churn); the scheduler already propagates a non-nil error,
so a totally-failed run aborts loudly instead of recording empty-as-success.
Scope note
base branch (
convertPaymentOptionreturns(string, error)), so it is notre-implemented here.
RateLimiter race) are deferred to keep this PR on the HIGH money-path fixes and
under the ~400-line review budget, per
16-remaining-findings-plan.md.Tests
Regression tests that fail pre-fix:
TestFindOfferingID_PaymentOptionMustMatch(08-H2): asserts the offeringmatching the requested payment option is selected and that a mismatch errors;
verified to FAIL against the pre-fix scan (returned the wrong-payment offering).
TestMatchesPaymentOption,TestDerivePaymentOption(08-H2).TestMergeServiceResults_AllFailIsError(08-H4): all-fail -> error,partial-fail -> survivors + nil error, empty-success -> nil error.
Verification
go build ./...(aws module + repo root): success.go vet ./services/redshift/... ./recommendations/...: no issues.go test ./services/redshift/... ./recommendations/...: 368 pass.go test -race. The package's pre-existing-racefinding is 08-C1 (shared RateLimiter in the concurrentGetAllRecommendationspath), out of scope here.providers/aws/services/savingsplans/client_test.go:991(a 2-argfindOfferingIDcall against a now-3-arg signature) exists on the base branch;that file is the FOLD-1039 bucket, untouched here.
Closes #1052
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests