Refs #33760 - Add reports proxy plugin #707

ofedoren · 2021-10-21T00:51:29Z

I've just noticed https://projects.theforeman.org/issues/33704...

Update based on the issue

ofedoren

Actually I need some help to write this properly...

ofedoren · 2021-10-21T00:58:10Z

manifests/plugin/host_reports.pp

+  file_line { "set report type in proxy ansible.cfg":
+    ensure => present,
+    path => "${foreman_proxy::config_dir}/ansible.cfg",
+    line => "report_type = proxy",
+    after => "^[callback_foreman]"
+  } ->
+  file_line { "set proxy url in proxy ansible.cfg":
+    ensure => present,
+    path => "${foreman_proxy::config_dir}/ansible.cfg",
+    line => "proxy_url = ${foreman_proxy::real_registered_proxy_url}",
+    after => "^[callback_foreman]"
+  }


@lzap, do we want the changes like report_type = proxy to be in the proxy's ansible.cfg? Or we want to change /etc/ansible/ansible.cfg? Or even make this a variable?

@ekohl, how do I write a check in Puppet to append some lines if the file exists? In this case I guess we need to rely on preconfigured ansible.cfg, since a user must have working ansible setup to be able to send/receive reports...

The plan is to add support into installer for changing Ansible configuration but leaving the default on the old setting. Then for 3.2 we want to make it the default and remove core feature leaving the plugin as the only way for reporting.

ofedoren · 2021-10-21T00:59:19Z

manifests/plugin/host_reports.pp

+
+  foreman_proxy::plugin::module { 'host_reports':
+    enabled   => $enabled,
+    # feature   => 'HostReports',


Not sure if we need to introduce a new feature. Please, let me know, so I can send a PR to foreman_host_reports.

I think that would be wise. You can use that in permission checks on API endpoints. Is your current API permission check just "it needs to be any Smart Proxy"?

Additionally, setting feature here means the installer verifies that the Foreman recognizes the feature. That means the foreman_host_reports plugin is installed. If it isn't, there's effectively a mismatch. This mechanism helps to detect that mismatch.

Good point, unfortunately our plugin had underscore in the name: host_reports and our feature framework only exposes plugin names. Historically, we only have one-word features, even plugins like remote_execution_ssh use plugin name ssh.

I considered creating a framework to set different feature names in smart proxy, but our endpoint appear to export plugin names directly. Also any change in Foreman feature mapping have potential to break existing plugins (treating underscores differently). Therefore, I renamed the smart proxy plugin name to simply reports (in the initializer) so the feature name is now just Reports in Foreman. This maps cleanly and follows what we have been historically doing.

theforeman/smart_proxy_reports@c4d86ab

Update rubygem-smart_proxy_host_reports to 0.0.5 foreman-packaging#7194

theforeman/foreman_host_reports@eaadd75

Added foreman_host_reports 0.0.4 foreman-packaging#7173

@ofedoren since it was the free Friday, I pushed the changed directly into repos and made new releases, if you don't mind. :-) I do not want to waste time, we need installer changes for RC1.

Just to explain the previous comment better: host_reports feature from Proxy does map to Host_Reports feature in Foreman which I find way to long and weird. We might even merge Host Reports plugin into core in the future and entitle the pages just "Reports" or "Configuration Reports" so this is much better for the future. And shorter.

ofedoren · 2021-10-21T01:00:18Z

spec/classes/foreman_proxy__plugin__host_reports_spec.rb

+          ])
+        end
+
+        it 'should configure ansible.cfg' do


I guess this will fail since it needs a file to exist.

That's configured here:

puppet-foreman_proxy/manifests/plugin/ansible.pp

Lines 59 to 65 in e1f8a20

file {"${foreman_proxy::config_dir}/ansible.cfg":

ensure => file,

content => template('foreman_proxy/plugin/ansible.cfg.erb'),

owner => 'root',

group => $foreman_proxy::user,

mode => '0640',

}

Why do you expect it to configure it as part of this plugin?

Well, we don't want to fully configure this file, we just need to add two entries: report_type and proxy_url to actually enable redirection of the reports.

ofedoren · 2021-10-21T01:02:30Z

manifests/plugin/host_reports.pp

