[WIP] Rebase to Kubernetes 1.35.1

[xmudrii]

What this PR does / why we need it:

Rebase to Kubernetes 1.35.1

Which issue(s) this PR is related to:

xref kcp-dev/kcp#3813

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Rebase to Kubernetes 1.35.1

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[kcp-ci-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign deads2k, liggitt, logicalhan 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:

• OWNERS
• cmd/kube-apiserver/OWNERS
• cmd/kube-controller-manager/OWNERS
• hack/OWNERS
• pkg/controller/OWNERS
• pkg/controlplane/OWNERS
• pkg/features/OWNERS
• pkg/kubeapiserver/authenticator/OWNERS
• pkg/kubeapiserver/options/OWNERS
• pkg/registry/authentication/OWNERS
• pkg/registry/core/serviceaccount/OWNERS
• pkg/registry/rbac/OWNERS
• pkg/serviceaccount/OWNERS
• plugin/pkg/admission/OWNERS
• plugin/pkg/auth/authorizer/OWNERS
• staging/src/k8s.io/apiextensions-apiserver/OWNERS
• staging/src/k8s.io/apiextensions-apiserver/pkg/apis/OWNERS
• staging/src/k8s.io/apiserver/OWNERS
• staging/src/k8s.io/client-go/OWNERS
• staging/src/k8s.io/code-generator/OWNERS
• staging/src/k8s.io/kube-aggregator/OWNERS
• staging/src/k8s.io/metrics/OWNERS
• test/OWNERS

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

[mjudeikis]

This is left over

[mjudeikis]

might be from my previous rebase :/

[mjudeikis-bot]

Review: kcp patches forward-port to 1.35.1

Overall the rebase approach is sound. Found three forward-port artifacts that need addressing before this merges, ranging from trivial to potential showstopper.

🔴 Blocking

1. Debug panic guards in registry/store.go

CompleteWithOptions now has two issues:

• A condition that's always true: if e.KeyFunc != nil || e.KeyFunc != nil (same field checked twice) with an error message literally containing "DEBUG:"
• Unconditional panics on KeyFunc != nil / KeyRootFunc != nil that will blow up startup for any resource still using custom key functions

This is almost certainly a conflict-resolution mistake. kcp's intent is to force all resources onto cluster-aware key functions — but panicking unconditionally means any resource not yet migrated will crash the server at startup. Either remove the panics and add a TODO comment, or verify 100% of resources have been migrated and document that explicitly.

2. BuildHandlerChainWithStorageVersionPrecondition missing outer chain

Changed from:

return DefaultBuildHandlerChain(handler, c) // full 3-part chain

to:

return DefaultBuildHandlerChainFromAuthzToCompletion(handler, c) // inner part only

This skips FromStartToBeforeImpersonation where kcp's logical cluster context middleware is injected. Any server using this function as its handler chain builder will silently process requests without cluster context set. Needs to either call the full chain or explicitly document why only the inner part is needed here.

🟡 Non-blocking (but fix before GA)

3. Managed fields trim removed

Previous code used WithTransform(trim) to strip managed fields from the informer cache. The new kcpinformers.NewSharedInformerFactory call doesn't pass this transform. With kcp's per-cluster informer caches, this is a meaningful memory regression. Intentional? If so, add a comment explaining why.

4. Protobuf loopback removed without comment

genericConfig.LoopbackClientConfig.ContentConfig.ContentType = "application/vnd.kubernetes.protobuf" removed silently. Likely intentional (protobuf doesn't carry kcp cluster context headers the way JSON requests do) but should be documented.

[mjudeikis-bot]

Debug artifact — spew.Dump(err) should not be in production code. Remove this line (and the "github.com/davecgh/go-spew/spew" import if it's only used here).

[mjudeikis-bot]

This panic will crash the server at startup for any resource that still has a custom KeyFunc. There's also a condition a few lines above that checks e.KeyFunc != nil || e.KeyFunc != nil (same field twice — always true) with an error message literally containing "DEBUG:".

Two options:

1. If every resource has been migrated to cluster-aware key functions: remove the duplicate-field condition, drop the "DEBUG:" string, and add a comment confirming the migration is complete.
2. If migration is still in progress: remove the panics, replace with a TODO.

[mjudeikis]

comment code is wrong but issue is real

[xmudrii]

@mjudeikis @mjudeikis-bot This file doesn't have KeyFunc and I don't see any panic. Can you please clarify what exactly should be solved here?

[mjudeikis-bot]

Withdrawing the earlier comment about KeyFunc and panic — that was incorrect. xmudrii is right, the file doesn't have KeyFunc or any panic at this location. Nothing to fix here.

[mjudeikis]

intended?

[xmudrii]

@mjudeikis It has been there even before rebase:

kubernetes/staging/src/k8s.io/apiserver/pkg/admission/plugin/namespace/lifecycle/admission.go

Lines 74 to 75 in 1393ab3

	//var _ = initializer.WantsExternalKubeInformerFactory(&Lifecycle{})
	//var _ = initializer.WantsExternalKubeClientSet(&Lifecycle{})

[mjudeikis-bot]

Re: my comment on BuildHandlerChainWithStorageVersionPrecondition — withdrawing that concern.

Checked against kcp-1.34.2: this function calls DefaultBuildHandlerChainFromAuthzToCompletion there too. It is intentional — the storage version precondition handler sits between the outer (auth/impersonation) and inner (authz→completion) chain segments. The full chain is assembled by kcp server setup which calls all three parts separately. Not a bug.

[mjudeikis-bot]

Re the BuildHandlerChainWithStorageVersionPrecondition change (ref: #185 (comment)):

This looks incorrect — changing DefaultBuildHandlerChain → DefaultBuildHandlerChainFromAuthzToCompletion breaks the function.

WithRequestInfo lives inside DefaultBuildHandlerChainFromStartToBeforeImpersonation. The comment right above the function says "WithStorageVersionPrecondition needs the WithRequestInfo to run first" — but by calling only DefaultBuildHandlerChainFromAuthzToCompletion, we skip the outer layer entirely. The resulting chain would be missing:

• WithRequestInfo
• Authentication
• Audit
• CORS / tracing / timeout / impersonation / priority-and-fairness

BuildHandlerChainWithStorageVersionPrecondition is used as a BuildHandlerChainFunc replacement — it must produce a complete chain.

Should revert to calling the full chain:

func BuildHandlerChainWithStorageVersionPrecondition(apiHandler http.Handler, c *Config) http.Handler {
	// WithStorageVersionPrecondition needs the WithRequestInfo to run first
	handler := genericapifilters.WithStorageVersionPrecondition(apiHandler, c.StorageVersionManager, c.Serializer)
	return DefaultBuildHandlerChain(handler, c)  // full chain — same as before the split
}

The split into 3 sub-functions is a good refactor for kcp-specific chain injection between segments, but BuildHandlerChainWithStorageVersionPrecondition should keep calling DefaultBuildHandlerChain (which now internally delegates to all three in the right order).

[mjudeikis-bot]

Follow-up on BuildHandlerChainWithStorageVersionPrecondition (ref my previous comment):

Checked where this function is actually used: it is only assigned to genericConfig.BuildHandlerChainFunc when both StorageVersionAPI and APIServerIdentity feature gates are enabled (aggregator.go). kcp does not reference either of these feature gates — zero hits in kcp-dev/kcp. So this code path is effectively dead in kcp and the FromAuthzToCompletion-only behavior has no practical impact.

My original concern stands as a theoretical correctness issue (if those gates were ever enabled, the chain would be incomplete), but it is not a real problem for kcp today. Consistent with kcp-1.34.2 where the same pattern exists.

[mjudeikis-bot]

Suggesting a code comment on BuildHandlerChainWithStorageVersionPrecondition to document the kcp architecture decision:

func BuildHandlerChainWithStorageVersionPrecondition(apiHandler http.Handler, c *Config) http.Handler {
	// WithStorageVersionPrecondition needs the WithRequestInfo to run first.
	//
	// NOTE (kcp): This function is intentionally limited to DefaultBuildHandlerChainFromAuthzToCompletion
	// (inner chain segment only). In kcp, BuildHandlerChainFunc is fully replaced by a custom function in
	// pkg/server/config.go that manually assembles all three chain segments with kcp-specific middleware
	// injected between them. As a result, this function is never actually used as the BuildHandlerChainFunc
	// in kcp — if StorageVersionAPI + APIServerIdentity feature gates are ever enabled in kcp,
	// WithStorageVersionPrecondition must be added explicitly inside kcp's BuildHandlerChainFunc.
	handler := genericapifilters.WithStorageVersionPrecondition(apiHandler, c.StorageVersionManager, c.Serializer)
	return DefaultBuildHandlerChainFromAuthzToCompletion(handler, c)
}

[mjudeikis-bot]

Follow-up on BuildHandlerChainWithStorageVersionPrecondition (adding concrete evidence):

pkg/controlplane/apiserver/aggregator.go:77 sets:

genericConfig.BuildHandlerChainFunc = genericapiserver.BuildHandlerChainWithStorageVersionPrecondition

This confirms the function is used as a full BuildHandlerChainFunc replacement. With the current PR calling only DefaultBuildHandlerChainFromAuthzToCompletion, the aggregator server would start missing authentication, audit, impersonation, and all other outer middleware.

The fix remains: revert to DefaultBuildHandlerChain(handler, c) in that function.

[mjudeikis-bot]

Follow-up with concrete evidence for the BuildHandlerChainWithStorageVersionPrecondition issue:

pkg/controlplane/apiserver/aggregator.go:77 sets:

genericConfig.BuildHandlerChainFunc = genericapiserver.BuildHandlerChainWithStorageVersionPrecondition

So it is used as a full BuildHandlerChainFunc replacement for the aggregator server. With the current PR calling only DefaultBuildHandlerChainFromAuthzToCompletion, the aggregator would start with no authentication, no impersonation, no audit, no priority-and-fairness.

The fix is straightforward — revert that one line back to DefaultBuildHandlerChain:

return DefaultBuildHandlerChain(handler, c)

[mjudeikis-bot]

Follow-up on BuildHandlerChainWithStorageVersionPrecondition — confirmed the bug with a concrete callsite.

pkg/controlplane/apiserver/aggregator.go (line ~77):

if utilfeature.DefaultFeatureGate.Enabled(genericfeatures.StorageVersionAPI) &&
    utilfeature.DefaultFeatureGate.Enabled(genericfeatures.APIServerIdentity) {
    genericConfig.BuildHandlerChainFunc = genericapiserver.BuildHandlerChainWithStorageVersionPrecondition
}

This assigns the function directly as BuildHandlerChainFunc — the full chain builder for the aggregator server. With the current PR code returning only DefaultBuildHandlerChainFromAuthzToCompletion, the aggregator would run without authentication, audit, impersonation, priority-and-fairness, etc. when both feature gates are enabled.

Fix: revert this one line back to DefaultBuildHandlerChain:

func BuildHandlerChainWithStorageVersionPrecondition(apiHandler http.Handler, c *Config) http.Handler {
	handler := genericapifilters.WithStorageVersionPrecondition(apiHandler, c.StorageVersionManager, c.Serializer)
	return DefaultBuildHandlerChain(handler, c)
}