Skip to content

Commit

Permalink
chore(proxy): refactor self-signed config (#10154)
Browse files Browse the repository at this point in the history
## Problem

While reviewing #10152 I found it tricky to actually determine whether
the connection used `allow_self_signed_compute` or not.

I've tried to remove this setting in the past:
* #7884
* #7437
* neondatabase/cloud#13702

But each time it seems it is used by e2e tests

## Summary of changes

The `node_info.allow_self_signed_computes` is always initialised to
false, and then sometimes inherits the proxy config value. There's no
need this needs to be in the node_info, so removing it and propagating
it via `TcpMechansim` is simpler.
  • Loading branch information
conradludgate authored Dec 16, 2024
1 parent ebcbc1a commit 24d6587
Show file tree
Hide file tree
Showing 11 changed files with 20 additions and 30 deletions.
1 change: 0 additions & 1 deletion proxy/src/auth/backend/console_redirect.rs
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,6 @@ async fn authenticate(
NodeInfo {
config,
aux: db_info.aux,
allow_self_signed_compute: false, // caller may override
},
db_info.allowed_ips,
))
Expand Down
1 change: 0 additions & 1 deletion proxy/src/auth/backend/local.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ impl LocalBackend {
branch_id: BranchIdTag::get_interner().get_or_intern("local"),
cold_start_info: ColdStartInfo::WarmCached,
},
allow_self_signed_compute: false,
},
}
}
Expand Down
2 changes: 1 addition & 1 deletion proxy/src/console_redirect_proxy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -213,9 +213,9 @@ pub(crate) async fn handle_client<S: AsyncRead + AsyncWrite + Unpin>(
params_compat: true,
params: &params,
locks: &config.connect_compute_locks,
allow_self_signed_compute: config.allow_self_signed_compute,
},
&user_info,
config.allow_self_signed_compute,
config.wake_compute_retry_config,
config.connect_to_compute_retry_config,
)
Expand Down
1 change: 0 additions & 1 deletion proxy/src/control_plane/client/cplane_proxy_v1.rs
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,6 @@ impl NeonControlPlaneClient {
let node = NodeInfo {
config,
aux: body.aux,
allow_self_signed_compute: false,
};

Ok(node)
Expand Down
1 change: 0 additions & 1 deletion proxy/src/control_plane/client/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,6 @@ impl MockControlPlane {
branch_id: (&BranchId::from("branch")).into(),
cold_start_info: crate::control_plane::messages::ColdStartInfo::Warm,
},
allow_self_signed_compute: false,
};

Ok(node)
Expand Down
13 changes: 3 additions & 10 deletions proxy/src/control_plane/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,28 +67,21 @@ pub(crate) struct NodeInfo {

/// Labels for proxy's metrics.
pub(crate) aux: MetricsAuxInfo,

/// Whether we should accept self-signed certificates (for testing)
pub(crate) allow_self_signed_compute: bool,
}

impl NodeInfo {
pub(crate) async fn connect(
&self,
ctx: &RequestContext,
allow_self_signed_compute: bool,
timeout: Duration,
) -> Result<compute::PostgresConnection, compute::ConnectionError> {
self.config
.connect(
ctx,
self.allow_self_signed_compute,
self.aux.clone(),
timeout,
)
.connect(ctx, allow_self_signed_compute, self.aux.clone(), timeout)
.await
}

pub(crate) fn reuse_settings(&mut self, other: Self) {
self.allow_self_signed_compute = other.allow_self_signed_compute;
self.config.reuse_password(other.config);
}

Expand Down
11 changes: 8 additions & 3 deletions proxy/src/proxy/connect_compute.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,9 @@ pub(crate) struct TcpMechanism<'a> {

/// connect_to_compute concurrency lock
pub(crate) locks: &'static ApiLocks<Host>,

/// Whether we should accept self-signed certificates (for testing)
pub(crate) allow_self_signed_compute: bool,
}

#[async_trait]
Expand All @@ -90,7 +93,11 @@ impl ConnectMechanism for TcpMechanism<'_> {
) -> Result<PostgresConnection, Self::Error> {
let host = node_info.config.get_host();
let permit = self.locks.get_permit(&host).await?;
permit.release_result(node_info.connect(ctx, timeout).await)
permit.release_result(
node_info
.connect(ctx, self.allow_self_signed_compute, timeout)
.await,
)
}

fn update_connect_config(&self, config: &mut compute::ConnCfg) {
Expand All @@ -104,7 +111,6 @@ pub(crate) async fn connect_to_compute<M: ConnectMechanism, B: ComputeConnectBac
ctx: &RequestContext,
mechanism: &M,
user_info: &B,
allow_self_signed_compute: bool,
wake_compute_retry_config: RetryConfig,
connect_to_compute_retry_config: RetryConfig,
) -> Result<M::Connection, M::Error>
Expand All @@ -117,7 +123,6 @@ where
wake_compute(&mut num_retries, ctx, user_info, wake_compute_retry_config).await?;

node_info.set_keys(user_info.get_keys());
node_info.allow_self_signed_compute = allow_self_signed_compute;
mechanism.update_connect_config(&mut node_info.config);

