Skip to content

Commit

Permalink
safekeeper,pageserver: fix CPU profiling allowlists (#9856)
Browse files Browse the repository at this point in the history
## Problem

The HTTP router allowlists matched both on the path and the query
string. This meant that only `/profile/cpu` would be allowed without
auth, while `/profile/cpu?format=svg` would require auth.

Follows #9764.

## Summary of changes

* Match allowlists on URI path, rather than the entire URI.
* Fix the allowlist for Safekeeper to use `/profile/cpu` rather than the
old `/pprof/profile`.
* Just use a constant slice for the allowlist; it's only a handful of
items, and these handlers are not on hot paths.
  • Loading branch information
erikgrinaker authored Nov 22, 2024
1 parent 211e417 commit e939d36
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 18 deletions.
11 changes: 4 additions & 7 deletions pageserver/src/http/routes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ pub struct State {
conf: &'static PageServerConf,
tenant_manager: Arc<TenantManager>,
auth: Option<Arc<SwappableJwtAuth>>,
allowlist_routes: Vec<Uri>,
allowlist_routes: &'static [&'static str],
remote_storage: GenericRemoteStorage,
broker_client: storage_broker::BrokerClientChannel,
disk_usage_eviction_state: Arc<disk_usage_eviction_task::State>,
Expand All @@ -147,16 +147,13 @@ impl State {
deletion_queue_client: DeletionQueueClient,
secondary_controller: SecondaryController,
) -> anyhow::Result<Self> {
let allowlist_routes = [
let allowlist_routes = &[
"/v1/status",
"/v1/doc",
"/swagger.yml",
"/metrics",
"/profile/cpu",
]
.iter()
.map(|v| v.parse().unwrap())
.collect::<Vec<_>>();
];
Ok(Self {
conf,
tenant_manager,
Expand Down Expand Up @@ -3155,7 +3152,7 @@ pub fn make_router(
if auth.is_some() {
router = router.middleware(auth_middleware(|request| {
let state = get_state(request);
if state.allowlist_routes.contains(request.uri()) {
if state.allowlist_routes.contains(&request.uri().path()) {
None
} else {
state.auth.as_deref()
Expand Down
15 changes: 4 additions & 11 deletions safekeeper/src/http/routes.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
use hyper::{Body, Request, Response, StatusCode, Uri};
use once_cell::sync::Lazy;
use hyper::{Body, Request, Response, StatusCode};
use serde::{Deserialize, Serialize};
use std::collections::{HashMap, HashSet};
use std::collections::HashMap;
use std::fmt;
use std::io::Write as _;
use std::str::FromStr;
Expand Down Expand Up @@ -574,14 +573,8 @@ pub fn make_router(conf: SafeKeeperConf) -> RouterBuilder<hyper::Body, ApiError>
let mut router = endpoint::make_router();
if conf.http_auth.is_some() {
router = router.middleware(auth_middleware(|request| {
#[allow(clippy::mutable_key_type)]
static ALLOWLIST_ROUTES: Lazy<HashSet<Uri>> = Lazy::new(|| {
["/v1/status", "/metrics", "/pprof/profile"]
.iter()
.map(|v| v.parse().unwrap())
.collect()
});
if ALLOWLIST_ROUTES.contains(request.uri()) {
const ALLOWLIST_ROUTES: &[&str] = &["/v1/status", "/metrics", "/profile/cpu"];
if ALLOWLIST_ROUTES.contains(&request.uri().path()) {
None
} else {
// Option<Arc<SwappableJwtAuth>> is always provided as data below, hence unwrap().
Expand Down

1 comment on commit e939d36

@github-actions
Copy link

Choose a reason for hiding this comment

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

5626 tests run: 5398 passed, 1 failed, 227 skipped (full report)


Failures on Postgres 16

  • test_compaction_l0_memory[github-actions-selfhosted]: release-x86-64
# Run all failed tests locally:
scripts/pytest -vv -n $(nproc) -k "test_compaction_l0_memory[release-pg16-github-actions-selfhosted]"
Flaky tests (1)

Postgres 17

Code coverage* (full report)

  • functions: 31.4% (7958 of 25344 functions)
  • lines: 49.3% (63157 of 128104 lines)

* collected from Rust tests only


The comment gets automatically updated with the latest test results
e939d36 at 2024-11-22T19:42:30.352Z :recycle:

Please sign in to comment.