file: truncating files which are not opened with append

docs/root/version_history/current.rst

@@ -13,6 +13,7 @@ Minor Behavior Changes
 
 * dynamic_forward_proxy: if a DNS resolution fails, failing immediately with a specific resolution error, rather than finishing up all local filters and failing to select an upstream host.
 * ext_authz: added requested server name in ext_authz network filter for auth review.
+* file: changed disk based files to truncate files which are not being appended to. This behavioral change can be temporarily reverted by setting runtime guard ``envoy.reloadable_features.append_or_truncate`` to false.
 * grpc: flip runtime guard ``envoy.reloadable_features.enable_grpc_async_client_cache`` to be default enabled. async grpc client created through getOrCreateRawAsyncClient will be cached by default.
 * http: now the max concurrent streams of http2 connection can not only be adjusted down according to the SETTINGS frame but also can be adjusted up, of course, it can not exceed the configured upper bounds. This fix is guarded by ``envoy.reloadable_features.http2_allow_capacity_increase_by_settings``.
 

envoy/filesystem/filesystem.h

@@ -26,9 +26,14 @@ class File {
   virtual ~File() = default;
 
   enum Operation {
+    // Open a file for reading.
     Read,
+    // Open a file for writing. The file will be truncated if Append is not set.
     Write,
+    // Create the file if it does not already exist
     Create,
+    // If writing, append to the file rather than writing to the begining and
+    // truncating after write.
     Append,
   };
 

source/common/filesystem/BUILD

@@ -54,6 +54,7 @@ envoy_cc_posix_library(
     strip_include_prefix = "posix",
     deps = [
         ":file_shared_lib",
+        "//source/common/runtime:runtime_features_lib",
     ],
 )
 

source/common/filesystem/posix/filesystem_impl.cc

@@ -16,6 +16,7 @@
 #include "source/common/common/logger.h"
 #include "source/common/common/utility.h"
 #include "source/common/filesystem/filesystem_impl.h"
+#include "source/common/runtime/runtime_features.h"
 
 #include "absl/strings/match.h"
 #include "absl/strings/str_cat.h"
@@ -63,6 +64,9 @@ FileImplPosix::FlagsAndMode FileImplPosix::translateFlag(FlagSet in) {
 
   if (in.test(File::Operation::Append)) {
     out |= O_APPEND;
+  } else if (in.test(File::Operation::Write) &&
+             Runtime::runtimeFeatureEnabled("envoy.reloadable_features.append_or_truncate")) {
+    out |= O_TRUNC;
   }
 
   if (in.test(File::Operation::Read) && in.test(File::Operation::Write)) {

source/common/filesystem/win32/filesystem_impl.cc

@@ -52,6 +52,9 @@ Api::IoCallSizeResult FileImplWin32::write(absl::string_view buffer) {
 Api::IoCallBoolResult FileImplWin32::close() {
   ASSERT(isOpen());
 
+  if (truncate_) {
+    SetEndOfFile(fd_);
+  }
   BOOL result = CloseHandle(fd_);
   fd_ = INVALID_HANDLE;
   if (result == 0) {
@@ -70,6 +73,9 @@ FileImplWin32::FlagsAndMode FileImplWin32::translateFlag(FlagSet in) {
 
   if (in.test(File::Operation::Write)) {
     access = GENERIC_WRITE;
+    if (!in.test(File::Operation::Append)) {
+      truncate_ = true;
+    }
   }
 
   // Order of tests matter here. There reason for that

source/common/filesystem/win32/filesystem_impl.h

@@ -27,6 +27,7 @@ class FileImplWin32 : public FileSharedImpl {
 
 private:
   friend class FileSystemImplTest;
+  bool truncate_{};
 };
 
 template <DWORD std_handle_> struct StdStreamFileImplWin32 : public FileImplWin32 {

source/common/runtime/runtime_features.cc

@@ -58,6 +58,7 @@ constexpr const char* runtime_features[] = {
     // Begin alphabetically sorted section.
     "envoy.reloadable_features.allow_response_for_timeout",
     "envoy.reloadable_features.allow_upstream_inline_write",
+    "envoy.reloadable_features.append_or_truncate",
     "envoy.reloadable_features.conn_pool_delete_when_idle",
     "envoy.reloadable_features.correct_scheme_and_xfp",
     "envoy.reloadable_features.correctly_validate_alpn",

test/common/filesystem/filesystem_impl_test.cc

@@ -463,5 +463,15 @@ TEST_F(FileSystemImplTest, TestIoFileError) {
   EXPECT_EQ(IoFileError::IoErrorCode::UnknownError, error3.getErrorCode());
 }
 
+TEST_F(FileSystemImplTest, Overwrite) {
+  const std::string original = "test_envoy";
+  std::string full_filename = TestEnvironment::writeStringToFileForTest("filename", original);
+  EXPECT_EQ(original, TestEnvironment::readFileToStringForTest(full_filename));
+
+  const std::string shorter = "short";
+  TestEnvironment::writeStringToFileForTest("filename", shorter, false, false);
+  EXPECT_EQ(shorter, TestEnvironment::readFileToStringForTest(full_filename));
+}
+
 } // namespace Filesystem
 } // namespace Envoy

test/test_common/environment.cc

@@ -402,17 +402,19 @@ void TestEnvironment::exec(const std::vector<std::string>& args) {
 
 std::string TestEnvironment::writeStringToFileForTest(const std::string& filename,
                                                       const std::string& contents,
-                                                      bool fully_qualified_path) {
+                                                      bool fully_qualified_path, bool do_unlink) {
   const std::string out_path =
       fully_qualified_path ? filename : TestEnvironment::temporaryPath(filename);
-  unlink(out_path.c_str());
+  if (do_unlink) {
+    unlink(out_path.c_str());
+  }
 
   Filesystem::FilePathAndType out_file_info{Filesystem::DestinationType::File, out_path};
   Filesystem::FilePtr file = Filesystem::fileSystemForTest().createFile(out_file_info);
   const Filesystem::FlagSet flags{1 << Filesystem::File::Operation::Write |
                                   1 << Filesystem::File::Operation::Create};
   const Api::IoCallBoolResult open_result = file->open(flags);
-  EXPECT_TRUE(open_result.return_value_);
+  EXPECT_TRUE(open_result.return_value_) << open_result.err_->getErrorDetails();
   const Api::IoCallSizeResult result = file->write(contents);
   EXPECT_EQ(contents.length(), result.return_value_);
   return out_path;

test/test_common/environment.h

@@ -200,11 +200,13 @@ class TestEnvironment {
    * @param filename: the name of the file to use
    * @param contents: the data to go in the file.
    * @param fully_qualified_path: if true, will write to filename without prepending the tempdir.
+   * @param unlink: if true will delete any prior file before writing.
    * @return the fully qualified path of the output file.
    */
   static std::string writeStringToFileForTest(const std::string& filename,
                                               const std::string& contents,
-                                              bool fully_qualified_path = false);
+                                              bool fully_qualified_path = false,
+                                              bool unlink = true);
   /**
    * Dumps the contents of the file into the string.
    *