Skip to content

Commit 838c220

Browse files
committed
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).
1 parent d40e7db commit 838c220

3 files changed

Lines changed: 67 additions & 22 deletions

File tree

operator/README.md

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -215,9 +215,12 @@ Discord bots are outbound-only and need no ingress. Webhook platforms (Telegram,
215215
LINE, ...) POST *into* the task, so they need a public HTTPS endpoint. Adding an
216216
optional `spec.ingress` block makes `oabctl apply` provision the cheapest
217217
AWS-native path in one shot — API Gateway HTTP API → VPC Link → Cloud Map → the
218-
task — instead of running ~7 manual `aws` commands. See
219-
[`docs/refarch/running-telegram-line-on-aws.md`](../docs/refarch/running-telegram-line-on-aws.md)
220-
(Option 1) for the architecture and cost breakdown.
218+
task — instead of running ~7 manual `aws` commands, replacing the manual steps
219+
implemented here. For a Kubernetes/Cloudflare-Tunnel alternative, see
220+
[`docs/refarch/telegram-cloudflare-tunnel.md`](../docs/refarch/telegram-cloudflare-tunnel.md).
221+
A dedicated AWS-native refarch doc covering this path in depth is tracked in
222+
[#1274](https://github.com/openabdev/openab/pull/1274); once merged it will be
223+
linked here.
221224

222225
```yaml
223226
spec:

operator/src/ingress.rs

Lines changed: 58 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,11 @@
11
//! Ingress reconciliation for webhook-based platforms (Telegram, LINE, ...).
22
//!
3-
//! Implements Option 1 of `docs/refarch/running-telegram-line-on-aws.md`:
4-
//! API Gateway HTTP API → VPC Link → Cloud Map → ECS Fargate task on `:port`.
3+
//! Implements the API Gateway HTTP API → VPC Link → Cloud Map → ECS Fargate
4+
//! path for inbound webhook ingress (Telegram, LINE, ...), replacing ~7 manual
5+
//! `aws apigatewayv2`/`servicediscovery`/`ecs` CLI steps. See
6+
//! `operator/README.md` ("Ingress — inbound webhooks") for the manifest schema
7+
//! and operational notes; a dedicated AWS reference architecture doc for this
8+
//! path is tracked in openabdev/openab#1274.
59
//!
610
//! All operations are idempotent — resources are looked up by name and reused,
711
//! so repeated `oabctl apply` runs converge instead of duplicating. The shared
@@ -180,11 +184,31 @@ pub async fn teardown(config: &aws_config::SdkConfig, namespace: &str, name: &st
180184
}
181185
}
182186
if let Some(service_id) = service_id {
183-
match sd.delete_service().id(&service_id).send().await {
184-
Ok(_) => eprintln!(" ✓ Deleted Cloud Map service: {service_name}"),
185-
Err(e) => eprintln!(
186-
" ⚠ Cloud Map service '{service_name}' not deleted — it may still have\n registered instances; retry once the ECS tasks have fully drained ({e})"
187-
),
187+
// ECS deregisters the task's Cloud Map instance asynchronously when a
188+
// service scales to 0 / is deleted, so `delete_service` can fail with
189+
// "still has registered instances" for a short window even though the
190+
// task is already gone. Retry briefly instead of giving up on the
191+
// first attempt — this is the common case, not an edge case.
192+
let mut last_err = None;
193+
let mut deleted = false;
194+
for attempt in 0..6 {
195+
if attempt > 0 {
196+
tokio::time::sleep(std::time::Duration::from_secs(5)).await;
197+
}
198+
match sd.delete_service().id(&service_id).send().await {
199+
Ok(_) => {
200+
eprintln!(" ✓ Deleted Cloud Map service: {service_name}");
201+
deleted = true;
202+
break;
203+
}
204+
Err(e) => last_err = Some(e),
205+
}
206+
}
207+
if !deleted {
208+
eprintln!(
209+
" ⚠ Cloud Map service '{service_name}' not deleted after retrying — it still\n has registered instances. It will be orphaned until manually removed:\n aws servicediscovery delete-service --id {service_id}\n ({})",
210+
last_err.map(|e| e.to_string()).unwrap_or_default()
211+
);
188212
}
189213
}
190214

@@ -399,35 +423,52 @@ async fn ensure_vpc_link(
399423
use aws_sdk_apigatewayv2::types::VpcLinkStatus;
400424
let link_name = vpc_link_name(vpc_id);
401425

402-
// Reuse an existing, non-failed VPC Link with our per-VPC name.
403-
let mut found: Option<(String, Option<VpcLinkStatus>)> = None;
426+
// Reuse an existing, non-failed VPC Link with our per-VPC name. VPC Link
427+
// names are NOT unique to the API — if two `oabctl apply` invocations race
428+
// to create the same-named link in a brand-new VPC (e.g. a fleet's agents
429+
// applied via separate concurrent processes), AWS will happily create two.
430+
// We can't prevent that race across processes, but we can make reuse
431+
// deterministic afterward: collect all matches and always pick the one
432+
// with the lexicographically smallest ID (stable across repeated calls,
433+
// regardless of list ordering), warning if more than one exists so an
434+
// operator can clean up the duplicate.
435+
let mut candidates: Vec<(String, Option<VpcLinkStatus>)> = Vec::new();
404436
let mut next: Option<String> = None;
405-
'outer: loop {
437+
loop {
406438
let mut req = api.get_vpc_links();
407439
if let Some(t) = &next {
408440
req = req.next_token(t);
409441
}
410442
let resp = req.send().await.context("failed to list VPC Links")?;
411443
for link in resp.items() {
412-
if link.name() == Some(link_name.as_str()) {
413-
if matches!(
444+
if link.name() == Some(link_name.as_str())
445+
&& !matches!(
414446
link.vpc_link_status(),
415447
Some(VpcLinkStatus::Failed) | Some(VpcLinkStatus::Deleting)
416-
) {
417-
continue;
418-
}
419-
found = Some((
448+
)
449+
{
450+
candidates.push((
420451
link.vpc_link_id().unwrap_or_default().to_string(),
421452
link.vpc_link_status().cloned(),
422453
));
423-
break 'outer;
424454
}
425455
}
426456
match resp.next_token() {
427457
Some(t) => next = Some(t.to_string()),
428458
None => break,
429459
}
430460
}
461+
candidates.sort_by(|(a, _), (b, _)| a.cmp(b));
462+
if candidates.len() > 1 {
463+
eprintln!(
464+
" ⚠ Found {} VPC Links named '{link_name}' (a race between concurrent\n `apply` runs can create duplicates — AWS does not enforce name\n uniqueness). Using the lexicographically first; consider deleting the extras:",
465+
candidates.len()
466+
);
467+
for (id, _) in &candidates[1..] {
468+
eprintln!(" aws apigatewayv2 delete-vpc-link --vpc-link-id {id}");
469+
}
470+
}
471+
let found = candidates.into_iter().next();
431472

432473
let link_id = if let Some((id, _status)) = found {
433474
eprintln!(" ✓ VPC Link exists: {link_name} ({id})");

operator/src/manifest.rs

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -167,8 +167,9 @@ pub struct Spec {
167167
/// Inbound HTTPS ingress for webhook-based platforms (Telegram, LINE, ...).
168168
///
169169
/// Provisions the cheapest AWS-native path: API Gateway HTTP API → VPC Link →
170-
/// Cloud Map → the ECS task on `containerPort`. See
171-
/// `docs/refarch/running-telegram-line-on-aws.md` (Option 1).
170+
/// Cloud Map → the ECS task on `containerPort`. See `operator/README.md`
171+
/// ("Ingress — inbound webhooks") for the manifest schema and operational
172+
/// notes.
172173
#[derive(Debug, Deserialize, Serialize, Clone)]
173174
#[serde(rename_all = "camelCase")]
174175
pub struct Ingress {

0 commit comments

Comments
 (0)