From 1a739405311690d054d176f6d965873914dae88d Mon Sep 17 00:00:00 2001 From: Lizan Zhou Date: Wed, 21 Aug 2019 14:48:53 -0700 Subject: [PATCH 1/4] accesslog: don't open log file with read flag Signed-off-by: Lizan Zhou --- source/common/access_log/access_log_manager_impl.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/source/common/access_log/access_log_manager_impl.cc b/source/common/access_log/access_log_manager_impl.cc index dd2615421db1..cf5a98daaf47 100644 --- a/source/common/access_log/access_log_manager_impl.cc +++ b/source/common/access_log/access_log_manager_impl.cc @@ -42,9 +42,9 @@ AccessLogFileImpl::AccessLogFileImpl(Filesystem::FilePtr&& file, Event::Dispatch } Filesystem::FlagSet AccessLogFileImpl::defaultFlags() { - static constexpr Filesystem::FlagSet default_flags{ - 1 << Filesystem::File::Operation::Read | 1 << Filesystem::File::Operation::Write | - 1 << Filesystem::File::Operation::Create | 1 << Filesystem::File::Operation::Append}; + static constexpr Filesystem::FlagSet default_flags{1 << Filesystem::File::Operation::Write | + 1 << Filesystem::File::Operation::Create | + 1 << Filesystem::File::Operation::Append}; return default_flags; } From 0a6c95867df8b312f31ac2d44b992f822393cde8 Mon Sep 17 00:00:00 2001 From: Lizan Zhou Date: Thu, 22 Aug 2019 04:19:12 +0000 Subject: [PATCH 2/4] unit test Signed-off-by: Lizan Zhou --- .../access_log_manager_impl_test.cc | 39 +++++++++++++------ test/extensions/access_loggers/file/BUILD | 1 + .../access_loggers/file/config_test.cc | 31 +++++++++++++++ test/mocks/filesystem/mocks.cc | 4 +- test/mocks/filesystem/mocks.h | 2 +- 5 files changed, 62 insertions(+), 15 deletions(-) diff --git a/test/common/access_log/access_log_manager_impl_test.cc b/test/common/access_log/access_log_manager_impl_test.cc index 151ac7544b47..8c73b0cb1f8d 100644 --- a/test/common/access_log/access_log_manager_impl_test.cc +++ b/test/common/access_log/access_log_manager_impl_test.cc @@ -14,6 +14,7 @@ using testing::_; using testing::ByMove; +using testing::Invoke; using testing::NiceMock; using testing::Return; using testing::ReturnNew; @@ -49,14 +50,28 @@ class AccessLogManagerImplTest : public testing::Test { TEST_F(AccessLogManagerImplTest, BadFile) { EXPECT_CALL(dispatcher_, createTimer_(_)); - EXPECT_CALL(*file_, open_()).WillOnce(Return(ByMove(Filesystem::resultFailure(false, 0)))); + EXPECT_CALL(*file_, open_(_)).WillOnce(Return(ByMove(Filesystem::resultFailure(false, 0)))); EXPECT_THROW(access_log_manager_.createAccessLog("foo"), EnvoyException); } +TEST_F(AccessLogManagerImplTest, OpenFileWithRightFlags) { + EXPECT_CALL(dispatcher_, createTimer_(_)); + EXPECT_CALL(*file_, open_(_)) + .WillOnce(Invoke([](Filesystem::FlagSet flags) -> Api::IoCallBoolResult { + EXPECT_FALSE(flags[Filesystem::File::Operation::Read]); + EXPECT_TRUE(flags[Filesystem::File::Operation::Write]); + EXPECT_TRUE(flags[Filesystem::File::Operation::Append]); + EXPECT_TRUE(flags[Filesystem::File::Operation::Create]); + return Filesystem::resultSuccess(true); + })); + EXPECT_NE(nullptr, access_log_manager_.createAccessLog("foo")); + EXPECT_CALL(*file_, close_()).WillOnce(Return(ByMove(Filesystem::resultSuccess(true)))); +} + TEST_F(AccessLogManagerImplTest, flushToLogFilePeriodically) { NiceMock* timer = new NiceMock(&dispatcher_); - EXPECT_CALL(*file_, open_()).WillOnce(Return(ByMove(Filesystem::resultSuccess(true)))); + EXPECT_CALL(*file_, open_(_)).WillOnce(Return(ByMove(Filesystem::resultSuccess(true)))); AccessLogFileSharedPtr log_file = access_log_manager_.createAccessLog("foo"); EXPECT_CALL(*timer, enableTimer(timeout_40ms_)); @@ -98,7 +113,7 @@ TEST_F(AccessLogManagerImplTest, flushToLogFilePeriodically) { TEST_F(AccessLogManagerImplTest, flushToLogFileOnDemand) { NiceMock* timer = new NiceMock(&dispatcher_); - EXPECT_CALL(*file_, open_()).WillOnce(Return(ByMove(Filesystem::resultSuccess(true)))); + EXPECT_CALL(*file_, open_(_)).WillOnce(Return(ByMove(Filesystem::resultSuccess(true)))); AccessLogFileSharedPtr log_file = access_log_manager_.createAccessLog("foo"); EXPECT_CALL(*timer, enableTimer(timeout_40ms_)); @@ -163,7 +178,7 @@ TEST_F(AccessLogManagerImplTest, reopenFile) { NiceMock* timer = new NiceMock(&dispatcher_); Sequence sq; - EXPECT_CALL(*file_, open_()) + EXPECT_CALL(*file_, open_(_)) .InSequence(sq) .WillOnce(Return(ByMove(Filesystem::resultSuccess(true)))); AccessLogFileSharedPtr log_file = access_log_manager_.createAccessLog("foo"); @@ -188,7 +203,7 @@ TEST_F(AccessLogManagerImplTest, reopenFile) { EXPECT_CALL(*file_, close_()) .InSequence(sq) .WillOnce(Return(ByMove(Filesystem::resultSuccess(true)))); - EXPECT_CALL(*file_, open_()) + EXPECT_CALL(*file_, open_(_)) .InSequence(sq) .WillOnce(Return(ByMove(Filesystem::resultSuccess(true)))); @@ -224,7 +239,7 @@ TEST_F(AccessLogManagerImplTest, reopenThrows) { })); Sequence sq; - EXPECT_CALL(*file_, open_()) + EXPECT_CALL(*file_, open_(_)) .InSequence(sq) .WillOnce(Return(ByMove(Filesystem::resultSuccess(true)))); @@ -232,7 +247,7 @@ TEST_F(AccessLogManagerImplTest, reopenThrows) { EXPECT_CALL(*file_, close_()) .InSequence(sq) .WillOnce(Return(ByMove(Filesystem::resultSuccess(true)))); - EXPECT_CALL(*file_, open_()) + EXPECT_CALL(*file_, open_(_)) .InSequence(sq) .WillOnce(Return(ByMove(Filesystem::resultFailure(false, 0)))); @@ -262,7 +277,7 @@ TEST_F(AccessLogManagerImplTest, reopenThrows) { } TEST_F(AccessLogManagerImplTest, bigDataChunkShouldBeFlushedWithoutTimer) { - EXPECT_CALL(*file_, open_()).WillOnce(Return(ByMove(Filesystem::resultSuccess(true)))); + EXPECT_CALL(*file_, open_(_)).WillOnce(Return(ByMove(Filesystem::resultSuccess(true)))); AccessLogFileSharedPtr log_file = access_log_manager_.createAccessLog("foo"); EXPECT_CALL(*file_, write_(_)) @@ -305,7 +320,7 @@ TEST_F(AccessLogManagerImplTest, reopenAllFiles) { EXPECT_CALL(dispatcher_, createTimer_(_)).WillRepeatedly(ReturnNew>()); Sequence sq; - EXPECT_CALL(*file_, open_()) + EXPECT_CALL(*file_, open_(_)) .InSequence(sq) .WillOnce(Return(ByMove(Filesystem::resultSuccess(true)))); AccessLogFileSharedPtr log = access_log_manager_.createAccessLog("foo"); @@ -315,7 +330,7 @@ TEST_F(AccessLogManagerImplTest, reopenAllFiles) { .WillOnce(Return(ByMove(std::unique_ptr>(file2)))); Sequence sq2; - EXPECT_CALL(*file2, open_()) + EXPECT_CALL(*file2, open_(_)) .InSequence(sq2) .WillOnce(Return(ByMove(Filesystem::resultSuccess(true)))); AccessLogFileSharedPtr log2 = access_log_manager_.createAccessLog("bar"); @@ -342,10 +357,10 @@ TEST_F(AccessLogManagerImplTest, reopenAllFiles) { .InSequence(sq2) .WillOnce(Return(ByMove(Filesystem::resultSuccess(true)))); - EXPECT_CALL(*file_, open_()) + EXPECT_CALL(*file_, open_(_)) .InSequence(sq) .WillOnce(Return(ByMove(Filesystem::resultSuccess(true)))); - EXPECT_CALL(*file2, open_()) + EXPECT_CALL(*file2, open_(_)) .InSequence(sq2) .WillOnce(Return(ByMove(Filesystem::resultSuccess(true)))); diff --git a/test/extensions/access_loggers/file/BUILD b/test/extensions/access_loggers/file/BUILD index 3feb9c7509a6..461c4d5bdaea 100644 --- a/test/extensions/access_loggers/file/BUILD +++ b/test/extensions/access_loggers/file/BUILD @@ -18,5 +18,6 @@ envoy_extension_cc_test( deps = [ "//source/extensions/access_loggers/file:config", "//test/mocks/server:server_mocks", + "//test/test_common:environment_lib", ], ) diff --git a/test/extensions/access_loggers/file/config_test.cc b/test/extensions/access_loggers/file/config_test.cc index a44146beb3b5..c78aa3977ef8 100644 --- a/test/extensions/access_loggers/file/config_test.cc +++ b/test/extensions/access_loggers/file/config_test.cc @@ -9,6 +9,7 @@ #include "extensions/access_loggers/well_known_names.h" #include "test/mocks/server/mocks.h" +#include "test/test_common/environment.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -19,6 +20,8 @@ namespace AccessLoggers { namespace File { namespace { +using testing::ReturnRef; + TEST(FileAccessLogConfigTest, ValidateFail) { NiceMock context; @@ -156,6 +159,34 @@ TEST(FileAccessLogConfigTest, FileAccessLogJsonWithNestedKeyTest) { } } +TEST(FileAccessLogConfigTest, FileAccessPermissionTest) { + auto factory = + Registry::FactoryRegistry::getFactory( + AccessLogNames::get().File); + ASSERT_NE(nullptr, factory); + + ProtobufTypes::MessagePtr message = factory->createEmptyConfigProto(); + ASSERT_NE(nullptr, message); + + std::string log_file = + absl::StrCat(TestEnvironment::temporaryDirectory(), "/log_file_permission_test.txt"); + + TestEnvironment::exec({"touch", log_file}); + TestEnvironment::exec({"chmod", "000", log_file}); + + auto& fs = Filesystem::fileSystemForTest(); + auto& file_access_log = dynamic_cast(*message); + file_access_log.set_path("log_file"); + file_access_log.set_format("%START_TIME%"); + + NiceMock context; + ON_CALL(context.api_, fileSystem()).WillByDefault(ReturnRef(fs)); + AccessLog::InstanceSharedPtr instance = + factory->createAccessLogInstance(*message, nullptr, context); + EXPECT_NE(nullptr, instance); + EXPECT_NE(nullptr, dynamic_cast(instance.get())); +} + } // namespace } // namespace File } // namespace AccessLoggers diff --git a/test/mocks/filesystem/mocks.cc b/test/mocks/filesystem/mocks.cc index 7cf733315030..af5d9fcddc7c 100644 --- a/test/mocks/filesystem/mocks.cc +++ b/test/mocks/filesystem/mocks.cc @@ -9,10 +9,10 @@ namespace Filesystem { MockFile::MockFile() : num_opens_(0), num_writes_(0), is_open_(false) {} MockFile::~MockFile() = default; -Api::IoCallBoolResult MockFile::open(FlagSet) { +Api::IoCallBoolResult MockFile::open(FlagSet flag) { Thread::LockGuard lock(open_mutex_); - Api::IoCallBoolResult result = open_(); + Api::IoCallBoolResult result = open_(flag); is_open_ = result.rc_; num_opens_++; open_event_.notifyOne(); diff --git a/test/mocks/filesystem/mocks.h b/test/mocks/filesystem/mocks.h index 459cdfd8ce65..ab7b9c70cecb 100644 --- a/test/mocks/filesystem/mocks.h +++ b/test/mocks/filesystem/mocks.h @@ -25,7 +25,7 @@ class MockFile : public File { bool isOpen() const override { return is_open_; }; MOCK_CONST_METHOD0(path, std::string()); - MOCK_METHOD0(open_, Api::IoCallBoolResult()); + MOCK_METHOD1(open_, Api::IoCallBoolResult(FlagSet flag)); MOCK_METHOD1(write_, Api::IoCallSizeResult(absl::string_view buffer)); MOCK_METHOD0(close_, Api::IoCallBoolResult()); From 75892e238d645b909640902c1266b29e7270f58f Mon Sep 17 00:00:00 2001 From: Lizan Zhou Date: Thu, 22 Aug 2019 05:46:23 +0000 Subject: [PATCH 3/4] massage libstdc++ Signed-off-by: Lizan Zhou --- test/mocks/filesystem/mocks.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/mocks/filesystem/mocks.h b/test/mocks/filesystem/mocks.h index ab7b9c70cecb..c4ae006a83ad 100644 --- a/test/mocks/filesystem/mocks.h +++ b/test/mocks/filesystem/mocks.h @@ -25,7 +25,8 @@ class MockFile : public File { bool isOpen() const override { return is_open_; }; MOCK_CONST_METHOD0(path, std::string()); - MOCK_METHOD1(open_, Api::IoCallBoolResult(FlagSet flag)); + // The first parameter here must be `const FlagSet&` otherwise it doesn't compile with libstdc++ + MOCK_METHOD1(open_, Api::IoCallBoolResult(const FlagSet& flag)); MOCK_METHOD1(write_, Api::IoCallSizeResult(absl::string_view buffer)); MOCK_METHOD0(close_, Api::IoCallBoolResult()); From ece4bdfb3472f598bd0e3b7aff812dd1acca52a1 Mon Sep 17 00:00:00 2001 From: Lizan Zhou Date: Thu, 22 Aug 2019 06:06:04 +0000 Subject: [PATCH 4/4] revert Signed-off-by: Lizan Zhou --- .../access_loggers/file/config_test.cc | 31 ------------------- 1 file changed, 31 deletions(-) diff --git a/test/extensions/access_loggers/file/config_test.cc b/test/extensions/access_loggers/file/config_test.cc index c78aa3977ef8..a44146beb3b5 100644 --- a/test/extensions/access_loggers/file/config_test.cc +++ b/test/extensions/access_loggers/file/config_test.cc @@ -9,7 +9,6 @@ #include "extensions/access_loggers/well_known_names.h" #include "test/mocks/server/mocks.h" -#include "test/test_common/environment.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -20,8 +19,6 @@ namespace AccessLoggers { namespace File { namespace { -using testing::ReturnRef; - TEST(FileAccessLogConfigTest, ValidateFail) { NiceMock context; @@ -159,34 +156,6 @@ TEST(FileAccessLogConfigTest, FileAccessLogJsonWithNestedKeyTest) { } } -TEST(FileAccessLogConfigTest, FileAccessPermissionTest) { - auto factory = - Registry::FactoryRegistry::getFactory( - AccessLogNames::get().File); - ASSERT_NE(nullptr, factory); - - ProtobufTypes::MessagePtr message = factory->createEmptyConfigProto(); - ASSERT_NE(nullptr, message); - - std::string log_file = - absl::StrCat(TestEnvironment::temporaryDirectory(), "/log_file_permission_test.txt"); - - TestEnvironment::exec({"touch", log_file}); - TestEnvironment::exec({"chmod", "000", log_file}); - - auto& fs = Filesystem::fileSystemForTest(); - auto& file_access_log = dynamic_cast(*message); - file_access_log.set_path("log_file"); - file_access_log.set_format("%START_TIME%"); - - NiceMock context; - ON_CALL(context.api_, fileSystem()).WillByDefault(ReturnRef(fs)); - AccessLog::InstanceSharedPtr instance = - factory->createAccessLogInstance(*message, nullptr, context); - EXPECT_NE(nullptr, instance); - EXPECT_NE(nullptr, dynamic_cast(instance.get())); -} - } // namespace } // namespace File } // namespace AccessLoggers