Skip to content

Unlisten sockets after they disconnect#580

Merged
djs55 merged 1 commit intomoby:masterfrom
fredericdalleau:unlisten-socket
Apr 26, 2022
Merged

Unlisten sockets after they disconnect#580
djs55 merged 1 commit intomoby:masterfrom
fredericdalleau:unlisten-socket

Conversation

@fredericdalleau
Copy link
Copy Markdown
Collaborator

After a successful connection forwarding to a server, if the server
disappears, a tentative to connect the server again will be accepted by
vpnkit even if vpnkit failed to reach the server.
Ensure this does not occur by calling unlisten.
Add test for valid and invalid connection cases

Signed-off-by: Frédéric Dalleau frederic.dalleau@docker.com

(* Now that a server is running, connect to an invalid port and ensure it fails quickly *)
let open Slirp_stack in
let ip = Ipaddr.V4.localhost in
let port = server.Server.port + 1 in
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps on the Windows machine server.Server.port + 1 was used?

**** Starting test TCP: test server invalid port
...
9.09694 [ERROR] Connected to localhost:1986

Maybe quickly allocating and deallocating a port will be more reliable, like

with_server (fun _ -> Lwt.return_unit) (fun server -> Server.destroy server >>= fun () -> Lwt.return server.Server.port)
>>= fun invalid_port ->

Copy link
Copy Markdown
Collaborator Author

@fredericdalleau fredericdalleau Apr 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably, in that case, it becomes pretty similar than the next test which ensure connection succeeds first and then destroys the server (which is also closer to the actual reported issue)

@djs55
Copy link
Copy Markdown
Collaborator

djs55 commented Apr 26, 2022

Regarding the Windows failure: it might be a Windows firewall issue on the test running machine. Perhaps the test should be gated with something like

if Sys.os_type = "Win32" then begin
  ...
end else begin
  ...
end

Or maybe a timeout problem? This Go program:

package main

import (
  "fmt"
  "net"
  "time"
)

func main(){
  start := time.Now()
  _, err := net.Dial("tcp", "127.0.0.1:8080")
  if err != nil {
    fmt.Printf("rejected after %s\n", time.Since(start))
    return
  }
  fmt.Printf("connected after %s\n", time.Since(start))
}

takes >2s:

PS C:\Users\dave\go\src\connect-localhost> go build
PS C:\Users\dave\go\src\connect-localhost> .\connect-localhost.exe
rejected after 2.0195789s

After a successful connection forwarding to a server, if the server
disappears, a tentative to connect the server again will be accepted by
vpnkit even if vpnkit failed to reach the server.
Ensure this does not occur by calling unlisten.
Add test for valid and invalid connection cases

Signed-off-by: Frédéric Dalleau <frederic.dalleau@docker.com>
Copy link
Copy Markdown
Collaborator

@djs55 djs55 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (and Mac and Windows CI are green!)

@djs55 djs55 merged commit e67cca0 into moby:master Apr 26, 2022
@yamt
Copy link
Copy Markdown
Contributor

yamt commented Jun 1, 2022

i feel lucky today because this bug i hit today has already been fixed. thank you!

(i was investigating something not working well, which polls the availability of a service by tcp-connecting it.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants