From a8fb5fb8c947de0a757111dabbca3338a29f0860 Mon Sep 17 00:00:00 2001 From: Rafid Aslam Date: Fri, 4 Sep 2026 08:20:35 +0700 Subject: [PATCH] feat(infracost): name the cost being measured and what it covers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A cost message was the comparison and nothing else: `300.1` is not less than or equal to `20` which says neither which cost was measured nor what it covered. A monthly and an hourly figure of the same size read identically, and the reader has to go and open the policy to find out which one they are looking at. Now: [all resources (2 resources)] total_monthly_cost: `300.1` is not less than or equal to `20` [aws_eks_cluster, aws_s3_bucket (1 resource)] total_monthly_cost: `100.1` is not less than or equal to `-1` This uses the same provider `context` mechanism terraform_plan uses, so it is also carried as structured fields — `operation_type`, `resource_type`, `matched_resources`, `currency`. The resource count is there for a specific trap. `resource_type` is matched against the breakdown, so a typo produces a cost of 0, and `LessThan 20` passes while measuring nothing at all. The verdict is unchanged — a policy that passed still passes — but `[aws_instances (0 resources)]` makes the reason visible rather than leaving a green check on an empty measurement. Two smaller things come with it: `format_context_prefix` gained a `qualifier` key, which renders in the same parenthetical as `action` for providers that have no planned action, and the core's non-`ProviderError` error branch now gets the context prefix like every other message. --- CHANGELOG.md | 9 ++ src/tirith/core/core.py | 6 +- src/tirith/providers/common.py | 8 +- src/tirith/providers/infracost/handler.py | 91 +++++++++-- .../test_infracost_message_context.py | 151 ++++++++++++++++++ 5 files changed, 252 insertions(+), 13 deletions(-) create mode 100644 tests/providers/infracost/test_infracost_message_context.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 79662ed0..7c852165 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -47,6 +47,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 of repeating the same text once per resource — with `terraform_resource_type: "*"` it was emitted once per resource with identical text. - `core`: "Could not find input value" now names the provider arguments that produced no value. +- `infracost`: Cost messages now name the cost that was measured and what it covered, e.g. + ``[all resources (2 resources)] total_monthly_cost: `300.1` is not less than or equal to `20` `` + instead of ``` `300.1` is not less than or equal to `20` ```. A monthly and an hourly figure of + the same size were previously indistinguishable. +- `infracost`: A `resource_type` that matches no resource now says so — `[aws_instances + (0 resources)]` — instead of reporting a genuine-looking `0`. A typo'd resource type silently + satisfied a `LessThan` while measuring nothing; the verdict is unchanged, the message is not. +- `core`: A provider error reported without a `ProviderError` severity now gets the same context + prefix as every other message. ### Fixed - **Verdict change.** A resource skipped through `error_tolerance` no longer overwrites the diff --git a/src/tirith/core/core.py b/src/tirith/core/core.py index ffdd8054..ee3ce061 100644 --- a/src/tirith/core/core.py +++ b/src/tirith/core/core.py @@ -119,7 +119,11 @@ def generate_evaluator_result(evaluator_obj, input_data, provider_module): # is discarded and `None` is evaluated against the condition, so a typo'd operation_type # reads as a genuine violation. if evaluator_input.get("err") and not isinstance(evaluator_input["value"], ProviderError): - evaluation_results.append({"passed": False, "message": evaluator_input["err"]}) + context = evaluator_input.get("context") + err_result = {"passed": False, "message": format_context_prefix(context) + evaluator_input["err"]} + if context: + err_result["context"] = context + evaluation_results.append(err_result) has_failure = True continue diff --git a/src/tirith/providers/common.py b/src/tirith/providers/common.py index 2c71e3fb..19dd101f 100644 --- a/src/tirith/providers/common.py +++ b/src/tirith/providers/common.py @@ -187,6 +187,10 @@ def format_context_prefix(context: Optional[Dict]) -> str: count). Only used when ``resource_address`` is absent. ``action`` Planned action(s) for the resource. Rendered next to the subject. + ``qualifier`` + Anything else worth saying about the subject, rendered in the same parenthetical as + ``action`` for providers that have no planned action -- how many resources a cost + covers, say. Ignored when ``action`` is present. ``attribute`` Name of the attribute being evaluated. @@ -207,6 +211,8 @@ def format_context_prefix(context: Optional[Dict]) -> str: '[aws_vpc.main] action: ' >>> format_context_prefix({"label": "aws_vpc", "attribute": "count"}) '[aws_vpc] count: ' + >>> format_context_prefix({"label": "aws_instance", "qualifier": "0 matched", "attribute": "cost"}) + '[aws_instance (0 matched)] cost: ' >>> format_context_prefix({"attribute": "terraform_version"}) 'terraform_version: ' >>> format_context_prefix({"resource_address": "aws_vpc.main", "action": "create"}) @@ -218,7 +224,7 @@ def format_context_prefix(context: Optional[Dict]) -> str: return "" subject = context.get("resource_address") or context.get("label") - action = context.get("action") + action = context.get("action") or context.get("qualifier") attribute = context.get("attribute") subject_prefix = "" diff --git a/src/tirith/providers/infracost/handler.py b/src/tirith/providers/infracost/handler.py index 956cc5f1..786b86d0 100644 --- a/src/tirith/providers/infracost/handler.py +++ b/src/tirith/providers/infracost/handler.py @@ -10,6 +10,7 @@ def __get_all_costs(operation_type, input_data): "total_hourly_cost": ["totalHourlyCost", "hourlyCost"], } total_sum = 0 + matched_count = 0 if "projects" in input_data: for project in input_data["projects"]: if "breakdown" in project and "resources" in project["breakdown"]: @@ -20,6 +21,7 @@ def __get_all_costs(operation_type, input_data): and resource[pointer[operation_type][0]] != "null" ): total_sum += float(resource[pointer[operation_type][0]]) + matched_count += 1 elif ( pointer[operation_type][1] in resource and resource[pointer[operation_type][1]] @@ -27,11 +29,12 @@ def __get_all_costs(operation_type, input_data): ): # Support new schema for Infracost total_sum += float(resource[pointer[operation_type][1]]) + matched_count += 1 else: pass # raise KeyError(f'{costType} not found in one of the resource') logger.debug(f"Total sum of {operation_type} of all resources : {total_sum}") - return total_sum + return total_sum, matched_count else: raise KeyError("breakdown/resources not found in one of the project") else: @@ -46,6 +49,7 @@ def __get_resources_costs(resource_type, operation_type, input_data): "total_hourly_cost": ["totalHourlyCost", "hourlyCost"], } total_sum = 0 + matched_count = 0 if "projects" in input_data: for project in input_data["projects"]: if "breakdown" in project and "resources" in project["breakdown"]: @@ -58,6 +62,7 @@ def __get_resources_costs(resource_type, operation_type, input_data): and resource[pointer[operation_type][0]] != "null" ): total_sum += float(resource[pointer[operation_type][0]]) + matched_count += 1 elif ( pointer[operation_type][1] in resource and "name" in resource @@ -66,32 +71,96 @@ def __get_resources_costs(resource_type, operation_type, input_data): and resource[pointer[operation_type][1]] != "null" ): total_sum += float(resource[pointer[operation_type][1]]) + matched_count += 1 else: pass # raise KeyError(f'{costType} not found in one of the resource') logger.debug(f"Total sum of {operation_type} of specific resources : {total_sum}") - return total_sum + return total_sum, matched_count else: raise KeyError("breakdown/resources not found in one of the project") else: raise KeyError("projects not found in input_data") +def _is_every_resource(resource_type): + """ + Whether the `resource_type` provider arg means "every resource". + + The same three spellings `provide` accepts, kept in one place so the message agrees with + what was actually measured. + + :param resource_type: The `resource_type` provider arg + :return: True when the arg covers every resource + """ + return not resource_type or resource_type == "*" or resource_type == ["*"] + + +def _describe_resource_type(resource_type): + """ + Render the `resource_type` provider arg as the subject of a message. + + :param resource_type: The `resource_type` provider arg, a string or a list of strings + :return: A human-readable subject + """ + if _is_every_resource(resource_type): + return "all resources" + if isinstance(resource_type, (list, tuple)): + return ", ".join(str(item) for item in resource_type) + return str(resource_type) + + +def _cost_context(operation_type, input_data, resource_type=None, matched_count=None): + """ + Build the result context for a cost figure. + + A cost message used to be nothing but the comparison -- ```0` is less than `20``` -- which + says neither which cost was measured nor what it covered. Two figures of the same size mean + very different things depending on whether they are monthly or hourly, and a `resource_type` + that matches nothing yields a genuine-looking 0 that quietly passes a `LessThan`. Both are + named here. + + :param operation_type: The cost being measured, e.g. `total_monthly_cost` + :param input_data: The infracost breakdown, read for its currency + :param resource_type: The `resource_type` provider arg, or None for every resource + :param matched_count: How many resources contributed a cost, if known + :return: The context dictionary + """ + context = { + "operation_type": operation_type, + "label": _describe_resource_type(resource_type), + "attribute": operation_type, + } + if not _is_every_resource(resource_type): + context["resource_type"] = resource_type + if matched_count is not None: + context["matched_resources"] = matched_count + context["qualifier"] = f"{matched_count} resource{'' if matched_count == 1 else 's'}" + currency = input_data.get("currency") if isinstance(input_data, dict) else None + if currency: + context["currency"] = currency + return context + + def provide(provider_args, input_data): logger.debug("infracost provider") logger.debug(f"infracost provider inputs : {provider_args}") + operation_type = provider_args.get("operation_type") + resource_type = provider_args.get("resource_type") try: if "resource_type" in provider_args and "operation_type" in provider_args: - resource_type = provider_args["resource_type"] - operation_type = provider_args["operation_type"] - if not resource_type or resource_type == "*" or resource_type == ["*"]: - value = __get_all_costs(operation_type, input_data) - output = [{"value": value, "meta": None, "err": None}] - return output + if _is_every_resource(resource_type): + value, matched_count = __get_all_costs(operation_type, input_data) + context = _cost_context(operation_type, input_data, matched_count=matched_count) + return [{"value": value, "meta": None, "err": None, "context": context}] else: - value = __get_resources_costs(resource_type, operation_type, input_data) - return [{"value": value, "meta": None, "err": None}] + value, matched_count = __get_resources_costs(resource_type, operation_type, input_data) + context = _cost_context(operation_type, input_data, resource_type, matched_count) + return [{"value": value, "meta": None, "err": None, "context": context}] else: raise KeyError("resource_type/operation_type not found in provider_args") except KeyError as e: - return [{"value": None, "meta": None, "err": str(e)}] + # Name what was being measured, so a missing `projects` key is not reported against a + # policy the reader has to go and look up + context = _cost_context(operation_type, input_data, resource_type) if operation_type else None + return [{"value": None, "meta": None, "err": str(e), "context": context}] diff --git a/tests/providers/infracost/test_infracost_message_context.py b/tests/providers/infracost/test_infracost_message_context.py new file mode 100644 index 00000000..efbce658 --- /dev/null +++ b/tests/providers/infracost/test_infracost_message_context.py @@ -0,0 +1,151 @@ +""" +Tests for the context the infracost provider attaches to its results. + +A cost message used to be the comparison and nothing else -- ```0` is less than `20``` -- which +says neither which cost was measured nor what it covered. Monthly and hourly figures are +indistinguishable, and a `resource_type` matching nothing produces a real-looking 0 that quietly +satisfies a `LessThan`. +""" + +import json +import os + +from pytest import mark + +from tirith.core.core import start_policy_evaluation_from_dict +from tirith.providers.infracost import handler + + +def load_json(name): + with open(os.path.join(os.path.dirname(os.path.abspath(__file__)), name)) as f: + return json.load(f) + + +INPUT = load_json("input.json") + + +def evaluate(provider_args, condition, input_data=None): + policy = { + "meta": {"version": "v1", "required_provider": "stackguardian/infracost"}, + "evaluators": [{"id": "cost", "provider_args": provider_args, "condition": condition}], + "eval_expression": "cost", + } + return start_policy_evaluation_from_dict(policy, input_data if input_data is not None else INPUT) + + +def results_of(result): + return result["evaluators"][0]["result"] + + +@mark.passing +def test_total_cost_message_names_the_metric_and_its_scope(): + result = evaluate( + {"operation_type": "total_monthly_cost", "resource_type": ["*"]}, + {"type": "LessThanEqualTo", "value": 20}, + ) + + assert [item["message"] for item in results_of(result)] == [ + "[all resources (2 resources)] total_monthly_cost: `300.1` is not less than or equal to `20`" + ] + + +@mark.passing +def test_specific_resource_types_are_named(): + result = evaluate( + {"operation_type": "total_monthly_cost", "resource_type": ["aws_eks_cluster", "aws_s3_bucket"]}, + {"type": "LessThanEqualTo", "value": -1}, + ) + + assert [item["message"] for item in results_of(result)] == [ + "[aws_eks_cluster, aws_s3_bucket (1 resource)] total_monthly_cost: `100.1` is not less than or equal to `-1`" + ] + + +@mark.passing +def test_hourly_and_monthly_are_no_longer_indistinguishable(): + monthly = evaluate( + {"operation_type": "total_monthly_cost", "resource_type": ["*"]}, {"type": "LessThan", "value": 1} + ) + hourly = evaluate({"operation_type": "total_hourly_cost", "resource_type": ["*"]}, {"type": "LessThan", "value": 1}) + + assert "total_monthly_cost:" in results_of(monthly)[0]["message"] + assert "total_hourly_cost:" in results_of(hourly)[0]["message"] + + +@mark.passing +def test_a_resource_type_matching_nothing_says_so_instead_of_reporting_a_bare_zero(): + # The trap this closes: a typo'd resource_type costs 0, and `LessThan 20` passes while + # measuring nothing at all. The verdict is unchanged -- the message now admits why. + result = evaluate( + {"operation_type": "total_monthly_cost", "resource_type": ["aws_instances"]}, + {"type": "LessThan", "value": 20}, + ) + item = results_of(result)[0] + + assert item["passed"] is True + assert item["message"] == "[aws_instances (0 resources)] total_monthly_cost: `0` is less than `20`" + assert item["context"]["matched_resources"] == 0 + + +@mark.passing +def test_context_carries_the_same_detail_as_the_message(): + result = evaluate( + {"operation_type": "total_monthly_cost", "resource_type": ["aws_s3_bucket"]}, + {"type": "LessThan", "value": 20}, + ) + + assert results_of(result)[0]["context"] == { + "operation_type": "total_monthly_cost", + "label": "aws_s3_bucket", + "attribute": "total_monthly_cost", + "resource_type": ["aws_s3_bucket"], + "matched_resources": 1, + "qualifier": "1 resource", + "currency": "USD", + } + + +@mark.passing +def test_a_string_resource_type_is_accepted_as_well_as_a_list(): + result = evaluate( + {"operation_type": "total_monthly_cost", "resource_type": "aws_s3_bucket"}, + {"type": "LessThan", "value": 20}, + ) + + assert "[aws_s3_bucket (1 resource)]" in results_of(result)[0]["message"] + + +@mark.passing +def test_a_malformed_breakdown_names_the_metric_it_could_not_measure(): + # Previously "projects not found in input_data" arrived with no indication of which + # evaluator it belonged to + result = evaluate( + {"operation_type": "total_monthly_cost", "resource_type": ["*"]}, + {"type": "LessThan", "value": 20}, + input_data={"currency": "USD"}, + ) + item = results_of(result)[0] + + assert item["passed"] is False + assert item["message"] == "[all resources] total_monthly_cost: 'projects not found in input_data'" + + +@mark.passing +def test_missing_provider_args_still_report_without_a_context(): + # Nothing is known about what was being measured, so there is nothing to name + result = evaluate({"resource_type": ["*"]}, {"type": "LessThan", "value": 20}) + item = results_of(result)[0] + + assert item["passed"] is False + assert item["message"] == "'resource_type/operation_type not found in provider_args'" + assert "context" not in item + + +@mark.passing +def test_the_cost_walks_still_return_the_same_totals(): + # The walks now return (total, matched_count); the totals themselves must not move + get_all_costs = getattr(handler, "__get_all_costs") + get_resources_costs = getattr(handler, "__get_resources_costs") + + assert get_all_costs("total_monthly_cost", INPUT) == (300.1, 2) + assert get_resources_costs(["aws_s3_bucket"], "total_monthly_cost", INPUT) == (100.1, 1)