From 8e767adf6f00c9add4b8854df7d784ad7d825182 Mon Sep 17 00:00:00 2001 From: Conor Schaefer Date: Thu, 22 Feb 2024 12:52:48 -0800 Subject: [PATCH] fix(pd): re-add cors headers 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. --- .github/workflows/smoke.yml | 5 ++++- crates/bin/pd/src/main.rs | 16 +++++++-------- crates/bin/pd/tests/network_integration.rs | 23 ++++++++++++++++++++++ deployments/scripts/smoke-test.sh | 3 +++ 4 files changed, 37 insertions(+), 10 deletions(-) create mode 100644 crates/bin/pd/tests/network_integration.rs diff --git a/.github/workflows/smoke.yml b/.github/workflows/smoke.yml index 610f5bb8d5..68af227f65 100644 --- a/.github/workflows/smoke.yml +++ b/.github/workflows/smoke.yml @@ -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 diff --git a/crates/bin/pd/src/main.rs b/crates/bin/pd/src/main.rs index 86417dedb8..2db2c0e0b8 100644 --- a/crates/bin/pd/src/main.rs +++ b/crates/bin/pd/src/main.rs @@ -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) => { @@ -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 @@ -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 diff --git a/crates/bin/pd/tests/network_integration.rs b/crates/bin/pd/tests/network_integration.rs new file mode 100644 index 0000000000..1739a7ad3b --- /dev/null +++ b/crates/bin/pd/tests/network_integration.rs @@ -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(()) +} diff --git a/deployments/scripts/smoke-test.sh b/deployments/scripts/smoke-test.sh index 56be8bf2fd..ac75a8acc8 100755 --- a/deployments/scripts/smoke-test.sh +++ b/deployments/scripts/smoke-test.sh @@ -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" \