Store published ports as a sorted Vec instead of a BTreeSet

helios-oci/Cargo.toml

@@ -27,3 +27,6 @@ serde_json.workspace = true
 serde_with.workspace = true
 thiserror.workspace = true
 tokio-stream.workspace = true
+
+[dev-dependencies]
+pretty_assertions.workspace = true

helios-oci/src/container.rs

@@ -1,4 +1,4 @@
-use std::collections::{BTreeSet, HashMap};
+use std::collections::HashMap;
 
 use bollard::{
     config::{
@@ -1367,10 +1367,9 @@ pub struct ContainerConfig {
     #[serde(skip_serializing_if = "Vec::is_empty")]
     pub volumes: Vec<Mount>,
 
-    /// Published container ports. Serialized as compose short-syntax strings;
-    /// the set keeps the serialized form deterministic and deduplicated.
-    #[serde(skip_serializing_if = "BTreeSet::is_empty")]
-    pub ports: BTreeSet<PortMapping>,
+    /// Published container ports.
+    #[serde(skip_serializing_if = "Vec::is_empty")]
+    pub ports: Vec<PortMapping>,
 
     /// Container healthcheck. `None` means defer to the image's HEALTHCHECK
     pub healthcheck: Option<Healthcheck>,
@@ -1581,6 +1580,7 @@ impl<N: Namespace> LocalContainer<N> {
 mod tests {
     use super::*;
     use bollard::models::{HostConfig, Mount as EngineMount};
+    use pretty_assertions::assert_eq;
 
     fn vol_mount(target: &str, source: &str) -> EngineMount {
         EngineMount {
@@ -1726,7 +1726,7 @@ mod tests {
     #[test]
     fn container_create_body_emits_ports() {
         let cfg = ContainerConfig {
-            ports: BTreeSet::from([
+            ports: Vec::from([
                 "8080:80".parse().unwrap(),
                 "127.0.0.1:8081:80".parse().unwrap(),
                 "53:53/udp".parse().unwrap(),
@@ -1793,15 +1793,17 @@ mod tests {
             ..Default::default()
         };
         let c: LocalContainer = resp.try_into().unwrap();
-        assert_eq!(c.config.ports, BTreeSet::from(["8080:80".parse().unwrap()]));
+        assert_eq!(c.config.ports, Vec::from(["8080:80".parse().unwrap()]));
     }
 
     #[test]
     fn ports_round_trip_through_create_and_inspect() {
-        let ports: BTreeSet<PortMapping> = BTreeSet::from([
+        // In canonical order (sorted by container port then protocol), so the
+        // round trip through create and inspect is the identity.
+        let ports = Vec::from([
+            "53:53/udp".parse().unwrap(),
             "8080:80".parse().unwrap(),
             "127.0.0.1:8081:80".parse().unwrap(),
-            "53:53/udp".parse().unwrap(),
             "443".parse().unwrap(),
             "8000:3000".parse().unwrap(),
         ]);

helios-oci/src/ports.rs

@@ -1,13 +1,10 @@
 //! Container port publishing configuration (Compose `ports`).
 //!
 //! A [`PortMapping`] holds a single container (target) port using the Compose
-//! long-syntax fields, but serializes as the canonical short-syntax string
-//! `[HOST_IP:][HOST_PORT:]CONTAINER_PORT/PROTOCOL` (protocol always explicit)
-//! so ports stay readable in serialized state. Mappings are kept in a
-//! `BTreeSet` since port order carries no meaning; the set guarantees a
+//! long-syntax fields. Port order carries no meaning,
+//! so mappings are kept sorted by container port then protocol to guarantee a
 //! deterministic serialized form for state comparison.
 
-use std::collections::BTreeSet;
 use std::fmt;
 use std::str::FromStr;
 
@@ -50,22 +47,20 @@ impl FromStr for PortProtocol {
 }
 
 /// A single published container port.
-///
-/// Field declaration order doubles as the derived `Ord` sort key, which
-/// determines the serialized order within a config's port set.
 #[serde_with::skip_serializing_none]
 #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, PartialOrd, Ord)]
 pub struct PortMapping {
     /// Container port
     pub target: u16,
+    /// Port protocol (tcp/udp)
+    #[serde(default)]
+    pub protocol: PortProtocol,
     /// Host port. `None` publishes to an ephemeral port.
     #[serde(default)]
     pub published: Option<u16>,
     /// Host IP to bind to. `None` binds to all interfaces.
     #[serde(default)]
     pub host_ip: Option<String>,
-    #[serde(default)]
-    pub protocol: PortProtocol,
 }
 
 impl PortMapping {
@@ -171,7 +166,7 @@ fn parse_host_ip(s: &str) -> std::result::Result<String, String> {
 /// Build the engine `ExposedPorts` keys and `HostConfig.PortBindings` map
 /// for a container create request. Multiple mappings of the same container
 /// port/protocol are grouped into a single binding list.
-pub(crate) fn to_oci_port_maps(ports: BTreeSet<PortMapping>) -> (Vec<String>, PortMap) {
+pub(crate) fn to_oci_port_maps(ports: Vec<PortMapping>) -> (Vec<String>, PortMap) {
     let mut bindings = PortMap::new();
     for mapping in ports {
         bindings
@@ -189,18 +184,16 @@ pub(crate) fn to_oci_port_maps(ports: BTreeSet<PortMapping>) -> (Vec<String>, Po
 }
 
 /// Read port mappings back from the engine's `HostConfig.PortBindings`,
-/// normalizing an unset host IP/port to absent. This considers both an empty host/IP
-/// or `0.0.0.0` to mean `bind all interfaces` so that gets converted to `None`.
-/// This is to work with different engines
-pub(crate) fn from_oci_port_map(map: PortMap) -> Result<BTreeSet<PortMapping>> {
-    let mut ports = BTreeSet::new();
+/// normalizing empty-string host IP/port (the engine's "unset") to absent.
+pub(crate) fn from_oci_port_map(map: PortMap) -> Result<Vec<PortMapping>> {
+    let mut ports = Vec::new();
     for (key, bindings) in map {
         let (target, protocol) = parse_engine_key(&key)
             .map_err(|e| Error::other(format!("invalid engine port binding `{key}`: {e}")))?;
 
         let bindings = bindings.unwrap_or_default();
         if bindings.is_empty() {
-            ports.insert(PortMapping {
+            ports.push(PortMapping {
                 target,
                 published: None,
                 host_ip: None,
@@ -215,7 +208,7 @@ pub(crate) fn from_oci_port_map(map: PortMap) -> Result<BTreeSet<PortMapping>> {
                 .filter(|ip| !ip.is_empty() && ip != "0.0.0.0");
             match binding.host_port.filter(|port| !port.is_empty()) {
                 None => {
-                    ports.insert(PortMapping {
+                    ports.push(PortMapping {
                         target,
                         published: None,
                         host_ip,
@@ -229,7 +222,7 @@ pub(crate) fn from_oci_port_map(map: PortMap) -> Result<BTreeSet<PortMapping>> {
                         Error::other(format!("invalid engine port binding `{key}`: {e}"))
                     })?;
                     for host_port in published {
-                        ports.insert(PortMapping {
+                        ports.push(PortMapping {
                             target,
                             published: Some(host_port),
                             host_ip: host_ip.clone(),
@@ -240,6 +233,10 @@ pub(crate) fn from_oci_port_map(map: PortMap) -> Result<BTreeSet<PortMapping>> {
             }
         }
     }
+    // `PortMap` is a hash map, so iteration order is unstable. Sort
+    // after the fact to match the canonical order the target state uses,
+    // so a reordering never triggers reconfiguration.
+    ports.sort();
     Ok(ports)
 }
 
@@ -324,7 +321,7 @@ mod tests {
 
     #[test]
     fn to_engine_port_maps_groups_by_container_port() {
-        let ports = BTreeSet::from([
+        let ports = Vec::from([
             mapping("8080:80"),
             mapping("127.0.0.1:8081:80"),
             mapping("53:53/udp"),
@@ -364,10 +361,7 @@ mod tests {
                 host_port: Some("".to_string()),
             }]),
         )]);
-        assert_eq!(
-            from_oci_port_map(map).unwrap(),
-            BTreeSet::from([mapping("80")])
-        );
+        assert_eq!(from_oci_port_map(map).unwrap(), Vec::from([mapping("80")]));
     }
 
     #[test]
@@ -383,7 +377,7 @@ mod tests {
         )]);
         assert_eq!(
             from_oci_port_map(map).unwrap(),
-            BTreeSet::from([mapping("8080:80")])
+            Vec::from([mapping("8080:80")])
         );
     }
 
@@ -399,7 +393,7 @@ mod tests {
         )]);
         assert_eq!(
             from_oci_port_map(map).unwrap(),
-            BTreeSet::from([mapping("8000:80"), mapping("8001:80"), mapping("8002:80"),])
+            Vec::from([mapping("8000:80"), mapping("8001:80"), mapping("8002:80"),])
         );
     }
 
@@ -409,16 +403,18 @@ mod tests {
         let map = PortMap::from([("80/udp".to_string(), None)]);
         assert_eq!(
             from_oci_port_map(map).unwrap(),
-            BTreeSet::from([mapping("80/udp")])
+            Vec::from([mapping("80/udp")])
         );
     }
 
     #[test]
     fn engine_round_trip() {
-        let ports = BTreeSet::from([
+        // In canonical order (sorted by container port then protocol), so the
+        // round trip through the engine representation is the identity.
+        let ports = Vec::from([
+            mapping("53:53/udp"),
             mapping("8080:80"),
             mapping("127.0.0.1:8081:80"),
-            mapping("53:53/udp"),
             mapping("443"),
             mapping("9000:3000"),
         ]);

helios-remote-model/src/service/mod.rs

@@ -696,15 +696,10 @@ mod tests {
         }))
         .unwrap();
         let ports: Vec<&PortMapping> = comp.ports.iter().collect();
+        // Canonicalized by container port then protocol: 53 < 443 < 8080.
         assert_eq!(
             ports,
             vec![
-                &PortMapping {
-                    target: 8080,
-                    published: None,
-                    host_ip: None,
-                    protocol: PortProtocol::Tcp,
-                },
                 &PortMapping {
                     target: 53,
                     published: Some(5353),
@@ -717,6 +712,12 @@ mod tests {
                     host_ip: None,
                     protocol: PortProtocol::Tcp,
                 },
+                &PortMapping {
+                    target: 8080,
+                    published: None,
+                    host_ip: None,
+                    protocol: PortProtocol::Tcp,
+                },
             ]
         );
     }

helios-remote-model/src/service/ports.rs

@@ -15,7 +15,7 @@ use std::fmt;
 use std::net::IpAddr;
 
 /// Transport protocol of a published port.
-#[derive(Debug, Default, Clone, Copy, PartialEq)]
+#[derive(Debug, Default, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
 pub enum PortProtocol {
     #[default]
     Tcp,
@@ -33,15 +33,16 @@ impl PortProtocol {
 }
 
 /// A single published container port, after range expansion.
-#[derive(Debug, PartialEq)]
+#[derive(Debug, PartialEq, Eq, PartialOrd, Ord)]
 pub struct PortMapping {
     /// Container port
     pub target: u16,
+    /// Port protocol
+    pub protocol: PortProtocol,
     /// Host port. `None` publishes to an ephemeral port.
     pub published: Option<u16>,
     /// Host IP to bind to. `None` binds to all interfaces.
     pub host_ip: Option<String>,
-    pub protocol: PortProtocol,
 }
 
 /// Service port mapping set. Ordering carries no meaning, so mappings are
@@ -188,6 +189,12 @@ impl<'de> Deserialize<'de> for Ports {
         for entry in raw {
             ports.extend(parse_raw_port(entry).map_err(serde::de::Error::custom)?);
         }
+
+        // Canonicalize order and drop duplicates so the result is stable
+        // against reorderings in the remote composition.
+        ports.sort();
+        ports.dedup();
+
         Ok(Ports(ports))
     }
 }
@@ -409,11 +416,12 @@ mod tests {
     #[test]
     fn short_form_with_protocol() {
         let p = ports(json!(["6060:6060/udp", "1234:1234/tcp"]));
+        // Canonicalized by container port: 1234 sorts before 6060.
         assert_eq!(
             p.0,
             Vec::from([
-                mapping(6060, Some(6060), None, PortProtocol::Udp),
                 mapping(1234, Some(1234), None, PortProtocol::Tcp),
+                mapping(6060, Some(6060), None, PortProtocol::Udp),
             ])
         );
     }
@@ -629,4 +637,16 @@ mod tests {
         let p = ports(json!([]));
         assert!(p.is_empty());
     }
+
+    #[test]
+    fn duplicate_mappings_are_deduplicated() {
+        let p = ports(json!(["8080:80", "8080:80", "8080:80/udp"]));
+        assert_eq!(
+            p.0,
+            Vec::from([
+                mapping(80, Some(8080), None, PortProtocol::Tcp),
+                mapping(80, Some(8080), None, PortProtocol::Udp),
+            ])
+        );
+    }
 }

helios-state/src/models/service/config.rs

@@ -475,10 +475,7 @@ mod tests {
             ..Default::default()
         };
         let svc = ServiceConfig::from(inspected);
-        assert_eq!(
-            svc.ports,
-            ["8080:80".parse().unwrap()].into_iter().collect()
-        );
+        assert_eq!(svc.ports, Vec::from(["8080:80".parse().unwrap()]));
     }
 
     #[test]

helios-state/src/models/service/mod.rs

@@ -2,7 +2,6 @@ use mahler::state::State;
 use serde::{Deserialize, Serialize};
 
 use crate::labels::LABEL_SERVICE_ID;
-use std::collections::BTreeSet;
 
 use crate::oci::{
     self, BindPropagation, Cgroup, ContainerConfig, DateTime, Healthcheck, LocalContainer, Mount,
@@ -229,9 +228,8 @@ impl From<RemoteServiceTarget> for ServiceTarget {
             })
             .collect();
 
-        // Convert the published ports. Both sides are sets ordered by the
-        // same key, so the conversion preserves the canonical form.
-        let ports: BTreeSet<PortMapping> = composition
+        // Convert the published ports.
+        let ports = composition
             .ports
             .into_iter()
             .map(|p| PortMapping {