Azure: add support for NS records #1864
Conversation
k8s-ci-robot
added
cncf-cla: yes
Signed-off-by: Sylvain Rabot <sylvain@abstraction.fr>
|
/kind feature |
k8s-ci-robot
added
the
kind/feature
|
Hello, getting this reviewed / merged would be great, as our logs are currently spammed by such messages : |
|
@sylr Your PR now has a conflict :( @seanmalloy @njuettner @Raffo HI guys! Can we get a review on this? We'd like to contribute these QoL improvements, but we need review and/or comments from you :) |
k8s-ci-robot
added
the
needs-rebase
k8s-ci-robot
removed
the
needs-rebase
|
/auto-cc |
|
getting this logs in external-dns pods |
|
Is there any update on this PR? Any plans to merge it in soon? Thanks! |
|
The errors in the logs mean that we are unable to take the latest version of external-dns. |
|
@bittrance @sfc-gh-jelsesiy Would you be able to help us get this PR reviewed and/or approved? Sorry for the random @ tag, but many of us are getting a bit desperate to get this merged |
There was a problem hiding this comment.
I think this change looks reasonable. I'm new to Kubernetesverse, so I'm not sure if I'm reviewing this at a good level, but I'll give it a shot.
The code is clear and readable and seems to follow local convention. The test coverage of error cases is not great in external-dns in general, but perhaps we could push it up a little with this PR? In particular, the error filtering in newRecordSet could do with a test.
Also, I tried to follow the linked history of PRs and issues, but I can't find a description of the motivation as to why SRV support is needed. What is it you want to do that requires this change? While simply being feature complete may be a sufficient argument, describing the motivation explicitly helps others understand how the software is used. In this case, one might wonder if we should also grow this support in azure_private_dns (in a separate PR, of course)? Maybe a line or two in the PR desc?
None of these things are blockers from my view, so I'll approve and let you decide how much you agree to.
| return b[i].Targets.String() <= b[j].Targets.String() | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
Can you supply a motivation for this change in the PR desc? The SameEndpoints function is used all over the place and it is hard for me to see from this PR that it is ok to change its behaviour.
There was a problem hiding this comment.
I've absolutely no recollection of why I did that ...
|
|
||
| if numErrors > 0 { | ||
| srvRecords = srvRecords[0 : len(srvRecords)-numErrors] | ||
| } |
There was a problem hiding this comment.
Rather than juggling an error count, you could do something like
nsRecords := make([]dns.NsRecord, 0, len(endpoint.Targets))
for _, target := range endpoint.Targets {
priority, weight, port, target, err := extractSrvFields(&target)
if err == nil {
srvRecords = append(srvRecords, dns.SrvRecord{
Priority: priority,
Weight: weight,
Port: port,
Target: target,
})
}
}
return ...According to https://stackoverflow.com/a/52148380/205674 (and similar testing on 1.16) there is a 10% performance penalty for this solution. Personally, I think that is a reasonable price to pay the simplification.
(I skipeed the error logging, which would be an else clause in this case.)
| for i, target := range endpoint.Targets { | ||
| priority, weight, port, target, err := extractSrvFields(&target) | ||
| if err != nil { | ||
| log.Errorf("Failed to add target %v as a SRV record to the list of targets': %v", target, err) |
There was a problem hiding this comment.
Can I suggest to make this a warning? This is not really an error in external-dns.
There was a problem hiding this comment.
why SRV support is needed
Regarding SRV, it comes from this Github issue #1388. The lack of support for SRV records has the side effect of spamming error logs on every External-DNS reconciliation loop.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: bittrance, sylr The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
What's the timeline to having this merged? |
|
@sylr thank you for this PR. Before we can generally merge NS or other type of records, we need to solve the ownership problem. The discussion was started in #1923, but we don't have a solid proposal yet. I would welcome an experiment on the lines of #1923 (comment). |
|
Yeah, anyway, I'm sorry but I switched jobs and I don't use Azure anymore so I can't maintain this PR. If anyone wants it, fell free to reopen a new PR with this branch and take on where I left it. |
|
For anyone primarily concerned with being spammed with error messages like (see a reply above #1864 (comment))
then @bittrance has merged a PR #2020 which should sort this out (many thanks for implementing this fix). |
Description
Superset of #1811 which also adds NS support.
Checklist