Passing quoted list to PIHOLE_DNS_ via docker-compose environment will break startup

[kylekurz]

This is a: Bug

Details

If you pass in a single DNS entry with quotes around it: - PIHOLE_DNS_='1.1.1.1', that works. If you pass in a list, it doesn't, as '- PIHOLE_DNS_='1.1.1.1;1.0.0.1'` yields:

PIHOLE_DNS_1='1.1.1.1
PIHOLE_DNS_2=1.0.0.1'

and the pihole container will exit.

My proposal is to remove all single and double-quotes before splitting the array, so that we're left with a value on $PIHOLE_DNS_ that is 1.1.1.1;1.0.0.1 in the example above. Once the string is cleaned up, the rest of the parsing can continue as it previously has. Note, this does not seem to be an issue with the raw docker run CLI, I'm only seeing it via compose files.

Related Issues

• I have searched this repository/Pi-hole forums for existing issues and pull requests that look similar

See discussion on #816 for more details.

These common fixes didn't work for my issue

• [x ] I have tried removing/destroying my container, and re-creating a new container
• [x ] I have tried fresh volume data by backing up and moving/removing the old volume data
• [x ] I have tried running the stock docker run example(s) in the readme (removing any customizations I added)
• I have tried a newer or older version of Docker Pi-hole (depending what version the issue started in for me)
• [x ] I have tried running without my volume data mounts to eliminate volumes as the cause

If the above debugging / fixes revealed any new information note it here.
Add any other debugging steps you've taken or theories on root cause that may help.

[PromoFaux]

What is the proper / best practice way to define environment variables in a compose script? Because it seems to accept multiple ways!

For documentation's sake:

Your example:

Without the quotes:

Defining it differently with/without quotes:

Edit:

Another....

[kylekurz]

@dschaper I think we can modify the if side of the block like this:

    echo "Setting DNS servers based on PIHOLE_DNS_ variable"
    # Strip out any string delimiters
    STRIPPED=$(echo $PIHOLE_DNS_ | tr -d \'\")
    # Split into an array (delimited by ;)
    PIHOLE_DNS_ARR=(${STRIPPED//;/ })
    count=1
    for i in "${PIHOLE_DNS_ARR[@]}"; do
        change_setting "PIHOLE_DNS_$count" "$i"
        ((count=count+1))
    done

Thoughts on that? I'm happy to PR it, but I'm not exactly a bash expert either.

[kylekurz]

@PromoFaux that's a great question. I was single-quoting based off the existing example for TZ that's in the README:

    environment:
      TZ: 'America/Chicago'

[PromoFaux]

Ah yes, sure, but note that that is NAME: 'value' as opposed to - NAME='value' - see how in my screen grabs the only time the quotes get pulled through are with the latter.

I'm just reading up to see if there is an expected way to do it.

[dschaper]

Promo: It's down to YAML, and really there should be no quotes.
https://stackoverflow.com/a/22235064

But there's two things that I think are being conflated.

1. YAML Scalars in the format of key:value

TZ: 'America/Chicago' (Note the colon)

2. YAML lists in the format of - value

environment:
  - THIS IS JUST A VALUE HERE

So what happens is:

- PIHOLE_DNS_='1.2.3.4;4.3.2.1'

is passed in to the environment exactly as written. Same as if you were to type:

PIHOLE_DNS_='1.2.3.4;4.3.2.1'

at the shell prompt.

A better test would be to set the different styles of environment with the different quotes and lack of quotes along with the semicolons and octothorpes and then run docker exec ... | echo $PIHOLE_DNS_ to see what the environment looks like.

[PromoFaux]

So, there doesn't seem to be a single way to do it, in a lot of places I'm seeing the following:

• NAME: 'value'
• NAME: value
• - NAME=value

But I'm never seeing anywhere where quotes are used on the right hand side of an =

(I wrote this just before Dan replied, but I'm hitting comment anyway)

[dschaper]

Sorry, please substitute tag for scalar in my post.

[dschaper]

Now, apparently docker-compose will handle either lists or dictionary key:value pairs.

https://docs.docker.com/compose/compose-file/compose-file-v3/#environment

[PromoFaux]

ffs. I was looking at that exact page and skimmed right past that section!

But that is pretty clear, I think. @kylekurz, what you're seeing is expected, if you use one of the correct ways of defining the variables, the issue will go away.

That all said, there is some merit in us using the already sourced valid_ip and valid_ip6 functions to sanitise the input like so:

which, in the case of your example would look like:

container would still be operational because it would fall back to default 8.8.8.8 (PIHOLE_DNS_2 being blank in this case is expected)

PR incoming...

[kylekurz]

@PromoFaux @dschaper thanks for digging in on that. I’ll update my files to do it “right”. That said, I’m a little concerned about falling back instead of failing. If I’m specifying my DNS to this tool that is all focused on helping with my privacy, falling back to Google DNS instead strikes me as anathema. Should we just continue to let it fail if the input doesn’t parse correctly?

[PromoFaux]

To reduce the barrier to entry - out of the box we need to provide something. Google is to one person what Microsoft is to another - we could wax lyrical about which companies = bad to have any of your data all day long, but ultimately it is down to the user to ensure they've configured it correctly if they want it to behave in a specific way.
Is Pi-hole a privacy focussed tool? Not inherently. It's a blocking DNS proxy - how you use it is up to you :)
Can Pi-hole be a privacy focussed tool? Absolutely. See last point.

TL;DR - it has to do something out of the box, or less experienced people will shrug it off as a product that doesn't work. Google just happens to be the DNS provider that was set as the default for no other reason than in 2016 that was the first one that came to mind.

AGAIN though, that all said I can see some merit, perhaps, in a simple check to the number of valid entries passed in through PIHOLE_DNS_. If none are detected, then we crash out. Perhaps.

[kylekurz]

@PromoFaux point taken. I think the difference is “nothing provided” vs “bad info provided”. My vote would be default to what you want (Google is fine), but crash if the user config is wrong and force them to fix it. Thanks again for discussing with me and letting me contribute!

[PromoFaux]

I've added commit to my PR to hard crash if the PIHOLE_DNS_ is full of gibberish

f6b9716

And no problem, it's good to hear different ideas on things - sometimes we can get too close to the code and not see the issues as clearly as a fresh set of eyes. I've lost count of the amount of mistakes I made (and still make!!) when I started contributing to Pi-hole 6 years ago.... ('kinell, where has the time gone??), I've learned to just roll with it ;)

[logopk]

Apparently breaks working old config with:

    environment:
      - DNS1='10.1.0.2#5054'

(which is my cloudflared on an internal docker network)

pi-hole        | Converting DNS1 to PIHOLE_DNS_
pi-hole        | Converting DNS2 to PIHOLE_DNS_
pi-hole        | Setting DNS servers based on PIHOLE_DNS_ variable
pi-hole        | Invalid IP detected in PIHOLE_DNS_: '10.1.0.2#5054'
pi-hole        | Invalid IP detected in PIHOLE_DNS_: ''
pi-hole        | No Valid IPs dectected in PIHOLE_DNS_. Aborting

[PromoFaux]

Per the discussion above, you're actually defining the environment variable incorrectly. it should be either:

• DNS1: '10.1.0.2#5054'
• DNS1: 10.1.0.2#5054
• - DNS1=10.1.0.2#5054

Some additional code has been added to ensure that the values passed through are valid, and the way you are passing it through is also bringing through the quotes, which is not valid.

Secondly, per the Readme, you should be using the PIHOLE_DNS_ env variable like.

[logopk]

Well I used to configure like the recommendation on https://hub.docker.com/r/pihole/pihole

And apparently my config did work for quite a long time.

Needs to be updated i think.

[PromoFaux]

Actually there are some changes to the Readme on this repo that need to go up to docker hub (i'll do that in about 10 mins) but please point out where it says on docker hub to set the environment like - DNS1='10.1.0.2#5054' :)

[PromoFaux]

I've updated it. Sorry, didn't mean to be combative in my last. The DNS1 comment was secondary, as one can still use it, it just gets converted to PIHOLE_DNS_ per the lines in your paste above:

pi-hole        | Converting DNS1 to PIHOLE_DNS_
pi-hole        | Converting DNS2 to PIHOLE_DNS_

I guess you were lucky that it worked before, but the point is that environment variables are expected to be passed in a very limited number of ways https://docs.docker.com/compose/compose-file/compose-file-v3/#environment