Code health after review: docs, metrics, structure, one more fuzz target
What the code-health review found, and what a fuzz target written for it
found in its first seconds.
- A set of attributes with the same attribute twice is refused by the
routing table. Its readers (the selection key, the path, the next hop)
could each take a different one of the two. The decoder never lets one
through; the new fuzz target `attrs` found that the table would.
- bgp_peer_up{peer}, to alert on a peer going away, and bgp_build_info.
Every metric is there from the first scrape. Alerts for a peer down,
hijack attempts and kernel errors are in the README.
- `bgp --check config` validates a configuration without starting, and
`--version` says which build it is. CI runs the first on the example
configuration, which a test now also parses with its commented fields
put in; the example had to learn about the next hop check.
- Path attributes have a module of their own, packet::attributes,
re-exported from packet::message as before.
- policy::From, which shadowed the trait, is RouteSource. The server's
connection, dialer and fsm modules are private. Logging arguments live
in the binary; the server library no longer depends on clap.
- One is_unicast_link_local (the standard library's) instead of two of
our own; SelectionKey::origin(); one place where an Import is made.
- READMEs say what the daemon does today rather than what it did not do
a year ago. Documentation links are checked in CI. Unused dependencies
and bgp_web::serve are gone; the Docker build is --locked and does not
ship the fuzz corpus.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This commit is contained in:
1 parent
9ca86ede2f
commit
3d946c32ce
41 files changed
+1323
-1102
No files matched your search
@@ -16,6 +16,7 @@ eyre.workspace = true
|
||||
protobuf.workspace = true
|
||||
tokio.workspace = true
|
||||
tracing.workspace = true
|
||||
tracing-subscriber.workspace = true
|
||||
|
||||
[lints]
|
||||
workspace = true
|
||||
+20
-6
@@ -7,6 +7,10 @@ daemon together with its read-only HTTP status interface.
|
||||
cargo run -- path/to/config.textpb
|
||||
```
|
||||
|
||||
`--check` reads the configuration, says whether it is in order, and stops
|
||||
without listening on anything or touching a route: for trying a change
|
||||
before the daemon is restarted with it.
|
||||
|
||||
The configuration format is defined in `crates/server/proto/config.proto`;
|
||||
`testdata/server.textpb` is a worked example. The daemon is configured
|
||||
entirely through that file: peers, and the routes it originates
|
||||
@@ -403,6 +407,8 @@ For alerting, `/metricz` has:
|
||||
| `bgp_peer_max_prefixes{peer}` | The peer's limit, where it has one |
|
||||
| `bgp_prefix_limit_exceeded_total{peer}` | Sessions ended for going over the limit |
|
||||
| `bgp_connections_refused_total{reason}` | Connections closed without a session |
|
||||
| `bgp_peer_up{peer}` | 1 while the session with a configured peer is Established, else 0 |
|
||||
| `bgp_build_info{version}` | Always 1; says which version is running |
|
||||
|
||||
Rules to start from:
|
||||
|
||||
@@ -429,6 +435,20 @@ groups:
|
||||
- alert: BgpPrefixLimitExceeded
|
||||
expr: increase(bgp_prefix_limit_exceeded_total[1h]) > 0
|
||||
labels: { severity: page }
|
||||
# A configured peer has no session.
|
||||
- alert: BgpPeerDown
|
||||
expr: bgp_peer_up == 0
|
||||
for: 5m
|
||||
labels: { severity: page }
|
||||
# Someone is trying to have our own prefixes, or our traffic, go
|
||||
# where they should not.
|
||||
- alert: BgpHijackAttempt
|
||||
expr: increase(bgp_routes_rejected_total{reason=~"prefix is our own|next hop is not the peer"}[10m]) > 0
|
||||
labels: { severity: page }
|
||||
# The kernel is refusing routes, or its answers are being lost.
|
||||
- alert: BgpKernelErrors
|
||||
expr: increase(bgp_kernel_errors_total[10m]) > 0
|
||||
labels: { severity: warning }
|
||||
```
|
||||
|
||||
## Trying it without a router
|
||||
@@ -445,9 +465,3 @@ cargo run -- testdata/server.textpb
|
||||
cargo run -p bgp-integration-tests --example fake_peer -- \
|
||||
--server 127.0.0.1:179 --asn 65001 --announce 203.0.113.0/24 --echo
|
||||
```
|
||||
|
||||
Please refer to the other implementation crates in this repository where most
|
||||
of the actual implementation lives.
|
||||
|
||||
Warning: This is still very much a work in progress and is not yet ready for
|
||||
production use.
|
||||
@@ -0,0 +1,29 @@
|
||||
//! What the daemon logs, and how much.
|
||||
|
||||
use clap::Args;
|
||||
use tracing::Level;
|
||||
use tracing_subscriber::EnvFilter;
|
||||
use tracing_subscriber::filter::ParseError;
|
||||
|
||||
/// What the level given on the command line is for: this program and the
|
||||
/// crates it is made of, which all begin with `bgp`. Everything else (the
|
||||
/// HTTP server, say) stays at what `RUST_LOG` says, or silent.
|
||||
const OURS: &str = "bgp";
|
||||
|
||||
#[derive(Debug, Args)]
|
||||
pub struct LoggingArgs {
|
||||
/// Set the minimum log level. At `debug` and below there is a line for
|
||||
/// every UPDATE and every route announced.
|
||||
#[arg(short, long, global = true, default_value_t = Level::INFO, help_heading = "Logging")]
|
||||
level: Level,
|
||||
}
|
||||
|
||||
impl LoggingArgs {
|
||||
pub fn install(&self) -> Result<(), ParseError> {
|
||||
let filter = EnvFilter::builder()
|
||||
.from_env_lossy()
|
||||
.add_directive(format!("{OURS}={}", self.level).parse()?);
|
||||
tracing_subscriber::fmt().with_env_filter(filter).init();
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
+28
-7
@@ -1,34 +1,55 @@
|
||||
use std::path::PathBuf;
|
||||
use std::sync::Arc;
|
||||
|
||||
use bgp_server::config::Config;
|
||||
use bgp_server::proto::config::BgpConfig;
|
||||
use bgp_server::server::{BgpServer, shut_down};
|
||||
use bgp_server::tracing::TracingArgs;
|
||||
use bgp_web::{AppState, RouterInfo};
|
||||
use clap::Parser;
|
||||
use eyre::{Context, Result};
|
||||
use tracing::{error, info};
|
||||
|
||||
use crate::logging::LoggingArgs;
|
||||
|
||||
mod logging;
|
||||
|
||||
/// A BGP speaker.
|
||||
#[derive(Parser, Debug)]
|
||||
#[command(version)]
|
||||
struct ServerArgs {
|
||||
/// Path to the textproto configuration file.
|
||||
config: String,
|
||||
config: PathBuf,
|
||||
|
||||
/// Read the configuration, say whether it is in order, and stop:
|
||||
/// nothing is listened on and no route is touched.
|
||||
#[arg(long)]
|
||||
check: bool,
|
||||
|
||||
#[command(flatten)]
|
||||
tracing: TracingArgs,
|
||||
logging: LoggingArgs,
|
||||
}
|
||||
|
||||
#[tokio::main]
|
||||
async fn main() -> Result<()> {
|
||||
let args = ServerArgs::parse();
|
||||
args.tracing.install()?;
|
||||
args.logging.install()?;
|
||||
log_panics();
|
||||
|
||||
let config_string =
|
||||
std::fs::read_to_string(&args.config).wrap_err("Failed to open config file")?;
|
||||
let config_string = std::fs::read_to_string(&args.config)
|
||||
.wrap_err_with(|| format!("Failed to read {}", args.config.display()))?;
|
||||
let proto_config = protobuf::text_format::parse_from_str::<BgpConfig>(&config_string)
|
||||
.wrap_err("Failed to parse config file")?;
|
||||
let config = Config::from_proto(proto_config).wrap_err("Invalid configuration")?;
|
||||
if args.check {
|
||||
println!(
|
||||
"{} is in order: AS{}, peers: {}, routes originated: {}",
|
||||
args.config.display(),
|
||||
config.local_asn,
|
||||
config.peers.len(),
|
||||
config.originate.len()
|
||||
);
|
||||
return Ok(());
|
||||
}
|
||||
|
||||
let server = BgpServer::bind(config).await?;
|
||||
|
||||
@@ -45,7 +66,7 @@ async fn main() -> Result<()> {
|
||||
server.config().local_asn,
|
||||
env!("CARGO_PKG_VERSION"),
|
||||
)
|
||||
.with_config(&args.config, &config_string),
|
||||
.with_config(args.config.to_string_lossy(), &config_string),
|
||||
),
|
||||
registry: server.registry.clone(),
|
||||
local_rib: server.local_rib.clone(),
|
||||
|
||||
Reference in new issue
Block a user