From 80fe344e52abaac4910eac379ee7c500379545a3 Mon Sep 17 00:00:00 2001 From: salonichf5 <146118978+salonichf5@users.noreply.github.com> Date: Wed, 19 Nov 2025 07:14:07 -0700 Subject: [PATCH 1/6] add all load balancing methods --- apis/v1alpha1/upstreamsettingspolicy_types.go | 95 +++++++- apis/v1alpha1/zz_generated.deepcopy.go | 5 + ...ay.nginx.org_upstreamsettingspolicies.yaml | 24 ++ deploy/crds.yaml | 24 ++ internal/controller/manager.go | 13 +- .../controller/nginx/config/http/config.go | 32 +++ .../policies/clientsettings/validator.go | 8 + .../policies/observability/validator.go | 8 + .../policies/policiesfakes/fake_validator.go | 74 +++++++ .../policies/upstreamsettings/processor.go | 6 + .../upstreamsettings/processor_test.go | 33 ++- .../policies/upstreamsettings/validator.go | 71 +++++- .../upstreamsettings/validator_test.go | 118 ++++++++++ .../nginx/config/policies/validator.go | 13 ++ internal/controller/nginx/config/upstreams.go | 23 +- .../controller/nginx/config/upstreams_test.go | 209 +++++++++++++++++- .../nginx/config/validation/generic.go | 21 ++ .../nginx/config/validation/generic_test.go | 22 ++ internal/controller/state/change_processor.go | 6 +- internal/controller/state/graph/graph.go | 16 +- internal/controller/state/graph/graph_test.go | 4 +- .../state/graph/multiple_gateways_test.go | 8 +- internal/controller/state/graph/policies.go | 15 +- .../controller/state/graph/policies_test.go | 42 +++- .../validationfakes/fake_generic_validator.go | 72 ++++++ .../validationfakes/fake_policy_validator.go | 74 +++++++ .../controller/state/validation/validator.go | 3 + tests/cel/common.go | 8 +- tests/cel/upstreamsettingspolicy_test.go | 84 +++++++ 29 files changed, 1094 insertions(+), 37 deletions(-) diff --git a/apis/v1alpha1/upstreamsettingspolicy_types.go b/apis/v1alpha1/upstreamsettingspolicy_types.go index a8f6c09a29..2c56ec4622 100644 --- a/apis/v1alpha1/upstreamsettingspolicy_types.go +++ b/apis/v1alpha1/upstreamsettingspolicy_types.go @@ -36,6 +36,9 @@ type UpstreamSettingsPolicyList struct { } // UpstreamSettingsPolicySpec defines the desired state of the UpstreamSettingsPolicy. +// +kubebuilder:validation:XValidation:rule="!(has(self.loadBalancingMethod) && (self.loadBalancingMethod == 'hash' || self.loadBalancingMethod == 'hash consistent')) || has(self.hashKey)",message="hashKey is required when loadBalancingMethod is 'hash' or 'hash consistent'" +// +//nolint:lll type UpstreamSettingsPolicySpec struct { // ZoneSize is the size of the shared memory zone used by the upstream. This memory zone is used to share // the upstream configuration between nginx worker processes. The more servers that an upstream has, @@ -58,6 +61,12 @@ type UpstreamSettingsPolicySpec struct { // +optional LoadBalancingMethod *LoadBalancingType `json:"loadBalancingMethod,omitempty"` + // HashKey defines the key used for hash-based load balancing methods. + // This field is required when `LoadBalancingMethod` is set to `hash` or `hash consistent`. + // + // +optional + HashKey *HashMethodKey `json:"hashKey,omitempty"` + // TargetRefs identifies API object(s) to apply the policy to. // Objects must be in the same namespace as the policy. // Support: Service @@ -108,19 +117,97 @@ type UpstreamKeepAlive struct { // LoadBalancingType defines the supported load balancing methods. // -// +kubebuilder:validation:Enum=ip_hash;random two least_conn +// +kubebuilder:validation:Enum=round_robin;least_conn;ip_hash;hash;hash consistent;random;random two;random two least_conn;random two least_time=header;random two least_time=last_byte;least_time header;least_time last_byte;least_time header inflight;least_time last_byte inflight +// +//nolint:lll type LoadBalancingType string const ( + // Combination of NGINX directive + // - https://nginx.org/en/docs/http/ngx_http_upstream_module.html#random + // - https://nginx.org/en/docs/http/ngx_http_upstream_module.html#least_conn + // - https://nginx.org/en/docs/http/ngx_http_upstream_module.html#least_time + // - https://nginx.org/en/docs/http/ngx_http_upstream_module.html#upstream + // - https://nginx.org/en/docs/http/ngx_http_upstream_module.html#ip_hash + // - https://nginx.org/en/docs/http/ngx_http_upstream_module.html#hash + + // LoadBalancingMethods for NGINX OSS. + + // LoadBalancingTypeRoundRobin enables round-robin load balancing, + // distributing requests evenly across all upstream servers. + // NGINX defaults to this method if no load balancing method is specified. + LoadBalancingTypeRoundRobin LoadBalancingType = "round_robin" + + // LoadBalancingTypeLeastConn enables least-connections load balancing, + // routing requests to the upstream server with the fewest active connections. + LoadBalancingTypeLeastConnection LoadBalancingType = "least_conn" + // LoadBalancingTypeIPHash enables IP hash-based load balancing, // ensuring requests from the same client IP are routed to the same upstream server. - // NGINX directive: https://nginx.org/en/docs/http/ngx_http_upstream_module.html#ip_hash LoadBalancingTypeIPHash LoadBalancingType = "ip_hash" + // LoadBalancingTypeHash enables generic hash-based load balancing, + // routing requests to upstream servers based on a hash of a specified key + // HashKey field must be set when this method is selected. + // Example configuration: hash $binary_remote_addr;. + LoadBalancingTypeHash LoadBalancingType = "hash" + + // LoadBalancingTypeHashConsistent enables consistent hash-based load balancing, + // which minimizes the number of keys remapped when a server is added or removed. + // HashKey field must be set when this method is selected. + // Example configuration: hash $binary_remote_addr consistent;. + LoadBalancingTypeHashConsistent LoadBalancingType = "hash consistent" + + // LoadBalancingTypeRandom enables random load balancing, + // routing requests to upstream servers in a random manner. + LoadBalancingTypeRandom LoadBalancingType = "random" + + // LoadBalancingTypeRandomTwo enables a variation of random load balancing + // that randomly selects two servers and forwards traffic to one of them. + // The default method is least_conn which passes a request to a server with the least number of active connections. + LoadBalancingTypeRandomTwo LoadBalancingType = "random two" + // LoadBalancingTypeRandomTwoLeastConnection enables a variation of least-connections // balancing that randomly selects two servers and forwards traffic to the one with // fewer active connections. - // NGINX directive least_conn: https://nginx.org/en/docs/http/ngx_http_upstream_module.html#least_conn - // NGINX directive random: https://nginx.org/en/docs/http/ngx_http_upstream_module.html#random LoadBalancingTypeRandomTwoLeastConnection LoadBalancingType = "random two least_conn" + + // LoadBalancingMethods for NGINX Plus along with OSS methods. + + // LoadBalancingTypeRandomTwoLeastTimeHeader enables a variation of least-time load balancing + // that randomly selects two servers and forwards traffic to the one with the least + // time to receive the response header. + LoadBalancingTypeRandomTwoLeastTimeHeader LoadBalancingType = "random two least_time=header" + + // LoadBalancingTypeRandomTwoLeastTimeLastByte enables a variation of least-time load balancing + // that randomly selects two servers and forwards traffic to the one with the least time + // to receive the full response. + LoadBalancingTypeRandomTwoLeastTimeLastByte LoadBalancingType = "random two least_time=last_byte" + + // LoadBalancingTypeLeastTimeHeader enables least-time load balancing, + // routing requests to the upstream server with the least time to receive the response header. + LoadBalancingTypeLeastTimeHeader LoadBalancingType = "least_time header" + + // LoadBalancingTypeLeastTimeLastByte enables least-time load balancing, + // routing requests to the upstream server with the least time to receive the full response. + LoadBalancingTypeLeastTimeLastByte LoadBalancingType = "least_time last_byte" + + // LoadBalancingTypeLeastTimeHeaderInflight enables least-time load balancing, + // routing requests to the upstream server with the least time to receive the response header, + // considering the incomplete requests. + LoadBalancingTypeLeastTimeHeaderInflight LoadBalancingType = "least_time header inflight" + + // LoadBalancingTypeLeastTimeLastByteInflight enables least-time load balancing, + // routing requests to the upstream server with the least time to receive the full response, + // considering the incomplete requests. + LoadBalancingTypeLeastTimeLastByteInflight LoadBalancingType = "least_time last_byte inflight" ) + +// HashMethodKey defines the key used for hash-based load balancing methods. +// The key must be a valid NGINX variable name starting with '$' followed by lowercase +// letters and underscores only. +// For a full list of NGINX variables, +// refer to: https://nginx.org/en/docs/http/ngx_http_upstream_module.html#variables +// +// +kubebuilder:validation:Pattern=`^\$[a-z_]+$` +type HashMethodKey string diff --git a/apis/v1alpha1/zz_generated.deepcopy.go b/apis/v1alpha1/zz_generated.deepcopy.go index 4f0c2bebc4..af4635e749 100644 --- a/apis/v1alpha1/zz_generated.deepcopy.go +++ b/apis/v1alpha1/zz_generated.deepcopy.go @@ -561,6 +561,11 @@ func (in *UpstreamSettingsPolicySpec) DeepCopyInto(out *UpstreamSettingsPolicySp *out = new(LoadBalancingType) **out = **in } + if in.HashKey != nil { + in, out := &in.HashKey, &out.HashKey + *out = new(HashMethodKey) + **out = **in + } if in.TargetRefs != nil { in, out := &in.TargetRefs, &out.TargetRefs *out = make([]apisv1.LocalPolicyTargetReference, len(*in)) diff --git a/config/crd/bases/gateway.nginx.org_upstreamsettingspolicies.yaml b/config/crd/bases/gateway.nginx.org_upstreamsettingspolicies.yaml index 6fa9de4104..120707cf58 100644 --- a/config/crd/bases/gateway.nginx.org_upstreamsettingspolicies.yaml +++ b/config/crd/bases/gateway.nginx.org_upstreamsettingspolicies.yaml @@ -51,6 +51,12 @@ spec: spec: description: Spec defines the desired state of the UpstreamSettingsPolicy. properties: + hashKey: + description: |- + HashKey defines the key used for hash-based load balancing methods. + This field is required when `LoadBalancingMethod` is set to `hash` or `hash consistent`. + pattern: ^\$[a-z_]+$ + type: string keepAlive: description: KeepAlive defines the keep-alive settings. properties: @@ -91,8 +97,20 @@ spec: If not specified, NGINX Gateway Fabric defaults to `random two least_conn`, which differs from the standard NGINX default `round-robin`. enum: + - round_robin + - least_conn - ip_hash + - hash + - hash consistent + - random + - random two - random two least_conn + - random two least_time=header + - random two least_time=last_byte + - least_time header + - least_time last_byte + - least_time header inflight + - least_time last_byte inflight type: string targetRefs: description: |- @@ -152,6 +170,12 @@ spec: required: - targetRefs type: object + x-kubernetes-validations: + - message: hashKey is required when loadBalancingMethod is 'hash' or 'hash + consistent' + rule: '!(has(self.loadBalancingMethod) && (self.loadBalancingMethod + == ''hash'' || self.loadBalancingMethod == ''hash consistent'')) || + has(self.hashKey)' status: description: Status defines the state of the UpstreamSettingsPolicy. properties: diff --git a/deploy/crds.yaml b/deploy/crds.yaml index ce16d35823..f21e71fff3 100644 --- a/deploy/crds.yaml +++ b/deploy/crds.yaml @@ -9578,6 +9578,12 @@ spec: spec: description: Spec defines the desired state of the UpstreamSettingsPolicy. properties: + hashKey: + description: |- + HashKey defines the key used for hash-based load balancing methods. + This field is required when `LoadBalancingMethod` is set to `hash` or `hash consistent`. + pattern: ^\$[a-z_]+$ + type: string keepAlive: description: KeepAlive defines the keep-alive settings. properties: @@ -9618,8 +9624,20 @@ spec: If not specified, NGINX Gateway Fabric defaults to `random two least_conn`, which differs from the standard NGINX default `round-robin`. enum: + - round_robin + - least_conn - ip_hash + - hash + - hash consistent + - random + - random two - random two least_conn + - random two least_time=header + - random two least_time=last_byte + - least_time header + - least_time last_byte + - least_time header inflight + - least_time last_byte inflight type: string targetRefs: description: |- @@ -9679,6 +9697,12 @@ spec: required: - targetRefs type: object + x-kubernetes-validations: + - message: hashKey is required when loadBalancingMethod is 'hash' or 'hash + consistent' + rule: '!(has(self.loadBalancingMethod) && (self.loadBalancingMethod + == ''hash'' || self.loadBalancingMethod == ''hash consistent'')) || + has(self.hashKey)' status: description: Status defines the state of the UpstreamSettingsPolicy. properties: diff --git a/internal/controller/manager.go b/internal/controller/manager.go index d4e2114e8a..8987c8ba4b 100644 --- a/internal/controller/manager.go +++ b/internal/controller/manager.go @@ -131,6 +131,11 @@ func StartManager(cfg config.Config) error { return err } + flags := graph.Flags{ + Plus: cfg.Plus, + Experimental: cfg.ExperimentalFeatures, + } + processor := state.NewChangeProcessorImpl(state.ChangeProcessorConfig{ GatewayCtlrName: cfg.GatewayCtlrName, GatewayClassName: cfg.GatewayClassName, @@ -140,10 +145,10 @@ func StartManager(cfg config.Config) error { GenericValidator: genericValidator, PolicyValidator: policyManager, }, - EventRecorder: recorder, - MustExtractGVK: mustExtractGVK, - PlusSecrets: plusSecrets, - ExperimentalFeatures: cfg.ExperimentalFeatures, + EventRecorder: recorder, + MustExtractGVK: mustExtractGVK, + PlusSecrets: plusSecrets, + Flags: flags, }) var handlerCollector handlerMetricsCollector = collectors.NewControllerNoopCollector() diff --git a/internal/controller/nginx/config/http/config.go b/internal/controller/nginx/config/http/config.go index 14af2c8ca7..3db66b4345 100644 --- a/internal/controller/nginx/config/http/config.go +++ b/internal/controller/nginx/config/http/config.go @@ -1,6 +1,7 @@ package http import ( + ngfAPI "github.com/nginx/nginx-gateway-fabric/v2/apis/v1alpha1" "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/nginx/config/shared" ) @@ -123,6 +124,7 @@ type Upstream struct { ZoneSize string // format: 512k, 1m StateFile string LoadBalancingMethod string + HashKey string KeepAlive UpstreamKeepAlive Servers []UpstreamServer } @@ -167,3 +169,33 @@ type ServerConfig struct { Plus bool DisableSNIHostValidation bool } + +var ( + PlusAllowedLBMethods = map[ngfAPI.LoadBalancingType]struct{}{ + ngfAPI.LoadBalancingTypeRoundRobin: {}, + ngfAPI.LoadBalancingTypeLeastConnection: {}, + ngfAPI.LoadBalancingTypeIPHash: {}, + ngfAPI.LoadBalancingTypeRandom: {}, + ngfAPI.LoadBalancingTypeHash: {}, + ngfAPI.LoadBalancingTypeHashConsistent: {}, + ngfAPI.LoadBalancingTypeRandomTwo: {}, + ngfAPI.LoadBalancingTypeRandomTwoLeastConnection: {}, + ngfAPI.LoadBalancingTypeLeastTimeHeader: {}, + ngfAPI.LoadBalancingTypeLeastTimeLastByte: {}, + ngfAPI.LoadBalancingTypeLeastTimeHeaderInflight: {}, + ngfAPI.LoadBalancingTypeLeastTimeLastByteInflight: {}, + ngfAPI.LoadBalancingTypeRandomTwoLeastTimeHeader: {}, + ngfAPI.LoadBalancingTypeRandomTwoLeastTimeLastByte: {}, + } + + OSSAllowedLBMethods = map[ngfAPI.LoadBalancingType]struct{}{ + ngfAPI.LoadBalancingTypeRoundRobin: {}, + ngfAPI.LoadBalancingTypeLeastConnection: {}, + ngfAPI.LoadBalancingTypeIPHash: {}, + ngfAPI.LoadBalancingTypeRandom: {}, + ngfAPI.LoadBalancingTypeHash: {}, + ngfAPI.LoadBalancingTypeHashConsistent: {}, + ngfAPI.LoadBalancingTypeRandomTwo: {}, + ngfAPI.LoadBalancingTypeRandomTwoLeastConnection: {}, + } +) diff --git a/internal/controller/nginx/config/policies/clientsettings/validator.go b/internal/controller/nginx/config/policies/clientsettings/validator.go index c4d83864e7..58d7655984 100644 --- a/internal/controller/nginx/config/policies/clientsettings/validator.go +++ b/internal/controller/nginx/config/policies/clientsettings/validator.go @@ -50,6 +50,14 @@ func (v *Validator) ValidateGlobalSettings( return nil } +// ValidateLoadBalancingMethod validates the load balancing method for upstream servers. +func (v *Validator) ValidateLoadBalancingMethod( + _ policies.Policy, + _ bool, +) []conditions.Condition { + return nil +} + // Conflicts returns true if the two ClientSettingsPolicies conflict. func (v *Validator) Conflicts(polA, polB policies.Policy) bool { cspA := helpers.MustCastObject[*ngfAPI.ClientSettingsPolicy](polA) diff --git a/internal/controller/nginx/config/policies/observability/validator.go b/internal/controller/nginx/config/policies/observability/validator.go index 43d47dce3e..1e4f65f151 100644 --- a/internal/controller/nginx/config/policies/observability/validator.go +++ b/internal/controller/nginx/config/policies/observability/validator.go @@ -141,3 +141,11 @@ func (v *Validator) validateSettings(spec ngfAPIv1alpha2.ObservabilityPolicySpec return allErrs.ToAggregate() } + +// ValidateLoadBalancingMethod validates the load balancing method for upstream servers. +func (v *Validator) ValidateLoadBalancingMethod( + _ policies.Policy, + _ bool, +) []conditions.Condition { + return nil +} diff --git a/internal/controller/nginx/config/policies/policiesfakes/fake_validator.go b/internal/controller/nginx/config/policies/policiesfakes/fake_validator.go index 9389e6ccff..2707518f16 100644 --- a/internal/controller/nginx/config/policies/policiesfakes/fake_validator.go +++ b/internal/controller/nginx/config/policies/policiesfakes/fake_validator.go @@ -44,6 +44,18 @@ type FakeValidator struct { validateGlobalSettingsReturnsOnCall map[int]struct { result1 []conditions.Condition } + ValidateLoadBalancingMethodStub func(policies.Policy, bool) []conditions.Condition + validateLoadBalancingMethodMutex sync.RWMutex + validateLoadBalancingMethodArgsForCall []struct { + arg1 policies.Policy + arg2 bool + } + validateLoadBalancingMethodReturns struct { + result1 []conditions.Condition + } + validateLoadBalancingMethodReturnsOnCall map[int]struct { + result1 []conditions.Condition + } invocations map[string][][]interface{} invocationsMutex sync.RWMutex } @@ -233,6 +245,68 @@ func (fake *FakeValidator) ValidateGlobalSettingsReturnsOnCall(i int, result1 [] }{result1} } +func (fake *FakeValidator) ValidateLoadBalancingMethod(arg1 policies.Policy, arg2 bool) []conditions.Condition { + fake.validateLoadBalancingMethodMutex.Lock() + ret, specificReturn := fake.validateLoadBalancingMethodReturnsOnCall[len(fake.validateLoadBalancingMethodArgsForCall)] + fake.validateLoadBalancingMethodArgsForCall = append(fake.validateLoadBalancingMethodArgsForCall, struct { + arg1 policies.Policy + arg2 bool + }{arg1, arg2}) + stub := fake.ValidateLoadBalancingMethodStub + fakeReturns := fake.validateLoadBalancingMethodReturns + fake.recordInvocation("ValidateLoadBalancingMethod", []interface{}{arg1, arg2}) + fake.validateLoadBalancingMethodMutex.Unlock() + if stub != nil { + return stub(arg1, arg2) + } + if specificReturn { + return ret.result1 + } + return fakeReturns.result1 +} + +func (fake *FakeValidator) ValidateLoadBalancingMethodCallCount() int { + fake.validateLoadBalancingMethodMutex.RLock() + defer fake.validateLoadBalancingMethodMutex.RUnlock() + return len(fake.validateLoadBalancingMethodArgsForCall) +} + +func (fake *FakeValidator) ValidateLoadBalancingMethodCalls(stub func(policies.Policy, bool) []conditions.Condition) { + fake.validateLoadBalancingMethodMutex.Lock() + defer fake.validateLoadBalancingMethodMutex.Unlock() + fake.ValidateLoadBalancingMethodStub = stub +} + +func (fake *FakeValidator) ValidateLoadBalancingMethodArgsForCall(i int) (policies.Policy, bool) { + fake.validateLoadBalancingMethodMutex.RLock() + defer fake.validateLoadBalancingMethodMutex.RUnlock() + argsForCall := fake.validateLoadBalancingMethodArgsForCall[i] + return argsForCall.arg1, argsForCall.arg2 +} + +func (fake *FakeValidator) ValidateLoadBalancingMethodReturns(result1 []conditions.Condition) { + fake.validateLoadBalancingMethodMutex.Lock() + defer fake.validateLoadBalancingMethodMutex.Unlock() + fake.ValidateLoadBalancingMethodStub = nil + fake.validateLoadBalancingMethodReturns = struct { + result1 []conditions.Condition + }{result1} +} + +func (fake *FakeValidator) ValidateLoadBalancingMethodReturnsOnCall(i int, result1 []conditions.Condition) { + fake.validateLoadBalancingMethodMutex.Lock() + defer fake.validateLoadBalancingMethodMutex.Unlock() + fake.ValidateLoadBalancingMethodStub = nil + if fake.validateLoadBalancingMethodReturnsOnCall == nil { + fake.validateLoadBalancingMethodReturnsOnCall = make(map[int]struct { + result1 []conditions.Condition + }) + } + fake.validateLoadBalancingMethodReturnsOnCall[i] = struct { + result1 []conditions.Condition + }{result1} +} + func (fake *FakeValidator) Invocations() map[string][][]interface{} { fake.invocationsMutex.RLock() defer fake.invocationsMutex.RUnlock() diff --git a/internal/controller/nginx/config/policies/upstreamsettings/processor.go b/internal/controller/nginx/config/policies/upstreamsettings/processor.go index 7c29f807c4..51906bcec0 100644 --- a/internal/controller/nginx/config/policies/upstreamsettings/processor.go +++ b/internal/controller/nginx/config/policies/upstreamsettings/processor.go @@ -15,6 +15,8 @@ type UpstreamSettings struct { ZoneSize string // LoadBalancingMethod is the load balancing method setting. LoadBalancingMethod string + // HashKey is the key to be used for hash-based load balancing methods. + HashKey string // KeepAlive contains the keepalive settings. KeepAlive http.UpstreamKeepAlive } @@ -67,6 +69,10 @@ func processPolicies(pols []policies.Policy) UpstreamSettings { if usp.Spec.LoadBalancingMethod != nil { upstreamSettings.LoadBalancingMethod = string(*usp.Spec.LoadBalancingMethod) } + + if usp.Spec.HashKey != nil { + upstreamSettings.HashKey = string(*usp.Spec.HashKey) + } } return upstreamSettings diff --git a/internal/controller/nginx/config/policies/upstreamsettings/processor_test.go b/internal/controller/nginx/config/policies/upstreamsettings/processor_test.go index 4156781663..9c58b6aba4 100644 --- a/internal/controller/nginx/config/policies/upstreamsettings/processor_test.go +++ b/internal/controller/nginx/config/policies/upstreamsettings/processor_test.go @@ -38,6 +38,7 @@ func TestProcess(t *testing.T) { Timeout: helpers.GetPointer[ngfAPIv1alpha1.Duration]("10s"), }), LoadBalancingMethod: helpers.GetPointer(ngfAPIv1alpha1.LoadBalancingTypeIPHash), + HashKey: helpers.GetPointer[ngfAPIv1alpha1.HashMethodKey]("$upstream_addr"), }, }, }, @@ -50,6 +51,7 @@ func TestProcess(t *testing.T) { Timeout: "10s", }, LoadBalancingMethod: string(ngfAPIv1alpha1.LoadBalancingTypeIPHash), + HashKey: "$upstream_addr", }, }, { @@ -69,6 +71,25 @@ func TestProcess(t *testing.T) { LoadBalancingMethod: string(ngfAPIv1alpha1.LoadBalancingTypeRandomTwoLeastConnection), }, }, + { + name: "load balancing method set with hash key", + policies: []policies.Policy{ + &ngfAPIv1alpha1.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp", + Namespace: "test", + }, + Spec: ngfAPIv1alpha1.UpstreamSettingsPolicySpec{ + LoadBalancingMethod: helpers.GetPointer(ngfAPIv1alpha1.LoadBalancingTypeHashConsistent), + HashKey: helpers.GetPointer[ngfAPIv1alpha1.HashMethodKey]("$request_time"), + }, + }, + }, + expUpstreamSettings: UpstreamSettings{ + LoadBalancingMethod: string(ngfAPIv1alpha1.LoadBalancingTypeHashConsistent), + HashKey: "$request_time", + }, + }, { name: "zone size set", policies: []policies.Policy{ @@ -245,7 +266,8 @@ func TestProcess(t *testing.T) { Namespace: "test", }, Spec: ngfAPIv1alpha1.UpstreamSettingsPolicySpec{ - LoadBalancingMethod: helpers.GetPointer(ngfAPIv1alpha1.LoadBalancingTypeIPHash), + LoadBalancingMethod: helpers.GetPointer(ngfAPIv1alpha1.LoadBalancingTypeHashConsistent), + HashKey: helpers.GetPointer[ngfAPIv1alpha1.HashMethodKey]("$upstream_addr"), }, }, }, @@ -257,7 +279,8 @@ func TestProcess(t *testing.T) { Time: "5s", Timeout: "10s", }, - LoadBalancingMethod: string(ngfAPIv1alpha1.LoadBalancingTypeIPHash), + LoadBalancingMethod: string(ngfAPIv1alpha1.LoadBalancingTypeHashConsistent), + HashKey: "$upstream_addr", }, }, { @@ -345,7 +368,8 @@ func TestProcess(t *testing.T) { Namespace: "test", }, Spec: ngfAPIv1alpha1.UpstreamSettingsPolicySpec{ - LoadBalancingMethod: helpers.GetPointer(ngfAPIv1alpha1.LoadBalancingTypeIPHash), + LoadBalancingMethod: helpers.GetPointer(ngfAPIv1alpha1.LoadBalancingTypeHash), + HashKey: helpers.GetPointer[ngfAPIv1alpha1.HashMethodKey]("$remote_addr"), }, }, }, @@ -357,7 +381,8 @@ func TestProcess(t *testing.T) { Time: "5s", Timeout: "10s", }, - LoadBalancingMethod: string(ngfAPIv1alpha1.LoadBalancingTypeIPHash), + LoadBalancingMethod: string(ngfAPIv1alpha1.LoadBalancingTypeHash), + HashKey: "$remote_addr", }, }, } diff --git a/internal/controller/nginx/config/policies/upstreamsettings/validator.go b/internal/controller/nginx/config/policies/upstreamsettings/validator.go index d3b235a3ba..f1072232b5 100644 --- a/internal/controller/nginx/config/policies/upstreamsettings/validator.go +++ b/internal/controller/nginx/config/policies/upstreamsettings/validator.go @@ -1,10 +1,14 @@ package upstreamsettings import ( + "fmt" + "strings" + "k8s.io/apimachinery/pkg/util/validation/field" gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" ngfAPI "github.com/nginx/nginx-gateway-fabric/v2/apis/v1alpha1" + httpConfig "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/nginx/config/http" "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/nginx/config/policies" "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/state/conditions" "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/state/validation" @@ -83,13 +87,25 @@ func conflicts(a, b ngfAPI.UpstreamSettingsPolicySpec) bool { } } - if a.LoadBalancingMethod != nil && b.LoadBalancingMethod != nil { + if !checkConflictForLoadBalancingFields(a, b) { return true } return false } +func checkConflictForLoadBalancingFields(a, b ngfAPI.UpstreamSettingsPolicySpec) bool { + if a.LoadBalancingMethod != nil && b.LoadBalancingMethod != nil { + return *a.LoadBalancingMethod == *b.LoadBalancingMethod + } + + if a.HashKey != nil && b.HashKey != nil { + return *a.HashKey == *b.HashKey + } + + return true +} + // validateSettings performs validation on fields in the spec that are vulnerable to code injection. // For all other fields, we rely on the CRD validation. func (v Validator) validateSettings(spec ngfAPI.UpstreamSettingsPolicySpec) error { @@ -134,3 +150,56 @@ func (v Validator) validateUpstreamKeepAlive( return allErrs } + +// ValidateLoadBalancingMethod validates the load balancing method for upstream servers. +func (v Validator) ValidateLoadBalancingMethod( + policy policies.Policy, + plusEnabled bool, +) []conditions.Condition { + var allErrs field.ErrorList + fieldPath := field.NewPath("spec") + usp := helpers.MustCastObject[*ngfAPI.UpstreamSettingsPolicy](policy) + + if usp.Spec.LoadBalancingMethod == nil { + return nil + } + + lbMethod := *usp.Spec.LoadBalancingMethod + if !plusEnabled { + if _, ok := httpConfig.OSSAllowedLBMethods[lbMethod]; !ok { + allErrs = append(allErrs, field.Invalid( + fieldPath.Child("loadBalancingMethod"), + lbMethod, + fmt.Sprintf( + "NGINX OSS only supports the following load balancing methods: %s", + getLoadBalancingMethodList(httpConfig.OSSAllowedLBMethods), + ), + )) + } + } else { + if _, ok := httpConfig.PlusAllowedLBMethods[lbMethod]; !ok { + allErrs = append(allErrs, field.Invalid( + fieldPath.Child("loadBalancingMethod"), + lbMethod, + fmt.Sprintf( + "NGINX Plus only supports the following load balancing methods: %s", + getLoadBalancingMethodList(httpConfig.PlusAllowedLBMethods), + ), + )) + } + } + + if len(allErrs) > 0 { + return []conditions.Condition{conditions.NewPolicyInvalid(allErrs.ToAggregate().Error())} + } + + return nil +} + +func getLoadBalancingMethodList(lbMethods map[ngfAPI.LoadBalancingType]struct{}) string { + var methods []string + for method := range lbMethods { + methods = append(methods, string(method)) + } + return strings.Join(methods, ", ") +} diff --git a/internal/controller/nginx/config/policies/upstreamsettings/validator_test.go b/internal/controller/nginx/config/policies/upstreamsettings/validator_test.go index 1bae51e5bb..f7a7fb38fb 100644 --- a/internal/controller/nginx/config/policies/upstreamsettings/validator_test.go +++ b/internal/controller/nginx/config/policies/upstreamsettings/validator_test.go @@ -39,6 +39,7 @@ func createValidPolicy() *ngfAPI.UpstreamSettingsPolicy { Connections: helpers.GetPointer[int32](100), }, LoadBalancingMethod: helpers.GetPointer(ngfAPI.LoadBalancingTypeRandomTwoLeastConnection), + HashKey: helpers.GetPointer[ngfAPI.HashMethodKey]("$upstream_addr"), }, Status: v1.PolicyStatus{}, } @@ -258,6 +259,17 @@ func TestValidator_Conflicts(t *testing.T) { }, conflicts: true, }, + { + name: "hash key conflicts", + polA: createValidPolicy(), + polB: &ngfAPI.UpstreamSettingsPolicy{ + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + LoadBalancingMethod: helpers.GetPointer(ngfAPI.LoadBalancingTypeHashConsistent), + HashKey: helpers.GetPointer[ngfAPI.HashMethodKey]("$upstream_addr"), + }, + }, + conflicts: true, + }, } v := upstreamsettings.NewValidator(nil) @@ -284,3 +296,109 @@ func TestValidator_ConflictsPanics(t *testing.T) { g.Expect(conflicts).To(Panic()) } + +func TestValidate_ValidateLoadBalancingMethod(t *testing.T) { + t.Parallel() + + tests := []struct { + policy *ngfAPI.UpstreamSettingsPolicy + name string + expConditions []conditions.Condition + plusEnabled bool + }{ + { + name: "oss method random with Plus disabled", + policy: &ngfAPI.UpstreamSettingsPolicy{ + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + LoadBalancingMethod: helpers.GetPointer(ngfAPI.LoadBalancingTypeRandom), + }, + }, + expConditions: nil, + }, + { + name: "oss method hash consistent with Plus disabled", + policy: &ngfAPI.UpstreamSettingsPolicy{ + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + LoadBalancingMethod: helpers.GetPointer(ngfAPI.LoadBalancingTypeHashConsistent), + }, + }, + expConditions: nil, + }, + { + name: "plus load balancing method least_time last_byte not allowed with Plus disabled", + policy: &ngfAPI.UpstreamSettingsPolicy{ + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + LoadBalancingMethod: helpers.GetPointer(ngfAPI.LoadBalancingTypeLeastTimeLastByte), + }, + }, + expConditions: []conditions.Condition{ + conditions.NewPolicyInvalid("spec.loadBalancingMethod: Invalid value: \"least_time last_byte\": " + + "NGINX OSS only supports the following load balancing methods: "), + }, + }, + { + name: "plus load balancing method least_time header allowed with Plus enabled", + policy: &ngfAPI.UpstreamSettingsPolicy{ + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + LoadBalancingMethod: helpers.GetPointer(ngfAPI.LoadBalancingTypeLeastTimeHeader), + }, + }, + plusEnabled: true, + expConditions: nil, + }, + { + name: "invalid load balancing method for NGINX OSS", + policy: &ngfAPI.UpstreamSettingsPolicy{ + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + LoadBalancingMethod: helpers.GetPointer(ngfAPI.LoadBalancingType("invalid-method")), + }, + }, + expConditions: []conditions.Condition{ + conditions.NewPolicyInvalid("spec.loadBalancingMethod: Invalid value: \"invalid-method\": " + + "NGINX OSS only supports the following load balancing methods: "), + }, + }, + { + name: "invalid load balancing method for NGINX Plus", + policy: &ngfAPI.UpstreamSettingsPolicy{ + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + LoadBalancingMethod: helpers.GetPointer(ngfAPI.LoadBalancingType("invalid-method")), + }, + }, + plusEnabled: true, + expConditions: []conditions.Condition{ + conditions.NewPolicyInvalid("spec.loadBalancingMethod: Invalid value: \"invalid-method\": " + + "NGINX Plus only supports the following load balancing methods: "), + }, + }, + } + + v := upstreamsettings.NewValidator(validation.GenericValidator{}) + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + conds := v.ValidateLoadBalancingMethod(test.policy, test.plusEnabled) + + if test.expConditions != nil { + g.Expect(conds).To(HaveLen(1)) + g.Expect(conds[0].Message).To(ContainSubstring(test.expConditions[0].Message)) + } + }) + } +} + +func TestValidator_ValidateLoadBalancingPanics(t *testing.T) { + t.Parallel() + v := upstreamsettings.NewValidator(nil) + + validateLoadBalancingMethod := func() { + _ = v.ValidateLoadBalancingMethod(&policiesfakes.FakePolicy{}, true) + } + + g := NewWithT(t) + + g.Expect(validateLoadBalancingMethod).To(Panic()) +} diff --git a/internal/controller/nginx/config/policies/validator.go b/internal/controller/nginx/config/policies/validator.go index 5182c9781a..c83465d46b 100644 --- a/internal/controller/nginx/config/policies/validator.go +++ b/internal/controller/nginx/config/policies/validator.go @@ -21,6 +21,8 @@ type Validator interface { ValidateGlobalSettings(policy Policy, globalSettings *GlobalSettings) []conditions.Condition // Conflicts returns true if the two Policies conflict. Conflicts(a, b Policy) bool + // ValidateLoadBalancingMethod validates the load balancing method for upstream servers. + ValidateLoadBalancingMethod(policy Policy, plusEnabled bool) []conditions.Condition } // CompositeValidator manages the validators for NGF Policies. @@ -93,3 +95,14 @@ func (m *CompositeValidator) Conflicts(polA, polB Policy) bool { return validator.Conflicts(polA, polB) } + +func (m *CompositeValidator) ValidateLoadBalancingMethod(policy Policy, plusEnabled bool) []conditions.Condition { + gvk := m.mustExtractGVK(policy) + + validator, ok := m.validators[gvk] + if !ok { + panic(fmt.Sprintf("no validator registered for policy %T", policy)) + } + + return validator.ValidateLoadBalancingMethod(policy, plusEnabled) +} diff --git a/internal/controller/nginx/config/upstreams.go b/internal/controller/nginx/config/upstreams.go index bf56ec052d..ac9f398497 100644 --- a/internal/controller/nginx/config/upstreams.go +++ b/internal/controller/nginx/config/upstreams.go @@ -4,6 +4,7 @@ import ( "fmt" gotemplate "text/template" + ngfAPI "github.com/nginx/nginx-gateway-fabric/v2/apis/v1alpha1" "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/nginx/config/http" "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/nginx/config/policies/upstreamsettings" "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/nginx/config/stream" @@ -162,6 +163,22 @@ func (g GeneratorImpl) createUpstream( zoneSize = upstreamPolicySettings.ZoneSize } + chosenLBMethod := defaultLBMethod + if upstreamPolicySettings.LoadBalancingMethod != "" { + lbMethod := upstreamPolicySettings.LoadBalancingMethod + + if lbMethod == string(ngfAPI.LoadBalancingTypeHash) { + lbMethod = fmt.Sprintf("hash %s", upstreamPolicySettings.HashKey) + } + if lbMethod == string(ngfAPI.LoadBalancingTypeHashConsistent) { + lbMethod = fmt.Sprintf("hash %s consistent", upstreamPolicySettings.HashKey) + } + if lbMethod == string(ngfAPI.LoadBalancingTypeRoundRobin) { + lbMethod = "" + } + chosenLBMethod = lbMethod + } + if len(up.Endpoints) == 0 { return http.Upstream{ Name: up.Name, @@ -172,6 +189,7 @@ func (g GeneratorImpl) createUpstream( Address: types.Nginx503Server, }, }, + LoadBalancingMethod: chosenLBMethod, } } @@ -187,11 +205,6 @@ func (g GeneratorImpl) createUpstream( } } - chosenLBMethod := defaultLBMethod - if upstreamPolicySettings.LoadBalancingMethod != "" { - chosenLBMethod = upstreamPolicySettings.LoadBalancingMethod - } - return http.Upstream{ Name: up.Name, ZoneSize: zoneSize, diff --git a/internal/controller/nginx/config/upstreams_test.go b/internal/controller/nginx/config/upstreams_test.go index c87761b35f..1888e927fd 100644 --- a/internal/controller/nginx/config/upstreams_test.go +++ b/internal/controller/nginx/config/upstreams_test.go @@ -105,7 +105,7 @@ func TestExecuteUpstreams(t *testing.T) { "zone up5-usp 2m;": 1, "ip_hash;": 1, - "random two least_conn;": 3, + "random two least_conn;": 4, } upstreams := gen.createUpstreams(stateUpstreams, upstreamsettings.NewProcessor()) @@ -233,6 +233,7 @@ func TestCreateUpstreams(t *testing.T) { Address: types.Nginx503Server, }, }, + LoadBalancingMethod: defaultLBMethod, }, { Name: "up4-ipv6", @@ -296,6 +297,7 @@ func TestCreateUpstream(t *testing.T) { Address: types.Nginx503Server, }, }, + LoadBalancingMethod: defaultLBMethod, }, msg: "nil endpoints", }, @@ -312,6 +314,7 @@ func TestCreateUpstream(t *testing.T) { Address: types.Nginx503Server, }, }, + LoadBalancingMethod: defaultLBMethod, }, msg: "no endpoints", }, @@ -705,6 +708,7 @@ func TestCreateUpstreamPlus(t *testing.T) { Address: types.Nginx503Server, }, }, + LoadBalancingMethod: defaultLBMethod, }, }, } @@ -1198,3 +1202,206 @@ func TestKeepAliveChecker(t *testing.T) { }) } } + +func TestExecuteUpstreams_LoadBalancingMethod(t *testing.T) { + t.Parallel() + + tests := []struct { + expectedSubStrings map[string]int + name string + lbType ngfAPI.LoadBalancingType + hashkey ngfAPI.HashMethodKey + }{ + { + name: "default load balancing method", + expectedSubStrings: map[string]int{ + "upstream up1-usp-ipv4": 1, + "upstream up2-usp-ipv6": 1, + "random two least_conn;": 2, + }, + }, + { + name: "round_robin load balancing method", + expectedSubStrings: map[string]int{ + "upstream up1-usp-ipv4": 1, + "upstream up2-usp-ipv6": 1, + }, + }, + { + name: "least_conn load balancing method", + lbType: ngfAPI.LoadBalancingTypeLeastConnection, + expectedSubStrings: map[string]int{ + "upstream up1-usp-ipv4": 1, + "upstream up2-usp-ipv6": 1, + "least_conn;": 2, + }, + }, + { + name: "ip_hash load balancing method", + lbType: ngfAPI.LoadBalancingTypeIPHash, + expectedSubStrings: map[string]int{ + "upstream up1-usp-ipv4": 1, + "upstream up2-usp-ipv6": 1, + "ip_hash;": 2, + }, + }, + { + name: "hash load balancing method with specific hash key", + lbType: ngfAPI.LoadBalancingTypeHash, + hashkey: ngfAPI.HashMethodKey("$request_uri"), + expectedSubStrings: map[string]int{ + "upstream up1-usp-ipv4": 1, + "upstream up2-usp-ipv6": 1, + "hash $request_uri;": 2, + }, + }, + { + name: "hash consistent load balancing method with specific hash key", + lbType: ngfAPI.LoadBalancingTypeHashConsistent, + hashkey: ngfAPI.HashMethodKey("$remote_addr"), + expectedSubStrings: map[string]int{ + "upstream up1-usp-ipv4": 1, + "upstream up2-usp-ipv6": 1, + "hash $remote_addr consistent;": 2, + }, + }, + { + name: "random load balancing method", + lbType: ngfAPI.LoadBalancingTypeRandom, + expectedSubStrings: map[string]int{ + "upstream up1-usp-ipv4": 1, + "upstream up2-usp-ipv6": 1, + "random;": 2, + }, + }, + { + name: "random two load balancing method", + lbType: ngfAPI.LoadBalancingTypeRandomTwo, + expectedSubStrings: map[string]int{ + "upstream up1-usp-ipv4": 1, + "upstream up2-usp-ipv6": 1, + "random two;": 2, + }, + }, + { + name: "random two least_time=header load balancing method", + lbType: ngfAPI.LoadBalancingTypeRandomTwoLeastTimeHeader, + expectedSubStrings: map[string]int{ + "upstream up1-usp-ipv4": 1, + "upstream up2-usp-ipv6": 1, + "random two least_time=header;": 2, + }, + }, + { + name: "random two least_time=last_byte load balancing method", + lbType: ngfAPI.LoadBalancingTypeRandomTwoLeastTimeLastByte, + expectedSubStrings: map[string]int{ + "upstream up1-usp-ipv4": 1, + "upstream up2-usp-ipv6": 1, + "random two least_time=last_byte;": 2, + }, + }, + { + name: "least_time header load balancing method", + lbType: ngfAPI.LoadBalancingTypeLeastTimeHeader, + expectedSubStrings: map[string]int{ + "upstream up1-usp-ipv4": 1, + "upstream up2-usp-ipv6": 1, + "least_time header;": 2, + }, + }, + { + name: "least_time last_byte load balancing method", + lbType: ngfAPI.LoadBalancingTypeLeastTimeLastByte, + expectedSubStrings: map[string]int{ + "upstream up1-usp-ipv4": 1, + "upstream up2-usp-ipv6": 1, + "least_time last_byte;": 2, + }, + }, + { + name: "least_time header inflight load balancing method", + lbType: ngfAPI.LoadBalancingTypeLeastTimeHeaderInflight, + expectedSubStrings: map[string]int{ + "upstream up1-usp-ipv4": 1, + "upstream up2-usp-ipv6": 1, + "least_time header inflight;": 2, + }, + }, + { + name: "least_time last_byte inflight load balancing method", + lbType: ngfAPI.LoadBalancingTypeLeastTimeLastByteInflight, + expectedSubStrings: map[string]int{ + "upstream up1-usp-ipv4": 1, + "upstream up2-usp-ipv6": 1, + "least_time last_byte inflight;": 2, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + gen := GeneratorImpl{} + stateUpstreams := []dataplane.Upstream{ + { + Name: "up1-usp-ipv4", + Endpoints: []resolver.Endpoint{ + { + Address: "12.0.0.0", + Port: 80, + }, + }, + Policies: []policies.Policy{ + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp-ipv4", + Namespace: "test", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + LoadBalancingMethod: helpers.GetPointer(tt.lbType), + HashKey: helpers.GetPointer(tt.hashkey), + }, + }, + }, + }, + { + Name: "up2-usp-ipv6", + Endpoints: []resolver.Endpoint{ + { + Address: "2001:db8::1", + Port: 80, + }, + }, + Policies: []policies.Policy{ + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp-ipv6", + Namespace: "test", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + LoadBalancingMethod: helpers.GetPointer(tt.lbType), + HashKey: helpers.GetPointer(tt.hashkey), + }, + }, + }, + }, + } + + upstreams := gen.createUpstreams(stateUpstreams, upstreamsettings.NewProcessor()) + upstreamResults := executeUpstreams(upstreams) + + g.Expect(upstreamResults).To(HaveLen(1)) + nginxUpstreams := string(upstreamResults[0].data) + + for expSubString, expectedCount := range tt.expectedSubStrings { + actualCount := strings.Count(nginxUpstreams, expSubString) + g.Expect(actualCount).To( + Equal(expectedCount), + fmt.Sprintf("substring %q expected %d occurrence(s), got %d", expSubString, expectedCount, actualCount), + ) + } + }) + } +} diff --git a/internal/controller/nginx/config/validation/generic.go b/internal/controller/nginx/config/validation/generic.go index 8342ab4134..f63073955b 100644 --- a/internal/controller/nginx/config/validation/generic.go +++ b/internal/controller/nginx/config/validation/generic.go @@ -106,3 +106,24 @@ func (GenericValidator) ValidateEndpoint(endpoint string) error { return nil } + +const ( + variableNameFmt = `\$[a-z_]+` + variableNameErrMsg = "must start with '$' followed by lowercase letters and underscores only" +) + +var variableNameRegexp = regexp.MustCompile("^" + variableNameFmt + "$") + +// ValidateNginxVariableName validates an nginx variable name. +func (GenericValidator) ValidateNginxVariableName(name string) error { + if !variableNameRegexp.MatchString(name) { + examples := []string{ + "$upstream_addr", + "$remote_addr", + } + + return errors.New(k8svalidation.RegexError(variableNameFmt, variableNameErrMsg, examples...)) + } + + return nil +} diff --git a/internal/controller/nginx/config/validation/generic_test.go b/internal/controller/nginx/config/validation/generic_test.go index 5f57b51c56..73be3f10cb 100644 --- a/internal/controller/nginx/config/validation/generic_test.go +++ b/internal/controller/nginx/config/validation/generic_test.go @@ -112,3 +112,25 @@ func TestValidateEndpoint(t *testing.T) { `my$endpoint`, ) } + +func TestValidateNginxVariableName(t *testing.T) { + t.Parallel() + validator := GenericValidator{} + + testValidValuesForSimpleValidator( + t, + validator.ValidateNginxVariableName, + `$upstream_bytes_sent`, + `$upstream_last_server_name`, + `$remote_addr`, + ) + + testInvalidValuesForSimpleValidator( + t, + validator.ValidateNginxVariableName, + `1varname`, + `var-name`, + `var name`, + `var$name`, + ) +} diff --git a/internal/controller/state/change_processor.go b/internal/controller/state/change_processor.go index d661903b8c..87e29d5ad7 100644 --- a/internal/controller/state/change_processor.go +++ b/internal/controller/state/change_processor.go @@ -64,8 +64,8 @@ type ChangeProcessorConfig struct { GatewayCtlrName string // GatewayClassName is the name of the GatewayClass resource. GatewayClassName string - // ExperimentalFeatures indicates if experimental features are enabled. - ExperimentalFeatures bool + // Flags hold the feature flags + Flags graph.Flags } // ChangeProcessorImpl is an implementation of ChangeProcessor. @@ -278,7 +278,7 @@ func (c *ChangeProcessorImpl) Process() *graph.Graph { c.cfg.PlusSecrets, c.cfg.Validators, c.cfg.Logger, - c.cfg.ExperimentalFeatures, + c.cfg.Flags, ) return c.latestGraph diff --git a/internal/controller/state/graph/graph.go b/internal/controller/state/graph/graph.go index ffd86ac6e0..e90b96a4c8 100644 --- a/internal/controller/state/graph/graph.go +++ b/internal/controller/state/graph/graph.go @@ -92,6 +92,14 @@ type NginxReloadResult struct { // ProtectedPorts are the ports that may not be configured by a listener with a descriptive name of each port. type ProtectedPorts map[int32]string +// Flags hold the configuration flags for building the Graph. +type Flags struct { + // Plus indicates whether NGINX Plus features are enabled. + Plus bool + // Experimental indicates whether experimental features are enabled. + Experimental bool +} + // IsReferenced returns true if the Graph references the resource. func (g *Graph) IsReferenced(resourceType ngftypes.ObjectType, nsname types.NamespacedName) bool { switch obj := resourceType.(type) { @@ -208,7 +216,7 @@ func BuildGraph( plusSecrets map[types.NamespacedName][]PlusSecretFile, validators validation.Validators, logger logr.Logger, - experimentalEnabled bool, + flags Flags, ) *Graph { processedGwClasses, gcExists := processGatewayClasses(state.GatewayClasses, gcName, controllerName) if gcExists && processedGwClasses.Winner == nil { @@ -228,7 +236,7 @@ func BuildGraph( processedGwClasses.Winner, processedNginxProxies, state.CRDMetadata, - experimentalEnabled, + flags.Experimental, ) secretResolver := newSecretResolver(state.Secrets) @@ -242,7 +250,7 @@ func BuildGraph( gc, refGrantResolver, processedNginxProxies, - experimentalEnabled, + flags.Experimental, ) processedBackendTLSPolicies := processBackendTLSPolicies( @@ -319,7 +327,7 @@ func BuildGraph( PlusSecrets: plusSecrets, } - g.attachPolicies(validators.PolicyValidator, controllerName, logger) + g.attachPolicies(validators.PolicyValidator, controllerName, logger, flags.Plus) return g } diff --git a/internal/controller/state/graph/graph_test.go b/internal/controller/state/graph/graph_test.go index ed0e39082f..676d18144b 100644 --- a/internal/controller/state/graph/graph_test.go +++ b/internal/controller/state/graph/graph_test.go @@ -1493,7 +1493,9 @@ func TestBuildGraph(t *testing.T) { PolicyValidator: fakePolicyValidator, }, logr.Discard(), - test.experimentalEnabled, + Flags{ + Experimental: test.experimentalEnabled, + }, ) g.Expect(helpers.Diff(test.expected, result)).To(BeEmpty()) diff --git a/internal/controller/state/graph/multiple_gateways_test.go b/internal/controller/state/graph/multiple_gateways_test.go index c20fd98516..dc5b25dd59 100644 --- a/internal/controller/state/graph/multiple_gateways_test.go +++ b/internal/controller/state/graph/multiple_gateways_test.go @@ -409,7 +409,9 @@ func Test_MultipleGateways_WithNginxProxy(t *testing.T) { PolicyValidator: fakePolicyValidator, }, logr.Discard(), - experimentalFeaturesEnabled, + Flags{ + Experimental: experimentalFeaturesEnabled, + }, ) g.Expect(helpers.Diff(test.expGraph, result)).To(BeEmpty()) @@ -899,7 +901,9 @@ func Test_MultipleGateways_WithListeners(t *testing.T) { PolicyValidator: fakePolicyValidator, }, logr.Discard(), - experimentalFeaturesEnabled, + Flags{ + Experimental: experimentalFeaturesEnabled, + }, ) g.Expect(helpers.Diff(test.expGraph, result)).To(BeEmpty()) diff --git a/internal/controller/state/graph/policies.go b/internal/controller/state/graph/policies.go index 30e44fb014..e45bb608b3 100644 --- a/internal/controller/state/graph/policies.go +++ b/internal/controller/state/graph/policies.go @@ -116,7 +116,12 @@ func collectOrderedGatewaysForService( return append(existingGateways, newGateways...) } -func (g *Graph) attachPolicies(validator validation.PolicyValidator, ctlrName string, logger logr.Logger) { +func (g *Graph) attachPolicies( + validator validation.PolicyValidator, + ctlrName string, + logger logr.Logger, + plusEnabled bool, +) { if len(g.Gateways) == 0 { return } @@ -139,7 +144,7 @@ func (g *Graph) attachPolicies(validator validation.PolicyValidator, ctlrName st continue } - attachPolicyToService(policy, svc, g.Gateways, ctlrName, logger) + attachPolicyToService(policy, svc, g.Gateways, ctlrName, logger, plusEnabled, validator) } } } @@ -151,6 +156,8 @@ func attachPolicyToService( gws map[types.NamespacedName]*Gateway, ctlrName string, logger logr.Logger, + plusEnabled bool, + validator validation.PolicyValidator, ) { var attachedToAnyGateway bool @@ -215,6 +222,10 @@ func attachPolicyToService( attachedToAnyGateway = true } + if conds := validator.ValidateLoadBalancingMethod(policy.Source, plusEnabled); len(conds) > 0 { + policy.Conditions = append(policy.Conditions, conds...) + } + // Attach policy to service if effective for at least one gateway if attachedToAnyGateway { svc.Policies = append(svc.Policies, policy) diff --git a/internal/controller/state/graph/policies_test.go b/internal/controller/state/graph/policies_test.go index 251ab5aeb8..02bb745caa 100644 --- a/internal/controller/state/graph/policies_test.go +++ b/internal/controller/state/graph/policies_test.go @@ -242,7 +242,7 @@ func TestAttachPolicies(t *testing.T) { NGFPolicies: test.ngfPolicies, } - graph.attachPolicies(nil, "nginx-gateway", logr.Discard()) + graph.attachPolicies(&policiesfakes.FakeValidator{}, "nginx-gateway", logr.Discard(), false) for _, expect := range test.expects { expect(g, graph) } @@ -681,12 +681,23 @@ func TestAttachPolicyToService(t *testing.T) { } } + validatorError := &policiesfakes.FakeValidator{ + ValidateLoadBalancingMethodStub: func(_ policies.Policy, plusEnabled bool) []conditions.Condition { + if !plusEnabled { + return []conditions.Condition{conditions.NewPolicyInvalid("invalid load balancing method: NGINX Plus is required")} + } + return nil + }, + } + tests := []struct { policy *Policy svc *ReferencedService gws map[types.NamespacedName]*Gateway name string + validator policies.Validator expAncestors []PolicyAncestor + plus bool expAttached bool }{ { @@ -704,6 +715,7 @@ func TestAttachPolicyToService(t *testing.T) { Ancestor: getGatewayParentRef(gwNsname), }, }, + validator: &policiesfakes.FakeValidator{}, }, { name: "attachment; ancestor already exists so don't duplicate", @@ -728,6 +740,7 @@ func TestAttachPolicyToService(t *testing.T) { Ancestor: getGatewayParentRef(gwNsname), // only one ancestor per Gateway }, }, + validator: &policiesfakes.FakeValidator{}, }, { name: "attachment; existing gateway from policy status processed first", @@ -768,6 +781,7 @@ func TestAttachPolicyToService(t *testing.T) { Ancestor: getGatewayParentRef(gw2Nsname), // Only new gateway gets added }, }, + validator: &policiesfakes.FakeValidator{}, }, { name: "attachment; ancestor doesn't exist so add it", @@ -796,6 +810,7 @@ func TestAttachPolicyToService(t *testing.T) { Ancestor: getGatewayParentRef(gwNsname), }, }, + validator: &policiesfakes.FakeValidator{}, }, { name: "no attachment; gateway is invalid", @@ -813,6 +828,7 @@ func TestAttachPolicyToService(t *testing.T) { Conditions: []conditions.Condition{conditions.NewPolicyTargetNotFound("The Parent Gateway is invalid")}, }, }, + validator: &policiesfakes.FakeValidator{}, }, { name: "no attachment; max ancestor", @@ -825,6 +841,7 @@ func TestAttachPolicyToService(t *testing.T) { gws: getGateway(true /*valid*/), expAttached: false, expAncestors: nil, + validator: &policiesfakes.FakeValidator{}, }, { name: "no attachment; does not belong to gateway", @@ -837,6 +854,7 @@ func TestAttachPolicyToService(t *testing.T) { gws: getGateway(true /*valid*/), expAttached: false, expAncestors: nil, + validator: &policiesfakes.FakeValidator{}, }, { name: "no attachment; gateway is invalid", @@ -863,6 +881,24 @@ func TestAttachPolicyToService(t *testing.T) { Ancestor: getGatewayParentRef(gwNsname), }, }, + validator: &policiesfakes.FakeValidator{}, + }, + { + name: "no attachment: invalid load balancing setting for OSS", + policy: &Policy{Source: &policiesfakes.FakePolicy{}, InvalidForGateways: map[types.NamespacedName]struct{}{}}, + svc: &ReferencedService{ + GatewayNsNames: map[types.NamespacedName]struct{}{ + gwNsname: {}, + }, + }, + gws: getGateway(true /*valid*/), + expAttached: true, + expAncestors: []PolicyAncestor{ + { + Ancestor: getGatewayParentRef(gwNsname), + }, + }, + validator: validatorError, }, } @@ -871,7 +907,7 @@ func TestAttachPolicyToService(t *testing.T) { t.Parallel() g := NewWithT(t) - attachPolicyToService(test.policy, test.svc, test.gws, "ctlr", logr.Discard()) + attachPolicyToService(test.policy, test.svc, test.gws, "ctlr", logr.Discard(), test.plus, test.validator) if test.expAttached { g.Expect(test.svc.Policies).To(HaveLen(1)) } else { @@ -2272,7 +2308,7 @@ func TestNGFPolicyAncestorLimitHandling(t *testing.T) { } // Call attachPolicies to trigger the ancestor limit logic - graph.attachPolicies(validator, "nginx-gateway", testLogger) + graph.attachPolicies(validator, "nginx-gateway", testLogger, false) // Verify that the policy with full ancestors has no actual ancestors assigned policyFullKey := PolicyKey{ diff --git a/internal/controller/state/validation/validationfakes/fake_generic_validator.go b/internal/controller/state/validation/validationfakes/fake_generic_validator.go index 8c83a4ff9a..cd162c7359 100644 --- a/internal/controller/state/validation/validationfakes/fake_generic_validator.go +++ b/internal/controller/state/validation/validationfakes/fake_generic_validator.go @@ -52,6 +52,17 @@ type FakeGenericValidator struct { validateNginxSizeReturnsOnCall map[int]struct { result1 error } + ValidateNginxVariableNameStub func(string) error + validateNginxVariableNameMutex sync.RWMutex + validateNginxVariableNameArgsForCall []struct { + arg1 string + } + validateNginxVariableNameReturns struct { + result1 error + } + validateNginxVariableNameReturnsOnCall map[int]struct { + result1 error + } ValidateServiceNameStub func(string) error validateServiceNameMutex sync.RWMutex validateServiceNameArgsForCall []struct { @@ -311,6 +322,67 @@ func (fake *FakeGenericValidator) ValidateNginxSizeReturnsOnCall(i int, result1 }{result1} } +func (fake *FakeGenericValidator) ValidateNginxVariableName(arg1 string) error { + fake.validateNginxVariableNameMutex.Lock() + ret, specificReturn := fake.validateNginxVariableNameReturnsOnCall[len(fake.validateNginxVariableNameArgsForCall)] + fake.validateNginxVariableNameArgsForCall = append(fake.validateNginxVariableNameArgsForCall, struct { + arg1 string + }{arg1}) + stub := fake.ValidateNginxVariableNameStub + fakeReturns := fake.validateNginxVariableNameReturns + fake.recordInvocation("ValidateNginxVariableName", []interface{}{arg1}) + fake.validateNginxVariableNameMutex.Unlock() + if stub != nil { + return stub(arg1) + } + if specificReturn { + return ret.result1 + } + return fakeReturns.result1 +} + +func (fake *FakeGenericValidator) ValidateNginxVariableNameCallCount() int { + fake.validateNginxVariableNameMutex.RLock() + defer fake.validateNginxVariableNameMutex.RUnlock() + return len(fake.validateNginxVariableNameArgsForCall) +} + +func (fake *FakeGenericValidator) ValidateNginxVariableNameCalls(stub func(string) error) { + fake.validateNginxVariableNameMutex.Lock() + defer fake.validateNginxVariableNameMutex.Unlock() + fake.ValidateNginxVariableNameStub = stub +} + +func (fake *FakeGenericValidator) ValidateNginxVariableNameArgsForCall(i int) string { + fake.validateNginxVariableNameMutex.RLock() + defer fake.validateNginxVariableNameMutex.RUnlock() + argsForCall := fake.validateNginxVariableNameArgsForCall[i] + return argsForCall.arg1 +} + +func (fake *FakeGenericValidator) ValidateNginxVariableNameReturns(result1 error) { + fake.validateNginxVariableNameMutex.Lock() + defer fake.validateNginxVariableNameMutex.Unlock() + fake.ValidateNginxVariableNameStub = nil + fake.validateNginxVariableNameReturns = struct { + result1 error + }{result1} +} + +func (fake *FakeGenericValidator) ValidateNginxVariableNameReturnsOnCall(i int, result1 error) { + fake.validateNginxVariableNameMutex.Lock() + defer fake.validateNginxVariableNameMutex.Unlock() + fake.ValidateNginxVariableNameStub = nil + if fake.validateNginxVariableNameReturnsOnCall == nil { + fake.validateNginxVariableNameReturnsOnCall = make(map[int]struct { + result1 error + }) + } + fake.validateNginxVariableNameReturnsOnCall[i] = struct { + result1 error + }{result1} +} + func (fake *FakeGenericValidator) ValidateServiceName(arg1 string) error { fake.validateServiceNameMutex.Lock() ret, specificReturn := fake.validateServiceNameReturnsOnCall[len(fake.validateServiceNameArgsForCall)] diff --git a/internal/controller/state/validation/validationfakes/fake_policy_validator.go b/internal/controller/state/validation/validationfakes/fake_policy_validator.go index 0cb8b7f232..789c42f383 100644 --- a/internal/controller/state/validation/validationfakes/fake_policy_validator.go +++ b/internal/controller/state/validation/validationfakes/fake_policy_validator.go @@ -45,6 +45,18 @@ type FakePolicyValidator struct { validateGlobalSettingsReturnsOnCall map[int]struct { result1 []conditions.Condition } + ValidateLoadBalancingMethodStub func(policies.Policy, bool) []conditions.Condition + validateLoadBalancingMethodMutex sync.RWMutex + validateLoadBalancingMethodArgsForCall []struct { + arg1 policies.Policy + arg2 bool + } + validateLoadBalancingMethodReturns struct { + result1 []conditions.Condition + } + validateLoadBalancingMethodReturnsOnCall map[int]struct { + result1 []conditions.Condition + } invocations map[string][][]interface{} invocationsMutex sync.RWMutex } @@ -234,6 +246,68 @@ func (fake *FakePolicyValidator) ValidateGlobalSettingsReturnsOnCall(i int, resu }{result1} } +func (fake *FakePolicyValidator) ValidateLoadBalancingMethod(arg1 policies.Policy, arg2 bool) []conditions.Condition { + fake.validateLoadBalancingMethodMutex.Lock() + ret, specificReturn := fake.validateLoadBalancingMethodReturnsOnCall[len(fake.validateLoadBalancingMethodArgsForCall)] + fake.validateLoadBalancingMethodArgsForCall = append(fake.validateLoadBalancingMethodArgsForCall, struct { + arg1 policies.Policy + arg2 bool + }{arg1, arg2}) + stub := fake.ValidateLoadBalancingMethodStub + fakeReturns := fake.validateLoadBalancingMethodReturns + fake.recordInvocation("ValidateLoadBalancingMethod", []interface{}{arg1, arg2}) + fake.validateLoadBalancingMethodMutex.Unlock() + if stub != nil { + return stub(arg1, arg2) + } + if specificReturn { + return ret.result1 + } + return fakeReturns.result1 +} + +func (fake *FakePolicyValidator) ValidateLoadBalancingMethodCallCount() int { + fake.validateLoadBalancingMethodMutex.RLock() + defer fake.validateLoadBalancingMethodMutex.RUnlock() + return len(fake.validateLoadBalancingMethodArgsForCall) +} + +func (fake *FakePolicyValidator) ValidateLoadBalancingMethodCalls(stub func(policies.Policy, bool) []conditions.Condition) { + fake.validateLoadBalancingMethodMutex.Lock() + defer fake.validateLoadBalancingMethodMutex.Unlock() + fake.ValidateLoadBalancingMethodStub = stub +} + +func (fake *FakePolicyValidator) ValidateLoadBalancingMethodArgsForCall(i int) (policies.Policy, bool) { + fake.validateLoadBalancingMethodMutex.RLock() + defer fake.validateLoadBalancingMethodMutex.RUnlock() + argsForCall := fake.validateLoadBalancingMethodArgsForCall[i] + return argsForCall.arg1, argsForCall.arg2 +} + +func (fake *FakePolicyValidator) ValidateLoadBalancingMethodReturns(result1 []conditions.Condition) { + fake.validateLoadBalancingMethodMutex.Lock() + defer fake.validateLoadBalancingMethodMutex.Unlock() + fake.ValidateLoadBalancingMethodStub = nil + fake.validateLoadBalancingMethodReturns = struct { + result1 []conditions.Condition + }{result1} +} + +func (fake *FakePolicyValidator) ValidateLoadBalancingMethodReturnsOnCall(i int, result1 []conditions.Condition) { + fake.validateLoadBalancingMethodMutex.Lock() + defer fake.validateLoadBalancingMethodMutex.Unlock() + fake.ValidateLoadBalancingMethodStub = nil + if fake.validateLoadBalancingMethodReturnsOnCall == nil { + fake.validateLoadBalancingMethodReturnsOnCall = make(map[int]struct { + result1 []conditions.Condition + }) + } + fake.validateLoadBalancingMethodReturnsOnCall[i] = struct { + result1 []conditions.Condition + }{result1} +} + func (fake *FakePolicyValidator) Invocations() map[string][][]interface{} { fake.invocationsMutex.RLock() defer fake.invocationsMutex.RUnlock() diff --git a/internal/controller/state/validation/validator.go b/internal/controller/state/validation/validator.go index 10dc1fe8c3..cf0138aa15 100644 --- a/internal/controller/state/validation/validator.go +++ b/internal/controller/state/validation/validator.go @@ -49,6 +49,7 @@ type GenericValidator interface { ValidateNginxDuration(duration string) error ValidateNginxSize(size string) error ValidateEndpoint(endpoint string) error + ValidateNginxVariableName(name string) error } // PolicyValidator validates an NGF Policy. @@ -61,6 +62,8 @@ type PolicyValidator interface { ValidateGlobalSettings(policy policies.Policy, globalSettings *policies.GlobalSettings) []conditions.Condition // Conflicts returns true if the two Policies conflict. Conflicts(a, b policies.Policy) bool + // ValidateLoadBalancingMethod validates the load balancing method for upstream servers. + ValidateLoadBalancingMethod(policy policies.Policy, plusEnabled bool) []conditions.Condition } // SkipValidator is used to skip validation on internally-created routes for request mirroring. diff --git a/tests/cel/common.go b/tests/cel/common.go index 6208c1a75f..2a0c3ddb7c 100644 --- a/tests/cel/common.go +++ b/tests/cel/common.go @@ -56,9 +56,11 @@ const ( // UpstreamSettingsPolicy validation errors. const ( - expectedTargetRefKindServiceError = `TargetRefs Kind must be: Service` - expectedTargetRefGroupCoreError = `TargetRefs Group must be core` - expectedTargetRefNameUniqueError = `TargetRef Name must be unique` + expectedTargetRefKindServiceError = `TargetRefs Kind must be: Service` + expectedTargetRefGroupCoreError = `TargetRefs Group must be core` + expectedTargetRefNameUniqueError = `TargetRef Name must be unique` + expectedHashKeyLoadBalancingTypeError = `hashKey is required when loadBalancingMethod ` + + `is 'hash' or 'hash consistent'` ) // SnippetsFilter validation errors. diff --git a/tests/cel/upstreamsettingspolicy_test.go b/tests/cel/upstreamsettingspolicy_test.go index 35a4fbd364..3a82af32be 100644 --- a/tests/cel/upstreamsettingspolicy_test.go +++ b/tests/cel/upstreamsettingspolicy_test.go @@ -7,6 +7,7 @@ import ( gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" ngfAPIv1alpha1 "github.com/nginx/nginx-gateway-fabric/v2/apis/v1alpha1" + "github.com/nginx/nginx-gateway-fabric/v2/internal/framework/helpers" ) func TestUpstreamSettingsPolicyTargetRefKind(t *testing.T) { @@ -372,3 +373,86 @@ func TestUpstreamSettingsPolicyTargetRefNameUniqueness(t *testing.T) { }) } } + +func TestUpstreamSettingsPolicy_LoadBalancing(t *testing.T) { + t.Parallel() + k8sClient := getKubernetesClient(t) + + tests := []struct { + spec ngfAPIv1alpha1.UpstreamSettingsPolicySpec + name string + wantErrors []string + }{ + { + name: "when load balancing method is hash, hash key is required, error expected", + spec: ngfAPIv1alpha1.UpstreamSettingsPolicySpec{ + TargetRefs: []gatewayv1.LocalPolicyTargetReference{ + { + Kind: serviceKind, + Group: coreGroup, + }, + }, + LoadBalancingMethod: helpers.GetPointer(ngfAPIv1alpha1.LoadBalancingTypeHash), + }, + wantErrors: []string{expectedHashKeyLoadBalancingTypeError}, + }, + { + name: "when load balancing method is hash consistent, hash key is required, error expected", + spec: ngfAPIv1alpha1.UpstreamSettingsPolicySpec{ + TargetRefs: []gatewayv1.LocalPolicyTargetReference{ + { + Kind: serviceKind, + Group: coreGroup, + }, + }, + LoadBalancingMethod: helpers.GetPointer(ngfAPIv1alpha1.LoadBalancingTypeHashConsistent), + }, + wantErrors: []string{expectedHashKeyLoadBalancingTypeError}, + }, + { + name: "specify load balancing method as hash and set the hash key, no error expected", + spec: ngfAPIv1alpha1.UpstreamSettingsPolicySpec{ + TargetRefs: []gatewayv1.LocalPolicyTargetReference{ + { + Kind: serviceKind, + Group: coreGroup, + }, + }, + LoadBalancingMethod: helpers.GetPointer(ngfAPIv1alpha1.LoadBalancingTypeHash), + HashKey: helpers.GetPointer(ngfAPIv1alpha1.HashMethodKey("$upstream_connect_time")), + }, + }, + { + name: "specify load balancing method as hash consistent and set the hash key, no error expected", + spec: ngfAPIv1alpha1.UpstreamSettingsPolicySpec{ + TargetRefs: []gatewayv1.LocalPolicyTargetReference{ + { + Kind: serviceKind, + Group: coreGroup, + }, + }, + LoadBalancingMethod: helpers.GetPointer(ngfAPIv1alpha1.LoadBalancingTypeHashConsistent), + HashKey: helpers.GetPointer(ngfAPIv1alpha1.HashMethodKey("$upstream_bytes_sent")), + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + for i := range tt.spec.TargetRefs { + tt.spec.TargetRefs[i].Name = gatewayv1.ObjectName(uniqueResourceName(testTargetRefName)) + } + + upstreamSettingsPolicy := &ngfAPIv1alpha1.UpstreamSettingsPolicy{ + ObjectMeta: controllerruntime.ObjectMeta{ + Name: uniqueResourceName(testResourceName), + Namespace: defaultNamespace, + }, + Spec: tt.spec, + } + validateCrd(t, tt.wantErrors, upstreamSettingsPolicy, k8sClient) + }) + } +} From fbfbd6c479e866d20abb56ef09fc0210afe49d87 Mon Sep 17 00:00:00 2001 From: Saloni Choudhary <146118978+salonichf5@users.noreply.github.com> Date: Fri, 21 Nov 2025 10:41:17 -0700 Subject: [PATCH 2/6] Apply suggestions from code review Co-authored-by: Saylor Berman --- apis/v1alpha1/upstreamsettingspolicy_types.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/apis/v1alpha1/upstreamsettingspolicy_types.go b/apis/v1alpha1/upstreamsettingspolicy_types.go index 2c56ec4622..e58c035a1a 100644 --- a/apis/v1alpha1/upstreamsettingspolicy_types.go +++ b/apis/v1alpha1/upstreamsettingspolicy_types.go @@ -131,7 +131,7 @@ const ( // - https://nginx.org/en/docs/http/ngx_http_upstream_module.html#ip_hash // - https://nginx.org/en/docs/http/ngx_http_upstream_module.html#hash - // LoadBalancingMethods for NGINX OSS. + // LoadBalancingMethods supported by NGINX OSS and NGINX Plus. // LoadBalancingTypeRoundRobin enables round-robin load balancing, // distributing requests evenly across all upstream servers. @@ -172,7 +172,7 @@ const ( // fewer active connections. LoadBalancingTypeRandomTwoLeastConnection LoadBalancingType = "random two least_conn" - // LoadBalancingMethods for NGINX Plus along with OSS methods. + // LoadBalancingMethods supported by NGINX Plus. // LoadBalancingTypeRandomTwoLeastTimeHeader enables a variation of least-time load balancing // that randomly selects two servers and forwards traffic to the one with the least From f3bddb747625dbe0af7b58007c2b7192239c5005 Mon Sep 17 00:00:00 2001 From: salonichf5 <146118978+salonichf5@users.noreply.github.com> Date: Fri, 21 Nov 2025 12:27:31 -0700 Subject: [PATCH 3/6] move load balancing checks to generic validator --- apis/v1alpha1/upstreamsettingspolicy_types.go | 11 ++- apis/v1alpha1/zz_generated.deepcopy.go | 4 +- ...ay.nginx.org_upstreamsettingspolicies.yaml | 10 +-- deploy/crds.yaml | 10 +-- internal/controller/manager.go | 18 ++--- .../controller/nginx/config/http/config.go | 33 +-------- .../policies/clientsettings/validator.go | 8 -- .../policies/observability/validator.go | 8 -- .../policies/policiesfakes/fake_validator.go | 74 ------------------- .../policies/upstreamsettings/processor.go | 8 +- .../upstreamsettings/processor_test.go | 16 ++-- .../policies/upstreamsettings/validator.go | 72 +++++++----------- .../upstreamsettings/validator_test.go | 47 +++--------- .../nginx/config/policies/validator.go | 13 ---- internal/controller/nginx/config/upstreams.go | 4 +- .../controller/nginx/config/upstreams_test.go | 18 ++--- .../nginx/config/validation/generic.go | 55 ++++++++++++++ .../nginx/config/validation/generic_test.go | 31 ++++++++ internal/controller/state/change_processor.go | 6 +- internal/controller/state/graph/graph.go | 16 +--- internal/controller/state/graph/graph_test.go | 4 +- .../state/graph/multiple_gateways_test.go | 8 +- internal/controller/state/graph/policies.go | 15 +--- .../controller/state/graph/policies_test.go | 42 +---------- .../validationfakes/fake_generic_validator.go | 74 +++++++++++++++++++ .../validationfakes/fake_policy_validator.go | 74 ------------------- .../controller/state/validation/validator.go | 3 +- tests/cel/common.go | 2 +- tests/cel/upstreamsettingspolicy_test.go | 4 +- 29 files changed, 263 insertions(+), 425 deletions(-) diff --git a/apis/v1alpha1/upstreamsettingspolicy_types.go b/apis/v1alpha1/upstreamsettingspolicy_types.go index e58c035a1a..82147d014c 100644 --- a/apis/v1alpha1/upstreamsettingspolicy_types.go +++ b/apis/v1alpha1/upstreamsettingspolicy_types.go @@ -36,7 +36,7 @@ type UpstreamSettingsPolicyList struct { } // UpstreamSettingsPolicySpec defines the desired state of the UpstreamSettingsPolicy. -// +kubebuilder:validation:XValidation:rule="!(has(self.loadBalancingMethod) && (self.loadBalancingMethod == 'hash' || self.loadBalancingMethod == 'hash consistent')) || has(self.hashKey)",message="hashKey is required when loadBalancingMethod is 'hash' or 'hash consistent'" +// +kubebuilder:validation:XValidation:rule="!(has(self.loadBalancingMethod) && (self.loadBalancingMethod == 'hash' || self.loadBalancingMethod == 'hash consistent')) || has(self.hashMethodKey)",message="hashMethodKey is required when loadBalancingMethod is 'hash' or 'hash consistent'" // //nolint:lll type UpstreamSettingsPolicySpec struct { @@ -61,11 +61,11 @@ type UpstreamSettingsPolicySpec struct { // +optional LoadBalancingMethod *LoadBalancingType `json:"loadBalancingMethod,omitempty"` - // HashKey defines the key used for hash-based load balancing methods. + // HashMethodKey defines the key used for hash-based load balancing methods. // This field is required when `LoadBalancingMethod` is set to `hash` or `hash consistent`. // // +optional - HashKey *HashMethodKey `json:"hashKey,omitempty"` + HashMethodKey *HashMethodKey `json:"hashMethodKey,omitempty"` // TargetRefs identifies API object(s) to apply the policy to. // Objects must be in the same namespace as the policy. @@ -135,7 +135,6 @@ const ( // LoadBalancingTypeRoundRobin enables round-robin load balancing, // distributing requests evenly across all upstream servers. - // NGINX defaults to this method if no load balancing method is specified. LoadBalancingTypeRoundRobin LoadBalancingType = "round_robin" // LoadBalancingTypeLeastConn enables least-connections load balancing, @@ -148,13 +147,13 @@ const ( // LoadBalancingTypeHash enables generic hash-based load balancing, // routing requests to upstream servers based on a hash of a specified key - // HashKey field must be set when this method is selected. + // HashMethodKey field must be set when this method is selected. // Example configuration: hash $binary_remote_addr;. LoadBalancingTypeHash LoadBalancingType = "hash" // LoadBalancingTypeHashConsistent enables consistent hash-based load balancing, // which minimizes the number of keys remapped when a server is added or removed. - // HashKey field must be set when this method is selected. + // HashMethodKey field must be set when this method is selected. // Example configuration: hash $binary_remote_addr consistent;. LoadBalancingTypeHashConsistent LoadBalancingType = "hash consistent" diff --git a/apis/v1alpha1/zz_generated.deepcopy.go b/apis/v1alpha1/zz_generated.deepcopy.go index af4635e749..164bef0cba 100644 --- a/apis/v1alpha1/zz_generated.deepcopy.go +++ b/apis/v1alpha1/zz_generated.deepcopy.go @@ -561,8 +561,8 @@ func (in *UpstreamSettingsPolicySpec) DeepCopyInto(out *UpstreamSettingsPolicySp *out = new(LoadBalancingType) **out = **in } - if in.HashKey != nil { - in, out := &in.HashKey, &out.HashKey + if in.HashMethodKey != nil { + in, out := &in.HashMethodKey, &out.HashMethodKey *out = new(HashMethodKey) **out = **in } diff --git a/config/crd/bases/gateway.nginx.org_upstreamsettingspolicies.yaml b/config/crd/bases/gateway.nginx.org_upstreamsettingspolicies.yaml index 120707cf58..c8cda0c218 100644 --- a/config/crd/bases/gateway.nginx.org_upstreamsettingspolicies.yaml +++ b/config/crd/bases/gateway.nginx.org_upstreamsettingspolicies.yaml @@ -51,9 +51,9 @@ spec: spec: description: Spec defines the desired state of the UpstreamSettingsPolicy. properties: - hashKey: + hashMethodKey: description: |- - HashKey defines the key used for hash-based load balancing methods. + HashMethodKey defines the key used for hash-based load balancing methods. This field is required when `LoadBalancingMethod` is set to `hash` or `hash consistent`. pattern: ^\$[a-z_]+$ type: string @@ -171,11 +171,11 @@ spec: - targetRefs type: object x-kubernetes-validations: - - message: hashKey is required when loadBalancingMethod is 'hash' or 'hash - consistent' + - message: hashMethodKey is required when loadBalancingMethod is 'hash' + or 'hash consistent' rule: '!(has(self.loadBalancingMethod) && (self.loadBalancingMethod == ''hash'' || self.loadBalancingMethod == ''hash consistent'')) || - has(self.hashKey)' + has(self.hashMethodKey)' status: description: Status defines the state of the UpstreamSettingsPolicy. properties: diff --git a/deploy/crds.yaml b/deploy/crds.yaml index f21e71fff3..6cf54dd1f5 100644 --- a/deploy/crds.yaml +++ b/deploy/crds.yaml @@ -9578,9 +9578,9 @@ spec: spec: description: Spec defines the desired state of the UpstreamSettingsPolicy. properties: - hashKey: + hashMethodKey: description: |- - HashKey defines the key used for hash-based load balancing methods. + HashMethodKey defines the key used for hash-based load balancing methods. This field is required when `LoadBalancingMethod` is set to `hash` or `hash consistent`. pattern: ^\$[a-z_]+$ type: string @@ -9698,11 +9698,11 @@ spec: - targetRefs type: object x-kubernetes-validations: - - message: hashKey is required when loadBalancingMethod is 'hash' or 'hash - consistent' + - message: hashMethodKey is required when loadBalancingMethod is 'hash' + or 'hash consistent' rule: '!(has(self.loadBalancingMethod) && (self.loadBalancingMethod == ''hash'' || self.loadBalancingMethod == ''hash consistent'')) || - has(self.hashKey)' + has(self.hashMethodKey)' status: description: Status defines the state of the UpstreamSettingsPolicy. properties: diff --git a/internal/controller/manager.go b/internal/controller/manager.go index 8987c8ba4b..fa5abd069e 100644 --- a/internal/controller/manager.go +++ b/internal/controller/manager.go @@ -124,18 +124,13 @@ func StartManager(cfg config.Config) error { mustExtractGVK := kinds.NewMustExtractGKV(scheme) genericValidator := ngxvalidation.GenericValidator{} - policyManager := createPolicyManager(mustExtractGVK, genericValidator) + policyManager := createPolicyManager(mustExtractGVK, cfg.Plus, genericValidator) plusSecrets, err := createPlusSecretMetadata(cfg, mgr.GetAPIReader()) if err != nil { return err } - flags := graph.Flags{ - Plus: cfg.Plus, - Experimental: cfg.ExperimentalFeatures, - } - processor := state.NewChangeProcessorImpl(state.ChangeProcessorConfig{ GatewayCtlrName: cfg.GatewayCtlrName, GatewayClassName: cfg.GatewayClassName, @@ -145,10 +140,10 @@ func StartManager(cfg config.Config) error { GenericValidator: genericValidator, PolicyValidator: policyManager, }, - EventRecorder: recorder, - MustExtractGVK: mustExtractGVK, - PlusSecrets: plusSecrets, - Flags: flags, + EventRecorder: recorder, + MustExtractGVK: mustExtractGVK, + PlusSecrets: plusSecrets, + ExperimentalFeatures: cfg.ExperimentalFeatures, }) var handlerCollector handlerMetricsCollector = collectors.NewControllerNoopCollector() @@ -327,6 +322,7 @@ func StartManager(cfg config.Config) error { func createPolicyManager( mustExtractGVK kinds.MustExtractGVK, + plusEnabled bool, validator validation.GenericValidator, ) *policies.CompositeValidator { cfgs := []policies.ManagerConfig{ @@ -340,7 +336,7 @@ func createPolicyManager( }, { GVK: mustExtractGVK(&ngfAPIv1alpha1.UpstreamSettingsPolicy{}), - Validator: upstreamsettings.NewValidator(validator), + Validator: upstreamsettings.NewValidator(validator, plusEnabled), }, } diff --git a/internal/controller/nginx/config/http/config.go b/internal/controller/nginx/config/http/config.go index 3db66b4345..c3dfbf23be 100644 --- a/internal/controller/nginx/config/http/config.go +++ b/internal/controller/nginx/config/http/config.go @@ -1,7 +1,6 @@ package http import ( - ngfAPI "github.com/nginx/nginx-gateway-fabric/v2/apis/v1alpha1" "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/nginx/config/shared" ) @@ -124,7 +123,7 @@ type Upstream struct { ZoneSize string // format: 512k, 1m StateFile string LoadBalancingMethod string - HashKey string + HashMethodKey string KeepAlive UpstreamKeepAlive Servers []UpstreamServer } @@ -169,33 +168,3 @@ type ServerConfig struct { Plus bool DisableSNIHostValidation bool } - -var ( - PlusAllowedLBMethods = map[ngfAPI.LoadBalancingType]struct{}{ - ngfAPI.LoadBalancingTypeRoundRobin: {}, - ngfAPI.LoadBalancingTypeLeastConnection: {}, - ngfAPI.LoadBalancingTypeIPHash: {}, - ngfAPI.LoadBalancingTypeRandom: {}, - ngfAPI.LoadBalancingTypeHash: {}, - ngfAPI.LoadBalancingTypeHashConsistent: {}, - ngfAPI.LoadBalancingTypeRandomTwo: {}, - ngfAPI.LoadBalancingTypeRandomTwoLeastConnection: {}, - ngfAPI.LoadBalancingTypeLeastTimeHeader: {}, - ngfAPI.LoadBalancingTypeLeastTimeLastByte: {}, - ngfAPI.LoadBalancingTypeLeastTimeHeaderInflight: {}, - ngfAPI.LoadBalancingTypeLeastTimeLastByteInflight: {}, - ngfAPI.LoadBalancingTypeRandomTwoLeastTimeHeader: {}, - ngfAPI.LoadBalancingTypeRandomTwoLeastTimeLastByte: {}, - } - - OSSAllowedLBMethods = map[ngfAPI.LoadBalancingType]struct{}{ - ngfAPI.LoadBalancingTypeRoundRobin: {}, - ngfAPI.LoadBalancingTypeLeastConnection: {}, - ngfAPI.LoadBalancingTypeIPHash: {}, - ngfAPI.LoadBalancingTypeRandom: {}, - ngfAPI.LoadBalancingTypeHash: {}, - ngfAPI.LoadBalancingTypeHashConsistent: {}, - ngfAPI.LoadBalancingTypeRandomTwo: {}, - ngfAPI.LoadBalancingTypeRandomTwoLeastConnection: {}, - } -) diff --git a/internal/controller/nginx/config/policies/clientsettings/validator.go b/internal/controller/nginx/config/policies/clientsettings/validator.go index 58d7655984..c4d83864e7 100644 --- a/internal/controller/nginx/config/policies/clientsettings/validator.go +++ b/internal/controller/nginx/config/policies/clientsettings/validator.go @@ -50,14 +50,6 @@ func (v *Validator) ValidateGlobalSettings( return nil } -// ValidateLoadBalancingMethod validates the load balancing method for upstream servers. -func (v *Validator) ValidateLoadBalancingMethod( - _ policies.Policy, - _ bool, -) []conditions.Condition { - return nil -} - // Conflicts returns true if the two ClientSettingsPolicies conflict. func (v *Validator) Conflicts(polA, polB policies.Policy) bool { cspA := helpers.MustCastObject[*ngfAPI.ClientSettingsPolicy](polA) diff --git a/internal/controller/nginx/config/policies/observability/validator.go b/internal/controller/nginx/config/policies/observability/validator.go index 1e4f65f151..43d47dce3e 100644 --- a/internal/controller/nginx/config/policies/observability/validator.go +++ b/internal/controller/nginx/config/policies/observability/validator.go @@ -141,11 +141,3 @@ func (v *Validator) validateSettings(spec ngfAPIv1alpha2.ObservabilityPolicySpec return allErrs.ToAggregate() } - -// ValidateLoadBalancingMethod validates the load balancing method for upstream servers. -func (v *Validator) ValidateLoadBalancingMethod( - _ policies.Policy, - _ bool, -) []conditions.Condition { - return nil -} diff --git a/internal/controller/nginx/config/policies/policiesfakes/fake_validator.go b/internal/controller/nginx/config/policies/policiesfakes/fake_validator.go index 2707518f16..9389e6ccff 100644 --- a/internal/controller/nginx/config/policies/policiesfakes/fake_validator.go +++ b/internal/controller/nginx/config/policies/policiesfakes/fake_validator.go @@ -44,18 +44,6 @@ type FakeValidator struct { validateGlobalSettingsReturnsOnCall map[int]struct { result1 []conditions.Condition } - ValidateLoadBalancingMethodStub func(policies.Policy, bool) []conditions.Condition - validateLoadBalancingMethodMutex sync.RWMutex - validateLoadBalancingMethodArgsForCall []struct { - arg1 policies.Policy - arg2 bool - } - validateLoadBalancingMethodReturns struct { - result1 []conditions.Condition - } - validateLoadBalancingMethodReturnsOnCall map[int]struct { - result1 []conditions.Condition - } invocations map[string][][]interface{} invocationsMutex sync.RWMutex } @@ -245,68 +233,6 @@ func (fake *FakeValidator) ValidateGlobalSettingsReturnsOnCall(i int, result1 [] }{result1} } -func (fake *FakeValidator) ValidateLoadBalancingMethod(arg1 policies.Policy, arg2 bool) []conditions.Condition { - fake.validateLoadBalancingMethodMutex.Lock() - ret, specificReturn := fake.validateLoadBalancingMethodReturnsOnCall[len(fake.validateLoadBalancingMethodArgsForCall)] - fake.validateLoadBalancingMethodArgsForCall = append(fake.validateLoadBalancingMethodArgsForCall, struct { - arg1 policies.Policy - arg2 bool - }{arg1, arg2}) - stub := fake.ValidateLoadBalancingMethodStub - fakeReturns := fake.validateLoadBalancingMethodReturns - fake.recordInvocation("ValidateLoadBalancingMethod", []interface{}{arg1, arg2}) - fake.validateLoadBalancingMethodMutex.Unlock() - if stub != nil { - return stub(arg1, arg2) - } - if specificReturn { - return ret.result1 - } - return fakeReturns.result1 -} - -func (fake *FakeValidator) ValidateLoadBalancingMethodCallCount() int { - fake.validateLoadBalancingMethodMutex.RLock() - defer fake.validateLoadBalancingMethodMutex.RUnlock() - return len(fake.validateLoadBalancingMethodArgsForCall) -} - -func (fake *FakeValidator) ValidateLoadBalancingMethodCalls(stub func(policies.Policy, bool) []conditions.Condition) { - fake.validateLoadBalancingMethodMutex.Lock() - defer fake.validateLoadBalancingMethodMutex.Unlock() - fake.ValidateLoadBalancingMethodStub = stub -} - -func (fake *FakeValidator) ValidateLoadBalancingMethodArgsForCall(i int) (policies.Policy, bool) { - fake.validateLoadBalancingMethodMutex.RLock() - defer fake.validateLoadBalancingMethodMutex.RUnlock() - argsForCall := fake.validateLoadBalancingMethodArgsForCall[i] - return argsForCall.arg1, argsForCall.arg2 -} - -func (fake *FakeValidator) ValidateLoadBalancingMethodReturns(result1 []conditions.Condition) { - fake.validateLoadBalancingMethodMutex.Lock() - defer fake.validateLoadBalancingMethodMutex.Unlock() - fake.ValidateLoadBalancingMethodStub = nil - fake.validateLoadBalancingMethodReturns = struct { - result1 []conditions.Condition - }{result1} -} - -func (fake *FakeValidator) ValidateLoadBalancingMethodReturnsOnCall(i int, result1 []conditions.Condition) { - fake.validateLoadBalancingMethodMutex.Lock() - defer fake.validateLoadBalancingMethodMutex.Unlock() - fake.ValidateLoadBalancingMethodStub = nil - if fake.validateLoadBalancingMethodReturnsOnCall == nil { - fake.validateLoadBalancingMethodReturnsOnCall = make(map[int]struct { - result1 []conditions.Condition - }) - } - fake.validateLoadBalancingMethodReturnsOnCall[i] = struct { - result1 []conditions.Condition - }{result1} -} - func (fake *FakeValidator) Invocations() map[string][][]interface{} { fake.invocationsMutex.RLock() defer fake.invocationsMutex.RUnlock() diff --git a/internal/controller/nginx/config/policies/upstreamsettings/processor.go b/internal/controller/nginx/config/policies/upstreamsettings/processor.go index 51906bcec0..9b4f23c7a7 100644 --- a/internal/controller/nginx/config/policies/upstreamsettings/processor.go +++ b/internal/controller/nginx/config/policies/upstreamsettings/processor.go @@ -15,8 +15,8 @@ type UpstreamSettings struct { ZoneSize string // LoadBalancingMethod is the load balancing method setting. LoadBalancingMethod string - // HashKey is the key to be used for hash-based load balancing methods. - HashKey string + // HashMethodKey is the key to be used for hash-based load balancing methods. + HashMethodKey string // KeepAlive contains the keepalive settings. KeepAlive http.UpstreamKeepAlive } @@ -70,8 +70,8 @@ func processPolicies(pols []policies.Policy) UpstreamSettings { upstreamSettings.LoadBalancingMethod = string(*usp.Spec.LoadBalancingMethod) } - if usp.Spec.HashKey != nil { - upstreamSettings.HashKey = string(*usp.Spec.HashKey) + if usp.Spec.HashMethodKey != nil { + upstreamSettings.HashMethodKey = string(*usp.Spec.HashMethodKey) } } diff --git a/internal/controller/nginx/config/policies/upstreamsettings/processor_test.go b/internal/controller/nginx/config/policies/upstreamsettings/processor_test.go index 9c58b6aba4..8473e59f40 100644 --- a/internal/controller/nginx/config/policies/upstreamsettings/processor_test.go +++ b/internal/controller/nginx/config/policies/upstreamsettings/processor_test.go @@ -38,7 +38,7 @@ func TestProcess(t *testing.T) { Timeout: helpers.GetPointer[ngfAPIv1alpha1.Duration]("10s"), }), LoadBalancingMethod: helpers.GetPointer(ngfAPIv1alpha1.LoadBalancingTypeIPHash), - HashKey: helpers.GetPointer[ngfAPIv1alpha1.HashMethodKey]("$upstream_addr"), + HashMethodKey: helpers.GetPointer[ngfAPIv1alpha1.HashMethodKey]("$upstream_addr"), }, }, }, @@ -51,7 +51,7 @@ func TestProcess(t *testing.T) { Timeout: "10s", }, LoadBalancingMethod: string(ngfAPIv1alpha1.LoadBalancingTypeIPHash), - HashKey: "$upstream_addr", + HashMethodKey: "$upstream_addr", }, }, { @@ -81,13 +81,13 @@ func TestProcess(t *testing.T) { }, Spec: ngfAPIv1alpha1.UpstreamSettingsPolicySpec{ LoadBalancingMethod: helpers.GetPointer(ngfAPIv1alpha1.LoadBalancingTypeHashConsistent), - HashKey: helpers.GetPointer[ngfAPIv1alpha1.HashMethodKey]("$request_time"), + HashMethodKey: helpers.GetPointer[ngfAPIv1alpha1.HashMethodKey]("$request_time"), }, }, }, expUpstreamSettings: UpstreamSettings{ LoadBalancingMethod: string(ngfAPIv1alpha1.LoadBalancingTypeHashConsistent), - HashKey: "$request_time", + HashMethodKey: "$request_time", }, }, { @@ -267,7 +267,7 @@ func TestProcess(t *testing.T) { }, Spec: ngfAPIv1alpha1.UpstreamSettingsPolicySpec{ LoadBalancingMethod: helpers.GetPointer(ngfAPIv1alpha1.LoadBalancingTypeHashConsistent), - HashKey: helpers.GetPointer[ngfAPIv1alpha1.HashMethodKey]("$upstream_addr"), + HashMethodKey: helpers.GetPointer[ngfAPIv1alpha1.HashMethodKey]("$upstream_addr"), }, }, }, @@ -280,7 +280,7 @@ func TestProcess(t *testing.T) { Timeout: "10s", }, LoadBalancingMethod: string(ngfAPIv1alpha1.LoadBalancingTypeHashConsistent), - HashKey: "$upstream_addr", + HashMethodKey: "$upstream_addr", }, }, { @@ -369,7 +369,7 @@ func TestProcess(t *testing.T) { }, Spec: ngfAPIv1alpha1.UpstreamSettingsPolicySpec{ LoadBalancingMethod: helpers.GetPointer(ngfAPIv1alpha1.LoadBalancingTypeHash), - HashKey: helpers.GetPointer[ngfAPIv1alpha1.HashMethodKey]("$remote_addr"), + HashMethodKey: helpers.GetPointer[ngfAPIv1alpha1.HashMethodKey]("$remote_addr"), }, }, }, @@ -382,7 +382,7 @@ func TestProcess(t *testing.T) { Timeout: "10s", }, LoadBalancingMethod: string(ngfAPIv1alpha1.LoadBalancingTypeHash), - HashKey: "$remote_addr", + HashMethodKey: "$remote_addr", }, }, } diff --git a/internal/controller/nginx/config/policies/upstreamsettings/validator.go b/internal/controller/nginx/config/policies/upstreamsettings/validator.go index f1072232b5..2972ef766a 100644 --- a/internal/controller/nginx/config/policies/upstreamsettings/validator.go +++ b/internal/controller/nginx/config/policies/upstreamsettings/validator.go @@ -1,14 +1,10 @@ package upstreamsettings import ( - "fmt" - "strings" - "k8s.io/apimachinery/pkg/util/validation/field" gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" ngfAPI "github.com/nginx/nginx-gateway-fabric/v2/apis/v1alpha1" - httpConfig "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/nginx/config/http" "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/nginx/config/policies" "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/state/conditions" "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/state/validation" @@ -20,11 +16,15 @@ import ( // Implements policies.Validator interface. type Validator struct { genericValidator validation.GenericValidator + plusEnabled bool } // NewValidator returns a new Validator. -func NewValidator(genericValidator validation.GenericValidator) Validator { - return Validator{genericValidator: genericValidator} +func NewValidator(genericValidator validation.GenericValidator, plusEnabled bool) Validator { + return Validator{ + genericValidator: genericValidator, + plusEnabled: plusEnabled, + } } // Validate validates the spec of an UpstreamsSettingsPolicy. @@ -99,8 +99,8 @@ func checkConflictForLoadBalancingFields(a, b ngfAPI.UpstreamSettingsPolicySpec) return *a.LoadBalancingMethod == *b.LoadBalancingMethod } - if a.HashKey != nil && b.HashKey != nil { - return *a.HashKey == *b.HashKey + if a.HashMethodKey != nil && b.HashMethodKey != nil { + return *a.HashMethodKey == *b.HashMethodKey } return true @@ -123,6 +123,8 @@ func (v Validator) validateSettings(spec ngfAPI.UpstreamSettingsPolicySpec) erro allErrs = append(allErrs, v.validateUpstreamKeepAlive(*spec.KeepAlive, fieldPath.Child("keepAlive"))...) } + allErrs = append(allErrs, v.validateLoadBalancingMethod(spec, v.plusEnabled)...) + return allErrs.ToAggregate() } @@ -152,54 +154,30 @@ func (v Validator) validateUpstreamKeepAlive( } // ValidateLoadBalancingMethod validates the load balancing method for upstream servers. -func (v Validator) ValidateLoadBalancingMethod( - policy policies.Policy, +func (v Validator) validateLoadBalancingMethod( + spec ngfAPI.UpstreamSettingsPolicySpec, plusEnabled bool, -) []conditions.Condition { +) field.ErrorList { var allErrs field.ErrorList fieldPath := field.NewPath("spec") - usp := helpers.MustCastObject[*ngfAPI.UpstreamSettingsPolicy](policy) - if usp.Spec.LoadBalancingMethod == nil { + if spec.LoadBalancingMethod == nil { return nil } - lbMethod := *usp.Spec.LoadBalancingMethod - if !plusEnabled { - if _, ok := httpConfig.OSSAllowedLBMethods[lbMethod]; !ok { - allErrs = append(allErrs, field.Invalid( - fieldPath.Child("loadBalancingMethod"), - lbMethod, - fmt.Sprintf( - "NGINX OSS only supports the following load balancing methods: %s", - getLoadBalancingMethodList(httpConfig.OSSAllowedLBMethods), - ), - )) - } - } else { - if _, ok := httpConfig.PlusAllowedLBMethods[lbMethod]; !ok { - allErrs = append(allErrs, field.Invalid( - fieldPath.Child("loadBalancingMethod"), - lbMethod, - fmt.Sprintf( - "NGINX Plus only supports the following load balancing methods: %s", - getLoadBalancingMethodList(httpConfig.PlusAllowedLBMethods), - ), - )) - } + lbMethod := *spec.LoadBalancingMethod + if err := v.genericValidator.ValidateLoadBalancingMethod(string(lbMethod), plusEnabled); err != nil { + path := fieldPath.Child("loadBalancingMethod") + allErrs = append(allErrs, field.Invalid(path, lbMethod, err.Error())) } - if len(allErrs) > 0 { - return []conditions.Condition{conditions.NewPolicyInvalid(allErrs.ToAggregate().Error())} + if spec.HashMethodKey != nil { + hashMethodKey := *spec.HashMethodKey + if err := v.genericValidator.ValidateNginxVariableName(string(hashMethodKey)); err != nil { + path := fieldPath.Child("hashMethodKey") + allErrs = append(allErrs, field.Invalid(path, hashMethodKey, err.Error())) + } } - return nil -} - -func getLoadBalancingMethodList(lbMethods map[ngfAPI.LoadBalancingType]struct{}) string { - var methods []string - for method := range lbMethods { - methods = append(methods, string(method)) - } - return strings.Join(methods, ", ") + return allErrs } diff --git a/internal/controller/nginx/config/policies/upstreamsettings/validator_test.go b/internal/controller/nginx/config/policies/upstreamsettings/validator_test.go index f7a7fb38fb..0fed0d2f5e 100644 --- a/internal/controller/nginx/config/policies/upstreamsettings/validator_test.go +++ b/internal/controller/nginx/config/policies/upstreamsettings/validator_test.go @@ -16,6 +16,8 @@ import ( "github.com/nginx/nginx-gateway-fabric/v2/internal/framework/kinds" ) +const plusDisabled = false + type policyModFunc func(policy *ngfAPI.UpstreamSettingsPolicy) *ngfAPI.UpstreamSettingsPolicy func createValidPolicy() *ngfAPI.UpstreamSettingsPolicy { @@ -39,7 +41,7 @@ func createValidPolicy() *ngfAPI.UpstreamSettingsPolicy { Connections: helpers.GetPointer[int32](100), }, LoadBalancingMethod: helpers.GetPointer(ngfAPI.LoadBalancingTypeRandomTwoLeastConnection), - HashKey: helpers.GetPointer[ngfAPI.HashMethodKey]("$upstream_addr"), + HashMethodKey: helpers.GetPointer[ngfAPI.HashMethodKey]("$upstream_addr"), }, Status: v1.PolicyStatus{}, } @@ -126,7 +128,7 @@ func TestValidator_Validate(t *testing.T) { }, } - v := upstreamsettings.NewValidator(validation.GenericValidator{}) + v := upstreamsettings.NewValidator(validation.GenericValidator{}, plusDisabled) for _, test := range tests { t.Run(test.name, func(t *testing.T) { @@ -141,7 +143,7 @@ func TestValidator_Validate(t *testing.T) { func TestValidator_ValidatePanics(t *testing.T) { t.Parallel() - v := upstreamsettings.NewValidator(nil) + v := upstreamsettings.NewValidator(nil, plusDisabled) validate := func() { _ = v.Validate(&policiesfakes.FakePolicy{}) @@ -156,7 +158,7 @@ func TestValidator_ValidateGlobalSettings(t *testing.T) { t.Parallel() g := NewWithT(t) - v := upstreamsettings.NewValidator(validation.GenericValidator{}) + v := upstreamsettings.NewValidator(validation.GenericValidator{}, plusDisabled) g.Expect(v.ValidateGlobalSettings(nil, nil)).To(BeNil()) } @@ -265,14 +267,14 @@ func TestValidator_Conflicts(t *testing.T) { polB: &ngfAPI.UpstreamSettingsPolicy{ Spec: ngfAPI.UpstreamSettingsPolicySpec{ LoadBalancingMethod: helpers.GetPointer(ngfAPI.LoadBalancingTypeHashConsistent), - HashKey: helpers.GetPointer[ngfAPI.HashMethodKey]("$upstream_addr"), + HashMethodKey: helpers.GetPointer[ngfAPI.HashMethodKey]("$upstream_addr"), }, }, conflicts: true, }, } - v := upstreamsettings.NewValidator(nil) + v := upstreamsettings.NewValidator(nil, plusDisabled) for _, test := range tests { t.Run(test.name, func(t *testing.T) { @@ -286,7 +288,7 @@ func TestValidator_Conflicts(t *testing.T) { func TestValidator_ConflictsPanics(t *testing.T) { t.Parallel() - v := upstreamsettings.NewValidator(nil) + v := upstreamsettings.NewValidator(nil, plusDisabled) conflicts := func() { _ = v.Conflicts(&policiesfakes.FakePolicy{}, &policiesfakes.FakePolicy{}) @@ -358,29 +360,15 @@ func TestValidate_ValidateLoadBalancingMethod(t *testing.T) { "NGINX OSS only supports the following load balancing methods: "), }, }, - { - name: "invalid load balancing method for NGINX Plus", - policy: &ngfAPI.UpstreamSettingsPolicy{ - Spec: ngfAPI.UpstreamSettingsPolicySpec{ - LoadBalancingMethod: helpers.GetPointer(ngfAPI.LoadBalancingType("invalid-method")), - }, - }, - plusEnabled: true, - expConditions: []conditions.Condition{ - conditions.NewPolicyInvalid("spec.loadBalancingMethod: Invalid value: \"invalid-method\": " + - "NGINX Plus only supports the following load balancing methods: "), - }, - }, } - v := upstreamsettings.NewValidator(validation.GenericValidator{}) - for _, test := range tests { t.Run(test.name, func(t *testing.T) { t.Parallel() g := NewWithT(t) - conds := v.ValidateLoadBalancingMethod(test.policy, test.plusEnabled) + v := upstreamsettings.NewValidator(validation.GenericValidator{}, test.plusEnabled) + conds := v.Validate(test.policy) if test.expConditions != nil { g.Expect(conds).To(HaveLen(1)) @@ -389,16 +377,3 @@ func TestValidate_ValidateLoadBalancingMethod(t *testing.T) { }) } } - -func TestValidator_ValidateLoadBalancingPanics(t *testing.T) { - t.Parallel() - v := upstreamsettings.NewValidator(nil) - - validateLoadBalancingMethod := func() { - _ = v.ValidateLoadBalancingMethod(&policiesfakes.FakePolicy{}, true) - } - - g := NewWithT(t) - - g.Expect(validateLoadBalancingMethod).To(Panic()) -} diff --git a/internal/controller/nginx/config/policies/validator.go b/internal/controller/nginx/config/policies/validator.go index c83465d46b..5182c9781a 100644 --- a/internal/controller/nginx/config/policies/validator.go +++ b/internal/controller/nginx/config/policies/validator.go @@ -21,8 +21,6 @@ type Validator interface { ValidateGlobalSettings(policy Policy, globalSettings *GlobalSettings) []conditions.Condition // Conflicts returns true if the two Policies conflict. Conflicts(a, b Policy) bool - // ValidateLoadBalancingMethod validates the load balancing method for upstream servers. - ValidateLoadBalancingMethod(policy Policy, plusEnabled bool) []conditions.Condition } // CompositeValidator manages the validators for NGF Policies. @@ -95,14 +93,3 @@ func (m *CompositeValidator) Conflicts(polA, polB Policy) bool { return validator.Conflicts(polA, polB) } - -func (m *CompositeValidator) ValidateLoadBalancingMethod(policy Policy, plusEnabled bool) []conditions.Condition { - gvk := m.mustExtractGVK(policy) - - validator, ok := m.validators[gvk] - if !ok { - panic(fmt.Sprintf("no validator registered for policy %T", policy)) - } - - return validator.ValidateLoadBalancingMethod(policy, plusEnabled) -} diff --git a/internal/controller/nginx/config/upstreams.go b/internal/controller/nginx/config/upstreams.go index ac9f398497..7708ba1d97 100644 --- a/internal/controller/nginx/config/upstreams.go +++ b/internal/controller/nginx/config/upstreams.go @@ -168,10 +168,10 @@ func (g GeneratorImpl) createUpstream( lbMethod := upstreamPolicySettings.LoadBalancingMethod if lbMethod == string(ngfAPI.LoadBalancingTypeHash) { - lbMethod = fmt.Sprintf("hash %s", upstreamPolicySettings.HashKey) + lbMethod = fmt.Sprintf("hash %s", upstreamPolicySettings.HashMethodKey) } if lbMethod == string(ngfAPI.LoadBalancingTypeHashConsistent) { - lbMethod = fmt.Sprintf("hash %s consistent", upstreamPolicySettings.HashKey) + lbMethod = fmt.Sprintf("hash %s consistent", upstreamPolicySettings.HashMethodKey) } if lbMethod == string(ngfAPI.LoadBalancingTypeRoundRobin) { lbMethod = "" diff --git a/internal/controller/nginx/config/upstreams_test.go b/internal/controller/nginx/config/upstreams_test.go index 1888e927fd..f387fa4ed9 100644 --- a/internal/controller/nginx/config/upstreams_test.go +++ b/internal/controller/nginx/config/upstreams_test.go @@ -1210,7 +1210,7 @@ func TestExecuteUpstreams_LoadBalancingMethod(t *testing.T) { expectedSubStrings map[string]int name string lbType ngfAPI.LoadBalancingType - hashkey ngfAPI.HashMethodKey + HashMethodKey ngfAPI.HashMethodKey }{ { name: "default load balancing method", @@ -1246,9 +1246,9 @@ func TestExecuteUpstreams_LoadBalancingMethod(t *testing.T) { }, }, { - name: "hash load balancing method with specific hash key", - lbType: ngfAPI.LoadBalancingTypeHash, - hashkey: ngfAPI.HashMethodKey("$request_uri"), + name: "hash load balancing method with specific hash key", + lbType: ngfAPI.LoadBalancingTypeHash, + HashMethodKey: ngfAPI.HashMethodKey("$request_uri"), expectedSubStrings: map[string]int{ "upstream up1-usp-ipv4": 1, "upstream up2-usp-ipv6": 1, @@ -1256,9 +1256,9 @@ func TestExecuteUpstreams_LoadBalancingMethod(t *testing.T) { }, }, { - name: "hash consistent load balancing method with specific hash key", - lbType: ngfAPI.LoadBalancingTypeHashConsistent, - hashkey: ngfAPI.HashMethodKey("$remote_addr"), + name: "hash consistent load balancing method with specific hash key", + lbType: ngfAPI.LoadBalancingTypeHashConsistent, + HashMethodKey: ngfAPI.HashMethodKey("$remote_addr"), expectedSubStrings: map[string]int{ "upstream up1-usp-ipv4": 1, "upstream up2-usp-ipv6": 1, @@ -1361,7 +1361,7 @@ func TestExecuteUpstreams_LoadBalancingMethod(t *testing.T) { }, Spec: ngfAPI.UpstreamSettingsPolicySpec{ LoadBalancingMethod: helpers.GetPointer(tt.lbType), - HashKey: helpers.GetPointer(tt.hashkey), + HashMethodKey: helpers.GetPointer(tt.HashMethodKey), }, }, }, @@ -1382,7 +1382,7 @@ func TestExecuteUpstreams_LoadBalancingMethod(t *testing.T) { }, Spec: ngfAPI.UpstreamSettingsPolicySpec{ LoadBalancingMethod: helpers.GetPointer(tt.lbType), - HashKey: helpers.GetPointer(tt.hashkey), + HashMethodKey: helpers.GetPointer(tt.HashMethodKey), }, }, }, diff --git a/internal/controller/nginx/config/validation/generic.go b/internal/controller/nginx/config/validation/generic.go index f63073955b..ddbe5c9467 100644 --- a/internal/controller/nginx/config/validation/generic.go +++ b/internal/controller/nginx/config/validation/generic.go @@ -2,9 +2,13 @@ package validation import ( "errors" + "fmt" "regexp" + "strings" k8svalidation "k8s.io/apimachinery/pkg/util/validation" + + ngfAPI "github.com/nginx/nginx-gateway-fabric/v2/apis/v1alpha1" ) // GenericValidator validates values for generic cases in the nginx conf. @@ -127,3 +131,54 @@ func (GenericValidator) ValidateNginxVariableName(name string) error { return nil } + +var ( + PlusAllowedLBMethods = map[ngfAPI.LoadBalancingType]struct{}{ + ngfAPI.LoadBalancingTypeRoundRobin: {}, + ngfAPI.LoadBalancingTypeLeastConnection: {}, + ngfAPI.LoadBalancingTypeIPHash: {}, + ngfAPI.LoadBalancingTypeRandom: {}, + ngfAPI.LoadBalancingTypeHash: {}, + ngfAPI.LoadBalancingTypeHashConsistent: {}, + ngfAPI.LoadBalancingTypeRandomTwo: {}, + ngfAPI.LoadBalancingTypeRandomTwoLeastConnection: {}, + ngfAPI.LoadBalancingTypeLeastTimeHeader: {}, + ngfAPI.LoadBalancingTypeLeastTimeLastByte: {}, + ngfAPI.LoadBalancingTypeLeastTimeHeaderInflight: {}, + ngfAPI.LoadBalancingTypeLeastTimeLastByteInflight: {}, + ngfAPI.LoadBalancingTypeRandomTwoLeastTimeHeader: {}, + ngfAPI.LoadBalancingTypeRandomTwoLeastTimeLastByte: {}, + } + + OSSAllowedLBMethods = map[ngfAPI.LoadBalancingType]struct{}{ + ngfAPI.LoadBalancingTypeRoundRobin: {}, + ngfAPI.LoadBalancingTypeLeastConnection: {}, + ngfAPI.LoadBalancingTypeIPHash: {}, + ngfAPI.LoadBalancingTypeRandom: {}, + ngfAPI.LoadBalancingTypeHash: {}, + ngfAPI.LoadBalancingTypeHashConsistent: {}, + ngfAPI.LoadBalancingTypeRandomTwo: {}, + ngfAPI.LoadBalancingTypeRandomTwoLeastConnection: {}, + } +) + +func (GenericValidator) ValidateLoadBalancingMethod(method string, plusEnabled bool) error { + lbMethod := ngfAPI.LoadBalancingType(method) + + if !plusEnabled { + if _, ok := OSSAllowedLBMethods[lbMethod]; ok { + return nil + } + } + + return fmt.Errorf("NGINX OSS only supports the following load balancing methods: %s", + getLoadBalancingMethodList(OSSAllowedLBMethods)) +} + +func getLoadBalancingMethodList(lbMethods map[ngfAPI.LoadBalancingType]struct{}) string { + var methods []string + for method := range lbMethods { + methods = append(methods, string(method)) + } + return strings.Join(methods, ", ") +} diff --git a/internal/controller/nginx/config/validation/generic_test.go b/internal/controller/nginx/config/validation/generic_test.go index 73be3f10cb..428511aa49 100644 --- a/internal/controller/nginx/config/validation/generic_test.go +++ b/internal/controller/nginx/config/validation/generic_test.go @@ -134,3 +134,34 @@ func TestValidateNginxVariableName(t *testing.T) { `var$name`, ) } + +func makeValidator(plusEnabled bool) simpleValidatorFunc[string] { + return func(v string) error { + return (GenericValidator{}).ValidateLoadBalancingMethod(v, plusEnabled) + } +} + +func TestValidateLoadBalancingMethod_OSS(t *testing.T) { + t.Helper() + + ossValidator := makeValidator(false) + testValidValuesForSimpleValidator(t, ossValidator, + "round_robin", + "least_conn", + "ip_hash", + "hash", + "hash consistent", + "random", + "random two", + "random two least_conn", + ) + + testInvalidValuesForSimpleValidator(t, ossValidator, + "random two least_time=header", + "random two least_time=last_byte", + "least_time header", + "least_time last_byte", + "least_time header inflight", + "least_time last_byte inflight", + ) +} diff --git a/internal/controller/state/change_processor.go b/internal/controller/state/change_processor.go index 87e29d5ad7..d661903b8c 100644 --- a/internal/controller/state/change_processor.go +++ b/internal/controller/state/change_processor.go @@ -64,8 +64,8 @@ type ChangeProcessorConfig struct { GatewayCtlrName string // GatewayClassName is the name of the GatewayClass resource. GatewayClassName string - // Flags hold the feature flags - Flags graph.Flags + // ExperimentalFeatures indicates if experimental features are enabled. + ExperimentalFeatures bool } // ChangeProcessorImpl is an implementation of ChangeProcessor. @@ -278,7 +278,7 @@ func (c *ChangeProcessorImpl) Process() *graph.Graph { c.cfg.PlusSecrets, c.cfg.Validators, c.cfg.Logger, - c.cfg.Flags, + c.cfg.ExperimentalFeatures, ) return c.latestGraph diff --git a/internal/controller/state/graph/graph.go b/internal/controller/state/graph/graph.go index e90b96a4c8..ffd86ac6e0 100644 --- a/internal/controller/state/graph/graph.go +++ b/internal/controller/state/graph/graph.go @@ -92,14 +92,6 @@ type NginxReloadResult struct { // ProtectedPorts are the ports that may not be configured by a listener with a descriptive name of each port. type ProtectedPorts map[int32]string -// Flags hold the configuration flags for building the Graph. -type Flags struct { - // Plus indicates whether NGINX Plus features are enabled. - Plus bool - // Experimental indicates whether experimental features are enabled. - Experimental bool -} - // IsReferenced returns true if the Graph references the resource. func (g *Graph) IsReferenced(resourceType ngftypes.ObjectType, nsname types.NamespacedName) bool { switch obj := resourceType.(type) { @@ -216,7 +208,7 @@ func BuildGraph( plusSecrets map[types.NamespacedName][]PlusSecretFile, validators validation.Validators, logger logr.Logger, - flags Flags, + experimentalEnabled bool, ) *Graph { processedGwClasses, gcExists := processGatewayClasses(state.GatewayClasses, gcName, controllerName) if gcExists && processedGwClasses.Winner == nil { @@ -236,7 +228,7 @@ func BuildGraph( processedGwClasses.Winner, processedNginxProxies, state.CRDMetadata, - flags.Experimental, + experimentalEnabled, ) secretResolver := newSecretResolver(state.Secrets) @@ -250,7 +242,7 @@ func BuildGraph( gc, refGrantResolver, processedNginxProxies, - flags.Experimental, + experimentalEnabled, ) processedBackendTLSPolicies := processBackendTLSPolicies( @@ -327,7 +319,7 @@ func BuildGraph( PlusSecrets: plusSecrets, } - g.attachPolicies(validators.PolicyValidator, controllerName, logger, flags.Plus) + g.attachPolicies(validators.PolicyValidator, controllerName, logger) return g } diff --git a/internal/controller/state/graph/graph_test.go b/internal/controller/state/graph/graph_test.go index 676d18144b..ed0e39082f 100644 --- a/internal/controller/state/graph/graph_test.go +++ b/internal/controller/state/graph/graph_test.go @@ -1493,9 +1493,7 @@ func TestBuildGraph(t *testing.T) { PolicyValidator: fakePolicyValidator, }, logr.Discard(), - Flags{ - Experimental: test.experimentalEnabled, - }, + test.experimentalEnabled, ) g.Expect(helpers.Diff(test.expected, result)).To(BeEmpty()) diff --git a/internal/controller/state/graph/multiple_gateways_test.go b/internal/controller/state/graph/multiple_gateways_test.go index dc5b25dd59..c20fd98516 100644 --- a/internal/controller/state/graph/multiple_gateways_test.go +++ b/internal/controller/state/graph/multiple_gateways_test.go @@ -409,9 +409,7 @@ func Test_MultipleGateways_WithNginxProxy(t *testing.T) { PolicyValidator: fakePolicyValidator, }, logr.Discard(), - Flags{ - Experimental: experimentalFeaturesEnabled, - }, + experimentalFeaturesEnabled, ) g.Expect(helpers.Diff(test.expGraph, result)).To(BeEmpty()) @@ -901,9 +899,7 @@ func Test_MultipleGateways_WithListeners(t *testing.T) { PolicyValidator: fakePolicyValidator, }, logr.Discard(), - Flags{ - Experimental: experimentalFeaturesEnabled, - }, + experimentalFeaturesEnabled, ) g.Expect(helpers.Diff(test.expGraph, result)).To(BeEmpty()) diff --git a/internal/controller/state/graph/policies.go b/internal/controller/state/graph/policies.go index e45bb608b3..30e44fb014 100644 --- a/internal/controller/state/graph/policies.go +++ b/internal/controller/state/graph/policies.go @@ -116,12 +116,7 @@ func collectOrderedGatewaysForService( return append(existingGateways, newGateways...) } -func (g *Graph) attachPolicies( - validator validation.PolicyValidator, - ctlrName string, - logger logr.Logger, - plusEnabled bool, -) { +func (g *Graph) attachPolicies(validator validation.PolicyValidator, ctlrName string, logger logr.Logger) { if len(g.Gateways) == 0 { return } @@ -144,7 +139,7 @@ func (g *Graph) attachPolicies( continue } - attachPolicyToService(policy, svc, g.Gateways, ctlrName, logger, plusEnabled, validator) + attachPolicyToService(policy, svc, g.Gateways, ctlrName, logger) } } } @@ -156,8 +151,6 @@ func attachPolicyToService( gws map[types.NamespacedName]*Gateway, ctlrName string, logger logr.Logger, - plusEnabled bool, - validator validation.PolicyValidator, ) { var attachedToAnyGateway bool @@ -222,10 +215,6 @@ func attachPolicyToService( attachedToAnyGateway = true } - if conds := validator.ValidateLoadBalancingMethod(policy.Source, plusEnabled); len(conds) > 0 { - policy.Conditions = append(policy.Conditions, conds...) - } - // Attach policy to service if effective for at least one gateway if attachedToAnyGateway { svc.Policies = append(svc.Policies, policy) diff --git a/internal/controller/state/graph/policies_test.go b/internal/controller/state/graph/policies_test.go index 02bb745caa..e4532f5a54 100644 --- a/internal/controller/state/graph/policies_test.go +++ b/internal/controller/state/graph/policies_test.go @@ -242,7 +242,7 @@ func TestAttachPolicies(t *testing.T) { NGFPolicies: test.ngfPolicies, } - graph.attachPolicies(&policiesfakes.FakeValidator{}, "nginx-gateway", logr.Discard(), false) + graph.attachPolicies(&policiesfakes.FakeValidator{}, "nginx-gateway", logr.Discard()) for _, expect := range test.expects { expect(g, graph) } @@ -681,23 +681,12 @@ func TestAttachPolicyToService(t *testing.T) { } } - validatorError := &policiesfakes.FakeValidator{ - ValidateLoadBalancingMethodStub: func(_ policies.Policy, plusEnabled bool) []conditions.Condition { - if !plusEnabled { - return []conditions.Condition{conditions.NewPolicyInvalid("invalid load balancing method: NGINX Plus is required")} - } - return nil - }, - } - tests := []struct { policy *Policy svc *ReferencedService gws map[types.NamespacedName]*Gateway name string - validator policies.Validator expAncestors []PolicyAncestor - plus bool expAttached bool }{ { @@ -715,7 +704,6 @@ func TestAttachPolicyToService(t *testing.T) { Ancestor: getGatewayParentRef(gwNsname), }, }, - validator: &policiesfakes.FakeValidator{}, }, { name: "attachment; ancestor already exists so don't duplicate", @@ -740,7 +728,6 @@ func TestAttachPolicyToService(t *testing.T) { Ancestor: getGatewayParentRef(gwNsname), // only one ancestor per Gateway }, }, - validator: &policiesfakes.FakeValidator{}, }, { name: "attachment; existing gateway from policy status processed first", @@ -781,7 +768,6 @@ func TestAttachPolicyToService(t *testing.T) { Ancestor: getGatewayParentRef(gw2Nsname), // Only new gateway gets added }, }, - validator: &policiesfakes.FakeValidator{}, }, { name: "attachment; ancestor doesn't exist so add it", @@ -810,7 +796,6 @@ func TestAttachPolicyToService(t *testing.T) { Ancestor: getGatewayParentRef(gwNsname), }, }, - validator: &policiesfakes.FakeValidator{}, }, { name: "no attachment; gateway is invalid", @@ -828,7 +813,6 @@ func TestAttachPolicyToService(t *testing.T) { Conditions: []conditions.Condition{conditions.NewPolicyTargetNotFound("The Parent Gateway is invalid")}, }, }, - validator: &policiesfakes.FakeValidator{}, }, { name: "no attachment; max ancestor", @@ -841,7 +825,6 @@ func TestAttachPolicyToService(t *testing.T) { gws: getGateway(true /*valid*/), expAttached: false, expAncestors: nil, - validator: &policiesfakes.FakeValidator{}, }, { name: "no attachment; does not belong to gateway", @@ -854,7 +837,6 @@ func TestAttachPolicyToService(t *testing.T) { gws: getGateway(true /*valid*/), expAttached: false, expAncestors: nil, - validator: &policiesfakes.FakeValidator{}, }, { name: "no attachment; gateway is invalid", @@ -881,24 +863,6 @@ func TestAttachPolicyToService(t *testing.T) { Ancestor: getGatewayParentRef(gwNsname), }, }, - validator: &policiesfakes.FakeValidator{}, - }, - { - name: "no attachment: invalid load balancing setting for OSS", - policy: &Policy{Source: &policiesfakes.FakePolicy{}, InvalidForGateways: map[types.NamespacedName]struct{}{}}, - svc: &ReferencedService{ - GatewayNsNames: map[types.NamespacedName]struct{}{ - gwNsname: {}, - }, - }, - gws: getGateway(true /*valid*/), - expAttached: true, - expAncestors: []PolicyAncestor{ - { - Ancestor: getGatewayParentRef(gwNsname), - }, - }, - validator: validatorError, }, } @@ -907,7 +871,7 @@ func TestAttachPolicyToService(t *testing.T) { t.Parallel() g := NewWithT(t) - attachPolicyToService(test.policy, test.svc, test.gws, "ctlr", logr.Discard(), test.plus, test.validator) + attachPolicyToService(test.policy, test.svc, test.gws, "ctlr", logr.Discard()) if test.expAttached { g.Expect(test.svc.Policies).To(HaveLen(1)) } else { @@ -2308,7 +2272,7 @@ func TestNGFPolicyAncestorLimitHandling(t *testing.T) { } // Call attachPolicies to trigger the ancestor limit logic - graph.attachPolicies(validator, "nginx-gateway", testLogger, false) + graph.attachPolicies(validator, "nginx-gateway", testLogger) // Verify that the policy with full ancestors has no actual ancestors assigned policyFullKey := PolicyKey{ diff --git a/internal/controller/state/validation/validationfakes/fake_generic_validator.go b/internal/controller/state/validation/validationfakes/fake_generic_validator.go index cd162c7359..eed9deda3e 100644 --- a/internal/controller/state/validation/validationfakes/fake_generic_validator.go +++ b/internal/controller/state/validation/validationfakes/fake_generic_validator.go @@ -30,6 +30,18 @@ type FakeGenericValidator struct { validateEscapedStringNoVarExpansionReturnsOnCall map[int]struct { result1 error } + ValidateLoadBalancingMethodStub func(string, bool) error + validateLoadBalancingMethodMutex sync.RWMutex + validateLoadBalancingMethodArgsForCall []struct { + arg1 string + arg2 bool + } + validateLoadBalancingMethodReturns struct { + result1 error + } + validateLoadBalancingMethodReturnsOnCall map[int]struct { + result1 error + } ValidateNginxDurationStub func(string) error validateNginxDurationMutex sync.RWMutex validateNginxDurationArgsForCall []struct { @@ -200,6 +212,68 @@ func (fake *FakeGenericValidator) ValidateEscapedStringNoVarExpansionReturnsOnCa }{result1} } +func (fake *FakeGenericValidator) ValidateLoadBalancingMethod(arg1 string, arg2 bool) error { + fake.validateLoadBalancingMethodMutex.Lock() + ret, specificReturn := fake.validateLoadBalancingMethodReturnsOnCall[len(fake.validateLoadBalancingMethodArgsForCall)] + fake.validateLoadBalancingMethodArgsForCall = append(fake.validateLoadBalancingMethodArgsForCall, struct { + arg1 string + arg2 bool + }{arg1, arg2}) + stub := fake.ValidateLoadBalancingMethodStub + fakeReturns := fake.validateLoadBalancingMethodReturns + fake.recordInvocation("ValidateLoadBalancingMethod", []interface{}{arg1, arg2}) + fake.validateLoadBalancingMethodMutex.Unlock() + if stub != nil { + return stub(arg1, arg2) + } + if specificReturn { + return ret.result1 + } + return fakeReturns.result1 +} + +func (fake *FakeGenericValidator) ValidateLoadBalancingMethodCallCount() int { + fake.validateLoadBalancingMethodMutex.RLock() + defer fake.validateLoadBalancingMethodMutex.RUnlock() + return len(fake.validateLoadBalancingMethodArgsForCall) +} + +func (fake *FakeGenericValidator) ValidateLoadBalancingMethodCalls(stub func(string, bool) error) { + fake.validateLoadBalancingMethodMutex.Lock() + defer fake.validateLoadBalancingMethodMutex.Unlock() + fake.ValidateLoadBalancingMethodStub = stub +} + +func (fake *FakeGenericValidator) ValidateLoadBalancingMethodArgsForCall(i int) (string, bool) { + fake.validateLoadBalancingMethodMutex.RLock() + defer fake.validateLoadBalancingMethodMutex.RUnlock() + argsForCall := fake.validateLoadBalancingMethodArgsForCall[i] + return argsForCall.arg1, argsForCall.arg2 +} + +func (fake *FakeGenericValidator) ValidateLoadBalancingMethodReturns(result1 error) { + fake.validateLoadBalancingMethodMutex.Lock() + defer fake.validateLoadBalancingMethodMutex.Unlock() + fake.ValidateLoadBalancingMethodStub = nil + fake.validateLoadBalancingMethodReturns = struct { + result1 error + }{result1} +} + +func (fake *FakeGenericValidator) ValidateLoadBalancingMethodReturnsOnCall(i int, result1 error) { + fake.validateLoadBalancingMethodMutex.Lock() + defer fake.validateLoadBalancingMethodMutex.Unlock() + fake.ValidateLoadBalancingMethodStub = nil + if fake.validateLoadBalancingMethodReturnsOnCall == nil { + fake.validateLoadBalancingMethodReturnsOnCall = make(map[int]struct { + result1 error + }) + } + fake.validateLoadBalancingMethodReturnsOnCall[i] = struct { + result1 error + }{result1} +} + func (fake *FakeGenericValidator) ValidateNginxDuration(arg1 string) error { fake.validateNginxDurationMutex.Lock() ret, specificReturn := fake.validateNginxDurationReturnsOnCall[len(fake.validateNginxDurationArgsForCall)] diff --git a/internal/controller/state/validation/validationfakes/fake_policy_validator.go b/internal/controller/state/validation/validationfakes/fake_policy_validator.go index 789c42f383..0cb8b7f232 100644 --- a/internal/controller/state/validation/validationfakes/fake_policy_validator.go +++ b/internal/controller/state/validation/validationfakes/fake_policy_validator.go @@ -45,18 +45,6 @@ type FakePolicyValidator struct { validateGlobalSettingsReturnsOnCall map[int]struct { result1 []conditions.Condition } - ValidateLoadBalancingMethodStub func(policies.Policy, bool) []conditions.Condition - validateLoadBalancingMethodMutex sync.RWMutex - validateLoadBalancingMethodArgsForCall []struct { - arg1 policies.Policy - arg2 bool - } - validateLoadBalancingMethodReturns struct { - result1 []conditions.Condition - } - validateLoadBalancingMethodReturnsOnCall map[int]struct { - result1 []conditions.Condition - } invocations map[string][][]interface{} invocationsMutex sync.RWMutex } @@ -246,68 +234,6 @@ func (fake *FakePolicyValidator) ValidateGlobalSettingsReturnsOnCall(i int, resu }{result1} } -func (fake *FakePolicyValidator) ValidateLoadBalancingMethod(arg1 policies.Policy, arg2 bool) []conditions.Condition { - fake.validateLoadBalancingMethodMutex.Lock() - ret, specificReturn := fake.validateLoadBalancingMethodReturnsOnCall[len(fake.validateLoadBalancingMethodArgsForCall)] - fake.validateLoadBalancingMethodArgsForCall = append(fake.validateLoadBalancingMethodArgsForCall, struct { - arg1 policies.Policy - arg2 bool - }{arg1, arg2}) - stub := fake.ValidateLoadBalancingMethodStub - fakeReturns := fake.validateLoadBalancingMethodReturns - fake.recordInvocation("ValidateLoadBalancingMethod", []interface{}{arg1, arg2}) - fake.validateLoadBalancingMethodMutex.Unlock() - if stub != nil { - return stub(arg1, arg2) - } - if specificReturn { - return ret.result1 - } - return fakeReturns.result1 -} - -func (fake *FakePolicyValidator) ValidateLoadBalancingMethodCallCount() int { - fake.validateLoadBalancingMethodMutex.RLock() - defer fake.validateLoadBalancingMethodMutex.RUnlock() - return len(fake.validateLoadBalancingMethodArgsForCall) -} - -func (fake *FakePolicyValidator) ValidateLoadBalancingMethodCalls(stub func(policies.Policy, bool) []conditions.Condition) { - fake.validateLoadBalancingMethodMutex.Lock() - defer fake.validateLoadBalancingMethodMutex.Unlock() - fake.ValidateLoadBalancingMethodStub = stub -} - -func (fake *FakePolicyValidator) ValidateLoadBalancingMethodArgsForCall(i int) (policies.Policy, bool) { - fake.validateLoadBalancingMethodMutex.RLock() - defer fake.validateLoadBalancingMethodMutex.RUnlock() - argsForCall := fake.validateLoadBalancingMethodArgsForCall[i] - return argsForCall.arg1, argsForCall.arg2 -} - -func (fake *FakePolicyValidator) ValidateLoadBalancingMethodReturns(result1 []conditions.Condition) { - fake.validateLoadBalancingMethodMutex.Lock() - defer fake.validateLoadBalancingMethodMutex.Unlock() - fake.ValidateLoadBalancingMethodStub = nil - fake.validateLoadBalancingMethodReturns = struct { - result1 []conditions.Condition - }{result1} -} - -func (fake *FakePolicyValidator) ValidateLoadBalancingMethodReturnsOnCall(i int, result1 []conditions.Condition) { - fake.validateLoadBalancingMethodMutex.Lock() - defer fake.validateLoadBalancingMethodMutex.Unlock() - fake.ValidateLoadBalancingMethodStub = nil - if fake.validateLoadBalancingMethodReturnsOnCall == nil { - fake.validateLoadBalancingMethodReturnsOnCall = make(map[int]struct { - result1 []conditions.Condition - }) - } - fake.validateLoadBalancingMethodReturnsOnCall[i] = struct { - result1 []conditions.Condition - }{result1} -} - func (fake *FakePolicyValidator) Invocations() map[string][][]interface{} { fake.invocationsMutex.RLock() defer fake.invocationsMutex.RUnlock() diff --git a/internal/controller/state/validation/validator.go b/internal/controller/state/validation/validator.go index cf0138aa15..f2f01d9654 100644 --- a/internal/controller/state/validation/validator.go +++ b/internal/controller/state/validation/validator.go @@ -50,6 +50,7 @@ type GenericValidator interface { ValidateNginxSize(size string) error ValidateEndpoint(endpoint string) error ValidateNginxVariableName(name string) error + ValidateLoadBalancingMethod(method string, plusEnabled bool) error } // PolicyValidator validates an NGF Policy. @@ -62,8 +63,6 @@ type PolicyValidator interface { ValidateGlobalSettings(policy policies.Policy, globalSettings *policies.GlobalSettings) []conditions.Condition // Conflicts returns true if the two Policies conflict. Conflicts(a, b policies.Policy) bool - // ValidateLoadBalancingMethod validates the load balancing method for upstream servers. - ValidateLoadBalancingMethod(policy policies.Policy, plusEnabled bool) []conditions.Condition } // SkipValidator is used to skip validation on internally-created routes for request mirroring. diff --git a/tests/cel/common.go b/tests/cel/common.go index 2a0c3ddb7c..067888a2d3 100644 --- a/tests/cel/common.go +++ b/tests/cel/common.go @@ -59,7 +59,7 @@ const ( expectedTargetRefKindServiceError = `TargetRefs Kind must be: Service` expectedTargetRefGroupCoreError = `TargetRefs Group must be core` expectedTargetRefNameUniqueError = `TargetRef Name must be unique` - expectedHashKeyLoadBalancingTypeError = `hashKey is required when loadBalancingMethod ` + + expectedHashKeyLoadBalancingTypeError = `hashMethodKey is required when loadBalancingMethod ` + `is 'hash' or 'hash consistent'` ) diff --git a/tests/cel/upstreamsettingspolicy_test.go b/tests/cel/upstreamsettingspolicy_test.go index 3a82af32be..5b0267350f 100644 --- a/tests/cel/upstreamsettingspolicy_test.go +++ b/tests/cel/upstreamsettingspolicy_test.go @@ -419,7 +419,7 @@ func TestUpstreamSettingsPolicy_LoadBalancing(t *testing.T) { }, }, LoadBalancingMethod: helpers.GetPointer(ngfAPIv1alpha1.LoadBalancingTypeHash), - HashKey: helpers.GetPointer(ngfAPIv1alpha1.HashMethodKey("$upstream_connect_time")), + HashMethodKey: helpers.GetPointer(ngfAPIv1alpha1.HashMethodKey("$upstream_connect_time")), }, }, { @@ -432,7 +432,7 @@ func TestUpstreamSettingsPolicy_LoadBalancing(t *testing.T) { }, }, LoadBalancingMethod: helpers.GetPointer(ngfAPIv1alpha1.LoadBalancingTypeHashConsistent), - HashKey: helpers.GetPointer(ngfAPIv1alpha1.HashMethodKey("$upstream_bytes_sent")), + HashMethodKey: helpers.GetPointer(ngfAPIv1alpha1.HashMethodKey("$upstream_bytes_sent")), }, }, } From c40f1f4c957d8b23e030ad5709c975853765984c Mon Sep 17 00:00:00 2001 From: salonichf5 <146118978+salonichf5@users.noreply.github.com> Date: Fri, 21 Nov 2025 14:41:04 -0700 Subject: [PATCH 4/6] remove lb check from generic validator --- internal/controller/manager.go | 4 +- .../controller/nginx/config/http/config.go | 12 +++ .../policies/upstreamsettings/validator.go | 35 ++++++--- .../nginx/config/validation/generic.go | 55 -------------- .../nginx/config/validation/generic_test.go | 31 -------- .../validationfakes/fake_generic_validator.go | 74 ------------------- .../controller/state/validation/validator.go | 1 - 7 files changed, 40 insertions(+), 172 deletions(-) diff --git a/internal/controller/manager.go b/internal/controller/manager.go index fa5abd069e..fe3c387d2e 100644 --- a/internal/controller/manager.go +++ b/internal/controller/manager.go @@ -124,7 +124,7 @@ func StartManager(cfg config.Config) error { mustExtractGVK := kinds.NewMustExtractGKV(scheme) genericValidator := ngxvalidation.GenericValidator{} - policyManager := createPolicyManager(mustExtractGVK, cfg.Plus, genericValidator) + policyManager := createPolicyManager(mustExtractGVK, genericValidator, cfg.Plus) plusSecrets, err := createPlusSecretMetadata(cfg, mgr.GetAPIReader()) if err != nil { @@ -322,8 +322,8 @@ func StartManager(cfg config.Config) error { func createPolicyManager( mustExtractGVK kinds.MustExtractGVK, - plusEnabled bool, validator validation.GenericValidator, + plusEnabled bool, ) *policies.CompositeValidator { cfgs := []policies.ManagerConfig{ { diff --git a/internal/controller/nginx/config/http/config.go b/internal/controller/nginx/config/http/config.go index c3dfbf23be..292b4d770f 100644 --- a/internal/controller/nginx/config/http/config.go +++ b/internal/controller/nginx/config/http/config.go @@ -1,6 +1,7 @@ package http import ( + ngfAPI "github.com/nginx/nginx-gateway-fabric/v2/apis/v1alpha1" "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/nginx/config/shared" ) @@ -168,3 +169,14 @@ type ServerConfig struct { Plus bool DisableSNIHostValidation bool } + +var OSSAllowedLBMethods = map[ngfAPI.LoadBalancingType]struct{}{ + ngfAPI.LoadBalancingTypeRoundRobin: {}, + ngfAPI.LoadBalancingTypeLeastConnection: {}, + ngfAPI.LoadBalancingTypeIPHash: {}, + ngfAPI.LoadBalancingTypeRandom: {}, + ngfAPI.LoadBalancingTypeHash: {}, + ngfAPI.LoadBalancingTypeHashConsistent: {}, + ngfAPI.LoadBalancingTypeRandomTwo: {}, + ngfAPI.LoadBalancingTypeRandomTwoLeastConnection: {}, +} diff --git a/internal/controller/nginx/config/policies/upstreamsettings/validator.go b/internal/controller/nginx/config/policies/upstreamsettings/validator.go index 2972ef766a..f46420d4db 100644 --- a/internal/controller/nginx/config/policies/upstreamsettings/validator.go +++ b/internal/controller/nginx/config/policies/upstreamsettings/validator.go @@ -1,10 +1,14 @@ package upstreamsettings import ( + "fmt" + "strings" + "k8s.io/apimachinery/pkg/util/validation/field" gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" ngfAPI "github.com/nginx/nginx-gateway-fabric/v2/apis/v1alpha1" + httpConfig "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/nginx/config/http" "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/nginx/config/policies" "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/state/conditions" "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/state/validation" @@ -123,7 +127,7 @@ func (v Validator) validateSettings(spec ngfAPI.UpstreamSettingsPolicySpec) erro allErrs = append(allErrs, v.validateUpstreamKeepAlive(*spec.KeepAlive, fieldPath.Child("keepAlive"))...) } - allErrs = append(allErrs, v.validateLoadBalancingMethod(spec, v.plusEnabled)...) + allErrs = append(allErrs, v.validateLoadBalancingMethod(spec)...) return allErrs.ToAggregate() } @@ -154,10 +158,7 @@ func (v Validator) validateUpstreamKeepAlive( } // ValidateLoadBalancingMethod validates the load balancing method for upstream servers. -func (v Validator) validateLoadBalancingMethod( - spec ngfAPI.UpstreamSettingsPolicySpec, - plusEnabled bool, -) field.ErrorList { +func (v Validator) validateLoadBalancingMethod(spec ngfAPI.UpstreamSettingsPolicySpec) field.ErrorList { var allErrs field.ErrorList fieldPath := field.NewPath("spec") @@ -165,10 +166,18 @@ func (v Validator) validateLoadBalancingMethod( return nil } - lbMethod := *spec.LoadBalancingMethod - if err := v.genericValidator.ValidateLoadBalancingMethod(string(lbMethod), plusEnabled); err != nil { - path := fieldPath.Child("loadBalancingMethod") - allErrs = append(allErrs, field.Invalid(path, lbMethod, err.Error())) + if !v.plusEnabled { + if _, ok := httpConfig.OSSAllowedLBMethods[*spec.LoadBalancingMethod]; !ok { + path := fieldPath.Child("loadBalancingMethod") + allErrs = append(allErrs, field.Invalid( + path, + *spec.LoadBalancingMethod, + fmt.Sprintf( + "NGINX OSS only supports the following load balancing methods: %s", + getLoadBalancingMethodList(httpConfig.OSSAllowedLBMethods), + ), + )) + } } if spec.HashMethodKey != nil { @@ -181,3 +190,11 @@ func (v Validator) validateLoadBalancingMethod( return allErrs } + +func getLoadBalancingMethodList(lbMethods map[ngfAPI.LoadBalancingType]struct{}) string { + methods := make([]string, 0, len(lbMethods)) + for method := range lbMethods { + methods = append(methods, string(method)) + } + return strings.Join(methods, ", ") +} diff --git a/internal/controller/nginx/config/validation/generic.go b/internal/controller/nginx/config/validation/generic.go index ddbe5c9467..f63073955b 100644 --- a/internal/controller/nginx/config/validation/generic.go +++ b/internal/controller/nginx/config/validation/generic.go @@ -2,13 +2,9 @@ package validation import ( "errors" - "fmt" "regexp" - "strings" k8svalidation "k8s.io/apimachinery/pkg/util/validation" - - ngfAPI "github.com/nginx/nginx-gateway-fabric/v2/apis/v1alpha1" ) // GenericValidator validates values for generic cases in the nginx conf. @@ -131,54 +127,3 @@ func (GenericValidator) ValidateNginxVariableName(name string) error { return nil } - -var ( - PlusAllowedLBMethods = map[ngfAPI.LoadBalancingType]struct{}{ - ngfAPI.LoadBalancingTypeRoundRobin: {}, - ngfAPI.LoadBalancingTypeLeastConnection: {}, - ngfAPI.LoadBalancingTypeIPHash: {}, - ngfAPI.LoadBalancingTypeRandom: {}, - ngfAPI.LoadBalancingTypeHash: {}, - ngfAPI.LoadBalancingTypeHashConsistent: {}, - ngfAPI.LoadBalancingTypeRandomTwo: {}, - ngfAPI.LoadBalancingTypeRandomTwoLeastConnection: {}, - ngfAPI.LoadBalancingTypeLeastTimeHeader: {}, - ngfAPI.LoadBalancingTypeLeastTimeLastByte: {}, - ngfAPI.LoadBalancingTypeLeastTimeHeaderInflight: {}, - ngfAPI.LoadBalancingTypeLeastTimeLastByteInflight: {}, - ngfAPI.LoadBalancingTypeRandomTwoLeastTimeHeader: {}, - ngfAPI.LoadBalancingTypeRandomTwoLeastTimeLastByte: {}, - } - - OSSAllowedLBMethods = map[ngfAPI.LoadBalancingType]struct{}{ - ngfAPI.LoadBalancingTypeRoundRobin: {}, - ngfAPI.LoadBalancingTypeLeastConnection: {}, - ngfAPI.LoadBalancingTypeIPHash: {}, - ngfAPI.LoadBalancingTypeRandom: {}, - ngfAPI.LoadBalancingTypeHash: {}, - ngfAPI.LoadBalancingTypeHashConsistent: {}, - ngfAPI.LoadBalancingTypeRandomTwo: {}, - ngfAPI.LoadBalancingTypeRandomTwoLeastConnection: {}, - } -) - -func (GenericValidator) ValidateLoadBalancingMethod(method string, plusEnabled bool) error { - lbMethod := ngfAPI.LoadBalancingType(method) - - if !plusEnabled { - if _, ok := OSSAllowedLBMethods[lbMethod]; ok { - return nil - } - } - - return fmt.Errorf("NGINX OSS only supports the following load balancing methods: %s", - getLoadBalancingMethodList(OSSAllowedLBMethods)) -} - -func getLoadBalancingMethodList(lbMethods map[ngfAPI.LoadBalancingType]struct{}) string { - var methods []string - for method := range lbMethods { - methods = append(methods, string(method)) - } - return strings.Join(methods, ", ") -} diff --git a/internal/controller/nginx/config/validation/generic_test.go b/internal/controller/nginx/config/validation/generic_test.go index 428511aa49..73be3f10cb 100644 --- a/internal/controller/nginx/config/validation/generic_test.go +++ b/internal/controller/nginx/config/validation/generic_test.go @@ -134,34 +134,3 @@ func TestValidateNginxVariableName(t *testing.T) { `var$name`, ) } - -func makeValidator(plusEnabled bool) simpleValidatorFunc[string] { - return func(v string) error { - return (GenericValidator{}).ValidateLoadBalancingMethod(v, plusEnabled) - } -} - -func TestValidateLoadBalancingMethod_OSS(t *testing.T) { - t.Helper() - - ossValidator := makeValidator(false) - testValidValuesForSimpleValidator(t, ossValidator, - "round_robin", - "least_conn", - "ip_hash", - "hash", - "hash consistent", - "random", - "random two", - "random two least_conn", - ) - - testInvalidValuesForSimpleValidator(t, ossValidator, - "random two least_time=header", - "random two least_time=last_byte", - "least_time header", - "least_time last_byte", - "least_time header inflight", - "least_time last_byte inflight", - ) -} diff --git a/internal/controller/state/validation/validationfakes/fake_generic_validator.go b/internal/controller/state/validation/validationfakes/fake_generic_validator.go index eed9deda3e..cd162c7359 100644 --- a/internal/controller/state/validation/validationfakes/fake_generic_validator.go +++ b/internal/controller/state/validation/validationfakes/fake_generic_validator.go @@ -30,18 +30,6 @@ type FakeGenericValidator struct { validateEscapedStringNoVarExpansionReturnsOnCall map[int]struct { result1 error } - ValidateLoadBalancingMethodStub func(string, bool) error - validateLoadBalancingMethodMutex sync.RWMutex - validateLoadBalancingMethodArgsForCall []struct { - arg1 string - arg2 bool - } - validateLoadBalancingMethodReturns struct { - result1 error - } - validateLoadBalancingMethodReturnsOnCall map[int]struct { - result1 error - } ValidateNginxDurationStub func(string) error validateNginxDurationMutex sync.RWMutex validateNginxDurationArgsForCall []struct { @@ -212,68 +200,6 @@ func (fake *FakeGenericValidator) ValidateEscapedStringNoVarExpansionReturnsOnCa }{result1} } -func (fake *FakeGenericValidator) ValidateLoadBalancingMethod(arg1 string, arg2 bool) error { - fake.validateLoadBalancingMethodMutex.Lock() - ret, specificReturn := fake.validateLoadBalancingMethodReturnsOnCall[len(fake.validateLoadBalancingMethodArgsForCall)] - fake.validateLoadBalancingMethodArgsForCall = append(fake.validateLoadBalancingMethodArgsForCall, struct { - arg1 string - arg2 bool - }{arg1, arg2}) - stub := fake.ValidateLoadBalancingMethodStub - fakeReturns := fake.validateLoadBalancingMethodReturns - fake.recordInvocation("ValidateLoadBalancingMethod", []interface{}{arg1, arg2}) - fake.validateLoadBalancingMethodMutex.Unlock() - if stub != nil { - return stub(arg1, arg2) - } - if specificReturn { - return ret.result1 - } - return fakeReturns.result1 -} - -func (fake *FakeGenericValidator) ValidateLoadBalancingMethodCallCount() int { - fake.validateLoadBalancingMethodMutex.RLock() - defer fake.validateLoadBalancingMethodMutex.RUnlock() - return len(fake.validateLoadBalancingMethodArgsForCall) -} - -func (fake *FakeGenericValidator) ValidateLoadBalancingMethodCalls(stub func(string, bool) error) { - fake.validateLoadBalancingMethodMutex.Lock() - defer fake.validateLoadBalancingMethodMutex.Unlock() - fake.ValidateLoadBalancingMethodStub = stub -} - -func (fake *FakeGenericValidator) ValidateLoadBalancingMethodArgsForCall(i int) (string, bool) { - fake.validateLoadBalancingMethodMutex.RLock() - defer fake.validateLoadBalancingMethodMutex.RUnlock() - argsForCall := fake.validateLoadBalancingMethodArgsForCall[i] - return argsForCall.arg1, argsForCall.arg2 -} - -func (fake *FakeGenericValidator) ValidateLoadBalancingMethodReturns(result1 error) { - fake.validateLoadBalancingMethodMutex.Lock() - defer fake.validateLoadBalancingMethodMutex.Unlock() - fake.ValidateLoadBalancingMethodStub = nil - fake.validateLoadBalancingMethodReturns = struct { - result1 error - }{result1} -} - -func (fake *FakeGenericValidator) ValidateLoadBalancingMethodReturnsOnCall(i int, result1 error) { - fake.validateLoadBalancingMethodMutex.Lock() - defer fake.validateLoadBalancingMethodMutex.Unlock() - fake.ValidateLoadBalancingMethodStub = nil - if fake.validateLoadBalancingMethodReturnsOnCall == nil { - fake.validateLoadBalancingMethodReturnsOnCall = make(map[int]struct { - result1 error - }) - } - fake.validateLoadBalancingMethodReturnsOnCall[i] = struct { - result1 error - }{result1} -} - func (fake *FakeGenericValidator) ValidateNginxDuration(arg1 string) error { fake.validateNginxDurationMutex.Lock() ret, specificReturn := fake.validateNginxDurationReturnsOnCall[len(fake.validateNginxDurationArgsForCall)] diff --git a/internal/controller/state/validation/validator.go b/internal/controller/state/validation/validator.go index f2f01d9654..5c1bd54e4e 100644 --- a/internal/controller/state/validation/validator.go +++ b/internal/controller/state/validation/validator.go @@ -50,7 +50,6 @@ type GenericValidator interface { ValidateNginxSize(size string) error ValidateEndpoint(endpoint string) error ValidateNginxVariableName(name string) error - ValidateLoadBalancingMethod(method string, plusEnabled bool) error } // PolicyValidator validates an NGF Policy. From 49b16b1b4775351504fb52f18c431819d0e0c44f Mon Sep 17 00:00:00 2001 From: salonichf5 <146118978+salonichf5@users.noreply.github.com> Date: Fri, 21 Nov 2025 15:44:52 -0700 Subject: [PATCH 5/6] add plus method check back --- .../controller/nginx/config/http/config.go | 39 ++++++++++++++----- .../policies/upstreamsettings/validator.go | 38 ++++++++++-------- .../upstreamsettings/validator_test.go | 17 +++++++- 3 files changed, 66 insertions(+), 28 deletions(-) diff --git a/internal/controller/nginx/config/http/config.go b/internal/controller/nginx/config/http/config.go index 292b4d770f..cd4d565965 100644 --- a/internal/controller/nginx/config/http/config.go +++ b/internal/controller/nginx/config/http/config.go @@ -170,13 +170,32 @@ type ServerConfig struct { DisableSNIHostValidation bool } -var OSSAllowedLBMethods = map[ngfAPI.LoadBalancingType]struct{}{ - ngfAPI.LoadBalancingTypeRoundRobin: {}, - ngfAPI.LoadBalancingTypeLeastConnection: {}, - ngfAPI.LoadBalancingTypeIPHash: {}, - ngfAPI.LoadBalancingTypeRandom: {}, - ngfAPI.LoadBalancingTypeHash: {}, - ngfAPI.LoadBalancingTypeHashConsistent: {}, - ngfAPI.LoadBalancingTypeRandomTwo: {}, - ngfAPI.LoadBalancingTypeRandomTwoLeastConnection: {}, -} +var ( + OSSAllowedLBMethods = map[ngfAPI.LoadBalancingType]struct{}{ + ngfAPI.LoadBalancingTypeRoundRobin: {}, + ngfAPI.LoadBalancingTypeLeastConnection: {}, + ngfAPI.LoadBalancingTypeIPHash: {}, + ngfAPI.LoadBalancingTypeRandom: {}, + ngfAPI.LoadBalancingTypeHash: {}, + ngfAPI.LoadBalancingTypeHashConsistent: {}, + ngfAPI.LoadBalancingTypeRandomTwo: {}, + ngfAPI.LoadBalancingTypeRandomTwoLeastConnection: {}, + } + + PlusAllowedLBMethods = map[ngfAPI.LoadBalancingType]struct{}{ + ngfAPI.LoadBalancingTypeRoundRobin: {}, + ngfAPI.LoadBalancingTypeLeastConnection: {}, + ngfAPI.LoadBalancingTypeIPHash: {}, + ngfAPI.LoadBalancingTypeRandom: {}, + ngfAPI.LoadBalancingTypeHash: {}, + ngfAPI.LoadBalancingTypeHashConsistent: {}, + ngfAPI.LoadBalancingTypeRandomTwo: {}, + ngfAPI.LoadBalancingTypeRandomTwoLeastConnection: {}, + ngfAPI.LoadBalancingTypeLeastTimeHeader: {}, + ngfAPI.LoadBalancingTypeLeastTimeLastByte: {}, + ngfAPI.LoadBalancingTypeLeastTimeHeaderInflight: {}, + ngfAPI.LoadBalancingTypeLeastTimeLastByteInflight: {}, + ngfAPI.LoadBalancingTypeRandomTwoLeastTimeHeader: {}, + ngfAPI.LoadBalancingTypeRandomTwoLeastTimeLastByte: {}, + } +) diff --git a/internal/controller/nginx/config/policies/upstreamsettings/validator.go b/internal/controller/nginx/config/policies/upstreamsettings/validator.go index f46420d4db..9df62a436c 100644 --- a/internal/controller/nginx/config/policies/upstreamsettings/validator.go +++ b/internal/controller/nginx/config/policies/upstreamsettings/validator.go @@ -159,31 +159,37 @@ func (v Validator) validateUpstreamKeepAlive( // ValidateLoadBalancingMethod validates the load balancing method for upstream servers. func (v Validator) validateLoadBalancingMethod(spec ngfAPI.UpstreamSettingsPolicySpec) field.ErrorList { - var allErrs field.ErrorList - fieldPath := field.NewPath("spec") - if spec.LoadBalancingMethod == nil { return nil } - if !v.plusEnabled { - if _, ok := httpConfig.OSSAllowedLBMethods[*spec.LoadBalancingMethod]; !ok { - path := fieldPath.Child("loadBalancingMethod") - allErrs = append(allErrs, field.Invalid( - path, - *spec.LoadBalancingMethod, - fmt.Sprintf( - "NGINX OSS only supports the following load balancing methods: %s", - getLoadBalancingMethodList(httpConfig.OSSAllowedLBMethods), - ), - )) - } + var allErrs field.ErrorList + path := field.NewPath("spec") + lbPath := path.Child("loadBalancingMethod") + + allowedMethods := httpConfig.OSSAllowedLBMethods + nginxType := "NGINX OSS" + if v.plusEnabled { + allowedMethods = httpConfig.PlusAllowedLBMethods + nginxType = "NGINX Plus" + } + + if _, ok := allowedMethods[*spec.LoadBalancingMethod]; !ok { + allErrs = append(allErrs, field.Invalid( + lbPath, + *spec.LoadBalancingMethod, + fmt.Sprintf( + "%s supports the following load balancing methods: %s", + nginxType, + getLoadBalancingMethodList(allowedMethods), + ), + )) } if spec.HashMethodKey != nil { hashMethodKey := *spec.HashMethodKey if err := v.genericValidator.ValidateNginxVariableName(string(hashMethodKey)); err != nil { - path := fieldPath.Child("hashMethodKey") + path := path.Child("hashMethodKey") allErrs = append(allErrs, field.Invalid(path, hashMethodKey, err.Error())) } } diff --git a/internal/controller/nginx/config/policies/upstreamsettings/validator_test.go b/internal/controller/nginx/config/policies/upstreamsettings/validator_test.go index 0fed0d2f5e..43285bdb70 100644 --- a/internal/controller/nginx/config/policies/upstreamsettings/validator_test.go +++ b/internal/controller/nginx/config/policies/upstreamsettings/validator_test.go @@ -335,7 +335,7 @@ func TestValidate_ValidateLoadBalancingMethod(t *testing.T) { }, expConditions: []conditions.Condition{ conditions.NewPolicyInvalid("spec.loadBalancingMethod: Invalid value: \"least_time last_byte\": " + - "NGINX OSS only supports the following load balancing methods: "), + "NGINX OSS supports the following load balancing methods: "), }, }, { @@ -357,9 +357,22 @@ func TestValidate_ValidateLoadBalancingMethod(t *testing.T) { }, expConditions: []conditions.Condition{ conditions.NewPolicyInvalid("spec.loadBalancingMethod: Invalid value: \"invalid-method\": " + - "NGINX OSS only supports the following load balancing methods: "), + "NGINX OSS supports the following load balancing methods: "), }, }, + { + name: "invalid load balancing method for NGINX Plus", + policy: &ngfAPI.UpstreamSettingsPolicy{ + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + LoadBalancingMethod: helpers.GetPointer(ngfAPI.LoadBalancingType("invalid-method")), + }, + }, + expConditions: []conditions.Condition{ + conditions.NewPolicyInvalid("spec.loadBalancingMethod: Invalid value: \"invalid-method\": " + + "NGINX Plus supports the following load balancing methods: "), + }, + plusEnabled: true, + }, } for _, test := range tests { From fdb07226c4a1084a0c32e222531b64c0f6108475 Mon Sep 17 00:00:00 2001 From: salonichf5 <146118978+salonichf5@users.noreply.github.com> Date: Fri, 21 Nov 2025 18:16:27 -0700 Subject: [PATCH 6/6] update validation for conflict --- apis/v1alpha1/upstreamsettingspolicy_types.go | 2 +- .../config/policies/upstreamsettings/validator.go | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/apis/v1alpha1/upstreamsettingspolicy_types.go b/apis/v1alpha1/upstreamsettingspolicy_types.go index 82147d014c..4b8131fad1 100644 --- a/apis/v1alpha1/upstreamsettingspolicy_types.go +++ b/apis/v1alpha1/upstreamsettingspolicy_types.go @@ -137,7 +137,7 @@ const ( // distributing requests evenly across all upstream servers. LoadBalancingTypeRoundRobin LoadBalancingType = "round_robin" - // LoadBalancingTypeLeastConn enables least-connections load balancing, + // LoadBalancingTypeLeastConnection enables least-connections load balancing, // routing requests to the upstream server with the fewest active connections. LoadBalancingTypeLeastConnection LoadBalancingType = "least_conn" diff --git a/internal/controller/nginx/config/policies/upstreamsettings/validator.go b/internal/controller/nginx/config/policies/upstreamsettings/validator.go index 9df62a436c..e56857f752 100644 --- a/internal/controller/nginx/config/policies/upstreamsettings/validator.go +++ b/internal/controller/nginx/config/policies/upstreamsettings/validator.go @@ -91,23 +91,23 @@ func conflicts(a, b ngfAPI.UpstreamSettingsPolicySpec) bool { } } - if !checkConflictForLoadBalancingFields(a, b) { + if checkConflictsForLoadBalancingFields(a, b) { return true } return false } -func checkConflictForLoadBalancingFields(a, b ngfAPI.UpstreamSettingsPolicySpec) bool { +func checkConflictsForLoadBalancingFields(a, b ngfAPI.UpstreamSettingsPolicySpec) bool { if a.LoadBalancingMethod != nil && b.LoadBalancingMethod != nil { - return *a.LoadBalancingMethod == *b.LoadBalancingMethod + return true } if a.HashMethodKey != nil && b.HashMethodKey != nil { - return *a.HashMethodKey == *b.HashMethodKey + return true } - return true + return false } // validateSettings performs validation on fields in the spec that are vulnerable to code injection.