diff --git a/internal/commitmentopts/probe.go b/internal/commitmentopts/probe.go index 2a0cc5e35..f96e68900 100644 --- a/internal/commitmentopts/probe.go +++ b/internal/commitmentopts/probe.go @@ -557,42 +557,10 @@ func (p *SavingsPlansProber) probeOnePlanType( if err != nil { return nil, err } - raw = dedupeRaw(raw) - // Use collect but override the service key to the per-product slug. - combos := collectWithService(serviceKey, raw) - return combos, nil -} - -// dedupeRaw removes duplicate (durationSeconds, payment) pairs so that -// collect does not see the same pair multiple times across an SP type's -// pages. SP offerings differ by ProductType (EC2, Lambda, Fargate ...) but -// share the same (duration, payment) matrix — we only need unique combos. -func dedupeRaw(raw []rawOffer) []rawOffer { - type key struct { - dur int64 - payment string - } - seen := make(map[key]struct{}, len(raw)) - out := make([]rawOffer, 0, len(raw)) - for _, r := range raw { - k := key{r.durationSeconds, r.payment} - if _, dup := seen[k]; dup { - continue - } - seen[k] = struct{}{} - out = append(out, r) - } - return out -} - -// collectWithService is like collect but uses the supplied serviceKey -// instead of the service embedded in each rawOffer (rawOffer has no -// service field; collect() uses the `service` argument, which is the -// per-prober name). Extracted to avoid a second copy of the -// normalization loop when the service name comes from outside the -// prober's Service() method (as it does for SP plan types). -func collectWithService(service string, raw []rawOffer) []Combo { - return collect(service, raw) + // collect dedupes on the normalized (term, payment) pair, which also + // collapses the same matrix repeated across an SP type's ProductTypes + // (EC2, Lambda, Fargate ...) and pages. + return collect(serviceKey, raw), nil } func (p *SavingsPlansProber) client(cfg aws.Config) SavingsPlansDescribeOfferings { diff --git a/internal/commitmentopts/probe_test.go b/internal/commitmentopts/probe_test.go index 48dbda377..d0d71aebb 100644 --- a/internal/commitmentopts/probe_test.go +++ b/internal/commitmentopts/probe_test.go @@ -651,6 +651,69 @@ func TestSavingsPlansProber_DedupedAcrossProductTypes(t *testing.T) { assert.Len(t, compute, 1, "duplicate (dur, pay) must collapse to one Combo") } +// TestSavingsPlansProber_DuplicatesAndNormalizedCollisionsPinned pins the +// exact, ordered output for one plan type whose pages repeat raw +// (durationSeconds, payment) pairs and also carry different raw spellings +// that normalize to the same (term, payment). collect() alone is the dedupe: +// the output keeps the first occurrence of each normalized pair, in page +// order, and drops unknown durations and legacy utilization tokens. Issue +// #325 removed a raw-pair pre-dedupe that ran before collect(); this test +// shows the output is unchanged without it. +func TestSavingsPlansProber_DuplicatesAndNormalizedCollisionsPinned(t *testing.T) { + const twoYearSeconds = 2 * 31536000 + sp := func(dur int64, pay string) savingsplanstypes.SavingsPlanOffering { + return spOffering(dur, savingsplanstypes.SavingsPlanPaymentOption(pay)) + } + pages := [][]savingsplanstypes.SavingsPlanOffering{ + { + sp(threeYearSeconds, "No Upfront"), + sp(oneYearSeconds, "All Upfront"), + sp(oneYearSeconds, "All Upfront"), // exact raw duplicate + sp(oneYearSeconds, "ALL_UPFRONT"), // normalizes to (1, all-upfront) + sp(twoYearSeconds, "All Upfront"), // unknown term: dropped + sp(twoYearSeconds, "All Upfront"), // duplicate of a dropped pair + sp(oneYearSeconds, "Heavy Utilization"), + sp(oneYearSeconds, "Heavy Utilization"), + }, + { + sp(threeYearSeconds, "no-upfront"), // normalized collision across pages + sp(oneYearSeconds, "All Upfront"), // exact raw duplicate across pages + sp(oneYearSeconds, "Partial Upfront"), // first new pair on page 2 + sp(threeYearSeconds, "NO_UPFRONT"), // another normalized collision + sp(oneYearSeconds, "partial_upfront"), // collides with the pair above + sp(threeYearSeconds, "Partial Upfront"), // second new pair on page 2 + }, + } + calls := 0 + fake := &fakeSavingsPlans{ + fn: func(in *savingsplans.DescribeSavingsPlansOfferingsInput) (*savingsplans.DescribeSavingsPlansOfferingsOutput, error) { + if in.PlanTypes[0] != savingsplanstypes.SavingsPlanTypeCompute { + return &savingsplans.DescribeSavingsPlansOfferingsOutput{}, nil + } + out := &savingsplans.DescribeSavingsPlansOfferingsOutput{SearchResults: pages[calls]} + calls++ + if calls < len(pages) { + out.NextToken = aws.String("page-2") + } + return out, nil + }, + } + p := &SavingsPlansProber{NewClient: func(_ aws.Config) SavingsPlansDescribeOfferings { return fake }} + + got, err := p.Probe(context.Background(), aws.Config{}) + require.NoError(t, err) + require.Equal(t, len(pages), calls, "both Compute pages must be read") + + const svc = "savings-plans-compute" + want := []Combo{ + {Provider: "aws", Service: svc, TermYears: 3, Payment: "no-upfront"}, + {Provider: "aws", Service: svc, TermYears: 1, Payment: "all-upfront"}, + {Provider: "aws", Service: svc, TermYears: 1, Payment: "partial-upfront"}, + {Provider: "aws", Service: svc, TermYears: 3, Payment: "partial-upfront"}, + } + assert.Equal(t, want, got, "first occurrence of each normalized pair, in page order") +} + // TestDefaultProbers_IncludesSavingsPlans verifies that SavingsPlansProber is // registered in DefaultProbers. func TestDefaultProbers_IncludesSavingsPlans(t *testing.T) { diff --git a/internal/purchase/approvals.go b/internal/purchase/approvals.go index b43544d12..5fb6479b2 100644 --- a/internal/purchase/approvals.go +++ b/internal/purchase/approvals.go @@ -98,12 +98,8 @@ func maskActor(actor string) string { if actor == "" { return "" } - if at := len(actor) - 1; at >= 0 { - for i, c := range actor { - if c == '@' { - return "***" + actor[i:] - } - } + if at := strings.Index(actor, "@"); at >= 0 { + return "***" + actor[at:] } // Not an email address -- log only the last 4 chars (e.g. a username). if len(actor) > 4 { diff --git a/internal/purchase/approvals_test.go b/internal/purchase/approvals_test.go index be7e9f2d4..75feeeeb8 100644 --- a/internal/purchase/approvals_test.go +++ b/internal/purchase/approvals_test.go @@ -1375,3 +1375,26 @@ func TestManager_ApproveAndExecute_FourEyesOn_SystemCreated(t *testing.T) { require.ErrorIs(t, gate(nil, " ", nil), ErrFourEyesDenied, "anonymous actor") require.ErrorIs(t, gate(&creatorID, "", &creatorID), ErrFourEyesDenied, "flag must not override a creator") } + +// TestMaskActor pins the PII mask written to the approval logs: everything +// before the first '@' is hidden, a non-email keeps only its last 4 bytes, +// and an empty actor is "". +func TestMaskActor(t *testing.T) { + cases := []struct{ in, want string }{ + {"", ""}, + {"approver@example.com", "***@example.com"}, + {"a@b", "***@b"}, + {"@example.com", "***@example.com"}, + {"trailing@", "***@"}, + {"first@second@example.com", "***@second@example.com"}, + {"josé@example.com", "***@example.com"}, + {"\xff\xfe@example.com", "***@example.com"}, + {"a", "****"}, + {"abcd", "****"}, + {"abcde", "...bcde"}, + {"service-account", "...ount"}, + } + for _, tc := range cases { + assert.Equal(t, tc.want, maskActor(tc.in), "maskActor(%q)", tc.in) + } +}