Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add NGINX configuration for UpstreamSettingsPolicy #2877

Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
78d1f01
Add NGINX configuration for UpstreamSettingsPolicy
bjee19 Dec 6, 2024
d39f4c0
Fix naming conventions, test messages, and add more comments
bjee19 Dec 9, 2024
1053ca2
Remove typo
bjee19 Dec 9, 2024
5f7eee7
Change funtionality so connection header value is empty when keep ali…
bjee19 Dec 9, 2024
a420810
Update generated files
bjee19 Dec 9, 2024
b188ef6
Add feedback for upstreams template
bjee19 Dec 10, 2024
939e782
Adjust comment in upstreams template
bjee19 Dec 10, 2024
032a6bc
Add feedback for naming of variables and fields
bjee19 Dec 10, 2024
7c403dd
Add upstreams test case for empty policy
bjee19 Dec 10, 2024
3618fbd
Add processor test case for multiple policies
bjee19 Dec 10, 2024
042cce5
Refactor generating proxy set headers
bjee19 Dec 10, 2024
95d3f8d
Add processor test case for processing non-UpstreamSettingsPolicies
bjee19 Dec 10, 2024
03f3c7e
Add another upstream test case
bjee19 Dec 10, 2024
c54b874
Fix upstream test case
bjee19 Dec 10, 2024
c53c24b
Extract keepalive settings outside of UpstreamSettings
bjee19 Dec 10, 2024
02705e0
Turn executeUpstreams into function
bjee19 Dec 10, 2024
513351e
Add UpstreamKeepAlive struct for http Upstream
bjee19 Dec 11, 2024
a2ace83
Add keepAliveChecker function
bjee19 Dec 11, 2024
b8a28a6
Add review feedback
bjee19 Dec 11, 2024
c489591
Adjust spacing in upstreams template
bjee19 Dec 11, 2024
65a5db6
Add end to end test for unset connection header
bjee19 Dec 11, 2024
db05606
Remove UpstreamSettingsProcessor interface and move UpstreamSettings …
bjee19 Dec 11, 2024
dd3f170
Refactor small code fix
bjee19 Dec 11, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions apis/v1alpha1/policy_methods.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand Down
17 changes: 13 additions & 4 deletions internal/mode/static/nginx/config/generator.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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())
bjee19 marked this conversation as resolved.
Show resolved Hide resolved
upstreamMap := g.createUpstreamMap(upstreams)
kate-osborn marked this conversation as resolved.
Show resolved Hide resolved

for _, execute := range g.getExecuteFuncs(generator, upstreams, upstreamMap) {
results := execute(conf)
for _, res := range results {
fileBytes[res.dest] = append(fileBytes[res.dest], res.data...)
Expand All @@ -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,
Expand Down
14 changes: 10 additions & 4 deletions internal/mode/static/nginx/config/http/config.go
Original file line number Diff line number Diff line change
@@ -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"
Expand Down Expand Up @@ -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
bjee19 marked this conversation as resolved.
Show resolved Hide resolved
KeepAliveTimeout string
Servers []UpstreamServer
KeepAliveConnections int32
KeepAliveRequests int32
}

// UpstreamServer holds all configuration for an HTTP upstream server.
Expand Down
21 changes: 21 additions & 0 deletions internal/mode/static/nginx/config/policies/processor.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
package policies

// UpstreamSettingsProcessor defines an interface for an UpstreamSettingsPolicy processor
// to implement the process function.
bjee19 marked this conversation as resolved.
Show resolved Hide resolved
type UpstreamSettingsProcessor interface {
kate-osborn marked this conversation as resolved.
Show resolved Hide resolved
Process(policies []Policy) UpstreamSettings
}

// UpstreamSettings contains settings from UpstreamSettingsPolicy.
type UpstreamSettings struct {
// ZoneSize is the zone size setting.
ZoneSize string
// KeepAliveTime is the keep-alive time setting.
KeepAliveTime string
bjee19 marked this conversation as resolved.
Show resolved Hide resolved
// 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
}
Original file line number Diff line number Diff line change
@@ -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.
bjee19 marked this conversation as resolved.
Show resolved Hide resolved
type Processor struct{}

// NewProcessor returns a new instance of Processor.
bjee19 marked this conversation as resolved.
Show resolved Hide resolved
func NewProcessor() *Processor {
kate-osborn marked this conversation as resolved.
Show resolved Hide resolved
return &Processor{}
}

// Process processes policy configuration for an upstream block.
bjee19 marked this conversation as resolved.
Show resolved Hide resolved
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

Check warning on line 27 in internal/mode/static/nginx/config/policies/upstreamsettings/processor.go

View check run for this annotation

Codecov / codecov/patch

internal/mode/static/nginx/config/policies/upstreamsettings/processor.go#L27

Added line #L27 was not covered by tests
bjee19 marked this conversation as resolved.
Show resolved Hide resolved
}

// 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
}
Original file line number Diff line number Diff line change
@@ -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: "zone size 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: "keep alive 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: "keep alive 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: "keep alive 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: "keep alive 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",
bjee19 marked this conversation as resolved.
Show resolved Hide resolved
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))
})
}
}
Loading
Loading