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

Ensure TXT RRs always end up in the Additional section except for ANY or TXT queries #4362

Merged
merged 1 commit into from
Jul 10, 2018

Conversation

mkeeler
Copy link
Member

@mkeeler mkeeler commented Jul 9, 2018

Fixes #4354

This also changes where the enforcement of the enable_additional_node_meta_txt configuration gets applied.

formatNodeRecord returns the main RRs and the meta/TXT RRs in separate slices. Its then up to the caller to add to the appropriate sections or not.

Copy link
Member

@banks banks left a comment

Choose a reason for hiding this comment

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

Assuming that you ran full test suite and we don't have any other calls into this method I think this looks good.

A couple of minor comments inline if you agree.

agent/dns.go Outdated
@@ -620,7 +628,7 @@ func encodeKVasRFC1464(key, value string) (txt string) {
}

// formatNodeRecord takes a Node and returns an A, AAAA, TXT or CNAME record
func (d *DNSServer) formatNodeRecord(node *structs.Node, addr, qName string, qType uint16, ttl time.Duration, edns, answer bool) (records []dns.RR) {
func (d *DNSServer) formatNodeRecord(node *structs.Node, addr, qName string, qType uint16, ttl time.Duration, edns bool) (records []dns.RR, meta []dns.RR) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: you don't need to repeat the type if it's the same, It's idiomatic to write that as records, meta []dns.RR

agent/dns.go Outdated
@@ -620,7 +628,7 @@ func encodeKVasRFC1464(key, value string) (txt string) {
}

// formatNodeRecord takes a Node and returns an A, AAAA, TXT or CNAME record
Copy link
Member

Choose a reason for hiding this comment

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

nit: I'd update the doc comment to describe the semantics of the two returned slices.

… or TXT queries

This also changes where the enforcement of the enable_additional_node_meta_txt configuration gets applied.

formatNodeRecord returns the main RRs and the meta/TXT RRs in separate slices. Its then up to the caller to add to the appropriate sections or not.
@mkeeler
Copy link
Member Author

mkeeler commented Jul 9, 2018

Ran full suite: 3 tests failed. Ran those tests individually and they passed.

So all tests CAN pass.

@mkeeler mkeeler added this to the 1.2.1 milestone Jul 9, 2018
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.

2 participants