feat: [SVLS-6272] fips features for bottlecap#1028
Conversation
BenchmarksComparisonBenchmark execution time: 2025-04-24 16:58:13 Comparing candidate commit 1a60690 in PR branch Found 17 performance improvements and 2 performance regressions! Performance is the same for 33 metrics, 2 unstable metrics. scenario:credit_card/is_card_number/ 3782-8224-6310-005
scenario:credit_card/is_card_number/ 378282246310005
scenario:credit_card/is_card_number/378282246310005
scenario:credit_card/is_card_number/x371413321323331
scenario:credit_card/is_card_number_no_luhn/x371413321323331
scenario:normalization/normalize_name/normalize_name/Too-Long-.Too-Long-.Too-Long-.Too-Long-.Too-Long-.Too-Lo...
scenario:normalization/normalize_name/normalize_name/bad-name
scenario:normalization/normalize_name/normalize_name/good
scenario:normalization/normalize_service/normalize_service/A0000000000000000000000000000000000000000000000000...
scenario:sql/obfuscate_sql_string
CandidateCandidate benchmark detailsGroup 1
Group 2
Group 3
Group 4
Group 5
Group 6
Group 7
Group 8
Group 9
Group 10
Group 11
Group 12
Group 13
BaselineOmitted due to size. |
Artifact Size Benchmark Reportaarch64-alpine-linux-musl
aarch64-unknown-linux-gnu
libdatadog-x64-windows
libdatadog-x86-windows
x86_64-alpine-linux-musl
x86_64-unknown-linux-gnu
|
f550411 to
e80a19b
Compare
bantonsson
left a comment
There was a problem hiding this comment.
Looks good with cargo tree -p ddcommon --features fips
f874750 to
404e0eb
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1028 +/- ##
==========================================
+ Coverage 71.35% 71.38% +0.03%
==========================================
Files 329 329
Lines 49165 49182 +17
==========================================
+ Hits 35082 35110 +28
+ Misses 14083 14072 -11
🚀 New features to boost your workflow:
|
097862c to
eef7b6e
Compare
There was a problem hiding this comment.
Honestly, the crypto provider and certificates stuff is a mess. We're shipping ring and aws-lc-rs, and rust-native-certs and webpki.
But I won't hold status quo against you. Just know in the future if we straighten it out, you may need to adjust features and code to match.
ya, this was pretty tricky to navigate. we have a build script in bottlecap which checks that |
| fips = ["https", "hyper-rustls/fips"] | ||
|
|
||
| [lints.rust] | ||
| unexpected_cfgs = { level = "warn", check-cfg = ['cfg(coverage)'] } |
There was a problem hiding this comment.
I would like a comment here to explain what this is doing.
| /// happens here. On non-unix platforms, ddcommon uses `ring` instead, which handles this | ||
| /// at rustls initialization. TODO: Move to the more ergonomic LazyLock when MSRV is 1.80 | ||
| /// In fips mode we expect someone to have done this already. | ||
| #[cfg(any(not(feature = "fips"), coverage))] |
There was a problem hiding this comment.
| #[cfg(any(not(feature = "fips"), coverage))] | |
| #[cfg(any(not(feature = "fips"), coverage))] |
Can't there be a default here? instead of doing not feature?
There was a problem hiding this comment.
how would we write that in a cfg line? we want this to be run in both coverage checks or for a non-fips build
d3bc0f3 to
6459250
Compare
6459250 to
4c441b2
Compare
4c441b2 to
9132f4e
Compare
9132f4e to
1a60690
Compare
We need a `ddcommon/fips` and a `trace_utils/fips` feature. This functionality is written specifically for use in the datadog-lambda-extension. Other use cases should probably validate carefully that the right dependencies are being used in their builds.
What does this PR do?
Adds FIPS support. This is intended for use with bottlecap for now, so the changes here are very focused on bottlecap usage: DataDog/datadog-lambda-extension#644 .
Additional Notes
I'm not a rust expert, please advise better, cleaner, more correct ways to do this.
How to test the change?
Built and deployed bottlecap in all of our various self-monitoring stacks.