sendgrid/rest

Document that a custom HTTP client should be used when deploying to AWS

bigokro opened this issue · 6 comments

rest/rest.go

Line 43 in 5a76b29

var DefaultClient = &Client{HTTPClient: http.DefaultClient}

For reasons I still haven't figured out, the fact that the library uses http.DefaultClient for the client in calls to the API can result in the Authorization: Bearer <API Key> header being replaced by one determined by AWS. This doesn't happen all the time (in our case, it was always working in our dev server, but always failing on our prod server - maybe it's a race condition to set up the DefaultClient):

Authorization:[AWS4-HMAC-SHA256 Credential=AKIATH.../us-east-1/es/aws4_request, SignedHeaders=accept;content-type;date;host;x-amz-date, Signature=368af...]

I was able to fix the problem by replacing your library with a raw net/http request, and using a custom http.Client:

client := &http.Client{Timeout: 3 * time.Second}

This is now working in production.

Hello @bigokrol,

Thank you for taking the time to report this issue. It looks like AWS suggests that you supply a custom client. Would it have been helpful if we documented this in the TROUBLESHOOTING section?

With best regards,

Elmer

Perhaps... it's been a while, so I don't remember how I came to discover the problem and the (custom) solution. I don't remember finding the Troubleshooting section, so not sure if that would be the best place (but wouldn't hurt, of course). I think I never found how to define my own client. It seems like the biggest help would be to have defining your own client be part of the initial "getting started" documentation, along with a special note that you really should do this if you are calling from AWS (I imagine this would impact a lot of users, but I could be wrong).

All the best,
Tim

Thats great feedback @bigokro! Thanks for taking the time to elaborate!

I agree that we should better surface the instructions on how to add a custom client with a special note that this is a strong suggestion for AWS users. I will rename and tag this issue accordingly.

I think it's better just to not use the default client in this lib. The line should be changed to just use a new client:

 var DefaultClient = &Client{HTTPClient: &http.Client{}}

I'd be happy to pick this up (first issue). Should I just update the docs or should I update the actual var as suggested above?

@mateorider I think just updating the var is fine. It's all yours!