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

Add support for wireguard_include_peers variable #196

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jelmer
Copy link

@jelmer jelmer commented Nov 13, 2023

This allows explicitly setting which peers a node can connect to. If the variable is not present, then all peers are included.

Fixes #195

@jelmer jelmer changed the title Add support for wireguard_peers variable Add support for wireguard_reachable_peers variable Nov 14, 2023
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 your PR! I'm currently not so happy with the variable name wireguard_reachable_peers 😉 While the term is "technically" true I think it takes a bit to figure out what this variable is all about. E.g. if the variable would be called wireguard_include_peers then it'd be possible to also introduce wireguard_exclude_peers later if needed. Formerly that would have been a "whitelist" and "blacklist" I guess but makes no sense anymore to use them.

Nevertheless there is definitely some documentation needed and the variable should be put separately in README. The variables in that variable block are covered by "wg_quick". That's why wireguard_unmanaged_peers shouldn't be there too. I'll change that with the next release.

So can you please put your variable and one or two sentences below the text The commands are executed in order as described in wg-quick.8?

@jelmer
Copy link
Author

jelmer commented Nov 21, 2023

That seems reasonable - updated to address your feedback.

@jelmer jelmer changed the title Add support for wireguard_reachable_peers variable Add support for wireguard_include_peers variable Nov 21, 2023
@jelmer jelmer requested a review from githubixx December 20, 2023 09:52
@jelmer
Copy link
Author

jelmer commented Jan 27, 2024

@githubixx any more thoughts on this?

@githubixx
Copy link
Owner

@jelmer Sorry, I'm currently a little bit short on time. I'll get back to you next week. I need to test the patch first to make sure it has no side effects. But it'd be helpful if you could extend the Molecule test accordingly. You don't have to execute it. It's just for me to have a starting point. Maybe make a copy of the Ubuntu 22.04 host and put it here with adjusted values. And the add the needed variable at the end here. Thanks!

gregorydlogan added a commit to gregorydlogan/ansible-role-wireguard that referenced this pull request Mar 5, 2024
…master

Pull request githubixx#196
Fixes githubixx#195
  Add support for wireguard_include_peers variable
@kbcz1989 kbcz1989 mentioned this pull request Nov 4, 2024
@jelmer
Copy link
Author

jelmer commented Nov 7, 2024

updated to resolve conflicts; I hope to get to adding tests sometime this month

@kbcz1989
Copy link
Contributor

kbcz1989 commented Nov 7, 2024

@jelmer Hello. Thanks for getting back at this. I am in dire need of this change :)
I wanted to make tests to get this going so Ill paste my change here, if it helps you anyhow:

-    - name: There should be as much WireGuard interfaces as hosts in vpn group minus one
+    - name: There should be as many WireGuard interfaces as hosts in vpn group minus one or as defined peers
       ansible.builtin.assert:
         that:
-          - "hosts_count|int -1 == wireguard__interfaces_count.stdout|int"
+          - >
+            (wireguard_include_peers is defined and
+             wireguard__interfaces_count.stdout|int == wireguard_include_peers|length) or
+            (wireguard_include_peers is not defined and
+             hosts_count|int - 1 == wireguard__interfaces_count.stdout|int)

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.

ability to restrict exported peers
3 participants