Skip to content

ratelimit add support for custom http response code#20520

Merged
htuch merged 6 commits intoenvoyproxy:mainfrom
KuonjiMayoi:ratelimit-support-custom-response-code
Apr 10, 2022
Merged

ratelimit add support for custom http response code#20520
htuch merged 6 commits intoenvoyproxy:mainfrom
KuonjiMayoi:ratelimit-support-custom-response-code

Conversation

@KuonjiMayoi
Copy link
Copy Markdown
Contributor

Signed-off-by: xiada maxxd@qq.com

Commit Message:ratelimit add support for custom http response code
Additional Description:
Risk Level:low
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:

Signed-off-by: xiada <maxxd@qq.com>
@repokitteh-read-only
Copy link
Copy Markdown

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

🐱

Caused by: #20520 was opened by KuonjiMayoi.

see: more, trace.

@daixiang0
Copy link
Copy Markdown
Member

I am curious in which cases it helps, please share more details.

@KuonjiMayoi
Copy link
Copy Markdown
Contributor Author

I am curious in which cases it helps, please share more details.

In our case, we usually use 5xx code to inform the client that the backend is is not able to provide service when it depends on the current capacity of the backend

: absl::nullopt),
http_context_(http_context), stat_names_(scope.symbolTable()) {}
http_context_(http_context), stat_names_(scope.symbolTable()),
status_(toErrorCode(config.status().code())) {}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add test for those new code? Then the code LGTM.

Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@esmet can you PTAL for initial pass? Thanks.

Signed-off-by: xiada <maxxd@qq.com>
Signed-off-by: xiada <maxxd@qq.com>
@KuonjiMayoi
Copy link
Copy Markdown
Contributor Author

@soulxu @htuch @esmet I have done the changes, PTAL,thanks~

const std::string rate_limited_status_config_ = R"EOF(
domain: foo
rate_limited_status:
code: 503
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great to test the case of status code < 400

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok,i think it's good to add another test for this case?I will do it later,thanks~

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@soulxu I have add the test for case of status code < 400,PTAL,thanks~

Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm api

@repokitteh-read-only repokitteh-read-only bot removed the api label Apr 1, 2022
soulxu
soulxu previously approved these changes Apr 5, 2022
Copy link
Copy Markdown
Member

@soulxu soulxu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for all the update!

Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, one last thing before merging, can you please add an entry to https://github.com/envoyproxy/envoy/blob/main/docs/root/version_history/current.rst and link to the new field from there? Thanks.

@KuonjiMayoi
Copy link
Copy Markdown
Contributor Author

Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@htuch htuch merged commit 6c297a7 into envoyproxy:main Apr 10, 2022
ravenblackx pushed a commit to ravenblackx/envoy that referenced this pull request Jun 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants