allow specifying the API version of bootstrap from the command line by snowp · Pull Request #10803 · envoyproxy/envoy

snowp · 2020-04-16T05:01:19Z

Adds a --boostrap_version flag that can be used to determine which API version the bootstrap should be parsed as.

Risk Level: Low
Testing: UTs
Docs Changes: Flag docs
Release Notes: n/a
Fixes #10343

snowp · 2020-04-16T05:04:16Z

Just wanted to put something up to see if we think this is the right kind of approach, there are a few issues currently:

having to map from a version string to LATEST_VERSION/EARLIEST_VERSION is a bit gross feeling, should this instead just bypass all of the API boosting?
As a result of hooking into the API boosting wrapper, the failure when its set to v2 throws an ApiBoostRetryException, which results in a less than idea error message prompting the user to "try again"

@htuch

htuch

Thanks for taking this on!

htuch · 2020-04-17T01:26:16Z

include/envoy/server/options.h

Should it just be an integer, since we won't have non-integer major versions?

htuch · 2020-04-17T01:27:42Z

source/common/protobuf/utility.cc

The repetition with the code below might support refactoring if we go down this path.

htuch · 2020-04-17T01:31:48Z

source/server/server.cc

Yeah, I agree this is a bit ungraceful. I think the alternative is we supply to loadFromJson etc. the type URL of the bootstrap that we know exists on the filesystem, e.g. envoy.config.bootstrap.v2. We can do that mapping here.

Then, we load JSON specifically into a message that we build from reflection with that type. We then have a v2 bootstrap message. Following this, we do a VersionConverter::upgrade to the v3 bootstrap Envoy is using internally. LMK if you need more details on this, we have some examples of each of these things around.

That makes sense to me, it's along the lines of what I had in mind as well for a way doing this by bypassing the whole withApiBoosting helper. I'll give that a go and see how it goes

repokitteh-read-only · 2020-04-17T19:39:39Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #10803 was synchronize by snowp.

see: more, trace.

htuch

Generally looks great, it's pretty easy to understand now..
/wait

htuch · 2020-04-20T00:27:17Z

source/common/protobuf/utility.h

Does this still need to move?

htuch · 2020-04-20T00:27:55Z

source/common/protobuf/utility.cc

Does this still need to be rewritten?

htuch · 2020-04-20T00:30:02Z

source/server/server.cc

Nit: prefer not explicitly state captures.

htuch · 2020-04-20T00:31:20Z

test/server/server_test.cc

Should there be some positive tests as well for explicit version CLI?

htuch · 2020-04-20T00:31:56Z

test/common/protobuf/utility_test.cc

Should this use a strict validator and reject?

snowp · 2020-04-21T21:20:15Z

Huh for some reason DCO is broken on one of the commits. I'll fix once this is approved

htuch

LGTM, thanks!

Signed-off-by: Snow Pettersen <kpettersen@netflix.com>

snowp · 2020-04-22T15:00:21Z

test/server/server_test.cc

