diff --git a/README.md b/README.md index cb0345c1..8cb3116a 100644 --- a/README.md +++ b/README.md @@ -573,6 +573,11 @@ clickhousectl cloud service update \ --add-tag env=staging \ --transparent-data-encryption-key-id tde-key-1 \ --enable-core-dumps false +# `--remove-ip-allow`/`--remove-private-endpoint-id`/`--remove-tag` are +# idempotent: removing something already absent still exits 0, but +# clickhousectl warns on stderr for each entry that matched nothing (tags are +# matched by key), so a +# typo doesn't silently no-op. # Update replica scaling (vertical autoscaling — fixed replica count, variable memory) clickhousectl cloud service scale \ diff --git a/crates/clickhousectl/src/cloud/services.rs b/crates/clickhousectl/src/cloud/services.rs index 8b60d450..52ddb59d 100644 --- a/crates/clickhousectl/src/cloud/services.rs +++ b/crates/clickhousectl/src/cloud/services.rs @@ -17,7 +17,9 @@ use clickhouse_cloud_api::models::{ ServicePostRequestReleasechannel, ServiceReplicaScalingPatchRequest, ServiceState, ServiceStatePatchRequest, ServiceStatePatchRequestCommand, }; -use std::{io::IsTerminal, time::Duration}; +#[cfg(test)] +use clickhouse_cloud_api::models::{IpAccessListEntryResponse, ResourceTagsV1Response}; +use std::{collections::HashSet, io::IsTerminal, time::Duration}; use tabled::{Table, Tabled, settings::Style}; #[derive(Clone, Copy)] @@ -1263,6 +1265,85 @@ fn build_update_service_request( }) } +/// Whether `options` requests removing anything from a service, so callers +/// can decide whether a pre-update `GET` is needed to check the removals +/// against the current state. +fn has_removals(options: &ServiceUpdateOptions) -> bool { + !options.remove_ip_allow.is_empty() + || !options.remove_private_endpoint_ids.is_empty() + || !options.remove_tags.is_empty() +} + +/// Requested values that do not match anything in `current` (by exact string +/// equality). The service update API silently no-ops a removal that matches +/// nothing, which is idempotent but can hide a typo, so the caller surfaces +/// each unmatched entry as a warning instead of failing. +fn unmatched<'a>(requested: &'a [String], current: &HashSet<&str>) -> Vec<&'a str> { + requested + .iter() + .map(String::as_str) + .filter(|value| !current.contains(value)) + .collect() +} + +/// Human-readable warnings for every `--remove-*` value on `options` that +/// does not match anything in `current`. +fn unmatched_removal_warnings(options: &ServiceUpdateOptions, current: &Service) -> Vec { + let mut warnings = Vec::new(); + + let current_ip_allow: HashSet<&str> = current + .ip_access_list + .iter() + .flatten() + .filter_map(|entry| entry.source.as_deref()) + .collect(); + for value in unmatched(&options.remove_ip_allow, ¤t_ip_allow) { + warnings.push(format!( + "--remove-ip-allow {value} did not match any entry in the service's current IP allow list; no entry was removed" + )); + } + + let current_private_endpoint_ids: HashSet<&str> = current + .private_endpoint_ids + .iter() + .flatten() + .map(String::as_str) + .collect(); + for value in unmatched( + &options.remove_private_endpoint_ids, + ¤t_private_endpoint_ids, + ) { + warnings.push(format!( + "--remove-private-endpoint-id {value} did not match any private endpoint on the service; nothing was removed" + )); + } + + let current_tags: HashSet<&str> = current + .tags + .iter() + .flatten() + .filter_map(|tag| tag.key.as_deref()) + .collect(); + let remove_tag_keys: Vec = options + .remove_tags + .iter() + .map(|value| tag_key(value).to_string()) + .collect(); + for value in unmatched(&remove_tag_keys, ¤t_tags) { + warnings.push(format!( + "--remove-tag {value} did not match any tag on the service; nothing was removed" + )); + } + + warnings +} + +/// The key portion of a `key` or `key=value` tag argument, matching how +/// [`crate::cloud::shared::parse_tag`] splits removal/addition arguments. +fn tag_key(value: &str) -> &str { + value.split_once('=').map_or(value, |(key, _)| key).trim() +} + fn build_service_password_patch_request( options: &ServiceResetPasswordOptions, ) -> ServicePasswordPatchRequest { @@ -1536,6 +1617,14 @@ async fn service_update( ) -> CloudResult<()> { let request = build_update_service_request(&options)?; let org_id = resolve_org_id(client, options.org_id.as_deref()).await?; + + if has_removals(&options) { + let current = client.get_service(&org_id, service_id).await?; + for warning in unmatched_removal_warnings(&options, ¤t) { + eprint_line(format!("Warning: {warning}")); + } + } + let service = client.update_service(&org_id, service_id, &request).await?; if json { @@ -4526,6 +4615,121 @@ mod tests { assert!(error.to_string().contains("turbo")); } + #[test] + fn has_removals_is_false_when_no_remove_flags_set() { + assert!(!has_removals(&ServiceUpdateOptions::default())); + assert!(!has_removals(&ServiceUpdateOptions { + add_ip_allow: vec!["10.0.0.0/8".to_string()], + ..Default::default() + })); + } + + #[test] + fn has_removals_is_true_when_any_remove_flag_set() { + assert!(has_removals(&ServiceUpdateOptions { + remove_ip_allow: vec!["10.0.0.0/8".to_string()], + ..Default::default() + })); + assert!(has_removals(&ServiceUpdateOptions { + remove_private_endpoint_ids: vec!["pe-1".to_string()], + ..Default::default() + })); + assert!(has_removals(&ServiceUpdateOptions { + remove_tags: vec!["env".to_string()], + ..Default::default() + })); + } + + #[test] + fn unmatched_returns_only_values_absent_from_current() { + let current: HashSet<&str> = ["a", "b"].into_iter().collect(); + assert_eq!( + unmatched(&["a".to_string(), "c".to_string()], ¤t), + vec!["c"] + ); + assert!(unmatched(&["a".to_string(), "b".to_string()], ¤t).is_empty()); + } + + #[test] + fn tag_key_strips_value_and_trims() { + assert_eq!(tag_key("env"), "env"); + assert_eq!(tag_key("env=prod"), "env"); + assert_eq!(tag_key(" env = prod"), "env"); + } + + fn service_with_ip_allow_endpoints_and_tags() -> Service { + Service { + ip_access_list: Some(vec![IpAccessListEntryResponse { + source: Some("10.0.0.0/8".to_string()), + description: None, + }]), + private_endpoint_ids: Some(vec!["pe-1".to_string()]), + tags: Some(vec![ResourceTagsV1Response { + key: Some("env".to_string()), + value: Some("prod".to_string()), + }]), + ..Default::default() + } + } + + #[test] + fn unmatched_removal_warnings_is_empty_when_everything_matches() { + let current = service_with_ip_allow_endpoints_and_tags(); + let options = ServiceUpdateOptions { + remove_ip_allow: vec!["10.0.0.0/8".to_string()], + remove_private_endpoint_ids: vec!["pe-1".to_string()], + remove_tags: vec!["env".to_string()], + ..Default::default() + }; + assert!(unmatched_removal_warnings(&options, ¤t).is_empty()); + } + + #[test] + fn unmatched_removal_warnings_names_every_unmatched_entry() { + let current = service_with_ip_allow_endpoints_and_tags(); + let options = ServiceUpdateOptions { + remove_ip_allow: vec!["10.99.99.99/32".to_string()], + remove_private_endpoint_ids: vec!["pe-missing".to_string()], + remove_tags: vec!["missing=value".to_string()], + ..Default::default() + }; + let warnings = unmatched_removal_warnings(&options, ¤t); + assert_eq!(warnings.len(), 3); + assert!( + warnings[0].contains("--remove-ip-allow 10.99.99.99/32") + && warnings[0].contains("did not match"), + "{warnings:?}" + ); + assert!( + warnings[1].contains("--remove-private-endpoint-id pe-missing"), + "{warnings:?}" + ); + assert!(warnings[2].contains("--remove-tag missing"), "{warnings:?}"); + } + + #[test] + fn unmatched_removal_warnings_matches_tag_removal_by_key_only() { + // Removing "env=staging" should still match a current "env=prod" tag: + // the key is what the server matches on for removal. + let current = service_with_ip_allow_endpoints_and_tags(); + let options = ServiceUpdateOptions { + remove_tags: vec!["env=staging".to_string()], + ..Default::default() + }; + assert!(unmatched_removal_warnings(&options, ¤t).is_empty()); + } + + #[test] + fn unmatched_removal_warnings_handles_empty_current_lists() { + let options = ServiceUpdateOptions { + remove_ip_allow: vec!["10.0.0.0/8".to_string()], + ..Default::default() + }; + let warnings = unmatched_removal_warnings(&options, &Service::default()); + assert_eq!(warnings.len(), 1); + assert!(warnings[0].contains("--remove-ip-allow 10.0.0.0/8")); + } + #[test] fn build_create_service_request_rejects_invalid_provider() { let error = build_create_service_request(&CreateServiceOptions { diff --git a/crates/clickhousectl/tests/cli_request_shape_test.rs b/crates/clickhousectl/tests/cli_request_shape_test.rs index 3b7d3273..315c70d6 100644 --- a/crates/clickhousectl/tests/cli_request_shape_test.rs +++ b/crates/clickhousectl/tests/cli_request_shape_test.rs @@ -6302,3 +6302,176 @@ async fn key_create_succeeds_for_a_pre_hashed_key_without_generated_material() { "a pre-hashed create must not report key material:\n{stdout}", ); } + +// ── `service update --remove-*` warns on unmatched entries (issue #612) ──── + +/// Mount a `GET` returning the given `ipAccessList`/`privateEndpointIds`/`tags` +/// snapshot and a `PATCH` that echoes it back unchanged, both scoped to +/// `svc-1` in `org-1`. +async fn mount_service_update_round_trip(mock: &MockServer, current: serde_json::Value) { + Mock::given(method("GET")) + .and(path("/v1/organizations/org-1/services/svc-1")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "result": current, + "status": 200, + "requestId": "stub-service-get", + }))) + .expect(1) + .mount(mock) + .await; + Mock::given(method("PATCH")) + .and(path("/v1/organizations/org-1/services/svc-1")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "result": current, + "status": 200, + "requestId": "stub-service-update", + }))) + .expect(1) + .mount(mock) + .await; +} + +#[tokio::test] +async fn service_update_warns_when_remove_ip_allow_matches_nothing() { + let mock = MockServer::start().await; + mount_service_update_round_trip( + &mock, + serde_json::json!({ + "id": "22222222-3333-4444-5555-666666666666", + "name": "demo", + "ipAccessList": [{ "source": "10.0.0.0/8" }], + }), + ) + .await; + + let output = invoke_cli_with_cloud_credentials( + &mock, + &[ + "service", + "update", + "svc-1", + "--org-id", + "org-1", + "--remove-ip-allow", + "10.99.99.99/32", + ], + ); + + assert_success(&output); + assert_eq!( + String::from_utf8_lossy(&output.stderr), + "Warning: --remove-ip-allow 10.99.99.99/32 did not match any entry in the service's \ + current IP allow list; no entry was removed\n" + ); +} + +#[tokio::test] +async fn service_update_does_not_warn_when_remove_ip_allow_matches() { + let mock = MockServer::start().await; + mount_service_update_round_trip( + &mock, + serde_json::json!({ + "id": "22222222-3333-4444-5555-666666666666", + "name": "demo", + "ipAccessList": [{ "source": "10.0.0.0/8" }], + }), + ) + .await; + + let output = invoke_cli_with_cloud_credentials( + &mock, + &[ + "service", + "update", + "svc-1", + "--org-id", + "org-1", + "--remove-ip-allow", + "10.0.0.0/8", + ], + ); + + assert_success(&output); + assert!( + output.stderr.is_empty(), + "unexpected stderr: {}", + String::from_utf8_lossy(&output.stderr) + ); +} + +#[tokio::test] +async fn service_update_warns_on_unmatched_private_endpoint_and_tag_removal() { + let mock = MockServer::start().await; + mount_service_update_round_trip( + &mock, + serde_json::json!({ + "id": "22222222-3333-4444-5555-666666666666", + "name": "demo", + "privateEndpointIds": ["pe-1"], + "tags": [{ "key": "env", "value": "prod" }], + }), + ) + .await; + + let output = invoke_cli_with_cloud_credentials( + &mock, + &[ + "service", + "update", + "svc-1", + "--org-id", + "org-1", + "--remove-private-endpoint-id", + "pe-missing", + "--remove-tag", + "missing", + ], + ); + + assert_success(&output); + let stderr = String::from_utf8_lossy(&output.stderr); + assert_eq!( + stderr, + "Warning: --remove-private-endpoint-id pe-missing did not match any private endpoint \ + on the service; nothing was removed\n\ + Warning: --remove-tag missing did not match any tag on the service; nothing was \ + removed\n" + ); +} + +#[tokio::test] +async fn service_update_skips_get_when_no_removals_requested() { + let mock = MockServer::start().await; + // No removal flags: the handler must not issue the pre-update GET at all, + // since idempotent adds/renames never risk a silent no-op. + Mock::given(method("GET")) + .and(path("/v1/organizations/org-1/services/svc-1")) + .respond_with(ResponseTemplate::new(500)) + .expect(0) + .mount(&mock) + .await; + Mock::given(method("PATCH")) + .and(path("/v1/organizations/org-1/services/svc-1")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "result": { "id": "22222222-3333-4444-5555-666666666666", "name": "renamed" }, + "status": 200, + "requestId": "stub-service-update", + }))) + .expect(1) + .mount(&mock) + .await; + + let output = invoke_cli_with_cloud_credentials( + &mock, + &[ + "service", "update", "svc-1", "--org-id", "org-1", "--name", "renamed", + ], + ); + + assert_success(&output); + assert!( + output.stderr.is_empty(), + "unexpected stderr: {}", + String::from_utf8_lossy(&output.stderr) + ); +}