Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

coreos-installer: Honor redirects via curl #28

Merged
merged 1 commit into from May 13, 2019

Conversation

Projects
None yet
6 participants
@ashcrow
Copy link
Contributor

commented May 13, 2019

Adds -L to all curl calls to honor redirects.

See: https://bugzilla.redhat.com/show_bug.cgi?id=1709247

coreos-installer: Honor redirects via curl
Fixes RHBZ#1709247

See: https://bugzilla.redhat.com/show_bug.cgi?id=1709247

Signed-off-by: Steve Milner <smilner@redhat.com>

@ashcrow ashcrow added the bug label May 13, 2019

@ashcrow ashcrow requested review from lucab and yuqi-zhang May 13, 2019

@miabbott

This comment has been minimized.

Copy link

commented May 13, 2019

Looks sane to me 👍

@ashcrow ashcrow changed the title coreos-installer: Honor redirects via curl WIP: coreos-installer: Honor redirects via curl May 13, 2019

@ashcrow

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

Setting WIP until after full testing.

@dustymabe

This comment has been minimized.

Copy link
Member

commented May 13, 2019

Looks sane to me +1

agree. tagging in @yuqi-zhang for a review since he's been handling the RHCOS side of the installer.

@jlebon

This comment has been minimized.

Copy link
Member

commented May 13, 2019

Looks sane to me as well. Might be worth factoring out some helper functions to not regress on this in the future, e.g. http_head() (for the -I case) and http_get() or something.

@arithx

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

lgtm

@yuqi-zhang

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

Hum, I created a redirect from localhost:8080->localhost:8081. If I do something like
wget http://192.168.122.1:8080/r8-bios.raw.gz I see the redirect and the GET request fine.

But even with this patch, with the installer, it doesn't actually do a GET for 8081. It sees the 301 from 8080 and says: oh that's fine, and proceeds to continue and then errors out after the reboot since it never got an image. It also can't get the ignition url either. (But it fails correctly for ignition and complains that it can't find the pre-redirect URL)

@ashcrow

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

Following up in a higher bandwidth environment to help debug.

@yuqi-zhang

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

Update: actually it will error for both (complains it can't find the image). I suspect its something else thats the problem and not just the curl acting up

@ashcrow ashcrow changed the title WIP: coreos-installer: Honor redirects via curl coreos-installer: Honor redirects via curl May 13, 2019

@ashcrow

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

Removing WIP as @yuqi-zhang's testing has succeeded.

@yuqi-zhang
Copy link
Contributor

left a comment

Upon further testing this works for me. I think this also circumnavigates the issue of failed fetches being silently ignored at least for this case. But it may be something to double check we handle it in all cases?

Either case LGTM. Merging

@yuqi-zhang yuqi-zhang merged commit d3fc540 into coreos:master May 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.