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

Subnet range selector, interface fixes #481

Merged
merged 10 commits into from
Dec 27, 2023

Conversation

0xCA
Copy link
Contributor

@0xCA 0xCA commented Nov 6, 2023

Subnet range selector

image

10.0.0.1/16 server with a very few clients, correctly suggesting an IP from 10.0.4.0/22 for a new client

Reason

This feature is intended for subdivision of large wireguard servers, such as /16 subnets.
I wanted to allocate smaller blocks of the server address space for specific purposes and easier firewalling.
Current IP suggestion is unaware of such blocks, which requires entering each IP manually.
So I made it aware.

Details

  • Subnet ranges are set in an optional variable SUBNET_RANGES
  • Format: SR Name| 10.0.1.0/24; SR2| 10.0.2.0/24, 10.0.3.0/24, 2001:0db8::/32
  • Each CIDR must be inside one of the server interfaces
  • Other than that, you can go for a really small CIDRs, no problem
  • Any invalid CIDRs or entire SRs are discarded during the server load
  • The list is only stored in memory, so you can change it by changing the env variable and restarting wireguard-ui
  • Multiple server interfaces or multiple client CIDRs are supported, but not auto-suggested
  • No part of this feature is saved to the database

IP suggestion

  • IP is suggested from a chosen SR
  • If no free IPs are left, an error is shown
  • Default SR is Any, which is "any server interface", same as no SR at all
  • For existing clients, you can select a different SR, a new IP is automatically suggested
  • If you select the same SR, a new IP is suggested
  • Fix for multiple interfaces: it only fails if no interface has free IPs

Subnet range filter

image

  • Allows to show clients from a selected subnet range for easier managing
  • Pure client side, no requests
  • A SR is detected server-side when wg client info is requested
  • Only the first match is used
  • There is an in-memory lookup cache on the server, that reduces subsequent cidr-contains-ip evaluations

Extra

image
I also Implemented some interface/UX fixes

  • Apply config button is properly shown or hidden when needed
  • Toast notifications are not obstructing the buttons anymore

@nebulosa2007
Copy link
Contributor

You done great work, but you want implement several options at one time and it's not convinient for development..
Could you split you changes on three branches:

  1. ip-range-selector
  2. interface/UX fixes
  3. adding a new makefile (if it new file really needed for options above)

It will increase the chances of appearing in the main branch

@0xCA
Copy link
Contributor Author

0xCA commented Nov 6, 2023

@nebulosa2007
It's actually going to be less convenient for development if I split it. One part will conflict with the other, because the changes are in the same places.
The only thing I can split without affecting anything, is input tag fix.
As for Makefile, it's there purely for convenience during the development. I can remove it from the branch, if asked by the maintainer.

@systemcrash
Copy link
Contributor

systemcrash commented Dec 3, 2023

@nebulosa2007 is right here.
-Remove the makefile additions. They're not useful in this repo. (maybe for you to compile locally)
-Separate the UX fixes from the go code fixes for the SR stuff.
-Drop most of the formatting fixes here and there

Makefile Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
handler/routes.go Outdated Show resolved Hide resolved
handler/routes.go Outdated Show resolved Hide resolved
templates/base.html Outdated Show resolved Hide resolved
util/config.go Outdated Show resolved Hide resolved
templates/base.html Show resolved Hide resolved
util/util.go Outdated Show resolved Hide resolved
@0xCA
Copy link
Contributor Author

0xCA commented Dec 3, 2023

-Separate the UX fixes from the go code fixes for the SR stuff.

The SR is not a purely server-side update. I have to change the UI. As for UX fixes, it's not worth the effort separating it, only for the merger to deal with additional conflicts.

-Drop most of the formatting fixes here and there

Is it really worth it? I do understand that it might prevent a minor conflict here and there. But it significantly degrades the DX. OR requires a manual "second pass" with all the formatters turned off (which is not particularly convenient to achieve, let alone the process of manually reverting every fmt change).

@0xCA
Copy link
Contributor Author

0xCA commented Dec 3, 2023

#481 (comment)
#481 (comment)
#481 (comment)
@systemcrash
Could you please keep your answers in their threads?

@systemcrash
Copy link
Contributor

