-
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
validation: Add config validation mode. (#499) #863
Changes from 27 commits
51eba6e
2b80353
10bf44f
3e5a64c
235be89
44e5b53
6ecdca9
9fda0da
b45e4ae
d351abc
8555de0
41e3e62
7732881
a3af91c
d4770dc
9948580
20a021b
10a4556
47670a2
f82eea8
93d6069
569265b
aecac42
1a7d6c7
9aa303b
8499fb1
1e4ed13
18fb3c2
f7ab514
1fdbb46
f0918d2
1a99564
c95e78c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,9 +6,12 @@ | |
#include <string> | ||
#include <unordered_map> | ||
|
||
#include "envoy/access_log/access_log.h" | ||
#include "envoy/http/async_client.h" | ||
#include "envoy/http/conn_pool.h" | ||
#include "envoy/json/json_object.h" | ||
#include "envoy/local_info/local_info.h" | ||
#include "envoy/runtime/runtime.h" | ||
#include "envoy/upstream/load_balancer.h" | ||
#include "envoy/upstream/thread_local_cluster.h" | ||
#include "envoy/upstream/upstream.h" | ||
|
@@ -103,6 +106,8 @@ class ClusterManager { | |
virtual void shutdown() PURE; | ||
}; | ||
|
||
typedef std::unique_ptr<ClusterManager> ClusterManagerPtr; | ||
|
||
/** | ||
* Global configuration for any SDS clusters. | ||
*/ | ||
|
@@ -139,6 +144,16 @@ class ClusterManagerFactory { | |
public: | ||
virtual ~ClusterManagerFactory() {} | ||
|
||
/** | ||
* Allocate a cluster manager from configuration JSON. | ||
*/ | ||
virtual ClusterManagerPtr clusterManagerFromJson(const Json::Object& config, Stats::Store& stats, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we remove the other methods from this interface now that we have this? E.g. can There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think not easily -- that distinction is there for testability, see https://github.com/lyft/envoy/blob/master/test/common/upstream/cluster_manager_impl_test.cc#L38. (I'm actually wasn't originally 100% sure that the ClusterManagerFactory is morally speaking the right place for, well, a ClusterManager factory function -- but I successfully talked myself into it. I could be talked back out of it, if there's a better place.) |
||
ThreadLocal::Instance& tls, | ||
Runtime::Loader& runtime, | ||
Runtime::RandomGenerator& random, | ||
const LocalInfo::LocalInfo& local_info, | ||
AccessLog::AccessLogManager& log_manager) PURE; | ||
|
||
/** | ||
* Allocate an HTTP connection pool. | ||
*/ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,7 +10,7 @@ namespace Envoy { | |
#define RELEASE_ASSERT(X) \ | ||
{ \ | ||
if (!(X)) { \ | ||
Logger::Registry::getLog(Logger::Id::assert) \ | ||
Envoy::Logger::Registry::getLog(Envoy::Logger::Id::assert) \ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are the changes in this file needed? Seems like they shouldn't be? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because I was using Turns out I don't need those macros there anymore, but I think morally speaking this is still correct. Happy to pull it out of this PR if you'd rather have it separately, though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nope fine, was just curious. |
||
.critical("assert failure: {}: {}:{}", #X, __FILE__, __LINE__); \ | ||
abort(); \ | ||
} \ | ||
|
@@ -26,7 +26,7 @@ namespace Envoy { | |
* Indicate a panic situation and exit. | ||
*/ | ||
#define PANIC(X) \ | ||
Logger::Registry::getLog(Logger::Id::assert) \ | ||
Envoy::Logger::Registry::getLog(Envoy::Logger::Id::assert) \ | ||
.critical("panic: {}: {}:{}", X, __FILE__, __LINE__); \ | ||
abort(); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,6 +40,7 @@ AsyncClient::Request* AsyncClientImpl::send(MessagePtr&& request, AsyncClient::C | |
const Optional<std::chrono::milliseconds>& timeout) { | ||
AsyncRequestImpl* async_request = | ||
new AsyncRequestImpl(std::move(request), *this, callbacks, timeout); | ||
async_request->initialize(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want consistency in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AsyncStreamImpl doesn't currently do any real work in its ctor, though. I could add an |
||
std::unique_ptr<AsyncStreamImpl> new_request{async_request}; | ||
|
||
// The request may get immediately failed. If so, we will return nullptr. | ||
|
@@ -155,7 +156,9 @@ AsyncRequestImpl::AsyncRequestImpl(MessagePtr&& request, AsyncClientImpl& parent | |
AsyncClient::Callbacks& callbacks, | ||
const Optional<std::chrono::milliseconds>& timeout) | ||
: AsyncStreamImpl(parent, *this, timeout), request_(std::move(request)), callbacks_(callbacks) { | ||
} | ||
|
||
void AsyncRequestImpl::initialize() { | ||
sendHeaders(request_->headers(), !request_->body()); | ||
if (!remoteClosed() && request_->body()) { | ||
sendData(*request_->body(), true); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,98 @@ | ||
licenses(["notice"]) # Apache 2 | ||
|
||
load("//bazel:envoy_build_system.bzl", "envoy_cc_library", "envoy_package") | ||
|
||
envoy_package() | ||
|
||
envoy_cc_library( | ||
name = "api_lib", | ||
srcs = ["api.cc"], | ||
hdrs = ["api.h"], | ||
deps = [ | ||
":dispatcher_lib", | ||
"//include/envoy/api:api_interface", | ||
"//include/envoy/filesystem:filesystem_interface", | ||
"//source/common/api:api_lib", | ||
], | ||
) | ||
|
||
envoy_cc_library( | ||
name = "async_client_lib", | ||
srcs = ["async_client.cc"], | ||
hdrs = ["async_client.h"], | ||
deps = [ | ||
"//include/envoy/http:async_client_interface", | ||
"//include/envoy/http:message_interface", | ||
], | ||
) | ||
|
||
envoy_cc_library( | ||
name = "cluster_manager_lib", | ||
srcs = ["cluster_manager.cc"], | ||
hdrs = ["cluster_manager.h"], | ||
deps = [ | ||
":async_client_lib", | ||
"//include/envoy/upstream:cluster_manager_interface", | ||
"//source/common/upstream:cluster_manager_lib", | ||
], | ||
) | ||
|
||
envoy_cc_library( | ||
name = "connection_handler_lib", | ||
srcs = ["connection_handler.cc"], | ||
hdrs = ["connection_handler.h"], | ||
deps = [ | ||
"//include/envoy/api:api_interface", | ||
"//include/envoy/network:connection_handler_interface", | ||
"//include/envoy/network:filter_interface", | ||
"//include/envoy/network:listen_socket_interface", | ||
"//source/common/common:assert_lib", | ||
], | ||
) | ||
|
||
envoy_cc_library( | ||
name = "dispatcher_lib", | ||
srcs = ["dispatcher.cc"], | ||
hdrs = ["dispatcher.h"], | ||
deps = [ | ||
"//include/envoy/event:dispatcher_interface", | ||
"//source/common/common:assert_lib", | ||
"//source/common/event:dispatcher_lib", | ||
], | ||
) | ||
|
||
envoy_cc_library( | ||
name = "dns_lib", | ||
srcs = ["dns.cc"], | ||
hdrs = ["dns.h"], | ||
deps = [ | ||
"//include/envoy/event:dispatcher_interface", | ||
"//include/envoy/network:dns_interface", | ||
], | ||
) | ||
|
||
envoy_cc_library( | ||
name = "server_lib", | ||
srcs = ["server.cc"], | ||
hdrs = ["server.h"], | ||
deps = [ | ||
":api_lib", | ||
":cluster_manager_lib", | ||
":connection_handler_lib", | ||
":dns_lib", | ||
"//include/envoy/common:optional", | ||
"//include/envoy/server:drain_manager_interface", | ||
"//include/envoy/server:instance_interface", | ||
"//include/envoy/ssl:context_manager_interface", | ||
"//include/envoy/tracing:http_tracer_interface", | ||
"//source/common/access_log:access_log_manager_lib", | ||
"//source/common/common:assert_lib", | ||
"//source/common/runtime:runtime_lib", | ||
"//source/common/ssl:context_lib", | ||
"//source/common/stats:stats_lib", | ||
"//source/common/thread_local:thread_local_lib", | ||
"//source/server:configuration_lib", | ||
"//source/server:server_lib", | ||
"//source/server/http:admin_lib", | ||
], | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
#include "server/config_validation/api.h" | ||
|
||
#include "server/config_validation/dispatcher.h" | ||
|
||
namespace Envoy { | ||
namespace Api { | ||
|
||
ValidationImpl::ValidationImpl(std::chrono::milliseconds file_flush_interval_msec) | ||
: Impl(file_flush_interval_msec) {} | ||
|
||
Event::DispatcherPtr ValidationImpl::allocateDispatcher() { | ||
return Event::DispatcherPtr{new Event::ValidationDispatcher()}; | ||
} | ||
|
||
} // Api | ||
} // Envoy |
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.
It's fine in the enum here, but for the command-line, I'm in agreement with @PiotrSikora, it's probably cleaner to have a
--dry-run
and--dry-run-lite
flags, I don't think anyone will ever set--mode=serve
.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.
FWIW, my thinking is that we might end up with any number of distinct modes for running Envoy -- "full validation" and "lite validation" are two, but conceptually
--hot-restart-version
is another: if we implemented that today, it might be a value for--mode
. These all have two important things in common: that they significantly modify the semantics of what it means to run the binary (you expect drastically different things to happen) and, more importantly to the flag question, that it doesn't make sense to specify more than one.We can grow a lot of boolean flags and document them all with "set either zero or one of these flags because they're mutually exclusive" or we can make that implicit with an enum-valued flag, and I argue that's much clearer. I agree nobody will ever explicitly set
--mode serve
, but I think it's still semantically a default state.I'm 100% open to different names (e.g. I don't feel strongly about "validate" over "dry run"). I'm also willing to be outvoted on an enum vs. a fleet of booleans, but it was an intentional stance.
(cc @PiotrSikora because this is in reply to your comment too)
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.
What does hot restart version do that isn't done by
--mode=serve
?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.
It prints the hot restart version and exits immediately (https://github.com/lyft/envoy/blob/master/source/server/options_impl.cc#L63) without serving, validating, or doing anything else.