server: Warn in logs on admin mutations via GET. Mutations should be POSTed. #2971
Conversation
Signed-off-by: Joshua Marantz <[email protected]>
envoyproxy/envoy#2971 adds warning-checks that mutations should be POSTed. This documents that status. In a future PR, mutations will fail if they are not POSTs. See envoyproxy/envoy#2763 for more detail. Also adds a doc stub for @jsedgewick to fill in for /runtime_modify.
Signed-off-by: Joshua Marantz <[email protected]>
source/server/http/admin.cc
Outdated
| @@ -758,7 +757,7 @@ void AdminImpl::createFilterChain(Http::FilterChainFactoryCallbacks& callbacks) | |||
| callbacks.addStreamDecoderFilter(Http::StreamDecoderFilterSharedPtr{new AdminFilter(*this)}); | |||
| } | |||
|
|
|||
| Http::Code AdminImpl::runCallback(absl::string_view path_and_query, | |||
| Http::Code AdminImpl::runCallback(absl::string_view path_and_query, AdminFilter* admin_filter, | |||
There was a problem hiding this comment.
nit: AdminFilter& admin_filter (can't be null)
Signed-off-by: Joshua Marantz <[email protected]>
Signed-off-by: Joshua Marantz <[email protected]>
source/server/http/admin.cc
Outdated
| @@ -770,6 +769,14 @@ Http::Code AdminImpl::runCallback(absl::string_view path_and_query, | |||
|
|
|||
| for (const UrlHandler& handler : handlers_) { | |||
| if (path_and_query.compare(0, query_index, handler.prefix_) == 0) { | |||
| if (handler.mutates_server_state_) { | |||
| const absl::string_view method = | |||
| admin_filter.requestHeaders().Method()->value().getStringView(); | |||
There was a problem hiding this comment.
Sorry I just noticed this. Is there any reason to pass the entire filter here vs. just passing the request headers? Seems a bit cleaner unless it's necessary (we can avoid accessor, etc.).
There was a problem hiding this comment.
Good question. What was on my mind here was eliminating the need for the dynamic_cast in https://github.com/envoyproxy/envoy/pull/2736/files/b9a4301d5b885ec2c244315791768edd68fa8352 in #2736 .
It makes sense to me to have admin handlers get more request context in general. And in the case of that PR, what's needed is Http::StreamDecoderFilterCallbacks* callbacks_. I'm also fine reducing the exposure now to just request-headers, and that could be broadened to the entire AdminFilter& in the future if needed. I admit, though, I don't really know whether those callbacks_ are truly needed to solve the hystrix issues.
It's a question of speculative generality vs. reducing the need for an antipated refactor in the future, and I'm fine either way.
There was a problem hiding this comment.
My preference is to keep it simple for now unless there is a proven need, but I don't feel that strongly about it.
…ative generality. We can always capture the AdminFilter* in a later PR if & when that is needed. Signed-off-by: Joshua Marantz <[email protected]>
envoyproxy/envoy#2971 adds warning-checks that mutations should be POSTed. This documents that status. In a future PR, mutations will fail if they are not POSTs. See envoyproxy/envoy#2763 for more detail. Signed-off-by: Joshua Marantz <[email protected]>
source/server/http/admin.h
Outdated
| @@ -31,6 +31,8 @@ | |||
| namespace Envoy { | |||
| namespace Server { | |||
|
|
|||
| class AdminFilter; | |||
Signed-off-by: Joshua Marantz <[email protected]>
envoyproxy/envoy#2971 adds warning-checks that mutations should be POSTed. This documents that status. In a future PR, mutations will fail if they are not POSTs. See envoyproxy/envoy#2763 for more detail. Signed-off-by: Joshua Marantz <[email protected]>
Description:
When a mutating admin-console endpoint is reached, verify that it's a POST. For now, the operation succeeds but a warning log is printed.
This is a second attempt of #2912, which (a) contained an earlier version of the log-testing infra
from that was submitted #2930, and (b) was impacted by a merged commit that lacked a DCO.
This is a step toward fixing #2763
Risk Level: Low
Testing: //test/...
Docs Changes:
Link to Data Plane PR]
[in progress, will update]
Release Notes: [in progress, will update]
If this change is user impacting you must add a release note via a discrete PR to
version_history.rst.
API Changes: envoyproxy/data-plane-api#602
Deprecated:
Deprecates supporting admin mutations with a GET.