-
-
Notifications
You must be signed in to change notification settings - Fork 9
ISSUE #239: Implement dynamic mechanism to capture local dns resolver and update nginx.conf resolver configuration #240
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements a dynamic DNS resolver configuration system for nginx by automatically extracting the local DNS resolver IP address from /etc/resolv.conf
and updating the nginx configuration template, replacing the hardcoded 127.0.0.11
resolver.
- Adds a shell script to dynamically capture DNS resolver addresses from
/etc/resolv.conf
- Converts the static nginx configuration to a template-based approach using environment variable substitution
- Updates the Docker build process to execute the DNS resolver detection script during container startup
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
Dockerfile | Updates build to use nginx.conf.template and adds execution permissions for the new DNS resolver script |
.docker/scripts/90-awk-nginx-conf-with-resolv-conf.sh | New script that extracts DNS resolvers from /etc/resolv.conf and generates nginx.conf from template |
.docker/nginx.conf.template | Converts hardcoded resolver to use NGX_DNS_RESOLVER environment variable placeholder |
I gave a quick look at the nginx repo. Looks like this logic might already be built-in? The resolver addition was ported after implementing the fix in HA add-on (that definitely needs it), but could be it just needs to be removed completely in plain Docker/Kube? Seeing the above, I'm thinking it might already be handled internally... |
@mmssantos were you able to check if the |
Haven't been able to get my hands dirty on it again so far. |
container image mechanism
Apologies for the delay, time as been running low lately. The entrypoint script "15-local-resolvers.envsh" present in the base nginx image is there to support these types of scenarios, requiring that an environment variable "NGINX_ENTRYPOINT_LOCAL_RESOLVERS" is set to true to activate. I have added a few changes to the repo that, when the "NGINX_ENTRYPOINT_LOCAL_RESOLVERS" is set to true will grab the locally configured dns servers using the native script in the original nginx container image, substitute the placeholder "NGINX_LOCAL_RESOLVERS" in the added nginx.conf.template with the captured dns servers and then substitute the nginx.conf with the contents of the nginx.conf.template. |
…ntainer image mechanism
We shouldn't duplicate the nginx.conf. Best to set the default in the script as needed and always use the template. Less maintenance and less error-prone. |
hardcoded dns server "127.0.0.11" will be used, if it is set, local dns servers will be grabbed and configured
Added additional logic to the ".docker/scripts/100-envsubst-on-nginx-conf.sh" to fallback to "127.0.0.11" in case the environment variable "NGINX_ENTRYPOINT_LOCAL_RESOLVERS" is not set. |
Can you take a look at the commit I pushed, from what I gathered from the container scripts, something like this should be the cleanest approach? Using a custom solution will bypass the init scripts of the container. I think we should avoid that, keep the behavior in line with the base container, keep just a thing wrapper. |
@Nerivec Reviewing all of the existing scripts inside the official Docker image and a few Issues on their github, it appears that the combination of the environment variable "NGINX_ENTRYPOINT_LOCAL_RESOLVERS=True", "15-local-resolvers.envsh", "20-envsubst-on-templates.sh" and a well placed .template file with a "resolver $NGINX_LOCAL_RESOLVERS ipv6=off;" directive on /etc/nginx/templates will indeed accomplish what we are looking for. However, the way things are built at this point in time in the zigbee2mqtt-windfront container image will cause some issues, given that the official mechanism inside nginx will convert the template into a .conf inside /etc/nginx/conf.d. |
That last commit includes conf.d/resolver.conf in nginx.conf |
Can confirm that with the changes in the last commit and the "NGINX_ENTRYPOINT_LOCAL_RESOLVERS=True" environment variable, everything works like expected. |
Took a quick shot at implementing an automatic mechanism to programmatically grab the local dns resolver ip address from /etc/resolv.conf and update the nginx.conf file resolver directive with it, removing the hardcoded configuration.
Tested on Kubernetes and on a local docker instance and working fine.
Probably needs a bit more testing and script sanitization.