WIP: config: implement delta CDS client in Envoy by fredlas · Pull Request #5466 · envoyproxy/envoy

fredlas · 2019-01-03T15:56:22Z

Description: Code for Envoy to speak incremental CDS with a management server. Provided as a second implementation of the CdsApi interface (along with some other new behind-the-scenes classes, analogous to the non-incremental implementation). INCREMENTAL_GRPC added to config_source.proto's ApiTypes, to allow bootstrap configs to ask for incremental xDS.

Part of #4991.

Risk Level: medium
Testing: new integration test, new code will (TODO) have unit tests

htuch · 2019-01-14T02:59:47Z

@fredlas can you merge master to deal with the conflicts here?

htuch

This looks like a great start to the incremental xDS work, exciting to see this happening! I've left mostly design & code architecture comments, with a few nits. I think we should converge on the right code structure before spending too much time in the weeds of implementation/tests as a next step.

api/envoy/api/v2/core/config_source.proto

include/envoy/config/incremental_subscription.h

source/common/upstream/cds_incremental.h

source/server/server.cc

source/server/worker_impl.cc

test/common/config/utility_test.cc

test/integration/xds_integration_test_base.cc

Refactor necessary for #5466 to avoid completely duplicating this logic. Risk Level: low Testing: existing tests Signed-off-by: Fred Douglas <fredlas@google.com>

source/common/config/incremental_subscription_impl.h

source/common/upstream/cds_api_incremental_impl.cc

source/common/config/incremental_subscription_impl.h

…roxy#5681) Refactor necessary for envoyproxy#5466 to avoid completely duplicating this logic. Risk Level: low Testing: existing tests Signed-off-by: Fred Douglas <fredlas@google.com> Signed-off-by: Fred Douglas <43351173+fredlas@users.noreply.github.com>

htuch

Some more high-level feedback.

api/XDS_PROTOCOL.md

source/common/config/delta_subscription_impl.h

source/common/upstream/cds_api_delta_impl.cc

fredlas · 2019-02-13T16:05:05Z

Ok, I believe it is now ready for a full review.

htuch

Design wise, I think this is looking pretty good. To me, there are two remaining design issues:

The path to making DeltaSubscriptionImpl muxable, supporting ADS and as a special case individual xDS types.
Unifying onConfigUpdate.

The rest are just implementation comments, will look at tests later.

/wait

api/XDS_PROTOCOL.md

source/common/config/delta_subscription_impl.h

source/common/upstream/cds_api_impl.cc

htuch

Great, this looks like it's on the right track!

/wait

api/XDS_PROTOCOL.md

source/common/upstream/cds_api_impl.cc

source/common/config/utility.h

source/common/upstream/cds_api_impl.cc

source/common/upstream/cds_api_impl.h

htuch

LG modulo CI fixes and comments.

htuch · 2019-02-28T17:24:36Z

api/envoy/api/v2/discovery.proto

@brian-avery FYI

source/common/config/delta_subscription_impl.h

test/integration/delta_cds_integration_test.cc

Bump up max configurable max_request_headers_kb to 96 KiB. Add a check to http1/codec_impl.cc for headers size. Raise the default library limits in http_parser nghttp2 so we'll rely on our own codec check. Risk Level: Medium. Testing: Moved all the large request headers tests to ProtocolIntegrationTest. Part of envoyproxy#5626. Signed-off-by: Auni Ahsan <auni@google.com> Signed-off-by: Fred Douglas <fredlas@google.com>

Signed-off-by: Fred Douglas <fredlas@google.com>

…roxy#5681) Refactor necessary for envoyproxy#5466 to avoid completely duplicating this logic. Risk Level: low Testing: existing tests Signed-off-by: Fred Douglas <fredlas@google.com> Signed-off-by: Fred Douglas <fredlas@google.com>

Signed-off-by: Fred Douglas <fredlas@google.com>

htuch · 2019-03-06T00:26:39Z

@fredlas docs still broken!

Signed-off-by: Fred Douglas <fredlas@google.com>

This provides genhtml, which is needed for Bazel native coverage report generation. Signed-off-by: Harvey Tuch <htuch@google.com>

Remove the last prebuilt dependencies and switches to foreign_cc with a slight wrapper script. Risk Level: Low Testing: CI Signed-off-by: Lizan Zhou <lizan@tetrate.io>

In order to compile envoy for iOS we need this commit protocolbuffers/protobuf@0894e07 from protobuf. This also includes the previous commits that required us to use a non-release version. Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>

…database, bazelbuild/bazel-skylib, gogo/protobuf (envoyproxy#6183) Signed-off-by: Michael Payne <michael@sooper.org>

Signed-off-by: Rama Chavali <rama.rao@salesforce.com> Signed-off-by: Fred Douglas <fredlas@google.com>

fredlas · 2019-03-06T15:38:35Z

Fixed the docs, but somehow DCO got rebroken, and after following DCO bot's suggested fix, it doesn't look very fixable. Starting over in #6191.

Description: Code for Envoy to speak delta CDS with a management server. DELTA_GRPC added to config_source.proto's ApiTypes, to allow bootstrap configs to ask for incremental xDS. Part of #4991. Was #5466; giving up on broken DCO craziness. Risk Level: medium Testing: new integration test Signed-off-by: Fred Douglas <fredlas@google.com>

@htuch

Refactor requested in #5466 by @htuch. Move all of delta_cds_integration_test.cc into cds_integration_test.cc, with some cute code sharing. Move the config fragment strings into test/config/utility.cc. Risk Level: none Signed-off-by: Fred Douglas <fredlas@google.com>

htuch self-assigned this Jan 9, 2019

htuch suggested changes Jan 14, 2019

View reviewed changes

This was referenced Jan 16, 2019

Clarify and simplify the ADS and xDS over gRPC implementations #5270

Closed

Implement incremental xDS in Envoy #4991

Closed

htuch added the waiting label Jan 18, 2019

fredlas mentioned this pull request Jan 22, 2019

config: add GrpcStream: gRPC mechanisms pulled out of GrpcMux #5681

Merged

repokitteh-read-only bot removed the waiting label Jan 24, 2019

dmitri-d reviewed Jan 25, 2019

View reviewed changes

source/common/config/incremental_subscription_impl.h Outdated Show resolved Hide resolved

fredlas mentioned this pull request Jan 28, 2019

config: partial rejections in incremental xDS #5739

Open

fredlas force-pushed the IMP_inc_sub_impl branch from e23975c to 43fd982 Compare January 29, 2019 00:38

dmitri-d reviewed Jan 29, 2019

View reviewed changes

source/common/upstream/cds_api_incremental_impl.cc Outdated Show resolved Hide resolved

dmitri-d reviewed Jan 30, 2019

View reviewed changes

source/common/config/incremental_subscription_impl.h Outdated Show resolved Hide resolved

dmitri-d reviewed Jan 30, 2019

View reviewed changes

source/common/config/incremental_subscription_impl.h Outdated Show resolved Hide resolved

fredlas changed the title ~~WIP: config: implement incremental CDS client in Envoy~~ WIP: config: implement delta CDS client in Envoy Feb 4, 2019

htuch suggested changes Feb 5, 2019

View reviewed changes

htuch added the waiting label Feb 8, 2019

repokitteh-read-only bot removed the waiting label Feb 13, 2019

htuch suggested changes Feb 15, 2019

View reviewed changes

repokitteh-read-only bot added waiting and removed waiting labels Feb 15, 2019

htuch suggested changes Feb 22, 2019

View reviewed changes

repokitteh-read-only bot added waiting and removed waiting labels Feb 22, 2019

htuch mentioned this pull request Feb 28, 2019

Add names and alias to resource #6128

Closed

htuch suggested changes Feb 28, 2019

View reviewed changes

aunu53 and others added 2 commits March 5, 2019 16:21

fix spelling

1f1362e

Signed-off-by: Fred Douglas <fredlas@google.com>

fredlas force-pushed the IMP_inc_sub_impl branch from 932139c to 24d50df Compare March 5, 2019 21:27

fredlas requested review from alyssawilk, lizan, mattklein123, snowp and zuercher as code owners March 5, 2019 21:27

fix DCO and overwrite crazy rebase result with real current state

220098e

Signed-off-by: Fred Douglas <fredlas@google.com>

fredlas force-pushed the IMP_inc_sub_impl branch from 24d50df to 220098e Compare March 5, 2019 21:28

fredlas added 2 commits March 5, 2019 16:54

i think resolves merge conflict

f82f61d

spelling dictionary exceptions

8d06f25

Signed-off-by: Fred Douglas <fredlas@google.com>

fredlas force-pushed the IMP_inc_sub_impl branch from 04c6937 to 8d06f25 Compare March 5, 2019 22:07

fredlas force-pushed the IMP_inc_sub_impl branch from 3694b59 to 7995563 Compare March 6, 2019 15:16

fredlas and others added 6 commits March 6, 2019 10:18

fix docs CI warning

1bd37d5

Signed-off-by: Fred Douglas <fredlas@google.com>

ci: add lcov to Docker image. (envoyproxy#6179)

ce452a3

This provides genhtml, which is needed for Bazel native coverage report generation. Signed-off-by: Harvey Tuch <htuch@google.com>

build: use foreign_cc for luajit (envoyproxy#6168)

b6e7215

Remove the last prebuilt dependencies and switches to foreign_cc with a slight wrapper script. Risk Level: Low Testing: CI Signed-off-by: Lizan Zhou <lizan@tetrate.io>

build: Update protobuf (envoyproxy#6180)

003d0d0

In order to compile envoy for iOS we need this commit protocolbuffers/protobuf@0894e07 from protobuf. This also includes the previous commits that required us to use a non-release version. Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>

Dependency: Update bazelbuild/buildtools, grailbio/bazel-compilation-…

2ea0b60

…database, bazelbuild/bazel-skylib, gogo/protobuf (envoyproxy#6183) Signed-off-by: Michael Payne <michael@sooper.org>

connected state across hot restarts (envoyproxy#6153)

751daf8

Signed-off-by: Rama Chavali <rama.rao@salesforce.com> Signed-off-by: Fred Douglas <fredlas@google.com>

fredlas force-pushed the IMP_inc_sub_impl branch from 7995563 to 751daf8 Compare March 6, 2019 15:19

fredlas mentioned this pull request Mar 6, 2019

config: implement incremental CDS in Envoy #6191

Merged

fredlas closed this Mar 6, 2019

fredlas mentioned this pull request Mar 7, 2019

test: merge delta CDS integration test into normal CDS #6214

Merged

fredlas deleted the IMP_inc_sub_impl branch March 11, 2019 17:40

Conversation

fredlas commented Jan 3, 2019

Uh oh!

htuch commented Jan 14, 2019

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fredlas commented Feb 13, 2019

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

htuch Feb 28, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

htuch commented Mar 6, 2019

Uh oh!

fredlas commented Mar 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

20 participants