Skip to content

Commit

Permalink
fix(pd): re-add cors headers
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
conorsch committed Feb 22, 2024
1 parent 6b99c0e commit bf25d29
Show file tree
Hide file tree
Showing 4 changed files with 37 additions and 10 deletions.
5 changes: 4 additions & 1 deletion .github/workflows/smoke.yml
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,12 @@ jobs:
- name: Display comet logs
if: always()
run: cat deployments/logs/comet.log
- name: Display pd logs
- name: Display pd runtime logs
if: always()
run: cat deployments/logs/pd.log
- name: Display pd test logs
if: always()
run: cat deployments/logs/pd-tests.log
- name: Display pclientd logs
if: always()
run: cat deployments/logs/pclientd.log
Expand Down
16 changes: 7 additions & 9 deletions crates/bin/pd/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -168,11 +168,6 @@ async fn main() -> anyhow::Result<()> {
use penumbra_shielded_pool::component::rpc::Server as ShieldedPoolServer;
use penumbra_stake::component::rpc::Server as StakeServer;

// Set rather permissive CORS headers for pd's gRPC: the service
// should be accessible from arbitrary web contexts, such as localhost,
// or any FQDN that wants to reference its data.
let cors_layer = CorsLayer::permissive();

let mut grpc_server = Server::builder()
.trace_fn(|req| match remote_addr(req) {
Some(remote_addr) => {
Expand All @@ -185,9 +180,6 @@ async fn main() -> anyhow::Result<()> {
// typically uses HTTP/2, which requires HTTPS. Accepting HTTP/2
// allows local applications such as web browsers to talk to pd.
.accept_http1(true)
// Add permissive CORS headers, so pd's gRPC services are accessible
// from arbitrary web contexts, including from localhost.
.layer(cors_layer)
// As part of #2932, we are disabling all timeouts until we circle back to our
// performance story.
// Sets a timeout for all gRPC requests, but note that in the case of streaming
Expand Down Expand Up @@ -242,7 +234,13 @@ async fn main() -> anyhow::Result<()> {
//
// TODO(kate): this is where we may attach additional routes upon this router in the
// future. see #3646 for more information.
let router = grpc_server.into_router();
let router = grpc_server
.into_router()
// Set rather permissive CORS headers for pd's gRPC: the service
// should be accessible from arbitrary web contexts, such as localhost,
// or any FQDN that wants to reference its data.
.layer(CorsLayer::permissive());

let make_svc = router.into_make_service();

// Now start the GRPC server, initializing an ACME client to use as a certificate
Expand Down
23 changes: 23 additions & 0 deletions crates/bin/pd/tests/network_integration.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
//! Basic integration testing for pd.
//!
//! Validates behavior of the pd binary itself, such as serving specific HTTP
//! headers in all contexts. Does NOT evaluate application logic; see the
//! integration tests for pcli/pclientd for that.

#[ignore]
#[tokio::test]
/// Confirm that permissive CORS headers are returned in HTTP responses
/// by pd. We want these headers to be served directly by pd, so that
/// an intermediate reverse-proxy is not required.
async fn check_cors_headers() -> anyhow::Result<()> {
let client = reqwest::Client::new();
let pd_url =
std::env::var("PENUMBRA_NODE_PD_URL").unwrap_or("http://localhost:8080".to_string());
let r = client.get(pd_url).send().await?;
assert_eq!(r.headers().get("access-control-allow-origin").unwrap(), "*");
assert_eq!(
r.headers().get("access-control-expose-headers").unwrap(),
"*"
);
Ok(())
}
3 changes: 3 additions & 0 deletions deployments/scripts/smoke-test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,9 @@ trap 'kill -9 "$cometbft_pid" "$pd_pid"' EXIT
echo "Waiting $TESTNET_BOOTTIME seconds for network to boot..."
sleep "$TESTNET_BOOTTIME"

echo "Running pd integration tests against running pd binary"
cargo test --release --package pd -- --ignored --test-threads 1 --nocapture | tee "${SMOKE_LOG_DIR}/pd-tests.log"

echo "Running pclientd integration tests against network"
PENUMBRA_NODE_PD_URL="http://127.0.0.1:8080" \
PCLI_UNLEASH_DANGER="yes" \
Expand Down

0 comments on commit bf25d29

Please sign in to comment.