diff --git a/.github/workflows/platform.yml b/.github/workflows/platform.yml index da38f520..c9201d54 100644 --- a/.github/workflows/platform.yml +++ b/.github/workflows/platform.yml @@ -57,6 +57,11 @@ jobs: CDK_PROJECT_PREFIX: ${{ vars.CDK_PROJECT_PREFIX }} CDK_DOMAIN_NAME: ${{ vars.CDK_DOMAIN_NAME }} CDK_CORS_ORIGINS: ${{ vars.CDK_CORS_ORIGINS }} + # The Environment tag value. Not cosmetic: it is the filter the managed-KB + # reconciler and scripts/teardown/managed-kb.sh match knowledge bases on, so + # an unset value here made a dev deploy tag its knowledge bases 'prod' while + # teardown looked for 'dev' — matching nothing and reporting success. + CDK_TAG_ENVIRONMENT: ${{ vars.CDK_TAG_ENVIRONMENT }} CDK_VPC_CIDR: ${{ vars.CDK_VPC_CIDR }} CDK_ALB_SUBDOMAIN: ${{ vars.CDK_ALB_SUBDOMAIN }} CDK_CERTIFICATE_ARN: ${{ vars.CDK_CERTIFICATE_ARN }} diff --git a/backend/tests/supply_chain/test_kb_tag_contract.py b/backend/tests/supply_chain/test_kb_tag_contract.py index 12764da0..6cf7d1bd 100644 --- a/backend/tests/supply_chain/test_kb_tag_contract.py +++ b/backend/tests/supply_chain/test_kb_tag_contract.py @@ -195,6 +195,80 @@ def test_the_construct_supplies_real_values_not_literals(self): assert "config." in line, f"prefix tag value is not derived from config: {line.strip()}" +class TestTheEnvironmentTagValueIsPlumbedNotGuessed: + """The gap the key-name tests above could not see. + + Every existing test here checks that the three languages agree on the tag + *keys*, and that the Python writer and Python filter share one fallback + chain. Nothing checked that the value the **CDK computes** and the value the + **teardown script defaults to** agree — and they did not. + + `managedKbEnvironmentTagValue` falls back to `production ? 'prod' : 'nonprod'`; + `config.production` defaults to `true` because `platform.yml` never passed a + production flag and `cdk.context.json` says `true`. So a *development* deploy + tagged its knowledge bases `ManagedKbEnvironment=prod`, while + `scripts/teardown/managed-kb.sh` falls back to `dev`. Teardown would have + matched nothing and reported a clean run, leaving billed Bedrock knowledge + bases behind — the exact symptom of the original tag-contract drift, one + layer up and invisible to the tests written for it. + + The fix is to stop relying on either fallback: the value is passed explicitly + per environment. These tests guard that plumbing end to end, because an + omission anywhere in the chain silently reinstates the guess. + """ + + WORKFLOW = REPO_ROOT / ".github/workflows/platform.yml" + LOAD_ENV = REPO_ROOT / "scripts/common/load-env.sh" + CONFIG_TS = REPO_ROOT / "infrastructure/lib/config.ts" + + def test_the_workflow_passes_an_environment_tag_value(self): + body = self.WORKFLOW.read_text(encoding="utf-8") + assert "CDK_TAG_ENVIRONMENT:" in body, ( + "platform.yml does not pass CDK_TAG_ENVIRONMENT, so config.tags.Environment " + "is unset and the construct falls back to the production guess" + ) + assert "vars.CDK_TAG_ENVIRONMENT" in body, ( + "CDK_TAG_ENVIRONMENT is declared but not sourced from an environment " + "variable, so it cannot differ between development and production" + ) + + def test_load_env_forwards_it_as_the_flat_dotted_context_key(self): + """`--context a.b=c` sets `context['a.b']`, never nested `a.b`.""" + body = self.LOAD_ENV.read_text(encoding="utf-8") + assert "CDK_TAG_ENVIRONMENT" in body, "load-env.sh does not export the variable" + assert '--context tags.Environment=' in body, ( + "load-env.sh does not forward the value to CDK, so the workflow variable " + "is exported and then dropped" + ) + + def test_config_reads_the_flat_key_and_not_only_the_nested_object(self): + """A nested-only read is how the two earlier flat-key defects worked.""" + body = self.CONFIG_TS.read_text(encoding="utf-8") + assert "tryGetContext('tags.Environment')" in body, ( + "config.ts reads only the nested `tags` object, so --context " + "tags.Environment=... is silently ignored" + ) + + def test_the_shell_fallback_is_not_the_constructs_fallback(self): + """Documents the divergence rather than pretending it is gone. + + Both fallbacks still exist and still disagree; they are simply no longer + reached in a deployed environment. Asserting the disagreement keeps the + reason the plumbing is mandatory visible — if someone later deletes the + plumbing, the tests above fail and this one explains why it matters. + """ + construct = CONSTRUCT.read_text(encoding="utf-8") + script = (REPO_ROOT / "scripts/teardown/managed-kb.sh").read_text(encoding="utf-8") + + assert "'prod' : 'nonprod'" in construct, ( + "the construct's documented fallback changed; re-check that it now " + "agrees with the teardown script's, or that neither is reachable" + ) + assert "CDK_ENVIRONMENT:-dev" in script, ( + "the teardown script's documented fallback changed; re-check the pair" + ) + + class TestTheTeardownScriptAgrees: def test_the_script_matches_the_canonical_key_names(self): """It previously matched ``prefix``/``env`` — keys nothing ever wrote.""" diff --git a/infrastructure/lib/config.ts b/infrastructure/lib/config.ts index f2e2be9d..a67026bc 100644 --- a/infrastructure/lib/config.ts +++ b/infrastructure/lib/config.ts @@ -758,6 +758,17 @@ export function loadConfig(scope: cdk.App): AppConfig { : undefined, tags: { ...(scope.node.tryGetContext('tags') || {}), + // `--context tags.Environment=dev` sets the FLAT dotted key + // `context['tags.Environment']`; it does NOT merge into the nested `tags` + // object above, so a nested-only read silently ignores an operator's own + // flag. That trap has already bitten this repo twice (the managed-KB byte + // caps and then the alarm thresholds), and here it had a sharper edge: the + // Environment tag is a *filter* for the reconciler and for teardown, so + // ignoring it does not degrade cosmetically — it makes teardown match + // nothing and report success. + ...(scope.node.tryGetContext('tags.Environment') + ? { Environment: String(scope.node.tryGetContext('tags.Environment')) } + : {}), }, }; diff --git a/scripts/common/load-env.sh b/scripts/common/load-env.sh index 5e56a6f3..95f64b50 100644 --- a/scripts/common/load-env.sh +++ b/scripts/common/load-env.sh @@ -121,6 +121,21 @@ build_cdk_context_params() { if [ -n "${CDK_CORS_ORIGINS:-}" ]; then context_params="${context_params} --context corsOrigins=\"${CDK_CORS_ORIGINS}\"" fi + + # The Environment tag value. Forwarded as the FLAT dotted key because that is + # what `--context a.b=c` sets; config.ts merges it into `config.tags` + # explicitly for the same reason. + # + # This is not a cosmetic label. `managedKbEnvironmentTagValue` uses it, and + # that value is the filter the reconciler and `scripts/teardown/managed-kb.sh` + # match knowledge bases on. Left unset, the construct falls back to + # `production ? 'prod' : 'nonprod'` while the teardown script falls back to + # `dev` — so a dev deploy tagged its knowledge bases `prod` and teardown, + # looking for `dev`, deleted nothing and reported success. Same shape as the + # tag-contract drift in the spec's defect list, one layer up. + if [ -n "${CDK_TAG_ENVIRONMENT:-}" ]; then + context_params="${context_params} --context tags.Environment=\"${CDK_TAG_ENVIRONMENT}\"" + fi # App API optional parameters if [ -n "${CDK_APP_API_CPU:-}" ]; then @@ -303,6 +318,13 @@ export CDK_TOKEN_EXCHANGE_CLIENT_ID="${CDK_TOKEN_EXCHANGE_CLIENT_ID:-$(get_json_ # Shared CORS origins — env var > context file (no hardcoded defaults) export CDK_CORS_ORIGINS="${CDK_CORS_ORIGINS:-$(get_json_value "corsOrigins" "${CONTEXT_FILE}")}" +# Environment tag value (`Environment` in config.tags). Stamped on every +# CDK-created resource by applyStandardTags, and — the part that matters — +# used as the match filter for managed knowledge base reconciliation and +# teardown. Empty is honoured rather than defaulted here so config.ts's own +# fallback stays the single documented default. +export CDK_TAG_ENVIRONMENT="${CDK_TAG_ENVIRONMENT:-$(get_json_value "tags.Environment" "${CONTEXT_FILE}")}" + # File upload configuration — env var > context file (no hardcoded defaults) export CDK_FILE_UPLOAD_MAX_SIZE_MB="${CDK_FILE_UPLOAD_MAX_SIZE_MB:-$(get_json_value "fileUpload.maxFileSizeBytes" "${CONTEXT_FILE}")}"