Support SRV record creation with cloudflare provider #4754

starcraft66 · 2024-09-17T21:30:54Z

Description

Implements SRV record creation when using the cloudflare provider. Cloudflare's API requires sending SRV record data in structured fields rather than a bare string with the record.

I'm not a Go developer and just did the bare minimum to get this working and tested, I've confirmed this works on my personal cluster. Feel free to suggest improvements.

Fixes #4751

Checklist

Unit tests updated
End user documentation updated

linux-foundation-easycla · 2024-09-17T21:31:00Z

The committers listed above are authorized under a signed CLA.

✅ login: starcraft66 / name: Tristan (d224576, f6941a2, 0bdebaf, 9fbca46, 7372d50)

k8s-ci-robot · 2024-09-17T21:31:03Z

Welcome @starcraft66!

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

k8s-ci-robot · 2024-09-17T21:31:03Z

Hi @starcraft66. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

mloiseleur · 2024-09-18T06:27:53Z

provider/cloudflare/cloudflare.go

+		return nil, fmt.Errorf("invalid port in SRV record content: %s", parts[2])
+	}
+
+	target := parts[3]


Wdyt of checking IP format with ParseIP ?

It wouldn't really make sense here afaik because the the target could be a hostname that would have to be resolved, so pretty much anything goes.

mloiseleur

Thanks for this PR. Overall it looks good, and it's tested 👍.
Wdyt of updating documentation in order to show how to create a SRV record ?

mloiseleur · 2024-09-18T06:29:36Z

/ok-to-test

starcraft66 · 2024-09-22T16:49:25Z

Just added documentation

starcraft66 · 2024-09-22T16:50:47Z

docs/sources/srv-record.md

+# SRV record with CRD source
+
+You can create and manage SRV records with the help of [CRD source](../contributing/crd-source.md)
+and `DNSEndpoint` CRD. The implementation of this feature depends on provider API support, this feature is currently known to be supported supported by `akamai`, `civo`, `cloudflare`, `ibmcloud`, `linode`, `rfc2136` and `pdns` providers.


This might not be completely accurate but I can't test every provider, I just built this list by grepping for mentions of SRV in the providers folder and searching the issue tracker.

docs/sources/srv-record.md

provider/cloudflare/cloudflare.go

mloiseleur · 2024-10-19T07:54:18Z

@starcraft66 Thanks for the documentation. See my comments for a first review of the code.

kashalls · 2024-10-23T03:14:11Z

@starcraft66 Do you need any help with this PR, glad to help if possible.

starcraft66 · 2024-10-23T15:28:35Z

I was going to take a look at this as soon as I have some time but haven't gotten to it yet.

Feel free to link some commits and I can push them to my branch, I love open source collaboration ☺️

mloiseleur · 2025-02-20T11:30:51Z

It seems we can proceed with final review and merge for this one.

@starcraft66 Do you think you can address the last comments and rebase this PR ?

starcraft66 · 2025-02-26T18:10:17Z

Hey sorry, I was on vacation. Will rebase this asap.

Signed-off-by: Tristan Gosselin-Hane <[email protected]>

k8s-ci-robot · 2025-02-27T01:23:27Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign mloiseleur for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

starcraft66 · 2025-02-27T01:33:45Z

@mloiseleur ready for review

ivankatliarchuk · 2025-02-27T07:13:30Z

Hi. Nice on, I'll do an initial review.

/label tide/merge-method-squash

ivankatliarchuk · 2025-02-27T07:15:43Z

docs/sources/srv-record.md

