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

Description:
File access log shouldn't need read access for a file.

Risk Level: Low
Testing: local in mac, CI
Docs Changes:
Release Notes:
Fixes #7997

Signed-off-by: Lizan Zhou <lizan@tetrate.io>

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;
 }

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<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_, _));
@@ -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_, _));
@@ -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");
@@ -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))));
 
@@ -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))));
 
@@ -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_(_))
@@ -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");
@@ -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");
@@ -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))));
 

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",
     ],
 )

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();

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_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());