Support expanded syntax of ports in `docker stack deploy` #30476

yongtang · 2017-01-26T17:55:35Z

- What I did

NOTE: The description has been updated based on #30476 (comment)

This fix tries to address the issue in #30447 where it was not possible to specify mode=host for ports in compose.

- How I did it

This fix addresses the issue by adding support for expanded ports syntax:

version: "3.1"
services:
  nginx:
    image: nginx
    ports:
      - mode: host
        target: 80
        published: 9005

- How to verify it

Several unit test has been added to cover the changes.

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

This fix fixes #30447.

This fix is related to #29961/#30103.

Signed-off-by: Yong Tang yong.tang.github@outlook.com

dnephin · 2017-01-26T19:21:45Z

Thanks for looking into this. Unfortunately I don't think this is the right approach.

I agree we should support a "long form" syntax for this field, but I don't think it should be the csv format. The csv format is a compromise because of limitations on the CLI. For the Compose format we should use a mapping structure with keys and values:

ports:
  - mode: host
    target: 80
    published: 9005

I'm about to push a PR that does this for volumes, it should be up later today.

Another related issue is #29193

yongtang · 2017-01-30T03:31:08Z

Thanks @dnephin for the insight and help. I will spend some time to try to get the new mapping structure working. Still trying to learn the mechanism of compose schema though I think I am close.

dnephin · 2017-01-30T15:26:27Z

@yongtang you might be interested in #30521 which is some cleanup of the compose loader, and https://github.com/dnephin/docker/tree/add-expanded-mount-format-to-stack-deploy which is the branch for expanded mount format (that I thought I would have finished last week).

thaJeztah · 2017-01-30T21:33:02Z

Yes, I agree with @dnephin here; @yongtang should we close this PR, or were you planning to update this one with the new approach?

yongtang · 2017-01-30T21:39:31Z

Thanks @dnephin and @thaJeztah for the help, and pointing me to #30521. Very useful to understand the work flow of compose schema. I will update the PR with the new mapping structure as discussed.

yongtang · 2017-01-31T22:13:43Z

@dnephin @thaJeztah @vdemeester The PR has been updated with the support for expanded port syntax. I only changed Compose schema for 3.1. Not entirely sure if this is the expected behavior. So please take a look and let me know if there are any issues.

dnephin

Design LGTM,

few comments

dnephin · 2017-02-01T15:27:08Z

cli/compose/schema/data/config_schema_v3.1.json

+                  "target": {"type": "integer"},
+                  "published": {"type": "integer"},
+                  "protocol": {"type": "string"}
+                }


I'm thinking name should be a field as well. I'm not sure why it's not supported in the cli.

@dnephin The Name is not used in the swarmkit as the moment. I added the name to the field. It is not actively but we could add the usage in the future.

Oh, I guess if it's not being used by swarmkit, we shouldn't add it. It's strange that it's in the API types if it's not used.

@dnephin The PR has been updated with name field removed. Please take a look.

dnephin · 2017-02-01T15:29:19Z

cli/compose/loader/loader_test.go

+	}
+
+	assert.Equal(t, 1, len(config.Services))
+	assert.Equal(t, len(expected), len(config.Services[0].Ports))


I would remove this assertion. It's covered by the next line, and it'll be easier to see the failure if the error shows the full diff, instead of just x != y

Thanks. Done.

dnephin · 2017-02-01T15:31:48Z

cli/compose/schema/data/config_schema_v3.1.json

+              {"type": "string"},
+              {
+                "type": "object",
+                "properties": {


This should probably also have additionalProperties: false

Thanks @dnephin. Done.

dnephin · 2017-02-01T15:32:23Z

cli/compose/schema/data/config_schema_v3.1.json

+                  "protocol": {"type": "string"}
+                }
+              }
+            ],
            "format": "ports"


I think you might need to move format: ports into the type: number, and type: string cases

Thanks @dnephin. Done.

dnephin · 2017-02-01T15:35:05Z

cli/compose/loader/loader.go

+				}
+				for _, vv := range v {
+					ports = append(ports, vv)
+				}


I think you can do this as ports = append(ports, v...) ?

(same above)

Thanks @dnephin. There was an compilation error when appending directly:

cli/compose/loader/loader.go:502: cannot use v (type []"github.com/docker/docker/cli/compose/types".ServicePortConfig) as type []interface {} in append

Now the toServicePortConfigs(value string) has be changed to return []interface{} (instead of []types.ServicePortConfig) so it is possible to append directly with the update.

