Skip to content

Commit

Permalink
[sled-agent-client] automatically derive Eq/Hash for a few structs (#…
Browse files Browse the repository at this point in the history
…5666)

These impls are currently correct, but
#5649 was going to add new
fields to these structs. That would make these impls incorrect in a
somewhat terrifying fashion. Having `PartialEq` and `Hash` disagree for
a type is a recipe for
difficult-to-debug bugs and security vulnerabilities. See
https://users.rust-lang.org/t/what-hapens-if-hash-and-partialeq-dont-match-when-using-hashmap/98052/19
for some discussion.

In this case we can simply tell progenitor to derive these traits. But
in the future, one way to handle this at compile time is to write:

```rust
let BgpConfig {
    asn,
    originate,
} = self;
asn.hash(hasher);
originate.hash(hasher);
```

If a new field is added, then this will fail loudly.
  • Loading branch information
sunshowers authored Apr 30, 2024
1 parent 3f6264b commit 3f9f572
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 62 deletions.
67 changes: 7 additions & 60 deletions clients/sled-agent-client/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,6 @@ use std::fmt;
use std::hash::Hash;
use std::net::IpAddr;
use std::net::SocketAddr;
use types::{
BfdPeerConfig, BgpConfig, BgpPeerConfig, PortConfigV1, RouteConfig,
};
use uuid::Uuid;

progenitor::generate_api!(
Expand All @@ -31,6 +28,13 @@ progenitor::generate_api!(
post_hook = (|log: &slog::Logger, result: &Result<_, _>| {
slog::debug!(log, "client response"; "result" => ?result);
}),
patch = {
BfdPeerConfig = { derives = [PartialEq, Eq, Hash, Serialize, Deserialize] },
BgpConfig = { derives = [PartialEq, Eq, Hash, Serialize, Deserialize] },
BgpPeerConfig = { derives = [PartialEq, Eq, Hash, Serialize, Deserialize] },
PortConfigV1 = { derives = [PartialEq, Eq, Hash, Serialize, Deserialize] },
RouteConfig = { derives = [PartialEq, Eq, Hash, Serialize, Deserialize] },
},
//TODO trade the manual transformations later in this file for the
// replace directives below?
replace = {
Expand Down Expand Up @@ -664,60 +668,3 @@ impl TestInterfaces for Client {
.expect("disk_finish_transition() failed unexpectedly");
}
}

impl Eq for BgpConfig {}

impl Hash for BgpConfig {
fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
self.asn.hash(state);
self.originate.hash(state);
}
}

impl Hash for BgpPeerConfig {
fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
self.addr.hash(state);
self.asn.hash(state);
self.port.hash(state);
self.hold_time.hash(state);
self.connect_retry.hash(state);
self.delay_open.hash(state);
self.idle_hold_time.hash(state);
self.keepalive.hash(state);
}
}

impl Hash for RouteConfig {
fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
self.destination.hash(state);
self.nexthop.hash(state);
}
}

impl Eq for PortConfigV1 {}

impl Hash for PortConfigV1 {
fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
self.addresses.hash(state);
self.autoneg.hash(state);
self.bgp_peers.hash(state);
self.port.hash(state);
self.routes.hash(state);
self.switch.hash(state);
self.uplink_port_fec.hash(state);
self.uplink_port_speed.hash(state);
}
}

impl Eq for BfdPeerConfig {}

impl Hash for BfdPeerConfig {
fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
self.local.hash(state);
self.remote.hash(state);
self.detection_threshold.hash(state);
self.required_rx.hash(state);
self.mode.hash(state);
self.switch.hash(state);
}
}
4 changes: 2 additions & 2 deletions common/src/api/internal/shared.rs
Original file line number Diff line number Diff line change
Expand Up @@ -362,7 +362,7 @@ pub enum ExternalPortDiscovery {

/// Switchport Speed options
#[derive(
Copy, Clone, Debug, Deserialize, Serialize, PartialEq, JsonSchema, Hash,
Copy, Clone, Debug, Deserialize, Serialize, PartialEq, Eq, JsonSchema, Hash,
)]
#[serde(rename_all = "snake_case")]
pub enum PortSpeed {
Expand All @@ -388,7 +388,7 @@ pub enum PortSpeed {

/// Switchport FEC options
#[derive(
Copy, Clone, Debug, Deserialize, Serialize, PartialEq, JsonSchema, Hash,
Copy, Clone, Debug, Deserialize, Serialize, PartialEq, Eq, JsonSchema, Hash,
)]
#[serde(rename_all = "snake_case")]
pub enum PortFec {
Expand Down

0 comments on commit 3f9f572

Please sign in to comment.