From f4ccd53ba52e05349f2f9cbe75a96f1fd4c8a3fe Mon Sep 17 00:00:00 2001 From: Marius Date: Tue, 30 Nov 2021 17:03:58 +0100 Subject: [PATCH] Enable returning partial HTTPResponses --- cmd/tusd/cli/hooks.go | 15 ++++---- cmd/tusd/cli/hooks/hooks.go | 2 ++ cmd/tusd/cli/hooks/plugin.go | 50 ++++++++++++++++++++++----- examples/hooks/plugin/hook_handler.go | 12 +++++-- pkg/handler/unrouted_handler.go | 35 +++++++++++++------ 5 files changed, 84 insertions(+), 30 deletions(-) diff --git a/cmd/tusd/cli/hooks.go b/cmd/tusd/cli/hooks.go index cfa2310..1bbb254 100644 --- a/cmd/tusd/cli/hooks.go +++ b/cmd/tusd/cli/hooks.go @@ -2,7 +2,6 @@ package cli import ( "errors" - "fmt" "strconv" "strings" @@ -151,14 +150,14 @@ func invokeHookSync(typ hooks.HookType, event handler.HookEvent) (httpRes handle }) if err != nil { - err = fmt.Errorf("%s hook failed: %s", typ, err) + //err = fmt.Errorf("%s hook failed: %s", typ, err) logEv(stderr, "HookInvocationError", "type", string(typ), "id", id, "error", err.Error()) MetricsHookErrorsTotal.WithLabelValues(string(typ)).Add(1) } else if Flags.VerboseOutput { logEv(stdout, "HookInvocationFinish", "type", string(typ), "id", id) } - // IDEA: PreHooks work like this: error return value does not carry HTTP response information + // IDEA: PreHooks work like this: error return value does carry HTTP response information for error // Instead the additional HTTP response return value httpRes = hookRes.HTTPResponse @@ -171,12 +170,10 @@ func invokeHookSync(typ hooks.HookType, event handler.HookEvent) (httpRes handle // If the hook response includes the instruction to reject the upload, reuse the error code // and message from ErrUploadRejectedByServer, but also include custom HTTP response values if typ == hooks.HookPreCreate && hookRes.RejectUpload { - // TODO: Merge httpRes with default of ErrUploadRejected, so we always have a response code. - return httpRes, handler.Error{ - ErrorCode: handler.ErrUploadRejectedByServer.ErrorCode, - Message: handler.ErrUploadRejectedByServer.Message, - HTTPResponse: httpRes, - } + err := handler.ErrUploadRejectedByServer + err.HTTPResponse = err.HTTPResponse.MergeWith(httpRes) + + return httpRes, err } if typ == hooks.HookPostReceive && hookRes.StopUpload { diff --git a/cmd/tusd/cli/hooks/hooks.go b/cmd/tusd/cli/hooks/hooks.go index 3f13fee..3f3ebbe 100644 --- a/cmd/tusd/cli/hooks/hooks.go +++ b/cmd/tusd/cli/hooks/hooks.go @@ -1,5 +1,7 @@ package hooks +// TODO: Move hooks into a package in /pkg + import ( "github.com/tus/tusd/pkg/handler" ) diff --git a/cmd/tusd/cli/hooks/plugin.go b/cmd/tusd/cli/hooks/plugin.go index d912c6e..cc33bc0 100644 --- a/cmd/tusd/cli/hooks/plugin.go +++ b/cmd/tusd/cli/hooks/plugin.go @@ -7,8 +7,12 @@ import ( "os/exec" "github.com/hashicorp/go-plugin" + "github.com/tus/tusd/pkg/handler" ) +// TODO: When the tusd process stops, the plugin does not get properly killed +// and lives on as a zombie process. + type PluginHook struct { Path string @@ -54,8 +58,8 @@ func (h *PluginHook) InvokeHook(req HookRequest) (HookResponse, error) { // directory. It is a UX feature, not a security feature. var handshakeConfig = plugin.HandshakeConfig{ ProtocolVersion: 1, - MagicCookieKey: "BASIC_PLUGIN", - MagicCookieValue: "hello", + MagicCookieKey: "TUSD_PLUGIN", + MagicCookieValue: "yes", } // pluginMap is the map of plugins we can dispense. @@ -63,19 +67,35 @@ var pluginMap = map[string]plugin.Plugin{ "hookHandler": &HookHandlerPlugin{}, } +// TODO: Explain, mention that it is internal only +// TODO: Do we actually need this? Maybe not... +type InvokeHookRPCAnswer struct { + HookResponse HookResponse + TusdError *handler.Error // Why is TusdError a pointer +} + // Here is an implementation that talks over RPC type HookHandlerRPC struct{ client *rpc.Client } func (g *HookHandlerRPC) Setup() error { var res interface{} err := g.client.Call("Plugin.Setup", new(interface{}), &res) - fmt.Println("after Setup") return err } -func (g *HookHandlerRPC) InvokeHook(req HookRequest) (res HookResponse, err error) { - err = g.client.Call("Plugin.InvokeHook", req, &res) - return res, err +func (g *HookHandlerRPC) InvokeHook(req HookRequest) (HookResponse, error) { + var answer InvokeHookRPCAnswer + err := g.client.Call("Plugin.InvokeHook", req, &answer) + fmt.Printf("Client: %#v\n", answer.TusdError) + if err != nil { + return answer.HookResponse, err + } + + if answer.TusdError != nil { + return answer.HookResponse, *answer.TusdError + } + + return answer.HookResponse, nil } // Here is the RPC server that HookHandlerRPC talks to, conforming to @@ -89,9 +109,21 @@ func (s *HookHandlerRPCServer) Setup(args interface{}, resp *interface{}) error return s.Impl.Setup() } -func (s *HookHandlerRPCServer) InvokeHook(args HookRequest, resp *HookResponse) (err error) { - *resp, err = s.Impl.InvokeHook(args) - return err +func (s *HookHandlerRPCServer) InvokeHook(args HookRequest, answer *InvokeHookRPCAnswer) error { + resp, err := s.Impl.InvokeHook(args) + if err != nil { + + if tusdErr, ok := err.(handler.Error); ok { + answer.TusdError = &tusdErr + return nil + } else { + return err + } + } + + answer.HookResponse = resp + + return nil } // This is the implementation of plugin.Plugin so we can serve/consume this diff --git a/examples/hooks/plugin/hook_handler.go b/examples/hooks/plugin/hook_handler.go index 9785d18..c679a51 100644 --- a/examples/hooks/plugin/hook_handler.go +++ b/examples/hooks/plugin/hook_handler.go @@ -26,6 +26,14 @@ func (g *MyHookHandler) InvokeHook(req hooks.HookRequest) (res hooks.HookRespons if req.Type == hooks.HookPreCreate { res.HTTPResponse.Headers["X-From-Pre-Create"] = "hello" + + if req.Event.Upload.Size > 10 { + res.HTTPResponse.StatusCode = 413 + res.HTTPResponse.Body = `{"error":"upload size is too large"}` + res.RejectUpload = true + + return res, nil + } } if req.Type == hooks.HookPreFinish { @@ -42,8 +50,8 @@ func (g *MyHookHandler) InvokeHook(req hooks.HookRequest) (res hooks.HookRespons // directory. It is a UX feature, not a security feature. var handshakeConfig = plugin.HandshakeConfig{ ProtocolVersion: 1, - MagicCookieKey: "BASIC_PLUGIN", - MagicCookieValue: "hello", + MagicCookieKey: "TUSD_PLUGIN", + MagicCookieValue: "yes", } func main() { diff --git a/pkg/handler/unrouted_handler.go b/pkg/handler/unrouted_handler.go index df7a3dc..21dfde4 100644 --- a/pkg/handler/unrouted_handler.go +++ b/pkg/handler/unrouted_handler.go @@ -118,18 +118,33 @@ func (resp HTTPResponse) writeTo(w http.ResponseWriter) { } } -func (resp *HTTPResponse) MergeWith(resp2 HTTPResponse) { - if resp2.StatusCode != 0 { - resp.StatusCode = resp2.StatusCode - } +func (resp1 HTTPResponse) MergeWith(resp2 HTTPResponse) HTTPResponse { + // Clone the response 1 and use it as a basis + newResp := resp1 - for key, value := range resp2.Headers { - resp.Headers[key] = value + // Take the status code and body from response 2 to + // overwrite values from response 1. + if resp2.StatusCode != 0 { + newResp.StatusCode = resp2.StatusCode } if len(resp2.Body) > 0 { - resp.Body = resp2.Body + newResp.Body = resp2.Body } + + // For the headers, me must make a new map to avoid writing + // into the header map from response 1. + newResp.Headers = make(HTTPHeaders, len(resp1.Headers)+len(resp2.Headers)) + + for key, value := range resp1.Headers { + newResp.Headers[key] = value + } + + for key, value := range resp2.Headers { + newResp.Headers[key] = value + } + + return newResp } // HookEvent represents an event from tusd which can be handled by the application. @@ -400,7 +415,7 @@ func (handler *UnroutedHandler) PostFile(w http.ResponseWriter, r *http.Request) handler.sendError(w, r, err) return } - resp.MergeWith(resp2) + resp = resp.MergeWith(resp2) } upload, err := handler.composer.Core.NewUpload(ctx, info) @@ -770,7 +785,7 @@ func (handler *UnroutedHandler) finishUploadIfComplete(ctx context.Context, uplo if err != nil { return resp, err } - resp.MergeWith(resp2) + resp = resp.MergeWith(resp2) } } @@ -1035,7 +1050,7 @@ func (handler *UnroutedHandler) sendError(w http.ResponseWriter, r *http.Request func (handler *UnroutedHandler) sendResp(w http.ResponseWriter, r *http.Request, resp HTTPResponse) { resp.writeTo(w) - handler.log("ResponseOutgoing", "status", strconv.Itoa(resp.StatusCode), "method", r.Method, "path", r.URL.Path, "requestId", getRequestId(r)) + handler.log("ResponseOutgoing", "status", strconv.Itoa(resp.StatusCode), "method", r.Method, "path", r.URL.Path, "requestId", getRequestId(r), "body", resp.Body) } // Make an absolute URLs to the given upload id. If the base path is absolute