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

Add init container for setting up base config #2649

Merged
merged 2 commits into from
Oct 8, 2024

Conversation

sjberman
Copy link
Contributor

@sjberman sjberman commented Oct 4, 2024

Problem: We are starting to introduce configuration options that exist in the main context. However, that configuration won't be written until the control plane writes it to nginx, meaning it doesn't exist on nginx startup. Therefore nginx uses its default configuration for a brief time, which is incorrect.

We want to be able to provide this configuration on startup.

Solution: Using an init container, we can mount a ConfigMap containing the dynamic base config, and copy it to the proper location in the filesystem before nginx starts. We can't mount the ConfigMap directly to the proper location because it would be read-only, preventing our control plane from writing to it.

This allows us to bootstrap the user config into nginx on startup, while also allowing our control plane to overwrite it if the user ever changes the config after the fact.

Removed logic that cleared out nginx files on startup because it would erase this bootstrap config, and it wasn't really needed since we delete nginx files when we write config anyway.

Also fixed an issue where the log level was not honored when no Gateway resources existed.

Testing: Verified that the log level is set properly on startup.

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

Release notes

If this PR introduces a change that affects users and needs to be mentioned in the release notes,
please add a brief note that summarizes the change.

Added an init container to bootstrap nginx config on startup.

Problem: We are starting to introduce configuration options that exist in the main context. However, that
configuration won't be written until the control plane writes it to nginx, meaning it doesn't exist on
nginx startup. Therefore nginx uses its default configuration for a brief time, which is incorrect.

We want to be able to provide this configuration on startup.

Solution: Using an init container, we can mount a ConfigMap containing the dynamic base config, and copy it
to the proper location in the filesystem before nginx starts. We can't mount the ConfigMap directly to the proper location
because it would be read-only, preventing our control plane from writing to it.

This allows us to bootstrap the user config into nginx on startup, while also allowing our control plane to overwrite it
if the user ever changes the config after the fact.

Removed logic that cleared out nginx files on startup because it would erase this bootstrap config, and it wasn't really
needed since we delete nginx files when we write config anyway.

Also fixed an issue where the log level was not honored when no Gateway resources existed.
@sjberman sjberman requested a review from a team as a code owner October 4, 2024 19:44
@github-actions github-actions bot added enhancement New feature or request helm-chart Relates to helm chart labels Oct 4, 2024
Copy link

codecov bot commented Oct 4, 2024

Codecov Report

Attention: Patch coverage is 58.53659% with 17 lines in your changes missing coverage. Please review.

Project coverage is 88.56%. Comparing base (f8ab3e0) to head (2837992).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
cmd/gateway/commands.go 56.75% 15 Missing and 1 partial ⚠️
cmd/gateway/main.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2649      +/-   ##
==========================================
- Coverage   88.66%   88.56%   -0.11%     
==========================================
  Files         106      106              
  Lines        8141     8174      +33     
  Branches       50       50              
==========================================
+ Hits         7218     7239      +21     
- Misses        866      877      +11     
- Partials       57       58       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@bjee19 bjee19 left a comment

Choose a reason for hiding this comment

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

👍

@sjberman sjberman enabled auto-merge (squash) October 8, 2024 18:26
@sjberman sjberman merged commit 30bdeb3 into nginxinc:main Oct 8, 2024
44 of 45 checks passed
@sjberman sjberman deleted the feat/init-container branch October 8, 2024 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request helm-chart Relates to helm chart
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants