Ping to check connection #84

kathrynfejer · 2019-07-15T20:48:10Z

Currently enftun can determine if there has been a connectivity change on the client's end and reconnects, but if the router or ENF is down, enftun does not recognize it for some time. By pinging the network we are connected to at configurable intervals, we can monitor our connectivity and increase availability.

This pings the network at the configured timeouts and whenever a new address or new link netlink event occurs. If one of these two occurs at the same time or within the reply timeout, only one echo request is sent.

Fixes #6 and Connected to #7

drbild

This is looking good, but I think redefining the responsibilities of the various components (enftun_conn_state, enftun_echo, enftun_tls, and enftun_netlink) can make the code more clear.

enftun_tls is now aware of the conn_state internals to call things like tls->conn_state->echo.send_ping and tls->conn_state->send_request_cb(...). This tight coupling makes the code hard to reason about (changing the internal of conn_state might break the tls internals).

Instead, let's look for a more generic mechanism. One option is to define a callbacktypedef void (*enftun_tls_on_receive )(). conn_state can register a function of this type with tls, which call it everytime a packet is received. (It would actually be better to define this on the channel, so that is could be used for any transport, not just tls.)

However, we already have a generic mechanism to examine incoming packets --- the ingress chain. conn_state can register a handler at the head of the ingress chain. It will be called for each incoming packet. Rather than parsing or consuming the packet, it can just reset the echo request timer.
The enftun_echo module is fairly anemic. This isn't necessarily a bad thing. However, here it can lead to some problems.

For example, it is not safe to call the enftun_echo_send_request function if there is already an outstanding echo request queued. However, this is not enforced. The caller must track that state and manage it. If there are multiple callers, who knows...

Extending the responsibilities of this module and making the public API more high-level will help. Specifically, move the request and reply timers out of conn_state into this module.

So this module becomes responsible for sending heartbeats periodically and on-demand, waiting for a reply to the outstanding heartbeat, and notifying a registered callback if a heartbeat reply is not received. Renaming to enftun_heartbeat will better reflect this expanding functionality.
- enftun_heartbeat_init(..., int (*on_timeout)() ) - on_timeout is called if a echo reply is not received
- enftun_heartbeat_start(...) - start sending heartbeats periodically
- enftun_heartbeat_restart(...) - reset the timer for periodic heartbeats
- enftun_heartbeat_stop(...) - stop sending heartbeats; cancel any outstanding crbs.
- enftun_heartbeat_now(...) - send a heartbeat immediately (or schedule to send one, if one is already in flight)
The enftun_netlink module is now aware of the heartbeat stuff and has to return a special error code NEEDS_PING. That sort of coupling belongs in the conn_state module, which ties everything together. enftun_netlink_read_message is not longer properly returning the number of bytes read, etc.

I suggest not modifying enftun_netlink_read_message. Instead, add helper function(s) in conn_state for parsing the message. conn_state can call both enftun_netlink_read_message and the helper functions to determine if it wants to do something with the message. The helper function(s) will need to #include the netlink headers, but that's ok.

src/config.c

src/icmp.c

src/echo.h

src/icmp.c

src/ip.h

drbild · 2019-08-05T22:19:49Z

src/heartbeat.c

+send_heartbeat(struct enftun_heartbeat* heartbeat)
+{
+    heartbeat->hb_inflight    = true;
+    heartbeat->reply_recieved = false;


recieved -> received

drbild · 2019-08-12T18:34:21Z

src/heartbeat.c

+    }
+
+    heartbeat->hb_inflight = false;
+    enftun_log_info("Recieved ping reply\n");


Make this enftun_log_debug.

drbild · 2019-08-12T18:37:47Z

src/heartbeat.h

