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

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

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

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

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

Choose a reason for hiding this comment

fahedouch commented Jun 8, 2023 • edited

AkihiroSuda commented Jun 12, 2023

junnplus commented Jun 13, 2023

junnplus commented Jun 18, 2023 • edited

fahedouch commented Jun 24, 2023 • edited

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

fahedouch May 1, 2023 •

edited

vsiravar commented May 18, 2023 •

edited

vsiravar Jun 6, 2023 •

edited

fahedouch commented Jun 8, 2023 •

edited

junnplus commented Jun 18, 2023 •

edited

fahedouch commented Jun 24, 2023 •

edited