Making windows installs more resilient

.github/workflows/lint.yml

@@ -20,6 +20,18 @@ jobs:
       - name: Cook Style
         run: cookstyle
 
+  unit:
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout code
+        uses: actions/checkout@v6
+      - name: Install Chef
+        uses: actionshub/chef-install@3.0.1
+      - name: Run unit tests
+        env:
+          CHEF_LICENSE: accept-no-persist
+        run: chef exec rspec spec/ --format documentation
+
   yamllint:
     runs-on: ubuntu-latest
     steps:

kitchen.yml

@@ -79,7 +79,7 @@ suites:
       - recipe[test::license_id]
     verifier:
       inspec_tests:
-        - path: ./test/integration/default/
+        - path: ./test/integration/license_id/
     attributes:
       chef_client_updater:
         license_id: 'free-79df705d-b685-419a-8b68-88401f74ff72-3999'

libraries/chef_client_updater_helper.rb

@@ -71,4 +71,30 @@ def validate_windows_package_availability(artifact)
     Chef::Log.warn("Package availability check failed, skipping upgrade: #{e.message}")
     false
   end
+
+  # The version we WANT TO INSTALL. If the user specifies a version in X.Y.Z format
+  # we use that without looking it up — UNLESS license_id is set, in which case the
+  # Trial API silently overrides any specific version to 'latest'. In that case we must
+  # resolve through artifact_info so desired_version reflects what will actually be
+  # installed (preventing an infinite update loop on subsequent runs).
+  # If :latest or a non-X.Y.Z format version, we look it up with mixlib-install.
+  # @return Mixlib::Versioning::Format::PartialSemVer
+  def desired_version
+    load_mixlib_versioning
+    if new_resource.version.to_sym == :latest
+      version = Mixlib::Versioning.parse(mixlib_install.available_versions.last)
+      Chef::Log.debug("User specified version of :latest. Looking up using mixlib-install. Value maps to #{version}.")
+    elsif new_resource.download_url_override
+      version = Mixlib::Versioning.parse(new_resource.version)
+      Chef::Log.debug("download_url_override specified. Using specified version of #{version}")
+    elsif new_resource.version.split('.').count == 3 && !new_resource.license_id
+      Chef::Log.debug("User specified version of #{new_resource.version}. No need check this against Chef servers.")
+      version = Mixlib::Versioning.parse(new_resource.version)
+    else
+      artifact = Array(mixlib_install.artifact_info).first
+      version = Mixlib::Versioning.parse(artifact.version)
+      Chef::Log.debug("User specified version of #{new_resource.version}. Looking up using mixlib-install. Value maps to #{version}.")
+    end
+    version
+  end
 end

providers/default.rb

@@ -128,27 +128,6 @@ def current_version
   end
 end
 
-# the version we WANT TO INSTALL. If the user specifies a version in X.Y.X format
-# we use that without looking it up. If :latest or a non-X.Y.Z format version we
-# look it up with mixlib-install to determine the latest version matching the request
-# @return Mixlib::Versioning::Format::PartialSemVer
-def desired_version
-  if new_resource.version.to_sym == :latest # we need to find what :latest really means
-    version = Mixlib::Versioning.parse(mixlib_install.available_versions.last)
-    Chef::Log.debug("User specified version of :latest. Looking up using mixlib-install. Value maps to #{version}.")
-  elsif new_resource.download_url_override # probably in an air-gapped environment.
-    version = Mixlib::Versioning.parse(new_resource.version)
-    Chef::Log.debug("download_url_override specified. Using specified version of #{version}")
-  elsif new_resource.version.split('.').count == 3 # X.Y.Z version format given
-    Chef::Log.debug("User specified version of #{new_resource.version}. No need check this against Chef servers.")
-    version = Mixlib::Versioning.parse(new_resource.version)
-  else # lookup their shortened version to find the X.Y.Z version
-    version = Mixlib::Versioning.parse(Array(mixlib_install.artifact_info).first.version)
-    Chef::Log.debug("User specified version of #{new_resource.version}. Looking up using mixlib-install as this is not X.Y.Z format. Value maps to #{version}.")
-  end
-  version
-end
-
 # why wouldn't we use the built in update_available? method in mixlib-install?
 # well that would use current_version from mixlib-install and it has no
 # concept of preventing downgrades
