Move bulk of logic to PuppetX::VaultLookup::Lookup

[natemccurdy]

Problem

The vault_lookup::lookup() function can not be stubbed/mocked in an rspec-puppet test when the function is deferred with Deferred().

For example, say you're writing a spec test for a defined type that calls vault_lookup::lookup() with Deferred. If you run your tests, you'll see that:

• The function attempts to connect to real Vault servers
• The function does file system checks for things like the token file, that won't exist.

e.g.

Failure/Error: it { is_expected.to compile }
  error during compilation: The agent_sink_file does not exist or is not readable: /etc/vault-agent/token

The problem is that the vault_lookup::lookup() function isn't actually being deferred in rspec-puppet. It's being evaluated because of this change: rodjek/rspec-puppet#740

Also, stubbing the function as a Puppet-language function doesn't work either. For example:

# In an rspec-puppet test for a class or defined type
# NOTE: This stub works only when vault_lookup::lookup() is NOT deferred.
let(:pre_condition) do
  'function vault_lookup::lookup($path, $opts) { Sensitive("hello world") }'
end

it { is_expected.to compile }

You'll see that rspec fails with:

Failure/Error: it { is_expected.to compile }

NoMethodError:
  undefined method `with_global_scope' for {}:Hash

Solution

Move all of vault_lookup::lookup()'s code into a PuppetX namespaced module so that there's a Ruby class whose methods can be stubbed using regular allow(<class>).to receive(<method>).and_return(<value>) patterns.

In this case, the internals of the function are moving to:

PuppetX::VaultLookup::Lookup

Outcome

The vault_lookup::lookup() function retains the exact same behavior as before when used in Puppet code.

The function's evaluation and return value and be stubbed in an rspec-puppet test like this:

before(:each) do
  allow(PuppetX::VaultLookup::Lookup).to receive(:lookup)
    .and_return(Puppet::Pops::Types::PSensitiveType::Sensitive.new('hello world'))
end

[joshcooper]

if I remember you correctly, types/providers/facts/report processors should always use require_relative to load other files, to ensure environment isolation?

Yes require_relative should always be used to load helper code, because when running in puppetserver context, the module's environment is not added to the ruby $LOAD_PATH otherwise code in one environment would be visible to another.

If I'm correct we should really work on a custom rubocop rule to detect this

Agree, that would be awesome.

The problem is that the vault_lookup::lookup() function isn't actually being deferred in rspec-puppet. It's being evaluated because of this change: rodjek/rspec-puppet#740

We added a setting in puppet 7 so it's possible to evaluate deferred functions lazily when the resource is evaluated. And that is the default behavior in puppet 8 (see https://tickets.puppetlabs.com/browse/PUP-9323) My expectation is if you set Puppet[:preprocess_deferred] = false then the deferred function won't be evaluated during an rspec-puppet run, specifically when Puppet::Resource::Catalog.to_ral is called. So I think maybe we should focus on making the module/tests work when deferred functions aren't eagerly evaluated, instead of trying to work around the eager evaluation.

Also, you can't stub in the function as a Puppet-language function because Puppet-language functions can't be deferred.

I'm not sure I understand this part. I would expect rspec-puppet to be able to stub Puppet-language functions, even those that are deferred. It may be that rspec-puppet hasn't pushed the loaders onto the context so or something like that?

[binford2k]

@joshcooper from slack conversation, when stubbing a Puppet language function that ends up being deferred, this exception is raised:

NoMethodError:
        undefined method `with_global_scope' for {}:Hash

Are you suggesting that something like this might make those function deferrable in this context? rodjek/rspec-puppet#825

[natemccurdy]

So I think maybe we should focus on making this module work when deferred functions aren't eagerly evaluated, instead of trying to work around the eager evaluation.

@joshcooper That sounds ideal, but I'm not sure I understand specifically what you mean. Do you have an example? Also, wouldn't we still want to allow this function's evaluation to be stubbed for cases where eager evaluation is used, and to make spec testing scenarios easier?

Stubbing as a puppet-language function

Also, you can't stub in the function as a Puppet-language function because Puppet-language functions can't be deferred.

I'm not sure I understand this part. I would expect rspec-puppet to be able to stub Puppet-language functions, even those that are deferred. It may be that rspec-puppet hasn't pushed the loaders onto the context so or something like that?

The only way that I'm aware of to stub a Ruby-API function in an rspec-puppet test is by defining a Puppet-language function of the same name.

For example, given this Puppet code that does NOT use deferred:

file { '/etc/creds.txt':
  ensure  => file,
  content => vault_lookup::lookup($path, $opts),
}

✅ This rspec-puppet test works, and the stub function is used instead of the real function:

# In an rspec-puppet test for a class or defined type
# NOTE: This stub works only when vault_lookup::lookup() is NOT deferred.
let(:pre_condition) do
  'function vault_lookup::lookup($path, $opts) { Sensitive("hello world") }'
end

it { is_expected.to compile }
it { is_expected.to contain_file('/etc/creds.txt').with_content(sensitive('hello world')) }

But when the Puppet code is updated to use Deferred, for example:

file { '/etc/creds.txt':
  ensure  => file,
  content => Deferred('vault_lookup::lookup', [$path, $opts]),
}

❌ The rspec-puppet test fails with:

Failure/Error: it { is_expected.to compile }

NoMethodError:
  undefined method `with_global_scope' for {}:Hash

Failure/Error: is_expected.to contain_file('/etc/credts.txtt').with_content(sensitive('hello world'))

NoMethodError:
  undefined method `with_global_scope' for {}:Hash

Stubbing as a ruby-api function

If I try to redefine the function as a Ruby-API function, the real function is used instead (though I admit I'm not convinced that this would be expected to work at all).

So given this deferred function call in Puppet:

file { '/etc/creds.txt':
  ensure  => file,
  content => Deferred('vault_lookup::lookup', [$path, $opts]),
}

And this rspec-puppet test:

# This does NOT work. The real function is called instead of this one.
before(:each) do
  Puppet::Functions.create_function(:'vault_lookup::lookup') do
    def lookup(path,opts)
       Puppet::Pops::Types::PSensitiveType::Sensitive.new('hello world')
    end
  end
end

it { is_expected.to compile }

❌ The rspec-puppet tests fail with errors related to the fact that the real function is being evaluated. In this case, it bombs out on the token file not existing locally when attempting to use auth_method => 'agent_sink':

Failure/Error: it { is_expected.to compile }
  error during compilation: The agent_sink_file does not exist or is not readable: /etc/vault-agent/token

[natemccurdy]

@joshcooper (or anyone else) One quirk with this change is that spec tests for Puppet code need to add the following require_relative line before they're able to stub the PuppetX::VaultLookup::Lookup class's methods:

require_relative '../fixtures/modules/vault_lookup/lib/puppet_x/vault_lookup/lookup'

# ...snip...

before(:each) do
  allow(PuppetX::VaultLookup::Lookup).to receive(:lookup)
    .and_return(Puppet::Pops::Types::PSensitiveType::Sensitive.new('hello world'))
end

Is that expected?

It feels wrong to be to have to require_relative into the fixtures directory. Maybe I'm missing something?

Without that line, the spec tests fail with:

NameError:
  uninitialized constant PuppetX

[natemccurdy]

Thanks @bastelfreak
FYI, I'm running a few more tests on this change internally to make sure there aren't any more issues.

I'll merge this when those tests are complete and I'm confident in this change.