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.

Sign up for GitHub

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

fix(Azure DNS): external_dns_registry_errors_total metrics counter value #4563

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

fix(Azure DNS): external_dns_registry_errors_total metrics counter value #4563

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f03bcae
fix external_dns_registry_errors_total metrics value 0
dongjiang1989 Jun 19, 2024
f423b60
add ApplyChanges return error case
dongjiang1989 Jun 27, 2024
3b21742
更新 azure_privatedns_test.go
dongjiang1989 Jul 4, 2024
fff8731
更新 azure_test.go
dongjiang1989 Jul 4, 2024
f2e4dee
Update provider/azure/azure_test.go
dongjiang1989 Jul 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 18 additions & 5 deletions provider/azure/azure.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package azure

import (
"context"
"errors"
"fmt"
"strings"

Expand Down Expand Up @@ -160,9 +161,9 @@ func (p *AzureProvider) ApplyChanges(ctx context.Context, changes *plan.Changes)
}

deleted, updated := p.mapChanges(zones, changes)
p.deleteRecords(ctx, deleted)
p.updateRecords(ctx, updated)
return nil
err1 := p.deleteRecords(ctx, deleted)
err2 := p.updateRecords(ctx, updated)
return errors.Join(err1, err2)
Copy link
Contributor

@Raffo Raffo Sep 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we generally fine with the change of semantic of how we deal the error? Bubbling this back up has obvious consequences. Specifically, do we want a soft error or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using errors.join makes it easier to see where an error occurred.
WDYT? @mloiseleur @johngmyers @szuecs

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn’t answer my question. This new error handling will definitely change the way metrics are reported, but also the semantics on what happens when we reach an error. I am not so sure this won’t be a surprise for some users, although we can say that erroring “the hard way” is most likely always the right thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn’t answer my question. This new error handling will definitely change the way metrics are reported, but also the semantics on what happens when we reach an error. I am not so sure this won’t be a surprise for some users, although we can say that erroring “the hard way” is most likely always the right thing.

Thanks @Raffo WDYT?
like:

if err := p.deleteRecords(ctx, deleted); err != nil {
    return err
}

if err := p.updateRecords(ctx, updated); err != nil {
    return err
}

return nil

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a different change in a different direction. You won't execute updateRecords in case deleteRecords fail and I am not sure this is the intended change. I think we should not proceed with this change as is unless you can provide a clear picture of what the error handling from the provider should be and what the consequences could be for the users of the provider.

}

