Update datastore serializer to expect JSON and correctly handle null values #80

sam-berning · 2024-08-12T23:40:18Z

Issue number:

This is a more robust/comprehensive fix to the first issue seen in bottlerocket-os/bottlerocket#4135

Description of changes:

Previously, the datastore serializer handled basically any serializable data type, which allowed us to use a range of input types, but also required us to be very generic in the implementation, making it difficult to special-case particular types.

As we move towards Out-of-Tree Builds and Settings Extensions, allowing this broad range of types as an input to the datastore serializer is actually becoming less helpful, since settings will be passed around as a more generic type that will be validated against a more concrete type on set.

Because of this, it makes sense to limit the input space of the Serializer to only allow this generic type, and to ensure that all edge cases for this generic type are handled correctly. Since Settings Plugins do all of their serialization and deserialization across the FFI border using JSON, it makes sense for JSON to be this generic type.

After we've limited the datastore serializer to only accept JSON, implementors of Settings Plugins and Settings Extensions can just focus on ensuring that their types serialize correctly to JSON, and they can be sure that it will serialize correctly to the datastore format for free.

A note for discussion: now that the datastore serializer has been limited to only accept JSON, there is serialization logic for a broader range of types than necessary. Does make sense to clean up these unused types to help hint that this serializer should only be used with serde_json::Value?

Testing done:

I added a bunch of unit tests to the datastore serializer to ensure that all of the types in serde_json::Value serialize correctly.

Then, I built the core-kit, and built an AMI using my core-kit to ensure that there was no strange behavior. The instance I launched booted fine, and was able to handle settings correctly.

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

sam-berning · 2024-08-12T23:43:36Z

sources/api/sundog/src/main.rs

-    // Build a Settings struct from the response.
-    let settings = serde_json::from_value::<model::Model>(response)
-        .context(error::InterpretModelSnafu)?
-        .settings;


I am assuming here that because the settings JSON comes from apiclient that it should contain only valid settings, so there's no need to convert to and from Settings just to verify this.

sam-berning · 2024-08-12T23:57:49Z

^ fix fmt

bcressey

Nice.

bcressey · 2024-08-13T17:01:01Z

sources/api/sundog/src/main.rs

@@ -218,15 +218,10 @@ where
        .context(error::GetPrefixSnafu { prefixes })?;
    debug!("API response model: {}", response.to_string());

-    // Build a Settings struct from the response.
-    let settings = serde_json::from_value::<model::Model>(response)
-        .context(error::InterpretModelSnafu)?


We can also get rid of the InterpretModelSnafu error.

bcressey · 2024-08-13T17:05:33Z

sources/api/apiserver/src/server/controller.rs

