local ratelimit support virtualhost config by StarryVae · Pull Request #22895 · envoyproxy/envoy

StarryVae · 2022-08-30T10:06:22Z

Signed-off-by: wangkai19 wangkai19@corp.netease.com

Commit Message: local ratelimit support virtualhost config
Additional Description: N/A
Risk Level: low
Testing: ut
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: wangkai19 <wangkai19@corp.netease.com>

repokitteh-read-only · 2022-08-30T10:06:30Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @adisuissa
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #22895 was opened by StarryVae.

see: more, trace.

StarryVae · 2022-08-30T10:13:57Z

issue #22793 PTAL, thanks. @wbpcode @mattklein123

adisuissa

Thanks for the fix!
Left a high-level API comment.
I think that the code that handles this configuration should also be shared (I'm assuming that if one adds any other enum value, or updates the code, this should be reflected for both the per-route and the local rate-limit).

adisuissa · 2022-08-30T10:55:38Z

+  }
+
+  // Specifies if the local rate limit filter should include the virtual host rate limits.
+  VhRateLimitsOptions vh_rate_limits = 13 [(validate.rules).enum = {defined_only: true}];


IMHO the enum from envoy.extensions.filters.http.ratelimit.v3 should be used.

It would have been better to move this enum to some common location, but unfortunately I think it will break the current API.

yes, thanks, i will use envoy.extensions.filters.http.ratelimit.v3 instead.

