thrift: filter integration tests

bazel/external/apache_thrift.BUILD

@@ -0,0 +1,21 @@
+# The apache-thrift distribution does not keep the thrift files in a directory with the
+# expected package name (it uses src/Thrift.py vs src/thrift/Thrift.py), so we provide a
+# genrule to copy src/**/*.py to thrift/**/*.py.
+src_files = glob(["src/**/*.py"])
+
+genrule(
+    name = "thrift_files",
+    srcs = src_files,
+    outs = [f.replace("src/", "thrift/") for f in src_files],
+    cmd = '\n'.join(
+        ['mkdir -p $$(dirname $(location %s)) && cp $(location %s) $(location :%s)' % (f, f, f.replace('src/', 'thrift/')) for f in src_files]
+    ),
+    visibility = ["//visibility:private"],
+)
+
+py_library(
+    name = "apache_thrift",
+    srcs = [":thrift_files"],
+    visibility = ["//visibility:public"],
+    deps = ["@six_archive//:six"],
+)

bazel/external/twitter_common_finagle_thrift.BUILD

@@ -0,0 +1,7 @@
+py_library(
+    name = "twitter_common_finagle_thrift",
+    srcs = glob([
+        "gen/**/*.py",
+    ]),
+    visibility = ["//visibility:public"],
+)

bazel/external/twitter_common_lang.BUILD

@@ -0,0 +1,7 @@
+py_library(
+    name = "twitter_common_lang",
+    srcs = glob([
+        "twitter/**/*.py",
+    ]),
+    visibility = ["//visibility:public"],
+)

bazel/external/twitter_common_rpc.BUILD

@@ -0,0 +1,11 @@
+py_library(
+    name = "twitter_common_rpc",
+    srcs = glob([
+        "twitter/**/*.py",
+    ]),
+    visibility = ["//visibility:public"],
+    deps = [
+         "@com_github_twitter_common_lang//:twitter_common_lang",
+         "@com_github_twitter_common_finagle_thrift//:twitter_common_finagle_thrift"
+    ],
+)

bazel/repositories.bzl

@@ -141,6 +141,22 @@ def _python_deps():
         name = "jinja2",
         actual = "@com_github_pallets_jinja//:jinja2",
     )
+    _repository_impl(
+        name = "com_github_apache_thrift",
+        build_file = "@envoy//bazel/external:apache_thrift.BUILD",
+    )
+    _repository_impl(
+        name = "com_github_twitter_common_lang",
+        build_file = "@envoy//bazel/external:twitter_common_lang.BUILD",
+    )
+    _repository_impl(
+        name = "com_github_twitter_common_rpc",
+        build_file = "@envoy//bazel/external:twitter_common_rpc.BUILD",
+    )
+    _repository_impl(
+        name = "com_github_twitter_common_finagle_thrift",
+        build_file = "@envoy//bazel/external:twitter_common_finagle_thrift.BUILD",
+    )
 
 # Bazel native C++ dependencies. For the depedencies that doesn't provide autoconf/automake builds.
 def _cc_deps():

bazel/repository_locations.bzl

@@ -8,6 +8,11 @@ REPOSITORY_LOCATIONS = dict(
         commit = "92020a042c0cd46979db9f6f0cb32783dc07765e",  # 2018-06-08
         remote = "https://github.com/abseil/abseil-cpp",
     ),
