Skip to content

Workhorse: Do not prepare uploads for non-multipart requests

Matthias Käppler requested to merge mk-wh-return-early-unless-multipart into master

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 into uploads.HandleFileUploads; this allows us to centralize control flow in one place.
  • Add an early return in HandleFileUploads if we fail to construct a MultipartReader for the current request.

Two other approaches I discarded:

  1. Return early in Accelerate by inspecting the Content-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's MultipartReader type instead to identify multipart uploads.
  2. Construct a MultipartReader in the caller of HandleFileUploads 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 for HandleFileUploads, 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 that HandleFileUploads is invoked with a nil 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.

Edited by Matthias Käppler

Merge request reports