Groundwork for pull provider #64

adamruzicka · 2021-08-12T12:19:58Z

TODOs:

expose this in settings. This will rely on some things pioneered in Use more native methods of SmartProxy for validation #57
Address todos in code
(optional) Add support for token-based auth
(optional) Switch polling script runner to the new api

ezr-ondrej

We need to work with subscription manager generated UUID instead of hostname, that's where the yggdrassil client listens for mqtt messages :(

lib/smart_proxy_remote_execution_ssh/actions/pull_script.rb

adamruzicka · 2021-08-24T13:07:04Z

We need to work with subscription manager generated UUID instead of hostname

Well, since foreman doesn't know what the proxy side needs, we'll need to send both and let the proxy pick. That means we'll need an extension point on Foreman's side where katello can inject the uuid into the job. Proxy will get a list of "names" and either will pick one depending on the mode, or in case of pull it will make the job available under all of them. How does that sound?

ezr-ondrej · 2021-08-25T07:12:56Z

Well, since foreman doesn't know what the proxy side needs, we'll need to send both and let the proxy pick. That means we'll need an extension point on Foreman's side where katello can inject the uuid into the job.

Sounds like exactly what we will need.

in case of pull it will make the job available under all of them

you mean to publish under the topics of the hostname AND uuid? sounds interesting, but probably better safe than sorry

adamruzicka · 2021-08-25T07:17:26Z

you mean to publish under the topics of the hostname AND uuid?

Probably not publish on mqtt using both, but through the api the job should be obtainable with a cert containing either of those

ezr-ondrej · 2021-08-25T07:52:56Z

Actually the mqtt topic depends on cert CN:
https://github.com/RedHatInsights/yggdrasil/blob/c5dc5ebeb1ec6326a5102cd5460b7209105b968d/cmd/yggd/main.go#L162

ezr-ondrej

Few more typos and we work if we introduce a hack to check for the uuid as well as for the hostname.

lib/smart_proxy_remote_execution_ssh/job_storage.rb

lib/smart_proxy_remote_execution_ssh/actions/pull_script.rb

adamruzicka · 2021-09-30T07:53:02Z

lib/smart_proxy_remote_execution_ssh/actions/pull_script.rb

+        # Otherwise we have to wait it out
+        if input[:with_mqtt]
+          cleanup
+          payload = {} # TODO


@ezr-ondrej What is the expected payload for job cancellation?

It seems to be quite up to us, there is not much documentation on the topic in https://github.com/RedHatInsights/cloud-connector#data tho I'd need to check wether it won't always try to fech content, but probably not if we ommit it.
then I'd go with data message, directive cancel ?

ezr-ondrej

The cancel is not exactly working and will need to be pollished (partly because of my missinformation), could we split that to a separate PR? All the other bits are working as they should, so I'd merge that and continue with just the cancalation.

ezr-ondrej · 2021-10-12T09:35:50Z

lib/smart_proxy_remote_execution_ssh/actions/pull_script.rb

+        # Client was notified or is already running, dealing with this situation
+        # is only supported if mqtt is available
+        # Otherwise we have to wait it out
+        mqtt_cancel if input[:with_mqtt]


What about switching the order, just in case mqtt_cancel fails for some reason?

Suggested change

mqtt_cancel if input[:with_mqtt]

mqtt_cancel and cleanup if input[:with_mqtt]

ezr-ondrej · 2021-10-12T10:31:44Z

lib/smart_proxy_remote_execution_ssh/plugin.rb

 module Proxy::RemoteExecution::Ssh
  class Plugin < Proxy::Plugin
    SSH_LOG_LEVELS = %w[debug info warn error fatal].freeze
+    MODES = %i[ssh async-ssh pull pull-mqtt]


Suggested change

MODES = %i[ssh async-ssh pull pull-mqtt]

MODES = %i[ssh async-ssh pull pull-mqtt].freeze

ezr-ondrej · 2021-10-12T11:46:26Z

lib/smart_proxy_remote_execution_ssh.rb

 require 'smart_proxy_remote_execution_ssh/version'
 require 'smart_proxy_remote_execution_ssh/plugin'
 require 'smart_proxy_remote_execution_ssh/webrick_ext'
+require 'sequel'


Are we using it somewhere? 🤔

https://github.com/theforeman/smart_proxy_remote_execution_ssh/pull/64/files#diff-0dfc8e9074322ee9f207169162199e1dd15edd7005e183840e3b34c017cca364R6 I'll move it around

lib/smart_proxy_remote_execution_ssh/actions/pull_script.rb

adamruzicka · 2021-10-12T13:02:20Z

Removed the cancellation for now, will open another PR once this goes in

- now uses in-memory sqlite - each job is identified by its uuid - get /jobs returns a list of job uuids available for a given host - get /jobs/:job_uuid returns the job contents - post /jobs/:job_uuid/update lets client upload updates for a job

Co-authored-by: Ondřej Ezr <[email protected]>

ezr-ondrej · 2021-10-12T13:57:40Z

