diff --git a/pkg/common/reservation_name.go b/pkg/common/reservation_name.go new file mode 100644 index 000000000..4d15bf01f --- /dev/null +++ b/pkg/common/reservation_name.go @@ -0,0 +1,224 @@ +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) + 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). + 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..ef2657bf2 --- /dev/null +++ b/pkg/common/reservation_name_test.go @@ -0,0 +1,290 @@ +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, "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") +} + +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) + }) + } +} 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{}