Skip to content

Conversation

@cristiangauma
Copy link
Contributor

linuxserver.io


We welcome all PR’s though this doesn’t guarantee it will be accepted.

Description:

This PR add the next features to this image:

  • Add necessary code to accept a list of strings in the PEERS environment variable while keeping compatibility with the old way of defining peers with a simple integer. Also, if someone is using an old peer but wants to start to use the list of strings feature, it can be added to the list too. For instance: PEERS=myPC,myPhone,myTablet,2
  • Add ALLOWEDIPS environment variable so the peers configuration will be limited to the values passed to that variable. The default value is the one that already existed before in the peers.conf template.
  • Add a commented line in each of the [Peer] sections in wg0.conf, so it will be easier to understant which peers are configured and where.
  • Modify show-peer accordingly so now it looks for that commented line.

Benefits of this PR and context:

  • Will provide a much easier peer configuration management if the list of strings is used as the peer configuration will have names. Users won't need to remember which device was the "peer3".
  • Users will be able to limit the IPs or Ranges where the devices connecting to the VPN with the ALLOWEDIPS environment variables.
  • Users will still be able to use the old way of defining the peers, the new way, or a mix of both.

How Has This Been Tested?

The following tests were done manually (and worked as expected) in my local environment:

  • Upgraded from the current LinuxServer image to my new code/image. Used show-peer and the result was the expected one.
  • Upgraded from the current LinuxServer image to my new code/image modifying the PEERS variable directly.
  • Tried to use weird names with weird characters in the PEERS environment variable.
  • Created a mix of configurations between the old way to create the PEERS and the new way. For instance: PEERS=myPC,myPhone,myTablet,2
  • Tried to recreate the container without the ALLOWEDIPS environment variable.

Breaking changes

One template has to be deleted in order to create the new peers if the user start to use a list of strings in the PEERS environment variable. The file that has to be deleted is:

  • /config/templates/peer.conf

Source / References:

@cristiangauma
Copy link
Contributor Author

It seems that the greeting github action is failing and it is a known problem. To fix that, the trigger has to be changed to pull_request_target: actions/first-interaction#10 (comment)

@nemchik
Copy link
Member

nemchik commented Aug 19, 2020

This is fixed in master. If you rebase or merge from our master it should pass.

@aptalca
Copy link
Member

aptalca commented Aug 19, 2020

@cristiangauma Thanks for the PR and the detailed description. At first glance, it looks good. But we'll have to do quite a bit of testing before merge since the PR touches on a lot of moving parts.

@aptalca
Copy link
Member

aptalca commented Aug 23, 2020

@cristiangauma I just dug a little deeper. My only concern is the way peer IPs are assigned. When using a single number for PEERS, it is pretty much hardcoded where peer X has last octet set to "peer X +1" and that never changes. But when PEERS is set to an array, peer X has last octet set to "array[X] + 2" and unfortunately, if the user adds a new entry to the beginning of the array, it will change the peer IP for all existing peers, shifting them up by one, and will invalidate all peer confs currently used, breaking existing client connections. It would require the user to update the peer/client confs on all devices.

@cristiangauma
Copy link
Contributor Author

cristiangauma commented Aug 27, 2020

@aptalca it should be fixed now. Now each time a Peer is added, then it will iterate within this range {2..254} and will check if the IP is used by any other peer (enabled or disabled, as it checks the peer*/*.conf).

I've tested moving peers between the list, deleting configs so the IP is reused, adding peers anywhere... and as far as I've been testing it, we won't have any problem with the IP assignment.

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@aptalca
Copy link
Member

aptalca commented Oct 4, 2020

Thanks so much for this PR. It seems to work great (except for the .donoteditthisfile choking on vars with spaces, but that's fixed now).

@aptalca aptalca merged commit c5a43f2 into linuxserver:master Oct 4, 2020
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.

3 participants