Repository navigation
fix(providers/aws): fail loud on unrecognized RI term strings - #1207
Conversation
cristim
added
triaged
|
@coderabbitai review |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 33 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughFive AWS RI service clients (ElastiCache, MemoryDB, RDS, OpenSearch, Redshift) stop silently defaulting to a 1-year duration. Term-to-duration helpers now return (value, error) and offering-selection paths propagate errors to abort on unsupported/empty reservation terms. ChangesStrict term validation across AWS RI service clients
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
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 unit tests (beta)
Comment |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
providers/aws/services/elasticache/client_test.go (1)
398-400: ⚡ Quick winGuard the error-message assertion to avoid panic on assertion failure.
assert.Error(t, err)does not stop execution; if it fails,err.Error()can panic and hide the real test failure. Gate the contains check on the assert result (or userequire.Error).Proposed test hardening
- if tt.expectErr { - assert.Error(t, err) - assert.Contains(t, err.Error(), "unsupported ElastiCache reservation term") - return - } + if tt.expectErr { + if assert.Error(t, err) { + assert.Contains(t, err.Error(), "unsupported ElastiCache reservation term") + } + return + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@providers/aws/services/elasticache/client_test.go` around lines 398 - 400, The test currently calls assert.Error(t, err) and then unconditionally uses err.Error(), which can panic if the first assertion fails; either change assert.Error to require.Error to stop execution on failure, or capture the boolean result (e.g., ok := assert.Error(t, err)) and only call assert.Contains(t, err.Error(), "unsupported ElastiCache reservation term") when ok is true; apply this change around the assertion lines that perform the error check and contains assertion in the ElastiCache client tests.providers/aws/services/rds/client_test.go (1)
560-595: ⚡ Quick winAdd a call-path regression test for invalid term short-circuiting.
TestClient_GetDurationStringvalidates the helper, but the ARCH-04 risk is in the offering lookup path. Add afindOfferingIDinvalid-term test that asserts noDescribeReservedDBInstancesOfferingscall occurs.Proposed test addition
+func TestFindOfferingID_InvalidTerm_ErrorsBeforeAPICall(t *testing.T) { + mockRDS := &MockRDSClient{} + client := &Client{client: mockRDS, region: "us-east-1"} + + rec := common.Recommendation{ + Service: common.ServiceRelationalDB, + ResourceType: "db.r5.large", + PaymentOption: "all-upfront", + Term: "0", + Details: &common.DatabaseDetails{ + Engine: "mysql", + AZConfig: "single-az", + }, + } + + _, err := client.findOfferingID(context.Background(), rec, "") + require.Error(t, err) + assert.Contains(t, err.Error(), "unsupported RDS reservation term") + mockRDS.AssertNotCalled(t, "DescribeReservedDBInstancesOfferings", mock.Anything, mock.Anything) +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@providers/aws/services/rds/client_test.go` around lines 560 - 595, Add a regression unit test for findOfferingID that verifies invalid/empty/unsupported terms short-circuit and do not call the AWS DescribeReservedDBInstancesOfferings API: create a new test (e.g., TestClient_FindOfferingID_InvalidTerm) that constructs a Client wired to a mocked RDS service, set the mock to expect NO calls to DescribeReservedDBInstancesOfferings (using your test mock's AssertNotCalled/ExpectNotCalled), call client.findOfferingID with an invalid term like "invalid" (and other cases like "" or "2yr"/"0"), assert it returns the expected error (contains "unsupported RDS reservation term") and finally assert the mock DescribeReservedDBInstancesOfferings was not invoked; reference the symbols Client.findOfferingID, Client.getDurationString (already covered), and DescribeReservedDBInstancesOfferings to locate code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@providers/aws/services/elasticache/client_test.go`:
- Around line 398-400: The test currently calls assert.Error(t, err) and then
unconditionally uses err.Error(), which can panic if the first assertion fails;
either change assert.Error to require.Error to stop execution on failure, or
capture the boolean result (e.g., ok := assert.Error(t, err)) and only call
assert.Contains(t, err.Error(), "unsupported ElastiCache reservation term") when
ok is true; apply this change around the assertion lines that perform the error
check and contains assertion in the ElastiCache client tests.
In `@providers/aws/services/rds/client_test.go`:
- Around line 560-595: Add a regression unit test for findOfferingID that
verifies invalid/empty/unsupported terms short-circuit and do not call the AWS
DescribeReservedDBInstancesOfferings API: create a new test (e.g.,
TestClient_FindOfferingID_InvalidTerm) that constructs a Client wired to a
mocked RDS service, set the mock to expect NO calls to
DescribeReservedDBInstancesOfferings (using your test mock's
AssertNotCalled/ExpectNotCalled), call client.findOfferingID with an invalid
term like "invalid" (and other cases like "" or "2yr"/"0"), assert it returns
the expected error (contains "unsupported RDS reservation term") and finally
assert the mock DescribeReservedDBInstancesOfferings was not invoked; reference
the symbols Client.findOfferingID, Client.getDurationString (already covered),
and DescribeReservedDBInstancesOfferings to locate code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 561e7d32-7692-47e0-b790-321a655d0345
📒 Files selected for processing (10)
providers/aws/services/elasticache/client.goproviders/aws/services/elasticache/client_test.goproviders/aws/services/memorydb/client.goproviders/aws/services/memorydb/client_test.goproviders/aws/services/opensearch/client.goproviders/aws/services/opensearch/client_test.goproviders/aws/services/rds/client.goproviders/aws/services/rds/client_test.goproviders/aws/services/redshift/client.goproviders/aws/services/redshift/client_test.go
Address CodeRabbit review feedback on PR #1207: - Guard the error-message Contains assertions behind the result of assert.Error in all five term-converter table tests, so a failed error expectation reports cleanly instead of panicking on a nil error. - Add TestFindOfferingID_InvalidTerm_ErrorsBeforeAPICall to the RDS client tests: an unrecognized term ("0", as produced by a 0/NULL Term DB row on the scheduler purchase path) must abort the offering lookup before any DescribeReservedDBInstancesOfferings call, which exercises the fail-loud path end to end rather than only the helper. Part of #1192.
Address CodeRabbit review feedback on PR #1207: - Guard the error-message Contains assertions behind the result of assert.Error in all five term-converter table tests, so a failed error expectation reports cleanly instead of panicking on a nil error. - Add TestFindOfferingID_InvalidTerm_ErrorsBeforeAPICall to the RDS client tests: an unrecognized term ("0", as produced by a 0/NULL Term DB row on the scheduler purchase path) must abort the offering lookup before any DescribeReservedDBInstancesOfferings call, which exercises the fail-loud path end to end rather than only the helper. Part of #1192.
7d85fc2 to
30ed652
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
Address CodeRabbit review feedback on PR #1207: - Guard the error-message Contains assertions behind the result of assert.Error in all five term-converter table tests, so a failed error expectation reports cleanly instead of panicking on a nil error. - Add TestFindOfferingID_InvalidTerm_ErrorsBeforeAPICall to the RDS client tests: an unrecognized term ("0", as produced by a 0/NULL Term DB row on the scheduler purchase path) must abort the offering lookup before any DescribeReservedDBInstancesOfferings call, which exercises the fail-loud path end to end rather than only the helper. Part of #1192.
30ed652 to
7a428c9
Compare
|
@coderabbitai review Rebased onto current main (was 5 commits behind, picking up #1261 migration-074-collision repair and #1085 SP fix already-merged context). Diff is back to the 10 service files only. Out-of-scope follow-ups filed:
|
|
✅ Action performedReview finished.
|
The term converters in the elasticache, memorydb, rds, redshift, and opensearch RI clients silently mapped any unrecognized or empty term to a 1-year duration before selecting the offering PurchaseCommitment buys. A malformed or NULL term reaching the purchase path (for example a 0 Term row executed by the scheduler, which does not re-validate) would silently buy a 1-year reservation when 3-year was intended. Mirror the savingsplans client fix: converters now return an explicit error for any term outside the supported set, and the error propagates from the offering lookup before any API call. Redshift and OpenSearch validate the term once up front via requiredMonthsForTerm and thread the resolved month count into matchesDuration. Regression tests cover empty, "0", "2yr", and garbage terms in all five packages. Closes #1192
Address CodeRabbit review feedback on PR #1207: - Guard the error-message Contains assertions behind the result of assert.Error in all five term-converter table tests, so a failed error expectation reports cleanly instead of panicking on a nil error. - Add TestFindOfferingID_InvalidTerm_ErrorsBeforeAPICall to the RDS client tests: an unrecognized term ("0", as produced by a 0/NULL Term DB row on the scheduler purchase path) must abort the offering lookup before any DescribeReservedDBInstancesOfferings call, which exercises the fail-loud path end to end rather than only the helper. Part of #1192.
For the four remaining ARCH-04 clients (ElastiCache, MemoryDB, OpenSearch, Redshift), add TestFindOfferingID_InvalidTerm_ErrorsBeforeAPICall tests that assert an invalid/zero term aborts before any describe-offerings API call. Mirrors the existing RDS call-path test added in the second commit on this PR. Each test uses AssertNotCalled to verify the AWS API is never reached when the term string is "0" (the scheduler-path 0/NULL Term DB row scenario from issue #1192/ARCH-04).
7a428c9 to
0c3cba5
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Merged to main after pre-merge adversarial review (verdict MERGE): all five RI service clients (elasticache, rds, memorydb, redshift, opensearch) now fail loud on every unrecognized/empty term string - no silent default-to-1yr remains, errors propagate before any API call, regression tests replicate real bad inputs ('', '0', '2yr', garbage) and the call-path tests assert the Describe API is never reached. Union with main green (299 tests). The same bug class found in the EC2 client during review is tracked as LeanerCloud/cloud-commitments-go#33. |
…ing to 1yr getDurationValue fell through to OneYearSeconds for any unrecognized or empty term string, missed by PR #1207's ARCH-04 sweep of the other five provider term converters. A malformed term (e.g. an empty string from a 0/NULL Term DB row on the scheduler purchase path) would silently buy a real 1-year EC2 RI instead of erroring, the same money-risk pattern #1207 fixed everywhere else. Mirror the whitelist-switch idiom used by the RDS/ElastiCache/MemoryDB/Redshift/OpenSearch converters: return an explicit error on anything outside 1yr/1/3yr/3 and propagate it through buildEC2QueryFromRec so findOfferingID aborts before any AWS API call.
…ing to 1yr (follow-up to #1207) (#1481) * fix(aws/ec2): reject unrecognized RI term instead of silently defaulting to 1yr getDurationValue fell through to OneYearSeconds for any unrecognized or empty term string, missed by PR #1207's ARCH-04 sweep of the other five provider term converters. A malformed term (e.g. an empty string from a 0/NULL Term DB row on the scheduler purchase path) would silently buy a real 1-year EC2 RI instead of erroring, the same money-risk pattern #1207 fixed everywhere else. Mirror the whitelist-switch idiom used by the RDS/ElastiCache/MemoryDB/Redshift/OpenSearch converters: return an explicit error on anything outside 1yr/1/3yr/3 and propagate it through buildEC2QueryFromRec so findOfferingID aborts before any AWS API call. * fix(scheduler): drop recommendations with unparseable term instead of defaulting to 3yr convertRecommendations coerced an empty or unparseable rec.Term to a Term=3 RecommendationRecord, logging only a warning (and nothing at all for an empty string). rec.Term round-trips through this int on the purchase path (internal/purchase/execution.go formats it back as "%dyr"), so the fabricated default let a malformed term launder into a real, purchasable 3-year commitment, bypassing every provider client's ARCH-04 term whitelist entirely since "3yr" is itself a valid value those whitelists accept. Drop the recommendation instead, with an ERROR-level log identifying the offending rec, rather than persisting a fabricated term.
1 participant
Problem
Closes #1192 (review finding ARCH-04, P2).
The term converters in five AWS RI clients (ElastiCache, MemoryDB, RDS, Redshift, OpenSearch) silently mapped any unrecognized or empty term string to a 1-year duration. The result feeds findOfferingID, which selects the offering that PurchaseCommitment buys, so a malformed or NULL term slipping past the boundary validators (for example a 0 Term DB row executed by the scheduler purchase path, which does not re-validate) would silently purchase a 1-year reservation when a 3-year commitment was intended. The savingsplans client in the same provider was already fixed to fail loud; these five were left inconsistently permissive.
Fix
Mirrors the savingsplans pattern in each client:
elasticacheandrds:getDurationStringnow returns(string, error)and errors on anything outside1yr, 1, 3yr, 3; the caller propagates the error before any API call.memorydb:getDurationStringForAPInow returns(string, error); keeps the previously accepted month-count forms (12,36) and errors on everything else.redshiftandopensearch: newrequiredMonthsForTermconverts the term to the offering duration in months once, up front infindOfferingID(failing loud before pagination); the resolved month count is threaded intomatchesDuration, removing the silent 12-month default from the per-offering matcher.No behavior change for valid terms; unknown terms now surface an explicit error instead of buying the wrong commitment length.
Test evidence
"","0","2yr", and"invalid"terms return an error (the empty term replicates the 0/NULL Term DB row scenario from the finding), plus positive cases for all accepted forms.assignment mismatch: 2 variables but client.getDurationString returns 1 value); the old code cannot return the error the tests require.go build ./...(root module) andgo build ./...(providers/aws module): success.go vet ./services/...: clean.gofmt -l: clean.go test ./...in providers/aws: 848 passed in 11 packages, 0 failures.The shared-purchase-core extraction that would deduplicate these converters across clients is tracked separately as ARCH-03.
Summary by CodeRabbit
Bug Fixes
Tests