systemcrash commented Dec 3, 2023 via email

@systemcrash
Copy link
Contributor

systemcrash commented Dec 3, 2023 via email

@systemcrash
Copy link
Contributor

systemcrash commented Dec 3, 2023 via email

@0xCA
Copy link
Contributor Author

0xCA commented Dec 3, 2023

Sorry, on email.

Are you going to move it later? I can wait and continue later. Or we could continue in the main thread, but I don't think it's a really good idea.

Edit: I just noticed you already moved your answers

@0xCA
Copy link
Contributor Author

0xCA commented Dec 3, 2023

So dedicate a PR to running go fmt on the code base. 👌

Sure thing, for an active repo. Not sure what to do here though. Turn off all the formatters for this repo only?..
The more time passes, the worse it gets for anyone trying to contribute.

@systemcrash
Copy link
Contributor

So dedicate a PR to running go fmt on the code base. 👌

Sure thing, for an active repo. Not sure what to do here though. Turn off all the formatters for this repo only?.. The more time passes, the worse it gets for anyone trying to contribute.

Agreed on the active repo thing. But prepare the PR and branch such that it could go into our repo 😉

Look at the quality of some of the PRs. Some are messy and a rats nest of dependencies where all commits slipped in and the original idea is lost in noise.

@0xCA
Copy link
Contributor Author

0xCA commented Dec 3, 2023

I resolved the issues.
As for undoing gofmt, I probably won't do it again. Vscode absolutely refused to disable it, even though I disabled it in 5 places. So I had to use a dumb text editor.
Next time (Telegram PR?) I will either ask someone to fix it and make a PR/diff for my PR branch, or will never publish my changes in the first place. It's just too much effort to combat gofmt, that is actually trying to make things better.

Copy link
Owner

@ngoduykhanh ngoduykhanh left a comment

Choose a reason for hiding this comment

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

Thank you @0xCA for the PR and everybody here for your contribution to making the PR better. Overall, the changes look good to me. I've just left one comment. I'm happy to merge it if @0xCA can follow up here and resolve the conflicts.

Comment on lines +214 to +219
<div class="form-group">
<label for="subnet_ranges" class="control-label">Subnet range</label>
<select id="subnet_ranges" class="select2"
data-placeholder="Select a subnet range" style="width: 100%;">
</select>
</div>
Copy link
Owner

Choose a reason for hiding this comment

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

I think we should hide the subnet range input by default if the SUBNET_RANGES environment variable is not set. Not everyone needs this feature. It will probably confuse new users.
And, the same comment for the dropdown list near the search box.

Copy link
Contributor Author

@0xCA 0xCA Dec 27, 2023

Choose a reason for hiding this comment

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

I actually intend to place this to "Additional configuration" <summary>, along with tg userid setting. To hide it, templates or js must be used, which isn't beautiful. It probably isn't necessary if it's already mostly out of the way?
As for dropdown, it's gonna be just default options if no subnet ranges are defined. No change required.

Copy link
Owner

Choose a reason for hiding this comment

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

IDK, it was my personal preference. No worries, I can keep it like this and refactor it later.
Leaving it in the "Additional configuration" doesn't seem nice to me as I think it should be part of the IP allocation section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then maybe Endpoint should be? It's an uncommon thing.

Copy link
Owner

Choose a reason for hiding this comment

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

Endpoint is a standard configuration supported by WireGuard, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is, I mean uncommon by usage. Very useful sometimes, but most of the time it's just server-clients, with only some clients having endpoint config.

0xCA added 9 commits December 27, 2023 06:07
fix: free ip search was stopping after no free ip on the first interface
fix: toast obstructing the buttons
fix: stuck apply config button
If the same subnet range is selected, a different IP is suggested
- Reverted formatting fixes where possible
- Clarified the note about updateSearchList()
- Fixed a typo
- Made SUBNET_RANGES example yaml-friendly
- Removed Makefile
@0xCA
Copy link
Contributor Author

0xCA commented Dec 27, 2023

@ngoduykhanh I rebased on the latest master. Let's decide on subnet ranges input and it's ready for merge.

@ngoduykhanh
Copy link
Owner

@0xCA I replied to your comment and merged this PR. Thanks again!

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.

4 participants