Fix grpc-web: move grpc-web filter in front of transcoder (#176)

Signed-off-by: Wayne Zhang <[email protected]>
GoogleCloudPlatform · Jun 2, 2020 · 7d7580b · 7d7580b
1 parent fa11613
commit 7d7580b
Show file tree

Hide file tree

Showing 5 changed files with 36 additions and 12 deletions.
diff --git 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 @@
       }
     ]
   }
-}
+}
diff --git a/src/go/configgenerator/listener_generator.go b/src/go/configgenerator/listener_generator.go
@@ -124,18 +124,23 @@ 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)
 			jsonStr, _ := util.ProtoToJson(transcoderFilter)
 			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
@@ -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
diff --git 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.