search

egor-tensin / setup-wireguard

GitHub action to set up WireGuard
MIT License
61 stars 21 forks source link

Add parameter keepalive #7

Closed Tycale closed 1 year ago

Tycale commented 1 year ago

This PR introduces a keepalive argument that I'm using to fix an unexpected "connection reset by peer" error that I've been experiencing. This is helpful for NAT traversal, so I'm happy to share my solution in case it benefits anyone else.

Signed-off-by: Thibault Gérondal thibault.gerondal@sortlist.com

egor-tensin commented 1 year ago

Sorry, I was in no fit state to deal with my GitHub Actions for the past year. I'll try to keep up in the future.

Released. There were some problems with the checks; I merged the commit and took the liberty of fixing them in commit 1aabc8c2311e95b25ec0fa0dd03f8e21910730f8.

Tycale commented 1 year ago

Thank you for merging.

I am curious about the changes you made, as it was working correctly on my end as submitted. Could it possibly be a conflict with something merged before, or because there is no target shell at the start of the script, meaning that it will depend on the Linux distribution you are using? I am not sure.

If you would like to improve this script, I suggest introducing Shellcheck (https://www.shellcheck.net/). It can be quite a nightmare to apply Shellcheck suggestions, but it will make this script robust.

egor-tensin commented 1 year ago

Here's a run of your original PR: https://github.com/egor-tensin/setup-wireguard/actions/runs/3806473133/jobs/6585603110, note the error

Error: any valid prefix is expected rather than "".

I assume this is because of weird usage of bash arrays. In particular, this line strikes me as a bug:

additionnal_wg_args+=("persistent-keepalive ${keepalive}")

This adds a single string to the array, while conceptually it should be two strings ("persistent-keepalive" and "${keepalive}"). I don't know fully how array expansion works in bash (I'm pretty sure nobody knows), but I would assume this array element would expand into a single argument for wg instead of two. My fix was to make sure that in each case (for the preshared key and the keepalive), two strings were added to the array.