gravitl / netmaker

Netmaker makes networks with WireGuard. Netmaker automates fast, secure, and distributed virtual networks.
https://netmaker.io
Other
9.5k stars 552 forks source link

netclient sets keepalive incorrectly #621

Closed shizunge closed 2 years ago

shizunge commented 2 years ago

Found a strange behavior on netclient and found the following codes in netclient/wireguard/common.go

        if keepAliveString == "0" {
            keepAliveString = "15"
        }

Anyone knows why we are doing this? IMO, Netclient should not decide what the value is, but follow the instructions from the server (otherwise what is the propose to having a server).

Also I found the keepalive value in the wireguard configuration file generated by the netclient was also not following the setting on the server. I have not read more codes and found where is the cause.


Besides the keepalive issue, I have comments on the golang style.

  1. Even if we need to change the keepalive value, we should compare and change it before the strconv.Itoa, because comparing number is faster than comparing strings.
  2. We do not really need the strconv.Itoa. Instead, we should use fmt.Printf("something %v something else", keepalive) to generate the command string.
afeiszli commented 2 years ago

basically the reason was because a value of 0 wont work for keepalive, but if the value is unset, it will default to 0. So this just replaces the default with 15

shizunge commented 2 years ago

The problem is that what the UI/API tells users and what the netclient does are different.

Could we generate a command that does not set keepalive, when the keepalive is 0?

If we cannot do it, can we move the check to the server, rather than on the netclient. Let user know we cannot disable keepalive on a netclient.

afeiszli commented 2 years ago

I think it is sensible to unset keepalive if it is 0. I will add this to triage.

shizunge commented 2 years ago

Thanks. Also seen in https://github.com/gravitl/netmaker/blob/master/netclient/ncutils/netclientutils.go

    if keepalive <= 0 {
        keepalive = 20
    }

Which should be removed.