Skip to content

Commit 6f92a2e

Browse files
committed
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.
1 parent 93c995b commit 6f92a2e

3 files changed

Lines changed: 58 additions & 87 deletions

File tree

operator/README.md

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -244,7 +244,7 @@ On `apply` this reconciles (idempotently, reused by name):
244244
1. **Cloud Map** private DNS namespace + a per-service A record
245245
2. **ECS service registry** wiring (attached at service creation)
246246
3. **VPC Link** (shared `oab-vpc-link`), waits until `AVAILABLE`
247-
4. **API Gateway HTTP API** (`oab-webhook`) + `HTTP_PROXY` integration over the VPC Link
247+
4. **API Gateway HTTP API** (`oab-webhook-<ns>-<name>`, one per bot) + `HTTP_PROXY` integration over the VPC Link
248248
5. One **route** per path + a `prod` auto-deploy **stage**
249249
6. A self-referencing **security-group** inbound rule on `containerPort`
250250

@@ -272,11 +272,16 @@ LINE console:
272272
> reminder when it reuses an existing link.
273273
>
274274
> **Teardown:** `oabctl delete oabservice <name>` removes the bot's per-bot ingress
275-
> resources — its Cloud Map service and its API Gateway routes + integration — on a
276-
> best-effort basis (it never blocks service deletion). The **shared** `oab-vpc-link`,
277-
> `oab-webhook` HTTP API, and the security-group inbound rule are intentionally left
278-
> in place for other bots. If the Cloud Map service still has registered instances
279-
> (tasks not yet drained), delete prints a note to retry.
275+
> resources — its Cloud Map service and its own HTTP API (`oab-webhook-<ns>-<name>`,
276+
> which cascades its routes/integration/stage) — on a best-effort basis (it never
277+
> blocks service deletion). The **shared** `oab-vpc-link` and the security-group
278+
> inbound rule are intentionally left in place for other bots. If the Cloud Map
279+
> service still has registered instances (tasks not yet drained), delete prints a
280+
> note to retry.
281+
>
282+
> **Per-bot API, no path collisions:** each ingress bot gets its own HTTP API, so
283+
> two bots can both use `/webhook/telegram` without clashing — each has a distinct
284+
> `{api-id}` endpoint URL.
280285

281286
### OABFleet — batch deploy
282287

