fix(srv): SRV records not created

[AmitBenAmi]

Although SRV records are supported (both in docs and code), they aren't created at all using DNSEndpoint.
This fix makes it happen properly

fixes #1814
fixes #1647

[k8s-ci-robot]

Welcome @AmitBenAmi!

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. 😃

[AmitBenAmi]

/assign @njuettner

[mstarostik]

Yes, please. I can confirm this fixes the immediate problem.
I also added support for MX records locally. I'm a little reluctant to create a PR against a PR though, so if you like: https://gist.github.com/mstarostik/39cf07c79b39d786ed7862cfaf6205bd.

However, applying your PR with or without my patch shows a deeper problem: external-dns will not create multiple RR types for a single label, so:

kind: DNSEndpoint
metadata:
  name: extdns-multi-rr-demo
spec:
  endpoints:
  - dnsName: demo.example.com
    recordType: A
    targets:
    - 192.0.2.1
  - dnsName: demo.example.com
    recordType: MX
    targets:
    - '10 mail.example.com'
  - dnsName: demo.example.com
    recordType: TXT
    targets:
    - '"v=spf1 mx -all"'

Will not work as intended, only one of the RR types is created (happens to be the TXT for me and if I remove that one, I get only the A). So despite endpoints being an array, its elements are not properly dealt with when they share the same dnsName :-(
The same applies when splitting out the entries into different DNSEndpoint resources.
Also, a TXT record will not be merged with the TXT registry's own record. So unless using a prefix or suffix, one cannot add TXTs as the update batch will inclue two distinct records with same label AND same type instead of a single TXT record with multiple values.

So sadly this controller isn't fit to manage all of my DNS needs (yet?)

FWIW, all tested with the Google provider.

[AmitBenAmi]

Yes I see what you mean by that. In AWS Route53 there is a MultiValue property that can be set which I utilizes to use multiple SRV records with the same dnsName, otherwise it will be problematic since they are all with the same name.

Maybe there is something in Google Provider that supports multiple DNS records and types with same names?

[AmitBenAmi]

/assign @Raffo

[mstarostik]

Yes, I'm not familiar with Google's Cloud DNS API directly, but from the error messages I presume that an upsert of a record always needs to contain all values. As I understand the docs, the corresponding rrdatas field is an array anyway, so you just add all values there. An update has to delete all previous records with the dnsName (explicitly listing all values as it seems) and add all wanted values in one go. So you will have to consolidate all targets for a given record type and dnsName (including the heritage value for TXT) into one change operation. And maybe all targets for all record types at the same dnsName into one transaction. A lot of guessing and assumptions here, but probably such a consolidation is required or at least harmless with other providers?

[AmitBenAmi]

Might be harmless, but kind of out of scope for this PR IMO

[mstarostik]

Oh it sure is. Only diverted into this direction because the issue popped up when experimenting with this PR, as it the current limitation very much reduces the usability of this PR, sorry.
Back to topic: regardless of the mentioned problem, the fix presented here seems correct and valuable to me in itself.

[AmitBenAmi]

Oh ok, I didn't get that.
Is this scenario (the mentioned issue) worked before applying my PR? (I guess that if so, tested on Google Provider as mentioned before)

[mstarostik]

Not a regression. The issue I described exists without this PR, it just really surfaces when it's possible to add more record types as introduced by this PR.
It's still useful to allow for different record types even if it comes with limitations.
Sorry again for the confusion.

[AmitBenAmi]

Oh now I get it.
Yes, since you added the ability to create MX through the controller it surfaces even better.
Thanks for clearing that out!

[carpenike]

Hey all!

Hoping to create some SRV DNS endpoints for a FreeIPA service. Can this get merged?

[AmitBenAmi]

Waiting for an approval.
@Raffo @njuettner

[starcraft66]

Any updates on this?

[sheerun]

I don't think I can approve this. I'm owner only of Cloudflare provider. Someone with permission to review whole project is needed here

[seanmalloy]

/kind bug

[caiorcferreira]

Any updates?

Support for SRV records is fundamental for many operations, would be great see this PR merged.

[AmitBenAmi]

Waiting for an approval and merge..

[k8s-ci-robot]

@AmitBenAmi: PR needs rebase.

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.

[k0da]

Yes, please. I can confirm this fixes the immediate problem.
I also added support for MX records locally. I'm a little reluctant to create a PR against a PR though, so if you like: https://gist.github.com/mstarostik/39cf07c79b39d786ed7862cfaf6205bd.

However, applying your PR with or without my patch shows a deeper problem: external-dns will not create multiple RR types for a single label, so:

kind: DNSEndpoint
metadata:
  name: extdns-multi-rr-demo
spec:
  endpoints:
  - dnsName: demo.example.com
    recordType: A
    targets:
    - 192.0.2.1
  - dnsName: demo.example.com
    recordType: MX
    targets:
    - '10 mail.example.com'
  - dnsName: demo.example.com
    recordType: TXT
    targets:
    - '"v=spf1 mx -all"'

Will not work as intended, only one of the RR types is created (happens to be the TXT for me and if I remove that one, I get only the A). So despite endpoints being an array, its elements are not properly dealt with when they share the same dnsName :-(
The same applies when splitting out the entries into different DNSEndpoint resources.
Also, a TXT record will not be merged with the TXT registry's own record. So unless using a prefix or suffix, one cannot add TXTs as the update batch will inclue two distinct records with same label AND same type instead of a single TXT record with multiple values.

I'm trying to solve it here: #2157

So sadly this controller isn't fit to manage all of my DNS needs (yet?)

FWIW, all tested with the Google provider.

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Reopen this issue or PR with /reopen
• Mark this issue or PR as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

[k8s-ci-robot]

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Reopen this issue or PR with /reopen
• Mark this issue or PR as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

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.

[pcornelissen]

Why was this not approved? It would be quite handy...

[michaelkorofiverr]

any chance to re-open and merge this?
@AmitBenAmi @mstarostik @Raffo

[AmitBenAmi]

@michaelkorofiverr I can reopen and try to rebase, but it will be a whole new PR because lots of commits have passed by, so not sure it will fit anymore.
Nevertheless, it will take me a considerable amount of time, so I don't want you to rely on it in the short time frame.