Thanks @adamruzicka ! 👍

theforeman-bot added Waiting on contributor Needs testing Not yet reviewed labels Aug 12, 2021

adamruzicka force-pushed the mqtt branch from 67bd611 to 4d8cd7d Compare August 12, 2021 17:10

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

ezr-ondrej reviewed Aug 24, 2021

View reviewed changes

lib/smart_proxy_remote_execution_ssh/actions/pull_script.rb Outdated Show resolved Hide resolved

lib/smart_proxy_remote_execution_ssh/actions/pull_script.rb Outdated Show resolved Hide resolved

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

ezr-ondrej reviewed Aug 25, 2021

View reviewed changes

lib/smart_proxy_remote_execution_ssh/job_storage.rb Outdated Show resolved Hide resolved

lib/smart_proxy_remote_execution_ssh/actions/pull_script.rb Outdated Show resolved Hide resolved

lib/smart_proxy_remote_execution_ssh/actions/pull_script.rb Show resolved Hide resolved

ezr-ondrej reviewed Aug 25, 2021

View reviewed changes

lib/smart_proxy_remote_execution_ssh/actions/pull_script.rb Outdated Show resolved Hide resolved

ezr-ondrej reviewed Aug 25, 2021

View reviewed changes

lib/smart_proxy_remote_execution_ssh/actions/pull_script.rb Outdated Show resolved Hide resolved

lib/smart_proxy_remote_execution_ssh/actions/pull_script.rb Outdated Show resolved Hide resolved

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels Sep 3, 2021

adamruzicka commented Sep 30, 2021

View reviewed changes

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels Oct 12, 2021

adamruzicka marked this pull request as ready for review October 12, 2021 08:11

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels Oct 12, 2021

ezr-ondrej reviewed Oct 12, 2021

View reviewed changes

adamruzicka force-pushed the mqtt branch from 56b15a2 to 16b06d0 Compare October 12, 2021 12:52

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels Oct 12, 2021

adamruzicka and others added 12 commits October 12, 2021 15:31

Fixes #33682 - Groundwork for pull provider

c839d90

Fixes #33682 - Make it configurable

54efb6b

Fixes #33682 - Rework storage

ca28a30

- now uses in-memory sqlite - each job is identified by its uuid - get /jobs returns a list of job uuids available for a given host - get /jobs/:job_uuid returns the job contents - post /jobs/:job_uuid/update lets client upload updates for a job

Fixes #33682 - Extract job store operations into a separate class

470ecca

Fixes #33682 - Add support for password based auth

b117ce7

Fixes #33682 - Cleaner event dispatch

870f2fc

Fixes #33682 - Apply suggestions from code review

dc26154

Co-authored-by: Ondřej Ezr <[email protected]>

Fixes #33682 - Apply suggestions from code review

c1eee2e

Co-authored-by: Ondřej Ezr <[email protected]>

Fixes #33682 - Prefer consumer uuid over hostname, if available

6462ce5

Fixes #33682 - Slightly better handling of async_ssh setting

0f8a2be

Fixes #33682 - Drop cancellation for now

3a2c9de

Fixes #33682 - Address review comments

0873377

adamruzicka force-pushed the mqtt branch from 16b06d0 to 0873377 Compare October 12, 2021 13:31

theforeman-bot removed the Waiting on contributor label Oct 12, 2021

ezr-ondrej merged commit 611f315 into theforeman:master Oct 12, 2021

adamruzicka deleted the mqtt branch October 21, 2021 10:39

	mqtt_cancel if input[:with_mqtt]
	mqtt_cancel and cleanup if input[:with_mqtt]

	MODES = %i[ssh async-ssh pull pull-mqtt]
	MODES = %i[ssh async-ssh pull pull-mqtt].freeze

Groundwork for pull provider #64

Groundwork for pull provider #64

Uh oh!

Conversation

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

Uh oh!

Uh oh!

ezr-ondrej left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

adamruzicka commented Aug 24, 2021

Uh oh!

ezr-ondrej commented Aug 25, 2021

Uh oh!

adamruzicka commented Aug 25, 2021

Uh oh!

ezr-ondrej commented Aug 25, 2021

Uh oh!

ezr-ondrej left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adamruzicka Sep 30, 2021

Choose a reason for hiding this comment

Uh oh!

ezr-ondrej Oct 4, 2021

Choose a reason for hiding this comment

Uh oh!

ezr-ondrej left a comment

Choose a reason for hiding this comment

Uh oh!

ezr-ondrej Oct 12, 2021

Choose a reason for hiding this comment

Uh oh!

ezr-ondrej Oct 12, 2021

Choose a reason for hiding this comment

Uh oh!

ezr-ondrej Oct 12, 2021

Choose a reason for hiding this comment

Uh oh!

adamruzicka Oct 12, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

adamruzicka commented Oct 12, 2021

Uh oh!

ezr-ondrej commented Oct 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

adamruzicka commented Aug 12, 2021 •

edited

Loading