feat: unify yamlconfig and hash and do not store hash if failed (#1299)

* feat: unify yamlconfig and hash in a new Remote Config values and don't store a failed hash.
* feat: Modify LocalConfig enum to use custom type
* feat: Modify update_hash method to upda_hash_state because a hash should never be updated, only the state
* feat: Improve Update Hash state error message
* feat: Correct wrong comment in config_repository
* feat: Simplify loading a file if it's present for Config Repository File
* feat: Remove hash apply and fail and call update_state from remote_config instead
* feat: Rename yaml_config_repository tests file to match the code
* feat: Rename opamp->RemoteConfig to opamp->OpampRemoteConfig to avoid collision and confusion with value's RemoteConfig
* feat: Add new case and test when we receive an empty remote config that forces to use local but local fails. In that case the remote config should still be reported as applied.
* feat: Return a reference in get_yaml_config
* feat: Modify fmt for imports in agent_control
* feat: parse remote before creating supervisor to be able to distinguish between a failed supervisor from remote or local
* feat: Add effective config reporting for reset to local and stop old supervisor if resetting to local fails
* feat: Add comments explaining error procedence

Author: alvarocabanas

agent-control/src/agent_control/agent_control.rs

@@ -4,7 +4,7 @@ use super::uptime_report::UptimeReportConfig;
 use crate::http::config::ProxyConfig;
 use crate::instrumentation::config::logs::config::LoggingConfig;
 use crate::opamp::auth::config::AuthConfig;
-use crate::opamp::remote_config::RemoteConfigError;
+use crate::opamp::remote_config::OpampRemoteConfigError;
 use crate::opamp::remote_config::validators::signature::validator::SignatureValidatorConfig;
 use crate::values::yaml_config::YAMLConfig;
 use crate::{
@@ -61,14 +61,16 @@ pub enum AgentControlConfigError {
     Load(String),
     #[error("storing agent control config: `{0}`")]
     Store(String),
+    #[error("updating agent control config: `{0}`")]
+    Update(String),
     #[error("building source to parse environment variables: `{0}`")]
     ConfigError(#[from] config::ConfigError),
     #[error("sub agent configuration `{0}` not found")]
     SubAgentNotFound(String),
     #[error("configuration is not valid YAML: `{0}`")]
     InvalidYamlConfiguration(#[from] serde_yaml::Error),
     #[error("remote config error: `{0}`")]
-    RemoteConfigError(#[from] RemoteConfigError),
+    RemoteConfigError(#[from] OpampRemoteConfigError),
     #[error("remote config error: `{0}`")]
     IOError(#[from] std::io::Error),
 }

agent-control/src/agent_control/config.rs

@@ -1,17 +1,35 @@
 use crate::agent_control::config::{
     AgentControlConfig, AgentControlConfigError, AgentControlDynamicConfig,
 };
-use crate::values::yaml_config::YAMLConfig;
+use crate::opamp::remote_config::hash::{ConfigState, Hash};
+use crate::values::config::RemoteConfig;
 
 /// AgentControlConfigLoader loads a whole AgentControlConfig
 #[cfg_attr(test, mockall::automock)]
 pub trait AgentControlConfigLoader {
     fn load(&self) -> Result<AgentControlConfig, AgentControlConfigError>;
 }
 
-/// AgentControlDynamicConfigStorer stores the dynamic part of the AgentControlConfig
-pub trait AgentControlDynamicConfigStorer {
-    fn store(&self, config: &YAMLConfig) -> Result<(), AgentControlConfigError>;
+/// AgentControlRemoteConfigStorer stores a remote_config containing
+/// the dynamic part of the AgentControlConfig and the remote config hash and status
+pub trait AgentControlRemoteConfigStorer {
+    fn store(&self, config: &RemoteConfig) -> Result<(), AgentControlConfigError>;
+}
+
+/// AgentControlRemoteConfigHashUpdater stores the hash and status for a remote_config
+pub trait AgentControlRemoteConfigHashStateUpdater {
+    fn update_hash_state(&self, state: &ConfigState) -> Result<(), AgentControlConfigError>;
+}
+
+/// AgentControlRemoteConfigHashGetter retrieves the hash and status
+/// from the stored remote_config if exists
+pub trait AgentControlRemoteConfigHashGetter {
+    fn get_hash(&self) -> Result<Option<Hash>, AgentControlConfigError>;
+}
+
+/// AgentControlRemoteConfigDeleter deletes the dynamic part of the AgentControlConfig
+pub trait AgentControlRemoteConfigDeleter {
+    fn delete(&self) -> Result<(), AgentControlConfigError>;
 }
 
 /// AgentControlDynamicConfigLoader loads the dynamic part of the AgentControlConfig
@@ -20,34 +38,36 @@ pub trait AgentControlDynamicConfigLoader {
     fn load(&self) -> Result<AgentControlDynamicConfig, AgentControlConfigError>;
 }
 
-/// AgentControlDynamicConfigDeleter deletes the dynamic part of the AgentControlConfig
-pub trait AgentControlDynamicConfigDeleter {
-    fn delete(&self) -> Result<(), AgentControlConfigError>;
-}
-
 #[cfg(test)]
 pub(crate) mod tests {
-    use super::AgentControlConfigError;
     use super::{
-        AgentControlDynamicConfigDeleter, AgentControlDynamicConfigLoader,
-        AgentControlDynamicConfigStorer,
+        AgentControlConfigError, AgentControlDynamicConfigLoader, AgentControlRemoteConfigDeleter,
+        AgentControlRemoteConfigHashGetter, AgentControlRemoteConfigHashStateUpdater,
+        AgentControlRemoteConfigStorer,
     };
     use crate::agent_control::config::AgentControlDynamicConfig;
-    use crate::values::yaml_config::YAMLConfig;
+    use crate::opamp::remote_config::hash::{ConfigState, Hash};
+    use crate::values::config::RemoteConfig;
     use mockall::{mock, predicate};
 
     mock! {
         pub AgentControlDynamicConfigStore {}
 
-        impl AgentControlDynamicConfigStorer for AgentControlDynamicConfigStore {
-            fn store(&self, config: &YAMLConfig) -> Result<(), AgentControlConfigError>;
+        impl AgentControlRemoteConfigStorer for AgentControlDynamicConfigStore {
+            fn store(&self, config: &RemoteConfig) -> Result<(), AgentControlConfigError>;
         }
         impl AgentControlDynamicConfigLoader for AgentControlDynamicConfigStore {
             fn load(&self) -> Result<AgentControlDynamicConfig, AgentControlConfigError>;
         }
-        impl AgentControlDynamicConfigDeleter for AgentControlDynamicConfigStore {
+        impl AgentControlRemoteConfigDeleter for AgentControlDynamicConfigStore {
             fn delete(&self) -> Result<(), AgentControlConfigError>;
         }
+        impl AgentControlRemoteConfigHashStateUpdater for AgentControlDynamicConfigStore {
+            fn update_hash_state(&self, state: &ConfigState) -> Result<(), AgentControlConfigError>;
+        }
+        impl AgentControlRemoteConfigHashGetter for AgentControlDynamicConfigStore {
+            fn get_hash(&self) -> Result<Option<Hash>, AgentControlConfigError>;
+        }
     }
 
     impl MockAgentControlDynamicConfigStore {
@@ -58,11 +78,10 @@ pub(crate) mod tests {
                 .returning(move || Ok(sub_agents_config.clone()));
         }
 
-        pub fn should_store(&mut self, sub_agents_config: &AgentControlDynamicConfig) {
-            let sub_agents_config: YAMLConfig = sub_agents_config.try_into().unwrap();
+        pub fn should_store(&mut self, remote_config: RemoteConfig) {
             self.expect_store()
                 .once()
-                .with(predicate::eq(sub_agents_config))
+                .with(predicate::eq(remote_config))
                 .returning(move |_| Ok(()));
         }
     }

agent-control/src/agent_control/config_storer/loader_storer.rs

@@ -3,20 +3,23 @@ use crate::agent_control::config::{
     AgentControlConfig, AgentControlConfigError, AgentControlDynamicConfig,
 };
 use crate::agent_control::config_storer::loader_storer::{
-    AgentControlConfigLoader, AgentControlDynamicConfigDeleter, AgentControlDynamicConfigLoader,
-    AgentControlDynamicConfigStorer,
+    AgentControlConfigLoader, AgentControlDynamicConfigLoader, AgentControlRemoteConfigDeleter,
+    AgentControlRemoteConfigHashGetter, AgentControlRemoteConfigHashStateUpdater,
+    AgentControlRemoteConfigStorer,
 };
 use crate::agent_control::defaults::{AGENT_CONTROL_CONFIG_ENV_VAR_PREFIX, default_capabilities};
-use crate::values::yaml_config::{YAMLConfig, YAMLConfigError};
-use crate::values::yaml_config_repository::{YAMLConfigRepository, YAMLConfigRepositoryError};
+use crate::opamp::remote_config::hash::{ConfigState, Hash};
+use crate::values::config::RemoteConfig;
+use crate::values::config_repository::{ConfigRepository, ConfigRepositoryError};
+use crate::values::yaml_config::YAMLConfigError;
 use config::builder::DefaultState;
 use config::{Config, ConfigBuilder, Environment, File, FileFormat};
 use opamp_client::operation::capabilities::Capabilities;
 use std::sync::Arc;
 
 pub struct AgentControlConfigStore<Y>
 where
-    Y: YAMLConfigRepository,
+    Y: ConfigRepository,
 {
     config_builder: ConfigBuilder<DefaultState>,
     values_repository: Arc<Y>,
@@ -26,7 +29,7 @@ where
 
 impl<Y> AgentControlConfigLoader for AgentControlConfigStore<Y>
 where
-    Y: YAMLConfigRepository,
+    Y: ConfigRepository,
 {
     fn load(&self) -> Result<AgentControlConfig, AgentControlConfigError> {
         self._load_config()
@@ -35,16 +38,16 @@ where
 
 impl<Y> AgentControlDynamicConfigLoader for AgentControlConfigStore<Y>
 where
-    Y: YAMLConfigRepository,
+    Y: ConfigRepository,
 {
     fn load(&self) -> Result<AgentControlDynamicConfig, AgentControlConfigError> {
         Ok(self._load_config()?.dynamic)
     }
 }
 
-impl<Y> AgentControlDynamicConfigDeleter for AgentControlConfigStore<Y>
+impl<Y> AgentControlRemoteConfigDeleter for AgentControlConfigStore<Y>
 where
-    Y: YAMLConfigRepository,
+    Y: ConfigRepository,
 {
     fn delete(&self) -> Result<(), AgentControlConfigError> {
         self.values_repository
@@ -53,20 +56,40 @@ where
     }
 }
 
-impl<Y> AgentControlDynamicConfigStorer for AgentControlConfigStore<Y>
+impl<Y> AgentControlRemoteConfigStorer for AgentControlConfigStore<Y>
 where
-    Y: YAMLConfigRepository,
+    Y: ConfigRepository,
 {
-    fn store(&self, yaml_config: &YAMLConfig) -> Result<(), AgentControlConfigError> {
+    fn store(&self, config: &RemoteConfig) -> Result<(), AgentControlConfigError> {
         self.values_repository
-            .store_remote(&self.agent_control_id, yaml_config)?;
+            .store_remote(&self.agent_control_id, config)?;
         Ok(())
     }
 }
 
+impl<Y> AgentControlRemoteConfigHashStateUpdater for AgentControlConfigStore<Y>
+where
+    Y: ConfigRepository,
+{
+    fn update_hash_state(&self, state: &ConfigState) -> Result<(), AgentControlConfigError> {
+        self.values_repository
+            .update_hash_state(&self.agent_control_id, state)?;
+        Ok(())
+    }
+}
+
+impl<Y> AgentControlRemoteConfigHashGetter for AgentControlConfigStore<Y>
+where
+    Y: ConfigRepository,
+{
+    fn get_hash(&self) -> Result<Option<Hash>, AgentControlConfigError> {
+        Ok(self.values_repository.get_hash(&self.agent_control_id)?)
+    }
+}
+
 impl<V> AgentControlConfigStore<V>
 where
-    V: YAMLConfigRepository,
+    V: ConfigRepository,
 {
     pub fn new(values_repository: Arc<V>) -> Self {
         let config_builder = Config::builder();
@@ -89,6 +112,8 @@ where
             .ok_or(AgentControlConfigError::Load(
                 "missing local agent control config".to_string(),
             ))?
+            .get_yaml_config()
+            .clone()
             .try_into()
             .map_err(|e: YAMLConfigError| AgentControlConfigError::Load(e.to_string()))?;
 
@@ -114,20 +139,22 @@ where
             .values_repository
             .load_remote(&self.agent_control_id, &self.agent_control_capabilities)?
         {
-            let dynamic_config: AgentControlDynamicConfig = remote_config.try_into()?;
+            let dynamic_config: AgentControlDynamicConfig =
+                remote_config.get_yaml_config().clone().try_into()?;
             config.dynamic = dynamic_config;
         }
 
         Ok(config)
     }
 }
 
-impl From<YAMLConfigRepositoryError> for AgentControlConfigError {
-    fn from(e: YAMLConfigRepositoryError) -> Self {
+impl From<ConfigRepositoryError> for AgentControlConfigError {
+    fn from(e: ConfigRepositoryError) -> Self {
         match e {
-            YAMLConfigRepositoryError::LoadError(e) => AgentControlConfigError::Load(e),
-            YAMLConfigRepositoryError::StoreError(e) => AgentControlConfigError::Store(e),
-            YAMLConfigRepositoryError::DeleteError(e) => AgentControlConfigError::Delete(e),
+            ConfigRepositoryError::LoadError(e) => AgentControlConfigError::Load(e),
+            ConfigRepositoryError::StoreError(e) => AgentControlConfigError::Store(e),
+            ConfigRepositoryError::DeleteError(e) => AgentControlConfigError::Delete(e),
+            ConfigRepositoryError::UpdateHashStateError(e) => AgentControlConfigError::Update(e),
         }
     }
 }
@@ -138,7 +165,7 @@ pub(crate) mod tests {
     use crate::agent_control::config::{AgentControlConfig, OpAMPClientConfig, SubAgentConfig};
     use crate::agent_control::defaults::AGENT_CONTROL_CONFIG_FILENAME;
     use crate::agent_type::agent_type_id::AgentTypeID;
-    use crate::values::file::YAMLConfigRepositoryFile;
+    use crate::values::file::ConfigRepositoryFile;
     use serial_test::serial;
     use std::path::PathBuf;
     use std::{collections::HashMap, env};
@@ -162,13 +189,16 @@ fleet_control:
         let remote_file = remote_dir.join(AGENT_CONTROL_CONFIG_FILENAME);
 
         let remote_config = r#"
-        agents:
-          rolldice:
-            agent_type: "namespace/com.newrelic.infrastructure:0.0.2"
+        config:
+            agents:
+              rolldice:
+                agent_type: "namespace/com.newrelic.infrastructure:0.0.2"
+        hash: a-hash
+        state: applying
         "#;
         std::fs::write(remote_file.as_path(), remote_config).unwrap();
 
-        let vr = YAMLConfigRepositoryFile::new(local_dir, remote_dir).with_remote();
+        let vr = ConfigRepositoryFile::new(local_dir, remote_dir).with_remote();
         let store = AgentControlConfigStore::new(Arc::new(vr));
         let actual = AgentControlConfigLoader::load(&store).unwrap();
 
@@ -215,7 +245,7 @@ fleet_control:
         let env_var_name = "NR_AC_AGENTS__ROLLDICE1__AGENT_TYPE";
         unsafe { env::set_var(env_var_name, "namespace/com.newrelic.infrastructure:0.0.2") };
 
-        let vr = YAMLConfigRepositoryFile::new(local_dir, PathBuf::new()).with_remote();
+        let vr = ConfigRepositoryFile::new(local_dir, PathBuf::new()).with_remote();
         let store = AgentControlConfigStore::new(Arc::new(vr));
         let actual = AgentControlConfigLoader::load(&store).unwrap();
 
@@ -264,7 +294,7 @@ agents:
         let env_var_name = "NR_AC_AGENTS__ROLLDICE2__AGENT_TYPE";
         unsafe { env::set_var(env_var_name, "namespace/com.newrelic.infrastructure:0.0.2") };
 
-        let vr = YAMLConfigRepositoryFile::new(local_dir, PathBuf::new()).with_remote();
+        let vr = ConfigRepositoryFile::new(local_dir, PathBuf::new()).with_remote();
         let store = AgentControlConfigStore::new(Arc::new(vr));
         let actual: AgentControlConfig = AgentControlConfigLoader::load(&store).unwrap();
 

agent-control/src/agent_control/config_storer/store.rs

@@ -6,15 +6,14 @@ use crate::agent_type::error::AgentTypeError;
 use crate::agent_type::render::persister::config_persister::PersistError;
 use crate::event::channel::EventPublisherError;
 use crate::opamp::client_builder::OpAMPClientBuilderError;
-use crate::opamp::hash_repository::repository::HashRepositoryError;
 use crate::opamp::instance_id;
 
 use crate::opamp::instance_id::on_host::getter::IdentifiersProviderError;
-use crate::opamp::remote_config::RemoteConfigError;
+use crate::opamp::remote_config::OpampRemoteConfigError;
 use crate::sub_agent::effective_agents_assembler::EffectiveAgentsAssemblerError;
 use crate::sub_agent::error::{SubAgentBuilderError, SubAgentCollectionError, SubAgentError};
+use crate::values::config_repository::ConfigRepositoryError;
 use crate::values::yaml_config::YAMLConfigError;
-use crate::values::yaml_config_repository::YAMLConfigRepositoryError;
 use fs::file_reader::FileReaderError;
 use opamp_client::{ClientError, NotStartedClientError, StartedClientError};
 use std::fmt::Debug;
@@ -71,17 +70,14 @@ pub enum AgentError {
     #[error("system time error: `{0}`")]
     SystemTime(#[from] SystemTimeError),
 
-    #[error("remote config hash error: `{0}`")]
-    RemoteConfigHash(#[from] HashRepositoryError),
-
     #[error("effective agents assembler error: `{0}`")]
     EffectiveAgentsAssembler(#[from] EffectiveAgentsAssemblerError),
 
     #[error("remote config error: `{0}`")]
-    RemoteConfig(#[from] RemoteConfigError),
+    RemoteConfig(#[from] OpampRemoteConfigError),
 
     #[error("sub agent remote config error: `{0}`")]
-    SubAgentRemoteConfig(#[from] YAMLConfigRepositoryError),
+    SubAgentRemoteConfig(#[from] ConfigRepositoryError),
 
     #[error("External module error: `{0}`")]
     ExternalError(String),