Reverse port-forwarding #2781
Reverse port-forwarding #2781
Conversation
gememma
force-pushed
the
reverse-portfwd
branch
4 times, most recently
from
5544be9
to
44c83fa
Compare
|
Before I even start to review it: reverse port-forwarding should obviously be called port-backwarding. |
|
I only narrowly decided to call the struct |
| mirrord-analytics = { path = "../analytics" } | ||
| mirrord-intproxy = { path = "../intproxy" } | ||
| mirrord-vpn = { path = "../vpn" } | ||
| mirrord-layer = { path = "../layer" } |
|
|
||
| /// Enable reverse port forwarding | ||
| #[arg(short = 'r', long)] | ||
| pub reverse_port_forward: bool, |
There was a problem hiding this comment.
This argument is redundant, let's remove it and run in reverse if a reverse mapping was passed.
| pub reverse_port_forward: bool, | ||
|
|
||
| /// Mappings for reverse port forwarding. | ||
| /// Expected format is: '-R \[remote_port:\]local_port'. |
There was a problem hiding this comment.
To include multiple values, do users pass -R multiple times? Maybe you could make it clear here how to pass multiple values.
And could you please also add a short explanation on port-backwarding that includes the remote and local ports? This will show up in the help message.
| @@ -397,11 +397,20 @@ pub(super) struct PortForwardArgs { | |||
| /// Otherwise, the remote is assumed to be a hostname and lookup is performed in the cluster | |||
| /// after a connection is made to the target. | |||
| #[arg(short = 'L', long)] | |||
| pub port_mappings: Vec<PortMapping>, | |||
| pub port_mappings: Option<Vec<AddrPortMapping>>, | |||
There was a problem hiding this comment.
Making the two port mappings optional and checking explicitly in our code that one of them exists, is not a good solution, we should let clap verify correct usage for us.
Also, this is fixable, but that verification happens after mirrord already does a bunch of stuff like connecting to the operator, creating an agent, etc. But a problem in the CLI usage should lead to an immediate error. And the error that is displayed to the user tells them it's a bug, which it shouldn't.
In fact, these two issues exist in port-forwarding already:
I recommend taking one of the two following solutions:
- Instead of making this optional we have two separate commands for port-forwarding and for port-backwarding, all the common arguments can be extracted to a third struct and then included using clap's
flatten, likeTargetParamsis included here. They each have a required port mapping arg with the appropriate type. - Leave it as one command, but require exactly one of the two mappings is passed using
required_unless_presentandconflicts_with(or maybe there is a "xor" function. I couldn't find it in a quick search in the docs).
Either way, I also think:
- This argument could be positional. I think that's a good interface for a single required value. Also, it would be similar to kubectl's port-forward, which many of our users are used to, wdyt?
- I think it should be called
port_mapping, singular, because it's a single mapping that maps multiple values.
There was a problem hiding this comment.
The original reason that mapping args are not positional and use -L is to imitate the ssh port forward command as closely as possible, with -R then being chosen for the reverse. After some internal discussion, it has been decided that the user should be able to use -L and -R arguments in the same command. In this light, I can remove the -r flag (you're right, it isn't needed) and go from there - I'll play around with the clap required-adjacent stuff to see if it can enforce the presence of at least one of -L, -R
| /// associates resolved destination ports with local ports. | ||
| mappings: HashMap<RemotePort, LocalPort>, |
| for mapping in &parsed_mappings { | ||
| // check destinations are unique | ||
| if mappings.contains_key(&mapping.remote) { | ||
| // two mappings shared a key thus keys were not unique | ||
| return Err(PortForwardError::ReversePortMapSetupError(mapping.remote)); | ||
| } | ||
| mappings.insert(mapping.remote, mapping.local); | ||
| } |
There was a problem hiding this comment.
How about making the type of the port mapping command argument a tuple type that holds a HashMap, and do this parsing and verification in the FromStr implementation for that type?
There was a problem hiding this comment.
I am not entirely sure this is possible within clap (see this discussion) - I think this check has to be done in ReversePortForwarder
There was a problem hiding this comment.
Ok so we can use a Vec, but the verification that there are no duplicates should happen early (before creating an agent etc.), and the error message should not say "This is a bug". We could accept a single delimited list (80:80;8080:8080) and parse it into a hashmap with FromStr, but apparently we want to copy the ssh interface, so we will have to live with verification after parsing.
| if mappings.contains_key(&mapping.remote) { | ||
| // two mappings shared a key thus keys were not unique | ||
| return Err(PortForwardError::ReversePortMapSetupError(mapping.remote)); | ||
| } | ||
| mappings.insert(mapping.remote, mapping.local); |
There was a problem hiding this comment.
nitpick
| if mappings.contains_key(&mapping.remote) { | |
| // two mappings shared a key thus keys were not unique | |
| return Err(PortForwardError::ReversePortMapSetupError(mapping.remote)); | |
| } | |
| mappings.insert(mapping.remote, mapping.local); | |
| if mappings.insert(mapping.remote, mapping.local).is_some() { | |
| // two mappings shared a key thus keys were not unique | |
| return Err(PortForwardError::ReversePortMapSetupError(mapping.remote)); | |
| } |
Closes #2609