Skip to content

feat(operator): oabctl ingress for Telegram/LINE (API Gateway + VPC Link + Cloud Map)#1275

Merged
thepagent merged 13 commits into
mainfrom
feat/oabctl-ingress-apigateway
Jul 2, 2026
Merged

feat(operator): oabctl ingress for Telegram/LINE (API Gateway + VPC Link + Cloud Map)#1275
thepagent merged 13 commits into
mainfrom
feat/oabctl-ingress-apigateway

Conversation

@chaodu-agent

Copy link
Copy Markdown
Collaborator

Summary

Adds an optional spec.ingress block to the oab.dev/v2 manifest so oabctl apply provisions inbound webhook ingress in one shot — the exact "Option 1" architecture (API Gateway HTTP API → VPC Link → Cloud Map → ECS task) documented in the Telegram/LINE-on-AWS reference architecture (#1274). This collapses the ~7 manual aws apigatewayv2 / servicediscovery / ecs commands from that doc into declarative manifest fields.

Motivation: #1274 recommends the API Gateway + VPC Link + Cloud Map path as the cheapest AWS-native ingress (~$5–10/mo), but blessing it as "recommended" while it requires 7 hand-run CLI steps — including a delete-and-recreate of the ECS service — is a lot of error-prone plumbing. oabctl already owns this layer (bootstrap + apply), so ingress is the missing piece.

What it does

With spec.ingress present (ECS runtime only), apply reconciles idempotently (all resources looked up/reused by name):

  1. Cloud Map private DNS namespace + a per-service A record
  2. ECS service registry wiring — attached at service creation
  3. VPC Link (shared oab-vpc-link), waited until AVAILABLE
  4. API Gateway HTTP API (oab-webhook) + HTTP_PROXY integration over the VPC Link
  5. one route per path + a prod auto-deploy stage
  6. a self-referencing security-group inbound rule on containerPort

Then it prints the stable webhook URL(s) to register with BotFather / the LINE console.

spec:
  # ... image / resources / configFrom / runtime.networking ...
  ingress:
    type: apigateway          # only supported type (default)
    cloudMapNamespace: oab    # reused across bots in the VPC (default: oab)
    containerPort: 8080       # default
    paths:
      - /webhook/telegram
      - /webhook/line

Backward compatibility

ingress is optional and defaults to absent. Existing outbound-only (Discord) deployments create no ingress resources and behave exactly as before. Covered by a unit test.

Notes / caveats (called out in code + README)

  • Service registries are create-only. apply attaches the registry on create. For a pre-existing service without service discovery it provisions the ingress resources and prints how to recreate the service — it does not auto-delete anything.
  • Transport is unauthenticated. The API Gateway endpoint is public; requests are authenticated at the app layer (LINE signature via LINE_CHANNEL_SECRET, Telegram token in the path). Documented explicitly.

Testing

Built on an M4 (macmini):

  • cargo build — OK
  • cargo clippy --all-targets -- -D warnings — clean
  • cargo test7 passed (schema parse/defaults, validation failures, k8s rejection, backward-compat, fleet passthrough)
  • schema/oabservice-v2.json — valid JSON; ingress added to serviceSpec / fleetTemplate / agentOverride

Live AWS reconciliation (real Cloud Map / VPC Link / API Gateway calls) has not been exercised end-to-end yet — the SDK call shapes compile and follow the refarch, but a real apply against an account is the recommended next validation step before docs promote it.

Follow-up

Once merged, #1274's Option 1 can add a "Recommended: provision with oabctl" section that replaces the manual aws steps with the ingress: block, keeping the manual commands as an under-the-hood appendix.

Refs #1274

Adds an optional `spec.ingress` block to the oab.dev/v2 manifest so
`oabctl apply` can provision inbound HTTPS webhook ingress for
Telegram/LINE bots in one shot — instead of the ~7 manual `aws`
commands documented in the Telegram/LINE-on-AWS refarch (Option 1).

When `ingress` is present (ECS runtime only), apply reconciles, all
idempotently and reused by name:

- Cloud Map private DNS namespace + per-service A record
- ECS service registry wiring (attached at service *creation*)
- VPC Link (shared `oab-vpc-link`), waited until AVAILABLE
- API Gateway HTTP API (`oab-webhook`) + HTTP_PROXY integration
- one route per path + a `prod` auto-deploy stage
- a self-referencing security-group inbound rule on containerPort

It then prints the stable webhook URL(s) to register with the platform.

Backward compatible: `ingress` is optional and defaults to absent, so
existing outbound-only (Discord) deployments create no ingress
resources and behave exactly as before.

Because ECS service registries can only be set at creation time, apply
attaches the registry on create and, for a pre-existing service without
service discovery, provisions the ingress resources and prints how to
recreate the service (non-destructive — no automatic delete).

- manifest.rs: Ingress struct, validation, fleet passthrough, 7 unit tests
- ingress.rs: Cloud Map + VPC Link + API Gateway reconciliation
- apply.rs: two-phase wiring (Cloud Map before create, gateway after)
- schema/oabservice-v2.json: ingress def + refs
- README: ingress section
- Cargo.toml: aws-sdk-apigatewayv2, aws-sdk-servicediscovery

Refs #1274
@chaodu-agent

This comment has been minimized.

The ci.yml `operator` job runs `cargo check` with working-directory
operator, but the crate is not listed in the repo-root workspace
(members: openab-core, openab-gateway). Cargo then walks up to the root
Cargo.toml and errors ("believes it's in a workspace when it's not").
This job only runs when operator/** changes, so it was latent until now.

Adding an empty [workspace] table marks operator as its own workspace
root, matching how it's built and released standalone (own Cargo.lock).
…VPC Link constraint

Addresses PR review feedback:
- Extract pure helpers (integration_uri, route_key, webhook_urls) from
  ensure_gateway and add 4 unit tests (F3 — ingress.rs had no coverage).
- Warn at apply time when reusing the shared oab-vpc-link and document
  in README that all ingress bots in a VPC must share subnets/SGs, since
  a VPC Link's subnets/SGs are fixed at creation (F2).
@chaodu-agent

This comment has been minimized.

@chaodu-agent

This comment has been minimized.

@chaodu-agent

This comment has been minimized.

… paginate API GW lists

Addresses PR review round 3:
- F1: recreate hint used `oabctl delete service` (invalid) and omitted
  --cluster; now prints `oabctl delete oabservice <name> --cluster oab
  --namespace <ns>` which matches delete.rs's contract and the hardcoded
  `oab` cluster.
- F2: oabctl delete oabservice now tears down the bot's per-bot ingress
  resources (Cloud Map service + API Gateway routes/integration),
  best-effort, leaving shared VPC Link / HTTP API / SG rule intact.
  No-op for bots that never had ingress. Documented in README.
- F3: apigatewayv2 GetApis/GetVpcLinks/GetIntegrations/GetRoutes/GetStages
  have no smithy paginator in this SDK, so paginate manually via
  next_token loops instead of reading only the first page.

Verified: cargo build, clippy --all-targets -D warnings, cargo test
(11 passed) in a nested layout mirroring the CI operator job.
@chaodu-agent

This comment has been minimized.

@chaodu-agent

This comment has been minimized.

…lisions

Round-4 review:
- F1 (critical): routes on the shared oab-webhook API were keyed by path
  only, so two bots both declaring /webhook/telegram would collide — the
  second apply saw the first's route, skipped, and silently misrouted
  traffic. Give each bot its own HTTP API (oab-webhook-<ns>-<name>) so
  webhook paths can never clash across bots; each bot gets a distinct
  {api-id} endpoint URL. Simplifies teardown to a single delete_api
  (cascades routes/integration/stage). VPC Link + SG rule stay shared.
- F2: removed duplicated comment line in ensure_vpc_link.
- Added api_name() helper + unit test (12 tests total).

Verified: build, clippy --all-targets -D warnings, cargo test (12 passed)
in a nested layout mirroring the CI operator job.
@chaodu-agent

This comment has been minimized.

@chaodu-agent

This comment has been minimized.

Round-5 review F1: ensure_sg_ingress classified the "rule already exists"
case by substring-matching the Debug-rendered error string, which breaks
if the SDK changes error formatting. Match the typed AWS error code
InvalidPermission.Duplicate via ProvideErrorMetadata::code() instead.

(F2 — CI green — the operator job now passes on the prior commit.)

Verified: build, clippy --all-targets -D warnings, cargo test (12 passed).
@chaodu-agent

This comment has been minimized.

@chaodu-agent

This comment has been minimized.

…e 503/BadRequest)

Live E2E against a real account revealed the documented Option-1 wiring
does not work: an HTTP API VPC_LINK integration rejects a raw
'http://<dns>:<port>' URI with

  BadRequestException: For VpcLink VPC_LINK, integration uri should be a
  valid ELB listener ARN or a valid Cloud Map service ARN.

Correct wiring (matches CDK's HttpServiceDiscoveryIntegration):
- Cloud Map service uses an SRV record (not A) so the container port is
  captured; ECS registers the task IP + port into it.
- ECS service registry sets containerName/containerPort and the task def
  exposes the container port, so ECS writes the SRV record.
- API Gateway integration URI is the Cloud Map *service ARN* with
  connectionType=VPC_LINK and integrationMethod=ANY; the port is resolved
  from SRV.

Verified end-to-end (POST through API Gateway → VPC Link → Cloud Map SRV
→ Fargate task returned HTTP 200 with the request echoed at
/prod/webhook/telegram), then torn down cleanly.

build + clippy -D warnings + cargo test (11 passed).
@chaodu-agent

This comment has been minimized.

@chaodu-agent

This comment has been minimized.

@chaodu-agent

This comment has been minimized.

@chaodu-agent

This comment has been minimized.

@chaodu-agent

This comment has been minimized.

…eting; namespace-change + SG mismatch detection

Round-10 review (full team pass on 838c220):

- F1 (blocking): the documented recreate path (oabctl delete && apply)
  deleted the bot's HTTP API resource itself via delete_api(), so the
  next apply's ensure_api() found nothing and created a NEW api-id,
  silently rotating the webhook URL hostname and breaking any Telegram/
  LINE registration made against the old URL. Fixed by splitting
  teardown into two functions:
    - ingress::teardown(): strips routes/integration/stage but keeps the
      HTTP API resource, so its api-id (and thus the public URL) survives
      an ECS-service recreate. Used by both apply's ingress-removed path
      and delete's ingress cleanup.
    - ingress::delete_api(): permanently deletes the HTTP API. Only
      called from oabctl delete's full-removal path, where there's no
      URL to keep stable since the bot itself is gone.

- F2: Cloud Map service teardown matched by name via an account-wide
  list_services() scan, so two bots with the same namespace/name in
  different VPCs/environments could collide. delete.rs now captures the
  ECS service's actual service_registries ARN BEFORE deleting it and
  passes it to teardown(), which resolves the exact Cloud Map service ID
  from that ARN instead of searching by name. Falls back to the by-name
  scan only when no ARN is known (apply's ingress-removed path, where the
  service may already be gone).

- F3: has_registries only checked whether SOME registry was attached,
  not whether it matched the currently-resolved registry for the
  manifest's ingress.cloudMapNamespace. Changing cloudMapNamespace on an
  existing service silently left it pointed at the old namespace (503s)
  without triggering the recreate warning. apply now compares the
  resolved registry ARN against the service's actual registry ARNs and
  flags a mismatch as needing recreate too, with a distinct message.

- F4: VPC Link reuse only printed a reminder about subnet/SG matching,
  never validated it. ensure_vpc_link now calls GetVpcLink on the reused
  link and compares its actual security groups against the manifest's,
  warning loudly on mismatch. (Subnets aren't exposed by GetVpcLink, so
  those remain a documented reminder only.)

- F5: VPC Link duplicate-name tie-breaker sorted candidates by ID only,
  ignoring status, so a Pending duplicate could be picked over an
  Available one (wasted wait time, not a correctness bug). Now sorts
  AVAILABLE first, ID as tiebreaker.

- F6: Cargo.lock was stale (missing the two new SDK deps entirely).
  Regenerated and committed.

Added 2 new unit tests (cloud_map_service_id_from_arn parsing) — 15
total. Rewrote the README ingress caveats section to describe the
stable-URL guarantee, exact-ARN teardown targeting, namespace-change
detection, and SG validation.

Verified: build, clippy --all-targets -D warnings, cargo test (15
passed).
@chaodu-agent

This comment has been minimized.

@chaodu-agent

This comment has been minimized.

…reate needed

Round-11 review (full team pass): the PR's core operational caveat —
"ECS service registries can only be set at creation time, so an existing
service needs delete-and-recreate to get ingress" — is outdated and
wrong. Confirmed directly against the AWS API_UpdateService reference:
serviceRegistries has been a documented UpdateService parameter since
March 2022. "When you add, update, or remove the service registries
configuration, Amazon ECS starts new tasks with the updated service
registries configuration, and then stops the old tasks when the new
tasks are running" — a normal rolling replacement, no downtime gap, no
delete needed. Requires the AWSServiceRoleForECS service-linked role,
which ECS creates automatically the first time any service in the
account uses service discovery.

Removed the entire recreate code path:
- apply_ecs's update_service branch now attaches or replaces the
  serviceRegistries directly when there's no registry or a mismatch
  (registry_mismatch detection from round-10 is kept — just resolved
  automatically instead of printed as a manual instruction).
- Deleted the "needs_recreate" plumbing through apply_ecs/run (dead code
  once the update path handles it directly).
- Corrected the "create-only" claim in ingress.rs's module doc and
  apply.rs's ensure_cloud_map-ordering comment.
- Rewrote README's "Recreate caveat" into "Adding/fixing service
  discovery never requires recreating the service", describing the
  actual UpdateService rolling-replacement behavior and the
  service-linked role note.

Also addressed the two non-blocking findings from the same review round:
- Added an "Additional permissions for spec.ingress" table to the
  Prerequisites section (Cloud Map, API Gateway, EC2, ECS UpdateService
  actions) — this is new AWS API surface the caller's credentials need
  that wasn't documented anywhere.
- Corrected the security note: Telegram validates the
  X-Telegram-Bot-Api-Secret-Token header + source-IP allowlist (not just
  "token in the path"), LINE does HMAC-SHA256 signature verification —
  verified against crates/openab-gateway/src/adapters/{telegram,line}.rs.

Verified live end-to-end against a real AWS account:
1. Applied a manifest WITHOUT ingress -> plain ECS service created.
2. Added ingress, re-applied -> printed "updated (service discovery
   attached; rolling replacement, no downtime)", NOT a recreate warning.
3. Confirmed via describe-services: same serviceArn, same createdAt
   timestamp (service was never deleted), serviceRegistries now
   populated with the correct Cloud Map ARN.
4. Confirmed the task rolled (old task ARN -> new task ARN) rather than
   the service being replaced.
5. curl through the printed webhook URL end-to-end -> HTTP 200.
6. Deleted and cleaned up all test resources (service, HTTP API, Cloud
   Map service/namespace, VPC Link, SG rule) - account left clean.

Verified: build, clippy --all-targets -D warnings, cargo test (15
passed, unchanged from round-10 since no new pure logic was added here).
@chaodu-agent

This comment has been minimized.

@chaodu-agent

This comment has been minimized.

@chaodu-agent

This comment has been minimized.

Round-12 review (full team pass on 536e858): the ingress-removal path
(manifest edited to drop spec.ingress) had two gaps left over from
round-11's UpdateService redesign, both only reachable via that specific
transition:

- F1 (critical): the update_service branch only ever set
  `serviceRegistries` inside `if let Some(cm) = &cloud_map`. When ingress
  is removed, cloud_map is None, so the field was omitted from the
  request entirely. Per AWS's own UpdateService reference, an omitted
  serviceRegistries leaves the existing configuration unchanged — only an
  explicit empty list detaches it. Since ingress::teardown (running
  earlier in the same call) deletes the underlying Cloud Map service,
  the ECS service was left pointing at a registry ARN that no longer
  existed, which surfaces as registration failures on the next
  deployment. Fixed by tracking `needs_detach = cloud_map.is_none() &&
  has_registries` and calling `update_req.set_service_registries(Some(vec![]))`
  in that case.

- F2: the ingress-removal call site passed `None` for teardown's
  `known_registry_arn`, forcing a fallback to an account-wide
  `list_services()` scan matching only on `oab-{namespace}-{name}`. Two
  bots with the same namespace/name in different VPCs/environments
  sharing one account could collide there. Fixed by hoisting the
  `describe_services` call (previously done further down, purely for the
  service_active check) to the top of `apply_ecs`, before the
  ingress-removal branch, so the real registry ARN captured from the
  live ECS service can be threaded through instead of `None`. The
  service_active check later in the function now reuses this same
  response instead of issuing a second describe_services call.

No new tests added — this only changes AWS SDK request-building logic in
a code path (ingress removal) that isn't exercised by the existing unit
tests (which cover manifest/schema parsing, not live apply_ecs behavior).

Verified via `cargo fmt --check` (no new diffs beyond the changed lines)
and manual review of the full diff; `cargo build`/`clippy`/`test` could
not be run in this environment (no linker available), so CI is the
first real compile/test signal for this commit — flagged explicitly in
the accompanying PR comment.
@chaodu-agent

This comment has been minimized.

@chaodu-agent

Copy link
Copy Markdown
Collaborator Author

LGTM ✅ — declarative webhook ingress for oabctl, converged through multiple review rounds; current commit (70a920c) has no outstanding 🔴 or 🟡 findings and CI is green.

What This PR Does

Adds an optional spec.ingress block to the oab.dev/v2 manifest so oabctl apply provisions inbound webhook ingress (Telegram/LINE) in one shot — API Gateway HTTP API → VPC Link → Cloud Map → ECS task — replacing ~7 manual aws CLI steps described in the companion reference-architecture doc (#1274).

How It Works

  • apply reconciles Cloud Map (private DNS namespace + a per-bot SRV service record) before the ECS create/update-service call. An SRV record is required — a plain A record carries no port, and API Gateway's private integration needs one to resolve the target via DiscoverInstances.
  • Adding or fixing service discovery on an existing ECS service uses UpdateService directly (not delete-and-recreate) — ECS has supported mutating serviceRegistries on a running service since March 2022, confirmed against AWS's own API reference. This was a correction from an earlier round of this PR, which had assumed registries were create-only and documented a manual delete/recreate flow that itself turned out to rotate the webhook's public URL.
  • Removing ingress from a manifest tears down the per-bot Cloud Map service and HTTP API routes/integration/stage, and explicitly detaches the ECS service's serviceRegistries with an empty list — an earlier round of this PR omitted the field instead, which (per AWS's documented semantics) leaves the existing registry attached rather than clearing it, silently pointing the service at a Cloud Map service that had just been deleted.
  • The Cloud Map service targeted for teardown is resolved from the ECS service's own registry ARN rather than a name-only account-wide scan, avoiding a cross-VPC/environment collision risk where two isolated bots share the same namespace/name.
  • VPC Link and Cloud Map namespace are scoped per-VPC; the HTTP API is scoped per-bot, so multiple bots can safely share a webhook path (e.g. /webhook/telegram) or a VPC without colliding.
  • Backward compatible by construction — ingress is Option, defaults to None, and existing outbound-only (Discord) manifests provision zero new resources, covered by a dedicated unit test.

Findings (current commit)

# Severity Finding Location
1 🟢 SRV record + Cloud Map ARN as the API Gateway integration URI matches AWS's documented private-integration pattern exactly operator/src/ingress.rs
2 🟢 Correctly distinguishes "omitted serviceRegistries" (AWS treats as unchanged) from "explicit empty list" (detaches) when ingress is removed from a manifest operator/src/apply.rs
3 🟢 Cloud Map teardown resolves the exact service via the ECS service's registry ARN instead of a name-only scan, avoiding cross-VPC/environment collisions operator/src/ingress.rs
4 🟢 Cloud Map service deletion retries for ~25s to absorb ECS's async instance-deregistration lag rather than failing on first attempt operator/src/ingress.rs
5 🟢 HTTP API resource is preserved (only routes/integration/stage are cleared) when ingress is removed via apply, so the webhook URL survives if ingress is re-added later; full deletion only happens via oabctl delete, where there's no URL left to preserve operator/src/ingress.rs, operator/src/delete.rs
6 🟢 Good test coverage: schema parsing/defaults, validation failures, ECS-only runtime restriction, fleet template/override inheritance, backward compatibility operator/src/manifest.rs, operator/src/ingress.rs

No 🔴 or 🟡 findings remain open. CI (cargo build, clippy --all-targets -D warnings, cargo test) is green on 70a920c.

Review History Summary

This PR went through many review rounds before converging; each round surfaced and fixed one substantive issue rather than accumulating unresolved debt:

  1. Shared HTTP API across bots → cross-bot webhook path collisions → fixed with a per-bot API.
  2. Plain A-record Cloud Map wiring → 503/BadRequest against a live AWS account → fixed with an SRV record + service-ARN integration.
  3. VPC-agnostic shared VPC Link/namespace → breakage when bots span multiple VPCs → fixed by scoping both per-VPC.
  4. Documented delete-and-recreate flow for adding service discovery to an existing service → silently rotated the webhook's public URL, breaking any already-registered Telegram/LINE webhook → root-caused as a false assumption (UpdateService has supported serviceRegistries since March 2022) and fixed by removing the recreate path entirely.
  5. The resulting ingress-removal path (dropping spec.ingress from a manifest) had two gaps not exercised by the ingress-addition path: an omitted (vs. explicitly emptied) serviceRegistries field left a stale registry attached after its Cloud Map service was deleted, and the teardown call for that path fell back to an account-wide name-only Cloud Map scan with a cross-VPC collision risk. Both fixed in 70a920c by capturing the ECS service's real registry ARN up front and using it for both the explicit detach and the exact-match teardown lookup.
Baseline Check
  • Main branch has no ingress concept anywhere in the operator crate — this is fully net-new functionality.
  • Net-new value: the entire spec.ingress reconciliation path (Cloud Map, VPC Link, API Gateway, security-group rule) plus teardown/detach handling on removal — nothing pre-existing is duplicated.
What's Good (🟢)
  • Idempotent reconciliation throughout — every resource is looked up by name/ARN and reused, so repeated apply runs converge rather than duplicate.
  • README is thorough and self-critical: manifest schema, an IAM permission table for apply/delete callers, teardown/detach semantics, and explicit callouts for known edge cases (VPC Link subnet/security-group immutability after creation, stale-route pruning on path rename).
  • The unauthenticated-transport tradeoff (API Gateway endpoint is public) is explicitly documented alongside the actual app-layer mitigations in place (Telegram secret-token header + IP allowlist, LINE HMAC signature) rather than silently left unaddressed.
  • Multiple rounds of this review validated specific claims against AWS's official API documentation rather than accepting assumptions at face value — this is what caught both the recreate-path webhook URL bug and the registry-detach semantics bug before merge.

smallgun01 added a commit to smallgun01/openab that referenced this pull request Jul 2, 2026
Fixes based on chaodu-agent's live E2E finding in openabdev#1275:

- Cloud Map service must use SRV records (not A) — the port comes from SRV
- Integration URI must be the Cloud Map service ARN (not http:// URL)
- API Gateway HTTP API VPC_LINK rejects http:// URIs with:
  BadRequestException: integration uri should be a valid Cloud Map service ARN
- Capture SERVICE_ID from create-service for the service ARN lookup
- Also fix --task-definition your-bot:latest → your-bot (ECS family name)
- Update architecture diagram, path passthrough callout, and DNS record
  descriptions to match the corrected configuration
@thepagent thepagent merged commit c72c86a into main Jul 2, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants