Don't expose ports to the outside and fix a race condition#1850
Don't expose ports to the outside and fix a race condition#1850dgageot merged 6 commits intoGoogleContainerTools:masterfrom
Conversation
dgageot
requested review from
balopat,
nkubala,
priyawadhwa and
r2d4
as code owners
Codecov Report
@@ Coverage Diff @@
## master #1850 +/- ##
=========================================
+ Coverage 49.18% 49.38% +0.2%
=========================================
Files 166 166
Lines 7287 7285 -2
=========================================
+ Hits 3584 3598 +14
+ Misses 3357 3340 -17
- Partials 346 347 +1
Continue to review full report at Codecov.
|
| // See https://www.iana.org/assignments/service-names-port-numbers/service-names-port-numbers.txt, | ||
| func GetAvailablePort(port int, forwardedPorts *sync.Map) int { | ||
| if isPortAvailable(port, forwardedPorts) { | ||
| forwardedPorts.Store(port, true) |
There was a problem hiding this comment.
These stores are necessary as there are race conditions: port-forwarding gets an available port but doesn't use it immediately I missed that you moved this into isPortAvailable()
|
|
||
| func isPortAvailable(p int, forwardedPorts *sync.Map) bool { | ||
| if _, ok := forwardedPorts.Load(p); ok { | ||
| alreadyUsed, loaded := forwardedPorts.LoadOrStore(p, true) |
There was a problem hiding this comment.
It seems odd for an is* method to mutate state
There was a problem hiding this comment.
Maybe rename the method to something like getPortIfAvailable()?
pkg/skaffold/event/server.go
Outdated
| } | ||
|
|
||
| l, err := net.Listen("tcp", fmt.Sprintf(":%d", port)) | ||
| l, err := net.Listen("tcp", fmt.Sprintf("127.0.0.1:%d", port)) |
There was a problem hiding this comment.
maybe move "127.0.0.1" to a constant.
Signed-off-by: David Gageot <david@gageot.net>
Signed-off-by: David Gageot <david@gageot.net>
Signed-off-by: David Gageot <david@gageot.net>
Signed-off-by: David Gageot <david@gageot.net>
Signed-off-by: David Gageot <david@gageot.net>
Signed-off-by: David Gageot <david@gageot.net>
dgageot
force-pushed
the
available-ports
branch
from
8e3d233 to
6cd6fe7
Compare
|
@tejal29 @briandealwis should be all good now |
briandealwis
left a comment
briandealwis
left a comment
There was a problem hiding this comment.
LGTM: The tests could do more to check conditions like single-threaded map check, and with an actual open port, but it's better than what's there.
7 participants
Ports used for eventing shouldn't be exposed to the outside world. Also this removed an alert on OSX each time a port needs to be opened on the firewall.
I also took the opportunity to fix a race condition where a port could be said available when it is not.