ThePorgs / Exegol

Fully featured and community-driven hacking environment
https://exegol.readthedocs.io/
GNU General Public License v3.0
1.95k stars 191 forks source link

FEATURE - Add range port #203

Closed LOLOLEKIK closed 9 months ago

LOLOLEKIK commented 9 months ago

Description

Code modified to expose port ranges rather than just ports.

In fact, in environments where it's not possible to have a bridged network, it's a pain to expose several ports at once.

The port exposure feature has been changed to allow commands of this kind:

exegol start dev -p 9000-9010:9000-9010 -p 80 -p 802-805:udp

Point of attention

The feature has been extensively tested, but I have two points to make.

  1. Creation timeout

First of all, if a port range is too large, it is possible to be timeout by exegol.

More precisely, if the machine takes longer than 60 seconds to create, exegol stops. I didn't touch this behavior to avoid blocking the user. In this case, the long creation time is not linked to exegol but to docker.

It's starting to be difficult to create a container with more than 2500 published ports.

Once again, this is specific to docker and exegol would behave in the same way as if you used the --ports option 2500 times.

ref : https://forums.docker.com/t/i-have-a-docker-container-that-needs-to-expose-10-000-ports/96048/3 https://github.com/moby/moby/issues/14288

  1. Display port

Similar effect on port display. Ports are displayed in sequence and not by range. For standard use, there's no visual imapact, but for exposing hundreds of ports, this has an impact on the visibility of the creation summary.

I did some local tests to manage the display a little better.

But I find the current interface more reassuring (ports are clearly visible).

To sum up, the modification also meant a significant increase in the complexity of the code and a less clear interface.

LOLOLEKIK commented 9 months ago

Here's a screenshot of the acutel result. As I said, I found it less clear to have the port range in the summary. For standard use, the output remains readable as you can see. If hundreds of ports are open, the terminal will just scroll.

image
ShutdownRepo commented 9 months ago

I love the addition, thank you @LOLOLEKIK :muscle: @Dramelac will handle the review of this Pull Request, but please don't forget to run some fuzzy testing to make sure that messed up entries still get handled. For instance :point_down:

# left array length < right array length
-p 1000-1010:2000-2999  

 # left int vs. right array
-p 3000:3000-3005 

# port out of bound
-p 99999 

# incomplete array 
-p 4000-  

# would this work?
-p  5000,5001,5002,5003-5005,5010-5015  
LOLOLEKIK commented 9 months ago

Thanks ! 🙏

I've already done these tests (except the last one) and everything works.

I've replayed all the tests and I'm posting the screenshots :)

-p string

image

# left array length < right array length
-p 1000-1010:2000-2999  

image

# left array length > right array length
-p 2020-2035:1000-1010  

image

 # left int vs. right array
-p 3000:3000-3005 

image

 # left array vs. right int
-p 3000-3005:3000 

image

# port out of bound
-p 99999 

image

# right incomplete array 
-p 4000-  

image

# left incomplete array 
-p -4000

image

# random test
-p 3000-3025:4000:1000-1050

image

# would this work?
-p  5000,5001,5002,5003-5005,5010-5015  

image

As you can see, the last syntax doesn't work. It doesn't work on the current version of exegol either, I just didn't modify this behavior.

image

Of course, the dev container is destroyed before each test.

ShutdownRepo commented 9 months ago

@LOLOLEKIK Awesome, nice job!

  1. In the first screenshot, we see the syntax. Imo, it should be printed without the automatic highlighting/coloring. And it would probably be best to have the syntax helper string be defined somewhere and used in the help as well as the error message, to avoid having one string different than another
  2. Same thing when the ports are printed back, let's remove the color there imo
  3. Maybe it would be nice to print a warning when you handle implicit syntax. For example, with -p 3001-3005:3000 it would be relevant to raise a warning like Ports sharing configuration could be wrong (3001-3005:3000). Assuming the following config: 3001-3005:3000-3004
  4. Also, fyi, you can specify the image name in the command line to avoid the interactive menu: exegol start dev full -p 1234
  5. Finally, @Dramelac what do you think about supporting the last syntax? I think it would be nice to have (e.g. 5000,5001,5002,5003-5005,5010-5015)
LOLOLEKIK commented 9 months ago

I've pushed some changes that display a message when the -p option is used in a non-standard way.

I've also "streamlined" the use of port sharing so that the following options give the same behaviors (see screenshot)

-p 8000-8005:8010
-p 8010:8000-8005

image

Note that it's exactly for this clarity that I think it's a good idea to keep the current ouput in the container summary.

For points 1 and 2, I'm sorry but I don't think I understood everything. @Dramelac asked me to add the [green] flag on the output. Is there any question of removing this flag? I've tried to understand how the logger works to display raw data but I have the impression that my modifications will impact all exegol outputs.

Maybe I'm missing something obvious...

As for point 5, it's because it wasn't possible to do that that motivated me to make this PR.

(thanks for the tips :) )

Dramelac commented 9 months ago

I've made some changes. Range are now dynamically detected for greater readability. image

Finally, @Dramelac what do you think about supporting the last syntax? I think it would be nice to have (e.g. 5000,5001,5002,5003-5005,5010-5015)

In the current version, multiple config must be set with multiple -p, for example: -p 5000-5010 -p 8089. @ShutdownRepo Not a fan to introduce more complexity into the already long pattern...