-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Added TCP healthcheck capabilities to the HdsDelegate #4079
Conversation
Signed-off-by: Lilika Markatou <lilika@google.com>
@hennna can you take a pass on this? Thanks. |
@hennna friendly ping |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, looks great, just a few cleanups.
@@ -145,6 +145,40 @@ class HdsIntegrationTest : public HttpIntegrationTest, | |||
return server_health_check_specifier_; | |||
} | |||
|
|||
// Creates a basic HealthCheckSpecifier message containing one endpoint and | |||
// one TCP health_check | |||
envoy::service::discovery::v2::HealthCheckSpecifier makeTCPHealthCheckSpecifier() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: makeTcpHealthCheckSpecifier
health_check->mutable_health_checks(0)->mutable_interval()->set_seconds(1); | ||
health_check->mutable_health_checks(0)->mutable_unhealthy_threshold()->set_value(2); | ||
health_check->mutable_health_checks(0)->mutable_healthy_threshold()->set_value(2); | ||
auto tcp_hc = health_check->mutable_health_checks(0)->mutable_tcp_health_check(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: auto*
->mutable_endpoints(0) | ||
->mutable_address() | ||
->mutable_socket_address() | ||
->set_port_value(host_upstream_->localAddress()->ip()->port()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: consider using addressToProtobufAddress()
in source/common/network/utility.h
for this (and elsewhere in the HDS tests).
@@ -293,12 +330,120 @@ TEST_P(HdsIntegrationTest, SingleEndpointUnhealthy) { | |||
cleanupHdsConnection(); | |||
} | |||
|
|||
// Tests that Envoy can healthcheck two hosts that are in the same cluster, and | |||
// Tests Envoy TCP healthchecking an endpoint that doesn't respond and reporting that it is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: health checking
ASSERT_TRUE(host_upstream_->waitForRawConnection(host_fake_raw_connection_)); | ||
ASSERT_TRUE( | ||
host_fake_raw_connection_->waitForData(FakeRawConnection::waitForInexactMatch("Ping"))); | ||
auto ret = host_fake_raw_connection_->write("Voronoi"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the references..
ASSERT_TRUE(host_upstream_->waitForRawConnection(host_fake_raw_connection_)); | ||
ASSERT_TRUE( | ||
host_fake_raw_connection_->waitForData(FakeRawConnection::waitForInexactMatch("Ping"))); | ||
auto ret = host_fake_raw_connection_->write("Pong"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: is ref
unused below?
Signed-off-by: Lilika Markatou <lilika@google.com>
Signed-off-by: Lilika Markatou <lilika@google.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM; @hennna LMK if this looks good to you as well, thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Sorry for the delay! I need to update my notifications from GitHub
hds_stream_->sendGrpcMessage(server_health_check_specifier_); | ||
test_server_->waitForCounterGe("hds_delegate.requests", ++hds_requests_); | ||
|
||
// Envoys asks the endpoint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment nit: what is Envoy asking for?
Signed-off-by: Lilika Markatou <lilika@google.com>
@markatou also need to resolve merge conflict :) |
Signed-off-by: Lilika Markatou <lilika@google.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rad.
* origin/master: (38 commits) test: add tests for corner-cases around sending requests before run() starts or after run() ends. (envoyproxy#4114) perf: reduce the memory usage of LC Trie construction (envoyproxy#4117) test: moving redundant code in websocket_integration_test to utilities (envoyproxy#4127) test: make YamlLoadFromStringFail less picky about error msg. (envoyproxy#4141) rbac: add rbac network filter. (envoyproxy#4083) fuzz: route lookup and header finalization fuzzer. (envoyproxy#4116) Set content-type and content-length (envoyproxy#4113) fault: use FractionalPercent for percent (envoyproxy#3978) test: Fix inverted exact match logic in IntegrationTcpClient::waitForData() (envoyproxy#4134) Added cluster_name to load assignment config for static cluster (envoyproxy#4123) ssl: refactor ContextConfig to use TlsCertificateConfig (envoyproxy#4115) syscall: refactor OsSysCalls for deeper errno latching (envoyproxy#4111) thrift_proxy: fix oneway bugs (envoyproxy#4025) Do not crash when converting YAML to JSON fails (envoyproxy#4110) config: allow unknown fields flag (take 2) (envoyproxy#4096) Use a jittered backoff strategy for handling HdsDelegate stream/connection failures (envoyproxy#4108) bazel: use GCS remote cache (envoyproxy#4050) Add thread local cache of overload action states (envoyproxy#4090) Added TCP healthcheck capabilities to the HdsDelegate (envoyproxy#4079) secret: add secret provider interface and use it for TlsCertificates (envoyproxy#4086) ... Signed-off-by: Snow Pettersen <snowp@squareup.com>
The HdsDelegate now informs the management server that it can perform TCP healthchecks.
This change also comes with three tests showing that indeed the HdsDelegate can perform TCP healthchecks.
Risk Level: Low
This is for #1310.