operator/src/apply.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -324,6 +324,8 @@ async fn apply_ecs(
324324
eprintln!(" 🌐 Reconciling ingress (VPC Link + API Gateway)...");
325325
let urls = crate::ingress::ensure_gateway(
326326
config,
327+
&m.metadata.namespace,
328+
&m.metadata.name,
327329
ingress,
328330
&ecs_rt.networking.subnets,
329331
&ecs_rt.networking.security_groups,

operator/src/ingress.rs

Lines changed: 45 additions & 81 deletions
Original file line numberDiff line numberDiff line change
@@ -19,9 +19,14 @@ use aws_sdk_apigatewayv2::types::{ConnectionType, IntegrationType, ProtocolType}
1919
use aws_sdk_servicediscovery::types::{DnsConfig, DnsRecord, RecordType};
2020

2121
const VPC_LINK_NAME: &str = "oab-vpc-link";
22-
const API_NAME: &str = "oab-webhook";
2322
const STAGE_NAME: &str = "prod";
2423

24+
/// Per-bot HTTP API name. Each ingress bot gets its own API so webhook paths
25+
/// (e.g. `/webhook/telegram`) can never collide between bots on a shared API.
26+
fn api_name(namespace: &str, name: &str) -> String {
27+
format!("oab-webhook-{namespace}-{name}")
28+
}
29+
2530
/// Result of Cloud Map reconciliation, consumed when creating the ECS service.
2631
pub struct CloudMapResult {
2732
/// Cloud Map service ARN — used as the ECS service registry ARN.
@@ -68,22 +73,25 @@ pub async fn ensure_cloud_map(
6873
/// security-group inbound rule. Returns the public webhook URLs (one per path).
6974
pub async fn ensure_gateway(
7075
config: &aws_config::SdkConfig,
76+
namespace: &str,
77+
name: &str,
7178
ingress: &Ingress,
7279
subnets: &[String],
7380
security_groups: &[String],
7481
dns_name: &str,
7582
) -> Result<Vec<String>> {
7683
let api = aws_sdk_apigatewayv2::Client::new(config);
7784
let ec2 = aws_sdk_ec2::Client::new(config);
85+
let api_name = api_name(namespace, name);
7886

7987
// ── Security group inbound rule (self-referencing on the container port) ─
8088
ensure_sg_ingress(&ec2, security_groups, ingress.container_port).await?;
8189

82-
// ── VPC Link (shared, waits for AVAILABLE) ─────────────────────────────
90+
// ── VPC Link (shared across all bots, waits for AVAILABLE) ─────────────
8391
let vpc_link_id = ensure_vpc_link(&api, subnets, security_groups).await?;
8492

85-
// ── HTTP API (shared) ──────────────────────────────────────────────────
86-
let (api_id, api_endpoint) = ensure_api(&api).await?;
93+
// ── HTTP API (one per bot — avoids cross-bot path collisions) ──────────
94+
let (api_id, api_endpoint) = ensure_api(&api, &api_name).await?;
8795

8896
// ── Integration: VPC Link → Cloud Map DNS name on the container port ────
8997
let integration_uri = integration_uri(dns_name, ingress.container_port);
@@ -121,78 +129,23 @@ fn webhook_urls(api_endpoint: &str, paths: &[String]) -> Vec<String> {
121129
}
122130

123131
/// Best-effort teardown of the *per-bot* ingress resources for `namespace/name`:
124-
/// the API Gateway routes + integration for this bot and its Cloud Map service.
132+
/// the bot's own HTTP API (`oab-webhook-<ns>-<name>`, which cascades its routes,
133+
/// integration and stage) and its Cloud Map service.
125134
///
126-
/// The shared resources (VPC Link `oab-vpc-link`, HTTP API `oab-webhook`, and the
127-
/// security-group inbound rule) are intentionally left in place since other bots
128-
/// may still use them. Safe to call for bots that never had ingress — it simply
129-
/// finds nothing and returns. Errors are logged, not propagated, so teardown
130-
/// never blocks service deletion.
131-
pub async fn teardown(
132-
config: &aws_config::SdkConfig,
133-
namespace: &str,
134-
name: &str,
135-
) -> Result<()> {
135+
/// The shared resources (the `oab-vpc-link` VPC Link and the security-group
136+
/// inbound rule) are intentionally left in place since other bots may still use
137+
/// them. Safe to call for bots that never had ingress — it simply finds nothing
138+
/// and returns. Errors are logged, not propagated, so teardown never blocks
139+
/// service deletion.
140+
pub async fn teardown(config: &aws_config::SdkConfig, namespace: &str, name: &str) -> Result<()> {
136141
let service_name = format!("oab-{namespace}-{name}");
137142
let api = aws_sdk_apigatewayv2::Client::new(config);
138143

139-
// ── API Gateway: routes + integration for this bot (matched by DNS host) ─
140-
if let Some((api_id, _)) = find_api(&api).await? {
141-
let mut integration_id: Option<String> = None;
142-
let mut next: Option<String> = None;
143-
'find: loop {
144-
let mut req = api.get_integrations().api_id(&api_id);
145-
if let Some(t) = &next {
146-
req = req.next_token(t);
147-
}
148-
let resp = req.send().await.context("failed to list integrations")?;
149-
for i in resp.items() {
150-
if i
151-
.integration_uri()
152-
.is_some_and(|u| u.contains(&format!("//{service_name}.")))
153-
{
154-
integration_id = i.integration_id().map(|s| s.to_string());
155-
break 'find;
156-
}
157-
}
158-
match resp.next_token() {
159-
Some(t) => next = Some(t.to_string()),
160-
None => break,
161-
}
162-
}
163-
164-
if let Some(integration_id) = integration_id {
165-
let target = format!("integrations/{integration_id}");
166-
let mut route_ids = Vec::new();
167-
let mut next: Option<String> = None;
168-
loop {
169-
let mut req = api.get_routes().api_id(&api_id);
170-
if let Some(t) = &next {
171-
req = req.next_token(t);
172-
}
173-
let resp = req.send().await.context("failed to list routes")?;
174-
for r in resp.items() {
175-
if r.target() == Some(target.as_str()) {
176-
if let Some(rid) = r.route_id() {
177-
route_ids.push(rid.to_string());
178-
}
179-
}
180-
}
181-
match resp.next_token() {
182-
Some(t) => next = Some(t.to_string()),
183-
None => break,
184-
}
185-
}
186-
for rid in route_ids {
187-
api.delete_route().api_id(&api_id).route_id(&rid).send().await.ok();
188-
}
189-
api.delete_integration()
190-
.api_id(&api_id)
191-
.integration_id(&integration_id)
192-
.send()
193-
.await
194-
.ok();
195-
eprintln!(" ✓ Removed API Gateway routes + integration for {name}");
144+
// ── API Gateway: delete the whole per-bot API (cascades routes/integration/stage) ─
145+
if let Some((api_id, _)) = find_api(&api, &api_name(namespace, name)).await? {
146+
match api.delete_api().api_id(&api_id).send().await {
147+
Ok(_) => eprintln!(" ✓ Deleted HTTP API: {}", api_name(namespace, name)),
148+
Err(e) => eprintln!(" ⚠ Failed to delete HTTP API {api_id}: {e}"),
196149
}
197150
}
198151

@@ -418,7 +371,6 @@ async fn ensure_vpc_link(
418371
) -> Result<String> {
419372
use aws_sdk_apigatewayv2::types::VpcLinkStatus;
420373

421-
// Reuse an existing, non-failed VPC Link with our name.
422374
// Reuse an existing, non-failed VPC Link with our name.
423375
let mut found: Option<(String, Option<VpcLinkStatus>)> = None;
424376
let mut next: Option<String> = None;
@@ -497,15 +449,18 @@ async fn ensure_vpc_link(
497449

498450
// ─── HTTP API ───────────────────────────────────────────────────────────────
499451

500-
async fn ensure_api(api: &aws_sdk_apigatewayv2::Client) -> Result<(String, String)> {
501-
if let Some((id, endpoint)) = find_api(api).await? {
502-
eprintln!(" ✓ HTTP API exists: {API_NAME} ({id})");
452+
async fn ensure_api(
453+
api: &aws_sdk_apigatewayv2::Client,
454+
api_name: &str,
455+
) -> Result<(String, String)> {
456+
if let Some((id, endpoint)) = find_api(api, api_name).await? {
457+
eprintln!(" ✓ HTTP API exists: {api_name} ({id})");
503458
return Ok((id, endpoint));
504459
}
505-
eprintln!(" ⊕ Creating HTTP API: {API_NAME}");
460+
eprintln!(" ⊕ Creating HTTP API: {api_name}");
506461
let out = api
507462
.create_api()
508-
.name(API_NAME)
463+
.name(api_name)
509464
.protocol_type(ProtocolType::Http)
510465
.send()
511466
.await
@@ -515,8 +470,11 @@ async fn ensure_api(api: &aws_sdk_apigatewayv2::Client) -> Result<(String, Strin
515470
Ok((id, endpoint))
516471
}
517472

518-
/// Find the shared `oab-webhook` HTTP API, returning `(api_id, api_endpoint)`.
519-
async fn find_api(api: &aws_sdk_apigatewayv2::Client) -> Result<Option<(String, String)>> {
473+
/// Find an HTTP API by name, returning `(api_id, api_endpoint)`.
474+
async fn find_api(
475+
api: &aws_sdk_apigatewayv2::Client,
476+
api_name: &str,
477+
) -> Result<Option<(String, String)>> {
520478
// apigatewayv2 has no smithy paginator for GetApis; page manually.
521479
let mut next: Option<String> = None;
522480
loop {
@@ -526,7 +484,7 @@ async fn find_api(api: &aws_sdk_apigatewayv2::Client) -> Result<Option<(String,
526484
}
527485
let resp = req.send().await.context("failed to list APIs")?;
528486
for a in resp.items() {
529-
if a.name() == Some(API_NAME) {
487+
if a.name() == Some(api_name) {
530488
let id = a.api_id().context("api missing id")?.to_string();
531489
let endpoint = a.api_endpoint().unwrap_or_default().to_string();
532490
return Ok(Some((id, endpoint)));
@@ -676,6 +634,12 @@ mod tests {
676634
assert_eq!(route_key("/webhook/line"), "POST /webhook/line");
677635
}
678636

637+
#[test]
638+
fn api_name_is_per_bot() {
639+
assert_eq!(api_name("prod", "mybot"), "oab-webhook-prod-mybot");
640+
assert_ne!(api_name("prod", "a"), api_name("prod", "b"));
641+
}
642+
679643
#[test]
680644
fn webhook_urls_join_endpoint_stage_and_path() {
681645
let paths = vec![

0 commit comments

Comments
 (0)