diff options
author | Lamont Granquist <lamont@scriptkiddie.org> | 2021-11-29 22:27:32 -0800 |
---|---|---|
committer | Lamont Granquist <lamont@scriptkiddie.org> | 2021-11-29 22:27:32 -0800 |
commit | 5496dc736fb28687902daa8a23705c6f234599fa (patch) | |
tree | 66da1e12037f5e24d9cb59f6f9e97ecd65251a27 | |
parent | c615ae3d594ee26746d547f3e25d299645e48190 (diff) | |
download | chef-5496dc736fb28687902daa8a23705c6f234599fa.tar.gz |
Add allow_downgrade back to DNF
Signed-off-by: Lamont Granquist <lamont@scriptkiddie.org>
-rw-r--r-- | cspell.json | 5 | ||||
-rw-r--r-- | lib/chef/provider/package.rb | 81 | ||||
-rw-r--r-- | lib/chef/provider/package/dnf.rb | 2 | ||||
-rw-r--r-- | lib/chef/resource/dnf_package.rb | 10 | ||||
-rw-r--r-- | omnibus/.tool-versions | 1 | ||||
-rw-r--r-- | spec/functional/resource/dnf_package_spec.rb | 54 |
6 files changed, 103 insertions, 50 deletions
diff --git a/cspell.json b/cspell.json index 06118297c8..72474a7fbc 100644 --- a/cspell.json +++ b/cspell.json @@ -181,6 +181,8 @@ "CNAME", "cname", "codepage", + "codepath", + "codepaths", "CODESEG", "COLORINDEX", "COLORREF", @@ -1338,6 +1340,9 @@ "unintuitive", "unixy", "Unjoin", + "unlazies", + "unlazied", + "unlazying", "Unmanaged", "unmanaged", "unmerge", diff --git a/lib/chef/provider/package.rb b/lib/chef/provider/package.rb index 65d2254258..82d7ed00cf 100644 --- a/lib/chef/provider/package.rb +++ b/lib/chef/provider/package.rb @@ -438,47 +438,81 @@ class Chef @target_version_array ||= begin target_version_array = [] - each_package do |package_name, new_version, current_version, candidate_version| + each_package do |package_name, new_version, current_version, candidate_version, magic_version| case action when :upgrade - if current_version.nil? - # with use_magic_version there may be a package installed, but it fails the user's - # requested new_resource.version constraints + if version_equals?(current_version, new_version) + # This is a short-circuit (mostly for the rubygems provider) to avoid needing to + # expensively query the candidate_version which must come later. This only checks + # exact matching, the check for fuzzy matching is later. + logger.trace("#{new_resource} #{package_name} #{new_version} is already installed") + target_version_array.push(nil) + elsif current_version.nil? + # This is a simple check to see if we have any currently installed version at all, this is + # safe to do before the allow_downgrade check so we check this before. logger.trace("#{new_resource} has no existing installed version. Installing install #{candidate_version}") target_version_array.push(candidate_version) - elsif !use_magic_version? && version_equals?(current_version, new_version) - # this is a short-circuit (mostly for the rubygems provider) to avoid needing to expensively query the candidate_version which must come later - logger.trace("#{new_resource} #{package_name} #{new_version} is already installed") + elsif !allow_downgrade && version_compare(current_version, candidate_version) == 1 + # This check for downgrading when allow_downgrade is false uses the current_version rather + # than the magic_version since we never want to downgrade even if the constraints are not met + # if the version is higher. This check does use the candidate_version and unlazies this so + # there will a perf hit on idempotent use when allow_downgrade is false which is unavoidable. + logger.trace("#{new_resource} #{package_name} has installed version #{current_version}, which is newer than available version #{candidate_version}. Skipping...)") target_version_array.push(nil) + elsif magic_version.nil? + # This is the check for fuzzy matching of the installed_version, where if the installed version + # does not match the desired version constraints (but is not an exact match) then we need to + # install the candidate_version (this must come after the allow_downgrade check) + logger.trace("#{new_resource} has an installed version that does not match the version constraint. Installing install #{candidate_version}") + target_version_array.push(candidate_version) elsif candidate_version.nil? + # This check necessarily unlazies the candidate_version and may be expensive (connecting to + # rubygems.org or whatever). It comes as late as possible. logger.trace("#{new_resource} #{package_name} has no candidate_version to upgrade to") target_version_array.push(nil) elsif version_equals?(current_version, candidate_version) + # This check sees if the candidate_version is already installed or if we should upgrade/update the + # package. This is the normal idempotent behavior of :upgrade and is inherently expensive due to + # unlazying the candidate_version. To prevent the perf hit the version may be specified with a full + # version constraint. Then the cookbook can roll the version forward and use :upgrade to force version + # pinning. logger.trace("#{new_resource} #{package_name} #{candidate_version} is already installed") target_version_array.push(nil) - elsif !allow_downgrade && version_compare(current_version, candidate_version) == 1 - logger.trace("#{new_resource} #{package_name} has installed version #{current_version}, which is newer than available version #{candidate_version}. Skipping...)") - target_version_array.push(nil) else - logger.trace("#{new_resource} #{package_name} is out of date, will upgrade to #{candidate_version}") + logger.trace("#{new_resource} #{package_name} is out of date, will update to #{candidate_version}") target_version_array.push(candidate_version) end when :install - if new_version && !use_magic_version? + if current_version && new_version && !allow_downgrade && version_compare(current_version, new_version) == 1 + # This is the idempotency guard for downgrades when downgrades are not allowed. This should perhaps raise + # an exception since we were told to install an exact package version but we are silently refusing to do so + # because a higher version is already installed. Maybe we need a flag for users to apply their preferred + # declarative philosophy? This has to come early and outside of the two code paths below. + logger.warn("#{new_resource} #{package_name} has installed version #{current_version}, which is newer than available version #{new_version}. Skipping...)") + target_version_array.push(nil) + elsif new_version && !use_magic_version? + # This is for "non magic version using" subclasses to do comparisons between the current_version and the + # desired new_version. XXX: If we converted this to current_version_requirement_satisfied? and made it specific + # to the current version check and then eliminated the magic_version, we might be able to eliminate separate codepaths + # here, and eliminate the semantic confusion around the magic_version? if version_requirement_satisfied?(current_version, new_version) logger.trace("#{new_resource} #{package_name} #{current_version} satisfies #{new_version} requirement") target_version_array.push(nil) - elsif current_version && !allow_downgrade && version_compare(current_version, new_version) == 1 - logger.warn("#{new_resource} #{package_name} has installed version #{current_version}, which is newer than available version #{new_version}. Skipping...)") - target_version_array.push(nil) else + # XXX: some subclasses seem to depend on this behavior where the new_version can be different from the + # candidate_version and we install the new_version, it seems like the candidate_version should be fixed to + # be resolved correctly to the new_version for those providers. although it may just be unit test bugs. + # it would be more correct to use the candidate_version here, but then it needs to be the correctly resolved + # candidate_version against the new_version constraint. logger.trace("#{new_resource} #{package_name} #{current_version} needs updating to #{new_version}") target_version_array.push(new_version) end - elsif current_version.nil? - # with use_magic_version there may be a package installed, but it fails the user's - # requested new_resource.version constraints + elsif magic_version.nil? + # This is for when we have a "magic version using" subclass and where the installed version does not match the + # constraint specified in the new_version, so we need to upgrade to the candidate_version. This is the only + # codepath in the :install branch which references the candidate_version so it is slow, but it is the path where + # we need to do work anyway. XXX: should we check for candidate_version.nil? somewhere around here? logger.trace("#{new_resource} #{package_name} not installed, installing #{candidate_version}") target_version_array.push(candidate_version) else @@ -512,8 +546,8 @@ class Chef @packages_missing_candidates ||= begin missing = [] - each_package do |package_name, new_version, current_version, candidate_version| - missing.push(package_name) if current_version.nil? && candidate_version.nil? + each_package do |package_name, new_version, current_version, candidate_version, magic_version| + missing.push(package_name) if magic_version.nil? && candidate_version.nil? end missing end @@ -536,7 +570,7 @@ class Chef @forced_packages_missing_candidates ||= begin missing = [] - each_package do |package_name, new_version, current_version, candidate_version| + each_package do |package_name, new_version, current_version, candidate_version, magic_version| next if new_version.nil? || current_version.nil? if use_magic_version? @@ -559,9 +593,10 @@ class Chef def each_package package_name_array.each_with_index do |package_name, i| candidate_version = candidate_version_array[i] - current_version = use_magic_version? ? magic_version[i] : current_version_array[i] + current_version = current_version_array[i] + magic_version = use_magic_version? ? magic_version_array[i] : current_version_array[i] new_version = new_version_array[i] - yield package_name, new_version, current_version, candidate_version + yield package_name, new_version, current_version, candidate_version, magic_version end end diff --git a/lib/chef/provider/package/dnf.rb b/lib/chef/provider/package/dnf.rb index 5c74ad0414..67bf24a411 100644 --- a/lib/chef/provider/package/dnf.rb +++ b/lib/chef/provider/package/dnf.rb @@ -98,7 +98,7 @@ class Chef end end - def magic_version + def magic_version_array package_name_array.each_with_index.map do |pkg, i| magical_version(i).version_with_arch end diff --git a/lib/chef/resource/dnf_package.rb b/lib/chef/resource/dnf_package.rb index 80727de7d0..aad0192490 100644 --- a/lib/chef/resource/dnf_package.rb +++ b/lib/chef/resource/dnf_package.rb @@ -68,12 +68,10 @@ class Chef end } - def allow_downgrade(arg = nil) - unless arg.nil? - Chef.deprecated(:dnf_package_allow_downgrade, "the allow_downgrade property on the dnf_package provider is not used, DNF supports downgrades by default.") - end - true - end + property :allow_downgrade, [ TrueClass, FalseClass ], + description: "Allow downgrading a package to satisfy requested version requirements.", + default: true, + desired_state: false end end end diff --git a/omnibus/.tool-versions b/omnibus/.tool-versions new file mode 100644 index 0000000000..1ade73219b --- /dev/null +++ b/omnibus/.tool-versions @@ -0,0 +1 @@ +ruby 2.7.4 diff --git a/spec/functional/resource/dnf_package_spec.rb b/spec/functional/resource/dnf_package_spec.rb index 9e03db5123..35af5d5c1a 100644 --- a/spec/functional/resource/dnf_package_spec.rb +++ b/spec/functional/resource/dnf_package_spec.rb @@ -455,7 +455,7 @@ describe Chef::Resource::DnfPackage, :requires_root, external: exclude_test do end context "downgrades" do - it "downgrades the package when allow_downgrade" do + it "downgrades the package when allow_downgrade is true" do flush_cache preinstall("chef_rpm-1.10-1.#{pkg_arch}.rpm") dnf_package "chef_rpm" do @@ -470,6 +470,18 @@ describe Chef::Resource::DnfPackage, :requires_root, external: exclude_test do action :install end.should_not_be_updated end + + it "does not downgrade the package when allow_downgrade is false" do + flush_cache + preinstall("chef_rpm-1.10-1.#{pkg_arch}.rpm") + dnf_package "chef_rpm" do + options default_options + allow_downgrade false + version "1.2-1" + action :install + end.should_not_be_updated + expect(shell_out("rpm -q --queryformat '%{NAME}-%{VERSION}-%{RELEASE}.%{ARCH}\n' chef_rpm").stdout.chomp).to match("^chef_rpm-1.10-1.#{pkg_arch}$") + end end context "with arches", :intel_64bit do @@ -763,6 +775,17 @@ describe Chef::Resource::DnfPackage, :requires_root, external: exclude_test do end.should_not_be_updated end + it "downgrade on a local file with allow_downgrade false does not downgrade" do + preinstall("chef_rpm-1.10-1.#{pkg_arch}.rpm") + dnf_package "#{CHEF_SPEC_ASSETS}/yumrepo/chef_rpm-1.2-1.#{pkg_arch}.rpm" do + options default_options + allow_downgrade false + version "1.2-1" + action :install + end.should_not_be_updated + expect(shell_out("rpm -q --queryformat '%{NAME}-%{VERSION}-%{RELEASE}.%{ARCH}\n' chef_rpm").stdout.chomp).to match("^chef_rpm-1.10-1.#{pkg_arch}$") + end + it "does not downgrade the package with :install" do preinstall("chef_rpm-1.10-1.#{pkg_arch}.rpm") dnf_package "#{CHEF_SPEC_ASSETS}/yumrepo/chef_rpm-1.2-1.#{pkg_arch}.rpm" do @@ -1027,25 +1050,6 @@ describe Chef::Resource::DnfPackage, :requires_root, external: exclude_test do action :install end.should_not_be_updated end - - it "throws a deprecation warning with allow_downgrade" do - Chef::Config[:treat_deprecation_warnings_as_errors] = false - expect(Chef).to receive(:deprecated).at_least(:once).with(:dnf_package_allow_downgrade, /^the allow_downgrade property on the dnf_package provider is not used/) - preinstall("chef_rpm-1.10-1.#{pkg_arch}.rpm") - dnf_package "chef_rpm" do - options default_options - version "1.2" - allow_downgrade true - action :install - end.should_be_updated - expect(shell_out("rpm -q chef_rpm").stdout.chomp).to match("^chef_rpm-1.2-1.#{pkg_arch}") - dnf_package "chef_rpm" do - options default_options - version "1.2" - allow_downgrade true - action :install - end.should_not_be_updated - end end context "with source arguments" do @@ -1092,6 +1096,16 @@ describe Chef::Resource::DnfPackage, :requires_root, external: exclude_test do end.should_not_be_updated end + it "does not downgrade the package when allow_downgrade is false" do + preinstall("chef_rpm-1.10-1.#{pkg_arch}.rpm") + dnf_package "#{CHEF_SPEC_ASSETS}/yumrepo/chef_rpm-1.2-1.#{pkg_arch}.rpm" do + options default_options + allow_downgrade false + action :upgrade + end.should_not_be_updated + expect(shell_out("rpm -q --queryformat '%{NAME}-%{VERSION}-%{RELEASE}.%{ARCH}\n' chef_rpm").stdout.chomp).to match("^chef_rpm-1.10-1.#{pkg_arch}$") + end + it "upgrades the package" do preinstall("chef_rpm-1.2-1.#{pkg_arch}.rpm") dnf_package "#{CHEF_SPEC_ASSETS}/yumrepo/chef_rpm-1.10-1.#{pkg_arch}.rpm" do |