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.

Sign up for GitHub

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

Jump to bottom

Add Origin header for Query API #5388

Merged
merged 17 commits into from
Jun 12, 2023
Merged

Add Origin header for Query API #5388

merged 17 commits into from
Jun 12, 2023

Conversation

cathy-qiu
Copy link
Contributor

@cathy-qiu cathy-qiu commented Jun 5, 2023 •
edited
Loading

What this PR does:
This PR adds the Access-Control-Allow-Origin header to the response of Query API. The functionality replicates how Prometheus sets CORS here. This is needed because I am trying to create a tool that calls a Cortex API from the browser and I am getting a CORS header error.

Which issue(s) this PR fixes:
Fixes #

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@friedrichg friedrichg self-requested a review June 6, 2023 08:24
friedrichg
friedrichg requested changes Jun 6, 2023
View reviewed changes
Copy link
Member

@friedrichg friedrichg 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 your contribution. I am ok with making the value configurable. Just mention that is only valid for the ruler for now.

Also:

  1. Sign DCO
  2. Include tests
  3. Modify changelog

pkg/ruler/api.go Outdated

w.Header().Set("Content-Type", "application/json")
if disableCORS != true {
w.Header().Set("Access-Control-Allow-Origin", "*")
Copy link
Member

Choose a reason for hiding this comment

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

why not make the value configurable? I think other things different than star are useful too
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Origin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, good idea 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

This is how Prometheus does this https://github.com/prometheus/prometheus/blob/main/util/httputil/cors.go#L30

@cathy-qiu
Copy link
Contributor Author

Thanks for your contribution. I am ok with making the value configurable. Just mention that is only valid for the ruler for now.

Also:

  1. Sign DCO
  2. Include tests
  3. Modify changelog

If we want to implement this for Query API, can I adapt an http.Handler to an http.HandlerFunc since the handler type is different from the other APIs? Example

@yeya24
Copy link
Contributor

yeya24 commented Jun 6, 2023

@cathy-qiu Can you try to use a common wrapper instead of changing every method?
https://github.com/prometheus/prometheus/blob/main/web/api/v1/api.go#L305 This is how Query API adds it

@pull-request-size pull-request-size bot added size/L and removed size/XL labels Jun 6, 2023
@cathy-qiu cathy-qiu force-pushed the enable-cors-ruler-api branch from 9cacf35 to 6f85afb Compare June 6, 2023 23:14
@cathy-qiu cathy-qiu changed the title Add Origin header for Ruler API Add Origin header for Query API Jun 6, 2023
@cathy-qiu cathy-qiu requested review from friedrichg and yeya24 June 6, 2023 23:28
friedrichg
friedrichg reviewed Jun 7, 2023
View reviewed changes
Copy link
Member

@friedrichg friedrichg left a comment

Choose a reason for hiding this comment

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

almost there, fix the lint issues too

pkg/api/api.go Outdated Show resolved Hide resolved
Cathy Qiu added 7 commits June 7, 2023 12:01
Set CORS header for ruler API
5477a2c 
Signed-off-by: Cathy Qiu <cathyqiu@amazon.com>
Set CORS headers
1089e05 
Signed-off-by: Cathy Qiu <cathyqiu@amazon.com>
disableCORS config variable added to add headers
b491425 
Signed-off-by: Cathy Qiu <cathyqiu@amazon.com>
set cors for query api
26752ff 
Signed-off-by: Cathy Qiu <cathyqiu@amazon.com>
CHANGELOG.md updated
4f85e92 
Signed-off-by: Cathy Qiu <cathyqiu@amazon.com>
change config variable flag
2911b06 
Signed-off-by: Cathy Qiu <cathyqiu@amazon.com>
lint issues fixed
1f9484a 
Signed-off-by: Cathy Qiu <cathyqiu@amazon.com>
@cathy-qiu cathy-qiu force-pushed the enable-cors-ruler-api branch from ed97b94 to a099c5a Compare June 7, 2023 19:02
@pull-request-size pull-request-size bot added size/XL and removed size/L labels Jun 7, 2023
@cathy-qiu cathy-qiu force-pushed the enable-cors-ruler-api branch from a099c5a to 1f9484a Compare June 7, 2023 19:10
@pull-request-size pull-request-size bot added size/L and removed size/XL labels Jun 7, 2023
fixed lint test
fce0ee3 
Signed-off-by: Cathy Qiu <cathyqiu@amazon.com>
yeya24
yeya24 reviewed Jun 7, 2023
View reviewed changes
pkg/util/httputil/cors.go Outdated Show resolved Hide resolved
use promethueus import
2366727 
Signed-off-by: Cathy Qiu <cathyqiu@amazon.com>
@pull-request-size pull-request-size bot added size/M and removed size/L labels Jun 7, 2023
Cathy Qiu added 2 commits June 7, 2023 15:46
added cors_origin flag to config
05523f9 
Signed-off-by: Cathy Qiu <cathyqiu@amazon.com>
cli flag added
d0dda32 
Signed-off-by: Cathy Qiu <cathyqiu@amazon.com>
@cathy-qiu cathy-qiu requested review from friedrichg and yeya24 June 8, 2023 00:08
Cathy Qiu added 3 commits June 8, 2023 10:12
changed cors regex default string
f40154b 
Signed-off-by: Cathy Qiu <cathyqiu@amazon.com>
fix lint check
e598474 
Signed-off-by: Cathy Qiu <cathyqiu@amazon.com>
cors default value changed
4213c87 
Signed-off-by: Cathy Qiu <cathyqiu@amazon.com>
yeya24
yeya24 reviewed Jun 8, 2023
View reviewed changes
pkg/api/api.go Outdated Show resolved Hide resolved
pkg/api/api.go Outdated Show resolved Hide resolved
pkg/api/api.go Show resolved Hide resolved
moved regex compilation functionality
05ccbd5 
Signed-off-by: Cathy Qiu <cathyqiu@amazon.com>
@cathy-qiu cathy-qiu requested a review from yeya24 June 9, 2023 21:37
yeya24
yeya24 reviewed Jun 9, 2023
View reviewed changes
pkg/api/api.go Outdated Show resolved Hide resolved
yeya24
yeya24 reviewed Jun 9, 2023
View reviewed changes
pkg/api/api.go Outdated Show resolved Hide resolved
flag description changed
58c8a06 
Signed-off-by: Cathy Qiu <cathyqiu@amazon.com>
yeya24
yeya24 approved these changes Jun 9, 2023
View reviewed changes
Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

Thanks!

ran make doc
04008ea 
Signed-off-by: Cathy Qiu <cathyqiu@amazon.com>
friedrichg
friedrichg approved these changes Jun 12, 2023
View reviewed changes
Copy link
Member

@friedrichg friedrichg left a comment

Choose a reason for hiding this comment

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

Thank you 🙇

@friedrichg friedrichg merged commit b082d12 into cortexproject:master Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yeya24 yeya24 yeya24 approved these changes

@friedrichg friedrichg friedrichg approved these changes

Assignees
No one assigned
Labels
size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cathy-qiu @yeya24 @friedrichg