Skip to content
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.

Sign up for GitHub

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

transport: Discard the buffer when empty after http connect handshake #7424

Merged
merged 4 commits into from
Jul 30, 2024
Merged

transport: Discard the buffer when empty after http connect handshake #7424

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b0e2b5d
Discard the buffer when empty after http connect handshake
arjan-bal Jul 18, 2024
3cf3bd6
configure the proxy to wait for server hello
arjan-bal Jul 26, 2024
c17668c
Extract test args to a struct
arjan-bal Jul 26, 2024
db50122
Change deadline sets
arjan-bal Jul 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
configure the proxy to wait for server hello
  • Loading branch information
@arjan-bal
arjan-bal committed Jul 26, 2024
commit 3cf3bd6776830a74803adf358ff4f76488f04340
51 changes: 25 additions & 26 deletions internal/transport/proxy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ type proxyServer struct {
requestCheck func(*http.Request) error
}

func (p *proxyServer) run() {
func (p *proxyServer) run(waitForServerHello bool) {
in, err := p.lis.Accept()
if err != nil {
return
Expand All @@ -84,24 +84,23 @@ func (p *proxyServer) run() {
p.t.Errorf("failed to dial to server: %v", err)
return
}
out.SetReadDeadline(time.Now().Add(defaultTestTimeout))
resp := http.Response{StatusCode: http.StatusOK, Proto: "HTTP/1.0"}
var buf bytes.Buffer
resp.Write(&buf)
// Batch the first message from the server with the http connect
// response. This is done to test the cases in which the grpc client has
// the response to the connect request and proxied packets from the
// destination server when it reads the transport.
out.SetReadDeadline(time.Now().Add(20 * time.Millisecond))
b := make([]byte, 50)
bytesRead, err := out.Read(b)
// The read is expected to fail with deadline exceeded if the server doesn't
// have a message to send.
if err != nil {
p.t.Logf("Got error while reading message from server: %v", err)
if waitForServerHello {
// Batch the first message from the server with the http connect
// response. This is done to test the cases in which the grpc client has
// the response to the connect request and proxied packets from the
// destination server when it reads the transport.
b := make([]byte, 50)
bytesRead, err := out.Read(b)
if err != nil {
p.t.Errorf("Got error while reading server hello: %v", err)
return
}
buf.Write(b[0:bytesRead])
}
// reset the deadline.
out.SetReadDeadline(time.Time{})
buf.Write(b[0:bytesRead])
p.in.Write(buf.Bytes())
p.out = out
go io.Copy(p.in, p.out)
Expand All @@ -128,7 +127,7 @@ func testHTTPConnect(t *testing.T, proxyURLModify func(*url.URL) *url.URL, proxy
lis: plis,
requestCheck: proxyReqCheck,
}
go p.run()
go p.run(len(serverMessage) > 0)
defer p.stop()

blis, err := net.Listen("tcp", "localhost:0")
Expand Down Expand Up @@ -177,16 +176,16 @@ func testHTTPConnect(t *testing.T, proxyURLModify func(*url.URL) *url.URL, proxy
t.Fatalf("received msg: %v, want %v", recvBuf, msg)
}

c.SetReadDeadline(time.Now().Add(20 * time.Millisecond))

gotServerMessage := make([]byte, len(serverMessage))
// This call will return a deadline exceeded error if the server has nothing
// to send. This is expected.
if _, err := c.Read(gotServerMessage); err != nil {
t.Logf("Got error while reading message from server: %v", err)
}
if string(gotServerMessage) != string(serverMessage) {
t.Fatalf("message from server: %v, want %v", gotServerMessage, serverMessage)
if len(serverMessage) > 0 {
c.SetReadDeadline(time.Now().Add(defaultTestTimeout))
Copy link
Member

Choose a reason for hiding this comment

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

There's now 3 places where we're setting this deadline.

Maybe instead when the conn is created in proxyDial we should just do c.SetDeadline(....defaultTestTimeout) (note Deadline not Read to also abort writes)?

Copy link
Contributor Author

@arjan-bal arjan-bal Jul 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

During testing, I noticed that the test can hang in proxyDial if the proxy server waits for a server hello when the server doesn't intend to send one. This was due to a logical error on my part which I fixed.

To be safe, I added read deadlines in both the places where reads could hang.

Maybe instead when the conn is created in proxyDial we should just do c.SetDeadline(....defaultTestTimeout) (note Deadline not Read to also abort writes)?

Do you mean adding a deadline in the actual proxyDial() implementation by introducing a timeout that only the tests use?
Or are you referring to adding a deadline on the conn returned by proxyDial in the test code?

The latter would not prevent proxyDial from hanging when the proxy server is stuck reading the server hello. The former would work but it involves changing non-test code, which I was trying to avoid.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I didn't mean in proxyDial... I meant the connection it returns, we could immediately, always, do a SetDeadline(...), instead of doing it later and conditionally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed SetReadDeadline to SetDeadline. Moved the SetDeadline immediately after the proxyDial call. The SetDeadline in proxyServer.run() is still present to avoid proxyDial from hanging indefinitely in case of test failures.

gotServerMessage := make([]byte, len(serverMessage))
if _, err := c.Read(gotServerMessage); err != nil {
t.Errorf("Got error while reading message from server: %v", err)
return
}
if string(gotServerMessage) != string(serverMessage) {
t.Fatalf("message from server: %v, want %v", gotServerMessage, serverMessage)
}
}
}

Expand Down