Jira (PUP-11451) Make collecting exported resources optional

17 views
Skip to first unread message

Nacho Barrientos (Jira)

unread,
Feb 4, 2022, 9:09:02 AM2/4/22
to puppe...@googlegroups.com
Nacho Barrientos created an issue
 
Puppet / New Feature PUP-11451
Make collecting exported resources optional
Issue Type: New Feature New Feature
Assignee: Unassigned
Created: 2022/02/04 6:08 AM
Priority: Normal Normal
Reporter: Nacho Barrientos

According to the documentation, storing catalogs and facts in PuppetDB and using exported resources are two actions that are governed by the same configuration option (storeconfigs):

Whether to store each client's configuration, including catalogs, facts, and related data. This also enables the import and export of resources in the Puppet language - a mechanism for exchange resources between nodes.

https://puppet.com/docs/puppet/7/configuration.html#storeconfigs

It'd be useful for us in our deployment if those two things were not tied to each other. In other words, if it was possible to keep "storing configs" but exported resource collectors were ignored. This is rather cheap to achieve by introducing a new configuration option and patching the exported resources collector, something like:

@@ -18,7 +18,7 @@ class Puppet::Pops::Evaluator::Collectors::ExportedCollector < Puppet::Pops::Eva
   # Ensures that storeconfigs is present before calling AbstractCollector's
   # evaluate method
   def evaluate
-    if Puppet[:storeconfigs] != true
+    if Puppet[:storeconfigs] != true || Puppet[:collect_exported_resources] == false
       return false
     end

https://github.com/puppet/puppet/blob/389dbf644582560c704304a7e7e7b0801efc2c83/lib/puppet/pops/evaluator/collectors/exported_collector.rb#L21

Would you be happy to review and eventually merge a patch adding this new configuration option with default value true and, as consequence, to modify the semantics of storeconfigs?

Add Comment Add Comment
 
This message was sent by Atlassian Jira (v8.20.2#820002-sha1:829506d)
Atlassian logo

Nirupama Mantha (Jira)

unread,
Feb 8, 2022, 4:21:01 PM2/8/22
to puppe...@googlegroups.com

Charlie Sharpsteen (Jira)

unread,
Mar 1, 2022, 7:08:03 PM3/1/22
to puppe...@googlegroups.com

This does seem like a simple settings addition + modification of the resource collector behavior.

But, I wonder if adding a setting that causes exported resources to be silently ignored is really the right solution because it would be confusing to debug if someone missed the release note. If exported resource collections are disallowed for some reason, it seems better to remove their use from the Puppet codebase and use lint checks to prohibit any re-introduction.

Ben Ford (Jira)

unread,
Mar 1, 2022, 7:26:03 PM3/1/22
to puppe...@googlegroups.com
Ben Ford commented on New Feature PUP-11451

Likewise, I'm curious to hear about the use case.

My concern with this would be the situation where someone enabled this setting maybe not knowing what it did, or forgetting about it later, or maybe not communicating that setting to others on the team, etc. And then later on, a user of that same infrastructure installed a module that used exported resources, say puppetlabs-haproxy or the like, and everything appeared to work fine but it just didn't do anything. That could be immensely frustrating and confusing, and difficult to debug since querying for the exported resources would show them in the database as expected.

Asking for community help probably wouldn't even be terribly productive since this setting would be such a change.

Nacho Barrientos (Jira)

unread,
Mar 16, 2022, 3:13:02 AM3/16/22
to puppe...@googlegroups.com

Hi Charlie, Ben,

Thanks for your input.

Our use case is a multi tenant configuration management infrastructure based on Puppet where somebody could accidentally or maliciously export a resource to be collected by somebody else that could lead to broken or malicious configuration being applied. In our case, the usage of exported resources can normally easily be replaced by PuppetDB queries so we'd like to disable resource collection to reduce the risks.

Replying to your comment Charlie, unfortunately we as admins don't have full control on the code that our masters compile and our users are not enforced (only encouraged) to implement lint checks.

Ben, I understand your concerns but I don't think bad communication from the admins justify not having the functionality. In my opinion, as long as the defaults are not modified and the setting well documented, it should be okay. We cannot influence how configuration changes on the masters are communicated everywhere smile.png

Anyway, thanks for your input and please let us know at your earliest convenience if you'd like us to contribute a change request. Otherwise, we'll most likely carry a local patch to disable exported resources collection.

Thanks again.

/cc traylenator

Charlie Sharpsteen (Jira)

unread,
Mar 17, 2022, 3:31:02 PM3/17/22
to puppe...@googlegroups.com

After taking a second look at this, I think the goal can already be accomplished without introducing a new setting. Exported resource collection is implemented via the Indirector subsystem using the Resource terminus. Configuration of this subsystem can be specified via the routes.yaml file.

So, disabling exported resources is just a matter of creating a new "none" terminus for Resource that returns an empty array for search operations:

# cat /opt/puppetlabs/puppet/lib/ruby/vendor_ruby/puppet/indirector/resource/none.rb
require 'puppet/indirector/none'
 
# A none terminus type, meant to always return nil
class Puppet::Resource::None < Puppet::Indirector::None
  def find(request)
    return nil
  end
 
  def search(request)
    return Array.new
  end
end

And then configuring that in routes.yaml along with other PuppetDB settings:

# cat /etc/puppetlabs/puppet/routes.yaml
master:
      facts:
        terminus: puppetdb
        cache: yaml
      resource:
        terminus: none

Nacho Barrientos (Jira)

unread,
Mar 24, 2022, 9:02:03 AM3/24/22
to puppe...@googlegroups.com

Hi Charlie,

That's great. As long as this does not prevent catalogs and resources from being stored in PuppetDB it could definitely fly for us. Would you be willing to distribute puppet/indirector/resource/none.rb as part of Puppet?

Nacho Barrientos (Jira)

unread,
Mar 28, 2022, 11:44:03 AM3/28/22
to puppe...@googlegroups.com

Thinking twice for us it'd be even better to stop the compilation than just collecting "nothing".

Something like this seems to work:

require 'puppet/indirector/none'
 
class Puppet::Resource::Fail < Puppet::Indirector::None
  def find(request)
    raise Puppet::Indirector::ValidationError, _("Collecting exported resources is disabled.")
  end
 
  def search(request)
    raise Puppet::Indirector::ValidationError, _("Collecting exported resources is disabled.")
  end
end

which would result in something like:

Info: Using environment 'qa'
Info: Retrieving pluginfacts
Info: Retrieving plugin
Info: Loading facts
Error: Could not retrieve catalog from remote server: Error 500 on SERVER: Server Error: Collecting exported resources is disabled. on node node.example.org.
Warning: Not using cache on failed catalog
Error: Could not retrieve catalog; skipping run

Reply all
Reply to author
Forward
0 new messages