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 keepalive support to vs/vsr #617

Merged
merged 8 commits into from
Jul 10, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
2 changes: 2 additions & 0 deletions docs/virtualserver-and-virtualserverroute.md
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,7 @@ port: 80
lb-method: round_robin
fail-timeout: 10s
max-fails: 1
keepalive: 32
connect-timeout: 30s
read-timeout: 30s
send-timeout: 30s
Expand All @@ -192,6 +193,7 @@ send-timeout: 30s
| `lb-method` | The load [balancing method](https://docs.nginx.com/nginx/admin-guide/load-balancer/http-load-balancer/#choosing-a-load-balancing-method). To use the round-robin method, specify `round_robin`. The default is specified in the `lb-method` ConfigMap key. | `string` | No |
| `fail-timeout` | The time during which the specified number of unsuccessful attempts to communicate with an upstream server should happen to consider the server unavailable. See the [fail_timeout](https://nginx.org/en/docs/http/ngx_http_upstream_module.html#fail_timeout) parameter of the server directive. The default is set in the `fail-timeout` ConfigMap key. | `string` | No |
| `max-fails` | The number of unsuccessful attempts to communicate with an upstream server that should happen in the duration set by the `fail-timeout` to consider the server unavailable. See the [max_fails](https://nginx.org/en/docs/http/ngx_http_upstream_module.html#max_fails) parameter of the server directive. The default is set in the `max-fails` ConfgMap key. | `int` | No |
| `keepalive` | Configures the cache for connections to upstream servers. See the [keepalive](https://nginx.org/en/docs/http/ngx_http_upstream_module.html#keepalive) directive. The default is set in the `keepalive` ConfigMap key. | `int` | No
Dean-Coakley marked this conversation as resolved.
Show resolved Hide resolved
`connect-timeout` | The timeout for establishing a connection with an upstream server. See the [proxy_connect_timeout](https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_connect_timeout) directive. The default is specified in the `proxy-connect-timeout` ConfigMap key. | `string` | No
`read-timeout` | The timeout for reading a response from an upstream server. See the [proxy_read_timeout](https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_read_timeout) directive. The default is specified in the `proxy-read-timeout` ConfigMap key. | `string` | No
`send-timeout` | The timeout for transmitting a request to an upstream server. See the [proxy_send_timeout](https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_send_timeout) directive. The default is `60s`. | `string` | No
Expand Down
7 changes: 4 additions & 3 deletions internal/configs/version2/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,10 @@ type VirtualServerConfig struct {

// Upstream defines an upstream.
type Upstream struct {
Name string
Servers []UpstreamServer
LBMethod string
Name string
Servers []UpstreamServer
LBMethod string
Keepalive int64
}

// UpstreamServer defines an upstream server.
Expand Down
6 changes: 4 additions & 2 deletions internal/configs/version2/nginx.virtualserver.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,10 @@ upstream {{ $u.Name }} {
server {{ $s.Address }} max_fails={{ $s.MaxFails }} fail_timeout={{ $s.FailTimeout }};
{{ end }}

{{ if $.Keepalive }}
keepalive {{ $.Keepalive }};
{{ if $u.Keepalive }}
Dean-Coakley marked this conversation as resolved.
Show resolved Hide resolved
keepalive {{ $u.Keepalive }};
{{ else }}
{{ if $.Keepalive }}keepalive {{ $.Keepalive }};{{ end }}
Dean-Coakley marked this conversation as resolved.
Show resolved Hide resolved
{{ end }}
}
{{ end }}
Expand Down
14 changes: 11 additions & 3 deletions internal/configs/virtualserver.go
Original file line number Diff line number Diff line change
Expand Up @@ -226,9 +226,10 @@ func generateUpstream(upstreamName string, upstream conf_v1alpha1.Upstream, endp
}

return version2.Upstream{
Name: upstreamName,
Servers: upsServers,
LBMethod: generateLBMethod(upstream.LBMethod, cfgParams.LBMethod),
Name: upstreamName,
Servers: upsServers,
LBMethod: generateLBMethod(upstream.LBMethod, cfgParams.LBMethod),
Keepalive: generatePositiveInt64FromPointer(upstream.Keepalive, cfgParams.Keepalive),
}
}

Expand All @@ -255,6 +256,13 @@ func generatePositiveIntFromPointer(n *int, defaultN int) int {
return *n
}

func generatePositiveInt64FromPointer(n *int64, defaultN int64) int64 {
if n == nil {
return defaultN
}
return *n
}

func generateLocation(path string, upstreamName string, upstream conf_v1alpha1.Upstream, cfgParams *ConfigParams) version2.Location {
return version2.Location{
Path: path,
Expand Down
6 changes: 5 additions & 1 deletion internal/configs/virtualserver_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,7 @@ func TestGenerateVirtualServerConfig(t *testing.T) {
Address: "10.0.0.20:80",
},
},
Keepalive: 16,
},
{
Name: "vs_default_cafe_vsr_default_coffee_coffee",
Expand All @@ -248,6 +249,7 @@ func TestGenerateVirtualServerConfig(t *testing.T) {
Address: "10.0.0.30:80",
},
},
Keepalive: 16,
},
},
Server: version2.Server{
Expand Down Expand Up @@ -739,6 +741,7 @@ func TestGenerateUpstream(t *testing.T) {
LBMethod: "random",
MaxFails: 1,
FailTimeout: "10s",
Keepalive: 21,
}

expected := version2.Upstream{
Expand All @@ -750,7 +753,8 @@ func TestGenerateUpstream(t *testing.T) {
FailTimeout: "10s",
},
},
LBMethod: "random",
LBMethod: "random",
Keepalive: 21,
}

result := generateUpstream(name, upstream, endpoints, isPlus, &cfgParams)
Expand Down
1 change: 1 addition & 0 deletions pkg/apis/configuration/v1alpha1/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ type Upstream struct {
LBMethod string `json:"lb-method"`
FailTimeout string `json:"fail-timeout"`
MaxFails *int `json:"max-fails"`
Keepalive *int64 `json:"keepalive"`
Dean-Coakley marked this conversation as resolved.
Show resolved Hide resolved
Dean-Coakley marked this conversation as resolved.
Show resolved Hide resolved
ProxyConnectTimeout string `json:"connect-timeout"`
ProxyReadTimeout string `json:"read-timeout"`
ProxySendTimeout string `json:"send-timeout"`
Expand Down
5 changes: 5 additions & 0 deletions pkg/apis/configuration/v1alpha1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

22 changes: 22 additions & 0 deletions pkg/apis/configuration/validation/validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,19 @@ func validatePositiveIntOrZero(n *int, fieldPath *field.Path) field.ErrorList {
return allErrs
}

func validatePositiveInt64OrZero(n *int64, fieldPath *field.Path) field.ErrorList {
allErrs := field.ErrorList{}
if n == nil {
return allErrs
}

if *n < 0 {
return append(allErrs, field.Invalid(fieldPath, n, "must be positive or zero"))
}

return allErrs
}

func validateTime(time string, fieldPath *field.Path) field.ErrorList {
allErrs := field.ErrorList{}

Expand Down Expand Up @@ -105,6 +118,14 @@ func validateUpstreamLBMethod(lBMethod string, fieldPath *field.Path, isPlus boo
return allErrs
}

func validatePositiveInt64(num int64, fieldPath *field.Path) field.ErrorList {
Dean-Coakley marked this conversation as resolved.
Show resolved Hide resolved
allErrs := field.ErrorList{}
if num < 0 {
return append(allErrs, field.Invalid(fieldPath, num, fmt.Errorf("Integer must be positive").Error()))
}
return allErrs
}

// validateSecretName checks if a secret name is valid.
// It performs the same validation as ValidateSecretName from k8s.io/kubernetes/pkg/apis/core/validation/validation.go.
func validateSecretName(name string, fieldPath *field.Path) field.ErrorList {
Expand Down Expand Up @@ -146,6 +167,7 @@ func validateUpstreams(upstreams []v1alpha1.Upstream, fieldPath *field.Path, isP
allErrs = append(allErrs, validateUpstreamLBMethod(u.LBMethod, idxPath.Child("lb-method"), isPlus)...)
allErrs = append(allErrs, validateTime(u.FailTimeout, idxPath.Child("fail-timeout"))...)
allErrs = append(allErrs, validatePositiveIntOrZero(u.MaxFails, idxPath.Child("max-fails"))...)
allErrs = append(allErrs, validatePositiveInt64OrZero(u.Keepalive, idxPath.Child("keepalive"))...)

for _, msg := range validation.IsValidPortNum(int(u.Port)) {
allErrs = append(allErrs, field.Invalid(idxPath.Child("port"), u.Port, msg))
Expand Down
56 changes: 49 additions & 7 deletions pkg/apis/configuration/validation/validation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,14 @@ package validation
import (
"testing"

"k8s.io/apimachinery/pkg/util/sets"

"github.com/nginxinc/kubernetes-ingress/pkg/apis/configuration/v1alpha1"
meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/apimachinery/pkg/util/validation/field"
)

func TestValidateVirtualServer(t *testing.T) {
var keepalive int64 = 32
virtualServer := v1alpha1.VirtualServer{
ObjectMeta: meta_v1.ObjectMeta{
Name: "cafe",
Expand All @@ -23,10 +23,11 @@ func TestValidateVirtualServer(t *testing.T) {
},
Upstreams: []v1alpha1.Upstream{
{
Name: "first",
Service: "service-1",
LBMethod: "random",
Port: 80,
Name: "first",
Service: "service-1",
LBMethod: "random",
Port: 80,
Keepalive: &keepalive,
Dean-Coakley marked this conversation as resolved.
Show resolved Hide resolved
},
{
Name: "second",
Expand Down Expand Up @@ -1160,7 +1161,7 @@ func TestIsValidMatchValue(t *testing.T) {
validValues := []string{
"abc",
"123",
`\"
`\"
abc\"`,
`\"`,
}
Expand Down Expand Up @@ -1477,6 +1478,47 @@ func TestValidatePositiveIntOrZeroFails(t *testing.T) {
if len(allErrs) == 0 {
t.Error("validatePositiveInt returned no errors for case: invalid (-1)")
}
}

func createPointerFromInt64(n int64) *int64 {
return &n
}

func TestValidatePositiveInt64OrZero(t *testing.T) {
tests := []struct {
number *int64
msg string
}{
{
number: nil,
msg: "valid (nil)",
},
{
number: createPointerFromInt64(0),
msg: "valid (0)",
},
{
number: createPointerFromInt64(1),
msg: "valid (1)",
},
}

for _, test := range tests {
allErrs := validatePositiveInt64OrZero(test.number, field.NewPath("int-field"))

if len(allErrs) != 0 {
t.Errorf("validatePositiveInt64OrZero returned errors for case: %v", test.msg)
}
}
}

func TestValidatePositiveInt64OrZeroFails(t *testing.T) {
number := createPointerFromInt64(-1)
allErrs := validatePositiveInt64OrZero(number, field.NewPath("int-field"))

if len(allErrs) == 0 {
t.Error("validatePositiveInt64OrZero returned no errors for case: invalid (-1)")
}

}

Expand Down