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

fix: Added support for vault kv-v1 (#2645) #2772

Closed

Conversation

chaunceyt
Copy link

@chaunceyt chaunceyt commented Mar 17, 2022

Signed-off-by: Chauncey Thorn chaunceyt@gmail.com

Adding support for HashiCorp Key-Value v1 secrets backend. Also fixed a typo in log output.

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO - learn more)
  • Tests have been added
  • A PR is opened to update our Helm chart (repo) (if applicable, ie. when deployment manifests are modified)
  • A PR is opened to update the documentation on (repo) (if applicable)
  • Changelog has been updated and is aligned with our changelog requirements

Fixes #2645

Signed-off-by: Chauncey Thorn <chaunceyt@gmail.com>
@chaunceyt chaunceyt requested a review from a team as a code owner March 17, 2022 22:29
Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

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

LGTM!
Thanks for the fix and the research you have done ❤️
Could you update the changelog?

Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

Thanks, do you think you can add a simple unit test for this?

@chaunceyt
Copy link
Author

I'll work to get the changelog update and unit test in. Hopefully I'll be able to get it done this weekend.

@zroubalik I was looking for the test for the v2 secret engine. Can you point me to that so I can extend that test?

Thanks

@zroubalik
Copy link
Member

zroubalik commented Mar 18, 2022

I'll work to get the changelog update and unit test in. Hopefully I'll be able to get it done this weekend.

@zroubalik I was looking for the test for the v2 secret engine. Can you point me to that so I can extend that test?

Thanks

Unfortunately there isn't any specific test for this :(

Signed-off-by: Chauncey Thorn <chaunceyt@gmail.com>
@zroubalik
Copy link
Member

@chaunceyt we have added HashiCorp E2E tests, do you think you can extend them to cover kv-v1? #2849

@chaunceyt
Copy link
Author

@zroubalik Yes following the pattern in tests/scalers/postgresql-hashicorp-vault.test.ts I can extend to cover kv-v1. This pattern seems less work that trying to add it to scale_resolvers_test.go

Thanks for pointing me to this.

@JorTurFer
Copy link
Member

JorTurFer commented Apr 1, 2022

one thing @chaunceyt
Please take into account that if you create another test file, they can run in parallel sometimes. I said because I had errors installing 2 instances of vault in due to Clusterroles, maybe the parameters need to be updated in any way

@zroubalik
Copy link
Member

@chaunceyt any update on thia please? We are going to release 2.7.0 soon (May 7). So would be great if we can include this as well.

@chaunceyt
Copy link
Author

@zroubalik
Currently on vacation, will return beginning of the month and will work to get the test written and submitted. Sorry for the long delay been really busy.

@zroubalik
Copy link
Member

@chaunceyt just FYI we plan to do the release May 5

@tomkerkhove
Copy link
Member

@chaunceyt Any thought if we can close this PR soon? We are planning to release in 2 weeks.

@zroubalik
Copy link
Member

@chaunceyt ping :)

@JorTurFer
Copy link
Member

You will need to rebase your PR too

@chaunceyt
Copy link
Author

Hi, sorry for lack of response and delay getting back to this. I am going to close the PR. I accidentally noticed the lack of v1 support using incorrect path for v2 setup. After fixing that the Vault integration works as desired without the change.

I'm going to speculate that very few if anyone is using v1 for Vault these days.

Thanks for the patience shown due to my lack of response.
Keep up the good work.

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.

Using Hashicorp Vault secrets with TriggerAuthentication results in unable to convert Vault Data value error
4 participants