Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Netplan support #210

Merged
merged 13 commits into from
Nov 6, 2024
Merged

Netplan support #210

merged 13 commits into from
Nov 6, 2024

Conversation

kbcz1989
Copy link
Contributor

Hello,

first of all thank you for creating such a great role. This PR is adding support for managing wireguard through Netplan.
It's non-invasive change, nothing should happen if variable wireguard_use_netplan stays false, as defined in defaults.

Thank you!

Copy link
Owner

@githubixx githubixx left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I'll try to have a look till end of next week or so. But one thing upfront: Netplan is specific to Ubuntu, right? In this case it should be wireguard_ubuntu_use_netplan and moved to the section were the other wireguard_ubuntu_* variables are defined.

@kbcz1989
Copy link
Contributor Author

@githubixx Thank you for the review and suggestions. I updated the code to reflect it.

Comment on lines 99 to 103
# Netplan directory to store WireGuard configuration on the remote hosts
wireguard_ubuntu_netplan_remote_directory: "/etc/netplan"

# Netplan configuration file priority
wireguard_ubuntu_netplan_conf_priority: "70"
Copy link
Owner

Choose a reason for hiding this comment

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

I actually don't see a need for these additional variables. Esp. the ternary() function used further down below makes the code so much more complicated and hard to read IMHO. Also if another distribution decides to introduce "whateverplan" another case might be needed.

There is already the variable wireguard_remote_directory with its default value of {{ '/etc/wireguard' if not ansible_os_family == 'Darwin' else '/opt/local/etc/wireguard' }}. This can be still used. I also introduced wireguard_conf_filename which makes wireguard_ubuntu_netplan_conf_priority obsolete and makes the current implementation more flexible.

I've implemented changes for your PR in https://github.com/githubixx/ansible-role-wireguard/pull/new/netplan-support-v2. That PR doesn't contain all your changes just the diffs. That's easier to show what should be changed then adding lots of comments here. Also templates/etc/netplan/wg0.yaml.j2 was renamed to templates/etc/netplan/wg.yaml.j2. I also added an additional Molecule test to check if Netplan support is working. Please also update README accordingly.

@kbcz1989
Copy link
Contributor Author

kbcz1989 commented Nov 3, 2024

@githubixx Great suggestions, thank you! I updated the code.

Molecule tests are executed successfully:

PLAY RECAP *********************************************************************
test-wg-ubuntu2004         : ok=23   changed=4    unreachable=0    failed=0    skipped=7    rescued=0    ignored=0
test-wg-ubuntu2204         : ok=23   changed=4    unreachable=0    failed=0    skipped=7    rescued=0    ignored=0
test-wg-ubuntu2404         : ok=23   changed=4    unreachable=0    failed=0    skipped=7    rescued=0    ignored=0

I also updated the docs, let me know if I provided enough info or if there is anything else I should change

@kbcz1989
Copy link
Contributor Author

kbcz1989 commented Nov 3, 2024

@githubixx I implemented changes made in #102, but I had to update the logic as changes in that PR breaks existing setups because if wireguard_endpoint is undefined (which is default) the peers and port will not be set.

Please let me know if I should open a separate PR for handling both netplan template as well as the original template or how should this be handled?

@kbcz1989
Copy link
Contributor Author

kbcz1989 commented Nov 3, 2024

@githubixx I now realize that molecule tests needs to be properly updated also. I will look into it.

@kbcz1989
Copy link
Contributor Author

kbcz1989 commented Nov 4, 2024

@githubixx The tests you prepared are actually covering netplan use-case correctly. I think the PR is now ready for review. Can you please check?

Once and if this gets merged I will look into 2 more issues:

  1. fix the conditions introduced in fa7b0db and make proper molecule tests for that
  2. implement changes from Add support for wireguard_include_peers variable #196 and make proper tests for that also

Copy link
Owner

@githubixx githubixx left a comment

Choose a reason for hiding this comment

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

I only found three minor issues left. If they're fixed I guess the PR can be merged.

version: 2
renderer: networkd
tunnels:
wg0:
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
wg0:
{{ wireguard_interface }}:

molecule/netplan/molecule.yml Show resolved Hide resolved
README.md Outdated
Comment on lines 142 to 144
wireguard_remote_directory: "/etc/wireguard" # On Linux
# wireguard_remote_directory: "/opt/local/etc/wireguard" # On MacOS
# wireguard_remote_directory: "/etc/netplan" # On Ubuntu if wireguard_ubuntu_use_netplan is true
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
wireguard_remote_directory: "/etc/wireguard" # On Linux
# wireguard_remote_directory: "/opt/local/etc/wireguard" # On MacOS
# wireguard_remote_directory: "/etc/netplan" # On Ubuntu if wireguard_ubuntu_use_netplan is true
wireguard_remote_directory: >-
{%- if wireguard_ubuntu_use_netplan -%}
/etc/netplan
{%- elif ansible_os_family == 'Darwin' -%}
/opt/local/etc/wireguard
{%- else -%}
/etc/wireguard
{%- endif %}

This is just a copy&paste of what's in defaults/main.yaml.

@kbcz1989
Copy link
Contributor Author

kbcz1989 commented Nov 6, 2024

@githubixx thanks, it's done now.

@githubixx githubixx merged commit 955e64b into githubixx:master Nov 6, 2024
@kbcz1989 kbcz1989 deleted the netplan-support branch November 6, 2024 23:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants