Add port forwarding support to python-esiclient

[larsks]

This is a draft pull request, but I am looking for feedback

Add the following commands:

• openstack esi port forward create -- create one or more port forwardings
• openstack esi port forward delete -- delete one or more port forwardings
• openstack esi port forward purge -- remove all port forwardings associated with a floating ip

Create

The create and delete commands operate with forwarding specifications of the form <internal ip>:<internal port>:<external ip>[:<external port>][/<protocol>. For example, to create a port forward from floating ip address 100.100.100.100 port 22 to internal address 10.10.10.10 port 22:

openstack esi port forward create 10.10.10.10:22:100.100.100.100

This will:

• Find an existing port with the internal address 10.10.10.10, or
• If no port exists, (a) find an appropriate internal network for address 10.10.10.10 and create a new port
• Create the port forwarding from the floating ip to the internal ip

If the external port does not match the internal port, you can add an additional component to the specification, like this:

openstack esi port forward create 10.10.10.10:22:100.100.100.100:2222

This will create a port forwarding from 100.100.100.100 port 2222 to internal address 10.10.10.10 port 22.

You can create multiple forwardings in a single invocation:

openstack esi port forward create \
  10.10.10.10:22:100.100.100.100 \
  10.10.10.10:80:100.100.100.100 \
  10.10.10.10:443:100.100.100.100

Delete

The syntax for the delete command is similar:

openstack esi port forward delete 10.10.10.10:22:100.100.100.100:2222

Purge

The purge command removes all the port forwardings associated with a given floating ip address:

openstack esi port forward purge 100.100.100.100

[tzumainn]

I kinda hate the number of colons in something like 10.10.10.10:443:100.100.100.100, but I understand it's necessary if you want to be able to create a bunch of port forwardings in one ESI command. Is it necessary to do that though?

Will there be optional arguments for things like protocol or description?

If there are multiple internal networks that could match the given internal IP, would you error out?

I think you initially mentioned the idea of having a floating IP address automatically created for you; is that something you'd still want to do? Perhaps you could specify a network name instead of an IP, and have Neutron create the port for you?

[larsks]

I kinda hate the number of colons in something like 10.10.10.10:443:100.100.100.100...

That's similar to the syntax we use for docker/podman port forwarding (-p addr:external_port:internal_port) or ssh port forwarding (-L [local_address:]localhost_port:remote_host:remote_port).

Is it necessary to do that though?

I think it's very common to want to create multiple port forwards for baremetal hosts (e.g., the example from the description, in which we're forwarding ssh, http, and https).

Will there be optional arguments for things like protocol or description?

Protocol can already be specified in a fashion similar to docker/podman:

10.10.10.10:67:100.100.100.100/udp

I haven't been able to figure out a good way of setting a description per port-forward; I guess you specify a single description that would be used for all of them.

If there are multiple internal networks that could match the given internal IP, would you error out?

It does not currently error out. That would be a relatively easy change. You can specify an explicit internal network (and subnet) using --internal-ip-network and --internal-ip-subnet options.

I think you initially mentioned the idea of having a floating IP address automatically created for you; is that something you'd still want to do? Perhaps you could specify a network name instead of an IP, and have Neutron create the port for you?

There is code for this, but it doesn't work because it's not possible to create specific addresses. We would need to change the cli syntax to allow creating a random floating ip and using it. I had some concerns about that because we have pretty strict quotas on floating ips and I wasn't sure if that would interact badly with a command that automatically allocates floating ips. I'm not dead set against it, though. See below for some thoughts.

---

If we assume that the most common case will be creating a set of port forwardings targeting the same external ip/internal ip pair, we could maybe do something like this instead:

openstack esi port forward create [--from <floating ip>|--create-floating-ip] <internal ip> [<outside port>:]inside_port[/<protocol>] [...]

Where the earlier example would become:

openstack esi port forward create --from 100.100.100.100 10.10.10.10 22 80 443

Or, to create a new floating ip:

