Workhorse: Do not prepare uploads for non-multipart requests
What does this MR do and why?
In the defaultUpstream
, we currently install a request handler that treats every Workhorse request as if it was an upload. This was initially done so as to buffer multipart form-posts on disk prior to Rails unpacking these requests.
This is not as bad as it sounds, since we do fail down the call-chain in rewrite.go
when trying to open a MultipartReader
for the request, which fails if it isn't actually a multipart. However this happens further down the stack, which has the following problems:
- We call
Prepare
on the default upload preparer, which performs work anticipating an Object Storage upload to happen. This isn't very expensive, but it's unnecessary and conceivable that a developer might add expensive logic to this step, not knowing that this is called for every unproxied request we serve. - The behavior is surprising; no one would expect any of this to happen, unless we are sure we are proxying an upload.
The way I solved this is as follows:
- Introduce a new type,
MultipartFormPreProcessor
, responsible for creating save options and constructing a processor that is handed off to. - Move upload preparation logic into instances of this type. (Logic itself remains unchanged.)
- Push invocation of
PreProcess
down intouploads.HandleFileUploads
; this allows us to centralize control flow in one place. - Add an early return in
HandleFileUploads
if we fail to construct aMultipartReader
for the current request.
Two other approaches I discarded:
- Return early in
Accelerate
by inspecting theContent-Type
. It was pointed out to me that this comes with security implications since we may fail to identify uploads that would sneak past us, so we should rely on Golang'sMultipartReader
type instead to identify multipart uploads. - Construct a
MultipartReader
in the caller ofHandleFileUploads
and pass it down the stack. This ended up being quite messy, because it would duplicate logic of testing its return value (it can return an error) in several places. Even if we centralized this somewhere, it was breaking every single test forHandleFileUploads
, since some tests were testing error cases in which that construction would fail; we'd have to rewrite them all. It also made the interface ergonomics worse, since suddenly it could happen thatHandleFileUploads
is invoked with anil
reader, should it fail to be constructed.
MR acceptance checklist
This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.
-
I have evaluated the MR acceptance checklist for this MR.
Edited by Matthias Käppler