diff --git a/internal/purchase/elasticache_recent_check.go b/internal/purchase/elasticache_recent_check.go index 36e914db6..a97bf9a1e 100644 --- a/internal/purchase/elasticache_recent_check.go +++ b/internal/purchase/elasticache_recent_check.go @@ -1,11 +1,26 @@ package purchase import ( + "context" + "errors" + "slices" + "strings" + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" + "github.com/LeanerCloud/cloud-commitments-go/pkg/provider" + "github.com/LeanerCloud/cloud-commitments-go/pkg/recfilter" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" ) +// Fixed refusal notes (#1863 D11). Both are stored verbatim and are on the +// stored-error allowlist (errnotes.go), so neither may carry provider text. +const ( + elastiCacheDuplicateCheckFailedNote = "ElastiCache duplicate check could not complete; no purchase was submitted; retry later, and if it persists check the account's ElastiCache reservations in AWS" + elastiCacheRecentReservationNote = "a recent ElastiCache reservation for this region, node type and a compatible engine already covers all or part of this purchase (a Redis reservation can also cover Valkey); no purchase was submitted, to avoid a duplicate; retry after 24 hours, or pick a different node type or region if more capacity is really needed" +) + // elastiCacheReservationIDPrefix mirrors the unexported prefix the elasticache // client gives a token-derived ReservedCacheNodeId (client.go:162). The parity // test drives the real client, so a rule change there fails it. @@ -35,3 +50,77 @@ func elastiCacheOwnReservationIDs(exec *config.PurchaseExecution, selected []int } return ids } + +// elastiCacheRecentDedupeClient lists without this execution's own reservations. +// It must not define FilterRecommendations: recfilter then takes the generic +// engine-aware map path, which is the one that understands cache nodes. +type elastiCacheRecentDedupeClient struct { + provider.ServiceClient + own map[string]bool + // self is this rec's own reservation ID; sawSelf records that the raw + // listing held it, so the purchase call can adopt it instead of being refused. + self string + sawSelf *bool +} + +func (c elastiCacheRecentDedupeClient) GetExistingCommitments(ctx context.Context) ([]common.Commitment, error) { + all, err := c.ServiceClient.GetExistingCommitments(ctx) + if err != nil { + return nil, err + } + for i := range all { + if c.self != "" && all[i].CommitmentID == c.self { + *c.sawSelf = true + } + } + return slices.DeleteFunc(slices.Clone(all), func(cm common.Commitment) bool { return c.own[cm.CommitmentID] }), nil +} + +// elastiCacheDedupeCopy returns the rec recfilter matches against: the library +// trims the engine when it buys, recfilter does not, so an untrimmed engine +// would buy yet be invisible to the check. Details is a pointer, so the +// CacheDetails struct is copied too and the rec that is bought is never changed. +func elastiCacheDedupeCopy(rec common.Recommendation) common.Recommendation { + if details, ok := rec.Details.(*common.CacheDetails); ok && details != nil { + d := *details + d.Engine = strings.TrimSpace(d.Engine) + rec.Details = &d + } + return rec +} + +// elastiCacheRefusalFor maps the duplicate checker's outcome to a refusal. The +// order is pinned: this rec's own reservation listed always wins (a retry must +// reach the library guard, which adopts it), then a failed check, then any +// reduction of the rec (D3), then an impossible result count. +func elastiCacheRefusalFor(passed, filtered []common.Recommendation, err error, sawSelf bool, rec common.Recommendation) error { + switch { + case sawSelf: + return nil + case err != nil: + logging.Errorf("ElastiCache duplicate check for %s in %s failed: %v", rec.ResourceType, rec.Region, err) + return beforeSubmission(errors.New(elastiCacheDuplicateCheckFailedNote)) + case len(filtered) > 0 || (len(passed) > 0 && passed[0].Count != rec.Count): + return beforeSubmission(errors.New(elastiCacheRecentReservationNote)) + case len(passed) != 1: + logging.Errorf("ElastiCache duplicate check for %s in %s returned %d passed recs, want 1", rec.ResourceType, rec.Region, len(passed)) + return beforeSubmission(errors.New(elastiCacheDuplicateCheckFailedNote)) + } + return nil +} + +// elastiCacheRecentRefusal refuses an AWS ElastiCache rec when a recent +// reservation in the client's region already covers all or part of it, and fails +// closed when the reservations cannot be listed. It returns nil for every other +// rec, for a rec with Count <= 0 (the library's own validation refuses it) and +// when this rec's own reservation (self) is already listed. A refusal is a +// beforeSubmission error: the purchase call is never made. +func elastiCacheRecentRefusal(ctx context.Context, client provider.ServiceClient, rec common.Recommendation, ownIDs map[string]bool, self string) error { + if !isAWSElastiCache(string(rec.Provider), rec.Service) || rec.Count <= 0 { + return nil + } + sawSelf := false + passed, filtered, err := recfilter.NewDuplicateChecker(0).AdjustRecommendationsForExisting( + ctx, []common.Recommendation{elastiCacheDedupeCopy(rec)}, elastiCacheRecentDedupeClient{ServiceClient: client, own: ownIDs, self: self, sawSelf: &sawSelf}) + return elastiCacheRefusalFor(passed, filtered, err, sawSelf, rec) +} diff --git a/internal/purchase/elasticache_recent_check_test.go b/internal/purchase/elasticache_recent_check_test.go new file mode 100644 index 000000000..4011f80bb --- /dev/null +++ b/internal/purchase/elasticache_recent_check_test.go @@ -0,0 +1,202 @@ +package purchase + +// #1863 S1: the ElastiCache recent-reservation check. Most cases run the real +// library elasticache.Client over the shared fake API (elasticache_fake_api_test.go), +// so the listing, the engine mapping and recfilter's generic path are the pinned ones. + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/service/elasticache/types" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func ecHeld(id, engine string, count int32, age time.Duration) types.ReservedCacheNode { + return types.ReservedCacheNode{ + ReservedCacheNodeId: aws.String(id), CacheNodeType: aws.String("cache.r6g.large"), + ProductDescription: aws.String(engine), CacheNodeCount: aws.Int32(count), + State: aws.String("active"), StartTime: aws.Time(time.Now().Add(-age)), + } +} + +func ecRec(engine string, count int) common.Recommendation { + return common.Recommendation{ + Provider: common.ProviderAWS, Service: common.ServiceCache, Region: ecRegion, + ResourceType: "cache.r6g.large", Count: count, Details: &common.CacheDetails{Engine: engine}, + } +} + +func ecCheck(t *testing.T, api *fakeElastiCacheAPI, rec common.Recommendation, own map[string]bool, self string) error { + t.Helper() + return elastiCacheRecentRefusal(context.Background(), newFakeElastiCacheClient(api), rec, own, self) +} + +func requireRefusal(t *testing.T, err error, note string) { + t.Helper() + require.Error(t, err) + assert.Equal(t, note, err.Error()) + var notSubmitted *beforeSubmissionError + assert.True(t, errors.As(err, ¬Submitted), "a refusal is a beforeSubmission error") +} + +func TestElastiCacheRecentRefusal_RecentReservationIsRefused(t *testing.T) { + api := &fakeElastiCacheAPI{held: []types.ReservedCacheNode{ecHeld("other", "redis", 2, time.Hour)}} + requireRefusal(t, ecCheck(t, api, ecRec("redis", 2), nil, ""), elastiCacheRecentReservationNote) +} + +func TestElastiCacheRecentRefusal_PartialCoverIsRefused(t *testing.T) { + api := &fakeElastiCacheAPI{held: []types.ReservedCacheNode{ecHeld("other", "redis", 2, time.Hour)}} + requireRefusal(t, ecCheck(t, api, ecRec("redis", 5), nil, ""), elastiCacheRecentReservationNote) +} + +func TestElastiCacheRecentRefusal_ListingErrorFailsClosed(t *testing.T) { + api := &fakeElastiCacheAPI{describeErr: errors.New("throttled: secret provider text")} + requireRefusal(t, ecCheck(t, api, ecRec("redis", 2), nil, ""), elastiCacheDuplicateCheckFailedNote) +} + +// Regression guard (passes on main): a listing error cannot set sawSelf, so an +// own reservation that exists still fails closed. +func TestElastiCacheRecentRefusal_ListingErrorWithOwnReservationHeldStillRefuses(t *testing.T) { + api := &fakeElastiCacheAPI{held: []types.ReservedCacheNode{ecHeld("own", "redis", 2, time.Minute)}, describeErr: errors.New("boom")} + requireRefusal(t, ecCheck(t, api, ecRec("redis", 2), map[string]bool{"own": true}, "own"), elastiCacheDuplicateCheckFailedNote) +} + +func TestElastiCacheRecentRefusal_NotRefused(t *testing.T) { + cases := map[string]struct { + held []types.ReservedCacheNode + rec common.Recommendation + }{ + "older than 24h": {[]types.ReservedCacheNode{ecHeld("old", "redis", 2, 25*time.Hour)}, ecRec("redis", 2)}, + "memcached held": {[]types.ReservedCacheNode{ecHeld("mc", "memcached", 2, time.Hour)}, ecRec("redis", 2)}, + "redis rec, valkey held": {[]types.ReservedCacheNode{ecHeld("vk", "valkey", 2, time.Hour)}, ecRec("redis", 2)}, + "nothing held": {nil, ecRec("redis", 2)}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + assert.NoError(t, ecCheck(t, &fakeElastiCacheAPI{held: tc.held}, tc.rec, nil, "")) + }) + } +} + +func TestElastiCacheRecentRefusal_OtherRecsNeverTouchTheClient(t *testing.T) { + for name, rec := range map[string]common.Recommendation{ + "azure cache": {Provider: common.ProviderAzure, Service: common.ServiceCache, Count: 1}, + "memorydb": {Provider: common.ProviderAWS, Service: common.ServiceMemoryDB, Count: 1}, + } { + t.Run(name, func(t *testing.T) { + // A nil client would panic if the gate let the rec through. + assert.NoError(t, elastiCacheRecentRefusal(context.Background(), nil, rec, nil, "")) + }) + } +} + +// A rec with Count <= 0 gets nil so the library's own validation refuses it; +// recfilter would mark it filtered and note B would be wrong for it. +func TestElastiCacheRecentRefusal_NonPositiveCountReturnsNil(t *testing.T) { + api := &fakeElastiCacheAPI{held: []types.ReservedCacheNode{ecHeld("other", "redis", 2, time.Hour)}} + for _, count := range []int{0, -1} { + assert.NoError(t, ecCheck(t, api, ecRec("redis", count), nil, "")) + } +} + +func TestElastiCacheRecentRefusal_UntrimmedEngineIsStillMatchedAndNotMutated(t *testing.T) { + api := &fakeElastiCacheAPI{held: []types.ReservedCacheNode{ecHeld("other", "redis", 2, time.Hour)}} + rec := ecRec(" redis ", 2) + + requireRefusal(t, ecCheck(t, api, rec, nil, ""), elastiCacheRecentReservationNote) + assert.Equal(t, " redis ", rec.Details.(*common.CacheDetails).Engine, "the rec that is bought keeps its engine") +} + +func TestElastiCacheRecentRefusal_UnexpectedDetailsDoNotPanic(t *testing.T) { + var typedNil *common.CacheDetails + for name, details := range map[string]common.ServiceDetails{ + "nil": nil, + "typed nil": typedNil, + "not cache": &common.ComputeDetails{}, + "value not ptr": common.CacheDetails{Engine: "redis"}, + } { + t.Run(name, func(t *testing.T) { + rec := ecRec("redis", 2) + rec.Details = details + assert.NotPanics(t, func() { _ = ecCheck(t, &fakeElastiCacheAPI{}, rec, nil, "") }) + }) + } +} + +func TestElastiCacheRecentRefusal_OwnReservationsAreDroppedFromTheListing(t *testing.T) { + api := &fakeElastiCacheAPI{held: []types.ReservedCacheNode{ecHeld("sibling", "redis", 2, time.Minute)}} + assert.NoError(t, ecCheck(t, api, ecRec("redis", 2), map[string]bool{"sibling": true}, "mine")) +} + +// Regression guard (passes on main): the own reservation listed wins over +// another lineage's overlapping reservation, whole or partial. +func TestElastiCacheRecentRefusal_OwnReservationListedWinsOverForeignOverlap(t *testing.T) { + for name, foreign := range map[string]int32{"whole": 5, "partial": 1} { + t.Run(name, func(t *testing.T) { + api := &fakeElastiCacheAPI{held: []types.ReservedCacheNode{ + ecHeld("own", "redis", 2, time.Minute), ecHeld("foreign", "redis", foreign, time.Hour)}} + assert.NoError(t, ecCheck(t, api, ecRec("redis", 5), map[string]bool{"own": true}, "own")) + }) + } +} + +// The scan covers the raw listing, so an own reservation outside the 24h window +// still sets sawSelf. +func TestElastiCacheRecentRefusal_OwnReservationOutsideLookbackStillWins(t *testing.T) { + api := &fakeElastiCacheAPI{held: []types.ReservedCacheNode{ + ecHeld("own", "redis", 2, 48*time.Hour), ecHeld("foreign", "redis", 2, time.Hour)}} + assert.NoError(t, ecCheck(t, api, ecRec("redis", 2), map[string]bool{"own": true}, "own")) +} + +func TestElastiCacheRecentRefusal_EmptySelfIsNotOwn(t *testing.T) { + api := &fakeElastiCacheAPI{held: []types.ReservedCacheNode{ecHeld("", "redis", 2, time.Hour)}} + requireRefusal(t, ecCheck(t, api, ecRec("redis", 2), nil, ""), elastiCacheRecentReservationNote) +} + +// recfilter takes its engine-aware generic path only when the client has no +// FilterRecommendationsForRecentCommitments method. +func TestElastiCacheRecentDedupeClient_HasNoRecentCommitmentFilter(t *testing.T) { + _, ok := any(elastiCacheRecentDedupeClient{}).(recentCommitmentFilter) + assert.False(t, ok) + _, ok = any(&elastiCacheRecentDedupeClient{}).(recentCommitmentFilter) + assert.False(t, ok) +} + +func TestElastiCacheRefusalFor_DecisionOrder(t *testing.T) { + rec := ecRec("redis", 5) + full, partial := []common.Recommendation{ecRec("redis", 5)}, []common.Recommendation{ecRec("redis", 3)} + boom := errors.New("boom") + cases := map[string]struct { + passed, filtered []common.Recommendation + err error + sawSelf bool + want string + }{ + "clean": {passed: full}, + "self wins over filtered": {filtered: full, sawSelf: true}, + "self wins over partial": {passed: partial, sawSelf: true}, + "self wins over error": {err: boom, sawSelf: true}, + "filtered": {filtered: full, want: elastiCacheRecentReservationNote}, + "partial count": {passed: partial, want: elastiCacheRecentReservationNote}, + "error": {passed: full, err: boom, want: elastiCacheDuplicateCheckFailedNote}, + "error wins over filtered": {filtered: full, err: boom, want: elastiCacheDuplicateCheckFailedNote}, + "nothing passed or filtered": {want: elastiCacheDuplicateCheckFailedNote}, + "two passed (defensive branch)": {passed: append(append([]common.Recommendation{}, full...), full...), want: elastiCacheDuplicateCheckFailedNote}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + err := elastiCacheRefusalFor(tc.passed, tc.filtered, tc.err, tc.sawSelf, rec) + if tc.want == "" { + assert.NoError(t, err) + return + } + requireRefusal(t, err, tc.want) + }) + } +} diff --git a/internal/purchase/errnotes.go b/internal/purchase/errnotes.go index 98efa74c1..78ad4e869 100644 --- a/internal/purchase/errnotes.go +++ b/internal/purchase/errnotes.go @@ -46,6 +46,8 @@ var serverNotePatterns = []string{ regexp.QuoteMeta(redshiftSettleRefusal), regexp.QuoteMeta(gcpDuplicateCheckFailedNote), regexp.QuoteMeta(gcpRecentCommitmentNote), + regexp.QuoteMeta(elastiCacheDuplicateCheckFailedNote), + regexp.QuoteMeta(elastiCacheRecentReservationNote), `EC2 recommendation for ` + resourceTypePattern + ` has zonal scope but no AvailabilityZone: refusing to buy a zonal RI in an arbitrary AZ; re-fetch the recommendation`, `zonal RI offering needs an availability zone; re-fetch the recommendation`, `email notifier not configured for this deployment`, diff --git a/internal/purchase/errnotes_test.go b/internal/purchase/errnotes_test.go index a27c3c8d2..a6debd96c 100644 --- a/internal/purchase/errnotes_test.go +++ b/internal/purchase/errnotes_test.go @@ -66,6 +66,8 @@ func TestSanitizeStoredError_AllowlistedNotesPassThroughVerbatim(t *testing.T) { redshiftSettleRefusal, gcpDuplicateCheckFailedNote, gcpRecentCommitmentNote, + elastiCacheDuplicateCheckFailedNote, + elastiCacheRecentReservationNote, "EC2 recommendation for m5.large has zonal scope but no AvailabilityZone: refusing to buy a zonal RI in an arbitrary AZ; re-fetch the recommendation", "zonal RI offering needs an availability zone; re-fetch the recommendation", "email notifier not configured for this deployment", @@ -218,7 +220,7 @@ func TestSanitizeStoredError_ResolveNoteExchangeIDIsClosed(t *testing.T) { // The Redshift gate texts are fixed: an appended execution id or raw text must // not pass verbatim. func TestSanitizeStoredError_RedshiftGateRefusalsAreClosed(t *testing.T) { - for _, text := range []string{redshiftConflictRefusal, redshiftHorizonRefusal, redshiftNoStampRefusal, redshiftFailedConflictRefusal, redshiftNoJournalRefusal, redshiftRecentBuyRefusal, redshiftSettleRefusal, gcpDuplicateCheckFailedNote, gcpRecentCommitmentNote} { + for _, text := range []string{redshiftConflictRefusal, redshiftHorizonRefusal, redshiftNoStampRefusal, redshiftFailedConflictRefusal, redshiftNoJournalRefusal, redshiftRecentBuyRefusal, redshiftSettleRefusal, gcpDuplicateCheckFailedNote, gcpRecentCommitmentNote, elastiCacheDuplicateCheckFailedNote, elastiCacheRecentReservationNote} { assert.Equal(t, text, SanitizeStoredError(text)) for _, variant := range []string{ text + " (execution 00000000-0000-0000-0000-000000000000, running)",