Fix docker ps --filter publish by jecepeda · Pull Request #40442 · moby/moby

jecepeda · 2020-02-01T18:11:32Z

- What I did
Fixed the filter on published ports when using the command docker ps --filter publish

- How I did it

I checked on PublicPort value inside container.Ports instead of types.Snapshot.PublishedPorts.

I also did the same on --filter expose. I checked on PrivatePort instead on types.Snapshot.ExposedPorts

- How to verify it
I followed the same steps like #40405

First, create some test containers:

docker run -d --name test_port_1080 -p 1080:80 nginx:alpine
docker run -d --name test_port_1090 -p 1090:80 nginx:alpine
docker run -d --name test_port_80_random -p 80 nginx:alpine
docker run -d --name test_port_all_random -P nginx:alpine


docker ps --filter name=test_

CONTAINER ID        IMAGE               COMMAND                  CREATED             STATUS              PORTS                   NAMES
3ce8232cdd04        nginx:alpine        "nginx -g 'daemon of…"   6 seconds ago       Up 6 seconds        0.0.0.0:32775->80/tcp   test_port_all_random
7f8fad7a0eb9        nginx:alpine        "nginx -g 'daemon of…"   7 seconds ago       Up 6 seconds        0.0.0.0:32774->80/tcp   test_port_80_random
3870ccc2f8f7        nginx:alpine        "nginx -g 'daemon of…"   7 seconds ago       Up 6 seconds        0.0.0.0:1090->80/tcp    test_port_1090
aa565211513e        nginx:alpine        "nginx -g 'daemon of…"   8 seconds ago       Up 7 seconds        0.0.0.0:1080->80/tcp    test_port_1080
c4a1c6e0e99b        nginx:alpine        "nginx -g 'daemon of…"   8 seconds ago       Up 7 seconds        80/tcp                  test_no_ports

Now, if you filter on publish ports:


docker ps --filter name=test_ --filter publish=1090
CONTAINER ID        IMAGE               COMMAND                  CREATED             STATUS              PORTS                 NAME
3870ccc2f8f7        nginx:alpine        "nginx -g 'daemon of…"   21 seconds ago      Up 20 seconds       0.0.0.0:1090->80/tcp    test_port_1090

We can find that the filter is applied successfully

- Description for the changelog
fix filter on docker ps --filter publish. Also change docker ps --filter expose

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

Fixes #40405

jecepeda · 2020-02-01T23:42:20Z

Can't complete tests on my machine because of #40237

thaJeztah · 2020-02-03T12:52:48Z

Can't complete tests on my machine because of #40237

oh! Hm, I should check if I run into the same issue on my setup.

I usually use the TEST_FILTER option to only run some integration tests (so that I don't have to wait for the whole test-suite to complete on my local machine); something like;

make DOCKER_GRAPHDRIVER=vfs TEST_FILTER='TestBuildPreserveXattrs' test-integration

Thanks for working on this! I'll have a look at the changes you made so far. I'll probably ask you to squash (some of) your commits before we merge

jecepeda · 2020-02-03T18:19:27Z

Can't complete tests on my machine because of #40237

oh! Hm, I should check if I run into the same issue on my setup.

I usually use the TEST_FILTER option to only run some integration tests (so that I don't have to wait for the whole test-suite to complete on my local machine); something like;
make DOCKER_GRAPHDRIVER=vfs TEST_FILTER='TestBuildPreserveXattrs' test-integration
Thanks for working on this! I'll have a look at the changes you made so far. I'll probably ask you to squash (some of) your commits before we merge

Thanks for the tip! Nevertheless, I was able to execute the suite and TestPsListContainersFilterPorts on my Linux machine.

The test suite seemed to work, although the pipeline failed. If I can work on improving something more tell me.

thaJeztah · 2020-02-03T13:12:48Z

daemon/list.go

Given that port.PublicPort and port.PrivatePort are already an integer; wondering if we should skip the nat.NewPort (which parses the port string back into an integer), and instead just;

publishedPort = nat.Port(fmt.Sprintf("%d/%s", port.PublicPort, port.Type)) exposedPort = nat.Port(fmt.Sprintf("%d/%s", port.PrivatePort, port.Type))

started reviewing, but got sidetracked ^ let me post the above at least for now (I'll try having a closer look tomorrow)

jecepeda · 2020-04-17T11:04:24Z

Hi @thaJeztah do I need to do something more on this PR?

AkihiroSuda · 2020-05-11T01:28:30Z

@thaJeztah PTAL

thaJeztah

thanks! sorry for the delay (again)

code LGTM. I see it needs a "squash" (there's seven commits in the PR, looks like we can squash them to a single one).

Let me do a quick rebase and squash of those commits, then we can merge once CI has run

thaJeztah · 2020-05-15T07:18:32Z

daemon/list.go

looks like we don't need the else here (as we'll exit the loop if the publishedPort above matches), in which case we could even omit the publishedPort / exposedPort variables.

I might be doing some cleaning up of this code in general, so no need to change

- Add tests to ensure it's working - Rename variables for better clarification - Fix validation test - Remove wrong filter assertion based on publish filter - Change port on test Signed-off-by: Jaime Cepeda <[email protected]>

jecepeda · 2020-05-15T09:17:35Z

Thanks @thaJeztah ! if you need some help on my side please tell me

thaJeztah · 2020-05-15T10:02:27Z

I think we should be good to go (praying to the CI gods to give us "green") 👍

thaJeztah · 2020-05-15T10:19:45Z

All green! Here we go 🎉

thaJeztah · 2020-05-15T10:20:00Z

Thank you!

olafbuitelaar · 2020-12-15T18:34:41Z

i'm not sure, but i think this fix causes an issues with ports which are mapped directly on the host, e.g;

 ports:
      - target: 3306
        published: 3306
        protocol: tcp
        mode: host

on machines running docker 19.X i can do this with services deployed in host mode;

[root@kube-db-04 ~]# docker --version
Docker version 19.03.14, build 5eb3275d40
[root@kube-db-04 ~]#  docker ps --filter "expose=3306"
CONTAINER ID        IMAGE                                                            COMMAND                  CREATED             STATUS                 PORTS               NAMES
2eabd026d09d        docker-registry:5000/mariadb:10.5.8   "docker-entrypoint.s…"   13 days ago         Up 13 days (healthy)   3306/tcp            xxxx.1.grkve05cgvrs8xanz73ixnq27
31fe8066a722        docker-registry:5000/mariadb:10.5.8    "docker-entrypoint.s…"   13 days ago         Up 13 days (healthy)                       xxxx.11.r71lm6ixuxywvc020x4imjqjp

on hosts running docker 20.X i get no output, while i'm certain the port is exposed;

[root@kube-db-05 ~]# docker --version
Docker version 20.10.0, build 7287ab3
[root@kube-db-05 ~]# docker ps --filter "expose=3306"
CONTAINER ID   IMAGE     COMMAND   CREATED   STATUS    PORTS     NAMES

docker inspect ef07e8

 "HostConfig": {
            "Binds": null,
            "ContainerIDFile": "",
            "LogConfig": {
                "Type": "json-file",
                "Config": {}
            },
            "NetworkMode": "host",
            "PortBindings": {
                "3306/tcp": [
                    {
                        "HostIp": "",
                        "HostPort": "3306"
                    }
                ]
            },
}

jdkr0220 · 2021-03-17T21:39:35Z

I'm also seeing an issue. In version 19.03.x, filter on exposed/published ports work for running & stopped containers

$ sudo docker ps -a --filter publish=443
CONTAINER ID        IMAGE            COMMAND            CREATED             STATUS                  PORTS                                      NAMES
7c83b72fa778        xxx/xxx:1.0.53   "dotnet xxx.dll"   7 days ago          Up 7 days               0.0.0.0:80->80/tcp, 0.0.0.0:443->443/tcp   xxx-1.0.53
de2c5acec437        xxx/xxx:1.0.52   "dotnet xxx.dll"   3 weeks ago         Exited (0) 7 days ago                                              xxx-1.0.52
$ sudo docker --version
Docker version 19.03.13, build 4484c46d9d

Starting with docker version 20.10.x, only running containers are reported for the filter.

jecepeda requested a review from thaJeztah February 1, 2020 18:11

jecepeda force-pushed the docker-ps-filter-publish-fix branch from cf3076c to a5bb08a Compare February 1, 2020 23:31

thaJeztah reviewed Feb 3, 2020

View reviewed changes

jecepeda requested a review from thaJeztah February 5, 2020 16:57

thaJeztah added kind/bugfix PR's that fix bugs process/cherry-pick status/2-code-review labels Apr 20, 2020

thaJeztah approved these changes May 15, 2020

View reviewed changes

Fix filter on expose and publish

f48b7d6

- Add tests to ensure it's working - Rename variables for better clarification - Fix validation test - Remove wrong filter assertion based on publish filter - Change port on test Signed-off-by: Jaime Cepeda <[email protected]>

thaJeztah force-pushed the docker-ps-filter-publish-fix branch from 95136b7 to f48b7d6 Compare May 15, 2020 09:14

thaJeztah added status/4-merge and removed status/2-code-review labels May 15, 2020

thaJeztah merged commit 8593b3d into moby:master May 15, 2020

thaJeztah added this to the 20.03.0 milestone Jun 10, 2020

StoneDot mentioned this pull request Nov 12, 2021

Fix broken setup_with_port awslabs/dynein#28

Merged

thaJeztah removed the process/cherry-pick label Feb 18, 2022

Conversation

jecepeda commented Feb 1, 2020

Uh oh!

jecepeda commented Feb 1, 2020

Uh oh!

thaJeztah commented Feb 3, 2020

Uh oh!

jecepeda commented Feb 3, 2020

Uh oh!

thaJeztah Feb 3, 2020

Choose a reason for hiding this comment

Uh oh!

thaJeztah Feb 3, 2020

Choose a reason for hiding this comment

Uh oh!

jecepeda commented Apr 17, 2020

Uh oh!

AkihiroSuda commented May 11, 2020

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah May 15, 2020

Choose a reason for hiding this comment

Uh oh!

jecepeda commented May 15, 2020

Uh oh!

thaJeztah commented May 15, 2020

Uh oh!

thaJeztah commented May 15, 2020

Uh oh!

thaJeztah commented May 15, 2020

Uh oh!

olafbuitelaar commented Dec 15, 2020

Uh oh!

jdkr0220 commented Mar 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants