-
Notifications
You must be signed in to change notification settings - Fork 45
Use FIPs endpoints in Govcloud regions #575
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
Use FIPs endpoints in Govcloud regions #575
Conversation
datadog_lambda/api.py
Outdated
if DD_API_KEY_SECRET_ARN: | ||
api_key = boto3.client("secretsmanager").get_secret_value( | ||
if is_gov_region and not use_non_fips_endpoints: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Secret can be from a different region, here we assume its from the same region as the Lambda
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can fix that in a separate PR
dadeeb5
to
bbf8042
Compare
datadog_lambda/api.py
Outdated
if is_gov_region: | ||
logger.info("Govcloud region detected. Using FIPs endpoints for secrets management.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logging on info
level here makes sense because this only runs once per Lambda execution context, and this only occurs in Govcloud regions where FIPs endpoints does matter to our users
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a comment, also, maybe add unit tests?
datadog_lambda/api.py
Outdated
if is_gov_region: | ||
logger.info( | ||
"Govcloud region detected. Using FIPs endpoints for secrets management." | ||
) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you really want to log it, use debug, I don't think we should add info
logs to our customers. We've had them complain in the past about us adding logs like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm open to changing it to debug, but just in case you didn't see my justification:
Logging on info level here makes sense because this only runs once per Lambda execution context, and this only occurs in Govcloud regions where FIPs endpoints does matter to our users
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did see it, I still stand by the same point. This still directly impacts billing, even though the scope is just once per (I imagine cold starts) execution context on GovCloud.
What does this PR do?
When creating the Secrets Manager, KMS, or SSM clients, use FIPs endpoints if in Govcloud regions.
Motivation
Make our products FIPs compliant
Testing Guidelines
Manually. Works in both commercial and govcloud regions.
Additional Notes
This part of the code only runs if the Lambda extension is not running, if
lambda_metric
is called with timestamp, or in LLM Observability (#572).Otherwise, the API key is handled by the lambda extension.
Types of Changes
Check all that apply