Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide Smithy models for default API Gateway errors #1170

Open
eduardomourar opened this issue Apr 3, 2022 · 5 comments
Open

Provide Smithy models for default API Gateway errors #1170

eduardomourar opened this issue Apr 3, 2022 · 5 comments
Labels
OpenAPI protocol-test New protocol tests are needed

Comments

@eduardomourar
Copy link
Contributor

eduardomourar commented Apr 3, 2022

Whenever we deploy our services using API Gateway from AWS, there are errors that we have no control over (as described here). The details of those errors are needed in order to allow the proper deserialization within the client SDKs. We have tried to solve this with two different approaches, although without success:

  1. By defining the model for them ourselves, but we have not been able to find what are the possible X-Amzn-ErrorType HTTP headers that are being sent by API Gateway. The closest possibility can apparently be found here.
  2. By overriding the X-Amzn-ErrorType HTTP header using the OpenAPI extension x-amazon-apigateway-gateway-responses. That caused the header to be sent two times containing our override value BadRequestException and the default value IncompleteSignatureException from API Gateway (as shown in the attached image). The client SDK does not seem to like that, and simply ignore our value.

Screenshot 2022-04-03 at 19 52 12

Is is possible for AWS to provide the Smithy models only of those default API Gateway errors (similar to this here), please?

@adamthom-amzn
Copy link
Contributor

The 5xx errors shouldn't need a model - the client should transform them to some sort of generic client exception.

The 4xx errors, particularly around input validation, we should look at a better experience for generating response templates for. The optimal experience for some of them, like bad request body or parameters, would be to remap them to a validation exception type (such as the built-in one). The others we might need to add an additional model for.

@DanielBauman88
Copy link

What about making the generated client look at values in the X-Amzn-ErrorType header to match one of the errors defined on the operation?

If there's only one matching value - deserialize correctly to that exception. This is reasonable and not surprising behaviour and it unblocks customers who are using api gateway and gateway responses to have a functional sdk for these errors.

If there's no match or if there's ambiguity (values for two different defined errors) then continue with the existing behaviour.

The others we might need to add an additional model for.

Could you elaborate on this? Does this imply that smithy itself will define some error for these cases which all customers must use for these cases? Wouldn't it make sense for smithy to let customers define the errors and headers they want for their exceptions (including auth/throttling) without smithy making this choice for them?

DanielBauman88 added a commit to DanielBauman88/aws-sdk-js-v3 that referenced this issue Aug 10, 2022
This change takes the first header value from x-amzn-errortype
if multiple headers are provided.

An example curl output for a response that this works for:
```
HTTP/2 401
date: Tue, 09 Aug 2022 21:24:20 GMT
content-type: application/json
content-length: 92
x-amzn-requestid: xxxx
access-control-allow-origin: *
x-amzn-errortype: UnauthorizedError
x-amzn-errortype: UnauthorizedException
x-amz-apigw-id: xxxx
x-amzn-trace-id: Root=xxxx
```
In this instance this code will select the first header value.

See related - smithy-lang/smithy#1170
@DanielBauman88
Copy link

@adamthom-amzn what do you think of the two options in the PRs?
Do you think one of them is viable for merging?

I think that customizing the header name is a safe and non-invasive change which enables customers to configure their APIs to their needs. It may not be strictly needed in the sdk package, but maybe it could live there until all the useful common logic that lives in the sdk is extracted and smithy clients are more usable without this dep.

kuhe pushed a commit to aws/aws-sdk-js-v3 that referenced this issue Sep 6, 2022
This change takes the first header value from x-amzn-errortype
if multiple headers are provided.

An example curl output for a response that this works for:
```
HTTP/2 401
date: Tue, 09 Aug 2022 21:24:20 GMT
content-type: application/json
content-length: 92
x-amzn-requestid: xxxx
access-control-allow-origin: *
x-amzn-errortype: UnauthorizedError
x-amzn-errortype: UnauthorizedException
x-amz-apigw-id: xxxx
x-amzn-trace-id: Root=xxxx
```
In this instance this code will select the first header value.

See related - smithy-lang/smithy#1170
@gosar
Copy link
Contributor

gosar commented Sep 6, 2022

@eduardomourar @DanielBauman88 are you unblocked with the change to js repo?

On the Smithy side, we'd still want to add a protocol test for this, so other language SDKs can also support this. So leaving this issue open for that followup.

@DanielBauman88
Copy link

This does unblock me.

Appreciate you keeping this open - would be great to have the same support for all the supported languages!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OpenAPI protocol-test New protocol tests are needed
Projects
None yet
Development

No branches or pull requests

6 participants