Skip to content

CM-1113: Replace unsafe.Pointer casts with Kubernetes conversion functions #438

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.

Sign up for GitHub

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

Open
sebrandon1 wants to merge 1 commit into
base: master
Choose a base branch
from
+35 −27
Open

CM-1113: Replace unsafe.Pointer casts with Kubernetes conversion functions #438

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 6 additions & 11 deletions pkg/controller/certmanager/deployment_helper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,13 @@ import (
"fmt"
"sort"
"strings"
"unsafe"

corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/kubernetes/pkg/apis/core"
"k8s.io/kubernetes/pkg/util/tolerations"

"github.com/openshift/cert-manager-operator/api/operator/v1alpha1"
"github.com/openshift/cert-manager-operator/pkg/controller/common"
certmanagerinformer "github.com/openshift/cert-manager-operator/pkg/operator/informers/externalversions/operator/v1alpha1"
)

Expand Down Expand Up @@ -116,15 +115,11 @@ func mergePodScheduling(sourceScheduling v1alpha1.CertManagerScheduling, overrid
// Merge the source and override NodeSelector.
mergedNodeSelector := labels.Merge(sourceScheduling.NodeSelector, overrideScheduling.NodeSelector)

// Convert corev1.Tolerations to core.Tolerations.
sourceTolerations := *(*[]core.Toleration)(unsafe.Pointer(&sourceScheduling.Tolerations))
overridingTolerations := *(*[]core.Toleration)(unsafe.Pointer(&overrideScheduling.Tolerations))

// Merge the source and override Tolerations.
mergedCoreTolerations := tolerations.MergeTolerations(sourceTolerations, overridingTolerations)

// Convert core.Tolerations to corev1.Tolerations.
mergedCorev1Tolerations := *(*[]corev1.Toleration)(unsafe.Pointer(&mergedCoreTolerations))
mergedCoreTolerations := tolerations.MergeTolerations(
common.ToCoreTolerations(sourceScheduling.Tolerations),
common.ToCoreTolerations(overrideScheduling.Tolerations),
)
mergedCorev1Tolerations := common.ToV1Tolerations(mergedCoreTolerations)

return v1alpha1.CertManagerScheduling{
NodeSelector: mergedNodeSelector,
Expand Down
8 changes: 2 additions & 6 deletions pkg/controller/certmanager/deployment_overrides_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,20 +2,19 @@ package certmanager

import (
"fmt"
"unsafe"

appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
metav1validation "k8s.io/apimachinery/pkg/apis/meta/v1/validation"
utilerrors "k8s.io/apimachinery/pkg/util/errors"
"k8s.io/apimachinery/pkg/util/validation/field"
"k8s.io/kubernetes/pkg/apis/core"
corevalidation "k8s.io/kubernetes/pkg/apis/core/validation"
"k8s.io/utils/strings/slices"

operatorv1 "github.com/openshift/api/operator/v1"

"github.com/openshift/cert-manager-operator/api/operator/v1alpha1"
"github.com/openshift/cert-manager-operator/pkg/controller/common"
certmanagerinformer "github.com/openshift/cert-manager-operator/pkg/operator/informers/externalversions/operator/v1alpha1"
)

Expand Down Expand Up @@ -300,10 +299,7 @@ func withPodSchedulingValidateHook(certmanagerinformer certmanagerinformer.CertM
func validateScheduling(scheduling v1alpha1.CertManagerScheduling, fldPath *field.Path) error {
errs := metav1validation.ValidateLabels(scheduling.NodeSelector, fldPath.Child("nodeSelector"))

// Convert corev1.Tolerations to core.Tolerations.
tolerations := *(*[]core.Toleration)(unsafe.Pointer(&scheduling.Tolerations))

errs = append(errs, corevalidation.ValidateTolerations(tolerations, fldPath.Child("tolerations"))...)
errs = append(errs, corevalidation.ValidateTolerations(common.ToCoreTolerations(scheduling.Tolerations), fldPath.Child("tolerations"))...)

return errs.ToAggregate()
}
37 changes: 27 additions & 10 deletions pkg/controller/common/validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,12 @@
package common

import (
"unsafe"

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"
)

Expand All @@ -20,25 +19,23 @@ func ValidateNodeSelectorConfig(nodeSelector map[string]string, fldPath *field.P
// ValidateTolerationsConfig validates the Tolerations configuration using
// the Kubernetes core toleration validation rules.
func ValidateTolerationsConfig(tolerations []corev1.Toleration, fldPath *field.Path) error {
// convert corev1.Tolerations to core.Tolerations, required for validation.
convTolerations := *(*[]core.Toleration)(unsafe.Pointer(&tolerations))
return corevalidation.ValidateTolerations(convTolerations, fldPath.Child("tolerations")).ToAggregate()
return corevalidation.ValidateTolerations(ToCoreTolerations(tolerations), fldPath.Child("tolerations")).ToAggregate()
}
Comment on lines +22 to 23

@coderabbitai coderabbitai Bot Jun 10, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 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" || true

Repository: 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" || true

Repository: 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\(" . || true

Repository: 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.go

Repository: 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

@sebrandon1 sebrandon1 Jun 10, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@coderabbitai coderabbitai Bot Jun 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.


// ValidateResourceRequirements validates the ResourceRequirements configuration
// using the Kubernetes core resource requirements validation rules.
func ValidateResourceRequirements(requirements corev1.ResourceRequirements, fldPath *field.Path) error {
// convert corev1.ResourceRequirements to core.ResourceRequirements, required for validation.
convRequirements := *(*core.ResourceRequirements)(unsafe.Pointer(&requirements))
var convRequirements core.ResourceRequirements
_ = corev1conversion.Convert_v1_ResourceRequirements_To_core_ResourceRequirements(&requirements, &convRequirements, nil)
return corevalidation.ValidateContainerResourceRequirements(&convRequirements, nil, fldPath.Child("resources"), corevalidation.PodValidationOptions{}).ToAggregate()
}

// ValidateAffinityRules validates the Affinity configuration using
// the Kubernetes core affinity validation rules.
func ValidateAffinityRules(affinity *corev1.Affinity, fldPath *field.Path) error {
// convert corev1.Affinity to core.Affinity, required for validation.
convAffinity := (*core.Affinity)(unsafe.Pointer(affinity))
return validateAffinity(convAffinity, corevalidation.PodValidationOptions{}, fldPath.Child("affinity")).ToAggregate()
var convAffinity core.Affinity
_ = corev1conversion.Convert_v1_Affinity_To_core_Affinity(affinity, &convAffinity, nil)
return validateAffinity(&convAffinity, corevalidation.PodValidationOptions{}, fldPath.Child("affinity")).ToAggregate()
}

// ValidateLabelsConfig validates label keys and values using the Kubernetes
Expand All @@ -52,3 +49,23 @@ func ValidateLabelsConfig(labels map[string]string, fldPath *field.Path) error {
func ValidateAnnotationsConfig(annotations map[string]string, fldPath *field.Path) error {
return apivalidation.ValidateAnnotations(annotations, fldPath.Child("annotations")).ToAggregate()
}

// ToCoreTolerations converts a slice of corev1.Toleration to core.Toleration
// using Kubernetes' auto-generated conversion functions.
func ToCoreTolerations(in []corev1.Toleration) []core.Toleration {
out := make([]core.Toleration, len(in))
for i := range in {
_ = corev1conversion.Convert_v1_Toleration_To_core_Toleration(&in[i], &out[i], nil)
}
return out
}

// ToV1Tolerations converts a slice of core.Toleration to corev1.Toleration
// using Kubernetes' auto-generated conversion functions.
func ToV1Tolerations(in []core.Toleration) []corev1.Toleration {
out := make([]corev1.Toleration, len(in))
for i := range in {
_ = corev1conversion.Convert_core_Toleration_To_v1_Toleration(&in[i], &out[i], nil)
}
return out
}