HTTP: Use channel arg instead of global to allow PUT requests (#29411)

* HTTP: Use channel arg instead of global to allow PUT requests

* Reviewer comments

* Reviewer comments

* clang-format

* Repo manager - AJ

* Reviewer comments

* unused parameter

BUILD

@@ -2743,6 +2743,7 @@ grpc_cc_library(
         "absl/types:optional",
     ],
     language = "c++",
+    visibility = ["@grpc:http"],
     deps = [
         "call_push_pull",
         "config",

bazel/grpc_build_system.bzl

@@ -95,6 +95,7 @@ def _update_visibility(visibility):
         "grpc_opencensus_plugin": PUBLIC,
         "grpc_resolver_fake": PRIVATE,
         "grpc++_test": PRIVATE,
+        "http": PRIVATE,
         "httpcli": PRIVATE,
         "public": PUBLIC,
         "ref_counted_ptr": PRIVATE,

src/core/ext/filters/http/server/http_server_filter.cc

@@ -40,8 +40,6 @@ static void hs_recv_trailing_metadata_ready(void* user_data,
 
 namespace {
 
-bool g_allow_put_requests;
-
 struct call_data {
   call_data(grpc_call_element* elem, const grpc_call_element_args& args)
       : call_combiner(args.call_combiner) {
@@ -74,18 +72,11 @@ struct call_data {
 
 struct channel_data {
   bool surface_user_agent;
+  bool allow_put_requests;
 };
 
 }  // namespace
 
-namespace grpc_core {
-
-void InternalOnlyDoNotUseUnlessYouHavePermissionFromGrpcTeamAllowBrokenPutRequests() {
-  g_allow_put_requests = true;
-}
-
-}  // namespace grpc_core
-
 static grpc_error_handle hs_filter_outgoing_metadata(grpc_metadata_batch* b) {
   if (grpc_core::Slice* grpc_message =
           b->get_pointer(grpc_core::GrpcMessageMetadata())) {
@@ -115,7 +106,10 @@ static grpc_error_handle hs_filter_incoming_metadata(grpc_call_element* elem,
       case grpc_core::HttpMethodMetadata::kPost:
         break;
       case grpc_core::HttpMethodMetadata::kPut:
-        if (g_allow_put_requests) break;
+        if (static_cast<channel_data*>(elem->channel_data)
+                ->allow_put_requests) {
+          break;
+        }
         ABSL_FALLTHROUGH_INTENDED;
       case grpc_core::HttpMethodMetadata::kInvalid:
       case grpc_core::HttpMethodMetadata::kGet:
@@ -314,6 +308,10 @@ static grpc_error_handle hs_init_channel_elem(grpc_channel_element* elem,
       grpc_channel_args_find(args->channel_args,
                              const_cast<char*>(GRPC_ARG_SURFACE_USER_AGENT)),
       true);
+  chand->allow_put_requests = grpc_channel_args_find_bool(
+      args->channel_args,
+      GRPC_ARG_INTERNAL_ONLY_DO_NOT_USE_UNLESS_YOU_HAVE_PERMISSION_FROM_GRPC_TEAM_ALLOW_BROKEN_PUT_REQUESTS,
+      false);
   return GRPC_ERROR_NONE;
 }
 

src/core/ext/filters/http/server/http_server_filter.h

@@ -26,10 +26,10 @@
 /* Processes metadata on the server side for HTTP2 transports */
 extern const grpc_channel_filter grpc_http_server_filter;
 
-namespace grpc_core {
+// A Temporary channel arg that allows servers to accept PUT requests. DO NOT
-// Temporary code that allows servers to accept PUT requests. DO NOT USE WITHOUT
+// USE WITHOUT PERMISSION.
-// PERMISSION.
+#define GRPC_ARG_INTERNAL_ONLY_DO_NOT_USE_UNLESS_YOU_HAVE_PERMISSION_FROM_GRPC_TEAM_ALLOW_BROKEN_PUT_REQUESTS \
-void InternalOnlyDoNotUseUnlessYouHavePermissionFromGrpcTeamAllowBrokenPutRequests();
+  "grpc.http.internal_only_do_not_use_unless_you_have_permission_from_grpc_"                                  \
-}  // namespace grpc_core
+  "team_allow_broken_put_requests"
 
 #endif /* GRPC_CORE_EXT_FILTERS_HTTP_SERVER_HTTP_SERVER_FILTER_H */

test/cpp/end2end/xds/xds_end2end_test.cc

@@ -60,7 +60,6 @@
 #include "src/core/ext/filters/client_channel/lb_policy/xds/xds_channel_args.h"
 #include "src/core/ext/filters/client_channel/resolver/fake/fake_resolver.h"
 #include "src/core/ext/filters/http/client/http_client_filter.h"
-#include "src/core/ext/filters/http/server/http_server_filter.h"
 #include "src/core/ext/xds/certificate_provider_registry.h"
 #include "src/core/ext/xds/xds_api.h"
 #include "src/core/ext/xds/xds_channel_args.h"
@@ -3137,6 +3136,7 @@ TEST_P(XdsRbacTestWithActionPermutations, MethodPostPermissionAnyPrincipal) {
   policy.add_principals()->set_any(true);
   (*rules->mutable_policies())["policy"] = policy;
   SetServerRbacPolicy(rbac);
+  backends_[0]->set_allow_put_requests(true);
   backends_[0]->Start();
   backends_[0]->notifier()->WaitOnServingStatusChange(
       absl::StrCat(ipv6_only_ ? "[::1]:" : "127.0.0.1:", backends_[0]->port()),
@@ -3187,6 +3187,7 @@ TEST_P(XdsRbacTestWithActionPermutations, MethodPutPermissionAnyPrincipal) {
   policy.add_principals()->set_any(true);
   (*rules->mutable_policies())["policy"] = policy;
   SetServerRbacPolicy(rbac);
+  backends_[0]->set_allow_put_requests(true);
   backends_[0]->Start();
   backends_[0]->notifier()->WaitOnServingStatusChange(
       absl::StrCat(ipv6_only_ ? "[::1]:" : "127.0.0.1:", backends_[0]->port()),
@@ -3448,6 +3449,7 @@ TEST_P(XdsRbacTestWithActionPermutations, AnyPermissionMethodPostPrincipal) {
   policy.add_permissions()->set_any(true);
   (*rules->mutable_policies())["policy"] = policy;
   SetServerRbacPolicy(rbac);
+  backends_[0]->set_allow_put_requests(true);
   backends_[0]->Start();
   backends_[0]->notifier()->WaitOnServingStatusChange(
       absl::StrCat(ipv6_only_ ? "[::1]:" : "127.0.0.1:", backends_[0]->port()),
@@ -3498,6 +3500,7 @@ TEST_P(XdsRbacTestWithActionPermutations, AnyPermissionMethodPutPrincipal) {
   policy.add_permissions()->set_any(true);
   (*rules->mutable_policies())["policy"] = policy;
   SetServerRbacPolicy(rbac);
+  backends_[0]->set_allow_put_requests(true);
   backends_[0]->Start();
   backends_[0]->notifier()->WaitOnServingStatusChange(
       absl::StrCat(ipv6_only_ ? "[::1]:" : "127.0.0.1:", backends_[0]->port()),
@@ -3956,9 +3959,6 @@ int main(int argc, char** argv) {
   // Make the backup poller poll very frequently in order to pick up
   // updates from all the subchannels's FDs.
   GPR_GLOBAL_CONFIG_SET(grpc_client_channel_backup_poll_interval_ms, 1);
-  // Allow testing PUT requests.
-  grpc_core::
-      InternalOnlyDoNotUseUnlessYouHavePermissionFromGrpcTeamAllowBrokenPutRequests();
 #if TARGET_OS_IPHONE
   // Workaround Apple CFStream bug
   gpr_setenv("grpc_cfstream", "0");

test/cpp/end2end/xds/xds_end2end_test_lib.cc

@@ -36,6 +36,7 @@
 
 #include <grpcpp/security/tls_certificate_provider.h>
 
+#include "src/core/ext/filters/http/server/http_server_filter.h"
 #include "src/core/ext/xds/xds_channel_args.h"
 #include "src/core/ext/xds/xds_client.h"
 #include "src/core/lib/gpr/env.h"
@@ -175,6 +176,13 @@ void XdsEnd2endTest::ServerThread::Serve(grpc_core::Mutex* mu,
     builder.experimental().set_drain_grace_time(
         test_obj_->xds_drain_grace_time_ms_);
     builder.AddListeningPort(server_address, Credentials());
+    // Allow gRPC Core's HTTP server to accept PUT requests for testing
+    // purposes.
+    if (allow_put_requests_) {
+      builder.AddChannelArgument(
+          GRPC_ARG_INTERNAL_ONLY_DO_NOT_USE_UNLESS_YOU_HAVE_PERMISSION_FROM_GRPC_TEAM_ALLOW_BROKEN_PUT_REQUESTS,
+          true);
+    }
     RegisterAllServices(&builder);
     server_ = builder.BuildAndStart();
   } else {

test/cpp/end2end/xds/xds_end2end_test_lib.h

@@ -253,6 +253,10 @@ class XdsEnd2endTest : public ::testing::TestWithParam<XdsTestType> {
 
     XdsServingStatusNotifier* notifier() { return &notifier_; }
 
+    void set_allow_put_requests(bool allow_put_requests) {
+      allow_put_requests_ = allow_put_requests;
+    }
+
    private:
     class XdsChannelArgsServerBuilderOption;
 
@@ -270,6 +274,7 @@ class XdsEnd2endTest : public ::testing::TestWithParam<XdsTestType> {
     std::unique_ptr<std::thread> thread_;
     bool running_ = false;
     const bool use_xds_enabled_server_;
+    bool allow_put_requests_;
   };
 
   // A server thread for a backend server.