-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
redirect: add support to specify response code #2030
Conversation
Signed-off-by: Constance Caramanolis <ccaramanolis@lyft.com>
Signed-off-by: Constance Caramanolis <ccaramanolis@lyft.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, few small comments
source/common/http/utility.h
Outdated
* @param callbacks supplies the filter callbacks to use. | ||
* @param new_path supplies the redirect target. | ||
* @param status_code supplies the response code to use. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: status_code vs. response_code below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
@@ -45,5 +45,23 @@ bool ConfigUtility::matchHeaders(const Http::HeaderMap& request_headers, | |||
return matches; | |||
} | |||
|
|||
Http::Code ConfigUtility::parseRedirectResponseCode( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please make sure you have coverage over all of this. There should be some tests in config_impl_test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a test case in config_impl_test.cc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need an actual test that loads a route table, gets the route, and makes sure a code is set. You don't have any test to make sure that that part is working. This should be in config_impl_test also. This may require converting one of the tests over to v2 YAML like we have been doing lately in other tests.
Signed-off-by: Constance Caramanolis <ccaramanolis@lyft.com>
Signed-off-by: Constance Caramanolis <ccaramanolis@lyft.com>
The patch is no longer needed as it was similarly patched upstream in nlohmann/json#3101. That patch was then shipped in v3.10.5, which is the version used in Envoy now. Signed-off-by: JP Simard <jp@jpsim.com>
The patch is no longer needed as it was similarly patched upstream in nlohmann/json#3101. That patch was then shipped in v3.10.5, which is the version used in Envoy now. Signed-off-by: JP Simard <jp@jpsim.com>
This PR allows for redirect actions to specify which HTTP Status code to return.
Consuming the enums created in envoyproxy/data-plane-api#225
Risk Level: Low
Testing: Unit Test
Signed-off-by: Constance Caramanolis ccaramanolis@lyft.com