feat(provider/cloudflare): add SRV and NAPTR record support#6462
feat(provider/cloudflare): add SRV and NAPTR record support#6462bayazee wants to merge 5 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Welcome @bayazee! |
|
Hi @bayazee. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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-sigs/prow repository. |
|
/ok-to-test |
Coverage Report for CI Build 27699082694Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage increased (+0.8%) to 81.408%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions582 previously-covered lines in 19 files lost coverage.
Coverage Stats
💛 - Coveralls |
d384c3d to
304aaf1
Compare
Signed-off-by: Mehdi Bayazee <bayazee@gmail.com>
…s text Signed-off-by: Mehdi Bayazee <bayazee@gmail.com>
…ng dot Signed-off-by: Mehdi Bayazee <bayazee@gmail.com>
Signed-off-by: Mehdi Bayazee <bayazee@gmail.com>
145d61d to
4294262
Compare
| SRV and NAPTR targets should use single-space field separators. Multi-space or | ||
| tab separators cause the record to be re-emitted as a plan change on every | ||
| reconcile. |
There was a problem hiding this comment.
This assertion is true and can be confirmed with a test like this:
func TestSRVNAPTRConverge(t *testing.T) {
cases := []struct {
name string
recordType string
dnsName string
target string
}{
{"SRV canonical", "SRV", "_sip._udp.bar.com", "10 5 5060 sip.bar.com."},
{"SRV multi-space", "SRV", "_sip._udp.bar.com", "10 5 5060 sip.bar.com."},
{"SRV no trailing dot", "SRV", "_sip._udp.bar.com", "10 5 5060 sip.bar.com"},
{"NAPTR canonical", "NAPTR", "bar.com", `10 20 "S" "SIP+D2U" "" _sip._udp.bar.com.`},
{"NAPTR multi-space", "NAPTR", "bar.com", `10 20 "S" "SIP+D2U" "" _sip._udp.bar.com.`},
{"NAPTR no trailing dot", "NAPTR", "bar.com", `10 20 "S" "SIP+D2U" "" _sip._udp.bar.com`},
}
for _, c := range cases {
t.Run(c.name, func(t *testing.T) {
updates := planUpdatesForTarget(t, c.recordType, c.dnsName, c.target)
assert.Zero(t, updates, "record should converge; got %d update(s) on the second reconcile", updates)
})
}
}To me, the root cause is that read-back rebuilds via miekg → single-space canonical; desired target keeps user's spacing . Plan diff never converges → a CF write every reconcile loop.
A possible fix could be in AdjustEndpoints: normalize SRV/NAPTR targets through the same miekg parse so desired == canonical read-back. Then they should actually converge.
Do you think you can fix it ? (and so improve UserXP & remove this part of the doc)
There was a problem hiding this comment.
@mloiseleur You are right and thanks for the test. Fixed in AdjustEndpoints as you suggested. Docs cleaned up. Added some tests.
Verified on a real CF zone. Before the fix and after.
Could you please check it again?
… reconcile churn Signed-off-by: Mehdi Bayazee <bayazee@gmail.com>
What does it do?
Fixes SRV record creation/update in the Cloudflare provider (today fails with HTTP 400 / error 9101) and adds NAPTR support.
Cloudflare's API needs structured
Datafor SRV and NAPTR. Upstream sends the canonical target as flatContent, which CF rejects. This routes both through cloudflare-go's typed batch union members (dns.SRVRecordParam,dns.NAPTRRecordParamfor POST;dns.BatchPutSRVRecordParam,dns.BatchPutNAPTRRecordParamfor PUT). The records carry structured Data and go through the same batch endpoint as every other record type.On read-back, cloudflare-go v5.1.0 leaves
RecordResponse.Datanil on List (see cloudflare/cloudflare-go#4300). The newsrvContent/naptrContenthelpers re-decode the raw JSON to recover the structured data, then rebuild the canonical content. The fallback can be revisited when the SDK is upgraded.Fixes #5551. Refs #4751.
Smoke test
Tested against a real Cloudflare zone, one batch chunk, 6 records (SRV + NAPTR + A + 3 TXT registry records).
Before, on upstream master:
After, this branch:
Re-reconcile (no churn, read-back path holds):
Field changes (SRV port 5060 to 5061, NAPTR preference 20 to 25) and deletion both run through the same batch endpoint with no errors.
digagainst 1.1.1.1 returns the expected records:Tests
Unit tests cover parsers, typed param builders, batch POST/PUT, chunk-retry, read-back, and end-to-end round-trip via the mock client. Package coverage 94.1%.
endpoint.ValidateNAPTRRecordis out of scope here. Happy to send a follow-up if you want it.More
/cc @ivankatliarchuk @mloiseleur
/kind feature
/kind bug
Related, stalled: #4754, #5569.