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 active health checks to TransportServer #1384

Merged
merged 9 commits into from
Feb 25, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,24 @@ spec:
properties:
failTimeout:
type: string
healthCheck:
description: HealthCheck defines the parameters for active Upstream HealthChecks.
type: object
properties:
enable:
type: boolean
fails:
type: integer
intervals:
type: string
jitter:
type: string
passes:
type: integer
port:
type: integer
timeout:
type: string
maxFails:
type: integer
name:
Expand Down
18 changes: 18 additions & 0 deletions deployments/common/crds/k8s.nginx.org_transportservers.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,24 @@ spec:
properties:
failTimeout:
type: string
healthCheck:
description: HealthCheck defines the parameters for active Upstream HealthChecks.
type: object
properties:
enable:
type: boolean
fails:
type: integer
intervals:
type: string
jitter:
type: string
passes:
type: integer
port:
type: integer
timeout:
type: string
maxFails:
type: integer
name:
Expand Down
18 changes: 18 additions & 0 deletions deployments/helm-chart/crds/k8s.nginx.org_transportservers.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,24 @@ spec:
properties:
failTimeout:
type: string
healthCheck:
description: HealthCheck defines the parameters for active Upstream HealthChecks.
type: object
properties:
enable:
type: boolean
fails:
type: integer
intervals:
type: string
jitter:
type: string
passes:
type: integer
port:
type: integer
timeout:
type: string
maxFails:
type: integer
name:
Expand Down
62 changes: 62 additions & 0 deletions docs-web/configuration/transportserver-resource.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ This document is the reference documentation for the TransportServer resource. T
- [TransportServer Specification](#transportserver-specification)
- [Listener](#listener)
- [Upstream](#upstream)
- [Upstream.Healthcheck](#upstream-healthcheck)
- [UpstreamParameters](#upstreamparameters)
- [SessionParameters](#sessionparameters)
- [Action](#action)
Expand Down Expand Up @@ -189,7 +190,68 @@ failTimeout: 30s
- Sets the `time <https://nginx.org/en/docs/stream/ngx_stream_upstream_module.html#fail_timeout>`_ during which the specified number of unsuccessful attempts to communicate with the server should happen to consider the server unavailable and the period of time the server will be considered unavailable. The default is ``10s``.
- ``string``
- No
* - ``healthCheck``
- The health check configuration for the Upstream. See the `health_check <https://nginx.org/en/docs/stream/ngx_stream_upstream_hc_module.html#health_check>`_ directive. Note: this feature is supported only in NGINX Plus.
- `healthcheck <#upstream-healthcheck>`_
- No

```

### Upstream.Healthcheck
LorcanMcVeigh marked this conversation as resolved.
Show resolved Hide resolved

The Healthcheck defines an [active health check](https://nginx.org/en/docs/stream/ngx_stream_upstream_hc_module.html?#health_check). In the example below we enable a health check for an upstream and configure all the available parameters:

```yaml
name: secure-app
service: secure-app
port: 8443
healthCheck:
enable: true
interval: 20s
timeout: 30s
jitter: 3s
fails: 5
passes: 5
port: 8080

```

```eval_rst
.. list-table::
:header-rows: 1

* - Field
- Description
- Type
- Required
* - ``enable``
- Enables a health check for an upstream server. The default is ``false``.
- ``boolean``
- No
* - ``interval``
- The interval between two consecutive health checks. The default is ``5s``.
- ``string``
- No
* - ``timeout``
- This overrides the timeout set by `proxy_timeout <http://nginx.org/en/docs/stream/ngx_stream_proxy_module.html#proxy_timeout>`_ which is set in `SessionParameters` for health checks. The default value is ``5s``.
- ``string``
- No
* - ``jitter``
- The time within which each health check will be randomly delayed. By default, there is no delay.
- ``string``
- No
* - ``fails``
- The number of consecutive failed health checks of a particular upstream server after which this server will be considered unhealthy. The default is ``1``.
- ``integer``
- No
* - ``passes``
- The number of consecutive passed health checks of a particular upstream server after which the server will be considered healthy. The default is ``1``.
- ``integer``
- No
* - ``port``
- The port used for health check requests. By default, the port of the upstream is used. Note: in contrast with the port of the upstream, this port is not a service port, but a port of a pod.
- ``integer``
- No
```

### UpstreamParameters
Expand Down
45 changes: 45 additions & 0 deletions internal/configs/transportserver.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ func generateTransportServerConfig(transportServerEx *TransportServerEx, listene

upstreams := generateStreamUpstreams(transportServerEx, upstreamNamer, isPlus)

healthCheck := generateTransportServerHealthCheck(transportServerEx.TransportServer.Spec.Action.Pass,
transportServerEx.TransportServer.Spec.Upstreams)
var proxyRequests, proxyResponses *int
var connectTimeout, nextUpstreamTimeout string
var nextUpstream bool
Expand Down Expand Up @@ -80,6 +82,7 @@ func generateTransportServerConfig(transportServerEx *TransportServerEx, listene
ProxyNextUpstream: nextUpstream,
ProxyNextUpstreamTimeout: generateTime(nextUpstreamTimeout, "0"),
ProxyNextUpstreamTries: nextUpstreamTries,
HealthCheck: healthCheck,
},
Upstreams: upstreams,
}
Expand Down Expand Up @@ -115,6 +118,48 @@ func generateStreamUpstreams(transportServerEx *TransportServerEx, upstreamNamer
return upstreams
}

func generateTransportServerHealthCheck(upstreamHealthCheckName string, upstreams []conf_v1alpha1.Upstream) *version2.StreamHealthCheck {
var hc *version2.StreamHealthCheck
for _, u := range upstreams {
if u.Name == upstreamHealthCheckName {
if u.HealthCheck == nil || !u.HealthCheck.Enabled {
return nil
}
hc = generateTransportServerHealthCheckWithDefaults(u)

hc.Enabled = u.HealthCheck.Enabled
hc.Interval = generateTime(u.HealthCheck.Interval)
hc.Jitter = generateTime(u.HealthCheck.Jitter)
hc.Timeout = generateTime(u.HealthCheck.Timeout)

if u.HealthCheck.Fails > 0 {
hc.Fails = u.HealthCheck.Fails
}

if u.HealthCheck.Passes > 0 {
hc.Passes = u.HealthCheck.Passes
}

if u.HealthCheck.Port > 0 {
hc.Port = u.HealthCheck.Port
}
}
}
return hc
}

func generateTransportServerHealthCheckWithDefaults(up conf_v1alpha1.Upstream) *version2.StreamHealthCheck {
return &version2.StreamHealthCheck{
Enabled: false,
Timeout: "5s",
Jitter: "0",
Port: up.Port,
Interval: "5s",
Passes: 1,
Fails: 1,
}
}

func generateStreamUpstream(upstream *conf_v1alpha1.Upstream, upstreamNamer *upstreamNamer, endpoints []string, isPlus bool) version2.StreamUpstream {
var upsServers []version2.StreamUpstreamServer

Expand Down
120 changes: 117 additions & 3 deletions internal/configs/transportserver_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"reflect"
"testing"

"github.com/google/go-cmp/cmp"
Copy link
Contributor

Choose a reason for hiding this comment

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

I mentioned previously about the pointer override, assuming this is not intended behaviour, can we add a test to confirm that two upstreams generates two health checks ? I think there's multiple health checks in the tests below, but some of them are invalid ?

"github.com/nginxinc/kubernetes-ingress/internal/configs/version2"
conf_v1alpha1 "github.com/nginxinc/kubernetes-ingress/pkg/apis/configuration/v1alpha1"
meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -134,6 +135,7 @@ func TestGenerateTransportServerConfigForTCP(t *testing.T) {
ProxyNextUpstreamTries: 0,
ProxyNextUpstreamTimeout: "0s",
ProxyTimeout: "50s",
HealthCheck: nil,
},
}

Expand Down Expand Up @@ -217,6 +219,7 @@ func TestGenerateTransportServerConfigForTLSPasstrhough(t *testing.T) {
ProxyNextUpstreamTimeout: "0s",
ProxyNextUpstreamTries: 0,
ProxyTimeout: "10m",
HealthCheck: nil,
},
}

Expand Down Expand Up @@ -244,9 +247,10 @@ func TestGenerateTransportServerConfigForUDP(t *testing.T) {
},
Upstreams: []conf_v1alpha1.Upstream{
{
Name: "udp-app",
Service: "udp-app-svc",
Port: 5001,
Name: "udp-app",
Service: "udp-app-svc",
Port: 5001,
HealthCheck: &conf_v1alpha1.HealthCheck{},
},
},
UpstreamParameters: &conf_v1alpha1.UpstreamParameters{
Expand Down Expand Up @@ -304,6 +308,7 @@ func TestGenerateTransportServerConfigForUDP(t *testing.T) {
ProxyNextUpstreamTimeout: "0s",
ProxyNextUpstreamTries: 0,
ProxyTimeout: "10m",
HealthCheck: nil,
},
}

Expand Down Expand Up @@ -345,6 +350,115 @@ func TestGenerateUnixSocket(t *testing.T) {
}
}

func TestGenerateTransportServerHealthChecks(t *testing.T) {
upstreamName := "dns-tcp"
tests := []struct {
upstreams []conf_v1alpha1.Upstream
expected *version2.StreamHealthCheck
msg string
}{
LorcanMcVeigh marked this conversation as resolved.
Show resolved Hide resolved
{
upstreams: []conf_v1alpha1.Upstream{
{
Name: "dns-tcp",
HealthCheck: &conf_v1alpha1.HealthCheck{
Enabled: false,
Timeout: "30s",
Jitter: "30s",
Port: 80,
Interval: "20s",
Passes: 4,
Fails: 5,
},
},
},
expected: nil,
msg: "health checks disabled",
},
{
upstreams: []conf_v1alpha1.Upstream{
{
Name: "dns-tcp",
HealthCheck: &conf_v1alpha1.HealthCheck{},
},
},
expected: nil,
msg: "empty health check",
},
{
upstreams: []conf_v1alpha1.Upstream{
{
Name: "dns-tcp",
HealthCheck: &conf_v1alpha1.HealthCheck{
Enabled: true,
Timeout: "40s",
Jitter: "30s",
Port: 88,
Interval: "20s",
Passes: 4,
Fails: 5,
},
},
},
expected: &version2.StreamHealthCheck{
Enabled: true,
Timeout: "40s",
Jitter: "30s",
Port: 88,
Interval: "20s",
Passes: 4,
Fails: 5,
},
msg: "valid health checks",
},
{
upstreams: []conf_v1alpha1.Upstream{
{
Name: "dns-tcp",
HealthCheck: &conf_v1alpha1.HealthCheck{
Enabled: true,
Timeout: "40s",
Jitter: "30s",
Port: 88,
Interval: "20s",
Passes: 4,
Fails: 5,
},
},
{
Name: "dns-tcp-2",
HealthCheck: &conf_v1alpha1.HealthCheck{
Enabled: false,
Timeout: "50s",
Jitter: "60s",
Port: 89,
Interval: "10s",
Passes: 9,
Fails: 7,
},
},
},
expected: &version2.StreamHealthCheck{
Enabled: true,
Timeout: "40s",
Jitter: "30s",
Port: 88,
Interval: "20s",
Passes: 4,
Fails: 5,
},
msg: "valid 2 health checks",
},
}

for _, test := range tests {
result := generateTransportServerHealthCheck(upstreamName, test.upstreams)
if diff := cmp.Diff(test.expected, result); diff != "" {
t.Errorf("generateTransportServerHealthCheck() '%v' mismatch (-want +got):\n%s", test.msg, diff)
}
}
}

func intPointer(value int) *int {
return &value
}
6 changes: 6 additions & 0 deletions internal/configs/version2/nginx-plus.transportserver.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,12 @@ server {

proxy_pass {{ $s.ProxyPass }};

{{ if $s.HealthCheck }}
health_check interval={{ $s.HealthCheck.Interval }} port={{ $s.HealthCheck.Port }}
passes={{ $s.HealthCheck.Passes }} jitter={{ $s.HealthCheck.Jitter }} fails={{ $s.HealthCheck.Fails }} {{ if $s.UDP }}udp{{ end }};
health_check_timeout {{ $s.HealthCheck.Timeout }};
{{ end }}

proxy_timeout {{ $s.ProxyTimeout }};
proxy_connect_timeout {{ $s.ProxyConnectTimeout }};

Expand Down
Loading