Merge pull request #6676 from inteon/azure_redact

Replace the azcore.ResponseError error message to make it stable across retries

pkg/issuer/acme/dns/azuredns/azuredns.go

@@ -12,6 +12,7 @@ package azuredns
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"os"
 	"strings"
@@ -154,7 +155,8 @@ func (c *DNSProvider) CleanUp(domain, fqdn, value string) error {
 		c.trimFqdn(fqdn, z),
 		dns.RecordTypeTXT, nil)
 	if err != nil {
-		return err
+		c.log.Error(err, "Error deleting TXT", "zone", z, "domain", fqdn, "resource group", c.resourceGroupName)
+		return stabilizeError(err)
 	}
 	return nil
 }
@@ -171,7 +173,6 @@ func (c *DNSProvider) createRecord(fqdn, value string, ttl int) error {
 
 	z, err := c.getHostedZoneName(fqdn)
 	if err != nil {
-		c.log.Error(err, "Error getting hosted zone name for:", fqdn)
 		return err
 	}
 
@@ -183,8 +184,8 @@ func (c *DNSProvider) createRecord(fqdn, value string, ttl int) error {
 		dns.RecordTypeTXT,
 		*rparams, nil)
 	if err != nil {
-		c.log.Error(err, "Error creating TXT:", z)
-		return err
+		c.log.Error(err, "Error creating TXT", "zone", z, "domain", fqdn, "resource group", c.resourceGroupName)
+		return stabilizeError(err)
 	}
 	return nil
 }
@@ -197,15 +198,13 @@ func (c *DNSProvider) getHostedZoneName(fqdn string) (string, error) {
 	if err != nil {
 		return "", err
 	}
-
 	if len(z) == 0 {
 		return "", fmt.Errorf("Zone %s not found for domain %s", z, fqdn)
 	}
 
-	_, err = c.zoneClient.Get(context.TODO(), c.resourceGroupName, util.UnFqdn(z), nil)
-
-	if err != nil {
-		return "", fmt.Errorf("Zone %s not found in AzureDNS for domain %s. Err: %v", z, fqdn, err)
+	if _, err := c.zoneClient.Get(context.TODO(), c.resourceGroupName, util.UnFqdn(z), nil); err != nil {
+		c.log.Error(err, "Error getting Zone for domain", "zone", z, "domain", fqdn, "resource group", c.resourceGroupName)
+		return "", fmt.Errorf("Zone %s not found in AzureDNS for domain %s. Err: %v", z, fqdn, stabilizeError(err))
 	}
 
 	return util.UnFqdn(z), nil
@@ -219,3 +218,47 @@ 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
+	}
+
+	var respErr *azcore.ResponseError
+	if errors.As(err, &respErr) {
+		method := "<UNKNOWN-METHOD>"
+		url := "<UNKNOWN-URL>"
+		response := "<UNKNOWN-RESPONSE>"
+		if respErr.RawResponse.Request != nil {
+			method = respErr.RawResponse.Request.Method
+			url = fmt.Sprintf(
+				"%s://%s%s",
+				respErr.RawResponse.Request.URL.Scheme,
+				respErr.RawResponse.Request.URL.Host,
+				respErr.RawResponse.Request.URL.Path,
+			)
+			response = fmt.Sprintf("%d: %s", respErr.RawResponse.StatusCode, respErr.RawResponse.Status)
+		}
+
+		errorCode := "<UNKNOWN-ERROR-CODE>"
+		if respErr.ErrorCode != "" {
+			errorCode = respErr.ErrorCode
+		}
+
+		return fmt.Errorf(
+			"Error making AzureDNS API request:\n%s %s\nRESPONSE %s\nERROR CODE: %s",
+			method,
+			url,
+			response,
+			errorCode,
+		)
+	}
+
+	return err
+}

pkg/issuer/acme/dns/azuredns/azuredns_test.go

@@ -11,6 +11,7 @@ package azuredns
 import (
 	"context"
 	"encoding/json"
+	"fmt"
 	"io"
 	"net/http"
 	"net/http/httptest"
@@ -20,12 +21,16 @@ import (
 	"testing"
 	"time"
 
+	"github.com/Azure/azure-sdk-for-go/sdk/azcore/arm"
 	"github.com/Azure/azure-sdk-for-go/sdk/azcore/cloud"
 	"github.com/Azure/azure-sdk-for-go/sdk/azcore/policy"
 	"github.com/Azure/azure-sdk-for-go/sdk/azidentity"
+	dns "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/dns/armdns"
 	v1 "github.com/cert-manager/cert-manager/pkg/apis/acme/v1"
 	"github.com/cert-manager/cert-manager/pkg/issuer/acme/dns/util"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"k8s.io/apimachinery/pkg/util/rand"
 )
 
 var (
@@ -264,3 +269,48 @@ func TestGetAuthorizationFederatedSPT(t *testing.T) {
 		assert.NotEmpty(t, token.Token, "Access token should have been set to a value returned by the webserver")
 	})
 }
+
+// TestStabilizeError tests that the errors returned by the AzureDNS API are
+// changed to be stable. We want our error messages to be the same when the cause
+// is the same to avoid spurious challenge updates.
+func TestStabilizeError(t *testing.T) {
+	ts := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusBadGateway)
+		randomMessage := "test error message: " + rand.String(10)
+		payload := fmt.Sprintf(`{"error":{"code":"TEST_ERROR_CODE","message":"%s"}}`, randomMessage)
+		if _, err := w.Write([]byte(payload)); err != nil {
+			assert.FailNow(t, err.Error())
+		}
+	}))
+
+	defer ts.Close()
+
+	clientOpt := policy.ClientOptions{
+		Cloud: cloud.Configuration{
+			ActiveDirectoryAuthorityHost: ts.URL,
+			Services: map[cloud.ServiceName]cloud.ServiceConfiguration{
+				cloud.ResourceManager: {
+					Audience: ts.URL,
+					Endpoint: ts.URL,
+				},
+			},
+		},
+		Transport: ts.Client(),
+	}
+
+	zc, err := dns.NewZonesClient("subscriptionID", nil, &arm.ClientOptions{ClientOptions: clientOpt})
+	require.NoError(t, err)
+
+	dnsProvider := DNSProvider{
+		dns01Nameservers:  util.RecursiveNameservers,
+		resourceGroupName: "resourceGroupName",
+		zoneClient:        zc,
+	}
+
+	err = dnsProvider.Present("test.com", "fqdn.test.com.", "test123")
+	require.Error(t, err)
+	require.ErrorContains(t, err, fmt.Sprintf(`Zone test.com. not found in AzureDNS for domain fqdn.test.com.. Err: Error making AzureDNS API request:
+GET %s/subscriptions/subscriptionID/resourceGroups/resourceGroupName/providers/Microsoft.Network/dnsZones/test.com
+RESPONSE 502: 502 Bad Gateway
+ERROR CODE: TEST_ERROR_CODE`, ts.URL))
+}