Add PDNS as a Provider #373

ffledgling · 2017-10-30T16:01:55Z

This is the first commit in what is likely going to be a bunch of commits. It adds pdns-go bindings to the vendor directory, that will let us the pdns provider to external-dns. (Issue #363 )

k8s-ci-robot · 2017-10-30T16:01:58Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-ci-robot · 2017-10-30T17:11:20Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

njuettner · 2017-11-01T16:15:25Z

@ffledgling Thanks for your PR!
It would be nice if you could rebase without the vendor commit as soon as everything is in place, so we can review your changes without going to commits one by one.

ffledgling · 2017-11-01T18:53:20Z

@njuettner Thanks for taking a look! This PR is in a bit of flux at the moment and I should've marked it as incomplete. I'll be updating this in the upcoming days, the vendoring, external package and a lot of other things have been changing very quickly.

When it's done I'll update either via a comment here or via Slack. Cheers!

ffledgling · 2017-11-06T17:06:53Z

I've updated this commit and it now implements the pdns provider fully (for the most part).

Some notes:

It relies on a swagger generated client which is currently under provider/internal/pdns-go. This addition is in a separate commit to make reviewing easier.
Ideally this can be added as an external vendored dependency and managed with glide. But I ran into issues getting glide to work correctly on my end.
Once the code is reviewed and OK'd I can spend more time trying to deal with glide and dependency management to get the dependency moved out of the code and into glide/vendor.

ideahitme

in general looks okay, but tests are really needed to have major functions covered for provider/pdns.go file.

ideahitme · 2017-11-08T14:12:01Z

pkg/apis/externaldns/types.go

@@ -129,7 +133,7 @@ func (cfg *Config) ParseFlags(args []string) error {
 	app.Flag("publish-internal-services", "Allow external-dns to publish DNS records for ClusterIP services (optional)").BoolVar(&cfg.PublishInternal)

 	// Flags related to providers
-	app.Flag("provider", "The DNS provider where the DNS records will be created (required, options: aws, google, azure, cloudflare, digitalocean, dnsimple, infoblox, inmemory)").Required().PlaceHolder("provider").EnumVar(&cfg.Provider, "aws", "google", "azure", "cloudflare", "digitalocean", "dnsimple", "infoblox", "inmemory")
+	app.Flag("provider", "The DNS provider where the DNS records will be created (required, options: aws, google, azure, cloudflare, digitalocean, dnsimple, infoblox, inmemory)").Required().PlaceHolder("provider").EnumVar(&cfg.Provider, "aws", "google", "azure", "cloudflare", "digitalocean", "dnsimple", "infoblox", "inmemory", "pdns")


please also mention it in the options list (same line)

ideahitme · 2017-11-08T14:23:58Z

provider/pdns.go

+	defaultServerID = "localhost"
+	defaultTTL      = 300
+
+	maxUInt32 = ^uint32(0)


this is already defined in SL: math.MaxUint32 and math.MaxInt32 respectively.

ideahitme · 2017-11-08T14:28:29Z

provider/pdns.go

+	// We do not support dry running, exit safely instead of surprising the user
+	// TODO: Add Dry Run support
+	if dryRun {
+		log.Fatalf("PDNS Provider does not currently support dry-run, stopping.")


why this can't be returned as an error instead. It would fatal in the main then, which I find slightly preferable

ideahitme · 2017-11-08T14:29:28Z

provider/pdns.go

+	}
+
+	if apikey == "" {
+		log.Warnf("API Key for PDNS is empty. Specify using --pdns-api-key=")


does it work without specifying apikey? if not then this should return an error

ideahitme · 2017-11-08T14:36:33Z

provider/pdns.go

+		log.Warnf("API Key for PDNS is empty. Specify using --pdns-api-key=")
+	}
+	if len(domainFilter.filters) == 0 {
+		log.Warnf("Domain Filter is not supported by PDNS. It will be ignored.")


would it be safer just to quit if domain filter is passed ?

ideahitme · 2017-11-08T14:40:24Z

provider/pdns.go

+package provider
+
+import (
+	//"strings"


this should really be removed

ideahitme · 2017-11-08T14:41:48Z

provider/pdns.go

+	endpoints = []*endpoint.Endpoint{}
+
+	for _, record := range rr.Records {
+		//func NewEndpointWithTTL(dnsName, target, recordType string, ttl TTL) *Endpoint


please remove commented code

ideahitme · 2017-11-08T15:36:53Z

provider/pdns.go

+		return err
+	}
+	for _, zone := range zonelist {
+		jso, _ := json.Marshal(zone)


the error here should be handled

ideahitme · 2017-11-08T15:38:30Z

provider/pdns.go

+		}
+	    }
+	*/
+	mastermap := make(map[string]map[string]map[string][]*endpoint.Endpoint)


is it possible to refactor this code, so that we don't have triple nested maps ?

@ffledgling could you take a look on that again?

I have a local copy of this patched, but this is blocked on mock testing which is exhibiting segfaults, I'm trying to still fix this. 👍

ideahitme · 2017-12-21T10:44:35Z

what is the status here ?

ffledgling · 2017-12-27T17:24:13Z

@ideahitme This has fallen by the wayside for a bit because some other stuff came up, but I'll try to clean it up and send it in by the second/third week of January.

I have addressed most of your concerns in the review, but I need to add tests, which requires a bit of refactoring.

ideahitme · 2017-12-28T10:57:38Z

@ffledgling sure, ping me when it is ready

fxpester · 2018-01-16T17:18:03Z

hi, I`m trying to build it, but no luck, can you check it ?

go version go1.8.5 linux/amd64
git clone -b 363-pdns https://github.com/ffledgling/external-dns/
cd external-dns
make

errors I get:

k8s.io/client-go/plugin/pkg/client/auth
_/root/external-dns
# _/root/external-dns
./main.go:95: not enough arguments in call to provider.NewAWSProvider
        have (provider.DomainFilter, provider.ZoneTypeFilter, bool)
        want (provider.DomainFilter, provider.ZoneIDFilter, provider.ZoneTypeFilter, bool)
./main.go:97: not enough arguments in call to provider.NewAzureProvider
        have (string, provider.DomainFilter, string, bool)
        want (string, provider.DomainFilter, provider.ZoneIDFilter, string, bool)
./main.go:99: not enough arguments in call to provider.NewCloudFlareProvider
        have (provider.DomainFilter, bool, bool)
        want (provider.DomainFilter, provider.ZoneIDFilter, bool, bool)
./main.go:101: not enough arguments in call to provider.NewGoogleProvider
        have (string, provider.DomainFilter, bool)
        want (string, provider.DomainFilter, provider.ZoneIDFilter, bool)
./main.go:105: not enough arguments in call to provider.NewDnsimpleProvider
        have (provider.DomainFilter, bool)
        want (provider.DomainFilter, provider.ZoneIDFilter, bool)
./main.go:122: undefined: provider.NewPDNSProvider
./main.go:122: cfg.PDNSServer undefined (type *externaldns.Config has no field or method PDNSServer)
./main.go:122: cfg.PDNSAPIKey undefined (type *externaldns.Config has no field or method PDNSAPIKey)
make: *** [build/external-dns] Error 2

ffledgling · 2018-01-16T17:51:01Z

@fxpester That shouldn't happen, looks like something's out of sync. I'll take a look when I get in tomorrow.

k8s-ci-robot · 2018-01-17T14:59:30Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

linki · 2018-02-12T15:40:16Z

Merged-in current master which uses dep now.

ffledgling · 2018-02-12T18:12:52Z

@linki I noticed, I'll switch over to dep.

ffledgling · 2018-02-20T09:30:38Z

While I work on this, here's the binary with the pdns bits built in for those who wish to play around with it - https://pastebin.ffledgling.com/external-dns/ (Please do not use this in production).

ffledgling · 2018-03-22T12:57:50Z

@ideahitme @njuettner I think this is ready for review on my end, can you please take a look? I think I've addressed all prior concerns here.

Raffo

I added some comments, mostly covering code comments and a bit of error handling.

Raffo · 2018-03-29T15:44:35Z

provider/pdns.go

+	"context"
+	"encoding/json"
+	"errors"
+	//"fmt"


nitpick: Can we get rid of this?

Raffo · 2018-04-04T15:44:11Z

provider/pdns.go

+
+// ConvertEndpointsToZones marshals endpoints into pdns compatible Zone structs
+func (p *PDNSProvider) ConvertEndpointsToZones(eps []*endpoint.Endpoint, changetype pdnsChangeType) (zonelist []pgo.Zone, _ error) {
+	/* eg of mastermap


Is this a leftover for developing purpose? If not, I would move it to the comment of the method, otherwise just drop it.

It's a stale comment from a previous implementation, I'll remove it.

Raffo · 2018-04-04T15:44:53Z

provider/pdns.go

+
+	sort.SliceStable(zones, func(i, j int) bool { return len(zones[i].Name) > len(zones[j].Name) })
+
+	// NOTE: Complexity of this loop is O(Zones*Endpoints).


Love that you mention the complexity here, might be useful in the future.

Raffo · 2018-04-04T15:47:01Z

provider/pdns.go

+			ep := endpoints[0]
+			dnsname := ensureTrailingDot(ep.DNSName)
+			if strings.HasSuffix(dnsname, zone.Name) {
+				// The assumption here is that there will only ever be one target


The PR is against master, so that shouldn't be a problem, am I right? If so, it is not a concern.

Indeed, this is just here as a note in case it changes in the future and results in a breakage. It's an assumption, so I thought to document it.

Raffo · 2018-04-04T15:49:50Z

provider/pdns.go

+	for _, zone := range zonelist {
+		jso, err := json.Marshal(zone)
+		if err != nil {
+			log.Debugf("JSON Marshal for zone struct failed!")


I think we should return the error if we fail to unmarshal. It's not so much of a common problem but I think that just logging is pretty dangerous. Also, the log is at debug level which means that it will pass completely unnoticed in case with a lower debug level.

Raffo · 2018-04-04T15:55:30Z

provider/pdns.go

+func (c *PDNSAPIClient) PatchZone(zoneID string, zoneStruct pgo.Zone) (*http.Response, error) {
+	resp, err := c.client.ZonesApi.PatchZone(c.authCtx, defaultServerID, zoneID, zoneStruct)
+	if err != nil {
+		log.Warnf("Unable to patch zone %s. %v", zoneStruct.Name, err)


Is it okay to only log this error?

@Raffo Would you prefer this be a hard fail instead? Or are you suggesting it should be logged at level info ?

I think it makes sense to return an error here, in the end we want to know that something went wrong. Also, I see that when we call p.mutateRecords we don't actually have any check for the error... is that intended and do I look at it right? If it is not intended, maybe a run of a linter would help to spot those cases.

The reason I left it as a warning because network blips might result in one-off failures of the request, so it might make sense to retry instead of hard failing the first time we see an error. An alternative of-course would be to add retry logic w/ back-off so that we hard fail if we see no response after say 3 tries.

please return errors and because api-sdks like aws do retries, you might want to do the retry inside the pdns provider.

This now retries thrice and returns an error otherwise.

Raffo · 2018-04-04T15:55:41Z

provider/pdns.go

+func (c *PDNSAPIClient) ListZones() ([]pgo.Zone, *http.Response, error) {
+	zones, resp, err := c.client.ZonesApi.ListZones(c.authCtx, defaultServerID)
+	if err != nil {
+		log.Warnf("Unable to fetch zones. %v", err)


Is it okay to only log this error?

njuettner · 2018-04-05T09:03:59Z

provider/pdns.go

+		jso, err := json.Marshal(zone)
+		if err != nil {
+			log.Debugf("JSON Marshal for zone struct failed!")
+		} else {


does this has to be in a else clause?

as Raffo mentioned if you return the err

So the problem is that the JSON marshal itself is not used for anything except debug logging itself, does it still make sense to return an error? We'd be throwing an error for something that failed in our own logging setup. I'm still fine with returning an error, just making sure that's the intention.

njuettner · 2018-04-05T09:11:26Z

provider/pdns.go

+	startTime := time.Now()
+
+	// Create
+	for _, change := range changes.Create {


I would check if debug is true before, otherwise it will loop for nothing everytime the method got called if you don't debug it. Same for Update and Delete, @ffledgling thoughts?

Good catch! I'll update code accordingly.

changes.Create could also implement Stringer interface String() string and then plan.Changes has to know about debug yes/no.

Ignoring this change for now because short-circuiting on debug will require a change in the PDNS provider creation struct, which will require a sizable refactor of the code and tests, which after discussion on IRC with @szuecs and @njuettner is more trouble than it's worth at the moment.

njuettner · 2018-04-05T09:42:12Z

@ffledgling I've resolved the conflict in types.go, besides that could you take a look again what Raffo was mentioning to you?

I think after that we're happy to merge it. Thanks again!

njuettner · 2018-04-13T11:29:39Z

@ffledgling did you had time to take a look? I think there's only a minor part missing here.

Commit adds: * Implementation of PowerDNS as a provider * Tests for said implementation * github.com/ffledgling/pdns-go, which provides go client bindings for PowerDNS's HTTP API, as a dependency * "pdns" as an additional option for the `--provider` flag * `--pdns-server` and `--pdns-api-key` as additional flags for PowerDNS specific configuration

njuettner · 2018-04-16T14:17:11Z

👍

tuapuikia · 2019-02-17T15:30:19Z

Is it possible to include basic auth authentication?

ffledgling · 2019-02-20T12:11:06Z

@tuapuikia what do you mean by basic auth authentication? The provider already supports a password protected PDNS API instance. Did you have something else in mind?

tuapuikia · 2019-02-20T17:17:27Z

@ffledgling the current provider only support x-auth token. But library support basic auth to authenticate api behind proxy.

ffledgling · 2019-02-24T08:10:40Z

@tuapuikia Do you mind opening a new issue with examples of what you want?

When pushing individual tags, specifying the repos is mandatory.

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 30, 2017

ffledgling force-pushed the 363-pdns branch from d55447a to 8a436ab Compare October 30, 2017 16:05

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Oct 30, 2017

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 30, 2017

ffledgling force-pushed the 363-pdns branch from 0f854d8 to e0cdd62 Compare October 30, 2017 17:18

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Oct 30, 2017

ffledgling force-pushed the 363-pdns branch from e0cdd62 to fdedf6d Compare November 6, 2017 17:02

ffledgling changed the title ~~Add fflegling/pdns-go as dep~~ Add PDNS as a Provider Nov 6, 2017

ffledgling force-pushed the 363-pdns branch 4 times, most recently from ec0ecfb to 6f14638 Compare November 7, 2017 10:36

ideahitme suggested changes Nov 8, 2017

View reviewed changes

ffledgling force-pushed the 363-pdns branch from 6f14638 to a4bcb83 Compare January 17, 2018 14:59

k8s-ci-robot removed the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 17, 2018

ffledgling force-pushed the 363-pdns branch from c79df35 to aff13fe Compare January 18, 2018 09:54

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jan 18, 2018

linki added the new-provider label Feb 21, 2018

ffledgling force-pushed the 363-pdns branch from 4c9f3de to 96704e4 Compare February 22, 2018 15:12

ffledgling force-pushed the 363-pdns branch from c1c627d to 831fe32 Compare March 22, 2018 12:04

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 22, 2018

ffledgling force-pushed the 363-pdns branch from 0a2f3a0 to 4e9d177 Compare March 22, 2018 12:53

ffledgling force-pushed the 363-pdns branch from 4e9d177 to c3568e7 Compare March 22, 2018 13:12

Raffo reviewed Apr 4, 2018

View reviewed changes

njuettner reviewed Apr 5, 2018

View reviewed changes

ffledgling force-pushed the 363-pdns branch from 3c3c937 to c3568e7 Compare April 16, 2018 13:36

ffledgling force-pushed the 363-pdns branch from c3568e7 to ec822d7 Compare April 16, 2018 13:43

njuettner merged commit b4a8c14 into kubernetes-sigs:master Apr 16, 2018

ffledgling mentioned this pull request Apr 17, 2018

Add integration for PowerDNS/pdns #363

Closed

lou-lan pushed a commit to lou-lan/external-dns that referenced this pull request May 11, 2022

Specify the repository to push the tag to (kubernetes-sigs#373)

908b7ad

When pushing individual tags, specifying the repos is mandatory.


		sort.SliceStable(zones, func(i, j int) bool { return len(zones[i].Name) > len(zones[j].Name) })

		// NOTE: Complexity of this loop is O(Zones*Endpoints).

Add PDNS as a Provider #373

Add PDNS as a Provider #373

Conversation

ffledgling commented Oct 30, 2017

k8s-ci-robot commented Oct 30, 2017

k8s-ci-robot commented Oct 30, 2017

njuettner commented Nov 1, 2017 • edited Loading

ffledgling commented Nov 1, 2017

ffledgling commented Nov 6, 2017

ideahitme left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ideahitme commented Dec 21, 2017

ffledgling commented Dec 27, 2017 • edited Loading

ideahitme commented Dec 28, 2017

fxpester commented Jan 16, 2018

ffledgling commented Jan 16, 2018

k8s-ci-robot commented Jan 17, 2018

linki commented Feb 12, 2018

ffledgling commented Feb 12, 2018

ffledgling commented Feb 20, 2018 • edited Loading

ffledgling commented Mar 22, 2018 • edited Loading

Raffo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szuecs Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njuettner commented Apr 5, 2018

njuettner commented Apr 13, 2018

njuettner commented Apr 16, 2018

tuapuikia commented Feb 17, 2019

ffledgling commented Feb 20, 2019

tuapuikia commented Feb 20, 2019

ffledgling commented Feb 24, 2019

njuettner commented Nov 1, 2017 •

edited

Loading

ffledgling commented Dec 27, 2017 •

edited

Loading

ffledgling commented Feb 20, 2018 •

edited

Loading

ffledgling commented Mar 22, 2018 •

edited

Loading

szuecs Apr 16, 2018 •

edited

Loading