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

Prometheus Scaler Add custom headers and custom auth support #4208

Merged
merged 27 commits into from
Feb 24, 2023

Conversation

prashant-shahi
Copy link
Contributor

@prashant-shahi prashant-shahi commented Feb 7, 2023

Checklist

Relates to #4206

Relates to kedacore/keda-docs#1064

…heus

Signed-off-by: Prashant Shahi <me@prashantshahi.dev>
Signed-off-by: Prashant Shahi <me@prashantshahi.dev>
Signed-off-by: Prashant Shahi <me@prashantshahi.dev>
Signed-off-by: Prashant Shahi <me@prashantshahi.dev>
@prashant-shahi prashant-shahi requested a review from a team as a code owner February 7, 2023 11:51
Signed-off-by: Prashant Shahi <me@prashantshahi.dev>
Signed-off-by: Prashant Shahi <me@prashantshahi.dev>
Signed-off-by: Prashant Shahi <me@prashantshahi.dev>
Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution! Looking good but I have one question:
Will these headers contain sensitive data? I mean, in the examples you have used "token". If these headers contain sensitive data, they should be read from TriggerAuthentication instead of metadata. We should avoid sensitive data as part of trigger metadata.
From the code pov, you should look for them config.AuthParams instead of config.TriggerMetadata (or maybe from both, joining them if there are headers from both places)

CHANGELOG.md Outdated Show resolved Hide resolved
Signed-off-by: Prashant Shahi <me@prashantshahi.dev>
Signed-off-by: Prashant Shahi <me@prashantshahi.dev>
Signed-off-by: Prashant Shahi <me@prashantshahi.dev>
Signed-off-by: Prashant Shahi <me@prashantshahi.dev>
@prashant-shahi
Copy link
Contributor Author

@JorTurFer yes, that makes sense for custom authentication header.

I have included changes related to the same and updated linked documentation PR as well.

@prashant-shahi prashant-shahi changed the title **Prometheus Scaler**: Add custom headers support **Prometheus Scaler**: Add custom headers and custom auth header support Feb 7, 2023
@prashant-shahi prashant-shahi changed the title **Prometheus Scaler**: Add custom headers and custom auth header support **Prometheus Scaler**: Add custom headers and custom auth support Feb 7, 2023
Signed-off-by: Prashant Shahi <me@prashantshahi.dev>
Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for the improvement :)
wdyt @zroubalik ?

@JorTurFer
Copy link
Member

JorTurFer commented Feb 7, 2023

/run-e2e prometheus*
Update: You can check the progress here

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.

Generally looking good, what will happen if user specify both cortexOrgID option and a custom header where he specify X-Scope-OrgID header for Cortex but with a different value. Which one will be used and valid?

@prashant-shahi
Copy link
Contributor Author

prashant-shahi commented Feb 8, 2023

Generally looking good, what will happen if user specify both cortexOrgID option and a custom header where he specify X-Scope-OrgID header for Cortex but with a different value. Which one will be used and valid?

@zroubalik Currently, the value from custom header will be used over cortexOrgID, when both are specified. However, I do not think one would be more valid than the other when both are passed.

Ideally, cortexOrgID can be depreciated after this PR, since the same can be fulfilled using custom headers.
We can mention in documentation to pass cortexOrgID using X-Scope-OrgID custom header.
But yeah, it can co-exist along with custom headers if breaking changes are not desired for now.

@tomkerkhove tomkerkhove changed the title **Prometheus Scaler**: Add custom headers and custom auth support Prometheus Scaler Add custom headers and custom auth support Feb 16, 2023
@prashant-shahi
Copy link
Contributor Author

prashant-shahi commented Feb 16, 2023

@zroubalik @tomkerkhove Do let me know if there are any changes required for making the PR good to merge.

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.

@prashant-shahi the PR is looking good, I agree that we should deprecate the cortex parameter. Could you please add log message here if this parameter is specified, that custom headers should be used instead?

@zroubalik
Copy link
Member

@prashant-shahi please also add note to Deprecated section in Changelog about this change.

@prashant-shahi
Copy link
Contributor Author

@zroubalik cortexOrgID parameter has been deprecated and relevant files are updated.

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, great job @prashant-shahi !

@tomkerkhove is there anything else needed to be done wrt deprecation? Could you please double check? 🙇‍♂️

@zroubalik
Copy link
Member

zroubalik commented Feb 23, 2023

/run-e2e prometheus*
Update: You can check the progress here

Signed-off-by: Prashant Shahi <me@prashantshahi.dev>
Signed-off-by: Prashant Shahi <me@prashantshahi.dev>
Signed-off-by: Prashant Shahi <me@prashantshahi.dev>
@prashant-shahi
Copy link
Contributor Author

@zroubalik Thank you! :)

I have also included checks for duplicate keys in custom headers as well as given higher precedence to authentication headers in case of collision with custom header.

@zroubalik
Copy link
Member

zroubalik commented Feb 23, 2023

/run-e2e prometheus*
Update: You can check the progress here

@tomkerkhove
Copy link
Member

LGTM, great job @prashant-shahi !

@tomkerkhove is there anything else needed to be done wrt deprecation? Could you please double check? 🙇‍♂️

We're not there yet, we need to:

  • Create GH discussion
  • Create issue
  • Document it with clear timelines

But we can do that after merging.

https://github.com/kedacore/governance/blob/main/DEPRECATIONS.md#introducing-new-deprecations

I also noticed doc entry is not in our policy today so I have opened kedacore/governance#95

@prashant-shahi
Copy link
Contributor Author

I also noticed doc entry is not in our policy today so I have opened kedacore/governance#95

That's great! I was not sure about format for deprecation, so I went along with what I had in mind.

@tomkerkhove
Copy link
Member

No worries!

@tomkerkhove tomkerkhove merged commit 38a0e1c into kedacore:main Feb 24, 2023
@prashant-shahi prashant-shahi deleted the feat/prometheus-headers branch February 24, 2023 18:30
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