Add support for NS delegation and MX records #70
Conversation
thriqon
changed the title
thriqon
force-pushed
the
add-support-for-ns-delegation
branch
from
606c6c1 to
209cb89
Compare
frittentheke
force-pushed
the
add-support-for-ns-delegation
branch
3 times, most recently
from
7e6ac0e to
055ebb5
Compare
|
Question: Does the webhook provider in external-DNS even support management of MX records? But if it works, and this is up to the maintainers' standards, please merge this soon. Edit: I asked a question to the community here: kubernetes-sigs/external-dns#6028 |
|
@linwalth , we' re doing it successfully right now :) I think the limitation in the docs is not actually true. |
And I shall get to this PR and the other issues by the end of this week so the webhook will also do it all ;-) |
|
@thriqon I concur. Elsewise webhook providers for mikrotik or hetzner would not be able to create MX records either. Probably just lack of documentation on external-DNS side. @frittentheke Thank you very much! |
The method contained a subtle bug, namely not accepting domains having a suffix already into the target list. This probably never failed for anyone as external-dns does not allow targets with a period suffix anyway. canonicalizeDomainNames should also just use canonicalizeDomainName for consistency.
In external-dns, targets MUST NOT end with a period suffix (.). Designate however requires this.
frittentheke
force-pushed
the
add-support-for-ns-delegation
branch
from
055ebb5 to
1c19355
Compare
|
Sorry this all took me a while. I had to (re-)create a proper test setup with K8s + DevStack since external-dns does NOT support all records types via the fake source -- see kubernetes-sigs/external-dns#6042. And to make MX records, which usually are APEX records usable, there is more work needed on the external-dns side needed, I believe:
as I ran into these issues when testing with examples similar to e.g. https://kubernetes-sigs.github.io/external-dns/latest/docs/sources/mx-record/ and https://kubernetes-sigs.github.io/external-dns/latest/docs/sources/ns-record/ Do you mind sharing your custom resources (redact the domain if you want), so I can reproduce in which cases your PR should work? |
There was a problem hiding this comment.
@thriqon thanks a bunch for your time and contribution.
In case you did not notice, I merged a few unrelated changes and updates and also released v2.1.0 of the webhook and rebased your branch.
|
|
||
| func (p designateProvider) supportedRecordType(recordType string) bool { | ||
| switch recordType { | ||
| case endpoint.RecordTypeA, endpoint.RecordTypeTXT, endpoint.RecordTypeCNAME, endpoint.RecordTypeNS, endpoint.RecordTypeMX: |
There was a problem hiding this comment.
Unfortunately this will need further adjustment as I noticed we don't even support all record types supported by external-dns (https://github.com/kubernetes-sigs/external-dns/blob/e22cd737cd666db7394df506cd3f1a596a57a184/endpoint/endpoint.go#L32-L65 ?).
Not your fault and not part of the change, but since I just looked a little closer at the code, this really needs fixing.
I recently also fiddled with the proper format of SRV records emitted by external-dns itself, see
kubernetes-sigs/external-dns@fde978f, but there is not even SRV support in this webhook provider yet.
After all, adding support for more records types can also happen after your MR is merged.
3 participants
Code was already almost ready for NS and MX records, only canonicalization was missing.
The code for canonicalization was slightly buggy, refactored and fixed.
Tested with a real Designate setup.