Make file descriptor limit adjustment portable

envoy/api/os_sys_calls.h

@@ -1,6 +1,5 @@
 #pragma once
 
-#include <sys/resource.h>
 #include <sys/stat.h>
 
 #include <chrono>
@@ -301,14 +300,9 @@ class OsSysCalls {
   virtual void freeaddrinfo(addrinfo* res) PURE;
 
   /**
-   * @see man getrlimit
+   * @see Increase soft file descriptor limit to match hard limit.
    */
-  virtual SysCallIntResult getrlimit(int resource, struct rlimit* rlim) PURE;
-
-  /**
-   * @see man setrlimit
-   */
-  virtual SysCallIntResult setrlimit(int resource, const struct rlimit* rlim) PURE;
+  virtual SysCallIntResult raiseFileLimits() PURE;
 };
 
 using OsSysCallsPtr = std::unique_ptr<OsSysCalls>;

source/common/api/posix/os_sys_calls_impl.cc

@@ -3,6 +3,7 @@
 #include <arpa/inet.h>
 #include <fcntl.h>
 #include <netinet/in.h>
+#include <sys/resource.h>
 #include <sys/stat.h>
 #include <unistd.h>
 
@@ -463,14 +464,20 @@ SysCallIntResult OsSysCallsImpl::getaddrinfo(const char* node, const char* servi
 
 void OsSysCallsImpl::freeaddrinfo(addrinfo* res) { ::freeaddrinfo(res); }
 
-SysCallIntResult OsSysCallsImpl::getrlimit(int resource, struct rlimit* rlim) {
-  const int rc = ::getrlimit(resource, rlim);
-  return {rc, errno};
-}
-
-SysCallIntResult OsSysCallsImpl::setrlimit(int resource, const struct rlimit* rlim) {
-  const int rc = ::setrlimit(resource, rlim);
-  return {rc, errno};
+SysCallIntResult OsSysCallsImpl::raiseFileLimits() {
+  struct rlimit rlim;
+  if (const int result = ::getrlimit(RLIMIT_NOFILE, &rlim); result != 0) {
+    return {result, errno};
+  }
+  const auto old = rlim.rlim_cur;
+  if (old == rlim.rlim_max) {
+    return {0, 0};
+  }
+  rlim.rlim_cur = rlim.rlim_max;
+  if (const int result = ::setrlimit(RLIMIT_NOFILE, &rlim); result != 0) {
+    return {result, errno};
+  }
+  return {0, 0};
 }
 
 } // namespace Api

source/common/api/posix/os_sys_calls_impl.h

@@ -70,8 +70,7 @@ class OsSysCallsImpl : public OsSysCalls {
   SysCallIntResult getaddrinfo(const char* node, const char* service, const addrinfo* hints,
                                addrinfo** res) override;
   void freeaddrinfo(addrinfo* res) override;
-  SysCallIntResult getrlimit(int resource, struct rlimit* rlim) override;
-  SysCallIntResult setrlimit(int resource, const struct rlimit* rlim) override;
+  SysCallIntResult raiseFileLimits() override;
 };
 
 using OsSysCallsSingleton = ThreadSafeSingleton<OsSysCallsImpl>;

source/common/api/win32/os_sys_calls_impl.cc

@@ -487,14 +487,10 @@ SysCallIntResult OsSysCallsImpl::getaddrinfo(const char* node, const char* servi
 
 void OsSysCallsImpl::freeaddrinfo(addrinfo* res) { ::freeaddrinfo(res); }
 
-SysCallIntResult OsSysCallsImpl::getrlimit(int resource, struct rlimit* rlim) {
-  // Windows does not support all resource limits.
+SysCallIntResult OsSysCallsImpl::raiseFileLimits() {
+  // Windows does not support this functionality.
   return {0, 0};
 }
 
-SysCallIntResult OsSysCallsImpl::setrlimit(int resource, const struct rlimit* rlim) {
-  PANIC("not implemented");
-}
-
 } // namespace Api
 } // namespace Envoy

source/common/api/win32/os_sys_calls_impl.h

@@ -72,8 +72,7 @@ class OsSysCallsImpl : public OsSysCalls {
   SysCallIntResult getaddrinfo(const char* node, const char* service, const addrinfo* hints,
                                addrinfo** res) override;
   void freeaddrinfo(addrinfo* res) override;
-  SysCallIntResult getrlimit(int resource, struct rlimit* rlim) override;
-  SysCallIntResult setrlimit(int resource, const struct rlimit* rlim) override;
+  SysCallIntResult raiseFileLimits() override;
 };
 
 using OsSysCallsSingleton = ThreadSafeSingleton<OsSysCallsImpl>;

source/server/server.cc

@@ -401,24 +401,12 @@ void InstanceUtil::raiseFileLimits() {
   if (!Runtime::runtimeFeatureEnabled("envoy.restart_features.raise_file_limits")) {
     return;
   }
-  struct rlimit rlim;
-  if (const auto result = Api::OsSysCallsSingleton::get().getrlimit(RLIMIT_NOFILE, &rlim);
+  if (const auto result = Api::OsSysCallsSingleton::get().raiseFileLimits();
       result.return_value_ != 0) {
-    ENVOY_LOG(warn, "Failed to read file descriptor limit, error {}.", errorDetails(result.errno_));
-    return;
-  }
-  const auto old = rlim.rlim_cur;
-  if (old == rlim.rlim_max) {
-    return;
-  }
-  rlim.rlim_cur = rlim.rlim_max;
-  if (const auto result = Api::OsSysCallsSingleton::get().setrlimit(RLIMIT_NOFILE, &rlim);
-      result.return_value_ != 0) {
-    ENVOY_LOG(warn, "Failed to raise file descriptor limit to maximum, error {}.",
+    ENVOY_LOG(warn, "Failed to raise file descriptor limit, error {}.",
               errorDetails(result.errno_));
     return;
   }
-  ENVOY_LOG(info, "Raised file descriptor limits from {} to {}.", old, rlim.rlim_max);
 }
 
 void InstanceBase::initialize(Network::Address::InstanceConstSharedPtr local_address,

test/common/api/os_sys_calls_test.cc

@@ -90,10 +90,9 @@ TEST(OsSyscallsTest, IoCtlInvalidFd) {
   EXPECT_NE(0, Api::OsSysCallsSingleton::get().ioctl(0, 0, nullptr, 0, nullptr, 0, nullptr).errno_);
 }
 
-TEST(OsSyscallsTest, Setrlimit) {
-  // Not all environments support it, but it is safe to read limits.
-  struct rlimit rlim;
-  Api::OsSysCallsSingleton::get().getrlimit(RLIMIT_NOFILE, &rlim);
+TEST(OsSyscallsTest, RaiseFileLimits) {
+  // Not all environments support it, but it is safe to call it.
+  Api::OsSysCallsSingleton::get().raiseFileLimits();
 }
 
 } // namespace Envoy

test/coverage.yaml

@@ -4,8 +4,8 @@ thresholds:
 
 directories:
   source/common: 96.4
-  source/common/api: 95.3  # some syscalls require sandboxing
-  source/common/api/posix: 94.9  # setns requires Linux CAP_NET_ADMIN privileges
+  source/common/api: 94.9  # some syscalls require sandboxing
+  source/common/api/posix: 94.6  # setns requires Linux CAP_NET_ADMIN privileges, no tests for set/getrlimit syscalls errors
   source/common/crypto: 91.2  # Static singleton initialization and OpenSSL internal error paths not testable
   source/common/filesystem/posix: 96.5  # FileReadToEndNotReadable fails in some env; createPath can't test all failure branches.
   source/common/http: 96.5

test/server/server_test.cc

@@ -1,3 +1,6 @@
+#ifndef WIN32
+#include <sys/resource.h>
+#endif
 #include <algorithm>
 #include <memory>
 #include <vector>
@@ -173,58 +176,29 @@ TEST(ServerInstanceUtil, flushImportModeUninitializedGauges) {
   InstanceUtil::flushMetricsToSinks(sinks, store, cm, time_system);
 }
 
+#ifndef WIN32
 TEST(ServerInstanceUtil, RaiseFileLimits) {
-  Api::MockOsSysCalls os_sys_calls_;
-  TestThreadsafeSingletonInjector<Api::OsSysCallsImpl> os_calls{&os_sys_calls_};
-  EXPECT_CALL(os_sys_calls_, getrlimit(RLIMIT_NOFILE, _))
-      .WillOnce(Invoke([&](int, struct rlimit* rlim) {
-        rlim->rlim_cur = 512;
-        rlim->rlim_max = 1024;
-        return Api::SysCallIntResult{0, 0};
-      }));
-  EXPECT_CALL(os_sys_calls_, setrlimit(RLIMIT_NOFILE, _))
-      .WillOnce(Invoke([&](int, const struct rlimit* rlim) {
-        EXPECT_EQ(1024, rlim->rlim_cur);
-        EXPECT_EQ(1024, rlim->rlim_max);
-        return Api::SysCallIntResult{0, 0};
-      }));
+  struct rlimit rlim;
+  EXPECT_EQ(::getrlimit(RLIMIT_NOFILE, &rlim), 0);
+  ASSERT_GT(rlim.rlim_max, 1);
+  // Set the soft limit lower than the hard limit.
+  rlim.rlim_cur = rlim.rlim_max / 2;
   InstanceUtil::raiseFileLimits();
+  EXPECT_EQ(::getrlimit(RLIMIT_NOFILE, &rlim), 0);
+  EXPECT_EQ(rlim.rlim_cur, rlim.rlim_max);
 }
 
 TEST(ServerInstanceUtil, RaiseFileLimitsAlreadyMaxed) {
-  Api::MockOsSysCalls os_sys_calls_;
-  TestThreadsafeSingletonInjector<Api::OsSysCallsImpl> os_calls{&os_sys_calls_};
-  EXPECT_CALL(os_sys_calls_, getrlimit(RLIMIT_NOFILE, _))
-      .WillOnce(Invoke([&](int, struct rlimit* rlim) {
-        rlim->rlim_cur = 1024;
-        rlim->rlim_max = 1024;
-        return Api::SysCallIntResult{0, 0};
-      }));
-  InstanceUtil::raiseFileLimits();
-}
-
-TEST(ServerInstanceUtil, RaiseFileLimitsReadError) {
-  Api::MockOsSysCalls os_sys_calls_;
-  TestThreadsafeSingletonInjector<Api::OsSysCallsImpl> os_calls{&os_sys_calls_};
-  EXPECT_CALL(os_sys_calls_, getrlimit(RLIMIT_NOFILE, _)).WillOnce(Invoke([&](int, struct rlimit*) {
-    return Api::SysCallIntResult{-1, 0};
-  }));
-  InstanceUtil::raiseFileLimits();
-}
-
-TEST(ServerInstanceUtil, RaiseFileLimitsWriteError) {
-  Api::MockOsSysCalls os_sys_calls_;
-  TestThreadsafeSingletonInjector<Api::OsSysCallsImpl> os_calls{&os_sys_calls_};
-  EXPECT_CALL(os_sys_calls_, getrlimit(RLIMIT_NOFILE, _))
-      .WillOnce(Invoke([&](int, struct rlimit* rlim) {
-        rlim->rlim_cur = 512;
-        rlim->rlim_max = 1024;
-        return Api::SysCallIntResult{0, 0};
-      }));
-  EXPECT_CALL(os_sys_calls_, setrlimit(RLIMIT_NOFILE, _))
-      .WillOnce(Invoke([&](int, const struct rlimit*) { return Api::SysCallIntResult{-1, 0}; }));
+  struct rlimit rlim;
+  EXPECT_EQ(::getrlimit(RLIMIT_NOFILE, &rlim), 0);
+  rlim.rlim_cur = rlim.rlim_max;
+  EXPECT_EQ(::setrlimit(RLIMIT_NOFILE, &rlim), 0);
+  // Verify that limits remain unchanged when they are the same.
   InstanceUtil::raiseFileLimits();
+  EXPECT_EQ(::getrlimit(RLIMIT_NOFILE, &rlim), 0);
+  EXPECT_EQ(rlim.rlim_cur, rlim.rlim_max);
 }
+#endif
 
 class RunHelperTest : public testing::Test {
 public: