From fd273a85aa0103fc1d05cc2b02084b4452de2f78 Mon Sep 17 00:00:00 2001 From: "Sean P. Kelly" Date: Thu, 19 Oct 2023 07:58:12 +0000 Subject: [PATCH] feat: remove extension ability to modify set values This change also puts all protocol output to stdout, reserving stderr for logging. --- src/example/empty.rs | 8 +++---- src/extension/proto1.rs | 38 ++++++++++++------------------- src/migrate/linear/interface.rs | 16 ++++++------- src/migrate/linear/mod.rs | 8 +++---- src/migrate/mod.rs | 4 ++-- src/model/erased.rs | 14 ++++-------- src/model/mod.rs | 16 ++++++------- tests/colliding_versions/mod.rs | 8 +++---- tests/migration_validation/mod.rs | 4 ++-- tests/motd/v1.rs | 24 ++++++++----------- tests/motd/v2.rs | 24 ++++++++----------- tests/sample_extensions.rs | 14 +++++++----- 12 files changed, 76 insertions(+), 102 deletions(-) diff --git a/src/example/empty.rs b/src/example/empty.rs index 16120404..e41e46e5 100644 --- a/src/example/empty.rs +++ b/src/example/empty.rs @@ -15,8 +15,8 @@ impl SettingsModel for EmptySetting { "v1" } - fn set(_current_value: Option, target: Self) -> Result { - Ok(target) + fn set(_current_value: Option, _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) -> Result { - Ok(true) + fn validate(_value: Self, _validated_settings: Option) -> Result<()> { + Ok(()) } } diff --git a/src/extension/proto1.rs b/src/extension/proto1.rs index 797e382f..07314dae 100644 --- 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(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, 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>; + fn set(&self, args: SetCommand) -> Result<(), SettingsExtensionError>; fn generate( &self, args: GenerateCommand, @@ -77,7 +75,7 @@ pub trait Proto1: Debug { fn validate( &self, args: ValidateCommand, - ) -> Result>; + ) -> Result<(), SettingsExtensionError>; } impl Proto1 for SettingsExtension @@ -88,10 +86,7 @@ where type MigratorErrorKind = Mi::ErrorKind; #[instrument(err)] - fn set( - &self, - args: SetCommand, - ) -> Result> { + fn set(&self, args: SetCommand) -> Result<(), SettingsExtensionError> { self.model(&args.setting_version) .context(error::NoSuchModelSnafu { setting_version: args.setting_version, @@ -156,7 +151,7 @@ where fn validate( &self, args: ValidateCommand, - ) -> Result> { + ) -> Result<(), SettingsExtensionError> { self.model(&args.setting_version) .context(error::NoSuchModelSnafu { setting_version: args.setting_version, @@ -164,8 +159,5 @@ where .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 index 314656fe..56ef4513 100644 --- 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, target: Self) -> Result { -/// # Ok(target) +/// # fn set(_current_value: Option, 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) -> Result { -/// # Ok(true) +/// # fn validate(_value: Self, _validated_settings: Option) -> Result<()> { +/// # Ok(()) /// # } /// # } /// # @@ -71,8 +71,8 @@ pub struct LinearMigrator; /// # "v2" /// # } /// # -/// # fn set(_current_value: Option, target: Self) -> Result { -/// # Ok(target) +/// # fn set(_current_value: Option, 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) -> Result { -/// # Ok(true) +/// # fn validate(_value: Self, _validated_settings: Option) -> Result<()> { +/// # Ok(()) /// # } /// # } /// impl LinearlyMigrateable for ScoreV1 { diff --git a/src/migrate/linear/mod.rs b/src/migrate/linear/mod.rs index b12c23ac..061a4282 100644 --- 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, _target: Self, - ) -> Result { - Ok(Self::new()) + ) -> Result<(), Infallible> { + Ok(()) } fn generate( @@ -415,8 +415,8 @@ mod test { fn validate( _value: Self, _validated_settings: Option, - ) -> Result { - Ok(true) + ) -> Result<(), Infallible> { + Ok(()) } } diff --git a/src/migrate/mod.rs b/src/migrate/mod.rs index 35b3eb27..2f5760eb 100644 --- a/src/migrate/mod.rs +++ b/src/migrate/mod.rs @@ -125,7 +125,7 @@ impl SettingsModel for NoMigration { ) } - fn set(_current_value: Option, _target: Self) -> Result { + fn set(_current_value: Option, _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, - ) -> Result { + ) -> 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 index df50fe2e..0c8c60b3 100644 --- a/src/model/erased.rs +++ b/src/model/erased.rs @@ -42,7 +42,7 @@ pub trait TypeErasedModel: Debug { &self, current: Option, target: serde_json::Value, - ) -> Result; + ) -> Result<(), BottlerocketSettingError>; /// Generates default values at system start. /// @@ -64,7 +64,7 @@ pub trait TypeErasedModel: Debug { &self, value: serde_json::Value, validated_settings: Option, - ) -> Result; + ) -> Result<(), BottlerocketSettingError>; /// Parses a JSON value into the underlying model type, then erases its type via the `Any` trait. /// @@ -99,7 +99,7 @@ impl TypeErasedModel for BottlerocketSetting { &self, current: Option, target: serde_json::Value, - ) -> Result { + ) -> Result<(), BottlerocketSettingError> { debug!( current_value = current.as_ref().map(|v| v.to_string()), target_value = target.to_string(), @@ -127,12 +127,6 @@ impl TypeErasedModel for BottlerocketSetting { .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 TypeErasedModel for BottlerocketSetting { &self, value: serde_json::Value, validated_settings: Option, - ) -> Result { + ) -> 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 index 9a8676c6..a8008c80 100644 --- a/src/model/mod.rs +++ b/src/model/mod.rs @@ -38,9 +38,9 @@ pub use error::BottlerocketSettingError; /// "v1" /// } /// -/// fn set(current_value: Option, target: Self) -> Result { +/// fn set(current_value: Option, 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) -> Result { +/// fn validate(_value: Self, _validated_settings: Option) -> 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, target: Self) -> Result; + /// Returns an error if the value is rejected. + fn set(current_value: Option, 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, - ) -> Result; + ) -> 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 index 63437761..4bb294f5 100644 --- 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) -> Result { + fn set(_: Option, _: Self) -> Result<()> { unimplemented!() } @@ -52,7 +52,7 @@ impl SettingsModel for ModelA { unimplemented!() } - fn validate(_: Self, _: Option) -> Result { + fn validate(_: Self, _: Option) -> Result<()> { unimplemented!() } } @@ -79,7 +79,7 @@ impl SettingsModel for ModelB { "v1" } - fn set(_: Option, _: Self) -> Result { + fn set(_: Option, _: Self) -> Result<()> { unimplemented!() } @@ -90,7 +90,7 @@ impl SettingsModel for ModelB { unimplemented!() } - fn validate(_: Self, _: Option) -> Result { + fn validate(_: Self, _: Option) -> Result<()> { unimplemented!() } } diff --git a/tests/migration_validation/mod.rs b/tests/migration_validation/mod.rs index b3fc1fe6..b974c5d7 100644 --- 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) -> Result { + fn set(_: Option, _: Self) -> Result<()> { unimplemented!() } @@ -29,7 +29,7 @@ macro_rules! define_model { unimplemented!() } - fn validate(_: Self, _: Option) -> Result { + fn validate(_: Self, _: Option) -> Result<()> { unimplemented!() } } diff --git a/tests/motd/v1.rs b/tests/motd/v1.rs index bc5e6e2e..53a0683e 100644 --- a/tests/motd/v1.rs +++ b/tests/motd/v1.rs @@ -22,10 +22,10 @@ impl SettingsModel for MotdV1 { fn set( // We allow any transition from current value to target, so we don't need the current value _current_value: Option, - target: Self, - ) -> Result { + _target: Self, + ) -> Result<()> { // Allow anything that parses as MotdV1 - Ok(target) + Ok(()) } fn generate( @@ -39,9 +39,9 @@ impl SettingsModel for MotdV1 { )) } - fn validate(_value: Self, _validated_settings: Option) -> Result { + fn validate(_value: Self, _validated_settings: Option) -> Result<()> { // No need to do any additional validation, any MotdV1 is acceptable - Ok(true) + Ok(()) } } @@ -72,12 +72,9 @@ fn test_motdv1_set_success() { // Then that input is successfully set. vec![json!("Hello!"), json!("")] .into_iter() - .for_each(|value| { - assert_eq!( - set_cli(motd_settings_extension(), "v1", value.clone()).unwrap(), - value - ) - }); + .for_each( + |value| assert!(set_cli(motd_settings_extension(), "v1", value.clone()).is_ok(),), + ); } #[test] @@ -105,10 +102,7 @@ fn test_motdv1_generate() { fn test_motdv1_validate() { // When validate is called on motdv1, // it is successful for any value that parses - assert_eq!( - validate_cli(motd_settings_extension(), "v1", json!("test"), None).unwrap(), - json!(true), - ); + assert!(validate_cli(motd_settings_extension(), "v1", json!("test"), None).is_ok(),); } #[test] diff --git a/tests/motd/v2.rs b/tests/motd/v2.rs index 0b3b1a8f..89def75e 100644 --- a/tests/motd/v2.rs +++ b/tests/motd/v2.rs @@ -19,10 +19,10 @@ impl SettingsModel for MotdV2 { fn set( // We allow any transition from current value to target, so we don't need the current value _current_value: Option, - target: Self, - ) -> anyhow::Result { + _target: Self, + ) -> anyhow::Result<()> { // Allow anything that parses as MotdV2 - Ok(target) + Ok(()) } fn generate( @@ -36,16 +36,15 @@ impl SettingsModel for MotdV2 { )) } - fn validate( - value: Self, - _validated_settings: Option, - ) -> anyhow::Result { + fn validate(value: Self, _validated_settings: Option) -> anyhow::Result<()> { let Self(inner_strings) = value; // No whitespace allowed in any of the substrings - Ok(!inner_strings + anyhow::ensure!(!inner_strings .iter() - .any(|i| i.contains(char::is_whitespace))) + .any(|i| i.contains(char::is_whitespace)),); + + Ok(()) } } @@ -80,12 +79,7 @@ fn test_motdv2_set_success() { json!([]), ] .into_iter() - .for_each(|value| { - assert_eq!( - set_cli(motd_settings_extension(), "v2", value.clone()).unwrap(), - value - ) - }); + .for_each(|value| assert!(set_cli(motd_settings_extension(), "v2", value.clone()).is_ok())); } #[test] diff --git a/tests/sample_extensions.rs b/tests/sample_extensions.rs index c3666045..481c9c1b 100644 --- a/tests/sample_extensions.rs +++ b/tests/sample_extensions.rs @@ -28,7 +28,7 @@ mod helpers { extension: SettingsExtension, version: &str, value: serde_json::Value, - ) -> Result + ) -> Result<()> where Mi: Migrator, Mo: AsTypeErasedModel, @@ -44,8 +44,9 @@ mod helpers { value.to_string().as_str(), ]) .context("Failed to run settings extension CLI") - .and_then(|s| { - serde_json::from_str(s.as_str()).context("Failed to parse CLI result as JSON") + .map(|s| { + assert!(s.is_empty()); + () }) } @@ -98,7 +99,7 @@ mod helpers { version: &str, value: serde_json::Value, required_settings: Option, - ) -> Result + ) -> Result<()> where Mi: Migrator, Mo: AsTypeErasedModel, @@ -126,8 +127,9 @@ mod helpers { extension .try_run_with_args(args) .context("Failed to run settings extension CLI") - .and_then(|s| { - serde_json::from_str(s.as_str()).context("Failed to parse CLI result as JSON") + .map(|s| { + assert!(s.is_empty()); + () }) }