gcoap_forward_proxy: send empty ACK when response takes too long #18386

miri64 · 2022-08-01T11:58:22Z

Contribution description

This allows a forward proxy to ACK a request early, if the response from upstream takes too long. This is based on the work by @chrysn for https://doi.org/10.1145/3405656.3418718 which is why I gave the main authorship of the commit to him. I merely adopted the state of change to the best practices and changes to forward_proxy in current master and optimized the client_ep_t to keep the increase in RAM-usage minimal (compared to just plane using uint8_ts, I was able to safe 3 bytes per entry, compared to using bitmaps, I was able to safe 16 bytes of ROM with my flag-based approach).

Testing procedure

I used a version of my trusty proxy testing script again:

#! /usr/bin/env python3
import subprocess
import sys
import time

from riotctrl.ctrl import RIOTCtrl

sys.path.append("dist/pythonlibs")
from riotctrl_shell.netif import IfconfigListParser, Ifconfig


def set_proxy(client, proxy):
    netifs = IfconfigListParser().parse(proxy.ifconfig_list())
    proxy_addr = list(netifs.values())[0]["ipv6_addrs"][0]["addr"]
    res = client.cmd(f"coap proxy set {proxy_addr} 5683")
    assert f"coap proxy set {proxy_addr} 5683" in res


def test_proxy(modules=["gcoap_forward_proxy"]):
    RIOTCtrl.MAKE_ARGS = ("-j", "--no-print-directory")
    envs = [
        {"PORT": "tap0", "QUIETER": "1"},
        {
            "USEMODULE": " ".join(modules),
            "PORT": "tap1",
            "BINDIRBASE": "proxy-bin",
            "QUIETER": "1",
        },
        {"PORT": "tap2", "QUIETER": "1"},
    ]
    ctrls = [RIOTCtrl(application_directory="examples/gcoap", env=env) for env in envs]
    ctrls[0].flash(stdout=None, stderr=None)
    ctrls[1].flash(stdout=None, stderr=None)
    with (
        ctrls[0].run_term(reset=False),
        ctrls[1].run_term(reset=False),
        ctrls[2].run_term(reset=False),
    ):
        shells = []
        for ctrl in ctrls:
            ctrl.term.logfile = sys.stdout
            shells.append(Ifconfig(ctrl))
        netifs = IfconfigListParser().parse(shells[0].ifconfig_list())
        server_addr = list(netifs.values())[0]["ipv6_addrs"][0]["addr"]
        set_proxy(shells[2], shells[1])
        shells[2].cmd(f"coap get -c {server_addr} 5683 /.well-known/core")
        # comes in asynchronously, so we need to expect instead of checking result
        shells[2].riotctrl.term.expect_exact(
            '</cli/stats>;ct=0;rt="count";obs,</riot/board>', timeout=10
        )
        for shell in reversed(shells):
            shell.cmd("help")


if __name__ == "__main__":
    test_proxy()
    test_proxy(["gcoap_forward_proxy", "nanocoap_cache"])
    print("")

To induce a delayed response from the server, I patched the examples/gcoap application:

diff --git a/examples/gcoap/server.c b/examples/gcoap/server.c
index bf2315cd01..da5f106747 100644
--- a/examples/gcoap/server.c
+++ b/examples/gcoap/server.c
@@ -30,2 +30,3 @@
 #include "od.h"
+#include "ztimer.h"
 
@@ -88,2 +89,3 @@ static ssize_t _encode_link(const coap_resource_t *resource, char *buf,
     ssize_t res = gcoap_encode_link(resource, buf, maxlen, context);
+    ztimer_sleep(ZTIMER_MSEC, 2100);
     if (res > 0) {

When running with 3 TAP interfaces, the output is as follows

Building application "gcoap_example" for "native" with MCU "native".

   text	   data	    bss	    dec	    hex	filename
 196251	   2020	  90280	 288551	  46727	/home/mlenders/Repositories/RIOT-OS/RIOT/examples/gcoap/bin/native/gcoap_example.elf
Building application "gcoap_example" for "native" with MCU "native".

   text	   data	    bss	    dec	    hex	filename
 204263	   2052	  90536	 296851	  48793	/home/mlenders/Repositories/RIOT-OS/RIOT/examples/gcoap/proxy-bin/native/gcoap_example.elf
/home/mlenders/Repositories/RIOT-OS/RIOT/examples/gcoap/bin/native/gcoap_example.elf tap0 
RIOT native interrupts/signals initialized.
RIOT native board initialized.
RIOT native hardware initialization complete.

main(): This is RIOT! (Version: 2022.10-devel-147-gda5cd-gcoap_forward_proxy/enh/empty-ack)
gcoap example app
All up, running the shell now
> 

> ifconfig
ifconfig
Iface  6  HWaddr: E2:BC:7D:CB:F5:50 
          L2-PDU:1500  MTU:1500  HL:64  Source address length: 6
          Link type: wired
          inet6 addr: fe80::e0bc:7dff:fecb:f550  scope: link  VAL
          inet6 group: ff02::1
          inet6 group: ff02::1:ffcb:f550
          
> /home/mlenders/Repositories/RIOT-OS/RIOT/examples/gcoap/proxy-bin/native/gcoap_example.elf tap1 
RIOT native interrupts/signals initialized.
RIOT native board initialized.
RIOT native hardware initialization complete.

main(): This is RIOT! (Version: 2022.10-devel-147-gda5cd-gcoap_forward_proxy/enh/empty-ack)
gcoap example app
All up, running the shell now
> 

> ifconfig
ifconfig
Iface  6  HWaddr: DA:27:1D:A8:64:24 
          L2-PDU:1500  MTU:1500  HL:64  Source address length: 6
          Link type: wired
          inet6 addr: fe80::d827:1dff:fea8:6424  scope: link  VAL
          inet6 group: ff02::1
          inet6 group: ff02::1:ffa8:6424
          
> /home/mlenders/Repositories/RIOT-OS/RIOT/examples/gcoap/bin/native/gcoap_example.elf tap2 
RIOT native interrupts/signals initialized.
RIOT native board initialized.
RIOT native hardware initialization complete.

main(): This is RIOT! (Version: 2022.10-devel-147-gda5cd-gcoap_forward_proxy/enh/empty-ack)
gcoap example app
All up, running the shell now
> 

> coap proxy set fe80::d827:1dff:fea8:6424 5683
coap proxy set fe80::d827:1dff:fea8:6424 5683
> coap get -c fe80::e0bc:7dff:fecb:f550 5683 /.well-known/core
coap get -c fe80::e0bc:7dff:fecb:f550 5683 /.well-known/core
gcoap_cli: sending msg ID 58505, 65 bytes
> gcoap: response Success, code 2.05, 46 bytes
</cli/stats>;ct=0;rt="count";obs,</riot/board>
help
help
Command              Description
---------------------------------------
coap                 CoAP example
ifconfig             Configure network interfaces
nib                  Configure neighbor information base
ping                 Alias for ping6
ping6                Ping via ICMPv6
pm                   interact with layered PM subsystem
ps                   Prints information about running threads.
reboot               Reboot the node
version              Prints current RIOT_VERSION
> help
help
Command              Description
---------------------------------------
coap                 CoAP example
ifconfig             Configure network interfaces
nib                  Configure neighbor information base
ping                 Alias for ping6
ping6                Ping via ICMPv6
pm                   interact with layered PM subsystem
ps                   Prints information about running threads.
reboot               Reboot the node
version              Prints current RIOT_VERSION
> help
help
Command              Description
---------------------------------------
coap                 CoAP example
ifconfig             Configure network interfaces
nib                  Configure neighbor information base
ping                 Alias for ping6
ping6                Ping via ICMPv6
pm                   interact with layered PM subsystem
ps                   Prints information about running threads.
reboot               Reboot the node
version              Prints current RIOT_VERSION
> Building application "gcoap_example" for "native" with MCU "native".

   text	   data	    bss	    dec	    hex	filename
 196251	   2020	  90280	 288551	  46727	/home/mlenders/Repositories/RIOT-OS/RIOT/examples/gcoap/bin/native/gcoap_example.elf
Building application "gcoap_example" for "native" with MCU "native".

   text	   data	    bss	    dec	    hex	filename
 213361	   2156	  92680	 308197	  4b3e5	/home/mlenders/Repositories/RIOT-OS/RIOT/examples/gcoap/proxy-bin/native/gcoap_example.elf
/home/mlenders/Repositories/RIOT-OS/RIOT/examples/gcoap/bin/native/gcoap_example.elf tap0 
RIOT native interrupts/signals initialized.
RIOT native board initialized.
RIOT native hardware initialization complete.

main(): This is RIOT! (Version: 2022.10-devel-147-gda5cd-gcoap_forward_proxy/enh/empty-ack)
gcoap example app
All up, running the shell now
> 

> ifconfig
ifconfig
Iface  6  HWaddr: E2:BC:7D:CB:F5:50 
          L2-PDU:1500  MTU:1500  HL:64  Source address length: 6
          Link type: wired
          inet6 addr: fe80::e0bc:7dff:fecb:f550  scope: link  VAL
          inet6 group: ff02::1
          inet6 group: ff02::1:ffcb:f550
          
> /home/mlenders/Repositories/RIOT-OS/RIOT/examples/gcoap/proxy-bin/native/gcoap_example.elf tap1 
RIOT native interrupts/signals initialized.
RIOT native board initialized.
RIOT native hardware initialization complete.

main(): This is RIOT! (Version: 2022.10-devel-147-gda5cd-gcoap_forward_proxy/enh/empty-ack)
gcoap example app
All up, running the shell now
> 

> ifconfig
ifconfig
Iface  6  HWaddr: DA:27:1D:A8:64:24 
          L2-PDU:1500  MTU:1500  HL:64  Source address length: 6
          Link type: wired
          inet6 addr: fe80::d827:1dff:fea8:6424  scope: link  VAL
          inet6 group: ff02::1
          inet6 group: ff02::1:ffa8:6424
          
> /home/mlenders/Repositories/RIOT-OS/RIOT/examples/gcoap/bin/native/gcoap_example.elf tap2 
RIOT native interrupts/signals initialized.
RIOT native board initialized.
RIOT native hardware initialization complete.

main(): This is RIOT! (Version: 2022.10-devel-147-gda5cd-gcoap_forward_proxy/enh/empty-ack)
gcoap example app
All up, running the shell now
> 

> coap proxy set fe80::d827:1dff:fea8:6424 5683
coap proxy set fe80::d827:1dff:fea8:6424 5683
> coap get -c fe80::e0bc:7dff:fecb:f550 5683 /.well-known/core
coap get -c fe80::e0bc:7dff:fecb:f550 5683 /.well-known/core
gcoap_cli: sending msg ID 48175, 65 bytes
> gcoap: response Success, code 2.05, 46 bytes
</cli/stats>;ct=0;rt="count";obs,</riot/board>
help
help
Command              Description
---------------------------------------
coap                 CoAP example
ifconfig             Configure network interfaces
nib                  Configure neighbor information base
ping                 Alias for ping6
ping6                Ping via ICMPv6
pm                   interact with layered PM subsystem
ps                   Prints information about running threads.
reboot               Reboot the node
version              Prints current RIOT_VERSION
> help
help
Command              Description
---------------------------------------
coap                 CoAP example
ifconfig             Configure network interfaces
nib                  Configure neighbor information base
ping                 Alias for ping6
ping6                Ping via ICMPv6
pm                   interact with layered PM subsystem
ps                   Prints information about running threads.
reboot               Reboot the node
version              Prints current RIOT_VERSION
> help
help
Command              Description
---------------------------------------
coap                 CoAP example
ifconfig             Configure network interfaces
nib                  Configure neighbor information base
ping                 Alias for ping6
ping6                Ping via ICMPv6
pm                   interact with layered PM subsystem
ps                   Prints information about running threads.
reboot               Reboot the node
version              Prints current RIOT_VERSION
>

With this PR there will be empty ACKs sent by both the proxy and the client (the latter sends it in response to the CON response by the proxy):

On master, there are no empty ACKs, but retransmissions by both the client and the proxy, even though the request should have reached the proxy already (illustrated by the fact, that in the second run, the proxy sends its retransmission before the client):

Issues/PRs references

benpicco

Looks good, needs a rebase

sys/net/application_layer/gcoap/forward_proxy.c

sys/net/application_layer/gcoap/gcoap.c

miri64 · 2022-10-12T14:16:39Z

Rebased and squashed

riot-ci · 2022-10-12T14:25:11Z

Murdock results

✔️ PASSED

63c4fe5 gcoap: fix "line is longer than 100 characters" pointed out by Vera++

Success	Failures	Total	Runtime
1980	0	1980	06m:50s

Artifacts

Documentation preview

This only reflects a subset of all builds from https://ci-prod.riot-os.org. Please refer to https://ci.riot-os.org for a complete build for now.

miri64 · 2022-10-12T14:34:45Z

Test fail seems unrelated..

benpicco

Code looks good, I trust your testing.

btw: That test script of yours could also be run by Murdock if you replace netdev_tap with socket_zep - see e.g. tests/gcoap_fileserver

sys/net/application_layer/gcoap/forward_proxy.c

miri64 · 2022-10-12T15:22:57Z

btw: That test script of yours could also be run by Murdock if you replace netdev_tap with socket_zep - see e.g. tests/gcoap_fileserver

Mhhh it somewhat relies on someone™ checking if the proxy actually sent an empty ACK. Since this requires a sniffer i.e. root permissions, I don't think having thing run on Murdock (beyond simply checking forward proxy functionality) for now, does not make sense in the scope of this PR.

Off-topic BTW: there are some quality defects with the way you defined the native rules with these kinds of tests, which

make the test fail when trying to run make test with any other board other than native (e.g. when running compile_and_test_for_board.py but also any unsuspecting user wanting to run the test on their board) and
mess up the dependency resolution for that application. Any board dependent build config should go into the application's Makefile.board.dep, so it will only be executed during the dependency resolution phase.

See also this discussion: #18722 (comment)

Co-Authored-By: Martine S. Lenders <m.lenders@fu-berlin.de> Signed-off-by: Martine Lenders <m.lenders@fu-berlin.de>

benpicco · 2022-10-12T15:35:33Z

arg, just squash directly

Since this requires a sniffer i.e. root permissions, I don't think having thing run on Murdock

Since with socket_zep all traffic is encapsulated in UDP and send via the ZEP dispatcher, you can just ask the dispatcher to also forward you all the traffic. No need for root permissions.

make the test fail when trying to run make test with any other board other than native (e.g. when running compile_and_test_for_board.py but also any unsuspecting user wanting to run the test on their board)

Maybe compile_and_test_for_board.py should learn about TEST_ON_CI_WHITELIST 😉
But tbh this script is questionable at best. Most of the tests that can be run without user interaction are software only, so not board dependent at all. While all the 'interesting tests' require to bridge some pins or inspect some LEDs to verify the configuration is correct.

miri64 · 2022-10-12T16:06:52Z

Maybe compile_and_test_for_board.py should learn about TEST_ON_CI_WHITELIST wink

The problem is not the script, the problem is that the build system is not aware about the TEST_ON_CI_WHITELIST variable. Just try make test on a non-native board for your tests.

But tbh this script is questionable at best.

Because that's exactly what this script is doing: test the apps and boards like a user would use them, without any CI sugaring.

miri64 · 2022-10-12T16:08:40Z

But again: off-topic.

miri64 · 2022-10-12T16:09:54Z

arg, just squash directly

Huh? I did just that... but I had to pull your suggestions first... Or am I missing a Github feature where you can squash suggestions directly into your own commits?

benpicco · 2022-10-12T16:23:27Z

Nvm, wrote the comment before you squashed the two fixups

miri64 · 2022-10-14T09:21:00Z

@maribu @leandrolanzieri is it okay to merge this still?

sys/net/application_layer/gcoap/forward_proxy.c

miri64 requested review from leandrolanzieri, jia200x, MrKevinWeiss, haukepetersen, PeterKietzmann and cgundogan as code owners August 1, 2022 11:58

github-actions bot added Area: CoAP Area: Constrained Application Protocol implementations Area: Kconfig Area: Kconfig integration Area: network Area: Networking Area: sys Area: System labels Aug 1, 2022

miri64 requested a review from benpicco August 1, 2022 12:04

miri64 added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 1, 2022

This was referenced Aug 9, 2022

gcoap: fixes around empty ACKs #18429

Merged

gcoap: fixes around empty ACKs [backport 2022.07] #18436

Merged

miri64 force-pushed the gcoap_forward_proxy/enh/empty-ack branch from 46d9f50 to 8d57ffd Compare August 11, 2022 13:54

benpicco requested a review from chrysn August 11, 2022 13:58

miri64 mentioned this pull request Aug 30, 2022

nanocoap: add support for no-response option #18154

Merged

benpicco reviewed Oct 11, 2022

View reviewed changes

miri64 force-pushed the gcoap_forward_proxy/enh/empty-ack branch from ad67ddc to cf58776 Compare October 12, 2022 14:16

benpicco approved these changes Oct 12, 2022

View reviewed changes

sys/net/application_layer/gcoap/forward_proxy.c Outdated Show resolved Hide resolved

sys/net/application_layer/gcoap/forward_proxy.c Outdated Show resolved Hide resolved

chrysn and others added 2 commits October 12, 2022 17:33

gcoap_forward_proxy: send empty ACK when response takes too long

798dedd

Co-Authored-By: Martine S. Lenders <m.lenders@fu-berlin.de> Signed-off-by: Martine Lenders <m.lenders@fu-berlin.de>

gcoap: fix "line is longer than 100 characters" pointed out by Vera++

63c4fe5

miri64 force-pushed the gcoap_forward_proxy/enh/empty-ack branch from 915684f to 63c4fe5 Compare October 12, 2022 15:34

leandrolanzieri merged commit 70acefa into RIOT-OS:master Oct 14, 2022

leandrolanzieri added this to the Release 2022.10 milestone Oct 14, 2022

miri64 deleted the gcoap_forward_proxy/enh/empty-ack branch October 14, 2022 09:24

Teufelchen1 reviewed Apr 9, 2024

View reviewed changes

sys/net/application_layer/gcoap/forward_proxy.c Show resolved Hide resolved

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gcoap_forward_proxy: send empty ACK when response takes too long #18386

gcoap_forward_proxy: send empty ACK when response takes too long #18386

miri64 commented Aug 1, 2022 •

edited

Loading

benpicco left a comment

miri64 commented Oct 12, 2022

riot-ci commented Oct 12, 2022 •

edited

Loading

miri64 commented Oct 12, 2022

benpicco left a comment

miri64 commented Oct 12, 2022

benpicco commented Oct 12, 2022

miri64 commented Oct 12, 2022

miri64 commented Oct 12, 2022

miri64 commented Oct 12, 2022

benpicco commented Oct 12, 2022

miri64 commented Oct 14, 2022

gcoap_forward_proxy: send empty ACK when response takes too long #18386

gcoap_forward_proxy: send empty ACK when response takes too long #18386

Conversation

miri64 commented Aug 1, 2022 • edited Loading

Contribution description

Testing procedure

Issues/PRs references

benpicco left a comment

Choose a reason for hiding this comment

miri64 commented Oct 12, 2022

riot-ci commented Oct 12, 2022 • edited Loading

Murdock results

Artifacts

miri64 commented Oct 12, 2022

benpicco left a comment

Choose a reason for hiding this comment

miri64 commented Oct 12, 2022

benpicco commented Oct 12, 2022

miri64 commented Oct 12, 2022

miri64 commented Oct 12, 2022

miri64 commented Oct 12, 2022

benpicco commented Oct 12, 2022

miri64 commented Oct 14, 2022

miri64 commented Aug 1, 2022 •

edited

Loading

riot-ci commented Oct 12, 2022 •

edited

Loading