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

fix: Fix reload errors due long matching conditions #1829

Merged
merged 29 commits into from Apr 29, 2024

Conversation

salonichf5
Copy link
Contributor

@salonichf5 salonichf5 commented Apr 15, 2024

Proposed changes

Problem: A large number of match conditions for a single hostname/path cause reload errors with NGINX about being parameter being too long

Solution: The solution is to use js_preload_objects to store the http_matches and then parse them through NGINX using NJS to map the correct match condition.

Changes:

  • writes a new file match.json to /etc/nginx/conf.d
  • stores the http_matches as key-value pairs in match.json
  • the key is sanitized to remove /, . , and replace = with EXACT. Reasons being NJS throws variable errors if the key contains /, . and having a = confuses NGINX separating the key into two words.
  • Updated the unit tests.

Testing: Manual testing

sa.choudhary@N9939CQ4P0 advanced-routing % curl --resolve cafe.example.com:$GW_PORT:$GW_IP http://cafe.example.com:$GW_PORT/coffee
Handling connection for 8080
Server address: 10.244.0.57:8080
Server name: coffee-v1-76c7c85bbd-48lnh
Date: 17/Apr/2024:16:22:45 +0000
URI: /coffee
Request ID: 3ab675f41960b2d9f43a160e87c39847

sa.choudhary@N9939CQ4P0 advanced-routing % curl --resolve cafe.example.com:$GW_PORT:$GW_IP http://cafe.example.com:$GW_PORT/coffee -H "version:v2"
Handling connection for 8080
Server address: 10.244.0.56:8080
Server name: coffee-v2-7d47fc86cb-q7ltw
Date: 17/Apr/2024:16:24:16 +0000
URI: /coffee
Request ID: 8a83e167202a3199ee6ab8d952ba20cc

sa.choudhary@N9939CQ4P0 advanced-routing % curl --resolve cafe.example.com:$GW_PORT:$GW_IP http://cafe.example.com:$GW_PORT/coffee\?TEST\=v2
Handling connection for 8080
Server address: 10.244.0.56:8080
Server name: coffee-v2-7d47fc86cb-q7ltw
Date: 17/Apr/2024:16:24:30 +0000
URI: /coffee?TEST=v2
Request ID: 3c9c54d47f450f0829518adb7a53d5c2

sa.choudhary@N9939CQ4P0 advanced-routing % curl --resolve cafe.example.com:$GW_PORT:$GW_IP http://cafe.example.com:$GW_PORT/tea
Handling connection for 8080
Server address: 10.244.0.59:8080
Server name: tea-9d8868bb4-v7hkr
Date: 17/Apr/2024:16:24:54 +0000
URI: /tea
Request ID: b38b7778054e19bddf9c9424b50eae91

sa.choudhary@N9939CQ4P0 advanced-routing % curl --resolve cafe.example.com:$GW_PORT:$GW_IP http://cafe.example.com:$GW_PORT/tea -X POST
Handling connection for 8080
Server address: 10.244.0.58:8080
Server name: tea-post-7677b4cf6d-rvq7r
Date: 17/Apr/2024:16:25:11 +0000
URI: /tea
Request ID: 8beb0f04c2eea53539cf16cd438cca2b

Testing with long matching conditions httproute

sa.choudhary@N9939CQ4P0 bug % curl -X GET \
--resolve cafe.example.com:$GW_PORT:$GW_IP \
-H "header-1: header-1-val" \
-H "header-2: header-2-val" \
-H "header-3: header-3-val" \
-H "header-4: header-4-val" \
-H "header-5: header-5-val" \
-H "header-6: header-6-val" \
-H "header-7: header-7-val" \
-H "header-8: header-8-val" \
-H "header-9: header-9-val" \
-H "header-10: header-10-val" \
-H "header-11: header-11-val" \
-H "header-12: header-12-val" \
-H "header-13: header-13-val" \
-H "header-14: header-14-val" \
-H "header-15: header-15-val" \
-H "header-16: header-16-val" \
"http://cafe.example.com:$GW_PORT/?param-1=param-1-val&param-2=param-2-val&param-3=param-3-val&param-4=param-4-val&param-5=param-5-val&param-6=param-6-val&param-7=param-7-val&param-8=param-8-val&param-9=param-9-val&param-10=param-10-val&param-11=param-11-val&param-12=param-12-val&param-13=param-13-val&param-14=param-14-val&param-15=param-15-val&param-16=param-16-val"

