From 1532ee71261f3a878931a8447f6b95d59c9622b3 Mon Sep 17 00:00:00 2001 From: Ciara Stacke Date: Tue, 7 Dec 2021 14:37:41 +0000 Subject: [PATCH] Enable setting mandatory and persistent in healthchecks --- .../k8s.nginx.org_virtualserverroutes.yaml | 4 +++ .../crds/k8s.nginx.org_virtualservers.yaml | 4 +++ .../k8s.nginx.org_virtualserverroutes.yaml | 4 +++ .../crds/k8s.nginx.org_virtualservers.yaml | 4 +++ ...server-and-virtualserverroute-resources.md | 7 ++++- internal/configs/version2/http.go | 2 ++ .../version2/nginx-plus.virtualserver.tmpl | 1 + internal/configs/version2/templates_test.go | 18 +++++++----- internal/configs/virtualserver.go | 4 +++ internal/configs/virtualserver_test.go | 29 +++++++++++++++++++ pkg/apis/configuration/v1/types.go | 2 ++ .../configuration/validation/virtualserver.go | 4 +++ .../validation/virtualserver_test.go | 9 ++++++ ...plus-virtual-server-with-invalid-keys.yaml | 1 + .../test_virtual_server_upstream_options.py | 6 ++-- 15 files changed, 87 insertions(+), 12 deletions(-) diff --git a/deployments/common/crds/k8s.nginx.org_virtualserverroutes.yaml b/deployments/common/crds/k8s.nginx.org_virtualserverroutes.yaml index b8645f5582..65db6d7ddd 100644 --- a/deployments/common/crds/k8s.nginx.org_virtualserverroutes.yaml +++ b/deployments/common/crds/k8s.nginx.org_virtualserverroutes.yaml @@ -511,10 +511,14 @@ spec: type: string jitter: type: string + mandatory: + type: boolean passes: type: integer path: type: string + persistent: + type: boolean port: type: integer read-timeout: diff --git a/deployments/common/crds/k8s.nginx.org_virtualservers.yaml b/deployments/common/crds/k8s.nginx.org_virtualservers.yaml index ad7cefb4fb..ad126fb3ca 100644 --- a/deployments/common/crds/k8s.nginx.org_virtualservers.yaml +++ b/deployments/common/crds/k8s.nginx.org_virtualservers.yaml @@ -543,10 +543,14 @@ spec: type: string jitter: type: string + mandatory: + type: boolean passes: type: integer path: type: string + persistent: + type: boolean port: type: integer read-timeout: diff --git a/deployments/helm-chart/crds/k8s.nginx.org_virtualserverroutes.yaml b/deployments/helm-chart/crds/k8s.nginx.org_virtualserverroutes.yaml index b8645f5582..65db6d7ddd 100644 --- a/deployments/helm-chart/crds/k8s.nginx.org_virtualserverroutes.yaml +++ b/deployments/helm-chart/crds/k8s.nginx.org_virtualserverroutes.yaml @@ -511,10 +511,14 @@ spec: type: string jitter: type: string + mandatory: + type: boolean passes: type: integer path: type: string + persistent: + type: boolean port: type: integer read-timeout: diff --git a/deployments/helm-chart/crds/k8s.nginx.org_virtualservers.yaml b/deployments/helm-chart/crds/k8s.nginx.org_virtualservers.yaml index ad7cefb4fb..ad126fb3ca 100644 --- a/deployments/helm-chart/crds/k8s.nginx.org_virtualservers.yaml +++ b/deployments/helm-chart/crds/k8s.nginx.org_virtualservers.yaml @@ -543,10 +543,14 @@ spec: type: string jitter: type: string + mandatory: + type: boolean passes: type: integer path: type: string + persistent: + type: boolean port: type: integer read-timeout: diff --git a/docs/content/configuration/virtualserver-and-virtualserverroute-resources.md b/docs/content/configuration/virtualserver-and-virtualserverroute-resources.md index 7a196d17dd..c0c7069cfb 100644 --- a/docs/content/configuration/virtualserver-and-virtualserverroute-resources.md +++ b/docs/content/configuration/virtualserver-and-virtualserverroute-resources.md @@ -327,12 +327,13 @@ Note: This feature is supported only in NGINX Plus. ### Upstream.Healthcheck -The Healthcheck defines an [active health check](https://docs.nginx.com/nginx/admin-guide/load-balancer/http-health-check/). In the example below we enable a health check for an upstream and configure all the available parameters: +The Healthcheck defines an [active health check](https://docs.nginx.com/nginx/admin-guide/load-balancer/http-health-check/). In the example below we enable a health check for an upstream and configure all the available parameters, including the `slow-start` parameter combined with [`mandatory` and `persistent`](https://docs.nginx.com/nginx/admin-guide/load-balancer/http-health-check/#mandatory-health-checks): ```yaml name: tea service: tea-svc port: 80 +slow-start: 30s healthCheck: enable: true path: /healthz @@ -350,6 +351,8 @@ healthCheck: - name: Host value: my.service statusMatch: "! 500" + mandatory: true + persistent: true ``` Note: This feature is supported only in NGINX Plus. @@ -372,6 +375,8 @@ Note: This feature is supported only in NGINX Plus. |``statusMatch`` | The expected response status codes of a health check. By default, the response should have status code 2xx or 3xx. Examples: ``"200"`` , ``"! 500"`` , ``"301-303 307"``. See the documentation of the [match](https://nginx.org/en/docs/http/ngx_http_upstream_hc_module.html?#match) directive. This not supported for gRPC type upstreams. | ``string`` | No | |``grpcStatus`` | The expected [gRPC status code](https://github.com/grpc/grpc/blob/master/doc/statuscodes.md#status-codes-and-their-use-in-grpc) of the upstream server response to the [Check method](https://github.com/grpc/grpc/blob/master/doc/health-checking.md). Configure this field only if your gRPC services do not implement the gRPC health checking protocol. For example, configure ``12`` if the upstream server responds with `12 (UNIMPLEMENTED)` status code. Only valid on gRPC type upstreams. | ``int`` | No | |``grpcService`` | The gRPC service to be monitored on the upstream server. Only valid on gRPC type upstreams. | ``string`` | No | +|``mandatory`` | Require every newly added server to pass all configured health checks before NGINX Plus sends traffic to it. If this is not specified, or is set to false, the server will be initially considered healthy. When combined with [slow-start](https://nginx.org/en/docs/http/ngx_http_upstream_module.html#slow_start), it gives a new server more time to connect to databases and “warm up” before being asked to handle their full share of traffic. | ``bool`` | No | +|``persistent`` | Set the initial “up” state for a server after reload if the server was considered healthy before reload. Enabling persistent requires that the mandatory parameter is also set to `true`. | ``bool`` | No | {{% /table %}} ### Upstream.SessionCookie diff --git a/internal/configs/version2/http.go b/internal/configs/version2/http.go index 0ec8355498..0815fa208f 100644 --- a/internal/configs/version2/http.go +++ b/internal/configs/version2/http.go @@ -251,6 +251,8 @@ type HealthCheck struct { GRPCPass string GRPCStatus *int GRPCService string + Mandatory bool + Persistent bool } // TLSRedirect defines a redirect in a Server. diff --git a/internal/configs/version2/nginx-plus.virtualserver.tmpl b/internal/configs/version2/nginx-plus.virtualserver.tmpl index 2440b84091..b3929c1397 100644 --- a/internal/configs/version2/nginx-plus.virtualserver.tmpl +++ b/internal/configs/version2/nginx-plus.virtualserver.tmpl @@ -250,6 +250,7 @@ server { {{ end }} health_check {{ if $hc.URI }}uri={{ $hc.URI }} {{ end }}port={{ $hc.Port }} interval={{ $hc.Interval }} jitter={{ $hc.Jitter }} fails={{ $hc.Fails }} passes={{ $hc.Passes }}{{ if $hc.Match }} match={{ $hc.Match }}{{ end }} + {{ if $hc.Mandatory }} mandatory{{ if $hc.Persistent }} persistent{{ end }}{{ end }} {{ if $hc.GRPCPass }} type=grpc{{ if $hc.GRPCStatus }} grpc_status={{ $hc.GRPCStatus }}{{ end }} {{ if $hc.GRPCService }} grpc_service={{ $hc.GRPCService }}{{ end }}{{ end }}; } diff --git a/internal/configs/version2/templates_test.go b/internal/configs/version2/templates_test.go index b4096603de..fa66161d8d 100644 --- a/internal/configs/version2/templates_test.go +++ b/internal/configs/version2/templates_test.go @@ -165,14 +165,16 @@ var virtualServerCfg = VirtualServerConfig{ }, HealthChecks: []HealthCheck{ { - Name: "coffee", - URI: "/", - Interval: "5s", - Jitter: "0s", - Fails: 1, - Passes: 1, - Port: 50, - ProxyPass: "http://coffee-v2", + Name: "coffee", + URI: "/", + Interval: "5s", + Jitter: "0s", + Fails: 1, + Passes: 1, + Port: 50, + ProxyPass: "http://coffee-v2", + Mandatory: true, + Persistent: true, }, { Name: "tea", diff --git a/internal/configs/virtualserver.go b/internal/configs/virtualserver.go index d044b1d56c..339986562f 100644 --- a/internal/configs/virtualserver.go +++ b/internal/configs/virtualserver.go @@ -1349,6 +1349,10 @@ func generateHealthCheck( hc.Match = generateStatusMatchName(upstreamName) } + hc.Mandatory = upstream.HealthCheck.Mandatory + + hc.Persistent = upstream.HealthCheck.Persistent + hc.GRPCStatus = upstream.HealthCheck.GRPCStatus hc.GRPCService = upstream.HealthCheck.GRPCService diff --git a/internal/configs/virtualserver_test.go b/internal/configs/virtualserver_test.go index 4a08e1a997..1e21a2425a 100644 --- a/internal/configs/virtualserver_test.go +++ b/internal/configs/virtualserver_test.go @@ -6800,6 +6800,35 @@ func TestGenerateHealthCheck(t *testing.T) { }, msg: "HealthCheck with time parameters have correct format", }, + { + upstream: conf_v1.Upstream{ + HealthCheck: &conf_v1.HealthCheck{ + Enable: true, + Mandatory: true, + Persistent: true, + }, + ProxyConnectTimeout: "30s", + ProxyReadTimeout: "30s", + ProxySendTimeout: "30s", + }, + upstreamName: upstreamName, + expected: &version2.HealthCheck{ + Name: upstreamName, + ProxyConnectTimeout: "30s", + ProxyReadTimeout: "30s", + ProxySendTimeout: "30s", + ProxyPass: fmt.Sprintf("http://%v", upstreamName), + URI: "/", + Interval: "5s", + Jitter: "0s", + Fails: 1, + Passes: 1, + Headers: make(map[string]string), + Mandatory: true, + Persistent: true, + }, + msg: "HealthCheck with mandatory and persistent set", + }, } baseCfgParams := &ConfigParams{ diff --git a/pkg/apis/configuration/v1/types.go b/pkg/apis/configuration/v1/types.go index bb84e5b724..390fb06225 100644 --- a/pkg/apis/configuration/v1/types.go +++ b/pkg/apis/configuration/v1/types.go @@ -111,6 +111,8 @@ type HealthCheck struct { StatusMatch string `json:"statusMatch"` GRPCStatus *int `json:"grpcStatus"` GRPCService string `json:"grpcService"` + Mandatory bool `json:"mandatory"` + Persistent bool `json:"persistent"` } // Header defines an HTTP Header. diff --git a/pkg/apis/configuration/validation/virtualserver.go b/pkg/apis/configuration/validation/virtualserver.go index 28052da170..629c8df6a4 100644 --- a/pkg/apis/configuration/validation/virtualserver.go +++ b/pkg/apis/configuration/validation/virtualserver.go @@ -269,6 +269,10 @@ func validateUpstreamHealthCheck(hc *v1.HealthCheck, typeName string, fieldPath } } + if hc.Persistent && !hc.Mandatory { + allErrs = append(allErrs, field.Required(fieldPath.Child("mandatory"), "must be true when `persistent` is true")) + } + return allErrs } diff --git a/pkg/apis/configuration/validation/virtualserver_test.go b/pkg/apis/configuration/validation/virtualserver_test.go index 5e3779f864..d889bd3d4e 100644 --- a/pkg/apis/configuration/validation/virtualserver_test.go +++ b/pkg/apis/configuration/validation/virtualserver_test.go @@ -2397,6 +2397,8 @@ func TestValidateUpstreamHealthCheck(t *testing.T) { }, }, StatusMatch: "! 500", + Mandatory: true, + Persistent: true, } allErrs := validateUpstreamHealthCheck(hc, "", field.NewPath("healthCheck")) @@ -2480,6 +2482,13 @@ func TestValidateUpstreamHealthCheckFails(t *testing.T) { GRPCService: "tea-servicev2", }, }, + { + hc: &v1.HealthCheck{ + Enable: true, + Path: "/healthz", + Persistent: true, + }, + }, } for _, test := range tests { diff --git a/tests/data/virtual-server-upstream-options/plus-virtual-server-with-invalid-keys.yaml b/tests/data/virtual-server-upstream-options/plus-virtual-server-with-invalid-keys.yaml index 355b3f534f..748f4d449f 100644 --- a/tests/data/virtual-server-upstream-options/plus-virtual-server-with-invalid-keys.yaml +++ b/tests/data/virtual-server-upstream-options/plus-virtual-server-with-invalid-keys.yaml @@ -30,6 +30,7 @@ spec: statusMatch: "invalid" grpcService: "notimplemented" grpcStatus: 12 + persistent: True slow-start: "-3s" queue: size: -100 diff --git a/tests/suite/test_virtual_server_upstream_options.py b/tests/suite/test_virtual_server_upstream_options.py index b1d24384e0..5b0602a4a8 100644 --- a/tests/suite/test_virtual_server_upstream_options.py +++ b/tests/suite/test_virtual_server_upstream_options.py @@ -302,7 +302,7 @@ def test_openapi_validation_flow(self, kube_apis, ingress_controller_prerequisit class TestOptionsSpecificForPlus: @pytest.mark.parametrize('options, expected_strings', [ ({"lb-method": "least_conn", - "healthCheck": {"enable": True, "port": 8080}, + "healthCheck": {"enable": True, "port": 8080, "mandatory": True, "persistent": True}, "slow-start": "3h", "queue": {"size": 100}, "ntlm": True, @@ -311,7 +311,7 @@ class TestOptionsSpecificForPlus: "path": "/some-valid/path", "expires": "max", "domain": "virtual-server-route.example.com", "httpOnly": True, "secure": True}}, - ["health_check uri=/ port=8080 interval=5s jitter=0s", "fails=1 passes=1", ";", + ["health_check uri=/ port=8080 interval=5s jitter=0s", "fails=1 passes=1", "mandatory persistent", ";", "slow_start=3h", "queue 100 timeout=60s;", "ntlm;", "sticky cookie TestCookie expires=max domain=virtual-server-route.example.com httponly secure path=/some-valid/path;"]), ({"lb-method": "least_conn", @@ -398,7 +398,7 @@ def test_validation_flow(self, kube_apis, ingress_controller_prerequisites, "upstreams[0].healthCheck.read-timeout", "upstreams[0].healthCheck.send-timeout", "upstreams[0].healthCheck.headers[0].name", "upstreams[0].healthCheck.headers[0].value", "upstreams[0].healthCheck.statusMatch", "upstreams[0].healthCheck.grpcStatus", - "upstreams[0].healthCheck.grpcService", + "upstreams[0].healthCheck.grpcService", "upstreams[0].healthCheck.mandatory", "upstreams[0].slow-start", "upstreams[0].queue.size", "upstreams[0].queue.timeout", "upstreams[0].sessionCookie.name", "upstreams[0].sessionCookie.path",