@@ -366,7 +366,8 @@ pub(crate) fn set_settings<D: DataStore>(
    transaction: &str,
 ) -> Result<()> {
    trace!("Serializing Settings to write to data store");
-    let pairs = to_pairs_with_prefix("settings", settings)
+    let settings_json = serde_json::to_value(settings).expect("struct to value can't fail");


Why is this infallible?

my rationale on this was that because Settings Plugins are serialized and deserialized through JSON, in order for anything to be assigned to a Settings type it must be serializable to JSON. I guess it's probably safer to handle the error gracefully if it does occur though, so I'll add a snafu error here.

cbgbt · 2024-08-13T17:29:36Z

sources/api/datastore/src/serialization/pairs.rs

-                key!("B.list") => "[3,4,5]".to_string(),
-                key!("B.boolean") => "true".to_string(),
+                key!("list") => "[3,4,5]".to_string(),
+                key!("boolean") => "true".to_string(),


The prefix dropping here in the test felt like an alarm bell for backwards incompatibility, so I dug into it a bit and think that it's safe, though I think it does indicate a change to make.

Basically, the serializer keeps track of a "key prefix" for all elements when it is converting them to pairs. In the case where no prefix is present and the element is a struct, it adds the struct's name as a prefix. In the past, this should only be triggered when dealing with a struct that represents the top-level of some stored object.

I believe this prefix hoisting will never be triggered if the input domain is serde_json::Value, since that type is an enum. We could potentially remove it -- what's important is to check that configuration-files, metadata, and services are all serialized to the appropriate pair names.

oh yeah, meant to leave a note on this in the PR. we had removed the cases where the serializer depended on the struct name as part of the settings plugins work because I couldn't find a way to rename serde_json::Value, so this shouldn't actually cause a change in behavior in practice

cbgbt · 2024-08-13T18:06:37Z

sources/api/storewolf/src/main.rs

    debug!("Serializing other defaults and writing new ones to datastore");
-    let defaults = to_pairs(&defaults_val).context(error::SerializationSnafu {
+    let defaults_json = serde_json::to_value(defaults_val).context(error::JsonConversionSnafu)?;
+    let defaults = to_pairs(&defaults_json).context(error::SerializationSnafu {


The configuration-files and services I mentioned should be handled here.

I think something that would be extremely helpful would be some integration-level checks of storewolf that the resulting datastore from a given set of defaults is as-expected. This would provide a ton of validation to the datastore serializer.

We could work on that in a followup though.

cbgbt · 2024-08-13T18:08:18Z

sources/api/datastore/src/serialization/pairs.rs

@@ -540,13 +541,14 @@ mod test {
            boolean: true,
        };
        let a = A { id: 42, b: Some(b) };
-        let keys = to_pairs(&a).unwrap();
+        let j = serde_json::to_value(a).unwrap();


nit: could be a good idea to define these test structures using the json! macro, but it isn't necessary.

I intentionally left the existing tests as-is with just a conversion to serde_json::Value to confirm that the way we handled non-json inputs was backwards compatible (apart from the prefix behavior mentioned above)

cbgbt · 2024-08-13T18:52:49Z

Nice work! None of my feedback is blocking.

This will simplify the input space for the serializer, allowing us to be confident that we can make changes to the serialize_{type} methods knowing that the space is testable. This also sets us up well for settings extensions, as in that world we won't have access to a Settings struct, so we will use serde_json::Value as the de facto type for settings in Bottlerocket. Signed-off-by: Sam Berning <[email protected]>

This set of tests should cover most edge cases for converting from a serde_json::Value to a datastore key/pair Signed-off-by: Sam Berning <[email protected]>

Now that we've limited the scope of the Serializer input to just serde_json::Value, we can be confident that serialize_unit() would only be called on a null value. We can now define the behavior for that, instead of throwing a "bad type" error. In this case, we prefer to omit the key/value pair from the serialization output because we'd rather not persist null values in our datastore, and the behavior should be the same on deserialization. This should make creating settings models for Bottlerocket less error-prone, as it's no longer necessary to add serde's `skip_serializing_if` macro to all of the Option<T>s in the model. This makes that behavior default for Option<T>. Signed-off-by: Sam Berning <[email protected]>

sam-berning · 2024-08-13T20:13:32Z

^ improved serialization error handling in apiserver, removed unused errors in sundog

bcressey · 2024-08-13T21:28:54Z

sources/api/apiserver/src/server/controller.rs

@@ -366,7 +366,8 @@ pub(crate) fn set_settings<D: DataStore>(
    transaction: &str,
 ) -> Result<()> {
    trace!("Serializing Settings to write to data store");
-    let pairs = to_pairs_with_prefix("settings", settings)
+    let settings_json = serde_json::to_value(settings).expect("struct to value can't fail");


Suggested change

let settings_json = serde_json::to_value(settings).expect("struct to value can't fail");

let settings_json = serde_json::to_value(settings).context(error::SettingsToJsonSnafu)?;

sam-berning requested review from bcressey and cbgbt August 12, 2024 23:40

sam-berning commented Aug 12, 2024

View reviewed changes

sam-berning force-pushed the datastore_expect_json branch from 3289ce5 to d2a02f6 Compare August 12, 2024 23:57

bcressey approved these changes Aug 13, 2024

View reviewed changes

cbgbt reviewed Aug 13, 2024

View reviewed changes

cbgbt approved these changes Aug 13, 2024

View reviewed changes

sam-berning force-pushed the datastore_expect_json branch from d2a02f6 to 3cba251 Compare August 13, 2024 20:07

sam-berning added 3 commits August 13, 2024 20:13

datastore: add unit tests for to_pairs on json Value

e048c08

This set of tests should cover most edge cases for converting from a serde_json::Value to a datastore key/pair Signed-off-by: Sam Berning <[email protected]>

sam-berning force-pushed the datastore_expect_json branch from 3cba251 to 8f882a4 Compare August 13, 2024 20:13

cbgbt approved these changes Aug 13, 2024

View reviewed changes

sam-berning merged commit 1e14cec into bottlerocket-os:develop Aug 13, 2024
2 checks passed

bcressey reviewed Aug 13, 2024

View reviewed changes

This was referenced Aug 13, 2024

apiserver: fix incorrect error handling #87

Merged

1.21.0 Status 500 when PATCHing /settings?tx=bottlerocket-launch bottlerocket-os/bottlerocket#4135

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update datastore serializer to expect JSON and correctly handle null values #80

Update datastore serializer to expect JSON and correctly handle null values #80

sam-berning commented Aug 12, 2024

sam-berning Aug 12, 2024

sam-berning commented Aug 12, 2024

bcressey left a comment

bcressey Aug 13, 2024

bcressey Aug 13, 2024

sam-berning Aug 13, 2024 •

edited

Loading

cbgbt Aug 13, 2024

sam-berning Aug 13, 2024

cbgbt Aug 13, 2024

cbgbt Aug 13, 2024

sam-berning Aug 13, 2024

cbgbt commented Aug 13, 2024

sam-berning commented Aug 13, 2024

bcressey Aug 13, 2024

	let settings_json = serde_json::to_value(settings).expect("struct to value can't fail");
	let settings_json = serde_json::to_value(settings).context(error::SettingsToJsonSnafu)?;

Update datastore serializer to expect JSON and correctly handle null values #80

Update datastore serializer to expect JSON and correctly handle null values #80

Conversation

sam-berning commented Aug 12, 2024

Choose a reason for hiding this comment

sam-berning commented Aug 12, 2024

bcressey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sam-berning Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbgbt commented Aug 13, 2024

sam-berning commented Aug 13, 2024

Choose a reason for hiding this comment

sam-berning Aug 13, 2024 •

edited

Loading