Fix legacy_proxy_filter_integration_test CI (#39613)

Commit Message: Fix legacy_proxy_filter_integration_test CI
Additional Description: Mocking out the DNS operations should make these
tests resilient against environmental differences. Unfortunately it's a
hideous thing to do, made worse by numeric IPv6 address parsing also
using the mockable function. There are two main issues being fixed by
this:
1. in some environments, DNS lookups for e.g. `nonexistent.example.com`
time out because the DNS server is unreachable.
2. in some environments, IPv6 isn't available, so the IPv6 versions of
the tests fails.

Risk Level: test-only (plus making a pair of API calls both use the
production-only OsSysCall like they should have been all along)
Testing: Yes it is.
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a

---------

Signed-off-by: Raven Black <[email protected]>

Author: ravenblackx

source/common/network/utility.cc

@@ -133,13 +133,31 @@ StatusOr<sockaddr_in6> parseV6Address(const std::string& ip_address, uint16_t po
   // we consume only the first element (if the call succeeds).
   hints.ai_socktype = SOCK_DGRAM;
   hints.ai_protocol = IPPROTO_UDP;
-  const Api::SysCallIntResult rc = Api::OsSysCallsSingleton::get().getaddrinfo(
-      ip_address.c_str(), /*service=*/nullptr, &hints, &res);
+
+  // We want to use the interface of OsSysCalls for this for the
+  // platform-independence, but we don't want to use the common
+  // OsSysCallsSingleton.
+  //
+  // The problem with using OsSysCallsSingleton is that we likely
+  // want to override getaddrinfo() for DNS lookups in tests, but
+  // typically that override would resolve a name to e.g.
+  // the address from resolveUrl("tcp://[::1]:80") - but resolveUrl
+  // calls parseV6Address, which calls getaddrinfo(), so if we use
+  // the mock *here* then mocking DNS causes infinite recursion.
+  //
+  // We don't ever need to mock *this* getaddrinfo() call, because
+  // it's only used to parse numeric IP addresses, per `ai_flags`,
+  // so it should be deterministic resolution; there's no need to
+  // mock it to test failure cases.
+  static Api::OsSysCallsImpl os_sys_calls;
+
+  const Api::SysCallIntResult rc =
+      os_sys_calls.getaddrinfo(ip_address.c_str(), /*service=*/nullptr, &hints, &res);
   if (rc.return_value_ != 0) {
     return absl::FailedPreconditionError(fmt::format("getaddrinfo error: {}", rc.return_value_));
   }
   sockaddr_in6 sa6 = *reinterpret_cast<sockaddr_in6*>(res->ai_addr);
-  freeaddrinfo(res);
+  os_sys_calls.freeaddrinfo(res);
   sa6.sin6_port = htons(port);
   return sa6;
 }

test/extensions/filters/http/dynamic_forward_proxy/BUILD

@@ -87,6 +87,7 @@ envoy_extension_cc_test(
         "//source/extensions/network/dns_resolver/getaddrinfo:config",
         "//test/integration:http_integration_lib",
         "//test/integration/filters:stream_info_to_headers_filter_lib",
+        "//test/test_common:threadsafe_singleton_injector_lib",
         "@envoy_api//envoy/config/bootstrap/v3:pkg_cc_proto",
         "@envoy_api//envoy/config/cluster/v3:pkg_cc_proto",
         "@envoy_api//envoy/extensions/filters/network/http_connection_manager/v3:pkg_cc_proto",
@@ -119,6 +120,7 @@ envoy_extension_cc_test(
         "//source/extensions/network/dns_resolver/getaddrinfo:config",
         "//test/integration:http_integration_lib",
         "//test/integration/filters:stream_info_to_headers_filter_lib",
+        "//test/test_common:threadsafe_singleton_injector_lib",
         "@envoy_api//envoy/config/bootstrap/v3:pkg_cc_proto",
         "@envoy_api//envoy/config/cluster/v3:pkg_cc_proto",
         "@envoy_api//envoy/extensions/filters/network/http_connection_manager/v3:pkg_cc_proto",

test/extensions/filters/http/dynamic_forward_proxy/proxy_filter_integration_test.cc

@@ -12,16 +12,72 @@
 #include "test/integration/http_integration.h"
 #include "test/integration/ssl_utility.h"
 #include "test/test_common/registry.h"
+#include "test/test_common/threadsafe_singleton_injector.h"
 
 using testing::HasSubstr;
 
 namespace Envoy {
 namespace {
 
+class OsSysCallsWithMockedDns : public Api::OsSysCallsImpl {
+public:
+  static addrinfo* makeAddrInfo(const Network::Address::InstanceConstSharedPtr& addr) {
+    addrinfo* ai = reinterpret_cast<addrinfo*>(malloc(sizeof(addrinfo)));
+    memset(ai, 0, sizeof(addrinfo));
+    ai->ai_protocol = IPPROTO_TCP;
+    ai->ai_socktype = SOCK_STREAM;
+    if (addr->ip()->ipv4() != nullptr) {
+      ai->ai_family = AF_INET;
+    } else {
+      ai->ai_family = AF_INET6;
+    }
+    sockaddr_storage* storage =
+        reinterpret_cast<sockaddr_storage*>(malloc(sizeof(sockaddr_storage)));
+    ai->ai_addr = reinterpret_cast<sockaddr*>(storage);
+    memcpy(ai->ai_addr, addr->sockAddr(), addr->sockAddrLen());
+    ai->ai_addrlen = addr->sockAddrLen();
+    return ai;
+  }
+
+  Api::SysCallIntResult getaddrinfo(const char* node, const char* /*service*/,
+                                    const addrinfo* /*hints*/, addrinfo** res) override {
+    *res = nullptr;
+    if (absl::string_view{"localhost"} == node) {
+      if (ip_version_ == Network::Address::IpVersion::v6) {
+        *res = makeAddrInfo(Network::Utility::getIpv6LoopbackAddress());
+      } else {
+        *res = makeAddrInfo(Network::Utility::getCanonicalIpv4LoopbackAddress());
+      }
+      return {0, 0};
+    }
+    if (nonexisting_addresses_.find(node) != nonexisting_addresses_.end()) {
+      return {EAI_NONAME, 0};
+    }
+    std::cerr << "Mock DNS does not have entry for: " << node << std::endl;
+    return {-1, 128};
+  }
+  void freeaddrinfo(addrinfo* ai) override {
+    while (ai != nullptr) {
+      addrinfo* p = ai;
+      ai = ai->ai_next;
+      free(p->ai_addr);
+      free(p);
+    }
+  }
+
+  void setIpVersion(Network::Address::IpVersion version) { ip_version_ = version; }
+
+  Network::Address::IpVersion ip_version_ = Network::Address::IpVersion::v4;
+
+  absl::flat_hash_set<absl::string_view> nonexisting_addresses_ = {"doesnotexist.example.com",
+                                                                   "itdoesnotexist"};
+};
+
 class ProxyFilterIntegrationTest : public testing::TestWithParam<Network::Address::IpVersion>,
                                    public HttpIntegrationTest {
 public:
   ProxyFilterIntegrationTest() : HttpIntegrationTest(Http::CodecType::HTTP1, GetParam()) {
+    mock_os_sys_calls_.setIpVersion(GetParam());
     upstream_tls_ = true;
     filename_ = TestEnvironment::temporaryPath("dns_cache.txt");
     ::unlink(filename_.c_str());
@@ -36,6 +92,14 @@ class ProxyFilterIntegrationTest : public testing::TestWithParam<Network::Addres
     setUpstreamProtocol(Http::CodecType::HTTP1);
   }
 
+  void TearDown() override {
+    // Shut down the server and upstreams before os_calls_ goes out of scope to avoid syscalls
+    // during its removal racing with the unlatching of the mocks.
+    test_server_.reset();
+    cleanupUpstreamAndDownstream();
+    fake_upstreams_.clear();
+  }
+
   void initialize() override { initializeWithArgs(); }
 
   void initializeWithArgs(uint64_t max_hosts = 1024, uint32_t max_pending_requests = 1024,
@@ -349,6 +413,8 @@ name: envoy.clusters.dynamic_forward_proxy
     }
   }
 
+  OsSysCallsWithMockedDns mock_os_sys_calls_;
+  TestThreadsafeSingletonInjector<Api::OsSysCallsImpl> os_calls_{&mock_os_sys_calls_};
   bool upstream_tls_{};
   bool low_stream_limits_{};
   std::string upstream_cert_name_{"upstreamlocalhost"};
@@ -422,9 +488,8 @@ TEST_P(ProxyFilterIntegrationTest, MultiPortTest) {
   requestWithBodyTest();
 
   // Create a second upstream, and send a request there.
-  // The second upstream is autonomous where the first was not so we'll only get a 200-ok if we hit
-  // the new port.
-  // this regression tests https://github.com/envoyproxy/envoy/issues/27331
+  // The second upstream is autonomous where the first was not so we'll only get a 200-ok if we
+  // hit the new port. this regression tests https://github.com/envoyproxy/envoy/issues/27331
   autonomous_upstream_ = true;
   createUpstream(Network::Test::getCanonicalLoopbackAddress(version_), upstreamConfig());
   default_request_headers_.setHost(
@@ -436,16 +501,6 @@ TEST_P(ProxyFilterIntegrationTest, MultiPortTest) {
 
 // Do a sanity check using the getaddrinfo() resolver.
 TEST_P(ProxyFilterIntegrationTest, RequestWithBodyGetAddrInfoResolver) {
-  // getaddrinfo() does not reliably return v6 addresses depending on the environment. For now
-  // just run this on v4 which is most likely to succeed. In v6 only environments this test won't
-  // run at all but should still be covered in public CI.
-  if (GetParam() != Network::Address::IpVersion::v4) {
-    return;
-  }
-
-  // See https://github.com/envoyproxy/envoy/issues/28504.
-  DISABLE_UNDER_WINDOWS;
-
   requestWithBodyTest(R"EOF(
     typed_dns_resolver_config:
       name: envoy.network.dns_resolver.getaddrinfo
@@ -877,7 +932,8 @@ TEST_P(ProxyFilterIntegrationTest, UpstreamCleartext) {
   checkSimpleRequestSuccess(0, 0, response.get());
 }
 
-// Regression test a bug where the host header was used for cache lookups rather than host:port key
+// Regression test a bug where the host header was used for cache lookups rather than host:port
+// key
 TEST_P(ProxyFilterIntegrationTest, CacheSansPort) {
   useAccessLog("%RESPONSE_CODE_DETAILS%");
   initializeWithArgs();