diff --git a/CHANGELOG.md b/CHANGELOG.md index c6955646d2..9361605bf6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,9 +5,6 @@ - Change default for `--origin` flag of `buf beta studio-agent` to `https://studio.buf.build` - Change default for `--timeout` flag of `buf beta studio-agent` to `0` (no timeout). Before it was `2m` (the default for all the other `buf` commands). -- Add `connect.CodeUnavailable` as an unhandled Connect error code for `buf beta studio-agent`, - which will trigger an `http.StatusBadGateway` response in case of a bad or unreachable upstream - server. Before the only unhandled Connect error code was `connect.CodeUnknown`. ## [v1.7.0] - 2022-06-27 diff --git a/private/bufpkg/bufstudioagent/bufstudioagent_test.go b/private/bufpkg/bufstudioagent/bufstudioagent_test.go index 7e58dca521..9cacc0823a 100644 --- a/private/bufpkg/bufstudioagent/bufstudioagent_test.go +++ b/private/bufpkg/bufstudioagent/bufstudioagent_test.go @@ -21,6 +21,7 @@ import ( "encoding/base64" "errors" "io" + "net" "net/http" "net/http/httptest" "strconv" @@ -37,9 +38,8 @@ import ( ) const ( - echoPath = "/echo.Service/EchoEcho" - errorPath = "/error.Service/Error" - unknownPath = "/unknown.Service/Unknown" + echoPath = "/echo.Service/EchoEcho" + errorPath = "/error.Service/Error" ) func TestPlainPostHandlerTLS(t *testing.T) { @@ -185,9 +185,31 @@ func testPlainPostHandlerErrors(t *testing.T, upstreamServer *httptest.Server) { assert.Equal(t, "something", upstreamResponseHeaders.Get("grpc-message")) }) - t.Run("unknown_response_bad_gateway", func(t *testing.T) { + t.Run("invalid_upstream", func(t *testing.T) { + // TODO: unskip this test. This is flaky because of two reasons: + // + // 1. When a connection is closed, the underlying HTTP client does not + // always knows it, since the http handler implementation in go has no way + // of changing the connection timeout. See: + // https://github.com/golang/go/issues/16100 + // + // 2. The expected status code is `StatusBadGateway` since the issue + // happened client-side (a response never came back from the server). This + // is not deterministic in the business logic because we're based on the + // connect error code that's returned. See + // https://linear.app/bufbuild/issue/BSR-383/flaky-test-in-bufstudioagent-testgo + t.SkipNow() + listener, err := net.Listen("tcp", "127.0.0.1:") + require.NoError(t, err) + go func() { + conn, err := listener.Accept() + require.NoError(t, err) + require.NoError(t, conn.Close()) + }() + defer listener.Close() + requestProto := &studiov1alpha1.InvokeRequest{ - Target: upstreamServer.URL + unknownPath, + Target: "http://" + listener.Addr().String(), Headers: goHeadersToProtoHeaders(http.Header{ "Content-Type": []string{"application/grpc"}, }), @@ -227,15 +249,6 @@ func newTestConnectServer(t *testing.T, tls bool) *httptest.Server { }, connect.WithCodec(&bufferCodec{name: "proto"}), )) - // unknownPath returns the body as error message with code unknown, to test - // studio agent behavior on potential server closed connections. - mux.Handle(unknownPath, connect.NewUnaryHandler( - unknownPath, - func(ctx context.Context, r *connect.Request[bytes.Buffer]) (*connect.Response[bytes.Buffer], error) { - return nil, connect.NewError(connect.CodeUnknown, errors.New(r.Msg.String())) - }, - connect.WithCodec(&bufferCodec{name: "proto"}), - )) if tls { upstreamServerTLS := httptest.NewUnstartedServer(mux) upstreamServerTLS.EnableHTTP2 = true diff --git a/private/bufpkg/bufstudioagent/plain_post_handler.go b/private/bufpkg/bufstudioagent/plain_post_handler.go index 600335bb2d..7c862a2b75 100644 --- a/private/bufpkg/bufstudioagent/plain_post_handler.go +++ b/private/bufpkg/bufstudioagent/plain_post_handler.go @@ -169,20 +169,47 @@ func (i *plainPostHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { // TODO(rvanginkel) should this context be cloned to remove attached values (but keep timeout)? response, err := client.CallUnary(r.Context(), request) if err != nil { + // TODO deal with this error handling using `connect.IsFromServer(err)` + // instead of these heuristics. See + // https://github.com/bufbuild/connect-go/issues/222 + // + // We need to differentiate client errors from server errors. In the former, + // trigger a `StatusBadGateway` result, and in the latter surface whatever + // error information came back from the server. + // + // Any error here is wrapped in a `connect.Error` struct. We need to check + // *first* if within it also wraps a low level network issue, so we can + // assume the request never left the client, or a response never arrived + // from the server. In those scenarios we trigger a `StatusBadGateway` to + // signal that the upstream server is unreachable or in a bad status... + if netErr := new(net.OpError); errors.As(err, &netErr) { + http.Error(w, err.Error(), http.StatusBadGateway) + return + } + if urlErr := new(url.Error); errors.As(err, &urlErr) { + http.Error(w, err.Error(), http.StatusBadGateway) + return + } + // ... but if a response was received from the server, we assume there's + // error information from the server we can surface to the user by including + // it in the headers response, unless it is a `CodeUnknown` error. Connect + // marks any issues connecting with the `CodeUnknown` error. if connectErr := new(connect.Error); errors.As(err, &connectErr) { - // Any Connect error except these codes can be wrapped in the headers. - if connectErr.Code() != connect.CodeUnknown && - connectErr.Code() != connect.CodeUnavailable { - i.writeProtoMessage(w, &studiov1alpha1.InvokeResponse{ - // connectErr.Meta contains the trailers for the - // caller to find out the error details. - Headers: goHeadersToProtoHeaders(connectErr.Meta()), - }) + if connectErr.Code() == connect.CodeUnknown { + http.Error(w, err.Error(), http.StatusBadGateway) return } + i.writeProtoMessage(w, &studiov1alpha1.InvokeResponse{ + // connectErr.Meta contains the trailers for the + // caller to find out the error details. + Headers: goHeadersToProtoHeaders(connectErr.Meta()), + }) + return } - // Any issue connecting that is not a Connect error is assumed to be because - // the server is in some kind of bad and/or unreachable state. + i.Logger.Warn( + "non_connect_unary_error", + zap.Error(err), + ) http.Error(w, err.Error(), http.StatusBadGateway) return }