diff --git a/agent-control/agent-type-registry/newrelic/com.newrelic.infrastructure-0.1.0.yaml b/agent-control/agent-type-registry/newrelic/com.newrelic.infrastructure-0.1.0.yaml index d2c605016c..9473cde9e8 100644 --- a/agent-control/agent-type-registry/newrelic/com.newrelic.infrastructure-0.1.0.yaml +++ b/agent-control/agent-type-registry/newrelic/com.newrelic.infrastructure-0.1.0.yaml @@ -5,22 +5,19 @@ variables: on_host: config_agent: description: "Newrelic infra configuration" - type: file + type: yaml required: false default: "" - file_path: "newrelic-infra.yml" config_integrations: description: "map of YAML configs for the OHIs" - type: map[string]file + type: map[string]yaml required: false - default: { } - file_path: "integrations.d" + default: {} config_logging: description: "map of YAML config for logging" - type: map[string]file + type: map[string]yaml required: false - default: { } - file_path: "logging.d" + default: {} backoff_delay: description: "seconds until next retry if agent fails to start" type: string @@ -48,28 +45,28 @@ variables: description: "newrelic-infrastructure chart values" type: yaml required: false - default: { } + default: {} nri-metadata-injection: description: "nri-metadata-injection chart values" type: yaml required: false - default: { } + default: {} kube-state-metrics: description: "kube-state-metrics chart values" type: yaml required: false - default: { } + default: {} nri-kube-events: description: "nri-kube-events chart values" type: yaml required: false - default: { } + default: {} # Global values must have their own key, even if there is only one sub-chart. global: description: "Global chart values" type: yaml required: false - default: { } + default: {} # HelmChart based sub agents should always have `chart_version`. chart_version: description: "nri-bundle chart version" @@ -101,7 +98,7 @@ variables: type: yaml required: false default: null - + deployment: on_host: enable_file_logging: ${nr-var:enable_file_logging} @@ -116,14 +113,20 @@ deployment: path: /usr/bin/newrelic-infra args: --version regex: \d+\.\d+\.\d+ + filesystem: + config: + newrelic-infra.yaml: | + ${nr-var:config_agent} + integrations.d: ${nr-var:config_integrations} + logging.d: ${nr-var:config_logging} executables: - id: newrelic-infra path: /usr/bin/newrelic-infra args: >- - --config=${nr-var:config_agent} + --config=${nr-sub:autogenerated_agent_dir}/config/newrelic-infra.yaml env: - NRIA_PLUGIN_DIR: "${nr-var:config_integrations}" - NRIA_LOGGING_CONFIGS_DIR: "${nr-var:config_logging}" + NRIA_PLUGIN_DIR: "${nr-sub:autogenerated_agent_dir}/integrations.d" + NRIA_LOGGING_CONFIGS_DIR: "${nr-sub:autogenerated_agent_dir}/logging.d" NRIA_STATUS_SERVER_ENABLED: true NRIA_STATUS_SERVER_PORT: "${nr-var:health_port}" NR_HOST_ID: "${nr-ac:host_id}" @@ -216,7 +219,7 @@ deployment: newrelic-prometheus-agent: enabled: false newrelic-k8s-metrics-adapter: - enabled: false + enabled: false # Flux [HelmRelease](https://fluxcd.io/flux/components/helmreleases/) that installs the nri-bundle chart. release: apiVersion: helm.toolkit.fluxcd.io/v2 @@ -240,7 +243,7 @@ deployment: chart: ${nr-var:chart_name} # This is where the chart version to be installed is defined. Whenever AC updates this Flux triggers a reconciliation. version: ${nr-var:chart_version} - # This value gets rendered even if not specified so, even 'ChartVersion' is the default, it is required to specify + # This value gets rendered even if not specified so, even 'ChartVersion' is the default, it is required to specify # it to avoid the drift detection on AC supervisor, and trigger re-apply on each interval. reconcileStrategy: ChartVersion sourceRef: diff --git a/agent-control/agent-type-registry/newrelic/io.opentelemetry.collector-0.1.0.yaml b/agent-control/agent-type-registry/newrelic/io.opentelemetry.collector-0.1.0.yaml index a773584ed9..d763dd13bc 100644 --- a/agent-control/agent-type-registry/newrelic/io.opentelemetry.collector-0.1.0.yaml +++ b/agent-control/agent-type-registry/newrelic/io.opentelemetry.collector-0.1.0.yaml @@ -5,10 +5,9 @@ variables: on_host: config: description: "Newrelic otel collector configuration" - type: file + type: yaml required: false - default: "" - file_path: "config.yaml" + default: {} backoff_delay: description: "seconds until next retry if agent fails to start" type: string @@ -34,12 +33,12 @@ variables: description: "Newrelic otel collector chart values" type: yaml required: false - default: { } + default: {} global: description: "Global values for the chart" type: yaml required: false - default: { } + default: {} chart_version: description: "Newrelic otel collector chart version" type: string @@ -81,12 +80,16 @@ deployment: path: /usr/bin/nrdot-collector-host args: -v regex: \d+\.\d+\.\d+ + filesystem: + otel-config: + config.yaml: | + ${nr-var:config} executables: - # Important to note the binary name is nrdot-collector-host matching the new nrdot binary id: nrdot-collector-host path: /usr/bin/nrdot-collector-host args: >- - --config=${nr-var:config} + --config=${nr-sub:autogenerated_agent_dir}/otel-config/config.yaml --feature-gates=-pkg.translator.prometheus.NormalizeName env: # sets the otel-collector "env" source resource detector diff --git a/agent-control/src/agent_type/runtime_config/on_host/filesystem.rs b/agent-control/src/agent_type/runtime_config/on_host/filesystem.rs index 4d5e693212..e3e4265349 100644 --- a/agent-control/src/agent_type/runtime_config/on_host/filesystem.rs +++ b/agent-control/src/agent_type/runtime_config/on_host/filesystem.rs @@ -1,10 +1,18 @@ +//! Module defining the file system configuration for sub-agents. +//! +//! This includes files and directories that should be created for the sub-agent at runtime, +//! based on templated content and paths. The paths are always relative to the sub-agent's +//! dedicated directory created by agent-control +//! (usually something like `/var/lib/newrelic-agent-control/auto-generated/`). +//! The files are created in a dedicated `files/` subdirectory, while directories are created in +//! a dedicated `directories/` subdirectory, to avoid name clashes. + use std::{ - collections::{HashMap, HashSet}, + collections::HashMap, + io::{Error as IOError, ErrorKind}, path::{Component, Path, PathBuf}, }; -use serde::{Deserialize, Deserializer}; - use crate::agent_type::{ agent_attributes::AgentAttributes, definition::Variables, @@ -14,91 +22,207 @@ use crate::agent_type::{ trivial_value::TrivialValue, variable::{Variable, namespace::Namespace}, }; +use serde::Deserialize; + +pub mod rendered; -/// Represents the file system configuration for the deployment of an agent. +/// Represents the file system configuration for the deployment of a host agent. Consisting of +/// a set of directories (map keys) which in turn contain a set of files (nested map keys) with +/// their respective content (map values). /// -/// It is a key-value structure in which every key is an identifier and the value is a file entry. -/// See [FileEntry] for details. -#[derive(Debug, Default, Clone, PartialEq)] -pub struct FileSystem(HashMap); +/// It would be equivalent to a YAML mapping of this format: +/// ```yaml +/// filesystem: +/// "path/to/my-dir": +/// # YAML content, expected to be a mapping string -> yaml +/// filepath1: "file1 content" +/// filepath2: | # multi-line string content +/// key: value +/// # fully templated content, expected to render to a valid YAML mapping string -> string +/// "another/path/to/my-dir": ${nr-var:some_var_that_renders_to_a_yaml_mapping} +/// ``` +/// +/// The files can be hardcoded, with the contents possibly containing templates, or the whole set of +/// files can be templated so a directory contains an arbitrary number of files (a place to use a +/// `map[string]yaml` variable type). **The paths cannot be templated.** +/// +/// See [`AgentDirectoryEntry`] and [`DirEntriesType`] for more details. +#[derive(Debug, Default, Deserialize, Clone, PartialEq)] +pub struct FileSystem(HashMap); impl FileSystem { - /// Returns the internal file entries as a [`HashMap`]. + /// Returns the internal file entries as a [`HashMap`] so they can + /// be written into the actual host filesystem. /// - /// **WARNING**: This must be called **after** the rendering process has finished or else AC will crash! - pub fn rendered(self) -> HashMap { + /// **WARNING**: This must be called **after** the rendering process has finished + /// or else AC might crash! + fn rendered(self) -> HashMap { self.0 - .into_values() - .map(|v| (v.relative_path, v.content.get())) + .into_iter() + .flat_map(|(dir_path, dir_entries)| dir_entries.rendered_with(&dir_path)) .collect() } } -impl<'de> Deserialize<'de> for FileSystem { - fn deserialize(deserializer: D) -> Result - where - D: Deserializer<'de>, - { - let entries = HashMap::::deserialize(deserializer)?; - if let Err(e) = validate_unique_paths(entries.values().map(|e| &e.relative_path)) { - return Err(serde::de::Error::custom(format!( - "duplicate file paths are not allowed. Found duplicate path: '{}'", - e.display() - ))); - } - Ok(Self(entries)) +/// A path to a file or directory that has been validated to be "safe", +/// i.e. relative and not escaping its base directory (e.g. with parent dir specifiers like `..`). +#[derive(Debug, Default, Deserialize, Clone, PartialEq, Eq, Hash)] +#[serde(try_from = "PathBuf")] +struct SafePath(PathBuf); + +/// Allow borrowing the inner [`Path`] from a [`SafePath`]. +impl AsRef for SafePath { + fn as_ref(&self) -> &Path { + &self.0 } } -/// A file entry consists on a path and its content. The path must always be relative, -/// as these represent files that will be created for a sub-agent's scope (i.e. in AC's -/// auto-generated directory for that sub-agent). -#[derive(Debug, Default, Clone, PartialEq)] -struct FileEntry { - relative_path: PathBuf, - content: TemplateableValue, +/// Try to create a [`SafePath`] from a [`PathBuf`], validating that the path is relative +/// and does not escape its base directory. If the path is invalid, an error string is returned +/// containing a comma-separated list of the issues found. +impl TryFrom for SafePath { + type Error = IOError; + + fn try_from(value: PathBuf) -> Result { + validate_file_entry_path(&value) + .map_err(|e| IOError::new(ErrorKind::InvalidFilename, e))?; + Ok(SafePath(value)) + } } -impl<'de> Deserialize<'de> for FileEntry { - fn deserialize(deserializer: D) -> Result - where - D: Deserializer<'de>, - { - use serde::de::Error; - - #[derive(Deserialize)] - struct PreValidationFileEntry { - path: PathBuf, - content: TemplateableValue, +impl From for PathBuf { + fn from(value: SafePath) -> Self { + value.0 + } +} + +/// The type of items present in a directory entry. +/// +/// There are two supported modes: +/// 1. A fixed set of entries, where each entry's content can be templated. +/// This implies the number and names of the entries are known at parse time. +/// 2. A fully templated set of entries, where it's expected that a full template is provided as +/// a placeholder for later rendering. +#[derive(Debug, Deserialize, PartialEq, Clone)] +#[serde(untagged)] +enum DirEntriesType { + /// A directory with a fixed set of entries (i.e. files). Each entry's content can be templated. + /// E.g. + /// ```yaml + /// "my/dir": + /// filepath1: "file1 content with ${nr-var:some_var}" + /// filepath2: "file2 content" + /// ``` + FixedWithTemplatedContent(HashMap>), + + /// A directory with a fully templated set of entries, where it's expected that a full template + /// is provided that renders to a valid YAML mapping of a safe [`PathBuf`] to [`String`]. + /// E.g. + /// ```yaml + /// "my/templated/dir": + /// ${nr-var:some_var_that_renders_to_a_yaml_mapping} + /// ``` + FullyTemplated(TemplateableValue), +} + +impl Default for DirEntriesType { + fn default() -> Self { + DirEntriesType::FixedWithTemplatedContent(HashMap::default()) + } +} + +impl DirEntriesType { + /// Renders the directory entries as an iterator of [`HashMap`] so they can + /// be written into the actual host filesystem. + /// + /// **WARNING**: This must be called **after** the rendering process has finished + /// or else AC might crash! + fn rendered_with(self, path: impl AsRef) -> HashMap { + match self { + DirEntriesType::FixedWithTemplatedContent(map) => map + .into_iter() + .map(|(k, v)| (path.as_ref().join(k), v.get())) + .collect(), + DirEntriesType::FullyTemplated(tv) => { + let map = HashMap::from(tv.get()); + map.into_iter() + .map(|(k, v)| (path.as_ref().join(k), v)) + .collect() + } } + } +} - let entry = PreValidationFileEntry::deserialize(deserializer)?; - // Perform validations on the provided Paths - if let Err(errs) = validate_file_entry_path(&entry.path) { - Err(Error::custom(errs.join(", "))) +/// A helper newtype to allow implementing `Templateable` for `TemplateableValue>` +/// without running into orphan rule issues. +#[derive(Debug, Default, PartialEq, Clone)] +struct DirEntriesMap(HashMap); + +impl From for HashMap { + fn from(value: DirEntriesMap) -> Self { + value.0 + } +} + +impl Templateable for FileSystem { + fn template_with(self, variables: &Variables) -> Result { + if let Some(TrivialValue::String(generated_dir)) = variables + .get(&Namespace::SubAgent.namespaced_name(AgentAttributes::GENERATED_DIR)) + .and_then(Variable::get_final_value) + { + let filesystem = self + .0 + .into_iter() + .map(|(k, v)| { + Ok(( + // The only place where we construct a `SafePath` directly, prepending the + // sub-agent's auto-generated directory to the user-provided relative path. + // FIXME: when we fix the templating and make the agent type definitions + // type-safe, we will make sure to always build correct "final paths" here. + SafePath(PathBuf::from(generated_dir.clone()).join(k)), + v.template_with(variables)?, + )) + }) + .collect::, AgentTypeError>>()?; + Ok(Self(filesystem)) } else { - Ok(Self { - relative_path: entry.path, - content: entry.content, - }) + Err(AgentTypeError::MissingValue( + Namespace::SubAgent.namespaced_name(AgentAttributes::GENERATED_DIR), + )) } } } -impl Templateable for FileSystem { +impl Templateable for DirEntriesType { + /// Replaces placeholders in the content with values from the `Variables` map. + /// + /// Behaves differently depending on the variant: + /// - For `FixedWithTemplatedContent`, it templates each entry's content individually. + /// - For `FullyTemplated`, it templates the entire content as a single unit, expecting it to + /// be a valid (YAML) mapping of safe `PathBuf` to `String`. + /// + /// See [`TemplateableValue::template_with`] for details. fn template_with(self, variables: &Variables) -> Result { - self.0 - .into_iter() - .map(|(k, v)| Ok((k, v.template_with(variables)?))) - .collect::, _>>() - .map(FileSystem) + match self { + DirEntriesType::FixedWithTemplatedContent(map) => { + let rendered_map = map + .into_iter() + .map(|(k, v)| Ok((k, v.template_with(variables)?))) + .collect::, AgentTypeError>>()?; + Ok(DirEntriesType::FixedWithTemplatedContent(rendered_map)) + } + DirEntriesType::FullyTemplated(tv) => { + Ok(DirEntriesType::FullyTemplated(tv.template_with(variables)?)) + } + } } } -impl Templateable for FileEntry { - /// Performs the templating of the defined file entries for this sub-agent. +impl Templateable for TemplateableValue { + /// Performs the templating of the defined directory entries for this sub-agent in the case where + /// they were fully templated (see [`DirEntriesType::FullyTemplated`]). /// - /// The paths present in the FileEntry structures are always assumed to start from the + /// The paths present in the DirectoryEntry structures are always assumed to start from the /// sub-agent's dedicated directory. /// /// Besides, we know the paths are relative and don't go above their base dir (e.g. `/../..`) @@ -106,39 +230,53 @@ impl Templateable for FileEntry { /// provided base dir to them, as it must be defined in the variables passed to the sub-agent. /// If the value of the sub-agent's dedicated directory is missing, the templating fails. fn template_with(self, variables: &Variables) -> Result { - if let Some(TrivialValue::String(generated_dir)) = variables - .get(&Namespace::SubAgent.namespaced_name(AgentAttributes::GENERATED_DIR)) - .and_then(Variable::get_final_value) - { - let rendered_file_entry = Self { - relative_path: PathBuf::from(generated_dir).join(self.relative_path), - content: self.content.template_with(variables)?, - }; - Ok(rendered_file_entry) + // Template content as a string first. Then parse as a YAML and attempt to convert to the + // expected HashMap type. + let templated_string = self.template.clone().template_with(variables)?; + let value: HashMap = if templated_string.is_empty() { + HashMap::new() } else { - Err(AgentTypeError::MissingValue( - Namespace::SubAgent.namespaced_name(AgentAttributes::GENERATED_DIR), - )) - } + let map_string_value: HashMap = + serde_yaml::from_str(&templated_string).map_err(|e| { + AgentTypeError::ValueNotParseableFromString(format!( + "Could not parse templated directory items as YAML: {e}" + )) + })?; + // Convert the serde_yaml::Value (i.e. the file contents) to String + + map_string_value + .into_iter() + .map(|(k, v)| Ok((k, output_string(v)?))) + .collect::, serde_yaml::Error>>()? + }; + + Ok(Self { + template: self.template, + value: Some(DirEntriesMap(value)), + }) } } -fn validate_unique_paths<'a>( - mut paths: impl Iterator, -) -> Result<(), &'a PathBuf> { - let mut seen_paths = HashSet::new(); - // Inserting already-inserted items in the hashset evaluates to `false`. - paths.try_for_each(|p| if seen_paths.insert(p) { Ok(()) } else { Err(p) }) +/// Converts a serde_yaml::Value to a String. +/// If the value is already a String, it is returned as-is. +/// Otherwise, it is serialized to a YAML string using serde_yaml. +fn output_string(value: serde_yaml::Value) -> Result { + match value { + // Pass the string directly (serde_yaml inserts literal syntax for multi-line strings) + serde_yaml::Value::String(s) => Ok(s), + // Else serialize the value to a YAML string using the default methods + v => serde_yaml::to_string(&v), + } } -fn validate_file_entry_path(path: &Path) -> Result<(), Vec> { +/// Validates that a file entry path is relative and does not escape its base directory. +/// Returns a comma-separated list of error messages, if any. +fn validate_file_entry_path(path: &Path) -> Result<(), String> { let mut errors = Vec::new(); if !path.is_relative() { let p = path.display(); - errors.push(format!( - "Only relative paths are allowed. Found absolute: {p}" - )); + errors.push(format!("path `{p}` is not relative")); } // Paths must not escape the base directory if let Err(e) = check_basedir_escape_safety(path) { @@ -148,20 +286,23 @@ fn validate_file_entry_path(path: &Path) -> Result<(), Vec> { if errors.is_empty() { Ok(()) } else { - Err(errors) + Err(errors.join(", ")) } } /// Makes sure the passed directory goes not traverse outside the directory where it's contained. -/// E.g. via relative path specificers like `./../../some_path`. +/// E.g. via relative path specifiers like `./../../some_path`. /// +/// This would make files and directories "safe" to be created inside a sub-agent's dedicated +/// directory, as they would not be able to write outside of it +/// (tampering with other sub-agents or worse). /// Returns an error string if this property does not hold. fn check_basedir_escape_safety(path: &Path) -> Result<(), String> { path.components().try_for_each(|comp| match comp { Component::Normal(_) | Component::CurDir => Ok(()), // Disallow other non-supported variants like roots or prefixes Component::ParentDir | Component::RootDir | Component::Prefix(_) => Err(format!( - "path '{}' has an invalid component: '{}'", + "path `{}` has an invalid component: `{}`", path.display(), comp.as_os_str().to_string_lossy() )), @@ -171,6 +312,7 @@ fn check_basedir_escape_safety(path: &Path) -> Result<(), String> { #[cfg(test)] mod tests { use rstest::rstest; + use serde_yaml::Value; use super::*; @@ -198,29 +340,46 @@ mod tests { Variable::new_final_string_variable("/base/dir"), )]); - let file_entry = FileEntry { - relative_path: PathBuf::from("my/file/path"), - content: TemplateableValue::new("some content".to_string()), - }; + let filesystem_entry = FileSystem(HashMap::from([( + PathBuf::from("my/path").try_into().unwrap(), + DirEntriesType::FixedWithTemplatedContent(HashMap::from([( + PathBuf::from("my/file/path").try_into().unwrap(), + TemplateableValue::from_template("some content".to_string()), + )])), + )])); - let rendered = file_entry.template_with(&variables); + let rendered = filesystem_entry.template_with(&variables); assert!(rendered.is_ok()); - assert_eq!( - rendered.unwrap().relative_path, - PathBuf::from("/base/dir/my/file/path") - ); + let rendered = rendered.unwrap(); + assert!(rendered.0.len() == 1); + + let expected_filesystem = FileSystem(HashMap::from([( + SafePath(PathBuf::from("/base/dir/my/path")), + DirEntriesType::FixedWithTemplatedContent(HashMap::from([( + PathBuf::from("my/file/path").try_into().unwrap(), + TemplateableValue::new("some content".to_string()) + .with_template("some content".to_string()), + )])), + )])); + + assert_eq!(rendered, expected_filesystem); } #[test] fn invalid_filepath_rendering_nonexisting_subagent_basepath() { + // If the sub-agent variable (nr-sub) containing the agent's auto-generated dir is missing, + // templating must fail. let variables = Variables::default(); - let file_entry = FileEntry { - relative_path: PathBuf::from("my/file/path"), - content: TemplateableValue::new("some content".to_string()), - }; + let filesystem_entry = FileSystem(HashMap::from([( + PathBuf::from("my/path").try_into().unwrap(), + DirEntriesType::FixedWithTemplatedContent(HashMap::from([( + PathBuf::from("my/file/path").try_into().unwrap(), + TemplateableValue::new("some content".to_string()), + )])), + )])); - let rendered = file_entry.template_with(&variables); + let rendered = filesystem_entry.template_with(&variables); assert!(rendered.is_err()); let rendered_err = rendered.unwrap_err(); assert!(matches!(rendered_err, AgentTypeError::MissingValue(_))); @@ -236,19 +395,199 @@ mod tests { #[rstest] #[case::valid_filesystem_parse("basic/path", |r: Result<_, _>| r.is_ok())] #[case::windows_style_path(r"some\\windows\\style\\path", |r: Result<_, _>| r.is_ok())] - #[case::invalid_absolute_path("/absolute/path", |r: Result<_, serde_yaml::Error>| r.is_err_and(|e| e.to_string().contains("absolute: /absolute/path")))] - #[case::invalid_reaches_parentdir("basedir/dir/../dir/../../../outdir/path", |r: Result<_, serde_yaml::Error>| r.is_err_and(|e| e.to_string().contains("invalid component: '..'")))] + #[case::invalid_absolute_path("/absolute/path", |r: Result<_, serde_yaml::Error>| r.is_err())] + #[case::invalid_reaches_parentdir("basedir/dir/../dir/../../../outdir/path", |r: Result<_, serde_yaml::Error>| r.is_err())] // #[case::invalid_windows_path_prefix(r"C:\\absolute\\windows\\path", |r: Result<_, serde_yaml::Error>| r.is_err_and(|e| e.to_string().contains("invalid path component")))] // #[case::invalid_windows_root_device("C:", |r: Result<_, serde_yaml::Error>| r.is_err_and(|e| e.to_string().contains("invalid path component")))] // #[case::invalid_windows_server_path(r"\\\\server\\share", |r: Result<_, serde_yaml::Error>| r.is_err_and(|e| e.to_string().contains("invalid path component")))] // TODO add windows paths to check that this handles the `Component::Prefix(_)` case correctly fn file_entry_parsing( #[case] path: &str, - #[case] validation: impl Fn(Result) -> bool, + #[case] validation: impl Fn(Result) -> bool, ) { - let yaml = format!("path: \"{}\"\ncontent: \"some random content\"", path); - let parsed = serde_yaml::from_str::(&yaml); + let yaml = format!("\"{path}\": \"some random content\""); + let parsed = serde_yaml::from_str::(&yaml); let parsed_display = format!("{parsed:?}"); - assert!(validation(parsed), "{}", parsed_display); + assert!(validation(parsed), "input: {yaml}, parsed:{parsed_display}"); + } + + const EXAMPLE_FILESYSTEM: &str = r#" +"path/to/my-dir": + filepath1: "file1 content" + filepath2: | + key: ${nr-var:some_var} +"another/path/to/my-dir": + ${nr-var:some_var_that_renders_to_a_yaml_mapping} +"#; + + #[test] + fn parse_valid_directories() { + let parsed: Result = serde_yaml::from_str(EXAMPLE_FILESYSTEM); + assert!( + parsed.as_ref().is_ok_and(|p| p.0.len() == 2), + "Parsed directories: {parsed:?}" + ); + + let parsed = parsed.unwrap().0; + let my_dir = parsed + .get(&SafePath(PathBuf::from("path/to/my-dir"))) + .unwrap(); + assert!(matches!( + my_dir, + DirEntriesType::FixedWithTemplatedContent(_) + )); + + let another_dir = parsed + .get(&SafePath(PathBuf::from("another/path/to/my-dir"))) + .unwrap(); + assert!(matches!(another_dir, DirEntriesType::FullyTemplated(_))); + } + + const FILESYSTEM_EXAMPLE: &str = r#" +"some/files": + "path/to/my-file": "something ${nr-var:some_file_var}" + "another/path/to/my-file": | + some + multi-line + content +"path/to/my-dir": + filepath1: "file1 content" + filepath2: | + key: ${nr-var:some_dir_var} +"another/path/to/my-dir": + ${nr-var:some_var_that_renders_to_a_yaml_mapping} +"#; + + #[test] + fn parse_and_template_filesystem() { + let parsed = serde_yaml::from_str::(FILESYSTEM_EXAMPLE); + assert!( + parsed.as_ref().is_ok_and(|fs| fs.0.len() == 3), + "Parsed filesystem: {parsed:?}" + ); + + let parsed = parsed.unwrap(); + let variables = Variables::from_iter(vec![ + ( + Namespace::SubAgent.namespaced_name(AgentAttributes::GENERATED_DIR), + Variable::new_final_string_variable("/test/base/dir"), + ), + ( + Namespace::Variable.namespaced_name("some_file_var"), + Variable::new_final_string_variable("file_var_value"), + ), + ( + Namespace::Variable.namespaced_name("some_dir_var"), + Variable::new_final_string_variable("dir_var_value"), + ), + ( + Namespace::Variable.namespaced_name("some_var_that_renders_to_a_yaml_mapping"), + // a map[string]yaml + Variable::new( + String::default(), + false, + None, + Some(HashMap::from([ + ("fileA".to_string(), Value::String("contentA".to_string())), + ( + "fileB".to_string(), + Value::String("multi-line\ncontentB".to_string()), + ), + ])), + ), + ), + ]); + + let templated = parsed.template_with(&variables); + assert!(templated.is_ok(), "Templated filesystem: {templated:?}"); + } + + #[test] + fn rendered_files() { + let parsed = serde_yaml::from_str::(FILESYSTEM_EXAMPLE); + assert!( + parsed.as_ref().is_ok_and(|fs| fs.0.len() == 3), + "Parsed filesystem: {parsed:?}" + ); + + let parsed = parsed.unwrap(); + let variables = Variables::from_iter(vec![ + ( + Namespace::SubAgent.namespaced_name(AgentAttributes::GENERATED_DIR), + Variable::new_final_string_variable("/test/base/dir"), + ), + ( + Namespace::Variable.namespaced_name("some_file_var"), + Variable::new_final_string_variable("file_var_value"), + ), + ( + Namespace::Variable.namespaced_name("some_dir_var"), + Variable::new_final_string_variable("dir_var_value"), + ), + ( + Namespace::Variable.namespaced_name("some_var_that_renders_to_a_yaml_mapping"), + // a map[string]yaml + Variable::new( + String::default(), + false, + None, + Some(HashMap::from([ + ("fileA".to_string(), Value::String("contentA".to_string())), + ( + "fileB".to_string(), + Value::String("multi-line\ncontentB".to_string()), + ), + ])), + ), + ), + ]); + + let templated = parsed.template_with(&variables); + assert!(templated.is_ok(), "Templated filesystem: {templated:?}"); + let templated = templated.unwrap(); + + // Expected rendered paths with contents. + // All paths must be prepended by the sub-agent's generated dir and the + // corresponding `files/` or `directories/` subdir, depending on where they came from. + // They also must have all variables rendered and have the correct content. + let expected_rendered = [ + ( + PathBuf::from("/test/base/dir/another/path/to/my-dir/fileA"), + String::from("contentA"), + ), + ( + PathBuf::from("/test/base/dir/path/to/my-dir/filepath1"), + String::from("file1 content"), + ), + ( + PathBuf::from("/test/base/dir/path/to/my-dir/filepath2"), + String::from("key: dir_var_value\n"), + ), + ( + PathBuf::from("/test/base/dir/some/files/path/to/my-file"), + String::from("something file_var_value"), + ), + ( + PathBuf::from("/test/base/dir/some/files/another/path/to/my-file"), + String::from("some\nmulti-line\ncontent\n"), + ), + ( + PathBuf::from("/test/base/dir/another/path/to/my-dir/fileB"), + String::from("multi-line\ncontentB"), + ), + ]; + let rendered = templated.rendered(); + assert_eq!( + rendered.len(), + expected_rendered.len(), + "Rendered filesystem not same size as expected: {rendered:?}, expected: {expected_rendered:?}" + ); + + assert!( + rendered.iter().all(|(r_p, r_s)| expected_rendered + .iter() + .any(|(e_p, e_s)| e_p == r_p && e_s == r_s)), + "Rendered filesystem not matching expected: {rendered:?}, expected: {expected_rendered:?}" + ); } } diff --git a/agent-control/src/sub_agent/on_host/command/filesystem_entries.rs b/agent-control/src/agent_type/runtime_config/on_host/filesystem/rendered.rs similarity index 94% rename from agent-control/src/sub_agent/on_host/command/filesystem_entries.rs rename to agent-control/src/agent_type/runtime_config/on_host/filesystem/rendered.rs index 88f802aa5f..bb869e6e3c 100644 --- a/agent-control/src/sub_agent/on_host/command/filesystem_entries.rs +++ b/agent-control/src/agent_type/runtime_config/on_host/filesystem/rendered.rs @@ -5,6 +5,7 @@ use ::fs::{ writer_file::{FileWriter, WriteError}, }; use thiserror::Error; +use tracing::trace; use crate::agent_type::runtime_config::on_host::filesystem::FileSystem; @@ -32,6 +33,7 @@ impl RenderedFileSystemEntries { dir_manager: &impl DirectoryManager, ) -> Result<(), FileSystemEntriesError> { self.0.iter().try_for_each(|(path, content)| { + trace!("Writing filesystem entry to {}", path.display()); let parent_dir = path .parent() .ok_or_else(|| { diff --git a/agent-control/src/agent_type/trivial_value.rs b/agent-control/src/agent_type/trivial_value.rs index de19ae0280..3d2f593bf0 100644 --- a/agent-control/src/agent_type/trivial_value.rs +++ b/agent-control/src/agent_type/trivial_value.rs @@ -11,15 +11,17 @@ use serde::{Deserialize, Serialize}; #[serde(untagged)] pub enum TrivialValue { String(String), + Bool(bool), + Number(serde_yaml::Number), #[serde(skip)] File(FilePathWithContent), #[serde(skip)] Yaml(serde_yaml::Value), - Bool(bool), - Number(serde_yaml::Number), #[serde(skip)] MapStringString(Map), #[serde(skip)] + MapStringYaml(Map), + #[serde(skip)] MapStringFile(Map), } @@ -63,10 +65,17 @@ impl Display for TrivialValue { TrivialValue::MapStringString(n) => { let flatten: Vec = n .iter() - .map(|(key, value)| format!("{key}={value}")) + // FIXME is this what we really want? key=value? + .map(|(key, value)| format!("{key}={value}")) .collect(); write!(f, "{}", flatten.join(" ")) } + TrivialValue::MapStringYaml(n) => write!( + f, + "{}", + serde_yaml::to_string(n) + .expect("A value of type HashMap should always be serializable") + ), TrivialValue::MapStringFile(n) => { let flatten: Vec = n .iter() diff --git a/agent-control/src/agent_type/variable.rs b/agent-control/src/agent_type/variable.rs index 6f52c878e8..6f9b06f85c 100644 --- a/agent-control/src/agent_type/variable.rs +++ b/agent-control/src/agent_type/variable.rs @@ -130,6 +130,15 @@ mod tests { } } + impl From>> for Variable { + fn from(kind_value: Fields>) -> Self { + Self { + description: String::new(), + variable_type: VariableType::MapStringYaml(kind_value), + } + } + } + impl Variable { pub(crate) fn new( description: String, diff --git a/agent-control/src/agent_type/variable/variable_type.rs b/agent-control/src/agent_type/variable/variable_type.rs index f2ad0b64f1..4fb9d820e9 100644 --- a/agent-control/src/agent_type/variable/variable_type.rs +++ b/agent-control/src/agent_type/variable/variable_type.rs @@ -32,6 +32,8 @@ pub enum VariableTypeDefinition { MapStringString(FieldsDefinition>), #[serde(rename = "map[string]file")] MapStringFile(FieldsWithPathDefinition>), + #[serde(rename = "map[string]yaml")] + MapStringYaml(FieldsDefinition>), #[serde(rename = "yaml")] Yaml(YamlFieldsDefinition), } @@ -45,6 +47,7 @@ pub enum VariableType { File(FieldsWithPath), MapStringString(Fields>), MapStringFile(FieldsWithPath>), + MapStringYaml(Fields>), Yaml(Fields), } @@ -62,53 +65,14 @@ impl VariableTypeDefinition { VariableTypeDefinition::MapStringFile(v) => { VariableType::MapStringFile(v.with_config(constraints)) } + VariableTypeDefinition::MapStringYaml(v) => { + VariableType::MapStringYaml(v.with_config(constraints)) + } VariableTypeDefinition::Yaml(v) => VariableType::Yaml(v.with_config(constraints)), } } } -impl From for VariableType { - fn from(fields: StringFields) -> Self { - VariableType::String(fields) - } -} - -impl From> for VariableType { - fn from(fields: Fields) -> Self { - VariableType::Bool(fields) - } -} - -impl From> for VariableType { - fn from(fields: Fields) -> Self { - VariableType::Number(fields) - } -} - -impl From> for VariableType { - fn from(fields: FieldsWithPath) -> Self { - VariableType::File(fields) - } -} - -impl From>> for VariableType { - fn from(fields: Fields>) -> Self { - VariableType::MapStringString(fields) - } -} - -impl From>> for VariableType { - fn from(fields: FieldsWithPath>) -> Self { - VariableType::MapStringFile(fields) - } -} - -impl From> for VariableType { - fn from(fields: Fields) -> Self { - VariableType::Yaml(fields) - } -} - /// The below methods are mostly concerned with delegating to the inner type on each `Kind` variant. /// It's a lot of boilerplate, but declarative and straight-forward. impl VariableType { @@ -120,6 +84,7 @@ impl VariableType { VariableType::File(f) => f.inner.required, VariableType::MapStringString(f) => f.required, VariableType::MapStringFile(f) => f.inner.required, + VariableType::MapStringYaml(f) => f.required, VariableType::Yaml(f) => f.required, } } @@ -146,6 +111,7 @@ impl VariableType { .for_each(|fp| fp.with_path(f.file_path.clone())); f.inner.set_final_value(files) } + VariableType::MapStringYaml(f) => f.set_final_value(serde_yaml::from_value(value)?), VariableType::Yaml(f) => f.set_final_value(value), }?; Ok(()) @@ -194,6 +160,12 @@ impl VariableType { .or(f.inner.default.as_ref()) .cloned() .map(TrivialValue::MapStringFile), + VariableType::MapStringYaml(f) => f + .final_value + .as_ref() + .or(f.default.as_ref()) + .cloned() + .map(TrivialValue::MapStringYaml), VariableType::Yaml(f) => f .final_value .as_ref() @@ -219,3 +191,50 @@ impl VariableType { } } } + +#[cfg(test)] +mod tests { + use super::*; + + impl From for VariableType { + fn from(fields: StringFields) -> Self { + VariableType::String(fields) + } + } + + impl From> for VariableType { + fn from(fields: Fields) -> Self { + VariableType::Bool(fields) + } + } + + impl From> for VariableType { + fn from(fields: Fields) -> Self { + VariableType::Number(fields) + } + } + + impl From> for VariableType { + fn from(fields: FieldsWithPath) -> Self { + VariableType::File(fields) + } + } + + impl From>> for VariableType { + fn from(fields: FieldsWithPath>) -> Self { + VariableType::MapStringFile(fields) + } + } + + impl From>> for VariableType { + fn from(fields: Fields>) -> Self { + VariableType::MapStringYaml(fields) + } + } + + impl From> for VariableType { + fn from(fields: Fields) -> Self { + VariableType::Yaml(fields) + } + } +} diff --git a/agent-control/src/sub_agent/on_host/builder.rs b/agent-control/src/sub_agent/on_host/builder.rs index e3893f4cea..bfb8c400f2 100644 --- a/agent-control/src/sub_agent/on_host/builder.rs +++ b/agent-control/src/sub_agent/on_host/builder.rs @@ -1,5 +1,6 @@ use crate::agent_control::defaults::{HOST_NAME_ATTRIBUTE_KEY, OPAMP_SERVICE_VERSION}; use crate::agent_control::run::Environment; +use crate::agent_type::runtime_config::on_host::filesystem::rendered::RenderedFileSystemEntries; use crate::context::Context; use crate::event::SubAgentEvent; use crate::event::broadcaster::unbounded::UnboundedBroadcast; @@ -10,7 +11,6 @@ use crate::sub_agent::SubAgent; use crate::sub_agent::effective_agents_assembler::{EffectiveAgent, EffectiveAgentsAssembler}; use crate::sub_agent::identity::AgentIdentity; use crate::sub_agent::on_host::command::executable_data::ExecutableData; -use crate::sub_agent::on_host::command::filesystem_entries::RenderedFileSystemEntries; use crate::sub_agent::on_host::supervisor::NotStartedSupervisorOnHost; use crate::sub_agent::remote_config_parser::RemoteConfigParser; use crate::sub_agent::supervisor::builder::SupervisorBuilder; diff --git a/agent-control/src/sub_agent/on_host/command.rs b/agent-control/src/sub_agent/on_host/command.rs index b6518ede02..257269adb3 100644 --- a/agent-control/src/sub_agent/on_host/command.rs +++ b/agent-control/src/sub_agent/on_host/command.rs @@ -1,7 +1,6 @@ pub mod command_os; pub mod error; pub mod executable_data; -pub mod filesystem_entries; pub mod logging; pub mod restart_policy; pub mod shutdown; diff --git a/agent-control/src/sub_agent/on_host/supervisor.rs b/agent-control/src/sub_agent/on_host/supervisor.rs index 54eb05ede5..1cf66a6a73 100644 --- a/agent-control/src/sub_agent/on_host/supervisor.rs +++ b/agent-control/src/sub_agent/on_host/supervisor.rs @@ -1,5 +1,6 @@ use crate::agent_control::agent_id::AgentID; use crate::agent_type::runtime_config::health_config::OnHostHealthConfig; +use crate::agent_type::runtime_config::on_host::filesystem::rendered::RenderedFileSystemEntries; use crate::agent_type::runtime_config::version_config::OnHostVersionConfig; use crate::context::Context; use crate::event::SubAgentInternalEvent; @@ -14,7 +15,6 @@ use crate::sub_agent::identity::{AgentIdentity, ID_ATTRIBUTE_NAME}; use crate::sub_agent::on_host::command::command_os::CommandOSNotStarted; use crate::sub_agent::on_host::command::error::CommandError; use crate::sub_agent::on_host::command::executable_data::ExecutableData; -use crate::sub_agent::on_host::command::filesystem_entries::RenderedFileSystemEntries; use crate::sub_agent::on_host::command::shutdown::{ ProcessTerminator, wait_exit_timeout, wait_exit_timeout_default, }; diff --git a/agent-control/src/sub_agent/supervisor/starter.rs b/agent-control/src/sub_agent/supervisor/starter.rs index 82e45cb1ae..f5627e76bf 100644 --- a/agent-control/src/sub_agent/supervisor/starter.rs +++ b/agent-control/src/sub_agent/supervisor/starter.rs @@ -1,8 +1,8 @@ +use crate::agent_type::runtime_config::on_host::filesystem::rendered::FileSystemEntriesError; use crate::event::SubAgentInternalEvent; use crate::event::channel::EventPublisher; use crate::health::health_checker::HealthCheckerError; use crate::sub_agent::error::SubAgentBuilderError; -use crate::sub_agent::on_host::command::filesystem_entries::FileSystemEntriesError; use crate::sub_agent::supervisor::stopper::SupervisorStopper; use thiserror::Error; diff --git a/agent-control/tests/on_host/scenarios/filesystem_ops.rs b/agent-control/tests/on_host/scenarios/filesystem_ops.rs index fb2c0dd7a4..52ecda59a3 100644 --- a/agent-control/tests/on_host/scenarios/filesystem_ops.rs +++ b/agent-control/tests/on_host/scenarios/filesystem_ops.rs @@ -28,6 +28,7 @@ fn writes_filesystem_entries() { let expected_file_contents = "Hello, world!"; let agent_id = "test-agent"; + let dir_entry = "example-filepath"; let file_path = "randomdir/randomfile.txt"; create_file( @@ -40,9 +41,8 @@ variables: {{}} deployment: on_host: filesystem: - somefile: - path: {file_path} - content: "{expected_file_contents}" + {dir_entry}: + {file_path}: "{expected_file_contents}" "#, ), local_dir.join(DYNAMIC_AGENT_TYPE_FILENAME), @@ -80,6 +80,7 @@ deployment: .remote_dir .join(GENERATED_FOLDER_NAME) .join(agent_id) + .join(dir_entry) .join(file_path); retry(30, Duration::from_secs(1), || { @@ -92,7 +93,7 @@ deployment: /// created in the appropriate location under the remote directory and with their contents properly /// rendered from the defined variables. #[test] -fn complete_render_and_and_write_files() { +fn complete_render_and_and_write_files_and_dirs() { let opamp_server = FakeServer::start_new(); let tempdir = tempdir().expect("failed to create temp dir"); @@ -100,9 +101,15 @@ fn complete_render_and_and_write_files() { let remote_dir = tempdir.path().join("remote"); let agent_id = "test-agent"; + // Rendered file paths let yaml_file_path = "randomdir/randomfile.yaml"; let string_file_path = "randomdir-2/some_string.txt"; + + // Rendered directory paths + let dir_path = "somedir"; + let fully_templated_dir = "fully_templated_dir"; + // String variable and file contents let string_var_content = "Hello, world!"; let yaml_var_content = "key: value"; @@ -110,6 +117,20 @@ fn complete_render_and_and_write_files() { let expected_string_file_contents = format!("Some string contents with a rendered variable: {string_var_content}"); + // Directory files and their contents. First element is the file name, second is the expected contents + let expected_dir_file1 = ("file1.txt", "File 1 contents".to_string()); + let expected_dir_file2 = ( + "file2.txt", + format!("File 2 contents with a variable: {string_var_content}\n"), + ); + let expected_dir_file3 = ("file3.txt", "File 3 contents".to_string()); + let expected_dir_file4 = ( + "file4.txt", + format!("File 4 contents with a variable: {string_var_content}\n"), + ); + let expected_dir_file5 = ("file5.yaml", "my_key: my_value\nmy_seq:\n- item1\n- item2\nmy_string: |-\n This is a multi-line\n string in YAML\n".to_string()); + + // Create agent type definition create_file( format!( r#" @@ -126,42 +147,61 @@ variables: description: "Contents of an arbitrary string file" type: string required: true + some_mapstringyaml: + description: "A directory structure" + type: map[string]yaml + required: true deployment: on_host: filesystem: - somefile: - path: {yaml_file_path} - content: |- + randomdir: + "{yaml_file_path}": |- ${{nr-var:yaml_file_contents}} - otherfile: - path: {string_file_path} - content: "Some string contents with a rendered variable: ${{nr-var:some_string}}" + "{string_file_path}": "Some string contents with a rendered variable: ${{nr-var:some_string}}" + {dir_path}: + file1.txt: "File 1 contents" + file2.txt: | + File 2 contents with a variable: ${{nr-var:some_string}} + "{fully_templated_dir}": ${{nr-var:some_mapstringyaml}} "#, ), local_dir.join(DYNAMIC_AGENT_TYPE_FILENAME), ); - let agents = format!( - r#" - {agent_id}: - agent_type: "test/test:0.0.0" -"# - ); - + // Create AC config create_agent_control_config( opamp_server.endpoint(), opamp_server.jwks_endpoint(), - agents.to_string(), + format!( + r#" + {agent_id}: + agent_type: "test/test:0.0.0" +"# + ), local_dir.to_path_buf(), opamp_server.cert_file_path(), ); + // Values. Contains 3 variables: a YAML, a string, and a map[string]yaml (to create files in a directory) create_sub_agent_values( agent_id.to_string(), format!( r#" yaml_file_contents: {yaml_var_content} -some_string: "{string_var_content}""# +some_string: "{string_var_content}" +some_mapstringyaml: + file3.txt: "File 3 contents" + file4.txt: | + File 4 contents with a variable: {string_var_content} + file5.yaml: + my_key: my_value + my_seq: + - item1 + - item2 + my_string: |- + This is a multi-line + string in YAML +"# ), local_dir.to_path_buf(), ); @@ -174,20 +214,59 @@ some_string: "{string_var_content}""# let _agent_control = start_agent_control_with_custom_config(base_paths.clone(), Environment::OnHost); + // Rendered files let yaml_search_path = base_paths .remote_dir .join(GENERATED_FOLDER_NAME) .join(agent_id) + .join("randomdir") .join(yaml_file_path); let string_search_path = base_paths .remote_dir .join(GENERATED_FOLDER_NAME) .join(agent_id) + .join("randomdir") .join(string_file_path); + let dir_search_path = base_paths + .remote_dir + .join(GENERATED_FOLDER_NAME) + .join(agent_id) + .join(dir_path); + let fully_templated_dir_search_path = base_paths + .remote_dir + .join(GENERATED_FOLDER_NAME) + .join(agent_id) + .join(fully_templated_dir); + + let expected_files_with_contents = [ + (yaml_search_path, expected_yaml_file_contents), + (string_search_path, expected_string_file_contents), + ( + dir_search_path.join(expected_dir_file1.0), + expected_dir_file1.1, + ), + ( + dir_search_path.join(expected_dir_file2.0), + expected_dir_file2.1, + ), + ( + fully_templated_dir_search_path.join(expected_dir_file3.0), + expected_dir_file3.1, + ), + ( + fully_templated_dir_search_path.join(expected_dir_file4.0), + expected_dir_file4.1, + ), + ( + fully_templated_dir_search_path.join(expected_dir_file5.0), + expected_dir_file5.1, + ), + ]; retry(30, Duration::from_secs(1), || { - read_file_and_expect_content(&yaml_search_path, &expected_yaml_file_contents)?; - read_file_and_expect_content(&string_search_path, &expected_string_file_contents)?; + for (path, contents) in expected_files_with_contents.iter() { + read_file_and_expect_content(path, contents)?; + } Ok(()) }); }