-
Notifications
You must be signed in to change notification settings - Fork 654
fix: Add used port check for publish flag #2190
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -302,6 +302,62 @@ func TestUniqueHostPortAssignement(t *testing.T) { | |
} | ||
} | ||
|
||
func TestHostPortAlreadyInUse(t *testing.T) { | ||
testCases := []struct { | ||
hostPort string | ||
containerPort string | ||
}{ | ||
{ | ||
hostPort: "5000", | ||
containerPort: "80/tcp", | ||
}, | ||
{ | ||
hostPort: "5000", | ||
containerPort: "80/tcp", | ||
}, | ||
{ | ||
hostPort: "5000", | ||
containerPort: "80/udp", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. may be adding a case for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added. |
||
}, | ||
{ | ||
hostPort: "5000", | ||
containerPort: "80/sctp", | ||
}, | ||
} | ||
|
||
tID := testutil.Identifier(t) | ||
|
||
for i, tc := range testCases { | ||
|
||
i := i | ||
tc := tc | ||
tcName := fmt.Sprintf("%+v", tc) | ||
t.Run(tcName, func(t *testing.T) { | ||
if strings.Contains(tc.containerPort, "sctp") && rootlessutil.IsRootless() { | ||
t.Skip("sctp is not supported in rootless mode") | ||
} | ||
testContainerName1 := fmt.Sprintf("%s-%d-1", tID, i) | ||
testContainerName2 := fmt.Sprintf("%s-%d-2", tID, i) | ||
base := testutil.NewBase(t) | ||
defer base.Cmd("rm", "-f", testContainerName1, testContainerName2).Run() | ||
|
||
pFlag := fmt.Sprintf("%s:%s", tc.hostPort, tc.containerPort) | ||
cmd1 := base.Cmd("run", "-d", | ||
"--name", testContainerName1, "-p", | ||
pFlag, | ||
testutil.NginxAlpineImage) | ||
|
||
cmd2 := base.Cmd("run", "-d", | ||
"--name", testContainerName2, "-p", | ||
pFlag, | ||
testutil.NginxAlpineImage) | ||
|
||
cmd1.AssertOK() | ||
cmd2.AssertFail() | ||
}) | ||
} | ||
} | ||
|
||
func TestRunPort(t *testing.T) { | ||
baseTestRunPort(t, testutil.NginxAlpineImage, testutil.NginxAlpineIndexHTMLSnippet, true) | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,24 +39,55 @@ 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) | ||
if err != nil { | ||
return 0, 0, err | ||
} | ||
netprocItems := procnet.Parse(netprocData) | ||
|
||
start := uint64(allocateStart) | ||
if count > uint64(allocateEnd-allocateStart+1) { | ||
return 0, 0, fmt.Errorf("can not allocate %d ports", count) | ||
} | ||
for start < allocateEnd { | ||
needReturn := true | ||
for i := start; i < start+count; i++ { | ||
if _, ok := usedPort[i]; ok { | ||
needReturn = false | ||
break | ||
} | ||
} | ||
if needReturn { | ||
return start, start + count - 1, nil | ||
} | ||
start += count | ||
} | ||
return 0, 0, fmt.Errorf("there is not enough %d free ports", count) | ||
} | ||
|
||
func getUsedPorts(ip string, protocol string) (map[uint64]bool, error) { | ||
netprocItems := []procnet.NetworkDetail{} | ||
|
||
if protocol == "tcp" || protocol == "udp" { | ||
netprocData, err := procnet.ReadStatsFileData(protocol) | ||
if err != nil { | ||
return nil, err | ||
} | ||
netprocItems = append(netprocItems, procnet.Parse(netprocData)...) | ||
} | ||
Comment on lines
+70
to
+76
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It appears that the SCTP protocol is currently unsupported, therefore no further processing is required. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I only checkd in https://github.com/containerd/nerdctl/blob/main/pkg/portutil/procnet/procnet_linux.go#L38 , Let me check it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I carefully checked the code and did not find an implementation that supports the SCTP protocol. The line just supports parsing command line arguments. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.
As a side note, if sctp is not supported then why do we support parsing it in the ParseFlagP ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep..., let it works There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I quickly checked, it looks like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated. |
||
|
||
// In some circumstances, when we bind address like "0.0.0.0:80", we will get the formation of ":::80" in /proc/net/tcp6. | ||
// So we need some trick to process this situation. | ||
if protocol == "tcp" { | ||
tempTCPV6Data, err := procnet.ReadStatsFileData("tcp6") | ||
if err != nil { | ||
return 0, 0, err | ||
return nil, err | ||
} | ||
netprocItems = append(netprocItems, procnet.Parse(tempTCPV6Data)...) | ||
} | ||
if protocol == "udp" { | ||
tempUDPV6Data, err := procnet.ReadStatsFileData("udp6") | ||
if err != nil { | ||
return 0, 0, err | ||
return nil, err | ||
} | ||
netprocItems = append(netprocItems, procnet.Parse(tempUDPV6Data)...) | ||
} | ||
|
@@ -75,30 +106,13 @@ func portAllocate(protocol string, ip string, count uint64) (uint64, uint64, err | |
|
||
ipTableItems, err := iptable.ReadIPTables("nat") | ||
if err != nil { | ||
return 0, 0, err | ||
return nil, err | ||
} | ||
destinationPorts := iptable.ParseIPTableRules(ipTableItems) | ||
|
||
for _, port := range destinationPorts { | ||
usedPort[port] = true | ||
} | ||
|
||
start := uint64(allocateStart) | ||
if count > uint64(allocateEnd-allocateStart+1) { | ||
return 0, 0, fmt.Errorf("can not allocate %d ports", count) | ||
} | ||
for start < allocateEnd { | ||
needReturn := true | ||
for i := start; i < start+count; i++ { | ||
if _, ok := usedPort[i]; ok { | ||
needReturn = false | ||
break | ||
} | ||
} | ||
if needReturn { | ||
return start, start + count - 1, nil | ||
} | ||
start += count | ||
} | ||
return 0, 0, fmt.Errorf("there is not enough %d free ports", count) | ||
return usedPort, nil | ||
} |
Uh oh!
There was an error while loading. Please reload this page.