fix(migrate): enforce correct architecture diagram topology in reports#130
Closed
herosjourney wants to merge 5 commits into
Closed
fix(migrate): enforce correct architecture diagram topology in reports#130herosjourney wants to merge 5 commits into
herosjourney wants to merge 5 commits into
Conversation
Codify Section 2b rules so diagrams match generated Terraform: ALB routes only to compute, public S3/CDN is a separate user path, and backends fan out from Fargate with NAT/private subnet labeled. Update the reference fixture diagram and README to match. Co-authored-by: Cursor <cursoragent@cursor.com>
Diagram topology rules must show TLS termination on 443; HTTP-only ALB forwarding is a Generate defect per generate-artifacts-infra.md, not something to reflect in customer-facing architecture diagrams. Co-authored-by: Cursor <cursoragent@cursor.com>
Add a static policy gate that fails Generate when internet-facing ALBs lack an HTTPS :443 listener or forward plain HTTP on :80. Wire Stage F into terraform-validation.md and Step 5a in generate-artifacts-infra.md, include the required listener HCL template, and add fixtures plus tests. Co-authored-by: Cursor <cursoragent@cursor.com>
herosjourney
force-pushed
the
fix/architecture-diagram-topology
branch
from
July 14, 2026 23:08
5517843 to
0f855fe
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Checkov found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
The security-scanners workflow runs `checkov -d .` repo-wide. This PR adds the repo's first .tf files under fixtures/terraform-policy/, two of which are intentionally non-compliant (e.g. an ALB forwarding plaintext HTTP) to exercise the failure paths of scripts/validate-terraform-policy.py. Checkov flagged 28 issues across them and failed the gate. Add a repo-root .checkov.yaml with skip-path for that fixture dir (auto-discovered by `checkov -d .`, no workflow change). Sanitizing the fixtures would break test_validate_terraform_policy.py, which asserts bad-http-forward returns POLICY_FAIL and internal-alb-only keeps HTTP. Also run dprint fmt on generate-artifacts-infra.md (table-column realignment after the internet-facing-ALB TLS rule text was added) to satisfy fmt:check.
icarthick
added a commit
to icarthick/startups
that referenced
this pull request
Jul 15, 2026
Introduces a shared, read-only tf-best-practices skill under migration-to-aws that owns generated-Terraform security posture + a deterministic policy gate, and rewires gcp-to-aws's Generate phase to consume it. Motivation: the ALB-HTTPS validation PR awslabs#130 raised is a cross-cutting concern, not a gcp-specific one — both gcp-to-aws and (later) heroku-to-aws generate Terraform and need the same posture enforcement. Rather than land a one-rule bespoke validator inside gcp, this factors it into a reusable unit. tf-best-practices (verdict producer, NOT a mutator): - SKILL.md — gate contract; two-part (authoring posture + read-only policy verdict); explicit boundary: never edits .tf, never touches .phase-status.json, never decides phase completion (all caller-owned). - scripts/validate-terraform-policy.py — read-only ALB-HTTPS policy checker with a --json structured verdict. Fixes the original's default_action nested-block regex bug via brace-depth matching (a valid block-form forward{} listener no longer false-fails). - scripts/test_validate_terraform_policy.py — 8 tests incl. the nested-block regression and JSON-verdict coverage. - references/security-posture-rules.md — the ALB TLS posture (thin v1; extensible). - references/terraform-validation.md — the fmt→init→validate→policy→fix-retry protocol, generalized: caller-neutral (no .phase-status.json/generate.md coupling), v2 report schema adds policy_status so a policy failure is recorded independently and can't be masked by passed_degraded_offline. - fixtures/terraform-policy/ — the intentionally-(non)compliant test fixtures. gcp-to-aws rewiring: - generate-artifacts-infra.md: new Step 6 runs the shared protocol + policy gate; Phase Completion now requires policy_status: POLICY_OK. - generate.md: route gate + status reporting surface policy_status. - removed the orphaned gcp-local references/shared/terraform-validation.md (nothing referenced it; superseded by the shared copy). .checkov.yaml: skip-path for the fixtures dir (repo-wide checkov -d . would otherwise flag the intentionally-bad fixtures; sanitizing them would break the tests). Scope: gcp-to-aws only. heroku-to-aws is intentionally untouched; the contract is designed for it to adopt later. Gates: 8 python tests pass; checkov 0 failures; frontmatter validator OK; dprint + markdownlint clean.
icarthick
added a commit
that referenced
this pull request
Jul 15, 2026
…e-verdict fixes (#142) * feat(migrate): extract tf-best-practices skill for generated-IaC policy Introduces a shared, read-only tf-best-practices skill under migration-to-aws that owns generated-Terraform security posture + a deterministic policy gate, and rewires gcp-to-aws's Generate phase to consume it. Motivation: the ALB-HTTPS validation PR #130 raised is a cross-cutting concern, not a gcp-specific one — both gcp-to-aws and (later) heroku-to-aws generate Terraform and need the same posture enforcement. Rather than land a one-rule bespoke validator inside gcp, this factors it into a reusable unit. tf-best-practices (verdict producer, NOT a mutator): - SKILL.md — gate contract; two-part (authoring posture + read-only policy verdict); explicit boundary: never edits .tf, never touches .phase-status.json, never decides phase completion (all caller-owned). - scripts/validate-terraform-policy.py — read-only ALB-HTTPS policy checker with a --json structured verdict. Fixes the original's default_action nested-block regex bug via brace-depth matching (a valid block-form forward{} listener no longer false-fails). - scripts/test_validate_terraform_policy.py — 8 tests incl. the nested-block regression and JSON-verdict coverage. - references/security-posture-rules.md — the ALB TLS posture (thin v1; extensible). - references/terraform-validation.md — the fmt→init→validate→policy→fix-retry protocol, generalized: caller-neutral (no .phase-status.json/generate.md coupling), v2 report schema adds policy_status so a policy failure is recorded independently and can't be masked by passed_degraded_offline. - fixtures/terraform-policy/ — the intentionally-(non)compliant test fixtures. gcp-to-aws rewiring: - generate-artifacts-infra.md: new Step 6 runs the shared protocol + policy gate; Phase Completion now requires policy_status: POLICY_OK. - generate.md: route gate + status reporting surface policy_status. - removed the orphaned gcp-local references/shared/terraform-validation.md (nothing referenced it; superseded by the shared copy). .checkov.yaml: skip-path for the fixtures dir (repo-wide checkov -d . would otherwise flag the intentionally-bad fixtures; sanitizing them would break the tests). Scope: gcp-to-aws only. heroku-to-aws is intentionally untouched; the contract is designed for it to adopt later. Gates: 8 python tests pass; checkov 0 failures; frontmatter validator OK; dprint + markdownlint clean. * feat(tf-best-practices): expand policy gate from 2 to 8 rules; fix 2 false verdicts Grows the read-only policy gate beyond internet-facing ALB TLS to cover the cross-cutting posture the consuming migration skill previously only self-checked. New rules (each fail-open on ambiguity — fire only on unambiguous in-block literal evidence, since POLICY_FAIL is a hard completion gate for the caller): - rds_not_public: aws_db_instance/aws_rds_cluster must not set publicly_accessible=true - rds_encryption_at_rest: RDS must set storage_encrypted=true (defaults to unencrypted) - elasticache_encryption_at_rest: replication groups must set at_rest_encryption_enabled=true - db_sg_no_public_ingress: inline SG ingress on 5432/3306 must not allow 0.0.0.0/0 - sg_no_public_admin_ingress: inline ingress must not open a curated never-public admin/datastore port (SSH/RDP/Redis/Memcached/Mongo/Elasticsearch/Kibana) to 0.0.0.0/0 — web and app/game ports intentionally not flagged - no_wildcard_iam: literal IAM policy Allow statements must not use Action/Resource "*" Bug fixes surfaced by running the gate against real generated migrations: - no_wildcard_iam missed list-form wildcards (Resource = ["*"]) — only bare string "*" was caught. Now matches both string and single-element list forms. - alb_https_listener false-fired on internet-facing Network/Gateway load balancers (L4/L3 legitimately have no HTTPS:443 listener). _has_internet_facing_alb now skips load_balancer_type = "network"/"gateway". Adds 13 fixtures (bad + good pairs, incl. fail-open/no-false-positive cases) and 15 tests (23 total, all passing). Verified no false positives against 4 real generated stacks (Cloud Functions, fintech, gaming, static-site). Docs updated: SKILL.md rules list + scope, security-posture-rules.md authoring sections.
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
generate-artifacts-report.mdwith explicit topology rules so migration report ASCII diagrams match generated Terraform traffic flows.migration-report-reference.html) to demonstrate the correct pattern: ALB → Fargate only, separate public S3 path for image GETs, backends branching from compute with NAT/private subnet labeled.fixtures/README.md.Motivation: The SF Beach migration run produced a diagram that incorrectly showed pipes from the ALB to RDS/S3/Bedrock, labeled HTTPS when the ALB listener is HTTP, and drew CloudFront OAC as a Fargate backend even though public images are served via direct S3 read in the generated baseline.
Topology rules (high level)
Relationship to other PRs
This branch includes the commits from #78 (comprehensive migration report + validator) because Section 2b and the reference fixture live there. Recommended merge order: land #78 first, then rebase this PR onto
mainfor a small diff — or review as one stack.Also independent of #129 (Q27 / Activate credits).
Test plan
python3 scripts/validate-migration-report.py fixtures/migration-report-reference.html …→REPORT_OKpytest tests/test_validate_migration_report.py→ 32 passedMade with Cursor