Commit c72c86a
authored
feat(operator): oabctl ingress for Telegram/LINE (API Gateway + VPC Link + Cloud Map) (#1275)
* feat(operator): add API Gateway ingress for webhook platforms
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
* build(operator): declare standalone [workspace] so CI cargo check works
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).
* test(operator): unit-test ingress URL/route helpers; document shared 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).
* fix(operator): correct recreate hint, add ingress teardown on delete, 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.
* fix(operator): per-bot HTTP API to prevent cross-bot webhook path collisions
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.
* fix(operator): match typed EC2 error code for duplicate SG rule
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).
* fix(operator): use Cloud Map SRV + service-ARN integration (fixes live 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).
* feat(operator): consolidated apply summary for services needing ingress recreation
Review F2: the 'service exists without service discovery' warning was a
mid-run eprintln that's easy to miss in non-interactive CI, so a partial
apply could look like a clean success. apply_ecs now returns whether the
service needs recreation; apply::run collects these and prints a single
consolidated ⚠ summary block (with the exact recreate commands) after the
'N service(s) applied' line.
build + clippy -D warnings + test (11 passed).
* fix(operator): scope VPC Link/namespace per-VPC; teardown on ingress removal; prune stale routes
Round-7 review:
- F1 (critical): VPC Link was matched by a hardcoded name ('oab-vpc-link')
with no VPC check. A VPC Link's ENIs live in one VPC and cannot route to
another, so a second bot in a *different* VPC would silently reuse the
first VPC's link and get unreachable integrations. Fixed by naming the
link 'oab-vpc-link-<vpc-id>', scoped per-VPC.
- F2: same collision class for the Cloud Map namespace — matched by
configured name only. Fixed by scoping the actual namespace name to
'<cloudMapNamespace>-<vpc-id>' (the DnsConfig VPC association makes
namespace-per-VPC the correct AWS-native mental model anyway; the
private DNS only resolves inside that VPC).
- F3: apply only ever added ingress resources — editing a manifest to
remove spec.ingress orphaned the per-bot HTTP API + Cloud Map service.
apply now detects 'had ingress before, doesn't now' by comparing
against the previously-stored S3 manifest and calls the same
best-effort ingress::teardown used by .
- F4: ensure_route only ever added routes; renaming/removing a webhook
path left a dead route on the bot's API forever. Added
prune_stale_routes, which deletes any route on the bot's API whose key
isn't in the current ingress.paths after ensuring the desired ones.
Added 3 unit tests (vpc_link_name, vpc_scoped_namespace) — 13 total.
Updated README + module doc to describe per-VPC scoping and the new
apply-time teardown/pruning behavior.
Verified: build, clippy --all-targets -D warnings, cargo test (13 passed).
* fix(operator): dead doc link, VPC Link race hardening, Cloud Map teardown retry
Round-8 review:
- F1 (blocking): README and code comments linked to
docs/refarch/running-telegram-line-on-aws.md, which does not exist on
main or this branch (its PR #1274 is still open). Repointed the README
to the existing docs/refarch/telegram-cloudflare-tunnel.md as an
interim cross-reference with a note that a dedicated AWS-native doc is
tracked in #1274; ingress.rs/manifest.rs doc comments now point at
operator/README.md instead of the nonexistent file.
- F2: VPC Link names are not unique to the apigatewayv2 API (confirmed:
AWS does not error on a duplicate name), so two 'oabctl apply'
processes racing to create the same per-VPC link in a brand-new VPC
could end up with two links. Within a single apply run this can't
happen (manifests are processed sequentially), but the reconciler
hardens against the cross-process race anyway: ensure_vpc_link now
collects ALL matching links, deterministically picks the
lexicographically-first ID (stable regardless of list ordering) rather
than an arbitrary one, and warns with cleanup commands if more than one
exists.
- F3: Cloud Map service teardown gave up on the first 'still has
registered instances' error, permanently orphaning the service if the
caller didn't manually retry. ECS deregisters the instance
asynchronously on scale-to-0/delete, so this is usually just a timing
window, not a real conflict — teardown now retries delete_service up to
6 times over ~25s before falling back to a warning with the exact
manual cleanup command.
Verified: build, clippy --all-targets -D warnings, cargo test (13 passed).
* fix(operator): preserve webhook URL on recreate; exact Cloud Map targeting; 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).
* fix(operator): attach/fix service discovery via UpdateService, no recreate 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).
* fix(operator): detach registry + exact-ARN teardown on ingress removal
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.
---------
Co-authored-by: chaodu-agent <chaodu-agent@users.noreply.github.com>1 parent 709d03f commit c72c86a
9 files changed
Lines changed: 2585 additions & 44 deletions
File tree
- operator
- schema
- src
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
4 | 4 | | |
5 | 5 | | |
6 | 6 | | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
7 | 12 | | |
8 | 13 | | |
9 | 14 | | |
| |||
18 | 23 | | |
19 | 24 | | |
20 | 25 | | |
| 26 | + | |
| 27 | + | |
21 | 28 | | |
22 | 29 | | |
23 | 30 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
209 | 209 | | |
210 | 210 | | |
211 | 211 | | |
| 212 | + | |
| 213 | + | |
| 214 | + | |
| 215 | + | |
| 216 | + | |
| 217 | + | |
| 218 | + | |
| 219 | + | |
| 220 | + | |
| 221 | + | |
| 222 | + | |
| 223 | + | |
| 224 | + | |
| 225 | + | |
| 226 | + | |
| 227 | + | |
| 228 | + | |
| 229 | + | |
| 230 | + | |
| 231 | + | |
| 232 | + | |
| 233 | + | |
| 234 | + | |
| 235 | + | |
| 236 | + | |
| 237 | + | |
| 238 | + | |
| 239 | + | |
| 240 | + | |
| 241 | + | |
| 242 | + | |
| 243 | + | |
| 244 | + | |
| 245 | + | |
| 246 | + | |
| 247 | + | |
| 248 | + | |
| 249 | + | |
| 250 | + | |
| 251 | + | |
| 252 | + | |
| 253 | + | |
| 254 | + | |
| 255 | + | |
| 256 | + | |
| 257 | + | |
| 258 | + | |
| 259 | + | |
| 260 | + | |
| 261 | + | |
| 262 | + | |
| 263 | + | |
| 264 | + | |
| 265 | + | |
| 266 | + | |
| 267 | + | |
| 268 | + | |
| 269 | + | |
| 270 | + | |
| 271 | + | |
| 272 | + | |
| 273 | + | |
| 274 | + | |
| 275 | + | |
| 276 | + | |
| 277 | + | |
| 278 | + | |
| 279 | + | |
| 280 | + | |
| 281 | + | |
| 282 | + | |
| 283 | + | |
| 284 | + | |
| 285 | + | |
| 286 | + | |
| 287 | + | |
| 288 | + | |
| 289 | + | |
| 290 | + | |
| 291 | + | |
| 292 | + | |
| 293 | + | |
| 294 | + | |
| 295 | + | |
| 296 | + | |
| 297 | + | |
| 298 | + | |
| 299 | + | |
| 300 | + | |
| 301 | + | |
| 302 | + | |
| 303 | + | |
| 304 | + | |
| 305 | + | |
| 306 | + | |
| 307 | + | |
| 308 | + | |
| 309 | + | |
| 310 | + | |
| 311 | + | |
| 312 | + | |
| 313 | + | |
| 314 | + | |
| 315 | + | |
212 | 316 | | |
213 | 317 | | |
214 | 318 | | |
| |||
234 | 338 | | |
235 | 339 | | |
236 | 340 | | |
237 | | - | |
| 341 | + | |
238 | 342 | | |
239 | 343 | | |
240 | 344 | | |
| |||
328 | 432 | | |
329 | 433 | | |
330 | 434 | | |
| 435 | + | |
| 436 | + | |
| 437 | + | |
| 438 | + | |
| 439 | + | |
| 440 | + | |
| 441 | + | |
| 442 | + | |
| 443 | + | |
| 444 | + | |
| 445 | + | |
| 446 | + | |
| 447 | + | |
| 448 | + | |
| 449 | + | |
| 450 | + | |
| 451 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
58 | 58 | | |
59 | 59 | | |
60 | 60 | | |
61 | | - | |
| 61 | + | |
| 62 | + | |
62 | 63 | | |
63 | 64 | | |
64 | 65 | | |
| |||
83 | 84 | | |
84 | 85 | | |
85 | 86 | | |
86 | | - | |
| 87 | + | |
| 88 | + | |
87 | 89 | | |
88 | 90 | | |
89 | 91 | | |
| |||
96 | 98 | | |
97 | 99 | | |
98 | 100 | | |
99 | | - | |
| 101 | + | |
| 102 | + | |
100 | 103 | | |
101 | 104 | | |
102 | 105 | | |
| |||
109 | 112 | | |
110 | 113 | | |
111 | 114 | | |
| 115 | + | |
| 116 | + | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
112 | 131 | | |
113 | 132 | | |
114 | 133 | | |
| |||
0 commit comments