Work on port ranges - change all to strings by larochef · Pull Request #566 · docker-java/docker-java

larochef · 2016-05-03T10:30:23Z

This is a fix for #556 allowing port ranges.

This change is

KostyaSha · 2016-05-03T14:34:27Z

src/main/java/com/github/dockerjava/api/model/Ports.java

         * @see Ports#bind(ExposedPort, Binding)
         * @see ExposedPort
         */
        public Binding(Integer hostPort) {


Change to string? to be consistent?

or require @Nonnull

The thing is there already is a constructor same signature, that's why I didn't change the Integer. I guess it would be safer to have static factories instead, and only keep the constructor wil everything. What do you think ?

KostyaSha · 2016-05-03T14:35:48Z

LGTM if api allow string, then it should be string.

marcuslinke · 2016-05-03T15:36:45Z

I'm not sure whether we should switch to String. Is it really needed? What about error handling when user passes invalid string? I guess we will get some unspecified error message from remote docker in this case which would be bad. Wouldn't it be better to hide single port / port ranges within the ExposedPort and Port.Binding classes via different constructors?

marcuslinke · 2016-05-03T16:26:21Z

Related documentation: https://docs.docker.com/engine/reference/run/#expose-incoming-ports

-p=[] : Publish a container᾿s port or a range of ports to the host
format: ip:hostPort:containerPort | ip::containerPort | hostPort:containerPort | containerPort
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, for example:
-p 1234-1236:1234-1236/tcp

When specifying a range for hostPort only, the
containerPort must not be a range. In this case the
container port is published somewhere within the
specified hostPort range. (e.g., -p 1234-1236:1234/tcp)

KostyaSha · 2016-05-03T17:42:40Z

@marcuslinke well, if you would bundle cli logic into direct API requests that would end in difficult code.
Until we make some porcelain cli imho it makes sense be close to docker API and provide end users ability to define calls. If docker-daemon accepts strings, then it should be possible to do it.

larochef · 2016-05-03T18:10:21Z

That's a good question, the string validation.

I've been wondering if we should have a regexp, something like [1-9][0-9]+(-[1-9][0-9]+)? but then, if some new format appears in the docker api in the future, then we'll have to change again. So should we validate or let docker do it instead ?

marcuslinke · 2016-05-03T20:51:19Z

I think the point here is whether docker deamon returns a meaningful error in the case of invalid string. @larochef Could you check that please? Maybe validation is realized in docker CLI or in remote API or not at all. If validation is done in remote API we can use string without any problem. If validation is implemented in docker CLI we need at least validation in docker-java for this.

larochef · 2016-05-04T07:31:20Z

I confirm that docker damon returns you a proper error.
I tried and got : com.github.dockerjava.api.exception.InternalServerErrorException: Invalid port specification: "pouet"

marcuslinke · 2016-05-04T07:44:22Z

@larochef Thanks! But what is with the more complex constraints:

When specifying ranges for both, the
number of container ports in the range must match the
number of host ports in the range, for example:
-p 1234-1236:1234-1236/tcp

When specifying a range for hostPort only, the
containerPort must not be a range. In this case the
container port is published somewhere within the
specified hostPort range. (e.g., -p 1234-1236:1234/tcp)

larochef · 2016-05-04T10:07:36Z

Ok, so after some investigation:

mapping a port from the container to a range on the host works as it is expected, I could have it work without any problem.
mapping a range to a range is translated by the cli to a list of mappings, taking them all 1 by 1.

Should we support ranges and do the same magic as docker-cli or let the user handle ranges manually ?

I don't think it's a good idea to let this too magic since each port binding requires some proxy magic by docker and consumes memory.

marcuslinke · 2016-05-04T17:42:18Z

OK, let summarize the possible cases for a mapping string from docker CLI perspective:

case 1234:1234: valid
case 1234-1236:1234: valid
case 1234-1236:1234-1236: valid, needs additional range validation and translation to single mappings
else: invalid

I think the third case (range to range) shouldn't be supported by docker-java. So we have to ensure that only the first and the second case will be possible with our API. Do you agree?

KostyaSha · 2016-05-04T19:47:25Z

src/test/java/com/github/dockerjava/netty/exec/StartContainerCmdExecTest.java

-import com.github.dockerjava.api.model.RestartPolicy;
-import com.github.dockerjava.api.model.Volume;
-import com.github.dockerjava.api.model.VolumesFrom;
+import com.github.dockerjava.api.model.*;


please don't replace imports with hashes.

larochef · 2016-05-05T20:00:57Z

Sorry for the imports, I'll check again my ide configuguration..

@marcuslinke I agree with that. I guess then we can leave the exposed port (the one from the container) as an int and have then the bound ports as strings since they can be ranges.

If we want all as Strings, I guess then we'll have to add a check in the corresponding serializer.

What do you think ?

marcuslinke · 2016-05-05T21:38:04Z

I guess then we can leave the exposed port (the one from the container) as an int and have then the bound ports as strings since they can be ranges.

@larochef Thats what I thought also. Letting ExposedPort be an int and Binding with both String and int constructors that internally stores it as String sounds good to me.

codecov-io · 2016-05-08T10:59:58Z

Current coverage is 30.51%

Merging #566 into master will increase coverage by +6.70%

2 files in ...dockerjava/api/model were modified. more
- Misses -1
- Partials -2
- Hits +3
3 files (not in diff) in ...erjava/netty/handler were modified. more
- Misses -19
- Partials -2
- Hits +21
17 files (not in diff) in ...ockerjava/netty/exec were modified. more
- Misses -35
- Hits +35
3 files (not in diff) in ...hub/dockerjava/netty were modified. more
- Misses -17
- Hits +17
5 files (not in diff) in ...kerjava/jaxrs/filter were modified. more
- Misses -23
- Hits +23
2 files (not in diff) in ...java/jaxrs/connector were modified. more
- Misses -49
- Hits +49
22 files (not in diff) in ...hub/dockerjava/jaxrs were modified. more
- Misses -66
- Hits +66
5 files (not in diff) in ...dockerjava/core/util were modified. more
- Misses -15
- Partials -6
- Hits +21
2 files (not in diff) in ...java/core/dockerfile were modified. more
- Misses -9
- Partials -4
- Hits +13
11 files (not in diff) in ...kerjava/core/command were modified. more
- Misses -27
- Partials -6
- Hits +33

@@             master       #566   diff @@
==========================================
  Files           296        296          
  Lines          6129       6129          
  Methods           0          0          
  Messages          0          0          
  Branches        532        532          
==========================================
+ Hits           1459       1870   +411   
+ Misses         4585       4259   -326   
+ Partials         85          0    -85

Powered by Codecov. Last updated by 1ab6e63...40dec22

larochef · 2016-05-08T11:24:42Z

Ok, I've re-worked the PR in that way, tell what you think of it now.

marcuslinke · 2016-05-08T20:31:52Z

@larochef I've polished it a bit. Closing this PR in favor of #575. WDYT?

larochef · 2016-05-08T20:38:31Z

Good for me :)

