Skip to content

fix(gcp-to-aws): stop generated scripts from prescribing a BigQuery data lake on S3 - #202

Open
leon1418 wants to merge 7 commits into
awslabs:mainfrom
leon1418:fix/gcp-bigquery-deferral-gate-scripts
Open

fix(gcp-to-aws): stop generated scripts from prescribing a BigQuery data lake on S3#202
leon1418 wants to merge 7 commits into
awslabs:mainfrom
leon1418:fix/gcp-bigquery-deferral-gate-scripts

Conversation

@leon1418

@leon1418 leon1418 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Problem

The design phase has a mandatory BigQuery specialist gate (references/phases/design/design-infra.md:37-40): for any gcp_type starting with google_bigquery_, set aws_service to Deferred — specialist engagement with no_automated_aws_target: true, and explicitly do not recommend Athena, Redshift, Glue, EMR, Lake Formation, or a prescribed "data lake on S3".

The generate phase then contradicted it. generate-artifacts-scripts.md emitted:

# BigQuery → S3 data export
# TODO: Configure BigQuery dataset and S3 bucket
# bq extract --destination_format=PARQUET 'dataset.table' 'gs://bucket/export/'
# aws s3 sync gs://bucket/export/ s3://target-bucket/import/

A customer told "no automated AWS target — engage the account team" was handed a Parquet-export-into-S3 plan. That is not near the edge of the three rules; it is precisely the "data lake on S3" architecture rule 1 forbids.

Mitigation, stated plainly: the block shipped commented out behind # TODO: Configure. Nothing executed silently. It misguides; it does not run. Hence High-mitigated, not Critical.

A second leak, found while fixing this one: the block was gated on has_databases, which is also true for google_sql_, google_firestore_, google_bigtable_, and google_redis_. So a Cloud SQL-only project with zero BigQuery resources also received a "BigQuery → S3" export plan — prescribing a forbidden analytics architecture to a customer who did not even have the source system.

Solution

Both halves of the finding's suggested options were needed; neither alone is sufficient.

  • Re-gating on a has_bigquery flag alone fails the primary requirement — the same forbidden block would still fire for exactly the customers the gate protects.
  • Dropping the block alone leaves the second leak — emitting the deferral notice on has_databases would tell Cloud SQL-only customers to engage a data-analytics partner about BigQuery they don't have.

So: the bq extract block is replaced by a specialist-engagement deferral notice, gated on a new has_bigquery flag.

has_bigquery is not a new invention — references/phases/discover/discover-preview.md:244 already defines it with the same semantics, and the dual predicate (google_bigquery_* or aws_service == "Deferred — specialist engagement") matches the established idiom in three sibling files (generate-artifacts-docs.md:112, generate-infra.md:217, generate-artifacts-infra.md:58). Verified that Deferred — specialist engagement is set only by the BigQuery gate anywhere in this skill, so the second clause cannot over-trigger on a non-BigQuery deferral.

The notice uses live echo lines, not comments-only, so it actually reaches the customer running 02-migrate-data.sh in both dry-run and --execute mode. Its wording is reused from the existing story (design-infra.md:40, generate-artifacts-docs.md:114, clarify.md:441) rather than invented, so the customer isn't handed a competing remediation narrative.

has_databases semantics left unchanged, deliberately. has_bigquery ⇒ has_databases ⇒ has_data_migration ⇒ script 02 exists. Removing google_bigquery_ would make a BigQuery-only design skip script 02 entirely — the exact silence this fix exists to prevent. has_databases also only decides whether a section is emitted; it never prescribes a target. The prescription lived in the block body, which is what changed.

Self-check rule added (rule 6) asserting no script prescribes an AWS analytics or warehouse target for BigQuery.

Swept for sibling leaks

Grepped the whole skill for bq/bq extract/BigQuery plus every forbidden target. This was the only defect. Everything else is a prohibition, a detection predicate, or unrelated:

  • Redshift at generate-artifacts-scripts.md:31 — legitimate: matches an aws_service a specialist may have chosen and recorded. Blanket-removing it would break has_databases. Left alone.
  • pricing-cache.md:333/342 (Redshift Serverless, Athena) — inert rate cards; confirmed nothing references them, and estimate-infra.md:148 forbids applying them as a projected analytics stack.
  • design-refs/storage.md:111 "data lakes" — generic bucket prose, no BigQuery context.
  • generate-artifacts-infra.md:289/291 "source-glue" — glue code, false positive.

Verification

  • pytest migrate/plugins/migration-to-aws/tests/37 passed.
  • Post-change grep: bq extract has exactly one hit repo-wide — the new prohibition line itself. No has_bigquery-gated block names a forbidden target except to forbid it.
  • Flag trace: has_bigquery defined once (:33), referenced once (:206); has_databases retains 4 consumers (:40, :84, :227, :368) with the BigQuery consumer removed — nothing orphaned in either direction.
  • Markdown fences balanced before and after.
  • mise run build → exit 0 (checkov 180/0, gitleaks clean, grype clean, dprint + markdownlint clean).
  • One file changed, +22/−5.

Recommended follow-ups (not in this PR)

  1. A storage-only project generates an empty 02-migrate-data.shhas_data_migration = has_databases OR has_storage, but every subsection in script 02 is gated on has_databases, so there is no GCS→S3 step and no shebang. Pre-existing, unrelated to BigQuery.
  2. Script 02's #!/usr/bin/env bash + set -euo pipefail preamble sits inside the Cloud SQL subsection, not at script scope — so any future narrowing of has_databases yields a header-less script. Worth hoisting.
  3. Coarse has_databases gating leaks flavor across engines (a BigQuery-only project still gets Cloud SQL pg_dump TODOs). Not a gate violation, but the same one-flag-many-engines imprecision that produced this defect; per-engine flags would fix the class.

Type of Change

  • Bug fix
  • Documentation update

Team Folder

  • migrate/

Checklist

  • I have read the CONTRIBUTING.md guidelines
  • My changes do not include hardcoded secrets, credentials, or internal-only content
  • I have run mise run build locally and it passes
  • I have updated documentation if needed
  • My changes are scoped to my team's folder only

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

…ata lake on S3

The design phase's BigQuery specialist gate (design-infra.md Pass 2 step 0) sets
`aws_service` to `Deferred — specialist engagement` with
`no_automated_aws_target: true` and explicitly forbids naming Athena, Redshift,
Glue, EMR, Lake Formation, or a prescribed "data lake on S3" for any
`google_bigquery_*` resource. The generate phase then contradicted that: a
"BigQuery to S3" block emitting `bq extract --destination_format=PARQUET` plus
`aws s3 sync` into a target bucket was gated on `has_databases` — the very
predicate the deferral keys on — so a customer told "no automated AWS target"
still received the forbidden architecture. It shipped commented out, so nothing
executed silently; the defect misguided rather than ran.

- Replace the BigQuery export block with a deferral notice that mirrors the
  `specialist_engagement` wording already used by design-infra.md and
  generate-artifacts-docs.md (engage AWS account team and/or a data analytics
  migration partner), so BigQuery users get a next step instead of silence.
- Add a `has_bigquery` detection flag (same name/semantics as the existing flag
  in discover-preview.md) and gate the notice on it. The old block fired for any
  database resource, so Cloud SQL-only projects were handed a BigQuery data lake
  plan with no BigQuery present.
- Add self-check rule 6 asserting no script prescribes an AWS analytics or
  warehouse target for BigQuery.

`has_databases` semantics are unchanged, so `has_data_migration`, the Cloud SQL
and Firestore blocks, and the 05 RDS validation section behave exactly as before.
@leon1418
leon1418 requested a review from a team as a code owner August 5, 2026 00:07

@leon1418 leon1418 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[🤖 AI review 🤖]

Reviewed the complete change and the related discovery, design, and artifact-generation gates. No actionable findings.