yongtang · 2017-02-01T21:01:07Z

Thanks @dnephin for the review. The PR has been updated. Please take a look and let me know if there are any additional issues.

dnephin

LGTM

Thanks!

vdemeester

LGTM 🐸
@yongtang needs a rebase 😅

This commit adds expanded port syntax to Compose schema and types so that it is possible to have ``` ports: - mode: host target: 80 published: 9005 ``` Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

This commit adds support for expanded ports in Compose loader, and add several unit tests for loading expanded port format. Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

yongtang · 2017-02-07T17:17:30Z

Thanks @vdemeester The PR has been rebased.

thaJeztah · 2017-02-11T19:13:51Z

All green and two LGTMS

thaJeztah · 2017-02-11T19:14:41Z

@dnephin do the changes to the compose format have to be synced back to the docker-compose repository? Also could you help getting these changes into the docs?

dnephin · 2017-02-16T17:02:52Z

Yes, and I guess so.

friism · 2017-03-10T23:13:12Z

@johnstep if you have a second and if you haven't already, could you test this on Windows? @brandonroyal took a quick look and couldn't get it working.

cc @icecrime

sixeyed · 2017-03-13T22:57:33Z

@friism this is working for Windows in 17.04. Sample compose file:

version: "3.1"
services:
  iis:
    image: microsoft/iis
    ports:
      - mode: host
        target: 80
        published: 80

Runs with docker stack deploy --compose-file docker-compose.yml iis.

cc @johnstep.

friism · 2017-03-13T23:02:52Z

@johnstep hey, @sixeyed tested this on Windows with 17.04 (master) server and client and it worked.

@dnephin one funny thing we found (sorry if you already thought of this) is that the error message for pre 17.04 clients is really bad (this is using the compose file that @sixeyed posted above):

services.iis.ports.0 must be a string or number

dnephin · 2017-03-14T02:18:44Z

What's bad about that error message? It seems to provide the necessary information. It has the path to offending value, and the problem with the value.

friism · 2017-03-14T02:25:50Z

@dnephin it seems not good if the actual solution is "upgrade to Docker 17.04"

…ntax Support expanded syntax of ports in `docker stack deploy`

raarts · 2017-05-23T21:14:41Z

Just a note for whoever reads this:

Using the long port format requires version: "3.2", not 3.1.

karthick0070 · 2017-09-14T18:49:48Z

hi,

@sixeyed I have tried this below stack file but i can't connect from out side of the host. Can you please suggest me

version: "3.2"
services:
 web:
  image: iiswithdb
  ports:
   - 8080:8080
   - mode: host
  deploy:
   replicas: 3

Below is this Docker ps out put. I think its publishing to a different port

CONTAINER ID        IMAGE                                      COMMAND                   CREATED             STATUS                  PORTS                                    NAMES
a24cee0efb74        iiswithdb:latest                           "C:\\ServiceMonitor..."   4 minutes ago       Up Less than a second   80/tcp, 8080/tcp, 0.0.0.0:37799->0/tcp   test_web.2.mnbq86t6bgifkkuycww0udqp5

thaJeztah · 2017-09-15T11:01:59Z

@karthick0070 that format is incorrect; either use the "shorthand" format (just 8080:8080) or the long format (where you specify target: and published:);

version: "3.2"
services:
 web:
  image: iiswithdb
  ports:
    - mode: host
      target: 8080
      published: 8080
  deploy:
   replicas: 3

karthick0070 · 2017-09-18T07:28:44Z

@thaJeztah Thanks for your help... it is working fine as excepted.

DianaDai · 2017-12-18T07:59:29Z

I still has this program "services.web.ports.0 must be a string or number" when run this
version: "3.2"
services:
web:
image: iiswithdb
ports:

8080:8080
mode: host
deploy:
replicas: 3
my docker version is
Docker version 17.10.0-ce, build f4ffd25

how can I solve this program @friism @sixeyed @thaJeztah

jravenger4 · 2019-01-13T14:27:57Z

I still has this program "services.web.ports.0 must be a string or number" when run this
version: "3.2"
services:
web:
image: iiswithdb
ports:

8080:8080

mode: host
deploy:
replicas: 3
my docker version is
Docker version 17.10.0-ce, build f4ffd25

how can I solve this program @friism @sixeyed @thaJeztah

Same. I'm using Docker EE for Windows and Windows Docker Container using Windows Server 2016 LTSC, as far as I know there is no routing mesh support yet.

Is it possible to use publish: 81, 82, 83 if your replicas is 3?

