OCPNODE-4114: Update TLSSecurityProfile doc to reflect CRI-O propagation and TLS 1.3 support#2747
OCPNODE-4114: Update TLSSecurityProfile doc to reflect CRI-O propagation and TLS 1.3 support#2747asahay19 wants to merge 1 commit intoopenshift:masterfrom
Conversation
…ion and TLS 1.3 support Signed-off-by: Aditi Sahay <asahay@redhat.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
Hello @asahay19! Some important instructions when contributing to openshift/api: |
|
@asahay19: This pull request references OCPNODE-4114 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
Review Summary by QodoUpdate TLSSecurityProfile docs for TLS 1.3 and CRI-O support
WalkthroughsDescription• Update TLSSecurityProfile documentation to reflect VersionTLS13 support • Document CRI-O propagation via ContainerRuntimeConfig controller • Clarify TLS configuration applies to both kubelet and CRI-O • Regenerate auto-generated files with updated documentation Diagramflowchart LR
A["KubeletConfigSpec<br/>TLSSecurityProfile"] -->|"Updated docs"| B["Supports VersionTLS13<br/>max version"]
A -->|"Propagates to"| C["Kubelet"]
A -->|"Propagates to"| D["CRI-O via<br/>drop-in config"]
D -->|"Managed by"| E["ContainerRuntimeConfig<br/>controller"]
File Changes1. machineconfiguration/v1/types.go
|
Code Review by Qodo
1. TLS profile docs contradict schema
|
openshift-ci
bot
added
the
size/S
📝 WalkthroughWalkthroughThis pull request updates documentation for KubeletConfig TLS security settings. The maximum supported minTLSVersion is updated from VersionTLS12 to VersionTLS13 in both the Go type documentation and the Kubernetes CRD schema. The changes also add clarification that TLS security profile configuration applies to both the kubelet and CRI-O on nodes matching the pool selector, with CRI-O receiving the minimum TLS version through a drop-in configuration managed by the ContainerRuntimeConfig controller. No functional code modifications are introduced. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| // Note that only Old and Intermediate profiles are currently supported, and | ||
| // the maximum available minTLSVersion is VersionTLS12. | ||
| // the maximum available minTLSVersion is VersionTLS13. | ||
| // When set, this TLS configuration is applied to both the kubelet and CRI-O | ||
| // on nodes matching the pool selector. CRI-O receives the minimum TLS version | ||
| // via a drop-in configuration file managed by the ContainerRuntimeConfig controller. |
There was a problem hiding this comment.
1. Tls profile docs contradict schema 🐞 Bug ✓ Correctness
The updated TLSSecurityProfile comment says only Old/Intermediate profiles are supported while also claiming the maximum minTLSVersion is VersionTLS13, which implies Modern/Custom (TLS 1.3) support. This contradicts the underlying TLSSecurityProfile schema and the generated CRD enum (which allow Modern/Custom and VersionTLS13), making the API documentation misleading and potentially causing users to assume unsupported/unsupported behavior incorrectly.
Agent Prompt
### Issue description
`KubeletConfigSpec.TLSSecurityProfile` docs are contradictory: they say only `Old`/`Intermediate` are supported, but also claim the max `minTLSVersion` is `VersionTLS13` (which implies `Modern`/`Custom`). This conflicts with the schema/CRD which allows `Modern`/`Custom` and `VersionTLS13`, and may mislead users.
### Issue Context
This PR is documentation-focused, so correctness/clarity of the API docstrings is the core deliverable. The CRD schema is broader than the current prose, so the prose must clearly state what the operator actually supports/propagates.
### Fix Focus Areas
- machineconfiguration/v1/types.go[758-765]
- machineconfiguration/v1/zz_generated.swagger_doc_generated.go[247-251]
- machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_kubeletconfigs.crd.yaml[111-117]
- machineconfiguration/v1/zz_generated.featuregated-crd-manifests/kubeletconfigs.machineconfiguration.openshift.io/AAA_ungated.yaml[112-118]
- payload-manifests/crds/0000_80_machine-config_01_kubeletconfigs.crd.yaml[111-117]
(After updating the source comment, re-run the repo’s generation scripts as appropriate so generated CRDs/swagger docs remain consistent.)
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@machineconfiguration/v1/types.go`:
- Around line 760-763: Update the comment and mirrored CRD description around
the TLS profile and CRI-O propagation to clearly state that only Old and
Intermediate named profiles are supported for full TLS/profile config, that
VersionTLS13 is the highest allowed value for minTLSVersion, and that CRI-O only
receives the minimum TLS version (minTLSVersion) via a drop-in file managed by
the ContainerRuntimeConfig controller — it does NOT receive Modern or custom
profiles or custom cipher suites or other profile-specific settings; change text
near the minTLSVersion comment and the corresponding CRD description to
explicitly limit CRI-O propagation to minTLSVersion only and to warn that
Modern/custom profiles and cipher settings are not applied to CRI-O.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro
Run ID: d73d6300-f70b-4af8-8d70-2541ca57fc62
⛔ Files ignored due to path filters (3)
machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_kubeletconfigs.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.featuregated-crd-manifests/kubeletconfigs.machineconfiguration.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*
📒 Files selected for processing (2)
machineconfiguration/v1/types.gopayload-manifests/crds/0000_80_machine-config_01_kubeletconfigs.crd.yaml
| // the maximum available minTLSVersion is VersionTLS13. | ||
| // When set, this TLS configuration is applied to both the kubelet and CRI-O | ||
| // on nodes matching the pool selector. CRI-O receives the minimum TLS version | ||
| // via a drop-in configuration file managed by the ContainerRuntimeConfig controller. |
There was a problem hiding this comment.
Clarify supported-profile scope and CRI-O propagation here.
Combined with the preceding sentence, this now reads inconsistently: it still says only Old/Intermediate are supported, but also advertises VersionTLS13 as the max minTLSVersion, and it says the TLS config applies to CRI-O even though the next sentence narrows that to only the minimum TLS version. Please tighten the wording so users do not infer that Modern/custom profiles or custom cipher settings are fully honored for CRI-O. This same text is mirrored into the generated CRD description, so update both in lockstep.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@machineconfiguration/v1/types.go` around lines 760 - 763, Update the comment
and mirrored CRD description around the TLS profile and CRI-O propagation to
clearly state that only Old and Intermediate named profiles are supported for
full TLS/profile config, that VersionTLS13 is the highest allowed value for
minTLSVersion, and that CRI-O only receives the minimum TLS version
(minTLSVersion) via a drop-in file managed by the ContainerRuntimeConfig
controller — it does NOT receive Modern or custom profiles or custom cipher
suites or other profile-specific settings; change text near the minTLSVersion
comment and the corresponding CRD description to explicitly limit CRI-O
propagation to minTLSVersion only and to warn that Modern/custom profiles and
cipher settings are not applied to CRI-O.
bitoku
left a comment
bitoku
left a comment
There was a problem hiding this comment.
APIserver needs change mentioning apiserver config will affect kubletconfig and container runtime config when tls is not configured in kubelet config.
3 participants
This PR is regarding updates in the KubeletConfigSpec.TLSSecurityProfile comment in machineconfiguration/v1/types.go to reflect two changes:
Related PRs: