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

Multiple conditional forwarding fields #1542

Closed
wants to merge 15 commits into from
Closed

Multiple conditional forwarding fields #1542

wants to merge 15 commits into from

Conversation

DL6ER
Copy link
Member

@DL6ER DL6ER commented Aug 2, 2020

By submitting this pull request, I confirm the following:

  • I have read and understood the contributors guide, as well as this entire template.
  • I have made only one major change in my proposed changes.
  • I have commented my proposed changes within the code.
  • I have tested my proposed changes, and have included unit tests where possible.
  • I am willing to help maintain this change if there are issues with it later.
  • I give this submission freely and claim no ownership.
  • It is compatible with the EUPL 1.2 license
  • I have squashed any insignificant commits. (git rebase)

What does this PR aim to accomplish?:

Allow users to define more than one conditional forwarding field without much effort

How does this PR accomplish the above?:

Add multiple fields + move conditional forwarding to its own tab (may be renamed)

What documentation changes (if any) are needed to support this PR?:

None

PromoFaux and others added 14 commits June 7, 2020 11:09
… with a mixture of hostnames/IP's

Signed-off-by: Adam Warner <me@adamwarner.co.uk>
Signed-off-by: Adam Warner <me@adamwarner.co.uk>
Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
Fix teleporter bug on hosts with . in their names
Fix ARPFLUSH button on the settings page
Fix query types links on the dashboard (Query Types pie chart)
Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/more-than-one-conditional-forwarding-entry-in-the-gui/11359/29

@jflattery
Copy link

I like and look forward to this change, but from the screenshot it appears as though that this adds 5 sets of fields. Why was this arbitrary value chosen over say 4 or 6, or better, just add a plus button so that a user may add as many as appropriate? Just my two cents.

@dschaper
Copy link
Member

dschaper commented Aug 3, 2020

Well, 5 is one more than 4 while still being one less than 6.

@DL6ER
Copy link
Member Author

DL6ER commented Aug 3, 2020

My first iteration had a text box where you could enter any number between 1 and 1024. However, I also have to balance added complexity with every being able to reinvent the entire web interface with the upcoming Pi-hole v6.0 API. Such very dynamic changes can easily get the bottleneck down the road. 6 was chosen fairly arbitrarily as it allows you to specify, e.g., 3x(IPv + IPv6). Everything more complicated that this should likely just to the configuration in the hard-coded config files.

I know this may not be the answer you wanted, however, I do not want to push the v6.0 transition further and further down the line. The more flexibility we add now, the more we have to reinvent on our way to Pi-hole v6.0. I do want this to become reality still this year (fingers crossed). After all, we have a working prototype with dashboard and Query Log, however, everything else still needs to be re-implemented. Each change on the web interface (and in FTL's API) hits the efforts hard and push them back.

This reply may be more than you requested, but I feel it is important to keep our users updated and sometimes this means we need to put things into perspective.

@jflattery
Copy link

jflattery commented Aug 6, 2020

@DL6ER, Thank you very much for the work you continue to contribute to the community and your response to my feedback. I thought it might be valuable if I shared a mock-up of what I was attempting to purpose. Below is my envisionment of the ideal solution. I believe this to be more aesthetically pleasing and styled in the same manner as the Static DHCP leases configuration. If there was still a need or desire to restrict this form to only allowing a total of 6 sets, this would still be possible.

pi-hole - conditionalForwarding Prototype

Additionally, in regards to the changes, this makes to the 01-pihole.conf file, I believe that for the sake of readability and structure, the added rules should be encapsulated between starting and ending comments. For instance:

# Begin Conditional Forwarding
rev-server=192.168.0.0/24,192.168.0.1
rev-server=192.168.1.0/24,192.168.1.1
# End Conditional Forwarding

I hope this is helpful. I would be happy to assist in this, but I'll need to familiarize myself with Go, and the overall structure of the project.

@PromoFaux
Copy link
Member

but I'll need to familiarize myself with Go

PHP and javascript, before you head down a rabbit hole that gains you nothing other than knowing another language! :)

@jflattery
Copy link

but I'll need to familiarize myself with Go

PHP and javascript, before you head down a rabbit hole that gains you nothing other than knowing another language! :)

Whoops I feel silly, for some reason, I thought I remember glancing at the changes and them being done in Go. I just checked and sure enough it is all PHP and JS. Less of a challenge than I anticipated! I'll give a look at it after I submit my finals this week. :D

@yubiuser
Copy link
Member

yubiuser commented Aug 6, 2020

I knew, I had seen a similar draft somewhere ;-)

https://web.pi-hole.net/settings/networking

Not conditional forwarding, but same basic idea

@bobbydamaker
Copy link

Novice comment. How would this work with IPv6 addresses. It works great with IPv4, but those of us with Unifi USGs (and I am guessing others) are not getting a return from the gateway and we just continue to populate our lists with IPv6 addresses.

Thanks in advance!

@mew1033
Copy link

mew1033 commented Feb 27, 2021

I was looking for this today. Any chance of getting it merged?

@DL6ER
Copy link
Member Author

DL6ER commented Feb 27, 2021

We're actively working on the v6.0 version which will have an altogether rewritten API. Multiple conditional forwarding should be easily doable and will likely come while we're at it. You may want to join the public beta once ready (expect maybe Summer this year, but no guarantees for this). The past has shown that new features have never been implemented as quickly as during the beta rounds (because we take some holidays from our everyday jobs and there are many users testing and giving immediate feedback).

@mew1033
Copy link

mew1033 commented Feb 27, 2021

Excellent, thanks for the update!

@dhritzkiv
Copy link

dhritzkiv commented Feb 27, 2021

@mew1033 I had good results manually setting up a .conf file inside of /etc/dnsmasq.d to achieve multiple conditional forwarding for my VLANs/subnets. I followed some of the instructions in this thread + others from around the web. It's easy and should tide you over until v6.0.

@DL6ER
Copy link
Member Author

DL6ER commented Jul 4, 2021

This will be address in v6.0 where the settings page is re-implemented (almost) from scratch.

@DL6ER DL6ER closed this Jul 4, 2021
@yubiuser yubiuser deleted the new/cond_forwards branch August 3, 2022 07:26
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.

None yet

9 participants