Configure policies within profile

[teluq-pbrideau]

I would like to configure my policies from within my own profile instead of using hiera directly, something like

class profile::choria (
  Boolean       $server = true,
  Boolean       $client = false,
  Array[String] $admins = [],
) {
  class { 'choria':
    manage_mcollective => false,
  }
  class { 'mcollective':
    client => $client,
    server => $server,
    plugin_classes => [
      'mcollective_choria',
      'mcollective_util_actionpolicy',
      'choria',
    ],
  }

  $_puppet_policies_admins = $admins.map |$a| {
    {
      action  => 'allow',
      callers => "choria=${a}",
      actions => 'status runonce enable disable',
      facts   => '*',
      classes => '*',
    }
  }
  class { 'mcollective_agent_puppet':
    policies => $_puppet_policies_admins,
  }
}

Where i can simply add in my hiera config for the node

profile::choria::client: true
profile::choria::admins:
  - admin1.mcollective
  - admin2.mcollective

Problem I encountered

This module does not read configs from the mcollective class,

Error: Could not retrieve catalog from remote server: Error 500 on SERVER: Server Error: Evaluation Error: Error while evaluating a Resource Statement, Class[Mcollective_agent_puppet]:
  parameter 'policy_default' expects a match for Mcollective::Policy_action = Enum['allow', 'deny'], got Undef
  parameter 'client' expects a Boolean value, got Undef
  parameter 'server' expects a Boolean value, got Undef

I’ve tried to order without success

  class { 'mcollective_agent_puppet':
    policies => $_puppet_policies_admins,
    require  => Class['mcollective'],
  }

so i must define

  class { 'mcollective_agent_puppet':
    policy_default => 'deny',
    client         => $client,
    server         => $server,
    policies       => $_puppet_policies_admins,
  }

But even then, it still does not work… I’ve tried defining explicitly the hiera configs like this:

  class { 'mcollective_agent_puppet':
    policy_default     => 'deny',
    client             => $client,
    server             => $server,
    policies           => $_puppet_policies_admins,
    server_files       => [
      'agent/puppet.rb',
    ],
    client_files       => [
      'application/puppet.rb',
      'aggregate/boolean_summary.rb',
      'aggregate/boolean_summary.ddl',
    ],
    common_directories => [
      'util/puppet_agent_mgr',
    ],
    common_files       => [
      'data/puppet_data.rb',
      'data/resource_data.rb',
      'data/puppet_data.ddl',
      'data/resource_data.ddl',
      'util/puppet_agent_mgr/mgr_v2.rb',
      'util/puppet_agent_mgr/mgr_v3.rb',
      'util/puppet_agent_mgr/mgr_windows.rb',
      'util/puppet_agent_mgr.rb',
      'util/puppet_server_address_validation.rb',
      'util/puppetrunner.rb',
      'validator/puppet_resource_validator.rb',
      'validator/puppet_server_address_validator.rb',
      'validator/puppet_tags_validator.rb',
      'validator/puppet_variable_validator.rb',
      'validator/puppet_resource_validator.ddl',
      'validator/puppet_server_address_validator.ddl',
      'validator/puppet_tags_validator.ddl',
      'validator/puppet_variable_validator.ddl',
      'agent/puppet.ddl',
      'agent/puppet.json',
    ],
  }

At last I have succeeded!

Therefore, my questions are

• Am I trying to do something that could be considered bad practice?
• Do you intend for this kind of feature to be available in the future?

The change is quite simple from my point of view, only remove default values from the init.pp so it fetch right the plugin.yml hiera config. But maybe you have a reason to have the empty default values?

--- init.pp 2024-11-08 14:12:26.779765400 -0500
+++ init2.pp    2024-11-08 14:12:20.959713882 -0500
@@ -1,12 +1,12 @@
 class mcollective_agent_puppet (
   String $config_name,
-  Array[String] $client_files = [],
-  Array[String] $client_directories = [],
-  Array[String] $server_files = [],
-  Array[String] $server_directories = [],
-  Array[String] $common_files = [],
-  Array[String] $common_directories = [],
-  Array[String] $executable_files = [],
+  Array[String] $client_files,
+  Array[String] $client_directories,
+  Array[String] $server_files,
+  Array[String] $server_directories,
+  Array[String] $common_files,
+  Array[String] $common_directories,
+  Array[String] $executable_files,
   Boolean $manage_gem_dependencies = true,
   Hash $gem_dependencies = {},
   Boolean $manage_package_dependencies = true,

I would gladly offer a PR if you think it would be OK on your side!

[ripienaar]

Defaults tend to be there just to avoid everyone having to specify everything when some things are not needed

I think I prefer the hierarchy based approach over explicitly defining in code - you could achieve your goal with a small wrapper define I think and it would be less annoying

[teluq-pbrideau]

Defaults tend to be there just to avoid everyone having to specify everything when some things are not needed

Seems to me that «specify everythiny» is what I have to do in this scenario.

Or do you mean to bring the defaults to the init.pp?

diff --git a/manifests/init.pp b/manifests/init.pp
index 69cff32..36078e8 100644
--- a/manifests/init.pp
+++ b/manifests/init.pp
@@ -1,11 +1,36 @@
 class mcollective_agent_puppet (
   String $config_name,
-  Array[String] $client_files = [],
+  Array[String] $client_files = [
+    'application/puppet.rb',
+    'aggregate/boolean_summary.rb',
+    'aggregate/boolean_summary.ddl',
+  ],
   Array[String] $client_directories = [],
-  Array[String] $server_files = [],
+  Array[String] $server_files = ['agent/puppet.rb'],
   Array[String] $server_directories = [],
-  Array[String] $common_files = [],
-  Array[String] $common_directories = [],
+  Array[String] $common_files = [
+    'data/puppet_data.rb',
+    'data/resource_data.rb',
+    'data/puppet_data.ddl',
+    'data/resource_data.ddl',
+    'util/puppet_agent_mgr/mgr_v2.rb',
+    'util/puppet_agent_mgr/mgr_v3.rb',
+    'util/puppet_agent_mgr/mgr_windows.rb',
+    'util/puppet_agent_mgr.rb',
+    'util/puppet_server_address_validation.rb',
+    'util/puppetrunner.rb',
+    'validator/puppet_resource_validator.rb',
+    'validator/puppet_server_address_validator.rb',
+    'validator/puppet_tags_validator.rb',
+    'validator/puppet_variable_validator.rb',
+    'validator/puppet_resource_validator.ddl',
+    'validator/puppet_server_address_validator.ddl',
+    'validator/puppet_tags_validator.ddl',
+    'validator/puppet_variable_validator.ddl',
+    'agent/puppet.ddl',
+    'agent/puppet.json',
+  ],
+  Array[String] $common_directories = ['util/puppet_agent_mgr'],
   Array[String] $executable_files = [],
   Boolean $manage_gem_dependencies = true,
   Hash $gem_dependencies = {},

you could achieve your goal with a small wrapper define I think

I do not understand what you mean, would you care to provide a basic example?

[ripienaar]

These files are basically generated by code and made specifically for using with Hiera, that's the only supported method.

So they default to empty values, honestly for reasons lost in time this was written like a decade ago - publishing to the forge 6 or 7 years ago only but its using data structures from a decade ago.

I wouldn't expect any chances.

If it's not reading values from the mcollective class it could be a ordering thing, are you including mcollective before this in code?

[teluq-pbrideau]

If it's not reading values from the mcollective class it could be a ordering thing, are you including mcollective before this in code?

That is also what I thought, I tried ordering explicitly with require => Class['mcollective’]. mcollective is not called anywhere else in code.

[ripienaar]

Not that kind of ordering:

Good:

include mcollective
class{"mcollective_agent_puppet: .... }

Bad:

class{"mcollective_agent_puppet: .... }
include mcollective

[ripienaar]

And in this case specifically maybe you are doing include choria, that would also have to be first

[teluq-pbrideau]

There is no include, but explicit definition of the classes. If there was another include before the definition, puppet would complain of having the class defined multiple times.

  class { 'choria':
    manage_mcollective => false,
  }
  class { 'mcollective':
    client => $client,
    server => $server,
    plugin_classes => [
      'mcollective_choria',
      'mcollective_util_actionpolicy',
      'choria',
    ],
  }

  $_puppet_policies_admins = $admins.map |$a| {
    {
      action  => 'allow',
      callers => "choria=${a}",
      actions => 'status runonce enable disable',
      facts   => '*',
      classes => '*',
    }
  }
  class { 'mcollective_agent_puppet':
    policies => $_puppet_policies_admins,
  }

[ripienaar]

this should correctly lookup the $mcollective:... vars then

[teluq-pbrideau]

That’s what I also thought, but obviously it does not

[smortex]

Regarding the compilation failure

Copy/pasting your first example does not cause compilation error, so the root cause should be in your control repo.

Regarding the default values in both init.pp and Hiera

This is generally not good and we (voxpupuli, opuscodium) try to avoid it since puppet-strings will get the default value form init.pp when generating the documentation and the shown parameters default values are not the "real" ones.

In this case, I think some parameters shall never be changed by the user (lists of files and directories) so I see 3 ways it can be made better:

1. Remove the default values from init.pp;
2. Move the default values from Hiera to init.pp;
3. Remove the parameters, and replace them with variables initialized with explicit lookup() to get the value from the current Hiera data.

2 is the cleanest, but then each module will need a custom init.pp file. I personally do not see it as major issue, but it is beyond the scope of this issue, so maybe we should discuss it in a dedicated issue if this is something we want to improve.

Regarding setting policies in profiles rather that control-repo Hiera data (what this PR is about)

Yes please :-) This is a pain point I have not taken the time to tackle yet.

[teluq-pbrideau]

Sorry for the delay, I was on another project for the last two weeks.

Regarding the compilation failure

I’m coming back to test, I’ve destroyed and rebuilt my vagrant environment and everything run smoothly now. I’m not sure what was the problem, but you were right @smortex, the problem was somewhere in my control repo or maybe on my puppetmaster.

Regarding the default values

You are right, this is out of scope of this issue

Regarding setting policies in profiles rather that control-repo Hiera data (what this PR is about)

Now that my solution works, I am able to configure my policies within my own profile without using hiera, so this issue is no longer relevant to me.

@smortex Feel free to re-open this issue if you think it is still relevant to you.