feat(ovh): add cache based on DNS zone SOA value

OVHcloud customers using 'ovh' provider for large DNS zone will spend a
lot of time waiting for all records to be retrieved from the API. As
records can't change without a modification of the SOA Serial of the DNS
zone, this commit adds a client-side cache that stores the value of the
DNS records for each zone, based of the current SOA Serial value of the
zone. In case the SOA Serial of the zone is identical to the one in
cache, we can serve all the records directly from cache. Otherwise,
fallback on refreshing the cache from the API.

Signed-off-by: Romain Beuque <556072+rbeuque74@users.noreply.github.com>

docs/tutorials/ovh.md

@@ -18,7 +18,7 @@ instructions for creating a zone.
 
 You first need to create an OVH application.
 
-Using the [OVH documentation](https://docs.ovh.com/gb/en/customer/first-steps-with-ovh-api/#creation-of-your-application-keys) you will have your `Application key` and `Application secret`
+Using the [OVH documentation](https://docs.ovh.com/gb/en/api/first-steps-with-ovh-api/#advanced-usage-pair-ovhcloud-apis-with-an-application_2) you will have your `Application key` and `Application secret`
 
 And you will need to generate your consumer key, here the permissions needed :
 - GET on `/domain/zone`
@@ -26,6 +26,7 @@ And you will need to generate your consumer key, here the permissions needed :
 - GET on `/domain/zone/*/record/*`
 - POST on `/domain/zone/*/record`
 - DELETE on `/domain/zone/*/record/*`
+- GET on `/domain/zone/*/soa`
 - POST on `/domain/zone/*/refresh`
 
 You can use the following `curl` request to generate & validated your `Consumer key`
@@ -37,6 +38,10 @@ curl -XPOST -H "X-Ovh-Application: <ApplicationKey>" -H "Content-type: applicati
       "method": "GET",
       "path": "/domain/zone"
     },
+    {
+      "method": "GET",
+      "path": "/domain/zone/*/soa"
+    },
     {
       "method": "GET",
       "path": "/domain/zone/*/record"

go.mod

@@ -44,6 +44,7 @@ require (
 	github.com/openshift/client-go v0.0.0-20230607134213-3cd0021bbee3
 	github.com/oracle/oci-go-sdk/v65 v65.41.0
 	github.com/ovh/go-ovh v1.4.1
+	github.com/patrickmn/go-cache v2.1.0+incompatible
 	github.com/pkg/errors v0.9.1
 	github.com/pluralsh/gqlclient v1.5.1
 	github.com/projectcontour/contour v1.25.0
@@ -157,7 +158,6 @@ require (
 	github.com/oklog/ulid v1.3.1 // indirect
 	github.com/openshift/gssapi v0.0.0-20161010215902-5fb4217df13b // indirect
 	github.com/opentracing/opentracing-go v1.2.1-0.20220228012449-10b1cf09e00b // indirect
-	github.com/patrickmn/go-cache v2.1.0+incompatible // indirect
 	github.com/pelletier/go-toml/v2 v2.0.6 // indirect
 	github.com/peterhellberg/link v1.1.0 // indirect
 	github.com/pmezard/go-difflib v1.0.0 // indirect

provider/ovh/ovh.go

@@ -21,12 +21,16 @@ import (
 	"errors"
 	"fmt"
 	"strings"
+	"time"
 
+	"github.com/miekg/dns"
 	"github.com/ovh/go-ovh/ovh"
+	"github.com/patrickmn/go-cache"
 	log "github.com/sirupsen/logrus"
 	"golang.org/x/sync/errgroup"
 
 	"sigs.k8s.io/external-dns/endpoint"
+	"sigs.k8s.io/external-dns/pkg/apis/externaldns"
 	"sigs.k8s.io/external-dns/plan"
 	"sigs.k8s.io/external-dns/provider"
 
@@ -56,6 +60,17 @@ type OVHProvider struct {
 
 	domainFilter endpoint.DomainFilter
 	DryRun       bool
+
+	// UseCache controls if the OVHProvider will cache records in memory, and serve them
+	// without recontacting the OVHcloud API if the SOA of the domain zone hasn't changed.
+	// Note that, when disabling cache, OVHcloud API has rate-limiting that will hit if
+	// your refresh rate/number of records is too big, which might cause issue with the
+	// provider.
+	// Default value: true
+	UseCache bool
+
+	cacheInstance *cache.Cache
+	dnsClient     dnsClient
 }
 
 type ovhClient interface {
@@ -64,6 +79,10 @@ type ovhClient interface {
 	Delete(string, interface{}) error
 }
 
+type dnsClient interface {
+	ExchangeContext(ctx context.Context, m *dns.Msg, a string) (*dns.Msg, time.Duration, error)
+}
+
 type ovhRecordFields struct {
 	FieldType string `json:"fieldType"`
 	SubDomain string `json:"subDomain"`
@@ -88,6 +107,9 @@ func NewOVHProvider(ctx context.Context, domainFilter endpoint.DomainFilter, end
 	if err != nil {
 		return nil, err
 	}
+
+	client.UserAgent = externaldns.Version
+
 	// TODO: Add Dry Run support
 	if dryRun {
 		return nil, ErrNoDryRun
@@ -97,6 +119,9 @@ func NewOVHProvider(ctx context.Context, domainFilter endpoint.DomainFilter, end
 		domainFilter:   domainFilter,
 		apiRateLimiter: ratelimit.New(apiRateLimit),
 		DryRun:         dryRun,
+		cacheInstance:  cache.New(cache.NoExpiration, cache.NoExpiration),
+		dnsClient:      new(dns.Client),
+		UseCache:       true,
 	}, nil
 }
 
@@ -217,14 +242,48 @@ func (p *OVHProvider) zones() ([]string, error) {
 	return filteredZones, nil
 }
 
+type ovhSoa struct {
+	Server  string `json:"server"`
+	Serial  uint32 `json:"serial"`
+	records []ovhRecord
+}
+
 func (p *OVHProvider) records(ctx *context.Context, zone *string, records chan<- []ovhRecord) error {
 	var recordsIds []uint64
 	ovhRecords := make([]ovhRecord, len(recordsIds))
 	eg, _ := errgroup.WithContext(*ctx)
 
+	if p.UseCache {
+		if cachedSoaItf, ok := p.cacheInstance.Get(*zone + "#soa"); ok {
+			cachedSoa := cachedSoaItf.(ovhSoa)
+
+			m := new(dns.Msg)
+			m.SetQuestion(dns.Fqdn(*zone), dns.TypeSOA)
+			in, _, err := p.dnsClient.ExchangeContext(*ctx, m, strings.TrimSuffix(cachedSoa.Server, ".")+":53")
+			if err == nil {
+				if s, ok := in.Answer[0].(*dns.SOA); ok {
+					// do something with t.Txt
+					if s.Serial == cachedSoa.Serial {
+						records <- cachedSoa.records
+						return nil
+					}
+				}
+			}
+
+			p.cacheInstance.Delete(*zone + "#soa")
+		}
+	}
+
 	log.Debugf("OVH: Getting records for %s", *zone)
 
 	p.apiRateLimiter.Take()
+	var soa ovhSoa
+	if p.UseCache {
+		if err := p.client.Get("/domain/zone/"+*zone+"/soa", &soa); err != nil {
+			return err
+		}
+	}
+
 	if err := p.client.Get(fmt.Sprintf("/domain/zone/%s/record", *zone), &recordsIds); err != nil {
 		return err
 	}
@@ -240,6 +299,12 @@ func (p *OVHProvider) records(ctx *context.Context, zone *string, records chan<-
 	for record := range chRecords {
 		ovhRecords = append(ovhRecords, record)
 	}
+
+	if p.UseCache {
+		soa.records = ovhRecords
+		_ = p.cacheInstance.Add(*zone+"#soa", soa, time.Hour)
+	}
+
 	records <- ovhRecords
 	return nil
 }

provider/ovh/ovh_test.go

@@ -19,10 +19,14 @@ package ovh
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	"sort"
 	"testing"
+	"time"
 
+	"github.com/miekg/dns"
 	"github.com/ovh/go-ovh/ovh"
+	"github.com/patrickmn/go-cache"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
 	"go.uber.org/ratelimit"
@@ -55,6 +59,19 @@ func (c *mockOvhClient) Delete(endpoint string, output interface{}) error {
 	return stub.Error(1)
 }
 
+type mockDnsClient struct {
+	mock.Mock
+}
+
+func (c *mockDnsClient) ExchangeContext(ctx context.Context, m *dns.Msg, addr string) (*dns.Msg, time.Duration, error) {
+	args := c.Called(ctx, m, addr)
+
+	msg := args.Get(0).(*dns.Msg)
+	err := args.Error(1)
+
+	return msg, time.Duration(0), err
+}
+
 func TestOvhZones(t *testing.T) {
 	assert := assert.New(t)
 	client := new(mockOvhClient)
@@ -62,6 +79,8 @@ func TestOvhZones(t *testing.T) {
 		client:         client,
 		apiRateLimiter: ratelimit.New(10),
 		domainFilter:   endpoint.NewDomainFilter([]string{"com"}),
+		cacheInstance:  cache.New(cache.NoExpiration, cache.NoExpiration),
+		dnsClient:      new(mockDnsClient),
 	}
 
 	// Basic zones
@@ -83,10 +102,12 @@ func TestOvhZones(t *testing.T) {
 func TestOvhZoneRecords(t *testing.T) {
 	assert := assert.New(t)
 	client := new(mockOvhClient)
-	provider := &OVHProvider{client: client, apiRateLimiter: ratelimit.New(10)}
+	provider := &OVHProvider{client: client, apiRateLimiter: ratelimit.New(10), cacheInstance: cache.New(cache.NoExpiration, cache.NoExpiration), dnsClient: nil, UseCache: true}
 
 	// Basic zones records
+	t.Log("Basic zones records")
 	client.On("Get", "/domain/zone").Return([]string{"example.org"}, nil).Once()
+	client.On("Get", "/domain/zone/example.org/soa").Return(ovhSoa{Server: "ns.example.org.", Serial: 2022090901}, nil).Once()
 	client.On("Get", "/domain/zone/example.org/record").Return([]uint64{24, 42}, nil).Once()
 	client.On("Get", "/domain/zone/example.org/record/24").Return(ovhRecord{ID: 24, Zone: "example.org", ovhRecordFields: ovhRecordFields{SubDomain: "ovh", FieldType: "NS", TTL: 10, Target: "203.0.113.42"}}, nil).Once()
 	client.On("Get", "/domain/zone/example.org/record/42").Return(ovhRecord{ID: 42, Zone: "example.org", ovhRecordFields: ovhRecordFields{SubDomain: "ovh", FieldType: "A", TTL: 10, Target: "203.0.113.42"}}, nil).Once()
@@ -97,6 +118,7 @@ func TestOvhZoneRecords(t *testing.T) {
 	client.AssertExpectations(t)
 
 	// Error on getting zones list
+	t.Log("Error on getting zones list")
 	client.On("Get", "/domain/zone").Return(nil, ovh.ErrAPIDown).Once()
 	zones, records, err = provider.zonesRecords(context.TODO())
 	assert.Error(err)
@@ -104,8 +126,21 @@ func TestOvhZoneRecords(t *testing.T) {
 	assert.Nil(records)
 	client.AssertExpectations(t)
 
-	// Error on getting zone records
+	// Error on getting zone SOA
+	t.Log("Error on getting zone SOA")
+	provider.cacheInstance = cache.New(cache.NoExpiration, cache.NoExpiration)
 	client.On("Get", "/domain/zone").Return([]string{"example.org"}, nil).Once()
+	client.On("Get", "/domain/zone/example.org/soa").Return(nil, ovh.ErrAPIDown).Once()
+	zones, records, err = provider.zonesRecords(context.TODO())
+	assert.Error(err)
+	assert.Nil(zones)
+	assert.Nil(records)
+	client.AssertExpectations(t)
+
+	// Error on getting zone records
+	t.Log("Error on getting zone records")
+	client.On("Get", "/domain/zone").Return([]string{"example.org"}, nil).Once()
+	client.On("Get", "/domain/zone/example.org/soa").Return(ovhSoa{Server: "ns.example.org.", Serial: 2022090902}, nil).Once()
 	client.On("Get", "/domain/zone/example.org/record").Return(nil, ovh.ErrAPIDown).Once()
 	zones, records, err = provider.zonesRecords(context.TODO())
 	assert.Error(err)
@@ -114,7 +149,9 @@ func TestOvhZoneRecords(t *testing.T) {
 	client.AssertExpectations(t)
 
 	// Error on getting zone record detail
+	t.Log("Error on getting zone record detail")
 	client.On("Get", "/domain/zone").Return([]string{"example.org"}, nil).Once()
+	client.On("Get", "/domain/zone/example.org/soa").Return(ovhSoa{Server: "ns.example.org.", Serial: 2022090902}, nil).Once()
 	client.On("Get", "/domain/zone/example.org/record").Return([]uint64{42}, nil).Once()
 	client.On("Get", "/domain/zone/example.org/record/42").Return(nil, ovh.ErrAPIDown).Once()
 	zones, records, err = provider.zonesRecords(context.TODO())
@@ -124,10 +161,110 @@ func TestOvhZoneRecords(t *testing.T) {
 	client.AssertExpectations(t)
 }
 
+func TestOvhZoneRecordsCache(t *testing.T) {
+	assert := assert.New(t)
+	client := new(mockOvhClient)
+	dnsClient := new(mockDnsClient)
+	provider := &OVHProvider{client: client, apiRateLimiter: ratelimit.New(10), cacheInstance: cache.New(cache.NoExpiration, cache.NoExpiration), dnsClient: dnsClient, UseCache: true}
+
+	// First call, cache miss
+	t.Log("First call, cache miss")
+	client.On("Get", "/domain/zone").Return([]string{"example.org"}, nil).Once()
+	client.On("Get", "/domain/zone/example.org/soa").Return(ovhSoa{Server: "ns.example.org.", Serial: 2022090901}, nil).Once()
+	client.On("Get", "/domain/zone/example.org/record").Return([]uint64{24, 42}, nil).Once()
+	client.On("Get", "/domain/zone/example.org/record/24").Return(ovhRecord{ID: 24, Zone: "example.org", ovhRecordFields: ovhRecordFields{SubDomain: "ovh", FieldType: "NS", TTL: 10, Target: "203.0.113.42"}}, nil).Once()
+	client.On("Get", "/domain/zone/example.org/record/42").Return(ovhRecord{ID: 42, Zone: "example.org", ovhRecordFields: ovhRecordFields{SubDomain: "ovh", FieldType: "A", TTL: 10, Target: "203.0.113.42"}}, nil).Once()
+
+	zones, records, err := provider.zonesRecords(context.TODO())
+	assert.NoError(err)
+	assert.ElementsMatch(zones, []string{"example.org"})
+	assert.ElementsMatch(records, []ovhRecord{{ID: 42, Zone: "example.org", ovhRecordFields: ovhRecordFields{SubDomain: "ovh", FieldType: "A", TTL: 10, Target: "203.0.113.42"}}, {ID: 24, Zone: "example.org", ovhRecordFields: ovhRecordFields{SubDomain: "ovh", FieldType: "NS", TTL: 10, Target: "203.0.113.42"}}})
+	client.AssertExpectations(t)
+	dnsClient.AssertExpectations(t)
+
+	// reset mock
+	client = new(mockOvhClient)
+	dnsClient = new(mockDnsClient)
+	provider.client, provider.dnsClient = client, dnsClient
+
+	// second call, cache hit
+	t.Log("second call, cache hit")
+	client.On("Get", "/domain/zone").Return([]string{"example.org"}, nil).Once()
+	dnsClient.On("ExchangeContext", mock.AnythingOfType("*context.cancelCtx"), mock.AnythingOfType("*dns.Msg"), "ns.example.org:53").
+		Return(&dns.Msg{Answer: []dns.RR{&dns.SOA{Serial: 2022090901}}}, nil)
+	zones, records, err = provider.zonesRecords(context.TODO())
+	assert.NoError(err)
+	assert.ElementsMatch(zones, []string{"example.org"})
+	assert.ElementsMatch(records, []ovhRecord{{ID: 42, Zone: "example.org", ovhRecordFields: ovhRecordFields{SubDomain: "ovh", FieldType: "A", TTL: 10, Target: "203.0.113.42"}}, {ID: 24, Zone: "example.org", ovhRecordFields: ovhRecordFields{SubDomain: "ovh", FieldType: "NS", TTL: 10, Target: "203.0.113.42"}}})
+	client.AssertExpectations(t)
+	dnsClient.AssertExpectations(t)
+
+	// reset mock
+	client = new(mockOvhClient)
+	dnsClient = new(mockDnsClient)
+	provider.client, provider.dnsClient = client, dnsClient
+
+	// third call, cache out of date
+	t.Log("third call, cache out of date")
+	client.On("Get", "/domain/zone").Return([]string{"example.org"}, nil).Once()
+	dnsClient.On("ExchangeContext", mock.AnythingOfType("*context.cancelCtx"), mock.AnythingOfType("*dns.Msg"), "ns.example.org:53").
+		Return(&dns.Msg{Answer: []dns.RR{&dns.SOA{Serial: 2022090902}}}, nil)
+	client.On("Get", "/domain/zone/example.org/soa").Return(ovhSoa{Server: "ns.example.org.", Serial: 2022090902}, nil).Once()
+	client.On("Get", "/domain/zone/example.org/record").Return([]uint64{24}, nil).Once()
+	client.On("Get", "/domain/zone/example.org/record/24").Return(ovhRecord{ID: 24, Zone: "example.org", ovhRecordFields: ovhRecordFields{SubDomain: "ovh", FieldType: "NS", TTL: 10, Target: "203.0.113.42"}}, nil).Once()
+
+	zones, records, err = provider.zonesRecords(context.TODO())
+	assert.NoError(err)
+	assert.ElementsMatch(zones, []string{"example.org"})
+	assert.ElementsMatch(records, []ovhRecord{{ID: 24, Zone: "example.org", ovhRecordFields: ovhRecordFields{SubDomain: "ovh", FieldType: "NS", TTL: 10, Target: "203.0.113.42"}}})
+	client.AssertExpectations(t)
+	dnsClient.AssertExpectations(t)
+
+	// reset mock
+	client = new(mockOvhClient)
+	dnsClient = new(mockDnsClient)
+	provider.client, provider.dnsClient = client, dnsClient
+
+	// fourth call, cache hit
+	t.Log("fourth call, cache hit")
+	client.On("Get", "/domain/zone").Return([]string{"example.org"}, nil).Once()
+	dnsClient.On("ExchangeContext", mock.AnythingOfType("*context.cancelCtx"), mock.AnythingOfType("*dns.Msg"), "ns.example.org:53").
+		Return(&dns.Msg{Answer: []dns.RR{&dns.SOA{Serial: 2022090902}}}, nil)
+
+	zones, records, err = provider.zonesRecords(context.TODO())
+	assert.NoError(err)
+	assert.ElementsMatch(zones, []string{"example.org"})
+	assert.ElementsMatch(records, []ovhRecord{{ID: 24, Zone: "example.org", ovhRecordFields: ovhRecordFields{SubDomain: "ovh", FieldType: "NS", TTL: 10, Target: "203.0.113.42"}}})
+	client.AssertExpectations(t)
+	dnsClient.AssertExpectations(t)
+
+	// reset mock
+	client = new(mockOvhClient)
+	dnsClient = new(mockDnsClient)
+	provider.client, provider.dnsClient = client, dnsClient
+
+	// fifth call, dns issue
+	t.Log("fourth call, cache hit")
+	client.On("Get", "/domain/zone").Return([]string{"example.org"}, nil).Once()
+	dnsClient.On("ExchangeContext", mock.AnythingOfType("*context.cancelCtx"), mock.AnythingOfType("*dns.Msg"), "ns.example.org:53").
+		Return(&dns.Msg{Answer: []dns.RR{}}, errors.New("dns issue"))
+	client.On("Get", "/domain/zone/example.org/soa").Return(ovhSoa{Server: "ns.example.org.", Serial: 2022090903}, nil).Once()
+	client.On("Get", "/domain/zone/example.org/record").Return([]uint64{24, 42}, nil).Once()
+	client.On("Get", "/domain/zone/example.org/record/24").Return(ovhRecord{ID: 24, Zone: "example.org", ovhRecordFields: ovhRecordFields{SubDomain: "ovh", FieldType: "NS", TTL: 10, Target: "203.0.113.42"}}, nil).Once()
+	client.On("Get", "/domain/zone/example.org/record/42").Return(ovhRecord{ID: 42, Zone: "example.org", ovhRecordFields: ovhRecordFields{SubDomain: "ovh", FieldType: "A", TTL: 10, Target: "203.0.113.42"}}, nil).Once()
+
+	zones, records, err = provider.zonesRecords(context.TODO())
+	assert.NoError(err)
+	assert.ElementsMatch(zones, []string{"example.org"})
+	assert.ElementsMatch(records, []ovhRecord{{ID: 42, Zone: "example.org", ovhRecordFields: ovhRecordFields{SubDomain: "ovh", FieldType: "A", TTL: 10, Target: "203.0.113.42"}}, {ID: 24, Zone: "example.org", ovhRecordFields: ovhRecordFields{SubDomain: "ovh", FieldType: "NS", TTL: 10, Target: "203.0.113.42"}}})
+	client.AssertExpectations(t)
+	dnsClient.AssertExpectations(t)
+}
+
 func TestOvhRecords(t *testing.T) {
 	assert := assert.New(t)
 	client := new(mockOvhClient)
-	provider := &OVHProvider{client: client, apiRateLimiter: ratelimit.New(10)}
+	provider := &OVHProvider{client: client, apiRateLimiter: ratelimit.New(10), cacheInstance: cache.New(cache.NoExpiration, cache.NoExpiration)}
 
 	// Basic zones records
 	client.On("Get", "/domain/zone").Return([]string{"example.org", "example.net"}, nil).Once()
@@ -160,7 +297,7 @@ func TestOvhRecords(t *testing.T) {
 
 func TestOvhRefresh(t *testing.T) {
 	client := new(mockOvhClient)
-	provider := &OVHProvider{client: client, apiRateLimiter: ratelimit.New(10)}
+	provider := &OVHProvider{client: client, apiRateLimiter: ratelimit.New(10), cacheInstance: cache.New(cache.NoExpiration, cache.NoExpiration)}
 
 	// Basic zone refresh
 	client.On("Post", "/domain/zone/example.net/refresh", nil).Return(nil, nil).Once()
@@ -201,7 +338,7 @@ func TestOvhNewChange(t *testing.T) {
 func TestOvhApplyChanges(t *testing.T) {
 	assert := assert.New(t)
 	client := new(mockOvhClient)
-	provider := &OVHProvider{client: client, apiRateLimiter: ratelimit.New(10)}
+	provider := &OVHProvider{client: client, apiRateLimiter: ratelimit.New(10), cacheInstance: cache.New(cache.NoExpiration, cache.NoExpiration)}
 	changes := plan.Changes{
 		Create: []*endpoint.Endpoint{
 			{DNSName: ".example.net", RecordType: "A", RecordTTL: 10, Targets: []string{"203.0.113.42"}},
@@ -254,7 +391,7 @@ func TestOvhApplyChanges(t *testing.T) {
 func TestOvhChange(t *testing.T) {
 	assert := assert.New(t)
 	client := new(mockOvhClient)
-	provider := &OVHProvider{client: client, apiRateLimiter: ratelimit.New(10)}
+	provider := &OVHProvider{client: client, apiRateLimiter: ratelimit.New(10), cacheInstance: cache.New(cache.NoExpiration, cache.NoExpiration)}
 
 	// Record creation
 	client.On("Post", "/domain/zone/example.net/record", ovhRecordFields{SubDomain: "ovh"}).Return(nil, nil).Once()