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 wireguard interface and peers #143

Merged
merged 2 commits into from
Dec 27, 2022

Conversation

cosandr
Copy link
Contributor

@cosandr cosandr commented Dec 26, 2022

SUMMARY

Adds support for configuring wireguard interfaces and peers.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME
  • api_modify
  • api_info
ADDITIONAL INFORMATION

Missing tests and equally as important, interface wireguard editing doesn't behave as expected (misconfiguration error, see below). The API obfuscates the value of private-key so the diff is broken, for example:

--- before
+++ after
@@ -6,7 +6,7 @@
             "listen-port": 13231,
             "mtu": 1420,
             "name": "WG1",
-            "private-key": "*****"
+            "private-key": "mLh5K+/uJmZJ1zqarPYh0OyBaCgI1n+xAUBvsPBC+3I="
         }
     ]
 }

This happens even if that private key is already configured on the router. I was unable to find documentation on how to get the API to return sensitive information like this.

UPDATE: This is fixed by adding the "sensitive" policy to the group used to connect.

/user/group> set ansible policy=read,write,api,sensitive

After that, the private-key is returned and diff works as expected.

@github-actions
Copy link

github-actions bot commented Dec 26, 2022

Docs Build 📝

Thank you for contribution!✨

This PR has been merged and the docs are now incorporated into main:
https://ansible-collections.github.io/community.routeros/branch/main

Copy link
Collaborator

@felixfontein felixfontein 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 contribution! Can you please add a changelog fragment? Thanks.

@felixfontein
Copy link
Collaborator

Missing tests and equally as important, interface wireguard editing doesn't behave as expected.

Hmm, that's unfortunate. We probably should document this behavior, and/or have some special code to handle this in a sane (and preferably user-configurable) way.

@codecov
Copy link

codecov bot commented Dec 26, 2022

Codecov Report

Merging #143 (803ae70) into main (8e3b197) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #143   +/-   ##
=======================================
  Coverage   67.44%   67.44%           
=======================================
  Files           2        2           
  Lines         172      172           
  Branches       39       39           
=======================================
  Hits          116      116           
  Misses         40       40           
  Partials       16       16           
Flag Coverage Δ
integration 66.86% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@cosandr
Copy link
Contributor Author

cosandr commented Dec 26, 2022

Hmm, that's unfortunate. We probably should document this behavior, and/or have some special code to handle this in a sane (and preferably user-configurable) way.

I feel like there has to be a way to actually get this information from the API, I can try asking them directly. In the CLI the private key is hidden unless using export (without hide-sensitive at least).

If there isn't, we could add a sensitive and/or force attribute to KeyInfo, without force it would ignore changes.

@cosandr
Copy link
Contributor Author

cosandr commented Dec 27, 2022

@felixfontein I've figured out the issue with private-key and edited the description, it was my fault for not giving my API user enough permissions. Let me know if there's anything more I should do to get this merged, thanks!

Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Great to hear! I have one small comment for the changelog fragment, besides that it looks good!

@felixfontein felixfontein merged commit 62da7dd into ansible-collections:main Dec 27, 2022
@felixfontein
Copy link
Collaborator

@cosandr thanks for your contribution!

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