server, ui: add multitenant login/logout and tenant dropdown

[Santamaura]

ui, server: add multitenant login/logout and tenant dropdown

This patch enables login/logout for all tenants on the cluster
by fanning out the incoming requests to each tenant server.
Multitenant login introduces a new multitenant session cookie
with the format as ,<tenant_name,,<tenant_name2>
etc. The admin ui displays a dropdown with a list of tenants
the user has successfully logged in to. Selecting a different
tenant sets the tenant cookie to the selected tenant name
and reloads the page. If the cluster is not multitenant, the
dropdown will not display.

Release note (ui change): added a top-level dropdown
on the admin ui which lists tenants the user has logged
in to. If not multitenant, the dropdown is not displayed.

Epic: https://cockroachlabs.atlassian.net/browse/CRDB-14546

[cockroach-teamcity]

This change is 

[knz]

Overall architecture LGTM. What's the followup issue/epic to discuss the semantics when one of the session expires?

This PR will also benefit from more unit tests.

Reviewed 13 of 14 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dhartunian, @koorosh, @Santamaura, and @zachlite)

---

pkg/server/api_v2_auth.go line 331 at r1 (raw file):

		var session string
		// This case is if the session cookie has a multi-tenant pattern.
		if strings.Contains(possibleSessions[i], ",") {

Please move this logic to its own function in authentication.go.
Then also please implement unit tests for that function.

---

pkg/server/api_v2_auth.go line 331 at r1 (raw file):

		var session string
		// This case is if the session cookie has a multi-tenant pattern.
		if strings.Contains(possibleSessions[i], ",") {

Also, this condition is brittle and makes the format non-extensible.
I recommend a prefix with a special name, and match that instead.

---

pkg/server/api_v2_auth.go line 336 at r1 (raw file):

				return "", nil, http.StatusBadRequest, errors.New("unable to find tenant cookie")
			}
			sessionSlice := strings.FieldsFunc(possibleSessions[i], func(r rune) bool {

I don't understand this condition. What are you trying to achieve?

---

pkg/server/authentication.go line 636 at r1 (raw file):

		// This case is if the session cookie has a multi-tenant pattern.

		if strings.Contains(c.Value, ",") {

Reuse the common function (see previous comment)

---

pkg/server/server_controller.go line 209 at r1 (raw file):

	// if the client didnt specify tenant name call these for login/logout.
	if !nameProvided {
		if r.URL.Path == loginPath {

nit: use switch

---

pkg/server/server_controller.go line 245 at r1 (raw file):

}

type sessionWriter struct {

I recommend you move this helper to a separate file.

---

pkg/server/server_controller.go line 287 at r1 (raw file):

}

// login for all tenant, find better nane

yes, please do. Also enhance the comment that explains what this function does.

---

pkg/server/server_controller.go line 289 at r1 (raw file):

// login for all tenant, find better nane
func (c *serverController) loginFanout() http.Handler {
	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {

It would also be good to sprinkle the body of the function with explanatory comments that explain what each part does. Ditto for logoutFanout.

---

pkg/server/server_controller.go line 308 at r1 (raw file):

			if err != nil {
				log.Warningf(ctx, "unable to find tserver for tenant %q: %v", name, err)
				w.WriteHeader(http.StatusInternalServerError)

missing return?

---

pkg/server/server_controller.go line 317 at r1 (raw file):

			} else {
				sessionCookieSlice := strings.Split(strings.ReplaceAll(setCookieHeader, "session=", ""), ";")
				sessionsStr += sessionCookieSlice[0] + "," + name + "&"

This composition needs to go into its own function alongside the other new one in authentication.go, so that someone working in that area can see them side-by-side.

---

pkg/server/server_controller.go line 333 at r1 (raw file):

			cookie = http.Cookie{
				Name:     TenantSelectCookieName,
				Value:    catconstants.SystemTenantName,

add a reference to #91741 in a comment around here.

---

pkg/server/server_controller.go line 368 at r1 (raw file):

			if err != nil {
				log.Warningf(ctx, "%q", err)
				w.WriteHeader(http.StatusInternalServerError)

missing return

---

pkg/server/server_controller.go line 374 at r1 (raw file):

			if err != nil {
				log.Warningf(ctx, "unable to find tserver for tenant %q: %v", name, err)
				w.WriteHeader(http.StatusInternalServerError)

missing return

---

pkg/server/server_controller.go line 379 at r1 (raw file):

			if sw.header.Get("Set-Cookie") == "" {
				log.Warningf(ctx, "logout for tenant %q failed", name)
				w.WriteHeader(http.StatusInternalServerError)

missing return

---

pkg/server/tenant.go line 112 at r1 (raw file):

	// TODO(davidh): demo only...
	baseCfg BaseConfig

This change and the one below do not belong to this PR.

---

pkg/server/testserver.go line 923 at r1 (raw file):

	}

	sw, err := ts.Server.serverController.getOrCreateServer(ctx, string(params.TenantName))

This change does not belong to this PR.

---

pkg/ui/workspaces/db-console/src/redux/alerts.ts line 657 at r1 (raw file):

    if (
      !state.login ||

Why did this need to change?

---

pkg/ui/workspaces/db-console/src/redux/login.ts line 240 at r1 (raw file):

    const tenants = selectTenantsFromCookies();
    // If in multi-tenant environment, we need to clear the tenant cookie so that
    // we can do a multi-tenant logout.

IMHO we should have two options: "log out from this tenant" vs "log out from all tenants"

[koorosh]

Reviewed 5 of 14 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dhartunian, @Santamaura, and @zachlite)

---

pkg/ui/workspaces/db-console/src/components/dropdown/dropdown.tsx line 33 at r1 (raw file):

  className?: string;
  menuPlacement?: "right" | "left";
  icon?: JSX.Element;

cluster-ui package contains another version of Dropdown component that allows to customize icon. See example: pkg/ui/workspaces/cluster-ui/src/dropdown/dropdown.stories.tsx:34

---

pkg/ui/workspaces/db-console/src/redux/tenantOptions.ts line 13 at r1 (raw file):

  const sessionCookieStr = document.cookie
    .split(";")
    .filter(row => row.trim().startsWith("session="))[0];

it is not safe to access [0]s element without assertion that array isn't empty.

---

pkg/ui/workspaces/db-console/src/redux/tenantOptions.ts line 17 at r1 (raw file):

    ? sessionCookieStr
        .replace(/["]/g, "")
        .replace("session=", "")

Why session= is replaced?

---

pkg/ui/workspaces/db-console/src/views/app/components/tenantDropdown/tenantDropdown.tsx line 36 at r1 (raw file):

  const setTenantCookie = (tenant: string) => {
    if (tenant !== currentTenant) {
      document.cookie = `${tenantIDKey}=${tenant};path=/`;

I suggest to define sagas to perform side effects like writing or reading cookies from document object.

[dhartunian]

Can you sync the PR description and commit messages? They shouldn't be different.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Santamaura and @zachlite)

---

pkg/server/server_controller.go line 221 at r1 (raw file):

}

func getTenantNameFromHTTPRequest(r *http.Request) (string, bool) {

is the bool necessary here or can we just check if the name returned is not the system tenant?

---

pkg/ui/workspaces/db-console/src/redux/alerts.ts line 657 at r1 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

Why did this need to change?

This was an unrelated bug that were trying to squash while working. @Santamaura can you lift the login-fail bug code out of here? It should be in a separate PR.

---

pkg/ui/workspaces/db-console/src/redux/tenantOptions.ts line 21 at r1 (raw file):

        .filter((_, idx) => idx % 2 == 1)
    : [];
};

any chance we can abstract all the cookie manipulation into a "library" in a single file? there's a lot of spread out code that deals with the cookies and it's tough to know if it's internally consistent and it's not well-tested.

[Santamaura]

Yep, copied it over to the PR desc.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dhartunian, @knz, @koorosh, and @zachlite)

---

pkg/server/api_v2_auth.go line 331 at r1 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

Please move this logic to its own function in authentication.go.
Then also please implement unit tests for that function.

Done.

---

pkg/server/api_v2_auth.go line 331 at r1 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

Also, this condition is brittle and makes the format non-extensible.
I recommend a prefix with a special name, and match that instead.

IIUC you want to add something to the session cookie value before the actual values, so the format will become e.g. session=<some_prefix>:<session>,<tenant_name>&<session2>,<tenant_name2> etc? If so, any idea for the prefix, maybe multi_tenant:?

---

pkg/server/api_v2_auth.go line 336 at r1 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

I don't understand this condition. What are you trying to achieve?

My reasoning was if for some reason the tenant value is an empty string, then there is something not right and we should return an error. If you think it's unnecessary I can remove it.

---

pkg/server/authentication.go line 636 at r1 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

Reuse the common function (see previous comment)

Done.

---

pkg/server/server_controller.go line 209 at r1 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

nit: use switch

Done.

---

pkg/server/server_controller.go line 221 at r1 (raw file):

Previously, dhartunian (David Hartunian) wrote…

is the bool necessary here or can we just check if the name returned is not the system tenant?

I think we do actually need the bool because it distinguishes between whether the client actually provided a value or not. If the client provides no tenant cookie then we should do multi-tenant login/logout.

---

pkg/server/server_controller.go line 245 at r1 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

I recommend you move this helper to a separate file.

Done.

---

pkg/server/server_controller.go line 287 at r1 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

yes, please do. Also enhance the comment that explains what this function does.

Done.

---

pkg/server/server_controller.go line 289 at r1 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

It would also be good to sprinkle the body of the function with explanatory comments that explain what each part does. Ditto for logoutFanout.

Done.

---

pkg/server/server_controller.go line 308 at r1 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

missing return?

Done.

---

pkg/server/server_controller.go line 317 at r1 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

This composition needs to go into its own function alongside the other new one in authentication.go, so that someone working in that area can see them side-by-side.

Done.

---

pkg/server/server_controller.go line 333 at r1 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

add a reference to #91741 in a comment around here.

Done.

---

pkg/server/server_controller.go line 368 at r1 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

missing return

Done.

---

pkg/server/server_controller.go line 374 at r1 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

missing return

Done.

---

pkg/server/server_controller.go line 379 at r1 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

missing return

Done.

---

pkg/ui/workspaces/db-console/src/redux/login.ts line 240 at r1 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

IMHO we should have two options: "log out from this tenant" vs "log out from all tenants"

Filed an issue see #92855.

---

pkg/ui/workspaces/db-console/src/views/app/components/tenantDropdown/tenantDropdown.tsx line 36 at r1 (raw file):

Previously, koorosh (Andrii Vorobiov) wrote…

I suggest to define sagas to perform side effects like writing or reading cookies from document object.

Talked with CRUX about this because I was unsure, the consensus was we don't think it's necessary to have it in sagas because we don't have the cookies using any data from the redux store. If you feel strongly about it we discuss more though.

---

pkg/server/tenant.go line 112 at r1 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

This change and the one below do not belong to this PR.

Removed.

---

pkg/server/testserver.go line 923 at r1 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

This change does not belong to this PR.

Removed.

---

pkg/ui/workspaces/db-console/src/components/dropdown/dropdown.tsx line 33 at r1 (raw file):

Previously, koorosh (Andrii Vorobiov) wrote…

cluster-ui package contains another version of Dropdown component that allows to customize icon. See example: pkg/ui/workspaces/cluster-ui/src/dropdown/dropdown.stories.tsx:34

Thanks for pointing this out! I should have checked if there was one in cluster-ui. I've switched to using the cluster-ui one and reverted the changes to the other dropdown.

---

pkg/ui/workspaces/db-console/src/redux/alerts.ts line 657 at r1 (raw file):

Previously, dhartunian (David Hartunian) wrote…

This was an unrelated bug that were trying to squash while working. @Santamaura can you lift the login-fail bug code out of here? It should be in a separate PR.

Removed, will add to separate PR.

---

pkg/ui/workspaces/db-console/src/redux/tenantOptions.ts line 13 at r1 (raw file):

Previously, koorosh (Andrii Vorobiov) wrote…

it is not safe to access [0]s element without assertion that array isn't empty.

Accessing arr[0] of an empty array will just return undefined. Either way, I moved around the index access so that it is "safer".

---

pkg/ui/workspaces/db-console/src/redux/tenantOptions.ts line 17 at r1 (raw file):

Previously, koorosh (Andrii Vorobiov) wrote…

Why session= is replaced?

the raw cookie will return in the format of session=abcd1234,system etc. so we need to strip the session= since we just want tenant names.

---

pkg/ui/workspaces/db-console/src/redux/tenantOptions.ts line 21 at r1 (raw file):

Previously, dhartunian (David Hartunian) wrote…

any chance we can abstract all the cookie manipulation into a "library" in a single file? there's a lot of spread out code that deals with the cookies and it's tough to know if it's internally consistent and it's not well-tested.

Sure, I've renamed the file to cookies.ts and moved all document.cookie logic into there. We can keep all cookie related code in there WDYT?

[Santamaura]

If there's any further comments let me know, otherwise it would be nice to get a thumbs up as I'd like to merge this before I go OOO next week.

[knz]

No more comment from me.

I stand by this - and you can get your review approval from your team.

[zachlite]

Reviewed 2 of 14 files at r1, 1 of 1 files at r4, 1 of 8 files at r7, 4 of 7 files at r8, 1 of 1 files at r9, 1 of 4 files at r10, 1 of 2 files at r12, 3 of 3 files at r13, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dhartunian, @knz, and @koorosh)

[Santamaura]

bors r+

[craig]

👎 Rejected by code reviews

addressed review and have approval

[Santamaura]

bors r+

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build failed:

• Bazel Essential CI (Cockroach)

[Santamaura]

bors retry

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)