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

pd: cors headers not being served (regression) #3865

Closed
conorsch opened this issue Feb 22, 2024 · 2 comments · Fixed by #3870
Closed

pd: cors headers not being served (regression) #3865

conorsch opened this issue Feb 22, 2024 · 2 comments · Fixed by #3870
Assignees
Labels
A-CI/CD Relates to continuous integration & deployment of Penumbra A-node Area: System design and implementation for node software

Comments

@conorsch
Copy link
Contributor

Describe the bug
Back in #3281 we modified pd to serve permissive cors headers directly. Some time in the intervening period that behavior was lost. First reported by @grod220 yesterday.

To Reproduce
Steps to reproduce the behavior:

  1. Run pd locally on your machine.
  2. Run curl -I http://localhost:8080

Expected behavior
Displayed headers should include:

access-control-allow-headers: *
access-control-allow-methods: *
access-control-allow-origin: *

Additional context
We can temporarily patch the hosting config to re-apply these headers. That's what I've done as a stopgap to unblock the web team. Those ad-hoc changes will be dropped on the next CI deploy, though, so we should try to fix the problem in pd.

@conorsch conorsch added A-node Area: System design and implementation for node software A-CI/CD Relates to continuous integration & deployment of Penumbra labels Feb 22, 2024
@conorsch
Copy link
Contributor Author

It's worth pointing out that we overlooked this regression because we didn't bother to wire up tests in CI to confirm the behavior. We should add a simple header check via the integration tests, which talk to pd directly, to make sure the headers are preserved during refactors.

@conorsch
Copy link
Contributor Author

Git bisect points to e7bcdba as the first bad commit, which makes sense: it was a substantial refactor. See PR #3652 for context. I'm going to pair with @cratelyn briefly today to try to restore the original functionality; if we don't achieve that in the timebox, then we'll stick with the stopgap solution of applying headers at the ingress layer to accommodate web team needs.

conorsch added a commit that referenced this issue Feb 22, 2024
This change restores the serving of CORS headers in HTTP responses by pd.
During the refactor of auto-https logic in #3652, we overlooked
that the tower [CorsLayer](https://docs.rs/tower-http/0.4.4/tower_http/cors/struct.CorsLayer.html#)
was no longer being carried through after type conversions between
tower, tonic, and axum. Simply moving the layer attachment
to the already-built axum router, rather than the previously
constructed tonic router, resolves the problem.

We now include an integration test to check for the headers,
so we don't inadvertently drop them again.

Closes #3865.
@conorsch conorsch self-assigned this Feb 22, 2024
@cratelyn cratelyn self-assigned this Feb 23, 2024
conorsch added a commit that referenced this issue Feb 23, 2024
This change restores the serving of CORS headers in HTTP responses by pd.
During the refactor of auto-https logic in #3652, we overlooked
that the tower [CorsLayer](https://docs.rs/tower-http/0.4.4/tower_http/cors/struct.CorsLayer.html#)
was no longer being carried through after type conversions between
tower, tonic, and axum. Simply moving the layer attachment
to the already-built axum router, rather than the previously
constructed tonic router, resolves the problem.

We now include an integration test to check for the headers,
so we don't inadvertently drop them again.

Closes #3865.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-CI/CD Relates to continuous integration & deployment of Penumbra A-node Area: System design and implementation for node software
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants