From 74d120034bb29b462ebb37ec14644135b5c39c06 Mon Sep 17 00:00:00 2001 From: Wayne Zhang Date: Wed, 12 Feb 2020 15:49:01 -0800 Subject: [PATCH 1/6] Support additional_bindings --- src/api_manager/config.cc | 122 ++++++++++++++++++++------------------ src/api_manager/config.h | 8 ++- 2 files changed, 72 insertions(+), 58 deletions(-) diff --git a/src/api_manager/config.cc b/src/api_manager/config.cc index 1fb658202..9f2df6a86 100644 --- a/src/api_manager/config.cc +++ b/src/api_manager/config.cc @@ -134,80 +134,88 @@ bool Config::LoadQuotaRule(ApiManagerEnvInterface *env) { bool Config::LoadHttpMethods(ApiManagerEnvInterface *env, PathMatcherBuilder *pmb) { - std::set all_urls, urls_with_options; + std::set all_urls; + + // HttpRules are allowed additonal bindings, but only at the top level. + // If the top level rule has failed to register, this means the addtional + // bindings won't be registered. + for (const auto &rule : service_.http().rules()) { + if(!RegisterHttpMethods(env, pmb, all_urls, rule)) { + env->LogDebug("Skipping aditional rule bindings."); + continue; + } + for (const auto &additional_rule : rule.additional_bindings()) { + RegisterHttpMethods(env, pmb, all_urls, additional_rule); + } + } + // By default, allow_cors is false. This means that the default behavior // of ESP is to reject all "OPTIONS" requests. If customers want to enable // CORS, they need to set "allow_cors" to true in swagger config. - bool allow_cors = false; for (const auto &endpoint : service_.endpoints()) { if (endpoint.name() == service_.name() && endpoint.allow_cors()) { - allow_cors = true; env->LogDebug("CORS is allowed."); - break; + return AddOptionsMethodForAllUrls(env, pmb, all_urls); } } - for (const auto &rule : service_.http().rules()) { - const string &selector = rule.selector(); - const string *url = nullptr; - const char *http_method = nullptr; - - switch (rule.pattern_case()) { - case ::google::api::HttpRule::kGet: - url = &rule.get(); - http_method = http_get; - break; - case ::google::api::HttpRule::kPut: - url = &rule.put(); - http_method = http_put; - break; - case ::google::api::HttpRule::kPost: - url = &rule.post(); - http_method = http_post; - break; - case ::google::api::HttpRule::kDelete: - url = &rule.delete_(); - http_method = http_delete; - break; - case ::google::api::HttpRule::kPatch: - url = &rule.patch(); - http_method = http_patch; - break; - case ::google::api::HttpRule::kCustom: - url = &rule.custom().path(); - http_method = rule.custom().kind().c_str(); - break; - default: - break; - } + return true; +} - if (http_method == nullptr || url == nullptr || url->empty()) { - env->LogError("Invalid HTTP binding encountered."); - continue; - } +bool Config::RegisterHttpMethods(ApiManagerEnvInterface *env, + PathMatcherBuilder *pmb, + std::set &all_urls, + const ::google::api::HttpRule &rule) { - MethodInfoImpl *mi = GetOrCreateMethodInfoImpl(selector, "", ""); + const string *url = nullptr; + const char *http_method = nullptr; - auto status = pmb->Register(http_method, *url, rule.body(), mi); - if (!status.ok()) { - env->LogError(status.message()); - } else if (allow_cors) { - all_urls.insert(*url); - if (strcmp(http_method, http_options) == 0) { - urls_with_options.insert(*url); - } - } + switch (rule.pattern_case()) { + case ::google::api::HttpRule::kGet: + url = &rule.get(); + http_method = http_get; + break; + case ::google::api::HttpRule::kPut: + url = &rule.put(); + http_method = http_put; + break; + case ::google::api::HttpRule::kPost: + url = &rule.post(); + http_method = http_post; + break; + case ::google::api::HttpRule::kDelete: + url = &rule.delete_(); + http_method = http_delete; + break; + case ::google::api::HttpRule::kPatch: + url = &rule.patch(); + http_method = http_patch; + break; + case ::google::api::HttpRule::kCustom: + url = &rule.custom().path(); + http_method = rule.custom().kind().c_str(); + break; + default: + break; } - if (!allow_cors) { - return true; + if (http_method == nullptr || url == nullptr || url->empty()) { + env->LogError("Invalid HTTP binding encountered."); + return false; + } + + MethodInfoImpl *mi = GetOrCreateMethodInfoImpl(rule.selector(), "", ""); + + if (!pmb->Register(http_method, *url, rule.body(), mi)) { + env->LogError(std::string("Invalid HTTP template: " + *url)); + return false; } - // Remove urls with options. - for (auto url : urls_with_options) { - all_urls.erase(url); + if (strcmp(http_method, http_options) != 0) { + all_urls.insert(*url); } - return AddOptionsMethodForAllUrls(env, pmb, all_urls); + + return true; } bool Config::AddOptionsMethodForAllUrls(ApiManagerEnvInterface *env, diff --git a/src/api_manager/config.h b/src/api_manager/config.h index 60342276e..cae0cd6ac 100644 --- a/src/api_manager/config.h +++ b/src/api_manager/config.h @@ -106,10 +106,16 @@ class Config { bool LoadService(ApiManagerEnvInterface *env, const std::string &service_config); - // Create MethodInfo for HTTP methods, register them to PathMatcher. + // Loads the Http rules, registers and adds CORS support if required. bool LoadHttpMethods(ApiManagerEnvInterface *env, PathMatcherBuilder *pmb); + // Create MethodInfo for HTTP methods, register them to PathMatcher. + bool RegisterHttpMethods(ApiManagerEnvInterface *env, + PathMatcherBuilder *pmb, + std::set &all_urls, + const ::google::api::HttpRule &rule); + // Add a special option method info for all URLs to support CORS. bool AddOptionsMethodForAllUrls(ApiManagerEnvInterface *env, PathMatcherBuilder *pmb, From 79ef525873f7fefa5af567d8fb9dcff345a6c0d5 Mon Sep 17 00:00:00 2001 From: Wayne Zhang Date: Wed, 12 Feb 2020 18:21:54 -0800 Subject: [PATCH 2/6] add some tests Signed-off-by: Wayne Zhang --- src/api_manager/config.cc | 38 ++++++++++++++++++--------------- src/api_manager/config.h | 10 ++++----- src/api_manager/config_test.cc | 30 ++++++++++++++++++++++++++ test/transcoding/README.md | 10 ++++++--- test/transcoding/service.pb.txt | 9 ++++---- 5 files changed, 67 insertions(+), 30 deletions(-) diff --git a/src/api_manager/config.cc b/src/api_manager/config.cc index 9f2df6a86..0d3e2c485 100644 --- a/src/api_manager/config.cc +++ b/src/api_manager/config.cc @@ -140,12 +140,18 @@ bool Config::LoadHttpMethods(ApiManagerEnvInterface *env, // If the top level rule has failed to register, this means the addtional // bindings won't be registered. for (const auto &rule : service_.http().rules()) { - if(!RegisterHttpMethods(env, pmb, all_urls, rule)) { - env->LogDebug("Skipping aditional rule bindings."); - continue; - } - for (const auto &additional_rule : rule.additional_bindings()) { - RegisterHttpMethods(env, pmb, all_urls, additional_rule); + // addtional_bindings use the same top level selector. + MethodInfoImpl *mi = GetOrCreateMethodInfoImpl(rule.selector(), "", ""); + + if (!RegisterHttpRule(env, pmb, mi, all_urls, rule)) { + env->LogError("Failed to add http rule: " + rule.DebugString()); + } else { + for (const auto &additional_rule : rule.additional_bindings()) { + if (!RegisterHttpRule(env, pmb, mi, all_urls, additional_rule)) { + env->LogError("Failed to add additional_binding rule: " + + additional_rule.DebugString()); + } + } } } @@ -154,7 +160,7 @@ bool Config::LoadHttpMethods(ApiManagerEnvInterface *env, // CORS, they need to set "allow_cors" to true in swagger config. for (const auto &endpoint : service_.endpoints()) { if (endpoint.name() == service_.name() && endpoint.allow_cors()) { - env->LogDebug("CORS is allowed."); + env->LogInfo("CORS is allowed."); return AddOptionsMethodForAllUrls(env, pmb, all_urls); } } @@ -162,11 +168,11 @@ bool Config::LoadHttpMethods(ApiManagerEnvInterface *env, return true; } -bool Config::RegisterHttpMethods(ApiManagerEnvInterface *env, - PathMatcherBuilder *pmb, - std::set &all_urls, - const ::google::api::HttpRule &rule) { - +bool Config::RegisterHttpRule(ApiManagerEnvInterface *env, + PathMatcherBuilder *pmb, + MethodInfoImpl *mi, + std::set &all_urls, + const ::google::api::HttpRule &rule) { const string *url = nullptr; const char *http_method = nullptr; @@ -204,17 +210,15 @@ bool Config::RegisterHttpMethods(ApiManagerEnvInterface *env, return false; } - MethodInfoImpl *mi = GetOrCreateMethodInfoImpl(rule.selector(), "", ""); - - if (!pmb->Register(http_method, *url, rule.body(), mi)) { - env->LogError(std::string("Invalid HTTP template: " + *url)); + auto status = pmb->Register(http_method, *url, rule.body(), mi); + if (!status.ok()) { + env->LogError(status.message()); return false; } if (strcmp(http_method, http_options) != 0) { all_urls.insert(*url); } - return true; } diff --git a/src/api_manager/config.h b/src/api_manager/config.h index cae0cd6ac..476b29a47 100644 --- a/src/api_manager/config.h +++ b/src/api_manager/config.h @@ -110,11 +110,11 @@ class Config { bool LoadHttpMethods(ApiManagerEnvInterface *env, PathMatcherBuilder *pmb); - // Create MethodInfo for HTTP methods, register them to PathMatcher. - bool RegisterHttpMethods(ApiManagerEnvInterface *env, - PathMatcherBuilder *pmb, - std::set &all_urls, - const ::google::api::HttpRule &rule); + // Register a Http rule to the path matcher builder. + bool RegisterHttpRule(ApiManagerEnvInterface *env, + PathMatcherBuilder *pmb, + MethodInfoImpl *mi, std::set &all_urls, + const ::google::api::HttpRule &rule); // Add a special option method info for all URLs to support CORS. bool AddOptionsMethodForAllUrls(ApiManagerEnvInterface *env, diff --git a/src/api_manager/config_test.cc b/src/api_manager/config_test.cc index 9053193da..16f807890 100644 --- a/src/api_manager/config_test.cc +++ b/src/api_manager/config_test.cc @@ -900,6 +900,36 @@ TEST(Config, TestHttpOptions) { ASSERT_EQ(nullptr, method2); } +TEST(Config, TestAdditionalBindings) { + MockApiManagerEnvironmentWithLog env; + + static const char config_text[] = R"( + name: "Service.Name" + http { + rules { + selector: "GetMessages" + get: "/v1/messages/{message_id}" + additional_bindings { + get: "/v1/users/{user_id}/messages/{message_id}" + } + additional_bindings { + get: "/v2/messages/{user_id}/{message_id}" + } + } + } +)"; + + std::unique_ptr config = Config::Create(&env, config_text, ""); + ASSERT_TRUE(config); + + auto method = config->GetMethodInfo("GET", "/v1/messages/1234"); + ASSERT_NE(nullptr, method); + + ASSERT_EQ(method, + config->GetMethodInfo("GET", "/v1/users/123/messages/1234")); + ASSERT_EQ(method, config->GetMethodInfo("GET", "/v2/messages/123/1234")); +} + TEST(Config, TestHttpOptionsSelector) { MockApiManagerEnvironmentWithLog env; diff --git a/test/transcoding/README.md b/test/transcoding/README.md index 34077670b..b6beb09d7 100644 --- a/test/transcoding/README.md +++ b/test/transcoding/README.md @@ -49,12 +49,16 @@ producer_project_id: "" name: ".appspot.com" id: "2016-08-25r1" ``` - * add following rules to http rule: + * add following additional_bindings to CreateBook http rule: ``` rules { selector: "endpoints.examples.bookstore.Bookstore.CreateBook" - post: "/shelves/{shelf}/books/{book.id}/{book.author}" - body: "book.title" + post: "/shelves/{shelf}/books" + body: "book" ++ additional_bindings { ++ post: "/shelves/{shelf}/books/{book.id}/{book.author}" ++ body: "book.title" ++ } } ``` - Replace /tmp/out.txt with test/transcoding/service.pb.txt diff --git a/test/transcoding/service.pb.txt b/test/transcoding/service.pb.txt index 313fc97f9..9a0055767 100644 --- a/test/transcoding/service.pb.txt +++ b/test/transcoding/service.pb.txt @@ -837,11 +837,10 @@ http { selector: "endpoints.examples.bookstore.Bookstore.CreateBook" post: "/shelves/{shelf}/books" body: "book" - } - rules { - selector: "endpoints.examples.bookstore.Bookstore.CreateBook" - post: "/shelves/{shelf}/books/{book.id}/{book.author}" - body: "book.title" + additional_bindings { + post: "/shelves/{shelf}/books/{book.id}/{book.author}" + body: "book.title" + } } rules { selector: "endpoints.examples.bookstore.Bookstore.GetBook" From 4b87870862590041f4b419128647c9fe84772124 Mon Sep 17 00:00:00 2001 From: Wayne Zhang Date: Wed, 12 Feb 2020 18:53:18 -0800 Subject: [PATCH 3/6] Add negative tests --- src/api_manager/config_test.cc | 56 ++++++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/src/api_manager/config_test.cc b/src/api_manager/config_test.cc index 16f807890..981f5207b 100644 --- a/src/api_manager/config_test.cc +++ b/src/api_manager/config_test.cc @@ -930,6 +930,62 @@ TEST(Config, TestAdditionalBindings) { ASSERT_EQ(method, config->GetMethodInfo("GET", "/v2/messages/123/1234")); } +TEST(Config, TestAdditionalBindingsFailedTopLevel) { + MockApiManagerEnvironmentWithLog env; + + // Top level rule has empty url, so it will fail to add + static const char config_text[] = R"( + name: "Service.Name" + http { + rules { + selector: "GetMessages" + get: "" + additional_bindings { + get: "/v1/users" + } + } + } +)"; + + std::unique_ptr config = Config::Create(&env, config_text, ""); + ASSERT_TRUE(config); + + // addtional_bindings is not added since top level rule fails + auto method = config->GetMethodInfo("GET", "/v1/users"); + ASSERT_EQ(nullptr, method); +} + +TEST(Config, TestAdditionalBindingsFailOne) { + MockApiManagerEnvironmentWithLog env; + + // one of additional_bindings fails with empty url + // but other additional_bindings is added + static const char config_text[] = R"( + name: "Service.Name" + http { + rules { + selector: "GetMessages" + get: "/v1/messages/{message_id}" + additional_bindings { + get: "" + } + additional_bindings { + get: "/v1/users/{user_id}/messages/{message_id}" + } + } + } +)"; + + std::unique_ptr config = Config::Create(&env, config_text, ""); + ASSERT_TRUE(config); + + auto method = config->GetMethodInfo("GET", "/v1/messages/1234"); + ASSERT_NE(nullptr, method); + + ASSERT_EQ(method, + config->GetMethodInfo("GET", "/v1/users/123/messages/1234")); +} + TEST(Config, TestHttpOptionsSelector) { MockApiManagerEnvironmentWithLog env; From d83d61b2a295ae6a4b5cc6224044b845aad0d622 Mon Sep 17 00:00:00 2001 From: Wayne Zhang Date: Wed, 12 Feb 2020 19:02:06 -0800 Subject: [PATCH 4/6] fix spell errors --- src/api_manager/config.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/api_manager/config.cc b/src/api_manager/config.cc index 0d3e2c485..6cc427c36 100644 --- a/src/api_manager/config.cc +++ b/src/api_manager/config.cc @@ -136,11 +136,11 @@ bool Config::LoadHttpMethods(ApiManagerEnvInterface *env, PathMatcherBuilder *pmb) { std::set all_urls; - // HttpRules are allowed additonal bindings, but only at the top level. - // If the top level rule has failed to register, this means the addtional - // bindings won't be registered. + // A HttpRule may have additional bindings, but only at the top level. + // If the top level rule fails to register, its additional bindings + // won't be registered. for (const auto &rule : service_.http().rules()) { - // addtional_bindings use the same top level selector. + // additional bindings use the same top level selector. MethodInfoImpl *mi = GetOrCreateMethodInfoImpl(rule.selector(), "", ""); if (!RegisterHttpRule(env, pmb, mi, all_urls, rule)) { From 42eb4d6f85af726302154142328a90ec92bea0e6 Mon Sep 17 00:00:00 2001 From: Wayne Zhang Date: Wed, 12 Feb 2020 19:10:06 -0800 Subject: [PATCH 5/6] spell --- src/api_manager/config_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/api_manager/config_test.cc b/src/api_manager/config_test.cc index 981f5207b..e52335677 100644 --- a/src/api_manager/config_test.cc +++ b/src/api_manager/config_test.cc @@ -950,7 +950,7 @@ TEST(Config, TestAdditionalBindingsFailedTopLevel) { std::unique_ptr config = Config::Create(&env, config_text, ""); ASSERT_TRUE(config); - // addtional_bindings is not added since top level rule fails + // The additional bindings is not added since top level rule fails auto method = config->GetMethodInfo("GET", "/v1/users"); ASSERT_EQ(nullptr, method); } From bb889427f1f91f4514780a788c42b738c43088a1 Mon Sep 17 00:00:00 2001 From: Wayne Zhang Date: Wed, 12 Feb 2020 19:20:01 -0800 Subject: [PATCH 6/6] add more negative tests --- src/api_manager/config_test.cc | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/api_manager/config_test.cc b/src/api_manager/config_test.cc index e52335677..04f9dd7df 100644 --- a/src/api_manager/config_test.cc +++ b/src/api_manager/config_test.cc @@ -905,6 +905,10 @@ TEST(Config, TestAdditionalBindings) { static const char config_text[] = R"( name: "Service.Name" + endpoints { + name: "Service.Name" + allow_cors: true + } http { rules { selector: "GetMessages" @@ -928,6 +932,17 @@ TEST(Config, TestAdditionalBindings) { ASSERT_EQ(method, config->GetMethodInfo("GET", "/v1/users/123/messages/1234")); ASSERT_EQ(method, config->GetMethodInfo("GET", "/v2/messages/123/1234")); + + // OPTIONS added + for (auto path : {"/v1/messages/1234", "/v1/users/123/messages/1234", + "/v2/messages/123/1234"}) { + auto method = config->GetMethodInfo("OPTIONS", path); + ASSERT_NE(nullptr, method); + ASSERT_EQ("CORS", method->name()); + ASSERT_FALSE(method->auth()); + // For all added OPTIONS methods, allow_unregistered_calls is true. + ASSERT_TRUE(method->allow_unregistered_calls()); + } } TEST(Config, TestAdditionalBindingsFailedTopLevel) { @@ -984,6 +999,11 @@ TEST(Config, TestAdditionalBindingsFailOne) { ASSERT_EQ(method, config->GetMethodInfo("GET", "/v1/users/123/messages/1234")); + + // OPTIONS are not added + ASSERT_EQ(nullptr, config->GetMethodInfo("OPTIONS", "/v1/messages/1234")); + ASSERT_EQ(nullptr, + config->GetMethodInfo("OPTIONS", "/v1/users/123/messages/1234")); } TEST(Config, TestHttpOptionsSelector) {