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

[sebrandon1]

Summary

• Replaces all 8 raw unsafe.Pointer casts between corev1.* and core.* types with Kubernetes auto-generated conversion functions from k8s.io/kubernetes/pkg/apis/core/v1
• Removes "unsafe" import from 3 files
• Adds ToCoreTolerations and ToV1Tolerations helpers in pkg/controller/common/validation.go to avoid repeating the conversion loop pattern

Why

The operator uses Kubernetes internal validation functions (corevalidation.ValidateTolerations, etc.) which require internal core.* types, but the public API uses corev1.* types. Previously, this was bridged with raw unsafe.Pointer casts like:

convTolerations := *(*[]core.Toleration)(unsafe.Pointer(&tolerations))

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.Pointer internally, but the cast is encapsulated, upstream-maintained, and follows the official Kubernetes conversion pattern.

Files changed

File	Before	After
pkg/controller/common/validation.go	3 unsafe casts	Conversion functions + exported helpers
pkg/controller/certmanager/deployment_helper.go	4 unsafe casts (3 in, 1 out)	common.ToCoreTolerations / common.ToV1Tolerations
pkg/controller/certmanager/deployment_overrides_validation.go	1 unsafe cast	common.ToCoreTolerations

Test plan

• go build ./... compiles cleanly
• make test passes
• make lint introduces no new issues
• CI e2e tests pass

Summary by CodeRabbit

• Refactor

  • Updated pod scheduling tolerations merging and deployment override scheduling validation to use safe, standardized tolerations conversions.
  • Introduced shared conversion helpers to keep tolerations handling consistent across validation and merging.
  • Switched tolerations, affinity, and resource requirements validation to rely on official Kubernetes conversion utilities instead of unsafe casting.

• Tests

  • Added unit tests to verify tolerations conversion round-trips and nil/empty input handling.
  • Added a test ensuring a custom TLS profile with missing custom settings returns an error.

[openshift-ci-robot]

@sebrandon1: This pull request explicitly references no jira issue.

Details

In response to this:

Summary

• Replaces all 8 raw unsafe.Pointer casts between corev1.* and core.* types with Kubernetes auto-generated conversion functions from k8s.io/kubernetes/pkg/apis/core/v1
• Removes "unsafe" import from 3 files
• Adds ToCoreTolerations and ToV1Tolerations helpers in pkg/controller/common/validation.go to avoid repeating the conversion loop pattern

Why

The operator uses Kubernetes internal validation functions (corevalidation.ValidateTolerations, etc.) which require internal core.* types, but the public API uses corev1.* types. Previously, this was bridged with raw unsafe.Pointer casts like:

convTolerations := *(*[]core.Toleration)(unsafe.Pointer(&tolerations))

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.Pointer internally, but the cast is encapsulated, upstream-maintained, and follows the official Kubernetes conversion pattern.

Files changed

File	Before	After
pkg/controller/common/validation.go	3 unsafe casts	Conversion functions + exported helpers
pkg/controller/certmanager/deployment_helper.go	4 unsafe casts (3 in, 1 out)	common.ToCoreTolerations / common.ToV1Tolerations
pkg/controller/certmanager/deployment_overrides_validation.go	1 unsafe cast	common.ToCoreTolerations

Test plan

• go build ./... compiles cleanly
• make test passes
• make lint introduces no new issues
• CI e2e tests pass

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.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: eee87ff6-8ba1-45ad-8405-2d6326640ff8

📥 Commits

Reviewing files that changed from the base of the PR and between 5140dd7 and dbb614e.

📒 Files selected for processing (5)

• pkg/controller/certmanager/deployment_helper.go
• pkg/controller/certmanager/deployment_overrides_validation.go
• pkg/controller/common/validation.go
• pkg/controller/common/validation_test.go
• pkg/tlsprofile/tlsprofile_test.go

🚧 Files skipped from review as they are similar to previous changes (2)

• pkg/controller/certmanager/deployment_helper.go
• pkg/controller/common/validation_test.go

---

Walkthrough

This PR replaces unsafe Kubernetes type reinterpretation with explicit conversion helpers. Shared validation and certmanager scheduling paths use the conversions, with tests for toleration round trips, empty inputs, and missing TLS custom settings.

Changes

Safe Kubernetes conversions

Layer / File(s)	Summary
Conversion helpers and shared validation pkg/controller/common/validation.go, pkg/controller/common/validation_test.go	Replaces unsafe conversions for tolerations, resource requirements, and affinity with explicit Kubernetes conversions, adds toleration helpers, and tests round-trip and empty inputs.
Certmanager scheduling integration pkg/controller/certmanager/deployment_helper.go, pkg/controller/certmanager/deployment_overrides_validation.go	Converts tolerations explicitly before merging and validating certmanager scheduling configuration.

