From 90cbbc9d879874f5c9187440675548e50992dcf9 Mon Sep 17 00:00:00 2001 From: Tim Ramlot <42113979+inteon@users.noreply.github.com> Date: Tue, 30 Jan 2024 16:20:52 +0100 Subject: [PATCH] replace the azcore.ResponseError error message to make it stable across retries Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com> --- pkg/issuer/acme/dns/azuredns/azuredns.go | 51 +++++++++++++++++-- pkg/issuer/acme/dns/azuredns/azuredns_test.go | 50 ++++++++++++++++++ 2 files changed, 98 insertions(+), 3 deletions(-) diff --git a/pkg/issuer/acme/dns/azuredns/azuredns.go b/pkg/issuer/acme/dns/azuredns/azuredns.go index e07ce67b3..971898a97 100644 --- a/pkg/issuer/acme/dns/azuredns/azuredns.go +++ b/pkg/issuer/acme/dns/azuredns/azuredns.go @@ -12,6 +12,7 @@ package azuredns import ( "context" + "errors" "fmt" "os" "strings" @@ -154,7 +155,7 @@ func (c *DNSProvider) CleanUp(domain, fqdn, value string) error { c.trimFqdn(fqdn, z), dns.RecordTypeTXT, nil) if err != nil { - return err + return stabilizeError(err) } return nil } @@ -184,7 +185,7 @@ func (c *DNSProvider) createRecord(fqdn, value string, ttl int) error { *rparams, nil) if err != nil { c.log.Error(err, "Error creating TXT:", z) - return err + return stabilizeError(err) } return nil } @@ -205,7 +206,7 @@ func (c *DNSProvider) getHostedZoneName(fqdn string) (string, error) { _, 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) + 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 +220,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 := "" + url := "" + 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 := "" + 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 +} diff --git a/pkg/issuer/acme/dns/azuredns/azuredns_test.go b/pkg/issuer/acme/dns/azuredns/azuredns_test.go index ed33fd991..bb3d2b09e 100644 --- a/pkg/issuer/acme/dns/azuredns/azuredns_test.go +++ b/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)) +}