Verified the BigQuery predicate matches the established dual-gate semantics, Deferred — specialist engagement is only introduced by the BigQuery specialist gate, executable bq extract guidance has been removed, 37 tests pass, and the Markdown structure remains valid. The change restores the intended safety boundary with minimal scope. Recommended for merge.

@ayn-builds

Copy link
Copy Markdown
Collaborator

The fix here is right, but it only covers one of the two copies of this file.

main already has a byte-identical copy at:

advisor/plugins/aws-startup-advisor/skills/migration-to-aws/references/phases/generate/generate-artifacts-scripts.md

I diffed it against the gcp-to-aws version this PR changes and they're identical on main. So
after this merges, the advisor plugin still has the BigQuery to S3 block with bq extract and
the gs:// to s3:// sync, which is the exact thing the specialist gate is there to prevent.
Someone running the advisor plugin gets the old behavior.

Could you add the same change to that file? Same three edits: the has_bigquery flag, the
deferral notice replacing the export block, and the quality rule.

Worth knowing this isn't caught by tooling. sync-vendored-shared.ts only keeps
skills/shared in sync with each skill's references/vendored/ inside a single plugin, and
mise.toml runs it once per plugin, so mise run shared:check passes while the two plugins
disagree. Nothing flags migrate and advisor drifting apart.

Two smaller things, neither blocking:

has_databases still lists google_bigquery_, so a BigQuery-only project sets
has_data_migration and generates the Cloud SQL to RDS and Firestore to DynamoDB blocks with
no such resources in the design. That predates this PR, just noting it since you're in here.

The second half of the has_bigquery predicate (aws_service = Deferred — specialist engagement)
is broader than the name suggests. I grepped and BigQuery is the only thing that ever gets that
value today, so it's harmless, but it'll quietly turn on the BigQuery notice if anything else is
ever deferred.

Rest of it checks out: bq extract is gone from the gcp-to-aws path, has_bigquery matches the
definition at discover-preview.md:244, and the wording lines up with the gate at
design-infra.md:40.

Gen Li added 3 commits August 11, 2026 14:31
…isor's copy of the scripts phase

The advisor plugin vendors a byte-identical copy of
generate-artifacts-scripts.md. The previous commit fixed only the
gcp-to-aws copy, so the advisor path still emitted the bq-extract /
S3-sync block the specialist gate exists to prevent. Same three edits:
the has_bigquery flag, the deferral notice replacing the export block,
and quality rule 6. The two files are byte-identical again.

Nothing catches this drift automatically: sync-vendored-shared.ts only
syncs skills/shared within a single plugin, so the two plugins can
disagree without failing shared:check.
…gration scripts

has_databases still listed google_bigquery_, so a BigQuery-only project
set has_data_migration and generated the Cloud SQL to RDS and Firestore
to DynamoDB blocks with no such resources in the design. BigQuery is
specialist-deferred and owns no automated data steps — it now counts
only toward has_bigquery. A project with BigQuery plus real databases
is unaffected (the other prefixes still set the flag). Applied to both
vendored copies; files remain byte-identical.

Noted by ayn-builds in awslabs#202 review.
@leon1418
leon1418 requested a review from a team as a code owner August 11, 2026 21:46
@leon1418

Copy link
Copy Markdown
Contributor Author

Both copies now, plus the invited fix:

Advisor copy (7bd8947) — same three edits applied to advisor/.../generate-artifacts-scripts.md; the two files are byte-identical again (verified with diff). Good catch on the tooling gap — you're right that sync-vendored-shared.ts only syncs within a plugin. A cross-plugin identity check would have caught this; happy to add one in a follow-up if you think it's worth having.

has_databases (e1d4d26) — took the invitation. google_bigquery_ is out of the list in both copies, with a NOT-comment at the definition so it doesn't creep back. BigQuery-only projects no longer set has_data_migration; projects with BigQuery plus real databases are unaffected (the other prefixes still set the flag).

