envoyproxy · yanavlasov · Dec 13, 2022 · Dec 5, 2022 · Dec 5, 2022 · Dec 5, 2022
@@ -35,10 +35,6 @@ absl::Status RateLimitClientImpl::startStream(const StreamInfo::StreamInfo& stre
         *this,
         Http::AsyncClient::RequestOptions().setParentContext(
             Http::AsyncClient::ParentContext{&stream_info}));
-
-    if (stream_ == nullptr) {
-      return absl::InternalError("Unable to establish the new stream");
-    }
   }
   return absl::OkStatus();
 }

@@ -30,10 +30,7 @@ class RateLimitClientImpl : public RateLimitClient,
   RateLimitClientImpl(const envoy::config::core::v3::GrpcService& grpc_service,
                       Server::Configuration::FactoryContext& context)
       : aync_client_(context.clusterManager().grpcAsyncClientManager().getOrCreateRawAsyncClient(
-            grpc_service, context.scope(), true)) {
-    // TODO(tyxia) startStream function is opened on the first request not at time when client is
-    // created here.
-  }
+            grpc_service, context.scope(), true)) {}
 
   void onReceiveMessage(RateLimitQuotaResponsePtr&& response) override;
 

@@ -24,14 +24,6 @@ Http::FilterFactoryCb RateLimitQuotaFilterFactory::createFilterFactoryFromProtoT
       };
 }
 
-Router::RouteSpecificFilterConfigConstSharedPtr
-RateLimitQuotaFilterFactory::createRouteSpecificFilterConfigTyped(
-    const envoy::extensions::filters::http::rate_limit_quota::v3::RateLimitQuotaOverride&,
-    Server::Configuration::ServerFactoryContext&, ProtobufMessage::ValidationVisitor&) {
-  // TODO(tyxia) Added per route config for override later.
-  return nullptr;
-}
-
 /**
  * Static registration for the filter. @see RegisterFactory.
  */

@@ -24,12 +24,6 @@ class RateLimitQuotaFilterFactory
       const envoy::extensions::filters::http::rate_limit_quota::v3::RateLimitQuotaFilterConfig&
           filter_config,
       const std::string& stats_prefix, Server::Configuration::FactoryContext& context) override;
-
-  Router::RouteSpecificFilterConfigConstSharedPtr createRouteSpecificFilterConfigTyped(
-      const envoy::extensions::filters::http::rate_limit_quota::v3::RateLimitQuotaOverride&
-          proto_config,
-      Server::Configuration::ServerFactoryContext& context,
-      ProtobufMessage::ValidationVisitor& validator) override;
 };
 
 } // namespace RateLimitQuota

@@ -24,20 +24,6 @@ Http::FilterHeadersStatus RateLimitQuotaFilter::decodeHeaders(Http::RequestHeade
     return Envoy::Http::FilterHeadersStatus::Continue;
   }
 
-  // Request is not matched by any matcher but the `on_no_match` field is configured. In this
-  // case, the request is matched to catch-all bucket and is DENIED by default.
-  // TODO(tyxia) Think about the way of representing DENIED and ALLOWED here.
-  if (match_result.value().bucket().empty()) {
-    return Envoy::Http::FilterHeadersStatus::Continue;
-  }
-
-  // Request has been matched and the corresponding bucket id has been generated successfully.
-  // Retrieve the quota assignment, if the entry with specific `bucket_id` is found.
-  if (quota_assignment_map_.find(match_result.value()) != quota_assignment_map_.end()) {
-  }
-  // Otherwise, send the request to RLQS server for the quota assignment and insert the bucket_id to
-  // the map.
-
   return Envoy::Http::FilterHeadersStatus::Continue;
 }
 
@@ -134,11 +120,7 @@ RateLimitOnMactchAction::generateBucketId(const Http::Matching::HttpMatchingData
         if (!result.data_.value().empty()) {
           // Build the bucket id from the matched result.
           bucket_id.mutable_bucket()->insert({bucket_id_key, result.data_.value()});
-        } else {
-          return absl::InternalError("Empty matched result.");
         }
-      } else {
-        return absl::InternalError("Failed to retrieve the result from custom value config.");
       }
       break;
     }

@@ -120,20 +120,6 @@ class RateLimitQuotaFilter : public Http::PassThroughFilter,
   RateLimitQuotaValidationVisitor visitor_ = {};
   Matcher::MatchTreeSharedPtr<Http::HttpMatchingData> matcher_ = nullptr;
   std::unique_ptr<Http::Matching::HttpMatchingDataImpl> data_ptr_ = nullptr;
-
-  // Customized hash and equal struct for `BucketId` hash key.
-  struct BucketIdHash {
-    size_t operator()(const BucketId& bucket_id) const { return MessageUtil::hash(bucket_id); }
-  };
-
-  struct BucketIdEqual {
-    bool operator()(const BucketId& id1, const BucketId& id2) const {
-      return Protobuf::util::MessageDifferencer::Equals(id1, id2);
-    }
-  };
-  // TODO(tyxia) Thread local storage.
-  absl::node_hash_map<BucketId, QuotaAssignmentAction, BucketIdHash, BucketIdEqual>
-      quota_assignment_map_;
 };
 
 } // namespace RateLimitQuota

@@ -111,6 +111,7 @@ TEST_F(RateLimitStreamTest, SendRequestAndReceiveResponse) {
   EXPECT_CALL(stream_, closeStream());
   EXPECT_CALL(stream_, resetStream());
   client_->closeStream();
+  client_->onRemoteClose(0, "");
 }
 
 } // namespace

@@ -125,6 +125,8 @@ class FilterTest : public testing::Test {
     TestUtility::loadFromYaml(GoogleGrpcConfig, config_);
   }
 
+  ~FilterTest() override { filter_->onDestroy(); }
+
   void addMatcherConfigAndCreateFilter(MatcherConfigType config_type) {
     // Add the matcher configuration.
     switch (config_type) {
@@ -150,10 +152,14 @@ class FilterTest : public testing::Test {
     default:
       break;
     }
+  }
 
+  void createFilter(bool set_callback = true) {
     filter_config_ = std::make_shared<FilterConfig>(config_);
     filter_ = std::make_unique<RateLimitQuotaFilter>(filter_config_, context_);
-    filter_->setDecoderFilterCallbacks(decoder_callbacks_);
+    if (set_callback) {
+      filter_->setDecoderFilterCallbacks(decoder_callbacks_);
+    }
   }
 
   void constructMismatchedRequestHeader() {
@@ -187,6 +193,7 @@ class FilterTest : public testing::Test {
 
 TEST_F(FilterTest, InvalidBucketMatcherConfig) {
   addMatcherConfigAndCreateFilter(MatcherConfigType::Invalid);
+  createFilter();
   auto match = filter_->requestMatching(default_headers_);
   EXPECT_FALSE(match.ok());
   EXPECT_THAT(match, StatusIs(absl::StatusCode::kInternal));
@@ -195,6 +202,7 @@ TEST_F(FilterTest, InvalidBucketMatcherConfig) {
 
 TEST_F(FilterTest, RequestMatchingSucceeded) {
   addMatcherConfigAndCreateFilter(MatcherConfigType::Valid);
+  createFilter();
   // Define the key value pairs that is used to build the bucket_id dynamically via `custom_value`
   // in the config.
   absl::flat_hash_map<std::string, std::string> custom_value_pairs = {{"environment", "staging"},
@@ -218,10 +226,14 @@ TEST_F(FilterTest, RequestMatchingSucceeded) {
       absl::flat_hash_map<std::string, std::string>(bucket_ids.begin(), bucket_ids.end());
   EXPECT_THAT(expected_bucket_ids,
               testing::UnorderedPointwise(testing::Eq(), serialized_bucket_ids));
+
+  envoy::service::rate_limit_quota::v3::RateLimitQuotaResponse resp;
+  filter_->onQuotaResponse(resp);
 }
 
 TEST_F(FilterTest, RequestMatchingFailed) {
   addMatcherConfigAndCreateFilter(MatcherConfigType::Valid);
+  createFilter();
   constructMismatchedRequestHeader();
 
   // Perform request matching.
@@ -232,8 +244,19 @@ TEST_F(FilterTest, RequestMatchingFailed) {
   EXPECT_EQ(match.status().message(), "The match was completed, no match found");
 }
 
+TEST_F(FilterTest, RequestMatchingFailedWithNoCallback) {
+  addMatcherConfigAndCreateFilter(MatcherConfigType::Valid);
+  createFilter(/*set_callback*/ false);
+
+  auto match = filter_->requestMatching(default_headers_);
+  EXPECT_FALSE(match.ok());
+  EXPECT_THAT(match, StatusIs(absl::StatusCode::kInternal));
+  EXPECT_EQ(match.status().message(), "Filter callback has not been initialized successfully yet.");
+}
+
 TEST_F(FilterTest, RequestMatchingFailedWithOnNoMatchConfigured) {
   addMatcherConfigAndCreateFilter(MatcherConfigType::IncludeOnNoMatchConfig);
+  createFilter();
   constructMismatchedRequestHeader();
 
   // Perform request matching.
@@ -247,6 +270,7 @@ TEST_F(FilterTest, RequestMatchingFailedWithOnNoMatchConfigured) {
 
 TEST_F(FilterTest, DecodeHeaderSucceeded) {
   addMatcherConfigAndCreateFilter(MatcherConfigType::Valid);
+  createFilter();
   // Define the key value pairs that is used to build the bucket_id dynamically via `custom_value`
   // in the config.
   absl::flat_hash_map<std::string, std::string> custom_value_pairs = {{"environment", "staging"},
@@ -256,6 +280,15 @@ TEST_F(FilterTest, DecodeHeaderSucceeded) {
   EXPECT_EQ(status, Envoy::Http::FilterHeadersStatus::Continue);
 }
 
+TEST_F(FilterTest, DecodeHeaderWithMismatchHeader) {
+  addMatcherConfigAndCreateFilter(MatcherConfigType::Valid);
+  createFilter();
+  constructMismatchedRequestHeader();
+
+  Http::FilterHeadersStatus status = filter_->decodeHeaders(default_headers_, false);
+  EXPECT_EQ(status, Envoy::Http::FilterHeadersStatus::Continue);
+}
+
 } // namespace
 } // namespace RateLimitQuota
 } // namespace HttpFilters

diff --git a/test/per_file_coverage.sh b/test/per_file_coverage.sh
@@ -43,7 +43,7 @@ declare -a KNOWN_LOW_COVERAGE=(
 "source/extensions/filters/http/kill_request:91.7" # Death tests don't report LCOV
 "source/extensions/filters/http/oauth2:96.1"
 "source/extensions/filters/http/wasm:1.9"
-"source/extensions/filters/http/rate_limit_quota:70" # TODO(tyxia) WIP. Improve coverage as development continues
+"source/extensions/filters/http/rate_limit_quota:94" # TODO(tyxia) WIP. Improve coverage as development continues
 "source/extensions/filters/listener/original_dst:82.9"
 "source/extensions/filters/listener/original_src:92.1"
 "source/extensions/filters/network/common:96.1"

diff --git a/test/run_envoy_bazel_coverage.sh b/test/run_envoy_bazel_coverage.sh
@@ -28,7 +28,7 @@ fi
 [[ -z "${SRCDIR}" ]] && SRCDIR="${PWD}"
 [[ -z "${VALIDATE_COVERAGE}" ]] && VALIDATE_COVERAGE=true
 [[ -z "${FUZZ_COVERAGE}" ]] && FUZZ_COVERAGE=false
-[[ -z "${COVERAGE_THRESHOLD}" ]] && COVERAGE_THRESHOLD=96.0
+[[ -z "${COVERAGE_THRESHOLD}" ]] && COVERAGE_THRESHOLD=96.1
 COVERAGE_TARGET="${COVERAGE_TARGET:-}"
 read -ra BAZEL_BUILD_OPTIONS <<< "${BAZEL_BUILD_OPTIONS:-}"