Skip to content

fix: report NONE gateways for replacement instead of an impossible update - #145

Merged
Atom-oh merged 1 commit into
devfrom
fix/gateway-auth-replacement
Sep 24, 2026
Merged

Atom-oh merged 1 commit into
devfrom
fix/gateway-auth-replacement

Conversation

@Atom-oh

@Atom-oh Atom-oh commented Sep 24, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Follow-up to fix: require IAM inbound auth on AgentCore gateways #144. The dev Deploy AgentCore run failed with ValidationException on all 9 gateways; the CLI message is Authorizer type cannot be updated for an existing gateway. An in-place NONE → AWS_IAM switch is impossible.
  • A deployed NONE gateway now records ERR with the new fixed code gateway_auth_replacement_required (visible in the bounded diagnostics stream) without sending the rejected update. Role/description drift still converges, preserving the deployed auth.
  • The provisioner still never deletes or recreates gateways (no automatic destructive recreation). docs/reference/05-agentcore.md documents the operator replacement procedure (stack-scoped selection, delete targets → gateway, poll to not-found, re-run provisioner, verify).
  • CHANGELOG bullet amended in place (EN/KO); CLAUDE.md + regenerated AGENTS.md.

Test plan

  • python3 -m pytest -q scripts/v2/agentcore — 88 passed (NONE → ERR with no update call; NONE + role drift still converges keeping NONE)
  • dev: replace the 9 awsops-v2-* gateways per the new procedure, re-run Deploy AgentCore smoke=true, confirm AWS_IAM + unsigned rejection

🤖 Generated with Claude Code

…date

