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

add secret_key for getting secret value #376

Closed
wants to merge 3 commits into from

Conversation

wangzz2019
Copy link

What does this PR do?

Described in issue #375
would like to add DD_API_KEY_SECRET_KEY for secret key setting

Motivation

would like to solve the issue soon

Testing Guidelines

Additional Notes

Types of changes

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

Check all that apply

  • 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
  • This PR passes the integration tests (ask a Datadog member to run the tests)
  • This PR passes the unit tests
  • This PR passes the installation tests (ask a Datadog member to run the tests)

"SecretString"
]
DD_API_KEY = json.loads(secretstring)[SECRET_KEY]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SecretsManager can return plaintext, its not always json and for the forwarders I believe it is the plaintext that is used so this would fail

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, secretstring is a plaintext as string

>>> import json
>>> s='{"secret_key":"apikey"}'
>>> print(s)
{"secret_key":"apikey"}
>>> print(json.loads(s)["secret_key"])
apikey

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, what I meant to say is that the value stored in SecretsManager is just the API key itself like 'xxxxxxxxxxxx' which means this would break for everyone who has it setup this way

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, Yes, agree with you bryanbigs.
If so, at least we should provide an options for people who used secret_key

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wangzz2019

If so, at least we should provide an options for people who used secret_key

Does any AWS or Datadog doc lead you to storing the api key in the {"secret_key":"apikey"} JSON format, instead of in the plain-text/string format?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tianchu
There is nothing mentioned in our docs, but here is the AWS documents:
https://docs.aws.amazon.com/secretsmanager/latest/userguide/manage_create-basic-secret.html

Specify the details of your custom secret as Key and Value pairs. For example, you can specify a key of UserName, and then supply the appropriate user name as the value. Add a second key with the name of Password and the password text as the value. You could also add entries for Database name, Server address, TCP port, and so on. You can add as many pairs as you need to store the information you require.

Alternatively, you can choose the Plaintext tab and enter the secret value in any format.

Key Value pairs is the default options on console.
If we only support plaintext, it is better to mention it in our docs

@tianchu
Copy link
Contributor

tianchu commented Nov 3, 2020

Closing this PR, as I think it's better to update the doc, rather than giving users two options to store the api key in secrets manager, which may cause even more confusion in the future. See the PR for doc updates: #385

@tianchu tianchu closed this Nov 3, 2020
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.

None yet

3 participants