Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FEATURE - Add range port #203

Merged
merged 10 commits into from
Feb 24, 2024
Merged

Conversation

LOLOLEKIK
Copy link

@LOLOLEKIK LOLOLEKIK commented Feb 9, 2024

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
moby/moby#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.

Copy link
Member

@Dramelac Dramelac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello,
Thank you for your PR ! That would be a nice addition.

Here is a first review focused on your code changes.

Can you add a screenshot of the recap table when there is a lot of port shared ? Maybe we can add a post-process logic to the function getTextPorts() of ContainerConfig to merge those range ?

exegol/model/ContainerConfig.py Outdated Show resolved Hide resolved
exegol/model/ContainerConfig.py Outdated Show resolved Hide resolved
exegol/model/ContainerConfig.py Outdated Show resolved Hide resolved
add and remove try/except useful/inuseful and update error message.

Signed-off-by: LOLOLEKIK <[email protected]>
@LOLOLEKIK
Copy link
Author

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

Add [green][/green] in logger

Signed-off-by: LOLOLEKIK <[email protected]>
@ShutdownRepo ShutdownRepo added the enhancement New feature or request label Feb 20, 2024
@ShutdownRepo
Copy link
Member

I love the addition, thank you @LOLOLEKIK 💪
@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 👇

# 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
Copy link
Author

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
Copy link
Member

@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)

warning message added for non-standard mappings

Signed-off-by: LOLOLEKIK <[email protected]>
@LOLOLEKIK
Copy link
Author

LOLOLEKIK commented Feb 20, 2024

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 :) )

exegol/console/cli/actions/GenericParameters.py Outdated Show resolved Hide resolved
exegol/model/ContainerConfig.py Outdated Show resolved Hide resolved
exegol/model/ContainerConfig.py Outdated Show resolved Hide resolved
@Dramelac Dramelac changed the base branch from dev to feat/port-range February 24, 2024 20:29
@Dramelac
Copy link
Member

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...

Copy link
Member

@Dramelac Dramelac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the addition !
I'll merge this on a dedicated dev branch for now to merge it with additional internal network rework before release.

Copy link
Member

@ShutdownRepo ShutdownRepo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine by me
Didn't check everything in detail, but if something's wrong we'll see it rather quickly in dev

@Dramelac Dramelac merged commit eeede32 into ThePorgs:feat/port-range Feb 24, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants