cli(on-hots): support empty proxy on config generation (#1834)

Author: sigilioso

agent-control/src/cli/on_host/config_gen.rs

@@ -4,16 +4,15 @@ use std::path::PathBuf;
 
 use tracing::info;
 
-use crate::{
-    cli::{
-        error::CliError,
-        on_host::config_gen::{
-            config::{AgentSet, AuthConfig, Config, FleetControl, Server, SignatureValidation},
-            identity::{Identity, provide_identity},
-            region::{Region, region_parser},
+use crate::cli::{
+    error::CliError,
+    on_host::config_gen::{
+        config::{
+            AgentSet, AuthConfig, Config, FleetControl, ProxyConfig, Server, SignatureValidation,
         },
+        identity::{Identity, provide_identity},
+        region::{Region, region_parser},
     },
-    http::config::ProxyConfig,
 };
 
 pub mod config;
@@ -121,6 +120,11 @@ impl Args {
                 ));
             }
         }
+        if let Some(proxy_config) = self.proxy_config.clone()
+            && let Err(err) = crate::http::config::ProxyConfig::try_from(proxy_config)
+        {
+            return Err(format!("invalid proxy configuration: {err}"));
+        }
         Ok(())
     }
 }
@@ -226,7 +230,7 @@ mod tests {
     fn test_args_validation(#[case] args: fn() -> String) {
         let cmd = Args::command().no_binary_name(true);
         let matches = cmd
-            .try_get_matches_from(args().split(" "))
+            .try_get_matches_from(args().split_ascii_whitespace())
             .expect("arguments should be valid");
         let args = Args::from_arg_matches(&matches).expect("should create the struct back");
         assert_matches!(args.validate(), Ok(_));
@@ -254,10 +258,13 @@ mod tests {
     #[case::missing_auth_parent_client_id_with_secret(
         || format!("--output-path /some/path --agent-set otel --region us --auth-private-key-path {} --auth-parent-client-secret SECRET --auth-parent-client-id id", pwd())
     )]
+    #[case::invalid_proxy_config(
+        || String::from("--fleet-disabled --output-path /some/path --agent-set otel --region us --proxy-url https::/invalid")
+    )]
     fn test_args_validation_errors(#[case] args: fn() -> String) {
         let cmd = Args::command().no_binary_name(true);
         let matches = cmd
-            .try_get_matches_from(args().split(" "))
+            .try_get_matches_from(args().split_ascii_whitespace())
             .expect("arguments should be valid");
         let args = Args::from_arg_matches(&matches).expect("should create the struct back");
 
@@ -348,11 +355,12 @@ mod tests {
     }
 
     fn some_proxy_config() -> Option<ProxyConfig> {
-        let proxy_config: ProxyConfig = serde_yaml::from_str(
-            r#"{"url": "https://some.proxy.url/", "ca_bundle_dir": "/test/bundle/dir",
-                "ca_bundle_file": "/test/bundle/file", "ignore_system_proxy": true}"#,
-        )
-        .unwrap();
+        let proxy_config = ProxyConfig {
+            proxy_url: Some("https://some.proxy.url/".to_string()),
+            proxy_ca_bundle_dir: Some("/test/bundle/dir".to_string()),
+            proxy_ca_bundle_file: Some("/test/bundle/file".to_string()),
+            ignore_system_proxy: true,
+        };
         Some(proxy_config)
     }
 

agent-control/src/cli/on_host/config_gen/config.rs

@@ -1,11 +1,9 @@
 //! Contains the definition of the configuration to be generated
 
-use std::collections::HashMap;
+use std::{collections::HashMap, convert::Infallible};
 
 use serde::Serialize;
 
-use crate::http::config::ProxyConfig;
-
 /// Represents the set of agents to be included in the AC configuration.
 #[derive(Debug, Copy, Clone, PartialEq, clap::ValueEnum)]
 pub enum AgentSet {
@@ -36,6 +34,46 @@ impl From<AgentSet> for HashMap<String, Agent> {
     }
 }
 
