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.

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

[BUGFIX] Update nginx deployment to mount nginx.conf as file #4954

Closed
wants to merge 1 commit into from

Conversation

blut
Copy link
Contributor

@blut blut commented May 8, 2023

What this PR does

At the moment, the nginx-config configmap is mounted as folder, masking everything else that is located in /etc/nginx.
I'm trying to load a module in the nginx.conf with load_module modules/ngx_http_js_module.so;.

With the current setup, this fails with the following logs:
/docker-entrypoint.sh: No files found in /docker-entrypoint.d/, skipping configuration 2023/05/09 11:46:27 [emerg] 1#1: dlopen() "/etc/nginx/modules/ngx_http_js_module.so" failed (/etc/nginx/modules/ngx_http_js_module.so: cannot open shared object file: No such file or directory) in /etc/nginx/nginx.conf:1 nginx: [emerg] dlopen() "/etc/nginx/modules/ngx_http_js_module.so" failed (/etc/nginx/modules/ngx_http_js_module.so: cannot open shared object file: No such file or directory) in /etc/nginx/nginx.conf:1

With the proposed change, the nginx.conf file is still mounted to /etc/nginx/nginx.conf, however loading modules is now possible.

Unfortunately, I did not find any parameters to nginx which would allow an alternative module path (e.g. in /usr/lib/nginx/modules) and since the directory is mounted read-only, it is not possible copying the module through an entrypoint script.

Alternative fixes would include larger changes, such as moving the config volume & mount to the values.yaml

Which issue(s) this PR fixes or relates to

Fixes #

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@CLAassistant
Copy link

CLAassistant commented May 8, 2023

CLA assistant check
All committers have signed the CLA.

@blut blut changed the title Main [BUGFIX] Update nginx deployment to mount nginx.conf as file May 8, 2023
@56quarters 56quarters added the helm label May 8, 2023
@dimitarvdimitrov
Copy link
Contributor

hi, thanks for opening this PR. Can you provide some instructions to reproduce this bug? When I install the chart with the default options I see the configmap is mounted as a file in /etc/nginx/nginx.conf

@blut
Copy link
Contributor Author

blut commented May 9, 2023

Hi @dimitarvdimitrov, sorry, I thought this was still in draft mode.
I tried creating an issue, but I seem to have trouble with github.

At the moment, the nginx-config configmap is mounted as folder, masking everything else that is located in /etc/nginx.
I'm trying to load a module in the nginx.conf with load_module modules/ngx_http_js_module.so;.

With the current setup, this fails with the following logs:
/docker-entrypoint.sh: No files found in /docker-entrypoint.d/, skipping configuration 2023/05/09 11:46:27 [emerg] 1#1: dlopen() "/etc/nginx/modules/ngx_http_js_module.so" failed (/etc/nginx/modules/ngx_http_js_module.so: cannot open shared object file: No such file or directory) in /etc/nginx/nginx.conf:1 nginx: [emerg] dlopen() "/etc/nginx/modules/ngx_http_js_module.so" failed (/etc/nginx/modules/ngx_http_js_module.so: cannot open shared object file: No such file or directory) in /etc/nginx/nginx.conf:1

With the proposed change, the nginx.conf file is still mounted to /etc/nginx/nginx.conf, however loading modules is now possible.

Unfortunately, I did not find any parameters to nginx which would allow an alternative module path (e.g. in /usr/lib/nginx/modules) and since the directory is mounted read-only, it is not possible copying the module through an entrypoint script.

Alternative fixes would include larger changes, such as moving the config volume & mount to the values.yaml

Please let me know what you think.

@dimitarvdimitrov
Copy link
Contributor

I understand, thank you for explaining. In that case I think this change makes sense.

I ran the change locally and it appears that there are also other files and directories we were overwriting. All of the below apart from nginx.conf were overwritten.

/ $ ls -alh /etc/nginx/
total 40K
drwxrwxr-x    1 nginx    root        4.0K Apr 17 00:03 .
drwxr-xr-x    1 root     root        4.0K May  9 12:21 ..
drwxrwxr-x    1 nginx    root        4.0K Apr 17 00:03 conf.d
-rw-rw-r--    1 nginx    root        1.1K Nov 25 07:21 fastcgi.conf
-rw-rw-r--    1 nginx    root        1007 Nov 25 07:21 fastcgi_params
-rw-rw-r--    1 nginx    root        5.2K Nov 25 07:21 mime.types
lrwxrwxrwx    1 nginx    root          22 Apr 17 00:03 modules -> /usr/lib/nginx/modules
-rw-r--r--    1 root     10001       3.6K May  9 12:21 nginx.conf
-rw-rw-r--    1 nginx    root         636 Nov 25 07:21 scgi_params
-rw-rw-r--    1 nginx    root         664 Nov 25 07:21 uwsgi_params

Can I also ask you to carry out the change on the gateway nginx deployment too? It's located here

- name: nginx-config
mountPath: /etc/nginx

Also, to get the CI passing, you can run make build-helm-tests locally and commit and push the changes you get.

@pracucci
Copy link
Collaborator

Closing this PR due to inactivity, but please feel free to re-open it anytime if you plan to get back to this. Thanks!

@pracucci pracucci closed this Oct 10, 2023
@blut
Copy link
Contributor Author

blut commented Jan 17, 2024

Reopen with updated fixes

@aknuds1
Copy link
Contributor

aknuds1 commented Jan 17, 2024

@blut the Reopen button is grayed out even for maintainers (tooltip says "The main branch was force-pushed or recreated"). Is it the same for you? If neither of us can reopen the PR, maybe you should just recreate it.

@blut
Copy link
Contributor Author

blut commented Jan 17, 2024

Thank you, I will recreate the PR

@blut
Copy link
Contributor Author

blut commented Jan 17, 2024

Moved to #7150

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants