From 7a8cdbe212d1d26624c11f9d44f004ffbc901ddb Mon Sep 17 00:00:00 2001 From: chaodu-agent <274062505+chaodu-agent@users.noreply.github.com> Date: Thu, 16 Jul 2026 00:12:50 -0400 Subject: [PATCH 01/20] feat(oabctl): programmatic delete API with explicit target contract Designs the delete contract deferred by the lib-split review, symmetric with apply_manifests: - delete_services(&SdkConfig, &[DeleteTarget], &DeleteOptions) -> Result - DeleteOptions requires an explicit cluster and shares apply's bucket resolution chain (explicit -> OAB_CONTROL_PLANE_BUCKET -> account) so delete always cleans the bucket apply wrote to; never reads CLI config - Contract guards before any AWS call: non-empty target set, non-empty cluster, non-blank namespace/name (regression-tested without AWS) - Typed DeleteError {Validation, Target, Teardown} preserving the report of targets completed before a failure; teardown remains resumable - Best-effort ingress skips surface in DeletedService.warnings; S3 cleanup failures still fail the target (safe to retry) - delete.rs output now routes through the shared progress gate: CLI behavior unchanged, programmatic calls are silent - CLI paths (delete , delete -f) unchanged --- operator/src/apply.rs | 9 ++ operator/src/delete.rs | 307 ++++++++++++++++++++++++++++++++++++++++- operator/src/lib.rs | 22 ++- 3 files changed, 327 insertions(+), 11 deletions(-) diff --git a/operator/src/apply.rs b/operator/src/apply.rs index 25e180604..ad5534a98 100644 --- a/operator/src/apply.rs +++ b/operator/src/apply.rs @@ -21,6 +21,15 @@ pub(crate) fn progress_enabled() -> bool { .unwrap_or(true) } +/// Run `future` with human-readable progress output suppressed (library +/// callers). Shared by the programmatic apply and delete entry points. +pub(crate) async fn with_progress_suppressed(future: F) -> F::Output +where + F: std::future::Future, +{ + PROGRESS_ENABLED.scope(false, future).await +} + macro_rules! println { ($($arg:tt)*) => {{ if progress_enabled() { std::println!($($arg)*); } }}; } diff --git a/operator/src/delete.rs b/operator/src/delete.rs index 953e863dd..66d17eec7 100644 --- a/operator/src/delete.rs +++ b/operator/src/delete.rs @@ -1,7 +1,251 @@ use anyhow::{Context, Result}; use aws_sdk_ecs::error::ProvideErrorMetadata; +use std::fmt; use std::path::Path; +// Route all human-readable progress through the same task-local gate as +// apply: CLI callers keep today's output, programmatic callers are silent. +macro_rules! println { + ($($arg:tt)*) => {{ if crate::apply::progress_enabled() { std::println!($($arg)*); } }}; +} +macro_rules! eprintln { + ($($arg:tt)*) => {{ if crate::apply::progress_enabled() { std::eprintln!($($arg)*); } }}; +} +macro_rules! eprint { + ($($arg:tt)*) => {{ if crate::apply::progress_enabled() { std::eprint!($($arg)*); } }}; +} + +/// Identity of a service targeted for deletion. +/// +/// Deletion does not require the original manifest: the control-plane copy +/// under `manifests/{namespace}/{name}.yaml` (and every other resource) is +/// addressed by `namespace` + `name` alone. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct DeleteTarget { + pub namespace: String, + pub name: String, +} + +impl DeleteTarget { + pub fn new(namespace: impl Into, name: impl Into) -> Self { + Self { + namespace: namespace.into(), + name: name.into(), + } + } + + /// ECS service name derived from the target (`oab-{namespace}-{name}`). + pub fn ecs_service_name(&self) -> String { + format!("oab-{}-{}", self.namespace, self.name) + } +} + +/// Target options for [`delete_services`]. Mirrors +/// [`ApplyOptions`](crate::apply::ApplyOptions): the cluster is required and +/// the library never reads `~/.oabctl/config.toml`. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct DeleteOptions { + /// ECS cluster name or ARN the services were deployed into. + pub cluster: String, + /// Optional control-plane bucket override. When absent, resolution uses + /// `OAB_CONTROL_PLANE_BUCKET`, then `oab-control-plane-{account}` from + /// the caller's AWS identity — the same chain as apply, so delete always + /// cleans the bucket apply wrote to. + pub control_plane_bucket: Option, +} + +impl DeleteOptions { + pub fn new(cluster: impl Into) -> Self { + Self { + cluster: cluster.into(), + control_plane_bucket: None, + } + } + + pub fn with_control_plane_bucket(mut self, bucket: impl Into) -> Self { + self.control_plane_bucket = Some(bucket.into()); + self + } +} + +/// Teardown outcome for one service. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct DeletedService { + pub namespace: String, + pub name: String, + pub ecs_service_name: String, + /// Best-effort steps that were skipped (e.g. ingress teardown pieces + /// already gone). The core teardown (ECS service, S3 manifest and + /// artifacts) either completed or the whole target failed. + pub warnings: Vec, +} + +/// Structured result of a successful (or partially completed) delete. +#[derive(Debug, Clone, Default, PartialEq, Eq)] +pub struct DeleteReport { + pub services: Vec, +} + +/// High-level phase in which delete failed. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum DeleteErrorKind { + /// Contract violation caught before any AWS call. + Validation, + /// The deployment target (bucket resolution) could not be established. + Target, + /// Teardown of a specific service failed. Cleanup is resumable: calling + /// [`delete_services`] again with the same target continues from the + /// remaining resources. + Teardown, +} + +/// Structured delete failure. Teardown failures identify the failed service +/// and retain the report for all services completed before it. +#[derive(Debug)] +pub struct DeleteError { + pub kind: DeleteErrorKind, + pub failed_service: Option, + pub completed: DeleteReport, + source: anyhow::Error, +} + +impl DeleteError { + fn validation(source: impl Into) -> Self { + Self { + kind: DeleteErrorKind::Validation, + failed_service: None, + completed: DeleteReport::default(), + source: source.into(), + } + } + + fn target(source: impl Into) -> Self { + Self { + kind: DeleteErrorKind::Target, + failed_service: None, + completed: DeleteReport::default(), + source: source.into(), + } + } + + fn teardown( + failed_service: DeleteTarget, + completed: DeleteReport, + source: impl Into, + ) -> Self { + Self { + kind: DeleteErrorKind::Teardown, + failed_service: Some(failed_service), + completed, + source: source.into(), + } + } + + pub fn source_error(&self) -> &anyhow::Error { + &self.source + } +} + +impl fmt::Display for DeleteError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match &self.failed_service { + Some(service) => write!( + f, + "delete {:?} error for {}/{}: {}", + self.kind, service.namespace, service.name, self.source + ), + None => write!(f, "delete {:?} error: {}", self.kind, self.source), + } + } +} + +impl std::error::Error for DeleteError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + Some(self.source.as_ref()) + } +} + +fn validate_delete_request( + targets: &[DeleteTarget], + cluster: &str, +) -> std::result::Result<(), DeleteError> { + if targets.is_empty() { + return Err(DeleteError::validation(anyhow::anyhow!( + "no targets to delete (empty target set)" + ))); + } + if cluster.trim().is_empty() { + return Err(DeleteError::validation(anyhow::anyhow!( + "DeleteOptions.cluster must not be empty" + ))); + } + for target in targets { + if target.namespace.trim().is_empty() || target.name.trim().is_empty() { + return Err(DeleteError::validation(anyhow::anyhow!( + "delete target namespace and name must not be empty (got '{}'/'{}')", + target.namespace, + target.name + ))); + } + } + Ok(()) +} + +/// Tear down OAB services programmatically, without reading CLI home +/// configuration or writing progress to process-global stdout/stderr. +/// +/// Contract (enforced before any AWS call): +/// - the target set must be non-empty +/// - [`DeleteOptions::cluster`] must be a non-empty cluster name +/// - every target's `namespace`/`name` must be non-empty +/// +/// Teardown per target is resumable: an `ACTIVE` service is scaled down and +/// deleted, a `DRAINING` service resumes its drain wait, and an absent one +/// proceeds straight to ingress/S3 cleanup. Best-effort ingress steps are +/// reported through [`DeletedService::warnings`]; S3 cleanup failures fail +/// the target (safe to retry). +pub async fn delete_services( + aws_config: &aws_config::SdkConfig, + targets: &[DeleteTarget], + opts: &DeleteOptions, +) -> std::result::Result { + crate::apply::with_progress_suppressed(async { + validate_delete_request(targets, &opts.cluster)?; + let bucket = crate::control_plane::resolve_bucket( + aws_config, + opts.control_plane_bucket.as_deref(), + ) + .await + .map_err(DeleteError::target)?; + + let mut report = DeleteReport::default(); + for target in targets { + match run_with_bucket( + aws_config, + "oabservice", + &target.name, + &opts.cluster, + &target.namespace, + &bucket, + ) + .await + { + Ok(warnings) => report.services.push(DeletedService { + namespace: target.namespace.clone(), + name: target.name.clone(), + ecs_service_name: target.ecs_service_name(), + warnings, + }), + Err(error) => { + return Err(DeleteError::teardown(target.clone(), report, error)); + } + } + } + Ok(report) + }) + .await +} + #[derive(Debug, Clone, Copy, PartialEq, Eq)] enum EcsDeletePhase { Delete, @@ -81,7 +325,9 @@ pub(crate) async fn run( let bucket = crate::control_plane::resolve_bucket(aws_config, oab_cfg.bootstrap.bucket.as_deref()) .await?; - run_with_bucket(aws_config, resource, name, cluster, namespace, &bucket).await + run_with_bucket(aws_config, resource, name, cluster, namespace, &bucket) + .await + .map(|_warnings| ()) } async fn run_with_bucket( @@ -91,11 +337,13 @@ async fn run_with_bucket( cluster: &str, namespace: &str, bucket: &str, -) -> Result<()> { +) -> Result> { if resource != "oabservice" { anyhow::bail!("unknown resource type: {resource}. Use 'oabservice'"); } + let mut warnings = Vec::new(); + let service_name = format!("oab-{namespace}-{name}"); let ecs = aws_sdk_ecs::Client::new(aws_config); let s3 = aws_sdk_s3::Client::new(aws_config); @@ -195,10 +443,14 @@ async fn run_with_bucket( if let Err(error) = crate::ingress::teardown(aws_config, namespace, name, registry_arn.as_deref()).await { - eprintln!(" ⚠ ingress teardown skipped: {error}"); + let w = format!("ingress teardown skipped: {error}"); + eprintln!(" ⚠ {w}"); + warnings.push(w); } if let Err(error) = crate::ingress::delete_api(aws_config, namespace, name).await { - eprintln!(" ⚠ HTTP API cleanup skipped: {error}"); + let w = format!("HTTP API cleanup skipped: {error}"); + eprintln!(" ⚠ {w}"); + warnings.push(w); } let mut cleanup_failures = Vec::new(); @@ -264,13 +516,58 @@ async fn run_with_bucket( } println!("\n✓ {name} deleted"); - Ok(()) + Ok(warnings) } #[cfg(test)] mod tests { use super::*; + fn test_sdk_config() -> aws_config::SdkConfig { + aws_config::SdkConfig::builder() + .behavior_version(aws_config::BehaviorVersion::latest()) + .build() + } + + #[tokio::test] + async fn delete_services_rejects_empty_target_set() { + let cfg = test_sdk_config(); + let err = delete_services(&cfg, &[], &DeleteOptions::new("cluster")) + .await + .unwrap_err(); + assert_eq!(err.kind, DeleteErrorKind::Validation); + assert!(err.to_string().contains("empty target set"), "{err}"); + } + + #[tokio::test] + async fn delete_services_rejects_empty_cluster() { + let cfg = test_sdk_config(); + let targets = [DeleteTarget::new("prod", "bot")]; + let err = delete_services(&cfg, &targets, &DeleteOptions::new("")) + .await + .unwrap_err(); + assert_eq!(err.kind, DeleteErrorKind::Validation); + assert!(err.to_string().contains("cluster must not be empty"), "{err}"); + } + + #[tokio::test] + async fn delete_services_rejects_blank_target_fields() { + let cfg = test_sdk_config(); + let targets = [DeleteTarget::new("prod", " ")]; + let err = delete_services(&cfg, &targets, &DeleteOptions::new("cluster")) + .await + .unwrap_err(); + assert_eq!(err.kind, DeleteErrorKind::Validation); + } + + #[test] + fn delete_target_derives_ecs_service_name() { + assert_eq!( + DeleteTarget::new("prod", "nest-my-oab").ecs_service_name(), + "oab-prod-nest-my-oab" + ); + } + #[test] fn delete_phase_requests_delete_only_for_active_service() { assert_eq!( diff --git a/operator/src/lib.rs b/operator/src/lib.rs index ced58b1dc..9fe0248c5 100644 --- a/operator/src/lib.rs +++ b/operator/src/lib.rs @@ -1,7 +1,7 @@ //! Programmatic OAB manifest validation and ECS reconciliation. //! -//! The public facade intentionally contains only the manifest model and the -//! structured apply API. CLI implementation details and resource-management +//! The public facade contains the manifest model and the structured apply +//! and delete APIs. CLI implementation details and other resource-management //! helpers remain private. //! //! # Example @@ -42,9 +42,15 @@ //! an ARN require the apply caller to have `secretsmanager:DescribeSecret`, so //! oabctl can resolve the name to the full ARN required by ECS. //! -//! Programmatic apply never reads `~/.oabctl/config.toml`. Use -//! [`ApplyOptions::with_control_plane_bucket`] for an explicit bucket; otherwise -//! resolution uses `OAB_CONTROL_PLANE_BUCKET` and then the caller's AWS account. +//! Programmatic apply and delete never read `~/.oabctl/config.toml`. Use +//! [`ApplyOptions::with_control_plane_bucket`] / [`DeleteOptions::with_control_plane_bucket`] +//! for an explicit bucket; otherwise resolution uses `OAB_CONTROL_PLANE_BUCKET` +//! and then the caller's AWS account — the same chain for both, so delete +//! always cleans the bucket apply wrote to. +//! +//! [`delete_services`] tears down by `namespace`+`name` ([`DeleteTarget`]) with +//! the same explicit-cluster contract as apply. Teardown is resumable: rerun +//! the same target to continue after a partial failure. pub mod apply; mod bootstrap; @@ -52,7 +58,7 @@ mod cli; mod config; mod control_plane; mod create; -mod delete; +pub mod delete; mod get; mod ingress; pub mod manifest; @@ -63,6 +69,10 @@ pub use apply::{ apply_manifests, AppliedService, ApplyAction, ApplyError, ApplyErrorKind, ApplyOptions, ApplyReport, ServiceTarget, }; +pub use delete::{ + delete_services, DeleteError, DeleteErrorKind, DeleteOptions, DeleteReport, DeleteTarget, + DeletedService, +}; pub use manifest::{ AgentOverride, EcsNetworking, EcsRuntime, FleetMetadata, FleetSpec, FleetTemplate, Ingress, KubernetesRuntime, Metadata, OABFleetManifest, OABServiceManifest, RawManifest, Resources, From e0f5333f2533061884f4428fee040095e4edf5f1 Mon Sep 17 00:00:00 2001 From: chaodu-agent <274062505+chaodu-agent@users.noreply.github.com> Date: Thu, 16 Jul 2026 05:17:44 +0000 Subject: [PATCH 02/20] fix(review): preserve delete teardown failures --- operator/src/delete.rs | 115 +++++++++++++++++++++++++++++++--------- operator/src/ingress.rs | 10 ++-- 2 files changed, 97 insertions(+), 28 deletions(-) diff --git a/operator/src/delete.rs b/operator/src/delete.rs index 66d17eec7..da80ef376 100644 --- a/operator/src/delete.rs +++ b/operator/src/delete.rs @@ -262,6 +262,40 @@ fn ecs_delete_phase(status: Option<&str>) -> Result { } } +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum DrainPollAction { + Complete, + Retry, + TimedOut, +} + +fn drain_poll_action(is_gone: bool, attempt: u32, max_attempts: u32) -> DrainPollAction { + debug_assert!(max_attempts > 0); + debug_assert!(attempt < max_attempts); + if is_gone { + DrainPollAction::Complete + } else if attempt + 1 == max_attempts { + DrainPollAction::TimedOut + } else { + DrainPollAction::Retry + } +} + +fn collect_best_effort_warnings( + warnings: &mut Vec, + result: Result>, + failure_context: &str, +) { + match result { + Ok(step_warnings) => warnings.extend(step_warnings), + Err(error) => { + let warning = format!("{failure_context}: {error}"); + eprintln!(" ⚠ {warning}"); + warnings.push(warning); + } + } +} + /// Delete every OABService defined in a manifest file or directory. pub(crate) async fn run_from_file( aws_config: &aws_config::SdkConfig, @@ -422,36 +456,39 @@ async fn run_with_bucket( false } }; - if is_gone { - if attempt == 0 { - eprintln!(" done (immediate)"); - } else { + match drain_poll_action(is_gone, attempt, DRAIN_POLL_ATTEMPTS) { + DrainPollAction::Complete => { + if attempt == 0 { + eprintln!(" done (immediate)"); + } else { + let elapsed = u64::from(attempt) * DRAIN_POLL_INTERVAL.as_secs(); + eprintln!(" done ({elapsed}s)"); + } + break; + } + DrainPollAction::Retry => { + eprint!("."); + tokio::time::sleep(DRAIN_POLL_INTERVAL).await; + } + DrainPollAction::TimedOut => { let elapsed = u64::from(attempt) * DRAIN_POLL_INTERVAL.as_secs(); - eprintln!(" done ({elapsed}s)"); + eprintln!(" timed out ({elapsed}s)"); + anyhow::bail!( + "ECS service {service_name} is still draining after {elapsed}s; dependent cleanup was not started (safe to retry)" + ); } - break; - } - if attempt == DRAIN_POLL_ATTEMPTS - 1 { - eprintln!(" timed out (service may still be draining)"); - } else { - eprint!("."); - tokio::time::sleep(DRAIN_POLL_INTERVAL).await; } } } - if let Err(error) = - crate::ingress::teardown(aws_config, namespace, name, registry_arn.as_deref()).await - { - let w = format!("ingress teardown skipped: {error}"); - eprintln!(" ⚠ {w}"); - warnings.push(w); - } - if let Err(error) = crate::ingress::delete_api(aws_config, namespace, name).await { - let w = format!("HTTP API cleanup skipped: {error}"); - eprintln!(" ⚠ {w}"); - warnings.push(w); - } + let ingress_result = + crate::ingress::teardown(aws_config, namespace, name, registry_arn.as_deref()).await; + collect_best_effort_warnings(&mut warnings, ingress_result, "ingress teardown skipped"); + + let delete_api_result = crate::ingress::delete_api(aws_config, namespace, name) + .await + .map(|()| Vec::new()); + collect_best_effort_warnings(&mut warnings, delete_api_result, "HTTP API cleanup skipped"); let mut cleanup_failures = Vec::new(); let manifest_key = format!("manifests/{namespace}/{name}.yaml"); @@ -568,6 +605,36 @@ mod tests { ); } + #[test] + fn drain_poll_action_requires_completion_before_cleanup() { + assert_eq!(drain_poll_action(true, 0, 12), DrainPollAction::Complete); + assert_eq!(drain_poll_action(false, 0, 12), DrainPollAction::Retry); + assert_eq!(drain_poll_action(false, 11, 12), DrainPollAction::TimedOut); + } + + #[test] + fn best_effort_warnings_preserve_step_warnings_and_errors() { + let mut warnings = Vec::new(); + collect_best_effort_warnings( + &mut warnings, + Ok(vec!["route cleanup incomplete".to_string()]), + "ingress teardown skipped", + ); + collect_best_effort_warnings( + &mut warnings, + Err(anyhow::anyhow!("access denied")), + "HTTP API cleanup skipped", + ); + + assert_eq!( + warnings, + vec![ + "route cleanup incomplete", + "HTTP API cleanup skipped: access denied", + ] + ); + } + #[test] fn delete_phase_requests_delete_only_for_active_service() { assert_eq!( diff --git a/operator/src/ingress.rs b/operator/src/ingress.rs index 57761ea3d..136f03167 100644 --- a/operator/src/ingress.rs +++ b/operator/src/ingress.rs @@ -476,10 +476,12 @@ pub async fn delete_api(config: &aws_config::SdkConfig, namespace: &str, name: & let api = aws_sdk_apigatewayv2::Client::new(config); let name_str = api_name(namespace, name); if let Some((api_id, _)) = find_api(&api, &name_str).await? { - match api.delete_api().api_id(&api_id).send().await { - Ok(_) => eprintln!(" ✓ Deleted HTTP API: {name_str}"), - Err(e) => eprintln!(" ⚠ Failed to delete HTTP API {api_id}: {e}"), - } + api.delete_api() + .api_id(&api_id) + .send() + .await + .with_context(|| format!("failed to delete HTTP API {api_id}"))?; + eprintln!(" ✓ Deleted HTTP API: {name_str}"); } Ok(()) } From 7b9b432683ddf1607a8a18314f8d2c1800bf76ca Mon Sep 17 00:00:00 2001 From: chaodu-agent <274062505+chaodu-agent@users.noreply.github.com> Date: Thu, 16 Jul 2026 22:24:47 +0000 Subject: [PATCH 03/20] fix(review): require exact delete resource identity --- docs/oabctl.md | 41 +- operator/README.md | 51 +- operator/src/apply.rs | 289 ++++++++-- operator/src/delete.rs | 1159 ++++++++++++++++++++++++++++++--------- operator/src/ingress.rs | 486 +++++++++------- operator/src/lib.rs | 18 +- 6 files changed, 1505 insertions(+), 539 deletions(-) diff --git a/docs/oabctl.md b/docs/oabctl.md index 1a03f6022..6344950bf 100644 --- a/docs/oabctl.md +++ b/docs/oabctl.md @@ -365,18 +365,35 @@ must still be registered manually in the LINE Developers console. > reminded, not verified). > > **Teardown:** `oabctl delete oabservice ` (or `oabctl delete -f `, -> using the same file `apply -f` deployed it from) permanently removes the bot's -> per-bot ingress resources — its exact Cloud Map service (resolved by the ECS -> service's own registry ARN, not a name search, so same-named bots in different -> VPCs/environments can't collide) and its HTTP API (`oab-webhook--`, -> including the API resource itself this time, since the bot is gone for good) — -> on a best-effort basis (it never blocks service deletion). If you instead edit -> a manifest to remove `spec.ingress` while keeping the bot, `apply` runs the same -> Cloud Map + routes/integration/stage cleanup automatically, but **keeps the HTTP -> API** in case ingress is re-added later. The **shared** VPC Link and the -> security-group inbound rule are always left in place for other bots. If the -> Cloud Map service still has registered instances, teardown retries for ~25s -> before falling back to a warning with the manual cleanup command. +> using the same file `apply -f` deployed it from) first captures the exact ECS +> cluster/service ARNs, ECS registry ARN, matching HTTP API ID, configured +> control-plane bucket, and caller partition/account/region in +> `delete-checkpoints//.json`. The checkpoint is written before +> ECS mutation and removed only after all dependent and S3 cleanup succeeds, so +> rerunning the same command safely resumes a partial delete. ECS HTTP-200 +> failures, ambiguous drain responses, and a missing service fail closed unless +> a matching checkpoint already authorizes that exact retry. +> +> API cleanup never selects the first same-named API: duplicate names fail +> closed, and the sole candidate is checkpointed only when its integration URI +> equals the ECS registry ARN. Cloud Map is deleted only by the service ID +> parsed from a validated Cloud Map service ARN. Exact-ID NotFound is +> treated as already complete; other cleanup errors remain fatal so the +> checkpoint survives. For old CLI-created orphans that predate checkpoints, +> the CLI has an isolated compatibility path: it may remove a uniquely named +> API and S3 data, but refuses duplicate API names and never guesses a Cloud Map +> service by name. This compatibility behavior is not available through the +> programmatic `delete_services` API. +> +> If you edit a manifest to remove `spec.ingress` while keeping the bot, `apply` +> first stores every exact ECS registry ARN in +> `ingress-teardown-checkpoints//.json`, clears only API wiring +> bound to those ARNs, detaches all registries from ECS, waits until the detach +> is observable, and only then deletes each exact Cloud Map service. It keeps +> the HTTP API resource so its URL can survive re-enabling ingress and removes +> the checkpoint only after the full apply succeeds, so retries cannot lose +> cleanup identity. The **shared** VPC Link and security-group inbound rule are +> always left in place for other bots. > > **Changing `paths`:** `apply` prunes routes on the bot's API that are no longer > in the manifest's `ingress.paths`, so renaming or removing a webhook path never diff --git a/operator/README.md b/operator/README.md index 2ad4de1f1..c4b0f4679 100644 --- a/operator/README.md +++ b/operator/README.md @@ -75,16 +75,19 @@ commands reference. ## Library API -The crate exposes a deliberately narrow manifest + apply facade for control -planes that should not shell out to the CLI: +The crate exposes a deliberately narrow manifest + apply/delete facade for +control planes that should not shell out to the CLI: ```rust,no_run -use oabctl::{apply_manifests, ApplyOptions, OABServiceManifest}; +use oabctl::{ + apply_manifests, delete_services, ApplyOptions, DeleteOptions, DeleteTarget, + OABServiceManifest, +}; -async fn deploy( +async fn reconcile( aws: &aws_config::SdkConfig, manifest: OABServiceManifest, -) -> Result<(), oabctl::ApplyError> { +) -> Result<(), Box> { let report = apply_manifests( aws, &[manifest], @@ -94,20 +97,40 @@ async fn deploy( for service in report.services { println!("{}: {:?}", service.ecs_service_name, service.action); } + + delete_services( + aws, + &[DeleteTarget::new("prod", "bot")], + &DeleteOptions::new("production-cluster", "my-control-plane-bucket"), + ) + .await?; Ok(()) } ``` -Programmatic apply emits no progress to process-global stdout/stderr. Success -returns per-service actions, webhook URLs, and warnings; reconciliation errors -identify the failed service and include the report completed before the failure. -Both CLI and programmatic apply verify that the target cluster exists and is -`ACTIVE` before mutation, so the caller identity requires -`ecs:DescribeClusters`. This ECS action does not support resource-level +Programmatic apply/delete emit no progress to process-global stdout/stderr. +Apply success returns per-service actions, webhook URLs, and warnings; +reconciliation errors identify the failed service and include the report +completed before the failure. Both CLI and programmatic apply verify that the +target cluster exists and is `ACTIVE` before mutation, so the caller identity +requires `ecs:DescribeClusters`. This ECS action does not support resource-level permissions; its IAM statement must use `Resource: "*"`. -The library never reads `~/.oabctl/config.toml`: set -`with_control_plane_bucket(...)` explicitly when needed, otherwise bucket -resolution uses `OAB_CONTROL_PLANE_BUCKET` and then the caller's AWS account. + +The library never reads `~/.oabctl/config.toml`. Apply can explicitly override +its bucket with `ApplyOptions::with_control_plane_bucket(...)`; otherwise it +uses `OAB_CONTROL_PLANE_BUCKET` and then the caller account. Programmatic delete +is intentionally stricter and requires the exact bucket in +`DeleteOptions::new(cluster, bucket)` before any AWS call. Before deleting ECS, +it stores `delete-checkpoints//.json` in that bucket with the +caller partition/account/region, canonical ECS cluster/service ARNs, registry ARN, and +matching API ID. Duplicate API names fail closed; the sole same-name API is +checkpointed only when its integration URI equals the registry ARN. Missing or +ambiguous ECS identity fails closed without this record; retries use only its +immutable IDs and delete the checkpoint last. +The CLI resolves its configured bucket privately. Its compatibility cleanup for +pre-checkpoint orphans is isolated from `delete_services`, refuses duplicate API +names, and never guesses a Cloud Map service by name. + For `aws-sm://#`, a non-ARN `` requires the caller to have `secretsmanager:DescribeSecret`; full-ARN shorthand does not need that lookup. diff --git a/operator/src/apply.rs b/operator/src/apply.rs index ad5534a98..6ce4f6156 100644 --- a/operator/src/apply.rs +++ b/operator/src/apply.rs @@ -506,6 +506,129 @@ fn parse_manifest_file(path: &Path) -> Result> { } } + +const INGRESS_TEARDOWN_CHECKPOINT_VERSION: u8 = 1; + +#[derive(Debug, Clone, PartialEq, Eq, serde::Serialize, serde::Deserialize)] +struct IngressTeardownCheckpoint { + version: u8, + cluster: String, + service_name: String, + registry_arns: Vec, +} + +fn ingress_teardown_checkpoint_key(namespace: &str, name: &str) -> String { + format!("ingress-teardown-checkpoints/{namespace}/{name}.json") +} + +async fn load_ingress_teardown_checkpoint( + s3: &aws_sdk_s3::Client, + bucket: &str, + namespace: &str, + name: &str, +) -> Result> { + use aws_sdk_s3::error::ProvideErrorMetadata; + + let key = ingress_teardown_checkpoint_key(namespace, name); + match s3.get_object().bucket(bucket).key(&key).send().await { + Ok(response) => { + let bytes = response + .body + .collect() + .await + .context("failed to read ingress teardown checkpoint")? + .into_bytes(); + Ok(Some(serde_json::from_slice(&bytes).with_context(|| { + format!("invalid ingress teardown checkpoint s3://{bucket}/{key}") + })?)) + } + Err(error) if matches!(error.code(), Some("NoSuchKey" | "NotFound")) => Ok(None), + Err(error) => Err(error).with_context(|| { + format!("failed to read ingress teardown checkpoint s3://{bucket}/{key}") + }), + } +} + +async fn save_ingress_teardown_checkpoint( + s3: &aws_sdk_s3::Client, + bucket: &str, + namespace: &str, + name: &str, + checkpoint: &IngressTeardownCheckpoint, +) -> Result<()> { + let key = ingress_teardown_checkpoint_key(namespace, name); + let body = serde_json::to_vec_pretty(checkpoint)?; + s3.put_object() + .bucket(bucket) + .key(&key) + .body(ByteStream::from(body)) + .content_type("application/json") + .send() + .await + .with_context(|| format!("failed to persist s3://{bucket}/{key}"))?; + Ok(()) +} + +async fn remove_ingress_teardown_checkpoint( + s3: &aws_sdk_s3::Client, + bucket: &str, + namespace: &str, + name: &str, +) -> Result<()> { + let key = ingress_teardown_checkpoint_key(namespace, name); + s3.delete_object() + .bucket(bucket) + .key(&key) + .send() + .await + .with_context(|| format!("failed to remove s3://{bucket}/{key}"))?; + Ok(()) +} + +async fn wait_for_registry_detach( + ecs: &aws_sdk_ecs::Client, + cluster: &str, + service_name: &str, +) -> Result<()> { + const ATTEMPTS: u32 = 12; + const INTERVAL: std::time::Duration = std::time::Duration::from_secs(5); + + for attempt in 0..ATTEMPTS { + let response = ecs + .describe_services() + .cluster(cluster) + .services(service_name) + .send() + .await + .context("failed to verify ECS service registry detach")?; + if !response.failures().is_empty() { + anyhow::bail!( + "ECS returned failure(s) while verifying registry detach: {:?}", + response.failures() + ); + } + let service = match response.services() { + [service] => service, + [] => anyhow::bail!( + "ECS service disappeared while verifying registry detach" + ), + services => anyhow::bail!( + "ECS returned {} services for one registry-detach target", + services.len() + ), + }; + if service.service_registries().is_empty() { + return Ok(()); + } + if attempt + 1 < ATTEMPTS { + tokio::time::sleep(INTERVAL).await; + } + } + + anyhow::bail!( + "ECS service {service_name} still reports service registries after waiting; Cloud Map cleanup was not started" + ) +} async fn apply_ecs( ecs: &aws_sdk_ecs::Client, s3: &aws_sdk_s3::Client, @@ -529,17 +652,14 @@ async fn apply_ecs( } let bootstrap_state = bootstrap.state.as_ref(); - // Read current generation from S3 manifest (if exists), increment. - // Also capture whether the *previous* apply had ingress configured, so we - // can detect "ingress was removed from the manifest" and tear it down - // below — apply only ever provisioned ingress resources before this, so a - // manifest edit that drops `spec.ingress` used to orphan the per-bot HTTP - // API and Cloud Map service. + // Read the current generation from the stored desired-state manifest. + // Ingress teardown retry state is kept separately in an exact-identity + // checkpoint, so writing a newer manifest cannot lose pending cleanup. let manifest_key = format!( "manifests/{}/{}.yaml", m.metadata.namespace, m.metadata.name ); - let (current_gen, previously_had_ingress) = match s3 + let current_gen = match s3 .get_object() .bucket(bucket) .key(&manifest_key) @@ -549,20 +669,13 @@ async fn apply_ecs( Ok(resp) => { let bytes = resp.body.collect().await?.into_bytes(); let existing: OABServiceManifest = serde_yaml::from_slice(&bytes)?; - ( - existing.metadata.generation, - existing.spec.ingress.is_some(), - ) + existing.metadata.generation } - Err(_) => (0, false), + Err(_) => 0, }; let generation = current_gen + 1; - // Look up the ECS service's current registry ARN(s) up front so both the - // ingress-removal teardown below and the update/create logic further down - // can use the *exact* registry rather than falling back to a name-only - // Cloud Map scan (which can collide across VPCs/environments that share - // an account and reuse the same namespace/name). + // Resolve the current ECS registry identities without first-match behavior. let describe_resp = ecs .describe_services() .cluster(cluster) @@ -570,43 +683,96 @@ async fn apply_ecs( .send() .await .context("failed to describe ECS service")?; - let existing_registry_arns: Vec = describe_resp - .services() - .first() - .map(|service| { - service - .service_registries() - .iter() - .filter_map(|registry| registry.registry_arn()) - .map(str::to_owned) - .collect() - }) - .unwrap_or_default(); + if !describe_resp.failures().is_empty() { + anyhow::bail!( + "ECS returned failure(s) while resolving apply identity: {:?}", + describe_resp.failures() + ); + } + let existing_service = match describe_resp.services() { + [] => None, + [service] => Some(service), + services => anyhow::bail!( + "ECS returned {} services for one apply target", + services.len() + ), + }; + let mut existing_registry_arns = Vec::new(); + if let Some(service) = existing_service { + for registry in service.service_registries() { + existing_registry_arns.push( + registry + .registry_arn() + .filter(|value| !value.trim().is_empty()) + .context("ECS returned a service registry without an ARN")? + .to_string(), + ); + } + } + existing_registry_arns.sort(); + existing_registry_arns.dedup(); let has_registries = !existing_registry_arns.is_empty(); - // If ingress was configured before but is absent now, tear down the - // orphaned per-bot ingress resources (best-effort, mirrors `oabctl delete`) - // and detach the stale registry from the ECS service itself — omitting - // `serviceRegistries` on `UpdateService` leaves the existing configuration - // untouched (AWS only clears it when explicitly passed an empty list), so - // without this the service would keep pointing at a Cloud Map service that - // teardown() is about to delete. - if previously_had_ingress && m.spec.ingress.is_none() { - eprintln!(" 🌐 ingress removed from manifest — tearing down orphaned resources..."); - match crate::ingress::teardown( - config, - &m.metadata.namespace, - &m.metadata.name, - existing_registry_arns.first().map(String::as_str), - ) - .await - { - Ok(teardown_warnings) => warnings.extend(teardown_warnings), - Err(error) => { - let warning = format!("ingress teardown skipped: {error}"); - eprintln!(" ⚠ {warning}"); - warnings.push(warning); + let stored_teardown = load_ingress_teardown_checkpoint( + s3, + bucket, + &m.metadata.namespace, + &m.metadata.name, + ) + .await?; + if m.spec.ingress.is_some() && stored_teardown.is_some() { + anyhow::bail!( + "a previous ingress teardown is still pending; apply the ingress-free manifest again before re-enabling ingress" + ); + } + let teardown_checkpoint = if m.spec.ingress.is_none() { + match stored_teardown { + Some(checkpoint) => { + if checkpoint.version != INGRESS_TEARDOWN_CHECKPOINT_VERSION + || checkpoint.cluster != cluster + || checkpoint.service_name != service_name + || checkpoint.registry_arns.is_empty() + { + anyhow::bail!("ingress teardown checkpoint does not match this apply target"); + } + Some(checkpoint) + } + None if has_registries => { + let checkpoint = IngressTeardownCheckpoint { + version: INGRESS_TEARDOWN_CHECKPOINT_VERSION, + cluster: cluster.to_string(), + service_name: service_name.clone(), + registry_arns: existing_registry_arns.clone(), + }; + save_ingress_teardown_checkpoint( + s3, + bucket, + &m.metadata.namespace, + &m.metadata.name, + &checkpoint, + ) + .await?; + Some(checkpoint) } + None => None, + } + } else { + None + }; + + if let Some(checkpoint) = &teardown_checkpoint { + eprintln!(" 🌐 ingress removed from manifest — tearing down exact API wiring..."); + for registry_arn in &checkpoint.registry_arns { + warnings.extend( + crate::ingress::teardown( + config, + &m.metadata.namespace, + &m.metadata.name, + Some(registry_arn), + ) + .await + .context("failed to tear down exact ingress API wiring")?, + ); } } @@ -1021,6 +1187,17 @@ async fn apply_ecs( ); } + if let Some(checkpoint) = &teardown_checkpoint { + wait_for_registry_detach(&ecs, cluster, &service_name).await?; + for registry_arn in &checkpoint.registry_arns { + crate::ingress::delete_cloud_map_exact(config, registry_arn, &service_name) + .await + .with_context(|| { + format!("failed exact Cloud Map cleanup for {registry_arn}") + })?; + } + } + // Ingress step 2: VPC Link + API Gateway + routes + SG rule. let mut webhook_urls = Vec::new(); if let (Some(ingress), Some(cm)) = (&m.spec.ingress, &cloud_map) { @@ -1069,6 +1246,16 @@ async fn apply_ecs( eprintln!(" ✓ {} is stable", m.metadata.name); } + if teardown_checkpoint.is_some() { + remove_ingress_teardown_checkpoint( + s3, + bucket, + &m.metadata.namespace, + &m.metadata.name, + ) + .await?; + } + Ok(AppliedService { namespace: m.metadata.namespace.clone(), name: m.metadata.name.clone(), diff --git a/operator/src/delete.rs b/operator/src/delete.rs index da80ef376..4aa193c0a 100644 --- a/operator/src/delete.rs +++ b/operator/src/delete.rs @@ -1,5 +1,7 @@ use anyhow::{Context, Result}; use aws_sdk_ecs::error::ProvideErrorMetadata; +use aws_sdk_s3::primitives::ByteStream; +use serde::{Deserialize, Serialize}; use std::fmt; use std::path::Path; @@ -40,32 +42,24 @@ impl DeleteTarget { } } -/// Target options for [`delete_services`]. Mirrors -/// [`ApplyOptions`](crate::apply::ApplyOptions): the cluster is required and -/// the library never reads `~/.oabctl/config.toml`. +/// Target options for [`delete_services`]. The library never reads CLI +/// configuration or guesses which control-plane bucket owns a deployment. #[derive(Debug, Clone, PartialEq, Eq)] pub struct DeleteOptions { /// ECS cluster name or ARN the services were deployed into. pub cluster: String, - /// Optional control-plane bucket override. When absent, resolution uses - /// `OAB_CONTROL_PLANE_BUCKET`, then `oab-control-plane-{account}` from - /// the caller's AWS identity — the same chain as apply, so delete always - /// cleans the bucket apply wrote to. - pub control_plane_bucket: Option, + /// Exact S3 bucket that owns the deployment and its delete checkpoint. + pub control_plane_bucket: String, } impl DeleteOptions { - pub fn new(cluster: impl Into) -> Self { + /// Bind a delete request to both its ECS cluster and control-plane bucket. + pub fn new(cluster: impl Into, control_plane_bucket: impl Into) -> Self { Self { cluster: cluster.into(), - control_plane_bucket: None, + control_plane_bucket: control_plane_bucket.into(), } } - - pub fn with_control_plane_bucket(mut self, bucket: impl Into) -> Self { - self.control_plane_bucket = Some(bucket.into()); - self - } } /// Teardown outcome for one service. @@ -74,9 +68,9 @@ pub struct DeletedService { pub namespace: String, pub name: String, pub ecs_service_name: String, - /// Best-effort steps that were skipped (e.g. ingress teardown pieces - /// already gone). The core teardown (ECS service, S3 manifest and - /// artifacts) either completed or the whole target failed. + /// Non-fatal diagnostics retained for API compatibility. Exact-identity + /// dependent and S3 cleanup failures are fatal so the durable checkpoint + /// remains available for retry. pub warnings: Vec, } @@ -91,8 +85,6 @@ pub struct DeleteReport { pub enum DeleteErrorKind { /// Contract violation caught before any AWS call. Validation, - /// The deployment target (bucket resolution) could not be established. - Target, /// Teardown of a specific service failed. Cleanup is resumable: calling /// [`delete_services`] again with the same target continues from the /// remaining resources. @@ -119,15 +111,6 @@ impl DeleteError { } } - fn target(source: impl Into) -> Self { - Self { - kind: DeleteErrorKind::Target, - failed_service: None, - completed: DeleteReport::default(), - source: source.into(), - } - } - fn teardown( failed_service: DeleteTarget, completed: DeleteReport, @@ -165,20 +148,253 @@ impl std::error::Error for DeleteError { } } +const DELETE_CHECKPOINT_VERSION: u8 = 1; + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum DeletePolicy { + ExactIdentity, + CliLegacyOrphanCleanup, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +struct DeleteCheckpoint { + version: u8, + namespace: String, + name: String, + bucket: String, + partition: String, + account: String, + region: String, + requested_cluster: String, + cluster_arn: String, + service_arn: String, + registry_arn: Option, + api_id: Option, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum ServiceIdentityState { + Live, + RetryGone, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +struct ArnParts<'a> { + partition: &'a str, + service: &'a str, + region: &'a str, + account: &'a str, + resource: &'a str, +} + +fn parse_arn(value: &str) -> Option> { + let mut parts = value.splitn(6, ':'); + if parts.next()? != "arn" { + return None; + } + let arn = ArnParts { + partition: parts.next()?, + service: parts.next()?, + region: parts.next()?, + account: parts.next()?, + resource: parts.next()?, + }; + if arn.partition.is_empty() + || arn.service.is_empty() + || arn.region.is_empty() + || arn.account.is_empty() + || arn.resource.is_empty() + { + return None; + } + Some(arn) +} + +fn cluster_reference_matches(cluster_arn: &str, reference: &str) -> bool { + let Some(cluster) = parse_arn(cluster_arn) else { + return false; + }; + if reference.starts_with("arn:") { + return reference == cluster_arn; + } + cluster.resource.strip_prefix("cluster/") == Some(reference) +} + +fn validate_checkpoint_arns( + checkpoint: &DeleteCheckpoint, + expected_service_name: &str, + partition: &str, + account: &str, + region: &str, +) -> Result<()> { + let cluster = parse_arn(&checkpoint.cluster_arn) + .context("delete checkpoint contains an invalid ECS cluster ARN")?; + if cluster.partition != partition + || cluster.service != "ecs" + || cluster.account != account + || cluster.region != region + { + anyhow::bail!("delete checkpoint ECS cluster ARN is outside the caller boundary"); + } + let cluster_name = cluster + .resource + .strip_prefix("cluster/") + .filter(|value| !value.is_empty() && !value.contains('/')) + .context("delete checkpoint contains an invalid ECS cluster resource")?; + + let service = parse_arn(&checkpoint.service_arn) + .context("delete checkpoint contains an invalid ECS service ARN")?; + if service.partition != cluster.partition + || service.service != "ecs" + || service.account != account + || service.region != region + { + anyhow::bail!("delete checkpoint ECS service ARN is outside the cluster boundary"); + } + let (service_cluster, service_name) = service + .resource + .strip_prefix("service/") + .and_then(|resource| resource.split_once('/')) + .context("delete checkpoint ECS service ARN lacks its cluster identity")?; + if service_cluster != cluster_name || service_name != expected_service_name { + anyhow::bail!("delete checkpoint ECS service ARN does not match the target cluster/service"); + } + + if let Some(registry_arn) = checkpoint.registry_arn.as_deref() { + let registry = parse_arn(registry_arn) + .context("delete checkpoint contains an invalid Cloud Map service ARN")?; + if registry.partition != cluster.partition + || registry.service != "servicediscovery" + || registry.account != account + || registry.region != region + || registry + .resource + .strip_prefix("service/") + .filter(|value| !value.is_empty() && !value.contains('/')) + .is_none() + { + anyhow::bail!( + "delete checkpoint Cloud Map ARN is outside the ECS account/region boundary" + ); + } + } + Ok(()) +} + +fn classify_service_identity( + has_checkpoint: bool, + service_present: bool, + status: Option<&str>, + failure_reasons: &[&str], +) -> std::result::Result { + if !failure_reasons.is_empty() { + if has_checkpoint + && !service_present + && failure_reasons + .iter() + .all(|reason| reason.eq_ignore_ascii_case("MISSING")) + { + return Ok(ServiceIdentityState::RetryGone); + } + return Err(format!( + "DescribeServices returned failure(s): {}", + failure_reasons.join(", ") + )); + } + + match (service_present, status) { + (true, Some("ACTIVE")) | (true, Some("DRAINING")) => { + Ok(ServiceIdentityState::Live) + } + (true, Some("INACTIVE")) if has_checkpoint => Ok(ServiceIdentityState::RetryGone), + (false, None) if has_checkpoint => Ok(ServiceIdentityState::RetryGone), + (false, None) | (true, Some("INACTIVE")) => Err( + "ECS service was not positively identified and no matching delete checkpoint exists" + .to_string(), + ), + (true, None) => Err("ECS returned a service without status".to_string()), + (true, Some(other)) => Err(format!( + "unexpected ECS service status during delete: {other}" + )), + (false, Some(_)) => Err("ECS returned status without a service identity".to_string()), + } +} + +fn checkpoint_key(namespace: &str, name: &str) -> String { + format!("delete-checkpoints/{namespace}/{name}.json") +} + +fn validate_checkpoint( + checkpoint: &DeleteCheckpoint, + namespace: &str, + name: &str, + cluster: &str, + bucket: &str, + partition: &str, + account: &str, + region: &str, +) -> Result<()> { + if checkpoint.version != DELETE_CHECKPOINT_VERSION { + anyhow::bail!( + "unsupported delete checkpoint version {}", + checkpoint.version + ); + } + if checkpoint.namespace != namespace + || checkpoint.name != name + || checkpoint.bucket != bucket + || checkpoint.partition != partition + || checkpoint.account != account + || checkpoint.region != region + { + anyhow::bail!( + "delete checkpoint identity does not match namespace/name, bucket, or caller boundary" + ); + } + if !cluster_reference_matches(&checkpoint.cluster_arn, &checkpoint.requested_cluster) + || !cluster_reference_matches(&checkpoint.cluster_arn, cluster) + { + anyhow::bail!("delete checkpoint canonical cluster does not match the requested cluster"); + } + if checkpoint.cluster_arn.trim().is_empty() || checkpoint.service_arn.trim().is_empty() { + anyhow::bail!("delete checkpoint is missing exact ECS identity"); + } + validate_checkpoint_arns( + checkpoint, + &format!("oab-{namespace}-{name}"), + partition, + account, + region, + )?; + if let Some(api_id) = checkpoint.api_id.as_deref() { + if api_id.trim().is_empty() || checkpoint.registry_arn.is_none() { + anyhow::bail!( + "delete checkpoint API identity requires a non-empty API ID and registry ARN" + ); + } + } + Ok(()) +} + fn validate_delete_request( targets: &[DeleteTarget], - cluster: &str, + opts: &DeleteOptions, ) -> std::result::Result<(), DeleteError> { if targets.is_empty() { return Err(DeleteError::validation(anyhow::anyhow!( "no targets to delete (empty target set)" ))); } - if cluster.trim().is_empty() { + if opts.cluster.trim().is_empty() { return Err(DeleteError::validation(anyhow::anyhow!( "DeleteOptions.cluster must not be empty" ))); } + if opts.control_plane_bucket.trim().is_empty() { + return Err(DeleteError::validation(anyhow::anyhow!( + "DeleteOptions.control_plane_bucket must not be empty" + ))); + } for target in targets { if target.namespace.trim().is_empty() || target.name.trim().is_empty() { return Err(DeleteError::validation(anyhow::anyhow!( @@ -196,27 +412,24 @@ fn validate_delete_request( /// /// Contract (enforced before any AWS call): /// - the target set must be non-empty -/// - [`DeleteOptions::cluster`] must be a non-empty cluster name +/// - [`DeleteOptions::cluster`] and the explicitly bound control-plane bucket +/// must be non-empty /// - every target's `namespace`/`name` must be non-empty /// -/// Teardown per target is resumable: an `ACTIVE` service is scaled down and -/// deleted, a `DRAINING` service resumes its drain wait, and an absent one -/// proceeds straight to ingress/S3 cleanup. Best-effort ingress steps are -/// reported through [`DeletedService::warnings`]; S3 cleanup failures fail -/// the target (safe to retry). +/// Before ECS mutation, delete persists an exact-identity checkpoint containing +/// the caller account/region, bucket, canonical cluster and service ARNs, and +/// any ECS registry/API IDs. An absent ECS service or ambiguous +/// `DescribeServices` response is rejected unless that matching durable +/// checkpoint already exists. Cleanup uses only IDs from the checkpoint and +/// removes the checkpoint last, making partial failures safe to retry. pub async fn delete_services( aws_config: &aws_config::SdkConfig, targets: &[DeleteTarget], opts: &DeleteOptions, ) -> std::result::Result { crate::apply::with_progress_suppressed(async { - validate_delete_request(targets, &opts.cluster)?; - let bucket = crate::control_plane::resolve_bucket( - aws_config, - opts.control_plane_bucket.as_deref(), - ) - .await - .map_err(DeleteError::target)?; + validate_delete_request(targets, opts)?; + let bucket = opts.control_plane_bucket.trim(); let mut report = DeleteReport::default(); for target in targets { @@ -226,7 +439,8 @@ pub async fn delete_services( &target.name, &opts.cluster, &target.namespace, - &bucket, + bucket, + DeletePolicy::ExactIdentity, ) .await { @@ -281,21 +495,6 @@ fn drain_poll_action(is_gone: bool, attempt: u32, max_attempts: u32) -> DrainPol } } -fn collect_best_effort_warnings( - warnings: &mut Vec, - result: Result>, - failure_context: &str, -) { - match result { - Ok(step_warnings) => warnings.extend(step_warnings), - Err(error) => { - let warning = format!("{failure_context}: {error}"); - eprintln!(" ⚠ {warning}"); - warnings.push(warning); - } - } -} - /// Delete every OABService defined in a manifest file or directory. pub(crate) async fn run_from_file( aws_config: &aws_config::SdkConfig, @@ -328,6 +527,7 @@ pub(crate) async fn run_from_file( cluster, &manifest.metadata.namespace, &bucket, + DeletePolicy::CliLegacyOrphanCleanup, ) .await { @@ -359,203 +559,406 @@ pub(crate) async fn run( let bucket = crate::control_plane::resolve_bucket(aws_config, oab_cfg.bootstrap.bucket.as_deref()) .await?; - run_with_bucket(aws_config, resource, name, cluster, namespace, &bucket) + run_with_bucket( + aws_config, + resource, + name, + cluster, + namespace, + &bucket, + DeletePolicy::CliLegacyOrphanCleanup, + ) + .await + .map(|_warnings| ()) +} + +async fn caller_context(aws_config: &aws_config::SdkConfig) -> Result<(String, String, String)> { + let identity = aws_sdk_sts::Client::new(aws_config) + .get_caller_identity() + .send() .await - .map(|_warnings| ()) + .context("failed to identify delete caller")?; + let account = identity + .account() + .context("STS response missing caller account")? + .to_string(); + let caller_arn = identity.arn().context("STS response missing caller ARN")?; + let mut caller_arn_parts = caller_arn.splitn(3, ':'); + if caller_arn_parts.next() != Some("arn") { + anyhow::bail!("STS returned an invalid caller ARN"); + } + let partition = caller_arn_parts + .next() + .filter(|value| !value.is_empty()) + .context("STS caller ARN is missing its partition")? + .to_string(); + let region = aws_config + .region() + .context("AWS region must be resolved before delete")? + .to_string(); + Ok((partition, account, region)) } -async fn run_with_bucket( +async fn load_checkpoint( + s3: &aws_sdk_s3::Client, bucket: &str, namespace: &str, name: &str, +) -> Result> { + let key = checkpoint_key(namespace, name); + match s3.get_object().bucket(bucket).key(&key).send().await { + Ok(response) => { + let bytes = response.body.collect().await + .context("failed to read delete checkpoint body")?.into_bytes(); + Ok(Some(serde_json::from_slice(&bytes) + .with_context(|| format!("invalid delete checkpoint s3://{bucket}/{key}"))?)) + } + Err(error) if matches!(error.code(), Some("NoSuchKey" | "NotFound")) => Ok(None), + Err(error) => Err(error) + .with_context(|| format!("failed to read delete checkpoint s3://{bucket}/{key}")), + } +} + +async fn save_checkpoint(s3: &aws_sdk_s3::Client, checkpoint: &DeleteCheckpoint) -> Result<()> { + let key = checkpoint_key(&checkpoint.namespace, &checkpoint.name); + let body = serde_json::to_vec_pretty(checkpoint)?; + s3.put_object().bucket(&checkpoint.bucket).key(&key) + .body(ByteStream::from(body)).content_type("application/json").send().await + .with_context(|| format!( + "failed to persist exact delete identity to s3://{}/{key}", checkpoint.bucket + ))?; + Ok(()) +} + +fn failure_reasons( + response: &aws_sdk_ecs::operation::describe_services::DescribeServicesOutput, +) -> Vec<&str> { + response.failures().iter() + .map(|failure| failure.reason().unwrap_or("UNKNOWN")) + .collect() +} + + +fn single_described_service( + response: &aws_sdk_ecs::operation::describe_services::DescribeServicesOutput, +) -> Result> { + match response.services() { + [] => Ok(None), + [service] => Ok(Some(service)), + services => anyhow::bail!( + "ECS returned {} services for one delete target", + services.len() + ), + } +} +enum PreparedIdentity { + Exact(DeleteCheckpoint, ServiceIdentityState), + LegacyOrphan, +} + +async fn prepare_identity( aws_config: &aws_config::SdkConfig, - resource: &str, + ecs: &aws_sdk_ecs::Client, + s3: &aws_sdk_s3::Client, + namespace: &str, name: &str, cluster: &str, - namespace: &str, bucket: &str, -) -> Result> { - if resource != "oabservice" { - anyhow::bail!("unknown resource type: {resource}. Use 'oabservice'"); + policy: DeletePolicy, +) -> Result { + let (partition, account, region) = caller_context(aws_config).await?; + if let Some(checkpoint) = load_checkpoint(s3, bucket, namespace, name).await? { + validate_checkpoint( + &checkpoint, + namespace, + name, + cluster, + bucket, + &partition, + &account, + ®ion, + )?; + let response = ecs.describe_services() + .cluster(&checkpoint.cluster_arn).services(&checkpoint.service_arn) + .send().await.context("failed to describe checkpointed ECS service")?; + let service = single_described_service(&response)?; + let reasons = failure_reasons(&response); + let state = classify_service_identity( + true, + service.is_some(), + service.and_then(|service| service.status()), + &reasons, + ).map_err(anyhow::Error::msg)?; + if let Some(service) = service { + validate_returned_service(&checkpoint, service)?; + } + return Ok(PreparedIdentity::Exact(checkpoint, state)); } - let mut warnings = Vec::new(); - let service_name = format!("oab-{namespace}-{name}"); - let ecs = aws_sdk_ecs::Client::new(aws_config); - let s3 = aws_sdk_s3::Client::new(aws_config); + let response = ecs.describe_services().cluster(cluster).services(&service_name) + .send().await.context("failed to describe ECS service before delete")?; + let service = single_described_service(&response)?; + let reasons = failure_reasons(&response); + let state = match classify_service_identity( + false, + service.is_some(), + service.and_then(|service| service.status()), + &reasons, + ) { + Ok(state) => state, + Err(_) if policy == DeletePolicy::CliLegacyOrphanCleanup + && service.is_none() + && (reasons.is_empty() || reasons.iter().all(|reason| reason.eq_ignore_ascii_case("MISSING"))) => + { + return Ok(PreparedIdentity::LegacyOrphan); + } + Err(message) => anyhow::bail!(message), + }; + let service = service.context("ECS service identity unexpectedly absent")?; + let service_arn = service.service_arn().filter(|value| !value.trim().is_empty()) + .context("ECS service response missing service ARN")?.to_string(); + let cluster_arn = service.cluster_arn().filter(|value| !value.trim().is_empty()) + .context("ECS service response missing canonical cluster ARN")?.to_string(); + let registry_arn = match service.service_registries() { + [] => None, + [registry] => Some( + registry + .registry_arn() + .filter(|value| !value.trim().is_empty()) + .context("ECS service registry is missing its exact ARN")? + .to_string(), + ), + _ => anyhow::bail!( + "ECS service has multiple registry identities; refusing ambiguous dependent cleanup" + ), + }; + let api_id = match registry_arn.as_deref() { + Some(registry_arn) => crate::ingress::resolve_api_id_for_registry( + aws_config, namespace, name, registry_arn, + ).await?, + None => None, + }; + let checkpoint = DeleteCheckpoint { + version: DELETE_CHECKPOINT_VERSION, + namespace: namespace.to_string(), + name: name.to_string(), + bucket: bucket.to_string(), + partition, + account, + region, + requested_cluster: cluster.to_string(), + cluster_arn, + service_arn, + registry_arn, + api_id, + }; + validate_checkpoint( + &checkpoint, + namespace, + name, + cluster, + bucket, + &checkpoint.partition, + &checkpoint.account, + &checkpoint.region, + )?; + save_checkpoint(s3, &checkpoint).await?; + Ok(PreparedIdentity::Exact(checkpoint, state)) +} - println!("Deleting {name}..."); +fn validate_returned_service( + checkpoint: &DeleteCheckpoint, service: &aws_sdk_ecs::types::Service, +) -> Result<()> { + if service.service_arn() != Some(checkpoint.service_arn.as_str()) + || service.cluster_arn() != Some(checkpoint.cluster_arn.as_str()) + { + anyhow::bail!("ECS drain poll returned a conflicting service identity"); + } + let registry_arn = match service.service_registries() { + [] => None, + [registry] => Some( + registry + .registry_arn() + .filter(|value| !value.trim().is_empty()) + .context("ECS drain poll returned a registry without an ARN")?, + ), + _ => anyhow::bail!("ECS drain poll returned multiple registry identities"), + }; + if registry_arn != checkpoint.registry_arn.as_deref() { + anyhow::bail!("ECS drain poll returned a conflicting registry identity"); + } + Ok(()) +} - let describe_response = ecs - .describe_services() - .cluster(cluster) - .services(&service_name) - .send() - .await - .context("failed to describe ECS service before delete")?; - let service = describe_response.services().first(); - let registry_arn: Option = service.and_then(|service| { - service - .service_registries() - .first() - .and_then(|registry| registry.registry_arn()) - .map(str::to_owned) - }); - let service_status = service.and_then(|service| service.status()); - let delete_phase = ecs_delete_phase(service_status)?; - let service_needs_delete = delete_phase == EcsDeletePhase::Delete; - let service_is_draining = delete_phase == EcsDeletePhase::Drain; - - if service_needs_delete { - let _ = ecs - .update_service() - .cluster(cluster) - .service(&service_name) - .desired_count(0) - .send() - .await; - println!(" ✓ Scaled to 0"); - - match ecs - .delete_service() - .cluster(cluster) - .service(&service_name) - .force(true) - .send() - .await - { +async fn delete_checkpointed_ecs( + ecs: &aws_sdk_ecs::Client, + checkpoint: &DeleteCheckpoint, + state: ServiceIdentityState, +) -> Result<()> { + if state == ServiceIdentityState::RetryGone { + println!(" ✓ ECS service already absent; resuming exact dependent cleanup"); + return Ok(()); + } + + let response = ecs.describe_services() + .cluster(&checkpoint.cluster_arn).services(&checkpoint.service_arn) + .send().await.context("failed to refresh ECS service before mutation")?; + let reasons = failure_reasons(&response); + let service = single_described_service(&response)?; + let state = classify_service_identity( + true, + service.is_some(), + service.and_then(|service| service.status()), + &reasons, + ).map_err(anyhow::Error::msg)?; + if state == ServiceIdentityState::RetryGone { + return Ok(()); + } + let service = service.context("checkpointed ECS service disappeared ambiguously")?; + validate_returned_service(checkpoint, service)?; + let delete_phase = ecs_delete_phase(service.status())?; + + if delete_phase == EcsDeletePhase::Delete { + match ecs.update_service().cluster(&checkpoint.cluster_arn) + .service(&checkpoint.service_arn).desired_count(0).send().await { + Ok(_) => println!(" ✓ Scaled to 0"), + Err(error) if error.code() == Some("ServiceNotFoundException") => {} + Err(error) => return Err(error).context("failed to scale ECS service to zero"), + } + match ecs.delete_service().cluster(&checkpoint.cluster_arn) + .service(&checkpoint.service_arn).force(true).send().await { Ok(_) => println!(" ✓ ECS service deleted"), - Err(error) if error.code() == Some("ServiceNotFoundException") => { - println!(" ✓ ECS service already absent") - } + Err(error) if error.code() == Some("ServiceNotFoundException") => {} Err(error) => return Err(error).context("failed to delete ECS service"), } - } else if service_is_draining { - println!(" ✓ ECS service is already draining; resuming delete cleanup"); } else { - println!(" ✓ ECS service already absent; resuming dependent cleanup"); - } - - if service_needs_delete || service_is_draining { - const DRAIN_POLL_ATTEMPTS: u32 = 12; - const DRAIN_POLL_INTERVAL: std::time::Duration = std::time::Duration::from_secs(5); - eprint!(" ⏳ Waiting for drain to complete..."); - for attempt in 0..DRAIN_POLL_ATTEMPTS { - let response = ecs - .describe_services() - .cluster(cluster) - .services(&service_name) - .send() - .await; - let is_gone = match response { - Ok(response) => response - .services() - .first() - .map(|service| service.status() == Some("INACTIVE")) - .unwrap_or(true), - Err(error) => { - eprintln!("\n ⚠ describe_services error (retrying): {error}"); - false - } - }; - match drain_poll_action(is_gone, attempt, DRAIN_POLL_ATTEMPTS) { - DrainPollAction::Complete => { - if attempt == 0 { - eprintln!(" done (immediate)"); - } else { - let elapsed = u64::from(attempt) * DRAIN_POLL_INTERVAL.as_secs(); - eprintln!(" done ({elapsed}s)"); + println!(" ✓ ECS service is already draining; resuming delete cleanup"); + } + + const DRAIN_POLL_ATTEMPTS: u32 = 12; + const DRAIN_POLL_INTERVAL: std::time::Duration = std::time::Duration::from_secs(5); + eprint!(" ⏳ Waiting for drain to complete..."); + for attempt in 0..DRAIN_POLL_ATTEMPTS { + let complete = match ecs.describe_services() + .cluster(&checkpoint.cluster_arn).services(&checkpoint.service_arn) + .send().await { + Ok(response) => { + let reasons = failure_reasons(&response); + let service = single_described_service(&response)?; + match classify_service_identity( + true, + service.is_some(), + service.and_then(|service| service.status()), + &reasons, + ) { + Ok(ServiceIdentityState::RetryGone) => true, + Ok(ServiceIdentityState::Live) => { + if let Some(service) = service { + validate_returned_service(checkpoint, service)?; + } + false + } + Err(error) => { + eprintln!("\n ⚠ ambiguous DescribeServices response (retrying): {error}"); + false } - break; - } - DrainPollAction::Retry => { - eprint!("."); - tokio::time::sleep(DRAIN_POLL_INTERVAL).await; - } - DrainPollAction::TimedOut => { - let elapsed = u64::from(attempt) * DRAIN_POLL_INTERVAL.as_secs(); - eprintln!(" timed out ({elapsed}s)"); - anyhow::bail!( - "ECS service {service_name} is still draining after {elapsed}s; dependent cleanup was not started (safe to retry)" - ); } } + Err(error) => { + eprintln!("\n ⚠ describe_services error (retrying): {error}"); + false + } + }; + match drain_poll_action(complete, attempt, DRAIN_POLL_ATTEMPTS) { + DrainPollAction::Complete => { eprintln!(" done"); return Ok(()); } + DrainPollAction::Retry => { + eprint!("."); + tokio::time::sleep(DRAIN_POLL_INTERVAL).await; + } + DrainPollAction::TimedOut => anyhow::bail!( + "checkpointed ECS service did not reach an unambiguous absent state; dependent cleanup was not started (safe to retry)" + ), } } + unreachable!() +} - let ingress_result = - crate::ingress::teardown(aws_config, namespace, name, registry_arn.as_deref()).await; - collect_best_effort_warnings(&mut warnings, ingress_result, "ingress teardown skipped"); - - let delete_api_result = crate::ingress::delete_api(aws_config, namespace, name) - .await - .map(|()| Vec::new()); - collect_best_effort_warnings(&mut warnings, delete_api_result, "HTTP API cleanup skipped"); - - let mut cleanup_failures = Vec::new(); +async fn cleanup_s3( + s3: &aws_sdk_s3::Client, bucket: &str, namespace: &str, name: &str, +) -> Result<()> { let manifest_key = format!("manifests/{namespace}/{name}.yaml"); - match s3 - .delete_object() - .bucket(bucket) - .key(&manifest_key) - .send() - .await - { - Ok(_) => println!(" ✓ Manifest removed from S3"), - Err(error) => cleanup_failures.push(format!( - "failed to delete s3://{bucket}/{manifest_key}: {error}" - )), - } + s3.delete_object().bucket(bucket).key(&manifest_key).send().await + .with_context(|| format!("failed to delete s3://{bucket}/{manifest_key}"))?; + println!(" ✓ Manifest removed from S3"); let artifact_prefix = format!("artifacts/{namespace}/{name}/"); let mut continuation_token = None; loop { - let response = match s3 - .list_objects_v2() - .bucket(bucket) - .prefix(&artifact_prefix) - .set_continuation_token(continuation_token) - .send() - .await - { - Ok(response) => response, - Err(error) => { - cleanup_failures.push(format!( - "failed to list config artifacts under s3://{bucket}/{artifact_prefix}: {error}" - )); - break; - } - }; + let response = s3.list_objects_v2().bucket(bucket).prefix(&artifact_prefix) + .set_continuation_token(continuation_token).send().await + .with_context(|| format!( + "failed to list config artifacts under s3://{bucket}/{artifact_prefix}" + ))?; for object in response.contents() { if let Some(key) = object.key() { - if let Err(error) = s3 - .delete_object() - .bucket(bucket) - .key(key) - .send() - .await - { - cleanup_failures - .push(format!("failed to delete s3://{bucket}/{key}: {error}")); - } + s3.delete_object().bucket(bucket).key(key).send().await + .with_context(|| format!("failed to delete s3://{bucket}/{key}"))?; } } continuation_token = response.next_continuation_token().map(str::to_owned); - if continuation_token.is_none() { - break; - } + if continuation_token.is_none() { break; } } - if cleanup_failures.is_empty() { - println!(" ✓ Config artifacts removed from S3"); - } else { - anyhow::bail!( - "post-delete cleanup incomplete (safe to retry): {}", - cleanup_failures.join("; ") - ); + println!(" ✓ Config artifacts removed from S3"); + Ok(()) +} + +async fn run_with_bucket( + aws_config: &aws_config::SdkConfig, + resource: &str, + name: &str, + cluster: &str, + namespace: &str, + bucket: &str, + policy: DeletePolicy, +) -> Result> { + if resource != "oabservice" { + anyhow::bail!("unknown resource type: {resource}. Use 'oabservice'"); } + let ecs = aws_sdk_ecs::Client::new(aws_config); + let s3 = aws_sdk_s3::Client::new(aws_config); + println!("Deleting {name}..."); + match prepare_identity( + aws_config, &ecs, &s3, namespace, name, cluster, bucket, policy, + ).await? { + PreparedIdentity::Exact(checkpoint, state) => { + delete_checkpointed_ecs(&ecs, &checkpoint, state).await?; + crate::ingress::delete_exact( + aws_config, + namespace, + name, + checkpoint.api_id.as_deref(), + checkpoint.registry_arn.as_deref(), + &checkpoint.partition, + &checkpoint.account, + &checkpoint.region, + ) + .await?; + cleanup_s3(&s3, bucket, namespace, name).await?; + let key = checkpoint_key(namespace, name); + s3.delete_object().bucket(bucket).key(&key).send().await + .with_context(|| format!( + "cleanup completed but failed to remove s3://{bucket}/{key}" + ))?; + println!(" ✓ Delete checkpoint removed"); + } + PreparedIdentity::LegacyOrphan => { + // CLI-only compatibility. Never delete Cloud Map without an ECS registry ARN. + crate::ingress::delete_api(aws_config, namespace, name).await?; + cleanup_s3(&s3, bucket, namespace, name).await?; + } + } println!("\n✓ {name} deleted"); - Ok(warnings) + Ok(Vec::new()) } - #[cfg(test)] mod tests { use super::*; @@ -566,12 +969,35 @@ mod tests { .build() } + fn checkpoint() -> DeleteCheckpoint { + DeleteCheckpoint { + version: DELETE_CHECKPOINT_VERSION, + namespace: "prod".to_string(), + name: "bot".to_string(), + bucket: "control-plane".to_string(), + partition: "aws".to_string(), + account: "123456789012".to_string(), + region: "us-east-1".to_string(), + requested_cluster: "cluster".to_string(), + cluster_arn: "arn:aws:ecs:us-east-1:123456789012:cluster/cluster".to_string(), + service_arn: "arn:aws:ecs:us-east-1:123456789012:service/cluster/oab-prod-bot".to_string(), + registry_arn: Some( + "arn:aws:servicediscovery:us-east-1:123456789012:service/srv-123".to_string(), + ), + api_id: Some("api-123".to_string()), + } + } + #[tokio::test] async fn delete_services_rejects_empty_target_set() { let cfg = test_sdk_config(); - let err = delete_services(&cfg, &[], &DeleteOptions::new("cluster")) - .await - .unwrap_err(); + let err = delete_services( + &cfg, + &[], + &DeleteOptions::new("cluster", "control-plane"), + ) + .await + .unwrap_err(); assert_eq!(err.kind, DeleteErrorKind::Validation); assert!(err.to_string().contains("empty target set"), "{err}"); } @@ -580,80 +1006,279 @@ mod tests { async fn delete_services_rejects_empty_cluster() { let cfg = test_sdk_config(); let targets = [DeleteTarget::new("prod", "bot")]; - let err = delete_services(&cfg, &targets, &DeleteOptions::new("")) + let err = delete_services( + &cfg, + &targets, + &DeleteOptions::new("", "control-plane"), + ) + .await + .unwrap_err(); + assert_eq!(err.kind, DeleteErrorKind::Validation); + assert!(err.to_string().contains("cluster must not be empty"), "{err}"); + } + + #[tokio::test] + async fn delete_services_rejects_blank_bucket_before_aws() { + let cfg = test_sdk_config(); + let targets = [DeleteTarget::new("prod", "bot")]; + let err = delete_services(&cfg, &targets, &DeleteOptions::new("cluster", " ")) .await .unwrap_err(); assert_eq!(err.kind, DeleteErrorKind::Validation); - assert!(err.to_string().contains("cluster must not be empty"), "{err}"); + assert!(err.to_string().contains("control_plane_bucket"), "{err}"); } #[tokio::test] async fn delete_services_rejects_blank_target_fields() { let cfg = test_sdk_config(); let targets = [DeleteTarget::new("prod", " ")]; - let err = delete_services(&cfg, &targets, &DeleteOptions::new("cluster")) - .await - .unwrap_err(); + let err = delete_services( + &cfg, + &targets, + &DeleteOptions::new("cluster", "control-plane"), + ) + .await + .unwrap_err(); assert_eq!(err.kind, DeleteErrorKind::Validation); } #[test] - fn delete_target_derives_ecs_service_name() { + fn initial_describe_failures_and_missing_service_fail_closed() { + assert!(classify_service_identity(false, false, None, &["MISSING"]).is_err()); + assert!(classify_service_identity(false, false, None, &["ACCESS_DENIED"]).is_err()); + assert!(classify_service_identity(false, false, None, &[]).is_err()); + assert!(classify_service_identity(false, true, Some("INACTIVE"), &[]).is_err()); + assert!(classify_service_identity(false, true, None, &[]).is_err()); + } + + #[test] + fn exact_retry_checkpoint_authorizes_only_unambiguous_missing_identity() { assert_eq!( - DeleteTarget::new("prod", "nest-my-oab").ecs_service_name(), - "oab-prod-nest-my-oab" + classify_service_identity(true, false, None, &["MISSING"]).unwrap(), + ServiceIdentityState::RetryGone + ); + assert_eq!( + classify_service_identity(true, true, Some("INACTIVE"), &[]).unwrap(), + ServiceIdentityState::RetryGone + ); + assert!(classify_service_identity(true, false, None, &["ACCESS_DENIED"]).is_err()); + assert!(classify_service_identity( + true, + false, + None, + &["MISSING", "ACCESS_DENIED"] + ) + .is_err()); + assert!(classify_service_identity( + true, + true, + Some("DRAINING"), + &["MISSING"] + ) + .is_err()); + assert!(classify_service_identity(true, true, None, &[]).is_err()); + assert_eq!( + classify_service_identity(true, true, Some("DRAINING"), &[]).unwrap(), + ServiceIdentityState::Live ); } #[test] - fn drain_poll_action_requires_completion_before_cleanup() { - assert_eq!(drain_poll_action(true, 0, 12), DrainPollAction::Complete); - assert_eq!(drain_poll_action(false, 0, 12), DrainPollAction::Retry); - assert_eq!(drain_poll_action(false, 11, 12), DrainPollAction::TimedOut); + fn checkpoint_rejects_boundary_mismatch_and_accepts_exact_retry() { + let checkpoint = checkpoint(); + validate_checkpoint( + &checkpoint, + "prod", + "bot", + "cluster", + "control-plane", + "aws", + "123456789012", + "us-east-1", + ) + .unwrap(); + validate_checkpoint( + &checkpoint, + "prod", + "bot", + &checkpoint.cluster_arn, + "control-plane", + "aws", + "123456789012", + "us-east-1", + ) + .unwrap(); + assert!(validate_checkpoint( + &checkpoint, + "prod", + "bot", + "cluster", + "other-bucket", + "aws", + "123456789012", + "us-east-1", + ) + .is_err()); + assert!(validate_checkpoint( + &checkpoint, + "prod", + "bot", + "cluster", + "control-plane", + "aws", + "999999999999", + "us-east-1", + ) + .is_err()); + + let mut mismatched_cluster = checkpoint.clone(); + mismatched_cluster.requested_cluster = "other-cluster".to_string(); + assert!(validate_checkpoint( + &mismatched_cluster, + "prod", + "bot", + "cluster", + "control-plane", + "aws", + "123456789012", + "us-east-1", + ) + .is_err()); + assert!(validate_checkpoint( + &checkpoint, + "prod", + "bot", + "other-cluster", + "control-plane", + "aws", + "123456789012", + "us-east-1", + ) + .is_err()); + + let mut mismatched_partition = checkpoint.clone(); + mismatched_partition.partition = "aws-cn".to_string(); + assert!(validate_checkpoint( + &mismatched_partition, + "prod", + "bot", + "cluster", + "control-plane", + "aws", + "123456789012", + "us-east-1", + ) + .is_err()); + + let mut invalid_dependency = checkpoint.clone(); + invalid_dependency.registry_arn = None; + assert!(validate_checkpoint( + &invalid_dependency, + "prod", + "bot", + "cluster", + "control-plane", + "aws", + "123456789012", + "us-east-1", + ) + .is_err()); + + invalid_dependency.api_id = None; + invalid_dependency.registry_arn = Some("srv-not-an-arn".to_string()); + assert!(validate_checkpoint( + &invalid_dependency, + "prod", + "bot", + "cluster", + "control-plane", + "aws", + "123456789012", + "us-east-1", + ) + .is_err()); } #[test] - fn best_effort_warnings_preserve_step_warnings_and_errors() { - let mut warnings = Vec::new(); - collect_best_effort_warnings( - &mut warnings, - Ok(vec!["route cleanup incomplete".to_string()]), - "ingress teardown skipped", - ); - collect_best_effort_warnings( - &mut warnings, - Err(anyhow::anyhow!("access denied")), - "HTTP API cleanup skipped", - ); + fn checkpoint_rejects_arn_boundary_mismatches() { + let cases = [ + ( + "arn:aws:ecs:us-east-1:999999999999:cluster/cluster", + "arn:aws:ecs:us-east-1:123456789012:service/cluster/oab-prod-bot", + "arn:aws:servicediscovery:us-east-1:123456789012:service/srv-123", + ), + ( + "arn:aws:ecs:us-west-2:123456789012:cluster/cluster", + "arn:aws:ecs:us-east-1:123456789012:service/cluster/oab-prod-bot", + "arn:aws:servicediscovery:us-east-1:123456789012:service/srv-123", + ), + ( + "arn:aws:ecs:us-east-1:123456789012:cluster/cluster", + "arn:aws:ecs:us-east-1:123456789012:service/other/oab-prod-bot", + "arn:aws:servicediscovery:us-east-1:123456789012:service/srv-123", + ), + ( + "arn:aws:ecs:us-east-1:123456789012:cluster/cluster", + "arn:aws:ecs:us-east-1:123456789012:service/cluster/oab-prod-other", + "arn:aws:servicediscovery:us-east-1:123456789012:service/srv-123", + ), + ( + "arn:aws:ecs:us-east-1:123456789012:cluster/cluster", + "arn:aws:ecs:us-east-1:123456789012:service/cluster/oab-prod-bot", + "arn:aws:servicediscovery:us-east-1:999999999999:service/srv-123", + ), + ( + "arn:aws:ecs:us-east-1:123456789012:cluster/cluster", + "arn:aws:ecs:us-east-1:123456789012:service/cluster/oab-prod-bot", + "arn:aws:servicediscovery:us-west-2:123456789012:service/srv-123", + ), + ( + "arn:aws:ecs:us-east-1:123456789012:cluster/cluster", + "arn:aws-cn:ecs:us-east-1:123456789012:service/cluster/oab-prod-bot", + "arn:aws:servicediscovery:us-east-1:123456789012:service/srv-123", + ), + ]; - assert_eq!( - warnings, - vec![ - "route cleanup incomplete", - "HTTP API cleanup skipped: access denied", - ] - ); + for (cluster_arn, service_arn, registry_arn) in cases { + let mut candidate = checkpoint(); + candidate.cluster_arn = cluster_arn.to_string(); + candidate.service_arn = service_arn.to_string(); + candidate.registry_arn = Some(registry_arn.to_string()); + assert!(validate_checkpoint( + &candidate, + "prod", + "bot", + "cluster", + "control-plane", + "aws", + "123456789012", + "us-east-1", + ) + .is_err()); + } } #[test] - fn delete_phase_requests_delete_only_for_active_service() { - assert_eq!( - ecs_delete_phase(Some("ACTIVE")).unwrap(), - EcsDeletePhase::Delete - ); - assert_eq!( - ecs_delete_phase(Some("DRAINING")).unwrap(), - EcsDeletePhase::Drain - ); + fn delete_target_derives_ecs_service_name() { assert_eq!( - ecs_delete_phase(Some("INACTIVE")).unwrap(), - EcsDeletePhase::Cleanup + DeleteTarget::new("prod", "nest-my-oab").ecs_service_name(), + "oab-prod-nest-my-oab" ); - assert_eq!(ecs_delete_phase(None).unwrap(), EcsDeletePhase::Cleanup); } #[test] - fn delete_phase_rejects_unknown_status() { + fn drain_poll_action_requires_completion_before_cleanup() { + assert_eq!(drain_poll_action(true, 0, 12), DrainPollAction::Complete); + assert_eq!(drain_poll_action(false, 0, 12), DrainPollAction::Retry); + assert_eq!(drain_poll_action(false, 11, 12), DrainPollAction::TimedOut); + } + + #[test] + fn delete_phase_requests_delete_only_for_active_service() { + assert_eq!(ecs_delete_phase(Some("ACTIVE")).unwrap(), EcsDeletePhase::Delete); + assert_eq!(ecs_delete_phase(Some("DRAINING")).unwrap(), EcsDeletePhase::Drain); + assert_eq!(ecs_delete_phase(Some("INACTIVE")).unwrap(), EcsDeletePhase::Cleanup); + assert_eq!(ecs_delete_phase(None).unwrap(), EcsDeletePhase::Cleanup); assert!(ecs_delete_phase(Some("UNKNOWN")).is_err()); } } diff --git a/operator/src/ingress.rs b/operator/src/ingress.rs index 136f03167..779423df7 100644 --- a/operator/src/ingress.rs +++ b/operator/src/ingress.rs @@ -26,6 +26,7 @@ use crate::manifest::{Ingress, OABServiceManifest}; use anyhow::{Context, Result}; +use aws_sdk_apigatewayv2::error::ProvideErrorMetadata; use aws_sdk_apigatewayv2::types::{ConnectionType, IntegrationType, ProtocolType}; use aws_sdk_servicediscovery::types::{DnsConfig, DnsRecord, RecordType}; use std::collections::HashMap; @@ -266,11 +267,44 @@ fn has_stage_path_override(request_parameters: Option<&HashMap>) /// Extract the Cloud Map service ID from its ARN /// (`arn:aws:servicediscovery:::service/`). -fn cloud_map_service_id_from_arn(arn: &str) -> Option { - arn.rsplit('/') - .next() - .filter(|s| !s.is_empty()) - .map(|s| s.to_string()) +pub(crate) fn cloud_map_service_id_from_arn(arn: &str) -> Option { + let mut parts = arn.splitn(6, ':'); + if parts.next() != Some("arn") + || parts.next().filter(|part| !part.is_empty()).is_none() + || parts.next() != Some("servicediscovery") + || parts.next().filter(|part| !part.is_empty()).is_none() + || parts.next().filter(|part| !part.is_empty()).is_none() + { + return None; + } + let resource = parts.next()?; + let service_id = resource.strip_prefix("service/")?; + if service_id.is_empty() || service_id.contains('/') { + return None; + } + Some(service_id.to_string()) +} + +fn cloud_map_service_id_for_boundary( + arn: &str, + expected_partition: &str, + expected_account: &str, + expected_region: &str, +) -> Option { + let mut parts = arn.splitn(6, ':'); + if parts.next() != Some("arn") + || parts.next() != Some(expected_partition) + || parts.next() != Some("servicediscovery") + || parts.next() != Some(expected_region) + || parts.next() != Some(expected_account) + { + return None; + } + let service_id = parts.next()?.strip_prefix("service/")?; + if service_id.is_empty() || service_id.contains('/') { + return None; + } + Some(service_id.to_string()) } /// Build the public webhook URL(s) from the API endpoint and paths. @@ -283,205 +317,207 @@ fn webhook_urls(api_endpoint: &str, paths: &[String]) -> Vec { .collect() } -/// Best-effort teardown of the *per-bot ingress wiring* for `namespace/name`: -/// its routes, integration, and stage on the per-bot HTTP API, plus its Cloud -/// Map service. Deliberately does NOT delete the HTTP API resource itself — -/// only what points at the now-gone task — so the API's `api-id` (and thus the -/// public webhook URL's hostname) survives an ECS-service recreate cycle. Use -/// [`delete_api`] separately when the bot is being permanently removed. +/// Exact-identity teardown of per-bot API Gateway wiring for apply-time ingress +/// removal. The ECS registry ARN selects the unique same-name HTTP API whose +/// integration URI equals that ARN. Without an ECS registry ARN this function +/// returns a warning and leaves resources untouched; it never falls back to a +/// name-only match. /// -/// The shared resources (the VPC Link and the security-group inbound rule) are -/// intentionally left in place since other bots may still use them. Safe to -/// call for bots that never had ingress — it simply finds nothing and returns. -/// Errors that prevent teardown are propagated. Degraded cleanup that can be -/// completed manually is returned as warning text so apply can include it in -/// its structured report while the CLI still renders it. +/// The HTTP API resource itself is retained so its endpoint survives if ingress +/// is re-enabled. Cloud Map must be deleted separately with +/// [`delete_cloud_map_exact`] only after ECS has detached the registry. Shared +/// VPC Link and security-group resources are also left in place. pub async fn teardown( config: &aws_config::SdkConfig, namespace: &str, name: &str, known_registry_arn: Option<&str>, ) -> Result> { - let mut warnings = Vec::new(); - let service_name = format!("oab-{namespace}-{name}"); - let api = aws_sdk_apigatewayv2::Client::new(config); + let Some(registry_arn) = known_registry_arn else { + let warning = format!( + "ingress teardown left resources untouched for {namespace}/{name}: ECS returned no exact registry ARN" + ); + eprintln!(" ⚠ {warning}"); + return Ok(vec![warning]); + }; - // ── API Gateway: strip routes + integration + stage, keep the API itself ─ - if let Some((api_id, _)) = find_api(&api, &api_name(namespace, name)).await? { - // Delete all routes on this API first (integrations can't be deleted - // while a route still targets them). - let mut route_ids = Vec::new(); - let mut next: Option = None; - loop { - let mut req = api.get_routes().api_id(&api_id); - if let Some(t) = &next { - req = req.next_token(t); - } - let resp = req.send().await.context("failed to list routes")?; - for r in resp.items() { - if let Some(id) = r.route_id() { - route_ids.push(id.to_string()); - } - } - match resp.next_token() { - Some(t) => next = Some(t.to_string()), - None => break, - } - } - for route_id in &route_ids { - if let Err(error) = api - .delete_route() - .api_id(&api_id) - .route_id(route_id) - .send() - .await - { - let warning = format!( - "failed to delete ingress route {route_id} from HTTP API {api_id}: {error}" - ); - eprintln!(" ⚠ {warning}"); - warnings.push(warning); - } - } + let api_id = resolve_api_id_for_registry(config, namespace, name, registry_arn).await?; + if let Some(api_id) = api_id.as_deref() { + clear_api_wiring(config, api_id).await?; + eprintln!(" ✓ Cleared exact ingress wiring on HTTP API {api_id}"); + } + Ok(Vec::new()) +} - // Delete integrations (there's normally just one, but clean up all). - let mut integration_ids = Vec::new(); - let mut next: Option = None; - loop { - let mut req = api.get_integrations().api_id(&api_id); - if let Some(t) = &next { - req = req.next_token(t); - } - let resp = req.send().await.context("failed to list integrations")?; - for i in resp.items() { - if let Some(id) = i.integration_id() { - integration_ids.push(id.to_string()); - } - } - match resp.next_token() { - Some(t) => next = Some(t.to_string()), - None => break, - } - } - for integration_id in &integration_ids { - if let Err(error) = api - .delete_integration() - .api_id(&api_id) - .integration_id(integration_id) - .send() - .await - { - let warning = format!( - "failed to delete ingress integration {integration_id} from HTTP API {api_id}: {error}" - ); - eprintln!(" ⚠ {warning}"); - warnings.push(warning); - } +async fn clear_api_wiring(config: &aws_config::SdkConfig, api_id: &str) -> Result<()> { + let api = aws_sdk_apigatewayv2::Client::new(config); + let mut route_ids = Vec::new(); + let mut next = None; + loop { + let mut request = api.get_routes().api_id(api_id); + if let Some(token) = &next { request = request.next_token(token); } + let response = request.send().await.context("failed to list exact API routes")?; + route_ids.extend(response.items().iter().filter_map(|route| route.route_id().map(str::to_owned))); + next = response.next_token().map(str::to_owned); + if next.is_none() { break; } + } + for route_id in route_ids { + match api.delete_route().api_id(api_id).route_id(&route_id).send().await { + Ok(_) => {} + Err(error) if error.code() == Some("NotFoundException") => {} + Err(error) => return Err(error).context("failed to delete exact API route"), } + } - if let Err(error) = api - .delete_stage() - .api_id(&api_id) - .stage_name(STAGE_NAME) - .send() - .await - { - let warning = format!( - "failed to delete ingress stage {STAGE_NAME} from HTTP API {api_id}: {error}" - ); - eprintln!(" ⚠ {warning}"); - warnings.push(warning); + let mut integration_ids = Vec::new(); + let mut next = None; + loop { + let mut request = api.get_integrations().api_id(api_id); + if let Some(token) = &next { request = request.next_token(token); } + let response = request.send().await.context("failed to list exact API integrations")?; + integration_ids.extend(response.items().iter().filter_map(|item| item.integration_id().map(str::to_owned))); + next = response.next_token().map(str::to_owned); + if next.is_none() { break; } + } + for integration_id in integration_ids { + match api.delete_integration().api_id(api_id).integration_id(&integration_id).send().await { + Ok(_) => {} + Err(error) if error.code() == Some("NotFoundException") => {} + Err(error) => return Err(error).context("failed to delete exact API integration"), } + } + match api.delete_stage().api_id(api_id).stage_name(STAGE_NAME).send().await { + Ok(_) => {} + Err(error) if error.code() == Some("NotFoundException") => {} + Err(error) => return Err(error).context("failed to delete exact API stage"), + } + Ok(()) +} - if warnings.is_empty() { - eprintln!( - " ✓ Cleared ingress wiring on HTTP API {} ({} route(s), {} integration(s)) — API itself kept so its URL survives a recreate", - api_name(namespace, name), - route_ids.len(), - integration_ids.len() - ); - } else { - eprintln!( - " ⚠ Ingress wiring cleanup on HTTP API {} completed with {} warning(s)", - api_name(namespace, name), - warnings.len() - ); +async fn delete_cloud_map_by_arn( + config: &aws_config::SdkConfig, + registry_arn: &str, + service_name: &str, + expected_boundary: Option<(&str, &str, &str)>, +) -> Result<()> { + let service_id = match expected_boundary { + Some((partition, account, region)) => { + cloud_map_service_id_for_boundary(registry_arn, partition, account, region) } + None => cloud_map_service_id_from_arn(registry_arn), } - - // ── Cloud Map: delete the per-bot service (needs no live instances) ────── - // Prefer resolving the exact service from the ECS service's own registry - // ARN (passed by the caller when known) over a name-only account-wide - // scan — two bots with the same namespace/name in different VPCs (e.g. - // staging vs. prod sharing an account) would otherwise collide and the - // wrong one could be deleted. - let sd = aws_sdk_servicediscovery::Client::new(config); - let service_id: Option = if let Some(arn) = known_registry_arn { - cloud_map_service_id_from_arn(arn) - } else { - let mut found: Option = None; - let mut pages = sd.list_services().into_paginator().send(); - 'svc: while let Some(page) = pages.next().await { - let page = page.context("failed to list Cloud Map services")?; - for s in page.services() { - if s.name() == Some(service_name.as_str()) { - found = s.id().map(|x| x.to_string()); - break 'svc; - } + .context("Cloud Map service ARN is outside the exact delete boundary")?; + let discovery = aws_sdk_servicediscovery::Client::new(config); + let mut last_error = None; + for attempt in 0..6 { + if attempt > 0 { tokio::time::sleep(std::time::Duration::from_secs(5)).await; } + match discovery.delete_service().id(&service_id).send().await { + Ok(_) => { + eprintln!(" ✓ Deleted Cloud Map service: {service_name} ({service_id})"); + return Ok(()); } + Err(error) if error.code() == Some("ResourceNotFoundException") => return Ok(()), + Err(error) => last_error = Some(error), } - found + } + Err(anyhow::Error::new(last_error.expect("delete attempt always records an error"))) + .with_context(|| format!("failed to delete exact Cloud Map service {service_id}")) +} + + +/// Delete the exact Cloud Map service selected by an ECS registry ARN. +/// Apply calls this only after it has observed the registry detached from ECS. +pub(crate) async fn delete_cloud_map_exact( + config: &aws_config::SdkConfig, + registry_arn: &str, + service_name: &str, +) -> Result<()> { + delete_cloud_map_by_arn(config, registry_arn, service_name, None).await +} +/// Resolve a same-name API only when the name identifies exactly one resource +/// and that API has an integration URI equal to the exact ECS registry ARN. +/// Duplicate names are rejected before inspecting integrations, even if only +/// one duplicate currently points at the registry. +pub(crate) async fn resolve_api_id_for_registry( + config: &aws_config::SdkConfig, + namespace: &str, + name: &str, + registry_arn: &str, +) -> Result> { + let api = aws_sdk_apigatewayv2::Client::new(config); + let expected_name = api_name(namespace, name); + let candidates = find_apis(&api, &expected_name).await?; + let Some((api_id, _)) = select_unique_named_api(&expected_name, &candidates)? else { + return Ok(None); }; - if let Some(service_id) = service_id { - // ECS deregisters the task's Cloud Map instance asynchronously when a - // service scales to 0 / is deleted, so `delete_service` can fail with - // "still has registered instances" for a short window even though the - // task is already gone. Retry briefly instead of giving up on the - // first attempt — this is the common case, not an edge case. - let mut last_err = None; - let mut deleted = false; - for attempt in 0..6 { - if attempt > 0 { - tokio::time::sleep(std::time::Duration::from_secs(5)).await; - } - match sd.delete_service().id(&service_id).send().await { - Ok(_) => { - eprintln!(" ✓ Deleted Cloud Map service: {service_name}"); - deleted = true; - break; - } - Err(e) => last_err = Some(e), - } + + let mut next = None; + let mut matched = false; + loop { + let mut request = api.get_integrations().api_id(&api_id); + if let Some(token) = &next { + request = request.next_token(token); } - if !deleted { - let warning = format!( - "Cloud Map service '{service_name}' was not deleted after retrying; it still has registered instances. Remove it manually with `aws servicediscovery delete-service --id {service_id}` ({})", - last_err.map(|error| error.to_string()).unwrap_or_default() - ); - eprintln!(" ⚠ {warning}"); - warnings.push(warning); + let response = request + .send() + .await + .with_context(|| format!("failed to list integrations for API {api_id}"))?; + matched |= response + .items() + .iter() + .any(|integration| integration.integration_uri() == Some(registry_arn)); + next = response.next_token().map(str::to_owned); + if next.is_none() { + break; } } - Ok(warnings) + Ok(matched.then_some(api_id)) } -/// Permanently delete the bot's per-bot HTTP API (`oab-webhook--`), -/// cascading its routes/integration/stage with it. This DESTROYS the `api-id` -/// and therefore the public webhook URL's hostname — only call this when the -/// bot itself is being permanently removed (`oabctl delete`), never from the -/// `apply` recreate path, which relies on the API surviving so its URL stays -/// stable across an ECS-service recreate. +/// Permanently delete only checkpointed dependent identities. Exact-ID +/// NotFound is idempotent success; every other failure is fatal so the caller +/// retains its durable checkpoint for retry. +pub(crate) async fn delete_exact( + config: &aws_config::SdkConfig, + namespace: &str, + name: &str, + api_id: Option<&str>, + registry_arn: Option<&str>, + partition: &str, + account: &str, + region: &str, +) -> Result<()> { + if let Some(api_id) = api_id { + let api = aws_sdk_apigatewayv2::Client::new(config); + match api.delete_api().api_id(api_id).send().await { + Ok(_) => eprintln!(" ✓ Deleted exact HTTP API: {api_id}"), + Err(error) if error.code() == Some("NotFoundException") => {} + Err(error) => return Err(error).context("failed to delete exact HTTP API"), + } + } + if let Some(registry_arn) = registry_arn { + delete_cloud_map_by_arn( + config, + registry_arn, + &format!("oab-{namespace}-{name}"), + Some((partition, account, region)), + ) + .await?; + } + Ok(()) +} + +/// CLI-only legacy orphan cleanup by name. Duplicate names fail closed. pub async fn delete_api(config: &aws_config::SdkConfig, namespace: &str, name: &str) -> Result<()> { let api = aws_sdk_apigatewayv2::Client::new(config); let name_str = api_name(namespace, name); if let Some((api_id, _)) = find_api(&api, &name_str).await? { - api.delete_api() - .api_id(&api_id) - .send() - .await - .with_context(|| format!("failed to delete HTTP API {api_id}"))?; - eprintln!(" ✓ Deleted HTTP API: {name_str}"); + match api.delete_api().api_id(&api_id).send().await { + Ok(_) => eprintln!(" ✓ Deleted HTTP API: {name_str}"), + Err(error) if error.code() == Some("NotFoundException") => {} + Err(error) => return Err(error).with_context(|| format!("failed to delete HTTP API {api_id}")), + } } Ok(()) } @@ -877,12 +913,21 @@ async fn ensure_api( Ok((id, endpoint)) } -/// Find an HTTP API by name, returning `(api_id, api_endpoint)`. +/// Find an HTTP API by name, rejecting duplicate names instead of selecting +/// whichever candidate AWS happens to return first. async fn find_api( api: &aws_sdk_apigatewayv2::Client, api_name: &str, ) -> Result> { - // apigatewayv2 has no smithy paginator for GetApis; page manually. + let candidates = find_apis(api, api_name).await?; + select_unique_named_api(api_name, &candidates) +} + +async fn find_apis( + api: &aws_sdk_apigatewayv2::Client, + api_name: &str, +) -> Result> { + let mut candidates = Vec::new(); let mut next: Option = None; loop { let mut req = api.get_apis(); @@ -890,20 +935,50 @@ async fn find_api( req = req.next_token(t); } let resp = req.send().await.context("failed to list APIs")?; - for a in resp.items() { - if a.name() == Some(api_name) { - let id = a.api_id().context("api missing id")?.to_string(); - let endpoint = a.api_endpoint().unwrap_or_default().to_string(); - return Ok(Some((id, endpoint))); + for candidate in resp.items() { + if is_matching_http_api( + candidate.name(), + candidate.protocol_type().map(ProtocolType::as_str), + api_name, + ) { + let id = candidate.api_id().context("api missing id")?.to_string(); + let endpoint = candidate.api_endpoint().unwrap_or_default().to_string(); + candidates.push((id, endpoint)); } } match resp.next_token() { - Some(t) => next = Some(t.to_string()), - None => return Ok(None), + Some(token) => next = Some(token.to_string()), + None => return Ok(candidates), } } } +fn is_matching_http_api( + candidate_name: Option<&str>, + candidate_protocol: Option<&str>, + expected_name: &str, +) -> bool { + candidate_name == Some(expected_name) && candidate_protocol == Some("HTTP") +} + +fn select_unique_named_api( + api_name: &str, + candidates: &[(String, String)], +) -> Result> { + match candidates { + [] => Ok(None), + [candidate] => Ok(Some(candidate.clone())), + _ => anyhow::bail!( + "multiple HTTP APIs named '{api_name}' exist; refusing first-match selection: {}", + candidates + .iter() + .map(|(id, _)| id.as_str()) + .collect::>() + .join(", ") + ), + } +} + async fn ensure_integration( api: &aws_sdk_apigatewayv2::Client, api_id: &str, @@ -1185,9 +1260,21 @@ mod tests { } #[test] - fn cloud_map_service_id_from_arn_rejects_empty() { + fn cloud_map_service_id_from_arn_rejects_non_service_arns() { assert_eq!(cloud_map_service_id_from_arn(""), None); - assert_eq!(cloud_map_service_id_from_arn("trailing/"), None); + assert_eq!(cloud_map_service_id_from_arn("srv-abc123"), None); + assert_eq!( + cloud_map_service_id_from_arn( + "arn:aws:servicediscovery:us-east-1:123456789012:namespace/ns-123" + ), + None + ); + assert_eq!( + cloud_map_service_id_from_arn( + "arn:aws:servicediscovery:us-east-1:123456789012:service/" + ), + None + ); } #[test] @@ -1238,4 +1325,29 @@ mod tests { vec!["https://abc123.example.com/prod/webhook/telegram"] ); } + + #[test] + fn api_candidates_require_http_protocol() { + let name = "oab-webhook-prod-bot"; + assert!(is_matching_http_api(Some(name), Some("HTTP"), name)); + assert!(!is_matching_http_api(Some(name), Some("WEBSOCKET"), name)); + assert!(!is_matching_http_api(Some("other"), Some("HTTP"), name)); + assert!(!is_matching_http_api(Some(name), None, name)); + } + + #[test] + fn duplicate_named_apis_fail_closed() { + let candidates = vec![ + ("api-a".to_string(), "https://a".to_string()), + ("api-b".to_string(), "https://b".to_string()), + ]; + assert!(select_unique_named_api("oab-webhook-prod-bot", &candidates).is_err()); + assert_eq!( + select_unique_named_api("oab-webhook-prod-bot", &candidates[..1]) + .unwrap() + .unwrap() + .0, + "api-a" + ); + } } diff --git a/operator/src/lib.rs b/operator/src/lib.rs index 9fe0248c5..d2a8e26e9 100644 --- a/operator/src/lib.rs +++ b/operator/src/lib.rs @@ -42,15 +42,17 @@ //! an ARN require the apply caller to have `secretsmanager:DescribeSecret`, so //! oabctl can resolve the name to the full ARN required by ECS. //! -//! Programmatic apply and delete never read `~/.oabctl/config.toml`. Use -//! [`ApplyOptions::with_control_plane_bucket`] / [`DeleteOptions::with_control_plane_bucket`] -//! for an explicit bucket; otherwise resolution uses `OAB_CONTROL_PLANE_BUCKET` -//! and then the caller's AWS account — the same chain for both, so delete -//! always cleans the bucket apply wrote to. +//! Programmatic apply and delete never read `~/.oabctl/config.toml`. +//! Apply may use [`ApplyOptions::with_control_plane_bucket`] or its documented +//! resolution chain. Delete is intentionally stricter: construct +//! [`DeleteOptions`] with both the cluster and exact control-plane bucket so a +//! destructive request cannot drift to another account-derived bucket. //! -//! [`delete_services`] tears down by `namespace`+`name` ([`DeleteTarget`]) with -//! the same explicit-cluster contract as apply. Teardown is resumable: rerun -//! the same target to continue after a partial failure. +//! [`delete_services`] tears down by `namespace`+`name` ([`DeleteTarget`]). +//! Before mutating ECS it durably records the caller partition/account/region, bucket, +//! canonical cluster/service ARNs, and exact ingress IDs in S3. Missing or +//! ambiguous ECS identity fails closed unless that matching checkpoint already +//! exists; retries use only checkpointed IDs and remove the checkpoint last. pub mod apply; mod bootstrap; From 7bdd2036f6a72216fcbb98f1d8ac238deaea5282 Mon Sep 17 00:00:00 2001 From: chaodu-agent <274062505+chaodu-agent@users.noreply.github.com> Date: Thu, 16 Jul 2026 18:44:16 -0400 Subject: [PATCH 04/20] fix(oabctl): satisfy clippy for the identity-checkpoint delete path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - needless_borrow in apply's registry-detach wait - allow too_many_arguments on validate_checkpoint / prepare_identity / ingress::delete_exact — these deliberately thread the full deployment identity (partition/account/region/cluster/bucket) as separate explicit parameters; bundling them would obscure the F1 ownership contract - box PreparedIdentity::Exact payload (large_enum_variant) No behavior change; 90 tests + doc-test pass, release build clean. --- operator/src/apply.rs | 2 +- operator/src/delete.rs | 11 +++++++---- operator/src/ingress.rs | 1 + 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/operator/src/apply.rs b/operator/src/apply.rs index 6ce4f6156..713400a22 100644 --- a/operator/src/apply.rs +++ b/operator/src/apply.rs @@ -1188,7 +1188,7 @@ async fn apply_ecs( } if let Some(checkpoint) = &teardown_checkpoint { - wait_for_registry_detach(&ecs, cluster, &service_name).await?; + wait_for_registry_detach(ecs, cluster, &service_name).await?; for registry_arn in &checkpoint.registry_arns { crate::ingress::delete_cloud_map_exact(config, registry_arn, &service_name) .await diff --git a/operator/src/delete.rs b/operator/src/delete.rs index 4aa193c0a..d1d7ef67b 100644 --- a/operator/src/delete.rs +++ b/operator/src/delete.rs @@ -324,6 +324,7 @@ fn checkpoint_key(namespace: &str, name: &str) -> String { format!("delete-checkpoints/{namespace}/{name}.json") } +#[allow(clippy::too_many_arguments)] fn validate_checkpoint( checkpoint: &DeleteCheckpoint, namespace: &str, @@ -649,10 +650,11 @@ fn single_described_service( } } enum PreparedIdentity { - Exact(DeleteCheckpoint, ServiceIdentityState), + Exact(Box<(DeleteCheckpoint, ServiceIdentityState)>), LegacyOrphan, } +#[allow(clippy::too_many_arguments)] async fn prepare_identity( aws_config: &aws_config::SdkConfig, ecs: &aws_sdk_ecs::Client, @@ -689,7 +691,7 @@ async fn prepare_identity( if let Some(service) = service { validate_returned_service(&checkpoint, service)?; } - return Ok(PreparedIdentity::Exact(checkpoint, state)); + return Ok(PreparedIdentity::Exact(Box::new((checkpoint, state)))); } let service_name = format!("oab-{namespace}-{name}"); @@ -761,7 +763,7 @@ async fn prepare_identity( &checkpoint.region, )?; save_checkpoint(s3, &checkpoint).await?; - Ok(PreparedIdentity::Exact(checkpoint, state)) + Ok(PreparedIdentity::Exact(Box::new((checkpoint, state)))) } fn validate_returned_service( @@ -929,7 +931,8 @@ async fn run_with_bucket( match prepare_identity( aws_config, &ecs, &s3, namespace, name, cluster, bucket, policy, ).await? { - PreparedIdentity::Exact(checkpoint, state) => { + PreparedIdentity::Exact(boxed) => { + let (checkpoint, state) = *boxed; delete_checkpointed_ecs(&ecs, &checkpoint, state).await?; crate::ingress::delete_exact( aws_config, diff --git a/operator/src/ingress.rs b/operator/src/ingress.rs index 779423df7..be208f327 100644 --- a/operator/src/ingress.rs +++ b/operator/src/ingress.rs @@ -478,6 +478,7 @@ pub(crate) async fn resolve_api_id_for_registry( /// Permanently delete only checkpointed dependent identities. Exact-ID /// NotFound is idempotent success; every other failure is fatal so the caller /// retains its durable checkpoint for retry. +#[allow(clippy::too_many_arguments)] pub(crate) async fn delete_exact( config: &aws_config::SdkConfig, namespace: &str, From be9b90971a5b00be5ce767908857da9dad438fec Mon Sep 17 00:00:00 2001 From: chaodu-agent <274062505+chaodu-agent@users.noreply.github.com> Date: Thu, 16 Jul 2026 22:44:30 +0000 Subject: [PATCH 05/20] fix(operator): bind delete cleanup to exact identity --- docs/oabctl.md | 32 ++-- operator/README.md | 32 ++-- operator/src/apply.rs | 372 ++++++++++++++++++++++++++++++++++++---- operator/src/delete.rs | 311 ++++++++++++++++++++++----------- operator/src/ingress.rs | 94 +++++++--- operator/src/lib.rs | 19 +- 6 files changed, 660 insertions(+), 200 deletions(-) diff --git a/docs/oabctl.md b/docs/oabctl.md index 6344950bf..b3b890a65 100644 --- a/docs/oabctl.md +++ b/docs/oabctl.md @@ -367,26 +367,26 @@ must still be registered manually in the LINE Developers console. > **Teardown:** `oabctl delete oabservice ` (or `oabctl delete -f `, > using the same file `apply -f` deployed it from) first captures the exact ECS > cluster/service ARNs, ECS registry ARN, matching HTTP API ID, configured -> control-plane bucket, and caller partition/account/region in -> `delete-checkpoints//.json`. The checkpoint is written before -> ECS mutation and removed only after all dependent and S3 cleanup succeeds, so -> rerunning the same command safely resumes a partial delete. ECS HTTP-200 -> failures, ambiguous drain responses, and a missing service fail closed unless -> a matching checkpoint already authorizes that exact retry. +> control-plane bucket, caller partition/account/region, and ECS service +> incarnation (`createdAt`) in `delete-checkpoints//.json`. The +> checkpoint is written before ECS mutation and removed only after all +> dependent and S3 cleanup succeeds, so rerunning the same command safely +> resumes a partial delete. ECS HTTP-200 failures, ambiguous drain responses, +> empty service responses, and a missing service fail closed; a matching checkpoint +> authorizes retry only when ECS explicitly reports exactly one `MISSING` +> failure with zero services, or an `INACTIVE` response from the original +> service incarnation. > > API cleanup never selects the first same-named API: duplicate names fail > closed, and the sole candidate is checkpointed only when its integration URI > equals the ECS registry ARN. Cloud Map is deleted only by the service ID -> parsed from a validated Cloud Map service ARN. Exact-ID NotFound is -> treated as already complete; other cleanup errors remain fatal so the -> checkpoint survives. For old CLI-created orphans that predate checkpoints, -> the CLI has an isolated compatibility path: it may remove a uniquely named -> API and S3 data, but refuses duplicate API names and never guesses a Cloud Map -> service by name. This compatibility behavior is not available through the -> programmatic `delete_services` API. -> -> If you edit a manifest to remove `spec.ingress` while keeping the bot, `apply` -> first stores every exact ECS registry ARN in +> parsed from a structurally and boundary-validated Cloud Map service ARN. +> Exact-ID NotFound is idempotent; other cleanup errors retain the checkpoint. +> There is no name-only API, Cloud Map, or S3 orphan cleanup path. If you edit +> a manifest to remove `spec.ingress` while keeping the bot, `apply` first +> stores every exact ECS registry ARN only when the previously stored OAB +> manifest owned ingress (or resumes an already-valid checkpoint); an arbitrary +> attached registry is never enough. > `ingress-teardown-checkpoints//.json`, clears only API wiring > bound to those ARNs, detaches all registries from ECS, waits until the detach > is observable, and only then deletes each exact Cloud Map service. It keeps diff --git a/operator/README.md b/operator/README.md index c4b0f4679..8ff7c34c0 100644 --- a/operator/README.md +++ b/operator/README.md @@ -101,7 +101,8 @@ async fn reconcile( delete_services( aws, &[DeleteTarget::new("prod", "bot")], - &DeleteOptions::new("production-cluster", "my-control-plane-bucket"), + &DeleteOptions::new("production-cluster") + .with_control_plane_bucket("my-control-plane-bucket"), ) .await?; Ok(()) @@ -116,20 +117,21 @@ target cluster exists and is `ACTIVE` before mutation, so the caller identity requires `ecs:DescribeClusters`. This ECS action does not support resource-level permissions; its IAM statement must use `Resource: "*"`. -The library never reads `~/.oabctl/config.toml`. Apply can explicitly override -its bucket with `ApplyOptions::with_control_plane_bucket(...)`; otherwise it -uses `OAB_CONTROL_PLANE_BUCKET` and then the caller account. Programmatic delete -is intentionally stricter and requires the exact bucket in -`DeleteOptions::new(cluster, bucket)` before any AWS call. Before deleting ECS, -it stores `delete-checkpoints//.json` in that bucket with the -caller partition/account/region, canonical ECS cluster/service ARNs, registry ARN, and -matching API ID. Duplicate API names fail closed; the sole same-name API is -checkpointed only when its integration URI equals the registry ARN. Missing or -ambiguous ECS identity fails closed without this record; retries use only its -immutable IDs and delete the checkpoint last. -The CLI resolves its configured bucket privately. Its compatibility cleanup for -pre-checkpoint orphans is isolated from `delete_services`, refuses duplicate API -names, and never guesses a Cloud Map service by name. +The library never reads `~/.oabctl/config.toml`. Apply and delete resolve the +control-plane bucket through the shared chain: an explicit +`with_control_plane_bucket(...)` override, then `OAB_CONTROL_PLANE_BUCKET`, then +`oab-control-plane-{account}` from the caller identity. Before deleting ECS, +`delete_services` stores the resolved bucket, caller partition/account/region, +canonical ECS cluster/service ARNs, the service `createdAt` incarnation, and +exact ingress IDs in `delete-checkpoints//.json`. The checkpoint +is written before ECS mutation and removed last. Initial DescribeServices +responses must identify exactly one expected live service; missing, empty-success, +ambiguous, or mixed-failure responses fail closed. Retries use only checkpointed +IDs after ECS explicitly reports exactly one `MISSING` failure with zero services +or a matching original `INACTIVE` incarnation. Duplicate APIs fail closed, +and a sole same-name API is checkpointed only when an integration URI exactly +matches the ECS registry ARN. +There is no name-only API, Cloud Map, or S3 orphan cleanup fallback. For `aws-sm://#`, a non-ARN `` requires the caller to have `secretsmanager:DescribeSecret`; full-ARN shorthand does not diff --git a/operator/src/apply.rs b/operator/src/apply.rs index 6ce4f6156..db4609563 100644 --- a/operator/src/apply.rs +++ b/operator/src/apply.rs @@ -5,6 +5,7 @@ use aws_sdk_ecs::types::{ AssignPublicIp, AwsVpcConfiguration, CapacityProviderStrategyItem, ContainerDefinition, KeyValuePair, NetworkConfiguration, RuntimePlatform, Secret, }; +use aws_sdk_s3::error::ProvideErrorMetadata; use aws_sdk_s3::primitives::ByteStream; use std::fmt; use std::path::Path; @@ -512,11 +513,76 @@ const INGRESS_TEARDOWN_CHECKPOINT_VERSION: u8 = 1; #[derive(Debug, Clone, PartialEq, Eq, serde::Serialize, serde::Deserialize)] struct IngressTeardownCheckpoint { version: u8, + bucket: String, cluster: String, service_name: String, + service_arn: String, + service_created_at: i128, registry_arns: Vec, } +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum ApplyServicePresence { + Absent, + Present, +} + +fn classify_apply_service_response( + service_count: usize, + failure_reasons: &[&str], +) -> Result { + match (service_count, failure_reasons) { + (1, []) => Ok(ApplyServicePresence::Present), + (0, [reason]) if reason.eq_ignore_ascii_case("MISSING") => { + Ok(ApplyServicePresence::Absent) + } + (0, failures) if failures.is_empty() => { + anyhow::bail!("ECS returned no service and no MISSING failure for apply") + } + _ => anyhow::bail!( + "ECS returned an ambiguous apply response: {service_count} service(s), {} failure(s)", + failure_reasons.len() + ), + } +} + +fn ingress_teardown_is_owned(previously_had_ingress: bool, checkpoint_present: bool) -> bool { + previously_had_ingress || checkpoint_present +} + +fn validate_apply_service_identity( + service_arn: &str, + cluster_arn: &str, + expected_service_name: &str, +) -> Result<()> { + let service_resource = service_arn + .split_once(":service/") + .and_then(|(_, resource)| resource.rsplit_once('/')) + .filter(|(_, name)| *name == expected_service_name) + .context("ECS apply response has a conflicting service ARN")?; + let service_parts: Vec<&str> = service_arn.splitn(6, ':').collect(); + let cluster_parts: Vec<&str> = cluster_arn.splitn(6, ':').collect(); + if service_parts.len() != 6 + || cluster_parts.len() != 6 + || service_parts[0] != "arn" + || cluster_parts[0] != "arn" + || service_parts[2] != "ecs" + || cluster_parts[2] != "ecs" + || service_parts[1..5] != cluster_parts[1..5] + { + anyhow::bail!("ECS apply response has an invalid cluster/service ARN boundary"); + } + let cluster_resource = cluster_arn + .split_once(":cluster/") + .map(|(_, resource)| resource) + .filter(|resource| !resource.is_empty() && !resource.contains('/')) + .context("ECS apply response has an invalid cluster ARN")?; + if service_resource.0 != cluster_resource { + anyhow::bail!("ECS apply response service ARN targets another cluster"); + } + Ok(()) +} + fn ingress_teardown_checkpoint_key(namespace: &str, name: &str) -> String { format!("ingress-teardown-checkpoints/{namespace}/{name}.json") } @@ -585,10 +651,94 @@ async fn remove_ingress_teardown_checkpoint( Ok(()) } +#[derive(Debug, Clone, PartialEq, Eq)] +struct ApplyServiceIdentity { + service_arn: String, + cluster_arn: String, + service_name: String, + created_at: i128, +} + +fn resolve_apply_service_identity( + service: &aws_sdk_ecs::types::Service, + expected_service_name: &str, +) -> Result { + let service_arn = service + .service_arn() + .filter(|value| !value.trim().is_empty()) + .context("ECS apply response missing service ARN")? + .to_string(); + let cluster_arn = service + .cluster_arn() + .filter(|value| !value.trim().is_empty()) + .context("ECS apply response missing cluster ARN")? + .to_string(); + validate_apply_service_identity(&service_arn, &cluster_arn, expected_service_name)?; + let service_name = service + .service_name() + .filter(|value| !value.trim().is_empty()) + .context("ECS apply response missing service name")?; + if service_name != expected_service_name { + anyhow::bail!("ECS apply response has a conflicting service name"); + } + let created_at = service + .created_at() + .map(|value| value.as_nanos()) + .context("ECS apply response missing createdAt incarnation")?; + Ok(ApplyServiceIdentity { + service_arn, + cluster_arn, + service_name: expected_service_name.to_string(), + created_at, + }) +} + +fn validate_expected_apply_service_identity( + expected: &ApplyServiceIdentity, + service: &aws_sdk_ecs::types::Service, +) -> Result<()> { + let actual = resolve_apply_service_identity(service, &expected.service_name)?; + if actual != *expected { + anyhow::bail!( + "ECS apply response belongs to a recreated same-name service or different cluster" + ); + } + Ok(()) +} + +async fn revalidate_apply_service_identity( + ecs: &aws_sdk_ecs::Client, + expected: &ApplyServiceIdentity, +) -> Result<()> { + let response = ecs + .describe_services() + .cluster(&expected.cluster_arn) + .services(&expected.service_arn) + .send() + .await + .context("failed to revalidate ECS service identity before update")?; + if !response.failures().is_empty() { + anyhow::bail!( + "ECS returned failure(s) while revalidating service identity before update: {:?}", + response.failures() + ); + } + let service = match response.services() { + [service] => service, + [] => anyhow::bail!( + "ECS service disappeared while revalidating identity before update" + ), + services => anyhow::bail!( + "ECS returned {} services while revalidating one update target", + services.len() + ), + }; + validate_expected_apply_service_identity(expected, service) +} + async fn wait_for_registry_detach( ecs: &aws_sdk_ecs::Client, - cluster: &str, - service_name: &str, + expected: &ApplyServiceIdentity, ) -> Result<()> { const ATTEMPTS: u32 = 12; const INTERVAL: std::time::Duration = std::time::Duration::from_secs(5); @@ -596,8 +746,8 @@ async fn wait_for_registry_detach( for attempt in 0..ATTEMPTS { let response = ecs .describe_services() - .cluster(cluster) - .services(service_name) + .cluster(&expected.cluster_arn) + .services(&expected.service_arn) .send() .await .context("failed to verify ECS service registry detach")?; @@ -609,14 +759,13 @@ async fn wait_for_registry_detach( } let service = match response.services() { [service] => service, - [] => anyhow::bail!( - "ECS service disappeared while verifying registry detach" - ), + [] => anyhow::bail!("ECS service disappeared while verifying registry detach"), services => anyhow::bail!( "ECS returned {} services for one registry-detach target", services.len() ), }; + validate_expected_apply_service_identity(expected, service)?; if service.service_registries().is_empty() { return Ok(()); } @@ -626,9 +775,11 @@ async fn wait_for_registry_detach( } anyhow::bail!( - "ECS service {service_name} still reports service registries after waiting; Cloud Map cleanup was not started" + "ECS service {} still reports service registries after waiting; Cloud Map cleanup was not started", + expected.service_name ) } + async fn apply_ecs( ecs: &aws_sdk_ecs::Client, s3: &aws_sdk_s3::Client, @@ -659,7 +810,7 @@ async fn apply_ecs( "manifests/{}/{}.yaml", m.metadata.namespace, m.metadata.name ); - let current_gen = match s3 + let (current_gen, previously_had_ingress) = match s3 .get_object() .bucket(bucket) .key(&manifest_key) @@ -669,9 +820,17 @@ async fn apply_ecs( Ok(resp) => { let bytes = resp.body.collect().await?.into_bytes(); let existing: OABServiceManifest = serde_yaml::from_slice(&bytes)?; - existing.metadata.generation + ( + existing.metadata.generation, + existing.spec.ingress.is_some(), + ) + } + Err(error) if matches!(error.code(), Some("NoSuchKey" | "NotFound")) => (0, false), + Err(error) => { + return Err(error).with_context(|| { + format!("failed to read existing manifest s3://{bucket}/{manifest_key}") + }); } - Err(_) => 0, }; let generation = current_gen + 1; @@ -683,19 +842,34 @@ async fn apply_ecs( .send() .await .context("failed to describe ECS service")?; - if !describe_resp.failures().is_empty() { - anyhow::bail!( - "ECS returned failure(s) while resolving apply identity: {:?}", - describe_resp.failures() - ); - } - let existing_service = match describe_resp.services() { - [] => None, - [service] => Some(service), - services => anyhow::bail!( - "ECS returned {} services for one apply target", - services.len() - ), + let failure_reasons: Vec<&str> = describe_resp + .failures() + .iter() + .map(|failure| failure.reason().unwrap_or("UNKNOWN")) + .collect(); + let existing_service = match classify_apply_service_response( + describe_resp.services().len(), + &failure_reasons, + )? { + ApplyServicePresence::Absent => None, + ApplyServicePresence::Present => { + let service = &describe_resp.services()[0]; + validate_apply_service_identity( + service + .service_arn() + .filter(|value| !value.trim().is_empty()) + .context("ECS apply response missing service ARN")?, + service + .cluster_arn() + .filter(|value| !value.trim().is_empty()) + .context("ECS apply response missing cluster ARN")?, + &service_name, + )?; + if !matches!(service.status(), Some("ACTIVE" | "DRAINING")) { + anyhow::bail!("ECS apply response has unexpected service status"); + } + Some(service) + } }; let mut existing_registry_arns = Vec::new(); if let Some(service) = existing_service { @@ -712,6 +886,10 @@ async fn apply_ecs( existing_registry_arns.sort(); existing_registry_arns.dedup(); let has_registries = !existing_registry_arns.is_empty(); + let service_active = existing_service.is_some(); + let existing_identity = existing_service + .map(|service| resolve_apply_service_identity(service, &service_name)) + .transpose()?; let stored_teardown = load_ingress_teardown_checkpoint( s3, @@ -725,23 +903,43 @@ async fn apply_ecs( "a previous ingress teardown is still pending; apply the ingress-free manifest again before re-enabling ingress" ); } - let teardown_checkpoint = if m.spec.ingress.is_none() { + let teardown_checkpoint = if m.spec.ingress.is_none() + && ingress_teardown_is_owned(previously_had_ingress, stored_teardown.is_some()) + { match stored_teardown { Some(checkpoint) => { if checkpoint.version != INGRESS_TEARDOWN_CHECKPOINT_VERSION + || checkpoint.bucket != bucket || checkpoint.cluster != cluster || checkpoint.service_name != service_name + || checkpoint.service_arn.trim().is_empty() + || checkpoint.service_created_at <= 0 || checkpoint.registry_arns.is_empty() { anyhow::bail!("ingress teardown checkpoint does not match this apply target"); } + if let Some(identity) = &existing_identity { + if identity.service_arn != checkpoint.service_arn + || identity.created_at != checkpoint.service_created_at + { + anyhow::bail!( + "ingress teardown checkpoint belongs to a different ECS service incarnation" + ); + } + } Some(checkpoint) } - None if has_registries => { + None if previously_had_ingress && has_registries => { + let identity = existing_identity + .clone() + .context("cannot checkpoint ingress teardown without an identified ECS service")?; let checkpoint = IngressTeardownCheckpoint { version: INGRESS_TEARDOWN_CHECKPOINT_VERSION, + bucket: bucket.to_string(), cluster: cluster.to_string(), service_name: service_name.clone(), + service_arn: identity.service_arn, + service_created_at: identity.created_at, registry_arns: existing_registry_arns.clone(), }; save_ingress_teardown_checkpoint( @@ -760,6 +958,29 @@ async fn apply_ecs( None }; + let expected_identity = if service_active { + let identity = existing_identity + .clone() + .context("cannot update ECS service without an exact identity")?; + if let Some(checkpoint) = &teardown_checkpoint { + if identity.service_arn != checkpoint.service_arn + || identity.created_at != checkpoint.service_created_at + { + anyhow::bail!( + "ingress teardown checkpoint belongs to a different ECS service incarnation" + ); + } + } + Some(identity) + } else { + if teardown_checkpoint.is_some() { + anyhow::bail!( + "cannot detach ingress from an absent ECS service; exact service identity is unavailable" + ); + } + None + }; + if let Some(checkpoint) = &teardown_checkpoint { eprintln!(" 🌐 ingress removed from manifest — tearing down exact API wiring..."); for registry_arn in &checkpoint.registry_arns { @@ -1024,10 +1245,6 @@ async fn apply_ecs( // Check if service exists. Reuses `describe_resp` captured above (before // the ingress-removal teardown) — `ensure_cloud_map` above doesn't touch // the ECS service, so its ACTIVE status can't have changed since then. - let service_active = describe_resp - .services() - .first() - .is_some_and(|service| service.status() == Some("ACTIVE")); let action; if service_active { @@ -1049,12 +1266,20 @@ async fn apply_ecs( // "clear"; only an explicit empty list detaches it. Without this the // service keeps pointing at the Cloud Map service that the // ingress-removal teardown (above) just deleted. - let needs_detach = cloud_map.is_none() && has_registries; + let needs_detach = teardown_checkpoint.is_some() && cloud_map.is_none() && has_registries; + let update_cluster = expected_identity + .as_ref() + .map(|identity| identity.cluster_arn.as_str()) + .unwrap_or(cluster); + let update_service = expected_identity + .as_ref() + .map(|identity| identity.service_arn.as_str()) + .unwrap_or(service_name.as_str()); let mut update_req = ecs .update_service() - .cluster(cluster) - .service(&service_name) + .cluster(update_cluster) + .service(update_service) .task_definition(&task_def_arn) .enable_execute_command(true) .network_configuration(network_config); @@ -1074,6 +1299,11 @@ async fn apply_ecs( update_req = update_req.set_service_registries(Some(Vec::new())); } + if let Some(expected) = &expected_identity { + // Revalidate immediately before the mutation. In particular, a + // recreated same-name service must never receive this update. + revalidate_apply_service_identity(ecs, expected).await?; + } update_req .send() .await @@ -1188,7 +1418,10 @@ async fn apply_ecs( } if let Some(checkpoint) = &teardown_checkpoint { - wait_for_registry_detach(&ecs, cluster, &service_name).await?; + let expected = expected_identity + .as_ref() + .context("missing exact identity for ingress detach wait")?; + wait_for_registry_detach(&ecs, expected).await?; for registry_arn in &checkpoint.registry_arns { crate::ingress::delete_cloud_map_exact(config, registry_arn, &service_name) .await @@ -1285,10 +1518,25 @@ async fn wait_for_stable(ecs: &aws_sdk_ecs::Client, cluster: &str, service: &str .await?; let elapsed = (i + 1) * 5; - let Some(svc) = resp.services().first() else { - eprintln!(" [{elapsed}s] service not found in describe-services response yet"); - continue; + if !resp.failures().is_empty() { + anyhow::bail!("ECS returned failure(s) while waiting for service stability: {:?}", resp.failures()); + } + let svc = match resp.services() { + [service] => service, + [] => { + eprintln!(" [{elapsed}s] service not found in describe-services response yet"); + continue; + } + services => anyhow::bail!( + "ECS returned {} services while waiting for one apply target", + services.len() + ), }; + validate_apply_service_identity( + svc.service_arn().context("ECS stability response missing service ARN")?, + svc.cluster_arn().context("ECS stability response missing cluster ARN")?, + service, + )?; let running = svc.running_count() as usize; let desired = svc.desired_count() as usize; @@ -1578,4 +1826,54 @@ spec: let result = resolve_task_role_arn(&manifest, bootstrap); assert_eq!(result, None); } + + #[test] + fn apply_accepts_only_single_missing_failure_as_first_absence() { + assert_eq!( + classify_apply_service_response(0, &["MISSING"]).unwrap(), + ApplyServicePresence::Absent + ); + assert!(classify_apply_service_response(0, &[]).is_err()); + assert!(classify_apply_service_response(0, &["MISSING", "ACCESS_DENIED"]).is_err()); + assert!(classify_apply_service_response(1, &["MISSING"]).is_err()); + } + + #[test] + fn apply_requires_one_service_for_present_identity() { + assert_eq!( + classify_apply_service_response(1, &[]).unwrap(), + ApplyServicePresence::Present + ); + assert!(classify_apply_service_response(2, &[]).is_err()); + assert!(validate_apply_service_identity( + "arn:aws:ecs:us-east-1:123456789012:service/cluster/oab-prod-bot", + "arn:aws:ecs:us-east-1:123456789012:cluster/cluster", + "oab-prod-bot", + ) + .is_ok()); + assert!(validate_apply_service_identity( + "arn:aws:ecs:us-east-1:123456789012:service/cluster/oab-prod-other", + "arn:aws:ecs:us-east-1:123456789012:cluster/cluster", + "oab-prod-bot", + ) + .is_err()); + } + + #[test] + fn apply_teardown_requires_manifest_ownership_or_checkpoint() { + assert!(ingress_teardown_is_owned(true, false)); + assert!(ingress_teardown_is_owned(false, true)); + assert!(!ingress_teardown_is_owned(false, false)); + } + + #[tokio::test] + async fn progress_gate_suppresses_nested_progress() { + assert!(progress_enabled()); + PROGRESS_ENABLED + .scope(false, async { + assert!(!progress_enabled()); + }) + .await; + assert!(progress_enabled()); + } } diff --git a/operator/src/delete.rs b/operator/src/delete.rs index 4aa193c0a..c55fbaf09 100644 --- a/operator/src/delete.rs +++ b/operator/src/delete.rs @@ -42,24 +42,31 @@ impl DeleteTarget { } } -/// Target options for [`delete_services`]. The library never reads CLI -/// configuration or guesses which control-plane bucket owns a deployment. +/// Target options for [`delete_services`]. Mirrors +/// [`ApplyOptions`](crate::apply::ApplyOptions): the cluster is required and +/// the library never reads `~/.oabctl/config.toml`. #[derive(Debug, Clone, PartialEq, Eq)] pub struct DeleteOptions { /// ECS cluster name or ARN the services were deployed into. pub cluster: String, - /// Exact S3 bucket that owns the deployment and its delete checkpoint. - pub control_plane_bucket: String, + /// Optional control-plane bucket override. When absent, resolution uses + /// `OAB_CONTROL_PLANE_BUCKET`, then `oab-control-plane-{account}` from + /// the caller's AWS identity, matching apply's shared resolver. + pub control_plane_bucket: Option, } impl DeleteOptions { - /// Bind a delete request to both its ECS cluster and control-plane bucket. - pub fn new(cluster: impl Into, control_plane_bucket: impl Into) -> Self { + pub fn new(cluster: impl Into) -> Self { Self { cluster: cluster.into(), - control_plane_bucket: control_plane_bucket.into(), + control_plane_bucket: None, } } + + pub fn with_control_plane_bucket(mut self, bucket: impl Into) -> Self { + self.control_plane_bucket = Some(bucket.into()); + self + } } /// Teardown outcome for one service. @@ -85,6 +92,8 @@ pub struct DeleteReport { pub enum DeleteErrorKind { /// Contract violation caught before any AWS call. Validation, + /// The deployment target (bucket resolution) could not be established. + Target, /// Teardown of a specific service failed. Cleanup is resumable: calling /// [`delete_services`] again with the same target continues from the /// remaining resources. @@ -111,6 +120,15 @@ impl DeleteError { } } + fn target(source: impl Into) -> Self { + Self { + kind: DeleteErrorKind::Target, + failed_service: None, + completed: DeleteReport::default(), + source: source.into(), + } + } + fn teardown( failed_service: DeleteTarget, completed: DeleteReport, @@ -150,12 +168,6 @@ impl std::error::Error for DeleteError { const DELETE_CHECKPOINT_VERSION: u8 = 1; -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -enum DeletePolicy { - ExactIdentity, - CliLegacyOrphanCleanup, -} - #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] struct DeleteCheckpoint { version: u8, @@ -168,6 +180,8 @@ struct DeleteCheckpoint { requested_cluster: String, cluster_arn: String, service_arn: String, + /// ECS `createdAt` as epoch nanoseconds; distinguishes a recreated same-name service. + service_created_at: i128, registry_arn: Option, api_id: Option, } @@ -290,9 +304,8 @@ fn classify_service_identity( if !failure_reasons.is_empty() { if has_checkpoint && !service_present - && failure_reasons - .iter() - .all(|reason| reason.eq_ignore_ascii_case("MISSING")) + && failure_reasons.len() == 1 + && failure_reasons[0].eq_ignore_ascii_case("MISSING") { return Ok(ServiceIdentityState::RetryGone); } @@ -303,15 +316,19 @@ fn classify_service_identity( } match (service_present, status) { - (true, Some("ACTIVE")) | (true, Some("DRAINING")) => { - Ok(ServiceIdentityState::Live) - } + (true, Some("ACTIVE")) | (true, Some("DRAINING")) => Ok(ServiceIdentityState::Live), + // INACTIVE is retry-gone only after the caller has validated the exact + // service ARN, cluster ARN, and incarnation discriminator. (true, Some("INACTIVE")) if has_checkpoint => Ok(ServiceIdentityState::RetryGone), - (false, None) if has_checkpoint => Ok(ServiceIdentityState::RetryGone), - (false, None) | (true, Some("INACTIVE")) => Err( - "ECS service was not positively identified and no matching delete checkpoint exists" + // An empty successful response is ambiguous, and a checkpoint only + // authorizes exactly zero services plus one MISSING failure. + (false, None) => Err( + "ECS returned no service without an explicit MISSING failure; refusing cleanup" .to_string(), ), + (true, Some("INACTIVE")) => Err( + "ECS returned INACTIVE without a matching delete checkpoint".to_string(), + ), (true, None) => Err("ECS returned a service without status".to_string()), (true, Some(other)) => Err(format!( "unexpected ECS service status during delete: {other}" @@ -356,8 +373,11 @@ fn validate_checkpoint( { anyhow::bail!("delete checkpoint canonical cluster does not match the requested cluster"); } - if checkpoint.cluster_arn.trim().is_empty() || checkpoint.service_arn.trim().is_empty() { - anyhow::bail!("delete checkpoint is missing exact ECS identity"); + if checkpoint.cluster_arn.trim().is_empty() + || checkpoint.service_arn.trim().is_empty() + || checkpoint.service_created_at <= 0 + { + anyhow::bail!("delete checkpoint is missing exact ECS identity or service incarnation"); } validate_checkpoint_arns( checkpoint, @@ -390,11 +410,6 @@ fn validate_delete_request( "DeleteOptions.cluster must not be empty" ))); } - if opts.control_plane_bucket.trim().is_empty() { - return Err(DeleteError::validation(anyhow::anyhow!( - "DeleteOptions.control_plane_bucket must not be empty" - ))); - } for target in targets { if target.namespace.trim().is_empty() || target.name.trim().is_empty() { return Err(DeleteError::validation(anyhow::anyhow!( @@ -412,8 +427,8 @@ fn validate_delete_request( /// /// Contract (enforced before any AWS call): /// - the target set must be non-empty -/// - [`DeleteOptions::cluster`] and the explicitly bound control-plane bucket -/// must be non-empty +/// - [`DeleteOptions::cluster`] must be non-empty; its optional bucket override +/// follows the shared control-plane resolver /// - every target's `namespace`/`name` must be non-empty /// /// Before ECS mutation, delete persists an exact-identity checkpoint containing @@ -429,7 +444,12 @@ pub async fn delete_services( ) -> std::result::Result { crate::apply::with_progress_suppressed(async { validate_delete_request(targets, opts)?; - let bucket = opts.control_plane_bucket.trim(); + let bucket = crate::control_plane::resolve_bucket( + aws_config, + opts.control_plane_bucket.as_deref(), + ) + .await + .map_err(DeleteError::target)?; let mut report = DeleteReport::default(); for target in targets { @@ -439,8 +459,7 @@ pub async fn delete_services( &target.name, &opts.cluster, &target.namespace, - bucket, - DeletePolicy::ExactIdentity, + &bucket, ) .await { @@ -527,7 +546,6 @@ pub(crate) async fn run_from_file( cluster, &manifest.metadata.namespace, &bucket, - DeletePolicy::CliLegacyOrphanCleanup, ) .await { @@ -566,7 +584,6 @@ pub(crate) async fn run( cluster, namespace, &bucket, - DeletePolicy::CliLegacyOrphanCleanup, ) .await .map(|_warnings| ()) @@ -650,7 +667,6 @@ fn single_described_service( } enum PreparedIdentity { Exact(DeleteCheckpoint, ServiceIdentityState), - LegacyOrphan, } async fn prepare_identity( @@ -661,7 +677,6 @@ async fn prepare_identity( name: &str, cluster: &str, bucket: &str, - policy: DeletePolicy, ) -> Result { let (partition, account, region) = caller_context(aws_config).await?; if let Some(checkpoint) = load_checkpoint(s3, bucket, namespace, name).await? { @@ -680,15 +695,16 @@ async fn prepare_identity( .send().await.context("failed to describe checkpointed ECS service")?; let service = single_described_service(&response)?; let reasons = failure_reasons(&response); + if let Some(service) = service { + validate_returned_service(&checkpoint, service)?; + } let state = classify_service_identity( true, service.is_some(), service.and_then(|service| service.status()), &reasons, - ).map_err(anyhow::Error::msg)?; - if let Some(service) = service { - validate_returned_service(&checkpoint, service)?; - } + ) + .map_err(anyhow::Error::msg)?; return Ok(PreparedIdentity::Exact(checkpoint, state)); } @@ -697,26 +713,22 @@ async fn prepare_identity( .send().await.context("failed to describe ECS service before delete")?; let service = single_described_service(&response)?; let reasons = failure_reasons(&response); - let state = match classify_service_identity( + let state = classify_service_identity( false, service.is_some(), service.and_then(|service| service.status()), &reasons, - ) { - Ok(state) => state, - Err(_) if policy == DeletePolicy::CliLegacyOrphanCleanup - && service.is_none() - && (reasons.is_empty() || reasons.iter().all(|reason| reason.eq_ignore_ascii_case("MISSING"))) => - { - return Ok(PreparedIdentity::LegacyOrphan); - } - Err(message) => anyhow::bail!(message), - }; + ) + .map_err(anyhow::Error::msg)?; let service = service.context("ECS service identity unexpectedly absent")?; let service_arn = service.service_arn().filter(|value| !value.trim().is_empty()) .context("ECS service response missing service ARN")?.to_string(); let cluster_arn = service.cluster_arn().filter(|value| !value.trim().is_empty()) .context("ECS service response missing canonical cluster ARN")?.to_string(); + let service_created_at = service + .created_at() + .map(|created_at| created_at.as_nanos()) + .context("ECS service response missing createdAt incarnation")?; let registry_arn = match service.service_registries() { [] => None, [registry] => Some( @@ -747,6 +759,7 @@ async fn prepare_identity( requested_cluster: cluster.to_string(), cluster_arn, service_arn, + service_created_at, registry_arn, api_id, }; @@ -764,30 +777,70 @@ async fn prepare_identity( Ok(PreparedIdentity::Exact(checkpoint, state)) } +fn validate_service_incarnation(expected: i128, actual: Option) -> Result<()> { + if actual != Some(expected) { + anyhow::bail!( + "ECS response belongs to a recreated same-name service (createdAt mismatch)" + ); + } + Ok(()) +} + fn validate_returned_service( - checkpoint: &DeleteCheckpoint, service: &aws_sdk_ecs::types::Service, + checkpoint: &DeleteCheckpoint, + service: &aws_sdk_ecs::types::Service, ) -> Result<()> { if service.service_arn() != Some(checkpoint.service_arn.as_str()) || service.cluster_arn() != Some(checkpoint.cluster_arn.as_str()) { - anyhow::bail!("ECS drain poll returned a conflicting service identity"); + anyhow::bail!("ECS response returned a conflicting service identity"); } + let created_at = service + .created_at() + .map(|created_at| created_at.as_nanos()); + validate_service_incarnation(checkpoint.service_created_at, created_at)?; let registry_arn = match service.service_registries() { [] => None, [registry] => Some( registry .registry_arn() .filter(|value| !value.trim().is_empty()) - .context("ECS drain poll returned a registry without an ARN")?, + .context("ECS response returned a registry without an ARN")?, ), - _ => anyhow::bail!("ECS drain poll returned multiple registry identities"), + _ => anyhow::bail!("ECS response returned multiple registry identities"), }; if registry_arn != checkpoint.registry_arn.as_deref() { - anyhow::bail!("ECS drain poll returned a conflicting registry identity"); + anyhow::bail!("ECS response returned a conflicting registry identity"); } Ok(()) } +async fn refresh_checkpointed_ecs( + ecs: &aws_sdk_ecs::Client, + checkpoint: &DeleteCheckpoint, + context: &str, +) -> Result { + let response = ecs + .describe_services() + .cluster(&checkpoint.cluster_arn) + .services(&checkpoint.service_arn) + .send() + .await + .with_context(|| context.to_string())?; + let reasons = failure_reasons(&response); + let service = single_described_service(&response)?; + if let Some(service) = service { + validate_returned_service(checkpoint, service)?; + } + classify_service_identity( + true, + service.is_some(), + service.and_then(|service| service.status()), + &reasons, + ) + .map_err(anyhow::Error::msg) +} + async fn delete_checkpointed_ecs( ecs: &aws_sdk_ecs::Client, checkpoint: &DeleteCheckpoint, @@ -798,37 +851,82 @@ async fn delete_checkpointed_ecs( return Ok(()); } - let response = ecs.describe_services() - .cluster(&checkpoint.cluster_arn).services(&checkpoint.service_arn) - .send().await.context("failed to refresh ECS service before mutation")?; + let response = ecs + .describe_services() + .cluster(&checkpoint.cluster_arn) + .services(&checkpoint.service_arn) + .send() + .await + .context("failed to refresh ECS service before mutation")?; let reasons = failure_reasons(&response); let service = single_described_service(&response)?; + if let Some(service) = service { + validate_returned_service(checkpoint, service)?; + } let state = classify_service_identity( true, service.is_some(), service.and_then(|service| service.status()), &reasons, - ).map_err(anyhow::Error::msg)?; + ) + .map_err(anyhow::Error::msg)?; if state == ServiceIdentityState::RetryGone { return Ok(()); } let service = service.context("checkpointed ECS service disappeared ambiguously")?; - validate_returned_service(checkpoint, service)?; let delete_phase = ecs_delete_phase(service.status())?; if delete_phase == EcsDeletePhase::Delete { - match ecs.update_service().cluster(&checkpoint.cluster_arn) - .service(&checkpoint.service_arn).desired_count(0).send().await { - Ok(_) => println!(" ✓ Scaled to 0"), - Err(error) if error.code() == Some("ServiceNotFoundException") => {} + match ecs + .update_service() + .cluster(&checkpoint.cluster_arn) + .service(&checkpoint.service_arn) + .desired_count(0) + .send() + .await + { + Ok(_) => { + println!(" ✓ Scaled to 0"); + // The service may disappear or be recreated between scale and + // delete. Re-describe the exact checkpointed ARN/incarnation + // immediately before issuing delete_service. + match refresh_checkpointed_ecs( + ecs, + checkpoint, + "failed to revalidate ECS service before delete", + ) + .await? + { + ServiceIdentityState::RetryGone => { + println!(" ✓ ECS service disappeared after scaling; skipping delete") + } + ServiceIdentityState::Live => { + match ecs + .delete_service() + .cluster(&checkpoint.cluster_arn) + .service(&checkpoint.service_arn) + .force(true) + .send() + .await + { + Ok(_) => println!(" ✓ ECS service deleted"), + Err(error) if error.code() == Some("ServiceNotFoundException") => { + println!(" ✓ ECS service disappeared while deleting") + } + Err(error) => { + return Err(error).context("failed to delete ECS service"); + } + } + } + } + } + Err(error) if error.code() == Some("ServiceNotFoundException") => { + // A successful scale is not a prerequisite for exact polling; + // never follow this response with delete_service. + println!(" ✓ ECS service disappeared while scaling; skipping delete") + } Err(error) => return Err(error).context("failed to scale ECS service to zero"), } - match ecs.delete_service().cluster(&checkpoint.cluster_arn) - .service(&checkpoint.service_arn).force(true).send().await { - Ok(_) => println!(" ✓ ECS service deleted"), - Err(error) if error.code() == Some("ServiceNotFoundException") => {} - Err(error) => return Err(error).context("failed to delete ECS service"), - } } else { println!(" ✓ ECS service is already draining; resuming delete cleanup"); } @@ -843,6 +941,9 @@ async fn delete_checkpointed_ecs( Ok(response) => { let reasons = failure_reasons(&response); let service = single_described_service(&response)?; + if let Some(service) = service { + validate_returned_service(checkpoint, service)?; + } match classify_service_identity( true, service.is_some(), @@ -850,12 +951,7 @@ async fn delete_checkpointed_ecs( &reasons, ) { Ok(ServiceIdentityState::RetryGone) => true, - Ok(ServiceIdentityState::Live) => { - if let Some(service) = service { - validate_returned_service(checkpoint, service)?; - } - false - } + Ok(ServiceIdentityState::Live) => false, Err(error) => { eprintln!("\n ⚠ ambiguous DescribeServices response (retrying): {error}"); false @@ -917,7 +1013,6 @@ async fn run_with_bucket( cluster: &str, namespace: &str, bucket: &str, - policy: DeletePolicy, ) -> Result> { if resource != "oabservice" { anyhow::bail!("unknown resource type: {resource}. Use 'oabservice'"); @@ -927,8 +1022,9 @@ async fn run_with_bucket( println!("Deleting {name}..."); match prepare_identity( - aws_config, &ecs, &s3, namespace, name, cluster, bucket, policy, - ).await? { + aws_config, &ecs, &s3, namespace, name, cluster, bucket, + ) + .await? { PreparedIdentity::Exact(checkpoint, state) => { delete_checkpointed_ecs(&ecs, &checkpoint, state).await?; crate::ingress::delete_exact( @@ -950,11 +1046,6 @@ async fn run_with_bucket( ))?; println!(" ✓ Delete checkpoint removed"); } - PreparedIdentity::LegacyOrphan => { - // CLI-only compatibility. Never delete Cloud Map without an ECS registry ARN. - crate::ingress::delete_api(aws_config, namespace, name).await?; - cleanup_s3(&s3, bucket, namespace, name).await?; - } } println!("\n✓ {name} deleted"); Ok(Vec::new()) @@ -981,6 +1072,7 @@ mod tests { requested_cluster: "cluster".to_string(), cluster_arn: "arn:aws:ecs:us-east-1:123456789012:cluster/cluster".to_string(), service_arn: "arn:aws:ecs:us-east-1:123456789012:service/cluster/oab-prod-bot".to_string(), + service_created_at: 1_700_000_000, registry_arn: Some( "arn:aws:servicediscovery:us-east-1:123456789012:service/srv-123".to_string(), ), @@ -994,7 +1086,7 @@ mod tests { let err = delete_services( &cfg, &[], - &DeleteOptions::new("cluster", "control-plane"), + &DeleteOptions::new("cluster").with_control_plane_bucket("control-plane"), ) .await .unwrap_err(); @@ -1009,7 +1101,7 @@ mod tests { let err = delete_services( &cfg, &targets, - &DeleteOptions::new("", "control-plane"), + &DeleteOptions::new("").with_control_plane_bucket("control-plane"), ) .await .unwrap_err(); @@ -1017,15 +1109,11 @@ mod tests { assert!(err.to_string().contains("cluster must not be empty"), "{err}"); } - #[tokio::test] - async fn delete_services_rejects_blank_bucket_before_aws() { - let cfg = test_sdk_config(); - let targets = [DeleteTarget::new("prod", "bot")]; - let err = delete_services(&cfg, &targets, &DeleteOptions::new("cluster", " ")) - .await - .unwrap_err(); - assert_eq!(err.kind, DeleteErrorKind::Validation); - assert!(err.to_string().contains("control_plane_bucket"), "{err}"); + #[test] + fn delete_options_support_optional_bucket_override() { + let options = DeleteOptions::new("cluster").with_control_plane_bucket("control-plane"); + assert_eq!(options.cluster, "cluster"); + assert_eq!(options.control_plane_bucket.as_deref(), Some("control-plane")); } #[tokio::test] @@ -1035,7 +1123,7 @@ mod tests { let err = delete_services( &cfg, &targets, - &DeleteOptions::new("cluster", "control-plane"), + &DeleteOptions::new("cluster").with_control_plane_bucket("control-plane"), ) .await .unwrap_err(); @@ -1076,7 +1164,18 @@ mod tests { &["MISSING"] ) .is_err()); + assert!(classify_service_identity( + true, + false, + None, + &["MISSING", "MISSING"] + ) + .is_err()); + assert!(classify_service_identity(true, false, None, &[]).is_err()); assert!(classify_service_identity(true, true, None, &[]).is_err()); + assert!(validate_service_incarnation(1_700_000_000, Some(1_700_000_001)).is_err()); + assert!(validate_service_incarnation(1_700_000_000, None).is_err()); + assert!(validate_service_incarnation(1_700_000_000, Some(1_700_000_000)).is_ok()); assert_eq!( classify_service_identity(true, true, Some("DRAINING"), &[]).unwrap(), ServiceIdentityState::Live @@ -1197,6 +1296,20 @@ mod tests { "us-east-1", ) .is_err()); + + let mut invalid_incarnation = checkpoint.clone(); + invalid_incarnation.service_created_at = 0; + assert!(validate_checkpoint( + &invalid_incarnation, + "prod", + "bot", + "cluster", + "control-plane", + "aws", + "123456789012", + "us-east-1", + ) + .is_err()); } #[test] diff --git a/operator/src/ingress.rs b/operator/src/ingress.rs index 779423df7..55e70f5ca 100644 --- a/operator/src/ingress.rs +++ b/operator/src/ingress.rs @@ -398,14 +398,15 @@ async fn delete_cloud_map_by_arn( config: &aws_config::SdkConfig, registry_arn: &str, service_name: &str, - expected_boundary: Option<(&str, &str, &str)>, + expected_boundary: (&str, &str, &str), ) -> Result<()> { - let service_id = match expected_boundary { - Some((partition, account, region)) => { - cloud_map_service_id_for_boundary(registry_arn, partition, account, region) - } - None => cloud_map_service_id_from_arn(registry_arn), - } + let (partition, account, region) = expected_boundary; + let service_id = cloud_map_service_id_for_boundary( + registry_arn, + partition, + account, + region, + ) .context("Cloud Map service ARN is outside the exact delete boundary")?; let discovery = aws_sdk_servicediscovery::Client::new(config); let mut last_error = None; @@ -432,8 +433,39 @@ pub(crate) async fn delete_cloud_map_exact( registry_arn: &str, service_name: &str, ) -> Result<()> { - delete_cloud_map_by_arn(config, registry_arn, service_name, None).await + let identity = aws_sdk_sts::Client::new(config) + .get_caller_identity() + .send() + .await + .context("failed to identify Cloud Map delete caller")?; + let account = identity + .account() + .context("STS response missing caller account")?; + let caller_arn = identity.arn().context("STS response missing caller ARN")?; + let mut arn_parts = caller_arn.splitn(3, ':'); + if arn_parts.next() != Some("arn") { + anyhow::bail!("STS returned an invalid caller ARN"); + } + let partition = arn_parts + .next() + .filter(|value| !value.is_empty()) + .context("STS caller ARN is missing its partition")?; + let region = config + .region() + .map(|region| region.as_ref().to_string()) + .context("AWS region must be resolved before Cloud Map delete")?; + delete_cloud_map_by_arn( + config, + registry_arn, + service_name, + (partition, account, ®ion), + ) + .await +} +fn integration_uri_matches(actual: Option<&str>, expected: &str) -> bool { + actual == Some(expected) } + /// Resolve a same-name API only when the name identifies exactly one resource /// and that API has an integration URI equal to the exact ECS registry ARN. /// Duplicate names are rejected before inspecting integrations, even if only @@ -465,7 +497,7 @@ pub(crate) async fn resolve_api_id_for_registry( matched |= response .items() .iter() - .any(|integration| integration.integration_uri() == Some(registry_arn)); + .any(|integration| integration_uri_matches(integration.integration_uri(), registry_arn)); next = response.next_token().map(str::to_owned); if next.is_none() { break; @@ -501,27 +533,13 @@ pub(crate) async fn delete_exact( config, registry_arn, &format!("oab-{namespace}-{name}"), - Some((partition, account, region)), + (partition, account, region), ) .await?; } Ok(()) } -/// CLI-only legacy orphan cleanup by name. Duplicate names fail closed. -pub async fn delete_api(config: &aws_config::SdkConfig, namespace: &str, name: &str) -> Result<()> { - let api = aws_sdk_apigatewayv2::Client::new(config); - let name_str = api_name(namespace, name); - if let Some((api_id, _)) = find_api(&api, &name_str).await? { - match api.delete_api().api_id(&api_id).send().await { - Ok(_) => eprintln!(" ✓ Deleted HTTP API: {name_str}"), - Err(error) if error.code() == Some("NotFoundException") => {} - Err(error) => return Err(error).with_context(|| format!("failed to delete HTTP API {api_id}")), - } - } - Ok(()) -} - // ─── VPC resolution ───────────────────────────────────────────────────────── async fn resolve_vpc_id(ec2: &aws_sdk_ec2::Client, m: &OABServiceManifest) -> Result { @@ -1350,4 +1368,32 @@ mod tests { "api-a" ); } + + #[test] + fn sole_api_requires_exact_registry_integration_uri() { + let registry = "arn:aws:servicediscovery:us-east-1:123456789012:service/srv-1"; + assert!(integration_uri_matches(Some(registry), registry)); + assert!(!integration_uri_matches( + Some("arn:aws:servicediscovery:us-east-1:123456789012:service/srv-other"), + registry + )); + assert!(!integration_uri_matches(None, registry)); + } + + #[test] + fn cloud_map_arn_requires_exact_boundary() { + let arn = "arn:aws:servicediscovery:us-east-1:123456789012:service/srv-1"; + assert_eq!( + cloud_map_service_id_for_boundary(arn, "aws", "123456789012", "us-east-1"), + Some("srv-1".to_string()) + ); + assert_eq!( + cloud_map_service_id_for_boundary(arn, "aws-cn", "123456789012", "us-east-1"), + None + ); + assert_eq!( + cloud_map_service_id_for_boundary(arn, "aws", "999999999999", "us-east-1"), + None + ); + } } diff --git a/operator/src/lib.rs b/operator/src/lib.rs index d2a8e26e9..0929764bb 100644 --- a/operator/src/lib.rs +++ b/operator/src/lib.rs @@ -42,17 +42,18 @@ //! an ARN require the apply caller to have `secretsmanager:DescribeSecret`, so //! oabctl can resolve the name to the full ARN required by ECS. //! -//! Programmatic apply and delete never read `~/.oabctl/config.toml`. -//! Apply may use [`ApplyOptions::with_control_plane_bucket`] or its documented -//! resolution chain. Delete is intentionally stricter: construct -//! [`DeleteOptions`] with both the cluster and exact control-plane bucket so a -//! destructive request cannot drift to another account-derived bucket. +//! Programmatic apply and delete never read `~/.oabctl/config.toml`. Use +//! [`ApplyOptions::with_control_plane_bucket`] or +//! [`DeleteOptions::with_control_plane_bucket`] for an explicit bucket; +//! otherwise both use the shared `OAB_CONTROL_PLANE_BUCKET` then caller-account +//! resolution chain. //! //! [`delete_services`] tears down by `namespace`+`name` ([`DeleteTarget`]). -//! Before mutating ECS it durably records the caller partition/account/region, bucket, -//! canonical cluster/service ARNs, and exact ingress IDs in S3. Missing or -//! ambiguous ECS identity fails closed unless that matching checkpoint already -//! exists; retries use only checkpointed IDs and remove the checkpoint last. +//! Before ECS mutation it durably records the resolved bucket, caller +//! partition/account/region, canonical cluster/service ARNs, the ECS service +//! `createdAt` incarnation, and exact ingress IDs in S3. Initial identity +//! ambiguity fails closed; retries use only checkpointed IDs and remove the +//! checkpoint last. There is no name-only orphan cleanup fallback. pub mod apply; mod bootstrap; From 1bd61e825e3f2e1308bde3be6294e10b0e3bead6 Mon Sep 17 00:00:00 2001 From: chaodu-agent <274062505+chaodu-agent@users.noreply.github.com> Date: Thu, 16 Jul 2026 19:02:15 -0400 Subject: [PATCH 06/20] fix(oabctl): satisfy rust 1.97 clippy on the merged tip MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - apply: redundant_guards — match empty failures with a slice pattern - ingress: cloud_map_service_id_from_arn became test-only after the exact-identity merge (all production paths use the boundary-pinned parser); mark it #[cfg(test)] with a note instead of carrying dead code Verified with the CI toolchain (rustc 1.97.1): clippy -D warnings clean, 96 tests + doc-test pass. --- operator/src/apply.rs | 2 +- operator/src/ingress.rs | 7 ++++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/operator/src/apply.rs b/operator/src/apply.rs index 3dfacb760..cbc7a4e98 100644 --- a/operator/src/apply.rs +++ b/operator/src/apply.rs @@ -536,7 +536,7 @@ fn classify_apply_service_response( (0, [reason]) if reason.eq_ignore_ascii_case("MISSING") => { Ok(ApplyServicePresence::Absent) } - (0, failures) if failures.is_empty() => { + (0, []) => { anyhow::bail!("ECS returned no service and no MISSING failure for apply") } _ => anyhow::bail!( diff --git a/operator/src/ingress.rs b/operator/src/ingress.rs index e58b641a7..29175bb66 100644 --- a/operator/src/ingress.rs +++ b/operator/src/ingress.rs @@ -267,7 +267,12 @@ fn has_stage_path_override(request_parameters: Option<&HashMap>) /// Extract the Cloud Map service ID from its ARN /// (`arn:aws:servicediscovery:::service/`). -pub(crate) fn cloud_map_service_id_from_arn(arn: &str) -> Option { +/// +/// Test-only reference parser: every production path resolves IDs through +/// [`cloud_map_service_id_for_boundary`], which additionally pins the +/// caller's partition/account/region (exact-identity delete contract). +#[cfg(test)] +fn cloud_map_service_id_from_arn(arn: &str) -> Option { let mut parts = arn.splitn(6, ':'); if parts.next() != Some("arn") || parts.next().filter(|part| !part.is_empty()).is_none() From 54bd8f28eda19c3bd209789be89c44740d3a55f1 Mon Sep 17 00:00:00 2001 From: chaodu-agent <274062505+chaodu-agent@users.noreply.github.com> Date: Sat, 18 Jul 2026 19:44:39 +0000 Subject: [PATCH 07/20] fix(oabctl): satisfy delete review contract --- docs/oabctl.md | 18 ++++++- operator/README.md | 16 ++++++- operator/src/apply.rs | 27 +++++++++++ operator/src/delete.rs | 104 +++++++++++++++++++++++++++++++++++------ operator/src/lib.rs | 21 +++++++-- 5 files changed, 164 insertions(+), 22 deletions(-) diff --git a/docs/oabctl.md b/docs/oabctl.md index b3b890a65..81a5a9ff6 100644 --- a/docs/oabctl.md +++ b/docs/oabctl.md @@ -382,8 +382,22 @@ must still be registered manually in the LINE Developers console. > equals the ECS registry ARN. Cloud Map is deleted only by the service ID > parsed from a structurally and boundary-validated Cloud Map service ARN. > Exact-ID NotFound is idempotent; other cleanup errors retain the checkpoint. -> There is no name-only API, Cloud Map, or S3 orphan cleanup path. If you edit -> a manifest to remove `spec.ingress` while keeping the bot, `apply` first +> There is no name-only API, Cloud Map, or S3 orphan cleanup path. Programmatic +> delete namespaces must not contain `-` (names may contain it), which makes the +> existing `oab-{namespace}-{name}` identity injective for every accepted delete +> target. If a per-target +> `ingress-teardown-checkpoints//.json` record exists, delete +> fails before destructive mutation so it cannot discard unfinished exact +> cleanup identity. Re-run the ingress-free apply to completion, then retry +> delete. +> +> Programmatic apply and delete do not provide an internal same-target lock. +> Callers must serialize mutations for the same AWS account, Region, +> control-plane bucket, ECS cluster, namespace, and name. After an accidental +> overlap, stop concurrent writers, inspect retained checkpoints, then +> explicitly re-apply the desired state or retry delete. +> +> If you edit a manifest to remove `spec.ingress` while keeping the bot, `apply` first > stores every exact ECS registry ARN only when the previously stored OAB > manifest owned ingress (or resumes an already-valid checkpoint); an arbitrary > attached registry is never enough. diff --git a/operator/README.md b/operator/README.md index 8ff7c34c0..41aab4daf 100644 --- a/operator/README.md +++ b/operator/README.md @@ -131,7 +131,21 @@ IDs after ECS explicitly reports exactly one `MISSING` failure with zero service or a matching original `INACTIVE` incarnation. Duplicate APIs fail closed, and a sole same-name API is checkpointed only when an integration URI exactly matches the ECS registry ARN. -There is no name-only API, Cloud Map, or S3 orphan cleanup fallback. +There is no name-only API, Cloud Map, or S3 orphan cleanup fallback. If a +per-target apply-side `ingress-teardown-checkpoints//.json` +record exists, delete fails before destructive mutation so it cannot discard +unfinished exact cleanup identity. Re-run the ingress-free apply to completion, +then retry delete. + +Delete namespaces must not contain `-`; names may contain it. This makes the +existing `oab-{namespace}-{name}` physical identity injective for every target +accepted by the programmatic delete API. + +Programmatic apply and delete do not provide an internal same-target lock. +Callers must serialize mutations for the same AWS account, Region, control-plane +bucket, ECS cluster, namespace, and name. If overlapping calls occur, stop +concurrent writers, inspect the retained checkpoint, then explicitly re-apply +the desired state or retry delete. For `aws-sm://#`, a non-ARN `` requires the caller to have `secretsmanager:DescribeSecret`; full-ARN shorthand does not diff --git a/operator/src/apply.rs b/operator/src/apply.rs index cbc7a4e98..a8f8560d6 100644 --- a/operator/src/apply.rs +++ b/operator/src/apply.rs @@ -399,6 +399,14 @@ pub(crate) async fn run( /// Validate and reconcile in-memory manifests without writing progress to /// process-global stdout or stderr. +/// +/// # Concurrency +/// +/// This function is not serialized with [`crate::delete::delete_services`]. +/// Callers must serialize mutations for the same AWS account, Region, +/// control-plane bucket, ECS cluster, namespace, and name. If that precondition +/// is violated, stop concurrent writers and re-apply the intended desired state +/// before resuming other mutations. pub async fn apply_manifests( aws_config: &aws_config::SdkConfig, manifests: &[OABServiceManifest], @@ -587,6 +595,15 @@ fn ingress_teardown_checkpoint_key(namespace: &str, name: &str) -> String { format!("ingress-teardown-checkpoints/{namespace}/{name}.json") } +pub(crate) fn require_no_pending_ingress_teardown(checkpoint_present: bool) -> Result<()> { + if checkpoint_present { + anyhow::bail!( + "a previous apply ingress teardown is still pending; re-run the ingress-free apply before delete" + ); + } + Ok(()) +} + async fn load_ingress_teardown_checkpoint( s3: &aws_sdk_s3::Client, bucket: &str, @@ -615,6 +632,16 @@ async fn load_ingress_teardown_checkpoint( } } +pub(crate) async fn ensure_no_pending_ingress_teardown( + s3: &aws_sdk_s3::Client, + bucket: &str, + namespace: &str, + name: &str, +) -> Result<()> { + let checkpoint = load_ingress_teardown_checkpoint(s3, bucket, namespace, name).await?; + require_no_pending_ingress_teardown(checkpoint.is_some()) +} + async fn save_ingress_teardown_checkpoint( s3: &aws_sdk_s3::Client, bucket: &str, diff --git a/operator/src/delete.rs b/operator/src/delete.rs index 3cd9a3aa7..1c09dc309 100644 --- a/operator/src/delete.rs +++ b/operator/src/delete.rs @@ -21,7 +21,11 @@ macro_rules! eprint { /// /// Deletion does not require the original manifest: the control-plane copy /// under `manifests/{namespace}/{name}.yaml` (and every other resource) is -/// addressed by `namespace` + `name` alone. +/// addressed by `namespace` + `name` alone. Because physical resource names +/// use `-` as the component delimiter, [`delete_services`] accepts namespaces +/// without `-`; names may still contain it. This keeps every accepted logical +/// target's physical delete identity injective without renaming existing +/// deployments. #[derive(Debug, Clone, PartialEq, Eq)] pub struct DeleteTarget { pub namespace: String, @@ -419,10 +423,35 @@ fn validate_delete_request( target.name ))); } + if target.namespace.contains('-') { + return Err(DeleteError::validation(anyhow::anyhow!( + "delete target namespace must not contain '-' because it is the physical identity delimiter (got '{}')", + target.namespace + ))); + } } Ok(()) } +fn record_delete_result( + mut report: DeleteReport, + target: &DeleteTarget, + outcome: Result>, +) -> std::result::Result { + match outcome { + Ok(warnings) => { + report.services.push(DeletedService { + namespace: target.namespace.clone(), + name: target.name.clone(), + ecs_service_name: target.ecs_service_name(), + warnings, + }); + Ok(report) + } + Err(error) => Err(DeleteError::teardown(target.clone(), report, error)), + } +} + /// Tear down OAB services programmatically, without reading CLI home /// configuration or writing progress to process-global stdout/stderr. /// @@ -431,6 +460,17 @@ fn validate_delete_request( /// - [`DeleteOptions::cluster`] must be non-empty; its optional bucket override /// follows the shared control-plane resolver /// - every target's `namespace`/`name` must be non-empty +/// - target namespaces must not contain `-`; that delimiter may appear in names, +/// so this restriction makes `oab-{namespace}-{name}` injective for every +/// accepted target +/// +/// # Concurrency +/// +/// This function is not serialized with [`crate::apply::apply_manifests`]. +/// Callers must serialize mutations for the same AWS account, Region, +/// control-plane bucket, ECS cluster, namespace, and name. If that precondition +/// is violated, stop concurrent writers, inspect the retained checkpoint, then +/// either re-apply the intended desired state or retry delete. /// /// Before ECS mutation, delete persists an exact-identity checkpoint containing /// the caller account/region, bucket, canonical cluster and service ARNs, and @@ -454,7 +494,7 @@ pub async fn delete_services( let mut report = DeleteReport::default(); for target in targets { - match run_with_bucket( + let outcome = run_with_bucket( aws_config, "oabservice", &target.name, @@ -462,18 +502,8 @@ pub async fn delete_services( &target.namespace, &bucket, ) - .await - { - Ok(warnings) => report.services.push(DeletedService { - namespace: target.namespace.clone(), - name: target.name.clone(), - ecs_service_name: target.ecs_service_name(), - warnings, - }), - Err(error) => { - return Err(DeleteError::teardown(target.clone(), report, error)); - } - } + .await; + report = record_delete_result(report, target, outcome)?; } Ok(report) }) @@ -1021,6 +1051,8 @@ async fn run_with_bucket( } let ecs = aws_sdk_ecs::Client::new(aws_config); let s3 = aws_sdk_s3::Client::new(aws_config); + crate::apply::ensure_no_pending_ingress_teardown(&s3, bucket, namespace, name) + .await?; println!("Deleting {name}..."); match prepare_identity( @@ -1382,6 +1414,50 @@ mod tests { ); } + #[tokio::test] + async fn ambiguous_namespace_delimiter_is_rejected_before_aws() { + let opts = DeleteOptions::new("cluster"); + let accepted = [DeleteTarget::new("prod", "team-bot")]; + let rejected = [DeleteTarget::new("prod-team", "bot")]; + assert_eq!( + accepted[0].ecs_service_name(), + rejected[0].ecs_service_name() + ); + assert!(validate_delete_request(&accepted, &opts).is_ok()); + + let error = delete_services(&test_sdk_config(), &rejected, &opts) + .await + .unwrap_err(); + assert_eq!(error.kind, DeleteErrorKind::Validation); + assert!(error.to_string().contains("must not contain '-'")); + } + + #[test] + fn later_failure_preserves_the_completed_partial_report() { + let first = DeleteTarget::new("prod", "first"); + let second = DeleteTarget::new("prod", "second"); + let report = record_delete_result(DeleteReport::default(), &first, Ok(Vec::new())) + .expect("first target should complete"); + let error = record_delete_result( + report, + &second, + Err(anyhow::anyhow!("synthetic teardown failure")), + ) + .unwrap_err(); + + assert_eq!(error.kind, DeleteErrorKind::Teardown); + assert_eq!(error.failed_service.as_ref(), Some(&second)); + assert_eq!(error.completed.services.len(), 1); + assert_eq!(error.completed.services[0].name, "first"); + } + + #[test] + fn pending_apply_ingress_teardown_blocks_delete_completion() { + assert!(crate::apply::require_no_pending_ingress_teardown(false).is_ok()); + let error = crate::apply::require_no_pending_ingress_teardown(true).unwrap_err(); + assert!(error.to_string().contains("re-run the ingress-free apply")); + } + #[test] fn drain_poll_action_requires_completion_before_cleanup() { assert_eq!(drain_poll_action(true, 0, 12), DrainPollAction::Complete); diff --git a/operator/src/lib.rs b/operator/src/lib.rs index 0929764bb..52ad9e4f6 100644 --- a/operator/src/lib.rs +++ b/operator/src/lib.rs @@ -49,11 +49,22 @@ //! resolution chain. //! //! [`delete_services`] tears down by `namespace`+`name` ([`DeleteTarget`]). -//! Before ECS mutation it durably records the resolved bucket, caller -//! partition/account/region, canonical cluster/service ARNs, the ECS service -//! `createdAt` incarnation, and exact ingress IDs in S3. Initial identity -//! ambiguity fails closed; retries use only checkpointed IDs and remove the -//! checkpoint last. There is no name-only orphan cleanup fallback. +//! Delete namespaces must not contain `-`; names may contain it, making the +//! existing `oab-{namespace}-{name}` resource identity injective for accepted +//! targets. Before ECS mutation delete durably records the resolved bucket, +//! caller partition/account/region, canonical cluster/service ARNs, the ECS +//! service `createdAt` incarnation, and exact ingress IDs in S3. Initial +//! identity ambiguity fails closed; retries use only checkpointed IDs and +//! remove the delete checkpoint on success. If apply has a pending exact +//! ingress-teardown checkpoint, delete fails before destructive mutation and +//! requires the caller to finish the ingress-free apply first, preserving that +//! cleanup identity. There is no name-only orphan cleanup fallback. +//! +//! Programmatic apply and delete do not provide an internal same-target lock. +//! Callers must serialize mutations for the same AWS account, Region, +//! control-plane bucket, ECS cluster, namespace, and name. After an accidental +//! overlap, stop concurrent writers, inspect any retained checkpoint, and +//! explicitly re-apply the desired state or retry delete. pub mod apply; mod bootstrap; From be006ec67f5329df90423785e417e0ffb4671d85 Mon Sep 17 00:00:00 2001 From: chaodu-agent <274062505+chaodu-agent@users.noreply.github.com> Date: Mon, 20 Jul 2026 21:57:23 -0400 Subject: [PATCH 08/20] fix(oabctl)!: enforce one injective apply/delete identity rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Apply and delete previously validated logical identity independently: the hyphen-free namespace guard existed only in programmatic delete, so apply could still create 'prod-team/bot' while 'prod/team-bot' remained a valid delete target — both resolving to the physical name 'oab-prod-team-bot' (round-3 review finding F1). A new shared module, operator/src/identity.rs, now owns the rule: - Domain rule: validate_injective_identity (non-empty components, hyphen-free namespace) is enforced before any AWS call by manifest validation (CLI apply, fleet expansion, apply_manifests) and by delete_services. - Ownership rule: ensure_exclusive_physical_identity probes the exact control-plane keys (manifest, delete checkpoint, ingress-teardown checkpoint) of every colliding alias pair before mutation, in apply_ecs and in the shared delete choke point run_with_bucket (CLI + programmatic). Contested physical identities fail closed naming the colliding pair. - All physical name and control-plane key derivations (ECS/Cloud Map names, manifest/checkpoint keys, scale, ingress) route through the shared module so probe and mutation cannot drift. - Manifest schema namespace pattern tightened to ^[a-z0-9]+$. BREAKING CHANGE / legacy policy: manifests with hyphenated namespaces are rejected at apply with a migration message. Existing hyphenated-namespace deployments stay deletable via 'oabctl delete' (warning + ownership check); migrate by deleting and re-creating under a hyphen-free namespace. Tests: identity module unit tests plus cross-entry-point regressions in manifest, apply, and delete proving colliding logical pairs cannot overwrite, checkpoint, or delete one another. cargo clippy --all-targets -- -D warnings and cargo test (113 + 1 doctest) pass. --- docs/oabctl.md | 21 ++- operator/README.md | 19 ++- operator/schema/oabservice-v2.json | 4 +- operator/src/apply.rs | 64 ++++++- operator/src/delete.rs | 108 +++++++++--- operator/src/identity.rs | 262 +++++++++++++++++++++++++++++ operator/src/ingress.rs | 2 +- operator/src/lib.rs | 16 +- operator/src/manifest.rs | 36 +++- operator/src/scale.rs | 2 +- 10 files changed, 485 insertions(+), 49 deletions(-) create mode 100644 operator/src/identity.rs diff --git a/docs/oabctl.md b/docs/oabctl.md index 81a5a9ff6..bd8ef4643 100644 --- a/docs/oabctl.md +++ b/docs/oabctl.md @@ -219,6 +219,11 @@ spec: securityGroups: [sg-xxx] ``` +`metadata.namespace` must be hyphen-free (`[a-z0-9]+`); `metadata.name` may +contain `-`. Physical resources are named `oab-{namespace}-{name}`, and a +hyphen-free namespace keeps that identity unambiguous (see the teardown note +below for the legacy-namespace policy). + ### Ingress — inbound webhooks (Telegram / LINE) Discord bots are outbound-only and need no ingress. Webhook platforms (Telegram, @@ -382,10 +387,18 @@ must still be registered manually in the LINE Developers console. > equals the ECS registry ARN. Cloud Map is deleted only by the service ID > parsed from a structurally and boundary-validated Cloud Map service ARN. > Exact-ID NotFound is idempotent; other cleanup errors retain the checkpoint. -> There is no name-only API, Cloud Map, or S3 orphan cleanup path. Programmatic -> delete namespaces must not contain `-` (names may contain it), which makes the -> existing `oab-{namespace}-{name}` identity injective for every accepted delete -> target. If a per-target +> There is no name-only API, Cloud Map, or S3 orphan cleanup path. Apply and +> delete share one injective identity rule: namespaces must not contain `-` +> (names may contain it), so the physical `oab-{namespace}-{name}` identity +> always maps back to exactly one `namespace`/`name` pair. Manifest validation +> and programmatic delete reject hyphenated namespaces before any AWS call, +> and before mutating anything both apply and delete verify in the control +> plane that no other recorded logical pair (for example legacy `prod-team/bot` +> versus `prod/team-bot`) claims the same physical name — contested identities +> fail closed. Legacy deployments created under a hyphenated namespace cannot +> be re-applied, but `oabctl delete` still accepts them (with a warning) under +> that same ownership check; migrate by deleting and re-creating them under a +> hyphen-free namespace. If a per-target > `ingress-teardown-checkpoints//.json` record exists, delete > fails before destructive mutation so it cannot discard unfinished exact > cleanup identity. Re-run the ingress-free apply to completion, then retry diff --git a/operator/README.md b/operator/README.md index 41aab4daf..3572581ca 100644 --- a/operator/README.md +++ b/operator/README.md @@ -137,9 +137,22 @@ record exists, delete fails before destructive mutation so it cannot discard unfinished exact cleanup identity. Re-run the ingress-free apply to completion, then retry delete. -Delete namespaces must not contain `-`; names may contain it. This makes the -existing `oab-{namespace}-{name}` physical identity injective for every target -accepted by the programmatic delete API. +Apply and delete share one injective logical-identity rule: namespaces must not +contain `-`; names may contain it, so the physical `oab-{namespace}-{name}` +identity always parses back to exactly one `namespace`/`name` pair. Manifest +validation (CLI apply, fleet expansion, and `apply_manifests`) and +`delete_services` all reject hyphenated namespaces before any AWS call. Before +mutating, apply and every delete entry point additionally verify in the +control plane that no *other* recorded logical pair (manifest, delete +checkpoint, or ingress-teardown checkpoint) claims the same physical name, and +fail closed with the colliding pair named in the error. + +Legacy policy: deployments created under a hyphenated namespace before this +rule cannot be re-applied — apply fails with a migration message. They remain +deletable through `oabctl delete`, which accepts a hyphenated namespace for +teardown, prints a warning, and relies on the same control-plane ownership +check to refuse contested physical names. Migrate by deleting the legacy +deployment and re-creating it under a hyphen-free namespace. Programmatic apply and delete do not provide an internal same-target lock. Callers must serialize mutations for the same AWS account, Region, control-plane diff --git a/operator/schema/oabservice-v2.json b/operator/schema/oabservice-v2.json index 40feefab4..0d92a7396 100644 --- a/operator/schema/oabservice-v2.json +++ b/operator/schema/oabservice-v2.json @@ -35,7 +35,7 @@ "required": ["name", "namespace"], "properties": { "name": { "type": "string", "pattern": "^[a-z0-9][a-z0-9-]*$" }, - "namespace": { "type": "string", "pattern": "^[a-z0-9][a-z0-9-]*$" }, + "namespace": { "type": "string", "pattern": "^[a-z0-9]+$" }, "generation": { "type": "integer", "minimum": 0 } }, "additionalProperties": false @@ -45,7 +45,7 @@ "required": ["name", "namespace"], "properties": { "name": { "type": "string" }, - "namespace": { "type": "string" } + "namespace": { "type": "string", "pattern": "^[a-z0-9]+$" } }, "additionalProperties": false }, diff --git a/operator/src/apply.rs b/operator/src/apply.rs index a8f8560d6..fd985a5a0 100644 --- a/operator/src/apply.rs +++ b/operator/src/apply.rs @@ -400,6 +400,14 @@ pub(crate) async fn run( /// Validate and reconcile in-memory manifests without writing progress to /// process-global stdout or stderr. /// +/// Every manifest must satisfy the shared injective identity rule (see +/// [`crate::identity`]): non-empty `metadata.namespace`/`metadata.name` and a +/// hyphen-free namespace, so the physical `oab-{namespace}-{name}` identity +/// maps back to exactly one logical pair. Before reconciling each service, +/// apply also verifies in the control plane that no other recorded logical +/// pair (for example a legacy hyphenated namespace) claims the same physical +/// identity, and fails closed if one does. +/// /// # Concurrency /// /// This function is not serialized with [`crate::delete::delete_services`]. @@ -592,7 +600,7 @@ fn validate_apply_service_identity( } fn ingress_teardown_checkpoint_key(namespace: &str, name: &str) -> String { - format!("ingress-teardown-checkpoints/{namespace}/{name}.json") + crate::identity::ingress_teardown_checkpoint_key(namespace, name) } pub(crate) fn require_no_pending_ingress_teardown(checkpoint_present: bool) -> Result<()> { @@ -818,6 +826,17 @@ async fn apply_ecs( ) -> Result { let bucket = prepared.bucket.as_str(); let bootstrap = &prepared.bootstrap; + // Shared ownership rule (see `crate::identity`): never reconcile a + // physical identity that another recorded logical pair claims — e.g. a + // legacy hyphenated-namespace deployment whose `oab-{namespace}-{name}` + // collides with this manifest's. + crate::identity::ensure_exclusive_physical_identity( + s3, + bucket, + &m.metadata.namespace, + &m.metadata.name, + ) + .await?; let ecs_rt = match &m.spec.runtime { Runtime::Ecs(rt) => rt, _ => unreachable!(), @@ -833,10 +852,7 @@ async fn apply_ecs( // Read the current generation from the stored desired-state manifest. // Ingress teardown retry state is kept separately in an exact-identity // checkpoint, so writing a newer manifest cannot lose pending cleanup. - let manifest_key = format!( - "manifests/{}/{}.yaml", - m.metadata.namespace, m.metadata.name - ); + let manifest_key = crate::identity::manifest_key(&m.metadata.namespace, &m.metadata.name); let (current_gen, previously_had_ingress) = match s3 .get_object() .bucket(bucket) @@ -1716,6 +1732,44 @@ spec: assert!(err.to_string().contains("empty or whitespace")); } + #[tokio::test] + async fn apply_manifests_rejects_hyphenated_namespace_locally() { + // Cross-entry-point injective identity regression: the pair + // `prod-team/bot` collides with `prod/team-bot` on the physical name + // `oab-prod-team-bot`, so apply must reject it before any AWS call — + // the same rule programmatic delete enforces on its targets. + let cfg = test_sdk_config(); + let mut manifest = minimal_manifest(); + manifest.metadata.namespace = "prod-team".to_string(); + manifest.metadata.name = "bot".to_string(); + let mut colliding = minimal_manifest(); + colliding.metadata.namespace = "prod".to_string(); + colliding.metadata.name = "team-bot".to_string(); + assert_eq!(manifest.ecs_service_name(), colliding.ecs_service_name()); + let err = apply_manifests(&cfg, &[manifest], &ApplyOptions::new("test-cluster")) + .await + .unwrap_err(); + assert_eq!(err.kind, ApplyErrorKind::Validation); + let chain = format!("{:#}", err.source_error()); + assert!(chain.contains("must not contain '-'"), "{chain}"); + } + + #[tokio::test] + async fn apply_manifests_accepts_hyphenated_name_locally() { + // Names may contain '-'; only the namespace is restricted. The + // manifest must pass local validation and fail later at the (absent) + // AWS boundary instead. + let cfg = test_sdk_config(); + let mut manifest = minimal_manifest(); + manifest.metadata.namespace = "prod".to_string(); + manifest.metadata.name = "team-bot".to_string(); + assert!(manifest.validate().is_ok()); + let err = apply_manifests(&cfg, &[manifest], &ApplyOptions::new("test-cluster")) + .await + .unwrap_err(); + assert_ne!(err.kind, ApplyErrorKind::Validation, "{err}"); + } + #[test] fn cluster_response_accepts_requested_active_cluster() { assert!(classify_cluster_response( diff --git a/operator/src/delete.rs b/operator/src/delete.rs index 1c09dc309..f0e9ab855 100644 --- a/operator/src/delete.rs +++ b/operator/src/delete.rs @@ -21,11 +21,11 @@ macro_rules! eprint { /// /// Deletion does not require the original manifest: the control-plane copy /// under `manifests/{namespace}/{name}.yaml` (and every other resource) is -/// addressed by `namespace` + `name` alone. Because physical resource names -/// use `-` as the component delimiter, [`delete_services`] accepts namespaces -/// without `-`; names may still contain it. This keeps every accepted logical -/// target's physical delete identity injective without renaming existing -/// deployments. +/// addressed by `namespace` + `name` alone. Targets are subject to the shared +/// injective identity rule (see [`crate::identity`]): namespaces must not +/// contain `-`, names may. Before any destructive mutation, delete also +/// verifies in the control plane that no other recorded logical pair claims +/// the same physical `oab-{namespace}-{name}` identity. #[derive(Debug, Clone, PartialEq, Eq)] pub struct DeleteTarget { pub namespace: String, @@ -42,7 +42,7 @@ impl DeleteTarget { /// ECS service name derived from the target (`oab-{namespace}-{name}`). pub fn ecs_service_name(&self) -> String { - format!("oab-{}-{}", self.namespace, self.name) + crate::identity::physical_service_name(&self.namespace, &self.name) } } @@ -342,7 +342,7 @@ fn classify_service_identity( } fn checkpoint_key(namespace: &str, name: &str) -> String { - format!("delete-checkpoints/{namespace}/{name}.json") + crate::identity::delete_checkpoint_key(namespace, name) } #[allow(clippy::too_many_arguments)] @@ -386,7 +386,7 @@ fn validate_checkpoint( } validate_checkpoint_arns( checkpoint, - &format!("oab-{namespace}-{name}"), + &crate::identity::physical_service_name(namespace, name), partition, account, region, @@ -416,19 +416,11 @@ fn validate_delete_request( ))); } for target in targets { - if target.namespace.trim().is_empty() || target.name.trim().is_empty() { - return Err(DeleteError::validation(anyhow::anyhow!( - "delete target namespace and name must not be empty (got '{}'/'{}')", - target.namespace, - target.name - ))); - } - if target.namespace.contains('-') { - return Err(DeleteError::validation(anyhow::anyhow!( - "delete target namespace must not contain '-' because it is the physical identity delimiter (got '{}')", - target.namespace - ))); - } + // Shared injective identity rule (see `crate::identity`): non-empty + // components and a hyphen-free namespace, matching what apply-side + // manifest validation accepts. + crate::identity::validate_injective_identity(&target.namespace, &target.name) + .map_err(DeleteError::validation)?; } Ok(()) } @@ -459,10 +451,16 @@ fn record_delete_result( /// - the target set must be non-empty /// - [`DeleteOptions::cluster`] must be non-empty; its optional bucket override /// follows the shared control-plane resolver -/// - every target's `namespace`/`name` must be non-empty -/// - target namespaces must not contain `-`; that delimiter may appear in names, -/// so this restriction makes `oab-{namespace}-{name}` injective for every -/// accepted target +/// - every target must satisfy the shared injective identity rule (see +/// [`crate::identity`]): non-empty `namespace`/`name` and a hyphen-free +/// namespace, the same rule apply-side manifest validation enforces. Legacy +/// deployments created under a hyphenated namespace are not accepted here; +/// remove them with the `oabctl delete` CLI, which performs the +/// control-plane ownership check for such targets. +/// +/// Additionally, before any destructive mutation, delete verifies in the +/// control plane that no other recorded logical pair claims the target's +/// physical `oab-{namespace}-{name}` identity, and fails closed if one does. /// /// # Concurrency /// @@ -740,7 +738,7 @@ async fn prepare_identity( return Ok(PreparedIdentity::Exact(Box::new((checkpoint, state)))); } - let service_name = format!("oab-{namespace}-{name}"); + let service_name = crate::identity::physical_service_name(namespace, name); let response = ecs.describe_services().cluster(cluster).services(&service_name) .send().await.context("failed to describe ECS service before delete")?; let service = single_described_service(&response)?; @@ -1012,7 +1010,7 @@ async fn delete_checkpointed_ecs( async fn cleanup_s3( s3: &aws_sdk_s3::Client, bucket: &str, namespace: &str, name: &str, ) -> Result<()> { - let manifest_key = format!("manifests/{namespace}/{name}.yaml"); + let manifest_key = crate::identity::manifest_key(namespace, name); s3.delete_object().bucket(bucket).key(&manifest_key).send().await .with_context(|| format!("failed to delete s3://{bucket}/{manifest_key}"))?; println!(" ✓ Manifest removed from S3"); @@ -1049,8 +1047,28 @@ async fn run_with_bucket( if resource != "oabservice" { anyhow::bail!("unknown resource type: {resource}. Use 'oabservice'"); } + if namespace.trim().is_empty() || name.trim().is_empty() { + anyhow::bail!( + "delete requires a non-empty namespace and name (got '{namespace}'/'{name}')" + ); + } let ecs = aws_sdk_ecs::Client::new(aws_config); let s3 = aws_sdk_s3::Client::new(aws_config); + if namespace.contains('-') { + // Legacy-deployment policy: hyphenated namespaces predate the shared + // injective identity rule (see `crate::identity`). Programmatic + // targets are rejected earlier; the CLI keeps them deletable, guarded + // by the exclusive-ownership check below. + eprintln!( + " ⚠ Namespace '{namespace}' contains '-', which predates the hyphen-free \ + namespace rule; continuing only if the control plane records no colliding \ + deployment for '{}'", + crate::identity::physical_service_name(namespace, name) + ); + } + // Shared ownership rule: refuse to touch a physical identity that another + // recorded logical pair (e.g. a legacy hyphenated namespace) claims. + crate::identity::ensure_exclusive_physical_identity(&s3, bucket, namespace, name).await?; crate::apply::ensure_no_pending_ingress_teardown(&s3, bucket, namespace, name) .await?; println!("Deleting {name}..."); @@ -1429,7 +1447,41 @@ mod tests { .await .unwrap_err(); assert_eq!(error.kind, DeleteErrorKind::Validation); - assert!(error.to_string().contains("must not contain '-'")); + assert!(error.to_string().contains("must not contain '-'"), "{error}"); + } + + #[test] + fn colliding_logical_pairs_guard_every_entry_point() { + // `prod/team-bot` and `prod-team/bot` share the physical identity + // `oab-prod-team-bot`. Cross-entry-point regression for the shared + // rule in `crate::identity`: + // + // 1. Apply cannot create the hyphenated-namespace side (domain rule in + // manifest validation, exercised in `manifest`/`apply` tests) and + // programmatic delete cannot target it + // (`ambiguous_namespace_delimiter_is_rejected_before_aws` above). + assert!( + crate::identity::validate_injective_identity("prod-team", "bot").is_err(), + "hyphenated namespace must be outside the accepted identity domain" + ); + // 2. For the accepted side, the pre-mutation ownership probe shared by + // apply (`apply_ecs`) and every delete entry point + // (`run_with_bucket`) checks exactly the legacy pair's manifest and + // checkpoint keys, so a recorded `prod-team/bot` blocks overwrite, + // checkpointing, and deletion of `oab-prod-team-bot` via + // `prod/team-bot` — and vice versa. + assert_eq!( + crate::identity::collision_aliases("prod", "team-bot"), + vec![("prod-team".to_string(), "bot".to_string())] + ); + assert!(crate::identity::collision_aliases("prod-team", "bot") + .contains(&("prod".to_string(), "team-bot".to_string()))); + // 3. The probed ownership keys are derived from the same helpers the + // mutation paths use, so probe and mutation cannot drift apart. + assert_eq!( + crate::identity::ownership_keys("prod-team", "bot")[1], + checkpoint_key("prod-team", "bot"), + ); } #[test] diff --git a/operator/src/identity.rs b/operator/src/identity.rs new file mode 100644 index 000000000..6df0795bf --- /dev/null +++ b/operator/src/identity.rs @@ -0,0 +1,262 @@ +//! Shared logical→physical identity rules for apply and delete. +//! +//! Apply and delete derive every physical resource identity — the ECS service +//! name, the Cloud Map service name, and the control-plane S3 keys — from the +//! logical `namespace`/`name` pair via `oab-{namespace}-{name}`. Because `-` +//! is the delimiter and names may legitimately contain `-`, that mapping is +//! injective only while namespaces stay hyphen-free: `prod/team-bot` and +//! `prod-team/bot` would otherwise both resolve to `oab-prod-team-bot`. +//! +//! Two rules enforce injectivity across every entry point: +//! +//! 1. **Domain rule** — [`validate_injective_identity`] rejects hyphenated +//! namespaces before any AWS call. It is enforced by +//! [`crate::manifest::OABServiceManifest::validate`] (covering CLI apply, +//! fleet expansion, and programmatic +//! [`crate::apply::apply_manifests`]) and by programmatic +//! [`crate::delete::delete_services`]. +//! 2. **Ownership rule** — [`ensure_exclusive_physical_identity`] runs before +//! any destructive mutation on both the apply and delete paths. It fails +//! closed when the control plane records any *other* logical pair (for +//! example a legacy hyphenated namespace) claiming the same physical name. +//! +//! # Legacy hyphenated namespaces +//! +//! Deployments created under a hyphenated namespace before the domain rule +//! existed can no longer be applied — apply fails with a migration message. +//! They remain deletable through the `oabctl delete` CLI, which accepts a +//! hyphenated namespace for teardown and relies on the ownership rule to +//! refuse when the physical name is contested by another recorded logical +//! pair. The migration path is: `oabctl delete` the legacy deployment, then +//! re-create it under a hyphen-free namespace. + +use anyhow::Context; +use aws_sdk_s3::error::ProvideErrorMetadata; + +/// Physical service identity shared by ECS and Cloud Map: +/// `oab-{namespace}-{name}`. +pub(crate) fn physical_service_name(namespace: &str, name: &str) -> String { + format!("oab-{namespace}-{name}") +} + +/// Control-plane key of the stored desired-state manifest. +pub(crate) fn manifest_key(namespace: &str, name: &str) -> String { + format!("manifests/{namespace}/{name}.yaml") +} + +/// Control-plane key of the durable delete checkpoint. +pub(crate) fn delete_checkpoint_key(namespace: &str, name: &str) -> String { + format!("delete-checkpoints/{namespace}/{name}.json") +} + +/// Control-plane key of the apply-side ingress-teardown checkpoint. +pub(crate) fn ingress_teardown_checkpoint_key(namespace: &str, name: &str) -> String { + format!("ingress-teardown-checkpoints/{namespace}/{name}.json") +} + +/// Domain rule: `namespace` and `name` must be non-empty and `namespace` must +/// not contain `-`, so that `oab-{namespace}-{name}` parses back to exactly +/// one logical pair. +pub(crate) fn validate_injective_identity(namespace: &str, name: &str) -> anyhow::Result<()> { + if namespace.trim().is_empty() { + anyhow::bail!("namespace must not be empty (got '{namespace}'/'{name}')"); + } + if name.trim().is_empty() { + anyhow::bail!("name must not be empty (got '{namespace}'/'{name}')"); + } + if namespace.contains('-') { + anyhow::bail!( + "namespace '{namespace}' must not contain '-': it is the delimiter of the physical \ + identity '{physical}', so a hyphenated namespace collides with other logical \ + targets (for example 'a-b/c' and 'a/b-c'). Legacy deployments created under a \ + hyphenated namespace remain deletable with `oabctl delete`, which verifies \ + exclusive ownership in the control plane; re-create them under a hyphen-free \ + namespace", + physical = physical_service_name(namespace, name), + ); + } + Ok(()) +} + +/// Every logical pair distinct from `(namespace, name)` that maps to the same +/// physical identity. These are the alternative split points of +/// `{namespace}-{name}` and only exist when a component contains `-`. +pub(crate) fn collision_aliases(namespace: &str, name: &str) -> Vec<(String, String)> { + let joined = format!("{namespace}-{name}"); + let mut aliases = Vec::new(); + for (index, _) in joined.match_indices('-') { + let alias_namespace = &joined[..index]; + let alias_name = &joined[index + 1..]; + if alias_namespace.is_empty() || alias_name.is_empty() { + continue; + } + if alias_namespace == namespace && alias_name == name { + continue; + } + aliases.push((alias_namespace.to_string(), alias_name.to_string())); + } + aliases +} + +/// Control-plane keys whose presence records ownership of a logical pair. +pub(crate) fn ownership_keys(namespace: &str, name: &str) -> [String; 3] { + [ + manifest_key(namespace, name), + delete_checkpoint_key(namespace, name), + ingress_teardown_checkpoint_key(namespace, name), + ] +} + +fn contested_identity_error( + namespace: &str, + name: &str, + alias_namespace: &str, + alias_name: &str, + bucket: &str, + key: &str, +) -> anyhow::Error { + anyhow::anyhow!( + "physical identity '{physical}' is contested: the control plane records \ + '{alias_namespace}/{alias_name}' (s3://{bucket}/{key}), which maps to the same \ + physical name as '{namespace}/{name}'. Refusing to mutate. Resolve the collision \ + first: delete the legacy '{alias_namespace}/{alias_name}' deployment with \ + `oabctl delete`, or pick a namespace/name pair that does not collide", + physical = physical_service_name(namespace, name), + ) +} + +/// Ownership rule: fail closed if the control plane records any logical pair, +/// distinct from `(namespace, name)`, that claims the same physical identity. +/// +/// Runs before any destructive mutation on the apply and delete paths. Probes +/// only the exact alias keys (stored manifest, delete checkpoint, and +/// ingress-teardown checkpoint), so hyphen-free pairs with hyphen-free names +/// cost zero requests. Probe failures other than a missing key fail closed. +pub(crate) async fn ensure_exclusive_physical_identity( + s3: &aws_sdk_s3::Client, + bucket: &str, + namespace: &str, + name: &str, +) -> anyhow::Result<()> { + for (alias_namespace, alias_name) in collision_aliases(namespace, name) { + for key in ownership_keys(&alias_namespace, &alias_name) { + match s3.get_object().bucket(bucket).key(&key).send().await { + Ok(_) => { + return Err(contested_identity_error( + namespace, + name, + &alias_namespace, + &alias_name, + bucket, + &key, + )); + } + Err(error) if matches!(error.code(), Some("NoSuchKey" | "NotFound")) => {} + Err(error) => { + return Err(error).with_context(|| { + format!( + "failed to verify exclusive ownership of '{}' via s3://{bucket}/{key}", + physical_service_name(namespace, name) + ) + }); + } + } + } + } + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn accepts_hyphen_free_namespace_with_hyphenated_name() { + validate_injective_identity("prod", "nest-my-oab").expect("valid identity"); + } + + #[test] + fn rejects_empty_components() { + assert!(validate_injective_identity("", "bot").is_err()); + assert!(validate_injective_identity("prod", " ").is_err()); + } + + #[test] + fn rejects_hyphenated_namespace_with_migration_hint() { + let error = validate_injective_identity("prod-team", "bot").unwrap_err(); + let message = error.to_string(); + assert!(message.contains("must not contain '-'"), "{message}"); + assert!(message.contains("oab-prod-team-bot"), "{message}"); + assert!(message.contains("oabctl delete"), "{message}"); + } + + #[test] + fn hyphen_free_pair_has_no_collision_aliases() { + assert!(collision_aliases("prod", "bot").is_empty()); + } + + #[test] + fn collision_aliases_enumerate_every_alternative_split() { + assert_eq!( + collision_aliases("prod", "team-bot"), + vec![("prod-team".to_string(), "bot".to_string())] + ); + assert_eq!( + collision_aliases("a", "b-c-d"), + vec![ + ("a-b".to_string(), "c-d".to_string()), + ("a-b-c".to_string(), "d".to_string()), + ] + ); + } + + #[test] + fn collision_aliases_are_symmetric_across_the_colliding_pair() { + // The new-domain pair sees the legacy pair… + assert!(collision_aliases("prod", "team-bot") + .contains(&("prod-team".to_string(), "bot".to_string()))); + // …and the legacy pair sees the new-domain pair, so the ownership + // probe protects both directions of the collision. + assert!(collision_aliases("prod-team", "bot") + .contains(&("prod".to_string(), "team-bot".to_string()))); + } + + #[test] + fn colliding_pairs_share_one_physical_identity() { + assert_eq!( + physical_service_name("prod", "team-bot"), + physical_service_name("prod-team", "bot"), + ); + } + + #[test] + fn ownership_keys_cover_manifest_and_both_checkpoints() { + assert_eq!( + ownership_keys("prod-team", "bot"), + [ + "manifests/prod-team/bot.yaml".to_string(), + "delete-checkpoints/prod-team/bot.json".to_string(), + "ingress-teardown-checkpoints/prod-team/bot.json".to_string(), + ] + ); + } + + #[test] + fn contested_identity_error_names_both_pairs_and_the_evidence_key() { + let error = contested_identity_error( + "prod", + "team-bot", + "prod-team", + "bot", + "control-plane", + "manifests/prod-team/bot.yaml", + ); + let message = error.to_string(); + assert!(message.contains("oab-prod-team-bot"), "{message}"); + assert!(message.contains("prod-team/bot"), "{message}"); + assert!( + message.contains("s3://control-plane/manifests/prod-team/bot.yaml"), + "{message}" + ); + } +} diff --git a/operator/src/ingress.rs b/operator/src/ingress.rs index 29175bb66..9d3ae1581 100644 --- a/operator/src/ingress.rs +++ b/operator/src/ingress.rs @@ -538,7 +538,7 @@ pub(crate) async fn delete_exact( delete_cloud_map_by_arn( config, registry_arn, - &format!("oab-{namespace}-{name}"), + &crate::identity::physical_service_name(namespace, name), (partition, account, region), ) .await?; diff --git a/operator/src/lib.rs b/operator/src/lib.rs index 52ad9e4f6..87b92075f 100644 --- a/operator/src/lib.rs +++ b/operator/src/lib.rs @@ -48,10 +48,19 @@ //! otherwise both use the shared `OAB_CONTROL_PLANE_BUCKET` then caller-account //! resolution chain. //! +//! Apply and delete share one injective logical-identity rule: namespaces +//! must not contain `-`; names may contain it, so the physical +//! `oab-{namespace}-{name}` identity parses back to exactly one accepted +//! `namespace`/`name` pair. Manifest validation (all apply entry points) and +//! [`delete_services`] both reject hyphenated namespaces before any AWS call. +//! Before mutating, apply and delete additionally verify in the control plane +//! that no other recorded logical pair (for example a legacy hyphenated +//! namespace) claims the same physical name, and fail closed otherwise. +//! Legacy hyphenated-namespace deployments cannot be re-applied but remain +//! deletable via the `oabctl delete` CLI under that same ownership check. +//! //! [`delete_services`] tears down by `namespace`+`name` ([`DeleteTarget`]). -//! Delete namespaces must not contain `-`; names may contain it, making the -//! existing `oab-{namespace}-{name}` resource identity injective for accepted -//! targets. Before ECS mutation delete durably records the resolved bucket, +//! Before ECS mutation delete durably records the resolved bucket, //! caller partition/account/region, canonical cluster/service ARNs, the ECS //! service `createdAt` incarnation, and exact ingress IDs in S3. Initial //! identity ambiguity fails closed; retries use only checkpointed IDs and @@ -74,6 +83,7 @@ mod control_plane; mod create; pub mod delete; mod get; +mod identity; mod ingress; pub mod manifest; mod scale; diff --git a/operator/src/manifest.rs b/operator/src/manifest.rs index fe665f82a..92f535f00 100644 --- a/operator/src/manifest.rs +++ b/operator/src/manifest.rs @@ -303,6 +303,14 @@ impl OABServiceManifest { if self.metadata.namespace.is_empty() { anyhow::bail!("metadata.namespace is required"); } + // Shared injective identity rule: apply must not create logical pairs + // whose physical `oab-{namespace}-{name}` identity collides with + // another pair's. See `crate::identity` for the rule and the legacy + // hyphenated-namespace policy. + crate::identity::validate_injective_identity( + &self.metadata.namespace, + &self.metadata.name, + )?; if self.spec.image.is_empty() { anyhow::bail!("spec.image is required"); } @@ -352,13 +360,13 @@ impl OABServiceManifest { } pub fn ecs_service_name(&self) -> String { - format!("oab-{}-{}", self.metadata.namespace, self.metadata.name) + crate::identity::physical_service_name(&self.metadata.namespace, &self.metadata.name) } /// Cloud Map service name for this manifest (unique per namespace+name). /// Resolves to `.` in private DNS. pub fn cloud_map_service_name(&self) -> String { - format!("oab-{}-{}", self.metadata.namespace, self.metadata.name) + crate::identity::physical_service_name(&self.metadata.namespace, &self.metadata.name) } } @@ -405,6 +413,30 @@ spec: m.validate().expect("valid"); } + #[test] + fn rejects_hyphenated_namespace_with_migration_hint() { + // Injective identity rule: `prod-team/bot` collides with + // `prod/team-bot` on `oab-prod-team-bot`, so manifest validation (the + // gate for CLI apply, fleet expansion, and programmatic apply) must + // reject hyphenated namespaces before any AWS call. + let mut m = parse(ECS_SERVICE_WITH_INGRESS); + m.metadata.namespace = "prod-team".to_string(); + m.metadata.name = "bot".to_string(); + let message = m.validate().unwrap_err().to_string(); + assert!(message.contains("must not contain '-'"), "{message}"); + assert!(message.contains("oabctl delete"), "{message}"); + } + + #[test] + fn accepts_hyphenated_name_with_hyphen_free_namespace() { + let mut m = parse(ECS_SERVICE_WITH_INGRESS); + m.metadata.namespace = "prod".to_string(); + m.metadata.name = "team-bot".to_string(); + m.validate().expect("hyphenated names remain valid"); + assert_eq!(m.ecs_service_name(), "oab-prod-team-bot"); + assert_eq!(m.cloud_map_service_name(), "oab-prod-team-bot"); + } + #[test] fn ingress_is_optional_and_backward_compatible() { let yaml = ECS_SERVICE_WITH_INGRESS.split(" ingress:").next().unwrap(); diff --git a/operator/src/scale.rs b/operator/src/scale.rs index 77765a3d8..b9ee183ce 100644 --- a/operator/src/scale.rs +++ b/operator/src/scale.rs @@ -12,7 +12,7 @@ async fn resolve_service( crate::config::OabConfig::load().context("failed to load ~/.oabctl/config.toml")?; let cluster = oab_cfg.defaults.cluster; let namespace = oab_cfg.defaults.namespace; - let service_name = format!("oab-{}-{}", namespace, name); + let service_name = crate::identity::physical_service_name(&namespace, name); // Verify the service exists in the oab cluster let ecs = aws_sdk_ecs::Client::new(aws_config); From 9e4e59bf0d0c85cbc2aa2d795e4c97f0c05259c3 Mon Sep 17 00:00:00 2001 From: chaodu-agent <274062505+chaodu-agent@users.noreply.github.com> Date: Wed, 22 Jul 2026 17:32:41 +0000 Subject: [PATCH 09/20] fix(oabctl): close delete review gaps --- docs/oabctl.md | 9 ++++++--- operator/README.md | 10 +++++++--- operator/src/apply.rs | 8 +++++--- operator/src/delete.rs | 30 +++++++++++++++++++++++++++--- operator/src/ingress.rs | 5 ++++- operator/src/lib.rs | 9 ++++++--- 6 files changed, 55 insertions(+), 16 deletions(-) diff --git a/docs/oabctl.md b/docs/oabctl.md index bd8ef4643..1a7a3c18b 100644 --- a/docs/oabctl.md +++ b/docs/oabctl.md @@ -406,9 +406,12 @@ must still be registered manually in the LINE Developers console. > > Programmatic apply and delete do not provide an internal same-target lock. > Callers must serialize mutations for the same AWS account, Region, -> control-plane bucket, ECS cluster, namespace, and name. After an accidental -> overlap, stop concurrent writers, inspect retained checkpoints, then -> explicitly re-apply the desired state or retry delete. +> control-plane bucket, ECS cluster, and physical service identity. This +> includes every alias-equivalent logical pair that could map to the same +> `oab-{namespace}-{name}` value (for example `prod/team-bot` and +> `prod-team/bot`). After an accidental overlap, stop concurrent writers, +> inspect retained checkpoints, then explicitly re-apply the desired state or +> retry delete. > > If you edit a manifest to remove `spec.ingress` while keeping the bot, `apply` first > stores every exact ECS registry ARN only when the previously stored OAB diff --git a/operator/README.md b/operator/README.md index 3572581ca..a50daf939 100644 --- a/operator/README.md +++ b/operator/README.md @@ -156,9 +156,13 @@ deployment and re-creating it under a hyphen-free namespace. Programmatic apply and delete do not provide an internal same-target lock. Callers must serialize mutations for the same AWS account, Region, control-plane -bucket, ECS cluster, namespace, and name. If overlapping calls occur, stop -concurrent writers, inspect the retained checkpoint, then explicitly re-apply -the desired state or retry delete. +bucket, ECS cluster, and physical service identity. This includes every +alias-equivalent logical pair that could map to the same +`oab-{namespace}-{name}` value (for example `prod/team-bot` and +`prod-team/bot`). The complete delete target set is validated before any AWS +request; duplicate logical targets are rejected. If overlapping calls occur, +stop concurrent writers, inspect the retained checkpoint, then explicitly +re-apply the desired state or retry delete. For `aws-sm://#`, a non-ARN `` requires the caller to have `secretsmanager:DescribeSecret`; full-ARN shorthand does not diff --git a/operator/src/apply.rs b/operator/src/apply.rs index fd985a5a0..1173e7c46 100644 --- a/operator/src/apply.rs +++ b/operator/src/apply.rs @@ -412,9 +412,11 @@ pub(crate) async fn run( /// /// This function is not serialized with [`crate::delete::delete_services`]. /// Callers must serialize mutations for the same AWS account, Region, -/// control-plane bucket, ECS cluster, namespace, and name. If that precondition -/// is violated, stop concurrent writers and re-apply the intended desired state -/// before resuming other mutations. +/// control-plane bucket, ECS cluster, and physical service identity. This +/// includes every alias-equivalent logical pair that could map to the same +/// `oab-{namespace}-{name}` value (for example `prod/team-bot` and +/// `prod-team/bot`). If that precondition is violated, stop concurrent writers +/// and re-apply the intended desired state before resuming other mutations. pub async fn apply_manifests( aws_config: &aws_config::SdkConfig, manifests: &[OABServiceManifest], diff --git a/operator/src/delete.rs b/operator/src/delete.rs index f0e9ab855..2ccbc4664 100644 --- a/operator/src/delete.rs +++ b/operator/src/delete.rs @@ -415,12 +415,21 @@ fn validate_delete_request( "DeleteOptions.cluster must not be empty" ))); } + + let mut seen = std::collections::HashSet::with_capacity(targets.len()); for target in targets { // Shared injective identity rule (see `crate::identity`): non-empty // components and a hyphen-free namespace, matching what apply-side // manifest validation accepts. crate::identity::validate_injective_identity(&target.namespace, &target.name) .map_err(DeleteError::validation)?; + if !seen.insert((&target.namespace, &target.name)) { + return Err(DeleteError::validation(anyhow::anyhow!( + "duplicate delete target '{}/{}'", + target.namespace, + target.name + ))); + } } Ok(()) } @@ -466,9 +475,12 @@ fn record_delete_result( /// /// This function is not serialized with [`crate::apply::apply_manifests`]. /// Callers must serialize mutations for the same AWS account, Region, -/// control-plane bucket, ECS cluster, namespace, and name. If that precondition -/// is violated, stop concurrent writers, inspect the retained checkpoint, then -/// either re-apply the intended desired state or retry delete. +/// control-plane bucket, ECS cluster, and physical service identity. This +/// includes every alias-equivalent logical pair that could map to the same +/// `oab-{namespace}-{name}` value (for example `prod/team-bot` and +/// `prod-team/bot`). If that precondition is violated, stop concurrent +/// writers, inspect the retained checkpoint, then either re-apply the intended +/// desired state or retry delete. /// /// Before ECS mutation, delete persists an exact-identity checkpoint containing /// the caller account/region, bucket, canonical cluster and service ARNs, and @@ -1183,6 +1195,18 @@ mod tests { assert_eq!(err.kind, DeleteErrorKind::Validation); } + #[test] + fn delete_request_rejects_duplicate_targets_before_aws() { + let targets = [ + DeleteTarget::new("prod", "bot"), + DeleteTarget::new("prod", "bot"), + ]; + let error = validate_delete_request(&targets, &DeleteOptions::new("cluster")) + .expect_err("duplicate targets must fail validation"); + assert_eq!(error.kind, DeleteErrorKind::Validation); + assert!(error.to_string().contains("duplicate delete target"), "{error}"); + } + #[test] fn initial_describe_failures_and_missing_service_fail_closed() { assert!(classify_service_identity(false, false, None, &["MISSING"]).is_err()); diff --git a/operator/src/ingress.rs b/operator/src/ingress.rs index 9d3ae1581..086423578 100644 --- a/operator/src/ingress.rs +++ b/operator/src/ingress.rs @@ -216,7 +216,10 @@ pub async fn register_telegram_webhook( form.push(("secret_token".to_string(), st)); } - let client = reqwest::Client::new(); + let client = reqwest::Client::builder() + .timeout(std::time::Duration::from_secs(10)) + .build() + .context("failed to build Telegram webhook HTTP client")?; let resp = client .post(format!( "https://api.telegram.org/bot{bot_token}/setWebhook" diff --git a/operator/src/lib.rs b/operator/src/lib.rs index 87b92075f..24108533c 100644 --- a/operator/src/lib.rs +++ b/operator/src/lib.rs @@ -71,9 +71,12 @@ //! //! Programmatic apply and delete do not provide an internal same-target lock. //! Callers must serialize mutations for the same AWS account, Region, -//! control-plane bucket, ECS cluster, namespace, and name. After an accidental -//! overlap, stop concurrent writers, inspect any retained checkpoint, and -//! explicitly re-apply the desired state or retry delete. +//! control-plane bucket, ECS cluster, and physical service identity. This +//! includes every alias-equivalent logical pair that could map to the same +//! `oab-{namespace}-{name}` value (for example `prod/team-bot` and +//! `prod-team/bot`). After an accidental overlap, stop concurrent writers, +//! inspect any retained checkpoint, and explicitly re-apply the desired state +//! or retry delete. pub mod apply; mod bootstrap; From a635c4853c142e10f7b7885ec6dd79f27176fd3d Mon Sep 17 00:00:00 2001 From: chaodu-agent <274062505+chaodu-agent@users.noreply.github.com> Date: Wed, 22 Jul 2026 21:41:01 +0000 Subject: [PATCH 10/20] docs(oabctl): clarify teardown and bucket fallback --- docs/oabctl.md | 12 ++++++------ operator/README.md | 5 +++-- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/docs/oabctl.md b/docs/oabctl.md index 1a7a3c18b..7b899a794 100644 --- a/docs/oabctl.md +++ b/docs/oabctl.md @@ -416,12 +416,12 @@ must still be registered manually in the LINE Developers console. > If you edit a manifest to remove `spec.ingress` while keeping the bot, `apply` first > stores every exact ECS registry ARN only when the previously stored OAB > manifest owned ingress (or resumes an already-valid checkpoint); an arbitrary -> attached registry is never enough. -> `ingress-teardown-checkpoints//.json`, clears only API wiring -> bound to those ARNs, detaches all registries from ECS, waits until the detach -> is observable, and only then deletes each exact Cloud Map service. It keeps -> the HTTP API resource so its URL can survive re-enabling ingress and removes -> the checkpoint only after the full apply succeeds, so retries cannot lose +> attached registry is never enough. The resulting exact-identity record is +> stored at `ingress-teardown-checkpoints//.json`; it clears only +> API wiring bound to those ARNs, detaches all registries from ECS, waits until +> the detach is observable, and only then deletes each exact Cloud Map service. +> It keeps the HTTP API resource so its URL can survive re-enabling ingress and +> removes the checkpoint only after the full apply succeeds, so retries cannot lose > cleanup identity. The **shared** VPC Link and security-group inbound rule are > always left in place for other bots. > diff --git a/operator/README.md b/operator/README.md index a50daf939..0ea4d2d93 100644 --- a/operator/README.md +++ b/operator/README.md @@ -101,8 +101,9 @@ async fn reconcile( delete_services( aws, &[DeleteTarget::new("prod", "bot")], - &DeleteOptions::new("production-cluster") - .with_control_plane_bucket("my-control-plane-bucket"), + // Omit the bucket override to use OAB_CONTROL_PLANE_BUCKET, + // then oab-control-plane-{account} from the caller identity. + &DeleteOptions::new("production-cluster"), ) .await?; Ok(()) From 55f0172746518ca24fc423b77c3ce30215758fdd Mon Sep 17 00:00:00 2001 From: chaodu-agent <274062505+chaodu-agent@users.noreply.github.com> Date: Wed, 22 Jul 2026 21:42:47 +0000 Subject: [PATCH 11/20] fix(oabctl): bound identity ownership probes --- operator/src/identity.rs | 116 ++++++++++++++++++++++++++++++++------- 1 file changed, 97 insertions(+), 19 deletions(-) diff --git a/operator/src/identity.rs b/operator/src/identity.rs index 6df0795bf..a1a741bb9 100644 --- a/operator/src/identity.rs +++ b/operator/src/identity.rs @@ -32,6 +32,9 @@ use anyhow::Context; use aws_sdk_s3::error::ProvideErrorMetadata; +use std::sync::Arc; +use tokio::sync::Semaphore; +use tokio::task::JoinSet; /// Physical service identity shared by ECS and Cloud Map: /// `oab-{namespace}-{name}`. @@ -125,41 +128,100 @@ fn contested_identity_error( ) } +const OWNERSHIP_PROBE_CONCURRENCY: usize = 8; + +async fn probe_ownership_key( + s3: aws_sdk_s3::Client, + bucket: String, + namespace: String, + name: String, + alias_namespace: String, + alias_name: String, + key: String, +) -> anyhow::Result<()> { + match s3.get_object().bucket(&bucket).key(&key).send().await { + Ok(_) => Err(contested_identity_error( + &namespace, + &name, + &alias_namespace, + &alias_name, + &bucket, + &key, + )), + Err(error) if matches!(error.code(), Some("NoSuchKey" | "NotFound")) => Ok(()), + Err(error) => Err(error).with_context(|| { + format!( + "failed to verify exclusive ownership of '{}' via s3://{bucket}/{key}", + physical_service_name(&namespace, &name) + ) + }), + } +} + /// Ownership rule: fail closed if the control plane records any logical pair, /// distinct from `(namespace, name)`, that claims the same physical identity. /// /// Runs before any destructive mutation on the apply and delete paths. Probes /// only the exact alias keys (stored manifest, delete checkpoint, and /// ingress-teardown checkpoint), so hyphen-free pairs with hyphen-free names -/// cost zero requests. Probe failures other than a missing key fail closed. +/// cost zero requests. Alias probes are bounded to +/// [`OWNERSHIP_PROBE_CONCURRENCY`] in flight; probe failures other than a +/// missing key fail closed. pub(crate) async fn ensure_exclusive_physical_identity( s3: &aws_sdk_s3::Client, bucket: &str, namespace: &str, name: &str, ) -> anyhow::Result<()> { + let semaphore = Arc::new(Semaphore::new(OWNERSHIP_PROBE_CONCURRENCY)); + let mut probes = JoinSet::new(); + for (alias_namespace, alias_name) in collision_aliases(namespace, name) { for key in ownership_keys(&alias_namespace, &alias_name) { - match s3.get_object().bucket(bucket).key(&key).send().await { - Ok(_) => { - return Err(contested_identity_error( - namespace, - name, - &alias_namespace, - &alias_name, - bucket, - &key, - )); - } - Err(error) if matches!(error.code(), Some("NoSuchKey" | "NotFound")) => {} + let permit = match semaphore.clone().acquire_owned().await { + Ok(permit) => permit, Err(error) => { - return Err(error).with_context(|| { - format!( - "failed to verify exclusive ownership of '{}' via s3://{bucket}/{key}", - physical_service_name(namespace, name) - ) - }); + probes.abort_all(); + while probes.join_next().await.is_some() {} + return Err(anyhow::anyhow!( + "ownership probe concurrency gate closed: {error}" + )); } + }; + let probe_s3 = s3.clone(); + let probe_bucket = bucket.to_owned(); + let probe_namespace = namespace.to_owned(); + let probe_name = name.to_owned(); + let probe_alias_namespace = alias_namespace; + let probe_alias_name = alias_name; + probes.spawn(async move { + let _permit = permit; + probe_ownership_key( + probe_s3, + probe_bucket, + probe_namespace, + probe_name, + probe_alias_namespace, + probe_alias_name, + key, + ) + .await + }); + } + } + + while let Some(result) = probes.join_next().await { + match result { + Ok(Ok(())) => {} + Ok(Err(error)) => { + probes.abort_all(); + while probes.join_next().await.is_some() {} + return Err(error); + } + Err(error) => { + probes.abort_all(); + while probes.join_next().await.is_some() {} + return Err(anyhow::anyhow!("ownership probe task failed: {error}")); } } } @@ -210,6 +272,22 @@ mod tests { ); } + #[test] + fn long_hyphenated_names_keep_alias_probe_shape_bounded_by_scheduler() { + let name = (0..101).map(|_| "x").collect::>().join("-"); + assert_eq!(name.len(), 201); + let aliases = collision_aliases("prod", &name); + assert_eq!(aliases.len(), 100); + assert_eq!( + aliases + .iter() + .map(|(namespace, name)| ownership_keys(namespace, name).len()) + .sum::(), + 300 + ); + assert!(OWNERSHIP_PROBE_CONCURRENCY < 300); + } + #[test] fn collision_aliases_are_symmetric_across_the_colliding_pair() { // The new-domain pair sees the legacy pair… From f196efeb104ee40685b32e461189e74a77b66f79 Mon Sep 17 00:00:00 2001 From: chaodu-agent <274062505+chaodu-agent@users.noreply.github.com> Date: Wed, 22 Jul 2026 21:47:43 +0000 Subject: [PATCH 12/20] fix(oabctl): preserve CLI cleanup warnings --- docs/oabctl.md | 7 +- operator/README.md | 7 ++ operator/src/apply.rs | 20 +++-- operator/src/delete.rs | 158 ++++++++++++++++++++++++++++++--------- operator/src/identity.rs | 21 ++---- operator/src/lib.rs | 7 ++ 6 files changed, 156 insertions(+), 64 deletions(-) diff --git a/docs/oabctl.md b/docs/oabctl.md index 7b899a794..aa0d6c60a 100644 --- a/docs/oabctl.md +++ b/docs/oabctl.md @@ -380,7 +380,12 @@ must still be registered manually in the LINE Developers console. > empty service responses, and a missing service fail closed; a matching checkpoint > authorizes retry only when ECS explicitly reports exactly one `MISSING` > failure with zero services, or an `INACTIVE` response from the original -> service incarnation. +> service incarnation. The control-plane bucket must enforce default +> server-side encryption and S3 versioning for durable checkpoint recovery; +> oabctl writes application JSON but does not weaken or validate those +> bucket-level policies per request. The legacy CLI preserves best-effort +> dependent/S3 cleanup warnings, while the programmatic `delete_services` API +> keeps exact-identity and checkpoint cleanup failures fatal for safe retry. > > API cleanup never selects the first same-named API: duplicate names fail > closed, and the sole candidate is checkpointed only when its integration URI diff --git a/operator/README.md b/operator/README.md index 0ea4d2d93..409a4e4fe 100644 --- a/operator/README.md +++ b/operator/README.md @@ -138,6 +138,13 @@ record exists, delete fails before destructive mutation so it cannot discard unfinished exact cleanup identity. Re-run the ingress-free apply to completion, then retry delete. +The control-plane bucket must enforce default server-side encryption and S3 +versioning for durable checkpoint recovery. The library writes application JSON +but does not weaken or validate those bucket-level policies per request. The +legacy CLI preserves best-effort warnings for dependent/S3 cleanup, while +`delete_services` keeps exact-identity and checkpoint cleanup failures fatal so +callers can retry safely. + Apply and delete share one injective logical-identity rule: namespaces must not contain `-`; names may contain it, so the physical `oab-{namespace}-{name}` identity always parses back to exactly one `namespace`/`name` pair. Manifest diff --git a/operator/src/apply.rs b/operator/src/apply.rs index 1173e7c46..20a2a49d9 100644 --- a/operator/src/apply.rs +++ b/operator/src/apply.rs @@ -213,13 +213,13 @@ async fn load_bootstrap_state(s3: &aws_sdk_s3::Client, bucket: &str) -> Bootstra Ok(None) => BootstrapResolution { state: None, warning: Some(format!( - "no bootstrap state found in s3://{bucket}/bootstrap-state.json (run `oabctl bootstrap` first)" + "no bootstrap state found in the control-plane bucket (run `oabctl bootstrap` first)" )), }, Err(error) => BootstrapResolution { state: None, warning: Some(format!( - "failed to read bootstrap state from s3://{bucket}: {error}" + "failed to read bootstrap state from the control-plane bucket: {error}" )), }, } @@ -631,14 +631,12 @@ async fn load_ingress_teardown_checkpoint( .await .context("failed to read ingress teardown checkpoint")? .into_bytes(); - Ok(Some(serde_json::from_slice(&bytes).with_context(|| { - format!("invalid ingress teardown checkpoint s3://{bucket}/{key}") - })?)) + Ok(Some( + serde_json::from_slice(&bytes).context("invalid ingress teardown checkpoint payload")?, + )) } Err(error) if matches!(error.code(), Some("NoSuchKey" | "NotFound")) => Ok(None), - Err(error) => Err(error).with_context(|| { - format!("failed to read ingress teardown checkpoint s3://{bucket}/{key}") - }), + Err(error) => Err(error).context("failed to read ingress teardown checkpoint"), } } @@ -668,7 +666,7 @@ async fn save_ingress_teardown_checkpoint( .content_type("application/json") .send() .await - .with_context(|| format!("failed to persist s3://{bucket}/{key}"))?; + .context("failed to persist ingress teardown checkpoint")?; Ok(()) } @@ -684,7 +682,7 @@ async fn remove_ingress_teardown_checkpoint( .key(&key) .send() .await - .with_context(|| format!("failed to remove s3://{bucket}/{key}"))?; + .context("failed to remove ingress teardown checkpoint")?; Ok(()) } @@ -873,7 +871,7 @@ async fn apply_ecs( Err(error) if matches!(error.code(), Some("NoSuchKey" | "NotFound")) => (0, false), Err(error) => { return Err(error).with_context(|| { - format!("failed to read existing manifest s3://{bucket}/{manifest_key}") + "failed to read existing control-plane manifest" }); } }; diff --git a/operator/src/delete.rs b/operator/src/delete.rs index 2ccbc4664..a525e8236 100644 --- a/operator/src/delete.rs +++ b/operator/src/delete.rs @@ -79,9 +79,11 @@ pub struct DeletedService { pub namespace: String, pub name: String, pub ecs_service_name: String, - /// Non-fatal diagnostics retained for API compatibility. Exact-identity - /// dependent and S3 cleanup failures are fatal so the durable checkpoint - /// remains available for retry. + /// Non-fatal diagnostics retained for API compatibility. Examples include + /// resuming from an already-absent ECS service or skipping dependent ingress + /// cleanup when no exact ingress identity was recorded. Exact-identity + /// dependent and S3 cleanup failures remain fatal for programmatic calls so + /// the durable checkpoint remains available for retry. pub warnings: Vec, } @@ -484,10 +486,14 @@ fn record_delete_result( /// /// Before ECS mutation, delete persists an exact-identity checkpoint containing /// the caller account/region, bucket, canonical cluster and service ARNs, and -/// any ECS registry/API IDs. An absent ECS service or ambiguous -/// `DescribeServices` response is rejected unless that matching durable -/// checkpoint already exists. Cleanup uses only IDs from the checkpoint and -/// removes the checkpoint last, making partial failures safe to retry. +/// any ECS registry/API IDs. The control-plane bucket must provide default +/// server-side encryption and S3 versioning; this library writes application +/// JSON but does not weaken or validate those bucket-level policies per request. +/// An absent ECS service or ambiguous `DescribeServices` response is rejected +/// unless that matching durable checkpoint already exists. Cleanup uses only IDs +/// from the checkpoint and removes the checkpoint last, making partial failures +/// safe to retry. The legacy CLI uses best-effort warnings for dependent/S3 +/// cleanup, while this programmatic API keeps those failures fatal. pub async fn delete_services( aws_config: &aws_config::SdkConfig, targets: &[DeleteTarget], @@ -511,6 +517,7 @@ pub async fn delete_services( &opts.cluster, &target.namespace, &bucket, + CleanupMode::Strict, ) .await; report = record_delete_result(report, target, outcome)?; @@ -587,6 +594,7 @@ pub(crate) async fn run_from_file( cluster, &manifest.metadata.namespace, &bucket, + CleanupMode::BestEffort, ) .await { @@ -625,6 +633,7 @@ pub(crate) async fn run( cluster, namespace, &bucket, + CleanupMode::BestEffort, ) .await .map(|_warnings| ()) @@ -666,11 +675,10 @@ async fn load_checkpoint( let bytes = response.body.collect().await .context("failed to read delete checkpoint body")?.into_bytes(); Ok(Some(serde_json::from_slice(&bytes) - .with_context(|| format!("invalid delete checkpoint s3://{bucket}/{key}"))?)) + .context("invalid delete checkpoint payload")?)) } Err(error) if matches!(error.code(), Some("NoSuchKey" | "NotFound")) => Ok(None), - Err(error) => Err(error) - .with_context(|| format!("failed to read delete checkpoint s3://{bucket}/{key}")), + Err(error) => Err(error).context("failed to read delete checkpoint"), } } @@ -679,9 +687,7 @@ async fn save_checkpoint(s3: &aws_sdk_s3::Client, checkpoint: &DeleteCheckpoint) let body = serde_json::to_vec_pretty(checkpoint)?; s3.put_object().bucket(&checkpoint.bucket).key(&key) .body(ByteStream::from(body)).content_type("application/json").send().await - .with_context(|| format!( - "failed to persist exact delete identity to s3://{}/{key}", checkpoint.bucket - ))?; + .context("failed to persist exact delete identity checkpoint")?; Ok(()) } @@ -706,6 +712,12 @@ fn single_described_service( ), } } +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum CleanupMode { + Strict, + BestEffort, +} + enum PreparedIdentity { Exact(Box<(DeleteCheckpoint, ServiceIdentityState)>), } @@ -1024,7 +1036,7 @@ async fn cleanup_s3( ) -> Result<()> { let manifest_key = crate::identity::manifest_key(namespace, name); s3.delete_object().bucket(bucket).key(&manifest_key).send().await - .with_context(|| format!("failed to delete s3://{bucket}/{manifest_key}"))?; + .context("failed to delete control-plane manifest")?; println!(" ✓ Manifest removed from S3"); let artifact_prefix = format!("artifacts/{namespace}/{name}/"); @@ -1032,13 +1044,11 @@ async fn cleanup_s3( loop { let response = s3.list_objects_v2().bucket(bucket).prefix(&artifact_prefix) .set_continuation_token(continuation_token).send().await - .with_context(|| format!( - "failed to list config artifacts under s3://{bucket}/{artifact_prefix}" - ))?; + .context("failed to list control-plane config artifacts")?; for object in response.contents() { if let Some(key) = object.key() { s3.delete_object().bucket(bucket).key(key).send().await - .with_context(|| format!("failed to delete s3://{bucket}/{key}"))?; + .context("failed to delete control-plane config artifact")?; } } continuation_token = response.next_continuation_token().map(str::to_owned); @@ -1055,6 +1065,7 @@ async fn run_with_bucket( cluster: &str, namespace: &str, bucket: &str, + cleanup_mode: CleanupMode, ) -> Result> { if resource != "oabservice" { anyhow::bail!("unknown resource type: {resource}. Use 'oabservice'"); @@ -1085,35 +1096,93 @@ async fn run_with_bucket( .await?; println!("Deleting {name}..."); + let mut warnings = Vec::new(); match prepare_identity( aws_config, &ecs, &s3, namespace, name, cluster, bucket, ) .await? { PreparedIdentity::Exact(boxed) => { let (checkpoint, state) = *boxed; + if state == ServiceIdentityState::RetryGone { + warnings.push( + "ECS service was already absent; resumed exact dependent cleanup".to_string(), + ); + } + if checkpoint.registry_arn.is_none() && checkpoint.api_id.is_none() { + warnings.push( + "no exact ingress identity was recorded; dependent ingress cleanup was skipped" + .to_string(), + ); + } delete_checkpointed_ecs(&ecs, &checkpoint, state).await?; - crate::ingress::delete_exact( - aws_config, - namespace, - name, - checkpoint.api_id.as_deref(), - checkpoint.registry_arn.as_deref(), - &checkpoint.partition, - &checkpoint.account, - &checkpoint.region, - ) - .await?; - cleanup_s3(&s3, bucket, namespace, name).await?; - let key = checkpoint_key(namespace, name); - s3.delete_object().bucket(bucket).key(&key).send().await - .with_context(|| format!( - "cleanup completed but failed to remove s3://{bucket}/{key}" - ))?; - println!(" ✓ Delete checkpoint removed"); + + match cleanup_mode { + CleanupMode::Strict => { + crate::ingress::delete_exact( + aws_config, + namespace, + name, + checkpoint.api_id.as_deref(), + checkpoint.registry_arn.as_deref(), + &checkpoint.partition, + &checkpoint.account, + &checkpoint.region, + ) + .await?; + cleanup_s3(&s3, bucket, namespace, name).await?; + let key = checkpoint_key(namespace, name); + s3.delete_object().bucket(bucket).key(&key).send().await + .context("cleanup completed but failed to remove delete checkpoint")?; + println!(" ✓ Delete checkpoint removed"); + } + CleanupMode::BestEffort => { + let mut cleanup_failed = false; + if let Err(error) = crate::ingress::delete_exact( + aws_config, + namespace, + name, + checkpoint.api_id.as_deref(), + checkpoint.registry_arn.as_deref(), + &checkpoint.partition, + &checkpoint.account, + &checkpoint.region, + ) + .await + { + let warning = format!("ingress cleanup skipped: {error}"); + eprintln!(" ⚠ {warning}"); + warnings.push(warning); + cleanup_failed = true; + } + if let Err(error) = cleanup_s3(&s3, bucket, namespace, name).await { + let warning = format!("S3 cleanup incomplete (checkpoint retained): {error}"); + eprintln!(" ⚠ {warning}"); + warnings.push(warning); + cleanup_failed = true; + } + if !cleanup_failed { + let key = checkpoint_key(namespace, name); + if let Err(error) = s3.delete_object().bucket(bucket).key(&key).send().await { + let warning = format!("delete checkpoint removal skipped (checkpoint retained): {error}"); + eprintln!(" ⚠ {warning}"); + warnings.push(warning); + cleanup_failed = true; + } else { + println!(" ✓ Delete checkpoint removed"); + } + } + if cleanup_failed { + let warning = + "delete checkpoint retained for a later exact-identity retry".to_string(); + eprintln!(" ⚠ {warning}"); + warnings.push(warning); + } + } + } } } println!("\n✓ {name} deleted"); - Ok(Vec::new()) + Ok(warnings) } #[cfg(test)] mod tests { @@ -1207,6 +1276,21 @@ mod tests { assert!(error.to_string().contains("duplicate delete target"), "{error}"); } + #[test] + fn completed_delete_retains_non_fatal_warnings() { + let target = DeleteTarget::new("prod", "bot"); + let report = record_delete_result( + DeleteReport::default(), + &target, + Ok(vec!["resumed from an existing checkpoint".to_string()]), + ) + .expect("completed delete should produce a report"); + assert_eq!( + report.services[0].warnings, + vec!["resumed from an existing checkpoint"] + ); + } + #[test] fn initial_describe_failures_and_missing_service_fail_closed() { assert!(classify_service_identity(false, false, None, &["MISSING"]).is_err()); diff --git a/operator/src/identity.rs b/operator/src/identity.rs index a1a741bb9..2a30cbe9f 100644 --- a/operator/src/identity.rs +++ b/operator/src/identity.rs @@ -115,15 +115,13 @@ fn contested_identity_error( name: &str, alias_namespace: &str, alias_name: &str, - bucket: &str, - key: &str, ) -> anyhow::Error { anyhow::anyhow!( "physical identity '{physical}' is contested: the control plane records \ - '{alias_namespace}/{alias_name}' (s3://{bucket}/{key}), which maps to the same \ - physical name as '{namespace}/{name}'. Refusing to mutate. Resolve the collision \ - first: delete the legacy '{alias_namespace}/{alias_name}' deployment with \ - `oabctl delete`, or pick a namespace/name pair that does not collide", + '{alias_namespace}/{alias_name}', which maps to the same physical name as \ + '{namespace}/{name}'. Refusing to mutate. Resolve the collision first: delete the \ + legacy '{alias_namespace}/{alias_name}' deployment with `oabctl delete`, or pick a \ + namespace/name pair that does not collide", physical = physical_service_name(namespace, name), ) } @@ -145,13 +143,11 @@ async fn probe_ownership_key( &name, &alias_namespace, &alias_name, - &bucket, - &key, )), Err(error) if matches!(error.code(), Some("NoSuchKey" | "NotFound")) => Ok(()), Err(error) => Err(error).with_context(|| { format!( - "failed to verify exclusive ownership of '{}' via s3://{bucket}/{key}", + "failed to verify exclusive ownership of physical identity '{}'", physical_service_name(&namespace, &name) ) }), @@ -326,15 +322,10 @@ mod tests { "team-bot", "prod-team", "bot", - "control-plane", - "manifests/prod-team/bot.yaml", ); let message = error.to_string(); assert!(message.contains("oab-prod-team-bot"), "{message}"); assert!(message.contains("prod-team/bot"), "{message}"); - assert!( - message.contains("s3://control-plane/manifests/prod-team/bot.yaml"), - "{message}" - ); + assert!(!message.contains("s3://"), "{message}"); } } diff --git a/operator/src/lib.rs b/operator/src/lib.rs index 24108533c..2a3e641c9 100644 --- a/operator/src/lib.rs +++ b/operator/src/lib.rs @@ -69,6 +69,13 @@ //! requires the caller to finish the ingress-free apply first, preserving that //! cleanup identity. There is no name-only orphan cleanup fallback. //! +//! The control-plane bucket is an infrastructure prerequisite: it must enforce +//! default server-side encryption and S3 versioning for checkpoint recovery. +//! The library writes application JSON but does not weaken or validate those +//! bucket-level policies on every request. The legacy CLI preserves best-effort +//! dependent/S3 cleanup warnings, while [`delete_services`] keeps exact-identity +//! and checkpoint cleanup failures fatal for retry safety. +//! //! Programmatic apply and delete do not provide an internal same-target lock. //! Callers must serialize mutations for the same AWS account, Region, //! control-plane bucket, ECS cluster, and physical service identity. This From 7471599c5af9168637dffa7d8607df907a8d608a Mon Sep 17 00:00:00 2001 From: chaodu-agent <274062505+chaodu-agent@users.noreply.github.com> Date: Wed, 22 Jul 2026 21:49:21 +0000 Subject: [PATCH 13/20] fix(oabctl): format cleanup diagnostics --- operator/src/delete.rs | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/operator/src/delete.rs b/operator/src/delete.rs index a525e8236..99c7ca9eb 100644 --- a/operator/src/delete.rs +++ b/operator/src/delete.rs @@ -1155,15 +1155,25 @@ async fn run_with_bucket( cleanup_failed = true; } if let Err(error) = cleanup_s3(&s3, bucket, namespace, name).await { - let warning = format!("S3 cleanup incomplete (checkpoint retained): {error}"); + let warning = format!( + "S3 cleanup incomplete (checkpoint retained): {error}" + ); eprintln!(" ⚠ {warning}"); warnings.push(warning); cleanup_failed = true; } if !cleanup_failed { let key = checkpoint_key(namespace, name); - if let Err(error) = s3.delete_object().bucket(bucket).key(&key).send().await { - let warning = format!("delete checkpoint removal skipped (checkpoint retained): {error}"); + if let Err(error) = s3 + .delete_object() + .bucket(bucket) + .key(&key) + .send() + .await + { + let warning = format!( + "delete checkpoint removal skipped (checkpoint retained): {error}" + ); eprintln!(" ⚠ {warning}"); warnings.push(warning); cleanup_failed = true; @@ -1287,7 +1297,7 @@ mod tests { .expect("completed delete should produce a report"); assert_eq!( report.services[0].warnings, - vec!["resumed from an existing checkpoint"] + vec!["resumed from an existing checkpoint".to_string()] ); } From 58027b357b6f9a170869fa36ca710938f5ae0a6d Mon Sep 17 00:00:00 2001 From: chaodu-agent <274062505+chaodu-agent@users.noreply.github.com> Date: Wed, 22 Jul 2026 21:52:05 +0000 Subject: [PATCH 14/20] fix(oabctl): preserve best-effort drain cleanup --- docs/oabctl.md | 13 +++--- operator/README.md | 3 +- operator/src/apply.rs | 2 +- operator/src/delete.rs | 103 ++++++++++++++++++++++++++++++++--------- operator/src/lib.rs | 7 +-- 5 files changed, 95 insertions(+), 33 deletions(-) diff --git a/docs/oabctl.md b/docs/oabctl.md index aa0d6c60a..6f5f7cfb5 100644 --- a/docs/oabctl.md +++ b/docs/oabctl.md @@ -377,14 +377,15 @@ must still be registered manually in the LINE Developers console. > checkpoint is written before ECS mutation and removed only after all > dependent and S3 cleanup succeeds, so rerunning the same command safely > resumes a partial delete. ECS HTTP-200 failures, ambiguous drain responses, -> empty service responses, and a missing service fail closed; a matching checkpoint -> authorizes retry only when ECS explicitly reports exactly one `MISSING` -> failure with zero services, or an `INACTIVE` response from the original -> service incarnation. The control-plane bucket must enforce default +> empty service responses, and a missing service fail closed for strict/programmatic +> cleanup; a matching checkpoint authorizes retry only when ECS explicitly reports +> exactly one `MISSING` failure with zero services, or an `INACTIVE` response from +> the original service incarnation. The legacy CLI warns on a drain timeout, +> continues dependent/S3 cleanup, retains the checkpoint for retry, and continues +> after individual S3 object failures. The control-plane bucket must enforce default > server-side encryption and S3 versioning for durable checkpoint recovery; > oabctl writes application JSON but does not weaken or validate those -> bucket-level policies per request. The legacy CLI preserves best-effort -> dependent/S3 cleanup warnings, while the programmatic `delete_services` API +> bucket-level policies per request. The programmatic `delete_services` API > keeps exact-identity and checkpoint cleanup failures fatal for safe retry. > > API cleanup never selects the first same-named API: duplicate names fail diff --git a/operator/README.md b/operator/README.md index 409a4e4fe..3dee4ac13 100644 --- a/operator/README.md +++ b/operator/README.md @@ -141,7 +141,8 @@ then retry delete. The control-plane bucket must enforce default server-side encryption and S3 versioning for durable checkpoint recovery. The library writes application JSON but does not weaken or validate those bucket-level policies per request. The -legacy CLI preserves best-effort warnings for dependent/S3 cleanup, while +legacy CLI warns on a drain timeout, continues dependent/S3 cleanup, retains the +checkpoint for retry, and continues after individual S3 object failures, while `delete_services` keeps exact-identity and checkpoint cleanup failures fatal so callers can retry safely. diff --git a/operator/src/apply.rs b/operator/src/apply.rs index 20a2a49d9..1b54e749e 100644 --- a/operator/src/apply.rs +++ b/operator/src/apply.rs @@ -794,7 +794,7 @@ async fn wait_for_registry_detach( } let service = match response.services() { [service] => service, - [] => anyhow::bail!("ECS service disappeared while verifying registry detach"), + [] => return Ok(()), services => anyhow::bail!( "ECS returned {} services for one registry-detach target", services.len() diff --git a/operator/src/delete.rs b/operator/src/delete.rs index 99c7ca9eb..e10b690d6 100644 --- a/operator/src/delete.rs +++ b/operator/src/delete.rs @@ -492,8 +492,10 @@ fn record_delete_result( /// An absent ECS service or ambiguous `DescribeServices` response is rejected /// unless that matching durable checkpoint already exists. Cleanup uses only IDs /// from the checkpoint and removes the checkpoint last, making partial failures -/// safe to retry. The legacy CLI uses best-effort warnings for dependent/S3 -/// cleanup, while this programmatic API keeps those failures fatal. +/// safe to retry. The legacy CLI warns on a drain timeout, continues dependent/S3 +/// cleanup, retains the checkpoint for retry, and continues after individual S3 +/// object failures, while this programmatic API keeps exact-identity dependent +/// and S3 cleanup failures fatal. pub async fn delete_services( aws_config: &aws_config::SdkConfig, targets: &[DeleteTarget], @@ -899,10 +901,11 @@ async fn delete_checkpointed_ecs( ecs: &aws_sdk_ecs::Client, checkpoint: &DeleteCheckpoint, state: ServiceIdentityState, -) -> Result<()> { + cleanup_mode: CleanupMode, +) -> Result { if state == ServiceIdentityState::RetryGone { println!(" ✓ ECS service already absent; resuming exact dependent cleanup"); - return Ok(()); + return Ok(false); } let response = ecs @@ -1018,44 +1021,93 @@ async fn delete_checkpointed_ecs( } }; match drain_poll_action(complete, attempt, DRAIN_POLL_ATTEMPTS) { - DrainPollAction::Complete => { eprintln!(" done"); return Ok(()); } + DrainPollAction::Complete => { + eprintln!(" done"); + return Ok(false); + } DrainPollAction::Retry => { eprint!("."); tokio::time::sleep(DRAIN_POLL_INTERVAL).await; } - DrainPollAction::TimedOut => anyhow::bail!( - "checkpointed ECS service did not reach an unambiguous absent state; dependent cleanup was not started (safe to retry)" - ), + DrainPollAction::TimedOut => { + let message = + "checkpointed ECS service did not reach an unambiguous absent state"; + match cleanup_mode { + CleanupMode::Strict => anyhow::bail!( + "{message}; dependent cleanup was not started (safe to retry)" + ), + CleanupMode::BestEffort => { + eprintln!( + "\n ⚠ {message}; continuing dependent cleanup and retaining checkpoint" + ); + return Ok(true); + } + } + } } } - unreachable!() + anyhow::bail!("ECS drain polling ended without a terminal result") } async fn cleanup_s3( s3: &aws_sdk_s3::Client, bucket: &str, namespace: &str, name: &str, ) -> Result<()> { + let mut failures = Vec::new(); let manifest_key = crate::identity::manifest_key(namespace, name); - s3.delete_object().bucket(bucket).key(&manifest_key).send().await - .context("failed to delete control-plane manifest")?; - println!(" ✓ Manifest removed from S3"); + match s3 + .delete_object() + .bucket(bucket) + .key(&manifest_key) + .send() + .await + { + Ok(_) => println!(" ✓ Manifest removed from S3"), + Err(_) => failures.push("failed to delete control-plane manifest"), + } let artifact_prefix = format!("artifacts/{namespace}/{name}/"); let mut continuation_token = None; loop { - let response = s3.list_objects_v2().bucket(bucket).prefix(&artifact_prefix) - .set_continuation_token(continuation_token).send().await - .context("failed to list control-plane config artifacts")?; + let response = match s3 + .list_objects_v2() + .bucket(bucket) + .prefix(&artifact_prefix) + .set_continuation_token(continuation_token) + .send() + .await + { + Ok(response) => response, + Err(_) => { + failures.push("failed to list control-plane config artifacts"); + break; + } + }; for object in response.contents() { if let Some(key) = object.key() { - s3.delete_object().bucket(bucket).key(key).send().await - .context("failed to delete control-plane config artifact")?; + if s3 + .delete_object() + .bucket(bucket) + .key(key) + .send() + .await + .is_err() + { + failures.push("failed to delete control-plane config artifact"); + } } } continuation_token = response.next_continuation_token().map(str::to_owned); - if continuation_token.is_none() { break; } + if continuation_token.is_none() { + break; + } + } + + if failures.is_empty() { + println!(" ✓ Config artifacts removed from S3"); + Ok(()) + } else { + anyhow::bail!("control-plane S3 cleanup incomplete: {}", failures.join("; ")) } - println!(" ✓ Config artifacts removed from S3"); - Ok(()) } async fn run_with_bucket( @@ -1114,7 +1166,14 @@ async fn run_with_bucket( .to_string(), ); } - delete_checkpointed_ecs(&ecs, &checkpoint, state).await?; + let drain_incomplete = + delete_checkpointed_ecs(&ecs, &checkpoint, state, cleanup_mode).await?; + if drain_incomplete { + warnings.push( + "ECS drain timed out; dependent cleanup continued and delete checkpoint was retained" + .to_string(), + ); + } match cleanup_mode { CleanupMode::Strict => { @@ -1136,7 +1195,7 @@ async fn run_with_bucket( println!(" ✓ Delete checkpoint removed"); } CleanupMode::BestEffort => { - let mut cleanup_failed = false; + let mut cleanup_failed = drain_incomplete; if let Err(error) = crate::ingress::delete_exact( aws_config, namespace, diff --git a/operator/src/lib.rs b/operator/src/lib.rs index 2a3e641c9..de6877c53 100644 --- a/operator/src/lib.rs +++ b/operator/src/lib.rs @@ -72,9 +72,10 @@ //! The control-plane bucket is an infrastructure prerequisite: it must enforce //! default server-side encryption and S3 versioning for checkpoint recovery. //! The library writes application JSON but does not weaken or validate those -//! bucket-level policies on every request. The legacy CLI preserves best-effort -//! dependent/S3 cleanup warnings, while [`delete_services`] keeps exact-identity -//! and checkpoint cleanup failures fatal for retry safety. +//! bucket-level policies on every request. The legacy CLI warns on a drain +//! timeout, continues dependent/S3 cleanup, retains the checkpoint for retry, +//! and continues after individual S3 object failures, while [`delete_services`] +//! keeps exact-identity and checkpoint cleanup failures fatal for retry safety. //! //! Programmatic apply and delete do not provide an internal same-target lock. //! Callers must serialize mutations for the same AWS account, Region, From fe0f089dd7fbb0df8554824826e296fc0f96f1ef Mon Sep 17 00:00:00 2001 From: chaodu-agent <274062505+chaodu-agent@users.noreply.github.com> Date: Wed, 22 Jul 2026 21:55:35 +0000 Subject: [PATCH 15/20] fix(oabctl): harden identity and apply presence --- docs/oabctl.md | 26 +++-- operator/README.md | 18 +-- operator/src/apply.rs | 33 ++++-- operator/src/delete.rs | 1 + operator/src/identity.rs | 243 +++++++++++++++++++++++++++++---------- operator/src/lib.rs | 22 ++-- 6 files changed, 242 insertions(+), 101 deletions(-) diff --git a/docs/oabctl.md b/docs/oabctl.md index 6f5f7cfb5..25bdbddc7 100644 --- a/docs/oabctl.md +++ b/docs/oabctl.md @@ -394,17 +394,21 @@ must still be registered manually in the LINE Developers console. > parsed from a structurally and boundary-validated Cloud Map service ARN. > Exact-ID NotFound is idempotent; other cleanup errors retain the checkpoint. > There is no name-only API, Cloud Map, or S3 orphan cleanup path. Apply and -> delete share one injective identity rule: namespaces must not contain `-` -> (names may contain it), so the physical `oab-{namespace}-{name}` identity -> always maps back to exactly one `namespace`/`name` pair. Manifest validation -> and programmatic delete reject hyphenated namespaces before any AWS call, -> and before mutating anything both apply and delete verify in the control -> plane that no other recorded logical pair (for example legacy `prod-team/bot` -> versus `prod/team-bot`) claims the same physical name — contested identities -> fail closed. Legacy deployments created under a hyphenated namespace cannot -> be re-applied, but `oabctl delete` still accepts them (with a warning) under -> that same ownership check; migrate by deleting and re-creating them under a -> hyphen-free namespace. If a per-target +> delete share one injective identity rule: namespaces must match `[a-z0-9]+` +> and names must match `[a-z0-9][a-z0-9-]*`; this rejects `/`, whitespace, +> `_`, `.`, and Unicode before any ECS, API, or S3 request. Namespaces must not +> contain `-` on apply and programmatic delete (names may contain it), so the +> physical `oab-{namespace}-{name}` identity always maps back to exactly one +> `namespace`/`name` pair. The legacy CLI delete path permits hyphenated +> namespaces only when the remaining S3-safe character rules pass. Manifest +> validation and programmatic delete reject hyphenated namespaces before any +> AWS call, and before mutating anything both apply and delete verify in the +> control plane that no other recorded logical pair (for example legacy +> `prod-team/bot` versus `prod/team-bot`) claims the same physical name — +> contested identities fail closed. Legacy deployments created under a +> hyphenated namespace cannot be re-applied, but `oabctl delete` still accepts +> them (with a warning) under that same ownership check; migrate by deleting +> and re-creating them under a hyphen-free namespace. If a per-target > `ingress-teardown-checkpoints//.json` record exists, delete > fails before destructive mutation so it cannot discard unfinished exact > cleanup identity. Re-run the ingress-free apply to completion, then retry diff --git a/operator/README.md b/operator/README.md index 3dee4ac13..d7808a680 100644 --- a/operator/README.md +++ b/operator/README.md @@ -148,13 +148,17 @@ callers can retry safely. Apply and delete share one injective logical-identity rule: namespaces must not contain `-`; names may contain it, so the physical `oab-{namespace}-{name}` -identity always parses back to exactly one `namespace`/`name` pair. Manifest -validation (CLI apply, fleet expansion, and `apply_manifests`) and -`delete_services` all reject hyphenated namespaces before any AWS call. Before -mutating, apply and every delete entry point additionally verify in the -control plane that no *other* recorded logical pair (manifest, delete -checkpoint, or ingress-teardown checkpoint) claims the same physical name, and -fail closed with the colliding pair named in the error. +identity always parses back to exactly one `namespace`/`name` pair. Namespaces +must match `[a-z0-9]+` and names must match `[a-z0-9][a-z0-9-]*`; this also +prevents `/`, whitespace, `_`, `.`, and Unicode from becoming S3 path +components. Manifest validation (CLI apply, fleet expansion, and +`apply_manifests`) and `delete_services` all reject hyphenated namespaces before +any AWS call. The legacy CLI delete path permits hyphenated namespaces only +when the same S3-safe character rules otherwise pass. Before mutating, apply +and every delete entry point additionally verify in the control plane that no +*other* recorded logical pair (manifest, delete checkpoint, or +ingress-teardown checkpoint) claims the same physical name, and fail closed +with the colliding pair named in the error. Legacy policy: deployments created under a hyphenated namespace before this rule cannot be re-applied — apply fails with a migration message. They remain diff --git a/operator/src/apply.rs b/operator/src/apply.rs index 1b54e749e..b69f56d26 100644 --- a/operator/src/apply.rs +++ b/operator/src/apply.rs @@ -548,9 +548,14 @@ enum ApplyServicePresence { fn classify_apply_service_response( service_count: usize, failure_reasons: &[&str], + service_status: Option<&str>, ) -> Result { match (service_count, failure_reasons) { - (1, []) => Ok(ApplyServicePresence::Present), + (1, []) if service_status == Some("INACTIVE") => Ok(ApplyServicePresence::Absent), + (1, []) if matches!(service_status, Some("ACTIVE" | "DRAINING")) => { + Ok(ApplyServicePresence::Present) + } + (1, []) => anyhow::bail!("ECS returned a service with an unexpected status"), (0, [reason]) if reason.eq_ignore_ascii_case("MISSING") => { Ok(ApplyServicePresence::Absent) } @@ -893,6 +898,7 @@ async fn apply_ecs( let existing_service = match classify_apply_service_response( describe_resp.services().len(), &failure_reasons, + describe_resp.services().first().and_then(|service| service.status()), )? { ApplyServicePresence::Absent => None, ApplyServicePresence::Present => { @@ -908,9 +914,6 @@ async fn apply_ecs( .context("ECS apply response missing cluster ARN")?, &service_name, )?; - if !matches!(service.status(), Some("ACTIVE" | "DRAINING")) { - anyhow::bail!("ECS apply response has unexpected service status"); - } Some(service) } }; @@ -1911,21 +1914,31 @@ spec: #[test] fn apply_accepts_only_single_missing_failure_as_first_absence() { assert_eq!( - classify_apply_service_response(0, &["MISSING"]).unwrap(), + classify_apply_service_response(0, &["MISSING"], None).unwrap(), + ApplyServicePresence::Absent + ); + assert!(classify_apply_service_response(0, &[], None).is_err()); + assert!(classify_apply_service_response(0, &["MISSING", "ACCESS_DENIED"], None).is_err()); + assert!(classify_apply_service_response(1, &["MISSING"], Some("ACTIVE")).is_err()); + assert_eq!( + classify_apply_service_response(1, &[], Some("INACTIVE")).unwrap(), ApplyServicePresence::Absent ); - assert!(classify_apply_service_response(0, &[]).is_err()); - assert!(classify_apply_service_response(0, &["MISSING", "ACCESS_DENIED"]).is_err()); - assert!(classify_apply_service_response(1, &["MISSING"]).is_err()); } #[test] fn apply_requires_one_service_for_present_identity() { assert_eq!( - classify_apply_service_response(1, &[]).unwrap(), + classify_apply_service_response(1, &[], Some("ACTIVE")).unwrap(), + ApplyServicePresence::Present + ); + assert_eq!( + classify_apply_service_response(1, &[], Some("DRAINING")).unwrap(), ApplyServicePresence::Present ); - assert!(classify_apply_service_response(2, &[]).is_err()); + assert!(classify_apply_service_response(1, &[], None).is_err()); + assert!(classify_apply_service_response(1, &[], Some("UNKNOWN")).is_err()); + assert!(classify_apply_service_response(2, &[], None).is_err()); assert!(validate_apply_service_identity( "arn:aws:ecs:us-east-1:123456789012:service/cluster/oab-prod-bot", "arn:aws:ecs:us-east-1:123456789012:cluster/cluster", diff --git a/operator/src/delete.rs b/operator/src/delete.rs index e10b690d6..af4cdda15 100644 --- a/operator/src/delete.rs +++ b/operator/src/delete.rs @@ -1127,6 +1127,7 @@ async fn run_with_bucket( "delete requires a non-empty namespace and name (got '{namespace}'/'{name}')" ); } + crate::identity::validate_legacy_delete_identity(namespace, name)?; let ecs = aws_sdk_ecs::Client::new(aws_config); let s3 = aws_sdk_s3::Client::new(aws_config); if namespace.contains('-') { diff --git a/operator/src/identity.rs b/operator/src/identity.rs index 2a30cbe9f..698acc187 100644 --- a/operator/src/identity.rs +++ b/operator/src/identity.rs @@ -9,12 +9,14 @@ //! //! Two rules enforce injectivity across every entry point: //! -//! 1. **Domain rule** — [`validate_injective_identity`] rejects hyphenated -//! namespaces before any AWS call. It is enforced by +//! 1. **Domain rule** — namespaces use `[a-z0-9]+`, names use +//! `[a-z0-9][a-z0-9-]*`, and [`validate_injective_identity`] additionally +//! rejects hyphenated namespaces before any AWS call. It is enforced by //! [`crate::manifest::OABServiceManifest::validate`] (covering CLI apply, //! fleet expansion, and programmatic //! [`crate::apply::apply_manifests`]) and by programmatic -//! [`crate::delete::delete_services`]. +//! [`crate::delete::delete_services`]. The legacy CLI delete path permits +//! hyphens in namespaces only through [`validate_legacy_delete_identity`]. //! 2. **Ownership rule** — [`ensure_exclusive_physical_identity`] runs before //! any destructive mutation on both the apply and delete paths. It fails //! closed when the control plane records any *other* logical pair (for @@ -32,8 +34,7 @@ use anyhow::Context; use aws_sdk_s3::error::ProvideErrorMetadata; -use std::sync::Arc; -use tokio::sync::Semaphore; +use std::future::Future; use tokio::task::JoinSet; /// Physical service identity shared by ECS and Cloud Map: @@ -57,16 +58,11 @@ pub(crate) fn ingress_teardown_checkpoint_key(namespace: &str, name: &str) -> St format!("ingress-teardown-checkpoints/{namespace}/{name}.json") } -/// Domain rule: `namespace` and `name` must be non-empty and `namespace` must -/// not contain `-`, so that `oab-{namespace}-{name}` parses back to exactly -/// one logical pair. +/// Domain rule shared by apply and programmatic delete: namespaces use +/// `[a-z0-9]+`, names use `[a-z0-9][a-z0-9-]*`, and neither component may +/// contain path delimiters or other opaque characters. pub(crate) fn validate_injective_identity(namespace: &str, name: &str) -> anyhow::Result<()> { - if namespace.trim().is_empty() { - anyhow::bail!("namespace must not be empty (got '{namespace}'/'{name}')"); - } - if name.trim().is_empty() { - anyhow::bail!("name must not be empty (got '{namespace}'/'{name}')"); - } + validate_identity_charset(namespace, name, true)?; if namespace.contains('-') { anyhow::bail!( "namespace '{namespace}' must not contain '-': it is the delimiter of the physical \ @@ -81,6 +77,45 @@ pub(crate) fn validate_injective_identity(namespace: &str, name: &str) -> anyhow Ok(()) } +/// Legacy CLI delete accepts hyphenated namespaces, but still enforces the +/// same S3-safe character domain so a caller cannot create nested or ambiguous +/// control-plane keys. +pub(crate) fn validate_legacy_delete_identity(namespace: &str, name: &str) -> anyhow::Result<()> { + validate_identity_charset(namespace, name, true) +} + +fn validate_identity_charset( + namespace: &str, + name: &str, + allow_hyphenated_namespace: bool, +) -> anyhow::Result<()> { + let namespace_valid = |value: &str| { + !value.is_empty() + && value.chars().all(|ch| { + ch.is_ascii_lowercase() + || ch.is_ascii_digit() + || (allow_hyphenated_namespace && ch == '-') + }) + }; + let mut name_chars = name.chars(); + let name_valid = matches!( + name_chars.next(), + Some(ch) if ch.is_ascii_lowercase() || ch.is_ascii_digit() + ) && name_chars.all(|ch| ch.is_ascii_lowercase() || ch.is_ascii_digit() || ch == '-'); + if !namespace_valid(namespace) { + anyhow::bail!( + "namespace must match [a-z0-9]+{} (got '{namespace}')", + if allow_hyphenated_namespace { " with legacy '-' support" } else { "" }, + ); + } + if !name_valid { + anyhow::bail!( + "name must match [a-z0-9][a-z0-9-]* (got '{name}')" + ); + } + Ok(()) +} + /// Every logical pair distinct from `(namespace, name)` that maps to the same /// physical identity. These are the alternative split points of /// `{namespace}-{name}` and only exist when a component contains `-`. @@ -154,59 +189,36 @@ async fn probe_ownership_key( } } -/// Ownership rule: fail closed if the control plane records any logical pair, -/// distinct from `(namespace, name)`, that claims the same physical identity. -/// -/// Runs before any destructive mutation on the apply and delete paths. Probes -/// only the exact alias keys (stored manifest, delete checkpoint, and -/// ingress-teardown checkpoint), so hyphen-free pairs with hyphen-free names -/// cost zero requests. Alias probes are bounded to -/// [`OWNERSHIP_PROBE_CONCURRENCY`] in flight; probe failures other than a -/// missing key fail closed. -pub(crate) async fn ensure_exclusive_physical_identity( - s3: &aws_sdk_s3::Client, - bucket: &str, - namespace: &str, - name: &str, -) -> anyhow::Result<()> { - let semaphore = Arc::new(Semaphore::new(OWNERSHIP_PROBE_CONCURRENCY)); +async fn run_bounded_probes(items: Vec, probe: F) -> anyhow::Result<()> +where + T: Send + 'static, + F: Fn(T) -> Fut + Clone + Send + Sync + 'static, + Fut: Future> + Send + 'static, +{ + let mut pending = items.into_iter(); let mut probes = JoinSet::new(); + let mut in_flight = 0; - for (alias_namespace, alias_name) in collision_aliases(namespace, name) { - for key in ownership_keys(&alias_namespace, &alias_name) { - let permit = match semaphore.clone().acquire_owned().await { - Ok(permit) => permit, - Err(error) => { - probes.abort_all(); - while probes.join_next().await.is_some() {} - return Err(anyhow::anyhow!( - "ownership probe concurrency gate closed: {error}" - )); - } + loop { + while in_flight < OWNERSHIP_PROBE_CONCURRENCY { + let Some(item) = pending.next() else { + break; }; - let probe_s3 = s3.clone(); - let probe_bucket = bucket.to_owned(); - let probe_namespace = namespace.to_owned(); - let probe_name = name.to_owned(); - let probe_alias_namespace = alias_namespace; - let probe_alias_name = alias_name; - probes.spawn(async move { - let _permit = permit; - probe_ownership_key( - probe_s3, - probe_bucket, - probe_namespace, - probe_name, - probe_alias_namespace, - probe_alias_name, - key, - ) - .await - }); + let probe = probe.clone(); + probes.spawn(async move { probe(item).await }); + in_flight += 1; + } + + if in_flight == 0 { + return Ok(()); } - } - while let Some(result) = probes.join_next().await { + let Some(result) = probes.join_next().await else { + return Err(anyhow::anyhow!( + "ownership probe scheduler lost an in-flight task" + )); + }; + in_flight -= 1; match result { Ok(Ok(())) => {} Ok(Err(error)) => { @@ -221,12 +233,68 @@ pub(crate) async fn ensure_exclusive_physical_identity( } } } - Ok(()) +} + +/// Ownership rule: fail closed if the control plane records any logical pair, +/// distinct from `(namespace, name)`, that claims the same physical identity. +/// +/// Runs before any destructive mutation on the apply and delete paths. Probes +/// only the exact alias keys (stored manifest, delete checkpoint, and +/// ingress-teardown checkpoint), so hyphen-free pairs with hyphen-free names +/// cost zero requests. Alias probes are bounded to +/// [`OWNERSHIP_PROBE_CONCURRENCY`] in flight; a completed successful probe is +/// required before another probe is admitted, and the first failure aborts all +/// remaining work. +pub(crate) async fn ensure_exclusive_physical_identity( + s3: &aws_sdk_s3::Client, + bucket: &str, + namespace: &str, + name: &str, +) -> anyhow::Result<()> { + let mut items = Vec::new(); + for (alias_namespace, alias_name) in collision_aliases(namespace, name) { + for key in ownership_keys(&alias_namespace, &alias_name) { + items.push(( + alias_namespace.clone(), + alias_name.clone(), + key, + )); + } + } + + let probe_s3 = s3.clone(); + let probe_bucket = bucket.to_owned(); + let probe_namespace = namespace.to_owned(); + let probe_name = name.to_owned(); + run_bounded_probes(items, move |(alias_namespace, alias_name, key)| { + let probe_s3 = probe_s3.clone(); + let probe_bucket = probe_bucket.clone(); + let probe_namespace = probe_namespace.clone(); + let probe_name = probe_name.clone(); + async move { + probe_ownership_key( + probe_s3, + probe_bucket, + probe_namespace, + probe_name, + alias_namespace, + alias_name, + key, + ) + .await + } + }) + .await } #[cfg(test)] mod tests { use super::*; + use std::sync::{ + atomic::{AtomicUsize, Ordering}, + Arc, + }; + use tokio::sync::Notify; #[test] fn accepts_hyphen_free_namespace_with_hyphenated_name() { @@ -239,6 +307,26 @@ mod tests { assert!(validate_injective_identity("prod", " ").is_err()); } + #[test] + fn rejects_non_s3_safe_identity_components() { + for (namespace, name) in [ + ("prod/team", "bot"), + ("prod", "bot/extra"), + ("prod", "../bot"), + ("prod", " bot"), + ("prod", "bot_1"), + ("prod", "ボット"), + ("prod", "-bot"), + ] { + assert!( + validate_injective_identity(namespace, name).is_err(), + "identity should be rejected: {namespace}/{name}" + ); + } + validate_legacy_delete_identity("prod-team", "bot").expect("legacy hyphenated namespace"); + assert!(validate_legacy_delete_identity("prod/team", "bot").is_err()); + } + #[test] fn rejects_hyphenated_namespace_with_migration_hint() { let error = validate_injective_identity("prod-team", "bot").unwrap_err(); @@ -284,6 +372,35 @@ mod tests { assert!(OWNERSHIP_PROBE_CONCURRENCY < 300); } + #[tokio::test] + async fn bounded_probe_scheduler_aborts_before_admitting_replacements() { + let started = Arc::new(AtomicUsize::new(0)); + let blocker = Arc::new(Notify::new()); + let result = run_bounded_probes((0..(OWNERSHIP_PROBE_CONCURRENCY + 4)).collect(), { + let started = started.clone(); + let blocker = blocker.clone(); + move |item| { + let started = started.clone(); + let blocker = blocker.clone(); + async move { + started.fetch_add(1, Ordering::SeqCst); + if item == 0 { + anyhow::bail!("deterministic probe failure"); + } + blocker.notified().await; + Ok(()) + } + } + }) + .await; + + assert!(result.is_err()); + assert!( + started.load(Ordering::SeqCst) <= OWNERSHIP_PROBE_CONCURRENCY, + "replacement probes were admitted after the first failure" + ); + } + #[test] fn collision_aliases_are_symmetric_across_the_colliding_pair() { // The new-domain pair sees the legacy pair… diff --git a/operator/src/lib.rs b/operator/src/lib.rs index de6877c53..d934fe06f 100644 --- a/operator/src/lib.rs +++ b/operator/src/lib.rs @@ -48,16 +48,18 @@ //! otherwise both use the shared `OAB_CONTROL_PLANE_BUCKET` then caller-account //! resolution chain. //! -//! Apply and delete share one injective logical-identity rule: namespaces -//! must not contain `-`; names may contain it, so the physical -//! `oab-{namespace}-{name}` identity parses back to exactly one accepted -//! `namespace`/`name` pair. Manifest validation (all apply entry points) and -//! [`delete_services`] both reject hyphenated namespaces before any AWS call. -//! Before mutating, apply and delete additionally verify in the control plane -//! that no other recorded logical pair (for example a legacy hyphenated -//! namespace) claims the same physical name, and fail closed otherwise. -//! Legacy hyphenated-namespace deployments cannot be re-applied but remain -//! deletable via the `oabctl delete` CLI under that same ownership check. +//! Apply and delete share one injective logical-identity rule: namespaces must +//! match `[a-z0-9]+` and names must match `[a-z0-9][a-z0-9-]*`; this rejects +//! path delimiters and opaque characters before any AWS call. Namespaces must +//! not contain `-` on apply and programmatic delete; names may contain it, so +//! the physical `oab-{namespace}-{name}` identity parses back to exactly one +//! accepted `namespace`/`name` pair. Manifest validation (all apply entry +//! points) and [`delete_services`] both reject hyphenated namespaces before any +//! AWS call, while the legacy CLI delete path permits them only under the same +//! S3-safe character rules. Before mutating, apply and delete additionally +//! verify in the control plane that no other recorded logical pair (for example +//! a legacy hyphenated namespace) claims the same physical name, and fail +//! closed otherwise. //! //! [`delete_services`] tears down by `namespace`+`name` ([`DeleteTarget`]). //! Before ECS mutation delete durably records the resolved bucket, From 8873ea20e0ae6fcef8ac4a372c6ce60dfe7b47ca Mon Sep 17 00:00:00 2001 From: chaodu-agent <274062505+chaodu-agent@users.noreply.github.com> Date: Wed, 22 Jul 2026 21:59:04 +0000 Subject: [PATCH 16/20] fix(oabctl): relax probe scheduler bounds --- operator/src/identity.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/operator/src/identity.rs b/operator/src/identity.rs index 698acc187..f0ccf3a53 100644 --- a/operator/src/identity.rs +++ b/operator/src/identity.rs @@ -192,7 +192,7 @@ async fn probe_ownership_key( async fn run_bounded_probes(items: Vec, probe: F) -> anyhow::Result<()> where T: Send + 'static, - F: Fn(T) -> Fut + Clone + Send + Sync + 'static, + F: Fn(T) -> Fut + Clone + Send + 'static, Fut: Future> + Send + 'static, { let mut pending = items.into_iter(); From 3799b053cd28845a48cd3daff4d2904abc4ffc2d Mon Sep 17 00:00:00 2001 From: chaodu-agent <274062505+chaodu-agent@users.noreply.github.com> Date: Wed, 22 Jul 2026 22:01:55 +0000 Subject: [PATCH 17/20] fix(oabctl): simplify ownership probe scheduler --- operator/src/identity.rs | 147 ++++++++++++++++++++++----------------- 1 file changed, 83 insertions(+), 64 deletions(-) diff --git a/operator/src/identity.rs b/operator/src/identity.rs index f0ccf3a53..4b5c38f5c 100644 --- a/operator/src/identity.rs +++ b/operator/src/identity.rs @@ -34,7 +34,6 @@ use anyhow::Context; use aws_sdk_s3::error::ProvideErrorMetadata; -use std::future::Future; use tokio::task::JoinSet; /// Physical service identity shared by ECS and Cloud Map: @@ -189,23 +188,53 @@ async fn probe_ownership_key( } } -async fn run_bounded_probes(items: Vec, probe: F) -> anyhow::Result<()> -where - T: Send + 'static, - F: Fn(T) -> Fut + Clone + Send + 'static, - Fut: Future> + Send + 'static, -{ - let mut pending = items.into_iter(); +/// Ownership rule: fail closed if the control plane records any logical pair, +/// distinct from `(namespace, name)`, that claims the same physical identity. +/// +/// Runs before any destructive mutation on the apply and delete paths. Probes +/// only the exact alias keys (stored manifest, delete checkpoint, and +/// ingress-teardown checkpoint), so hyphen-free pairs with hyphen-free names +/// cost zero requests. Alias probes are bounded to +/// [`OWNERSHIP_PROBE_CONCURRENCY`] in flight; a completed successful probe is +/// required before another probe is admitted, and the first failure aborts all +/// remaining work. +pub(crate) async fn ensure_exclusive_physical_identity( + s3: &aws_sdk_s3::Client, + bucket: &str, + namespace: &str, + name: &str, +) -> anyhow::Result<()> { + let mut pending = collision_aliases(namespace, name) + .into_iter() + .flat_map(|(alias_namespace, alias_name)| { + ownership_keys(&alias_namespace, &alias_name) + .into_iter() + .map(move |key| (alias_namespace.clone(), alias_name.clone(), key)) + }); let mut probes = JoinSet::new(); let mut in_flight = 0; loop { while in_flight < OWNERSHIP_PROBE_CONCURRENCY { - let Some(item) = pending.next() else { + let Some((alias_namespace, alias_name, key)) = pending.next() else { break; }; - let probe = probe.clone(); - probes.spawn(async move { probe(item).await }); + let probe_s3 = s3.clone(); + let probe_bucket = bucket.to_owned(); + let probe_namespace = namespace.to_owned(); + let probe_name = name.to_owned(); + probes.spawn(async move { + probe_ownership_key( + probe_s3, + probe_bucket, + probe_namespace, + probe_name, + alias_namespace, + alias_name, + key, + ) + .await + }); in_flight += 1; } @@ -235,67 +264,57 @@ where } } -/// Ownership rule: fail closed if the control plane records any logical pair, -/// distinct from `(namespace, name)`, that claims the same physical identity. -/// -/// Runs before any destructive mutation on the apply and delete paths. Probes -/// only the exact alias keys (stored manifest, delete checkpoint, and -/// ingress-teardown checkpoint), so hyphen-free pairs with hyphen-free names -/// cost zero requests. Alias probes are bounded to -/// [`OWNERSHIP_PROBE_CONCURRENCY`] in flight; a completed successful probe is -/// required before another probe is admitted, and the first failure aborts all -/// remaining work. -pub(crate) async fn ensure_exclusive_physical_identity( - s3: &aws_sdk_s3::Client, - bucket: &str, - namespace: &str, - name: &str, -) -> anyhow::Result<()> { - let mut items = Vec::new(); - for (alias_namespace, alias_name) in collision_aliases(namespace, name) { - for key in ownership_keys(&alias_namespace, &alias_name) { - items.push(( - alias_namespace.clone(), - alias_name.clone(), - key, - )); - } - } - - let probe_s3 = s3.clone(); - let probe_bucket = bucket.to_owned(); - let probe_namespace = namespace.to_owned(); - let probe_name = name.to_owned(); - run_bounded_probes(items, move |(alias_namespace, alias_name, key)| { - let probe_s3 = probe_s3.clone(); - let probe_bucket = probe_bucket.clone(); - let probe_namespace = probe_namespace.clone(); - let probe_name = probe_name.clone(); - async move { - probe_ownership_key( - probe_s3, - probe_bucket, - probe_namespace, - probe_name, - alias_namespace, - alias_name, - key, - ) - .await - } - }) - .await -} - #[cfg(test)] mod tests { use super::*; + use std::future::Future; use std::sync::{ atomic::{AtomicUsize, Ordering}, Arc, }; use tokio::sync::Notify; + async fn run_test_bounded_probes(items: Vec, probe: F) -> anyhow::Result<()> + where + T: Send + 'static, + F: Fn(T) -> Fut + Clone + Send + 'static, + Fut: Future> + Send + 'static, + { + let mut pending = items.into_iter(); + let mut probes = JoinSet::new(); + let mut in_flight = 0; + loop { + while in_flight < OWNERSHIP_PROBE_CONCURRENCY { + let Some(item) = pending.next() else { + break; + }; + let probe = probe.clone(); + probes.spawn(async move { probe(item).await }); + in_flight += 1; + } + if in_flight == 0 { + return Ok(()); + } + let Some(result) = probes.join_next().await else { + return Err(anyhow::anyhow!("test scheduler lost an in-flight task")); + }; + in_flight -= 1; + match result { + Ok(Ok(())) => {} + Ok(Err(error)) => { + probes.abort_all(); + while probes.join_next().await.is_some() {} + return Err(error); + } + Err(error) => { + probes.abort_all(); + while probes.join_next().await.is_some() {} + return Err(anyhow::anyhow!("test probe task failed: {error}")); + } + } + } + } + #[test] fn accepts_hyphen_free_namespace_with_hyphenated_name() { validate_injective_identity("prod", "nest-my-oab").expect("valid identity"); @@ -376,7 +395,7 @@ mod tests { async fn bounded_probe_scheduler_aborts_before_admitting_replacements() { let started = Arc::new(AtomicUsize::new(0)); let blocker = Arc::new(Notify::new()); - let result = run_bounded_probes((0..(OWNERSHIP_PROBE_CONCURRENCY + 4)).collect(), { + let result = run_test_bounded_probes((0..(OWNERSHIP_PROBE_CONCURRENCY + 4)).collect(), { let started = started.clone(); let blocker = blocker.clone(); move |item| { From 17419c3178e1caef231666210129afccebd784ff Mon Sep 17 00:00:00 2001 From: chaodu-agent <274062505+chaodu-agent@users.noreply.github.com> Date: Wed, 22 Jul 2026 22:05:58 +0000 Subject: [PATCH 18/20] fix(oabctl): simplify probe admission queue --- operator/src/identity.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/operator/src/identity.rs b/operator/src/identity.rs index 4b5c38f5c..9c9b87a8c 100644 --- a/operator/src/identity.rs +++ b/operator/src/identity.rs @@ -204,13 +204,13 @@ pub(crate) async fn ensure_exclusive_physical_identity( namespace: &str, name: &str, ) -> anyhow::Result<()> { - let mut pending = collision_aliases(namespace, name) - .into_iter() - .flat_map(|(alias_namespace, alias_name)| { - ownership_keys(&alias_namespace, &alias_name) - .into_iter() - .map(move |key| (alias_namespace.clone(), alias_name.clone(), key)) - }); + let mut items = Vec::new(); + for (alias_namespace, alias_name) in collision_aliases(namespace, name) { + for key in ownership_keys(&alias_namespace, &alias_name) { + items.push((alias_namespace.clone(), alias_name.clone(), key)); + } + } + let mut pending = items.into_iter(); let mut probes = JoinSet::new(); let mut in_flight = 0; From e01bbadec312525e6c88b8b3862ddd7f429f73ef Mon Sep 17 00:00:00 2001 From: chaodu-agent <274062505+chaodu-agent@users.noreply.github.com> Date: Wed, 22 Jul 2026 22:09:16 +0000 Subject: [PATCH 19/20] fix(oabctl): type ownership probe tasks explicitly --- operator/src/identity.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/operator/src/identity.rs b/operator/src/identity.rs index 9c9b87a8c..71396559e 100644 --- a/operator/src/identity.rs +++ b/operator/src/identity.rs @@ -211,7 +211,7 @@ pub(crate) async fn ensure_exclusive_physical_identity( } } let mut pending = items.into_iter(); - let mut probes = JoinSet::new(); + let mut probes: JoinSet> = JoinSet::new(); let mut in_flight = 0; loop { @@ -281,7 +281,7 @@ mod tests { Fut: Future> + Send + 'static, { let mut pending = items.into_iter(); - let mut probes = JoinSet::new(); + let mut probes: JoinSet> = JoinSet::new(); let mut in_flight = 0; loop { while in_flight < OWNERSHIP_PROBE_CONCURRENCY { From 18322e16f2b63e5484b800507fe23451bbac1751 Mon Sep 17 00:00:00 2001 From: chaodu-agent <274062505+chaodu-agent@users.noreply.github.com> Date: Wed, 22 Jul 2026 22:14:53 +0000 Subject: [PATCH 20/20] fix(oabctl): resume draining ECS deletes --- docs/oabctl.md | 3 +++ operator/README.md | 4 ++++ operator/src/apply.rs | 6 ++++++ operator/src/delete.rs | 19 +++++++++++++++++-- operator/src/ingress.rs | 11 +++++++++-- operator/src/lib.rs | 5 +++++ 6 files changed, 44 insertions(+), 4 deletions(-) diff --git a/docs/oabctl.md b/docs/oabctl.md index 25bdbddc7..982667a85 100644 --- a/docs/oabctl.md +++ b/docs/oabctl.md @@ -476,6 +476,9 @@ spec: ### Design Principles - **Manifest = infra desired state** — image, CPU, networking +- **Apply failure recovery** — apply persists the manifest before ECS mutation; + if a later AWS/configuration step fails, correct the cause and re-run apply to + reconcile that persisted desired state. Apply has no rollback checkpoint. - **Agent config is external** — `configFrom` points to config.toml (managed via `--sync`) - **Secrets resolved by OpenAB** — `[secrets.refs]` in config.toml, not in manifest - **Runtime-agnostic spec** — same top-level fields regardless of ECS or K8S diff --git a/operator/README.md b/operator/README.md index d7808a680..5ea71af7b 100644 --- a/operator/README.md +++ b/operator/README.md @@ -138,6 +138,10 @@ record exists, delete fails before destructive mutation so it cannot discard unfinished exact cleanup identity. Re-run the ingress-free apply to completion, then retry delete. +The manifest is persisted as the infrastructure desired state before ECS +mutation. If a later AWS/configuration step fails, correct the underlying issue +and re-run `apply`/`apply_manifests` to reconcile the persisted desired state; +apply has no rollback checkpoint. The control-plane bucket must enforce default server-side encryption and S3 versioning for durable checkpoint recovery. The library writes application JSON but does not weaken or validate those bucket-level policies per request. The diff --git a/operator/src/apply.rs b/operator/src/apply.rs index b69f56d26..3585dc049 100644 --- a/operator/src/apply.rs +++ b/operator/src/apply.rs @@ -408,6 +408,12 @@ pub(crate) async fn run( /// pair (for example a legacy hyphenated namespace) claims the same physical /// identity, and fails closed if one does. /// +/// If reconciliation fails after the desired-state manifest is persisted, the +/// manifest remains the intended state and there is no apply checkpoint to +/// roll back. Callers should correct the underlying AWS/configuration issue and +/// re-run `apply_manifests` (or the CLI `apply`) to reconcile the persisted +/// desired state before resuming other mutations. +/// /// # Concurrency /// /// This function is not serialized with [`crate::delete::delete_services`]. diff --git a/operator/src/delete.rs b/operator/src/delete.rs index af4cdda15..c49052eca 100644 --- a/operator/src/delete.rs +++ b/operator/src/delete.rs @@ -928,7 +928,7 @@ async fn delete_checkpointed_ecs( ) .map_err(anyhow::Error::msg)?; if state == ServiceIdentityState::RetryGone { - return Ok(()); + return Ok(false); } let service = service.context("checkpointed ECS service disappeared ambiguously")?; let delete_phase = ecs_delete_phase(service.status())?; @@ -984,8 +984,23 @@ async fn delete_checkpointed_ecs( } Err(error) => return Err(error).context("failed to scale ECS service to zero"), } + } else if delete_phase == EcsDeletePhase::Drain { + match ecs + .delete_service() + .cluster(&checkpoint.cluster_arn) + .service(&checkpoint.service_arn) + .force(true) + .send() + .await + { + Ok(_) => println!(" ✓ ECS draining service deletion resumed"), + Err(error) if error.code() == Some("ServiceNotFoundException") => { + println!(" ✓ ECS draining service disappeared while resuming delete") + } + Err(error) => return Err(error).context("failed to resume ECS service deletion"), + } } else { - println!(" ✓ ECS service is already draining; resuming delete cleanup"); + println!(" ✓ ECS service is already inactive; resuming delete cleanup"); } const DRAIN_POLL_ATTEMPTS: u32 = 12; diff --git a/operator/src/ingress.rs b/operator/src/ingress.rs index 086423578..eff471198 100644 --- a/operator/src/ingress.rs +++ b/operator/src/ingress.rs @@ -417,9 +417,16 @@ async fn delete_cloud_map_by_arn( ) .context("Cloud Map service ARN is outside the exact delete boundary")?; let discovery = aws_sdk_servicediscovery::Client::new(config); + const DELETE_ATTEMPTS: u32 = 12; + const BASE_RETRY_DELAY_SECS: u64 = 5; + const MAX_RETRY_DELAY_SECS: u64 = 30; let mut last_error = None; - for attempt in 0..6 { - if attempt > 0 { tokio::time::sleep(std::time::Duration::from_secs(5)).await; } + for attempt in 0..DELETE_ATTEMPTS { + if attempt > 0 { + let exponent = u32::min(attempt - 1, 3); + let delay_secs = (BASE_RETRY_DELAY_SECS << exponent).min(MAX_RETRY_DELAY_SECS); + tokio::time::sleep(std::time::Duration::from_secs(delay_secs)).await; + } match discovery.delete_service().id(&service_id).send().await { Ok(_) => { eprintln!(" ✓ Deleted Cloud Map service: {service_name} ({service_id})"); diff --git a/operator/src/lib.rs b/operator/src/lib.rs index d934fe06f..f2b8d9764 100644 --- a/operator/src/lib.rs +++ b/operator/src/lib.rs @@ -61,6 +61,11 @@ //! a legacy hyphenated namespace) claims the same physical name, and fail //! closed otherwise. //! +//! Apply persists the manifest as desired state before ECS mutation. If a later +//! AWS or configuration step fails, callers must correct the cause and re-run +//! apply to reconcile that persisted desired state; apply has no rollback +//! checkpoint. +//! //! [`delete_services`] tears down by `namespace`+`name` ([`DeleteTarget`]). //! Before ECS mutation delete durably records the resolved bucket, //! caller partition/account/region, canonical cluster/service ARNs, the ECS