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

Support all secret types #63

Merged
merged 5 commits into from
Feb 12, 2021
Merged

Support all secret types #63

merged 5 commits into from
Feb 12, 2021

Conversation

tomhjp
Copy link
Contributor

@tomhjp tomhjp commented Feb 9, 2021

Adds support for all secret engines via updated config options in SecretProviderClass e.g.:

    objects: |
      - objectName: "certs"
        secretPath: "pki/issue/example-dot-com"
        secretKey: "certificate"
        secretArgs:
          common_name: "test.example.com"
          ttl: "24h"
        method: "PUT"

These config updates make it a breaking change for existing SecretProviderClasses, and is one of two breaking changes that are planned for the next release - the other being that we will auth to Vault as the requesting pod. Things should then be more stable after that.

As part of the updated config/support for all secret engines, this PR:

  • Adds a cache that lasts for the lifetime of one mount request (i.e. one whole SecretProviderClass) to ensure repeated dynamic secret paths are not read multiple times and the username/password match
  • Supports dumping the whole JSON response to disk instead of a specific secretKey

Other small additions:

  • gRPC interceptor logging so errors always show up in the provider logs, giving one convenient location to debug failures for a specific pod.
  • Use hashicorp.jfrog.io/docker images where possible
  • Upgrade vault-helm chart to v0.9.1 to pull in dev mode fix

You may want to review the provider.bats diff with whitespace off, as I corrected some inconsistent indentation there.

@tomhjp tomhjp added this to the 0.0.8 milestone Feb 9, 2021
if err != nil {
return "", err
func generateRequest(client *api.Client, secret config.Secret) (*api.Request, error) {
secretPath := ensureV1Prefix(secret.SecretPath)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if splitting on / would be better and checking what the first value is. Then we can prepend the API version cleanly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had a little play with this idea but personally found the logic both harder to follow and make robust, but perhaps I don't fully understand your idea. If you think this could simplify it, do you have a little code sample?

@jasonodonnell jasonodonnell self-requested a review February 10, 2021 15:26
Copy link
Contributor

@jasonodonnell jasonodonnell left a comment

Choose a reason for hiding this comment

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

Couple small things but looks great to me!

@tomhjp
Copy link
Contributor Author

tomhjp commented Feb 12, 2021

Thanks for the review - as mentioned I'm deferring addressing a few of these comments into the next PR as it's already open and based on this branch.

@tomhjp tomhjp merged commit a49aa8a into master Feb 12, 2021
@tomhjp tomhjp deleted the support-all-secret-types branch February 12, 2021 16:03
@tomhjp tomhjp mentioned this pull request Mar 24, 2021
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.

2 participants