KostyaSha reviewed May 3, 2016
View reviewed changes

KostyaSha reviewed May 4, 2016
View reviewed changes

larochef force-pushed the master branch from 40dec22 to c2edd7c Compare May 8, 2016 10:55

larochef force-pushed the master branch 3 times, most recently from 7d0b988 to 3901348 Compare May 8, 2016 11:21

Work on port ranges - change all to strings

4bf2648

larochef force-pushed the master branch from 3901348 to 4bf2648 Compare May 8, 2016 11:23

marcuslinke closed this May 8, 2016

Conversation

larochef commented May 3, 2016 • edited by marcuslinke Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KostyaSha May 3, 2016

Choose a reason for hiding this comment

Uh oh!

KostyaSha May 3, 2016

Choose a reason for hiding this comment

Uh oh!

larochef May 3, 2016

Choose a reason for hiding this comment

Uh oh!

KostyaSha commented May 3, 2016

Uh oh!

marcuslinke commented May 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marcuslinke commented May 3, 2016

Uh oh!

KostyaSha commented May 3, 2016

Uh oh!

larochef commented May 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marcuslinke commented May 3, 2016

Uh oh!

larochef commented May 4, 2016

Uh oh!

marcuslinke commented May 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

larochef commented May 4, 2016

Uh oh!

marcuslinke commented May 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KostyaSha May 4, 2016

Choose a reason for hiding this comment

Uh oh!

larochef commented May 5, 2016

Uh oh!

marcuslinke commented May 5, 2016

Uh oh!

codecov-io commented May 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 30.51%

Uh oh!

larochef commented May 8, 2016

Uh oh!

marcuslinke commented May 8, 2016

Uh oh!

larochef commented May 8, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

larochef commented May 3, 2016 •

edited by marcuslinke

Loading

marcuslinke commented May 3, 2016 •

edited

Loading

larochef commented May 3, 2016 •

edited

Loading

marcuslinke commented May 4, 2016 •

edited

Loading

marcuslinke commented May 4, 2016 •

edited

Loading

codecov-io commented May 8, 2016 •

edited

Loading