Skip to content

grpc access log: verify cluster exists and is static #2360

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
htuch merged 2 commits into from
Jan 14, 2018
Merged

grpc access log: verify cluster exists and is static #2360

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
df0e14d
grpc access log: verify cluster exists and is static
mattklein123 Jan 12, 2018
fb354f0
fix
mattklein123 Jan 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 11 additions & 5 deletions source/server/config/access_log/grpc_access_log.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,14 +41,20 @@ AccessLog::InstanceSharedPtr HttpGrpcAccessLogFactory::createAccessLogInstance(
const auto& proto_config = MessageUtil::downcastAndValidate<
const envoy::api::v2::filter::accesslog::HttpGrpcAccessLogConfig&>(config);

// TODO(htuch): Support Google gRPC client.
const auto cluster_name = proto_config.common_config().grpc_service().envoy_grpc().cluster_name();
auto cluster = context.clusterManager().get(cluster_name);
if (cluster == nullptr || cluster->info()->addedViaApi()) {
Copy link
Copy Markdown

@lita lita Jan 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the difference between a static cluster vs a cluster added by api?

Copy link
Copy Markdown
Contributor

@ccaraman ccaraman Jan 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Static cluster is one defined in the config. A cluster added by api would be a cluster added via CDS.

throw EnvoyException(fmt::format(
"invalid access log cluster '{}'. Missing or not a static cluster.", cluster_name));
}

std::shared_ptr<AccessLog::GrpcAccessLogStreamer> grpc_access_log_streamer =
context.singletonManager().getTyped<AccessLog::GrpcAccessLogStreamer>(
SINGLETON_MANAGER_REGISTERED_NAME(grpc_access_log_streamer), [&context, &proto_config] {
SINGLETON_MANAGER_REGISTERED_NAME(grpc_access_log_streamer), [&context, cluster_name] {
return std::make_shared<AccessLog::GrpcAccessLogStreamerImpl>(
std::make_unique<GrpcAccessLogClientFactoryImpl>(
context.clusterManager(),
// TODO(htuch): Support Google gRPC client.
proto_config.common_config().grpc_service().envoy_grpc().cluster_name()),
std::make_unique<GrpcAccessLogClientFactoryImpl>(context.clusterManager(),
cluster_name),
context.threadLocal(), context.localInfo());
});

Expand Down
55 changes: 41 additions & 14 deletions test/server/config/access_log/config_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,33 +9,60 @@
#include "gmock/gmock.h"
#include "gtest/gtest.h"

using testing::Return;

namespace Envoy {
namespace Server {
namespace Configuration {

TEST(AccessLogConfigTest, HttpGrpcAccessLogTest) {
auto factory = Registry::FactoryRegistry<AccessLogInstanceFactory>::getFactory(
Config::AccessLogNames::get().HTTP_GRPC);
ASSERT_NE(nullptr, factory);
class HttpGrpcAccessLogConfigTest : public testing::Test {
public:
void SetUp() override {
factory_ = Registry::FactoryRegistry<AccessLogInstanceFactory>::getFactory(
Config::AccessLogNames::get().HTTP_GRPC);
ASSERT_NE(nullptr, factory_);

ProtobufTypes::MessagePtr message = factory->createEmptyConfigProto();
ASSERT_NE(nullptr, message);
message_ = factory_->createEmptyConfigProto();
ASSERT_NE(nullptr, message_);

envoy::api::v2::filter::accesslog::HttpGrpcAccessLogConfig http_grpc_access_log;
auto* common_config = http_grpc_access_log.mutable_common_config();
common_config->set_log_name("foo");
common_config->mutable_grpc_service()->mutable_envoy_grpc()->set_cluster_name("bar");
MessageUtil::jsonConvert(http_grpc_access_log, *message);
auto* common_config = http_grpc_access_log_.mutable_common_config();
common_config->set_log_name("foo");
common_config->mutable_grpc_service()->mutable_envoy_grpc()->set_cluster_name("bar");
MessageUtil::jsonConvert(http_grpc_access_log_, *message_);
}

AccessLog::FilterPtr filter;
NiceMock<Server::Configuration::MockFactoryContext> context;
AccessLog::FilterPtr filter_;
NiceMock<Server::Configuration::MockFactoryContext> context_;
envoy::api::v2::filter::accesslog::HttpGrpcAccessLogConfig http_grpc_access_log_;
ProtobufTypes::MessagePtr message_;
AccessLogInstanceFactory* factory_{};
};

// Normal OK configuration.
TEST_F(HttpGrpcAccessLogConfigTest, Ok) {
AccessLog::InstanceSharedPtr instance =
factory->createAccessLogInstance(*message, std::move(filter), context);
factory_->createAccessLogInstance(*message_, std::move(filter_), context_);
EXPECT_NE(nullptr, instance);
EXPECT_NE(nullptr, dynamic_cast<AccessLog::HttpGrpcAccessLog*>(instance.get()));
}

// Configuration with no matching cluster.
TEST_F(HttpGrpcAccessLogConfigTest, NoCluster) {
ON_CALL(context_.cluster_manager_, get("bar")).WillByDefault(Return(nullptr));
EXPECT_THROW_WITH_MESSAGE(
factory_->createAccessLogInstance(*message_, std::move(filter_), context_), EnvoyException,
"invalid access log cluster 'bar'. Missing or not a static cluster.");
}

// Configuration with cluster but not a static cluster.
TEST_F(HttpGrpcAccessLogConfigTest, ClusterAddedViaApi) {
ON_CALL(*context_.cluster_manager_.thread_local_cluster_.cluster_.info_, addedViaApi())
.WillByDefault(Return(true));
EXPECT_THROW_WITH_MESSAGE(
factory_->createAccessLogInstance(*message_, std::move(filter_), context_), EnvoyException,
"invalid access log cluster 'bar'. Missing or not a static cluster.");
}

} // namespace Configuration
} // namespace Server
} // namespace Envoy