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

Setting both wireguard_endpoint to "" and wireguard_port to a non default port results in invalid template output #176

Open
daanh432 opened this issue Jan 29, 2023 · 6 comments · May be fixed by #177

Comments

@daanh432
Copy link

The way the template is built. If you set the wireguard_endpoint host_var to an empty string (meaning that that host shouldn't have an endpoint) the if statements end up with in undesired option.

Whilst having a custom port it is not possible to remove the endpoint by setting the wireguard_endpoint to an empty string.

{%       if hostvars[host].wireguard_endpoint is defined and hostvars[host].wireguard_endpoint != "" %}
Endpoint = {{hostvars[host].wireguard_endpoint}}:{{hostvars[host].wireguard_port}}
{%       else %}
Endpoint = {{host}}:{{hostvars[host].wireguard_port}}
{%       endif %}
daanh432 added a commit to daanh432/ansible-role-wireguard that referenced this issue Jan 29, 2023
@aut0
Copy link

aut0 commented Apr 25, 2023

Any reason this is not being addressed?

@githubixx
Copy link
Owner

I've no test case for it 😉 That means I first need to setup a Molecule test case to be able to reproduce the behavior. I normally want to first see the result in a test environment before I merge something that changes the "core" template. But I don't have that much time to create a Molecule test where I need to guess what values covers the PRs case. So I normally only work on such PRs if I've "too much time" or someone provides a Molecule test case that I can use as a template.

@aut0
Copy link

aut0 commented May 3, 2023

Understood. I propose to add the "bug" label.

@githubixx
Copy link
Owner

@daanh432 Can you provide some test variables that helps to reproduce the issue?

@daanh432
Copy link
Author

It has to do with some hosts not having a valid endpoint to connect to because they are behind a firewall for example. This is fine within the Wireguard world because of the ability to use persistent keepalive which will allow the reverse connection anyway without it having an endpoint defined.

With the template in the main branch at this time, setting the endpoint to an empty string and port to a valid value results in it falling back to Endpoint = {{host}}:{{hostvars[host].wireguard_port}}.

So for one of your hosts set the variables as follows (the port value is irrelevant it just needs to be defined):

wireguard_endpoint: ""
wireguard_port: 5555

This will have the unintended output of:

Endpoint = 1.2.3.4:5555

The expected output would be leaving out the Endpoint entirely because we're explicitly telling it to not have an endpoint address.

The pull request I opened a while ago splits up the check for defined and empty string. If it is undefined it will still use Endpoint = {{host}}:{{hostvars[host].wireguard_port}}.

But if it is defined with an empty string it will leave out the endpoint definition entirely and output
# No endpoint defined for this peer.

In short: as of right now it is only possible to remove the endpoint statement in its entirety by not using a custom port. With the change in #177 it is also possible to remove the endpoint statement when having a globally defined custom port.

I would like to help you out with the Molecule test but I have had no available time yet to look into the workings of Molecule.

@daanh432
Copy link
Author

This issue has also been mentioned in an earlier PR. So maybe it is more worthwhile to follow up on that pull request instead.

gregorydlogan added a commit to gregorydlogan/ansible-role-wireguard that referenced this issue Mar 5, 2024
…nsible-role-wireguard into master

Pull request githubixx#177
Closes githubixx#176
  Fixed unsetting the Wireguard endpoint whilst using a custom port
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 a pull request may close this issue.

3 participants