config: Specify Bootstrap as proto object to Envoy::OptionsImpl#7722
config: Specify Bootstrap as proto object to Envoy::OptionsImpl#7722jmarantz merged 10 commits intoenvoyproxy:masterfrom
Conversation
Signed-off-by: Frank Fort <ffort@google.com>
…ide the command line Signed-off-by: Frank Fort <ffort@google.com>
Signed-off-by: Frank Fort <ffort@google.com>
Signed-off-by: Frank Fort <ffort@google.com>
…breaks everything Signed-off-by: Frank Fort <ffort@google.com>
Signed-off-by: Frank Fort <ffort@google.com>
mattklein123
left a comment
There was a problem hiding this comment.
Thank you, a few small comments.
/wait
| void setBaseId(uint64_t base_id) { base_id_ = base_id; }; | ||
| void setConcurrency(uint32_t concurrency) { concurrency_ = concurrency; } | ||
| void setConfigPath(const std::string& config_path) { config_path_ = config_path; } | ||
| void setConfigProto(const envoy::config::bootstrap::v2::Bootstrap& config_proto) { |
There was a problem hiding this comment.
Any reason to not just take this as an optional constructor arg? Then it can also be const below.
There was a problem hiding this comment.
The reason I wanted to make this a setter was because we use the argc/argv version of OptionsImpl to first initialize OptionsImpl from the command line and then later mutate it via these setter functions.
Signed-off-by: Frank Fort <ffort@google.com>
Signed-off-by: Frank Fort <ffort@google.com>
mattklein123
left a comment
There was a problem hiding this comment.
Thanks, LGTM modulo a few small comments.
/wait
| bootstrap.MergeFrom(bootstrap_override); | ||
| } | ||
| if (config_proto.ByteSize() != 0) { | ||
| bootstrap.MergeFrom(config_proto); |
There was a problem hiding this comment.
Do you have a server test that actually covers this case?
There was a problem hiding this comment.
Sorry missed this. Added a few tests to verify the order that Bootstrap config is read from the various config functions.
Signed-off-by: Frank Fort <ffort@google.com>
Description: Adds a method to Envoy::OptionsImpl to allow specifying the bootstrap configuration as a proto
Bootstrapobject directly rather than using theconfigYamlorconfigPathoptions.This would allow clients to directly specify a Bootstrap proto object without having to deal with yaml serialization and would simplify our configuration process since we construct the Bootstrap proto programmatically as a protocol buffer.
This is not a command line flag like
configYamlorconfigPathsince this is not a serialized attribute of the options and we don't want to allow for a separate configuration path that isn't YAML.I moved the validation of the config flags to
source/server/options_impl.ccso thattest/integration:run_envoy_testwould pass, since we now have a config source that isn't specified on the command line.Risk Level: Low
Testing: Ran
bazel test test/common/... test/exe/... test/integration/... test/mocks/... test/config_test/... test/server:options_impl_testDocs Changes: N/A
Release Notes: N/A
Fixes #7713