Add support for unmanaged WireGuard peers

[joneskoo]

In order to support peers that are not managed in Ansible, add support to add peers not managed by ansible to WireGuard config. This allows easily defining peers without endpoint that can talk to all nodes being deployed. Especially useful for workstations without an endpoint address.

Add variable wireguard_unmanaged_peers that appends the peers into config.

This closes #41, and closes #45.

Example of how to use this:

vpn:
  hosts:
    server1:
      ...
    server2:
      ...
      wireguard_unmanaged_peers:
        client.example.com:
          public_key: 5zsSBeZZ8P9pQaaJvY9RbELQulcwC5VBXaZ93egzOlI=
          allowed_ips: 10.0.0.3/32
  vars:
    wireguard_unmanaged_peers:
      client.example.com:
        public_key: 5zsSBeZZ8P9pQaaJvY9RbELQulcwC5VBXaZ93egzOlI=
        allowed_ips: 10.0.0.3/32

[joneskoo]

Based on quick test this is exactly what I need for the moment to take this role into use. Hopefully it solves a generic need. At least it's simple, and no-op by default.

[joneskoo]

@githubixx I saw you merged my other PR. I hope you don't mind a friendly ping as there was no comment if this approach would be welcome? No worries if you just didn't have time to look at it yet.

[githubixx]

@joneskoo I didn't have time to test it, yet. And I need to think a little bit about it. I'll get back to you around end of next week I think.

[j8r]

The idea is good, but it will be better to be a dictionary with endpoint, public_key, allowed_ips, persistent_keepalive as possible keys.

[joneskoo]

The idea is good, but it will be better to be a dictionary with endpoint, public_key, allowed_ips, persistent_keepalive as possible keys.

Why? What about preshared key and such?

[joneskoo]

@j8r I updated the PR including description of original post based on your comment.

[j8r]

Nice @joneskoo.
I was looking at https://docs.sweeting.me/s/wireguard#Peer, which did only mention pre-shared keys in another section.

What do you think of having a list of dict, with an other optional key name, which will be # Name = {{name}}?

Also, Endpoint and PublicKey are mandatory, no? If so, no need to check if it they are defined – they must be.

[joneskoo]

Hi @j8r. The dict of dict is symmetric with how hosts are defined in inventory. This role already writes the ansible host name of other hosts as comment for the peers generated so what I did matches that. Further, an explicit "name" makes you wonder if it's going to be written to the config so that's not a good name.

My version:

vpn:
  hosts:
    server2:
      ...
      wireguard_unmanaged_peers:
        client.example.com:
          public_key: 5zsSBeZZ8P9pQaaJvY9RbELQulcwC5VBXaZ93egzOlI=
          allowed_ips: 10.0.0.3/32

If I understand right you propose instead this:

vpn:
  hosts:
    server2:
      ...
      wireguard_unmanaged_peers:
        - comment: client.example.com
          public_key: 5zsSBeZZ8P9pQaaJvY9RbELQulcwC5VBXaZ93egzOlI=
          allowed_ips: 10.0.0.3/32

The config we generate looks like this:

[Interface]
# server2
Address = ...
PrivateKey = ...
ListenPort = 51820

    [Peer]
    # server1
    PublicKey = ...
    AllowedIPs = ...
    Endpoint = ...

    # Peers not managed by ansible from wireguard_unmanaged_peers
    [Peer]
    # client.example.com
    PublicKey = 5zsSBeZZ8P9pQaaJvY9RbELQulcwC5VBXaZ93egzOlI=
    AllowedIPs = 10.0.0.3/32

PublicKey is required. AllowedIPs is effectively required but not a syntax error to be missing. Endpoint is not required, it's common to have it empty for roaming clients that will connect to this node. https://manpages.debian.org/unstable/wireguard-tools/wg.8.en.html#CONFIGURATION_FILE_FORMAT_EXAMPLE

[githubixx]

@joneskoo To give you some feedback: I currently testing it. If it works out as intended I guess I'll merge it.

[githubixx]

In general LGTM. Two comments inline.

But there is one thing that I'm not sure about if it should be left as is or changed. E.g. lets assume we've host_vars/host1.yml with:

wireguard_unmanaged_peers:
  client.example.com:
    public_key: 5zsSBeZZ8P9pQaaJvY9RbELQulcwC5VBXaZ93egzOlI=
    allowed_ips: 10.8.0.218/32

and group_vars/all.yml with

wireguard_unmanaged_peers:
  client2.example.com:
    public_key: 6zsSBeZZ8P9pQaaJvY9RbELQulcwC5VBXaZ93egzOlI=
    allowed_ips: 10.8.0.219/32

Personally I would assume that client2.example.com should appear in the WireGuard config of ALL hosts in the affected group. But that's not the case with the setup above. host1 will only contain client and not client2 peer config.

I think that's a perfectly valid use case to have clients that should be able to connect to all WireGuard hosts while others should be able to only connect to specific WireGuard hosts. WDYT?

[j8r]

@githubixx The variable overriding is expected, that's how Ansible works.

One can create a variable, and merge it to the another one.

[githubixx]

@j8r I know about the variable overriding but the question is if it should stay like this or if in this case the "global" variable and the host variable should be merged in general. As mentioned in my comment for me that's a perfectly valid use case.

[Legogris]

@j8r I know about the variable overriding but the question is if it should stay like this or if in this case the "global" variable and the host variable should be merged in general. As mentioned in my comment for me that's a perfectly valid use case.

For the record, ansible can be configured with either behavior by the user: https://docs.ansible.com/ansible/2.3/intro_configuration.html#hash-behaviour

It's common practice to have replace configured and explicitly merging dictionaries that should be merged inside the respective vars file.

[githubixx]

@Legogris I definitely don't want users to change their Ansible configuration just for that. That makes no sense and documentation also discourages to use that option. So combine filter is at least one way implement that behavior if desired. The point is I'm 💯 sure that someone will ask for that feature in the future 😉 That's why I want to discuss it now.

We can think about a variable to either enable or disable merging "global" and host wireguard_unmanaged_peers variables with default to "no". That's something I can think of currently. But if all people think that's not needed and current behavior is fine then that's ok for me too.

[joneskoo]

We can think about a variable to either enable or disable merging "global" and host wireguard_unmanaged_peers variables with default to "no". That's something I can think of currently. But if all people think that's not needed and current behavior is fine then that's ok for me too.

My 2 cents. It's unlikely peers are defined both globally and per host, and if someone does that, that's a scenario that can be addressed when there's a concrete use case. I don't see breaking that behaviour in a future major version would be unthinkable if this turns out to be wrong.

Most important is to be able to define peers. I think they'll be special case and not that complex in the context of this role (edited). Besides it's always possible to use multiple playbooks to set up multiple tunnels for complex scenarios.

[githubixx]

@joneskoo You're probably right. As I'm the only one who thought about that use case and since it should be possible to add that later if needed we leave it as is and don't think about variable merging any further.

[githubixx]

@joneskoo From my POV it's ready to go. If you don't have anything further to add I'd merge this.

[joneskoo]

I don't have anything to add.

The failure when public_key is missing is not best ever but I think that's because of Ansible and not anything I added. Works OK in my brief testing. I have no idea how to make it better anyways.

FAILED! => {"changed": false, "msg": "AnsibleUndefinedVariable: 'dict object' has no attribute 'public_key'"}

To be clear, I think this is still better than failing after writing a non-working config.

[githubixx]

@joneskoo Thanks for that PR! We leave it as is now. Possible improvements can be done later if needed. Hopefully that makes life of some users easier now 😃