dns: adding resolution details by alyssawilk · Pull Request #34490 · envoyproxy/envoy

alyssawilk · 2024-06-03T19:25:56Z

Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

repokitteh-read-only · 2024-06-03T19:26:01Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #34490 was opened by alyssawilk.

see: more, trace.

repokitteh-read-only · 2024-06-03T19:26:03Z

CC @envoyproxy/runtime-guard-changes: FYI only for changes made to (source/common/runtime/runtime_features.cc).

🐱

Caused by: #34490 was opened by alyssawilk.

see: more, trace.

abeyad · 2024-06-03T19:40:53Z

    address_changed = true;
    stats_.host_address_changed_.inc();
+  } else if (current_address == nullptr) {
+    primary_host_info->host_info_->setDetails(details);


this will end up setting error details for DNS even if we already have an address for the host, but this particular DNS query failed to give us an address (i.e. current_address==nullptr). So probably instead of an else if, right after the if on L434, we should do if (!primary_host_info->host_info_->hasAddress()) { primary_host_info->host_info_->setDetails(details); }.

Unless I'm missing something?

so my thought was if we had a non-null address and we resolve a null address, we don't actually update the resolved address so we also shouldn't update the details. but if current address is null it's one we never had an address for at which point we'll use the null address and should set details. WDYT?

abeyad · 2024-06-03T19:44:49Z

    // events indicates that the sd_ref state is broken.
    // Therefore, finish resolving with an error.
    pending_response_.status_ = ResolutionStatus::Failure;
+    pending_response_.details = absl::StrCat(error);


maybe we should append "onEventCallback" to the details here?

alyssawilk

sorry for lack of diffs for you - I generally force push changes until reviewer is assigned just to have a clean first commit!

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

RyanTheOptimist

Yay more detailed logging!

RyanTheOptimist · 2024-06-04T21:52:40Z

@@ -97,7 +97,8 @@ class DnsResolver {
   * @param status supplies the final status of the resolution.
   * @param response supplies the list of resolved IP addresses and TTLs.


Can you add the new details param to this comment?

Should details be a string_view, or a const string&? Or do we want the move semantics? (Presumably I'll figure this out down below)

Looking below, it seems like we're doing some copies. I'd be inclined to make this a string_view but if we actually need the callback to own the string, can we do string&& instead?

had started with string view but I think it needs to be a string to handle cross-thread lifetime issues.

Oh, does this cross threads? I didn't see any dispatcher calls. How does that happen? In any case, if we need the string data to be owned by the callback, can we use string&& to avoid a copy?

ah sorry it's the outer one that's cross thread - see source/extensions/network/dns_resolver/getaddrinfo/getaddrinfo.cc post so I might be able to make it string view? lmk if you think it's worth the change.

Ah! Yeah, I'd switch this to string_view and change the post() call in source/extensions/network/dns_resolver/getaddrinfo/getaddrinfo.cc to do details = std::string(details) in the capture list. WDYT?

done (once I test locally and push)

RyanTheOptimist · 2024-06-04T21:53:19Z

  void setResponseCode(uint32_t code) override { response_code_ = code; }

  void setResponseCodeDetails(absl::string_view rc_details) override {
+    // Callers should sanitize with StringUtil::replaceAllEmptySpace if necessary.


nit: "should sanitize" => "should have sanitized"?

I think both are correct - callers should have done so, but if you're reading this comment it's probable you should and you haven't =P

RyanTheOptimist · 2024-06-04T21:56:32Z

    struct PendingResponse {
      ResolutionStatus status_;
      std::list<DnsResponse> address_list_;
+      std::string details_{"not_set"};


I've not see "not_set" as a default error details before. I wonder if empty string would be more clear? In particular if I were debugging something and I saw "not_set" in an error log, I can totally imagine thinking that I didn't set something in my call and that was causing a "not_set" error.

yeah I preferred an explicit not set in case some error to string function didn't have details, or some extension lazily just set the callback with an empty string, so you'd be able to differentiate a resolve resulting in an empty string from a corner case where the string was accidentally not set. I can change if you prefer

RyanTheOptimist · 2024-06-04T21:57:09Z

    // onDNSServiceGetAddrInfoReply callback.
    struct PendingResponse {
      ResolutionStatus status_ = ResolutionStatus::Success;
+      std::string details_ = "not_set";


ditto about "not_set" vs "".

RyanTheOptimist · 2024-06-04T21:59:08Z

+  /**
+   * Returns details about the last resolution.
+   */
+  virtual std::string details() PURE;


Are these error details? Or is this set for success too? (Would be good to comment this)

RyanTheOptimist · 2024-06-04T22:01:32Z

    latchTime(decoder_callbacks_, DNS_END);
    if (!host.has_value() || !host.value()->address()) {
-      onDnsResolutionFail();
+      onDnsResolutionFail(host.has_value() ? *host : nullptr);


How 'bout we just pass host down to onDnsResolutionFail? This will avoid making a copy of the shared pointer, I think.

eh, I mildly prefer doing the optional sorting here rather than in the function but no strong feelings

Copying shared pointer involve locking operations which is a bit of a bummer. Looking at the method, it looks like we don't actually use the value of the host at all, just the details. Maybe we could just pass in the details?

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

RyanTheOptimist

Two minor comments, but lgtm otherwise

RyanTheOptimist · 2024-06-06T13:59:04Z

@@ -97,7 +97,8 @@ class DnsResolver {
   * @param status supplies the final status of the resolution.
   * @param response supplies the list of resolved IP addresses and TTLs.


Oh, does this cross threads? I didn't see any dispatcher calls. How does that happen? In any case, if we need the string data to be owned by the callback, can we use string&& to avoid a copy?

RyanTheOptimist · 2024-06-06T14:03:19Z

    latchTime(decoder_callbacks_, DNS_END);
    if (!host.has_value() || !host.value()->address()) {
-      onDnsResolutionFail();
+      onDnsResolutionFail(host.has_value() ? *host : nullptr);


Copying shared pointer involve locking operations which is a bit of a bummer. Looking at the method, it looks like we don't actually use the value of the host at all, just the details. Maybe we could just pass in the details?

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk

Ci may bounce - can't check apple changes locally

alyssawilk · 2024-06-06T15:05:53Z

    latchTime(decoder_callbacks_, DNS_END);
    if (!host.has_value() || !host.value()->address()) {
-      onDnsResolutionFail();
+      onDnsResolutionFail(host.has_value() ? *host : nullptr);


alyssawilk · 2024-06-06T15:08:00Z

@@ -97,7 +97,8 @@ class DnsResolver {
   * @param status supplies the final status of the resolution.
   * @param response supplies the list of resolved IP addresses and TTLs.


ah sorry it's the outer one that's cross thread - see source/extensions/network/dns_resolver/getaddrinfo/getaddrinfo.cc post so I might be able to make it string view? lmk if you think it's worth the change.

RyanTheOptimist

/wait

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk · 2024-06-11T18:42:00Z

/retest

alyssawilk · 2024-06-11T19:38:55Z

done, PTAL

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk · 2024-06-12T13:47:08Z

/wait on CI

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk · 2024-06-12T18:05:48Z

/wait on CI. will ping when this is ready for a pass

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk · 2024-06-18T13:06:21Z

/retest

Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: Neal Soni <Nealsoni00@gmail.com>

Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: antoniovleonti <leonti@google.com>

Commit Message: Additional Description: I believe it is stable now since we conduct a security review and improvement for dns filter like #18651 #20744 #22861 #17479 #34409 #34456 #34490 and so on Risk Level: low Testing: Docs Changes: Release Notes: Signed-off-by: Boteng Yao <boteng@google.com>

abeyad reviewed Jun 3, 2024

View reviewed changes

alyssawilk force-pushed the dns_details branch from 17dd04c to 4532326 Compare June 3, 2024 20:05

alyssawilk commented Jun 3, 2024

View reviewed changes

Comment thread source/extensions/network/dns_resolver/apple/apple_dns_impl.cc

Comment thread source/extensions/network/dns_resolver/apple/apple_dns_impl.cc Outdated

Comment thread source/extensions/network/dns_resolver/apple/apple_dns_impl.cc Outdated

alyssawilk force-pushed the dns_details branch 2 times, most recently from 65d432e to 9db7151 Compare June 4, 2024 12:39

alyssawilk added 2 commits June 4, 2024 12:49

dns: adding resolution details

7cdfb2d

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Merge branch 'main' into dns_details

f1fd180

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk force-pushed the dns_details branch from 9db7151 to f1fd180 Compare June 4, 2024 18:39

alyssawilk marked this pull request as ready for review June 4, 2024 18:39

alyssawilk requested review from lizan, mattklein123 and yanavlasov as code owners June 4, 2024 18:39

alyssawilk assigned RyanTheOptimist Jun 4, 2024

RyanTheOptimist reviewed Jun 4, 2024

View reviewed changes

comments

703897e

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

RyanTheOptimist reviewed Jun 6, 2024

View reviewed changes

comments

b4c0367

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk commented Jun 6, 2024

View reviewed changes

RyanTheOptimist reviewed Jun 7, 2024

View reviewed changes

repokitteh-read-only Bot added the waiting label Jun 7, 2024

comments

ad0c416

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

repokitteh-read-only Bot removed the waiting label Jun 10, 2024

Merge branch 'main' into dns_details

49055a3

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Merge branch 'main' into dns_details

de36b93

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

repokitteh-read-only Bot added the waiting label Jun 12, 2024

macos fix

980c039

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

repokitteh-read-only Bot removed the waiting label Jun 12, 2024

RyanTheOptimist previously approved these changes Jun 12, 2024

View reviewed changes

v

8263f8e

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk dismissed RyanTheOptimist’s stale review via 8263f8e June 12, 2024 16:24

repokitteh-read-only Bot added the waiting label Jun 12, 2024

fixing bad merge

fe924e4

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

repokitteh-read-only Bot removed the waiting label Jun 12, 2024

alyssawilk added 3 commits June 13, 2024 12:48

Merge branch 'main' into dns_details

8e129ec

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Merge branch 'main' into dns_details

f35f203

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Merge branch 'main' into dns_details

90b23a3

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

RyanTheOptimist approved these changes Jun 18, 2024

View reviewed changes

alyssawilk merged commit 91c37ce into envoyproxy:main Jun 18, 2024

Nealsoni00 pushed a commit to Nealsoni00/envoy that referenced this pull request Jun 18, 2024

dns: adding resolution details (envoyproxy#34490)

99cf84a

Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: Neal Soni <Nealsoni00@gmail.com>

antoniovleonti pushed a commit to antoniovleonti/envoy that referenced this pull request Jun 26, 2024

dns: adding resolution details (envoyproxy#34490)

07bb155

Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: antoniovleonti <leonti@google.com>

andy-fong mentioned this pull request Sep 26, 2024

remove swift DirectResponse APIs #24428

Closed

alyssawilk deleted the dns_details branch October 9, 2024 19:53

botengyao mentioned this pull request Dec 11, 2024

dns_filter: change dns filter to stable #37618

Merged

		@@ -97,7 +97,8 @@ class DnsResolver {
		* @param status supplies the final status of the resolution.
		* @param response supplies the list of resolved IP addresses and TTLs.

Conversation

alyssawilk commented Jun 3, 2024

Uh oh!

repokitteh-read-only Bot commented Jun 3, 2024

Uh oh!

repokitteh-read-only Bot commented Jun 3, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RyanTheOptimist left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RyanTheOptimist left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RyanTheOptimist left a comment

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented Jun 11, 2024

Uh oh!

alyssawilk commented Jun 11, 2024

Uh oh!