Set content-type and content-length#4113
Set content-type and content-length#4113mattklein123 merged 3 commits intoenvoyproxy:masterfrom dio:set-content-type-and-length
Conversation
This patch sets the content-type and content-length of HTTP subscription requests. Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
LGTM, thanks for fixing. 1 question.
| {":authority", "foo_cluster"}}), | ||
| request->headers()); | ||
| Http::TestHeaderMapImpl expected_headers{ | ||
| {":method", v2_rest_ ? "POST" : "GET"}, |
There was a problem hiding this comment.
Any reason to not just do this for v1 also?
There was a problem hiding this comment.
No specific reason. My intention was only converting existing relevant tests.
There was a problem hiding this comment.
I think it's probably work doing in both places since it's more correct and it will remove the special cases from the tests?
There was a problem hiding this comment.
Sorry, a clarification: If I get you correctly, you want to make v1 Envoy's SDS subscription "agent" sends requests using POST (and sends along node information to the management server) as well? Since the current SDS request only sets method (as GET) and path only.
envoy/source/common/upstream/sds_subscription.cc
Lines 86 to 87 in 2662bf1
Or keep it GET but send the node info via headers (but this is a different story).
There was a problem hiding this comment.
I was just suggesting to set content-length and content-type for the v1 requests so the tests don't need special casing?
There was a problem hiding this comment.
I don't think we can do that since the GET request for v1 has no payload sent along with the request.
There was a problem hiding this comment.
You can set content-length to 0 and still set the content-type? It's not a big deal but it seems like we should be consistent if possible?
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
| request.headers().insertContentType().value().setReference( | ||
| Http::Headers::get().ContentTypeValues.Json); | ||
|
|
||
| const size_t empty_body_size = 0; |
There was a problem hiding this comment.
nit: I don't think the local var here adds much. 0 is pretty obvious and I would just pass to value. Same elsewhere.
| {":authority", "foo_cluster"}, | ||
| {"content-type", "application/json"}, | ||
| {"content-length", | ||
| v2_rest_ ? fmt::FormatInt(request->body()->length()).str() : "0"}}), |
There was a problem hiding this comment.
Do we need to pivot on v2 here? Can we just pivot on the existence of a body or not? Same below.
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
* origin/master: (38 commits) test: add tests for corner-cases around sending requests before run() starts or after run() ends. (envoyproxy#4114) perf: reduce the memory usage of LC Trie construction (envoyproxy#4117) test: moving redundant code in websocket_integration_test to utilities (envoyproxy#4127) test: make YamlLoadFromStringFail less picky about error msg. (envoyproxy#4141) rbac: add rbac network filter. (envoyproxy#4083) fuzz: route lookup and header finalization fuzzer. (envoyproxy#4116) Set content-type and content-length (envoyproxy#4113) fault: use FractionalPercent for percent (envoyproxy#3978) test: Fix inverted exact match logic in IntegrationTcpClient::waitForData() (envoyproxy#4134) Added cluster_name to load assignment config for static cluster (envoyproxy#4123) ssl: refactor ContextConfig to use TlsCertificateConfig (envoyproxy#4115) syscall: refactor OsSysCalls for deeper errno latching (envoyproxy#4111) thrift_proxy: fix oneway bugs (envoyproxy#4025) Do not crash when converting YAML to JSON fails (envoyproxy#4110) config: allow unknown fields flag (take 2) (envoyproxy#4096) Use a jittered backoff strategy for handling HdsDelegate stream/connection failures (envoyproxy#4108) bazel: use GCS remote cache (envoyproxy#4050) Add thread local cache of overload action states (envoyproxy#4090) Added TCP healthcheck capabilities to the HdsDelegate (envoyproxy#4079) secret: add secret provider interface and use it for TlsCertificates (envoyproxy#4086) ... Signed-off-by: Snow Pettersen <snowp@squareup.com>
2 participants
Description:
This patch sets the content-type and content-length of HTTP subscription
requests.
Risk Level: Low
Testing: Added
Docs Changes: N/A
Release Notes: N/A
Fixes #4112
Signed-off-by: Dhi Aurrahman dio@tetrate.io