libdns / cloudflare

Cloudflare provider implementation for libdns
MIT License
63 stars 16 forks source link

Cannot create SRV records #7

Open Adphi opened 2 years ago

Adphi commented 2 years ago

I am trying to create an SRV Record, e.g. _imap._tcp.example.org. 60 IN SRV 10 10 143 example.org..

As there is no real support for it in the libdns package, I tried to set the value to 10 10 143 example.org (both with dot suffix and without) but always get got error status: HTTP 400: [{Code:1004 Message:DNS Validation Error}].

mholt commented 2 years ago

Can you please post your code to minimally reproduce the issue? (Redact credentials) A simple main package would be fine. Thanks!

Adphi commented 2 years ago

I did not use the Priority as the implementation does not use it. Note: the mail.example.org is an existing A Record

package main

import (
    "context"
    "log"
    "time"

    "github.com/libdns/cloudflare"
    "github.com/libdns/libdns"
)

func main() {
    ctx, cancel := context.WithCancel(context.Background())
    defer cancel()

    zone := "example.org"
    ttl := 3600 * time.Second

    c := cloudflare.Provider{APIToken: "REDACTED"}
    recs := []libdns.Record{
        {
            Type:  "SRV",
            Name:  "_imaps._tcp",
            Value: "10 10 993 mail" + "." + zone,
            TTL:   ttl,
        },
        {
            Type:  "SRV",
            Name:  "_imaps._tcp" + "." + zone,
            Value: "10 10 993 mail" + "." + zone,
            TTL:   ttl,
        },
        {
            Type:  "SRV",
            Name:  "_imaps._tcp",
            Value: "10 10 993 mail",
            TTL:   ttl,
        },
        {
            Type:  "SRV",
            Name:  "_imaps._tcp" + "." + zone,
            Value: "10\t10\t993\tmail" + "." + zone,
            TTL:   ttl,
        },
        {
            Type:  "SRV",
            Name:  "_imaps._tcp",
            Value: "10\t10\t993\tmail" + "." + zone,
            TTL:   ttl,
        },
        {
            Type:  "SRV",
            Name:  "_imaps._tcp",
            Value: "10\t10\t993\tmail",
            TTL:   ttl,
        },
    }
    // split requests to check individual records
    for _, v := range recs {
        if _, err := c.AppendRecords(ctx, zone, []libdns.Record{v}); err != nil {
            log.Printf("%s: %s: %v", v.Name, v.Value, err)
        } else {
            log.Printf("%s: Created record: %v", v.Name, v.Value)
        }
    }
}
2022/10/06 19:32:49 _imaps._tcp: 10 10 993 mail.example.org: got error status: HTTP 400: [{Code:1004 Message:DNS Validation Error}]
2022/10/06 19:32:49 _imaps._tcp.example.org: 10 10 993 mail.example.org: got error status: HTTP 400: [{Code:1004 Message:DNS Validation Error}]
2022/10/06 19:32:49 _imaps._tcp: 10 10 993 mail: got error status: HTTP 400: [{Code:1004 Message:DNS Validation Error}]
2022/10/06 19:32:50 _imaps._tcp.example.org: 10     10      993     mail.example.org: got error status: HTTP 400: [{Code:1004 Message:DNS Validation Error}]
2022/10/06 19:32:50 _imaps._tcp: 10     10      993     mail.example.org: got error status: HTTP 400: [{Code:1004 Message:DNS Validation Error}]
2022/10/06 19:32:50 _imaps._tcp: 10     10      993     mail: got error status: HTTP 400: [{Code:1004 Message:DNS Validation Error}]
Adphi commented 2 years ago

While testing with curl I got: service is a required data field:

{"result":null,"success":false,"errors":[{"code":1004,"message":"DNS Validation Error","error_chain":[{"code":9101,"message":"service is a required data field."}]}],"messages":[]}
mholt commented 2 years ago

Thanks for the info, that was helpful. I pushed a commit that enhances error messages (prints the full error_chain, whatever that is) and handles SRV records appropriately. I guess Cloudflare expects them to be specially parsed out.

Adphi commented 2 years ago

Thank you very much for the quick support !

This works:

libdns.Record{
    Type:     "SRV",
    Name:     "_imaps._tcp.@",
    Value:    "993 mail" + "." + zone,
    TTL:      ttl,
    Priority: 10,
    Weight:   10,
}

... but this is not the usual SRV format. I am afraid this implementation breaks the compatibility with other libdns providers. For the given record _imap._tcp.example.org. 60 IN SRV 10 10 143 mail.example.org., most provider would expect a record with name _imap._tcp and value of 10 10 143 mail.example.org. Let the libdns provider chose the implementation details by parsing the canonical format as mention in the 'priority' issue may be a more backward compatible solution.

Adphi commented 2 years ago

If following the current implementation, the record field may need to be:

type Record struct {
    // provider-specific metadata
    ID string

    // general record fields
    Type  string
    Name  string // partially-qualified (relative to zone)
    Value string
    TTL   time.Duration

    // type-dependent record fields
    Priority   uint // HTTPS, SRV, and URI records
    Weight     uint // SRV and URI records
    Port       uint // SRV
    Preference uint // MX
}

or re-use the go types (mx, srv... ) or the miekg dns record implementation used by coredns.

mholt commented 2 years ago

@Adphi Good point. I actually don't like expanding every possible record-specific field into separate fields on the struct. I'd even be interested in removing Priority, I didn't realize there was a canonical way to represent these in a single string. But I guess that makes sense.

Let the libdns provider chose the implementation details by parsing the canonical format as mention in https://github.com/libdns/libdns/issues/38#issuecomment-848819213 may be a more backward compatible solution.

Yes, so let's do that. I'll go look at your PR too.

You'll notice I added some helpers for SRV<-->Record conversion, and I wonder if we should do the same thing for MX and other structured record types. That way, Record can stay general and we have specific types for specific record types.