yongtang mentioned this pull request Jan 26, 2017

docker stack deploy not compatible with Publish mode=host #30447

Closed

GordonTheTurtle added the status/0-triage label Jan 26, 2017

vdemeester added status/1-design-review and removed status/0-triage labels Jan 27, 2017

yongtang force-pushed the 30447-port-config-long-syntax branch from f0a6048 to 6e7db22 Compare January 31, 2017 20:56

yongtang changed the title ~~Support long syntax of ports in docker stack deploy~~ Support expanded syntax of ports in docker stack deploy Jan 31, 2017

yongtang force-pushed the 30447-port-config-long-syntax branch from 6e7db22 to 3ab110f Compare January 31, 2017 21:49

dnephin reviewed Feb 1, 2017

View reviewed changes

yongtang force-pushed the 30447-port-config-long-syntax branch from 3ab110f to 3c833e4 Compare February 1, 2017 20:53

yongtang force-pushed the 30447-port-config-long-syntax branch from 3c833e4 to db200f6 Compare February 1, 2017 21:39

dnephin approved these changes Feb 1, 2017

View reviewed changes

thaJeztah added impact/changelog impact/documentation status/2-code-review and removed status/1-design-review labels Feb 2, 2017

vdemeester self-requested a review February 2, 2017 15:21

vdemeester approved these changes Feb 7, 2017

View reviewed changes

yongtang added 2 commits February 7, 2017 09:12

Add expanded port syntax to Compose schema and types.

60c1eaf

This commit adds expanded port syntax to Compose schema and types so that it is possible to have ``` ports: - mode: host target: 80 published: 9005 ``` Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

Support expanded ports in Compose loader

f07a28a

This commit adds support for expanded ports in Compose loader, and add several unit tests for loading expanded port format. Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

yongtang force-pushed the 30447-port-config-long-syntax branch from db200f6 to f07a28a Compare February 7, 2017 17:16

GordonTheTurtle assigned crosbymichael Feb 10, 2017

thaJeztah merged commit e5066ef into moby:master Feb 11, 2017

GordonTheTurtle added this to the 1.14.0 milestone Feb 11, 2017

yongtang deleted the 30447-port-config-long-syntax branch February 11, 2017 19:32

dnephin mentioned this pull request Feb 16, 2017

Support expanded port syntax docker/compose#4502

Closed

hcguersoy mentioned this pull request Feb 17, 2017

docker and ufw serious problems #4737

Closed

cirocosta mentioned this pull request Feb 27, 2017

Support expanded ports and mount format in the the compose v3 format #29193

Closed

dnephin mentioned this pull request Mar 13, 2017

Add ports and volumes long form syntax to compose-file reference docker/docs#2283

Merged

dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017

Merge pull request moby#30476 from yongtang/30447-port-config-long-sy…

ad5f808

…ntax Support expanded syntax of ports in `docker stack deploy`

Support expanded syntax of ports in docker stack deploy #30476

Support expanded syntax of ports in docker stack deploy #30476

Conversation

yongtang commented Jan 26, 2017 • edited Loading

dnephin commented Jan 26, 2017

yongtang commented Jan 30, 2017

dnephin commented Jan 30, 2017

thaJeztah commented Jan 30, 2017

yongtang commented Jan 30, 2017

yongtang commented Jan 31, 2017

dnephin left a 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

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

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

yongtang commented Feb 1, 2017

dnephin left a comment

Choose a reason for hiding this comment

vdemeester left a comment

Choose a reason for hiding this comment

yongtang commented Feb 7, 2017

thaJeztah commented Feb 11, 2017

thaJeztah commented Feb 11, 2017

dnephin commented Feb 16, 2017

friism commented Mar 10, 2017

sixeyed commented Mar 13, 2017

friism commented Mar 13, 2017

dnephin commented Mar 14, 2017

friism commented Mar 14, 2017

raarts commented May 23, 2017

karthick0070 commented Sep 14, 2017 • edited Loading

thaJeztah commented Sep 15, 2017

karthick0070 commented Sep 18, 2017

DianaDai commented Dec 18, 2017 • edited Loading

jravenger4 commented Jan 13, 2019

Support expanded syntax of ports in `docker stack deploy` #30476

Support expanded syntax of ports in `docker stack deploy` #30476

yongtang commented Jan 26, 2017 •

edited

Loading

karthick0070 commented Sep 14, 2017 •

edited

Loading

DianaDai commented Dec 18, 2017 •

edited

Loading