Prepare types for CRD generation#11622
Conversation
marvin-tigera
added
release-note-required
There was a problem hiding this comment.
Pull request overview
This PR prepares Calico's custom types for CRD generation with a new version of controller-gen that doesn't have patches for numorstring types. The changes add kubebuilder validation markers to numorstring types and update the CRD generation paths to be more specific.
Key Changes:
- Added kubebuilder validation markers (
Type=integer,XIntOrString,Pattern) toUint8OrString,Protocol, andPorttypes to enable proper CRD schema generation - Updated Makefile CRD generation paths from
./lib/apis/...to./lib/apis/crd.projectcalico.org/...to limit scope
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| libcalico-go/Makefile | Narrowed CRD generation paths to specifically target crd.projectcalico.org APIs |
| api/pkg/lib/numorstring/uint8orstring.go | Added kubebuilder validation markers for CRD schema generation |
| api/pkg/lib/numorstring/protocol.go | Added kubebuilder validation markers for CRD schema generation |
| api/pkg/lib/numorstring/port.go | Added kubebuilder validation markers for CRD schema generation |
| // +kubebuilder:validation:Type=integer | ||
| // +kubebuilder:validation:XIntOrString | ||
| // +kubebuilder:validation:Pattern=`^.*` |
There was a problem hiding this comment.
The kubebuilder markers seem contradictory. Type=integer suggests only integer values are valid, but Pattern=^.*$ allows any string. Additionally, XIntOrString is typically used for Kubernetes IntOrString types. Please clarify the intended validation behavior and ensure the markers accurately reflect whether this type accepts integers only, strings only, or both.
| // +kubebuilder:validation:Type=integer | |
| // +kubebuilder:validation:XIntOrString | |
| // +kubebuilder:validation:Pattern=`^.*` | |
| // +kubebuilder:validation:Type=string | |
| // +kubebuilder:validation:XIntOrString |
|
|
||
| // +kubebuilder:validation:Type=integer | ||
| // +kubebuilder:validation:XIntOrString | ||
| // +kubebuilder:validation:Pattern=`^.*` |
There was a problem hiding this comment.
The same marker inconsistency exists here as in Uint8OrString. The combination of Type=integer with Pattern=^.*$ (which matches any string) creates ambiguous validation semantics. Verify these markers correctly represent the type's dual integer/string nature.
| // +kubebuilder:validation:Pattern=`^.*` | |
| // +kubebuilder:validation:Pattern=`^(UDP|TCP|ICMP|ICMPv6|SCTP|UDPLite|[0-9]+)$` |
| // +kubebuilder:validation:Type=integer | ||
| // +kubebuilder:validation:XIntOrString | ||
| // +kubebuilder:validation:Pattern=`^.*` |
There was a problem hiding this comment.
The kubebuilder markers appear inconsistent. Type=integer conflicts with Pattern=^.*$ which accepts any string. For a Port type that can represent port ranges or named ports, clarify whether these markers should validate the struct fields (MinPort, MaxPort, PortName) or if different markers are needed.
| // +kubebuilder:validation:Type=integer | |
| // +kubebuilder:validation:XIntOrString | |
| // +kubebuilder:validation:Pattern=`^.*` |
|
Related toolchain changes in projectcalico/toolchain#754. |
| // PortName to "". | ||
| // - For a single port, set MinPort = MaxPort and PortName = "". | ||
| // | ||
| // +kubebuilder:validation:Type=integer |
There was a problem hiding this comment.
Does this change force the type to be an integer only?
There was a problem hiding this comment.
No, just set type to Integer, and the XIntOrString relax it to accept string as well. I'm using the same for DSCP
| @@ -429,6 +426,7 @@ spec: | |||
| a problem if this range overlaps with the operating systems. Both ends of the range are | |||
| inclusive. [Default: 20000:29999] | |||
| pattern: ^.* | |||
| type: integer | |||
There was a problem hiding this comment.
Are these breaking changes @radixo ? Same question for other places.
| "ParsedDefaultJSON": "0", | ||
| "ParsedType": "numorstring.Port", | ||
| "YAMLType": "integer or string", | ||
| "YAMLType": "integer", |
There was a problem hiding this comment.
The felix doc for YAMLType is affected by the +kubebuilder:validation:Type=integer marker.
hjiawei
force-pushed
the
fix-for-unpatched-controller-gen
branch
from
df2093b to
4efbb3d
Compare
This update builds a new calico/go-build container image that includes the controller-gen tool and removes patches for numorstring types. The changes prepare the Calico repository for the updated toolchain.
hjiawei
force-pushed
the
fix-for-unpatched-controller-gen
branch
from
4efbb3d to
8c47499
Compare
|
This PR is stale because it has been open for 60 days with no activity. |
radixo
added
docs-not-required
marvin-tigera
removed
docs-not-required
|
Doing an alternative solution. |
Description
It will be built a new calico/go-build image with a controller-gen without patches for numorstring types, those changes prepares calico repository to be built with this changes.
Related issues/PRs
Todos
Release Note
Reminder for the reviewer
Make sure that this PR has the correct labels and milestone set.
Every PR needs one
docs-*label.docs-pr-required: This change requires a change to the documentation that has not been completed yet.docs-completed: This change has all necessary documentation completed.docs-not-required: This change has no user-facing impact and requires no docs.Every PR needs one
release-note-*label.release-note-required: This PR has user-facing changes. Most PRs should have this label.release-note-not-required: This PR has no user-facing changes.Other optional labels:
cherry-pick-candidate: This PR should be cherry-picked to an earlier release. For bug fixes only.needs-operator-pr: This PR is related to install and requires a corresponding change to the operator.