From dead7c221121370df87fff466c8ff7a83d187be9 Mon Sep 17 00:00:00 2001 From: Bartosz Slawianowski Date: Mon, 28 Aug 2023 11:37:40 +0200 Subject: [PATCH 1/5] feat: Support concurrent updates for Azure DNS Signed-off-by: Bartosz Slawianowski --- pkg/issuer/acme/dns/azuredns/azuredns.go | 60 ++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/pkg/issuer/acme/dns/azuredns/azuredns.go b/pkg/issuer/acme/dns/azuredns/azuredns.go index da5a768e1ff..aca2c92dacd 100644 --- a/pkg/issuer/acme/dns/azuredns/azuredns.go +++ b/pkg/issuer/acme/dns/azuredns/azuredns.go @@ -255,3 +255,63 @@ func stabilizeError(err error) error { return err } + +// Updates or removes DNS TXT record while respecting optimistic concurrency control +func (c *DNSProvider) updateTXTRecord(zone, name string, updater func(*dns.RecordSet)) error { + var set *dns.RecordSet + + resp, err := c.recordClient.Get(context.TODO(), c.resourceGroupName, zone, name, dns.RecordTypeTXT, nil) + if err != nil { + var respErr *azcore.ResponseError + if errors.As(err, &respErr); respErr.StatusCode == 404 { + set = &dns.RecordSet{ + Properties: &dns.RecordSetProperties{ + TTL: to.Ptr(int64(60)), + TxtRecords: []*dns.TxtRecord{}, + }, + Etag: to.Ptr(""), + } + } else { + return fmt.Errorf("cannot get DNS record set: %w", err) + } + } else { + set = &resp.RecordSet + } + + updater(set) + + if len(set.Properties.TxtRecords) == 0 { + if *set.Etag != "" { + // Etag will cause the deletion to fail if any updates happen concurrently + _, err = c.recordClient.Delete(context.TODO(), c.resourceGroupName, zone, name, dns.RecordTypeTXT, &dns.RecordSetsClientDeleteOptions{IfMatch: set.Etag}) + if err != nil { + return fmt.Errorf("cannot delete DNS record set: %w", err) + } + } + + return nil + } + + opts := &dns.RecordSetsClientCreateOrUpdateOptions{} + if *set.Etag == "" { + // This is used to indicate that we want the API call to fail if a conflicting record was created concurrently + // Only relevant when this is a new record, for updates conflicts are solved with Etag + opts.IfNoneMatch = to.Ptr("*") + } else { + opts.IfMatch = set.Etag + } + + _, err = c.recordClient.CreateOrUpdate( + context.TODO(), + c.resourceGroupName, + zone, + name, + dns.RecordTypeTXT, + *set, + opts) + if err != nil { + return fmt.Errorf("cannot upsert DNS record set: %w", err) + } + + return nil +} From 53f73d589162a89417d025706c1c8f9c59934aee Mon Sep 17 00:00:00 2001 From: Bartosz Slawianowski Date: Mon, 18 Sep 2023 16:30:17 +0200 Subject: [PATCH 2/5] Fix error handling and add basic test Signed-off-by: Bartosz Slawianowski --- pkg/issuer/acme/dns/azuredns/azuredns_test.go | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/pkg/issuer/acme/dns/azuredns/azuredns_test.go b/pkg/issuer/acme/dns/azuredns/azuredns_test.go index 1d5947e4bba..c8d9f85cb6a 100644 --- a/pkg/issuer/acme/dns/azuredns/azuredns_test.go +++ b/pkg/issuer/acme/dns/azuredns/azuredns_test.go @@ -69,6 +69,19 @@ func TestLiveAzureDnsPresent(t *testing.T) { assert.NoError(t, err) } +func TestLiveAzureDnsPresentMultiple(t *testing.T) { + if !azureLiveTest { + t.Skip("skipping live test") + } + provider, err := NewDNSProviderCredentials("", azureClientID, azureClientSecret, azuresubscriptionID, azureTenantID, azureResourceGroupName, azureHostedZoneName, util.RecursiveNameservers, false, &v1.AzureManagedIdentity{}) + assert.NoError(t, err) + + err = provider.Present(azureDomain, "_acme-challenge."+azureDomain+".", "123d==") + assert.NoError(t, err) + err = provider.Present(azureDomain, "_acme-challenge."+azureDomain+".", "1123d==") + assert.NoError(t, err) +} + func TestLiveAzureDnsCleanUp(t *testing.T) { if !azureLiveTest { t.Skip("skipping live test") @@ -83,6 +96,22 @@ func TestLiveAzureDnsCleanUp(t *testing.T) { assert.NoError(t, err) } +func TestLiveAzureDnsCleanUpMultiple(t *testing.T) { + if !azureLiveTest { + t.Skip("skipping live test") + } + + time.Sleep(time.Second * 10) + + provider, err := NewDNSProviderCredentials("", azureClientID, azureClientSecret, azuresubscriptionID, azureTenantID, azureResourceGroupName, azureHostedZoneName, util.RecursiveNameservers, false, &v1.AzureManagedIdentity{}) + assert.NoError(t, err) + + err = provider.CleanUp(azureDomain, "_acme-challenge."+azureDomain+".", "123d==") + assert.NoError(t, err) + err = provider.CleanUp(azureDomain, "_acme-challenge."+azureDomain+".", "1123d==") + assert.NoError(t, err) +} + func TestInvalidAzureDns(t *testing.T) { validEnv := []string{"", "AzurePublicCloud", "AzureChinaCloud", "AzureUSGovernmentCloud"} for _, env := range validEnv { From 747d88ce663593e110d4abbb6392d6b967c0265c Mon Sep 17 00:00:00 2001 From: Bartosz Slawianowski Date: Fri, 10 May 2024 11:05:14 +0200 Subject: [PATCH 3/5] Rewrite to new Azure SDK Signed-off-by: Bartosz Slawianowski --- pkg/issuer/acme/dns/azuredns/azuredns.go | 163 ++++++++---------- pkg/issuer/acme/dns/azuredns/azuredns_test.go | 8 +- 2 files changed, 80 insertions(+), 91 deletions(-) diff --git a/pkg/issuer/acme/dns/azuredns/azuredns.go b/pkg/issuer/acme/dns/azuredns/azuredns.go index aca2c92dacd..f7a1b3a949a 100644 --- a/pkg/issuer/acme/dns/azuredns/azuredns.go +++ b/pkg/issuer/acme/dns/azuredns/azuredns.go @@ -27,6 +27,7 @@ import ( "github.com/Azure/azure-sdk-for-go/sdk/azcore/to" "github.com/Azure/azure-sdk-for-go/sdk/azidentity" dns "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/dns/armdns" + "github.com/aws/smithy-go/ptr" "github.com/go-logr/logr" cmacme "github.com/cert-manager/cert-manager/pkg/apis/acme/v1" @@ -138,57 +139,35 @@ func getAuthorization(clientOpt policy.ClientOptions, clientID, clientSecret, te // Present creates a TXT record using the specified parameters func (c *DNSProvider) Present(ctx context.Context, domain, fqdn, value string) error { - return c.createRecord(ctx, fqdn, value, 60) + return c.updateTXTRecord(ctx, fqdn, func(set *dns.RecordSet) { + var found bool + for _, r := range set.Properties.TxtRecords { + if len(r.Value) > 0 && *r.Value[0] == value { + found = true + break + } + } + + if !found { + set.Properties.TxtRecords = append(set.Properties.TxtRecords, &dns.TxtRecord{ + Value: []*string{ptr.String(value)}, + }) + } + }) } // CleanUp removes the TXT record matching the specified parameters func (c *DNSProvider) CleanUp(ctx context.Context, domain, fqdn, value string) error { - z, err := c.getHostedZoneName(ctx, fqdn) - if err != nil { - c.log.Error(err, "Error getting hosted zone name for fqdn", "fqdn", fqdn) - return err - } + return c.updateTXTRecord(ctx, fqdn, func(set *dns.RecordSet) { + var records []*dns.TxtRecord + for _, r := range set.Properties.TxtRecords { + if len(r.Value) > 0 && *r.Value[0] != value { + records = append(records, r) + } + } - _, err = c.recordClient.Delete( - ctx, - c.resourceGroupName, - z, - c.trimFqdn(fqdn, z), - dns.RecordTypeTXT, nil) - if err != nil { - c.log.Error(err, "Error deleting TXT", "zone", z, "domain", fqdn, "resource group", c.resourceGroupName) - return stabilizeError(err) - } - return nil -} - -func (c *DNSProvider) createRecord(ctx context.Context, fqdn, value string, ttl int) error { - rparams := &dns.RecordSet{ - Properties: &dns.RecordSetProperties{ - TTL: to.Ptr(int64(ttl)), - TxtRecords: []*dns.TxtRecord{ - {Value: []*string{&value}}, - }, - }, - } - - z, err := c.getHostedZoneName(ctx, fqdn) - if err != nil { - return err - } - - _, err = c.recordClient.CreateOrUpdate( - ctx, - c.resourceGroupName, - z, - c.trimFqdn(fqdn, z), - dns.RecordTypeTXT, - *rparams, nil) - if err != nil { - c.log.Error(err, "Error creating TXT", "zone", z, "domain", fqdn, "resource group", c.resourceGroupName) - return stabilizeError(err) - } - return nil + set.Properties.TxtRecords = records + }) } func (c *DNSProvider) getHostedZoneName(ctx context.Context, fqdn string) (string, error) { @@ -220,47 +199,18 @@ func (c *DNSProvider) trimFqdn(fqdn string, zone string) string { return strings.TrimSuffix(strings.TrimSuffix(fqdn, "."), "."+z) } -// The azure-sdk library returns the contents of the HTTP requests in its -// error messages. We want our error messages to be the same when the cause -// is the same to avoid spurious challenge updates. -// -// The given error must not be nil. This function must be called everywhere -// we have a non-nil error coming from a azure-sdk func that makes API calls. -func stabilizeError(err error) error { - if err == nil { - return nil - } - - redactResponse := func(resp *http.Response) *http.Response { - if resp == nil { - return nil - } - - response := *resp - response.Body = io.NopCloser(bytes.NewReader([]byte(""))) - return &response - } - - var authErr *azidentity.AuthenticationFailedError - if errors.As(err, &authErr) { - //nolint: bodyclose // False positive, this already a processed body, probably just pointing to a buffer. - authErr.RawResponse = redactResponse(authErr.RawResponse) - } - - var respErr *azcore.ResponseError - if errors.As(err, &respErr) { - //nolint: bodyclose // False positive, this already a processed body, probably just pointing to a buffer. - respErr.RawResponse = redactResponse(respErr.RawResponse) +// Updates or removes DNS TXT record while respecting optimistic concurrency control +func (c *DNSProvider) updateTXTRecord(ctx context.Context, fqdn string, updater func(*dns.RecordSet)) error { + zone, err := c.getHostedZoneName(ctx, fqdn) + if err != nil { + return err } - return err -} + name := c.trimFqdn(fqdn, zone) -// Updates or removes DNS TXT record while respecting optimistic concurrency control -func (c *DNSProvider) updateTXTRecord(zone, name string, updater func(*dns.RecordSet)) error { var set *dns.RecordSet - resp, err := c.recordClient.Get(context.TODO(), c.resourceGroupName, zone, name, dns.RecordTypeTXT, nil) + resp, err := c.recordClient.Get(ctx, c.resourceGroupName, zone, name, dns.RecordTypeTXT, nil) if err != nil { var respErr *azcore.ResponseError if errors.As(err, &respErr); respErr.StatusCode == 404 { @@ -272,7 +222,8 @@ func (c *DNSProvider) updateTXTRecord(zone, name string, updater func(*dns.Recor Etag: to.Ptr(""), } } else { - return fmt.Errorf("cannot get DNS record set: %w", err) + c.log.Error(err, "Error reading TXT", "zone", zone, "domain", fqdn, "resource group", c.resourceGroupName) + return stabilizeError(err) } } else { set = &resp.RecordSet @@ -283,9 +234,10 @@ func (c *DNSProvider) updateTXTRecord(zone, name string, updater func(*dns.Recor if len(set.Properties.TxtRecords) == 0 { if *set.Etag != "" { // Etag will cause the deletion to fail if any updates happen concurrently - _, err = c.recordClient.Delete(context.TODO(), c.resourceGroupName, zone, name, dns.RecordTypeTXT, &dns.RecordSetsClientDeleteOptions{IfMatch: set.Etag}) + _, err = c.recordClient.Delete(ctx, c.resourceGroupName, zone, name, dns.RecordTypeTXT, &dns.RecordSetsClientDeleteOptions{IfMatch: set.Etag}) if err != nil { - return fmt.Errorf("cannot delete DNS record set: %w", err) + c.log.Error(err, "Error deleting TXT", "zone", zone, "domain", fqdn, "resource group", c.resourceGroupName) + return stabilizeError(err) } } @@ -302,7 +254,7 @@ func (c *DNSProvider) updateTXTRecord(zone, name string, updater func(*dns.Recor } _, err = c.recordClient.CreateOrUpdate( - context.TODO(), + ctx, c.resourceGroupName, zone, name, @@ -310,8 +262,45 @@ func (c *DNSProvider) updateTXTRecord(zone, name string, updater func(*dns.Recor *set, opts) if err != nil { - return fmt.Errorf("cannot upsert DNS record set: %w", err) + c.log.Error(err, "Error upserting TXT", "zone", zone, "domain", fqdn, "resource group", c.resourceGroupName) + return stabilizeError(err) } return nil } + +// The azure-sdk library returns the contents of the HTTP requests in its +// error messages. We want our error messages to be the same when the cause +// is the same to avoid spurious challenge updates. +// +// The given error must not be nil. This function must be called everywhere +// we have a non-nil error coming from a azure-sdk func that makes API calls. +func stabilizeError(err error) error { + if err == nil { + return nil + } + + redactResponse := func(resp *http.Response) *http.Response { + if resp == nil { + return nil + } + + response := *resp + response.Body = io.NopCloser(bytes.NewReader([]byte(""))) + return &response + } + + var authErr *azidentity.AuthenticationFailedError + if errors.As(err, &authErr) { + //nolint: bodyclose // False positive, this already a processed body, probably just pointing to a buffer. + authErr.RawResponse = redactResponse(authErr.RawResponse) + } + + var respErr *azcore.ResponseError + if errors.As(err, &respErr) { + //nolint: bodyclose // False positive, this already a processed body, probably just pointing to a buffer. + respErr.RawResponse = redactResponse(respErr.RawResponse) + } + + return err +} diff --git a/pkg/issuer/acme/dns/azuredns/azuredns_test.go b/pkg/issuer/acme/dns/azuredns/azuredns_test.go index c8d9f85cb6a..0cd7c0c31b2 100644 --- a/pkg/issuer/acme/dns/azuredns/azuredns_test.go +++ b/pkg/issuer/acme/dns/azuredns/azuredns_test.go @@ -76,9 +76,9 @@ func TestLiveAzureDnsPresentMultiple(t *testing.T) { provider, err := NewDNSProviderCredentials("", azureClientID, azureClientSecret, azuresubscriptionID, azureTenantID, azureResourceGroupName, azureHostedZoneName, util.RecursiveNameservers, false, &v1.AzureManagedIdentity{}) assert.NoError(t, err) - err = provider.Present(azureDomain, "_acme-challenge."+azureDomain+".", "123d==") + err = provider.Present(context.TODO(), azureDomain, "_acme-challenge."+azureDomain+".", "123d==") assert.NoError(t, err) - err = provider.Present(azureDomain, "_acme-challenge."+azureDomain+".", "1123d==") + err = provider.Present(context.TODO(), azureDomain, "_acme-challenge."+azureDomain+".", "1123d==") assert.NoError(t, err) } @@ -106,9 +106,9 @@ func TestLiveAzureDnsCleanUpMultiple(t *testing.T) { provider, err := NewDNSProviderCredentials("", azureClientID, azureClientSecret, azuresubscriptionID, azureTenantID, azureResourceGroupName, azureHostedZoneName, util.RecursiveNameservers, false, &v1.AzureManagedIdentity{}) assert.NoError(t, err) - err = provider.CleanUp(azureDomain, "_acme-challenge."+azureDomain+".", "123d==") + err = provider.CleanUp(context.TODO(), azureDomain, "_acme-challenge."+azureDomain+".", "123d==") assert.NoError(t, err) - err = provider.CleanUp(azureDomain, "_acme-challenge."+azureDomain+".", "1123d==") + err = provider.CleanUp(context.TODO(), azureDomain, "_acme-challenge."+azureDomain+".", "1123d==") assert.NoError(t, err) } From c180fefc9c14e5a837ae44740203a9c37be6c15d Mon Sep 17 00:00:00 2001 From: Bartosz Slawianowski Date: Fri, 10 May 2024 11:08:43 +0200 Subject: [PATCH 4/5] Remove unnecessary AWS SDK dependency Signed-off-by: Bartosz Slawianowski --- pkg/issuer/acme/dns/azuredns/azuredns.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/issuer/acme/dns/azuredns/azuredns.go b/pkg/issuer/acme/dns/azuredns/azuredns.go index f7a1b3a949a..0b6181ec5a4 100644 --- a/pkg/issuer/acme/dns/azuredns/azuredns.go +++ b/pkg/issuer/acme/dns/azuredns/azuredns.go @@ -27,7 +27,6 @@ import ( "github.com/Azure/azure-sdk-for-go/sdk/azcore/to" "github.com/Azure/azure-sdk-for-go/sdk/azidentity" dns "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/dns/armdns" - "github.com/aws/smithy-go/ptr" "github.com/go-logr/logr" cmacme "github.com/cert-manager/cert-manager/pkg/apis/acme/v1" @@ -150,7 +149,7 @@ func (c *DNSProvider) Present(ctx context.Context, domain, fqdn, value string) e if !found { set.Properties.TxtRecords = append(set.Properties.TxtRecords, &dns.TxtRecord{ - Value: []*string{ptr.String(value)}, + Value: []*string{to.Ptr(value)}, }) } }) From 0f6eaa9ab818a29f9dff374bc08285189934c631 Mon Sep 17 00:00:00 2001 From: Bartosz Slawianowski Date: Fri, 10 May 2024 11:28:01 +0200 Subject: [PATCH 5/5] Fix lint Signed-off-by: Bartosz Slawianowski --- pkg/issuer/acme/dns/azuredns/azuredns.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/issuer/acme/dns/azuredns/azuredns.go b/pkg/issuer/acme/dns/azuredns/azuredns.go index 0b6181ec5a4..27994c73be5 100644 --- a/pkg/issuer/acme/dns/azuredns/azuredns.go +++ b/pkg/issuer/acme/dns/azuredns/azuredns.go @@ -212,7 +212,7 @@ func (c *DNSProvider) updateTXTRecord(ctx context.Context, fqdn string, updater resp, err := c.recordClient.Get(ctx, c.resourceGroupName, zone, name, dns.RecordTypeTXT, nil) if err != nil { var respErr *azcore.ResponseError - if errors.As(err, &respErr); respErr.StatusCode == 404 { + if errors.As(err, &respErr); respErr.StatusCode == http.StatusNotFound { set = &dns.RecordSet{ Properties: &dns.RecordSetProperties{ TTL: to.Ptr(int64(60)),