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.

Sign up for GitHub

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

Jump to bottom

Fix addKey to handle substrings of existing keys #5211

Merged
merged 2 commits into from
Mar 21, 2023
Merged

Fix addKey to handle substrings of existing keys #5211

merged 2 commits into from
Mar 21, 2023

Conversation

wblew
Copy link
Contributor

@wblew wblew commented Mar 17, 2023 •
edited
Loading

What does this PR aim to accomplish?:

This PR intends to address the case where a second upstream DNS server's address is a substring of the first upstream DNS
server's address. For example: using the Pi-hole docker image, with PIHOLE_DNS_="192.168.1.178;192.168.1.1".

How does this PR accomplish the above?:

Fix addKey in utils.sh to handle the case where a key is being added, and that key is the leading substring of an already existing key within that file. The new test test_key_addition_substr is added, to verify this behaviour is correct.

For example: add "server=192.168.1.1", when "server=192.168.1.178" already exists within the etc/dnsmasq.d/01-pihole.conf file.

Check pihole docker with PIHOLE_DNS_="192.168.1.178;192.168.1.1". Its /etc/dnsmasq/01-pihole.conf will be missing its second server= entry.

Link documentation PRs if any are needed to support this PR:

None necessary, this is a bug fix. There are no intended changes in observable behaviour.

By submitting this pull request, I confirm the following:

  1. I have read and understood the contributors guide, as well as this entire template. I understand which branch to base my commits and Pull Requests against.
  2. I have commented my proposed changes within the code and I have tested my changes.
  3. I am willing to help maintain this change if there are issues with it later.
  4. It is compatible with the EUPL 1.2 license
  5. I have squashed any insignificant commits. (git rebase)
  6. I have checked that another pull request for this purpose does not exist.
  7. I have considered, and confirmed that this submission will be valuable to others.
  8. I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  9. I give this submission freely, and claim no ownership to its content.
  • [XXX ] I have read the above and my PR is ready for review. Check this box to confirm

@wblew wblew changed the base branch from master to development March 17, 2023 04:32
@wblew
Copy link
Contributor Author

wblew commented Mar 17, 2023

I added a pretty heft comment to the addKey function, to describe the situation. Depending on project expectations, that comment might need trimming, or even complete removal.

Such a decision is better in the hands of more experienced project contributors.

yubiuser
yubiuser reviewed Mar 17, 2023
View reviewed changes
Copy link
Member

@yubiuser yubiuser 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 PR. This clearly an issue which should be fixed.
Maybe it's easier just to add the anchor at the end of the line as well?

advanced/Scripts/utils.sh Outdated Show resolved Hide resolved
@wblew wblew closed this Mar 17, 2023
@yubiuser
Copy link
Member

Why did you close the PR?
__

I think the long comment can go now and please squash all your commits down. I think afterwards it's ready to go...

@wblew
Copy link
Contributor Author

wblew commented Mar 17, 2023 •
edited
Loading

Why did you close the PR? __

I think the long comment can go now and please squash all your commits down. I think afterwards it's ready to go...

Because I am an inexperienced idiot? How do I get it open again?

@yubiuser yubiuser reopened this Mar 17, 2023
yubiuser
yubiuser previously approved these changes Mar 17, 2023
View reviewed changes
@yubiuser yubiuser requested a review from dschaper March 17, 2023 12:23
@dschaper
Copy link
Member

Sorry, I'd like the comment to be there please.

@PromoFaux This potentially affects environment variables for docker. Please check this over.

@wblew
Fix addKey to handle substrings of existing keys
b9a6970 
Fix addKey to handle the case where a key is being added, and that key
is the leading substring of an already existing key within that file.

For example: add "server=192.168.1.1", when "server=192.168.1.178"
already exists within the /etc/dnsmasq.d/01-pihole.conf file.

Check pihole docker with PIHOLE_DNS="192.168.1.178;192.168.1.1". Its
/etc/dnsmasq/01-pihole.conf will be missing its second server= entry.

Add the test_key_addition_substr, to test addKey when
its adding a substring key of an existing key in the file.

Signed-off-by: William Blew <william@kulian.org>
@wblew
restore the addKey comment, reworded for anchors
3c91b65 
Per @dschaper, restore the addKey clarifying comment. It has
been reworded to describe the use of anchors where before it
referenced using grep's  'match only an entire line' argument.

Signed-off-by: William Blew <william@kulian.org>
@wblew
Copy link
Contributor Author

wblew commented Mar 17, 2023

Sorry, I'd like the comment to be there please.

@PromoFaux This potentially affects environment variables for docker. Please check this over.

I failed to add my signoff, in the first attempt. Here is another try to restore the comment, with my signoff.

@PromoFaux
Copy link
Member

I started off a little confused as to what was being fixed/changed here..

The issue you describe with docker does not exist as you describe it - at least, I cannot reproduce it:

image

Which is because they are added as PIHOLE_DNS_1=, and PIHOLE_DNS_2= respectively...

But then I re-read the issue and realised that the issue is actually nothing to do with docker at all, rather it is when setupVars.conf is transposed into /etc/dnsmasq.d/01-pihole.conf in webpage.sh, here:

pi-hole/advanced/Scripts/webpage.sh

Line 157 in d86b325

addKey "${dnsmasqconfig}" "server=${!var}"

That was all a very long way of saying "I've tested this and confirm it does what it says on the tin"

@wblew
Copy link
Contributor Author

wblew commented Mar 17, 2023

Yes, my problem statement is woefully incomplete. Thus causing confusion. Why is hindsight always so clear?

@PromoFaux PromoFaux merged commit 4d21bae into pi-hole:development Mar 21, 2023
@wblew
Copy link
Contributor Author

wblew commented Mar 21, 2023 •
edited
Loading

Thank you all. For your time, patience, and advice.

@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/pi-hole-ftl-v5-22-web-v5-19-and-core-v5-16-1-released/61999/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PromoFaux PromoFaux PromoFaux approved these changes

@yubiuser yubiuser yubiuser left review comments

@dschaper dschaper Awaiting requested review from dschaper

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@wblew @yubiuser @dschaper @PromoFaux @pralor-bot