Skip to content

[sui-proxy] Allow sui bridge validator metrics #19076

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Sep 13, 2024
Merged

[sui-proxy] Allow sui bridge validator metrics #19076

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d60d00b
refactor: rename SuiPeer to AllowedPeer, remove unused p2p_address, r…
johnjmartin Aug 20, 2024
42f4f42
feature: add bridge metrics key fetching to poll_peer_list
johnjmartin Aug 21, 2024
52fa2da
addressing review comments
johnjmartin Aug 28, 2024
0db4cf5
refactor poll_peer_list to not wipe the allow list when rpc requests
johnjmartin Aug 28, 2024
54cce36
feedback: split sui nodes and bridge nodes into separate maps
johnjmartin Sep 9, 2024
238bb18
feedback: make bridge metrics key fetching less failure prone
johnjmartin Sep 10, 2024
7ab836b
fallback_to_cached_key if invalid response + unit testing
johnjmartin Sep 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions crates/sui-proxy/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ anyhow.workspace = true
bytes.workspace = true
clap.workspace = true
protobuf.workspace = true
futures.workspace = true
url.workspace = true
tokio = { workspace = true, features = ["full"] }
tracing.workspace = true
const-str.workspace = true
Expand Down
46 changes: 21 additions & 25 deletions crates/sui-proxy/src/admin.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,10 @@
use crate::config::{DynamicPeerValidationConfig, RemoteWriteConfig, StaticPeerValidationConfig};
use crate::handlers::publish_metrics;
use crate::histogram_relay::HistogramRelay;
use crate::ip::{is_private, to_multiaddr};
use crate::middleware::{
expect_content_length, expect_mysten_proxy_header, expect_valid_public_key,
};
use crate::peers::{SuiNodeProvider, SuiPeer};
use crate::peers::{AllowedPeer, SuiNodeProvider};
use crate::var;
use anyhow::Error;
use anyhow::Result;
Expand All @@ -16,7 +15,7 @@ use fastcrypto::ed25519::{Ed25519KeyPair, Ed25519PublicKey};
use fastcrypto::traits::{KeyPair, ToFromBytes};
use std::fs;
use std::io::BufReader;
use std::net::{IpAddr, SocketAddr};
use std::net::SocketAddr;
use std::sync::Arc;
use std::time::Duration;
use sui_tls::SUI_VALIDATOR_SERVER_NAME;
Expand All @@ -29,7 +28,7 @@ use tower_http::{
trace::{DefaultOnResponse, TraceLayer},
LatencyUnit,
};
use tracing::{error, info, Level};
use tracing::{info, Level};

/// Configure our graceful shutdown scenarios
pub async fn shutdown_signal(h: axum_server::Handle) {
Expand Down Expand Up @@ -200,30 +199,27 @@ fn load_private_key(filename: &str) -> rustls::pki_types::PrivateKeyDer<'static>
/// load the static keys we'll use to allow external non-validator nodes to push metrics
fn load_static_peers(
static_peers: Option<StaticPeerValidationConfig>,
) -> Result<Vec<SuiPeer>, Error> {
) -> Result<Vec<AllowedPeer>, Error> {
let Some(static_peers) = static_peers else {
return Ok(vec![]);
};
let static_keys = static_peers.pub_keys.into_iter().filter_map(|spk|{
let p2p_address: IpAddr = spk.p2p_address.parse().unwrap();
if is_private(p2p_address) {
error!("{} appears to be a private address. We only allow 169.254.0.0/16 addresses to be private; ignoring this entry", p2p_address);
dbg!("skipping {}", spk);
return None;
}
Some(spk)
}).map(|spk|{
let peer_id = hex::decode(spk.peer_id).unwrap();
let public_key = Ed25519PublicKey::from_bytes(peer_id.as_ref()).unwrap();
let p2p_address: IpAddr = spk.p2p_address.parse().unwrap();
let s = SuiPeer{
name:spk.name.clone(),
p2p_address: to_multiaddr(p2p_address),
public_key,
};
info!("loaded static peer: {} public key: {} p2p address: {}", &s.name, &s.public_key, &s.p2p_address);
s
}).collect();
let static_keys = static_peers
.pub_keys
.into_iter()
.map(|spk| {
let peer_id = hex::decode(spk.peer_id).unwrap();
let public_key = Ed25519PublicKey::from_bytes(peer_id.as_ref()).unwrap();
let s = AllowedPeer {
name: spk.name.clone(),
public_key,
};
info!(
"loaded static peer: {} public key: {}",
&s.name, &s.public_key,
);
s
})
.collect();
Ok(static_keys)
}

Expand Down
4 changes: 1 addition & 3 deletions crates/sui-proxy/src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ pub struct RemoteWriteConfig {
pub pool_max_idle_per_host: usize,
}

/// DynamicPeerValidationConfig controls what sui-node binaries that are functioning as a validator that we'll speak with.
/// DynamicPeerValidationConfig controls what sui-node & sui-bridge binaries that are functioning as a validator that we'll speak with.
/// Peer in this case is peers within the consensus committee, for each epoch. This membership is determined dynamically
/// for each epoch via json-rpc calls to a full node.
#[serde_as]
Expand Down Expand Up @@ -79,8 +79,6 @@ pub struct StaticPeerValidationConfig {
pub struct StaticPubKey {
/// friendly name we will see in metrics
pub name: String,
/// friendly ip address we may see in metrics
pub p2p_address: String,
Comment on lines -82 to -83
Copy link
Contributor

Choose a reason for hiding this comment

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

why? don't we want this for sui-node?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

afaict, it's not used anywhere in the proxy. The downside of keeping it is it forces us to set meaningless defaults for the static peers feature (as well as for the bridge validators). For example, this is a snippet of what our prod config looks like right now:

static-peers:
  pub-keys:
    - name: ewr-mysten-ssfn1
      p2p-address: 0.0.0.1
      peer-id: c7bf6cb93ca8fdda655c47ebb85ace28e6931464564332bf63e27e90199c50ee
    - name: ewr-mysten-ssfn2
      p2p-address: 0.0.0.2
      peer-id: 3227f8a05f0faa1a197c075d31135a366a1c6f3d4872cb8af66c14dea3e0eb66
    - name: lhr-mysten-ssfn
      p2p-address: 0.0.0.3
      peer-id: c619a5e0f8f36eac45118c1f8bda28f0f508e2839042781f1d4a9818043f732c

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @suiwombat for thoughts too

Copy link
Contributor

Choose a reason for hiding this comment

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

If its not uses for anything then i agree we should remove it

/// the peer_id from a node config file (Ed25519 PublicKey)
pub peer_id: String,
}
Expand Down
6 changes: 2 additions & 4 deletions crates/sui-proxy/src/handlers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use crate::admin::{Labels, ReqwestClient};
use crate::consumer::{convert_to_remote_write, populate_labels, NodeMetric};
use crate::histogram_relay::HistogramRelay;
use crate::middleware::LenDelimProtobuf;
use crate::peers::SuiPeer;
use crate::peers::AllowedPeer;
use axum::{
extract::{ConnectInfo, Extension},
http::StatusCode,
Expand Down Expand Up @@ -45,9 +45,7 @@ pub async fn publish_metrics(
Extension(labels): Extension<Labels>,
Extension(client): Extension<ReqwestClient>,
ConnectInfo(addr): ConnectInfo<SocketAddr>,
Extension(SuiPeer {
name, public_key, ..
}): Extension<SuiPeer>,
Extension(AllowedPeer { name, public_key }): Extension<AllowedPeer>,
Extension(relay): Extension<HistogramRelay>,
LenDelimProtobuf(data): LenDelimProtobuf,
) -> (StatusCode, &'static str) {
Expand Down
77 changes: 0 additions & 77 deletions crates/sui-proxy/src/ip.rs
View file
Open in desktop

This file was deleted.

5 changes: 1 addition & 4 deletions crates/sui-proxy/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ pub mod config;
pub mod consumer;
pub mod handlers;
pub mod histogram_relay;
mod ip;
pub mod metrics;
pub mod middleware;
pub mod peers;
Expand Down Expand Up @@ -43,7 +42,6 @@ mod tests {
use axum::http::StatusCode;
use axum::routing::post;
use axum::Router;
use multiaddr::Multiaddr;
use prometheus::Encoder;
use prometheus::PROTOBUF_FORMAT;
use protobuf::RepeatedField;
Expand Down Expand Up @@ -145,9 +143,8 @@ mod tests {
// Insert the client's public key into the allowlist and verify the request is successful
allower.get_mut().write().unwrap().insert(
client_pub_key.to_owned(),
peers::SuiPeer {
peers::AllowedPeer {
name: "some-node".into(),
p2p_address: Multiaddr::empty(),
public_key: client_pub_key.to_owned(),
},
);
Expand Down
2 changes: 1 addition & 1 deletion crates/sui-proxy/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ async fn main() -> Result<()> {
let listener = std::net::TcpListener::bind(config.listen_address).unwrap();

let (tls_config, allower) =
// we'll only use the dynamic peers in some cases - it makes little sense to run with the statics
// we'll only use the dynamic peers in some cases - it makes little sense to run with the static's
// since this first mode allows all.
if config.dynamic_peers.certificate_file.is_none() || config.dynamic_peers.private_key.is_none() {
(
Expand Down
Loading
Loading