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
-
-
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
Merge request reports
Activity
marked the checklist item Separation of EE specific content as completed
added databasereview pending label
added database label
-
1 Message 
This merge request adds or changes files that require a review from the Database team. This merge request requires a database review. To make sure these changes are reviewed, take the following steps:
- Ensure the merge request has database and databasereview pending labels. If the merge request modifies database files, Danger will do this for you.
- Prepare your MR for database review according to the docs.
- Assign and mention the database reviewer suggested by Reviewer Roulette.
The following files require a review from the Database team:
ee/lib/gitlab/database/load_balancing/resolver.rb
Reviewer roulette
Changes that require review have been detected! A merge request is normally reviewed by both a reviewer and a maintainer in its primary category (e.g. frontend or backend), and by a maintainer in all other categories.
To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot, based on their timezone. Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.
To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.
Once you've decided who will review this merge request, assign them as a reviewer! Danger does not automatically notify them for you.
Category Reviewer Maintainer backend Marc Shaw ( @marc_shaw) (UTC+13, 17 hours ahead of@WarheadsSE)Nick Thomas ( @nick.thomas) (UTC+0, 4 hours ahead of@WarheadsSE)database Alex Ives ( @alexives) (UTC-5, 1 hour behind@WarheadsSE)Steve Abrams ( @sabrams) (UTC-6, 2 hours behind@WarheadsSE)If needed, you can retry the
danger-reviewjob that generated this comment.Generated by
Edited by 🤖 GitLab Bot 🤖 requested review from @stanhu
mentioned in issue #271575 (closed)
requested review from @WarheadsSE and removed review request for @stanhu
removed review request for @WarheadsSE
assigned to @WarheadsSE
changed milestone to %13.11
added typemaintenance label
added typefeature label
-
Setting label groupdistribution based on
@WarheadsSE's group. added groupdistribution label
requested review from @stanhu
added backend label
Note: failure in https://gitlab.com/gitlab-org/gitlab/-/jobs/1125662919 has no impact on this MR. We know the cause of that.
added 942 commits
-
6ea7f356...b4bf4e7a - 941 commits from branch
master - 3e33c161 - Catch NoResponseError from Net::DNS in LoadBalancing::Resolver
-
6ea7f356...b4bf4e7a - 941 commits from branch
enabled an automatic merge when the pipeline for 6bf4b23a succeeds
mentioned in commit 97a3733f
added workflowstaging label
added workflowproduction label and removed workflowstaging label
added releasedcandidate label
removed typefeature label
mentioned in issue #364370 (closed)