+In order to start managing MX records you need to set the `--managed-record-types SRV` flag.
+
+```console
+external-dns --source crd --provider {akamai|civo|cloudflare|ibmcloud|linode|rfc2136|pdns} --managed-record-types A --managed-record-types CNAME --managed-record-types SRV


are we sure all this providers support SRV?

ivankatliarchuk · 2025-02-27T07:17:43Z

docs/sources/srv-record.md

+    - dnsName: _sip._udp.example.com
+      recordTTL: 180
+      recordType: SRV
+      targets:


Would it be possible to share smoke tests as well, could you please also provide a way to test this manually with manifests and kubectl commands? Similar to this example #5111 (comment)

for cloudflare is enough, I'll do for aws or google?

I'm unsure where or not multiple targets supported, never tried that case.

ivankatliarchuk · 2025-02-27T07:20:33Z

provider/cloudflare/cloudflare.go

 	}
+
+	if cfc.ResourceRecord.Type == "SRV" {


this seems identical to cloudflare.CreateDNSRecordParams. Shell we create a shared method?

New lines added, not covered with unit tests

ivankatliarchuk · 2025-02-27T07:22:46Z

provider/cloudflare/cloudflare.go

+}
+
+// parseSRVContent parses the SRV record content string into the structured data required by the Cloudflare API
+func parseSRVContent(content string) (map[string]interface{}, error) {


Error cases not covered, could we add unit-test coverege for them? We are aiming to increase test-coverage

ivankatliarchuk · 2025-02-27T07:24:16Z

provider/cloudflare/cloudflare.go

+
+	target := parts[3]
+
+	return map[string]interface{}{


Could we have a struct instead?

ivankatliarchuk · 2025-02-27T07:25:09Z

provider/cloudflare/cloudflare.go

+	if endpoint.RecordType == "SRV" {
+		srvData, err := parseSRVContent(target)
+		if err != nil {
+			log.Errorf("Error parsing SRV content: %v", err)


error case not covered

ivankatliarchuk · 2025-02-27T07:25:51Z

provider/cloudflare/cloudflare.go

@@ -707,7 +765,19 @@ func groupByNameAndTypeWithCustomHostnames(records []cloudflare.DNSRecord, chs [
 		}
 		targets := make([]string, len(records))
 		for i, record := range records {
-			targets[i] = record.Content
+			if record.Type == "SRV" {
+				if dataMap, ok := record.Data.(map[string]interface{}); ok {


Not covered with tests

ivankatliarchuk · 2025-02-27T07:26:23Z

provider/cloudflare/cloudflare.go

-			targets[i] = record.Content
+			if record.Type == "SRV" {
+				if dataMap, ok := record.Data.(map[string]interface{}); ok {
+					priority, _ := dataMap["priority"].(float64)


same apply, could we have a concrete struct, so we do not need to cast?

k8s-ci-robot requested review from mloiseleur and szuecs September 17, 2024 21:30

starcraft66 force-pushed the feature/tgh/cloudflare-srv-records branch 4 times, most recently from 78a12d5 to 5f828ae Compare September 17, 2024 21:43

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 17, 2024

mloiseleur reviewed Sep 18, 2024

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 18, 2024

starcraft66 requested a review from mloiseleur September 22, 2024 16:49

starcraft66 commented Sep 22, 2024

View reviewed changes

mloiseleur reviewed Sep 30, 2024

View reviewed changes

docs/sources/srv-record.md Outdated Show resolved Hide resolved

starcraft66 requested a review from mloiseleur October 1, 2024 21:26

mloiseleur reviewed Oct 19, 2024

View reviewed changes

provider/cloudflare/cloudflare.go Outdated Show resolved Hide resolved

mloiseleur reviewed Oct 19, 2024

View reviewed changes

provider/cloudflare/cloudflare.go Outdated Show resolved Hide resolved

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

starcraft66 added 4 commits February 26, 2025 20:13

Support SRV record creation with cloudflare provider

9fbca46

Signed-off-by: Tristan Gosselin-Hane <[email protected]>

Add SRV record docs

f6941a2

Fix spelling mistake

0bdebaf

Apply code review suggestions

7372d50

starcraft66 force-pushed the feature/tgh/cloudflare-srv-records branch from 8dd6fe6 to 7372d50 Compare February 27, 2025 01:23

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

go fmt

d224576

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Feb 27, 2025

ivankatliarchuk suggested changes Feb 27, 2025

View reviewed changes

k8s-ci-robot assigned ivankatliarchuk Feb 27, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support SRV record creation with cloudflare provider #4754

Support SRV record creation with cloudflare provider #4754

starcraft66 commented Sep 17, 2024 •

edited

Loading

linux-foundation-easycla bot commented Sep 17, 2024 •

edited

Loading

k8s-ci-robot commented Sep 17, 2024

k8s-ci-robot commented Sep 17, 2024

mloiseleur Sep 18, 2024

starcraft66 Sep 22, 2024 •

edited

Loading

mloiseleur left a comment

mloiseleur commented Sep 18, 2024

starcraft66 commented Sep 22, 2024

starcraft66 Sep 22, 2024

mloiseleur commented Oct 19, 2024

kashalls commented Oct 23, 2024

starcraft66 commented Oct 23, 2024

mloiseleur commented Feb 20, 2025 •

edited

Loading

starcraft66 commented Feb 26, 2025

k8s-ci-robot commented Feb 27, 2025

starcraft66 commented Feb 27, 2025

ivankatliarchuk commented Feb 27, 2025

ivankatliarchuk Feb 27, 2025

ivankatliarchuk Feb 27, 2025

ivankatliarchuk Feb 27, 2025

ivankatliarchuk Feb 27, 2025

ivankatliarchuk Feb 27, 2025

ivankatliarchuk Feb 27, 2025

ivankatliarchuk Feb 27, 2025

ivankatliarchuk Feb 27, 2025

ivankatliarchuk Feb 27, 2025

Support SRV record creation with cloudflare provider #4754

Are you sure you want to change the base?

Support SRV record creation with cloudflare provider #4754

Conversation

starcraft66 commented Sep 17, 2024 • edited Loading

linux-foundation-easycla bot commented Sep 17, 2024 • edited Loading

k8s-ci-robot commented Sep 17, 2024

k8s-ci-robot commented Sep 17, 2024

Choose a reason for hiding this comment

starcraft66 Sep 22, 2024 • edited Loading

Choose a reason for hiding this comment

mloiseleur left a comment

Choose a reason for hiding this comment

mloiseleur commented Sep 18, 2024

starcraft66 commented Sep 22, 2024

Choose a reason for hiding this comment

mloiseleur commented Oct 19, 2024

kashalls commented Oct 23, 2024

starcraft66 commented Oct 23, 2024

mloiseleur commented Feb 20, 2025 • edited Loading

starcraft66 commented Feb 26, 2025

k8s-ci-robot commented Feb 27, 2025

starcraft66 commented Feb 27, 2025

ivankatliarchuk commented Feb 27, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

starcraft66 commented Sep 17, 2024 •

edited

Loading

linux-foundation-easycla bot commented Sep 17, 2024 •

edited

Loading

starcraft66 Sep 22, 2024 •

edited

Loading

mloiseleur commented Feb 20, 2025 •

edited

Loading