Skip to content

[feature] Added parser for Wireguard #100#103

Merged
nemesifier merged 4 commits intomasterfrom
issues/100-wireguard-parser
Jun 28, 2022
Merged

[feature] Added parser for Wireguard #100#103
nemesifier merged 4 commits intomasterfrom
issues/100-wireguard-parser

Conversation

@codesankalp
Copy link
Member

Closes #100

@codesankalp codesankalp force-pushed the issues/100-wireguard-parser branch 2 times, most recently from c50919c to 2a5dd77 Compare June 1, 2022 18:35
@codesankalp codesankalp marked this pull request as ready for review June 1, 2022 18:35
@codesankalp codesankalp force-pushed the issues/100-wireguard-parser branch from 50ff9e2 to e48a281 Compare June 1, 2022 19:39
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The list of available parsers need to be updated: https://github.com/openwisp/netdiff#parsers, we need to add at least the information of the command that needs to be executed to get wireguard info.

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to improve the deafult readability a bit we could set the NetJSON label to the allowed IPs.

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's change also the detection of down links as discussed: looking at latest handshake, it's zero or older than 5 minutes (use unix timestamp to calculate these to avoid time zone issues) we can consider the link down.

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add assertions which make sure that changes of the status of the links from up to down are noticed by the parser? We'll need to have at least two wg-data.txt files with different links and make sure the parser can notice differences when going from one state to the other and vices versa.

At the moment in my testing I have noticed the parser cannot detect links which should be flagged as down, it seems to me those links are simply ignored right now which is not correct.

@codesankalp
Copy link
Member Author

it seems to me those links are simply ignored right now which is not correct.

Yes because if it is not up, ignoring it will remove it from diff of old and new wireguard parse and will show it as down in topology graph.

In openwisp-network-topology, I tested it by creating topology using the fetch strategy and after 5 minutes without updating the url I ran the fetch strategy again and it showed as down. I have added a diff test for the same.

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One request: can we make sure we do not save the private key?
It's a security concern.

@codesankalp codesankalp force-pushed the issues/100-wireguard-parser branch from 425c5d8 to 32ee9d2 Compare June 13, 2022 20:39
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're almost there!

Please update the README section which lists parsers:
https://github.com/openwisp/netdiff#parsers.

See also my comments below.

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! 👍

@nemesifier nemesifier changed the title [change] Added parser for Wireguard #100 [feature] Added parser for Wireguard #100 Jun 28, 2022
@nemesifier nemesifier merged commit ba54050 into master Jun 28, 2022
@nemesifier nemesifier deleted the issues/100-wireguard-parser branch June 28, 2022 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

[feature] Add parser for Wireguard

2 participants