diff --git a/pkg/issuer/acme/dns/route53/BUILD.bazel b/pkg/issuer/acme/dns/route53/BUILD.bazel index f1dacfdb4..f4586ca29 100644 --- a/pkg/issuer/acme/dns/route53/BUILD.bazel +++ b/pkg/issuer/acme/dns/route53/BUILD.bazel @@ -33,6 +33,7 @@ go_test( "//pkg/issuer/acme/dns/util:go_default_library", "//pkg/logs:go_default_library", "@com_github_aws_aws_sdk_go//aws:go_default_library", + "@com_github_aws_aws_sdk_go//aws/awserr:go_default_library", "@com_github_aws_aws_sdk_go//aws/credentials:go_default_library", "@com_github_aws_aws_sdk_go//aws/session:go_default_library", "@com_github_aws_aws_sdk_go//service/route53:go_default_library", diff --git a/pkg/issuer/acme/dns/route53/fixtures_test.go b/pkg/issuer/acme/dns/route53/fixtures_test.go index 236122cda..248f81cbf 100644 --- a/pkg/issuer/acme/dns/route53/fixtures_test.go +++ b/pkg/issuer/acme/dns/route53/fixtures_test.go @@ -40,6 +40,16 @@ var ListHostedZonesByNameResponse = ` 10 + + /hostedzone/OPQRSTU + bar.example.com. + D2224C5B-684A-DB4A-BB9A-E09E3BAFEA7A + + Test comment + false + + 10 + true example2.com @@ -55,3 +65,13 @@ var GetChangeResponse = ` 2016-02-10T01:36:41.958Z ` + +var ChangeResourceRecordSets403Response = ` + + + Sender + AccessDenied + User: arn:aws:iam::0123456789:user/test-cert-manager is not authorized to perform: route53:ChangeResourceRecordSets on resource: arn:aws:route53:::hostedzone/OPQRSTU + + SOMEREQUESTID +` diff --git a/pkg/issuer/acme/dns/route53/route53.go b/pkg/issuer/acme/dns/route53/route53.go index 3d002e521..dd1e07415 100644 --- a/pkg/issuer/acme/dns/route53/route53.go +++ b/pkg/issuer/acme/dns/route53/route53.go @@ -205,7 +205,7 @@ func (r *DNSProvider) changeRecord(action, fqdn, value string, ttl int) error { return nil } } - return fmt.Errorf("Failed to change Route 53 record set: %v", err) + return fmt.Errorf("Failed to change Route 53 record set: %v", removeReqID(err)) } @@ -217,7 +217,7 @@ func (r *DNSProvider) changeRecord(action, fqdn, value string, ttl int) error { } resp, err := r.client.GetChange(reqParams) if err != nil { - return false, fmt.Errorf("Failed to query Route 53 change status: %v", err) + return false, fmt.Errorf("Failed to query Route 53 change status: %v", removeReqID(err)) } if *resp.ChangeInfo.Status == route53.ChangeStatusInsync { return true, nil @@ -242,7 +242,7 @@ func (r *DNSProvider) getHostedZoneID(fqdn string) (string, error) { } resp, err := r.client.ListHostedZonesByName(reqParams) if err != nil { - return "", err + return "", removeReqID(err) } zoneToID := make(map[string]string) @@ -282,3 +282,24 @@ func newTXTRecordSet(fqdn, value string, ttl int) *route53.ResourceRecordSet { }, } } + +// The aws-sdk-go library appends a request id to 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 an aws-sdk-go func. +func removeReqID(err error) error { + // NOTE(mael): I first tried to unwrap the RequestFailure to get rid of + // this request id. But the concrete type requestFailure is private, so + // I can't unwrap it. Instead, I recreate a new awserr.baseError. It's + // also a awserr.Error except it doesn't have the request id. + // + // Also note that we do not give the origErr to awserr.New. If we did, + // err.Error() would show the origErr, which we don't want since it + // contains a request id. + if e, ok := err.(awserr.RequestFailure); ok { + return awserr.New(e.Code(), e.Message(), nil) + } + return err +} diff --git a/pkg/issuer/acme/dns/route53/route53_test.go b/pkg/issuer/acme/dns/route53/route53_test.go index e3e69c8d1..b73d02140 100644 --- a/pkg/issuer/acme/dns/route53/route53_test.go +++ b/pkg/issuer/acme/dns/route53/route53_test.go @@ -9,6 +9,7 @@ this directory. package route53 import ( + "errors" "fmt" "net/http/httptest" "os" @@ -17,12 +18,14 @@ import ( logf "github.com/jetstack/cert-manager/pkg/logs" "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/aws/awserr" "github.com/aws/aws-sdk-go/aws/credentials" "github.com/aws/aws-sdk-go/aws/session" "github.com/aws/aws-sdk-go/service/route53" "github.com/aws/aws-sdk-go/service/sts" "github.com/aws/aws-sdk-go/service/sts/stsiface" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/jetstack/cert-manager/pkg/issuer/acme/dns/util" ) @@ -111,6 +114,7 @@ func TestRoute53Present(t *testing.T) { "/2013-04-01/hostedzone/ABCDEFG/rrset/": MockResponse{StatusCode: 200, Body: ChangeResourceRecordSetsResponse}, "/2013-04-01/hostedzone/HIJKLMN/rrset/": MockResponse{StatusCode: 200, Body: ChangeResourceRecordSetsResponse}, "/2013-04-01/change/123456": MockResponse{StatusCode: 200, Body: GetChangeResponse}, + "/2013-04-01/hostedzone/OPQRSTU/rrset/": MockResponse{StatusCode: 403, Body: ChangeResourceRecordSets403Response}, } ts := newMockServer(t, mockResponses) @@ -136,6 +140,13 @@ func TestRoute53Present(t *testing.T) { nonExistentDomain := "baz.com" err = provider.Present(nonExistentDomain, nonExistentDomain+".", keyAuth) assert.Error(t, err, "Expected Present to return an error") + + // This test case makes sure that the request id has been properly + // stripped off. It has to be stripped because it changes on every + // request which causes spurious challenge updates. + err = provider.Present("bar.example.com", "bar.example.com.", keyAuth) + require.Error(t, err, "Expected Present to return an error") + assert.Equal(t, `Failed to change Route 53 record set: AccessDenied: User: arn:aws:iam::0123456789:user/test-cert-manager is not authorized to perform: route53:ChangeResourceRecordSets on resource: arn:aws:route53:::hostedzone/OPQRSTU`, err.Error()) } func TestAssumeRole(t *testing.T) { @@ -275,3 +286,38 @@ func makeMockSessionProvider(defaultSTSProvider func(sess *session.Session) stsi log: logf.Log.WithName("route53-session"), }, nil } + +func Test_removeReqID(t *testing.T) { + tests := []struct { + name string + err error + wantErr error + }{ + { + name: "should remove the request id and the origin error", + err: awserr.NewRequestFailure(awserr.New("foo", "bar", nil), 400, "SOMEREQUESTID"), + wantErr: awserr.New("foo", "bar", nil), + }, + { + name: "should do nothing if no request id is set", + err: awserr.New("foo", "bar", nil), + wantErr: awserr.New("foo", "bar", nil), + }, + { + name: "should do nothing if the error is not an aws error", + err: errors.New("foo"), + wantErr: errors.New("foo"), + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := removeReqID(tt.err) + if tt.wantErr != nil { + require.Error(t, err) + assert.Equal(t, tt.wantErr.Error(), err.Error()) + return + } + require.NoError(t, err) + }) + } +} diff --git a/pkg/issuer/acme/dns/route53/testutil_test.go b/pkg/issuer/acme/dns/route53/testutil_test.go index 57c84eab7..eacdc2a38 100644 --- a/pkg/issuer/acme/dns/route53/testutil_test.go +++ b/pkg/issuer/acme/dns/route53/testutil_test.go @@ -37,8 +37,9 @@ func newMockServer(t *testing.T, responses MockResponseMap) *httptest.Server { } w.Header().Set("Content-Type", "application/xml") + w.Header().Set("X-Amzn-Requestid", "SOMEREQUESTID") w.WriteHeader(resp.StatusCode) - w.Write([]byte(resp.Body)) + _, _ = w.Write([]byte(resp.Body)) })) time.Sleep(100 * time.Millisecond)