Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed status check #2032

Closed
wants to merge 2 commits into from
Closed

Fixed status check #2032

wants to merge 2 commits into from

Conversation

nmnellis
Copy link

@nmnellis nmnellis commented Nov 9, 2017

  • router:fix lua status code

Description:

What does this PR do? What was the behavior before the PR?
What is the behavior with the PR? If fixing a bug, please describe what
the original issue is and how the change resolves it. How does this
feature get enabled? By default, config change, etc...

Risk Level: Low

Low: Small bug fix or small optional feature.

Signed-off-by: Nick <nick.nellis@weather.com>
@@ -117,7 +117,7 @@ int StreamHandleWrapper::luaRespond(lua_State* state) {
uint64_t status;
if (headers->Status() == nullptr ||
!StringUtil::atoul(headers->Status()->value().c_str(), status) || status < 200 ||
status >= 500) {
status >= 600) {
Copy link
Member

Choose a reason for hiding this comment

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

Thanks. What about the status < 200 clause and claim in error that the range 100-599?

Signed-off-by: Nick Nellis <nick.nellis@weather.com>
@nmnellis
Copy link
Author

nmnellis commented Nov 9, 2017

@htuch fixed lowerbound

@@ -116,8 +116,8 @@ int StreamHandleWrapper::luaRespond(lua_State* state) {

uint64_t status;
if (headers->Status() == nullptr ||
!StringUtil::atoul(headers->Status()->value().c_str(), status) || status < 200 ||
status >= 500) {
!StringUtil::atoul(headers->Status()->value().c_str(), status) || status < 100 ||
Copy link
Member

Choose a reason for hiding this comment

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

Informational (1xx) status codes have special meaning and we should not allow a Lua script to send them, as they will confuse the connection manager, so that part was correct, but the error message below is wrong. But the other one was a bug. Thank you for fixing. We should have a test though. Do you mind changing one of the response tests to use a 5xx status code? If you don't want to bother I can fix this later today.

Copy link
Author

Choose a reason for hiding this comment

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

I am leaving town this afternoon and will be off for a few days. Otherwise i would be able to fix it after i get back.

Copy link
Member

Choose a reason for hiding this comment

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

Either way. If you would like to contribute we can wait, otherwise I'm happy to fix it. If you want to go that route just close the PR and I will do another PR later today.

Copy link
Author

Choose a reason for hiding this comment

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

You can add the fix, i would rather come back to it merged than wait

Copy link
Member

Choose a reason for hiding this comment

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

@mattklein123
Copy link
Member

Closing out in favor of #2034

Shikugawa pushed a commit to Shikugawa/envoy that referenced this pull request Mar 28, 2020
jpsim added a commit that referenced this pull request Nov 28, 2022
Previous URL started returning a zip archive with a different checksum:
bazelbuild/rules_android#43

Signed-off-by: JP Simard <jp@jpsim.com>
jpsim added a commit that referenced this pull request Nov 29, 2022
Previous URL started returning a zip archive with a different checksum:
bazelbuild/rules_android#43

Signed-off-by: JP Simard <jp@jpsim.com>
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.

4 participants