Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle DNS queries of type MX #2041

Merged
merged 2 commits into from
Jan 9, 2018
Merged

Handle DNS queries of type MX #2041

merged 2 commits into from
Jan 9, 2018

Conversation

ddebroy
Copy link
Contributor

@ddebroy ddebroy commented Dec 20, 2017

The Docker embedded server was not handling DNS queries of type MX for containers. They were instead being passed down to the host leading to NXDOMAIN responses not compliant with SMTP RFC 5321 (in case someone wanted to host a mail server in a container and deliver emails to the server). This PR adds support to respond to MX queries in a way compliant with RFC 5321 for containers.

Note that providing a full mechanism for starting a mail server in a container/service and registering MX RRs within the Docker resolver is beyond the scope of this PR.

Signed-off-by: Deep Debroy <ddebroy@docker.com>
@ddebroy
Copy link
Contributor Author

ddebroy commented Dec 20, 2017

Addresses #2026

@thaJeztah
Copy link
Member

Do we need a unit-test for this?

@codecov-io
Copy link

codecov-io commented Dec 20, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@83862f4). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master   #2041   +/-   ##
========================================
  Coverage          ?   40.4%           
========================================
  Files             ?     138           
  Lines             ?   22141           
  Branches          ?       0           
========================================
  Hits              ?    8947           
  Misses            ?   11880           
  Partials          ?    1314
Impacted Files Coverage Δ
resolver.go 36.64% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 83862f4...3338c65. Read the comment docs.

@ddebroy
Copy link
Contributor Author

ddebroy commented Dec 20, 2017

@thaJeztah good point. I was indeed looking for unit-tests around the resolver to add something for this but could not find anything. If there is a suite already, let me know. Otherwise we will need something like resolver_test.go to cover all the name query scenarios.

@thaJeztah
Copy link
Member

Yes, I noticed the same (no _test.go); would probably be good to have, to also encourage adding tests (where they make sense of course)

return nil, nil
}

// We were able to resolve the name. Respond with an empty list with

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this comment but not sure how is it implemented below? does it mean that is enough to just reply with the same records?

Copy link
Contributor Author

@ddebroy ddebroy Dec 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are able to resolve the hostname, for MX, we need to reply with a DNS msg with empty answers section (with status set to default RcodeSuccess). createRespMsg generates the DNS msg with empty answers section and status set to RcodeSuccess. This is very similar to what we are doing down below in handleIPQuery for ipv6 AAAA querries that resolve to a ipv4 address.

@ddebroy
Copy link
Contributor Author

ddebroy commented Jan 9, 2018

I have added the unit test for resolver.

Signed-off-by: Deep Debroy <ddebroy@docker.com>
@selansen
Copy link
Collaborator

selansen commented Jan 9, 2018

LGTM

Copy link

@fcrisciani fcrisciani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants