Don't expose ports to the outside and fix a race condition #1850
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.
|
| @@ -32,7 +32,6 @@ import ( | |||
| // 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()
| return p | ||
| } | ||
|
|
||
| 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
| if err != nil { | ||
| return func() error { return nil }, err | ||
| } | ||
|
|
||
| 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 <[email protected]>
Signed-off-by: David Gageot <[email protected]>
Signed-off-by: David Gageot <[email protected]>
Signed-off-by: David Gageot <[email protected]>
Signed-off-by: David Gageot <[email protected]>
Signed-off-by: David Gageot <[email protected]>
dgageot
force-pushed
the
available-ports
branch
from
8e3d233 to
6cd6fe7
Compare
|
@tejal29 @briandealwis should be all good now |
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.