- 
                Notifications
    You must be signed in to change notification settings 
- Fork 15
feat: [SVLS-6272] fips features for bottlecap #1028
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
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
  
    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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like a comment here to explain what this is doing.
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.
done.
| /// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| #[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.