migrate CRD generation to controller-gen and automate Helm RBAC sync#2269
Conversation
There was a problem hiding this comment.
@kahirokunn: 0 warnings.
Details
In response to this:
Fixes #
Proposed Changes
Release Note
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.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2269 +/- ##
==========================================
+ Coverage 63.97% 64.58% +0.60%
==========================================
Files 51 51
Lines 1960 1999 +39
==========================================
+ Hits 1254 1291 +37
- Misses 605 606 +1
- Partials 101 102 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
64fda73 to
51abee7
Compare
51abee7 to
552a8a9
Compare
552a8a9 to
cf94405
Compare
3b7b007 to
baf6c99
Compare
786194b to
4582245
Compare
houshengbo
left a comment
There was a problem hiding this comment.
Thanks for this PR — the goal of migrating to controller-gen and automating Helm sync is valuable. However, the generated CRD schema introduces backward-incompatible changes that will break existing user CRs on upgrade. The CRD schema must not change field names, remove existing fields, or change field types.
Specific blockers:
-
Istio TLS field renames (12 fields): The Istio proto Go types use snake_case JSON tags (
json:"credential_name"), so controller-gen generates snake_case field names. The existing CRD uses camelCase (credentialName,httpsRedirect, etc.). This rename breaks any existing CR that uses these fields. Additionally,tls.modechanges fromstringtoint32. -
Ghost fields removed:
workloads[].versionandworkloads[].volumeMountsexist in the current CRD but have no backing Go struct fields. controller-gen drops them. Existing CRs that set these fields will fail validation.
Suggested approach: Create operator-local wrapper types for the Istio Server/Port/ServerTLSSettings structs with camelCase JSON tags matching the existing schema, and add the ghost fields to WorkloadOverride with deprecation comments. This preserves backward compatibility while achieving the controller-gen automation goal.
See inline comments for details.
houshengbo
left a comment
There was a problem hiding this comment.
A few additional non-blocking suggestions below.
19b1507 to
f9f7647
Compare
Signed-off-by: kahirokunn <okinakahiro@gmail.com>
Signed-off-by: kahirokunn <okinakahiro@gmail.com>
Signed-off-by: kahirokunn <okinakahiro@gmail.com>
Signed-off-by: kahirokunn <okinakahiro@gmail.com>
f9f7647 to
956e019
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: houshengbo, kahirokunn The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Changes
controller-genauto-generation from Go typesconfig/crd/bases/hack/sync-helm-crds.shto auto-sync Helm chart CRDs from controller-gen output(split out from
operator.yaml)hack/sync-helm-rbac.shto auto-sync Helm chart RBAC ClusterRoles fromconfig/rbac/role.yamlinto separate per-operator files(
rbac/serving-operator-role.yaml,rbac/eventing-operator-role.yaml)hack/update-codegen.shandhack/verify-codegen.shWhy
The old hand-maintained CRDs had diverged from Go type definitions:
credentialName) but Go struct JSON tags usesnake_case (
credential_name); values were silently ignored on deserializationworkloads[].version,workloads[].volumeMountsexisted in CRD but had no backing Gostruct fields. Setting them had no effect.
tls.modewasstringin CRD butint32in GoSimilarly, the Helm chart's
operator.yamlcontained hand-maintained ClusterRole definitionsthat could diverge from
config/rbac/role.yaml.Generating from Go types with controller-gen and syncing from a single source of truth
eliminates these inconsistencies and prevents future drift.
Release Note