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

Update backend group name with a prefix #2730

Merged
merged 3 commits into from
Nov 5, 2024
Merged

Update backend group name with a prefix #2730

merged 3 commits into from
Nov 5, 2024

Conversation

salonichf5
Copy link
Contributor

@salonichf5 salonichf5 commented Oct 28, 2024

Proposed changes

Write a clear and concise description that helps reviewers understand the purpose and impact of your changes. Use the
following format:

Problem: User want to configure split client upstream in a namespace starting with a digit without running into any issues.

When a user was configuring split clients in a namespace starting with a digit, it led to proxy_pass variable being http://$9c_network__some_route_name_rule0$request_uri , which led to issues with nginx since variable names cannot start with a digit, leading to a 502 Bad Gateway error. The above problem existed for cases when a variable name is mentioned for using split weightage of upstreams.

Solution: Updated backend group name with a prefix "group_". Hence, we update split client variable names, distribution names and upstream names.

Testing: Testing with cafe and traffic splitting example.

Config with cafe example

location = /coffee {
       proxy_pass http://9test_coffee_80$request_uri;
    }
    location = /tea {
       proxy_pass http://9test_tea_80$request_uri;
    }


upstream 9test_coffee_80 {
    random two least_conn;
    zone 9test_coffee_80 512k;

    server 10.244.0.40:8080;
}

upstream 9test_tea_80 {
    random two least_conn;
    zone 9test_tea_80 512k;

    server 10.244.0.41:8080;
}

Config with trafic-splitting example

location /coffee/ {
        proxy_pass http://$group_9test__cafe_route_rule0$request_uri;
    }

location = /coffee {
       proxy_pass http://$group_9test__cafe_route_rule0$request_uri;
 }

upstream 9test_coffee-v1_80 {
    random two least_conn;
    zone 9test_coffee-v1_80 512k;

    server 10.244.0.38:8080;
}

upstream 9test_coffee-v2_80 {
    random two least_conn;
    zone 9test_coffee-v2_80 512k;

    server 10.244.0.39:8080;
}

split_clients $request_id $group_9test__cafe_route_rule0 {
    80.00% 9test_coffee-v1_80;
    20.00% 9test_coffee-v2_80;
}

Please focus on (optional): If you any specific areas where you would like reviewers to focus their attention or provide
specific feedback, add them here.

Closes #2606

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.


@salonichf5 salonichf5 requested a review from a team as a code owner October 28, 2024 20:43
@github-actions github-actions bot added the bug Something isn't working label Oct 28, 2024
Copy link

codecov bot commented Oct 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.40%. Comparing base (fed4239) to head (781816f).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2730      +/-   ##
==========================================
+ Coverage   89.38%   89.40%   +0.01%     
==========================================
  Files         110      110              
  Lines       10913    10913              
  Branches       50       50              
==========================================
+ Hits         9755     9757       +2     
+ Misses       1100     1098       -2     
  Partials       58       58              

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

@salonichf5 salonichf5 changed the title update backends and upstreams with a prefix Update backend and upstream names with a prefix Oct 28, 2024
@salonichf5 salonichf5 marked this pull request as draft October 28, 2024 21:08
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Oct 28, 2024
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/2730/

@salonichf5 salonichf5 force-pushed the bug/bg-nsname branch 2 times, most recently from 893cbd2 to ad3e324 Compare October 29, 2024 23:22
@github-actions github-actions bot added the tests Pull requests that update tests label Oct 30, 2024
@salonichf5 salonichf5 force-pushed the bug/bg-nsname branch 3 times, most recently from 00dc128 to 80e3e14 Compare October 30, 2024 21:55
@github-actions github-actions bot removed the tests Pull requests that update tests label Oct 30, 2024
@salonichf5 salonichf5 self-assigned this Oct 30, 2024
@salonichf5 salonichf5 marked this pull request as ready for review October 30, 2024 22:53
@salonichf5 salonichf5 requested a review from a team as a code owner October 30, 2024 22:53
@salonichf5 salonichf5 changed the title Update backend and upstream names with a prefix Update backend group name with a prefix Oct 31, 2024
@sjberman
Copy link
Contributor

sjberman commented Nov 5, 2024

Be sure to use your fork in the future, this doesn't need to be a branch on the main repo.

@salonichf5
Copy link
Contributor Author

Be sure to use your fork in the future, this doesn't need to be a branch on the main repo.

Thank you for noticing that ! Need to fix my setup, probably messed it up !

@salonichf5 salonichf5 enabled auto-merge (squash) November 5, 2024 20:18
@salonichf5 salonichf5 merged commit a7c6977 into main Nov 5, 2024
43 checks passed
@salonichf5 salonichf5 deleted the bug/bg-nsname branch November 5, 2024 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Improvements or additions to documentation
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

BackendGroups with namespace starts with digit doesn't proxy_passed to split_clients correctly
6 participants