ModuleDataProvider::validate_data_hash does not prune hash as advertised

[joshcooper]

Originally reported in https://puppet.atlassian.net/browse/PUP-11719

Describe the Bug

The function ModuleDataProvider::validate_data_hash is supposed to prune all hiera keys that are not prefixed with the configured module_name. It does not. Instead it returns the hash that it was called with unchanged. All pruning is done on a cloned version of the supplied hash that’s discarded.

https://github.com/puppetlabs/puppet/blob/main/lib/puppet/pops/lookup/module_data_provider.rb#L46

Desired Behavior:

It should prune all hiera keys that are not prefixed with the configured module name.

It should include the name of any offending key(s) in the warning message that it prints. The warning message as currently implemented doesn't provide the developer with any meaningful actionable information.

https://github.com/puppetlabs/puppet/blob/main/lib/puppet/pops/lookup/module_data_provider.rb#L54 should be modified to be something like:

Puppet.warning("Module '#{module_name}': #{msg}, key=#{k}") to help identify the offending key(s).

Actual Behavior:

Hiera keys not prefixed with the module name are not pruned from the supplied hash. No warning advising on the offending key(s) is printed.

Fix:

Delete this line and the pruned hash is returned:
https://github.com/puppetlabs/puppet/blob/main/lib/puppet/pops/lookup/module_data_provider.rb#L57

Environment

Puppet Version: 7.20
Puppet Server Version:
OS Name/Version:

Additional Context

Add any other context about the problem here.

[joshcooper]

The behavior can be seen with the following changes. Removing the clone step allows the data_hash to be mutated as the comments claim should be done:

diff --git a/lib/puppet/pops/lookup/module_data_provider.rb b/lib/puppet/pops/lookup/module_data_provider.rb
index bca3b3f224..443c1bfd94 100644
--- a/lib/puppet/pops/lookup/module_data_provider.rb
+++ b/lib/puppet/pops/lookup/module_data_provider.rb
@@ -51,7 +51,6 @@ class ModuleDataProvider < ConfiguredDataProvider
       next memo if k == LOOKUP_OPTIONS || k.start_with?(module_prefix)
 
       msg = "#{yield} must use keys qualified with the name of the module"
-      memo = memo.clone if memo.equal?(data_hash)
       memo.delete(k)
       Puppet.warning("Module '#{module_name}': #{msg}")
       memo

Oddly no tests fail with this change. But the following test does filter out the other value from the module's data hash:

diff --git a/spec/unit/pops/lookup/lookup_spec.rb b/spec/unit/pops/lookup/lookup_spec.rb
index 3e7b6de29b..69a55acc2d 100644
--- a/spec/unit/pops/lookup/lookup_spec.rb
+++ b/spec/unit/pops/lookup/lookup_spec.rb
@@ -96,6 +96,7 @@ describe 'The lookup API' do
         'data' => {
           'common.yaml' => <<-YAML.unindent
             mod::x: mod::x (from module)
+            other::y: should be removed
             YAML
         },
         'manifests' => {

[joshcooper]

Fixed in #9326