Catch NoResponseError from Net::DNS in LoadBalancing::Resolver
What does this MR do?
Catch NoResponseError from Net::DNS in LoadBalancing::Resolver
Catch the NoResponseError rendered from Net::DNS::Resolver when it detects no repsonse from the DNS server. We do this so that we can raise our own UnresolvableNameserverError stating that there was no response. This allows us to differentiate between getting no answers and getting no response from the system's DNS servers.
Related to #271575 (closed)
The GitLab::Database::LoadBalancing::Resolver class is written to expect the user to call .resolve. .resolve will either return the first address found, or raise UnresolvableNameserverError if address is empty after exhausting all methods. "Swallowing" NoResponseError in ip_address_from_dns makes sense within this context, and is the MVC to address lack of visibility found in #271575 (closed)
The flaw addressed here actually lies within ip_address_from_dns because it can never return if answer.empty? as Net::DNS::Resolver.query will raise NoResponseError if there was no response from the systems' DNS. As no class here handles that error, the caller of .resolve will have priority, which ends up being GitLab::Database::LoadBalancing.start. The problem there, is that that class can not differentiate between failure to resolve nameserver from local resolution and failure to resolve record with that nameserver.
Here we swallow NoResponseError and raise our own in .ip_address_from_dns. Thus, when ...::LoadBalancing.start catch the error, we'll actually know it was a failure to resolve nameserver, not a failure of Consul (most common nameserver) to give us response(s) for the record.
Does this MR meet the acceptance criteria?
Conformity
-
📋 Does this MR need a changelog?-
I have included a changelog entry. -
I have not included a changelog entry because _____.
-
-
Documentation (if required) -
Code review guidelines -
Merge request performance guidelines -
Style guides -
Database guides -
Separation of EE specific content
Availability and Testing
-
Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process. -
Tested in all supported browsers -
Informed Infrastructure department of a default or new setting change, if applicable per definition of done
Security
If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:
-
Label as security and @ mention @gitlab-com/gl-security/appsec -
The MR includes necessary changes to maintain consistency between UI, API, email, or other methods -
Security reports checked/validated by a reviewer from the AppSec team