Skip to content

Commit

Permalink
require http and metrics for respective flags (#4674)
Browse files Browse the repository at this point in the history 
## Issue Addressed

following discussion on #4639 (comment) this PR makes the `http` and `metrics` sub-flags to require those main flags enabled
  • Loading branch information
@jxs
jxs committed Sep 22, 2023
1 parent aa867f0 commit 46ba930
Show file tree
Hide file tree
Showing 4 changed files with 90 additions and 64 deletions.
31 changes: 23 additions & 8 deletions beacon_node/src/cli.rs
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use clap::{App, Arg};
use clap::{App, Arg, ArgGroup};
use strum::VariantNames;
use types::ProgressiveBalancesMode;

Expand Down Expand Up @@ -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). \
Expand All @@ -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.")
Expand All @@ -403,52 +408,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(
Expand All @@ -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 \
Expand Down Expand Up @@ -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"]))
}
102 changes: 51 additions & 51 deletions beacon_node/src/config.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,69 +94,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;
Expand Down
5 changes: 5 additions & 0 deletions lighthouse/tests/beacon_node.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand All @@ -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));
Expand All @@ -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()
Expand Down Expand Up @@ -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));
Expand All @@ -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| {
Expand Down
16 changes: 11 additions & 5 deletions validator_client/src/cli.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 \
Expand All @@ -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). \
Expand All @@ -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 */
Expand All @@ -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). \
Expand Down

0 comments on commit 46ba930

Please sign in to comment.