Wireguard by lxnt · Pull Request #82 · canonical/netplan

lxnt · 2019-03-16T15:57:06Z

Description

preliminary.
add wireguard and l2tp support.

questions:

is it a good idea to put them into tunnels section at all
l2tp has interesting format for local_ip, so what should be done:
- new keyword, not use existing local (as is done now)
- reuse local, change its parser
wireguard has (optional) endpoint, should the remote key be reused for that?
coverage does not exclude glib, the results are useless (bionic). any tips?

Checklist

[ x] Runs make check successfully.
Retains 100% code coverage (make check-coverage).
[ x] New/changed keys in YAML format are documented.
(Optional) Closes an open bug in Launchpad.

lxnt · 2019-03-16T18:41:03Z

l2tp's local address can be either ipv4 or ipv6 or one of 'auto, 'static' or 'dynamic' (for networkd)
wireguard remote address (endpoint) can be either ipv4, ipv6 or a hostname and must include port.

so the question is how to do that - reuse local and remote keys and move all verification to validate_tunnel() or have separate keys or even separate sections (not include them in "tunnels")

lxnt · 2019-03-17T19:34:51Z

Tunnel address validation was reworked to allow for hostnames, ports and keywords.
now used for every other address too.

Dummy interfaces were added so that empty bridges can be forced up on networkd (by adding a dummy).

This is now feature-complete as far as I'm concerned. Please review the general idea.

lxnt · 2019-03-17T20:39:40Z

No way to test l2tp due to https://github.com/lxc/lxd/issues/5439 at the moment.

Request overall review.

cyphermox

is it a good idea to put them into tunnels section at all

No issues at all, those do make sense as tunnels.

l2tp has interesting format for local_ip, so what should be done:
Steve, what do you think? I think it would be best to keep the same key, but parse it differently for that tunnel type.
wireguard has (optional) endpoint, should the remote key be reused for that?
One more for Steve. Reusing 'remote' seems like the most straightforward, but it might be confusing if common parlance for wireguard is to refer this as "endpoint" only.
coverage does not exclude glib, the results are useless (bionic). any tips?

Coverage reports work fine here and automated through codecov. Have you run 'make check-coverage'? This should generate a proper html report for you under test-coverage/C/; and it only contains the relevant bits.

Could you please add unit tests that test the various changes to config generation you made so we can be sure there are no regressions in other parts of the code than to handle tunnels? Then I'll gladly write integration tests, with your input on what would be a good way to test these (with lxd or without).

cyphermox · 2019-03-20T16:56:13Z

doc/netplan.md

 :    netplan ID of the underlying device definition on which this VLAN gets
     created.

+## Properties for device type ``dummies:``


Please keep dummy interfaces separate, those are unrelated to L2TP / Wireguard tunnels.

Also, you couldn't know about this beforehand, but I have some different plans on how to address this particular device type :)

What I mean by this is that it probably should be in a completely separate PR, since it's not directly related to tunnels.

Well, yes, I agree it doesn't belong here. It just was slapped on top in hurry, because of how systemd-networkd refuses to up the bridge interface if it doesn't contain at least one other interface.

Can you share your plans?

lxnt · 2019-03-20T19:35:04Z

In the current form it reuses local/remote for l2tp and wireguard. In wireguard it also parses 'endpoint' as equivalent to 'remote' because I think this minimizes confusion.

re coverage - yes, I run check-coverage (btw it's broken on gcc8, but that's another topic)

The result on master branch looks like

Overall coverage rate:
  lines......: 21.4% (391 of 1830 lines)
  functions..: 33.9% (41 of 121 functions)

due to what I suspect --remove generate.info "/usr*" bit not working
so that every glib function is listed as not covered.
which makes writing tests let's say inconvenient

i'd just make a container with identical setup to codecov if I only knew what setup they have

EDIT: ok, it sort of works on xenial, so I'll use this for the time being.

vorlonofportland · 2019-03-21T20:12:22Z

On Wed, Mar 20, 2019 at 10:13:48AM -0700, Mathieu Trudel-Lapierre wrote: - l2tp has interesting format for local_ip, so what should be done: Steve, what do you think? I think it would be best to keep the same key, but parse it differently for that tunnel type.

