Use more native methods of SmartProxy for validation

This uses more built in validators. It also creates a setting for the
public key file and derives it if not specified.

The only downside is that the validate_readable doesn't print a helpful
command to generate it if needed. Programmable settings can be used to
generate it on the fly if the parent directory is readable. Another
alternative is to introduce another custom validator.

It slightly abuses the dependency injection hook to configure the task
launcher registry.

Author: ekohl

lib/smart_proxy_remote_execution_ssh.rb

@@ -6,83 +6,6 @@
 module Proxy::RemoteExecution
   module Ssh
     class << self
-      def validate!
-        unless private_key_file
-          raise "settings for `ssh_identity_key` not set"
-        end
-
-        unless File.exist?(private_key_file)
-          raise "Ssh public key file #{private_key_file} doesn't exist.\n"\
-            "You can generate one with `ssh-keygen -t rsa -b 4096 -f #{private_key_file} -N ''`"
-        end
-
-        unless File.exist?(public_key_file)
-          raise "Ssh public key file #{public_key_file} doesn't exist"
-        end
-
-        validate_mode!
-        validate_ssh_log_level!
-        validate_mqtt_settings!
-      end
-
-      def private_key_file
-        File.expand_path(Plugin.settings.ssh_identity_key_file)
-      end
-
-      def public_key_file
-        File.expand_path("#{private_key_file}.pub")
-      end
-
-      def validate_mode!
-        Plugin.settings.mode = Plugin.settings.mode.to_sym
-
-        unless Plugin::MODES.include? Plugin.settings.mode
-          raise "Mode has to be one of #{Plugin::MODES.join(', ')}, given #{Plugin.settings.mode}"
-        end
-
-        if Plugin.settings.async_ssh
-          Plugin.logger.warn('Option async_ssh is deprecated, use ssh-async mode instead.')
-
-          case Plugin.settings.mode
-          when :ssh
-            Plugin.logger.warn('Deprecated option async_ssh used together with ssh mode, switching mode to ssh-async.')
-            Plugin.settings.mode = :'ssh-async'
-          when :'async-ssh'
-            # This is a noop
-          else
-            Plugin.logger.warn('Deprecated option async_ssh used together with incompatible mode, ignoring.')
-          end
-        end
-      end
-
-      def validate_mqtt_settings!
-        return unless Plugin.settings.mode == :'pull-mqtt'
-
-        raise 'mqtt_broker has to be set when pull-mqtt mode is used' if Plugin.settings.mqtt_broker.nil?
-        raise 'mqtt_port has to be set when pull-mqtt mode is used' if Plugin.settings.mqtt_port.nil?
-      end
-
-      def validate_ssh_log_level!
-        wanted_level = Plugin.settings.ssh_log_level.to_s
-        levels = Plugin::SSH_LOG_LEVELS
-        unless levels.include? wanted_level
-          raise "Wrong value '#{Plugin.settings.ssh_log_level}' for ssh_log_level, must be one of #{levels.join(', ')}"
-        end
-
-        current = ::Proxy::SETTINGS.log_level.to_s.downcase
-
-        # regular log levels correspond to upcased ssh logger levels
-        ssh, regular = [wanted_level, current].map do |wanted|
-          levels.each_with_index.find { |value, _index| value == wanted }.last
-        end
-
-        if ssh < regular
-          raise 'ssh_log_level cannot be more verbose than regular log level'
-        end
-
-        Plugin.settings.ssh_log_level = Plugin.settings.ssh_log_level.to_sym
-      end
-
       def job_storage
         @job_storage ||= Proxy::RemoteExecution::Ssh::JobStorage.new
       end

lib/smart_proxy_remote_execution_ssh/api.rb

@@ -10,7 +10,7 @@ class Api < ::Sinatra::Base
       include Proxy::Dynflow::Helpers
 
       get "/pubkey" do
-        File.read(Ssh.public_key_file)
+        File.read(Proxy::RemoteExecution::Plugin.settings.ssh_identity_public_key_file)
       rescue Errno::ENOENT
         halt 404
       end

lib/smart_proxy_remote_execution_ssh/cockpit.rb

@@ -242,7 +242,7 @@ def params
       end
 
       def key_file
-        @key_file ||= Proxy::RemoteExecution::Ssh.private_key_file
+        @key_file ||= Proxy::RemoteExecution::Plugin.settings.ssh_identity_key_file
       end
 
       def buf_socket

lib/smart_proxy_remote_execution_ssh/plugin.rb

