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

Build and use custom nginx container #934

Merged
merged 30 commits into from
Aug 10, 2023

Conversation

kate-osborn
Copy link
Contributor

Proposed changes

Replace the publicly available nginx image with a custom nginx image.

Problem: Using the publicly available nginx image requires users to create ConfigMaps for the nginx.conf file and the njs module and mount them to the NKG Pod as volumes. This pattern is not extensible and adds extra steps for developers and users. Additionally, an init container is required in order to set up the nginx config environment.

Solution: Build and use a custom nginx container. The nginx.conf and njs module are now baked into the nginx image. This eliminates the need for ConfigMaps. The config directories /etc/nginx/conf.d and /etc/nginx/secrets are created as volumes and mounted to the Pod with a group ID 1001. This allows the control plane to write to the directories and nginx to read from them. Both the nginx and nginx-gateway processes run under group ID 1001 but have different user IDs (101 and 102). The nginx container now runs as user 101 instead of root.

Testing: Verified conformance tests pass and that the directory permissions are correct:

/etc/nginx $ whoami
nginx
/etc/nginx $ ls -l
total 36
drwxrwsrwx    2 root     1001          4096 Aug  4 16:36 conf.d
-rw-r--r--    1 nginx    1001          1077 Jun 13 17:34 fastcgi.conf
-rw-r--r--    1 nginx    1001          1007 Jun 13 17:34 fastcgi_params
-rw-r--r--    1 nginx    1001          5349 Jun 13 17:34 mime.types
lrwxrwxrwx    1 nginx    1001            22 Jun 15 00:20 modules -> /usr/lib/nginx/modules
-rw-r--r--    1 nginx    1001           440 Aug  3 21:23 nginx.conf
-rw-r--r--    1 nginx    1001           636 Jun 13 17:34 scgi_params
drwxrwsrwx    2 root     1001          4096 Aug  4 16:36 secrets
-rw-r--r--    1 nginx    1001           664 Jun 13 17:34 uwsgi_params

Note
Marking this as draft because I'm still working on the pipeline steps and have not updated the architecture doc yet.

Closes #798

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

@github-actions github-actions bot added documentation Improvements or additions to documentation enhancement New feature or request labels Aug 4, 2023
Makefile Outdated Show resolved Hide resolved
build/Dockerfile.nginx Outdated Show resolved Hide resolved
build/Dockerfile.nginx Outdated Show resolved Hide resolved
conformance/Makefile Outdated Show resolved Hide resolved
conformance/Makefile Outdated Show resolved Hide resolved
deploy/helm-chart/templates/deployment.yaml Outdated Show resolved Hide resolved
build/Dockerfile.nginx Show resolved Hide resolved
docs/building-the-images.md Show resolved Hide resolved
docs/building-the-images.md Show resolved Hide resolved
internal/mode/static/nginx/runtime/manager.go Outdated Show resolved Hide resolved
Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

Copy link
Member

@ciarams87 ciarams87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The conformance CI file needs to be updated with the new container, as the conformance tests aren't running, but otherwise looks great! 🚀

@kate-osborn
Copy link
Contributor Author

The conformance CI file needs to be updated with the new container, as the conformance tests aren't running, but otherwise looks great! 🚀

Thanks! Just updated the conformance CI file and the tests are now passing. Not sure if there's a better way to build and reference the two images so LMK what you think!

@kate-osborn kate-osborn force-pushed the feat/build-custom-nginx-image branch from 3bbf0c6 to ea56a00 Compare August 8, 2023 18:24
@kate-osborn kate-osborn marked this pull request as ready for review August 8, 2023 21:27
@kate-osborn kate-osborn requested a review from a team as a code owner August 8, 2023 21:28
build/Dockerfile.nginx Outdated Show resolved Hide resolved
Copy link
Member

@lucacome lucacome left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a couple of suggestion for the CI, mostly about making it more compact, but the most important one is the cache, we need to use two different scopes for the containers.
Let me know if I can help making the changes.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
Copy link
Member

@ciarams87 ciarams87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 🚀

@kate-osborn kate-osborn merged commit f820591 into nginxinc:main Aug 10, 2023
22 checks passed
@kate-osborn kate-osborn mentioned this pull request Aug 11, 2023
6 tasks
kate-osborn added a commit that referenced this pull request Aug 11, 2023
Problem: PR #934 did not remove the njs-modules ConfigMap 
and they are some lingering references to it.

Solution: Remove the njs modules ConfigMap and remove references.
kate-osborn added a commit that referenced this pull request Aug 17, 2023
Problem: The architecture doc still mentions the init container even though it was removed in #934

Solution: Remove the init container from the architecture doc.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Build and use custom nginx container
5 participants