I agree that we should extend the existing key to accept these additional values and define the schema as such; and reject these additional values when used on a tunnel type that does not support them. I see that the default value is defined as 'auto'. Arguably that is already the default for other tunnel types that do not support this as an explicit value, and we should accept this value for all tunnel types.

- wireguard has (optional) endpoint, should the remote key be reused for that? One more for Steve. Reusing 'remote' seems like the most straightforward, but it might be confusing if common parlance for wireguard is to refer this as "endpoint" only.

I think it's preferable to be consistent with the schema for tunnels and use 'remote', not conform to the tunnel-specific terms. We might consider allowing 'endpoint' as an alias for 'remote'.

…

-- Steve Langasek Give me a lever long enough and a Free OS Debian Developer to set it on, and I can move the world. Ubuntu Developer https://www.debian.org/ [email protected] [email protected]

sil2100 · 2020-04-21T16:59:17Z

Since this was split to separate PRs, I think we might want to close this. Either me or Lukas will look at the new ones in the nearest time. Thanks!

lxnt added 3 commits March 16, 2019 16:49

Add wireguard and l2tp support for networkd

2a67e1b

initial docs

18b51dd

support hostnames in wireguard endpoints

1505525

lxnt added 15 commits March 17, 2019 13:04

refactor tunnel address validation

86cc195

fix tunnel address validation

e281a0a

remove printfs

df0e4c6

move validate_address() to the top

d931281

return address type

1f89d48

use validate_address in handle_addresses()

a567895

use validate_address() in handle_nameservers_addresses()

f66d7ca

use in validate_address() in handle_wireguard_allowed_ips()

10fe965

required address flags are implicitly permitted

0c5dfe6

ADDR_IS_OPTIONAL is better sent in permitted

97c56be

use validate_address() for routes

c2b51d3

allow prefixes in routes to/from/via

fd0c02e

allow zero-length prefixes

7910273

It appears that PrivateKeyFile is of too new a systemd. Disable for now.

c7399d5

add dummy interfaces

ac5aa08

reenable PrivateKeyFile.

45e6e50

lxnt marked this pull request as ready for review March 17, 2019 20:39

cyphermox suggested changes Mar 20, 2019

View reviewed changes

This was referenced Dec 4, 2019

Add wireguard support, take two. #113

Merged

Add L2TP support #114

Open

Add dummy interfaces #115

Closed

sil2100 closed this Apr 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wireguard#82

Wireguard#82
lxnt wants to merge 19 commits intocanonical:masterfrom
lxnt:wireguard

lxnt commented Mar 16, 2019

Uh oh!

lxnt commented Mar 16, 2019

Uh oh!

lxnt commented Mar 17, 2019 •

edited

Loading

Uh oh!

lxnt commented Mar 17, 2019

Uh oh!

cyphermox left a comment

Uh oh!

cyphermox Mar 20, 2019

Uh oh!

cyphermox Mar 20, 2019

Uh oh!

lxnt Mar 20, 2019

Uh oh!

lxnt commented Mar 20, 2019 •

edited

Loading

Uh oh!

vorlonofportland commented Mar 21, 2019 via email

Uh oh!

sil2100 commented Apr 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

lxnt commented Mar 16, 2019

Description

Checklist

Uh oh!

lxnt commented Mar 16, 2019

Uh oh!

lxnt commented Mar 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lxnt commented Mar 17, 2019

Uh oh!

cyphermox left a comment

Choose a reason for hiding this comment

Uh oh!

cyphermox Mar 20, 2019

Choose a reason for hiding this comment

Uh oh!

cyphermox Mar 20, 2019

Choose a reason for hiding this comment

Uh oh!

lxnt Mar 20, 2019

Choose a reason for hiding this comment

Uh oh!

lxnt commented Mar 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vorlonofportland commented Mar 21, 2019 via email

Uh oh!

sil2100 commented Apr 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lxnt commented Mar 17, 2019 •

edited

Loading

lxnt commented Mar 20, 2019 •

edited

Loading