From a2d0873af95ace4089984b505441e12f675ad0fc Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 22 May 2026 22:51:53 +0200 Subject: [PATCH 1/3] feat(common): add BuildReservationName for rich AWS reservation IDs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a shared builder that composes self-describing identifiers for AWS reservation purchases. The format mirrors the Azure DisplayName format introduced in #686 (cross-cloud symmetry for ops dashboards): {svc}-{region}-{sku}-{count}x-{term}-{paymt}-{ts}-{rand} Output is always sanitized via SanitizeReservationID for the AWS reservation-name allowlist ([a-zA-Z0-9-], dots in SKUs become hyphens) and capped at 60 characters — the tightest length cap across the five AWS services that accept a customer-supplied reservation ID/name (RDS ReservedDBInstanceId, ElastiCache ReservedCacheNodeId, MemoryDB ReservationId, OpenSearch ReservationName). When composed length exceeds the cap, optional tail segments drop in priority order: random suffix first, then timestamp, then payment option. The service code, region, SKU, count, and term are never dropped. In the pathological worst case the builder truncates the SKU itself so count and term still survive the final assembly. Time and random sources are injectable for deterministic tests. Refs #687 pkg/common/reservation_name.go | +201 pkg/common/reservation_name_test.go | +281 --- pkg/common/reservation_name.go | 220 +++++++++++++++++++++ pkg/common/reservation_name_test.go | 289 ++++++++++++++++++++++++++++ 2 files changed, 509 insertions(+) create mode 100644 pkg/common/reservation_name.go create mode 100644 pkg/common/reservation_name_test.go diff --git a/pkg/common/reservation_name.go b/pkg/common/reservation_name.go new file mode 100644 index 000000000..9cd0ba3e8 --- /dev/null +++ b/pkg/common/reservation_name.go @@ -0,0 +1,220 @@ +package common + +import ( + "crypto/rand" + "encoding/hex" + "fmt" + "strings" + "time" +) + +// awsReservationNameMaxLen is the tightest reservation-name length cap across +// the 5 AWS services that accept a customer-supplied reservation ID/name: +// RDS (ReservedDBInstanceId), ElastiCache (ReservedCacheNodeId), MemoryDB +// (ReservationId), and OpenSearch (ReservationName) all advertise a 60-char +// cap. Redshift doesn't accept a name field — its rich descriptors travel +// as tags instead. 60 is therefore the safe upper bound for any name the +// builder produces. +const awsReservationNameMaxLen = 60 + +// ReservationNameFields carries the inputs needed by BuildReservationName. +// +// Now and randSource are exposed so tests can pin time and randomness for +// deterministic assertions. Production callers leave randSource nil (the +// builder then uses crypto/rand) and pass time.Now(). +type ReservationNameFields struct { + // Service is the short identifier for the AWS service, e.g. "rds", + // "cache", "memdb", "opensearch", "redshift". Set per-call by the + // service client; the builder treats it as opaque and sanitizes it. + Service string + + // Region is the AWS region string (e.g. "us-east-1", "eu-west-1"). + Region string + + // ResourceType is the AWS instance/node type (e.g. "db.t4g.medium", + // "m5.large"). Dots are converted to hyphens by SanitizeReservationID. + ResourceType string + + // Count is the reservation quantity. Always rendered as "{N}x". + Count int + + // Term is the commitment term, normalized to "1yr" / "3yr" by upstream + // recommendation parsers. The builder collapses it to "1yr"/"3yr" when + // possible, and sanitizes otherwise. + Term string + + // Payment is the payment-option string from the recommendation + // ("all-upfront", "no-upfront", "partial-upfront"). The builder + // normalizes to a short form ("allup", "noup", "partup") so the + // segment stays under 8 chars. + Payment string + + // Now is the timestamp baseline. Must be non-zero for production calls. + // Tests inject a fixed value for determinism. + Now time.Time + + // randSource is an optional 4-byte source for the random suffix. + // When nil (production), the builder reads from crypto/rand. Tests set + // it via WithRandSource to make output deterministic. + randSource []byte +} + +// WithRandSource returns a copy of f with the given bytes used as the +// random suffix source (test hook). Production code does not call this. +func (f ReservationNameFields) WithRandSource(b []byte) ReservationNameFields { + f.randSource = b + return f +} + +// BuildReservationName composes a rich, parseable identifier for an AWS +// reservation purchase. The format mirrors the Azure DisplayName format +// from #686 so cross-cloud parsers can share logic: +// +// {svc}-{region}-{sku}-{count}x-{term}-{paymt}-{ts}-{rand} +// +// e.g. "opensearch-us-east-1-r6gd-large-search-3x-1yr-allup-20260521T002019-a1b2c3d4" +// (which then gets truncated to fit the 60-char cap — see below). +// +// The result is always sanitized via SanitizeReservationID for AWS +// reservation-name allowlists ([a-zA-Z0-9-], so underscores in SKU names +// become '-') and never longer than awsReservationNameMaxLen (60). If the +// composed string would exceed 60, optional tail fields are progressively +// dropped: random suffix first, then timestamp, then payment-option. The +// service code, region, SKU, count, and term are NEVER dropped — those are +// the high-signal segments operators rely on to identify the reservation +// in the AWS console. +// +// fallbackPrefix is the prefix passed to SanitizeReservationID for the +// unreachable empty-output fallback (e.g. "rds-reserved-"); it preserves +// the prior call-site behaviour at every service when the builder ever +// emits an unsanitisable input. +func BuildReservationName(f ReservationNameFields, fallbackPrefix string) string { + svc := normalizeReservationSegment(f.Service) + region := normalizeReservationSegment(f.Region) + sku := normalizeReservationSegment(f.ResourceType) + count := fmt.Sprintf("%dx", f.Count) + term := normalizeReservationTerm(f.Term) + paymt := normalizeReservationPayment(f.Payment) + ts := f.Now.UTC().Format("20060102T150405") + randHex := generateReservationRandSuffix(f.randSource) + + // Required segments (order matters — never dropped, never reordered). + required := []string{svc, region, sku, count, term} + + // Optional tail segments, in drop priority. The slice order here is + // "keep" order; dropping happens from the right. + tail := []string{paymt, ts, randHex} + + // Try full -> drop random -> drop timestamp -> drop payment. + for keep := len(tail); keep >= 0; keep-- { + segments := append([]string{}, required...) + segments = append(segments, tail[:keep]...) + candidate := joinReservationNonEmpty(segments, "-") + // Defensive sanitization: even though each segment is already + // allowlist-conformant, SanitizeReservationID guarantees the + // invariant (no leading/trailing/double hyphens) in one place + // rather than relying on every caller. + candidate = SanitizeReservationID(candidate, fallbackPrefix) + if len(candidate) <= awsReservationNameMaxLen { + return candidate + } + } + + // All optional segments dropped and we still bust the cap — the SKU + // is pathologically long (the other required segments are bounded: + // service codes are short, regions ≤14 chars, count "Nx" is 2–3 chars, + // term is 3 chars). Truncate the SKU itself (not the joined output) + // so count and term still survive the final assembly. + overflow := len(joinReservationNonEmpty(required, "-")) - awsReservationNameMaxLen + if overflow > 0 { + // Each separator counts; truncating N chars from the SKU shrinks + // the joined string by exactly N. Leave at least 1 SKU char so + // the segment doesn't collapse to "" and produce a "--" run. + newLen := len(sku) - overflow + if newLen < 1 { + newLen = 1 + } + sku = sku[:newLen] + required[2] = sku + } + out := SanitizeReservationID(joinReservationNonEmpty(required, "-"), fallbackPrefix) + if len(out) > awsReservationNameMaxLen { + // Last-resort hard cap if the SKU truncation above wasn't enough + // (e.g. a region name longer than expected). All output is ASCII + // so byte index == rune index. + out = strings.TrimRight(out[:awsReservationNameMaxLen], "-") + } + return out +} + +// normalizeReservationSegment strips disallowed characters from a single +// segment via the existing SanitizeReservationID rules. Dots become hyphens +// (so "db.t4g.medium" becomes "db-t4g-medium"), and underscores are dropped +// entirely. Leading/trailing hyphens and double-hyphen runs are collapsed +// by SanitizeReservationID itself. +func normalizeReservationSegment(s string) string { + return SanitizeReservationID(s, "") +} + +// normalizeReservationTerm maps "1"/"1yr"/"1y"/"P1Y" -> "1yr" and similar +// for 3-year. Anything else falls back to a sanitized passthrough. +func normalizeReservationTerm(s string) string { + switch strings.ToLower(strings.TrimSpace(s)) { + case "1", "1yr", "1y", "p1y": + return "1yr" + case "3", "3yr", "3y", "p3y": + return "3yr" + default: + return normalizeReservationSegment(s) + } +} + +// normalizeReservationPayment maps known payment-option strings to short +// forms. Unknown values are sanitized and truncated to keep the segment +// short (≤6 chars). +func normalizeReservationPayment(s string) string { + switch strings.ToLower(strings.TrimSpace(s)) { + case "all-upfront", "allupfront", "upfront": + return "allup" + case "no-upfront", "noupfront": + return "noup" + case "partial-upfront", "partialupfront", "partial": + return "partup" + case "": + return "" + default: + out := normalizeReservationSegment(s) + if len(out) > 6 { + out = out[:6] + } + return out + } +} + +// joinReservationNonEmpty joins parts with sep, skipping empty strings so +// callers don't produce double-separator artifacts when an optional segment +// is missing. +func joinReservationNonEmpty(parts []string, sep string) string { + out := make([]string, 0, len(parts)) + for _, p := range parts { + if p != "" { + out = append(out, p) + } + } + return strings.Join(out, sep) +} + +// generateReservationRandSuffix returns 8 hex chars from src (test hook) or +// crypto/rand. If randomness can't be obtained (extremely unlikely on +// supported platforms) returns an empty string — the builder treats that as +// a dropped suffix. +func generateReservationRandSuffix(src []byte) string { + if len(src) >= 4 { + return hex.EncodeToString(src[:4]) + } + var b [4]byte + if _, err := rand.Read(b[:]); err != nil { + return "" + } + return hex.EncodeToString(b[:]) +} diff --git a/pkg/common/reservation_name_test.go b/pkg/common/reservation_name_test.go new file mode 100644 index 000000000..70b0010cd --- /dev/null +++ b/pkg/common/reservation_name_test.go @@ -0,0 +1,289 @@ +package common + +import ( + "fmt" + "regexp" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/assert" +) + +var testFixedNow = time.Date(2026, 5, 21, 0, 20, 19, 0, time.UTC) + +func testFixedRand() []byte { return []byte{0xa1, 0xb2, 0xc3, 0xd4} } + +func TestBuildReservationName_HappyPath(t *testing.T) { + got := BuildReservationName(ReservationNameFields{ + Service: "opensearch", + Region: "us-east-1", + ResourceType: "r6g.large.search", + Count: 3, + Term: "1yr", + Payment: "all-upfront", + Now: testFixedNow, + }.WithRandSource(testFixedRand()), "opensearch-reserved-") + + // The composed full name is too long for the 60-char cap; the builder + // drops the random suffix and (here) the timestamp to fit. The + // high-signal required segments (svc/region/sku/count/term) and the + // payment-option always remain. + assert.LessOrEqual(t, len(got), awsReservationNameMaxLen) + assert.True(t, strings.HasPrefix(got, "opensearch-us-east-1-r6g-large-search-3x-1yr"), + "required segments must be present: got %q", got) + assert.Contains(t, got, "allup", "payment normalization must survive") +} + +func TestBuildReservationName_ShortInput_FullName(t *testing.T) { + // Short fields fit comfortably under the cap; verify the exact format. + got := BuildReservationName(ReservationNameFields{ + Service: "rds", + Region: "us-east-1", + ResourceType: "db.medium", + Count: 1, + Term: "1yr", + Payment: "no-upfront", + Now: testFixedNow, + }.WithRandSource(testFixedRand()), "rds-reserved-") + + // Full assembled name: "rds-us-east-1-db-medium-1x-1yr-noup-20260521T002019-a1b2c3d4" + // = 60 chars exactly. + assert.Equal(t, "rds-us-east-1-db-medium-1x-1yr-noup-20260521T002019-a1b2c3d4", got) + assert.LessOrEqual(t, len(got), awsReservationNameMaxLen) +} + +func TestBuildReservationName_LengthFit_DropsRandomFirst(t *testing.T) { + // Pick fields that compose just barely over 60 with the rand suffix but + // fit without it: svc(rds=3) + region(eu-west-1=9) + sku("m6gd.xlarge"->"m6gd-xlarge"=11) + + // count(1x=2) + term(1yr=3) + paymt(allup=5) + ts(15) + rand(8) + 7 separators + // = 3+9+11+2+3+5+15+8 + 7 = 63 chars. Without rand: 63-8-1 = 54. Fits. + got := BuildReservationName(ReservationNameFields{ + Service: "rds", + Region: "eu-west-1", + ResourceType: "m6gd.xlarge", + Count: 1, + Term: "1yr", + Payment: "all-upfront", + Now: testFixedNow, + }.WithRandSource(testFixedRand()), "rds-reserved-") + + assert.LessOrEqual(t, len(got), awsReservationNameMaxLen) + assert.NotContains(t, got, "a1b2c3d4", "random suffix should be the first to drop") + assert.Contains(t, got, "20260521T002019", "timestamp should still be present") + assert.Contains(t, got, "allup", "payment should still be present") +} + +func TestBuildReservationName_LengthFit_DropsTimestampNext(t *testing.T) { + // Pick a length where rand-dropped still overflows by ts-and-not-payment. + // With cache service code (5) + region (us-east-1 = 9) + a 24-char SKU, + // count + term + payment + ts = ~24+9+5+2+3+5+15 + 6 seps = 69; drop + // rand (8+1=-9) -> 60, exactly at the cap, keeps timestamp. + // To push ts off but keep payment, use a 25-char SKU. + got := BuildReservationName(ReservationNameFields{ + Service: "cache", + Region: "us-east-1", + ResourceType: "cache.r6gd.xlarge.foobarbaz", // 27 chars -> normalized 27 + Count: 2, + Term: "3yr", + Payment: "partial-upfront", // -> "partup" (6 chars) + Now: testFixedNow, + }.WithRandSource(testFixedRand()), "cache-reserved-") + + assert.LessOrEqual(t, len(got), awsReservationNameMaxLen) + assert.NotContains(t, got, "a1b2c3d4", "rand should drop first") + assert.NotContains(t, got, "20260521T002019", "ts should drop next") + assert.Contains(t, got, "partup", "payment should still survive") + assert.Contains(t, got, "2x-3yr", "count+term must never drop") +} + +func TestBuildReservationName_LengthFit_DropsPaymentLast(t *testing.T) { + // Compose so that even dropping rand+ts is not enough, forcing + // payment to drop too. Required-only must still fit at ≤60. + got := BuildReservationName(ReservationNameFields{ + Service: "opensearch", + Region: "ap-northeast-1", + ResourceType: "r6gd.16xlarge.search.opensearch", + Count: 12, + Term: "3yr", + Payment: "all-upfront", + Now: testFixedNow, + }.WithRandSource(testFixedRand()), "opensearch-reserved-") + + assert.LessOrEqual(t, len(got), awsReservationNameMaxLen) + assert.True(t, strings.HasPrefix(got, "opensearch-ap-northeast-1-"), + "service and region must never drop") + assert.Contains(t, got, "12x", "count must never drop") + assert.Contains(t, got, "3yr", "term must never drop") + assert.NotContains(t, got, "20260521T002019", "ts should drop in this length regime") + assert.NotContains(t, got, "a1b2c3d4", "rand should drop in this length regime") +} + +func TestBuildReservationName_LengthFit_WorstCase_TruncatesSKUNotCountTerm(t *testing.T) { + // Pathological: a SKU so long that even all optional segments dropped + // leaves us over 60. The builder must truncate the SKU itself rather + // than the joined string, so count and term still survive the cap. + got := BuildReservationName(ReservationNameFields{ + Service: "opensearch", + Region: "ap-northeast-1", + ResourceType: strings.Repeat("super-long-sku-", 6), + Count: 99, + Term: "3yr", + Payment: "all-upfront", + Now: testFixedNow, + }.WithRandSource(testFixedRand()), "opensearch-reserved-") + + assert.LessOrEqual(t, len(got), awsReservationNameMaxLen, "must never exceed the cap") + assert.False(t, strings.HasSuffix(got, "-"), "must not end with a hyphen after truncation: %q", got) + assert.True(t, strings.HasPrefix(got, "opensearch-ap-northeast-1-"), + "service and region must never drop, even in worst case") + assert.Contains(t, got, "99x", "count must survive SKU truncation") + assert.Contains(t, got, "3yr", "term must survive SKU truncation") +} + +func TestBuildReservationName_PaymentNormalization(t *testing.T) { + cases := []struct { + in string + want string + }{ + {"all-upfront", "allup"}, + {"upfront", "allup"}, + {"no-upfront", "noup"}, + {"partial-upfront", "partup"}, + {"partial", "partup"}, + {"", ""}, + } + for _, tc := range cases { + t.Run(tc.in, func(t *testing.T) { + got := normalizeReservationPayment(tc.in) + assert.Equal(t, tc.want, got) + }) + } +} + +func TestBuildReservationName_PaymentUnknown_TruncatedAndSanitized(t *testing.T) { + got := normalizeReservationPayment("monthly_billing") + // Dropped underscores, then truncated to 6 chars. + assert.LessOrEqual(t, len(got), 6) + assert.Regexp(t, regexp.MustCompile(`^[a-zA-Z0-9-]*$`), got) +} + +func TestBuildReservationName_SKUDotsToHyphens(t *testing.T) { + got := BuildReservationName(ReservationNameFields{ + Service: "rds", + Region: "us-east-1", + ResourceType: "db.t4g.medium", + Count: 1, + Term: "1yr", + Payment: "no-upfront", + Now: testFixedNow, + }.WithRandSource(testFixedRand()), "rds-reserved-") + + assert.Contains(t, got, "db-t4g-medium", "dots in SKU must become hyphens") + assert.NotContains(t, got, "db.t4g.medium") +} + +func TestBuildReservationName_TermNormalization(t *testing.T) { + cases := []struct { + in string + want string + }{ + {"1yr", "1yr"}, + {"1y", "1yr"}, + {"1", "1yr"}, + {"P1Y", "1yr"}, + {"3yr", "3yr"}, + {"3y", "3yr"}, + {"3", "3yr"}, + {"P3Y", "3yr"}, + } + for _, tc := range cases { + t.Run(tc.in, func(t *testing.T) { + got := normalizeReservationTerm(tc.in) + assert.Equal(t, tc.want, got) + }) + } +} + +func TestBuildReservationName_DeterministicWithFixedSources(t *testing.T) { + fields := ReservationNameFields{ + Service: "rds", + Region: "us-east-1", + ResourceType: "db.t4g.medium", + Count: 1, + Term: "1yr", + Payment: "all-upfront", + Now: testFixedNow, + }.WithRandSource(testFixedRand()) + + a := BuildReservationName(fields, "rds-reserved-") + b := BuildReservationName(fields, "rds-reserved-") + assert.Equal(t, a, b, "same inputs (incl. fixed Now + rand) must yield the same output") +} + +func TestBuildReservationName_AlwaysSanitized(t *testing.T) { + got := BuildReservationName(ReservationNameFields{ + Service: "rds", + Region: "us-east-1", + ResourceType: "db.t4g.medium", + Count: 1, + Term: "1yr", + Payment: "all-upfront", + Now: testFixedNow, + }.WithRandSource(testFixedRand()), "rds-reserved-") + + // AWS reservation-name allowlist is [a-zA-Z0-9-]. + assert.Regexp(t, regexp.MustCompile(`^[a-zA-Z0-9-]+$`), got) + assert.NotContains(t, got, "--", "must not contain consecutive hyphens") + assert.False(t, strings.HasPrefix(got, "-"), "must not start with a hyphen") + assert.False(t, strings.HasSuffix(got, "-"), "must not end with a hyphen") +} + +func TestBuildReservationName_NowInUTC(t *testing.T) { + // Pass a non-UTC time; builder must normalize to UTC for the timestamp. + loc, err := time.LoadLocation("America/Los_Angeles") + if err != nil { + t.Skip("LoadLocation unavailable") + } + localTime := time.Date(2026, 5, 20, 17, 20, 19, 0, loc) // = 2026-05-21T00:20:19Z + got := BuildReservationName(ReservationNameFields{ + Service: "rds", + Region: "us-east-1", + ResourceType: "db.t4g.medium", + Count: 1, + Term: "1yr", + Payment: "all-upfront", + Now: localTime, + }.WithRandSource(testFixedRand()), "rds-reserved-") + + assert.Contains(t, got, "20260521T002019", "timestamp must be UTC-formatted: %q", got) +} + +func TestBuildReservationName_PerServicePrefixes(t *testing.T) { + cases := []struct { + svc string + prefix string + }{ + {"rds", "rds-"}, + {"cache", "cache-"}, + {"memdb", "memdb-"}, + {"opensearch", "opensearch-"}, + {"redshift", "redshift-"}, + } + for _, tc := range cases { + t.Run(tc.svc, func(t *testing.T) { + got := BuildReservationName(ReservationNameFields{ + Service: tc.svc, + Region: "us-east-1", + ResourceType: "x.large", + Count: 1, + Term: "1yr", + Payment: "all-upfront", + Now: testFixedNow, + }.WithRandSource(testFixedRand()), fmt.Sprintf("%sreserved-", tc.prefix)) + + assert.True(t, strings.HasPrefix(got, tc.prefix), + "service code must lead the name: got %q want prefix %q", got, tc.prefix) + }) + } +} From 532deaf5e5e264042dbd77b2de547b60d8e98923 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 22 May 2026 22:52:40 +0200 Subject: [PATCH 2/3] feat(aws): rich self-describing reservation names matching Azure (closes #687) Wire common.BuildReservationName into the five AWS reservation purchase paths. On the no-token CLI fallback path (i.e. when opts.IdempotencyToken is empty), each service now composes a self-describing identifier of shape: {svc}-{region}-{sku}-{count}x-{term}-{paymt}-{ts}-{rand} For example, an OpenSearch RI purchase that previously yielded the opaque `opensearch-Standard_X-1779468234` now becomes `opensearch-us-east-1-r6g-large-search-3x-1yr-allup-20260521T002019-a1b2c3d4` (truncated to 60 chars per service allowlist; see #687). Per service: * rds: deriveReservationID's fmt.Sprintf fallback -> BuildReservationName ("rds-" prefix). opts.ReservationID still wins when set; idempotency token still wins above both. * elasticache: ReservedCacheNodeId fallback -> BuildReservationName ("cache-" prefix). * memorydb: ReservationId fallback -> BuildReservationName ("memdb-"). * opensearch: ReservationName fallback -> BuildReservationName ("opensearch-"). * redshift: PurchaseReservedNodeOffering doesn't accept a customer- supplied node ID, so the rich descriptors travel as tags instead. tagReservedNode now writes Count / Term / PaymentOption alongside the pre-existing NodeType / Region / PurchaseDate / Tool tags so the reserved node is identifiable from the AWS console alone. The token-based path (issue #641) is unchanged: IdempotentReservationID still derives the deterministic ID from the token so a re-drive sends the identical name and AWS rejects the duplicate server-side. The rich builder applies ONLY to the no-token fallback. Tests: one new per-service capture test asserts ^{svc-code}- prefix, region presence, SKU (dots->hyphens) presence, count+term embedded, and the 60-char cap. Redshift's test asserts the new tag keys (Count/Term/PaymentOption) appear in the CreateTags input. Closes #687 --- providers/aws/services/elasticache/client.go | 16 ++++- .../aws/services/elasticache/client_test.go | 37 +++++++++++ providers/aws/services/memorydb/client.go | 16 ++++- .../aws/services/memorydb/client_test.go | 37 +++++++++++ providers/aws/services/opensearch/client.go | 22 +++++-- .../aws/services/opensearch/client_test.go | 37 +++++++++++ providers/aws/services/rds/client.go | 23 ++++--- providers/aws/services/rds/client_test.go | 40 ++++++++++++ providers/aws/services/redshift/client.go | 10 +++ .../aws/services/redshift/client_test.go | 63 +++++++++++++++++++ 10 files changed, 282 insertions(+), 19 deletions(-) diff --git a/providers/aws/services/elasticache/client.go b/providers/aws/services/elasticache/client.go index 509c2ab12..6736f6899 100644 --- a/providers/aws/services/elasticache/client.go +++ b/providers/aws/services/elasticache/client.go @@ -129,11 +129,21 @@ func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendati // When an idempotency token is supplied (issue #641) the reservation ID is // derived deterministically from it, so a re-drive sends the identical // ReservedCacheNodeId and ElastiCache rejects the duplicate server-side - // (ReservedCacheNodeAlreadyExistsFault). Otherwise keep the prior - // timestamp-based ID (non-idempotent path). + // (ReservedCacheNodeAlreadyExistsFault). On the no-token CLI path + // (issue #687) compose a rich, self-describing identifier matching the + // Azure DisplayName format so operators can identify the reservation in + // the AWS console without cross-referencing CUDly's purchase audit log. reservationID := common.IdempotentReservationID("elasticache-id-", opts.IdempotencyToken) if reservationID == "" { - reservationID = common.SanitizeReservationID(fmt.Sprintf("elasticache-%s-%d", rec.ResourceType, time.Now().Unix()), "elasticache-reserved-") + reservationID = common.BuildReservationName(common.ReservationNameFields{ + Service: "cache", + Region: rec.Region, + ResourceType: rec.ResourceType, + Count: rec.Count, + Term: rec.Term, + Payment: rec.PaymentOption, + Now: time.Now(), + }, "elasticache-reserved-") } // Idempotency dedupe guard (issue #641): short-circuit if a reservation diff --git a/providers/aws/services/elasticache/client_test.go b/providers/aws/services/elasticache/client_test.go index 3e2fb39dc..b6e3263c1 100644 --- a/providers/aws/services/elasticache/client_test.go +++ b/providers/aws/services/elasticache/client_test.go @@ -3,6 +3,7 @@ package elasticache import ( "context" "fmt" + "strings" "testing" "time" @@ -525,6 +526,42 @@ func TestClient_PurchaseCommitment_Idempotent_FailLoudOnLookupError(t *testing.T mockEC.AssertNotCalled(t, "PurchaseReservedCacheNodesOffering", mock.Anything, mock.Anything) } +// TestClient_PurchaseCommitment_NoToken_RichReservationName asserts the +// no-token CLI path (issue #687) composes a self-describing +// ReservedCacheNodeId carrying the service code, region, SKU, count, and +// term. The token-based path is exercised by the Idempotent_* tests above. +func TestClient_PurchaseCommitment_NoToken_RichReservationName(t *testing.T) { + mockEC := &MockElastiCacheClient{} + client := &Client{client: mockEC, region: "us-east-1"} + + rec := common.Recommendation{ + Service: common.ServiceCache, + ResourceType: "cache.m6g.large", + Region: "us-east-1", + Count: 2, + PaymentOption: "all-upfront", + Term: "1yr", + Details: &common.CacheDetails{Engine: "redis", NodeType: "cache.m6g.large"}, + } + + expectECOffering(mockEC) + var capturedID string + mockEC.On("PurchaseReservedCacheNodesOffering", mock.Anything, mock.MatchedBy(func(in *elasticache.PurchaseReservedCacheNodesOfferingInput) bool { + capturedID = aws.ToString(in.ReservedCacheNodeId) + return true + })).Return(&elasticache.PurchaseReservedCacheNodesOfferingOutput{ + ReservedCacheNode: &types.ReservedCacheNode{ReservedCacheNodeId: aws.String("ec-x")}, + }, nil) + + _, err := client.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{}) + assert.NoError(t, err) + assert.True(t, strings.HasPrefix(capturedID, "cache-"), "name must lead with cache- service code: %q", capturedID) + assert.Contains(t, capturedID, "us-east-1", "region must be embedded: %q", capturedID) + assert.Contains(t, capturedID, "cache-m6g-large", "SKU (dots->hyphens) must be embedded: %q", capturedID) + assert.Contains(t, capturedID, "2x-1yr", "count and term must be embedded: %q", capturedID) + assert.LessOrEqual(t, len(capturedID), 60, "must fit AWS reservation-ID cap") +} + func TestCreatePurchaseTags_IncludesPurchaseAutomation(t *testing.T) { c := &Client{} rec := common.Recommendation{ResourceType: "cache.m5.large", Region: "us-east-1"} diff --git a/providers/aws/services/memorydb/client.go b/providers/aws/services/memorydb/client.go index 25d377709..3bb451ac9 100644 --- a/providers/aws/services/memorydb/client.go +++ b/providers/aws/services/memorydb/client.go @@ -125,11 +125,21 @@ func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendati // When an idempotency token is supplied (issue #641) the reservation ID is // derived deterministically from it, so a re-drive sends the identical // ReservationId and MemoryDB rejects the duplicate server-side - // (ReservedNodeAlreadyExistsFault). Otherwise keep the prior timestamp-based - // ID (non-idempotent path). + // (ReservedNodeAlreadyExistsFault). On the no-token CLI path (issue #687) + // compose a rich, self-describing identifier matching the Azure + // DisplayName format so operators can identify the reservation in the + // AWS console without cross-referencing CUDly's purchase audit log. reservationID := common.IdempotentReservationID("memorydb-id-", opts.IdempotencyToken) if reservationID == "" { - reservationID = common.SanitizeReservationID(fmt.Sprintf("memorydb-%s-%d", rec.ResourceType, time.Now().Unix()), "memorydb-reserved-") + reservationID = common.BuildReservationName(common.ReservationNameFields{ + Service: "memdb", + Region: rec.Region, + ResourceType: rec.ResourceType, + Count: rec.Count, + Term: rec.Term, + Payment: rec.PaymentOption, + Now: time.Now(), + }, "memorydb-reserved-") } // Idempotency dedupe guard (issue #641): short-circuit if a reserved node diff --git a/providers/aws/services/memorydb/client_test.go b/providers/aws/services/memorydb/client_test.go index a252eac02..f4f22b0d8 100644 --- a/providers/aws/services/memorydb/client_test.go +++ b/providers/aws/services/memorydb/client_test.go @@ -3,6 +3,7 @@ package memorydb import ( "context" "fmt" + "strings" "testing" "time" @@ -840,3 +841,39 @@ func TestCreatePurchaseTags_OmitsPurchaseAutomationWhenSourceEmpty(t *testing.T) assert.NotEqual(t, common.PurchaseTagKey, aws.ToString(tag.Key), "tag must be skipped when source is empty") } } + +// TestClient_PurchaseCommitment_NoToken_RichReservationName asserts the +// no-token CLI path (issue #687) composes a self-describing ReservationId +// carrying the service code, region, SKU, count, and term. The token-based +// path is exercised by the Idempotent_* tests above. +func TestClient_PurchaseCommitment_NoToken_RichReservationName(t *testing.T) { + mockMDB := &MockMemoryDBClient{} + client := &Client{client: mockMDB, region: "us-east-1"} + + rec := common.Recommendation{ + Service: common.ServiceCache, + ResourceType: "db.r6gd.large", + Region: "us-east-1", + Count: 3, + PaymentOption: "all-upfront", // must match expectMDBOffering's OfferingType + Term: "1yr", + Details: common.CacheDetails{Engine: "redis", NodeType: "db.r6gd.large"}, + } + + expectMDBOffering(mockMDB) + var capturedID string + mockMDB.On("PurchaseReservedNodesOffering", mock.Anything, mock.MatchedBy(func(in *memorydb.PurchaseReservedNodesOfferingInput) bool { + capturedID = aws.ToString(in.ReservationId) + return true + })).Return(&memorydb.PurchaseReservedNodesOfferingOutput{ + ReservedNode: &types.ReservedNode{ReservationId: aws.String("mdb-x")}, + }, nil) + + _, err := client.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{}) + assert.NoError(t, err) + assert.True(t, strings.HasPrefix(capturedID, "memdb-"), "name must lead with memdb- service code: %q", capturedID) + assert.Contains(t, capturedID, "us-east-1", "region must be embedded: %q", capturedID) + assert.Contains(t, capturedID, "db-r6gd-large", "SKU (dots->hyphens) must be embedded: %q", capturedID) + assert.Contains(t, capturedID, "3x-1yr", "count and term must be embedded: %q", capturedID) + assert.LessOrEqual(t, len(capturedID), 60, "must fit AWS reservation-ID cap") +} diff --git a/providers/aws/services/opensearch/client.go b/providers/aws/services/opensearch/client.go index 6773cee2d..f2bf413d0 100644 --- a/providers/aws/services/opensearch/client.go +++ b/providers/aws/services/opensearch/client.go @@ -150,15 +150,25 @@ func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendati return result, result.Error } - // When an idempotency token is supplied (issue #641) the ReservationName is - // derived deterministically from it. ReservationName is unique per + // When an idempotency token is supplied (issue #641) the ReservationName + // is derived deterministically from it — ReservationName is unique per // account+region, so a re-drive sends the identical name and OpenSearch - // rejects the duplicate server-side (ResourceAlreadyExistsException) — it - // cannot create a second reservation. Otherwise keep the prior timestamp-based - // name (non-idempotent path). + // rejects the duplicate server-side (ResourceAlreadyExistsException). + // On the no-token CLI path (issue #687) compose a rich, self-describing + // identifier matching the Azure DisplayName format so operators can + // identify the reservation in the AWS console without cross-referencing + // CUDly's purchase audit log. reservationName := common.IdempotentReservationID("opensearch-id-", opts.IdempotencyToken) if reservationName == "" { - reservationName = common.SanitizeReservationID(fmt.Sprintf("opensearch-%s-%d", rec.ResourceType, time.Now().Unix()), "opensearch-reserved-") + reservationName = common.BuildReservationName(common.ReservationNameFields{ + Service: "opensearch", + Region: rec.Region, + ResourceType: rec.ResourceType, + Count: rec.Count, + Term: rec.Term, + Payment: rec.PaymentOption, + Now: time.Now(), + }, "opensearch-reserved-") } // Idempotency dedupe guard (issue #641): short-circuit if a reservation with diff --git a/providers/aws/services/opensearch/client_test.go b/providers/aws/services/opensearch/client_test.go index 6c47fbd6d..00a3ebf6b 100644 --- a/providers/aws/services/opensearch/client_test.go +++ b/providers/aws/services/opensearch/client_test.go @@ -3,6 +3,7 @@ package opensearch import ( "context" "fmt" + "strings" "testing" "time" @@ -912,3 +913,39 @@ func TestFindOfferingID_HappyPath(t *testing.T) { assert.NoError(t, err) assert.Equal(t, "offering-ok", id) } + +// TestClient_PurchaseCommitment_NoToken_RichReservationName asserts the +// no-token CLI path (issue #687) composes a self-describing +// ReservationName carrying the service code, region, SKU, count, and term. +// The token-based path is exercised by the Idempotent_* tests above. +func TestClient_PurchaseCommitment_NoToken_RichReservationName(t *testing.T) { + mockOS := &MockOpenSearchClient{} + client := &Client{client: mockOS, region: "us-east-1"} + + rec := common.Recommendation{ + Service: common.ServiceSearch, + ResourceType: "m5.xlarge.search", + Region: "us-east-1", + Count: 2, + PaymentOption: "all-upfront", + Term: "3yr", + Details: common.SearchDetails{InstanceType: "m5.xlarge.search"}, + } + + expectOSOffering(mockOS) + var capturedName string + mockOS.On("PurchaseReservedInstanceOffering", mock.Anything, mock.MatchedBy(func(in *opensearch.PurchaseReservedInstanceOfferingInput) bool { + capturedName = aws.ToString(in.ReservationName) + return true + })).Return(&opensearch.PurchaseReservedInstanceOfferingOutput{ + ReservedInstanceId: aws.String("os-x"), + }, nil) + + _, err := client.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{}) + assert.NoError(t, err) + assert.True(t, strings.HasPrefix(capturedName, "opensearch-"), "name must lead with opensearch- service code: %q", capturedName) + assert.Contains(t, capturedName, "us-east-1", "region must be embedded: %q", capturedName) + assert.Contains(t, capturedName, "m5-xlarge-search", "SKU (dots->hyphens) must be embedded: %q", capturedName) + assert.Contains(t, capturedName, "2x-3yr", "count and term must be embedded: %q", capturedName) + assert.LessOrEqual(t, len(capturedName), 60, "must fit AWS reservation-ID cap") +} diff --git a/providers/aws/services/rds/client.go b/providers/aws/services/rds/client.go index 2a2e0ae13..199e4004d 100644 --- a/providers/aws/services/rds/client.go +++ b/providers/aws/services/rds/client.go @@ -192,17 +192,26 @@ func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendati // idempotency token is supplied (issue #641) the ID is derived deterministically // from it, so a re-drive sends the identical ID and RDS rejects the duplicate // server-side (ReservedDBInstanceAlreadyExistsFault). Otherwise it prefers the -// caller-supplied descriptive ID and falls back to a generic timestamped one -// (prior non-idempotent behaviour). +// caller-supplied descriptive ID; if absent (the no-token CLI path, issue #687) +// it composes a rich, self-describing identifier matching the Azure DisplayName +// format so operators can identify the reservation in the AWS console without +// cross-referencing CUDly's purchase audit log. func (c *Client) deriveReservationID(rec common.Recommendation, opts common.PurchaseOptions) string { if id := common.IdempotentReservationID("rds-id-", opts.IdempotencyToken); id != "" { return id } - rawID := opts.ReservationID - if rawID == "" { - rawID = fmt.Sprintf("rds-%s-%d", rec.ResourceType, time.Now().Unix()) - } - return common.SanitizeReservationID(rawID, "rds-reserved-") + if opts.ReservationID != "" { + return common.SanitizeReservationID(opts.ReservationID, "rds-reserved-") + } + return common.BuildReservationName(common.ReservationNameFields{ + Service: "rds", + Region: rec.Region, + ResourceType: rec.ResourceType, + Count: rec.Count, + Term: rec.Term, + Payment: rec.PaymentOption, + Now: time.Now(), + }, "rds-reserved-") } // idempotencyGuard short-circuits a re-drive (issue #641): when token is set, it diff --git a/providers/aws/services/rds/client_test.go b/providers/aws/services/rds/client_test.go index 4af798789..7f5d884a3 100644 --- a/providers/aws/services/rds/client_test.go +++ b/providers/aws/services/rds/client_test.go @@ -3,6 +3,7 @@ package rds import ( "context" "fmt" + "strings" "testing" "time" @@ -796,3 +797,42 @@ func TestFindOfferingID_HappyPath(t *testing.T) { assert.NoError(t, err) assert.Equal(t, "offering-ok", id) } + +// TestClient_PurchaseCommitment_NoToken_RichReservationName asserts the +// no-token CLI path (issue #687) composes a self-describing +// ReservedDBInstanceId carrying the service code, region, SKU, count, and +// term. The token-based path is exercised by the Idempotent_* tests above. +func TestClient_PurchaseCommitment_NoToken_RichReservationName(t *testing.T) { + mockRDS := &MockRDSClient{} + client := &Client{client: mockRDS, region: "eu-west-1"} + + rec := common.Recommendation{ + Service: common.ServiceRelationalDB, + ResourceType: "db.t4g.medium", + Region: "eu-west-1", + Count: 1, + PaymentOption: "all-upfront", + Term: "1yr", + Details: &common.DatabaseDetails{ + Engine: "mysql", + AZConfig: "single-az", + }, + } + + expectOffering(mockRDS) + var capturedID string + mockRDS.On("PurchaseReservedDBInstancesOffering", mock.Anything, mock.MatchedBy(func(in *rds.PurchaseReservedDBInstancesOfferingInput) bool { + capturedID = aws.ToString(in.ReservedDBInstanceId) + return true + })).Return(&rds.PurchaseReservedDBInstancesOfferingOutput{ + ReservedDBInstance: &types.ReservedDBInstance{ReservedDBInstanceId: aws.String("ri-x")}, + }, nil) + + _, err := client.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{}) + assert.NoError(t, err) + assert.True(t, strings.HasPrefix(capturedID, "rds-"), "name must lead with rds- service code: %q", capturedID) + assert.Contains(t, capturedID, "eu-west-1", "region must be embedded: %q", capturedID) + assert.Contains(t, capturedID, "db-t4g-medium", "SKU (dots->hyphens) must be embedded: %q", capturedID) + assert.Contains(t, capturedID, "1x-1yr", "count and term must be embedded: %q", capturedID) + assert.LessOrEqual(t, len(capturedID), 60, "must fit AWS reservation-ID cap") +} diff --git a/providers/aws/services/redshift/client.go b/providers/aws/services/redshift/client.go index f71f2c65d..ac194e0f0 100644 --- a/providers/aws/services/redshift/client.go +++ b/providers/aws/services/redshift/client.go @@ -340,10 +340,20 @@ func (c *Client) tagReservedNode(ctx context.Context, nodeID string, rec common. } arn := fmt.Sprintf("arn:aws:redshift:%s:%s:reservednode:%s", c.region, accountID, nodeID) + // Rich self-describing tag set (issue #687): the Redshift purchase API + // does not accept a customer-supplied node ID, so the only way to make + // the reserved node identifiable from the AWS console alone (without + // cross-referencing CUDly) is to encode the same descriptors that the + // other AWS service clients embed in their reservation name. Term, + // PaymentOption, and Count join the pre-existing NodeType/Region/ + // PurchaseDate set. tags := []redshifttypes.Tag{ {Key: aws.String("Purpose"), Value: aws.String("Reserved Node Purchase")}, {Key: aws.String("NodeType"), Value: aws.String(rec.ResourceType)}, {Key: aws.String("Region"), Value: aws.String(rec.Region)}, + {Key: aws.String("Count"), Value: aws.String(fmt.Sprintf("%d", rec.Count))}, + {Key: aws.String("Term"), Value: aws.String(rec.Term)}, + {Key: aws.String("PaymentOption"), Value: aws.String(rec.PaymentOption)}, {Key: aws.String("PurchaseDate"), Value: aws.String(time.Now().Format("2006-01-02"))}, {Key: aws.String("Tool"), Value: aws.String("CUDly")}, } diff --git a/providers/aws/services/redshift/client_test.go b/providers/aws/services/redshift/client_test.go index a650a547e..03b43dff9 100644 --- a/providers/aws/services/redshift/client_test.go +++ b/providers/aws/services/redshift/client_test.go @@ -379,6 +379,69 @@ func TestClient_PurchaseCommitment_TagsWithResolvedARN(t *testing.T) { mockSTS.AssertExpectations(t) } +// TestClient_PurchaseCommitment_TagsCarryRichDescriptors asserts the rich +// self-describing tag set (issue #687): the Redshift purchase API does not +// accept a customer-supplied node ID, so the only way to make the reserved +// node identifiable from the AWS console alone (without cross-referencing +// CUDly's purchase audit log) is to encode service / region / SKU / count / +// term / payment as tags. +func TestClient_PurchaseCommitment_TagsCarryRichDescriptors(t *testing.T) { + mockRS := &MockRedshiftClient{} + mockSTS := &MockRedshiftSTSClient{} + client := &Client{ + client: mockRS, + stsClient: mockSTS, + region: "us-east-1", + } + + rec := common.Recommendation{ + Service: common.ServiceDataWarehouse, + ResourceType: "ra3.xlplus", + Count: 2, + PaymentOption: "partial-upfront", + Term: "3yr", + Region: "us-east-1", + Details: common.DataWarehouseDetails{NodeType: "ra3.xlplus", NumberOfNodes: 2}, + } + + mockRS.On("DescribeReservedNodeOfferings", mock.Anything, mock.Anything). + Return(&redshift.DescribeReservedNodeOfferingsOutput{ + ReservedNodeOfferings: []types.ReservedNodeOffering{{ + ReservedNodeOfferingId: aws.String("off-rich"), + NodeType: aws.String("ra3.xlplus"), + Duration: aws.Int32(94608000), + ReservedNodeOfferingType: types.ReservedNodeOfferingType("Regular"), + FixedPrice: aws.Float64(500.0), + }}, + }, nil) + + mockRS.On("PurchaseReservedNodeOffering", mock.Anything, mock.Anything). + Return(&redshift.PurchaseReservedNodeOfferingOutput{ + ReservedNode: &types.ReservedNode{ReservedNodeId: aws.String("rn-rich"), FixedPrice: aws.Float64(500.0)}, + }, nil) + + mockSTS.On("GetCallerIdentity", mock.Anything, mock.Anything). + Return(&sts.GetCallerIdentityOutput{Account: aws.String("123456789012")}, nil) + + mockRS.On("CreateTags", mock.Anything, mock.MatchedBy(func(in *redshift.CreateTagsInput) bool { + got := map[string]string{} + for _, tag := range in.Tags { + got[aws.ToString(tag.Key)] = aws.ToString(tag.Value) + } + // Required new descriptors from #687 (the existing NodeType / Region / + // PurchaseDate / Tool / Purpose tags are covered by the existing + // TagsWithResolvedARN test). + return got["Count"] == "2" && + got["Term"] == "3yr" && + got["PaymentOption"] == "partial-upfront" + })).Return(&redshift.CreateTagsOutput{}, nil) + + result, err := client.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) + assert.NoError(t, err) + assert.True(t, result.Success) + mockRS.AssertExpectations(t) +} + func TestClient_MatchesDuration(t *testing.T) { client := &Client{} From 0ddd4e5bfec781c452195d6dc633028611e387bf Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 22 May 2026 23:49:54 +0200 Subject: [PATCH 3/3] fix(common): default zero Now in reservation name + assert payment drop in length test Default f.Now to time.Now() when IsZero() so a missed caller field produces a current timestamp rather than a year-0001 artifact. Deterministic tests are unaffected because they set Now: testFixedNow explicitly. Add assert.NotContains(t, got, "allup", ...) to TestBuildReservationName_LengthFit_DropsPaymentLast so the test actually verifies the payment segment is dropped in the tight-length regime, not just that timestamp and random were removed. Addresses CodeRabbit findings on PR #689. --- pkg/common/reservation_name.go | 6 +++++- pkg/common/reservation_name_test.go | 1 + 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/pkg/common/reservation_name.go b/pkg/common/reservation_name.go index 9cd0ba3e8..4d15bf01f 100644 --- a/pkg/common/reservation_name.go +++ b/pkg/common/reservation_name.go @@ -95,7 +95,11 @@ func BuildReservationName(f ReservationNameFields, fallbackPrefix string) string count := fmt.Sprintf("%dx", f.Count) term := normalizeReservationTerm(f.Term) paymt := normalizeReservationPayment(f.Payment) - ts := f.Now.UTC().Format("20060102T150405") + tsTime := f.Now + if tsTime.IsZero() { + tsTime = time.Now() + } + ts := tsTime.UTC().Format("20060102T150405") randHex := generateReservationRandSuffix(f.randSource) // Required segments (order matters — never dropped, never reordered). diff --git a/pkg/common/reservation_name_test.go b/pkg/common/reservation_name_test.go index 70b0010cd..ef2657bf2 100644 --- a/pkg/common/reservation_name_test.go +++ b/pkg/common/reservation_name_test.go @@ -115,6 +115,7 @@ func TestBuildReservationName_LengthFit_DropsPaymentLast(t *testing.T) { "service and region must never drop") assert.Contains(t, got, "12x", "count must never drop") assert.Contains(t, got, "3yr", "term must never drop") + assert.NotContains(t, got, "allup", "payment should be the last optional segment to drop") assert.NotContains(t, got, "20260521T002019", "ts should drop in this length regime") assert.NotContains(t, got, "a1b2c3d4", "rand should drop in this length regime") }