thrift: Reject router config if cluster/weighted clusters/mirror cluster doesn't exist for static route config provider by JuniorHsu · Pull Request #20577 · envoyproxy/envoy

JuniorHsu · 2022-03-30T03:36:45Z

Signed-off-by: kuochunghsu [email protected]

Commit Message:
Expose ServerFactoryContext to ConfigTraitsImpl::createConfig so the underlying RouteEntryImpl is able to validate its clusters.

Currently HTTP validates the clusters for static route config by default but doesn't validate the clusters for RDS.
Moreover, optional field validate_clusters changes the default behavior.

In this diff, thrift route config provider manager do the same default behavior for cluster validation.
That is, validate the clusters for static route config by default but not validate the clusters for TRDS.

Optional field validate_clusters is coming soon.

Risk Level: low
Testing: unit test
[Optional Fixes #Issue] #20559

…er doesnt exist Signed-off-by: kuochunghsu <[email protected]>

JuniorHsu · 2022-03-30T03:52:16Z

Before I fixed all the interesting tests and add the appropriate test coverage.

Like http config setup, we don't validate the cluster for rds. Instead, only validate the clusters in static config.

Which introduce two interfaces I'm hesitating to determine:

virtual ConfigConstSharedPtr createConfig(const Protobuf::Message& rc,
                                            Server::Configuration::ServerFactoryContext& context,
                                            bool validate_clusters_default) const PURE;

I'm not sure if we need to have the flexibility the change the validate_clusters_default while createConfig
Or once we decide the configTraits, the rule is fixed.

Currently I keep the flexibility since reverting interface is expensive .
2.

RouteMatcher::RouteMatcher(
    const envoy::extensions::filters::network::thrift_proxy::v3::RouteConfiguration& config,
    Server::Configuration::ServerFactoryContext& factory_context, bool validate_clusters)

Same thing, for thrift RouteMatcher, we could pass the limited information absl::optional<Upstream::ClusterManager::ClusterInfoMaps> instead of while context.

It's not an interface, we might want the minimal exposure principal?

cc @zuercher @rgs1 @tkovacs-2

JuniorHsu · 2022-03-30T16:53:17Z

After I take another look, 1. is not an issue given the config traits in common/rds doesn't know if it should create a config with validation. Unless the traits take another parameters.

I'll go ahead with 2. to minimal exposure and fix the tests.

Signed-off-by: kuochunghsu <[email protected]>

rgs1

Generally LGTM, just one nit

source/extensions/filters/network/thrift_proxy/router/router_impl.cc

Signed-off-by: kuochunghsu <[email protected]>

test/extensions/filters/network/thrift_proxy/conn_manager_test.cc

tkovacs-2 · 2022-03-31T03:59:37Z

I'm not sure if we need to have the flexibility the change the validate_clusters_default while createConfig. Or once we decide the configTraits, the rule is fixed.

Since it is not protocol specific, I think the validate_clusters_default is ok here.

It's not an interface, we might want the minimal exposure principal?

I also vote for minimal exposure.

Signed-off-by: kuochunghsu <[email protected]>

tkovacs-2 · 2022-03-31T05:30:43Z

Don't you add flag for the validation into the config proto?
I'm not sure but in that case if the listener and the routing is in the bootstrap config but the clusters are through xDS then the validation fails at startup.

JuniorHsu · 2022-03-31T06:10:17Z

Don't you add flag for the validation into the config proto? I'm not sure but in that case if the listener and the routing is in the bootstrap config but the clusters are through xDS then the validation fails at startup.

No. Currently follow what the http did: only validate in static provider. No config proto is able to change the behavior at the moment.

As for your scenario, envoy will retry after the cluster arrives after it fails at startup without cluster information. Not perfect but it works.

JuniorHsu · 2022-03-31T06:12:25Z

Writing fixes to clang-tidy-fixes.yaml ...
clang-tidy check failed, potentially fixed by clang-apply-replacements:
Diagnostics:
- BuildDirectory: /build/tmp/_bazel_envoybuild/b570b5ccd0454dc9af9f65ab1833764d/execroot/envoy
  DiagnosticMessage:
    FileOffset: 298
    FilePath: ./source/common/common/logger.h
    Message: '''source/common/common/logger_impl.h'' file not found'
    Replacements: []
  DiagnosticName: clang-diagnostic-error
  Level: Error
  Ranges:
  - FileOffset: 298
    FilePath: ./source/common/common/logger.h
    Length: 36

For the test failure in "envoy-presubmit (check linux_x64 clang_tidy)" , I guess it's unrelated since I didn't touch nor include the logger.

JuniorHsu · 2022-03-31T17:51:27Z

Note: We might want validate_clusters in proto at some point like http.

envoy/api/envoy/config/route/v3/route.proto

Line 110 in a308789

google.protobuf.BoolValue validate_clusters = 7;

JuniorHsu · 2022-04-01T06:08:19Z

/retest

repokitteh-read-only · 2022-04-01T06:08:23Z

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

🐱

Caused by: a #20577 (comment) was created by @JuniorHsu.

see: more, trace.

rgs1

This looks reasonable to me, thanks.

@htuch do you have some cycles to look at the common RDS code?

tkovacs-2 · 2022-04-02T03:48:13Z

Note: We might want validate_clusters in proto at some point like http.

I would set the default to false even for of static case and add the flag to the proto to control the validation.
The current state is not fully backward compatible: If someone has a dummy/legacy rule referring to an unknown cluster but never matches, this change breaks a previously working system.

tkovacs-2 · 2022-04-02T09:32:04Z

test/extensions/filters/network/thrift_proxy/conn_manager_test.cc

+      route:
+        cluster: cluster2
+)EOF";
+


nit: if you want, you can add back the assert for the case when the cluster list is empty.

done. thanks!

htuch · 2022-04-04T00:21:46Z

envoy/rds/config_traits.h

-  virtual ConfigConstSharedPtr createConfig(const Protobuf::Message& rc) const PURE;
+  virtual ConfigConstSharedPtr createConfig(const Protobuf::Message& rc,
+                                            Server::Configuration::ServerFactoryContext& context,
+                                            bool validate_clusters_default) const PURE;


Please document the new params in the Doxygen-style comments above.

done. thanks.

source/extensions/filters/network/thrift_proxy/router/router_impl.cc

Signed-off-by: kuochunghsu <[email protected]>

source/common/rds/route_config_update_receiver_impl.cc

htuch

LGTM modulo testing question.

Signed-off-by: kuochunghsu <[email protected]>

JuniorHsu · 2022-04-05T00:31:25Z

@htuch it's ready to have another review with three follow-up items
(a) make validate_clusters configurable (I can do it after this lands)
(b) consider cluster manager return a ref
(c) consider remove ClusterHeader() from ThriftProxy::RouteEntry

thanks

htuch

LGTM, thanks!

JuniorHsu · 2022-04-05T17:31:31Z

Writing fixes to clang-tidy-fixes.yaml ...
clang-tidy check failed, potentially fixed by clang-apply-replacements:
Diagnostics:
- BuildDirectory: /build/tmp/_bazel_envoybuild/b570b5ccd0454dc9af9f65ab1833764d/execroot/envoy
  DiagnosticMessage:
    FileOffset: 298
    FilePath: ./source/common/common/logger.h
    Message: '''source/common/common/logger_impl.h'' file not found'
    Replacements: []
  DiagnosticName: clang-diagnostic-error
  Level: Error
  Ranges:
  - FileOffset: 298
    FilePath: ./source/common/common/logger.h
    Length: 36

For the test failure in "envoy-presubmit (check linux_x64 clang_tidy)" , I guess it's unrelated since I didn't touch nor include the logger.

This might need a manual merge.
The clang-tidy error from Error while processing /source/envoy/rds/config_traits.h.
which includes context files which include logger

However the clang-tidy can't parse strip_include_prefix

envoy/source/common/common/BUILD

Line 218 in 2be0d7d

strip_include_prefix = "standard",

wrowe · 2022-04-06T01:13:04Z

The logger was indirectly added by including "envoy/server/factory_context.h", probably without adding the deps for it to envoy/rds/config_traits.h

wrowe · 2022-04-06T01:13:24Z

/wait for deps to be patched up

Signed-off-by: kuochunghsu <[email protected]>

JuniorHsu · 2022-04-06T02:51:59Z

The logger was indirectly added by including "envoy/server/factory_context.h", probably without adding the deps for it to envoy/rds/config_traits.h

It's a promising solution and it takes some effort to break the circular reference. Next commit is comming

.-> //envoy/router:router_interface (8a9baa446c4cdc335df7924c31f550a82dcac9f2cefefd75763ac169448146ca)
|   //envoy/rds:rds_interface (8a9baa446c4cdc335df7924c31f550a82dcac9f2cefefd75763ac169448146ca)
|   //envoy/server:factory_context_interface (8a9baa446c4cdc335df7924c31f550a82dcac9f2cefefd75763ac169448146ca)
|   //envoy/http:filter_interface (8a9baa446c4cdc335df7924c31f550a82dcac9f2cefefd75763ac169448146ca)
`-- //envoy/router:router_interface (8a9baa446c4cdc335df7924c31f550a82dcac9f2cefefd75763ac169448146ca)

JuniorHsu · 2022-04-06T07:38:09Z

/retest

repokitteh-read-only · 2022-04-06T07:38:13Z

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

🐱

Caused by: a #20577 (comment) was created by @JuniorHsu.

see: more, trace.

wrowe · 2022-04-06T15:20:58Z

Would a reviewer please revisit? It looks that all the comments and feedback have been addressed.

JuniorHsu · 2022-04-06T17:26:05Z

@htuch could you move this forward? I commited @wrowe 's great suggestion to make the test happier. Thanks.

htuch

LGTM.

#20874) This is a continuation of #20577. Additional Description: Risk Level: low Testing: unit test Signed-off-by: kuochunghsu <[email protected]>

…ter doesn't exist for static route config provider (envoyproxy#20577) * Reject thrift router config if cluster/weighted clusters/mirror cluster don't exist * Only expose cluster information to `RouteMatcher` * Fix router ratelimit test, add unit test, add test coverage * address rgs1's , tkovacs-2's comment, Tamas and Harvey's comments * remove redundant ref, dead code and * add comment to state the current status * add dep lib Signed-off-by: kuochunghsu <[email protected]>

envoyproxy#20874) This is a continuation of envoyproxy#20577. Additional Description: Risk Level: low Testing: unit test Signed-off-by: kuochunghsu <[email protected]>

Reject thrift router config if cluster/weighted clusters/mirror clust…

ddd3d9b

…er doesnt exist Signed-off-by: kuochunghsu <[email protected]>

JuniorHsu requested a review from zuercher as a code owner March 30, 2022 03:36

JuniorHsu marked this pull request as draft March 30, 2022 03:37

kuochunghsu added 2 commits March 30, 2022 10:05

Only expose cluster information to RouteMatcher

48751ad

Signed-off-by: kuochunghsu <[email protected]>

Fix router ratelimit test

6c84567

Signed-off-by: kuochunghsu <[email protected]>

rgs1 reviewed Mar 30, 2022

View reviewed changes

source/extensions/filters/network/thrift_proxy/router/router_impl.cc Outdated Show resolved Hide resolved

Add unit test and address rgs1's comment

6ac0195

Signed-off-by: kuochunghsu <[email protected]>

JuniorHsu marked this pull request as ready for review March 30, 2022 19:41

tkovacs-2 reviewed Mar 31, 2022

View reviewed changes

test/extensions/filters/network/thrift_proxy/conn_manager_test.cc Outdated Show resolved Hide resolved

tkovacs-2 reviewed Mar 31, 2022

View reviewed changes

test/extensions/filters/network/thrift_proxy/conn_manager_test.cc Outdated Show resolved Hide resolved

tkovacs-2 reviewed Mar 31, 2022

View reviewed changes

test/extensions/filters/network/thrift_proxy/conn_manager_test.cc Outdated Show resolved Hide resolved

kuochunghsu added 2 commits March 30, 2022 21:03

address tkovacs-2's comment

8b2beb8

Signed-off-by: kuochunghsu <[email protected]>

fix nit

308ed34

Signed-off-by: kuochunghsu <[email protected]>

Merge remote-tracking branch 'upstream/main' into unknownCluster

23760b3

Merge remote-tracking branch 'upstream/main' into unknownCluster

d092234

rgs1 reviewed Apr 1, 2022

View reviewed changes

tkovacs-2 reviewed Apr 2, 2022

View reviewed changes

htuch reviewed Apr 4, 2022

View reviewed changes

Address Tamas and Harvey's comments

7f38390

Signed-off-by: kuochunghsu <[email protected]>

htuch reviewed Apr 4, 2022

View reviewed changes

source/common/rds/route_config_update_receiver_impl.cc Show resolved Hide resolved

htuch reviewed Apr 4, 2022

View reviewed changes

remove dead code and add test coverage

b05112d

Signed-off-by: kuochunghsu <[email protected]>

htuch previously approved these changes Apr 5, 2022

View reviewed changes

htuch enabled auto-merge (squash) April 5, 2022 03:51

JuniorHsu mentioned this pull request Apr 5, 2022

thrift: Explicitly express no cluster header for weighted clusters #20679

Merged

repokitteh-read-only bot added the waiting label Apr 6, 2022

add dep lib

08dc374

Signed-off-by: kuochunghsu <[email protected]>

auto-merge was automatically disabled April 6, 2022 02:52
Head branch was pushed to by a user without write access

JuniorHsu dismissed htuch’s stale review via 08dc374 April 6, 2022 02:52

repokitteh-read-only bot removed the waiting label Apr 6, 2022

wrowe enabled auto-merge (squash) April 6, 2022 15:19

wrowe self-assigned this Apr 6, 2022

htuch approved these changes Apr 7, 2022

View reviewed changes

wrowe merged commit 456e449 into envoyproxy:main Apr 7, 2022

JuniorHsu deleted the unknownCluster branch April 15, 2022 00:43

JuniorHsu mentioned this pull request Apr 19, 2022

thrift: configurable validate_clusters to override the default cluste… #20874

Merged

adisuissa mentioned this pull request Mar 10, 2026

vhds: add support for VHDS in a static route-configuration #43852

Open

Conversation

JuniorHsu commented Mar 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JuniorHsu commented Mar 30, 2022

Uh oh!

JuniorHsu commented Mar 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rgs1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tkovacs-2 commented Mar 31, 2022

Uh oh!

tkovacs-2 commented Mar 31, 2022

Uh oh!

JuniorHsu commented Mar 31, 2022

Uh oh!

JuniorHsu commented Mar 31, 2022

Uh oh!

JuniorHsu commented Mar 31, 2022

Uh oh!

JuniorHsu commented Apr 1, 2022

Uh oh!

repokitteh-read-only bot commented Apr 1, 2022

Uh oh!

rgs1 left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

tkovacs-2 Apr 2, 2022

Choose a reason for hiding this comment

Uh oh!

JuniorHsu Apr 4, 2022

Choose a reason for hiding this comment

Uh oh!

htuch Apr 4, 2022

Choose a reason for hiding this comment

Uh oh!

JuniorHsu Apr 4, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

JuniorHsu commented Apr 5, 2022

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

JuniorHsu commented Apr 5, 2022

Uh oh!

wrowe commented Apr 6, 2022

Uh oh!

wrowe commented Apr 6, 2022

Uh oh!

JuniorHsu commented Apr 6, 2022

Uh oh!

JuniorHsu commented Apr 6, 2022

Uh oh!

repokitteh-read-only bot commented Apr 6, 2022

Uh oh!

wrowe commented Apr 6, 2022

Uh oh!

JuniorHsu commented Apr 6, 2022

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

JuniorHsu commented Mar 30, 2022 •

edited

Loading

JuniorHsu commented Mar 30, 2022 •

edited

Loading

tkovacs-2 commented Apr 2, 2022 •

edited

Loading