From 39f0ad4d5019cc182e2326ccc4da1a1c62fce4be Mon Sep 17 00:00:00 2001 From: Sorah Fukumori Date: Sun, 11 Oct 2020 09:54:54 +0900 Subject: [PATCH 1/9] config: respect address.pipe.mode in v3 api It seems listener[].address.pipe.mode configuration wasn't working under v3 api. Fixes https://github.com/envoyproxy/envoy/issues/11809 Signed-off-by: Sorah Fukumori --- source/common/network/resolver_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/network/resolver_impl.cc b/source/common/network/resolver_impl.cc index 8aa49b8bb8bca..66554c0d23674 100644 --- a/source/common/network/resolver_impl.cc +++ b/source/common/network/resolver_impl.cc @@ -48,7 +48,7 @@ InstanceConstSharedPtr resolveProtoAddress(const envoy::config::core::v3::Addres case envoy::config::core::v3::Address::AddressCase::kSocketAddress: return resolveProtoSocketAddress(address.socket_address()); case envoy::config::core::v3::Address::AddressCase::kPipe: - return std::make_shared(address.pipe().path()); + return std::make_shared(address.pipe().path(), address.pipe().mode()); case envoy::config::core::v3::Address::AddressCase::kEnvoyInternalAddress: switch (address.envoy_internal_address().address_name_specifier_case()) { case envoy::config::core::v3::EnvoyInternalAddress::AddressNameSpecifierCase:: From 2387bec9285f562e40ebc22fe7f956d37b63d773 Mon Sep 17 00:00:00 2001 From: Sorah Fukumori Date: Sun, 11 Oct 2020 13:40:44 +0900 Subject: [PATCH 2/9] address: Add integration test for pipe.mode commit 750431ad659c86173b152de53fed1054f10270dd added the feature w/ unit tests but wasn't working properly. Adding integration test to make it sure. Signed-off-by: Sorah Fukumori --- test/integration/uds_integration_test.cc | 31 ++++++++++++++++++++++-- test/integration/uds_integration_test.h | 7 ++++-- 2 files changed, 34 insertions(+), 4 deletions(-) diff --git a/test/integration/uds_integration_test.cc b/test/integration/uds_integration_test.cc index e1e63b8fbfb55..2a4b231bd4b60 100644 --- a/test/integration/uds_integration_test.cc +++ b/test/integration/uds_integration_test.cc @@ -1,5 +1,7 @@ #include "uds_integration_test.h" +#include + #include "envoy/config/bootstrap/v3/bootstrap.pb.h" #include "common/event/dispatcher_impl.h" @@ -47,14 +49,20 @@ TEST_P(UdsUpstreamIntegrationTest, RouterDownstreamDisconnectBeforeResponseCompl INSTANTIATE_TEST_SUITE_P( TestParameters, UdsListenerIntegrationTest, testing::Combine(testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), - testing::Values(false, true))); + testing::Values(false, true), testing::Values(0))); #else INSTANTIATE_TEST_SUITE_P( TestParameters, UdsListenerIntegrationTest, testing::Combine(testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), - testing::Values(false))); + testing::Values(false, true), testing::Values(0))); #endif +// Test the mode parameter, excluding abstract namespace enabled +INSTANTIATE_TEST_SUITE_P( + TestModeParameter, UdsListenerIntegrationTest, + testing::Combine(testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), + testing::Values(false), testing::Values(0662))); + void UdsListenerIntegrationTest::initialize() { config_helper_.addConfigModifier([&](envoy::config::bootstrap::v3::Bootstrap& bootstrap) -> void { auto* admin_addr = bootstrap.mutable_admin()->mutable_address(); @@ -68,6 +76,7 @@ void UdsListenerIntegrationTest::initialize() { auto* listener = listeners->Add(); listener->set_name("listener_0"); listener->mutable_address()->mutable_pipe()->set_path(getListenerSocketName()); + listener->mutable_address()->mutable_pipe()->set_mode(getMode()); *(listener->mutable_filter_chains()) = filter_chains; }); HttpIntegrationTest::initialize(); @@ -84,6 +93,24 @@ HttpIntegrationTest::ConnectionCreationFunction UdsListenerIntegrationTest::crea }; } +TEST_P(UdsListenerIntegrationTest, TestSocketMode) { + if (abstract_namespace_) { + // stat(2) against sockets in abstract namespace is not possible + GTEST_SKIP(); + } + + initialize(); + struct stat listener_stat; + EXPECT_EQ(::stat(getListenerSocketName().c_str(), &listener_stat), 0); + ENVOY_LOG_MISC(debug, "expectedmode={} mode={} uid={} gid={}", mode_, listener_stat.st_mode, + listener_stat.st_uid, listener_stat.st_gid); + if (mode_ == 0) { + EXPECT_NE(listener_stat.st_mode & 0777, 0); + } else { + EXPECT_EQ(listener_stat.st_mode & mode_, mode_); + } +} + TEST_P(UdsListenerIntegrationTest, TestPeerCredentials) { fake_upstreams_count_ = 1; initialize(); diff --git a/test/integration/uds_integration_test.h b/test/integration/uds_integration_test.h index 9ebd30cd006ee..43fdeabd5b569 100644 --- a/test/integration/uds_integration_test.h +++ b/test/integration/uds_integration_test.h @@ -54,12 +54,12 @@ class UdsUpstreamIntegrationTest }; class UdsListenerIntegrationTest - : public testing::TestWithParam>, + : public testing::TestWithParam>, public HttpIntegrationTest { public: UdsListenerIntegrationTest() : HttpIntegrationTest(Http::CodecClient::Type::HTTP1, std::get<0>(GetParam())), - abstract_namespace_(std::get<1>(GetParam())) {} + abstract_namespace_(std::get<1>(GetParam())), mode_(std::get<2>(GetParam())) {} void initialize() override; @@ -71,10 +71,13 @@ class UdsListenerIntegrationTest return TestEnvironment::unixDomainSocketPath("listener_0.sock", abstract_namespace_); } + mode_t getMode() { return mode_; } + protected: HttpIntegrationTest::ConnectionCreationFunction createConnectionFn(); const bool abstract_namespace_; + const mode_t mode_; }; } // namespace Envoy From 0cd5e3e262434b42bd6af216178ff25d4b07b656 Mon Sep 17 00:00:00 2001 From: Sorah Fukumori Date: Sun, 11 Oct 2020 14:26:24 +0900 Subject: [PATCH 3/9] fixup; only Linux supports abstract namespace fix miss copy and paste Signed-off-by: Sorah Fukumori --- test/integration/uds_integration_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/uds_integration_test.cc b/test/integration/uds_integration_test.cc index 2a4b231bd4b60..50038dd7cc779 100644 --- a/test/integration/uds_integration_test.cc +++ b/test/integration/uds_integration_test.cc @@ -54,7 +54,7 @@ INSTANTIATE_TEST_SUITE_P( INSTANTIATE_TEST_SUITE_P( TestParameters, UdsListenerIntegrationTest, testing::Combine(testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), - testing::Values(false, true), testing::Values(0))); + testing::Values(false), testing::Values(0))); #endif // Test the mode parameter, excluding abstract namespace enabled From beb3660f878a05001c198dc0d25ec0ddb3be4979 Mon Sep 17 00:00:00 2001 From: Sorah Fukumori Date: Sun, 11 Oct 2020 14:27:19 +0900 Subject: [PATCH 4/9] fixup; use OsSysCallsSingleton Signed-off-by: Sorah Fukumori --- test/integration/uds_integration_test.cc | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test/integration/uds_integration_test.cc b/test/integration/uds_integration_test.cc index 50038dd7cc779..ee347719e4504 100644 --- a/test/integration/uds_integration_test.cc +++ b/test/integration/uds_integration_test.cc @@ -4,6 +4,7 @@ #include "envoy/config/bootstrap/v3/bootstrap.pb.h" +#include "common/api/os_sys_calls_impl.h" #include "common/event/dispatcher_impl.h" #include "common/network/utility.h" @@ -100,10 +101,10 @@ TEST_P(UdsListenerIntegrationTest, TestSocketMode) { } initialize(); + + Api::OsSysCalls& os_sys_calls = Api::OsSysCallsSingleton::get(); struct stat listener_stat; - EXPECT_EQ(::stat(getListenerSocketName().c_str(), &listener_stat), 0); - ENVOY_LOG_MISC(debug, "expectedmode={} mode={} uid={} gid={}", mode_, listener_stat.st_mode, - listener_stat.st_uid, listener_stat.st_gid); + EXPECT_EQ(os_sys_calls.stat(getListenerSocketName().c_str(), &listener_stat).rc_, 0); if (mode_ == 0) { EXPECT_NE(listener_stat.st_mode & 0777, 0); } else { From 8f46d1f0385241acf537c0cab7d156e2e6f83797 Mon Sep 17 00:00:00 2001 From: Sorah Fukumori Date: Sun, 11 Oct 2020 14:38:24 +0900 Subject: [PATCH 5/9] fixup; remove unneeded include Signed-off-by: Sorah Fukumori --- test/integration/uds_integration_test.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/integration/uds_integration_test.cc b/test/integration/uds_integration_test.cc index ee347719e4504..f71e2aa0897cd 100644 --- a/test/integration/uds_integration_test.cc +++ b/test/integration/uds_integration_test.cc @@ -1,7 +1,5 @@ #include "uds_integration_test.h" -#include - #include "envoy/config/bootstrap/v3/bootstrap.pb.h" #include "common/api/os_sys_calls_impl.h" From 8aad5896394dfcbf9e4bfa1836eb5c88bce54318 Mon Sep 17 00:00:00 2001 From: Sorah Fukumori Date: Sun, 11 Oct 2020 15:37:47 +0900 Subject: [PATCH 6/9] fixup; Skip added test on win32 the unit test, PipeInstanceTest/BasicPermission is also skipped on win32 Signed-off-by: Sorah Fukumori --- test/integration/uds_integration_test.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/integration/uds_integration_test.cc b/test/integration/uds_integration_test.cc index f71e2aa0897cd..ac05cbb907d90 100644 --- a/test/integration/uds_integration_test.cc +++ b/test/integration/uds_integration_test.cc @@ -56,11 +56,13 @@ INSTANTIATE_TEST_SUITE_P( testing::Values(false), testing::Values(0))); #endif +#ifndef WIN32 // Test the mode parameter, excluding abstract namespace enabled INSTANTIATE_TEST_SUITE_P( TestModeParameter, UdsListenerIntegrationTest, testing::Combine(testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), testing::Values(false), testing::Values(0662))); +#endif void UdsListenerIntegrationTest::initialize() { config_helper_.addConfigModifier([&](envoy::config::bootstrap::v3::Bootstrap& bootstrap) -> void { From 5be26a5191c6e3c29e8f35e34861e97153c355ac Mon Sep 17 00:00:00 2001 From: Sorah Fukumori Date: Sun, 11 Oct 2020 18:37:40 +0900 Subject: [PATCH 7/9] fixup; disable added test entirely on Windows stat(2) fails with ENOENT against Windows dialect AF_UNIX socket files, while chmod(2) succeeds. Signed-off-by: Sorah Fukumori --- test/integration/uds_integration_test.cc | 6 ++++-- tools/spelling/spelling_dictionary.txt | 1 + 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/test/integration/uds_integration_test.cc b/test/integration/uds_integration_test.cc index ac05cbb907d90..6fce94e56646c 100644 --- a/test/integration/uds_integration_test.cc +++ b/test/integration/uds_integration_test.cc @@ -56,13 +56,11 @@ INSTANTIATE_TEST_SUITE_P( testing::Values(false), testing::Values(0))); #endif -#ifndef WIN32 // Test the mode parameter, excluding abstract namespace enabled INSTANTIATE_TEST_SUITE_P( TestModeParameter, UdsListenerIntegrationTest, testing::Combine(testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), testing::Values(false), testing::Values(0662))); -#endif void UdsListenerIntegrationTest::initialize() { config_helper_.addConfigModifier([&](envoy::config::bootstrap::v3::Bootstrap& bootstrap) -> void { @@ -94,6 +92,9 @@ HttpIntegrationTest::ConnectionCreationFunction UdsListenerIntegrationTest::crea }; } +// Excluding Windows; chmod(2) against Windows AF_UNIX socket files succeeds, +// but stat(2) against those returns ENOENT. +#ifndef WIN32 TEST_P(UdsListenerIntegrationTest, TestSocketMode) { if (abstract_namespace_) { // stat(2) against sockets in abstract namespace is not possible @@ -111,6 +112,7 @@ TEST_P(UdsListenerIntegrationTest, TestSocketMode) { EXPECT_EQ(listener_stat.st_mode & mode_, mode_); } } +#endif TEST_P(UdsListenerIntegrationTest, TestPeerCredentials) { fake_upstreams_count_ = 1; diff --git a/tools/spelling/spelling_dictionary.txt b/tools/spelling/spelling_dictionary.txt index 0a28f00119709..5eaaceff978fc 100644 --- a/tools/spelling/spelling_dictionary.txt +++ b/tools/spelling/spelling_dictionary.txt @@ -95,6 +95,7 @@ EINPROGRESS EINVAL ELB EMSGSIZE +ENOENT ENOTFOUND ENOTSUP ENV From 2f449387af550a84a4b1e92a3982d1dac29159d9 Mon Sep 17 00:00:00 2001 From: Sorah Fukumori Date: Sun, 11 Oct 2020 18:42:26 +0900 Subject: [PATCH 8/9] fixup; give same commentary about Windows on unit test Signed-off-by: Sorah Fukumori --- test/common/network/address_impl_test.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/common/network/address_impl_test.cc b/test/common/network/address_impl_test.cc index 2090ac3933453..c4dc6249f9f91 100644 --- a/test/common/network/address_impl_test.cc +++ b/test/common/network/address_impl_test.cc @@ -335,6 +335,8 @@ TEST(InteralInstanceTest, Basic) { EXPECT_EQ(static_cast(0), address.sockAddrLen()); } +// Excluding Windows; chmod(2) againsts Windows AF_UNIX socket files succeeds, +// but stat(2) against those returns ENOENT. #ifndef WIN32 TEST(PipeInstanceTest, BasicPermission) { std::string path = TestEnvironment::unixDomainSocketPath("foo.sock"); From afcbcb9a06a5ad19bfc3f3ef3cdada7020e84058 Mon Sep 17 00:00:00 2001 From: Sorah Fukumori Date: Sun, 11 Oct 2020 20:39:24 +0900 Subject: [PATCH 9/9] fixup; fix typo Signed-off-by: Sorah Fukumori --- test/common/network/address_impl_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/common/network/address_impl_test.cc b/test/common/network/address_impl_test.cc index c4dc6249f9f91..d1a01ca6734b7 100644 --- a/test/common/network/address_impl_test.cc +++ b/test/common/network/address_impl_test.cc @@ -335,7 +335,7 @@ TEST(InteralInstanceTest, Basic) { EXPECT_EQ(static_cast(0), address.sockAddrLen()); } -// Excluding Windows; chmod(2) againsts Windows AF_UNIX socket files succeeds, +// Excluding Windows; chmod(2) against Windows AF_UNIX socket files succeeds, // but stat(2) against those returns ENOENT. #ifndef WIN32 TEST(PipeInstanceTest, BasicPermission) {