Allow the user to prevent the lookup from raising an exception #12
Conversation
| it 'returns nil instead of raising when raising is disabled' do | ||
| expect { | ||
| result = function.execute('/v1/whatever', 'vault.docker', false) | ||
| expect(result).to be(nil) |
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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?
Magisus
force-pushed
the
optional-raise
branch
from
f63b04f
to
6197236
Compare
|
@Magisus I just merged my PR so this will need rebasing, sorry! I think this might be useful, esp for bootstrapping |
|
Cool, I'll update. |
Magisus
force-pushed
the
optional-raise
branch
from
6197236
to
308e7a3
Compare
|
Rebased. |
|
Hey, thanks for the contribution! Can you please rebase? That will enable the acceptance tests. |
This reverts commit 5e5ed91, putting back the option to prevent exceptions from being raised.
Magisus
force-pushed
the
optional-raise
branch
from
308e7a3
to
9f7b6db
Compare
|
Done. |
| end | ||
|
|
||
| def lookup(path, vault_url = nil) | ||
| def lookup(path, vault_url = nil, raise_exceptions = true) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
moreover I don't think in this case a warning should be made, the function has done exactly what the user asked for.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think it's worse than that.. with deferred you can't just exclude the resource because the catalogue is already compiled/resolved.
There was a problem hiding this comment.
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.
|
Hello. Is this still planned for merge and release? |
|
Dear @Magisus, thanks for the PR! This is Vox Pupuli Tasks, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase? You can find my sourcecode at voxpupuli/vox-pupuli-tasks |
If a lookup function raises an exception, the whole catalog compilation will fail. This PR adds the ability to configure the lookup to not raise and instead simply log and return nil if it encounters an error, so that the users can structure their manifests to only conditionally depend on the result of the lookup.
We're not sure if this is useful or not, and are looking for use cases and feedback on the approach.
Fixes #13