Skip to content

config: making v2-config-only a boolean flag #3847

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.

Sign up for GitHub

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

Merged
mattklein123 merged 3 commits into from
Jul 12, 2018
Merged

config: making v2-config-only a boolean flag #3847

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
abd06be
Making v2-config-only a boolean flag
alyssawilk Jul 12, 2018
7f71af9
--allow-deprecated-v1-api
alyssawilk Jul 12, 2018
dfe6e7d
cleanup
alyssawilk Jul 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions docs/root/configuration/overview/v2_overview.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,9 @@ flag, i.e.:

.. code-block:: console

./envoy -c <path to config>.{json,yaml,pb,pb_text} --v2-config-only
./envoy -c <path to config>.{json,yaml,pb,pb_text}

where the extension reflects the underlying v2 config representation. The
:option:`--v2-config-only` flag is not strictly required as Envoy will attempt
to autodetect the config file version, but this option provides an enhanced
debug experience when configuration parsing fails.
where the extension reflects the underlying v2 config representation.

The :ref:`Bootstrap <envoy_api_msg_config.bootstrap.v2.Bootstrap>` message is the root of the
configuration. A key concept in the :ref:`Bootstrap <envoy_api_msg_config.bootstrap.v2.Bootstrap>`
Expand Down
2 changes: 1 addition & 1 deletion docs/root/intro/version_history.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ Version history
to filter based on the presence of Envoy response flags.
* admin: added :http:get:`/hystrix_event_stream` as an endpoint for monitoring envoy's statistics
through `Hystrix dashboard <https://github.com/Netflix-Skunkworks/hystrix-dashboard/wiki>`_.
* config: v1 disabled by default. v1 support remains available until October via flipping --v2-config-only=false.
* config: v1 disabled by default. v1 support remains available until October via setting --v2-config-only 0.
* health check: added support for :ref:`custom health check <envoy_api_field_core.HealthCheck.custom_health_check>`.
* health check: added support for :ref:`specifying jitter as a percentage <envoy_api_field_core.HealthCheck.interval_jitter_percent>`.
* health_check: added support for :ref:`health check event logging <arch_overview_health_check_logging>`.
Expand Down
4 changes: 2 additions & 2 deletions docs/root/operations/cli.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,11 @@ following are the command line options that Envoy supports.

./envoy -c bootstrap.yaml --config-yaml "node: {id: 'node1'}"

.. option:: --v2-config-only
.. option:: --v2-config-only <bool>

*(optional)* This flag determines whether the configuration file should only
be parsed as a :ref:`v2 bootstrap configuration file
<config_overview_v2_bootstrap>`. If false (default), when a v2 bootstrap
<config_overview_v2_bootstrap>`. If false when a v2 bootstrap
config parse fails, a second attempt to parse the config as a :ref:`v1 JSON
configuration file <config_overview_v1>` will be made.

@mattklein123 mattklein123 Jul 12, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: leave line break

Expand Down
4 changes: 3 additions & 1 deletion source/server/options_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,9 @@ OptionsImpl::OptionsImpl(int argc, const char* const* argv,
TCLAP::ValueArg<std::string> config_yaml(
"", "config-yaml", "Inline YAML configuration, merges with the contents of --config-path",
false, "", "string", cmd);
TCLAP::SwitchArg v2_config_only("", "v2-config-only", "parse config as v2 only", cmd, true);

TCLAP::ValueArg<bool> v2_config_only("", "v2-config-only", "parse config as v2 only", false,
false, "bool", cmd);
TCLAP::ValueArg<std::string> admin_address_path("", "admin-address-path", "Admin address path",
false, "", "string", cmd);
TCLAP::ValueArg<std::string> local_address_ip_version("", "local-address-ip-version",
Expand Down
22 changes: 21 additions & 1 deletion test/server/options_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,12 +60,32 @@ TEST(OptionsImplTest, InvalidCommandLine) {
"Couldn't find match for argument");
}

TEST(OptionsImplTest, v2True) {
std::unique_ptr<OptionsImpl> options = createOptionsImpl(
"envoy --mode validate --concurrency 2 -c hello --admin-address-path path --restart-epoch 1 "
"--local-address-ip-version v6 -l info --service-cluster cluster --service-node node "
"--service-zone zone --file-flush-interval-msec 9000 --drain-time-s 60 --log-format [%v] "
"--parent-shutdown-time-s 90 --log-path /foo/bar --v2-config-only 1 --disable-hot-restart");

@htuch htuch Jul 12, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tclap only supports 0/1 rather than false/true? We might want to use an independent flag to avoid breaking folks production equivalent of Borg config.

@alyssawilk alyssawilk Jul 12, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I tested and true/false chokes. I was tempted to do a string literal true/false but I figured folks use to TCLAP rather than google flags would find that more confusing

@alyssawilk alyssawilk Jul 12, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I'm open to just adding a second flag but if the concern is folks who want v2 only will have it specified, we're going to break them eventually when we remove v1 config and the flag. Is it worth adding the complexity of a second flag to avoid a problem they're going to face in a few months anyway?

@mattklein123 mattklein123 Jul 12, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO we should add a second flag and not change this one (basically make it do nothing since the default is already what it implies). I think this is less likely to break people?

EXPECT_EQ(Server::Mode::Validate, options->mode());
EXPECT_TRUE(options->v2ConfigOnly());
}

TEST(OptionsImplTest, v2False) {
std::unique_ptr<OptionsImpl> options = createOptionsImpl(
"envoy --mode validate --concurrency 2 -c hello --admin-address-path path --restart-epoch 1 "
"--local-address-ip-version v6 -l info --service-cluster cluster --service-node node "
"--service-zone zone --file-flush-interval-msec 9000 --drain-time-s 60 --log-format [%v] "
"--parent-shutdown-time-s 90 --log-path /foo/bar --v2-config-only 0 --disable-hot-restart");
EXPECT_EQ(Server::Mode::Validate, options->mode());
EXPECT_FALSE(options->v2ConfigOnly());
}

TEST(OptionsImplTest, All) {
std::unique_ptr<OptionsImpl> options = createOptionsImpl(
"envoy --mode validate --concurrency 2 -c hello --admin-address-path path --restart-epoch 1 "
"--local-address-ip-version v6 -l info --service-cluster cluster --service-node node "
"--service-zone zone --file-flush-interval-msec 9000 --drain-time-s 60 --log-format [%v] "
"--parent-shutdown-time-s 90 --log-path /foo/bar --v2-config-only --disable-hot-restart");
"--parent-shutdown-time-s 90 --log-path /foo/bar --v2-config-only 0 --disable-hot-restart");
EXPECT_EQ(Server::Mode::Validate, options->mode());
EXPECT_EQ(2U, options->concurrency());
EXPECT_EQ("hello", options->configPath());
Expand Down