Merge pull request #15 from bottlerocket-os/set-no-edit

feat: remove extension ability to modify set values
bottlerocket-os · Oct 24, 2023 · 66e3b7b · 66e3b7b
2 parents c250540 + fd273a8
commit 66e3b7b
Show file tree

Hide file tree

Showing 12 changed files with 76 additions and 102 deletions.
diff --git a/src/example/empty.rs b/src/example/empty.rs
@@ -15,8 +15,8 @@ impl SettingsModel for EmptySetting {
         "v1"
     }
 
-    fn set(_current_value: Option<Self>, target: Self) -> Result<Self> {
-        Ok(target)
+    fn set(_current_value: Option<Self>, _target: Self) -> Result<()> {
+        Ok(())
     }
 
     fn generate(
@@ -26,8 +26,8 @@ impl SettingsModel for EmptySetting {
         Ok(GenerateResult::Complete(Self))
     }
 
-    fn validate(_value: Self, _validated_settings: Option<serde_json::Value>) -> Result<bool> {
-        Ok(true)
+    fn validate(_value: Self, _validated_settings: Option<serde_json::Value>) -> Result<()> {
+        Ok(())
     }
 }
 

diff --git a/src/extension/proto1.rs b/src/extension/proto1.rs
@@ -20,15 +20,14 @@ use tracing::instrument;
 /// Once the extension has run, the program terminates.
 pub fn run_extension<P: Proto1>(extension: P, cmd: Proto1Command) -> ExitCode {
     match try_run_extension(extension, cmd) {
-        Err(e) => {
-            // TODO use machine-readable output on error.
-            eprintln!("{}", e);
-            ExitCode::FAILURE
-        }
         Ok(output) => {
             println!("{}", &output);
             ExitCode::SUCCESS
         }
+        Err(e) => {
+            println!("{}", e);
+            ExitCode::FAILURE
+        }
     }
 }
 
@@ -44,16 +43,18 @@ where
     P: Proto1<MigratorErrorKind = ME>,
     ME: std::error::Error + Send + Sync + 'static,
 {
+    let json_stringify =
+        |value| serde_json::to_string_pretty(&value).context(error::SerializeResultSnafu);
+
     match cmd {
-        Proto1Command::Set(s) => extension.set(s),
-        Proto1Command::Generate(g) => extension.generate(g),
-        Proto1Command::Migrate(m) => extension.migrate(m),
-        Proto1Command::Validate(v) => extension.validate(v),
+        Proto1Command::Set(s) => extension.set(s).map(|_| String::new()),
+        Proto1Command::Generate(g) => extension.generate(g).and_then(json_stringify),
+        Proto1Command::Migrate(m) => extension.migrate(m).and_then(json_stringify),
+        Proto1Command::Validate(v) => extension.validate(v).map(|_| String::new()),
         Proto1Command::Helper(_h) => {
             todo!("https://github.com/bottlerocket-os/bottlerocket-settings-sdk/issues/3")
         }
     }
-    .and_then(|value| serde_json::to_string_pretty(&value).context(error::SerializeResultSnafu))
 }
 
 /// A trait representing adherence to Bottlerocket settings extension CLI proto1.
@@ -62,10 +63,7 @@ where
 pub trait Proto1: Debug {
     type MigratorErrorKind: std::error::Error + Send + Sync + 'static;
 
-    fn set(
-        &self,
-        args: SetCommand,
-    ) -> Result<serde_json::Value, SettingsExtensionError<Self::MigratorErrorKind>>;
+    fn set(&self, args: SetCommand) -> Result<(), SettingsExtensionError<Self::MigratorErrorKind>>;
     fn generate(
         &self,
         args: GenerateCommand,
@@ -77,7 +75,7 @@ pub trait Proto1: Debug {
     fn validate(
         &self,
         args: ValidateCommand,
-    ) -> Result<serde_json::Value, SettingsExtensionError<Self::MigratorErrorKind>>;
+    ) -> Result<(), SettingsExtensionError<Self::MigratorErrorKind>>;
 }
 
 impl<Mi, Mo> Proto1 for SettingsExtension<Mi, Mo>
@@ -88,10 +86,7 @@ where
     type MigratorErrorKind = Mi::ErrorKind;
 
     #[instrument(err)]
-    fn set(
-        &self,
-        args: SetCommand,
-    ) -> Result<serde_json::Value, SettingsExtensionError<Self::MigratorErrorKind>> {
+    fn set(&self, args: SetCommand) -> Result<(), SettingsExtensionError<Self::MigratorErrorKind>> {
         self.model(&args.setting_version)
             .context(error::NoSuchModelSnafu {
                 setting_version: args.setting_version,
@@ -156,16 +151,13 @@ where
     fn validate(
         &self,
         args: ValidateCommand,
-    ) -> Result<serde_json::Value, SettingsExtensionError<Self::MigratorErrorKind>> {
+    ) -> Result<(), SettingsExtensionError<Self::MigratorErrorKind>> {
         self.model(&args.setting_version)
             .context(error::NoSuchModelSnafu {
                 setting_version: args.setting_version,
             })?
             .as_model()
             .validate(args.value, args.required_settings)
             .context(error::ValidateSnafu)
-            .and_then(|validation| {
-                serde_json::to_value(validation).context(error::SerializeResultSnafu)
-            })
     }
 }
diff --git a/src/migrate/linear/interface.rs b/src/migrate/linear/interface.rs
@@ -47,8 +47,8 @@ pub struct LinearMigrator;
 /// #         "v1"
 /// #     }
 /// #
-/// #     fn set(_current_value: Option<Self>, target: Self) -> Result<Self> {
-/// #         Ok(target)
+/// #     fn set(_current_value: Option<Self>, target: Self) -> Result<()> {
+/// #         Ok(())
 /// #     }
 /// #
 /// #     fn generate(
@@ -58,8 +58,8 @@ pub struct LinearMigrator;
 /// #         Ok(GenerateResult::Complete(Self::default()))
 /// #     }
 /// #
-/// #     fn validate(_value: Self, _validated_settings: Option<serde_json::Value>) -> Result<bool> {
-/// #         Ok(true)
+/// #     fn validate(_value: Self, _validated_settings: Option<serde_json::Value>) -> Result<()> {
+/// #         Ok(())
 /// #     }
 /// # }
 /// #
@@ -71,8 +71,8 @@ pub struct LinearMigrator;
 /// #         "v2"
 /// #     }
 /// #
-/// #     fn set(_current_value: Option<Self>, target: Self) -> Result<Self> {
-/// #         Ok(target)
+/// #     fn set(_current_value: Option<Self>, target: Self) -> Result<()> {
+/// #         Ok(())
 /// #     }
 /// #
 /// #     fn generate(
@@ -82,8 +82,8 @@ pub struct LinearMigrator;
 /// #         Ok(GenerateResult::Complete(Self::default()))
 /// #     }
 /// #
-/// #     fn validate(_value: Self, _validated_settings: Option<serde_json::Value>) -> Result<bool> {
-/// #         Ok(true)
+/// #     fn validate(_value: Self, _validated_settings: Option<serde_json::Value>) -> Result<()> {
+/// #         Ok(())
 /// #     }
 /// # }
 /// impl LinearlyMigrateable for ScoreV1 {

diff --git a/src/migrate/linear/mod.rs b/src/migrate/linear/mod.rs
@@ -400,8 +400,8 @@ mod test {
                     // We allow any transition from current value to target, so we don't need the current value
                     _current_value: Option<Self>,
                     _target: Self,
-                ) -> Result<Self, Infallible> {
-                    Ok(Self::new())
+                ) -> Result<(), Infallible> {
+                    Ok(())
                 }
 
                 fn generate(
@@ -415,8 +415,8 @@ mod test {
                 fn validate(
                     _value: Self,
                     _validated_settings: Option<serde_json::Value>,
-                ) -> Result<bool, Infallible> {
-                    Ok(true)
+                ) -> Result<(), Infallible> {
+                    Ok(())
                 }
             }
 

diff --git a/src/migrate/mod.rs b/src/migrate/mod.rs
@@ -125,7 +125,7 @@ impl SettingsModel for NoMigration {
         )
     }
 
-    fn set(_current_value: Option<Self>, _target: Self) -> Result<Self, Self::ErrorKind> {
+    fn set(_current_value: Option<Self>, _target: Self) -> Result<(), Self::ErrorKind> {
         unimplemented!(
             "`NoMigration` used as a marker type. Its settings model should never be used."
         )
@@ -143,7 +143,7 @@ impl SettingsModel for NoMigration {
     fn validate(
         _value: Self,
         _validated_settings: Option<serde_json::Value>,
-    ) -> Result<bool, Self::ErrorKind> {
+    ) -> Result<(), Self::ErrorKind> {
         unimplemented!(
             "`NoMigration` used as a marker type. Its settings model should never be used."
         )

diff --git a/src/model/erased.rs b/src/model/erased.rs
@@ -42,7 +42,7 @@ pub trait TypeErasedModel: Debug {
         &self,
         current: Option<serde_json::Value>,
         target: serde_json::Value,
-    ) -> Result<serde_json::Value, BottlerocketSettingError>;
+    ) -> Result<(), BottlerocketSettingError>;
 
     /// Generates default values at system start.
     ///
@@ -64,7 +64,7 @@ pub trait TypeErasedModel: Debug {
         &self,
         value: serde_json::Value,
         validated_settings: Option<serde_json::Value>,
-    ) -> Result<bool, BottlerocketSettingError>;
+    ) -> Result<(), BottlerocketSettingError>;
 
     /// Parses a JSON value into the underlying model type, then erases its type via the `Any` trait.
     ///
@@ -99,7 +99,7 @@ impl<T: SettingsModel + 'static> TypeErasedModel for BottlerocketSetting<T> {
         &self,
         current: Option<serde_json::Value>,
         target: serde_json::Value,
-    ) -> Result<serde_json::Value, BottlerocketSettingError> {
+    ) -> Result<(), BottlerocketSettingError> {
         debug!(
             current_value = current.as_ref().map(|v| v.to_string()),
             target_value = target.to_string(),
@@ -127,12 +127,6 @@ impl<T: SettingsModel + 'static> TypeErasedModel for BottlerocketSetting<T> {
             .context(error::SetSettingSnafu {
                 version: T::get_version(),
             })
-            .and_then(|retval| {
-                serde_json::to_value(retval).context(error::SerializeResultSnafu {
-                    version: T::get_version(),
-                    operation: "set",
-                })
-            })
     }
 
     #[instrument(skip(self), err)]
@@ -176,7 +170,7 @@ impl<T: SettingsModel + 'static> TypeErasedModel for BottlerocketSetting<T> {
         &self,
         value: serde_json::Value,
         validated_settings: Option<serde_json::Value>,
-    ) -> Result<bool, BottlerocketSettingError> {
+    ) -> Result<(), BottlerocketSettingError> {
         debug!(
             %value,
             validated_settings = validated_settings.as_ref().map(|v| v.to_string()),

diff --git a/src/model/mod.rs b/src/model/mod.rs
@@ -38,9 +38,9 @@ pub use error::BottlerocketSettingError;
 ///         "v1"
 ///     }
 ///
-///     fn set(current_value: Option<Self>, target: Self) -> Result<Self> {
+///     fn set(current_value: Option<Self>, target: Self) -> Result<()> {
 ///         // Perform any additional validations of the new value here...
-///         Ok(target)
+///         Ok(())
 ///     }
 ///
 ///     fn generate(
@@ -51,9 +51,9 @@ pub use error::BottlerocketSettingError;
 ///         Ok(GenerateResult::Complete(MySettings::default()))
 ///     }
 ///
-///     fn validate(_value: Self, _validated_settings: Option<serde_json::Value>) -> Result<bool> {
+///     fn validate(_value: Self, _validated_settings: Option<serde_json::Value>) -> Result<()> {
 ///         // Cross-validation of new values can occur against other settings here...
-///         Ok(true)
+///         Ok(())
 ///     }
 /// }
 ///
@@ -76,10 +76,8 @@ pub trait SettingsModel: Sized + Serialize + DeserializeOwned + Debug {
 
     /// Determines whether this setting can be set to the `target` value, given its current value.
     ///
-    /// The returned value is what is ultimately set in the settings datastore. While this leaves
-    /// room for the extension to modify the value that is stored, this should be done cautiously
-    /// so as not to confuse users.
-    fn set(current_value: Option<Self>, target: Self) -> Result<Self, Self::ErrorKind>;
+    /// Returns an error if the value is rejected.
+    fn set(current_value: Option<Self>, target: Self) -> Result<(), Self::ErrorKind>;
 
     /// Generates default values at system start.
     ///
@@ -99,7 +97,7 @@ pub trait SettingsModel: Sized + Serialize + DeserializeOwned + Debug {
     fn validate(
         _value: Self,
         _validated_settings: Option<serde_json::Value>,
-    ) -> Result<bool, Self::ErrorKind>;
+    ) -> Result<(), Self::ErrorKind>;
 }
 
 /// This struct wraps [`SettingsModel`]s in a referencable object which is passed to the

diff --git a/tests/colliding_versions/mod.rs b/tests/colliding_versions/mod.rs
@@ -41,7 +41,7 @@ impl SettingsModel for ModelA {
         "v1"
     }
 
-    fn set(_: Option<Self>, _: Self) -> Result<Self> {
+    fn set(_: Option<Self>, _: Self) -> Result<()> {
         unimplemented!()
     }
 
@@ -52,7 +52,7 @@ impl SettingsModel for ModelA {
         unimplemented!()
     }
 
-    fn validate(_: Self, _: Option<serde_json::Value>) -> Result<bool> {
+    fn validate(_: Self, _: Option<serde_json::Value>) -> Result<()> {
         unimplemented!()
     }
 }
@@ -79,7 +79,7 @@ impl SettingsModel for ModelB {
         "v1"
     }
 
-    fn set(_: Option<Self>, _: Self) -> Result<Self> {
+    fn set(_: Option<Self>, _: Self) -> Result<()> {
         unimplemented!()
     }
 
@@ -90,7 +90,7 @@ impl SettingsModel for ModelB {
         unimplemented!()
     }
 
-    fn validate(_: Self, _: Option<serde_json::Value>) -> Result<bool> {
+    fn validate(_: Self, _: Option<serde_json::Value>) -> Result<()> {
         unimplemented!()
     }
 }

diff --git a/tests/migration_validation/mod.rs b/tests/migration_validation/mod.rs
@@ -18,7 +18,7 @@ macro_rules! define_model {
                 $version
             }
 
-            fn set(_: Option<Self>, _: Self) -> Result<Self> {
+            fn set(_: Option<Self>, _: Self) -> Result<()> {
                 unimplemented!()
             }
 
@@ -29,7 +29,7 @@ macro_rules! define_model {
                 unimplemented!()
             }
 
-            fn validate(_: Self, _: Option<serde_json::Value>) -> Result<bool> {
+            fn validate(_: Self, _: Option<serde_json::Value>) -> Result<()> {
                 unimplemented!()
             }
         }