MoJo2600 / pihole-kubernetes

PiHole on kubernetes
493 stars 171 forks source link

New configuration method for permit and deny lists #275

Open specialcircumstances opened 7 months ago

specialcircumstances commented 7 months ago

The existing configuration method does not allow for whitelist regex (for example) and makes use of legacy file conversion at startup. This proposal adds a new configuration method, which creates a list of pihole command that are run after the pod starts. This allows for all six of the methods to be used. The additional method is not written as a replacement, but as an alternate option.

specialcircumstances commented 7 months ago

PS not sure why the diffs are being so unhelpful...

MoJo2600 commented 6 months ago

I had an in depth look into your pr and i really like it. I'd like to get rid of the obsolete way to define deny / permit lists and use your way in the v3 release.

I renamed the config option to align it a little more to the pihole GUI (menu entry domains vs cmdlist), but i refrain from using black/whitelist and rathe go with deny/permit.

Just one issue I observed: I did frequent installs and deletes and sometimes the pod got stuck in the Creating phase. I did not have this issue on my cluster before. My suspicion is, that your pihole ready state check is not working properly and the container gets stuck. But I'm not sure yet. Why are you using until grep "file renamed while open" and not the pihole status command?

specialcircumstances commented 6 months ago

Just one issue I observed: I did frequent installs and deletes and sometimes the pod got stuck in the Creating phase. I did not have this issue on my cluster before. My suspicion is, that your pihole ready state check is not working properly and the container gets stuck. But I'm not sure yet. Why are you using until grep "file renamed while open" and not the pihole status command?

Glad you like it :)

I'm not seeing the pod getting stuck, so would be interested in seeing a bit more detail there, but yes, this is a bit of a troublesome area...

The underlying challenge is the startup behaviour of the pihole container, which looks for legacy configuration that it then converts into the newer DB driven configuration. This means that the container starts up with an empty DB, then reads the legacy configuration files, converts them into a new DB and then loads it (more or less I think). The configuration script this PR runs needs to happen after that process has completed.

Using pihole status (which a previous iteration did) didn't work because pihole status went "good" before the DB reload, and so although the configuration script worked, all the configuration it added was then dumped and replaced (took me a while to figure that out and hence the >> /var/log/domains.log ).

The "file renamed while open" is watching for that DB replacement, after which time I know it's safe to add in the additional configuration.

I guess there could be scenarios in which this "file renamed while open" doesn't occur because there is no need to do it. I haven't looked in depth at pihole to see if that's the case, but I seems the most likely explanation for what you see.

There are also some gotchas to using the spec.lifecycle.postStart in that the Pod will not become Ready until it's complete, which has implications for the Service becoming available until the lifecycle is complete, so it's not possible to use that as an indicator either (loop condition).

I'd absolutely agree the configuration trigger condition is a bit of a kludge - and it would be great to find a better one!

An alternate way of doing the configuration might be to use a Sidecar Container but that's beta in k8s 1.29 so feels a bit too exclusive for this at this point.

MoJo2600 commented 6 months ago

I was testing the other pr about disabling the admin password and was installing the container multiple times. It never got stuck in the creating state. So I think it is this pr which causes the issue. When I do a docker ps the container seems to run.

A sidecar container was the first thing that came to my mind as well. I'll have a look if I find another solution.