From d12bdf89ec8901da2b7acb2d00990c2b5df086dc Mon Sep 17 00:00:00 2001 From: dmbuil Date: Sun, 20 Sep 2026 01:15:08 +0200 Subject: [PATCH 1/2] feat(config): Add `clouds add` command Add `osc config clouds add`, a hand-written command that reads the JSON of an application credential from stdin and merges a ready-to-use cloud entry into clouds.yaml. Connection settings (auth_url, region, TLS options) are inherited from the cloud selected with --os-cloud; no authentication is performed. Supports --split to keep the credential in a separate secure.yaml, --overwrite to replace an existing entry, and --file to target a specific path. Files carrying the secret are written with mode 0600 and the credential is zeroized in memory after use. `osc config` is dispatched before entry_point resolves a cloud, since most config commands need none. This one does, so it repeats the lookup itself and requires --os-cloud (or --cloud-config-from-env) rather than falling back to the interactive picker: writing a config file should not prompt-and-guess which cloud to copy. Closes #1323 Signed-off-by: dmbuil --- Cargo.lock | 19 + cli/config/Cargo.toml | 15 +- cli/config/src/clouds.rs | 44 ++ cli/config/src/clouds/add.rs | 645 +++++++++++++++++++++++++++ cli/config/src/lib.rs | 12 +- openstack_cli/Cargo.toml | 1 + openstack_cli/tests/config/clouds.rs | 143 ++++++ openstack_cli/tests/config/mod.rs | 15 + openstack_cli/tests/main.rs | 1 + 9 files changed, 886 insertions(+), 9 deletions(-) create mode 100644 cli/config/src/clouds.rs create mode 100644 cli/config/src/clouds/add.rs create mode 100644 openstack_cli/tests/config/clouds.rs create mode 100644 openstack_cli/tests/config/mod.rs diff --git a/Cargo.lock b/Cargo.lock index 55773b52d..e35267a96 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3816,17 +3816,21 @@ name = "openstack-cli-config" version = "0.13.8" dependencies = [ "clap", + "dirs", "eyre", "indexmap", "openstack-cli-core", + "openstack_sdk_core", "serde", "serde_json", "structable", + "tempfile", "thiserror 2.0.20", "tracing", "yaml_serde", "yamlpatch", "yamlpath", + "zeroize", ] [[package]] @@ -4649,6 +4653,7 @@ dependencies = [ "tokio", "tracing", "tracing-subscriber", + "yaml_serde", ] [[package]] @@ -8830,6 +8835,20 @@ name = "zeroize" version = "1.9.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e13c156562582aa81c60cb29407084cdb54c4164760106ab78e6c5b0858cf64e" +dependencies = [ + "zeroize_derive", +] + +[[package]] +name = "zeroize_derive" +version = "1.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3c50655cbb0fe3fc43170059e702f1ce5e19b84cec58dc87b037a09935c2f328" +dependencies = [ + "proc-macro2", + "quote", + "syn 2.0.119", +] [[package]] name = "zerotrie" diff --git a/cli/config/Cargo.toml b/cli/config/Cargo.toml index ffb1e59ac..136281456 100644 --- a/cli/config/Cargo.toml +++ b/cli/config/Cargo.toml @@ -6,10 +6,11 @@ license.workspace = true edition.workspace = true authors.workspace = true # `yamlpatch`/`yamlpath` (see src/yaml_edit.rs) require a newer toolchain -# than the rest of the workspace. Scoped to this crate only: nothing else -# in the workspace depends on `openstack-cli-config`, so SDK-only consumers -# never pull this requirement in. See CONTRIBUTING.md / the PR description -# for context. +# than the rest of the workspace. Scoped to this crate rather than raised +# on the workspace: only `openstack_cli` (the `osc` binary) depends on +# `openstack-cli-config`, so `openstack_sdk` and the `sdk/*` crates never +# pull this requirement in and SDK-only consumers are unaffected. See +# CONTRIBUTING.md / the PR description for context. # Keep in sync with .github/workflows/ci.yml's `rust_ver` — the workspace # default build includes this crate, so a mismatch breaks CI. rust-version = "1.97" @@ -18,9 +19,11 @@ repository.workspace = true [dependencies] clap.workspace = true +dirs.workspace = true eyre.workspace = true indexmap.workspace = true openstack-cli-core.workspace = true +openstack_sdk_core.workspace = true serde.workspace = true serde_json.workspace = true structable = { workspace = true } @@ -29,6 +32,10 @@ tracing.workspace = true yaml_serde.workspace = true yamlpatch.workspace = true yamlpath.workspace = true +zeroize = { workspace = true, features = ["derive"] } + +[dev-dependencies] +tempfile.workspace = true [lints] workspace = true diff --git a/cli/config/src/clouds.rs b/cli/config/src/clouds.rs new file mode 100644 index 000000000..2581d1671 --- /dev/null +++ b/cli/config/src/clouds.rs @@ -0,0 +1,44 @@ +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +// SPDX-License-Identifier: Apache-2.0 + +//! Cloud entry operations on clouds.yaml/secure.yaml + +use clap::{Parser, Subcommand}; + +use openstack_cli_core::{cli::CliArgs, error::OpenStackCliError}; + +pub mod add; + +/// Manage cloud entries in clouds.yaml/secure.yaml +#[derive(Debug, Parser)] +pub struct CloudsCommand { + /// Cloud entry commands + #[command(subcommand)] + pub command: CloudsCommands, +} + +#[allow(missing_docs)] +#[derive(Debug, Subcommand)] +pub enum CloudsCommands { + Add(add::AddCommand), +} + +impl CloudsCommand { + /// Perform command action + pub fn take_action(&self, parsed_args: &C) -> Result<(), OpenStackCliError> { + match &self.command { + CloudsCommands::Add(cmd) => cmd.take_action(parsed_args), + } + } +} diff --git a/cli/config/src/clouds/add.rs b/cli/config/src/clouds/add.rs new file mode 100644 index 000000000..c68d529e3 --- /dev/null +++ b/cli/config/src/clouds/add.rs @@ -0,0 +1,645 @@ +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +// SPDX-License-Identifier: Apache-2.0 + +//! Add a cloud entry built from an application credential. +//! +//! Hand-written command implementing +//! . + +use std::io::{IsTerminal, Read, Write}; +use std::path::{Path, PathBuf}; + +use clap::Args; +use eyre::{OptionExt, WrapErr, eyre}; +use serde::Serialize; +use tracing::info; +use zeroize::{ZeroizeOnDrop, Zeroizing}; + +use openstack_cli_core::cli::CliArgs; +use openstack_cli_core::error::OpenStackCliError; +use openstack_sdk_core::config::{CloudConfig, ConfigFile, find_clouds_file, find_secure_file}; + +/// Add a cloud entry built from an application credential. +/// +/// Reads the JSON printed by `osc identity user application-credential +/// create -o json` from stdin and merges a ready-to-use cloud entry into +/// clouds.yaml (or, with --split, the credential into secure.yaml). The +/// entry inherits connection settings (auth_url, region, TLS options) from +/// the cloud selected with `--os-cloud`, which is required here; no +/// authentication is performed. +/// +/// An existing target file is merged into, but rewritten: comments and +/// formatting are not preserved. +#[derive(Debug, Args)] +#[command(about = "Add a cloud entry from an application credential")] +pub struct AddCommand { + /// Name of the cloud entry to add. + #[arg(default_value = "openstack", long)] + cloud_name: String, + + /// Target clouds.yaml path. Defaults to `--os-client-config-file`, else + /// the discovered standard clouds.yaml, else + /// `$XDG_CONFIG_HOME/openstack/clouds.yaml`. + #[arg(long, value_name = "PATH")] + file: Option, + + /// Write the credential id and secret into a separate secure.yaml + /// instead of clouds.yaml. + #[arg(action = clap::ArgAction::SetTrue, long)] + split: bool, + + /// Replace an existing cloud entry of the same name. + #[arg(action = clap::ArgAction::SetTrue, long)] + overwrite: bool, +} + +impl AddCommand { + /// Perform command action + pub fn take_action(&self, parsed_args: &C) -> Result<(), OpenStackCliError> { + info!("Add cloud entry to clouds.yaml"); + + let cloud_config = resolve_source_cloud(parsed_args)?; + + let input = read_stdin()?; + let (credential_id, secret) = extract_credential(&input)?; + + let connection = &parsed_args.global_opts().connection; + let clouds_path = resolve_clouds_path( + self.file.as_deref(), + connection.os_client_config_file.as_deref(), + ); + let secure_path = self.split.then(|| { + resolve_secure_path( + self.file.as_deref(), + connection.os_client_secure_file.as_deref(), + &clouds_path, + ) + }); + + // Merge is the default: an existing target is read and the entry + // added to it. + let clouds_existing = read_existing(&clouds_path)?; + let secure_existing = secure_path + .as_deref() + .map(read_existing) + .transpose()? + .flatten(); + + let (clouds_entry, secure_entry) = + build_entries(&cloud_config, &credential_id, &secret, self.split)?; + + // Render everything before writing anything: a same-name collision + // or malformed target aborts with no file touched. The rendered + // contents carry the credential; wipe them on drop. + let clouds_content = Zeroizing::new(render_target( + clouds_existing.as_deref(), + &self.cloud_name, + &clouds_entry, + self.overwrite, + )?); + let secure_content = secure_entry + .as_ref() + .map(|entry| { + render_target( + secure_existing.as_deref(), + &self.cloud_name, + entry, + self.overwrite, + ) + .map(Zeroizing::new) + }) + .transpose()?; + + write_yaml_file(&clouds_path, &clouds_content, !self.split)?; + println!( + "Added cloud `{}` to {}", + self.cloud_name, + clouds_path.display() + ); + if let (Some(path), Some(content)) = (&secure_path, &secure_content) { + write_yaml_file(path, content, true)?; + println!("Added cloud `{}` to {}", self.cloud_name, path.display()); + } + Ok(()) + } +} + +/// Read the application credential JSON from stdin. +fn read_stdin() -> Result, eyre::Report> { + let mut stdin = std::io::stdin(); + if stdin.is_terminal() { + return Err(eyre!( + "the application credential JSON is expected on stdin, e.g. `osc identity user application-credential create --name foo -o json | osc config clouds add`" + )); + } + let mut buf = Zeroizing::new(String::new()); + stdin + .read_to_string(&mut buf) + .wrap_err("cannot read stdin")?; + Ok(buf) +} + +/// Extract the credential id and secret from the piped create response. +/// Both the bare resource (the `-o json` output) and the +/// `{"application_credential": {...}}` wrapped API form are accepted. +fn extract_credential(input: &str) -> Result<(Zeroizing, Zeroizing), eyre::Report> { + let doc: serde_json::Value = serde_json::from_str(input).wrap_err("stdin is not valid JSON")?; + let resource = doc.get("application_credential").unwrap_or(&doc); + let id = resource + .get("id") + .and_then(|v| v.as_str()) + .map(|v| Zeroizing::new(v.to_string())) + .ok_or_eyre("the input is missing the credential `id`")?; + let secret = resource + .get("secret") + .and_then(|v| v.as_str()) + .map(|v| Zeroizing::new(v.to_string())) + .ok_or_eyre( + "the input is missing the credential `secret` (the secret is only returned by the create call)", + )?; + Ok((id, secret)) +} + +/// Resolve the target clouds.yaml: `--file` > `--os-client-config-file` > +/// the discovered standard file > the XDG default location. +fn resolve_clouds_path(file: Option<&Path>, os_client_config_file: Option<&str>) -> PathBuf { + if let Some(path) = file { + path.to_path_buf() + } else if let Some(path) = os_client_config_file { + PathBuf::from(path) + } else if let Some(path) = find_clouds_file() { + path + } else { + default_config_dir().join("clouds.yaml") + } +} + +/// Resolve the secure.yaml target. With `--file` the sibling secure.yaml is +/// used so the pair stays together; otherwise `--os-client-secure-file` > +/// the discovered standard file > sibling of the resolved clouds.yaml. +fn resolve_secure_path( + file: Option<&Path>, + os_client_secure_file: Option<&str>, + clouds_path: &Path, +) -> PathBuf { + if file.is_none() { + if let Some(path) = os_client_secure_file { + return PathBuf::from(path); + } + if let Some(path) = find_secure_file() { + return path; + } + } + clouds_path + .parent() + .filter(|parent| !parent.as_os_str().is_empty()) + .unwrap_or(Path::new(".")) + .join("secure.yaml") +} + +/// `$XDG_CONFIG_HOME/openstack`, matching the SDK config file discovery. +fn default_config_dir() -> PathBuf { + dirs::config_dir() + .unwrap_or_else(|| PathBuf::from(".")) + .join("openstack") +} + +/// One entry under the `clouds:` key of a clouds.yaml/secure.yaml file. +#[derive(Debug, Default, Serialize)] +struct CloudEntry { + #[serde(skip_serializing_if = "Option::is_none")] + auth_type: Option, + auth: AuthBlock, + #[serde(skip_serializing_if = "Option::is_none")] + region_name: Option, + #[serde(skip_serializing_if = "Option::is_none")] + interface: Option, + #[serde(skip_serializing_if = "Option::is_none")] + cacert: Option, + #[serde(skip_serializing_if = "Option::is_none")] + verify: Option, +} + +/// The `auth` block of a cloud entry. The block holds the credential; +/// [`ZeroizeOnDrop`] wipes it from memory on drop. +#[derive(Debug, Default, Serialize, ZeroizeOnDrop)] +struct AuthBlock { + #[serde(skip_serializing_if = "Option::is_none")] + auth_url: Option, + #[serde(skip_serializing_if = "Option::is_none")] + application_credential_id: Option, + #[serde(skip_serializing_if = "Option::is_none")] + application_credential_secret: Option, +} + +/// Build the clouds.yaml entry and, in split mode, the secure.yaml entry +/// carrying the credential id and secret. +fn build_entries( + config: &CloudConfig, + credential_id: &str, + secret: &str, + split: bool, +) -> Result<(CloudEntry, Option), eyre::Report> { + let auth_url = config + .auth + .as_ref() + .and_then(|auth| auth.auth_url.clone()) + .ok_or_eyre("cannot determine the auth_url of the current cloud")?; + + let clouds_entry = CloudEntry { + auth_type: Some("v3applicationcredential".into()), + auth: AuthBlock { + auth_url: Some(auth_url), + application_credential_id: (!split).then(|| credential_id.into()), + application_credential_secret: (!split).then(|| secret.into()), + }, + region_name: config.region_name.clone(), + // `interface` is serde-defaulted to "public" on config load; only a + // non-default value is worth exporting. + interface: config.interface.clone().filter(|i| i != "public"), + cacert: config.cacert.clone(), + verify: config.verify, + }; + let secure_entry = split.then(|| CloudEntry { + auth: AuthBlock { + auth_url: None, + application_credential_id: Some(credential_id.into()), + application_credential_secret: Some(secret.into()), + }, + ..Default::default() + }); + Ok((clouds_entry, secure_entry)) +} + +/// Resolve the cloud whose connection settings the new entry inherits. +/// +/// `osc config` is dispatched before `entry_point` resolves a cloud, since +/// most config commands need none; this command does, so it repeats the +/// lookup here. Unlike `entry_point` it never falls back to the +/// interactive picker: writing a config file is not a prompt-and-guess +/// operation, so the cloud must be named explicitly. +fn resolve_source_cloud(parsed_args: &C) -> Result { + let connection = &parsed_args.global_opts().connection; + + let mut cloud_config = if connection.cloud_config_from_env { + let mut cloud_config = CloudConfig::from_env()?; + cloud_config.name = Some( + connection + .os_cloud_name + .clone() + .unwrap_or_else(|| String::from("envvars")), + ); + cloud_config + } else { + let cloud_name = connection.os_cloud.as_ref().ok_or_eyre( + "`--os-cloud` (or `OS_CLOUD`) must be given to say which cloud's connection settings the new entry inherits; `--cloud-config-from-env` works too", + )?; + let cfg = ConfigFile::new_with_user_specified_configs( + connection.os_client_config_file.as_deref(), + connection.os_client_secure_file.as_deref(), + )?; + cfg.get_cloud_config(cloud_name)? + .ok_or_else(|| eyre!("cloud `{cloud_name}` is not configured"))? + }; + + if let Some(region_name) = &connection.os_region_name { + cloud_config.region_name = Some(region_name.clone()); + } + + Ok(cloud_config) +} + +/// Produce the final YAML for a target file. `existing` carries the current +/// file content when merging into it; comments in it are not preserved. +fn render_target( + existing: Option<&str>, + cloud_name: &str, + entry: &CloudEntry, + overwrite_entry: bool, +) -> Result { + match existing { + None => { + let mut clouds = yaml_serde::Mapping::new(); + clouds.insert(cloud_name.into(), yaml_serde::to_value(entry)?); + let mut root = yaml_serde::Mapping::new(); + root.insert("clouds".into(), yaml_serde::Value::Mapping(clouds)); + Ok(yaml_serde::to_string(&yaml_serde::Value::Mapping(root))?) + } + Some(current) => { + let mut doc: yaml_serde::Value = + yaml_serde::from_str(current).wrap_err("the target file is not valid YAML")?; + let root = doc + .as_mapping_mut() + .ok_or_eyre("the target file is not a YAML mapping")?; + let clouds = root + .entry("clouds".into()) + .or_insert_with(|| yaml_serde::Value::Mapping(Default::default())) + .as_mapping_mut() + .ok_or_eyre("`clouds` in the target file is not a mapping")?; + let name_key: yaml_serde::Value = cloud_name.into(); + if clouds.contains_key(&name_key) && !overwrite_entry { + return Err(eyre!( + "cloud `{cloud_name}` already exists in the target file; pass --overwrite to replace it" + )); + } + clouds.insert(name_key, yaml_serde::to_value(entry)?); + Ok(yaml_serde::to_string(&doc)?) + } + } +} + +/// Read the current content of a target file; `None` when it does not +/// exist. +fn read_existing(path: &Path) -> Result, eyre::Report> { + if !path.exists() { + return Ok(None); + } + Ok(Some(std::fs::read_to_string(path).wrap_err_with(|| { + format!("cannot read {}", path.display()) + })?)) +} + +/// Write a YAML file, creating parent directories; files carrying the +/// credential secret get 0600. +fn write_yaml_file(path: &Path, content: &str, contains_secret: bool) -> Result<(), eyre::Report> { + if let Some(parent) = path + .parent() + .filter(|parent| !parent.as_os_str().is_empty()) + { + std::fs::create_dir_all(parent) + .wrap_err_with(|| format!("cannot create {}", parent.display()))?; + } + let mut options = std::fs::OpenOptions::new(); + options.write(true).create(true).truncate(true); + #[cfg(unix)] + if contains_secret { + use std::os::unix::fs::OpenOptionsExt; + options.mode(0o600); + } + let mut file = options + .open(path) + .wrap_err_with(|| format!("cannot open {} for writing", path.display()))?; + file.write_all(content.as_bytes()) + .wrap_err_with(|| format!("cannot write {}", path.display()))?; + // The mode above only applies on creation; harden pre-existing files too. + #[cfg(unix)] + if contains_secret { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o600)) + .wrap_err_with(|| format!("cannot set permissions on {}", path.display()))?; + } + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn config_with(auth_url: Option<&str>) -> CloudConfig { + CloudConfig { + auth: Some(openstack_sdk_core::config::Auth { + auth_url: auth_url.map(Into::into), + ..Default::default() + }), + ..Default::default() + } + } + + #[test] + fn extract_bare_and_wrapped_input() { + let bare = r#"{"id": "cid", "secret": "sec", "name": "foo"}"#; + let (id, secret) = extract_credential(bare).unwrap(); + assert_eq!(id.as_str(), "cid"); + assert_eq!(secret.as_str(), "sec"); + + let wrapped = r#"{"application_credential": {"id": "cid", "secret": "sec"}}"#; + let (id, secret) = extract_credential(wrapped).unwrap(); + assert_eq!(id.as_str(), "cid"); + assert_eq!(secret.as_str(), "sec"); + } + + #[test] + fn extract_rejects_bad_input() { + assert!(extract_credential("not json").is_err()); + assert!(extract_credential(r#"{"secret": "sec"}"#).is_err()); + assert!(extract_credential(r#"{"id": "cid"}"#).is_err()); + assert!(extract_credential(r#"{"id": "cid", "secret": null}"#).is_err()); + } + + #[test] + fn clouds_path_resolution_order() { + assert_eq!( + resolve_clouds_path(Some(Path::new("/tmp/c.yaml")), Some("/tmp/os.yaml")), + PathBuf::from("/tmp/c.yaml") + ); + assert_eq!( + resolve_clouds_path(None, Some("/tmp/os.yaml")), + PathBuf::from("/tmp/os.yaml") + ); + } + + #[test] + fn secure_path_sibling_of_explicit_file() { + // With --file the secure.yaml must live next to it, even when a + // standard secure file would be discovered. + assert_eq!( + resolve_secure_path( + Some(Path::new("/tmp/foo/clouds.yaml")), + Some("/other/secure.yaml"), + Path::new("/tmp/foo/clouds.yaml"), + ), + PathBuf::from("/tmp/foo/secure.yaml") + ); + // Without --file the explicit secure file option wins. + assert_eq!( + resolve_secure_path(None, Some("/other/secure.yaml"), Path::new("/tmp/c.yaml")), + PathBuf::from("/other/secure.yaml") + ); + } + + #[test] + fn single_file_entry_carries_secret() { + let (clouds, secure) = build_entries( + &config_with(Some("https://keystone:5000")), + "cid", + "sec", + false, + ) + .unwrap(); + assert_eq!(clouds.auth_type.as_deref(), Some("v3applicationcredential")); + assert_eq!( + clouds.auth.auth_url.as_deref(), + Some("https://keystone:5000") + ); + assert_eq!( + clouds.auth.application_credential_id.as_deref(), + Some("cid") + ); + assert_eq!( + clouds.auth.application_credential_secret.as_deref(), + Some("sec") + ); + assert!(secure.is_none()); + } + + #[test] + fn split_moves_credentials_to_secure_entry() { + let (clouds, secure) = build_entries( + &config_with(Some("https://keystone:5000")), + "cid", + "sec", + true, + ) + .unwrap(); + assert!(clouds.auth.application_credential_secret.is_none()); + assert!(clouds.auth.application_credential_id.is_none()); + let secure = secure.expect("secure entry in split mode"); + assert_eq!( + secure.auth.application_credential_secret.as_deref(), + Some("sec") + ); + assert_eq!( + secure.auth.application_credential_id.as_deref(), + Some("cid") + ); + assert!(secure.auth.auth_url.is_none()); + assert!(secure.auth_type.is_none()); + } + + #[test] + fn inherits_allowlist_but_skips_default_interface() { + let mut config = config_with(Some("https://keystone:5000")); + config.region_name = Some("RegionOne".into()); + config.cacert = Some("/etc/ssl/custom.pem".into()); + config.verify = Some(false); + config.interface = Some("public".into()); + let (clouds, _) = build_entries(&config, "cid", "sec", false).unwrap(); + assert_eq!(clouds.region_name.as_deref(), Some("RegionOne")); + assert_eq!(clouds.cacert.as_deref(), Some("/etc/ssl/custom.pem")); + assert_eq!(clouds.verify, Some(false)); + assert!( + clouds.interface.is_none(), + "default interface must be omitted" + ); + + config.interface = Some("internal".into()); + let (clouds, _) = build_entries(&config, "cid", "sec", false).unwrap(); + assert_eq!(clouds.interface.as_deref(), Some("internal")); + } + + #[test] + fn missing_auth_url_is_an_error() { + assert!(build_entries(&config_with(None), "cid", "sec", false).is_err()); + assert!(build_entries(&CloudConfig::default(), "cid", "sec", false).is_err()); + } + + #[test] + fn render_fresh_file() { + let (entry, _) = build_entries( + &config_with(Some("https://keystone:5000")), + "cid", + "sec", + false, + ) + .unwrap(); + let out = render_target(None, "mycloud", &entry, false).unwrap(); + let doc: yaml_serde::Value = yaml_serde::from_str(&out).unwrap(); + assert_eq!( + doc["clouds"]["mycloud"]["auth"]["application_credential_id"], + yaml_serde::Value::String("cid".into()) + ); + assert_eq!( + doc["clouds"]["mycloud"]["auth_type"], + yaml_serde::Value::String("v3applicationcredential".into()) + ); + } + + #[test] + fn merge_preserves_other_clouds() { + let existing = "clouds:\n other:\n auth:\n auth_url: https://other:5000\n"; + let (entry, _) = build_entries( + &config_with(Some("https://keystone:5000")), + "cid", + "sec", + false, + ) + .unwrap(); + let out = render_target(Some(existing), "mycloud", &entry, false).unwrap(); + let doc: yaml_serde::Value = yaml_serde::from_str(&out).unwrap(); + assert_eq!( + doc["clouds"]["other"]["auth"]["auth_url"], + yaml_serde::Value::String("https://other:5000".into()) + ); + assert!(doc["clouds"]["mycloud"]["auth"]["application_credential_id"].is_string()); + } + + #[test] + fn merge_same_name_errors_without_overwrite() { + let existing = "clouds:\n mycloud:\n auth:\n auth_url: https://old:5000\n"; + let (entry, _) = build_entries( + &config_with(Some("https://keystone:5000")), + "cid", + "sec", + false, + ) + .unwrap(); + assert!(render_target(Some(existing), "mycloud", &entry, false).is_err()); + let out = render_target(Some(existing), "mycloud", &entry, true).unwrap(); + let doc: yaml_serde::Value = yaml_serde::from_str(&out).unwrap(); + assert_eq!( + doc["clouds"]["mycloud"]["auth"]["application_credential_id"], + yaml_serde::Value::String("cid".into()) + ); + } + + #[test] + fn merge_into_malformed_yaml_errors() { + let (entry, _) = build_entries( + &config_with(Some("https://keystone:5000")), + "cid", + "sec", + false, + ) + .unwrap(); + assert!(render_target(Some(": not yaml : ["), "mycloud", &entry, false).is_err()); + assert!(render_target(Some("- a\n- list\n"), "mycloud", &entry, false).is_err()); + } + + #[test] + fn read_existing_semantics() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("clouds.yaml"); + assert!(read_existing(&path).unwrap().is_none()); + + std::fs::write(&path, "clouds: {}\n").unwrap(); + assert_eq!( + read_existing(&path).unwrap().as_deref(), + Some("clouds: {}\n") + ); + } + + #[test] + fn write_creates_parents_and_sets_mode() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("nested/dir/clouds.yaml"); + write_yaml_file(&path, "clouds: {}\n", true).unwrap(); + assert_eq!(std::fs::read_to_string(&path).unwrap(), "clouds: {}\n"); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let mode = std::fs::metadata(&path).unwrap().permissions().mode(); + assert_eq!(mode & 0o777, 0o600); + } + } +} diff --git a/cli/config/src/lib.rs b/cli/config/src/lib.rs index 162c8de56..d438356b0 100644 --- a/cli/config/src/lib.rs +++ b/cli/config/src/lib.rs @@ -13,19 +13,19 @@ // SPDX-License-Identifier: Apache-2.0 //! Local client configuration file operations. //! -//! This crate is the foundation for `osc config` commands. It currently -//! provides: +//! This crate backs the `osc config` commands. It currently provides: //! //! * [`show`], displaying the effective local CLI configuration //! (`$XDG_CONFIG_HOME/osc/config.yaml`). -//! * [`yaml_edit`], a comment- and anchor-preserving YAML editor for -//! `clouds.yaml`/`secure.yaml`; command implementations built on top of -//! it (e.g. `clouds add`) land separately. +//! * [`clouds`], editing cloud entries in `clouds.yaml`/`secure.yaml`. +//! * [`yaml_edit`], the comment- and anchor-preserving YAML editing +//! primitives the `clouds` commands are built on. use clap::{Parser, Subcommand}; use openstack_cli_core::{cli::CliArgs, error::OpenStackCliError}; +pub mod clouds; pub mod show; pub mod yaml_edit; @@ -40,6 +40,7 @@ pub struct ConfigCommand { #[allow(missing_docs)] #[derive(Debug, Subcommand)] pub enum ConfigCommands { + Clouds(clouds::CloudsCommand), Show(show::ShowCommand), } @@ -47,6 +48,7 @@ impl ConfigCommand { /// Perform command action. pub async fn take_action(&self, parsed_args: &C) -> Result<(), OpenStackCliError> { match &self.command { + ConfigCommands::Clouds(cmd) => cmd.take_action(parsed_args), ConfigCommands::Show(cmd) => cmd.take_action(parsed_args).await, } } diff --git a/openstack_cli/Cargo.toml b/openstack_cli/Cargo.toml index f5c91d0ba..a6efe09e9 100644 --- a/openstack_cli/Cargo.toml +++ b/openstack_cli/Cargo.toml @@ -94,6 +94,7 @@ md5 = "^0.8.1" rand = "^0.10" reqwest.workspace = true serde_json.workspace = true +yaml_serde.workspace = true tempfile.workspace = true [[test]] diff --git a/openstack_cli/tests/config/clouds.rs b/openstack_cli/tests/config/clouds.rs new file mode 100644 index 000000000..3e9f496d8 --- /dev/null +++ b/openstack_cli/tests/config/clouds.rs @@ -0,0 +1,143 @@ +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +// SPDX-License-Identifier: Apache-2.0 + +use assert_cmd::Command; + +/// Source config the command inherits connection settings from. +const SOURCE_CLOUDS: &str = r#"clouds: + src: + auth: + auth_url: https://keystone:5000/v3 + region_name: RegionOne +"#; + +const CREATE_RESPONSE: &str = r#"{"id": "cid", "secret": "sec", "name": "deploy"}"#; + +fn add_cmd(source: &std::path::Path) -> Command { + let mut cmd = Command::cargo_bin("osc").expect("osc binary"); + cmd.arg("--os-cloud") + .arg("src") + .arg("--os-client-config-file") + .arg(source) + .arg("config") + .arg("clouds") + .arg("add"); + cmd +} + +#[test] +fn help() -> Result<(), Box> { + let mut cmd = Command::cargo_bin("osc")?; + + cmd.arg("config").arg("clouds").arg("add").arg("--help"); + cmd.assert().success(); + + Ok(()) +} + +#[test] +fn add_writes_new_file() -> Result<(), Box> { + let dir = tempfile::tempdir()?; + let source = dir.path().join("src-clouds.yaml"); + std::fs::write(&source, SOURCE_CLOUDS)?; + let target = dir.path().join("out/clouds.yaml"); + + add_cmd(&source) + .arg("--cloud-name") + .arg("prod") + .arg("--file") + .arg(&target) + .write_stdin(CREATE_RESPONSE) + .assert() + .success(); + + let doc: yaml_serde::Value = yaml_serde::from_str(&std::fs::read_to_string(&target)?)?; + let entry = &doc["clouds"]["prod"]; + assert_eq!( + entry["auth"]["application_credential_id"].as_str(), + Some("cid") + ); + assert_eq!( + entry["auth"]["application_credential_secret"].as_str(), + Some("sec") + ); + assert_eq!( + entry["auth"]["auth_url"].as_str(), + Some("https://keystone:5000/v3") + ); + assert_eq!(entry["auth_type"].as_str(), Some("v3applicationcredential")); + assert_eq!(entry["region_name"].as_str(), Some("RegionOne")); + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let mode = std::fs::metadata(&target)?.permissions().mode(); + assert_eq!(mode & 0o777, 0o600, "file with secret must be 0600"); + } + + Ok(()) +} + +#[test] +fn add_split_writes_secure_sibling() -> Result<(), Box> { + let dir = tempfile::tempdir()?; + let source = dir.path().join("src-clouds.yaml"); + std::fs::write(&source, SOURCE_CLOUDS)?; + let target = dir.path().join("out/clouds.yaml"); + + add_cmd(&source) + .arg("--split") + .arg("--file") + .arg(&target) + .write_stdin(CREATE_RESPONSE) + .assert() + .success(); + + let clouds = std::fs::read_to_string(&target)?; + assert!(!clouds.contains("sec"), "secret must not be in clouds.yaml"); + let secure = std::fs::read_to_string(dir.path().join("out/secure.yaml"))?; + assert!(secure.contains("application_credential_secret: sec")); + + Ok(()) +} + +#[test] +fn add_same_name_needs_overwrite() -> Result<(), Box> { + let dir = tempfile::tempdir()?; + let source = dir.path().join("src-clouds.yaml"); + std::fs::write(&source, SOURCE_CLOUDS)?; + let target = dir.path().join("clouds.yaml"); + std::fs::write(&target, "clouds:\n openstack:\n auth: {}\n")?; + + let output = add_cmd(&source) + .arg("--file") + .arg(&target) + .write_stdin(CREATE_RESPONSE) + .output()?; + assert!(!output.status.success()); + assert!( + String::from_utf8_lossy(&output.stderr).contains("--overwrite"), + "collision error must point at --overwrite" + ); + + add_cmd(&source) + .arg("--file") + .arg(&target) + .arg("--overwrite") + .write_stdin(CREATE_RESPONSE) + .assert() + .success(); + + Ok(()) +} diff --git a/openstack_cli/tests/config/mod.rs b/openstack_cli/tests/config/mod.rs new file mode 100644 index 000000000..1c2c590ce --- /dev/null +++ b/openstack_cli/tests/config/mod.rs @@ -0,0 +1,15 @@ +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +// SPDX-License-Identifier: Apache-2.0 + +mod clouds; diff --git a/openstack_cli/tests/main.rs b/openstack_cli/tests/main.rs index 4aa420d0e..4e3e61cbd 100644 --- a/openstack_cli/tests/main.rs +++ b/openstack_cli/tests/main.rs @@ -20,6 +20,7 @@ mod block_storage; mod catalog; #[cfg(feature = "compute")] mod compute; +mod config; #[cfg(feature = "container_infra")] mod container_infrastructure_management; #[cfg(feature = "dns")] From b05d81565ef00abb5ddcdc9a9139d569e4e816f7 Mon Sep 17 00:00:00 2001 From: dmbuil Date: Sat, 19 Sep 2026 18:39:54 +0200 Subject: [PATCH 2/2] feat(config): Preserve YAML layout in clouds add Rewire `clouds add` off serde_yaml onto the yaml_edit module, so an existing clouds.yaml or secure.yaml is edited in place: comments, &anchor/<<: *anchor merge keys and formatting outside the target entry survive byte-for-byte. --overwrite now merges into the existing entry via a new YamlDocument::merge_mapping_entry, which rewrites only scalar leaves and so keeps every comment and the entry's position. Nested mappings such as `auth` are still replaced wholesale, so a credential never half-merges; other hand-set keys on the entry are kept. Files that are empty or comment-only are handled by emitting the block directly, keeping any header comment. Tests move to yaml_serde. Signed-off-by: dmbuil --- cli/config/src/clouds/add.rs | 245 ++++++++++++++++--- cli/config/src/yaml_edit.rs | 336 ++++++++++++++++++++++++++- openstack_cli/Cargo.toml | 2 +- openstack_cli/tests/config/clouds.rs | 124 +++++++++- 4 files changed, 660 insertions(+), 47 deletions(-) diff --git a/cli/config/src/clouds/add.rs b/cli/config/src/clouds/add.rs index c68d529e3..809b802df 100644 --- a/cli/config/src/clouds/add.rs +++ b/cli/config/src/clouds/add.rs @@ -30,6 +30,8 @@ use openstack_cli_core::cli::CliArgs; use openstack_cli_core::error::OpenStackCliError; use openstack_sdk_core::config::{CloudConfig, ConfigFile, find_clouds_file, find_secure_file}; +use crate::yaml_edit; + /// Add a cloud entry built from an application credential. /// /// Reads the JSON printed by `osc identity user application-credential @@ -39,8 +41,9 @@ use openstack_sdk_core::config::{CloudConfig, ConfigFile, find_clouds_file, find /// the cloud selected with `--os-cloud`, which is required here; no /// authentication is performed. /// -/// An existing target file is merged into, but rewritten: comments and -/// formatting are not preserved. +/// An existing target file is edited in place: the new entry is spliced in +/// and everything else (comments, `&anchor`/`<<: *anchor` merge keys, +/// formatting) is left byte-for-byte as it was. #[derive(Debug, Args)] #[command(about = "Add a cloud entry from an application credential")] pub struct AddCommand { @@ -59,7 +62,11 @@ pub struct AddCommand { #[arg(action = clap::ArgAction::SetTrue, long)] split: bool, - /// Replace an existing cloud entry of the same name. + /// Update an existing cloud entry of the same name. + /// + /// The credential and the settings this command manages are replaced; + /// any other key you have set on the entry by hand (e.g. `interface`, + /// `cacert`) is kept, as are all comments in the file. #[arg(action = clap::ArgAction::SetTrue, long)] overwrite: bool, } @@ -320,43 +327,67 @@ fn resolve_source_cloud(parsed_args: &C) -> Result, cloud_name: &str, entry: &CloudEntry, overwrite_entry: bool, ) -> Result { - match existing { - None => { - let mut clouds = yaml_serde::Mapping::new(); - clouds.insert(cloud_name.into(), yaml_serde::to_value(entry)?); - let mut root = yaml_serde::Mapping::new(); - root.insert("clouds".into(), yaml_serde::Value::Mapping(clouds)); - Ok(yaml_serde::to_string(&yaml_serde::Value::Mapping(root))?) + // A file with no YAML node at all - absent, empty, or nothing but + // comments - has nothing to splice into. Build the block ourselves and + // keep whatever was there as a prefix, so a comment-only file keeps its + // header instead of tripping the parser. + let Some(current) = existing.filter(|c| has_yaml_content(c)) else { + let mut block = format!("clouds:\n {cloud_name}:\n"); + for line in yaml_serde::to_string(entry)?.lines() { + block.push_str(" "); + block.push_str(line); + block.push('\n'); } - Some(current) => { - let mut doc: yaml_serde::Value = - yaml_serde::from_str(current).wrap_err("the target file is not valid YAML")?; - let root = doc - .as_mapping_mut() - .ok_or_eyre("the target file is not a YAML mapping")?; - let clouds = root - .entry("clouds".into()) - .or_insert_with(|| yaml_serde::Value::Mapping(Default::default())) - .as_mapping_mut() - .ok_or_eyre("`clouds` in the target file is not a mapping")?; - let name_key: yaml_serde::Value = cloud_name.into(); - if clouds.contains_key(&name_key) && !overwrite_entry { - return Err(eyre!( - "cloud `{cloud_name}` already exists in the target file; pass --overwrite to replace it" - )); + return Ok(match existing { + Some(preamble) if !preamble.trim().is_empty() => { + format!("{}\n{block}", preamble.trim_end()) } - clouds.insert(name_key, yaml_serde::to_value(entry)?); - Ok(yaml_serde::to_string(&doc)?) + _ => block, + }); + }; + + let mut doc = + yaml_edit::YamlDocument::parse(current).wrap_err("the target file is not valid YAML")?; + + if doc.contains_key("clouds", cloud_name) { + if !overwrite_entry { + return Err(eyre!( + "cloud `{cloud_name}` already exists in the target file; pass --overwrite to replace it" + )); } + // Merge rather than replace: `upsert_mapping_entry` would drop the + // comment attached to the next cloud entry. See + // `merge_mapping_entry`'s doc comment for the trade-off - keys of + // the old entry that this one does not set are kept, which for a + // credential rotation is what the user wants. + doc.merge_mapping_entry("clouds", cloud_name, entry) + .wrap_err_with(|| format!("could not update cloud `{cloud_name}`"))?; + } else { + doc.upsert_mapping_entry("clouds", cloud_name, entry) + .wrap_err_with(|| format!("could not add cloud `{cloud_name}`"))?; } + + Ok(doc.source().to_string()) +} + +/// Whether `text` holds any YAML node, as opposed to being empty or only +/// blank lines and comments. +fn has_yaml_content(text: &str) -> bool { + text.lines() + .map(str::trim) + .any(|line| !line.is_empty() && !line.starts_with('#')) } /// Read the current content of a target file; `None` when it does not @@ -642,4 +673,158 @@ mod tests { assert_eq!(mode & 0o777, 0o600); } } + + /// A realistic hand-maintained clouds.yaml: header comment, an + /// `&anchor` base entry, a `<<: *anchor` consumer, inline comments and + /// a trailing comment. + fn handwritten_clouds_yaml() -> &'static str { + "\ +# Managed by hand - please keep the comments! +clouds: + base: &base + region_name: RegionOne # our only region + interface: internal + # the production cloud + devstack: + <<: *base + auth_url: https://devstack:5000 +# end of file +" + } + + #[test] + fn render_preserves_comments_and_anchors() { + let (entry, _) = build_entries( + &config_with(Some("https://keystone:5000")), + "cid", + "sec", + false, + ) + .unwrap(); + let out = render_target(Some(handwritten_clouds_yaml()), "mycloud", &entry, false).unwrap(); + + for expected in [ + "# Managed by hand - please keep the comments!", + "&base", + "# our only region", + "# the production cloud", + "<<: *base", + "# end of file", + ] { + assert!(out.contains(expected), "{expected} must survive:\n{out}"); + } + // The anchored entries must be byte-identical, not re-serialized + // with the anchor expanded into inline keys. + assert!( + out.contains(" base: &base\n region_name: RegionOne # our only region\n"), + "anchor block must be untouched:\n{out}" + ); + assert!( + out.contains(" devstack:\n <<: *base\n auth_url: https://devstack:5000\n"), + "merge-key block must be untouched:\n{out}" + ); + // And the new entry really landed. + let doc: yaml_serde::Value = yaml_serde::from_str(&out).unwrap(); + assert!(doc["clouds"]["mycloud"]["auth"]["application_credential_id"].is_string()); + } + + #[test] + fn overwrite_preserves_comments_and_keeps_hand_set_keys() { + let existing = "\ +clouds: + # my cloud + mycloud: + auth_type: v3password + interface: internal + auth: + auth_url: https://old:5000 + username: admin + password: hunter2 + # the other one + other: + auth_url: https://other:5000 +"; + let (entry, _) = build_entries( + &config_with(Some("https://keystone:5000")), + "cid", + "sec", + false, + ) + .unwrap(); + let out = render_target(Some(existing), "mycloud", &entry, true).unwrap(); + + // Comments on both the edited entry and its neighbour survive. + assert!(out.contains("# my cloud"), "{out}"); + assert!(out.contains("# the other one"), "{out}"); + // The old password credential is gone, not merged with the new one. + assert!(!out.contains("username"), "stale username:\n{out}"); + assert!(!out.contains("hunter2"), "stale password:\n{out}"); + // A hand-set connection setting the new entry does not carry is kept. + assert!(out.contains("interface: internal"), "{out}"); + + let doc: yaml_serde::Value = yaml_serde::from_str(&out).unwrap(); + assert_eq!( + doc["clouds"]["mycloud"]["auth"]["application_credential_id"], + yaml_serde::Value::String("cid".into()) + ); + assert_eq!( + doc["clouds"]["mycloud"]["auth_type"], + yaml_serde::Value::String("v3applicationcredential".into()) + ); + } + + #[test] + fn render_into_empty_clouds_key() { + // `clouds:` with nothing under it - `MergeInto` cannot handle this, + // so `yaml_edit` falls back to building the mapping wholesale. + let (entry, _) = build_entries( + &config_with(Some("https://keystone:5000")), + "cid", + "sec", + false, + ) + .unwrap(); + let out = render_target(Some("# my clouds\nclouds:\n"), "mycloud", &entry, false).unwrap(); + + assert!(out.contains("# my clouds"), "{out}"); + let doc: yaml_serde::Value = yaml_serde::from_str(&out).unwrap(); + assert!(doc["clouds"]["mycloud"]["auth"]["application_credential_id"].is_string()); + } + + #[test] + fn render_into_comment_only_file_keeps_the_header() { + // Nothing to splice into, but the user's comments must not be lost + // and the parser must not be handed a document with no root node. + let (entry, _) = build_entries( + &config_with(Some("https://keystone:5000")), + "cid", + "sec", + false, + ) + .unwrap(); + let out = render_target(Some("# just a header\n"), "mycloud", &entry, false).unwrap(); + + assert!(out.starts_with("# just a header"), "{out}"); + let doc: yaml_serde::Value = yaml_serde::from_str(&out).unwrap(); + assert!(doc["clouds"]["mycloud"]["auth"]["application_credential_id"].is_string()); + } + + #[test] + fn render_into_blank_file_is_block_style() { + // An existing but empty file behaves like a fresh one, and must not + // come out in flow style (`clouds: { ... }`). + let (entry, _) = build_entries( + &config_with(Some("https://keystone:5000")), + "cid", + "sec", + false, + ) + .unwrap(); + let out = render_target(Some("\n \n"), "mycloud", &entry, false).unwrap(); + + assert!(out.starts_with("clouds:\n mycloud:\n"), "{out}"); + assert!(!out.contains('{'), "must not be flow style:\n{out}"); + let doc: yaml_serde::Value = yaml_serde::from_str(&out).unwrap(); + assert!(doc["clouds"]["mycloud"]["auth"]["application_credential_id"].is_string()); + } } diff --git a/cli/config/src/yaml_edit.rs b/cli/config/src/yaml_edit.rs index 5924eab3d..972b75a67 100644 --- a/cli/config/src/yaml_edit.rs +++ b/cli/config/src/yaml_edit.rs @@ -33,7 +33,7 @@ use indexmap::IndexMap; use serde::Serialize; use yamlpatch::{Op, Patch, apply_yaml_patches}; -use yamlpath::{Document, Route, route}; +use yamlpath::{Component, Document, Route, route}; /// Errors from parsing or splice-editing a YAML document. #[derive(Debug, thiserror::Error)] @@ -48,6 +48,10 @@ pub enum YamlEditError { /// an unsupported document shape such as a multi-line flow mapping). #[error("the edit could not be applied: {0}")] Edit(#[from] yamlpatch::Error), + /// A merge was asked for with a value that is not a string-keyed + /// mapping, so it has no fields to merge. + #[error("only a mapping can be merged into an existing entry")] + NotAMapping, } /// A parsed YAML document that supports byte-preserving splice edits. @@ -102,19 +106,165 @@ impl YamlDocument { value: &T, ) -> Result<(), YamlEditError> { let value = yaml_serde::to_value(value)?; - let mut updates = IndexMap::new(); - updates.insert(key.to_string(), value); - let patch = Patch { - route: Route::default(), - operation: Op::MergeInto { - key: parent.to_string(), - updates, - }, + + // A `parent:` key that is present but empty (`clouds:` with + // nothing under it) has no mapping for `MergeInto` to merge into + // and makes it fail outright, so build the mapping wholesale + // instead. `Replace` at the parent route preserves the comments + // around it, and there are no existing entries to lose. + let patch = if self.parent_is_empty(parent) { + let mut mapping = yaml_serde::Mapping::new(); + mapping.insert(key.into(), value); + Patch { + route: route![parent], + operation: Op::Replace(yaml_serde::Value::Mapping(mapping)), + } + } else { + let mut updates = IndexMap::new(); + updates.insert(key.to_string(), value); + Patch { + route: Route::default(), + operation: Op::MergeInto { + key: parent.to_string(), + updates, + }, + } + }; + self.0 = apply_yaml_patches(&self.0, std::slice::from_ref(&patch))?; + Ok(()) + } + + /// Merge `value`'s fields into the existing entry at `parent.key`, + /// leaving the entry's position and every surrounding comment intact. + /// + /// This differs from [`upsert_mapping_entry`](Self::upsert_mapping_entry) + /// in both directions, and the choice between them is a real trade-off: + /// + /// - `upsert_mapping_entry` replaces the entry *wholesale*, so no key of + /// the old entry survives — but it rewrites a region of the document + /// wide enough that the **following** entry's leading comment is lost. + /// - This method rewrites only individual scalar leaves, so all comments + /// survive — but keys present in the old entry and absent from `value` + /// are **left in place** at the top level of the entry. + /// + /// Nested mappings in `value` (e.g. an `auth` block) are still replaced + /// wholesale: a key inside one that `value` does not set is removed, so + /// a credential never half-merges into the previous one. + /// + /// Prefer this when the document is hand-maintained and the caller's + /// semantics are "update this entry", and `upsert_mapping_entry` when + /// the entry must end up exactly equal to `value` and no comment can + /// follow it. + /// + /// `value` must serialize to a string-keyed mapping; anything else is + /// rejected as [`YamlEditError::NotAMapping`]. The entry must already + /// exist — check with [`contains_key`](Self::contains_key) first. + /// + /// Note: a field whose existing value is a mapping and whose new value + /// is a scalar (or vice versa) is replaced in place, which reintroduces + /// the comment loss described above. `clouds.yaml` entries have fixed + /// field shapes, so this does not arise in practice. + pub fn merge_mapping_entry( + &mut self, + parent: &str, + key: &str, + value: &T, + ) -> Result<(), YamlEditError> { + let yaml_serde::Value::Mapping(fields) = yaml_serde::to_value(value)? else { + return Err(YamlEditError::NotAMapping); }; + self.merge_fields(&[parent.to_string(), key.to_string()], fields) + } + + /// Merge `fields` into the mapping at `base`, one scalar leaf at a time. + /// + /// Rewriting a multi-line block node as a whole is what loses the + /// following entry's comment, so this only ever hands `yamlpatch` a + /// scalar replacement, a scalar insertion, or a leaf removal — each of + /// which leaves neighbouring comments alone. + fn merge_fields( + &mut self, + base: &[String], + fields: yaml_serde::Mapping, + ) -> Result<(), YamlEditError> { + for (field, new_value) in fields { + let yaml_serde::Value::String(field) = field else { + return Err(YamlEditError::NotAMapping); + }; + let mut path = base.to_vec(); + path.push(field.clone()); + + match (new_value, self.value_at(&path)) { + // A nested mapping replacing a nested mapping: drop the keys + // the new value does not carry, then recurse so only leaves + // are ever rewritten. + (yaml_serde::Value::Mapping(new_fields), Some(yaml_serde::Value::Mapping(old))) => { + for stale in old.keys().filter_map(|k| match k { + yaml_serde::Value::String(k) if !new_fields.contains_key(k.as_str()) => { + Some(k.clone()) + } + _ => None, + }) { + let mut stale_path = path.clone(); + stale_path.push(stale); + self.patch(Patch { + route: route_of(&stale_path), + operation: Op::Remove, + })?; + } + self.merge_fields(&path, new_fields)?; + } + // An existing leaf: replace it in place. + (new_value, Some(_)) => self.patch(Patch { + route: route_of(&path), + operation: Op::Replace(new_value), + })?, + // Absent: insert it into the mapping at `base`. Inserting a + // key is comment-safe even when the value is a block. + (new_value, None) => { + let (owner, owner_key) = base + .split_last() + .map(|(key, rest)| (rest, key.clone())) + .ok_or(YamlEditError::NotAMapping)?; + let mut updates = IndexMap::new(); + updates.insert(field, new_value); + self.patch(Patch { + route: route_of(owner), + operation: Op::MergeInto { + key: owner_key, + updates, + }, + })?; + } + } + } + Ok(()) + } + + /// Apply one patch to the document. + fn patch(&mut self, patch: Patch<'_>) -> Result<(), YamlEditError> { self.0 = apply_yaml_patches(&self.0, std::slice::from_ref(&patch))?; Ok(()) } + /// The current value at `path`, or `None` when `path` does not resolve. + fn value_at(&self, path: &[String]) -> Option { + let mut value: yaml_serde::Value = yaml_serde::from_str(self.0.source()).ok()?; + for key in path { + value = value.get(key.as_str())?.clone(); + } + Some(value) + } + + /// Whether `parent` is present in the document but holds no mapping + /// (`clouds:` with nothing under it). + fn parent_is_empty(&self, parent: &str) -> bool { + yaml_serde::from_str::(self.0.source()) + .ok() + .and_then(|root| root.get(parent).cloned()) + .is_some_and(|value| value.is_null()) + } + /// Remove the mapping entry at `parent.key`. pub fn remove_mapping_entry(&mut self, parent: &str, key: &str) -> Result<(), YamlEditError> { let patch = Patch { @@ -131,6 +281,15 @@ impl YamlDocument { } } +/// Build a [`Route`] from an owned key path. +fn route_of(path: &[String]) -> Route<'_> { + Route::from( + path.iter() + .map(|key| Component::Key(key.as_str().into())) + .collect::>(), + ) +} + #[cfg(test)] mod tests { use super::*; @@ -277,4 +436,163 @@ mod tests { ); Ok(()) } + #[test] + fn insert_into_empty_parent_key() -> Result<(), YamlEditError> { + // `clouds:` with nothing under it has no mapping to merge into; + // the wholesale-build fallback must still keep the comments. + let mut doc = YamlDocument::parse("# my clouds\nclouds:\n")?; + doc.upsert_mapping_entry( + "clouds", + "mycloud", + &entry("https://keystone:5000", "RegionOne"), + )?; + + let out = doc.source(); + assert!(out.contains("# my clouds"), "header must survive: {out}"); + let parsed: yaml_serde::Value = yaml_serde::from_str(out).map_err(YamlEditError::from)?; + assert_eq!( + parsed["clouds"]["mycloud"]["auth_url"].as_str(), + Some("https://keystone:5000") + ); + Ok(()) + } + + #[test] + fn insert_into_empty_parent_key_keeps_siblings() -> Result<(), YamlEditError> { + // The fallback replaces the `clouds` node only; keys after it and + // their comments must be untouched. + let mut doc = + YamlDocument::parse("clouds:\n# cache settings\ncache:\n expiration_time: 600\n")?; + doc.upsert_mapping_entry("clouds", "mycloud", &entry("https://x:5000", "RegionOne"))?; + + let out = doc.source(); + assert!(out.contains("# cache settings"), "{out}"); + assert!(out.contains("expiration_time: 600"), "{out}"); + Ok(()) + } + + #[test] + fn merge_keeps_the_next_entry_comment() -> Result<(), YamlEditError> { + // The reason `merge_mapping_entry` exists: replacing an entry with + // `upsert_mapping_entry` rewrites a wide enough region to take the + // *next* entry's leading comment with it (pinned as known + // behavior in `upsert_loses_the_next_entry_comment`), which + // is silent data loss in a hand-maintained clouds.yaml. + let existing = "# header\nclouds:\n # attached to mycloud\n mycloud:\n auth_url: https://old:5000\n region_name: RegionOld\n # attached to other\n other:\n auth_url: https://other:5000\n# trailing\n"; + let mut doc = YamlDocument::parse(existing)?; + doc.merge_mapping_entry("clouds", "mycloud", &entry("https://new:5000", "RegionNew"))?; + + let out = doc.source(); + for comment in [ + "# header", + "# attached to mycloud", + "# attached to other", + "# trailing", + ] { + assert!(out.contains(comment), "{comment} must survive: {out}"); + } + assert!(out.contains("https://new:5000"), "{out}"); + assert!(!out.contains("https://old:5000"), "{out}"); + Ok(()) + } + + #[test] + fn merge_keeps_the_entry_position() -> Result<(), YamlEditError> { + let existing = "clouds:\n mycloud:\n auth_url: https://old:5000\n region_name: RegionOld\n zzz:\n auth_url: https://zzz:5000\n"; + let mut doc = YamlDocument::parse(existing)?; + doc.merge_mapping_entry("clouds", "mycloud", &entry("https://new:5000", "RegionNew"))?; + + let out = doc.source(); + assert!( + out.find("mycloud:") < out.find("zzz:"), + "entry must not move to the end: {out}" + ); + Ok(()) + } + + #[test] + fn merge_leaves_keys_absent_from_the_new_value() -> Result<(), YamlEditError> { + // The documented trade-off of `merge_mapping_entry`: a key the old + // entry had and the new value does not is kept. For `clouds add + // --overwrite` that is the point - hand-set connection settings + // survive a credential rotation. + let existing = "clouds:\n mycloud:\n auth_url: https://old:5000\n region_name: RegionOld\n interface: internal\n"; + let mut doc = YamlDocument::parse(existing)?; + doc.merge_mapping_entry("clouds", "mycloud", &entry("https://new:5000", "RegionNew"))?; + + let out = doc.source(); + assert!(out.contains("interface: internal"), "{out}"); + assert!(out.contains("https://new:5000"), "{out}"); + Ok(()) + } + + #[test] + fn merge_replaces_nested_mappings_wholesale() -> Result<(), YamlEditError> { + // A credential block must never half-merge: the old username has + // to be gone, not merged with the new application credential. + #[derive(Serialize)] + struct Nested { + auth: IndexMap, + } + let mut auth = IndexMap::new(); + auth.insert("auth_url".to_string(), "https://new:5000".to_string()); + auth.insert( + "application_credential_id".to_string(), + "abc123".to_string(), + ); + + let existing = "clouds:\n mycloud:\n auth:\n auth_url: https://old:5000\n username: admin\n password: secret\n"; + let mut doc = YamlDocument::parse(existing)?; + doc.merge_mapping_entry("clouds", "mycloud", &Nested { auth })?; + + let out = doc.source(); + assert!(!out.contains("username"), "stale username: {out}"); + assert!(!out.contains("password"), "stale password: {out}"); + assert!(out.contains("application_credential_id"), "{out}"); + Ok(()) + } + + #[test] + fn merge_rejects_a_non_mapping_value() { + let mut doc = + YamlDocument::parse("clouds:\n mycloud:\n auth_url: https://x:5000\n").unwrap(); + assert!(matches!( + doc.merge_mapping_entry("clouds", "mycloud", &"just a string"), + Err(YamlEditError::NotAMapping) + )); + } + + #[test] + fn upsert_loses_the_next_entry_comment() -> Result<(), YamlEditError> { + // Known `yamlpatch` behavior, pinned so a future upstream fix is + // noticed here rather than silently. Callers that must not lose + // comments use `merge_mapping_entry` instead. + let existing = "clouds:\n mycloud:\n auth_url: https://old:5000\n region_name: RegionOld\n # attached to other\n other:\n auth_url: https://other:5000\n"; + let mut doc = YamlDocument::parse(existing)?; + doc.upsert_mapping_entry("clouds", "mycloud", &entry("https://new:5000", "RegionNew"))?; + + assert!( + !doc.source().contains("# attached to other"), + "upstream may have fixed this - see merge_mapping_entry's doc \ + comment and reconsider which operation --overwrite uses: {}", + doc.source() + ); + Ok(()) + } + + #[test] + fn remove_loses_and_orphans_neighbouring_comments() -> Result<(), YamlEditError> { + // Known `yamlpatch` behavior, pinned for the same reason. Removing + // a middle entry drops the *next* entry's comment and leaves the + // removed entry's own comment behind, now misattributed to its + // neighbour. A future `clouds remove` must account for this. + let existing = "clouds:\n # drop me\n drop:\n auth_url: https://drop:5000\n # keep me\n keep:\n auth_url: https://keep:5000\n"; + let mut doc = YamlDocument::parse(existing)?; + doc.remove_mapping_entry("clouds", "drop")?; + + let out = doc.source(); + assert!(!out.contains("# keep me"), "next comment eaten: {out}"); + assert!(out.contains("# drop me"), "own comment orphaned: {out}"); + Ok(()) + } } diff --git a/openstack_cli/Cargo.toml b/openstack_cli/Cargo.toml index a6efe09e9..2df20d230 100644 --- a/openstack_cli/Cargo.toml +++ b/openstack_cli/Cargo.toml @@ -94,8 +94,8 @@ md5 = "^0.8.1" rand = "^0.10" reqwest.workspace = true serde_json.workspace = true -yaml_serde.workspace = true tempfile.workspace = true +yaml_serde.workspace = true [[test]] name = "functional" diff --git a/openstack_cli/tests/config/clouds.rs b/openstack_cli/tests/config/clouds.rs index 3e9f496d8..0b5f3a13d 100644 --- a/openstack_cli/tests/config/clouds.rs +++ b/openstack_cli/tests/config/clouds.rs @@ -24,8 +24,8 @@ const SOURCE_CLOUDS: &str = r#"clouds: const CREATE_RESPONSE: &str = r#"{"id": "cid", "secret": "sec", "name": "deploy"}"#; -fn add_cmd(source: &std::path::Path) -> Command { - let mut cmd = Command::cargo_bin("osc").expect("osc binary"); +fn add_cmd(source: &std::path::Path) -> Result> { + let mut cmd = Command::cargo_bin("osc")?; cmd.arg("--os-cloud") .arg("src") .arg("--os-client-config-file") @@ -33,7 +33,7 @@ fn add_cmd(source: &std::path::Path) -> Command { .arg("config") .arg("clouds") .arg("add"); - cmd + Ok(cmd) } #[test] @@ -53,7 +53,7 @@ fn add_writes_new_file() -> Result<(), Box> { std::fs::write(&source, SOURCE_CLOUDS)?; let target = dir.path().join("out/clouds.yaml"); - add_cmd(&source) + add_cmd(&source)? .arg("--cloud-name") .arg("prod") .arg("--file") @@ -96,7 +96,7 @@ fn add_split_writes_secure_sibling() -> Result<(), Box> { std::fs::write(&source, SOURCE_CLOUDS)?; let target = dir.path().join("out/clouds.yaml"); - add_cmd(&source) + add_cmd(&source)? .arg("--split") .arg("--file") .arg(&target) @@ -120,7 +120,7 @@ fn add_same_name_needs_overwrite() -> Result<(), Box> { let target = dir.path().join("clouds.yaml"); std::fs::write(&target, "clouds:\n openstack:\n auth: {}\n")?; - let output = add_cmd(&source) + let output = add_cmd(&source)? .arg("--file") .arg(&target) .write_stdin(CREATE_RESPONSE) @@ -131,7 +131,98 @@ fn add_same_name_needs_overwrite() -> Result<(), Box> { "collision error must point at --overwrite" ); - add_cmd(&source) + add_cmd(&source)? + .arg("--file") + .arg(&target) + .arg("--overwrite") + .write_stdin(CREATE_RESPONSE) + .assert() + .success(); + + Ok(()) +} + +/// A hand-maintained clouds.yaml must come back out with everything the +/// user wrote still in it. This is the whole reason the command splices +/// rather than re-serializing. +#[test] +fn add_preserves_comments_anchors_and_formatting() -> Result<(), Box> { + let dir = tempfile::tempdir()?; + let source = dir.path().join("src-clouds.yaml"); + std::fs::write(&source, SOURCE_CLOUDS)?; + + let target = dir.path().join("clouds.yaml"); + let original = "\ +# Managed by hand - keep the comments! +clouds: + base: &base + region_name: RegionOne # our only region + interface: internal + # the production cloud + devstack: + <<: *base + auth_url: https://devstack:5000 +# end of file +"; + std::fs::write(&target, original)?; + + add_cmd(&source)? + .arg("--cloud-name") + .arg("prod") + .arg("--file") + .arg(&target) + .write_stdin(CREATE_RESPONSE) + .assert() + .success(); + + let out = std::fs::read_to_string(&target)?; + for expected in [ + "# Managed by hand - keep the comments!", + " base: &base\n region_name: RegionOne # our only region\n", + "# the production cloud", + " devstack:\n <<: *base\n auth_url: https://devstack:5000\n", + "# end of file", + ] { + assert!(out.contains(expected), "{expected:?} must survive:\n{out}"); + } + + let doc: yaml_serde::Value = yaml_serde::from_str(&out)?; + assert_eq!( + doc["clouds"]["prod"]["auth"]["application_credential_id"].as_str(), + Some("cid") + ); + + Ok(()) +} + +/// `--overwrite` must keep the file's comments and any key on the entry +/// that this command does not manage. +#[test] +fn overwrite_keeps_comments_and_hand_set_keys() -> Result<(), Box> { + let dir = tempfile::tempdir()?; + let source = dir.path().join("src-clouds.yaml"); + std::fs::write(&source, SOURCE_CLOUDS)?; + + let target = dir.path().join("clouds.yaml"); + std::fs::write( + &target, + "\ +clouds: + # the one we rotate + openstack: + auth_type: v3password + cacert: /etc/ssl/corp.pem + auth: + auth_url: https://old:5000 + username: admin + password: hunter2 + # leave me alone + other: + auth_url: https://other:5000 +", + )?; + + add_cmd(&source)? .arg("--file") .arg(&target) .arg("--overwrite") @@ -139,5 +230,24 @@ fn add_same_name_needs_overwrite() -> Result<(), Box> { .assert() .success(); + let out = std::fs::read_to_string(&target)?; + assert!(out.contains("# the one we rotate"), "{out}"); + assert!(out.contains("# leave me alone"), "{out}"); + // The superseded password credential must be gone, not merged. + assert!(!out.contains("username"), "stale username:\n{out}"); + assert!(!out.contains("hunter2"), "stale password:\n{out}"); + // A key the user set by hand and this command does not manage is kept. + assert!(out.contains("cacert: /etc/ssl/corp.pem"), "{out}"); + + let doc: yaml_serde::Value = yaml_serde::from_str(&out)?; + assert_eq!( + doc["clouds"]["openstack"]["auth"]["application_credential_id"].as_str(), + Some("cid") + ); + assert_eq!( + doc["clouds"]["openstack"]["auth_type"].as_str(), + Some("v3applicationcredential") + ); + Ok(()) }