Use more native methods of SmartProxy for validation #57

ekohl · 2021-06-24T08:45:06Z

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 also still uses the Smart Proxy's LegacyModuleLoader due to the presence of after_activation.

Currently a draft since I can't test this locally (smart_proxy_dynflow is not Ruby 3 compatible). Consider it more of an RFC.

adamruzicka · 2021-06-24T09:13:00Z

I like this.

It also still uses the Smart Proxy's LegacyModuleLoader due to the presence of after_activation.

Does the non-legacy loader have support for some kind of hooks?

Currently a draft since I can't test this locally (smart_proxy_dynflow is not Ruby 3 compatible)

That's just an oversight on our side theforeman/smart_proxy_dynflow#89 , it works just fine once we loosen the requirements.

ekohl · 2021-06-24T09:28:16Z

Does the non-legacy loader have support for some kind of hooks?

Technically you can implement it as a service, but I think that's really ugly so I chose to (ab)use the dependency injection wiring part. That means it should now avoid the LegacyModuleLoader. Relevant code is here:

https://github.com/theforeman/smart-proxy/blob/develop/lib/proxy/plugin_initializer.rb

ekohl · 2021-07-09T14:49:06Z

This is now updated with the lesson we learned today.

adamruzicka · 2021-08-12T08:16:37Z

Could you rebase this so it can get in?

adamruzicka · 2021-08-16T10:20:23Z

Also, test failures are related

ekohl · 2021-08-16T11:31:05Z

I've rebased this and added more tests (which fixed a few bugs). The new integration tests do lack a test for the actual key retrieval. That's quite hard to do via the API call. What do you think about exposing the public key location as a setting? Later on that also means the Proxy can be extended to use the setting from registration without the additional HTTP API call.

Edit: the only downside is that it means the key is read on startup and can't be replaced during runtime. To do so, we would need settings to accept a callable. However, that could have some other implications.

adamruzicka · 2021-08-16T11:55:03Z

I'm probably still in vacation mode so please bear with me.

What do you think about exposing the public key location as a setting?

I wouldn't object, although I don't really see the benefit. We currently expose location of the private key and the public one is derived from it. Apart from supporting unconventionally named keypairs, what would this give us?

Proxy can be extended to use the setting from registration without the additional HTTP API call

Setting from registration? By additional http api call you mean the one to retrieve proxy's public key?

I would be fine with not being able to replace the key at runtime.

ekohl · 2021-08-16T11:59:32Z

I'm probably still in vacation mode so please bear with me.

Well, so am I :D

What do you think about exposing the public key location as a setting?

So I thought we should expose the public key itself, but then later realized this has some limitations.

I wouldn't object, although I don't really see the benefit. We currently expose location of the private key and the public one is derived from it. Apart from supporting unconventionally named keypairs, what would this give us?

You're right, the location itself has no value since it can't be read anyway.

Setting from registration? By additional http api call you mean the one to retrieve proxy's public key?

Yes. What I meant was that during the regular Smart Proxy registration all exposed settings are stored in the Foreman database. If we expose the public key (content) as a setting, that means it already obtained the key and no additional HTTP call is needed.

This properly loads the test settings, which includes loading defaults. This means no workaround is needed.

ekohl · 2021-12-13T14:24:44Z

I rebased it, but I'm having some trouble. I think it may be that I'm running on Ruby 3 and the Smart Proxy code probably fails:
https://github.com/theforeman/smart-proxy/blob/a2393b841d32986a6a6e774c078a2f6d6c9b7498/lib/proxy/pluggable.rb#L45-L60

However, CI also fails. I'm not sure how that translates exactly. Does it somehow translate the if: part into a setting?

If I add a small path:

diff --git a/lib/proxy/plugin_initializer.rb b/lib/proxy/plugin_initializer.rb
index 9b9f574..d4b0264 100644
--- a/lib/proxy/plugin_initializer.rb
+++ b/lib/proxy/plugin_initializer.rb
@@ -118,6 +118,7 @@ class ::Proxy::PluginGroup
   end
 
   def fail_group_with_message(a_message, an_exception = nil)
+    raise an_exception
     set_group_state_to_failed
     logger.error(a_message, an_exception)
     members.each do |m|

I get this traceback:

  2) Error:
SmartProxyRemoteExecutionSshApiFeaturesTest#test_features_with_dynflow_and_required_options:
NoMethodError: undefined method `to_sym' for {:if=>#<Proc:0x000055f4bf203000 /home/ekohl/dev/smart_proxy_remote_execution_ssh/lib/smart_proxy_remote_execution_ssh/plugin.rb:53 (lambda)>}:Hash
Did you mean?  to_s
               to_m
               to_set
    /home/ekohl/dev/smart-proxy/lib/proxy/plugin_validators.rb:5:in `initialize'
    /home/ekohl/dev/smart-proxy/lib/proxy/plugin_initializer.rb:352:in `new'
    /home/ekohl/dev/smart-proxy/lib/proxy/plugin_initializer.rb:352:in `block in execute_validators'
    /home/ekohl/dev/smart-proxy/lib/proxy/plugin_initializer.rb:349:in `each'
    /home/ekohl/dev/smart-proxy/lib/proxy/plugin_initializer.rb:349:in `inject'
    /home/ekohl/dev/smart-proxy/lib/proxy/plugin_initializer.rb:349:in `execute_validators'
    /home/ekohl/dev/smart-proxy/lib/proxy/plugin_initializer.rb:343:in `validate_settings'
    /home/ekohl/dev/smart-proxy/lib/proxy/plugin_initializer.rb:289:in `load_settings'
    /home/ekohl/dev/smart-proxy/lib/proxy/plugin_initializer.rb:264:in `load_plugin_settings'
    /home/ekohl/dev/smart-proxy/lib/proxy/plugin_initializer.rb:80:in `load_plugin_settings'
    /home/ekohl/dev/smart-proxy/lib/proxy/plugin_initializer.rb:168:in `each'
    /home/ekohl/dev/smart-proxy/lib/proxy/plugin_initializer.rb:168:in `initialize_plugins'
    /home/ekohl/dev/smart_proxy_remote_execution_ssh/test/integration_test.rb:11:in `app'
    /home/ekohl/.gem/ruby/gems/rack-test-1.1.0/lib/rack/test/methods.rb:30:in `build_rack_mock_session'
    /home/ekohl/.gem/ruby/gems/rack-test-1.1.0/lib/rack/test/methods.rb:26:in `rack_mock_session'
    /home/ekohl/.gem/ruby/gems/rack-test-1.1.0/lib/rack/test/methods.rb:41:in `build_rack_test_session'
    /home/ekohl/.gem/ruby/gems/rack-test-1.1.0/lib/rack/test/methods.rb:37:in `rack_test_session'
    /home/ekohl/.gem/ruby/gems/rack-test-1.1.0/lib/rack/test/methods.rb:45:in `current_session'
    /usr/share/ruby/forwardable.rb:232:in `get'
    /home/ekohl/dev/smart_proxy_remote_execution_ssh/test/integration_test.rb:64:in `test_features_with_dynflow_and_required_options'

@adamruzicka any idea how I would use validate_presence with if:?

adamruzicka · 2021-12-13T15:00:18Z

lib/smart_proxy_remote_execution_ssh/plugin.rb

+      settings[:ssh_log_level] = settings[:ssh_log_level].to_sym
+    end
+
+    validate_readable :ssh_identity_key_file, :ssh_identity_public_key_file


Am I reading it right that this doesn't expand the path?

No, it doesn't. Perhaps it needs to be in load_programmable_settings instead of lib/smart_proxy_remote_execution_ssh.rb?

Also, does this plugin still use the SSH keys if MQTT is used? I think so due to cockpit, but I'm starting to wonder if this shouldn't be named smart_proxy_remote_execution instead of ..._ssh.

Does it make sense to expose the mode as a setting so you can externally read that out? Or as a capability?

ekohl · 2021-12-13T15:00:23Z

any idea how I would use validate_presence with if:?

theforeman/smart-proxy#806 is what made it work for me. Looks like this is an unused feature.

Introduced in Smart Proxy 2.3 and this simplifies the code.

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.

Loading the smart_proxy_dynflow (or any Smart Proxy plugin really) in load_classes does not work due to the way plugin initialization happens. For this plugin to work, dynflow must be running. This uses requires to validate that dynflow is actually correctly enabled and running. This also means that if dynflow was loaded but ended up in a failed state that REX SSH also ends up in a failed state.

ekohl · 2021-12-13T16:28:14Z

test/integration_test.rb

+require 'root/root_v2_api'
+require 'smart_proxy_remote_execution_ssh/plugin'
+
+class SmartProxyRemoteExecutionSshApiFeaturesTest < MiniTest::Test


There is currently no test for the legacy async-ssh mode. Would probably be useful.

theforeman-bot added Waiting on contributor Needs testing Not yet reviewed labels Jun 24, 2021

ekohl force-pushed the more-native-methods branch from 85bf5ff to 20e9684 Compare June 24, 2021 09:27

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels Jun 24, 2021

ekohl force-pushed the more-native-methods branch from 20e9684 to 07c6231 Compare July 9, 2021 14:48

ekohl marked this pull request as ready for review July 9, 2021 14:48

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels Jul 9, 2021

ekohl force-pushed the more-native-methods branch from 07c6231 to 06cc6de Compare July 9, 2021 15:23

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels Jul 9, 2021

ekohl mentioned this pull request Jul 16, 2021

Fixes #32971 - Do not enable by default #59

Closed

adamruzicka mentioned this pull request Aug 12, 2021

Groundwork for pull provider #64

Merged

4 tasks

ekohl force-pushed the more-native-methods branch from 06cc6de to 358bc09 Compare August 16, 2021 11:28

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels Aug 16, 2021

ekohl mentioned this pull request Dec 13, 2021

Update smart_proxy_remote_execution_ssh to 0.5.0 theforeman/foreman-packaging#7355

Merged

Use load_test_settings in test helper

91bafe7

This properly loads the test settings, which includes loading defaults. This means no workaround is needed.

ekohl force-pushed the more-native-methods branch from 358bc09 to 071a607 Compare December 13, 2021 14:14

theforeman-bot removed the Waiting on contributor label Dec 13, 2021

theforeman-bot added the Waiting on contributor label Dec 13, 2021

ekohl force-pushed the more-native-methods branch from 071a607 to 5d7632d Compare December 13, 2021 14:37

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels Dec 13, 2021

adamruzicka reviewed Dec 13, 2021

View reviewed changes

ekohl added 5 commits December 13, 2021 17:00

Add integration test

c4b8160

Use the rackup_path helper

88b0fc3

Introduced in Smart Proxy 2.3 and this simplifies the code.

Return a 404 if the public key file doesn't exist

b3d0c0d

ekohl commented Dec 13, 2021

View reviewed changes

ekohl force-pushed the more-native-methods branch from 5d7632d to 47f71ca Compare December 13, 2021 16:28

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels Dec 13, 2021

ekohl mentioned this pull request Dec 14, 2021

Fixes #34141 - Make validators modern Ruby compatible theforeman/smart-proxy#806

Merged

Use more native methods of SmartProxy for validation #57

Are you sure you want to change the base?

Use more native methods of SmartProxy for validation #57

Uh oh!

Conversation

ekohl commented Jun 24, 2021

Uh oh!

adamruzicka commented Jun 24, 2021

Uh oh!

ekohl commented Jun 24, 2021

Uh oh!

ekohl commented Jul 9, 2021

Uh oh!

adamruzicka commented Aug 12, 2021

Uh oh!

adamruzicka commented Aug 16, 2021

Uh oh!

ekohl commented Aug 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamruzicka commented Aug 16, 2021

Uh oh!

ekohl commented Aug 16, 2021

Uh oh!

ekohl commented Dec 13, 2021

Uh oh!

adamruzicka Dec 13, 2021

Choose a reason for hiding this comment

Uh oh!

ekohl Dec 13, 2021

Choose a reason for hiding this comment

Uh oh!

ekohl Dec 13, 2021

Choose a reason for hiding this comment

Uh oh!

ekohl commented Dec 13, 2021

Uh oh!

ekohl Dec 13, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ekohl commented Aug 16, 2021 •

edited

Loading