Feedback for "Allow defining clients by their MAC address, host name and networking interface"

PromoFaux · June 25, 2020, 5:00pm

Yep, I think I found where the issue was, and have prepared a fix for this here:

github.com/pi-hole/web

Tweak/custom dns

pi-hole:devel ← pi-hole:tweak/custom_DNS

opened 04:58PM - 25 Jun 20 UTC

PromoFaux

+17 -23

**By submitting this pull request, I confirm the following:** - [x] I have r…ead and understood the [contributors guide](https://github.com/pi-hole/pi-hole/blob/master/CONTRIBUTING.md), as well as this entire template. - [x] I have made only one major change in my proposed changes. - [ ] I have commented my proposed changes within the code. - [x] I have tested my proposed changes, and have included unit tests where possible. - [x] I am willing to help maintain this change if there are issues with it later. - [x] I give this submission freely and claim no ownership. - [x] It is compatible with the [EUPL 1.2 license](https://opensource.org/licenses/EUPL-1.1) - [x] I have squashed any insignificant commits. ([`git rebase`](http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html)) --- **What does this PR aim to accomplish?:** Addresses issues raised in [this comment on discourse](https://discourse.pi-hole.net/t/feedback-for-allow-defining-clients-by-their-mac-address-host-name-and-networking-interface/32324/141?u=promofaux). Seems that the PR that added the execution wrapped to the `pihole` command in the PHP introduced an issue with the error that is displayed back to the user (Despite completing successfully) Additionally I have tweaked the logic around custom ports, a port is only added to the address is it is _explicitly_ added by the user.

DL6ER · June 27, 2020, 11:51am

Is it still running? We're currently haunting another issue with incorrect database pointers when a massive number of TCP clients is spawned. The new/mac_client branch may not be fully up-to-date with respect to development so this may have already been fixed.

drwatson · June 28, 2020, 2:53pm

Yes, still running since 2 days now. Maybe it was an unrelated glitch. It seems my issues are resolved for now. I will continue using new/mac_client and report new issues.

For now, thank you for your help and excellent work!

ampfinger · July 16, 2020, 4:28am

Hi,
As version 5.1.1 is out and there are some bugs already fixed in development branch, i wanted to ask you to update the branch with latest commits?
Especially updating of DNS settings and conditional forwarding is broken at the moment within this branch.

Thanks for your hard work! I like this project very much and as dual-stack user this branch is very useful (happy wife, happy life )

Coro · July 16, 2020, 11:05am

I second this request.

DL6ER · July 17, 2020, 11:46am

See

ampfinger · July 17, 2020, 1:32pm

Thanks for the feedback and hard work, we will wait for it

ubenmackin · July 18, 2020, 10:03pm

Currently I am on:

pi@pihole:~ $ pihole -v
  Pi-hole version is development v5.0-134-g60f0ab51 (Latest: v5.1.1)
  AdminLTE version is new/mac_clients v5.0-403-ga901489a (Latest: v5.1)
  FTL version is new/mac_clients vDev-661dad9 (Latest: v5.1)

Am I understanding that now the best way to keep up with this version to switch over to the dev branch? Is that by doing:

pihole checkout dev

or do I need to do each component:

pihole checkout core dev
pihole checkout web dev
pihole checkout ftl dev

Or should I stick on this branch for now? I know there was DB changes with this, so don't want to corrupt things by switching out of this branch prematurely.

yubiuser · July 19, 2020, 7:28am

It has not been merged now. I'm sure, DL6ER will post a message here when he merged the branches into development.

For now you should stay on this branch if you don't want to loose the ability to define clients by MAC, hostname, etc. and want to avoid to delete your long-term-database.

This two will be sufficient.

DL6ER · July 19, 2020, 8:29pm

Current status is: Waiting for FTL#845 to get merged.

Then I will update all the features branches floating around. And I will also mark them as ready-to-be-merged into development.

Depending on the team's review abilities this may happen within minutes to hours to days. I hope that we can release FTL v5.1.1 tomorrow which also means you will get all the updated branches tomorrow.

I could already do it now, however, there are two drawbacks:

I have double amount of work (I don't care too much about this, but see no. 2)
There would be a needless extra merge commit. It is just cleaner to wait for release/v5.1.1 to be done in order to reduce the number of merge commits floating around. They are just noise. However,. given the complexity of the currently discussed branches, a rebase would result in even (much) more work than an ordinary merge which is why I'll prefer the latter.

I hope this makes my motivation clearer. It's not about me simply not wanting to do it.

ubenmackin · July 19, 2020, 11:35pm

Sorry if my post came across in any way other than just seeking clarification. I totally understand that in the world of software development, things take time. I also get that cleaner is better, and would wish extra work on no one.

I do want to take a moment to thank you (and the rest of the team) on putting this all together. Specifically this feature is a really great one, that is helping to fine tune my setup for the different use cases in my household.

Coro · July 20, 2020, 5:55am

I don't think your post comes across as demanding in any way. Re-reading his reply, I don't think he received it this way, either, so all good.

There seems to be a lot of work going on behind the scenes. They added another optimization yesterday and still have one more bugfix prepared and even an update for the dnsmasq core to the (today released!!) dnsmasq v2.82.

github.com/pi-hole/FTL

Safely reopen gravity database

pi-hole:release/v5.1.1 ← pi-hole:fix/guard_gravity_reopening

opened 03:42AM - 20 Jul 20 UTC

DL6ER

+10 -2

**By submitting this pull request, I confirm the following:** - [X] I have re…ad and understood the [contributors guide](https://github.com/pi-hole/pi-hole/blob/master/CONTRIBUTING.md). - [X] I have checked that [another pull request](https://github.com/pi-hole/FTL/pulls) for this purpose does not exist. - [X] I have considered, and confirmed that this submission will be valuable to others. - [X] I accept that this submission may not be used, and the pull request closed at the will of the maintainer. - [X] I give this submission freely, and claim no ownership to its content. **How familiar are you with the codebase?:** ## 10 --- Wrap gravity reopening in locking to avoid a race collision with API requests. As these are performed from independent threads, it was possible that API calls could have been received in the middle of a database reopening. This can lead to a crash in SQLite3. Fixes #841 where the fix has already been confirmed to work.

github.com/pi-hole/FTL

Update embedded dnsmasq to v2.82

pi-hole:release/v5.2 ← pi-hole:update/dnsmasq-v2.82

opened 04:00AM - 20 Jul 20 UTC

DL6ER

+216 -74

**By submitting this pull request, I confirm the following:** - [X] I have re…ad and understood the [contributors guide](https://github.com/pi-hole/pi-hole/blob/master/CONTRIBUTING.md). - [X] I have checked that [another pull request](https://github.com/pi-hole/FTL/pulls) for this purpose does not exist. - [X] I have considered, and confirmed that this submission will be valuable to others. - [X] I accept that this submission may not be used, and the pull request closed at the will of the maintainer. - [X] I give this submission freely, and claim no ownership to its content. **How familiar are you with the codebase?:** ## 10 --- Update the embedded `dnsmasq` to the just released (5 hrs ago) dnsmasq v2.82. Changelog: ``` version 2.82 Improve behaviour in the face of network interfaces which come and go and change index. Thanks to Petr Mensik for the patch. Convert hard startup failure on NETLINK_NO_ENOBUFS under qemu-user to a warning. Allow IPv6 addresses ofthe form [::ffff:1.2.3.4] in --dhcp-option. Fix crash under heavy TCP connection load introduced in 2.81. <--- This is already included in FTL 5.1 Thanks to Frank for good work chasing this down. Change default lease time for DHCPv6 to one day. Alter calculation of preferred and valid times in router advertisements, so that these do not have a floor applied of the lease time in the dhcp-range if this is not explicitly specified and is merely the default. Thanks to Martin-Éric Racine for suggestions on this. ```

How cool it this!

Also, @DanSchaper commented that the update of dnsmasq will push FTL to v5.2 so v5.1.1 will maybe be skipped. We can stay tuned for more awesome Pi-hole things to come.

DL6ER · July 29, 2020, 10:49am

This feature branch (new/mac_clients) has been updated to the latest version of FTL (release/v5.2).

ampfinger · July 29, 2020, 11:19am

@DL6ER Thank you very much! Will the AdminLTE also be updated so changes (for example Conditional Forwarding on DNS page) can be updated?

Thanks for your hard work

DL6ER · July 30, 2020, 7:06am

Yes, however, the AdminLTE branch is still awaiting two more PRs to be reviewed + merged, first.

ampfinger · July 30, 2020, 7:52am

Alright, thank you very much for your fast reply

DL6ER · July 30, 2020, 8:00pm

PRs have been merged, so I just updated the web branch as well now. Please test if adding custom IPs still works, I had to resolve some merge conflicts and I did so with a "best efforts" approach. I'm limited to my mobile development system right now so I cannot really do much to test my changes. If adding custom IPs in the dropdown select menu still works (Groups -> Clients page), then everything went well.

ampfinger · July 31, 2020, 9:28am

Just tried it, seems to work! Thank you very much

SomeWhereOutThere · August 2, 2020, 2:19am

Thank you very much for pushing the new release to this. pihole is working exceedingly well now. From my experience testing release/5.2, it seems to fix alot of stuff.

DL6ER · August 9, 2020, 8:53am

One further addition is to ask the kernel immediately (through Netlink) if it knows the MAC address of a new client before doing the group lookups in the database. This should make recognizing clients by their MAC addresses finally pretty reliable.

Please test this some more, but I have the feeling that this feature has become ready for inclusion in the v5.3 release.