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

Provide support for using Prometheus scaler with Azure Monitor managed service for Prometheus (preview) #4153

Closed
tomkerkhove opened this issue Jan 23, 2023 · 15 comments
Assignees
Labels
azure All issues concerning integration with Azure feature All issues for new features that have been committed to needs-discussion

Comments

@tomkerkhove
Copy link
Member

tomkerkhove commented Jan 23, 2023

Proposal

Provide support for using Prometheus scaler with Azure Monitor managed service for Prometheus (preview) which relies on Azure AD authentication.

Use-Case

Customers using Azure Monitor managed service for Prometheus (preview) can scale their applications based on existing metrics.

Is this a feature you are interested in implementing yourself?

No

Anything else?

This requires the addition of Azure AD authentication, I'll dig in to the requirements for this.

@tomkerkhove tomkerkhove added needs-discussion azure All issues concerning integration with Azure feature All issues for new features that have been committed to labels Jan 23, 2023
@raggupta-ms
Copy link
Contributor

Thankyou @tomkerkhove for opening this issue. I am starting to look into this support and would need some time to go through the details and come up with a plan. Will share the proppsal/idea for further discussion/brainstorming once I am there!

@tomkerkhove
Copy link
Member Author

Great, thank you @raggupta-ms!

@raggupta-ms
Copy link
Contributor

raggupta-ms commented Feb 8, 2023

I spent some time on KEDA code and this is what I am thinking:

@tomkerkhove, please share your comments on the above thought. Also, please add/tag other folks who can help reviewing.

@tomkerkhove
Copy link
Member Author

  • Add support for Azure Workload Identity in the first iteration i.e. hoping to get released in March's release.
  • Add support for Azure AD App Auth iteratively after the first version is shipped.

This is OK for me, if we agree on it with other maintainers then we should track a new issue for it.

I am on the fence on this one since we do support it for other Azure scalers and implementation should be minimal. Pod Identity is deprecated, but not archived yet so I'm leaning toward adding it. @JorTurFer @zroubalik?

  • As a sample of ScaledObject and TriggerAuthtication, it will be very similar to how it looks today for Prometheus scaler and a sample is:
    image

Correct, this should work as an end outcome.

This is already a pre-requisite so a non-action for you.

  • For the implementation of workload identity support:
    -- Use/enhance existing Azure workload identity code to get the token in required format (main/pkg/scalers/azure/azure_aad_workload_identity.go)
    -- Add a new http round tripper for Azure auth that will utilize the code in above mentioned file azure_aad_workload_identity.go.
    -- The round tripper will ensure to inject the workload identity token in authorization header.
    -- Use this new Azure round tripper in existing Prometheus scaler i.e. if the auth in scaler config is set to azure workload identity (
    PodIdentity kedav1alpha1.AuthPodIdentity

    ), use this azure round tripper instead of the one used currently in code (main/pkg/scalers/prometheus_scaler.go#L94).
    -- Might need to change some code here and there such as auth config parsing in Prometheus scaler etc.

@JorTurFer @zroubalik can you validate this, Sounds legit to me though.

-- Overall, doesn't seem to be a lot of changes.

I agree, changes should be easy.

@JorTurFer
Copy link
Member

JorTurFer commented Feb 8, 2023

  • Add support for Azure Workload Identity in the first iteration i.e. hoping to get released in March's release.
  • Add support for Azure AD App Auth iteratively after the first version is shipped.

This is OK for me, if we agree on it with other maintainers then we should track a new issue for it.

I have no idea about what is Azure AD App auth, but it looks good to me

I am on the fence on this one since we do support it for other Azure scalers and implementation should be minimal. Pod Identity is deprecated, but not archived yet so I'm leaning toward adding it. @JorTurFer @zroubalik?

Agree with @tomkerkhove , if AAD-Pod-Identity is still alive (its EOL is December if I'm not wrong), we should support both. The best part is that we already have the code to get the token (like for AAD-WI), so the extra required code should be low

About the implementation, sound good in my mind, but I need to see the code to have a real opinion about it. From my side, you can go ahead.

Remember that e2e test is also required as part of the feature. You will need to do another PR to the testing infra repository, adding the needed terraform to deploy the managed prometheus and performing the role assignment required to grant permissions to the e2e managed identity

@raggupta-ms
Copy link
Contributor

Thankyou @tomkerkhove and @JorTurFer for reviweing. I didn't have strong opinion on not adding support for Pod Identity. Since you feel it should be there (I also agree somewhat that it is going to stick in for probably around an year more), I will add that as well.

Will proceed to make code changes at my end. Noted the testing part, I may have more questions, especially around testing when I reach there. Thanks for the support!

@tomkerkhove
Copy link
Member Author

I have no idea about what is Azure AD App auth, but it looks good to me

@JorTurFer This is just Azure AD authentication with a client app

@raggupta-ms
Copy link
Contributor

@tomkerkhove @JorTurFer I am facing an issue while trying to setup my VSCode for debugging and could use your help, in case you know.

I am following the steps listed here: https://github.com/kedacore/keda/blob/main/BUILD.md#operator but when I launch VS Code, I get following error:

{"level":"error","ts":"2023-02-12T00:28:16-08:00","logger":"setup","msg":"problem running manager","error":"open /certs/ca.crt: no such file or directory","stacktrace":"main.main\n\t/home/raggupta/code/keda/cmd/operator/main.go:284\nruntime.main\n\t/usr/local/go/src/runtime/proc.go:250"}

Looks like the operator is trying to get hold of ca and tls certs. Is this expected when debugging locally? Is there a way to disable tls on local setup? I am using minikube cluster. I could find some annotation related to tls cert in keda-metrics-apiserver yaml file and removed all of it, annotated with --kubelet-insecure-tls and --kubelet-preferred-address-types="InternalIP", but very likely I am looking at wrong place since I want to debug operator but these config is for api server.

I tried to supply a ca.crt at the mentioned location but then it asked for tls.crt etc. and I could not resolve that completely - am no expert on tls and certs :(

Any idea what I might be doing wrong or to get it working? Note that when I deploy by scaling keda operator to 0 and then make deploy, it works fine - I don't see any cert issues in logs and keda containers are up and running fine. Just that VS Code launch isn't working.

@JorTurFer
Copy link
Member

JorTurFer commented Feb 12, 2023

Oh! Sorry, this has been my fault :(
I'll update the docs asap, in the meantime, you can get the certificates that the operator has created in the cluster (it's a secret in KEDA namespace, you will find them together inside the secret) and create the files in that folder
In the cluster it works because the secret is automatically mounted into the pod

@JorTurFer
Copy link
Member

Hi @raggupta-ms ,
I have just opened a PR with the information you need. It sin't merged yet, but you can check the process directly there.

Thanks for notifying the documentation gap :)

@raggupta-ms
Copy link
Contributor

@JorTurFer thankyou for the instructions. It did the resolve the issue :)

@JorTurFer
Copy link
Member

no worries, it was missing due to my fault xD

@raggupta-ms
Copy link
Contributor

created a PR: #4256
@tomkerkhove @JorTurFer Please take a look.

@tomkerkhove
Copy link
Member Author

Docs and feature are in, we are good to go!

@tomkerkhove
Copy link
Member Author

Thank you @raggupta-ms !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
azure All issues concerning integration with Azure feature All issues for new features that have been committed to needs-discussion
Projects
Archived in project
Development

No branches or pull requests

3 participants