Translate AWS Rate limiting errors to 502 errors #5270

catsby · 2018-09-04T20:55:22Z

Builds off of and includes #5216

When interacting with AWS, if we hit any throttling errors return those to the users as a 502 "Bad Gateway" error code.

Supports:

Secret backend
Auth backend

builtin/logical/aws/path_user.go

catsby · 2018-09-05T22:00:22Z

helper/awsutil/error.go

+
+// CheckAWSError will examine an error and convert to a logical error if
+// approrpiate. If no appropriate error is found, return nil
+func CheckAWSError(err error) error {


Suggestions for better names for this function as well as AppendLogicalError are most welcome

A couple of comments on this file:

Do you need have CheckAWSError as a separate, exported function? The functionality seems like it could just be part of AppendLogicalError.

Is multierror the best response pattern for AppendLogicalError, or would errwrap be more appropriate? I view the logical error as adding context to the underlying AWS error (very much an errwrap concept), vs. multierror which is often used to accumulate various errors together

Depending on what you think of ^^^, maybe a single WrapAWSError would work.

Do you need have CheckAWSError as a separate, exported function?

CheckAWSError is for transforming an upstream error into a different error of our choosing. In this case (and only case I'm aware of), it's "AWS gave us 503 (or other throttle error) so we change that to a BadGateway). In short "do we want to change this error to something else?" If yes, that something gets returned. If no, then return nothing. Another way of doing it would be to return (error, bool), with bool indicating if a change was made. If false, then the error returned is the same as the function received.

The need for a separate method is shown in

vault/builtin/logical/aws/secret_access_keys.go

Line 218 in 7b00112

"Error creating IAM user: %s", err)), awsutil.CheckAWSError(err)

, where we're already returning a error response, and we're using the returned error here to indicate a category of error. That error is later checked in response_util to control how Vault responds.

AppendLogicalError came up because in in auth/aws/backend we weren't using logical.ErrorResponse, so the original error wasn't going to be returned if I just used CheckAWSError. So, I wanted to return both errors, the original and the logical one, if necessary.

My first approach used errwrap however, either it's not suited or I did it wrong, because I wasn't able to get errwrap.Contains to find the logical.ErrUpstreamRateLimited when wrapping the errors. Using multierror did work, and is compatible with errwrap and honestly made more sense to me than errwrap.

kalafut · 2018-09-05T22:25:42Z

builtin/credential/aws/path_login.go

@@ -168,7 +169,8 @@ func (b *backend) validateInstance(ctx context.Context, s logical.Storage, insta
 		},
 	})
 	if err != nil {
-		return nil, errwrap.Wrapf(fmt.Sprintf("error fetching description for instance ID %q: {{err}}", instanceID), err)
+		errW := errwrap.Wrapf(fmt.Sprintf("error fetching description for instance ID %q: {{err}}", instanceID), err)
+		return nil, awsutil.AppendLogicalError(errW)


After err is wrapped, won't awsRequest.IsErrorThrottle() be unable to detect that it is a throttling error?

Great catch, thanks. I modified this in b01aaad

kalafut · 2018-09-05T22:32:48Z

builtin/logical/aws/secret_access_keys.go

-	if err != nil {
-		return logical.ErrorResponse(err.Error()), nil
+
+	b.clientMutex.Lock()


Consider making clientMutex an RWLock that can be upgraded from RLock to Lock if client is nil. This avoids synchronizing all of the read requests. You can see an example upgrade pattern (which shows up in various place in Vault) here: https://github.com/hashicorp/vault-plugin-secrets-azure/blob/master/client.go#L37

(Comment applies to the other uses of this lock too.)

kalafut · 2018-09-05T22:36:27Z

builtin/logical/aws/secret_access_keys.go

-	STSClient, err := clientSTS(ctx, s)
-	if err != nil {
-		return logical.ErrorResponse(err.Error()), nil
+	if b.stsClient == nil {


Guard with the lock?

kalafut · 2018-09-05T22:39:15Z

helper/awsutil/error.go

+)
+
+// CheckAWSError will examine an error and convert to a logical error if
+// approrpiate. If no appropriate error is found, return nil


s/approrpiate/appropriate

kalafut · 2018-09-05T22:39:59Z

logical/error.go

@@ -20,6 +20,10 @@ var (
 	// ErrMultiAuthzPending is returned if the the request needs more
 	// authorizations
 	ErrMultiAuthzPending = errors.New("request needs further approval")
+
+	// ErrUpstreamRateLimited is returned when Vault recieves a rate limited


s/recieves/receives

kalafut · 2018-09-06T18:02:53Z

You'll want to run make fmt with go 1.11 installed.

helper/awsutil/error_test.go

- bump aws iam and sts packages to v1.14.31 to get mocking interface - promote the iam and sts clients to the aws backend struct, for mocking in tests - this also promotes some functions to methods on the Backend struct, so that we can use the injected client Generating creds requires reading config/root for credentials to contact IAM. Here we make pathConfigRoot a method on aws/backend so we can clear the clients on successful update of config/root path. Adds a mutex to safely clear the clients

vishalnayak · 2018-09-10T22:13:44Z

builtin/logical/aws/secret_access_keys.go

-	if err != nil {
-		return logical.ErrorResponse(err.Error()), nil
+
+	b.clientMutex.RLock()


May it be a good idea to abstract out the nil checking and client setting in the backend struct? I see that all the functions that use either b.iamClient or b.stsClient should check those for nil and perform the logic of setting it in the backend. This seems easy to get wrong, or simply forget.

How about we push this logic in a function, say b.IAMClient() and b.STSClient() which return the b.iamClient and b.stsClient respectively if populated. If not, it can then do the locking to set the client and return the client. The calling functions will then have to use the returned client as opposed to b.[iam|sts]Client. That way calling functions can be sure that the client is a non-nil value.

Seems reasonable, I made the changes in 3c74986 please take a look

builtin/logical/aws/backend.go

jefferai

Just the one comment, LGTM!

catsby requested a review from chrishoffman September 4, 2018 20:55

joelthompson reviewed Sep 4, 2018

View reviewed changes

builtin/logical/aws/path_user.go Outdated Show resolved Hide resolved

catsby commented Sep 5, 2018

View reviewed changes

kalafut reviewed Sep 5, 2018

View reviewed changes

kalafut reviewed Sep 6, 2018

View reviewed changes

helper/awsutil/error_test.go Show resolved Hide resolved

catsby force-pushed the aws-upstream-ratelimiting branch from 649010b to a3e59aa Compare September 7, 2018 15:38

catsby changed the title ~~[WIP] Translate AWS Rate limiting errors to 502 errors~~ Translate AWS Rate limiting errors to 502 errors Sep 10, 2018

vishalnayak reviewed Sep 10, 2018

View reviewed changes

catsby added 2 commits September 11, 2018 16:38

refactor locking and unlocking into methods on *backend

3c74986

refactor/simply the locking

cf9e95c

vishalnayak previously approved these changes Sep 13, 2018

View reviewed changes

vishalnayak requested a review from jefferai September 17, 2018 14:49

vishalnayak added this to the 0.11.2 milestone Sep 17, 2018

jefferai reviewed Sep 18, 2018

View reviewed changes

builtin/logical/aws/backend.go Outdated Show resolved Hide resolved

jefferai previously approved these changes Sep 18, 2018

View reviewed changes

check client after grabbing lock

3d468da

catsby dismissed stale reviews from jefferai and vishalnayak via 3d468da September 18, 2018 16:46

catsby merged commit f39bafb into master Sep 18, 2018

catsby deleted the aws-upstream-ratelimiting branch September 18, 2018 20:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Translate AWS Rate limiting errors to 502 errors #5270

Translate AWS Rate limiting errors to 502 errors #5270

catsby commented Sep 4, 2018 •

edited

Loading

catsby Sep 5, 2018

kalafut Sep 5, 2018

catsby Sep 6, 2018

kalafut Sep 5, 2018 •

edited

Loading

catsby Sep 6, 2018

kalafut Sep 5, 2018 •

edited

Loading

kalafut Sep 5, 2018

kalafut Sep 5, 2018

kalafut Sep 5, 2018

kalafut commented Sep 6, 2018

vishalnayak Sep 10, 2018 •

edited

Loading

catsby Sep 11, 2018

jefferai left a comment

Translate AWS Rate limiting errors to 502 errors #5270

Translate AWS Rate limiting errors to 502 errors #5270

Conversation

catsby commented Sep 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kalafut Sep 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kalafut Sep 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kalafut commented Sep 6, 2018

vishalnayak Sep 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jefferai left a comment

Choose a reason for hiding this comment

catsby commented Sep 4, 2018 •

edited

Loading

kalafut Sep 5, 2018 •

edited

Loading

kalafut Sep 5, 2018 •

edited

Loading

vishalnayak Sep 10, 2018 •

edited

Loading