diff --git a/docs/root/operations/admin.rst b/docs/root/operations/admin.rst index d954edfe6ad5c..38cc64f7fe182 100644 --- a/docs/root/operations/admin.rst +++ b/docs/root/operations/admin.rst @@ -32,8 +32,8 @@ modify different aspects of the server: In the future additional security options will be added to the administration interface. This work is tracked in `this `_ issue. - All mutations should be sent as HTTP POST operations. For a limited time, they will continue - to work with HTTP GET, with a warning logged. + All mutations must be sent as HTTP POST operations. When a mutation is requested via GET, + the request has no effect, and an HTTP 400 (Invalid Request) response is returned. .. http:get:: / @@ -354,4 +354,4 @@ The fields are: set to '0'. * Latency information is currently unavailable. - \ No newline at end of file + diff --git a/source/server/http/admin.cc b/source/server/http/admin.cc index 81ef4b77ad0ab..267e557ca0639 100644 --- a/source/server/http/admin.cc +++ b/source/server/http/admin.cc @@ -953,16 +953,19 @@ 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) { + found_handler = true; if (handler.mutates_server_state_) { const absl::string_view method = admin_stream.getRequestHeaders().Method()->value().getStringView(); if (method != Http::Headers::get().MethodValues.Post) { - ENVOY_LOG(warn, "admin path \"{}\" mutates state, method={} rather than POST", + ENVOY_LOG(error, "admin path \"{}\" mutates state, method={} rather than POST", handler.prefix_, method); + code = Http::Code::BadRequest; + response.add("Invalid request; POST required"); + break; } } code = handler.handler_(path_and_query, response_headers, response, admin_stream); - found_handler = true; break; } } diff --git a/test/integration/integration_admin_test.cc b/test/integration/integration_admin_test.cc index 9ebd2b9e53877..0376aa9190acf 100644 --- a/test/integration/integration_admin_test.cc +++ b/test/integration/integration_admin_test.cc @@ -21,12 +21,12 @@ TEST_P(IntegrationAdminTest, HealthCheck) { initialize(); BufferingStreamDecoderPtr response = IntegrationUtil::makeSingleRequest( - lookupPort("http"), "GET", "/healthcheck", "", downstreamProtocol(), version_); + lookupPort("http"), "POST", "/healthcheck", "", downstreamProtocol(), version_); EXPECT_TRUE(response->complete()); EXPECT_STREQ("200", response->headers().Status()->value().c_str()); - response = IntegrationUtil::makeSingleRequest(lookupPort("admin"), "GET", "/healthcheck/fail", "", - downstreamProtocol(), version_); + response = IntegrationUtil::makeSingleRequest(lookupPort("admin"), "POST", "/healthcheck/fail", + "", downstreamProtocol(), version_); EXPECT_TRUE(response->complete()); EXPECT_STREQ("200", response->headers().Status()->value().c_str()); @@ -35,7 +35,7 @@ TEST_P(IntegrationAdminTest, HealthCheck) { EXPECT_TRUE(response->complete()); EXPECT_STREQ("503", response->headers().Status()->value().c_str()); - response = IntegrationUtil::makeSingleRequest(lookupPort("admin"), "GET", "/healthcheck/ok", "", + response = IntegrationUtil::makeSingleRequest(lookupPort("admin"), "POST", "/healthcheck/ok", "", downstreamProtocol(), version_); EXPECT_TRUE(response->complete()); EXPECT_STREQ("200", response->headers().Status()->value().c_str()); @@ -60,39 +60,39 @@ TEST_P(IntegrationAdminTest, AdminLogging) { initialize(); BufferingStreamDecoderPtr response = IntegrationUtil::makeSingleRequest( - lookupPort("admin"), "GET", "/logging", "", downstreamProtocol(), version_); + lookupPort("admin"), "POST", "/logging", "", downstreamProtocol(), version_); EXPECT_TRUE(response->complete()); EXPECT_STREQ("404", response->headers().Status()->value().c_str()); // Bad level - response = IntegrationUtil::makeSingleRequest(lookupPort("admin"), "GET", "/logging?level=blah", + response = IntegrationUtil::makeSingleRequest(lookupPort("admin"), "POST", "/logging?level=blah", "", downstreamProtocol(), version_); EXPECT_TRUE(response->complete()); EXPECT_STREQ("404", response->headers().Status()->value().c_str()); // Bad logger - response = IntegrationUtil::makeSingleRequest(lookupPort("admin"), "GET", "/logging?blah=info", + response = IntegrationUtil::makeSingleRequest(lookupPort("admin"), "POST", "/logging?blah=info", "", downstreamProtocol(), version_); EXPECT_TRUE(response->complete()); EXPECT_STREQ("404", response->headers().Status()->value().c_str()); // This is going to stomp over custom log levels that are set on the command line. response = IntegrationUtil::makeSingleRequest( - lookupPort("admin"), "GET", "/logging?level=warning", "", downstreamProtocol(), version_); + lookupPort("admin"), "POST", "/logging?level=warning", "", downstreamProtocol(), version_); EXPECT_TRUE(response->complete()); EXPECT_STREQ("200", response->headers().Status()->value().c_str()); for (const Logger::Logger& logger : Logger::Registry::loggers()) { EXPECT_EQ("warning", logger.levelString()); } - response = IntegrationUtil::makeSingleRequest(lookupPort("admin"), "GET", "/logging?assert=trace", - "", downstreamProtocol(), version_); + response = IntegrationUtil::makeSingleRequest( + lookupPort("admin"), "POST", "/logging?assert=trace", "", downstreamProtocol(), version_); EXPECT_TRUE(response->complete()); EXPECT_STREQ("200", response->headers().Status()->value().c_str()); EXPECT_EQ(spdlog::level::trace, Logger::Registry::getLog(Logger::Id::assert).level()); const char* level_name = spdlog::level::level_names[default_log_level_]; - response = IntegrationUtil::makeSingleRequest(lookupPort("admin"), "GET", + response = IntegrationUtil::makeSingleRequest(lookupPort("admin"), "POST", fmt::format("/logging?level={}", level_name), "", downstreamProtocol(), version_); EXPECT_TRUE(response->complete()); @@ -224,7 +224,7 @@ TEST_P(IntegrationAdminTest, Admin) { EXPECT_THAT(response->body(), testing::HasSubstr("added_via_api")); EXPECT_STREQ("text/plain; charset=UTF-8", ContentType(response)); - response = IntegrationUtil::makeSingleRequest(lookupPort("admin"), "GET", "/cpuprofiler", "", + response = IntegrationUtil::makeSingleRequest(lookupPort("admin"), "POST", "/cpuprofiler", "", downstreamProtocol(), version_); EXPECT_TRUE(response->complete()); EXPECT_STREQ("400", response->headers().Status()->value().c_str()); @@ -236,7 +236,7 @@ TEST_P(IntegrationAdminTest, Admin) { EXPECT_STREQ("200", response->headers().Status()->value().c_str()); EXPECT_STREQ("text/plain; charset=UTF-8", ContentType(response)); - response = IntegrationUtil::makeSingleRequest(lookupPort("admin"), "GET", "/reset_counters", "", + response = IntegrationUtil::makeSingleRequest(lookupPort("admin"), "POST", "/reset_counters", "", downstreamProtocol(), version_); EXPECT_TRUE(response->complete()); EXPECT_STREQ("200", response->headers().Status()->value().c_str()); @@ -341,12 +341,12 @@ TEST_P(IntegrationAdminTest, AdminCpuProfilerStart) { initialize(); BufferingStreamDecoderPtr response = IntegrationUtil::makeSingleRequest( - lookupPort("admin"), "GET", "/cpuprofiler?enable=y", "", downstreamProtocol(), version_); + lookupPort("admin"), "POST", "/cpuprofiler?enable=y", "", downstreamProtocol(), version_); EXPECT_TRUE(response->complete()); EXPECT_STREQ("200", response->headers().Status()->value().c_str()); - response = IntegrationUtil::makeSingleRequest(lookupPort("admin"), "GET", "/cpuprofiler?enable=n", - "", downstreamProtocol(), version_); + response = IntegrationUtil::makeSingleRequest( + lookupPort("admin"), "POST", "/cpuprofiler?enable=n", "", downstreamProtocol(), version_); EXPECT_TRUE(response->complete()); EXPECT_STREQ("200", response->headers().Status()->value().c_str()); } diff --git a/test/integration/server.cc b/test/integration/server.cc index 642dd6e0257f1..38e33b148c976 100644 --- a/test/integration/server.cc +++ b/test/integration/server.cc @@ -59,7 +59,7 @@ IntegrationTestServer::~IntegrationTestServer() { ENVOY_LOG(info, "stopping integration test server"); BufferingStreamDecoderPtr response = - IntegrationUtil::makeSingleRequest(server_->admin().socket().localAddress(), "GET", + IntegrationUtil::makeSingleRequest(server_->admin().socket().localAddress(), "POST", "/quitquitquit", "", Http::CodecClient::Type::HTTP1); EXPECT_TRUE(response->complete()); EXPECT_STREQ("200", response->headers().Status()->value().c_str()); diff --git a/test/server/http/admin_test.cc b/test/server/http/admin_test.cc index 546724e98ed4f..7fd74e7ea69ee 100644 --- a/test/server/http/admin_test.cc +++ b/test/server/http/admin_test.cc @@ -417,15 +417,15 @@ TEST_P(AdminInstanceTest, AdminProfiler) { #endif -TEST_P(AdminInstanceTest, MutatesWarnWithGet) { +TEST_P(AdminInstanceTest, MutatesErrorWithGet) { Buffer::OwnedImpl data; Http::HeaderMapImpl header_map; const std::string path("/healthcheck/fail"); // TODO(jmarantz): the call to getCallback should be made to fail, but as an interim we will // just issue a warning, so that scripts using curl GET comamnds to mutate state can be fixed. - EXPECT_LOG_CONTAINS("warning", + EXPECT_LOG_CONTAINS("error", "admin path \"" + path + "\" mutates state, method=GET rather than POST", - EXPECT_EQ(Http::Code::OK, getCallback(path, header_map, data))); + EXPECT_EQ(Http::Code::BadRequest, getCallback(path, header_map, data))); } TEST_P(AdminInstanceTest, AdminBadProfiler) { @@ -638,7 +638,7 @@ TEST_P(AdminInstanceTest, RuntimeModify) { overrides["nothing"] = ""; EXPECT_CALL(loader, mergeValues(overrides)).Times(1); EXPECT_EQ(Http::Code::OK, - getCallback("/runtime_modify?foo=bar&x=42¬hing=", header_map, response)); + postCallback("/runtime_modify?foo=bar&x=42¬hing=", header_map, response)); EXPECT_EQ("OK\n", response.toString()); } @@ -646,7 +646,7 @@ TEST_P(AdminInstanceTest, RuntimeModifyNoArguments) { Http::HeaderMapImpl header_map; Buffer::OwnedImpl response; - EXPECT_EQ(Http::Code::BadRequest, getCallback("/runtime_modify", header_map, response)); + EXPECT_EQ(Http::Code::BadRequest, postCallback("/runtime_modify", header_map, response)); EXPECT_TRUE(absl::StartsWith(response.toString(), "usage:")); }