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

fix(pd): re-add cors headers #3870

Merged
merged 1 commit into from
Feb 23, 2024
Merged

fix(pd): re-add cors headers #3870

merged 1 commit into from
Feb 23, 2024

Conversation

conorsch
Copy link
Contributor

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 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.

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.
Copy link
Contributor

@cratelyn cratelyn left a comment

Choose a reason for hiding this comment

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

it was a pleasure to track down this mystery together today! nice work 🙂

@cratelyn cratelyn added A-node Area: System design and implementation for node software C-bug Category: a bug labels Feb 23, 2024
@conorsch conorsch merged commit 8e767ad into main Feb 23, 2024
6 checks passed
@conorsch conorsch deleted the 3865-re-add-cors-headers branch February 23, 2024 03:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-node Area: System design and implementation for node software C-bug Category: a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pd: cors headers not being served (regression)
2 participants