🐛 tlsprofiles: guard empty parse results; write JSON atomically#2648
Conversation
Panic in parseProfile if all curves or all pre-TLS-1.3 ciphers are skipped, preventing a silent zero-value profile. Download mozilla_data.json to a temp file and mv atomically to avoid leaving a corrupt file on partial curl failure. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Todd Short <tshort@redhat.com>
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes edge cases where TLS profile parsing could silently produce empty/zero-value fields, and makes the Mozilla JSON update script resilient to partial downloads by writing atomically.
Changes:
- Add guards in
parseProfileto panic if no supported curves (or no pre-TLS1.3 cipher suites) are resolved. - Update the TLS profile update script to download to a temp file and
mvinto place atomically.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| internal/shared/util/tlsprofiles/mozilla_data.go | Adds validation to prevent empty curve/cipher results from being accepted silently. |
| hack/tools/update-tls-profiles.sh | Downloads JSON to a temp file and atomically moves it into place to avoid corrupt outputs on failure. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| panic(fmt.Sprintf("tlsprofiles: profile %q resolved no supported tls_curves from embedded mozilla_data.json", name)) | ||
| } | ||
| if version < tlsVersion(tls.VersionTLS13) && len(cipherNums) == 0 { | ||
| panic(fmt.Sprintf("tlsprofiles: profile %q resolved no supported cipher suites from embedded mozilla_data.json", name)) |
There was a problem hiding this comment.
This string literal is missing a closing quote (\") before the comma, which will cause the package to fail to compile. Add the missing quote so the fmt.Sprintf call is syntactically valid.
There was a problem hiding this comment.
It obviously compiled since the CI passed.
|
this is a small patch driven from a downstream review comment - it should be ok to downstream without major impact to QE efforts /lgtm |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2648 +/- ##
==========================================
+ Coverage 68.90% 68.91% +0.01%
==========================================
Files 141 141
Lines 10005 10009 +4
==========================================
+ Hits 6894 6898 +4
Misses 2595 2595
Partials 516 516
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
adding approved since Rashmi lgtmd /approve |
openshift-ci
bot
added
the
approved
|
/override codecov/patch |
|
@pedjak: Overrode contexts on behalf of pedjak: codecov/patch DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: perdasilva, rashmigottipati The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
bot
merged commit 8ad71d4
into
operator-framework:main
Panic in parseProfile if all curves or all pre-TLS-1.3 ciphers are skipped, preventing a silent zero-value profile. Download mozilla_data.json to a temp file and mv atomically to avoid leaving a corrupt file on partial curl failure.
Description
Reviewer Checklist