tonic_web::enable() is convenient but doesn't let us set a custom CORS configuration. as a result, the server currently rejects request headers "code_version" and "device_type" from web. therefore, i'm modifying our Server builder to take a cors layer and a GrcpWebLayer. it's slightly more verbose, but we need this to send request metadata over grpc-web.
Details
Details
- Reviewers
bartek michal • jon ashoat - Commits
- rCOMM62a378159314: [identity] pull out CORS logic
confirmed the metadata could be parsed when i sent a GenerateNonce request to my local identity service from web
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- grpc-web (branched from master)
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
New dependencies look good. Rust code looks simple, but will leave it for somebody else to review
Comment Actions
Nice to see the magic string constants removed!
| services/identity/src/constants.rs | ||
|---|---|---|
| 153–157 ↗ | (On Diff #32604) | I find it a bit cleaner to use hierarchy for cases like this but it's fine either way. |
| services/identity/src/cors.rs | ||
| 9 ↗ | (On Diff #32604) | Is allow_methods not needed? |
| 10 ↗ | (On Diff #32604) | In other web services (services/comm-services-lib/src/http.rs::cors_config) we currently set:
We might want to do that here too (at least the production setup). |
| 16 ↗ | (On Diff #32604) | Why are these cloned() needed? |
Comment Actions
address @michal's feedback
| services/identity/src/cors.rs | ||
|---|---|---|
| 9 ↗ | (On Diff #32604) | i think simple methods are allowed by default. that's why requests succeeded in testing. also i took the CorsLayer from here |
| 16 ↗ | (On Diff #32604) | The cloned() method takes each &&'static str yielded by the original iterator (the one created with iter()) and clones the &'static str, effectively turning the iterator into one that yields &'static str. |