feat(rust): added support for multiple concurrent routes to tcp inlets #8832

davide-baldo · 2025-02-28T13:07:21Z

This PR adds the possibility of multiple MultiAddr to the inlets.

There are few new parameters:

--ping-timeout, this is a proxy for ping interval and gets divided by the max. amount of failed pings
--target-redundancy, the amount of redundant routes instantiated, 0 means only one route will be instantiated, can still be specified when a single MultiAddr since dns could resolve into multiple IP addresses. By default it inherits the number of --to parameters.
--to can now be passed multiple times

A single MultiAddr can turn into multiple variants:
/project/default/... => /dnsaddr/xx.projects.orchestrator.io/... => [ /ip4/1.1.1.1/..., /ip4/2.2.2.2/...]

A session is created for each route (target redundancy+1) , every time it reconnects it selects a MultiAddr from the selector, making sure every MultiAddr variant gets used(unless redundancy+1 > available variants).
When DNS fails (or return empty) and no other MultiAddr is available, the session will retry every 15 seconds.

The inlet port will be bind upon creation, when a client connection reaches the inlet, a random route will be picked from the available ones, if no route is available it'll wait until one is available.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot

Pull Request Overview

This PR adds support for multiple concurrent routes to TCP inlets, enabling the use of multiple MultiAddr values along with new parameters for target redundancy and ping timeouts. Key changes include updating CLI parameters and models, modifying repository and registry types to handle multiple addresses, and adjusting connection logic and API endpoints accordingly.

Updated access control, CLI, and storage modules to support multiple outlet addresses.
Refactored InletStatus to InletStatusView with active route tracking.
Modified connection instantiation and secure channel logic, along with minor logging and debug improvements.

Reviewed Changes

Copilot reviewed 53 out of 53 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
implementations/rust/ockam/ockam_api/src/nodes/service/tcp_inlets/access_control.rs	Introduces optional authority handling using original MultiAddr.
implementations/rust/ockam/ockam_api/src/cli_state/storage/tcp_portals_repository_sql.rs	Adjusts binding of outlet addresses by joining them.
implementations/rust/ockam/ockam_api/src/control_api/backend/inlet.rs	Adds validation for empty `to` and manages multiaddresses properly.
implementations/rust/ockam/ockam_api/src/nodes/registry.rs	Renames InletInfo to TcpInletHandle and updates related fields.
implementations/rust/ockam/ockam_api/src/nodes/service/in_memory_node.rs	Updates stop method to gracefully terminate inlets and relays.
implementations/rust/ockam/ockam_api/src/control_api/protocol/inlet.rs	Transitions to InletStatusView with active_routes and target redundancy.
implementations/rust/ockam/ockam_api/src/nodes/models/portal.rs	Updates CreateInlet and InletStatus (now InletStatusView) models to handle multiple routes.
implementations/rust/ockam/ockam_api/src/nodes/connection/mod.rs	Revises multiaddr matching logic in ConnectionBuilder.
implementations/rust/ockam/ockam_api/src/influxdb/portal.rs	Adjusts outlet address handling for InfluxDB portals.
implementations/rust/ockam/ockam_api/src/nodes/connection/plain_tcp.rs, plain_udp.rs	Adds Debug derives for instantiators.
implementations/rust/ockam/ockam_api/src/nodes/service/tcp_inlets/background_node_client.rs	Updates payload creation to support multiple outlet addresses.
implementations/rust/ockam/ockam_api/src/nodes/service/kafka_services.rs	Changes outlet address handling in Kafka services.
implementations/rust/ockam/ockam_api/src/nodes/connection/project.rs	Adds Debug derive for project instantiator.
implementations/rust/ockam/ockam_api/src/nodes/service/relay.rs	Updates Session::create invocation to include new parameters.
implementations/rust/ockam/ockam_api/src/cli_state/storage/tcp_portals_repository.rs	Adapts TcpInlet definition to manage a list of outlet addresses.
implementations/rust/ockam/ockam_api/src/nodes/connection/secure.rs	Enhances debug logging for secure channel instantiator.
implementations/rust/ockam/ockam_api/src/kafka/inlet_controller.rs	Updates outlet address handling for Kafka inlet payloads.
implementations/rust/ockam/ockam_api/src/nodes/models/node.rs	Reflects inlet changes using the new InletStatusView.
implementations/rust/ockam/ockam_api/src/cli_state/tcp_portals.rs	Adjusts CLI state to persist TcpInlet with multiple outlet addresses.

implementations/rust/ockam/ockam_api/src/nodes/connection/mod.rs

implementations/rust/ockam/ockam_api/src/cli_state/storage/tcp_portals_repository.rs

adrianbenavides · 2025-03-18T09:56:40Z

implementations/rust/ockam/ockam_api/src/control_api/protocol/inlet.rs

@@ -13,6 +13,10 @@ fn retry_wait_default() -> u64 {
    20000
 }

+fn ping_timeout_default() -> u64 {


Why not use a Duration instead? At least I'd include the units I the function name. If it's because of some API arguments constraints the duration parser we use in the command?

Overall, I took a more "classical" API approach, where the API is low-level as possible, and everything else can be added on top via libraries.
Given the last meeting, I probably took the wrong approach and we should lift the API level.

Duration is a part of core so I don't mind using it everywhere where applicable

implementations/rust/ockam/ockam_api/src/control_api/protocol/inlet.rs

implementations/rust/ockam/ockam_api/src/influxdb/portal.rs

implementations/rust/ockam/ockam_api/src/nodes/models/portal.rs

implementations/rust/ockam/ockam_transport_tcp/src/portal/inlet_shared_state.rs

implementations/rust/ockam/ockam_transport_tcp/src/transport/portals.rs

implementations/rust/ockam/ockam_api/src/nodes/service/tcp_inlets/node_manager.rs

implementations/rust/ockam/ockam_api/src/nodes/service/tcp_inlets/session_replacer/mod.rs

implementations/rust/ockam/ockam_api/src/nodes/service/tcp_inlets/session_replacer/selector.rs

implementations/rust/ockam/ockam_api/src/nodes/service/tcp_inlets/session_replacer/mod.rs

adrianbenavides · 2025-03-20T07:40:08Z

I checked that the replacer's messages are OK, and they look good. One difference I noticed is that the inlet now reacts almost instantly when it loses the connection (compared to the ~30 seconds we had before):

SanjoDeundiak

This is a big PR, could only consume a part of it so far. Submitting what I have

SanjoDeundiak · 2025-03-20T16:53:39Z

implementations/rust/ockam/ockam_api/src/cli_state/storage/tcp_portals_repository.rs

    alias: String,
    privileged: bool,
 }

 impl TcpInlet {
    pub fn new(
        bind_addr: &SocketAddr,
-        outlet_addr: &MultiAddr,
+        outlet_addresses: &[MultiAddr],


NIT: Let's make it a Vector here

SanjoDeundiak · 2025-03-20T16:55:05Z

implementations/rust/ockam/ockam_api/src/cli_state/storage/tcp_portals_repository.rs

    }

-    pub fn outlet_addr(&self) -> MultiAddr {
-        self.outlet_addr.clone()
+    pub fn outlet_addresses(&self) -> &Vec<MultiAddr> {


NIT: I would return a slice here, unless a caller of that function rely on the fact that it's a Vec (I can't think of such case)

SanjoDeundiak · 2025-03-20T16:56:21Z

implementations/rust/ockam/ockam_api/src/cli_state/storage/tcp_portals_repository.rs

    }

-    pub fn alias(&self) -> String {
-        self.alias.clone()
+    pub fn alias(&self) -> &str {


SanjoDeundiak · 2025-03-20T17:05:00Z

implementations/rust/ockam/ockam_api/src/cli_state/storage/tcp_portals_repository_sql.rs

+                .outlet_addresses()
+                .iter()
+                .map(|x| x.to_string())
+                .join("//"),


NIT: A small comment would be great, but overall was easy to understand

SanjoDeundiak · 2025-03-20T17:06:09Z

implementations/rust/ockam/ockam_api/src/cli_state/storage/tcp_portals_repository_sql.rs

+    fn outlet_addresses(&self) -> Result<Vec<MultiAddr>> {
+        let mut multiaddresses = Vec::new();
+
+        for addr in self.outlet_addr.split("//") {


Could that be shrank into self.outlet_addr.split("//").map()? Or is error handling ruining it?

SanjoDeundiak · 2025-03-20T18:47:46Z

...entations/rust/ockam/ockam_transport_tcp/src/privileged_portal/workers/internal_processor.rs

@@ -88,11 +79,6 @@ impl Processor for InternalProcessor {
                            raw_socket_read_result.ipv4_info.source_ip(),
                            raw_socket_read_result.tcp_info.source_port(),
                        );
-
-                        if connection.their_identifier != inlet_shared_state.their_identifier() {


Are you removing that check just because it's checked in a different place, or some other reason?

SanjoDeundiak · 2025-03-20T19:08:42Z

implementations/rust/ockam/ockam_api/src/nodes/service/tcp_inlets/node_manager.rs

+
+            let mut session =
+                Session::create(ctx, replacer, additional_session_options, ping_timeout)?;
+            session.start_monitoring()?;


Does it make sense to start with a first session instead?

SanjoDeundiak · 2025-03-20T19:33:01Z

implementations/rust/ockam/ockam_transport_tcp/src/portal/inlet_shared_state.rs

+            }
+            // let's wait until something changes and try again
+            debug!("No active route found for the inlet, waiting for a connection");
+            self.notify.notified().await;


I think for both regular and privileged portals choose_active_route would better return an error instead of waiting for a connection. WDYT?

SanjoDeundiak · 2025-03-20T19:37:24Z

...entations/rust/ockam/ockam_transport_tcp/src/privileged_portal/workers/internal_processor.rs

@@ -122,15 +110,15 @@ impl Processor for InternalProcessor {

                let portal_packet = OckamPortalPacket::from_tcp_packet(
                    connection.connection_identifier.clone(),
-                    inlet_shared_state.route_index(),
+                    connection.inlet_route_state.route_index(),


It uses locking internally and locks again to get the route ~10 lines below that. That's a small performance penalty, but also brings a possibility of a data race.

SanjoDeundiak · 2025-03-20T19:39:46Z

implementations/rust/ockam/ockam_transport_tcp/tests/portal.rs

    Ok(())
 }

 #[allow(non_snake_case)]
-#[ockam_macros::test(timeout = 5000)]
+#[ockam_macros::test(timeout = 5000_0000)]


Probably left from debugging

davide-baldo force-pushed the davide-baldo/ha-inlet-communication branch 7 times, most recently from 1a7d9ea to 1087b99 Compare March 4, 2025 15:05

davide-baldo marked this pull request as ready for review March 4, 2025 15:22

davide-baldo requested a review from a team as a code owner March 4, 2025 15:22

mrinalwadhwa requested a review from Copilot March 4, 2025 16:02

Copilot AI reviewed Mar 4, 2025

View reviewed changes

mrinalwadhwa requested a review from Copilot March 4, 2025 16:17

Copilot AI reviewed Mar 4, 2025

View reviewed changes

davide-baldo marked this pull request as draft March 4, 2025 18:51

davide-baldo force-pushed the davide-baldo/ha-inlet-communication branch 3 times, most recently from 47b7701 to 57004e0 Compare March 12, 2025 15:36

davide-baldo requested a review from Copilot March 12, 2025 17:16

Copilot AI reviewed Mar 12, 2025

View reviewed changes

davide-baldo requested a review from Copilot March 12, 2025 17:23

Copilot AI reviewed Mar 12, 2025

View reviewed changes

implementations/rust/ockam/ockam_api/src/nodes/connection/mod.rs Show resolved Hide resolved

davide-baldo force-pushed the davide-baldo/ha-inlet-communication branch 2 times, most recently from 4c380e1 to 418cf46 Compare March 17, 2025 18:55

davide-baldo marked this pull request as ready for review March 18, 2025 09:37

adrianbenavides reviewed Mar 18, 2025

View reviewed changes

implementations/rust/ockam/ockam_api/src/nodes/service/tcp_inlets/session_replacer/selector.rs Show resolved Hide resolved

adrianbenavides reviewed Mar 18, 2025

View reviewed changes

implementations/rust/ockam/ockam_api/src/nodes/service/tcp_inlets/session_replacer/mod.rs Show resolved Hide resolved

feat(rust): added support for multiple concurrent routes to tcp inlets

bcd6803

davide-baldo force-pushed the davide-baldo/ha-inlet-communication branch from 418cf46 to bcd6803 Compare March 19, 2025 18:21

SanjoDeundiak requested changes Mar 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rust): added support for multiple concurrent routes to tcp inlets #8832

feat(rust): added support for multiple concurrent routes to tcp inlets #8832

davide-baldo commented Feb 28, 2025 •

edited

Loading

Copilot AI left a comment

adrianbenavides Mar 18, 2025

davide-baldo Mar 19, 2025

SanjoDeundiak Mar 20, 2025

adrianbenavides commented Mar 20, 2025

SanjoDeundiak left a comment

SanjoDeundiak Mar 20, 2025

SanjoDeundiak Mar 20, 2025

SanjoDeundiak Mar 20, 2025

SanjoDeundiak Mar 20, 2025

SanjoDeundiak Mar 20, 2025

SanjoDeundiak Mar 20, 2025

SanjoDeundiak Mar 20, 2025

SanjoDeundiak Mar 20, 2025

SanjoDeundiak Mar 20, 2025

SanjoDeundiak Mar 20, 2025

feat(rust): added support for multiple concurrent routes to tcp inlets #8832

Are you sure you want to change the base?

feat(rust): added support for multiple concurrent routes to tcp inlets #8832

Conversation

davide-baldo commented Feb 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrianbenavides commented Mar 20, 2025

SanjoDeundiak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davide-baldo commented Feb 28, 2025 •

edited

Loading