Skip to content

cmd-buildupload: Set Content-Type and Content-Encoding headers#862

Merged
jlebon merged 1 commit intocoreos:masterfrom
jlebon:pr/fix-content-type
Nov 20, 2019
Merged

cmd-buildupload: Set Content-Type and Content-Encoding headers#862
jlebon merged 1 commit intocoreos:masterfrom
jlebon:pr/fix-content-type

Conversation

@jlebon
Copy link
Copy Markdown
Member

@jlebon jlebon commented Oct 18, 2019

Automatically detect the type of data being uploaded and set the
Content-{Type,Encoding} headers accordingly if not overriden.

This allows one to do curl --compressed to decompress on the fly. Note
we don't automatically do the Content-Disposition: inline; filename=
trick; this was found to be more confusing than useful in practice.

Closes: coreos/fedora-coreos-tracker#295

Comment thread src/cmd-buildupload Outdated
Comment thread src/cmd-buildupload Outdated
@jlebon jlebon force-pushed the pr/fix-content-type branch 2 times, most recently from cbd66eb to 661cbe9 Compare October 18, 2019 21:05
Comment thread src/cmd-buildupload
Comment thread src/cmd-buildupload
@dustymabe
Copy link
Copy Markdown
Member

where are we on this? a few outstanding comments?

Automatically detect the type of data being uploaded and set the
`Content-{Type,Encoding}` headers accordingly if not overriden.

This allows one to do `curl --compressed` to decompress on the fly. Note
we don't automatically do the `Content-Disposition: inline; filename=`
trick; this was found to be more confusing than useful in practice.

Closes: coreos/fedora-coreos-tracker#295
@jlebon jlebon force-pushed the pr/fix-content-type branch from 661cbe9 to 61b5442 Compare November 19, 2019 17:39
@jlebon
Copy link
Copy Markdown
Member Author

jlebon commented Nov 19, 2019

OK, updated this! ⬆️

Copy link
Copy Markdown
Member

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jlebon jlebon merged commit b658611 into coreos:master Nov 20, 2019
@jlebon jlebon deleted the pr/fix-content-type branch November 20, 2019 14:52
@cgwalters
Copy link
Copy Markdown
Member

cgwalters commented Dec 18, 2019

So this turned out to be another change for how RHCOS ships qcow2 images and broke some OpenStack code trying to download it.

Previously, RHCOS switched to match FCOS in having an explicit .gz extension and not using content headers. Now this change introduces a new case where we have the .gz extension and Content-Encoding.

One argument made in chat about this is that using both is wrong.

The reason this breaks is because the Golang HTTP stack will transparently decompress if it detects Content-Encoding, so code doing a download will end up with a .gz file that isn't compressed.

Probably...we should just revert this and stop trying to use headers entirely?

cgwalters added a commit to cgwalters/coreos-assembler that referenced this pull request Dec 18, 2019
coreos#862 (comment)
This regressed OpenShift on OpenStack installs, which had adjusted
to deal with the lack of `Content-Encoding` and a switch to add `.gz`
to images.

Basically, if we're going to say things are explicitly compressed,
let's not also tell people to *transparently* decompress them because
it's easy to end up with a file named `.gz` that was decompressed.
@hardys
Copy link
Copy Markdown

hardys commented Dec 18, 2019

Yeah the problem is we made changes now to consume the qcow2.gz files, but there are still some curl --compressed calls which have now started unzipping the qcow2.gz files, so the later explicit gz fails.

IMHO we should pick one approach or the other, and since we've already dealt with the fallout of moving away from transport level compression, it seems reasonable to stick with the file extension approach consistently?

cgwalters added a commit that referenced this pull request Dec 18, 2019
#862 (comment)
This regressed OpenShift on OpenStack installs, which had adjusted
to deal with the lack of `Content-Encoding` and a switch to add `.gz`
to images.

Basically, if we're going to say things are explicitly compressed,
let's not also tell people to *transparently* decompress them because
it's easy to end up with a file named `.gz` that was decompressed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Release artifact content types set to binary/octet-stream

5 participants