NR-179188 report invalid Sub Agent remote config

NR-179188 store config state with hash

Author: rubenruizdegauna

src/opamp/remote_config_hash.rs

@@ -18,10 +18,20 @@ use crate::super_agent::defaults::{REMOTE_AGENT_DATA_DIR, SUPER_AGENT_DATA_DIR};
 #[cfg(target_family = "unix")]
 const DIRECTORY_PERMISSIONS: u32 = 0o700;
 
+#[derive(Debug, Serialize, Deserialize, PartialEq, Clone, Hash, Eq)]
+#[serde(rename_all = "snake_case")]
+#[serde(tag = "state")]
+enum ConfigState {
+    Applying,
+    Applied,
+    Failed { error_message: String },
+}
+
 #[derive(Debug, Serialize, Deserialize, PartialEq, Clone, Hash, Eq)]
 pub struct Hash {
     hash: String,
-    applied: bool,
+    #[serde(flatten)]
+    state: ConfigState,
 }
 
 #[derive(Error, Debug)]
@@ -43,22 +53,44 @@ pub enum HashRepositoryError {
 }
 
 impl Hash {
-    pub fn new(hash: String) -> Self {
-        Self {
-            hash,
-            applied: false,
-        }
-    }
     pub fn get(&self) -> String {
         self.hash.clone()
     }
-
     pub fn is_applied(&self) -> bool {
-        self.applied
+        self.state == ConfigState::Applied
+    }
+
+    pub fn is_applying(&self) -> bool {
+        self.state == ConfigState::Applying
+    }
+
+    pub fn is_failed(&self) -> bool {
+        // if let self.state = ConfigState::Failed(msg)
+        matches!(&self.state, ConfigState::Failed { .. })
     }
 
+    pub fn error_message(&self) -> Option<String> {
+        match &self.state {
+            ConfigState::Failed { error_message: msg } => Some(msg.clone()),
+            _ => None,
+        }
+    }
+}
+
+impl Hash {
+    pub fn new(hash: String) -> Self {
+        Self {
+            hash,
+            state: ConfigState::Applying,
+        }
+    }
     pub fn apply(&mut self) {
-        self.applied = true
+        self.state = ConfigState::Applied;
+    }
+
+    // It is mandatory for a failed hash to have the error
+    pub fn fail(&mut self, error_message: String) {
+        self.state = ConfigState::Failed { error_message };
     }
 }
 
