[SG-4917] feat(infracost): name the cost being measured and what it covers

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

src/tirith/core/core.py

@@ -119,24 +119,28 @@
             # 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
 
             if isinstance(evaluator_input["value"], ProviderError) and evaluator_input.get("err", None):
                 severity_value = evaluator_input["value"].severity_value
                 context = evaluator_input.get("context")
                 err_result = dict(message=format_context_prefix(context) + evaluator_input["err"])
                 if context:
                     err_result["context"] = context
 
                 if severity_value > evaluator_error_tolerance:
                     err_result.update(dict(passed=False))
                     evaluation_results.append(err_result)
                     has_failure = True
                     continue
                 # Within tolerance: this resource is skipped and does not touch the verdict
                 err_result.update(dict(passed=None))
                 evaluation_results.append(err_result)
                 continue
 
@@ -166,7 +170,7 @@
     return result
 
 
 def generate_compiled_code_without_none_and_variables(eval_str: str) -> Tuple[Optional[CodeType], List[str]]:
     # To make sure that the AST tree loop doesn't run forever
     MAX_TRIES = 2000
 
@@ -268,7 +272,7 @@
     for key in eval_id_values:
         regex_string = "\\b" + key + "\\b"
         eval_string = re.sub(regex_string, str(eval_id_values[key]), eval_string)
         # eval_string = eval_string.replace(key, str(eval_id_values[key]["passed"]))
         # print (eval_string)
 
     # TODO: shall we use and, or and not instead of symbols?
@@ -314,7 +318,7 @@
     # TODO: validate policy_data against schema
 
     with open(input_path) as f:
         if input_path.endswith(".yaml") or input_path.endswith(".yml"):
             input_data = list(yaml.safe_load_all(f))
             if len(input_data) == 1:
                 input_data = input_data[0]

src/tirith/providers/common.py

@@ -4,7 +4,7 @@
 
 
 def create_result_dict(value=None, meta=None, err=None) -> Dict:
     return dict(value=value, meta=meta, err=err)
 
 
 class PydashPathNotFound:
@@ -187,6 +187,10 @@
         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 @@
     '[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 @@
         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 = ""

src/tirith/providers/infracost/handler.py

@@ -10,6 +10,7 @@
         "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,18 +21,20 @@
                         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]]
                         and resource[pointer[operation_type][1]] != "null"
                     ):
                         # 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 @@
         "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 @@
                         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 @@
                         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}]

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)