fix(aws/ec2): reject unrecognized RI term instead of silently defaulting to 1yr (follow-up to #1207) by cristim · Pull Request #1481 · LeanerCloud/cloud-commitments-cli

cristim · 2026-07-21T19:59:26Z

Summary

Follow-up to #1207 (ARCH-04): that PR fixed five provider term converters
(elasticache, rds, memorydb, redshift, opensearch) that silently defaulted
an unrecognized/malformed RI/SP term string to a valid duration instead of
erroring. Its inventory missed a sixth converter with the exact same bug,
plus a related issue one step upstream of all six.

1. `providers/aws/services/ec2/client.go`: `getDurationValue`

func (c *Client) getDurationValue(term string) int64 {
	if term == "3yr" || term == "3" {
		return ThreeYearSeconds
	}
	return OneYearSeconds
}

Any unrecognized or empty term (including an empty string produced by a
0/NULL Term DB row on the scheduler purchase path) silently fell through
to OneYearSeconds, feeding into buildEC2QueryFromRec -> findOfferingID
-> PurchaseCommitment and buying a real 1-year EC2 RI instead of erroring.
The existing test even asserted this fallback as intended behavior. Fixed
by mirroring the whitelist-switch idiom from the five already-fixed
converters: getDurationValue now returns (int64, error) and rejects
anything outside 1yr/1/3yr/3, with the error propagated up through
buildEC2QueryFromRec.

2. `internal/scheduler/scheduler.go`: `convertRecommendations`

An empty or unparseable rec.Term was silently coerced to Term: 3 when
persisting the RecommendationRecord (only a warn-log for a parse
failure, no log 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 this
let a malformed term launder into a valid, purchasable 3-year commitment
upstream of every provider client's ARCH-04 whitelist -- those
whitelists can never catch it, because "3yr" is itself one of the
values they accept. Fixed by dropping the recommendation (skip
persisting it) with a clear ERROR-level log instead of fabricating a
term.

Testing

go build ./... -- clean (exit 0)
go vet ./... -- clean (exit 0)
go test ./providers/aws/... ./internal/scheduler/... -- all packages
ok (exit 0); internal/scheduler includes the new
TestScheduler_ConvertRecommendations_InvalidTermDropped regression
test, which fails on the pre-fix code (asserts the batch drops both a
blank-term and a garbage-term rec while keeping a valid one) and
passes on the fix
go test ./... (full suite, root module) -- all ok, no failures
golangci-lint v2.10.1 (exact CI pin) run exactly as CI invokes it
(golangci-lint run --timeout=10m, no path args) -- 0 issues on this
branch
gocyclo -over 10 -ignore "_test\.go" . (exact CI invocation) -- no
functions over the threshold

Note on CI coverage

While reproducing CI's exact lint/vet/test invocations to verify this
fix, I found that go vet ./..., the unit-test step, the
integration-test step, and golangci-lint in ci.yml all run bare
./... from the repo root, which in this multi-module (go.work)
layout only covers the root module -- providers/aws (where the EC2
fix in this PR lives), providers/azure, providers/gcp, pkg, and
tests/e2e are silently skipped by all four checks. Only govulncheck
and gosec already loop per-module correctly. Filed as #1478; not
fixed here to keep this PR scoped to the ARCH-04 follow-up. All
verification above for the providers/aws/services/ec2 changes was
done locally with the exact CI-pinned toolchain since CI itself
currently can't verify that module.

Closes: follow-up to #1207

coderabbitai · 2026-07-21T19:59:35Z

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 6 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 858ba813-4611-47a0-a537-21e45141326b

📥 Commits

Reviewing files that changed from the base of the PR and between 1cc33d3 and 18d49cc.

📒 Files selected for processing (4)

internal/scheduler/scheduler.go
internal/scheduler/scheduler_test.go
providers/aws/services/ec2/client.go
providers/aws/services/ec2/client_test.go

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/1207-ec2-term-silent-default

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

cristim · 2026-07-21T19:59:37Z

@coderabbitai review

coderabbitai · 2026-07-21T19:59:42Z

✅ Action performed

Review finished.

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.

…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.

… 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.

cristim · 2026-07-22T21:35:45Z

@coderabbitai review

coderabbitai · 2026-07-22T21:35:50Z

✅ Action performed

Review finished.

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 added triaged Item has been triaged priority/p1 Next up; this sprint severity/high Significant harm urgency/now Drop other things impact/all-users Affects every user effort/s Hours type/bug Defect labels Jul 21, 2026

cristim added 2 commits July 22, 2026 23:27

cristim force-pushed the fix/1207-ec2-term-silent-default branch from 37bd810 to 18d49cc Compare July 22, 2026 21:35

cristim merged commit c59c116 into main Jul 22, 2026
19 checks passed

cristim deleted the fix/1207-ec2-term-silent-default branch July 27, 2026 11:09

This was referenced Jul 27, 2026

providers/aws/ec2: getDurationValue silently defaults unknown RI terms to 1yr LeanerCloud/cloud-commitments-go#33

Open

fix(providers/aws): getDurationString/getDurationValue silently fallback to 1yr on unrecognized term #1266

Closed

cristim mentioned this pull request Sep 2, 2026

fix(providers/aws): fail loud on unrecognized RI term in EC2 getDurationValue #1260

Closed

Navigation Menu

Repository navigation

fix(aws/ec2): reject unrecognized RI term instead of silently defaulting to 1yr (follow-up to #1207) - #1481

fix(aws/ec2): reject unrecognized RI term instead of silently defaulting to 1yr (follow-up to #1207)#1481
cristim merged 2 commits into
mainfrom
fix/1207-ec2-term-silent-default

cristim commented Jul 21, 2026

Uh oh!

coderabbitai Bot commented Jul 21, 2026 •

edited

Loading

Review limit reached

Uh oh!

cristim commented Jul 21, 2026

Uh oh!

coderabbitai Bot commented Jul 21, 2026 •

edited

Loading

Uh oh!

cristim commented Jul 22, 2026

Uh oh!

coderabbitai Bot commented Jul 22, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

cristim commented Jul 21, 2026

Summary

1. providers/aws/services/ec2/client.go: getDurationValue

2. internal/scheduler/scheduler.go: convertRecommendations

Testing

Note on CI coverage

Uh oh!

coderabbitai Bot commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Uh oh!

cristim commented Jul 21, 2026

Uh oh!

coderabbitai Bot commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cristim commented Jul 22, 2026

Uh oh!

coderabbitai Bot commented Jul 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

1. `providers/aws/services/ec2/client.go`: `getDurationValue`

2. `internal/scheduler/scheduler.go`: `convertRecommendations`

coderabbitai Bot commented Jul 21, 2026 •

edited

Loading

coderabbitai Bot commented Jul 21, 2026 •

edited

Loading

coderabbitai Bot commented Jul 22, 2026 •

edited

Loading