openstack esi port forward create --create-floating-ip 10.10.10.10 22 80 443

The purge command would remain unmodified, and the delete command would mirror the create command:

openstack esi port forward delete --from 100.100.100.100 10.10.10.10 22 80 443

There would obviously be no --create-floating-ip option in this case, and --from would be a required option (and perhaps --from * would delete any matching port forwardings).

[tzumainn]

I'm vaguely - but honestly, only vaguely - bothered by how different this is from typical OpenStack CLI syntax, in that the OpenStack CLI loves to use argument flags to make it clear what's being passed in. I'm not a huge fan of moving entirely towards docker port forwarding syntax, because there is still (currently) the expectation that the user uses other non-ESI OpenStack CLIs, and I do think it should feel "similar". I do understand that multiple port forwardings will often be desired; what I meant to push back on is the idea that those port forwardings have to be accomplished with a single command, rather than running the command multiple times with different parameters.

All that being said, I see you are compromising in your updated example; maybe we could close the gap with one more small modification?

openstack esi port forward create --port <protocol port designation 1>,<protocol port designation 2>,... --from <external network designation> --to <internal network designation>

The protocol port designation is pretty much exactly as you describe - X or X:Y with the option of /<protocol> if the user wants something other than tcp.

The internal network designation could either be an IP address or a Neutron port name, or a Neutron network name; in the latter case, a Neutron port is created for you (and if it happens that a port and network - or IP address? - have the same name, then we throw an error). The external network designation would either be an existing floating IP address you've created, or simply the name of the external floating IP network; in the latter case, the floating IP is created for you (and we throw an error if you're past your quota).

We allow users to pass in --description. If they want different values for that, then... they just have to run the command multiple times.

What do you think?

[larsks]

The internal network designation could either be an IP address or a Neutron port name, or a Neutron network name; in the latter case, a Neutron port is created for you (and if it happens that a port and network - or IP address? - have the same name, then we throw an error).

I think it only makes sense for the internal network designation to be either an ip address or an existing port uuid/name; I don't think it makes sense to create a forward to a random internal address.

