From 3a0c2ca9b2055740d5ca1f75f110baaa45056ad3 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Sat, 11 Aug 2018 16:09:00 +0700 Subject: [PATCH 1/3] Set content-type and content-length This patch sets the content-type and content-length of HTTP subscription requests. Signed-off-by: Dhi Aurrahman --- source/common/config/http_subscription_impl.h | 3 +++ .../config/http_subscription_test_harness.h | 4 ++++ test/common/upstream/cds_api_impl_test.cc | 17 +++++++++++------ test/server/lds_api_test.cc | 17 +++++++++++------ 4 files changed, 29 insertions(+), 12 deletions(-) diff --git a/source/common/config/http_subscription_impl.h b/source/common/config/http_subscription_impl.h index b0d0ac94f0597..b084c9fa71196 100644 --- a/source/common/config/http_subscription_impl.h +++ b/source/common/config/http_subscription_impl.h @@ -69,6 +69,9 @@ class HttpSubscriptionImpl : public Http::RestApiFetcher, request.headers().insertMethod().value().setReference(Http::Headers::get().MethodValues.Post); request.headers().insertPath().value(path_); request.body().reset(new Buffer::OwnedImpl(MessageUtil::getJsonStringFromMessage(request_))); + request.headers().insertContentType().value().setReference( + Http::Headers::get().ContentTypeValues.Json); + request.headers().insertContentLength().value(request.body()->length()); } void parseResponse(const Http::Message& response) override { diff --git a/test/common/config/http_subscription_test_harness.h b/test/common/config/http_subscription_test_harness.h index 0853b50c8c7db..180b7647bf131 100644 --- a/test/common/config/http_subscription_test_harness.h +++ b/test/common/config/http_subscription_test_harness.h @@ -59,6 +59,8 @@ class HttpSubscriptionTestHarness : public SubscriptionTestHarness { http_callbacks_ = &callbacks; UNREFERENCED_PARAMETER(timeout); EXPECT_EQ("POST", std::string(request->headers().Method()->value().c_str())); + EXPECT_EQ(Http::Headers::get().ContentTypeValues.Json, + std::string(request->headers().ContentType()->value().c_str())); EXPECT_EQ("eds_cluster", std::string(request->headers().Host()->value().c_str())); EXPECT_EQ("/v2/discovery:endpoints", std::string(request->headers().Path()->value().c_str())); @@ -73,6 +75,8 @@ class HttpSubscriptionTestHarness : public SubscriptionTestHarness { } expected_request += "}"; EXPECT_EQ(expected_request, request->bodyAsString()); + EXPECT_EQ(fmt::FormatInt(expected_request.size()).str(), + std::string(request->headers().ContentLength()->value().c_str())); request_in_progress_ = true; return &http_request_; })); diff --git a/test/common/upstream/cds_api_impl_test.cc b/test/common/upstream/cds_api_impl_test.cc index f99897acf3bc8..7b9e536a22733 100644 --- a/test/common/upstream/cds_api_impl_test.cc +++ b/test/common/upstream/cds_api_impl_test.cc @@ -77,12 +77,17 @@ class CdsApiImplTest : public testing::Test { .WillOnce(Invoke( [&](Http::MessagePtr& request, Http::AsyncClient::Callbacks& callbacks, const absl::optional&) -> Http::AsyncClient::Request* { - EXPECT_EQ((Http::TestHeaderMapImpl{ - {":method", v2_rest_ ? "POST" : "GET"}, - {":path", v2_rest_ ? "/v2/discovery:clusters" - : "/v1/clusters/cluster_name/node_name"}, - {":authority", "foo_cluster"}}), - request->headers()); + Http::TestHeaderMapImpl expected_headers{ + {":method", v2_rest_ ? "POST" : "GET"}, + {":path", + v2_rest_ ? "/v2/discovery:clusters" : "/v1/clusters/cluster_name/node_name"}, + {":authority", "foo_cluster"}}; + if (v2_rest_) { + expected_headers.addCopy("content-type", "application/json"); + expected_headers.addCopy("content-length", + fmt::FormatInt(request->body()->length()).str()); + } + EXPECT_EQ(expected_headers, request->headers()); callbacks_ = &callbacks; return &request_; })); diff --git a/test/server/lds_api_test.cc b/test/server/lds_api_test.cc index dec19cc34908c..a9c65b12cd4f1 100644 --- a/test/server/lds_api_test.cc +++ b/test/server/lds_api_test.cc @@ -82,12 +82,17 @@ class LdsApiTest : public testing::Test { .WillOnce(Invoke( [&](Http::MessagePtr& request, Http::AsyncClient::Callbacks& callbacks, const absl::optional&) -> Http::AsyncClient::Request* { - EXPECT_EQ((Http::TestHeaderMapImpl{ - {":method", v2_rest_ ? "POST" : "GET"}, - {":path", v2_rest_ ? "/v2/discovery:listeners" - : "/v1/listeners/cluster_name/node_name"}, - {":authority", "foo_cluster"}}), - request->headers()); + Http::TestHeaderMapImpl expected_headers{ + {":method", v2_rest_ ? "POST" : "GET"}, + {":path", + v2_rest_ ? "/v2/discovery:listeners" : "/v1/listeners/cluster_name/node_name"}, + {":authority", "foo_cluster"}}; + if (v2_rest_) { + expected_headers.addCopy("content-type", "application/json"); + expected_headers.addCopy("content-length", + fmt::FormatInt(request->body()->length()).str()); + } + EXPECT_EQ(expected_headers, request->headers()); callbacks_ = &callbacks; return &request_; })); From f9a74712ee4459161c62fcbe808603d705320bf8 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Mon, 13 Aug 2018 08:57:49 +0700 Subject: [PATCH 2/3] review: update v1 Signed-off-by: Dhi Aurrahman --- source/common/upstream/cds_subscription.cc | 5 +++++ source/common/upstream/sds_subscription.cc | 5 +++++ source/server/lds_subscription.cc | 5 +++++ test/common/upstream/cds_api_impl_test.cc | 20 +++++++++----------- test/server/lds_api_test.cc | 20 +++++++++----------- 5 files changed, 33 insertions(+), 22 deletions(-) diff --git a/source/common/upstream/cds_subscription.cc b/source/common/upstream/cds_subscription.cc index 7e975b833ecd7..d1c1187253288 100644 --- a/source/common/upstream/cds_subscription.cc +++ b/source/common/upstream/cds_subscription.cc @@ -27,6 +27,11 @@ void CdsSubscription::createRequest(Http::Message& request) { request.headers().insertMethod().value().setReference(Http::Headers::get().MethodValues.Get); request.headers().insertPath().value( fmt::format("/v1/clusters/{}/{}", local_info_.clusterName(), local_info_.nodeName())); + request.headers().insertContentType().value().setReference( + Http::Headers::get().ContentTypeValues.Json); + + const size_t empty_body_size = 0; + request.headers().insertContentLength().value(empty_body_size); } void CdsSubscription::parseResponse(const Http::Message& response) { diff --git a/source/common/upstream/sds_subscription.cc b/source/common/upstream/sds_subscription.cc index 788c431441465..97799e34d8bc5 100644 --- a/source/common/upstream/sds_subscription.cc +++ b/source/common/upstream/sds_subscription.cc @@ -85,6 +85,11 @@ void SdsSubscription::createRequest(Http::Message& message) { message.headers().insertMethod().value().setReference(Http::Headers::get().MethodValues.Get); message.headers().insertPath().value("/v1/registration/" + cluster_name_); + message.headers().insertContentType().value().setReference( + Http::Headers::get().ContentTypeValues.Json); + + const size_t empty_body_size = 0; + message.headers().insertContentLength().value(empty_body_size); } void SdsSubscription::onFetchComplete() { diff --git a/source/server/lds_subscription.cc b/source/server/lds_subscription.cc index 713e6b751a789..2082fa43e09b9 100644 --- a/source/server/lds_subscription.cc +++ b/source/server/lds_subscription.cc @@ -30,6 +30,11 @@ void LdsSubscription::createRequest(Http::Message& request) { request.headers().insertMethod().value().setReference(Http::Headers::get().MethodValues.Get); request.headers().insertPath().value( fmt::format("/v1/listeners/{}/{}", local_info_.clusterName(), local_info_.nodeName())); + request.headers().insertContentType().value().setReference( + Http::Headers::get().ContentTypeValues.Json); + + const size_t empty_body_size = 0; + request.headers().insertContentLength().value(empty_body_size); } void LdsSubscription::parseResponse(const Http::Message& response) { diff --git a/test/common/upstream/cds_api_impl_test.cc b/test/common/upstream/cds_api_impl_test.cc index 7b9e536a22733..f40c7635d29e1 100644 --- a/test/common/upstream/cds_api_impl_test.cc +++ b/test/common/upstream/cds_api_impl_test.cc @@ -77,17 +77,15 @@ class CdsApiImplTest : public testing::Test { .WillOnce(Invoke( [&](Http::MessagePtr& request, Http::AsyncClient::Callbacks& callbacks, const absl::optional&) -> Http::AsyncClient::Request* { - Http::TestHeaderMapImpl expected_headers{ - {":method", v2_rest_ ? "POST" : "GET"}, - {":path", - v2_rest_ ? "/v2/discovery:clusters" : "/v1/clusters/cluster_name/node_name"}, - {":authority", "foo_cluster"}}; - if (v2_rest_) { - expected_headers.addCopy("content-type", "application/json"); - expected_headers.addCopy("content-length", - fmt::FormatInt(request->body()->length()).str()); - } - EXPECT_EQ(expected_headers, request->headers()); + EXPECT_EQ((Http::TestHeaderMapImpl{ + {":method", v2_rest_ ? "POST" : "GET"}, + {":path", v2_rest_ ? "/v2/discovery:clusters" + : "/v1/clusters/cluster_name/node_name"}, + {":authority", "foo_cluster"}, + {"content-type", "application/json"}, + {"content-length", + v2_rest_ ? fmt::FormatInt(request->body()->length()).str() : "0"}}), + request->headers()); callbacks_ = &callbacks; return &request_; })); diff --git a/test/server/lds_api_test.cc b/test/server/lds_api_test.cc index a9c65b12cd4f1..8a521f156341b 100644 --- a/test/server/lds_api_test.cc +++ b/test/server/lds_api_test.cc @@ -82,17 +82,15 @@ class LdsApiTest : public testing::Test { .WillOnce(Invoke( [&](Http::MessagePtr& request, Http::AsyncClient::Callbacks& callbacks, const absl::optional&) -> Http::AsyncClient::Request* { - Http::TestHeaderMapImpl expected_headers{ - {":method", v2_rest_ ? "POST" : "GET"}, - {":path", - v2_rest_ ? "/v2/discovery:listeners" : "/v1/listeners/cluster_name/node_name"}, - {":authority", "foo_cluster"}}; - if (v2_rest_) { - expected_headers.addCopy("content-type", "application/json"); - expected_headers.addCopy("content-length", - fmt::FormatInt(request->body()->length()).str()); - } - EXPECT_EQ(expected_headers, request->headers()); + EXPECT_EQ((Http::TestHeaderMapImpl{ + {":method", v2_rest_ ? "POST" : "GET"}, + {":path", v2_rest_ ? "/v2/discovery:listeners" + : "/v1/listeners/cluster_name/node_name"}, + {":authority", "foo_cluster"}, + {"content-type", "application/json"}, + {"content-length", + v2_rest_ ? fmt::FormatInt(request->body()->length()).str() : "0"}}), + request->headers()); callbacks_ = &callbacks; return &request_; })); From 852bf2df78e019b8d0be40fa6dd00377da251989 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Tue, 14 Aug 2018 05:21:49 +0700 Subject: [PATCH 3/3] review: remove temp var and check body Signed-off-by: Dhi Aurrahman --- source/common/upstream/cds_subscription.cc | 4 +--- source/common/upstream/sds_subscription.cc | 4 +--- source/server/lds_subscription.cc | 4 +--- test/common/upstream/cds_api_impl_test.cc | 19 ++++++++++--------- test/server/lds_api_test.cc | 19 ++++++++++--------- 5 files changed, 23 insertions(+), 27 deletions(-) diff --git a/source/common/upstream/cds_subscription.cc b/source/common/upstream/cds_subscription.cc index d1c1187253288..b0398d818e45c 100644 --- a/source/common/upstream/cds_subscription.cc +++ b/source/common/upstream/cds_subscription.cc @@ -29,9 +29,7 @@ void CdsSubscription::createRequest(Http::Message& request) { fmt::format("/v1/clusters/{}/{}", local_info_.clusterName(), local_info_.nodeName())); request.headers().insertContentType().value().setReference( Http::Headers::get().ContentTypeValues.Json); - - const size_t empty_body_size = 0; - request.headers().insertContentLength().value(empty_body_size); + request.headers().insertContentLength().value(size_t(0)); } void CdsSubscription::parseResponse(const Http::Message& response) { diff --git a/source/common/upstream/sds_subscription.cc b/source/common/upstream/sds_subscription.cc index 97799e34d8bc5..4e5f6b1682d4d 100644 --- a/source/common/upstream/sds_subscription.cc +++ b/source/common/upstream/sds_subscription.cc @@ -87,9 +87,7 @@ void SdsSubscription::createRequest(Http::Message& message) { message.headers().insertPath().value("/v1/registration/" + cluster_name_); message.headers().insertContentType().value().setReference( Http::Headers::get().ContentTypeValues.Json); - - const size_t empty_body_size = 0; - message.headers().insertContentLength().value(empty_body_size); + message.headers().insertContentLength().value(size_t(0)); } void SdsSubscription::onFetchComplete() { diff --git a/source/server/lds_subscription.cc b/source/server/lds_subscription.cc index 2082fa43e09b9..86858f48d2739 100644 --- a/source/server/lds_subscription.cc +++ b/source/server/lds_subscription.cc @@ -32,9 +32,7 @@ void LdsSubscription::createRequest(Http::Message& request) { fmt::format("/v1/listeners/{}/{}", local_info_.clusterName(), local_info_.nodeName())); request.headers().insertContentType().value().setReference( Http::Headers::get().ContentTypeValues.Json); - - const size_t empty_body_size = 0; - request.headers().insertContentLength().value(empty_body_size); + request.headers().insertContentLength().value(size_t(0)); } void LdsSubscription::parseResponse(const Http::Message& response) { diff --git a/test/common/upstream/cds_api_impl_test.cc b/test/common/upstream/cds_api_impl_test.cc index f40c7635d29e1..574935c6ef263 100644 --- a/test/common/upstream/cds_api_impl_test.cc +++ b/test/common/upstream/cds_api_impl_test.cc @@ -77,15 +77,16 @@ class CdsApiImplTest : public testing::Test { .WillOnce(Invoke( [&](Http::MessagePtr& request, Http::AsyncClient::Callbacks& callbacks, const absl::optional&) -> Http::AsyncClient::Request* { - EXPECT_EQ((Http::TestHeaderMapImpl{ - {":method", v2_rest_ ? "POST" : "GET"}, - {":path", v2_rest_ ? "/v2/discovery:clusters" - : "/v1/clusters/cluster_name/node_name"}, - {":authority", "foo_cluster"}, - {"content-type", "application/json"}, - {"content-length", - v2_rest_ ? fmt::FormatInt(request->body()->length()).str() : "0"}}), - request->headers()); + EXPECT_EQ( + (Http::TestHeaderMapImpl{ + {":method", v2_rest_ ? "POST" : "GET"}, + {":path", + v2_rest_ ? "/v2/discovery:clusters" : "/v1/clusters/cluster_name/node_name"}, + {":authority", "foo_cluster"}, + {"content-type", "application/json"}, + {"content-length", + request->body() ? fmt::FormatInt(request->body()->length()).str() : "0"}}), + request->headers()); callbacks_ = &callbacks; return &request_; })); diff --git a/test/server/lds_api_test.cc b/test/server/lds_api_test.cc index 8a521f156341b..906cfd920d220 100644 --- a/test/server/lds_api_test.cc +++ b/test/server/lds_api_test.cc @@ -82,15 +82,16 @@ class LdsApiTest : public testing::Test { .WillOnce(Invoke( [&](Http::MessagePtr& request, Http::AsyncClient::Callbacks& callbacks, const absl::optional&) -> Http::AsyncClient::Request* { - EXPECT_EQ((Http::TestHeaderMapImpl{ - {":method", v2_rest_ ? "POST" : "GET"}, - {":path", v2_rest_ ? "/v2/discovery:listeners" - : "/v1/listeners/cluster_name/node_name"}, - {":authority", "foo_cluster"}, - {"content-type", "application/json"}, - {"content-length", - v2_rest_ ? fmt::FormatInt(request->body()->length()).str() : "0"}}), - request->headers()); + EXPECT_EQ( + (Http::TestHeaderMapImpl{ + {":method", v2_rest_ ? "POST" : "GET"}, + {":path", v2_rest_ ? "/v2/discovery:listeners" + : "/v1/listeners/cluster_name/node_name"}, + {":authority", "foo_cluster"}, + {"content-type", "application/json"}, + {"content-length", + request->body() ? fmt::FormatInt(request->body()->length()).str() : "0"}}), + request->headers()); callbacks_ = &callbacks; return &request_; }));