run clair for all builds

app/models/docker_builder_service.rb

@@ -45,14 +45,21 @@ def run!(image_name: nil, push: false, tag_as_latest: false)
       @output = execution.output
       repository.executor = execution.executor
 
-      if build.kubernetes_job
-        run_build_image_job(job, image_name, push: push, tag_as_latest: tag_as_latest)
-      elsif build_image(tmp_dir)
-        ret = true
-        ret = push_image(image_name, tag_as_latest: tag_as_latest) if push
-        build.docker_image.remove(force: true) unless ENV["DOCKER_KEEP_BUILT_IMGS"] == "1"
-        ret
+      success =
+        if build.kubernetes_job
+          run_build_image_job(job, image_name, push: push, tag_as_latest: tag_as_latest)
+        elsif build_image(tmp_dir)
+          ret = true
+          ret = push_image(image_name, tag_as_latest: tag_as_latest) if push
+          build.docker_image.remove(force: true) unless ENV["DOCKER_KEEP_BUILT_IMGS"] == "1"
+          ret
+        end
+
+      if success
+        Samson::Clair.append_job_with_scan(job, docker_image_ref(image_name, build))
       end
+
+      success
     end
 
     job_execution.on_complete { send_after_notifications }

lib/samson/clair.rb

@@ -0,0 +1,42 @@
+# frozen_string_literal: true
+module Samson
+  # Hyperclair will pull the image from registry and run scan with Clair scanner
+  # TODO: should check based on docker_repo_digest not tag
+  # TODO: this should be a plugin instead and use hooks
+  module Clair
+    class << self
+      def append_job_with_scan(job, docker_tag)
+        return unless clair = ENV['HYPERCLAIR_PATH']
+
+        Thread.new do
+          sleep 0.1 if Rails.env.test? # in test we reuse the same connection, so we cannot use it at the same time
+          success, output, time = scan(clair, job.project, docker_tag)
+          status = (success ? "success" : "errored or vulnerabilities found")
+          output = "### Clair scan: #{status} in #{time}s\n#{output}"
+          job.reload
+          job.update_column(:output, job.output + output)
+        end
+      end
+
+      private
+
+      def scan(executable, project, docker_ref)
+        with_time do
+          Samson::CommandExecutor.execute(
+            executable,
+            *project.docker_repo.split('/', 2),
+            docker_ref,
+            whitelist_env: ['DOCKER_REGISTRY_USER', 'DOCKER_REGISTRY_PASS', 'PATH'],
+            timeout: 60 * 60
+          )
+        end
+      end
+
+      def with_time
+        result = []
+        time = Benchmark.realtime { result = yield }
+        result << time
+      end
+    end
+  end
+end

lib/samson/command_executor.rb

@@ -0,0 +1,46 @@
+# frozen_string_literal: true
+module Samson
+  # TODO: reuse in git_repo ?
+  # safe command execution that makes sure to use timeouts for everything and cleans up dead sub processes
+  module CommandExecutor
+    class << self
+      # timeout could be done more reliably with timeout(1) from gnu coreutils ... but that would add another dependency
+      def execute(*command, timeout:, whitelist_env: [])
+        raise ArgumentError, "Positive timeout required" if timeout <= 0
+        output = "ABORTED"
+        pid = nil
+
+        wait = Thread.new do
+          begin
+            IO.popen(ENV.to_h.slice(*whitelist_env), command, unsetenv_others: true, err: [:child, :out]) do |io|
+              pid = io.pid
+              output = io.read
+            end
+            $?&.success? || false
+          rescue Errno::ENOENT
+            output = "No such file or directory - #{command.first}"
+            false
+          end
+        end
+        success = Timeout.timeout(timeout) { wait.value } # using timeout in a blocking thread never interrupts
+
+        return success, output
+      rescue Timeout::Error
+        kill_process pid if pid
+        return false, $!.message
+      end
+
+      private
+
+      # timeout or parent process interrupted by user with Interrupt or SystemExit
+      def kill_process(pid)
+        Process.kill :INT, pid # tell it to stop
+        sleep 1 # give it a second to clean up
+        Process.kill :KILL, pid # kill it
+        Process.wait pid # prevent zombie processes
+      rescue Errno::ESRCH, Errno::ECHILD # kill or wait failing because pid was already gone
+        nil
+      end
+    end
+  end
+end

plugins/kubernetes/app/models/kubernetes/build_job_executor.rb

@@ -34,10 +34,6 @@ def execute!(build, project, docker_ref:, push: false, tag_as_latest: false)
       message = (success ? "completed successfully" : "failed or timed out")
       @output.puts "### Remote build job #{job_name} #{message}"
 
-      if success && clair = ENV['HYPERCLAIR_PATH']
-        Thread.new { scan_with_clair(clair, project, docker_ref) }
-      end
-
       return success, job_log
     ensure
       # Jobs will still be there regardless of their statuses
@@ -84,7 +80,7 @@ def fill_job_details(k8s_job, build, project, docker_ref:, push: false, tag_as_l
 
       # Pass all necessary information so that remote container can build the image
       container_params = {
-        env: [{name: 'DOCKER_REGISTRY', value: @registry[:serveraddress] }],
+        env: [{name: 'DOCKER_REGISTRY', value: @registry.fetch(:serveraddress) }],
         args: [
           project.repository_url, build.git_sha, project.docker_repo, docker_ref,
           push ? "yes" : "no",
@@ -94,55 +90,6 @@ def fill_job_details(k8s_job, build, project, docker_ref:, push: false, tag_as_l
       k8s_job[:spec][:template][:spec][:containers][0].update(container_params)
     end
 
-    # Run a shell script that wraps hyperclair.
-    # Hyperclair will pull the image from registry and run scan with Clair scanner
-    def scan_with_clair(script, project, git_ref)
-      sleep 0.1 if Rails.env.test? # in test we reuse the same connection, so we cannot use it at the same time
-      project_param = project.permalink.tr('_', '-')
-      success, output, time = execute_command(
-        script,
-        @registry.fetch(:serveraddress),
-        project_param,
-        git_ref,
-        timeout: 60 * 60
-      )
-      status = (success ? "success" : "errored or vulnerabilities found")
-      output = "### Clair scan: #{status} in #{time}s\n#{output}"
-      @job.reload
-      @job.update_column(:output, @job.output + output)
-    end
-
-    # timeout could be done more reliably with timeout(1) from gnu coreutils ... but that would add another dependency
-    def execute_command(*command, timeout:)
-      output = "ABORTED"
-
-      time = Benchmark.realtime do
-        IO.popen(command, unsetenv_others: true, err: [:child, :out]) do |io|
-          output = Timeout.timeout(timeout) { kill_child_on_error(io) { io.read } }
-        end
-      end.round
-      success = $?.success?
-
-      return success, output, time
-    rescue Errno::ENOENT, Timeout::Error
-      return false, $!.message, 0
-    end
-
-    # timeout or parent process interrupted by user with Interrupt or SystemExit
-    def kill_child_on_error(io)
-      yield
-    rescue Exception => e # rubocop:disable Lint/RescueException
-      begin
-        Process.kill :INT, io.pid # tell it to stop
-        sleep 1 # give it a second to clean up
-        Process.kill :KILL, io.pid # kill it
-        Process.wait io.pid # prevent zombie processes
-      rescue Errno::ESRCH # rubocop:disable Lint/HandleExceptions
-        # pid was already gone
-      end
-      raise e
-    end
-
     def job_config(build, project, docker_ref:, push: false, tag_as_latest: false)
       # Read the external config path and create a new job config instance
       contents = File.read(build_job_config_path)

plugins/kubernetes/test/models/kubernetes/build_job_executor_test.rb

@@ -150,87 +150,6 @@ def execute!(push: false)
           assert success
           assert_equal(job_pod_log.join("\n") << "\n", job_log)
         end
-
-        describe "clair" do
-          before { ActiveRecord::Base.stubs(connection: ActiveRecord::Base.connection) } # we update in another thread
-
-          around do |t|
-            Tempfile.open('clair') do |f|
-              f.write("#!/bin/bash\necho HELLO\nexit 0")
-              f.close
-              File.chmod 0o755, f.path
-              with_env(HYPERCLAIR_PATH: f.path, &t)
-            end
-          end
-
-          it "runs clair and reports success to the database" do
-            success, job_log = execute!
-            job_log.wont_include "Clair"
-            assert success
-
-            wait_for_threads
-
-            job = Job.first
-            job.output.must_include "Clair scan: success"
-            job.output.must_include "HELLO"
-          end
-
-          it "does not run clair when build failed" do
-            job_api_obj.stubs(:failure?).returns true
-            success, job_log = execute!
-            job_log.wont_include "Clair"
-            refute success
-
-            wait_for_threads # just in case something goes wrong / to keep tests symmetric
-
-            job = Job.first
-            job.output.wont_include "Clair scan"
-          end
-
-          it "runs clair and reports missing script to the database" do
-            File.unlink ENV['HYPERCLAIR_PATH']
-
-            success, job_log = execute!
-            job_log.wont_include "Clair"
-            assert success
-
-            wait_for_threads
-
-            job = Job.first
-            job.output.must_include "Clair scan: errored"
-            job.output.must_include "No such file or directory"
-          end
-
-          it "runs clair and reports failed script to the database" do
-            File.write ENV['HYPERCLAIR_PATH'], "#!/bin/bash\necho WORLD\nexit 1"
-
-            success, job_log = execute!
-            job_log.wont_include "Clair"
-            assert success
-
-            wait_for_threads
-
-            job = Job.first
-            job.output.must_include "Clair scan: errored"
-            job.output.must_include "WORLD"
-          end
-
-          it "runs clair and reports timed out script to the database" do
-            IO.any_instance.expects(:read).raises(Timeout::Error)
-            Process.expects(:kill).times(2)
-            Process.expects(:wait)
-
-            success, job_log = execute!
-            job_log.wont_include "Clair"
-            assert success
-
-            wait_for_threads
-
-            job = Job.first
-            job.output.must_include "Clair scan: errored"
-            job.output.must_include "Timeout::Error"
-          end
-        end
       end
     end
   end

test/lib/samson/clair_test.rb

@@ -0,0 +1,57 @@
+# frozen_string_literal: true
+require_relative '../../test_helper'
+
+SingleCov.covered!
+
+describe Samson::Clair do
+  def execute!
+    Samson::Clair.append_job_with_scan(job, 'latest')
+  end
+
+  let(:job) { jobs(:succeeded_test) }
+
+  before { ActiveRecord::Base.stubs(connection: ActiveRecord::Base.connection) } # we update in another thread
+
+  around do |t|
+    Tempfile.open('clair') do |f|
+      f.write("#!/bin/bash\necho HELLO\nexit 0")
+      f.close
+      File.chmod 0o755, f.path
+      with_env(HYPERCLAIR_PATH: f.path, DOCKER_REGISTRY: 'my.registry', &t)
+    end
+  end
+
+  it "runs clair and reports success to the database" do
+    execute!
+
+    wait_for_threads
+
+    job.reload
+    job.output.must_include "Clair scan: success"
+    job.output.must_include "HELLO"
+  end
+
+  it "runs clair and reports missing script to the database" do
+    File.unlink ENV['HYPERCLAIR_PATH']
+
+    execute!
+
+    wait_for_threads
+
+    job.reload
+    job.output.must_include "Clair scan: errored"
+    job.output.must_include "No such file or directory"
+  end
+
+  it "runs clair and reports failed script to the database" do
+    File.write ENV['HYPERCLAIR_PATH'], "#!/bin/bash\necho WORLD\nexit 1"
+
+    execute!
+
+    wait_for_threads
+
+    job.reload
+    job.output.must_include "Clair scan: errored"
+    job.output.must_include "WORLD"
+  end
+end