Upload config fail #2483

[kanecko]

Codefix provided by @FroggyFlox .
Tested by @kanecko on 15.4 V4.5.5-0.

[EDIT]
Fixes #2483
See issue text/comments for details.

[FroggyFlox]

Thanks a lot, @kanecko, for taking the time to create this PR and of course for testing the proposed fix in #2483.

A bit of explanation here, for reference.

Although I cannot reproduce exactly the issue detailed in #2483, I can reproduce the HTTP 400 error (Bad request) upon uploading a config backup file. In my case, however, no response content at all is returned and nothing can be found in the logs (rockstor logs, or gunicorn logs). The only error I could find (in addition to the HTTP 400 response) was the following in /var/log/nginx/error.log:

2023/01/23 16:08:33 [error] 14064#14064: *313 readv() failed (104: Connection reset by peer) while reading upstream, client: 192.168.122.1, server: ~^(?<myhost>.+)$, request: "POST /api/config-backup/file-upload HTTP/1.1", upstream: "http://127.0.0.1:8000/api/config-backup/file-upload", host: "buildvm", referrer: "https://buildvm/"

I thus looked further into gunicorn but I couldn't find any indication of error in its logs (even after increasing its log level). Increasing request size limits in nginx, gunicorn, and even Django itself didn't make a difference.

Looking back into our Django side of things, we are dealing with this upload request in the ConfigBackupUpload() class:

rockstor-core/src/rockstor/storageadmin/views/config_backup.py

Lines 628 to 629 in c99f9f8

	class ConfigBackupUpload(ConfigBackupMixin, rfc.GenericView):
	parser_classes = (FileUploadParser, MultiPartParser)

We can here see we are using two parsers defined by Django Rest Framework: FileUploadParser and MultiPartParser. Their docs state:

The FileUploadParser is for usage with native clients that can upload the file as a raw data request. For web-based uploads, or for native clients with multipart upload support, you should use the MultiPartParser instead.

This thus recommends the use of MultiPartParser in our case, instead of FileUploadParser. We currently use both, which seems to conflict here.

The question is why is this showing up as a problem now for us, however, as this has always been the case (and working thus far). Note that #2483 appeared in 4.5.4, which could thus be related to our Django version update, or Django Rest Framework update.

@phillxnet, as you can see above, the reason why the error appeared is still uncleared to me and maybe we should look into this a bit further to make sure we're not missing the real reason for #2483 here. It may not be worth wasting too much time on that, however, as given DRF's docs quoted above, using only MultiPartParser seems like the appropriate way to go in our case upon a brief look.

[phillxnet]

@kanecko Thanks from me also for testing and preparing this pr and attributing @FroggyFlox . Much appreciated.

@FroggyFlox Thanks for the exposition here, always best if we can have some context as to why, or why we think :) , a change was required. Other than the operational definition of course.

It may not be worth wasting too much time on that, however, as given DRF's docs quoted above, using only MultiPartParser seems like the appropriate way to go in our case upon a brief look.

Agreed. I'll do a final build test and if all is well I'll get this merged ready for our pending 4.5.6-0 release.

[phillxnet]

And rpm built and installed with this patch applied, successfully passed the testing outlined by @FroggyFlox in the following comment #2483 (comment) under the linked issue of #2483:

[25/Jan/2023 16:01:45] INFO [storageadmin.tasks:64] Task [restore_rockons], id: 70715cb9-7087-455c-8dc2-3743f2aad96f completed OK