apiextentionserver: refactor returning 503 for custom resource requests during server start #105653
Conversation
k8s-ci-robot
added
release-note-none
k8s-ci-robot
added
area/apiserver
sig/api-machinery
k8s-ci-robot
added
triage/accepted
| // to ensure that requests to potential custom resource endpoints while the server | ||
| // hasn't been fully initialized get a 503 error instead of a 404 | ||
| // | ||
| hasCRDInformerSyncedSignal chan struct{} |
There was a problem hiding this comment.
nit: I wonder if this can be an attribute of the GenericAPIServer (or if we will overload the GenericAPIServer type)-- each extensible server can report if it has warmed up its cache from the storage
There was a problem hiding this comment.
we decided to add RegisterMuxAndDiscoveryCompleteSignal on the GenericAPIServer
| @@ -246,19 +244,11 @@ func (r *crdHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) { | |||
| // only match /apis/<group>/<version> | |||
| // only registered under /apis | |||
| if len(pathParts) == 3 { | |||
| if !r.hasSynced() { | |||
| responsewriters.ErrorNegotiated(serverStartingError(), Codecs, schema.GroupVersion{Group: requestInfo.APIGroup, Version: requestInfo.APIVersion}, w, req) | |||
There was a problem hiding this comment.
Do you know if there is any race that can cause #104342 (comment) to CRs? It looks like the existing code is protecting us already, right?
There was a problem hiding this comment.
it seems that the existing code already protects us.
p0lyn0mial
force-pushed
the
crd-503-refactor
branch
from
81cf07c
to
4ebf719
Compare
k8s-ci-robot
added
size/XL
|
Please rebase |
k8s-ci-robot
added
the
needs-rebase
…ts during server start Previously the customresource handler explicitly checked if the crd informer has been synced and replied with a 503 in case it hasn't. This PR moves logic to the NotFoundHandler and WithMuxAndDiscoveryCompleteProtection filter.
p0lyn0mial
force-pushed
the
crd-503-refactor
branch
from
4ebf719
to
86d8458
Compare
k8s-ci-robot
added
size/M
k8s-ci-robot
removed
the
needs-rebase
|
/lgtm |
k8s-ci-robot
added
the
lgtm
| if !r.hasSynced() { | ||
| responsewriters.ErrorNegotiated(serverStartingError(), Codecs, schema.GroupVersion{Group: requestInfo.APIGroup, Version: requestInfo.APIVersion}, w, req) | ||
| return | ||
| } |
There was a problem hiding this comment.
groupDiscoveryHandler would return what for unknown groups?
There was a problem hiding this comment.
it would return 503, the groupDiscoveryHandler delegates to our custom handler. This scenario is covered by a unit tests
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: p0lyn0mial, roycaihw, sttts The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
This PR moves logic to the notfoundhandler that is executed at the end of the delegation chain and checks if the crd informer has been fully initialized before replying to the client.
Previously the customresource handler explicitly checked if the crd informer has been synced and replied with a 503 in case it hasn't.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
requires #104748 first
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: