feat: add CORS support for HTTP API and metrics endpoints#756
feat: add CORS support for HTTP API and metrics endpoints#756zemse wants to merge 4 commits intosigp:unstablefrom
Conversation
jxs
left a comment
jxs
left a comment
There was a problem hiding this comment.
Hi, thanks for getting into this!
Left some comments
anchor/http_metrics/src/lib.rs
Outdated
| 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.
see comment above, this seems un-required, we should be able to receive allow_origin as AllowOrigin to not parsed it again here
anchor/http_api/src/lib.rs
Outdated
| 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.
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.
- Parse CLI args directly into AllowOrigin in from_cli - Remove duplicate parsing in http_api and http_metrics - Replace hyper with tower-http dependency in client crate Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
dknopik
left a comment
dknopik
left a comment
There was a problem hiding this comment.
This bumps all dependencies, so probably cargo update was run. Usually we do those in separate PRs - please restore the Cargo.lock from unstable
| listen_addr: IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1)), | ||
| listen_port: 5062, | ||
| allow_origin: None, | ||
| allow_origin: AllowOrigin::any(), |
There was a problem hiding this comment.
I am not sure if any is a good default.
E.g. Lighthouse defaults to the listen address and port: https://github.com/sigp/lighthouse/blob/f3fd1f210b2f4ed7d208f81f9a09e1edced3bb3d/beacon_node/http_api/src/lib.rs#L340-L344
There was a problem hiding this comment.
yeah makes sense thanks Daniel.
I suggested any based on following the default behavior of None but it makes more sense to use the listen address and port as lighthouse does
4 participants
Issue Addressed
Closes #249
Proposed Changes
--metrics-allow-originCLI flag--http-allow-originflag existed but wasn't wired up.Additional Info
None