func (p *AzureProvider) zones(ctx context.Context) ([]dns.Zone, error) {
Expand Down Expand Up @@ -235,8 +236,9 @@ func (p *AzureProvider) mapChanges(zones []dns.Zone, changes *plan.Changes) (azu
return deleted, updated
}

func (p *AzureProvider) deleteRecords(ctx context.Context, deleted azureChangeMap) {
func (p *AzureProvider) deleteRecords(ctx context.Context, deleted azureChangeMap) error {
// Delete records first
var failedCount uint64
for zone, endpoints := range deleted {
for _, ep := range endpoints {
name := p.recordSetNameForZone(zone, ep)
Expand All @@ -249,6 +251,7 @@ func (p *AzureProvider) deleteRecords(ctx context.Context, deleted azureChangeMa
} else {
log.Infof("Deleting %s record named '%s' for Azure DNS zone '%s'.", ep.RecordType, name, zone)
if _, err := p.recordSetsClient.Delete(ctx, p.resourceGroup, zone, name, dns.RecordType(ep.RecordType), nil); err != nil {
failedCount++
log.Errorf(
"Failed to delete %s record named '%s' for Azure DNS zone '%s': %v",
ep.RecordType,
Expand All @@ -260,9 +263,14 @@ func (p *AzureProvider) deleteRecords(ctx context.Context, deleted azureChangeMa
}
}
}
if failedCount > 0 {
return fmt.Errorf("Failed to delete records for Azure DNS zone: %d", failedCount)
}
return nil
}

func (p *AzureProvider) updateRecords(ctx context.Context, updated azureChangeMap) {
func (p *AzureProvider) updateRecords(ctx context.Context, updated azureChangeMap) error {
var failedCount uint64
for zone, endpoints := range updated {
for _, ep := range endpoints {
name := p.recordSetNameForZone(zone, ep)
Expand Down Expand Up @@ -302,6 +310,7 @@ func (p *AzureProvider) updateRecords(ctx context.Context, updated azureChangeMa
)
}
if err != nil {
failedCount++
log.Errorf(
"Failed to update %s record named '%s' to '%s' for DNS zone '%s': %v",
ep.RecordType,
Expand All @@ -313,6 +322,10 @@ func (p *AzureProvider) updateRecords(ctx context.Context, updated azureChangeMa
}
}
}
if failedCount > 0 {
return fmt.Errorf("Failed to update records for Azure DNS zone: %d", failedCount)
}
return nil
}

func (p *AzureProvider) recordSetNameForZone(zone string, endpoint *endpoint.Endpoint) string {
Expand Down
25 changes: 20 additions & 5 deletions provider/azure/azure_private_dns.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package azure

import (
"context"
"errors"
"fmt"
"strings"

Expand Down Expand Up @@ -170,9 +171,9 @@ func (p *AzurePrivateDNSProvider) ApplyChanges(ctx context.Context, changes *pla
}

deleted, updated := p.mapChanges(zones, changes)
p.deleteRecords(ctx, deleted)
p.updateRecords(ctx, updated)
return nil
err1 := p.deleteRecords(ctx, deleted)
err2 := p.updateRecords(ctx, updated)
return errors.Join(err1, err2)
}

func (p *AzurePrivateDNSProvider) zones(ctx context.Context) ([]privatedns.PrivateZone, error) {
Expand Down Expand Up @@ -241,9 +242,10 @@ func (p *AzurePrivateDNSProvider) mapChanges(zones []privatedns.PrivateZone, cha
return deleted, updated
}

func (p *AzurePrivateDNSProvider) deleteRecords(ctx context.Context, deleted azurePrivateDNSChangeMap) {
func (p *AzurePrivateDNSProvider) deleteRecords(ctx context.Context, deleted azurePrivateDNSChangeMap) error {
log.Debugf("Records to be deleted: %d", len(deleted))
// Delete records first
var failedCount uint64
for zone, endpoints := range deleted {
for _, ep := range endpoints {
name := p.recordSetNameForZone(zone, ep)
Expand All @@ -256,6 +258,7 @@ func (p *AzurePrivateDNSProvider) deleteRecords(ctx context.Context, deleted azu
} else {
log.Infof("Deleting %s record named '%s' for Azure Private DNS zone '%s'.", ep.RecordType, name, zone)
if _, err := p.recordSetsClient.Delete(ctx, p.resourceGroup, zone, privatedns.RecordType(ep.RecordType), name, nil); err != nil {
failedCount++
log.Errorf(
"Failed to delete %s record named '%s' for Azure Private DNS zone '%s': %v",
ep.RecordType,
Expand All @@ -267,10 +270,17 @@ func (p *AzurePrivateDNSProvider) deleteRecords(ctx context.Context, deleted azu
}
}
}

if failedCount > 0 {
return fmt.Errorf("Failed to delete records for Azure Private DNS zone: %d", failedCount)
}

return nil
}

func (p *AzurePrivateDNSProvider) updateRecords(ctx context.Context, updated azurePrivateDNSChangeMap) {
func (p *AzurePrivateDNSProvider) updateRecords(ctx context.Context, updated azurePrivateDNSChangeMap) error {
log.Debugf("Records to be updated: %d", len(updated))
var failedCount uint64
for zone, endpoints := range updated {
for _, ep := range endpoints {
name := p.recordSetNameForZone(zone, ep)
Expand Down Expand Up @@ -310,6 +320,7 @@ func (p *AzurePrivateDNSProvider) updateRecords(ctx context.Context, updated azu
)
}
if err != nil {
failedCount++
log.Errorf(
"Failed to update %s record named '%s' to '%s' for Azure Private DNS zone '%s': %v",
ep.RecordType,
Expand All @@ -321,6 +332,10 @@ func (p *AzurePrivateDNSProvider) updateRecords(ctx context.Context, updated azu
}
}
}
if failedCount > 0 {
return fmt.Errorf("Failed to update records for Azure Private DNS zone: %d", failedCount)
}
return nil
}

func (p *AzurePrivateDNSProvider) recordSetNameForZone(zone string, endpoint *endpoint.Endpoint) string {
Expand Down
49 changes: 49 additions & 0 deletions provider/azure/azure_privatedns_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package azure

import (
"context"
"errors"
"testing"

azcoreruntime "github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime"
Expand Down Expand Up @@ -352,6 +353,54 @@ func TestAzurePrivateDNSApplyChanges(t *testing.T) {
})
}

func TestAzurePrivateDNSApplyChangesReturnError(t *testing.T) {
recordsClient := mockPrivateRecordSetsClient{}
zones := []*privatedns.PrivateZone{
createMockPrivateZone("example.com", "/privateDnsZones/example.com"),
}

zonesClient := newMockPrivateZonesClient(zones)

provider := newAzurePrivateDNSProvider(
endpoint.NewDomainFilter([]string{"foo.example.com"}),
endpoint.NewDomainFilter([]string{"example.com"}),
provider.NewZoneIDFilter([]string{""}),
false,
"group",
&zonesClient,
&recordsClient,
)

createRecords := []*endpoint.Endpoint{}

currentRecords := []*endpoint.Endpoint{
endpoint.NewEndpoint("old.foo.example.com", endpoint.RecordTypeA, "121.212.121.212"),
endpoint.NewEndpoint("oldcname.foo.example.com", endpoint.RecordTypeCNAME, "other.com"),
endpoint.NewEndpoint("old.nope.example.com", endpoint.RecordTypeA, "121.212.121.212"),
}
updatedRecords := []*endpoint.Endpoint{
endpoint.NewEndpointWithTTL("new.foo.example.com", endpoint.RecordTypeA, 3600, "111.222.111.222"),
endpoint.NewEndpointWithTTL("new.foo.example.com", endpoint.RecordTypeAAAA, 3600, "2001::111:222:111:222"),
endpoint.NewEndpointWithTTL("newcname.foo.example.com", endpoint.RecordTypeCNAME, 10, "other.com"),
endpoint.NewEndpoint("new.nope.example.com", endpoint.RecordTypeA, "222.111.222.111"),
endpoint.NewEndpoint("new.nope.example.com", endpoint.RecordTypeAAAA, "2001::222:111:222:111"),
endpoint.NewEndpointWithTTL("@.foo.example.com", endpoint.RecordTypeNAPTR, 10, "other.com"),
}

deleteRecords := []*endpoint.Endpoint{}

changes := &plan.Changes{
Create: createRecords,
UpdateNew: updatedRecords,
UpdateOld: currentRecords,
Delete: deleteRecords,
}

if err := provider.ApplyChanges(context.Background(), changes); err == nil {
t.Fatal(errors.New("ApplyChanges NotError"))
}
}

func TestAzurePrivateDNSApplyChangesDryRun(t *testing.T) {
recordsClient := mockRecordSetsClient{}

Expand Down
46 changes: 46 additions & 0 deletions provider/azure/azure_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package azure

import (
"context"
"errors"
"testing"

azcoreruntime "github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime"
Expand Down Expand Up @@ -358,6 +359,51 @@ func TestAzureApplyChanges(t *testing.T) {
})
}

func TestAzureApplyChangesReturnError(t *testing.T) {
recordsClient := mockRecordSetsClient{}
zonesClient := newMockZonesClient([]*dns.Zone{createMockZone("example.com", "/dnszones/example.com")})
provider := newAzureProvider(
endpoint.NewDomainFilter([]string{"foo.example.com"}),
endpoint.NewDomainFilter([]string{"example.com"}),
provider.NewZoneIDFilter([]string{""}),
false,
"group",
"",
"",
&zonesClient,
&recordsClient,
)

createRecords := []*endpoint.Endpoint{}

currentRecords := []*endpoint.Endpoint{
endpoint.NewEndpoint("old.foo.example.com", endpoint.RecordTypeA, "121.212.121.212"),
endpoint.NewEndpoint("oldcname.foo.example.com", endpoint.RecordTypeCNAME, "other.com"),
endpoint.NewEndpoint("old.nope.example.com", endpoint.RecordTypeA, "121.212.121.212"),
}
updatedRecords := []*endpoint.Endpoint{
endpoint.NewEndpointWithTTL("new.foo.example.com", endpoint.RecordTypeA, 3600, "111.222.111.222"),
endpoint.NewEndpointWithTTL("new.foo.example.com", endpoint.RecordTypeAAAA, 3600, "2001::111:222:111:222"),
endpoint.NewEndpointWithTTL("newcname.foo.example.com", endpoint.RecordTypeCNAME, 10, "other.com"),
endpoint.NewEndpoint("new.nope.example.com", endpoint.RecordTypeA, "222.111.222.111"),
endpoint.NewEndpoint("new.nope.example.com", endpoint.RecordTypeAAAA, "2001::222:111:222:111"),
endpoint.NewEndpoint("@.new.example.com", endpoint.RecordTypeAAAA, "aabb::222:111:222:111"),
}

deleteRecords := []*endpoint.Endpoint{}

changes := &plan.Changes{
Create: createRecords,
UpdateNew: updatedRecords,
UpdateOld: currentRecords,
Delete: deleteRecords,
}

if err := provider.ApplyChanges(context.Background(), changes); err != nil {
t.Fatal(errors.New("ApplyChanges Error"))
}
}

func TestAzureApplyChangesDryRun(t *testing.T) {
recordsClient := mockRecordSetsClient{}

Expand Down
Loading