thrift: adding xDS RDS support for thrift proxy. by tkovacs-2 · Pull Request #19982 · envoyproxy/envoy

tkovacs-2 · 2022-02-16T10:58:23Z

Commit Message:
Adding xDS routing discovery support for thrift proxy.

Additional Description:

The routing discovery is supported only through ADS. No separate service endpoints added
for thrift routing config type url.

This PR is split from #17631

Risk Level:
Low

Testing:

Unittest
Manual test with static routing from bootstrap config, static routing and dynamic routing from GRPC based xDS server.

Docs Changes:
Comment added to the new API field.

Release Notes:

Platform Specific Features:

The implementation is mostly copy form source/common/router/ minus the unnecessary parts. Unfortunatly no common base classes to share code. Signed-off-by: Tamas Kovacs <tamas.2.kovacs@nokia-sbell.com>

Signed-off-by: Tamas Kovacs <tamas.2.kovacs@nokia-sbell.com>

Conflicts: source/common/config/type_to_endpoint.cc Signed-off-by: Tamas Kovacs <tamas.2.kovacs@nokia-sbell.com>

Signed-off-by: Tamas Kovacs <tamas.2.kovacs@nokia-sbell.com>

Conflicts: api/envoy/extensions/filters/network/thrift_proxy/v4alpha/BUILD api/envoy/extensions/filters/network/thrift_proxy/v4alpha/route.proto api/envoy/extensions/filters/network/thrift_proxy/v4alpha/thrift_proxy.proto generated_api_shadow/envoy/annotations/resource.proto generated_api_shadow/envoy/extensions/filters/network/thrift_proxy/v3/route.proto generated_api_shadow/envoy/extensions/filters/network/thrift_proxy/v3/thrift_proxy.proto generated_api_shadow/envoy/extensions/filters/network/thrift_proxy/v4alpha/BUILD generated_api_shadow/envoy/extensions/filters/network/thrift_proxy/v4alpha/route.proto generated_api_shadow/envoy/extensions/filters/network/thrift_proxy/v4alpha/thrift_proxy.proto source/extensions/filters/network/thrift_proxy/config.cc test/extensions/filters/network/thrift_proxy/router_ratelimit_test.cc Signed-off-by: Tamas Kovacs <tamas.2.kovacs@nokia-sbell.com>

Adding template base classes for rds handling Use base classes instead previsous copy-pasted solution in thrift. Rework existing rds implementation for http to use the new base classes. Signed-off-by: Tamas Kovacs <tamas.2.kovacs@nokia-sbell.com>

Signed-off-by: Tamas Kovacs <tamas.2.kovacs@nokia-sbell.com>

Even if the config was invalid it was stored. This confused delta updates. Signed-off-by: Tamas Kovacs <tamas.2.kovacs@nokia-sbell.com>

Signed-off-by: Tamas Kovacs <tamas.2.kovacs@nokia-sbell.com>

Remove reference to Rds::ConfigFactory form Rds::RdsRouteConfigProviderImpl and RdsRouteConfigProviderImpl. Move init_manager.add() call into Rds::RouteConfigProviderManagerImpl::insertDynamicProvider, in this way no need to take care of it in each provider manager. Signed-off-by: Tamas Kovacs <tamas.2.kovacs@nokia-sbell.com>

Signed-off-by: Tamas Kovacs <tamas.2.kovacs@nokia-sbell.com>

tkovacs-2 · 2022-03-01T10:52:21Z

Are there integration tests planned?

There is unit test to check if the proper TRDS endpoints are referred towards server, also for thrift config with rds on filter level.
The integration of these two parts done by common code which already has higher level test, so I did not plan to add new.

Signed-off-by: Tamas Kovacs <tamas.2.kovacs@nokia-sbell.com>

tkovacs-2 · 2022-03-09T09:15:01Z

/retest

repokitteh-read-only · 2022-03-09T09:15:05Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #19982 (comment) was created by @tkovacs-2.

see: more, trace.

tkovacs-2 · 2022-03-10T10:16:36Z

/retest

repokitteh-read-only · 2022-03-10T10:16:40Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #19982 (comment) was created by @tkovacs-2.

see: more, trace.

Signed-off-by: Tamas Kovacs <tamas.2.kovacs@nokia-sbell.com>

adisuissa

/lgtm api
LGTM code changes.
/assign @htuch

htuch

Some tiny API nits. I think we should have a full integration test, since while individual parts and their integration may be tested, it would be helpful to see this demoed e2e in case we miss anything and to prevent any regression. Otherwise LGTM and ready to merge.

htuch · 2022-03-11T15:26:55Z

/wait

tkovacs-2 · 2022-03-12T13:31:26Z

I think we should have a full integration test, since while individual parts and their integration may be tested,

Ok, I have just noticed there are integration tests for thrift I try to add one for trds.

Signed-off-by: Tamas Kovacs <tamas.2.kovacs@nokia-sbell.com>

Signed-off-by: Tamas Kovacs <tamas.2.kovacs@nokia-sbell.com> Signed-off-by: Tamas Kovacs <tamas.2.kovacs@nokia-sbell.com>

htuch

LGTM, thanks!

wbpcode · 2022-03-15T05:43:14Z

Great PR. Thanks for the rds templating. Then, we can try to add dubbo rds to Envoy just by adding little codes.

Adding xDS routing discovery support for thrift proxy. The routing discovery is supported only through ADS. No separate service endpoints added for thrift routing config type url. This PR is split from envoyproxy#17631 Risk Level: Low Testing: - Unittest - Manual test with static routing from bootstrap config, static routing and dynamic routing from GRPC based xDS server. Docs Changes: Comment added to the new API field. Signed-off-by: Tamas Kovacs <tamas.2.kovacs@nokia-sbell.com>

Tamas Kovacs added 30 commits August 9, 2021 12:13

Adding xDS RDS support for thrift protocol.

7448070

The implementation is mostly copy form source/common/router/ minus the unnecessary parts. Unfortunatly no common base classes to share code. Signed-off-by: Tamas Kovacs <tamas.2.kovacs@nokia-sbell.com>

Fix generated_api_shadow

eb5dc21

Signed-off-by: Tamas Kovacs <tamas.2.kovacs@nokia-sbell.com>

Merge main

fcc2abf

Signed-off-by: Tamas Kovacs <tamas.2.kovacs@nokia-sbell.com>

Fix missed file

d1e2124

Signed-off-by: Tamas Kovacs <tamas.2.kovacs@nokia-sbell.com>

Fix conn_manager_test

4609c02

Signed-off-by: Tamas Kovacs <tamas.2.kovacs@nokia-sbell.com>

Fix router_ratelimit_test

f48daed

Signed-off-by: Tamas Kovacs <tamas.2.kovacs@nokia-sbell.com>

Fix clang_tidy

0022eed

Signed-off-by: Tamas Kovacs <tamas.2.kovacs@nokia-sbell.com>

More type urls can identify the same RouteDiscoveryService endpoint

3235184

Signed-off-by: Tamas Kovacs <tamas.2.kovacs@nokia-sbell.com>

Merge branch 'main' into add_thrift_rds

12aa485

Conflicts: source/common/config/type_to_endpoint.cc Signed-off-by: Tamas Kovacs <tamas.2.kovacs@nokia-sbell.com>

Fix merge conflict resolution, add test

6506f20

Signed-off-by: Tamas Kovacs <tamas.2.kovacs@nokia-sbell.com>

Dummy

e9bb514

Signed-off-by: Tamas Kovacs <tamas.2.kovacs@nokia-sbell.com>

Template base classes for rds handling.

fbf27dc

Adding template base classes for rds handling Use base classes instead previsous copy-pasted solution in thrift. Rework existing rds implementation for http to use the new base classes. Signed-off-by: Tamas Kovacs <tamas.2.kovacs@nokia-sbell.com>

Merge branch 'main' into add_thrift_rds_rework

cd88857

Signed-off-by: Tamas Kovacs <tamas.2.kovacs@nokia-sbell.com>

Fix RouteConfigUpdateReceiverImpl::onVhdsUpdate

aeb9476

Even if the config was invalid it was stored. This confused delta updates. Signed-off-by: Tamas Kovacs <tamas.2.kovacs@nokia-sbell.com>

Fix clang-tidy errors

7811138

Signed-off-by: Tamas Kovacs <tamas.2.kovacs@nokia-sbell.com>

Fix clang-tidy errors, missing build dependencies

508f711

Signed-off-by: Tamas Kovacs <tamas.2.kovacs@nokia-sbell.com>

Merge branch 'main' into add_thrift_rds_rework

c28ff34

Signed-off-by: Tamas Kovacs <tamas.2.kovacs@nokia-sbell.com>

Debug strange lua filter coverage issue

8ca0a4f

Signed-off-by: Tamas Kovacs <tamas.2.kovacs@nokia-sbell.com>

Revert: Debug strange lua filter coverage issue

639148d

Signed-off-by: Tamas Kovacs <tamas.2.kovacs@nokia-sbell.com>

Separate service endpoint to Thrift rds

054460e

Signed-off-by: Tamas Kovacs <tamas.2.kovacs@nokia-sbell.com>

Move service to thrift rds into api/envoy/services

0695c52

Signed-off-by: Tamas Kovacs <tamas.2.kovacs@nokia-sbell.com>

Kick CI

15e1747

Signed-off-by: Tamas Kovacs <tamas.2.kovacs@nokia-sbell.com>

Merge branch 'main' into add_thrift_rds

4414291

Signed-off-by: Tamas Kovacs <tamas.2.kovacs@nokia-sbell.com>

Add prefix to Thrift rds related proto names in the api.

b9ab26f

Signed-off-by: Tamas Kovacs <tamas.2.kovacs@nokia-sbell.com>

Fix missed file

44f9ea8

Signed-off-by: Tamas Kovacs <tamas.2.kovacs@nokia-sbell.com>

Kick CI

4ae0e46

Signed-off-by: Tamas Kovacs <tamas.2.kovacs@nokia-sbell.com>

Kick CI

ab9e419

Signed-off-by: Tamas Kovacs <tamas.2.kovacs@nokia-sbell.com>

Change TRDS method names similar to other DS method names

bbc830b

Signed-off-by: Tamas Kovacs <tamas.2.kovacs@nokia-sbell.com>

Tamas Kovacs added 2 commits March 1, 2022 14:57

Added more test for trds endpoints

4f38c0f

Signed-off-by: Tamas Kovacs <tamas.2.kovacs@nokia-sbell.com>

Rename Rds::Basic to Rds::Common

c0ed60c

Signed-off-by: Tamas Kovacs <tamas.2.kovacs@nokia-sbell.com>

tkovacs-2 commented Mar 9, 2022

View reviewed changes

Comment thread source/extensions/filters/network/thrift_proxy/config.cc Outdated

tkovacs-2 requested review from adisuissa and zuercher March 9, 2022 10:00

Remove trds service

38d2671

Signed-off-by: Tamas Kovacs <tamas.2.kovacs@nokia-sbell.com>

adisuissa previously approved these changes Mar 11, 2022

View reviewed changes

repokitteh-read-only Bot removed the api label Mar 11, 2022

htuch suggested changes Mar 11, 2022

View reviewed changes

Comment thread api/envoy/extensions/filters/network/thrift_proxy/v3/thrift_proxy.proto Outdated

Comment thread api/envoy/extensions/filters/network/thrift_proxy/v3/thrift_proxy.proto

Comment thread api/envoy/extensions/filters/network/thrift_proxy/v3/thrift_proxy.proto

repokitteh-read-only Bot added the waiting label Mar 11, 2022

Add trds integration test. Fix review comments

522032e

Signed-off-by: Tamas Kovacs <tamas.2.kovacs@nokia-sbell.com>

tkovacs-2 dismissed adisuissa’s stale review via 522032e March 13, 2022 05:38

repokitteh-read-only Bot added api and removed waiting labels Mar 13, 2022

Merge branch 'main' into add_thrift_rds

6d414b8

Signed-off-by: Tamas Kovacs <tamas.2.kovacs@nokia-sbell.com> Signed-off-by: Tamas Kovacs <tamas.2.kovacs@nokia-sbell.com>

tkovacs-2 requested review from adisuissa and htuch March 13, 2022 16:41

htuch approved these changes Mar 15, 2022

View reviewed changes

repokitteh-read-only Bot removed the api label Mar 15, 2022

htuch merged commit ec7ef59 into envoyproxy:main Mar 15, 2022

wbpcode mentioned this pull request Mar 15, 2022

dubbo rds for dubbo proxy #20344

Closed

Conversation

tkovacs-2 commented Feb 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tkovacs-2 commented Mar 1, 2022

Uh oh!

Uh oh!

tkovacs-2 commented Mar 9, 2022

Uh oh!

repokitteh-read-only Bot commented Mar 9, 2022

Uh oh!

tkovacs-2 commented Mar 10, 2022

Uh oh!

repokitteh-read-only Bot commented Mar 10, 2022

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

htuch commented Mar 11, 2022

Uh oh!

tkovacs-2 commented Mar 12, 2022

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

wbpcode commented Mar 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tkovacs-2 commented Feb 16, 2022 •

edited

Loading