nginx-gateway-fabric icon indicating copy to clipboard operation
nginx-gateway-fabric copied to clipboard

Remove zone size for invalid backend ref

Open bjee19 opened this issue 9 months ago • 3 comments

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.

  • [x] I have read the CONTRIBUTING doc
  • [x] I have added tests that prove my fix is effective or that my feature works
  • [x] I have checked that all unit tests pass after adding my changes
  • [ ] I have updated necessary documentation
  • [x] I have rebased my branch onto main
  • [x] 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 avatar May 03 '24 22:05 bjee19

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?

bjee19 avatar May 03 '24 22:05 bjee19

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.

bjee19 avatar May 03 '24 22:05 bjee19

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

:exclamation: 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              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar May 03 '24 22:05 codecov[bot]