From f2398122e3fcbb1de0bd1c292aaca22902018528 Mon Sep 17 00:00:00 2001 From: Ciara Stacke Date: Mon, 7 Feb 2022 13:17:31 +0000 Subject: [PATCH] Fix healthcheck ports --- docs/content/configuration/transportserver-resource.md | 2 +- .../virtualserver-and-virtualserverroute-resources.md | 2 +- internal/configs/transportserver.go | 6 +----- internal/configs/transportserver_test.go | 2 -- internal/configs/version2/nginx-plus.transportserver.tmpl | 2 +- internal/configs/version2/nginx-plus.virtualserver.tmpl | 2 +- internal/configs/virtualserver.go | 7 ++----- tests/suite/test_transport_server_tcp_load_balance.py | 4 ++-- tests/suite/test_transport_server_udp_load_balance.py | 4 ++-- tests/suite/test_virtual_server_upstream_options.py | 4 ++-- 10 files changed, 13 insertions(+), 22 deletions(-) diff --git a/docs/content/configuration/transportserver-resource.md b/docs/content/configuration/transportserver-resource.md index 82bb8ad5f2..e404176c33 100644 --- a/docs/content/configuration/transportserver-resource.md +++ b/docs/content/configuration/transportserver-resource.md @@ -168,7 +168,7 @@ Note: This feature is supported only in NGINX Plus. |``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 | +|``port`` | The port used for health check requests. By default, the [server port is used](https://nginx.org/en/docs/stream/ngx_stream_upstream_hc_module.html#health_check_port). Note: in contrast with the port of the upstream, this port is not a service port, but a port of a pod. | ``integer`` | No | |``match`` | Controls the data to send and the response to expect for the healthcheck. | [match](#upstreamhealthcheckmatch) | No | {{% /table %}} diff --git a/docs/content/configuration/virtualserver-and-virtualserverroute-resources.md b/docs/content/configuration/virtualserver-and-virtualserverroute-resources.md index 483eab3352..99db196289 100644 --- a/docs/content/configuration/virtualserver-and-virtualserverroute-resources.md +++ b/docs/content/configuration/virtualserver-and-virtualserverroute-resources.md @@ -367,7 +367,7 @@ Note: This feature is supported only in NGINX Plus. |``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 | +|``port`` | The port used for health check requests. By default, the [server port is used](https://nginx.org/en/docs/http/ngx_http_upstream_hc_module.html#health_check_port). Note: in contrast with the port of the upstream, this port is not a service port, but a port of a pod. | ``integer`` | No | |``tls`` | The TLS configuration used for health check requests. By default, the ``tls`` field of the upstream is used. | [upstream.tls](#upstreamtls) | No | |``connect-timeout`` | The timeout for establishing a connection with an upstream server. By default, the ``connect-timeout`` of the upstream is used. | ``string`` | No | |``read-timeout`` | The timeout for reading a response from an upstream server. By default, the ``read-timeout`` of the upstream is used. | ``string`` | No | diff --git a/internal/configs/transportserver.go b/internal/configs/transportserver.go index ca1009e280..e0cb1ea5d1 100644 --- a/internal/configs/transportserver.go +++ b/internal/configs/transportserver.go @@ -144,6 +144,7 @@ func generateTransportServerHealthCheck(upstreamName string, generatedUpstreamNa hc.Interval = generateTimeWithDefault(u.HealthCheck.Interval, hc.Interval) hc.Jitter = generateTimeWithDefault(u.HealthCheck.Jitter, hc.Jitter) hc.Timeout = generateTimeWithDefault(u.HealthCheck.Timeout, hc.Timeout) + hc.Port = u.HealthCheck.Port if u.HealthCheck.Fails > 0 { hc.Fails = u.HealthCheck.Fails @@ -153,10 +154,6 @@ func generateTransportServerHealthCheck(upstreamName string, generatedUpstreamNa hc.Passes = u.HealthCheck.Passes } - if u.HealthCheck.Port > 0 { - hc.Port = u.HealthCheck.Port - } - if u.HealthCheck.Match != nil { name := "match_" + generatedUpstreamName match = generateHealthCheckMatch(u.HealthCheck.Match, name) @@ -175,7 +172,6 @@ func generateTransportServerHealthCheckWithDefaults(up conf_v1alpha1.Upstream) * Enabled: false, Timeout: "5s", Jitter: "0s", - Port: up.Port, Interval: "5s", Passes: 1, Fails: 1, diff --git a/internal/configs/transportserver_test.go b/internal/configs/transportserver_test.go index 95a9a6ad7e..b6ccea697c 100644 --- a/internal/configs/transportserver_test.go +++ b/internal/configs/transportserver_test.go @@ -642,7 +642,6 @@ func TestGenerateTransportServerHealthChecks(t *testing.T) { Enabled: true, Timeout: "5s", Jitter: "0s", - Port: 90, Interval: "5s", Passes: 1, Fails: 1, @@ -668,7 +667,6 @@ func TestGenerateTransportServerHealthChecks(t *testing.T) { Enabled: true, Timeout: "5s", Jitter: "0s", - Port: 90, Interval: "5s", Passes: 1, Fails: 1, diff --git a/internal/configs/version2/nginx-plus.transportserver.tmpl b/internal/configs/version2/nginx-plus.transportserver.tmpl index 2252221f42..ead3646501 100644 --- a/internal/configs/version2/nginx-plus.transportserver.tmpl +++ b/internal/configs/version2/nginx-plus.transportserver.tmpl @@ -53,7 +53,7 @@ server { proxy_pass {{ $s.ProxyPass }}; {{ if $s.HealthCheck }} - health_check interval={{ $s.HealthCheck.Interval }} port={{ $s.HealthCheck.Port }} + health_check interval={{ $s.HealthCheck.Interval }} {{ if $s.HealthCheck.Port }} port={{ $s.HealthCheck.Port }}{{ end }} passes={{ $s.HealthCheck.Passes }} jitter={{ $s.HealthCheck.Jitter }} fails={{ $s.HealthCheck.Fails }}{{ if $s.UDP }} udp{{ end }}{{ if $s.HealthCheck.Match }} match={{ $s.HealthCheck.Match }}{{ end }}; health_check_timeout {{ $s.HealthCheck.Timeout }}; {{ end }} diff --git a/internal/configs/version2/nginx-plus.virtualserver.tmpl b/internal/configs/version2/nginx-plus.virtualserver.tmpl index b3929c1397..2a03e6f1d2 100644 --- a/internal/configs/version2/nginx-plus.virtualserver.tmpl +++ b/internal/configs/version2/nginx-plus.virtualserver.tmpl @@ -248,7 +248,7 @@ server { {{ else }} proxy_pass {{ $hc.ProxyPass }}; {{ end }} - health_check {{ if $hc.URI }}uri={{ $hc.URI }} {{ end }}port={{ $hc.Port }} interval={{ $hc.Interval }} jitter={{ $hc.Jitter }} + health_check {{ if $hc.URI }}uri={{ $hc.URI }} {{ end }}{{ if $hc.Port }}port={{ $hc.Port }} {{ end }}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 }} diff --git a/internal/configs/virtualserver.go b/internal/configs/virtualserver.go index 339986562f..0395c88025 100644 --- a/internal/configs/virtualserver.go +++ b/internal/configs/virtualserver.go @@ -216,7 +216,6 @@ func newHealthCheckWithDefaults(upstream conf_v1.Upstream, upstreamName string, Jitter: "0s", Fails: 1, Passes: 1, - Port: int(upstream.Port), ProxyPass: fmt.Sprintf("%v://%v", generateProxyPassProtocol(upstream.TLS.Enable), upstreamName), ProxyConnectTimeout: generateTimeWithDefault(upstream.ProxyConnectTimeout, cfgParams.ProxyConnectTimeout), ProxyReadTimeout: generateTimeWithDefault(upstream.ProxyReadTimeout, cfgParams.ProxyReadTimeout), @@ -1321,10 +1320,6 @@ func generateHealthCheck( hc.Passes = upstream.HealthCheck.Passes } - if upstream.HealthCheck.Port > 0 { - hc.Port = upstream.HealthCheck.Port - } - if upstream.HealthCheck.ConnectTimeout != "" { hc.ProxyConnectTimeout = generateTime(upstream.HealthCheck.ConnectTimeout) } @@ -1349,6 +1344,8 @@ func generateHealthCheck( hc.Match = generateStatusMatchName(upstreamName) } + hc.Port = upstream.HealthCheck.Port + hc.Mandatory = upstream.HealthCheck.Mandatory hc.Persistent = upstream.HealthCheck.Persistent diff --git a/tests/suite/test_transport_server_tcp_load_balance.py b/tests/suite/test_transport_server_tcp_load_balance.py index 1d1db8d13f..0bd5e72ce4 100644 --- a/tests/suite/test_transport_server_tcp_load_balance.py +++ b/tests/suite/test_transport_server_tcp_load_balance.py @@ -494,7 +494,7 @@ def test_tcp_passing_healthcheck_with_match( match = f"match_ts_{transport_server_setup.namespace}_transport-server_tcp-app" - assert "health_check interval=5s port=3333" in result_conf + assert "health_check interval=5s" in result_conf assert f"passes=1 jitter=0s fails=1 match={match}" in result_conf assert "health_check_timeout 3s;" assert 'send "health"' in result_conf @@ -559,7 +559,7 @@ def test_tcp_failing_healthcheck_with_match( match = f"match_ts_{transport_server_setup.namespace}_transport-server_tcp-app" - assert "health_check interval=5s port=3333" in result_conf + assert "health_check interval=5s" in result_conf assert f"passes=1 jitter=0s fails=1 match={match}" in result_conf assert "health_check_timeout 3s" assert 'send "health"' in result_conf diff --git a/tests/suite/test_transport_server_udp_load_balance.py b/tests/suite/test_transport_server_udp_load_balance.py index 824cef2166..2a843c8eef 100644 --- a/tests/suite/test_transport_server_udp_load_balance.py +++ b/tests/suite/test_transport_server_udp_load_balance.py @@ -284,7 +284,7 @@ def test_udp_passing_healthcheck_with_match( match = f"match_ts_{transport_server_setup.namespace}_transport-server_udp-app" - assert "health_check interval=5s port=3334" in result_conf + assert "health_check interval=5s" in result_conf assert f"passes=1 jitter=0s fails=1 udp match={match}" in result_conf assert "health_check_timeout 3s;" assert 'send "health"' in result_conf @@ -350,7 +350,7 @@ def test_udp_failing_healthcheck_with_match( match = f"match_ts_{transport_server_setup.namespace}_transport-server_udp-app" - assert "health_check interval=5s port=3334" in result_conf + assert "health_check interval=5s" in result_conf assert f"passes=1 jitter=0s fails=1 udp match={match}" in result_conf assert "health_check_timeout 3s;" assert 'send "health"' in result_conf diff --git a/tests/suite/test_virtual_server_upstream_options.py b/tests/suite/test_virtual_server_upstream_options.py index 5b0602a4a8..9db4860902 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, "mandatory": True, "persistent": True}, + "healthCheck": {"enable": True, "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", "mandatory persistent", ";", + ["health_check uri=/ 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",