return 503 for aggregated APIs when the APIServiceRegistrationController hasn't finished installing all known APIServices #104748

p0lyn0mial · 2021-09-03T09:24:31Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

Ensures that requests to aggregated APIs will get 503, not 404 while the APIServiceRegistrationController hasn't finished installing all known APIServices. It is especially important for controllers like GC and NS since they act on 404

Note that this is an already existing behavior (returning 503 when the CRD informer hasn't been sync).
We already have it for the apiextentionserver, see #105653 for more details.

Which issue(s) this PR fixes:

Fixes #104342

Does this PR introduce a user-facing change?

resolves a potential issue with GC and NS controllers which may delete objects after getting a 404 response from the server during its startup. This PR ensures that requests to aggregated APIs will get 503, not 404 while the APIServiceRegistrationController hasn't finished its job.

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

p0lyn0mial · 2021-09-03T09:24:58Z

/assing @tkashem @sttts

sttts · 2021-09-03T09:50:41Z

cmd/kube-apiserver/app/server.go

@@ -22,6 +22,7 @@ package app
 import (
 	"crypto/tls"
 	"fmt"
+	"k8s.io/apiserver/pkg/util/notfoundhandler"


sttts · 2021-09-03T09:52:10Z

staging/src/k8s.io/apiserver/pkg/server/genericapiserver.go

 func (s emptyDelegate) UnprotectedHandler() http.Handler {
-	return nil
+	return s.handler


remind me why we only need this for the unprotected handler

this is the one we use at the end of the chain, here https://github.com/kubernetes/kubernetes/blob/master/cmd/kube-apiserver/app/server.go#L190

Can you please help me understand, by spelling out the rest of the answer?

The API server is composed of three servers. The AggregatorServer is first in the chain and delegates to the KubeAPIServer which in turn delegates to the ExtentionAPIServer.

Delegation happens when the server doesn't have a path registered to handle a request.

Only the first server in the chain is protected in a sense that it will apply all the registered filters, like AuthZ/AuthnN.

Unprotected in this case means give me a handler that won't apply the filters. In this specific case it make sense since the first server have already done the hard work.

@sttts : I am not sure I understand your question. A DelegationTarget has only one method that returns a Handler (this PR does not change this fact).

sttts · 2021-09-03T09:54:49Z

staging/src/k8s.io/apiserver/pkg/util/notfoundhandler/not_found_handler.go

+	"k8s.io/apiserver/pkg/util/signal"
+)
+
+// New returns an HTTP handler that is meant to be executed at the end of the delegation chain and check if the server has been fully prepared before replying to the client.


mention that this is long before readiness and is meant to block request before the muxes and handlers are completely initialized such that requests are properly routed.

And that preparedToServe must not depend on other components in the cluster other than etcd

sttts · 2021-09-03T09:56:04Z

staging/src/k8s.io/apiserver/pkg/util/notfoundhandler/not_found_handler.go

+
+func (h *Handler) ServeHTTP(rw http.ResponseWriter, req *http.Request) {
+	for _, preparedToServeSignal := range h.preparedToServeSignals {
+		if !preparedToServeSignal.IsSignaled() {


continue early

sttts · 2021-09-03T09:57:52Z

staging/src/k8s.io/kube-aggregator/pkg/apiserver/apiserver.go

@@ -113,10 +114,26 @@ type preparedAPIAggregator struct {
 	runnable runnable
 }

+// preparedToServeSignals holds signals/events that indicate this server has been fully initialized and is ready to serve requests
+type preparedToServeSignals struct {


why a wrapper around another type? Just use signal.Interface directly

I wanted to expose only the read-only interface

But the current text exposes the full interface.
It looks to me like the reason for a struct here is to prepare for more signals in the future. For example, one about RBAC being initialized.

p0lyn0mial · 2021-09-03T10:33:12Z

I will remove the signal pkg and use raw types instead. I will also add a signal from the extension API server.

tkashem · 2021-09-03T13:29:53Z

staging/src/k8s.io/apiserver/pkg/util/signal/signal.go

+	case <-e.ch:
+		// already closed, don't close again.
+	default:
+		close(e.ch)


if Signal is used concurrently, i think we need to use a sync.Once to protect it.

tkashem · 2021-09-03T13:34:35Z

staging/src/k8s.io/apiserver/pkg/util/signal/signal.go

+	}
+}
+
+type namedChannelWrapper struct {


since we are converting this to a shared utility, i would recommend unit tests for it.

caesarxuchao · 2021-09-07T20:04:13Z

/cc @cheftako @jpbetz

Walter, I think you mentioned a specific http status code during the sig meeting.

caesarxuchao · 2021-09-07T20:04:16Z

/triage accepted

tkashem · 2021-09-07T20:30:29Z

/cc @MikeSpreitzer

MikeSpreitzer · 2021-09-07T22:43:31Z

staging/src/k8s.io/apiserver/pkg/server/genericapiserver.go

 func (s emptyDelegate) UnprotectedHandler() http.Handler {
-	return nil
+	return s.handler


Can you please help me understand, by spelling out the rest of the answer?

MikeSpreitzer · 2021-09-07T22:44:14Z

staging/src/k8s.io/apiserver/pkg/server/genericapiserver.go

@@ -271,14 +271,19 @@ func (s *GenericAPIServer) NextDelegate() DelegationTarget {
 }

 type emptyDelegate struct {
+	handler http.Handler


What did "empty delegate" mean, exactly? Is it still empty?

The empty delegate is an HTTP handler that doesn't have any paths registered and won't pass the requests through to the next delegate.

It will still be empty as it doesn't registers any paths nor the next delegate. It allows us to register a special NotFound handler.

add a go comment describing what handler is and when it is called

MikeSpreitzer · 2021-09-07T22:51:22Z

staging/src/k8s.io/kube-aggregator/pkg/apiserver/apiserver.go

@@ -113,10 +114,26 @@ type preparedAPIAggregator struct {
 	runnable runnable
 }

+// preparedToServeSignals holds signals/events that indicate this server has been fully initialized and is ready to serve requests
+type preparedToServeSignals struct {


But the current text exposes the full interface.
It looks to me like the reason for a struct here is to prepare for more signals in the future. For example, one about RBAC being initialized.

… as a MuxComplete signal apiServiceRegistrationControllerInitiated is closed when APIServiceRegistrationController has finished "installing" all known APIServices. At this point we know that the proxy handler knows about APIServices and can handle client requests. Before it might have resulted in a 404 response which could have serious consequences for some controllers like GC and NS

p0lyn0mial · 2021-10-19T09:18:41Z

/retest

sttts · 2021-10-19T10:55:21Z

staging/src/k8s.io/apiserver/pkg/server/genericapiserver.go

+			}
+		}
+		s.lifecycleSignals.MuxAndDiscoveryComplete.Signal()
+		klog.V(1).Infof("%s completed, all registered MuxAndDiscoveryComplete signals (%d) have finished", s.lifecycleSignals.MuxAndDiscoveryComplete.Name(), s.GenericAPIServer.MuxAndDiscoveryCompleteSignals())


would be less technical. Better: %s has all endpoints registered and discovery information is complete

sttts · 2021-10-19T10:56:19Z

staging/src/k8s.io/apiserver/pkg/util/notfoundhandler/not_found_handler.go

+//
+// Note that we don't want to add additional checks to the readyz path as it might prevent fixing bricked clusters.
+// This specific handler is meant to "protect" requests that arrive before the paths and handlers are fully initialized.
+func New(serializer runtime.NegotiatedSerializer, hasMuxAndDiscoveryIncompleteKeyFn func(ctx context.Context) bool) *Handler {


remove the negation: isMuxAndDiscoveryComplete

sttts · 2021-10-19T13:02:30Z

Lgtm and approved.

Will give @MikeSpreitzer @caesarxuchao and others a chance to take a final look.

MikeSpreitzer

What about https://docs.google.com/document/d/1JQNT0eu6Gpn9UZ4rcfIUDkJf1wKc3f_57KKC4c-_s-U ? My recollection of the SIG api-machinery discussion is that a client opt-in was favored, out of concern that a non-optional solution would ruining some startup sequences that involve multiple processes.

MikeSpreitzer · 2021-10-20T05:58:45Z

staging/src/k8s.io/apiserver/pkg/server/genericapiserver.go

 type emptyDelegate struct {
+	// handler is called at the end of the delegation chain
+	// when a request has been made against an unregistered HTTP path the individual servers will simply pass it through until it reaches the handler.
+	handler http.Handler
 }

 func NewEmptyDelegate() DelegationTarget {


Do we still need this func?

projects using the generic api server libary might still use this method.

MikeSpreitzer · 2021-10-20T06:08:28Z

staging/src/k8s.io/apiserver/pkg/util/notfoundhandler/not_found_handler.go

+
+func (h *Handler) ServeHTTP(rw http.ResponseWriter, req *http.Request) {
+	if !h.isMuxAndDiscoveryCompleteFn(req.Context()) {
+		errMsg := fmt.Sprintf("the request has been made before all known HTTP paths have been installed, please try again")


What is fmt.Sprintf doing for us here?

MikeSpreitzer · 2021-10-20T06:17:21Z

staging/src/k8s.io/kube-aggregator/pkg/apiserver/apiserver.go

+	// Before it might have resulted in a 404 response which could have serious consequences for some controllers like  GC and NS
+	//
+	// Note that the APIServiceRegistrationController waits for APIServiceInformer to synced before doing its work.
+	apiServiceRegistrationControllerInitiated := make(chan struct{})


Do we need both this channel and the handlerSyncedCh channel in the apiservice-registration-controller post start hook? Could we use just one channel for both purposes?

good point, we can have just one channel.

MikeSpreitzer · 2021-10-20T06:28:28Z

staging/src/k8s.io/apiserver/pkg/endpoints/filters/mux_discovery_complete.go

+//
+// The presence of the key is checked in the NotFoundHandler (staging/src/k8s.io/apiserver/pkg/util/notfoundhandler/not_found_handler.go)
+//
+// The race may happen when a request reaches the NotFoundHandler because not all paths have been registered in the mux


Referring to "the race" here is appropriate if the reader already knows there is a race involved.
However, I think the reader does not know that.
"A race" would be more appropriate for introducing the fact that there is a race.

updated the gdoc.

sttts · 2021-10-20T08:58:07Z

My recollection of the SIG api-machinery discussion is that a client opt-in was favored, out of concern that a non-optional solution would ruining some startup sequences that involve multiple processes.

@MikeSpreitzer in that discussion we didn't know that we had already a solution in place for CRDs, i.e. something that has become part of the API semantics expected by clients and that we cannot easily change.

Whether we want anything beyond this, we can discuss.

p0lyn0mial · 2021-10-20T09:11:57Z

@sttts @MikeSpreitzer many thanks for reviewing the PR. I applied the last batch of comments. I think we are ready to tag.

p0lyn0mial · 2021-10-20T10:06:42Z

/test pull-kubernetes-unit

MikeSpreitzer

This looks basically OK to me.
I have just a few minor comments.

MikeSpreitzer · 2021-10-21T04:14:50Z

staging/src/k8s.io/apiserver/pkg/server/genericapiserver.go

 func (s emptyDelegate) UnprotectedHandler() http.Handler {
-	return nil
+	return s.handler


@sttts : I am not sure I understand your question. A DelegationTarget has only one method that returns a Handler (this PR does not change this fact).

staging/src/k8s.io/apiserver/pkg/server/genericapiserver_graceful_termination_test.go

staging/src/k8s.io/apiserver/pkg/server/genericapiserver_test.go

…ectly to apiserviceRegistration controller

p0lyn0mial · 2021-10-25T13:51:40Z

My recollection of the SIG api-machinery discussion is that a client opt-in was favored, out of concern that a non-optional solution would ruining some startup sequences that involve multiple processes.

@MikeSpreitzer in that discussion we didn't know that we had already a solution in place for CRDs, i.e. something that has become part of the API semantics expected by clients and that we cannot easily change.

Whether we want anything beyond this, we can discuss.

It looks like there is still room for improvement. It looks like we should protect requests to the discovery endpoint /apis too. The restmapper used by the GC might get incomplete discovery info (missing Kinds).

This could be fixed as proposed in the KEP by rejecting requests with a 503 response when the server hasn't become ready (/readyz) for some controllers.

p0lyn0mial · 2021-10-26T14:02:11Z

This could be fixed as proposed in the KEP by rejecting requests with a 503 response when the server hasn't become ready (/readyz) for some controllers.

or we could open a new PR that would simply reject (503) requests made by the GC controller to the discovery API when it hasn't observed all GVK

sttts · 2021-10-26T17:10:43Z

#104748 (comment) will follow for the remaining bit. For now this clearly improves the situation.

/lgtm
/approve

k8s-ci-robot · 2021-10-26T17:11:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: p0lyn0mial, 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:

~~cmd/kube-apiserver/OWNERS~~ [sttts]
~~staging/src/k8s.io/apiserver/OWNERS~~ [sttts]
~~staging/src/k8s.io/kube-aggregator/OWNERS~~ [sttts]
~~vendor/OWNERS~~ [sttts]

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

k8s-ci-robot requested review from hzxuzhonghu and liggitt September 3, 2021 09:25

k8s-ci-robot added area/apiserver sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 3, 2021

sttts reviewed Sep 3, 2021

View reviewed changes

tkashem reviewed Sep 3, 2021

View reviewed changes

k8s-ci-robot requested review from cheftako and jpbetz September 7, 2021 20:04

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Sep 7, 2021

MikeSpreitzer reviewed Sep 7, 2021

View reviewed changes

p0lyn0mial force-pushed the not-found-handler branch from d3fdf02 to c54463d Compare October 19, 2021 08:10

p0lyn0mial force-pushed the not-found-handler branch from 7cea69a to 2e2d22a Compare October 19, 2021 10:37

sttts reviewed Oct 19, 2021

View reviewed changes

rename to muxAndDiscoveryComplete

9e2bdfe

p0lyn0mial force-pushed the not-found-handler branch from 2e2d22a to 9e2bdfe Compare October 19, 2021 11:28

sttts added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Oct 19, 2021

k8s-ci-robot removed the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Oct 19, 2021

MikeSpreitzer reviewed Oct 20, 2021

View reviewed changes

ardaguclu mentioned this pull request Oct 20, 2021

WIP: Add waiting for OpenAPI specs being imported before starting readyz #104323

Closed

MikeSpreitzer reviewed Oct 21, 2021

View reviewed changes

p0lyn0mial force-pushed the not-found-handler branch from 728a40d to b6c2c4b Compare October 21, 2021 13:22

aggregator: pass apiServiceRegistrationControllerInitiated signal dir…

5116a50

…ectly to apiserviceRegistration controller

p0lyn0mial force-pushed the not-found-handler branch from b6c2c4b to 5116a50 Compare October 21, 2021 13:25

k8s-ci-robot assigned sttts Oct 26, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 26, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 26, 2021

k8s-ci-robot merged commit 7c53095 into kubernetes:master Oct 26, 2021

k8s-ci-robot added this to the v1.23 milestone Oct 26, 2021

liggitt mentioned this pull request Feb 1, 2023

Potential inconsistent behaviour with apiserver apiextensions (custom resources) and 404s #115241

Open

return 503 for aggregated APIs when the APIServiceRegistrationController hasn't finished installing all known APIServices #104748

return 503 for aggregated APIs when the APIServiceRegistrationController hasn't finished installing all known APIServices #104748

Conversation

p0lyn0mial commented Sep 3, 2021 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?

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

p0lyn0mial commented Sep 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0lyn0mial commented Sep 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caesarxuchao commented Sep 7, 2021

caesarxuchao commented Sep 7, 2021

tkashem commented Sep 7, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0lyn0mial commented Oct 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts commented Oct 19, 2021

MikeSpreitzer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts commented Oct 20, 2021

p0lyn0mial commented Oct 20, 2021

p0lyn0mial commented Oct 20, 2021

MikeSpreitzer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0lyn0mial commented Oct 25, 2021 • edited by sttts

p0lyn0mial commented Oct 26, 2021

sttts commented Oct 26, 2021

k8s-ci-robot commented Oct 26, 2021

p0lyn0mial commented Sep 3, 2021 •

edited

tkashem commented Sep 7, 2021 •

edited

p0lyn0mial commented Oct 25, 2021 •

edited by sttts