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

ClusterTriggerAuthentication to support headers so that accountId can be passed for NewRelic Scaler via authenticationRef #2883

Closed
pragmaticivan opened this issue Apr 5, 2022 · 6 comments · Fixed by #2898
Labels
feature-request All issues for new features that have not been committed to needs-discussion

Comments

@pragmaticivan
Copy link
Contributor

Proposal

apiVersion: keda.sh/v1alpha1
kind: ClusterTriggerAuthentication
metadata:
  name: newrelic
spec:
  secretTargetRef:
  - parameter: account
    name: keda-newrelic-creds
    key: account-number
  - parameter: queryKey
    name: keda-newrelic-creds
    key: api-key

Use-Case

Have multiple New Relic accounts and want to be able to set, apiKey, account number and region in the cluster authentication so developers don't need to care about that.

This would also help with NewRelic promql, which requires an api-key in different ways.

Anything else?

No response

@pragmaticivan pragmaticivan added feature-request All issues for new features that have not been committed to needs-discussion labels Apr 5, 2022
@tomkerkhove tomkerkhove moved this to Proposed in Roadmap - KEDA Core Apr 5, 2022
@pragmaticivan
Copy link
Contributor Author

account is currently required by the CRD, so I'm not sure if that currently works.

@pragmaticivan
Copy link
Contributor Author

Perhaps it should be optional when not using trigger authentication.

@JorTurFer
Copy link
Member

hey @pragmaticivan
Sorry for the slow response...
It sounds totally reasonable. In fact, I have checked the code and it's not documented in keda.sh but I'd say that the region can be passed by TriggerAuthentication (based on the code).
Are you open to contribute with this?

@pragmaticivan
Copy link
Contributor Author

@JorTurFer It can easily be passed by using the function GetFromAuthOrMeta instead of the existing code. The region seems to be already handling that correctly.

@JorTurFer
Copy link
Member

Yes, you should use that function instead of current code (like it's already done in region and key) and that's all.
Adding one test case and updating the docs are the only other task to do

pragmaticivan added a commit to pragmaticivan/keda that referenced this issue Apr 11, 2022
pragmaticivan added a commit to pragmaticivan/keda that referenced this issue Apr 11, 2022
fixes kedacore#2883

Signed-off-by: Ivan Santos <pragmaticivan@gmail.com>
pragmaticivan added a commit to pragmaticivan/keda that referenced this issue Apr 11, 2022
fixes kedacore#2883

Signed-off-by: Ivan Santos <pragmaticivan@gmail.com>
pragmaticivan added a commit to pragmaticivan/keda that referenced this issue Apr 11, 2022
fixes kedacore#2883

Signed-off-by: Ivan Santos <pragmaticivan@gmail.com>
Signed-off-by: Ivan Santos <301291+pragmaticivan@users.noreply.github.com>
@pragmaticivan
Copy link
Contributor Author

Done!
#2898
kedacore/keda-docs#744

@JorTurFer JorTurFer moved this from Proposed to In Review in Roadmap - KEDA Core Apr 12, 2022
Repository owner moved this from In Review to Ready To Ship in Roadmap - KEDA Core Apr 12, 2022
@tomkerkhove tomkerkhove moved this from Ready To Ship to Done in Roadmap - KEDA Core Aug 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request All issues for new features that have not been committed to needs-discussion
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants