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.

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

v1 candidate #36

Closed
wants to merge 10 commits into from
Closed

Conversation

traviscosgrave
Copy link
Contributor

This PR is closer to a request for comment.

I was unable on my host to get the specs running, and as such did not add any (although the approach should allow all of the functionality to exist. Also, these changes require the changes I added to the hiera-vault gem to support real LIST requests hashicorp/vault-ruby#201.

The function has been re-written such that it behaves more like the other standard hiera back ends, running the function once per uri passed in. It takes advantage of the global vault client, but reconfigures it before each lookup to ensure environment caching doesn't cause any problems. Configuration is cached for each path, and the keys found at each path and any values returned are cache to reduce the calls to vault and provide as static a view on the secret set as possible.

There is a construct for adding other backends, and supporting interleaved backends is now possible as the configuration for the mount and the list of paths are handled separately.

A pattern for handling environment specific field values (eg for testing a new version of a secret before rolling it out infra wide) also has been developed with the environment_delimiter option.

caching vault object for entire run and closing sockets when done
adding support for deeply nested object stored in Vault
allow hiera to keep looking if value is not found
This change moves from a one-time backend to a URI based
hiera backend wherein the lookup_key function will be called for
each configured URI.

This change moves the paths settings out of the options hash to
the URIs array and uses the mounts options to control what kind of
secret backend is being used. It has been tested against kv2, and
assumed to work with kv1, structures are in place for other path based
backends if needed.

This change requires a modified version of the vault ruby gem which
correctly calls LIST method endpoints to help reduce the number of calls
being made to vault during the lookup process.

This change introduces the `environment_delimeter` option which will
allow the fields at a secret path to be selected based on the current
environment, bare fields will be selected by default. This feature
behaves as expected when default field is used.

This change will require updating the configuration to use uris
instead of putting the paths in the options block.
@traviscosgrave traviscosgrave mentioned this pull request Jun 7, 2019
@petems
Copy link
Owner

petems commented Jun 19, 2019

@traviscosgrave This looks great! The main issue is that I dont really want to merge this until the features are added into the core gem...

Let me check on the status of that and see if I can get that merged

@traviscosgrave
Copy link
Contributor Author

Of course! We're running both forks locally, and I'm not sure my patch to the core gem is the best way to solve the problem of the HTTP client on supporting the LIST method. Gotta keep moving though.

Also, this is now up-to-date with our current implementation. I ran into some issues with cached environments and JRubies either garbage collecting or nodes in a weird state (migrating from one cluster to another) hitting the error and the global $vault being blown away. The most recent change removes the forced nil on the $vault client and will recreate one if it get's GC'd. Thanks for following up

@fduranti
Copy link

Any news on this?
The hiera fallback is something that I'll really like to have :)

@traviscosgrave
Copy link
Contributor Author

I'm still battling the error mentioned above, I hope to have some kind of solution this week and I'll update the PR accordingly.

This change resolves a race condition, especially with cached environments,
where the client connection pool will shutdown during a requst resulting
in a catalog compilation failure by using the built in retry
functionality of the vault-gem.

This change also moves to use the `Vault::logical.list` method over
requiring the `LIST` HTTP method as part of PR 201 on the vault gem.
The workaround resulted from a bug in the `vault agent` request
forwarding whereby query string parameteres failed to be forwarded.
Using the latest `vault agent` allows the typical method to be used.
@traviscosgrave
Copy link
Contributor Author

Using the retry feature of the vault client I was able to resolve the race condition whereby a pool is shutting down mid-request especially in cached environments. I've also eliminated the need for the weird support of the LIST http method by instead upgrading the local vault agent to the latest 1.1.3 (in which a bug preventing query string parameters from being proxied was fixed).

This is now looking good in our environment. Looking forward to the comments.

Asside: I did attempt to use the built in Puppet::HttpPool, but it required some nasty tricks and was about 50% slower on catalogs with many lookups even after forcing it to reuse connections.

@fduranti
Copy link

@traviscosgrave You still need hashicorp/vault-ruby#201 before merging this ?

@traviscosgrave
Copy link
Contributor Author

hashicorp/vault-ruby#201 is only required if you plan to use vault agent before version 1.1.3. It shouldn't be required if you talk directly from your puppet server/agent to vault. (Although that case is currently outside our scope due to a strict vault cipher set that is unsupported by the jruby net:http implementeation). [The current code is what we have have running now.]

@petems
Copy link
Owner

petems commented Sep 11, 2019

@traviscosgrave Can you resolve the conflict then I can merge? 👍

@traviscosgrave
Copy link
Contributor Author

I'll see what I can do, there's a lot that I moved around.

@petems
Copy link
Owner

petems commented Sep 25, 2019

I've merged #41 in the mean time, which doesnt seem to require changes to the gem, can you test this works for you now?

@AndreasDilworth
Copy link

AndreasDilworth commented Oct 3, 2019

I tried #41- and it is broken for v2 stores (and actually doesn't work well for v1 either).
This PR works very well (really good job, @traviscosgrave!) and I do hope this gets merged! As mentioned, it does not require hashicorp/vault-ruby#201 as long as an updated Vault-agent is used.

I just have two nitpicks:

  • The continue_if_not_found option should be documented and/or should be true by default as Hiera will otherwise stop making further lookups if no value is found on first URI - which might not be expected behaviour.
  • The environment_delimiter option could be made optional as some might prefer separating environments by URI instead.

@petems petems closed this Dec 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants