Skip to content

cli/cli: use len() to check frontend ports in the port command#6063

Merged
thaJeztah merged 1 commit intodocker:masterfrom
giautm:patch-1
May 16, 2025
Merged

cli/cli: use len() to check frontend ports in the port command#6063
thaJeztah merged 1 commit intodocker:masterfrom
giautm:patch-1

Conversation

@giautm
Copy link
Contributor

@giautm giautm commented May 12, 2025

- What I did
I ran into the case that docker port return an empty string and exist successfully, checking the code base, I found this function check the frontend ports using nil check - which may incorrect in the case of empty slice.

So, this PR change to logic to use len(frontends) == 0 for check ports. If the slice is empty (nil or non-nil) it both return an error.

- How I did it
use len() to check instead of nil check

- How to verify it

- Human readable description for the release notes

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

This ensure the command won't print an empty output if the `frontends` port is nil

Signed-off-by: Giau. Tran Minh <hello@giautm.dev>
@codecov-commenter
Copy link

codecov-commenter commented May 12, 2025

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 55.15%. Comparing base (75a5713) to head (c409383).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6063   +/-   ##
=======================================
  Coverage   55.15%   55.15%           
=======================================
  Files         358      358           
  Lines       30023    30023           
=======================================
  Hits        16558    16558           
  Misses      12519    12519           
  Partials      946      946           
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@giautm
Copy link
Contributor Author

giautm commented May 12, 2025

@thaJeztah Sorry to bother you, please help me review this.

@giautm giautm changed the title cli/cli: use len() to check frontends ports in the port command cli/cli: use len() to check frontend ports in the port command May 13, 2025
@thaJeztah
Copy link
Member

Do you have more information about the problem you ran into? Looking at the code, it's expected to not return an error unless querying for a specific port;

if opts.port != "" {
port, proto, _ := strings.Cut(opts.port, "/")
if proto == "" {
proto = "tcp"
}
if _, err = strconv.ParseUint(port, 10, 16); err != nil {
return errors.Wrapf(err, "Error: invalid port (%s)", port)
}
frontends, exists := c.NetworkSettings.Ports[nat.Port(port+"/"+proto)]
if !exists || frontends == nil {
return errors.Errorf("Error: No public port '%s' published for %s", opts.port, opts.container)
}

The code-change overall looks harmless, but I'm curious in what situation you ran into where it didn't work as expected.

With the current version of the CLI;

docker run -d --name mycontainer nginx:alpine

docker port mycontainer
# no output and no error

docker port mycontainer 80
Error: No public port '80' published for mycontainer

@giautm
Copy link
Contributor Author

giautm commented May 13, 2025

The code-change overall looks harmless, but I'm curious in what situation you ran into where it didn't work as expected.

I've updated the reason in PR's description:

I ran into the case that docker port return an empty string and exist successfully, checking the code base, I found this function check the frontend ports using nil check - which may incorrect in the case of empty slice.
So, this PR change to logic to use len(frontends) == 0 for check ports. If the slice is empty (nil or non-nil) it both return an error.

I was unable to reproduce this issue yet, my willing guess it's related to OS / Docker Engine that unable to allocate a free port right after the container started.

fyi, I started the container with --publish 5432 to request a random port on the docker host to mapping to port 5432 of an Postgres instance.

And right after the container created (it prints the ID), the command docker port returns an empty string.

@thaJeztah thaJeztah added this to the 28.2.0 milestone May 16, 2025
@thaJeztah
Copy link
Member

Hm, gotcha! Was that on Docker Desktop or was it a docker installed on Linux? (On Docker Desktop, there's some additional steps involved in mapping ports, so wondering if that could play a role, resulting in a race condition).

@thaJeztah
Copy link
Member

Either way; the change in this PR looks fine to me, so let me approve and merge

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah thaJeztah merged commit 77fbbc3 into docker:master May 16, 2025
108 checks passed
@giautm
Copy link
Contributor Author

giautm commented May 16, 2025

Hm, gotcha! Was that on Docker Desktop or was it a docker installed on Linux? (On Docker Desktop, there's some additional steps involved in mapping ports, so wondering if that could play a role, resulting in a race condition).

That's Docker Desktop and I invoke the command inside WSL.

@thaJeztah
Copy link
Member

Thanks!

cc @akerouanton @robmry FYI (port-mapping)

@giautm giautm deleted the patch-1 branch May 16, 2025 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants