Is it bad form to do additional things inside the expect block like this? If so, does the assertion for no error seem necessary or should I just check the return value (since the test will fail/error if the function errors anyway)?

Hmm, I don't know enough about rspec expectation best practise, but if it works and it's more explicit to test both parts (the result being nil and the error being raised) that seems fine to me?

Rather than not raise exception I'd say a better name would be

allowmissing => true,

There maybe many other reasons to raise an exception which are not just nokey.

moreover I don't think in this case a warning should be made, the function has done exactly what the user asked for.

As currently written, this will catch and squash all exceptions, not just nokey. This is to prevent a Puppet run from terminating if something goes wrong with the vault lookup (whether or not there is a value stored for the key). For example, a bad Vault URL or malformed JSON response normally cause an exception, which if the param is true would instead just log an error and allow the run to continue. This allows users to conditionally rely on their vault values if they want.

Are you proposing a completely different model as more useful? It kind of sounds like you would like it to only squash the missing key exception, and still error out in all other cases.

I think failing the compilation is always the correct thing to do if you can't get a sensible result. e.g a value of key or return some other default otherwise.

Supporting the specification of a default may make sense.

$value = puppet::lookup('mykey', 'defaultvalue')

but as for cases where vault URL is wrong it should just fail always and not I would say be an option to override that.

I think this is kinda dangerous.

The most common case for vault_lookup is credentials and certificates.

If you have a fallback it's going to either be. a credential in code or a private cert in code.

Default values here are I'd say bad and head back in the direction that using vault is trying to get away from in most orgs.

I used to have a local patch that avoided the failure, if requested. Using it requires testing the returned value for undef-ness and skipping the rest of the class / resource. This was useful in certain corner cases. When we updated to the current upstream code, we no longer had a use for it and didn't bother to patch it back in. So I'm ambivalent about it. It's definitely a sharp edge you can cut yourself on, but then what in puppet isn't? And padded rooms aren't the UNIX way.

I think it's worse than that.. with deferred you can't just exclude the resource because the catalogue is already compiled/resolved.

Perhaps I'm missing something but I don't see how this is dangerous? If the client can't contact the vault, no secret is retrieved and thus can't be exposed no matter what the puppet code tries to do.

In my current environment we have an alternate way to deploy the vault config to the client securely via puppet, but are unable to do so since it fails the catalog outright. This PR would solve that problem for us and allow us to get the vault config onto the client via puppet, then succeed at applying the secrets from the vault on the next puppet run.

I agree with @cgaspar-deshaw even if you could burn yourself with this, it's still a useful feature and there's no substitute for knowing what you're doing and accepting the consequences.