investigation: nullable compositeRoles/compositeRolesRefs is an upjet limitation (confirmed through v2.3.0)#586
Conversation
|
@copilot change of generated files is Forbidden. |
…fields When a compositeRolesSelector no longer matches any roles (e.g. after deletion of the referenced role), the reference resolver produces nil for the CompositeRoles and CompositeRolesRefs fields. This causes the server-side apply JSON merge patch to contain "null" for these array fields, which the CRD structural schema rejects with: "spec.forProvider.compositeRoles: Invalid value: "null"" Adding nullable: true to the OpenAPI schema allows null values for these fields, enabling the controller to properly clear them when no references match. Also adds // +nullable markers to the Go types so future controller-gen regeneration preserves the nullable schema property. Closes #425
7003dbd to
405becb
Compare
…ng generated files Reverts direct changes to generated files (zz_role_types.go, CRD YAMLs). Instead, adds: - hack/nullable_override.sh: post-generation script that injects +nullable markers into the generated Go types before controller-gen runs - generate/generate.go: new //go:generate step between upjet and controller-gen that runs the script When `go generate` is run, the script patches CompositeRoles and CompositeRolesRefs fields with +nullable so controller-gen produces CRDs with `nullable: true`, allowing null values in SSA patches. Closes #425
Reverted all generated file changes in commit |
|
@copilot nö. Is it an upjet limitation? Review the source Code of upjet for this. |
After reviewing upjet v2.2.0 source code, confirmed this is an upjet framework limitation. Upjet's marker system (pkg/types/markers/kubebuilder) only supports Required, Minimum, Maximum, and Default options - there is no Nullable marker support. The issue occurs because: 1. APISimpleReferenceResolver uses JSON merge patch with ApplyPatchType 2. When reference resolution produces empty results for slice fields, the merge patch sends null to clear previously-set values 3. CRD structural schema rejects null for array-type fields without nullable: true This needs to be addressed upstream in upjet by adding Nullable support to the kubebuilder marker options.
Reviewed the upjet v2.2.0 source code. Yes, this is an upjet limitation:
The root cause: Reverted the hack script approach in |
|
@copilot Research if this is still a Problem in main / upjet 2.3 |
Confirmed this is still a limitation in upjet v2.3.0 and upjet v2.3.0 / main:
crossplane-runtime (latest
The SSA-related PRs (#301, #308) added |
|
@copilot test against our patched Version https://github.com/Breee/upjet/pull/2/changes if it helps to resolve the issue |
Tested Breee/upjet#2 against this provider. Results: The patch does resolve the issue, but has a marker naming bug:
Root cause: controller-tools registers the marker as
Fix needed in the upjet patch: In m += "+kubebuilder:validation:Nullable\n"to: m += "+nullable\n"After that fix, the full chain works: upjet generates |
|
@copilot make sure you use "make generate". |
Ran
The marker naming bug in Breee/upjet#2 needs to be fixed (emit |
|
@copilot try again |
Re-ran
The patched upjet fully resolves the issue. Once Breee/upjet#2 is merged, this provider can pick it up and regenerate. |
When a
compositeRolesSelectorpreviously matched roles but those roles are deleted, the reference resolver producesnilforCompositeRolesandCompositeRolesRefs. The managed reconciler's JSON Merge Patch (RFC 7396) then emits"compositeRoles": nullto clear the field, which the CRD rejects because the schema only allowstype: array.Investigation: Upjet Source Code Review
Reviewed upjet v2.2.0, v2.3.0, and main branch source code and confirmed this is an upjet framework limitation that persists across all current versions:
pkg/types/markers/kubebuilder/kubebuilder.go— theOptionsstruct only supportsRequired,Minimum,Maximum, andDefault. There is noNullableoption. Unchanged in v2.3.0 and main.pkg/types/reference.go—generateReferenceFields()creates slice reference fields with only+kubebuilder:validation:Optional, no+nullablemarker. Unchanged in v2.3.0 and main.pkg/config/resource.go—SchemaElementOptionandInitProviderOverridesdon't expose any mechanism to add custom markers to generated fields.pkg/controller/proposed_state.go.crossplane/upjetaddress this limitation.The SSA-related PRs (upjet #301, #308) added
+listType/+mapType/+structTypemarkers for merge strategy configuration, but did not address the+nullablemarker needed for this scenario.Root Cause
The crossplane-runtime
APISimpleReferenceResolver(still present in latestmain) computes a JSON merge patch between pre- and post-resolution states usingApplyPatchType. When resolution yields nil for a previously-set slice (selector matches nothing after referenced roles are deleted), the merge patch emitsnullto clear the field. Withoutnullable: truein the CRD structural schema, this valid patch operation is rejected.Testing: Breee/upjet#2 Patched Version
Tested the patched upjet from Breee/upjet#2 against this provider using
make generate:Initial test (
@cf2199d) — identified a marker naming bug:make generatecompletes successfully ✅CompositeRolesSelectorunchanged ✅nullable: true❌ — because the patch emitted+kubebuilder:validation:Nullablebut controller-tools only recognizes+nullableRe-test after fix (
@1f0bc5f) — fully resolves the issue:make generatecompletes successfully ✅+nullable(correct marker name) on slice reference fields across 20 files ✅CompositeRolesSelectorcorrectly left without nullable ✅nullable: trueoncompositeRolesandcompositeRolesRefsin both forProvider/initProvider (and status for compositeRoles), across both cluster-scoped and namespaced CRDs ✅Conclusion
The approach in Breee/upjet#2 is correct and fully resolves the issue after the marker name fix. Once the upstream upjet PR is merged, this provider can pick it up and regenerate to produce CRDs that accept
nullfrom merge patches when reference resolution clears a slice field. The limitation is confirmed present in upjet v2.2.0, v2.3.0, and current main. Previous hack script approach has been reverted.