Skip to content

Commit

Permalink
Improve help text for cloud profiles and correct url/device id valida…
Browse files Browse the repository at this point in the history 
…tion

Signed-off-by: James Rhodes <[email protected]>
  • Loading branch information
@jarhodes314
jarhodes314 committed Dec 13, 2024
1 parent 3409583 commit 02a73aa
Show file tree
Hide file tree
Showing 5 changed files with 243 additions and 6 deletions.
1 change: 1 addition & 0 deletions Cargo.lock
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions crates/core/tedge/Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Expand Down
9 changes: 9 additions & 0 deletions crates/core/tedge/src/cli/common.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<ProfileName>,
},
Az {
/// The cloud profile you wish to use
///
/// [env: TEDGE_CLOUD_PROFILE]
#[clap(long)]
profile: Option<ProfileName>,
},
Aws {
/// The cloud profile you wish to use
///
/// [env: TEDGE_CLOUD_PROFILE]
#[clap(long)]
profile: Option<ProfileName>,
Expand Down Expand Up @@ -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(),
}
}
}
30 changes: 25 additions & 5 deletions crates/core/tedge/src/cli/config/cli.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<ProfileName>,
},
Expand All @@ -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<ProfileName>,
},
Expand All @@ -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<ProfileName>,
},
Expand All @@ -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<ProfileName>,
},
Expand All @@ -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<ProfileName>,
},

Expand Down
208 changes: 207 additions & 1 deletion crates/core/tedge/src/cli/connect/command.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,49 @@ fn validate_config(config: &TEdgeConfig, cloud: &MaybeBorrowedCloud<'_>) -> anyh
Ok(())
}

fn disallow_matching_url_device_id(
config: &TEdgeConfig,
url: fn(Option<String>) -> ReadableKey,
device_id: fn(Option<String>) -> ReadableKey,
profiles: &[Option<String>],
) -> anyhow::Result<()> {
let url_entries = profiles.into_iter().filter_map(|profile| {
let key = url(profile.clone());
let value = config.read_string(&key).ok();
Some(((profile, key), value))
});

for url_matches in find_all_matching(url_entries) {
let device_id_entries = profiles.into_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::<Vec<_>>()
.join(", ");
let device_id_keys: String = matches
.iter()
.map(|(_, key)| format!("{}", key.yellow().bold()))
.collect::<Vec<_>>()
.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<String>) -> ReadableKey,
Expand All @@ -337,7 +380,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::<Vec<_>>()
Expand All @@ -357,6 +400,15 @@ fn find_matching<K, V: Hash + Eq>(entries: impl Iterator<Item = (K, V)>) -> Opti
match_map.into_values().find(|t| t.len() > 1)
}

fn find_all_matching<K, V: Hash + Eq>(entries: impl Iterator<Item = (K, V)>) -> Vec<Vec<K>> {
let match_map = entries.fold(HashMap::<V, Vec<K>>::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<'_>,
Expand Down Expand Up @@ -1208,6 +1260,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()));
Expand Down

0 comments on commit 02a73aa

Please sign in to comment.