+}
+
+// Validate that we correctly parse a V2 file when configured to do so.
+TEST_P(ServerInstanceImplTest, LoadsV2ConfigWhenV2SelectedFromPbText) {


Hmm this seems to fail under compile_time_options with

[ RUN ] IpVersions/ServerInstanceImplTest.LoadsV2ConfigWhenV2SelectedFromPbText/IPv4 [libprotobuf WARNING external/com_google_protobuf/src/google/protobuf/text_format.cc:324] Warning parsing text-format envoy.config.bootstrap.v2.Bootstrap: 4:1: text format contains deprecated field "build_version" [2020-04-22 05:18:57.856][17][critical][main] [external/envoy/source/server/server.cc:98] error initializing configuration '/b/f/w/_tmp/fa9accb9e25432420d3f5adcb6c78680/valid_v2_but_invalid_v3_bootstrap.pb_text.with.ports.pb_text': Proto constraint validation failed (Using deprecated option 'envoy.api.v2.core.Node.build_version' from file base.proto. This configuration will be removed from Envoy soon. Please see https://www.envoyproxy.io/docs/envoy/latest/intro/deprecated for details. If continued use of this field is absolutely necessary, see https://www.envoyproxy.io/docs/envoy/latest/configuration/operations/runtime#using-runtime-overrides-for-deprecated-features for how to apply a temporary and highly discouraged override.): id: "bootstrap_id" build_version: "foo" unknown file: Failure C++ exception with description "Proto constraint validation failed (Using deprecated option 'envoy.api.v2.core.Node.build_version' from file base.proto. This configuration will be removed from Envoy soon. Please see https://www.envoyproxy.io/docs/envoy/latest/intro/deprecated for details. If continued use of this field is absolutely necessary, see https://www.envoyproxy.io/docs/envoy/latest/configuration/operations/runtime#using-runtime-overrides-for-deprecated-features for how to apply a temporary and highly discouraged override.): id: "bootstrap_id" build_version: "foo" " thrown in the test body. [ FAILED ] IpVersions/ServerInstanceImplTest.LoadsV2ConfigWhenV2SelectedFromPbText/IPv4, where GetParam() = 4-byte object <00-00 00-00> (4 ms)

presumably since we run the test with --disabled-features=disabled. Thoughts on how to make this test work @htuch ?

Can this be a DEPRECATED_FEATURE_TEST?

Yep this worked, thanks!

Signed-off-by: Snow Pettersen <aickck@gmail.com>

htuch

LGTM, thanks!

Signed-off-by: Spencer Lewis <slewis@squareup.com> * master: (46 commits) allow specifying the API version of bootstrap from the command line (envoyproxy#10803) config: adding connect matcher (unused) (envoyproxy#10894) Add missing dependency on `assert.h` (envoyproxy#10918) Lower heap and disk space used by kafka tests (envoyproxy#10915) [tools] handle commits merged without PR in deprecated script (envoyproxy#10723) tools: including working tree in modified_since_last_github_commit.sh diff. (envoyproxy#10911) rocketmq_proxy: implement rocketmq proxy [docs] PR template to include commit message (envoyproxy#10900) docs: breaking long word to stop content overflow. (envoyproxy#10880) Delete legacy connection pool code. (envoyproxy#10881) wasm: clarify how configuration is passed (envoyproxy#10782) issue template: clarify security/crash reporting (envoyproxy#10885) api/faq: add entry on incremental xDS. (envoyproxy#10876) router: retry overloaded requests (envoyproxy#10847) Remove inclusion of pthread.h, not needed for linux compilation (envoyproxy#10895) request_id: Add option to always set request id in response (envoyproxy#10808) xray: Use correct types for segment document output (envoyproxy#10834) router: fixing a watermark bug for streaming retries (envoyproxy#10866) http: auditing Path() calls for safety with Pathless CONNECT (envoyproxy#10851) Remove hardcoded type urls Part.2 (envoyproxy#10848) ...

moderation · 2020-04-23T21:05:59Z

FYI @snowp, I'm running envoy --bootstrap-version 3 --service-cluster <servicename> --service-node <hostname> --base-id 1 --config-path <config.json> --log-level info but the output of :<adminport>/server_info returns "bootstrap_version": 0,. Is that expected?

yanavlasov assigned htuch Apr 16, 2020

htuch reviewed Apr 17, 2020

View reviewed changes

repokitteh-read-only bot added the api label Apr 17, 2020

htuch suggested changes Apr 20, 2020

View reviewed changes

repokitteh-read-only bot added waiting and removed waiting labels Apr 20, 2020

htuch previously approved these changes Apr 22, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Apr 22, 2020

Snow Pettersen added 16 commits April 21, 2020 17:54

make it possible to specify which xds version to load bootstrap as

cdee4e6

Signed-off-by: Snow Pettersen <kpettersen@netflix.com>

add tests

f40dbc0

Signed-off-by: Snow Pettersen <kpettersen@netflix.com>

format

a0bcbe5

Signed-off-by: Snow Pettersen <kpettersen@netflix.com>

cleanup

d053993

Signed-off-by: Snow Pettersen <kpettersen@netflix.com>

disable boosting instead of trying to control targetted version, use int

07bba1d

Signed-off-by: Snow Pettersen <kpettersen@netflix.com>

spelling

1c9bc8c

Signed-off-by: Snow Pettersen <kpettersen@netflix.com>

spellign

bac39b8

Signed-off-by: Snow Pettersen <kpettersen@netflix.com>

format fix

d2b4869

Signed-off-by: Snow Pettersen <kpettersen@netflix.com>

update proto

bf04695

Signed-off-by: Snow Pettersen <kpettersen@netflix.com>

proto sync

4af978e

Signed-off-by: Snow Pettersen <kpettersen@netflix.com>

revert bad file, fix lambda captures

7bc1b10

Signed-off-by: Snow Pettersen <kpettersen@netflix.com>

revert more changes

76948ee

Signed-off-by: Snow Pettersen <kpettersen@netflix.com>

strict validation in utility_test

b65c7c9

Signed-off-by: Snow Pettersen <kpettersen@netflix.com>

add positive tests

364d58f

Signed-off-by: Snow Pettersen <kpettersen@netflix.com>

format

4d3de0c

Signed-off-by: Snow Pettersen <kpettersen@netflix.com>

add docs

be08546

Signed-off-by: Snow Pettersen <kpettersen@netflix.com>

snowp dismissed htuch’s stale review via be08546 April 22, 2020 00:56

snowp force-pushed the specify-version branch from 4958535 to be08546 Compare April 22, 2020 00:56

repokitteh-read-only bot added the api label Apr 22, 2020

snowp commented Apr 22, 2020

View reviewed changes

snowp added 2 commits April 22, 2020 22:20

remove virtual specifier

b5219ed

Signed-off-by: Snow Pettersen <aickck@gmail.com>

mark test indicating that it uses deprecated apis

622d5a7

Signed-off-by: Snow Pettersen <aickck@gmail.com>

htuch approved these changes Apr 23, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Apr 23, 2020

htuch merged commit 62777e8 into envoyproxy:master Apr 23, 2020

Conversation

snowp commented Apr 16, 2020

Uh oh!

snowp commented Apr 16, 2020

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

repokitteh-read-only bot commented Apr 17, 2020

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snowp commented Apr 21, 2020

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

moderation commented Apr 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants