Skip to content

Commit bbdfde5

Browse files
apolcynapolcyn
committed
Include the relevant query type and name in all c-ares DNS resolution errors
1 parent 06d4be0 commit bbdfde5

File tree

1 file changed

+76
-31
lines changed

1 file changed

+76
-31
lines changed

‎src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.cc‎

Lines changed: 76 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,8 @@ struct grpc_ares_request {
7575
grpc_error* error;
7676
};
7777

78+
// TODO(apolcyn): make grpc_ares_hostbyname_request a sub-class
79+
// of GrpcAresQuery.
7880
typedef struct grpc_ares_hostbyname_request {
7981
/** following members are set in create_hostbyname_request_locked
8082
*/
@@ -86,8 +88,37 @@ typedef struct grpc_ares_hostbyname_request {
8688
uint16_t port;
8789
/** is it a grpclb address */
8890
bool is_balancer;
91+
/** for logging and errors: the query type ("A" or "AAAA") */
92+
const char* qtype;
8993
} grpc_ares_hostbyname_request;
9094

95+
static void grpc_ares_request_ref_locked(grpc_ares_request* r);
96+
static void grpc_ares_request_unref_locked(grpc_ares_request* r);
97+
98+
// TODO(apolcyn): as a part of C++-ification, find a way to
99+
// organize per-query and per-resolution information in such a way
100+
// that doesn't involve allocating a number of different data
101+
// structures.
102+
class GrpcAresQuery {
103+
public:
104+
explicit GrpcAresQuery(grpc_ares_request* r, const std::string& name)
105+
: r_(r), name_(name) {
106+
grpc_ares_request_ref_locked(r_);
107+
}
108+
109+
~GrpcAresQuery() { grpc_ares_request_unref_locked(r_); }
110+
111+
grpc_ares_request* parent_request() { return r_; }
112+
113+
const std::string& name() { return name_; }
114+
115+
private:
116+
/* the top level request instance */
117+
grpc_ares_request* r_;
118+
/** for logging and errors */
119+
const std::string name_;
120+
};
121+
91122
static void log_address_sorting_list(const grpc_ares_request* r,
92123
const ServerAddressList& addresses,
93124
const char* input_output_str) {
@@ -162,19 +193,21 @@ void grpc_ares_complete_request_locked(grpc_ares_request* r) {
162193
grpc_core::ExecCtx::Run(DEBUG_LOCATION, r->on_done, r->error);
163194
}
164195

196+
/* Note that the returned object takes a reference to qtype, so
197+
* qtype must outlive it. */
165198
static grpc_ares_hostbyname_request* create_hostbyname_request_locked(
166199
grpc_ares_request* parent_request, const char* host, uint16_t port,
167-
bool is_balancer) {
200+
bool is_balancer, const char* qtype) {
168201
GRPC_CARES_TRACE_LOG(
169202
"request:%p create_hostbyname_request_locked host:%s port:%d "
170-
"is_balancer:%d",
171-
parent_request, host, port, is_balancer);
172-
grpc_ares_hostbyname_request* hr = static_cast<grpc_ares_hostbyname_request*>(
173-
gpr_zalloc(sizeof(grpc_ares_hostbyname_request)));
203+
"is_balancer:%d qtype:%s",
204+
parent_request, host, port, is_balancer, qtype);
205+
grpc_ares_hostbyname_request* hr = new grpc_ares_hostbyname_request();
174206
hr->parent_request = parent_request;
175207
hr->host = gpr_strdup(host);
176208
hr->port = port;
177209
hr->is_balancer = is_balancer;
210+
hr->qtype = qtype;
178211
grpc_ares_request_ref_locked(parent_request);
179212
return hr;
180213
}
@@ -183,7 +216,7 @@ static void destroy_hostbyname_request_locked(
183216
grpc_ares_hostbyname_request* hr) {
184217
grpc_ares_request_unref_locked(hr->parent_request);
185218
gpr_free(hr->host);
186-
gpr_free(hr);
219+
delete hr;
187220
}
188221

189222
static void on_hostbyname_done_locked(void* arg, int status, int /*timeouts*/,
@@ -193,8 +226,8 @@ static void on_hostbyname_done_locked(void* arg, int status, int /*timeouts*/,
193226
grpc_ares_request* r = hr->parent_request;
194227
if (status == ARES_SUCCESS) {
195228
GRPC_CARES_TRACE_LOG(
196-
"request:%p on_hostbyname_done_locked host=%s ARES_SUCCESS", r,
197-
hr->host);
229+
"request:%p on_hostbyname_done_locked qtype=%s host=%s ARES_SUCCESS", r,
230+
hr->qtype, hr->host);
198231
std::unique_ptr<ServerAddressList>* address_list_ptr =
199232
hr->is_balancer ? r->balancer_addresses_out : r->addresses_out;
200233
if (*address_list_ptr == nullptr) {
@@ -248,10 +281,12 @@ static void on_hostbyname_done_locked(void* arg, int status, int /*timeouts*/,
248281
}
249282
} else {
250283
char* error_msg;
251-
gpr_asprintf(&error_msg, "C-ares status is not ARES_SUCCESS: %s",
252-
ares_strerror(status));
253-
GRPC_CARES_TRACE_LOG("request:%p on_hostbyname_done_locked host=%s %s", r,
254-
hr->host, error_msg);
284+
gpr_asprintf(&error_msg,
285+
"C-ares status is not ARES_SUCCESS "
286+
"qtype=%s name=%s is_balancer=%d: %s",
287+
hr->qtype, hr->host, hr->is_balancer, ares_strerror(status));
288+
GRPC_CARES_TRACE_LOG("request:%p on_hostbyname_done_locked: %s", r,
289+
error_msg);
255290
grpc_error* error = GRPC_ERROR_CREATE_FROM_COPIED_STRING(error_msg);
256291
gpr_free(error_msg);
257292
r->error = grpc_error_add_child(error, r->error);
@@ -261,9 +296,12 @@ static void on_hostbyname_done_locked(void* arg, int status, int /*timeouts*/,
261296

262297
static void on_srv_query_done_locked(void* arg, int status, int /*timeouts*/,
263298
unsigned char* abuf, int alen) {
264-
grpc_ares_request* r = static_cast<grpc_ares_request*>(arg);
299+
GrpcAresQuery* q = static_cast<GrpcAresQuery*>(arg);
300+
grpc_ares_request* r = q->parent_request();
265301
if (status == ARES_SUCCESS) {
266-
GRPC_CARES_TRACE_LOG("request:%p on_srv_query_done_locked ARES_SUCCESS", r);
302+
GRPC_CARES_TRACE_LOG(
303+
"request:%p on_srv_query_done_locked name=%s ARES_SUCCESS", r,
304+
q->name().c_str());
267305
struct ares_srv_reply* reply;
268306
const int parse_status = ares_parse_srv_reply(abuf, alen, &reply);
269307
GRPC_CARES_TRACE_LOG("request:%p ares_parse_srv_reply: %d", r,
@@ -275,12 +313,13 @@ static void on_srv_query_done_locked(void* arg, int status, int /*timeouts*/,
275313
srv_it = srv_it->next) {
276314
if (grpc_ares_query_ipv6()) {
277315
grpc_ares_hostbyname_request* hr = create_hostbyname_request_locked(
278-
r, srv_it->host, htons(srv_it->port), true /* is_balancer */);
316+
r, srv_it->host, htons(srv_it->port), true /* is_balancer */,
317+
"AAAA");
279318
ares_gethostbyname(*channel, hr->host, AF_INET6,
280319
on_hostbyname_done_locked, hr);
281320
}
282321
grpc_ares_hostbyname_request* hr = create_hostbyname_request_locked(
283-
r, srv_it->host, htons(srv_it->port), true /* is_balancer */);
322+
r, srv_it->host, htons(srv_it->port), true /* is_balancer */, "A");
284323
ares_gethostbyname(*channel, hr->host, AF_INET,
285324
on_hostbyname_done_locked, hr);
286325
grpc_ares_ev_driver_start_locked(r->ev_driver);
@@ -291,29 +330,33 @@ static void on_srv_query_done_locked(void* arg, int status, int /*timeouts*/,
291330
}
292331
} else {
293332
char* error_msg;
294-
gpr_asprintf(&error_msg, "C-ares status is not ARES_SUCCESS: %s",
295-
ares_strerror(status));
296-
GRPC_CARES_TRACE_LOG("request:%p on_srv_query_done_locked %s", r,
333+
gpr_asprintf(&error_msg,
334+
"C-ares status is not ARES_SUCCESS "
335+
"qtype=SRV name=%s: %s",
336+
q->name().c_str(), ares_strerror(status));
337+
GRPC_CARES_TRACE_LOG("request:%p on_srv_query_done_locked: %s", r,
297338
error_msg);
298339
grpc_error* error = GRPC_ERROR_CREATE_FROM_COPIED_STRING(error_msg);
299340
gpr_free(error_msg);
300341
r->error = grpc_error_add_child(error, r->error);
301342
}
302-
grpc_ares_request_unref_locked(r);
343+
delete q;
303344
}
304345

305346
static const char g_service_config_attribute_prefix[] = "grpc_config=";
306347

307348
static void on_txt_done_locked(void* arg, int status, int /*timeouts*/,
308349
unsigned char* buf, int len) {
309350
char* error_msg;
310-
grpc_ares_request* r = static_cast<grpc_ares_request*>(arg);
351+
GrpcAresQuery* q = static_cast<GrpcAresQuery*>(arg);
352+
grpc_ares_request* r = q->parent_request();
311353
const size_t prefix_len = sizeof(g_service_config_attribute_prefix) - 1;
312354
struct ares_txt_ext* result = nullptr;
313355
struct ares_txt_ext* reply = nullptr;
314356
grpc_error* error = GRPC_ERROR_NONE;
315357
if (status != ARES_SUCCESS) goto fail;
316-
GRPC_CARES_TRACE_LOG("request:%p on_txt_done_locked ARES_SUCCESS", r);
358+
GRPC_CARES_TRACE_LOG("request:%p on_txt_done_locked name=%s ARES_SUCCESS", r,
359+
q->name().c_str());
317360
status = ares_parse_txt_reply_ext(buf, len, &reply);
318361
if (status != ARES_SUCCESS) goto fail;
319362
// Find service config in TXT record.
@@ -348,14 +391,16 @@ static void on_txt_done_locked(void* arg, int status, int /*timeouts*/,
348391
ares_free_data(reply);
349392
goto done;
350393
fail:
351-
gpr_asprintf(&error_msg, "C-ares TXT lookup status is not ARES_SUCCESS: %s",
352-
ares_strerror(status));
394+
gpr_asprintf(&error_msg,
395+
"C-ares status is not ARES_SUCCESS "
396+
"qtype=TXT name=%s: %s",
397+
q->name().c_str(), ares_strerror(status));
353398
error = GRPC_ERROR_CREATE_FROM_COPIED_STRING(error_msg);
354399
GRPC_CARES_TRACE_LOG("request:%p on_txt_done_locked %s", r, error_msg);
355400
gpr_free(error_msg);
356401
r->error = grpc_error_add_child(error, r->error);
357402
done:
358-
grpc_ares_request_unref_locked(r);
403+
delete q;
359404
}
360405

361406
void grpc_dns_lookup_ares_continue_after_check_localhost_and_ip_literals_locked(
@@ -429,30 +474,30 @@ void grpc_dns_lookup_ares_continue_after_check_localhost_and_ip_literals_locked(
429474
if (grpc_ares_query_ipv6()) {
430475
hr = create_hostbyname_request_locked(r, host.c_str(),
431476
grpc_strhtons(port.c_str()),
432-
/*is_balancer=*/false);
477+
/*is_balancer=*/false, "AAAA");
433478
ares_gethostbyname(*channel, hr->host, AF_INET6, on_hostbyname_done_locked,
434479
hr);
435480
}
436481
hr = create_hostbyname_request_locked(r, host.c_str(),
437482
grpc_strhtons(port.c_str()),
438-
/*is_balancer=*/false);
483+
/*is_balancer=*/false, "A");
439484
ares_gethostbyname(*channel, hr->host, AF_INET, on_hostbyname_done_locked,
440485
hr);
441486
if (r->balancer_addresses_out != nullptr) {
442487
/* Query the SRV record */
443-
grpc_ares_request_ref_locked(r);
444488
char* service_name;
445489
gpr_asprintf(&service_name, "_grpclb._tcp.%s", host.c_str());
490+
GrpcAresQuery* srv_query = new GrpcAresQuery(r, service_name);
446491
ares_query(*channel, service_name, ns_c_in, ns_t_srv,
447-
on_srv_query_done_locked, r);
492+
on_srv_query_done_locked, srv_query);
448493
gpr_free(service_name);
449494
}
450495
if (r->service_config_json_out != nullptr) {
451-
grpc_ares_request_ref_locked(r);
452496
char* config_name;
453497
gpr_asprintf(&config_name, "_grpc_config.%s", host.c_str());
498+
GrpcAresQuery* txt_query = new GrpcAresQuery(r, config_name);
454499
ares_search(*channel, config_name, ns_c_in, ns_t_txt, on_txt_done_locked,
455-
r);
500+
txt_query);
456501
gpr_free(config_name);
457502
}
458503
grpc_ares_ev_driver_start_locked(r->ev_driver);

0 commit comments

Comments
 (0)