RFC: Follow redirects when downloading assets for cloning #5335

kalikiana · 2023-10-18T13:06:05Z

Note: Untested by me as I couldn't reproduce the issue.

Martchus · 2023-10-18T13:12:10Z

lib/OpenQA/Script/CloneJob.pm

@@ -141,6 +141,7 @@ sub clone_job_download_assets ($jobid, $job, $url_handler, $options) {

            print STDERR "downloading\n$from\nto\n$dst\n";
            my $r = $ua->mirror($from, $dst);
+            $r = $ua->mirror($r->header('Location'), $dst) if ($r->code == 308);


What if Location is undef? What if there's yet another redirection? We should also not follow a location from https to http.

But generally a good idea of course.

@Martchus FYI in my case it was the redirect from http to https on o3.

I know. I pointed that out in the chat. And we should of course allow this case. What we should not allow is the opposite for security reasons.

Maybe add a comment that this is a temporary fix until LWP 6.48 is available on all our supported platforms?

@kalikiana I suppose this is expected you add before merge, right @okurz ?

yes, or anyone else who can add a commit to this PR

Apparently we can't achieve with a reasonable effort to update LWP in SLE/Leap hence we need to keep this workaround in place until eventually we use only more recent versions in newer OS versions

Suggested change

$r = $ua->mirror($r->header('Location'), $dst) if ($r->code == 308);

# Workaround for behaviour LWP < 6.48, see https://github.com/libwww-perl/libwww-perl/pull/349

$r = $ua->mirror($r->header('Location'), $dst) if $r->code == 308;

@pevik Maybe it's best if you take over from here? I don't even remember what issue this was addressing.

pevik

Works as expected, thanks! (I reported the issue and also test this fix).

Martchus · 2023-10-18T13:22:53Z

Maybe we just need to tweak our usage of LWP (e.g. via https://metacpan.org/pod/LWP::UserAgent#requests_redirectable) instead of trying to implement the behavior manually.

Note that some HTTP requests like posting the job are done via Mojolicious and judging by the code (and my tests) they definitely follow up to 3 redirections.

Note: Untested by me as I couldn't reproduce the issue.

Good point. I've just tried to reproduce it here as well and also wasn't able to:

script/openqa-clone-job --dir /hdd/openqa-devel/openqa/share/factory --show-progress --host http://localhost:9526 http://openqa.opensuse.org/tests/3655171
downloading
http://openqa.opensuse.org/tests/3655171/asset/hdd/obs-server.x86_64-2.10.51-qcow2-Build26.586.qcow2
to
/hdd/openqa-devel/openqa/share/factory/hdd/obs-server.x86_64-2.10.51-qcow2-Build26.586.qcow2
** GET http://openqa.opensuse.org/tests/3655171/asset/hdd/obs-server.x86_64-2.10.51-qcow2-Build26.586.qcow2 ==> 308 Permanent Redirect
** GET https://openqa.opensuse.org/tests/3655171/asset/hdd/obs-server.x86_64-2.10.51-qcow2-Build26.586.qcow2 ==> 302 Found
** GET https://openqa.opensuse.org/assets/hdd/obs-server.x86_64-2.10.51-qcow2-Build26.586.qcow2 ==> 200 OK (159s)
1 job has been created:
 - obs-Unstable-Appliance-x86_64-Build2.10.51-qcow2-26.586-obs_appliance@64bit-4G -> http://localhost:9526/tests/3757

I only used http here and as shown in the logs it follows the redirect. It also works without asset downloads.

codecov · 2023-10-18T13:25:25Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.32%. Comparing base (b25b3ea) to head (407cbc2).
Report is 1245 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #5335   +/-   ##
=======================================
  Coverage   98.32%   98.32%           
=======================================
  Files         389      389           
  Lines       37286    37287    +1     
=======================================
+ Hits        36660    36661    +1     
  Misses        626      626

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

perlpunk · 2023-10-19T10:29:04Z

I was able to reproduce it on Leap 15.4 with LWP::UserAgent 6.31:

./script/openqa-clone-job --host http://localhost:9526 --skip-chained-deps --max-depth 0 --parental-inheritance --from http://openqa.opensuse.org/tests/3650601
Cloning parents of opensuse-Tumbleweed-DVD-x86_64-Build20231016-ltp_cve@64bit
downloading
http://openqa.opensuse.org/tests/3650601/asset/hdd/[email protected]
to
/home/tina/openqa-toolbox/openqa/share/factory/hdd/[email protected]
3650601 failed: [email protected], 308 Permanent Redirect
Can't clone due to missing assets: 308 Permanent Redirect

It was fixed in 6.48: https://metacpan.org/dist/libwww-perl/changes#L98

6.48      2020-09-20 15:25:51Z
    - Support 308 Permanent Redirect (GH#349) (Galen Huntington)

Martchus · 2023-10-19T14:42:11Z

Then I suggest to add libwww-perl to our Leap 15.4 specific devel repository instead of introducing any extra handling on our side.

perlpunk · 2023-10-19T19:59:57Z

Also 15.5, as we still have the same version there...

okurz · 2023-10-20T05:08:07Z

Then I suggest to add libwww-perl to our Leap 15.4
As I don't see the issue as critical how about just submitting to Leap as maintenance update and wait for that?

pevik · 2023-12-07T06:19:58Z

@Martchus @okurz Could we please just merge this one liner fix? It saves me adding it manually all the time because perl-libwww-perl got newer updated in Leap 15.5.

okurz · 2023-12-07T06:34:09Z

@Martchus @okurz Could we please just merge this one liner fix? It saves me adding it manually all the time because perl-libwww-perl got newer updated in Leap 15.5.

Yes, as soon as the comment from perlpunk is followed

Martchus · 2023-12-07T11:49:29Z

Sounds like we should just link the latest version in our devel repos (but I currently cannot do it because OBS is overloaded).

Martchus · 2023-12-07T16:36:59Z

I linked the package via osc linkpac openSUSE:Factory perl-libwww-perl devel:openQA:Leap:15.5 but it is unresolvable: https://build.opensuse.org/package/show/devel:openQA:Leap:15.5/perl-libwww-perl

nothing provides perl(HTTP::Daemon) >= 6.12, (got version 6.01 provided by perl-HTTP-Daemon), nothing provides perl(HTTP::Request) >= 6.18, (got version 6.14 provided by perl-HTTP-Message), nothing provides perl(HTTP::Request::Common) >= 6.18, (got version 6.14 provided by perl-HTTP-Message), nothing provides perl(HTTP::Response) >= 6.18, (got version 6.14 provided by perl-HTTP-Message), nothing provides perl(HTTP::Status) >= 6.18, (got version 6.14 provided by perl-HTTP-Message), nothing provides perl(Net::HTTP) >= 6.18, (got version 6.17 provided by perl-Net-HTTP)

That are a lot of dependencies. So should we go down that path or just keep things as-is (which would make sense for a "stable" distribution like Leap)?

okurz · 2023-12-08T08:33:49Z

I linked the package via osc linkpac openSUSE:Factory perl-libwww-perl devel:openQA:Leap:15.5 but it is unresolvable: https://build.opensuse.org/package/show/devel:openQA:Leap:15.5/perl-libwww-perl

It is interesting to see that according to
https://software.opensuse.org/package/perl-libwww-perl?search_term=perl-libwww-perl

perl-libwww-perl is part of Tumbleweed but not in Leap although it was part of Leap until 15.2. I wonder if there is a better way to do it then without needing to pull this in again. But with such requirements and dependencies missing the original approach is obviously not straight forward. Maybe we can think of alternative approaches then?

Martchus · 2023-12-08T12:44:51Z

We could also just move forward with the PR as an alternative approach. It only needs minor improvements to be mergable:

Handle the case when Location is undef
Handle the case of multiple redirections but with a limit to avoid getting stuck in a redirection loop
Disallow redirections from https to http
Add a comment in the code to make it clear that this is to support versions of LWP::UserAgent older than 6.48

okurz · 2023-12-08T13:17:47Z

So, is this really only a workaround for OSs like Leap 15.4 which goes EOL in a couple of weeks if not already? Current is Leap 15.5 but so far I did not understand which packages of Leap 15.5 this would involve

Martchus · 2023-12-08T13:42:44Z

According to @pevik Leap 15.5 is also affected:

It saves me adding it manually all the time because perl-libwww-perl got newer updated in Leap 15.5.

(I think "newer" is supposed to be "never".)

That's also why I invoked the linkpac command for Leap 15.5 and it involves the package I tried to link (and the unresolved dependencies).

okurz · 2023-12-08T15:31:31Z

So why not fix it at the root and submit an update to Leap?

pevik · 2024-06-18T14:35:21Z

So why not fix it at the root and submit an update to Leap?

I finally submitted the package to Leap 15.6: https://build.opensuse.org/request/show/1181525. Feel free to comment.

pevik · 2024-06-19T12:50:38Z

@kalikiana OK, the update is unlikely to be accepted and even if it is accepted @okurz proposed to have this as a temporary fix until not needed. Could you please rebase? I'll retest it for you. FYI I've been using this code in my instance, because it's needed.

Martchus · 2024-06-19T13:38:57Z

As mentioned in the chat: If you really need this, please make sure to not allow redirections from https to http and make sure it cannot be stuck in an endless loop.

pevik · 2024-10-17T14:24:15Z

FYI I keep using this patch (it's still needed). And I'm not sure if submitting package to openSUSE:Leap:15.6:Update would be accepted (suggested at rejected SR https://build.opensuse.org/request/show/1181525).

okurz · 2024-10-17T17:03:17Z

I don't see a problem why the patch shouldn't be acceptable for SLE or Leap. https://build.opensuse.org/request/show/1181525 looks like you revoked it even though nobody rejected, or is it?

kalikiana · 2024-10-18T09:22:30Z

I don't see a problem why the patch shouldn't be acceptable for SLE or Leap. https://build.opensuse.org/request/show/1181525 looks like you revoked it even though nobody rejected, or is it?

https://build.opensuse.org/request/show/1181525#comment-1950196

RFC: Follow redirects when downloading assets for cloning

407cbc2

Note: Untested by me as I couldn't reproduce the issue.

Martchus requested changes Oct 18, 2023

View reviewed changes

pevik approved these changes Oct 18, 2023

View reviewed changes

kalikiana closed this Oct 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: Follow redirects when downloading assets for cloning #5335

RFC: Follow redirects when downloading assets for cloning #5335

kalikiana commented Oct 18, 2023

Martchus Oct 18, 2023

Martchus Oct 18, 2023

pevik Oct 18, 2023

Martchus Oct 18, 2023

perlpunk Oct 19, 2023

pevik Dec 7, 2023

okurz Dec 7, 2023

okurz Jun 19, 2024

kalikiana Jun 19, 2024

pevik left a comment

Martchus commented Oct 18, 2023 •

edited

Loading

codecov bot commented Oct 18, 2023 •

edited

Loading

perlpunk commented Oct 19, 2023

Martchus commented Oct 19, 2023

perlpunk commented Oct 19, 2023

okurz commented Oct 20, 2023

pevik commented Dec 7, 2023

okurz commented Dec 7, 2023

Martchus commented Dec 7, 2023 •

edited

Loading

Martchus commented Dec 7, 2023 •

edited

Loading

okurz commented Dec 8, 2023

Martchus commented Dec 8, 2023 •

edited

Loading

okurz commented Dec 8, 2023

Martchus commented Dec 8, 2023

okurz commented Dec 8, 2023

pevik commented Jun 18, 2024

pevik commented Jun 19, 2024

Martchus commented Jun 19, 2024

pevik commented Oct 17, 2024

okurz commented Oct 17, 2024

kalikiana commented Oct 18, 2024

	$r = $ua->mirror($r->header('Location'), $dst) if ($r->code == 308);
	# Workaround for behaviour LWP < 6.48, see https://github.com/libwww-perl/libwww-perl/pull/349
	$r = $ua->mirror($r->header('Location'), $dst) if $r->code == 308;

RFC: Follow redirects when downloading assets for cloning #5335

RFC: Follow redirects when downloading assets for cloning #5335

Conversation

kalikiana commented Oct 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pevik left a comment

Choose a reason for hiding this comment

Martchus commented Oct 18, 2023 • edited Loading

codecov bot commented Oct 18, 2023 • edited Loading

Codecov Report

perlpunk commented Oct 19, 2023

Martchus commented Oct 19, 2023

perlpunk commented Oct 19, 2023

okurz commented Oct 20, 2023

pevik commented Dec 7, 2023

okurz commented Dec 7, 2023

Martchus commented Dec 7, 2023 • edited Loading

Martchus commented Dec 7, 2023 • edited Loading

okurz commented Dec 8, 2023

Martchus commented Dec 8, 2023 • edited Loading

okurz commented Dec 8, 2023

Martchus commented Dec 8, 2023

okurz commented Dec 8, 2023

pevik commented Jun 18, 2024

pevik commented Jun 19, 2024

Martchus commented Jun 19, 2024

pevik commented Oct 17, 2024

okurz commented Oct 17, 2024

kalikiana commented Oct 18, 2024

Martchus commented Oct 18, 2023 •

edited

Loading

codecov bot commented Oct 18, 2023 •

edited

Loading

Martchus commented Dec 7, 2023 •

edited

Loading

Martchus commented Dec 7, 2023 •

edited

Loading

Martchus commented Dec 8, 2023 •

edited

Loading