-
Notifications
You must be signed in to change notification settings - Fork 582
NO-JIRA: update TLS security profile documentation for clarity and consistency #2595
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
base: master
Are you sure you want to change the base?
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Hello @sanchezl! Some important instructions when contributing to openshift/api: |
WalkthroughUpdated documentation and comments in the TLS security profile types file to reference Mozilla TLS 5.0, clarify profile field meanings and equivalences (Old, Intermediate, Modern, Custom), and refine wording and forward-compatibility notes. No structural or API changes. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
config/v1/types_tlssecurityprofile.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
config/v1/types_tlssecurityprofile.go
🔇 Additional comments (7)
config/v1/types_tlssecurityprofile.go (7)
7-20: ✓ Clear, consistent documentation for Type field.The expanded comments effectively explain the profile types and their relationship to Mozilla guidelines. The intent-based nature of profiles and potential cipher list reduction are now explicit, which will help users understand forward compatibility considerations.
22-63: ✓ Old profile documentation effectively demonstrates equivalence.The expanded comment clearly explains the use case and provides a concrete Custom profile equivalent. This will help users understand what they're getting with the Old profile and how to replicate it if needed.
69-89: ✓ Intermediate profile documentation is clear and consistent.The expanded documentation provides good context about use cases and equivalence to Custom profiles, maintaining consistency with the Old profile documentation style.
91-103: ✓ Modern profile documentation is clear and appropriately concise.The comment effectively communicates the TLS 1.3 focus and provides the Custom profile equivalent. The reduced verbosity is appropriate given the profile's simplicity.
121-131: ✓ Profile type struct documentation improves discoverability.Adding documentation comments to
OldTLSProfile,IntermediateTLSProfile, andModernTLSProfiletypes provides better context for developers exploring the API and improves code documentation generation tools (e.g.,godoc).
144-157: ✓ TLSProfileType constant documentation is clear and consistent.Updated comments consistently reference the Mozilla Server Side TLS guidelines and provide good context for each constant value. This improves maintainability and user understanding.
201-210: ✓ TLSProfiles map documentation is clearer and more actionable.The updated comments provide explicit Mozilla guidelines reference with version and date, and the NOTE section is reworded for better clarity about validation and whitelisting. This will help operators understand the constraints and appropriate usage patterns.
ce5b632 to
be117b4
Compare
|
@sanchezl: This pull request explicitly references no jira issue. In 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 openshift-eng/jira-lifecycle-plugin repository. |
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
config/v1/types_tlssecurityprofile.go (1)
201-210: Consider replacing “whitelist” with “allowlist” in NOTEFor more modern and inclusive terminology, you may want to replace “whitelist” with “allowlist” in the NOTE about callers validating constants. For example:
-// of ties, the kube-apiserver wins. Do not fail, just be sure to whitelist only -// and everything will be ok. +// of ties, the kube-apiserver wins. Do not fail, just be sure to allowlist only +// and everything will be ok.This keeps the meaning while aligning with current wording conventions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
config/v1/types_tlssecurityprofile.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
config/v1/types_tlssecurityprofile.go
🔇 Additional comments (1)
config/v1/types_tlssecurityprofile.go (1)
7-118: Doc updates for TLS profiles look consistent and accurateThe updated comments for
TLSSecurityProfile, the Old/Intermediate/Modern profile fields, and the associated profile types cleanly describe intent, Mozilla TLS 5.0 basis, and the equivalence to the concreteTLSProfilesentries (minTLSVersion and cipher lists) without changing behavior. This improves clarity while keeping the API stable.Also applies to: 121-158
|
@sanchezl: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
everettraven
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.
These docs updates look good to me, but you will need to regenerate the CRDs.
If you run PROTO_OPTIONAL=true make update and include the changes in your commit, that should resolve the verify failure
Update TLS security profile documentation for clarity and consistency in preparation for possibly updating the pre-defined TLS profiles in an upcoming release.