fix: validate if agent-id collides with release-name (#1742)

sigilioso · web-flow · commit d3b2bdf54d90 · 2025-10-02T16:14:26.000+02:00
diff --git a/agent-control/src/agent_control/config_validator.rs b/agent-control/src/agent_control/config_validator.rs
@@ -1,13 +1,13 @@
 use crate::agent_control::config::AgentControlDynamicConfig;
-use crate::agent_type::agent_type_registry::{AgentRegistry, AgentRepositoryError};
+use crate::agent_type::agent_type_registry::AgentRegistry;
 use std::sync::Arc;
 use thiserror::Error;
 
+pub mod k8s;
+
 #[derive(Error, Debug)]
-pub enum DynamicConfigValidatorError {
-    #[error("{0}")]
-    AgentRepositoryError(#[from] AgentRepositoryError),
-}
+#[error("config validation failed: {0}")]
+pub struct DynamicConfigValidatorError(String);
 
 /// Represents a validator for dynamic config
 pub trait DynamicConfigValidator {
@@ -40,7 +40,12 @@ impl<R: AgentRegistry> DynamicConfigValidator for RegistryDynamicConfigValidator
             .try_for_each(|sub_agent_cfg| {
                 let _ = self
                     .agent_type_registry
-                    .get(sub_agent_cfg.agent_type.to_string().as_str())?;
+                    .get(sub_agent_cfg.agent_type.to_string().as_str())
+                    .map_err(|err| {
+                        DynamicConfigValidatorError(format!(
+                            "AgentType registry check failed: {err}"
+                        ))
+                    })?;
                 Ok(())
             })
     }
@@ -78,9 +83,7 @@ pub mod tests {
             if self.valid {
                 Ok(())
             } else {
-                Err(DynamicConfigValidatorError::AgentRepositoryError(
-                    AgentRepositoryError::NotFound("not-found".to_string()),
-                ))
+                Err(DynamicConfigValidatorError("not-found".to_string()))
             }
         }
     }
diff --git a/agent-control/src/agent_control/config_validator/k8s.rs b/agent-control/src/agent_control/config_validator/k8s.rs
@@ -0,0 +1,148 @@
+use crate::agent_control::config::AgentControlDynamicConfig;
+
+use super::{DynamicConfigValidator, DynamicConfigValidatorError};
+
+pub struct K8sReleaseNamesConfigValidator<V: DynamicConfigValidator> {
+    inner: V,
+    ac_release_name: String,
+    cd_release_name: String,
+}
+
+impl<V: DynamicConfigValidator> DynamicConfigValidator for K8sReleaseNamesConfigValidator<V> {
+    fn validate(
+        &self,
+        dynamic_config: &AgentControlDynamicConfig,
+    ) -> Result<(), DynamicConfigValidatorError> {
+        self.inner.validate(dynamic_config)?;
+
+        dynamic_config
+            .agents
+            .keys()
+            .try_for_each(|agent_id| match agent_id.as_str() {
+                agent_id if agent_id == self.ac_release_name => Err(validation_error(
+                    agent_id,
+                    "Agent Control itself (agent-control-deployment)",
+                )),
+                agent_id if agent_id == self.cd_release_name => Err(validation_error(
+                    agent_id,
+                    "Agent Control CD (agent-control-cd)",
+                )),
+                _ => Ok(()),
+            })
+    }
+}
+
+impl<V: DynamicConfigValidator> K8sReleaseNamesConfigValidator<V> {
+    pub fn new(inner: V, ac_release_name: String, cd_release_name: String) -> Self {
+        Self {
+            inner,
+            ac_release_name,
+            cd_release_name,
+        }
+    }
+}
+
+fn validation_error(agent_id: &str, component: &str) -> DynamicConfigValidatorError {
+    DynamicConfigValidatorError(format!(
+        "agent_id '{agent_id}' collides with the release name to deploy {component}"
+    ))
+}
+
+#[cfg(test)]
+mod tests {
+    use assert_matches::assert_matches;
+
+    use super::*;
+    use crate::agent_control::{
+        config::tests::helper_get_agent_list, config_validator::tests::MockDynamicConfigValidator,
+    };
+
+    #[test]
+    fn test_validate_no_collision() {
+        let mut inner = MockDynamicConfigValidator::new();
+        inner.expect_validate().once().returning(|_| Ok(()));
+
+        let validator = K8sReleaseNamesConfigValidator {
+            inner,
+            ac_release_name: "agent-control-deployment".to_string(),
+            cd_release_name: "agent-control-cd".to_string(),
+        };
+
+        let dynamic_config = AgentControlDynamicConfig {
+            agents: helper_get_agent_list(), // contains 'infra-agent' and 'nrdot' agent-ids
+            ..Default::default()
+        };
+
+        let result = validator.validate(&dynamic_config);
+        assert!(result.is_ok());
+    }
+
+    #[test]
+    fn test_validate_inner_failure() {
+        let mut inner = MockDynamicConfigValidator::new();
+        inner
+            .expect_validate()
+            .once()
+            .returning(|_| Err(DynamicConfigValidatorError("inner failure".to_string())));
+
+        let validator = K8sReleaseNamesConfigValidator {
+            inner,
+            ac_release_name: "agent-control-deployment".to_string(),
+            cd_release_name: "agent-control-cd".to_string(),
+        };
+
+        let dynamic_config = AgentControlDynamicConfig {
+            agents: helper_get_agent_list(), // contains 'infra-agent' and 'nrdot' agent-ids
+            ..Default::default()
+        };
+
+        let result = validator.validate(&dynamic_config);
+        assert_matches!(result, Err(DynamicConfigValidatorError(s)) => {
+            assert!(s.contains("inner failure"));
+        });
+    }
+
+    #[test]
+    fn test_validate_ac_release_name_collision() {
+        let mut inner = MockDynamicConfigValidator::new();
+        inner.expect_validate().once().returning(|_| Ok(()));
+
+        let validator = K8sReleaseNamesConfigValidator {
+            inner,
+            ac_release_name: "nrdot".to_string(),
+            cd_release_name: "agent-control-cd".to_string(),
+        };
+
+        let dynamic_config = AgentControlDynamicConfig {
+            agents: helper_get_agent_list(), // contains 'infra-agent' and 'nrdot' agent-ids
+            ..Default::default()
+        };
+
+        let result = validator.validate(&dynamic_config);
+        assert_matches!(result, Err(DynamicConfigValidatorError(s)) => {
+            assert!(s.contains("nrdot") && s.contains("agent-control-deployment"));
+        });
+    }
+
+    #[test]
+    fn test_validate_cd_release_name_collision() {
+        let mut inner = MockDynamicConfigValidator::new();
+        inner.expect_validate().once().returning(|_| Ok(()));
+
+        let validator = K8sReleaseNamesConfigValidator {
+            inner,
+            ac_release_name: "random-release-name".to_string(),
+            cd_release_name: "infra-agent".to_string(),
+        };
+
+        let dynamic_config = AgentControlDynamicConfig {
+            agents: helper_get_agent_list(), // contains 'infra-agent' and 'nrdot' agent-ids
+            ..Default::default()
+        };
+
+        let result = validator.validate(&dynamic_config);
+        assert_matches!(result, Err(DynamicConfigValidatorError(s)) => {
+            assert!(s.contains("infra-agent") && s.contains("agent-control-cd"));
+        });
+    }
+}
diff --git a/agent-control/src/agent_control/resource_cleaner/k8s_garbage_collector.rs b/agent-control/src/agent_control/resource_cleaner/k8s_garbage_collector.rs
@@ -32,8 +32,6 @@ pub struct K8sGarbageCollector {
     /// The namespace where agents are running. We are garbage collecting resources here only due to Instrumentation
     pub namespace_agents: String,
     pub cr_type_meta: Vec<TypeMeta>,
-    pub ac_release_name: String,
-    pub cd_release_name: String,
 }
 
 impl K8sGarbageCollector {
@@ -142,13 +140,6 @@ impl K8sGarbageCollector {
             .ok_or(K8sGarbageCollectorError::MissingLabels)?
             .as_str();
 
-        // TODO AgentId should be aware of the "ac" and "cd" release names.
-        if agent_id_from_labels == self.ac_release_name
-            || agent_id_from_labels == self.cd_release_name
-        {
-            return Ok(false);
-        }
-
         let agent_id_from_labels = match AgentID::try_from(agent_id_from_labels) {
             Ok(id) => id,
             // We must not delete anything with reserved AgentIDs (currently only Agent Control)
@@ -277,8 +268,6 @@ mod tests {
 
     const TEST_NAMESPACE: &str = "test-namespace";
     const TEST_NAMESPACE_AGENTS: &str = "test-namespace-agents";
-    const TEST_AC_RELEASE_NAME: &str = "test-ac-release-name";
-    const TEST_CD_RELEASE_NAME: &str = "test-cd-release-name";
 
     #[test]
     fn errors_if_ac_id() {
@@ -293,8 +282,6 @@ mod tests {
             cr_type_meta: vec![],
             namespace: TEST_NAMESPACE.to_string(),
             namespace_agents: TEST_NAMESPACE_AGENTS.to_string(),
-            ac_release_name: TEST_AC_RELEASE_NAME.to_string(),
-            cd_release_name: TEST_CD_RELEASE_NAME.to_string(),
         };
         let ac_id = &AgentID::AgentControl;
         let ac_type_id =
@@ -339,8 +326,6 @@ mod tests {
             cr_type_meta: vec![type_meta],
             namespace: TEST_NAMESPACE.to_string(),
             namespace_agents: TEST_NAMESPACE_AGENTS.to_string(),
-            ac_release_name: TEST_AC_RELEASE_NAME.to_string(),
-            cd_release_name: TEST_CD_RELEASE_NAME.to_string(),
         };
         let ac_id = &AgentID::try_from("foo-agent").unwrap();
         let agent_type_id = &AgentTypeID::try_from("newrelic/com.example.foo:0.0.1").unwrap();
diff --git a/agent-control/src/agent_control/run/k8s.rs b/agent-control/src/agent_control/run/k8s.rs
@@ -3,6 +3,7 @@ use crate::agent_control::config::{AgentControlConfigError, K8sConfig, helmrelea
 use crate::agent_control::config_repository::repository::AgentControlConfigLoader;
 use crate::agent_control::config_repository::store::AgentControlConfigStore;
 use crate::agent_control::config_validator::RegistryDynamicConfigValidator;
+use crate::agent_control::config_validator::k8s::K8sReleaseNamesConfigValidator;
 use crate::agent_control::defaults::{
     AGENT_CONTROL_VERSION, FLEET_ID_ATTRIBUTE_KEY, HOST_NAME_ATTRIBUTE_KEY,
     OPAMP_AC_CHART_VERSION_ATTRIBUTE_KEY, OPAMP_AGENT_VERSION_ATTRIBUTE_KEY,
@@ -189,8 +190,6 @@ impl AgentControlRunner {
             namespace: self.k8s_config.namespace.clone(),
             namespace_agents: self.k8s_config.namespace_agents.clone(),
             cr_type_meta: self.k8s_config.cr_type_meta,
-            ac_release_name: self.k8s_config.ac_release_name.clone(),
-            cd_release_name: self.k8s_config.cd_release_name.clone(),
         };
 
         info!("Initiating cleanup of outdated resources from previous Agent Control executions");
@@ -202,9 +201,15 @@ impl AgentControlRunner {
             ))
             .map_err(ResourceCleanerError::from)?;
 
-        let dynamic_config_validator =
+        let registry_config_validator =
             RegistryDynamicConfigValidator::new(self.agent_type_registry);
 
+        let dynamic_config_validator = K8sReleaseNamesConfigValidator::new(
+            registry_config_validator,
+            self.k8s_config.ac_release_name.to_string(),
+            self.k8s_config.cd_release_name.to_string(),
+        );
+
         // The http server stops on Drop. We need to keep it while the agent control is running.
         let _http_server = self.http_server_runner.map(Runner::start);
 
diff --git a/agent-control/tests/k8s/garbage_collector.rs b/agent-control/tests/k8s/garbage_collector.rs
@@ -41,9 +41,6 @@ use newrelic_agent_control::{
 };
 use std::{collections::HashMap, sync::Arc, time::Duration};
 
-const TEST_AC_RELEASE_NAME: &str = "test-ac-release-name";
-const TEST_CD_RELEASE_NAME: &str = "test-cd-release-name";
-
 // Setup AgentControlConfigLoader mock
 mock! {
     pub AgentControlConfigLoader {}
@@ -176,8 +173,6 @@ agents:
                 kind: "Secret".to_string(),
             },
         ],
-        ac_release_name: TEST_AC_RELEASE_NAME.to_string(),
-        cd_release_name: TEST_CD_RELEASE_NAME.to_string(),
     };
 
     // Expects the GC to keep the agent cr and secret from the config, event if looking for multiple kinds or that
@@ -248,8 +243,6 @@ fn k8s_garbage_collector_with_missing_and_extra_kinds() {
         namespace: test_ns.clone(),
         namespace_agents: test_ns.clone(),
         cr_type_meta: vec![missing_kind, foo_type_meta()],
-        ac_release_name: TEST_AC_RELEASE_NAME.to_string(),
-        cd_release_name: TEST_CD_RELEASE_NAME.to_string(),
     };
 
     let agents_config = serde_yaml::from_str::<AgentControlDynamicConfig>("agents: {}")
@@ -292,8 +285,6 @@ fn k8s_garbage_collector_does_not_remove_agent_control() {
         namespace: test_ns.clone(),
         namespace_agents: test_ns.clone(),
         cr_type_meta: default_group_version_kinds(),
-        ac_release_name: TEST_AC_RELEASE_NAME.to_string(),
-        cd_release_name: TEST_CD_RELEASE_NAME.to_string(),
     };
 
     // Expects the GC do not clean any resource related to the SA.
@@ -374,8 +365,6 @@ agents:
         namespace: test_ns.clone(),
         namespace_agents: test_ns.clone(),
         cr_type_meta: vec![foo_type_meta()],
-        ac_release_name: TEST_AC_RELEASE_NAME.to_string(),
-        cd_release_name: TEST_CD_RELEASE_NAME.to_string(),
     };
 
     // Expects the GC do not clean any resource related to the SA, running SubAgents or unmanaged resources.
