Skip to content
This repository has been archived by the owner on Oct 23, 2024. It is now read-only.

records: use SOA mname instead of rname #502

Merged
merged 4 commits into from
Jul 12, 2017
Merged

records: use SOA mname instead of rname #502

merged 4 commits into from
Jul 12, 2017

Conversation

jdef
Copy link
Contributor

@jdef jdef commented Jul 6, 2017

fixes #501

@jdef jdef requested review from gpaul and nlsun July 6, 2017 22:24
Copy link
Contributor

@gpaul gpaul left a comment

Choose a reason for hiding this comment

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

Using the mname instead of the rname is definitely correct. Would you mind adding a test to cover the failure we reported in #501 to this PR?

@jdef
Copy link
Contributor Author

jdef commented Jul 7, 2017

@gpaul I did a small bit of refactoring in a separate commit from the unit test I added. The refactoring helped to keep the test small, and more cleanly separates state loading from record generation (as it should be). When this PR lands, I'd like to keep the commit history vs. squashing it.

@jdef
Copy link
Contributor Author

jdef commented Jul 11, 2017

There's another, related issue that we could solve as part of this ticket: if the SOA mname isn't part of the mesos-dns domain, then mesos-dns SHALL NOT report an A RR. We don't perform any checks for this (https://github.com/mesosphere/mesos-dns/blob/9d21ea6de38bd371e34bde196ceaee94cdac8a2c/records/generator.go#L478).

See the examples here: http://www.zytrax.com/books/dns/ch8/soa.html

We should either:
(a) validate that the SOA mname is within the mesos-dns domain, or else;
(b) NOT generate A RR's for an mname that is outside the scope of the mesos-dns domain

Should I write up a separate ticket for this, or just fix as part of this one? @gpaul

@jdef
Copy link
Contributor Author

jdef commented Jul 11, 2017

answered my own question re: the above: opened issue #504 to track additional SOA mname rules

Copy link
Contributor

@gpaul gpaul left a comment

Choose a reason for hiding this comment

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

LGTM, nice one

@jdef jdef merged commit a32855c into master Jul 12, 2017
@jdef jdef deleted the jdef_fix_501 branch July 12, 2017 12:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ns namaserver's A record is not returned
2 participants