From 158ccdfe5830c817497bdd45c730cf0e03a745d3 Mon Sep 17 00:00:00 2001 From: Jihyeon Gim Date: Fri, 4 Sep 2026 17:30:46 +0900 Subject: [PATCH] rdma: test RC route errors through the production server Cover the route error boundary with the real S3 server: the shared serializer keeps status and body for wrapped S3 errors, raw fiber errors stay 500, and the platform stub answers 501. The stub-answer classifier moves next to the shared marker type in the same commit so every build answers 501 at the point the test first runs, and the general CI workflow builds the session-server archive before go test, which the Linux link of this package now requires. --- .github/workflows/go.yml | 3 + rdma/rcroutes/errors.go | 52 +++++-- rdma/rcroutes/errors_linux.go | 4 - rdma/rcroutes/errors_mapping_linux_test.go | 130 +++++++++++++++++ rdma/rcroutes/errors_stub.go | 17 --- rdma/rcroutes/routes_linux.go | 6 +- rdma/rcroutes/routes_stub.go | 2 +- s3api/rc_route_error_test.go | 158 +++++++++++++++++++++ 8 files changed, 333 insertions(+), 39 deletions(-) create mode 100644 rdma/rcroutes/errors_mapping_linux_test.go create mode 100644 s3api/rc_route_error_test.go diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index e7ed3251..2da868f2 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -24,6 +24,9 @@ jobs: run: | go get -v -t -d ./... + - name: Build RDMA session server archive + run: make rdma/librcserver.a + - name: Test run: go test -coverprofile profile.txt -race -v -timeout 30s -tags=github ./... diff --git a/rdma/rcroutes/errors.go b/rdma/rcroutes/errors.go index ee6221a7..5f3d0627 100644 --- a/rdma/rcroutes/errors.go +++ b/rdma/rcroutes/errors.go @@ -18,26 +18,41 @@ import ( "github.com/gofiber/fiber/v3" + "github.com/versity/versitygw/debuglogger" "github.com/versity/versitygw/s3api/utils" "github.com/versity/versitygw/s3err" ) -// writeRouteError renders err as the terminal S3-style XML response +// ErrRouteNotImplemented is the platform-stub answer of the +// control routes on builds without the RC data plane. +type ErrRouteNotImplemented struct{} + +func (ErrRouteNotImplemented) Error() string { + return "RDMA not supported on this platform" +} + +// isRouteNotImplemented reports whether err is the platform-stub +// answer of the control routes. The marker type is shared, so +// every build classifies it the same way. +func isRouteNotImplemented(err error) bool { + var ni ErrRouteNotImplemented + return errors.As(err, &ni) +} + +// WriteRouteError renders err as the terminal S3-style XML response // of an RC control route. The production S3 error handler converts // ordinary Fiber errors into a generic 500 response, so the route // must send its final status and body itself. Errors that already // carry S3 semantics (authentication, authorization, object // backend) keep their status and code; anything else is mapped to -// a protocol error without exposing internal detail. -func writeRouteError(ctx fiber.Ctx, err error) error { +// a protocol error without exposing internal detail. The gateway +// auth adapter uses it for the same reason. +func WriteRouteError(ctx fiber.Ctx, err error) error { requestID, hostID := utils.EnsureRequestIDs(ctx) - var apiErr s3err.APIError - switch e := classifyRouteError(err).(type) { - case s3err.S3Error: - apiErr = e.BaseError() - case s3err.APIError: - apiErr = e + apiErr := routeError(err) + if apiErr.HTTPStatusCode == fiber.StatusInternalServerError { + logInternalRouteError(ctx, err) } if isRouteNotImplemented(err) { apiErr = s3err.APIError{ @@ -52,13 +67,14 @@ func writeRouteError(ctx fiber.Ctx, err error) error { Send(apiErr.XMLBody(requestID, hostID)) } -// classifyRouteError resolves err to a value implementing -// s3err.S3Error. S3-aware errors pass through unchanged; RC -// transport errors are mapped to the closest protocol error. -func classifyRouteError(err error) s3err.S3Error { +// routeError resolves err to its S3-style response. Errors that +// already carry S3 semantics (authentication, authorization, +// object backend) keep their status and code; RC transport errors +// map to the closest protocol error. +func routeError(err error) s3err.APIError { var s3Err s3err.S3Error if errors.As(err, &s3Err) { - return s3Err + return s3Err.BaseError() } code, status, description := routeErrorDetails(err) @@ -69,6 +85,14 @@ func classifyRouteError(err error) s3err.S3Error { } } +// logInternalRouteError keeps a server-side trace of unexpected +// failures. The wire response stays generic; without this the +// terminal serializer would hide the production diagnostics the +// global error handler used to log. +func logInternalRouteError(ctx fiber.Ctx, err error) { + debuglogger.InternalError(err) +} + // routeErrorDetails maps a non-S3 route error to its protocol // error code, HTTP status, and description. func routeErrorDetails(err error) (code string, status int, description string) { diff --git a/rdma/rcroutes/errors_linux.go b/rdma/rcroutes/errors_linux.go index bdd7b3b6..25b9f4a1 100644 --- a/rdma/rcroutes/errors_linux.go +++ b/rdma/rcroutes/errors_linux.go @@ -83,7 +83,3 @@ func isRouteUnavailable(err error) bool { var unavail errRouteUnavailable return errors.As(err, &unavail) } - -// isRouteNotImplemented reports whether the platform stub answered -// the request; always false on Linux builds. -func isRouteNotImplemented(err error) bool { return false } diff --git a/rdma/rcroutes/errors_mapping_linux_test.go b/rdma/rcroutes/errors_mapping_linux_test.go new file mode 100644 index 00000000..74adc866 --- /dev/null +++ b/rdma/rcroutes/errors_mapping_linux_test.go @@ -0,0 +1,130 @@ +// Copyright 2026 Versity Software +// This file is licensed under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an "AS +// IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either +// express or implied. See the License for the specific language +// governing permissions and limitations under the License. + +//go:build linux && amd64 && cgo + +package rcroutes + +import ( + "errors" + "fmt" + "net/http" + "testing" + + "github.com/versity/versitygw/rdma/rcserver" +) + +// This file verifies the mapping from session-server failures to +// protocol error responses. It runs where the Linux session +// server links; the s3api package exercises the shared +// serializer through the production S3 server on every platform. + +func TestRouteErrorProtocolMapping(t *testing.T) { + tests := []struct { + name string + err error + code string + want int + }{ + { + name: "invalid header is 400", + err: invalidHeader(hdrProtocol, "bogus"), + code: "InvalidRdmaRequest", + want: http.StatusBadRequest, + }, + { + name: "unknown session is 404", + err: rcserver.ErrNoSession, + code: "NoSuchRdmaSession", + want: http.StatusNotFound, + }, + { + name: "owner mismatch answers as unknown session", + err: fmt.Errorf("session owner mismatch: %w", rcserver.ErrNoSession), + code: "NoSuchRdmaSession", + want: http.StatusNotFound, + }, + { + name: "stale session is 409", + err: rcserver.ErrStale, + code: "RdmaSessionConflict", + want: http.StatusConflict, + }, + { + name: "duplicate borrow is 409", + err: fmt.Errorf("peer busy: %w", rcserver.ErrDouble), + code: "RdmaSessionConflict", + want: http.StatusConflict, + }, + { + name: "wrong session state is 409", + err: rcserver.ErrState, + code: "RdmaSessionConflict", + want: http.StatusConflict, + }, + { + name: "resource limit is 429", + err: rcserver.ErrLimit, + code: "RdmaResourceLimit", + want: http.StatusTooManyRequests, + }, + { + name: "wire failure is 502", + err: rcserver.ErrWire, + code: "RdmaTransferFailed", + want: http.StatusBadGateway, + }, + { + name: "short transfer is 400", + err: rcserver.ErrShort, + code: "InvalidRdmaRequest", + want: http.StatusBadRequest, + }, + { + name: "value too long is 400", + err: rcserver.ErrTrunc, + code: "InvalidRdmaRequest", + want: http.StatusBadRequest, + }, + { + name: "invalid argument is 400", + err: rcserver.ErrArg, + code: "InvalidRdmaRequest", + want: http.StatusBadRequest, + }, + { + name: "admission refusal is 503", + err: errNotAdmitted(), + code: "RdmaServiceUnavailable", + want: http.StatusServiceUnavailable, + }, + { + name: "unexpected error is 500", + err: errors.New("boom"), + code: "InternalError", + want: http.StatusInternalServerError, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := routeError(tt.err) + if got.HTTPStatusCode != tt.want { + t.Fatalf("status = %d, want %d", got.HTTPStatusCode, tt.want) + } + if got.Code != tt.code { + t.Fatalf("code = %q, want %q", got.Code, tt.code) + } + }) + } +} diff --git a/rdma/rcroutes/errors_stub.go b/rdma/rcroutes/errors_stub.go index 4d8af5bb..97e1c178 100644 --- a/rdma/rcroutes/errors_stub.go +++ b/rdma/rcroutes/errors_stub.go @@ -15,23 +15,6 @@ package rcroutes -import "errors" - -// errRouteNotImplemented marks the platform-stub answer of the -// control routes. -type errRouteNotImplemented struct{} - -func (errRouteNotImplemented) Error() string { - return "RDMA not supported on this platform" -} - -// isRouteNotImplemented reports whether the platform stub answered -// the request. -func isRouteNotImplemented(err error) bool { - var ni errRouteNotImplemented - return errors.As(err, &ni) -} - // isRouteBadRequest reports whether err is a malformed-request // class error. The stub never classifies request errors because it // answers before validation. diff --git a/rdma/rcroutes/routes_linux.go b/rdma/rcroutes/routes_linux.go index af1d7a18..e0a6c818 100644 --- a/rdma/rcroutes/routes_linux.go +++ b/rdma/rcroutes/routes_linux.go @@ -112,7 +112,7 @@ func (h *Handler) Prepare(ctx fiber.Ctx) error { // production S3 error handler turns ordinary Fiber // errors into a generic 500 response. if err := h.prepareCore(ctx); err != nil { - return writeRouteError(ctx, err) + return WriteRouteError(ctx, err) } return nil } @@ -269,7 +269,7 @@ func (h *Handler) Ready(ctx fiber.Ctx) error { // production S3 error handler turns ordinary Fiber // errors into a generic 500 response. if err := h.readyCore(ctx); err != nil { - return writeRouteError(ctx, err) + return WriteRouteError(ctx, err) } return nil } @@ -462,7 +462,7 @@ func (h *Handler) Cancel(ctx fiber.Ctx) error { // production S3 error handler turns ordinary Fiber // errors into a generic 500 response. if err := h.cancelCore(ctx); err != nil { - return writeRouteError(ctx, err) + return WriteRouteError(ctx, err) } return nil } diff --git a/rdma/rcroutes/routes_stub.go b/rdma/rcroutes/routes_stub.go index ad8dc60b..387f4a5e 100644 --- a/rdma/rcroutes/routes_stub.go +++ b/rdma/rcroutes/routes_stub.go @@ -37,7 +37,7 @@ func New(svc any, be backend.Backend, iam auth.IAMService, // notImplemented answers 501 through the shared terminal error // serializer so the response shape matches the Linux routes. func notImplemented(ctx fiber.Ctx) error { - return writeRouteError(ctx, errRouteNotImplemented{}) + return WriteRouteError(ctx, ErrRouteNotImplemented{}) } // Prepare is a stub handler that answers 501 Not Implemented. diff --git a/s3api/rc_route_error_test.go b/s3api/rc_route_error_test.go new file mode 100644 index 00000000..680677b5 --- /dev/null +++ b/s3api/rc_route_error_test.go @@ -0,0 +1,158 @@ +// Copyright 2026 Versity Software +// This file is licensed under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an "AS +// IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either +// express or implied. See the License for the specific language +// governing permissions and limitations under the License. + +package s3api + +import ( + "encoding/xml" + "errors" + "net/http" + "net/http/httptest" + "testing" + + "github.com/gofiber/fiber/v3" + + "github.com/versity/versitygw/rdma/rcroutes" + "github.com/versity/versitygw/s3err" +) + +// This file pins the wire behavior of RDMA control-route error +// responses at the production server boundary: the real error +// handler, the real request-ID middleware, and the terminal +// serializer the RDMA routes use. The RC route handlers live in +// rdma/rcroutes; these tests exercise the same response path a +// client observes. + +type rcErrorResponse struct { + XMLName xml.Name `xml:"Error"` + Code string `xml:"Code"` + Message string `xml:"Message"` + RequestID string `xml:"RequestId"` + HostID string `xml:"HostId"` +} + +func TestRCRouteErrorPreservesS3Error(t *testing.T) { + server, err := newTestS3ApiServer( + WithRoute(http.MethodPost, "/.hipobj-rc/op", func(ctx fiber.Ctx) error { + return rcroutes.WriteRouteError(ctx, + s3err.GetAPIError(s3err.ErrAccessDenied)) + }), + ) + if err != nil { + t.Fatalf("New() error = %v", err) + } + + resp, err := server.app.Test(httptest.NewRequest(http.MethodPost, "/.hipobj-rc/op", nil)) + if err != nil { + t.Fatalf("app.Test() error = %v", err) + } + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != http.StatusForbidden { + t.Fatalf("status = %d, want %d", resp.StatusCode, http.StatusForbidden) + } + var er rcErrorResponse + if err := xml.NewDecoder(resp.Body).Decode(&er); err != nil { + t.Fatalf("decode XML body: %v", err) + } + if er.Code != "AccessDenied" { + t.Fatalf("code = %q, want AccessDenied", er.Code) + } + if er.RequestID == "" || er.HostID == "" { + t.Fatal("response missing RequestId or HostId") + } + if ct := resp.Header.Get("Content-Type"); ct != fiber.MIMEApplicationXML { + t.Fatalf("content-type = %q, want %q", ct, fiber.MIMEApplicationXML) + } +} + +func TestRCRouteErrorRawFiberIs500(t *testing.T) { + // Control case: an ordinary fiber error collapses into the + // generic 500 of the production error handler. This is the + // behavior WriteRouteError exists to avoid. + server, err := newTestS3ApiServer( + WithRoute(http.MethodPost, "/.hipobj-rc/op", func(ctx fiber.Ctx) error { + return fiber.NewError(fiber.StatusTeapot, "raw") + }), + ) + if err != nil { + t.Fatalf("New() error = %v", err) + } + + resp, err := server.app.Test(httptest.NewRequest(http.MethodPost, "/.hipobj-rc/op", nil)) + if err != nil { + t.Fatalf("app.Test() error = %v", err) + } + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != http.StatusInternalServerError { + t.Fatalf("status = %d, want %d (production handler collapses fiber errors)", + resp.StatusCode, http.StatusInternalServerError) + } +} + +func TestRCRouteErrorUnexpectedIsGeneric500(t *testing.T) { + server, err := newTestS3ApiServer( + WithRoute(http.MethodPost, "/.hipobj-rc/op", func(ctx fiber.Ctx) error { + return rcroutes.WriteRouteError(ctx, errors.New("boom")) + }), + ) + if err != nil { + t.Fatalf("New() error = %v", err) + } + + resp, err := server.app.Test(httptest.NewRequest(http.MethodPost, "/.hipobj-rc/op", nil)) + if err != nil { + t.Fatalf("app.Test() error = %v", err) + } + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != http.StatusInternalServerError { + t.Fatalf("status = %d, want %d", resp.StatusCode, http.StatusInternalServerError) + } + var er rcErrorResponse + if err := xml.NewDecoder(resp.Body).Decode(&er); err != nil { + t.Fatalf("decode XML body: %v", err) + } + if er.Code != "InternalError" { + t.Fatalf("code = %q, want InternalError", er.Code) + } +} + +func TestRCRouteErrorStubNotImplemented(t *testing.T) { + server, err := newTestS3ApiServer( + WithRoute(http.MethodPost, "/.hipobj-rc/op", func(ctx fiber.Ctx) error { + return rcroutes.WriteRouteError(ctx, rcroutes.ErrRouteNotImplemented{}) + }), + ) + if err != nil { + t.Fatalf("New() error = %v", err) + } + + resp, err := server.app.Test(httptest.NewRequest(http.MethodPost, "/.hipobj-rc/op", nil)) + if err != nil { + t.Fatalf("app.Test() error = %v", err) + } + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != http.StatusNotImplemented { + t.Fatalf("status = %d, want %d", resp.StatusCode, http.StatusNotImplemented) + } + var er rcErrorResponse + if err := xml.NewDecoder(resp.Body).Decode(&er); err != nil { + t.Fatalf("decode XML body: %v", err) + } + if er.Code != "NotImplemented" { + t.Fatalf("code = %q, want NotImplemented", er.Code) + } +}