healthcheck: support TCP health check with ProxyProtocol #31691

Shikugawa · 2024-01-08T12:38:42Z

Commit Message: healthcheck: support TCP health check with ProxyProtocol
Additional Description: This PR aims to support TCP healthcheck with ProxyProtocol. This feature is useful to ensure Proxy Protocol is enabled or not from upstream servers. It works in addition to normal TCP based healthcheck. This feature is the equivalent with check-send-proxy provided by HAProxy. https://cbonte.github.io/haproxy-dconv/2.0/configuration.html#5.2-check-send-proxy
Risk Level: Low
Testing: Unit/Integration
Docs Changes: Required
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

repokitteh-read-only · 2024-01-08T12:38:52Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @lizan
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #31691 was opened by Shikugawa.

see: more, trace.

lizan · 2024-01-10T01:23:15Z

/lgtm api

@zuercher for owner review

Shikugawa · 2024-01-16T06:10:21Z

@zuercher friendly ping

zuercher · 2024-01-16T20:07:11Z

api/envoy/config/core/v3/health_check.proto

+    // When setting this value, it tries to attempt health check request with ProxyProtocol.
+    // Payload is sent after preceding ProxyProtocol header.
+    ProxyProtocolConfig proxy_protocol_config = 3;


For the comment, would prefer: "Use the proxy protocol for health checks. The send payload is sent immediately after the proxy protocol header."

Furthermore, is it valid to have an empty send payload when this flag is enabled? If so we should both document that and guard against configurations that leave send empty.

Yes. Empty send payload is allowed when this flag is enabled. Let me fix.

Sorry I forcefully pushed. The fixed commits are shown here. 495ba97#diff-4ccbbf045baa1ad3abd43bb7534d765e115e50be2ac9d9c3639588a4fd54e8a6R183-R185

zuercher · 2024-01-16T20:23:32Z

source/extensions/health_checkers/tcp/health_checker_impl.cc

