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

plugin/transfer: Zone transfer plugin #3223

Merged
merged 24 commits into from Nov 1, 2019

Conversation

@chrisohaver
Copy link
Member

chrisohaver commented Aug 28, 2019

1. Why is this pull request needed and what does it do?

New plugin transfer implementing the following parts of #3121

  • AXFR
  • Simple IXFR with AXFR fallback
  • Define allowed transfer to hosts

NOTIFY is not implemented in this PR.

2. Which issues (if any) are related?

#3121 #2585

3. Which documentation changes (if any) need to be made?

Included

4. Does this introduce a backward incompatible change or deprecation?

@chrisohaver

This comment has been minimized.

Copy link
Member Author

chrisohaver commented Aug 28, 2019

Status:

  • Basic AXFR structure in place.
  • Simple IXFR handling (fallback AXFR)

Todo:

  • Notify (in a following PR)
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Aug 28, 2019

Codecov Report

Merging #3223 into master will increase coverage by 0.18%.
The diff coverage is 67.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3223      +/-   ##
==========================================
+ Coverage   56.34%   56.52%   +0.18%     
==========================================
  Files         218      220       +2     
  Lines       10832    10972     +140     
==========================================
+ Hits         6103     6202      +99     
- Misses       4253     4287      +34     
- Partials      476      483       +7
Impacted Files Coverage Δ
plugin/transfer/setup.go 50.81% <50.81%> (ø)
plugin/transfer/transfer.go 79.74% <79.74%> (ø)
plugin/forward/connect.go 85.91% <0%> (+4.22%) ⬆️
plugin/file/reload.go 75% <0%> (+5.55%) ⬆️

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 6f375cb...641d1e3. Read the comment docs.

@chrisohaver

This comment has been minimized.

Copy link
Member Author

chrisohaver commented Aug 29, 2019

I copied chunks of the transfer code from the file implementation. I see that #3227 modifies/reworks some of that. I'll pull those changes in here when it merges.

type Transferer interface {
// AllRecords must return the SOA and all other records in the zone, including NS records
// for the given zone.
AllRecords(zone string) (*dns.SOA, []dns.RR, error)

This comment has been minimized.

Copy link
@miekg

miekg Aug 31, 2019

Member

This is, I think, not the correct interface. It needs to be more like an iterator.

type Transferer interface {
   Walk(zone string, serial uint32) ([]dns.RR, error)
}
~~~
Or something like that, see latest tweaks in `plugin/file/xfr.go`
Two things that:
1. First RR returned must be a SOA
2. `io.EOF` signals the end of the list

This comment has been minimized.

Copy link
@johnbelamaric

johnbelamaric Sep 3, 2019

Member

What about returning a channel?

This comment has been minimized.

Copy link
@chrisohaver

chrisohaver Sep 3, 2019

Author Member

Ah, a channel, so we can start responding to the client in parallel with the plugin generating the records. Yeah, makes sense.

AllRecords(zone string) (*dns.SOA, []dns.RR, error)

// SoaSerial must return the current serial for the given zone.
SoaSerial(zone string) (uint32, error)

This comment has been minimized.

Copy link
@miekg

miekg Aug 31, 2019

Member

Probably can be folded into Walk as I did above

}
if x.Transferers[z] != nil {
// Acknowledge only the first transferer for each zone.
// This doesn't play nice with fallthrough, where more than one plugin can

This comment has been minimized.

Copy link
@johnbelamaric

johnbelamaric Sep 3, 2019

Member

Would be nice to see if a later version could do something for this case.

This comment has been minimized.

Copy link
@chrisohaver

chrisohaver Sep 3, 2019

Author Member

Yes, I thought about this and decided it was complicated enough to leave out for now, to keep this PR small and easily reviewable. I think it's solvable. The transfer plugin should be able to determine the "order" of plugins and merge them down with the records of other plugins later in the chain, preferring records from earlier plugins when there are collisions. Multi instance plugins would need be handled by each plugin's implementation of transfer.

plugin/kubernetes/xfr.go Outdated Show resolved Hide resolved
@miekg

This comment has been minimized.

Copy link
Member

miekg commented Sep 5, 2019

I would prefer the entire xfr impl of k8s_external to be in another PR (I get the usefulness of testing your transfer impl, but it muddles this pr)

// AllRecords writes all records in the zone to the channel. The
// first and last records written to the channel must be the SOA for the zone.
// If serial is non zero, an incremental zone transfer should be assumed.
AllRecords(zone string, serial uint32, ch chan dns.RR)

This comment has been minimized.

Copy link
@miekg

miekg Sep 5, 2019

Member

I don't like this interface yet. I think returning a channel is more natural, also the channel should be recieve only, not the send/receive it is now.
I.e.

Transfer(zone string, serial uint32) (ch <-chan []dns.RR)

This also means the plugin closes the channel after is sent the last rrrs, which is more natural, not sure how that would work now.
And allowing a slice is also way more efficient, because that's probably what most plugins are holding anyway. Also sending the SOA twice seems axfr implemention detail and not super relevant to the implementation. The transfer plugin can take care of that.

func (Transfer) Name() string { return pluginName }

const (
pluginName = "transfer"

This comment has been minimized.

Copy link
@miekg

miekg Sep 5, 2019

Member

meh, please just use "transfer" the few times it's need in the code.

state := request.Request{W: w, Req: r}

var serial uint32
if state.QType() == dns.TypeIXFR {

This comment has been minimized.

Copy link
@miekg

miekg Sep 5, 2019

Member

this needs the ixfr handling that was recently added in file's tranfer

This comment has been minimized.

Copy link
@chrisohaver

chrisohaver Sep 6, 2019

Author Member

I am currently handling this in the plugin implementations, to allow them to implement non-axfr-fallback if they could. But implementing non-axfr-fallback is probably too much to expect of a plugin, and most plugins probably woudn't bother (the full ixfr answer protocol being a tad fiddly). So, yeah , makes more practical sense to move the simple ixfr handling to the transfer plugin, and later we can extend the transfer plugin, perhaps with an IncrTransferer interface down the road.

This comment has been minimized.

Copy link
@miekg

miekg Sep 18, 2019

Member

Yep, that's was my thinking why AllRecords would be a bad name for the functions in the interfaces. Now we can extend this and keep a similar naming scheme

return dns.RcodeServerFailure, nil
}
serial = soa.Serial
} else if state.QType() != dns.TypeAXFR {

This comment has been minimized.

Copy link
@miekg

miekg Sep 5, 2019

Member

switch is probably naturally

@chrisohaver chrisohaver force-pushed the chrisohaver:xfrplug branch 2 times, most recently from 643cd73 to 1e9d15d Sep 6, 2019
@chrisohaver

This comment has been minimized.

Copy link
Member Author

chrisohaver commented Sep 6, 2019

@miekg, I think I got all of your feedback thus far rolled in...

  • remove kubernetes and k8s_external implementations from this PR
  • use RR slice in the interface channel. create and close the channel in sender.
  • use literal instead of constant for plugin name
  • move simple ixfr response logic from plugin implementations to transfer plugin
  • use a switch instead of if in ServeDNS

Let me know if you'd like me to squash the commits.

@chrisohaver chrisohaver force-pushed the chrisohaver:xfrplug branch from bb3b1aa to 4af2669 Sep 9, 2019
@miekg

This comment has been minimized.

Copy link
Member

miekg commented Sep 10, 2019

move the k8s transfer impl to a new pr.

Allrecords is oddly named and doesnt allow for incremental transfers to be
added, unless you go with some super weird naming scheme. SOA should not be
handled separately. And I don't think authoritative is needed as a method, you can just call Transfer and see what you get.
Probably best to let Transfer also return an predefined error or DNS rcode, to signal this

@chrisohaver

This comment has been minimized.

Copy link
Member Author

chrisohaver commented Sep 10, 2019

move the k8s transfer impl to a new pr.

@miekg, It's already removed from this PR ... Do you want me to open a new PR in parallel before this one is finalized?

@chrisohaver

This comment has been minimized.

Copy link
Member Author

chrisohaver commented Sep 10, 2019

@miekg,

renamed AllRecords() -> Transfer() (per your earlier suggestion)
removed SOA() and Authoritative()

@chrisohaver chrisohaver force-pushed the chrisohaver:xfrplug branch from 3c57370 to 7358592 Sep 10, 2019
chrisohaver added 17 commits Sep 11, 2019
Signed-off-by: Chris O'Haver <cohaver@infoblox.com>
Signed-off-by: Chris O'Haver <cohaver@infoblox.com>
Signed-off-by: Chris O'Haver <cohaver@infoblox.com>
Signed-off-by: Chris O'Haver <cohaver@infoblox.com>
Signed-off-by: Chris O'Haver <cohaver@infoblox.com>
Signed-off-by: Chris O'Haver <cohaver@infoblox.com>
Signed-off-by: Chris O'Haver <cohaver@infoblox.com>
Signed-off-by: Chris O'Haver <cohaver@infoblox.com>
Signed-off-by: Chris O'Haver <cohaver@infoblox.com>
Signed-off-by: Chris O'Haver <cohaver@infoblox.com>
Signed-off-by: Chris O'Haver <cohaver@infoblox.com>
Signed-off-by: Chris O'Haver <cohaver@infoblox.com>
Signed-off-by: Chris O'Haver <cohaver@infoblox.com>
Signed-off-by: Chris O'Haver <cohaver@infoblox.com>
Signed-off-by: Chris O'Haver <cohaver@infoblox.com>
Signed-off-by: Chris O'Haver <cohaver@infoblox.com>
Signed-off-by: Chris O'Haver <cohaver@infoblox.com>
@chrisohaver chrisohaver force-pushed the chrisohaver:xfrplug branch from 5989251 to 5c82104 Oct 21, 2019
Signed-off-by: Chris O'Haver <cohaver@infoblox.com>
@chrisohaver

This comment has been minimized.

Copy link
Member Author

chrisohaver commented Oct 22, 2019

FWIW - I've got notify + k8s_external working in locally in a separate set of branches - tested with a NIOS dns grid member as the secondary. I added Notify to the existing Transferer interface, which I think is cleaner than to create a new interface for it. One change from the proposed solution above is that the message only contains zone (serial not needed when sending a notify).

@chrisohaver

This comment has been minimized.

Copy link
Member Author

chrisohaver commented Oct 25, 2019

OK to merge?

// in the zone to the channel. The SOA should be written to the channel first, followed
// by all other records, including all NS + glue records.
//
// If serial is not 0, handle as an IXFR request. If the serial is >

This comment has been minimized.

Copy link
@johnbelamaric

johnbelamaric Oct 30, 2019

Member

Was that supposed to be <?

This comment has been minimized.

Copy link
@chrisohaver

chrisohaver Oct 30, 2019

Author Member

yep thanks! actually it's supposed to be >=, and the next comparison <. I got them swapped, thanks for drawing my attention to it.

Signed-off-by: Chris O'Haver <cohaver@infoblox.com>
@chrisohaver chrisohaver requested a review from johnbelamaric Oct 30, 2019
@johnbelamaric

This comment has been minimized.

Copy link
Member

johnbelamaric commented Oct 30, 2019

/lgtm

@corbot
corbot bot approved these changes Oct 30, 2019
Copy link

corbot bot left a comment

LGTM by johnbelamaric

@chrisohaver

This comment has been minimized.

Copy link
Member Author

chrisohaver commented Oct 31, 2019

@miekg, I think all comments have been addressed and this is ready to go. I'd like to merge this shortly unless you have any objections.

@chrisohaver chrisohaver merged commit a7ab592 into coredns:master Nov 1, 2019
3 of 4 checks passed
3 of 4 checks passed
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
DCO DCO
Details
ci/circleci: kubernetes-tests Your tests passed on CircleCI!
Details
stickler-ci No lint errors found.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.