napt: Fixes and improvements (IDFGH-6861)

[rojer]

1. Fix enable/disable to properly allocate and deallocate tables. Current algorithm is just broken.
2. Introduce eviction policy when table gets full: oldest connection is evicted, instead of new ones getting silently dropped. this results in much better behavior with small tables than before. When TCP connection is dropped, RSTs are sent both ways to inform parties instead of dropping silently. thiw requires additional 8 bytes per entry but is, again, a big improvement for clients in terms of usability.
3. FIxed handling of timestamp wraparound (every ~50 days of uptime).
4. Added ip_portmap_get() to retrieve current port mapping settings.
5. Added ip_napt_get_stats() for some insight into the state of NAT.

[Alvin1Zhang]

Thanks for your contribution.

[david-cermak]

Current algorithm is just broken.

It would be nice to cover this in the test cases.

Also, the current NAPT test suite fails (on compilation):

../../../../src/core/ipv4/ip4_napt.c:64:16: error: C++ style comments are incompatible with C90 [-Werror]
   u32_t src;   // net
                ^

To run the lwip unit tests on host, please follow these steps:

https://github.com/espressif/esp-lwip/blob/76303df2386902e0d7873be4217f1d9d1b50f982/.gitlab-ci.yml#L33-L46

[rojer]

thanks for the review, all done. however, i was not able to run the tests as they don't seem to be present in the public repository - there's no ports/ directory.

[david-cermak]

@rojer Thanks for the update! Seems like you forgot to revert the changes in src/core/timeouts.c after using sys_timeout()

there's no ports/ directory.

The port directory is present in the contrib repo. I'm sorry, running the CI locally hasn't been very straight-forward. Currently working on improvements for the upcoming branches using GitHub workflows (so contributors can easily check their work). You can temporarily use these workflows to test your changes, though it's very WIP and present only on my personal fork. Tried to pick your changes here, CI fails with the forgotten timeout changes, mentioned above.

[rojer]

Seems like you forgot to revert the changes in src/core/timeouts.c after using sys_timeout()

yes, you're right, forgot to commit that change. done.

The port directory is present in the contrib repo

ok, i cloned it and checked out STABLE-2_1_0_RELEASE but now check.h is missing:

[rojer@nbd ~/cesanta/esp/lwip-contrib 35b011d4cf4c4b480f8859c456587a884ec9d287]$ ./qq.sh 
+ cd ports/unix/check/
+ export LWIPDIR=/home/rojer/cesanta/esp/esp-lwip/src
+ LWIPDIR=/home/rojer/cesanta/esp/esp-lwip/src
+ export CK_DEFAULT_TIMEOUT=1000
+ CK_DEFAULT_TIMEOUT=1000
+ export EXTRA_CFLAGS=
+ EXTRA_CFLAGS=
+ export 'CC=cc '
+ CC='cc '
+ export CCDEP=cc
+ CCDEP=cc
+ make -j 1 check
cc  -DLWIP_NOASSERT_ON_ERROR -I/usr/include/check -I/home/rojer/cesanta/esp/esp-lwip/src/../test/unit -g -DLWIP_DEBUG -Wall -pedantic -Werror -Wparentheses -Wsequence-point -Wswitch-default -Wextra -Wundef -Wshadow -Wpointer-arith -Wcast-qual -Wc++-compat -Wwrite-strings -Wold-style-definition -Wcast-align -Wmissing-prototypes -Wredundant-decls -Wnested-externs -Wunreachable-code -Wuninitialized -Wmissing-prototypes -Wredundant-decls -Waggregate-return -Wlogical-not-parentheses -Wlogical-op -Wc90-c99-compat -Wtrampolines -I. -I../../.. -I/home/rojer/cesanta/esp/esp-lwip/src/include -I../../../ports/unix/port/include -c /home/rojer/cesanta/esp/esp-lwip/src/../test/unit/lwip_unittests.c
In file included from /home/rojer/cesanta/esp/esp-lwip/src/../test/unit/lwip_unittests.c:1:
/home/rojer/cesanta/esp/esp-lwip/src/../test/unit/lwip_check.h:7:10: fatal error: check.h: No such file or directory
    7 | #include <check.h>
      |          ^~~~~~~~~
compilation terminated.
make: *** [/home/rojer/cesanta/esp/lwip-contrib/ports/unix/../Common.allports.mk:91: lwip_unittests.o] Error 1

[rojer]

check.h is missing

apt-get install check fixed it.

ok, updated the branch. in addition to issues with my patch i had to fix two other unrelated C90/C99 issues that prevented code from building, i have no idea how it works on the CI but they are legit in C90 mode.

tests are now passing:

+ export 'EXTRA_CFLAGS=-DIP_FORWARD=1 -DIP_NAPT=1 -DLWIP_ARCH_CC_H -include cc_esp_platform.h'
+ EXTRA_CFLAGS='-DIP_FORWARD=1 -DIP_NAPT=1 -DLWIP_ARCH_CC_H -include cc_esp_platform.h'
+ export 'CC=cc -DIP_FORWARD=1 -DIP_NAPT=1 -DLWIP_ARCH_CC_H -include cc_esp_platform.h'
+ CC='cc -DIP_FORWARD=1 -DIP_NAPT=1 -DLWIP_ARCH_CC_H -include cc_esp_platform.h'
+ make -j 4 check
...
Running suite(s): IPv4
 IPv6
 UDP
 TCP
 TCP_OOS
 TCP_STATE
 DEF
 MEM
 NETIF
 PBUF
 TIMERS
 ETHARP
 DHCP
 MDNS
 MQTT
 SOCKETS
 IP4_ROUTE
100%: Checks: 126, Failures: 0, Errors: 0

[david-cermak]

Nice, that you succeeded in running the tests eventually! Still some issues in the debug prints though:

../../../../src/core/ipv4/ip4_napt.c: In function ‘napt_debug_print’:
../../../../src/core/ipv4/ip4_napt.c:143:6: error: ISO C90 forbids mixed declarations and code [-Werror=c90-c99-compat]
  143 |      uint8_t p = t->proto;
      |      ^~~~~~~

Please note, that the test with debug logs are not executed in the CI:

https://github.com/espressif/esp-lwip/blob/76303df2386902e0d7873be4217f1d9d1b50f982/.gitlab-ci.yml#L47-L50

just wanted to see how the NAPT tests would work with the newly introduced eviction mechanism you implemented, seems like the oldest record was removed here:

[david-cermak]

Note: I'm passing this PR to an internal review, which usually takes some time until accepted and merged.

[rojer]

@david-cermak addressed comments, rebased onto most recent 2.1.2-esp