@@ -1,3 +1,5 @@
+require_relative 'validators'
+
 module Proxy::RemoteExecution::Ssh
   class Plugin < Proxy::Plugin
     SSH_LOG_LEVELS = %w[debug info error fatal].freeze
@@ -19,9 +21,49 @@ class Plugin < Proxy::Plugin
                      # :mqtt_port               => nil,
                      :mode                    => :ssh
 
+    load_validators ssh_log_level: Proxy::RemoteExecution::Ssh::Validators::SshLogLevel,
+                    rex_ssh_mode: Proxy::RemoteExecution::Ssh::Validators::RexSshMode
+
+    load_programmable_settings do |settings|
+      if settings[:ssh_identity_key_file]
+        settings[:ssh_identity_key_file] = File.expand_path(settings[:ssh_identity_key_file])
+      end
+
+      if settings[:ssh_identity_public_key_file]
+        settings[:ssh_identity_public_key_file] = File.expand_path(settings[:ssh_identity_public_key_file])
+      elsif settings[:ssh_identity_key_file]
+        settings[:ssh_identity_public_key_file] = "#{settings[:ssh_identity_key_file]}.pub"
+      end
+
+      if settings[:async_ssh]
+        Plugin.logger.warn('Option async_ssh is deprecated, use ssh-async mode instead.')
+
+        case setting_value
+        when :ssh
+          Plugin.logger.warn('Deprecated option async_ssh used together with ssh mode, switching mode to ssh-async.')
+          settings[:mode] = :'ssh-async'
+        when :'async-ssh'
+          # This is a noop
+        else
+          Plugin.logger.warn('Deprecated option async_ssh used together with incompatible mode, ignoring.')
+        end
+      end
+
+      settings[:mode] = settings[:mode].to_sym
+      settings[:ssh_log_level] = settings[:ssh_log_level].to_sym
+    end
+
+    validate_readable :ssh_identity_key_file, :ssh_identity_public_key_file
+    validate :ssh_log_level, ssh_log_level: SSH_LOG_LEVELS
+    validate :mode, rex_ssh_mode: MODES
+    validate_presence :mqtt_broker, :mqtt_port, if: ->(settings) { settings[:mode] == :'pull-mqtt' }
+
     plugin :ssh, Proxy::RemoteExecution::Ssh::VERSION
-    after_activation do
+
+    load_classes do
       require 'smart_proxy_dynflow'
+      require 'smart_proxy_dynflow/task_launcher'
+      require 'smart_proxy_dynflow/runner'
       require 'smart_proxy_remote_execution_ssh/version'
       require 'smart_proxy_remote_execution_ssh/cockpit'
       require 'smart_proxy_remote_execution_ssh/api'
@@ -31,9 +73,10 @@ class Plugin < Proxy::Plugin
       require 'smart_proxy_remote_execution_ssh/runners'
       require 'smart_proxy_remote_execution_ssh/utils'
       require 'smart_proxy_remote_execution_ssh/job_storage'
+    end
 
-      Proxy::RemoteExecution::Ssh.validate!
-
+    # Not really Smart Proxy dependency injection, but similar enough
+    load_dependency_injection_wirings do |container_instance, settings|
       Proxy::Dynflow::TaskLauncherRegistry.register('ssh', Proxy::Dynflow::TaskLauncher::Batch)
     end
 

lib/smart_proxy_remote_execution_ssh/validators.rb

@@ -0,0 +1,42 @@
+module Proxy
+  module RemoteExecution
+    module Ssh
+      module Validators
+        class SshLogLevel < ::Proxy::PluginValidators::Base
+          def validate!(settings)
+            setting_value = settings[@setting_name].to_s
+
+            unless @params.include?(setting_value)
+              raise ::Proxy::Error::ConfigurationError, "Parameter '#{@setting_name}' must be one of #{@params.join(', ')}"
+            end
+
+            current = ::Proxy::SETTINGS.log_level.to_s.downcase
+
+            # regular log levels correspond to upcased ssh logger levels
+            ssh, regular = [setting_value, current].map do |wanted|
+              @params.each_with_index.find { |value, _index| value == wanted }.last
+            end
+
+            if ssh < regular
+              raise ::Proxy::Error::ConfigurationError, "Parameter '#{@setting_name}' cannot be more verbose than regular log level (#{current})"
+            end
+
+            true
+          end
+        end
+
+        class RexSshMode < ::Proxy::PluginValidators::Base
+          def validate!(settings)
+            setting_value = settings[@setting_name]
+
+            unless @params.include?(setting_value)
+              raise ::Proxy::Error::ConfigurationError, "Parameter '#{@setting_name}' must be one of #{@params.join(', ')}"
+            end
+
+            true
+          end
+        end
+      end
+    end
+  end
+end

