From b5b99069c52f10aaaefa138559000e2746aeaaf1 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Mon, 31 Jan 2022 15:07:31 -0500 Subject: [PATCH 1/3] file: truncating files which are not opened with append Signed-off-by: Alyssa Wilk --- docs/root/version_history/current.rst | 1 + envoy/filesystem/filesystem.h | 5 +++++ source/common/filesystem/BUILD | 1 + source/common/filesystem/posix/filesystem_impl.cc | 4 ++++ source/common/runtime/runtime_features.cc | 1 + test/common/filesystem/filesystem_impl_test.cc | 10 ++++++++++ test/test_common/environment.cc | 8 +++++--- test/test_common/environment.h | 4 +++- 8 files changed, 30 insertions(+), 4 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 28bcc3de28e4..11767786c1e5 100644 --- a/docs/root/version_history/current.rst +++ b/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``. diff --git a/envoy/filesystem/filesystem.h b/envoy/filesystem/filesystem.h index 947302acd421..98b78de04fb4 100644 --- a/envoy/filesystem/filesystem.h +++ b/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. 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, }; diff --git a/source/common/filesystem/BUILD b/source/common/filesystem/BUILD index 9fc6afae7863..34b812833eed 100644 --- a/source/common/filesystem/BUILD +++ b/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", ], ) diff --git a/source/common/filesystem/posix/filesystem_impl.cc b/source/common/filesystem/posix/filesystem_impl.cc index 44dfd6118047..968f00da57ea 100644 --- a/source/common/filesystem/posix/filesystem_impl.cc +++ b/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)) { diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 6b4031a212ff..b3f4b88c04b4 100644 --- a/source/common/runtime/runtime_features.cc +++ b/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", diff --git a/test/common/filesystem/filesystem_impl_test.cc b/test/common/filesystem/filesystem_impl_test.cc index a079d2e422e2..7efc2df10ad0 100644 --- a/test/common/filesystem/filesystem_impl_test.cc +++ b/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 diff --git a/test/test_common/environment.cc b/test/test_common/environment.cc index 37ee5679ec3a..e4a59000dfc2 100644 --- a/test/test_common/environment.cc +++ b/test/test_common/environment.cc @@ -402,17 +402,19 @@ void TestEnvironment::exec(const std::vector& 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; diff --git a/test/test_common/environment.h b/test/test_common/environment.h index 0aa1cdd632d7..48b9f01d7c9e 100644 --- a/test/test_common/environment.h +++ b/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. * From 8093fd486757526c533d4060e109599b9c3a2e7b Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Wed, 2 Feb 2022 08:50:42 -0500 Subject: [PATCH 2/3] windows fix Signed-off-by: Alyssa Wilk --- source/common/filesystem/win32/filesystem_impl.cc | 6 ++++++ source/common/filesystem/win32/filesystem_impl.h | 1 + 2 files changed, 7 insertions(+) diff --git a/source/common/filesystem/win32/filesystem_impl.cc b/source/common/filesystem/win32/filesystem_impl.cc index c5435989e99e..08a5b2101e07 100644 --- a/source/common/filesystem/win32/filesystem_impl.cc +++ b/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 diff --git a/source/common/filesystem/win32/filesystem_impl.h b/source/common/filesystem/win32/filesystem_impl.h index 4c009dd30bea..733f5d02a3e9 100644 --- a/source/common/filesystem/win32/filesystem_impl.h +++ b/source/common/filesystem/win32/filesystem_impl.h @@ -27,6 +27,7 @@ class FileImplWin32 : public FileSharedImpl { private: friend class FileSystemImplTest; + bool truncate_{}; }; template struct StdStreamFileImplWin32 : public FileImplWin32 { From 784f6cd192fec34c17875e118303c98177b1370a Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Wed, 2 Feb 2022 16:23:20 -0500 Subject: [PATCH 3/3] comment Signed-off-by: Alyssa Wilk --- envoy/filesystem/filesystem.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/envoy/filesystem/filesystem.h b/envoy/filesystem/filesystem.h index 98b78de04fb4..417162afb9da 100644 --- a/envoy/filesystem/filesystem.h +++ b/envoy/filesystem/filesystem.h @@ -28,7 +28,7 @@ class File { enum Operation { // Open a file for reading. Read, - // Open a file for writing. + // 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,