Skip to content

Commit

Permalink
servercontroller: serve http from default tenant on error
Browse files Browse the repository at this point in the history
Previously, when an HTTP request arrived with a tenant cookie attached, we
would attempt to connect to that tenant and fail if the tenant couldn't be
started or if it didn't exist. This causes a bad user-facing experience when
someone's browser contains stale tenant cookies and they attempt to load DB
Console. The error returns a 500 to the browser and user sees no UI, can't try
and login again, and is basically stuck until they clear their cookies.

This change falls back to serving HTTP from the default tenant when the
requested one can't be reached. This ensures that *something* is served to
ensure that unauthenticated endpoints can still be loaded in these scenarios
and users can attempt to login again.

Epic: CRDB-12100

Release note: None
  • Loading branch information
dhartunian committed Feb 9, 2023
1 parent f8eb2ef commit e496d93
Show file tree
Hide file tree
Showing 2 changed files with 56 additions and 2 deletions.
42 changes: 42 additions & 0 deletions pkg/ccl/serverccl/server_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,48 @@ VALUES($1, $2, $3, $4, $5)`, id, secret, username, created, expires)
require.Equal(t, body.Sessions[0].ApplicationName, "hello system")
}

// TestServerControllerBadHTTPCookies tests the controller's proxy
// layer for correct behavior under scenarios where the client has
// stale or invalid cookies. This helps ensure that we continue to
// serve static assets even when the browser is referencing an
// unknown tenant, or bad sessions.
func TestServerControllerBadHTTPCookies(t *testing.T) {
defer leaktest.AfterTest(t)()
defer log.Scope(t).Close(t)

ctx := context.Background()

s, _, _ := serverutils.StartServer(t, base.TestServerArgs{})
defer s.Stopper().Stop(ctx)

client, err := s.GetUnauthenticatedHTTPClient()
require.NoError(t, err)

c := &http.Cookie{
Name: server.TenantSelectCookieName,
Value: "some-nonexistent-tenant",
Path: "/",
HttpOnly: true,
Secure: true,
}

req, err := http.NewRequest("GET", s.AdminURL()+"/", nil)
require.NoError(t, err)
req.AddCookie(c)
resp, err := client.Do(req)
require.NoError(t, err)
defer resp.Body.Close()
require.Equal(t, 200, resp.StatusCode)

req, err = http.NewRequest("GET", s.AdminURL()+"/bundle.js", nil)
require.NoError(t, err)
req.AddCookie(c)
resp, err = client.Do(req)
require.NoError(t, err)
defer resp.Body.Close()
require.Equal(t, 200, resp.StatusCode)
}

func TestServerStartStop(t *testing.T) {
defer leaktest.AfterTest(t)()
defer log.Scope(t).Close(t)
Expand Down
16 changes: 14 additions & 2 deletions pkg/server/server_controller_http.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ func (c *serverController) httpMux(w http.ResponseWriter, r *http.Request) {
s, err := c.getServer(ctx, tenantName)
if err != nil {
log.Warningf(ctx, "unable to find server for tenant %q: %v", tenantName, err)
// Clear session and tenant cookies after all logouts have completed.
// Clear session and tenant cookies since it appears they reference invalid state.
http.SetCookie(w, &http.Cookie{
Name: MultitenantSessionCookieName,
Value: "",
Expand All @@ -79,7 +79,19 @@ func (c *serverController) httpMux(w http.ResponseWriter, r *http.Request) {
HttpOnly: false,
Expires: timeutil.Unix(0, 0),
})
w.WriteHeader(http.StatusInternalServerError)
// Fall back to serving requests from the default tenant. This helps us serve
// the root path along with static assets even when the browser contains invalid
// tenant names or sessions (common during development). Otherwise the user can
// get into a state where they cannot load DB Console assets at all due to invalid
// cookies.
defaultTenantName := roachpb.TenantName(defaultTenantSelect.Get(&c.st.SV))
s, err = c.getServer(ctx, defaultTenantName)
if err != nil {
log.Warningf(ctx, "unable to find server for default tenant %q: %v", defaultTenantName, err)
w.WriteHeader(http.StatusInternalServerError)
return
}
s.getHTTPHandlerFn()(w, r)
return
}
s.getHTTPHandlerFn()(w, r)
Expand Down

0 comments on commit e496d93

Please sign in to comment.