Add rex cockpit integration support #748

iNecas · 2019-08-18T19:42:23Z

This adds --foreman-plugin-remote-execution-cockpit boolean option to
add support for cockpit installation.

It enables foreman-cockpit.service (comming from
rubygem-foreman_remote_execution-cockpit package), that provides the
backend service for the cockpit tunel, and configures the settings
and authentication for the whole setup to work.

Requires:

Update rubygem-foreman_remote_execution to 1.8.3 foreman-packaging#4018.

When testing, SELinux needs to be disabled for now, as there are rules missing
for the foreman-cockpit service to work in enforcing mode.

This adds --foreman-plugin-remote-execution-cockpit boolean option to add support for cockpit installation. It enables foreman-cockpit.service (comming from rubygem-foreman_remote_execution-cockpit package), that provides the backend service for the cockpit tunel, and configures the settings and authentication for the whole setup to work.

ekohl · 2019-08-19T09:41:04Z

manifests/plugin/remote_execution.pp

+# $cockpit:: Install cockpit support
+#
+class foreman::plugin::remote_execution (
+  Boolean $cockpit = $::foreman::plugin::remote_execution::params::cockpit,


It's ok not to inherit if it's a trivial parameter.

ekohl · 2019-08-19T09:43:12Z

manifests/plugin/remote_execution.pp

+      }
+    }
+
+    package { $cockpit_package:


You can replace this with foreman::plugin { 'remote_execution-cockpit': } and drop the whole package name determination.

ekohl · 2019-08-19T09:43:43Z

manifests/plugin/remote_execution.pp

+    service { 'foreman-cockpit':
+      ensure     => running,
+      enable     => true,
+      hasstatus  => true,


This defaults to true since puppet 2.7 and can be dropped:

Suggested change

hasstatus => true,

ekohl · 2019-08-19T09:43:53Z

manifests/plugin/remote_execution.pp

+      ensure     => running,
+      enable     => true,
+      hasstatus  => true,
+      hasrestart => true,


Same here

Suggested change

hasrestart => true,

ekohl · 2019-08-19T09:46:15Z

manifests/plugin/remote_execution.pp

+      ssl_content => template('foreman/cockpit-apache-ssl.conf.erb'),
+    }
+
+    foreman_config_entry { 'remote_execution_cockpit_url':


Why does this need to be configured? Isn't this the default and can we rely on that?

The default config is null and it actually determines whether the cockpit integration is enabled or not.

Since /webcon is repeated a few times, perhaps this should be a variable? Doesn't have to be a class parameter but it's a good way to show that they're the same thing.

ekohl · 2019-08-19T09:47:57Z

templates/cockpit-apache-ssl.conf.erb

+### File managed with puppet ###
+
+<Location /webcon>
+  LoadModule proxy_wstunnel_module modules/mod_proxy_wstunnel.so


Please don't use LoadModule this way. In the class you should include apache::mod::proxy_wstunnel.

ekohl · 2019-08-19T09:48:13Z

templates/cockpit-apache-ssl.conf.erb

+
+<Location /webcon>
+  LoadModule proxy_wstunnel_module modules/mod_proxy_wstunnel.so
+  LoadModule proxy_http_module modules/mod_proxy_http.so


Same here with include apache::mod::proxy_http

templates/remote_execution_cockpit.conf.erb

templates/remote_execution_cockpit_session.yml.erb

Co-Authored-By: Ewoud Kohl van Wijngaarden <ewoud@kohlvanwijngaarden.nl>

iNecas · 2019-09-02T16:13:23Z

Ready for review again

ekohl · 2019-09-20T16:25:25Z

templates/cockpit-apache-ssl.conf.erb

@@ -0,0 +1,11 @@
+### File managed with puppet ###
+
+<Location /<%= scope.lookupvar("cockpit_subpath") %>>


It's in the local scope so this is sufficient:

Suggested change

<Location /<%= scope.lookupvar("cockpit_subpath") %>>

<Location /<%= @cockpit_subpath %>>

ekohl · 2019-09-20T16:25:46Z

templates/cockpit-apache-ssl.conf.erb

+
+  RewriteEngine On
+  RewriteCond %{HTTP:Upgrade} =websocket [NC]
+  RewriteRule /<%= scope.lookupvar("cockpit_subpath") %>/(.*)           ws://127.0.0.1:9999/<%= scope.lookupvar("cockpit_subpath") %>/$1 [P]


Suggested change

RewriteRule /<%= scope.lookupvar("cockpit_subpath") %>/(.*) ws://127.0.0.1:9999/<%= scope.lookupvar("cockpit_subpath") %>/$1 [P]

RewriteRule /<%= @cockpit_subpath %>/(.*) ws://127.0.0.1:9999/<%= @cockpit_subpath %>/$1 [P]

ekohl · 2019-09-20T16:26:29Z

templates/cockpit-apache-ssl.conf.erb

+  RewriteCond %{HTTP:Upgrade} =websocket [NC]
+  RewriteRule /<%= scope.lookupvar("cockpit_subpath") %>/(.*)           ws://127.0.0.1:9999/<%= scope.lookupvar("cockpit_subpath") %>/$1 [P]
+  RewriteCond %{HTTP:Upgrade} !=websocket [NC]
+  RewriteRule /<%= scope.lookupvar("cockpit_subpath") %>/(.*)           http://127.0.0.1:9999/<%= scope.lookupvar("cockpit_subpath") %>/$1 [P]


Suggested change

RewriteRule /<%= scope.lookupvar("cockpit_subpath") %>/(.*) http://127.0.0.1:9999/<%= scope.lookupvar("cockpit_subpath") %>/$1 [P]

RewriteRule /<%= @cockpit_subpath %>/(.*) http://127.0.0.1:9999/<%= @cockpit_subpath %>/$1 [P]

ekohl · 2019-09-20T16:26:46Z

templates/remote_execution_cockpit.conf.erb

@@ -0,0 +1,14 @@
+[WebService]
+LoginTitle = Foreman Cockpit
+UrlRoot = /<%= scope.lookupvar("cockpit_subpath") %>/


Suggested change

UrlRoot = /<%= scope.lookupvar("cockpit_subpath") %>/

UrlRoot = /<%= @cockpit_subpath %>/

ekohl · 2019-09-20T16:28:45Z

manifests/plugin/remote_execution.pp

  include ::foreman::plugin::tasks
+  include apache::mod::proxy_wstunnel


Can you move this into the if $cockpit block?

ekohl · 2019-09-20T16:30:01Z

manifests/plugin/remote_execution.pp

+  $cockpit_subpath = 'webcon'
+
+  if $cockpit {
+    if $::osfamily != 'RedHat' {


I'm a bit on the fence about this. Is there a reason why it can't work on Debian?

ekohl · 2019-09-20T16:30:55Z

manifests/plugin/remote_execution.pp

+      enable => true,
+    }
+
+    file { '/etc/foreman/cockpit/cockpit.conf':


If the package creates /etc/foreman/cockpit then it also needs a require on Foreman::Plugin['remote_execution-cockpit']. Otherwise you need to ensure the directory exists.

iNecas · 2019-09-23T07:34:32Z

Unfortunately, my focus has already shifted quite far from this PR, therefore I would suggest someone else to pick this up to finish the comments. @ekohl with you Puppet experience, it seems like it might be quite low hanging fruit for you to shape it the way you need? Otherwise, reaching to @ares to try to find another set of hands for this one. It could be good also for actually sharing some knowledge about this functionality.

tstrych · 2019-09-24T11:11:13Z

Not sure if this helps, but I was able with @adamruzicka help to install theforeman/foreman-packaging#3816 and patch it with this PR and cockpit seems to work correctly.

Hopefully this will be merged soon, as this is last step from developer side of work, for now.

ekohl · 2019-09-24T15:38:41Z

templates/remote_execution_cockpit_session.yml.erb

@@ -0,0 +1,4 @@
+:foreman_url: <%= scope.lookupvar("foreman::foreman_url") %>
+:ssl_ca_file: <%= scope.lookupvar("foreman::client_ssl_ca") %>


I'm picking this up, but why are these needed? From reading the docs this appears to be to connect back to Foreman. It's not guarantees these certs actually work for that. When connecting to Foreman itself you should use the server CA.

Since you're moving away from Foreman, should I pick this up with @adamruzicka?

ack on that, it should probably be the server ca

Thanks for taking over this.

ekohl · 2019-09-24T15:41:19Z

Replaced by #760

theforeman-bot added Needs testing Not yet reviewed labels Aug 18, 2019

ekohl requested changes Aug 19, 2019

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Aug 19, 2019

Update templates/remote_execution_cockpit.conf.erb

d869959

Co-Authored-By: Ewoud Kohl van Wijngaarden <ewoud@kohlvanwijngaarden.nl>

theforeman-bot added Needs re-review and removed Waiting on contributor labels Aug 19, 2019

Update templates/remote_execution_cockpit_session.yml.erb

68658fc

Co-Authored-By: Ewoud Kohl van Wijngaarden <ewoud@kohlvanwijngaarden.nl>

Address review comments

3964fac

iNecas force-pushed the rex-cockpit branch from 3d0cce0 to 3964fac Compare September 3, 2019 09:34

ekohl requested changes Sep 20, 2019

View reviewed changes

theforeman-bot added Waiting on contributor and removed Needs re-review labels Sep 20, 2019

ekohl reviewed Sep 24, 2019

View reviewed changes

ekohl mentioned this pull request Sep 24, 2019

Fixes #27932 - Add REX Cockpit support #760

Merged

ekohl closed this Sep 24, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add rex cockpit integration support #748

Add rex cockpit integration support #748

iNecas commented Aug 18, 2019

ekohl Aug 19, 2019

iNecas Sep 2, 2019

ekohl Aug 19, 2019

iNecas Sep 2, 2019

ekohl Aug 19, 2019

iNecas Sep 2, 2019

ekohl Aug 19, 2019

ekohl Aug 19, 2019

iNecas Aug 19, 2019

ekohl Aug 19, 2019

iNecas Sep 2, 2019

ekohl Aug 19, 2019

iNecas Sep 2, 2019

ekohl Aug 19, 2019

iNecas Sep 2, 2019

iNecas commented Sep 2, 2019 •

edited

Loading

ekohl Sep 20, 2019

ekohl Sep 20, 2019

ekohl Sep 20, 2019

ekohl Sep 20, 2019

ekohl Sep 20, 2019

ekohl Sep 20, 2019

ekohl Sep 20, 2019

iNecas commented Sep 23, 2019

tstrych commented Sep 24, 2019

ekohl Sep 24, 2019

iNecas Sep 25, 2019

iNecas Sep 25, 2019

ekohl commented Sep 24, 2019

		@@ -0,0 +1,11 @@
		### File managed with puppet ###

		<Location /<%= scope.lookupvar("cockpit_subpath") %>>

	<Location /<%= scope.lookupvar("cockpit_subpath") %>>
	<Location /<%= @cockpit_subpath %>>

	RewriteRule /<%= scope.lookupvar("cockpit_subpath") %>/(.*) ws://127.0.0.1:9999/<%= scope.lookupvar("cockpit_subpath") %>/$1 [P]
	RewriteRule /<%= @cockpit_subpath %>/(.*) ws://127.0.0.1:9999/<%= @cockpit_subpath %>/$1 [P]

	UrlRoot = /<%= scope.lookupvar("cockpit_subpath") %>/
	UrlRoot = /<%= @cockpit_subpath %>/

		include ::foreman::plugin::tasks
		include apache::mod::proxy_wstunnel

		@@ -0,0 +1,4 @@
		:foreman_url: <%= scope.lookupvar("foreman::foreman_url") %>
		:ssl_ca_file: <%= scope.lookupvar("foreman::client_ssl_ca") %>

Add rex cockpit integration support #748

Add rex cockpit integration support #748

Conversation

iNecas commented Aug 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iNecas commented Sep 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iNecas commented Sep 23, 2019

tstrych commented Sep 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekohl commented Sep 24, 2019

iNecas commented Sep 2, 2019 •

edited

Loading