-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[fips140][configtls.TestTPM_tpmCertificate_errors] Skip test if GODEBUG=fips140=only is set
#14255
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
[fips140][configtls.TestTPM_tpmCertificate_errors] Skip test if GODEBUG=fips140=only is set
#14255
Conversation
CodSpeed Performance ReportMerging #14255 will degrade performances by 28.96%Comparing
|
| Benchmark | BASE |
HEAD |
Change | |
|---|---|---|---|---|
| ❌ | BenchmarkTraceSizeBytes |
309 µs | 430.9 µs | -28.28% |
| ❌ | BenchmarkLogsToProto2k |
45 µs | 63.3 µs | -28.96% |
| ❌ | BenchmarkTracesToProto2k |
69 µs | 95.8 µs | -27.94% |
Footnotes
-
20 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #14255 +/- ##
==========================================
- Coverage 92.15% 92.14% -0.02%
==========================================
Files 668 668
Lines 41513 41513
==========================================
- Hits 38257 38251 -6
- Misses 2219 2223 +4
- Partials 1037 1039 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dmathieu
left a comment
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 don't know about this. It silently skips tests, and seeing which test is being skipped could be unclear.
I think it would be clearer (though more verbose) to explicitly skip within eaach test rather than within an helper.
Makes sense. Updated in d0cff30. |
560225c to
d0cff30
Compare
8a96420 to
26220bd
Compare
|
Hi @dmathieu @bogdandrutu, the CI failures in this PR seem unrelated to the changes in this PR. I've tried rebasing on the latest |
|
The failures are due to a breaking change with contrib. They need to be fixed in another PR. |
26220bd to
d048a21
Compare
d048a21 to
c9bcee3
Compare
2c4964c
…DEBUG=fips140=only` is set (open-telemetry#14255) <!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description In open-telemetry#14225, we skipped the `configtls.TestTPM_loadCertificate` unit test if the tests were run with `GODEBUG=fips140=only`. Otherwise, the unit test failed with a `panic: crypto/cipher: use of CFB is not allowed in FIPS 140-only mode` error. Turns out there was a second unit test in the same package that needed skipping for the same reason: `configtls.TestTPM_tpmCertificate_errors`. This PR skips it too. <!-- Issue number if applicable --> #### Link to tracking issue Follow up to open-telemetry#14225
…DEBUG=fips140=only` is set (open-telemetry#14255) <!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description In open-telemetry#14225, we skipped the `configtls.TestTPM_loadCertificate` unit test if the tests were run with `GODEBUG=fips140=only`. Otherwise, the unit test failed with a `panic: crypto/cipher: use of CFB is not allowed in FIPS 140-only mode` error. Turns out there was a second unit test in the same package that needed skipping for the same reason: `configtls.TestTPM_tpmCertificate_errors`. This PR skips it too. <!-- Issue number if applicable --> #### Link to tracking issue Follow up to open-telemetry#14225
Description
In #14225, we skipped the
configtls.TestTPM_loadCertificateunit test if the tests were run withGODEBUG=fips140=only. Otherwise, the unit test failed with apanic: crypto/cipher: use of CFB is not allowed in FIPS 140-only modeerror.Turns out there was a second unit test in the same package that needed skipping for the same reason:
configtls.TestTPM_tpmCertificate_errors. This PR skips it too.Link to tracking issue
Follow up to #14225