From c5588eb66e26272bb99b590492c6bd7bb5a5ff81 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Oliveira?= Date: Fri, 22 Sep 2023 02:33:10 +0000 Subject: [PATCH] require http and metrics for respective flags (#4674) ## Issue Addressed following discussion on https://github.com/sigp/lighthouse/pull/4639#discussion_r1305183750 this PR makes the `http` and `metrics` sub-flags to require those main flags enabled --- beacon_node/src/cli.rs | 31 +++++--- beacon_node/src/config.rs | 122 ++++++++++++++++---------------- lighthouse/tests/beacon_node.rs | 5 ++ validator_client/src/cli.rs | 16 +++-- 4 files changed, 100 insertions(+), 74 deletions(-) diff --git a/beacon_node/src/cli.rs b/beacon_node/src/cli.rs index ffc29365c..dacff79f6 100644 --- 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; @@ -355,22 +355,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). \ @@ -381,11 +384,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.") @@ -403,6 +408,7 @@ 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) @@ -410,6 +416,7 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> { .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) @@ -417,6 +424,7 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> { .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.") @@ -424,8 +432,9 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> { .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.") @@ -433,8 +442,9 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> { .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.") @@ -442,13 +452,14 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> { .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( @@ -461,22 +472,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 \ @@ -1259,4 +1273,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 index b6d0b75d9..4ab92a7fd 100644 --- a/beacon_node/src/config.rs +++ b/beacon_node/src/config.rs @@ -94,70 +94,70 @@ pub fn get_config( * 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::() + .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::() + .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")?; + + 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 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::() + .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::() + .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; + } + + 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.duplicate_block_status_code = + parse_required(cli_args, "http-duplicate-block-status")?; } - if let Some(address) = cli_args.value_of("http-address") { - client_config.http_api.listen_addr = address - .parse::() - .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::() - .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")?; - - 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 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::() - .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::() - .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; - } - - 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.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 index 10fcb101a..f5fe7334d 100644 --- 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::().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::().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() @@ -2446,6 +2449,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)); @@ -2463,6 +2467,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 index 0789ac78a..0af92a9e3 100644 --- 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). \