Skip to content

Conversation

@salonichf5
Copy link
Contributor

@salonichf5 salonichf5 commented Nov 21, 2025

Proposed changes

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

Problem: Users want to specify all load balancing methods.

Solution: Specify a way for user to be able to specify all combinations of load balancing methods.

Testing:

Manual testing

Errors

spec:
  targetRefs:
  - group: core
    kind: Service
    name: tea
  loadBalancingMethod: "hash"

The UpstreamSettingsPolicy "usp-lb" is invalid: spec: Invalid value: "object": hashKey is required when loadBalancingMethod is 'hash' or 'hash consistent'

hash key format wrong

  loadBalancingMethod: "hash"
  hashKey: "upstream"
The UpstreamSettingsPolicy "usp-lb" is invalid: spec.hashKey: Invalid value: "upstream": spec.hashKey in body should match '^\$[a-z_]+$'

plus features with OSS

  loadBalancingMethod: "least_time"
    Conditions:
      Last Transition Time:  2025-11-21T05:54:56Z
      Message:               spec.loadBalancingMethod: Invalid value: "least_time": NGINX OSS only supports the following load balancing methods: round_robin, least_conn, ip_hash, random, hash, hash consistent, random two, random two least_conn

Spec does not allow any other features so do not need to validate for plus(everything allowed)

The UpstreamSettingsPolicy "usp-lb" is invalid: 
* spec.loadBalancingMethod: Unsupported value: "invalid-method": supported values: "round_robin", "least_conn", "ip_hash", "hash", "hash consistent", "random", "random two", "random two least_conn", "random two least_time", "random two least_time=header", "random two least_time=last_byte", "least_time", "least_time header", "least_time last_byte", "least_time header inflight", "least_time last_byte inflight"
* <nil>: Invalid value: null: some validation rules were not checked because the object was invalid; correct the existing errors to complete validation

USP is set for tea svc for all , verified traffic passing for all

Only mentioning special cases -- round_robin, hash and hash consistent

OSS

  • loadBalancingMethod: "round_robin"
upstream default_tea_80 {
    zone default_tea_80 1m;
    state /var/lib/nginx/state/default_tea_80.conf;


  loadBalancingMethod: "hash"
  hashKey: "$upstream_addr"

upstream default_tea_80 {
    hash $upstream_addr;
    zone default_tea_80 512k;


    server 10.244.0.27:8080;


  loadBalancingMethod: "hash consistent"
  hashKey: "$upstream_addr"

upstream default_tea_80 {
    hash $upstream_addr consistent;
    zone default_tea_80 512k;

Plus verified on tea svc

upstream default_tea_80 {
    least_time header;
    zone default_tea_80 1m;

    state /var/lib/nginx/state/default_tea_80.conf;

}

Verified all load balancing methods with working traffic for plus

Closes #4190

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.

NONE

@github-actions github-actions bot added enhancement New feature or request tests Pull requests that update tests labels Nov 21, 2025
@salonichf5 salonichf5 marked this pull request as ready for review November 21, 2025 16:52
@salonichf5 salonichf5 requested a review from a team as a code owner November 21, 2025 16:52
@sjberman
Copy link
Collaborator

In your round_robin example in the description, it's using random two least_conn.

@sjberman
Copy link
Collaborator

And your hash example shows hash consistent.

}

if a.LoadBalancingMethod != nil && b.LoadBalancingMethod != nil {
if !checkConflictForLoadBalancingFields(a, b) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reasoning behind inverting this function? Can we make the function return what we want without needing to invert it?

Copy link
Contributor Author

@salonichf5 salonichf5 Nov 22, 2025

Choose a reason for hiding this comment

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

Yeah agree, feels counter intuitive. Just checks if anything of the load balancing fields conflicts, good catch.

@github-project-automation github-project-automation bot moved this from 🆕 New to 🏗 In Progress in NGINX Gateway Fabric Nov 22, 2025
@salonichf5 salonichf5 requested a review from bjee19 November 22, 2025 01:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request tests Pull requests that update tests

Projects

Status: 🏗 In Progress

Development

Successfully merging this pull request may close these issues.

4 participants