From 95d9c3c71d911841a5ac3a8a4f14eb8bfbed5e0b Mon Sep 17 00:00:00 2001 From: Nicolas Maupu Date: Thu, 5 Dec 2024 11:33:52 +0100 Subject: [PATCH 1/8] OVH: fixing cache refresh issues and duplicates - Cache was not updated when a change occurred which resulted in a change to be redone at each loop - OVH DNS can have multiple times the exact same entry. Make sure we process all the IDs (instead of picking indefinitely the first one) --- provider/ovh/ovh.go | 97 +++++++++++++++++++++++++++++----------- provider/ovh/ovh_test.go | 7 ++- 2 files changed, 77 insertions(+), 27 deletions(-) diff --git a/provider/ovh/ovh.go b/provider/ovh/ovh.go index d75d343bc5..0fcb08807b 100644 --- a/provider/ovh/ovh.go +++ b/provider/ovh/ovh.go @@ -137,20 +137,53 @@ func (p *OVHProvider) Records(ctx context.Context) ([]*endpoint.Endpoint, error) } // ApplyChanges applies a given set of changes in a given zone. -func (p *OVHProvider) ApplyChanges(ctx context.Context, changes *plan.Changes) error { +func (p *OVHProvider) ApplyChanges(ctx context.Context, changes *plan.Changes) (err error) { zones, records, err := p.zonesRecords(ctx) - zonesChangeUniques := map[string]bool{} if err != nil { - return err + return provider.NewSoftError(err) } - allChanges := make([]ovhChange, 0, countTargets(changes.Create, changes.UpdateNew, changes.UpdateOld, changes.Delete)) + zonesChangeUniques := map[string]bool{} + + // Always refresh zones even in case of errors. + defer func() { + log.Infof("OVH: %d zones will be refreshed", len(zonesChangeUniques)) + + eg, _ := errgroup.WithContext(ctx) + for zone := range zonesChangeUniques { + zone := zone + eg.Go(func() error { return p.refresh(zone) }) + } - allChanges = append(allChanges, newOvhChange(ovhCreate, changes.Create, zones, records)...) - allChanges = append(allChanges, newOvhChange(ovhCreate, changes.UpdateNew, zones, records)...) + if e := eg.Wait(); e != nil && err == nil { // return the error only if there is no error during the changes + err = provider.NewSoftError(e) + } + }() - allChanges = append(allChanges, newOvhChange(ovhDelete, changes.UpdateOld, zones, records)...) - allChanges = append(allChanges, newOvhChange(ovhDelete, changes.Delete, zones, records)...) + allChanges := make([]ovhChange, 0, countTargets(changes.Create, changes.UpdateNew, changes.UpdateOld, changes.Delete)) + + changesCreate := newOvhChange(ovhCreate, changes.Create, zones, records) + changesUpdateNew := newOvhChange(ovhCreate, changes.UpdateNew, zones, records) + changesUpdateOld := newOvhChange(ovhDelete, changes.UpdateOld, zones, records) + changesDelete := newOvhChange(ovhDelete, changes.Delete, zones, records) + + allChanges = append(allChanges, changesCreate...) + allChanges = append(allChanges, changesUpdateNew...) + allChanges = append(allChanges, changesUpdateOld...) + allChanges = append(allChanges, changesDelete...) + + // Changes debug + if log.IsLevelEnabled(log.DebugLevel) { + debugF := func(changeType string, changes []ovhChange) { + for _, c := range changes { + log.Debugf("OVH: change type %s - %s", changeType, c.String()) + } + } + debugF("OVH: (ovhCreate) Create", changesCreate) + debugF("OVH: (ovhCreate) UpdateNew", changesUpdateNew) + debugF("OVH: (ovhDelete) UpdateOld", changesUpdateOld) + debugF("OVH: (ovhDelete) Delete", changesDelete) + } log.Infof("OVH: %d changes will be done", len(allChanges)) @@ -161,27 +194,24 @@ func (p *OVHProvider) ApplyChanges(ctx context.Context, changes *plan.Changes) e eg.Go(func() error { return p.change(change) }) } if err := eg.Wait(); err != nil { - return err + return provider.NewSoftError(err) } - log.Infof("OVH: %d zones will be refreshed", len(zonesChangeUniques)) - - eg, _ = errgroup.WithContext(ctx) - for zone := range zonesChangeUniques { - zone := zone - eg.Go(func() error { return p.refresh(zone) }) - } - if err := eg.Wait(); err != nil { - return err - } return nil } func (p *OVHProvider) refresh(zone string) error { log.Debugf("OVH: Refresh %s zone", zone) + // Zone has been altered so we invalidate the cache + // so that the next run will reload it. + p.invalidateCache(zone) + p.apiRateLimiter.Take() - return p.client.Post(fmt.Sprintf("/domain/zone/%s/refresh", zone), nil, nil) + if err := p.client.Post(fmt.Sprintf("/domain/zone/%s/refresh", zone), nil, nil); err != nil { + return provider.NewSoftError(err) + } + return nil } func (p *OVHProvider) change(change ovhChange) error { @@ -201,11 +231,15 @@ func (p *OVHProvider) change(change ovhChange) error { return nil } +func (p *OVHProvider) invalidateCache(zone string) { + p.cacheInstance.Delete(zone + "#soa") +} + func (p *OVHProvider) zonesRecords(ctx context.Context) ([]string, []ovhRecord, error) { var allRecords []ovhRecord zones, err := p.zones() if err != nil { - return nil, nil, err + return nil, nil, provider.NewSoftError(err) } chRecords := make(chan []ovhRecord, len(zones)) @@ -215,7 +249,7 @@ func (p *OVHProvider) zonesRecords(ctx context.Context) ([]string, []ovhRecord, eg.Go(func() error { return p.records(&ctx, &zone, chRecords) }) } if err := eg.Wait(); err != nil { - return nil, nil, err + return nil, nil, provider.NewSoftError(err) } close(chRecords) for records := range chRecords { @@ -270,7 +304,7 @@ func (p *OVHProvider) records(ctx *context.Context, zone *string, records chan<- } } - p.cacheInstance.Delete(*zone + "#soa") + p.invalidateCache(*zone) } } @@ -359,6 +393,10 @@ func ovhGroupByNameAndType(records []ovhRecord) []*endpoint.Endpoint { } func newOvhChange(action int, endpoints []*endpoint.Endpoint, zones []string, records []ovhRecord) []ovhChange { + // Copy the records because we need to mutate the list. + newRecords := make([]ovhRecord, len(records)) + copy(newRecords, records) + zoneNameIDMapper := provider.ZoneIDName{} ovhChanges := make([]ovhChange, 0, countTargets(endpoints)) for _, zone := range zones { @@ -390,9 +428,16 @@ func newOvhChange(action int, endpoints []*endpoint.Endpoint, zones []string, re if e.RecordTTL.IsConfigured() { change.TTL = int64(e.RecordTTL) } - for _, record := range records { - if record.Zone == change.Zone && record.SubDomain == change.SubDomain && record.FieldType == change.FieldType && record.Target == change.Target { - change.ID = record.ID + + // The Zone might have multiple records with the same target. In order to avoid applying the action to the + // same OVH record, we remove a record from the list when a match is found. + for i := 0; i < len(newRecords); i++ { + rec := newRecords[i] + if rec.Zone == change.Zone && rec.SubDomain == change.SubDomain && rec.FieldType == change.FieldType && rec.Target == change.Target { + change.ID = rec.ID + // Deleting this record from the list to avoid retargetting it later if a change with a similar target exists. + newRecords = append(newRecords[:i], newRecords[i+1:]...) + break } } ovhChanges = append(ovhChanges, change) diff --git a/provider/ovh/ovh_test.go b/provider/ovh/ovh_test.go index 9aea3ad85f..56db3fbbbd 100644 --- a/provider/ovh/ovh_test.go +++ b/provider/ovh/ovh_test.go @@ -324,14 +324,18 @@ func TestOvhNewChange(t *testing.T) { // Delete change endpoints = []*endpoint.Endpoint{ - {DNSName: "ovh.example.net", RecordType: "A", Targets: []string{"203.0.113.42"}}, + {DNSName: "ovh.example.net", RecordType: "A", Targets: []string{"203.0.113.42", "203.0.113.42", "203.0.113.42"}}, } records := []ovhRecord{ {ID: 42, Zone: "example.net", ovhRecordFields: ovhRecordFields{FieldType: "A", SubDomain: "ovh", Target: "203.0.113.42"}}, + {ID: 43, Zone: "example.net", ovhRecordFields: ovhRecordFields{FieldType: "A", SubDomain: "ovh", Target: "203.0.113.42"}}, + {ID: 44, Zone: "example.net", ovhRecordFields: ovhRecordFields{FieldType: "A", SubDomain: "ovh", Target: "203.0.113.42"}}, } changes = newOvhChange(ovhDelete, endpoints, []string{"example.net"}, records) assert.ElementsMatch(changes, []ovhChange{ {Action: ovhDelete, ovhRecord: ovhRecord{ID: 42, Zone: "example.net", ovhRecordFields: ovhRecordFields{SubDomain: "ovh", FieldType: "A", TTL: ovhDefaultTTL, Target: "203.0.113.42"}}}, + {Action: ovhDelete, ovhRecord: ovhRecord{ID: 43, Zone: "example.net", ovhRecordFields: ovhRecordFields{SubDomain: "ovh", FieldType: "A", TTL: ovhDefaultTTL, Target: "203.0.113.42"}}}, + {Action: ovhDelete, ovhRecord: ovhRecord{ID: 44, Zone: "example.net", ovhRecordFields: ovhRecordFields{SubDomain: "ovh", FieldType: "A", TTL: ovhDefaultTTL, Target: "203.0.113.42"}}}, }) } @@ -368,6 +372,7 @@ func TestOvhApplyChanges(t *testing.T) { client.On("Get", "/domain/zone").Return([]string{"example.net"}, nil).Once() client.On("Get", "/domain/zone/example.net/record").Return([]uint64{}, nil).Once() client.On("Post", "/domain/zone/example.net/record", ovhRecordFields{SubDomain: "", FieldType: "A", TTL: 10, Target: "203.0.113.42"}).Return(nil, ovh.ErrAPIDown).Once() + client.On("Post", "/domain/zone/example.net/refresh", nil).Return(nil, nil).Once() assert.Error(provider.ApplyChanges(context.TODO(), &plan.Changes{ Create: []*endpoint.Endpoint{ {DNSName: ".example.net", RecordType: "A", RecordTTL: 10, Targets: []string{"203.0.113.42"}}, From 4fc95194fa51ca2ebf3f46931bc5c6b3e927eb03 Mon Sep 17 00:00:00 2001 From: Nicolas Maupu Date: Sat, 21 Dec 2024 19:42:55 +0100 Subject: [PATCH 2/8] Removing debug information Co-authored-by: Michel Loiseleur <97035654+mloiseleur@users.noreply.github.com> --- provider/ovh/ovh.go | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/provider/ovh/ovh.go b/provider/ovh/ovh.go index 0fcb08807b..027a47174b 100644 --- a/provider/ovh/ovh.go +++ b/provider/ovh/ovh.go @@ -172,18 +172,6 @@ func (p *OVHProvider) ApplyChanges(ctx context.Context, changes *plan.Changes) ( allChanges = append(allChanges, changesUpdateOld...) allChanges = append(allChanges, changesDelete...) - // Changes debug - if log.IsLevelEnabled(log.DebugLevel) { - debugF := func(changeType string, changes []ovhChange) { - for _, c := range changes { - log.Debugf("OVH: change type %s - %s", changeType, c.String()) - } - } - debugF("OVH: (ovhCreate) Create", changesCreate) - debugF("OVH: (ovhCreate) UpdateNew", changesUpdateNew) - debugF("OVH: (ovhDelete) UpdateOld", changesUpdateOld) - debugF("OVH: (ovhDelete) Delete", changesDelete) - } log.Infof("OVH: %d changes will be done", len(allChanges)) From 89aa8e1620ea286024f8b8f6afa7ab0f669cc5d0 Mon Sep 17 00:00:00 2001 From: Nicolas Maupu Date: Sat, 21 Dec 2024 19:43:43 +0100 Subject: [PATCH 3/8] Fixing log from info -> debug Co-authored-by: Michel Loiseleur <97035654+mloiseleur@users.noreply.github.com> --- provider/ovh/ovh.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/provider/ovh/ovh.go b/provider/ovh/ovh.go index 027a47174b..34fd32918a 100644 --- a/provider/ovh/ovh.go +++ b/provider/ovh/ovh.go @@ -147,7 +147,7 @@ func (p *OVHProvider) ApplyChanges(ctx context.Context, changes *plan.Changes) ( // Always refresh zones even in case of errors. defer func() { - log.Infof("OVH: %d zones will be refreshed", len(zonesChangeUniques)) + log.Debugf("OVH: %d zones will be refreshed", len(zonesChangeUniques)) eg, _ := errgroup.WithContext(ctx) for zone := range zonesChangeUniques { From f6741545da22d85558991d5ae46ad60d51f04e6a Mon Sep 17 00:00:00 2001 From: Nicolas Maupu Date: Sat, 21 Dec 2024 19:49:17 +0100 Subject: [PATCH 4/8] Removed useless newline --- provider/ovh/ovh.go | 1 - 1 file changed, 1 deletion(-) diff --git a/provider/ovh/ovh.go b/provider/ovh/ovh.go index 34fd32918a..ffa7b494cb 100644 --- a/provider/ovh/ovh.go +++ b/provider/ovh/ovh.go @@ -172,7 +172,6 @@ func (p *OVHProvider) ApplyChanges(ctx context.Context, changes *plan.Changes) ( allChanges = append(allChanges, changesUpdateOld...) allChanges = append(allChanges, changesDelete...) - log.Infof("OVH: %d changes will be done", len(allChanges)) eg, _ := errgroup.WithContext(ctx) From 4e899ca37dbf47dd59473727c73db1f5b2b1dad6 Mon Sep 17 00:00:00 2001 From: Nicolas Maupu Date: Sun, 22 Dec 2024 12:21:09 +0100 Subject: [PATCH 5/8] Removing temporary and useless vars Co-authored-by: Michel Loiseleur <97035654+mloiseleur@users.noreply.github.com> --- provider/ovh/ovh.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/provider/ovh/ovh.go b/provider/ovh/ovh.go index ffa7b494cb..118e93327c 100644 --- a/provider/ovh/ovh.go +++ b/provider/ovh/ovh.go @@ -167,10 +167,10 @@ func (p *OVHProvider) ApplyChanges(ctx context.Context, changes *plan.Changes) ( changesUpdateOld := newOvhChange(ovhDelete, changes.UpdateOld, zones, records) changesDelete := newOvhChange(ovhDelete, changes.Delete, zones, records) - allChanges = append(allChanges, changesCreate...) - allChanges = append(allChanges, changesUpdateNew...) - allChanges = append(allChanges, changesUpdateOld...) - allChanges = append(allChanges, changesDelete...) + allChanges = append(allChanges, newOvhChange(ovhCreate, changes.Create, zones, records)) + allChanges = append(allChanges, newOvhChange(ovhCreate, changes.UpdateNew, zones, records)) + allChanges = append(allChanges, newOvhChange(ovhCreate, changes.UpdateOld, zones, records)) + allChanges = append(allChanges, newOvhChange(ovhCreate, changes.Delete, zones, records)) log.Infof("OVH: %d changes will be done", len(allChanges)) From b7c7123982fcb60efa750969479eaa5b1aaf56de Mon Sep 17 00:00:00 2001 From: Nicolas Maupu Date: Sun, 22 Dec 2024 12:21:17 +0100 Subject: [PATCH 6/8] Removing temporary and useless vars Co-authored-by: Michel Loiseleur <97035654+mloiseleur@users.noreply.github.com> --- provider/ovh/ovh.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/provider/ovh/ovh.go b/provider/ovh/ovh.go index 118e93327c..50a508c3b1 100644 --- a/provider/ovh/ovh.go +++ b/provider/ovh/ovh.go @@ -162,10 +162,6 @@ func (p *OVHProvider) ApplyChanges(ctx context.Context, changes *plan.Changes) ( allChanges := make([]ovhChange, 0, countTargets(changes.Create, changes.UpdateNew, changes.UpdateOld, changes.Delete)) - changesCreate := newOvhChange(ovhCreate, changes.Create, zones, records) - changesUpdateNew := newOvhChange(ovhCreate, changes.UpdateNew, zones, records) - changesUpdateOld := newOvhChange(ovhDelete, changes.UpdateOld, zones, records) - changesDelete := newOvhChange(ovhDelete, changes.Delete, zones, records) allChanges = append(allChanges, newOvhChange(ovhCreate, changes.Create, zones, records)) allChanges = append(allChanges, newOvhChange(ovhCreate, changes.UpdateNew, zones, records)) From ae4ebf5af02216a1b8b8ed1140f45cd60de80ca3 Mon Sep 17 00:00:00 2001 From: Nicolas Maupu Date: Sun, 22 Dec 2024 12:25:41 +0100 Subject: [PATCH 7/8] lint --- provider/ovh/ovh.go | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/provider/ovh/ovh.go b/provider/ovh/ovh.go index 50a508c3b1..cb7ac1f2f0 100644 --- a/provider/ovh/ovh.go +++ b/provider/ovh/ovh.go @@ -161,12 +161,10 @@ func (p *OVHProvider) ApplyChanges(ctx context.Context, changes *plan.Changes) ( }() allChanges := make([]ovhChange, 0, countTargets(changes.Create, changes.UpdateNew, changes.UpdateOld, changes.Delete)) - - - allChanges = append(allChanges, newOvhChange(ovhCreate, changes.Create, zones, records)) - allChanges = append(allChanges, newOvhChange(ovhCreate, changes.UpdateNew, zones, records)) - allChanges = append(allChanges, newOvhChange(ovhCreate, changes.UpdateOld, zones, records)) - allChanges = append(allChanges, newOvhChange(ovhCreate, changes.Delete, zones, records)) + allChanges = append(allChanges, newOvhChange(ovhCreate, changes.Create, zones, records)...) + allChanges = append(allChanges, newOvhChange(ovhCreate, changes.UpdateNew, zones, records)...) + allChanges = append(allChanges, newOvhChange(ovhCreate, changes.UpdateOld, zones, records)...) + allChanges = append(allChanges, newOvhChange(ovhCreate, changes.Delete, zones, records)...) log.Infof("OVH: %d changes will be done", len(allChanges)) From 4b1ee0661822c47b3e2199a9f5625d59ae82af0f Mon Sep 17 00:00:00 2001 From: Nicolas Maupu Date: Mon, 6 Jan 2025 16:02:37 +0100 Subject: [PATCH 8/8] Fixing changes type --- provider/ovh/ovh.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/provider/ovh/ovh.go b/provider/ovh/ovh.go index cb7ac1f2f0..501ed33073 100644 --- a/provider/ovh/ovh.go +++ b/provider/ovh/ovh.go @@ -163,8 +163,8 @@ func (p *OVHProvider) ApplyChanges(ctx context.Context, changes *plan.Changes) ( allChanges := make([]ovhChange, 0, countTargets(changes.Create, changes.UpdateNew, changes.UpdateOld, changes.Delete)) allChanges = append(allChanges, newOvhChange(ovhCreate, changes.Create, zones, records)...) allChanges = append(allChanges, newOvhChange(ovhCreate, changes.UpdateNew, zones, records)...) - allChanges = append(allChanges, newOvhChange(ovhCreate, changes.UpdateOld, zones, records)...) - allChanges = append(allChanges, newOvhChange(ovhCreate, changes.Delete, zones, records)...) + allChanges = append(allChanges, newOvhChange(ovhDelete, changes.UpdateOld, zones, records)...) + allChanges = append(allChanges, newOvhChange(ovhDelete, changes.Delete, zones, records)...) log.Infof("OVH: %d changes will be done", len(allChanges))