+    com_github_apache_thrift = dict(
+        sha256 = "7d59ac4fdcb2c58037ebd4a9da5f9a49e3e034bf75b3f26d9fe48ba3d8806e6b",
+        urls = ["https://files.pythonhosted.org/packages/c6/b4/510617906f8e0c5660e7d96fbc5585113f83ad547a3989b80297ac72a74c/thrift-0.11.0.tar.gz"], # 0.11.0
+        strip_prefix = "thrift-0.11.0",
+    ),
     com_github_bombela_backward = dict(
         commit = "44ae9609e860e3428cd057f7052e505b4819eb84",  # 2018-02-06
         remote = "https://github.com/bombela/backward-cpp",
@@ -80,6 +85,21 @@ REPOSITORY_LOCATIONS = dict(
         commit = "f54b0e47a08782a6131cc3d60f94d038fa6e0a51",  # v1.1.0
         remote = "https://github.com/tencent/rapidjson",
     ),
+    com_github_twitter_common_lang = dict(
+        sha256 = "56d1d266fd4767941d11c27061a57bc1266a3342e551bde3780f9e9eb5ad0ed1",
+        urls = ["https://files.pythonhosted.org/packages/08/bc/d6409a813a9dccd4920a6262eb6e5889e90381453a5f58938ba4cf1d9420/twitter.common.lang-0.3.9.tar.gz"],  # 0.3.9
+        strip_prefix = "twitter.common.lang-0.3.9/src",
+    ),
+    com_github_twitter_common_rpc = dict(
+        sha256 = "0792b63fb2fb32d970c2e9a409d3d00633190a22eb185145fe3d9067fdaa4514",
+        urls = ["https://files.pythonhosted.org/packages/be/97/f5f701b703d0f25fbf148992cd58d55b4d08d3db785aad209255ee67e2d0/twitter.common.rpc-0.3.9.tar.gz"],  # 0.3.9
+        strip_prefix = "twitter.common.rpc-0.3.9/src",
+    ),
+    com_github_twitter_common_finagle_thrift = dict(
+        sha256 = "1e3a57d11f94f58745e6b83348ecd4fa74194618704f45444a15bc391fde497a",
+        urls = ["https://files.pythonhosted.org/packages/f9/e7/4f80d582578f8489226370762d2cf6bc9381175d1929eba1754e03f70708/twitter.common.finagle-thrift-0.3.9.tar.gz"], # 0.3.9
+        strip_prefix = "twitter.common.finagle-thrift-0.3.9/src",
+    ),
     com_google_googletest = dict(
         commit = "43863938377a9ea1399c0596269e0890b5c5515a",
         remote = "https://github.com/google/googletest",

source/extensions/filters/network/thrift_proxy/binary_protocol.cc

@@ -97,7 +97,11 @@ bool BinaryProtocolImpl::readFieldBegin(Buffer::Instance& buffer, std::string& n
     if (buffer.length() < 3) {
       return false;
     }
-    field_id = BufferHelper::peekI16(buffer, 1);
+    int16_t id = BufferHelper::peekI16(buffer, 1);
+    if (id < 0) {
+      throw EnvoyException(fmt::format("invalid binary protocol field id {}", id));
+    }
+    field_id = id;
     buffer.drain(3);
   }
 

source/extensions/filters/network/thrift_proxy/compact_protocol.cc

@@ -140,7 +140,7 @@ bool CompactProtocolImpl::readFieldBegin(Buffer::Instance& buffer, std::string&
       return false;
     }
 
-    if (id <= 0 || id > INT16_MAX) {
+    if (id < 0 || id > INT16_MAX) {
       throw EnvoyException(fmt::format("invalid compact protocol field id {}", id));
     }
 
@@ -390,7 +390,7 @@ bool CompactProtocolImpl::readString(Buffer::Instance& buffer, std::string& valu
   }
 
   int len_size;
-  int32_t str_len = BufferHelper::peekZigZagI32(buffer, 0, len_size);
+  int32_t str_len = BufferHelper::peekVarIntI32(buffer, 0, len_size);
   if (len_size < 0) {
     return false;
   }

test/extensions/extensions_build_system.bzl

@@ -1,4 +1,4 @@
-load("//bazel:envoy_build_system.bzl", "envoy_cc_test", "envoy_cc_mock")
+load("//bazel:envoy_build_system.bzl", "envoy_cc_test", "envoy_cc_test_library", "envoy_cc_mock")
 load("@envoy_build_config//:extensions_build_config.bzl", "EXTENSIONS")
 
 # All extension tests should use this version of envoy_cc_test(). It allows compiling out
@@ -12,10 +12,18 @@ def envoy_extension_cc_test(name,
 
     envoy_cc_test(name, **kwargs)
 
+def envoy_extension_cc_test_library(name,
+                                    extension_name,
+                                    **kwargs):
+    if not extension_name in EXTENSIONS:
+        return
+
+    envoy_cc_test_library(name, **kwargs)
+
 def envoy_extension_cc_mock(name,
                             extension_name,
                             **kwargs):
     if not extension_name in EXTENSIONS:
         return
 
-    envoy_cc_mock(name, **kwargs)
+    envoy_cc_mock(name, **kwargs)

test/extensions/filters/network/thrift_proxy/BUILD

@@ -2,30 +2,32 @@ licenses(["notice"])  # Apache 2
 
 load(
     "//bazel:envoy_build_system.bzl",
-    "envoy_cc_mock",
-    "envoy_cc_test_library",
     "envoy_package",
 )
 load(
     "//test/extensions:extensions_build_system.bzl",
+    "envoy_extension_cc_mock",
     "envoy_extension_cc_test",
+    "envoy_extension_cc_test_library",
 )
 
 envoy_package()
 
-envoy_cc_mock(
+envoy_extension_cc_mock(
     name = "mocks",
     srcs = ["mocks.cc"],
     hdrs = ["mocks.h"],
+    extension_name = "envoy.filters.network.thrift_proxy",
     deps = [
         "//source/extensions/filters/network/thrift_proxy:transport_lib",
         "//test/test_common:printers_lib",
     ],
 )
 
-envoy_cc_test_library(
+envoy_extension_cc_test_library(
     name = "utility_lib",
     hdrs = ["utility.h"],
+    extension_name = "envoy.filters.network.thrift_proxy",
     deps = [
         "//source/common/buffer:buffer_lib",
         "//source/common/common:byte_order_lib",
@@ -87,6 +89,7 @@ envoy_extension_cc_test(
     extension_name = "envoy.filters.network.thrift_proxy",
     deps = [
         ":mocks",
+        ":utility_lib",
         "//source/extensions/filters/network/thrift_proxy:decoder_lib",
         "//test/test_common:printers_lib",
         "//test/test_common:utility_lib",
@@ -131,3 +134,21 @@ envoy_extension_cc_test(
         "//test/test_common:utility_lib",
     ],
 )
+
+envoy_extension_cc_test(
+    name = "filter_integration_test",
+    srcs = ["filter_integration_test.cc"],
+    data = [
+        "//test/extensions/filters/network/thrift_proxy/driver:generate_fixture",
+    ],
+    extension_name = "envoy.filters.network.thrift_proxy",
+    deps = [
+        "//source/extensions/filters/network/tcp_proxy:config",
+        "//source/extensions/filters/network/thrift_proxy:config",
+        "//source/extensions/filters/network/thrift_proxy:filter_lib",
+        "//test/integration:integration_lib",
+        "//test/test_common:environment_lib",
+        "//test/test_common:network_utility_lib",
+        "//test/test_common:printers_lib",
+    ],
+)

test/extensions/filters/network/thrift_proxy/binary_protocol_test.cc

@@ -224,7 +224,7 @@ TEST(BinaryProtocolTest, ReadFieldBegin) {
     EXPECT_EQ(field_id, 1);
   }
 
-  // Non-terminal field
+  // Non-stop field
   {
     Buffer::OwnedImpl buffer;
     std::string name = "-";
@@ -241,6 +241,24 @@ TEST(BinaryProtocolTest, ReadFieldBegin) {
     EXPECT_EQ(field_id, 99);
     EXPECT_EQ(buffer.length(), 0);
   }
+
+  // field id < 0
+  {
+    Buffer::OwnedImpl buffer;
+    std::string name = "-";
+    FieldType field_type = FieldType::String;
+    int16_t field_id = 1;
+
+    addInt8(buffer, FieldType::I32);
+    addInt16(buffer, -1);
+
+    EXPECT_THROW_WITH_MESSAGE(proto.readFieldBegin(buffer, name, field_type, field_id),
+                              EnvoyException, "invalid binary protocol field id -1");
+    EXPECT_EQ(name, "-");
+    EXPECT_EQ(field_type, FieldType::String);
+    EXPECT_EQ(field_id, 1);
+    EXPECT_EQ(buffer.length(), 3);
+  }
 }
 
 TEST(BinaryProtocolTest, ReadFieldEnd) {

test/extensions/filters/network/thrift_proxy/compact_protocol_test.cc

@@ -336,24 +336,42 @@ TEST(CompactProtocolTest, ReadFieldBegin) {
     EXPECT_EQ(buffer.length(), 6);
   }
 
-  // Long-form field header, field id out of range
+  // Long-form field header, field id > 32767
   {
     Buffer::OwnedImpl buffer;
     std::string name = "-";
     FieldType field_type = FieldType::String;
     int16_t field_id = 1;
 
     addInt8(buffer, 0x05);
-    addSeq(buffer, {0xFE, 0xFF, 0x7F}); // zigzag(0x1FFFFE) = 0xFFFFF
+    addSeq(buffer, {0x80, 0x80, 0x04}); // zigzag(0x10000) = 0x8000
 
     EXPECT_THROW_WITH_MESSAGE(proto.readFieldBegin(buffer, name, field_type, field_id),
-                              EnvoyException, "invalid compact protocol field id 1048575");
+                              EnvoyException, "invalid compact protocol field id 32768");
     EXPECT_EQ(name, "-");
     EXPECT_EQ(field_type, FieldType::String);
     EXPECT_EQ(field_id, 1);
     EXPECT_EQ(buffer.length(), 4);
   }
 
+  // Long-form field header, field id < 0
+  {
+    Buffer::OwnedImpl buffer;
+    std::string name = "-";
+    FieldType field_type = FieldType::String;
+    int16_t field_id = 1;
+
+    addInt8(buffer, 0x05);
+    addSeq(buffer, {0x01}); // zigzag(1) = -1
+
+    EXPECT_THROW_WITH_MESSAGE(proto.readFieldBegin(buffer, name, field_type, field_id),
+                              EnvoyException, "invalid compact protocol field id -1");
+    EXPECT_EQ(name, "-");
+    EXPECT_EQ(field_type, FieldType::String);
+    EXPECT_EQ(field_id, 1);
+    EXPECT_EQ(buffer.length(), 2);
+  }
+
   // Unknown compact protocol field type
   {
     Buffer::OwnedImpl buffer;
@@ -962,7 +980,7 @@ TEST(CompactProtocolTest, ReadString) {
     Buffer::OwnedImpl buffer;
     std::string value = "-";
 
-    addInt8(buffer, 0x8); // zigzag(8) = 4
+    addInt8(buffer, 0x4);
 
     EXPECT_FALSE(proto.readString(buffer, value));
     EXPECT_EQ(value, "-");
@@ -974,12 +992,12 @@ TEST(CompactProtocolTest, ReadString) {
     Buffer::OwnedImpl buffer;
     std::string value = "-";
 
-    addInt8(buffer, 0x01); // zigzag(1) = -1
+    addSeq(buffer, {0xFF, 0xFF, 0xFF, 0xFF, 0x1F}); // -1
 
     EXPECT_THROW_WITH_MESSAGE(proto.readString(buffer, value), EnvoyException,
                               "negative compact protocol string/binary length -1");
     EXPECT_EQ(value, "-");
-    EXPECT_EQ(buffer.length(), 1);
+    EXPECT_EQ(buffer.length(), 5);
   }
 
   // empty string
@@ -999,7 +1017,7 @@ TEST(CompactProtocolTest, ReadString) {
     Buffer::OwnedImpl buffer;
     std::string value = "-";
 
-    addInt8(buffer, 0x0C); // zigzag(0x0C) = 0x06
+    addInt8(buffer, 0x06);
     addString(buffer, "string");
 
     EXPECT_TRUE(proto.readString(buffer, value));
@@ -1015,7 +1033,7 @@ TEST(CompactProtocolTest, ReadBinary) {
   Buffer::OwnedImpl buffer;
   std::string value = "-";
 
-  addInt8(buffer, 0x0C); // zigzag(0x0C) = 0x06
+  addInt8(buffer, 0x06);
   addString(buffer, "string");
 
   EXPECT_TRUE(proto.readBinary(buffer, value));