Skip to content

Fix AWS RI purchase: details assertion + reservation ID rules#4

Open
babyhuey wants to merge 3 commits intoLeanerCloud:mainfrom
babyhuey:main
Open

Fix AWS RI purchase: details assertion + reservation ID rules#4
babyhuey wants to merge 3 commits intoLeanerCloud:mainfrom
babyhuey:main

Conversation

@babyhuey
Copy link

Fix AWS RI purchase (details + reservation IDs)
RDS purchases were failing for two reasons; fixed both and applied the same fixes elsewhere they mattered.
Recommendations were setting rec.Details as pointers but RDS/EC2/ElastiCache were asserting to the value type, so the assertion always failed. Updated those clients to assert the pointer type (with a nil check) and adjusted the tests to pass pointers too.
We were also building reservation IDs from rec.ResourceType (e.g. db.t3.small), so they had dots in them. AWS only allows letters, digits, and hyphens for those IDs. Added sanitization where we set the ID (RDS, ElastiCache, OpenSearch, MemoryDB): replace dots with hyphens, collapse/trim hyphens, and use a fallback like rds-reserved- if the result is empty. Redshift is unchanged since it doesn’t send a custom ID.
All relevant tests pass.

…servation ID

Recommendations set rec.Details as pointers (e.g. &DatabaseDetails) but
RDS/EC2/ElastiCache asserted to the value type, so the assertion always
failed and purchases hit 'invalid service details'. Switched those
clients to assert the pointer type and updated tests to pass pointers.

Reservation IDs were built from rec.ResourceType (e.g. db.t3.small) so
they contained dots; AWS only allows letters, digits, and hyphens.
Added sanitization for the custom ID/name field in RDS, ElastiCache,
OpenSearch, and MemoryDB so we only send valid identifiers.

Co-authored-by: Cursor <cursoragent@cursor.com>
@cristim
Copy link
Member

cristim commented Feb 12, 2026

@babyhuey Thank you for this contribution, I really appreciate it!

To be honest I'm surprised to see reports of failures for RDS and ElastiCache RI purchasess, since I used those extensively not long ago.

One small thing I'd like to see changed is deduplicating the sanitization code if possible, since much of it seems to be duplicated across services.

@cristim
Copy link
Member

cristim commented Feb 12, 2026

I also had Claude Code review this PR and it came up with the same suggestion to deduplicate that sanitization code, and in addition to that it also complained about inconsistent naming for those sanitization functions:

PR #4 Review: Fix AWS RI purchase — details assertion + reservation ID rules

+118 / -22 across 8 files


Summary

The PR fixes two real bugs:

  1. Type assertion mismatchrec.Details is stored as a pointer (e.g. *common.DatabaseDetails) but asserted as a value type, causing assertions to always fail
  2. Invalid reservation IDs — dots in resource types like db.t3.small produce IDs that violate AWS naming rules (letters, digits, hyphens only)

Both bugs are real and would cause RI purchases to fail at runtime. The fixes are correct.


Issues Found

Major: Duplicated sanitization function (4 copies)

The sanitizeReservationID function is copy-pasted identically across:

  • rds/client.gosanitizeReservedDBInstanceID
  • elasticache/client.gosanitizeReservationID
  • memorydb/client.gosanitizeReservationID
  • opensearch/client.gosanitizeReservationName

The logic is identical — only the fallback prefix differs. This should be a shared utility function, e.g. in pkg/common/:

func SanitizeReservationID(id, fallbackPrefix string) string { ... }

Minor: Inconsistent function naming

  • RDS: sanitizeReservedDBInstanceID
  • ElastiCache/MemoryDB: sanitizeReservationID
  • OpenSearch: sanitizeReservationName

If keeping them per-service (not recommended), at least use a consistent name.

Minor: Methods don't use receiver

All sanitize* functions are methods on *Client (func (c *Client)) but never use c. They should be standalone package-level functions, or better yet, a shared utility.


What the PR Gets Right

  • The core bug diagnoses are correct — both the pointer assertion mismatch and the reservation ID dots are real production-breaking bugs
  • Nil checks added after pointer assertions (|| details == nil) — good defensive programming
  • Tests updated to use pointer types, matching production behavior
  • Redshift correctly excluded — it doesn't use rec.Details in findOfferingID and doesn't send a custom reservation ID

Verdict

The PR is sound. Both fixes address real bugs that would cause RI purchases to fail. The main suggestion is extracting the duplicated sanitization into a shared utility function in pkg/common/ to reduce the 4 near-identical copies.


🤖 Generated with Claude Code

Extract SanitizeReservationID into pkg/common/identifiers.go and use it
from RDS, ElastiCache, OpenSearch, and MemoryDB instead of four
per-service copies. Addresses PR review feedback.
@babyhuey
Copy link
Author

Pulled the sanitization logic into pkg/common and wired everyone up to it.
Added pkg/common/identifiers.go with a single SanitizeReservationID(id, fallbackPrefix string) that does the letters/digits/hyphens + collapse/trim thing. RDS, ElastiCache, OpenSearch, and MemoryDB now call that with their own prefix (rds-reserved-, etc.) and I removed the four duplicate helpers. All tests are passing.

Cost Explorer can return InstanceSize as the full type (e.g. t3.medium.search).
Concatenating with InstanceClass produced duplicates (t3.medium.t3.medium.search).
Use InstanceSize as-is when it already ends with .search, otherwise build
InstanceClass.InstanceSize.search.

findOfferingID only read the first page of DescribeReservedInstanceOfferings;
offerings for types like i3.large.search or t3.medium.search can be on later
pages. Paginate with NextToken until a matching offering is found or pages
are exhausted.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants