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

Remove zone size for invalid backend ref #1931

Merged
merged 5 commits into from
May 7, 2024

Conversation

bjee19
Copy link
Contributor

@bjee19 bjee19 commented May 3, 2024

Proposed changes

Remove zone size for invalid backend ref.

Problem: The invalid-backend-ref size of 32k is too small for some environments causing an inability of the gateway to program.

Solution: Do not specify zone size for invalid-backend-ref so that NGINX does not share upstream state across backends. This is ok for this specific upstream because we will only ever proxy to one backend.

Testing: Adjusted unit test to match. Deployed NGF with HTTPRoutes and verified it correctly still generates zone sizes for normal upstreams, an upstream missing its endpoint (service is present but no deployment/pod), and invalid-backend-ref:

upstream default_coffee_80 {
   random two least_conn;
   zone default_coffee_80 512k;

   server 10.244.0.25:8080;
}

upstream default_tea_80 {
   random two least_conn;
   zone default_tea_80 512k;

   server unix:/var/lib/nginx/nginx-502-server.sock;
}

upstream invalid-backend-ref {
   random two least_conn;

   server unix:/var/lib/nginx/nginx-500-server.sock;
}

Note: This PR does not test if the fix would work in the bug-submitter's environment. Recreating the exact test environment is probably not worth the effort.

Closes #1794

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.

Remove zone size for invalid-backend-ref

@bjee19 bjee19 requested a review from a team as a code owner May 3, 2024 22:00
@github-actions github-actions bot added the bug Something isn't working label May 3, 2024
@bjee19
Copy link
Contributor Author

bjee19 commented May 3, 2024

I went with @pleshakov 's suggestion:

One approach to increase the zone is to just not specify. In that case, NGINX will not share upstream state across backends. Which is ok for that upstream, since we only proxy to one destination.

Am I missing anything else?

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.

@bjee19 In the PR description you mentioned that you changed a unit test but I don't see any test changes....

internal/mode/static/nginx/config/upstreams.go Outdated Show resolved Hide resolved
@bjee19
Copy link
Contributor Author

bjee19 commented May 3, 2024

Oops, by changing the constant invalidBackendZoneSize which is used in the upstreams_test.go my brain thought of that as me changing the test, but there was no actual change in code in the test file.

Copy link

codecov bot commented May 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.71%. Comparing base (4cb9578) to head (18d635a).
Report is 1 commits behind head on main.

❗ Current head 18d635a differs from pull request most recent head 80f1b06. Consider uploading reports for the commit 80f1b06 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1931      +/-   ##
==========================================
- Coverage   86.72%   86.71%   -0.01%     
==========================================
  Files          88       88              
  Lines        5972     5971       -1     
  Branches       50       50              
==========================================
- Hits         5179     5178       -1     
  Misses        741      741              
  Partials       52       52              

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

@bjee19 bjee19 force-pushed the fix/increase-invalid-backend-ref-size branch from 18d635a to c80c559 Compare May 7, 2024 15:24
@bjee19 bjee19 force-pushed the fix/increase-invalid-backend-ref-size branch from c80c559 to 80f1b06 Compare May 7, 2024 15:26
@bjee19 bjee19 merged commit b07458d into nginxinc:main May 7, 2024
38 checks passed
@bjee19 bjee19 deleted the fix/increase-invalid-backend-ref-size branch May 7, 2024 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working release-notes
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

zone "invalid-backend-ref" is too small
3 participants