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 configurable data plane log level #2603

Merged
merged 5 commits into from
Oct 4, 2024

Conversation

bjee19
Copy link
Contributor

@bjee19 bjee19 commented Sep 25, 2024

Add Nginx error log level to NginxProxy.

Problem: Users would like to assign a log level for the data plane.

Solution: Add Nginx error log level to NginxProxy which allows users to set the error log level for Nginx.

Testing: Unit tests and manual testing.

Manual testing included

  • Validating the enum values in the new ErrorLevel field in the NginxProxy resource.
  • Deploying NGF, setting up the cafe-app, deploying an NginxProxy resource, and testing each of the possible log levels and checking that the nginx conf (the main.conf file and running nginx -T) in the nginx container was updated correctly. NGINX Debug logs work.
  • Checked that if the error level was not set in the NginxProxy resource, the nginx conf would be correctly updated back to the default of info log level.
  • Tested that if a log level was set to something "beneath" notice such as the error level, after making changes which would result in notice log levels being outputted (such as deleting and re-applying cafe.yaml in the cafe example), no notice log levels are outputted.
  • nginx -T correctly shows the main.conf configuration file being included in the nginx configuration.
  • ps aux |grep nginx when inside the nginx container correctly showed nginx-debug processes.
  • Deployed the cafe example and deleted it a few times and NGF still worked properly with no error logs in either container.
  • Dry run of Helm template shows correct formatting.
  • If NGF is run with NGINX in debug mode (nginx.debug), but the log level is not debug, NGF still works correctly. Also NGINX debug logs are not printed.
  • All of the above worked with NGINX Plus.

Closes #901

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.

Add ability to assign a log level for the data plane

@bjee19 bjee19 requested review from a team as code owners September 25, 2024 17:14
@github-actions github-actions bot added documentation Improvements or additions to documentation enhancement New feature or request helm-chart Relates to helm chart labels Sep 25, 2024
@bjee19
Copy link
Contributor Author

bjee19 commented Sep 25, 2024

Opening this and closing #2561 in order for the NGINX Plus image to be built correctly with the repo's certs.

Copy link
Contributor

Deploy Preview will be available once build job completes!

Name Link
😎 Deploy Preview https://frontdoor-test-docs.nginx.com/previews/nginx-gateway-fabric/2603/

charts/nginx-gateway-fabric/README.md Outdated Show resolved Hide resolved
charts/nginx-gateway-fabric/templates/deployment.yaml Outdated Show resolved Hide resolved
charts/nginx-gateway-fabric/templates/deployment.yaml Outdated Show resolved Hide resolved
charts/nginx-gateway-fabric/values.yaml Outdated Show resolved Hide resolved
examples/nginxproxy-example/nginxproxy-error-loglevel.yaml Outdated Show resolved Hide resolved
site/content/how-to/data-plane-configuration.md Outdated Show resolved Hide resolved
site/content/how-to/data-plane-configuration.md Outdated Show resolved Hide resolved
site/content/installation/installing-ngf/manifests.md Outdated Show resolved Hide resolved
site/content/installation/installing-ngf/manifests.md Outdated Show resolved Hide resolved
site/content/how-to/data-plane-configuration.md Outdated Show resolved Hide resolved
@bjee19 bjee19 force-pushed the enh/configure-data-plane-log-level branch 2 times, most recently from d34a1f8 to 226cc61 Compare September 30, 2024 22:27
Copy link

codecov bot commented Sep 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.66%. Comparing base (b41bad9) to head (58e48b1).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2603      +/-   ##
==========================================
+ Coverage   88.62%   88.66%   +0.03%     
==========================================
  Files         105      106       +1     
  Lines        8116     8141      +25     
  Branches       50       50              
==========================================
+ Hits         7193     7218      +25     
  Misses        866      866              
  Partials       57       57              

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

@bjee19
Copy link
Contributor Author

bjee19 commented Sep 30, 2024

Needed to squash and rebase so the commit trail is gone, no major changes to actual functionality or code since previous review. Only big changes has been adding the Helm schema to the chart's values.yaml and documentation changes.

@github-actions github-actions bot added the tests Pull requests that update tests label Sep 30, 2024
Copy link
Contributor

@kate-osborn kate-osborn left a comment

Choose a reason for hiding this comment

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

Approving, but see my review for a couple of small suggestions. Nice work!

internal/mode/static/nginx/config/generator.go Outdated Show resolved Hide resolved
site/content/how-to/data-plane-configuration.md Outdated Show resolved Hide resolved
@bjee19 bjee19 requested a review from sjberman October 3, 2024 18:52
@bjee19 bjee19 force-pushed the enh/configure-data-plane-log-level branch from 8d4f608 to 58e48b1 Compare October 4, 2024 14:47
@bjee19 bjee19 merged commit c7a5b3b into main Oct 4, 2024
53 checks passed
@bjee19 bjee19 deleted the enh/configure-data-plane-log-level branch October 4, 2024 15:09
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 helm-chart Relates to helm chart tests Pull requests that update tests
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Configurable Data Plane Log-Level
5 participants