+{
+    struct enftun_channel* chan;
+
+    const struct in6_addr* addr;


Instead of addr, call this source or source_addr.

Also, add a similar member,dest or dest_addr.

Also, change the dest address from all_routers/nodes to fe80::1.

drbild · 2019-08-12T18:38:13Z

src/heartbeat.h

+enftun_heartbeat_init(struct enftun_heartbeat* heartbeat,
+                      uv_loop_t* loop,
+                      struct enftun_channel* chan,
+                      const struct in6_addr* ipv6,


Instead of ipv6, call this source or source_addr. Also, add the dest or dest_addr parameter.

drbild · 2019-08-12T18:50:09Z

src/heartbeat.c

+    if (!icmph)
+        goto pass;
+
+    heartbeat->reply_recieved = true;


Need to cancel the reply timer here and start the request timer again.

drbild · 2019-08-12T20:13:54Z

src/heartbeat.c

+{
+    if (crb->status)
+        enftun_log_error("PING: failed to send heartbeat reply: %d\n",
+                         crb->status);


Rather than ignoring the error, we should have enftun reconnect.

drbild · 2019-08-16T18:36:06Z

@kathrynfejer Could you please rebase this on the latest master (with the just merged psock stuff).

-ping the network every 12 seconds, waits for a response -after 3 seconds, if no response, reconnect

drbild

A few comments, but this is looking pretty good.

Unfortunately, I don't have time to fully review the main reconnect logic today, to get this merged. Could you open three new PRs with the following subsets of changes --- those I can get merged today.

Just the change to reserve 2 bytes in the enftun_packet_reset, rather than in enftun_crb_read.
Just the addition of icmp6 echo request/reply creating and parsing (enftun_icmp6_echo_reply_pull and enftun_icmp6_echo_request in icmp.h/c).
Just the addition of the heartbeat service (heartbeat.h,c and the associated parameters in config.h/c).

Those three I can approve and merge today.

Then a final PR with the changes to conn_state.c, ip.h,c and enftun.c. That one won't get merged until next week, when I can spend more time reviewing the logic in conn_state.

drbild · 2019-08-16T18:52:04Z

src/enftun.c

-                                        ctx->config.fwmark)) < 0)
+    if ((rc = enftun_conn_state_prepare(
+                  &ctx->conn_state, &ctx->loop, trigger_reconnect, (void*) ctx,
+                  ctx->config.fwmark, &ctx->tlschan, &ctx->ipv6, &ip6_self,


ip6_self has the correct address, has a misleading name.

Instead, let's create a new constant ip6_enf_router in ip.h,c with the same value and use it instead. Add a comment stating that enftun and the ENF router share the same IP since they split the duties of a traditional IPv6 router.

drbild · 2021-03-19T16:10:17Z

src/conn_state.c

@@ -102,6 +120,10 @@ enftun_conn_state_prepare(struct enftun_conn_state* conn_state,
 int
 enftun_conn_state_close(struct enftun_conn_state* conn_state)
 {
+    int rc = enftun_heartbeat_free(&conn_state->heartbeat);
+    if (0 != rc)


Still need to close netlink, even if the hearbeat free fails. Just log a warning and continue.

drbild · 2021-03-24T23:51:08Z

Replaced by #137. The core of this PR was cherry-picked into it.

kathrynfejer requested a review from drbild July 15, 2019 20:48

kathrynfejer force-pushed the enftun-ping branch from d469853 to 7e3ee49 Compare July 16, 2019 16:34

drbild suggested changes Jul 16, 2019

View reviewed changes

kathrynfejer force-pushed the enftun-ping branch 3 times, most recently from 9bd260f to ff2eea9 Compare July 22, 2019 18:00

drbild self-requested a review August 5, 2019 16:02

kathrynfejer force-pushed the enftun-ping branch from b17ae2a to 4ca1887 Compare August 6, 2019 16:36

drbild suggested changes Aug 12, 2019

View reviewed changes

drbild reviewed Aug 12, 2019

View reviewed changes

kathrynfejer mentioned this pull request Aug 15, 2019

Improve Error Handling #89

Open

kathrynfejer force-pushed the enftun-ping branch from 259c862 to 506d13e Compare August 16, 2019 16:05

kathrynfejer requested a review from drbild August 16, 2019 18:32

kathrynfejer added 4 commits August 16, 2019 13:40

Add basic timer loop to ping network

f4b0bc2

-ping the network every 12 seconds, waits for a response -after 3 seconds, if no response, reconnect

Ping on netlink events and reset timer on real traffic

fcf792c

generalize reconnect callback function

c01a1db

Restart ping timer as soon as reply is received

16a0ab6

kathrynfejer force-pushed the enftun-ping branch from 506d13e to 16a0ab6 Compare August 16, 2019 18:41

drbild suggested changes Aug 16, 2019

View reviewed changes

add separate ip address for the ENF router

dc5e34c

kathrynfejer force-pushed the enftun-ping branch from 40d58e2 to dc5e34c Compare August 16, 2019 19:37

drbild reviewed Mar 19, 2021

View reviewed changes

drbild mentioned this pull request Mar 24, 2021

Enable heartbeats to check TLS tunnel status #137

Merged

drbild closed this Mar 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ping to check connection #84

Ping to check connection #84

kathrynfejer commented Jul 15, 2019

drbild left a comment •

edited

Loading

drbild Aug 5, 2019

drbild Aug 12, 2019

drbild Aug 12, 2019

drbild Aug 12, 2019

drbild Aug 12, 2019

drbild Aug 12, 2019

drbild Aug 12, 2019

drbild commented Aug 16, 2019

drbild left a comment

drbild Aug 16, 2019

drbild Mar 19, 2021

drbild commented Mar 24, 2021

Ping to check connection #84

Ping to check connection #84

Conversation

kathrynfejer commented Jul 15, 2019

drbild left a comment • edited Loading

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

drbild commented Aug 16, 2019

drbild left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drbild commented Mar 24, 2021

drbild left a comment •

edited

Loading