Nike-Inc / hal

hal provides an AWS Lambda Custom Runtime environment for your Haskell applications.
BSD 3-Clause "New" or "Revised" License
240 stars 13 forks source link

Support optional response bodies in API Gateway Proxy Responses #91

Closed JackKelly-Bellroy closed 3 years ago

JackKelly-Bellroy commented 3 years ago

It's useful to be able to generate 3xx redirections from an API Gateway Lambda Proxy Integration. These don't need to return a body.

You can test that this is fine by integrating a JS lambda like the one below with API Gateway:

index.js
exports.handler = async (event) => {
    const response = {
        statusCode: 302,
        headers: {
            "Location": "https://www.youtube.com/watch?v=dQw4w9WgXcQ"
        }
    };
    return response;
};

I've tried to keep the changes as small as possible, but this does force a breaking change to the API (if you weren't using smart constructors to generate response bodies). responseNoBody is a new smart constructor to avoid changing the type of response. Since it's a rarer occasion, a longer name feels fine.

Unless there's massive urgency for a new release, I think this should be batched up with #90, once I deploy some test lambdas.

Do we want to remove all the deprecated runtime functions when we bump the version number? I'm happy to do this in another PR.

IamfromSpace commented 3 years ago

This was purposefully omitted originally, because it appeared that proxy integration defaulted to application/json if one wasn't specified otherwise. Possibly this has changed or this was a sam-local-only behavior that I never double checked. Does not returning a body make for an actually valid no-body HTTP message back from API Gateway?

JackKelly-Bellroy commented 3 years ago

Using the index.js from the OP, here's what AWS spits out:

curl -v https://2jo1jx2sek.execute-api.us-east-1.amazonaws.com/jacko
*   Trying 23.21.95.64:443...
* Connected to 2jo1jx2sek.execute-api.us-east-1.amazonaws.com (23.21.95.64) port 443 (#0)
* ALPN, offering h2
* ALPN, offering http/1.1
* TLSv1.3 (OUT), TLS handshake, Client hello (1):
* TLSv1.3 (IN), TLS handshake, Server hello (2):
* TLSv1.2 (IN), TLS handshake, Certificate (11):
* TLSv1.2 (IN), TLS handshake, Server key exchange (12):
* TLSv1.2 (IN), TLS handshake, Server finished (14):
* TLSv1.2 (OUT), TLS handshake, Client key exchange (16):
* TLSv1.2 (OUT), TLS change cipher, Change cipher spec (1):
* TLSv1.2 (OUT), TLS handshake, Finished (20):
* TLSv1.2 (IN), TLS handshake, Finished (20):
* SSL connection using TLSv1.2 / ECDHE-RSA-AES128-GCM-SHA256
* ALPN, server accepted to use h2
* Server certificate:
*  subject: CN=*.execute-api.us-east-1.amazonaws.com
*  start date: Aug 19 00:00:00 2020 GMT
*  expire date: Sep 19 12:00:00 2021 GMT
*  subjectAltName: host "2jo1jx2sek.execute-api.us-east-1.amazonaws.com" matched cert's "*.execute-api.us-east-1.amazonaws.com"
*  issuer: C=US; O=Amazon; OU=Server CA 1B; CN=Amazon
*  SSL certificate verify ok.
* Using HTTP2, server supports multi-use
* Connection state changed (HTTP/2 confirmed)
* Copying HTTP/2 data in stream buffer to connection buffer after upgrade: len=0
* Using Stream ID: 1 (easy handle 0x1fe5dd0)
> GET /jacko HTTP/2
> Host: 2jo1jx2sek.execute-api.us-east-1.amazonaws.com
> user-agent: curl/7.74.0
> accept: */*
> 
* Connection state changed (MAX_CONCURRENT_STREAMS == 128)!
< HTTP/2 302 
< date: Mon, 03 May 2021 20:07:12 GMT
< content-type: application/json
< content-length: 0
< location: https://www.youtube.com/watch?v=dQw4w9WgXcQ
< x-amzn-requestid: eba6e610-3e50-4962-bea3-3c24dad07fb9
< x-amz-apigw-id: exKtiGiCIAMF61A=
< x-amzn-trace-id: Root=1-609057f0-37623e4d576770ac0305e4f8;Sampled=0
< 
* Connection #0 to host 2jo1jx2sek.execute-api.us-east-1.amazonaws.com left intact

We are definitely generating redirects that browsers obey, but it does looks like API Gateway will say content-type: application/json content-length: 0 if nothing is specified.

JackKelly-Bellroy commented 3 years ago

Okay, even if I return a Content-Type: application/octet-stream and a body: "" in my response, I can still get a request that redirects. Perhaps we close this PR, because it doesn't feel worth breaking backwards compatibility over?

IamfromSpace commented 3 years ago

Thanks for checking that! I saw that from sam-local, but should have really double checked it originally.

And yeah, I would opt for close as this is “as intended,” in that it captures the (less than ideal) model accurately—you have to specify a body either way, where you should or not!

I have been meaning to look into HttpApi integration, perhaps they have a better story around this.

JackKelly-Bellroy commented 3 years ago

Agreed.