[Atom-oh]

Summary

Split out of #183 so that PR stays within the AI review input limit; independent of the connector changes.

• New jaeger_vpc_enabled / dynatrace_vpc_enabled variables (default false, nullable = false), validated to require agentcore_enabled && integrations_enabled like the existing per-kind VPC flags.
• When enabled, the Jaeger / Dynatrace connector Lambdas get the shared vpc_config so private endpoints are reachable; the ENI-policy attachment only widens its count predicate — no IAM statement changes.
• tests/runtime_iam.tftest.hcl pins defaults-dark (plan = no changes with both flags false) and selective attachment.

Operators must review a saved plan and confirm private DNS, subnet routes and target security groups before enabling either flag; this PR activates nothing.

Tests

• terraform fmt -check clean; terraform test (mock provider) → see run log: all cases passed.

🤖 Generated with Claude Code

[github-actions]

🤖 AI Code Review (two independent reviewers)

_Reviewer responses: codex/ALL claude/ALL _

Status: PASSED — No blocking issues found

All base-file claims verified — both jaeger-mcp/dynatrace-mcp Lambda keys exist in local.agent_lambdas gated on agentcore_enabled && integrations_enabled, Terraform is pinned ≥ 1.15 (cross-variable validation is legal), and the ENI policy is the pre-existing wildcard body. Here is the synthesized chair review:

---

Chair Review — PR #190: feat(terraform): add default-off Jaeger and Dynatrace VPC attachment flags

Summary

This Terraform-only patch adds two default-off, nullable = false boolean flags (jaeger_vpc_enabled, dynatrace_vpc_enabled) that opt the existing jaeger-mcp and dynatrace-mcp connector Lambdas into the private subnets, extends the compound-gated ENI IAM policy and vpc_config dynamic block accordingly, and adds three tftest runs covering dark defaults, prerequisite validation, and selective attachment. I verified the key base-reconciliation facts directly: both Lambda keys exist in local.agent_lambdas (ai.tf:873-874, gated on integ_count), datadog-mcp exists for the negative assertion, existing_private_subnet_ids is a declared variable used by the test harness, and backend.tf pins TF ≥ 1.15 so the cross-variable validation blocks are valid. The two panels agree there are no blocking issues; claude-ALL surfaced several verified MINOR improvements that codex-ALL did not, all consistent with the diff and base.

Issues per lens

L2 — Code correctness

No CRITICAL/MAJOR. Both panels agree the gating logic is correct: the validation blocks make flag=true without agentcore_enabled && integrations_enabled a plan-time error, so aws_iam_role.agent_lambda[0] can never be indexed when absent, and the each.key strings match the map keys exactly.

• MINOR (claude only; verified) — The new attach test enables both flags together, so a transposed flag/key pairing (e.g. each.key == "jaeger-mcp" && var.dynatrace_vpc_enabled) would still pass. An asymmetric run (one flag on, asserting the other Lambda's vpc_config length is 0) would pin the mapping.
• MINOR (claude only; verified) — Three sub-conditions in the new assert (!var.official_mcp_enabled && !var.remediation_enabled && !var.integrations_write_enabled) re-read variable defaults the run never overrides, so they are tautological; asserting resource counts (e.g. length(aws_iam_role_policy.official_mcp_credentials) == 0) would actually test the claim. The six-clause bundle also shares one error_message.
• MINOR (claude only; verified) — The six sibling *_vpc_enabled flags carry equivalent "Requires …" prose but no validation block, so the new pair is stricter than its peers. Not a defect of this PR (it improves on the status quo), but worth unifying.

L3 — Security / AWS mutation safety

No CRITICAL/MAJOR. Both panels agree ADR-005 is not crossed: no new AWS write action, no change to remediation_enabled/integrations_write_enabled/official-MCP gates, no secrets, and the extended defaults_remain_dark run pins the $0 no-op default. The patch body of the ENI policy is unchanged.

• MINOR (claude only; verified against ai.tf:518-525) — The newly-reachable agent_lambda_vpc_eni policy grants ec2:CreateNetworkInterface/DescribeNetworkInterfaces/DeleteNetworkInterface on Resource = "*" with no condition, on the role shared by all connector Lambdas. Pre-existing, but this diff adds two activation paths; scoping with ec2:Subnet/ec2:Vpc conditions would be least-privilege-correct.
• MINOR (claude only; pre-existing pattern) — Attachment reuses the shared aws_security_group.service, which Aurora trusts on 5432, giving the VPC-attached connectors a network path (not a credential) to in-VPC data planes. Identical to the six existing flags; a dedicated egress-only connector SG would bound blast radius.

L4 — Observability / data-integration correctness

No CRITICAL/MAJOR. Both panels agree the change doesn't touch Steampipe, CIS checks, or diagnosis logic, and that the dynatrace_vpc_enabled claim of not affecting the official hosted MCP preset is accurate (the preset routes via the external-obs gateway, not this Lambda).

• MINOR (claude only) — Neither connector participates in chat routing or the diagnosis fan-out (local.ds_connector_arns lists only prometheus/loki/tempo/mimir/clickhouse); only the BFF Datasources explore/test path gains private reachability. Stating this in the descriptions would prevent an operator enabling the flag expecting diagnosis coverage.
• MINOR (claude only) — Flipping dynatrace_vpc_enabled moves a hosted *.live.dynatrace.com connector into private subnets; without an operator NAT route, a previously working connector starts timing out with an opaque error. The description warns about egress, but a runbook note would make the failure mode discoverable.

L5 — Docs / ADR consistency

No CRITICAL/MAJOR. There is no docs/decisions/ directory in this tree (ADRs live in the private upstream, cited by number — per root CLAUDE.md), and the README flag tables deliberately exclude all *_vpc_enabled flags, so no doc is made stale.

• MINOR (claude only) — No operator runbook covers the datasource-connector VPC flags (unlike istio_vpc_enabled, which has docs/runbooks/istio-agent-eks-access.md); all prerequisites live only in variable descriptions. Any new runbook content must be English-only per the docs language scoping rules.
• MINOR (claude only) — The rewritten ENI-policy comment ("ENI permissions exist only while a managed Lambda VPC option is enabled") drops the note that the grant lands on the shared agent_lambda role; with nine activation paths and a wildcard resource, that specificity was worth keeping.

Panel agreement note: codex-ALL reported zero findings across all lenses; claude-ALL's findings are all MINOR and each one I spot-checked against the base held up. The disagreement is one of depth, not of substance — neither panel found a blocking issue, and I confirmed none exists in the resulting code.

Suggestions

1. Add an asymmetric tftest run (one flag on, one off) to pin the flag→Lambda mapping, and replace the tautological !var.* assertions with resource-count assertions.
2. Consider scoping agent_lambda_vpc_eni with ec2:Subnet/ec2:Vpc ARN conditions and/or a dedicated connector SG in a follow-up — pre-existing, but each new flag widens its activation surface.
3. Add a short English-only runbook subsection enumerating the nine *_vpc_enabled flags and their routing/DNS/NAT prerequisites, including the Dynatrace-SaaS NAT caveat.
4. Backfill the prerequisite validation blocks onto the six sibling VPC flags for uniformity.

Verdict

Default-off, validation-gated, selectively attached, test-covered, and consistent with the repo's flag-gating discipline and ADR-005 freeze. All findings are MINOR; none blocks merge. The image manifest lists zero images, zero omissions, and zero unavailable entries.

IMAGE_COVERAGE: NOT_REQUIRED

---

Triggered by commit 1a82c02988f012e15447c3c342de70740ae39cab · workflow: .github/workflows/pr-review.yml