From 2cb245d6e33641ec68461b81eebc1b5d64d4bf5c Mon Sep 17 00:00:00 2001 From: Jakub Piecuch Date: Wed, 16 Apr 2025 20:28:39 +0200 Subject: [PATCH] SetRecords: preserve RRSets not set in the input MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit According to the libdns specification for SetRecords [1]: > SetRecords updates the zone so that the records described in the input > are reflected in the output. It may create or update records > or—depending on the record type—delete records to maintain parity with > the input. No other records are affected. It returns the records which > were set. > > For any (name, type) pair in the input, SetRecords ensures that the > only records in the output zone with that (name, type) pair are those > that were provided in the input. From this description, especially the "No other records are affected" part, I believe that RRSets which are not mentioned in the input should not be modified or deleted by SetRecords. This is also made clear by one of the examples. Note that the TXT record is preserved: > Example 1: > > ;; Original zone > example.com. 3600 IN A 192.0.2.1 > example.com. 3600 IN A 192.0.2.2 > example.com. 3600 IN TXT "hello world" > > ;; Input > example.com. 3600 IN A 192.0.2.3 > > ;; Resultant zone > example.com. 3600 IN A 192.0.2.3 > example.com. 3600 IN TXT "hello world" The current implementation of SetRecords deletes all RRSets from the zone which are not mentioned in the input. This commit changes the behavior to match the spec. [1] https://github.com/libdns/libdns/blob/5ab1d4de259f1eb914085c61784ad9176ea8e803/libdns.go#L108 --- provider.go | 49 ++++------------- provider_test.go | 139 +++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 144 insertions(+), 44 deletions(-) diff --git a/provider.go b/provider.go index 455303c..973daa9 100644 --- a/provider.go +++ b/provider.go @@ -199,9 +199,6 @@ func (p *Provider) AppendRecords(ctx context.Context, zone string, records []lib // SetRecords sets the records in the zone, either by updating existing records or creating new ones. // It returns the updated records. -// -// Caveat: This method will fail if there are more than 500 RRsets in the zone. See package -// documentation for more detail. func (p *Provider) SetRecords(ctx context.Context, zone string, records []libdns.Record) ([]libdns.Record, error) { err := acquireWriteToken(ctx) if err != nil { @@ -209,12 +206,12 @@ func (p *Provider) SetRecords(ctx context.Context, zone string, records []libdns } defer releaseWriteToken() - // Build the desired state - rrsets := make(map[rrKey]*rrSet) + // Group records by rrKey (name, type) + rrsetMap := make(map[rrKey]*rrSet) for _, r := range records { rr := r.RR() key := rrKey{rrSetSubname(rr), rr.Type} - rrset := rrsets[key] + rrset := rrsetMap[key] if rrset == nil { rrset = &rrSet{ Subname: key.Subname, @@ -222,41 +219,17 @@ func (p *Provider) SetRecords(ctx context.Context, zone string, records []libdns Records: nil, TTL: rrSetTTL(rr), } - rrsets[key] = rrset + rrsetMap[key] = rrset } rrset.Records = append(rrset.Records, rrSetRecord(rr)) } - // Fetch existing rrSets and compare to desired state - existing, err := p.listRRSets(ctx, zone) - if err != nil { - return nil, fmt.Errorf("listing RRSets: %v", err) - } - for _, g := range existing { - key := rrKey{g.Subname, g.Type} - w := rrsets[key] - switch { - case w == nil: - // rrset exists, but not in the input, delete it by adding it to rrsets and set - // records to an empty slice to represent the deletion. - // See https://desec.readthedocs.io/en/latest/dns/rrsets.html#deleting-an-rrset - w0 := g - w0.Records = []string{} - rrsets[key] = &w0 - case g.equal(w): - // rrset exists and is equal to the one we want; skip it in the update. - delete(rrsets, key) - } - } - - // Generate updates to arrive at desired state. - update := make([]rrSet, 0, len(rrsets)) - var ret []libdns.Record - for _, rrset := range rrsets { - update = append(update, *rrset) - - // Add all records being set here. This ignores records that are being deleted, because - // those are represented as an rrset without any records. + // Build list of RRSets to pass to the API and list of libdns records + // to return from the function + rrsetList := make([]rrSet, 0, len(rrsetMap)) + ret := make([]libdns.Record, 0, len(records)) + for _, rrset := range rrsetMap { + rrsetList = append(rrsetList, *rrset) records0, err := libdnsRecords(*rrset) if err != nil { return nil, fmt.Errorf("parsing RRSet: %v", err) @@ -264,7 +237,7 @@ func (p *Provider) SetRecords(ctx context.Context, zone string, records []libdns ret = append(ret, records0...) } - if err := p.putRRSets(ctx, zone, update); err != nil { + if err := p.putRRSets(ctx, zone, rrsetList); err != nil { return nil, fmt.Errorf("writing RRSets: %v", err) } return ret, nil diff --git a/provider_test.go b/provider_test.go index b98bedc..822afda 100644 --- a/provider_test.go +++ b/provider_test.go @@ -293,10 +293,12 @@ func TestSetRecords(t *testing.T) { ctx := setup(t, `[ {"subname": "www", "type": "A", "ttl": 3600, "records": ["127.0.1.1", "127.0.1.2"]}, {"subname": "", "type": "TXT", "ttl": 3600, "records": ["\"will be overridden\""]}, + {"subname": "sub", "type": "TXT", "ttl": 3600, "records": ["\"will stay the same\""]}, {"subname": "www", "type": "HTTPS", "ttl": 3600, "records": ["1 . alpn=\"h2\""]}, {"subname": "_sip._tcp.sub", "type": "SRV", "ttl": 3600, "records": ["1 100 5061 sip.example.com."]}, {"subname": "_ftp._tcp", "type": "URI", "ttl": 3600, "records": ["1 2 \"ftp://example.com/arst\""]}, - {"subname": "", "type": "MX", "ttl": 3600, "records": ["0 mx0.example.com.", "10 mx1.example.com."]} + {"subname": "", "type": "MX", "ttl": 3600, "records": ["0 mx0.example.com.", "10 mx1.example.com."]}, + {"subname": "www", "type": "NS", "ttl": 3600, "records": ["ns0.example.com.", "ns1.example.com."]} ]`) p := &desec.Provider{ @@ -369,13 +371,14 @@ func TestSetRecords(t *testing.T) { }, } - created, err := p.SetRecords(ctx, *domain+".", records) + recordsSet, err := p.SetRecords(ctx, *domain+".", records) if err != nil { t.Fatal(err) } - // The records that already existed are not returned by SetRecords. - wantCreated := []libdns.Record{ + // All set records, including the ones that already existed, should be returned + // by SetRecords. + wantSet := []libdns.Record{ libdns.Address{ Name: "@", IP: netip.MustParseAddr("127.0.0.3"), @@ -391,6 +394,16 @@ func TestSetRecords(t *testing.T) { IP: netip.MustParseAddr("127.0.0.1"), TTL: 3600 * time.Second, }, + libdns.ServiceBinding{ + Scheme: "https", + Name: "www", + Target: ".", + Params: libdns.SvcParams{ + "alpn": []string{"h2"}, + }, + Priority: 1, + TTL: 3600 * time.Second, + }, libdns.Address{ Name: "www", IP: netip.MustParseAddr("127.0.0.2"), @@ -401,16 +414,130 @@ func TestSetRecords(t *testing.T) { IP: netip.MustParseAddr("127.0.0.5"), TTL: 3600 * time.Second, }, + libdns.MX{ + Name: "@", + Target: "mx0.example.com.", + TTL: 3600 * time.Second, + Preference: 0, + }, + libdns.MX{ + Name: "@", + Target: "mx1.example.com.", + TTL: 3600 * time.Second, + Preference: 10, + }, + libdns.SRV{ + Service: "sip", + Transport: "tcp", + Name: "sub", + Target: "sip.example.com.", + TTL: 3600 * time.Second, + Priority: 1, + Weight: 100, + Port: 5061, + }, + libdns.RR{ + Type: "URI", + Name: "_ftp._tcp", + Data: `1 2 "ftp://example.com/arst"`, + TTL: 3600 * time.Second, + }, } - if diff := cmp.Diff(wantCreated, created, cmpRecord); diff != "" { + + if diff := cmp.Diff(wantSet, recordsSet, cmpRecord); diff != "" { t.Fatalf("p.SetRecords() unexpected diff [-want +got]: %s", diff) } + wantCurrent := []libdns.Record{ + // Records for (name, type) pairs which were not present in the SetRecords input + // should be unaffected. + libdns.TXT{ + Name: "sub", + Text: `will stay the same`, + TTL: time.Second * 3600, + }, + libdns.NS{ + Name: "www", + Target: "ns0.example.com.", + TTL: time.Second * 3600, + }, + libdns.NS{ + Name: "www", + Target: "ns1.example.com.", + TTL: time.Second * 3600, + }, + // Records for (name, type) pairs which were present in the SetRecords input + // should match the output of SetRecords. + libdns.Address{ + Name: "@", + IP: netip.MustParseAddr("127.0.0.3"), + TTL: time.Second * 3601, + }, + libdns.TXT{ + Name: "@", + Text: `hello dns!`, + TTL: time.Second * 3600, + }, + libdns.Address{ + Name: "www", + IP: netip.MustParseAddr("127.0.0.1"), + TTL: 3600 * time.Second, + }, + libdns.ServiceBinding{ + Scheme: "https", + Name: "www", + Target: ".", + Params: libdns.SvcParams{ + "alpn": []string{"h2"}, + }, + Priority: 1, + TTL: 3600 * time.Second, + }, + libdns.Address{ + Name: "www", + IP: netip.MustParseAddr("127.0.0.2"), + TTL: 3600 * time.Second, + }, + libdns.Address{ + Name: "subsub.sub", + IP: netip.MustParseAddr("127.0.0.5"), + TTL: 3600 * time.Second, + }, + libdns.MX{ + Name: "@", + Target: "mx0.example.com.", + TTL: 3600 * time.Second, + Preference: 0, + }, + libdns.MX{ + Name: "@", + Target: "mx1.example.com.", + TTL: 3600 * time.Second, + Preference: 10, + }, + libdns.SRV{ + Service: "sip", + Transport: "tcp", + Name: "sub", + Target: "sip.example.com.", + TTL: 3600 * time.Second, + Priority: 1, + Weight: 100, + Port: 5061, + }, + libdns.RR{ + Type: "URI", + Name: "_ftp._tcp", + Data: `1 2 "ftp://example.com/arst"`, + TTL: 3600 * time.Second, + }, + } + got, err := p.GetRecords(ctx, *domain+".") if err != nil { t.Fatal(err) } - if diff := cmp.Diff(records, got, cmpRecord); diff != "" { + if diff := cmp.Diff(wantCurrent, got, cmpRecord); diff != "" { t.Fatalf("p.GetRecords() unexpected diff [-want +got]: %s", diff) } }