Allow object storage acceleration from JSON encoded body

This issue originates from gitlab-org/gitlab-ce#63097

Our NPM package doesn't have workhorse upload acceleration, and members of the wider community are contributing new features based on that https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/9012#note_179607151

The reason being is that we don't have a body hijacking handler for JSON encoded requests.

What happens with these kinds of uploads is that a file is provided in base64 encoding inside the JSON body.

This is how we handle NPM Object Storage in gitlab-workhorse uploading on rails side.

Proposal

We should implement a handler that can hijack JSON encoded body, stripping base64 encoded files and replacing them with the usual metadata workhorse generates on upload acceleration.

/cc @marin @dzaporozhets

added bugperformance grouppackage registry + 1 deleted label

I agree with proposal. I think its better compared to current implementation

@nolith I also think this makes sense. I wonder how we would detect files though? E.g. which JSON field in an NPM JSON body gets treated as an "upload". Is there a way for us to know, or do we have to register each "JSON-hidden upload" route in workhorse along with the upload fields?

Is there a way for us to know, or do we have to register each "JSON-hidden upload" route in workhorse along with the upload fields?

I was thinking about registering routes and fields as the first iteration. We can improve it later

mentioned in merge request gitlab-com/www-gitlab-com!26287 (merged)

added workflowready for development label

Setting gitlab-workhorse31034541 based on gitlab-workhorse10690711.

added devopspackage label

mentioned in merge request !17417 (closed)

added package:active label

added 1 deleted label and removed package:active label

added workflowscheduling label and removed workflowready for development + 1 deleted label

added teamDelivery label

mentioned in issue gitlab-foss#63097 (closed)

removed workflowscheduling label

added sectionops label

mentioned in issue #243780 (closed)

Moving this issue because of &4826 (closed).

added workhorse label

moved from gitlab-workhorse#226 (moved)

mentioned in issue gitlab-org/quality/triage-reports#2407 (closed)

added Category:Package Registry label

changed milestone to %Backlog

added [deprecated] Accepting merge requests label

mentioned in merge request !57625 (merged)

changed milestone to %14.4

added Package:P1 label

mentioned in issue #337966 (closed)

mentioned in issue #243780 (closed)

@sabrams @10io @hortiz5 @michelletorres I was looking to prioritize some performance enhancements and remember discussing moving npm to workhorse. I see we have this issue and #13078 to do so. What is the expected impact of this change? Can we do this work or do we need help from other teams?

Are there other performance or reliability issues that you think are higher priority than this?

The change is in two parts:

1. Workhorse changes (weight 3)
   • Read the uploaded file from the json encoded body instead of the whole body or a multipart form.
   • The rest of the upload processing (in particular, the :ping_pong:s stays the same).
   • The uploaded file should be in uploaded file params (the ones that are JWT signed) as usual.
     • Consider using the same approach as for multipart uploads as we could have multiple files in a single json structure.
   • caution must be taken here to not put the whole body in memory to (json) decode it.
   • Ideally, this change should be done in a generic way so that we can programmatically code where the uploaded file is in the json structure (perhaps using the json path with the dot notation)
     • This is to support any upload that would be json encoded
2. Rails changes (weight 2)
   • Implement the /authorize endpoint
   • Update the upload endpoint to actually read the file passed by Workhorse (and processed by the rails middleware)

(2.) is the usual code area of the package team. I don't see any major obstacle here.

(1.) is limited in scope (it's just a matter of reading the uploaded file in a different way that we currently can) but we have less visibility here as it's not our usual code area although we already worked on package uploads. This change has more grey areas for us (package team). That's why I would put a 3.

Having said that, this provides a good opportunity for the Package team to extend its knowledge on Workhorse in general, as it is a piece of code we need to look at (and update) from time to time. We might need to layout a solution for (2.) and ping a Workhorse maintainer to acknowledge the overall direction.

Lastly, the npm Registry is one of the most used registry on gitlab.com. A feature flag must be used here.

What is the expected impact of this change?

This should lower the amount of time to process an npm package upload. Related Kibana chart.

NPM uploads would gain the benefits of workhorse accelerated uploads.

Lastly, we would have a consistent approach to all uploads in the Package code area: they would all be handled by workhorse.

@trizzi Does that answer your questions?

Just to clarify, this issue specifically addresses the workhorse changes; part 1 of David's answer. Part 2 is captured in #13078.

Thank you @10io, that is really helpful. Looking at the plan for milestone 14.4, it's looking tight to take on this project. But maybe we can take one step forward to create a layout for (2.) and ping a Workhorse maintainer? Then we can schedule the implementation issues in a later milestone.

added golang label

set weight to 3

changed milestone to %14.5

mentioned in issue #341294 (closed)

mentioned in issue #342356

changed milestone to %14.6

marked this issue as related to #342356

changed milestone to %Backlog

added to epic &7037

removed Package:P1 label

added typemaintenance label