tools: deprecated field check in Route Checker tool (#8058)

We need a way to run the deprecated field check on the RouteConfiguration. Today the schema check tool validates the bootstrap config. This change will help achieve similar functionality on routes served from rds. Risk Level: Low Testing: Manual testing Docs Changes: included Release Notes: included Signed-off-by: Jyoti Mahapatra <[email protected]>
envoyproxy · Aug 29, 2019 · 29f199c · 29f199c
1 parent 4f2c5a4
commit 29f199c
Show file tree

Hide file tree

Showing 7 changed files with 83 additions and 19 deletions.
diff --git a/docs/root/install/tools/route_table_check_tool.rst b/docs/root/install/tools/route_table_check_tool.rst
@@ -40,6 +40,16 @@ Usage
     -p,  --useproto
       Use Proto test file schema
 
+    -f, --fail-under
+      Represents a percent value for route test coverage under which the run should fail.
+
+    --covall
+      Enables comprehensive code coverage percent calculation taking into account all the possible
+      asserts.
+
+    --disable-deprecation-check
+      Disables the deprecation check for RouteConfiguration proto.
+
     -h,  --help
       Displays usage information and exits.
 

diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst
@@ -45,6 +45,7 @@ Version history
 * router: added :ref:`rq_retry_skipped_request_not_complete <config_http_filters_router_stats>` counter stat to router stats.
 * router check tool: add coverage reporting & enforcement.
 * router check tool: add comprehensive coverage reporting.
+* router check tool: add deprecated field check.
 * tls: added verification of IP address SAN fields in certificates against configured SANs in the
   certificate validation context.
 * tracing: added tags for gRPC response status and meesage.

diff --git a/test/tools/router_check/router.cc b/test/tools/router_check/router.cc
@@ -63,7 +63,8 @@ ToolConfig::ToolConfig(std::unique_ptr<Http::TestHeaderMapImpl> headers, int ran
     : headers_(std::move(headers)), random_value_(random_value) {}
 
 // static
-RouterCheckTool RouterCheckTool::create(const std::string& router_config_file) {
+RouterCheckTool RouterCheckTool::create(const std::string& router_config_file,
+                                        const bool disableDeprecationCheck) {
   // TODO(hennna): Allow users to load a full config and extract the route configuration from it.
   envoy::api::v2::RouteConfiguration route_config;
   auto stats = std::make_unique<Stats::IsolatedStoreImpl>();
@@ -72,6 +73,9 @@ RouterCheckTool RouterCheckTool::create(const std::string& router_config_file) {
 
   auto factory_context = std::make_unique<NiceMock<Server::Configuration::MockFactoryContext>>();
   auto config = std::make_unique<Router::ConfigImpl>(route_config, *factory_context, false);
+  if (!disableDeprecationCheck) {
+    MessageUtil::checkForDeprecation(route_config, &factory_context->runtime_loader_);
+  }
 
   return RouterCheckTool(std::move(factory_context), std::move(config), std::move(stats),
                          std::move(api), Coverage(route_config));
@@ -439,6 +443,8 @@ Options::Options(int argc, char** argv) {
   TCLAP::CmdLine cmd("router_check_tool", ' ', "none", true);
   TCLAP::SwitchArg is_proto("p", "useproto", "Use Proto test file schema", cmd, false);
   TCLAP::SwitchArg is_detailed("d", "details", "Show detailed test execution results", cmd, false);
+  TCLAP::SwitchArg disable_deprecation_check("", "disable-deprecation-check",
+                                             "Disable deprecated fields check", cmd, false);
   TCLAP::ValueArg<double> fail_under("f", "fail-under",
                                      "Fail if test coverage is under a specified amount", false,
                                      0.0, "float", cmd);
@@ -461,6 +467,7 @@ Options::Options(int argc, char** argv) {
   is_detailed_ = is_detailed.getValue();
   fail_under_ = fail_under.getValue();
   comprehensive_coverage_ = comprehensive_coverage.getValue();
+  disable_deprecation_check_ = disable_deprecation_check.getValue();
 
   if (is_proto_) {
     config_path_ = config_path.getValue();

diff --git a/test/tools/router_check/router.h b/test/tools/router_check/router.h
@@ -65,10 +65,12 @@ class RouterCheckTool : Logger::Loggable<Logger::Id::testing> {
 public:
   /**
    * @param router_config_file v2 router config file.
+   * @param disableDeprecationCheck flag to disable the RouteConfig deprecated field check
    * @return RouterCheckTool a RouterCheckTool instance with member variables set by the router
    * config file.
    * */
-  static RouterCheckTool create(const std::string& router_config_file);
+  static RouterCheckTool create(const std::string& router_config_file,
+                                const bool disableDeprecationCheck);
 
   /**
    * TODO(tonya11en): Use a YAML format for the expected routes. This will require a proto.
@@ -198,6 +200,11 @@ class Options {
    */
   bool isDetailed() const { return is_detailed_; }
 
+  /**
+   * @return true if the deprecated field check for RouteConfiguration is disabled.
+   */
+  bool disableDeprecationCheck() const { return disable_deprecation_check_; }
+
 private:
   std::string test_path_;
   std::string config_path_;
@@ -207,5 +214,6 @@ class Options {
   bool comprehensive_coverage_;
   bool is_proto_;
   bool is_detailed_;
+  bool disable_deprecation_check_;
 };
 } // namespace Envoy
diff --git a/test/tools/router_check/router_check.cc b/test/tools/router_check/router_check.cc
@@ -10,8 +10,9 @@ int main(int argc, char* argv[]) {
   const bool enforce_coverage = options.failUnder() != 0.0;
   try {
     Envoy::RouterCheckTool checktool =
-        options.isProto() ? Envoy::RouterCheckTool::create(options.configPath())
-                          : Envoy::RouterCheckTool::create(options.unlabelledConfigPath());
+        options.isProto() ? Envoy::RouterCheckTool::create(options.configPath(),
+                                                           options.disableDeprecationCheck())
+                          : Envoy::RouterCheckTool::create(options.unlabelledConfigPath(), true);
 
     if (options.isDetailed()) {
       checktool.setShowDetails();

diff --git a/test/tools/router_check/test/config/HeaderMatchedRouting.yaml b/test/tools/router_check/test/config/HeaderMatchedRouting.yaml
@@ -30,7 +30,9 @@ virtual_hosts:
       prefix: /
       headers:
       - name: test_header_pattern
-        regex_match: ^user=test-\d+$
+        safe_regex_match:
+          google_re2: {}
+          regex: ^user=test-\d+$
     route:
       cluster: local_service_with_header_pattern_set_regex
   - match:

diff --git a/test/tools/router_check/test/config/TestRoutes.yaml b/test/tools/router_check/test/config/TestRoutes.yaml
@@ -104,26 +104,61 @@ virtual_hosts:
           timeout:
             seconds: 30
     virtual_clusters:
-      - pattern: ^/rides$
-        method: POST
+      - headers:
+          - name: :path
+            safe_regex_match:
+              google_re2: {}
+              regex: ^/rides$
+          - name: :method
+            exact_match: POST
         name: ride_request
-      - pattern: ^/rides/\d+$
-        method: PUT
+      - headers:
+          - name: :path
+            safe_regex_match:
+              google_re2: {}
+              regex: ^/rides/\d+$
+          - name: :method
+            exact_match: PUT
         name: update_ride
-      - pattern: ^/users/\d+/chargeaccounts$
-        method: POST
+      - headers:
+          - name: :path
+            safe_regex_match:
+              google_re2: {}
+              regex: ^/users/\d+/chargeaccounts$
+          - name: :method
+            exact_match: POST
         name: cc_add
-      - pattern: ^/users/\d+/chargeaccounts/(?!validate)\w+$
-        method: PUT
+      - headers:
+          - name: :path
+            safe_regex_match:
+              google_re2: {}
+              regex: ^/users/\d+/chargeaccounts/[^validate]\w+$
+          - name: :method
+            exact_match: PUT
         name: cc_add
-      - pattern: ^/users$
-        method: POST
+      - headers:
+          - name: :path
+            safe_regex_match:
+              google_re2: {}
+              regex: ^/users$
+          - name: :method
+            exact_match: POST
         name: create_user_login
-      - pattern: ^/users/\d+$
-        method: PUT
+      - headers:
+          - name: :path
+            safe_regex_match:
+              google_re2: {}
+              regex: ^/users/\d+$
+          - name: :method
+            exact_match: PUT
         name: update_user
-      - pattern: ^/users/\d+/location$
-        method: POST
+      - headers:
+          - name: :path
+            safe_regex_match:
+              google_re2: {}
+              regex: ^/users/\d+/location$
+          - name: :method
+            exact_match: POST
         name: ulu
 internal_only_headers:
   - x-lyft-user-id