AWS + Azure: add read-only sanity checks + AWS RI exchange

[crisjermaglasang]

• Adds v1 AWS & Azure deployment sanity checks (dry-run / read-only) with JSON reports + CI workflows.
• Adds AWS Convertible RI exchange: quote-only by default; execute requires explicit ack + spend cap.

[cristim]

@crisjermaglasang thank you for your contribution, really appreciate it.

I asked Claude Code to help me review it and it came up with the below output, which I largely agree with.

Please address the below issues before we can merge this.

PR #3 Review: AWS + Azure read-only sanity checks + AWS RI exchange

+1025 / -1 across 17 files (3 commits)

---

Summary

Adds three features:

1. AWS sanity checks — read-only STS/EC2/RDS probes with JSON report output
2. Azure sanity checks — read-only az CLI probes with JSON report output
3. AWS Convertible RI exchange — quote-only by default; execute requires --ack YES + spend cap

The RI exchange safety model is well-designed (quote-first, explicit ack, spend cap guardrail). The sanity check framework is clean and useful. However, there are several issues that should be addressed before merging.

---

Issues Found

Critical: Compiled binaries committed to the repo

Three compiled binaries are included in the diff:

• sanity (root)
• azure-sanity (root)
• ri-exchange (root)

These should not be checked into git. Add them to .gitignore.

Critical: Sensitive data committed in report files

Three JSON report files contain real credentials/identifiers:

• sanity_report.json — AWS account ID 816582314462, IAM ARN with username, IAM user ID AIDA34IAZJHPPYVORASPA
• azure_sanity_report.json — Azure tenant ID, subscription ID, subscription name, user email
• ri_exchange_quote.json — AWS account ID

These files are test artifacts that should be .gitignore-d, not committed. Even though the info is relatively low-risk, it's bad practice to commit cloud account identifiers to a public repo.

Critical: CI workflows won't trigger from current location

The GitHub Actions workflows are placed at:

ci_cd_sanity_tests/.github/workflows/aws_sanity.yml
ci_cd_sanity_tests/.github/workflows/azure_sanity.yml

GitHub only picks up workflows from .github/workflows/ at the repository root. These workflows will never run from their current location.

Major: Hardcoded AWS account ID in CI workflow

ci_cd_sanity_tests/.github/workflows/aws_sanity.yml line 16:

EXPECTED_ACCOUNT: "816582314462" # change per test account

This should reference a GitHub secret, not be hardcoded.

Major: Azure sanity uses shell-out + fragile string matching

ci_cd_sanity_tests/pkg/sanity/azure/azure.go shells out to az CLI and validates subscription/tenant IDs with strings.Contains:

if !strings.Contains(string(out), fmt.Sprintf(`"id": "%s"`, opts.ExpectedSubID)) {

This is fragile — it depends on exact JSON formatting from the CLI (key spacing, quote style). Since the rest of the codebase uses Azure SDK v2, the sanity checks should use it too, or at minimum use encoding/json to parse the output properly.

Minor: ci_cd_sanity_tests/providers/aws/client.go is unused

The Clients struct and NewClients function are never referenced. The AWS sanity checks (aws.go) create clients inline, and the RI exchange code (ri_exchange.go) has its own loadCfg helper. This file should either be used or removed.

Minor: No unit tests

Despite the PR being called "sanity tests", none of the Go packages include unit tests. The RI exchange logic in particular (spend cap comparison, decimal parsing, quote validation) would benefit from table-driven tests.

Minor: az account show called twice

In azure.go, az account show -o json is called once unconditionally (line 87), then again if ExpectedSubID or ExpectedTenantID is set (line 94). The second call could just reuse the output from the first.

Nit: go.mod change

The sts dependency moves from indirect to direct — this is correct since the new code directly imports sts.

---

What the PR Gets Right

• RI exchange safety model is solid — quote-first flow, explicit --ack YES gating, and --max-payment-due-usd spend cap with big.Rat precision for financial comparison
• Account assertion guard — both sanity and RI exchange verify the active AWS account matches expectations before proceeding
• Clean report framework — report.Report is reusable across cloud providers with consistent JSON output
• Read-only design — sanity checks are genuinely non-mutating (Describe/List calls only)

---

Verdict

Good concept, needs cleanup before merge. The committed binaries, sensitive report files, and misplaced CI workflows are the blockers. The Azure string-matching fragility and missing tests are secondary concerns.

Recommended: Request changes — remove binaries and report files, add them to .gitignore, move CI workflows to .github/workflows/, remove the hardcoded account ID, and delete the unused client.go.

---

🤖 Generated with Claude Code