From b006c9cf77c129943b6d35c99419dcd7b27183ef Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 9 Oct 2026 18:45:19 +0200 Subject: [PATCH 1/2] feat(mcp): suppress search recommendations covered by recent commitments cudly_search_recommendations now checks results against commitments bought in the last 24 hours with the library's duplicate checker. Fully covered RDS, OpenSearch, Redshift and ElastiCache (known engine) recommendations in the caller's own account are removed and listed under dedupe.suppressed. EC2, Azure, GCP and MemoryDB, whose matching key is too coarse, and other accounts are only flagged. A reduced count is never returned, listing failures fail the search naming the permission, and Savings Plans are not checked. Closes #42 Co-Authored-By: Claude Sonnet 5.5 --- CHANGELOG.md | 9 + README.md | 1 + go.mod | 6 +- .../gcp_computeengine_cud_safeguards_test.go | 5 +- tools/search_dedupe.go | 421 ++++++++++++++++++ tools/search_dedupe_test.go | 387 ++++++++++++++++ tools/search_recommendations.go | 14 +- tools/search_recommendations_test.go | 10 +- 8 files changed, 843 insertions(+), 10 deletions(-) create mode 100644 tools/search_dedupe.go create mode 100644 tools/search_dedupe_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 211f66a..05fb1c8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,15 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ## [Unreleased] +### Changed + +- `cudly_search_recommendations` now checks results against commitments bought + in the last 24 hours and returns a `dedupe` block: fully covered RDS, + OpenSearch, Redshift and ElastiCache recommendations are suppressed and + listed, possibly covered EC2, Azure, GCP and MemoryDB ones are flagged. + The search now needs permission to list existing commitments and fails + loudly, naming it, when it cannot. + ### Breaking - **Real purchases now require operator spend caps.** With diff --git a/README.md b/README.md index 5494fca..5758d27 100644 --- a/README.md +++ b/README.md @@ -162,6 +162,7 @@ These are pre-existing behaviours in the underlying purchase clients, not someth - **GCP Compute Engine CUDs commit resources, not instances.** `cudly_gcp_computeengine_cud_purchase` takes `vcpu_count` and `memory_gb` directly (a CUD is a vCPU+memory commitment), not an instance count -- there is no implicit vCPU-per-instance conversion. - **AWS Savings Plans searches require term/payment/lookback; `cudly_search_recommendations` defaults them.** Unlike EC2/RDS reservation searches, `GetSavingsPlansPurchaseRecommendation` rejects the call unless all three are set. When `service` targets a Savings Plans search on AWS, the tool fills in unset fields with `term_years=1` (1yr), `payment_option=no-upfront`, `lookback_period=30d` -- an explicitly supplied value is never overridden. A Savings Plans search therefore resolves to exactly one (term, payment, lookback) triple and issues exactly one Cost Explorer call. - **Reservation searches fan out over every term and payment option when you omit them.** `GetReservationPurchaseRecommendation` accepts exactly one `TermInYears` and one `PaymentOption` per request and returns recommendations only for that cell -- there is no "give me every variant" mode. So when `term_years` and/or `payment_option` are omitted on an EC2/RDS/ElastiCache/etc search, the tool expands the omitted dimension to its full menu and issues one call per combination (6 when both are omitted, 2 or 3 when one is), returning the concatenated results. Each returned recommendation carries the `term`/`payment_option` of the request that produced it, so its money figures are attributable to a specific offer. If any one combination fails, the whole search fails rather than returning a partial menu -- five of six offers is indistinguishable from "these are all your options". Note that `lookback_period` is NOT fanned out: omitting it leaves Cost Explorer's own server-side default (7 days) in place, since the lookback window is the usage evidence behind an offer rather than another offer to choose from. The fan-out multiplies the API bill along with the coverage: `cudly_search_recommendations` buys no commitment, but Cost Explorer bills per request, so an omitted-term/omitted-payment search costs six requests rather than one (more with pagination). The tool description states this too, so a model does not treat searching as free and re-run it in a loop. +- **`cudly_search_recommendations` checks results against recent commitments, with limits.** After fetching recommendations, the tool lists the commitments already bought (active, payment-pending or queued, started within the last 24 hours) for each service and region, using the credentials of the search. A recommendation fully covered by them is removed from `recommendations` and listed in `dedupe.suppressed` (service, region, resource type, account, term, payment option, count, reason). Suppression applies only to RDS, OpenSearch, Redshift and ElastiCache (when the commitment's engine is known) and only for recommendations in the credentials' own AWS account. For EC2, Azure, GCP and MemoryDB the matching key is too coarse to hide anything, so a possibly covered recommendation stays and appears in `dedupe.flagged` as `possibly_covered`; a partly covered one is returned unchanged (same count, cost and savings) with `covered_count`. Savings Plans are not checked. The six term/payment variants of one demand are all treated alike. Limits: purchases in other accounts, and anything older than 24 hours, are not seen; RDS size flexibility is not detected, so a covered recommendation can still be shown; the key ignores term and payment option. If listing commitments fails (for example for lack of `rds:DescribeReservedDBInstances`), the whole search fails and names the permission, rather than returning unchecked results. The key gaps are tracked in . - **`region` on `cudly_search_recommendations` is a client-side post-filter for AWS reservation/Savings Plans searches.** `GetReservationPurchaseRecommendation` and `GetSavingsPlansPurchaseRecommendation` are account-level Cost Explorer calls with no region parameter -- AWS returns recommendations from every region the account has usage in. The tool filters the returned recommendations down to `region`/`include_regions` (minus `exclude_regions`) after the call; when no region constraint is supplied, all regions are returned as before. ## Deployment model diff --git a/go.mod b/go.mod index 05704bf..88bbd70 100644 --- a/go.mod +++ b/go.mod @@ -7,11 +7,11 @@ require ( github.com/aws/aws-sdk-go-v2/config v1.29.12 // indirect github.com/aws/aws-sdk-go-v2/service/costexplorer v1.61.0 // indirect github.com/aws/aws-sdk-go-v2/service/ec2 v1.251.2 - github.com/aws/aws-sdk-go-v2/service/elasticache v1.50.3 // indirect + github.com/aws/aws-sdk-go-v2/service/elasticache v1.50.3 github.com/aws/aws-sdk-go-v2/service/memorydb v1.31.4 // indirect - github.com/aws/aws-sdk-go-v2/service/opensearch v1.52.3 // indirect + github.com/aws/aws-sdk-go-v2/service/opensearch v1.52.3 github.com/aws/aws-sdk-go-v2/service/rds v1.97.3 - github.com/aws/aws-sdk-go-v2/service/redshift v1.58.3 // indirect + github.com/aws/aws-sdk-go-v2/service/redshift v1.58.3 github.com/stretchr/testify v1.11.1 ) diff --git a/tools/gcp_computeengine_cud_safeguards_test.go b/tools/gcp_computeengine_cud_safeguards_test.go index 7a60e58..b34f1ad 100644 --- a/tools/gcp_computeengine_cud_safeguards_test.go +++ b/tools/gcp_computeengine_cud_safeguards_test.go @@ -117,9 +117,8 @@ func TestGCPComputeEnginePurchaseReportsExplicitZeroCost(t *testing.T) { // FilterRecommendationsForRecentCommitments, so this recommendation passed // through; at a32fd1a the family pool match filters it. // -// Deliberately NOT protocol-level: this consumer has no dedupe path of its -// own (nothing here imports pkg/recfilter outside this test); the wiring gap -// is tracked as issue #42. +// This pins the library boundary; the search tool's own use of the checker +// is covered in search_dedupe_test.go. func TestGCPComputeEngineDedupeDefersFamilyWithRecentCUD(t *testing.T) { ctx := context.Background() diff --git a/tools/search_dedupe.go b/tools/search_dedupe.go new file mode 100644 index 0000000..e3c0219 --- /dev/null +++ b/tools/search_dedupe.go @@ -0,0 +1,421 @@ +package tools + +import ( + "context" + "fmt" + "reflect" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/pkg/provider" + "github.com/LeanerCloud/cloud-commitments-go/pkg/recfilter" +) + +// dedupeMode says what the search does with a recommendation that recent +// commitments appear to cover. The library key (resource type, region, engine, +// deployment) is exact only for some services, so a rec is hidden only where +// that key is sufficient; elsewhere it stays visible and is flagged. +type dedupeMode int + +const ( + // dedupeNotApplied: no check is run (Savings Plans, unlisted services). + dedupeNotApplied dedupeMode = iota + // dedupeFlagOnly: the rec is kept and flagged possibly_covered. + dedupeFlagOnly + // dedupeSuppress: a fully covered rec is removed and listed in the summary. + dedupeSuppress +) + +const ( + dedupeWindow = "24h" // recfilter.DefaultDuplicateCheckLookbackHours + + statusPossiblyCovered = "possibly_covered" + statusPartiallyCovered = "partially_covered" + statusNotCheckedAccount = "not_checked_other_account" + + reasonEC2Lossy = "a recent reservation of this instance type exists in this region; platform, tenancy, scope, AZ and term are not compared, so it may not cover this recommendation" + reasonGCP = "family has a recent CUD; the library defers the whole machine family without comparing vCPU or memory amounts" + reasonAzure = "a recent reservation of this size exists in this region; scope and term are not compared, so it may not cover this recommendation" + reasonMemoryDB = "MemoryDB reserved nodes are listed with a fixed engine, so engine matching is not reliable" + reasonWildcard = "a recent ElastiCache reservation with an unspecified engine exists and could cover any engine" +) + +// dedupeModeFor is the single support table. Anything not listed is not +// checked: an unknown service must never be silently deduplicated. +func dedupeModeFor(rec common.Recommendation) (mode dedupeMode, reason string) { + if common.IsSavingsPlan(rec.Service) { + return dedupeNotApplied, "Savings Plan recommendations have no resource type or region to match" + } + switch rec.Provider { + case common.ProviderAWS: + switch rec.Service { + case common.ServiceRDS, common.ServiceRelationalDB, + common.ServiceOpenSearch, common.ServiceSearch, common.ServiceRedshift, common.ServiceDataWarehouse, + common.ServiceElastiCache, common.ServiceCache: + return dedupeSuppress, "" + case common.ServiceMemoryDB: + return dedupeFlagOnly, reasonMemoryDB + case common.ServiceEC2, common.ServiceCompute: + return dedupeFlagOnly, reasonEC2Lossy + } + case common.ProviderAzure: + if rec.Service == common.ServiceCompute { + return dedupeFlagOnly, reasonAzure + } + case common.ProviderGCP: + if rec.Service == common.ServiceCompute { + return dedupeFlagOnly, reasonGCP + } + } + return dedupeNotApplied, "no duplicate check is defined for this service" +} + +func isElastiCache(s common.ServiceType) bool { + return s == common.ServiceElastiCache || s == common.ServiceCache +} + +// listPermission names what the credentials need to list existing commitments, +// for the failure message. +func listPermission(rec common.Recommendation) string { + switch rec.Provider { + case common.ProviderAWS: + switch rec.Service { + case common.ServiceEC2, common.ServiceCompute: + return "ec2:DescribeReservedInstances" + case common.ServiceRDS, common.ServiceRelationalDB: + return "rds:DescribeReservedDBInstances" + case common.ServiceElastiCache, common.ServiceCache: + return "elasticache:DescribeReservedCacheNodes" + case common.ServiceMemoryDB: + return "memorydb:DescribeReservedNodes" + case common.ServiceOpenSearch, common.ServiceSearch: + return "es:DescribeReservedInstances" + case common.ServiceRedshift, common.ServiceDataWarehouse: + return "redshift:DescribeReservedNodes" + } + case common.ProviderAzure: + return "read access to reservation orders (Reservations Reader)" + case common.ProviderGCP: + return "compute.regionCommitments.list" + } + return "read access to existing commitments" +} + +type dedupeGroup struct { + Service string `json:"service"` + Region string `json:"region"` + Status string `json:"status"` // checked | not_applied + Reason string `json:"reason,omitempty"` +} + +type suppressedRec struct { + Service string `json:"service"` + Region string `json:"region"` + ResourceType string `json:"resource_type"` + Account string `json:"account,omitempty"` + Term string `json:"term,omitempty"` + Payment string `json:"payment_option,omitempty"` + Reason string `json:"reason"` + Count int `json:"count"` +} + +// flaggedRec points at a returned recommendation by index. The recommendation +// itself is never modified, so its cost and savings figures stay those of the +// recommended count. +type flaggedRec struct { + Status string `json:"status"` + Reason string `json:"reason"` + Service string `json:"service"` + Region string `json:"region"` + ResourceType string `json:"resource_type"` + CoveredCount int `json:"covered_count,omitempty"` + RecommendationIndex int `json:"recommendation_index"` +} + +// searchDedupe is the audit trail of the duplicate check: nothing the check +// removes or doubts is hidden from the caller. +type searchDedupe struct { + Window string `json:"window"` + Groups []dedupeGroup `json:"groups"` + Suppressed []suppressedRec `json:"suppressed"` + Flagged []flaggedRec `json:"flagged"` +} + +type recentFilterClient interface { + FilterRecommendationsForRecentCommitments(recs []common.Recommendation, existing []common.Commitment) (passed, filtered []common.Recommendation, err error) +} + +// memoClient lists existing commitments once per (service, region) per search +// and remembers whether any ElastiCache reservation has no engine. +type memoClient struct { + provider.ServiceClient + err error + list []common.Commitment + listed bool + wildcard bool + checkEC bool +} + +func (m *memoClient) GetExistingCommitments(ctx context.Context) ([]common.Commitment, error) { + if !m.listed { + m.list, m.err = m.ServiceClient.GetExistingCommitments(ctx) + m.listed = true + if m.checkEC { + for i := range m.list { + if common.NormalizeEngineName(m.list[i].Engine) == "" { + m.wildcard = true + } + } + } + } + return m.list, m.err +} + +// hookedMemoClient additionally forwards the provider's own duplicate filter +// (GCP), which recfilter discovers by type assertion and a plain embed hides. +type hookedMemoClient struct { + *memoClient + hook recentFilterClient +} + +func (h hookedMemoClient) FilterRecommendationsForRecentCommitments(recs []common.Recommendation, existing []common.Commitment) (passed, filtered []common.Recommendation, err error) { + return h.hook.FilterRecommendationsForRecentCommitments(recs, existing) +} + +func newMemoClient(inner provider.ServiceClient, service common.ServiceType) (*memoClient, provider.ServiceClient) { + m := &memoClient{ServiceClient: inner, checkEC: isElastiCache(service)} + if hook, ok := inner.(recentFilterClient); ok { + return m, hookedMemoClient{memoClient: m, hook: hook} + } + return m, m +} + +type groupKey struct { + service common.ServiceType + region string +} + +type variantKey struct{ term, payment string } + +// dedupeSearchResults applies the duplicate check to a search result. It +// returns the recommendations to show and the audit trail. Any listing error +// fails the whole search: returning the recs as if checked would show covered +// ones without a warning. +func dedupeSearchResults(ctx context.Context, prov provider.Provider, recs []common.Recommendation) ([]common.Recommendation, searchDedupe, error) { + out := searchDedupe{Window: dedupeWindow, Groups: []dedupeGroup{}, Suppressed: []suppressedRec{}, Flagged: []flaggedRec{}} + if len(recs) == 0 { + return recs, out, nil + } + + order, byGroup := groupRecs(recs) + checker := recfilter.NewDuplicateChecker(0) + callerAccount := accountResolver{prov: prov} + decisions := make([]recDecision, len(recs)) + + for _, gk := range order { + idxs := byGroup[gk] + first := recs[idxs[0]] + mode, reason := dedupeModeFor(first) + group := dedupeGroup{Service: string(gk.service), Region: gk.region, Status: "checked"} + if mode == dedupeNotApplied { + group.Status, group.Reason = "not_applied", reason + out.Groups = append(out.Groups, group) + continue + } + if err := checkGroup(ctx, prov, checker, &callerAccount, recs, idxs, mode, reason, decisions); err != nil { + return nil, searchDedupe{}, fmt.Errorf("dedupe check failed for %s in %s: %w; the credentials need %s to list existing commitments; no recommendations returned", + gk.service, gk.region, err, listPermission(first)) + } + out.Groups = append(out.Groups, group) + } + + kept := make([]common.Recommendation, 0, len(recs)) + for i := range recs { + rec := &recs[i] + d := decisions[i] + if d.suppress { + out.Suppressed = append(out.Suppressed, suppressedRec{ + Service: string(rec.Service), Region: rec.Region, ResourceType: rec.ResourceType, Account: rec.Account, + Term: rec.Term, Payment: rec.PaymentOption, Count: rec.Count, Reason: d.reason, + }) + continue + } + if d.status != "" { + out.Flagged = append(out.Flagged, flaggedRec{ + Status: d.status, Reason: d.reason, Service: string(rec.Service), Region: rec.Region, + ResourceType: rec.ResourceType, CoveredCount: d.covered, RecommendationIndex: len(kept), + }) + } + kept = append(kept, *rec) + } + return kept, out, nil +} + +type recDecision struct { + status string + reason string + covered int + suppress bool +} + +func groupRecs(recs []common.Recommendation) (order []groupKey, byGroup map[groupKey][]int) { + byGroup = map[groupKey][]int{} + for i := range recs { + k := groupKey{recs[i].Service, recs[i].Region} + if _, ok := byGroup[k]; !ok { + order = append(order, k) + } + byGroup[k] = append(byGroup[k], i) + } + return order, byGroup +} + +// checkGroup runs the duplicate check for one (service, region) group and +// records a decision per recommendation. +func checkGroup(ctx context.Context, prov provider.Provider, checker *recfilter.DuplicateChecker, accounts *accountResolver, + recs []common.Recommendation, idxs []int, mode dedupeMode, flagReason string, decisions []recDecision) error { + first := recs[idxs[0]] + svc, err := prov.GetServiceClient(ctx, first.Service, first.Region) + if err != nil { + return fmt.Errorf("get service client: %w", err) + } + if svc == nil { + return fmt.Errorf("no service client available") + } + memo, client := newMemoClient(svc, first.Service) + + variants, variantOrder, err := partitionVariants(ctx, accounts, recs, idxs, decisions) + if err != nil { + return err + } + + for _, vk := range variantOrder { + vidx := variants[vk] + batch := make([]common.Recommendation, len(vidx)) + for j, i := range vidx { + batch[j] = recs[i] + } + passed, filtered, err := checker.AdjustRecommendationsForExisting(ctx, batch, client) + if err != nil { + return err + } + outcomes, err := pairOutcomes(batch, passed, filtered) + if err != nil { + return err + } + for j, i := range vidx { + decisions[i] = decide(outcomes[j], mode, flagReason) + } + } + + if memo.wildcard { + flagWildcard(idxs, decisions) + } + return nil +} + +// flagWildcard keeps every rec of the group visible: an engine-less +// reservation could cover any engine. Recs for another account stay as they are. +func flagWildcard(idxs []int, decisions []recDecision) { + for _, i := range idxs { + if decisions[i].status == statusNotCheckedAccount { + continue + } + decisions[i] = recDecision{status: statusPossiblyCovered, reason: reasonWildcard} + } +} + +// partitionVariants splits a group into (term, payment) variants: variants are +// alternatives for the same demand, so each needs a fresh budget rather than +// sharing one. On AWS, recs for an account other than the caller's are +// decided here as not checked and left out. +func partitionVariants(ctx context.Context, accounts *accountResolver, recs []common.Recommendation, idxs []int, + decisions []recDecision) (variants map[variantKey][]int, order []variantKey, err error) { + variants = map[variantKey][]int{} + for _, i := range idxs { + if recs[i].Provider == common.ProviderAWS { + caller, callerErr := accounts.callerID(ctx) + if callerErr != nil { + return nil, nil, callerErr + } + if caller == "" || recs[i].Account != caller { + decisions[i] = recDecision{status: statusNotCheckedAccount, + reason: "this recommendation is for a different account than the credentials; commitments there are not visible to this check"} + continue + } + } + k := variantKey{recs[i].Term, recs[i].PaymentOption} + if _, ok := variants[k]; !ok { + order = append(order, k) + } + variants[k] = append(variants[k], i) + } + return variants, order, nil +} + +type outcome struct { + fully bool + covered int +} + +// pairOutcomes matches the library's passed (possibly count-reduced) and +// filtered (fully covered) lists back to the input order. +func pairOutcomes(batch, passed, filtered []common.Recommendation) (out []outcome, err error) { + if len(passed)+len(filtered) != len(batch) { + return nil, fmt.Errorf("duplicate checker returned %d+%d recommendations for %d inputs", len(passed), len(filtered), len(batch)) + } + out = make([]outcome, len(batch)) + pi, fi := 0, 0 + for i := range batch { + if fi < len(filtered) && reflect.DeepEqual(filtered[fi], batch[i]) { + out[i] = outcome{fully: true, covered: batch[i].Count} + fi++ + continue + } + if pi >= len(passed) { + return nil, fmt.Errorf("duplicate checker output does not match its input") + } + out[i] = outcome{covered: batch[i].Count - passed[pi].Count} + pi++ + } + return out, nil +} + +func decide(o outcome, mode dedupeMode, flagReason string) recDecision { + switch { + case o.fully && mode == dedupeSuppress: + return recDecision{suppress: true, reason: "fully covered by recent commitments within " + dedupeWindow} + case o.fully && mode == dedupeFlagOnly: + return recDecision{status: statusPossiblyCovered, reason: flagReason} + case o.covered > 0 && mode == dedupeSuppress: + return recDecision{status: statusPartiallyCovered, covered: o.covered, + reason: "recent commitments cover part of the recommended count; the recommendation is shown unchanged, including its cost and savings figures"} + case o.covered > 0: + return recDecision{status: statusPossiblyCovered, covered: o.covered, reason: flagReason} + } + return recDecision{} +} + +// accountResolver resolves the caller's AWS account id once per search. +type accountResolver struct { + prov provider.Provider + id string + done bool +} + +func (a *accountResolver) callerID(ctx context.Context) (string, error) { + if a.done { + return a.id, nil + } + accounts, err := a.prov.GetAccounts(ctx) + if err != nil { + return "", fmt.Errorf("resolve caller account: %w", err) + } + a.done = true + for _, acct := range accounts { + if acct.IsDefault { + a.id = acct.ID + break + } + } + return a.id, nil +} diff --git a/tools/search_dedupe_test.go b/tools/search_dedupe_test.go new file mode 100644 index 0000000..b8519a6 --- /dev/null +++ b/tools/search_dedupe_test.go @@ -0,0 +1,387 @@ +package tools + +import ( + "context" + "encoding/json" + "errors" + "testing" + "time" + + "cloud.google.com/go/compute/apiv1/computepb" + "github.com/aws/aws-sdk-go-v2/aws" + ecsdk "github.com/aws/aws-sdk-go-v2/service/elasticache" + ectypes "github.com/aws/aws-sdk-go-v2/service/elasticache/types" + ossdk "github.com/aws/aws-sdk-go-v2/service/opensearch" + ostypes "github.com/aws/aws-sdk-go-v2/service/opensearch/types" + rdssdk "github.com/aws/aws-sdk-go-v2/service/rds" + rdstypes "github.com/aws/aws-sdk-go-v2/service/rds/types" + rssdk "github.com/aws/aws-sdk-go-v2/service/redshift" + rstypes "github.com/aws/aws-sdk-go-v2/service/redshift/types" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "google.golang.org/protobuf/proto" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/pkg/provider" + eclib "github.com/LeanerCloud/cloud-commitments-go/providers/aws/services/elasticache" + oslib "github.com/LeanerCloud/cloud-commitments-go/providers/aws/services/opensearch" + rdslib "github.com/LeanerCloud/cloud-commitments-go/providers/aws/services/rds" + rslib "github.com/LeanerCloud/cloud-commitments-go/providers/aws/services/redshift" + "github.com/LeanerCloud/cloud-commitments-go/providers/gcp/services/computeengine" +) + +const ( + dedupeCaller = "111111111111" + dedupeOther = "222222222222" +) + +var dedupeRecent = time.Now().Add(-time.Hour) + +// listingClient is a ServiceClient whose existing-commitment listing is +// scripted and counted. +type listingClient struct { + err error + fakeServiceClient + commitments []common.Commitment + listCalls int +} + +func (l *listingClient) GetExistingCommitments(_ context.Context) ([]common.Commitment, error) { + l.listCalls++ + return l.commitments, l.err +} + +// svcProvider routes GetServiceClient to a per-service client and counts calls. +type svcProvider struct { + byService map[common.ServiceType]provider.ServiceClient + fakeProvider + calls int +} + +func (p *svcProvider) GetServiceClient(_ context.Context, s common.ServiceType, _ string) (provider.ServiceClient, error) { + p.calls++ + return p.byService[s], nil +} + +func awsProvider(clients map[common.ServiceType]provider.ServiceClient) *svcProvider { + return &svcProvider{ + fakeProvider: fakeProvider{name: "aws", accounts: []common.Account{ + {ID: dedupeCaller, IsDefault: true}, {ID: dedupeOther}, + }}, + byService: clients, + } +} + +func runDedupe(t *testing.T, p provider.Provider, recs []common.Recommendation) ([]common.Recommendation, searchDedupe, error) { + t.Helper() + return dedupeSearchResults(context.Background(), p, recs) +} + +// The rec shapes below mirror the parsers in cloud-commitments-go +// providers/aws/recommendations/parser_services.go (resource type, normalized +// region, linked-account id, pointer Details), with the engine strings AWS +// reports in Cost Explorer. +func rdsRec(count int, term, payment, account string) common.Recommendation { + return common.Recommendation{ + Provider: common.ProviderAWS, Service: common.ServiceRDS, Account: account, Region: "us-east-1", + ResourceType: "db.r6g.large", Count: count, Term: term, PaymentOption: payment, EstimatedSavings: 1234, + Details: &common.DatabaseDetails{Engine: "MySQL", AZConfig: "multi-az"}, + } +} + +type rdsAPI struct { + rdslib.API + out []rdstypes.ReservedDBInstance +} + +func (r rdsAPI) DescribeReservedDBInstances(_ context.Context, _ *rdssdk.DescribeReservedDBInstancesInput, _ ...func(*rdssdk.Options)) (*rdssdk.DescribeReservedDBInstancesOutput, error) { + return &rdssdk.DescribeReservedDBInstancesOutput{ReservedDBInstances: r.out}, nil +} + +func rdsClient() *rdslib.Client { + c := rdslib.NewClient(aws.Config{Region: "us-east-1"}) + c.SetRDSAPI(rdsAPI{out: []rdstypes.ReservedDBInstance{{ + ReservedDBInstanceId: aws.String("ri-1"), DBInstanceClass: aws.String("db.r6g.large"), + ProductDescription: aws.String("mysql"), MultiAZ: aws.Bool(true), DBInstanceCount: aws.Int32(2), + State: aws.String("active"), StartTime: aws.Time(dedupeRecent), Duration: aws.Int32(31536000), + }}}) + return c +} + +func TestDedupeRDSFullyCoveredIsSuppressedAndListed(t *testing.T) { + p := awsProvider(map[common.ServiceType]provider.ServiceClient{common.ServiceRDS: rdsClient()}) + kept, d, err := runDedupe(t, p, []common.Recommendation{rdsRec(2, "1yr", "all-upfront", dedupeCaller)}) + require.NoError(t, err) + assert.Empty(t, kept) + require.Len(t, d.Suppressed, 1) + s := d.Suppressed[0] + assert.Equal(t, "1yr", s.Term) + assert.Equal(t, "all-upfront", s.Payment) + assert.Equal(t, dedupeCaller, s.Account) + assert.Equal(t, 2, s.Count) + assert.NotEmpty(t, s.Reason) +} + +func TestDedupePartialCoverKeepsRecUnchangedWithCoveredCount(t *testing.T) { + p := awsProvider(map[common.ServiceType]provider.ServiceClient{common.ServiceRDS: rdsClient()}) + rec := rdsRec(3, "1yr", "all-upfront", dedupeCaller) + kept, d, err := runDedupe(t, p, []common.Recommendation{rec}) + require.NoError(t, err) + require.Len(t, kept, 1) + assert.Equal(t, rec, kept[0], "a partially covered rec must keep its count, cost and savings") + require.Len(t, d.Flagged, 1) + assert.Equal(t, statusPartiallyCovered, d.Flagged[0].Status) + assert.Equal(t, 2, d.Flagged[0].CoveredCount) + assert.Equal(t, 0, d.Flagged[0].RecommendationIndex) + assert.Empty(t, d.Suppressed) +} + +func TestDedupeVariantsOfSameDemandAreTreatedAlike(t *testing.T) { + p := awsProvider(map[common.ServiceType]provider.ServiceClient{common.ServiceRDS: rdsClient()}) + recs := make([]common.Recommendation, 0, 6) + for _, term := range []string{"1yr", "3yr"} { + for _, pay := range []string{"all-upfront", "partial-upfront", "no-upfront"} { + recs = append(recs, rdsRec(2, term, pay, dedupeCaller)) + } + } + kept, d, err := runDedupe(t, p, recs) + require.NoError(t, err) + assert.Empty(t, kept, "all six alternatives for the same covered demand must be suppressed, not just the first") + assert.Len(t, d.Suppressed, 6) +} + +func TestDedupeOtherAccountIsNeverSuppressed(t *testing.T) { + p := awsProvider(map[common.ServiceType]provider.ServiceClient{common.ServiceRDS: rdsClient()}) + kept, d, err := runDedupe(t, p, []common.Recommendation{ + rdsRec(2, "1yr", "all-upfront", dedupeOther), + rdsRec(2, "1yr", "no-upfront", ""), + }) + require.NoError(t, err) + assert.Len(t, kept, 2) + assert.Empty(t, d.Suppressed) + require.Len(t, d.Flagged, 2) + for _, f := range d.Flagged { + assert.Equal(t, statusNotCheckedAccount, f.Status) + } +} + +func TestDedupeNothingRecentLeavesRecsUntouched(t *testing.T) { + p := awsProvider(map[common.ServiceType]provider.ServiceClient{common.ServiceRDS: &listingClient{}}) + rec := rdsRec(2, "1yr", "all-upfront", dedupeCaller) + kept, d, err := runDedupe(t, p, []common.Recommendation{rec}) + require.NoError(t, err) + assert.Equal(t, []common.Recommendation{rec}, kept) + assert.Empty(t, d.Flagged) + assert.Empty(t, d.Suppressed) + require.Len(t, d.Groups, 1) + assert.Equal(t, "checked", d.Groups[0].Status) +} + +func TestDedupeListsOncePerServiceRegion(t *testing.T) { + lc := &listingClient{} + p := awsProvider(map[common.ServiceType]provider.ServiceClient{common.ServiceRDS: lc}) + recs := []common.Recommendation{ + rdsRec(2, "1yr", "all-upfront", dedupeCaller), rdsRec(2, "3yr", "all-upfront", dedupeCaller), + rdsRec(2, "1yr", "no-upfront", dedupeCaller), + } + _, _, err := runDedupe(t, p, recs) + require.NoError(t, err) + assert.Equal(t, 1, lc.listCalls, "one listing per (service, region) per search, however many variants") + assert.Equal(t, 1, p.calls) +} + +func TestDedupeEC2IsFlagOnlyNeverHidden(t *testing.T) { + lc := &listingClient{commitments: []common.Commitment{{ + Provider: common.ProviderAWS, Service: common.ServiceEC2, Region: "us-east-1", ResourceType: "m5.large", + Count: 10, State: common.CommitmentStateActive, StartDate: dedupeRecent, + }}} + p := awsProvider(map[common.ServiceType]provider.ServiceClient{common.ServiceEC2: lc}) + rec := common.Recommendation{ + Provider: common.ProviderAWS, Service: common.ServiceEC2, Account: dedupeCaller, Region: "us-east-1", + ResourceType: "m5.large", Count: 4, Term: "1yr", PaymentOption: "no-upfront", + Details: &common.ComputeDetails{InstanceType: "m5.large", Platform: "Linux/UNIX"}, + } + kept, d, err := runDedupe(t, p, []common.Recommendation{rec}) + require.NoError(t, err) + assert.Equal(t, []common.Recommendation{rec}, kept, "a lossy key must never hide a recommendation") + require.Len(t, d.Flagged, 1) + assert.Equal(t, statusPossiblyCovered, d.Flagged[0].Status) + assert.Empty(t, d.Suppressed) +} + +func TestDedupeSavingsPlansAreNotApplied(t *testing.T) { + p := awsProvider(nil) + rec := common.Recommendation{Provider: common.ProviderAWS, Service: common.ServiceSavingsPlansCompute, Account: dedupeCaller, Region: "us-east-1", Count: 1} + kept, d, err := runDedupe(t, p, []common.Recommendation{rec}) + require.NoError(t, err) + assert.Equal(t, []common.Recommendation{rec}, kept) + require.Len(t, d.Groups, 1) + assert.Equal(t, "not_applied", d.Groups[0].Status) + assert.Equal(t, 0, p.calls, "no client is resolved for an unchecked group") +} + +func TestDedupeListingFailureFailsLoudNamingThePermission(t *testing.T) { + lc := &listingClient{err: errors.New("AccessDenied")} + p := awsProvider(map[common.ServiceType]provider.ServiceClient{common.ServiceRDS: lc}) + kept, _, err := runDedupe(t, p, []common.Recommendation{rdsRec(2, "1yr", "all-upfront", dedupeCaller)}) + require.Error(t, err) + assert.Nil(t, kept) + assert.Contains(t, err.Error(), "rds:DescribeReservedDBInstances") + assert.Contains(t, err.Error(), "AccessDenied") +} + +func TestDedupeNoCallerAccountFlagsEverything(t *testing.T) { + p := awsProvider(map[common.ServiceType]provider.ServiceClient{common.ServiceRDS: &listingClient{}}) + p.accounts = nil + // No default account: nothing may be suppressed, everything is flagged. + kept, d, err := runDedupe(t, p, []common.Recommendation{rdsRec(2, "1yr", "all-upfront", dedupeCaller)}) + require.NoError(t, err) + assert.Len(t, kept, 1) + assert.Equal(t, statusNotCheckedAccount, d.Flagged[0].Status) +} + +func TestDedupeElastiCacheWildcardEngineNeverSuppresses(t *testing.T) { + cache := func(engine string) common.Recommendation { + return common.Recommendation{ + Provider: common.ProviderAWS, Service: common.ServiceElastiCache, Account: dedupeCaller, Region: "us-east-1", + ResourceType: "cache.r6g.large", Count: 2, Term: "1yr", PaymentOption: "no-upfront", + Details: &common.CacheDetails{NodeType: "cache.r6g.large", Engine: engine}, + } + } + for engine, wantSuppressed := range map[string]bool{"redis": true, "": false} { + c := eclib.NewClient(aws.Config{Region: "us-east-1"}) + c.SetElastiCacheAPI(ecAPI{engine: engine}) + p := awsProvider(map[common.ServiceType]provider.ServiceClient{common.ServiceElastiCache: c}) + kept, d, err := runDedupe(t, p, []common.Recommendation{cache("Redis")}) + require.NoError(t, err) + if wantSuppressed { + assert.Empty(t, kept, "engine %q", engine) + assert.Len(t, d.Suppressed, 1) + } else { + assert.Len(t, kept, 1, "an engine-less reservation must not hide a rec") + require.Len(t, d.Flagged, 1) + assert.Equal(t, statusPossiblyCovered, d.Flagged[0].Status) + } + } +} + +type ecAPI struct { + eclib.API + engine string +} + +func (e ecAPI) DescribeReservedCacheNodes(_ context.Context, _ *ecsdk.DescribeReservedCacheNodesInput, _ ...func(*ecsdk.Options)) (*ecsdk.DescribeReservedCacheNodesOutput, error) { + return &ecsdk.DescribeReservedCacheNodesOutput{ReservedCacheNodes: []ectypes.ReservedCacheNode{{ + ReservedCacheNodeId: aws.String("n1"), CacheNodeType: aws.String("cache.r6g.large"), + ProductDescription: aws.String(e.engine), CacheNodeCount: aws.Int32(2), State: aws.String("active"), + StartTime: aws.Time(dedupeRecent), Duration: aws.Int32(31536000), + }}}, nil +} + +type osAPI struct{ oslib.API } + +func (osAPI) DescribeReservedInstances(_ context.Context, _ *ossdk.DescribeReservedInstancesInput, _ ...func(*ossdk.Options)) (*ossdk.DescribeReservedInstancesOutput, error) { + return &ossdk.DescribeReservedInstancesOutput{ReservedInstances: []ostypes.ReservedInstance{{ + ReservedInstanceId: aws.String("o1"), InstanceType: ostypes.OpenSearchPartitionInstanceType("r6g.large.search"), + InstanceCount: 3, State: aws.String("active"), StartTime: aws.Time(dedupeRecent), Duration: 31536000, + }}}, nil +} + +type rsAPI struct{ rslib.API } + +func (rsAPI) DescribeReservedNodes(_ context.Context, _ *rssdk.DescribeReservedNodesInput, _ ...func(*rssdk.Options)) (*rssdk.DescribeReservedNodesOutput, error) { + return &rssdk.DescribeReservedNodesOutput{ReservedNodes: []rstypes.ReservedNode{{ + ReservedNodeId: aws.String("r1"), NodeType: aws.String("ra3.xlplus"), NodeCount: aws.Int32(3), + State: aws.String("active"), StartTime: aws.Time(dedupeRecent), Duration: aws.Int32(31536000), + }}}, nil +} + +// Each "exact" service must actually suppress end to end through its real +// listing: a key mismatch (an engine on one side only) would make the feature +// silently do nothing. +func TestDedupeEverySuppressingServiceSuppressesThroughItsRealListing(t *testing.T) { + osc := oslib.NewClient(aws.Config{Region: "us-east-1"}) + osc.SetOpenSearchAPI(osAPI{}) + rsc := rslib.NewClient(aws.Config{Region: "us-east-1"}) + rsc.SetRedshiftAPI(rsAPI{}) + p := awsProvider(map[common.ServiceType]provider.ServiceClient{ + common.ServiceOpenSearch: osc, common.ServiceRedshift: rsc, common.ServiceRDS: rdsClient(), + }) + recs := []common.Recommendation{ + {Provider: common.ProviderAWS, Service: common.ServiceOpenSearch, Account: dedupeCaller, Region: "us-east-1", + ResourceType: "r6g.large.search", Count: 3, Term: "1yr", PaymentOption: "no-upfront", Details: &common.SearchDetails{InstanceType: "r6g.large.search"}}, + {Provider: common.ProviderAWS, Service: common.ServiceRedshift, Account: dedupeCaller, Region: "us-east-1", + ResourceType: "ra3.xlplus", Count: 3, Term: "1yr", PaymentOption: "no-upfront"}, + rdsRec(2, "1yr", "no-upfront", dedupeCaller), + } + kept, d, err := runDedupe(t, p, recs) + require.NoError(t, err) + assert.Empty(t, kept) + assert.Len(t, d.Suppressed, 3) +} + +func TestDedupeMemoryDBIsFlagOnly(t *testing.T) { + lc := &listingClient{commitments: []common.Commitment{{ + Provider: common.ProviderAWS, Service: common.ServiceMemoryDB, Region: "us-east-1", ResourceType: "db.r6g.large", + Engine: "redis", Count: 2, State: common.CommitmentStateActive, StartDate: dedupeRecent, + }}} + p := awsProvider(map[common.ServiceType]provider.ServiceClient{common.ServiceMemoryDB: lc}) + rec := common.Recommendation{Provider: common.ProviderAWS, Service: common.ServiceMemoryDB, Account: dedupeCaller, + Region: "us-east-1", ResourceType: "db.r6g.large", Count: 2, Term: "1yr", PaymentOption: "no-upfront", + Details: &common.CacheDetails{NodeType: "db.r6g.large", Engine: "redis"}} + kept, d, err := runDedupe(t, p, []common.Recommendation{rec}) + require.NoError(t, err) + assert.Len(t, kept, 1) + assert.Empty(t, d.Suppressed) + require.Len(t, d.Flagged, 1) + assert.Equal(t, statusPossiblyCovered, d.Flagged[0].Status) +} + +// GCP is flag-only, and the provider's own family filter must still be +// reached through the memoizing client. +func TestDedupeGCPForwardsTheFamilyHookAndOnlyFlags(t *testing.T) { + ctx := context.Background() + client, err := computeengine.NewClient(ctx, "test-project", "us-central1") + require.NoError(t, err) + client.SetCommitmentsService(&fakeGCPCommitmentsService{commitments: []*computepb.Commitment{{ + Name: proto.String("existing-n2-cud"), + Type: proto.String(computepb.Commitment_GENERAL_PURPOSE_N2.String()), + Status: proto.String(computepb.Commitment_ACTIVE.String()), + StartTimestamp: proto.String(dedupeRecent.UTC().Format(time.RFC3339)), + Resources: []*computepb.ResourceCommitment{ + {Type: proto.String(computepb.ResourceCommitment_VCPU.String()), Amount: proto.Int64(2)}, + {Type: proto.String(computepb.ResourceCommitment_MEMORY.String()), Amount: proto.Int64(8192)}, + }, + }}}) + rec, _, _, _, err := gcpComputeEngineRecommendationFromArgs(gcpComputeEngineCUDPurchaseArgs{ + Region: "us-central1", MachineType: "n2-standard-64", VCPUCount: 64, MemoryGB: 256, TermYears: 1, + }) + require.NoError(t, err) + rec.Account = "test-project" + + p := &svcProvider{fakeProvider: fakeProvider{name: "gcp"}, byService: map[common.ServiceType]provider.ServiceClient{common.ServiceCompute: client}} + kept, d, err := runDedupe(t, p, []common.Recommendation{rec}) + require.NoError(t, err) + assert.Len(t, kept, 1, "2 vCPU bought must not hide a 64 vCPU recommendation") + require.Len(t, d.Flagged, 1) + assert.Equal(t, statusPossiblyCovered, d.Flagged[0].Status) + assert.Contains(t, d.Flagged[0].Reason, "recent CUD") +} + +func TestSearchResultCarriesTheDedupeBlock(t *testing.T) { + client := &fakeRecommendationsClient{recs: []common.Recommendation{rdsRec(2, "1yr", "all-upfront", dedupeCaller)}} + fp := &fakeProvider{name: "aws", services: []common.ServiceType{common.ServiceRDS}, recClient: client, + svc: rdsClient(), accounts: []common.Account{{ID: dedupeCaller, IsDefault: true}}} + _, result, err := newTestSearchTool(fp).handle(context.Background(), nil, searchRecommendationsArgs{ + Provider: "aws", Service: "rds", Region: "us-east-1", TermYears: 1, PaymentOption: "all-upfront", + }) + require.NoError(t, err) + assert.Equal(t, 0, result.Count) + raw, err := json.Marshal(result) + require.NoError(t, err) + var wire map[string]any + require.NoError(t, json.Unmarshal(raw, &wire)) + d, ok := wire["dedupe"].(map[string]any) + require.True(t, ok, "%s", raw) + assert.Len(t, d["suppressed"], 1, "a suppressed rec is listed, never silently dropped") +} diff --git a/tools/search_recommendations.go b/tools/search_recommendations.go index 984b4d1..f649f40 100644 --- a/tools/search_recommendations.go +++ b/tools/search_recommendations.go @@ -40,7 +40,11 @@ const searchRecommendationsDescription = "Search for reserved-capacity purchase "one or more Cost Explorer requests (up to 6 when term_years and payment_option are both omitted, more " + "with pagination), and AWS bills those per request, so avoid re-running the same search in a loop. Use " + "this first to find what to buy, then feed a result's region/resource_type/count into the matching " + - "cudly____purchase tool." + "cudly____purchase tool. Results are checked against commitments bought in " + + "the last 24 hours in the credentials' own account: a fully covered RDS, OpenSearch, Redshift or " + + "ElastiCache recommendation is removed and listed under dedupe.suppressed; EC2, Azure, GCP and MemoryDB " + + "recommendations that may be covered stay in the results and are listed under dedupe.flagged " + + "(possibly_covered). Check dedupe before acting. A failure to list existing commitments fails the search." // searchRecommendationsArgs mirrors common.RecommendationParams, adding the // provider selector and the optional per-call credential overrides from the @@ -65,6 +69,7 @@ type searchRecommendationsArgs struct { // searchRecommendationsResult is the tool's structured output. type searchRecommendationsResult struct { + Dedupe searchDedupe `json:"dedupe"` Recommendations []common.Recommendation `json:"recommendations"` Count int `json:"count"` } @@ -153,7 +158,12 @@ func (t *searchRecommendationsTool) handle(ctx context.Context, _ *mcp.CallToolR return nil, searchRecommendationsResult{}, err } - return nil, searchRecommendationsResult{Count: len(recs), Recommendations: recs}, nil + recs, dedupe, err := dedupeSearchResults(ctx, prov, recs) + if err != nil { + return nil, searchRecommendationsResult{}, err + } + + return nil, searchRecommendationsResult{Count: len(recs), Recommendations: recs, Dedupe: dedupe}, nil } // searchCombo is one (term, payment option) pair to query. Cost Explorer's diff --git a/tools/search_recommendations_test.go b/tools/search_recommendations_test.go index 455e167..3df0bfb 100644 --- a/tools/search_recommendations_test.go +++ b/tools/search_recommendations_test.go @@ -65,6 +65,9 @@ var _ provider.RecommendationsClient = (*fakeRecommendationsClient)(nil) type fakeProvider struct { recClient provider.RecommendationsClient recErr error + svc provider.ServiceClient + svcErr error + accounts []common.Account name string services []common.ServiceType } @@ -77,7 +80,7 @@ func (f *fakeProvider) GetCredentials() (provider.Credentials, error) { } func (f *fakeProvider) ValidateCredentials(_ context.Context) error { return nil } func (f *fakeProvider) GetAccounts(_ context.Context) ([]common.Account, error) { - return nil, nil + return f.accounts, nil } func (f *fakeProvider) GetRegions(_ context.Context) ([]common.Region, error) { return nil, nil @@ -87,7 +90,10 @@ func (f *fakeProvider) GetSupportedServices() []common.ServiceType { return f.services } func (f *fakeProvider) GetServiceClient(_ context.Context, _ common.ServiceType, _ string) (provider.ServiceClient, error) { - return nil, nil + if f.svc != nil { + return f.svc, f.svcErr + } + return &fakeServiceClient{}, f.svcErr } func (f *fakeProvider) GetRecommendationsClient(_ context.Context) (provider.RecommendationsClient, error) { return f.recClient, f.recErr From 173d3b5ee8a807cb72ba1dca651d47f1c4b57139 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 9 Oct 2026 18:58:07 +0200 Subject: [PATCH 2/2] test(mcp): pin Azure flag-only, name the right missing permission, add protocol test An Azure rec covered by a recent reservation must be flagged, never suppressed. A failure to resolve the caller account now names sts:GetCallerIdentity or organizations:ListAccounts, and a service client failure claims no permission; only a failed listing names the describe permission. An in-memory MCP client test drives the search end to end. Refs #42 Co-Authored-By: Claude Sonnet 5.5 --- cmd/cudly-mcp/search_dedupe_protocol_test.go | 135 +++++++++++++++++++ tools/search_dedupe.go | 45 ++++++- tools/search_dedupe_test.go | 49 +++++++ tools/search_recommendations_test.go | 3 +- 4 files changed, 227 insertions(+), 5 deletions(-) create mode 100644 cmd/cudly-mcp/search_dedupe_protocol_test.go diff --git a/cmd/cudly-mcp/search_dedupe_protocol_test.go b/cmd/cudly-mcp/search_dedupe_protocol_test.go new file mode 100644 index 0000000..0008044 --- /dev/null +++ b/cmd/cudly-mcp/search_dedupe_protocol_test.go @@ -0,0 +1,135 @@ +package main + +import ( + "context" + "encoding/json" + "errors" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/pkg/provider" + awsprovider "github.com/LeanerCloud/cloud-commitments-go/providers/aws" + + cudlymcp "github.com/LeanerCloud/cloud-commitments-mcp" + + gosdk "github.com/modelcontextprotocol/go-sdk/mcp" +) + +const searchCallerAccount = "111111111111" + +func searchProtocolRec() common.Recommendation { + return common.Recommendation{ + Provider: common.ProviderAWS, Service: common.ServiceRDS, Account: searchCallerAccount, Region: "us-east-1", + ResourceType: "db.r6g.large", Count: 2, Term: "1yr", PaymentOption: "all-upfront", + Details: &common.DatabaseDetails{Engine: "MySQL", AZConfig: "multi-az"}, + } +} + +// searchRecsClient serves a fixed recommendation list. +type searchRecsClient struct{ provider.RecommendationsClient } + +func (searchRecsClient) GetRecommendations(_ context.Context, _ *common.RecommendationParams) ([]common.Recommendation, error) { + return []common.Recommendation{searchProtocolRec()}, nil +} + +// recentRDSClient lists one recent reservation that covers the recommendation. +type recentRDSClient struct{ provider.ServiceClient } + +func (recentRDSClient) GetExistingCommitments(_ context.Context) ([]common.Commitment, error) { + return []common.Commitment{{ + Provider: common.ProviderAWS, Service: common.ServiceRelationalDB, Region: "us-east-1", ResourceType: "db.r6g.large", + Engine: "mysql", Deployment: "multi-az", Count: 2, State: common.CommitmentStateActive, + StartDate: time.Now().Add(-time.Hour), + }}, nil +} + +type fakeSearchAWSProvider struct{} + +func (fakeSearchAWSProvider) Name() string { return "aws" } +func (fakeSearchAWSProvider) DisplayName() string { return "Amazon Web Services" } +func (fakeSearchAWSProvider) IsConfigured() bool { return true } +func (fakeSearchAWSProvider) GetCredentials() (provider.Credentials, error) { + return nil, errors.New("not implemented") +} +func (fakeSearchAWSProvider) ValidateCredentials(_ context.Context) error { + return errors.New("not implemented") +} +func (fakeSearchAWSProvider) GetAccounts(_ context.Context) ([]common.Account, error) { + return []common.Account{{ID: searchCallerAccount, IsDefault: true}}, nil +} +func (fakeSearchAWSProvider) GetRegions(_ context.Context) ([]common.Region, error) { + return nil, errors.New("not implemented") +} +func (fakeSearchAWSProvider) GetDefaultRegion() string { return "us-east-1" } +func (fakeSearchAWSProvider) GetSupportedServices() []common.ServiceType { + return []common.ServiceType{common.ServiceRDS} +} +func (fakeSearchAWSProvider) GetServiceClient(_ context.Context, service common.ServiceType, _ string) (provider.ServiceClient, error) { + if service != common.ServiceRDS { + return nil, errors.New("only rds is implemented") + } + return recentRDSClient{}, nil +} +func (fakeSearchAWSProvider) GetRecommendationsClient(_ context.Context) (provider.RecommendationsClient, error) { + return searchRecsClient{}, nil +} + +// TestSearchSuppressesCoveredRecommendationOverTheProtocol drives the search +// through the real server and an in-memory MCP client, with a fake aws +// provider: a recommendation covered by a recent reservation must leave the +// result and appear in dedupe.suppressed, and the structured output must pass +// the SDK's output-schema validation. +func TestSearchSuppressesCoveredRecommendationOverTheProtocol(t *testing.T) { + provider.GetRegistry().Unregister("aws") + require.NoError(t, provider.RegisterProvider("aws", func(_ *provider.ProviderConfig) (provider.Provider, error) { + return fakeSearchAWSProvider{}, nil + })) + t.Cleanup(func() { + provider.GetRegistry().Unregister("aws") + if err := provider.RegisterProvider("aws", func(cfg *provider.ProviderConfig) (provider.Provider, error) { + return awsprovider.NewAWSProvider(cfg) + }); err != nil { + t.Errorf("restore real aws provider factory: %v", err) + } + }) + + server, err := cudlymcp.NewServer("test-search-dedupe") + require.NoError(t, err) + clientTransport, serverTransport := gosdk.NewInMemoryTransports() + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + go func() { _ = server.Run(ctx, serverTransport) }() + + client := gosdk.NewClient(&gosdk.Implementation{Name: "test-client"}, nil) + session, err := client.Connect(ctx, clientTransport, nil) + require.NoError(t, err) + defer session.Close() + + result, err := session.CallTool(ctx, &gosdk.CallToolParams{ + Name: "cudly_search_recommendations", + Arguments: map[string]any{ + "provider": "aws", "service": "rds", "region": "us-east-1", "term_years": 1, "payment_option": "all-upfront", + }, + }) + require.NoError(t, err) + require.False(t, result.IsError, "%+v", result.Content) + + encoded, err := json.Marshal(result.StructuredContent) + require.NoError(t, err) + var out struct { + Dedupe struct { + Suppressed []map[string]any `json:"suppressed"` + } `json:"dedupe"` + Recommendations []any `json:"recommendations"` + Count int `json:"count"` + } + require.NoError(t, json.Unmarshal(encoded, &out), "%s", encoded) + assert.Equal(t, 0, out.Count, "%s", encoded) + assert.Empty(t, out.Recommendations, "%s", encoded) + require.Len(t, out.Dedupe.Suppressed, 1, "%s", encoded) + assert.Equal(t, searchCallerAccount, out.Dedupe.Suppressed[0]["account"]) +} diff --git a/tools/search_dedupe.go b/tools/search_dedupe.go index e3c0219..cb4b61a 100644 --- a/tools/search_dedupe.go +++ b/tools/search_dedupe.go @@ -2,8 +2,10 @@ package tools import ( "context" + "errors" "fmt" "reflect" + "strings" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/LeanerCloud/cloud-commitments-go/pkg/provider" @@ -222,8 +224,7 @@ func dedupeSearchResults(ctx context.Context, prov provider.Provider, recs []com continue } if err := checkGroup(ctx, prov, checker, &callerAccount, recs, idxs, mode, reason, decisions); err != nil { - return nil, searchDedupe{}, fmt.Errorf("dedupe check failed for %s in %s: %w; the credentials need %s to list existing commitments; no recommendations returned", - gk.service, gk.region, err, listPermission(first)) + return nil, searchDedupe{}, dedupeFailure(gk, first, err) } out.Groups = append(out.Groups, group) } @@ -250,6 +251,38 @@ func dedupeSearchResults(ctx context.Context, prov provider.Provider, recs []com return kept, out, nil } +// accountLookupError is a failure to resolve the caller's account, which needs +// a different permission than listing commitments. +type accountLookupError struct { + err error + permission string +} + +func (e *accountLookupError) Error() string { return e.err.Error() } +func (e *accountLookupError) Unwrap() error { return e.err } + +// listingError is a failure to list existing commitments. +type listingError struct{ err error } + +func (e *listingError) Error() string { return e.err.Error() } +func (e *listingError) Unwrap() error { return e.err } + +// dedupeFailure builds the loud failure, naming only the permission that the +// failing step actually needs. +func dedupeFailure(gk groupKey, first common.Recommendation, err error) error { + var acct *accountLookupError + var list *listingError + switch { + case errors.As(err, &acct): + return fmt.Errorf("dedupe check failed for %s in %s: %w; the credentials need %s to identify the calling account; no recommendations returned", + gk.service, gk.region, err, acct.permission) + case errors.As(err, &list): + return fmt.Errorf("dedupe check failed for %s in %s: %w; the credentials need %s to list existing commitments; no recommendations returned", + gk.service, gk.region, err, listPermission(first)) + } + return fmt.Errorf("dedupe check failed for %s in %s: %w; no recommendations returned", gk.service, gk.region, err) +} + type recDecision struct { status string reason string @@ -296,7 +329,7 @@ func checkGroup(ctx context.Context, prov provider.Provider, checker *recfilter. } passed, filtered, err := checker.AdjustRecommendationsForExisting(ctx, batch, client) if err != nil { - return err + return &listingError{err: err} } outcomes, err := pairOutcomes(batch, passed, filtered) if err != nil { @@ -408,7 +441,11 @@ func (a *accountResolver) callerID(ctx context.Context) (string, error) { } accounts, err := a.prov.GetAccounts(ctx) if err != nil { - return "", fmt.Errorf("resolve caller account: %w", err) + permission := "sts:GetCallerIdentity" + if strings.Contains(err.Error(), "organizations") { + permission = "organizations:ListAccounts" + } + return "", &accountLookupError{err: fmt.Errorf("resolve caller account: %w", err), permission: permission} } a.done = true for _, acct := range accounts { diff --git a/tools/search_dedupe_test.go b/tools/search_dedupe_test.go index b8519a6..50dffcf 100644 --- a/tools/search_dedupe_test.go +++ b/tools/search_dedupe_test.go @@ -385,3 +385,52 @@ func TestSearchResultCarriesTheDedupeBlock(t *testing.T) { require.True(t, ok, "%s", raw) assert.Len(t, d["suppressed"], 1, "a suppressed rec is listed, never silently dropped") } + +// Azure's reservation key ignores scope and term, so it is flag-only: a +// commitment that would cover the rec must produce a flag, never a removal. +func TestDedupeAzureIsFlagOnlyNeverSuppressed(t *testing.T) { + lc := &listingClient{commitments: []common.Commitment{{ + Provider: common.ProviderAzure, Service: common.ServiceCompute, Region: "eastus", ResourceType: "Standard_D2s_v3", + Count: 4, State: common.CommitmentStateActive, StartDate: dedupeRecent, + }}} + p := &svcProvider{fakeProvider: fakeProvider{name: "azure"}, byService: map[common.ServiceType]provider.ServiceClient{common.ServiceCompute: lc}} + rec := common.Recommendation{ + Provider: common.ProviderAzure, Service: common.ServiceCompute, Region: "eastus", ResourceType: "Standard_D2s_v3", + Count: 4, Term: "1yr", PaymentOption: "upfront", + } + kept, d, err := runDedupe(t, p, []common.Recommendation{rec}) + require.NoError(t, err) + assert.Equal(t, []common.Recommendation{rec}, kept, "an Azure rec covered by a recent reservation must stay visible") + assert.Empty(t, d.Suppressed) + require.Len(t, d.Flagged, 1) + assert.Equal(t, statusPossiblyCovered, d.Flagged[0].Status) + assert.Equal(t, 1, lc.listCalls) +} + +// The error must name the permission that is actually missing. +func TestDedupeAccountLookupFailureNamesTheRightPermission(t *testing.T) { + cases := map[string]struct{ err, want, notWant string }{ + "sts": {"failed to get current account: AccessDenied", "sts:GetCallerIdentity", "rds:DescribeReservedDBInstances"}, + "organizations": {"organizations: list accounts: AccessDenied", "organizations:ListAccounts", "rds:DescribeReservedDBInstances"}, + } + for name, c := range cases { + t.Run(name, func(t *testing.T) { + p := awsProvider(map[common.ServiceType]provider.ServiceClient{common.ServiceRDS: &listingClient{}}) + p.acctErr = errors.New(c.err) + kept, _, err := runDedupe(t, p, []common.Recommendation{rdsRec(2, "1yr", "all-upfront", dedupeCaller)}) + require.Error(t, err) + assert.Nil(t, kept) + assert.Contains(t, err.Error(), c.want) + assert.NotContains(t, err.Error(), c.notWant) + assert.Contains(t, err.Error(), "AccessDenied") + }) + } +} + +func TestDedupeServiceClientFailureClaimsNoPermission(t *testing.T) { + p := awsProvider(nil) + p.byService = map[common.ServiceType]provider.ServiceClient{} + _, _, err := runDedupe(t, p, []common.Recommendation{rdsRec(2, "1yr", "all-upfront", dedupeCaller)}) + require.Error(t, err) + assert.NotContains(t, err.Error(), "the credentials need") +} diff --git a/tools/search_recommendations_test.go b/tools/search_recommendations_test.go index 3df0bfb..b45349c 100644 --- a/tools/search_recommendations_test.go +++ b/tools/search_recommendations_test.go @@ -68,6 +68,7 @@ type fakeProvider struct { svc provider.ServiceClient svcErr error accounts []common.Account + acctErr error name string services []common.ServiceType } @@ -80,7 +81,7 @@ func (f *fakeProvider) GetCredentials() (provider.Credentials, error) { } func (f *fakeProvider) ValidateCredentials(_ context.Context) error { return nil } func (f *fakeProvider) GetAccounts(_ context.Context) ([]common.Account, error) { - return f.accounts, nil + return f.accounts, f.acctErr } func (f *fakeProvider) GetRegions(_ context.Context) ([]common.Region, error) { return nil, nil