From 68b491eb621e39239f2d4bb8a1e23d4d999fa93a Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Wed, 4 Sep 2019 09:12:56 +0700 Subject: [PATCH 1/4] Add cluster existence check Signed-off-by: Dhi Aurrahman --- .../common/ext_authz/ext_authz_http_impl.cc | 17 +++++++++-- .../ext_authz/ext_authz_http_impl_test.cc | 30 +++++++++++++++---- 2 files changed, 38 insertions(+), 9 deletions(-) diff --git a/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc b/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc index b40a927fdbfb..43f5346257d9 100644 --- a/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc +++ b/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc @@ -202,9 +202,20 @@ void RawHttpClientImpl::check(RequestCallbacks& callbacks, std::make_unique(request.attributes().request().http().body()); } - request_ = cm_.httpAsyncClientForCluster(config_->cluster()) - .send(std::move(message), *this, - Http::AsyncClient::RequestOptions().setTimeout(config_->timeout())); + const std::string& cluster = config_->cluster(); + + // It's possible that the cluster specified in the filter configuration no longer exists due to a + // CDS removal. + if (!cm_.get(cluster)) { + // TODO(dio): Add stats related to this. + ENVOY_LOG(debug, "ext_authz cluster '{}' does not exist", cluster); + callbacks_->onComplete(std::make_unique(errorResponse())); + callbacks_ = nullptr; + } else { + request_ = cm_.httpAsyncClientForCluster(cluster).send( + std::move(message), *this, + Http::AsyncClient::RequestOptions().setTimeout(config_->timeout())); + } } void RawHttpClientImpl::onSuccess(Http::MessagePtr&& message) { diff --git a/test/extensions/filters/common/ext_authz/ext_authz_http_impl_test.cc b/test/extensions/filters/common/ext_authz/ext_authz_http_impl_test.cc index 62e89216af2b..2f21d7254368 100644 --- a/test/extensions/filters/common/ext_authz/ext_authz_http_impl_test.cc +++ b/test/extensions/filters/common/ext_authz/ext_authz_http_impl_test.cc @@ -16,6 +16,8 @@ using testing::_; using testing::AllOf; +using testing::Eq; +using testing::InSequence; using testing::Invoke; using testing::Ref; using testing::Return; @@ -31,13 +33,10 @@ namespace Common { namespace ExtAuthz { namespace { -class ExtAuthzHttpClientTest : public testing::Test { +class ExtAuthzHttpTest : public testing::Test { public: - ExtAuthzHttpClientTest() - : async_request_{&async_client_}, config_{createConfig()}, client_{cm_, config_} { - ON_CALL(cm_, httpAsyncClientForCluster(config_->cluster())) - .WillByDefault(ReturnRef(async_client_)); - } + ExtAuthzHttpTest() + : async_request_{&async_client_}, config_{createConfig()}, client_{cm_, config_} {} static ClientConfigSharedPtr createConfig(std::string yaml = "", uint32_t timeout = 250, std::string path_prefix = "/bar") { @@ -116,6 +115,14 @@ class ExtAuthzHttpClientTest : public testing::Test { MockRequestCallbacks request_callbacks_; }; +class ExtAuthzHttpClientTest : public ExtAuthzHttpTest { +public: + ExtAuthzHttpClientTest() { + ON_CALL(cm_, httpAsyncClientForCluster(config_->cluster())) + .WillByDefault(ReturnRef(async_client_)); + } +}; + // Test HTTP client config default values. TEST_F(ExtAuthzHttpClientTest, ClientConfig) { const Http::LowerCaseString foo{"foo"}; @@ -377,6 +384,17 @@ TEST_F(ExtAuthzHttpClientTest, CancelledAuthorizationRequest) { client_.cancel(); } +TEST_F(ExtAuthzHttpClientTest, NoCluster) { + InSequence s; + + EXPECT_CALL(cm_, get(Eq("ext_authz"))).WillOnce(Return(nullptr)); + EXPECT_CALL(cm_, httpAsyncClientForCluster("ext_authz")).Times(0); + EXPECT_CALL(request_callbacks_, + onComplete_(WhenDynamicCastTo(AuthzErrorResponse(CheckStatus::Error)))); + client_.check(request_callbacks_, envoy::service::auth::v2::CheckRequest{}, + Tracing::NullSpan::instance()); +} + } // namespace } // namespace ExtAuthz } // namespace Common From eaa449d274d7ceddfebbb0d83c9cfc115b9a0b4c Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Wed, 4 Sep 2019 09:44:38 +0700 Subject: [PATCH 2/4] Fix Signed-off-by: Dhi Aurrahman --- .../ext_authz/ext_authz_http_impl_test.cc | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/test/extensions/filters/common/ext_authz/ext_authz_http_impl_test.cc b/test/extensions/filters/common/ext_authz/ext_authz_http_impl_test.cc index 2f21d7254368..793b9fbe4014 100644 --- a/test/extensions/filters/common/ext_authz/ext_authz_http_impl_test.cc +++ b/test/extensions/filters/common/ext_authz/ext_authz_http_impl_test.cc @@ -33,10 +33,13 @@ namespace Common { namespace ExtAuthz { namespace { -class ExtAuthzHttpTest : public testing::Test { +class ExtAuthzHttpClientTest : public testing::Test { public: - ExtAuthzHttpTest() - : async_request_{&async_client_}, config_{createConfig()}, client_{cm_, config_} {} + ExtAuthzHttpClientTest() + : async_request_{&async_client_}, config_{createConfig()}, client_{cm_, config_} { + ON_CALL(cm_, httpAsyncClientForCluster(config_->cluster())) + .WillByDefault(ReturnRef(async_client_)); + } static ClientConfigSharedPtr createConfig(std::string yaml = "", uint32_t timeout = 250, std::string path_prefix = "/bar") { @@ -115,14 +118,6 @@ class ExtAuthzHttpTest : public testing::Test { MockRequestCallbacks request_callbacks_; }; -class ExtAuthzHttpClientTest : public ExtAuthzHttpTest { -public: - ExtAuthzHttpClientTest() { - ON_CALL(cm_, httpAsyncClientForCluster(config_->cluster())) - .WillByDefault(ReturnRef(async_client_)); - } -}; - // Test HTTP client config default values. TEST_F(ExtAuthzHttpClientTest, ClientConfig) { const Http::LowerCaseString foo{"foo"}; From 2509db9aecb8b9dffe779a7f6a95441178599601 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Wed, 4 Sep 2019 11:09:55 +0700 Subject: [PATCH 3/4] Comments Signed-off-by: Dhi Aurrahman --- .../extensions/filters/common/ext_authz/ext_authz_http_impl.cc | 2 +- .../filters/common/ext_authz/ext_authz_http_impl_test.cc | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc b/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc index 43f5346257d9..ad9eba0ab0e7 100644 --- a/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc +++ b/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc @@ -207,7 +207,7 @@ void RawHttpClientImpl::check(RequestCallbacks& callbacks, // It's possible that the cluster specified in the filter configuration no longer exists due to a // CDS removal. if (!cm_.get(cluster)) { - // TODO(dio): Add stats related to this. + // TODO(dio): Add stats and tracing related to this. ENVOY_LOG(debug, "ext_authz cluster '{}' does not exist", cluster); callbacks_->onComplete(std::make_unique(errorResponse())); callbacks_ = nullptr; diff --git a/test/extensions/filters/common/ext_authz/ext_authz_http_impl_test.cc b/test/extensions/filters/common/ext_authz/ext_authz_http_impl_test.cc index 793b9fbe4014..1987b1bca1e9 100644 --- a/test/extensions/filters/common/ext_authz/ext_authz_http_impl_test.cc +++ b/test/extensions/filters/common/ext_authz/ext_authz_http_impl_test.cc @@ -379,6 +379,7 @@ TEST_F(ExtAuthzHttpClientTest, CancelledAuthorizationRequest) { client_.cancel(); } +// Test the client when the configured cluster is missing/removed. TEST_F(ExtAuthzHttpClientTest, NoCluster) { InSequence s; From 4927674f3cd63007fbe051a6f24b2451bf2209b9 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Wed, 4 Sep 2019 23:49:09 +0700 Subject: [PATCH 4/4] Explicit nullptr comparison Signed-off-by: Dhi Aurrahman --- .../extensions/filters/common/ext_authz/ext_authz_http_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc b/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc index ad9eba0ab0e7..b49960bf511a 100644 --- a/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc +++ b/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc @@ -206,7 +206,7 @@ void RawHttpClientImpl::check(RequestCallbacks& callbacks, // It's possible that the cluster specified in the filter configuration no longer exists due to a // CDS removal. - if (!cm_.get(cluster)) { + if (cm_.get(cluster) == nullptr) { // TODO(dio): Add stats and tracing related to this. ENVOY_LOG(debug, "ext_authz cluster '{}' does not exist", cluster); callbacks_->onComplete(std::make_unique(errorResponse()));