diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9d72feb..3ec0ae9 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -8,11 +8,11 @@ jobs: runs-on: ubuntu-latest strategy: matrix: - python-version: ["3.10", "3.11", "3.12"] + python-version: ["3.10", "3.11", "3.12", "3.13"] steps: - uses: actions/checkout@v4 - uses: actions/setup-python@v5 with: python-version: ${{ matrix.python-version }} - - run: pip install -e . pytest + - run: pip install -e '.[dev]' - run: pytest diff --git a/README.md b/README.md index e42fe7b..738c261 100644 --- a/README.md +++ b/README.md @@ -13,8 +13,11 @@ pull request. Optionally fails the check when the delta crosses a threshold. ## How it works 1. Parses `terraform show -json ` into create / update / destroy sets. + Input that is not plan JSON — state output, a truncated download — is + **rejected**, never reported as "no changes". 2. Prices cost-relevant resources with a small built-in AWS sheet (override with - your own JSON via `--price-sheet`). + your own JSON via `--price-sheet`). A resource whose cost cannot be + determined is reported as **unknown**, never counted as $0. 3. Renders a Markdown table and upserts one sticky PR comment. ## CLI @@ -53,12 +56,61 @@ steps: terraform plan -out tf.plan terraform show -json tf.plan > plan.json - uses: moveeeax/tf-cost-diff@v0 + id: cost with: - plan: plan.json + plan: plan.json # relative to the workspace threshold: "100" # optional: fail if Δ > $100/mo # price-sheet: prices.json # optional override ``` +### Outputs + +Every figure the tool computes is readable by later steps. They are written +*before* the threshold gate is applied, so they are still available on the run +where the gate fails — use `if: always()` to read them there. + +| Output | Example | Meaning | +| --- | --- | --- | +| `total-delta` | `97.96` | Monthly delta in dollars (negative when the plan saves money). | +| `created` / `updated` / `destroyed` | `2` / `1` / `1` | Cost-relevant resource counts. | +| `unpriced` | `0` | Resources excluded from the total because their cost is unknown. | +| `complete` | `true` | `false` when `unpriced > 0`, i.e. the total is a partial figure. | +| `threshold-exceeded` | `false` | Whether the delta crossed `threshold`. | +| `comment-url` | `https://github.com/...` | The sticky comment, empty if none was posted. | + +```yaml + - if: always() && steps.cost.outputs.complete == 'false' + run: echo "::warning::${{ steps.cost.outputs.unpriced }} resources could not be priced" +``` + +### Exit codes + +| Code | Meaning | +| --- | --- | +| `0` | Success. | +| `1` | The delta exceeded `--threshold`. | +| `2` | Bad input: not a Terraform plan, unreadable file, or an invalid price sheet. | + +Posting the comment is best effort: a pull request from a fork gets a read-only +`GITHUB_TOKEN`, so the comment is skipped with a warning rather than failing the +run. The threshold gate still applies. + +## Unknown costs + +The built-in sheet covers a handful of resource types and a handful of instance +sizes within them. Anything outside that is treated as **unknown**, not free: + +- A resource type with no price model (IAM roles, security groups) is excluded + from the estimate silently — the comment carries a standing caveat. +- A *modelled* type the sheet cannot price for this configuration — an + unrecognised `instance_type`, or a size that is not known until apply — is + listed in a separate "Cost unknown" table, excluded from the total, and the + headline is marked `(partial)`. + +This matters for the threshold gate: guessing a rate for an unrecognised +`m5.24xlarge` would report ~$36/mo for a resource that really costs over +$3,000/mo, and it would sail straight through `threshold: 100`. + ## Price sheet override `--price-sheet prices.json` where the file maps a Terraform resource type to a @@ -71,12 +123,13 @@ flat monthly price: } ``` -Anything not in the built-in sheet or your override is treated as $0 and skipped. +Values must be finite, non-negative numbers; anything else is rejected with exit +code `2` rather than silently producing a nonsense total. ## Development ```bash -pip install -e . pytest +pip install -e '.[dev]' pytest ``` diff --git a/action.yml b/action.yml index eb577c1..80a6001 100644 --- a/action.yml +++ b/action.yml @@ -5,29 +5,70 @@ branding: color: "green" inputs: plan: - description: "Path to `terraform show -json` output (a JSON file)." + description: "Path to `terraform show -json` output (a JSON file), relative to the workspace." required: true threshold: description: "Fail the check when the monthly delta exceeds this many dollars." required: false price-sheet: - description: "Optional path to a JSON price-sheet override." + description: "Optional path to a JSON price-sheet override, relative to the workspace." required: false github-token: description: "Token used to post the PR comment." required: false default: ${{ github.token }} +outputs: + total-delta: + description: "Estimated monthly cost delta in dollars, e.g. `-12.40`." + value: ${{ steps.cost.outputs.total-delta }} + created: + description: "Number of cost-relevant resources being created." + value: ${{ steps.cost.outputs.created }} + updated: + description: "Number of cost-relevant resources being changed or replaced." + value: ${{ steps.cost.outputs.updated }} + destroyed: + description: "Number of cost-relevant resources being destroyed." + value: ${{ steps.cost.outputs.destroyed }} + unpriced: + description: "Number of resources excluded from the total because their cost is unknown." + value: ${{ steps.cost.outputs.unpriced }} + complete: + description: "`true` when every cost-relevant resource could be priced." + value: ${{ steps.cost.outputs.complete }} + threshold-exceeded: + description: "`true` when the delta exceeded `threshold`." + value: ${{ steps.cost.outputs.threshold-exceeded }} + comment-url: + description: "URL of the sticky pull request comment, empty if none was posted." + value: ${{ steps.cost.outputs.comment-url }} runs: using: "composite" steps: - - shell: bash + - id: cost + shell: bash + # Inputs are passed through the environment rather than interpolated into + # the script, so a value containing shell metacharacters cannot execute. + # PYTHONPATH (not working-directory) makes the package importable, so that + # `plan` stays relative to the caller's workspace as documented. env: GITHUB_TOKEN: ${{ inputs.github-token }} GITHUB_REPOSITORY: ${{ github.repository }} + PYTHONPATH: ${{ github.action_path }} + TFCD_PLAN: ${{ inputs.plan }} + TFCD_PRICE_SHEET: ${{ inputs.price-sheet }} + TFCD_THRESHOLD: ${{ inputs.threshold }} + TFCD_PR: ${{ github.event.pull_request.number }} run: | - python3 -m tf_cost_diff \ - --plan "${{ inputs.plan }}" \ - ${{ inputs.price-sheet && format('--price-sheet {0}', inputs.price-sheet) || '' }} \ - ${{ inputs.threshold && format('--threshold {0}', inputs.threshold) || '' }} \ - ${{ github.event.pull_request.number && format('--pr {0}', github.event.pull_request.number) || '' }} - working-directory: ${{ github.action_path }} + set -euo pipefail + args=(--plan "$TFCD_PLAN" --github-output "$GITHUB_OUTPUT") + if [ -n "${TFCD_PRICE_SHEET:-}" ]; then + args+=(--price-sheet "$TFCD_PRICE_SHEET") + fi + if [ -n "${TFCD_THRESHOLD:-}" ]; then + args+=(--threshold "$TFCD_THRESHOLD") + fi + if [ -n "${TFCD_PR:-}" ]; then + args+=(--pr "$TFCD_PR") + fi + python3 -m tf_cost_diff "${args[@]}" diff --git a/pyproject.toml b/pyproject.toml index 52fccdd..ce63f9c 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta" [project] name = "tf-cost-diff" -version = "0.1.0" +version = "0.2.0" description = "Estimate the monthly cost delta of a Terraform plan and post it to a pull request." readme = "README.md" requires-python = ">=3.10" @@ -12,6 +12,9 @@ license = { text = "MIT" } authors = [{ name = "Michael Tarassov", email = "michael@tarassov.me" }] keywords = ["terraform", "finops", "cost", "github-actions", "aws"] +[project.optional-dependencies] +dev = ["pytest>=7", "pyyaml>=6"] + [project.urls] Homepage = "https://github.com/moveeeax/tf-cost-diff" diff --git a/tests/test_action.py b/tests/test_action.py new file mode 100644 index 0000000..9df8056 --- /dev/null +++ b/tests/test_action.py @@ -0,0 +1,65 @@ +"""Keep action.yml honest about what the CLI actually produces. + +The failure mode this guards against is drift: the tool computes numbers, the +Action declares no `outputs:` for them, and the calling workflow silently has +nothing to read. +""" +import pathlib + +import pytest + +yaml = pytest.importorskip("yaml") + +ACTION = pathlib.Path(__file__).resolve().parent.parent / "action.yml" + + +@pytest.fixture(scope="module") +def action(): + return yaml.safe_load(ACTION.read_text(encoding="utf-8")) + + +def _cli_output_names(tmp_path): + from tf_cost_diff.__main__ import write_github_output + from tf_cost_diff.plan import PlanSummary + + out = tmp_path / "gh-output" + write_github_output(str(out), PlanSummary(resources=[]), False, "") + return {line.split("=", 1)[0] for line in out.read_text().splitlines()} + + +def test_action_declares_every_output_the_cli_writes(action, tmp_path): + declared = set(action.get("outputs") or {}) + assert declared == _cli_output_names(tmp_path) + + +def test_every_declared_output_is_wired_to_the_step(action): + step_ids = {s.get("id") for s in action["runs"]["steps"]} + for name, spec in action["outputs"].items(): + assert spec.get("description"), f"output {name} has no description" + value = spec["value"] + step = value.split("steps.", 1)[1].split(".", 1)[0] + assert step in step_ids, f"output {name} references unknown step {step}" + assert value.endswith(f"outputs.{name} }}}}"), f"output {name} is mis-wired" + + +def test_inputs_are_passed_through_env_not_interpolated_into_the_script(action): + """`run:` must not interpolate inputs, or a crafted value runs as shell.""" + for step in action["runs"]["steps"]: + assert "${{" not in step.get("run", ""), "inputs must reach the script via env:" + + +def test_plan_path_is_resolved_against_the_workspace(action): + # working-directory: ${{ github.action_path }} would make the documented + # relative `plan: plan.json` resolve inside the action checkout instead. + for step in action["runs"]["steps"]: + assert "working-directory" not in step + assert "PYTHONPATH" in action["runs"]["steps"][0]["env"] + + +def test_declared_inputs_are_all_consumed(action): + script = "".join(s.get("run", "") for s in action["runs"]["steps"]) + env = {k: str(v) for s in action["runs"]["steps"] for k, v in (s.get("env") or {}).items()} + for name in action["inputs"]: + referenced = any(f"inputs.{name}" in value for value in env.values()) + assert referenced, f"input {name} is declared but never used" + assert "--plan" in script diff --git a/tests/test_cli.py b/tests/test_cli.py new file mode 100644 index 0000000..d9ef241 --- /dev/null +++ b/tests/test_cli.py @@ -0,0 +1,162 @@ +"""End-to-end CLI behaviour: exit codes, the threshold gate, Action outputs.""" +import json + +import pytest + +from tf_cost_diff import CostDiffError +from tf_cost_diff.__main__ import EXIT_ERROR, EXIT_OK, EXIT_OVER_THRESHOLD, main +from tf_cost_diff.pricing import PriceSheet + +PLAN = { + "format_version": "1.2", + "resource_changes": [ + { + "address": "aws_instance.web", + "type": "aws_instance", + "change": { + "actions": ["create"], + "before": None, + "after": {"instance_type": "m5.large"}, + }, + } + ], +} + + +def _write(tmp_path, name, payload): + path = tmp_path / name + path.write_text(payload if isinstance(payload, str) else json.dumps(payload)) + return str(path) + + +def test_under_threshold_exits_zero(tmp_path, capsys): + plan = _write(tmp_path, "plan.json", PLAN) + assert main(["--plan", plan, "--threshold", "1000"]) == EXIT_OK + + +def test_over_threshold_exits_one(tmp_path, capsys): + plan = _write(tmp_path, "plan.json", PLAN) + assert main(["--plan", plan, "--threshold", "10"]) == EXIT_OVER_THRESHOLD + assert "exceeds threshold" in capsys.readouterr().err + + +def test_state_output_fails_loudly_instead_of_reporting_no_changes(tmp_path, capsys): + state = _write( + tmp_path, + "state.json", + {"format_version": "1.0", "values": {"root_module": {"resources": []}}}, + ) + assert main(["--plan", state, "--threshold", "10"]) == EXIT_ERROR + err = capsys.readouterr().err + assert "::error::" in err and "state" in err + + +def test_truncated_plan_fails_with_a_message_not_a_traceback(tmp_path, capsys): + plan = _write(tmp_path, "plan.json", '{"resource_changes":[{"addre') + assert main(["--plan", plan]) == EXIT_ERROR + assert "not valid JSON" in capsys.readouterr().err + + +def test_missing_plan_file_is_reported_cleanly(tmp_path, capsys): + assert main(["--plan", str(tmp_path / "nope.json")]) == EXIT_ERROR + assert "not found" in capsys.readouterr().err + + +def test_nan_price_sheet_is_rejected_rather_than_disabling_the_gate(tmp_path, capsys): + plan = _write(tmp_path, "plan.json", PLAN) + sheet = _write(tmp_path, "prices.json", '{"aws_instance": NaN}') + # Previously this produced "$nan" and quietly returned 0 from the gate. + assert main(["--plan", plan, "--price-sheet", sheet, "--threshold", "1"]) == EXIT_ERROR + assert "non-finite" in capsys.readouterr().err + + +def test_nan_threshold_is_rejected(tmp_path): + plan = _write(tmp_path, "plan.json", PLAN) + with pytest.raises(SystemExit): + main(["--plan", plan, "--threshold", "nan"]) + + +def test_negative_price_in_sheet_is_rejected(tmp_path): + sheet = _write(tmp_path, "prices.json", {"aws_instance": -5}) + with pytest.raises(CostDiffError, match="negative"): + PriceSheet.load(sheet) + + +def test_non_numeric_price_is_rejected(tmp_path): + sheet = _write(tmp_path, "prices.json", {"aws_instance": "cheap"}) + with pytest.raises(CostDiffError, match="finite number"): + PriceSheet.load(sheet) + + +def test_github_output_is_written(tmp_path): + plan = _write(tmp_path, "plan.json", PLAN) + out = tmp_path / "gh-output" + assert main(["--plan", plan, "--threshold", "10", "--github-output", str(out)]) == ( + EXIT_OVER_THRESHOLD + ) + values = dict(line.split("=", 1) for line in out.read_text().splitlines()) + assert values == { + "total-delta": "70.08", + "created": "1", + "updated": "0", + "destroyed": "0", + "unpriced": "0", + "complete": "true", + # Written even though the gate failed on this same run. + "threshold-exceeded": "true", + "comment-url": "", + } + + +def test_github_output_reports_unpriced_resources(tmp_path): + plan = _write( + tmp_path, + "plan.json", + { + "resource_changes": [ + { + "address": "aws_instance.big", + "type": "aws_instance", + "change": { + "actions": ["create"], + "after": {"instance_type": "m5.24xlarge"}, + }, + } + ] + }, + ) + out = tmp_path / "gh-output" + assert main(["--plan", plan, "--github-output", str(out)]) == EXIT_OK + values = dict(line.split("=", 1) for line in out.read_text().splitlines()) + assert values["unpriced"] == "1" + assert values["complete"] == "false" + + +def test_pr_without_token_warns_and_does_not_crash(tmp_path, capsys, monkeypatch): + monkeypatch.delenv("GITHUB_TOKEN", raising=False) + monkeypatch.delenv("GITHUB_REPOSITORY", raising=False) + plan = _write(tmp_path, "plan.json", PLAN) + assert main(["--plan", plan, "--pr", "7"]) == EXIT_OK + assert "::warning::" in capsys.readouterr().err + + +def test_failure_to_comment_does_not_mask_the_threshold_verdict( + tmp_path, capsys, monkeypatch +): + def boom(*_args, **_kwargs): + raise CostDiffError("HTTP 403") + + monkeypatch.setattr("tf_cost_diff.__main__.upsert_comment", boom) + plan = _write(tmp_path, "plan.json", PLAN) + code = main( + ["--plan", plan, "--pr", "7", "--repo", "o/n", "--token", "t", "--threshold", "10"] + ) + assert code == EXIT_OVER_THRESHOLD + err = capsys.readouterr().err + assert "could not post the PR comment" in err + assert "exceeds threshold" in err + + +def test_bundled_example_plan_parses(capsys): + assert main(["--plan", "examples/plan.json"]) == EXIT_OK + assert "Terraform monthly cost estimate" in capsys.readouterr().out diff --git a/tests/test_plan.py b/tests/test_plan.py index 23bd805..6c22d27 100644 --- a/tests/test_plan.py +++ b/tests/test_plan.py @@ -1,4 +1,7 @@ -from tf_cost_diff.plan import parse_plan +import pytest + +from tf_cost_diff import CostDiffError +from tf_cost_diff.plan import parse_plan, validate_plan_document from tf_cost_diff.pricing import PriceSheet from tf_cost_diff.report import MARKER, render @@ -62,6 +65,7 @@ def test_zero_cost_resources_are_skipped(): plan = _plan(_change("aws_iam_role.r", "aws_iam_role", ["create"], after={})) summary = parse_plan(plan, PriceSheet()) assert summary.resources == [] + assert summary.unpriced == [] def test_price_sheet_override_wins(): @@ -82,3 +86,124 @@ def test_render_contains_marker_and_total(): assert MARKER in body assert "+$3.60/mo" in body assert "`aws_eip.nat`" in body + + +def test_no_op_and_read_actions_are_ignored(): + plan = _plan( + _change("aws_eip.a", "aws_eip", ["no-op"], before={}, after={}), + _change("aws_eip.b", "aws_eip", ["read"], after={}), + ) + summary = parse_plan(plan, PriceSheet()) + assert summary.resources == [] + assert summary.created == 0 + + +def test_rows_sum_to_the_headline_total(): + plan = _plan( + _change("aws_eip.a", "aws_eip", ["create"], after={}), + _change("aws_eip.b", "aws_eip", ["create"], after={}), + _change("aws_ebs_volume.c", "aws_ebs_volume", ["create"], after={"size": 33}), + ) + summary = parse_plan(plan, PriceSheet()) + assert summary.total_delta == round(sum(r.delta for r in summary.resources), 2) + assert summary.total_delta == 9.84 + + +# --- input validation: not every valid JSON document is a Terraform plan ----- + + +def test_state_output_is_rejected_not_reported_as_no_changes(): + state = { + "format_version": "1.0", + "terraform_version": "1.9.0", + "values": {"root_module": {"resources": []}}, + } + with pytest.raises(CostDiffError, match="state"): + validate_plan_document(state, "plan.json") + + +def test_arbitrary_json_is_rejected(): + with pytest.raises(CostDiffError, match="not a Terraform plan"): + validate_plan_document({"hello": "world"}, "plan.json") + + +def test_non_object_json_is_rejected(): + with pytest.raises(CostDiffError, match="JSON object"): + validate_plan_document([1, 2, 3], "plan.json") + + +def test_empty_plan_with_no_changes_is_accepted(): + doc = {"format_version": "1.2", "planned_values": {"root_module": {}}} + assert validate_plan_document(doc, "plan.json") is doc + assert parse_plan(doc, PriceSheet()).resources == [] + + +def test_resource_changes_must_be_a_list(): + with pytest.raises(CostDiffError, match="must be a list"): + validate_plan_document({"resource_changes": {}}, "plan.json") + + +def test_malformed_resource_change_raises_rather_than_costing_zero(): + with pytest.raises(CostDiffError, match="change"): + parse_plan({"resource_changes": [{"address": "a", "type": "aws_eip"}]}, PriceSheet()) + + +# --- unknown prices are reported, never silently turned into a number ------- + + +def test_unknown_instance_type_is_unpriced_not_guessed(): + plan = _plan( + _change( + "aws_instance.big", + "aws_instance", + ["create"], + after={"instance_type": "m5.24xlarge"}, + ) + ) + summary = parse_plan(plan, PriceSheet()) + assert summary.resources == [] + assert [u.address for u in summary.unpriced] == ["aws_instance.big"] + assert summary.total_delta == 0.0 + assert summary.is_complete is False + assert summary.created == 1 + + +def test_attribute_unknown_until_apply_is_unpriced_not_zero(): + plan = _plan( + _change("aws_ebs_volume.d", "aws_ebs_volume", ["create"], after={"size": None}), + _change("aws_instance.w", "aws_instance", ["create"], after={"instance_type": None}), + ) + summary = parse_plan(plan, PriceSheet()) + assert len(summary.unpriced) == 2 + assert summary.total_delta == 0.0 + + +def test_unpriced_resources_are_flagged_in_the_report(): + plan = _plan( + _change( + "aws_db_instance.main", + "aws_db_instance", + ["create"], + after={"instance_class": "db.r5.24xlarge"}, + ) + ) + body = render(parse_plan(plan, PriceSheet())) + assert "could not be priced" in body + assert "(partial)" in body + assert "`aws_db_instance.main`" in body + + +def test_replace_with_unknown_after_side_is_unpriced(): + plan = _plan( + _change( + "aws_instance.web", + "aws_instance", + ["create", "delete"], + before={"instance_type": "t3.small"}, + after={"instance_type": "x9.enormous"}, + ) + ) + summary = parse_plan(plan, PriceSheet()) + assert summary.resources == [] + assert len(summary.unpriced) == 1 + assert summary.updated == 1 diff --git a/tests/test_report.py b/tests/test_report.py new file mode 100644 index 0000000..16a2a9a --- /dev/null +++ b/tests/test_report.py @@ -0,0 +1,103 @@ +"""The rendered comment must survive hostile resource addresses and big plans.""" +import re + +from tf_cost_diff.plan import PlanSummary, ResourceDelta, UnpricedResource +from tf_cost_diff.report import COMMENT_LIMIT, MARKER, render + + +def _delta(address, after=10.0, action="create"): + return ResourceDelta( + address=address, + resource_type="aws_eip", + action=action, + before_monthly=0.0, + after_monthly=after, + ) + + +def _outside_code_spans(body): + """The comment with every `code span` removed, i.e. what Markdown parses.""" + return re.sub(r"`[^`\n]*`", "", body.replace(MARKER, "")) + + +def test_backtick_in_for_each_key_cannot_close_the_code_span(): + # `for_each` keys are arbitrary strings. A backtick that closes the span + # lets the rest be parsed as raw HTML, and a bare `" +# GitHub rejects an issue-comment body longer than this with HTTP 422, which +# would mean posting nothing at all on exactly the large plans people most want +# reviewed. Stay under it and say what was dropped. +COMMENT_LIMIT = 65536 + _ARROW = {"create": "🟢 create", "update": "🟡 update", "delete": "🔴 delete"} +_CONTROL = re.compile(r"[\x00-\x1f\x7f]") +_CELL_MAX = 200 + + +def _code(text: str) -> str: + """Render untrusted text as a Markdown code span that cannot break out. + + Resource addresses embed ``for_each`` keys, which are arbitrary attacker- or + typo-supplied strings. A backtick would close the code span and let the rest + of the key be interpreted as Markdown or raw HTML -- ``