@@ -168,8 +200,8 @@ pub mod test {
     use std::os::unix::fs::PermissionsExt;
 
     use super::{
-        Hash, HashRepository, HashRepositoryError, HashRepositoryFile, DIRECTORY_PERMISSIONS,
-        HASH_FILE_EXTENSION,
+        ConfigState, Hash, HashRepository, HashRepositoryError, HashRepositoryFile,
+        DIRECTORY_PERMISSIONS, HASH_FILE_EXTENSION,
     };
     use crate::config::persister::config_persister_file::FILE_PERMISSIONS;
     use crate::config::persister::config_writer_file::test::MockFileWriterMock;
@@ -186,8 +218,15 @@ pub mod test {
     impl Hash {
         pub fn applied(hash: String) -> Self {
             Self {
-                applied: true,
                 hash,
+                state: ConfigState::Applied,
+            }
+        }
+
+        pub fn failed(hash: String, error_message: String) -> Self {
+            Self {
+                hash,
+                state: ConfigState::Failed { error_message },
             }
         }
     }
@@ -206,15 +245,11 @@ pub mod test {
     }
 
     impl MockHashRepositoryMock {
-        pub fn should_get_applied_hash(&mut self, agent_id: &AgentID, hash: Hash) {
+        pub fn should_get_hash(&mut self, agent_id: &AgentID, hash: Hash) {
             self.expect_get()
                 .with(predicate::eq(agent_id.clone()))
                 .once()
-                .returning(move |_| {
-                    let mut hash = hash.clone();
-                    hash.apply();
-                    Ok(hash)
-                });
+                .returning(move |_| Ok(hash.clone()));
         }
         pub fn should_save_hash(&mut self, agent_id: &AgentID, hash: &Hash) {
             self.expect_save()
@@ -257,7 +292,7 @@ pub mod test {
 
         // This indentation and the single quotes is to match serde yaml
         let content = r#"hash: '123456789'
-applied: true
+state: applied
 "#;
 
         let mut expected_path = some_path.clone();
@@ -294,4 +329,53 @@ applied: true
         let result = hash_repository.get(&agent_id);
         assert_eq!(hash, result.unwrap());
     }
+
+    #[test]
+    fn test_config_state_default_status() {
+        //default status for a hash should be applying
+        let hash = Hash::new("some-hash".into());
+        assert!(hash.is_applying())
+    }
+
+    #[test]
+    fn test_config_state_transition() {
+        // hash can change state. This is not ideal, as an applied hash should not go to failed
+        let mut hash = Hash::new("some-hash".into());
+        assert!(hash.is_applying());
+        hash.apply();
+        assert!(hash.is_applied());
+        hash.fail("this is an error message".to_string());
+        assert!(hash.is_failed());
+    }
+
+    #[test]
+    fn test_hash_serialization() {
+        let mut hash = Hash::new("123456789".to_string());
+        let expected = "hash: '123456789'\nstate: applying\n";
+        assert_eq!(expected, serde_yaml::to_string(&hash).unwrap());
+
+        hash.apply();
+        let expected = "hash: '123456789'\nstate: applied\n";
+        assert_eq!(expected, serde_yaml::to_string(&hash).unwrap());
+
+        hash.fail("this is an error message".to_string());
+        let expected =
+            "hash: '123456789'\nstate: failed\nerror_message: this is an error message\n";
+        assert_eq!(expected, serde_yaml::to_string(&hash).unwrap());
+    }
+
+    #[test]
+    fn test_hash_deserialization() {
+        let mut hash = Hash::new("123456789".to_string());
+        let content = "hash: '123456789'\nstate: applying\n";
+        assert_eq!(hash, serde_yaml::from_str::<Hash>(&content).unwrap());
+
+        hash.apply();
+        let content = "hash: '123456789'\nstate: applied\n";
+        assert_eq!(hash, serde_yaml::from_str::<Hash>(&content).unwrap());
+
+        hash.fail("this is an error message".to_string());
+        let content = "hash: '123456789'\nstate: failed\nerror_message: this is an error message\n";
+        assert_eq!(hash, serde_yaml::from_str::<Hash>(&content).unwrap());
+    }
 }

src/sub_agent/on_host/builder.rs

@@ -117,10 +117,18 @@ where
                         Vec::default(),
                         maybe_opamp_client,
                     )?);
-                } else if !hash.is_applied() {
+                } else if hash.is_applying() {
                     report_remote_config_status_applied(opamp_client, &hash)?;
                     hash.apply();
                     self.hash_repository.save(&agent_id, &hash)?;
+                } else if hash.is_failed() {
+                    // failed hash always has the error message
+                    let error_message = hash.error_message().unwrap();
+                    report_remote_config_status_error(
+                        opamp_client,
+                        &hash,
+                        error_message.to_string(),
+                    )?;
                 }
             }
         }
