-
Notifications
You must be signed in to change notification settings - Fork 316
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Correctly serialize exports in service rumors #5987
Conversation
Thanks for the pull request! Here is what will happen next:
Thank you for contributing! |
8368821
to
6b7b07c
Compare
This is code in an area I'm not at all familiar with, so it's going to take me some time to give you good feedback. What's the urgency level of this change? |
The bug has been there for a while already and there are workarounds. Doesn't need to be squeezed into this release. |
1a77748
to
4308a20
Compare
7ce497d
to
373d118
Compare
Apologies for the delay; I'll start digging into this tomorrow |
I've mostly got my head wrapped around this code now (it's kind of a doozy). Hopefully I'll be able to finish this review tomorrow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, I think we can make this change smaller (see inline comments) and reduce the amount of code we have for serialization by relying more on the external crates.
Additionally, I think we should add a test to components/butterfly/src/rumor/service.rs
which explicitly tests the regression. I've confirmed this panics the existing code (with the minor change that Some(map)
becomes Some(&map)
if we change the cfg
parameter to Service::new
.
#[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));
}
The root of the panic is due to us trying to serialize a `Toml::Value::Table` to a Vec<u8> 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 <james@chef.io>
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 <james@chef.io>
Signed-off-by: James Casey <james@chef.io>
373d118
to
26457fa
Compare
Obvious fix; these changes are the result of automation not creative thinking.
Solves #5854 by making sure that we always serialize the export configuration as valid TOML.
This PR moves the logic into
Cfg::to_exported
from theService::new
constructor so we can handle the error more sanely and reuse some of the serialization logic that is already there.It feels like there might be a more natural way to do this via
Serde
by constructing some more objects to represent this service config but not sure if it's worth the extra code and abstraction. Would love some input on this.