+  file_line { "set proxy url in proxy ansible.cfg":
+    ensure => present,
+    path => "${foreman_proxy::config_dir}/ansible.cfg",
+    line => "proxy_url = ${foreman_proxy::real_registered_proxy_url}",


I use here the default/current proxy URL, but I'm open to a better suggestion...

ofedoren · 2021-10-21T01:03:23Z

manifests/plugin/host_reports.pp

+  file_line { "set report type in proxy ansible.cfg":
+    ensure => present,
+    path => "${foreman_proxy::config_dir}/ansible.cfg",
+    line => "report_type = proxy",


~~If users install this plugin, I guess they need to be aware that it will work only with this setting in ansible.cfg, thus switching the method automatically.~~

ekohl

Do you want me close @lzap's issue as a duplicate or will you still use it?

ekohl · 2021-10-21T12:38:53Z

manifests/plugin/host_reports.pp

+#
+# === Parameters:
+#
+# $proxy_name::               Proxy hostname to appear in reports JSON


Shouldn't this be an advanced parameter? I wonder in which case we actually do want this to be different than the registered name.

For context, the "standard" parameters section is shown with foreman-installer --help and advanced parameters are only shown with foreman-installer --full-help.

I don't have strong opinion here, but I think it's safer even for the user to not change that unless they know what they are doing. But in that case it's easy to change in the config itself.

ekohl · 2021-10-21T12:39:23Z

manifests/plugin/host_reports.pp

+class foreman_proxy::plugin::host_reports (
+  Optional[String] $proxy_name = undef,
+  Stdlib::Absolutepath $spool_dir = '/var/lib/foreman-proxy/host_reports',
+  Optional[String] $keep_reports = false,


This looks more like a Boolean

ekohl · 2021-10-21T12:41:20Z

manifests/plugin/host_reports.pp

+  Stdlib::Absolutepath $spool_dir = '/var/lib/foreman-proxy/host_reports',
+  Optional[String] $keep_reports = false,
+  # No reports are redirected by default
+  Optional[String] $report_type = 'foreman',


If there's a default, it's not Optional. In Puppet the type Optional[String] is equivalent to Variant[Undef, String] but you can't get back to undef if there's a default. If there are just 2 values possible (as the documentation suggests) you can also use an even stricter type:

Suggested change

Optional[String] $report_type = 'foreman',

Enum['foreman', 'proxy'] $report_type = 'foreman',

ekohl · 2021-10-21T12:44:11Z

spec/classes/foreman_proxy__plugin__host_reports_spec.rb

+          ])
+        end
+
+        it 'should configure ansible.cfg' do


That's configured here:

puppet-foreman_proxy/manifests/plugin/ansible.pp

Lines 59 to 65 in e1f8a20

file {"${foreman_proxy::config_dir}/ansible.cfg":

ensure => file,

content => template('foreman_proxy/plugin/ansible.cfg.erb'),

owner => 'root',

group => $foreman_proxy::user,

mode => '0640',

}

Why do you expect it to configure it as part of this plugin?

ekohl · 2021-10-21T12:45:05Z

templates/plugin/host_reports.yml.erb

+:enabled: <%= @module_enabled %>
+
+# Proxy hostname to appear in reports JSON
+:reported_proxy_hostname: <%= scope.lookupvar('::foreman_proxy::plugin::host_reports::reported_proxy_hostname') %>


It's no longer needed to use ::foreman_proxy. That was a thing for Puppet 3 and Puppet 4 to behave in the same way but thankfully we've dropped Puppet 3 a long time ago.

Suggested change

:reported_proxy_hostname: <%= scope.lookupvar('::foreman_proxy::plugin::host_reports::reported_proxy_hostname') %>

:reported_proxy_hostname: <%= scope.lookupvar('foreman_proxy::plugin::host_reports::reported_proxy_hostname') %>

ekohl · 2021-10-21T12:46:07Z

templates/plugin/host_reports.yml.erb

+# a regular basis (e.g. via cronjob).
+:keep_reports: <%= scope.lookupvar('::foreman_proxy::plugin::host_reports::keep_reports') %>
+
+# Development settings (do not use)


If these are "do not use", should we remove these lines?

ofedoren

Thanks, @ekohl, updated.

Do you want me close @lzap's issue as a duplicate or will you still use it?

Sure, I guess we don't need that.

Regarding #707 (comment), I still need to append those changes only if the file exists since Foreman can be used with Puppet only.

ofedoren · 2021-10-21T13:20:05Z

manifests/plugin/host_reports.pp

+#
+# === Parameters:
+#
+# $proxy_name::               Proxy hostname to appear in reports JSON


I don't have strong opinion here, but I think it's safer even for the user to not change that unless they know what they are doing. But in that case it's easy to change in the config itself.

ekohl · 2021-10-21T13:32:53Z

manifests/plugin/host_reports.pp

+  file_line { 'set report type in proxy ansible.cfg':
+    ensure => present,
+    path   => "${foreman_proxy::config_dir}/ansible.cfg",
+    line   => "report_type = ${report_type}",
+    after  => '^[callback_foreman]',
+  }
+  -> file_line { 'set proxy url in proxy ansible.cfg':
+    ensure => present,
+    path   => "${foreman_proxy::config_dir}/ansible.cfg",
+    line   => "proxy_url = ${foreman_proxy::real_registered_proxy_url}",
+    after  => '^[callback_foreman]',
+  }


You can't do this reliably since the Ansible plugin already manages the file. To have that as leading and modify the template:
https://github.com/theforeman/puppet-foreman_proxy/blob/master/templates/plugin/ansible.cfg.erb

It may mean that you need to introduce additional parameters in ansible.pp

ekohl · 2021-10-21T14:16:41Z

templates/plugin/ansible.cfg.erb

+proxy_url = <%= @proxy_url %>
+report_type = <%= @report_type %>
 url = <%= @foreman_url %>


Why does the callback have a url and proxy_url? Is that because facts are still sent to Foreman itself unconditionally?

Exactly. Also, AFAIU the proxy can have a different url than the server, so it's safer to configure it separately.

But @lzap can say more about that.

Not just can, its URL is always different since it's never on the same port. So yes, this makes sense.

ekohl · 2021-10-21T14:17:36Z

templates/plugin/host_reports.yml.erb

+# Proxy hostname to appear in reports JSON
+:reported_proxy_hostname: <%= scope.lookupvar('foreman_proxy::plugin::host_reports::reported_proxy_hostname') %>
+
+# Print intput and output to the debug level


Suggested change

# Print intput and output to the debug level

# Print input and output to the debug level

ekohl · 2021-10-21T14:18:36Z

spec/classes/foreman_proxy__plugin__host_reports_spec.rb

+
+      describe 'with default settings' do
+        it { should contain_foreman_proxy__plugin__module('host_reports') }
+        it 'host_reports.yml should contain the correct configuration' do


I think this reads a bit more natural:

Suggested change

it 'host_reports.yml should contain the correct configuration' do

it 'should contain the correct configuration in host_reports.yml' do

ekohl · 2021-10-21T14:19:11Z

manifests/plugin/host_reports.pp

+#
+# === Parameters:
+#
+# $spool_dir::                Spool directory with processed reports


Isn't this also more of an advanced parameter that most users wouldn't change?

ekohl · 2021-10-21T14:21:22Z

manifests/plugin/ansible.pp

@@ -50,11 +53,13 @@
  String $callback = $foreman_proxy::plugin::ansible::params::callback,
  String $runner_package_name = $foreman_proxy::plugin::ansible::params::runner_package_name,
  Array[Stdlib::Absolutepath] $collections_paths = $foreman_proxy::plugin::ansible::params::collections_paths,
+  Enum['foreman', 'proxy'] $report_type = $foreman_proxy::plugin::ansible::params::report_type,


Just thinking out loud: should proxy type automatically enable the host reporting feature? The only downside is that it wouldn't get disabled again if the users switches it to foreman again. I'd be OK with adding a line to the parameter documentation that tells the user to enable the host_reports plugin (both the Foreman and Proxy parts)

Not sure I follow what you mean... I see there are two ways for "enabling":

Install host_reports*; enable proxy in config.

Set report_type to proxy.

The whole feature (for Ansible) will work only if both ways are applied. For Puppet the first one only will do (but with additional configuration of puppet client/server).

UPD: Also, for now switching proxy to foreman should disable forwarding and should "return things they were". This wouldn't be the case in the future though, since we're going to remove old API and stuff.

The whole feature (for Ansible) will work only if both ways are applied. For Puppet the first one only will do (but with additional configuration of puppet client/server).

Right, so I'm wondering if you set this value to proxy, should we do something about warning the user they'll have an incompatible setup somehow. But as I said: I'm fine with adding a line to the parameter documentation in this class that's something like "If set to proxy, the host_reports plugin must be enabled".

~~Oh, right, will do.~~ Done.

ofedoren · 2021-10-21T14:43:13Z

manifests/plugin/host_reports.pp

+
+  foreman_proxy::plugin::module { 'host_reports':
+    enabled   => $enabled,
+    feature   => 'HostReports',


Returned this since it's going to be added in theforeman/foreman_host_reports#10. Probably, we need to extract that to not block this PR.

ekohl · 2021-10-25T12:36:04Z

spec/classes/foreman_proxy__plugin__host_reports_spec.rb

+      describe 'with default settings' do
+        it { should contain_foreman_proxy__plugin__module('host_reports') }
+        it 'should contain the correct configuration in host_reports.yml' do
+          verify_exact_contents(catalogue, '/etc/foreman-proxy/settings.d/host_reports.yml', [


Looks like this was renamed in 0.0.4 to just reports.yml. In theforeman/foreman-packaging#7194 (review) I suggested to rename everything to be consistent. Perhaps we should first have that discussion and then decide on what we do here.

ofedoren · 2021-10-26T13:59:14Z

Updated to match the renaming.

lzap · 2021-11-01T16:22:47Z

Packaging PR has been merged: theforeman/foreman-packaging#7194

ekohl

Thanks!

theforeman-bot added Needs testing Not yet reviewed labels Oct 21, 2021

ofedoren mentioned this pull request Oct 21, 2021

Fixes #33760 - Add host_reports plugins theforeman/foreman-installer#727

Merged

2 tasks

ofedoren commented Oct 21, 2021

View reviewed changes

ofedoren force-pushed the add-host-reports branch 3 times, most recently from c997cbb to d0cee78 Compare October 21, 2021 11:29

ofedoren changed the title ~~Add host_reports proxy plugin~~ Refs #33760 - Add host_reports proxy plugin Oct 21, 2021

ekohl reviewed Oct 21, 2021

View reviewed changes

ofedoren force-pushed the add-host-reports branch from d0cee78 to 13e5586 Compare October 21, 2021 13:23

ofedoren commented Oct 21, 2021

View reviewed changes

ekohl reviewed Oct 21, 2021

View reviewed changes

ofedoren force-pushed the add-host-reports branch from 13e5586 to fd1b960 Compare October 21, 2021 13:55

ekohl reviewed Oct 21, 2021

View reviewed changes

ofedoren force-pushed the add-host-reports branch from fd1b960 to c41428d Compare October 21, 2021 14:17

ekohl reviewed Oct 21, 2021

View reviewed changes

ofedoren force-pushed the add-host-reports branch from c41428d to 74bf17c Compare October 21, 2021 14:41

ofedoren commented Oct 21, 2021

View reviewed changes

ofedoren force-pushed the add-host-reports branch 2 times, most recently from b9674aa to 1dd2f24 Compare October 22, 2021 12:55

ehelms mentioned this pull request Oct 24, 2021

Release 20.0.0 #709

Merged

7 tasks

ekohl requested changes Oct 25, 2021

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Oct 25, 2021

ofedoren force-pushed the add-host-reports branch from 1dd2f24 to fe5b4b9 Compare October 26, 2021 13:56

theforeman-bot added Needs re-review and removed Waiting on contributor labels Oct 26, 2021

ofedoren force-pushed the add-host-reports branch from fe5b4b9 to 19d8513 Compare October 26, 2021 13:57

ofedoren changed the title ~~Refs #33760 - Add host_reports proxy plugin~~ Refs #33760 - Add reports proxy plugin Oct 26, 2021

Refs #33760 - Add reports proxy plugin

ecdb15b

ofedoren force-pushed the add-host-reports branch from 19d8513 to ecdb15b Compare October 26, 2021 13:58

ekohl approved these changes Nov 1, 2021

View reviewed changes

theforeman-bot removed the Needs re-review label Nov 1, 2021

ekohl merged commit e07f581 into theforeman:master Nov 1, 2021

ekohl added the Enhancement label Nov 1, 2021

	file {"${foreman_proxy::config_dir}/ansible.cfg":
	ensure => file,
	content => template('foreman_proxy/plugin/ansible.cfg.erb'),
	owner => 'root',
	group => $foreman_proxy::user,
	mode => '0640',
	}

	Optional[String] $report_type = 'foreman',
	Enum['foreman', 'proxy'] $report_type = 'foreman',

	:reported_proxy_hostname: <%= scope.lookupvar('::foreman_proxy::plugin::host_reports::reported_proxy_hostname') %>
	:reported_proxy_hostname: <%= scope.lookupvar('foreman_proxy::plugin::host_reports::reported_proxy_hostname') %>

	# Print intput and output to the debug level
	# Print input and output to the debug level

	it 'host_reports.yml should contain the correct configuration' do
	it 'should contain the correct configuration in host_reports.yml' do

Refs #33760 - Add reports proxy plugin #707

Refs #33760 - Add reports proxy plugin #707

Conversation

ofedoren commented Oct 21, 2021 • edited Loading

ofedoren left a 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

lzap Oct 22, 2021 • 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

ofedoren Oct 21, 2021 • edited Loading

Choose a reason for hiding this comment

ekohl left a 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

ofedoren left a comment • 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

ofedoren Oct 21, 2021 • 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

ofedoren Oct 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ofedoren Oct 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ofedoren commented Oct 26, 2021

lzap commented Nov 1, 2021

ekohl left a comment

Choose a reason for hiding this comment

ofedoren commented Oct 21, 2021 •

edited

Loading

lzap Oct 22, 2021 •

edited

Loading

ofedoren Oct 21, 2021 •

edited

Loading

ofedoren left a comment •

edited

Loading

ofedoren Oct 21, 2021 •

edited

Loading

ofedoren Oct 21, 2021 •

edited

Loading

ofedoren Oct 21, 2021 •

edited

Loading