Skip to content

CM-1038: Replace istiocsr type-specific decoders with generic DecodeObjBytes#418

Open
sebrandon1 wants to merge 1 commit into
openshift:masterfrom
sebrandon1:replace-decoders-with-generic
Open

CM-1038: Replace istiocsr type-specific decoders with generic DecodeObjBytes#418
sebrandon1 wants to merge 1 commit into
openshift:masterfrom
sebrandon1:replace-decoders-with-generic

Conversation

@sebrandon1
Copy link
Copy Markdown
Member

@sebrandon1 sebrandon1 commented May 6, 2026

Summary

  • Remove 8 type-specific decode*ObjBytes functions from the istiocsr package (~95 lines) and replace all 22 call sites with the existing generic common.DecodeObjBytes[T] helper
  • Cache the ServiceAccount name at init time to avoid redundant YAML deserialization on every reconcile cycle
  • Aligns the istiocsr package with the pattern already used by the trustmanager package

Test plan

  • Unit tests pass (go test ./pkg/controller/istiocsr/...)
  • Build succeeds (go build ./pkg/controller/istiocsr/)
  • No new lint issues introduced
  • CI checks pass

Jira: https://redhat.atlassian.net/browse/CM-1038

Summary by CodeRabbit

  • Refactor
    • Unified object decoding across the IstioCSR controller, replacing multiple bespoke decoders with a single shared decoder to simplify resource handling.
    • Introduced an asset-based cache for the service account name to avoid repeated decoding during reconciliation.
    • No changes to public APIs or runtime behavior; existing functionality remains unchanged.

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label May 6, 2026
@openshift-ci-robot
Copy link
Copy Markdown

openshift-ci-robot commented May 6, 2026

@sebrandon1: This pull request references CM-1038 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 task to target the "5.0.0" version, but no target version was set.

Details

In response to this:

Summary

  • Remove 8 type-specific decode*ObjBytes functions from the istiocsr package (~95 lines) and replace all 22 call sites with the existing generic common.DecodeObjBytes[T] helper
  • Cache the ServiceAccount name at init time to avoid redundant YAML deserialization on every reconcile cycle
  • Aligns the istiocsr package with the pattern already used by the trustmanager package

Test plan

  • Unit tests pass (go test ./pkg/controller/istiocsr/...)
  • Build succeeds (go build ./pkg/controller/istiocsr/)
  • No new lint issues introduced
  • CI checks pass

Jira: https://redhat.atlassian.net/browse/CM-1038

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
Copy link
Copy Markdown

coderabbitai Bot commented May 6, 2026

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: 1d65486e-b28b-4de5-8d88-e880fb0cf9c2

📥 Commits

Reviewing files that changed from the base of the PR and between ee01305 and b251d62.

📒 Files selected for processing (7)
  • pkg/controller/istiocsr/certificates.go
  • pkg/controller/istiocsr/deployments.go
  • pkg/controller/istiocsr/rbacs.go
  • pkg/controller/istiocsr/serviceaccounts.go
  • pkg/controller/istiocsr/services.go
  • pkg/controller/istiocsr/test_utils.go
  • pkg/controller/istiocsr/utils.go
🚧 Files skipped from review as they are similar to previous changes (7)
  • pkg/controller/istiocsr/certificates.go
  • pkg/controller/istiocsr/deployments.go
  • pkg/controller/istiocsr/serviceaccounts.go
  • pkg/controller/istiocsr/test_utils.go
  • pkg/controller/istiocsr/services.go
  • pkg/controller/istiocsr/rbacs.go
  • pkg/controller/istiocsr/utils.go

Walkthrough

Replaces many bespoke decode helpers with a single generic common.DecodeObjBytes[T](codecs, SchemeGroupVersion, asset) across the IstioCSR controller; removes the old decode helpers and adds a package-level cached ServiceAccount name initialized from the ServiceAccount asset.

Changes

Decoding Consolidation

Layer / File(s) Summary
Caching foundation
pkg/controller/istiocsr/utils.go
Adds cachedServiceAccountName and initializes it in init() by decoding the embedded ServiceAccount asset via common.DecodeObjBytes[*corev1.ServiceAccount]; removes the previous unexported decode*ObjBytes helper functions.
RBAC decoding & SA usage
pkg/controller/istiocsr/rbacs.go
All RBAC getters (ClusterRole, ClusterRoleBinding, Role, RoleBinding, and lease variants) now use common.DecodeObjBytes[...]; createOrApplyRBACResource uses cachedServiceAccountName rather than decoding the ServiceAccount inline.
ServiceAccount decoding
pkg/controller/istiocsr/serviceaccounts.go
getServiceAccountObject now decodes via common.DecodeObjBytes[*corev1.ServiceAccount](...).
Certificate decoding
pkg/controller/istiocsr/certificates.go
getCertificateObject now uses common.DecodeObjBytes[*certmanagerv1.Certificate](codecs, certmanagerv1.SchemeGroupVersion, assets.MustAsset(...)).
Deployment decoding
pkg/controller/istiocsr/deployments.go
getDeploymentObject switched to common.DecodeObjBytes[*appsv1.Deployment](codecs, appsv1.SchemeGroupVersion, assets.MustAsset(...)).
Service decoding
pkg/controller/istiocsr/services.go
getServiceObject and getMetricsServiceObject now decode Service manifests with common.DecodeObjBytes[*corev1.Service](...).
Test utilities
pkg/controller/istiocsr/test_utils.go
Test helpers updated to use common.DecodeObjBytes[T] for Certificate, ClusterRole, ClusterRoleBinding, Deployment, Role, RoleBinding, RoleLeases, RoleBindingLeases, Service, and ServiceAccount; adds common import.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 14 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (14 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly summarizes the main change: replacing type-specific decoder functions with a generic DecodeObjBytes helper across the istiocsr package.
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 PR does not use Ginkgo tests. The istiocsr package uses standard Go testing package with t.Run(), not Ginkgo. No test names (It, Describe, Context, When) are present to assess.
Test Structure And Quality ✅ Passed PR contains no Ginkgo tests. Repository uses standard Go testing.T framework exclusively. Check for Ginkgo test quality is not applicable to this PR.
Microshift Test Compatibility ✅ Passed No new Ginkgo e2e tests are added in this PR. The PR only refactors existing decoder functions in the istiocsr package to use a generic helper; it contains no test additions.
Single Node Openshift (Sno) Test Compatibility ✅ Passed No new Ginkgo e2e tests added in this PR. Changes are refactoring of production controller code and test utilities, not e2e test definitions. Check is not applicable.
Topology-Aware Scheduling Compatibility ✅ Passed PR refactors internal decoder functions with no changes to deployment manifests or scheduling constraints. IstioCSR Deployment topology-compatible.
Ote Binary Stdout Contract ✅ Passed PR introduces no stdout writes in process-level code; only standard K8s API registration and YAML decoding in init() function.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed This PR refactors controller code and does not add new Ginkgo e2e tests. The custom check is not applicable as it specifically targets new Ginkgo e2e tests, which are absent from this PR.
No-Weak-Crypto ✅ Passed PR refactors decoder functions without introducing weak crypto algorithms, custom implementations, or insecure comparisons. Uses only strong standard library cryptography.
Container-Privileges ✅ Passed PR only refactors istiocsr decoders; no manifests modified. Deployment uses secure settings: allowPrivilegeEscalation: false, dropped capabilities, non-root user, read-only filesystem.
No-Sensitive-Data-In-Logs ✅ Passed No sensitive logging added. PR replaces type-specific decoders with generic one; logs only record resource names/namespaces, DecodeObjBytes only panics with type info.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

@openshift-ci openshift-ci Bot requested review from bharath-b-rh and mytreya-rh May 6, 2026 16:16
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented May 6, 2026

[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 bharath-b-rh 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:

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

@sebrandon1 sebrandon1 force-pushed the replace-decoders-with-generic branch from b622911 to ee01305 Compare May 14, 2026 19:07
@sebrandon1
Copy link
Copy Markdown
Member Author

/retest

…bjBytes

Remove 8 type-specific decode*ObjBytes functions from the istiocsr
package and replace all call sites with the existing generic
common.DecodeObjBytes[T] helper, matching the pattern already used
by the trustmanager package.

Cache the ServiceAccount name at init time to avoid redundant YAML
deserialization on every reconcile cycle.
@sebrandon1 sebrandon1 force-pushed the replace-decoders-with-generic branch from ee01305 to b251d62 Compare May 29, 2026 15:51
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented May 29, 2026

@sebrandon1: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-operator b251d62 link true /test e2e-operator
ci/prow/e2e-operator-tech-preview b251d62 link false /test e2e-operator-tech-preview

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

jira/valid-reference Indicates that this PR references a valid Jira ticket of any type.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants