fix: Add used port check for publish flag #2190

vsiravar · 2023-04-18T00:21:40Z

Add check for used ports while allocating host port in -p/--publish to make it compatible with docker.

fahedouch · 2023-04-18T07:59:05Z

cmd/nerdctl/container_run_network_linux_test.go

@@ -1,3 +1,4 @@
+/*


this line is useless

fahedouch · 2023-04-19T16:30:18Z

pkg/portutil/portutil.go

+		if err != nil {
+			return nil, err
+		}
+		if usedPorts[startHostPort] {


why checking only startHostPort ? why not testing all the range ?

Good point, updated.

fahedouch · 2023-04-19T16:33:51Z

cmd/nerdctl/container_run_network_linux_test.go

@@ -302,6 +302,50 @@ func TestUniqueHostPortAssignement(t *testing.T) {
 	}
 }

+func TestHostPortAlreadyInUse(t *testing.T) {
+	if rootlessutil.IsRootless() {
+		t.Skip("Auto port assign is not supported rootless mode yet")


it is not an auto port assign here, we explicit set port, right ?

Updated, it was leftover from copying code from TestUniqueHostPortAssignement.

pkg/portutil/portutil.go

cmd/nerdctl/container_run_network_linux_test.go

junnplus · 2023-04-27T00:13:25Z

pkg/portutil/port_allocate_linux.go

+	if protocol == "tcp" || protocol == "udp" {
+		netprocData, err := procnet.ReadStatsFileData(protocol)
+		if err != nil {
+			return nil, err
+		}
+		netprocItems = append(netprocItems, procnet.Parse(netprocData)...)
+	}


It appears that the SCTP protocol is currently unsupported, therefore no further processing is required.

I could be wrong but from this line https://github.com/containerd/nerdctl/blob/main/pkg/portutil/portutil.go#L61, it seems like users could use sctp. There is also a test to check the parsing of sctp https://github.com/containerd/nerdctl/blob/main/pkg/portutil/portutil_test.go#L348.

I only checkd in https://github.com/containerd/nerdctl/blob/main/pkg/portutil/procnet/procnet_linux.go#L38 , Let me check it.

I carefully checked the code and did not find an implementation that supports the SCTP protocol. The line just supports parsing command line arguments.

I carefully checked the code and did not find an implementation that supports the SCTP protocol. The line just supports parsing command line arguments.

Additionally there a unit test that checks the parsing of "sctp" https://github.com/containerd/nerdctl/blob/main/pkg/portutil/portutil_test.go#L348, so without the protocol check below, the unit test fails.

if protocol == "tcp" || protocol == "udp" { netprocData, err := procnet.ReadStatsFileData(protocol) if err != nil { return nil, err } netprocItems = append(netprocItems, procnet.Parse(netprocData)...) }

As a side note, if sctp is not supported then why do we support parsing it in the ParseFlagP ?

Yep..., let it works

I quickly checked, it looks like SCTP is not supported but it is but it is not blocking either. IMHO, we have to add a comment here to highlight this point ( It's misleading without a comment)

junnplus · 2023-04-27T10:56:30Z

Test failed https://github.com/containerd/nerdctl/actions/runs/4814951537/jobs/8573169644?pr=2190#step:6:982

time="2023-04-27T01:30:38Z" level=fatal msg="failed to load networking flags: bind for :8080 failed: port is already allocated"

vsiravar · 2023-04-28T23:39:38Z

Test failed https://github.com/containerd/nerdctl/actions/runs/4814951537/jobs/8573169644?pr=2190#step:6:982
time="2023-04-27T01:30:38Z" level=fatal msg="failed to load networking flags: bind for :8080 failed: port is already allocated"

This is a test seems is testing TestComposeUpBuild in compose up , not sure if this is related to my change or a flaky test.

junnplus · 2023-05-01T01:06:51Z

Can you test this case locally? It seems different from what was expected?

$ sudo nerdctl run -p 8080:80 --name=test -d nginx
7f4a2be3bd30eba9d9b5c45ea83213af9728fa456bd3b23783a3352282afc078
$ sudo nerdctl stop test
test
$ sudo nerdctl run -p 8080:80 -d nginx
FATA[0000] failed to load networking flags: bind for :8080 failed: port is already allocated

fahedouch · 2023-05-01T20:46:45Z

pkg/portutil/port_allocate_linux.go

@@ -39,27 +39,60 @@ func filter(ss []procnet.NetworkDetail, filterFunc func(detail procnet.NetworkDe
 }

 func portAllocate(protocol string, ip string, count uint64) (uint64, uint64, error) {
-	netprocData, err := procnet.ReadStatsFileData(protocol)
+	usedPort, err := getUsedPorts(ip, protocol)
+


this line is useless

I don't quite understand this point? When the user does not specify the host port, we still want to bind an unused port from the host to the container port.

I mean the empty line

My bad :(, updated.

fahedouch · 2023-05-01T22:17:18Z

pkg/portutil/port_allocate_linux.go

+	if protocol == "tcp" || protocol == "udp" {
+		netprocData, err := procnet.ReadStatsFileData(protocol)
+		if err != nil {
+			return nil, err
+		}
+		netprocItems = append(netprocItems, procnet.Parse(netprocData)...)
+	}


I quickly checked, it looks like SCTP is not supported but it is but it is not blocking either. IMHO, we have to add a comment here to highlight this point ( It's misleading without a comment)

fahedouch · 2023-05-01T22:23:54Z

cmd/nerdctl/container_run_network_linux_test.go

+		},
+		{
+			hostPort:      "5000",
+			containerPort: "80/udp",


may be adding a case for SCTP to controller support/behavior of SCTP

fahedouch · 2023-05-01T22:38:11Z

Test failed https://github.com/containerd/nerdctl/actions/runs/4814951537/jobs/8573169644?pr=2190#step:6:982
time="2023-04-27T01:30:38Z" level=fatal msg="failed to load networking flags: bind for :8080 failed: port is already allocated"
This is a test seems is testing TestComposeUpBuild in compose up , not sure if this is related to my change or a flaky test.

Not related. It is a flaky test

AkihiroSuda · 2023-05-18T01:28:00Z

@vsiravar Could you confirm this?
#2190 (comment)

vsiravar · 2023-05-18T18:13:08Z

@vsiravar Could you confirm this? #2190 (comment)

Sorry I have been a bit busy. I will have all the comments addressed this weekend.

Can you test this case locally? It seems different from what was expected?

$ sudo nerdctl run -p 8080:80 --name=test -d nginx
7f4a2be3bd30eba9d9b5c45ea83213af9728fa456bd3b23783a3352282afc078
$ sudo nerdctl stop test
test
$ sudo nerdctl run -p 8080:80 -d nginx
FATA[0000] failed to load networking flags: bind for :8080 failed: port is already allocated

I think this is because nerdctl stop does not clean up the network unlike nerdctl remove . I can make a follow up PR to fix this, if it's the behavior we want. cc @junnplus

Edit: Tagged Jun for feeback.

fahedouch · 2023-05-29T19:49:20Z

cmd/nerdctl/container_run_network_linux_test.go

+	}
+}
+
+func TestHostPortAlreadyInUseForSctp(t *testing.T) {


TestHostPortAlreadyInUseForSctp can simply be added as a case in TestHostPortAlreadyInUse

Updated, note though that sctp does not work in rootless mode. Error message failed to create shim task: OCI runtime create failed: runc create failed: unable to start container process: error during container init: error running hook #0: error running hook: exit status 1, stdout: , stderr: time="2023-05-29T21:35:05Z" level=fatal msg="failed to expose ports in rootless mode: spec was not validated?".

so, we have to skip the sctp case in rootless environnement

so, we have to skip the sctp case in rootless environnement

Yeah I was just calling out the behavior, it's already skipped in this test in rootless mode

nerdctl/cmd/nerdctl/container_run_network_linux_test.go

Line 336 in afd1cee

if strings.Contains(tc.containerPort, "sctp") && rootlessutil.IsRootless() {

.

Signed-off-by: Vishwas Siravara <vsiravara@gmail.com>

fahedouch · 2023-06-08T17:01:49Z

@vsiravar Could you confirm this? #2190 (comment)

Sorry I have been a bit busy. I will have all the comments addressed this weekend.

Can you test this case locally? It seems different from what was expected?
$ sudo nerdctl run -p 8080:80 --name=test -d nginx
7f4a2be3bd30eba9d9b5c45ea83213af9728fa456bd3b23783a3352282afc078
$ sudo nerdctl stop test
test
$ sudo nerdctl run -p 8080:80 -d nginx
FATA[0000] failed to load networking flags: bind for :8080 failed: port is already allocated
I think this is because nerdctl stop does not clean up the network unlike nerdctl remove . I can make a follow up PR to fix this, if it's the behavior we want. cc @junnplus

Edit: Tagged Jun for feeback.

LGTM it is the nerdctl stop responsibility to binded free port(s)

AkihiroSuda · 2023-06-12T09:36:49Z

@junnplus LGTY?

junnplus · 2023-06-13T04:18:11Z

I think this is because nerdctl stop does not clean up the network unlike nerdctl remove . I can make a follow up PR to fix this, if it's the behavior we want. cc @junnplus

I apologize for just seeing the new update, thank you for your hard work, I will review it this week.

junnplus · 2023-06-18T23:29:19Z

LGTM it is the nerdctl stop responsibility to binded free port(s)

I concur with it, but hold reservations about merging this PR. I believe we should first have nerdctl stop manage the release of ports.

The rest LGTM.

fahedouch · 2023-06-24T12:47:32Z

LGTM it is the nerdctl stop responsibility to binded free port(s)

I concur with it, but hold reservations about merging this PR. I believe we should first have nerdctl stop manage the release of ports.

The rest LGTM.

cc vsiravar, could you please fix the stop ( it can be a different PR) so we can merge this accordingly? Thanks

vsiravar · 2023-06-27T16:01:27Z

LGTM it is the nerdctl stop responsibility to binded free port(s)

I concur with it, but hold reservations about merging this PR. I believe we should first have nerdctl stop manage the release of ports.
The rest LGTM.

cc vsiravar, could please fix the stop ( it can be a different PR) so we can merge this accordingly? Thanks

Sure, I'll work on it in the next couple of weeks.

vsiravar · 2023-07-05T04:49:38Z

LGTM it is the nerdctl stop responsibility to binded free port(s)

I concur with it, but hold reservations about merging this PR. I believe we should first have nerdctl stop manage the release of ports.
The rest LGTM.

cc vsiravar, could you please fix the stop ( it can be a different PR) so we can merge this accordingly? Thanks

Sure. I will have a PR out this week. Cheers!

AkihiroSuda · 2023-08-07T06:32:03Z

What's the current status of this?

vsiravar · 2023-08-08T21:48:52Z

What's the current status of this?

I think this PR is good. I am still working on the PR to fix the stop to free binded ports on stop.

AkihiroSuda · 2023-09-12T06:25:40Z

@junnplus PTAL

fahedouch

Thanks

AkihiroSuda · 2024-12-03T06:53:57Z

Sorry, needs rebase

swagatbora90 · 2025-04-01T17:33:00Z

We ran into the same issue with finch runfinch/finch#1333.

@vsiravar I noticed this PR has the approvals but needs a rebase. Are you still working on this fix?

fahedouch · 2025-04-02T11:23:36Z

@vsiravar please rebase this PR

vsiravar · 2025-04-07T18:17:36Z

@swagatbora90 can you help take this forward. I'm not available this week.

swagatbora90 · 2025-04-09T23:06:35Z

I have rebased and opened a new PR here #4097.

fahedouch added the area/network label Apr 18, 2023

fahedouch added this to the v1.4.0 (tentative) milestone Apr 18, 2023

fahedouch reviewed Apr 18, 2023

View reviewed changes

vsiravar force-pushed the vsiravar/fix-2179 branch from 8c29e4f to ed7a153 Compare April 18, 2023 16:05

vsiravar requested a review from fahedouch April 18, 2023 16:06

fahedouch reviewed Apr 19, 2023

View reviewed changes

vsiravar force-pushed the vsiravar/fix-2179 branch from ed7a153 to 6dbac87 Compare April 20, 2023 00:44

vsiravar requested a review from fahedouch April 20, 2023 15:54

junnplus reviewed Apr 25, 2023

View reviewed changes

pkg/portutil/portutil.go Outdated Show resolved Hide resolved

junnplus reviewed Apr 25, 2023

View reviewed changes

pkg/portutil/portutil.go Outdated Show resolved Hide resolved

vsiravar force-pushed the vsiravar/fix-2179 branch 3 times, most recently from 879bea2 to 3442f88 Compare April 26, 2023 19:00

vsiravar requested a review from junnplus April 26, 2023 21:15

junnplus reviewed Apr 27, 2023

View reviewed changes

cmd/nerdctl/container_run_network_linux_test.go Show resolved Hide resolved

junnplus reviewed Apr 27, 2023

View reviewed changes

vsiravar force-pushed the vsiravar/fix-2179 branch from 3442f88 to 7c3703c Compare April 27, 2023 01:21

vsiravar requested a review from junnplus April 27, 2023 01:26

fahedouch reviewed May 1, 2023

View reviewed changes

AkihiroSuda modified the milestones: v1.4.0, v1.4.1 (tentative) May 18, 2023

vsiravar force-pushed the vsiravar/fix-2179 branch 2 times, most recently from b86c6ba to f63c2ba Compare May 23, 2023 06:03

vsiravar requested a review from fahedouch May 23, 2023 21:46

vsiravar force-pushed the vsiravar/fix-2179 branch from f63c2ba to 984ffc5 Compare May 23, 2023 22:07

fahedouch reviewed May 29, 2023

View reviewed changes

vsiravar force-pushed the vsiravar/fix-2179 branch 2 times, most recently from eded758 to aa8e13d Compare May 29, 2023 21:08

Check if port is used in publish flag

afd1cee

Signed-off-by: Vishwas Siravara <vsiravara@gmail.com>

vsiravar force-pushed the vsiravar/fix-2179 branch from aa8e13d to afd1cee Compare May 29, 2023 21:10

vsiravar requested a review from fahedouch May 29, 2023 21:42

AkihiroSuda modified the milestones: v1.5.0, v1.5.1 (tentative) Jul 30, 2023

AkihiroSuda modified the milestones: v1.5.1, v1.5.2 Sep 11, 2023

fahedouch approved these changes Sep 12, 2023

View reviewed changes

AkihiroSuda removed this from the v1.6.1 (tentative) milestone Oct 8, 2023

swagatbora90 mentioned this pull request Apr 9, 2025

[Bugfix] Check if port is used in publish flag [duplicate of #2190] #4097

Open

fix: Add used port check for publish flag #2190

Are you sure you want to change the base?

fix: Add used port check for publish flag #2190

Conversation

vsiravar commented Apr 18, 2023

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

Choose a reason for hiding this comment

junnplus commented Apr 27, 2023

vsiravar commented Apr 28, 2023

junnplus commented May 1, 2023

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

fahedouch May 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fahedouch commented May 1, 2023

AkihiroSuda commented May 18, 2023

vsiravar commented May 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vsiravar Jun 6, 2023 • edited Loading

Choose a reason for hiding this comment

fahedouch commented Jun 8, 2023 • edited Loading

AkihiroSuda commented Jun 12, 2023

junnplus commented Jun 13, 2023

junnplus commented Jun 18, 2023 • edited Loading

fahedouch commented Jun 24, 2023 • edited Loading

vsiravar commented Jun 27, 2023

vsiravar commented Jul 5, 2023

AkihiroSuda commented Aug 7, 2023

vsiravar commented Aug 8, 2023

AkihiroSuda commented Sep 12, 2023

fahedouch left a comment

Choose a reason for hiding this comment

AkihiroSuda commented Dec 3, 2024

swagatbora90 commented Apr 1, 2025

fahedouch commented Apr 2, 2025

vsiravar commented Apr 7, 2025

swagatbora90 commented Apr 9, 2025

fahedouch May 1, 2023 •

edited

Loading

vsiravar commented May 18, 2023 •

edited

Loading

vsiravar Jun 6, 2023 •

edited

Loading

fahedouch commented Jun 8, 2023 •

edited

Loading

junnplus commented Jun 18, 2023 •

edited

Loading

fahedouch commented Jun 24, 2023 •

edited

Loading