From 160714948f971a1d64b614a9dc3ffc7f4a5ce7ce Mon Sep 17 00:00:00 2001 From: Jason Song Date: Sun, 30 Aug 2026 19:25:47 +0800 Subject: [PATCH 1/3] feat: harden config diff and apply semantics --- README.md | 21 ++ README.zh.md | 8 + src/cli.rs | 10 +- src/command.rs | 448 +++++++++++++++++++++++++++--- src/error.rs | 25 ++ src/http.rs | 25 +- src/mutation.rs | 39 ++- tests/cli.rs | 21 ++ tests/openapi.rs | 690 ++++++++++++++++++++++++++++++++++++++++++++++- 9 files changed, 1234 insertions(+), 53 deletions(-) diff --git a/README.md b/README.md index c24805b..b0d6300 100644 --- a/README.md +++ b/README.md @@ -326,6 +326,27 @@ With `--yes`, table mode still writes the plan before the request. A successful one valid JSON document and preserves the existing top-level `status` and `data` fields while adding the top-level `operation` plan. +### Config synchronization contract + +`config diff` and `config apply` use a conservative merge contract. Source-only keys are created, +keys whose source value or comment differs are updated, matching keys are unchanged, and target-only +keys are preserved. An empty source is therefore a successful no-op, not a request to empty the +target. The CLI does not currently provide `--prune`; deletion requires a separate explicit config +deletion workflow. If a Portal version reports delete operations from the synchronize diff endpoint, +the CLI rejects the plan and does not call `items/synchronize`. + +Both table and JSON output report the source and target scopes plus `create`, `update`, `delete`, and +`unchanged` counts. The JSON contract also reports `strategy: "merge"` and +`targetOnlyBehavior: "preserve"`. It never includes config values in the diff result, apply plan, or +apply result. + +A standalone `config diff` is advisory; it does not create a plan artifact for a later invocation. +`config apply` captures its own fully paginated source snapshot, assesses that exact snapshot through +`items/diff`, and builds the detailed mutation plan from the returned change set. After approval, it +repeats the assessment with the same captured source snapshot. If the target assessment changed, the +command returns `stale_plan` and sends no synchronize request. If all counts are zero, it returns the +deterministic `data.result: "no-op"` success response without calling `items/synchronize`. + ## OpenAPI behavior The first v0 implementation uses a small generic HTTP client instead of a generated SDK. This keeps diff --git a/README.zh.md b/README.zh.md index 13e49c4..792b28f 100644 --- a/README.zh.md +++ b/README.zh.md @@ -277,6 +277,14 @@ namespace 创建只会在首次批准后发送只读预检请求。如果 Apollo 传入 `--yes` 时，table 模式仍会在请求前输出计划。成功的 JSON 输出仍是一个完整 JSON 文档，并保留现有顶层 `status` 和 `data` 字段，同时新增顶层 `operation` 计划。 +### 配置同步约定 + +`config diff` 和 `config apply` 采用保守合并约定：只存在于源端的 key 会被创建，源端 value 或 comment 不同的 key 会被更新，相同 key 保持不变，只存在于目标端的 key 会被保留。因此，空源端会得到成功的 no-op，而不会清空目标端。CLI 当前不提供 `--prune`；删除必须通过独立、显式的配置删除流程完成。如果某个 Portal 版本通过同步 diff 接口返回删除操作，CLI 会拒绝该计划，并且不会调用 `items/synchronize`。 + +table 和 JSON 输出都会给出源端与目标端 scope，以及 `create`、`update`、`delete`、`unchanged` 计数。JSON 约定还会返回 `strategy: "merge"` 和 `targetOnlyBehavior: "preserve"`。diff 结果、apply 计划和 apply 结果都不会包含配置 value。 + +单独执行的 `config diff` 仅供参考，不会生成可供后续调用消费的计划制品。`config apply` 会自行捕获完整分页的源端快照，用这份快照调用 `items/diff`，并根据返回的变更集生成详细变更计划。批准后，CLI 会使用同一份已捕获的源端快照再次评估目标端；如果评估结果发生变化，命令会返回 `stale_plan`，且不会发送同步请求。如果所有变更计数均为零，命令会返回确定性的 `data.result: "no-op"` 成功结果，并且不会调用 `items/synchronize`。 + ## OpenAPI 行为 第一版 v0 实现使用一个小型通用 HTTP client，而不是生成式 SDK。这样可以让 CLI 与 Apollo 服务端仓库解耦，同时仍然保证所有内置资源命令都限定在 `/openapi/v1/*`。 diff --git a/src/cli.rs b/src/cli.rs index f1bbbc5..bb9fbf6 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -328,9 +328,10 @@ pub enum ConfigCommand { #[arg(long, help = "Operator for consumer-token mode")] operator: Option, }, - /// Compare a namespace against another target. + /// Preview a conservative namespace merge into another target. #[command( - override_usage = "apollo config diff [OPTIONS] --env --app [--cluster ] [--namespace ] --target-env [--target-cluster ] [--target-namespace ]" + override_usage = "apollo config diff [OPTIONS] --env --app [--cluster ] [--namespace ] --target-env [--target-cluster ] [--target-namespace ]", + after_help = "Conservative merge preserves target-only keys. This preview is advisory; config apply captures and verifies its own snapshot." )] Diff { #[command(flatten)] @@ -342,9 +343,10 @@ pub enum ConfigCommand { #[arg(long, help = "Target namespace, defaults to source namespace")] target_namespace: Option, }, - /// Sync namespace configuration items to another target. + /// Conservatively merge namespace items into another target. #[command( - override_usage = "apollo config apply [OPTIONS] --env --app [--cluster ] [--namespace ] --target-env [--target-cluster ] [--target-namespace ]" + override_usage = "apollo config apply [OPTIONS] --env --app [--cluster ] [--namespace ] --target-env [--target-cluster ] [--target-namespace ]", + after_help = "Creates and updates source keys, preserves target-only keys, and sends no mutation when the assessed change set is empty. Deletion requires a separate explicit config delete." )] Apply { #[command(flatten)] diff --git a/src/command.rs b/src/command.rs index e41e8f9..78158d2 100644 --- a/src/command.rs +++ b/src/command.rs @@ -17,7 +17,7 @@ use crate::config::{ use crate::credential; use crate::error::CliError; use crate::http::{OpenApiClient, OpenApiResponse, append_query, encode_path_segment}; -use crate::mutation::{MutationPlan, MutationScope}; +use crate::mutation::{MutationChangeCounts, MutationPlan, MutationScope}; use crate::output::{OutputWriter, RenderedOutput}; use crate::redaction::{Redactor, Sensitive}; @@ -1164,19 +1164,27 @@ fn execute_config( let sync_items = source_sync_items(&openapi, &scope)?; let body = sync_body( &scope, - target_env, - target_cluster, - target_namespace, - sync_items, + &target_env, + &target_cluster, + target_namespace.as_deref(), + &sync_items, ); - let path = format!("{}/items/diff", namespace_path(&scope)); - let response = openapi.client.request("POST", &path, Some(body))?; - let data = redact_config_item_values(response.data.clone()); - Ok(render_openapi_response_with_data( - &openapi.writer, - &response, - data, - )) + let target = sync_target_scope( + &scope, + &target_env, + &target_cluster, + target_namespace.as_deref(), + ); + let assessment = assess_config_sync(&openapi, &scope, &target, &body)?; + let response = ConfigSyncResponse::new( + "preview", + namespace_mutation_scope(&scope), + namespace_mutation_scope(&target), + assessment.changes, + ); + Ok(openapi + .writer + .render_success(&response, response.render_table())) } ConfigCommand::Apply { scope, @@ -1185,10 +1193,7 @@ fn execute_config( target_namespace, operator, } => { - let mutation_plan = &mutation_plan - .as_ref() - .expect("config apply mutation plan") - .plan; + let initial_mutation = mutation_plan.as_ref().expect("config apply mutation plan"); let operator = operator_for_mutation( operator.as_deref(), &openapi.context, @@ -1205,17 +1210,68 @@ fn execute_config( let sync_items = source_sync_items(&openapi, &scope)?; let body = sync_body( &scope, - target_env, - target_cluster, - target_namespace, - sync_items, + &target_env, + &target_cluster, + target_namespace.as_deref(), + &sync_items, + ); + let target = sync_target_scope( + &scope, + &target_env, + &target_cluster, + target_namespace.as_deref(), ); + let assessment = assess_config_sync(&openapi, &scope, &target, &body)?; + let detailed_plan = initial_mutation + .plan + .clone() + .with_config_sync(assessment.changes.clone()); + + if assessment.is_noop() { + let response = ConfigSyncResponse::new( + "no-op", + namespace_mutation_scope(&scope), + namespace_mutation_scope(&target), + assessment.changes, + ); + return Ok(openapi.writer.render_mutation_success( + &detailed_plan, + &response, + response.render_table(), + )); + } + + let approved = prepare_mutation_with_openapi_context(cli, &openapi, detailed_plan)?; + let current_assessment = request_config_diff(&openapi, &scope, &target, &body)?; + if current_assessment != assessment { + return Err(CliError::stale_plan( + "The target configuration changed after the apply plan was approved; no synchronize request was sent. Re-run the command to review a fresh plan.", + approved.plan, + openapi.context.output, + )); + } + let path = append_optional_query( format!("{}/items/synchronize", namespace_path(&scope)), "operator", operator.as_deref(), ); - openapi.mutation_request(mutation_plan, "POST", &path, Some(body)) + let synchronize_response = + openapi + .client + .request_with_redacted_error_body("POST", &path, Some(body))?; + let response = ConfigSyncResponse::with_status( + synchronize_response.status, + "applied", + namespace_mutation_scope(&scope), + namespace_mutation_scope(&target), + assessment.changes, + ); + Ok(openapi.writer.render_mutation_success( + &approved.plan, + &response, + response.render_table(), + )) } } } @@ -2255,59 +2311,307 @@ fn source_sync_items( let mut items = Vec::new(); let mut page = DEFAULT_PAGE; + let mut expected_total = None; loop { let mut path = format!("{}/items", namespace_path(scope)); path = append_query(path, "page", &page.to_string()); path = append_query(path, "size", &SYNC_ITEMS_PAGE_SIZE.to_string()); - let response = openapi.client.request("GET", &path, None)?; - let content = item_page_content(&response.data, openapi.context.output)?; + let response = openapi + .client + .request_with_redacted_error_body("GET", &path, None)?; + let (content, total) = validated_item_page( + &response.data, + page, + SYNC_ITEMS_PAGE_SIZE, + openapi.context.output, + )?; + if let Some(expected_total) = expected_total + && expected_total != total + { + return Err(CliError::invalid_input( + "OpenAPI item pagination total changed while the source snapshot was being read", + openapi.context.output, + )); + } + expected_total = Some(total); let content_len = content.len(); items.extend(content); - let total = response.data.get("total").and_then(Value::as_u64); - if total.is_some_and(|total| items.len() as u64 >= total) - || content_len < SYNC_ITEMS_PAGE_SIZE as usize - { + if items.len() as u64 > total { + return Err(CliError::invalid_input( + "OpenAPI item pagination returned more source items than its declared total", + openapi.context.output, + )); + } + if items.len() as u64 == total { break; } - page += 1; + if content_len != SYNC_ITEMS_PAGE_SIZE as usize { + return Err(CliError::invalid_input( + "OpenAPI item pagination returned a partial source page before the declared total was reached", + openapi.context.output, + )); + } + page = page.checked_add(1).ok_or_else(|| { + CliError::invalid_input( + "OpenAPI item pagination exceeded the supported page range", + openapi.context.output, + ) + })?; } Ok(items) } -fn item_page_content(data: &Value, output: OutputFormat) -> Result, CliError> { - data.get("content") +fn validated_item_page( + data: &Value, + expected_page: u32, + expected_size: u32, + output: OutputFormat, +) -> Result<(Vec, u64), CliError> { + let content = data + .get("content") .and_then(Value::as_array) - .or_else(|| data.as_array()) .cloned() .ok_or_else(|| { CliError::invalid_input( "OpenAPI item list response did not contain a content array", output, ) - }) + })?; + let page = data.get("page").and_then(Value::as_u64).ok_or_else(|| { + CliError::invalid_input( + "OpenAPI item list response did not contain a valid page number", + output, + ) + })?; + let size = data.get("size").and_then(Value::as_u64).ok_or_else(|| { + CliError::invalid_input( + "OpenAPI item list response did not contain a valid page size", + output, + ) + })?; + let total = data.get("total").and_then(Value::as_u64).ok_or_else(|| { + CliError::invalid_input( + "OpenAPI item list response did not contain a valid total", + output, + ) + })?; + + if page != u64::from(expected_page) || size != u64::from(expected_size) { + return Err(CliError::invalid_input( + "OpenAPI item list pagination metadata did not match the requested source page", + output, + )); + } + if content.len() > expected_size as usize { + return Err(CliError::invalid_input( + "OpenAPI item list response exceeded the requested source page size", + output, + )); + } + + Ok((content, total)) } fn sync_body( scope: &NamespaceScopeArgs, - target_env: String, - target_cluster: String, - target_namespace: Option, - sync_items: Vec, + target_env: &str, + target_cluster: &str, + target_namespace: Option<&str>, + sync_items: &[Value], ) -> Value { json!({ "syncToNamespaces": [{ "appId": scope.cluster_scope.app.clone(), "env": target_env, "clusterName": target_cluster, - "namespaceName": target_namespace.unwrap_or_else(|| scope.namespace.clone()), + "namespaceName": target_namespace.unwrap_or(&scope.namespace), }], "syncItems": sync_items, }) } +fn sync_target_scope( + source: &NamespaceScopeArgs, + target_env: &str, + target_cluster: &str, + target_namespace: Option<&str>, +) -> NamespaceScopeArgs { + NamespaceScopeArgs { + cluster_scope: ClusterScopeArgs { + env: target_env.to_owned(), + app: source.cluster_scope.app.clone(), + cluster: target_cluster.to_owned(), + }, + namespace: target_namespace.unwrap_or(&source.namespace).to_owned(), + } +} + +#[derive(Clone, Debug, Eq, PartialEq)] +struct ConfigSyncAssessment { + changes: MutationChangeCounts, + change_set: Value, +} + +impl ConfigSyncAssessment { + fn is_noop(&self) -> bool { + self.changes.create == 0 && self.changes.update == 0 && self.changes.delete == 0 + } +} + +fn assess_config_sync( + openapi: &OpenApiCommandContext, + source: &NamespaceScopeArgs, + target: &NamespaceScopeArgs, + body: &Value, +) -> Result { + request_config_diff(openapi, source, target, body) +} + +fn request_config_diff( + openapi: &OpenApiCommandContext, + source: &NamespaceScopeArgs, + target: &NamespaceScopeArgs, + body: &Value, +) -> Result { + let path = format!("{}/items/diff", namespace_path(source)); + let sync_items = body + .get("syncItems") + .and_then(Value::as_array) + .map(Vec::as_slice) + .unwrap_or_default(); + let response = + openapi + .client + .request_with_redacted_error_body("POST", &path, Some(body.clone()))?; + parse_config_diff_assessment( + &response.data, + target, + sync_items.len(), + openapi.context.output, + ) +} + +fn parse_config_diff_assessment( + data: &Value, + target: &NamespaceScopeArgs, + source_item_count: usize, + output: OutputFormat, +) -> Result { + let entries = data.as_array().ok_or_else(|| { + CliError::invalid_input("OpenAPI config diff response was not an array", output) + })?; + if entries.len() != 1 { + return Err(CliError::invalid_input( + "OpenAPI config diff response did not contain exactly one target assessment", + output, + )); + } + let entry = &entries[0]; + if entry + .get("code") + .and_then(Value::as_i64) + .is_some_and(|code| code != 0) + || entry + .get("message") + .and_then(Value::as_str) + .is_some_and(|message| !message.trim().is_empty()) + || entry + .get("extInfo") + .and_then(Value::as_str) + .is_some_and(|message| !message.trim().is_empty()) + { + return Err(CliError::invalid_input( + "Apollo could not assess the requested target namespace; no synchronize request was sent", + output, + )); + } + validate_diff_target(entry.get("namespace"), target, output)?; + + let change_set = entry.get("diffs").unwrap_or(entry); + let create_items = diff_items(change_set, "createItems", output)?; + let update_items = diff_items(change_set, "updateItems", output)?; + let delete_items = diff_items(change_set, "deleteItems", output)?; + if !delete_items.is_empty() { + return Err(CliError::invalid_input( + "Apollo reported delete operations for config apply, which violates the conservative merge contract; no synchronize request was sent", + output, + )); + } + let changed = create_items + .len() + .checked_add(update_items.len()) + .ok_or_else(|| CliError::invalid_input("OpenAPI config diff counts overflowed", output))?; + if changed > source_item_count { + return Err(CliError::invalid_input( + "OpenAPI config diff reported more changes than the captured source snapshot contains", + output, + )); + } + + Ok(ConfigSyncAssessment { + changes: MutationChangeCounts { + create: create_items.len(), + update: update_items.len(), + delete: 0, + unchanged: source_item_count - changed, + }, + change_set: json!({ + "createItems": create_items, + "updateItems": update_items, + "deleteItems": delete_items, + }), + }) +} + +fn validate_diff_target( + namespace: Option<&Value>, + target: &NamespaceScopeArgs, + output: OutputFormat, +) -> Result<(), CliError> { + let namespace = namespace.and_then(Value::as_object).ok_or_else(|| { + CliError::invalid_input( + "OpenAPI config diff response did not identify the assessed target namespace", + output, + ) + })?; + let matches = namespace.get("appId").and_then(Value::as_str) + == Some(target.cluster_scope.app.as_str()) + && namespace + .get("env") + .and_then(Value::as_str) + .is_some_and(|env| env.eq_ignore_ascii_case(&target.cluster_scope.env)) + && namespace.get("clusterName").and_then(Value::as_str) + == Some(target.cluster_scope.cluster.as_str()) + && namespace.get("namespaceName").and_then(Value::as_str) + == Some(target.namespace.as_str()); + if !matches { + return Err(CliError::invalid_input( + "OpenAPI config diff response target did not match the requested target namespace", + output, + )); + } + Ok(()) +} + +fn diff_items<'a>( + change_set: &'a Value, + field: &str, + output: OutputFormat, +) -> Result<&'a Vec, CliError> { + change_set + .get(field) + .and_then(Value::as_array) + .ok_or_else(|| { + CliError::invalid_input( + &format!("OpenAPI config diff response did not contain a valid {field} array"), + output, + ) + }) +} + fn resolve_setup_profile_name( options: &ProfileSetupOptions, cli: &Cli, @@ -2794,6 +3098,74 @@ fn read_prompt_line(reader: &mut R, output: OutputFormat) -> Result< Ok(line) } +#[derive(Serialize)] +struct ConfigSyncResponse { + status: u16, + data: ConfigSyncResult, +} + +#[derive(Serialize)] +#[serde(rename_all = "camelCase")] +struct ConfigSyncResult { + result: &'static str, + strategy: &'static str, + target_only_behavior: &'static str, + source: MutationScope, + target: MutationScope, + changes: MutationChangeCounts, +} + +impl ConfigSyncResponse { + fn new( + result: &'static str, + source: MutationScope, + target: MutationScope, + changes: MutationChangeCounts, + ) -> Self { + Self::with_status(200, result, source, target, changes) + } + + fn with_status( + status: u16, + result: &'static str, + source: MutationScope, + target: MutationScope, + changes: MutationChangeCounts, + ) -> Self { + Self { + status, + data: ConfigSyncResult { + result, + strategy: "merge", + target_only_behavior: "preserve", + source, + target, + changes, + }, + } + } + + fn render_table(&self) -> String { + let result = match self.data.result { + "preview" => "Config synchronization preview", + "no-op" => "Config synchronization is already up to date; no mutation was sent", + "applied" => "Config synchronization completed successfully", + result => result, + }; + format!( + "{result}.\nStrategy: {}\nTarget-only behavior: {}\nSource: {}\nTarget: {}\nCreate count: {}\nUpdate count: {}\nDelete count: {}\nUnchanged count: {}", + self.data.strategy, + self.data.target_only_behavior, + self.data.source.render_table(), + self.data.target.render_table(), + self.data.changes.create, + self.data.changes.update, + self.data.changes.delete, + self.data.changes.unchanged, + ) + } +} + #[derive(Serialize)] struct ProfileListResponse { #[serde(rename = "activeProfile")] diff --git a/src/error.rs b/src/error.rs index 3c70ecf..3cf5d2a 100644 --- a/src/error.rs +++ b/src/error.rs @@ -18,6 +18,10 @@ pub enum CliErrorKind { message: String, operation: Option>, }, + StalePlan { + message: String, + operation: Box, + }, CredentialStoreUnavailable { message: String, }, @@ -130,6 +134,16 @@ impl CliError { } } + pub fn stale_plan(message: &str, operation: MutationPlan, format: OutputFormat) -> Self { + Self { + kind: CliErrorKind::StalePlan { + message: message.to_owned(), + operation: Box::new(operation), + }, + format, + } + } + pub fn invalid_input(message: &str, format: OutputFormat) -> Self { Self { kind: CliErrorKind::InvalidInput { @@ -175,6 +189,7 @@ impl CliError { CliErrorKind::InvalidConfig { .. } | CliErrorKind::MissingConfigBase { .. } | CliErrorKind::ConfirmationRequired { .. } + | CliErrorKind::StalePlan { .. } | CliErrorKind::CredentialStoreUnavailable { .. } | CliErrorKind::InvalidInput { .. } | CliErrorKind::AuthenticationRequired { .. } @@ -243,6 +258,16 @@ impl CliError { profile: None, }) } + CliErrorKind::StalePlan { message, operation } => OutputWriter::new(self.format) + .render_error(&StructuredError { + code: "stale_plan", + category: "conflict", + message: message.clone(), + operation: Some(operation.as_ref().clone()), + command: None, + path: None, + profile: None, + }), CliErrorKind::CredentialStoreUnavailable { message } => OutputWriter::new(self.format) .render_error(&StructuredError { code: "credential_store_unavailable", diff --git a/src/http.rs b/src/http.rs index 5e99f46..e24f48f 100644 --- a/src/http.rs +++ b/src/http.rs @@ -89,6 +89,25 @@ impl OpenApiClient { method: &str, path: &str, body: Option, + ) -> Result { + self.request_internal(method, path, body, false) + } + + pub fn request_with_redacted_error_body( + &self, + method: &str, + path: &str, + body: Option, + ) -> Result { + self.request_internal(method, path, body, true) + } + + fn request_internal( + &self, + method: &str, + path: &str, + body: Option, + redact_error_body: bool, ) -> Result { let path = normalize_openapi_path(path, self.format)?; let url = format!("{}{}", self.server, path); @@ -118,7 +137,11 @@ impl OpenApiClient { .map_err(|error| CliError::network(&path, &error.to_string(), self.format))?; if !status.is_success() { - let mut body = sanitize_error_body(&body, self.token.expose_secret()); + let mut body = if redact_error_body && !body.trim().is_empty() { + "[REDACTED]".to_owned() + } else { + sanitize_error_body(&body, self.token.expose_secret()) + }; if body.is_empty() && let Some(location) = redirect_location { diff --git a/src/mutation.rs b/src/mutation.rs index 8b5612a..b7c5659 100644 --- a/src/mutation.rs +++ b/src/mutation.rs @@ -31,6 +31,12 @@ pub struct MutationPlan { #[serde(skip_serializing_if = "Option::is_none")] pub append_namespace_prefix: Option, #[serde(skip_serializing_if = "Option::is_none")] + pub strategy: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub target_only_behavior: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub changes: Option, + #[serde(skip_serializing_if = "Option::is_none")] pub request: Option, } @@ -50,6 +56,9 @@ impl MutationPlan { to_release_id: None, public_namespace: None, append_namespace_prefix: None, + strategy: None, + target_only_behavior: None, + changes: None, request: None, } } @@ -94,6 +103,13 @@ impl MutationPlan { self } + pub fn with_config_sync(mut self, changes: MutationChangeCounts) -> Self { + self.strategy = Some("merge".to_owned()); + self.target_only_behavior = Some("preserve".to_owned()); + self.changes = Some(changes); + self + } + pub fn with_request(mut self, method: impl Into, path: &str) -> Self { self.request = Some(MutationRequest::new(method, path)); self @@ -134,6 +150,18 @@ impl MutationPlan { "Append namespace prefix: {append_namespace_prefix}" )); } + push_optional(&mut lines, "Strategy", self.strategy.as_deref()); + push_optional( + &mut lines, + "Target-only behavior", + self.target_only_behavior.as_deref(), + ); + if let Some(changes) = &self.changes { + lines.push(format!("Create count: {}", changes.create)); + lines.push(format!("Update count: {}", changes.update)); + lines.push(format!("Delete count: {}", changes.delete)); + lines.push(format!("Unchanged count: {}", changes.unchanged)); + } if let Some(request) = &self.request { lines.push(format!("Method: {}", table_value(&request.method))); lines.push(format!("Path: {}", table_value(&request.path))); @@ -153,6 +181,15 @@ impl MutationPlan { } } +#[derive(Clone, Debug, Eq, PartialEq, Serialize)] +#[serde(rename_all = "camelCase")] +pub struct MutationChangeCounts { + pub create: usize, + pub update: usize, + pub delete: usize, + pub unchanged: usize, +} + #[derive(Clone, Debug, Eq, PartialEq, Serialize)] #[serde(rename_all = "camelCase")] pub struct MutationScope { @@ -190,7 +227,7 @@ impl MutationScope { } } - fn render_table(&self) -> String { + pub fn render_table(&self) -> String { let fields = [ ("app", self.app.as_deref()), ("env", self.env.as_deref()), diff --git a/tests/cli.rs b/tests/cli.rs index 8280f85..2994d12 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -254,6 +254,27 @@ fn config_and_release_group_help_mentions_namespace_scope_options() { } } +#[test] +fn config_sync_help_documents_conservative_merge_behavior() { + Command::cargo_bin("apollo") + .expect("apollo binary") + .args(["config", "diff", "--help"]) + .assert() + .success() + .stdout(predicate::str::contains( + "Conservative merge preserves target-only keys", + )) + .stdout(predicate::str::contains("preview is advisory")); + + Command::cargo_bin("apollo") + .expect("apollo binary") + .args(["config", "apply", "--help"]) + .assert() + .success() + .stdout(predicate::str::contains("preserves target-only keys")) + .stdout(predicate::str::contains("separate explicit config delete")); +} + #[test] fn payload_option_help_lists_namespace_and_config_fields() { Command::cargo_bin("apollo") diff --git a/tests/openapi.rs b/tests/openapi.rs index 2097558..a601376 100644 --- a/tests/openapi.rs +++ b/tests/openapi.rs @@ -760,7 +760,7 @@ fn single_read_commands_redact_broad_config_values() { ( 200, "application/json", - r#"{"createItems":[{"key":"db.password","value":"s3cr3t"}],"updateItems":[{"key":"plain","oldValue":"old","newValue":"s3cr3t"}]}"#, + r#"[{"code":0,"message":"","namespace":{"appId":"demo","env":"FAT","clusterName":"default","namespaceName":"application"},"createItems":[{"key":"db.password","value":"s3cr3t"}],"updateItems":[],"deleteItems":[]}]"#, ), ]); write_config( @@ -785,10 +785,12 @@ fn single_read_commands_redact_broad_config_values() { .success(); let stdout = String::from_utf8(assert.get_output().stdout.clone()).expect("utf8 stdout"); let json: Value = serde_json::from_str(&stdout).expect("json stdout"); - assert_eq!(json["data"]["createItems"][0]["value"], "[REDACTED]"); - assert_eq!(json["data"]["updateItems"][0]["oldValue"], "[REDACTED]"); - assert_eq!(json["data"]["updateItems"][0]["newValue"], "[REDACTED]"); + assert_eq!(json["data"]["result"], "preview"); + assert_eq!(json["data"]["strategy"], "merge"); + assert_eq!(json["data"]["targetOnlyBehavior"], "preserve"); + assert_eq!(json["data"]["changes"]["create"], 1); assert!(!stdout.contains("s3cr3t")); + assert!(!stdout.contains("db.password")); } #[test] @@ -2550,6 +2552,16 @@ fn config_apply_with_yes_uses_synchronize_endpoint() { "application/json", r#"{"content":[{"key":"timeout","value":"3000"}],"page":0,"size":500,"total":1}"#, ), + ( + 200, + "application/json", + r#"[{"code":0,"message":"","namespace":{"appId":"demo","env":"FAT","clusterName":"default","namespaceName":"application"},"createItems":[{"key":"timeout","value":"3000"}],"updateItems":[],"deleteItems":[]}]"#, + ), + ( + 200, + "application/json", + r#"[{"code":0,"message":"","namespace":{"appId":"demo","env":"FAT","clusterName":"default","namespaceName":"application"},"createItems":[{"key":"timeout","value":"3000"}],"updateItems":[],"deleteItems":[]}]"#, + ), (200, "application/json", "{}"), ]); let home = temp_home(); @@ -2582,8 +2594,15 @@ fn config_apply_with_yes_uses_synchronize_endpoint() { assert_eq!(json["operation"]["source"]["env"], "DEV"); assert_eq!(json["operation"]["target"]["env"], "FAT"); assert_eq!(json["operation"]["target"]["namespace"], "application"); + assert_eq!(json["operation"]["strategy"], "merge"); + assert_eq!(json["operation"]["targetOnlyBehavior"], "preserve"); + assert_eq!(json["operation"]["changes"]["create"], 1); + assert_eq!(json["operation"]["changes"]["update"], 0); + assert_eq!(json["operation"]["changes"]["delete"], 0); + assert_eq!(json["operation"]["changes"]["unchanged"], 0); + assert_eq!(json["data"]["result"], "applied"); - let requests = server.requests(2); + let requests = server.requests(4); assert_eq!(requests[0].method, "GET"); assert_eq!( requests[0].path, @@ -2592,9 +2611,21 @@ fn config_apply_with_yes_uses_synchronize_endpoint() { assert_eq!(requests[1].method, "POST"); assert_eq!( requests[1].path, + "/openapi/v1/envs/DEV/apps/demo/clusters/default/namespaces/application/items/diff" + ); + assert_eq!(requests[2].method, "POST"); + assert_eq!( + requests[2].path, + "/openapi/v1/envs/DEV/apps/demo/clusters/default/namespaces/application/items/diff" + ); + assert_eq!(requests[3].method, "POST"); + assert_eq!( + requests[3].path, "/openapi/v1/envs/DEV/apps/demo/clusters/default/namespaces/application/items/synchronize" ); - let body: Value = serde_json::from_str(&requests[1].body).expect("json body"); + assert_eq!(requests[1].body, requests[2].body); + assert_eq!(requests[2].body, requests[3].body); + let body: Value = serde_json::from_str(&requests[3].body).expect("json body"); assert_eq!(body["syncToNamespaces"][0]["appId"], "demo"); assert_eq!(body["syncToNamespaces"][0]["env"], "FAT"); assert_eq!(body["syncToNamespaces"][0]["clusterName"], "default"); @@ -2603,6 +2634,86 @@ fn config_apply_with_yes_uses_synchronize_endpoint() { assert_eq!(body["syncItems"][0]["value"], "3000"); } +#[test] +fn config_apply_table_shows_detailed_redacted_plan_before_mutation() { + let diff = r#"[{"code":0,"message":"","namespace":{"appId":"demo","env":"FAT","clusterName":"default","namespaceName":"application"},"createItems":[{"key":"plain-key","value":"source-secret"}],"updateItems":[],"deleteItems":[]}]"#; + let server = TestServer::sequence(vec![ + ( + 200, + "application/json", + r#"{"content":[{"key":"plain-key","value":"source-secret"}],"page":0,"size":500,"total":1}"#, + ), + (200, "application/json", diff), + (200, "application/json", diff), + (200, "application/json", "{}"), + ]); + let home = temp_home(); + write_config( + &home, + &profile_config_with_auth_mode(&server.url(), "user-token"), + ); + + let assert = base_command(&home) + .env("APOLLO_TOKEN", "apollo_pat_test_token") + .args([ + "--yes", + "config", + "apply", + "--env", + "DEV", + "--app", + "demo", + "--target-env", + "FAT", + ]) + .assert() + .success(); + let stderr = String::from_utf8(assert.get_output().stderr.clone()).expect("utf8 stderr"); + assert!(stderr.contains("Strategy: merge")); + assert!(stderr.contains("Target-only behavior: preserve")); + assert!(stderr.contains("Create count: 1")); + assert!(stderr.contains("Delete count: 0")); + assert!(!stderr.contains("plain-key")); + assert!(!stderr.contains("source-secret")); + + let requests = server.requests(4); + assert!(requests[2].path.ends_with("/items/diff")); + assert!(requests[3].path.ends_with("/items/synchronize")); +} + +#[test] +fn config_apply_requires_initial_confirmation_before_preflight() { + let server = TestServer::empty(); + let home = temp_home(); + write_config( + &home, + &profile_config_with_auth_mode(&server.url(), "user-token"), + ); + + let assert = base_command(&home) + .env("APOLLO_TOKEN", "apollo_pat_test_token") + .args([ + "--output", + "json", + "config", + "apply", + "--env", + "DEV", + "--app", + "demo", + "--target-env", + "FAT", + ]) + .assert() + .failure(); + let stderr = String::from_utf8(assert.get_output().stderr.clone()).expect("utf8 stderr"); + let json: Value = serde_json::from_str(&stderr).expect("confirmation error json"); + assert_eq!(json["error"]["code"], "confirmation_required"); + assert_eq!(json["error"]["operation"]["source"]["env"], "DEV"); + assert_eq!(json["error"]["operation"]["target"]["env"], "FAT"); + server.assert_no_request(); +} + #[test] fn config_apply_rejects_cross_namespace_target_before_syncing() { let server = TestServer::empty(); @@ -2647,7 +2758,11 @@ fn config_diff_populates_sync_items_from_source_namespace() { "application/json", r#"{"content":[{"key":"timeout","value":"3000"}],"page":0,"size":500,"total":1}"#, ), - (200, "application/json", "{}"), + ( + 200, + "application/json", + r#"[{"code":0,"message":"","namespace":{"appId":"demo","env":"FAT","clusterName":"default","namespaceName":"application"},"createItems":[{"key":"timeout","value":"3000"}],"updateItems":[],"deleteItems":[]}]"#, + ), ]); let home = temp_home(); write_config( @@ -2688,6 +2803,102 @@ fn config_diff_populates_sync_items_from_source_namespace() { assert_eq!(body["syncItems"][0]["value"], "3000"); } +#[test] +fn config_diff_reports_add_update_unchanged_and_target_only_merge_semantics() { + let server = TestServer::sequence(vec![ + ( + 200, + "application/json", + r#"{"content":[{"key":"add","value":"add-value-secret"},{"key":"update","value":"update-value-secret"},{"key":"same","value":"same-value-secret"}],"page":0,"size":500,"total":3}"#, + ), + ( + 200, + "application/json", + r#"[{"code":0,"message":"","namespace":{"appId":"demo","env":"FAT","clusterName":"default","namespaceName":"application"},"createItems":[{"key":"add","value":"add-value-secret"}],"updateItems":[{"key":"update","value":"update-value-secret"}],"deleteItems":[]}]"#, + ), + ]); + let home = temp_home(); + write_config( + &home, + &profile_config_with_auth_mode(&server.url(), "user-token"), + ); + + let assert = base_command(&home) + .env("APOLLO_TOKEN", "apollo_pat_test_token") + .args([ + "--output", + "json", + "config", + "diff", + "--env", + "DEV", + "--app", + "demo", + "--target-env", + "FAT", + ]) + .assert() + .success(); + + let stdout = String::from_utf8(assert.get_output().stdout.clone()).expect("utf8 stdout"); + let json: Value = serde_json::from_str(&stdout).expect("merge matrix json"); + assert_eq!(json["data"]["changes"]["create"], 1); + assert_eq!(json["data"]["changes"]["update"], 1); + assert_eq!(json["data"]["changes"]["delete"], 0); + assert_eq!(json["data"]["changes"]["unchanged"], 1); + assert_eq!(json["data"]["targetOnlyBehavior"], "preserve"); + for value in [ + "add-value-secret", + "update-value-secret", + "same-value-secret", + ] { + assert!(!stdout.contains(value)); + } +} + +#[test] +fn config_diff_table_output_contains_only_scopes_and_counts() { + let server = TestServer::sequence(vec![ + ( + 200, + "application/json", + r#"{"content":[{"key":"plain-key","value":"source-secret"}],"page":0,"size":500,"total":1}"#, + ), + ( + 200, + "application/json", + r#"[{"code":0,"message":"","namespace":{"appId":"demo","env":"FAT","clusterName":"default","namespaceName":"application"},"createItems":[{"key":"plain-key","value":"source-secret"}],"updateItems":[],"deleteItems":[]}]"#, + ), + ]); + let home = temp_home(); + write_config( + &home, + &profile_config_with_auth_mode(&server.url(), "user-token"), + ); + + let assert = base_command(&home) + .env("APOLLO_TOKEN", "apollo_pat_test_token") + .args([ + "config", + "diff", + "--env", + "DEV", + "--app", + "demo", + "--target-env", + "FAT", + ]) + .assert() + .success(); + let stdout = String::from_utf8(assert.get_output().stdout.clone()).expect("utf8 stdout"); + assert!(stdout.contains("Source: app=demo env=DEV cluster=default namespace=application")); + assert!(stdout.contains("Target: app=demo env=FAT cluster=default namespace=application")); + assert!(stdout.contains("Create count: 1")); + assert!(stdout.contains("Delete count: 0")); + assert!(!stdout.contains("plain-key")); + assert!(!stdout.contains("source-secret")); +} + #[test] fn config_diff_keeps_source_sync_items_unredacted_while_redacting_output() { let server = TestServer::sequence(vec![ @@ -2699,7 +2910,7 @@ fn config_diff_keeps_source_sync_items_unredacted_while_redacting_output() { ( 200, "application/json", - r#"{"message":"apollo_pat_test_token"}"#, + r#"[{"code":0,"message":"","namespace":{"appId":"demo","env":"FAT","clusterName":"default","namespaceName":"application"},"createItems":[{"key":"token-value","value":"source-secret"}],"updateItems":[],"deleteItems":[]}]"#, ), ]); let home = temp_home(); @@ -2727,8 +2938,11 @@ fn config_diff_keeps_source_sync_items_unredacted_while_redacting_output() { let stdout = String::from_utf8(assert.get_output().stdout.clone()).expect("utf8 stdout"); let json: Value = serde_json::from_str(&stdout).expect("json stdout"); - assert_eq!(json["data"]["message"], "[REDACTED]"); + assert_eq!(json["data"]["changes"]["create"], 1); + assert_eq!(json["data"]["changes"]["update"], 0); assert!(!stdout.contains("apollo_pat_test_token")); + assert!(!stdout.contains("source-secret")); + assert!(!stdout.contains("token-value")); let requests = server.requests(2); let body: Value = serde_json::from_str(&requests[1].body).expect("json body"); @@ -2736,6 +2950,464 @@ fn config_diff_keeps_source_sync_items_unredacted_while_redacting_output() { assert_eq!(body["syncItems"][0]["value"], "source-secret"); } +#[test] +fn config_apply_noop_returns_stable_success_without_synchronize_request() { + let server = TestServer::sequence(vec![ + ( + 200, + "application/json", + r#"{"content":[{"key":"timeout","value":"3000"}],"page":0,"size":500,"total":1}"#, + ), + ( + 200, + "application/json", + r#"[{"code":0,"message":"","namespace":{"appId":"demo","env":"FAT","clusterName":"default","namespaceName":"application"},"createItems":[],"updateItems":[],"deleteItems":[]}]"#, + ), + ]); + let home = temp_home(); + write_config( + &home, + &profile_config_with_auth_mode(&server.url(), "user-token"), + ); + + let assert = base_command(&home) + .env("APOLLO_TOKEN", "apollo_pat_test_token") + .args([ + "--yes", + "--output", + "json", + "config", + "apply", + "--env", + "DEV", + "--app", + "demo", + "--target-env", + "FAT", + ]) + .assert() + .success(); + + let stdout = String::from_utf8(assert.get_output().stdout.clone()).expect("utf8 stdout"); + let json: Value = serde_json::from_str(&stdout).expect("no-op json"); + assert_eq!(json["status"], 200); + assert_eq!(json["data"]["result"], "no-op"); + assert_eq!(json["data"]["strategy"], "merge"); + assert_eq!(json["data"]["targetOnlyBehavior"], "preserve"); + assert_eq!(json["data"]["changes"]["create"], 0); + assert_eq!(json["data"]["changes"]["update"], 0); + assert_eq!(json["data"]["changes"]["delete"], 0); + assert_eq!(json["data"]["changes"]["unchanged"], 1); + assert_eq!(json["operation"]["changes"], json["data"]["changes"]); + + let requests = server.requests(2); + assert_eq!(requests[0].method, "GET"); + assert_eq!(requests[1].method, "POST"); + assert!(requests[1].path.ends_with("/items/diff")); +} + +#[test] +fn config_apply_empty_source_preserves_target_items_and_does_not_mutate() { + let server = TestServer::sequence(vec![ + ( + 200, + "application/json", + r#"{"content":[],"page":0,"size":500,"total":0}"#, + ), + ( + 200, + "application/json", + r#"[{"code":0,"message":"","namespace":{"appId":"demo","env":"FAT","clusterName":"default","namespaceName":"application"},"createItems":[],"updateItems":[],"deleteItems":[]}]"#, + ), + ]); + let home = temp_home(); + write_config( + &home, + &profile_config_with_auth_mode(&server.url(), "user-token"), + ); + + let assert = base_command(&home) + .env("APOLLO_TOKEN", "apollo_pat_test_token") + .args([ + "--yes", + "--output", + "json", + "config", + "apply", + "--env", + "DEV", + "--app", + "demo", + "--target-env", + "FAT", + ]) + .assert() + .success(); + + let stdout = String::from_utf8(assert.get_output().stdout.clone()).expect("utf8 stdout"); + let json: Value = serde_json::from_str(&stdout).expect("empty-source json"); + assert_eq!(json["data"]["result"], "no-op"); + assert_eq!(json["data"]["targetOnlyBehavior"], "preserve"); + assert_eq!(json["data"]["changes"]["delete"], 0); + + let requests = server.requests(2); + assert_eq!( + requests[1].path, + "/openapi/v1/envs/DEV/apps/demo/clusters/default/namespaces/application/items/diff" + ); + let body: Value = serde_json::from_str(&requests[1].body).expect("empty-source diff body"); + assert_eq!(body["syncItems"].as_array().map(Vec::len), Some(0)); +} + +#[test] +fn config_apply_aborts_when_target_assessment_changes_after_approval() { + let server = TestServer::sequence(vec![ + ( + 200, + "application/json", + r#"{"content":[{"key":"db.password","value":"source-secret"}],"page":0,"size":500,"total":1}"#, + ), + ( + 200, + "application/json", + r#"[{"code":0,"message":"","namespace":{"appId":"demo","env":"FAT","clusterName":"default","namespaceName":"application"},"createItems":[{"key":"db.password","value":"source-secret"}],"updateItems":[],"deleteItems":[]}]"#, + ), + ( + 200, + "application/json", + r#"[{"code":0,"message":"","namespace":{"appId":"demo","env":"FAT","clusterName":"default","namespaceName":"application"},"createItems":[],"updateItems":[{"key":"db.password","value":"source-secret"}],"deleteItems":[]}]"#, + ), + ]); + let home = temp_home(); + write_config( + &home, + &profile_config_with_auth_mode(&server.url(), "user-token"), + ); + + let assert = base_command(&home) + .env("APOLLO_TOKEN", "apollo_pat_test_token") + .args([ + "--yes", + "--output", + "json", + "config", + "apply", + "--env", + "DEV", + "--app", + "demo", + "--target-env", + "FAT", + ]) + .assert() + .failure(); + + assert!(assert.get_output().stdout.is_empty()); + let stderr = String::from_utf8(assert.get_output().stderr.clone()).expect("utf8 stderr"); + let json: Value = serde_json::from_str(&stderr).expect("stale-plan error json"); + assert_eq!(json["error"]["code"], "stale_plan"); + assert_eq!(json["error"]["category"], "conflict"); + assert_eq!(json["error"]["operation"]["changes"]["create"], 1); + assert!(!stderr.contains("source-secret")); + assert!(!stderr.contains("db.password")); + + let requests = server.requests(3); + assert!(requests[1].path.ends_with("/items/diff")); + assert!(requests[2].path.ends_with("/items/diff")); +} + +#[test] +fn config_diff_rejects_partial_source_page_before_declared_total() { + let server = TestServer::json( + r#"{"content":[{"key":"only-item","value":"secret"}],"page":0,"size":500,"total":2}"#, + ); + let home = temp_home(); + write_config( + &home, + &profile_config_with_auth_mode(&server.url(), "user-token"), + ); + + let assert = base_command(&home) + .env("APOLLO_TOKEN", "apollo_pat_test_token") + .args([ + "--output", + "json", + "config", + "diff", + "--env", + "DEV", + "--app", + "demo", + "--target-env", + "FAT", + ]) + .assert() + .failure(); + + let stderr = String::from_utf8(assert.get_output().stderr.clone()).expect("utf8 stderr"); + let json: Value = serde_json::from_str(&stderr).expect("partial-page error json"); + assert_eq!(json["error"]["code"], "invalid_input"); + assert!(stderr.contains("partial source page")); + assert!(!stderr.contains("secret")); +} + +#[test] +fn config_diff_redacts_source_page_error_body() { + let server = TestServer::new( + 500, + "application/json", + r#"{"message":"failed while reading old-target-secret"}"#, + ); + let home = temp_home(); + write_config( + &home, + &profile_config_with_auth_mode(&server.url(), "user-token"), + ); + + let assert = base_command(&home) + .env("APOLLO_TOKEN", "apollo_pat_test_token") + .args([ + "--output", + "json", + "config", + "diff", + "--env", + "DEV", + "--app", + "demo", + "--target-env", + "FAT", + ]) + .assert() + .failure(); + let stderr = String::from_utf8(assert.get_output().stderr.clone()).expect("utf8 stderr"); + let json: Value = serde_json::from_str(&stderr).expect("source error json"); + assert_eq!(json["error"]["code"], "server_error"); + assert!(stderr.contains("[REDACTED]")); + assert!(!stderr.contains("old-target-secret")); +} + +#[test] +fn config_diff_reads_more_than_one_source_page() { + let first_items = (0..500) + .map(|index| serde_json::json!({"key": format!("key-{index}"), "value": "secret"})) + .collect::>(); + let first_page = leak_json(serde_json::json!({ + "content": first_items, + "page": 0, + "size": 500, + "total": 501, + })); + let second_page = leak_json(serde_json::json!({ + "content": [{"key": "key-500", "value": "secret"}], + "page": 1, + "size": 500, + "total": 501, + })); + let diff = leak_json(serde_json::json!([{ + "code": 0, + "message": "", + "namespace": { + "appId": "demo", + "env": "FAT", + "clusterName": "default", + "namespaceName": "application", + }, + "createItems": [], + "updateItems": [], + "deleteItems": [], + }])); + let server = TestServer::sequence(vec![ + (200, "application/json", first_page), + (200, "application/json", second_page), + (200, "application/json", diff), + ]); + let home = temp_home(); + write_config( + &home, + &profile_config_with_auth_mode(&server.url(), "user-token"), + ); + + let assert = base_command(&home) + .env("APOLLO_TOKEN", "apollo_pat_test_token") + .args([ + "--output", + "json", + "config", + "diff", + "--env", + "DEV", + "--app", + "demo", + "--target-env", + "FAT", + ]) + .assert() + .success(); + + let stdout = String::from_utf8(assert.get_output().stdout.clone()).expect("utf8 stdout"); + let json: Value = serde_json::from_str(&stdout).expect("multi-page diff json"); + assert_eq!(json["data"]["changes"]["unchanged"], 501); + assert!(!stdout.contains("secret")); + + let requests = server.requests(3); + assert!(requests[0].path.ends_with("items?page=0&size=500")); + assert!(requests[1].path.ends_with("items?page=1&size=500")); + let body: Value = serde_json::from_str(&requests[2].body).expect("diff request json"); + assert_eq!(body["syncItems"].as_array().map(Vec::len), Some(501)); +} + +#[test] +fn config_apply_rejects_missing_target_assessment_and_permission_failure() { + for (status, body, expected_code) in [ + ( + 200, + r#"[{"code":0,"message":"target namespace does not exist","namespace":{"appId":"demo","env":"FAT","clusterName":"default","namespaceName":"application"},"createItems":[],"updateItems":[],"deleteItems":[]}]"#, + "invalid_input", + ), + (403, r#"{"message":"forbidden"}"#, "permission_denied"), + ( + 400, + r#"{"message":"invalid source value 3000"}"#, + "invalid_input", + ), + ] { + let server = TestServer::sequence(vec![ + ( + 200, + "application/json", + r#"{"content":[{"key":"timeout","value":"3000"}],"page":0,"size":500,"total":1}"#, + ), + (status, "application/json", body), + ]); + let home = temp_home(); + write_config( + &home, + &profile_config_with_auth_mode(&server.url(), "user-token"), + ); + + let assert = base_command(&home) + .env("APOLLO_TOKEN", "apollo_pat_test_token") + .args([ + "--yes", + "--output", + "json", + "config", + "apply", + "--env", + "DEV", + "--app", + "demo", + "--target-env", + "FAT", + ]) + .assert() + .failure(); + let stderr = String::from_utf8(assert.get_output().stderr.clone()).expect("utf8 stderr"); + let json: Value = serde_json::from_str(&stderr).expect("apply error json"); + assert_eq!(json["error"]["code"], expected_code); + assert!(!stderr.contains("3000")); + } +} + +#[test] +fn config_apply_rejects_server_side_deletes_under_merge_contract() { + let server = TestServer::sequence(vec![ + ( + 200, + "application/json", + r#"{"content":[{"key":"source","value":"secret"}],"page":0,"size":500,"total":1}"#, + ), + ( + 200, + "application/json", + r#"[{"code":0,"message":"","namespace":{"appId":"demo","env":"FAT","clusterName":"default","namespaceName":"application"},"createItems":[],"updateItems":[],"deleteItems":[{"key":"target-only","value":"keep-me"}]}]"#, + ), + ]); + let home = temp_home(); + write_config( + &home, + &profile_config_with_auth_mode(&server.url(), "user-token"), + ); + + let assert = base_command(&home) + .env("APOLLO_TOKEN", "apollo_pat_test_token") + .args([ + "--yes", + "--output", + "json", + "config", + "apply", + "--env", + "DEV", + "--app", + "demo", + "--target-env", + "FAT", + ]) + .assert() + .failure(); + let stderr = String::from_utf8(assert.get_output().stderr.clone()).expect("utf8 stderr"); + assert!(stderr.contains("conservative merge contract")); + assert!(!stderr.contains("target-only")); + assert!(!stderr.contains("keep-me")); +} + +#[test] +fn config_apply_redacts_source_values_from_synchronize_errors() { + let diff = r#"[{"code":0,"message":"","namespace":{"appId":"demo","env":"FAT","clusterName":"default","namespaceName":"application"},"createItems":[{"key":"plain-key","value":"source-secret"}],"updateItems":[],"deleteItems":[]}]"#; + let server = TestServer::sequence(vec![ + ( + 200, + "application/json", + r#"{"content":[{"key":"plain-key","value":"source-secret"}],"page":0,"size":500,"total":1}"#, + ), + (200, "application/json", diff), + (200, "application/json", diff), + ( + 400, + "application/json", + r#"{"message":"could not apply source-secret"}"#, + ), + ]); + let home = temp_home(); + write_config( + &home, + &profile_config_with_auth_mode(&server.url(), "user-token"), + ); + + let assert = base_command(&home) + .env("APOLLO_TOKEN", "apollo_pat_test_token") + .args([ + "--yes", + "--output", + "json", + "config", + "apply", + "--env", + "DEV", + "--app", + "demo", + "--target-env", + "FAT", + ]) + .assert() + .failure(); + let stderr = String::from_utf8(assert.get_output().stderr.clone()).expect("utf8 stderr"); + let json: Value = serde_json::from_str(&stderr).expect("synchronize error json"); + assert_eq!(json["error"]["code"], "invalid_input"); + assert!(stderr.contains("[REDACTED]")); + assert!(!stderr.contains("source-secret")); +} + +fn leak_json(value: Value) -> &'static str { + Box::leak( + serde_json::to_string(&value) + .expect("serialize test json") + .into_boxed_str(), + ) +} + #[derive(Debug)] struct CapturedRequest { method: String, From 39e4eec8dac8aff9e0f31c241db0916467af6a41 Mon Sep 17 00:00:00 2001 From: Jason Song Date: Sun, 30 Aug 2026 19:41:25 +0800 Subject: [PATCH 2/3] fix: detect target changes during config apply --- README.md | 11 +++-- README.zh.md | 2 +- src/command.rs | 66 +++++++++++++++++++++++++----- tests/openapi.rs | 103 +++++++++++++++++++++++++++++++++++++++++------ 4 files changed, 156 insertions(+), 26 deletions(-) diff --git a/README.md b/README.md index b0d6300..ddaeb58 100644 --- a/README.md +++ b/README.md @@ -342,10 +342,13 @@ apply result. A standalone `config diff` is advisory; it does not create a plan artifact for a later invocation. `config apply` captures its own fully paginated source snapshot, assesses that exact snapshot through -`items/diff`, and builds the detailed mutation plan from the returned change set. After approval, it -repeats the assessment with the same captured source snapshot. If the target assessment changed, the -command returns `stale_plan` and sends no synchronize request. If all counts are zero, it returns the -deterministic `data.result: "no-op"` success response without calling `items/synchronize`. +`items/diff`, captures the fully paginated target state, and builds the detailed mutation plan from +the returned change set. After approval, it reads the target again and repeats the assessment with +the same captured source snapshot. If either the target item state or assessment changed, the +command returns `stale_plan` and sends no synchronize request. The initial approval intentionally +occurs before these preflight reads; the detailed approval covers the resulting change counts. If +all counts are zero, the command returns the deterministic `data.result: "no-op"` success response +without calling `items/synchronize`. ## OpenAPI behavior diff --git a/README.zh.md b/README.zh.md index 792b28f..ff81e97 100644 --- a/README.zh.md +++ b/README.zh.md @@ -283,7 +283,7 @@ namespace 创建只会在首次批准后发送只读预检请求。如果 Apollo table 和 JSON 输出都会给出源端与目标端 scope，以及 `create`、`update`、`delete`、`unchanged` 计数。JSON 约定还会返回 `strategy: "merge"` 和 `targetOnlyBehavior: "preserve"`。diff 结果、apply 计划和 apply 结果都不会包含配置 value。 -单独执行的 `config diff` 仅供参考，不会生成可供后续调用消费的计划制品。`config apply` 会自行捕获完整分页的源端快照，用这份快照调用 `items/diff`，并根据返回的变更集生成详细变更计划。批准后，CLI 会使用同一份已捕获的源端快照再次评估目标端；如果评估结果发生变化，命令会返回 `stale_plan`，且不会发送同步请求。如果所有变更计数均为零，命令会返回确定性的 `data.result: "no-op"` 成功结果，并且不会调用 `items/synchronize`。 +单独执行的 `config diff` 仅供参考，不会生成可供后续调用消费的计划制品。`config apply` 会自行捕获完整分页的源端快照，用这份快照调用 `items/diff`，同时捕获完整分页的目标端状态，并根据返回的变更集生成详细变更计划。批准后，CLI 会重新读取目标端，并使用同一份已捕获的源端快照再次评估；如果目标端配置状态或评估结果发生变化，命令会返回 `stale_plan`，且不会发送同步请求。首次批准有意发生在这些预检读取之前，第二次详细批准则确认实际变更计数。如果所有变更计数均为零，命令会返回确定性的 `data.result: "no-op"` 成功结果，并且不会调用 `items/synchronize`。 ## OpenAPI 行为 diff --git a/src/command.rs b/src/command.rs index 78158d2..8bb22b2 100644 --- a/src/command.rs +++ b/src/command.rs @@ -1161,7 +1161,7 @@ fn execute_config( target_cluster, target_namespace, } => { - let sync_items = source_sync_items(&openapi, &scope)?; + let sync_items = config_sync_items(&openapi, &scope, ConfigSnapshotSide::Source)?; let body = sync_body( &scope, &target_env, @@ -1207,7 +1207,7 @@ fn execute_config( openapi.context.output, )); } - let sync_items = source_sync_items(&openapi, &scope)?; + let sync_items = config_sync_items(&openapi, &scope, ConfigSnapshotSide::Source)?; let body = sync_body( &scope, &target_env, @@ -1241,7 +1241,17 @@ fn execute_config( )); } + let target_snapshot = config_sync_items(&openapi, &target, ConfigSnapshotSide::Target)?; let approved = prepare_mutation_with_openapi_context(cli, &openapi, detailed_plan)?; + let current_target_snapshot = + config_sync_items(&openapi, &target, ConfigSnapshotSide::Target)?; + if current_target_snapshot != target_snapshot { + return Err(CliError::stale_plan( + "The target configuration changed after the apply plan was approved; no synchronize request was sent. Re-run the command to review a fresh plan.", + approved.plan, + openapi.context.output, + )); + } let current_assessment = request_config_diff(&openapi, &scope, &target, &body)?; if current_assessment != assessment { return Err(CliError::stale_plan( @@ -1256,6 +1266,8 @@ fn execute_config( "operator", operator.as_deref(), ); + // Apollo Portal's v1 syncItems contract is ResponseEntity, so a successful HTTP + // status is the complete response contract; there is no per-target result envelope. let synchronize_response = openapi .client @@ -2303,9 +2315,25 @@ fn item_path(scope: &NamespaceScopeArgs, key: &str) -> String { } } -fn source_sync_items( +#[derive(Clone, Copy)] +enum ConfigSnapshotSide { + Source, + Target, +} + +impl ConfigSnapshotSide { + fn label(self) -> &'static str { + match self { + Self::Source => "source", + Self::Target => "target", + } + } +} + +fn config_sync_items( openapi: &OpenApiCommandContext, scope: &NamespaceScopeArgs, + side: ConfigSnapshotSide, ) -> Result, CliError> { ensure_config_item_read_supported(openapi)?; @@ -2324,13 +2352,17 @@ fn source_sync_items( &response.data, page, SYNC_ITEMS_PAGE_SIZE, + side, openapi.context.output, )?; if let Some(expected_total) = expected_total && expected_total != total { return Err(CliError::invalid_input( - "OpenAPI item pagination total changed while the source snapshot was being read", + &format!( + "OpenAPI item pagination total changed while the {} snapshot was being read", + side.label() + ), openapi.context.output, )); } @@ -2340,7 +2372,10 @@ fn source_sync_items( if items.len() as u64 > total { return Err(CliError::invalid_input( - "OpenAPI item pagination returned more source items than its declared total", + &format!( + "OpenAPI item pagination returned more {} items than its declared total", + side.label() + ), openapi.context.output, )); } @@ -2349,13 +2384,19 @@ fn source_sync_items( } if content_len != SYNC_ITEMS_PAGE_SIZE as usize { return Err(CliError::invalid_input( - "OpenAPI item pagination returned a partial source page before the declared total was reached", + &format!( + "OpenAPI item pagination returned a partial {} page before the declared total was reached", + side.label() + ), openapi.context.output, )); } page = page.checked_add(1).ok_or_else(|| { CliError::invalid_input( - "OpenAPI item pagination exceeded the supported page range", + &format!( + "OpenAPI item pagination for the {} snapshot exceeded the supported page range", + side.label() + ), openapi.context.output, ) })?; @@ -2368,6 +2409,7 @@ fn validated_item_page( data: &Value, expected_page: u32, expected_size: u32, + side: ConfigSnapshotSide, output: OutputFormat, ) -> Result<(Vec, u64), CliError> { let content = data @@ -2401,13 +2443,19 @@ fn validated_item_page( if page != u64::from(expected_page) || size != u64::from(expected_size) { return Err(CliError::invalid_input( - "OpenAPI item list pagination metadata did not match the requested source page", + &format!( + "OpenAPI item list pagination metadata did not match the requested {} page", + side.label() + ), output, )); } if content.len() > expected_size as usize { return Err(CliError::invalid_input( - "OpenAPI item list response exceeded the requested source page size", + &format!( + "OpenAPI item list response exceeded the requested {} page size", + side.label() + ), output, )); } diff --git a/tests/openapi.rs b/tests/openapi.rs index a601376..2ac07ec 100644 --- a/tests/openapi.rs +++ b/tests/openapi.rs @@ -11,6 +11,8 @@ use predicates::prelude::predicate; use serde_json::Value; use tempfile::TempDir; +const EMPTY_ITEMS_PAGE: &str = r#"{"content":[],"page":0,"size":500,"total":0}"#; + #[test] fn api_get_calls_openapi_with_consumer_token() { let server = TestServer::json(r#"[{"appId":"demo"}]"#); @@ -2557,6 +2559,8 @@ fn config_apply_with_yes_uses_synchronize_endpoint() { "application/json", r#"[{"code":0,"message":"","namespace":{"appId":"demo","env":"FAT","clusterName":"default","namespaceName":"application"},"createItems":[{"key":"timeout","value":"3000"}],"updateItems":[],"deleteItems":[]}]"#, ), + (200, "application/json", EMPTY_ITEMS_PAGE), + (200, "application/json", EMPTY_ITEMS_PAGE), ( 200, "application/json", @@ -2602,7 +2606,7 @@ fn config_apply_with_yes_uses_synchronize_endpoint() { assert_eq!(json["operation"]["changes"]["unchanged"], 0); assert_eq!(json["data"]["result"], "applied"); - let requests = server.requests(4); + let requests = server.requests(6); assert_eq!(requests[0].method, "GET"); assert_eq!( requests[0].path, @@ -2613,19 +2617,29 @@ fn config_apply_with_yes_uses_synchronize_endpoint() { requests[1].path, "/openapi/v1/envs/DEV/apps/demo/clusters/default/namespaces/application/items/diff" ); - assert_eq!(requests[2].method, "POST"); + assert_eq!(requests[2].method, "GET"); assert_eq!( requests[2].path, - "/openapi/v1/envs/DEV/apps/demo/clusters/default/namespaces/application/items/diff" + "/openapi/v1/envs/FAT/apps/demo/clusters/default/namespaces/application/items?page=0&size=500" ); - assert_eq!(requests[3].method, "POST"); + assert_eq!(requests[3].method, "GET"); assert_eq!( requests[3].path, + "/openapi/v1/envs/FAT/apps/demo/clusters/default/namespaces/application/items?page=0&size=500" + ); + assert_eq!(requests[4].method, "POST"); + assert_eq!( + requests[4].path, + "/openapi/v1/envs/DEV/apps/demo/clusters/default/namespaces/application/items/diff" + ); + assert_eq!(requests[5].method, "POST"); + assert_eq!( + requests[5].path, "/openapi/v1/envs/DEV/apps/demo/clusters/default/namespaces/application/items/synchronize" ); - assert_eq!(requests[1].body, requests[2].body); - assert_eq!(requests[2].body, requests[3].body); - let body: Value = serde_json::from_str(&requests[3].body).expect("json body"); + assert_eq!(requests[1].body, requests[4].body); + assert_eq!(requests[4].body, requests[5].body); + let body: Value = serde_json::from_str(&requests[5].body).expect("json body"); assert_eq!(body["syncToNamespaces"][0]["appId"], "demo"); assert_eq!(body["syncToNamespaces"][0]["env"], "FAT"); assert_eq!(body["syncToNamespaces"][0]["clusterName"], "default"); @@ -2644,6 +2658,8 @@ fn config_apply_table_shows_detailed_redacted_plan_before_mutation() { r#"{"content":[{"key":"plain-key","value":"source-secret"}],"page":0,"size":500,"total":1}"#, ), (200, "application/json", diff), + (200, "application/json", EMPTY_ITEMS_PAGE), + (200, "application/json", EMPTY_ITEMS_PAGE), (200, "application/json", diff), (200, "application/json", "{}"), ]); @@ -2676,9 +2692,9 @@ fn config_apply_table_shows_detailed_redacted_plan_before_mutation() { assert!(!stderr.contains("plain-key")); assert!(!stderr.contains("source-secret")); - let requests = server.requests(4); - assert!(requests[2].path.ends_with("/items/diff")); - assert!(requests[3].path.ends_with("/items/synchronize")); + let requests = server.requests(6); + assert!(requests[4].path.ends_with("/items/diff")); + assert!(requests[5].path.ends_with("/items/synchronize")); } #[test] @@ -3072,6 +3088,8 @@ fn config_apply_aborts_when_target_assessment_changes_after_approval() { "application/json", r#"[{"code":0,"message":"","namespace":{"appId":"demo","env":"FAT","clusterName":"default","namespaceName":"application"},"createItems":[{"key":"db.password","value":"source-secret"}],"updateItems":[],"deleteItems":[]}]"#, ), + (200, "application/json", EMPTY_ITEMS_PAGE), + (200, "application/json", EMPTY_ITEMS_PAGE), ( 200, "application/json", @@ -3111,9 +3129,68 @@ fn config_apply_aborts_when_target_assessment_changes_after_approval() { assert!(!stderr.contains("source-secret")); assert!(!stderr.contains("db.password")); - let requests = server.requests(3); + let requests = server.requests(5); + assert!(requests[1].path.ends_with("/items/diff")); + assert!(requests[4].path.ends_with("/items/diff")); +} + +#[test] +fn config_apply_aborts_when_target_value_changes_but_diff_classification_does_not() { + let diff = r#"[{"code":0,"message":"","namespace":{"appId":"demo","env":"FAT","clusterName":"default","namespaceName":"application"},"createItems":[],"updateItems":[{"key":"timeout","value":"source-secret"}],"deleteItems":[]}]"#; + let server = TestServer::sequence(vec![ + ( + 200, + "application/json", + r#"{"content":[{"key":"timeout","value":"source-secret"}],"page":0,"size":500,"total":1}"#, + ), + (200, "application/json", diff), + ( + 200, + "application/json", + r#"{"content":[{"key":"timeout","value":"old-target-secret"}],"page":0,"size":500,"total":1}"#, + ), + ( + 200, + "application/json", + r#"{"content":[{"key":"timeout","value":"concurrent-target-secret"}],"page":0,"size":500,"total":1}"#, + ), + ]); + let home = temp_home(); + write_config( + &home, + &profile_config_with_auth_mode(&server.url(), "user-token"), + ); + + let assert = base_command(&home) + .env("APOLLO_TOKEN", "apollo_pat_test_token") + .args([ + "--yes", + "--output", + "json", + "config", + "apply", + "--env", + "DEV", + "--app", + "demo", + "--target-env", + "FAT", + ]) + .assert() + .failure(); + + assert!(assert.get_output().stdout.is_empty()); + let stderr = String::from_utf8(assert.get_output().stderr.clone()).expect("utf8 stderr"); + let json: Value = serde_json::from_str(&stderr).expect("stale-plan error json"); + assert_eq!(json["error"]["code"], "stale_plan"); + assert!(!stderr.contains("source-secret")); + assert!(!stderr.contains("old-target-secret")); + assert!(!stderr.contains("concurrent-target-secret")); + + let requests = server.requests(4); assert!(requests[1].path.ends_with("/items/diff")); - assert!(requests[2].path.ends_with("/items/diff")); + assert!(requests[2].path.contains("/envs/FAT/")); + assert!(requests[3].path.contains("/envs/FAT/")); } #[test] @@ -3363,6 +3440,8 @@ fn config_apply_redacts_source_values_from_synchronize_errors() { r#"{"content":[{"key":"plain-key","value":"source-secret"}],"page":0,"size":500,"total":1}"#, ), (200, "application/json", diff), + (200, "application/json", EMPTY_ITEMS_PAGE), + (200, "application/json", EMPTY_ITEMS_PAGE), (200, "application/json", diff), ( 400, From ae09d36c299811cf6bf2951ff77b8c20edc5771d Mon Sep 17 00:00:00 2001 From: Jason Song Date: Sun, 30 Aug 2026 19:50:01 +0800 Subject: [PATCH 3/3] docs: clarify config apply concurrency limit --- README.md | 7 +++++++ README.zh.md | 2 ++ src/command.rs | 2 ++ 3 files changed, 11 insertions(+) diff --git a/README.md b/README.md index ddaeb58..d7133da 100644 --- a/README.md +++ b/README.md @@ -350,6 +350,13 @@ occurs before these preflight reads; the detailed approval covers the resulting all counts are zero, the command returns the deterministic `data.result: "no-op"` success response without calling `items/synchronize`. +This stale-plan check is optimistic and best-effort. The current Apollo `items/synchronize` OpenAPI +contract has no target revision, ETag, or conditional-write precondition, so a target write after +the final check can still race with synchronization. Eliminating that window requires a +contract-first Apollo OpenAPI and server change that validates the target revision atomically with +the item update. Callers that require exclusive writes must coordinate them outside the current CLI +workflow. + ## OpenAPI behavior The first v0 implementation uses a small generic HTTP client instead of a generated SDK. This keeps diff --git a/README.zh.md b/README.zh.md index ff81e97..4635016 100644 --- a/README.zh.md +++ b/README.zh.md @@ -285,6 +285,8 @@ table 和 JSON 输出都会给出源端与目标端 scope，以及 `create`、`u 单独执行的 `config diff` 仅供参考，不会生成可供后续调用消费的计划制品。`config apply` 会自行捕获完整分页的源端快照，用这份快照调用 `items/diff`，同时捕获完整分页的目标端状态，并根据返回的变更集生成详细变更计划。批准后，CLI 会重新读取目标端，并使用同一份已捕获的源端快照再次评估；如果目标端配置状态或评估结果发生变化，命令会返回 `stale_plan`，且不会发送同步请求。首次批准有意发生在这些预检读取之前，第二次详细批准则确认实际变更计数。如果所有变更计数均为零，命令会返回确定性的 `data.result: "no-op"` 成功结果，并且不会调用 `items/synchronize`。 +该 stale-plan 检查属于乐观、best-effort 防护。当前 Apollo `items/synchronize` OpenAPI 约定没有目标 revision、ETag 或条件写入前置条件，因此最终检查之后发生的目标端写入仍可能与同步竞争。要彻底消除这个窗口，需要先修改 Apollo OpenAPI contract，再由服务端在更新配置的同一原子操作中校验目标 revision。对写入互斥有严格要求的调用方，在当前 CLI 工作流之外仍需自行协调。 + ## OpenAPI 行为 第一版 v0 实现使用一个小型通用 HTTP client，而不是生成式 SDK。这样可以让 CLI 与 Apollo 服务端仓库解耦，同时仍然保证所有内置资源命令都限定在 `/openapi/v1/*`。 diff --git a/src/command.rs b/src/command.rs index 8bb22b2..219dce7 100644 --- a/src/command.rs +++ b/src/command.rs @@ -1261,6 +1261,8 @@ fn execute_config( )); } + // Best-effort stale detection only: the current synchronize contract has no target + // revision or conditional-write precondition to close the remaining check/write race. let path = append_optional_query( format!("{}/items/synchronize", namespace_path(&scope)), "operator",