Skip to content

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

Merged
merged 11 commits into from
Mar 12, 2025

Conversation

nhulston
Copy link
Contributor

@nhulston nhulston commented Mar 11, 2025

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

  • 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)

@nhulston nhulston requested a review from a team as a code owner March 11, 2025 20:22
@nhulston nhulston changed the title Nicholas.hulston/use fips endpoints in gov regions Use FIPs endpoints in Govcloud regions Mar 11, 2025
if DD_API_KEY_SECRET_ARN:
api_key = boto3.client("secretsmanager").get_secret_value(
if is_gov_region and not use_non_fips_endpoints:
Copy link
Contributor

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

Copy link
Contributor Author

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

@nhulston nhulston force-pushed the nicholas.hulston/use-fips-endpoints-in-gov-regions branch from dadeeb5 to bbf8042 Compare March 11, 2025 20:52
Comment on lines 69 to 70
if is_gov_region:
logger.info("Govcloud region detected. Using FIPs endpoints for secrets management.")
Copy link
Contributor Author

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

Copy link
Contributor

@duncanista duncanista left a 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?

Comment on lines 69 to 73
if is_gov_region:
logger.info(
"Govcloud region detected. Using FIPs endpoints for secrets management."
)

Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor

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.

@nhulston nhulston requested a review from duncanista March 12, 2025 14:38
@nhulston nhulston merged commit 71b64fa into main Mar 12, 2025
60 checks passed
@nhulston nhulston deleted the nicholas.hulston/use-fips-endpoints-in-gov-regions branch March 12, 2025 18:16
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.

2 participants