From 3af709f4830b79c268681bd830b7989eba91e2dd Mon Sep 17 00:00:00 2001 From: Lukasz Szaszkiewicz Date: Wed, 20 Oct 2021 11:06:27 +0200 Subject: [PATCH] aggregator: pass apiServiceRegistrationControllerInitiated signal directly to apiserviceRegistration controller Kubernetes-commit: 5116a508a7bf84844f4987ab2db14af88bfd296f --- pkg/endpoints/filters/mux_discovery_complete.go | 6 +++--- .../genericapiserver_graceful_termination_test.go | 6 +++--- pkg/server/genericapiserver_test.go | 14 +++++++------- pkg/util/notfoundhandler/not_found_handler.go | 3 +-- 4 files changed, 14 insertions(+), 15 deletions(-) diff --git a/pkg/endpoints/filters/mux_discovery_complete.go b/pkg/endpoints/filters/mux_discovery_complete.go index 818f648f8..d2fee3b15 100644 --- a/pkg/endpoints/filters/mux_discovery_complete.go +++ b/pkg/endpoints/filters/mux_discovery_complete.go @@ -41,9 +41,9 @@ func NoMuxAndDiscoveryIncompleteKey(ctx context.Context) bool { // // 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 -// but when the registered checks are examined in the handler they indicate that the paths have been actually installed. -// In that case, the presence of the key will make the handler return 503 instead of 404. +// The primary reason this filter exists is to protect from a potential race between the client's requests reaching the NotFoundHandler and the server becoming ready. +// Without the protection key a request could still get a 404 response when the registered signals changed their status just slightly before reaching the new handler. +// In that case, the presence of the key will make the handler return a 503 instead of a 404. func WithMuxAndDiscoveryComplete(handler http.Handler, muxAndDiscoveryCompleteSignal <-chan struct{}) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { if muxAndDiscoveryCompleteSignal != nil && !isClosed(muxAndDiscoveryCompleteSignal) { diff --git a/pkg/server/genericapiserver_graceful_termination_test.go b/pkg/server/genericapiserver_graceful_termination_test.go index 32a742852..6c84271f0 100644 --- a/pkg/server/genericapiserver_graceful_termination_test.go +++ b/pkg/server/genericapiserver_graceful_termination_test.go @@ -381,10 +381,10 @@ func TestGracefulTerminationWithKeepListeningDuringGracefulTerminationEnabled(t func TestMuxAndDiscoveryComplete(t *testing.T) { // setup - testSignal := make(chan struct{}) + testSignal1 := make(chan struct{}) testSignal2 := make(chan struct{}) s := newGenericAPIServer(t, true) - s.muxAndDiscoveryCompleteSignals["TestSignal"] = testSignal + s.muxAndDiscoveryCompleteSignals["TestSignal1"] = testSignal1 s.muxAndDiscoveryCompleteSignals["TestSignal2"] = testSignal2 doer := setupDoer(t, s.SecureServingInfo) isChanClosed := func(ch <-chan struct{}, delay time.Duration) bool { @@ -410,7 +410,7 @@ func TestMuxAndDiscoveryComplete(t *testing.T) { t.Fatalf("%s is closed whereas the TestSignal is still open", s.lifecycleSignals.MuxAndDiscoveryComplete.Name()) } - close(testSignal) + close(testSignal1) if isChanClosed(s.lifecycleSignals.MuxAndDiscoveryComplete.Signaled(), 1*time.Second) { t.Fatalf("%s is closed whereas the TestSignal2 is still open", s.lifecycleSignals.MuxAndDiscoveryComplete.Name()) } diff --git a/pkg/server/genericapiserver_test.go b/pkg/server/genericapiserver_test.go index df14f3d12..af452347c 100644 --- a/pkg/server/genericapiserver_test.go +++ b/pkg/server/genericapiserver_test.go @@ -489,32 +489,32 @@ func TestMuxAndDiscoveryCompleteSignals(t *testing.T) { // setup cfg, assert := setUp(t) - // scenario 1: single server with some mux signals + // scenario 1: single server with some signals root, err := cfg.Complete(nil).New("rootServer", NewEmptyDelegate()) assert.NoError(err) if len(root.MuxAndDiscoveryCompleteSignals()) != 0 { - assert.Error(fmt.Errorf("unexpected mux signals registered %v in the root server", root.MuxAndDiscoveryCompleteSignals())) + assert.Error(fmt.Errorf("unexpected signals %v registered in the root server", root.MuxAndDiscoveryCompleteSignals())) } root.RegisterMuxAndDiscoveryCompleteSignal("rootTestSignal", make(chan struct{})) if len(root.MuxAndDiscoveryCompleteSignals()) != 1 { - assert.Error(fmt.Errorf("unexpected mux signals registered %v in the root server", root.MuxAndDiscoveryCompleteSignals())) + assert.Error(fmt.Errorf("unexpected signals %v registered in the root server", root.MuxAndDiscoveryCompleteSignals())) } - // scenario 2: multiple servers with some mux signals + // scenario 2: multiple servers with some signals delegate, err := cfg.Complete(nil).New("delegateServer", NewEmptyDelegate()) assert.NoError(err) delegate.RegisterMuxAndDiscoveryCompleteSignal("delegateTestSignal", make(chan struct{})) if len(delegate.MuxAndDiscoveryCompleteSignals()) != 1 { - assert.Error(fmt.Errorf("unexpected mux signals registered %v in the delegate server", delegate.MuxAndDiscoveryCompleteSignals())) + assert.Error(fmt.Errorf("unexpected signals %v registered in the delegate server", delegate.MuxAndDiscoveryCompleteSignals())) } newRoot, err := cfg.Complete(nil).New("newRootServer", delegate) assert.NoError(err) if len(newRoot.MuxAndDiscoveryCompleteSignals()) != 1 { - assert.Error(fmt.Errorf("unexpected mux signals registered %v in the newRoot server", newRoot.MuxAndDiscoveryCompleteSignals())) + assert.Error(fmt.Errorf("unexpected signals %v registered in the newRoot server", newRoot.MuxAndDiscoveryCompleteSignals())) } newRoot.RegisterMuxAndDiscoveryCompleteSignal("newRootTestSignal", make(chan struct{})) if len(newRoot.MuxAndDiscoveryCompleteSignals()) != 2 { - assert.Error(fmt.Errorf("unexpected mux signals registered %v in the newRoot server", newRoot.MuxAndDiscoveryCompleteSignals())) + assert.Error(fmt.Errorf("unexpected signals %v registered in the newRoot server", newRoot.MuxAndDiscoveryCompleteSignals())) } } diff --git a/pkg/util/notfoundhandler/not_found_handler.go b/pkg/util/notfoundhandler/not_found_handler.go index 1a8d3aac1..93c1d0a04 100644 --- a/pkg/util/notfoundhandler/not_found_handler.go +++ b/pkg/util/notfoundhandler/not_found_handler.go @@ -18,7 +18,6 @@ package notfoundhandler import ( "context" - "fmt" "net/http" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -46,7 +45,7 @@ type Handler struct { 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") + errMsg := "the request has been made before all known HTTP paths have been installed, please try again" err := apierrors.NewServiceUnavailable(errMsg) if err.ErrStatus.Details == nil { err.ErrStatus.Details = &metav1.StatusDetails{}