My only other complaint is that by allowing either an ip address or a (port/network) name, we make option validation much more difficult (we can't reject an invalid ip address, for example, until after we contact the api to see if maybe it was a port name or id).

openstack esi port forward create --port <protocol port designation 1>,<protocol port designation 2>,... --from <external network designation> --to <internal network designation>

The thing that bothers me here is that both --to and --from are required. In the general scheme of things, this suggests they should be positional arguments rather than option arguments...

openstack esi port forward create [--port <portspec> [...]] <internal network designation> <external network designation>

...but that loses the contextual clues provided by the option names. Do you have a preference?

What do you think?

I think that overall it sounds like a reasonable plan.

[tzumainn]

The internal network designation could either be an IP address or a Neutron port name, or a Neutron network name; in the latter case, a Neutron port is created for you (and if it happens that a port and network - or IP address? - have the same name, then we throw an error).

I think it only makes sense for the internal network designation to be either an ip address or an existing port uuid/name; I don't think it makes sense to create a forward to a random internal address.

Yeah, that's fair. Part of me just wants it to be parallel to allowing someone to specify the external network (and have a floating IP created for you). Do you think someone would ever run this first to create a port with the appropriate port forwardings, and then attach it to a node?

In the back of my mind, I was also thinking that the "network designation" logic would be generalized into a function that something like openstack esi node network list could use instead of its current heap of optional arguments.

My only other complaint is that by allowing either an ip address or a (port/network) name, we make option validation much more difficult (we can't reject an invalid ip address, for example, until after we contact the api to see if maybe it was a port name or id).

Yep - that logic definitely becomes more difficult, but I think the alternative is a spaghetti selection of optional arguments which I think looks pretty convoluted to the end user. I think it may be better to err on the side of avoiding that.

openstack esi port forward create --port <protocol port designation 1>,<protocol port designation 2>,... --from <external network designation> --to <internal network designation>

The thing that bothers me here is that both --to and --from are required. In the general scheme of things, this suggests they should be positional arguments rather than option arguments...

openstack esi port forward create [--port <portspec> [...]] <internal network designation> <external network designation>

...but that loses the contextual clues provided by the option names. Do you have a preference?

Not a strong one - I know plenty of OpenStack CLI commands that actually require optional arguments, but I've always kind of hated that. I'm fine with either choice if the ports are part of the optional argument; that's what really made the command difficult to parse.

What do you think?

I think that overall it sounds like a reasonable plan.

[larsks]

I've refactored the code in line with our conversation; the following are now all valid commands:

Create

• Forward a port from an existing floating ip to the same port on an existing internal port:

  openstack esi port forward create -p 22 10.10.10.10 100.100.100.100
  

• Forward a port from a new floating ip to the same port on existing internal port:

  openstack esi port forward create -p 22 my_internal_port external
  

• Forward a port from an existing floating ip to a different port on an internal port:

  openstack esi port forward create -p 2222:22/tcp 10.10.10.10 100.100.100.100
  

• Forward multiple ports:

  openstack esi port forward create -p 22 -p 80 -p 443 10.10.10.10 100.100.100.100
  

Delete

The delete command mirrors the create command:

openstack esi port forward delete -p 22 10.10.10.10 100.100.100.100

Purge

The purge command is unchanged.

[tzumainn]

some initial comments!

[larsks]

There is now support for --description and the beginnings of a test suite.

[larsks]

    from enum import StrEnum
ImportError: cannot import name 'StrEnum' from 'enum' (/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/enum.py)

Man, Python 3.8 is just the worst.

Actually, StrEnum appears to be a Python-3.11-ism.

[larsks]

The tests are complete. I've squashed everything together. I think I've addressed all the review comments thus far. I was going to add some documentation to this pr, but doc/commands.md made me sad and I think I will tackle that in a future pull request.

[tzumainn]

Works pretty well! A few issues:

• when creating port forwardings, --description is actually required right now
• if you don't pass in a --port argument, you get an unintuitive 'NoneType' object is not iterable message
• the help text for port should maybe state that it can be specified multiple times, and give examples of allowed port formats
• are you sure that an arbitrary protocol name (beyond tcp/udp) shouldn't be allowed? regardless, the error message if you specify something like "foo" isn't the clearest (error: argument --port/-p: invalid from_spec value: '29:30/foo', as opposed to a message that mentions the protocol)
• if you run openstack esi port forwarding purge with no arguments, the command "works" with no message - I feel like it may be worth checking this case and asking the user to supply at least one floating IP

[larsks]

• when creating port forwardings, --description is actually required right now

Ugh, I'm calling that an API foul. Treating description=None different from description is not specified breaks a pretty common behavior. I've updated the code to handle this situation more gracefully.

• if you don't pass in a --port argument, you get an unintuitive 'NoneType' object is not iterable message

Yeah, that was ugly. I will fix that.

• the help text for port should maybe state that it can be specified multiple times, and give examples of allowed port formats

I'll update the help text and included a few examples.

• are you sure that an arbitrary protocol name (beyond tcp/udp) shouldn't be allowed? regardless, the error message if you specify something like "foo" isn't the clearest (error: argument --port/-p: invalid from_spec value: '29:30/foo', as opposed to a message that mentions the protocol)

Sigh. This is argparse being annoying. The ValueError exception in this case already has a perfectly good error message:

>>> Protocol('foo')
[...]
ValueError: 'foo' is not a valid Protocol
>>>

But argparse is ignoring the exception value. I'll see what the options are for changing how things are handled.

• if you run openstack esi port forwarding purge with no arguments, the command "works" with no message - I feel like it may be worth checking this case and asking the user to supply at least one floating IP

I'll add an error message in this case.

[larsks]

I think the recent changes resolve all the issues from your last comment.

[tzumainn]

Works great! Can you squash the commits?

[larsks]

The commits have been squashed.

[tzumainn]

Thanks!