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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThis PR replaces unsafe pointer-based toleration conversions with explicit Kubernetes conversion functions. It adds reusable toleration slice helpers in ChangesUnsafe conversion refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
| return corevalidation.ValidateTolerations(ToCoreTolerations(tolerations), fldPath.Child("tolerations")).ToAggregate() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="pkg/controller/common/validation.go"
echo "=== File exists? ==="
ls -la "$FILE" || exit 1
echo
echo "=== Lines 1-140 with numbers ==="
nl -ba "$FILE" | sed -n '1,140p'
echo
echo "=== Lines 140-260 with numbers ==="
nl -ba "$FILE" | sed -n '140,260p' || true
echo
echo "=== Find corev1conversion.Convert_* usage with ignored errors ==="
rg -n --hidden --no-ignore-vcs 'corev1conversion\.Convert_.*\(\s*[^)]*nil\)\s*$|_\s*=\s*corev1conversion\.Convert_|=\s*corev1conversion\.Convert_.*;?\s*$' "$FILE" || true
echo
echo "=== Find corev1conversion.Convert_* and see whether return is used ==="
rg -n --hidden --no-ignore-vcs 'corev1conversion\.Convert_[A-Za-z0-9_]*\(' "$FILE" || trueRepository: openshift/cert-manager-operator
Length of output: 277
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="pkg/controller/common/validation.go"
echo "=== Lines 1-140 with numbers (awk) ==="
awk '{printf "%d\t%s\n", NR, $0}' "$FILE" | sed -n '1,140p'
echo
echo "=== Lines 140-260 with numbers (awk) ==="
awk '{printf "%d\t%s\n", NR, $0}' "$FILE" | sed -n '140,260p'
echo
echo "=== corev1conversion.Convert_* calls that may ignore errors ==="
rg -n --hidden --no-ignore-vcs 'corev1conversion\.Convert_.*\(' "$FILE" || true
echo
echo "=== Specifically look for '_' assignment to Convert_* ==="
rg -n --hidden --no-ignore-vcs '(_\s*=\s*)?corev1conversion\.Convert_.*' "$FILE" || trueRepository: openshift/cert-manager-operator
Length of output: 4672
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "ToCoreTolerations\(" pkg/controller || true
rg -n "ToV1Tolerations\(" pkg/controller || true
rg -n "ToCoreTolerations\(" . || true
rg -n "ToV1Tolerations\(" . || trueRepository: openshift/cert-manager-operator
Length of output: 1933
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect call sites mentioned by rg
sed -n '260,340p' pkg/controller/certmanager/deployment_overrides_validation.go
echo "----"
sed -n '80,160p' pkg/controller/certmanager/deployment_helper.goRepository: openshift/cert-manager-operator
Length of output: 5464
Handle conversion errors instead of discarding them.
corev1conversion.Convert_* return errors are ignored in pkg/controller/common/validation.go (lines 29, 37, 58, 68), and ValidateTolerationsConfig passes the result of ToCoreTolerations without any error channel—this can silently validate partially/incorrectly converted data. Propagate these conversion errors back to callers; if changing ToCoreTolerations/ToV1Tolerations to return (…, error), update their call sites in pkg/controller/certmanager/deployment_overrides_validation.go and pkg/controller/certmanager/deployment_helper.go accordingly.
Proposed fix (pattern)
import (
+ "fmt"
corev1 "k8s.io/api/core/v1"
apivalidation "k8s.io/apimachinery/pkg/api/validation"
metav1validation "k8s.io/apimachinery/pkg/apis/meta/v1/validation"
"k8s.io/apimachinery/pkg/util/validation/field"
"k8s.io/kubernetes/pkg/apis/core"
corev1conversion "k8s.io/kubernetes/pkg/apis/core/v1"
corevalidation "k8s.io/kubernetes/pkg/apis/core/validation"
)
@@
func ValidateTolerationsConfig(tolerations []corev1.Toleration, fldPath *field.Path) error {
- return corevalidation.ValidateTolerations(ToCoreTolerations(tolerations), fldPath.Child("tolerations")).ToAggregate()
+ convTolerations, err := ToCoreTolerations(tolerations)
+ if err != nil {
+ return fmt.Errorf("convert tolerations: %w", err)
+ }
+ return corevalidation.ValidateTolerations(convTolerations, fldPath.Child("tolerations")).ToAggregate()
}
@@
func ValidateResourceRequirements(requirements corev1.ResourceRequirements, fldPath *field.Path) error {
var convRequirements core.ResourceRequirements
- _ = corev1conversion.Convert_v1_ResourceRequirements_To_core_ResourceRequirements(&requirements, &convRequirements, nil)
+ if err := corev1conversion.Convert_v1_ResourceRequirements_To_core_ResourceRequirements(&requirements, &convRequirements, nil); err != nil {
+ return fmt.Errorf("convert resource requirements: %w", err)
+ }
return corevalidation.ValidateContainerResourceRequirements(&convRequirements, nil, fldPath.Child("resources"), corevalidation.PodValidationOptions{}).ToAggregate()
}
@@
func ValidateAffinityRules(affinity *corev1.Affinity, fldPath *field.Path) error {
var convAffinity core.Affinity
- _ = corev1conversion.Convert_v1_Affinity_To_core_Affinity(affinity, &convAffinity, nil)
+ if err := corev1conversion.Convert_v1_Affinity_To_core_Affinity(affinity, &convAffinity, nil); err != nil {
+ return fmt.Errorf("convert affinity: %w", err)
+ }
return validateAffinity(&convAffinity, corevalidation.PodValidationOptions{}, fldPath.Child("affinity")).ToAggregate()
}
@@
-func ToCoreTolerations(in []corev1.Toleration) []core.Toleration {
+func ToCoreTolerations(in []corev1.Toleration) ([]core.Toleration, error) {
out := make([]core.Toleration, len(in))
for i := range in {
- _ = corev1conversion.Convert_v1_Toleration_To_core_Toleration(&in[i], &out[i], nil)
+ if err := corev1conversion.Convert_v1_Toleration_To_core_Toleration(&in[i], &out[i], nil); err != nil {
+ return nil, err
+ }
}
- return out
+ return out, nil
}
@@
-func ToV1Tolerations(in []corev1.Toleration) []corev1.Toleration {
+func ToV1Tolerations(in []corev1.Toleration) ([]corev1.Toleration, error) {
out := make([]corev1.Toleration, len(in))
for i := range in {
- _ = corev1conversion.Convert_core_Toleration_To_v1_Toleration(&in[i], &out[i], nil)
+ if err := corev1conversion.Convert_core_Toleration_To_v1_Toleration(&in[i], &out[i], nil); err != nil {
+ return nil, err
+ }
}
- return out
+ return out, nil
}🤖 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.go` around lines 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.
Source: Coding guidelines
There was a problem hiding this comment.
These conversion functions are auto-generated by Kubernetes' conversion-gen and always return nil — they just copy fields between structurally identical types. You can verify by reading the implementations in vendor/k8s.io/kubernetes/pkg/apis/core/v1/zz_generated.conversion.go:
func autoConvert_v1_Toleration_To_core_Toleration(in *corev1.Toleration, out *core.Toleration, s conversion.Scope) error {
out.Key = in.Key
out.Operator = core.TolerationOperator(in.Operator)
out.Value = in.Value
out.Effect = core.TaintEffect(in.Effect)
out.TolerationSeconds = (*int64)(unsafe.Pointer(in.TolerationSeconds))
return nil // always nil
}Same pattern for Convert_v1_Affinity_To_core_Affinity and Convert_v1_ResourceRequirements_To_core_ResourceRequirements — all unconditionally return nil.
Adding if err != nil handling for errors that can never occur would add boilerplate for an impossible code path and obscure the actual logic. The _ = assignment explicitly acknowledges the return value while keeping the code clean.
There was a problem hiding this comment.
@sebrandon1, that's a convincing explanation — thank you for pointing to the actual vendor implementation. Since zz_generated.conversion.go unconditionally returns nil for these field-by-field struct copies, adding if err != nil guards would indeed be dead code that obscures the intent. The _ = idiom cleanly acknowledges the return value without pretending the error path is reachable. I'll withdraw this concern.
✏️ Learnings added
Learnt from: sebrandon1
Repo: openshift/cert-manager-operator PR: 438
File: pkg/controller/common/validation.go:22-23
Timestamp: 2026-06-10T18:07:19.810Z
Learning: In the openshift/cert-manager-operator repository, the Kubernetes auto-generated conversion functions used in `pkg/controller/common/validation.go` (e.g., `corev1conversion.Convert_v1_Toleration_To_core_Toleration`, `Convert_v1_Affinity_To_core_Affinity`, `Convert_v1_ResourceRequirements_To_core_ResourceRequirements`) are defined in `vendor/k8s.io/kubernetes/pkg/apis/core/v1/zz_generated.conversion.go` and unconditionally return nil — they simply copy fields between structurally identical types. Error handling (`if err != nil`) for these calls is unnecessary boilerplate; using `_ =` to explicitly discard the return value is the intentional and idiomatic pattern in this codebase.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
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. |
sebrandon1
force-pushed
the
fix-unsafe-pointer-casts
branch
2 times, most recently
from
2904b5e to
8325b32
Compare
sebrandon1
force-pushed
the
fix-unsafe-pointer-casts
branch
from
8325b32 to
d567c8f
Compare
sebrandon1
force-pushed
the
fix-unsafe-pointer-casts
branch
from
d567c8f to
c051781
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. |
2 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