+  bool should_write_data = false;
+
+  if (parent_.proxy_protocol_config_ != nullptr) {
+    Extensions::Common::ProxyProtocol::generateProxyProtoHeader(*parent_.proxy_protocol_config_,


There another proxy protocol command "local" which looks like it's meant for health checks (and that the remote server should use the connection endpoints and causes the endpoints in the protocol to be ignored). I think it's okay to use the command this function generates since we'll set the endpoints to be ourselves the server being health checked, but I thought I'd point it out.

Actually I think it's enough to use local to check whether servers can accept ProxyProtocol or not. But the capability to fetch downstream info from ProxyProtocol header is the main focus of them. So some backend implementation may check the capability whether it can fetch downstream info or not (AFAIK I don't know the actual usecase. But it might be helpful).
So having generateProxyProtoHeader may help users more than using local directly.

Could you elaborate more for the meaning of address setting in the Proxy Protocol header? The health check client's local address is the upstream local address, and the remote is the health check address of the remote host. I think this is different from the real upstream request which is using the downstream connection as the input of this generateProxyProtoHeader util.

I'd incline to add some comments for this, and describe the LOCAL as well as the address setting. Also could you add a test to see if the proxy protocol with address is actually sent/received?

@botengyao @zuercher Hi. Sorry for late response.
I revisited how to provide this feature and came up with that we should provide this feature with LOCAL command for V2, and present local/remote address for V1 because it doesn't support them. It is because I found that some users faced confusion with this behavior to provide PROXY command for healthcheck in HAProxy community. haproxy/haproxy#511

So, I noticed the previous implementation may make users confused by the difference between Envoy and HAProxy's one. Thus, I cleaned the entire code in this patch to support them.

Signed-off-by: Rei Shimizu <shimizu.rei@linecorp.com>

zuercher

@ilzan sorry, I asked for a comment change in the API and it needs to be re-approved.

Otherwise, lgtm.

lizan · 2024-01-19T06:29:34Z

/lgtm api

lizan · 2024-01-19T06:30:48Z

@Shikugawa one last thing, can you add a release note for the new field?

Signed-off-by: Rei Shimizu <shimizu.rei@linecorp.com>

zuercher · 2024-03-13T04:39:34Z

ping @Shikugawa

github-actions · 2024-04-12T08:01:04Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

github-actions · 2024-04-19T08:01:13Z

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Signed-off-by: Rei Shimizu <shimizu.rei@linecorp.com>

phlax · 2024-05-06T09:58:56Z

@botengyao @zuercher i think this is waiting for final review

zuercher

@lizan this needs API re-approval because a comment was updated

lizan

/lgtm api

botengyao

/assign-from @envoyproxy/senior-maintainers

For a pass and merging.

Signed-off-by: Rei Shimizu <shimizu.rei@linecorp.com>

Shikugawa · 2024-05-11T07:11:28Z

@zuercher Fixed conflict. Could you please approve again?

ravenblackx · 2024-05-14T15:37:07Z

/retest

ravenblackx

Approving the conflict-merge commit only, and assuming zuercher's review of everything before that still applies.

Shikugawa requested a review from zuercher as a code owner January 8, 2024 12:38

repokitteh-read-only bot added the api label Jan 8, 2024

repokitteh-read-only bot assigned lizan Jan 8, 2024

Shikugawa force-pushed the proxy-proto-hc branch 3 times, most recently from 246c869 to 44e9615 Compare January 9, 2024 03:19

repokitteh-read-only bot removed the api label Jan 10, 2024

lizan assigned zuercher Jan 10, 2024

zuercher reviewed Jan 16, 2024

View reviewed changes

repokitteh-read-only bot added the api label Jan 17, 2024

Shikugawa force-pushed the proxy-proto-hc branch 4 times, most recently from 1476e57 to 8aca444 Compare January 17, 2024 17:36

healthcheck: support TCP health check with ProxyProtocol

495ba97

Signed-off-by: Rei Shimizu <shimizu.rei@linecorp.com>

Shikugawa force-pushed the proxy-proto-hc branch from 8aca444 to 495ba97 Compare January 17, 2024 17:41

Shikugawa requested a review from zuercher January 17, 2024 17:45

fix format

41a221b

Signed-off-by: Rei Shimizu <shimizu.rei@linecorp.com>

zuercher previously approved these changes Jan 18, 2024

View reviewed changes

repokitteh-read-only bot removed the api label Jan 19, 2024

add release not

1e3d9b5

Signed-off-by: Rei Shimizu <shimizu.rei@linecorp.com>

Shikugawa dismissed zuercher’s stale review via 1e3d9b5 January 23, 2024 16:06

Shikugawa added 3 commits January 24, 2024 01:08

add release note

ef53f5a

Signed-off-by: Rei Shimizu <shimizu.rei@linecorp.com>

remove unused changes

1363cbf

Signed-off-by: Rei Shimizu <shimizu.rei@linecorp.com>

tmp

ff40a2d

Signed-off-by: Rei Shimizu <shimizu.rei@linecorp.com>

github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Apr 12, 2024

github-actions bot closed this Apr 19, 2024

Shikugawa reopened this Apr 20, 2024

github-actions bot removed the stale stalebot believes this issue/PR has not been touched recently label Apr 20, 2024

Shikugawa added 2 commits May 1, 2024 12:22

fix design

2b2b2bc

Signed-off-by: Rei Shimizu <shimizu.rei@linecorp.com>

fix design

75fd6f1

Signed-off-by: Rei Shimizu <shimizu.rei@linecorp.com>

Shikugawa dismissed zuercher’s stale review via 75fd6f1 May 1, 2024 12:43

repokitteh-read-only bot added api and removed waiting labels May 1, 2024

tmp

a8ea04e

Signed-off-by: Rei Shimizu <shimizu.rei@linecorp.com>

Shikugawa requested review from botengyao and zuercher May 1, 2024 12:54

cleanup

b218e86

Signed-off-by: Rei Shimizu <shimizu.rei@linecorp.com>

zuercher approved these changes May 9, 2024

View reviewed changes

lizan reviewed May 9, 2024

View reviewed changes

repokitteh-read-only bot removed the api label May 9, 2024

botengyao approved these changes May 9, 2024

View reviewed changes

botengyao reviewed May 9, 2024

View reviewed changes

zuercher previously approved these changes May 10, 2024

View reviewed changes

Merge branch 'main' into proxy-proto-hc

a907641

Signed-off-by: Rei Shimizu <shimizu.rei@linecorp.com>

Shikugawa dismissed zuercher’s stale review via a907641 May 11, 2024 07:10

ravenblackx approved these changes May 14, 2024

View reviewed changes

ravenblackx enabled auto-merge (squash) May 14, 2024 15:40

ravenblackx merged commit a3ecbf0 into envoyproxy:main May 14, 2024
52 of 53 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

healthcheck: support TCP health check with ProxyProtocol #31691

healthcheck: support TCP health check with ProxyProtocol #31691

Shikugawa commented Jan 8, 2024 •

edited

repokitteh-read-only bot commented Jan 8, 2024

lizan commented Jan 10, 2024

Shikugawa commented Jan 16, 2024

zuercher Jan 16, 2024

Shikugawa Jan 17, 2024

Shikugawa Jan 17, 2024

zuercher Jan 16, 2024

Shikugawa Jan 17, 2024 •

edited

botengyao Jan 25, 2024 •

edited

Shikugawa May 1, 2024 •

edited

zuercher left a comment

lizan commented Jan 19, 2024

lizan commented Jan 19, 2024

zuercher commented Mar 13, 2024

github-actions bot commented Apr 12, 2024

github-actions bot commented Apr 19, 2024

phlax commented May 6, 2024

zuercher left a comment

lizan left a comment

botengyao left a comment •

edited

Shikugawa commented May 11, 2024 •

edited

ravenblackx commented May 14, 2024

ravenblackx left a comment

healthcheck: support TCP health check with ProxyProtocol #31691

healthcheck: support TCP health check with ProxyProtocol #31691

Conversation

Shikugawa commented Jan 8, 2024 • edited

repokitteh-read-only bot commented Jan 8, 2024

lizan commented Jan 10, 2024

Shikugawa commented Jan 16, 2024

zuercher Jan 16, 2024

Choose a reason for hiding this comment

Shikugawa Jan 17, 2024

Choose a reason for hiding this comment

Shikugawa Jan 17, 2024

Choose a reason for hiding this comment

zuercher Jan 16, 2024

Choose a reason for hiding this comment

Shikugawa Jan 17, 2024 • edited

Choose a reason for hiding this comment

botengyao Jan 25, 2024 • edited

Choose a reason for hiding this comment

Shikugawa May 1, 2024 • edited

Choose a reason for hiding this comment

zuercher left a comment

Choose a reason for hiding this comment

lizan commented Jan 19, 2024

lizan commented Jan 19, 2024

zuercher commented Mar 13, 2024

github-actions bot commented Apr 12, 2024

github-actions bot commented Apr 19, 2024

phlax commented May 6, 2024

zuercher left a comment

Choose a reason for hiding this comment

lizan left a comment

Choose a reason for hiding this comment

botengyao left a comment • edited

Choose a reason for hiding this comment

Shikugawa commented May 11, 2024 • edited

ravenblackx commented May 14, 2024

ravenblackx left a comment

Choose a reason for hiding this comment

Shikugawa commented Jan 8, 2024 •

edited

Shikugawa Jan 17, 2024 •

edited

botengyao Jan 25, 2024 •

edited

Shikugawa May 1, 2024 •

edited

botengyao left a comment •

edited

Shikugawa commented May 11, 2024 •

edited