+/// Holds the proxy configuration.
+/// Cannot use [crate::http::config::ProxyConfig] directly due lack of support for defaults in clap.
+/// See <https://github.com/clap-rs/clap/issues/4746> for details.
+#[derive(Debug, Default, Clone, PartialEq, Serialize, clap::Args)]
+pub struct ProxyConfig {
+    #[serde(skip_serializing_if = "is_none_or_empty_string", rename = "url")]
+    #[arg(long, required = false)]
+    pub proxy_url: Option<String>,
+
+    #[serde(
+        skip_serializing_if = "is_none_or_empty_string",
+        rename = "ca_bundle_dir"
+    )]
+    #[arg(long, required = false)]
+    pub proxy_ca_bundle_dir: Option<String>,
+
+    #[serde(
+        skip_serializing_if = "is_none_or_empty_string",
+        rename = "ca_bundle_file"
+    )]
+    #[arg(long, required = false)]
+    pub proxy_ca_bundle_file: Option<String>,
+
+    #[arg(long, default_value_t = false, value_parser = ignore_system_proxy_parser, action = clap::ArgAction::Set)]
+    pub ignore_system_proxy: bool,
+}
+
+// Helper to avoid serializing empty values
+fn is_none_or_empty_string(v: &Option<String>) -> bool {
+    v.as_ref().map(|s| s.is_empty()).unwrap_or(true)
+}
+
+// Custom parser to allow empty values as false booleans
+fn ignore_system_proxy_parser(s: &str) -> Result<bool, Infallible> {
+    match s.to_lowercase().as_str() {
+        "" | "false" => Ok(false),
+        _ => Ok(true),
+    }
+}
+
 /// Configuration to be written as result of the corresponding command.
 #[derive(Debug, PartialEq, Serialize)]
 pub struct Config {
@@ -84,6 +122,7 @@ pub struct Agent {
 #[cfg(test)]
 mod tests {
     use super::*;
+    use clap::{Args, FromArgMatches};
     use rstest::rstest;
 
     #[rstest]
@@ -109,4 +148,54 @@ mod tests {
 
         assert_eq!(result, expected_map);
     }
+
+    #[test]
+    fn test_serialize_proxy_config_all_empty_options() {
+        let proxy_config = ProxyConfig {
+            proxy_url: Some(String::new()),
+            proxy_ca_bundle_dir: Some(String::new()),
+            proxy_ca_bundle_file: Some(String::new()),
+            ignore_system_proxy: false,
+        };
+
+        let serialized = serde_yaml::to_string(&proxy_config).unwrap();
+        // Only ignore_system_proxy should be present
+        assert_eq!(serialized.trim(), "ignore_system_proxy: false");
+    }
+
+    #[test]
+    fn test_serialize_proxy_config_none_options() {
+        let proxy_config = ProxyConfig {
+            proxy_url: None,
+            proxy_ca_bundle_dir: None,
+            proxy_ca_bundle_file: None,
+            ignore_system_proxy: true,
+        };
+
+        let serialized = serde_yaml::to_string(&proxy_config).unwrap();
+        // Only ignore_system_proxy should be present
+        assert_eq!(serialized.trim(), "ignore_system_proxy: true");
+    }
+
+    #[rstest]
+    #[case("", ProxyConfig::default())]
+    #[case(
+        "--proxy-url https://proxy.url --proxy-ca-bundle-dir=/bundle/dir --proxy-ca-bundle-file=/bundle/file --ignore-system-proxy true",
+        ProxyConfig{proxy_url: Some("https://proxy.url".into()), proxy_ca_bundle_dir: Some("/bundle/dir".into()), proxy_ca_bundle_file: Some("/bundle/file".into()), ignore_system_proxy: true},
+    )]
+    #[case("--proxy-url= --proxy-ca-bundle-dir= --proxy-ca-bundle-file= --ignore-system-proxy=", ProxyConfig{proxy_url: Some("".into()), proxy_ca_bundle_dir: Some("".into()), proxy_ca_bundle_file: Some("".into()), ignore_system_proxy: false})]
+    #[case(" --ignore-system-proxy=", ProxyConfig{ignore_system_proxy: false, ..Default::default()})]
+    #[case(" --ignore-system-proxy=false", ProxyConfig{ignore_system_proxy: false, ..Default::default()})]
+    #[case(" --ignore-system-proxy=true", ProxyConfig{ignore_system_proxy: true, ..Default::default()})]
+    #[case(" --ignore-system-proxy=False", ProxyConfig{ignore_system_proxy: false, ..Default::default()})]
+    #[case(" --ignore-system-proxy=True", ProxyConfig{ignore_system_proxy: true, ..Default::default()})]
+    fn test_proxy_args(#[case] args: &str, #[case] expected: ProxyConfig) {
+        let cmd = clap::Command::new("test").no_binary_name(true);
+        let cmd = ProxyConfig::augment_args(cmd);
+        let matches = cmd
+            .try_get_matches_from(args.split_ascii_whitespace())
+            .expect("arguments should be valid");
+        let value = ProxyConfig::from_arg_matches(&matches).expect("should create the struct back");
+        assert_eq!(value, expected)
+    }
 }

agent-control/src/cli/on_host/config_gen/identity.rs

@@ -23,7 +23,7 @@ use nr_auth::{
 };
 use tracing::info;
 
-use crate::{cli::error::CliError, http::config::ProxyConfig};
+use crate::cli::{error::CliError, on_host::config_gen::config::ProxyConfig};
 
 use super::Args;
 
@@ -142,13 +142,13 @@ fn build_nr_auth_proxy_config(
     ac_cfg: ProxyConfig,
 ) -> Result<nr_auth::http::config::ProxyConfig, CliError> {
     let auth_cfg = nr_auth::http::config::ProxyConfig::new(
-        ac_cfg.url_as_string(),
-        ac_cfg.ca_bundle_dir().to_owned(),
-        ac_cfg.ca_bundle_file().to_owned(),
+        ac_cfg.proxy_url.unwrap_or_default(),
+        PathBuf::from(ac_cfg.proxy_ca_bundle_dir.unwrap_or_default()),
+        PathBuf::from(ac_cfg.proxy_ca_bundle_file.unwrap_or_default()),
     )
     .map_err(|err| CliError::Command(format!("invalid proxy configuration: {err}")))?;
 
-    if ac_cfg.ignore_system_proxy() {
+    if ac_cfg.ignore_system_proxy {
         Ok(auth_cfg)
     } else {
         auth_cfg

agent-control/src/http/config.rs

@@ -124,26 +124,21 @@ impl ProxyUrl {
 /// // The url will contain the value corresponding to the standard environment variables.
 /// let proxy_config = ProxyConfig::default().try_with_url_from_env().unwrap();
 /// ```
-#[derive(Debug, Deserialize, Serialize, PartialEq, Clone, Default, clap::Args)]
-#[group()]
+#[derive(Debug, Deserialize, Serialize, PartialEq, Clone, Default)]
 pub struct ProxyConfig {
     /// Proxy URL proxy:
     /// <protocol>://<user>:<password>@<host>:<port>
     /// (All parts except host are optional)
     #[serde(default)]
-    #[arg(long="proxy-url", value_parser = clap::builder::ValueParser::new(|s: &str| ProxyUrl::try_from(s)), required=false)]
     url: ProxyUrl,
     /// System path with the CA certificates in PEM format. All `.pem` files in the directory are read.
     #[serde(default)]
-    #[arg(long = "proxy-ca-bundle-dir", required = false)]
     ca_bundle_dir: PathBuf,
     /// System path with the CA certificate in PEM format.
     #[serde(default)]
-    #[arg(long = "proxy-ca-bundle-file", required = false)]
     ca_bundle_file: PathBuf,
     /// When set to true, the HTTPS_PROXY and HTTP_PROXY environment variables are ignored, defaults to false.
     #[serde(default)]
-    #[arg(long, required = false)]
     ignore_system_proxy: bool,
 }
 
@@ -191,6 +186,22 @@ impl ProxyConfig {
     }
 }
 
+impl TryFrom<crate::cli::on_host::config_gen::config::ProxyConfig> for ProxyConfig {
+    type Error = ProxyError;
+
+    fn try_from(
+        value: crate::cli::on_host::config_gen::config::ProxyConfig,
+    ) -> Result<Self, Self::Error> {
+        let url = value.proxy_url.unwrap_or_default();
+        Ok(Self {
+            url: ProxyUrl::try_from(url.as_str())?,
+            ca_bundle_dir: PathBuf::from(value.proxy_ca_bundle_dir.unwrap_or_default()),
+            ca_bundle_file: PathBuf::from(value.proxy_ca_bundle_file.unwrap_or_default()),
+            ignore_system_proxy: value.ignore_system_proxy,
+        })
+    }
+}
+
 #[cfg(test)]
 pub(crate) mod tests {
     use super::ProxyError;

agent-control/tests/on_host/agent_control_cli.rs

@@ -10,9 +10,9 @@ fn test_config_generator_fleet_disabled_proxy() {
 
     let mut cmd = cargo_bin_cmd!("newrelic-agent-control-cli");
     let args = format!(
-        "generate-config --fleet-disabled --agent-set infra-agent --region us --proxy-url https://some.proxy.url/ --proxy-ca-bundle-dir /test/bundle/dir --proxy-ca-bundle-file /test/bundle/file --ignore-system-proxy --output-path {path}",
+        "generate-config --fleet-disabled --agent-set infra-agent --region us --proxy-url https://some.proxy.url/ --proxy-ca-bundle-dir /test/bundle/dir --ignore-system-proxy true --output-path {path}",
     );
-    cmd.args(args.split(" "));
+    cmd.args(args.split_ascii_whitespace());
     cmd.assert().success();
 
     let expected_value: serde_yaml::Value = serde_yaml::from_str(
@@ -25,7 +25,6 @@ agents:
 proxy:
   url: https://some.proxy.url/
   ca_bundle_dir: /test/bundle/dir
-  ca_bundle_file: /test/bundle/file
   ignore_system_proxy: true
     "#,
     )
@@ -35,6 +34,35 @@ proxy:
     assert_eq!(actual_value, expected_value);
 }
 
+#[test]
+fn test_config_generator_fleet_disabled_proxy_empty_fields() {
+    let tmp = TempDir::new().unwrap();
+    let path = tmp.path().join("output.yaml").to_string_lossy().to_string();
+
+    let mut cmd = cargo_bin_cmd!("newrelic-agent-control-cli");
+    let args = format!(
+        "generate-config --fleet-disabled --agent-set infra-agent --region us --proxy-url= --proxy-ca-bundle-dir= --proxy-ca-bundle-file= --ignore-system-proxy= --output-path {path}",
+    );
+    cmd.args(args.split_ascii_whitespace());
+    cmd.assert().success();
+
+    let expected_value: serde_yaml::Value = serde_yaml::from_str(
+        r#"
+server:
+  enabled: true
+agents:
+  nr-infra:
+    agent_type: "newrelic/com.newrelic.infrastructure:0.1.0"
+proxy:
+  ignore_system_proxy: false
+    "#,
+    )
+    .unwrap();
+    let actual_content = std::fs::read_to_string(&path).unwrap();
+    let actual_value: serde_yaml::Value = serde_yaml::from_str(&actual_content).unwrap();
+    assert_eq!(actual_value, expected_value);
+}
+
 #[test]
 fn test_config_generator_fleet_enabled_identity_provisioned() {
     let tmp = TempDir::new().unwrap();
@@ -47,7 +75,7 @@ fn test_config_generator_fleet_enabled_identity_provisioned() {
     let args = format!(
         "generate-config --agent-set infra-agent --region us --fleet-id FLEET-ID --auth-client-id CLIENT-ID --auth-private-key-path {key_path} --output-path {path}",
     );
-    cmd.args(args.split(" "));
+    cmd.args(args.split_ascii_whitespace());
     cmd.assert().success();
 
     let expected_value: serde_yaml::Value = serde_yaml::from_str(