DigitalOcean: support multiple targets per endpoint #1595

tdyas · 2020-05-22T03:27:11Z

This PR teaches the DigialOcean provider how to support multiple targets per endpoint. (The DigitalOcean provider currently only supports one endpoint per target which other providers do support.)

The Problem

I am running nginx-ingress in DaemonSet mode with host networking enabled on my Kubernetes cluster. I want the IPs for every node running nginx-ingress to appear as A records in DNS. First, because the DigitalOcean provider only supports one endpoint per ingress currently, only one of the A records is created which means the additional nginx-ingress pods will not receive traffic.

Moreover, because external-dns believes that there should be multiple A records for the endpoint in question, the reconciliation loop never converges and external-dns continually sends update API calls every minute. This is not how external-dns seems intended to operate. (Indeed, with this PR, the reconciliation loop does converge and avoids sending an update every minute.)

Solution

If the DigitalOcean API were like the AWS or GCP APIs, then the API representation of a DNS record would have an attribute that is a list of RR datas, and supporting multiple targets per endpoint would be as simple as setting that attribute to the list of targets. The DigitalOcean API is not like that though because it represents multiple A records as multiple "domain record" instances.

This representation is actually quite similar to the Linode API. The Linode provider models updates by expanding an endpoint with multiple targets into multiple domain records to be sent to the Linode API. I have taken that approach with this PR: The DigitalOcean provider now expands an endpoint with multiple targets into multiple godo.DomainRecord instances.

Overview of the changes:

The provider's Records method uses the new mergeEndpointsByNameType function to combine multiple godo.DomainRecords for the same DNS name into a single endpoint.Endpoint with multiple targets. This maps the DigitalOcean representation to the external-dns data model
The provider's ApplyChanges method has been rewritten to compute a set of digitalOceanChangeCreate, digitalOceanChangeUpdate, and digitalOceanChangeDelete instances representing how to modify the godo.DomainRecord instances associated with a godo.Domain. Computation of each type of change has been put into the processCreateActions, processUpdateActions, and processDeleteActions functions.
Tests: Lots more testing of individual functions. I added go-cmp to the project so I could use its Equal and Diff functions. These handle structs with nested pointers to other structs much better than the testify package. I did that so I could actually compare the *godo.DomainRecordEditRequest struct contained in the change structs. (Added a TODO to try and see if testify would be willing to allow custom matching functions.)
Upgraded godo to the latest just to keep it current. go bumped the version of two other packages in the go.mod as well, not sure why it did that.

This is currently in production for my cluster. Seems to be working nicely. I did some basic tests against production by deleting and adding some records and external-dns figures out what changed and fixes it. And the reconciliation loop actually converges.

k8s-ci-robot · 2020-05-22T03:27:14Z

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 log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: [email protected]

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 · 2020-05-22T03:27:19Z

Welcome @tdyas!

It looks like this is your first PR to kubernetes-sigs/external-dns 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/external-dns has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

tdyas · 2020-05-22T03:36:38Z

/assign @hjacobs
(the k8s-ci-robot said to assign)

tdyas · 2020-05-31T20:23:39Z

/assign @Raffo

tdyas · 2020-06-04T18:13:48Z

cc @andrewsomething: Just a heads up in case anyone at DigitalOcean has an interest in external-dns or Kubernetes stuff.

Raffo

Thank you for this PR @tdyas especially for the great explanation of what this is doing, I really enjoyed reading it.
I added a few minor comments, but overall the code looks okay.

Raffo · 2020-06-14T08:27:48Z

provider/digitalocean/digital_ocean.go

-		ttl = int(endpoint.RecordTTL)
+// ApplyChanges applies the given set of generic changes to the provider.
+func (p *DigitalOceanProvider) ApplyChanges(ctx context.Context, planChanges *plan.Changes) error {
+	// TODO: This should only retrieve zones affected by the given `planChanges`.


Should we have this done before merging this PR or do you want to keep it like this? What are the consequences?

Not implementing this TODO would leave the status quo in place which is that we would just retrieve all of the records that Records retrieves, just as the existing version of the provider does. (--domain-filter would still be applied to limit the provider to only those zones where external-dns is managing records.)

My concern here was for avoiding having to retrieve all of the records if the zone had potentially many records. #1429 was on my mind as it flagged too many API calls as a potential hazard in such cases.

This TODO is a nice to have but should not block landing this PR.

provider/digitalocean/digital_ocean_test.go

Raffo · 2020-06-14T08:37:21Z

@andrewsomething should you be willing to maintain the DO provider, feel free to ping me here or on slack, we are trying to scale how we maintain providers and we would definitely be happy to have someone from DO maintain the DO provider 😄

Raffo · 2020-06-24T07:44:18Z

/approve
/lgtm

k8s-ci-robot · 2020-06-24T07:44:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Raffo, tdyas

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Raffo]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

map multiple DNS records with the same name/type into the external-dns which models such records as a single Endpoint with multiple entries in the Targets attribute. Solution: Generalize the solution introduced to the DigitalOcean provider by kubernetes-sigs#1595 by refactoring the mergeEndpointsByNameType function into the endpoint module. Then use that function in the RFC2136 provider. Authored-by: Tom Dyas <[email protected]> Signed-off-by: Bogdan Dobrelya <[email protected]>

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label May 22, 2020

k8s-ci-robot requested review from njuettner and Raffo May 22, 2020 03:27

k8s-ci-robot assigned hjacobs May 22, 2020

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 May 22, 2020

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 31, 2020

tdyas force-pushed the digitalocean_support_multiple_targets branch from 7a34cd0 to f19d0bc Compare May 31, 2020 18:53

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 31, 2020

tdyas mentioned this pull request May 31, 2020

DigitalOcean record type change fails #1117

Closed

k8s-ci-robot assigned Raffo May 31, 2020

tdyas force-pushed the digitalocean_support_multiple_targets branch from d0af711 to ba05243 Compare June 9, 2020 14:59

Raffo reviewed Jun 14, 2020

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 17, 2020

tdyas added 5 commits June 18, 2020 23:37

digitalocean: support multiple targets per endpoint

d53d9dc

remove change from Dockerfile

035d484

fix golangcii-lint issue

726d343

remove TODO

e3c0e6d

add test for elementsMatch plus fix for nil

84e514b

tdyas force-pushed the digitalocean_support_multiple_targets branch from 7174db3 to 84e514b Compare June 19, 2020 06:38

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 19, 2020

versions got bumped after rebase

c32e40b

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 24, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 24, 2020

k8s-ci-robot merged commit b616801 into kubernetes-sigs:master Jun 24, 2020

stefanlasiewski mentioned this pull request Jul 16, 2020

rfc2136 continually removing then adding A records that have more than 1 target #1596

Closed

tdyas mentioned this pull request Jul 17, 2020

rfc2136: merge endpoints with same name/type into single endpoint #1678

Closed

bogdando mentioned this pull request Jul 15, 2024

rfc2136: merge Endpoints with same name/type into single Endpoint #4613

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DigitalOcean: support multiple targets per endpoint #1595

DigitalOcean: support multiple targets per endpoint #1595

tdyas commented May 22, 2020 •

edited

Loading

k8s-ci-robot commented May 22, 2020

k8s-ci-robot commented May 22, 2020

tdyas commented May 22, 2020

tdyas commented May 31, 2020

tdyas commented Jun 4, 2020

Raffo left a comment

Raffo Jun 14, 2020

tdyas Jun 15, 2020

Raffo commented Jun 14, 2020

Raffo commented Jun 24, 2020

k8s-ci-robot commented Jun 24, 2020

DigitalOcean: support multiple targets per endpoint #1595

DigitalOcean: support multiple targets per endpoint #1595

Conversation

tdyas commented May 22, 2020 • edited Loading

The Problem

Solution

k8s-ci-robot commented May 22, 2020

k8s-ci-robot commented May 22, 2020

tdyas commented May 22, 2020

tdyas commented May 31, 2020

tdyas commented Jun 4, 2020

Raffo left a comment

Choose a reason for hiding this comment

Raffo Jun 14, 2020

Choose a reason for hiding this comment

tdyas Jun 15, 2020

Choose a reason for hiding this comment

Raffo commented Jun 14, 2020

Raffo commented Jun 24, 2020

k8s-ci-robot commented Jun 24, 2020

tdyas commented May 22, 2020 •

edited

Loading