add ability to publish range of ports #9097

brahmaroutu · 2014-11-11T19:41:43Z

Closes #8899
Signed-off-by: Srini Brahmaroutu srbrahma@us.ibm.com

thaJeztah · 2014-11-11T21:41:47Z

Does this feature handle a large range of ports well? Thinking of this because I read this issue; #9021

SvenDowideit · 2014-11-12T01:22:48Z

fredlf · 2014-11-13T02:32:02Z

docs/man/docker-create.1.md

                               format: ip:hostPort:containerPort | ip::containerPort | hostPort:containerPort | containerPort
+			       `hostPort` can be a range of ports (e.g. 49160-49170) and `containerPort` can be
+			       a range of ports (e.g. 3300-3310). When specifying ranges for both the number of 
+			       container ports in the range must match number host ports in the range. 


Needs some cleanup, simplification, and clarification. Suggest:
Both hostPort and containerPort can be specified as a range of ports. When specifying ranges for both, the number of container ports in the range must match the number of host ports in the range. E.g., a complete example.

@fredlf Please check the changes I made to the docs as suggested.

brahmaroutu · 2014-11-14T06:33:41Z

@thaJeztah publish should not affect the parsing of EXPOSE in the Dockerfile. Do you think of any tests that we should add?

thaJeztah · 2014-11-14T07:38:49Z

@brahmaroutu Just wondering if publishing a (silly?) range like 1-65000 would cause problems. (I saw the other issue was worked on and they added some test for a port range 1-5000)

I haven't properly looked into the other issue to see what the bottleneck was there, but when working with ranges, that testing some "extremes" may be a good thing. Obviously the problem might be what to test; if it works, or also if the performance of it doesn't regress.

These are just suggestions btw, I hope the maintainers can provide info if this is actually doable and useful,

fredlf · 2014-11-14T17:36:39Z

docs/sources/reference/commandline/cli.md

@@ -523,9 +523,12 @@ Creates a new container.
                                   'container:<name|id>': reuses another container network stack
                                   'host': use the host network stack inside the container.  Note: the host mode gives the container full access to local system services such as D-bus and is therefore considered insecure.
      -P, --publish-all=false    Publish all exposed ports to the host interfaces
-      -p, --publish=[]           Publish a container's port to the host
+      -p, --publish=[]           Publish a container's port, or a range of ports (e.g. -p 3300-3310), to the host


Need a comma after e.g., and code needs to be in backticks (`). Also below. Thanks.

fredlf · 2014-11-14T17:38:25Z

LGTM once the little formatting issue is fixed. Many thanks for your help.

brahmaroutu · 2014-11-14T20:07:41Z

@thaJeztah I have added more tests with range of ports.
@fredlf I made changes as per your comments

thaJeztah · 2014-11-14T20:28:28Z

Oh, thanks!

jessfraz · 2014-12-16T20:20:39Z

integration-cli/docker_cli_create_test.go

+
+	deleteAllContainers()
+
+	logDone("create - portrange")


can we do port range

What about this?

I do not understand, I am doing check on port range, is it what you meant?

No, it is just about rewording done message. port range publishing instead of portrange.

Yikes, I took @jfrazelle 's comment differently, as we had earlier port checking issues with other bug fixes, I really went ahead and implemented checking on entire port range(for loop). I got this fixed now. Sorry about that.

brahmaroutu · 2014-12-16T22:33:34Z

Rebased, changed code as per the comments. Thanks for the review.

jessfraz · 2014-12-16T22:43:34Z

ping @LK4D4

LK4D4 · 2014-12-17T22:07:36Z

integration-cli/docker_cli_run_test.go

+			t.Fatal("Port is not mapped for the port "+port, out)
+		}
+	}
+	if err := deleteContainer(id); err != nil {


I don't see why you need to check this in this test, we usually use defer deleteAllContainers().

added defer statement.

md5 · 2014-12-17T23:06:24Z

Forgive me if this is something that should be obvious from the diff, but what happens in the --publish-all case? Will Docker map port ranges verbatim for --publish-all or try to assign random ports for every port in the range like it does for single ports in the --publish-all case?

I have a similar question about what happens when someone says -p 20000-29999 without a target port range. Will that map those ports as 20,000 through 29,999 or map them to 10,000 random ports?

brahmaroutu · 2014-12-17T23:18:28Z

@md5 -P option works just as it is today, it will assign random ports at least in my testing 49xxx.

brahmaroutu · 2014-12-18T01:17:31Z

@LK4D4 I did a bit of formatting on the docs. Please let me know if I addressed all your concerns.

jessfraz · 2014-12-30T17:38:56Z

oh man sorry, this needs another rebase, please ping me after and i will be sure to review and hopefully get it through :)

brahmaroutu · 2015-01-02T18:35:25Z

np, I am happy to rebase. Please see that this gets merged.

Closes moby#8899 Signed-off-by: Srini Brahmaroutu <srbrahma@us.ibm.com>

jessfraz · 2015-01-05T19:24:39Z

LGTM

LK4D4 · 2015-01-05T22:05:44Z

LGTM

add ability to publish range of ports

md5 · 2015-01-05T22:09:56Z

🎆

thaJeztah · 2015-01-05T22:10:36Z

\o/

brahmaroutu force-pushed the publish_ports_8899 branch from 767f67e to 40364a9 Compare November 11, 2014 19:50

SvenDowideit added the /project/doc label Nov 12, 2014

fredlf reviewed Nov 13, 2014
View reviewed changes

brahmaroutu force-pushed the publish_ports_8899 branch from 40364a9 to 15c3054 Compare November 14, 2014 06:14

fredlf reviewed Nov 14, 2014
View reviewed changes

brahmaroutu force-pushed the publish_ports_8899 branch from 15c3054 to e4e4f16 Compare November 14, 2014 20:06

brahmaroutu force-pushed the publish_ports_8899 branch 3 times, most recently from d656fca to c8da86f Compare November 26, 2014 23:03

icecrime assigned jessfraz Dec 16, 2014

jessfraz mentioned this pull request Dec 16, 2014

Allow specifying --port-range-begin and --port-range-end #8412

Closed

jessfraz reviewed Dec 16, 2014
View reviewed changes

brahmaroutu force-pushed the publish_ports_8899 branch from c8da86f to 0518031 Compare December 16, 2014 22:32

brahmaroutu force-pushed the publish_ports_8899 branch 2 times, most recently from 617fce8 to b830793 Compare December 16, 2014 23:32

LK4D4 reviewed Dec 17, 2014
View reviewed changes

brahmaroutu force-pushed the publish_ports_8899 branch 2 times, most recently from 5892d4c to 9d62fc6 Compare December 18, 2014 01:17

brahmaroutu force-pushed the publish_ports_8899 branch 3 times, most recently from 399d269 to 4113586 Compare December 19, 2014 18:31

brahmaroutu force-pushed the publish_ports_8899 branch from 4113586 to 18e7312 Compare January 2, 2015 18:18

brahmaroutu force-pushed the publish_ports_8899 branch from 18e7312 to c2f6bd2 Compare January 2, 2015 21:41

add ability to publish range of ports

2338a9c

Closes moby#8899 Signed-off-by: Srini Brahmaroutu <srbrahma@us.ibm.com>

brahmaroutu force-pushed the publish_ports_8899 branch from c2f6bd2 to 2338a9c Compare January 2, 2015 23:21

jessfraz removed their assignment Jan 5, 2015

LK4D4 added a commit that referenced this pull request Jan 5, 2015

Merge pull request #9097 from brahmaroutu/publish_ports_8899

25f8c26

add ability to publish range of ports

LK4D4 merged commit 25f8c26 into moby:master Jan 5, 2015

brahmaroutu deleted the publish_ports_8899 branch January 23, 2015 22:48

SvenDowideit mentioned this pull request Jan 27, 2015

Post 1.4.1 docs update 2 #9739

Merged

thaJeztah mentioned this pull request Mar 5, 2015

user land proxy uses all RAM memory when exposing a big range of ports #11185

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add ability to publish range of ports #9097

add ability to publish range of ports #9097

brahmaroutu commented Nov 11, 2014

thaJeztah commented Nov 11, 2014

SvenDowideit commented Nov 12, 2014

fredlf Nov 13, 2014

brahmaroutu Nov 14, 2014

brahmaroutu commented Nov 14, 2014

thaJeztah commented Nov 14, 2014

fredlf Nov 14, 2014

fredlf commented Nov 14, 2014

brahmaroutu commented Nov 14, 2014

thaJeztah commented Nov 14, 2014

jessfraz Dec 16, 2014

LK4D4 Jan 2, 2015

brahmaroutu Jan 2, 2015

LK4D4 Jan 2, 2015

brahmaroutu Jan 2, 2015

brahmaroutu commented Dec 16, 2014

jessfraz commented Dec 16, 2014

LK4D4 Dec 17, 2014

brahmaroutu Dec 18, 2014

md5 commented Dec 17, 2014

brahmaroutu commented Dec 17, 2014

brahmaroutu commented Dec 18, 2014

jessfraz commented Dec 30, 2014

brahmaroutu commented Jan 2, 2015

jessfraz commented Jan 5, 2015

LK4D4 commented Jan 5, 2015

md5 commented Jan 5, 2015

thaJeztah commented Jan 5, 2015

add ability to publish range of ports #9097

add ability to publish range of ports #9097

Conversation

brahmaroutu commented Nov 11, 2014

thaJeztah commented Nov 11, 2014

SvenDowideit commented Nov 12, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brahmaroutu commented Nov 14, 2014

thaJeztah commented Nov 14, 2014

Choose a reason for hiding this comment

fredlf commented Nov 14, 2014

brahmaroutu commented Nov 14, 2014

thaJeztah commented Nov 14, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brahmaroutu commented Dec 16, 2014

jessfraz commented Dec 16, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

md5 commented Dec 17, 2014

brahmaroutu commented Dec 17, 2014

brahmaroutu commented Dec 18, 2014

jessfraz commented Dec 30, 2014

brahmaroutu commented Jan 2, 2015

jessfraz commented Jan 5, 2015

LK4D4 commented Jan 5, 2015

md5 commented Jan 5, 2015

thaJeztah commented Jan 5, 2015