Skip to content

Commit 6d62b67

Browse files
fix(billing-cost-management-mcp-server): reject multi-value SERVICE filters in get_reservation_coverage (#4888)
* fix(billing-cost-management-mcp-server): reject multi-value SERVICE filters in get_reservation_coverage Without group_by, GetReservationCoverage returns one CoveragesByTime entry per SERVICE value for the same time period, and no entry says which service it holds. The tool merges same-period entries as pages and keeps only the first entry's Total, so the period total silently dropped every service but one while the top-level total still covered all of them. With group_by, the groups of every service come back mixed, with no service attribute, so two groups can carry the same attributes. The description already said "one SERVICE value per call"; this enforces it. Before any API call, get_reservation_coverage walks the filter (Dimensions and nested And / Or) and returns a validation_error naming the values when it selects more than one distinct SERVICE value, telling the caller to call once per service. Values under Not are not counted: they exclude services rather than select them, and a live Not filter on SERVICE returns no per-service split. Repeated copies of one value count once. * Update src/billing-cost-management-mcp-server/awslabs/billing_cost_management_mcp_server/tools/ri_performance_tools.py Co-authored-by: finklen-amazon <finklen@amazon.com> --------- Co-authored-by: finklen-amazon <finklen@amazon.com>
1 parent e36627a commit 6d62b67

2 files changed

Lines changed: 140 additions & 2 deletions

File tree

‎src/billing-cost-management-mcp-server/awslabs/billing_cost_management_mcp_server/tools/ri_performance_tools.py‎

Lines changed: 37 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,9 @@
7171
SERVICE, INVOICING_ENTITY and OPERATING_SYSTEM are NOT valid group_by attributes here —
7272
grouping coverage by them returns a ValidationException. SERVICE is a FILTER dimension instead: pass it in
7373
`filter` (one SERVICE value per call; if you omit a SERVICE filter, Cost Explorer
74-
defaults to EC2), and call once per service to compare across services.
74+
defaults to EC2), and call once per service to compare across services. A filter
75+
with more than one SERVICE value is rejected, because Cost Explorer returns each
76+
service's figures without saying which service they belong to.
7577
Reservation coverage applies only to reservable services. The valid SERVICE
7678
filter values are "Amazon Elastic Compute Cloud - Compute" (the default when
7779
omitted), "Amazon Relational Database Service", "Amazon ElastiCache",
@@ -147,7 +149,7 @@ async def ri_performance(
147149
granularity: Time granularity of the data (DAILY or MONTHLY). Defaults to DAILY. Ignored when group_by is provided (granularity and group_by are mutually exclusive).
148150
metrics: List of metrics to retrieve for coverage as a JSON string (e.g., '["Hour", "Cost"]'). Valid values are Hour, Unit and Cost; Unit is EC2 only. Defaults to Hour only.
149151
group_by: Optional grouping as a JSON string. For coverage: AZ, CACHE_ENGINE, DATABASE_ENGINE, DEPLOYMENT_OPTION, INSTANCE_TYPE, INSTANCE_TYPE_FAMILY, LINKED_ACCOUNT, PLATFORM, REGION, or TENANCY, depending on the service (see the tool description) (SERVICE is NOT valid — filter on it instead). For utilization: SUBSCRIPTION_ID only.
150-
filter: Optional filter as a JSON string (e.g., by service, region, or instance type). SERVICE defaults to EC2 when omitted; valid coverage SERVICE values are "Amazon Elastic Compute Cloud - Compute", "Amazon Relational Database Service", "Amazon ElastiCache", "Amazon MemoryDB", "Amazon Redshift", "Amazon OpenSearch Service", and "Amazon Elasticsearch Service" (use the full display name, not "EC2"/"RDS"). SubscriptionId values are numeric, not ARNs/UUIDs. A wide grouped/DAILY query may exceed the result size cap (ToolResultOversized) — scope it with a filter, a shorter window, or MONTHLY granularity.
152+
filter: Optional filter as a JSON string (e.g., by service, region, or instance type). SERVICE defaults to EC2 when omitted; valid coverage SERVICE values are "Amazon Elastic Compute Cloud - Compute", "Amazon Relational Database Service", "Amazon ElastiCache", "Amazon MemoryDB", "Amazon Redshift", "Amazon OpenSearch Service", and "Amazon Elasticsearch Service" (use the full display name, not "EC2"/"RDS"); coverage takes one SERVICE value per call and rejects more. SubscriptionId values are numeric, not ARNs/UUIDs. A wide grouped/DAILY query may exceed the result size cap (ToolResultOversized) — scope it with a filter, a shorter window, or MONTHLY granularity.
151153
sort_by: Optional sorting configuration as a JSON string with key and direction (ASCENDING or DESCENDING).
152154
max_items: Total number of groups to return across all pages. Omit it to fetch every
153155
page. Applies only to get_reservation_coverage with group_by set.
@@ -206,6 +208,28 @@ async def ri_performance(
206208
return await handle_aws_error(ctx, e, 'ri_performance', 'Cost Explorer')
207209

208210

211+
def _selected_services(expression: Any) -> List[str]:
212+
"""Return the distinct SERVICE values a filter expression selects, in order.
213+
214+
Walks Dimensions and nested And / Or. Values under Not are not counted: Cost Explorer
215+
does not split coverage by them (a Not on SERVICE matches no usage).
216+
"""
217+
if not isinstance(expression, dict):
218+
return []
219+
services: List[str] = []
220+
dimension = expression.get('Dimensions')
221+
if isinstance(dimension, dict) and dimension.get('Key') == 'SERVICE':
222+
values = dimension.get('Values')
223+
if isinstance(values, list):
224+
services.extend(v for v in values if isinstance(v, str))
225+
for operator in ('And', 'Or'):
226+
operands = expression.get(operator)
227+
if isinstance(operands, list):
228+
for operand in operands:
229+
services.extend(_selected_services(operand))
230+
return list(dict.fromkeys(services))
231+
232+
209233
def _count_groups(entries: List[Dict[str, Any]]) -> int:
210234
"""Count the groups across a page's time-period entries."""
211235
return sum(len(entry.get('Groups') or []) for entry in entries)
@@ -316,6 +340,17 @@ async def get_reservation_coverage(
316340

317341
if filter_expr:
318342
request_params['Filter'] = parse_json(filter_expr, 'filter')
343+
# With several SERVICE values the API returns one entry per service for the same
344+
# period (grouped: groups of every service), none saying which service it holds.
345+
services = _selected_services(request_params['Filter'])
346+
if len(services) > 1:
347+
named = ', '.join(f'"{service}"' for service in services)
348+
raise ValueError(
349+
'get_reservation_coverage takes one SERVICE value per call, but the filter '
350+
f'names {len(services)}: {named}. Cost Explorer would return their figures '
351+
'without saying which service each belongs to; call once per service and '
352+
'combine the results.'
353+
)
319354

320355
if sort_by:
321356
request_params['SortBy'] = parse_json(sort_by, 'sort_by')

‎src/billing-cost-management-mcp-server/tests/tools/test_ri_performance_tools.py‎

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
"""
2424

2525
import botocore.session
26+
import json
2627
import pytest
2728
import re
2829
from awslabs.billing_cost_management_mcp_server.tools.ri_performance_tools import (
@@ -1258,6 +1259,108 @@ async def test_ungrouped_coverage_sends_granularity_to_api_call(self, mock_conte
12581259
assert kwargs['Granularity'] == 'MONTHLY'
12591260

12601261

1262+
EC2_SERVICE = 'Amazon Elastic Compute Cloud - Compute'
1263+
RDS_SERVICE = 'Amazon Relational Database Service'
1264+
REGION_FILTER = {'Dimensions': {'Key': 'REGION', 'Values': ['us-east-1', 'us-west-2']}}
1265+
1266+
1267+
def _service_filter(*values):
1268+
return {'Dimensions': {'Key': 'SERVICE', 'Values': list(values)}}
1269+
1270+
1271+
@pytest.mark.asyncio
1272+
class TestCoverageServiceFilter:
1273+
"""GetReservationCoverage takes one SERVICE value per call.
1274+
1275+
With several, the API returns one unlabeled entry per service for the same period (or,
1276+
grouped, groups of every service with no service attribute), so they are rejected.
1277+
"""
1278+
1279+
@pytest.mark.parametrize(
1280+
'filter_expr',
1281+
[
1282+
_service_filter(EC2_SERVICE, RDS_SERVICE),
1283+
{'And': [_service_filter(EC2_SERVICE, RDS_SERVICE), REGION_FILTER]},
1284+
{'Or': [_service_filter(EC2_SERVICE), _service_filter(RDS_SERVICE)]},
1285+
],
1286+
ids=['dimensions', 'nested_and', 'or_of_single_values'],
1287+
)
1288+
@pytest.mark.parametrize(
1289+
'group_by',
1290+
[None, '[{"Type": "DIMENSION", "Key": "REGION"}]'],
1291+
ids=['ungrouped', 'grouped'],
1292+
)
1293+
async def test_more_than_one_service_is_rejected_before_the_api_call(
1294+
self, mock_context, filter_expr, group_by
1295+
):
1296+
"""Two SERVICE values return a validation_error and Cost Explorer is never called."""
1297+
client = MagicMock()
1298+
1299+
result = await _call_tool(
1300+
client,
1301+
mock_context,
1302+
operation='get_reservation_coverage',
1303+
granularity='MONTHLY',
1304+
group_by=group_by,
1305+
filter=json.dumps(filter_expr),
1306+
)
1307+
1308+
assert result['status'] == 'error'
1309+
assert result['error_type'] == 'validation_error'
1310+
message = result['message']
1311+
assert 'one SERVICE value per call' in message
1312+
assert f'"{EC2_SERVICE}"' in message and f'"{RDS_SERVICE}"' in message
1313+
assert 'call once per service' in message
1314+
client.get_reservation_coverage.assert_not_called()
1315+
1316+
@pytest.mark.parametrize(
1317+
'filter_expr',
1318+
[
1319+
_service_filter(EC2_SERVICE),
1320+
_service_filter(RDS_SERVICE, RDS_SERVICE),
1321+
{'Not': _service_filter(EC2_SERVICE, RDS_SERVICE)},
1322+
{'And': [_service_filter(RDS_SERVICE), {'Not': _service_filter(EC2_SERVICE)}]},
1323+
REGION_FILTER,
1324+
# A malformed operand is left for Cost Explorer to reject.
1325+
{'And': ['not-an-expression', _service_filter(EC2_SERVICE)]},
1326+
{'Dimensions': {'Key': 'SERVICE', 'Values': RDS_SERVICE}},
1327+
],
1328+
ids=[
1329+
'one_value',
1330+
'repeated_value',
1331+
'not',
1332+
'one_value_and_not',
1333+
'no_service',
1334+
'malformed_operand',
1335+
'values_not_a_list',
1336+
],
1337+
)
1338+
async def test_at_most_one_selected_service_reaches_the_api(self, mock_context, filter_expr):
1339+
"""One distinct SERVICE value, or values only under Not, pass through unchanged."""
1340+
client = MagicMock()
1341+
client.get_reservation_coverage.return_value = {'CoveragesByTime': []}
1342+
1343+
result = await _call_tool(
1344+
client,
1345+
mock_context,
1346+
operation='get_reservation_coverage',
1347+
granularity='MONTHLY',
1348+
filter=json.dumps(filter_expr),
1349+
)
1350+
1351+
assert result['status'] == 'success'
1352+
assert client.get_reservation_coverage.call_args.kwargs['Filter'] == filter_expr
1353+
1354+
async def test_description_says_the_limit_is_enforced(self):
1355+
"""The description states that more than one SERVICE value is rejected."""
1356+
description = ' '.join((await _ri_description()).split())
1357+
assert 'more than one SERVICE value is rejected' in description
1358+
tool = await ri_performance_server.get_tool('ri-performance')
1359+
assert tool is not None
1360+
filter_doc = tool.parameters['properties']['filter'].get('description', '')
1361+
assert 'coverage takes one SERVICE value per call and rejects more' in filter_doc
1362+
1363+
12611364
@pytest.mark.asyncio
12621365
class TestMaxPages:
12631366
"""get_reservation_utilization takes max_pages; coverage keeps max_items."""

0 commit comments

Comments
 (0)