time: Add 'format' test to ensure no one directly instantiates Prod*T…

…ime from source. (envoyproxy#4248)

Time should be injected in all production code, so that it can ultimately be tested using mock time or simulated time. Production code that directly instantiates ProdSystemTimeSource makes this impossible, and this PR ensures that 'format' checks fail if that is violated. This partially addresses issue envoyproxy#4160.

Risk Level: low
Testing: //test/...
Docs Changes: n/a
Release Notes: n/a

Signed-off-by: Joshua Marantz <jmarantz@google.com>

tools/check_format.py

@@ -23,6 +23,15 @@
 GOOGLE_PROTOBUF_WHITELIST = ("ci/prebuilt", "source/common/protobuf", "api/test")
 REPOSITORIES_BZL = "bazel/repositories.bzl"
 
+# Files matching these exact names can reference prod time. These include the class
+# definitions for prod time, the construction of them in main(), and perf annotation.
+# For now it includes the validation server but that really should be injected too.
+PROD_TIME_WHITELIST = ('./source/common/common/utility.h',
+                       './source/exe/main_common.cc',
+                       './source/exe/main_common.h',
+                       './source/server/config_validation/server.cc',
+                       './source/common/common/perf_annotation.h')
+
 CLANG_FORMAT_PATH = os.getenv("CLANG_FORMAT", "clang-format-6.0")
 BUILDIFIER_PATH = os.getenv("BUILDIFIER_BIN", "$GOPATH/bin/buildifier")
 ENVOY_BUILD_FIXER_PATH = os.path.join(
@@ -65,6 +74,12 @@ def whitelistedForProtobufDeps(file_path):
   return (file_path.endswith(PROTO_SUFFIX) or file_path.endswith(REPOSITORIES_BZL) or \
           any(path_segment in file_path for path_segment in GOOGLE_PROTOBUF_WHITELIST))
 
+# Production time sources should not be instantiated in the source, except for a few
+# specific cases. They should be passed down from where they are instantied to where
+# they need to be used, e.g. through the ServerInstance, Dispatcher, or ClusterManager.
+def whitelistedForProdTime(file_path):
+  return file_path in PROD_TIME_WHITELIST or file_path.startswith('./test/')
+
 def findSubstringAndReturnError(pattern, file_path, error_message):
   with open(file_path) as f:
     text = f.read()
@@ -145,6 +160,9 @@ def checkSourceLine(line, file_path, reportError):
     # comments, for example this one.
     reportError("Don't use <mutex> or <condition_variable*>, switch to "
                 "Thread::MutexBasicLockable in source/common/common/thread.h")
+  if not whitelistedForProdTime(file_path):
+    if 'ProdSystemTimeSource' in line or 'ProdMonotonicTimeSource' in line:
+      reportError("Don't reference real-time sources from production code; use injection")
 
 def checkBuildLine(line, file_path, reportError):
   if not whitelistedForProtobufDeps(file_path) and '"protobuf"' in line:

tools/check_format_test_helper.py

@@ -143,6 +143,10 @@ def checkFileExpectingOK(filename):
                                     "Don't use <mutex> or <condition_variable*>")
   errors += fixFileExpectingFailure("condition_variable_any.cc",
                                     "Don't use <mutex> or <condition_variable*>")
+  errors += fixFileExpectingFailure(
+      "prod_system_time.cc", "Don't reference real-time sources from production code; use injection")
+  errors += fixFileExpectingFailure(
+      "prod_monotonic_time.cc", "Don't reference real-time sources from production code; use injection")
 
   errors += fixFileExpectingNoChange("ok_file.cc")
 
@@ -163,6 +167,10 @@ def checkFileExpectingOK(filename):
   errors += checkFileExpectingError("bad_envoy_build_sys_ref.BUILD",
                                     "Superfluous '@envoy//' prefix")
   errors += checkFileExpectingError("proto_format.proto", "clang-format check failed")
+  errors += checkFileExpectingError(
+      "prod_system_time.cc", "Don't reference real-time sources from production code; use injection")
+  errors += checkFileExpectingError(
+      "prod_monotonic_time.cc", "Don't reference real-time sources from production code; use injection")
 
   errors += checkFileExpectingOK("ok_file.cc")
 

tools/testdata/check_format/prod_monotonic_time.cc

@@ -0,0 +1,4 @@
+int foo() {
+  ProdSystemTimeSource system_time;
+  ProdMonotonicTimeSource monotonic_time;
+}

tools/testdata/check_format/prod_system_time.cc

@@ -0,0 +1,3 @@
+int foo() {
+  ProdMonotonicTimeSource monotonic_time;
+}