UpdateGateway rejects authorizer-type changes ("Authorizer type cannot be
updated for an existing gateway"), so the in-place NONE -> AWS_IAM switch
failed with ValidationException on every existing gateway. Record a fixed
ERR code (gateway_auth_replacement_required) without sending the update,
keep converging role/description drift, and document the operator
replacement procedure. The provisioner still never deletes gateways.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Atom-oh
Atom-oh deployed to ci-review-auto September 24, 2026 02:27 — with GitHub Actions Active
@github-actions

Copy link
Copy Markdown

🤖 AI Code Review (two independent reviewers)

_Reviewer responses: codex/ALL claude/ALL _

Status: PASSED — No blocking issues found

All claims are now verified against the base file plus the patch. I have what I need to synthesize the chair review.

Chair Review — PR #145: fix: report NONE gateways for replacement instead of an impossible update

1. Summary

This PR correctly removes the impossible in-place authorizer upgrade (UpdateGateway rejects authorizer-type changes), replacing it with a fail-loud ERR gateway_auth_replacement_required and a documented manual replacement procedure, and it updates every doc surface (CHANGELOG en/ko, CLAUDE.md, AGENTS.md, reference) coherently. The change is conservative on the ADR-005 boundary — it removes an automated mutation and adds no delete/recreate path. Verification against the base code confirmed one real behavioral wart (a NONE gateway with no other drift is reported both ERR and EXISTS) and one documentation edge case in the new replacement procedure; both are MINOR.

2. Issues per lens

L2 — Code correctness

  • MINOR (claude flagged; verified CONFIRMED): a compliant-otherwise NONE gateway is reported both ERR and EXISTS. In the patched ensure_gateways, when authorizerType == "NONE" but role and description match, the code logs ERR gateway_auth_replacement_required and then falls into the unchanged else: log(..., "EXISTS", name) branch (base scripts/v2/agentcore/provision.py:174-175 survives the patch). The run still exits nonzero (provision.py:1356-1357 fails on any ERR), so the security posture is correct, but the same gateway simultaneously claims "configuration match" — which docs/reference/05-agentcore.md still defines EXISTS to mean. Codex found no L2 defect (its test run was blocked by a sandbox ENOSPC); the disagreement resolves in claude's favor on direct code reading.
  • MINOR (claude flagged; CONFIRMED): the new test doesn't pin that behavior. test_none_authorizer_fails_without_an_impossible_update unpacks statuses ({"ERR", "EXISTS"} given _run_gateways("new text", ...) where "new text" matches the mocked catalog description) but never asserts on it, silently accepting the dual record. assertEqual({"ERR"}, statuses) after the code fix would lock the contract.
  • MINOR (claude flagged; CONFIRMED as intended-but-fragile): authorizerType="NONE" is re-sent on the role/description-drift update path. test_none_authorizer_role_drift_still_converges_without_changing_auth asserts this explicitly, and it follows from the preserve-fields design. It also means failure_status is WARN for a NONE gateway with description-only drift whose update fails — safe only because the separate auth ERR already fails the run. That coupling deserves a one-line code comment.

L3 — Security / AWS mutation safety

  • No CRITICAL/MAJOR. Both panels agree the ADR-005 direction is correct: the patch deletes an automated mutation, never calls delete_gateway, preserves gateway IDs for Runtime routing and ADR-017 teardown (asserted by the new test), and keeps create-time AWS_IAM (GATEWAY_AUTHORIZER is still consumed at provision.py:184, so no orphaned symbol). No IAM policy, secret, or Terraform change. The new diagnostic code is a fixed-vocabulary token added to provision_report.CODES, consistent with the no-raw-messages contract.
  • Codex's MAJOR — downgraded to MINOR (L5) after verification. Codex flagged that the replacement procedure's step 1 filter ("roleArn is this stack's AgentCore role and ID appears in this stack's Runtime GATEWAYS_JSON") can exclude the very gateway needing replacement: a NONE gateway with role drift whose role-converging update persistently fails keeps its old roleArn, so the conjunctive filter never matches it while every make agentcore run keeps failing. I verified the doc text and the code path: the scenario is real, but it requires a compound edge case (legacy NONE authorizer and a persistently failing role update), the provisioner identifies the gateway by key in the ERR report on every run, and the conjunctive filter is deliberate cross-stack deletion protection — relaxing it as suggested trades this remediation-availability edge case against a worse mutation-safety risk (deleting another stack's gateway). Claude independently reviewed the same doc section and did not reach this finding. Not gate-blocking; addressed as a doc suggestion below.
  • MINOR (claude flagged): AGENTS.md relaxes "flag any code that creates or keeps NONE" to "creates NONE", while the update path now records a CloudTrail UpdateGateway event re-asserting authorizerType: NONE. Behaviorally required by the preserve-fields API shape, but worth an explanatory comment; cosmetic description convergence on a gateway slated for deletion buys little.

L4 — Observability / data-integration correctness

  • No Steampipe/CIS/diagnosis logic is in scope — both panels agree. The report plumbing is correct: gateway_auth_replacement_required is in CODES, so _reason() returns it verbatim (provision_report.py:78) instead of collapsing to operation_failed; the code fits the Node-side ^[a-z][a-z0-9_]{0,63}$ allowlist; _resource("gateway:<key>") maps correctly (provision_report.py:113).
  • MINOR (claude flagged; consequence of the L2 finding): the affected gateway emits two contradictory records (ERR + EXISTS), so summary counts and the step-summary line show an EXISTS for a gateway simultaneously requiring replacement. Fixing the L2 branch resolves this.

L5 — Docs / ADR consistency

  • Docs are coherently and completely updated — both panels agree. No stale auto-upgrade claim survives anywhere (CHANGELOG en+ko mirrored 1:1 per the CLAUDE.md rule — the fix correctly amends the existing [Unreleased] bullet in place rather than appending; CLAUDE.md, generated AGENTS.md with bumped sha, reference, and both docstrings all rewritten). Referenced commands (make agentcore SMOKE=1, Deploy AgentCore smoke input) exist.
  • MINOR (codex flagged as MAJOR, downgraded — see L3): the step-1 ownership filter can exclude a role-drifted NONE gateway. Suggested doc fix below rather than a gate block.
  • MINOR (claude flagged): the new procedure omits the operator permissions it needs (ListGatewayTargets, DeleteGatewayTarget, DeleteGateway, plus create verbs for the re-run), in a document that otherwise calls out bedrock-agentcore:GetGateway explicitly, and gives CLI verbs without the aws bedrock-agentcore-control prefix.
  • MINOR (claude flagged; CONFIRMED): EXISTS doc/code mismatch — the reference still says "EXISTS means configuration match", contradicted by the dual-record behavior until the L2 fix lands.

3. Suggestions

  1. Make the no-drift branch an elif/guard so a NONE gateway never also logs EXISTS, and tighten the new test to assertEqual({"ERR"}, statuses). This single fix resolves the L2, L4, and EXISTS-doc findings together.
  2. In docs/reference/05-agentcore.md step 1, add one sentence covering the role-drift edge case: a gateway named in the provisioner's ERR report whose ID appears in this stack's GATEWAYS_JSON qualifies even if its roleArn has not converged (e.g. after a failed role update), subject to the same explicit cross-stack confirmation. Keep the conservative conjunctive default.
  3. Add a short code comment where the preserve-fields request re-sends authorizerType="NONE", noting it is required by the update API shape and that the separate auth ERR is what fails the run.
  4. List the delete/create permissions the replacement procedure requires and use fully prefixed aws bedrock-agentcore-control CLI commands.

4. Verdict

All confirmed findings are MINOR. Codex's sole MAJOR was verified as a real but narrow documentation edge case in a deliberately conservative operator procedure, with fail-loud behavior preserved on every run — downgraded and excluded from the gate. No CRITICAL/MAJOR remains; the manifest lists no images.

IMAGE_COVERAGE: NOT_REQUIRED


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

@Atom-oh
Atom-oh merged commit db1289c into dev Sep 24, 2026
6 checks passed

This branch was successfully deployed

1 active deployment
ci-review-auto — be870a78 Deployed Sep 24, 2026 by Atom-oh via AI Code Review #581
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.

1 participant