Handling connection for 8080
backend
sa.choudhary@N9939CQ4P0 bug % curl -X POST \
--resolve cafe.example.com:$GW_PORT:$GW_IP \
-H "header-1: header-1-val" \
-H "header-2: header-2-val" \
-H "header-3: header-3-val" \
-H "header-4: header-4-val" \
-H "header-5: header-5-val" \
-H "header-6: header-6-val" \
-H "header-7: header-7-val" \
-H "header-8: header-8-val" \
-H "header-9: header-9-val" \
-H "header-10: header-10-val" \
-H "header-11: header-11-val" \
-H "header-12: header-12-val" \
-H "header-13: header-13-val" \
-H "header-14: header-14-val" \
-H "header-15: header-15-val" \
-H "header-16: header-16-val" \
"http://cafe.example.com:$GW_PORT/?param-1=param-1-val&param-2=param-2-val&param-3=param-3-val&param-4=param-4-val&param-5=param-5-val&param-6=param-6-val&param-7=param-7-val&param-8=param-8-val&param-9=param-9-val&param-10=param-10-val&param-11=param-11-val&param-12=param-12-val&param-13=param-13-val&param-14=param-14-val&param-15=param-15-val&param-16=param-16-val"

Handling connection for 8080
backend
sa.choudhary@N9939CQ4P0 bug % curl -X PATCH \
--resolve cafe.example.com:$GW_PORT:$GW_IP \
-H "header-1: header-1-val" \
-H "header-2: header-2-val" \
-H "header-3: header-3-val" \
-H "header-4: header-4-val" \
-H "header-5: header-5-val" \
-H "header-6: header-6-val" \
-H "header-7: header-7-val" \
-H "header-8: header-8-val" \
-H "header-9: header-9-val" \
-H "header-10: header-10-val" \
-H "header-11: header-11-val" \
-H "header-12: header-12-val" \
-H "header-13: header-13-val" \
-H "header-14: header-14-val" \
-H "header-15: header-15-val" \
-H "header-16: header-16-val" \
"http://cafe.example.com:$GW_PORT/?param-1=param-1-val&param-2=param-2-val&param-3=param-3-val&param-4=param-4-val&param-5=param-5-val&param-6=param-6-val&param-7=param-7-val&param-8=param-8-val&param-9=param-9-val&param-10=param-10-val&param-11=param-11-val&param-12=param-12-val&param-13=param-13-val&param-14=param-14-val&param-15=param-15-val&param-16=param-16-val"
Handling connection for 8080
backend
sa.choudhary@N9939CQ4P0 bug % curl -X PUT \
--resolve cafe.example.com:$GW_PORT:$GW_IP \
-H "header-1: header-1-val" \
-H "header-2: header-2-val" \
-H "header-3: header-3-val" \
-H "header-4: header-4-val" \
-H "header-5: header-5-val" \
-H "header-6: header-6-val" \
-H "header-7: header-7-val" \
-H "header-8: header-8-val" \
-H "header-9: header-9-val" \
-H "header-10: header-10-val" \
-H "header-11: header-11-val" \
-H "header-12: header-12-val" \
-H "header-13: header-13-val" \
-H "header-14: header-14-val" \
-H "header-15: header-15-val" \
-H "header-16: header-16-val" \
"http://cafe.example.com:$GW_PORT/?param-1=param-1-val&param-2=param-2-val&param-3=param-3-val&param-4=param-4-val&param-5=param-5-val&param-6=param-6-val&param-7=param-7-val&param-8=param-8-val&param-9=param-9-val&param-10=param-10-val&param-11=param-11-val&param-12=param-12-val&param-13=param-13-val&param-14=param-14-val&param-15=param-15-val&param-16=param-16-val"
Handling connection for 8080
backend

Closes #1107

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.


@github-actions github-actions bot added the bug Something isn't working label Apr 15, 2024
Copy link

codecov bot commented Apr 15, 2024

Codecov Report

Attention: Patch coverage is 95.08197% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 86.12%. Comparing base (57b76b7) to head (377d9f7).

Files Patch % Lines
...ernal/mode/static/nginx/modules/src/httpmatches.js 90.47% 4 Missing ⚠️
internal/mode/static/nginx/config/servers.go 96.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1829      +/-   ##
==========================================
- Coverage   86.20%   86.12%   -0.09%     
==========================================
  Files          83       83              
  Lines        5540     5586      +46     
  Branches       52       50       -2     
