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

feat: allow vault secret to handle write operation #5068

Merged
merged 3 commits into from
Oct 13, 2023
Merged

feat: allow vault secret to handle write operation #5068

merged 3 commits into from
Oct 13, 2023

Conversation

loispostula
Copy link
Contributor

@loispostula loispostula commented Oct 10, 2023

Allow hashicorp vault secret to use write operation

Checklist

Fixes #5067

Relates to kedacore/charts#547 and kedacore/keda-docs#1242

@loispostula loispostula requested a review from a team as a code owner October 10, 2023 18:59
@github-actions
Copy link

Thank you for your contribution! 🙏 We will review your PR as soon as possible.

While you are waiting, make sure to:

Learn more about:

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.

Looking good!

@loispostula do you think you can add unit test for this? hashicorpvault_handler.go is missing test coverage, so we should work on improving this 🙏

@loispostula
Copy link
Contributor Author

@zroubalik I'm working on adding a e2e test as well, and while adding it I discover an issue, so I'll update my commit with all that

@loispostula
Copy link
Contributor Author

@zroubalik I've changed the approach for the PR.

I've added support for different vault secret backend. The default behaviour remains the same, if no type is probided, the secret is handled as either a v1 or v2 secret (to keep behaviour of #4152). This allows me to conduct specific pki related unmarshalling as not all keys are string for that one. This would allow to specified additional type in the future if needs be.

Regarding testing, I've move the secret fetching inside of the HashicorpVaultHandler struct, this allows me to make it easier to test the functionalities of it.

I've also made another notable change which is to bundle the call to the vault api by path. This is the default behaviour of consul-template and is especially useful in dynamic secret, as you only want to fetch the secret once and access the different keys

Signed-off-by: Loïs Postula <lois@postu.la>
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 a lot for the great contribution! I like the general direction, I love the tests! I have just a few nits.

apis/keda/v1alpha1/triggerauthentication_types.go Outdated Show resolved Hide resolved
pkg/scaling/resolver/hashicorpvault_handler_test.go Outdated Show resolved Hide resolved
Signed-off-by: Loïs Postula <lois@postu.la>
@zroubalik
Copy link
Member

zroubalik commented Oct 12, 2023

/run-e2e hashicorp
Update: You can check the progress here

@zroubalik
Copy link
Member

/run-e2e hashicorp Update: You can check the progress here

@loispostula what I can see in the test logs logs:

Execution of tests/secret-providers/hashicorp_vault/hashicorp_vault_test.go, has failed after "three" attempts
***0***3-***0-***T***3:36:05Z	ERROR	failed to ensure HPA is correctly created for ScaledObject	***"controller": "scaledobject", "controllerGroup": "keda.sh", "controllerKind": "ScaledObject", "ScaledObject": ***"name":"hashicorp-vault-test-so","namespace":"hashicorp-vault-test-ns"***, "namespace": "hashicorp-vault-test-ns", "name": "hashicorp-vault-test-so", "reconcileID": "30b589***a-88d0-4dfa-b***5d-df9adfd0dd86", "error": "error par sing prometheus metadata: no cert given"***

@loispostula
Copy link
Contributor Author

/run-e2e hashicorp Update: You can check the progress here

@loispostula what I can see in the test logs logs:

Execution of tests/secret-providers/hashicorp_vault/hashicorp_vault_test.go, has failed after "three" attempts
***0***3-***0-***T***3:36:05Z	ERROR	failed to ensure HPA is correctly created for ScaledObject	***"controller": "scaledobject", "controllerGroup": "keda.sh", "controllerKind": "ScaledObject", "ScaledObject": ***"name":"hashicorp-vault-test-so","namespace":"hashicorp-vault-test-ns"***, "namespace": "hashicorp-vault-test-ns", "name": "hashicorp-vault-test-so", "reconcileID": "30b589***a-88d0-4dfa-b***5d-df9adfd0dd86", "error": "error par sing prometheus metadata: no cert given"***

Thanks a lot, I've replicated locally, and I had not update the e2e test to use the camelCase, retriggering a local test then I'll push

Signed-off-by: Loïs Postula <lois@postu.la>
@zroubalik
Copy link
Member

zroubalik commented Oct 12, 2023

/run-e2e hashicorp
Update: You can check the progress here

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.

LGTM, great job! Thanks a lot for this great contribution. 🚀

Please just resolve the last few things on the docs PR and we can go ahead and merge this!

🙏

@loispostula
Copy link
Contributor Author

@zroubalik Thanks, doc pr updated :D Thanks for all the feedback

@zroubalik zroubalik merged commit 0462144 into kedacore:main Oct 13, 2023
19 checks passed
toniiiik pushed a commit to toniiiik/keda that referenced this pull request Jan 15, 2024
Signed-off-by: Loïs Postula <lois@postu.la>
Signed-off-by: anton.lysina <alysina@gmail.com>
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.

Hashicorp Vault: Support for writable secret
2 participants