weiguoqiang/cert-manager
1
0
Fork 0
Code Issues Pull Requests Actions 3 Packages Projects Releases Wiki Activity

Merge pull request #3485 from maelvls/bug-spurious-updates-aws

Browse Source 
Strip X-Amzn-RequestId to avoid spurious challenge updates
This commit is contained in:
jetstack-bot 2020-12-02 15:42:33 +01:00 committed by GitHub
commit fe84c50f7b
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 93 additions and 4 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
pkg/issuer/acme/dns/route53/BUILD.bazel
View File

@ -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",

20
pkg/issuer/acme/dns/route53/fixtures_test.go
View File

@ -40,6 +40,16 @@ var ListHostedZonesByNameResponse = `<?xml version="1.0" encoding="UTF-8"?>
</Config>
<ResourceRecordSetCount>10</ResourceRecordSetCount>
</HostedZone>
<HostedZone>
<Id>/hostedzone/OPQRSTU</Id>
<Name>bar.example.com.</Name>
<CallerReference>D2224C5B-684A-DB4A-BB9A-E09E3BAFEA7A</CallerReference>
<Config>
<Comment>Test comment</Comment>
<PrivateZone>false</PrivateZone>
</Config>
<ResourceRecordSetCount>10</ResourceRecordSetCount>
</HostedZone>
</HostedZones>
<IsTruncated>true</IsTruncated>
<NextDNSName>example2.com</NextDNSName>
@ -55,3 +65,13 @@ var GetChangeResponse = `<?xml version="1.0" encoding="UTF-8"?>
<SubmittedAt>2016-02-10T01:36:41.958Z</SubmittedAt>
</ChangeInfo>
</GetChangeResponse>`
var ChangeResourceRecordSets403Response = `<?xml version="1.0"?>
<ErrorResponse xmlns="https://route53.amazonaws.com/doc/2013-04-01/">
<Error>
<Type>Sender</Type>
<Code>AccessDenied</Code>
<Message>User: arn:aws:iam::0123456789:user/test-cert-manager is not authorized to perform: route53:ChangeResourceRecordSets on resource: arn:aws:route53:::hostedzone/OPQRSTU</Message>
</Error>
<RequestId>SOMEREQUESTID</RequestId>
</ErrorResponse>`

27
pkg/issuer/acme/dns/route53/route53.go
View File

@ -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
}

46
pkg/issuer/acme/dns/route53/route53_test.go
View File

@ -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)
})
}
}

3
pkg/issuer/acme/dns/route53/testutil_test.go
View File

@ -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)