feat: do not fail if server returns 200 OK

[scabala]

Another rather easy fix.

Closes #958

FYI @dmacvicar @volker-raschek @hansingt

[hansingt]

Thanks for the fix @scabala!

Some thoughts I have about this change and the code here in general:

If we accept HTTP 200 responses here, the response might contain the whole image. This depends on whether the http go module supports streaming or not. But I'd guess it does.

If it does, only the first chunks will be received until the network buffer is filled. Thus, this is not that much of waste of network traffic, but should be considered. Maybe, it should be considered / checked, whether the IsQCOW2 check could be merged with the Import method, to download things only once? But this might be a separate issue.

Anyway, right now, the io.ReadAll() in Line 147 causes the whole image to be downloaded. This does not only block until the download is complete, it does also waste (possibly several gigabytes) memory, or might even fail, if the computer runs out of memory. This depends on the size of the image to be downloaded.

We already know, that we only need the first 8 bytes of the file, thus, it would be better to only read that much. Maybe
io.ReadFull would be correct here?

Last, but not least, actually, all HTTP Codes in the 200-299 range are to be considered as successful responses, but that doesn't really matter here, because the other status codes defined in this range arn't important in this case (see https://developer.mozilla.org/en-US/docs/Web/HTTP/Status#successful_responses).

Besides this, this change looks good for me! 👍

[scabala]

Thanks for review.

First of all, I was aiming for minimal change as I do not (yet) know codebase very well. In general, it should work well but could be improved.

I'll try to use io.ReadFull, should be a better fit.

I also thought about all 2xx status codes but decided to keep it simple and minimal change. However, if others deem it worth it, I'll change it to accept other 2xx status codes as well.

[dmacvicar]

I understand the ReadFull is harmless if the server answered with StatusPartialContent.

I don't get why we should support servers that do not accept ranges? What problem are we trying to solve?

[hansingt]

I understand the ReadFull is harmless if the server answered with StatusPartialContent.

I don't get why we should support servers that do not accept ranges? What problem are we trying to solve?

Hi @dmacvicar,

Please see #958 (comment):

There are HTTP-Servers, which do not support Range-Requests, like the talos download servers. And according to the RFC 9110, they don't have to, as it is an optional feature.

In this case, the server is responding with a normal HTTP 200 response which contains the whole image.

Thus, to support these servers, it would be better to explicitly only read the first bytes required from the request, to avoid loading the whole image into memory.