@@ -170,6 +178,8 @@ mod test {
     use std::sync::mpsc::channel;
 
     use nix::unistd::gethostname;
+    use opamp_client::opamp::proto::RemoteConfigStatus;
+    use opamp_client::opamp::proto::RemoteConfigStatuses::Failed;
     use opamp_client::operation::{
         capabilities::Capabilities,
         settings::{AgentDescription, DescriptionValueType, StartSettings},
@@ -256,6 +266,71 @@ mod test {
             .is_ok())
     }
 
+    #[test]
+    fn test_builder_should_report_failed_config() {
+        let ctx = Context::new();
+        let (tx, _rx) = channel();
+        // Mocks
+        let mut opamp_builder = MockOpAMPClientBuilderMock::new();
+        let mut hash_repository_mock = MockHashRepositoryMock::new();
+        let mut instance_id_getter = MockInstanceIDGetterMock::new();
+        let mut effective_agent_assembler = MockEffectiveAgentAssemblerMock::new();
+
+        // Structures
+        let hostname = gethostname().unwrap_or_default().into_string().unwrap();
+        let start_settings_infra = infra_agent_default_start_settings(&hostname);
+        let final_agent = on_host_final_agent();
+        let sub_agent_id = AgentID::new("infra_agent").unwrap();
+        let sub_agent_config = SubAgentConfig {
+            agent_type: final_agent.agent_type().clone(),
+        };
+
+        // Expectations
+        // Infra Agent OpAMP no final stop nor health, just after stopping on reload
+        instance_id_getter.should_get(
+            sub_agent_id.to_string(),
+            "infra_agent_instance_id".to_string(),
+        );
+
+        opamp_builder.should_build_and_start(
+            sub_agent_id.clone(),
+            start_settings_infra,
+            |_, _, _| {
+                let mut started_client = MockOpAMPClientMock::new();
+                // failed conf should be reported
+                started_client.should_set_remote_config_status(RemoteConfigStatus {
+                    error_message: "this is an error message".to_string(),
+                    status: Failed as i32,
+                    last_remote_config_hash: "a-hash".as_bytes().to_vec(),
+                });
+                Ok(started_client)
+            },
+        );
+
+        effective_agent_assembler.should_assemble_agent(
+            &sub_agent_id,
+            &sub_agent_config,
+            final_agent,
+        );
+
+        // return a failed hash
+        let failed_hash =
+            Hash::failed("a-hash".to_string(), "this is an error message".to_string());
+        hash_repository_mock.should_get_hash(&sub_agent_id, failed_hash);
+
+        // Sub Agent Builder
+        let on_host_builder = OnHostSubAgentBuilder::new(
+            Some(&opamp_builder),
+            &instance_id_getter,
+            &hash_repository_mock,
+            &effective_agent_assembler,
+        );
+
+        assert!(on_host_builder
+            .build(sub_agent_id, &sub_agent_config, tx, ctx)
+            .is_ok());
+    }
+
     // HELPERS
     fn on_host_final_agent() -> FinalAgent {
         let mut final_agent: FinalAgent = FinalAgent::default();

src/super_agent/super_agent.rs

@@ -385,7 +385,7 @@ where
     // Sub Agent on remote config
     fn process_sub_agent_remote_config(
         &self,
-        remote_config: RemoteConfig,
+        mut remote_config: RemoteConfig,
         sub_agents: &mut StartedSubAgents<
             <S::NotStartedSubAgent as NotStartedSubAgent>::StartedSubAgent,
         >,
@@ -397,13 +397,28 @@ where
         self.sub_agent_remote_config_hash_repository
             .save(&remote_config.agent_id, &remote_config.hash)?;
         let remote_config_value = remote_config.get_unique()?;
+        // If remote config is empty, we delete the persisted remote config so later the store
+        // will load the local config
         if remote_config_value.is_empty() {
             self.remote_values_repo
                 .delete_remote(&remote_config.agent_id)?;
         } else {
-            let agent_values = AgentValues::try_from(remote_config_value.to_string())?;
-            self.remote_values_repo
-                .store_remote(&remote_config.agent_id, &agent_values)?;
+            // If the config is not valid log we cannot report it to OpAMP as
+            // we don't have access to the Sub Agent OpAMP Client here (yet) so
+            // for now we mark the remote config as failed and we don't persist it.
+            // When the Sub Agent is "recreated" it will report the remote config
+            // as failed.
+            match AgentValues::try_from(remote_config_value.to_string()) {
+                Err(e) => {
+                    error!("Error applying Sub Agent remote config: {}", e);
+                    remote_config.hash.fail(e.to_string());
+                    self.sub_agent_remote_config_hash_repository
+                        .save(&remote_config.agent_id, &remote_config.hash)?;
+                }
+                Ok(agent_values) => self
+                    .remote_values_repo
+                    .store_remote(&remote_config.agent_id, &agent_values)?,
+            }
         }
 
         let config = self.sub_agents_config_store.load()?;
@@ -451,7 +466,7 @@ where
         if let Err(err) =
             self.apply_remote_config(remote_config.clone(), tx, running_sub_agents, ctx)
         {
-            let error_message = format!("Error applying remote config: {}", err);
+            let error_message = format!("Error applying Super Agent remote config: {}", err);
             error!(error_message);
             Ok(report_remote_config_status_error(
                 opamp_client,
@@ -1219,9 +1234,9 @@ config_file: /some/path/newrelic-infra.yml
             .times(2)
             .returning(move || Ok(sub_agents_config.clone()));
 
-        hash_repository_mock.should_get_applied_hash(
+        hash_repository_mock.should_get_hash(
             &AgentID::new_super_agent_id(),
-            Hash::new("a-hash".to_string()),
+            Hash::applied("a-hash".to_string()),
         );
 
         // two agents in the supervisor group
@@ -1293,9 +1308,9 @@ config_file: /some/path/newrelic-infra.yml
         sub_agent_builder.should_build(3);
 
         let mut hash_repository_mock = MockHashRepositoryMock::new();
-        hash_repository_mock.should_get_applied_hash(
+        hash_repository_mock.should_get_hash(
             &AgentID::new_super_agent_id(),
-            Hash::new("a-hash".to_string()),
+            Hash::applied("a-hash".to_string()),
         );
 
         let mut sub_agents_config_store = MockSubAgentsConfigStore::new();
@@ -1506,7 +1521,7 @@ agents:
         let status = RemoteConfigStatus {
             status: Failed as i32,
             last_remote_config_hash: remote_config.hash.get().into_bytes(),
-            error_message: "Error applying remote config: could not resolve config: `configuration is not valid YAML: `invalid type: string \"invalid_yaml_content:{}\", expected struct SubAgentsConfig``".to_string(),
+            error_message: "Error applying Super Agent remote config: could not resolve config: `configuration is not valid YAML: `invalid type: string \"invalid_yaml_content:{}\", expected struct SubAgentsConfig``".to_string(),
         };
         started_client.should_set_remote_config_status(status);