Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow the user to prevent the lookup from raising an exception #12

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Allow the user to prevent the lookup from raising an exception #12

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c2e5d8c
Revert "Remove option for not raising exceptions for now."
Magisus Nov 28, 2018
9f7b6db
Test the ability to disable raising exceptions
Magisus Nov 29, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions lib/puppet/functions/vault_lookup/lookup.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,21 @@
dispatch :lookup do
param 'String', :path
optional_param 'String', :vault_url
optional_param 'Boolean', :raise_exceptions
end

def lookup(path, vault_url = nil)
def lookup(path, vault_url = nil, raise_exceptions = true)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

_lookup(path, vault_url)
rescue StandardError => e
raise if raise_exceptions

Puppet.err(e.message)
nil
end

private

def _lookup(path, vault_url)
if vault_url.nil?
Puppet.debug 'No Vault address was set on function, defaulting to value from VAULT_ADDR env value'
vault_url = ENV['VAULT_ADDR']
Expand Down Expand Up @@ -38,8 +50,6 @@ def lookup(path, vault_url = nil)
Puppet::Pops::Types::PSensitiveType::Sensitive.new(data)
end

private

def get_auth_token(connection)
response = connection.post('/v1/auth/cert/login', '')
unless response.is_a?(Net::HTTPOK)
Expand Down
7 changes: 7 additions & 0 deletions spec/functions/lookup_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,13 @@
}.to raise_error(Puppet::Error, %r{No vault_url given and VAULT_ADDR env variable not set})
end

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)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}.not_to raise_error
end

it 'raises a Puppet error when auth fails' do
connection = instance_double('Puppet::Network::HTTP::Connection', address: 'vault.doesnotexist')
expect(Puppet::Network::HttpPool).to receive(:http_instance).and_return(connection)
Expand Down