Predicate breadth — leaving as-is deliberately. You're right it's broader than the name; today Deferred — specialist engagement is only ever BigQuery, and if a second deferral class shows up, renaming/splitting the flag at that point is the honest fix rather than pre-guessing its shape now.

Branch is updated with main (includes #196/#197).

@leon1418

Copy link
Copy Markdown
Contributor Author

Note on the two merge commits: #206 moved the advisor's copy (skills/migration-to-aws/skills/gcp-to-aws/) after this PR's fix landed on the old path, which put the PR into conflict. The fix has followed the file to its new home — both remaining copies (migrate/.../gcp-to-aws/ and advisor/.../gcp-to-aws/) re-verified byte-identical, with the has_bigquery gate, the deferral notice, quality rule 6, and the has_databases fix all present at the new path.

@ayn-builds ayn-builds left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed at the new path. The advisor half is in and both copies are byte-identical, drift:check is green (250 identical, 25 allowlisted, 0 drift, 0 missing), bq extract has exactly one hit per copy (the prohibition line), and I re-grepped the deferred marker today - Deferred — specialist engagement is still set only by the two BigQuery gates, so the second clause of has_bigquery can't over-trigger. Narrowing has_databases is also right where it's consumed: script 05's block at :369 is an RDS-only check.

Two notes inline, both fallout from the has_databases change I asked for, and neither blocking - the harmful guidance is gone either way and the docs callout at generate-artifacts-docs.md:112 still reaches the customer. I'd land the first one here rather than in a follow-up, since it's one line in a file that's already open and it has to be applied to both copies.

Minor: the description still says "One file changed, +22/−5" and ticks only migrate/ under Team Folder, while the diff is 2 files across both plugins.

`google_bigtable_`, `google_redis_` (NOT `google_bigquery_` — BigQuery is specialist-deferred
and carries no automated data-migration steps; see `has_bigquery`)
- **has_bigquery**: true if ANY resource has `gcp_type` starting with `google_bigquery_`
OR `aws_service` = "Deferred — specialist engagement"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new flag needs wiring into has_data_migration on line 41, which is still has_databases OR has_storage.

Now that google_bigquery_ is out of has_databases, a BigQuery-only project (BigQuery + Cloud Run, no Cloud SQL/Firestore/Bigtable/Redis, no GCS bucket) has both flags false - so line 81 applies, "Skip this script entirely if has_data_migration is false", and script 02 is never generated. The deferral notice this PR adds never renders for exactly the customer the specialist gate exists to protect. Before the change BigQuery set has_databases = true, so the block always had a script to live in.

One line:

- **has_data_migration**: has_databases OR has_storage OR has_bigquery (used for script 02)

Not blocking - the report callout at generate-artifacts-docs.md:112 fires independently, so the customer still gets the advisory. But it does mean the live-echo rationale in the description ("actually reaches the customer running 02-migrate-data.sh") doesn't hold for the pure-BigQuery case.

# data warehouse, lake, SQL analytics, or BI cutover planning — query patterns,
# data volumes, ETL/ELT, and downstream consumers must be assessed by specialists.
# No BigQuery migration steps are generated here by design.
echo "BigQuery: AWS target deferred — specialist engagement required."

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth hoisting the preamble in this PR rather than deferring it, because this change makes it reachable.

#!/usr/bin/env bash, set -euo pipefail, and the --execute plumbing all sit inside the Cloud SQL block at :97-98, gated on has_databases. Script 02 has no has_storage subsection at all - its three subsections are Cloud SQL (has_databases), this one (has_bigquery), and Firestore (has_databases).

So a BigQuery + GCS project (no relational DB) now generates an 02-migrate-data.sh that is these echo lines and nothing else: no shebang, no set -euo pipefail, and no dry-run vs --execute distinction. That contradicts Script Quality Rule 1 at :396 - "All scripts use set -euo pipefail" - two lines above the new rule 6.

This is follow-up 2 in your description. It was unreachable while BigQuery forced has_databases = true; it isn't now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants