From 7d7580b3179a5cb94a332863e00401bf1b836d08 Mon Sep 17 00:00:00 2001 From: Wayne Zhang Date: Tue, 2 Jun 2020 14:04:40 -0700 Subject: [PATCH] Fix grpc-web: move grpc-web filter in front of transcoder (#176) Signed-off-by: Wayne Zhang --- .../grpc_dynamic_routing/envoy_config.json | 8 ++++---- src/go/configgenerator/listener_generator.go | 15 +++++++++----- src/go/configmanager/config_manager_test.go | 6 +++--- .../bookstore_grpc/proto/api_descriptor.pb | Bin 11180 -> 11353 bytes .../bookstore_grpc/proto/v1/bookstore.proto | 19 ++++++++++++++++++ 5 files changed, 36 insertions(+), 12 deletions(-) diff --git a/examples/grpc_dynamic_routing/envoy_config.json b/examples/grpc_dynamic_routing/envoy_config.json index 21936b212..f34ba1922 100644 --- a/examples/grpc_dynamic_routing/envoy_config.json +++ b/examples/grpc_dynamic_routing/envoy_config.json @@ -1036,6 +1036,9 @@ ] } }, + { + "name": "envoy.filters.http.grpc_web" + }, { "name": "envoy.filters.http.grpc_json_transcoder", "typedConfig": { @@ -1054,9 +1057,6 @@ ] } }, - { - "name": "envoy.filters.http.grpc_web" - }, { "name": "envoy.filters.http.grpc_stats", "typedConfig": { @@ -1244,4 +1244,4 @@ } ] } -} \ No newline at end of file +} diff --git a/src/go/configgenerator/listener_generator.go b/src/go/configgenerator/listener_generator.go index 0fddb9684..34a6c4026 100644 --- a/src/go/configgenerator/listener_generator.go +++ b/src/go/configgenerator/listener_generator.go @@ -124,6 +124,16 @@ func makeListener(serviceInfo *sc.ServiceInfo) (*listenerpb.Listener, error) { // Add gRPC Transcoder filter and gRPCWeb filter configs for gRPC backend. if serviceInfo.GrpcSupportRequired { + // grpc-web filter should be before grpc transcoder filter. + // It converts content-type application/grpc-web to application/grpc and + // grpc transcoder will bypass requests with application/grpc content type. + // Otherwise grpc transcoder will try to transcode a grpc-web request which + // will fail. + grpcWebFilter := &hcmpb.HttpFilter{ + Name: util.GRPCWeb, + } + httpFilters = append(httpFilters, grpcWebFilter) + transcoderFilter := makeTranscoderFilter(serviceInfo) if transcoderFilter != nil { httpFilters = append(httpFilters, transcoderFilter) @@ -131,11 +141,6 @@ func makeListener(serviceInfo *sc.ServiceInfo) (*listenerpb.Listener, error) { glog.Infof("adding Transcoder Filter config: %v", jsonStr) } - grpcWebFilter := &hcmpb.HttpFilter{ - Name: util.GRPCWeb, - } - httpFilters = append(httpFilters, grpcWebFilter) - // GrpcStats filter is used to count gRPC frames. // The data is stored in filterState and used by ServiceControl // filter in the final report call. diff --git a/src/go/configmanager/config_manager_test.go b/src/go/configmanager/config_manager_test.go index e1c18ef6f..3a5687134 100644 --- a/src/go/configmanager/config_manager_test.go +++ b/src/go/configmanager/config_manager_test.go @@ -130,6 +130,9 @@ func TestFetchListeners(t *testing.T) { ] } }, + { + "name":"envoy.filters.http.grpc_web" + }, { "name":"envoy.filters.http.grpc_json_transcoder", "typedConfig":{ @@ -147,9 +150,6 @@ func TestFetchListeners(t *testing.T) { ] } }, - { - "name":"envoy.filters.http.grpc_web" - }, { "name":"envoy.filters.http.grpc_stats", "typedConfig":{ diff --git a/tests/endpoints/bookstore_grpc/proto/api_descriptor.pb b/tests/endpoints/bookstore_grpc/proto/api_descriptor.pb index d157e86a1a77b37fe5351d3bf69b0214910b4a65..1363f730c8f13187aee7e1bd55ddd2dc53f55608 100755 GIT binary patch delta 208 zcmZ1zeluc2r6SWZq0QBb+n5Dba&d7w<>zM?m*f|v3TDzSq$z #VLL zL;d26)SR-^;wTFxWBthisbqJ}VQl&O*dT(2-huX`$3YZ_3a(C*25)Y3RZ!Fv3)pn9;_=aQO}T9OK~5u|Oi Ioq8S%0JeHWJOBUy delta 119 zcmcZ^u_k;&r6SW$fz8#5+nD+PaBy)t<>zM?m*f|v3RO*ZR812RZo2$rG83;5w|<$S zesM->PFZU4CmqRX-%u${8~Ma F1psG}EEfO( diff --git a/tests/endpoints/bookstore_grpc/proto/v1/bookstore.proto b/tests/endpoints/bookstore_grpc/proto/v1/bookstore.proto index 39bb77621..36d39b058 100644 --- a/tests/endpoints/bookstore_grpc/proto/v1/bookstore.proto +++ b/tests/endpoints/bookstore_grpc/proto/v1/bookstore.proto @@ -36,6 +36,15 @@ service Bookstore { // curl http://DOMAIN_NAME/v1/shelves option (google.api.http) = { get: "/v1/shelves" + // This is added to test grpc-web, to make sure the method is enabled + // for transcoder filter, and it should bypass this method. + // grpc-web client is using POST + /service_name/method_name. + // It could be auto-generated by setting auto_mapping. But auto_mapping + // will not apply if a method already has http.rules options defined. + additional_bindings { + post: "/endpoints.examples.bookstore.Bookstore/ListShelves" + body: "*" + } }; } // Creates a new shelf in the bookstore. @@ -53,6 +62,11 @@ service Bookstore { // curl http://DOMAIN_NAME/v1/shelves/1 option (google.api.http) = { get: "/v1/shelves/{shelf}" + // This is added to test grpc-web, please see comment at ListShelves. + additional_bindings { + post: "/endpoints.examples.bookstore.Bookstore/GetShelf" + body: "*" + } }; } // Deletes a shelf, including all books that are stored on the shelf. @@ -61,6 +75,11 @@ service Bookstore { // curl -X DELETE http://DOMAIN_NAME/v1/shelves/2 option (google.api.http) = { delete: "/v1/shelves/{shelf}" + // This is added to test grpc-web, please see comment at ListShelves. + additional_bindings { + post: "/endpoints.examples.bookstore.Bookstore/DeleteShelf" + body: "*" + } }; } // Returns a list of books on a shelf.