// try once
Expand Down
2 changes: 1 addition & 1 deletion proxy/src/proxy/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -355,9 +355,9 @@ pub(crate) async fn handle_client<S: AsyncRead + AsyncWrite + Unpin>(
params_compat,
params: &params,
locks: &config.connect_compute_locks,
allow_self_signed_compute: mode.allow_self_signed_compute(config),
},
&user_info,
mode.allow_self_signed_compute(config),
config.wake_compute_retry_config,
config.connect_to_compute_retry_config,
)
Expand Down
14 changes: 6 additions & 8 deletions proxy/src/proxy/tests/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -553,7 +553,6 @@ fn helper_create_cached_node_info(cache: &'static NodeInfoCache) -> CachedNodeIn
branch_id: (&BranchId::from("branch")).into(),
cold_start_info: crate::control_plane::messages::ColdStartInfo::Warm,
},
allow_self_signed_compute: false,
};
let (_, node2) = cache.insert_unit("key".into(), Ok(node.clone()));
node2.map(|()| node)
Expand Down Expand Up @@ -588,7 +587,7 @@ async fn connect_to_compute_success() {
max_retries: 5,
backoff_factor: 2.0,
};
connect_to_compute(&ctx, &mechanism, &user_info, false, config, config)
connect_to_compute(&ctx, &mechanism, &user_info, config, config)
.await
.unwrap();
mechanism.verify();
Expand All @@ -606,7 +605,7 @@ async fn connect_to_compute_retry() {
max_retries: 5,
backoff_factor: 2.0,
};
connect_to_compute(&ctx, &mechanism, &user_info, false, config, config)
connect_to_compute(&ctx, &mechanism, &user_info, config, config)
.await
.unwrap();
mechanism.verify();
Expand All @@ -625,7 +624,7 @@ async fn connect_to_compute_non_retry_1() {
max_retries: 5,
backoff_factor: 2.0,
};
connect_to_compute(&ctx, &mechanism, &user_info, false, config, config)
connect_to_compute(&ctx, &mechanism, &user_info, config, config)
.await
.unwrap_err();
mechanism.verify();
Expand All @@ -644,7 +643,7 @@ async fn connect_to_compute_non_retry_2() {
max_retries: 5,
backoff_factor: 2.0,
};
connect_to_compute(&ctx, &mechanism, &user_info, false, config, config)
connect_to_compute(&ctx, &mechanism, &user_info, config, config)
.await
.unwrap();
mechanism.verify();
Expand Down Expand Up @@ -674,7 +673,6 @@ async fn connect_to_compute_non_retry_3() {
&ctx,
&mechanism,
&user_info,
false,
wake_compute_retry_config,
connect_to_compute_retry_config,
)
Expand All @@ -696,7 +694,7 @@ async fn wake_retry() {
max_retries: 5,
backoff_factor: 2.0,
};
connect_to_compute(&ctx, &mechanism, &user_info, false, config, config)
connect_to_compute(&ctx, &mechanism, &user_info, config, config)
.await
.unwrap();
mechanism.verify();
Expand All @@ -715,7 +713,7 @@ async fn wake_non_retry() {
max_retries: 5,
backoff_factor: 2.0,
};
connect_to_compute(&ctx, &mechanism, &user_info, false, config, config)
connect_to_compute(&ctx, &mechanism, &user_info, config, config)
.await
.unwrap_err();
mechanism.verify();
Expand Down
2 changes: 1 addition & 1 deletion proxy/src/redis/notifications.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,14 @@ use pq_proto::CancelKeyData;
use redis::aio::PubSub;
use serde::{Deserialize, Serialize};
use tokio_util::sync::CancellationToken;
use tracing::Instrument;
use uuid::Uuid;

use super::connection_with_credentials_provider::ConnectionWithCredentialsProvider;
use crate::cache::project_info::ProjectInfoCache;
use crate::cancellation::{CancelMap, CancellationHandler};
use crate::intern::{ProjectIdInt, RoleNameInt};
use crate::metrics::{Metrics, RedisErrors, RedisEventsCount};
use tracing::Instrument;

const CPLANE_CHANNEL_NAME: &str = "neondb-proxy-ws-updates";
pub(crate) const PROXY_CHANNEL_NAME: &str = "neondb-proxy-to-proxy-updates";
Expand Down
2 changes: 0 additions & 2 deletions proxy/src/serverless/backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,6 @@ impl PoolingBackend {
locks: &self.config.connect_compute_locks,
},
&backend,
false, // do not allow self signed compute for http flow
self.config.wake_compute_retry_config,
self.config.connect_to_compute_retry_config,
)
Expand Down Expand Up @@ -237,7 +236,6 @@ impl PoolingBackend {
locks: &self.config.connect_compute_locks,
},
&backend,
false, // do not allow self signed compute for http flow
self.config.wake_compute_retry_config,
self.config.connect_to_compute_retry_config,
)
Expand Down

1 comment on commit 24d6587

@github-actions
Copy link

Choose a reason for hiding this comment

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

7084 tests run: 6786 passed, 1 failed, 297 skipped (full report)


Failures on Postgres 17

# Run all failed tests locally:
scripts/pytest -vv -n $(nproc) -k "test_timeline_archival_chaos[release-pg17]"
Flaky tests (4)

Postgres 15

Postgres 14

  • test_pgdata_import_smoke[None-1024-RelBlockSize.MULTIPLE_RELATION_SEGMENTS]: release-arm64

Test coverage report is not available

The comment gets automatically updated with the latest test results
24d6587 at 2024-12-16T12:19:43.658Z :recycle:

Please sign in to comment.