CM-1113: Replace unsafe.Pointer casts with Kubernetes conversion functions - #438
CM-1113: Replace unsafe.Pointer casts with Kubernetes conversion functions#438sebrandon1 wants to merge 1 commit into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@sebrandon1: This pull request explicitly references no jira issue. 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR replaces unsafe Kubernetes type reinterpretation with explicit conversion helpers. Shared validation and certmanager scheduling paths use these conversions. Tests cover toleration round trips, empty inputs, nil affinity rules, and missing TLS custom settings. ChangesKubernetes conversion cleanup
TLS profile validation coverage
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sebrandon1 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 |
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/controller/common/validation.go`:
- Around line 22-23: The conversion functions ToCoreTolerations and
ToV1Tolerations must return (converted, error) instead of swallowing conversion
errors; update their signatures to return the error from
corev1conversion.Convert_* and change ValidateTolerationsConfig (and any callers
like the functions in deployment_overrides_validation.go and
deployment_helper.go) to accept the error, propagate it, and include conversion
errors in the returned validation.Aggregate (or wrap them into an appropriate
field error) so conversion failures are not silently ignored during
ValidateTolerations/ValidateTolerationsConfig flows.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e1b6f377-bc1b-484f-ac46-8a4699facf78
📒 Files selected for processing (3)
pkg/controller/certmanager/deployment_helper.gopkg/controller/certmanager/deployment_overrides_validation.gopkg/controller/common/validation.go
sebrandon1
changed the title
|
@sebrandon1: This pull request references CM-1113 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 "5.0.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. |
2904b5e to
8325b32
Compare
d567c8f to
c051781
Compare
c051781 to
b93c005
Compare
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
b93c005 to
1d984a0
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/controller/common/validation_test.go (1)
23-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding empty-slice coverage for
ToV1Tolerations.
TestToCoreTolerations_Emptyonly exercisesToCoreTolerationswith an empty slice. Adding the symmetricToV1Tolerationscase would make the nil/empty coverage consistent across both helpers.♻️ Optional improvement
func TestToCoreTolerations_Empty(t *testing.T) { result := ToCoreTolerations([]corev1.Toleration{}) require.Empty(t, result) + require.Empty(t, ToV1Tolerations([]core.Toleration{})) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/controller/common/validation_test.go` around lines 23 - 31, Add empty-slice coverage for ToV1Tolerations alongside TestToCoreTolerations_Empty, invoking ToV1Tolerations with an empty toleration slice and asserting the result is empty, while preserving the existing nil and ToCoreTolerations tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/controller/common/validation_test.go`:
- Around line 23-31: Add empty-slice coverage for ToV1Tolerations alongside
TestToCoreTolerations_Empty, invoking ToV1Tolerations with an empty toleration
slice and asserting the result is empty, while preserving the existing nil and
ToCoreTolerations tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a83f40c0-c2cc-4ed3-b591-674edae8f1b4
📒 Files selected for processing (2)
pkg/controller/common/validation_test.gopkg/tlsprofile/tlsprofile_test.go
5140dd7 to
dbb614e
Compare
|
/rebase |
dbb614e to
e15d83b
Compare
e15d83b to
b19a9b2
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/controller/common/validation_test.go (2)
19-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the local variable to avoid shadowing the
corepackage import.The local variable
coreon Line 19 shadows the imported package aliascorefromk8s.io/kubernetes/pkg/apis/core(Line 8). The test still compiles because the package alias isn't referenced again in this function, but this creates a naming hazard for future edits in this scope.♻️ Proposed fix
- core := ToCoreTolerations(input) - require.Len(t, core, 2) - roundTripped := ToV1Tolerations(core) + coreTolerations := ToCoreTolerations(input) + require.Len(t, coreTolerations, 2) + roundTripped := ToV1Tolerations(coreTolerations)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/controller/common/validation_test.go` at line 19, Rename the local variable assigned from ToCoreTolerations in the affected test so it no longer shadows the imported core package alias; update all subsequent references to that local variable while preserving the test behavior.
35-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the non-nil
ValidateAffinityRulespath.
TestValidateAffinityRules_Nilonly exercises the nil-affinity short-circuit. The conversion path inValidateAffinityRules(viacorev1conversion.Convert_v1_Affinity_To_core_Affinity) is left untested in this file. Add a test with a populatedcorev1.Affinityto confirm the generated conversion and validation produce the expected result or error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/controller/common/validation_test.go` around lines 35 - 37, Add a test alongside TestValidateAffinityRules_Nil that passes a populated corev1.Affinity to ValidateAffinityRules, exercising the corev1conversion.Convert_v1_Affinity_To_core_Affinity path and asserting the expected validation result or error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/controller/common/validation_test.go`:
- Line 19: Rename the local variable assigned from ToCoreTolerations in the
affected test so it no longer shadows the imported core package alias; update
all subsequent references to that local variable while preserving the test
behavior.
- Around line 35-37: Add a test alongside TestValidateAffinityRules_Nil that
passes a populated corev1.Affinity to ValidateAffinityRules, exercising the
corev1conversion.Convert_v1_Affinity_To_core_Affinity path and asserting the
expected validation result or error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d70455ca-775b-4fb7-bbd6-1b9b96cf408f
📒 Files selected for processing (5)
pkg/controller/certmanager/deployment_helper.gopkg/controller/certmanager/deployment_overrides_validation.gopkg/controller/common/validation.gopkg/controller/common/validation_test.gopkg/tlsprofile/tlsprofile_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/tlsprofile/tlsprofile_test.go
- pkg/controller/certmanager/deployment_helper.go
- pkg/controller/certmanager/deployment_overrides_validation.go
- pkg/controller/common/validation.go
…tions Replace unsafe.Pointer casts in validation.go and deployment_helper.go with proper Kubernetes auto-generated conversion functions (corev1conversion.Convert_*). Add ToCoreTolerations/ToV1Tolerations helper functions for safe toleration type conversion. Add round-trip unit tests for the new conversion functions and a test for the EffectiveSpec nil Custom field error path.
b19a9b2 to
bc6fa2f
Compare
|
@sebrandon1: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
3 participants
Summary
unsafe.Pointercasts betweencorev1.*andcore.*types with Kubernetes auto-generated conversion functions fromk8s.io/kubernetes/pkg/apis/core/v1"unsafe"import from 3 filesToCoreTolerationsandToV1Tolerationshelpers inpkg/controller/common/validation.goto avoid repeating the conversion loop patternWhy
The operator uses Kubernetes internal validation functions (
corevalidation.ValidateTolerations, etc.) which require internalcore.*types, but the public API usescorev1.*types. Previously, this was bridged with rawunsafe.Pointercasts like:While the types are memory-layout compatible, raw unsafe casts are fragile and bypass Go's type system. Kubernetes already provides auto-generated conversion functions that handle this properly. The conversion functions still use
unsafe.Pointerinternally, but the cast is encapsulated, upstream-maintained, and follows the official Kubernetes conversion pattern.Files changed
pkg/controller/common/validation.gopkg/controller/certmanager/deployment_helper.gocommon.ToCoreTolerations/common.ToV1Tolerationspkg/controller/certmanager/deployment_overrides_validation.gocommon.ToCoreTolerationsTest plan
go build ./...compiles cleanlymake testpassesmake lintintroduces no new issuesSummary by CodeRabbit
Bug Fixes
Tests