Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot create SRV records #7

Open
Adphi opened this issue Oct 6, 2022 · 7 comments
Open

Cannot create SRV records #7

Adphi opened this issue Oct 6, 2022 · 7 comments
Labels
bug Something isn't working

Comments

@Adphi
Copy link

Adphi commented Oct 6, 2022

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

mholt commented Oct 6, 2022

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

@Adphi
Copy link
Author

Adphi commented Oct 6, 2022

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

Adphi commented Oct 6, 2022

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

mholt commented Oct 6, 2022

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.

@mholt mholt closed this as completed Oct 6, 2022
mholt added a commit that referenced this issue Oct 6, 2022
@mholt mholt added the bug Something isn't working label Oct 6, 2022
@Adphi
Copy link
Author

Adphi commented Oct 7, 2022

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

Adphi commented Oct 7, 2022

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

mholt commented Oct 7, 2022

@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 libdns/libdns#38 (comment) 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.

@mholt mholt reopened this Oct 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants