From 5354f1255f3c688ee26639870f1a8bf47fb46966 Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Tue, 3 Dec 2024 17:27:55 +0000 Subject: [PATCH 01/17] Add unit tests for matching configurations validation Signed-off-by: James Rhodes --- Cargo.lock | 1 + crates/core/tedge/Cargo.toml | 1 + crates/core/tedge/src/cli/connect/command.rs | 103 ++++++++++++++++++- 3 files changed, 103 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index dc2ea1ae40b..f0971715a00 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3848,6 +3848,7 @@ dependencies = [ "tedge-write", "tedge_api", "tedge_config", + "tedge_test_utils", "tedge_utils", "tempfile", "test-case", diff --git a/crates/core/tedge/Cargo.toml b/crates/core/tedge/Cargo.toml index f7a265fc9bc..e21edde3a33 100644 --- a/crates/core/tedge/Cargo.toml +++ b/crates/core/tedge/Cargo.toml @@ -62,6 +62,7 @@ mockito = { workspace = true } mqtt_tests = { workspace = true } pem = { workspace = true } predicates = { workspace = true } +tedge_test_utils = { workspace = true } tempfile = { workspace = true } test-case = { workspace = true } tokio = { workspace = true } diff --git a/crates/core/tedge/src/cli/connect/command.rs b/crates/core/tedge/src/cli/connect/command.rs index fbc3922d1e7..71057e8d769 100644 --- a/crates/core/tedge/src/cli/connect/command.rs +++ b/crates/core/tedge/src/cli/connect/command.rs @@ -279,7 +279,6 @@ impl ConnectCommand { } } -// TODO unit test this fn validate_config(config: &TEdgeConfig, cloud: &MaybeBorrowedCloud<'_>) -> anyhow::Result<()> { match cloud { MaybeBorrowedCloud::Aws(_) => { @@ -288,6 +287,7 @@ fn validate_config(config: &TEdgeConfig, cloud: &MaybeBorrowedCloud<'_>) -> anyh .keys() .map(|s| Some(s?.to_string())) .collect::>(); + disallow_matching_configurations(config, ReadableKey::AwsUrl, &profiles)?; disallow_matching_configurations(config, ReadableKey::AwsBridgeTopicPrefix, &profiles)?; } MaybeBorrowedCloud::Azure(_) => { @@ -296,6 +296,7 @@ fn validate_config(config: &TEdgeConfig, cloud: &MaybeBorrowedCloud<'_>) -> anyh .keys() .map(|s| Some(s?.to_string())) .collect::>(); + disallow_matching_configurations(config, ReadableKey::AzUrl, &profiles)?; disallow_matching_configurations(config, ReadableKey::AzBridgeTopicPrefix, &profiles)?; } MaybeBorrowedCloud::C8y(_) => { @@ -304,6 +305,7 @@ fn validate_config(config: &TEdgeConfig, cloud: &MaybeBorrowedCloud<'_>) -> anyh .keys() .map(|s| Some(s?.to_string())) .collect::>(); + disallow_matching_configurations(config, ReadableKey::C8yUrl, &profiles)?; disallow_matching_configurations(config, ReadableKey::C8yBridgeTopicPrefix, &profiles)?; disallow_matching_configurations(config, ReadableKey::C8yProxyBindPort, &profiles)?; } @@ -332,7 +334,7 @@ fn disallow_matching_configurations( .collect::>() .join(", "); - bail!("The configurations: {keys} should be set to diffrent values, but are currently set to the same value"); + bail!("The configurations: {keys} should be set to different values before connecting, but are currently set to the same value"); } Ok(()) } @@ -1160,4 +1162,101 @@ mod tests { Duration::from_millis(n) } } + + mod validate_config { + use super::super::validate_config; + use super::MaybeBorrowedCloud; + use tedge_config::TEdgeConfigLocation; + use tedge_test_utils::fs::TempTedgeDir; + + #[test] + fn allows_default_config() { + let cloud = MaybeBorrowedCloud::C8y(None); + let ttd = TempTedgeDir::new(); + let loc = TEdgeConfigLocation::from_custom_root(ttd.path()); + let config = loc.load().unwrap(); + + validate_config(&config, &cloud).unwrap(); + } + + #[test] + fn rejects_conflicting_topic_prefixes() { + let cloud = MaybeBorrowedCloud::C8y(None); + let ttd = TempTedgeDir::new(); + let loc = TEdgeConfigLocation::from_custom_root(ttd.path()); + loc.update_toml(&|dto, _| { + dto.try_update_str(&"c8y.url".parse().unwrap(), "latest.example.com") + .unwrap(); + dto.try_update_str(&"c8y@new.url".parse().unwrap(), "example.com") + .unwrap(); + dto.try_update_str(&"c8y@new.proxy.bind.port".parse().unwrap(), "8002") + .unwrap(); + Ok(()) + }) + .unwrap(); + let config = loc.load().unwrap(); + + let err = validate_config(&config, &cloud).unwrap_err(); + eprintln!("err={err}"); + assert!(err.to_string().contains("c8y.bridge.topic_prefix")); + assert!(err.to_string().contains("c8y@new.bridge.topic_prefix")); + } + + #[test] + fn rejects_conflicting_bind_ports() { + let cloud = MaybeBorrowedCloud::C8y(None); + let ttd = TempTedgeDir::new(); + let loc = TEdgeConfigLocation::from_custom_root(ttd.path()); + loc.update_toml(&|dto, _| { + dto.try_update_str(&"c8y.url".parse().unwrap(), "latest.example.com") + .unwrap(); + dto.try_update_str(&"c8y@new.url".parse().unwrap(), "example.com") + .unwrap(); + dto.try_update_str(&"c8y@new.bridge.topic_prefix".parse().unwrap(), "c8y-new") + .unwrap(); + Ok(()) + }) + .unwrap(); + let config = loc.load().unwrap(); + + let err = validate_config(&config, &cloud).unwrap_err(); + eprintln!("err={err}"); + assert!(err.to_string().contains("c8y.proxy.bind.port")); + assert!(err.to_string().contains("c8y@new.proxy.bind.port")); + } + + #[test] + fn ignores_conflicting_configs_for_other_clouds() { + let cloud = MaybeBorrowedCloud::Azure(None); + let ttd = TempTedgeDir::new(); + let loc = TEdgeConfigLocation::from_custom_root(ttd.path()); + loc.update_toml(&|dto, _| { + dto.try_update_str(&"c8y.url".parse().unwrap(), "latest.example.com") + .unwrap(); + dto.try_update_str(&"c8y@new.url".parse().unwrap(), "example.com") + .unwrap(); + Ok(()) + }) + .unwrap(); + let config = loc.load().unwrap(); + + validate_config(&config, &cloud).unwrap(); + } + + #[test] + fn allows_non_conflicting_topic_prefixes() { + let cloud = MaybeBorrowedCloud::Azure(None); + let ttd = TempTedgeDir::new(); + let loc = TEdgeConfigLocation::from_custom_root(ttd.path()); + loc.update_toml(&|dto, _| { + dto.try_update_str(&"az@new.bridge.topic_prefix".parse().unwrap(), "az-new") + .unwrap(); + Ok(()) + }) + .unwrap(); + let config = loc.load().unwrap(); + + validate_config(&config, &cloud).unwrap(); + } + } } From 8b27df84ad682b5ab70fcf966752e029e8fb536f Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Tue, 3 Dec 2024 17:49:21 +0000 Subject: [PATCH 02/17] Add ability to override profiled configurations with env vars Previously, environment variables weren't supported for profiled configurations due to limitations with the generated regex for matching the configuration keys. Additionally, the keys aren't naturally suited to environment variables, since they contain an `@`. With these changes, the key regex now supports both `c8y@name.url` and `c8y.profiles.name.url`, allowing the environment variable `TEDGE_C8Y_PROFILES_NAME_URL` to override a profiled configuration. Signed-off-by: James Rhodes --- Cargo.lock | 2 + .../src/tedge_config_cli/figment.rs | 98 ++++++++++++++++++- .../tedge_config_macros/impl/Cargo.toml | 2 + .../tedge_config_macros/impl/src/query.rs | 37 ++++++- 4 files changed, 135 insertions(+), 4 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index f0971715a00..7794e8217c4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4113,7 +4113,9 @@ dependencies = [ "prettyplease", "proc-macro2", "quote", + "regex", "syn 2.0.77", + "test-case", ] [[package]] diff --git a/crates/common/tedge_config/src/tedge_config_cli/figment.rs b/crates/common/tedge_config/src/tedge_config_cli/figment.rs index 9e5affcddc1..2b5e4f91545 100644 --- a/crates/common/tedge_config/src/tedge_config_cli/figment.rs +++ b/crates/common/tedge_config/src/tedge_config_cli/figment.rs @@ -216,7 +216,7 @@ impl TEdgeEnv { tracing::subscriber::NoSubscriber::default(), || lowercase_name.parse::(), ) - .map(|key| key.to_string()) + .map(|key| key.to_string().replace("@", ".profiles.")) .map_err(|err| { let is_read_only_key = matches!(err, crate::ParseKeyError::ReadOnly(_)); if is_read_only_key && !WARNINGS.lock().unwrap().insert(lowercase_name.clone()) { @@ -237,6 +237,7 @@ mod tests { use std::path::PathBuf; use serde::Deserialize; + use tedge_config_macros::define_tedge_config; use super::*; @@ -259,6 +260,7 @@ mod tests { Ok(()) }) } + #[test] fn environment_variables_override_config_file() { #[derive(Deserialize)] @@ -376,4 +378,98 @@ mod tests { Ok(()) }) } + + #[test] + fn environment_variables_can_override_profiled_configurations() { + use crate::AppendRemoveItem; + use crate::ReadError; + use tedge_config_macros::*; + + define_tedge_config!( + #[tedge_config(multi)] + c8y: { + url: String, + } + ); + + figment::Jail::expect_with(|jail| { + jail.create_file( + "tedge.toml", + r#" + [c8y.profiles.test] + url = "test.c8y.io" + "#, + )?; + + jail.set_env("TEDGE_C8Y_PROFILES_TEST_URL", "override.c8y.io"); + + let dto = + extract_data::(&PathBuf::from("tedge.toml")) + .unwrap() + .0; + assert_eq!( + dto.c8y.try_get(Some("test"), "c8y").unwrap().url.as_deref(), + Some("override.c8y.io") + ); + Ok(()) + }) + } + + #[test] + fn environment_variable_profile_warnings_use_key_with_correct_format() { + use crate::AppendRemoveItem; + use crate::ReadError; + use tedge_config_macros::*; + + define_tedge_config!( + #[tedge_config(multi)] + c8y: { + url: String, + } + ); + + figment::Jail::expect_with(|jail| { + jail.set_env("TEDGE_C8Y_PROFILES_TEST_UNKNOWN", "override.c8y.io"); + + let warnings = + extract_data::(&PathBuf::from("tedge.toml")) + .unwrap() + .1; + assert_eq!( + warnings.0, + ["Unknown configuration field \"c8y_profiles_test_unknown\" from environment variable TEDGE_C8Y_PROFILES_TEST_UNKNOWN"] + ); + Ok(()) + }) + } + + #[test] + fn toml_profile_warnings_use_key_with_correct_format() { + use crate::AppendRemoveItem; + use crate::ReadError; + use tedge_config_macros::*; + + define_tedge_config!( + #[tedge_config(multi)] + c8y: { + url: String, + } + ); + + figment::Jail::expect_with(|jail| { + jail.create_file( + "tedge.toml", + r#" + [c8y.profiles.test] + unknown = "test.c8y.io" + "#, + )?; + let warnings = + extract_data::(&PathBuf::from("tedge.toml")) + .unwrap() + .1; + assert!(dbg!(warnings.0.first().unwrap()).contains("c8y.profiles.test.unknown")); + Ok(()) + }) + } } diff --git a/crates/common/tedge_config_macros/impl/Cargo.toml b/crates/common/tedge_config_macros/impl/Cargo.toml index 36d37b8971a..4ce1514f90e 100644 --- a/crates/common/tedge_config_macros/impl/Cargo.toml +++ b/crates/common/tedge_config_macros/impl/Cargo.toml @@ -19,6 +19,8 @@ syn = { workspace = true } [dev-dependencies] pretty_assertions = { workspace = true } prettyplease = { workspace = true } +regex = { workspace = true } +test-case = { workspace = true } [lints] workspace = true diff --git a/crates/common/tedge_config_macros/impl/src/query.rs b/crates/common/tedge_config_macros/impl/src/query.rs index b4906606c53..34fbc55710b 100644 --- a/crates/common/tedge_config_macros/impl/src/query.rs +++ b/crates/common/tedge_config_macros/impl/src/query.rs @@ -782,12 +782,14 @@ fn enum_variant(segments: &VecDeque<&FieldOrGroup>) -> ConfigurationKey { let re = segments .iter() .map(|fog| match fog { - FieldOrGroup::Multi(m) => format!("{}(?:@([^\\.]+))?", m.ident), + FieldOrGroup::Multi(m) => { + format!("{}(?:(?:@|[\\._]profiles[\\._])([^\\.]+))?", m.ident) + } FieldOrGroup::Field(f) => f.ident().to_string(), FieldOrGroup::Group(g) => g.ident.to_string(), }) .collect::>() - .join("\\."); + .join("[\\._]"); let re = format!("^{re}$"); let regex_parser = parse_quote_spanned!(ident.span()=> if let Some(captures) = ::regex::Regex::new(#re).unwrap().captures(value) {}); let formatters = field_names @@ -882,6 +884,7 @@ fn is_read_write(path: &VecDeque<&FieldOrGroup>) -> bool { mod tests { use super::*; use syn::ItemImpl; + use test_case::test_case; #[test] fn output_parses() { @@ -937,6 +940,34 @@ mod tests { ); } + /// The regex generated for `c8y.url` + /// + /// This is used to verify both the output of the macro matches this regex + /// and that the regex itself functions as intended + const C8Y_URL_REGEX: &str = "^c8y(?:(?:@|[\\._]profiles[\\._])([^\\.]+))?[\\._]url$"; + + #[test_case("c8y.url", None; "with no profile specified")] + #[test_case("c8y@name.url", Some("name"); "with profile shorthand syntax")] + #[test_case("c8y@name_underscore.url", Some("name_underscore"); "with underscore profile shorthand syntax")] + #[test_case("c8y.profiles.name.url", Some("name"); "with profile toml syntax")] + #[test_case("c8y_profiles_name_url", Some("name"); "with environment variable profile")] + #[test_case("c8y_profiles_name_underscore_url", Some("name_underscore"); "with environment variable underscore profile")] + fn regex_matches(input: &str, output: Option<&str>) { + let re = regex::Regex::new(C8Y_URL_REGEX).unwrap(); + assert_eq!( + re.captures(input).unwrap().get(1).map(|s| s.as_str()), + output + ); + } + + #[test_case("not.c8y.url"; "with an invalid prefix")] + #[test_case("c8y.url.something"; "with an invalid suffix")] + #[test_case("c8y@multiple.profile.sections.url"; "with an invalid profile name")] + fn regex_fails(input: &str) { + let re = regex::Regex::new(C8Y_URL_REGEX).unwrap(); + assert!(re.captures(input).is_none()); + } + #[test] fn from_str_generates_regex_matches_for_multi_fields() { let input: crate::input::Configuration = parse_quote!( @@ -966,7 +997,7 @@ mod tests { }, _ => unimplemented!("just a test, no error handling"), }; - if let Some(captures) = ::regex::Regex::new("^c8y(?:@([^\\.]+))?\\.url$").unwrap().captures(value) { + if let Some(captures) = ::regex::Regex::new(#C8Y_URL_REGEX).unwrap().captures(value) { let key0 = captures.get(1usize).map(|re_match| re_match.as_str().to_owned()); return Ok(Self::C8yUrl(key0)); }; From f3e06c545a5e391195b537d90a97d7caed5ca424 Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Tue, 3 Dec 2024 17:53:16 +0000 Subject: [PATCH 03/17] Delete dead code Signed-off-by: James Rhodes --- .../tedge_config_macros/impl/src/reader.rs | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/crates/common/tedge_config_macros/impl/src/reader.rs b/crates/common/tedge_config_macros/impl/src/reader.rs index 231d454fc71..782cad20c8b 100644 --- a/crates/common/tedge_config_macros/impl/src/reader.rs +++ b/crates/common/tedge_config_macros/impl/src/reader.rs @@ -131,14 +131,6 @@ fn generate_structs( .map(|(name, ty, function, parent_ty, id)| -> syn::ItemImpl { if let Some((ok, err)) = extract_type_from_result(ty) { parse_quote_spanned! {name.span()=> - // impl #name { - // // TODO don't just guess we're called tedgeconfigreader - // #[deprecated] - // pub fn try_read(&self, reader: &#parent_ty) -> Result<&#ok, #err> { - // self.0.get_or_try_init(|| #function(reader)) - // } - // } - impl #parent_ty { pub fn #id(&self) -> Result<&#ok, #err> { self.#id.0.get_or_try_init(|| #function(self)) @@ -147,14 +139,6 @@ fn generate_structs( } } else { parse_quote_spanned! {name.span()=> - // impl #name { - // // TODO don't just guess we're called tedgeconfigreader - // #[deprecated] - // pub fn read(&self, reader: &#parent_ty) -> &#ty { - // self.0.get_or_init(|| #function(reader)) - // } - // } - impl #parent_ty { pub fn #id(&self) -> &#ty { self.#id.0.get_or_init(|| #function(self)) From 6525ff26bef28b0b7e0bfac3ca7ad226f10f2267 Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Tue, 3 Dec 2024 18:26:49 +0000 Subject: [PATCH 04/17] Clean up unnecessary code from profile operation test Signed-off-by: James Rhodes --- .../configuration_operation_multi_cloud.robot | 50 ++++++------------- 1 file changed, 15 insertions(+), 35 deletions(-) diff --git a/tests/RobotFramework/tests/cumulocity/configuration/configuration_operation_multi_cloud.robot b/tests/RobotFramework/tests/cumulocity/configuration/configuration_operation_multi_cloud.robot index d3f891f28c1..a68917e598f 100644 --- a/tests/RobotFramework/tests/cumulocity/configuration/configuration_operation_multi_cloud.robot +++ b/tests/RobotFramework/tests/cumulocity/configuration/configuration_operation_multi_cloud.robot @@ -5,26 +5,21 @@ Library ThinEdgeIO Library Cumulocity Suite Setup Suite Setup -Suite Teardown Get Logs name=${PARENT_SN} -Test Setup Test Setup +Suite Teardown Get Logs name=${DEVICE_SN} -Test Tags theme:configuration theme:childdevices +Test Tags theme:configuration *** Variables *** -${PARENT_SN} ${EMPTY} +${DEVICE_SN} ${EMPTY} ${SECOND_DEVICE_SN} ${EMPTY} -*** Test Cases *** DEVICE EXTERNALID CONFIG_TYPE DEVICE_FILE FILE PERMISSION OWNERSHIP -# -# Get configuration -# - +*** Test Cases *** Get Configuration from Main Device [Template] Get Configuration from Device - Text file topic_prefix=c8y external_id=${PARENT_SN} config_type=CONFIG1 device_file=/etc/config1.json - Binary file topic_prefix=c8y external_id=${PARENT_SN} config_type=CONFIG1_BINARY device_file=/etc/binary-config1.tar.gz + Text file topic_prefix=c8y external_id=${DEVICE_SN} config_type=CONFIG1 device_file=/etc/config1.json + Binary file topic_prefix=c8y external_id=${DEVICE_SN} config_type=CONFIG1_BINARY device_file=/etc/binary-config1.tar.gz Get Configuration from Second Device [Template] Get Configuration from Device @@ -61,16 +56,17 @@ Get Configuration from Device # Suite Setup - # Parent - ${parent_sn}= Setup skip_bootstrap=${False} - Set Suite Variable $PARENT_SN ${parent_sn} + # Original device + ${device_sn}= Setup skip_bootstrap=${False} + Set Suite Variable $DEVICE_SN ${device_sn} - # Child + Copy Configuration Files ${DEVICE_SN} + # "Profiled" device Setup Second Device Setup Second Device - ${child_sn}= Catenate SEPARATOR=_ ${parent_sn} second - Set Suite Variable $SECOND_DEVICE_SN ${child_sn} + ${second_device_sn}= Catenate SEPARATOR=_ ${device_sn} second + Set Suite Variable $SECOND_DEVICE_SN ${second_device_sn} Execute Command ... tedge config set c8y@second.device.cert_path /etc/tedge/device-certs/tedge@second-certificate.pem @@ -79,17 +75,13 @@ Setup Second Device Execute Command tedge config set c8y@second.bridge.topic_prefix c8y-second Execute Command tedge config set c8y@second.url "$(tedge config get c8y.url)" - Execute Command tedge cert create c8y@second --device-id ${child_sn} + Execute Command tedge cert create c8y@second --device-id ${second_device_sn} Execute Command ... cmd=sudo env C8Y_USER='${C8Y_CONFIG.username}' C8Y_PASSWORD='${C8Y_CONFIG.password}' tedge cert upload c8y@second Execute Command tedge connect c8y@second - RETURN ${child_sn} - -Test Setup - Customize Operation Workflows ${PARENT_SN} - Copy Configuration Files ${PARENT_SN} + RETURN ${second_device_sn} Copy Configuration Files [Arguments] ${device} @@ -101,15 +93,3 @@ Copy Configuration Files # make sure initial files have the same permissions on systems with different umasks Execute Command chmod 664 /etc/config1.json /etc/config2.json /etc/binary-config1.tar.gz - - # on a child device, user with uid 1000 doesn't exist, so make sure files we're testing on have a well defined user - Execute Command - ... chown root:root /etc/tedge/plugins/tedge-configuration-plugin.toml /etc/config1.json /etc/binary-config1.tar.gz - -Customize Operation Workflows - [Arguments] ${device} - ThinEdgeIO.Set Device Context ${device} - ThinEdgeIO.Transfer To Device ${CURDIR}/sub_config_snapshot.toml /etc/tedge/operations/ - ThinEdgeIO.Transfer To Device ${CURDIR}/sub_config_update.toml /etc/tedge/operations/ - Restart Service tedge-agent - ThinEdgeIO.Service Health Status Should Be Up tedge-agent From fd41260b1ae0b04b6c75ce4c316b213e9e737dc0 Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Mon, 9 Dec 2024 11:22:45 +0000 Subject: [PATCH 05/17] Remove @profile syntax from CLI in favour of --profile Signed-off-by: James Rhodes --- .../init/systemd/tedge-mapper-aws@.service | 2 +- .../init/systemd/tedge-mapper-az@.service | 2 +- .../init/systemd/tedge-mapper-c8y@.service | 2 +- .../src/tedge_config_cli/figment.rs | 2 +- .../tedge_config_macros/examples/multi.rs | 78 +++-------- .../tedge_config_macros/impl/src/query.rs | 131 +++++++++++++++++- .../common/tedge_config_macros/src/multi.rs | 16 ++- crates/core/tedge/src/cli/certificate/cli.rs | 114 ++++++++------- crates/core/tedge/src/cli/common.rs | 115 +++++++++++---- crates/core/tedge/src/cli/config/cli.rs | 64 ++++++++- crates/core/tedge/src/cli/connect/cli.rs | 10 +- crates/core/tedge/src/cli/connect/command.rs | 12 +- crates/core/tedge/src/cli/disconnect/cli.rs | 8 +- crates/core/tedge/src/cli/reconnect/cli.rs | 8 +- crates/core/tedge_mapper/src/lib.rs | 127 ++++++----------- plugins/c8y_firmware_plugin/src/lib.rs | 2 +- plugins/c8y_remote_access_plugin/src/input.rs | 2 +- .../configuration_operation_multi_cloud.robot | 19 +-- 18 files changed, 457 insertions(+), 257 deletions(-) diff --git a/configuration/init/systemd/tedge-mapper-aws@.service b/configuration/init/systemd/tedge-mapper-aws@.service index de038ab7845..8c493f2c371 100644 --- a/configuration/init/systemd/tedge-mapper-aws@.service +++ b/configuration/init/systemd/tedge-mapper-aws@.service @@ -5,7 +5,7 @@ After=syslog.target network.target mosquitto.service [Service] User=tedge ExecStartPre=+-/usr/bin/tedge init -ExecStart=/usr/bin/tedge-mapper aws@%i +ExecStart=/usr/bin/tedge-mapper aws --profile %i Restart=on-failure RestartPreventExitStatus=255 RestartSec=5 diff --git a/configuration/init/systemd/tedge-mapper-az@.service b/configuration/init/systemd/tedge-mapper-az@.service index 10085b9e35d..578a80c168d 100644 --- a/configuration/init/systemd/tedge-mapper-az@.service +++ b/configuration/init/systemd/tedge-mapper-az@.service @@ -5,7 +5,7 @@ After=syslog.target network.target mosquitto.service [Service] User=tedge ExecStartPre=+-/usr/bin/tedge init -ExecStart=/usr/bin/tedge-mapper az@%i +ExecStart=/usr/bin/tedge-mapper az --profile %i Restart=on-failure RestartPreventExitStatus=255 RestartSec=5 diff --git a/configuration/init/systemd/tedge-mapper-c8y@.service b/configuration/init/systemd/tedge-mapper-c8y@.service index f063b184d9e..8f332dbf14e 100644 --- a/configuration/init/systemd/tedge-mapper-c8y@.service +++ b/configuration/init/systemd/tedge-mapper-c8y@.service @@ -5,7 +5,7 @@ After=syslog.target network.target mosquitto.service [Service] User=tedge ExecStartPre=+-/usr/bin/tedge init -ExecStart=/usr/bin/tedge-mapper c8y@%i +ExecStart=/usr/bin/tedge-mapper c8y --profile %i Restart=on-failure RestartPreventExitStatus=255 RestartSec=5 diff --git a/crates/common/tedge_config/src/tedge_config_cli/figment.rs b/crates/common/tedge_config/src/tedge_config_cli/figment.rs index 2b5e4f91545..05f3f00c2c5 100644 --- a/crates/common/tedge_config/src/tedge_config_cli/figment.rs +++ b/crates/common/tedge_config/src/tedge_config_cli/figment.rs @@ -209,7 +209,7 @@ impl TEdgeEnv { fn provider(&self) -> figment::providers::Env { static WARNINGS: Lazy>> = Lazy::new(<_>::default); - figment::providers::Env::prefixed(self.prefix).ignore(&["CONFIG_DIR"]).map(move |name| { + figment::providers::Env::prefixed(self.prefix).ignore(&["CONFIG_DIR", "CLOUD_PROFILE"]).map(move |name| { let lowercase_name = name.as_str().to_ascii_lowercase(); Uncased::new( tracing::subscriber::with_default( diff --git a/crates/common/tedge_config_macros/examples/multi.rs b/crates/common/tedge_config_macros/examples/multi.rs index 7bf44662030..ffdd2341376 100644 --- a/crates/common/tedge_config_macros/examples/multi.rs +++ b/crates/common/tedge_config_macros/examples/multi.rs @@ -45,11 +45,6 @@ define_tedge_config! { #[tedge_config(example = "true", default(value = true))] use_operation_id: bool, }, - - #[tedge_config(multi)] - something: { - test: String, - } }, } @@ -64,83 +59,53 @@ fn url_for<'a>(reader: &'a TEdgeConfigReader, o: Option<&str>) -> &'a str { } fn main() { - let single_c8y_toml = "c8y.url = \"https://example.com\""; - let single_c8y_dto = toml::from_str(single_c8y_toml).unwrap(); - let single_c8y_reader = TEdgeConfigReader::from_dto(&single_c8y_dto, &TEdgeConfigLocation); - assert_eq!(url_for(&single_c8y_reader, None), "https://example.com"); - - let multi_c8y_toml = "c8y.cloud.url = \"https://cloud.example.com\"\nc8y.edge.url = \"https://edge.example.com\""; - let multi_c8y_dto = toml::from_str(multi_c8y_toml).unwrap(); - let multi_c8y_reader = TEdgeConfigReader::from_dto(&multi_c8y_dto, &TEdgeConfigLocation); + let c8y_toml = "c8y.url = \"https://example.com\"\nc8y.profiles.cloud.url = \"https://cloud.example.com\"\nc8y.profiles.edge.url = \"https://edge.example.com\""; + let c8y_dto = toml::from_str(c8y_toml).unwrap(); + let c8y_reader = TEdgeConfigReader::from_dto(&c8y_dto, &TEdgeConfigLocation); assert_eq!( - url_for(&multi_c8y_reader, Some("cloud")), + url_for(&c8y_reader, Some("cloud")), "https://cloud.example.com" ); assert_eq!( - url_for(&multi_c8y_reader, Some("edge")), + url_for(&c8y_reader, Some("edge")), "https://edge.example.com" ); assert_eq!( - single_c8y_reader - .c8y - .try_get(Some("cloud")) - .unwrap_err() - .to_string(), - "You are trying to access a profile `cloud` of c8y, but profiles are not enabled for c8y" - ); - assert_eq!( - multi_c8y_reader + c8y_reader .c8y .try_get(Some("unknown")) .unwrap_err() .to_string(), "Unknown profile `unknown` for the multi-profile property c8y" ); - assert_eq!( - multi_c8y_reader - .c8y - .try_get::<&str>(None) - .unwrap_err() - .to_string(), - "A profile is required for the multi-profile property c8y" - ); + assert_eq!(url_for(&c8y_reader, None), "https://example.com",); assert_eq!( "c8y.url".parse::().unwrap(), ReadableKey::C8yUrl(None) ); assert_eq!( - "c8y.cloud.url".parse::().unwrap(), + "c8y@cloud.url".parse::().unwrap(), ReadableKey::C8yUrl(Some("cloud".to_owned())) ); assert_eq!( - "c8y.cloud.something.test".parse::().unwrap(), - ReadableKey::C8ySomethingTest(Some("cloud".to_owned()), None) - ); - assert_eq!( - "c8y.cloud.something.thing.test" - .parse::() - .unwrap(), - ReadableKey::C8ySomethingTest(Some("cloud".to_owned()), Some("thing".to_owned())) - ); - assert_eq!( - "c8y.something.thing.test".parse::().unwrap(), - ReadableKey::C8ySomethingTest(None, Some("thing".to_owned())) + "c8y.profiles.cloud.url".parse::().unwrap(), + ReadableKey::C8yUrl(Some("cloud".to_owned())) ); assert_eq!( - "c8y.cloud.not_a_real_key" + "c8y@cloud.not_a_real_key" .parse::() .unwrap_err() .to_string(), - "Unknown key: 'c8y.cloud.not_a_real_key'" + "Unknown key: 'c8y@cloud.not_a_real_key'" ); assert_eq!( "c8y.urll".parse::().unwrap_err().to_string(), "Unknown key: 'c8y.urll'" ); - let mut keys = multi_c8y_reader + let mut keys = c8y_reader .readable_keys() .map(|r| r.to_string()) .collect::>(); @@ -149,14 +114,15 @@ fn main() { assert_eq!( keys, [ - "c8y.cloud.http", - "c8y.cloud.smartrest.use_operation_id", - "c8y.cloud.something.test", - "c8y.cloud.url", - "c8y.edge.http", - "c8y.edge.smartrest.use_operation_id", - "c8y.edge.something.test", - "c8y.edge.url" + "c8y.http", + "c8y.smartrest.use_operation_id", + "c8y.url", + "c8y@cloud.http", + "c8y@cloud.smartrest.use_operation_id", + "c8y@cloud.url", + "c8y@edge.http", + "c8y@edge.smartrest.use_operation_id", + "c8y@edge.url" ] ); } diff --git a/crates/common/tedge_config_macros/impl/src/query.rs b/crates/common/tedge_config_macros/impl/src/query.rs index 34fbc55710b..39f3b77f052 100644 --- a/crates/common/tedge_config_macros/impl/src/query.rs +++ b/crates/common/tedge_config_macros/impl/src/query.rs @@ -482,6 +482,43 @@ fn keys_enum( .is_empty() .then_some::(parse_quote!(_ => unimplemented!("Cope with empty enum"))); + let (wp_match, wp_ret): (Vec<_>, Vec<_>) = configuration_key + .iter() + .flat_map(|k| k.insert_profiles.clone()) + .unzip(); + + let max_profile_count = configuration_key.iter().map(|k| k.field_names.len()).max(); + + let try_with_profile_impl = match max_profile_count { + Some(1) => quote! { + pub fn try_with_profile(self, profile: ProfileName) -> ::anyhow::Result { + match self { + #( + #wp_match => #wp_ret, + )* + other => { + ::anyhow::bail!("You've supplied a profile, but {other} is not a profiled configuration") + }, + } + } + }, + + // If no profiles, just don't implement the method + Some(0) | None => quote! {}, + + // If profiles are nested, we need to rethink this method entirely + // This likely won't ever be needed, but it's good to have a clear error message if someone does stumble across it + Some(2..) => { + let error_loc = format!("{}:{}", file!(), line!() + 1); + let error = format!("try_with_profile cannot be implemented (in its current form) for nested profiles. You'll need to modify the code at {error_loc} to support this."); + quote! { + pub fn try_with_profile(self, profile: ProfileName) -> ::anyhow::Result { + compile_error!(#error) + } + } + } + }; + quote! { #[derive(Clone, Debug, PartialEq, Eq)] #[non_exhaustive] @@ -509,6 +546,8 @@ fn keys_enum( #uninhabited_catch_all } } + + #try_with_profile_impl } impl ::std::fmt::Display for #type_name { @@ -744,6 +783,8 @@ struct ConfigurationKey { /// ] /// ``` formatters: Vec<(syn::Pat, syn::Expr)>, + + insert_profiles: Vec<(syn::Pat, syn::Expr)>, } fn ident_for(segments: &VecDeque<&FieldOrGroup>) -> syn::Ident { @@ -829,6 +870,27 @@ fn enum_variant(segments: &VecDeque<&FieldOrGroup>) -> ConfigurationKey { } }) .collect(); + let insert_profiles = field_names + .iter() + .map(|_| [parse_quote!(None), parse_quote!(Some(_))]) + .multi_cartesian_product() + .enumerate() + .map(|(i, options): (_, Vec)| { + if i == 0 { + ( + parse_quote!(Self::#ident(#(#options),*)), + parse_quote!(Ok(Self::#ident(Some(profile.into())))), + ) + } else { + ( + parse_quote!(c @ Self::#ident(#(#options),*)), + parse_quote!(::anyhow::bail!( + "Multiple profiles selected from the arguments {c} and --profile {profile}" + )), + ) + } + }) + .collect(); ConfigurationKey { enum_variant, iter_field, @@ -837,6 +899,7 @@ fn enum_variant(segments: &VecDeque<&FieldOrGroup>) -> ConfigurationKey { regex_parser: Some(regex_parser), field_names, formatters, + insert_profiles, } } else { ConfigurationKey { @@ -850,6 +913,7 @@ fn enum_variant(segments: &VecDeque<&FieldOrGroup>) -> ConfigurationKey { parse_quote!(#ident), parse_quote!(::std::borrow::Cow::Borrowed(#key_str)), )], + insert_profiles: vec![], } } } @@ -883,6 +947,7 @@ fn is_read_write(path: &VecDeque<&FieldOrGroup>) -> bool { #[cfg(test)] mod tests { use super::*; + use syn::ImplItem; use syn::ItemImpl; use test_case::test_case; @@ -1206,7 +1271,7 @@ mod tests { ); let paths = configuration_paths_from(&input.groups); let config_keys = configuration_strings(paths.iter()); - let impl_block = keys_enum_impl_block(&config_keys); + let impl_block = retain_fn(keys_enum_impl_block(&config_keys), "to_cow_str"); let expected = parse_quote! { impl ReadableKey { @@ -1234,7 +1299,7 @@ mod tests { ); let paths = configuration_paths_from(&input.groups); let config_keys = configuration_strings(paths.iter()); - let impl_block = keys_enum_impl_block(&config_keys); + let impl_block = retain_fn(keys_enum_impl_block(&config_keys), "to_cow_str"); let expected = parse_quote! { impl ReadableKey { @@ -1266,7 +1331,7 @@ mod tests { ); let paths = configuration_paths_from(&input.groups); let config_keys = configuration_strings(paths.iter()); - let impl_block = keys_enum_impl_block(&config_keys); + let impl_block = retain_fn(keys_enum_impl_block(&config_keys), "to_cow_str"); let expected = parse_quote! { impl ReadableKey { @@ -1287,6 +1352,46 @@ mod tests { ); } + #[test] + fn impl_try_with_profile() { + let input: crate::input::Configuration = parse_quote!( + #[tedge_config(multi)] + c8y: { + url: String, + + availability: { + interval: i32, + } + }, + + sudo: { + enable: bool, + }, + ); + let paths = configuration_paths_from(&input.groups); + let config_keys = configuration_strings(paths.iter()); + let impl_block = retain_fn(keys_enum_impl_block(&config_keys), "try_with_profile"); + + let expected = parse_quote! { + impl ReadableKey { + pub fn try_with_profile(self, profile: ProfileName) -> ::anyhow::Result { + match self { + Self::C8yUrl(None) => Ok(Self::C8yUrl(Some(profile.into()))), + c @ Self::C8yUrl(Some(_)) => ::anyhow::bail!("Multiple profiles selected from the arguments {c} and --profile {profile}"), + Self::C8yAvailabilityInterval(None) => Ok(Self::C8yAvailabilityInterval(Some(profile.into()))), + c @ Self::C8yAvailabilityInterval(Some(_)) => ::anyhow::bail!("Multiple profiles selected from the arguments {c} and --profile {profile}"), + other => ::anyhow::bail!("You've supplied a profile, but {other} is not a profiled configuration"), + } + } + } + }; + + pretty_assertions::assert_eq!( + prettyplease::unparse(&parse_quote!(#impl_block)), + prettyplease::unparse(&expected) + ); + } + fn keys_enum_impl_block(config_keys: &(Vec, Vec)) -> ItemImpl { let generated = keys_enum(parse_quote!(ReadableKey), config_keys, "DOC FRAGMENT"); let generated_file: syn::File = syn::parse2(generated).unwrap(); @@ -1311,4 +1416,24 @@ mod tests { impl_block } + + fn retain_fn(mut impl_block: ItemImpl, fn_name: &str) -> ItemImpl { + let ident = syn::Ident::new(fn_name, Span::call_site()); + let all_fn_names: Vec<_> = impl_block + .items + .iter() + .filter_map(|i| match i { + ImplItem::Fn(f) => Some(f.sig.ident.clone()), + _ => None, + }) + .collect(); + impl_block + .items + .retain(|i| matches!(i, ImplItem::Fn(f) if f.sig.ident == ident)); + assert!( + !impl_block.items.is_empty(), + "{ident:?} did not appear in methods. The valid method names are {all_fn_names:?}" + ); + impl_block + } } diff --git a/crates/common/tedge_config_macros/src/multi.rs b/crates/common/tedge_config_macros/src/multi.rs index eb4100394b2..1c381ac0991 100644 --- a/crates/common/tedge_config_macros/src/multi.rs +++ b/crates/common/tedge_config_macros/src/multi.rs @@ -30,8 +30,7 @@ pub struct ProfileName(String); impl TryFrom for ProfileName { type Error = anyhow::Error; fn try_from(value: String) -> Result { - validate_profile_name(&value)?; - Ok(Self(value)) + value.parse() } } @@ -54,6 +53,11 @@ fn validate_profile_name(value: &str) -> Result<(), anyhow::Error> { .all(|c| c.is_alphanumeric() || ['-', '_'].contains(&c)), "Profile names can only contain letters, numbers, `-` or `_`" ); + ensure!(!value.is_empty(), "Profile names cannot be empty"); + ensure!( + value.chars().any(|c| c.is_alphanumeric()), + "Profile names must contain at least one letter or number" + ); Ok(()) } @@ -61,7 +65,7 @@ impl FromStr for ProfileName { type Err = anyhow::Error; fn from_str(s: &str) -> Result { validate_profile_name(s)?; - Ok(Self(s.to_owned())) + Ok(Self(s.to_lowercase())) } } @@ -113,6 +117,12 @@ impl Deref for ProfileName { } } +impl From for String { + fn from(value: ProfileName) -> Self { + value.0 + } +} + #[derive(Debug, thiserror::Error)] pub enum MultiError { #[error( diff --git a/crates/core/tedge/src/cli/certificate/cli.rs b/crates/core/tedge/src/cli/certificate/cli.rs index c384d6d7560..7a1f3b21672 100644 --- a/crates/core/tedge/src/cli/certificate/cli.rs +++ b/crates/core/tedge/src/cli/certificate/cli.rs @@ -1,5 +1,5 @@ use crate::cli::common::Cloud; -use anyhow::anyhow; +use crate::cli::common::OptionalCloudArgs; use camino::Utf8PathBuf; use tedge_config::OptionalConfigError; use tedge_config::ProfileName; @@ -24,7 +24,8 @@ pub enum TEdgeCertCli { #[clap(long = "device-id")] id: String, - cloud: Option, + #[clap(flatten)] + cloud: OptionalCloudArgs, }, /// Create a certificate signing request @@ -37,19 +38,30 @@ pub enum TEdgeCertCli { #[clap(long = "output-path")] output_path: Option, - cloud: Option, + #[clap(flatten)] + cloud: OptionalCloudArgs, }, /// Renew the device certificate - Renew { cloud: Option }, + Renew { + #[clap(flatten)] + cloud: OptionalCloudArgs, + }, /// Show the device certificate, if any - Show { cloud: Option }, + Show { + #[clap(flatten)] + cloud: OptionalCloudArgs, + }, /// Remove the device certificate - Remove { cloud: Option }, + Remove { + #[clap(flatten)] + cloud: OptionalCloudArgs, + }, /// Upload root certificate + #[clap(subcommand)] Upload(UploadCertCli), } @@ -64,6 +76,7 @@ impl BuildCommand for TEdgeCertCli { let cmd = match self { TEdgeCertCli::Create { id, cloud } => { + let cloud: Option = cloud.try_into()?; let cmd = CreateCertCmd { id, cert_path: config.device_cert_path(cloud.as_ref())?.to_owned(), @@ -79,6 +92,7 @@ impl BuildCommand for TEdgeCertCli { output_path, cloud, } => { + let cloud: Option = cloud.try_into()?; // Use the current device id if no id is provided let id = match id { Some(id) => id, @@ -96,6 +110,7 @@ impl BuildCommand for TEdgeCertCli { } TEdgeCertCli::Show { cloud } => { + let cloud: Option = cloud.try_into()?; let cmd = ShowCertCmd { cert_path: config.device_cert_path(cloud.as_ref())?.to_owned(), }; @@ -103,6 +118,7 @@ impl BuildCommand for TEdgeCertCli { } TEdgeCertCli::Remove { cloud } => { + let cloud: Option = cloud.try_into()?; let cmd = RemoveCertCmd { cert_path: config.device_cert_path(cloud.as_ref())?.to_owned(), key_path: config.device_key_path(cloud.as_ref())?.to_owned(), @@ -110,29 +126,24 @@ impl BuildCommand for TEdgeCertCli { cmd.into_boxed() } - TEdgeCertCli::Upload(cmd) => { - let cmd = match cmd.cloud { - Cloud::C8y(profile) => { - let c8y = config.c8y.try_get(profile.as_deref())?; - UploadCertCmd { - device_id: c8y.device.id()?.clone(), - path: c8y.device.cert_path.clone(), - host: c8y.http.or_err()?.to_owned(), - cloud_root_certs: config.cloud_root_certs(), - username: cmd.username, - password: cmd.password, - } - } - cloud => { - return Err(anyhow!( - "Uploading certificates via the tedge cli isn't supported for {cloud}" - ) - .into()) - } + TEdgeCertCli::Upload(UploadCertCli::C8y { + username, + password, + profile, + }) => { + let c8y = config.c8y.try_get(profile.as_deref())?; + let cmd = UploadCertCmd { + device_id: c8y.device.id()?.clone(), + path: c8y.device.cert_path.clone(), + host: c8y.http.or_err()?.to_owned(), + cloud_root_certs: config.cloud_root_certs(), + username, + password, }; cmd.into_boxed() } TEdgeCertCli::Renew { cloud } => { + let cloud: Option = cloud.try_into()?; let cmd = RenewCertCmd { cert_path: config.device_cert_path(cloud.as_ref())?.to_owned(), key_path: config.device_key_path(cloud.as_ref())?.to_owned(), @@ -144,29 +155,34 @@ impl BuildCommand for TEdgeCertCli { } } -#[derive(clap::Args, Debug)] -pub struct UploadCertCli { - cloud: Cloud, - #[clap(long = "user")] - #[arg( - env = "C8Y_USER", - hide_env_values = true, - hide_default_value = true, - default_value = "" - )] - /// Provided username should be a Cumulocity IoT user with tenant management permissions. - /// You will be prompted for input if the value is not provided or is empty - username: String, - - #[clap(long = "password")] - #[arg(env = "C8Y_PASSWORD", hide_env_values = true, hide_default_value = true, default_value_t = std::env::var("C8YPASS").unwrap_or_default().to_string())] - // Note: Prefer C8Y_PASSWORD over the now deprecated C8YPASS env variable as the former is also supported by other tooling such as go-c8y-cli - /// Cumulocity IoT Password. - /// You will be prompted for input if the value is not provided or is empty +#[derive(clap::Subcommand, Debug)] +pub enum UploadCertCli { + /// Upload root certificate to Cumulocity /// - /// Notes: `C8YPASS` is deprecated. Please use the `C8Y_PASSWORD` env variable instead - password: String, - - #[clap(long, hide = true)] - profile: Option, + /// The command will upload root certificate to Cumulocity. + C8y { + #[clap(long = "user")] + #[arg( + env = "C8Y_USER", + hide_env_values = true, + hide_default_value = true, + default_value = "" + )] + /// Provided username should be a Cumulocity IoT user with tenant management permissions. + /// You will be prompted for input if the value is not provided or is empty + username: String, + + #[clap(long = "password")] + #[arg(env = "C8Y_PASSWORD", hide_env_values = true, hide_default_value = true, default_value_t = std::env::var("C8YPASS").unwrap_or_default().to_string())] + // Note: Prefer C8Y_PASSWORD over the now deprecated C8YPASS env variable as the former is also supported by other tooling such as go-c8y-cli + /// Cumulocity IoT Password. + /// You will be prompted for input if the value is not provided or is empty + /// + /// Notes: `C8YPASS` is deprecated. Please use the `C8Y_PASSWORD` env variable instead + password: String, + + /// The cloud profile you wish to upload the certificate to + #[clap(long)] + profile: Option, + }, } diff --git a/crates/core/tedge/src/cli/common.rs b/crates/core/tedge/src/cli/common.rs index 5f3e7a3c838..d607e97c6ab 100644 --- a/crates/core/tedge/src/cli/common.rs +++ b/crates/core/tedge/src/cli/common.rs @@ -1,43 +1,86 @@ -use anyhow::bail; +use anyhow::Context; use std::borrow::Cow; -use std::str::FromStr; +use std::fmt; use tedge_config::system_services::SystemService; use tedge_config::ProfileName; -use yansi::Paint; -pub type Cloud = MaybeBorrowedCloud<'static>; +#[derive(clap::Args, PartialEq, Eq, Debug, Clone)] +pub struct CloudArgs { + /// The cloud you wish to interact with + cloud: CloudType, -impl FromStr for Cloud { - type Err = ::Err; - - fn from_str(s: &str) -> Result { - match (s, s.split_once("@")) { - (_, Some(("c8y", profile))) => Ok(Self::c8y(Some(profile.parse()?))), - ("c8y", None) => Ok(Self::c8y(None)), - (_, Some(("az", profile))) => Ok(Self::az(Some(profile.parse()?))), - ("az", None) => Ok(Self::Azure(None)), - (_, Some(("aws", profile))) => Ok(Self::aws(Some(profile.parse()?))), - ("aws", None) => Ok(Self::Aws(None)), - (cloud, _) => bail!( - "Unknown cloud type {cloud:?}. Valid cloud types are {}.", - valid_cloud_types() - ), - } + /// The cloud profile you wish to use, if not specified as part of the cloud + #[clap(long)] + profile: Option, +} + +#[derive(clap::Args, PartialEq, Eq, Debug, Clone)] +pub struct OptionalCloudArgs { + /// The cloud you wish to interact with + cloud: Option, + + /// The cloud profile you wish to use, if not specified as part of the cloud + #[clap(long)] + profile: Option, +} + +#[derive(clap::ValueEnum, Debug, Copy, Clone, PartialEq, Eq)] +#[clap(rename_all = "snake_case")] +enum CloudType { + C8y, + Az, + Aws, +} + +impl TryFrom for Cloud { + type Error = anyhow::Error; + + fn try_from(args: CloudArgs) -> Result { + args.cloud.try_with_profile_and_env(args.profile) + } +} + +impl TryFrom for Option { + type Error = anyhow::Error; + + fn try_from(args: OptionalCloudArgs) -> Result { + args.cloud + .map(|cloud| cloud.try_with_profile_and_env(args.profile)) + .transpose() } } -pub fn valid_cloud_types() -> String { - format!( - "{}, {}, {}", - "c8y".yellow().bold(), - "az".yellow().bold(), - "aws".yellow().bold() - ) +impl CloudType { + pub fn try_with_profile_and_env(self, profile: Option) -> anyhow::Result { + let env = "TEDGE_CLOUD_PROFILE"; + + match profile { + Some(profile) => Ok(self.with_profile(Some(profile))), + None => match std::env::var(env).as_deref() { + Ok("") => Ok(self.with_profile(None)), + Ok(e) => Ok(self.with_profile(Some(e.parse().with_context(|| { + format!("Parsing profile from environment variable {env}={e:?}") + })?))), + _ => Ok(self.with_profile(None)), + }, + } + } + + fn with_profile(self, profile: Option) -> Cloud { + let profile = profile.map(Cow::Owned); + match self { + Self::Aws => Cloud::Aws(profile), + Self::Az => Cloud::Azure(profile), + Self::C8y => Cloud::C8y(profile), + } + } } +pub type Cloud = MaybeBorrowedCloud<'static>; + pub type CloudBorrow<'a> = MaybeBorrowedCloud<'a>; -#[derive(Clone, Debug, strum_macros::Display, strum_macros::IntoStaticStr, PartialEq, Eq)] +#[derive(Clone, Debug, strum_macros::IntoStaticStr, PartialEq, Eq)] pub enum MaybeBorrowedCloud<'a> { #[strum(serialize = "Cumulocity")] C8y(Option>), @@ -45,6 +88,20 @@ pub enum MaybeBorrowedCloud<'a> { Aws(Option>), } +impl fmt::Display for MaybeBorrowedCloud<'_> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!( + f, + "{}", + match self { + Self::C8y(_) => "Cumulocity", + Self::Azure(_) => "Azure", + Self::Aws(_) => "Aws", + } + ) + } +} + impl<'a> From<&'a MaybeBorrowedCloud<'a>> for tedge_config::Cloud<'a> { fn from(value: &'a MaybeBorrowedCloud<'a>) -> tedge_config::Cloud<'a> { match value { @@ -59,9 +116,11 @@ impl Cloud { pub fn c8y(profile: Option) -> Self { Self::C8y(profile.map(Cow::Owned)) } + pub fn az(profile: Option) -> Self { Self::Azure(profile.map(Cow::Owned)) } + pub fn aws(profile: Option) -> Self { Self::Aws(profile.map(Cow::Owned)) } diff --git a/crates/core/tedge/src/cli/config/cli.rs b/crates/core/tedge/src/cli/config/cli.rs index e0a1cd0aa49..50024a7a0f5 100644 --- a/crates/core/tedge/src/cli/config/cli.rs +++ b/crates/core/tedge/src/cli/config/cli.rs @@ -1,6 +1,7 @@ use crate::cli::config::commands::*; use crate::command::*; use crate::ConfigError; +use tedge_config::ProfileName; use tedge_config::ReadableKey; use tedge_config::WritableKey; @@ -10,6 +11,10 @@ pub enum ConfigCmd { Get { /// Configuration key. Run `tedge config list --doc` for available keys key: ReadableKey, + + /// Cloud profile + #[clap(long)] + profile: Option, }, /// Set or update the provided configuration key with the given value @@ -19,12 +24,20 @@ pub enum ConfigCmd { /// Configuration value. value: String, + + /// Cloud profile + #[clap(long)] + profile: Option, }, /// Unset the provided configuration key Unset { /// Configuration key. Run `tedge config list --doc` for available keys key: WritableKey, + + /// Cloud profile + #[clap(long)] + profile: Option, }, /// Append or set the provided configuration key with the given value @@ -34,6 +47,10 @@ pub enum ConfigCmd { /// Configuration value. value: String, + + /// Cloud profile + #[clap(long)] + profile: Option, }, /// Remove value from the provided configuration key @@ -43,6 +60,10 @@ pub enum ConfigCmd { /// Configuration value. value: String, + + #[clap(long)] + /// Cloud profile + profile: Option, }, /// Print the configuration keys and their values @@ -60,36 +81,65 @@ pub enum ConfigCmd { }, } +macro_rules! try_with_profile { + ($key:ident, $profile:ident) => {{ + use anyhow::Context; + let profiled_key = match $profile { + None => $key, + Some(profile) => $key.try_with_profile(profile)?, + }; + match std::env::var("TEDGE_CLOUD_PROFILE").as_deref() { + Ok("") | Err(_) => profiled_key, + Ok(value) => profiled_key + .clone() + .try_with_profile(value.parse().context("Parsing TEDGE_CLOUD_PROFILE")?) + .unwrap_or(profiled_key), + } + }}; +} + impl BuildCommand for ConfigCmd { fn build_command(self, context: BuildContext) -> Result, ConfigError> { let config_location = context.config_location; match self { - ConfigCmd::Get { key } => Ok(GetConfigCommand { - key, + ConfigCmd::Get { key, profile } => Ok(GetConfigCommand { + key: try_with_profile!(key, profile), config: config_location.load()?, } .into_boxed()), - ConfigCmd::Set { key, value } => Ok(SetConfigCommand { + ConfigCmd::Set { key, value, + profile, + } => Ok(SetConfigCommand { + key: try_with_profile!(key, profile), + value, config_location, } .into_boxed()), - ConfigCmd::Unset { key } => Ok(UnsetConfigCommand { - key, + ConfigCmd::Unset { key, profile } => Ok(UnsetConfigCommand { + key: try_with_profile!(key, profile), config_location, } .into_boxed()), - ConfigCmd::Add { key, value } => Ok(AddConfigCommand { + ConfigCmd::Add { key, value, + profile, + } => Ok(AddConfigCommand { + key: try_with_profile!(key, profile), + value, config_location, } .into_boxed()), - ConfigCmd::Remove { key, value } => Ok(RemoveConfigCommand { + ConfigCmd::Remove { key, value, + profile, + } => Ok(RemoveConfigCommand { + key: try_with_profile!(key, profile), + value, config_location, } .into_boxed()), diff --git a/crates/core/tedge/src/cli/connect/cli.rs b/crates/core/tedge/src/cli/connect/cli.rs index 8f23b5aaaf0..4ee83b28d94 100644 --- a/crates/core/tedge/src/cli/connect/cli.rs +++ b/crates/core/tedge/src/cli/connect/cli.rs @@ -1,4 +1,4 @@ -use crate::cli::common::Cloud; +use crate::cli::common::CloudArgs; use crate::cli::connect::*; use crate::command::BuildCommand; use crate::command::BuildContext; @@ -7,9 +7,6 @@ use tedge_config::system_services::service_manager; #[derive(clap::Args, Debug, Eq, PartialEq)] pub struct TEdgeConnectOpt { - /// The cloud you wish to connect to, e.g. `c8y`, `az`, or `aws` - cloud: Cloud, - /// Test an existing connection #[clap(long = "test")] is_test_connection: bool, @@ -17,6 +14,9 @@ pub struct TEdgeConnectOpt { /// Ignore connection registration and connection check #[clap(long = "offline")] offline_mode: bool, + + #[clap(flatten)] + cloud: CloudArgs, } impl BuildCommand for TEdgeConnectOpt { @@ -29,7 +29,7 @@ impl BuildCommand for TEdgeConnectOpt { Ok(Box::new(ConnectCommand { config_location: context.config_location.clone(), config: context.load_config()?, - cloud, + cloud: cloud.try_into()?, is_test_connection, offline_mode, service_manager: service_manager(&context.config_location.tedge_config_root_path)?, diff --git a/crates/core/tedge/src/cli/connect/command.rs b/crates/core/tedge/src/cli/connect/command.rs index 71057e8d769..40d9ff39334 100644 --- a/crates/core/tedge/src/cli/connect/command.rs +++ b/crates/core/tedge/src/cli/connect/command.rs @@ -287,7 +287,9 @@ fn validate_config(config: &TEdgeConfig, cloud: &MaybeBorrowedCloud<'_>) -> anyh .keys() .map(|s| Some(s?.to_string())) .collect::>(); - disallow_matching_configurations(config, ReadableKey::AwsUrl, &profiles)?; + disallow_matching_configurations(config, ReadableKey::AwsUrl, &profiles).or_else( + |_| disallow_matching_configurations(config, ReadableKey::AwsDeviceId, &profiles), + )?; disallow_matching_configurations(config, ReadableKey::AwsBridgeTopicPrefix, &profiles)?; } MaybeBorrowedCloud::Azure(_) => { @@ -296,7 +298,9 @@ fn validate_config(config: &TEdgeConfig, cloud: &MaybeBorrowedCloud<'_>) -> anyh .keys() .map(|s| Some(s?.to_string())) .collect::>(); - disallow_matching_configurations(config, ReadableKey::AzUrl, &profiles)?; + disallow_matching_configurations(config, ReadableKey::AzUrl, &profiles).or_else( + |_| disallow_matching_configurations(config, ReadableKey::AzDeviceId, &profiles), + )?; disallow_matching_configurations(config, ReadableKey::AzBridgeTopicPrefix, &profiles)?; } MaybeBorrowedCloud::C8y(_) => { @@ -305,7 +309,9 @@ fn validate_config(config: &TEdgeConfig, cloud: &MaybeBorrowedCloud<'_>) -> anyh .keys() .map(|s| Some(s?.to_string())) .collect::>(); - disallow_matching_configurations(config, ReadableKey::C8yUrl, &profiles)?; + disallow_matching_configurations(config, ReadableKey::C8yUrl, &profiles).or_else( + |_| disallow_matching_configurations(config, ReadableKey::C8yDeviceId, &profiles), + )?; disallow_matching_configurations(config, ReadableKey::C8yBridgeTopicPrefix, &profiles)?; disallow_matching_configurations(config, ReadableKey::C8yProxyBindPort, &profiles)?; } diff --git a/crates/core/tedge/src/cli/disconnect/cli.rs b/crates/core/tedge/src/cli/disconnect/cli.rs index 2a72b59c268..5029efb264d 100644 --- a/crates/core/tedge/src/cli/disconnect/cli.rs +++ b/crates/core/tedge/src/cli/disconnect/cli.rs @@ -1,19 +1,19 @@ -use crate::cli::common::Cloud; +use crate::cli::common::CloudArgs; use crate::cli::disconnect::disconnect_bridge::*; use crate::command::*; use tedge_config::system_services::service_manager; #[derive(clap::Args, Debug)] pub struct TEdgeDisconnectBridgeCli { - /// The cloud to remove the connection from - cloud: Cloud, + #[clap(flatten)] + cloud: CloudArgs, } impl BuildCommand for TEdgeDisconnectBridgeCli { fn build_command(self, context: BuildContext) -> Result, crate::ConfigError> { let cmd = DisconnectBridgeCommand { config_location: context.config_location.clone(), - cloud: self.cloud, + cloud: self.cloud.try_into()?, use_mapper: true, service_manager: service_manager(&context.config_location.tedge_config_root_path)?, }; diff --git a/crates/core/tedge/src/cli/reconnect/cli.rs b/crates/core/tedge/src/cli/reconnect/cli.rs index 190a52dfb7c..daed7a6c830 100644 --- a/crates/core/tedge/src/cli/reconnect/cli.rs +++ b/crates/core/tedge/src/cli/reconnect/cli.rs @@ -1,12 +1,12 @@ use super::command::ReconnectBridgeCommand; -use crate::cli::common::Cloud; +use crate::cli::common::CloudArgs; use crate::command::*; use tedge_config::system_services::service_manager; #[derive(clap::Args, Debug)] pub struct TEdgeReconnectCli { - /// The cloud you wish to re-establish the connection to - cloud: Cloud, + #[clap(flatten)] + cloud: CloudArgs, } impl BuildCommand for TEdgeReconnectCli { @@ -15,7 +15,7 @@ impl BuildCommand for TEdgeReconnectCli { config: context.load_config()?, service_manager: service_manager(&context.config_location.tedge_config_root_path)?, config_location: context.config_location, - cloud: self.cloud, + cloud: self.cloud.try_into()?, use_mapper: true, } .into_boxed()) diff --git a/crates/core/tedge_mapper/src/lib.rs b/crates/core/tedge_mapper/src/lib.rs index 2c49dd38519..d1de6fe3efe 100644 --- a/crates/core/tedge_mapper/src/lib.rs +++ b/crates/core/tedge_mapper/src/lib.rs @@ -1,16 +1,13 @@ +use std::fmt; + use crate::aws::mapper::AwsMapper; use crate::az::mapper::AzureMapper; use crate::c8y::mapper::CumulocityMapper; use crate::collectd::mapper::CollectdMapper; use crate::core::component::TEdgeComponent; -use anyhow::bail; use anyhow::Context; -use clap::Command; -use clap::FromArgMatches; use clap::Parser; use flockfile::check_another_instance_is_not_running; -use std::fmt; -use std::str::FromStr; use tedge_config::cli::CommonArgs; use tedge_config::system_services::log_init; use tedge_config::ProfileName; @@ -22,6 +19,9 @@ mod c8y; mod collectd; mod core; +/// Set the cloud profile either from the CLI argument or env variable, +/// then set the environment variable so child processes automatically +/// have the correct profile set. macro_rules! read_and_set_var { ($profile:ident, $var:literal) => { $profile @@ -40,15 +40,15 @@ macro_rules! read_and_set_var { fn lookup_component(component_name: MapperName) -> Box { match component_name { - MapperName::Az(profile) => Box::new(AzureMapper { - profile: read_and_set_var!(profile, "AZ_PROFILE"), + MapperName::Az { profile } => Box::new(AzureMapper { + profile: read_and_set_var!(profile, "TEDGE_CLOUD_PROFILE"), }), - MapperName::Aws(profile) => Box::new(AwsMapper { - profile: read_and_set_var!(profile, "AWS_PROFILE"), + MapperName::Aws { profile } => Box::new(AwsMapper { + profile: read_and_set_var!(profile, "TEDGE_CLOUD_PROFILE"), }), MapperName::Collectd => Box::new(CollectdMapper), - MapperName::C8y(profile) => Box::new(CumulocityMapper { - profile: read_and_set_var!(profile, "C8Y_PROFILE"), + MapperName::C8y { profile } => Box::new(CumulocityMapper { + profile: read_and_set_var!(profile, "TEDGE_CLOUD_PROFILE"), }), } } @@ -61,101 +61,66 @@ fn lookup_component(component_name: MapperName) -> Box { )] pub struct MapperOpt { #[clap(subcommand)] - pub name: MapperName, + name: MapperName, /// Start the mapper with clean session off, subscribe to the topics, so that no messages are lost #[clap(short, long)] - pub init: bool, + init: bool, /// Start the agent with clean session on, drop the previous session and subscriptions /// /// WARNING: All pending messages will be lost. #[clap(short, long)] - pub clear: bool, + clear: bool, #[command(flatten)] pub common: CommonArgs, } -#[derive(Debug, Clone)] +#[derive(Debug, clap::Subcommand, Clone)] +#[clap(rename_all = "snake_case")] pub enum MapperName { - Az(Option), - Aws(Option), - C8y(Option), + Az { + /// The cloud profile to use + #[clap(long)] + profile: Option, + }, + Aws { + /// The cloud profile to use + #[clap(long)] + profile: Option, + }, + C8y { + /// The cloud profile to use + #[clap(long)] + profile: Option, + }, Collectd, } -impl FromStr for MapperName { - type Err = anyhow::Error; - fn from_str(s: &str) -> Result { - match (s, s.split_once("@")) { - ("az", _) => Ok(Self::Az(None)), - ("aws", _) => Ok(Self::Aws(None)), - ("c8y", _) => Ok(Self::C8y(None)), - ("collectd", _) => Ok(Self::Collectd), - (_, Some(("az", profile))) => Ok(Self::Az(Some(profile.parse()?))), - (_, Some(("aws", profile))) => Ok(Self::Aws(Some(profile.parse()?))), - (_, Some(("c8y", profile))) => Ok(Self::C8y(Some(profile.parse()?))), - _ => bail!("Unknown subcommand `{s}`"), - } - } -} - -impl FromArgMatches for MapperName { - fn from_arg_matches(matches: &clap::ArgMatches) -> Result { - match matches.subcommand() { - Some((cmd, _)) => cmd.parse().map_err(|_| { - clap::Error::raw( - clap::error::ErrorKind::InvalidSubcommand, - "Valid subcommands are `c8y`, `aws` and `az`", - ) - }), - None => Err(clap::Error::raw( - clap::error::ErrorKind::MissingSubcommand, - "Valid subcommands are `c8y`, `aws` and `az`", - )), - } - } - - fn update_from_arg_matches(&mut self, matches: &clap::ArgMatches) -> Result<(), clap::Error> { - *self = Self::from_arg_matches(matches)?; - Ok(()) - } -} - -impl clap::Subcommand for MapperName { - fn augment_subcommands(cmd: clap::Command) -> clap::Command { - cmd.subcommand(Command::new("c8y")) - .subcommand(Command::new("c8y@")) - .subcommand_required(true) - .allow_external_subcommands(true) - } - - fn augment_subcommands_for_update(cmd: clap::Command) -> clap::Command { - Self::augment_subcommands(cmd) - } - - fn has_subcommand(name: &str) -> bool { - name.parse::().is_ok() - } -} - impl fmt::Display for MapperName { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { - MapperName::Az(None) => write!(f, "tedge-mapper-az"), - MapperName::Az(Some(profile)) => write!(f, "tedge-mapper-az@{profile}"), - MapperName::Aws(None) => write!(f, "tedge-mapper-aws"), - MapperName::Aws(Some(profile)) => write!(f, "tedge-mapper-aws@{profile}"), - MapperName::C8y(None) => write!(f, "tedge-mapper-c8y"), - MapperName::C8y(Some(profile)) => write!(f, "tedge-mapper-c8y@{profile}"), + MapperName::Az { profile: None } => write!(f, "tedge-mapper-az"), + MapperName::Az { + profile: Some(profile), + } => write!(f, "tedge-mapper-az@{profile}"), + MapperName::Aws { profile: None } => write!(f, "tedge-mapper-aws"), + MapperName::Aws { + profile: Some(profile), + } => write!(f, "tedge-mapper-aws@{profile}"), + MapperName::C8y { profile: None } => write!(f, "tedge-mapper-c8y"), + MapperName::C8y { + profile: Some(profile), + } => write!(f, "tedge-mapper-c8y@{profile}"), MapperName::Collectd => write!(f, "tedge-mapper-collectd"), } } } pub async fn run(mapper_opt: MapperOpt) -> anyhow::Result<()> { - let component = lookup_component(mapper_opt.name.clone()); + let mapper_name = mapper_opt.name.to_string(); + let component = lookup_component(mapper_opt.name); let tedge_config_location = tedge_config::TEdgeConfigLocation::from_custom_root(&mapper_opt.common.config_dir); @@ -171,7 +136,7 @@ pub async fn run(mapper_opt: MapperOpt) -> anyhow::Result<()> { let mut _flock = None; if config.run.lock_files { let run_dir = config.run.path.as_std_path(); - _flock = check_another_instance_is_not_running(&mapper_opt.name.to_string(), run_dir)?; + _flock = check_another_instance_is_not_running(&mapper_name, run_dir)?; } if mapper_opt.init { diff --git a/plugins/c8y_firmware_plugin/src/lib.rs b/plugins/c8y_firmware_plugin/src/lib.rs index 7250a04394d..68a45c531ae 100644 --- a/plugins/c8y_firmware_plugin/src/lib.rs +++ b/plugins/c8y_firmware_plugin/src/lib.rs @@ -43,7 +43,7 @@ pub struct FirmwarePluginOpt { #[command(flatten)] pub common: CommonArgs, - #[clap(long, env = "C8Y_PROFILE", hide = true)] + #[clap(long, env = "TEDGE_CLOUD_PROFILE", hide = true)] pub profile: Option, } diff --git a/plugins/c8y_remote_access_plugin/src/input.rs b/plugins/c8y_remote_access_plugin/src/input.rs index dea4018337c..6cc0327c9c0 100644 --- a/plugins/c8y_remote_access_plugin/src/input.rs +++ b/plugins/c8y_remote_access_plugin/src/input.rs @@ -58,7 +58,7 @@ pub struct C8yRemoteAccessPluginOpt { #[arg(long, requires("init"), default_value = "tedge")] group: Option, - #[arg(long, env = "C8Y_PROFILE", hide = true)] + #[arg(long, env = "TEDGE_CLOUD_PROFILE", hide = true)] /// The c8y profile to use pub profile: Option, diff --git a/tests/RobotFramework/tests/cumulocity/configuration/configuration_operation_multi_cloud.robot b/tests/RobotFramework/tests/cumulocity/configuration/configuration_operation_multi_cloud.robot index a68917e598f..0d783332593 100644 --- a/tests/RobotFramework/tests/cumulocity/configuration/configuration_operation_multi_cloud.robot +++ b/tests/RobotFramework/tests/cumulocity/configuration/configuration_operation_multi_cloud.robot @@ -69,17 +69,20 @@ Setup Second Device Set Suite Variable $SECOND_DEVICE_SN ${second_device_sn} Execute Command - ... tedge config set c8y@second.device.cert_path /etc/tedge/device-certs/tedge@second-certificate.pem - Execute Command tedge config set c8y@second.device.key_path /etc/tedge/device-certs/tedge@second-key.pem - Execute Command tedge config set c8y@second.proxy.bind.port 8002 - Execute Command tedge config set c8y@second.bridge.topic_prefix c8y-second - Execute Command tedge config set c8y@second.url "$(tedge config get c8y.url)" + ... tedge config set c8y.device.cert_path --profile second /etc/tedge/device-certs/tedge@second-certificate.pem + Execute Command + ... tedge config set c8y.device.key_path --profile second /etc/tedge/device-certs/tedge@second-key.pem + Execute Command tedge config set c8y.proxy.bind.port --profile second 8002 + Execute Command tedge config set c8y.bridge.topic_prefix --profile second c8y-second + Execute Command tedge config set c8y.url --profile second "$(tedge config get c8y.url)" - Execute Command tedge cert create c8y@second --device-id ${second_device_sn} + Execute Command tedge cert create c8y --profile second --device-id ${second_device_sn} Execute Command - ... cmd=sudo env C8Y_USER='${C8Y_CONFIG.username}' C8Y_PASSWORD='${C8Y_CONFIG.password}' tedge cert upload c8y@second + ... cmd=sudo env C8Y_USER='${C8Y_CONFIG.username}' C8Y_PASSWORD='${C8Y_CONFIG.password}' tedge cert upload c8y --profile second - Execute Command tedge connect c8y@second + Execute Command tedge connect c8y --profile second + # Verify the mapper has actually started successfully + Execute Command systemctl is-active tedge-mapper-c8y@second RETURN ${second_device_sn} From 25913e01460e584d53d7364f0203700e8e2f8b16 Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Mon, 9 Dec 2024 11:24:33 +0000 Subject: [PATCH 06/17] Change the c8y bridge local client ID to be backwards compatible to avoid lost messages Signed-off-by: James Rhodes --- crates/core/tedge/src/bridge/c8y.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/core/tedge/src/bridge/c8y.rs b/crates/core/tedge/src/bridge/c8y.rs index 615eef0b13b..1d7399c36e2 100644 --- a/crates/core/tedge/src/bridge/c8y.rs +++ b/crates/core/tedge/src/bridge/c8y.rs @@ -160,9 +160,9 @@ impl From for BridgeConfig { bridge_root_cert_path, remote_clientid, local_clientid: if let Some(profile) = &profile_name { - format!("c8y-bridge@{profile}") + format!("Cumulocity@{profile}") } else { - "c8y-bridge".into() + "Cumulocity".into() }, bridge_certfile, bridge_keyfile, From b2187764f0828caf14edcaabd166cb39c8f2115e Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Mon, 9 Dec 2024 11:44:32 +0000 Subject: [PATCH 07/17] Fix tests Signed-off-by: James Rhodes --- crates/core/tedge/src/bridge/c8y.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/core/tedge/src/bridge/c8y.rs b/crates/core/tedge/src/bridge/c8y.rs index 1d7399c36e2..7fbbf20a872 100644 --- a/crates/core/tedge/src/bridge/c8y.rs +++ b/crates/core/tedge/src/bridge/c8y.rs @@ -255,7 +255,7 @@ mod tests { remote_password: None, bridge_root_cert_path: Utf8PathBuf::from("./test_root.pem"), remote_clientid: "alpha".into(), - local_clientid: "c8y-bridge".into(), + local_clientid: "Cumulocity".into(), bridge_certfile: "./test-certificate.pem".into(), bridge_keyfile: "./test-private-key.pem".into(), use_mapper: true, @@ -349,7 +349,7 @@ mod tests { remote_password: Some("abcd1234".into()), bridge_root_cert_path: Utf8PathBuf::from("./test_root.pem"), remote_clientid: "alpha".into(), - local_clientid: "c8y-bridge@profile".into(), + local_clientid: "Cumulocity@profile".into(), bridge_certfile: "./test-certificate.pem".into(), bridge_keyfile: "./test-private-key.pem".into(), use_mapper: true, From 9409874f57f6c9704fd035387cc68aa3293416dc Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Tue, 10 Dec 2024 18:30:39 +0000 Subject: [PATCH 08/17] Ignore default profile in validation if URL is not set Signed-off-by: James Rhodes --- crates/core/tedge/src/cli/connect/command.rs | 64 ++++++++++++++++++-- 1 file changed, 58 insertions(+), 6 deletions(-) diff --git a/crates/core/tedge/src/cli/connect/command.rs b/crates/core/tedge/src/cli/connect/command.rs index 40d9ff39334..0388fc4ecff 100644 --- a/crates/core/tedge/src/cli/connect/command.rs +++ b/crates/core/tedge/src/cli/connect/command.rs @@ -284,8 +284,9 @@ fn validate_config(config: &TEdgeConfig, cloud: &MaybeBorrowedCloud<'_>) -> anyh MaybeBorrowedCloud::Aws(_) => { let profiles = config .aws - .keys() - .map(|s| Some(s?.to_string())) + .entries() + .filter(|(_, config)| config.url.or_none().is_some()) + .map(|(s, _)| Some(s?.to_string())) .collect::>(); disallow_matching_configurations(config, ReadableKey::AwsUrl, &profiles).or_else( |_| disallow_matching_configurations(config, ReadableKey::AwsDeviceId, &profiles), @@ -295,8 +296,9 @@ fn validate_config(config: &TEdgeConfig, cloud: &MaybeBorrowedCloud<'_>) -> anyh MaybeBorrowedCloud::Azure(_) => { let profiles = config .az - .keys() - .map(|s| Some(s?.to_string())) + .entries() + .filter(|(_, config)| config.url.or_none().is_some()) + .map(|(s, _)| Some(s?.to_string())) .collect::>(); disallow_matching_configurations(config, ReadableKey::AzUrl, &profiles).or_else( |_| disallow_matching_configurations(config, ReadableKey::AzDeviceId, &profiles), @@ -306,8 +308,9 @@ fn validate_config(config: &TEdgeConfig, cloud: &MaybeBorrowedCloud<'_>) -> anyh MaybeBorrowedCloud::C8y(_) => { let profiles = config .c8y - .keys() - .map(|s| Some(s?.to_string())) + .entries() + .filter(|(_, config)| config.http.or_none().is_some()) + .map(|(s, _)| Some(s?.to_string())) .collect::>(); disallow_matching_configurations(config, ReadableKey::C8yUrl, &profiles).or_else( |_| disallow_matching_configurations(config, ReadableKey::C8yDeviceId, &profiles), @@ -429,6 +432,7 @@ pub fn bridge_config( bridge_location, topic_prefix: c8y_config.bridge.topic_prefix.clone(), profile_name: profile.clone().map(Cow::into_owned), + mqtt_topic_root: config.mqtt.topic_root.clone(), }; Ok(BridgeConfig::from(params)) @@ -1185,6 +1189,54 @@ mod tests { validate_config(&config, &cloud).unwrap(); } + #[test] + fn allows_single_named_c8y_profile_without_default_profile() { + let cloud = MaybeBorrowedCloud::c8y(Some("new".parse().unwrap())); + let ttd = TempTedgeDir::new(); + let loc = TEdgeConfigLocation::from_custom_root(ttd.path()); + loc.update_toml(&|dto, _| { + dto.try_update_str(&"c8y@new.url".parse().unwrap(), "example.com") + .unwrap(); + Ok(()) + }) + .unwrap(); + let config = loc.load().unwrap(); + + validate_config(&config, &cloud).unwrap(); + } + + #[test] + fn allows_single_named_az_profile_without_default_profile() { + let cloud = MaybeBorrowedCloud::az(Some("new".parse().unwrap())); + let ttd = TempTedgeDir::new(); + let loc = TEdgeConfigLocation::from_custom_root(ttd.path()); + loc.update_toml(&|dto, _| { + dto.try_update_str(&"az@new.url".parse().unwrap(), "example.com") + .unwrap(); + Ok(()) + }) + .unwrap(); + let config = loc.load().unwrap(); + + validate_config(&config, &cloud).unwrap(); + } + + #[test] + fn allows_single_named_aws_profile_without_default_profile() { + let cloud = MaybeBorrowedCloud::aws(Some("new".parse().unwrap())); + let ttd = TempTedgeDir::new(); + let loc = TEdgeConfigLocation::from_custom_root(ttd.path()); + loc.update_toml(&|dto, _| { + dto.try_update_str(&"aws@new.url".parse().unwrap(), "example.com") + .unwrap(); + Ok(()) + }) + .unwrap(); + let config = loc.load().unwrap(); + + validate_config(&config, &cloud).unwrap(); + } + #[test] fn rejects_conflicting_topic_prefixes() { let cloud = MaybeBorrowedCloud::C8y(None); From 153f12369f21f99df294c11b2b17628595fa571e Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Wed, 11 Dec 2024 15:24:46 +0000 Subject: [PATCH 09/17] Rename mosquitto bridge notification topics to include topic prefix Signed-off-by: James Rhodes --- crates/core/tedge/src/bridge/aws.rs | 36 +++++++++++------ crates/core/tedge/src/bridge/azure.rs | 40 ++++++++++++------- crates/core/tedge/src/bridge/c8y.rs | 24 +++++++---- crates/core/tedge/src/cli/connect/command.rs | 5 ++- .../extensions/c8y_mapper_ext/src/config.rs | 2 +- .../c8y_mapper_ext/src/converter.rs | 12 +++--- .../c8y_mapper_ext/src/service_monitor.rs | 14 ++++++- 7 files changed, 89 insertions(+), 44 deletions(-) diff --git a/crates/core/tedge/src/bridge/aws.rs b/crates/core/tedge/src/bridge/aws.rs index 122c657b3d9..96cbfdef7e9 100644 --- a/crates/core/tedge/src/bridge/aws.rs +++ b/crates/core/tedge/src/bridge/aws.rs @@ -2,14 +2,15 @@ use super::BridgeConfig; use crate::bridge::config::BridgeLocation; use camino::Utf8PathBuf; use std::borrow::Cow; +use tedge_api::mqtt_topics::Channel; +use tedge_api::mqtt_topics::EntityTopicId; +use tedge_api::mqtt_topics::MqttSchema; use tedge_config::HostPort; use tedge_config::ProfileName; use tedge_config::TopicPrefix; use tedge_config::MQTT_TLS_PORT; -const MOSQUITTO_BRIDGE_TOPIC: &str = "te/device/main/service/mosquitto-aws-bridge/status/health"; - -#[derive(Debug, Eq, PartialEq)] +#[derive(Debug)] pub struct BridgeConfigAwsParams { pub mqtt_host: HostPort, pub config_file: Cow<'static, str>, @@ -20,6 +21,7 @@ pub struct BridgeConfigAwsParams { pub bridge_location: BridgeLocation, pub topic_prefix: TopicPrefix, pub profile_name: Option, + pub mqtt_schema: MqttSchema, } impl From for BridgeConfig { @@ -34,6 +36,7 @@ impl From for BridgeConfig { bridge_location, topic_prefix, profile_name, + mqtt_schema, } = params; let user_name = remote_clientid.to_string(); @@ -54,6 +57,11 @@ impl From for BridgeConfig { r#""" in 1 {topic_prefix}/connection-success thinedge/devices/{remote_clientid}/test-connection"# ); + let service_name = format!("mosquitto-{topic_prefix}-bridge"); + let health = mqtt_schema.topic_for( + &EntityTopicId::default_main_service(&service_name).unwrap(), + &Channel::Health, + ); Self { cloud_name: "aws".into(), config_file, @@ -83,7 +91,7 @@ impl From for BridgeConfig { local_clean_session: false, notifications: true, notifications_local_only: true, - notification_topic: MOSQUITTO_BRIDGE_TOPIC.into(), + notification_topic: health.name, bridge_attempt_unsubscribe: false, topics: vec![ pub_msg_topic, @@ -115,6 +123,7 @@ fn test_bridge_config_from_aws_params() -> anyhow::Result<()> { bridge_location: BridgeLocation::Mosquitto, topic_prefix: "aws".try_into().unwrap(), profile_name: None, + mqtt_schema: MqttSchema::with_root("te".into()), }; let bridge = BridgeConfig::from(params); @@ -147,7 +156,7 @@ fn test_bridge_config_from_aws_params() -> anyhow::Result<()> { local_clean_session: false, notifications: true, notifications_local_only: true, - notification_topic: MOSQUITTO_BRIDGE_TOPIC.into(), + notification_topic: "te/device/main/service/mosquitto-aws-bridge/status/health".into(), bridge_attempt_unsubscribe: false, bridge_location: BridgeLocation::Mosquitto, connection_check_attempts: 5, @@ -170,8 +179,9 @@ fn test_bridge_config_aws_custom_topic_prefix() -> anyhow::Result<()> { bridge_certfile: "./test-certificate.pem".into(), bridge_keyfile: "./test-private-key.pem".into(), bridge_location: BridgeLocation::Mosquitto, - topic_prefix: "custom".try_into().unwrap(), + topic_prefix: "aws-custom".try_into().unwrap(), profile_name: Some("profile".parse().unwrap()), + mqtt_schema: MqttSchema::with_root("te".into()), }; let bridge = BridgeConfig::from(params); @@ -191,11 +201,12 @@ fn test_bridge_config_aws_custom_topic_prefix() -> anyhow::Result<()> { use_mapper: true, use_agent: false, topics: vec![ - "td/# out 1 custom/ thinedge/alpha/".into(), - "cmd/# in 1 custom/ thinedge/alpha/".into(), - "shadow/# both 1 custom/ $aws/things/alpha/".into(), - r#""" out 1 custom/test-connection thinedge/devices/alpha/test-connection"#.into(), - r#""" in 1 custom/connection-success thinedge/devices/alpha/test-connection"#.into(), + "td/# out 1 aws-custom/ thinedge/alpha/".into(), + "cmd/# in 1 aws-custom/ thinedge/alpha/".into(), + "shadow/# both 1 aws-custom/ $aws/things/alpha/".into(), + r#""" out 1 aws-custom/test-connection thinedge/devices/alpha/test-connection"#.into(), + r#""" in 1 aws-custom/connection-success thinedge/devices/alpha/test-connection"# + .into(), ], try_private: false, start_type: "automatic".into(), @@ -204,7 +215,8 @@ fn test_bridge_config_aws_custom_topic_prefix() -> anyhow::Result<()> { local_clean_session: false, notifications: true, notifications_local_only: true, - notification_topic: MOSQUITTO_BRIDGE_TOPIC.into(), + notification_topic: "te/device/main/service/mosquitto-aws-custom-bridge/status/health" + .into(), bridge_attempt_unsubscribe: false, bridge_location: BridgeLocation::Mosquitto, connection_check_attempts: 5, diff --git a/crates/core/tedge/src/bridge/azure.rs b/crates/core/tedge/src/bridge/azure.rs index de0e1147541..3b12846e7db 100644 --- a/crates/core/tedge/src/bridge/azure.rs +++ b/crates/core/tedge/src/bridge/azure.rs @@ -2,14 +2,15 @@ use super::BridgeConfig; use crate::bridge::config::BridgeLocation; use camino::Utf8PathBuf; use std::borrow::Cow; +use tedge_api::mqtt_topics::Channel; +use tedge_api::mqtt_topics::EntityTopicId; +use tedge_api::mqtt_topics::MqttSchema; use tedge_config::HostPort; use tedge_config::ProfileName; use tedge_config::TopicPrefix; use tedge_config::MQTT_TLS_PORT; -const MOSQUITTO_BRIDGE_TOPIC: &str = "te/device/main/service/mosquitto-az-bridge/status/health"; - -#[derive(Debug, Eq, PartialEq)] +#[derive(Debug)] pub struct BridgeConfigAzureParams { pub mqtt_host: HostPort, pub config_file: Cow<'static, str>, @@ -20,6 +21,7 @@ pub struct BridgeConfigAzureParams { pub bridge_location: BridgeLocation, pub topic_prefix: TopicPrefix, pub profile_name: Option, + pub mqtt_schema: MqttSchema, } impl From for BridgeConfig { @@ -34,6 +36,7 @@ impl From for BridgeConfig { bridge_location, topic_prefix, profile_name, + mqtt_schema, } = params; let address = mqtt_host.clone(); @@ -46,6 +49,12 @@ impl From for BridgeConfig { format!("messages/events/# out 1 {topic_prefix}/ devices/{remote_clientid}/"); let sub_msg_topic = format!("messages/devicebound/# in 1 {topic_prefix}/ devices/{remote_clientid}/"); + + let service_name = format!("mosquitto-{topic_prefix}-bridge"); + let health = mqtt_schema.topic_for( + &EntityTopicId::default_main_service(&service_name).unwrap(), + &Channel::Health, + ); Self { cloud_name: "az".into(), config_file, @@ -75,7 +84,7 @@ impl From for BridgeConfig { local_clean_session: false, notifications: true, notifications_local_only: true, - notification_topic: MOSQUITTO_BRIDGE_TOPIC.into(), + notification_topic: health.name, bridge_attempt_unsubscribe: false, topics: vec![ // See Azure IoT Hub documentation for detailed explanation on the topics @@ -112,6 +121,7 @@ fn test_bridge_config_from_azure_params() -> anyhow::Result<()> { bridge_location: BridgeLocation::Mosquitto, topic_prefix: "az".try_into().unwrap(), profile_name: None, + mqtt_schema: MqttSchema::with_root("te".into()), }; let bridge = BridgeConfig::from(params); @@ -146,7 +156,7 @@ fn test_bridge_config_from_azure_params() -> anyhow::Result<()> { local_clean_session: false, notifications: true, notifications_local_only: true, - notification_topic: MOSQUITTO_BRIDGE_TOPIC.into(), + notification_topic: "te/device/main/service/mosquitto-az-bridge/status/health".into(), bridge_attempt_unsubscribe: false, bridge_location: BridgeLocation::Mosquitto, connection_check_attempts: 1, @@ -171,8 +181,9 @@ fn test_azure_bridge_config_with_custom_prefix() -> anyhow::Result<()> { bridge_certfile: "./test-certificate.pem".into(), bridge_keyfile: "./test-private-key.pem".into(), bridge_location: BridgeLocation::Mosquitto, - topic_prefix: "custom".try_into().unwrap(), + topic_prefix: "az-custom".try_into().unwrap(), profile_name: Some("profile".parse().unwrap()), + mqtt_schema: MqttSchema::with_root("te".into()), }; let bridge = BridgeConfig::from(params); @@ -192,13 +203,13 @@ fn test_azure_bridge_config_with_custom_prefix() -> anyhow::Result<()> { use_mapper: true, use_agent: false, topics: vec![ - "messages/events/# out 1 custom/ devices/alpha/".into(), - "messages/devicebound/# in 1 custom/ devices/alpha/".into(), - "methods/POST/# in 1 custom/ $iothub/".into(), - "methods/res/# out 1 custom/ $iothub/".into(), - "twin/res/# in 1 custom/ $iothub/".into(), - "twin/GET/# out 1 custom/ $iothub/".into(), - "twin/PATCH/# out 1 custom/ $iothub/".into(), + "messages/events/# out 1 az-custom/ devices/alpha/".into(), + "messages/devicebound/# in 1 az-custom/ devices/alpha/".into(), + "methods/POST/# in 1 az-custom/ $iothub/".into(), + "methods/res/# out 1 az-custom/ $iothub/".into(), + "twin/res/# in 1 az-custom/ $iothub/".into(), + "twin/GET/# out 1 az-custom/ $iothub/".into(), + "twin/PATCH/# out 1 az-custom/ $iothub/".into(), ], try_private: false, start_type: "automatic".into(), @@ -207,7 +218,8 @@ fn test_azure_bridge_config_with_custom_prefix() -> anyhow::Result<()> { local_clean_session: false, notifications: true, notifications_local_only: true, - notification_topic: MOSQUITTO_BRIDGE_TOPIC.into(), + notification_topic: "te/device/main/service/mosquitto-az-custom-bridge/status/health" + .into(), bridge_attempt_unsubscribe: false, bridge_location: BridgeLocation::Mosquitto, connection_check_attempts: 1, diff --git a/crates/core/tedge/src/bridge/c8y.rs b/crates/core/tedge/src/bridge/c8y.rs index 7fbbf20a872..e2c8fbded08 100644 --- a/crates/core/tedge/src/bridge/c8y.rs +++ b/crates/core/tedge/src/bridge/c8y.rs @@ -3,6 +3,9 @@ use crate::bridge::config::BridgeLocation; use camino::Utf8PathBuf; use std::borrow::Cow; use std::process::Command; +use tedge_api::mqtt_topics::Channel; +use tedge_api::mqtt_topics::EntityTopicId; +use tedge_api::mqtt_topics::MqttSchema; use tedge_config::auth_method::AuthMethod; use tedge_config::AutoFlag; use tedge_config::HostPort; @@ -12,9 +15,7 @@ use tedge_config::TopicPrefix; use tedge_config::MQTT_TLS_PORT; use which::which; -const C8Y_BRIDGE_HEALTH_TOPIC: &str = "te/device/main/service/mosquitto-c8y-bridge/status/health"; - -#[derive(Debug, Eq, PartialEq)] +#[derive(Debug)] pub struct BridgeConfigC8yParams { pub mqtt_host: HostPort, pub config_file: Cow<'static, str>, @@ -30,6 +31,7 @@ pub struct BridgeConfigC8yParams { pub bridge_location: BridgeLocation, pub topic_prefix: TopicPrefix, pub profile_name: Option, + pub mqtt_schema: MqttSchema, } impl From for BridgeConfig { @@ -49,6 +51,7 @@ impl From for BridgeConfig { bridge_location, topic_prefix, profile_name, + mqtt_schema, } = params; let mut topics: Vec = vec![ @@ -146,6 +149,11 @@ impl From for BridgeConfig { AuthMethod::Certificate }; + let service_name = format!("mosquitto-{topic_prefix}-bridge"); + let health = mqtt_schema.topic_for( + &EntityTopicId::default_main_service(&service_name).unwrap(), + &Channel::Health, + ); Self { cloud_name: "c8y".into(), config_file, @@ -175,9 +183,7 @@ impl From for BridgeConfig { local_clean_session: false, notifications: true, notifications_local_only: true, - - // FIXME: doesn't account for custom topic root, use MQTT scheme API here - notification_topic: C8Y_BRIDGE_HEALTH_TOPIC.into(), + notification_topic: health.name, bridge_attempt_unsubscribe: false, topics, bridge_location, @@ -242,6 +248,7 @@ mod tests { bridge_location: BridgeLocation::Mosquitto, topic_prefix: "c8y".try_into().unwrap(), profile_name: None, + mqtt_schema: MqttSchema::with_root("te".into()), }; let bridge = BridgeConfig::from(params); @@ -306,7 +313,7 @@ mod tests { local_clean_session: false, notifications: true, notifications_local_only: true, - notification_topic: C8Y_BRIDGE_HEALTH_TOPIC.into(), + notification_topic: "te/device/main/service/mosquitto-c8y-bridge/status/health".into(), bridge_attempt_unsubscribe: false, bridge_location: BridgeLocation::Mosquitto, connection_check_attempts: 1, @@ -336,6 +343,7 @@ mod tests { bridge_location: BridgeLocation::Mosquitto, topic_prefix: "c8y".try_into().unwrap(), profile_name: Some("profile".parse().unwrap()), + mqtt_schema: MqttSchema::with_root("te".into()), }; let bridge = BridgeConfig::from(params); @@ -407,7 +415,7 @@ mod tests { local_clean_session: false, notifications: true, notifications_local_only: true, - notification_topic: C8Y_BRIDGE_HEALTH_TOPIC.into(), + notification_topic: "te/device/main/service/mosquitto-c8y-bridge/status/health".into(), bridge_attempt_unsubscribe: false, bridge_location: BridgeLocation::Mosquitto, connection_check_attempts: 1, diff --git a/crates/core/tedge/src/cli/connect/command.rs b/crates/core/tedge/src/cli/connect/command.rs index 0388fc4ecff..0988567d061 100644 --- a/crates/core/tedge/src/cli/connect/command.rs +++ b/crates/core/tedge/src/cli/connect/command.rs @@ -365,6 +365,7 @@ pub fn bridge_config( true => BridgeLocation::BuiltIn, false => BridgeLocation::Mosquitto, }; + let mqtt_schema = MqttSchema::with_root(config.mqtt.topic_root.clone()); match cloud { MaybeBorrowedCloud::Azure(profile) => { let az_config = config.az.try_get(profile.as_deref())?; @@ -381,6 +382,7 @@ pub fn bridge_config( bridge_location, topic_prefix: az_config.bridge.topic_prefix.clone(), profile_name: profile.clone().map(Cow::into_owned), + mqtt_schema, }; Ok(BridgeConfig::from(params)) @@ -400,6 +402,7 @@ pub fn bridge_config( bridge_location, topic_prefix: aws_config.bridge.topic_prefix.clone(), profile_name: profile.clone().map(Cow::into_owned), + mqtt_schema, }; Ok(BridgeConfig::from(params)) @@ -432,7 +435,7 @@ pub fn bridge_config( bridge_location, topic_prefix: c8y_config.bridge.topic_prefix.clone(), profile_name: profile.clone().map(Cow::into_owned), - mqtt_topic_root: config.mqtt.topic_root.clone(), + mqtt_schema, }; Ok(BridgeConfig::from(params)) diff --git a/crates/extensions/c8y_mapper_ext/src/config.rs b/crates/extensions/c8y_mapper_ext/src/config.rs index e775d3b82af..2d730f4c6a2 100644 --- a/crates/extensions/c8y_mapper_ext/src/config.rs +++ b/crates/extensions/c8y_mapper_ext/src/config.rs @@ -113,7 +113,7 @@ impl C8yMapperConfig { let bridge_service_name = if bridge_in_mapper { format!("tedge-mapper-bridge-{}", bridge_config.c8y_prefix) } else { - "mosquitto-c8y-bridge".into() + format!("mosquitto-{}-bridge", bridge_config.c8y_prefix) }; let bridge_health_topic = service_health_topic(&mqtt_schema, &device_topic_id, &bridge_service_name); diff --git a/crates/extensions/c8y_mapper_ext/src/converter.rs b/crates/extensions/c8y_mapper_ext/src/converter.rs index 157e9fa558d..da2bbebd031 100644 --- a/crates/extensions/c8y_mapper_ext/src/converter.rs +++ b/crates/extensions/c8y_mapper_ext/src/converter.rs @@ -1079,7 +1079,7 @@ impl CumulocityConverter { tokio::spawn(async move { let op_name = op_name.as_str(); - let topic = C8yTopic::SmartRestResponse.to_topic(&c8y_prefix).unwrap(); + let topic = C8yTopic::upstream_topic(&c8y_prefix); if !skip_status_update { // mqtt client publishes executing @@ -1474,7 +1474,7 @@ impl CumulocityConverter { let device_data_message = self.inventory_device_type_update_message()?; let pending_operations_message = - create_get_pending_operations_message(&self.config.bridge_config.c8y_prefix)?; + create_get_pending_operations_message(&self.config.bridge_config.c8y_prefix); messages.append(&mut vec![ supported_operations_message, @@ -1512,11 +1512,9 @@ impl CumulocityConverter { } } -fn create_get_pending_operations_message( - prefix: &TopicPrefix, -) -> Result { - let topic = C8yTopic::SmartRestResponse.to_topic(prefix)?; - Ok(MqttMessage::new(&topic, request_pending_operations())) +pub fn create_get_pending_operations_message(prefix: &TopicPrefix) -> MqttMessage { + let topic = C8yTopic::upstream_topic(prefix); + MqttMessage::new(&topic, request_pending_operations()) } impl CumulocityConverter { diff --git a/crates/extensions/c8y_mapper_ext/src/service_monitor.rs b/crates/extensions/c8y_mapper_ext/src/service_monitor.rs index 46bcaa3176b..048194e93be 100644 --- a/crates/extensions/c8y_mapper_ext/src/service_monitor.rs +++ b/crates/extensions/c8y_mapper_ext/src/service_monitor.rs @@ -3,11 +3,14 @@ use tedge_api::entity_store::EntityMetadata; use tedge_api::entity_store::EntityType; use tedge_api::mqtt_topics::MqttSchema; use tedge_api::HealthStatus; +use tedge_api::Status; use tedge_config::TopicPrefix; use tedge_mqtt_ext::MqttMessage; use tedge_mqtt_ext::Topic; use tracing::error; +use crate::converter::create_get_pending_operations_message; + pub fn is_c8y_bridge_established( message: &MqttMessage, mqtt_schema: &MqttSchema, @@ -60,7 +63,16 @@ pub fn convert_health_status_message( return vec![]; }; - vec![status_message] + let mut value = vec![status_message]; + + if display_name == format!("mosquitto-{prefix}-bridge") && status == Status::Up { + // Receiving this message indicates mosquitto has reconnected (following a + // disconnection) to the cloud. We need to re-request operations in case any + // were triggered while we were down + value.push(create_get_pending_operations_message(prefix)); + } + + value } #[cfg(test)] From ef52d2ad2413cc66d984afc6c82f724315eba50b Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Wed, 11 Dec 2024 15:51:33 +0000 Subject: [PATCH 10/17] Use subcommand to simplify argument handling Signed-off-by: James Rhodes --- crates/core/tedge/src/cli/certificate/cli.rs | 32 +++--- crates/core/tedge/src/cli/common.rs | 110 +++++++++---------- crates/core/tedge/src/cli/connect/cli.rs | 6 +- crates/core/tedge/src/cli/connect/command.rs | 18 +-- crates/core/tedge/src/cli/disconnect/cli.rs | 6 +- crates/core/tedge/src/cli/reconnect/cli.rs | 6 +- 6 files changed, 83 insertions(+), 95 deletions(-) diff --git a/crates/core/tedge/src/cli/certificate/cli.rs b/crates/core/tedge/src/cli/certificate/cli.rs index 7a1f3b21672..a31b17f675b 100644 --- a/crates/core/tedge/src/cli/certificate/cli.rs +++ b/crates/core/tedge/src/cli/certificate/cli.rs @@ -1,5 +1,5 @@ use crate::cli::common::Cloud; -use crate::cli::common::OptionalCloudArgs; +use crate::cli::common::CloudArg; use camino::Utf8PathBuf; use tedge_config::OptionalConfigError; use tedge_config::ProfileName; @@ -24,8 +24,8 @@ pub enum TEdgeCertCli { #[clap(long = "device-id")] id: String, - #[clap(flatten)] - cloud: OptionalCloudArgs, + #[clap(subcommand)] + cloud: Option, }, /// Create a certificate signing request @@ -38,26 +38,26 @@ pub enum TEdgeCertCli { #[clap(long = "output-path")] output_path: Option, - #[clap(flatten)] - cloud: OptionalCloudArgs, + #[clap(subcommand)] + cloud: Option, }, /// Renew the device certificate Renew { - #[clap(flatten)] - cloud: OptionalCloudArgs, + #[clap(subcommand)] + cloud: Option, }, /// Show the device certificate, if any Show { - #[clap(flatten)] - cloud: OptionalCloudArgs, + #[clap(subcommand)] + cloud: Option, }, /// Remove the device certificate Remove { - #[clap(flatten)] - cloud: OptionalCloudArgs, + #[clap(subcommand)] + cloud: Option, }, /// Upload root certificate @@ -76,7 +76,7 @@ impl BuildCommand for TEdgeCertCli { let cmd = match self { TEdgeCertCli::Create { id, cloud } => { - let cloud: Option = cloud.try_into()?; + let cloud: Option = cloud.map(<_>::try_into).transpose()?; let cmd = CreateCertCmd { id, cert_path: config.device_cert_path(cloud.as_ref())?.to_owned(), @@ -92,7 +92,7 @@ impl BuildCommand for TEdgeCertCli { output_path, cloud, } => { - let cloud: Option = cloud.try_into()?; + let cloud: Option = cloud.map(<_>::try_into).transpose()?; // Use the current device id if no id is provided let id = match id { Some(id) => id, @@ -110,7 +110,7 @@ impl BuildCommand for TEdgeCertCli { } TEdgeCertCli::Show { cloud } => { - let cloud: Option = cloud.try_into()?; + let cloud: Option = cloud.map(<_>::try_into).transpose()?; let cmd = ShowCertCmd { cert_path: config.device_cert_path(cloud.as_ref())?.to_owned(), }; @@ -118,7 +118,7 @@ impl BuildCommand for TEdgeCertCli { } TEdgeCertCli::Remove { cloud } => { - let cloud: Option = cloud.try_into()?; + let cloud: Option = cloud.map(<_>::try_into).transpose()?; let cmd = RemoveCertCmd { cert_path: config.device_cert_path(cloud.as_ref())?.to_owned(), key_path: config.device_key_path(cloud.as_ref())?.to_owned(), @@ -143,7 +143,7 @@ impl BuildCommand for TEdgeCertCli { cmd.into_boxed() } TEdgeCertCli::Renew { cloud } => { - let cloud: Option = cloud.try_into()?; + let cloud: Option = cloud.map(<_>::try_into).transpose()?; let cmd = RenewCertCmd { cert_path: config.device_cert_path(cloud.as_ref())?.to_owned(), key_path: config.device_key_path(cloud.as_ref())?.to_owned(), diff --git a/crates/core/tedge/src/cli/common.rs b/crates/core/tedge/src/cli/common.rs index d607e97c6ab..1f3eb9fdce1 100644 --- a/crates/core/tedge/src/cli/common.rs +++ b/crates/core/tedge/src/cli/common.rs @@ -4,75 +4,63 @@ use std::fmt; use tedge_config::system_services::SystemService; use tedge_config::ProfileName; -#[derive(clap::Args, PartialEq, Eq, Debug, Clone)] -pub struct CloudArgs { - /// The cloud you wish to interact with - cloud: CloudType, - - /// The cloud profile you wish to use, if not specified as part of the cloud - #[clap(long)] - profile: Option, -} - -#[derive(clap::Args, PartialEq, Eq, Debug, Clone)] -pub struct OptionalCloudArgs { - /// The cloud you wish to interact with - cloud: Option, - - /// The cloud profile you wish to use, if not specified as part of the cloud - #[clap(long)] - profile: Option, -} - -#[derive(clap::ValueEnum, Debug, Copy, Clone, PartialEq, Eq)] +#[derive(clap::Subcommand, Debug, Clone, PartialEq, Eq)] #[clap(rename_all = "snake_case")] -enum CloudType { - C8y, - Az, - Aws, -} - -impl TryFrom for Cloud { - type Error = anyhow::Error; - - fn try_from(args: CloudArgs) -> Result { - args.cloud.try_with_profile_and_env(args.profile) - } +pub enum CloudArg { + C8y { + /// The cloud profile you wish to use + #[clap(long)] + profile: Option, + }, + Az { + /// The cloud profile you wish to use + #[clap(long)] + profile: Option, + }, + Aws { + /// The cloud profile you wish to use + #[clap(long)] + profile: Option, + }, } -impl TryFrom for Option { +impl TryFrom for Cloud { type Error = anyhow::Error; - fn try_from(args: OptionalCloudArgs) -> Result { - args.cloud - .map(|cloud| cloud.try_with_profile_and_env(args.profile)) - .transpose() + fn try_from(args: CloudArg) -> Result { + args.try_with_profile_and_env() } } -impl CloudType { - pub fn try_with_profile_and_env(self, profile: Option) -> anyhow::Result { - let env = "TEDGE_CLOUD_PROFILE"; - - match profile { - Some(profile) => Ok(self.with_profile(Some(profile))), - None => match std::env::var(env).as_deref() { - Ok("") => Ok(self.with_profile(None)), - Ok(e) => Ok(self.with_profile(Some(e.parse().with_context(|| { - format!("Parsing profile from environment variable {env}={e:?}") - })?))), - _ => Ok(self.with_profile(None)), - }, - } - } - - fn with_profile(self, profile: Option) -> Cloud { - let profile = profile.map(Cow::Owned); - match self { - Self::Aws => Cloud::Aws(profile), - Self::Az => Cloud::Azure(profile), - Self::C8y => Cloud::C8y(profile), - } +impl CloudArg { + fn try_with_profile_and_env(self) -> anyhow::Result { + let read_env = || { + let env = "TEDGE_CLOUD_PROFILE"; + match std::env::var(env).as_deref() { + Ok("") => Ok(None), + Ok(var) => var + .parse() + .with_context(|| { + format!("Parsing profile from environment variable {env}={var:?}") + }) + .map(Some), + _ => Ok(None), + } + }; + Ok(match self { + Self::Aws { + profile: Some(profile), + } => Cloud::aws(Some(profile)), + Self::Az { + profile: Some(profile), + } => Cloud::az(Some(profile)), + Self::C8y { + profile: Some(profile), + } => Cloud::c8y(Some(profile)), + Self::Aws { profile: None } => Cloud::aws(read_env()?), + Self::Az { profile: None } => Cloud::az(read_env()?), + Self::C8y { profile: None } => Cloud::c8y(read_env()?), + }) } } diff --git a/crates/core/tedge/src/cli/connect/cli.rs b/crates/core/tedge/src/cli/connect/cli.rs index 4ee83b28d94..0fd50cb92fa 100644 --- a/crates/core/tedge/src/cli/connect/cli.rs +++ b/crates/core/tedge/src/cli/connect/cli.rs @@ -1,4 +1,4 @@ -use crate::cli::common::CloudArgs; +use crate::cli::common::CloudArg; use crate::cli::connect::*; use crate::command::BuildCommand; use crate::command::BuildContext; @@ -15,8 +15,8 @@ pub struct TEdgeConnectOpt { #[clap(long = "offline")] offline_mode: bool, - #[clap(flatten)] - cloud: CloudArgs, + #[clap(subcommand)] + cloud: CloudArg, } impl BuildCommand for TEdgeConnectOpt { diff --git a/crates/core/tedge/src/cli/connect/command.rs b/crates/core/tedge/src/cli/connect/command.rs index 0988567d061..4fe397caff0 100644 --- a/crates/core/tedge/src/cli/connect/command.rs +++ b/crates/core/tedge/src/cli/connect/command.rs @@ -1178,13 +1178,13 @@ mod tests { mod validate_config { use super::super::validate_config; - use super::MaybeBorrowedCloud; + use super::Cloud; use tedge_config::TEdgeConfigLocation; use tedge_test_utils::fs::TempTedgeDir; #[test] fn allows_default_config() { - let cloud = MaybeBorrowedCloud::C8y(None); + let cloud = Cloud::C8y(None); let ttd = TempTedgeDir::new(); let loc = TEdgeConfigLocation::from_custom_root(ttd.path()); let config = loc.load().unwrap(); @@ -1194,7 +1194,7 @@ mod tests { #[test] fn allows_single_named_c8y_profile_without_default_profile() { - let cloud = MaybeBorrowedCloud::c8y(Some("new".parse().unwrap())); + let cloud = Cloud::c8y(Some("new".parse().unwrap())); let ttd = TempTedgeDir::new(); let loc = TEdgeConfigLocation::from_custom_root(ttd.path()); loc.update_toml(&|dto, _| { @@ -1210,7 +1210,7 @@ mod tests { #[test] fn allows_single_named_az_profile_without_default_profile() { - let cloud = MaybeBorrowedCloud::az(Some("new".parse().unwrap())); + let cloud = Cloud::az(Some("new".parse().unwrap())); let ttd = TempTedgeDir::new(); let loc = TEdgeConfigLocation::from_custom_root(ttd.path()); loc.update_toml(&|dto, _| { @@ -1226,7 +1226,7 @@ mod tests { #[test] fn allows_single_named_aws_profile_without_default_profile() { - let cloud = MaybeBorrowedCloud::aws(Some("new".parse().unwrap())); + let cloud = Cloud::aws(Some("new".parse().unwrap())); let ttd = TempTedgeDir::new(); let loc = TEdgeConfigLocation::from_custom_root(ttd.path()); loc.update_toml(&|dto, _| { @@ -1242,7 +1242,7 @@ mod tests { #[test] fn rejects_conflicting_topic_prefixes() { - let cloud = MaybeBorrowedCloud::C8y(None); + let cloud = Cloud::C8y(None); let ttd = TempTedgeDir::new(); let loc = TEdgeConfigLocation::from_custom_root(ttd.path()); loc.update_toml(&|dto, _| { @@ -1265,7 +1265,7 @@ mod tests { #[test] fn rejects_conflicting_bind_ports() { - let cloud = MaybeBorrowedCloud::C8y(None); + let cloud = Cloud::C8y(None); let ttd = TempTedgeDir::new(); let loc = TEdgeConfigLocation::from_custom_root(ttd.path()); loc.update_toml(&|dto, _| { @@ -1288,7 +1288,7 @@ mod tests { #[test] fn ignores_conflicting_configs_for_other_clouds() { - let cloud = MaybeBorrowedCloud::Azure(None); + let cloud = Cloud::Azure(None); let ttd = TempTedgeDir::new(); let loc = TEdgeConfigLocation::from_custom_root(ttd.path()); loc.update_toml(&|dto, _| { @@ -1306,7 +1306,7 @@ mod tests { #[test] fn allows_non_conflicting_topic_prefixes() { - let cloud = MaybeBorrowedCloud::Azure(None); + let cloud = Cloud::Azure(None); let ttd = TempTedgeDir::new(); let loc = TEdgeConfigLocation::from_custom_root(ttd.path()); loc.update_toml(&|dto, _| { diff --git a/crates/core/tedge/src/cli/disconnect/cli.rs b/crates/core/tedge/src/cli/disconnect/cli.rs index 5029efb264d..7031f1bdd51 100644 --- a/crates/core/tedge/src/cli/disconnect/cli.rs +++ b/crates/core/tedge/src/cli/disconnect/cli.rs @@ -1,12 +1,12 @@ -use crate::cli::common::CloudArgs; +use crate::cli::common::CloudArg; use crate::cli::disconnect::disconnect_bridge::*; use crate::command::*; use tedge_config::system_services::service_manager; #[derive(clap::Args, Debug)] pub struct TEdgeDisconnectBridgeCli { - #[clap(flatten)] - cloud: CloudArgs, + #[clap(subcommand)] + cloud: CloudArg, } impl BuildCommand for TEdgeDisconnectBridgeCli { diff --git a/crates/core/tedge/src/cli/reconnect/cli.rs b/crates/core/tedge/src/cli/reconnect/cli.rs index daed7a6c830..99f07e207a4 100644 --- a/crates/core/tedge/src/cli/reconnect/cli.rs +++ b/crates/core/tedge/src/cli/reconnect/cli.rs @@ -1,12 +1,12 @@ use super::command::ReconnectBridgeCommand; -use crate::cli::common::CloudArgs; +use crate::cli::common::CloudArg; use crate::command::*; use tedge_config::system_services::service_manager; #[derive(clap::Args, Debug)] pub struct TEdgeReconnectCli { - #[clap(flatten)] - cloud: CloudArgs, + #[clap(subcommand)] + cloud: CloudArg, } impl BuildCommand for TEdgeReconnectCli { From bf45dff81d8d13d2908252b1359e42b575dc6f4f Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Wed, 11 Dec 2024 16:50:46 +0000 Subject: [PATCH 11/17] Add system test for tedge connect validation Signed-off-by: James Rhodes --- crates/core/tedge/src/cli/certificate/cli.rs | 6 +-- crates/core/tedge/src/cli/connect/cli.rs | 4 +- .../tedge_connect_profile_validation.robot | 37 +++++++++++++++++++ 3 files changed, 42 insertions(+), 5 deletions(-) create mode 100644 tests/RobotFramework/tests/tedge_connect/tedge_connect_profile_validation.robot diff --git a/crates/core/tedge/src/cli/certificate/cli.rs b/crates/core/tedge/src/cli/certificate/cli.rs index a31b17f675b..599dc8b4466 100644 --- a/crates/core/tedge/src/cli/certificate/cli.rs +++ b/crates/core/tedge/src/cli/certificate/cli.rs @@ -21,7 +21,7 @@ pub enum TEdgeCertCli { /// Create a self-signed device certificate Create { /// The device identifier to be used as the common name for the certificate - #[clap(long = "device-id")] + #[clap(long = "device-id", global = true)] id: String, #[clap(subcommand)] @@ -31,11 +31,11 @@ pub enum TEdgeCertCli { /// Create a certificate signing request CreateCsr { /// The device identifier to be used as the common name for the certificate - #[clap(long = "device-id")] + #[clap(long = "device-id", global = true)] id: Option, /// Path where a Certificate signing request will be stored - #[clap(long = "output-path")] + #[clap(long = "output-path", global = true)] output_path: Option, #[clap(subcommand)] diff --git a/crates/core/tedge/src/cli/connect/cli.rs b/crates/core/tedge/src/cli/connect/cli.rs index 0fd50cb92fa..50a7ea54255 100644 --- a/crates/core/tedge/src/cli/connect/cli.rs +++ b/crates/core/tedge/src/cli/connect/cli.rs @@ -8,11 +8,11 @@ use tedge_config::system_services::service_manager; #[derive(clap::Args, Debug, Eq, PartialEq)] pub struct TEdgeConnectOpt { /// Test an existing connection - #[clap(long = "test")] + #[clap(long = "test", global = true)] is_test_connection: bool, /// Ignore connection registration and connection check - #[clap(long = "offline")] + #[clap(long = "offline", global = true)] offline_mode: bool, #[clap(subcommand)] diff --git a/tests/RobotFramework/tests/tedge_connect/tedge_connect_profile_validation.robot b/tests/RobotFramework/tests/tedge_connect/tedge_connect_profile_validation.robot new file mode 100644 index 00000000000..58220627312 --- /dev/null +++ b/tests/RobotFramework/tests/tedge_connect/tedge_connect_profile_validation.robot @@ -0,0 +1,37 @@ +*** Settings *** +Resource ../../resources/common.resource +Library ThinEdgeIO + +Suite Setup Setup +Suite Teardown Get Logs + +Test Tags theme:mqtt theme:c8y adapter:docker + + +*** Test Cases *** +Bridge topic prefix must be unique to connect + ThinEdgeIO.Execute Command sudo tedge config set c8y.url --profile second example.com timeout=0 + Verify conflicting configuration error appears sudo tedge connect c8y --profile second bridge.topic_prefix + +Bridge topic prefix must be unique to test connection + ThinEdgeIO.Execute Command sudo tedge config set c8y.url --profile second example.com timeout=0 + Verify conflicting configuration error appears sudo tedge connect c8y --test --profile second bridge.topic_prefix + +Device ID must be unique if cloud URLs are the same + ThinEdgeIO.Execute Command sudo tedge config set c8y.url --profile second "$(tedge config get c8y.url)" timeout=0 + Verify conflicting configuration error appears sudo tedge connect c8y --profile second device.id + +Proxy bind port must be unique + ThinEdgeIO.Execute Command sudo tedge config set c8y.url --profile second example.com timeout=0 + ThinEdgeIO.Execute Command sudo tedge config set c8y.bridge.topic_prefix --profile second c8y-second timeout=0 + Verify conflicting configuration error appears sudo tedge connect c8y --profile second proxy.bind.port + + +*** Keywords *** +Verify conflicting configuration error appears + [Arguments] ${command} ${conflicting_configuration} + ${output}= ThinEdgeIO.Execute Command ${command} exp_exit_code=1 stdout=False stderr=True timeout=0 + Should Be Equal + ... ${output} + ... error: The configurations: c8y.${conflicting_configuration}, c8y@second.${conflicting_configuration} should be set to different values before connecting, but are currently set to the same value\n + From d6a05012aa13f8c0f2527a8fe5442f012f34b901 Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Wed, 11 Dec 2024 16:51:19 +0000 Subject: [PATCH 12/17] Remove unnecessary result handling --- crates/extensions/c8y_firmware_manager/src/worker.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/extensions/c8y_firmware_manager/src/worker.rs b/crates/extensions/c8y_firmware_manager/src/worker.rs index 786e806b333..27b6df5a938 100644 --- a/crates/extensions/c8y_firmware_manager/src/worker.rs +++ b/crates/extensions/c8y_firmware_manager/src/worker.rs @@ -438,7 +438,7 @@ impl FirmwareManagerWorker { &mut self, ) -> Result<(), FirmwareManagementError> { let message = MqttMessage::new( - &C8yTopic::SmartRestResponse.to_topic(&self.config.c8y_prefix)?, + &C8yTopic::upstream_topic(&self.config.c8y_prefix), GET_PENDING_OPERATIONS.to_string(), ); self.mqtt_publisher.send(message).await?; From b31039234d819c8d615fc758b48b875a13f2652e Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Thu, 12 Dec 2024 19:48:50 +0000 Subject: [PATCH 13/17] Replace `@` syntax with `.profiles.` in configuration keys --- .../src/tedge_config_cli/figment.rs | 2 +- .../tedge_config_macros/examples/multi.rs | 20 +++++++---------- .../tedge_config_macros/impl/src/query.rs | 18 +++++++-------- crates/core/tedge/src/cli/connect/command.rs | 22 +++++++++---------- .../configuration_operation_multi_cloud.robot | 2 +- ...st_remote_access_with_cloud_profiles.robot | 2 +- .../tedge_connect_profile_validation.robot | 2 +- 7 files changed, 31 insertions(+), 37 deletions(-) diff --git a/crates/common/tedge_config/src/tedge_config_cli/figment.rs b/crates/common/tedge_config/src/tedge_config_cli/figment.rs index 05f3f00c2c5..6e077f42c60 100644 --- a/crates/common/tedge_config/src/tedge_config_cli/figment.rs +++ b/crates/common/tedge_config/src/tedge_config_cli/figment.rs @@ -216,7 +216,7 @@ impl TEdgeEnv { tracing::subscriber::NoSubscriber::default(), || lowercase_name.parse::(), ) - .map(|key| key.to_string().replace("@", ".profiles.")) + .map(|key| key.to_string()) .map_err(|err| { let is_read_only_key = matches!(err, crate::ParseKeyError::ReadOnly(_)); if is_read_only_key && !WARNINGS.lock().unwrap().insert(lowercase_name.clone()) { diff --git a/crates/common/tedge_config_macros/examples/multi.rs b/crates/common/tedge_config_macros/examples/multi.rs index ffdd2341376..7414c39ed98 100644 --- a/crates/common/tedge_config_macros/examples/multi.rs +++ b/crates/common/tedge_config_macros/examples/multi.rs @@ -85,20 +85,16 @@ fn main() { "c8y.url".parse::().unwrap(), ReadableKey::C8yUrl(None) ); - assert_eq!( - "c8y@cloud.url".parse::().unwrap(), - ReadableKey::C8yUrl(Some("cloud".to_owned())) - ); assert_eq!( "c8y.profiles.cloud.url".parse::().unwrap(), ReadableKey::C8yUrl(Some("cloud".to_owned())) ); assert_eq!( - "c8y@cloud.not_a_real_key" + "c8y.profiles.cloud.not_a_real_key" .parse::() .unwrap_err() .to_string(), - "Unknown key: 'c8y@cloud.not_a_real_key'" + "Unknown key: 'c8y.profiles.cloud.not_a_real_key'" ); assert_eq!( "c8y.urll".parse::().unwrap_err().to_string(), @@ -117,12 +113,12 @@ fn main() { "c8y.http", "c8y.smartrest.use_operation_id", "c8y.url", - "c8y@cloud.http", - "c8y@cloud.smartrest.use_operation_id", - "c8y@cloud.url", - "c8y@edge.http", - "c8y@edge.smartrest.use_operation_id", - "c8y@edge.url" + "c8y.profiles.cloud.http", + "c8y.profiles.cloud.smartrest.use_operation_id", + "c8y.profiles.cloud.url", + "c8y.profiles.edge.http", + "c8y.profiles.edge.smartrest.use_operation_id", + "c8y.profiles.edge.url" ] ); } diff --git a/crates/common/tedge_config_macros/impl/src/query.rs b/crates/common/tedge_config_macros/impl/src/query.rs index 39f3b77f052..69a5b1d1960 100644 --- a/crates/common/tedge_config_macros/impl/src/query.rs +++ b/crates/common/tedge_config_macros/impl/src/query.rs @@ -824,7 +824,7 @@ fn enum_variant(segments: &VecDeque<&FieldOrGroup>) -> ConfigurationKey { .iter() .map(|fog| match fog { FieldOrGroup::Multi(m) => { - format!("{}(?:(?:@|[\\._]profiles[\\._])([^\\.]+))?", m.ident) + format!("{}(?:[\\._]profiles[\\._]([^\\.]+))?", m.ident) } FieldOrGroup::Field(f) => f.ident().to_string(), FieldOrGroup::Group(g) => g.ident.to_string(), @@ -855,7 +855,7 @@ fn enum_variant(segments: &VecDeque<&FieldOrGroup>) -> ConfigurationKey { if *opt == none { m.ident.to_string() } else { - format!("{}@{{{}}}", m.ident, binding) + format!("{}.profiles.{{{}}}", m.ident, binding) } } FieldOrGroup::Group(g) => g.ident.to_string(), @@ -1009,11 +1009,9 @@ mod tests { /// /// This is used to verify both the output of the macro matches this regex /// and that the regex itself functions as intended - const C8Y_URL_REGEX: &str = "^c8y(?:(?:@|[\\._]profiles[\\._])([^\\.]+))?[\\._]url$"; + const C8Y_URL_REGEX: &str = "^c8y(?:[\\._]profiles[\\._]([^\\.]+))?[\\._]url$"; #[test_case("c8y.url", None; "with no profile specified")] - #[test_case("c8y@name.url", Some("name"); "with profile shorthand syntax")] - #[test_case("c8y@name_underscore.url", Some("name_underscore"); "with underscore profile shorthand syntax")] #[test_case("c8y.profiles.name.url", Some("name"); "with profile toml syntax")] #[test_case("c8y_profiles_name_url", Some("name"); "with environment variable profile")] #[test_case("c8y_profiles_name_underscore_url", Some("name_underscore"); "with environment variable underscore profile")] @@ -1027,7 +1025,7 @@ mod tests { #[test_case("not.c8y.url"; "with an invalid prefix")] #[test_case("c8y.url.something"; "with an invalid suffix")] - #[test_case("c8y@multiple.profile.sections.url"; "with an invalid profile name")] + #[test_case("c8y.profiles.multiple.profile.sections.url"; "with an invalid profile name")] fn regex_fails(input: &str) { let re = regex::Regex::new(C8Y_URL_REGEX).unwrap(); assert!(re.captures(input).is_none()); @@ -1306,7 +1304,7 @@ mod tests { pub fn to_cow_str(&self) -> ::std::borrow::Cow<'static, str> { match self { Self::C8yUrl(None) => ::std::borrow::Cow::Borrowed("c8y.url"), - Self::C8yUrl(Some(key0)) => ::std::borrow::Cow::Owned(format!("c8y@{key0}.url")), + Self::C8yUrl(Some(key0)) => ::std::borrow::Cow::Owned(format!("c8y.profiles.{key0}.url")), } } } @@ -1338,9 +1336,9 @@ mod tests { pub fn to_cow_str(&self) -> ::std::borrow::Cow<'static, str> { match self { Self::TopNestedField(None, None) => ::std::borrow::Cow::Borrowed("top.nested.field"), - Self::TopNestedField(None, Some(key1)) => ::std::borrow::Cow::Owned(format!("top.nested@{key1}.field")), - Self::TopNestedField(Some(key0), None) => ::std::borrow::Cow::Owned(format!("top@{key0}.nested.field")), - Self::TopNestedField(Some(key0), Some(key1)) => ::std::borrow::Cow::Owned(format!("top@{key0}.nested@{key1}.field")), + Self::TopNestedField(None, Some(key1)) => ::std::borrow::Cow::Owned(format!("top.nested.profiles.{key1}.field")), + Self::TopNestedField(Some(key0), None) => ::std::borrow::Cow::Owned(format!("top.profiles.{key0}.nested.field")), + Self::TopNestedField(Some(key0), Some(key1)) => ::std::borrow::Cow::Owned(format!("top.profiles.{key0}.nested.profiles.{key1}.field")), } } } diff --git a/crates/core/tedge/src/cli/connect/command.rs b/crates/core/tedge/src/cli/connect/command.rs index 4fe397caff0..e0f5d4e2714 100644 --- a/crates/core/tedge/src/cli/connect/command.rs +++ b/crates/core/tedge/src/cli/connect/command.rs @@ -1198,7 +1198,7 @@ mod tests { let ttd = TempTedgeDir::new(); let loc = TEdgeConfigLocation::from_custom_root(ttd.path()); loc.update_toml(&|dto, _| { - dto.try_update_str(&"c8y@new.url".parse().unwrap(), "example.com") + dto.try_update_str(&"c8y.profiles.new.url".parse().unwrap(), "example.com") .unwrap(); Ok(()) }) @@ -1214,7 +1214,7 @@ mod tests { let ttd = TempTedgeDir::new(); let loc = TEdgeConfigLocation::from_custom_root(ttd.path()); loc.update_toml(&|dto, _| { - dto.try_update_str(&"az@new.url".parse().unwrap(), "example.com") + dto.try_update_str(&"az.profiles.new.url".parse().unwrap(), "example.com") .unwrap(); Ok(()) }) @@ -1230,7 +1230,7 @@ mod tests { let ttd = TempTedgeDir::new(); let loc = TEdgeConfigLocation::from_custom_root(ttd.path()); loc.update_toml(&|dto, _| { - dto.try_update_str(&"aws@new.url".parse().unwrap(), "example.com") + dto.try_update_str(&"aws.profiles.new.url".parse().unwrap(), "example.com") .unwrap(); Ok(()) }) @@ -1248,9 +1248,9 @@ mod tests { loc.update_toml(&|dto, _| { dto.try_update_str(&"c8y.url".parse().unwrap(), "latest.example.com") .unwrap(); - dto.try_update_str(&"c8y@new.url".parse().unwrap(), "example.com") + dto.try_update_str(&"c8y.profiles.new.url".parse().unwrap(), "example.com") .unwrap(); - dto.try_update_str(&"c8y@new.proxy.bind.port".parse().unwrap(), "8002") + dto.try_update_str(&"c8y.profiles.new.proxy.bind.port".parse().unwrap(), "8002") .unwrap(); Ok(()) }) @@ -1260,7 +1260,7 @@ mod tests { let err = validate_config(&config, &cloud).unwrap_err(); eprintln!("err={err}"); assert!(err.to_string().contains("c8y.bridge.topic_prefix")); - assert!(err.to_string().contains("c8y@new.bridge.topic_prefix")); + assert!(err.to_string().contains("c8y.profiles.new.bridge.topic_prefix")); } #[test] @@ -1271,9 +1271,9 @@ mod tests { loc.update_toml(&|dto, _| { dto.try_update_str(&"c8y.url".parse().unwrap(), "latest.example.com") .unwrap(); - dto.try_update_str(&"c8y@new.url".parse().unwrap(), "example.com") + dto.try_update_str(&"c8y.profiles.new.url".parse().unwrap(), "example.com") .unwrap(); - dto.try_update_str(&"c8y@new.bridge.topic_prefix".parse().unwrap(), "c8y-new") + dto.try_update_str(&"c8y.profiles.new.bridge.topic_prefix".parse().unwrap(), "c8y-new") .unwrap(); Ok(()) }) @@ -1283,7 +1283,7 @@ mod tests { let err = validate_config(&config, &cloud).unwrap_err(); eprintln!("err={err}"); assert!(err.to_string().contains("c8y.proxy.bind.port")); - assert!(err.to_string().contains("c8y@new.proxy.bind.port")); + assert!(err.to_string().contains("c8y.profiles.new.proxy.bind.port")); } #[test] @@ -1294,7 +1294,7 @@ mod tests { loc.update_toml(&|dto, _| { dto.try_update_str(&"c8y.url".parse().unwrap(), "latest.example.com") .unwrap(); - dto.try_update_str(&"c8y@new.url".parse().unwrap(), "example.com") + dto.try_update_str(&"c8y.profiles.new.url".parse().unwrap(), "example.com") .unwrap(); Ok(()) }) @@ -1310,7 +1310,7 @@ mod tests { let ttd = TempTedgeDir::new(); let loc = TEdgeConfigLocation::from_custom_root(ttd.path()); loc.update_toml(&|dto, _| { - dto.try_update_str(&"az@new.bridge.topic_prefix".parse().unwrap(), "az-new") + dto.try_update_str(&"az.profiles.new.bridge.topic_prefix".parse().unwrap(), "az-new") .unwrap(); Ok(()) }) diff --git a/tests/RobotFramework/tests/cumulocity/configuration/configuration_operation_multi_cloud.robot b/tests/RobotFramework/tests/cumulocity/configuration/configuration_operation_multi_cloud.robot index 0d783332593..c6cf88d65b1 100644 --- a/tests/RobotFramework/tests/cumulocity/configuration/configuration_operation_multi_cloud.robot +++ b/tests/RobotFramework/tests/cumulocity/configuration/configuration_operation_multi_cloud.robot @@ -76,7 +76,7 @@ Setup Second Device Execute Command tedge config set c8y.bridge.topic_prefix --profile second c8y-second Execute Command tedge config set c8y.url --profile second "$(tedge config get c8y.url)" - Execute Command tedge cert create c8y --profile second --device-id ${second_device_sn} + Execute Command tedge cert create --device-id ${second_device_sn} c8y --profile second Execute Command ... cmd=sudo env C8Y_USER='${C8Y_CONFIG.username}' C8Y_PASSWORD='${C8Y_CONFIG.password}' tedge cert upload c8y --profile second diff --git a/tests/RobotFramework/tests/cumulocity/remote-access/test_remote_access_with_cloud_profiles.robot b/tests/RobotFramework/tests/cumulocity/remote-access/test_remote_access_with_cloud_profiles.robot index 6eb26758411..387619ac9bd 100644 --- a/tests/RobotFramework/tests/cumulocity/remote-access/test_remote_access_with_cloud_profiles.robot +++ b/tests/RobotFramework/tests/cumulocity/remote-access/test_remote_access_with_cloud_profiles.robot @@ -30,4 +30,4 @@ Custom Setup Start Service ssh Execute Command sed -i 's/\\[c8y\\]/\[c8y.profiles.test\]/g' /etc/tedge/tedge.toml Execute Command tedge disconnect c8y - Execute Command tedge connect c8y@test timeout=0 + Execute Command tedge connect c8y --profile test timeout=0 diff --git a/tests/RobotFramework/tests/tedge_connect/tedge_connect_profile_validation.robot b/tests/RobotFramework/tests/tedge_connect/tedge_connect_profile_validation.robot index 58220627312..ef6d7e47c7e 100644 --- a/tests/RobotFramework/tests/tedge_connect/tedge_connect_profile_validation.robot +++ b/tests/RobotFramework/tests/tedge_connect/tedge_connect_profile_validation.robot @@ -33,5 +33,5 @@ Verify conflicting configuration error appears ${output}= ThinEdgeIO.Execute Command ${command} exp_exit_code=1 stdout=False stderr=True timeout=0 Should Be Equal ... ${output} - ... error: The configurations: c8y.${conflicting_configuration}, c8y@second.${conflicting_configuration} should be set to different values before connecting, but are currently set to the same value\n + ... error: The configurations: c8y.${conflicting_configuration}, c8y.profiles.second.${conflicting_configuration} should be set to different values before connecting, but are currently set to the same value\n From 91c4cb508e5e9f80f99cfdd4523d98a11641b0dd Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Thu, 12 Dec 2024 19:49:18 +0000 Subject: [PATCH 14/17] Remove global from required device-id argument to fix tedge cert create --- crates/core/tedge/src/cli/certificate/cli.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/core/tedge/src/cli/certificate/cli.rs b/crates/core/tedge/src/cli/certificate/cli.rs index 599dc8b4466..a32320e2e07 100644 --- a/crates/core/tedge/src/cli/certificate/cli.rs +++ b/crates/core/tedge/src/cli/certificate/cli.rs @@ -21,7 +21,7 @@ pub enum TEdgeCertCli { /// Create a self-signed device certificate Create { /// The device identifier to be used as the common name for the certificate - #[clap(long = "device-id", global = true)] + #[clap(long = "device-id")] id: String, #[clap(subcommand)] From 35dc544cb15ff4392849c9054e146c57ace281f2 Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Thu, 12 Dec 2024 19:49:59 +0000 Subject: [PATCH 15/17] Document environment variable for cloud profile --- crates/core/tedge/src/cli/common.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/crates/core/tedge/src/cli/common.rs b/crates/core/tedge/src/cli/common.rs index 1f3eb9fdce1..45442c24339 100644 --- a/crates/core/tedge/src/cli/common.rs +++ b/crates/core/tedge/src/cli/common.rs @@ -9,16 +9,19 @@ use tedge_config::ProfileName; pub enum CloudArg { C8y { /// The cloud profile you wish to use + /// [env: TEDGE_CLOUD_PROFILE] #[clap(long)] profile: Option, }, Az { /// The cloud profile you wish to use + /// [env: TEDGE_CLOUD_PROFILE] #[clap(long)] profile: Option, }, Aws { /// The cloud profile you wish to use + /// [env: TEDGE_CLOUD_PROFILE] #[clap(long)] profile: Option, }, From 3409583a3973656ac63f1fe390d79511d3fa29fc Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Thu, 12 Dec 2024 20:44:41 +0000 Subject: [PATCH 16/17] Run formatter --- crates/core/tedge/src/cli/connect/command.rs | 18 +++++++++---- .../tedge_connect_profile_validation.robot | 26 +++++++++++++------ 2 files changed, 31 insertions(+), 13 deletions(-) diff --git a/crates/core/tedge/src/cli/connect/command.rs b/crates/core/tedge/src/cli/connect/command.rs index e0f5d4e2714..36661e23ffc 100644 --- a/crates/core/tedge/src/cli/connect/command.rs +++ b/crates/core/tedge/src/cli/connect/command.rs @@ -1260,7 +1260,9 @@ mod tests { let err = validate_config(&config, &cloud).unwrap_err(); eprintln!("err={err}"); assert!(err.to_string().contains("c8y.bridge.topic_prefix")); - assert!(err.to_string().contains("c8y.profiles.new.bridge.topic_prefix")); + assert!(err + .to_string() + .contains("c8y.profiles.new.bridge.topic_prefix")); } #[test] @@ -1273,8 +1275,11 @@ mod tests { .unwrap(); dto.try_update_str(&"c8y.profiles.new.url".parse().unwrap(), "example.com") .unwrap(); - dto.try_update_str(&"c8y.profiles.new.bridge.topic_prefix".parse().unwrap(), "c8y-new") - .unwrap(); + dto.try_update_str( + &"c8y.profiles.new.bridge.topic_prefix".parse().unwrap(), + "c8y-new", + ) + .unwrap(); Ok(()) }) .unwrap(); @@ -1310,8 +1315,11 @@ mod tests { let ttd = TempTedgeDir::new(); let loc = TEdgeConfigLocation::from_custom_root(ttd.path()); loc.update_toml(&|dto, _| { - dto.try_update_str(&"az.profiles.new.bridge.topic_prefix".parse().unwrap(), "az-new") - .unwrap(); + dto.try_update_str( + &"az.profiles.new.bridge.topic_prefix".parse().unwrap(), + "az-new", + ) + .unwrap(); Ok(()) }) .unwrap(); diff --git a/tests/RobotFramework/tests/tedge_connect/tedge_connect_profile_validation.robot b/tests/RobotFramework/tests/tedge_connect/tedge_connect_profile_validation.robot index ef6d7e47c7e..189e0483238 100644 --- a/tests/RobotFramework/tests/tedge_connect/tedge_connect_profile_validation.robot +++ b/tests/RobotFramework/tests/tedge_connect/tedge_connect_profile_validation.robot @@ -10,28 +10,38 @@ Test Tags theme:mqtt theme:c8y adapter:docker *** Test Cases *** Bridge topic prefix must be unique to connect - ThinEdgeIO.Execute Command sudo tedge config set c8y.url --profile second example.com timeout=0 + ThinEdgeIO.Execute Command sudo tedge config set c8y.url --profile second example.com timeout=0 Verify conflicting configuration error appears sudo tedge connect c8y --profile second bridge.topic_prefix Bridge topic prefix must be unique to test connection - ThinEdgeIO.Execute Command sudo tedge config set c8y.url --profile second example.com timeout=0 - Verify conflicting configuration error appears sudo tedge connect c8y --test --profile second bridge.topic_prefix + ThinEdgeIO.Execute Command sudo tedge config set c8y.url --profile second example.com timeout=0 + Verify conflicting configuration error appears + ... sudo tedge connect c8y --test --profile second + ... bridge.topic_prefix Device ID must be unique if cloud URLs are the same - ThinEdgeIO.Execute Command sudo tedge config set c8y.url --profile second "$(tedge config get c8y.url)" timeout=0 + ThinEdgeIO.Execute Command + ... sudo tedge config set c8y.url --profile second "$(tedge config get c8y.url)" + ... timeout=0 Verify conflicting configuration error appears sudo tedge connect c8y --profile second device.id Proxy bind port must be unique - ThinEdgeIO.Execute Command sudo tedge config set c8y.url --profile second example.com timeout=0 - ThinEdgeIO.Execute Command sudo tedge config set c8y.bridge.topic_prefix --profile second c8y-second timeout=0 + ThinEdgeIO.Execute Command sudo tedge config set c8y.url --profile second example.com timeout=0 + ThinEdgeIO.Execute Command + ... sudo tedge config set c8y.bridge.topic_prefix --profile second c8y-second + ... timeout=0 Verify conflicting configuration error appears sudo tedge connect c8y --profile second proxy.bind.port *** Keywords *** Verify conflicting configuration error appears [Arguments] ${command} ${conflicting_configuration} - ${output}= ThinEdgeIO.Execute Command ${command} exp_exit_code=1 stdout=False stderr=True timeout=0 + ${output}= ThinEdgeIO.Execute Command + ... ${command} + ... exp_exit_code=1 + ... stdout=False + ... stderr=True + ... timeout=0 Should Be Equal ... ${output} ... error: The configurations: c8y.${conflicting_configuration}, c8y.profiles.second.${conflicting_configuration} should be set to different values before connecting, but are currently set to the same value\n - From c077ab061bc29ecce6c4785e85de0f054e1af6da Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Fri, 13 Dec 2024 10:27:22 +0000 Subject: [PATCH 17/17] Improve help text for cloud profiles and correct url/device id validation Signed-off-by: James Rhodes --- Cargo.lock | 1 + crates/core/tedge/Cargo.toml | 1 + crates/core/tedge/src/cli/common.rs | 9 + crates/core/tedge/src/cli/config/cli.rs | 30 ++- crates/core/tedge/src/cli/connect/command.rs | 237 +++++++++++++++++- crates/core/tedge/src/cli/log.rs | 9 + .../tedge_connect_profile_validation.robot | 6 +- 7 files changed, 279 insertions(+), 14 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 7794e8217c4..fad2044cdfa 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3834,6 +3834,7 @@ dependencies = [ "pad", "pem 1.1.1", "predicates 2.1.5", + "rcgen", "reqwest", "rpassword", "rumqttc", diff --git a/crates/core/tedge/Cargo.toml b/crates/core/tedge/Cargo.toml index e21edde3a33..947a8086289 100644 --- a/crates/core/tedge/Cargo.toml +++ b/crates/core/tedge/Cargo.toml @@ -62,6 +62,7 @@ mockito = { workspace = true } mqtt_tests = { workspace = true } pem = { workspace = true } predicates = { workspace = true } +rcgen = { workspace = true } tedge_test_utils = { workspace = true } tempfile = { workspace = true } test-case = { workspace = true } diff --git a/crates/core/tedge/src/cli/common.rs b/crates/core/tedge/src/cli/common.rs index 45442c24339..842a826bc64 100644 --- a/crates/core/tedge/src/cli/common.rs +++ b/crates/core/tedge/src/cli/common.rs @@ -9,18 +9,21 @@ use tedge_config::ProfileName; pub enum CloudArg { C8y { /// The cloud profile you wish to use + /// /// [env: TEDGE_CLOUD_PROFILE] #[clap(long)] profile: Option, }, Az { /// The cloud profile you wish to use + /// /// [env: TEDGE_CLOUD_PROFILE] #[clap(long)] profile: Option, }, Aws { /// The cloud profile you wish to use + /// /// [env: TEDGE_CLOUD_PROFILE] #[clap(long)] profile: Option, @@ -148,4 +151,10 @@ impl MaybeBorrowedCloud<'_> { Self::Azure(Some(profile)) => format!("az@{profile}-bridge.conf").into(), } } + + pub fn profile_name(&self) -> Option<&ProfileName> { + match self { + Self::Aws(profile) | Self::Azure(profile) | Self::C8y(profile) => profile.as_deref(), + } + } } diff --git a/crates/core/tedge/src/cli/config/cli.rs b/crates/core/tedge/src/cli/config/cli.rs index 50024a7a0f5..cfe63e51562 100644 --- a/crates/core/tedge/src/cli/config/cli.rs +++ b/crates/core/tedge/src/cli/config/cli.rs @@ -12,7 +12,11 @@ pub enum ConfigCmd { /// Configuration key. Run `tedge config list --doc` for available keys key: ReadableKey, - /// Cloud profile + /// The cloud profile you wish to use, if accessing a cloud configuration + /// (i.e. `c8y.*`, `az.*` or `aws.*`). If you don't wish to use cloud profiles, + /// or want to access the default profile, don't supply this. + /// + /// [env: TEDGE_CLOUD_PROFILE] #[clap(long)] profile: Option, }, @@ -25,7 +29,11 @@ pub enum ConfigCmd { /// Configuration value. value: String, - /// Cloud profile + /// The cloud profile you wish to use, if accessing a cloud configuration + /// (i.e. `c8y.*`, `az.*` or `aws.*`). If you don't wish to use cloud profiles, + /// or want to access the default profile, don't supply this. + /// + /// [env: TEDGE_CLOUD_PROFILE] #[clap(long)] profile: Option, }, @@ -35,7 +43,11 @@ pub enum ConfigCmd { /// Configuration key. Run `tedge config list --doc` for available keys key: WritableKey, - /// Cloud profile + /// The cloud profile you wish to use, if accessing a cloud configuration + /// (i.e. `c8y.*`, `az.*` or `aws.*`). If you don't wish to use cloud profiles, + /// or want to access the default profile, don't supply this. + /// + /// [env: TEDGE_CLOUD_PROFILE] #[clap(long)] profile: Option, }, @@ -48,7 +60,11 @@ pub enum ConfigCmd { /// Configuration value. value: String, - /// Cloud profile + /// The cloud profile you wish to use, if accessing a cloud configuration + /// (i.e. `c8y.*`, `az.*` or `aws.*`). If you don't wish to use cloud profiles, + /// or want to access the default profile, don't supply this. + /// + /// [env: TEDGE_CLOUD_PROFILE] #[clap(long)] profile: Option, }, @@ -61,8 +77,12 @@ pub enum ConfigCmd { /// Configuration value. value: String, + /// The cloud profile you wish to use, if accessing a cloud configuration + /// (i.e. `c8y.*`, `az.*` or `aws.*`). If you don't wish to use cloud profiles, + /// or want to access the default profile, don't supply this. + /// + /// [env: TEDGE_CLOUD_PROFILE] #[clap(long)] - /// Cloud profile profile: Option, }, diff --git a/crates/core/tedge/src/cli/connect/command.rs b/crates/core/tedge/src/cli/connect/command.rs index 36661e23ffc..c6f5be10daf 100644 --- a/crates/core/tedge/src/cli/connect/command.rs +++ b/crates/core/tedge/src/cli/connect/command.rs @@ -96,6 +96,12 @@ impl ConnectCommand { validate_config(config, &self.cloud)?; if self.is_test_connection { + ConfigLogger::log( + format!("Testing {} connection with config", self.cloud), + &bridge_config, + &*self.service_manager, + &self.cloud, + ); // If the bridge is part of the mapper, the bridge config file won't exist // TODO tidy me up once mosquitto is no longer required for bridge return if self.check_if_bridge_exists(&bridge_config) { @@ -129,7 +135,7 @@ impl ConnectCommand { false => format!("Connecting to {} with config", self.cloud), true => "Reconnecting with config".into(), }; - ConfigLogger::log(title, &bridge_config, &*self.service_manager); + ConfigLogger::log(title, &bridge_config, &*self.service_manager, &self.cloud); let device_type = &config.device.ty; @@ -288,8 +294,11 @@ fn validate_config(config: &TEdgeConfig, cloud: &MaybeBorrowedCloud<'_>) -> anyh .filter(|(_, config)| config.url.or_none().is_some()) .map(|(s, _)| Some(s?.to_string())) .collect::>(); - disallow_matching_configurations(config, ReadableKey::AwsUrl, &profiles).or_else( - |_| disallow_matching_configurations(config, ReadableKey::AwsDeviceId, &profiles), + disallow_matching_url_device_id( + config, + ReadableKey::AwsUrl, + ReadableKey::AwsDeviceId, + &profiles, )?; disallow_matching_configurations(config, ReadableKey::AwsBridgeTopicPrefix, &profiles)?; } @@ -300,8 +309,11 @@ fn validate_config(config: &TEdgeConfig, cloud: &MaybeBorrowedCloud<'_>) -> anyh .filter(|(_, config)| config.url.or_none().is_some()) .map(|(s, _)| Some(s?.to_string())) .collect::>(); - disallow_matching_configurations(config, ReadableKey::AzUrl, &profiles).or_else( - |_| disallow_matching_configurations(config, ReadableKey::AzDeviceId, &profiles), + disallow_matching_url_device_id( + config, + ReadableKey::AzUrl, + ReadableKey::AzDeviceId, + &profiles, )?; disallow_matching_configurations(config, ReadableKey::AzBridgeTopicPrefix, &profiles)?; } @@ -312,8 +324,11 @@ fn validate_config(config: &TEdgeConfig, cloud: &MaybeBorrowedCloud<'_>) -> anyh .filter(|(_, config)| config.http.or_none().is_some()) .map(|(s, _)| Some(s?.to_string())) .collect::>(); - disallow_matching_configurations(config, ReadableKey::C8yUrl, &profiles).or_else( - |_| disallow_matching_configurations(config, ReadableKey::C8yDeviceId, &profiles), + disallow_matching_url_device_id( + config, + ReadableKey::C8yUrl, + ReadableKey::C8yDeviceId, + &profiles, )?; disallow_matching_configurations(config, ReadableKey::C8yBridgeTopicPrefix, &profiles)?; disallow_matching_configurations(config, ReadableKey::C8yProxyBindPort, &profiles)?; @@ -322,6 +337,49 @@ fn validate_config(config: &TEdgeConfig, cloud: &MaybeBorrowedCloud<'_>) -> anyh Ok(()) } +fn disallow_matching_url_device_id( + config: &TEdgeConfig, + url: fn(Option) -> ReadableKey, + device_id: fn(Option) -> ReadableKey, + profiles: &[Option], +) -> anyhow::Result<()> { + let url_entries = profiles.iter().map(|profile| { + let key = url(profile.clone()); + let value = config.read_string(&key).ok(); + ((profile, key), value) + }); + + for url_matches in find_all_matching(url_entries) { + let device_id_entries = profiles.iter().filter_map(|profile| { + url_matches.iter().find(|(p, _)| *p == profile)?; + let key = device_id(profile.clone()); + let value = config.read_string(&key).ok(); + Some(((profile, key), value)) + }); + if let Some(matches) = find_matching(device_id_entries) { + let url_keys: String = matches + .iter() + .map(|&(k, _)| format!("{}", url(k.clone()).yellow().bold())) + .collect::>() + .join(", "); + let device_id_keys: String = matches + .iter() + .map(|(_, key)| format!("{}", key.yellow().bold())) + .collect::>() + .join(", "); + bail!( + "You have matching URLs and device IDs for different profiles. + +{url_keys} are set to the same value, but so are {device_id_keys}. + +Each cloud profile requires either a unique URL or unique device ID, \ +so it corresponds to a unique device in the associated cloud." + ); + } + } + Ok(()) +} + fn disallow_matching_configurations( config: &TEdgeConfig, configuration: fn(Option) -> ReadableKey, @@ -337,7 +395,7 @@ fn disallow_matching_configurations( Some((key, value)) }); if let Some(matches) = find_matching(entries) { - let keys = matches + let keys: String = matches .iter() .map(|k| format!("{}", k.yellow().bold())) .collect::>() @@ -357,6 +415,15 @@ fn find_matching(entries: impl Iterator) -> Opti match_map.into_values().find(|t| t.len() > 1) } +fn find_all_matching(entries: impl Iterator) -> Vec> { + let match_map = entries.fold(HashMap::>::new(), |mut acc, (key, value)| { + acc.entry(value).or_default().push(key); + acc + }); + + match_map.into_values().filter(|t| t.len() > 1).collect() +} + pub fn bridge_config( config: &TEdgeConfig, cloud: &MaybeBorrowedCloud<'_>, @@ -1208,6 +1275,160 @@ mod tests { validate_config(&config, &cloud).unwrap(); } + #[test] + fn disallows_matching_device_id_same_urls() { + yansi::disable(); + let cloud = Cloud::c8y(Some("new".parse().unwrap())); + let ttd = TempTedgeDir::new(); + let loc = TEdgeConfigLocation::from_custom_root(ttd.path()); + loc.update_toml(&|dto, _| { + dto.try_update_str(&"c8y.url".parse().unwrap(), "example.com") + .unwrap(); + dto.try_update_str(&"c8y.profiles.new.url".parse().unwrap(), "example.com") + .unwrap(); + Ok(()) + }) + .unwrap(); + let config = loc.load().unwrap(); + + let err = validate_config(&config, &cloud).unwrap_err(); + assert_eq!(err.to_string(), "You have matching URLs and device IDs for different profiles. + +c8y.url, c8y.profiles.new.url are set to the same value, but so are c8y.device.id, c8y.profiles.new.device.id. + +Each cloud profile requires either a unique URL or unique device ID, so it corresponds to a unique device in the associated cloud.") + } + + #[test] + fn allows_different_urls() { + let cloud = Cloud::c8y(Some("new".parse().unwrap())); + let ttd = TempTedgeDir::new(); + let loc = TEdgeConfigLocation::from_custom_root(ttd.path()); + loc.update_toml(&|dto, _| { + dto.try_update_str(&"c8y.url".parse().unwrap(), "example.com") + .unwrap(); + dto.try_update_str( + &"c8y.profiles.new.url".parse().unwrap(), + "different.example.com", + ) + .unwrap(); + dto.try_update_str( + &"c8y.profiles.new.bridge.topic_prefix".parse().unwrap(), + "c8y-new", + ) + .unwrap(); + dto.try_update_str(&"c8y.profiles.new.proxy.bind.port".parse().unwrap(), "8002") + .unwrap(); + Ok(()) + }) + .unwrap(); + let config = loc.load().unwrap(); + + validate_config(&config, &cloud).unwrap(); + } + + #[test] + fn allows_different_device_ids() { + let cloud = Cloud::c8y(Some("new".parse().unwrap())); + let ttd = TempTedgeDir::new(); + let cert = rcgen::generate_simple_self_signed(["test-device".into()]).unwrap(); + let mut cert_path = ttd.path().to_owned(); + cert_path.push("test.crt"); + let mut key_path = ttd.path().to_owned(); + key_path.push("test.key"); + std::fs::write(&cert_path, cert.serialize_pem().unwrap()).unwrap(); + std::fs::write(&key_path, cert.serialize_private_key_pem()).unwrap(); + let loc = TEdgeConfigLocation::from_custom_root(ttd.path()); + loc.update_toml(&|dto, _| { + dto.try_update_str(&"c8y.url".parse().unwrap(), "example.com") + .unwrap(); + dto.try_update_str(&"c8y.profiles.new.url".parse().unwrap(), "example.com") + .unwrap(); + dto.try_update_str( + &"c8y.profiles.new.device.cert_path".parse().unwrap(), + &cert_path.display().to_string(), + ) + .unwrap(); + dto.try_update_str( + &"c8y.profiles.new.device.key_path".parse().unwrap(), + &key_path.display().to_string(), + ) + .unwrap(); + dto.try_update_str( + &"c8y.profiles.new.bridge.topic_prefix".parse().unwrap(), + "c8y-new", + ) + .unwrap(); + dto.try_update_str(&"c8y.profiles.new.proxy.bind.port".parse().unwrap(), "8002") + .unwrap(); + Ok(()) + }) + .unwrap(); + let config = loc.load().unwrap(); + + validate_config(&config, &cloud).unwrap(); + } + + #[test] + fn allows_combination_of_urls_and_device_ids() { + let cloud = Cloud::c8y(Some("new".parse().unwrap())); + let ttd = TempTedgeDir::new(); + let cert = rcgen::generate_simple_self_signed(["test-device".into()]).unwrap(); + let mut cert_path = ttd.path().to_owned(); + cert_path.push("test.crt"); + let mut key_path = ttd.path().to_owned(); + key_path.push("test.key"); + std::fs::write(&cert_path, cert.serialize_pem().unwrap()).unwrap(); + std::fs::write(&key_path, cert.serialize_private_key_pem()).unwrap(); + let loc = TEdgeConfigLocation::from_custom_root(ttd.path()); + loc.update_toml(&|dto, _| { + dto.try_update_str(&"c8y.url".parse().unwrap(), "example.com") + .unwrap(); + dto.try_update_str(&"c8y.profiles.diff_id.url".parse().unwrap(), "example.com") + .unwrap(); + dto.try_update_str( + &"c8y.profiles.diff_id.device.cert_path".parse().unwrap(), + &cert_path.display().to_string(), + ) + .unwrap(); + dto.try_update_str( + &"c8y.profiles.diff_id.device.key_path".parse().unwrap(), + &key_path.display().to_string(), + ) + .unwrap(); + dto.try_update_str( + &"c8y.profiles.diff_id.bridge.topic_prefix".parse().unwrap(), + "c8y-diff-id", + ) + .unwrap(); + dto.try_update_str( + &"c8y.profiles.diff_id.proxy.bind.port".parse().unwrap(), + "8002", + ) + .unwrap(); + dto.try_update_str( + &"c8y.profiles.diff_url.url".parse().unwrap(), + "different.example.com", + ) + .unwrap(); + dto.try_update_str( + &"c8y.profiles.diff_url.bridge.topic_prefix".parse().unwrap(), + "c8y-diff-url", + ) + .unwrap(); + dto.try_update_str( + &"c8y.profiles.diff_url.proxy.bind.port".parse().unwrap(), + "8003", + ) + .unwrap(); + Ok(()) + }) + .unwrap(); + let config = loc.load().unwrap(); + + validate_config(&config, &cloud).unwrap(); + } + #[test] fn allows_single_named_az_profile_without_default_profile() { let cloud = Cloud::az(Some("new".parse().unwrap())); diff --git a/crates/core/tedge/src/cli/log.rs b/crates/core/tedge/src/cli/log.rs index aa134c1e4db..a5f1630ff6d 100644 --- a/crates/core/tedge/src/cli/log.rs +++ b/crates/core/tedge/src/cli/log.rs @@ -16,6 +16,7 @@ use crate::bridge::BridgeConfig; use crate::bridge::BridgeLocation; use crate::error::TEdgeError; +use super::common::MaybeBorrowedCloud; use super::disconnect::error::DisconnectBridgeError; use super::CertError; use super::ConnectError; @@ -249,6 +250,7 @@ pub struct ConfigLogger<'a> { auth_method: Option, service_manager: &'a dyn SystemServiceManager, mosquitto_version: Option<&'a str>, + cloud: &'a MaybeBorrowedCloud<'a>, } impl<'a> ConfigLogger<'a> { @@ -257,6 +259,7 @@ impl<'a> ConfigLogger<'a> { title: impl Into>, config: &'a BridgeConfig, service_manager: &'a dyn SystemServiceManager, + cloud: &'a MaybeBorrowedCloud<'a>, ) { println!( "{}", @@ -269,6 +272,7 @@ impl<'a> ConfigLogger<'a> { auth_method: None, service_manager, mosquitto_version: config.mosquitto_version.as_deref(), + cloud, } ) } @@ -292,6 +296,11 @@ impl fmt::Display for ConfigLogger<'_> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!(f, "{}:", self.title)?; self.log_single_entry(f, "device id", &self.device_id)?; + if let Some(profile) = self.cloud.profile_name() { + self.log_single_entry(f, "cloud profile", profile)?; + } else { + self.log_single_entry(f, "cloud profile", &"")?; + } self.log_single_entry(f, "cloud host", &self.cloud_host)?; self.log_single_entry(f, "certificate file", &self.cert_path)?; self.log_single_entry(f, "bridge", &self.bridge_location)?; diff --git a/tests/RobotFramework/tests/tedge_connect/tedge_connect_profile_validation.robot b/tests/RobotFramework/tests/tedge_connect/tedge_connect_profile_validation.robot index 189e0483238..0a2db48dfe6 100644 --- a/tests/RobotFramework/tests/tedge_connect/tedge_connect_profile_validation.robot +++ b/tests/RobotFramework/tests/tedge_connect/tedge_connect_profile_validation.robot @@ -23,7 +23,11 @@ Device ID must be unique if cloud URLs are the same ThinEdgeIO.Execute Command ... sudo tedge config set c8y.url --profile second "$(tedge config get c8y.url)" ... timeout=0 - Verify conflicting configuration error appears sudo tedge connect c8y --profile second device.id + # Just assert the command fails, we have unit tests that assert the correct error message appears + ThinEdgeIO.Execute Command + ... sudo tedge connect c8y --test --profile second + ... exp_exit_code=1 + ... timeout=0 Proxy bind port must be unique ThinEdgeIO.Execute Command sudo tedge config set c8y.url --profile second example.com timeout=0