-
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(scaler/prometheus): support authentication for Google Managed Prometheus #4675
feat(scaler/prometheus): support authentication for Google Managed Prometheus #4675
Conversation
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: Claudio Netto <nettinhorama@gmail.com>
Signed-off-by: Claudio Netto <nettinhorama@gmail.com>
Signed-off-by: Claudio Netto <nettinhorama@gmail.com>
Signed-off-by: Claudio Netto <nettinhorama@gmail.com>
b1387ca
to
d856d49
Compare
Signed-off-by: Claudio Netto <nettinhorama@gmail.com>
d4f74cd
to
bf5e83b
Compare
Signed-off-by: Claudio Netto <nettinhorama@gmail.com>
Signed-off-by: Claudio Netto <nettinhorama@gmail.com>
Signed-off-by: Claudio Netto <nettinhorama@gmail.com>
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! ❤️
Could you add an e2e test too? You can add any required service/permission using the testing-infrastructure repository. We manage the infrestructure using terraform 😄
Sure, I do. Although I think I'll need an extra hand to configure it correctly. 😬 Firstly, can you clarify some doubts about the infrastructure?
|
Sure, you can ask whatever you need 😄 (we have a Slack channel in Kubernetes workspace for development stuff if you prefer that channel to be faster during the development)
We prefer to run all the test in AKS if it's possible, otherwise we'd need to manage multiple clusters in different platforms for main and also for PRs (there are 2 different clusters). If we need to export metrics, we could use opentelemetry collector to export the metrics to there using the googlemanagedprometheusexporter but I don't think that's necessary. I mean, as this test won't test the prometheus scaler behavior itself but the authentication, we could just use an scalar query in the ScaledObject and just update it updating the scaled object. We are already doing it for some tests: https://github.com/kedacore/keda/blob/main/tests/scalers/azure/azure_log_analytics/azure_log_analytics_test.go#L119
The federation between KEDA and GCP is already done. KEDA can access to GCP resources using workload identity federation (for example, this). I think that for this tests you need to:
As I said, the federation is already done , the mutating webhook is already there, and everything should be working for just adding the extra required info for the new tests |
Got it... thank you so much, Jorge.
I absolutely agree. Whaterever rather than unauthenticated status code should be enough in this test case. So gimme a bunch of minutes trying to create it. |
Sure, let me know if I can help with something 😄 |
Signed-off-by: Claudio Netto <nettinhorama@gmail.com>
tests/scalers/gcp/gcp_prometheus_workload_identity/gcp_prometheus_workload_identity_test.go
Outdated
Show resolved
Hide resolved
Signed-off-by: Claudio Netto <nettinhorama@gmail.com>
/run-e2e gcp |
This PR adds Google's authentication in order to access metrics from Google Managed Prometheus.
Checklist
Fixes #4674
Relates to kedacore/keda-docs#1153