From 31f4c19ded8153f373b0ef6e5a88cb18a244fcc7 Mon Sep 17 00:00:00 2001 From: Martin Hoffmann Date: Fri, 10 Jan 2025 13:49:54 +0100 Subject: [PATCH] Restructure limiting of ASPA provider set sizes. --- doc/manual/source/manual-page.rst | 14 --------- src/config.rs | 25 ---------------- src/http/metrics.rs | 12 +++++++- src/http/status.rs | 15 ++++++---- src/metrics.rs | 34 +++++++++++++++++++-- src/output.rs | 10 +++---- src/payload/validation.rs | 49 ++++++++----------------------- 7 files changed, 69 insertions(+), 90 deletions(-) diff --git a/doc/manual/source/manual-page.rst b/doc/manual/source/manual-page.rst index c47975f2..a03cfc67 100644 --- a/doc/manual/source/manual-page.rst +++ b/doc/manual/source/manual-page.rst @@ -356,13 +356,6 @@ The available options are: If this option is present, ASPA assertions will be processed during validation and included in the produced data set. -.. option:: --aspa-provider-limit - - Limits the number of provider ASNs allowed in an ASPA object. If more - providers are given, all ASPA assertions for the customer ASN are - dropped to avoid false rejections. The default value if not changed - via configuration or this option is 10,000 provider ASNs. - .. option:: --dirty If this option is present, unused files and directories will not be @@ -1208,13 +1201,6 @@ All values can be overridden via the command line options. included in the published dataset. If false or missing, no ASPA assertions will be included. - aspa_provider_limit - An integer value specifying the maximum number of provider ASNs - allowed in an ASPA object. If more providers are given, all ASPA - assertions for the customer ASN are dropped to avoid false - rejections. If the option is missing, a default of 10,000 - provider ASNs is used. - dirty A boolean value which, if true, specifies that unused files and directories should not be deleted from the repository directory diff --git a/src/config.rs b/src/config.rs index 30150834..e2ccefc3 100644 --- a/src/config.rs +++ b/src/config.rs @@ -93,9 +93,6 @@ const DEFAULT_MAX_CA_DEPTH: usize = 32; #[cfg(unix)] const DEFAULT_SYSLOG_FACILITY: Facility = Facility::LOG_DAEMON; -/// The default limit of provider ASNs in ASPA objects. -const DEFAULT_ASPA_PROVIDER_LIMIT: usize = 10_000; - //------------ Config -------------------------------------------------------- @@ -272,12 +269,6 @@ pub struct Config { /// Whether to process ASPA objects. pub enable_aspa: bool, - /// The maximum number of provider ASNs accepted in ASPA objects. - /// - /// If this number is exceeded, all ASPAs for the customer ASN of the - /// offending object are dropped. - pub aspa_provider_limit: usize, - /// Whether to not cleanup the repository directory after a validation run. /// /// If this is `false` and update has not been disabled otherwise, all @@ -630,11 +621,6 @@ impl Config { self.enable_aspa = true } - // aspa_provider_limit - if let Some(value) = args.aspa_provider_limit { - self.aspa_provider_limit = value; - } - // dirty_repository if args.dirty_repository { self.dirty_repository = true @@ -988,11 +974,6 @@ impl Config { enable_aspa: file.take_bool("enable-aspa")?.unwrap_or(false), - aspa_provider_limit: { - file.take_usize("aspa-provider-limit")? - .unwrap_or(DEFAULT_ASPA_PROVIDER_LIMIT) - }, - dirty_repository: file.take_bool("dirty")?.unwrap_or(false), validation_threads: { file.take_small_usize( @@ -1202,7 +1183,6 @@ impl Config { max_ca_depth: DEFAULT_MAX_CA_DEPTH, enable_bgpsec: false, enable_aspa: false, - aspa_provider_limit: DEFAULT_ASPA_PROVIDER_LIMIT, dirty_repository: DEFAULT_DIRTY_REPOSITORY, validation_threads: Config::default_validation_threads(), refresh: Duration::from_secs(DEFAULT_REFRESH), @@ -1429,7 +1409,6 @@ impl Config { insert_int(&mut res, "max-ca-depth", self.max_ca_depth); insert(&mut res, "enable-bgpsec", self.enable_bgpsec); insert(&mut res, "enable-aspa", self.enable_aspa); - insert_int(&mut res, "aspa-provider-limit", self.aspa_provider_limit); insert(&mut res, "dirty", self.dirty_repository); insert_int(&mut res, "validation-threads", self.validation_threads); insert_int(&mut res, "refresh", self.refresh.as_secs()); @@ -1889,10 +1868,6 @@ struct GlobalArgs { #[arg(long)] enable_aspa: bool, - /// Maximum number of provider ASNs in ASPA objects - #[arg(long, value_name = "COUNT")] - aspa_provider_limit: Option, - /// Do not clean up repository directory after validation #[arg(long)] dirty_repository: bool, diff --git a/src/http/metrics.rs b/src/http/metrics.rs index 0ec70f6a..fe67d642 100644 --- a/src/http/metrics.rs +++ b/src/http/metrics.rs @@ -152,6 +152,16 @@ async fn handle_metrics( metrics.local.vrps().contributed ); + // Large ASPAs + target.single( + Metric::new( + "aspa_large_provider_set", + "ASPAs customer ASNs rejected due to large provider set", + MetricType::Gauge + ), + metrics.snapshot.large_aspas + ); + // Collector metrics. rrdp_metrics(&mut target, &metrics.rrdp); rsync_metrics(&mut target, &metrics.rsync); @@ -799,7 +809,7 @@ fn deprecated_metrics( Metric::new( "vrps_final", "final number of VRPs", MetricType::Gauge ), - metrics.payload.vrps().contributed + metrics.snapshot.payload.vrps().contributed ); // Overall number of stale objects diff --git a/src/http/status.rs b/src/http/status.rs index bb415f0d..f281adde 100644 --- a/src/http/status.rs +++ b/src/http/status.rs @@ -115,7 +115,7 @@ async fn handle_status( writeln!(res); // vrps - writeln!(res, "vrps: {}", metrics.payload.vrps().valid); + writeln!(res, "vrps: {}", metrics.snapshot.payload.vrps().valid); // vrps-per-tal write!(res, "vrps-per-tal: "); @@ -128,7 +128,7 @@ async fn handle_status( // unsafe-filtered-vrps writeln!(res, "unsafe-vrps: {}", - metrics.payload.vrps().marked_unsafe + metrics.snapshot.payload.vrps().marked_unsafe ); // unsafe-vrps-per-tal @@ -146,7 +146,7 @@ async fn handle_status( // locally-filtered-vrps writeln!(res, "locally-filtered-vrps: {}", - metrics.payload.vrps().locally_filtered + metrics.snapshot.payload.vrps().locally_filtered ); // locally-filtered-vrps-per-tal @@ -175,7 +175,7 @@ async fn handle_status( // final-vrps writeln!(res, "final-vrps: {}", - metrics.payload.vrps().contributed + metrics.snapshot.payload.vrps().contributed ); // final-vrps-per-tal @@ -373,7 +373,12 @@ async fn handle_api_status( target.member_raw("lastUpdateDuration", "null"); } - json_payload_metrics(target, &metrics.payload); + json_payload_metrics(target, &metrics.snapshot.payload); + + target.member_raw( + "aspasLargeProviderSet", + metrics.snapshot.large_aspas + ); target.member_object("tals", |target| { for tal in &metrics.tals { diff --git a/src/metrics.rs b/src/metrics.rs index 94079d02..ca54669f 100644 --- a/src/metrics.rs +++ b/src/metrics.rs @@ -47,7 +47,7 @@ pub struct Metrics { pub local: PayloadMetrics, /// Overall payload metrics. - pub payload: PayloadMetrics, + pub snapshot: SnapshotMetrics, } impl Metrics { @@ -61,7 +61,7 @@ impl Metrics { repositories: Vec::new(), publication: Default::default(), local: Default::default(), - payload: Default::default(), + snapshot: Default::default(), } } @@ -74,7 +74,7 @@ impl Metrics { metric.finalize(); } self.local.finalize(); - self.payload.finalize(); + self.snapshot.finalize(); } /// Returns the time the metrics were created as a Unix timestamp. @@ -370,6 +370,31 @@ impl ops::AddAssign for PublicationMetrics { } +//------------ SnapshotMetrics ----------------------------------------------- + +/// Metrics regarding the full payload set. +#[derive(Clone, Debug, Default)] +pub struct SnapshotMetrics { + /// Payload metrics portion. + pub payload: PayloadMetrics, + + /// The number of ASPA customer ASNs that had too large ASPAs. + /// + /// This number is subtracted from `payload.aspa.valid` through the + /// `finalize` method, so you don’t have to do that yourself. + pub large_aspas: u32, +} + +impl SnapshotMetrics { + /// Finalizes the metrics by summing up the generated attributes. + pub fn finalize(&mut self) { + self.payload.finalize(); + self.payload.aspas.valid = + self.payload.aspas.valid.saturating_sub(self.large_aspas); + } +} + + //------------ PayloadMetrics ------------------------------------------------ /// Metrics regarding the generated payload set. @@ -390,6 +415,9 @@ pub struct PayloadMetrics { /// The metrics for ASPA payload. pub aspas: VrpMetrics, + /// The number of ASPA payload that was too large. + pub large_aspas: u32, + /// The metrics for all payload items. pub all: VrpMetrics, } diff --git a/src/output.rs b/src/output.rs index 6699633e..a7c608d4 100644 --- a/src/output.rs +++ b/src/output.rs @@ -1419,8 +1419,8 @@ impl Summary { ))?; line(format_args!( " VRPs: {:7} verified, {:7} final;", - metrics.payload.vrps().valid, - metrics.payload.vrps().contributed + metrics.snapshot.payload.vrps().valid, + metrics.snapshot.payload.vrps().contributed ))?; line(format_args!( " router certs: {:7} verified;", @@ -1428,13 +1428,13 @@ impl Summary { ))?; line(format_args!( " router keys: {:7} verified, {:7} final;", - metrics.payload.router_keys.valid, - metrics.payload.router_keys.contributed + metrics.snapshot.payload.router_keys.valid, + metrics.snapshot.payload.router_keys.contributed ))?; line(format_args!( " ASPAs: {:7} verified, {:7} final;", metrics.publication.valid_aspas, - metrics.payload.aspas.contributed + metrics.snapshot.payload.aspas.contributed ))?; Ok(()) } diff --git a/src/payload/validation.rs b/src/payload/validation.rs index ea76c4c9..79a13425 100644 --- a/src/payload/validation.rs +++ b/src/payload/validation.rs @@ -15,7 +15,7 @@ use std::cmp; use std::collections::hash_map; -use std::collections::{HashMap, HashSet}; +use std::collections::HashMap; use std::sync::Arc; use crossbeam_queue::SegQueue; use log::{info, warn}; @@ -75,9 +75,6 @@ pub struct ValidationReport { /// How are we dealing with unsafe VRPs? unsafe_vrps: FilterPolicy, - - /// Maximum number of provider ASNs on ASPA objects. - aspa_provider_limit: usize, } impl ValidationReport { @@ -92,7 +89,6 @@ impl ValidationReport { limit_v4_len: config.limit_v4_len, limit_v6_len: config.limit_v6_len, unsafe_vrps: config.unsafe_vrps, - aspa_provider_limit: config.aspa_provider_limit, } } @@ -275,7 +271,7 @@ impl ProcessPubPoint for PubPointProcessor<'_> { fn process_aspa( &mut self, - uri: &uri::Rsync, + _uri: &uri::Rsync, cert: ResourceCert, aspa: AsProviderAttestation ) -> Result<(), Failed> { @@ -283,18 +279,6 @@ impl ProcessPubPoint for PubPointProcessor<'_> { return Ok(()) } self.pub_point.update_refresh(cert.validity().not_after()); - if aspa.provider_as_set().len() > self.report.aspa_provider_limit { - warn!( - "{}: {} provider ASNs is over the limit of {}. \ - Skipping ASPA for {}.", - uri, - aspa.provider_as_set().len(), - self.report.aspa_provider_limit, - aspa.customer_as() - ); - self.report.rejected.aspa_customers.push(aspa.customer_as()); - return Ok(()) - } self.pub_point.add_aspa( aspa, Arc::new(PublishInfo::signed_object( @@ -529,7 +513,6 @@ pub struct PubAspa { pub struct RejectedResources { v4: IpBlocks, v6: IpBlocks, - aspa_customers: HashSet, } impl RejectedResources { @@ -560,9 +543,6 @@ struct RejectedResourcesBuilder { /// The queue of rejected AS blocks. asns: SegQueue, - - /// The queue of rejected ASPA customer ASNs. - aspa_customers: SegQueue, } impl RejectedResourcesBuilder { @@ -598,7 +578,6 @@ impl RejectedResourcesBuilder { RejectedResources { v4: v4.finalize(), v6: v6.finalize(), - aspa_customers: self.aspa_customers.into_iter().collect(), } } } @@ -774,11 +753,6 @@ impl<'a> SnapshotBuilder<'a> { fn process_aspa(&mut self, aspa: PubAspa, metrics: &mut AllVrpMetrics) { metrics.update(|m| m.aspas.valid += 1); - if self.rejected.aspa_customers.contains(&aspa.customer) { - metrics.update(|m| m.aspas.marked_unsafe += 1); - return - } - // SLURM filtering goes here ... match self.aspas.entry(aspa.customer) { @@ -806,7 +780,7 @@ impl<'a> SnapshotBuilder<'a> { self.insert_assertions(metrics); metrics.finalize(); - self.into_snapshot() + self.into_snapshot(metrics) } fn insert_assertions(&mut self, metrics: &mut Metrics) { @@ -816,22 +790,22 @@ impl<'a> SnapshotBuilder<'a> { entry.insert(info.into()); if origin.is_v4() { metrics.local.v4_origins.contributed += 1; - metrics.payload.v4_origins.contributed += 1; + metrics.snapshot.payload.v4_origins.contributed += 1; } else { metrics.local.v6_origins.contributed += 1; - metrics.payload.v6_origins.contributed += 1; + metrics.snapshot.payload.v6_origins.contributed += 1; } } hash_map::Entry::Occupied(mut entry) => { entry.get_mut().add_local(info); if origin.is_v4() { metrics.local.v4_origins.duplicate += 1; - metrics.payload.v4_origins.duplicate += 1; + metrics.snapshot.payload.v4_origins.duplicate += 1; } else { metrics.local.v6_origins.duplicate += 1; - metrics.payload.v6_origins.duplicate += 1; + metrics.snapshot.payload.v6_origins.duplicate += 1; } } } @@ -842,12 +816,12 @@ impl<'a> SnapshotBuilder<'a> { hash_map::Entry::Vacant(entry) => { entry.insert(info.into()); metrics.local.router_keys.contributed += 1; - metrics.payload.router_keys.contributed += 1; + metrics.snapshot.payload.router_keys.contributed += 1; } hash_map::Entry::Occupied(mut entry) => { entry.get_mut().add_local(info); metrics.local.router_keys.duplicate += 1; - metrics.payload.router_keys.duplicate += 1; + metrics.snapshot.payload.router_keys.duplicate += 1; } } } @@ -855,7 +829,7 @@ impl<'a> SnapshotBuilder<'a> { // XXX ASPA assertions. } - fn into_snapshot(self) -> PayloadSnapshot { + fn into_snapshot(self, metrics: &mut Metrics) -> PayloadSnapshot { PayloadSnapshot::new( self.origins.into_iter(), self.router_keys.into_iter(), @@ -871,6 +845,7 @@ impl<'a> SnapshotBuilder<'a> { with {} provider ASNs.", customer, providers.len() ); + metrics.snapshot.large_aspas += 1; None } } @@ -901,7 +876,7 @@ impl<'a> AllVrpMetrics<'a> { Some(index) => Some(&mut metrics.repositories[index].payload), None => None }, - all: &mut metrics.payload, + all: &mut metrics.snapshot.payload, } }