Skip to content

Add support for DD_API_KEY_SECRET_ARN environment variable #101

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

Closed
wants to merge 5 commits into from

Conversation

skatsuta
Copy link
Contributor

@skatsuta skatsuta commented Mar 15, 2022

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

  • Bug fix
  • New feature
  • Breaking change
  • Misc (docs, refactoring, dependency upgrade, etc.)

Checklist

  • This PR's description is comprehensive
  • This PR contains breaking changes that are documented in the description
  • This PR introduces new APIs or parameters that are documented and unlikely to change in the foreseeable future
  • This PR impacts documentation, and it has been updated (or a ticket has been logged)
  • This PR's changes are covered by the automated tests
  • This PR collects user input/sensitive content into Datadog

@skatsuta skatsuta changed the title Support secret arn Add support for DD_API_KEY_SECRET_ARN environment variable Mar 15, 2022
@skatsuta skatsuta changed the title Add support for DD_API_KEY_SECRET_ARN environment variable Add support for DD_API_KEY_SECRET_ARN environment variable Mar 15, 2022
@skatsuta skatsuta marked this pull request as ready for review March 15, 2022 16:04
@skatsuta skatsuta requested a review from a team as a code owner March 15, 2022 16:04
@tianchu tianchu self-assigned this Mar 15, 2022
@tianchu
Copy link
Contributor

tianchu commented Mar 15, 2022

@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

if mc.APIKey == "" && mc.KMSAPIKey == "" && !mc.ShouldUseLogForwarder {
logger.Error(fmt.Errorf("couldn't read DD_API_KEY or DD_KMS_API_KEY from environment"))
to NOT error if isExtensionRunning
isExtensionRunning bool

We actually just fixed something similar for the Node.js library here DataDog/datadog-lambda-js#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
Copy link
Contributor Author

skatsuta commented Mar 16, 2022

@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
Copy link
Contributor

tianchu commented Mar 16, 2022

@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
Copy link
Contributor Author

skatsuta commented Mar 17, 2022

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

@skatsuta
Copy link
Contributor Author

Created #102

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for DD_API_KEY_SECRET_ARN environment variable
2 participants