apple dns: remove sharing UDS to local DNS daemon#17226
apple dns: remove sharing UDS to local DNS daemon#17226mattklein123 merged 7 commits intoenvoyproxy:mainfrom junr03:apple-dns-simple
Conversation
Signed-off-by: Jose Nino <jnino@lyft.com>
|
cc @Reflejo |
Signed-off-by: Jose Nino <jnino@lyft.com>
mattklein123
left a comment
There was a problem hiding this comment.
Thanks this is much much simpler. I have one question. It would be good for @Reflejo to take a look also.
/wait-any
| // If the reference's underlying socket is used in a run loop or select() call, it should | ||
| // be removed BEFORE DNSServiceRefDeallocate() is called, as this function closes the | ||
| // reference's socket. | ||
| sd_ref_event_.reset(); |
There was a problem hiding this comment.
what happens here if sd_ref_event_ was never initialized? (because fd was -1, etc) would this no-op or crash?
There was a problem hiding this comment.
it would no-op. std::unique_ptr::reset() destroys the currently managed object if any.
| // thus the DNSServiceRef is null. | ||
| // Therefore, only deallocate if the ref is not null. | ||
| if (individual_sd_ref_) { | ||
| if (sd_ref_) { |
There was a problem hiding this comment.
mh, if DNSServiceGetAddrInfo can, in fact, null out sd_ref_, is there any path where https://github.com/envoyproxy/envoy/pull/17226/files#diff-2a0d7d0f6663ada23d2b1acdedd05b116e96a3b518db0a3d300bbe9b666ad816R146 would be called with a null ref? or current ordering of operation would never allow that?
There was a problem hiding this comment.
That would be a programming error if it happened because:
a) We don't register the file event onto the event loop in case of synchronous error
b) Cancelation removes destructs the file event synchronously, which prevents the callback you linked from firing.
| ENVOY_LOG(debug, "Address to add address={}, ttl={}", | ||
| dns_response.address_->ip()->addressAsString(), ttl); | ||
| pending_cb_.responses_.push_back(dns_response); | ||
| } |
There was a problem hiding this comment.
To matt's comment: What happens on the else here? aka flags & kDNSServiceFlagsAdd == 0 which implies "removal"?
There was a problem hiding this comment.
If a successfully-registered name later suffers a name conflict or similar problem and has to be deregistered, the callback will be invoked with the kDNSServiceFlagsAdd flag not set.
There was a problem hiding this comment.
@mattklein123 consolidating your question here to keep everything in one place.
The current resolver API is purely additive and up until now we have let the consumer of the resolver (e.g, logical dns cluster, dns cache, etc.) deal with the result from the query as they see fit. So for instance the logical DNS cluster will take the first address in the response, and replace the existing value if it is different (or one doesn't exist). So removal is implicit and is based on the consumer's behavior.
@mattklein123 I don't understand what you mean by
Will it always "add" things that have already been added?
do you mind expanding on that?
There was a problem hiding this comment.
What I mean is the apple API seems to internally support add/remove. Do we have to handle removal? Or because we are using a new connection for each query it doesn't matter? Can you add more comments on that?
There was a problem hiding this comment.
Oh, I think I see what you mean now @mattklein123. Given that the API handles removal, it seems to internally track what has happened with a particular DNS name in the past and keep state. So the concern is that potentially two independent queries from consumers of the resolver would interfere with each other. For instance, potentially a subsequent query would only receive a "remove" response and end up with no addresses, because the resolution doesn't always "add" things that have already been added, as you mentioned originally.
Interesting. Let me read through the API docs again.
|
@Reflejo @mattklein123 responded to your comments. Lmk what you think |
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
|
@mattklein123 if you could stamp again that'd be great. I had to merge main. |
|
/retest |
|
Retrying Azure Pipelines: |
Description: Includes three relevant commits: envoyproxy/envoy#17226 envoyproxy/envoy#17274 envoyproxy/envoy#17207 Signed-off-by: Jose Nino <jnino@lyft.com>
Description: Includes three relevant commits: envoyproxy/envoy#17226 envoyproxy/envoy#17274 envoyproxy/envoy#17207 Signed-off-by: Jose Nino <jnino@lyft.com> Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Description: Includes three relevant commits: envoyproxy/envoy#17226 envoyproxy/envoy#17274 envoyproxy/envoy#17207 Signed-off-by: Jose Nino <jnino@lyft.com> Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
Commit Message: apple dns - remove sharing UDS to local DNS daemon
Additional Description: reduce the code complexity.
Risk Level: med - although only affects Apple based deployments
Testing: updated test suite
Signed-off-by: Jose Nino jnino@lyft.com