From 78d1f015427144d410f67abca1aaaca58802c4c2 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Fri, 6 Dec 2024 13:46:46 -0800 Subject: [PATCH 01/23] Add NGINX configuration for UpstreamSettingsPolicy --- apis/v1alpha1/policy_methods.go | 12 + .../mode/static/nginx/config/generator.go | 17 +- .../mode/static/nginx/config/http/config.go | 14 +- .../static/nginx/config/policies/processor.go | 15 + .../policies/upstreamsettings/processor.go | 56 +++ .../upstreamsettings/processor_test.go | 166 +++++++++ internal/mode/static/nginx/config/servers.go | 186 +++++----- .../mode/static/nginx/config/servers_test.go | 324 ++++++++++-------- .../mode/static/nginx/config/upstreams.go | 70 +++- .../static/nginx/config/upstreams_template.go | 26 ++ .../static/nginx/config/upstreams_test.go | 303 +++++++++++++++- internal/mode/static/state/dataplane/types.go | 2 + 12 files changed, 932 insertions(+), 259 deletions(-) create mode 100644 internal/mode/static/nginx/config/policies/processor.go create mode 100644 internal/mode/static/nginx/config/policies/upstreamsettings/processor.go create mode 100644 internal/mode/static/nginx/config/policies/upstreamsettings/processor_test.go diff --git a/apis/v1alpha1/policy_methods.go b/apis/v1alpha1/policy_methods.go index 97e7074a62..ad399c0897 100644 --- a/apis/v1alpha1/policy_methods.go +++ b/apis/v1alpha1/policy_methods.go @@ -31,3 +31,15 @@ func (p *ObservabilityPolicy) GetPolicyStatus() v1alpha2.PolicyStatus { func (p *ObservabilityPolicy) SetPolicyStatus(status v1alpha2.PolicyStatus) { p.Status = status } + +func (p *UpstreamSettingsPolicy) GetTargetRefs() []v1alpha2.LocalPolicyTargetReference { + return p.Spec.TargetRefs +} + +func (p *UpstreamSettingsPolicy) GetPolicyStatus() v1alpha2.PolicyStatus { + return p.Status +} + +func (p *UpstreamSettingsPolicy) SetPolicyStatus(status v1alpha2.PolicyStatus) { + p.Status = status +} diff --git a/internal/mode/static/nginx/config/generator.go b/internal/mode/static/nginx/config/generator.go index 9792eb4d9a..ce27c012de 100644 --- a/internal/mode/static/nginx/config/generator.go +++ b/internal/mode/static/nginx/config/generator.go @@ -6,9 +6,11 @@ import ( "github.com/go-logr/logr" ngfConfig "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/config" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/http" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies/clientsettings" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies/observability" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies/upstreamsettings" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/file" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/dataplane" ) @@ -131,7 +133,10 @@ func (g GeneratorImpl) executeConfigTemplates( ) []file.File { fileBytes := make(map[string][]byte) - for _, execute := range g.getExecuteFuncs(generator) { + upstreams := g.createUpstreams(conf.Upstreams, upstreamsettings.NewProcessor()) + upstreamMap := g.createUpstreamMap(upstreams) + + for _, execute := range g.getExecuteFuncs(generator, upstreams, upstreamMap) { results := execute(conf) for _, res := range results { fileBytes[res.dest] = append(fileBytes[res.dest], res.data...) @@ -156,12 +161,16 @@ func (g GeneratorImpl) executeConfigTemplates( return files } -func (g GeneratorImpl) getExecuteFuncs(generator policies.Generator) []executeFunc { +func (g GeneratorImpl) getExecuteFuncs( + generator policies.Generator, + upstreams []http.Upstream, + upstreamMap UpstreamMap, +) []executeFunc { return []executeFunc{ executeMainConfig, executeBaseHTTPConfig, - g.newExecuteServersFunc(generator), - g.executeUpstreams, + g.newExecuteServersFunc(generator, upstreamMap), + g.newExecuteUpstreamsFunc(upstreams), executeSplitClients, executeMaps, executeTelemetry, diff --git a/internal/mode/static/nginx/config/http/config.go b/internal/mode/static/nginx/config/http/config.go index 24aecaa3e4..eda12cb8ff 100644 --- a/internal/mode/static/nginx/config/http/config.go +++ b/internal/mode/static/nginx/config/http/config.go @@ -1,6 +1,8 @@ package http -import "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/shared" +import ( + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/shared" +) const ( InternalRoutePathPrefix = "/_ngf-internal" @@ -82,9 +84,13 @@ const ( // Upstream holds all configuration for an HTTP upstream. type Upstream struct { - Name string - ZoneSize string // format: 512k, 1m - Servers []UpstreamServer + Name string + ZoneSize string // format: 512k, 1m + KeepAliveTime string + KeepAliveTimeout string + Servers []UpstreamServer + KeepAliveConnections int32 + KeepAliveRequests int32 } // UpstreamServer holds all configuration for an HTTP upstream server. diff --git a/internal/mode/static/nginx/config/policies/processor.go b/internal/mode/static/nginx/config/policies/processor.go new file mode 100644 index 0000000000..d1b70247d8 --- /dev/null +++ b/internal/mode/static/nginx/config/policies/processor.go @@ -0,0 +1,15 @@ +package policies + +// UpstreamSettingsProcessor defines an interface for an UpstreamSettingsPolicy to implement the process function. +type UpstreamSettingsProcessor interface { + Process(policies []Policy) UpstreamSettings +} + +// UpstreamSettings contains settings from UpstreamSettingsPolicy. +type UpstreamSettings struct { + ZoneSize string + KeepAliveTime string + KeepAliveTimeout string + KeepAliveConnections int32 + KeepAliveRequests int32 +} diff --git a/internal/mode/static/nginx/config/policies/upstreamsettings/processor.go b/internal/mode/static/nginx/config/policies/upstreamsettings/processor.go new file mode 100644 index 0000000000..dd403e957a --- /dev/null +++ b/internal/mode/static/nginx/config/policies/upstreamsettings/processor.go @@ -0,0 +1,56 @@ +package upstreamsettings + +import ( + ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies" +) + +// Processor processes UpstreamSettingsPolicies. +type Processor struct{} + +// NewProcessor returns a new instance of Processor. +func NewProcessor() *Processor { + return &Processor{} +} + +// Process processes policy configuration for an upstream block. +func (g Processor) Process(pols []policies.Policy) policies.UpstreamSettings { + return processPolicies(pols) +} + +func processPolicies(pols []policies.Policy) policies.UpstreamSettings { + upstreamSettings := policies.UpstreamSettings{} + + for _, pol := range pols { + usp, ok := pol.(*ngfAPI.UpstreamSettingsPolicy) + if !ok { + continue + } + + // we can assume that there will be no instance of two or more policies setting the same + // field for the same service + if usp.Spec.ZoneSize != nil { + upstreamSettings.ZoneSize = string(*usp.Spec.ZoneSize) + } + + if usp.Spec.KeepAlive != nil { + if usp.Spec.KeepAlive.Connections != nil { + upstreamSettings.KeepAliveConnections = *usp.Spec.KeepAlive.Connections + } + + if usp.Spec.KeepAlive.Requests != nil { + upstreamSettings.KeepAliveRequests = *usp.Spec.KeepAlive.Requests + } + + if usp.Spec.KeepAlive.Time != nil { + upstreamSettings.KeepAliveTime = string(*usp.Spec.KeepAlive.Time) + } + + if usp.Spec.KeepAlive.Timeout != nil { + upstreamSettings.KeepAliveTimeout = string(*usp.Spec.KeepAlive.Timeout) + } + } + } + + return upstreamSettings +} diff --git a/internal/mode/static/nginx/config/policies/upstreamsettings/processor_test.go b/internal/mode/static/nginx/config/policies/upstreamsettings/processor_test.go new file mode 100644 index 0000000000..7bd7159acb --- /dev/null +++ b/internal/mode/static/nginx/config/policies/upstreamsettings/processor_test.go @@ -0,0 +1,166 @@ +package upstreamsettings + +import ( + "testing" + + . "github.com/onsi/gomega" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1" + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies" +) + +func TestProcess(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + expUpstreamSettings policies.UpstreamSettings + policies []policies.Policy + }{ + { + name: "all fields populated", + policies: []policies.Policy{ + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp", + Namespace: "test", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + ZoneSize: helpers.GetPointer[ngfAPI.Size]("2m"), + KeepAlive: helpers.GetPointer(ngfAPI.UpstreamKeepAlive{ + Connections: helpers.GetPointer(int32(1)), + Requests: helpers.GetPointer(int32(1)), + Time: helpers.GetPointer[ngfAPI.Duration]("5s"), + Timeout: helpers.GetPointer[ngfAPI.Duration]("10s"), + }), + }, + }, + }, + expUpstreamSettings: policies.UpstreamSettings{ + ZoneSize: "2m", + KeepAliveConnections: 1, + KeepAliveRequests: 1, + KeepAliveTime: "5s", + KeepAliveTimeout: "10s", + }, + }, + { + name: "zoneSize set", + policies: []policies.Policy{ + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp", + Namespace: "test", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + ZoneSize: helpers.GetPointer[ngfAPI.Size]("2m"), + }, + }, + }, + expUpstreamSettings: policies.UpstreamSettings{ + ZoneSize: "2m", + }, + }, + { + name: "keepAlive Connections set", + policies: []policies.Policy{ + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp", + Namespace: "test", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + KeepAlive: helpers.GetPointer(ngfAPI.UpstreamKeepAlive{ + Connections: helpers.GetPointer(int32(1)), + }), + }, + }, + }, + expUpstreamSettings: policies.UpstreamSettings{ + KeepAliveConnections: 1, + }, + }, + { + name: "keepAlive Requests set", + policies: []policies.Policy{ + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp", + Namespace: "test", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + KeepAlive: helpers.GetPointer(ngfAPI.UpstreamKeepAlive{ + Requests: helpers.GetPointer(int32(1)), + }), + }, + }, + }, + expUpstreamSettings: policies.UpstreamSettings{ + KeepAliveRequests: 1, + }, + }, + { + name: "keepAlive Time set", + policies: []policies.Policy{ + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp", + Namespace: "test", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + KeepAlive: helpers.GetPointer(ngfAPI.UpstreamKeepAlive{ + Time: helpers.GetPointer[ngfAPI.Duration]("5s"), + }), + }, + }, + }, + expUpstreamSettings: policies.UpstreamSettings{ + KeepAliveTime: "5s", + }, + }, + { + name: "keepAlive Timeout set", + policies: []policies.Policy{ + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp", + Namespace: "test", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + KeepAlive: helpers.GetPointer(ngfAPI.UpstreamKeepAlive{ + Timeout: helpers.GetPointer[ngfAPI.Duration]("10s"), + }), + }, + }, + }, + expUpstreamSettings: policies.UpstreamSettings{ + KeepAliveTimeout: "10s", + }, + }, + { + name: "no fields populated", + policies: []policies.Policy{ + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp", + Namespace: "test", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{}, + }, + }, + expUpstreamSettings: policies.UpstreamSettings{}, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + processor := NewProcessor() + + g.Expect(processor.Process(test.policies)).To(Equal(test.expUpstreamSettings)) + }) + } +} diff --git a/internal/mode/static/nginx/config/servers.go b/internal/mode/static/nginx/config/servers.go index 33ea858f31..559ae202e6 100644 --- a/internal/mode/static/nginx/config/servers.go +++ b/internal/mode/static/nginx/config/servers.go @@ -23,82 +23,33 @@ const ( rootPath = "/" ) -// httpBaseHeaders contains the constant headers set in each HTTP server block. -var httpBaseHeaders = []http.Header{ - { - Name: "Host", - Value: "$gw_api_compliant_host", - }, - { - Name: "X-Forwarded-For", - Value: "$proxy_add_x_forwarded_for", - }, - { - Name: "Upgrade", - Value: "$http_upgrade", - }, - { - Name: "Connection", - Value: "$connection_upgrade", - }, - { - Name: "X-Real-IP", - Value: "$remote_addr", - }, - { - Name: "X-Forwarded-Proto", - Value: "$scheme", - }, - { - Name: "X-Forwarded-Host", - Value: "$host", - }, - { - Name: "X-Forwarded-Port", - Value: "$server_port", - }, -} - -// grpcBaseHeaders contains the constant headers set in each gRPC server block. -var grpcBaseHeaders = []http.Header{ - { - Name: "Host", - Value: "$gw_api_compliant_host", - }, - { - Name: "X-Forwarded-For", - Value: "$proxy_add_x_forwarded_for", - }, - { - Name: "Authority", - Value: "$gw_api_compliant_host", - }, - { - Name: "X-Real-IP", - Value: "$remote_addr", - }, - { - Name: "X-Forwarded-Proto", - Value: "$scheme", - }, - { - Name: "X-Forwarded-Host", - Value: "$host", - }, - { - Name: "X-Forwarded-Port", - Value: "$server_port", - }, -} - -func (g GeneratorImpl) newExecuteServersFunc(generator policies.Generator) executeFunc { +var authorityHeader = http.Header{ + Name: "Authority", + Value: "$gw_api_compliant_host", +} + +var connectionHeader = http.Header{ + Name: "Connection", + Value: "$connection_upgrade", +} + +var upgradeHeader = http.Header{ + Name: "Upgrade", + Value: "$http_upgrade", +} + +func (g GeneratorImpl) newExecuteServersFunc(generator policies.Generator, um UpstreamMap) executeFunc { return func(configuration dataplane.Configuration) []executeResult { - return g.executeServers(configuration, generator) + return g.executeServers(configuration, generator, um) } } -func (g GeneratorImpl) executeServers(conf dataplane.Configuration, generator policies.Generator) []executeResult { - servers, httpMatchPairs := createServers(conf, generator) +func (g GeneratorImpl) executeServers( + conf dataplane.Configuration, + generator policies.Generator, + um UpstreamMap, +) []executeResult { + servers, httpMatchPairs := createServers(conf, generator, um) serverConfig := http.ServerConfig{ Servers: servers, @@ -145,7 +96,11 @@ func getIPFamily(baseHTTPConfig dataplane.BaseHTTPConfig) shared.IPFamily { return shared.IPFamily{IPv4: true, IPv6: true} } -func createServers(conf dataplane.Configuration, generator policies.Generator) ([]http.Server, httpMatchPairs) { +func createServers( + conf dataplane.Configuration, + generator policies.Generator, + um UpstreamMap, +) ([]http.Server, httpMatchPairs) { servers := make([]http.Server, 0, len(conf.HTTPServers)+len(conf.SSLServers)) finalMatchPairs := make(httpMatchPairs) sharedTLSPorts := make(map[int32]struct{}) @@ -156,7 +111,7 @@ func createServers(conf dataplane.Configuration, generator policies.Generator) ( for idx, s := range conf.HTTPServers { serverID := fmt.Sprintf("%d", idx) - httpServer, matchPairs := createServer(s, serverID, generator) + httpServer, matchPairs := createServer(s, serverID, generator, um) servers = append(servers, httpServer) maps.Copy(finalMatchPairs, matchPairs) } @@ -164,7 +119,7 @@ func createServers(conf dataplane.Configuration, generator policies.Generator) ( for idx, s := range conf.SSLServers { serverID := fmt.Sprintf("SSL_%d", idx) - sslServer, matchPairs := createSSLServer(s, serverID, generator) + sslServer, matchPairs := createSSLServer(s, serverID, generator, um) if _, portInUse := sharedTLSPorts[s.Port]; portInUse { sslServer.Listen = getSocketNameHTTPS(s.Port) sslServer.IsSocket = true @@ -180,6 +135,7 @@ func createSSLServer( virtualServer dataplane.VirtualServer, serverID string, generator policies.Generator, + um UpstreamMap, ) (http.Server, httpMatchPairs) { listen := fmt.Sprint(virtualServer.Port) if virtualServer.IsDefault { @@ -189,7 +145,7 @@ func createSSLServer( }, nil } - locs, matchPairs, grpc := createLocations(&virtualServer, serverID, generator) + locs, matchPairs, grpc := createLocations(&virtualServer, serverID, generator, um) server := http.Server{ ServerName: virtualServer.Hostname, @@ -218,6 +174,7 @@ func createServer( virtualServer dataplane.VirtualServer, serverID string, generator policies.Generator, + um UpstreamMap, ) (http.Server, httpMatchPairs) { listen := fmt.Sprint(virtualServer.Port) @@ -228,7 +185,7 @@ func createServer( }, nil } - locs, matchPairs, grpc := createLocations(&virtualServer, serverID, generator) + locs, matchPairs, grpc := createLocations(&virtualServer, serverID, generator, um) server := http.Server{ ServerName: virtualServer.Hostname, @@ -264,6 +221,7 @@ func createLocations( server *dataplane.VirtualServer, serverID string, generator policies.Generator, + um UpstreamMap, ) ([]http.Location, httpMatchPairs, bool) { maxLocs, pathsAndTypes := getMaxLocationCountAndPathMap(server.PathRules) locs := make([]http.Location, 0, maxLocs) @@ -292,7 +250,7 @@ func createLocations( if !needsInternalLocations(rule) { for _, r := range rule.MatchRules { - extLocations = updateLocations(r.Filters, extLocations, r, server.Port, rule.Path, rule.GRPC) + extLocations = updateLocations(r.Filters, extLocations, r, server.Port, rule.Path, rule.GRPC, um) } locs = append(locs, extLocations...) @@ -314,6 +272,7 @@ func createLocations( server.Port, rule.Path, rule.GRPC, + um, ) internalLocations = append(internalLocations, intLocation) @@ -450,6 +409,7 @@ func updateLocation( listenerPort int32, path string, grpc bool, + um UpstreamMap, ) http.Location { if filters.InvalidFilter != nil { location.Return = &http.Return{Code: http.StatusInternalServerError} @@ -465,7 +425,7 @@ func updateLocation( } rewrites := createRewritesValForRewriteFilter(filters.RequestURLRewrite, path) - proxySetHeaders := generateProxySetHeaders(&matchRule.Filters, grpc) + proxySetHeaders := generateProxySetHeaders(&matchRule.Filters, grpc, um, matchRule.BackendGroup.Backends) responseHeaders := generateResponseHeaders(&matchRule.Filters) if rewrites != nil { @@ -502,11 +462,12 @@ func updateLocations( listenerPort int32, path string, grpc bool, + um UpstreamMap, ) []http.Location { updatedLocations := make([]http.Location, len(buildLocations)) for i, loc := range buildLocations { - updatedLocations[i] = updateLocation(filters, loc, matchRule, listenerPort, path, grpc) + updatedLocations[i] = updateLocation(filters, loc, matchRule, listenerPort, path, grpc, um) } return updatedLocations @@ -760,16 +721,34 @@ func createMatchLocation(path string, grpc bool) http.Location { return loc } -func generateProxySetHeaders(filters *dataplane.HTTPFilters, grpc bool) []http.Header { - var headers []http.Header - if !grpc { - headers = make([]http.Header, len(httpBaseHeaders)) - copy(headers, httpBaseHeaders) +func generateProxySetHeaders( + filters *dataplane.HTTPFilters, + grpc bool, + um UpstreamMap, + backends []dataplane.Backend, +) []http.Header { + var keepAlive bool + + for _, backend := range backends { + if um.keepAliveEnabled(backend.UpstreamName) { + keepAlive = true + break + } + } + + var extraHeaders []http.Header + if grpc { + extraHeaders = append(extraHeaders, authorityHeader) } else { - headers = make([]http.Header, len(grpcBaseHeaders)) - copy(headers, grpcBaseHeaders) + extraHeaders = append(extraHeaders, upgradeHeader) + + if !keepAlive { + extraHeaders = append(extraHeaders, connectionHeader) + } } + headers := createBaseProxySetHeaders(extraHeaders...) + if filters != nil && filters.RequestURLRewrite != nil && filters.RequestURLRewrite.Hostname != nil { for i, header := range headers { if header.Name == "Host" { @@ -887,3 +866,36 @@ func getRewriteClientIPSettings(rewriteIPConfig dataplane.RewriteClientIPSetting ProxyProtocol: proxyProtocol, } } + +func createBaseProxySetHeaders(headers ...http.Header) []http.Header { + baseHeaders := []http.Header{ + { + Name: "Host", + Value: "$gw_api_compliant_host", + }, + { + Name: "X-Forwarded-For", + Value: "$proxy_add_x_forwarded_for", + }, + { + Name: "X-Real-IP", + Value: "$remote_addr", + }, + { + Name: "X-Forwarded-Proto", + Value: "$scheme", + }, + { + Name: "X-Forwarded-Host", + Value: "$host", + }, + { + Name: "X-Forwarded-Port", + Value: "$server_port", + }, + } + + baseHeaders = append(baseHeaders, headers...) + + return baseHeaders +} diff --git a/internal/mode/static/nginx/config/servers_test.go b/internal/mode/static/nginx/config/servers_test.go index 112991b521..ec271bbff2 100644 --- a/internal/mode/static/nginx/config/servers_test.go +++ b/internal/mode/static/nginx/config/servers_test.go @@ -17,6 +17,11 @@ import ( "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/dataplane" ) +var ( + httpBaseHeaders = createBaseProxySetHeaders(upgradeHeader, connectionHeader) + grpcBaseHeaders = createBaseProxySetHeaders(authorityHeader) +) + func TestExecuteServers(t *testing.T) { t.Parallel() @@ -182,7 +187,7 @@ func TestExecuteServers(t *testing.T) { ) gen := GeneratorImpl{} - results := gen.executeServers(conf, fakeGenerator) + results := gen.executeServers(conf, fakeGenerator, UpstreamMap{}) g.Expect(results).To(HaveLen(len(expectedResults))) for _, res := range results { @@ -321,7 +326,7 @@ func TestExecuteServers_IPFamily(t *testing.T) { g := NewWithT(t) gen := GeneratorImpl{} - results := gen.executeServers(test.config, &policiesfakes.FakeGenerator{}) + results := gen.executeServers(test.config, &policiesfakes.FakeGenerator{}, UpstreamMap{}) g.Expect(results).To(HaveLen(2)) serverConf := string(results[0].data) httpMatchConf := string(results[1].data) @@ -439,7 +444,7 @@ func TestExecuteServers_RewriteClientIP(t *testing.T) { g := NewWithT(t) gen := GeneratorImpl{} - results := gen.executeServers(test.config, &policiesfakes.FakeGenerator{}) + results := gen.executeServers(test.config, &policiesfakes.FakeGenerator{}, UpstreamMap{}) g.Expect(results).To(HaveLen(2)) serverConf := string(results[0].data) httpMatchConf := string(results[1].data) @@ -481,7 +486,7 @@ func TestExecuteServers_Plus(t *testing.T) { g := NewWithT(t) gen := GeneratorImpl{plus: true} - results := gen.executeServers(config, &policiesfakes.FakeGenerator{}) + results := gen.executeServers(config, &policiesfakes.FakeGenerator{}, UpstreamMap{}) g.Expect(results).To(HaveLen(2)) serverConf := string(results[0].data) @@ -565,7 +570,7 @@ func TestExecuteForDefaultServers(t *testing.T) { g := NewWithT(t) gen := GeneratorImpl{} - serverResults := gen.executeServers(tc.conf, &policiesfakes.FakeGenerator{}) + serverResults := gen.executeServers(tc.conf, &policiesfakes.FakeGenerator{}, UpstreamMap{}) g.Expect(serverResults).To(HaveLen(2)) serverConf := string(serverResults[0].data) httpMatchConf := string(serverResults[1].data) @@ -1072,14 +1077,6 @@ func TestCreateServers(t *testing.T) { Name: "X-Forwarded-For", Value: "$proxy_add_x_forwarded_for", }, - { - Name: "Upgrade", - Value: "$http_upgrade", - }, - { - Name: "Connection", - Value: "$connection_upgrade", - }, { Name: "X-Real-IP", Value: "$remote_addr", @@ -1096,6 +1093,14 @@ func TestCreateServers(t *testing.T) { Name: "X-Forwarded-Port", Value: "$server_port", }, + { + Name: "Upgrade", + Value: "$http_upgrade", + }, + { + Name: "Connection", + Value: "$connection_upgrade", + }, } externalIncludes := []shared.Include{ @@ -1343,44 +1348,12 @@ func TestCreateServers(t *testing.T) { { Path: "/proxy-set-headers/", ProxyPass: "http://test_foo_80$request_uri", - ProxySetHeaders: []http.Header{ + ProxySetHeaders: append([]http.Header{ { Name: "my-header", Value: "${my_header_header_var}some-value-123", }, - { - Name: "Host", - Value: "$gw_api_compliant_host", - }, - { - Name: "X-Forwarded-For", - Value: "$proxy_add_x_forwarded_for", - }, - { - Name: "Upgrade", - Value: "$http_upgrade", - }, - { - Name: "Connection", - Value: "$connection_upgrade", - }, - { - Name: "X-Real-IP", - Value: "$remote_addr", - }, - { - Name: "X-Forwarded-Proto", - Value: "$scheme", - }, - { - Name: "X-Forwarded-Host", - Value: "$host", - }, - { - Name: "X-Forwarded-Port", - Value: "$server_port", - }, - }, + }, httpBaseHeaders...), ResponseHeaders: http.ResponseHeaders{ Add: []http.Header{ { @@ -1397,44 +1370,12 @@ func TestCreateServers(t *testing.T) { { Path: "= /proxy-set-headers", ProxyPass: "http://test_foo_80$request_uri", - ProxySetHeaders: []http.Header{ + ProxySetHeaders: append([]http.Header{ { Name: "my-header", Value: "${my_header_header_var}some-value-123", }, - { - Name: "Host", - Value: "$gw_api_compliant_host", - }, - { - Name: "X-Forwarded-For", - Value: "$proxy_add_x_forwarded_for", - }, - { - Name: "Upgrade", - Value: "$http_upgrade", - }, - { - Name: "Connection", - Value: "$connection_upgrade", - }, - { - Name: "X-Real-IP", - Value: "$remote_addr", - }, - { - Name: "X-Forwarded-Proto", - Value: "$scheme", - }, - { - Name: "X-Forwarded-Host", - Value: "$host", - }, - { - Name: "X-Forwarded-Port", - Value: "$server_port", - }, - }, + }, httpBaseHeaders...), ResponseHeaders: http.ResponseHeaders{ Add: []http.Header{ { @@ -1540,8 +1481,7 @@ func TestCreateServers(t *testing.T) { Content: []byte("include-1"), }, }) - - result, httpMatchPair := createServers(conf, fakeGenerator) + result, httpMatchPair := createServers(conf, fakeGenerator, UpstreamMap{}) g.Expect(httpMatchPair).To(Equal(allExpMatchPair)) g.Expect(helpers.Diff(expectedServers, result)).To(BeEmpty()) @@ -1762,6 +1702,7 @@ func TestCreateServersConflicts(t *testing.T) { result, _ := createServers( dataplane.Configuration{HTTPServers: httpServers}, &policiesfakes.FakeGenerator{}, + UpstreamMap{}, ) g.Expect(helpers.Diff(expectedServers, result)).To(BeEmpty()) }) @@ -1912,7 +1853,7 @@ func TestCreateServers_Includes(t *testing.T) { conf := dataplane.Configuration{HTTPServers: httpServers, SSLServers: sslServers} - actualServers, matchPairs := createServers(conf, fakeGenerator) + actualServers, matchPairs := createServers(conf, fakeGenerator, UpstreamMap{}) g.Expect(matchPairs).To(BeEmpty()) g.Expect(actualServers).To(HaveLen(len(expServers))) @@ -2073,7 +2014,7 @@ func TestCreateLocations_Includes(t *testing.T) { }, }) - locations, matches, grpc := createLocations(&httpServer, "1", fakeGenerator) + locations, matches, grpc := createLocations(&httpServer, "1", fakeGenerator, UpstreamMap{}) g := NewWithT(t) g.Expect(grpc).To(BeFalse()) @@ -2256,10 +2197,15 @@ func TestCreateLocationsRootPath(t *testing.T) { t.Parallel() g := NewWithT(t) - locs, httpMatchPair, grpc := createLocations(&dataplane.VirtualServer{ - PathRules: test.pathRules, - Port: 80, - }, "1", &policiesfakes.FakeGenerator{}) + locs, httpMatchPair, grpc := createLocations( + &dataplane.VirtualServer{ + PathRules: test.pathRules, + Port: 80, + }, + "1", + &policiesfakes.FakeGenerator{}, + UpstreamMap{}, + ) g.Expect(locs).To(Equal(test.expLocations)) g.Expect(httpMatchPair).To(BeEmpty()) g.Expect(grpc).To(Equal(test.grpc)) @@ -2897,6 +2843,8 @@ func TestGenerateProxySetHeaders(t *testing.T) { filters *dataplane.HTTPFilters msg string expectedHeaders []http.Header + upstreamMap UpstreamMap + backends []dataplane.Backend GRPC bool }{ { @@ -2918,7 +2866,7 @@ func TestGenerateProxySetHeaders(t *testing.T) { Remove: []string{"my-header"}, }, }, - expectedHeaders: []http.Header{ + expectedHeaders: append([]http.Header{ { Name: "Authorization", Value: "${authorization_header_var}my-auth", @@ -2931,39 +2879,7 @@ func TestGenerateProxySetHeaders(t *testing.T) { Name: "my-header", Value: "", }, - { - Name: "Host", - Value: "$gw_api_compliant_host", - }, - { - Name: "X-Forwarded-For", - Value: "$proxy_add_x_forwarded_for", - }, - { - Name: "Upgrade", - Value: "$http_upgrade", - }, - { - Name: "Connection", - Value: "$connection_upgrade", - }, - { - Name: "X-Real-IP", - Value: "$remote_addr", - }, - { - Name: "X-Forwarded-Proto", - Value: "$scheme", - }, - { - Name: "X-Forwarded-Host", - Value: "$host", - }, - { - Name: "X-Forwarded-Port", - Value: "$server_port", - }, - }, + }, httpBaseHeaders...), }, { msg: "with url rewrite hostname", @@ -2993,14 +2909,6 @@ func TestGenerateProxySetHeaders(t *testing.T) { Name: "X-Forwarded-For", Value: "$proxy_add_x_forwarded_for", }, - { - Name: "Upgrade", - Value: "$http_upgrade", - }, - { - Name: "Connection", - Value: "$connection_upgrade", - }, { Name: "X-Real-IP", Value: "$remote_addr", @@ -3017,6 +2925,14 @@ func TestGenerateProxySetHeaders(t *testing.T) { Name: "X-Forwarded-Port", Value: "$server_port", }, + { + Name: "Upgrade", + Value: "$http_upgrade", + }, + { + Name: "Connection", + Value: "$connection_upgrade", + }, }, }, { @@ -3039,7 +2955,7 @@ func TestGenerateProxySetHeaders(t *testing.T) { Remove: []string{"my-header"}, }, }, - expectedHeaders: []http.Header{ + expectedHeaders: append([]http.Header{ { Name: "Authorization", Value: "${authorization_header_var}my-auth", @@ -3052,33 +2968,77 @@ func TestGenerateProxySetHeaders(t *testing.T) { Name: "my-header", Value: "", }, - { - Name: "Host", - Value: "$gw_api_compliant_host", + }, grpcBaseHeaders...), + }, + { + msg: "upstream with keepAlive enabled", + expectedHeaders: createBaseProxySetHeaders(upgradeHeader), + upstreamMap: UpstreamMap{ + nameToUpstream: map[string]http.Upstream{ + "upstream": { + Name: "upstream", + KeepAliveConnections: 1, + }, }, + }, + backends: []dataplane.Backend{ { - Name: "X-Forwarded-For", - Value: "$proxy_add_x_forwarded_for", + UpstreamName: "upstream", + }, + }, + }, + { + msg: "multiple upstreams with keepAlive enabled", + expectedHeaders: createBaseProxySetHeaders(upgradeHeader), + upstreamMap: UpstreamMap{ + nameToUpstream: map[string]http.Upstream{ + "upstream1": { + Name: "upstream1", + KeepAliveConnections: 1, + }, + "upstream2": { + Name: "upstream2", + KeepAliveRequests: 1, + }, + "upstream3": { + Name: "upstream3", + KeepAliveTime: "5s", + }, }, + }, + backends: []dataplane.Backend{ { - Name: "Authority", - Value: "$gw_api_compliant_host", + UpstreamName: "upstream1", }, { - Name: "X-Real-IP", - Value: "$remote_addr", + UpstreamName: "upstream2", }, { - Name: "X-Forwarded-Proto", - Value: "$scheme", + UpstreamName: "upstream3", }, + }, + }, + { + msg: "mix of upstreams with keepAlive enabled and disabled", + expectedHeaders: createBaseProxySetHeaders(upgradeHeader), + upstreamMap: UpstreamMap{ + nameToUpstream: map[string]http.Upstream{ + "upstream1": { + Name: "upstream1", + KeepAliveConnections: 1, + }, + "upstream2": { + Name: "upstream2", + ZoneSize: "2m", + }, + }, + }, + backends: []dataplane.Backend{ { - Name: "X-Forwarded-Host", - Value: "$host", + UpstreamName: "upstream1", }, { - Name: "X-Forwarded-Port", - Value: "$server_port", + UpstreamName: "upstream2", }, }, }, @@ -3089,12 +3049,80 @@ func TestGenerateProxySetHeaders(t *testing.T) { t.Parallel() g := NewWithT(t) - headers := generateProxySetHeaders(tc.filters, tc.GRPC) + headers := generateProxySetHeaders(tc.filters, tc.GRPC, tc.upstreamMap, tc.backends) g.Expect(headers).To(Equal(tc.expectedHeaders)) }) } } +func TestCreateBaseProxySetHeaders(t *testing.T) { + t.Parallel() + + baseHeaders := []http.Header{ + { + Name: "Host", + Value: "$gw_api_compliant_host", + }, + { + Name: "X-Forwarded-For", + Value: "$proxy_add_x_forwarded_for", + }, + { + Name: "X-Real-IP", + Value: "$remote_addr", + }, + { + Name: "X-Forwarded-Proto", + Value: "$scheme", + }, + { + Name: "X-Forwarded-Host", + Value: "$host", + }, + { + Name: "X-Forwarded-Port", + Value: "$server_port", + }, + } + + tests := []struct { + msg string + additionalHeaders []http.Header + expBaseHeaders []http.Header + }{ + { + msg: "no additional headers", + additionalHeaders: []http.Header{}, + expBaseHeaders: baseHeaders, + }, + { + msg: "single additional headers", + additionalHeaders: []http.Header{ + authorityHeader, + }, + expBaseHeaders: append(baseHeaders, authorityHeader), + }, + { + msg: "multiple additional headers", + additionalHeaders: []http.Header{ + connectionHeader, + upgradeHeader, + }, + expBaseHeaders: append(baseHeaders, connectionHeader, upgradeHeader), + }, + } + + for _, test := range tests { + t.Run(test.msg, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + result := createBaseProxySetHeaders(test.additionalHeaders...) + g.Expect(result).To(Equal(test.expBaseHeaders)) + }) + } +} + func TestConvertBackendTLSFromGroup(t *testing.T) { t.Parallel() diff --git a/internal/mode/static/nginx/config/upstreams.go b/internal/mode/static/nginx/config/upstreams.go index 88c66c47fd..289bf4bd24 100644 --- a/internal/mode/static/nginx/config/upstreams.go +++ b/internal/mode/static/nginx/config/upstreams.go @@ -6,11 +6,15 @@ import ( "github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/http" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/stream" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/dataplane" ) -var upstreamsTemplate = gotemplate.Must(gotemplate.New("upstreams").Parse(upstreamsTemplateText)) +var ( + upstreamsTemplate = gotemplate.Must(gotemplate.New("upstreams").Parse(upstreamsTemplateText)) + streamUpstreamsTemplate = gotemplate.Must(gotemplate.New("streamUpstreams").Parse(streamUpstreamsTemplateText)) +) const ( // nginx503Server is used as a backend for services that cannot be resolved (have no IP address). @@ -29,9 +33,29 @@ const ( plusZoneSizeStream = "1m" ) -func (g GeneratorImpl) executeUpstreams(conf dataplane.Configuration) []executeResult { - upstreams := g.createUpstreams(conf.Upstreams) +// UpstreamMap holds a map which maps upstream name to http.Upstream. +type UpstreamMap struct { + nameToUpstream map[string]http.Upstream +} + +func (um UpstreamMap) keepAliveEnabled(name string) bool { + if upstream, exists := um.nameToUpstream[name]; exists { + return upstream.KeepAliveConnections != 0 || + upstream.KeepAliveRequests != 0 || + upstream.KeepAliveTime != "" || + upstream.KeepAliveTimeout != "" + } + + return false +} +func (g GeneratorImpl) newExecuteUpstreamsFunc(upstreams []http.Upstream) executeFunc { + return func(_ dataplane.Configuration) []executeResult { + return g.executeUpstreams(upstreams) + } +} + +func (g GeneratorImpl) executeUpstreams(upstreams []http.Upstream) []executeResult { result := executeResult{ dest: httpConfigFile, data: helpers.MustExecuteTemplate(upstreamsTemplate, upstreams), @@ -40,12 +64,22 @@ func (g GeneratorImpl) executeUpstreams(conf dataplane.Configuration) []executeR return []executeResult{result} } +func (g GeneratorImpl) createUpstreamMap(upstreams []http.Upstream) UpstreamMap { + upstreamMap := UpstreamMap{nameToUpstream: make(map[string]http.Upstream)} + + for _, upstream := range upstreams { + upstreamMap.nameToUpstream[upstream.Name] = upstream + } + + return upstreamMap +} + func (g GeneratorImpl) executeStreamUpstreams(conf dataplane.Configuration) []executeResult { upstreams := g.createStreamUpstreams(conf.StreamUpstreams) result := executeResult{ dest: streamConfigFile, - data: helpers.MustExecuteTemplate(upstreamsTemplate, upstreams), + data: helpers.MustExecuteTemplate(streamUpstreamsTemplate, upstreams), } return []executeResult{result} @@ -87,12 +121,15 @@ func (g GeneratorImpl) createStreamUpstream(up dataplane.Upstream) stream.Upstre } } -func (g GeneratorImpl) createUpstreams(upstreams []dataplane.Upstream) []http.Upstream { +func (g GeneratorImpl) createUpstreams( + upstreams []dataplane.Upstream, + generator policies.UpstreamSettingsProcessor, +) []http.Upstream { // capacity is the number of upstreams + 1 for the invalid backend ref upstream ups := make([]http.Upstream, 0, len(upstreams)+1) for _, u := range upstreams { - ups = append(ups, g.createUpstream(u)) + ups = append(ups, g.createUpstream(u, generator)) } ups = append(ups, createInvalidBackendRefUpstream()) @@ -100,12 +137,21 @@ func (g GeneratorImpl) createUpstreams(upstreams []dataplane.Upstream) []http.Up return ups } -func (g GeneratorImpl) createUpstream(up dataplane.Upstream) http.Upstream { +func (g GeneratorImpl) createUpstream( + up dataplane.Upstream, + generator policies.UpstreamSettingsProcessor, +) http.Upstream { + upstreamPolicySettings := generator.Process(up.Policies) + zoneSize := ossZoneSize if g.plus { zoneSize = plusZoneSize } + if upstreamPolicySettings.ZoneSize != "" { + zoneSize = upstreamPolicySettings.ZoneSize + } + if len(up.Endpoints) == 0 { return http.Upstream{ Name: up.Name, @@ -130,9 +176,13 @@ func (g GeneratorImpl) createUpstream(up dataplane.Upstream) http.Upstream { } return http.Upstream{ - Name: up.Name, - ZoneSize: zoneSize, - Servers: upstreamServers, + Name: up.Name, + ZoneSize: zoneSize, + Servers: upstreamServers, + KeepAliveConnections: upstreamPolicySettings.KeepAliveConnections, + KeepAliveRequests: upstreamPolicySettings.KeepAliveRequests, + KeepAliveTime: upstreamPolicySettings.KeepAliveTime, + KeepAliveTimeout: upstreamPolicySettings.KeepAliveTimeout, } } diff --git a/internal/mode/static/nginx/config/upstreams_template.go b/internal/mode/static/nginx/config/upstreams_template.go index a04915bec8..b879ba247c 100644 --- a/internal/mode/static/nginx/config/upstreams_template.go +++ b/internal/mode/static/nginx/config/upstreams_template.go @@ -7,6 +7,32 @@ package config // https://github.com/nginxinc/nginx-gateway-fabric/issues/483 const upstreamsTemplateText = ` {{ range $u := . }} +upstream {{ $u.Name }} { + random two least_conn; + {{ if $u.ZoneSize -}} + zone {{ $u.Name }} {{ $u.ZoneSize }}; + {{ end -}} + {{ range $server := $u.Servers }} + server {{ $server.Address }}; + {{- end }} + {{ if $u.KeepAliveConnections -}} + keepalive {{ $u.KeepAliveConnections }}; + {{- end }} + {{ if $u.KeepAliveRequests -}} + keepalive_requests {{ $u.KeepAliveRequests }}; + {{- end }} + {{ if $u.KeepAliveTime -}} + keepalive_time {{ $u.KeepAliveTime }}; + {{- end }} + {{ if $u.KeepAliveTimeout -}} + keepalive_timeout {{ $u.KeepAliveTimeout }}; + {{- end }} +} +{{ end -}} +` + +const streamUpstreamsTemplateText = ` +{{ range $u := . }} upstream {{ $u.Name }} { random two least_conn; {{ if $u.ZoneSize -}} diff --git a/internal/mode/static/nginx/config/upstreams_test.go b/internal/mode/static/nginx/config/upstreams_test.go index 5b3a8268a3..001ca9e4ee 100644 --- a/internal/mode/static/nginx/config/upstreams_test.go +++ b/internal/mode/static/nginx/config/upstreams_test.go @@ -4,8 +4,13 @@ import ( "testing" . "github.com/onsi/gomega" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1" + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/http" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies/upstreamsettings" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/stream" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/dataplane" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/resolver" @@ -47,6 +52,32 @@ func TestExecuteUpstreams(t *testing.T) { }, }, }, + { + Name: "up5-usp", + Endpoints: []resolver.Endpoint{ + { + Address: "12.0.0.0", + Port: 80, + }, + }, + Policies: []policies.Policy{ + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp", + Namespace: "test", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + ZoneSize: helpers.GetPointer[ngfAPI.Size]("2m"), + KeepAlive: helpers.GetPointer(ngfAPI.UpstreamKeepAlive{ + Connections: helpers.GetPointer(int32(1)), + Requests: helpers.GetPointer(int32(1)), + Time: helpers.GetPointer[ngfAPI.Duration]("5s"), + Timeout: helpers.GetPointer[ngfAPI.Duration]("10s"), + }), + }, + }, + }, + }, } expectedSubStrings := []string{ @@ -54,21 +85,32 @@ func TestExecuteUpstreams(t *testing.T) { "upstream up2", "upstream up3", "upstream up4-ipv6", + "upstream up5-usp", "upstream invalid-backend-ref", + "server 10.0.0.0:80;", "server 11.0.0.0:80;", "server [2001:db8::1]:80", + "server 12.0.0.0:80;", "server unix:/var/run/nginx/nginx-503-server.sock;", + + "keepalive 1;", + "keepalive_requests 1;", + "keepalive_time 5s;", + "keepalive_timeout 10s;", + "zone up5-usp 2m;", } - upstreamResults := gen.executeUpstreams(dataplane.Configuration{Upstreams: stateUpstreams}) + upstreams := gen.createUpstreams(stateUpstreams, upstreamsettings.NewProcessor()) + + upstreamResults := gen.executeUpstreams(upstreams) g := NewWithT(t) g.Expect(upstreamResults).To(HaveLen(1)) - upstreams := string(upstreamResults[0].data) + nginxUpstreams := string(upstreamResults[0].data) g.Expect(upstreamResults[0].dest).To(Equal(httpConfigFile)) for _, expSubString := range expectedSubStrings { - g.Expect(upstreams).To(ContainSubstring(expSubString)) + g.Expect(nginxUpstreams).To(ContainSubstring(expSubString)) } } @@ -116,6 +158,32 @@ func TestCreateUpstreams(t *testing.T) { }, }, }, + { + Name: "up5-usp", + Endpoints: []resolver.Endpoint{ + { + Address: "12.0.0.0", + Port: 80, + }, + }, + Policies: []policies.Policy{ + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp", + Namespace: "test", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + ZoneSize: helpers.GetPointer[ngfAPI.Size]("2m"), + KeepAlive: helpers.GetPointer(ngfAPI.UpstreamKeepAlive{ + Connections: helpers.GetPointer(int32(1)), + Requests: helpers.GetPointer(int32(1)), + Time: helpers.GetPointer[ngfAPI.Duration]("5s"), + Timeout: helpers.GetPointer[ngfAPI.Duration]("10s"), + }), + }, + }, + }, + }, } expUpstreams := []http.Upstream{ @@ -161,6 +229,19 @@ func TestCreateUpstreams(t *testing.T) { }, }, }, + { + Name: "up5-usp", + ZoneSize: "2m", + Servers: []http.UpstreamServer{ + { + Address: "12.0.0.0:80", + }, + }, + KeepAliveConnections: 1, + KeepAliveRequests: 1, + KeepAliveTime: "5s", + KeepAliveTimeout: "10s", + }, { Name: invalidBackendRef, Servers: []http.UpstreamServer{ @@ -172,7 +253,7 @@ func TestCreateUpstreams(t *testing.T) { } g := NewWithT(t) - result := gen.createUpstreams(stateUpstreams) + result := gen.createUpstreams(stateUpstreams, upstreamsettings.NewProcessor()) g.Expect(result).To(Equal(expUpstreams)) } @@ -273,13 +354,107 @@ func TestCreateUpstream(t *testing.T) { }, msg: "endpoint ipv6", }, + { + stateUpstream: dataplane.Upstream{ + Name: "single upstreamSettingsPolicy", + Endpoints: []resolver.Endpoint{ + { + Address: "10.0.0.1", + Port: 80, + }, + }, + Policies: []policies.Policy{ + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp", + Namespace: "test", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + ZoneSize: helpers.GetPointer[ngfAPI.Size]("2m"), + KeepAlive: helpers.GetPointer(ngfAPI.UpstreamKeepAlive{ + Connections: helpers.GetPointer(int32(1)), + Requests: helpers.GetPointer(int32(1)), + Time: helpers.GetPointer[ngfAPI.Duration]("5s"), + Timeout: helpers.GetPointer[ngfAPI.Duration]("10s"), + }), + }, + }, + }, + }, + expectedUpstream: http.Upstream{ + Name: "single upstreamSettingsPolicy", + ZoneSize: "2m", + Servers: []http.UpstreamServer{ + { + Address: "10.0.0.1:80", + }, + }, + KeepAliveConnections: 1, + KeepAliveRequests: 1, + KeepAliveTime: "5s", + KeepAliveTimeout: "10s", + }, + msg: "single upstreamSettingsPolicy", + }, + { + stateUpstream: dataplane.Upstream{ + Name: "multiple upstreamSettingsPolicies", + Endpoints: []resolver.Endpoint{ + { + Address: "10.0.0.1", + Port: 80, + }, + }, + Policies: []policies.Policy{ + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp1", + Namespace: "test", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + ZoneSize: helpers.GetPointer[ngfAPI.Size]("2m"), + KeepAlive: helpers.GetPointer(ngfAPI.UpstreamKeepAlive{ + Time: helpers.GetPointer[ngfAPI.Duration]("5s"), + Timeout: helpers.GetPointer[ngfAPI.Duration]("10s"), + }), + }, + }, + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp2", + Namespace: "test", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + KeepAlive: helpers.GetPointer(ngfAPI.UpstreamKeepAlive{ + Connections: helpers.GetPointer(int32(1)), + Requests: helpers.GetPointer(int32(1)), + }), + }, + }, + }, + }, + expectedUpstream: http.Upstream{ + Name: "multiple upstreamSettingsPolicies", + ZoneSize: "2m", + Servers: []http.UpstreamServer{ + { + Address: "10.0.0.1:80", + }, + }, + KeepAliveConnections: 1, + KeepAliveRequests: 1, + KeepAliveTime: "5s", + KeepAliveTimeout: "10s", + }, + msg: "multiple upstreamSettingsPolicies", + }, } for _, test := range tests { t.Run(test.msg, func(t *testing.T) { t.Parallel() g := NewWithT(t) - result := gen.createUpstream(test.stateUpstream) + result := gen.createUpstream(test.stateUpstream, upstreamsettings.NewProcessor()) g.Expect(result).To(Equal(test.expectedUpstream)) }) } @@ -308,7 +483,7 @@ func TestCreateUpstreamPlus(t *testing.T) { }, } - result := gen.createUpstream(stateUpstream) + result := gen.createUpstream(stateUpstream, upstreamsettings.NewProcessor()) g := NewWithT(t) g.Expect(result).To(Equal(expectedUpstream)) @@ -505,3 +680,119 @@ func TestCreateStreamUpstreamPlus(t *testing.T) { g := NewWithT(t) g.Expect(result).To(Equal(expectedUpstream)) } + +func TestCreateUpstreamMap(t *testing.T) { + t.Parallel() + gen := GeneratorImpl{} + + up1 := http.Upstream{ + Name: "up1", + } + + up2 := http.Upstream{ + Name: "up2", + } + + up3 := http.Upstream{ + Name: "up3", + } + + upstreamMap := gen.createUpstreamMap([]http.Upstream{up1, up2, up3}) + expUpstreamMap := UpstreamMap{ + nameToUpstream: map[string]http.Upstream{ + "up1": up1, + "up2": up2, + "up3": up3, + }, + } + + g := NewWithT(t) + g.Expect(upstreamMap).To(Equal(expUpstreamMap)) +} + +func TestKeepAliveEnabled(t *testing.T) { + t.Parallel() + + tests := []struct { + msg string + upstream http.Upstream + expKeepAliveEnabled bool + }{ + { + msg: "upstream with all keepAlive fields set", + upstream: http.Upstream{ + Name: "upAllKeepAliveFieldsSet", + KeepAliveConnections: 1, + KeepAliveRequests: 1, + KeepAliveTime: "5s", + KeepAliveTimeout: "10s", + }, + expKeepAliveEnabled: true, + }, + { + msg: "upstream with keepAlive connection field set", + upstream: http.Upstream{ + Name: "upKeepAliveConnectionsSet", + KeepAliveConnections: 1, + }, + expKeepAliveEnabled: true, + }, + { + msg: "upstream with keepAlive requests field set", + upstream: http.Upstream{ + Name: "upKeepAliveRequestsSet", + KeepAliveRequests: 1, + }, + expKeepAliveEnabled: true, + }, + { + msg: "upstream with keepAlive time field set", + upstream: http.Upstream{ + Name: "upKeepAliveTimeSet", + KeepAliveTime: "5s", + }, + expKeepAliveEnabled: true, + }, + { + msg: "upstream with keepAlive timeout field set", + upstream: http.Upstream{ + Name: "upKeepAliveTimeoutSet", + KeepAliveTimeout: "10s", + }, + expKeepAliveEnabled: true, + }, + { + msg: "upstream with no keepAlive fields set", + upstream: http.Upstream{ + Name: "upNoKeepAliveFieldsSet", + }, + expKeepAliveEnabled: false, + }, + { + msg: "upstream with keepAlive fields set to empty values", + upstream: http.Upstream{ + Name: "upNoKeepAliveFieldsSet", + KeepAliveConnections: 0, + KeepAliveRequests: 0, + KeepAliveTime: "", + KeepAliveTimeout: "", + }, + expKeepAliveEnabled: false, + }, + } + + for _, test := range tests { + t.Run(test.msg, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + upstreamMap := UpstreamMap{ + nameToUpstream: map[string]http.Upstream{ + test.upstream.Name: test.upstream, + }, + } + + g.Expect(upstreamMap.keepAliveEnabled(test.upstream.Name)).To(Equal(test.expKeepAliveEnabled)) + }) + } +} diff --git a/internal/mode/static/state/dataplane/types.go b/internal/mode/static/state/dataplane/types.go index 274897c007..974efbb142 100644 --- a/internal/mode/static/state/dataplane/types.go +++ b/internal/mode/static/state/dataplane/types.go @@ -111,6 +111,8 @@ type Upstream struct { ErrorMsg string // Endpoints are the endpoints of the Upstream. Endpoints []resolver.Endpoint + // Policies contains the list of policies that are applied to this Upstream. + Policies []policies.Policy } // SSL is the SSL configuration for a server. From d39f4c0ca7dc83d0d5edfa9f5147c662721cfc1b Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Mon, 9 Dec 2024 13:56:53 -0800 Subject: [PATCH 02/23] Fix naming conventions, test messages, and add more comments --- .../static/nginx/config/policies/processor.go | 16 +++++++++++----- .../policies/upstreamsettings/processor_test.go | 10 +++++----- internal/mode/static/nginx/config/upstreams.go | 8 ++++---- 3 files changed, 20 insertions(+), 14 deletions(-) diff --git a/internal/mode/static/nginx/config/policies/processor.go b/internal/mode/static/nginx/config/policies/processor.go index d1b70247d8..6f50c71f96 100644 --- a/internal/mode/static/nginx/config/policies/processor.go +++ b/internal/mode/static/nginx/config/policies/processor.go @@ -1,15 +1,21 @@ package policies -// UpstreamSettingsProcessor defines an interface for an UpstreamSettingsPolicy to implement the process function. +// UpstreamSettingsProcessor defines an interface for an UpstreamSettingsPolicy processor' +// to implement the process function. type UpstreamSettingsProcessor interface { Process(policies []Policy) UpstreamSettings } // UpstreamSettings contains settings from UpstreamSettingsPolicy. type UpstreamSettings struct { - ZoneSize string - KeepAliveTime string - KeepAliveTimeout string + // ZoneSize is the zone size setting. + ZoneSize string + // KeepAliveTime is the keep-alive time setting. + KeepAliveTime string + // KeepAliveTimeout is the keep-alive timeout setting. + KeepAliveTimeout string + // KeepAliveConnections is the keep-alive connections setting. KeepAliveConnections int32 - KeepAliveRequests int32 + // KeepAliveRequests is the keep-alive requests setting. + KeepAliveRequests int32 } diff --git a/internal/mode/static/nginx/config/policies/upstreamsettings/processor_test.go b/internal/mode/static/nginx/config/policies/upstreamsettings/processor_test.go index 7bd7159acb..2fc9e115ae 100644 --- a/internal/mode/static/nginx/config/policies/upstreamsettings/processor_test.go +++ b/internal/mode/static/nginx/config/policies/upstreamsettings/processor_test.go @@ -47,7 +47,7 @@ func TestProcess(t *testing.T) { }, }, { - name: "zoneSize set", + name: "zone size set", policies: []policies.Policy{ &ngfAPI.UpstreamSettingsPolicy{ ObjectMeta: metav1.ObjectMeta{ @@ -64,7 +64,7 @@ func TestProcess(t *testing.T) { }, }, { - name: "keepAlive Connections set", + name: "keep alive connections set", policies: []policies.Policy{ &ngfAPI.UpstreamSettingsPolicy{ ObjectMeta: metav1.ObjectMeta{ @@ -83,7 +83,7 @@ func TestProcess(t *testing.T) { }, }, { - name: "keepAlive Requests set", + name: "keep alive requests set", policies: []policies.Policy{ &ngfAPI.UpstreamSettingsPolicy{ ObjectMeta: metav1.ObjectMeta{ @@ -102,7 +102,7 @@ func TestProcess(t *testing.T) { }, }, { - name: "keepAlive Time set", + name: "keep alive time set", policies: []policies.Policy{ &ngfAPI.UpstreamSettingsPolicy{ ObjectMeta: metav1.ObjectMeta{ @@ -121,7 +121,7 @@ func TestProcess(t *testing.T) { }, }, { - name: "keepAlive Timeout set", + name: "keep alive timeout set", policies: []policies.Policy{ &ngfAPI.UpstreamSettingsPolicy{ ObjectMeta: metav1.ObjectMeta{ diff --git a/internal/mode/static/nginx/config/upstreams.go b/internal/mode/static/nginx/config/upstreams.go index 289bf4bd24..0cecaf5955 100644 --- a/internal/mode/static/nginx/config/upstreams.go +++ b/internal/mode/static/nginx/config/upstreams.go @@ -123,13 +123,13 @@ func (g GeneratorImpl) createStreamUpstream(up dataplane.Upstream) stream.Upstre func (g GeneratorImpl) createUpstreams( upstreams []dataplane.Upstream, - generator policies.UpstreamSettingsProcessor, + processor policies.UpstreamSettingsProcessor, ) []http.Upstream { // capacity is the number of upstreams + 1 for the invalid backend ref upstream ups := make([]http.Upstream, 0, len(upstreams)+1) for _, u := range upstreams { - ups = append(ups, g.createUpstream(u, generator)) + ups = append(ups, g.createUpstream(u, processor)) } ups = append(ups, createInvalidBackendRefUpstream()) @@ -139,9 +139,9 @@ func (g GeneratorImpl) createUpstreams( func (g GeneratorImpl) createUpstream( up dataplane.Upstream, - generator policies.UpstreamSettingsProcessor, + processor policies.UpstreamSettingsProcessor, ) http.Upstream { - upstreamPolicySettings := generator.Process(up.Policies) + upstreamPolicySettings := processor.Process(up.Policies) zoneSize := ossZoneSize if g.plus { From 1053ca2bc6cd5c4d812e446b5f461e67d32bc0cf Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Mon, 9 Dec 2024 13:57:41 -0800 Subject: [PATCH 03/23] Remove typo --- internal/mode/static/nginx/config/policies/processor.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/mode/static/nginx/config/policies/processor.go b/internal/mode/static/nginx/config/policies/processor.go index 6f50c71f96..7a2317208f 100644 --- a/internal/mode/static/nginx/config/policies/processor.go +++ b/internal/mode/static/nginx/config/policies/processor.go @@ -1,6 +1,6 @@ package policies -// UpstreamSettingsProcessor defines an interface for an UpstreamSettingsPolicy processor' +// UpstreamSettingsProcessor defines an interface for an UpstreamSettingsPolicy processor // to implement the process function. type UpstreamSettingsProcessor interface { Process(policies []Policy) UpstreamSettings From 5f7eee794ee4d8c2298bf645a5a428575ee43adb Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Mon, 9 Dec 2024 14:14:34 -0800 Subject: [PATCH 04/23] Change funtionality so connection header value is empty when keep alive settings are enabled --- internal/mode/static/nginx/config/servers.go | 14 +++++++------ .../mode/static/nginx/config/servers_test.go | 21 +++++++++++++------ 2 files changed, 23 insertions(+), 12 deletions(-) diff --git a/internal/mode/static/nginx/config/servers.go b/internal/mode/static/nginx/config/servers.go index 559ae202e6..4774c8a392 100644 --- a/internal/mode/static/nginx/config/servers.go +++ b/internal/mode/static/nginx/config/servers.go @@ -727,11 +727,16 @@ func generateProxySetHeaders( um UpstreamMap, backends []dataplane.Backend, ) []http.Header { - var keepAlive bool + modifiedConnectionHeader := connectionHeader for _, backend := range backends { if um.keepAliveEnabled(backend.UpstreamName) { - keepAlive = true + // if keep-alive settings are enabled on any upstream, the connection header value + // must be empty for the location + modifiedConnectionHeader = http.Header{ + Name: connectionHeader.Name, + Value: "", + } break } } @@ -741,10 +746,7 @@ func generateProxySetHeaders( extraHeaders = append(extraHeaders, authorityHeader) } else { extraHeaders = append(extraHeaders, upgradeHeader) - - if !keepAlive { - extraHeaders = append(extraHeaders, connectionHeader) - } + extraHeaders = append(extraHeaders, modifiedConnectionHeader) } headers := createBaseProxySetHeaders(extraHeaders...) diff --git a/internal/mode/static/nginx/config/servers_test.go b/internal/mode/static/nginx/config/servers_test.go index ec271bbff2..7ffc67fe56 100644 --- a/internal/mode/static/nginx/config/servers_test.go +++ b/internal/mode/static/nginx/config/servers_test.go @@ -2971,8 +2971,11 @@ func TestGenerateProxySetHeaders(t *testing.T) { }, grpcBaseHeaders...), }, { - msg: "upstream with keepAlive enabled", - expectedHeaders: createBaseProxySetHeaders(upgradeHeader), + msg: "upstream with keepAlive enabled", + expectedHeaders: append(createBaseProxySetHeaders(upgradeHeader), http.Header{ + Name: connectionHeader.Name, + Value: "", + }), upstreamMap: UpstreamMap{ nameToUpstream: map[string]http.Upstream{ "upstream": { @@ -2988,8 +2991,11 @@ func TestGenerateProxySetHeaders(t *testing.T) { }, }, { - msg: "multiple upstreams with keepAlive enabled", - expectedHeaders: createBaseProxySetHeaders(upgradeHeader), + msg: "multiple upstreams with keepAlive enabled", + expectedHeaders: append(createBaseProxySetHeaders(upgradeHeader), http.Header{ + Name: connectionHeader.Name, + Value: "", + }), upstreamMap: UpstreamMap{ nameToUpstream: map[string]http.Upstream{ "upstream1": { @@ -3019,8 +3025,11 @@ func TestGenerateProxySetHeaders(t *testing.T) { }, }, { - msg: "mix of upstreams with keepAlive enabled and disabled", - expectedHeaders: createBaseProxySetHeaders(upgradeHeader), + msg: "mix of upstreams with keepAlive enabled and disabled", + expectedHeaders: append(createBaseProxySetHeaders(upgradeHeader), http.Header{ + Name: connectionHeader.Name, + Value: "", + }), upstreamMap: UpstreamMap{ nameToUpstream: map[string]http.Upstream{ "upstream1": { From a420810bed544c66cd82dcb1e480d77eb94c666b Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Mon, 9 Dec 2024 14:36:03 -0800 Subject: [PATCH 05/23] Update generated files --- .../bases/gateway.nginx.org_upstreamsettingspolicies.yaml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/config/crd/bases/gateway.nginx.org_upstreamsettingspolicies.yaml b/config/crd/bases/gateway.nginx.org_upstreamsettingspolicies.yaml index dbe0462862..992af79932 100644 --- a/config/crd/bases/gateway.nginx.org_upstreamsettingspolicies.yaml +++ b/config/crd/bases/gateway.nginx.org_upstreamsettingspolicies.yaml @@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1 kind: CustomResourceDefinition metadata: annotations: - controller-gen.kubebuilder.io/version: v0.16.2 + controller-gen.kubebuilder.io/version: v0.16.5 labels: gateway.networking.k8s.io/policy: direct name: upstreamsettingspolicies.gateway.nginx.org @@ -76,13 +76,13 @@ spec: Time defines the maximum time during which requests can be processed through one keep-alive connection. After this time is reached, the connection is closed following the subsequent request processing. Directive: https://nginx.org/en/docs/http/ngx_http_upstream_module.html#keepalive_time - pattern: ^\d{1,4}(ms|s)?$ + pattern: ^[0-9]{1,4}(ms|s|m|h)?$ type: string timeout: description: |- Timeout defines the keep-alive timeout for upstreams. Directive: https://nginx.org/en/docs/http/ngx_http_upstream_module.html#keepalive_timeout - pattern: ^\d{1,4}(ms|s)?$ + pattern: ^[0-9]{1,4}(ms|s|m|h)?$ type: string type: object targetRefs: From b188ef66983e1fb616730500395d0e891eb62e1f Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Tue, 10 Dec 2024 10:42:02 -0800 Subject: [PATCH 06/23] Add feedback for upstreams template --- .../static/nginx/config/upstreams_template.go | 26 ++++++++++--------- 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/internal/mode/static/nginx/config/upstreams_template.go b/internal/mode/static/nginx/config/upstreams_template.go index b879ba247c..69a25def63 100644 --- a/internal/mode/static/nginx/config/upstreams_template.go +++ b/internal/mode/static/nginx/config/upstreams_template.go @@ -8,6 +8,8 @@ package config const upstreamsTemplateText = ` {{ range $u := . }} upstream {{ $u.Name }} { + # if there is a keepalive directive present, it is necessary to activate the load balancing method before the + # keepalive directive random two least_conn; {{ if $u.ZoneSize -}} zone {{ $u.Name }} {{ $u.ZoneSize }}; @@ -15,18 +17,18 @@ upstream {{ $u.Name }} { {{ range $server := $u.Servers }} server {{ $server.Address }}; {{- end }} - {{ if $u.KeepAliveConnections -}} - keepalive {{ $u.KeepAliveConnections }}; - {{- end }} - {{ if $u.KeepAliveRequests -}} - keepalive_requests {{ $u.KeepAliveRequests }}; - {{- end }} - {{ if $u.KeepAliveTime -}} - keepalive_time {{ $u.KeepAliveTime }}; - {{- end }} - {{ if $u.KeepAliveTimeout -}} - keepalive_timeout {{ $u.KeepAliveTimeout }}; - {{- end }} + {{ if $u.KeepAliveConnections -}} + keepalive {{ $u.KeepAliveConnections }}; + {{- end }} + {{ if $u.KeepAliveRequests -}} + keepalive_requests {{ $u.KeepAliveRequests }}; + {{- end }} + {{ if $u.KeepAliveTime -}} + keepalive_time {{ $u.KeepAliveTime }}; + {{- end }} + {{ if $u.KeepAliveTimeout -}} + keepalive_timeout {{ $u.KeepAliveTimeout }}; + {{- end }} } {{ end -}} ` From 939e78284a6f664f7490acda1a5da66c753e04eb Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Tue, 10 Dec 2024 10:47:54 -0800 Subject: [PATCH 07/23] Adjust comment in upstreams template --- internal/mode/static/nginx/config/upstreams_template.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/internal/mode/static/nginx/config/upstreams_template.go b/internal/mode/static/nginx/config/upstreams_template.go index 69a25def63..a855235317 100644 --- a/internal/mode/static/nginx/config/upstreams_template.go +++ b/internal/mode/static/nginx/config/upstreams_template.go @@ -8,8 +8,7 @@ package config const upstreamsTemplateText = ` {{ range $u := . }} upstream {{ $u.Name }} { - # if there is a keepalive directive present, it is necessary to activate the load balancing method before the - # keepalive directive + # if the keepalive directive us present, it is necessary to activate the load balancing method before the directive random two least_conn; {{ if $u.ZoneSize -}} zone {{ $u.Name }} {{ $u.ZoneSize }}; From 032a6bc01ba9e827b93073854166511116761e35 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Tue, 10 Dec 2024 10:53:31 -0800 Subject: [PATCH 08/23] Add feedback for naming of variables and fields --- .../mode/static/nginx/config/generator.go | 6 ++-- internal/mode/static/nginx/config/servers.go | 18 +++++------ .../mode/static/nginx/config/servers_test.go | 30 +++++++++---------- 3 files changed, 27 insertions(+), 27 deletions(-) diff --git a/internal/mode/static/nginx/config/generator.go b/internal/mode/static/nginx/config/generator.go index ce27c012de..977bbcc73f 100644 --- a/internal/mode/static/nginx/config/generator.go +++ b/internal/mode/static/nginx/config/generator.go @@ -133,10 +133,10 @@ func (g GeneratorImpl) executeConfigTemplates( ) []file.File { fileBytes := make(map[string][]byte) - upstreams := g.createUpstreams(conf.Upstreams, upstreamsettings.NewProcessor()) - upstreamMap := g.createUpstreamMap(upstreams) + httpUpstreams := g.createUpstreams(conf.Upstreams, upstreamsettings.NewProcessor()) + upstreamMap := g.createUpstreamMap(httpUpstreams) - for _, execute := range g.getExecuteFuncs(generator, upstreams, upstreamMap) { + for _, execute := range g.getExecuteFuncs(generator, httpUpstreams, upstreamMap) { results := execute(conf) for _, res := range results { fileBytes[res.dest] = append(fileBytes[res.dest], res.data...) diff --git a/internal/mode/static/nginx/config/servers.go b/internal/mode/static/nginx/config/servers.go index 4774c8a392..78ec9c18ba 100644 --- a/internal/mode/static/nginx/config/servers.go +++ b/internal/mode/static/nginx/config/servers.go @@ -23,17 +23,17 @@ const ( rootPath = "/" ) -var authorityHeader = http.Header{ +var grpcAuthorityHeader = http.Header{ Name: "Authority", Value: "$gw_api_compliant_host", } -var connectionHeader = http.Header{ +var httpConnectionHeader = http.Header{ Name: "Connection", Value: "$connection_upgrade", } -var upgradeHeader = http.Header{ +var httpUpgradeHeader = http.Header{ Name: "Upgrade", Value: "$http_upgrade", } @@ -727,14 +727,14 @@ func generateProxySetHeaders( um UpstreamMap, backends []dataplane.Backend, ) []http.Header { - modifiedConnectionHeader := connectionHeader + modifiedConnectionHeader := httpConnectionHeader for _, backend := range backends { if um.keepAliveEnabled(backend.UpstreamName) { // if keep-alive settings are enabled on any upstream, the connection header value // must be empty for the location modifiedConnectionHeader = http.Header{ - Name: connectionHeader.Name, + Name: httpConnectionHeader.Name, Value: "", } break @@ -743,9 +743,9 @@ func generateProxySetHeaders( var extraHeaders []http.Header if grpc { - extraHeaders = append(extraHeaders, authorityHeader) + extraHeaders = append(extraHeaders, grpcAuthorityHeader) } else { - extraHeaders = append(extraHeaders, upgradeHeader) + extraHeaders = append(extraHeaders, httpUpgradeHeader) extraHeaders = append(extraHeaders, modifiedConnectionHeader) } @@ -869,7 +869,7 @@ func getRewriteClientIPSettings(rewriteIPConfig dataplane.RewriteClientIPSetting } } -func createBaseProxySetHeaders(headers ...http.Header) []http.Header { +func createBaseProxySetHeaders(extraHeaders ...http.Header) []http.Header { baseHeaders := []http.Header{ { Name: "Host", @@ -897,7 +897,7 @@ func createBaseProxySetHeaders(headers ...http.Header) []http.Header { }, } - baseHeaders = append(baseHeaders, headers...) + baseHeaders = append(baseHeaders, extraHeaders...) return baseHeaders } diff --git a/internal/mode/static/nginx/config/servers_test.go b/internal/mode/static/nginx/config/servers_test.go index 7ffc67fe56..c28d58f533 100644 --- a/internal/mode/static/nginx/config/servers_test.go +++ b/internal/mode/static/nginx/config/servers_test.go @@ -18,8 +18,8 @@ import ( ) var ( - httpBaseHeaders = createBaseProxySetHeaders(upgradeHeader, connectionHeader) - grpcBaseHeaders = createBaseProxySetHeaders(authorityHeader) + httpBaseHeaders = createBaseProxySetHeaders(httpUpgradeHeader, httpConnectionHeader) + grpcBaseHeaders = createBaseProxySetHeaders(grpcAuthorityHeader) ) func TestExecuteServers(t *testing.T) { @@ -2972,8 +2972,8 @@ func TestGenerateProxySetHeaders(t *testing.T) { }, { msg: "upstream with keepAlive enabled", - expectedHeaders: append(createBaseProxySetHeaders(upgradeHeader), http.Header{ - Name: connectionHeader.Name, + expectedHeaders: append(createBaseProxySetHeaders(httpUpgradeHeader), http.Header{ + Name: httpConnectionHeader.Name, Value: "", }), upstreamMap: UpstreamMap{ @@ -2992,8 +2992,8 @@ func TestGenerateProxySetHeaders(t *testing.T) { }, { msg: "multiple upstreams with keepAlive enabled", - expectedHeaders: append(createBaseProxySetHeaders(upgradeHeader), http.Header{ - Name: connectionHeader.Name, + expectedHeaders: append(createBaseProxySetHeaders(httpUpgradeHeader), http.Header{ + Name: httpConnectionHeader.Name, Value: "", }), upstreamMap: UpstreamMap{ @@ -3026,8 +3026,8 @@ func TestGenerateProxySetHeaders(t *testing.T) { }, { msg: "mix of upstreams with keepAlive enabled and disabled", - expectedHeaders: append(createBaseProxySetHeaders(upgradeHeader), http.Header{ - Name: connectionHeader.Name, + expectedHeaders: append(createBaseProxySetHeaders(httpUpgradeHeader), http.Header{ + Name: httpConnectionHeader.Name, Value: "", }), upstreamMap: UpstreamMap{ @@ -3067,7 +3067,7 @@ func TestGenerateProxySetHeaders(t *testing.T) { func TestCreateBaseProxySetHeaders(t *testing.T) { t.Parallel() - baseHeaders := []http.Header{ + expBaseHeaders := []http.Header{ { Name: "Host", Value: "$gw_api_compliant_host", @@ -3102,22 +3102,22 @@ func TestCreateBaseProxySetHeaders(t *testing.T) { { msg: "no additional headers", additionalHeaders: []http.Header{}, - expBaseHeaders: baseHeaders, + expBaseHeaders: expBaseHeaders, }, { msg: "single additional headers", additionalHeaders: []http.Header{ - authorityHeader, + grpcAuthorityHeader, }, - expBaseHeaders: append(baseHeaders, authorityHeader), + expBaseHeaders: append(expBaseHeaders, grpcAuthorityHeader), }, { msg: "multiple additional headers", additionalHeaders: []http.Header{ - connectionHeader, - upgradeHeader, + httpConnectionHeader, + httpUpgradeHeader, }, - expBaseHeaders: append(baseHeaders, connectionHeader, upgradeHeader), + expBaseHeaders: append(expBaseHeaders, httpConnectionHeader, httpUpgradeHeader), }, } From 7c403dd024e0cf348db49363e7d8bd88e2696744 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Tue, 10 Dec 2024 10:57:27 -0800 Subject: [PATCH 09/23] Add upstreams test case for empty policy --- .../static/nginx/config/upstreams_test.go | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/internal/mode/static/nginx/config/upstreams_test.go b/internal/mode/static/nginx/config/upstreams_test.go index 001ca9e4ee..0bde1ace14 100644 --- a/internal/mode/static/nginx/config/upstreams_test.go +++ b/internal/mode/static/nginx/config/upstreams_test.go @@ -448,6 +448,35 @@ func TestCreateUpstream(t *testing.T) { }, msg: "multiple upstreamSettingsPolicies", }, + { + stateUpstream: dataplane.Upstream{ + Name: "empty upstreamSettingsPolicies", + Endpoints: []resolver.Endpoint{ + { + Address: "10.0.0.1", + Port: 80, + }, + }, + Policies: []policies.Policy{ + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp1", + Namespace: "test", + }, + }, + }, + }, + expectedUpstream: http.Upstream{ + Name: "empty upstreamSettingsPolicies", + ZoneSize: ossZoneSize, + Servers: []http.UpstreamServer{ + { + Address: "10.0.0.1:80", + }, + }, + }, + msg: "empty upstreamSettingsPolicies", + }, } for _, test := range tests { From 3618fbd86b583103d1d897522055a291f0af9d6b Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Tue, 10 Dec 2024 11:15:27 -0800 Subject: [PATCH 10/23] Add processor test case for multiple policies --- .../upstreamsettings/processor_test.go | 65 +++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/internal/mode/static/nginx/config/policies/upstreamsettings/processor_test.go b/internal/mode/static/nginx/config/policies/upstreamsettings/processor_test.go index 2fc9e115ae..3e735cce22 100644 --- a/internal/mode/static/nginx/config/policies/upstreamsettings/processor_test.go +++ b/internal/mode/static/nginx/config/policies/upstreamsettings/processor_test.go @@ -152,6 +152,71 @@ func TestProcess(t *testing.T) { }, expUpstreamSettings: policies.UpstreamSettings{}, }, + { + name: "multiple policies", + policies: []policies.Policy{ + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp-zonesize", + Namespace: "test", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + ZoneSize: helpers.GetPointer[ngfAPI.Size]("2m"), + }, + }, + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp-keepalive-connections", + Namespace: "test", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + KeepAlive: helpers.GetPointer(ngfAPI.UpstreamKeepAlive{ + Connections: helpers.GetPointer(int32(1)), + }), + }, + }, + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp-keepalive-requests", + Namespace: "test", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + KeepAlive: helpers.GetPointer(ngfAPI.UpstreamKeepAlive{ + Requests: helpers.GetPointer(int32(1)), + }), + }, + }, + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp-keepalive-time", + Namespace: "test", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + KeepAlive: helpers.GetPointer(ngfAPI.UpstreamKeepAlive{ + Time: helpers.GetPointer[ngfAPI.Duration]("5s"), + }), + }, + }, + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp-keepalive-timeout", + Namespace: "test", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + KeepAlive: helpers.GetPointer(ngfAPI.UpstreamKeepAlive{ + Timeout: helpers.GetPointer[ngfAPI.Duration]("10s"), + }), + }, + }, + }, + expUpstreamSettings: policies.UpstreamSettings{ + ZoneSize: "2m", + KeepAliveConnections: 1, + KeepAliveRequests: 1, + KeepAliveTime: "5s", + KeepAliveTimeout: "10s", + }, + }, } for _, test := range tests { From 042cce5cafdab3537c5b25bacf16e93063363e9c Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Tue, 10 Dec 2024 12:03:16 -0800 Subject: [PATCH 11/23] Refactor generating proxy set headers --- internal/mode/static/nginx/config/servers.go | 66 +++--- .../mode/static/nginx/config/servers_test.go | 215 +++++++++++------- 2 files changed, 161 insertions(+), 120 deletions(-) diff --git a/internal/mode/static/nginx/config/servers.go b/internal/mode/static/nginx/config/servers.go index 78ec9c18ba..7420c26428 100644 --- a/internal/mode/static/nginx/config/servers.go +++ b/internal/mode/static/nginx/config/servers.go @@ -33,6 +33,11 @@ var httpConnectionHeader = http.Header{ Value: "$connection_upgrade", } +var unsetHTTPConnectionHeader = http.Header{ + Name: "Connection", + Value: "", +} + var httpUpgradeHeader = http.Header{ Name: "Upgrade", Value: "$http_upgrade", @@ -425,7 +430,16 @@ func updateLocation( } rewrites := createRewritesValForRewriteFilter(filters.RequestURLRewrite, path) - proxySetHeaders := generateProxySetHeaders(&matchRule.Filters, grpc, um, matchRule.BackendGroup.Backends) + + extraHeaders := make([]http.Header, 0, 3) + if grpc { + extraHeaders = append(extraHeaders, grpcAuthorityHeader) + } else { + extraHeaders = append(extraHeaders, httpUpgradeHeader) + extraHeaders = append(extraHeaders, getConnectionHeader(um, matchRule.BackendGroup.Backends)) + } + + proxySetHeaders := generateProxySetHeaders(&matchRule.Filters, createBaseProxySetHeaders(extraHeaders...)) responseHeaders := generateResponseHeaders(&matchRule.Filters) if rewrites != nil { @@ -723,50 +737,24 @@ func createMatchLocation(path string, grpc bool) http.Location { func generateProxySetHeaders( filters *dataplane.HTTPFilters, - grpc bool, - um UpstreamMap, - backends []dataplane.Backend, + baseHeaders []http.Header, ) []http.Header { - modifiedConnectionHeader := httpConnectionHeader - - for _, backend := range backends { - if um.keepAliveEnabled(backend.UpstreamName) { - // if keep-alive settings are enabled on any upstream, the connection header value - // must be empty for the location - modifiedConnectionHeader = http.Header{ - Name: httpConnectionHeader.Name, - Value: "", - } - break - } - } - - var extraHeaders []http.Header - if grpc { - extraHeaders = append(extraHeaders, grpcAuthorityHeader) - } else { - extraHeaders = append(extraHeaders, httpUpgradeHeader) - extraHeaders = append(extraHeaders, modifiedConnectionHeader) - } - - headers := createBaseProxySetHeaders(extraHeaders...) - if filters != nil && filters.RequestURLRewrite != nil && filters.RequestURLRewrite.Hostname != nil { - for i, header := range headers { + for i, header := range baseHeaders { if header.Name == "Host" { - headers[i].Value = *filters.RequestURLRewrite.Hostname + baseHeaders[i].Value = *filters.RequestURLRewrite.Hostname break } } } if filters == nil || filters.RequestHeaderModifiers == nil { - return headers + return baseHeaders } headerFilter := filters.RequestHeaderModifiers - headerLen := len(headerFilter.Add) + len(headerFilter.Set) + len(headerFilter.Remove) + len(headers) + headerLen := len(headerFilter.Add) + len(headerFilter.Set) + len(headerFilter.Remove) + len(baseHeaders) proxySetHeaders := make([]http.Header, 0, headerLen) if len(headerFilter.Add) > 0 { addHeaders := createHeadersWithVarName(headerFilter.Add) @@ -784,7 +772,7 @@ func generateProxySetHeaders( }) } - return append(proxySetHeaders, headers...) + return append(proxySetHeaders, baseHeaders...) } func generateResponseHeaders(filters *dataplane.HTTPFilters) http.ResponseHeaders { @@ -901,3 +889,15 @@ func createBaseProxySetHeaders(extraHeaders ...http.Header) []http.Header { return baseHeaders } + +func getConnectionHeader(um UpstreamMap, backends []dataplane.Backend) http.Header { + for _, backend := range backends { + if um.keepAliveEnabled(backend.UpstreamName) { + // if keep-alive settings are enabled on any upstream, the connection header value + // must be empty for the location + return unsetHTTPConnectionHeader + } + } + + return httpConnectionHeader +} diff --git a/internal/mode/static/nginx/config/servers_test.go b/internal/mode/static/nginx/config/servers_test.go index c28d58f533..0581362797 100644 --- a/internal/mode/static/nginx/config/servers_test.go +++ b/internal/mode/static/nginx/config/servers_test.go @@ -2843,9 +2843,7 @@ func TestGenerateProxySetHeaders(t *testing.T) { filters *dataplane.HTTPFilters msg string expectedHeaders []http.Header - upstreamMap UpstreamMap - backends []dataplane.Backend - GRPC bool + baseHeaders []http.Header }{ { msg: "header filter", @@ -2880,6 +2878,7 @@ func TestGenerateProxySetHeaders(t *testing.T) { Value: "", }, }, httpBaseHeaders...), + baseHeaders: httpBaseHeaders, }, { msg: "with url rewrite hostname", @@ -2934,10 +2933,10 @@ func TestGenerateProxySetHeaders(t *testing.T) { Value: "$connection_upgrade", }, }, + baseHeaders: createBaseProxySetHeaders(httpUpgradeHeader, httpConnectionHeader), }, { - msg: "header filter with gRPC", - GRPC: true, + msg: "header filter with gRPC", filters: &dataplane.HTTPFilters{ RequestHeaderModifiers: &dataplane.HTTPHeaderFilter{ Add: []dataplane.HTTPHeader{ @@ -2969,87 +2968,7 @@ func TestGenerateProxySetHeaders(t *testing.T) { Value: "", }, }, grpcBaseHeaders...), - }, - { - msg: "upstream with keepAlive enabled", - expectedHeaders: append(createBaseProxySetHeaders(httpUpgradeHeader), http.Header{ - Name: httpConnectionHeader.Name, - Value: "", - }), - upstreamMap: UpstreamMap{ - nameToUpstream: map[string]http.Upstream{ - "upstream": { - Name: "upstream", - KeepAliveConnections: 1, - }, - }, - }, - backends: []dataplane.Backend{ - { - UpstreamName: "upstream", - }, - }, - }, - { - msg: "multiple upstreams with keepAlive enabled", - expectedHeaders: append(createBaseProxySetHeaders(httpUpgradeHeader), http.Header{ - Name: httpConnectionHeader.Name, - Value: "", - }), - upstreamMap: UpstreamMap{ - nameToUpstream: map[string]http.Upstream{ - "upstream1": { - Name: "upstream1", - KeepAliveConnections: 1, - }, - "upstream2": { - Name: "upstream2", - KeepAliveRequests: 1, - }, - "upstream3": { - Name: "upstream3", - KeepAliveTime: "5s", - }, - }, - }, - backends: []dataplane.Backend{ - { - UpstreamName: "upstream1", - }, - { - UpstreamName: "upstream2", - }, - { - UpstreamName: "upstream3", - }, - }, - }, - { - msg: "mix of upstreams with keepAlive enabled and disabled", - expectedHeaders: append(createBaseProxySetHeaders(httpUpgradeHeader), http.Header{ - Name: httpConnectionHeader.Name, - Value: "", - }), - upstreamMap: UpstreamMap{ - nameToUpstream: map[string]http.Upstream{ - "upstream1": { - Name: "upstream1", - KeepAliveConnections: 1, - }, - "upstream2": { - Name: "upstream2", - ZoneSize: "2m", - }, - }, - }, - backends: []dataplane.Backend{ - { - UpstreamName: "upstream1", - }, - { - UpstreamName: "upstream2", - }, - }, + baseHeaders: grpcBaseHeaders, }, } @@ -3058,7 +2977,7 @@ func TestGenerateProxySetHeaders(t *testing.T) { t.Parallel() g := NewWithT(t) - headers := generateProxySetHeaders(tc.filters, tc.GRPC, tc.upstreamMap, tc.backends) + headers := generateProxySetHeaders(tc.filters, tc.baseHeaders) g.Expect(headers).To(Equal(tc.expectedHeaders)) }) } @@ -3132,6 +3051,128 @@ func TestCreateBaseProxySetHeaders(t *testing.T) { } } +func TestGetConnectionHeader(t *testing.T) { + t.Parallel() + + tests := []struct { + msg string + upstreamMap UpstreamMap + expConnectionHeader http.Header + backends []dataplane.Backend + }{ + { + msg: "no upstreams with keepAlive enabled", + upstreamMap: UpstreamMap{ + nameToUpstream: map[string]http.Upstream{ + "upstream1": { + Name: "upstream1", + }, + "upstream2": { + Name: "upstream2", + }, + "upstream3": { + Name: "upstream3", + }, + }, + }, + backends: []dataplane.Backend{ + { + UpstreamName: "upstream1", + }, + { + UpstreamName: "upstream2", + }, + { + UpstreamName: "upstream3", + }, + }, + expConnectionHeader: httpConnectionHeader, + }, + { + msg: "upstream with keepAlive enabled", + upstreamMap: UpstreamMap{ + nameToUpstream: map[string]http.Upstream{ + "upstream": { + Name: "upstream", + KeepAliveConnections: 1, + }, + }, + }, + backends: []dataplane.Backend{ + { + UpstreamName: "upstream", + }, + }, + expConnectionHeader: unsetHTTPConnectionHeader, + }, + { + msg: "multiple upstreams with keepAlive enabled", + upstreamMap: UpstreamMap{ + nameToUpstream: map[string]http.Upstream{ + "upstream1": { + Name: "upstream1", + KeepAliveConnections: 1, + }, + "upstream2": { + Name: "upstream2", + KeepAliveRequests: 1, + }, + "upstream3": { + Name: "upstream3", + KeepAliveTime: "5s", + }, + }, + }, + backends: []dataplane.Backend{ + { + UpstreamName: "upstream1", + }, + { + UpstreamName: "upstream2", + }, + { + UpstreamName: "upstream3", + }, + }, + expConnectionHeader: unsetHTTPConnectionHeader, + }, + { + msg: "mix of upstreams with keepAlive enabled and disabled", + expConnectionHeader: unsetHTTPConnectionHeader, + upstreamMap: UpstreamMap{ + nameToUpstream: map[string]http.Upstream{ + "upstream1": { + Name: "upstream1", + KeepAliveConnections: 1, + }, + "upstream2": { + Name: "upstream2", + ZoneSize: "2m", + }, + }, + }, + backends: []dataplane.Backend{ + { + UpstreamName: "upstream1", + }, + { + UpstreamName: "upstream2", + }, + }, + }, + } + + for _, tc := range tests { + t.Run(tc.msg, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + connectionHeader := getConnectionHeader(tc.upstreamMap, tc.backends) + g.Expect(connectionHeader).To(Equal(tc.expConnectionHeader)) + }) + } +} + func TestConvertBackendTLSFromGroup(t *testing.T) { t.Parallel() From 95d3f8d4608f4b594800f1bc94be59c3901ed0a1 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Tue, 10 Dec 2024 12:16:52 -0800 Subject: [PATCH 12/23] Add processor test case for processing non-UpstreamSettingsPolicies --- .../static/nginx/config/policies/processor.go | 4 +- .../policies/upstreamsettings/processor.go | 4 +- .../upstreamsettings/processor_test.go | 90 ++++++++++++++++++- 3 files changed, 94 insertions(+), 4 deletions(-) diff --git a/internal/mode/static/nginx/config/policies/processor.go b/internal/mode/static/nginx/config/policies/processor.go index 7a2317208f..d04a499392 100644 --- a/internal/mode/static/nginx/config/policies/processor.go +++ b/internal/mode/static/nginx/config/policies/processor.go @@ -1,7 +1,7 @@ package policies -// UpstreamSettingsProcessor defines an interface for an UpstreamSettingsPolicy processor -// to implement the process function. +// UpstreamSettingsProcessor is an interface that defines a method for processing a list of policies +// into upstream settings. type UpstreamSettingsProcessor interface { Process(policies []Policy) UpstreamSettings } diff --git a/internal/mode/static/nginx/config/policies/upstreamsettings/processor.go b/internal/mode/static/nginx/config/policies/upstreamsettings/processor.go index dd403e957a..65573c11d3 100644 --- a/internal/mode/static/nginx/config/policies/upstreamsettings/processor.go +++ b/internal/mode/static/nginx/config/policies/upstreamsettings/processor.go @@ -13,7 +13,9 @@ func NewProcessor() *Processor { return &Processor{} } -// Process processes policy configuration for an upstream block. +// Process processes policies into an UpstreamSettings object. The policies are already validated and are guaranteed +// to not contain overlapping settings. This method merges all fields in the policies into a single UpstreamSettings +// object. func (g Processor) Process(pols []policies.Policy) policies.UpstreamSettings { return processPolicies(pols) } diff --git a/internal/mode/static/nginx/config/policies/upstreamsettings/processor_test.go b/internal/mode/static/nginx/config/policies/upstreamsettings/processor_test.go index 3e735cce22..9ed8410026 100644 --- a/internal/mode/static/nginx/config/policies/upstreamsettings/processor_test.go +++ b/internal/mode/static/nginx/config/policies/upstreamsettings/processor_test.go @@ -153,7 +153,7 @@ func TestProcess(t *testing.T) { expUpstreamSettings: policies.UpstreamSettings{}, }, { - name: "multiple policies", + name: "multiple UpstreamSettingsPolicies", policies: []policies.Policy{ &ngfAPI.UpstreamSettingsPolicy{ ObjectMeta: metav1.ObjectMeta{ @@ -217,6 +217,94 @@ func TestProcess(t *testing.T) { KeepAliveTimeout: "10s", }, }, + { + name: "multiple UpstreamSettingsPolicies along with other policies", + policies: []policies.Policy{ + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp-zonesize", + Namespace: "test", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + ZoneSize: helpers.GetPointer[ngfAPI.Size]("2m"), + }, + }, + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp-keepalive-connections", + Namespace: "test", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + KeepAlive: helpers.GetPointer(ngfAPI.UpstreamKeepAlive{ + Connections: helpers.GetPointer(int32(1)), + }), + }, + }, + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp-keepalive-requests", + Namespace: "test", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + KeepAlive: helpers.GetPointer(ngfAPI.UpstreamKeepAlive{ + Requests: helpers.GetPointer(int32(1)), + }), + }, + }, + &ngfAPI.ClientSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "client-settings-policy", + Namespace: "test", + }, + Spec: ngfAPI.ClientSettingsPolicySpec{ + Body: &ngfAPI.ClientBody{ + MaxSize: helpers.GetPointer[ngfAPI.Size]("1m"), + }, + }, + }, + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp-keepalive-time", + Namespace: "test", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + KeepAlive: helpers.GetPointer(ngfAPI.UpstreamKeepAlive{ + Time: helpers.GetPointer[ngfAPI.Duration]("5s"), + }), + }, + }, + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp-keepalive-timeout", + Namespace: "test", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + KeepAlive: helpers.GetPointer(ngfAPI.UpstreamKeepAlive{ + Timeout: helpers.GetPointer[ngfAPI.Duration]("10s"), + }), + }, + }, + &ngfAPI.ObservabilityPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "observability-policy", + Namespace: "test", + }, + Spec: ngfAPI.ObservabilityPolicySpec{ + Tracing: &ngfAPI.Tracing{ + Strategy: ngfAPI.TraceStrategyRatio, + Ratio: helpers.GetPointer(int32(1)), + }, + }, + }, + }, + expUpstreamSettings: policies.UpstreamSettings{ + ZoneSize: "2m", + KeepAliveConnections: 1, + KeepAliveRequests: 1, + KeepAliveTime: "5s", + KeepAliveTimeout: "10s", + }, + }, } for _, test := range tests { From 03f3c7e9a7fc1a81ceabfac6ef04eab86662f624 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Tue, 10 Dec 2024 15:05:06 -0800 Subject: [PATCH 13/23] Add another upstream test case --- .../static/nginx/config/upstreams_test.go | 82 +++++++++++++++++++ 1 file changed, 82 insertions(+) diff --git a/internal/mode/static/nginx/config/upstreams_test.go b/internal/mode/static/nginx/config/upstreams_test.go index 0bde1ace14..b4c11452cc 100644 --- a/internal/mode/static/nginx/config/upstreams_test.go +++ b/internal/mode/static/nginx/config/upstreams_test.go @@ -477,6 +477,88 @@ func TestCreateUpstream(t *testing.T) { }, msg: "empty upstreamSettingsPolicies", }, + { + stateUpstream: dataplane.Upstream{ + Name: "upstreamSettingsPolicy with only keep alive settings", + Endpoints: []resolver.Endpoint{ + { + Address: "10.0.0.1", + Port: 80, + }, + }, + Policies: []policies.Policy{ + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp1", + Namespace: "test", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + KeepAlive: helpers.GetPointer(ngfAPI.UpstreamKeepAlive{ + Connections: helpers.GetPointer(int32(1)), + Requests: helpers.GetPointer(int32(1)), + Time: helpers.GetPointer[ngfAPI.Duration]("5s"), + Timeout: helpers.GetPointer[ngfAPI.Duration]("10s"), + }), + }, + }, + }, + }, + expectedUpstream: http.Upstream{ + Name: "upstreamSettingsPolicy with only keep alive settings", + ZoneSize: ossZoneSize, + Servers: []http.UpstreamServer{ + { + Address: "10.0.0.1:80", + }, + }, + KeepAliveConnections: 1, + KeepAliveRequests: 1, + KeepAliveTime: "5s", + KeepAliveTimeout: "10s", + }, + msg: "upstreamSettingsPolicy with only keep alive settings", + }, + { + stateUpstream: dataplane.Upstream{ + Name: "upstreamSettingsPolicy with only keep alive settings", + Endpoints: []resolver.Endpoint{ + { + Address: "10.0.0.1", + Port: 80, + }, + }, + Policies: []policies.Policy{ + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp1", + Namespace: "test", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + KeepAlive: helpers.GetPointer(ngfAPI.UpstreamKeepAlive{ + Connections: helpers.GetPointer(int32(1)), + Requests: helpers.GetPointer(int32(1)), + Time: helpers.GetPointer[ngfAPI.Duration]("5s"), + Timeout: helpers.GetPointer[ngfAPI.Duration]("10s"), + }), + }, + }, + }, + }, + expectedUpstream: http.Upstream{ + Name: "upstreamSettingsPolicy with only keep alive settings", + ZoneSize: ossZoneSize, + Servers: []http.UpstreamServer{ + { + Address: "10.0.0.1:80", + }, + }, + KeepAliveConnections: 1, + KeepAliveRequests: 1, + KeepAliveTime: "5s", + KeepAliveTimeout: "10s", + }, + msg: "upstreamSettingsPolicy with only keep alive settings", + }, } for _, test := range tests { From c54b8744208ba8c36b746852f70b1bb394115033 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Tue, 10 Dec 2024 15:05:34 -0800 Subject: [PATCH 14/23] Fix upstream test case --- .../static/nginx/config/upstreams_test.go | 41 ------------------- 1 file changed, 41 deletions(-) diff --git a/internal/mode/static/nginx/config/upstreams_test.go b/internal/mode/static/nginx/config/upstreams_test.go index b4c11452cc..1388b20308 100644 --- a/internal/mode/static/nginx/config/upstreams_test.go +++ b/internal/mode/static/nginx/config/upstreams_test.go @@ -518,47 +518,6 @@ func TestCreateUpstream(t *testing.T) { }, msg: "upstreamSettingsPolicy with only keep alive settings", }, - { - stateUpstream: dataplane.Upstream{ - Name: "upstreamSettingsPolicy with only keep alive settings", - Endpoints: []resolver.Endpoint{ - { - Address: "10.0.0.1", - Port: 80, - }, - }, - Policies: []policies.Policy{ - &ngfAPI.UpstreamSettingsPolicy{ - ObjectMeta: metav1.ObjectMeta{ - Name: "usp1", - Namespace: "test", - }, - Spec: ngfAPI.UpstreamSettingsPolicySpec{ - KeepAlive: helpers.GetPointer(ngfAPI.UpstreamKeepAlive{ - Connections: helpers.GetPointer(int32(1)), - Requests: helpers.GetPointer(int32(1)), - Time: helpers.GetPointer[ngfAPI.Duration]("5s"), - Timeout: helpers.GetPointer[ngfAPI.Duration]("10s"), - }), - }, - }, - }, - }, - expectedUpstream: http.Upstream{ - Name: "upstreamSettingsPolicy with only keep alive settings", - ZoneSize: ossZoneSize, - Servers: []http.UpstreamServer{ - { - Address: "10.0.0.1:80", - }, - }, - KeepAliveConnections: 1, - KeepAliveRequests: 1, - KeepAliveTime: "5s", - KeepAliveTimeout: "10s", - }, - msg: "upstreamSettingsPolicy with only keep alive settings", - }, } for _, test := range tests { From c53c24ba0c43c5e948dbaef4ff74f429f5121392 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Tue, 10 Dec 2024 15:22:11 -0800 Subject: [PATCH 15/23] Extract keepalive settings outside of UpstreamSettings --- .../static/nginx/config/policies/processor.go | 22 +++++--- .../policies/upstreamsettings/processor.go | 14 ++--- .../upstreamsettings/processor_test.go | 52 ++++++++++++------- .../mode/static/nginx/config/upstreams.go | 8 +-- 4 files changed, 58 insertions(+), 38 deletions(-) diff --git a/internal/mode/static/nginx/config/policies/processor.go b/internal/mode/static/nginx/config/policies/processor.go index d04a499392..e84d22dcbc 100644 --- a/internal/mode/static/nginx/config/policies/processor.go +++ b/internal/mode/static/nginx/config/policies/processor.go @@ -10,12 +10,18 @@ type UpstreamSettingsProcessor interface { type UpstreamSettings struct { // ZoneSize is the zone size setting. ZoneSize string - // KeepAliveTime is the keep-alive time setting. - KeepAliveTime string - // KeepAliveTimeout is the keep-alive timeout setting. - KeepAliveTimeout string - // KeepAliveConnections is the keep-alive connections setting. - KeepAliveConnections int32 - // KeepAliveRequests is the keep-alive requests setting. - KeepAliveRequests int32 + // KeepAlive contains the keepalive settings. + KeepAlive KeepAlive +} + +// KeepAlive contains the keepalive settings. +type KeepAlive struct { + // Time is the keepalive time value. + Time string + // Timeout is the keepalive timeout value. + Timeout string + // Connections is the keepalive connections value. + Connections int32 + // Requests is the keepalive requests value. + Requests int32 } diff --git a/internal/mode/static/nginx/config/policies/upstreamsettings/processor.go b/internal/mode/static/nginx/config/policies/upstreamsettings/processor.go index 65573c11d3..b819c28a94 100644 --- a/internal/mode/static/nginx/config/policies/upstreamsettings/processor.go +++ b/internal/mode/static/nginx/config/policies/upstreamsettings/processor.go @@ -5,12 +5,12 @@ import ( "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies" ) -// Processor processes UpstreamSettingsPolicies. +// Processor processes UpstreamSettingsPolicies. It implements policies.UpstreamSettingsProcessor. type Processor struct{} // NewProcessor returns a new instance of Processor. -func NewProcessor() *Processor { - return &Processor{} +func NewProcessor() Processor { + return Processor{} } // Process processes policies into an UpstreamSettings object. The policies are already validated and are guaranteed @@ -37,19 +37,19 @@ func processPolicies(pols []policies.Policy) policies.UpstreamSettings { if usp.Spec.KeepAlive != nil { if usp.Spec.KeepAlive.Connections != nil { - upstreamSettings.KeepAliveConnections = *usp.Spec.KeepAlive.Connections + upstreamSettings.KeepAlive.Connections = *usp.Spec.KeepAlive.Connections } if usp.Spec.KeepAlive.Requests != nil { - upstreamSettings.KeepAliveRequests = *usp.Spec.KeepAlive.Requests + upstreamSettings.KeepAlive.Requests = *usp.Spec.KeepAlive.Requests } if usp.Spec.KeepAlive.Time != nil { - upstreamSettings.KeepAliveTime = string(*usp.Spec.KeepAlive.Time) + upstreamSettings.KeepAlive.Time = string(*usp.Spec.KeepAlive.Time) } if usp.Spec.KeepAlive.Timeout != nil { - upstreamSettings.KeepAliveTimeout = string(*usp.Spec.KeepAlive.Timeout) + upstreamSettings.KeepAlive.Timeout = string(*usp.Spec.KeepAlive.Timeout) } } } diff --git a/internal/mode/static/nginx/config/policies/upstreamsettings/processor_test.go b/internal/mode/static/nginx/config/policies/upstreamsettings/processor_test.go index 9ed8410026..b4756db60a 100644 --- a/internal/mode/static/nginx/config/policies/upstreamsettings/processor_test.go +++ b/internal/mode/static/nginx/config/policies/upstreamsettings/processor_test.go @@ -39,11 +39,13 @@ func TestProcess(t *testing.T) { }, }, expUpstreamSettings: policies.UpstreamSettings{ - ZoneSize: "2m", - KeepAliveConnections: 1, - KeepAliveRequests: 1, - KeepAliveTime: "5s", - KeepAliveTimeout: "10s", + ZoneSize: "2m", + KeepAlive: policies.KeepAlive{ + Connections: 1, + Requests: 1, + Time: "5s", + Timeout: "10s", + }, }, }, { @@ -79,7 +81,9 @@ func TestProcess(t *testing.T) { }, }, expUpstreamSettings: policies.UpstreamSettings{ - KeepAliveConnections: 1, + KeepAlive: policies.KeepAlive{ + Connections: 1, + }, }, }, { @@ -98,7 +102,9 @@ func TestProcess(t *testing.T) { }, }, expUpstreamSettings: policies.UpstreamSettings{ - KeepAliveRequests: 1, + KeepAlive: policies.KeepAlive{ + Requests: 1, + }, }, }, { @@ -117,7 +123,9 @@ func TestProcess(t *testing.T) { }, }, expUpstreamSettings: policies.UpstreamSettings{ - KeepAliveTime: "5s", + KeepAlive: policies.KeepAlive{ + Time: "5s", + }, }, }, { @@ -136,7 +144,9 @@ func TestProcess(t *testing.T) { }, }, expUpstreamSettings: policies.UpstreamSettings{ - KeepAliveTimeout: "10s", + KeepAlive: policies.KeepAlive{ + Timeout: "10s", + }, }, }, { @@ -210,11 +220,13 @@ func TestProcess(t *testing.T) { }, }, expUpstreamSettings: policies.UpstreamSettings{ - ZoneSize: "2m", - KeepAliveConnections: 1, - KeepAliveRequests: 1, - KeepAliveTime: "5s", - KeepAliveTimeout: "10s", + ZoneSize: "2m", + KeepAlive: policies.KeepAlive{ + Connections: 1, + Requests: 1, + Time: "5s", + Timeout: "10s", + }, }, }, { @@ -298,11 +310,13 @@ func TestProcess(t *testing.T) { }, }, expUpstreamSettings: policies.UpstreamSettings{ - ZoneSize: "2m", - KeepAliveConnections: 1, - KeepAliveRequests: 1, - KeepAliveTime: "5s", - KeepAliveTimeout: "10s", + ZoneSize: "2m", + KeepAlive: policies.KeepAlive{ + Connections: 1, + Requests: 1, + Time: "5s", + Timeout: "10s", + }, }, }, } diff --git a/internal/mode/static/nginx/config/upstreams.go b/internal/mode/static/nginx/config/upstreams.go index 0cecaf5955..dbb6697a50 100644 --- a/internal/mode/static/nginx/config/upstreams.go +++ b/internal/mode/static/nginx/config/upstreams.go @@ -179,10 +179,10 @@ func (g GeneratorImpl) createUpstream( Name: up.Name, ZoneSize: zoneSize, Servers: upstreamServers, - KeepAliveConnections: upstreamPolicySettings.KeepAliveConnections, - KeepAliveRequests: upstreamPolicySettings.KeepAliveRequests, - KeepAliveTime: upstreamPolicySettings.KeepAliveTime, - KeepAliveTimeout: upstreamPolicySettings.KeepAliveTimeout, + KeepAliveConnections: upstreamPolicySettings.KeepAlive.Connections, + KeepAliveRequests: upstreamPolicySettings.KeepAlive.Requests, + KeepAliveTime: upstreamPolicySettings.KeepAlive.Time, + KeepAliveTimeout: upstreamPolicySettings.KeepAlive.Timeout, } } From 02705e0b836db7cefa4c403831a24518419d03dc Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Tue, 10 Dec 2024 15:26:24 -0800 Subject: [PATCH 16/23] Turn executeUpstreams into function --- internal/mode/static/nginx/config/generator.go | 2 +- internal/mode/static/nginx/config/upstreams.go | 6 +++--- internal/mode/static/nginx/config/upstreams_test.go | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/internal/mode/static/nginx/config/generator.go b/internal/mode/static/nginx/config/generator.go index 977bbcc73f..e90249221e 100644 --- a/internal/mode/static/nginx/config/generator.go +++ b/internal/mode/static/nginx/config/generator.go @@ -170,7 +170,7 @@ func (g GeneratorImpl) getExecuteFuncs( executeMainConfig, executeBaseHTTPConfig, g.newExecuteServersFunc(generator, upstreamMap), - g.newExecuteUpstreamsFunc(upstreams), + newExecuteUpstreamsFunc(upstreams), executeSplitClients, executeMaps, executeTelemetry, diff --git a/internal/mode/static/nginx/config/upstreams.go b/internal/mode/static/nginx/config/upstreams.go index dbb6697a50..f9c5e53c05 100644 --- a/internal/mode/static/nginx/config/upstreams.go +++ b/internal/mode/static/nginx/config/upstreams.go @@ -49,13 +49,13 @@ func (um UpstreamMap) keepAliveEnabled(name string) bool { return false } -func (g GeneratorImpl) newExecuteUpstreamsFunc(upstreams []http.Upstream) executeFunc { +func newExecuteUpstreamsFunc(upstreams []http.Upstream) executeFunc { return func(_ dataplane.Configuration) []executeResult { - return g.executeUpstreams(upstreams) + return executeUpstreams(upstreams) } } -func (g GeneratorImpl) executeUpstreams(upstreams []http.Upstream) []executeResult { +func executeUpstreams(upstreams []http.Upstream) []executeResult { result := executeResult{ dest: httpConfigFile, data: helpers.MustExecuteTemplate(upstreamsTemplate, upstreams), diff --git a/internal/mode/static/nginx/config/upstreams_test.go b/internal/mode/static/nginx/config/upstreams_test.go index 1388b20308..50a7640473 100644 --- a/internal/mode/static/nginx/config/upstreams_test.go +++ b/internal/mode/static/nginx/config/upstreams_test.go @@ -103,7 +103,7 @@ func TestExecuteUpstreams(t *testing.T) { upstreams := gen.createUpstreams(stateUpstreams, upstreamsettings.NewProcessor()) - upstreamResults := gen.executeUpstreams(upstreams) + upstreamResults := executeUpstreams(upstreams) g := NewWithT(t) g.Expect(upstreamResults).To(HaveLen(1)) nginxUpstreams := string(upstreamResults[0].data) From 513351eb18148f2703ac1f23ece4e0c075a4715e Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Tue, 10 Dec 2024 16:11:56 -0800 Subject: [PATCH 17/23] Add UpstreamKeepAlive struct for http Upstream --- .../mode/static/nginx/config/http/config.go | 19 ++-- .../mode/static/nginx/config/servers_test.go | 30 ++++--- .../mode/static/nginx/config/upstreams.go | 24 ++--- .../static/nginx/config/upstreams_template.go | 16 ++-- .../static/nginx/config/upstreams_test.go | 90 +++++++++++-------- 5 files changed, 108 insertions(+), 71 deletions(-) diff --git a/internal/mode/static/nginx/config/http/config.go b/internal/mode/static/nginx/config/http/config.go index eda12cb8ff..f1c6caae46 100644 --- a/internal/mode/static/nginx/config/http/config.go +++ b/internal/mode/static/nginx/config/http/config.go @@ -84,13 +84,18 @@ const ( // Upstream holds all configuration for an HTTP upstream. type Upstream struct { - Name string - ZoneSize string // format: 512k, 1m - KeepAliveTime string - KeepAliveTimeout string - Servers []UpstreamServer - KeepAliveConnections int32 - KeepAliveRequests int32 + Name string + ZoneSize string // format: 512k, 1m + KeepAlive UpstreamKeepAlive + Servers []UpstreamServer +} + +// UpstreamKeepAlive holds the keepalive configuration for an HTTP upstream. +type UpstreamKeepAlive struct { + Time string + Timeout string + Connections int32 + Requests int32 } // UpstreamServer holds all configuration for an HTTP upstream server. diff --git a/internal/mode/static/nginx/config/servers_test.go b/internal/mode/static/nginx/config/servers_test.go index 0581362797..3e1f7df247 100644 --- a/internal/mode/static/nginx/config/servers_test.go +++ b/internal/mode/static/nginx/config/servers_test.go @@ -3093,8 +3093,10 @@ func TestGetConnectionHeader(t *testing.T) { upstreamMap: UpstreamMap{ nameToUpstream: map[string]http.Upstream{ "upstream": { - Name: "upstream", - KeepAliveConnections: 1, + Name: "upstream", + KeepAlive: http.UpstreamKeepAlive{ + Connections: 1, + }, }, }, }, @@ -3110,16 +3112,22 @@ func TestGetConnectionHeader(t *testing.T) { upstreamMap: UpstreamMap{ nameToUpstream: map[string]http.Upstream{ "upstream1": { - Name: "upstream1", - KeepAliveConnections: 1, + Name: "upstream1", + KeepAlive: http.UpstreamKeepAlive{ + Connections: 1, + }, }, "upstream2": { - Name: "upstream2", - KeepAliveRequests: 1, + Name: "upstream2", + KeepAlive: http.UpstreamKeepAlive{ + Requests: 1, + }, }, "upstream3": { - Name: "upstream3", - KeepAliveTime: "5s", + Name: "upstream3", + KeepAlive: http.UpstreamKeepAlive{ + Time: "5s", + }, }, }, }, @@ -3142,8 +3150,10 @@ func TestGetConnectionHeader(t *testing.T) { upstreamMap: UpstreamMap{ nameToUpstream: map[string]http.Upstream{ "upstream1": { - Name: "upstream1", - KeepAliveConnections: 1, + Name: "upstream1", + KeepAlive: http.UpstreamKeepAlive{ + Connections: 1, + }, }, "upstream2": { Name: "upstream2", diff --git a/internal/mode/static/nginx/config/upstreams.go b/internal/mode/static/nginx/config/upstreams.go index f9c5e53c05..aca58be21a 100644 --- a/internal/mode/static/nginx/config/upstreams.go +++ b/internal/mode/static/nginx/config/upstreams.go @@ -40,10 +40,10 @@ type UpstreamMap struct { func (um UpstreamMap) keepAliveEnabled(name string) bool { if upstream, exists := um.nameToUpstream[name]; exists { - return upstream.KeepAliveConnections != 0 || - upstream.KeepAliveRequests != 0 || - upstream.KeepAliveTime != "" || - upstream.KeepAliveTimeout != "" + return upstream.KeepAlive.Connections != 0 || + upstream.KeepAlive.Requests != 0 || + upstream.KeepAlive.Time != "" || + upstream.KeepAlive.Timeout != "" } return false @@ -176,13 +176,15 @@ func (g GeneratorImpl) createUpstream( } return http.Upstream{ - Name: up.Name, - ZoneSize: zoneSize, - Servers: upstreamServers, - KeepAliveConnections: upstreamPolicySettings.KeepAlive.Connections, - KeepAliveRequests: upstreamPolicySettings.KeepAlive.Requests, - KeepAliveTime: upstreamPolicySettings.KeepAlive.Time, - KeepAliveTimeout: upstreamPolicySettings.KeepAlive.Timeout, + Name: up.Name, + ZoneSize: zoneSize, + Servers: upstreamServers, + KeepAlive: http.UpstreamKeepAlive{ + Connections: upstreamPolicySettings.KeepAlive.Connections, + Requests: upstreamPolicySettings.KeepAlive.Requests, + Time: upstreamPolicySettings.KeepAlive.Time, + Timeout: upstreamPolicySettings.KeepAlive.Timeout, + }, } } diff --git a/internal/mode/static/nginx/config/upstreams_template.go b/internal/mode/static/nginx/config/upstreams_template.go index a855235317..8f82fa049b 100644 --- a/internal/mode/static/nginx/config/upstreams_template.go +++ b/internal/mode/static/nginx/config/upstreams_template.go @@ -16,17 +16,17 @@ upstream {{ $u.Name }} { {{ range $server := $u.Servers }} server {{ $server.Address }}; {{- end }} - {{ if $u.KeepAliveConnections -}} - keepalive {{ $u.KeepAliveConnections }}; + {{ if $u.KeepAlive.Connections -}} + keepalive {{ $u.KeepAlive.Connections }}; {{- end }} - {{ if $u.KeepAliveRequests -}} - keepalive_requests {{ $u.KeepAliveRequests }}; + {{ if $u.KeepAlive.Requests -}} + keepalive_requests {{ $u.KeepAlive.Requests }}; {{- end }} - {{ if $u.KeepAliveTime -}} - keepalive_time {{ $u.KeepAliveTime }}; + {{ if $u.KeepAlive.Time -}} + keepalive_time {{ $u.KeepAlive.Time }}; {{- end }} - {{ if $u.KeepAliveTimeout -}} - keepalive_timeout {{ $u.KeepAliveTimeout }}; + {{ if $u.KeepAlive.Timeout -}} + keepalive_timeout {{ $u.KeepAlive.Timeout }}; {{- end }} } {{ end -}} diff --git a/internal/mode/static/nginx/config/upstreams_test.go b/internal/mode/static/nginx/config/upstreams_test.go index 50a7640473..510c975674 100644 --- a/internal/mode/static/nginx/config/upstreams_test.go +++ b/internal/mode/static/nginx/config/upstreams_test.go @@ -237,10 +237,12 @@ func TestCreateUpstreams(t *testing.T) { Address: "12.0.0.0:80", }, }, - KeepAliveConnections: 1, - KeepAliveRequests: 1, - KeepAliveTime: "5s", - KeepAliveTimeout: "10s", + KeepAlive: http.UpstreamKeepAlive{ + Connections: 1, + Requests: 1, + Time: "5s", + Timeout: "10s", + }, }, { Name: invalidBackendRef, @@ -262,8 +264,8 @@ func TestCreateUpstream(t *testing.T) { gen := GeneratorImpl{} tests := []struct { msg string - stateUpstream dataplane.Upstream expectedUpstream http.Upstream + stateUpstream dataplane.Upstream }{ { stateUpstream: dataplane.Upstream{ @@ -389,10 +391,12 @@ func TestCreateUpstream(t *testing.T) { Address: "10.0.0.1:80", }, }, - KeepAliveConnections: 1, - KeepAliveRequests: 1, - KeepAliveTime: "5s", - KeepAliveTimeout: "10s", + KeepAlive: http.UpstreamKeepAlive{ + Connections: 1, + Requests: 1, + Time: "5s", + Timeout: "10s", + }, }, msg: "single upstreamSettingsPolicy", }, @@ -441,10 +445,12 @@ func TestCreateUpstream(t *testing.T) { Address: "10.0.0.1:80", }, }, - KeepAliveConnections: 1, - KeepAliveRequests: 1, - KeepAliveTime: "5s", - KeepAliveTimeout: "10s", + KeepAlive: http.UpstreamKeepAlive{ + Connections: 1, + Requests: 1, + Time: "5s", + Timeout: "10s", + }, }, msg: "multiple upstreamSettingsPolicies", }, @@ -511,10 +517,12 @@ func TestCreateUpstream(t *testing.T) { Address: "10.0.0.1:80", }, }, - KeepAliveConnections: 1, - KeepAliveRequests: 1, - KeepAliveTime: "5s", - KeepAliveTimeout: "10s", + KeepAlive: http.UpstreamKeepAlive{ + Connections: 1, + Requests: 1, + Time: "5s", + Timeout: "10s", + }, }, msg: "upstreamSettingsPolicy with only keep alive settings", }, @@ -791,43 +799,53 @@ func TestKeepAliveEnabled(t *testing.T) { { msg: "upstream with all keepAlive fields set", upstream: http.Upstream{ - Name: "upAllKeepAliveFieldsSet", - KeepAliveConnections: 1, - KeepAliveRequests: 1, - KeepAliveTime: "5s", - KeepAliveTimeout: "10s", + Name: "upAllKeepAliveFieldsSet", + KeepAlive: http.UpstreamKeepAlive{ + Connections: 1, + Requests: 1, + Time: "5s", + Timeout: "10s", + }, }, expKeepAliveEnabled: true, }, { msg: "upstream with keepAlive connection field set", upstream: http.Upstream{ - Name: "upKeepAliveConnectionsSet", - KeepAliveConnections: 1, + Name: "upKeepAliveConnectionsSet", + KeepAlive: http.UpstreamKeepAlive{ + Connections: 1, + }, }, expKeepAliveEnabled: true, }, { msg: "upstream with keepAlive requests field set", upstream: http.Upstream{ - Name: "upKeepAliveRequestsSet", - KeepAliveRequests: 1, + Name: "upKeepAliveRequestsSet", + KeepAlive: http.UpstreamKeepAlive{ + Requests: 1, + }, }, expKeepAliveEnabled: true, }, { msg: "upstream with keepAlive time field set", upstream: http.Upstream{ - Name: "upKeepAliveTimeSet", - KeepAliveTime: "5s", + Name: "upKeepAliveTimeSet", + KeepAlive: http.UpstreamKeepAlive{ + Time: "5s", + }, }, expKeepAliveEnabled: true, }, { msg: "upstream with keepAlive timeout field set", upstream: http.Upstream{ - Name: "upKeepAliveTimeoutSet", - KeepAliveTimeout: "10s", + Name: "upKeepAliveTimeoutSet", + KeepAlive: http.UpstreamKeepAlive{ + Timeout: "10s", + }, }, expKeepAliveEnabled: true, }, @@ -841,11 +859,13 @@ func TestKeepAliveEnabled(t *testing.T) { { msg: "upstream with keepAlive fields set to empty values", upstream: http.Upstream{ - Name: "upNoKeepAliveFieldsSet", - KeepAliveConnections: 0, - KeepAliveRequests: 0, - KeepAliveTime: "", - KeepAliveTimeout: "", + Name: "upNoKeepAliveFieldsSet", + KeepAlive: http.UpstreamKeepAlive{ + Connections: 0, + Requests: 0, + Time: "", + Timeout: "", + }, }, expKeepAliveEnabled: false, }, From a2ace838537abac0608f0206035a044700f103b4 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Wed, 11 Dec 2024 13:31:38 -0800 Subject: [PATCH 18/23] Add keepAliveChecker function --- .../mode/static/nginx/config/generator.go | 8 +- internal/mode/static/nginx/config/servers.go | 51 ++-- .../mode/static/nginx/config/servers_test.go | 128 +++++----- .../mode/static/nginx/config/upstreams.go | 35 ++- .../static/nginx/config/upstreams_test.go | 238 ++++++++++++------ 5 files changed, 280 insertions(+), 180 deletions(-) diff --git a/internal/mode/static/nginx/config/generator.go b/internal/mode/static/nginx/config/generator.go index e90249221e..24b8ff8996 100644 --- a/internal/mode/static/nginx/config/generator.go +++ b/internal/mode/static/nginx/config/generator.go @@ -134,9 +134,9 @@ func (g GeneratorImpl) executeConfigTemplates( fileBytes := make(map[string][]byte) httpUpstreams := g.createUpstreams(conf.Upstreams, upstreamsettings.NewProcessor()) - upstreamMap := g.createUpstreamMap(httpUpstreams) + keepAliveCheck := newKeepAliveChecker(httpUpstreams) - for _, execute := range g.getExecuteFuncs(generator, httpUpstreams, upstreamMap) { + for _, execute := range g.getExecuteFuncs(generator, httpUpstreams, keepAliveCheck) { results := execute(conf) for _, res := range results { fileBytes[res.dest] = append(fileBytes[res.dest], res.data...) @@ -164,12 +164,12 @@ func (g GeneratorImpl) executeConfigTemplates( func (g GeneratorImpl) getExecuteFuncs( generator policies.Generator, upstreams []http.Upstream, - upstreamMap UpstreamMap, + keepAliveCheck keepAliveChecker, ) []executeFunc { return []executeFunc{ executeMainConfig, executeBaseHTTPConfig, - g.newExecuteServersFunc(generator, upstreamMap), + g.newExecuteServersFunc(generator, keepAliveCheck), newExecuteUpstreamsFunc(upstreams), executeSplitClients, executeMaps, diff --git a/internal/mode/static/nginx/config/servers.go b/internal/mode/static/nginx/config/servers.go index 7420c26428..e7ccef05fc 100644 --- a/internal/mode/static/nginx/config/servers.go +++ b/internal/mode/static/nginx/config/servers.go @@ -43,18 +43,21 @@ var httpUpgradeHeader = http.Header{ Value: "$http_upgrade", } -func (g GeneratorImpl) newExecuteServersFunc(generator policies.Generator, um UpstreamMap) executeFunc { +func (g GeneratorImpl) newExecuteServersFunc( + generator policies.Generator, + keepAliveCheck keepAliveChecker, +) executeFunc { return func(configuration dataplane.Configuration) []executeResult { - return g.executeServers(configuration, generator, um) + return g.executeServers(configuration, generator, keepAliveCheck) } } func (g GeneratorImpl) executeServers( conf dataplane.Configuration, generator policies.Generator, - um UpstreamMap, + keepAliveCheck keepAliveChecker, ) []executeResult { - servers, httpMatchPairs := createServers(conf, generator, um) + servers, httpMatchPairs := createServers(conf, generator, keepAliveCheck) serverConfig := http.ServerConfig{ Servers: servers, @@ -104,7 +107,7 @@ func getIPFamily(baseHTTPConfig dataplane.BaseHTTPConfig) shared.IPFamily { func createServers( conf dataplane.Configuration, generator policies.Generator, - um UpstreamMap, + keepAliveCheck keepAliveChecker, ) ([]http.Server, httpMatchPairs) { servers := make([]http.Server, 0, len(conf.HTTPServers)+len(conf.SSLServers)) finalMatchPairs := make(httpMatchPairs) @@ -116,7 +119,7 @@ func createServers( for idx, s := range conf.HTTPServers { serverID := fmt.Sprintf("%d", idx) - httpServer, matchPairs := createServer(s, serverID, generator, um) + httpServer, matchPairs := createServer(s, serverID, generator, keepAliveCheck) servers = append(servers, httpServer) maps.Copy(finalMatchPairs, matchPairs) } @@ -124,7 +127,7 @@ func createServers( for idx, s := range conf.SSLServers { serverID := fmt.Sprintf("SSL_%d", idx) - sslServer, matchPairs := createSSLServer(s, serverID, generator, um) + sslServer, matchPairs := createSSLServer(s, serverID, generator, keepAliveCheck) if _, portInUse := sharedTLSPorts[s.Port]; portInUse { sslServer.Listen = getSocketNameHTTPS(s.Port) sslServer.IsSocket = true @@ -140,7 +143,7 @@ func createSSLServer( virtualServer dataplane.VirtualServer, serverID string, generator policies.Generator, - um UpstreamMap, + keepAliveCheck keepAliveChecker, ) (http.Server, httpMatchPairs) { listen := fmt.Sprint(virtualServer.Port) if virtualServer.IsDefault { @@ -150,7 +153,7 @@ func createSSLServer( }, nil } - locs, matchPairs, grpc := createLocations(&virtualServer, serverID, generator, um) + locs, matchPairs, grpc := createLocations(&virtualServer, serverID, generator, keepAliveCheck) server := http.Server{ ServerName: virtualServer.Hostname, @@ -179,7 +182,7 @@ func createServer( virtualServer dataplane.VirtualServer, serverID string, generator policies.Generator, - um UpstreamMap, + keepAliveCheck keepAliveChecker, ) (http.Server, httpMatchPairs) { listen := fmt.Sprint(virtualServer.Port) @@ -190,7 +193,7 @@ func createServer( }, nil } - locs, matchPairs, grpc := createLocations(&virtualServer, serverID, generator, um) + locs, matchPairs, grpc := createLocations(&virtualServer, serverID, generator, keepAliveCheck) server := http.Server{ ServerName: virtualServer.Hostname, @@ -226,7 +229,7 @@ func createLocations( server *dataplane.VirtualServer, serverID string, generator policies.Generator, - um UpstreamMap, + keepAliveCheck keepAliveChecker, ) ([]http.Location, httpMatchPairs, bool) { maxLocs, pathsAndTypes := getMaxLocationCountAndPathMap(server.PathRules) locs := make([]http.Location, 0, maxLocs) @@ -255,7 +258,15 @@ func createLocations( if !needsInternalLocations(rule) { for _, r := range rule.MatchRules { - extLocations = updateLocations(r.Filters, extLocations, r, server.Port, rule.Path, rule.GRPC, um) + extLocations = updateLocations( + r.Filters, + extLocations, + r, + server.Port, + rule.Path, + rule.GRPC, + keepAliveCheck, + ) } locs = append(locs, extLocations...) @@ -277,7 +288,7 @@ func createLocations( server.Port, rule.Path, rule.GRPC, - um, + keepAliveCheck, ) internalLocations = append(internalLocations, intLocation) @@ -414,7 +425,7 @@ func updateLocation( listenerPort int32, path string, grpc bool, - um UpstreamMap, + keepAliveCheck keepAliveChecker, ) http.Location { if filters.InvalidFilter != nil { location.Return = &http.Return{Code: http.StatusInternalServerError} @@ -436,7 +447,7 @@ func updateLocation( extraHeaders = append(extraHeaders, grpcAuthorityHeader) } else { extraHeaders = append(extraHeaders, httpUpgradeHeader) - extraHeaders = append(extraHeaders, getConnectionHeader(um, matchRule.BackendGroup.Backends)) + extraHeaders = append(extraHeaders, getConnectionHeader(keepAliveCheck, matchRule.BackendGroup.Backends)) } proxySetHeaders := generateProxySetHeaders(&matchRule.Filters, createBaseProxySetHeaders(extraHeaders...)) @@ -476,12 +487,12 @@ func updateLocations( listenerPort int32, path string, grpc bool, - um UpstreamMap, + keepAliveCheck keepAliveChecker, ) []http.Location { updatedLocations := make([]http.Location, len(buildLocations)) for i, loc := range buildLocations { - updatedLocations[i] = updateLocation(filters, loc, matchRule, listenerPort, path, grpc, um) + updatedLocations[i] = updateLocation(filters, loc, matchRule, listenerPort, path, grpc, keepAliveCheck) } return updatedLocations @@ -890,9 +901,9 @@ func createBaseProxySetHeaders(extraHeaders ...http.Header) []http.Header { return baseHeaders } -func getConnectionHeader(um UpstreamMap, backends []dataplane.Backend) http.Header { +func getConnectionHeader(keepAliveCheck keepAliveChecker, backends []dataplane.Backend) http.Header { for _, backend := range backends { - if um.keepAliveEnabled(backend.UpstreamName) { + if keepAliveCheck(backend.UpstreamName) { // if keep-alive settings are enabled on any upstream, the connection header value // must be empty for the location return unsetHTTPConnectionHeader diff --git a/internal/mode/static/nginx/config/servers_test.go b/internal/mode/static/nginx/config/servers_test.go index 3e1f7df247..bdf40f8bd8 100644 --- a/internal/mode/static/nginx/config/servers_test.go +++ b/internal/mode/static/nginx/config/servers_test.go @@ -187,7 +187,7 @@ func TestExecuteServers(t *testing.T) { ) gen := GeneratorImpl{} - results := gen.executeServers(conf, fakeGenerator, UpstreamMap{}) + results := gen.executeServers(conf, fakeGenerator, newKeepAliveChecker([]http.Upstream{})) g.Expect(results).To(HaveLen(len(expectedResults))) for _, res := range results { @@ -326,7 +326,11 @@ func TestExecuteServers_IPFamily(t *testing.T) { g := NewWithT(t) gen := GeneratorImpl{} - results := gen.executeServers(test.config, &policiesfakes.FakeGenerator{}, UpstreamMap{}) + results := gen.executeServers( + test.config, + &policiesfakes.FakeGenerator{}, + newKeepAliveChecker([]http.Upstream{}), + ) g.Expect(results).To(HaveLen(2)) serverConf := string(results[0].data) httpMatchConf := string(results[1].data) @@ -444,7 +448,11 @@ func TestExecuteServers_RewriteClientIP(t *testing.T) { g := NewWithT(t) gen := GeneratorImpl{} - results := gen.executeServers(test.config, &policiesfakes.FakeGenerator{}, UpstreamMap{}) + results := gen.executeServers( + test.config, + &policiesfakes.FakeGenerator{}, + newKeepAliveChecker([]http.Upstream{}), + ) g.Expect(results).To(HaveLen(2)) serverConf := string(results[0].data) httpMatchConf := string(results[1].data) @@ -486,7 +494,7 @@ func TestExecuteServers_Plus(t *testing.T) { g := NewWithT(t) gen := GeneratorImpl{plus: true} - results := gen.executeServers(config, &policiesfakes.FakeGenerator{}, UpstreamMap{}) + results := gen.executeServers(config, &policiesfakes.FakeGenerator{}, newKeepAliveChecker([]http.Upstream{})) g.Expect(results).To(HaveLen(2)) serverConf := string(results[0].data) @@ -570,7 +578,11 @@ func TestExecuteForDefaultServers(t *testing.T) { g := NewWithT(t) gen := GeneratorImpl{} - serverResults := gen.executeServers(tc.conf, &policiesfakes.FakeGenerator{}, UpstreamMap{}) + serverResults := gen.executeServers( + tc.conf, + &policiesfakes.FakeGenerator{}, + newKeepAliveChecker([]http.Upstream{}), + ) g.Expect(serverResults).To(HaveLen(2)) serverConf := string(serverResults[0].data) httpMatchConf := string(serverResults[1].data) @@ -1481,7 +1493,7 @@ func TestCreateServers(t *testing.T) { Content: []byte("include-1"), }, }) - result, httpMatchPair := createServers(conf, fakeGenerator, UpstreamMap{}) + result, httpMatchPair := createServers(conf, fakeGenerator, newKeepAliveChecker([]http.Upstream{})) g.Expect(httpMatchPair).To(Equal(allExpMatchPair)) g.Expect(helpers.Diff(expectedServers, result)).To(BeEmpty()) @@ -1702,7 +1714,7 @@ func TestCreateServersConflicts(t *testing.T) { result, _ := createServers( dataplane.Configuration{HTTPServers: httpServers}, &policiesfakes.FakeGenerator{}, - UpstreamMap{}, + newKeepAliveChecker([]http.Upstream{}), ) g.Expect(helpers.Diff(expectedServers, result)).To(BeEmpty()) }) @@ -1853,7 +1865,7 @@ func TestCreateServers_Includes(t *testing.T) { conf := dataplane.Configuration{HTTPServers: httpServers, SSLServers: sslServers} - actualServers, matchPairs := createServers(conf, fakeGenerator, UpstreamMap{}) + actualServers, matchPairs := createServers(conf, fakeGenerator, newKeepAliveChecker([]http.Upstream{})) g.Expect(matchPairs).To(BeEmpty()) g.Expect(actualServers).To(HaveLen(len(expServers))) @@ -2014,7 +2026,12 @@ func TestCreateLocations_Includes(t *testing.T) { }, }) - locations, matches, grpc := createLocations(&httpServer, "1", fakeGenerator, UpstreamMap{}) + locations, matches, grpc := createLocations( + &httpServer, + "1", + fakeGenerator, + newKeepAliveChecker([]http.Upstream{}), + ) g := NewWithT(t) g.Expect(grpc).To(BeFalse()) @@ -2204,7 +2221,7 @@ func TestCreateLocationsRootPath(t *testing.T) { }, "1", &policiesfakes.FakeGenerator{}, - UpstreamMap{}, + newKeepAliveChecker([]http.Upstream{}), ) g.Expect(locs).To(Equal(test.expLocations)) g.Expect(httpMatchPair).To(BeEmpty()) @@ -3056,23 +3073,21 @@ func TestGetConnectionHeader(t *testing.T) { tests := []struct { msg string - upstreamMap UpstreamMap + upstreams []http.Upstream expConnectionHeader http.Header backends []dataplane.Backend }{ { msg: "no upstreams with keepAlive enabled", - upstreamMap: UpstreamMap{ - nameToUpstream: map[string]http.Upstream{ - "upstream1": { - Name: "upstream1", - }, - "upstream2": { - Name: "upstream2", - }, - "upstream3": { - Name: "upstream3", - }, + upstreams: []http.Upstream{ + { + Name: "upstream1", + }, + { + Name: "upstream2", + }, + { + Name: "upstream3", }, }, backends: []dataplane.Backend{ @@ -3090,13 +3105,11 @@ func TestGetConnectionHeader(t *testing.T) { }, { msg: "upstream with keepAlive enabled", - upstreamMap: UpstreamMap{ - nameToUpstream: map[string]http.Upstream{ - "upstream": { - Name: "upstream", - KeepAlive: http.UpstreamKeepAlive{ - Connections: 1, - }, + upstreams: []http.Upstream{ + { + Name: "upstream", + KeepAlive: http.UpstreamKeepAlive{ + Connections: 1, }, }, }, @@ -3109,25 +3122,25 @@ func TestGetConnectionHeader(t *testing.T) { }, { msg: "multiple upstreams with keepAlive enabled", - upstreamMap: UpstreamMap{ - nameToUpstream: map[string]http.Upstream{ - "upstream1": { - Name: "upstream1", - KeepAlive: http.UpstreamKeepAlive{ - Connections: 1, - }, + upstreams: []http.Upstream{ + { + Name: "upstream1", + KeepAlive: http.UpstreamKeepAlive{ + Connections: 1, }, - "upstream2": { - Name: "upstream2", - KeepAlive: http.UpstreamKeepAlive{ - Requests: 1, - }, + }, + { + Name: "upstream2", + KeepAlive: http.UpstreamKeepAlive{ + Connections: 2, + Requests: 1, }, - "upstream3": { - Name: "upstream3", - KeepAlive: http.UpstreamKeepAlive{ - Time: "5s", - }, + }, + { + Name: "upstream3", + KeepAlive: http.UpstreamKeepAlive{ + Connections: 3, + Time: "5s", }, }, }, @@ -3147,19 +3160,16 @@ func TestGetConnectionHeader(t *testing.T) { { msg: "mix of upstreams with keepAlive enabled and disabled", expConnectionHeader: unsetHTTPConnectionHeader, - upstreamMap: UpstreamMap{ - nameToUpstream: map[string]http.Upstream{ - "upstream1": { - Name: "upstream1", - KeepAlive: http.UpstreamKeepAlive{ - Connections: 1, - }, - }, - "upstream2": { - Name: "upstream2", - ZoneSize: "2m", + upstreams: []http.Upstream{ + { + Name: "upstream1", + KeepAlive: http.UpstreamKeepAlive{ + Connections: 1, }, }, + { + Name: "upstream2", + }, }, backends: []dataplane.Backend{ { @@ -3177,7 +3187,9 @@ func TestGetConnectionHeader(t *testing.T) { t.Parallel() g := NewWithT(t) - connectionHeader := getConnectionHeader(tc.upstreamMap, tc.backends) + keepAliveCheck := newKeepAliveChecker(tc.upstreams) + + connectionHeader := getConnectionHeader(keepAliveCheck, tc.backends) g.Expect(connectionHeader).To(Equal(tc.expConnectionHeader)) }) } diff --git a/internal/mode/static/nginx/config/upstreams.go b/internal/mode/static/nginx/config/upstreams.go index aca58be21a..6a1503fbf1 100644 --- a/internal/mode/static/nginx/config/upstreams.go +++ b/internal/mode/static/nginx/config/upstreams.go @@ -33,20 +33,23 @@ const ( plusZoneSizeStream = "1m" ) -// UpstreamMap holds a map which maps upstream name to http.Upstream. -type UpstreamMap struct { - nameToUpstream map[string]http.Upstream -} +// keepAliveChecker takes an upstream name and returns if it has keep alive settings enabled. +type keepAliveChecker func(upstreamName string) bool + +func newKeepAliveChecker(upstreams []http.Upstream) keepAliveChecker { + upstreamMap := make(map[string]http.Upstream) -func (um UpstreamMap) keepAliveEnabled(name string) bool { - if upstream, exists := um.nameToUpstream[name]; exists { - return upstream.KeepAlive.Connections != 0 || - upstream.KeepAlive.Requests != 0 || - upstream.KeepAlive.Time != "" || - upstream.KeepAlive.Timeout != "" + for _, upstream := range upstreams { + upstreamMap[upstream.Name] = upstream } - return false + return func(upstreamName string) bool { + if upstream, exists := upstreamMap[upstreamName]; exists { + return upstream.KeepAlive.Connections != 0 + } + + return false + } } func newExecuteUpstreamsFunc(upstreams []http.Upstream) executeFunc { @@ -64,16 +67,6 @@ func executeUpstreams(upstreams []http.Upstream) []executeResult { return []executeResult{result} } -func (g GeneratorImpl) createUpstreamMap(upstreams []http.Upstream) UpstreamMap { - upstreamMap := UpstreamMap{nameToUpstream: make(map[string]http.Upstream)} - - for _, upstream := range upstreams { - upstreamMap.nameToUpstream[upstream.Name] = upstream - } - - return upstreamMap -} - func (g GeneratorImpl) executeStreamUpstreams(conf dataplane.Configuration) []executeResult { upstreams := g.createStreamUpstreams(conf.StreamUpstreams) diff --git a/internal/mode/static/nginx/config/upstreams_test.go b/internal/mode/static/nginx/config/upstreams_test.go index 510c975674..8ce0cbd763 100644 --- a/internal/mode/static/nginx/config/upstreams_test.go +++ b/internal/mode/static/nginx/config/upstreams_test.go @@ -759,115 +759,201 @@ func TestCreateStreamUpstreamPlus(t *testing.T) { g.Expect(result).To(Equal(expectedUpstream)) } -func TestCreateUpstreamMap(t *testing.T) { - t.Parallel() - gen := GeneratorImpl{} - - up1 := http.Upstream{ - Name: "up1", - } - - up2 := http.Upstream{ - Name: "up2", - } - - up3 := http.Upstream{ - Name: "up3", - } - - upstreamMap := gen.createUpstreamMap([]http.Upstream{up1, up2, up3}) - expUpstreamMap := UpstreamMap{ - nameToUpstream: map[string]http.Upstream{ - "up1": up1, - "up2": up2, - "up3": up3, - }, - } - - g := NewWithT(t) - g.Expect(upstreamMap).To(Equal(expUpstreamMap)) -} - -func TestKeepAliveEnabled(t *testing.T) { +func TestKeepAliveChecker(t *testing.T) { t.Parallel() tests := []struct { msg string - upstream http.Upstream - expKeepAliveEnabled bool + upstreams []http.Upstream + expKeepAliveEnabled []bool }{ { msg: "upstream with all keepAlive fields set", - upstream: http.Upstream{ - Name: "upAllKeepAliveFieldsSet", - KeepAlive: http.UpstreamKeepAlive{ - Connections: 1, - Requests: 1, - Time: "5s", - Timeout: "10s", + upstreams: []http.Upstream{ + { + Name: "upAllKeepAliveFieldsSet", + KeepAlive: http.UpstreamKeepAlive{ + Connections: 1, + Requests: 1, + Time: "5s", + Timeout: "10s", + }, }, }, - expKeepAliveEnabled: true, + expKeepAliveEnabled: []bool{ + true, + }, }, { msg: "upstream with keepAlive connection field set", - upstream: http.Upstream{ - Name: "upKeepAliveConnectionsSet", - KeepAlive: http.UpstreamKeepAlive{ - Connections: 1, + upstreams: []http.Upstream{ + { + Name: "upKeepAliveConnectionsSet", + KeepAlive: http.UpstreamKeepAlive{ + Connections: 1, + }, }, }, - expKeepAliveEnabled: true, + expKeepAliveEnabled: []bool{ + true, + }, }, { msg: "upstream with keepAlive requests field set", - upstream: http.Upstream{ - Name: "upKeepAliveRequestsSet", - KeepAlive: http.UpstreamKeepAlive{ - Requests: 1, + upstreams: []http.Upstream{ + { + Name: "upKeepAliveRequestsSet", + KeepAlive: http.UpstreamKeepAlive{ + Requests: 1, + }, }, }, - expKeepAliveEnabled: true, + expKeepAliveEnabled: []bool{ + false, + }, }, { msg: "upstream with keepAlive time field set", - upstream: http.Upstream{ - Name: "upKeepAliveTimeSet", - KeepAlive: http.UpstreamKeepAlive{ - Time: "5s", + upstreams: []http.Upstream{ + { + Name: "upKeepAliveTimeSet", + KeepAlive: http.UpstreamKeepAlive{ + Time: "5s", + }, }, }, - expKeepAliveEnabled: true, + expKeepAliveEnabled: []bool{ + false, + }, }, { msg: "upstream with keepAlive timeout field set", - upstream: http.Upstream{ - Name: "upKeepAliveTimeoutSet", - KeepAlive: http.UpstreamKeepAlive{ - Timeout: "10s", + upstreams: []http.Upstream{ + { + Name: "upKeepAliveTimeoutSet", + KeepAlive: http.UpstreamKeepAlive{ + Timeout: "10s", + }, }, }, - expKeepAliveEnabled: true, + expKeepAliveEnabled: []bool{ + false, + }, }, { msg: "upstream with no keepAlive fields set", - upstream: http.Upstream{ - Name: "upNoKeepAliveFieldsSet", + upstreams: []http.Upstream{ + { + Name: "upNoKeepAliveFieldsSet", + }, + }, + expKeepAliveEnabled: []bool{ + false, }, - expKeepAliveEnabled: false, }, { msg: "upstream with keepAlive fields set to empty values", - upstream: http.Upstream{ - Name: "upNoKeepAliveFieldsSet", - KeepAlive: http.UpstreamKeepAlive{ - Connections: 0, - Requests: 0, - Time: "", - Timeout: "", + upstreams: []http.Upstream{ + { + Name: "upKeepAliveFieldsEmpty", + KeepAlive: http.UpstreamKeepAlive{ + Connections: 0, + Requests: 0, + Time: "", + Timeout: "", + }, + }, + }, + expKeepAliveEnabled: []bool{ + false, + }, + }, + { + msg: "multiple upstreams with keepAlive fields set", + upstreams: []http.Upstream{ + { + Name: "upstream1", + KeepAlive: http.UpstreamKeepAlive{ + Connections: 1, + Requests: 1, + Time: "5s", + Timeout: "10s", + }, + }, + { + Name: "upstream2", + KeepAlive: http.UpstreamKeepAlive{ + Connections: 1, + Requests: 1, + Time: "5s", + Timeout: "10s", + }, + }, + { + Name: "upstream3", + KeepAlive: http.UpstreamKeepAlive{ + Connections: 1, + Requests: 1, + Time: "5s", + Timeout: "10s", + }, + }, + }, + expKeepAliveEnabled: []bool{ + true, + true, + true, + }, + }, + { + msg: "mix of keepAlive enabled upstreams and disabled upstreams", + upstreams: []http.Upstream{ + { + Name: "upstream1", + KeepAlive: http.UpstreamKeepAlive{ + Connections: 1, + Requests: 1, + Time: "5s", + Timeout: "10s", + }, + }, + { + Name: "upstream2", }, + { + Name: "upstream3", + KeepAlive: http.UpstreamKeepAlive{ + Connections: 1, + Requests: 1, + Time: "5s", + Timeout: "10s", + }, + }, + }, + expKeepAliveEnabled: []bool{ + true, + false, + true, + }, + }, + { + msg: "all upstreams without keepAlive fields set", + upstreams: []http.Upstream{ + { + Name: "upstream1", + }, + { + Name: "upstream2", + }, + { + Name: "upstream3", + }, + }, + expKeepAliveEnabled: []bool{ + false, + false, + false, }, - expKeepAliveEnabled: false, }, } @@ -876,13 +962,11 @@ func TestKeepAliveEnabled(t *testing.T) { t.Parallel() g := NewWithT(t) - upstreamMap := UpstreamMap{ - nameToUpstream: map[string]http.Upstream{ - test.upstream.Name: test.upstream, - }, - } + keepAliveCheck := newKeepAliveChecker(test.upstreams) - g.Expect(upstreamMap.keepAliveEnabled(test.upstream.Name)).To(Equal(test.expKeepAliveEnabled)) + for index, upstream := range test.upstreams { + g.Expect(keepAliveCheck(upstream.Name)).To(Equal(test.expKeepAliveEnabled[index])) + } }) } } From b8a28a60d5e2e2e1e9d52efdb42d565f2aa038b1 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Wed, 11 Dec 2024 14:23:29 -0800 Subject: [PATCH 19/23] Add review feedback --- .../policies/upstreamsettings/processor.go | 2 +- .../mode/static/nginx/config/servers_test.go | 51 ++++++++----------- 2 files changed, 23 insertions(+), 30 deletions(-) diff --git a/internal/mode/static/nginx/config/policies/upstreamsettings/processor.go b/internal/mode/static/nginx/config/policies/upstreamsettings/processor.go index b819c28a94..cf7dd0b295 100644 --- a/internal/mode/static/nginx/config/policies/upstreamsettings/processor.go +++ b/internal/mode/static/nginx/config/policies/upstreamsettings/processor.go @@ -8,7 +8,7 @@ import ( // Processor processes UpstreamSettingsPolicies. It implements policies.UpstreamSettingsProcessor. type Processor struct{} -// NewProcessor returns a new instance of Processor. +// NewProcessor returns a new Processor. func NewProcessor() Processor { return Processor{} } diff --git a/internal/mode/static/nginx/config/servers_test.go b/internal/mode/static/nginx/config/servers_test.go index bdf40f8bd8..fc7710b8f1 100644 --- a/internal/mode/static/nginx/config/servers_test.go +++ b/internal/mode/static/nginx/config/servers_test.go @@ -18,8 +18,9 @@ import ( ) var ( - httpBaseHeaders = createBaseProxySetHeaders(httpUpgradeHeader, httpConnectionHeader) - grpcBaseHeaders = createBaseProxySetHeaders(grpcAuthorityHeader) + httpBaseHeaders = createBaseProxySetHeaders(httpUpgradeHeader, httpConnectionHeader) + grpcBaseHeaders = createBaseProxySetHeaders(grpcAuthorityHeader) + alwaysFalseKeepAliveChecker = func(_ string) bool { return false } ) func TestExecuteServers(t *testing.T) { @@ -187,7 +188,7 @@ func TestExecuteServers(t *testing.T) { ) gen := GeneratorImpl{} - results := gen.executeServers(conf, fakeGenerator, newKeepAliveChecker([]http.Upstream{})) + results := gen.executeServers(conf, fakeGenerator, alwaysFalseKeepAliveChecker) g.Expect(results).To(HaveLen(len(expectedResults))) for _, res := range results { @@ -326,11 +327,8 @@ func TestExecuteServers_IPFamily(t *testing.T) { g := NewWithT(t) gen := GeneratorImpl{} - results := gen.executeServers( - test.config, - &policiesfakes.FakeGenerator{}, - newKeepAliveChecker([]http.Upstream{}), - ) + results := gen.executeServers(test.config, &policiesfakes.FakeGenerator{}, alwaysFalseKeepAliveChecker) + g.Expect(results).To(HaveLen(2)) serverConf := string(results[0].data) httpMatchConf := string(results[1].data) @@ -448,11 +446,7 @@ func TestExecuteServers_RewriteClientIP(t *testing.T) { g := NewWithT(t) gen := GeneratorImpl{} - results := gen.executeServers( - test.config, - &policiesfakes.FakeGenerator{}, - newKeepAliveChecker([]http.Upstream{}), - ) + results := gen.executeServers(test.config, &policiesfakes.FakeGenerator{}, alwaysFalseKeepAliveChecker) g.Expect(results).To(HaveLen(2)) serverConf := string(results[0].data) httpMatchConf := string(results[1].data) @@ -494,7 +488,7 @@ func TestExecuteServers_Plus(t *testing.T) { g := NewWithT(t) gen := GeneratorImpl{plus: true} - results := gen.executeServers(config, &policiesfakes.FakeGenerator{}, newKeepAliveChecker([]http.Upstream{})) + results := gen.executeServers(config, &policiesfakes.FakeGenerator{}, alwaysFalseKeepAliveChecker) g.Expect(results).To(HaveLen(2)) serverConf := string(results[0].data) @@ -578,11 +572,7 @@ func TestExecuteForDefaultServers(t *testing.T) { g := NewWithT(t) gen := GeneratorImpl{} - serverResults := gen.executeServers( - tc.conf, - &policiesfakes.FakeGenerator{}, - newKeepAliveChecker([]http.Upstream{}), - ) + serverResults := gen.executeServers(tc.conf, &policiesfakes.FakeGenerator{}, alwaysFalseKeepAliveChecker) g.Expect(serverResults).To(HaveLen(2)) serverConf := string(serverResults[0].data) httpMatchConf := string(serverResults[1].data) @@ -1493,7 +1483,7 @@ func TestCreateServers(t *testing.T) { Content: []byte("include-1"), }, }) - result, httpMatchPair := createServers(conf, fakeGenerator, newKeepAliveChecker([]http.Upstream{})) + result, httpMatchPair := createServers(conf, fakeGenerator, alwaysFalseKeepAliveChecker) g.Expect(httpMatchPair).To(Equal(allExpMatchPair)) g.Expect(helpers.Diff(expectedServers, result)).To(BeEmpty()) @@ -1714,7 +1704,7 @@ func TestCreateServersConflicts(t *testing.T) { result, _ := createServers( dataplane.Configuration{HTTPServers: httpServers}, &policiesfakes.FakeGenerator{}, - newKeepAliveChecker([]http.Upstream{}), + alwaysFalseKeepAliveChecker, ) g.Expect(helpers.Diff(expectedServers, result)).To(BeEmpty()) }) @@ -1865,7 +1855,7 @@ func TestCreateServers_Includes(t *testing.T) { conf := dataplane.Configuration{HTTPServers: httpServers, SSLServers: sslServers} - actualServers, matchPairs := createServers(conf, fakeGenerator, newKeepAliveChecker([]http.Upstream{})) + actualServers, matchPairs := createServers(conf, fakeGenerator, alwaysFalseKeepAliveChecker) g.Expect(matchPairs).To(BeEmpty()) g.Expect(actualServers).To(HaveLen(len(expServers))) @@ -2026,12 +2016,7 @@ func TestCreateLocations_Includes(t *testing.T) { }, }) - locations, matches, grpc := createLocations( - &httpServer, - "1", - fakeGenerator, - newKeepAliveChecker([]http.Upstream{}), - ) + locations, matches, grpc := createLocations(&httpServer, "1", fakeGenerator, alwaysFalseKeepAliveChecker) g := NewWithT(t) g.Expect(grpc).To(BeFalse()) @@ -2221,7 +2206,7 @@ func TestCreateLocationsRootPath(t *testing.T) { }, "1", &policiesfakes.FakeGenerator{}, - newKeepAliveChecker([]http.Upstream{}), + alwaysFalseKeepAliveChecker, ) g.Expect(locs).To(Equal(test.expLocations)) g.Expect(httpMatchPair).To(BeEmpty()) @@ -3055,6 +3040,14 @@ func TestCreateBaseProxySetHeaders(t *testing.T) { }, expBaseHeaders: append(expBaseHeaders, httpConnectionHeader, httpUpgradeHeader), }, + { + msg: "unset connection header and upgrade header", + additionalHeaders: []http.Header{ + unsetHTTPConnectionHeader, + httpUpgradeHeader, + }, + expBaseHeaders: append(expBaseHeaders, unsetHTTPConnectionHeader, httpUpgradeHeader), + }, } for _, test := range tests { From c4895910897015543ba4639c208cee9395af890b Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Wed, 11 Dec 2024 14:27:36 -0800 Subject: [PATCH 20/23] Adjust spacing in upstreams template --- internal/mode/static/nginx/config/upstreams_template.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/internal/mode/static/nginx/config/upstreams_template.go b/internal/mode/static/nginx/config/upstreams_template.go index 8f82fa049b..47fac2dd00 100644 --- a/internal/mode/static/nginx/config/upstreams_template.go +++ b/internal/mode/static/nginx/config/upstreams_template.go @@ -5,14 +5,15 @@ package config // NGINX Plus needs 1m to support roughly the same amount of http servers (556 upstream servers). // For stream upstream servers, 512k will support 576 in OSS and 1m will support 991 in NGINX Plus // https://github.com/nginxinc/nginx-gateway-fabric/issues/483 +// +// if the keepalive directive is present, it is necessary to activate the load balancing method before the directive. const upstreamsTemplateText = ` {{ range $u := . }} upstream {{ $u.Name }} { - # if the keepalive directive us present, it is necessary to activate the load balancing method before the directive random two least_conn; {{ if $u.ZoneSize -}} zone {{ $u.Name }} {{ $u.ZoneSize }}; - {{ end -}} + {{- end }} {{ range $server := $u.Servers }} server {{ $server.Address }}; {{- end }} @@ -38,7 +39,7 @@ upstream {{ $u.Name }} { random two least_conn; {{ if $u.ZoneSize -}} zone {{ $u.Name }} {{ $u.ZoneSize }}; - {{ end -}} + {{- end }} {{ range $server := $u.Servers }} server {{ $server.Address }}; {{- end }} From 65a5db643a2db3fbd39451d6fd930c7ebb7c26cd Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Wed, 11 Dec 2024 14:49:18 -0800 Subject: [PATCH 21/23] Add end to end test for unset connection header --- .../mode/static/nginx/config/servers_test.go | 40 ++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/internal/mode/static/nginx/config/servers_test.go b/internal/mode/static/nginx/config/servers_test.go index fc7710b8f1..24b40d0f60 100644 --- a/internal/mode/static/nginx/config/servers_test.go +++ b/internal/mode/static/nginx/config/servers_test.go @@ -656,6 +656,18 @@ func TestCreateServers(t *testing.T) { }, } + keepAliveGroup := dataplane.BackendGroup{ + Source: hrNsName, + RuleIdx: 4, + Backends: []dataplane.Backend{ + { + UpstreamName: "test_keep_alive_80", + Valid: true, + Weight: 1, + }, + }, + } + filterGroup1 := dataplane.BackendGroup{Source: hrNsName, RuleIdx: 3} filterGroup2 := dataplane.BackendGroup{Source: hrNsName, RuleIdx: 4} @@ -976,6 +988,16 @@ func TestCreateServers(t *testing.T) { }, }, }, + { + Path: "/keep-alive-enabled", + PathType: dataplane.PathTypeExact, + MatchRules: []dataplane.MatchRule{ + { + Match: dataplane.Match{}, + BackendGroup: keepAliveGroup, + }, + }, + }, } conf := dataplane.Configuration{ @@ -1432,6 +1454,13 @@ func TestCreateServers(t *testing.T) { Type: http.InternalLocationType, Includes: internalIncludes, }, + { + Path: "= /keep-alive-enabled", + ProxyPass: "http://test_keep_alive_80$request_uri", + ProxySetHeaders: createBaseProxySetHeaders(httpUpgradeHeader, unsetHTTPConnectionHeader), + Type: http.ExternalLocationType, + Includes: externalIncludes, + }, } } @@ -1483,7 +1512,16 @@ func TestCreateServers(t *testing.T) { Content: []byte("include-1"), }, }) - result, httpMatchPair := createServers(conf, fakeGenerator, alwaysFalseKeepAliveChecker) + + keepAliveEnabledUpstream := http.Upstream{ + Name: "test_keep_alive_80", + KeepAlive: http.UpstreamKeepAlive{ + Connections: 1, + }, + } + keepAliveCheck := newKeepAliveChecker([]http.Upstream{keepAliveEnabledUpstream}) + + result, httpMatchPair := createServers(conf, fakeGenerator, keepAliveCheck) g.Expect(httpMatchPair).To(Equal(allExpMatchPair)) g.Expect(helpers.Diff(expectedServers, result)).To(BeEmpty()) From db05606fd4b72adb02e357947c60cf362bcfa961 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Wed, 11 Dec 2024 15:01:04 -0800 Subject: [PATCH 22/23] Remove UpstreamSettingsProcessor interface and move UpstreamSettings into upstreamsettings package --- .../static/nginx/config/policies/processor.go | 27 -------------- .../policies/upstreamsettings/processor.go | 17 ++++++--- .../upstreamsettings/processor_test.go | 35 ++++++++++--------- .../mode/static/nginx/config/upstreams.go | 6 ++-- 4 files changed, 34 insertions(+), 51 deletions(-) delete mode 100644 internal/mode/static/nginx/config/policies/processor.go diff --git a/internal/mode/static/nginx/config/policies/processor.go b/internal/mode/static/nginx/config/policies/processor.go deleted file mode 100644 index e84d22dcbc..0000000000 --- a/internal/mode/static/nginx/config/policies/processor.go +++ /dev/null @@ -1,27 +0,0 @@ -package policies - -// UpstreamSettingsProcessor is an interface that defines a method for processing a list of policies -// into upstream settings. -type UpstreamSettingsProcessor interface { - Process(policies []Policy) UpstreamSettings -} - -// UpstreamSettings contains settings from UpstreamSettingsPolicy. -type UpstreamSettings struct { - // ZoneSize is the zone size setting. - ZoneSize string - // KeepAlive contains the keepalive settings. - KeepAlive KeepAlive -} - -// KeepAlive contains the keepalive settings. -type KeepAlive struct { - // Time is the keepalive time value. - Time string - // Timeout is the keepalive timeout value. - Timeout string - // Connections is the keepalive connections value. - Connections int32 - // Requests is the keepalive requests value. - Requests int32 -} diff --git a/internal/mode/static/nginx/config/policies/upstreamsettings/processor.go b/internal/mode/static/nginx/config/policies/upstreamsettings/processor.go index cf7dd0b295..5df29eed64 100644 --- a/internal/mode/static/nginx/config/policies/upstreamsettings/processor.go +++ b/internal/mode/static/nginx/config/policies/upstreamsettings/processor.go @@ -2,12 +2,21 @@ package upstreamsettings import ( ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/http" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies" ) -// Processor processes UpstreamSettingsPolicies. It implements policies.UpstreamSettingsProcessor. +// Processor processes UpstreamSettingsPolicies. type Processor struct{} +// UpstreamSettings contains settings from UpstreamSettingsPolicy. +type UpstreamSettings struct { + // ZoneSize is the zone size setting. + ZoneSize string + // KeepAlive contains the keepalive settings. + KeepAlive http.UpstreamKeepAlive +} + // NewProcessor returns a new Processor. func NewProcessor() Processor { return Processor{} @@ -16,12 +25,12 @@ func NewProcessor() Processor { // Process processes policies into an UpstreamSettings object. The policies are already validated and are guaranteed // to not contain overlapping settings. This method merges all fields in the policies into a single UpstreamSettings // object. -func (g Processor) Process(pols []policies.Policy) policies.UpstreamSettings { +func (g Processor) Process(pols []policies.Policy) UpstreamSettings { return processPolicies(pols) } -func processPolicies(pols []policies.Policy) policies.UpstreamSettings { - upstreamSettings := policies.UpstreamSettings{} +func processPolicies(pols []policies.Policy) UpstreamSettings { + upstreamSettings := UpstreamSettings{} for _, pol := range pols { usp, ok := pol.(*ngfAPI.UpstreamSettingsPolicy) diff --git a/internal/mode/static/nginx/config/policies/upstreamsettings/processor_test.go b/internal/mode/static/nginx/config/policies/upstreamsettings/processor_test.go index b4756db60a..b7c785376f 100644 --- a/internal/mode/static/nginx/config/policies/upstreamsettings/processor_test.go +++ b/internal/mode/static/nginx/config/policies/upstreamsettings/processor_test.go @@ -8,6 +8,7 @@ import ( ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/http" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies" ) @@ -16,7 +17,7 @@ func TestProcess(t *testing.T) { tests := []struct { name string - expUpstreamSettings policies.UpstreamSettings + expUpstreamSettings UpstreamSettings policies []policies.Policy }{ { @@ -38,9 +39,9 @@ func TestProcess(t *testing.T) { }, }, }, - expUpstreamSettings: policies.UpstreamSettings{ + expUpstreamSettings: UpstreamSettings{ ZoneSize: "2m", - KeepAlive: policies.KeepAlive{ + KeepAlive: http.UpstreamKeepAlive{ Connections: 1, Requests: 1, Time: "5s", @@ -61,7 +62,7 @@ func TestProcess(t *testing.T) { }, }, }, - expUpstreamSettings: policies.UpstreamSettings{ + expUpstreamSettings: UpstreamSettings{ ZoneSize: "2m", }, }, @@ -80,8 +81,8 @@ func TestProcess(t *testing.T) { }, }, }, - expUpstreamSettings: policies.UpstreamSettings{ - KeepAlive: policies.KeepAlive{ + expUpstreamSettings: UpstreamSettings{ + KeepAlive: http.UpstreamKeepAlive{ Connections: 1, }, }, @@ -101,8 +102,8 @@ func TestProcess(t *testing.T) { }, }, }, - expUpstreamSettings: policies.UpstreamSettings{ - KeepAlive: policies.KeepAlive{ + expUpstreamSettings: UpstreamSettings{ + KeepAlive: http.UpstreamKeepAlive{ Requests: 1, }, }, @@ -122,8 +123,8 @@ func TestProcess(t *testing.T) { }, }, }, - expUpstreamSettings: policies.UpstreamSettings{ - KeepAlive: policies.KeepAlive{ + expUpstreamSettings: UpstreamSettings{ + KeepAlive: http.UpstreamKeepAlive{ Time: "5s", }, }, @@ -143,8 +144,8 @@ func TestProcess(t *testing.T) { }, }, }, - expUpstreamSettings: policies.UpstreamSettings{ - KeepAlive: policies.KeepAlive{ + expUpstreamSettings: UpstreamSettings{ + KeepAlive: http.UpstreamKeepAlive{ Timeout: "10s", }, }, @@ -160,7 +161,7 @@ func TestProcess(t *testing.T) { Spec: ngfAPI.UpstreamSettingsPolicySpec{}, }, }, - expUpstreamSettings: policies.UpstreamSettings{}, + expUpstreamSettings: UpstreamSettings{}, }, { name: "multiple UpstreamSettingsPolicies", @@ -219,9 +220,9 @@ func TestProcess(t *testing.T) { }, }, }, - expUpstreamSettings: policies.UpstreamSettings{ + expUpstreamSettings: UpstreamSettings{ ZoneSize: "2m", - KeepAlive: policies.KeepAlive{ + KeepAlive: http.UpstreamKeepAlive{ Connections: 1, Requests: 1, Time: "5s", @@ -309,9 +310,9 @@ func TestProcess(t *testing.T) { }, }, }, - expUpstreamSettings: policies.UpstreamSettings{ + expUpstreamSettings: UpstreamSettings{ ZoneSize: "2m", - KeepAlive: policies.KeepAlive{ + KeepAlive: http.UpstreamKeepAlive{ Connections: 1, Requests: 1, Time: "5s", diff --git a/internal/mode/static/nginx/config/upstreams.go b/internal/mode/static/nginx/config/upstreams.go index 6a1503fbf1..915001ff64 100644 --- a/internal/mode/static/nginx/config/upstreams.go +++ b/internal/mode/static/nginx/config/upstreams.go @@ -6,7 +6,7 @@ import ( "github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/http" - "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies/upstreamsettings" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/stream" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/dataplane" ) @@ -116,7 +116,7 @@ func (g GeneratorImpl) createStreamUpstream(up dataplane.Upstream) stream.Upstre func (g GeneratorImpl) createUpstreams( upstreams []dataplane.Upstream, - processor policies.UpstreamSettingsProcessor, + processor upstreamsettings.Processor, ) []http.Upstream { // capacity is the number of upstreams + 1 for the invalid backend ref upstream ups := make([]http.Upstream, 0, len(upstreams)+1) @@ -132,7 +132,7 @@ func (g GeneratorImpl) createUpstreams( func (g GeneratorImpl) createUpstream( up dataplane.Upstream, - processor policies.UpstreamSettingsProcessor, + processor upstreamsettings.Processor, ) http.Upstream { upstreamPolicySettings := processor.Process(up.Policies) From dd3f17007030d3bb1832b12a73c5a38fdb78c620 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Wed, 11 Dec 2024 15:20:23 -0800 Subject: [PATCH 23/23] Refactor small code fix --- internal/mode/static/nginx/config/upstreams.go | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/internal/mode/static/nginx/config/upstreams.go b/internal/mode/static/nginx/config/upstreams.go index 915001ff64..7722f6f0a9 100644 --- a/internal/mode/static/nginx/config/upstreams.go +++ b/internal/mode/static/nginx/config/upstreams.go @@ -169,15 +169,10 @@ func (g GeneratorImpl) createUpstream( } return http.Upstream{ - Name: up.Name, - ZoneSize: zoneSize, - Servers: upstreamServers, - KeepAlive: http.UpstreamKeepAlive{ - Connections: upstreamPolicySettings.KeepAlive.Connections, - Requests: upstreamPolicySettings.KeepAlive.Requests, - Time: upstreamPolicySettings.KeepAlive.Time, - Timeout: upstreamPolicySettings.KeepAlive.Timeout, - }, + Name: up.Name, + ZoneSize: zoneSize, + Servers: upstreamServers, + KeepAlive: upstreamPolicySettings.KeepAlive, } }