From 0340774523d7b11bd54c59780e1504d12d697f3d Mon Sep 17 00:00:00 2001 From: Adam Williamson Date: Wed, 18 Nov 2020 10:02:08 -0800 Subject: [PATCH 4/4] parse_url_settings: don't parse value for non-asset types If a test sets NOT_ASSET_URL: http://some/parseable.url , we don't actually want to parse that to NOT_ASSET: parseable.url , because we aren't going to download it later. We had this bug back in 2016 and fixed it specifically in f54a18f , then regressed it again in 391f95e3 because we needed to pass the parsed file name to the asset type check. When we split up `parse_url_settings` and `create_downloads_list` the asset type check wound up in `create_downloads_list`. To fix the bug we can't avoid duplicating it here - we do have to check it in both places, here to avoid setting $settings->{$short} if it's not an asset type, and there to avoid doing the download if somehow both NOT_ASSET_URL and NOT_ASSET wind up being set by templates or the scheduler. It's not worth splitting the check to a shared utility function as you still need flow control. I don't think the log message will ever be duplicated as we should never manage to reach and fail the check *both* times (only one or the other). Signed-off-by: Adam Williamson --- lib/OpenQA/JobSettings.pm | 13 +++++++++---- t/api/02-iso-download.t | 1 + 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/lib/OpenQA/JobSettings.pm b/lib/OpenQA/JobSettings.pm index c3c7d97c9..335c8d1ef 100644 --- a/lib/OpenQA/JobSettings.pm +++ b/lib/OpenQA/JobSettings.pm @@ -21,7 +21,7 @@ use File::Basename; use Mojo::URL; use Mojo::Util 'url_unescape'; use OpenQA::Log 'log_debug'; -use OpenQA::Utils 'get_url_short'; +use OpenQA::Utils qw(asset_type_from_setting get_url_short); sub generate_settings { my ($params) = @_; @@ -113,9 +113,9 @@ sub handle_plus_in_settings { # Given a hashref of settings, parse any whose names end in _URL # to the short name, then if there is not already a setting with -# the short name, set it to the filename from the URL (with the -# compression extension removed in the case of _DECOMPRESS_URL). -# This has to happen *before* generate_jobs +# the short name and the setting is an asset type, set it to the +# filename from the URL (with the compression extension removed +# in the case of _DECOMPRESS_URL). sub parse_url_settings { my ($settings) = @_; for my $setting (keys %$settings) { @@ -137,6 +137,11 @@ sub parse_url_settings { log_debug("Unable to get filename from $url. Ignoring $setting"); next; } + # We shouldn't set the short setting for non-asset types + unless (asset_type_from_setting($short, $filename)) { + log_debug("_URL downloading only allowed for asset types! $short is not an asset type"); + next; + } $settings->{$short} = $filename; } return undef; diff --git a/t/api/02-iso-download.t b/t/api/02-iso-download.t index 914d30552..a718ef154 100644 --- a/t/api/02-iso-download.t +++ b/t/api/02-iso-download.t @@ -158,6 +158,7 @@ check_job_setting($t, $rsp, 'KERNEL', 'callitvmlinuz', $rsp = schedule_iso({%params, NO_ASSET_URL => 'http://localhost/nonexistent.iso'}); is($rsp->json->{count}, $expected_job_count, 'a regular ISO post creates the expected number of jobs'); check_download_asset('non-asset _URL'); +check_job_setting($t, $rsp, 'NO_ASSET', undef, 'NO_ASSET is not parsed from NO_ASSET_URL'); # Using asset _URL but without filename extractable from URL create warning in log file, jobs, but no gru job $rsp = schedule_iso({%iso, ISO_URL => 'http://localhost'}); -- 2.29.2