test/api_test.rb

@@ -27,14 +27,15 @@ class ApiTest < MiniTest::Spec
 
     def setup
       super
-      Proxy::RemoteExecution::Ssh::Plugin.load_test_settings(ssh_identity_key_file: FAKE_PRIVATE_KEY_FILE)
+      Proxy::RemoteExecution::Ssh::Plugin.load_test_settings(ssh_identity_key_file: FAKE_PRIVATE_KEY_FILE, ssh_identity_public_key_file: FAKE_PUBLIC_KEY_FILE)
     end
 
     let(:app) { Proxy::RemoteExecution::Ssh::Api.new }
 
     describe '/pubkey' do
       it 'returns the content of the public key' do
         get '/pubkey'
+        _(last_response.status).must_equal 200, last_response.body
         _(last_response.body).must_equal '===public-key==='
       end
     end

test/integration_test.rb

@@ -14,9 +14,11 @@ def app
 
   def test_features_for_default_mode_without_dynflow
     Proxy::LegacyModuleLoader.any_instance.expects(:load_configuration_file).with('dynflow.yml').returns(enabled: false)
-    Proxy::LegacyModuleLoader.any_instance.expects(:load_configuration_file).with('remote_execution_ssh.yml').returns(
+    Proxy::DefaultModuleLoader.any_instance.expects(:load_configuration_file).with('remote_execution_ssh.yml').returns(
       enabled: true,
       ssh_identity_key_file: FAKE_PRIVATE_KEY_FILE,
+      mqtt_broker: 'broker.example.com',
+      mqtt_port: 1883,
     )
 
     get '/features'
@@ -30,7 +32,7 @@ def test_features_for_default_mode_without_dynflow
 
   def test_features_for_default_mode_with_dynflow
     Proxy::LegacyModuleLoader.any_instance.expects(:load_configuration_file).with('dynflow.yml').returns(enabled: true)
-    Proxy::LegacyModuleLoader.any_instance.expects(:load_configuration_file).with('remote_execution_ssh.yml').returns(
+    Proxy::DefaultModuleLoader.any_instance.expects(:load_configuration_file).with('remote_execution_ssh.yml').returns(
       enabled: true,
       ssh_identity_key_file: FAKE_PRIVATE_KEY_FILE,
     )
@@ -52,7 +54,7 @@ def test_features_for_default_mode_with_dynflow
 
   def test_features_for_pull_mqtt_mode_without_required_options
     Proxy::LegacyModuleLoader.any_instance.expects(:load_configuration_file).with('dynflow.yml').returns(enabled: true)
-    Proxy::LegacyModuleLoader.any_instance.expects(:load_configuration_file).with('remote_execution_ssh.yml').returns(
+    Proxy::DefaultModuleLoader.any_instance.expects(:load_configuration_file).with('remote_execution_ssh.yml').returns(
       enabled: true,
       ssh_identity_key_file: FAKE_PRIVATE_KEY_FILE,
       mode: 'pull-mqtt',
@@ -68,12 +70,14 @@ def test_features_for_pull_mqtt_mode_without_required_options
 
     mod = response['ssh']
     refute_nil(mod)
-    assert_equal('running', mod['state'], Proxy::LogBuffer::Buffer.instance.info[:failed_modules][:ssh])
+    assert_equal('failed', mod['state'], Proxy::LogBuffer::Buffer.instance.info[:failed_modules][:ssh])
+    assert_equal("Disabling all modules in the group ['ssh'] due to a failure in one of them: Parameter 'mqtt_broker' is expected to have a non-empty value",
+                 Proxy::LogBuffer::Buffer.instance.info[:failed_modules][:ssh])
   end
 
   def test_features_with_dynflow_and_required_options
     Proxy::LegacyModuleLoader.any_instance.expects(:load_configuration_file).with('dynflow.yml').returns(enabled: true)
-    Proxy::LegacyModuleLoader.any_instance.expects(:load_configuration_file).with('remote_execution_ssh.yml').returns(
+    Proxy::DefaultModuleLoader.any_instance.expects(:load_configuration_file).with('remote_execution_ssh.yml').returns(
       enabled: true,
       ssh_identity_key_file: FAKE_PRIVATE_KEY_FILE,
       mode: 'pull-mqtt',