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..6e31f2a0 --- /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..35ec5675 --- /dev/null +++ b/internal/scheduler/zonal_recs.go @@ -0,0 +1,49 @@ +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 { + 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) (kept []common.Recommendation, dropped int) { + if expectedAccount == "" { + return recs, 0 + } + kept = recs[:0:0] + for i := range recs { + r := recs[i] + if r.Account != "" && r.Account != expectedAccount && zonalAvailabilityZone(r.Details) != "" { + dropped++ + continue + } + kept = append(kept, r) + } + return kept, dropped +} diff --git a/internal/scheduler/zonal_recs_test.go b/internal/scheduler/zonal_recs_test.go new file mode 100644 index 00000000..c5c6873f --- /dev/null +++ b/internal/scheduler/zonal_recs_test.go @@ -0,0 +1,184 @@ +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" +) + +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") +} + +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}) +}