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

Add Bearer Auth for Metrics API scaler #2028

Merged
merged 5 commits into from
Aug 18, 2021
Merged

Conversation

JorTurFer
Copy link
Member

@JorTurFer JorTurFer commented Aug 11, 2021

Adds an authorization method named bearer for Metrics API scaler

Checklist

Relates to #1081

Signed-off-by: jorturfer <jorge_turrado@hotmail.es>
Signed-off-by: jorturfer <jorge_turrado@hotmail.es>
Signed-off-by: jorturfer <jorge_turrado@hotmail.es>
@JorTurFer
Copy link
Member Author

I'm not sure if it's better to create a new AuthType or reuse the existing BearerAuthType

Maybe bearer is a specific case of jwt but ... 🤔
any opinion @zroubalik @ahmelsayed ?

@ahmelsayed
Copy link
Contributor

JWT would be a specific case, or format, of a bearer token, right? I'm assuming the value of jwtToken is expected to be Bearer <jwt> in this case, correct?

@tomkerkhove
Copy link
Member

JWT would be a specific case, or format, of a bearer token, right? I'm assuming the value of jwtToken is expected to be Bearer <jwt> in this case, correct?

I'd say that the JWT would just be the JWT token and we add the bearer for you, no?

@JorTurFer
Copy link
Member Author

I'm not an expert here, but I think that JWT is more like the format and Bearer is one use case of the format, but as I said, I'm not an expert :(
I will try to get some help from an expert

@tomkerkhove
Copy link
Member

I'd say that the JWT would just be the JWT token and we add the bearer for you, no?

This remark is purely from a user-perspective and making it simple/straight-forward - Not saying this is how JWT works

@JorTurFer
Copy link
Member Author

JorTurFer commented Aug 16, 2021

Maybe the auth method should be named as Bearer instead JWT, we can simplify the process to the final user just placing the token, and we automatically add the Authorization header with the value Bearer {given token}
This could be clearer than naming it JWT auth for a Bearer auth.
WDTY?

@zroubalik
Copy link
Member

Maybe the auth method should be named as Bearer instead JWT, we can simplify the process to the final user just placing the token, and we automatically add the Authorization header with the value Bearer {given token}
This could be clearer than naming it JWT auth for a Bearer auth.
WDTY?

I like this approach.

Signed-off-by: jorturfer <jorge_turrado@hotmail.es>
@JorTurFer JorTurFer changed the title Add JWT auth for Metrics API scaler Add Bearer Auth for Metrics API scaler Aug 17, 2021
@JorTurFer
Copy link
Member Author

I have just changed it @zroubalik :)
I think that it's ready for review

@zroubalik zroubalik added this to the v2.5.0 milestone Aug 18, 2021
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 :)

@ahmelsayed
Copy link
Contributor

@JorTurFer one merge conflict in CHANGELOG.md

@JorTurFer
Copy link
Member Author

JorTurFer commented Aug 18, 2021

@JorTurFer one merge conflict in CHANGELOG.md
Hi @ahmelsayed !
I see... I had another merge conflict in the same file with the other PR that I have and I have just fixed it. I will update this but when one of them is merged, the other will need changes again :(

Signed-off-by: jorturfer <jorge_turrado@hotmail.es>
@ahmelsayed ahmelsayed merged commit d0b8983 into kedacore:main Aug 18, 2021
@JorTurFer JorTurFer deleted the jwt_support branch August 27, 2021 17:32
nilayasiktoprak pushed a commit to nilayasiktoprak/keda that referenced this pull request Oct 23, 2021
* Add support to jwt token in metrics api scaler

Signed-off-by: jorturfer <jorge_turrado@hotmail.es>

* Add test

Signed-off-by: jorturfer <jorge_turrado@hotmail.es>

* Update changelog

Signed-off-by: jorturfer <jorge_turrado@hotmail.es>

* Update auth method and add test

Signed-off-by: jorturfer <jorge_turrado@hotmail.es>
Signed-off-by: nilayasiktoprak <nilayasiktoprak@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.

4 participants