Improved image upload process

[ipanova]

[noissue]
Required PR: pulp/pulpcore#2779

[mdellweg]

I'm not sure i understand the change properly, but i have the feeling, that one would be able to "steal" an artifact here by claiming to upload a blob with a certain checksum, not showing any chunks to the server and then committing said upload object.

[ipanova]

I'm not sure i understand the change properly, but i have the feeling, that one would be able to "steal" an artifact here by claiming to upload a blob with a certain checksum, not showing any chunks to the server and then committing said upload object.

I would not be so worried about artifact stealing because we have many other windows when it can happen. For example in the sync pipeline. Artifact can be stolen by the time it reaches Content stage.
This PR change becomes clear and simple once you grasp the docker api specs. Docker client uses it a weird way - there is monolithic upload and chunked upload. Monolithic upload starts with POST by creating upload and then proceeds with PUT where one big chunk would be uploaded.
Chunk upload starts with POST by creating upload and then proceeds with PATCH where each PATCH contains a chunk with the range header. Then it finishes with PUT where it is allowed to whether submit last chunk or just empty body. This the signal of 'i have finished my chunked upload'.
Docker uses a hybrid of these 2 approaches. It uses chunked upload, but sends just one chunk and does not seem to use range header at all :)
Because of this hybrid approach we just perform extra actions like - create a chunk, save it to storage, retrieve all upload chunks( always one ) read it back from storage, assemble artifact from 1 chunk ( i.e. again write to storage)

[mdellweg]

I do not think there is a way to steal artifacts by using uploads in the current implementation. (I agree, until we have all the necessary queryset-scoping on the artifacts the holes are wide open in other areas, but we intend to close them.)
Creating the blob when the big chunk arrives is probably not possible, because that is really the duty of that last PUT call. So i would try to keep the artifact attached to the upload with on_delete=PROTECT.
Another approach may be to see if we can rename the single chunk in the storage backend without moving data around to turn it into an artifact.

[ipanova]

I don't understand your last comment. This PR does not address or solve the artifact stealing issues at all - it focuses on performance. The data is not read nor written twice. This is important because even calculating the digest of the same data adds time. More importantly how object storage is involved here because users pay money for every write/read request to the bucket.

[mdellweg]

I don't understand your last comment. This PR does not address or solve the artifact stealing issues at all - it focuses on performance. The data is not read nor written twice. This is important because even calculating the digest of the same data adds time. More importantly how object storage is involved here because users pay money for every write/read request to the bucket.

What i wanted to say is that it introduces a new way to steal an artifact. One more we that need to think about later if we don't solve it right away.

[mdellweg]

One last concern i found.