From 80fe99822fa1272698a4c0877cfea964f70795b8 Mon Sep 17 00:00:00 2001 From: Ajay Veluvolu Date: Wed, 16 Feb 2022 11:15:35 -0500 Subject: [PATCH] Remove the stale Puma pidfile if it exists --- CHANGELOG.md | 3 ++ bin/conjur-cli/commands/server.rb | 21 +++++++++ spec/conjurctl/server_spec.rb | 39 +++++++++++++++++ spec/spec_helper.rb | 71 +++++++++++++++++++++++++++++++ 4 files changed, 134 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 015f11a9f0..e88d00956b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -47,6 +47,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ### Fixed - IAM Authn bug fix - Take rexml gem to production configuration [#2493](https://github.com/cyberark/conjur/pull/2493) +- Previously, a stale puma pid file would prevent the Conjur server from starting + successfully. Conjur now removes a stale pid file at startup, if it exists. + [#2498](https://github.com/cyberark/conjur/pull/2498) ### Security - Updated nokogiri to 1.13.3 to resolve CVE-2022-23308 and CVE-2021-30560 diff --git a/bin/conjur-cli/commands/server.rb b/bin/conjur-cli/commands/server.rb index 73099847d8..d44afb91ee 100644 --- a/bin/conjur-cli/commands/server.rb +++ b/bin/conjur-cli/commands/server.rb @@ -31,6 +31,9 @@ def call create_account load_bootstrap_policy + # Remove a stale puma PID file, if it exists + cleanup_pidfile + # Start the Conjur API and service # processes fork_server_process @@ -85,6 +88,24 @@ def load_bootstrap_policy ) || exit(($CHILD_STATUS.exitstatus)) end + # This method is needed because in some versions of conjur server it has been observed that + # docker restart of the conjur server results in an error stating that the puma PID file is still present. + # Hence we check to see if this stale PID File exists and delete it, which ensures a smooth restart. + # This issue is described in detail in Issue 2381. + + def cleanup_pidfile + # Get the path to conjurctl + conjurctl_path = `readlink -f $(which conjurctl)` + + # Navigate from its directory (/bin) to the root Conjur server directory + conjur_server_dir = Pathname.new(File.join(File.dirname(conjurctl_path), '..')).cleanpath + pid_file_path = File.join(conjur_server_dir, 'tmp/pids/server.pid') + return unless File.exist?(pid_file_path) + + puts("Removing existing PID file: #{pid_file_path}") + File.delete(pid_file_path) + end + def fork_server_process Process.fork do puts("Conjur v#{conjur_version} starting up...") diff --git a/spec/conjurctl/server_spec.rb b/spec/conjurctl/server_spec.rb index 3ea424e201..39ff877eff 100644 --- a/spec/conjurctl/server_spec.rb +++ b/spec/conjurctl/server_spec.rb @@ -47,5 +47,44 @@ def wait_for_conjur expect(Slosilo["authn:demo"]).to be expect(Role["demo:user:admin"]).to be end + + it "deletes an existing PID file on start up" do + pid_file_path = File.join(conjur_server_dir, 'tmp/pids/server.pid') + + # Ensure the pid file exists before starting Conjur + FileUtils.mkdir_p(File.dirname(pid_file_path)) + FileUtils.touch(pid_file_path) + + # Start Conjur and wait for it to finish its initialization + output = with_background_process( + 'conjurctl server --account demo' + ) do + wait_for_conjur + end + + # Ensure that the Conjur output reports that the PID was removed + expect(output).to include( + "Removing existing PID file: #{pid_file_path}" + ) + end + + it "doesn't attempt to delete a non-existent PID file" do + pid_file_path = File.join(conjur_server_dir, 'tmp/pids/server.pid') + + # Ensure the pid file doesn't exist before starting Conjur + File.delete(pid_file_path) if File.exist?(pid_file_path) + + # Start Conjur and wait for it to finish its initialization + output = with_background_process( + 'conjurctl server --account demo' + ) do + wait_for_conjur + end + + # Ensure that the Conjur output doesn't report that the PID was removed + expect(output).not_to include( + "Removing existing PID file: #{pid_file_path}" + ) + end end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 534450aab4..beaf5afa17 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -82,4 +82,75 @@ def access_token_for(user, account: 'rspec') "Token token=\"#{Base64.strict_encode64(bearer_token.to_json)}\"" end +def with_background_process(cmd, &block) + puts("Running: #{cmd}") + Open3.popen2e( + cmd, + pgroup: true + ) do |stdin, stdout_and_err, wait_thr| + # We don't need to interact with stdin for the background process, + # so close that stream immediately. + stdin.close_write + + # Read the output of the background process in a thread to run + # the given block in parallel. + out_reader = Thread.new do + output = StringIO.new + + loop do + ready = IO.select( + [stdout_and_err], # Watch for reading + [], # Not watching any files for writing + [], # Not watching any files for exceptions + # When the background process is killed, it doesn't end the + # the output stream, so we need a timeout here to recognize the + # stream has closed: + 1 # 1 second timeout + ) + + # If the stream has closed, break the read loop. + break if stdout_and_err.closed? + + # If we've reached the end of the stream, break the read loop. + break if stdout_and_err.eof? + + # If this was the result of a IO#select timeout, enter the select + # loop again. + next unless ready + + # Read the next available output on the stream + output << stdout_and_err.read_nonblock(1024) + end + + # Return the collected output as the result of the read thread + output.string + end + + # Call the given block + block.call + + # Kill the background process and any children processes + pgid = Process.getpgid(wait_thr.pid) + Process.kill("-TERM", pgid) if wait_thr.alive? + + # Wait for the background process to end + wait_thr.value + + # Close the output thread + stdout_and_err.close + + # Wait for the result from the reader thread + out_reader.value + end +end + +def conjur_server_dir + # Get the path to conjurctl + conjurctl_path = `readlink -f $(which conjurctl)` + + # Navigate from its directory (/bin) to the root Conjur server directory + Pathname.new(File.join(File.dirname(conjurctl_path), '..')).cleanpath +end + + require 'stringio'