Skip to content
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

Jump to bottom

accesslog: don't open log file with read flag #7998

Merged
merged 4 commits into from
Aug 22, 2019
Merged

accesslog: don't open log file with read flag #7998

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1a73940
accesslog: don't open log file with read flag
lizan Aug 21, 2019
0a6c958
unit test
lizan Aug 22, 2019
75892e2
massage libstdc++
lizan Aug 22, 2019
ece4bdf
revert
lizan Aug 22, 2019
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
6 changes: 3 additions & 3 deletions source/common/access_log/access_log_manager_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
39 changes: 27 additions & 12 deletions test/common/access_log/access_log_manager_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

using testing::_;
using testing::ByMove;
using testing::Invoke;
using testing::NiceMock;
using testing::Return;
using testing::ReturnNew;
Expand Down Expand Up @@ -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<bool>(false, 0))));
EXPECT_CALL(*file_, open_(_)).WillOnce(Return(ByMove(Filesystem::resultFailure<bool>(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<bool>(true);
}));
EXPECT_NE(nullptr, access_log_manager_.createAccessLog("foo"));
EXPECT_CALL(*file_, close_()).WillOnce(Return(ByMove(Filesystem::resultSuccess<bool>(true))));
}

TEST_F(AccessLogManagerImplTest, flushToLogFilePeriodically) {
NiceMock<Event::MockTimer>* timer = new NiceMock<Event::MockTimer>(&dispatcher_);

EXPECT_CALL(*file_, open_()).WillOnce(Return(ByMove(Filesystem::resultSuccess<bool>(true))));
EXPECT_CALL(*file_, open_(_)).WillOnce(Return(ByMove(Filesystem::resultSuccess<bool>(true))));
AccessLogFileSharedPtr log_file = access_log_manager_.createAccessLog("foo");

EXPECT_CALL(*timer, enableTimer(timeout_40ms_));
Expand Down Expand Up @@ -98,7 +113,7 @@ TEST_F(AccessLogManagerImplTest, flushToLogFilePeriodically) {
TEST_F(AccessLogManagerImplTest, flushToLogFileOnDemand) {
NiceMock<Event::MockTimer>* timer = new NiceMock<Event::MockTimer>(&dispatcher_);

EXPECT_CALL(*file_, open_()).WillOnce(Return(ByMove(Filesystem::resultSuccess<bool>(true))));
EXPECT_CALL(*file_, open_(_)).WillOnce(Return(ByMove(Filesystem::resultSuccess<bool>(true))));
AccessLogFileSharedPtr log_file = access_log_manager_.createAccessLog("foo");

EXPECT_CALL(*timer, enableTimer(timeout_40ms_));
Expand Down Expand Up @@ -163,7 +178,7 @@ TEST_F(AccessLogManagerImplTest, reopenFile) {
NiceMock<Event::MockTimer>* timer = new NiceMock<Event::MockTimer>(&dispatcher_);

Sequence sq;
EXPECT_CALL(*file_, open_())
EXPECT_CALL(*file_, open_(_))
.InSequence(sq)
.WillOnce(Return(ByMove(Filesystem::resultSuccess<bool>(true))));
AccessLogFileSharedPtr log_file = access_log_manager_.createAccessLog("foo");
Expand All @@ -188,7 +203,7 @@ TEST_F(AccessLogManagerImplTest, reopenFile) {
EXPECT_CALL(*file_, close_())
.InSequence(sq)
.WillOnce(Return(ByMove(Filesystem::resultSuccess<bool>(true))));
EXPECT_CALL(*file_, open_())
EXPECT_CALL(*file_, open_(_))
.InSequence(sq)
.WillOnce(Return(ByMove(Filesystem::resultSuccess<bool>(true))));

Expand Down Expand Up @@ -224,15 +239,15 @@ TEST_F(AccessLogManagerImplTest, reopenThrows) {
}));

Sequence sq;
EXPECT_CALL(*file_, open_())
EXPECT_CALL(*file_, open_(_))
.InSequence(sq)
.WillOnce(Return(ByMove(Filesystem::resultSuccess<bool>(true))));

AccessLogFileSharedPtr log_file = access_log_manager_.createAccessLog("foo");
EXPECT_CALL(*file_, close_())
.InSequence(sq)
.WillOnce(Return(ByMove(Filesystem::resultSuccess<bool>(true))));
EXPECT_CALL(*file_, open_())
EXPECT_CALL(*file_, open_(_))
.InSequence(sq)
.WillOnce(Return(ByMove(Filesystem::resultFailure<bool>(false, 0))));

Expand Down Expand Up @@ -262,7 +277,7 @@ TEST_F(AccessLogManagerImplTest, reopenThrows) {
}

TEST_F(AccessLogManagerImplTest, bigDataChunkShouldBeFlushedWithoutTimer) {
EXPECT_CALL(*file_, open_()).WillOnce(Return(ByMove(Filesystem::resultSuccess<bool>(true))));
EXPECT_CALL(*file_, open_(_)).WillOnce(Return(ByMove(Filesystem::resultSuccess<bool>(true))));
AccessLogFileSharedPtr log_file = access_log_manager_.createAccessLog("foo");

EXPECT_CALL(*file_, write_(_))
Expand Down Expand Up @@ -305,7 +320,7 @@ TEST_F(AccessLogManagerImplTest, reopenAllFiles) {
EXPECT_CALL(dispatcher_, createTimer_(_)).WillRepeatedly(ReturnNew<NiceMock<Event::MockTimer>>());

Sequence sq;
EXPECT_CALL(*file_, open_())
EXPECT_CALL(*file_, open_(_))
.InSequence(sq)
.WillOnce(Return(ByMove(Filesystem::resultSuccess<bool>(true))));
AccessLogFileSharedPtr log = access_log_manager_.createAccessLog("foo");
Expand All @@ -315,7 +330,7 @@ TEST_F(AccessLogManagerImplTest, reopenAllFiles) {
.WillOnce(Return(ByMove(std::unique_ptr<NiceMock<Filesystem::MockFile>>(file2))));

Sequence sq2;
EXPECT_CALL(*file2, open_())
EXPECT_CALL(*file2, open_(_))
.InSequence(sq2)
.WillOnce(Return(ByMove(Filesystem::resultSuccess<bool>(true))));
AccessLogFileSharedPtr log2 = access_log_manager_.createAccessLog("bar");
Expand All @@ -342,10 +357,10 @@ TEST_F(AccessLogManagerImplTest, reopenAllFiles) {
.InSequence(sq2)
.WillOnce(Return(ByMove(Filesystem::resultSuccess<bool>(true))));

EXPECT_CALL(*file_, open_())
EXPECT_CALL(*file_, open_(_))
.InSequence(sq)
.WillOnce(Return(ByMove(Filesystem::resultSuccess<bool>(true))));
EXPECT_CALL(*file2, open_())
EXPECT_CALL(*file2, open_(_))
.InSequence(sq2)
.WillOnce(Return(ByMove(Filesystem::resultSuccess<bool>(true))));

Expand Down
1 change: 1 addition & 0 deletions test/extensions/access_loggers/file/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
],
)
4 changes: 2 additions & 2 deletions test/mocks/filesystem/mocks.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
3 changes: 2 additions & 1 deletion test/mocks/filesystem/mocks.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ class MockFile : public File {
bool isOpen() const override { return is_open_; };
MOCK_CONST_METHOD0(path, std::string());

MOCK_METHOD0(open_, Api::IoCallBoolResult());
// 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());

Expand Down