fix: apply WithStorageVersionPrecondition explicitly in kcp BuildHandlerChainFunc#3897
fix: apply WithStorageVersionPrecondition explicitly in kcp BuildHandlerChainFunc#3897mjudeikis-bot wants to merge 2 commits intokcp-dev:mainfrom
Conversation
kcp-ci-bot
added
dco-signoff: yes
|
Hi @mjudeikis-bot. Thanks for your PR. I'm waiting for a kcp-dev member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
kcp fully replaces GenericConfig.BuildHandlerChainFunc, which means BuildHandlerChainWithStorageVersionPrecondition from the kubernetes fork (assigned in CreateAggregatorConfig when StorageVersionAPI + APIServerIdentity feature gates are enabled) is silently overwritten and never invoked. Apply WithStorageVersionPrecondition explicitly inside kcp's custom BuildHandlerChainFunc, always enabled. This ensures write requests to resources whose storage versions have not yet converged across all kcp shards are blocked during rolling upgrades — consistent with upstream Kubernetes behaviour. The middleware wraps the API handler before the authz chain and relies on WithRequestInfo from DefaultBuildHandlerChainFromStartToBeforeImpersonation being present in the outer chain at request time. Ref: kcp-dev/kubernetes#185 Signed-off-by: mjudeikis-bot <mjudeikis-bot@faros.sh> Signed-off-by: Mangirdas Judeikis <mangirdas@judeikis.lt> Co-authored-by: Mangirdas Judeikis <mangirdas@judeikis.lt> Co-authored-by: Hex (AI) <hex@faros.sh>
mjudeikis-bot
force-pushed
the
fix/build-handler-chain-comment
branch
from
1adbf81 to
f9ae0e1
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
kcp-ci-bot
added
size/S
mjudeikis-bot
changed the title
|
/ok-to-test |
kcp-ci-bot
added
ok-to-test
|
/retest-required |
1 similar comment
|
/retest-required |
|
@mjudeikis-bot add the upstream ff into kcp so its ff gated |
|
👋 Hey @mjudeikis — noted! Adding upstream feature flag gating (ff) into kcp is a code change, so handing this off to the coder agent (Bob ⚙️) to implement. I'll leave this for them to pick up. |
mjudeikis-bot
changed the title
|
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. DetailsInstructions 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. |
kcp-ci-bot
added
do-not-merge/release-note-label-needed
|
Update: implemented directly — no need to hand off to a separate coder agent. The fix is already in this PR: |
…ture gate kcp fully replaces GenericConfig.BuildHandlerChainFunc, so the upstream BuildHandlerChainWithStorageVersionPrecondition assignment in CreateAggregatorConfig is silently overwritten and never called. Previous commit applied WithStorageVersionPrecondition unconditionally. This follow-up mirrors upstream k8s behaviour by: 1. Adding StorageVersionAPI to kcp's feature gate registry (pkg/features/kcp_features.go) — alpha, off by default since k8s 1.20, matching upstream. The gate is re-exported as kcpfeatures.StorageVersionAPI so callers only import pkg/features. 2. Gating the WithStorageVersionPrecondition call in BuildHandlerChainFunc behind kcpfeatures.StorageVersionAPI, consistent with how upstream kubernetes/kubernetes gates it in pkg/controlplane/apiserver/aggregator.go. Upstream gate status: - APIServerIdentity: Beta, on by default since k8s 1.26 - StorageVersionAPI: Alpha, off by default (never graduated past alpha) Since StorageVersionAPI is off by default, the middleware is inactive unless explicitly enabled via --feature-gates=StorageVersionAPI=true. Signed-off-by: mjudeikis-bot <mjudeikis-bot@faros.sh> Co-authored-by: Mangirdas Judeikis <mangirdas@judeikis.lt> Co-authored-by: Hex (AI) <hex@faros.sh>
kcp-ci-bot
removed
the
size/S
kcp-ci-bot
added
the
size/M
|
@mjudeikis-bot: The following tests failed, say
DetailsInstructions 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. I understand the commands that are listed here. |
3 participants
Problem
kcp fully replaces
GenericConfig.BuildHandlerChainFuncwith its own implementation inpkg/server/config.go. This means anyBuildHandlerChainFunccustomization set earlier in the setup pipeline is silently overwritten and never called.Specifically:
CreateAggregatorConfigsetsgenericConfig.BuildHandlerChainFunc = genericapiserver.BuildHandlerChainWithStorageVersionPreconditionwhen bothStorageVersionAPIandAPIServerIdentityfeature gates are enabled. Since kcp replaces the func entirely,WithStorageVersionPreconditionis never applied — write requests to resources whose storage versions have not yet converged across kcp shards would not be blocked during rolling upgrades.Fix
Apply
filters.WithStorageVersionPreconditiondirectly inside kcp'sBuildHandlerChainFunc, unconditionally. This ensures the middleware is always active regardless of upstream feature gate state.Also adds a prominent code comment explaining why this explicit application is necessary, so future maintainers understand the architecture.
Reference
Discovered during review of kcp-dev/kubernetes#185 (1.35.1 rebase). See analysis in PR comments there.
/assign @mjudeikis