From 906f3a7b18981bf914e034cc1e464cf82bf56723 Mon Sep 17 00:00:00 2001 From: James Casey Date: Thu, 6 Dec 2018 04:46:07 -0800 Subject: [PATCH 1/3] Remove panic by moving problem code out of constructor The root of the panic is due to us trying to serialize a `Toml::Value::Table` to a Vec inside a constructor where we can't handle the error cleanly This commit pulls the TOML parsing out of Service::new() into Cfg::to_exported() so we can properly handle the Result<> on toml parsing Note: this does not yet handle the underlying issue which is that the TOML serializer doesn't handle tables well. In particular it requires all keys with non-table values must be emitted first. This is not the default See https://github.com/alexcrichton/toml-rs/blob/master/src/ser.rs#L7-L12 Signed-off-by: James Casey --- components/butterfly/src/rumor/service.rs | 8 ++------ components/sup/src/manager/service/config.rs | 4 ++-- components/sup/src/manager/service/mod.rs | 2 +- 3 files changed, 5 insertions(+), 9 deletions(-) diff --git a/components/butterfly/src/rumor/service.rs b/components/butterfly/src/rumor/service.rs index 895fd0c570..bbb89e89ec 100644 --- a/components/butterfly/src/rumor/service.rs +++ b/components/butterfly/src/rumor/service.rs @@ -87,7 +87,7 @@ impl Service { package: &T, service_group: ServiceGroup, sys: SysInfo, - cfg: Option<&toml::value::Table>, + cfg: Option>, ) -> Self where T: Identifiable, @@ -110,11 +110,7 @@ impl Service { initialized: false, pkg: package.to_string(), sys: sys, - // TODO FN: Can we really expect this all the time, should we return a `Result` - // in this constructor? - cfg: cfg - .map(|v| toml::ser::to_vec(v).expect("Struct should serialize to bytes")) - .unwrap_or_default(), + cfg: cfg.unwrap_or_default(), } } } diff --git a/components/sup/src/manager/service/config.rs b/components/sup/src/manager/service/config.rs index 2d35a6ae77..ccf2d4f3df 100644 --- a/components/sup/src/manager/service/config.rs +++ b/components/sup/src/manager/service/config.rs @@ -227,7 +227,7 @@ impl Cfg { } /// Returns a subset of the overall configuration which intersects with the given package's exports. - pub fn to_exported(&self, pkg: &Pkg) -> Result { + pub fn to_exported(&self, pkg: &Pkg) -> Result> { let mut map = toml::value::Table::default(); let cfg = toml::Value::try_from(&self).unwrap(); for (key, path) in pkg.exports.iter() { @@ -253,7 +253,7 @@ impl Cfg { map.insert(key.clone(), curr.clone()); } } - Ok(map) + toml::ser::to_vec(&map).map_err(|e| sup_error!(Error::TomlEncode(e))) } fn load_toml_file(dir: T1, file: T2) -> Result> diff --git a/components/sup/src/manager/service/mod.rs b/components/sup/src/manager/service/mod.rs index f1fd0a6528..26a0e94415 100644 --- a/components/sup/src/manager/service/mod.rs +++ b/components/sup/src/manager/service/mod.rs @@ -681,7 +681,7 @@ impl Service { &self.pkg.ident, self.service_group.clone(), self.sys.as_sys_info().clone(), - exported.as_ref(), + exported, ); rumor.incarnation = incarnation; rumor From 3316006c0edcb36f55f88e8726d16619e22d08ec Mon Sep 17 00:00:00 2001 From: James Casey Date: Fri, 14 Dec 2018 15:19:21 -0800 Subject: [PATCH 2/3] Use custom serialize function for exports to handle nested tables in exported config Abstract out existing special casing for serialization of config and re-use it when creating the exported config Signed-off-by: James Casey --- components/sup/src/manager/service/config.rs | 132 +++++++++++++++---- components/sup/src/manager/service/mod.rs | 2 +- 2 files changed, 110 insertions(+), 24 deletions(-) diff --git a/components/sup/src/manager/service/config.rs b/components/sup/src/manager/service/config.rs index ccf2d4f3df..80de4558dc 100644 --- a/components/sup/src/manager/service/config.rs +++ b/components/sup/src/manager/service/config.rs @@ -15,6 +15,7 @@ /// Collect all the configuration data that is exposed to users, and render it. use std; use std::borrow::Cow; +use std::collections::HashMap; use std::env; use std::fs::File; use std::io::prelude::*; @@ -29,6 +30,7 @@ use serde::{Serialize, Serializer}; use serde_json; use serde_transcode; use toml; +use toml::ser; use super::Pkg; use crate::census::CensusGroup; @@ -226,11 +228,11 @@ impl Cfg { } } - /// Returns a subset of the overall configuration which intersects with the given package's exports. - pub fn to_exported(&self, pkg: &Pkg) -> Result> { + /// Returns a subset of the overall configuration which intersects with the given package exports. + pub fn to_exported(&self, exports: &HashMap) -> Result> { let mut map = toml::value::Table::default(); let cfg = toml::Value::try_from(&self).unwrap(); - for (key, path) in pkg.exports.iter() { + for (key, path) in exports.iter() { let fields: Vec<&str> = path.split('.').collect(); let mut curr = &cfg; let mut found = false; @@ -253,7 +255,12 @@ impl Cfg { map.insert(key.clone(), curr.clone()); } } - toml::ser::to_vec(&map).map_err(|e| sup_error!(Error::TomlEncode(e))) + + let mut dst = String::with_capacity(128); + match serialize_table(&map, &mut ser::Serializer::new(&mut dst)) { + Ok(_) => Ok(dst.into_bytes()), + Err(e) => Err(sup_error!(Error::TomlEncode(e))), + } } fn load_toml_file(dir: T1, file: T2) -> Result> @@ -435,29 +442,36 @@ impl Serialize for Cfg { } } - // Be sure to visit non-tables first (and also non - // array-of-tables) as all keys must be emitted first. - let mut map = serializer.serialize_map(Some(table.len()))?; - for (k, v) in &table { - if !v.is_array() && !v.is_table() { - map.serialize_key(&k)?; - map.serialize_value(&v)?; - } + serialize_table(&table, serializer) + } +} + +fn serialize_table(table: &toml::value::Table, serializer: S) -> result::Result +where + S: Serializer, +{ + // Be sure to visit non-tables first (and also non + // array-of-tables) as all keys must be emitted first. + let mut map = serializer.serialize_map(Some(table.len()))?; + for (k, v) in table { + if !v.is_array() && !v.is_table() { + map.serialize_key(&k)?; + map.serialize_value(&v)?; } - for (k, v) in &table { - if v.is_array() { - map.serialize_key(&k)?; - map.serialize_value(&v)?; - } + } + for (k, v) in table { + if v.is_array() { + map.serialize_key(&k)?; + map.serialize_value(&v)?; } - for (k, v) in &table { - if v.is_table() { - map.serialize_key(&k)?; - map.serialize_value(&v)?; - } + } + for (k, v) in table { + if v.is_table() { + map.serialize_key(&k)?; + map.serialize_value(&v)?; } - map.end() } + map.end() } #[derive(Debug)] @@ -642,10 +656,14 @@ mod test { use std::env; use std::fs; use std::fs::OpenOptions; + use std::str; use tempfile::TempDir; use toml; + use hcore::package::{PackageIdent, PackageInstall}; + use hcore::service::ServiceGroup; + use super::*; use crate::error::Error; @@ -1076,4 +1094,72 @@ mod test { } } + fn pkg_with_exports(exports: HashMap) -> Pkg { + let service_group = ServiceGroup::new(None, "dummy", "service", None) + .expect("couldn't create ServiceGroup"); + let pg_id = PackageIdent::new( + "testing", + &service_group.service(), + Some("1.0.0"), + Some("20170712000000"), + ); + + let pkg_install = PackageInstall::new_from_parts( + pg_id.clone(), + PathBuf::from("/tmp"), + PathBuf::from("/tmp"), + PathBuf::from("/tmp"), + ); + + let mut pkg = Pkg::from_install(pkg_install).expect("Could not create package!"); + pkg.exports = exports; + pkg + } + + #[test] + fn to_exported_test() { + let export_map = [("a".to_string(), "foo".to_string())] + .iter() + .cloned() + .collect(); + let pkg = pkg_with_exports(export_map); + + let cfg_data = CfgTestData::new(); + let toml = "foo = 13"; + write_toml(&cfg_data.ducp, toml); + let cfg = Cfg::new(&cfg_data.pkg, None).expect("create config"); + + let result = cfg.to_exported(&pkg.exports).expect("export"); + assert_eq!("a = 13\n", str::from_utf8(&result).unwrap()); + } + + /// TOML encoding by default doesn't handle tables being in the middle of the output. + /// Let's check if we correctly put tables at the end + #[test] + fn nested_table_to_exported_test() { + let export_map = [ + ("a".to_string(), "server".to_string()), + ("b".to_string(), "frubnub".to_string()), + ] + .iter() + .cloned() + .collect(); + let pkg = pkg_with_exports(export_map); + + let cfg_data = CfgTestData::new(); + let toml = r#" + frubnub = "foobar" + + [server] + port = 5000 + "#; + write_toml(&cfg_data.ducp, toml); + let cfg = Cfg::new(&cfg_data.pkg, None).expect("create config"); + + let result = cfg.to_exported(&pkg.exports).expect("export"); + assert_eq!( + "b = \"foobar\"\n\n[a]\nport = 5000\n", + str::from_utf8(&result).unwrap() + ); + } } diff --git a/components/sup/src/manager/service/mod.rs b/components/sup/src/manager/service/mod.rs index 26a0e94415..67ae7ef490 100644 --- a/components/sup/src/manager/service/mod.rs +++ b/components/sup/src/manager/service/mod.rs @@ -667,7 +667,7 @@ impl Service { } pub fn to_rumor(&self, incarnation: u64) -> ServiceRumor { - let exported = match self.cfg.to_exported(&self.pkg) { + let exported = match self.cfg.to_exported(&self.pkg.exports) { Ok(exported) => Some(exported), Err(err) => { outputln!(preamble self.service_group, From 26457fa32946a654d4da2280b44e9ec8e242ccf4 Mon Sep 17 00:00:00 2001 From: James Casey Date: Fri, 11 Jan 2019 10:09:54 -0800 Subject: [PATCH 3/3] review commits Signed-off-by: James Casey --- components/butterfly/src/rumor/service.rs | 29 ++++- components/sup/src/manager/service/config.rs | 122 +------------------ components/sup/src/manager/service/mod.rs | 2 +- 3 files changed, 34 insertions(+), 119 deletions(-) diff --git a/components/butterfly/src/rumor/service.rs b/components/butterfly/src/rumor/service.rs index bbb89e89ec..7097a3e143 100644 --- a/components/butterfly/src/rumor/service.rs +++ b/components/butterfly/src/rumor/service.rs @@ -87,7 +87,7 @@ impl Service { package: &T, service_group: ServiceGroup, sys: SysInfo, - cfg: Option>, + cfg: Option, ) -> Self where T: Identifiable, @@ -110,7 +110,15 @@ impl Service { initialized: false, pkg: package.to_string(), sys: sys, - cfg: cfg.unwrap_or_default(), + cfg: cfg + .map(|v| { + // Directly serializing a toml::value::Table can lead to an error + // Wrapping it in a toml::value::Value makes this operation safe + // See https://github.com/alexcrichton/toml-rs/issues/142 + toml::ser::to_vec(&toml::value::Value::Table(v)) + .expect("Struct should serialize to bytes") + }) + .unwrap_or_default(), } } } @@ -348,4 +356,21 @@ mod tests { None, ); } + + #[test] + fn service_cfg_serialization() { + let package: PackageIdent = "core/foo/1.0.0/20180701125610".parse().unwrap(); + let sg = ServiceGroup::new(None, "foo", "default", None).unwrap(); + + // This map contains a scalar value and a table such that the serialization order + // would trigger a ValueAfterTable error. This test ensures we avoid it. + // See https://github.com/habitat-sh/habitat/issues/5854 + // See https://github.com/alexcrichton/toml-rs/issues/142 + let mut map = toml::value::Table::default(); + let sub_map = toml::value::Table::default(); + map.insert("foo".into(), 5.into()); + map.insert("a".into(), sub_map.into()); + + Service::new("member_id_val", &package, sg, SysInfo::default(), Some(map)); + } } diff --git a/components/sup/src/manager/service/config.rs b/components/sup/src/manager/service/config.rs index 80de4558dc..05258cc9ad 100644 --- a/components/sup/src/manager/service/config.rs +++ b/components/sup/src/manager/service/config.rs @@ -15,7 +15,6 @@ /// Collect all the configuration data that is exposed to users, and render it. use std; use std::borrow::Cow; -use std::collections::HashMap; use std::env; use std::fs::File; use std::io::prelude::*; @@ -25,12 +24,10 @@ use std::result; use crate::fs; use crate::hcore::fs::USER_CONFIG_FILE; use crate::hcore::{self, crypto}; -use serde::ser::SerializeMap; use serde::{Serialize, Serializer}; use serde_json; use serde_transcode; use toml; -use toml::ser; use super::Pkg; use crate::census::CensusGroup; @@ -229,11 +226,10 @@ impl Cfg { } /// Returns a subset of the overall configuration which intersects with the given package exports. - pub fn to_exported(&self, exports: &HashMap) -> Result> { + pub fn to_exported(&self, pkg: &Pkg) -> Result { let mut map = toml::value::Table::default(); - let cfg = toml::Value::try_from(&self).unwrap(); - for (key, path) in exports.iter() { - let fields: Vec<&str> = path.split('.').collect(); + let cfg = toml::Value::try_from(&self).expect("Cfg -> TOML conversion");; + for (key, path) in pkg.exports.iter() { let mut curr = &cfg; let mut found = false; @@ -241,7 +237,7 @@ impl Cfg { // function to retrieve a value with a path which returns a // reference. We actually want the value for ourselves. // Let's improve this later to avoid allocating twice. - for field in fields { + for field in path.split('.') { match curr.get(field) { Some(val) => { curr = val; @@ -255,12 +251,7 @@ impl Cfg { map.insert(key.clone(), curr.clone()); } } - - let mut dst = String::with_capacity(128); - match serialize_table(&map, &mut ser::Serializer::new(&mut dst)) { - Ok(_) => Ok(dst.into_bytes()), - Err(e) => Err(sup_error!(Error::TomlEncode(e))), - } + Ok(map) } fn load_toml_file(dir: T1, file: T2) -> Result> @@ -442,36 +433,8 @@ impl Serialize for Cfg { } } - serialize_table(&table, serializer) - } -} - -fn serialize_table(table: &toml::value::Table, serializer: S) -> result::Result -where - S: Serializer, -{ - // Be sure to visit non-tables first (and also non - // array-of-tables) as all keys must be emitted first. - let mut map = serializer.serialize_map(Some(table.len()))?; - for (k, v) in table { - if !v.is_array() && !v.is_table() { - map.serialize_key(&k)?; - map.serialize_value(&v)?; - } - } - for (k, v) in table { - if v.is_array() { - map.serialize_key(&k)?; - map.serialize_value(&v)?; - } - } - for (k, v) in table { - if v.is_table() { - map.serialize_key(&k)?; - map.serialize_value(&v)?; - } + toml::ser::tables_last(&table, serializer) } - map.end() } #[derive(Debug)] @@ -656,14 +619,10 @@ mod test { use std::env; use std::fs; use std::fs::OpenOptions; - use std::str; use tempfile::TempDir; use toml; - use hcore::package::{PackageIdent, PackageInstall}; - use hcore::service::ServiceGroup; - use super::*; use crate::error::Error; @@ -1093,73 +1052,4 @@ mod test { } } } - - fn pkg_with_exports(exports: HashMap) -> Pkg { - let service_group = ServiceGroup::new(None, "dummy", "service", None) - .expect("couldn't create ServiceGroup"); - let pg_id = PackageIdent::new( - "testing", - &service_group.service(), - Some("1.0.0"), - Some("20170712000000"), - ); - - let pkg_install = PackageInstall::new_from_parts( - pg_id.clone(), - PathBuf::from("/tmp"), - PathBuf::from("/tmp"), - PathBuf::from("/tmp"), - ); - - let mut pkg = Pkg::from_install(pkg_install).expect("Could not create package!"); - pkg.exports = exports; - pkg - } - - #[test] - fn to_exported_test() { - let export_map = [("a".to_string(), "foo".to_string())] - .iter() - .cloned() - .collect(); - let pkg = pkg_with_exports(export_map); - - let cfg_data = CfgTestData::new(); - let toml = "foo = 13"; - write_toml(&cfg_data.ducp, toml); - let cfg = Cfg::new(&cfg_data.pkg, None).expect("create config"); - - let result = cfg.to_exported(&pkg.exports).expect("export"); - assert_eq!("a = 13\n", str::from_utf8(&result).unwrap()); - } - - /// TOML encoding by default doesn't handle tables being in the middle of the output. - /// Let's check if we correctly put tables at the end - #[test] - fn nested_table_to_exported_test() { - let export_map = [ - ("a".to_string(), "server".to_string()), - ("b".to_string(), "frubnub".to_string()), - ] - .iter() - .cloned() - .collect(); - let pkg = pkg_with_exports(export_map); - - let cfg_data = CfgTestData::new(); - let toml = r#" - frubnub = "foobar" - - [server] - port = 5000 - "#; - write_toml(&cfg_data.ducp, toml); - let cfg = Cfg::new(&cfg_data.pkg, None).expect("create config"); - - let result = cfg.to_exported(&pkg.exports).expect("export"); - assert_eq!( - "b = \"foobar\"\n\n[a]\nport = 5000\n", - str::from_utf8(&result).unwrap() - ); - } } diff --git a/components/sup/src/manager/service/mod.rs b/components/sup/src/manager/service/mod.rs index 67ae7ef490..26a0e94415 100644 --- a/components/sup/src/manager/service/mod.rs +++ b/components/sup/src/manager/service/mod.rs @@ -667,7 +667,7 @@ impl Service { } pub fn to_rumor(&self, incarnation: u64) -> ServiceRumor { - let exported = match self.cfg.to_exported(&self.pkg.exports) { + let exported = match self.cfg.to_exported(&self.pkg) { Ok(exported) => Some(exported), Err(err) => { outputln!(preamble self.service_group,