x1b[0m/build/tmp/_bazel_envoybuild/b570b5ccd0454dc9af9f65ab1833764d/external/envoy_api/envoy/extensions/filters/http/local_ratelimit/v3/BUILD:7:18: GoCompilePkg external/envoy_api/envoy/extensions/filters/http/local_ratelimit/v3/pkg_go_proto.a failed: (Exit 1): builder failed: error executing command (from target @envoy_api//envoy/extensions/filters/http/local_ratelimit/v3:pkg_go_proto) bazel-out/k8-opt-exec-2B5CBBC6/bin/external/go_sdk/builder compilepkg -sdk external/go_sdk -installsuffix linux_amd64 -src ... (remaining 85 arguments skipped)\n\nUse --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging\nbazel-out/k8-fastbuild/bin/external/envoy_api/envoy/extensions/filters/http/local_ratelimit/v3/pkg_go_proto_/github.com/envoyproxy/go-control-plane/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.pb.validate.go:43:6: undefined: "github.com/envoyproxy/go-control-plane/envoy/extensions/filters/http/ratelimit/v3".XRateLimitHeadersRFCVersion\nbazel-out/k8-fastbuild/bin/external/envoy_api/envoy/extensions/filters/http/local_ratelimit/v3/pkg_go_proto_/github.com/envoyproxy/go-control-plane/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.pb.validate.go:343:11: assignment mismatch: 2 variables but 1 value\nbazel-out/k8-fastbuild/bin/external/envoy_api/envoy/extensions/filters/http/local_ratelimit/v3/pkg_go_proto_/github.com/envoyproxy/go-control-plane/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.pb.validate.go:343:14: undefined: "github.com/envoyproxy/go-control-plane/envoy/extensions/common/ratelimit/v3".RateLimitPerRoute_VhRateLimitsOptions_name\ncompilepkg: error running subcommand external/go_sdk/pkg/tool/linux_amd64/compile: exit status 2\n\x1b[32mINFO: \x1b[0mElapsed time: 189.979s, Critical Path: 20.43s\n\x1b[32mINFO: \x1b[0m1278 processes: 24 internal, 1254 processwrapper-sandbox.\n\x1b[31m\x1b[1mFAILED:\x1b[0m Build did NOT complete successfully\n\x1b[31m\x1b[1mFAILED:\x1b[0m Build did NOT complete successfully\n\x1b[0m'

@adisuissa i tried use envoy.extensions.filters.http.ratelimit.v3, but the bazel.api check failed, detailed errors are above. it seems that the go_package name of envoy.extensions.common.ratelimit.v3 and envoy.extensions.filters.http.ratelimit.v3 conflict?

Yeah, sorry about that.
I guess the way to get around this is to do something similar to what was done for XRateLimitHeadersRFCVersion (context in PR #18157):

Copy the enum into common.

In the local ratelimit use the enum from common.

Add comments for the definition and usage of the enum (similar to this and this).

ok, thanks！

wbpcode · 2022-08-31T01:52:05Z

Please check the CI, thanks.

/wait

Signed-off-by: wangkai19 <wangkai19@corp.netease.com>

StarryVae · 2022-09-01T01:50:58Z

/retest

repokitteh-read-only · 2022-09-01T01:51:02Z

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

🐱

Caused by: a #22895 (comment) was created by @StarryVae.

see: more, trace.

StarryVae · 2022-09-01T01:57:54Z

WARNING: Download from https://github.com/antlr/antlr4/archive/v4.10.1.tar.gz failed: class java.io.FileNotFoundException GET returned 404 Not Found
ERROR: An error occurred during the fetch of repository 'antlr4_runtimes':

this blocks the pipeline.

StarryVae · 2022-09-01T15:50:06Z

/retest

repokitteh-read-only · 2022-09-01T15:50:10Z

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

🐱

Caused by: a #22895 (comment) was created by @StarryVae.

see: more, trace.

StarryVae · 2022-09-02T01:32:00Z

@wbpcode CI is passed, PTAL, thanks.

wbpcode

Thanks for this contribution and some comments are added.

wbpcode · 2022-09-06T01:51:06Z

+
+  switch (vh_rate_limit_option) {
+  case VhRateLimitOptions::Ignore:
+    break;


nit: maybe use return here?

done, thanks.

wbpcode · 2022-09-06T01:51:28Z

+  default:
+    PANIC("not reached");


nit: PANIC_DUE_TO_CORRUPT_ENUM could be used here.

done, thanks.

wbpcode · 2022-09-06T01:56:11Z

+      switch (specific_per_route_config->virtualHostRateLimits()) {
+      case envoy::extensions::common::ratelimit::v3::INCLUDE:
+        vh_rate_limits_ = VhRateLimitOptions::Include;
+        break;
+      case envoy::extensions::common::ratelimit::v3::IGNORE:
+        vh_rate_limits_ = VhRateLimitOptions::Ignore;
+        break;
+      case envoy::extensions::common::ratelimit::v3::OVERRIDE:
+      default:
+        vh_rate_limits_ = VhRateLimitOptions::Override;
+      }


use PANIC_ON_PROTO_ENUM_SENTINEL_VALUES here to avoid the default usage.

done, thanks. @wbpcode

adisuissa

/lgtm api

Signed-off-by: wangkai19 <wangkai19@corp.netease.com>

wbpcode · 2022-09-08T03:52:50Z

It's looks good overall for me. And please merge main again to fix the CI. Thanks.

/wait

Signed-off-by: wangkai19 <wangkai19@corp.netease.com>

wbpcode · 2022-09-13T01:45:11Z

/wait

Signed-off-by: wangkai19 <wangkai19@corp.netease.com>

adisuissa

/lgtm api

StarryVae · 2022-09-15T01:53:45Z

@wbpcode CI is passed, PTAL again, thanks.

wbpcode

LGTM. Thanks. Defer this to @mattklein123 for final pass.

mattklein123

Thanks LGTM. Can you add a release note please?

/wait

Signed-off-by: wangkai19 <wangkai19@corp.netease.com>

StarryVae · 2022-09-16T06:02:14Z

Thanks LGTM. Can you add a release note please?

/wait

done, thanks.

Signed-off-by: wangkai19 <wangkai19@corp.netease.com>

wbpcode · 2022-09-16T07:53:35Z

/retest

repokitteh-read-only · 2022-09-16T07:53:39Z

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #22895 (comment) was created by @wbpcode.

see: more, trace.

StarryVae · 2022-09-16T08:20:38Z

/retest

repokitteh-read-only · 2022-09-16T08:20:42Z

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

🐱

Caused by: a #22895 (comment) was created by @StarryVae.

see: more, trace.

StarryVae · 2022-09-16T10:31:05Z

/retest

repokitteh-read-only · 2022-09-16T10:31:09Z

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

🐱

Caused by: a #22895 (comment) was created by @StarryVae.

see: more, trace.

mattklein123

Thanks!

local ratelimit support virtualhost config

f4ae0ba

Signed-off-by: wangkai19 <wangkai19@corp.netease.com>

StarryVae requested review from mattklein123 and wbpcode as code owners August 30, 2022 10:06

repokitteh-read-only Bot added the api label Aug 30, 2022

repokitteh-read-only Bot assigned adisuissa Aug 30, 2022

adisuissa reviewed Aug 30, 2022

View reviewed changes

mattklein123 assigned wbpcode Aug 30, 2022

repokitteh-read-only Bot added the waiting label Aug 31, 2022

fix pb

d9f3eef

Signed-off-by: wangkai19 <wangkai19@corp.netease.com>

repokitteh-read-only Bot removed the waiting label Aug 31, 2022

StarryVae added 2 commits August 31, 2022 20:32

fix bazel.api test

1c88841

Signed-off-by: wangkai19 <wangkai19@corp.netease.com>

fix pb

de62c50

Signed-off-by: wangkai19 <wangkai19@corp.netease.com>

wbpcode reviewed Sep 6, 2022

View reviewed changes

adisuissa reviewed Sep 6, 2022

View reviewed changes

repokitteh-read-only Bot removed the api label Sep 6, 2022

fix some review comments

6771450

Signed-off-by: wangkai19 <wangkai19@corp.netease.com>

repokitteh-read-only Bot added the waiting label Sep 8, 2022

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

6a1fd66

Signed-off-by: wangkai19 <wangkai19@corp.netease.com>

repokitteh-read-only Bot added api and removed waiting labels Sep 8, 2022

StarryVae added 2 commits September 9, 2022 16:27

ad more test

c8ff897

Signed-off-by: wangkai19 <wangkai19@corp.netease.com>

fix format

f6e4d3e

Signed-off-by: wangkai19 <wangkai19@corp.netease.com>

repokitteh-read-only Bot added the waiting label Sep 13, 2022

optimize some code

344bd20

Signed-off-by: wangkai19 <wangkai19@corp.netease.com>

repokitteh-read-only Bot removed the waiting label Sep 14, 2022

adisuissa reviewed Sep 14, 2022

View reviewed changes

repokitteh-read-only Bot removed the api label Sep 14, 2022

wbpcode previously approved these changes Sep 16, 2022

View reviewed changes

wbpcode assigned mattklein123 Sep 16, 2022

mattklein123 requested changes Sep 16, 2022

View reviewed changes

repokitteh-read-only Bot added the waiting label Sep 16, 2022

StarryVae added 2 commits September 16, 2022 13:56

add release note

8937891

Signed-off-by: wangkai19 <wangkai19@corp.netease.com>

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

be35e5f

Signed-off-by: wangkai19 <wangkai19@corp.netease.com>

StarryVae dismissed wbpcode’s stale review via 8937891 September 16, 2022 05:59

repokitteh-read-only Bot removed the waiting label Sep 16, 2022

wbpcode reviewed Sep 16, 2022

View reviewed changes

Comment thread changelogs/current.yaml Outdated

StarryVae added 2 commits September 16, 2022 14:28

fix review comments

49c0d8e

Signed-off-by: wangkai19 <wangkai19@corp.netease.com>

fix format

c538345

Signed-off-by: wangkai19 <wangkai19@corp.netease.com>

mattklein123 approved these changes Sep 16, 2022

View reviewed changes

mattklein123 merged commit 53837e6 into envoyproxy:main Sep 16, 2022

Conversation

StarryVae commented Aug 30, 2022

Uh oh!

repokitteh-read-only Bot commented Aug 30, 2022

Uh oh!

StarryVae commented Aug 30, 2022

Uh oh!

adisuissa 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!

wbpcode commented Aug 31, 2022

Uh oh!

StarryVae commented Sep 1, 2022

Uh oh!

repokitteh-read-only Bot commented Sep 1, 2022

Uh oh!

StarryVae commented Sep 1, 2022

Uh oh!

StarryVae commented Sep 1, 2022

Uh oh!

repokitteh-read-only Bot commented Sep 1, 2022

Uh oh!

StarryVae commented Sep 2, 2022

Uh oh!

wbpcode 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!

Choose a reason for hiding this comment

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

wbpcode commented Sep 8, 2022

Uh oh!

wbpcode commented Sep 13, 2022

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

StarryVae commented Sep 15, 2022

Uh oh!

wbpcode left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

StarryVae commented Sep 16, 2022

Uh oh!

Uh oh!

wbpcode commented Sep 16, 2022

Uh oh!

repokitteh-read-only Bot commented Sep 16, 2022

Uh oh!

StarryVae commented Sep 16, 2022

Uh oh!

repokitteh-read-only Bot commented Sep 16, 2022

Uh oh!

StarryVae commented Sep 16, 2022

Uh oh!