server/ui: fix crash caused by race in server initialization #30746
Conversation
|
This change is |
vilterp
force-pushed
the
fix-ui-auth-mux-race
branch
from
1fbf38a
to
5be3275
Compare
There was a problem hiding this comment.
The commits you wrote 
Reviewed 1 of 1 files at r1, 4 of 4 files at r2, 3 of 3 files at r3.
Reviewable status:complete! 1 of 0 LGTMs obtained
pkg/server/server.go, line 1583 at r3 (raw file):
// Serve UI assets. This needs to be before the gRPC handlers are registered, otherwise // the `s.mux.Handle("/", ...)` would cover all URLs, allowing anonymous access.
I know you're just copy/pasting, but I don't understand what this paragraph is saying. And whatever it's saying I'm not sure is true? I don't see why the order in which the routes are installed would matter.
pkg/ui/ui.go, line 127 at r3 (raw file):
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { fmt.Println("hello from ")
stray
There was a problem hiding this comment.
LGTM, thanks for getting to the bottom of this.
Reviewable status:
There was a problem hiding this comment.
Reviewed 3 of 4 files at r2, 3 of 3 files at r3.
Reviewable status:
HTTP requests are passed through an authentication mux, which checks that the user is authenticated based on the cookies on the request. To check authentication, the mux runs SQL to look up a session in the system.web_sessions table. Prior to this change, it was possible to hit the cluster with an HTTP request which would cause the SQL to be run, before the SQL system was fully initialized -- specifically before the cluster version setting was initialized. This would crash the node. This change delays installation of the authentication mux until after the SQL system has been initialized, removing the possibility of this crash. Release note: None
Teach the cluster-init test to reproduce the crash during bootup described in issue cockroachdb#25771. The trick is to send requests with a session cookie, as the admin UI would, while the cluster is waiting for init. Note that the crash was caused by the authenticationMux, which is used to protect secure clusters, but this test uses an insecure cluster. This is relying on a wart: an authenticationMux is installed for UI assets whether or not the cluster is secure. This could change in the future. A better test would use a secure cluster, however roachprod/roachtest do not easily support that at this time, nor is it easy to write a unit test that exercises the crash more robustly. Release note: None
Including - more comments - rename `maybeAuthMux` to `authenticatedUIHandler` - move `authHandler` down near where it's initialized Release note: None
There was a problem hiding this comment.
Reviewable status:
pkg/server/server.go, line 1583 at r3 (raw file):
Previously, benesch (Nikhil Benesch) wrote…
I know you're just copy/pasting, but I don't understand what this paragraph is saying. And whatever it's saying I'm not sure is true? I don't see why the order in which the routes are installed would matter.
Removed. The order can matter when there is some time between registrations (as there used to be in this method) but that's not really what the comment is explaining. Not quite sure what I was thinking when I wrote it.
pkg/ui/ui.go, line 127 at r3 (raw file):
Previously, benesch (Nikhil Benesch) wrote…
stray
Done.
vilterp
force-pushed
the
fix-ui-auth-mux-race
branch
from
5be3275
to
b617f1c
Compare
|
TFTRs! bors r+ |
30746: server/ui: fix crash caused by race in server initialization r=vilterp a=vilterp Fixes #25771 See commits for details. Co-authored-by: Pete Vilter <[email protected]> Co-authored-by: Nikhil Benesch <[email protected]>
Build succeeded |
| // assets are served up whether or not there is a session. If there is a session, the mux | ||
| // adds it to the context, and it is templated into index.html so that the UI can show | ||
| // the username of the currently-logged-in user. | ||
| authenticatedUIHandler := newAuthenticationMuxAllowAnonymous( |
There was a problem hiding this comment.
nit: I think mux is more accurate than handler for this, since it's not handling any requests itself.
There was a problem hiding this comment.
it's a handler wrapped in an auth mux (which fulfills the http.Handler) interface, which is passed to s.mux.Handle(path, http.Handler)… so ¯\_(ツ)_/¯
Fixes #25771
See commits for details.