search

DataDog / datadog-lambda-go

The Datadog AWS Lambda package for Go
Apache License 2.0
59 stars 40 forks source link

Add support for `DD_API_KEY_SECRET_ARN` environment variable #101

Closed skatsuta closed 2 years ago

skatsuta commented 2 years ago

What does this PR do?

Adds support for DD_API_KEY_SECRET_ARN environment variable.

Closes #98.

Motivation

When using the Datadog Lambda Extension for Lambda APM, it is necessary to set the API key to an environment variable, but it is not secure to set the API key in plain text to DD_API_KEY. I use DD_KMS_API_KEY for this reason, but I felt it was very troublesome to have to encrypt the key using KMS.

I looked at Serverless Agent and found that it also supports DD_API_KEY_SECRET_ARN, which I think would be very useful. I also saw #98 and knew that there were others with similar needs.

So I wanted to add support for DD_API_KEY_SECRET_ARN to this library. I believe this feature would be very beneficial to Datadog users who use the Lambda Extension.

Testing Guidelines

I first tagged this working branch in my forked repository, added a replace directive to go.mod for our microservice that uses this library so that it uses my forked repository instead, and built the Go binary.

I then set DD_API_KEY_SECRET_ARN instead of DD_KMS_API_KEY to environment variables for the microservice's Lambda function, and deployed the binary to Lambda.

Finally, I ran this Lambda function and confirmed that metrics and traces were sent to Datadog as expected. I also confirmed that the debug logs I added were output on CloudWatch Logs.

I strongly felt that not having to set the API key (even if it is encrypted) directly to an environment variable is a very good user experience.

Additional Notes

I also added an explanation for DD_KMS_API_KEY and DD_API_KEY_SECRET_ARN to README, but I'm not sure if this explanation is clear enough for users, so if you have any idea to improve it, I would appreciate it.

Types of changes

Checklist

tianchu commented 2 years ago

@skatsuta Thanks for your great contribution! But unfortunately our readme must mislead you and I'm very sorry 🙇 . When using the Lambda Extension (serverless agent), this go library actually does NOT even need the Datadog API Key at all. Only the Extension needs the Datadog API Key and the Extension already support DD_API_KEY_SECRET_ARN. This library only needs the Datadog API Key for a legacy feature that sends metrics synchronously to Datadog API.

Therefore, the right fix is actually simpler, we just need to update this line https://github.com/DataDog/datadog-lambda-go/blob/23c42de8629d79c3beab3aaf3221cc73424a08ba/ddlambda.go#L266-L267 to NOT error if isExtensionRunning https://github.com/DataDog/datadog-lambda-go/blob/23c42de8629d79c3beab3aaf3221cc73424a08ba/internal/extension/extension.go#L37

We actually just fixed something similar for the Node.js library here https://github.com/DataDog/datadog-lambda-js/pull/273.

Again, very sorry for the confusion we have caused 🙇‍♂️ If you are interested in updating your PR, let me know. Otherwise, we should be able to find a Datadog engineer to fix this soon.

skatsuta commented 2 years ago

@tianchu Oh, I see! I actually wondered about that, "Does this library really need an API key when using the Lambda Extension? Isn't it only Serverless Agent that needs to use an API key?" However, I was not entirely sure that this library never send any data directly to Datadog servers when using the Lambda Extension, so I conservatively assumed that this library would also require an API key. So, no need to apologize at all, that's rather good news for me because the question has been resolved! It's simply that the error message you point to is wrong to appear!

Sure, I'm willing to make the fix you suggest if necessary, but as for the README, I'm not sure how to fix it, so would it be okay if I just fix the line of code you pointed out (and test it in my microservice)? In that case, I hope you will update the README later.

If the above fix is OK, I will either update this PR (actually, I will delete all current commits and force push the above fix), or close this PR and create a new one with only the above fix. On the other hand, if it is better to update the README as well, I will simply close this PR and leave the fix to you or other Datadog engineers.

tianchu commented 2 years ago

@skatsuta Thanks, if you are willing to help, please go ahead make the code changes 🙇‍♂️ I 'm happy to take care of the readme update separately.

skatsuta commented 2 years ago

@tianchu Thanks for your support! I'm closing this PR and will make a new one.

skatsuta commented 2 years ago

Created #102