@@ -614,7 +593,19 @@ def execute_install_script(install_script)
 
           Write-Output "Running product install script..."
           try {
+            # Reset $LASTEXITCODE to $null before invoking the install script so that any
+            # non-zero exit code left behind by #{uninstall_if_necessary} (e.g. msiexec /x)
+            # does not produce a false positive on the check below.
+            $LASTEXITCODE = $null
             #{install_script}
+            # PowerShell's try/catch only catches *terminating* errors. External programs
+            # such as msiexec.exe signal failure via a non-zero exit code, not by throwing
+            # an exception, so a failed install would otherwise silently fall through and
+            # leave the node in an indeterminate state. Explicitly convert a non-zero exit
+            # code into a terminating error so the catch block fires and the rollback runs.
+            if ($LASTEXITCODE -ne $null -and $LASTEXITCODE -ne 0) {
+              throw "Install script exited with non-zero exit code: $LASTEXITCODE"
+            }
           }
           catch {
             Write-Output "An error occurred while trying to install product"

spec/unit/libraries/chef_client_updater_helper_spec.rb

@@ -167,4 +167,91 @@
       end
     end
   end
+
+  describe '#desired_version' do
+    let(:mixlib_instance) { double('Mixlib::Install') }
+    let(:versioning) { double('Mixlib::Versioning') }
+
+    before do
+      allow(provider).to receive(:mixlib_install).and_return(mixlib_instance)
+      allow(provider).to receive(:load_mixlib_versioning)
+      stub_const('Mixlib::Versioning', versioning)
+      allow(versioning).to receive(:parse) { |v| v }
+      allow(Chef::Log).to receive(:debug)
+    end
+
+    context 'when version is :latest' do
+      before { allow(resource).to receive(:version).and_return('latest') }
+
+      it 'resolves to the last value from available_versions' do
+        allow(mixlib_instance).to receive(:available_versions).and_return(%w(18.10.17 18.11.11))
+        expect(provider.desired_version).to eq('18.11.11')
+      end
+
+      it 'does not call artifact_info' do
+        allow(mixlib_instance).to receive(:available_versions).and_return(%w(18.11.11))
+        expect(mixlib_instance).not_to receive(:artifact_info)
+        provider.desired_version
+      end
+    end
+
+    context 'when version is X.Y.Z and license_id is nil' do
+      before do
+        allow(resource).to receive(:version).and_return('18.10.17')
+        allow(resource).to receive(:download_url_override).and_return(nil)
+        allow(resource).to receive(:license_id).and_return(nil)
+      end
+
+      it 'returns the requested version without calling the API' do
+        expect(mixlib_instance).not_to receive(:artifact_info)
+        expect(provider.desired_version).to eq('18.10.17')
+      end
+    end
+
+    context 'when version is X.Y.Z and license_id is set (Trial API scenario)' do
+      let(:artifact) { double('artifact', version: '18.11.11') }
+
+      before do
+        allow(resource).to receive(:version).and_return('18.10.17')
+        allow(resource).to receive(:download_url_override).and_return(nil)
+        allow(resource).to receive(:license_id).and_return('free-trial-key')
+        allow(mixlib_instance).to receive(:artifact_info).and_return([artifact])
+      end
+
+      it 'resolves through artifact_info to reflect what the Trial API will actually install' do
+        expect(provider.desired_version).to eq('18.11.11')
+      end
+
+      it 'does not return the originally-requested version' do
+        expect(provider.desired_version).not_to eq('18.10.17')
+      end
+    end
+
+    context 'when version is a short format (not X.Y.Z) without license_id' do
+      let(:artifact) { double('artifact', version: '18.11.11') }
+
+      before do
+        allow(resource).to receive(:version).and_return('18')
+        allow(resource).to receive(:download_url_override).and_return(nil)
+        allow(resource).to receive(:license_id).and_return(nil)
+        allow(mixlib_instance).to receive(:artifact_info).and_return([artifact])
+      end
+
+      it 'resolves through artifact_info to find the full X.Y.Z version' do
+        expect(provider.desired_version).to eq('18.11.11')
+      end
+    end
+
+    context 'when download_url_override is set' do
+      before do
+        allow(resource).to receive(:version).and_return('18.10.17')
+        allow(resource).to receive(:download_url_override).and_return('https://example.com/chef.rpm')
+      end
+
+      it 'uses the specified version directly without calling the API' do
+        expect(mixlib_instance).not_to receive(:artifact_info)
+        expect(provider.desired_version).to eq('18.10.17')
+      end
+    end
+  end
 end

spec/unit/providers/default_spec.rb

@@ -1,36 +1,42 @@
 require 'spec_helper'
 
+# NOTE: The Windows upgrade path in execute_install_script (the powershell_script resource
+# rendered via .run_action(:run)) cannot be meaningfully unit tested here because ChefSpec
+# does not intercept immediate run_action calls. Behavioral coverage for the Windows
+# $LASTEXITCODE exit-code check and rollback path requires a Windows Test Kitchen suite
+# that converges with a mock install script that exits non-zero and then asserts the
+# backup directory was restored. See kitchen.yml for the existing Linux suites.
+
 describe 'chef_client_updater provider' do
   platform 'ubuntu', '22.04'
 
-  let(:chef_run) do
-    ChefSpec::SoloRunner.new(platform: 'ubuntu', version: '22.04', step_into: ['chef_client_updater']) do |node|
-      node.normal['chef_client_updater']['product_name'] = product_name
-      node.normal['chef_client_updater']['version'] = '18.0.0'
-      node.normal['chef_client_updater']['license_id'] = 'test-license-key'
-    end
-  end
-
   context 'when product_name is chef-ice' do
-    let(:product_name) { 'chef-ice' }
+    let(:chef_run) do
+      ChefSpec::SoloRunner.new(platform: 'ubuntu', version: '22.04', step_into: ['chef_client_updater']) do |node|
+        node.normal['chef_client_updater']['product_name'] = 'chef-ice'
+        node.normal['chef_client_updater']['version'] = '18.0.0'
+      end
+    end
 
     it 'raises an error about unsupported product' do
-      chef_run.converge('chef_client_updater::default')
-      expect { chef_run }.to raise_error(RuntimeError, /Unsupported product: chef-ice/)
+      expect { chef_run.converge('chef_client_updater::default') }.to raise_error(RuntimeError, /Unsupported product: chef-ice/)
     end
 
     it 'includes error message stating cookbook only supports Chef 18 and below' do
-      chef_run.converge('chef_client_updater::default')
-      expect { chef_run }.to raise_error(RuntimeError, /only supports omnibus versions of Chef Infra Client 18 and below/)
+      expect { chef_run.converge('chef_client_updater::default') }.to raise_error(RuntimeError, /only supports omnibus versions of Chef Infra Client 18 and below/)
     end
   end
 
+  # For 'does not raise' contexts we set matching current/desired versions (both 18.0.0)
+  # and populate node.automatic['chef_packages'] so current_version works without ohai.
+  # With no license_id, desired_version uses the X.Y.Z fast path (no API call needed).
   context 'when product_name is chef' do
-    let(:product_name) { 'chef' }
-
-    before do
-      # Mock the version check to avoid actual upgrade logic
-      allow_any_instance_of(Object).to receive(:update_necessary?).and_return(false)
+    let(:chef_run) do
+      ChefSpec::SoloRunner.new(platform: 'ubuntu', version: '22.04', step_into: ['chef_client_updater']) do |node|
+        node.normal['chef_client_updater']['product_name'] = 'chef'
+        node.normal['chef_client_updater']['version'] = '18.0.0'
+        node.automatic['chef_packages'] = { 'chef' => { 'version' => '18.0.0' } }
+      end
     end
 
     it 'does not raise an error' do
@@ -39,11 +45,11 @@
   end
 
   context 'when product_name is not set (defaults to chef)' do
-    let(:product_name) { nil }
-
-    before do
-      # Mock the version check to avoid actual upgrade logic
-      allow_any_instance_of(Object).to receive(:update_necessary?).and_return(false)
+    let(:chef_run) do
+      ChefSpec::SoloRunner.new(platform: 'ubuntu', version: '22.04', step_into: ['chef_client_updater']) do |node|
+        node.normal['chef_client_updater']['version'] = '18.0.0'
+        node.automatic['chef_packages'] = { 'chef' => { 'version' => '18.0.0' } }
+      end
     end
 
     it 'does not raise an error' do

spec/unit/resources/default_spec.rb

@@ -4,56 +4,76 @@
   platform 'ubuntu', '22.04'
 
   describe 'chef-ice product validation' do
-    let(:chef_run) do
-      ChefSpec::SoloRunner.new(platform: 'ubuntu', version: '22.04', step_into: ['chef_client_updater']) do |node|
-        node.normal['chef_client_updater']['license_id'] = 'test-license-key'
+    # Use chef_client_updater::default (reads product_name from node attribute).
+    # test::chef_ice does not exist; the default recipe is the canonical entry point.
+    context 'when using chef-ice as product_name' do
+      let(:chef_run) do
+        ChefSpec::SoloRunner.new(platform: 'ubuntu', version: '22.04', step_into: ['chef_client_updater']) do |node|
+          node.normal['chef_client_updater']['product_name'] = 'chef-ice'
+          node.normal['chef_client_updater']['version'] = '18.0.0'
+        end
       end
-    end
 
-    context 'when using chef-ice as product_name' do
       it 'raises an error about unsupported product' do
-        chef_run.converge('test::chef_ice')
-        expect { chef_run }.to raise_error(RuntimeError, /Unsupported product: chef-ice/)
+        expect { chef_run.converge('chef_client_updater::default') }.to raise_error(RuntimeError, /Unsupported product: chef-ice/)
       end
     end
 
     context 'when using chef as product_name' do
-      before do
-        # Mock the version check to avoid actual upgrade logic
-        allow_any_instance_of(Object).to receive(:update_necessary?).and_return(false)
+      let(:chef_run) do
+        ChefSpec::SoloRunner.new(platform: 'ubuntu', version: '22.04', step_into: ['chef_client_updater']) do |node|
+          node.normal['chef_client_updater']['version'] = '18.0.0'
+          node.automatic['chef_packages'] = { 'chef' => { 'version' => '18.0.0' } }
+        end
       end
 
       it 'does not raise an error' do
-        expect { chef_run.converge('test::default') }.not_to raise_error
+        expect { chef_run.converge('chef_client_updater::default') }.not_to raise_error
       end
     end
   end
 
   describe 'license_id parameter' do
-    let(:chef_run) do
-      ChefSpec::SoloRunner.new(platform: 'ubuntu', version: '22.04', step_into: ['chef_client_updater'])
-    end
-
+    # Set attributes in the runner block (not in before) so they are guaranteed to be
+    # present when the recipe is compiled during converge.
+    # Stub mixlib_install at the ChefClientUpdaterHelper level (where desired_version lives)
+    # to return a fake artifact matching the current version, so update_necessary? returns
+    # false without making any real HTTP calls. allow_any_instance_of(Chef::Provider) cannot
+    # stub methods defined on the LWRP subclass itself.
     context 'when license_id is provided' do
+      let(:chef_run) do
+        ChefSpec::SoloRunner.new(platform: 'ubuntu', version: '22.04', step_into: ['chef_client_updater']) do |node|
+          node.normal['chef_client_updater']['version'] = '18.0.0'
+          node.normal['chef_client_updater']['license_id'] = 'test-license-123'
+          node.automatic['chef_packages'] = { 'chef' => { 'version' => '18.0.0' } }
+        end
+      end
+
       before do
-        allow_any_instance_of(Object).to receive(:update_necessary?).and_return(false)
-        chef_run.node.default['chef_client_updater']['license_id'] = 'test-license-123'
+        fake_artifact = double('artifact', version: '18.0.0', url: 'https://example.com/chef.rpm')
+        fake_mixlib = double('Mixlib::Install', artifact_info: [fake_artifact])
+        allow_any_instance_of(ChefClientUpdaterHelper).to receive(:load_mixlib_install)
+        allow_any_instance_of(ChefClientUpdaterHelper).to receive(:load_mixlib_versioning)
+        allow_any_instance_of(ChefClientUpdaterHelper).to receive(:mixlib_install).and_return(fake_mixlib)
       end
 
       it 'does not log a warning about missing license_id' do
         expect(Chef::Log).not_to receive(:warn).with(/No license_id specified/)
-        chef_run.converge('test::default')
+        chef_run.converge('chef_client_updater::default')
       end
     end
 
     context 'when license_id is not provided' do
-      before do
-        allow_any_instance_of(Object).to receive(:update_necessary?).and_return(false)
+      let(:chef_run) do
+        ChefSpec::SoloRunner.new(platform: 'ubuntu', version: '22.04', step_into: ['chef_client_updater']) do |node|
+          node.normal['chef_client_updater']['version'] = '18.0.0'
+          node.automatic['chef_packages'] = { 'chef' => { 'version' => '18.0.0' } }
+        end
       end
 
       it 'logs a warning about missing license_id' do
         expect(Chef::Log).to receive(:warn).with(/No license_id specified/)
-        chef_run.converge('test::default')
+        chef_run.converge('chef_client_updater::default')
       end
     end
   end

test/integration/license_id/default_spec.rb

@@ -0,0 +1,11 @@
+# The license-id suite uses the Trial API which only supports 'latest', so we cannot
+# assert a specific pinned version. Instead we verify that the chef-client was upgraded
+# beyond the kitchen baseline (18.6.2) and that the embedded gem meets the minimum.
+describe command('chef-client -v') do
+  its('stdout') { should match '^Chef Infra Client: ' }
+  its('stdout') { should_not match '^Chef Infra Client: 18\.6\.2' }
+end
+
+describe command('/opt/chef/embedded/bin/gem -v') do
+  its('stdout') { should cmp >= '2.6.11' }
+end