TLS profile validation coverage

Layer / File(s)	Summary
Missing custom settings validation pkg/tlsprofile/tlsprofile_test.go	Verifies that a custom TLS profile without custom settings returns the expected error.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Suggested reviewers: swghosh, bharath-b-rh, chiragkyal

🚥 Pre-merge checks | ✅ 15

✅ Passed checks (15 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: replacing unsafe.Pointer casts with Kubernetes conversion functions.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names	✅ Passed	The added/updated tests use static Go test function names only; no Ginkgo It/Describe/Context/When titles or dynamic values appear in the changed files.
Test Structure And Quality	✅ Passed	PASS: The new tests are focused unit tests; they create no resources, use no waits/timeouts, and the assertions stay within single scenarios and repo style.
Microshift Test Compatibility	✅ Passed	Only Go unit tests were added; no Ginkgo e2e tests or MicroShift-unsupported APIs/features were introduced.
Single Node Openshift (Sno) Test Compatibility	✅ Passed	Only unit tests were added; no Ginkgo e2e specs or multi-node/HA assumptions appear in the changed files.
Topology-Aware Scheduling Compatibility	✅ Passed	Only toleration conversion/validation code changed; no new node selectors, affinity, spread constraints, replica logic, or topology assumptions were added.
Ote Binary Stdout Contract	✅ Passed	Touched files are helper/tests only; no main/init/TestMain/BeforeSuite code or stdout-printing calls were added.
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	PASS: The PR only adds unit tests (testing.T), not Ginkgo e2e tests, and no IPv4 literals or external-network calls were found.
No-Weak-Crypto	✅ Passed	Diff only replaces unsafe.Pointer casts and adds toleration/TLS tests; no MD5/SHA1/DES/RC4/3DES/Blowfish/ECB, custom crypto, or secret compares found.
Container-Privileges	✅ Passed	PR only changes Go conversion/validation code; no container/K8s manifests were modified and no privilege-related fields were added.
No-Sensitive-Data-In-Logs	✅ Passed	No new logging statements were added in the changed files; only validation/error returns and test assertions appear, with no sensitive data emitted to logs.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sebrandon1
Once this PR has been reviewed and has the lgtm label, please assign swghosh for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[coderabbitai]

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

📥 Commits

Reviewing files that changed from the base of the PR and between db88f80 and e2a2be8.

📒 Files selected for processing (3)

• pkg/controller/certmanager/deployment_helper.go
• pkg/controller/certmanager/deployment_overrides_validation.go
• pkg/controller/common/validation.go

[openshift-ci-robot]

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

Details

In response to this:

Summary

• Replaces all 8 raw unsafe.Pointer casts between corev1.* and core.* types with Kubernetes auto-generated conversion functions from k8s.io/kubernetes/pkg/apis/core/v1
• Removes "unsafe" import from 3 files
• Adds ToCoreTolerations and ToV1Tolerations helpers in pkg/controller/common/validation.go to avoid repeating the conversion loop pattern

Why

The operator uses Kubernetes internal validation functions (corevalidation.ValidateTolerations, etc.) which require internal core.* types, but the public API uses corev1.* types. Previously, this was bridged with raw unsafe.Pointer casts like:

convTolerations := *(*[]core.Toleration)(unsafe.Pointer(&tolerations))

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.Pointer internally, but the cast is encapsulated, upstream-maintained, and follows the official Kubernetes conversion pattern.

Files changed

File	Before	After
pkg/controller/common/validation.go	3 unsafe casts	Conversion functions + exported helpers
pkg/controller/certmanager/deployment_helper.go	4 unsafe casts (3 in, 1 out)	common.ToCoreTolerations / common.ToV1Tolerations
pkg/controller/certmanager/deployment_overrides_validation.go	1 unsafe cast	common.ToCoreTolerations

Test plan

• go build ./... compiles cleanly
• make test passes
• make lint introduces no new issues
• CI e2e tests pass

Summary by CodeRabbit

• Refactor
• Improved internal type conversion handling for pod scheduling tolerations and resource requirements validation.

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]

@coderabbitai resume

[coderabbitai]

✅ Action performed

Reviews resumed.

[coderabbitai]

🧹 Nitpick comments (1)

pkg/controller/common/validation_test.go (1)

23-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider adding empty-slice coverage for ToV1Tolerations.

TestToCoreTolerations_Empty only exercises ToCoreTolerations with an empty slice. Adding the symmetric ToV1Tolerations case 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

📥 Commits

Reviewing files that changed from the base of the PR and between 1d984a0 and 5140dd7.

📒 Files selected for processing (2)

• pkg/controller/common/validation_test.go
• pkg/tlsprofile/tlsprofile_test.go

[openshift-ci]

@sebrandon1: all tests passed!

Full PR test history. Your PR dashboard.

Details

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.