==========================================
+ Hits         4776     4811      +35     
- Misses        715      726      +11     
  Partials       49       49              

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

@salonichf5 salonichf5 force-pushed the bug/http-match branch 5 times, most recently from 62958e7 to 5abcc8a Compare April 16, 2024 21:11
@salonichf5 salonichf5 marked this pull request as ready for review April 17, 2024 16:04
@salonichf5 salonichf5 requested a review from a team as a code owner April 17, 2024 16:04
@salonichf5 salonichf5 self-assigned this Apr 17, 2024
internal/mode/static/nginx/config/generator.go Outdated Show resolved Hide resolved
internal/mode/static/nginx/config/servers.go Outdated Show resolved Hide resolved
internal/mode/static/nginx/config/servers.go Outdated Show resolved Hide resolved
internal/mode/static/nginx/config/servers.go Outdated Show resolved Hide resolved
internal/mode/static/nginx/config/servers.go Show resolved Hide resolved
internal/mode/static/nginx/modules/src/httpmatches.js Outdated Show resolved Hide resolved
internal/mode/static/nginx/modules/src/httpmatches.js Outdated Show resolved Hide resolved
internal/mode/static/nginx/modules/src/httpmatches.js Outdated Show resolved Hide resolved
internal/mode/static/nginx/config/servers.go Outdated Show resolved Hide resolved
internal/mode/static/nginx/config/servers_template.go Outdated Show resolved Hide resolved
internal/mode/static/nginx/config/generator.go Outdated Show resolved Hide resolved
internal/mode/static/nginx/config/generator_test.go Outdated Show resolved Hide resolved
internal/mode/static/nginx/config/servers.go Outdated Show resolved Hide resolved
internal/mode/static/nginx/config/servers_test.go Outdated Show resolved Hide resolved
internal/mode/static/nginx/config/servers_test.go Outdated Show resolved Hide resolved
internal/mode/static/nginx/config/servers_test.go Outdated Show resolved Hide resolved
@sjberman
Copy link
Contributor

Please rebase and ensure the pipeline runs. I'm assuming you tested with the large example from the issue, not just the advanced routing example?

internal/mode/static/nginx/config/generator.go Outdated Show resolved Hide resolved
internal/mode/static/nginx/config/generator.go Outdated Show resolved Hide resolved
internal/mode/static/nginx/config/servers.go Outdated Show resolved Hide resolved
internal/mode/static/nginx/config/upstreams_test.go Outdated Show resolved Hide resolved
internal/mode/static/nginx/modules/src/httpmatches.js Outdated Show resolved Hide resolved
internal/mode/static/nginx/modules/src/httpmatches.js Outdated Show resolved Hide resolved
internal/mode/static/nginx/modules/src/httpmatches.js Outdated Show resolved Hide resolved
salonichf5 and others added 2 commits April 24, 2024 13:15
Co-authored-by: Michael Pleshakov <pleshakov@users.noreply.github.com>
Co-authored-by: Michael Pleshakov <pleshakov@users.noreply.github.com>
salonichf5 and others added 6 commits April 25, 2024 09:23
Co-authored-by: Ciara Stacke <18287516+ciarams87@users.noreply.github.com>
Co-authored-by: Ciara Stacke <18287516+ciarams87@users.noreply.github.com>
Co-authored-by: Ciara Stacke <18287516+ciarams87@users.noreply.github.com>
Co-authored-by: Ciara Stacke <18287516+ciarams87@users.noreply.github.com>
Co-authored-by: Ciara Stacke <18287516+ciarams87@users.noreply.github.com>
Co-authored-by: Ciara Stacke <18287516+ciarams87@users.noreply.github.com>
@salonichf5
Copy link
Contributor Author

Codecov shows a few lines that are missing tests that may be testable. Please take a look.

yes have addressed those !

@salonichf5 salonichf5 enabled auto-merge (squash) April 29, 2024 17:02
@salonichf5 salonichf5 merged commit f70d692 into nginxinc:main Apr 29, 2024
40 checks passed
@salonichf5 salonichf5 deleted the bug/http-match branch April 29, 2024 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Large number of match conditions can cause reload errors
5 participants