require http and metrics for respective flags (sigp#4674)

jxs · Woodpile37 · commit 23ba3fbd46e1 · 2024-01-06T07:08:31.000-06:00
## Issue Addressed following discussion on sigp#4639 (comment) this PR makes the `http` and `metrics` sub-flags to require those main flags enabled
diff --git a/beacon_node/src/cli.rs b/beacon_node/src/cli.rs
@@ -1,4 +1,4 @@
-use clap::{App, Arg};
+use clap::{App, Arg, ArgGroup};
 use strum::VariantNames;
 use types::ProgressiveBalancesMode;
 
@@ -362,22 +362,25 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> {
         .arg(
             Arg::with_name("http-address")
                 .long("http-address")
+                .requires("enable_http")
                 .value_name("ADDRESS")
                 .help("Set the listen address for the RESTful HTTP API server.")
-                .default_value("127.0.0.1")
+                .default_value_if("enable_http", None, "127.0.0.1")
                 .takes_value(true),
         )
         .arg(
             Arg::with_name("http-port")
                 .long("http-port")
+                .requires("enable_http")
                 .value_name("PORT")
                 .help("Set the listen TCP port for the RESTful HTTP API server.")
-                .default_value("5052")
+                .default_value_if("enable_http", None, "5052")
                 .takes_value(true),
         )
         .arg(
             Arg::with_name("http-allow-origin")
                 .long("http-allow-origin")
+                .requires("enable_http")
                 .value_name("ORIGIN")
                 .help("Set the value of the Access-Control-Allow-Origin response HTTP header. \
                     Use * to allow any origin (not recommended in production). \
@@ -388,11 +391,13 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> {
         .arg(
             Arg::with_name("http-disable-legacy-spec")
                 .long("http-disable-legacy-spec")
+                .requires("enable_http")
                 .hidden(true)
         )
         .arg(
             Arg::with_name("http-spec-fork")
                 .long("http-spec-fork")
+                .requires("enable_http")
                 .value_name("FORK")
                 .help("Serve the spec for a specific hard fork on /eth/v1/config/spec. It should \
                        not be necessary to set this flag.")
@@ -410,52 +415,58 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> {
         .arg(
             Arg::with_name("http-tls-cert")
                 .long("http-tls-cert")
+                .requires("enable_http")
                 .help("The path of the certificate to be used when serving the HTTP API server \
                     over TLS.")
                 .takes_value(true)
         )
         .arg(
             Arg::with_name("http-tls-key")
                 .long("http-tls-key")
+                .requires("enable_http")
                 .help("The path of the private key to be used when serving the HTTP API server \
                     over TLS. Must not be password-protected.")
                 .takes_value(true)
         )
         .arg(
             Arg::with_name("http-allow-sync-stalled")
                 .long("http-allow-sync-stalled")
+                .requires("enable_http")
                 .help("Forces the HTTP to indicate that the node is synced when sync is actually \
                     stalled. This is useful for very small testnets. TESTING ONLY. DO NOT USE ON \
                     MAINNET.")
         )
         .arg(
             Arg::with_name("http-sse-capacity-multiplier")
                 .long("http-sse-capacity-multiplier")
+                .requires("enable_http")
                 .takes_value(true)
-                .default_value("1")
+                .default_value_if("enable_http", None, "1")
                 .value_name("N")
                 .help("Multiplier to apply to the length of HTTP server-sent-event (SSE) channels. \
                        Increasing this value can prevent messages from being dropped.")
         )
         .arg(
             Arg::with_name("http-duplicate-block-status")
                 .long("http-duplicate-block-status")
+                .requires("enable_http")
                 .takes_value(true)
-                .default_value("202")
+                .default_value_if("enable_http", None, "202")
                 .value_name("STATUS_CODE")
                 .help("Status code to send when a block that is already known is POSTed to the \
                        HTTP API.")
         )
         .arg(
             Arg::with_name("http-enable-beacon-processor")
                 .long("http-enable-beacon-processor")
+                .requires("enable_http")
                 .value_name("BOOLEAN")
                 .help("The beacon processor is a scheduler which provides quality-of-service and \
                     DoS protection. When set to \"true\", HTTP API requests will be queued and scheduled \
                     alongside other tasks. When set to \"false\", HTTP API responses will be executed \
                     immediately.")
                 .takes_value(true)
-                .default_value("true")
+                .default_value_if("enable_http", None, "true")
         )
         /* Prometheus metrics HTTP server related arguments */
         .arg(
@@ -468,22 +479,25 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> {
             Arg::with_name("metrics-address")
                 .long("metrics-address")
                 .value_name("ADDRESS")
+                .requires("metrics")
                 .help("Set the listen address for the Prometheus metrics HTTP server.")
-                .default_value("127.0.0.1")
+                .default_value_if("metrics", None, "127.0.0.1")
                 .takes_value(true),
         )
         .arg(
             Arg::with_name("metrics-port")
                 .long("metrics-port")
+                .requires("metrics")
                 .value_name("PORT")
                 .help("Set the listen TCP port for the Prometheus metrics HTTP server.")
-                .default_value("5054")
+                .default_value_if("metrics", None, "5054")
                 .takes_value(true),
         )
         .arg(
             Arg::with_name("metrics-allow-origin")
                 .long("metrics-allow-origin")
                 .value_name("ORIGIN")
+                .requires("metrics")
                 .help("Set the value of the Access-Control-Allow-Origin response HTTP header. \
                     Use * to allow any origin (not recommended in production). \
                     If no value is supplied, the CORS allowed origin is set to the listen \
@@ -1301,4 +1315,5 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> {
                 .default_value("64")
                 .takes_value(true)
         )
+        .group(ArgGroup::with_name("enable_http").args(&["http", "gui", "staking"]))
 }
diff --git a/beacon_node/src/config.rs b/beacon_node/src/config.rs
@@ -95,69 +95,69 @@ pub fn get_config<E: EthSpec>(
      * Http API server
      */
 
-    if cli_args.is_present("http") {
+    if cli_args.is_present("enable_http") {
         client_config.http_api.enabled = true;
-    }
 
-    if let Some(address) = cli_args.value_of("http-address") {
-        client_config.http_api.listen_addr = address
-            .parse::<IpAddr>()
-            .map_err(|_| "http-address is not a valid IP address.")?;
-    }
+        if let Some(address) = cli_args.value_of("http-address") {
+            client_config.http_api.listen_addr = address
+                .parse::<IpAddr>()
+                .map_err(|_| "http-address is not a valid IP address.")?;
+        }
 
-    if let Some(port) = cli_args.value_of("http-port") {
-        client_config.http_api.listen_port = port
-            .parse::<u16>()
-            .map_err(|_| "http-port is not a valid u16.")?;
-    }
+        if let Some(port) = cli_args.value_of("http-port") {
+            client_config.http_api.listen_port = port
+                .parse::<u16>()
+                .map_err(|_| "http-port is not a valid u16.")?;
+        }
 
-    if let Some(allow_origin) = cli_args.value_of("http-allow-origin") {
-        // Pre-validate the config value to give feedback to the user on node startup, instead of
-        // as late as when the first API response is produced.
-        hyper::header::HeaderValue::from_str(allow_origin)
-            .map_err(|_| "Invalid allow-origin value")?;
+        if let Some(allow_origin) = cli_args.value_of("http-allow-origin") {
+            // Pre-validate the config value to give feedback to the user on node startup, instead of
+            // as late as when the first API response is produced.
+            hyper::header::HeaderValue::from_str(allow_origin)
+                .map_err(|_| "Invalid allow-origin value")?;
 
-        client_config.http_api.allow_origin = Some(allow_origin.to_string());
-    }
+            client_config.http_api.allow_origin = Some(allow_origin.to_string());
+        }
 
-    if cli_args.is_present("http-disable-legacy-spec") {
-        warn!(
-            log,
-            "The flag --http-disable-legacy-spec is deprecated and will be removed"
-        );
-    }
+        if cli_args.is_present("http-disable-legacy-spec") {
+            warn!(
+                log,
+                "The flag --http-disable-legacy-spec is deprecated and will be removed"
+            );
+        }
 
-    if let Some(fork_name) = clap_utils::parse_optional(cli_args, "http-spec-fork")? {
-        client_config.http_api.spec_fork_name = Some(fork_name);
-    }
+        if let Some(fork_name) = clap_utils::parse_optional(cli_args, "http-spec-fork")? {
+            client_config.http_api.spec_fork_name = Some(fork_name);
+        }
 
-    if cli_args.is_present("http-enable-tls") {
-        client_config.http_api.tls_config = Some(TlsConfig {
-            cert: cli_args
-                .value_of("http-tls-cert")
-                .ok_or("--http-tls-cert was not provided.")?
-                .parse::<PathBuf>()
-                .map_err(|_| "http-tls-cert is not a valid path name.")?,
-            key: cli_args
-                .value_of("http-tls-key")
-                .ok_or("--http-tls-key was not provided.")?
-                .parse::<PathBuf>()
-                .map_err(|_| "http-tls-key is not a valid path name.")?,
-        });
-    }
+        if cli_args.is_present("http-enable-tls") {
+            client_config.http_api.tls_config = Some(TlsConfig {
+                cert: cli_args
+                    .value_of("http-tls-cert")
+                    .ok_or("--http-tls-cert was not provided.")?
+                    .parse::<PathBuf>()
+                    .map_err(|_| "http-tls-cert is not a valid path name.")?,
+                key: cli_args
+                    .value_of("http-tls-key")
+                    .ok_or("--http-tls-key was not provided.")?
+                    .parse::<PathBuf>()
+                    .map_err(|_| "http-tls-key is not a valid path name.")?,
+            });
+        }
 
-    if cli_args.is_present("http-allow-sync-stalled") {
-        client_config.http_api.allow_sync_stalled = true;
-    }
+        if cli_args.is_present("http-allow-sync-stalled") {
+            client_config.http_api.allow_sync_stalled = true;
+        }
 
-    client_config.http_api.sse_capacity_multiplier =
-        parse_required(cli_args, "http-sse-capacity-multiplier")?;
+        client_config.http_api.sse_capacity_multiplier =
+            parse_required(cli_args, "http-sse-capacity-multiplier")?;
 
-    client_config.http_api.enable_beacon_processor =
-        parse_required(cli_args, "http-enable-beacon-processor")?;
+        client_config.http_api.enable_beacon_processor =
+            parse_required(cli_args, "http-enable-beacon-processor")?;
 
-    client_config.http_api.duplicate_block_status_code =
-        parse_required(cli_args, "http-duplicate-block-status")?;
+        client_config.http_api.duplicate_block_status_code =
+            parse_required(cli_args, "http-duplicate-block-status")?;
+    }
 
     if let Some(cache_size) = clap_utils::parse_optional(cli_args, "shuffling-cache-size")? {
         client_config.chain.shuffling_cache_size = cache_size;
diff --git a/lighthouse/tests/beacon_node.rs b/lighthouse/tests/beacon_node.rs
@@ -1466,6 +1466,7 @@ fn http_flag() {
 fn http_address_flag() {
     let addr = "127.0.0.99".parse::<IpAddr>().unwrap();
     CommandLineTest::new()
+        .flag("http", None)
         .flag("http-address", Some("127.0.0.99"))
         .run_with_zero_port()
         .with_config(|config| assert_eq!(config.http_api.listen_addr, addr));
@@ -1474,6 +1475,7 @@ fn http_address_flag() {
 fn http_address_ipv6_flag() {
     let addr = "::1".parse::<IpAddr>().unwrap();
     CommandLineTest::new()
+        .flag("http", None)
         .flag("http-address", Some("::1"))
         .run_with_zero_port()
         .with_config(|config| assert_eq!(config.http_api.listen_addr, addr));
@@ -1483,6 +1485,7 @@ fn http_port_flag() {
     let port1 = unused_tcp4_port().expect("Unable to find unused port.");
     let port2 = unused_tcp4_port().expect("Unable to find unused port.");
     CommandLineTest::new()
+        .flag("http", None)
         .flag("http-port", Some(port1.to_string().as_str()))
         .flag("port", Some(port2.to_string().as_str()))
         .run()
@@ -2485,6 +2488,7 @@ fn http_sse_capacity_multiplier_default() {
 #[test]
 fn http_sse_capacity_multiplier_override() {
     CommandLineTest::new()
+        .flag("http", None)
         .flag("http-sse-capacity-multiplier", Some("10"))
         .run_with_zero_port()
         .with_config(|config| assert_eq!(config.http_api.sse_capacity_multiplier, 10));
@@ -2502,6 +2506,7 @@ fn http_duplicate_block_status_default() {
 #[test]
 fn http_duplicate_block_status_override() {
     CommandLineTest::new()
+        .flag("http", None)
         .flag("http-duplicate-block-status", Some("301"))
         .run_with_zero_port()
         .with_config(|config| {
diff --git a/validator_client/src/cli.rs b/validator_client/src/cli.rs
@@ -170,6 +170,7 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> {
          .arg(
              Arg::with_name("http-address")
                  .long("http-address")
+                 .requires("http")
                  .value_name("ADDRESS")
                  .help("Set the address for the HTTP address. The HTTP server is not encrypted \
                         and therefore it is unsafe to publish on a public network. When this \
@@ -189,14 +190,16 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> {
         .arg(
             Arg::with_name("http-port")
                 .long("http-port")
+                .requires("http")
                 .value_name("PORT")
                 .help("Set the listen TCP port for the RESTful HTTP API server.")
-                .default_value("5062")
+                .default_value_if("http", None, "5062")
                 .takes_value(true),
         )
         .arg(
             Arg::with_name("http-allow-origin")
                 .long("http-allow-origin")
+                .requires("http")
                 .value_name("ORIGIN")
                 .help("Set the value of the Access-Control-Allow-Origin response HTTP header. \
                     Use * to allow any origin (not recommended in production). \
@@ -207,21 +210,21 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> {
         .arg(
             Arg::with_name("http-allow-keystore-export")
                 .long("http-allow-keystore-export")
+                .requires("http")
                 .help("If present, allow access to the DELETE /lighthouse/keystores HTTP \
                     API method, which allows exporting keystores and passwords to HTTP API \
                     consumers who have access to the API token. This method is useful for \
                     exporting validators, however it should be used with caution since it \
                     exposes private key data to authorized users.")
-                .required(false)
                 .takes_value(false),
         )
         .arg(
             Arg::with_name("http-store-passwords-in-secrets-dir")
                 .long("http-store-passwords-in-secrets-dir")
+                .requires("http")
                 .help("If present, any validators created via the HTTP will have keystore \
                     passwords stored in the secrets-dir rather than the validator \
                     definitions file.")
-                .required(false)
                 .takes_value(false),
         )
         /* Prometheus metrics HTTP server related arguments */
@@ -234,22 +237,25 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> {
         .arg(
             Arg::with_name("metrics-address")
                 .long("metrics-address")
+                .requires("metrics")
                 .value_name("ADDRESS")
                 .help("Set the listen address for the Prometheus metrics HTTP server.")
-                .default_value("127.0.0.1")
+                .default_value_if("metrics", None, "127.0.0.1")
                 .takes_value(true),
         )
         .arg(
             Arg::with_name("metrics-port")
                 .long("metrics-port")
+                .requires("metrics")
                 .value_name("PORT")
                 .help("Set the listen TCP port for the Prometheus metrics HTTP server.")
-                .default_value("5064")
+                .default_value_if("metrics", None, "5064")
                 .takes_value(true),
         )
         .arg(
             Arg::with_name("metrics-allow-origin")
                 .long("metrics-allow-origin")
+                .requires("metrics")
                 .value_name("ORIGIN")
                 .help("Set the value of the Access-Control-Allow-Origin response HTTP header. \
                     Use * to allow any origin (not recommended in production). \
