Skip to content

Commit

Permalink
address review comment
Browse files Browse the repository at this point in the history
  • Loading branch information
@williampsmith
williampsmith committed Nov 6, 2024
1 parent 1c21039 commit 6bdc92e
Show file tree
Hide file tree
Showing 3 changed files with 48 additions and 35 deletions.
39 changes: 24 additions & 15 deletions crates/sui-core/src/authority_server.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,7 @@ pub struct ValidatorServiceMetrics {
forwarded_header_parse_error: IntCounter,
forwarded_header_invalid: IntCounter,
forwarded_header_not_included: IntCounter,
client_id_source_config_mismatch: IntCounter,
}

impl ValidatorServiceMetrics {
Expand Down Expand Up @@ -329,6 +330,12 @@ impl ValidatorServiceMetrics {
registry,
)
.unwrap(),
client_id_source_config_mismatch: register_int_counter_with_registry!(
"validator_service_client_id_source_config_mismatch",
"Number of times detected that client id source config doesn't agree with x-forwarded-for header",
registry,
)
.unwrap(),
}
}

Expand Down Expand Up @@ -1225,19 +1232,19 @@ impl ValidatorService {
return None;
}
let contents_len = header_contents.len();
// Network topology should not be very dynamic, therefore if it changes and the above
// invariant is violated, we should fail loudly so that the node config can be updated.
assert!(
contents_len >= *num_hops,
"x-forwarded-for header value of {:?} contains {} values, but {} hops were specified. \
Expected at least {} values. Please correctly set the `x-forwarded-for` value under \
`client-id-source` in the node config.",
header_contents,
contents_len,
num_hops,
contents_len,
);
let contents_len = header_contents.len();
if contents_len < *num_hops {
error!(
"x-forwarded-for header value of {:?} contains {} values, but {} hops were specified. \
Expected at least {} values. Please correctly set the `x-forwarded-for` value under \
`client-id-source` in the node config.",
header_contents,
contents_len,
num_hops,
contents_len,
);
self.metrics.client_id_source_config_mismatch.inc();
return None;
}
let Some(client_ip) = header_contents.get(contents_len - num_hops)
else {
error!(
Expand Down Expand Up @@ -1337,8 +1344,10 @@ fn make_tonic_request_for_testing<T>(message: T) -> tonic::Request<T> {
// TODO: refine error matching here
fn normalize(err: SuiError) -> Weight {
match err {
SuiError::UserInputError { .. }
| SuiError::InvalidSignature { .. }
SuiError::UserInputError {
error: UserInputError::IncorrectUserSignature { .. },
} => Weight::one(),
SuiError::InvalidSignature { .. }
| SuiError::SignerSignatureAbsent { .. }
| SuiError::SignerSignatureNumberMismatch { .. }
| SuiError::IncorrectSigner { .. }
Expand Down
9 changes: 4 additions & 5 deletions crates/sui-core/src/traffic_controller/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,6 @@ async fn run_clear_blocklists_loop(blocklists: Blocklists, metrics: Arc<TrafficC
metrics
.connection_ip_blocklist_len
.set(blocklists.clients.len() as i64);
debug!("Connection_ip_blocklist: {:?}", blocklists.clients);
metrics
.proxy_ip_blocklist_len
.set(blocklists.proxied_clients.len() as i64);
Expand Down Expand Up @@ -379,13 +378,13 @@ async fn run_tally_loop(
metrics
.highest_direct_spam_rate
.set(highest_direct_rate.0 as i64);
trace!("Recent highest direct spam rate: {:?}", highest_direct_rate);
debug!("Recent highest direct spam rate: {:?}", highest_direct_rate);
}
if let Some(highest_proxied_rate) = spam_policy.highest_proxied_rate() {
metrics
.highest_proxied_spam_rate
.set(highest_proxied_rate.0 as i64);
trace!(
debug!(
"Recent highest proxied spam rate: {:?}",
highest_proxied_rate
);
Expand All @@ -396,7 +395,7 @@ async fn run_tally_loop(
metrics
.highest_direct_error_rate
.set(highest_direct_rate.0 as i64);
trace!(
debug!(
"Recent highest direct error rate: {:?}",
highest_direct_rate
);
Expand All @@ -405,7 +404,7 @@ async fn run_tally_loop(
metrics
.highest_proxied_error_rate
.set(highest_proxied_rate.0 as i64);
trace!(
debug!(
"Recent highest proxied error rate: {:?}",
highest_proxied_rate
);
Expand Down
35 changes: 20 additions & 15 deletions crates/sui-e2e-tests/tests/traffic_control_tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,15 @@ use sui_network::default_mysten_network_config;
use sui_swarm_config::network_config_builder::ConfigBuilder;
use sui_test_transaction_builder::batch_make_transfer_transactions;
use sui_types::{
crypto::Ed25519SuiSignature,
quorum_driver_types::ExecuteTransactionRequestType,
signature::GenericSignature,
traffic_control::{
FreqThresholdConfig, PolicyConfig, PolicyType, RemoteFirewallConfig, Weight,
},
};
use test_cluster::{TestCluster, TestClusterBuilder};
use tracing::{error, info};

#[tokio::test]
async fn test_validator_traffic_control_noop() -> Result<(), anyhow::Error> {
Expand Down Expand Up @@ -225,7 +228,7 @@ async fn test_validator_traffic_control_error_blocked() -> Result<(), anyhow::Er
.with_policy_config(Some(policy_config))
.build();
let committee = network_config.committee_with_network();
let _test_cluster = TestClusterBuilder::new()
let test_cluster = TestClusterBuilder::new()
.set_network_config(network_config)
.build()
.await;
Expand All @@ -235,12 +238,13 @@ async fn test_validator_traffic_control_error_blocked() -> Result<(), anyhow::Er
);
let (_, auth_client) = local_clients.first_key_value().unwrap();

// transaction signed using user wallet from a different chain/genesis,
// therefore we should fail with UserInputError
let other_cluster = TestClusterBuilder::new().build().await;

let mut txns = batch_make_transfer_transactions(&other_cluster.wallet, n as usize).await;
let tx = txns.swap_remove(0);
let mut txns = batch_make_transfer_transactions(&test_cluster.wallet, n as usize).await;
let mut tx = txns.swap_remove(0);
let signatures = tx.tx_signatures_mut_for_testing();
signatures.pop();
signatures.push(GenericSignature::Signature(
sui_types::crypto::Signature::Ed25519SuiSignature(Ed25519SuiSignature::default()),
));

// it should take no more than 4 requests to be added to the blocklist
for _ in 0..n {
Expand All @@ -251,7 +255,7 @@ async fn test_validator_traffic_control_error_blocked() -> Result<(), anyhow::Er
}
}
}
panic!("Expected spam policy to trigger within {n} requests");
panic!("Expected error policy to trigger within {n} requests");
}

#[tokio::test]
Expand Down Expand Up @@ -406,7 +410,7 @@ async fn test_validator_traffic_control_error_delegated() -> Result<(), anyhow::
.with_firewall_config(Some(firewall_config))
.build();
let committee = network_config.committee_with_network();
let _test_cluster = TestClusterBuilder::new()
let test_cluster = TestClusterBuilder::new()
.set_network_config(network_config)
.build()
.await;
Expand All @@ -416,12 +420,13 @@ async fn test_validator_traffic_control_error_delegated() -> Result<(), anyhow::
);
let (_, auth_client) = local_clients.first_key_value().unwrap();

// transaction signed using user wallet from a different chain/genesis,
// therefore we should fail with UserInputError
let other_cluster = TestClusterBuilder::new().build().await;

let mut txns = batch_make_transfer_transactions(&other_cluster.wallet, n as usize).await;
let tx = txns.swap_remove(0);
let mut txns = batch_make_transfer_transactions(&test_cluster.wallet, n as usize).await;
let mut tx = txns.swap_remove(0);
let signatures = tx.tx_signatures_mut_for_testing();
signatures.pop();
signatures.push(GenericSignature::Signature(
sui_types::crypto::Signature::Ed25519SuiSignature(Ed25519SuiSignature::default()),
));

// start test firewall server
let mut server = NodeFwTestServer::new();
Expand Down

0 comments on commit 6bdc92e

Please sign in to comment.