pkg/specgen: fix port conflict on host assignment#28889
Conversation
|
I approved this but given the host port test failures, which I didn't look at very deeply, I'm not sure if something isnt quite right. |
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
Which failure, this one? That is unrelated and has flaked before I have not looked into into what is going on with that. |
Honny1
left a comment
Honny1
left a comment
There was a problem hiding this comment.
LGTM, just one non-blocking comment.
| h := port.HostPort + i | ||
| allHostPorts[h] = true | ||
| if currentHostPorts[h] { | ||
| return fmt.Errorf("host port %q was already assigned in another port mapping", net.JoinHostPort(port.HostIP, strconv.Itoa(int(port.HostPort)))+"/"+port.Protocol) |
There was a problem hiding this comment.
Non-blocking: This error msg will contain port.HostPort. That could be misleading if the port is used in range.
| return fmt.Errorf("host port %q was already assigned in another port mapping", net.JoinHostPort(port.HostIP, strconv.Itoa(int(port.HostPort)))+"/"+port.Protocol) | |
| return fmt.Errorf("host port %q was already assigned in another port mapping", net.JoinHostPort(port.HostIP, strconv.Itoa(int(h)))+"/"+port.Protocol) |
There was a problem hiding this comment.
oh right, let me repush and add a test for it
There was a problem hiding this comment.
ok I can fix this inconsistency but I cannot add a test because we never get here in this case
We are failing before that with "conflicting port mappings for host port "
And if there are overlapping ranges we accept and merge them into one
|
I think you should rebase on upstream main. At least, that fixed the same failure for me on my PR. |
This fixes two problems when parsing ports. First, check for host port conflicts. When we are given the same ip:host port combo twice then we need to reject that as invalid, the backend cannot bind the same port twice and thus we always get a runtime failure. Failing early in the create code path is much better. Second, when assigning random ports for expose we still have to check for proper conflicts. The first error was using allUsedContainerPortsMap to check for conflicts but this holds container side ports, we need to ensure there are no conflicts on the host port. Then there was the other issue that the array in the map was copied and accessed by value on lookup. And because the code did not reassign the value it then failed to actually update the correct ports. To address that I switch the map to store the array by reference which will avoid the bigger copies as we only need to update the pointer now. Signed-off-by: Paul Holzinger <pholzing@redhat.com>
I thought I did but I guess not, I really do not get the random golangci-lint failures like that. |
Me neither, but it seems very strange. I'm not sure where the problem could be. |
4 participants
This fixes two problems when parsing ports.
First, check for host port conflicts. When we are given the same ip:host port combo twice then we need to reject that as invalid, the backend cannot bind the same port twice and thus we always get a runtime failure. Failing early in the create code path is much better.
Second, when assigning random ports for expose we still have to check for proper conflicts. The first error was using allUsedContainerPortsMap to check for conflicts but this holds container side ports, we need to ensure there are no conflicts on the host port.
Then there was the other issue that the array in the map was copied and accessed by value on lookup. And because the code did not reassign the value it then failed to actually update the correct ports. To address that I switch the map to store the array by reference which will avoid the bigger copies as we only need to update the pointer now.
Does this PR introduce a user-facing change?