diff --git a/lib/mixlib/install/backend/package_router.rb b/lib/mixlib/install/backend/package_router.rb index ccf16316..6b64d634 100644 --- a/lib/mixlib/install/backend/package_router.rb +++ b/lib/mixlib/install/backend/package_router.rb @@ -33,11 +33,97 @@ class PackageRouter < Base COMPAT_DOWNLOAD_URL_ENDPOINT = "http://packages.chef.io".freeze + # Maximum number of attempts to validate a download URL before raising an error. + MAX_DOWNLOAD_VALIDATE_RETRIES = 3 + + # Base delay in seconds for exponential backoff between download URL validation retries. + DOWNLOAD_VALIDATE_RETRY_BASE_DELAY = 2 + # Architecture strings that appear as level-2 keys in PM-structure packages # responses (platform -> arch -> pm -> ...) vs version strings in standard # responses (platform -> version -> arch -> ...). KNOWN_ARCHITECTURES = %w{x86_64 aarch64 i386 arm64 ppc64 ppc64le s390x universal x86}.freeze + # Overrides Base#info to validate the resolved download URL is accessible + # before returning. When a platform is specified, only one artifact is + # returned and we can confirm the CDN has propagated the package before + # handing the URL back to the caller. Retries with exponential backoff to + # tolerate short CDN propagation windows during version promotions. + def info + result = super + return result unless platform_filters_available? + + validate_artifact_url(result) + result + end + + # Validates that the download URL on +artifact+ is reachable, retrying + # up to MAX_DOWNLOAD_VALIDATE_RETRIES times with exponential backoff. + # Raises ArtifactsNotFound if the URL remains inaccessible after all + # attempts. + def validate_artifact_url(artifact) + url = artifact.url + + raise ArtifactsNotFound, <<-MSG if url.nil? || url.empty? +Artifact resolved but download URL is nil or empty. + product: #{options.product_name} + channel: #{options.channel} + version: #{artifact.version} +MSG + + accessible = MAX_DOWNLOAD_VALIDATE_RETRIES.times.any? do |attempt| + break true if download_url_accessible?(url) + + if attempt < MAX_DOWNLOAD_VALIDATE_RETRIES - 1 + delay = DOWNLOAD_VALIDATE_RETRY_BASE_DELAY**(attempt + 1) + $stderr.puts "WARNING: Download URL not yet accessible (attempt #{attempt + 1} of #{MAX_DOWNLOAD_VALIDATE_RETRIES}). Retrying in #{delay}s..." + sleep(delay) + end + end + + raise ArtifactsNotFound, <<-MSG unless accessible +Download URL is not yet accessible after #{MAX_DOWNLOAD_VALIDATE_RETRIES} attempts. CDN propagation may still be in progress. + product: #{options.product_name} + channel: #{options.channel} + version: #{artifact.version} + url: #{url} +MSG + end + + # Issues a HEAD request to +url+, following up to +redirect_limit+ + # redirects. Returns +true+ when the server responds with 2xx, +false+ + # for 4xx/5xx responses. Raises for non-HTTP errors (DNS failure, SSL, + # connection refused, etc.) so callers can distinguish a temporarily + # unavailable resource from a configuration or network problem. + def download_url_accessible?(url, redirect_limit = 3) + return false if redirect_limit == 0 + + uri = URI.parse(url) + raise URI::InvalidURIError, "Redirect resolved to a relative URL: #{url}" unless uri.absolute? + + http = Net::HTTP.new(uri.host, uri.port) + http.use_ssl = (uri.scheme == "https") + http.open_timeout = 10 + http.read_timeout = 10 + + request = Net::HTTP::Head.new(uri.request_uri) + request.add_field("User-Agent", Util.user_agent_string(options.user_agent_headers)) + + response = http.request(request) + + case response + when Net::HTTPSuccess + true + when Net::HTTPRedirection + location = response["location"] + return false if location.nil? || location.empty? + + download_url_accessible?(location, redirect_limit - 1) + else + false + end + end + # Create filtered list of artifacts # # @return [Array] list of artifacts for the configured diff --git a/lib/mixlib/install/generator/powershell/scripts/install_project.ps1.erb b/lib/mixlib/install/generator/powershell/scripts/install_project.ps1.erb index 739bcee7..bc3c9b43 100644 --- a/lib/mixlib/install/generator/powershell/scripts/install_project.ps1.erb +++ b/lib/mixlib/install/generator/powershell/scripts/install_project.ps1.erb @@ -203,27 +203,62 @@ function Install-Project { } Write-Host "Installing $project from $download_destination" - $installingProject = $True - $installAttempts = 0 - $maxAttempts = 5 - while ($installingProject) { - $installAttempts++ - $result = $false - if ($download_destination.EndsWith(".appx")) { - $result = Install-ChefAppx $download_destination $project + + $backup_dir = $null + try { + $backup_dir = Backup-ChefInstallation -project $project + } + catch { + throw "Could not create a pre-upgrade backup of $project. Aborting to avoid leaving the system in an unrecoverable state. Error: $_" + } + + try { + $installingProject = $True + $installAttempts = 0 + $maxAttempts = 5 + while ($installingProject) { + $installAttempts++ + $result = $false + if ($download_destination.EndsWith(".appx")) { + $result = Install-ChefAppx $download_destination $project + } + else { + $result = Install-ChefMsi $download_destination $daemon + } + if (!$result) { + if ($installAttempts -ge $maxAttempts) { + Write-Host "Failed to install $project after $installAttempts attempts." + throw "Installation failed after $installAttempts attempts." + } + continue + } + $installingProject = $False + Write-Host "$project installation completed successfully." } - else { - $result = Install-ChefMsi $download_destination $daemon + } + catch { + $install_error = $_ + Write-Host "Installation failed. Attempting to restore the previous $project installation." + try { + Restore-ChefInstallation -project $project -backup_dir $backup_dir } - if (!$result) { - if ($installAttempts -ge $maxAttempts) { - Write-Host "Failed to install $project after $installAttempts attempts." - throw "Installation failed after $installAttempts attempts." - } - continue + catch { + Write-Host "WARNING: Restore also failed. The pre-upgrade backup is still at: $backup_dir" + Write-Host "WARNING: To recover manually, run these two commands in order:" + Write-Host "WARNING: 1. Remove-Item '$env:SystemDrive\<%= windows_dir %>\$project' -Recurse -Force" + Write-Host "WARNING: 2. Move-Item '$backup_dir' '$env:SystemDrive\<%= windows_dir %>\$project' -Force" + Write-Host "WARNING: Restore error: $_" } - $installingProject = $False - Write-Host "$project installation completed successfully." + throw $install_error + } + + # Remove the backup only after confirming installation succeeded. + # Wrapped separately so a cleanup failure never rolls back a working install. + try { + Remove-ChefBackup -backup_dir $backup_dir + } + catch { + Write-Host "Warning: Failed to remove installation backup at $backup_dir. You may remove it manually. Error: $_" } } } @@ -244,6 +279,11 @@ Function Install-ChefMsi($msi, $addlocal) { if ($p.ExitCode -eq 1618) { Write-Host "$((Get-Date).ToString()) - Another msi install is in progress (exit code 1618), retrying ($($installAttempts))..." return $false + } elseif ($p.ExitCode -eq 3010 -or $p.ExitCode -eq 1641) { + # 3010 = success, reboot required; 1641 = success, reboot initiated. + # Both are success codes. Treat them as success so the restore path is not triggered. + Write-Host "msiexec completed successfully with exit code $($p.ExitCode). A system reboot may be required." + return $true } elseif ($p.ExitCode -ne 0) { throw "msiexec was not successful. Received exit code $($p.ExitCode)" } @@ -273,4 +313,75 @@ Function Install-ChefAppx($appx, $project) { return $true } + +# CAUTION: chef_client_updater cookbook compatibility +# +# The chef_client_updater cookbook calls Install-Project indirectly on Windows. +# Its upgrade flow: +# 1. Calls mixlib_install.install_command to get the install script. +# 2. Calls prepare_windows, which copies C:\opscode\chef -> C:\opscode\chef.upgrade +# and creates a scheduled task named chef_upgrade (or _upgrade). +# 3. The scheduled task script runs Remove-Item "C:\opscode\chef" -Recurse -Force, +# then invokes the install script from step 1, which calls Install-Project. +# 4. On Install-Project failure the scheduled task catch block runs: +# Move-Item "C:\opscode\chef.upgrade" "C:\opscode\chef" to restore from backup. +# +# Because the install directory is removed before Install-Project is called, +# Backup-ChefInstallation finds no directory at that path and returns $null. +# Restore-ChefInstallation and Remove-ChefBackup both guard on $null and return +# early. All three functions are no-ops in the chef_client_updater flow. +# +# There is no directory naming conflict: chef_client_updater uses the suffix +# .upgrade (e.g. C:\opscode\chef.upgrade) while Install-Project uses +# .upgrade-backup (e.g. C:\opscode\chef.upgrade-backup). + +# Copies the existing product installation directory to a timestamped backup path +# before a destructive upgrade begins. Returns the backup path, or $null if there +# was no existing installation to back up. +function Backup-ChefInstallation { + param ($project) + $install_dir = "$env:SystemDrive\<%= windows_dir %>\$project" + if (-not (Test-Path $install_dir)) { + return $null + } + $backup_dir = "${install_dir}.upgrade-backup" + Write-Host "Backing up existing $project installation from $install_dir to $backup_dir" + if (Test-Path $backup_dir) { + Remove-Item $backup_dir -Recurse -Force + } + Copy-Item $install_dir $backup_dir -Recurse -Force + Write-Host "Backup created at $backup_dir" + return $backup_dir +} + +# Restores a backup created by Backup-ChefInstallation, replacing whatever is +# currently at the install path. Called when an upgrade fails so that the node +# is left with a working Chef installation rather than no installation. +function Restore-ChefInstallation { + param ($project, $backup_dir) + if ([string]::IsNullOrEmpty($backup_dir) -or -not (Test-Path $backup_dir)) { + return + } + $install_dir = "$env:SystemDrive\<%= windows_dir %>\$project" + Write-Host "Restoring $project installation from backup at $backup_dir" + if (Test-Path $install_dir) { + Remove-Item $install_dir -Recurse -Force + if (Test-Path $install_dir) { + throw "Could not fully remove $install_dir before restore. Some files may be locked. Restore aborted." + } + } + Move-Item $backup_dir $install_dir -Force + Write-Host "$project installation restored successfully." +} + +# Removes a backup directory that is no longer needed after a successful upgrade. +function Remove-ChefBackup { + param ($backup_dir) + if ([string]::IsNullOrEmpty($backup_dir) -or -not (Test-Path $backup_dir)) { + return + } + Write-Host "Removing installation backup at $backup_dir" + Remove-Item $backup_dir -Recurse -Force +} + export-modulemember -function 'Install-Project','Get-ProjectMetadata' -alias 'install' diff --git a/spec/unit/mixlib/install/backend/package_router_spec.rb b/spec/unit/mixlib/install/backend/package_router_spec.rb index 1e9c470c..41919218 100644 --- a/spec/unit/mixlib/install/backend/package_router_spec.rb +++ b/spec/unit/mixlib/install/backend/package_router_spec.rb @@ -52,6 +52,12 @@ let(:package_router) { Mixlib::Install::Backend::PackageRouter.new(mixlib_options) } let(:artifact_info) { package_router.info } + # Prevent download URL HEAD requests in unit tests. Individual contexts that + # test validation behaviour override this stub explicitly. + before do + allow(package_router).to receive(:download_url_accessible?).and_return(true) + end + context "for chef/stable" do let(:channel) { :stable } let(:product_name) { "chef" } @@ -985,4 +991,143 @@ expect(artifact_info.software_dependencies).to be_nil end end + + context "download URL validation" do + let(:channel) { :stable } + let(:product_name) { "chef" } + let(:product_version) { "18.0.0" } + let(:platform) { "ubuntu" } + let(:platform_version) { "20.04" } + let(:architecture) { "x86_64" } + let(:license_id) { "test-license-key-123" } + + let(:mock_metadata) do + { + "version" => "18.0.0", + "sha256" => "abc123def456", + "sha1" => "ghi789", + } + end + + before do + allow(package_router).to receive(:get).and_return(mock_metadata) + end + + context "when download URL is accessible on the first attempt" do + before do + allow(package_router).to receive(:download_url_accessible?).and_return(true) + end + + it "returns the artifact without error" do + expect(artifact_info).to be_a Mixlib::Install::ArtifactInfo + end + + it "checks the download URL exactly once" do + expect(package_router).to receive(:download_url_accessible?).once.and_return(true) + artifact_info + end + end + + context "when download URL is never accessible" do + before do + allow(package_router).to receive(:download_url_accessible?).and_return(false) + allow(package_router).to receive(:sleep) + end + + it "raises ArtifactsNotFound after exhausting retries" do + expect { artifact_info }.to raise_error( + Mixlib::Install::Backend::ArtifactsNotFound, + /CDN propagation/ + ) + end + + it "retries MAX_DOWNLOAD_VALIDATE_RETRIES times" do + expect(package_router).to receive(:download_url_accessible?).exactly( + Mixlib::Install::Backend::PackageRouter::MAX_DOWNLOAD_VALIDATE_RETRIES + ).times.and_return(false) + expect { artifact_info }.to raise_error(Mixlib::Install::Backend::ArtifactsNotFound) + end + + it "sleeps with exponential backoff between retries" do + expect(package_router).to receive(:sleep).with(2).ordered + expect(package_router).to receive(:sleep).with(4).ordered + expect { artifact_info }.to raise_error(Mixlib::Install::Backend::ArtifactsNotFound) + end + + it "includes product information in the error message" do + expect { artifact_info }.to raise_error(Mixlib::Install::Backend::ArtifactsNotFound) do |error| + expect(error.message).to include("chef") + expect(error.message).to include("stable") + expect(error.message).to include("18.0.0") + end + end + end + + context "when download URL becomes accessible on a retry" do + before do + allow(package_router).to receive(:download_url_accessible?).and_return(false, true) + allow(package_router).to receive(:sleep) + end + + it "returns the artifact after the retry" do + expect(artifact_info).to be_a Mixlib::Install::ArtifactInfo + end + + it "performs exactly two URL checks" do + expect(package_router).to receive(:download_url_accessible?).twice.and_return(false, true) + artifact_info + end + + it "sleeps once before the successful retry" do + expect(package_router).to receive(:sleep).once.with(2) + artifact_info + end + end + + context "when platform filters are not available" do + let(:platform) { nil } + let(:platform_version) { nil } + let(:architecture) { nil } + + it "skips download URL validation entirely" do + expect(package_router).not_to receive(:validate_artifact_url) + expect(package_router.platform_filters_available?).to be false + end + end + + context "when the resolved artifact has a nil URL" do + let(:nil_url_artifact) { instance_double(Mixlib::Install::ArtifactInfo, url: nil, version: "18.0.0") } + + it "raises ArtifactsNotFound immediately without making any HTTP requests" do + expect(package_router).not_to receive(:download_url_accessible?) + expect { package_router.validate_artifact_url(nil_url_artifact) }.to raise_error( + Mixlib::Install::Backend::ArtifactsNotFound, + /download URL is nil or empty/i + ) + end + end + + context "when the resolved artifact has an empty URL" do + let(:empty_url_artifact) { instance_double(Mixlib::Install::ArtifactInfo, url: "", version: "18.0.0") } + + it "raises ArtifactsNotFound immediately without making any HTTP requests" do + expect(package_router).not_to receive(:download_url_accessible?) + expect { package_router.validate_artifact_url(empty_url_artifact) }.to raise_error( + Mixlib::Install::Backend::ArtifactsNotFound, + /download URL is nil or empty/i + ) + end + end + + context "when a non-HTTP error occurs during URL validation" do + before do + allow(package_router).to receive(:download_url_accessible?).and_raise(SocketError, "getaddrinfo: Name or service not known") + allow(package_router).to receive(:sleep) + end + + it "propagates the error rather than masking it as a CDN issue" do + expect { artifact_info }.to raise_error(SocketError, /getaddrinfo/) + end + end + end end diff --git a/spec/unit/mixlib/install/generator_spec.rb b/spec/unit/mixlib/install/generator_spec.rb index e692a43b..a8f11feb 100644 --- a/spec/unit/mixlib/install/generator_spec.rb +++ b/spec/unit/mixlib/install/generator_spec.rb @@ -418,6 +418,40 @@ expect(install_script).to start_with("new-module -name Installer-Module -scriptblock") expect(install_script).to include("set-alias install -value Install-Project") end + + it "includes the backup helper function" do + expect(install_script).to include("function Backup-ChefInstallation") + end + + it "includes the restore helper function" do + expect(install_script).to include("function Restore-ChefInstallation") + end + + it "includes the backup cleanup helper function" do + expect(install_script).to include("function Remove-ChefBackup") + end + + it "wraps the install block in a try/catch that restores on failure" do + expect(install_script).to include("Backup-ChefInstallation -project $project") + expect(install_script).to include("Restore-ChefInstallation -project $project -backup_dir $backup_dir") + expect(install_script).to include("Remove-ChefBackup -backup_dir $backup_dir") + end + + it "re-throws the original install error after a restore attempt" do + expect(install_script).to include("$install_error = $_") + expect(install_script).to include("throw $install_error") + end + + it "includes a manual recovery warning when restore fails" do + expect(install_script).to include("WARNING: Restore also failed") + expect(install_script).to include("WARNING: To recover manually, run these two commands in order:") + end + + it "wraps backup cleanup in its own try/catch so a cleanup failure never rolls back a working install" do + # Verify Remove-ChefBackup is followed by its own catch block and not inside the install catch + expect(install_script).to include("Remove-ChefBackup -backup_dir $backup_dir") + expect(install_script).to include("You may remove it manually") + end end context "when platform is set" do