feat(variabled): emit a warning instead of exiting (#1005)

Author: paologallinaharbur

agent-control/src/agent_type/definition.rs

@@ -18,6 +18,7 @@ use crate::values::yaml_config::YAMLConfig;
 use opamp_client::operation::capabilities::Capabilities;
 use serde::{Deserialize, Deserializer};
 use std::{collections::HashMap, str::FromStr};
+use tracing::warn;
 
 /// AgentTypeDefinition represents the definition of an [AgentType]. It defines the variables and runtime for any supported
 /// environment.
@@ -294,15 +295,16 @@ fn update_specs(
     values: HashMap<String, serde_yaml::Value>,
     agent_vars: &mut HashMap<String, VariableDefinitionTree>,
 ) -> Result<(), AgentTypeError> {
-    for (ref k, v) in values.into_iter() {
-        let spec = agent_vars
-            .get_mut(k)
-            .ok_or_else(|| AgentTypeError::UnexpectedValueKey(k.clone()))?;
+    for (ref key, value) in values.into_iter() {
+        let Some(spec) = agent_vars.get_mut(key) else {
+            warn!(%key, "Unexpected variable in the configuration");
+            continue;
+        };
 
         match spec {
-            VariableDefinitionTree::End(e) => e.merge_with_yaml_value(v)?,
+            VariableDefinitionTree::End(e) => e.merge_with_yaml_value(value)?,
             VariableDefinitionTree::Mapping(m) => {
-                let v: HashMap<String, serde_yaml::Value> = serde_yaml::from_value(v)?;
+                let v: HashMap<String, serde_yaml::Value> = serde_yaml::from_value(value)?;
                 update_specs(v, m)?
             }
         }
@@ -352,7 +354,8 @@ pub(crate) type Variables = HashMap<String, VariableDefinition>;
 
 #[cfg(test)]
 pub mod tests {
-
+    use super::*;
+    use crate::agent_type::runtime_config::Deployment;
     use crate::{
         agent_type::{
             environment::Environment,
@@ -362,9 +365,6 @@ pub mod tests {
         },
         sub_agent::effective_agents_assembler::build_agent_type,
     };
-
-    use super::*;
-    use crate::agent_type::runtime_config::Deployment;
     use assert_matches::assert_matches;
     use serde_yaml::{Error, Number};
     use std::collections::HashMap as Map;
@@ -556,7 +556,6 @@ deployment:
     #[test]
     fn test_normalize_agent_spec() {
         // create AgentSpec
-
         let given_agent = AgentType::build_for_testing(AGENT_GIVEN_YAML, &Environment::OnHost);
 
         let expected_map: Map<String, VariableDefinition> = Map::from([(
@@ -718,8 +717,8 @@ deployment:
             args: TemplateableValue {
                 value: Some(Args("--config config_path --plugin_dir integration_path --verbose true --logs trace".to_string())),
                 template:
-                    "--config ${nr-var:config} --plugin_dir ${nr-var:integrations} --verbose ${nr-var:deployment.on_host.verbose} --logs ${nr-var:deployment.on_host.log_level}"
-                        .to_string(),
+                "--config ${nr-var:config} --plugin_dir ${nr-var:integrations} --verbose ${nr-var:deployment.on_host.verbose} --logs ${nr-var:deployment.on_host.log_level}"
+                    .to_string(),
             },
             env: Env::default(),
             restart_policy: RestartPolicyConfig {
@@ -911,6 +910,7 @@ deployment:
 "#;
 
     const GIVEN_NEWRELIC_INFRA_USER_CONFIG_YAML: &str = r#"
+unknown_variable: ignored
 config3:
   log_level: trace
   forward: "true"
@@ -1018,7 +1018,8 @@ status_server_port: 8004
                 .as_ref()
                 .unwrap()
                 .clone()
-        )
+        );
+        assert!(!filled_variables.contains_key("unknown_variable"))
     }
 
     const AGENT_TYPE_WITH_VARIANTS: &str = r#"
@@ -1055,16 +1056,9 @@ restart_policy:
     fn test_variables_with_variants() {
         let agent_type =
             AgentType::build_for_testing(AGENT_TYPE_WITH_VARIANTS, &Environment::OnHost);
-        let values: YAMLConfig =
-            serde_yaml::from_str(VALUES_VALID_VARIANT).expect("Failed to parse user config");
 
         // Valid variant
-        let filled_variables = agent_type
-            .variables
-            .clone()
-            .fill_with_values(values)
-            .unwrap()
-            .flatten();
+        let filled_variables = agent_type.fill_variables(VALUES_VALID_VARIANT);
 
         let var = filled_variables.get("restart_policy.type").unwrap();
         assert_eq!(
@@ -1086,12 +1080,7 @@ restart_policy:
         );
 
         // Default invalid variant is allowed
-        let filled_variables_default = agent_type
-            .variables
-            .clone()
-            .fill_with_values(YAMLConfig::default())
-            .unwrap()
-            .flatten();
+        let filled_variables_default = agent_type.fill_variables("");
         let var = filled_variables_default.get("restart_policy.type").unwrap();
         assert_eq!(
             "exponential".to_string(),

agent-control/src/agent_type/error.rs

@@ -1,5 +1,4 @@
 use crate::sub_agent::persister::config_persister::PersistError;
-use std::io;
 use thiserror::Error;
 
 /// The different error types to be returned by operations involving the [`Agent`] type.
@@ -9,12 +8,8 @@ pub enum AgentTypeError {
     SerdeYaml(#[from] serde_yaml::Error),
     #[error("Missing value for key: `{0}`")]
     MissingValue(String),
-    #[error("Unexpected key in agent type config values: {0}")]
-    UnexpectedValueKey(String),
     #[error("Unexpected value for key: key({0}) val({1})")]
     UnexpectedValueForKey(String, String),
-    #[error("I/O error: `{0}`")]
-    IOError(#[from] io::Error),
     #[error("Missing required template key: `{0}`")]
     MissingTemplateKey(String),
     #[error("Missing default value for a non-required spec key")]

agent-control/src/agent_type/renderer.rs

@@ -308,16 +308,12 @@ pub(crate) mod tests {
     fn test_render_with_persister() {
         let agent_id = AgentID::new("some-agent-id").unwrap();
         let agent_type = AgentType::build_for_testing(AGENT_TYPE_WITH_FILES, &Environment::OnHost);
-        let values = testing_values(AGENT_VALUES_WITH_FILES);
+        let values = AGENT_VALUES_WITH_FILES;
         let attributes = testing_agent_attributes(&agent_id);
         // The persister should receive filled variables with the path expanded.
         let path_as_string = test_data_dir().join(GENERATED_FOLDER_NAME).join(&agent_id);
-        let filled_variables = agent_type
-            .variables
-            .clone()
-            .fill_with_values(values.clone())
-            .unwrap()
-            .flatten();
+        let filled_variables = agent_type.fill_variables(values);
+
         let expanded_path_filled_variables =
             TemplateRenderer::<MockConfigurationPersisterMock>::extend_variables_file_path(
                 path_as_string.clone(),
@@ -331,7 +327,7 @@ pub(crate) mod tests {
         let renderer = TemplateRenderer::default().with_config_persister(persister);
 
         let runtime_config = renderer
-            .render(&agent_id, agent_type, values, attributes)
+            .render(&agent_id, agent_type, testing_values(values), attributes)
             .unwrap();
         assert_eq!(
             Args(format!(
@@ -379,14 +375,9 @@ pub(crate) mod tests {
     fn test_render_with_persister_persists_error() {
         let agent_id = AgentID::new("some-agent-id").unwrap();
         let agent_type = AgentType::build_for_testing(SIMPLE_AGENT_TYPE, &Environment::OnHost);
-        let values = testing_values(SIMPLE_AGENT_VALUES);
+        let values = SIMPLE_AGENT_VALUES;
         let attributes = testing_agent_attributes(&agent_id);
-        let filled_variables = agent_type
-            .variables
-            .clone()
-            .fill_with_values(values.clone())
-            .unwrap()
-            .flatten();
+        let filled_variables = agent_type.fill_variables(values);
 
         let mut persister = MockConfigurationPersisterMock::new();
         let err = PersistError::DirectoryError(DirectoryManagementError::ErrorDeletingDirectory(
@@ -398,7 +389,7 @@ pub(crate) mod tests {
         let renderer = TemplateRenderer::default().with_config_persister(persister);
 
         let expected_error = renderer
-            .render(&agent_id, agent_type, values, attributes)
+            .render(&agent_id, agent_type, testing_values(values), attributes)
             .err()
             .unwrap();
         assert_matches!(

agent-control/src/sub_agent/on_host/builder.rs

@@ -215,9 +215,8 @@ mod tests {
     use super::*;
     use crate::agent_control::config::AgentTypeFQN;
     use crate::agent_control::defaults::{
-        default_capabilities, default_sub_agent_custom_capabilities,
-        OPAMP_AGENT_VERSION_ATTRIBUTE_KEY, OPAMP_SERVICE_NAME, OPAMP_SERVICE_NAMESPACE,
-        OPAMP_SERVICE_VERSION, PARENT_AGENT_ID_ATTRIBUTE_KEY,
+        default_capabilities, default_sub_agent_custom_capabilities, OPAMP_SERVICE_NAME,
+        OPAMP_SERVICE_NAMESPACE, OPAMP_SERVICE_VERSION, PARENT_AGENT_ID_ATTRIBUTE_KEY,
     };
     use crate::agent_type::environment::Environment;
     use crate::agent_type::runtime_config::{Deployment, OnHost, Runtime};

agent-control/tests/on_host/scenarios/opamp.rs

@@ -502,7 +502,8 @@ fn onhost_opamp_sub_agent_wrong_remote_effective_config() {
     // When a new incorrect config is received from OpAMP
     opamp_server.set_config_response(
         sub_agent_instance_id.clone(),
-        ConfigResponse::from("config_agent: aa"),
+        // The configuration is invalid since a string is expected
+        ConfigResponse::from("backoff_delay: 123"),
     );
 
     retry(60, Duration::from_secs(1), || {