-
-
Notifications
You must be signed in to change notification settings - Fork 177
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
New configuration method for permit and deny lists #275
base: main
Are you sure you want to change the base?
New configuration method for permit and deny lists #275
Conversation
PS not sure why the diffs are being so unhelpful... |
update chart version minor cosmetic updates test tes add default comments test test test test test test update postStart command test test test test use bash test corrections move from sidecar to lifecycle handler move cmdline configurator out of initContainer use Sidecar init cmdlist initContainers correction update values file correction single quote regex correct capcase correction fix indentation add cmdlist method for permit deny lists
995b2df
to
b000106
Compare
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 Just one issue I observed: I did frequent installs and deletes and sometimes the pod got stuck in the |
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. |
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 A sidecar container was the first thing that came to my mind as well. I'll have a look if I find another solution. |
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.