[CLI] Make service hosts configurable in sui start
#18607
Conversation
…es to avoid setting the port to 0
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@stefan-mysten is attempting to deploy a commit to the Mysten Labs Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
I would like us to avoid proliferating flags here again -- can we make this all the job of the with_* flag? i.e.
# No indexer enabled
--with-indexer # Indexer enabled at default host and port
--with-indexer=9124 # At default host, port 9124
--with-indexer=0.0.0.0:9124 # At 0.0.0.0:9124
--with-indexer=0.0.0.0 # At 0.0.0.0 and default port
crates/sui/src/sui_commands.rs
Outdated
| /// When providing a specific value, please use the = sign between the flag and value: | ||
| /// `--with-indexer=6124`. | ||
| /// The indexer will be started in writer mode and reader mode. | ||
| #[clap(long, | ||
| default_value = "9124", |
There was a problem hiding this comment.
What's the distinction between default_value and default_missing_value -- do we need both?
There was a problem hiding this comment.
I found a bug where if the default_value is not provided, calling unwrap_or_default will return value 0 (if that field is an int). For example, if we call --with-graphql, the indexer port would have been set to 0 because the default_missing_value is only set when the flag is passed, but not when it is not passed, which is the case here due with-indexer being implicit enabled.
There was a problem hiding this comment.
Ah I see -- I guess my thinking here was that it shouldn't matter that it was zero if the flag was not passed, because at that point, the port was not going to be used for anything. Combining host and port into one flag addresses this issue anyway, I think.
But then if you only want a different port, you'd also have to type the address. Not so sure I follow why having a flag for port and one for host is problematic. Edit: I re-read what you wrote, ok, sure. |
|
@amnn I updated as per your suggestion. We need to have a fn to parse the provided input which can be nothing, a port, an ip, or both, which makes it more error prone than having two separate flags, but I added some tests that should cover all cases. |
crates/sui/src/sui_commands.rs
Outdated
| /// The indexer will be started in writer mode and reader mode. | ||
| #[clap(long, | ||
| default_missing_value = "9124", | ||
| default_value = "127.0.0.1:9124", |
There was a problem hiding this comment.
You should now no longer need to call unwrap_or_default on this value -- if it's None, then we are not going to run the indexer at all -- so you can remove this once again.
There was a problem hiding this comment.
We decided that with-graphql implies --with-indexer. If we keep that, then this is needed, unless we want to manually construct the IP + port instead of unwrapping a default.
crates/sui/src/sui_commands.rs
Outdated
| @@ -1142,3 +1155,19 @@ fn read_line() -> Result<String, anyhow::Error> { | |||
| io::stdin().read_line(&mut s)?; | |||
| Ok(s.trim_end().to_string()) | |||
| } | |||
|
|
|||
| pub fn parse_host_port(input: Option<String>, port: u16) -> Result<SocketAddr, AddrParseError> { | |||
There was a problem hiding this comment.
- nit: can we call it
default_port? It's a little confusing to follow at the moment because I keep thinking this is the port being parsed. - Does the host have to be a socket address (meaning does it need to have an IP address on the LHS, or can someone say
localhost:1234, for example)? - I think the default address here should be
0.0.0.0so that these services can accept connections from interfaces other than the loopback by default. - Does this function ever get called with
input = None? I would have thought not, anymore, in which case, let's makeinputnon-Option-al. - When do we encounter the
input.is_empty()case -- is that someone writing--with-indexer=? It may be better to treat that as an error rather than interpret it the same as--with-indexer, because they may have written--with-indexer= 1234or something, and we don't want to silently ignore the port assignment. This may require this function to return ananyhow::Resultso you can return an expressive error message for this case.
There was a problem hiding this comment.
can we call it default_port? It's a little confusing to follow at the moment because I keep thinking this is the port being parsed.
Fair point. The issue was that each service needs a different port, so we need to pass the port for each service as we do not know if the input from the command line has a port or not yet.
Does the host have to be a socket address (meaning does it need to have an IP address on the LHS, or can someone say localhost:1234, for example)?
Address, yes. We can probably allow localhost too!
I think the default address here should be 0.0.0.0
Sounds good
When do we encounter the input.is_empty() case -- is that someone writing --with-indexer=? It may be better to treat that as an error rather than interpret it the same as --with-indexer, because they may have written --with-indexer= 1234 or something, and we don't want to silently ignore the port assignment. This may require this function to return an anyhow::Result so you can return an expressive error message for this case.
I think this is not allowed by clap anyway, but it is an extra check for us. For example, using with-indexer= 1235 (note the space after =) clap will complain."
Edit: --with-indexer= is treated by clap as no argument is provided, so it will use the default value.
## Description Make indexer, GraphQL, faucet hosts configurable. When using Docker, due to the way it sets up a network interface for the container, the services need to bind to 0.0.0.0 to be able to be accessed from outside the container. This PR enables configurable hosts for these three services via optional flags: - `--indexer-host 0.0.0.0` - `--faucet-host 0.0.0.0` - `--graphql-host 0.0.0.0` If no host flag is provided, it will use the default `0.0.0.0` one. In addition, I found a bug where if the `default_value` is not provided, calling `unwrap_or_default` will return value 0 (if that field is an int). For example, if we call `--with-graphql`, the indexer port would have been set to 0 because the `default_missing_value` is only set when the flag is passed, but not when it is not passed, which is the case here due `with-indexer` being implicit enabled. This supersedes MystenLabs#14701 and should close MystenLabs#14701. ## Test plan How did you test the new or updated feature? --- ## Release notes Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required. For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates. - [ ] Protocol: - [ ] Nodes (Validators and Full nodes): - [ ] Indexer: - [ ] JSON-RPC: - [ ] GraphQL: - [x] CLI: Changed `sui start` to allow configurable hosts for indexer (`--indexer-host `), GraphQL (`--graphql-host `), and faucet (`--faucet-host`) services. This will enable to use `sui start` from a Docker container. By default, all services start with `0.0.0.0` host. - [ ] Rust SDK:
Description
Make indexer, GraphQL, faucet hosts configurable. When using Docker, due to the way it sets up a network interface for the container, the services need to bind to 0.0.0.0 to be able to be accessed from outside the container. This PR enables configurable hosts for these three services via optional flags:
--indexer-host 0.0.0.0--faucet-host 0.0.0.0--graphql-host 0.0.0.0If no host flag is provided, it will use the default
0.0.0.0one.In addition, I found a bug where if the
default_valueis not provided, callingunwrap_or_defaultwill return value 0 (if that field is an int). For example, if we call--with-graphql, the indexer port would have been set to 0 because thedefault_missing_valueis only set when the flag is passed, but not when it is not passed, which is the case here duewith-indexerbeing implicit enabled.This supersedes #14701 and should close #14701.
Test plan
How did you test the new or updated feature?
Release notes
Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.
For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.
sui startto allow configurable hosts for indexer (--indexer-host), GraphQL (--graphql-host), and faucet (--faucet-host) services. This enables the use ofsui startfrom a Docker container. By default, all services start with0.0.0.0host.