-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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: Adds GCP secret manager triggerauthentication #4864
feat: Adds GCP secret manager triggerauthentication #4864
Conversation
Signed-off-by: neelanjan00 <[email protected]>
Signed-off-by: neelanjan00 <[email protected]>
Thank you for your contribution! 🙏 We will review your PR as soon as possible.
While you are waiting, make sure to:
Learn more about: |
Signed-off-by: neelanjan00 <[email protected]>
…eelanjan00/keda into gcp-secret-manager-triggerauth
Signed-off-by: neelanjan00 <[email protected]>
Would you mind adding some e2e tests? |
Sure, is there any guide to how to get started on this? |
You can read the doc here: https://github.com/kedacore/keda/blob/main/tests/README.md and see examples in tests/secret-provider. Maybe you need to update E2E Test infrastructure to add GCP secret manager also. |
Yeah, please add an e2e test for this feature (which is awesome BTW). |
Hi Jorge, I will be very glad if you can explain the overall e2e test flow. Upon checking the E2E test infrastructure repo, I found Terraform scripts which I presume will provision the requisite resources needed to perform the test. But at which phase of the test are they invoked? Also, I am new to Terraform so it will be helpful if someone can list the steps needed to be followed to use The GCP Secret Manager. |
Let's go step by step xD
Yeah, that's it. We define all the testing infrastructure using that repository with helm.
They are executed on merge on that repo. It means that the resources are always available. For the moment, we don't create and delete them because it can increase significantly the execution time. We scale out/in the clusters during the e2e tests, but that's all.
As this is a gcp resource, you should create a module inside GCP folder or maybe using one like this: https://registry.terraform.io/modules/GoogleCloudPlatform/secret-manager/google/latest. Other option is instead of already provisioning the secrets in terraform, you could do it as part of the tests, as we do for HashiCorp Vault test case: https://github.com/kedacore/keda/blob/main/tests/secret-providers/hashicorp_vault/hashicorp_vault_test.go After that, you have to set all the secrets that you need for your e2e tests, adding them here: https://github.com/kedacore/testing-infrastructure/blob/main/terraform/main.tf#L242
You already have it explained here: https://github.com/kedacore/keda/blob/main/tests/README.md
Each e2e test has to setup all the local deps that it needs, and it has to remove them as clean up As I said, if you have any specific question or you need help with something, you can ping us. If you prefer, I could try to do the terraform changes this week to spin up the service in GCP and set the secrets in the repo |
Thanks for the detailed explanation Jorge, it clears up all my doubts. Let me take a stab at it. |
Signed-off-by: neelanjan00 <[email protected]>
Signed-off-by: neelanjan00 <[email protected]>
Signed-off-by: neelanjan00 <[email protected]>
…tion newImage fields Signed-off-by: neelanjan00 <[email protected]>
90dc34f
to
fab71a5
Compare
E2E test logs for GCP Secret Manager TriggerAuth
|
Signed-off-by: neelanjan00 <[email protected]>
/run-e2e gcp_secret_manager |
good catch! |
Signed-off-by: neelanjan00 <[email protected]>
Signed-off-by: neelanjan00 <[email protected]>
Thank you so much for the detailed review @wozniakjan, I have added all the requested changes. The conflict has been resolved as well. |
/run-e2e secret-provider |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, @neelanjan00 the only outstanding nit is the rename to credentials
, don't forget to update docs PR as well. Thanks!
Signed-off-by: neelanjan00 <[email protected]>
I have resolved the comment, @zroubalik @wozniakjan thanks for the review. However, the unit test is failing as the |
@neelanjan00 could you please fix static checks?
|
Signed-off-by: neelanjan00 <[email protected]>
Fixed, PTAL. |
/run-e2e secret-provider |
Hi @zroubalik / @wozniakjan can you please help with the failing unit tests? |
sure, I will take a look today :) |
indeed a flaky test but it will get better over time - #5372. either you can rebase on top of the latest main or maybe @zroubalik can you please retrigger the unit tests? |
/run-e2e secret-provider |
@tomkerkhove @wozniakjan @zroubalik |
@neelanjan00 given there is no need for a PR to helm chart, can you remove please that item from PR description?
it's holding up the |
now it's just a matter of someone from @kedacore/keda-core-contributors to approve, they will do that momentarily :) |
oh, @neelanjan00, looks like there is a merge conflict again, can you please rebase/merge latest main? |
Signed-off-by: Zbynek Roubalik <[email protected]>
I fixed it. |
A description of what has been changed
Checklist
I have verified that my change is according to the deprecations & breaking changes policy
Tests have been added
Changelog has been updated and is aligned with our changelog requirements
A PR is opened to update the documentation on (repo) (if applicable)
Commits are signed with Developer Certificate of Origin (DCO - learn more)
Fixes #4831
Relates to kedacore/keda-docs#1203