-
Notifications
You must be signed in to change notification settings - Fork 28
feat: add CORS support for HTTP API and metrics endpoints #756
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
base: unstable
Are you sure you want to change the base?
Conversation
jxs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, thanks for getting into this!
Left some comments
| let origin = allow_origin | ||
| .map(|o| AllowOrigin::exact(o.parse().expect("validated in config"))) | ||
| .unwrap_or(AllowOrigin::any()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comment above, this seems un-required, we should be able to receive allow_origin as AllowOrigin to not parsed it again here
| if let Some(allow_origin) = &cli_args.metrics_allow_origin { | ||
| // Pre-validate the config value to give feedback to the user on node startup. | ||
| hyper::header::HeaderValue::from_str(allow_origin) | ||
| .map_err(|_| "Invalid metrics-allow-origin value")?; | ||
|
|
||
| config.http_metrics.allow_origin = Some(allow_origin.to_string()); | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should be able to parse cli_args.allow_origin and cli_args.metrics.allow_origin as tower_http::cors::AllowOrigin to store both as AllowOrigin in the config and not have the need to parse them again later
| let origin = config | ||
| .allow_origin | ||
| .map(|o| AllowOrigin::exact(o.parse().expect("validated in config"))) | ||
| .unwrap_or(AllowOrigin::any()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comment above, this seems unrequired, we can have config.allow_origin's type to be AllowOrigin to not have to be parsed again here.
Issue Addressed
Closes #249
Proposed Changes
--metrics-allow-originCLI flag--http-allow-originflag existed but wasn't wired up.Additional Info
None