comments

Signed-off-by: Matt Klein <mklein@lyft.com>
envoyproxy · Jul 13, 2022 · 0db567e · 0db567e
1 parent 4f7405f
commit 0db567e
Show file tree

Hide file tree

Showing 3 changed files with 38 additions and 29 deletions.
diff --git a/api/envoy/extensions/network/dns_resolver/getaddrinfo/v3/getaddrinfo_dns_resolver.proto b/api/envoy/extensions/network/dns_resolver/getaddrinfo/v3/getaddrinfo_dns_resolver.proto
@@ -19,7 +19,7 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE;
 // .. attention::
 //
 //   This resolver uses a single background thread to do resolutions. As such, it is not currently
-//   advised for use in situations requiring a high resolution late. A thread pool can be added
+//   advised for use in situations requiring a high resolution rate. A thread pool can be added
 //   in the future if needed.
 //
 // .. attention::

diff --git a/source/extensions/network/dns_resolver/getaddrinfo/getaddrinfo.cc b/source/extensions/network/dns_resolver/getaddrinfo/getaddrinfo.cc
@@ -62,7 +62,11 @@ class GetAddrInfoDnsResolver : public DnsResolver, public Logger::Loggable<Logge
   class AddrInfoWrapper : NonCopyable {
   public:
     AddrInfoWrapper(addrinfo* info) : info_(info) {}
-    ~AddrInfoWrapper() { Api::OsSysCallsSingleton::get().freeaddrinfo(info_); }
+    ~AddrInfoWrapper() {
+      if (info_ != nullptr) {
+        Api::OsSysCallsSingleton::get().freeaddrinfo(info_);
+      }
+    }
     const addrinfo* get() { return info_; }
 
   private:

diff --git a/test/extensions/filters/http/dynamic_forward_proxy/proxy_filter_integration_test.cc b/test/extensions/filters/http/dynamic_forward_proxy/proxy_filter_integration_test.cc
@@ -178,22 +178,19 @@ name: envoy.clusters.dynamic_forward_proxy
 
     initializeWithArgs(1024, 1024, "", typed_dns_resolver_config);
     codec_client_ = makeHttpConnection(lookupPort("http"));
-    const Http::TestRequestHeaderMapImpl request_headers{
-        {":method", "POST"},
-        {":path", "/test/long/url"},
-        {":scheme", "http"},
-        {":authority",
-         fmt::format("localhost:{}", fake_upstreams_[0]->localAddress()->ip()->port())}};
-
-    auto response =
-        sendRequestAndWaitForResponse(request_headers, 1024, default_response_headers_, 1024);
+    default_request_headers_.setHost(
+        fmt::format("localhost:{}", fake_upstreams_[0]->localAddress()->ip()->port()));
+
+    auto response = sendRequestAndWaitForResponse(default_request_headers_, 1024,
+                                                  default_response_headers_, 1024);
     checkSimpleRequestSuccess(1024, 1024, response.get());
     testConnectionTiming(response, false, original_usec);
     EXPECT_EQ(1, test_server_->counter("dns_cache.foo.dns_query_attempt")->value());
     EXPECT_EQ(1, test_server_->counter("dns_cache.foo.host_added")->value());
 
     // Now send another request. This should hit the DNS cache.
-    response = sendRequestAndWaitForResponse(request_headers, 512, default_response_headers_, 512);
+    response = sendRequestAndWaitForResponse(default_request_headers_, 512,
+                                             default_response_headers_, 512);
     checkSimpleRequestSuccess(512, 512, response.get());
     testConnectionTiming(response, true, original_usec);
     EXPECT_EQ(1, test_server_->counter("dns_cache.foo.dns_query_attempt")->value());
@@ -208,6 +205,23 @@ name: envoy.clusters.dynamic_forward_proxy
     EXPECT_STREQ("localhost", SSL_get_servername(ssl_socket->ssl(), TLSEXT_NAMETYPE_host_name));
   }
 
+  void requestWithUnknownDomainTest(const std::string& typed_dns_resolver_config = "") {
+    useAccessLog("%RESPONSE_CODE_DETAILS%");
+    initializeWithArgs(1024, 1024, "", typed_dns_resolver_config);
+    codec_client_ = makeHttpConnection(lookupPort("http"));
+    default_request_headers_.setHost("doesnotexist.example.com");
+
+    auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_);
+    ASSERT_TRUE(response->waitForEndStream());
+    EXPECT_EQ("503", response->headers().getStatusValue());
+    EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("dns_resolution_failure"));
+
+    response = codec_client_->makeHeaderOnlyRequest(default_request_headers_);
+    ASSERT_TRUE(response->waitForEndStream());
+    EXPECT_EQ("503", response->headers().getStatusValue());
+    EXPECT_THAT(waitForAccessLog(access_log_name_, 1), HasSubstr("dns_resolution_failure"));
+  }
+
   bool upstream_tls_{};
   std::string upstream_cert_name_{"upstreamlocalhost"};
   CdsHelper cds_helper_;
@@ -286,24 +300,15 @@ TEST_P(ProxyFilterIntegrationTest, RequestWithBodyGetAddrInfoResolver) {
 
 // Currently if the first DNS resolution fails, the filter will continue with
 // a null address. Make sure this mode fails gracefully.
-TEST_P(ProxyFilterIntegrationTest, RequestWithUnknownDomain) {
-  useAccessLog("%RESPONSE_CODE_DETAILS%");
-  initializeWithArgs();
-  codec_client_ = makeHttpConnection(lookupPort("http"));
-  const Http::TestRequestHeaderMapImpl request_headers{{":method", "GET"},
-                                                       {":path", "/test/long/url"},
-                                                       {":scheme", "http"},
-                                                       {":authority", "doesnotexist.example.com"}};
+TEST_P(ProxyFilterIntegrationTest, RequestWithUnknownDomain) { requestWithUnknownDomainTest(); }
 
-  auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_);
-  ASSERT_TRUE(response->waitForEndStream());
-  EXPECT_EQ("503", response->headers().getStatusValue());
-  EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("dns_resolution_failure"));
-
-  response = codec_client_->makeHeaderOnlyRequest(default_request_headers_);
-  ASSERT_TRUE(response->waitForEndStream());
-  EXPECT_EQ("503", response->headers().getStatusValue());
-  EXPECT_THAT(waitForAccessLog(access_log_name_, 1), HasSubstr("dns_resolution_failure"));
+// Do a sanity check using the getaddrinfo() resolver.
+TEST_P(ProxyFilterIntegrationTest, RequestWithUnknownDomainGetAddrInfoResolver) {
+  requestWithUnknownDomainTest(R"EOF(
+    typed_dns_resolver_config:
+      name: envoy.network.dns_resolver.getaddrinfo
+      typed_config:
+        "@type": type.googleapis.com/envoy.extensions.network.dns_resolver.getaddrinfo.v3.GetAddrInfoDnsResolverConfig)EOF");
 }
 
 TEST_P(ProxyFilterIntegrationTest, RequestWithUnknownDomainAndNoCaching) {