Repository navigation
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: kvaps The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
746113f to
be3af7c
Compare
…st GVK This commit refactors the apiserver REST handlers to use typed objects (appsv1alpha1.Application) instead of unstructured.Unstructured, eliminating the need for runtime conversions and simplifying the codebase. Additionally, it fixes an issue where UnstructuredList objects were using the first registered kind from typeToGVK instead of the kind from the object's field when multiple kinds are registered with the same Go type. This is a more comprehensive fix for the problem addressed in #1630, which was reverted in #1677. The fix includes the upstream fix from kubernetes/kubernetes#135537, which enables short-circuit path for UnstructuredList similar to regular Unstructured objects, using GVK from the object field instead of typeToGVK. Changes: - Refactored rest.go handlers to use typed Application objects - Removed unstructured.Unstructured conversions - Fixed UnstructuredList GVK handling - Updated dependencies in go.mod/go.sum - Added e2e test for OpenAPI validation - Updated Dockerfile Signed-off-by: Andrei Kvapil <[email protected]>
…st GVK This commit refactors the apiserver REST handlers to use typed objects (appsv1alpha1.Application) instead of unstructured.Unstructured, eliminating the need for runtime conversions and simplifying the codebase. Additionally, it fixes an issue where UnstructuredList objects were using the first registered kind from typeToGVK instead of the kind from the object's field when multiple kinds are registered with the same Go type. This is a more comprehensive fix for the problem addressed in #1630, which was reverted in #1677. The fix includes the upstream fix from kubernetes/kubernetes#135537, which enables short-circuit path for UnstructuredList similar to regular Unstructured objects, using GVK from the object field instead of typeToGVK. Changes: - Refactored rest.go handlers to use typed Application objects - Removed unstructured.Unstructured conversions - Fixed UnstructuredList GVK handling - Updated dependencies in go.mod/go.sum - Added e2e test for OpenAPI validation - Updated Dockerfile Signed-off-by: Andrei Kvapil <[email protected]>
be3af7c to
88775b8
Compare
…st GVK This commit refactors the apiserver REST handlers to use typed objects (appsv1alpha1.Application) instead of unstructured.Unstructured, eliminating the need for runtime conversions and simplifying the codebase. Additionally, it fixes an issue where UnstructuredList objects were using the first registered kind from typeToGVK instead of the kind from the object's field when multiple kinds are registered with the same Go type. This is a more comprehensive fix for the problem addressed in #1630, which was reverted in #1677. The fix includes the upstream fix from kubernetes/kubernetes#135537, which enables short-circuit path for UnstructuredList similar to regular Unstructured objects, using GVK from the object field instead of typeToGVK. Changes: - Refactored rest.go handlers to use typed Application objects - Removed unstructured.Unstructured conversions - Fixed UnstructuredList GVK handling - Updated dependencies in go.mod/go.sum - Added e2e test for OpenAPI validation - Updated Dockerfile Signed-off-by: Andrei Kvapil <[email protected]>
…st GVK This commit refactors the apiserver REST handlers to use typed objects (appsv1alpha1.Application) instead of unstructured.Unstructured, eliminating the need for runtime conversions and simplifying the codebase. Additionally, it fixes an issue where UnstructuredList objects were using the first registered kind from typeToGVK instead of the kind from the object's field when multiple kinds are registered with the same Go type. This is a more comprehensive fix for the problem addressed in #1630, which was reverted in #1677. The fix includes the upstream fix from kubernetes/kubernetes#135537, which enables short-circuit path for UnstructuredList similar to regular Unstructured objects, using GVK from the object field instead of typeToGVK. Changes: - Refactored rest.go handlers to use typed Application objects - Removed unstructured.Unstructured conversions - Fixed UnstructuredList GVK handling - Updated dependencies in go.mod/go.sum - Added e2e test for OpenAPI validation - Updated Dockerfile Signed-off-by: Andrei Kvapil <[email protected]>
|
/test pull-kubernetes-e2e-gce |
|
/triage accepted |
|
/cc @liggitt |
…ed kind When multiple kinds are registered with the same Go type using AddKnownTypeWithName, ConvertToVersion was always selecting the first registered kind from typeToGVK instead of using the actual GVK from the object's field. This causes incorrect behavior in dynamic API servers where the same Go type is registered multiple times with different kinds. Reorder the kinds slice so the object's group/kind is preferred while still passing the full list to KindForGroupVersionKinds for priority decisions. Fall back to the original order when the chosen GVK is not in the registered list (e.g., InternalGroupVersioner constructs a fallback from the first element). Signed-off-by: Andrei Kvapil <[email protected]>
29a8fe5 to
cad151d
Compare
|
Hey @dims Done, I added an AI disclosure to the PR description. |
|
/retest |
1 similar comment
|
/retest |
|
Hi @liggitt, friendly nudge on this one. Should I join an upcoming SIG API Machinery meeting to walk through this fix, or is there another way that would help move it forward? Thanks! |
neolit123
left a comment
There was a problem hiding this comment.
it might regress some users who were relying on the existing behavior for the same go structs. did you grep to check if we have some of that in k/k?
performance wise it can be made close to no impact on existing setups that don't do it, so that's at least nice.
| var exactMatch []schema.GroupVersionKind | ||
| var groupKindMatch []schema.GroupVersionKind | ||
| var rest []schema.GroupVersionKind |
There was a problem hiding this comment.
| var exactMatch []schema.GroupVersionKind | |
| var groupKindMatch []schema.GroupVersionKind | |
| var rest []schema.GroupVersionKind | |
| var exactMatch, groupKindMatch, rest []schema.GroupVersionKind |
There was a problem hiding this comment.
Rewrote this block, see below
| // constructed a fallback GVK from the first element (e.g., InternalGroupVersioner). | ||
| // Reordering can change which element is first, producing an incorrect GVK. | ||
| // Retry with the original order to get the correct fallback. | ||
| if &kinds[0] != &originalKinds[0] { |
There was a problem hiding this comment.
much cleaner if there is reordered bool var.
| objGVK := in.GetObjectKind().GroupVersionKind() | ||
| if len(objGVK.Kind) > 0 && len(objGVK.Version) > 0 && len(kinds) > 1 { |
There was a problem hiding this comment.
maybe:
| objGVK := in.GetObjectKind().GroupVersionKind() | |
| if len(objGVK.Kind) > 0 && len(objGVK.Version) > 0 && len(kinds) > 1 { | |
| if len(kinds) > 1 { | |
| objGVK := in.GetObjectKind().GroupVersionKind() | |
| if len(objGVK.Version) > 0 && len(objGVK.Kind) > 0 { |
| var exactMatch []schema.GroupVersionKind | ||
| var groupKindMatch []schema.GroupVersionKind | ||
| var rest []schema.GroupVersionKind | ||
| for _, k := range kinds { | ||
| switch { | ||
| case k == objGVK: | ||
| exactMatch = append(exactMatch, k) | ||
| case k.Group == objGVK.Group && k.Kind == objGVK.Kind: | ||
| groupKindMatch = append(groupKindMatch, k) | ||
| default: | ||
| rest = append(rest, k) | ||
| } | ||
| } | ||
| if len(exactMatch) > 0 || len(groupKindMatch) > 0 { | ||
| reordered = append(reordered, exactMatch...) | ||
| reordered = append(reordered, groupKindMatch...) | ||
| reordered = append(reordered, rest...) | ||
| kinds = reordered | ||
| } |
There was a problem hiding this comment.
...but this does do a number of allocations.
instead there could be a single allocation:
reordered := make([]schema.GroupVersionKind, 0, len(kinds))
then a copy() and e..g. a slices.SortStableFunc that compares each slice A and B elements against the object GVs Group/Kind and determines the A / B element order.
There was a problem hiding this comment.
Done, slices.Clone + SortStableFunc. Stable sort keeps registration order within buckets
|
/cc |
|
/assign @benluddy |
|
We discussed this issue, in the Aug 5th SIG Meeting, and there was a general agreement that needs to be fixed and that is trending in the right direction.
|
I'm on it, just taking some time to better understand other API implications of registering multiple kinds per type. I'll sync with @liggitt and follow up next week. |
Address review feedback: hoist the cheap len(kinds) check before reading the object's GVK, replace the three bucket slices with a single cloned slice ordered by a stable sort, and track the reorder with a boolean instead of comparing slice element addresses. Signed-off-by: Andrei Kvapil <[email protected]>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: kvaps The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Yes, grepped. The only non-test multi-kind registration in k/k is metav1.WatchEvent, and its GetObjectKind() returns EmptyObjectKind, so the reorder never triggers for it. The rest are fake/test schemes (dynamic/fake, metadata/fake, kubectl testing), their suites pass unchanged. Typed in-tree objects carry empty TypeMeta, so nothing changes for them either. |
|
The endpoint handlers appear to have the correct (singular) GVK on the request scope, but they discard the kind just before serializing response objects: The GVK on the request scope is populated from the REST storage's GroupVersionKindProvider implementation: kubernetes/staging/src/k8s.io/apiserver/pkg/endpoints/installer.go Lines 260 to 263 in 52ba901 A GroupVersionKindProvider implementation seems to be effectively required today, in order to avoid the same class of problem in discovery and OpenAPI stemming from multiple registered GVKs per kind. If we pass a GroupVersioner implementation that recognizes the expected kind, instead of one that picks the first matching GV, does that address the same use case without changing Scheme's conversion behavior? I agree that it's not good that conversion fails to take the hint when apiVersion and kind are populated on the object itself, but I would like to solve this issue without requiring apiVersion and kind to be populated on every object. From what I've seen, the expectations around those fields are inconsistent across the codebase. I took a stab at |
…3784) ## What this PR does Brings `GOVERNANCE.md` in line with the organisation it describes: what the project governs, and what it ships but does not. ### Sub-projects The `Code Repositories` list named seven repositories and was wrong in both directions. Five of the six sub-projects whose code ships in the distribution were missing from it — `etcd-operator`, `keycloak-kms-proxy`, `local-ccm`, `cozystack-scheduler` and `ingress-nginx-with-protobuf-exporter` — while `talos-bootstrap` and `talos-meta-tool` were listed although an adopter installs neither. Nothing stated what being on the list meant, which is why it drifted. It is replaced by a section organised around the distinction that decides scope: whether a repository's output reaches the artifact an adopter installs. Packaged sub-projects get a table naming what each one contributes, its stability, and its owners — the same handles each repository's `CODEOWNERS` already carries. Independent tooling, project infrastructure and upstream forks are described in a line each, as explicitly outside that scope. Three things an evaluator finds anyway are stated rather than left to be discovered: that one fork (`apimachinery`) is compiled into the product, why, and that the fix is proposed upstream as kubernetes/kubernetes#135537; that four repositories are private, with the reason for each, and that none of them contributes to the distribution; and that telemetry is received by a component adopters do not install, with a link to the opt-out. ### Third-party Dependencies Writes down which shipped components the project does not govern. Cozystack vendors software it does not own, which is ordinary and needs no defence — but the boundary was nowhere stated. This is a Required item on the Incubation checklist ("All project metadata and resources are vendor-neutral"), and an evaluator currently has nothing in the governance document to read it off. The section says two things: what the arrangement is, and where the components are listed. It names nothing itself. The [Licenses](https://cozystack.io/docs/v1.6/operations/configuration/licenses/) page carries the list, and by its own framing carries upstreams only — Cozystack-maintained components are Apache-2.0 and are not listed there individually. Appearing on that page is therefore already the classification, so a second list here would only drift from it. Two components that belong on that page were missing from it despite shipping since v1.3.4 and v1.4.3; cozystack/website#649 adds them. Nothing moves, nothing is relicensed, no mirroring changes. This documents the existing arrangement, which each package's `values.yaml` already describes at its own level. ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` against the diff. It touches `GOVERNANCE.md` only, and no trigger concerns governance documents — the website triggers are package additions, platform values, variants, components, release assets, `ApplicationDefinition` semantics, the Talos pin and developer tooling. The website PR linked above is a companion, not a consequence of this diff: it fixes a pre-existing gap on the Licenses page that stands on its own. - [x] No downstream repository is affected by this change ### Release note ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added governance guidance for project sub-components, including ownership, licensing, maintenance, and repository classifications. * Documented project infrastructure, upstream forks, private repositories, and telemetry services. * Clarified how third-party software is sourced, governed, mirrored, and versioned. * Linked to a license inventory listing third-party components and their applicable licenses. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
@benluddy I tested your POC against the real use case, and the short answer to your question is yes — it solves it without touching Our apiserver registers a single Go type under about 27 kinds in one GroupVersion. On stock 0.35 both a GET and a LIST come back with the wrong kind, whichever one the scheme happens to return first. With your POC the GET is correct. Lists were still wrong, because The important part is that all of this ran on upstream apimachinery, so if your approach lands we can drop the fork we carry today and this PR is no longer needed for us. I am happy for #141344 to supersede it. One practical note: your POC applies cleanly to the v0.35 staging module, so backporting it is realistic if anyone needs it before 1.38. And if you are short on time, I am glad to pick the POC up and carry it through — just say the word. Disclosure: AI-assisted (Claude) for building the test variants; I reviewed the results and the code. |
…3784) ## What this PR does Brings `GOVERNANCE.md` in line with the organisation it describes: what the project governs, and what it ships but does not. ### Sub-projects The `Code Repositories` list named seven repositories and was wrong in both directions. Five of the six sub-projects whose code ships in the distribution were missing from it — `etcd-operator`, `keycloak-kms-proxy`, `local-ccm`, `cozystack-scheduler` and `ingress-nginx-with-protobuf-exporter` — while `talos-bootstrap` and `talos-meta-tool` were listed although an adopter installs neither. Nothing stated what being on the list meant, which is why it drifted. It is replaced by a section organised around the distinction that decides scope: whether a repository's output reaches the artifact an adopter installs. Packaged sub-projects get a table naming what each one contributes, its stability, and its owners — the same handles each repository's `CODEOWNERS` already carries. Independent tooling, project infrastructure and upstream forks are described in a line each, as explicitly outside that scope. Three things an evaluator finds anyway are stated rather than left to be discovered: that one fork (`apimachinery`) is compiled into the product, why, and that the fix is proposed upstream as kubernetes/kubernetes#135537; that four repositories are private, with the reason for each, and that none of them contributes to the distribution; and that telemetry is received by a component adopters do not install, with a link to the opt-out. ### Third-party Dependencies Writes down which shipped components the project does not govern. Cozystack vendors software it does not own, which is ordinary and needs no defence — but the boundary was nowhere stated. This is a Required item on the Incubation checklist ("All project metadata and resources are vendor-neutral"), and an evaluator currently has nothing in the governance document to read it off. The section says two things: what the arrangement is, and where the components are listed. It names nothing itself. The [Licenses](https://cozystack.io/docs/v1.6/operations/configuration/licenses/) page carries the list, and by its own framing carries upstreams only — Cozystack-maintained components are Apache-2.0 and are not listed there individually. Appearing on that page is therefore already the classification, so a second list here would only drift from it. Two components that belong on that page were missing from it despite shipping since v1.3.4 and v1.4.3; cozystack/website#649 adds them. Nothing moves, nothing is relicensed, no mirroring changes. This documents the existing arrangement, which each package's `values.yaml` already describes at its own level. ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` against the diff. It touches `GOVERNANCE.md` only, and no trigger concerns governance documents — the website triggers are package additions, platform values, variants, components, release assets, `ApplicationDefinition` semantics, the Talos pin and developer tooling. The website PR linked above is a companion, not a consequence of this diff: it fixes a pre-existing gap on the Licenses page that stands on its own. - [x] No downstream repository is affected by this change ### Release note ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added governance guidance for project sub-components, including ownership, licensing, maintenance, and repository classifications. * Documented project infrastructure, upstream forks, private repositories, and telemetry services. * Clarified how third-party software is sourced, governed, mirrored, and versioned. * Linked to a license inventory listing third-party components and their applicable licenses. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
What type of PR is this?
/kind bug
What this PR does / why we need it:
Fixes
ConvertToVersion()to prefer the object's GVK field over the first registered kind when multiple kinds are registered with the same Go type.When multiple kinds are registered with the same Go type using
scheme.AddKnownTypeWithName(),ConvertToVersion()was always selecting the first registered kind fromtypeToGVKinstead of using the actual GVK from the object's field. This causes incorrect behavior in dynamic API servers where the same Go type (e.g.,Application{}) is registered multiple times with different kinds (e.g.,BootBox,Tenant,Postgres) at runtime.Use case:
In the cozystack.io project, we use a dynamic API-server where types are registered dynamically at runtime using
gvk.GroupVersion().WithKind(kind)multiple times for the same Go typeApplication{}. WhenConvertToVersion()is called, it incorrectly returns the first registered type instead of the actual type from the object's field. For more details on how this dynamic API server works, see the blog post.Workaround limitations:
unstructured.Unstructured{}, which reads the type from the field via a fast path inapimachinery/pkg/runtime/serializer/versioning/versioning.go, avoidingConvertToVersion().This PR ensures that when an object has a valid GVK in its field that matches one of the registered kinds, that GVK is used instead of always defaulting to the first registered kind.
Which issue(s) this PR is related to:
N/A
Special notes for your reviewer:
This change modifies the
convertToVersionfunction instaging/src/k8s.io/apimachinery/pkg/runtime/scheme.goto:The fix includes comprehensive tests for both typed objects and typed lists to verify that:
Example usage context:
In cozystack.io, types are registered like this:
Before this fix, all three would return the GVK of "BootBox" (first registered) after
ConvertToVersion(). After this fix, each object returns its actual GVK from the field.Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
AI Disclosure
AI tooling (Claude) was used to assist in preparing this PR. All changes have been reviewed and verified by the author. This fix has been running in production (cozystack) for a long time from a forked apimachinery.