http: return per route config when direct response is set by soulxu · Pull Request #17449 · envoyproxy/envoy

soulxu · 2021-07-22T09:22:40Z

Commit Message: http: return per route config when direct response is set
Additional Description:
When the direct response is set, the Route::routeEntry() could be nullptr, but it
doesn't mean there is no per route typed config. So the most specific route config
should be able to get from the class Route. So move the mostSpecificPerFilterConfigTyped
interface into class Route.
Risk Level: low
Testing: unittest
Docs Changes: n/a
Release Notes: n/a
[Optional Runtime guard:]
Fixes #17377

Signed-off-by: He Jie Xu hejie.xu@intel.com

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu · 2021-07-22T12:33:14Z

/retest

repokitteh-read-only · 2021-07-22T12:33:18Z

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

🐱

Caused by: a #17449 (comment) was created by @soulxu.

see: more, trace.

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

mattklein123

Thanks for fixing! One question.

/wait-any

envoy/router/router.h

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

envoy/router/router.h

soulxu · 2021-07-26T05:24:57Z

/retest

repokitteh-read-only · 2021-07-26T05:25:00Z

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

🐱

Caused by: a #17449 (comment) was created by @soulxu.

see: more, trace.

This reverts commit 202bf56. Signed-off-by: He Jie Xu <hejie.xu@intel.com>

This reverts commit 9cb5663. Signed-off-by: He Jie Xu <hejie.xu@intel.com>

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

mattklein123

Thanks, looks great. One small comment.

/wait

mattklein123 · 2021-07-28T15:49:35Z

source/common/router/config_impl.cc

+void RouteEntryImplBase::traversePerFilterConfig(
+    const std::string& filter_name,
+    std::function<void(const Router::RouteSpecificFilterConfig&)> cb) const {
+  const Router::RouteEntry* route_entry = routeEntry();


Isn't this the same bug? Shouldn't we not be gating this on routeEntry()? Or are you not fixing this because you want it changes extauth and you want to fix this separately? If the latter can you add a comment/TODO here?

Yes, I plan to fix this in separate PR, along with #17502, Let me add a todo.

Actually, I'm a little confused about the separate PR. I used to use gerrit before. Everybody is reviewing the each commit and merge each commit separately. But PR includes multiple commits, but look like people doesn't care about how many commits in a PR. But let me know what is preference way people do here :)

We squash all the commits when we merge, so just add as many commits as you want. The GH review system is very bad compared to Gerrit and this is the only way to make it reasonably work.

mattklein123

Thanks!

soulxu · 2021-07-29T02:44:02Z

/retest

repokitteh-read-only · 2021-07-29T02:44:06Z

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

🐱

Caused by: a #17449 (comment) was created by @soulxu.

see: more, trace.

mattklein123 · 2021-07-29T19:14:25Z

Sorry looks like this needs a main merge.

/wait

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu · 2021-07-29T23:37:21Z

Sorry looks like this needs a main merge.

/wait

done

mattklein123 · 2021-07-30T20:53:12Z

Sorry can you merge main again which should fix the verify examples breakage?

/wait

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu · 2021-07-31T00:18:11Z

Sorry can you merge main again which should fix the verify examples breakage?

/wait

haha, did merge again :)

mattklein123 · 2021-08-02T15:29:34Z

/retest

repokitteh-read-only · 2021-08-02T15:29:38Z

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

🐱

Caused by: a #17449 (comment) was created by @mattklein123.

see: more, trace.

mattklein123

Thanks!

ggreenway · 2021-08-04T19:46:45Z

/backport

This results in broken configs, with no workaround.

…bridge-stream * upstream/main: (32 commits) tls: move ssl connection info into SocketAddressProvider (envoyproxy#17334) conn pool: default enable runtime feature `conn_pool_delete_when_idle` (envoyproxy#17577) api: LEDS api introduction (envoyproxy#17419) kafka: add support for api versions request in mesh-filter (envoyproxy#17475) ext_proc: Implement BUFFERED_PARTIAL processing mode (envoyproxy#17531) tooling: Async/pathlib/mypy cleanups and utils (envoyproxy#17505) xds: restructure CertificateProvider fields (envoyproxy#17201) Refactor OverloadIntegrationTest breaking out a test base, and the fake resource monitors. (envoyproxy#17530) listener: move active connection collection out of active tcp listener (envoyproxy#16947) tools: format checks for backticks (envoyproxy#17566) coverage: set lower limit for common/quic and common (envoyproxy#17573) v2: final source removal (envoyproxy#17565) test: bumping coverage (envoyproxy#17564) quic: enforcing header size and contents (envoyproxy#17520) Support for canonicalizing URI properly for AWS SigV4 signer (envoyproxy#17137) listener: add a stat for transport socket connect timeout (envoyproxy#17458) listener: add listen() error handling (envoyproxy#17427) http: return per route config when direct response is set (envoyproxy#17449) removing most v2 references from source/ (envoyproxy#17415) bug fix: return bootstrap when validating config (envoyproxy#17499) ... Signed-off-by: Garrett Bourg <bourg@squareup.com>

Backport of envoyproxy#17449, e071417 Co-authored-by: He Jie Xu <hejie.xu@intel.com> Signed-off-by: Greg Greenway <ggreenway@apple.com>

Backport of #17449, e071417 Co-authored-by: He Jie Xu <hejie.xu@intel.com> Signed-off-by: Greg Greenway <ggreenway@apple.com>

…#17449) Signed-off-by: He Jie Xu <hejie.xu@intel.com>

http: return per route config when direct response is set

4dfbd60

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu requested review from alyssawilk, htuch, lizan, mattklein123, snowp and zuercher as code owners July 22, 2021 09:22

Fix test

53a962d

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

htuch assigned mattklein123 Jul 23, 2021

mattklein123 reviewed Jul 23, 2021

View reviewed changes

envoy/router/router.h Show resolved Hide resolved

repokitteh-read-only bot added waiting:any and removed waiting:any labels Jul 23, 2021

soulxu added 4 commits July 25, 2021 13:58

Move the traversePerFilterConfig into Route

305600d

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

remove the perFilterConfigTyped

f073a66

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

Moving the perfilterConfig method as private

9cb5663

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

fix format

202bf56

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu commented Jul 25, 2021

View reviewed changes

envoy/router/router.h Outdated Show resolved Hide resolved

mattklein123 added the waiting label Jul 26, 2021

soulxu added 5 commits July 28, 2021 05:52

Revert "fix format"

460a4c4

This reverts commit 202bf56. Signed-off-by: He Jie Xu <hejie.xu@intel.com>

Revert "Moving the perfilterConfig method as private"

2fa8268

This reverts commit 9cb5663. Signed-off-by: He Jie Xu <hejie.xu@intel.com>

remove the dependence of perfiltertypedconfig

f412b30

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

remove useless perFilterConfig interface

e09d2c9

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

fix format

cf40947

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

repokitteh-read-only bot removed the waiting label Jul 28, 2021

mattklein123 requested changes Jul 28, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Jul 28, 2021

mattklein123 previously approved these changes Jul 28, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Jul 29, 2021

Merge branch 'main' into fix_per_filter_regression

a5e695f

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu dismissed mattklein123’s stale review via a5e695f July 29, 2021 23:37

repokitteh-read-only bot removed the waiting label Jul 29, 2021

soulxu mentioned this pull request Jul 30, 2021

ext_authz: do the authentication even the direct response is set #17546

Merged

repokitteh-read-only bot added the waiting label Jul 30, 2021

Merge branch 'main' into fix_per_filter_regression

7d5afe2

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

repokitteh-read-only bot removed the waiting label Jul 31, 2021

mattklein123 approved these changes Aug 2, 2021

View reviewed changes

mattklein123 merged commit e071417 into envoyproxy:main Aug 2, 2021

repokitteh-read-only bot added the backport/review Request to backport to stable releases label Aug 4, 2021

ggreenway added a commit to ggreenway/envoy that referenced this pull request Aug 5, 2021

http: return per route config when direct response is set

2496767

Backport of envoyproxy#17449, e071417 Co-authored-by: He Jie Xu <hejie.xu@intel.com> Signed-off-by: Greg Greenway <ggreenway@apple.com>

ggreenway mentioned this pull request Aug 5, 2021

http: return per route config when direct response is set #17602

Merged

ggreenway added a commit that referenced this pull request Aug 6, 2021

http: return per route config when direct response is set (#17602)

68920f7

Backport of #17449, e071417 Co-authored-by: He Jie Xu <hejie.xu@intel.com> Signed-off-by: Greg Greenway <ggreenway@apple.com>

ggreenway added backport/approved Approved backports to stable releases and removed backport/review Request to backport to stable releases labels Aug 9, 2021

mattklein123 mentioned this pull request Aug 11, 2021

[route] Create route-table level typed_per_filter_config for "global" route config. #17662

Closed

soulxu mentioned this pull request Aug 13, 2021

ext_authz: support external authorization checks when a route contains a direct response entry #14989

Closed

leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request Sep 30, 2021

http: return per route config when direct response is set (envoyproxy…

3e4d876

…#17449) Signed-off-by: He Jie Xu <hejie.xu@intel.com>

Conversation

soulxu commented Jul 22, 2021

Uh oh!

soulxu commented Jul 22, 2021

Uh oh!

repokitteh-read-only bot commented Jul 22, 2021

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

soulxu commented Jul 26, 2021

Uh oh!

repokitteh-read-only bot commented Jul 26, 2021

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 Jul 28, 2021

Choose a reason for hiding this comment

Uh oh!

soulxu Jul 28, 2021

Choose a reason for hiding this comment

Uh oh!

mattklein123 Jul 28, 2021

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

soulxu commented Jul 29, 2021

Uh oh!

repokitteh-read-only bot commented Jul 29, 2021

Uh oh!

mattklein123 commented Jul 29, 2021

Uh oh!

soulxu commented Jul 29, 2021

Uh oh!

mattklein123 commented Jul 30, 2021

Uh oh!

soulxu commented Jul 31, 2021

Uh oh!

mattklein123 commented Aug 2, 2021

Uh oh!

repokitteh-read-only bot commented Aug 2, 2021

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

ggreenway commented Aug 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants