From b64725a32af27e7beece87ec61230346d195af18 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 12 Oct 2026 16:34:37 +0200 Subject: [PATCH 1/3] fix(purchase): make zonal EC2 AZ part of the guard, identity key, idempotency key and rec ID Zonal EC2 recs now keep their availability zone (#805) but three purchase paths ignored it: - computeDetailMismatch refuses a re-matched rec whose AZ differs from, or is unknown against, a known origin AZ. - recIdentityKey adds the AZ so the stored-rec index and pricing resolve the requested zone instead of an arbitrary one. - the 2-minute submit idempotency key reads the AZ from the rec Details (also present on rows written before this change), so two submits that differ only by AZ no longer collapse into one execution. A candidate whose key cannot be derived is skipped with an error log. The scheduler stamps the new RecommendationRecord.AvailabilityZone and appends it to the rec ID only for zonal recs (regional IDs unchanged). AZ names are per account, so zonal EC2 recs that Cost Explorer computed for a member account other than the collecting one are dropped with a warning. Refs #792 Co-Authored-By: Claude Sonnet 5.5 --- internal/api/handler_purchases.go | 53 +++++- internal/api/handler_purchases_guards_test.go | 26 ++- ...urchases_submit_dedupe_integration_test.go | 4 +- internal/api/purchase_az_test.go | 174 ++++++++++++++++++ internal/api/purchase_pricing.go | 15 +- internal/config/types.go | 10 +- .../scheduler/partial_sweep_eviction_test.go | 2 +- internal/scheduler/scheduler.go | 53 ++++-- internal/scheduler/scheduler_test.go | 4 +- internal/scheduler/zonal_recs.go | 44 +++++ internal/scheduler/zonal_recs_test.go | 59 ++++++ 11 files changed, 401 insertions(+), 43 deletions(-) create mode 100644 internal/api/purchase_az_test.go create mode 100644 internal/scheduler/zonal_recs.go create mode 100644 internal/scheduler/zonal_recs_test.go diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 9e8ec54d..16260e9d 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -2770,7 +2770,15 @@ func matchDuplicateInList(pending []config.PurchaseExecution, creatorID, idempot if ex.CreatedByUserID != nil { exCreator = *ex.CreatedByUserID } - if exCreator == creatorID && purchaseIdempotencyKey(exCreator, ex.Recommendations, ex.CapacityPercent) == idempotencyKey { + if exCreator != creatorID { + continue + } + exKey, err := purchaseIdempotencyKey(exCreator, ex.Recommendations, ex.CapacityPercent) + if err != nil { + logging.Errorf("purchase idempotency: skipping pending execution %s, cannot derive its key: %v", ex.ExecutionID, err) + continue + } + if exKey == idempotencyKey { return ex } } @@ -2836,7 +2844,7 @@ const purchaseIdempotencyWindow = 2 * time.Minute // ordering can't change the hash. Account scope is implicit: each rec carries // its CloudAccountID, so the same recs targeting a different account hash // differently. Closes issue #644. -func purchaseIdempotencyKey(creatorID string, recs []config.RecommendationRecord, capacityPercent int) string { +func purchaseIdempotencyKey(creatorID string, recs []config.RecommendationRecord, capacityPercent int) (string, error) { tuples := make([]string, 0, len(recs)) for _rvc := range recs { r := recs[_rvc] @@ -2844,15 +2852,38 @@ func purchaseIdempotencyKey(creatorID string, recs []config.RecommendationRecord if r.CloudAccountID != nil { acct = *r.CloudAccountID } - tuples = append(tuples, fmt.Sprintf("%s|%s|%s|%s|%s|%s|%d|%d|%s", + az, err := recDetailsAvailabilityZone(&r) + if err != nil { + return "", err + } + tuples = append(tuples, fmt.Sprintf("%s|%s|%s|%s|%s|%s|%d|%d|%s|%s", strings.ToLower(strings.TrimSpace(r.Provider)), r.Service, r.Region, r.ResourceType, r.Engine, acct, - r.Count, r.Term, strings.ToLower(strings.TrimSpace(r.Payment)))) + r.Count, r.Term, strings.ToLower(strings.TrimSpace(r.Payment)), az)) } sort.Strings(tuples) h := sha256.New() fmt.Fprintf(h, "%s\x1f%d\x1f%s", creatorID, capacityPercent, strings.Join(tuples, "\x1e")) - return hex.EncodeToString(h.Sum(nil)) + return hex.EncodeToString(h.Sum(nil)), nil +} + +// recDetailsAvailabilityZone returns the zonal EC2 AZ carried in the rec's +// Details blob, "" for non-compute recs and for absent or legacy Details. +// The AZ is read from Details, not RecommendationRecord.AvailabilityZone, +// because pending executions written before issue #792 carry it only there +// and the idempotency key is recomputed from them (issue #792). +func recDetailsAvailabilityZone(r *config.RecommendationRecord) (string, error) { + if r.Service != string(common.ServiceEC2) && r.Service != string(common.ServiceCompute) { + return "", nil + } + details, err := common.DecodeServiceDetailsFor(r.Service, r.Details) + if err != nil { + return "", fmt.Errorf("decode %s details of %s/%s: %w", r.Service, r.Region, r.ResourceType, err) + } + if cd, ok := details.(*common.ComputeDetails); ok && cd != nil { + return strings.TrimSpace(cd.AvailabilityZone), nil + } + return "", nil } // findDuplicatePendingExecution returns an existing pending/notified execution @@ -2884,7 +2915,12 @@ func (h *Handler) findDuplicatePendingExecution(ctx context.Context, creatorID, if exCreator != creatorID { continue } - if purchaseIdempotencyKey(exCreator, ex.Recommendations, ex.CapacityPercent) == key { + exKey, err := purchaseIdempotencyKey(exCreator, ex.Recommendations, ex.CapacityPercent) + if err != nil { + logging.Errorf("purchase idempotency: skipping pending execution %s, cannot derive its key: %v", ex.ExecutionID, err) + continue + } + if exKey == key { return ex, nil } } @@ -2962,7 +2998,10 @@ func (h *Handler) executePurchase(ctx context.Context, req *events.LambdaFunctio // which closes the TOCTOU race that a pre-tx read alone cannot prevent. creator := resolveCreatorUserID(session) creatorID := derefStringOrEmpty(creator) - idempotencyKey := purchaseIdempotencyKey(creatorID, execReq.Recommendations, execReq.CapacityPercent) + idempotencyKey, err := purchaseIdempotencyKey(creatorID, execReq.Recommendations, execReq.CapacityPercent) + if err != nil { + return nil, fmt.Errorf("derive purchase idempotency key: %w", err) + } execution, rawApprovalToken, err := newPendingExecution(&execReq, totalUpfront, totalSavings) if err != nil { diff --git a/internal/api/handler_purchases_guards_test.go b/internal/api/handler_purchases_guards_test.go index 8684fdb1..36e1e395 100644 --- a/internal/api/handler_purchases_guards_test.go +++ b/internal/api/handler_purchases_guards_test.go @@ -464,27 +464,27 @@ func TestPurchaseIdempotencyKey_StableAndDiscriminating(t *testing.T) { reordered := []config.RecommendationRecord{r2, validRec()} assert.Equal(t, - purchaseIdempotencyKey("user-1", recsA, 100), - purchaseIdempotencyKey("user-1", recsA, 100), + mustPurchaseIdempotencyKey("user-1", recsA, 100), + mustPurchaseIdempotencyKey("user-1", recsA, 100), "identical input must hash identically") assert.Equal(t, - purchaseIdempotencyKey("user-1", ordered, 100), - purchaseIdempotencyKey("user-1", reordered, 100), + mustPurchaseIdempotencyKey("user-1", ordered, 100), + mustPurchaseIdempotencyKey("user-1", reordered, 100), "slice order must not change the key") // Discriminating dimensions. - assert.NotEqual(t, purchaseIdempotencyKey("user-1", recsA, 100), purchaseIdempotencyKey("user-2", recsA, 100), "creator") - assert.NotEqual(t, purchaseIdempotencyKey("user-1", recsA, 100), purchaseIdempotencyKey("user-1", recsA, 50), "capacity") + assert.NotEqual(t, mustPurchaseIdempotencyKey("user-1", recsA, 100), mustPurchaseIdempotencyKey("user-2", recsA, 100), "creator") + assert.NotEqual(t, mustPurchaseIdempotencyKey("user-1", recsA, 100), mustPurchaseIdempotencyKey("user-1", recsA, 50), "capacity") scaled := []config.RecommendationRecord{validRec()} scaled[0].Count = 1 - assert.NotEqual(t, purchaseIdempotencyKey("user-1", recsA, 100), purchaseIdempotencyKey("user-1", scaled, 100), "count") + assert.NotEqual(t, mustPurchaseIdempotencyKey("user-1", recsA, 100), mustPurchaseIdempotencyKey("user-1", scaled, 100), "count") acctA := []config.RecommendationRecord{validRec()} acctA[0].CloudAccountID = strptr("acct-A") acctB := []config.RecommendationRecord{validRec()} acctB[0].CloudAccountID = strptr("acct-B") - assert.NotEqual(t, purchaseIdempotencyKey("user-1", acctA, 100), purchaseIdempotencyKey("user-1", acctB, 100), "account") + assert.NotEqual(t, mustPurchaseIdempotencyKey("user-1", acctA, 100), mustPurchaseIdempotencyKey("user-1", acctB, 100), "account") } func TestFindDuplicatePendingExecution(t *testing.T) { @@ -492,7 +492,7 @@ func TestFindDuplicatePendingExecution(t *testing.T) { now := time.Now() creator := "11111111-1111-1111-1111-111111111111" recs := []config.RecommendationRecord{validRec()} - key := purchaseIdempotencyKey(creator, recs, 100) + key := mustPurchaseIdempotencyKey(creator, recs, 100) makeExec := func(id string, age time.Duration, src string, c *string, capacity int) config.PurchaseExecution { // Copy recs so a subtest that mutates exec.Recommendations does not @@ -646,3 +646,11 @@ func TestValidateCapacityConsistency(t *testing.T) { }) } } + +func mustPurchaseIdempotencyKey(creatorID string, recs []config.RecommendationRecord, capacityPercent int) string { + key, err := purchaseIdempotencyKey(creatorID, recs, capacityPercent) + if err != nil { + panic(err) + } + return key +} diff --git a/internal/api/handler_purchases_submit_dedupe_integration_test.go b/internal/api/handler_purchases_submit_dedupe_integration_test.go index d921aa26..22ea060f 100644 --- a/internal/api/handler_purchases_submit_dedupe_integration_test.go +++ b/internal/api/handler_purchases_submit_dedupe_integration_test.go @@ -49,7 +49,7 @@ func submitFixture(t *testing.T) ([]config.RecommendationRecord, string) { Provider: "aws", Service: "ec2", Region: "us-east-1", ResourceType: "m5.large", Count: 2, Term: 1, Payment: "no-upfront", }} - return recs, purchaseIdempotencyKey("", recs, 100) + return recs, mustPurchaseIdempotencyKey("", recs, 100) } func newSubmitExecution(t *testing.T, recs []config.RecommendationRecord) *config.PurchaseExecution { @@ -171,7 +171,7 @@ func TestPersistExecution_DedupeIsPerCreator(t *testing.T) { submit := func(creator string) *config.PurchaseExecution { exec := newSubmitExecution(t, recs) exec.CreatedByUserID = &creator - dup, err := h.persistExecutionAndSuppressions(ctx, exec, nil, creator, purchaseIdempotencyKey(creator, recs, 100)) + dup, err := h.persistExecutionAndSuppressions(ctx, exec, nil, creator, mustPurchaseIdempotencyKey(creator, recs, 100)) require.NoError(t, err) return dup } diff --git a/internal/api/purchase_az_test.go b/internal/api/purchase_az_test.go new file mode 100644 index 00000000..fa2c7936 --- /dev/null +++ b/internal/api/purchase_az_test.go @@ -0,0 +1,174 @@ +package api + +import ( + "context" + "encoding/json" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Real zonal EC2 Details shape as persisted by the scheduler (issue #792). +func zonalEC2Details(az string) json.RawMessage { + return json.RawMessage(`{"instance_type":"m5.large","platform":"Linux/UNIX","tenancy":"default","scope":"availability-zone","availability_zone":"` + az + `"}`) +} + +func zonalEC2Rec(az string, upfront float64) config.RecommendationRecord { + return config.RecommendationRecord{ + ID: "rec-" + az, Provider: "aws", Service: "ec2", Region: "us-east-1", ResourceType: "m5.large", + Term: 1, Payment: "all-upfront", Count: 1, UpfrontCost: upfront, Savings: 10, + AvailabilityZone: az, Details: zonalEC2Details(az), + } +} + +func TestPurchaseIdempotencyKey_DiffersByAvailabilityZone(t *testing.T) { + t.Parallel() + a := []config.RecommendationRecord{zonalEC2Rec("us-east-1a", 100)} + b := []config.RecommendationRecord{zonalEC2Rec("us-east-1b", 100)} + assert.NotEqual(t, mustPurchaseIdempotencyKey("u", a, 100), mustPurchaseIdempotencyKey("u", b, 100), + "two submits differing only by AZ must not dedupe into one") + assert.Equal(t, mustPurchaseIdempotencyKey("u", a, 100), mustPurchaseIdempotencyKey("u", a, 100)) +} + +func TestPurchaseIdempotencyKey_LegacyRecWithoutTopLevelAZMatchesNewShape(t *testing.T) { + t.Parallel() + newShape := zonalEC2Rec("us-east-1a", 100) + legacy := newShape + legacy.AvailabilityZone = "" // pending row written before the field existed: AZ only in Details + assert.Equal(t, + mustPurchaseIdempotencyKey("u", []config.RecommendationRecord{newShape}, 100), + mustPurchaseIdempotencyKey("u", []config.RecommendationRecord{legacy}, 100), + "a double-submit straddling the deploy must still dedupe") +} + +func TestPurchaseIdempotencyKey_MalformedComputeDetailsErrorsAndCandidateIsSkipped(t *testing.T) { + t.Parallel() + bad := zonalEC2Rec("us-east-1a", 100) + bad.Details = json.RawMessage(`{"availability_zone":`) + _, err := purchaseIdempotencyKey("u", []config.RecommendationRecord{bad}, 100) + require.Error(t, err) + + now := time.Now() + good := []config.RecommendationRecord{zonalEC2Rec("us-east-1a", 100)} + key := mustPurchaseIdempotencyKey("u", good, 100) + creator := "u" + pending := []config.PurchaseExecution{ + {ExecutionID: "bad", Source: common.PurchaseSourceWeb, ScheduledDate: now, CreatedByUserID: &creator, Recommendations: []config.RecommendationRecord{bad}, CapacityPercent: 100}, + {ExecutionID: "good", Source: common.PurchaseSourceWeb, ScheduledDate: now, CreatedByUserID: &creator, Recommendations: good, CapacityPercent: 100}, + } + got := matchDuplicateInList(pending, creator, key, now) + require.NotNil(t, got) + assert.Equal(t, "good", got.ExecutionID) +} + +func TestPurchaseIdempotencyKey_NoDetailsIsEmptyAZ(t *testing.T) { + t.Parallel() + r := config.RecommendationRecord{Provider: "aws", Service: "ec2", Region: "us-east-1", ResourceType: "m5.large", Count: 1, Term: 1, Payment: "all-upfront"} + _, err := purchaseIdempotencyKey("u", []config.RecommendationRecord{r}, 100) + require.NoError(t, err) +} + +func TestCheckPurchaseDetailIdentity_AvailabilityZoneMismatchRefused(t *testing.T) { + for name, matchAZ := range map[string]string{"different az": "us-east-1b", "unknown az": ""} { + t.Run(name, func(t *testing.T) { + origin := zonalEC2Rec("us-east-1a", 100) + match := zonalEC2Rec("us-east-1b", 100) + match.ID = "other" + if matchAZ == "" { + match.Details = json.RawMessage(`{"instance_type":"m5.large","platform":"Linux/UNIX","tenancy":"default","scope":"availability-zone"}`) + } + req := config.RecommendationRecord{ID: origin.ID, Service: "ec2", Term: 1, Count: 1} + err := checkPurchaseDetailIdentity(&origin, &match, &req, 0) + require.Error(t, err) + assert.Contains(t, err.Error(), "availability_zone") + ce, isClient := IsClientError(err) + require.True(t, isClient) + assert.Equal(t, 409, ce.code) + }) + } +} + +func TestCheckPurchaseDetailIdentity_LegacyOriginWithoutAZAllowed(t *testing.T) { + origin := zonalEC2Rec("us-east-1a", 100) + origin.Details = json.RawMessage(`{"instance_type":"m5.large","platform":"Linux/UNIX","tenancy":"default","scope":"availability-zone"}`) + match := zonalEC2Rec("us-east-1b", 100) + match.ID = "other" + req := config.RecommendationRecord{ID: origin.ID, Service: "ec2", Term: 1, Count: 1} + require.NoError(t, checkPurchaseDetailIdentity(&origin, &match, &req, 0)) +} + +func TestRecIdentityKey_DiffersByAvailabilityZone(t *testing.T) { + a, b := zonalEC2Rec("us-east-1a", 1), zonalEC2Rec("us-east-1b", 1) + assert.NotEqual(t, recIdentityKey(&a), recIdentityKey(&b)) +} + +// Two zonal rows that differ only by AZ must both load (no "more than one row" +// refusal) and each request must be priced from its own AZ's row. +func TestPriceRecommendationsFromStore_PricesFromRequestedAZ(t *testing.T) { + ctx := context.Background() + store := new(MockConfigStore) + t.Cleanup(func() { store.AssertExpectations(t) }) + expectStoredRecs(store, zonalEC2Rec("us-east-1a", 100), zonalEC2Rec("us-east-1b", 900)) + h := &Handler{config: store} + + req := []config.RecommendationRecord{{ + Provider: "aws", Service: "ec2", Region: "us-east-1", ResourceType: "m5.large", + Term: 1, Payment: "all-upfront", Count: 1, AvailabilityZone: "us-east-1b", + }} + require.NoError(t, h.priceRecommendationsFromStore(ctx, req)) + assert.Equal(t, 900.0, req[0].UpfrontCost) + assert.Equal(t, "us-east-1b", req[0].AvailabilityZone) +} + +func TestPriceRecommendationsFromStore_ZonalRequestWithoutAZRefused(t *testing.T) { + ctx := context.Background() + store := new(MockConfigStore) + t.Cleanup(func() { store.AssertExpectations(t) }) + expectStoredRecs(store, zonalEC2Rec("us-east-1a", 100)) + h := &Handler{config: store} + + req := []config.RecommendationRecord{{ + Provider: "aws", Service: "ec2", Region: "us-east-1", ResourceType: "m5.large", + Term: 1, Payment: "all-upfront", Count: 1, + }} + err := h.priceRecommendationsFromStore(ctx, req) + require.Error(t, err) + ce, isClient := IsClientError(err) + require.True(t, isClient) + assert.Equal(t, 409, ce.code) +} + +// No-ID path: the request's stated Details AZ must agree with the matched row, +// whether the top-level AZ is empty (no match) or names another AZ. +func TestPriceRecommendationsFromStore_NoIDDetailsAZMismatchRefused(t *testing.T) { + for name, topLevel := range map[string]string{"empty top-level": "", "other top-level": "us-east-1b"} { + t.Run(name, func(t *testing.T) { + ctx := context.Background() + store := new(MockConfigStore) + t.Cleanup(func() { store.AssertExpectations(t) }) + expectStoredRecs(store, zonalEC2Rec("us-east-1a", 100), zonalEC2Rec("us-east-1b", 900)) + h := &Handler{config: store} + req := []config.RecommendationRecord{{ + Provider: "aws", Service: "ec2", Region: "us-east-1", ResourceType: "m5.large", + Term: 1, Payment: "all-upfront", Count: 1, + AvailabilityZone: topLevel, Details: zonalEC2Details("us-east-1a"), + }} + err := h.priceRecommendationsFromStore(ctx, req) + require.Error(t, err) + ce, isClient := IsClientError(err) + require.True(t, isClient) + assert.Equal(t, 409, ce.code) + }) + } +} + +func TestDescribeRec_IncludesAvailabilityZone(t *testing.T) { + r := zonalEC2Rec("us-east-1a", 1) + assert.Contains(t, describeRec(&r), "az=us-east-1a") + r.AvailabilityZone = "" + assert.NotContains(t, describeRec(&r), "az=") +} diff --git a/internal/api/purchase_pricing.go b/internal/api/purchase_pricing.go index 02f18e8d..0b1e25e0 100644 --- a/internal/api/purchase_pricing.go +++ b/internal/api/purchase_pricing.go @@ -116,8 +116,9 @@ func (h *Handler) loadStoredRecommendationIndex(ctx context.Context, recs []conf return byKey, byID, nil } -// recIdentityKey is the tuple a price is a function of: the same eight -// components the scheduler encodes into RecommendationRecord.ID, with a nil +// recIdentityKey is the tuple a price is a function of: the same nine +// components (the ninth is the zonal EC2 availability zone, "" for every +// other rec; issue #792) behind RecommendationRecord.ID, with a nil // and an empty CloudAccountID collapsed together (as purchaseConstraintSets // and the store's account_key both do). Provider and payment are lowercased // because validatePurchaseRecommendation canonicalises the request side. @@ -129,6 +130,7 @@ func recIdentityKey(rec *config.RecommendationRecord) string { return strings.Join([]string{ strings.ToLower(rec.Provider), account, rec.Service, rec.Region, rec.ResourceType, rec.Engine, strconv.Itoa(rec.Term), strings.ToLower(rec.Payment), + rec.AvailabilityZone, }, "\x1f") } @@ -322,6 +324,9 @@ func computeDetailMismatch(o, m *common.ComputeDetails) string { mismatchField("tenancy", o.Tenancy, m.Tenancy), mismatchField("platform", o.Platform, m.Platform), mismatchField("scope", o.Scope, m.Scope), + // Zonal EC2 reservations are bought in one AZ; a match in another + // AZ (or with an unknown one) is a different commitment (issue #792). + mismatchField("availability_zone", o.AvailabilityZone, m.AvailabilityZone), // GCP custom machine types (Compute Engine CUDs) read MemoryGB at // purchase time to build the machine spec, so a memory mismatch is // as purchase-critical as tenancy for that provider. @@ -361,6 +366,10 @@ func formatOptionalCost(v *float64) string { } func describeRec(rec *config.RecommendationRecord) string { - return fmt.Sprintf("%s/%s %s %s engine=%q term=%d payment=%s account=%s", + desc := fmt.Sprintf("%s/%s %s %s engine=%q term=%d payment=%s account=%s", rec.Provider, rec.Service, rec.Region, rec.ResourceType, rec.Engine, rec.Term, rec.Payment, derefStringOrEmpty(rec.CloudAccountID)) + if rec.AvailabilityZone != "" { + desc += " az=" + rec.AvailabilityZone + } + return desc } diff --git a/internal/config/types.go b/internal/config/types.go index b1a9a209..376ebb44 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -529,7 +529,15 @@ type RecommendationRecord struct { // carry the full Details, so non-default platforms (Windows EC2, // Postgres RDS, etc.) round-trip correctly. Details json.RawMessage `json:"details,omitempty" dynamodbav:"-"` - Count int `json:"count" dynamodbav:"count"` + // AvailabilityZone is the zonal EC2 reservation's AZ name (e.g. + // "us-east-1a"), empty for regional and non-EC2 recs. It mirrors + // ComputeDetails.AvailabilityZone inside Details (config must not import + // pkg/common) so the identity tuple, the rec ID and the recommendations + // natural key can distinguish two zonal recs that differ only by AZ + // (issue #792). The scheduler stamps it at collection time; request recs + // echo it from the listed rec. + AvailabilityZone string `json:"availability_zone,omitempty" dynamodbav:"availability_zone,omitempty"` + Count int `json:"count" dynamodbav:"count"` // RecommendedCount is the pre-scaling count the collector originally // recommended, before the bulk-purchase Capacity % slider scaled it down. // The web execute path stamps it so the backend can verify the diff --git a/internal/scheduler/partial_sweep_eviction_test.go b/internal/scheduler/partial_sweep_eviction_test.go index dd1e69cd..19e59b81 100644 --- a/internal/scheduler/partial_sweep_eviction_test.go +++ b/internal/scheduler/partial_sweep_eviction_test.go @@ -73,7 +73,7 @@ func TestAWSIncompleteFallbackCannotAuthorizeEviction(t *testing.T) { prov := new(MockProvider) prov.On("GetRecommendationsClient", mock.Anything).Return(client, nil).Once() s := &Scheduler{} - recs, complete, err := s.fetchAndConvert(context.Background(), prov, "aws", nil, + recs, complete, err := s.fetchAndConvert(context.Background(), prov, "aws", nil, "", &config.GlobalConfig{DefaultTerm: 1, DefaultPayment: "no-upfront", RecommendationsLookbackDays: 30}) require.NoError(t, err) require.Len(t, recs, 1) diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index c27c072b..9dd669cd 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -648,7 +648,26 @@ func (s *Scheduler) collectAWSAmbient(ctx context.Context, globalCfg *config.Glo if err != nil { return nil, false, fmt.Errorf("failed to create AWS provider: %w", err) } - return s.fetchAndConvert(ctx, prov, "aws", nil, globalCfg) + return s.fetchAndConvert(ctx, prov, "aws", nil, s.hostAWSAccountID(ctx), globalCfg) +} + +// hostAWSAccountID returns the Lambda's own AWS account ID via STS +// GetCallerIdentity, or "" when STS is unavailable or fails (logged at warn). +func (s *Scheduler) hostAWSAccountID(ctx context.Context) string { + if s.stsClient == nil { + return "" + } + stsCtx, cancel := context.WithTimeout(ctx, 3*time.Second) + defer cancel() + out, err := s.stsClient.GetCallerIdentity(stsCtx, &sts.GetCallerIdentityInput{}) + if err != nil { + logging.Warnf("ambient host-account lookup: STS GetCallerIdentity failed: %v", err) + return "" + } + if out == nil || out.Account == nil { + return "" + } + return *out.Account } // resolveAmbientHostAccountID looks up the Lambda's own AWS account ID @@ -662,20 +681,10 @@ func (s *Scheduler) collectAWSAmbient(ctx context.Context, globalCfg *config.Glo // is a best-effort UX improvement on the ambient path and must not // break the collection. func (s *Scheduler) resolveAmbientHostAccountID(ctx context.Context) string { - if s.stsClient == nil { - return "" - } - stsCtx, cancel := context.WithTimeout(ctx, 3*time.Second) - defer cancel() - out, err := s.stsClient.GetCallerIdentity(stsCtx, &sts.GetCallerIdentityInput{}) - if err != nil { - logging.Warnf("ambient host-account lookup: STS GetCallerIdentity failed: %v", err) + hostAcctID := s.hostAWSAccountID(ctx) + if hostAcctID == "" { return "" } - if out == nil || out.Account == nil || *out.Account == "" { - return "" - } - hostAcctID := *out.Account acct, err := s.config.GetCloudAccountByExternalID(ctx, "aws", hostAcctID) if err != nil { @@ -778,7 +787,7 @@ func (s *Scheduler) collectAWSForAccount(ctx context.Context, globalCfg *config. if err != nil { return nil, false, fmt.Errorf("create ambient provider: %w", err) } - return s.fetchAndConvert(ctx, prov, "aws", &acct.ID, globalCfg) + return s.fetchAndConvert(ctx, prov, "aws", &acct.ID, acct.ExternalID, globalCfg) } awsCreds, err := credentials.ResolveAWSCredentialProvider(ctx, &acct, s.credStore, s.assumeRoleSTS) if err != nil { @@ -789,7 +798,7 @@ func (s *Scheduler) collectAWSForAccount(ctx context.Context, globalCfg *config. if err != nil { return nil, false, fmt.Errorf("create provider: %w", err) } - return s.fetchAndConvert(ctx, prov, "aws", &acct.ID, globalCfg) + return s.fetchAndConvert(ctx, prov, "aws", &acct.ID, acct.ExternalID, globalCfg) } // collectAzureRecommendations fans out across all enabled Azure accounts, @@ -1024,7 +1033,7 @@ func tolerateIncompleteSweep(providerName string, err error) (complete bool, _ e // The bool reports whether the sweep covered everything it was asked to; see // tolerateIncompleteSweep for why an incomplete one must not authorize // stale-row eviction. -func (s *Scheduler) fetchAndConvert(ctx context.Context, prov provider.Provider, providerName string, accountID *string, globalCfg *config.GlobalConfig) ([]config.RecommendationRecord, bool, error) { +func (s *Scheduler) fetchAndConvert(ctx context.Context, prov provider.Provider, providerName string, accountID *string, expectedAccount string, globalCfg *config.GlobalConfig) ([]config.RecommendationRecord, bool, error) { recClient, err := prov.GetRecommendationsClient(ctx) if err != nil { return nil, false, fmt.Errorf("failed to get %s recommendations client: %w", providerName, err) @@ -1034,6 +1043,7 @@ func (s *Scheduler) fetchAndConvert(ctx context.Context, prov provider.Provider, if err != nil { return nil, false, fmt.Errorf("failed to get %s recommendations: %w", providerName, err) } + recs = dropForeignZonalEC2(recs, expectedAccount) if len(recs) == 0 && globalCfg != nil { lookbackDays := globalCfg.RecommendationsLookbackDays if lookbackDays == 0 { @@ -1052,7 +1062,7 @@ func (s *Scheduler) fetchAndConvert(ctx context.Context, prov provider.Provider, return nil, false, fmt.Errorf("failed to get %s recommendations with default term/payment fallback (service=%s, term=%s, payment=%s, lookback=%s): %w", providerName, params.Service, params.Term, params.PaymentOption, params.LookbackPeriod, recErr) } - recs = append(recs, fallbackRecs...) + recs = append(recs, dropForeignZonalEC2(fallbackRecs, expectedAccount)...) complete = complete && fallbackComplete } } @@ -1078,9 +1088,15 @@ func recommendationID(rec config.RecommendationRecord) string { if rec.CloudAccountID != nil { accountID = *rec.CloudAccountID } - return fmt.Sprintf("%s|%s|%s|%s|%s|%s|%d|%s", + id := fmt.Sprintf("%s|%s|%s|%s|%s|%s|%d|%s", rec.Provider, accountID, rec.Service, rec.Region, rec.ResourceType, rec.Engine, rec.Term, rec.Payment) + if rec.AvailabilityZone != "" { + // Appended only for zonal EC2 recs so every other rec keeps its + // existing ID (issue #792). + id += "|" + rec.AvailabilityZone + } + return id } // ListRecommendations reads cached recommendations from the store, applying @@ -1587,6 +1603,7 @@ func (s *Scheduler) convertRecommendations(recs []common.Recommendation, provide ResourceType: rec.ResourceType, Engine: engine, Details: detailsBlob, // full ServiceDetails payload (issue #453) + AvailabilityZone: zonalAvailabilityZone(rec.Details), Count: rec.Count, Term: term, Payment: rec.PaymentOption, diff --git a/internal/scheduler/scheduler_test.go b/internal/scheduler/scheduler_test.go index e14d2168..58033fbe 100644 --- a/internal/scheduler/scheduler_test.go +++ b/internal/scheduler/scheduler_test.go @@ -755,7 +755,7 @@ func TestScheduler_FetchAndConvert_KeepsPartialSweepData(t *testing.T) { // globalCfg nil so the zero-results fallback branch stays out of the way; // collected is non-empty anyway. - recs, complete, err := s.fetchAndConvert(ctx, prov, "azure", nil, nil) + recs, complete, err := s.fetchAndConvert(ctx, prov, "azure", nil, "", nil) require.NoError(t, err, "a partial multi-subscription sweep must not fail the whole collection") @@ -781,7 +781,7 @@ func TestScheduler_FetchAndConvert_RealErrorStillFailsLoud(t *testing.T) { s := &Scheduler{config: new(MockConfigStore)} - recs, complete, err := s.fetchAndConvert(ctx, prov, "azure", nil, nil) + recs, complete, err := s.fetchAndConvert(ctx, prov, "azure", nil, "", nil) require.Error(t, err, "a genuine error must still fail the collection") assert.Contains(t, err.Error(), "credentials expired") diff --git a/internal/scheduler/zonal_recs.go b/internal/scheduler/zonal_recs.go new file mode 100644 index 00000000..0bcef573 --- /dev/null +++ b/internal/scheduler/zonal_recs.go @@ -0,0 +1,44 @@ +package scheduler + +import ( + "strings" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" +) + +// zonalAvailabilityZone returns the AZ of a zonal EC2 recommendation, "" for +// regional recs and every non-compute service (issue #792). +func zonalAvailabilityZone(details common.ServiceDetails) string { + if cd, ok := details.(*common.ComputeDetails); ok && cd != nil { + return strings.TrimSpace(cd.AvailabilityZone) + } + return "" +} + +// dropForeignZonalEC2 removes zonal EC2 recs that Cost Explorer computed for +// a member account other than the one being collected. AZ names map to +// different physical zones per account, and the record is stamped with the +// collecting account, so such a rec would be bought in the wrong zone +// (issue #792, F4). Recs with an empty Account are kept; an empty +// expectedAccount (identity unknown, or a non-AWS provider) disables the +// check. The drop is logged and does not mark the collection incomplete. +func dropForeignZonalEC2(recs []common.Recommendation, expectedAccount string) []common.Recommendation { + if expectedAccount == "" { + return recs + } + kept := recs[:0:0] + dropped := 0 + for i := range recs { + r := recs[i] + if r.Account != "" && r.Account != expectedAccount && zonalAvailabilityZone(r.Details) != "" { + dropped++ + continue + } + kept = append(kept, r) + } + if dropped > 0 { + logging.Warnf("dropped %d zonal EC2 recommendation(s) computed for a member account other than %s: AZ names are per account and cannot be mapped safely", dropped, expectedAccount) + } + return kept +} diff --git a/internal/scheduler/zonal_recs_test.go b/internal/scheduler/zonal_recs_test.go new file mode 100644 index 00000000..cd124a51 --- /dev/null +++ b/internal/scheduler/zonal_recs_test.go @@ -0,0 +1,59 @@ +package scheduler + +import ( + "encoding/json" + "testing" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func zonalRec(account, az string) common.Recommendation { + return common.Recommendation{ + Provider: common.ProviderAWS, Account: account, Service: common.ServiceEC2, Region: "us-east-1", + ResourceType: "m5.large", Count: 1, Term: "1yr", PaymentOption: "all-upfront", + Details: &common.ComputeDetails{InstanceType: "m5.large", Platform: "Linux/UNIX", Tenancy: "default", Scope: "availability-zone", AvailabilityZone: az}, + } +} + +// Two zonal recs for the same type and term that differ only by AZ must get +// distinct IDs and carry the AZ at the top level and inside Details (#792). +func TestConvertRecommendations_ZonalRecsKeepAZAndGetDistinctIDs(t *testing.T) { + s := &Scheduler{} + out := s.convertRecommendations([]common.Recommendation{zonalRec("", "us-east-1a"), zonalRec("", "us-east-1b")}, "aws") + require.Len(t, out, 2) + assert.Equal(t, "us-east-1a", out[0].AvailabilityZone) + assert.Equal(t, "us-east-1b", out[1].AvailabilityZone) + assert.NotEqual(t, out[0].ID, out[1].ID) + var d common.ComputeDetails + require.NoError(t, json.Unmarshal(out[1].Details, &d)) + assert.Equal(t, "us-east-1b", d.AvailabilityZone) +} + +func TestConvertRecommendations_RegionalRecIDUnchanged(t *testing.T) { + s := &Scheduler{} + r := zonalRec("", "") + r.Details.(*common.ComputeDetails).Scope = "region" + out := s.convertRecommendations([]common.Recommendation{r}, "aws") + require.Len(t, out, 1) + assert.Empty(t, out[0].AvailabilityZone) + assert.Equal(t, "aws||ec2|us-east-1|m5.large||1|all-upfront", out[0].ID) +} + +func TestDropForeignZonalEC2(t *testing.T) { + regional := zonalRec("222222222222", "") + recs := []common.Recommendation{ + zonalRec("111111111111", "us-east-1a"), // own account: kept + zonalRec("", "us-east-1b"), // no account: kept + zonalRec("222222222222", "us-east-1c"), // member account zonal: dropped + regional, // member account regional: kept + } + got := dropForeignZonalEC2(recs, "111111111111") + require.Len(t, got, 3) + assert.Equal(t, "us-east-1a", zonalAvailabilityZone(got[0].Details)) + assert.Equal(t, "us-east-1b", zonalAvailabilityZone(got[1].Details)) + assert.Equal(t, "", zonalAvailabilityZone(got[2].Details)) + + assert.Len(t, dropForeignZonalEC2(recs, ""), 4, "unknown collecting identity disables the check") +} From 6074f12c03cb74e69bc0a05dee5aec3ff663b331 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 12 Oct 2026 17:53:51 +0200 Subject: [PATCH 2/3] test(scheduler): cover zonal EC2 foreign-account drop at the collection path Co-Authored-By: Claude Sonnet 5.5 --- internal/api/purchase_az_test.go | 6 +- internal/scheduler/zonal_recs.go | 16 +++- internal/scheduler/zonal_recs_test.go | 127 +++++++++++++++++++++++++- 3 files changed, 140 insertions(+), 9 deletions(-) diff --git a/internal/api/purchase_az_test.go b/internal/api/purchase_az_test.go index fa2c7936..6e31f2a0 100644 --- a/internal/api/purchase_az_test.go +++ b/internal/api/purchase_az_test.go @@ -14,7 +14,7 @@ import ( // Real zonal EC2 Details shape as persisted by the scheduler (issue #792). func zonalEC2Details(az string) json.RawMessage { - return json.RawMessage(`{"instance_type":"m5.large","platform":"Linux/UNIX","tenancy":"default","scope":"availability-zone","availability_zone":"` + az + `"}`) + return json.RawMessage(`{"instance_type":"m5.large","platform":"Linux/UNIX","tenancy":"default","scope":"Availability Zone","availability_zone":"` + az + `"}`) } func zonalEC2Rec(az string, upfront float64) config.RecommendationRecord { @@ -79,7 +79,7 @@ func TestCheckPurchaseDetailIdentity_AvailabilityZoneMismatchRefused(t *testing. match := zonalEC2Rec("us-east-1b", 100) match.ID = "other" if matchAZ == "" { - match.Details = json.RawMessage(`{"instance_type":"m5.large","platform":"Linux/UNIX","tenancy":"default","scope":"availability-zone"}`) + match.Details = json.RawMessage(`{"instance_type":"m5.large","platform":"Linux/UNIX","tenancy":"default","scope":"Availability Zone"}`) } req := config.RecommendationRecord{ID: origin.ID, Service: "ec2", Term: 1, Count: 1} err := checkPurchaseDetailIdentity(&origin, &match, &req, 0) @@ -94,7 +94,7 @@ func TestCheckPurchaseDetailIdentity_AvailabilityZoneMismatchRefused(t *testing. func TestCheckPurchaseDetailIdentity_LegacyOriginWithoutAZAllowed(t *testing.T) { origin := zonalEC2Rec("us-east-1a", 100) - origin.Details = json.RawMessage(`{"instance_type":"m5.large","platform":"Linux/UNIX","tenancy":"default","scope":"availability-zone"}`) + origin.Details = json.RawMessage(`{"instance_type":"m5.large","platform":"Linux/UNIX","tenancy":"default","scope":"Availability Zone"}`) match := zonalEC2Rec("us-east-1b", 100) match.ID = "other" req := config.RecommendationRecord{ID: origin.ID, Service: "ec2", Term: 1, Count: 1} diff --git a/internal/scheduler/zonal_recs.go b/internal/scheduler/zonal_recs.go index 0bcef573..07dbde86 100644 --- a/internal/scheduler/zonal_recs.go +++ b/internal/scheduler/zonal_recs.go @@ -24,8 +24,17 @@ func zonalAvailabilityZone(details common.ServiceDetails) string { // expectedAccount (identity unknown, or a non-AWS provider) disables the // check. The drop is logged and does not mark the collection incomplete. func dropForeignZonalEC2(recs []common.Recommendation, expectedAccount string) []common.Recommendation { + kept, dropped := partitionForeignZonalEC2(recs, expectedAccount) + if dropped > 0 { + logging.Warnf("dropped %d zonal EC2 recommendation(s) computed for a member account other than %s: AZ names are per account and cannot be mapped safely", dropped, expectedAccount) + } + return kept +} + +// partitionForeignZonalEC2 returns the recs to keep and the number dropped. +func partitionForeignZonalEC2(recs []common.Recommendation, expectedAccount string) ([]common.Recommendation, int) { if expectedAccount == "" { - return recs + return recs, 0 } kept := recs[:0:0] dropped := 0 @@ -37,8 +46,5 @@ func dropForeignZonalEC2(recs []common.Recommendation, expectedAccount string) [ } kept = append(kept, r) } - if dropped > 0 { - logging.Warnf("dropped %d zonal EC2 recommendation(s) computed for a member account other than %s: AZ names are per account and cannot be mapped safely", dropped, expectedAccount) - } - return kept + return kept, dropped } diff --git a/internal/scheduler/zonal_recs_test.go b/internal/scheduler/zonal_recs_test.go index cd124a51..c5c6873f 100644 --- a/internal/scheduler/zonal_recs_test.go +++ b/internal/scheduler/zonal_recs_test.go @@ -1,11 +1,15 @@ package scheduler import ( + "context" "encoding/json" "testing" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/pkg/provider" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" ) @@ -13,7 +17,7 @@ func zonalRec(account, az string) common.Recommendation { return common.Recommendation{ Provider: common.ProviderAWS, Account: account, Service: common.ServiceEC2, Region: "us-east-1", ResourceType: "m5.large", Count: 1, Term: "1yr", PaymentOption: "all-upfront", - Details: &common.ComputeDetails{InstanceType: "m5.large", Platform: "Linux/UNIX", Tenancy: "default", Scope: "availability-zone", AvailabilityZone: az}, + Details: &common.ComputeDetails{InstanceType: "m5.large", Platform: "Linux/UNIX", Tenancy: "default", Scope: "Availability Zone", AvailabilityZone: az}, } } @@ -57,3 +61,124 @@ func TestDropForeignZonalEC2(t *testing.T) { assert.Len(t, dropForeignZonalEC2(recs, ""), 4, "unknown collecting identity disables the check") } + +func TestPartitionForeignZonalEC2_CountsDropped(t *testing.T) { + recs := []common.Recommendation{ + zonalRec("111111111111", "us-east-1a"), + zonalRec("222222222222", "us-east-1b"), + zonalRec("333333333333", "us-east-1c"), + zonalRec("222222222222", ""), + } + kept, dropped := partitionForeignZonalEC2(recs, "111111111111") + assert.Equal(t, 2, dropped) + assert.Len(t, kept, 2) + kept, dropped = partitionForeignZonalEC2(recs, "") + assert.Equal(t, 0, dropped) + assert.Len(t, kept, 4) +} + +// mixedAccountRecs is what Cost Explorer returns for a payer: the collecting +// account's own zonal rec, a member account's zonal rec, a member account's +// regional rec. +func mixedAccountRecs(own string) []common.Recommendation { + memberRegional := zonalRec("222222222222", "") + memberRegional.Details.(*common.ComputeDetails).Scope = "Region" + memberRegional.ResourceType = "c5.large" + return []common.Recommendation{zonalRec(own, "us-east-1a"), zonalRec("222222222222", "us-east-1b"), memberRegional} +} + +func assertOnlyMemberZonalDropped(t *testing.T, recs []config.RecommendationRecord, complete bool) { + t.Helper() + assert.True(t, complete, "dropping a foreign zonal rec must not mark the collection incomplete") + require.Len(t, recs, 2) + azs := []string{recs[0].AvailabilityZone, recs[1].AvailabilityZone} + assert.ElementsMatch(t, []string{"us-east-1a", ""}, azs, "own zonal and member regional kept; member zonal dropped") +} + +func newRecProvider(client *MockRecommendationsClient) *MockProvider { + prov := new(MockProvider) + prov.On("GetRecommendationsClient", mock.Anything).Return(client, nil) + return prov +} + +func TestFetchAndConvert_DropsForeignZonalEC2_Primary(t *testing.T) { + client := new(MockRecommendationsClient) + client.On("GetAllRecommendations", mock.Anything).Return(mixedAccountRecs("111111111111"), nil) + s := &Scheduler{} + recs, complete, err := s.fetchAndConvert(context.Background(), newRecProvider(client), "aws", nil, "111111111111", nil) + require.NoError(t, err) + assertOnlyMemberZonalDropped(t, recs, complete) +} + +func TestFetchAndConvert_DropsForeignZonalEC2_LookbackFallback(t *testing.T) { + client := new(MockRecommendationsClient) + client.On("GetAllRecommendations", mock.Anything).Return([]common.Recommendation{}, nil) + client.On("GetRecommendations", mock.Anything, mock.MatchedBy(func(p *common.RecommendationParams) bool { + return p.Service == common.ServiceEC2 + })).Return(mixedAccountRecs("111111111111"), nil) + client.On("GetRecommendations", mock.Anything, mock.MatchedBy(func(p *common.RecommendationParams) bool { + return p.Service != common.ServiceEC2 + })).Return([]common.Recommendation{}, nil) + s := &Scheduler{} + recs, complete, err := s.fetchAndConvert(context.Background(), newRecProvider(client), "aws", nil, "111111111111", + &config.GlobalConfig{DefaultTerm: 1, DefaultPayment: "no-upfront", RecommendationsLookbackDays: 30}) + require.NoError(t, err) + assertOnlyMemberZonalDropped(t, recs, complete) +} + +func TestFetchAndConvert_NilExpectedAccountKeepsForeignZonal(t *testing.T) { + client := new(MockRecommendationsClient) + client.On("GetAllRecommendations", mock.Anything).Return(mixedAccountRecs("111111111111"), nil) + s := &Scheduler{} + recs, _, err := s.fetchAndConvert(context.Background(), newRecProvider(client), "aws", nil, "", nil) + require.NoError(t, err) + assert.Len(t, recs, 3, "an unknown collecting identity disables the drop") +} + +// The ambient path must compare against the STS host account. +func TestCollectAWSAmbient_PassesHostAccount(t *testing.T) { + ctx := context.Background() + factory := new(MockProviderFactory) + client := new(MockRecommendationsClient) + client.On("GetAllRecommendations", mock.Anything).Return(mixedAccountRecs("111111111111"), nil) + factory.On("CreateAndValidateProvider", ctx, "aws", (*provider.ProviderConfig)(nil)).Return(newRecProvider(client), nil) + s := &Scheduler{providerFactory: factory, stsClient: &fakeSTSClient{accountID: "111111111111"}} + recs, complete, err := s.collectAWSAmbient(ctx, nil) + require.NoError(t, err) + assertOnlyMemberZonalDropped(t, recs, complete) +} + +// A registered self account must compare against its ExternalID. +func TestCollectAWSForAccount_SelfAccountPassesExternalID(t *testing.T) { + ctx := context.Background() + factory := new(MockProviderFactory) + client := new(MockRecommendationsClient) + client.On("GetAllRecommendations", mock.Anything).Return(mixedAccountRecs("111111111111"), nil) + factory.On("CreateAndValidateProvider", ctx, "aws", (*provider.ProviderConfig)(nil)).Return(newRecProvider(client), nil) + s := &Scheduler{config: new(MockConfigStore), providerFactory: factory, stsClient: &fakeSTSClient{accountID: "111111111111"}} + recs, complete, err := s.collectAWSForAccount(ctx, nil, config.CloudAccount{ + ID: "self", Provider: "aws", AWSAuthMode: "role_arn", ExternalID: "111111111111", Enabled: true, + }) + require.NoError(t, err) + assertOnlyMemberZonalDropped(t, recs, complete) +} + +// A member account reached through an assumed role must compare against its +// ExternalID, not the host account. +func TestCollectAWSForAccount_AssumedRolePassesExternalID(t *testing.T) { + ctx := context.Background() + factory := new(MockProviderFactory) + client := new(MockRecommendationsClient) + client.On("GetAllRecommendations", mock.Anything).Return(mixedAccountRecs("333333333333"), nil) + factory.On("CreateAndValidateProvider", ctx, "aws", mock.AnythingOfType("*provider.ProviderConfig")).Return(newRecProvider(client), nil) + s := &Scheduler{config: new(MockConfigStore), providerFactory: factory, stsClient: &fakeSTSClient{accountID: "111111111111"}} + recs, complete, err := s.collectAWSForAccount(ctx, nil, config.CloudAccount{ + ID: "member", Provider: "aws", AWSAuthMode: "role_arn", AWSRoleARN: "arn:aws:iam::333333333333:role/cudly", + ExternalID: "333333333333", Enabled: true, + }) + require.NoError(t, err) + assert.True(t, complete) + require.Len(t, recs, 2) + // 333333333333 owns the first zonal rec; the 222222222222 zonal rec is dropped. + assert.ElementsMatch(t, []string{"us-east-1a", ""}, []string{recs[0].AvailabilityZone, recs[1].AvailabilityZone}) +} From f4b05f447d94814c5625f6fbbe9360ad77223306 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 12 Oct 2026 18:06:52 +0200 Subject: [PATCH 3/3] style(scheduler): name partitionForeignZonalEC2 results --- internal/scheduler/zonal_recs.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/internal/scheduler/zonal_recs.go b/internal/scheduler/zonal_recs.go index 07dbde86..35ec5675 100644 --- a/internal/scheduler/zonal_recs.go +++ b/internal/scheduler/zonal_recs.go @@ -32,12 +32,11 @@ func dropForeignZonalEC2(recs []common.Recommendation, expectedAccount string) [ } // partitionForeignZonalEC2 returns the recs to keep and the number dropped. -func partitionForeignZonalEC2(recs []common.Recommendation, expectedAccount string) ([]common.Recommendation, int) { +func partitionForeignZonalEC2(recs []common.Recommendation, expectedAccount string) (kept []common.Recommendation, dropped int) { if expectedAccount == "" { return recs, 0 } - kept := recs[:0:0] - dropped := 0 + kept = recs[:0:0] for i := range recs { r := recs[i] if r.Account != "" && r.Account != expectedAccount && zonalAvailabilityZone(r.Details) != "" {