dns: removing exceptions from resolver#34409
Conversation
alyssawilk
requested review from
lizan,
mattklein123 and
yanavlasov
as code owners
|
Yanjun -- can you do a first pass? After your lgtm I'll assign a maintainer. |
| const envoy::extensions::network::dns_resolver::cares::v3::CaresDnsResolverConfig& config, | ||
| Event::Dispatcher& dispatcher, | ||
| const std::vector<Network::Address::InstanceConstSharedPtr>& resolvers, | ||
| Event::Dispatcher& dispatcher, absl::optional<std::string> resolvers_csv, |
There was a problem hiding this comment.
Just curious what's the motivation for this change?
There was a problem hiding this comment.
envoyproxy/envoy-mobile#176 :-)
but more relevant to Envoy, there's also #27412
| } | ||
| } | ||
| return std::make_shared<Network::DnsResolverImpl>(cares, dispatcher, resolvers, | ||
| auto csv_or_error = DnsResolverImpl::maybeBuildResolversCsv(resolvers); |
There was a problem hiding this comment.
It looks to me there are some logic change here. Previously, if resolvers is empty, the code still go ahead create DnsResolverImpl:
.Now we skipped it.
There was a problem hiding this comment.
I don't think so
we only fail if we have an error status. in the case of empty list, we still return absl::nullopt ("no resolvers") not absl::InvalidArgumentError ("failure creating configured resolvers")
There was a problem hiding this comment.
I see. That static function DnsResolverImpl::maybeBuildResolversCsv() never returns Error currently.
But, it throw exceptions here:
Should we change this throw into a return error?
There was a problem hiding this comment.
yeah I had set up to remove it and not yet removed it. Happy to do so (fingers crossed on unit tests, else I'll pick back up monday)
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
| return absl::InvalidArgumentError("Redis cluster can only created with redis cluster type."); | ||
| } | ||
| auto resolver = | ||
| THROW_OR_RETURN_VALUE(selectDnsResolver(cluster, context), Network::DnsResolverSharedPtr); |
There was a problem hiding this comment.
nit: can we use THROW_IF_NOT_OK() here to be consistent with logical/strict cluster? It's up to you.
There was a problem hiding this comment.
I think THROW_IF_NOT_OK is significantly more readable than THROW_OR_RETURN_VALUE, too (i.e. consistency is nice, and my preference is for consistency to be achieved in that direction.)
|
LGTM modulo CI error. |
|
@ravenblackx for non-google review (just realized I don't need a senior maintainer since Alyssa is). |
|
yeah I prefer THROW_OR_SET_VALUE to |
<!-- !!!ATTENTION!!! If you are fixing *any* crash or *any* potential security issue, *do not* open a pull request in this repo. Please report the issue via emailing envoy-security@googlegroups.com where the issue will be triaged appropriately. Thank you in advance for helping to keep Envoy secure. !!!ATTENTION!!! For an explanation of how to fill out the fields, please see the relevant section in [PULL_REQUESTS.md](https://github.com/envoyproxy/envoy/blob/main/PULL_REQUESTS.md) --> 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>
5 participants
Risk Level: low
Testing: updated tests
Docs Changes: n/a
Release Notes: n/a
envoyproxy/envoy-mobile#176