Fix vpn client #901

ersonp · 2021-09-30T13:44:44Z

Did you run make format && make check?
yes

Fixes #783
Fixes #900

Changes:

Vpn retirer only trys 3 times
Added errors from server hello to retire whitelist
Removed conSummarry check in AppStates()
Added new app state AppStatusErrored : "status": 2 which gives the error inside detailed_status
Added a transport check before dailing routes adapted from here
Added RPCErr to preserve the type of the error we return via RPC (as suggested by @alexadhy)

How to test this PR:
Test #783

Run visorA with VPN server and passcode
Run visorB create a transport to vpn-server and connect to Vpn-server of visor A with the wrong passcode.
Visor won't retry after the first try.
Check http://localhost:8000/api/visors/<pk>/summary and see if vpn-client has

{
  "name": "vpn-client",
  "args": [
    "-srv",
    "02d27b250feea1fc2fca518baff5eb30df0d77d43fd2cc323d1a25be6ce3abc7be"
  ],
  "auto_start": false,
  "port": 43,
  "status": 2,
  "detailed_status": "Password didn't match"
}

Test #900

Run visorB without creating a transport to vpn-server and connect to Vpn-server of visor A.
Visor won't retry after the first try.
Check http://localhost:8000/api/visors/<pk>/summary and see if vpn-client has

{
  "name": "vpn-client",
  "args": [
    "-srv",
    "02d27b250feea1fc2fca518baff5eb30df0d77d43fd2cc323d1a25be6ce3abc7be"
  ],
  "auto_start": false,
  "port": 43,
  "status": 2,
  "detailed_status": "transport not found"
}

We update the retirer to not retry till success anymore. We only retry three times.

We whitelist errors sent from server hello as it's pointless to retry.

The commit contains changes to the logic of AppStates(). We are removing the connection summary check as whenever a vpn-client gets an error it closes all the connections to retry again. Because of this even when the vpn-client is still running AppStates() gives the app status for vpn-client as stopped. Insted depending on ProcByName() is sufficient.

The commit contains additions to the proc and rpc code. The addition is of the app status AppStatusErrored that is set if there is an entry in statusErr which is set if the app encounters an error and stops. A . The commit only makes required changes in vpn-client to set the error.

This commit contains updates to MockRPCIngressClient after the changes to RPCIngressClient in previous commit. The code was auto generated via mockery.

This commit contains nolint:errcheck wherever the return value from a func was not checked to suppress warnings.

This commit removes log that was used for testing.

alexadhy · 2021-10-04T10:50:24Z

CMIIW, I actually think that the DetailedStatus can be better with it being a struct containing the actual status of the running application (the enums like the current one in develop, as well as the current error description like the one you have). Can be beneficial also for tests.

So we can eliminate SetDetailedStatusError and instead just use the SetDetailedStatus for everything.

ersonp · 2021-10-04T12:25:59Z

SetDetailedStatusError and SetDetailedStatus are separate because both of them are stored and fetched from different places, i.e. detailed_status is fetched from proc whereas the error is fetched from procManager as the proc is deleted when the app stops after an error. But I think it would be better to just have SetDetailedStatus just like you have suggested.

The commit changes SetDetailedStatusError to SetError as it was confusing and SetError is nore consise and accurate.

ersonp · 2021-10-04T13:04:58Z

@alexadhy I went through the code again and removing SetDetailedStatusError would make the code needlessly complex so instead renamed all instances of SetDetailedStatusError to SetError which is more precise and easy to understand.

jdknives

Just a few questions from my end. It seems like there may be a better way to handle processes so we do not need to set status and errors separately but its not the task of the PR to rewrite the whole library. So apart from the questions this looks good.

internal/vpn/client.go

internal/vpn/handshake_status.go

pkg/visor/api.go

The commit contains a transport check in vpn-client. Before dialing the routes the available transports to the server are checked. If there is none then we send a ErrTransportNotFound. ErrTransportNotFound is whitelisted in the dail retrier.

The commit contains updated error for unknown code, aErr changed to appErr, removed switch statements and whitelisted ErrTransportNotFound in main retrier.

ersonp · 2021-10-05T08:09:33Z

Should I add a max no of retries to the retrier of dialServer too?

The commit removes the retrier in dialServer as it's redundant and the same job can be done by the main retrier.

The commit adds RPCErr struct that is used to preserve the type of the error from RPC. Currelty only the err of Dial() is wrapped with it.

mrpalide

Functionality work as well as possible, and code was clear, and just one little comment.

internal/vpn/client.go

pkg/router/router.go

The commit updates the checkIfTransportAvalailable to check if any transports are established for the visor and not if there is a transport between local and remote visor. checkIfTransportAvalailable also sends a new error ErrNoTransportFound if no transports are established for the visor. Both ErrTransportNotFound and ErrNoTransportFound are whitelisted as ErrTransportNotFound is received from router whenever it is unsuccessful at creating a route to the remote visor..

The commit changes the lock in SetError() from RLock to Lock.

ersonp added 7 commits September 30, 2021 12:15

Update retrier

ab0a25a

We update the retirer to not retry till success anymore. We only retry three times.

Add whitelisted errors

b192e36

We whitelist errors sent from server hello as it's pointless to retry.

Update MockRPCIngressClient

909da77

This commit contains updates to MockRPCIngressClient after the changes to RPCIngressClient in previous commit. The code was auto generated via mockery.

Add nolint:errcheck

0be9645

This commit contains nolint:errcheck wherever the return value from a func was not checked to suppress warnings.

Remove test log

c168993

This commit removes log that was used for testing.

ersonp marked this pull request as ready for review October 4, 2021 10:33

Change SetDetailedStatusError to SetError

ad32d71

The commit changes SetDetailedStatusError to SetError as it was confusing and SetError is nore consise and accurate.

ersonp added 2 commits October 4, 2021 18:46

Fix func comment

7b37430

Fix linting error

84b3024

jdknives reviewed Oct 4, 2021

View reviewed changes

ersonp added 3 commits October 5, 2021 12:09

Add transport check

2db92fb

The commit contains a transport check in vpn-client. Before dialing the routes the available transports to the server are checked. If there is none then we send a ErrTransportNotFound. ErrTransportNotFound is whitelisted in the dail retrier.

Requested changes

b437f92

The commit contains updated error for unknown code, aErr changed to appErr, removed switch statements and whitelisted ErrTransportNotFound in main retrier.

Make format

abf6e35

ersonp added 3 commits October 5, 2021 16:20

Remove dialServer retrier

bb2aa61

The commit removes the retrier in dialServer as it's redundant and the same job can be done by the main retrier.

Add RPCErr

9eab017

The commit adds RPCErr struct that is used to preserve the type of the error from RPC. Currelty only the err of Dial() is wrapped with it.

Fix linting

e1258ba

mrpalide reviewed Oct 5, 2021

View reviewed changes

internal/vpn/client.go Outdated Show resolved Hide resolved

Requested changes

0b6d34a

jdknives requested changes Oct 5, 2021

View reviewed changes

pkg/router/router.go Outdated Show resolved Hide resolved

ersonp added 2 commits October 6, 2021 11:30

Fix datarace

a7349db

The commit changes the lock in SetError() from RLock to Lock.

jdknives approved these changes Oct 6, 2021

View reviewed changes

jdknives merged commit 63b29ae into skycoin:develop Oct 6, 2021

ersonp deleted the fix/vpn-client branch April 11, 2022 15:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix vpn client #901

Fix vpn client #901

ersonp commented Sep 30, 2021 •

edited

Loading

alexadhy commented Oct 4, 2021 •

edited

Loading

ersonp commented Oct 4, 2021

ersonp commented Oct 4, 2021

jdknives left a comment

ersonp commented Oct 5, 2021

mrpalide left a comment

Fix vpn client #901

Fix vpn client #901

Conversation

ersonp commented Sep 30, 2021 • edited Loading

alexadhy commented Oct 4, 2021 • edited Loading

ersonp commented Oct 4, 2021

ersonp commented Oct 4, 2021

jdknives left a comment

Choose a reason for hiding this comment

ersonp commented Oct 5, 2021

mrpalide left a comment

Choose a reason for hiding this comment

ersonp commented Sep 30, 2021 •

edited

Loading

alexadhy commented Oct 4, 2021 •

edited

Loading