Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions docs/sources/srv-record.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
# 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 (at least) by `akamai`, `civo`, `cloudflare`, `ibmcloud`, `linode`, `rfc2136` and `pdns` providers.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we sure all this providers support SRV?

```

Targets within the CRD need to be specified according to the RFC 2782. Below is an example of
`example.com` DNS SRV record. It specifies a `sip` service of `udp` protocol. It has two targets
of identical priority but with different weights. They point to different backend servers.

```yaml
apiVersion: externaldns.k8s.io/v1alpha1
kind: DNSEndpoint
metadata:
name: examplesrvrecord
spec:
endpoints:
- dnsName: _sip._udp.example.com
recordTTL: 180
recordType: SRV
targets:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

- 10 10 5060 sip1.example.com
- 10 20 5060 sip2.example.com
```
88 changes: 79 additions & 9 deletions provider/cloudflare/cloudflare.go
Original file line number Diff line number Diff line change
Expand Up @@ -172,13 +172,20 @@ type RecordParamsTypes interface {

// updateDNSRecordParam is a function that returns the appropriate Record Param based on the cloudFlareChange passed in
func updateDNSRecordParam(cfc cloudFlareChange) cloudflare.UpdateDNSRecordParams {
return cloudflare.UpdateDNSRecordParams{
recordParam := cloudflare.UpdateDNSRecordParams{
Name: cfc.ResourceRecord.Name,
TTL: cfc.ResourceRecord.TTL,
Proxied: cfc.ResourceRecord.Proxied,
Type: cfc.ResourceRecord.Type,
Content: cfc.ResourceRecord.Content,
}

if cfc.ResourceRecord.Type == "SRV" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New lines added, not covered with unit tests
Screenshot 2025-02-27 at 07 18 54

recordParam.Data = cfc.ResourceRecord.Data
} else {
recordParam.Content = cfc.ResourceRecord.Content
}

return recordParam
}

// updateDataLocalizationRegionalHostnameParams is a function that returns the appropriate RegionalHostname Param based on the cloudFlareChange passed in
Expand All @@ -191,13 +198,52 @@ func updateDataLocalizationRegionalHostnameParams(cfc cloudFlareChange) cloudfla

// getCreateDNSRecordParam is a function that returns the appropriate Record Param based on the cloudFlareChange passed in
func getCreateDNSRecordParam(cfc cloudFlareChange) cloudflare.CreateDNSRecordParams {
return cloudflare.CreateDNSRecordParams{
recordParam := cloudflare.CreateDNSRecordParams{
Name: cfc.ResourceRecord.Name,
TTL: cfc.ResourceRecord.TTL,
Proxied: cfc.ResourceRecord.Proxied,
Type: cfc.ResourceRecord.Type,
Content: cfc.ResourceRecord.Content,
}

if cfc.ResourceRecord.Type == "SRV" {
recordParam.Data = cfc.ResourceRecord.Data
} else {
recordParam.Content = cfc.ResourceRecord.Content
}

return recordParam
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Screenshot 2025-02-27 at 07 21 33

parts := strings.Fields(content)
if len(parts) != 4 {
return nil, fmt.Errorf("invalid SRV record content: %s", content)
}

priority, err := strconv.Atoi(parts[0])
if err != nil {
return nil, fmt.Errorf("invalid priority in SRV record content: %s", parts[0])
}

weight, err := strconv.Atoi(parts[1])
if err != nil {
return nil, fmt.Errorf("invalid weight in SRV record content: %s", parts[1])
}

port, err := strconv.Atoi(parts[2])
if err != nil {
return nil, fmt.Errorf("invalid port in SRV record content: %s", parts[2])
}

target := parts[3]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wdyt of checking IP format with ParseIP ?

Copy link
Author

@starcraft66 starcraft66 Sep 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


return map[string]interface{}{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we have a struct instead?

"priority": priority,
"weight": weight,
"port": port,
"target": target,
}, nil
}

// NewCloudFlareProvider initializes a new CloudFlare DNS based Provider.
Expand Down Expand Up @@ -432,7 +478,7 @@ func (p *CloudFlareProvider) submitChanges(ctx context.Context, changes []*cloud
}
recordParam := updateDNSRecordParam(*change)
recordParam.ID = recordID
err := p.Client.UpdateDNSRecord(ctx, resourceContainer, recordParam)
err = p.Client.UpdateDNSRecord(ctx, resourceContainer, recordParam)
if err != nil {
failedChange = true
log.WithFields(logFields).Errorf("failed to update record: %v", err)
Expand Down Expand Up @@ -466,7 +512,7 @@ func (p *CloudFlareProvider) submitChanges(ctx context.Context, changes []*cloud
}
} else if change.Action == cloudFlareCreate {
recordParam := getCreateDNSRecordParam(*change)
_, err := p.Client.CreateDNSRecord(ctx, resourceContainer, recordParam)
_, err = p.Client.CreateDNSRecord(ctx, resourceContainer, recordParam)
if err != nil {
failedChange = true
log.WithFields(logFields).Errorf("failed to create record: %v", err)
Expand Down Expand Up @@ -549,12 +595,11 @@ func (p *CloudFlareProvider) getCustomHostnameIDbyOrigin(chs []cloudflare.Custom
func (p *CloudFlareProvider) newCloudFlareChange(action string, endpoint *endpoint.Endpoint, target string) *cloudFlareChange {
ttl := defaultCloudFlareRecordTTL
proxied := shouldBeProxied(endpoint, p.proxiedByDefault)

if endpoint.RecordTTL.IsConfigured() {
ttl = int(endpoint.RecordTTL)
}
dt := time.Now()
return &cloudFlareChange{
change := &cloudFlareChange{
Action: action,
ResourceRecord: cloudflare.DNSRecord{
Name: endpoint.DNSName,
Expand Down Expand Up @@ -587,6 +632,19 @@ func (p *CloudFlareProvider) newCloudFlareChange(action string, endpoint *endpoi
},
},
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

error case not covered
Screenshot 2025-02-27 at 07 24 57

return nil
}
change.ResourceRecord.Data = srvData
} else {
change.ResourceRecord.Content = target
}

return change
}

// listDNSRecordsWithAutoPagination performs automatic pagination of results on requests to cloudflare.ListDNSRecords with custom per_page values
Expand Down Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not covered with tests
Screenshot 2025-02-27 at 07 25 23

priority, _ := dataMap["priority"].(float64)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

weight, _ := dataMap["weight"].(float64)
port, _ := dataMap["port"].(float64)
target, _ := dataMap["target"].(string)
targets[i] = fmt.Sprintf("%d %d %d %s", int(priority), int(weight), int(port), target)
} else {
log.Errorf("SRV record data is not in the expected format for record %s", record.Name)
}
} else {
targets[i] = record.Content
}
}
ep := endpoint.NewEndpointWithTTL(
records[0].Name,
Expand Down
115 changes: 98 additions & 17 deletions provider/cloudflare/cloudflare_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -111,21 +111,31 @@ func NewMockCloudFlareClientWithRecords(records map[string][]cloudflare.DNSRecor
func getDNSRecordFromRecordParams(rp any) cloudflare.DNSRecord {
switch params := rp.(type) {
case cloudflare.CreateDNSRecordParams:
return cloudflare.DNSRecord{
record := cloudflare.DNSRecord{
Name: params.Name,
TTL: params.TTL,
Proxied: params.Proxied,
Type: params.Type,
Content: params.Content,
}
if params.Type == "SRV" {
record.Data = params.Data
} else {
record.Content = params.Content
}
return record
case cloudflare.UpdateDNSRecordParams:
return cloudflare.DNSRecord{
record := cloudflare.DNSRecord{
Name: params.Name,
TTL: params.TTL,
Proxied: params.Proxied,
Type: params.Type,
Content: params.Content,
}
if params.Type == "SRV" {
record.Data = params.Data
} else {
record.Content = params.Content
}
return record
default:
return cloudflare.DNSRecord{}
}
Expand Down Expand Up @@ -373,9 +383,9 @@ func AssertActions(t *testing.T, provider *CloudFlareProvider, endpoints []*endp

changes := plan.Calculate().Changes

// Records other than A, CNAME and NS are not supported by planner, just create them
// Records other than A, CNAME, SRV and NS are not supported by planner, just create them
for _, endpoint := range endpoints {
if endpoint.RecordType != "A" && endpoint.RecordType != "CNAME" && endpoint.RecordType != "NS" {
if endpoint.RecordType != "A" && endpoint.RecordType != "CNAME" && endpoint.RecordType != "NS" && endpoint.RecordType != "SRV" {
changes.Create = append(changes.Create, endpoint)
}
}
Expand Down Expand Up @@ -489,6 +499,60 @@ func TestCloudflareCustomTTL(t *testing.T) {
)
}

func TestCloudflareSRVRecord(t *testing.T) {
endpoints := []*endpoint.Endpoint{
{
RecordType: "SRV",
DNSName: "_sip._tcp.srv.bar.com",
Targets: endpoint.Targets{"10 20 5060 sip.bar.com", "10 50 5060 sip2.bar.com"},
RecordTTL: 120,
},
}

expectedActions := []MockAction{
{
Name: "Create",
ZoneId: "001",
RecordData: cloudflare.DNSRecord{
Type: "SRV",
Name: "_sip._tcp.srv.bar.com",
Data: map[string]interface{}{
"priority": 10,
"weight": 20,
"port": 5060,
"target": "sip.bar.com",
},
TTL: 120,
Proxied: proxyDisabled,
},
},
{
Name: "Create",
ZoneId: "001",
RecordData: cloudflare.DNSRecord{
Type: "SRV",
Name: "_sip._tcp.srv.bar.com",
Data: map[string]interface{}{
"priority": 10,
"weight": 50,
"port": 5060,
"target": "sip2.bar.com",
},
TTL: 120,
Proxied: proxyDisabled,
},
},
}

AssertActions(
t,
&CloudFlareProvider{},
endpoints,
expectedActions,
[]string{endpoint.RecordTypeSRV},
)
}

func TestCloudflareProxiedDefault(t *testing.T) {
endpoints := []*endpoint.Endpoint{
{
Expand Down Expand Up @@ -635,7 +699,12 @@ func TestCloudflareSetProxied(t *testing.T) {
{
RecordType: testCase.recordType,
DNSName: testCase.domain,
Targets: endpoint.Targets{"127.0.0.1"},
Targets: func() endpoint.Targets {
if testCase.recordType == "SRV" {
return endpoint.Targets{"0 0 8080 127.0.0.1"} // Example SRV format: priority weight port target
}
return endpoint.Targets{"127.0.0.1"}
}(),
ProviderSpecific: endpoint.ProviderSpecific{
endpoint.ProviderSpecificProperty{
Name: "external-dns.alpha.kubernetes.io/cloudflare-proxied",
Expand All @@ -645,19 +714,31 @@ func TestCloudflareSetProxied(t *testing.T) {
},
}

expectedRecord := cloudflare.DNSRecord{
Type: testCase.recordType,
Name: testCase.domain,
TTL: 1,
Proxied: testCase.proxiable,
}

if testCase.recordType == "SRV" {
expectedRecord.Data = map[string]interface{}{
"priority": 0,
"weight": 0,
"port": 8080,
"target": "127.0.0.1",
}
} else {
expectedRecord.Content = "127.0.0.1"
}

AssertActions(t, &CloudFlareProvider{}, endpoints, []MockAction{
{
Name: "Create",
ZoneId: "001",
RecordData: cloudflare.DNSRecord{
Type: testCase.recordType,
Name: testCase.domain,
Content: "127.0.0.1",
TTL: 1,
Proxied: testCase.proxiable,
},
Name: "Create",
ZoneId: "001",
RecordData: expectedRecord,
},
}, []string{endpoint.RecordTypeA, endpoint.RecordTypeCNAME, endpoint.RecordTypeNS}, testCase.recordType+" record on "+testCase.domain)
}, []string{endpoint.RecordTypeA, endpoint.RecordTypeCNAME, endpoint.RecordTypeNS, endpoint.RecordTypeSRV}, testCase.recordType+" record on "+testCase.domain)
}
}

Expand Down
Loading