From 06b8d00e9048d9483a985eb66bae448e8ec965e4 Mon Sep 17 00:00:00 2001 From: Argus Li Date: Wed, 21 Oct 2026 22:44:07 -0700 Subject: [PATCH 1/9] Require AUTH before client reuse and limit failed AUTH Before the server reuses an existing Valkey client, a short-lived client must authenticate with the presented credentials. A retry closes the stale client only after the new client authenticates. Failed AUTH attempts are counted per node and owner identity. After 10 failures in 60 seconds, the server rejects new attempts without contacting Valkey. Signed-off-by: Argus Li --- .../server/src/__tests__/auth-limiter.test.ts | 149 ++++++++++++++++++ apps/server/src/__tests__/connection.test.ts | 73 ++++++++- apps/server/src/__tests__/owner.test.ts | 56 +++++++ apps/server/src/auth-limiter.ts | 66 ++++++++ apps/server/src/connection.ts | 52 ++++-- apps/server/src/owner.ts | 24 +++ 6 files changed, 403 insertions(+), 17 deletions(-) create mode 100644 apps/server/src/__tests__/auth-limiter.test.ts create mode 100644 apps/server/src/__tests__/owner.test.ts create mode 100644 apps/server/src/auth-limiter.ts create mode 100644 apps/server/src/owner.ts diff --git a/apps/server/src/__tests__/auth-limiter.test.ts b/apps/server/src/__tests__/auth-limiter.test.ts new file mode 100644 index 00000000..9c13c944 --- /dev/null +++ b/apps/server/src/__tests__/auth-limiter.test.ts @@ -0,0 +1,149 @@ +/* eslint-disable @typescript-eslint/no-explicit-any */ +import { describe, it, beforeEach, afterEach, mock } from "node:test" +import assert from "node:assert" +import { GlideClient, ClosingError } from "@valkey/valkey-glide" +import { VALKEY } from "valkey-common" +import { + AUTH_FAILURE_LIMIT, + authLimitKey, + isAuthError, + _resetAuthLimiter +} from "../auth-limiter" +import { + _resetConnectInFlight, + _resetInFlightClusterClients, + connectToValkey, + discoverTopology +} from "../connection" +import { _reset as resetNodeWatchers } from "../node-watchers" +import type { ConnectionDetails } from "../actions/connection" + +const DETAILS: ConnectionDetails = { + host: "127.0.0.1", + port: "6379", + username: "alice", + password: "wrong", + tls: false, + verifyTlsCertificate: false, + endpointType: "node", + db: 0, +} + +const wrongPass = () => + new ClosingError( + "Connection error: Standalone(Received error for address `127.0.0.1:6379`: " + + "WRONGPASS: invalid username-password pair or user is disabled.\n)", + ) + +describe("auth-limiter", () => { + let messages: any[] + let ws: any + let createCalls: number + let originalCreate: typeof GlideClient.createClient + let realNow: () => number + let now: number + + const ctx = () => ({ + clients: new Map(), + connectedNodesByCluster: new Map(), + clusterNodesRegistry: new Map(), + metricsServerMap: new Map(), + }) + + const connect = (details: ConnectionDetails = DETAILS, connectionId = "conn-1") => + connectToValkey(ctx() as any, ws, { connectionDetails: details, connectionId, sessionId: "s" } as any) + + const lastType = () => messages[messages.length - 1]?.type + const lastPayload = () => messages[messages.length - 1]?.payload + + beforeEach(() => { + _resetAuthLimiter() + _resetConnectInFlight() + _resetInFlightClusterClients() + resetNodeWatchers() + messages = [] + ws = { send: (m: string) => messages.push(JSON.parse(m)), readyState: 1, OPEN: 1 } + createCalls = 0 + originalCreate = GlideClient.createClient + GlideClient.createClient = (async () => { + createCalls++ + throw wrongPass() + }) as any + realNow = Date.now + now = 1_000_000_000 + Date.now = () => now + mock.method(console, "error", () => {}) + mock.method(console, "warn", () => {}) + }) + + afterEach(() => { + GlideClient.createClient = originalCreate + Date.now = realNow + mock.restoreAll() + }) + + it("classifies authentication errors only", () => { + assert.strictEqual(isAuthError(wrongPass()), true) + assert.strictEqual(isAuthError(new Error("NOAUTH HELLO must be called with the client already authenticated")), true) + assert.strictEqual(isAuthError(new Error("Connection refused")), false) + assert.strictEqual(isAuthError(new Error("PermissionDenied: User x has no permissions to run the 'info' command")), false) + }) + + it("keys by host:port and owner, not by connectionId or db", () => { + assert.strictEqual(authLimitKey({ ...DETAILS, db: 3 }), authLimitKey(DETAILS)) + assert.notStrictEqual(authLimitKey({ ...DETAILS, username: "bob" }), authLimitKey(DETAILS)) + assert.notStrictEqual(authLimitKey({ ...DETAILS, port: "6380" }), authLimitKey(DETAILS)) + }) + + it(`rejects without contacting Valkey after ${AUTH_FAILURE_LIMIT} failures`, async () => { + for (let i = 0; i < AUTH_FAILURE_LIMIT; i++) { + await connect() + assert.strictEqual(lastType(), VALKEY.CONNECTION.connectRejected) + } + assert.strictEqual(createCalls, AUTH_FAILURE_LIMIT) + + await connect() + assert.strictEqual(createCalls, AUTH_FAILURE_LIMIT, "a locked key must not reach Valkey") + assert.strictEqual(lastType(), VALKEY.CONNECTION.connectRejected) + assert.match(lastPayload().errorMessage, /Too many failed authentication attempts/) + }) + + it("is not bypassed by changing the connectionId", async () => { + for (let i = 0; i < AUTH_FAILURE_LIMIT; i++) await connect(DETAILS, `conn-${i}`) + await connect(DETAILS, "conn-new") + assert.strictEqual(createCalls, AUTH_FAILURE_LIMIT) + }) + + it("does not block another owner or another node", async () => { + for (let i = 0; i < AUTH_FAILURE_LIMIT; i++) await connect() + await connect({ ...DETAILS, username: "bob" }) + await connect({ ...DETAILS, port: "6380" }, "conn-6380") + assert.strictEqual(createCalls, AUTH_FAILURE_LIMIT + 2) + }) + + it("does not count errors that are not authentication failures", async () => { + GlideClient.createClient = (async () => { + createCalls++ + throw new Error("Connection refused") + }) as any + for (let i = 0; i < AUTH_FAILURE_LIMIT + 2; i++) await connect() + assert.strictEqual(createCalls, AUTH_FAILURE_LIMIT + 2) + }) + + it("unlocks when the window expires", async () => { + for (let i = 0; i < AUTH_FAILURE_LIMIT; i++) await connect() + now += 61_000 + await connect() + assert.strictEqual(createCalls, AUTH_FAILURE_LIMIT + 1) + }) + + it("also limits topology discovery", async () => { + for (let i = 0; i < AUTH_FAILURE_LIMIT; i++) { + await discoverTopology(ws, { discoveryId: "d", connectionDetails: DETAILS }) + } + await discoverTopology(ws, { discoveryId: "d", connectionDetails: DETAILS }) + assert.strictEqual(createCalls, AUTH_FAILURE_LIMIT) + assert.strictEqual(lastType(), VALKEY.TOPOLOGY.discoveryEndpointRejected) + assert.match(lastPayload().errorMessage, /Too many failed authentication attempts/) + }) +}) diff --git a/apps/server/src/__tests__/connection.test.ts b/apps/server/src/__tests__/connection.test.ts index fb79df24..9d4c9b27 100644 --- a/apps/server/src/__tests__/connection.test.ts +++ b/apps/server/src/__tests__/connection.test.ts @@ -276,10 +276,12 @@ describe("connectToValkey", () => { connectToValkey(ctx(), mockWs, payload), ]) + // The second connector reuses the first one's client, but only after a + // short-lived client proved its credentials (AUTH-gated reuse): 1 client + 1 probe. assert.strictEqual( (GlideClient.createClient as any).mock.calls.length, - 1, - "expected exactly one standalone client creation", + 2, + "expected one standalone client and one AUTH probe", ) assert.strictEqual(a, standalone) assert.strictEqual(b, standalone) @@ -748,6 +750,73 @@ describe("connectToValkey", () => { }) }) + describe("AUTH-gated reuse", () => { + const wrongPass = () => new Error("WRONGPASS: invalid username-password pair or user is disabled.") + + it("authenticates the presented credentials before it reuses an existing client", async () => { + const existing = buildStandaloneMock() + const probe = buildStandaloneMock() + clients.set("conn-R", { client: existing }) + authorizeConnection(testSessionId, "conn-R") + + await withMockedClients(probe, null, async () => { + const result = await connectToValkey(ctx(), mockWs, { ...DEFAULT_PAYLOAD, connectionId: "conn-R" }) + + const calls = (GlideClient.createClient as any).mock.calls + assert.strictEqual(calls.length, 1, "reuse must open exactly one probe client") + assert.deepStrictEqual(calls[0].arguments[0].credentials, { + username: DEFAULT_PAYLOAD.connectionDetails.username, + password: DEFAULT_PAYLOAD.connectionDetails.password, + }) + assert.strictEqual(probe.close.mock.calls.length, 1, "the probe client must be closed") + assert.strictEqual(result, existing) + assert.strictEqual(clients.get("conn-R")?.client, existing) + }) + }) + + it("rejects a wrong password and leaves the existing client open", async () => { + const existing = buildStandaloneMock() + clients.set("conn-R", { client: existing }) + authorizeConnection(testSessionId, "conn-R") + const original = GlideClient.createClient + GlideClient.createClient = mock.fn(async () => { throw wrongPass() }) as any + try { + const result = await connectToValkey(ctx(), mockWs, { + ...DEFAULT_PAYLOAD, + connectionId: "conn-R", + connectionDetails: { ...DEFAULT_PAYLOAD.connectionDetails, password: "wrong" }, + }) + assert.strictEqual(result, undefined) + const sent = messages.map((m) => JSON.parse(m)) + assert.strictEqual(sent.at(-1).type, VALKEY.CONNECTION.connectRejected) + assert.strictEqual(existing.close.mock.calls.length, 0, "the existing client must stay open") + assert.strictEqual(clients.get("conn-R")?.client, existing) + } finally { + GlideClient.createClient = original + } + }) + + it("does not close the existing client when a retry fails to authenticate", async () => { + const existing = buildStandaloneMock() + clients.set("conn-R", { client: existing }) + authorizeConnection(testSessionId, "conn-R") + const original = GlideClient.createClient + GlideClient.createClient = mock.fn(async () => { throw wrongPass() }) as any + try { + await connectToValkey(ctx(), mockWs, { + ...DEFAULT_PAYLOAD, + connectionId: "conn-R", + connectionDetails: { ...DEFAULT_PAYLOAD.connectionDetails, password: "wrong" }, + isRetry: true, + }) + assert.strictEqual(existing.close.mock.calls.length, 0, "a failed retry must not close the client") + assert.strictEqual(clients.get("conn-R")?.client, existing) + } finally { + GlideClient.createClient = original + } + }) + }) + it("skips the proactive range check when CONFIG GET is denied and still connects", async () => { await withMockedClients(buildStandaloneMock({ configGetError: true }), null, async () => { const connectionId = buildConnectionId("127.0.0.1", "6379", 20) diff --git a/apps/server/src/__tests__/owner.test.ts b/apps/server/src/__tests__/owner.test.ts new file mode 100644 index 00000000..e1c6ea11 --- /dev/null +++ b/apps/server/src/__tests__/owner.test.ts @@ -0,0 +1,56 @@ +import { describe, it } from "node:test" +import assert from "node:assert" +import { ownerOf } from "../owner" +import type { ConnectionDetails } from "../actions/connection" + +const base: ConnectionDetails = { + host: "127.0.0.1", + port: "6379", + tls: false, + verifyTlsCertificate: false, + endpointType: "node", + db: 0, +} + +describe("ownerOf", () => { + it("keys password auth by username", () => { + assert.strictEqual(ownerOf({ ...base, username: "alice", password: "pw" }), "password~alice") + }) + + it("uses default when the username is empty or absent", () => { + assert.strictEqual(ownerOf({ ...base, username: "", password: "pw" }), "password~default") + assert.strictEqual(ownerOf({ ...base, password: "pw" }), "password~default") + }) + + // Without a password Glide sends no credentials, so the connection runs as `default` + // whatever username was typed. Naming a user must not give access to that user's client. + it("uses default when there is no password, even with a username", () => { + assert.strictEqual(ownerOf({ ...base, username: "admin_user" }), "password~default") + assert.strictEqual(ownerOf({ ...base, username: "admin_user", password: "" }), "password~default") + }) + + it("does not ignore case in the username", () => { + assert.notStrictEqual( + ownerOf({ ...base, username: "Alice", password: "pw" }), + ownerOf({ ...base, username: "alice", password: "pw" }), + ) + }) + + it("keys ElastiCache IAM by region, replication group and username", () => { + assert.strictEqual( + ownerOf({ ...base, authType: "iam", username: "iam-user", awsRegion: "us-east-1", awsReplicationGroupId: "my-rg" }), + "iam~us-east-1~my-rg~iam-user", + ) + }) + + it("keys GCP IAM as default", () => { + assert.strictEqual(ownerOf({ ...base, authType: "gcp-iam", username: "ignored" }), "gcp-iam~default") + }) + + it("does not depend on the password value", () => { + assert.strictEqual( + ownerOf({ ...base, username: "alice", password: "one" }), + ownerOf({ ...base, username: "alice", password: "two" }), + ) + }) +}) diff --git a/apps/server/src/auth-limiter.ts b/apps/server/src/auth-limiter.ts new file mode 100644 index 00000000..650c0ea4 --- /dev/null +++ b/apps/server/src/auth-limiter.ts @@ -0,0 +1,66 @@ +import { MemoryStore } from "express-rate-limit" +import { buildConnectionId, toNodeId } from "valkey-common" +import { ownerOf } from "./owner" +import type { ConnectionDetails } from "./actions/connection" + +/** + * Failed-AUTH limit for user-supplied credentials. + * + * Valkey does not limit AUTH attempts, and every connect request becomes one, + * so without this limit the server could be used to guess passwords. Failures + * are counted per (node, owner identity) in a fixed window. The key is built + * from the connection details, not from the client-supplied connectionId, so + * a new connectionId does not reset the count. + */ +export const AUTH_FAILURE_LIMIT = 10 +export const AUTH_FAILURE_WINDOW_MS = 60_000 +export const AUTH_LOCKED_MESSAGE = "Too many failed authentication attempts. Try again later." + +let store = new MemoryStore() +store.init({ windowMs: AUTH_FAILURE_WINDOW_MS } as Parameters[0]) + +export const _resetAuthLimiter = (): void => { + store.shutdown() + store = new MemoryStore() + store.init({ windowMs: AUTH_FAILURE_WINDOW_MS } as Parameters[0]) +} + +export const authLimitKey = (details: ConnectionDetails): string => + `${toNodeId(buildConnectionId(details.host, details.port))}|${ownerOf(details)}` + +const AUTH_ERROR_PATTERN = /WRONGPASS|NOAUTH|invalid username-password pair|user is disabled/i + +export const isAuthError = (err: unknown): boolean => + AUTH_ERROR_PATTERN.test(err instanceof Error ? err.message : String(err)) + +export class AuthLockedError extends Error { + constructor() { + super(AUTH_LOCKED_MESSAGE) + this.name = "AuthLockedError" + } +} + +// MemoryStore.get() can return a counter from an expired window until its +// next sweep, so the reset time is checked here. +const isLocked = async (key: string): Promise => { + const entry = await store.get(key) + if (!entry) return false + if (entry.resetTime && entry.resetTime.getTime() <= Date.now()) return false + return entry.totalHits >= AUTH_FAILURE_LIMIT +} + +/** + * Run `fn`, which authenticates with the credentials in `details`. Throws + * `AuthLockedError` without calling `fn` when the key is locked, and counts + * a failure when `fn` fails with an authentication error. + */ +export async function withAuthLimit(details: ConnectionDetails, fn: () => Promise): Promise { + const key = authLimitKey(details) + if (await isLocked(key)) throw new AuthLockedError() + try { + return await fn() + } catch (err) { + if (isAuthError(err)) await store.increment(key) + throw err + } +} diff --git a/apps/server/src/connection.ts b/apps/server/src/connection.ts index d34b160e..1ba6375f 100644 --- a/apps/server/src/connection.ts +++ b/apps/server/src/connection.ts @@ -29,6 +29,7 @@ import { subscribe } from "./node-watchers" import { clearCpuSamples } from "./node-utilization" import { createClusterValkeyClient, createStandaloneValkeyClient } from "./valkey-client" import { isConnectionAuthorized } from "./session" +import { withAuthLimit } from "./auth-limiter" export type ConnectionContext = { clients: Map @@ -134,6 +135,21 @@ const connectInFlight = new Map connectInFlight.clear() +/** + * Prove that Valkey accepts `details`' credentials on this node: open a + * short-lived client, then close it. Throws on rejection. Counted by the + * failed-AUTH limit. Valkey Admin never stores or compares secrets. + */ +async function verifyCredentials( + details: ConnectionDetails, + options: Parameters[0], +): Promise { + const probe = await withAuthLimit(details, () => createStandaloneValkeyClient(options)) + try { probe.close() } catch (err) { + console.warn("Error closing AUTH probe client:", err) + } +} + export async function connectToValkey( ctx: ConnectionContext, ws: WebSocket, @@ -213,21 +229,25 @@ async function connectToValkeyLocked( ) } - // If retrying, we need to close stale client - if (payload.isRetry) { - const existing = clients.get(connectionId) - // Only close standalone clients here. Cluster clients are shared across - // slots; let updateClusterNodesClient handle the close + repoint atomically. - if (existing && existing.client instanceof GlideClient) { - try { existing.client.close() } catch (error) { - console.log(`Error closing stale client for ${connectionId}:`, error) - } + // A retry replaces the stale client, but only after the new client has + // authenticated below, so a retry with wrong credentials cannot close a + // client that works. Cluster clients are repointed in updateClusterNodesClient. + const staleStandalone = payload.isRetry ? clients.get(connectionId)?.client : undefined + const replaceStale = () => { + if (!(staleStandalone instanceof GlideClient)) return + if (clients.get(connectionId)?.client === staleStandalone) clients.delete(connectionId) + try { staleStandalone.close() } catch (error) { + console.log(`Error closing stale client for ${connectionId}:`, error) } - clients.delete(connectionId) } const existingConnection = await getExistingConnection(payload, clients) if (existingConnection) { + // Owner identity is not secret, so it never grants reuse by itself. Valkey + // must accept the presented credentials before the existing client is handed over. + await verifyCredentials(payload.connectionDetails, { + addresses, credentials, useTLS, verifyTlsCertificate, caCertPath, + }) if (existingConnection.clusterId) { const clusterClient = existingConnection.client as GlideClusterClient const { clusterId } = existingConnection @@ -260,13 +280,14 @@ async function connectToValkeyLocked( return existingStandalone } - standaloneClient = await createStandaloneValkeyClient({ + standaloneClient = await withAuthLimit(payload.connectionDetails, () => createStandaloneValkeyClient({ addresses, credentials, useTLS, verifyTlsCertificate, caCertPath, - }) + })) + replaceStale() // Open the registration gate: the metrics process spawned below will POST // /register back to the orchestrator and the handler checks clients.has(connectionId). @@ -415,14 +436,14 @@ async function connectToValkeyLocked( try { previousProbe.close() } catch (err) { console.error(`Error closing standalone probe for ${connectionId}:`, err) } - standaloneClient = await createStandaloneValkeyClient({ + standaloneClient = await withAuthLimit(payload.connectionDetails, () => createStandaloneValkeyClient({ addresses, credentials, useTLS, verifyTlsCertificate, caCertPath, databaseId: db, - }) + })) clients.set(connectionId, { client: standaloneClient }) } @@ -522,7 +543,8 @@ export async function discoverTopology( // database selection is irrelevant for cluster commands. Skip // `databaseId` entirely so Glide does not issue `SELECT` (cluster nodes // reject `SELECT` even for db 0). - client = await createStandaloneValkeyClient({ addresses, credentials, useTLS, verifyTlsCertificate, caCertPath }) + client = await withAuthLimit(connectionDetails, () => + createStandaloneValkeyClient({ addresses, credentials, useTLS, verifyTlsCertificate, caCertPath })) const { discoveredClusterNodes } = await discoverCluster(client, { connectionDetails }) if (Object.keys(discoveredClusterNodes).length < 1) { throw new Error("Unable to discover cluster") diff --git a/apps/server/src/owner.ts b/apps/server/src/owner.ts new file mode 100644 index 00000000..a6eae744 --- /dev/null +++ b/apps/server/src/owner.ts @@ -0,0 +1,24 @@ +import type { ConnectionDetails } from "./actions/connection" + +/** + * Owner identity: the Valkey identity a connection authenticated as. + * + * Every piece of server state that holds a client, a collector or per-cluster + * data is keyed by this value, so users who authenticate as different Valkey + * users share nothing. The key is not secret and never grants access by + * itself: reuse of an existing client is gated by a fresh AUTH. + * + * - password: `password~`. Without a password Glide sends no + * credentials and the connection runs as `default`, whatever username the + * user typed, so the owner is `default` too. + * - ElastiCache IAM: `iam~~~`. + * - GCP IAM: `gcp-iam~default` (the only user GCP IAM supports). + */ +export const ownerOf = (details: ConnectionDetails): string => { + if (details.authType === "iam") { + return ["iam", details.awsRegion ?? "", details.awsReplicationGroupId ?? "", details.username ?? ""].join("~") + } + if (details.authType === "gcp-iam") return "gcp-iam~default" + const username = details.password && details.username ? details.username : "default" + return `password~${username}` +} From 8dd4a62407a16fad9e373e7695c320761d253f03 Mon Sep 17 00:00:00 2001 From: Argus Li Date: Thu, 22 Oct 2026 10:01:33 -0700 Subject: [PATCH 2/9] Key server state by Valkey identity and bind connections to the socket Each Valkey identity (auth type and username) now has its own clients, cluster topology, collectors, collector keys, CPU baselines, config sessions and watchers. Users who connect as different Valkey users share nothing. Each collector gets its own data directory and settings file under DATA_DIR//. Registration finds the collector by its key. Cookie sessions are removed. A WebSocket may act only on connections it opened with credentials that Valkey accepted. A resume without a password asks for the password. When the last socket goes away, the owner's client and unused collectors stop. The preconfigured identity keeps its old lifecycle. Every connect and topology discovery first proves the credentials with a short-lived client. The working clients use static node discovery, which does not reject a wrong password at creation. The integration stack gets a second ACL user, and a new test checks that two users on one node share no client and no collector. Signed-off-by: Argus Li --- apps/server/src/__integration__/README.md | 5 +- .../src/__integration__/harness/fixture.ts | 20 ++ .../identity-isolation.integration.test.ts | 189 +++++++++++++ .../session-authorization.integration.test.ts | 147 ---------- .../src/__tests__/collector-keys.test.ts | 76 +++++- .../command-restrictions-action.test.ts | 5 + apps/server/src/__tests__/connection.test.ts | 122 ++++----- .../__tests__/metrics-orchestrator.test.ts | 6 +- .../src/__tests__/monitorAction.test.ts | 41 ++- .../orchestrator-auth-routes.test.ts | 58 ++++ apps/server/src/__tests__/owner.test.ts | 55 +++- apps/server/src/__tests__/session.test.ts | 122 --------- .../src/__tests__/socket-binding.test.ts | 247 +++++++++++++++++ .../topology-refresh-preconfigured.test.ts | 11 + apps/server/src/__tests__/utils.test.ts | 33 +-- apps/server/src/actions/cluster.ts | 4 +- apps/server/src/actions/config.ts | 15 +- apps/server/src/actions/connection.ts | 103 +++---- apps/server/src/actions/monitorAction.ts | 12 +- apps/server/src/actions/utils.ts | 8 +- apps/server/src/connection.ts | 168 +++++++----- apps/server/src/index.ts | 243 +++++++---------- apps/server/src/metrics-orchestrator.ts | 254 ++++++++++++------ apps/server/src/node-utilization.ts | 14 +- apps/server/src/node-watchers.ts | 6 + apps/server/src/owner.ts | 92 ++++++- apps/server/src/session.ts | 114 -------- apps/server/src/set-dashboard-data.ts | 16 +- apps/server/src/socket-binding.ts | 120 +++++++++ apps/server/src/utils.ts | 11 +- apps/server/src/valkey-client.ts | 15 ++ docker/docker-compose.test.yml | 43 ++- tools/valkey-cluster/scripts/cluster_init.sh | 22 +- 33 files changed, 1503 insertions(+), 894 deletions(-) create mode 100644 apps/server/src/__integration__/identity-isolation.integration.test.ts delete mode 100644 apps/server/src/__integration__/session-authorization.integration.test.ts delete mode 100644 apps/server/src/__tests__/session.test.ts create mode 100644 apps/server/src/__tests__/socket-binding.test.ts delete mode 100644 apps/server/src/session.ts create mode 100644 apps/server/src/socket-binding.ts diff --git a/apps/server/src/__integration__/README.md b/apps/server/src/__integration__/README.md index ed2df2f8..97bb562a 100644 --- a/apps/server/src/__integration__/README.md +++ b/apps/server/src/__integration__/README.md @@ -4,6 +4,8 @@ End-to-end tests that drive the server's WebSocket API against a live Valkey sta ## Quick run +With Podman, use `podman compose` in place of `docker compose`. On Apple silicon, set `DOCKER_PLATFORM=linux/arm64`. + ```bash docker compose -f docker/docker-compose.test.yml up -d --build --wait npm run test:integration @@ -13,7 +15,8 @@ docker compose -f docker/docker-compose.test.yml down -v ## Stack - **Cluster:** `valkey-7001..7006` on ports `7001..7006`. Initialized by the `cluster-init` service and seeded by `populate`. Used by `cluster-topology`, `connection`, `key-browser`, `monitoring`, and `send-command` integration tests via `defaultConnectionDetails()`. -- **Standalone:** `valkey-standalone` on port `6379`. No ACL, no cluster mode. Used by the "two databases" test in `connection.integration.test.ts` via `defaultStandaloneConnectionDetails(db)` to verify that `(host, port, db)` triples produce isolated clients. +- **Standalone:** `valkey-standalone` on port `6379`. No cluster mode. Used by the "two databases" test in `connection.integration.test.ts` via `defaultStandaloneConnectionDetails(db)` to verify that `(host, port, db)` triples produce isolated clients. +- **ACL users:** Every node has the users `appuser`/`admin` (all commands) and `restricted`/`restricted` (all commands except `ACL` and `COMMANDLOG`). The standalone node also keeps the passwordless `default` user. `cluster_init.sh` sets the cluster users on every run, and the standalone node gets them from its `--user` arguments. `identity-isolation.integration.test.ts` uses the two users to check that users who connect as different Valkey users share no client and no metrics collector. ## Note diff --git a/apps/server/src/__integration__/harness/fixture.ts b/apps/server/src/__integration__/harness/fixture.ts index e8373430..0b871d12 100644 --- a/apps/server/src/__integration__/harness/fixture.ts +++ b/apps/server/src/__integration__/harness/fixture.ts @@ -26,3 +26,23 @@ export const defaultStandaloneConnectionDetails = (db: number = 0) => { db, } } + +/** + * A second ACL user with every command except `ACL` and `COMMANDLOG`. Created + * on the cluster nodes by `cluster_init.sh` and on the standalone node by its + * `--user` arguments in `docker/docker-compose.test.yml`. + */ +export const RESTRICTED_USER = { username: "restricted", password: "restricted" } + +export const restrictedClusterConnectionDetails = () => ({ + ...defaultConnectionDetails(), + ...RESTRICTED_USER, +}) + +export const standaloneConnectionDetailsAs = ( + user: { username: string; password: string }, + db: number = 0, +) => ({ + ...defaultStandaloneConnectionDetails(db), + ...user, +}) diff --git a/apps/server/src/__integration__/identity-isolation.integration.test.ts b/apps/server/src/__integration__/identity-isolation.integration.test.ts new file mode 100644 index 00000000..27d107af --- /dev/null +++ b/apps/server/src/__integration__/identity-isolation.integration.test.ts @@ -0,0 +1,189 @@ +import { after, describe, it } from "node:test" +import assert from "node:assert/strict" +import { buildConnectionId, COMMANDLOG_TYPE, MONITOR_ACTION, toNodeId, VALKEY } from "valkey-common" +import { WsClient, type InboundMessage } from "./harness/wsClient" +import { + defaultConnectionDetails, + defaultStandaloneConnectionDetails, + RESTRICTED_USER, + restrictedClusterConnectionDetails, + standaloneConnectionDetailsAs, + WS_URL +} from "./harness/fixture" + +/** + * Per-identity isolation (Phase 1 of discussion #580). + * + * Each WsClient is one browser socket. `admin` connects as `appuser` (all + * commands). `limited` connects as `restricted` (no ACL, no COMMANDLOG). The + * two users must never share a Valkey client or a metrics collector, and a + * socket may act only on connections it opened itself. + * + * A rejected action produces no reply at all: the guard returns without + * sending. So the absence of a reply within a window is the assertion. + */ +describe("integration / identity isolation", async () => { + const admin = await WsClient.connect(WS_URL) + const limited = await WsClient.connect(WS_URL) + const stranger = await WsClient.connect(WS_URL) + + const clusterAdmin = defaultConnectionDetails() + const clusterId0 = buildConnectionId(clusterAdmin.host, clusterAdmin.port, 0) + const standaloneAdmin = standaloneConnectionDetailsAs({ username: "appuser", password: "admin" }) + const standaloneLimited = standaloneConnectionDetailsAs(RESTRICTED_USER) + const standaloneId = buildConnectionId(standaloneAdmin.host, standaloneAdmin.port, 0) + + let clusterId: string | undefined + + const run = async (ws: WsClient, connectionId: string, command: string): Promise => { + ws.send({ type: VALKEY.COMMAND.sendRequested, payload: { connectionId, command } }) + const reply = await Promise.race([ + ws.waitFor(VALKEY.COMMAND.sendFulfilled, 10000), + ws.waitFor(VALKEY.COMMAND.sendFailed, 10000), + ]) + return reply + } + + const silentFor = async (ws: WsClient, types: string[], windowMs = 3000) => { + const results = await Promise.all(types.map((type, i) => ws.collectFor(type, i === 0 ? windowMs : 100))) + return results.reduce((n, r) => n + r.length, 0) + } + + after(async () => { + await admin.close() + await limited.close() + await stranger.close() + }) + + it("connects two Valkey users to the same cluster node and the same standalone node", async () => { + admin.send({ type: VALKEY.CONNECTION.connectPending, payload: { connectionId: clusterId0, connectionDetails: clusterAdmin } }) + const adminCluster = await admin.waitFor(VALKEY.CONNECTION.clusterConnectFulfilled, 30000) + clusterId = adminCluster.payload?.connectionDetails?.clusterId + assert.ok(clusterId) + + limited.send({ + type: VALKEY.CONNECTION.connectPending, + payload: { connectionId: clusterId0, connectionDetails: restrictedClusterConnectionDetails() }, + }) + await limited.waitFor(VALKEY.CONNECTION.clusterConnectFulfilled, 30000) + + admin.send({ type: VALKEY.CONNECTION.connectPending, payload: { connectionId: standaloneId, connectionDetails: standaloneAdmin } }) + await admin.waitFor(VALKEY.CONNECTION.standaloneConnectFulfilled, 30000) + limited.send({ type: VALKEY.CONNECTION.connectPending, payload: { connectionId: standaloneId, connectionDetails: standaloneLimited } }) + await limited.waitFor(VALKEY.CONNECTION.standaloneConnectFulfilled, 30000) + }) + + it("runs each user's commands as that user (the reported ACL LIST bug)", async () => { + for (const connectionId of [clusterId0, standaloneId]) { + const adminWho = await run(admin, connectionId, "ACL WHOAMI") + assert.equal(adminWho.payload, "appuser", `admin on ${connectionId}`) + const limitedWho = await run(limited, connectionId, "ACL WHOAMI") + assert.equal(limitedWho.type, VALKEY.COMMAND.sendFailed, "restricted has no ACL permission") + assert.match(JSON.stringify(limitedWho.payload), /NOPERM|no permissions/i) + assert.doesNotMatch(JSON.stringify(limitedWho.payload), /appuser/) + + const adminList = await run(admin, connectionId, "ACL LIST") + assert.equal(adminList.type, VALKEY.COMMAND.sendFulfilled, `admin ACL LIST on ${connectionId}`) + } + }) + + it("gives each user a metrics collector that runs as that user", async () => { + // Collectors register a moment after connect; poll CLIENT LIST for both users. + let users = new Set() + for (let attempt = 0; attempt < 30 && users.size < 2; attempt++) { + const list = await run(admin, standaloneId, "CLIENT LIST") + // The server parses `key:value` replies into an object; rebuild the lines. + const text = typeof list.payload === "string" + ? list.payload + : Object.entries(list.payload ?? {}).map(([k, v]) => `${k}:${String(v)}`).join("\n") + users = new Set( + text + .split("\n") + .filter((line) => line.includes("valkey_admin_metrics")) + .map((line) => / user=(\S+)/.exec(line)?.[1] ?? ""), + ) + if (users.size < 2) await new Promise((r) => setTimeout(r, 1000)) + } + assert.ok(users.has("appuser"), `appuser collector missing: ${[...users].join(",")}`) + assert.ok(users.has(RESTRICTED_USER.username), `restricted collector missing: ${[...users].join(",")}`) + }) + + it("rejects a wrong password for an existing owner and keeps that owner's socket working", async () => { + stranger.send({ + type: VALKEY.CONNECTION.connectPending, + payload: { connectionId: standaloneId, connectionDetails: { ...standaloneAdmin, password: "wrong" } }, + }) + const rejected = await stranger.waitFor(VALKEY.CONNECTION.connectRejected, 15000) + assert.equal(rejected.payload?.connectionId, standaloneId) + + const stillWorks = await run(admin, standaloneId, "PING") + assert.equal(stillWorks.payload, "PONG") + }) + + it("rejects actions on a connection the socket did not open", async () => { + stranger.send({ type: VALKEY.COMMAND.sendRequested, payload: { connectionId: standaloneId, command: "PING" } }) + assert.equal(await silentFor(stranger, [VALKEY.COMMAND.sendFulfilled, VALKEY.COMMAND.sendFailed]), 0) + }) + + it("rejects a clusterId with no connectionId", async () => { + assert.ok(clusterId) + stranger.send({ + type: VALKEY.COMMANDLOGS.commandLogsRequested, + payload: { clusterId, commandLogType: COMMANDLOG_TYPE.SLOW }, + }) + assert.equal(await silentFor(stranger, [VALKEY.COMMANDLOGS.commandLogsFulfilled, VALKEY.COMMANDLOGS.commandLogsError]), 0) + }) + + it("rejects a foreign clusterId paired with an owned connectionId", async () => { + assert.ok(clusterId) + const standaloneOnly = await WsClient.connect(WS_URL) + try { + const details = defaultStandaloneConnectionDetails(0) + standaloneOnly.send({ type: VALKEY.CONNECTION.connectPending, payload: { connectionId: standaloneId, connectionDetails: details } }) + await standaloneOnly.waitFor(VALKEY.CONNECTION.standaloneConnectFulfilled, 30000) + standaloneOnly.send({ + type: VALKEY.COMMANDLOGS.commandLogsRequested, + payload: { connectionId: standaloneId, clusterId, commandLogType: COMMANDLOG_TYPE.SLOW }, + }) + assert.equal( + await silentFor(standaloneOnly, [VALKEY.COMMANDLOGS.commandLogsFulfilled, VALKEY.COMMANDLOGS.commandLogsError]), + 0, + ) + } finally { + await standaloneOnly.close() + } + }) + + it("accepts a db-less nodeId with its own clusterId (monitor banner path)", async () => { + assert.ok(clusterId) + admin.send({ + type: VALKEY.MONITOR.monitorRequested, + payload: { connectionId: toNodeId(clusterId0), clusterId, monitorAction: MONITOR_ACTION.STATUS }, + }) + const replies = await admin.collectFor(VALKEY.MONITOR.monitorFulfilled, 10000) + assert.ok(replies.length >= 1, `expected at least one node to answer STATUS; got ${replies.length}`) + }) + + it("accepts an empty-string clusterId on a standalone connection", async () => { + admin.send({ + type: VALKEY.COMMANDLOGS.commandLogsRequested, + payload: { connectionId: standaloneId, clusterId: "", commandLogType: COMMANDLOG_TYPE.SLOW }, + }) + const fulfilled = await admin.collectFor(VALKEY.COMMANDLOGS.commandLogsFulfilled, 15000) + const errored = await admin.collectFor(VALKEY.COMMANDLOGS.commandLogsError, 100) + assert.ok(fulfilled.length + errored.length >= 1) + }) + + it("asks for the password when a password connection resumes without one", async () => { + stranger.send({ + type: VALKEY.CONNECTION.connectPending, + payload: { + connectionId: standaloneId, + connectionDetails: { ...standaloneAdmin, password: undefined }, + isResume: true, + }, + }) + const rejected = await stranger.waitFor(VALKEY.CONNECTION.connectRejected, 10000) + assert.equal(rejected.payload?.requiresAuth, true) + }) +}) diff --git a/apps/server/src/__integration__/session-authorization.integration.test.ts b/apps/server/src/__integration__/session-authorization.integration.test.ts deleted file mode 100644 index 141b86a1..00000000 --- a/apps/server/src/__integration__/session-authorization.integration.test.ts +++ /dev/null @@ -1,147 +0,0 @@ -import { after, describe, it } from "node:test" -import assert from "node:assert/strict" -import { buildConnectionId, toNodeId, COMMANDLOG_TYPE, MONITOR_ACTION, VALKEY } from "valkey-common" -import { WsClient } from "./harness/wsClient" -import { - defaultConnectionDetails, - defaultStandaloneConnectionDetails, - WS_URL -} from "./harness/fixture" - -/** - * Session ownership of cluster-scoped actions. - * - * Two WsClients are two sessions: the harness sends no cookie, so each upgrade - * mints a fresh `vk_sid`. `owner` connects to the cluster; `other` connects only - * to the standalone node, so it never earns the cluster. - * - * A rejected action produces no reply at all — the guard returns without - * sending. Absence within a window is therefore the assertion. The allow cases - * below exercise the same code path and reply well inside it. - */ -describe("integration / session authorization (cluster scope)", async () => { - const owner = await WsClient.connect(WS_URL) - const other = await WsClient.connect(WS_URL) - - const clusterDetails = defaultConnectionDetails() - const clusterConnectionId = buildConnectionId(clusterDetails.host, clusterDetails.port, 0) - const standaloneDetails = defaultStandaloneConnectionDetails(0) - const standaloneConnectionId = buildConnectionId(standaloneDetails.host, standaloneDetails.port, 0) - - let clusterId: string | undefined - - after(async () => { - await owner.close() - await other.close() - }) - - it("establishes one cluster session and one unrelated standalone session", async () => { - owner.send({ - type: VALKEY.CONNECTION.connectPending, - payload: { connectionId: clusterConnectionId, connectionDetails: clusterDetails }, - }) - const connected = await owner.waitFor(VALKEY.CONNECTION.clusterConnectFulfilled, 30000) - clusterId = connected.payload?.connectionDetails?.clusterId as string - assert.ok(clusterId, "clusterId must be present after cluster connect") - - other.send({ - type: VALKEY.CONNECTION.connectPending, - payload: { connectionId: standaloneConnectionId, connectionDetails: standaloneDetails }, - }) - const standalone = await other.waitFor(VALKEY.CONNECTION.standaloneConnectFulfilled, 30000) - assert.equal(standalone.payload?.connectionId, standaloneConnectionId) - }) - - it("rejects a clusterId with no connectionId to derive ownership from", async () => { - assert.ok(clusterId, "setup must have run") - other.send({ - type: VALKEY.COMMANDLOGS.commandLogsRequested, - payload: { clusterId, commandLogType: COMMANDLOG_TYPE.SLOW }, - }) - - const fulfilled = await other.collectFor(VALKEY.COMMANDLOGS.commandLogsFulfilled, 3000) - const errored = await other.collectFor(VALKEY.COMMANDLOGS.commandLogsError, 100) - assert.equal( - fulfilled.length + errored.length, - 0, - "a clusterId-only payload must be rejected before the handler runs", - ) - }) - - it("rejects a foreign clusterId paired with an owned connectionId", async () => { - assert.ok(clusterId, "setup must have run") - other.send({ - type: VALKEY.COMMANDLOGS.commandLogsRequested, - payload: { - connectionId: standaloneConnectionId, // owned by this session - clusterId, // but belongs to the other session's cluster - commandLogType: COMMANDLOG_TYPE.SLOW, - }, - }) - - const fulfilled = await other.collectFor(VALKEY.COMMANDLOGS.commandLogsFulfilled, 3000) - const errored = await other.collectFor(VALKEY.COMMANDLOGS.commandLogsError, 100) - assert.equal( - fulfilled.length + errored.length, - 0, - "an owned connectionId must not authorize a cluster it does not belong to", - ) - }) - - it("allows a db-less nodeId paired with its own clusterId (monitor banner path)", async () => { - assert.ok(clusterId, "setup must have run") - // The monitor banner names the node, not the connection: it sends the - // db-less nodeId in the `connectionId` field. - owner.send({ - type: VALKEY.MONITOR.monitorRequested, - payload: { - connectionId: toNodeId(clusterConnectionId), - clusterId, - monitorAction: MONITOR_ACTION.STATUS, - }, - }) - - const replies = await owner.collectFor(VALKEY.MONITOR.monitorFulfilled, 10000) - assert.ok(replies.length >= 1, `expected at least one node to answer STATUS; got ${replies.length}`) - }) - - it("allows an empty-string clusterId on a standalone connection", async () => { - // The metrics-retry epic sends `clusterId: details?.clusterId ?? ""`, so a - // standalone connection puts "" on the wire. The guard must treat that as - // absent — tightening it to `!== undefined` would reject every standalone - // metrics refresh. - // - // Probed with commandLogs rather than monitor on purpose: commandLogs reads - // `clusterNodesRegistry[clusterId]` and gets `undefined` for "", so it falls - // through to the standalone path and always answers. `monitorRequested` - // branches on `typeof clusterId === "string"`, which is true for "", so it - // takes the cluster path, resolves zero nodes and replies nothing — which - // would be indistinguishable from the guard rejecting the action. - other.send({ - type: VALKEY.COMMANDLOGS.commandLogsRequested, - payload: { - connectionId: standaloneConnectionId, - clusterId: "", - commandLogType: COMMANDLOG_TYPE.SLOW, - }, - }) - - const fulfilled = await other.collectFor(VALKEY.COMMANDLOGS.commandLogsFulfilled, 10000) - const errored = await other.collectFor(VALKEY.COMMANDLOGS.commandLogsError, 100) - assert.ok( - fulfilled.length + errored.length >= 1, - "an empty-string clusterId must not be treated as a cluster claim", - ) - }) - - it("keeps the socket usable after a rejection", async () => { - // The guard returns; it must not close the connection. - other.send({ - type: VALKEY.MONITOR.monitorRequested, - payload: { connectionId: standaloneConnectionId, monitorAction: MONITOR_ACTION.STATUS }, - }) - const fulfilled = await other.collectFor(VALKEY.MONITOR.monitorFulfilled, 10000) - const errored = await other.collectFor(VALKEY.MONITOR.monitorError, 100) - assert.ok(fulfilled.length + errored.length >= 1, "session must still work after rejected actions") - }) -}) diff --git a/apps/server/src/__tests__/collector-keys.test.ts b/apps/server/src/__tests__/collector-keys.test.ts index ff78402f..1797d095 100644 --- a/apps/server/src/__tests__/collector-keys.test.ts +++ b/apps/server/src/__tests__/collector-keys.test.ts @@ -1,6 +1,10 @@ import { describe, it, beforeEach, afterEach, mock } from "node:test" import assert from "node:assert" import { ORCHESTRATOR_AUTH_KEY_ENV } from "valkey-common" +import fs from "node:fs" +import os from "node:os" +import path from "node:path" +import { createOwnerState, encodeOwnerDir, getOwnerState } from "../owner" import { metricsServerMap, resolveCollectorKey, @@ -133,10 +137,80 @@ describe("collector key material", () => { await startMetricsServer(NODE, "node-a") await startMetricsServer({ ...NODE, port: "6380" }, "node-b") - await stopAllMetricsServers(metricsServerMap) + stopAllMetricsServers() assert.strictEqual(resolveCollectorKey("node-a"), undefined) assert.strictEqual(resolveCollectorKey("node-b"), undefined) assert.strictEqual(__test__.collectorKeys.size, 0) }) }) + +describe("per-owner collector files", () => { + let spawnedEnvs: SpawnOptions["env"][] + let tmp: string + const saved = { DATA_DIR: process.env.DATA_DIR, CONFIG_PATH: process.env.CONFIG_PATH } + + beforeEach(() => { + spawnedEnvs = [] + mock.method(__test__, "spawnProcess", (_c: string, _a: string[], options: SpawnOptions) => { + spawnedEnvs.push(options.env) + return fakeChild(4242) + }) + tmp = fs.mkdtempSync(path.join(os.tmpdir(), "va-owner-")) + fs.writeFileSync(path.join(tmp, "base.yml"), "base: true\n") + process.env.DATA_DIR = path.join(tmp, "data") + process.env.CONFIG_PATH = path.join(tmp, "base.yml") + }) + + afterEach(() => { + mock.restoreAll() + for (const [k, v] of Object.entries(saved)) { + if (v === undefined) delete process.env[k] + else process.env[k] = v + } + fs.rmSync(tmp, { recursive: true, force: true }) + }) + + it("gives each owner its own data directory and settings file", async () => { + const alice = createOwnerState("password~alice") + const bob = createOwnerState("password~Bob") + await startMetricsServer(NODE, "node-a", alice) + await startMetricsServer(NODE, "node-a", bob) + + const [a, b] = spawnedEnvs + assert.strictEqual(a?.DATA_DIR, path.join(tmp, "data", "password~alice", "node-a")) + assert.strictEqual(a?.CONFIG_PATH, path.join(tmp, "data", "password~alice", "config.yml")) + assert.strictEqual(b?.DATA_DIR, path.join(tmp, "data", encodeOwnerDir("password~Bob"), "node-a")) + assert.notStrictEqual(a?.CONFIG_PATH, b?.CONFIG_PATH) + assert.strictEqual(fs.readFileSync(String(a?.CONFIG_PATH), "utf8"), "base: true\n") + assert.strictEqual(fs.readFileSync(String(b?.CONFIG_PATH), "utf8"), "base: true\n") + + assert.ok(alice.metricsServerMap.has("node-a") && bob.metricsServerMap.has("node-a")) + assert.notStrictEqual(resolveCollectorKey("node-a", alice), resolveCollectorKey("node-a", bob)) + }) + + it("seeds the settings file once and never overwrites it", async () => { + const alice = createOwnerState("password~alice") + await startMetricsServer(NODE, "node-a", alice) + const cfg = String(spawnedEnvs[0]?.CONFIG_PATH) + fs.writeFileSync(cfg, "changed: true\n") + + await startMetricsServer({ ...NODE, port: "6380" }, "node-b", alice) + assert.strictEqual(spawnedEnvs[1]?.CONFIG_PATH, cfg) + assert.strictEqual(fs.readFileSync(cfg, "utf8"), "changed: true\n") + }) + + it("stops every owner's collectors on shutdown", async () => { + const killed: number[] = [] + mock.method(process, "kill", (pid: number) => { killed.push(pid); return true }) + const alice = getOwnerState("password~shutdown-a") + const bob = getOwnerState("password~shutdown-b") + alice.metricsServerMap.set("n1", { metricsURI: "", pid: 11, lastSeen: 0 }) + bob.metricsServerMap.set("n1", { metricsURI: "", pid: 12, lastSeen: 0 }) + + stopAllMetricsServers() + + assert.ok(killed.includes(11) && killed.includes(12)) + assert.strictEqual(alice.metricsServerMap.size + bob.metricsServerMap.size, 0) + }) +}) diff --git a/apps/server/src/__tests__/command-restrictions-action.test.ts b/apps/server/src/__tests__/command-restrictions-action.test.ts index 1796a757..6914ab9c 100644 --- a/apps/server/src/__tests__/command-restrictions-action.test.ts +++ b/apps/server/src/__tests__/command-restrictions-action.test.ts @@ -3,6 +3,8 @@ import assert from "node:assert/strict" import { GlideClient, GlideClusterClient } from "@valkey/valkey-glide" import { VALKEY } from "valkey-common" import { sendRequested } from "../actions/command" +import { createOwnerState } from "../owner" +import { createSocketBinding } from "../socket-binding" import type { Deps } from "../actions/utils" import type WebSocket from "ws" @@ -22,6 +24,9 @@ describe("Send Command connection-state restrictions", () => { metricsServerMap: new Map(), connectedNodesByCluster: new Map(), clusterNodesRegistry: new Map(), + ownerKey: "password~default", + ownerState: createOwnerState("password~default"), + binding: createSocketBinding(), } await sendRequested(deps)({ type: VALKEY.COMMAND.sendRequested, diff --git a/apps/server/src/__tests__/connection.test.ts b/apps/server/src/__tests__/connection.test.ts index 9d4c9b27..434cd327 100644 --- a/apps/server/src/__tests__/connection.test.ts +++ b/apps/server/src/__tests__/connection.test.ts @@ -19,8 +19,6 @@ import { checkJsonModuleAvailability } from "../check-json-module" import { ConnectionDetails } from "../actions/connection" import { type MetricsServerMap } from "../metrics-orchestrator" import { _reset as resetNodeWatchers } from "../node-watchers" -import { ensureSession, authorizeConnection, _resetSessions } from "../session" -import type { IncomingMessage } from "http" const DEFAULT_PAYLOAD = { connectionDetails: { @@ -34,7 +32,6 @@ const DEFAULT_PAYLOAD = { db: 0, } as ConnectionDetails, connectionId: "", - sessionId: "test-session", } const SLOTS_RESPONSE = [ @@ -114,6 +111,9 @@ const buildClusterMock = (overrides: { delayMs?: number, clusterDatabases?: stri } } +// Configs of AUTH probe clients opened inside `withMockedClients`. +const authProbeCalls: any[] = [] + async function withMockedClients( standalone: unknown | (() => unknown), cluster: unknown | null, @@ -124,19 +124,32 @@ async function withMockedClients( const originalCreateClusterClient = GlideClusterClient.createClient const standaloneFactory = typeof standalone === "function" - ? standalone as () => unknown + ? standalone as (config?: unknown) => unknown : () => standalone - GlideClient.createClient = mock.fn(async () => { + // Every connect first opens a short-lived AUTH probe client + // (`valkey_admin_auth_probe`). Probe calls are answered by a separate mock and + // recorded in `authProbeCalls`, so `GlideClient.createClient` counts only the + // working clients, as before the probe existed. + const realCreate = mock.fn(async (config?: unknown) => { if (opts.delayMs) await new Promise((r) => setTimeout(r, opts.delayMs)) - return standaloneFactory() as any + return standaloneFactory(config) as any }) + const dispatch = async (config: any) => { + if (config?.clientName === "valkey_admin_auth_probe") { + authProbeCalls.push(config) + return { close: mock.fn() } as any + } + return realCreate(config) + } + GlideClient.createClient = Object.assign(dispatch, { mock: realCreate.mock }) as any if (cluster !== null) { GlideClusterClient.createClient = mock.fn(async () => { if (opts.delayMs) await new Promise((r) => setTimeout(r, opts.delayMs)) return cluster as any }) } + authProbeCalls.length = 0 try { return await fn() } finally { @@ -151,7 +164,6 @@ describe("connectToValkey", () => { let clients: Map let connectedNodesByCluster: Map let metricsServerMap: MetricsServerMap - let testSessionId: string beforeEach(() => { messages = [] mockWs = { @@ -160,11 +172,6 @@ describe("connectToValkey", () => { clients = new Map() connectedNodesByCluster = new Map() metricsServerMap = new Map() - // Seed a real session and point the default payload at it so that reuse tests, - // which now require the session to own the reused connection, can authorize it. - _resetSessions() - testSessionId = ensureSession({ headers: {}, socket: {} } as unknown as IncomingMessage).sessionId - DEFAULT_PAYLOAD.sessionId = testSessionId // metricsServerMap is keyed by metrics-node-id; the empty Connection_Identifier // strips to the empty string, which matches the empty connectionId used by // the default payload in these tests. @@ -267,22 +274,18 @@ describe("connectToValkey", () => { await withMockedClients(standalone, null, async () => { const payload = { ...DEFAULT_PAYLOAD, connectionId: "conn-shared" } - // The session owns the connection it is (re)connecting to; the handler authorizes - // it on connect. Simulate that here so the concurrent duplicate reuses rather than - // creating a second client. - authorizeConnection(testSessionId, "conn-shared") const [a, b] = await Promise.all([ connectToValkey(ctx(), mockWs, payload), connectToValkey(ctx(), mockWs, payload), ]) - // The second connector reuses the first one's client, but only after a - // short-lived client proved its credentials (AUTH-gated reuse): 1 client + 1 probe. assert.strictEqual( (GlideClient.createClient as any).mock.calls.length, - 2, - "expected one standalone client and one AUTH probe", + 1, + "expected exactly one standalone client creation", ) + // Both connectors proved their credentials (AUTH-gated reuse). + assert.strictEqual(authProbeCalls.length, 2) assert.strictEqual(a, standalone) assert.strictEqual(b, standalone) assert.strictEqual(clients.get("conn-shared")?.client, standalone) @@ -299,10 +302,6 @@ describe("connectToValkey", () => { assert.strictEqual(first, cluster) assert.strictEqual((GlideClusterClient.createClient as any).mock.calls.length, 1) - // The handler authorizes the connection after a successful connect; simulate that - // here (this test bypasses the handler) so the session owns the connection it reuses. - authorizeConnection(testSessionId, "conn-Z") - messages.length = 0 // Same connectionId again: must hit the early-return cluster reuse path. @@ -517,7 +516,6 @@ describe("connectToValkey", () => { it("should use correct client configuration", async () => { const standalone = buildStandaloneMock() const altPayload = { - sessionId: "test-session", connectionDetails: { host: "192.168.1.1", port: "7000", @@ -636,7 +634,6 @@ describe("connectToValkey", () => { await withMockedClients(newStandalone, newCluster, async () => { await connectToValkey(ctx(), mockWs, { - sessionId: "test-session", connectionId, connectionDetails: { ...DEFAULT_PAYLOAD.connectionDetails, ...tc.overrides }, isRetry: true, @@ -659,7 +656,6 @@ describe("connectToValkey", () => { await withMockedClients(buildStandaloneMock({ databases: undefined }), null, async () => { const connectionId = buildConnectionId("127.0.0.1", "6379", 15) await connectToValkey(ctx(), mockWs, { - sessionId: "test-session", connectionId, connectionDetails: { ...DEFAULT_PAYLOAD.connectionDetails, db: 15 }, }) @@ -687,7 +683,6 @@ describe("connectToValkey", () => { await withMockedClients(buildStandaloneMock({ databases: undefined }), null, async () => { const connectionId = buildConnectionId("127.0.0.1", "6379", 16) await connectToValkey(ctx(), mockWs, { - sessionId: "test-session", connectionId, connectionDetails: { ...DEFAULT_PAYLOAD.connectionDetails, db: 16 }, }) @@ -715,7 +710,6 @@ describe("connectToValkey", () => { await withMockedClients(buildStandaloneMock({ databases: "32" }), null, async () => { const connectionId = buildConnectionId("127.0.0.1", "6379", 31) await connectToValkey(ctx(), mockWs, { - sessionId: "test-session", connectionId, connectionDetails: { ...DEFAULT_PAYLOAD.connectionDetails, db: 31 }, }) @@ -751,24 +745,41 @@ describe("connectToValkey", () => { }) describe("AUTH-gated reuse", () => { + it("gives two owners on one connectionId two clients and keeps both", async () => { + const aliceClient = buildStandaloneMock() + const bobClient = buildStandaloneMock() + const clientsFor = [aliceClient, bobClient] + const alice = { ...ctx(), clients: new Map() } + const bob = { ...ctx(), clients: new Map() } + await withMockedClients(() => clientsFor.shift(), null, async () => { + await connectToValkey(alice, mockWs, { ...DEFAULT_PAYLOAD, connectionId: "conn-O" }) + await connectToValkey(bob, mockWs, { + ...DEFAULT_PAYLOAD, + connectionId: "conn-O", + connectionDetails: { ...DEFAULT_PAYLOAD.connectionDetails, username: "bob", password: "bobpw" }, + }) + }) + assert.strictEqual(alice.clients.get("conn-O")?.client, aliceClient) + assert.strictEqual(bob.clients.get("conn-O")?.client, bobClient) + assert.strictEqual(aliceClient.close.mock.calls.length, 0) + assert.strictEqual(bobClient.close.mock.calls.length, 0) + }) const wrongPass = () => new Error("WRONGPASS: invalid username-password pair or user is disabled.") it("authenticates the presented credentials before it reuses an existing client", async () => { const existing = buildStandaloneMock() const probe = buildStandaloneMock() clients.set("conn-R", { client: existing }) - authorizeConnection(testSessionId, "conn-R") await withMockedClients(probe, null, async () => { const result = await connectToValkey(ctx(), mockWs, { ...DEFAULT_PAYLOAD, connectionId: "conn-R" }) - const calls = (GlideClient.createClient as any).mock.calls - assert.strictEqual(calls.length, 1, "reuse must open exactly one probe client") - assert.deepStrictEqual(calls[0].arguments[0].credentials, { + assert.strictEqual((GlideClient.createClient as any).mock.calls.length, 0, "reuse opens no working client") + assert.strictEqual(authProbeCalls.length, 1, "reuse must open exactly one probe client") + assert.deepStrictEqual(authProbeCalls[0].credentials, { username: DEFAULT_PAYLOAD.connectionDetails.username, password: DEFAULT_PAYLOAD.connectionDetails.password, }) - assert.strictEqual(probe.close.mock.calls.length, 1, "the probe client must be closed") assert.strictEqual(result, existing) assert.strictEqual(clients.get("conn-R")?.client, existing) }) @@ -777,7 +788,6 @@ describe("connectToValkey", () => { it("rejects a wrong password and leaves the existing client open", async () => { const existing = buildStandaloneMock() clients.set("conn-R", { client: existing }) - authorizeConnection(testSessionId, "conn-R") const original = GlideClient.createClient GlideClient.createClient = mock.fn(async () => { throw wrongPass() }) as any try { @@ -799,7 +809,6 @@ describe("connectToValkey", () => { it("does not close the existing client when a retry fails to authenticate", async () => { const existing = buildStandaloneMock() clients.set("conn-R", { client: existing }) - authorizeConnection(testSessionId, "conn-R") const original = GlideClient.createClient GlideClient.createClient = mock.fn(async () => { throw wrongPass() }) as any try { @@ -821,7 +830,6 @@ describe("connectToValkey", () => { await withMockedClients(buildStandaloneMock({ configGetError: true }), null, async () => { const connectionId = buildConnectionId("127.0.0.1", "6379", 20) await connectToValkey(ctx(), mockWs, { - sessionId: "test-session", connectionId, connectionDetails: { ...DEFAULT_PAYLOAD.connectionDetails, db: 20 }, }) @@ -859,7 +867,6 @@ describe("connectToValkey", () => { await withMockedClients(standaloneFactory, null, async () => { const connectionId = buildConnectionId("127.0.0.1", "6379", 5) await connectToValkey(ctx(), mockWs, { - sessionId: "test-session", connectionId, connectionDetails: { ...DEFAULT_PAYLOAD.connectionDetails, db: 5 }, }) @@ -888,7 +895,6 @@ describe("connectToValkey", () => { await withMockedClients(standalone, cluster, async () => { const connectionId = buildConnectionId("127.0.0.1", "6379", 8) await connectToValkey(ctx(), mockWs, { - sessionId: "test-session", connectionId, connectionDetails: { ...DEFAULT_PAYLOAD.connectionDetails, db: 8 }, }) @@ -921,7 +927,6 @@ describe("connectToValkey", () => { await withMockedClients(standalone, cluster, async () => { const connectionId = buildConnectionId("127.0.0.1", "6379", 2) await connectToValkey(ctx(), mockWs, { - sessionId: "test-session", connectionId, connectionDetails: { ...DEFAULT_PAYLOAD.connectionDetails, db: 2 }, }) @@ -981,7 +986,6 @@ describe("connectToValkey", () => { await withMockedClients(standalone, cluster, async () => { const connectionId = buildConnectionId("127.0.0.1", "6379", 1) await connectToValkey(ctx(), mockWs, { - sessionId: "test-session", connectionId, connectionDetails: { ...DEFAULT_PAYLOAD.connectionDetails, db: 1 }, }) @@ -1058,11 +1062,8 @@ describe("resolveHostnameOrIpAddress", () => { }) describe("getExistingConnection", () => { - let unitSessionId: string beforeEach(() => { mock.restoreAll() - _resetSessions() - unitSessionId = ensureSession({ headers: {}, socket: {} } as unknown as IncomingMessage).sessionId }) it("returns the existing connection when host:port resolves to one already in clients", async () => { @@ -1073,12 +1074,9 @@ describe("getExistingConnection", () => { const existingClient = {} as any const clients = new Map() clients.set(buildConnectionId("10.0.0.1", "6379", 0), { client: existingClient }) - // Session must own the matched id for reuse to be returned. - authorizeConnection(unitSessionId, buildConnectionId("10.0.0.1", "6379", 0)) const result = await getExistingConnection( { - sessionId: unitSessionId, connectionId: "abc123", connectionDetails: { host: "my-host", port: "6379", tls: false, verifyTlsCertificate: false, endpointType: "node", @@ -1100,7 +1098,6 @@ describe("getExistingConnection", () => { const result = await getExistingConnection( { - sessionId: unitSessionId, connectionId: "abc123", connectionDetails: { host: "my-host", port: "6379", tls: false, verifyTlsCertificate: false, endpointType: "node", @@ -1122,7 +1119,6 @@ describe("getExistingConnection", () => { const result = await getExistingConnection( { - sessionId: unitSessionId, connectionId: "abc123", isRetry: true, connectionDetails: { @@ -1143,12 +1139,9 @@ describe("getExistingConnection", () => { const directClient = {} as any const clients = new Map() clients.set("abc123", { client: directClient }) - // Session must own the matched id for reuse to be returned. - authorizeConnection(unitSessionId, "abc123") const result = await getExistingConnection( { - sessionId: unitSessionId, connectionId: "abc123", connectionDetails: { host: "my-host", port: "6379", tls: false, verifyTlsCertificate: false, endpointType: "node", @@ -1161,33 +1154,28 @@ describe("getExistingConnection", () => { assert.strictEqual(result.client, directClient) }) - it("does NOT return another session's client when this session is not authorized", async () => { - // Credential-blind reuse regression: a different session must not inherit a client - // it never authenticated. The match exists but this session does not own it. + it("never sees another owner's client, because each owner has its own map", async () => { + // Owner scoping replaces the old session check: a connect looks up only the + // clients of the identity it authenticates as. mock.method(dns, "lookup", async () => [ { address: "10.0.0.1", family: 4 }, ]) - const victimClient = {} as any - const clients = new Map() - const matchedId = buildConnectionId("10.0.0.1", "6379", 0) - clients.set(matchedId, { client: victimClient }) - // Authorize the id for a DIFFERENT session (the victim), not ours. - const victimSession = ensureSession({ headers: {}, socket: {} } as unknown as IncomingMessage).sessionId - authorizeConnection(victimSession, matchedId) + const otherOwnerClients = new Map() + otherOwnerClients.set(buildConnectionId("10.0.0.1", "6379", 0), { client: {} as any }) + const ownClients = new Map() const result = await getExistingConnection( { - sessionId: unitSessionId, // our session — never authorized for matchedId connectionId: "abc123", connectionDetails: { host: "my-host", port: "6379", tls: false, verifyTlsCertificate: false, endpointType: "node", } as any, }, - clients, + ownClients, ) - assert.strictEqual(result, undefined, "must not hand back a client this session does not own") + assert.strictEqual(result, undefined) }) }) @@ -1278,7 +1266,7 @@ describe("closeMetricsServer", () => { metricsServerMap.set(expectedNodeId, { metricsURI: "http://localhost:9999", pid: 123, lastSeen: Date.now() }) const clients: Map = new Map() - await closeMetricsServer(connectionId, metricsServerMap, clients) + await closeMetricsServer(connectionId, { metricsServerMap, clients, collectorKeys: new Map() }) const parsed = JSON.parse(capturedBody!) assert.strictEqual(parsed.connectionId, expectedNodeId, @@ -1300,7 +1288,7 @@ describe("closeMetricsServer", () => { const clients: Map = new Map() clients.set("127-0-0-1-7000-db1", { client: {} }) - await closeMetricsServer(connectionId, metricsServerMap, clients) + await closeMetricsServer(connectionId, { metricsServerMap, clients, collectorKeys: new Map() }) assert.strictEqual((globalThis.fetch as any).mock.calls.length, 0, "should not call close when other connections still reference the node") diff --git a/apps/server/src/__tests__/metrics-orchestrator.test.ts b/apps/server/src/__tests__/metrics-orchestrator.test.ts index 3cc6e4a0..e231db36 100644 --- a/apps/server/src/__tests__/metrics-orchestrator.test.ts +++ b/apps/server/src/__tests__/metrics-orchestrator.test.ts @@ -217,7 +217,7 @@ describe("metrics-orchestrator", () => { metricsServerMap.set("node2", { metricsURI: "uri", pid: 2, lastSeen: 2 }) mock.method(process, "kill", (pid: number) => killed.push(pid)) - await stopAllMetricsServers(metricsServerMap) + stopAllMetricsServers() assert.strictEqual(metricsServerMap.size, 0) assert.strictEqual(killed.includes(1), true) assert.strictEqual(killed.includes(2), true) @@ -246,7 +246,7 @@ describe("metrics-orchestrator", () => { const findDiff = mock.method(__test__, "findDiff", async () => ({ nodesToAdd: {}, nodesToRemove: [] })) clusterNodesRegistry.clear() - await reconcileClusterMetricsServers(metricsServerMap) + await reconcileClusterMetricsServers() // No clusters to reconcile, so the diff is never computed. assert.strictEqual(findDiff.mock.callCount(), 0) @@ -259,7 +259,7 @@ describe("metrics-orchestrator", () => { node1: { host: "127.0.0.1", port: 6379, tls: false, verifyTlsCertificate: false }, }) - await reconcileClusterMetricsServers(metricsServerMap) + await reconcileClusterMetricsServers() // The cluster is inspected, but findDiff reports no changes, so no // metrics servers are started or stopped. diff --git a/apps/server/src/__tests__/monitorAction.test.ts b/apps/server/src/__tests__/monitorAction.test.ts index c2a0aa05..81a94537 100644 --- a/apps/server/src/__tests__/monitorAction.test.ts +++ b/apps/server/src/__tests__/monitorAction.test.ts @@ -4,7 +4,7 @@ import assert from "node:assert" import { VALKEY } from "valkey-common" import { monitorRequested } from "../actions/monitorAction" import { updateConfig } from "../actions/config" -import { subscribe, _reset as resetNodeWatchers } from "../node-watchers" +import { scopedId, subscribe, _reset as resetNodeWatchers } from "../node-watchers" import { ClusterNodeMap } from "../metrics-orchestrator" function createMockResponse(body: any, ok = true, status = 200) { @@ -15,6 +15,8 @@ function createMockResponse(body: any, ok = true, status = 200) { } } +const OWNER = "password~default" + describe("monitorAction", () => { let mockWs: any let messages: string[] @@ -59,7 +61,7 @@ describe("monitorAction", () => { } const deps = () => ({ - ws: mockWs, metricsServerMap, connectedNodesByCluster, clients: new Map(), connectionId: "", clusterNodesRegistry, + ws: mockWs, metricsServerMap, connectedNodesByCluster, clients: new Map(), connectionId: "", clusterNodesRegistry, ownerKey: OWNER, } as any) describe("status request", () => { @@ -301,8 +303,8 @@ describe("monitorAction", () => { const otherWs: any = { send: mock.fn((msg: string) => watcherMessages.push(msg)) } // Both ws clients watch the same node - subscribe("conn-1", mockWs) - subscribe("conn-1", otherWs) + subscribe(scopedId(OWNER, "conn-1"), mockWs) + subscribe(scopedId(OWNER, "conn-1"), otherWs) const action = { type: VALKEY.MONITOR.monitorRequested, @@ -333,8 +335,8 @@ describe("monitorAction", () => { const watcherMessages: string[] = [] const otherWs: any = { send: mock.fn((msg: string) => watcherMessages.push(msg)) } - subscribe("conn-1", mockWs) - subscribe("conn-1", otherWs) + subscribe(scopedId(OWNER, "conn-1"), mockWs) + subscribe(scopedId(OWNER, "conn-1"), otherWs) const action = { type: VALKEY.MONITOR.monitorRequested, @@ -356,8 +358,8 @@ describe("monitorAction", () => { const watcherMessages: string[] = [] const otherWs: any = { send: mock.fn((msg: string) => watcherMessages.push(msg)) } - subscribe("conn-1", mockWs) - subscribe("conn-1", otherWs) + subscribe(scopedId(OWNER, "conn-1"), mockWs) + subscribe(scopedId(OWNER, "conn-1"), otherWs) const action = { type: VALKEY.MONITOR.monitorRequested, @@ -377,7 +379,7 @@ describe("monitorAction", () => { mockFetch({ monitorRunning: true, checkAt: 12345 }) // Only the originator is watching - subscribe("conn-1", mockWs) + subscribe(scopedId(OWNER, "conn-1"), mockWs) const action = { type: VALKEY.MONITOR.monitorRequested, @@ -390,6 +392,23 @@ describe("monitorAction", () => { assert.strictEqual(messages.length, 1) }) + it("does not broadcast to a watcher of another owner on the same connection", async () => { + metricsServerMap.set("conn-1", { metricsURI: "http://localhost:9999" }) + mockFetch({ monitorRunning: true, checkAt: 12345 }) + + const otherOwnerMessages: string[] = [] + const otherOwnerWs: any = { send: mock.fn((msg: string) => otherOwnerMessages.push(msg)) } + subscribe(scopedId(OWNER, "conn-1"), mockWs) + subscribe(scopedId("password~someone-else", "conn-1"), otherOwnerWs) + + await monitorRequested(deps())({ + type: VALKEY.MONITOR.monitorRequested, + payload: { connectionId: "conn-1", monitorAction: "start" }, + } as any) + + assert.strictEqual(otherOwnerMessages.length, 0) + }) + it("looks up other watchers by the db-suffixed connectionId while replying with a db-less nodeId", async () => { // Metrics is keyed db-less; watcher subscriptions stay keyed by the // db-suffixed connectionId (Req 6.5). The reply payload is node-level. @@ -400,8 +419,8 @@ describe("monitorAction", () => { const otherWs: any = { send: mock.fn((msg: string) => watcherMessages.push(msg)) } // Both clients subscribe under the db-suffixed connectionId. - subscribe("host-6379-db2", mockWs) - subscribe("host-6379-db2", otherWs) + subscribe(scopedId(OWNER, "host-6379-db2"), mockWs) + subscribe(scopedId(OWNER, "host-6379-db2"), otherWs) const action = { type: VALKEY.MONITOR.monitorRequested, diff --git a/apps/server/src/__tests__/orchestrator-auth-routes.test.ts b/apps/server/src/__tests__/orchestrator-auth-routes.test.ts index f901c430..3b0f3c6c 100644 --- a/apps/server/src/__tests__/orchestrator-auth-routes.test.ts +++ b/apps/server/src/__tests__/orchestrator-auth-routes.test.ts @@ -7,6 +7,7 @@ import { createOrchestratorAuthCredential } from "valkey-common" import { metricsServerMap, __test__ } from "../metrics-orchestrator" +import { getOwnerState } from "../owner" import type { Request, Response } from "express" const NODE_ID = "127-0-0-1-6379" @@ -575,3 +576,60 @@ describe("POST /orchestrator/ping", () => { assert.strictEqual(captured.body, "Unauthorized") }) }) + +describe("registration with several owners on one node", () => { + const ALICE_KEY = "b".repeat(64) + const BOB_KEY = "c".repeat(64) + let alice: ReturnType + let bob: ReturnType + + beforeEach(() => { + alice = getOwnerState("password~reg-alice") + bob = getOwnerState("password~reg-bob") + for (const [state, key] of [[alice, ALICE_KEY], [bob, BOB_KEY]] as const) { + state.collectorKeys.set(NODE_ID, key) + state.metricsServerMap.set(NODE_ID, { metricsURI: SEEDED_URI, pid: 1, lastSeen: SEEDED_LAST_SEEN }) + } + }) + + afterEach(() => { + for (const state of [alice, bob]) { + state.collectorKeys.clear() + state.metricsServerMap.clear() + } + mock.restoreAll() + }) + + it("updates only the entry of the collector whose key signed the request", () => { + const timestamp = Date.now() + const { res, captured } = makeRes() + __test__.handleRegister( + makeReq({ nodeId: NODE_ID, metricsServerUri: URI, timestamp }, signRegister({ timestamp }, BOB_KEY)), + res, + ) + assert.strictEqual(captured.statusCode, 200) + assert.strictEqual(bob.metricsServerMap.get(NODE_ID)?.metricsURI, URI) + assert.strictEqual(alice.metricsServerMap.get(NODE_ID)?.metricsURI, SEEDED_URI) + }) + + it("refreshes only the signing collector on ping", () => { + const { res, captured } = makeRes() + __test__.handlePing(makeReq({ nodeId: NODE_ID, timestamp: Date.now() }, signPing({}, ALICE_KEY)), res) + assert.strictEqual(captured.statusCode, 200) + assert.ok((alice.metricsServerMap.get(NODE_ID)?.lastSeen ?? 0) > SEEDED_LAST_SEEN) + assert.strictEqual(bob.metricsServerMap.get(NODE_ID)?.lastSeen, SEEDED_LAST_SEEN) + }) + + it("rejects a key that no owner issued", () => { + mock.method(console, "warn", () => {}) + const timestamp = Date.now() + const { res, captured } = makeRes() + __test__.handleRegister( + makeReq({ nodeId: NODE_ID, metricsServerUri: URI, timestamp }, signRegister({ timestamp }, "d".repeat(64))), + res, + ) + assert.strictEqual(captured.statusCode, 401) + assert.strictEqual(alice.metricsServerMap.get(NODE_ID)?.metricsURI, SEEDED_URI) + assert.strictEqual(bob.metricsServerMap.get(NODE_ID)?.metricsURI, SEEDED_URI) + }) +}) diff --git a/apps/server/src/__tests__/owner.test.ts b/apps/server/src/__tests__/owner.test.ts index e1c6ea11..7758a784 100644 --- a/apps/server/src/__tests__/owner.test.ts +++ b/apps/server/src/__tests__/owner.test.ts @@ -1,6 +1,6 @@ import { describe, it } from "node:test" import assert from "node:assert" -import { ownerOf } from "../owner" +import { allOwnerStates, decodeOwnerDir, encodeOwnerDir, getOwnerState, ownerOf } from "../owner" import type { ConnectionDetails } from "../actions/connection" const base: ConnectionDetails = { @@ -54,3 +54,56 @@ describe("ownerOf", () => { ) }) }) + +describe("encodeOwnerDir / decodeOwnerDir", () => { + const keys = [ + "password~alice", + "password~Alice", + "password~default", + "password~a.b/c\\d..e", + "password~ünïcødé", + "iam~us-east-1~my-rg~iam-user", + "gcp-iam~default", + ] + + it("round-trips", () => { + for (const key of keys) assert.strictEqual(decodeOwnerDir(encodeOwnerDir(key)), key) + }) + + it("keeps lowercase letters, digits, - _ ~ as they are", () => { + assert.strictEqual(encodeOwnerDir("password~alice_01-x"), "password~alice_01-x") + }) + + it("keeps names distinct on case-insensitive filesystems", () => { + const a = encodeOwnerDir("password~Alice").toLowerCase() + const b = encodeOwnerDir("password~alice").toLowerCase() + assert.notStrictEqual(a, b) + }) + + it("produces a single safe path segment", () => { + for (const key of keys) { + const dir = encodeOwnerDir(key) + assert.match(dir, /^[a-z0-9_~%A-F-]+$/) + assert.ok(!dir.includes("/") && !dir.includes("\\") && !dir.includes("."), dir) + } + }) + + it("caps long names with a hash suffix", () => { + const long = `password~${"U".repeat(300)}` + const dir = encodeOwnerDir(long) + assert.ok(Buffer.byteLength(dir) <= 200, `length ${Buffer.byteLength(dir)}`) + assert.match(dir, /~h[0-9a-f]{16}$/) + assert.notStrictEqual(encodeOwnerDir(`password~${"U".repeat(301)}`), dir) + }) +}) + +describe("owner state registry", () => { + it("gives each owner its own maps and reuses them", () => { + const a = getOwnerState("password~a-test") + const b = getOwnerState("password~b-test") + assert.notStrictEqual(a.clients, b.clients) + assert.notStrictEqual(a.metricsServerMap, b.metricsServerMap) + assert.strictEqual(getOwnerState("password~a-test"), a) + assert.ok(allOwnerStates().includes(a) && allOwnerStates().includes(b)) + }) +}) diff --git a/apps/server/src/__tests__/session.test.ts b/apps/server/src/__tests__/session.test.ts deleted file mode 100644 index 86feb029..00000000 --- a/apps/server/src/__tests__/session.test.ts +++ /dev/null @@ -1,122 +0,0 @@ -/* eslint-disable @typescript-eslint/no-explicit-any */ -import { describe, it, beforeEach, afterEach } from "node:test" -import assert from "node:assert" -import { - ensureSession, - authorizeConnection, - isConnectionAuthorized, - revokeConnection, - hasAuthorizedSession, - setSessionExpiryListener, - _resetSessions -} from "../session" -import type { IncomingMessage } from "http" - -const TTL_MS = 24 * 60 * 60 * 1000 - -const makeReq = (cookie?: string): IncomingMessage => - ({ headers: cookie ? { cookie } : {}, socket: {} }) as any - -const cookieFor = (sessionId: string) => `vk_sid=${sessionId}` - -// override Date.now so we can control the passage of time in tests and not wait for real time to pass -let now = 0 -const realNow = Date.now - -beforeEach(() => { - now = 1_000_000_000 - Date.now = () => now - _resetSessions() -}) - -afterEach(() => { - Date.now = realNow -}) - -describe("session", () => { - // When two sessions share a connection, it should stay authorized until the last one revokes it. - it("only frees a connection once the last session using it revokes it", () => { - const sessionA = ensureSession(makeReq()).sessionId - const sessionB = ensureSession(makeReq()).sessionId - authorizeConnection(sessionA, "shared") - authorizeConnection(sessionB, "shared") - - revokeConnection(sessionA, "shared") - assert.strictEqual(hasAuthorizedSession("shared"), true) - - revokeConnection(sessionB, "shared") - assert.strictEqual(hasAuthorizedSession("shared"), false) - }) - - it("hands the expiry listener any connection left with no session behind it", () => { - const orphaned: string[][] = [] - setSessionExpiryListener((ids) => orphaned.push(ids)) - - const { sessionId } = ensureSession(makeReq()) - authorizeConnection(sessionId, "conn-1") - - now += TTL_MS + 1 - ensureSession(makeReq(cookieFor(sessionId))) - - assert.deepStrictEqual(orphaned, [["conn-1"]]) - }) - - // One session expiring shouldn't drop a connection another session is still using. - it("leaves a shared connection alone when only one of its sessions expires", () => { - const orphaned: string[][] = [] - setSessionExpiryListener((ids) => orphaned.push(ids)) - - const sessionA = ensureSession(makeReq()).sessionId - authorizeConnection(sessionA, "shared") - - // Start sessionB almost a full TTL later so it's still alive once sessionA times out. - now += TTL_MS - 1 - const sessionB = ensureSession(makeReq()).sessionId - authorizeConnection(sessionB, "shared") - - // sessionA is now past its TTL but sessionB isn't; touching sessionA trips its expiry. - now += 2 - ensureSession(makeReq(cookieFor(sessionA))) - - assert.deepStrictEqual(orphaned, []) - assert.strictEqual(isConnectionAuthorized(sessionB, "shared"), true) - }) - - // Shared use: two users connect to the same cluster — both are authorized - it("allows multiple sessions to share a connection when both have connected", () => { - const sessionA = ensureSession(makeReq()).sessionId - const sessionB = ensureSession(makeReq()).sessionId - const connectionId = "10-0-1-5-6379-db0" - - // Both users go through the connect flow (connectPending → authorizeConnection) - authorizeConnection(sessionA, connectionId) - authorizeConnection(sessionB, connectionId) - - // Both can access the shared connection - assert.strictEqual(isConnectionAuthorized(sessionA, connectionId), true) - assert.strictEqual(isConnectionAuthorized(sessionB, connectionId), true) - }) - - // Attack: a session tries to use a connection it never established - it("rejects a session that never connected from using another session's connection", () => { - const victim = ensureSession(makeReq()).sessionId - const attacker = ensureSession(makeReq()).sessionId - const connectionId = "10-0-1-5-6379-db0" - - // Victim connects — gets authorized - authorizeConnection(victim, connectionId) - - // Attacker skips connectPending and tries to use the connection directly - assert.strictEqual(isConnectionAuthorized(attacker, connectionId), false) - }) - - // Attack: guessing deterministic connectionIds without connecting - it("rejects guessed connectionIds from unauthorized sessions", () => { - const attacker = ensureSession(makeReq()).sessionId - - // connectionIds are deterministic (host-port-dbN) and therefore guessable - assert.strictEqual(isConnectionAuthorized(attacker, "production-redis-6379-db0"), false) - assert.strictEqual(isConnectionAuthorized(attacker, "10-0-1-5-6379-db0"), false) - assert.strictEqual(isConnectionAuthorized(attacker, "cache-cluster-6379-db2"), false) - }) -}) diff --git a/apps/server/src/__tests__/socket-binding.test.ts b/apps/server/src/__tests__/socket-binding.test.ts new file mode 100644 index 00000000..6347a0f2 --- /dev/null +++ b/apps/server/src/__tests__/socket-binding.test.ts @@ -0,0 +1,247 @@ +/* eslint-disable @typescript-eslint/no-explicit-any */ +import { describe, it, beforeEach, afterEach, mock } from "node:test" +import assert from "node:assert" +import { GlideClient } from "@valkey/valkey-glide" +import { CONNECTION_TEARDOWN_DELAY_MS, VALKEY } from "valkey-common" +import { + bindConnection, + createSocketBinding, + resolveAction, + scheduleRelease, + unbindConnection +} from "../socket-binding" +import { createOwnerState, getEnvOwnerState, type OwnerState } from "../owner" +import { teardownConnection } from "../connection" +import { connectPending } from "../actions/connection" +import { scopedId, subscribe, unsubscribe, _reset as resetNodeWatchers } from "../node-watchers" + +const fakeClient = () => ({ close: mock.fn() }) as any + +const action = (type: string, payload: Record) => ({ type, payload }) as any + +describe("resolveAction", () => { + let alice: OwnerState + const lookup = (key: string) => (key === alice.ownerKey ? alice : undefined) + + beforeEach(() => { + alice = createOwnerState("password~alice") + alice.clients.set("h-7001-db0", { client: fakeClient(), clusterId: "cluster-1" }) + alice.clients.set("h-6379-db0", { client: fakeClient() }) + }) + + it("rejects a connectionId this socket did not bind", () => { + const binding = createSocketBinding() + const result = resolveAction(binding, action(VALKEY.COMMAND.sendRequested, { connectionId: "h-6379-db0" }), lookup) + assert.strictEqual(result.ok, false) + }) + + it("accepts a bound connectionId and returns that owner's state", () => { + const binding = createSocketBinding() + bindConnection(binding, alice.ownerKey, "h-6379-db0") + const result = resolveAction(binding, action(VALKEY.COMMAND.sendRequested, { connectionId: "h-6379-db0" }), lookup) + assert.ok(result.ok) + assert.strictEqual(result.ownerKey, alice.ownerKey) + assert.strictEqual(result.state, alice) + }) + + it("accepts the db-less nodeId of a bound connection with its own clusterId", () => { + const binding = createSocketBinding() + bindConnection(binding, alice.ownerKey, "h-7001-db0") + const result = resolveAction( + binding, + action(VALKEY.MONITOR.monitorRequested, { connectionId: "h-7001", clusterId: "cluster-1" }), + lookup, + ) + assert.ok(result.ok) + }) + + it("rejects a clusterId that is not the cluster of the bound connection", () => { + const binding = createSocketBinding() + bindConnection(binding, alice.ownerKey, "h-6379-db0") + const result = resolveAction( + binding, + action(VALKEY.COMMANDLOGS.commandLogsRequested, { connectionId: "h-6379-db0", clusterId: "cluster-1" }), + lookup, + ) + assert.strictEqual(result.ok, false) + }) + + it("rejects a clusterId with no connectionId", () => { + const binding = createSocketBinding() + bindConnection(binding, alice.ownerKey, "h-7001-db0") + const result = resolveAction(binding, action(VALKEY.COMMANDLOGS.commandLogsRequested, { clusterId: "cluster-1" }), lookup) + assert.strictEqual(result.ok, false) + }) + + it("treats an empty clusterId as absent", () => { + const binding = createSocketBinding() + bindConnection(binding, alice.ownerKey, "h-6379-db0") + const result = resolveAction( + binding, + action(VALKEY.COMMANDLOGS.commandLogsRequested, { connectionId: "h-6379-db0", clusterId: "" }), + lookup, + ) + assert.ok(result.ok) + }) + + it("resolves a connect to the owner named by its connection details", () => { + const binding = createSocketBinding() + const result = resolveAction( + binding, + action(VALKEY.CONNECTION.connectPending, { + connectionId: "h-6379-db0", + connectionDetails: { host: "h", port: "6379", username: "bob", password: "pw" }, + }), + ) + assert.ok(result.ok) + assert.strictEqual(result.ownerKey, "password~bob") + }) + + it("rejects a connect without connection details", () => { + const result = resolveAction(createSocketBinding(), action(VALKEY.CONNECTION.connectPending, { connectionId: "x" })) + assert.strictEqual(result.ok, false) + }) + + it("forgets a connection after unbind", () => { + const binding = createSocketBinding() + bindConnection(binding, alice.ownerKey, "h-6379-db0") + unbindConnection(binding, "h-6379-db0") + const result = resolveAction(binding, action(VALKEY.COMMAND.sendRequested, { connectionId: "h-6379-db0" }), lookup) + assert.strictEqual(result.ok, false) + }) + + it("returns the previous owner when a socket rebinds a connectionId as another user", () => { + const binding = createSocketBinding() + assert.strictEqual(bindConnection(binding, "password~alice", "h-6379-db0"), undefined) + assert.strictEqual(bindConnection(binding, "password~bob", "h-6379-db0"), "password~alice") + }) +}) + +describe("connectPending resume", () => { + it("asks for the password when a password connection resumes without one", async () => { + const sent: any[] = [] + const ws = { send: (m: string) => sent.push(JSON.parse(m)) } as any + const original = GlideClient.createClient + const createClient = mock.fn(async () => { throw new Error("must not connect") }) + GlideClient.createClient = createClient as any + try { + const state = createOwnerState("password~default") + await connectPending({ ws, ownerKey: state.ownerKey, ownerState: state, binding: createSocketBinding() } as any)( + action(VALKEY.CONNECTION.connectPending, { + connectionId: "h-6379-db0", + isResume: true, + connectionDetails: { host: "h", port: "6379", username: "alice", tls: false, verifyTlsCertificate: false, db: 0 }, + }), + ) + assert.strictEqual(createClient.mock.calls.length, 0) + assert.strictEqual(sent.length, 1) + assert.strictEqual(sent[0].type, VALKEY.CONNECTION.connectRejected) + assert.strictEqual(sent[0].payload.requiresAuth, true) + } finally { + GlideClient.createClient = original + } + }) +}) + +describe("release", () => { + beforeEach(() => { + resetNodeWatchers() + mock.timers.enable({ apis: ["setTimeout"] }) + }) + afterEach(() => { + mock.timers.reset() + mock.restoreAll() + }) + + it("tears down a client after the delay when no socket binds it", () => { + const state = createOwnerState("password~rel-a") + const client = fakeClient() + state.clients.set("h-6379-db0", { client }) + + scheduleRelease(state, "h-6379-db0") + mock.timers.tick(CONNECTION_TEARDOWN_DELAY_MS - 1) + assert.strictEqual(client.close.mock.calls.length, 0) + mock.timers.tick(1) + assert.strictEqual(client.close.mock.calls.length, 1) + assert.strictEqual(state.clients.has("h-6379-db0"), false) + }) + + it("keeps the client when a socket binds it again within the delay", () => { + const state = createOwnerState("password~rel-b") + const client = fakeClient() + state.clients.set("h-6379-db0", { client }) + const ws = {} as any + + scheduleRelease(state, "h-6379-db0") + subscribe(scopedId(state.ownerKey, "h-6379-db0"), ws) + mock.timers.tick(CONNECTION_TEARDOWN_DELAY_MS) + assert.strictEqual(client.close.mock.calls.length, 0) + unsubscribe(scopedId(state.ownerKey, "h-6379-db0"), ws) + }) + + it("does not touch another owner's client on the same connectionId", () => { + const a = createOwnerState("password~rel-c") + const b = createOwnerState("password~rel-d") + const clientA = fakeClient() + const clientB = fakeClient() + a.clients.set("h-6379-db0", { client: clientA }) + b.clients.set("h-6379-db0", { client: clientB }) + + scheduleRelease(a, "h-6379-db0") + mock.timers.tick(CONNECTION_TEARDOWN_DELAY_MS) + assert.strictEqual(clientA.close.mock.calls.length, 1) + assert.strictEqual(clientB.close.mock.calls.length, 0) + assert.strictEqual(b.clients.has("h-6379-db0"), true) + }) +}) + +describe("teardown prunes an owner's collectors", () => { + afterEach(() => mock.restoreAll()) + + it("stops the cluster's collectors after the owner's last cluster connection, and keeps nodes still used", () => { + const killed: number[] = [] + mock.method(process, "kill", (pid: number) => { killed.push(pid); return true }) + + const state = createOwnerState("password~prune") + const clusterClient = fakeClient() + state.clients.set("a-6379-db0", { client: fakeClient() }) + state.clients.set("b-7001-db0", { client: clusterClient, clusterId: "c1" }) + state.clusterNodesRegistry.set("c1", { + "b-7001": { host: "b", port: 7001, tls: false, verifyTlsCertificate: false }, + "c-7002": { host: "c", port: 7002, tls: false, verifyTlsCertificate: false }, + }) + state.clusterCredentials.set("c1", "pw") + for (const [nodeId, pid] of [["a-6379", 1], ["b-7001", 2], ["c-7002", 3]] as const) { + state.metricsServerMap.set(nodeId, { metricsURI: "", pid, lastSeen: 0 }) + state.collectorKeys.set(nodeId, `key-${pid}`) + } + + teardownConnection(state, "b-7001-db0") + + assert.strictEqual(clusterClient.close.mock.calls.length, 1) + assert.deepStrictEqual(killed.sort(), [2, 3]) + assert.deepStrictEqual([...state.metricsServerMap.keys()], ["a-6379"]) + assert.deepStrictEqual([...state.collectorKeys.keys()], ["a-6379"]) + assert.strictEqual(state.clusterNodesRegistry.has("c1"), false) + assert.strictEqual(state.clusterCredentials.has("c1"), false) + }) + + it("does not prune the env owner's collectors", () => { + const killed: number[] = [] + mock.method(process, "kill", (pid: number) => { killed.push(pid); return true }) + mock.method(globalThis, "fetch", async () => new Response("{}", { status: 500 })) + mock.method(console, "warn", () => {}) + + const env = getEnvOwnerState() + env.clients.set("env-6379-db0", { client: fakeClient() }) + env.metricsServerMap.set("other-7000", { metricsURI: "http://127.0.0.1:1", pid: 9, lastSeen: 0 }) + try { + teardownConnection(env, "env-6379-db0") + assert.deepStrictEqual(killed, []) + assert.strictEqual(env.metricsServerMap.has("other-7000"), true) + } finally { + env.metricsServerMap.clear() + env.clients.clear() + } + }) +}) diff --git a/apps/server/src/__tests__/topology-refresh-preconfigured.test.ts b/apps/server/src/__tests__/topology-refresh-preconfigured.test.ts index 83ee5fa0..ece50b62 100644 --- a/apps/server/src/__tests__/topology-refresh-preconfigured.test.ts +++ b/apps/server/src/__tests__/topology-refresh-preconfigured.test.ts @@ -68,3 +68,14 @@ describe("resolveClusterRefreshTarget (preconfigured / headless)", () => { assert.deepStrictEqual(target?.nodeInfo, clusterNodes.node1) }) }) + +describe("env owner (preconfigured connection)", () => { + it("maps the preconfigured identity to the module state", async () => { + const { clients, metricsServerMap } = await import("../metrics-orchestrator") + const { getEnvOwnerState, getOwnerState, ownerOf } = await import("../owner") + const state = getOwnerState(ownerOf(initialConnectionDetails)) + assert.strictEqual(state, getEnvOwnerState()) + assert.strictEqual(state.clients, clients) + assert.strictEqual(state.metricsServerMap, metricsServerMap) + }) +}) diff --git a/apps/server/src/__tests__/utils.test.ts b/apps/server/src/__tests__/utils.test.ts index df7189c6..f935882c 100644 --- a/apps/server/src/__tests__/utils.test.ts +++ b/apps/server/src/__tests__/utils.test.ts @@ -1,48 +1,31 @@ /* eslint-disable @typescript-eslint/no-explicit-any */ -import { describe, it, beforeEach } from "node:test" +import { describe, it } from "node:test" import assert from "node:assert" import { GlideClusterClient } from "@valkey/valkey-glide" import { parseInfo, parseResponse, parseClusterInfo, getExistingClusterClient } from "../utils" -import { ensureSession, authorizeConnection, _resetSessions } from "../session" -import type { IncomingMessage } from "http" -describe("getExistingClusterClient (session-scoped reuse)", () => { - let sessionId: string +describe("getExistingClusterClient (owner-scoped reuse)", () => { const makeClusterEntry = () => ({ // Satisfies isClusterClientEntry's `instanceof GlideClusterClient` check without a real client. client: Object.create(GlideClusterClient.prototype) as GlideClusterClient, clusterId: "cluster-1", }) - beforeEach(() => { - _resetSessions() - sessionId = ensureSession({ headers: {}, socket: {} } as unknown as IncomingMessage).sessionId - }) - - it("reuses the shared cluster client when the session owns the matched sibling node", async () => { + it("reuses the owner's cluster client for a sibling node", async () => { const entry = makeClusterEntry() const clients = new Map([["node-A", entry]]) - // Session connected to node-A earlier -> owns it. - authorizeConnection(sessionId, "node-A") // Connecting to node-B of the same cluster: discovery surfaces node-A as an existing entry. const discovered = { "node-A": {} as never, "node-B": {} as never } - const result = await getExistingClusterClient(discovered, clients, sessionId) + const result = await getExistingClusterClient(discovered, clients) - assert.strictEqual(result?.client, entry.client, "should reuse the owned cluster's client for the new node") + assert.strictEqual(result?.client, entry.client, "should reuse the owner's cluster client for the new node") }) - it("does NOT reuse when the session does not own the matched node", async () => { - const entry = makeClusterEntry() - const clients = new Map([["node-A", entry]]) - // node-A belongs to a DIFFERENT session. - const otherSession = ensureSession({ headers: {}, socket: {} } as unknown as IncomingMessage).sessionId - authorizeConnection(otherSession, "node-A") - + it("finds nothing in another owner's empty map", async () => { const discovered = { "node-A": {} as never } - const result = await getExistingClusterClient(discovered, clients, sessionId) - - assert.strictEqual(result, undefined, "must not inherit another session's cluster client") + const result = await getExistingClusterClient(discovered, new Map()) + assert.strictEqual(result, undefined) }) }) diff --git a/apps/server/src/actions/cluster.ts b/apps/server/src/actions/cluster.ts index e43d9ca2..afa5e2e4 100644 --- a/apps/server/src/actions/cluster.ts +++ b/apps/server/src/actions/cluster.ts @@ -3,12 +3,12 @@ import { type Deps, withDeps } from "./utils" import { setClusterDashboardData } from "../set-dashboard-data" export const setClusterData = withDeps( - async ({ ws, clients, connectionId, action, clusterNodesRegistry }) => { + async ({ ws, clients, connectionId, action, ownerState }) => { const connection = clients.get(connectionId) if (connection && connection.client instanceof GlideClusterClient) { const { clusterId } = action.payload - await setClusterDashboardData(clusterId as string, connection.client, ws, connectionId, clusterNodesRegistry) + await setClusterDashboardData(clusterId as string, connection.client, ws, connectionId, ownerState) } }, ) diff --git a/apps/server/src/actions/config.ts b/apps/server/src/actions/config.ts index 90e68142..28d94781 100644 --- a/apps/server/src/actions/config.ts +++ b/apps/server/src/actions/config.ts @@ -95,12 +95,14 @@ export const abortConfigSessionsForSocket = (ws: WebSocket): void => { * reply reaches the frontend. */ export const runConfigPushSession = withDeps( - async ({ ws, metricsServerMap, action, clusterNodesRegistry }) => { + async ({ ws, metricsServerMap, action, clusterNodesRegistry, ownerKey }) => { const { connectionId, clusterId, config } = action.payload const replyId: AggregateReplyId = typeof clusterId === "string" ? { clusterId } : { nodeId: toNodeId(connectionId) } const targetId = "clusterId" in replyId ? replyId.clusterId : replyId.nodeId + // Sessions are per owner, so one user's update never supersedes another's. + const sessionKey = `${ownerKey ?? ""}|${targetId}` const targetNodeIds = typeof clusterId === "string" ? Object.keys(clusterNodesRegistry.get(clusterId) ?? {}) : [targetId] @@ -110,9 +112,9 @@ export const runConfigPushSession = withDeps( })) // Supersede any in-flight session for this target. - configSessions.get(targetId)?.controller.abort() + configSessions.get(sessionKey)?.controller.abort() const controller = new AbortController() - configSessions.set(targetId, { controller, ws }) + configSessions.set(sessionKey, { controller, ws }) const onNodeStatusUpdate = (update: NodeStatusUpdate): void => { safeSend(ws, JSON.stringify({ @@ -134,8 +136,8 @@ export const runConfigPushSession = withDeps( } return result } finally { - if (configSessions.get(targetId)?.controller === controller) { - configSessions.delete(targetId) + if (configSessions.get(sessionKey)?.controller === controller) { + configSessions.delete(sessionKey) } } }, @@ -148,8 +150,7 @@ export const runConfigPushSession = withDeps( * node samples with stale settings. */ export const updateConfig = withDeps( - async ({ ws, clients, connectionId, metricsServerMap, connectedNodesByCluster, clusterNodesRegistry, action }) => { - const deps: Deps = { ws, clients, connectionId, metricsServerMap, connectedNodesByCluster, clusterNodesRegistry } + async ({ action, ...deps }: Deps & { action: ReduxAction }) => { const { config, monitorAction } = action.payload // Without a config there is no session to run diff --git a/apps/server/src/actions/connection.ts b/apps/server/src/actions/connection.ts index a4543dfa..475cc544 100644 --- a/apps/server/src/actions/connection.ts +++ b/apps/server/src/actions/connection.ts @@ -1,11 +1,12 @@ import { GlideClusterClient } from "@valkey/valkey-glide" -import { EndpointType, toNodeId } from "valkey-common" +import { EndpointType } from "valkey-common" import { VALKEY } from "valkey-common" -import { connectToValkey, teardownConnection } from "../connection" -import { unsubscribe, getWatcherCount } from "../node-watchers" +import { connectToValkey, teardownConnection } from "../connection" +import { unsubscribe, getWatcherCount, scopedId } from "../node-watchers" +import { bindConnection, connectedNodesFor, scheduleRelease, unbindConnection } from "../socket-binding" +import { getOwnerState } from "../owner" import { type Deps, withDeps } from "./utils" import { setClusterDashboardData } from "../set-dashboard-data" -import { authorizeConnection, isConnectionAuthorized, revokeConnection, hasAuthorizedSession } from "../session" export interface ConnectionDetails { host: string; @@ -33,60 +34,45 @@ type ConnectPayload = { isResume?: boolean, } +const isPasswordAuth = (details: ConnectionDetails) => + details.authType !== "iam" && details.authType !== "gcp-iam" + export const connectPending = withDeps( - async ({ ws, clients, action, connectedNodesByCluster, metricsServerMap, clusterNodesRegistry, sessionId }) => { + async ({ ws, action, ownerKey, ownerState, binding }) => { const payload = action.payload as ConnectPayload - const { connectionId } = payload - - // ws.sessionId is assigned for every upgrade (ensureSession), so this is a defensive - // invariant, not the expiry path (expiry is handled by isConnectionAuthorized returning - // false because the session record is gone). Fail closed rather than connect without a - // session to scope client reuse to. - if (!sessionId) { - ws.send(JSON.stringify({ - type: VALKEY.CONNECTION.connectRejected, - payload: { connectionId, errorMessage: "Unable to establish a session. Please reload and try again.", requiresAuth: true }, - })) - return - } - - // if connection is being resumed, check if the session is still valid and if the connection exists - if (payload.isResume && (!isConnectionAuthorized(sessionId, connectionId) || !clients.has(connectionId))) { - ws.send(JSON.stringify({ - type: VALKEY.CONNECTION.connectRejected, - payload: { connectionId, errorMessage: "Session expired. Please sign in again.", requiresAuth: true }, - })) - return - } + const { connectionId, connectionDetails } = payload - // A retry reconnects an existing connection and (for clusters) closes + repoints the shared - // client, so require ownership — same rule as resume. The frontend only retries connections it - // previously established, so this never rejects a legitimate retry; it prevents one session's - // retry from closing or repointing a connection another session owns (e.g. a same-endpoint - // collision on a shared instance). - if (payload.isRetry && !isConnectionAuthorized(sessionId, connectionId)) { + // A resume without a password cannot prove the identity, and no server-side + // session remembers it, so the user must enter the password again. + if (payload.isResume && isPasswordAuth(connectionDetails) && !connectionDetails.password) { ws.send(JSON.stringify({ type: VALKEY.CONNECTION.connectRejected, - payload: { connectionId, errorMessage: "Session expired. Please sign in again.", requiresAuth: true }, + payload: { connectionId, errorMessage: "Enter the password to reconnect.", requiresAuth: true }, })) return } const client = await connectToValkey( - { clients, connectedNodesByCluster, clusterNodesRegistry, metricsServerMap }, + { ...ownerState, connectedNodesByCluster: connectedNodesFor(binding, ownerKey) }, ws, - { ...payload, sessionId }, + payload, ) - - if (client) { - authorizeConnection(sessionId, connectionId) - authorizeConnection(sessionId, toNodeId(connectionId)) + if (!client) return + + // Valkey accepted the credentials: the socket may now act on this connection. + const previousOwner = bindConnection(binding, ownerKey, connectionId) + if (previousOwner !== undefined) { + // The socket reconnected the same connectionId as another Valkey user. + // Release the old owner's watch so its client can be torn down. + if (unsubscribe(scopedId(previousOwner, connectionId), ws)) { + scheduleRelease(getOwnerState(previousOwner), connectionId) + } } }, ) export const resetConnection = withDeps( - async ({ ws, connectionId, clients, action, clusterNodesRegistry }) => { + async ({ ws, connectionId, clients, action, ownerState }) => { const entry = clients.get(connectionId) if (!entry) { @@ -98,19 +84,19 @@ export const resetConnection = withDeps( const { clusterId } = action.payload as unknown as { clusterId: string } if (client instanceof GlideClusterClient) { - await setClusterDashboardData(clusterId, client, ws, connectionId, clusterNodesRegistry) + await setClusterDashboardData(clusterId, client, ws, connectionId, ownerState) } }, ) export const closeConnection = withDeps( - async ({ ws, clients, action, metricsServerMap, connectedNodesByCluster, clusterNodesRegistry, sessionId }) => { + async ({ ws, clients, action, connectedNodesByCluster, ownerKey, ownerState, binding }) => { const { connectionId } = action.payload const connection = clients.get(connectionId) const clusterId = connection?.clusterId - revokeConnection(sessionId, connectionId) - unsubscribe(connectionId, ws) + unbindConnection(binding, connectionId) + unsubscribe(scopedId(ownerKey, connectionId), ws) // Always ack the requesting client — UI needs confirmation ws.send(JSON.stringify({ @@ -118,26 +104,17 @@ export const closeConnection = withDeps( payload: { connectionId }, })) - // Don't tear down a shared connection another session still owns - if (getWatcherCount(connectionId) > 0 || hasAuthorizedSession(connectionId)) { - return - } const nodes = connectedNodesByCluster.get(clusterId!) - - // Remove node from cluster map accordingly + // Remove node from this socket's cluster map accordingly if (clusterId && nodes) { - if (nodes.length === 1) { - connectedNodesByCluster.delete(clusterId) - } else { - const index = nodes.indexOf(connectionId) - if (index !== -1) { - nodes.splice(index, 1) - } - } + const index = nodes.indexOf(connectionId) + if (index !== -1) nodes.splice(index, 1) + if (nodes.length === 0) connectedNodesByCluster.delete(clusterId) } - teardownConnection( - { clients, clusterNodesRegistry, metricsServerMap }, - connectionId, - ) + + // Don't tear down a connection another socket of the same owner still uses + if (getWatcherCount(scopedId(ownerKey, connectionId)) > 0) return + + teardownConnection(ownerState, connectionId) }, ) diff --git a/apps/server/src/actions/monitorAction.ts b/apps/server/src/actions/monitorAction.ts index 4c099402..158a58af 100644 --- a/apps/server/src/actions/monitorAction.ts +++ b/apps/server/src/actions/monitorAction.ts @@ -1,7 +1,7 @@ import { type WebSocket } from "ws" import { VALKEY, type MonitorAction, type NodeReplyId, toNodeId, buildUrl } from "valkey-common" import { withDeps, type Deps, fetchWithTimeout } from "./utils" -import { getOtherWatchers } from "../node-watchers" +import { getOtherWatchers, scopedId } from "../node-watchers" type MonitorResponse = { monitorRunning: boolean @@ -44,7 +44,7 @@ const sendMonitorError = ( } export const monitorRequested = withDeps( - async ({ ws, metricsServerMap, action, clusterNodesRegistry }) => { + async ({ ws, metricsServerMap, action, clusterNodesRegistry, ownerKey }) => { const { connectionId, clusterId, monitorAction } = action.payload // Internal restriction used by the save flow to toggle only the nodes // whose config push succeeded. @@ -57,13 +57,17 @@ export const monitorRequested = withDeps( : allNodeIds const registered = targetNodeIds.filter((id) => metricsServerMap.has(id)) await Promise.all(registered.map((nodeId) => - runMonitorForNode(ws, metricsServerMap.get(nodeId)?.metricsURI, monitorAction, { clusterId, nodeId }, nodeId), + runMonitorForNode( + ws, metricsServerMap.get(nodeId)?.metricsURI, monitorAction, { clusterId, nodeId }, scopedId(ownerKey, nodeId), + ), )) } else { // Standalone path. Monitor state is keyed by the db-less nodeId, so the // reply carries { nodeId }. const nodeId = toNodeId(connectionId) - await runMonitorForNode(ws, metricsServerMap.get(nodeId)?.metricsURI, monitorAction, { nodeId }, connectionId) + await runMonitorForNode( + ws, metricsServerMap.get(nodeId)?.metricsURI, monitorAction, { nodeId }, scopedId(ownerKey, connectionId), + ) } }) diff --git a/apps/server/src/actions/utils.ts b/apps/server/src/actions/utils.ts index f5e7ae47..20f98d3c 100644 --- a/apps/server/src/actions/utils.ts +++ b/apps/server/src/actions/utils.ts @@ -1,6 +1,8 @@ import { type GlideClient, type GlideClusterClient } from "@valkey/valkey-glide" import { FETCH_TIMEOUT_MS } from "valkey-common" import { ClusterNodeMap, MetricsServerMap } from "../metrics-orchestrator" +import type { OwnerState } from "../owner" +import type { SocketBinding } from "../socket-binding" import type WebSocket from "ws" export type Deps = { @@ -10,7 +12,11 @@ export type Deps = { metricsServerMap: MetricsServerMap, connectedNodesByCluster: Map, clusterNodesRegistry: Map, - sessionId?: string, + /** Owner identity the action runs as. The maps above are this owner's. */ + ownerKey: string, + ownerState: OwnerState, + /** What the requesting socket may act on. */ + binding: SocketBinding, } export type ReduxAction = { diff --git a/apps/server/src/connection.ts b/apps/server/src/connection.ts index 1ba6375f..eedff9c3 100644 --- a/apps/server/src/connection.ts +++ b/apps/server/src/connection.ts @@ -17,25 +17,45 @@ import { checkJsonModuleAvailability } from "./check-json-module" import { type ConnectionDetails } from "./actions/connection" import { isWebMode, - MetricsServerMap, startMetricsServer, - clusterCredentials, reconcileClusterMetricsServers, + pruneOwnerCollectors, isKubernetes, forgetCollectorKey, ClusterNodeMap, type NodeInfo } from "./metrics-orchestrator" -import { subscribe } from "./node-watchers" +import { scopedId, subscribe } from "./node-watchers" import { clearCpuSamples } from "./node-utilization" -import { createClusterValkeyClient, createStandaloneValkeyClient } from "./valkey-client" -import { isConnectionAuthorized } from "./session" +import { createAuthProbeClient, createClusterValkeyClient, createStandaloneValkeyClient } from "./valkey-client" import { withAuthLimit } from "./auth-limiter" +import { createOwnerState, type OwnerState } from "./owner" -export type ConnectionContext = { - clients: Map +/** + * One owner's state plus the requesting socket's cluster bookkeeping. All + * clients, topology and collectors here belong to the Valkey identity the + * connection authenticates as. + */ +export type ConnectionContext = OwnerState & { connectedNodesByCluster: Map - clusterNodesRegistry: Map - metricsServerMap: MetricsServerMap +} + +// Fill-in state for callers that pass only some maps (tests), keyed by their +// `clients` map so concurrent calls with the same maps share in-flight state. +const fillIns = new WeakMap() + +/** + * Complete a partial context with the missing owner-state fields. + */ +export const toConnectionContext = ( + partial: Partial & { connectedNodesByCluster?: Map }, +): ConnectionContext => { + const anchor = partial.clients ?? partial + let fill = fillIns.get(anchor) + if (!fill) { + fill = createOwnerState(partial.ownerKey ?? "password~default") + fillIns.set(anchor, fill) + } + return { ...fill, connectedNodesByCluster: new Map(), ...partial } } type ClusterCommit = { @@ -75,11 +95,9 @@ type ClusterConnectFulfilledPayload = { connectionDetails: ClusterConnectionDetails } -// Dedup concurrent cluster-client creations by clusterId. -const inFlightClusterClients = new Map>() - -// Test-only: reset in-flight state between cases. -export const _resetInFlightClusterClients = () => inFlightClusterClients.clear() +// Concurrent cluster-client creations are deduplicated per owner by clusterId +// (OwnerState.inFlightClusterClients). Kept for the existing tests. +export const _resetInFlightClusterClients = () => {} /** * Read the server's configured database count via `CONFIG GET`, trying each of @@ -131,35 +149,38 @@ class ConnectionRejectedError extends Error { // only after the first has fully committed (or failed), at which point the // getExistingConnection path below reuses the committed // entry (standalone or cluster). -const connectInFlight = new Map>() - -export const _resetConnectInFlight = () => connectInFlight.clear() +// The map lives in the owner's state (OwnerState.connectInFlight). Kept for the existing tests. +export const _resetConnectInFlight = () => {} /** * Prove that Valkey accepts `details`' credentials on this node: open a * short-lived client, then close it. Throws on rejection. Counted by the * failed-AUTH limit. Valkey Admin never stores or compares secrets. + * + * Every connect and discovery runs this first. The working clients use + * static node discovery, which does not reject bad credentials at creation. */ async function verifyCredentials( details: ConnectionDetails, - options: Parameters[0], + options: Parameters[0], ): Promise { - const probe = await withAuthLimit(details, () => createStandaloneValkeyClient(options)) + const probe = await withAuthLimit(details, () => createAuthProbeClient(options)) try { probe.close() } catch (err) { console.warn("Error closing AUTH probe client:", err) } } export async function connectToValkey( - ctx: ConnectionContext, + partialCtx: Partial, ws: WebSocket, payload: { connectionDetails: ConnectionDetails connectionId: string isRetry?: boolean - sessionId: string }, ) { + const ctx = toConnectionContext(partialCtx) + const { connectInFlight } = ctx const { connectionId } = payload const priorConnect = connectInFlight.get(connectionId) ?? Promise.resolve() // .catch swallows the prior connect's failure so our turn still runs. @@ -183,10 +204,9 @@ async function connectToValkeyLocked( connectionDetails: ConnectionDetails connectionId: string isRetry?: boolean - sessionId: string }, ) { - const { clients, clusterNodesRegistry, metricsServerMap } = ctx + const { clients, clusterNodesRegistry, metricsServerMap, clusterCredentials, inFlightClusterClients } = ctx const { host, port, username, password, tls: useTLS, @@ -229,9 +249,16 @@ async function connectToValkeyLocked( ) } - // A retry replaces the stale client, but only after the new client has - // authenticated below, so a retry with wrong credentials cannot close a - // client that works. Cluster clients are repointed in updateClusterNodesClient. + // Valkey must accept the presented credentials before anything else: before + // an existing client is handed over (the owner identity is not secret, so it + // never grants reuse by itself) and before a retry closes the stale client. + await verifyCredentials(payload.connectionDetails, { + addresses, credentials, useTLS, verifyTlsCertificate, caCertPath, + }) + + // A retry replaces the stale client only after the new client exists, so a + // failed retry cannot close a client that works. Cluster clients are + // repointed in updateClusterNodesClient. const staleStandalone = payload.isRetry ? clients.get(connectionId)?.client : undefined const replaceStale = () => { if (!(staleStandalone instanceof GlideClient)) return @@ -243,11 +270,6 @@ async function connectToValkeyLocked( const existingConnection = await getExistingConnection(payload, clients) if (existingConnection) { - // Owner identity is not secret, so it never grants reuse by itself. Valkey - // must accept the presented credentials before the existing client is handed over. - await verifyCredentials(payload.connectionDetails, { - addresses, credentials, useTLS, verifyTlsCertificate, caCertPath, - }) if (existingConnection.clusterId) { const clusterClient = existingConnection.client as GlideClusterClient const { clusterId } = existingConnection @@ -276,17 +298,17 @@ async function connectToValkeyLocked( ...(existingDatabasesCount !== null && { databasesCount: existingDatabasesCount }), }, }) - subscribe(connectionId, ws) + subscribe(scopedId(ctx.ownerKey, connectionId), ws) return existingStandalone } - standaloneClient = await withAuthLimit(payload.connectionDetails, () => createStandaloneValkeyClient({ + standaloneClient = await createStandaloneValkeyClient({ addresses, credentials, useTLS, verifyTlsCertificate, caCertPath, - })) + }) replaceStale() // Open the registration gate: the metrics process spawned below will POST @@ -299,7 +321,7 @@ async function connectToValkeyLocked( if (!isKubernetes) { const metricsNodeId = toNodeId(connectionId) if (!metricsServerMap.has(metricsNodeId)) { - await startMetricsServer(payload.connectionDetails, metricsNodeId) + await startMetricsServer(payload.connectionDetails, metricsNodeId, ctx) } } @@ -345,7 +367,7 @@ async function connectToValkeyLocked( const { discoveredClusterNodes, clusterId } = await discoverCluster(standaloneClient, payload) standaloneClient.close() - const existingClusterConnection = await getExistingClusterClient(discoveredClusterNodes, clients, payload.sessionId) + const existingClusterConnection = await getExistingClusterClient(discoveredClusterNodes, clients) let clusterClient: GlideClusterClient if (existingClusterConnection && !payload.isRetry) { clusterClient = existingClusterConnection.client @@ -382,7 +404,7 @@ async function connectToValkeyLocked( clusterNodesRegistry.set(clusterId, discoveredClusterNodes) if (isWebMode) { - reconcileClusterMetricsServers(metricsServerMap) + reconcileClusterMetricsServers(ctx) } await commitClusterConnection(ctx, ws, { @@ -436,20 +458,20 @@ async function connectToValkeyLocked( try { previousProbe.close() } catch (err) { console.error(`Error closing standalone probe for ${connectionId}:`, err) } - standaloneClient = await withAuthLimit(payload.connectionDetails, () => createStandaloneValkeyClient({ + standaloneClient = await createStandaloneValkeyClient({ addresses, credentials, useTLS, verifyTlsCertificate, caCertPath, databaseId: db, - })) + }) clients.set(connectionId, { client: standaloneClient }) } console.log("Connected to standalone") - subscribe(payload.connectionId, ws) + subscribe(scopedId(ctx.ownerKey, payload.connectionId), ws) const [keyEvictionPolicy, jsonModuleAvailable] = await Promise.all([ getKeyEvictionPolicy(standaloneClient), @@ -543,8 +565,8 @@ export async function discoverTopology( // database selection is irrelevant for cluster commands. Skip // `databaseId` entirely so Glide does not issue `SELECT` (cluster nodes // reject `SELECT` even for db 0). - client = await withAuthLimit(connectionDetails, () => - createStandaloneValkeyClient({ addresses, credentials, useTLS, verifyTlsCertificate, caCertPath })) + await verifyCredentials(connectionDetails, { addresses, credentials, useTLS, verifyTlsCertificate, caCertPath }) + client = await createStandaloneValkeyClient({ addresses, credentials, useTLS, verifyTlsCertificate, caCertPath }) const { discoveredClusterNodes } = await discoverCluster(client, { connectionDetails }) if (Object.keys(discoveredClusterNodes).length < 1) { throw new Error("Unable to discover cluster") @@ -616,7 +638,7 @@ async function commitClusterConnection( if (nodes === undefined) connectedNodesByCluster.set(clusterId, [connectionId]) else if (!nodes.includes(connectionId)) nodes.push(connectionId) - subscribe(connectionId, ws) + subscribe(scopedId(ctx.ownerKey, connectionId), ws) sendAddCluster(ws, clusterId, discoveredClusterNodes) sendClusterConnectFulfilled(ws, { @@ -721,43 +743,37 @@ function sendClusterConnectFulfilled(ws: WebSocket, payload: ClusterConnectFulfi } export async function getExistingConnection( - payload:{connectionId: string, connectionDetails: ConnectionDetails, isRetry?: boolean, sessionId: string}, + payload:{connectionId: string, connectionDetails: ConnectionDetails, isRetry?: boolean}, clients: Map, ) : Promise<{ client: GlideClient | GlideClusterClient; clusterId?: string | undefined; } | undefined> { - const { connectionId, connectionDetails, isRetry, sessionId } = payload + const { connectionId, connectionDetails, isRetry } = payload // If the frontend is retrying a broken connection, it's not a duplicate if (isRetry) { return undefined } - const resolvedAddresses = (await resolveHostnameOrIpAddress(connectionDetails.host)).addresses + // `clients` holds only the clients of the connecting owner identity, and the + // caller must prove the credentials with a fresh AUTH before it reuses a match. // Prevent duplicate connections: - // 1) True if any resolved host:port:db is already connected - // 2) Or if this connectionId already exists as a standalone connection - - // Only reuse a cached client this session is authorized for. Otherwise a session - // could inherit another session's authenticated client by supplying a matching - // endpoint with the wrong/blank credentials (credential-blind reuse). The matched id - // is the exact node being (re)connected to; owning it is required to reuse its client. - if (clients.has(connectionId)) { - return isConnectionAuthorized(sessionId, connectionId) ? clients.get(connectionId) : undefined - } + // 1) This connectionId already has a client + // 2) Or any resolved host:port:db is already connected + if (clients.has(connectionId)) return clients.get(connectionId) + const resolvedAddresses = (await resolveHostnameOrIpAddress(connectionDetails.host)).addresses const db = connectionDetails.db const existingConnectionId = resolvedAddresses .map((address) => buildConnectionId(address, connectionDetails.port, db)) .find((key) => clients.has(key)) - if (!existingConnectionId) return undefined - return isConnectionAuthorized(sessionId, existingConnectionId) ? clients.get(existingConnectionId) : undefined + return existingConnectionId ? clients.get(existingConnectionId) : undefined } export async function closeMetricsServer( connectionId: string, - metricsServerMap: MetricsServerMap, - clients: Map, + ctx: Pick, ) { + const { metricsServerMap, clients } = ctx // Map Connection_Identifier → metrics-node-id at the boundary. const nodeId = toNodeId(connectionId) @@ -781,22 +797,32 @@ export async function closeMetricsServer( }) if (res.ok) { metricsServerMap.delete(nodeId) - forgetCollectorKey(nodeId) + forgetCollectorKey(nodeId, ctx as OwnerState) console.log(`Metrics server for ${nodeId} closed successfully`) } else console.warn("Could not kill metrics server process") } } +/** + * Close an owner's client for `connectionId` when no other entry shares it. + * + * The env owner (the preconfigured identity) keeps the lifecycle it had + * before: in Web mode its collectors and cluster topology stay. For every + * other owner, the cluster topology goes with the owner's last client on the + * cluster, and the owner's collectors stop when no remaining connection of + * that owner uses their node. + */ export function teardownConnection( - ctx: Omit, + partialCtx: Partial, connectionId: string, ) { - const { clients, clusterNodesRegistry, metricsServerMap } = ctx + const ctx = toConnectionContext(partialCtx) + const { clients, clusterNodesRegistry, clusterCredentials, ownerKey, env } = ctx - // In web mode, metrics servers are managed by the orchestrator - if (!isWebMode) { - closeMetricsServer(connectionId, metricsServerMap, clients).catch((err) => + // In web mode, the env owner's metrics servers are managed by the orchestrator + if (env && !isWebMode) { + closeMetricsServer(connectionId, ctx).catch((err) => console.error(`Error closing metrics server for ${connectionId}:`, err), ) } @@ -815,13 +841,19 @@ export function teardownConnection( if (connection.clusterId) { // Polling ends with the client; drop all CPU baselines so a reconnect starts fresh Object.keys(clusterNodesRegistry.get(connection.clusterId) ?? {}).forEach( - (nodeId) => clearCpuSamples(nodeId), + (nodeId) => clearCpuSamples(scopedId(ownerKey, nodeId)), ) - if (!isWebMode) { + if (!env || !isWebMode) { clusterNodesRegistry.delete(connection.clusterId) clusterCredentials.delete(connection.clusterId) } } } + + if (!env) { + pruneOwnerCollectors(ctx).catch((err) => + console.error(`Error stopping collectors after ${connectionId} closed:`, err), + ) + } } diff --git a/apps/server/src/index.ts b/apps/server/src/index.ts index 8fae6514..34b01864 100644 --- a/apps/server/src/index.ts +++ b/apps/server/src/index.ts @@ -5,9 +5,6 @@ import path from "path" import http from "http" import { VALKEY, - CONNECTION_TEARDOWN_DELAY_MS, - isNodeId, - toNodeId, ORCHESTRATOR_RATE_LIMIT_MAX_ENV, resolveOrchestratorRateLimitMax } from "valkey-common" @@ -32,17 +29,13 @@ import { updateConfig, enableClusterSlotStats, abortConfigSessionsForSocket } fr import { cpuUsageRequested } from "./actions/cpuUsage" import { memoryUsageRequested } from "./actions/memoryUsage" import { monitorRequested } from "./actions/monitorAction" -import { unsubscribeAll, getWatcherCount } from "./node-watchers" -import { teardownConnection } from "./connection" +import { unsubscribeAll } from "./node-watchers" import { Handler, ReduxAction, safeSend, unknownHandler, type WsActionMessage } from "./actions/utils" import { createMetricsOrchestratorRouter, startPreconfiguredMetricsServers, - metricsServerMap, - clusterNodesRegistry, initialConnectionDetails, cleanupOrchestratorResources, - clients, isKubernetes, preConfiguredConnection, getInitialClient, @@ -51,17 +44,21 @@ import { setPreconfiguredClusterId } from "./metrics-orchestrator" import { isAllowedWebSocketOrigin } from "./websocket-origin" -import { ensureSession, hasAuthorizedSession, isConnectionAuthorized, setSessionExpiryListener } from "./session" +import { allOwnerStates, getOwnerState } from "./owner" +import { + connectedNodesFor, + createSocketBinding, + resolveAction, + scheduleRelease, + type SocketBinding +} from "./socket-binding" import type { Request, Response } from "express" -import type { IncomingMessage } from "http" interface AliveWebSocket extends WebSocket { isAlive: boolean - sessionId?: string + binding: SocketBinding } -type SessionRequest = IncomingMessage & { sessionSetCookie?: string } - interface MetricsServerMessage { type: string payload: { @@ -150,53 +147,58 @@ const delay = (ms: number) => new Promise((res) => setTimeout(res, ms)) const TOPOLOGY_REDISCOVERY_TIMEOUT_MS = 10_000 async function refreshAllClusterRegistries() { - // Group live connections by cluster once; used both to pick a client to - // re-discover each cluster's topology and to target the broadcast below. - const connectionIdsByCluster = new Map() - for (const [connectionId, entry] of clients) { - if (!entry.clusterId) continue - const ids = connectionIdsByCluster.get(entry.clusterId) ?? [] - ids.push(connectionId) - connectionIdsByCluster.set(entry.clusterId, ids) - } + // Each owner identity has its own clients and topology, so refresh and + // broadcast per owner. A socket receives a cluster's update only when it + // bound one of that owner's connections to the cluster. + await Promise.all(allOwnerStates().map(async (state) => { + // Group live connections by cluster once; used both to pick a client to + // re-discover each cluster's topology and to target the broadcast below. + const connectionIdsByCluster = new Map() + for (const [connectionId, entry] of state.clients) { + if (!entry.clusterId) continue + const ids = connectionIdsByCluster.get(entry.clusterId) ?? [] + ids.push(connectionId) + connectionIdsByCluster.set(entry.clusterId, ids) + } - // Re-discover each tracked cluster before broadcasting. User-connected clusters - // use their own client + node metadata; a preconfigured cluster (K8s / headless - // Web) has no entry in `clients`, so it falls back to the initial client so - // scaled-in nodes are picked up without a UI session. Time-bounded so one hung - // node can't stall the whole loop. - await Promise.all( - [...clusterNodesRegistry.entries()].map(async ([clusterId, clusterNodes]) => { - const connectionId = connectionIdsByCluster.get(clusterId)?.[0] - const userClient = connectionId ? clients.get(connectionId)?.client : undefined - - const target = await resolveClusterRefreshTarget(clusterId, clusterNodes, userClient) - if (!target) return - - await Promise.race([ - updateClusterNodeRegistry(target.client, target.nodeInfo, clusterId), - delay(TOPOLOGY_REDISCOVERY_TIMEOUT_MS).then(() => - console.warn(`Topology re-discovery for cluster ${clusterId} timed out; broadcasting last known nodes.`), - ), - ]) - }), - ) - - for (const [clusterId, clusterNodes] of clusterNodesRegistry) { - const connectionIds = connectionIdsByCluster.get(clusterId) - if (!connectionIds) continue - - const message = JSON.stringify({ - type: VALKEY.CLUSTER.updateClusterInfo, - payload: { clusterId, clusterNodes }, - }) - - for (const ws of wss.clients) { - const { sessionId } = ws as AliveWebSocket - if (!connectionIds.some((id) => isConnectionAuthorized(sessionId, id))) continue - safeSend(ws, message) + // Re-discover each tracked cluster before broadcasting. User-connected clusters + // use their own client + node metadata; a preconfigured cluster (K8s / headless + // Web) has no entry in `clients`, so it falls back to the initial client so + // scaled-in nodes are picked up without a UI session. Time-bounded so one hung + // node can't stall the whole loop. + await Promise.all( + [...state.clusterNodesRegistry.entries()].map(async ([clusterId, clusterNodes]) => { + const connectionId = connectionIdsByCluster.get(clusterId)?.[0] + const userClient = connectionId ? state.clients.get(connectionId)?.client : undefined + + const target = await resolveClusterRefreshTarget(clusterId, clusterNodes, userClient, state.clusterNodesRegistry) + if (!target) return + + await Promise.race([ + updateClusterNodeRegistry(target.client, target.nodeInfo, clusterId, state.clusterNodesRegistry), + delay(TOPOLOGY_REDISCOVERY_TIMEOUT_MS).then(() => + console.warn(`Topology re-discovery for cluster ${clusterId} timed out; broadcasting last known nodes.`), + ), + ]) + }), + ) + + for (const [clusterId, clusterNodes] of state.clusterNodesRegistry) { + const connectionIds = connectionIdsByCluster.get(clusterId) + if (!connectionIds) continue + + const message = JSON.stringify({ + type: VALKEY.CLUSTER.updateClusterInfo, + payload: { clusterId, clusterNodes }, + }) + + for (const ws of wss.clients) { + const { binding } = ws as AliveWebSocket + if (!connectionIds.some((id) => binding?.connections.get(id) === state.ownerKey)) continue + safeSend(ws, message) + } } - } + })) } async function refreshAllClusterRegistriesLoop() { @@ -246,34 +248,14 @@ const interval = setInterval(() => { }) }, 30000) -wss.on("headers", (headers, req: SessionRequest) => { - if (req.sessionSetCookie) headers.push(`Set-Cookie: ${req.sessionSetCookie}`) -}) - -// when a session expires, check if any of its connections are still authorized by other sessions, and if not, tear them down -setSessionExpiryListener((connectionIds) => { - for (const connectionId of connectionIds) { - if (getWatcherCount(connectionId) === 0) { - teardownConnection( - { clients, clusterNodesRegistry, metricsServerMap }, - connectionId, - ) - } - } -}) - -server.on("upgrade", (req: SessionRequest, socket, head) => { +server.on("upgrade", (req, socket, head) => { if (!isAllowedWebSocketOrigin(req)) { socket.write("HTTP/1.1 403 Forbidden\r\n\r\n") socket.destroy() return } - const { sessionId, setCookie } = ensureSession(req) - req.sessionSetCookie = setCookie - wss.handleUpgrade(req, socket, head, (ws) => { - ;(ws as AliveWebSocket).sessionId = sessionId wss.emit("connection", ws, req) }) }) @@ -282,8 +264,9 @@ wss.on("connection", (ws: AliveWebSocket) => { console.log("Client connected.") ws.isAlive = true ws.on("pong", () => {ws.isAlive = true}) - // This is a simplified cluster node map that stores clusterIds and their corresponding nodeIds - const connectedNodesByCluster: Map = new Map() + // The Valkey connections this socket opened, each with the owner identity it + // authenticated as. It also holds the per-owner cluster bookkeeping. + ws.binding = createSocketBinding() const handlers: Record = { [VALKEY.CONNECTION.connectPending]: connectPending, @@ -349,53 +332,25 @@ wss.on("connection", (ws: AliveWebSocket) => { try { const handler = handlers[action.type] ?? unknownHandler - // Enforce session ownership: reject actions targeting a connection this session doesn't own. - // Connection-establishing actions are exempt (authorization happens after successful connect). - const exempt = action.type === VALKEY.CONNECTION.connectPending - || action.type === VALKEY.TOPOLOGY.discoveryEndpointPending - - if (!exempt) { - const targetClusterId = action.payload?.clusterId - - if (connectionId && !isConnectionAuthorized(ws.sessionId, connectionId)) { - console.warn(`Rejected: session does not own connection ${connectionId}`) - return - } - - // clusterId only legitimate if it's the cluster of the connection the session proved it owns - if (targetClusterId) { - if (!connectionId) { - console.warn(`Rejected: session does not have connectionId for clusterId ${targetClusterId}`) - return - } - - let connectionIdClient = clients.get(connectionId) - if (!connectionIdClient?.clusterId && isNodeId(connectionId)) { // connectionId is a nodeId (which is not stored in clients) - for (const id of clients.keys()) { - if (toNodeId(id) !== connectionId) continue - const entry = clients.get(id) - if (entry?.clusterId) { - connectionIdClient = entry - break - } - } - } - - if (connectionIdClient?.clusterId !== targetClusterId) { - console.warn(`Rejected: connection ${connectionId} is not part of cluster ${targetClusterId}`) - return - } - } + // A socket may act only on connections it opened with credentials that + // Valkey accepted. Connects resolve to the owner named by their details. + const resolved = resolveAction(ws.binding, action) + if (!resolved.ok) { + console.warn(`Rejected ${action.type}: ${resolved.reason}`) + return } - + const { ownerKey, state } = resolved + await handler( { ws, - clients, + clients: state.clients, connectionId: connectionId!, - metricsServerMap, - connectedNodesByCluster, - clusterNodesRegistry, - sessionId: ws.sessionId })(action as ReduxAction) + metricsServerMap: state.metricsServerMap, + connectedNodesByCluster: connectedNodesFor(ws.binding, ownerKey), + clusterNodesRegistry: state.clusterNodesRegistry, + ownerKey, + ownerState: state, + binding: ws.binding })(action as ReduxAction) } catch (error) { console.error(`Error handling action ${action.type}:`, error) } @@ -405,29 +360,15 @@ wss.on("connection", (ws: AliveWebSocket) => { }) ws.on("close", (code, reason) => { abortConfigSessionsForSocket(ws) - const removedIds = unsubscribeAll(ws) + unsubscribeAll(ws) console.log("Client disconnected. Reason:", code, reason.toString()) - const scheduled = new Set(removedIds) - for (const connectionId of removedIds) { - setTimeout(() => { - if (getWatcherCount(connectionId) === 0 && !hasAuthorizedSession(connectionId)) { - teardownConnection( - { clients, clusterNodesRegistry, metricsServerMap }, - connectionId, - ) - } - }, CONNECTION_TEARDOWN_DELAY_MS) - } - - for (const [id] of clients) { - if (!scheduled.has(id) && getWatcherCount(id) === 0 && !hasAuthorizedSession(id)) { - teardownConnection( - { clients, clusterNodesRegistry, metricsServerMap }, - id, - ) - } + // Release every connection this socket bound. Each one is torn down after + // the teardown delay unless another socket of the same owner watches it. + for (const [connectionId, ownerKey] of ws.binding.connections) { + scheduleRelease(getOwnerState(ownerKey), connectionId) } + ws.binding = createSocketBinding() }) }) @@ -462,12 +403,14 @@ function shutdown() { } }) - // Step 3: Close all Valkey clients (releases connections on the Valkey side) - for (const [, { client }] of clients) { - try { - client.close() - } catch (err) { - console.error("Error closing Valkey client:", err) + // Step 3: Close every owner's Valkey clients (releases connections on the Valkey side) + for (const state of allOwnerStates()) { + for (const [, { client }] of state.clients) { + try { + client.close() + } catch (err) { + console.error("Error closing Valkey client:", err) + } } } } diff --git a/apps/server/src/metrics-orchestrator.ts b/apps/server/src/metrics-orchestrator.ts index 86d5d120..806ca7a1 100644 --- a/apps/server/src/metrics-orchestrator.ts +++ b/apps/server/src/metrics-orchestrator.ts @@ -14,6 +14,7 @@ */ import { GlideClient, GlideClusterClient, ConnectionError, ServiceType } from "@valkey/valkey-glide" import { ChildProcess, spawn, type SpawnOptions } from "child_process" +import fs from "node:fs" import { fileURLToPath } from "url" import { Router, type Request, type Response } from "express" import path from "path" @@ -35,6 +36,14 @@ import { import { discoverCluster, belongsToCluster } from "./connection" import { ConnectionDetails } from "./actions/connection" import { createOrchestratorValkeyClient } from "./valkey-client" +import { + allOwnerStates, + encodeOwnerDir, + getEnvOwnerState, + ownerOf, + registerEnvOwner, + type OwnerState +} from "./owner" // Assumes nodeId is unique among all clusters export type MetricsServerMap = Map; -export const clients: Map = new Map() +// The module-level maps are the env owner's state: the identity configured +// through VALKEY_* environment variables. The preconfigured and Kubernetes +// paths use them. User connections use their own owner's state. +const envState = getEnvOwnerState() + +export const clients = envState.clients -export const clusterNodesRegistry: Map = new Map() +export const clusterNodesRegistry = envState.clusterNodesRegistry -export const clusterCredentials: Map = new Map() +export const clusterCredentials = envState.clusterCredentials let preconfiguredClusterId: string | undefined export function setPreconfiguredClusterId(clusterId: string | undefined) { preconfiguredClusterId = clusterId } -export const metricsServerMap: MetricsServerMap = new Map() +export const metricsServerMap: MetricsServerMap = envState.metricsServerMap /** - * HMAC key material for spawned collectors, keyed by metrics-node-id. + * HMAC key material for the env owner's spawned collectors, keyed by metrics-node-id. + * Each owner state keeps its own `collectorKeys`. */ -const collectorKeys: Map = new Map() +const collectorKeys: Map = envState.collectorKeys /** * Resolve the only key material acceptable for `nodeId`. @@ -86,14 +101,14 @@ const collectorKeys: Map = new Map() * be accepted in its place, or a credential valid for one node could be * replayed against another. */ -export function resolveCollectorKey(nodeId: string): string | undefined { - const spawnedKey = collectorKeys.get(nodeId) +export function resolveCollectorKey(nodeId: string, state: OwnerState = envState): string | undefined { + const spawnedKey = state.collectorKeys.get(nodeId) if (spawnedKey) return spawnedKey // K8s collectors are external sidecars the orchestrator never spawns, so there // is no per-node minted key. They authenticate with a shared key provisioned to // both the orchestrator and every sidecar via a Kubernetes Secret. Scoped to K8s // so the per-node property is preserved for spawned (Web/Electron) collectors. - if (isKubernetes) return process.env[ORCHESTRATOR_AUTH_KEY_ENV] + if (isKubernetes && state === envState) return process.env[ORCHESTRATOR_AUTH_KEY_ENV] return undefined } @@ -102,8 +117,8 @@ export function resolveCollectorKey(nodeId: string): string | undefined { * removed so the two stay in lockstep and no secret outlives the process it * belonged to. */ -export function forgetCollectorKey(nodeId: string): void { - collectorKeys.delete(nodeId) +export function forgetCollectorKey(nodeId: string, state: OwnerState = envState): void { + state.collectorKeys.delete(nodeId) } export const isWebMode = process.env.DEPLOYMENT_MODE === DEPLOYMENT_TYPE.WEB @@ -136,6 +151,10 @@ export const initialConnectionDetails: ConnectionDetails = { db: Number(process.env.VALKEY_DB ?? 0), } +// A user who authenticates as the preconfigured identity reuses the env +// owner's clients and collectors. +if (preConfiguredConnection) registerEnvOwner(ownerOf(initialConnectionDetails)) + const ttl = Number(process.env.TTL) || 60000 // Reconciliation works on flat node ids, but cluster discovery stores replicas under their primary. @@ -257,6 +276,35 @@ function isKnownClusterNode(nodeId: string): boolean { return false } +/** + * Find the owner whose collector signed a request for `nodeId`. + * + * Several owners can run a collector on the same node, and each collector + * signs with its own key. The request names only the node, so the credential + * is checked against each key issued for it. Only the issuing collector's key + * verifies, so the match identifies the collector. + */ +function verifyCollector( + nodeId: string, + credential: string | undefined, + domain: (typeof ORCHESTRATOR_AUTH_DOMAIN)[keyof typeof ORCHESTRATOR_AUTH_DOMAIN], + fields: { nodeId: string; metricsServerUri?: string; timestamp: number }, +): { ok: true; state: OwnerState } | { ok: false; reason: string; skewMs?: number } { + const windowMs = resolveOrchestratorAuthWindowMs(process.env[ORCHESTRATOR_AUTH_WINDOW_ENV]) + const candidates = allOwnerStates().filter((state) => resolveCollectorKey(nodeId, state) !== undefined) + let failure: { ok: false; reason: string; skewMs?: number } | undefined + for (const state of candidates) { + const verification = verifyOrchestratorAuthCredential({ + key: resolveCollectorKey(nodeId, state), credential, domain, fields, windowMs, + }) + if (verification.ok) return { ok: true, state } + failure ??= verification + } + if (failure) return failure + const verification = verifyOrchestratorAuthCredential({ key: undefined, credential, domain, fields, windowMs }) + return verification.ok ? { ok: false, reason: "missing_key" } : verification +} + /** * `POST /orchestrator/register` — a collector advertising where it can be * reached. @@ -274,13 +322,12 @@ function handleRegister(req: Request, res: Response): void { return } - const verification = verifyOrchestratorAuthCredential({ - key: resolveCollectorKey(nodeId), - credential: readOrchestratorAuthHeader(req), - domain: ORCHESTRATOR_AUTH_DOMAIN.REGISTER, - fields: { nodeId, metricsServerUri, timestamp }, - windowMs: resolveOrchestratorAuthWindowMs(process.env[ORCHESTRATOR_AUTH_WINDOW_ENV]), - }) + const verification = verifyCollector( + nodeId, + readOrchestratorAuthHeader(req), + ORCHESTRATOR_AUTH_DOMAIN.REGISTER, + { nodeId, metricsServerUri, timestamp }, + ) if (!verification.ok) { const skew = verification.skewMs === undefined ? "" : ` (clock skew ${verification.skewMs}ms)` @@ -304,17 +351,19 @@ function handleRegister(req: Request, res: Response): void { // Allowed if we already have an entry (orchestrator-spawned) or, in K8s, // the node is part of the discovered cluster (sidecars are external, so // they create their entry on first register). - const allowed = metricsServerMap.has(nodeId) || (isKubernetes && isKnownClusterNode(nodeId)) + const ownerMap = verification.state.metricsServerMap + const isSidecar = isKubernetes && verification.state === envState && isKnownClusterNode(nodeId) + const allowed = ownerMap.has(nodeId) || isSidecar if (!allowed) { console.warn(`Rejected metrics registration for ${nodeId}: no metrics server entry`) res.status(401).send("Unauthorized") return } - let entry = metricsServerMap.get(nodeId) + let entry = ownerMap.get(nodeId) if (!entry) { entry = { metricsURI: "", pid: undefined, lastSeen: Date.now() } - metricsServerMap.set(nodeId, entry) + ownerMap.set(nodeId, entry) } entry.metricsURI = metricsServerUri @@ -337,13 +386,12 @@ function handlePing(req: Request, res: Response): void { return } - const verification = verifyOrchestratorAuthCredential({ - key: resolveCollectorKey(nodeId), - credential: readOrchestratorAuthHeader(req), - domain: ORCHESTRATOR_AUTH_DOMAIN.PING, - fields: { nodeId, timestamp }, - windowMs: resolveOrchestratorAuthWindowMs(process.env[ORCHESTRATOR_AUTH_WINDOW_ENV]), - }) + const verification = verifyCollector( + nodeId, + readOrchestratorAuthHeader(req), + ORCHESTRATOR_AUTH_DOMAIN.PING, + { nodeId, timestamp }, + ) if (!verification.ok) { const skew = verification.skewMs === undefined ? "" : ` (clock skew ${verification.skewMs}ms)` @@ -352,7 +400,7 @@ function handlePing(req: Request, res: Response): void { return } - const entry = metricsServerMap.get(nodeId) + const entry = verification.state.metricsServerMap.get(nodeId) if (!entry) { console.warn(`Rejected metrics ping for ${nodeId}: no metrics server entry`) res.status(401).send("Unauthorized") @@ -416,6 +464,7 @@ export async function updateClusterNodeRegistry( client: GlideClusterClient | GlideClient, nodeInfo: NodeInfo, clusterId?: string, + registry: Map = clusterNodesRegistry, ) { try { const { discoveredClusterNodes, clusterId: discoveredClusterId } = await discoverCluster(client, { connectionDetails: nodeInfo }) @@ -424,7 +473,7 @@ export async function updateClusterNodeRegistry( // change on failover/resharding and would otherwise orphan the old entry. const key = clusterId ?? discoveredClusterId if (key && discoveredClusterNodes) { - clusterNodesRegistry.set(key, discoveredClusterNodes) + registry.set(key, discoveredClusterNodes) return key } } @@ -449,17 +498,22 @@ export async function resolveClusterRefreshTarget( clusterId: string, clusterNodes: ClusterNodeMap, userClient: GlideClusterClient | GlideClient | undefined, + registry: Map = clusterNodesRegistry, ): Promise<{ client: GlideClusterClient | GlideClient; nodeInfo: NodeInfo } | undefined> { if (userClient) return { client: userClient, nodeInfo: Object.values(clusterNodes)[0] } - // Only the preconfigured cluster may be refreshed via the initial client. - if (preConfiguredConnection && clusterId === preconfiguredClusterId) { + // Only the env owner's preconfigured cluster may be refreshed via the initial client. + if (preConfiguredConnection && clusterId === preconfiguredClusterId && registry === clusterNodesRegistry) { return { client: await internals.getInitialClient(), nodeInfo: initialConnectionDetails } } return undefined } -async function findDiff(metricsServerMap: MetricsServerMap, clusterNodeMap: ClusterNodeMap) { +async function findDiff( + metricsServerMap: MetricsServerMap, + clusterNodeMap: ClusterNodeMap, + ownerClients: OwnerState["clients"] = clients, +) { const clusterNodes = isKubernetes ? flattenClusterNodeMap(clusterNodeMap) : clusterNodeMap // These are nodes that are in the clusterMap but not metricsMap // TODO: Could use R.pickBy instead @@ -475,7 +529,7 @@ async function findDiff(metricsServerMap: MetricsServerMap, clusterNodeMap: Clus // Treat the metrics process as still-claimed if any open client strips // down to this node (N:1). Avoids evicting standalone metrics whose // owner connection is keyed `-db`. - const knownToClients = [...clients.keys()].some( + const knownToClients = [...ownerClients.keys()].some( (id) => toNodeId(id) === key, ) return (!clusterNodes[key] && !knownToClients) || (now - value.lastSeen) > ttl @@ -485,45 +539,87 @@ async function findDiff(metricsServerMap: MetricsServerMap, clusterNodeMap: Clus return { nodesToAdd, nodesToRemove } } -async function updateMetricsServers(nodesToAdd: ClusterNodeMap, nodesToRemove: string[], clusterId: string) { - await startMetricsServers(nodesToAdd, clusterId) - await stopMetricsServers(nodesToRemove) +async function updateMetricsServers( + nodesToAdd: ClusterNodeMap, + nodesToRemove: string[], + clusterId: string, + state: OwnerState = envState, +) { + await internals.startMetricsServers(nodesToAdd, clusterId, state) + await internals.stopMetricsServers(nodesToRemove, state) } -async function startMetricsServers(nodesToStart: ClusterNodeMap, clusterId: string) { - const password = clusterCredentials.get(clusterId) +async function startMetricsServers(nodesToStart: ClusterNodeMap, clusterId: string, state: OwnerState = envState) { + const password = state.clusterCredentials.get(clusterId) await Promise.all( Object.entries(nodesToStart).map(async ([nodeId, nodeInfo]) => { - if (!metricsServerMap.has(nodeId)) { - await startMetricsServer({ ...nodeInfo, password }, nodeId) + if (!state.metricsServerMap.has(nodeId)) { + await startMetricsServer({ ...nodeInfo, password }, nodeId, state) } }), ) } -async function stopMetricsServers(nodesToStop: string[]) { +async function stopMetricsServers(nodesToStop: string[], state: OwnerState = envState) { await Promise.all( nodesToStop.map(async (node) => { - if (metricsServerMap.has(node)) { - await stopMetricsServer(node) - } + if (state.metricsServerMap.has(node)) { + await internals.stopMetricsServer(node, state) + } }), ) } -export async function stopAllMetricsServers(metricsMap: MetricsServerMap) { - if (!isKubernetes) { - metricsMap.forEach((metricsServer, nodeId) => { - try { - if (metricsServer.pid) - process.kill(metricsServer.pid) - } catch (e) { - console.warn(`Failed to kill metrics server ${nodeId}:`, e) - } - }) +/** Stop every owner's collectors. Used on graceful shutdown. */ +export function stopAllMetricsServers() { + for (const state of allOwnerStates()) { + if (!isKubernetes) { + state.metricsServerMap.forEach((metricsServer, nodeId) => { + try { + if (metricsServer.pid) + process.kill(metricsServer.pid) + } catch (e) { + console.warn(`Failed to kill metrics server ${nodeId}:`, e) + } + }) + } + state.metricsServerMap.clear() + state.collectorKeys.clear() + } +} + +/** + * Stop the collectors of `state` that no remaining connection of that owner + * uses: a node is still used when one of the owner's clients is on it, or it + * is in one of the owner's remaining clusters. + */ +export async function pruneOwnerCollectors(state: OwnerState) { + const usedNodes = new Set([...state.clients.keys()].map(toNodeId)) + for (const clusterNodes of state.clusterNodesRegistry.values()) { + Object.keys(clusterNodes).forEach((nodeId) => usedNodes.add(nodeId)) } - metricsMap.clear() - collectorKeys.clear() + const unused = [...state.metricsServerMap.keys()].filter((nodeId) => !usedNodes.has(nodeId)) + await stopMetricsServers(unused, state) +} + +const defaultDataDir = () => + process.env.DATA_DIR ?? path.join(path.dirname(fileURLToPath(import.meta.url)), "..", "data") + +/** + * The owner's own copy of the collector settings file. It is seeded once from + * the base file, so a settings change from one owner never reaches another. + */ +function ownerConfigPath(ownerDir: string, baseConfigPath: string): string { + const configPath = path.join(ownerDir, "config.yml") + try { + fs.mkdirSync(ownerDir, { recursive: true }) + if (!fs.existsSync(configPath)) fs.copyFileSync(baseConfigPath, configPath, fs.constants.COPYFILE_EXCL) + } catch (err) { + if ((err as NodeJS.ErrnoException).code !== "EEXIST") { + console.error(`Could not create the collector settings file ${configPath}:`, err) + } + } + return configPath } /** @@ -534,25 +630,27 @@ function spawnProcess(command: string, args: string[], options: SpawnOptions): C return spawn(command, args, options) } -export async function startMetricsServer(nodeToStart: NodeInfo, nodeId: string) { +export async function startMetricsServer(nodeToStart: NodeInfo, nodeId: string, state: OwnerState = envState) { const processResourcesPath = process.env.PROCESS_RESOURCES_PATH ?? "" const metricsServerPath = isElectron ? path.join(processResourcesPath, "server-metrics.js") : fileURLToPath(new URL("../../metrics/dist/index.cjs", import.meta.url)) - const configPath = process.env.CONFIG_PATH + const baseConfigPath = process.env.CONFIG_PATH ?? (isElectron ? path.join(processResourcesPath, "config.yml") : fileURLToPath(new URL("../../metrics/config.yml", import.meta.url))) - const data_dir = process.env.DATA_DIR ?? path.join(path.dirname(fileURLToPath(import.meta.url)), "..", "data") + // Each owner keeps its own metrics history and collector settings. + const ownerDir = path.join(defaultDataDir(), encodeOwnerDir(state.ownerKey)) + const configPath = ownerConfigPath(ownerDir, baseConfigPath) console.log("Starting metrics server for: ", nodeId) // Minted before the spawn so the key is already resolvable when the child // races ahead and registers. const collectorKey = generateOrchestratorAuthKey() - collectorKeys.set(nodeId, collectorKey) + state.collectorKeys.set(nodeId, collectorKey) const proc: ChildProcess = internals.spawnProcess(process.execPath, [metricsServerPath], { env: { @@ -571,7 +669,7 @@ export async function startMetricsServer(nodeToStart: NodeInfo, nodeId: string) SERVER_HOST: process.env.SERVER_HOST ?? "localhost", SERVER_PORT: process.env.SERVER_PORT ?? "8080", METRICS_BIND_HOST: process.env.METRICS_BIND_HOST ?? (isKubernetes ? "0.0.0.0" : "127.0.0.1"), - DATA_DIR: `${data_dir}/${nodeId}`, + DATA_DIR: path.join(ownerDir, nodeId), CONFIG_PATH: configPath, // MUST stay after the `...process.env` spread. The orchestrator may // itself carry an ORCHESTRATOR_KEY (the shared key used by externally @@ -600,7 +698,7 @@ export async function startMetricsServer(nodeToStart: NodeInfo, nodeId: string) }) // Don't need to set metricsURI here since we need to wait for server to register itself - metricsServerMap.set(nodeId, + state.metricsServerMap.set(nodeId, { metricsURI: "", pid: proc.pid, @@ -609,19 +707,19 @@ export async function startMetricsServer(nodeToStart: NodeInfo, nodeId: string) ) } -async function stopMetricsServer(nodeToStop: string) { +async function stopMetricsServer(nodeToStop: string, state: OwnerState = envState) { try { console.log("Killing metrics server for ", nodeToStop) - const entry = metricsServerMap.get(nodeToStop) + const entry = state.metricsServerMap.get(nodeToStop) if (isKubernetes) { - metricsServerMap.delete(nodeToStop) - forgetCollectorKey(nodeToStop) + state.metricsServerMap.delete(nodeToStop) + forgetCollectorKey(nodeToStop, state) return } if (entry?.pid) { try { process?.kill(entry.pid, "SIGTERM") } catch { /* already dead */ } - metricsServerMap.delete(nodeToStop) - forgetCollectorKey(nodeToStop) + state.metricsServerMap.delete(nodeToStop) + forgetCollectorKey(nodeToStop, state) } } catch (e) { @@ -629,22 +727,24 @@ async function stopMetricsServer(nodeToStop: string) { } } -export async function reconcileClusterMetricsServers( - metricsServerMap: MetricsServerMap, -) { - const clusterIds = [...clusterNodesRegistry.keys()] +export async function reconcileClusterMetricsServers(state: OwnerState = envState) { + const clusterIds = [...state.clusterNodesRegistry.keys()] if (clusterIds.length === 0) return await Promise.all( clusterIds.map(async (clusterId) => { try { - const { nodesToAdd, nodesToRemove } = await internals.findDiff(metricsServerMap, clusterNodesRegistry.get(clusterId) ?? {}) + const { nodesToAdd, nodesToRemove } = await internals.findDiff( + state.metricsServerMap, + state.clusterNodesRegistry.get(clusterId) ?? {}, + state.clients, + ) // Early return if nothing has changed if (Object.keys(nodesToAdd).length === 0 && nodesToRemove.length === 0) { console.debug("Cluster nodes and metrics servers are in sync") return } - await internals.updateMetricsServers(nodesToAdd, nodesToRemove, clusterId) + await internals.updateMetricsServers(nodesToAdd, nodesToRemove, clusterId, state) } catch (err) { console.error(`Failed to reconcile metrics servers for cluster ${clusterId}:`, err) } @@ -678,7 +778,7 @@ export async function runReconcileLoop() { const delay = (ms: number) => new Promise((res) => setTimeout(res, ms)) while (true) { try { - await reconcileClusterMetricsServers(metricsServerMap) + await Promise.all(allOwnerStates().map((state) => reconcileClusterMetricsServers(state))) await delay(10000) } catch (err) { console.error("Failed to reconcile metrics servers", err) @@ -689,7 +789,7 @@ export async function runReconcileLoop() { export function cleanupOrchestratorResources() { initialClient?.close() - stopAllMetricsServers(metricsServerMap) + stopAllMetricsServers() } // To help mock internal methods in tests diff --git a/apps/server/src/node-utilization.ts b/apps/server/src/node-utilization.ts index be91b7ca..a8c67841 100644 --- a/apps/server/src/node-utilization.ts +++ b/apps/server/src/node-utilization.ts @@ -48,7 +48,8 @@ const CPU_SECTION = "CPU" const cpuSamples: Map = new Map() -// Drop retained samples so a reconnecting node re-baselines. +// Drop retained samples so a reconnecting node re-baselines. Keys are +// `|` when the caller scoped them by owner identity. export const clearCpuSamples = (nodeId?: string): void => { if (nodeId === undefined) cpuSamples.clear() else cpuSamples.delete(nodeId) @@ -175,10 +176,14 @@ export const computeNodeUtilization = ( nodeId: string, sections: InfoSections, now: number = Date.now(), + scope?: string, ): NodeUtilization => { + // Baselines are kept per owner identity, so two users polling the same node + // never share CPU samples. + const sampleKey = scope === undefined ? nodeId : `${scope}|${nodeId}` const memory = calculateMemoryUtilization(sections[MEMORY_SECTION] ?? {}) const nextSample = readCpuSample(sections[CPU_SECTION] ?? {}, now) - const previousSample = cpuSamples.get(nodeId) ?? null + const previousSample = cpuSamples.get(sampleKey) ?? null const cpu = nextSample === null @@ -191,7 +196,7 @@ export const computeNodeUtilization = ( nextSample !== null && nextSample.ts - previousSample.ts < MIN_CPU_SAMPLE_MS - if (nextSample !== null && !isTooSoon) cpuSamples.set(nodeId, nextSample) + if (nextSample !== null && !isTooSoon) cpuSamples.set(sampleKey, nextSample) return { nodeId, @@ -209,8 +214,9 @@ export const computeNodeUtilization = ( export const computeClusterUtilization = ( clusterInfo: ParsedClusterInfo, now: number = Date.now(), + scope?: string, ): Record => R.mapObjIndexed( - (sections, nodeId) => computeNodeUtilization(nodeId, sections, now), + (sections, nodeId) => computeNodeUtilization(nodeId, sections, now, scope), clusterInfo, ) diff --git a/apps/server/src/node-watchers.ts b/apps/server/src/node-watchers.ts index 49ee4b3f..0cd10048 100644 --- a/apps/server/src/node-watchers.ts +++ b/apps/server/src/node-watchers.ts @@ -1,5 +1,11 @@ import type WebSocket from "ws" +/** + * Watcher ids are scoped by owner identity, so sockets of different Valkey + * users on the same connectionId never see each other's updates. + */ +export const scopedId = (ownerKey: string, id: string): string => `${ownerKey}|${id}` + // connectionId → Set of WebSocket clients watching this node const nodeWatchers: Map> = new Map() diff --git a/apps/server/src/owner.ts b/apps/server/src/owner.ts index a6eae744..2bec210c 100644 --- a/apps/server/src/owner.ts +++ b/apps/server/src/owner.ts @@ -1,4 +1,7 @@ +import crypto from "node:crypto" +import type { GlideClient, GlideClusterClient } from "@valkey/valkey-glide" import type { ConnectionDetails } from "./actions/connection" +import type { ClusterNodeMap, MetricsServerMap } from "./metrics-orchestrator" /** * Owner identity: the Valkey identity a connection authenticated as. @@ -14,7 +17,8 @@ import type { ConnectionDetails } from "./actions/connection" * - ElastiCache IAM: `iam~~~`. * - GCP IAM: `gcp-iam~default` (the only user GCP IAM supports). */ -export const ownerOf = (details: ConnectionDetails): string => { +export const ownerOf = (details: Pick): string => { if (details.authType === "iam") { return ["iam", details.awsRegion ?? "", details.awsReplicationGroupId ?? "", details.username ?? ""].join("~") } @@ -22,3 +26,89 @@ export const ownerOf = (details: ConnectionDetails): string => { const username = details.password && details.username ? details.username : "default" return `password~${username}` } + +const SAFE_BYTE = /^[a-z0-9_~-]$/ +const MAX_OWNER_DIR_BYTES = 200 +const TRUNCATED_PREFIX_BYTES = 180 + +/** + * Encode an owner key as one directory name. Lowercase letters, digits, `_`, + * `-` and `~` stay as they are. Every other UTF-8 byte, uppercase included, + * becomes `%XX`, so names stay distinct on case-insensitive filesystems and + * stay readable. Names longer than 200 bytes are cut and get a hash suffix + * (`~h<16 hex>`), which is not reversible. + */ +export const encodeOwnerDir = (ownerKey: string): string => { + const encoded = [...Buffer.from(ownerKey, "utf8")] + .map((byte) => { + const char = String.fromCharCode(byte) + return SAFE_BYTE.test(char) ? char : `%${byte.toString(16).toUpperCase().padStart(2, "0")}` + }) + .join("") + if (encoded.length <= MAX_OWNER_DIR_BYTES) return encoded + const hash = crypto.createHash("sha256").update(ownerKey, "utf8").digest("hex").slice(0, 16) + return `${encoded.slice(0, TRUNCATED_PREFIX_BYTES)}~h${hash}` +} + +export const decodeOwnerDir = (dir: string): string => decodeURIComponent(dir) + +type ClientEntry = { client: GlideClient | GlideClusterClient; clusterId?: string } + +/** All server state that belongs to one owner identity. */ +export type OwnerState = { + ownerKey: string + /** True for the identity configured through `VALKEY_*` environment variables. */ + env: boolean + clients: Map + clusterNodesRegistry: Map + clusterCredentials: Map + metricsServerMap: MetricsServerMap + /** HMAC key material for this owner's spawned collectors, keyed by metrics-node-id. */ + collectorKeys: Map + inFlightClusterClients: Map> + connectInFlight: Map> +} + +export const createOwnerState = (ownerKey: string, env = false): OwnerState => ({ + ownerKey, + env, + clients: new Map(), + clusterNodesRegistry: new Map(), + clusterCredentials: new Map(), + metricsServerMap: new Map(), + collectorKeys: new Map(), + inFlightClusterClients: new Map(), + connectInFlight: new Map(), +}) + +/** + * The env owner's state. It also backs the module-level maps exported by + * `metrics-orchestrator.ts`, which the preconfigured and Kubernetes paths use. + * A user reaches it only when they authenticate as the preconfigured identity. + */ +const envState = createOwnerState("env", true) +const owners = new Map() + +export const getEnvOwnerState = (): OwnerState => envState + +/** Make the preconfigured identity resolve to the env owner's state. */ +export const registerEnvOwner = (ownerKey: string): void => { + owners.delete(envState.ownerKey) + envState.ownerKey = ownerKey + owners.set(ownerKey, envState) +} + +export const getOwnerState = (ownerKey: string): OwnerState => { + let state = owners.get(ownerKey) + if (!state) { + state = createOwnerState(ownerKey) + owners.set(ownerKey, state) + } + return state +} + +/** Every owner state, the env owner's included. */ +export const allOwnerStates = (): OwnerState[] => { + const states = [...owners.values()] + return states.includes(envState) ? states : [envState, ...states] +} diff --git a/apps/server/src/session.ts b/apps/server/src/session.ts deleted file mode 100644 index cd131888..00000000 --- a/apps/server/src/session.ts +++ /dev/null @@ -1,114 +0,0 @@ -import crypto from "crypto" -import type { IncomingMessage } from "http" - -const SESSION_COOKIE_NAME = "vk_sid" -const SESSION_TTL_MS = 24 * 60 * 60 * 1000 -const SWEEP_INTERVAL_MS = 60 * 1000 - -type Session = { - connectionIds: Set - expireAt: number -} - -// this listener is called when a session expires and has connections that are no longer authorized -type SessionExpiryListener = (connectionIds: string[]) => void - -const sessions = new Map() -let onSessionExpiry: SessionExpiryListener | undefined - -export const setSessionExpiryListener = (listener: SessionExpiryListener): void => { - onSessionExpiry = listener -} - -const isExpired = (session: Session) => session.expireAt <= Date.now() - -const parseSessionId = (req: IncomingMessage): string | undefined => { - for (const part of req.headers.cookie?.split(";") ?? []) { - const [name, ...value] = part.trim().split("=") - if (name === SESSION_COOKIE_NAME) return value.join("=") - } - return undefined -} - -// Expire the session and notify the listener of any connections that are no longer authorized -const expireSession = (sessionId: string, session: Session): void => { - sessions.delete(sessionId) - const orphaned = [...session.connectionIds].filter((id) => !hasAuthorizedSession(id)) - if (orphaned.length > 0) onSessionExpiry?.(orphaned) -} - -// Get the session if it exists and is not expired, otherwise return undefined -const getSession = (sessionId: string | undefined): Session | undefined => { - if (!sessionId) return undefined - const session = sessions.get(sessionId) - if (!session) return undefined - if (isExpired(session)) { - expireSession(sessionId, session) - return undefined - } - return session -} - -// True over HTTPS (directly or behind a proxy), so we only add the Secure cookie flag then. -const isSecureRequest = (req: IncomingMessage): boolean => { - const forwarded = req.headers["x-forwarded-proto"] - const proto = Array.isArray(forwarded) ? forwarded[0] : forwarded?.split(",")[0]?.trim() - if (proto) return proto === "https" - return Boolean((req.socket as { encrypted?: boolean }).encrypted) -} - -const buildSetCookie = (sessionId: string, req: IncomingMessage): string => { - const attributes = [ - `${SESSION_COOKIE_NAME}=${sessionId}`, - "HttpOnly", - "SameSite=Strict", - "Path=/", - `Max-Age=${Math.floor(SESSION_TTL_MS / 1000)}`, - ] - if (isSecureRequest(req)) attributes.push("Secure") - return attributes.join("; ") -} - -// create a new session or refresh an existing one, returning the session ID and the Set-Cookie header value -export const ensureSession = (req: IncomingMessage): { sessionId: string; setCookie: string } => { - const existingId = parseSessionId(req) - const existing = getSession(existingId) - const sessionId = existing && existingId ? existingId : crypto.randomBytes(32).toString("hex") - - const session = existing ?? { connectionIds: new Set(), expireAt: 0 } - session.expireAt = Date.now() + SESSION_TTL_MS - sessions.set(sessionId, session) - - return { sessionId, setCookie: buildSetCookie(sessionId, req) } -} - -export const authorizeConnection = (sessionId: string | undefined, connectionId: string): void => { - getSession(sessionId)?.connectionIds.add(connectionId) -} - -export const isConnectionAuthorized = (sessionId: string | undefined, connectionId: string): boolean => - getSession(sessionId)?.connectionIds.has(connectionId) ?? false - -export const revokeConnection = (sessionId: string | undefined, connectionId: string): void => { - getSession(sessionId)?.connectionIds.delete(connectionId) -} - -// check if a connection is authorized by any session, not just the one provided -export const hasAuthorizedSession = (connectionId: string): boolean => { - for (const session of sessions.values()) { - if (!isExpired(session) && session.connectionIds.has(connectionId)) return true - } - return false -} - -// sweep expired sessions every minute, notifying the listener of any connections that are no longer authorized -setInterval(() => { - for (const [id, session] of sessions) { - if (isExpired(session)) expireSession(id, session) - } -}, SWEEP_INTERVAL_MS).unref() - -export const _resetSessions = (): void => { - sessions.clear() - onSessionExpiry = undefined -} diff --git a/apps/server/src/set-dashboard-data.ts b/apps/server/src/set-dashboard-data.ts index 9d1aa462..9855d326 100644 --- a/apps/server/src/set-dashboard-data.ts +++ b/apps/server/src/set-dashboard-data.ts @@ -9,10 +9,10 @@ import { discoverCluster } from "./connection" import { type ConnectionDetails } from "./actions/connection" import { isWebMode, - metricsServerMap, reconcileClusterMetricsServers, type ClusterNodeMap } from "./metrics-orchestrator" +import type { OwnerState } from "./owner" type DashboardInfo = { info: Record @@ -84,9 +84,10 @@ export async function setDashboardData( // Never let utilization math break the dashboard payload. const safeComputeClusterUtilization = ( clusterInfo: ParsedClusterInfo, + scope: string, ): Record => { try { - return computeClusterUtilization(clusterInfo) + return computeClusterUtilization(clusterInfo, Date.now(), scope) } catch (error) { console.error("Unable to compute cluster utilization:", error) return {} @@ -116,8 +117,9 @@ type ClusterNodesRefresh = { const refreshClusterNodes = async ( clusterId: string, client: GlideClusterClient, - clusterNodesRegistry: Map, + state: OwnerState, ): Promise => { + const { clusterNodesRegistry } = state const current = clusterNodesRegistry.get(clusterId) const template = current && Object.values(current)[0] if (!template) return { clusterNodes: undefined, topologyError: "No stored topology for this cluster" } @@ -129,7 +131,7 @@ const refreshClusterNodes = async ( if (!R.equals(discoveredClusterNodes, current)) { clusterNodesRegistry.set(clusterId, discoveredClusterNodes) if (isWebMode) { - reconcileClusterMetricsServers(metricsServerMap).catch((err) => + reconcileClusterMetricsServers(state).catch((err) => console.error(`Metrics server reconcile failed for cluster ${clusterId}:`, err), ) } @@ -149,12 +151,12 @@ export async function setClusterDashboardData( client: GlideClusterClient, ws: WebSocket, connectionId: string, - clusterNodesRegistry: Map, + state: OwnerState, ) { try { const [rawInfo, { clusterNodes, topologyError }] = await Promise.all([ client.info(), - refreshClusterNodes(clusterId, client, clusterNodesRegistry), + refreshClusterNodes(clusterId, client, state), ]) const clusterInfo = parseClusterInfo(rawInfo) @@ -164,7 +166,7 @@ export async function setClusterDashboardData( payload: { clusterId, info: clusterInfo, - utilization: safeComputeClusterUtilization(clusterInfo), + utilization: safeComputeClusterUtilization(clusterInfo, state.ownerKey), clusterNodes, topologyError, }, diff --git a/apps/server/src/socket-binding.ts b/apps/server/src/socket-binding.ts new file mode 100644 index 00000000..4936991d --- /dev/null +++ b/apps/server/src/socket-binding.ts @@ -0,0 +1,120 @@ +import { VALKEY, toNodeId } from "valkey-common" +import { CONNECTION_TEARDOWN_DELAY_MS } from "valkey-common" +import { getOwnerState, ownerOf, type OwnerState } from "./owner" +import { getWatcherCount, scopedId } from "./node-watchers" +import { teardownConnection } from "./connection" +import type { ConnectionDetails } from "./actions/connection" + +/** + * What one browser WebSocket may act on. + * + * A socket may act only on Valkey connections it opened itself with + * credentials that Valkey accepted. Each bound connection records the owner + * identity it authenticated as, and every action resolves to that owner's + * state. This replaces the old cookie sessions. + */ +export type SocketBinding = { + /** connectionId → owner key */ + connections: Map + /** db-less node id → owner key, for actions that name a node */ + nodes: Map + /** owner key → (clusterId → connectionIds), per-socket cluster bookkeeping */ + clustersByOwner: Map> +} + +export const createSocketBinding = (): SocketBinding => ({ + connections: new Map(), + nodes: new Map(), + clustersByOwner: new Map(), +}) + +/** Bind a connection to the socket. Returns the owner it was bound to before, if that was another owner. */ +export const bindConnection = (binding: SocketBinding, ownerKey: string, connectionId: string): string | undefined => { + const previous = binding.connections.get(connectionId) + binding.connections.set(connectionId, ownerKey) + binding.nodes.set(toNodeId(connectionId), ownerKey) + return previous !== undefined && previous !== ownerKey ? previous : undefined +} + +export const unbindConnection = (binding: SocketBinding, connectionId: string): void => { + binding.connections.delete(connectionId) + const nodeId = toNodeId(connectionId) + const nodeStillBound = [...binding.connections.keys()].some((id) => toNodeId(id) === nodeId) + if (!nodeStillBound) binding.nodes.delete(nodeId) +} + +export const connectedNodesFor = (binding: SocketBinding, ownerKey: string): Map => { + let clusters = binding.clustersByOwner.get(ownerKey) + if (!clusters) { + clusters = new Map() + binding.clustersByOwner.set(ownerKey, clusters) + } + return clusters +} + +type ActionLike = { type: string; payload?: { connectionId?: string; clusterId?: unknown; connectionDetails?: unknown } } + +export type ResolvedAction = + | { ok: true; ownerKey: string; state: OwnerState } + | { ok: false; reason: string } + +// The cluster that a bound connection (or a db-less node id) belongs to in its owner's state. +const clusterOf = (state: OwnerState, connectionId: string): string | undefined => { + const direct = state.clients.get(connectionId)?.clusterId + if (direct) return direct + for (const [id, entry] of state.clients) { + if (toNodeId(id) === connectionId && entry.clusterId) return entry.clusterId + } + return undefined +} + +/** + * Decide which owner's state an action runs against, or reject it. + * + * - A connect is resolved to the owner named by its connection details. It is + * bound to the socket only after Valkey accepts the credentials. + * - Any other action must name a connectionId (or db-less node id) bound on + * this socket. A clusterId is accepted only if it is the cluster of that + * connection in the owner's state. An empty clusterId counts as absent. + */ +export const resolveAction = ( + binding: SocketBinding, + action: ActionLike, + lookup: (ownerKey: string) => OwnerState | undefined = getOwnerState, +): ResolvedAction => { + const payload = action.payload ?? {} + + if (action.type === VALKEY.CONNECTION.connectPending || action.type === VALKEY.TOPOLOGY.discoveryEndpointPending) { + const details = payload.connectionDetails as ConnectionDetails | undefined + if (!details || typeof details !== "object") return { ok: false, reason: "missing connection details" } + const ownerKey = ownerOf(details) + return { ok: true, ownerKey, state: getOwnerState(ownerKey) } + } + + const { connectionId } = payload + if (typeof connectionId !== "string" || connectionId.length === 0) { + return { ok: false, reason: "missing connectionId" } + } + const ownerKey = binding.connections.get(connectionId) ?? binding.nodes.get(connectionId) + if (ownerKey === undefined) return { ok: false, reason: `connection ${connectionId} is not bound to this socket` } + const state = lookup(ownerKey) + if (!state) return { ok: false, reason: `no state for the owner of ${connectionId}` } + + const { clusterId } = payload + if (clusterId && clusterOf(state, connectionId) !== clusterId) { + return { ok: false, reason: `connection ${connectionId} is not part of cluster ${String(clusterId)}` } + } + return { ok: true, ownerKey, state } +} + +/** + * Tear down an owner's connection after the teardown delay, unless a socket + * watches it again in that time. + */ +export const scheduleRelease = (state: OwnerState, connectionId: string): void => { + setTimeout(() => { + if (getWatcherCount(scopedId(state.ownerKey, connectionId)) === 0) { + teardownConnection(state, connectionId) + } + }, CONNECTION_TEARDOWN_DELAY_MS) +} diff --git a/apps/server/src/utils.ts b/apps/server/src/utils.ts index 77f824a6..f7fc50ad 100644 --- a/apps/server/src/utils.ts +++ b/apps/server/src/utils.ts @@ -9,7 +9,6 @@ import * as R from "ramda" import { lookup, reverse } from "node:dns/promises" import { KEY_EVICTION_POLICY, KeyEvictionPolicy, sanitizeUrl } from "valkey-common" import { ClusterNodeMap } from "./metrics-orchestrator" -import { isConnectionAuthorized } from "./session" export const dns = { lookup, reverse, @@ -158,20 +157,14 @@ function isClusterClientEntry( export async function getExistingClusterClient( discoveredClusterNodes: ClusterNodeMap, clients: Map, - sessionId: string, ) { // Check if we've already connected to this cluster before const existingKey = Object.keys(discoveredClusterNodes).find( (key) => isClusterClientEntry(clients.get(key)), ) - // Only reuse the shared cluster client if the calling session owns the matched - // sibling node (e.g. the node it connected to earlier). Otherwise a different - // session could inherit this cluster's authenticated client without authenticating. - if (existingKey && !isConnectionAuthorized(sessionId, existingKey)) { - return undefined - } - + // `clients` holds only the connecting owner's clients, and the caller has + // already authenticated a discovery client with the presented credentials. const existingConnection = existingKey ? clients.get(existingKey) as { client: GlideClusterClient; clusterId?: string } : undefined diff --git a/apps/server/src/valkey-client.ts b/apps/server/src/valkey-client.ts index 2fa693d1..9bcb8b7d 100644 --- a/apps/server/src/valkey-client.ts +++ b/apps/server/src/valkey-client.ts @@ -109,3 +109,18 @@ export const createOrchestratorValkeyClient = ({ clientName: "valkey_admin_orchestrator_client", nodeDiscoveryMode: NodeDiscoveryMode.Static, }) + +/** + * Short-lived client that only proves credentials. It uses Glide's default + * node discovery, because with `NodeDiscoveryMode.Static` `createClient` does + * not reject a wrong password or a disabled user: it returns a client and + * keeps retrying in the background. Here `createClient` rejects with + * WRONGPASS / NOAUTH when Valkey refuses the credentials. + */ +export const createAuthProbeClient = ({ + ...options +}: ClientOptions) => + GlideClient.createClient({ + ...buildSharedOptions({ ...options, databaseId: undefined }), + clientName: "valkey_admin_auth_probe", + }) diff --git a/docker/docker-compose.test.yml b/docker/docker-compose.test.yml index 69b34587..171647a1 100644 --- a/docker/docker-compose.test.yml +++ b/docker/docker-compose.test.yml @@ -24,13 +24,42 @@ services: valkey-standalone: image: valkey/valkey:latest platform: ${DOCKER_PLATFORM:-linux/amd64} - command: > - valkey-server - --port 6379 - --bind 0.0.0.0 - --appendonly no - --protected-mode no - --ignore-warnings ARM64-COW-BUG + # Exec form passes each ACL rule as its own argument, unchanged. + command: + - valkey-server + - --port + - "6379" + - --bind + - 0.0.0.0 + - --appendonly + - "no" + - --protected-mode + - "no" + - --ignore-warnings + - ARM64-COW-BUG + - --user + - "default" + - "on" + - "nopass" + - "~*" + - "&*" + - "+@all" + - --user + - "appuser" + - "on" + - ">admin" + - "~*" + - "&*" + - "+@all" + - --user + - "restricted" + - "on" + - ">restricted" + - "~*" + - "&*" + - "+@all" + - "-acl" + - "-commandlog" healthcheck: test: ["CMD", "valkey-cli", "-p", "6379", "PING"] interval: 2s diff --git a/tools/valkey-cluster/scripts/cluster_init.sh b/tools/valkey-cluster/scripts/cluster_init.sh index 25e79867..bb260646 100755 --- a/tools/valkey-cluster/scripts/cluster_init.sh +++ b/tools/valkey-cluster/scripts/cluster_init.sh @@ -6,6 +6,9 @@ FIRST_HOST=valkey-7001 FIRST_PORT=7001 VALKEY_USER=${VALKEY_USER:-appuser} VALKEY_PASS=${VALKEY_PASS:-admin} +# Second ACL user for identity-isolation tests: everything except ACL and COMMANDLOG. +RESTRICTED_USER=${RESTRICTED_USER:-restricted} +RESTRICTED_PASS=${RESTRICTED_PASS:-restricted} NODES=" valkey-7001:7001 @@ -31,6 +34,22 @@ wait_port() { exit 1 } +# ACL users are per node and are lost when a node restarts, so set them on +# every run, before the "already healthy" early exit. +setup_acl_users() { + for hp in $NODES; do + host="${hp%%:*}" + port="${hp##*:}" + valkey-cli -h "$host" -p "$port" ACL SETUSER "$VALKEY_USER" on ">$VALKEY_PASS" '~*' '&*' +@all + valkey-cli -h "$host" -p "$port" ACL SETUSER "$RESTRICTED_USER" on ">$RESTRICTED_PASS" '~*' '&*' +@all -acl -commandlog + done +} + +for hp in $NODES; do + wait_port "$hp" +done +setup_acl_users + echo "Checking cluster..." info="$(valkey-cli -h "$FIRST_HOST" -p "$FIRST_PORT" CLUSTER INFO 2>/dev/null || true | tr -d '\r')" state="$(printf '%s\n' "$info" | awk -F: '/^cluster_state/ {print $2}')" @@ -89,8 +108,7 @@ while [ "$i" -lt 120 ]; do valkey-cli -h "$host" -p "$port" CONFIG SET maxmemory 100mb valkey-cli -h "$host" -p "$port" CONFIG SET maxmemory-policy allkeys-lfu valkey-cli -h "$host" -p "$port" CONFIG SET cluster-slot-stats-enabled yes - valkey-cli -h "$host" -p "$port" ACL SETUSER $VALKEY_USER on \>$VALKEY_PASS ~* +@all - + done echo "✅ LFU eviction configured." exit 0 From c28b5f7247400317d604cffc2343ef7854154602 Mon Sep 17 00:00:00 2001 From: Argus Li Date: Thu, 22 Oct 2026 10:03:53 -0700 Subject: [PATCH 3/9] Turn off collector features in Kubernetes Each Valkey pod runs one metrics sidecar under one set of credentials, so its data cannot be scoped to the requesting user. In Kubernetes mode, hot keys, big keys, command logs, monitor, CPU, memory, the node dashboard and config updates now return their usual error reply with the message "Metrics collection is not available in Kubernetes in this release." They do not contact the sidecars. Commands, the key browser and the cluster dashboard keep working. Signed-off-by: Argus Li --- .../orchestrator-k8s-register.test.ts | 91 ++++++++++++++++++- apps/server/src/actions/bigkeys.ts | 8 +- apps/server/src/actions/commandLogs.ts | 8 +- apps/server/src/actions/config.ts | 26 +++++- apps/server/src/actions/cpuUsage.ts | 6 +- apps/server/src/actions/hotkeys.ts | 8 +- apps/server/src/actions/memoryUsage.ts | 6 +- apps/server/src/actions/monitorAction.ts | 8 +- apps/server/src/actions/stats.ts | 8 +- apps/server/src/actions/utils.ts | 11 +++ apps/server/src/set-dashboard-data.ts | 2 +- 11 files changed, 171 insertions(+), 11 deletions(-) diff --git a/apps/server/src/__tests__/orchestrator-k8s-register.test.ts b/apps/server/src/__tests__/orchestrator-k8s-register.test.ts index 3512fabf..bc4d7eb5 100644 --- a/apps/server/src/__tests__/orchestrator-k8s-register.test.ts +++ b/apps/server/src/__tests__/orchestrator-k8s-register.test.ts @@ -1,4 +1,4 @@ -import { describe, it, afterEach } from "node:test" +import { describe, it, afterEach, mock } from "node:test" import assert from "node:assert" import { ORCHESTRATOR_AUTH_DOMAIN, @@ -112,3 +112,92 @@ describe("K8s shared-key registration", () => { }) }) }) + +describe("collector-backed actions in Kubernetes", async () => { + const { VALKEY } = await import("valkey-common") + const { KUBERNETES_METRICS_UNAVAILABLE } = await import("../actions/utils") + const { hotKeysRequested } = await import("../actions/hotkeys") + const { bigKeysRequested } = await import("../actions/bigkeys") + const { commandLogsRequested } = await import("../actions/commandLogs") + const { monitorRequested } = await import("../actions/monitorAction") + const { cpuUsageRequested } = await import("../actions/cpuUsage") + const { memoryUsageRequested } = await import("../actions/memoryUsage") + const { setData } = await import("../actions/stats") + const { updateConfig } = await import("../actions/config") + + const NODE = "valkey-0-6379" + const CONN = `${NODE}-db0` + + const cases = [ + { name: "hot keys", handler: hotKeysRequested, type: VALKEY.HOTKEYS.hotKeysError, payload: {} }, + { name: "big keys", handler: bigKeysRequested, type: VALKEY.BIGKEYS.bigKeysError, payload: {} }, + { name: "command logs", handler: commandLogsRequested, type: VALKEY.COMMANDLOGS.commandLogsError, payload: { commandLogType: "slow" } }, + { name: "monitor", handler: monitorRequested, type: VALKEY.MONITOR.monitorError, payload: { monitorAction: "status" } }, + { name: "cpu", handler: cpuUsageRequested, type: VALKEY.CPU.cpuUsageError, payload: {} }, + { name: "memory", handler: memoryUsageRequested, type: VALKEY.MEMORY.memoryUsageError, payload: {} }, + { name: "dashboard", handler: setData, type: VALKEY.STATS.setError, payload: {} }, + ] + + for (const c of cases) { + it(`answers ${c.name} with the Kubernetes message and makes no fetch`, async () => { + const sent: { type: string; payload: Record }[] = [] + const fetchMock = mock.method(globalThis, "fetch", async () => new Response("{}")) + metricsServerMap.set(NODE, { metricsURI: "http://10.0.0.1:3000", pid: undefined, lastSeen: Date.now() }) + try { + await c.handler({ + ws: { send: (m: string) => sent.push(JSON.parse(m)), readyState: 1, OPEN: 1 }, + connectionId: CONN, + metricsServerMap, + clusterNodesRegistry, + connectedNodesByCluster: new Map(), + clients: new Map(), + ownerKey: "password~default", + } as never)({ type: "x", payload: { connectionId: CONN, ...c.payload }, meta: undefined }) + + assert.strictEqual(fetchMock.mock.callCount(), 0) + assert.strictEqual(sent.length, 1) + assert.strictEqual(sent[0].type, c.type) + assert.strictEqual(sent[0].payload.error, KUBERNETES_METRICS_UNAVAILABLE) + assert.strictEqual(sent[0].payload.errorKind, undefined) + } finally { + mock.restoreAll() + } + }) + } + + it("answers a config update with a failed node result per node and makes no fetch", async () => { + const sent: { type: string; payload: Record }[] = [] + const fetchMock = mock.method(globalThis, "fetch", async () => new Response("{}")) + clusterNodesRegistry.set("c1", { + "n1-6379": { host: "n1", port: 6379, tls: false, verifyTlsCertificate: false }, + "n2-6379": { host: "n2", port: 6379, tls: false, verifyTlsCertificate: false }, + }) + try { + await updateConfig({ + ws: { send: (m: string) => sent.push(JSON.parse(m)), readyState: 1, OPEN: 1 }, + connectionId: "n1-6379-db0", + metricsServerMap, + clusterNodesRegistry, + connectedNodesByCluster: new Map(), + clients: new Map(), + ownerKey: "password~default", + } as never)({ + type: VALKEY.CONFIG.updateConfig, + payload: { connectionId: "n1-6379-db0", clusterId: "c1", config: { epics: {} }, monitorAction: "start" }, + meta: undefined, + }) + assert.strictEqual(fetchMock.mock.callCount(), 0) + assert.strictEqual(sent.length, 1) + assert.strictEqual(sent[0].type, VALKEY.CONFIG.updateConfigFailed) + assert.strictEqual(sent[0].payload.clusterId, "c1") + assert.strictEqual(sent[0].payload.outcome, "failed") + assert.deepStrictEqual(sent[0].payload.nodeResults, [ + { nodeId: "n1-6379", success: false, message: KUBERNETES_METRICS_UNAVAILABLE }, + { nodeId: "n2-6379", success: false, message: KUBERNETES_METRICS_UNAVAILABLE }, + ]) + assert.deepStrictEqual(sent[0].payload.notAttemptedNodeIds, []) + } finally { + mock.restoreAll() + } + }) +}) diff --git a/apps/server/src/actions/bigkeys.ts b/apps/server/src/actions/bigkeys.ts index c0c78d59..7ff9fbd5 100644 --- a/apps/server/src/actions/bigkeys.ts +++ b/apps/server/src/actions/bigkeys.ts @@ -1,7 +1,7 @@ import { type WebSocket } from "ws" import { VALKEY, type AggregateReplyId, toNodeId, buildUrl } from "valkey-common" import * as R from "ramda" -import { withDeps, Deps } from "./utils" +import { withDeps, Deps, collectorFeaturesDisabled, KUBERNETES_METRICS_UNAVAILABLE } from "./utils" type BigKey = { key: string @@ -52,12 +52,14 @@ const sendBigKeysError = ( ws: WebSocket, nodeId: string, error: unknown, + clusterId?: unknown, ) => { console.error(error) ws.send( JSON.stringify({ type: VALKEY.BIGKEYS.bigKeysError, payload: { + ...(typeof clusterId === "string" && clusterId && { clusterId }), nodeId, error: error instanceof Error ? error.message : String(error), }, @@ -68,6 +70,10 @@ const sendBigKeysError = ( export const bigKeysRequested = withDeps( async ({ ws, metricsServerMap, action, clusterNodesRegistry }) => { const { connectionId, clusterId, scanLimit, topN } = action.payload + if (collectorFeaturesDisabled()) { + sendBigKeysError(ws, toNodeId(connectionId), KUBERNETES_METRICS_UNAVAILABLE, clusterId) + return + } // Resolve once so every node and the merge cap use the same value. const effectiveTopN = Number(topN) || DEFAULT_TOP_N diff --git a/apps/server/src/actions/commandLogs.ts b/apps/server/src/actions/commandLogs.ts index 146a499d..c6a0a659 100644 --- a/apps/server/src/actions/commandLogs.ts +++ b/apps/server/src/actions/commandLogs.ts @@ -1,7 +1,7 @@ import { type WebSocket } from "ws" import { VALKEY, COMMANDLOG_TYPE, type AggregateReplyId, toNodeId, buildUrl } from "valkey-common" import * as R from "ramda" -import { withDeps, Deps } from "./utils" +import { withDeps, Deps, collectorFeaturesDisabled, KUBERNETES_METRICS_UNAVAILABLE } from "./utils" type CommandLogType = typeof COMMANDLOG_TYPE.SLOW | typeof COMMANDLOG_TYPE.LARGE_REQUEST | typeof COMMANDLOG_TYPE.LARGE_REPLY @@ -67,12 +67,14 @@ const sendCommandLogsError = ( ws: WebSocket, nodeId: string, error: unknown, + clusterId?: unknown, ) => { console.error(error) ws.send( JSON.stringify({ type: VALKEY.COMMANDLOGS.commandLogsError, payload: { + ...(typeof clusterId === "string" && clusterId && { clusterId }), nodeId, error: error instanceof Error ? error.message : String(error), }, @@ -100,6 +102,10 @@ const fetchCommandLogs = async (metricsServerURI: string, commandLogType: Comman export const commandLogsRequested = withDeps( async ({ ws, metricsServerMap, action, clusterNodesRegistry }) => { const { connectionId, clusterId } = action.payload + if (collectorFeaturesDisabled()) { + sendCommandLogsError(ws, toNodeId(connectionId), KUBERNETES_METRICS_UNAVAILABLE, clusterId) + return + } const commandLogType: CommandLogType = action.payload.commandLogType as CommandLogType const count = Number(action.payload.count) || Number(process.env.COMMAND_LOGS_COUNT) || 100 diff --git a/apps/server/src/actions/config.ts b/apps/server/src/actions/config.ts index 28d94781..eb5d98c0 100644 --- a/apps/server/src/actions/config.ts +++ b/apps/server/src/actions/config.ts @@ -6,7 +6,15 @@ import { toNodeId, buildUrl } from "valkey-common" -import { Deps, withDeps, fetchWithTimeout, safeSend, type ReduxAction } from "./utils" +import { + Deps, + withDeps, + fetchWithTimeout, + safeSend, + collectorFeaturesDisabled, + KUBERNETES_METRICS_UNAVAILABLE, + type ReduxAction +} from "./utils" import { toOutcome, type CollectionResult } from "./node-fanout" import { runWithRetry, type RetryRunResult, type NodeStatusUpdate } from "./retry-runner" import { monitorRequested } from "./monitorAction" @@ -156,6 +164,22 @@ export const updateConfig = withDeps( // Without a config there is no session to run if (!config) return + // Kubernetes: no collector may be configured. Reply once, with every + // target node failed, and skip the monitor rider. + if (collectorFeaturesDisabled()) { + const { connectionId, clusterId } = action.payload + const replyId: AggregateReplyId = + typeof clusterId === "string" && clusterId ? { clusterId } : { nodeId: toNodeId(connectionId) } + const nodeIds = "clusterId" in replyId + ? Object.keys(deps.clusterNodesRegistry.get(replyId.clusterId) ?? {}) + : [replyId.nodeId] + sendConfigReply(deps.ws, replyId, { + attempted: nodeIds.map((nodeId) => ({ nodeId, success: false, message: KUBERNETES_METRICS_UNAVAILABLE })), + notAttempted: [], + }, config) + return + } + // Complete the config push and collect its per-node results (sending the // Config_Reply) before issuing the toggle. The session reads only // `payload.{connectionId, clusterId, config}`. diff --git a/apps/server/src/actions/cpuUsage.ts b/apps/server/src/actions/cpuUsage.ts index a1b81a5e..931dbea7 100644 --- a/apps/server/src/actions/cpuUsage.ts +++ b/apps/server/src/actions/cpuUsage.ts @@ -1,6 +1,6 @@ import { type WebSocket } from "ws" import { VALKEY, toNodeId, buildUrl, MILLISECONDS_IN_AN_HOUR } from "valkey-common" -import { withDeps, Deps, fetchWithTimeout } from "./utils" +import { withDeps, Deps, fetchWithTimeout, collectorFeaturesDisabled, KUBERNETES_METRICS_UNAVAILABLE } from "./utils" type CpuUsageResponse = Array<{ timestamp: number @@ -49,6 +49,10 @@ type RequestPayload = { export const cpuUsageRequested = withDeps( async ({ ws, metricsServerMap, action, connectedNodesByCluster }) => { const { connectionId, clusterId, timeRange = 12 * MILLISECONDS_IN_AN_HOUR } = action.payload as unknown as RequestPayload + if (collectorFeaturesDisabled()) { + sendCpuUsageError(ws, connectionId, new Error(KUBERNETES_METRICS_UNAVAILABLE)) + return + } const connectionIds = clusterId ? connectedNodesByCluster.get(clusterId as string) ?? [] : [connectionId] const promises = connectionIds.map(async (connectionId: string) => { diff --git a/apps/server/src/actions/hotkeys.ts b/apps/server/src/actions/hotkeys.ts index d87cf8f8..ee94199f 100644 --- a/apps/server/src/actions/hotkeys.ts +++ b/apps/server/src/actions/hotkeys.ts @@ -1,7 +1,7 @@ import { type WebSocket } from "ws" import { VALKEY, type AggregateReplyId, toNodeId, buildUrl } from "valkey-common" import * as R from "ramda" -import { withDeps, Deps } from "./utils" +import { withDeps, Deps, collectorFeaturesDisabled, KUBERNETES_METRICS_UNAVAILABLE } from "./utils" type HotKeysResponse = { nodeId: string @@ -38,12 +38,14 @@ const sendHotKeysError = ( ws: WebSocket, nodeId: string, error: unknown, + clusterId?: unknown, ) => { console.error(error) ws.send( JSON.stringify({ type: VALKEY.HOTKEYS.hotKeysError, payload: { + ...(typeof clusterId === "string" && clusterId && { clusterId }), nodeId, error: error instanceof Error ? error.message : String(error), }, @@ -54,6 +56,10 @@ const sendHotKeysError = ( export const hotKeysRequested = withDeps( async ({ ws, metricsServerMap, action, clusterNodesRegistry }) => { const { connectionId, clusterId, lfuEnabled, clusterSlotStatsEnabled } = action.payload + if (collectorFeaturesDisabled()) { + sendHotKeysError(ws, toNodeId(connectionId), KUBERNETES_METRICS_UNAVAILABLE, clusterId) + return + } const count = Number(action.payload.count) || Number(process.env.HOT_KEYS_COUNT) || 50 const nodes = diff --git a/apps/server/src/actions/memoryUsage.ts b/apps/server/src/actions/memoryUsage.ts index 51d3f726..7870248d 100644 --- a/apps/server/src/actions/memoryUsage.ts +++ b/apps/server/src/actions/memoryUsage.ts @@ -1,6 +1,6 @@ import { type WebSocket } from "ws" import { VALKEY, toNodeId, buildUrl, MILLISECONDS_IN_AN_HOUR } from "valkey-common" -import { withDeps, Deps, fetchWithTimeout } from "./utils" +import { withDeps, Deps, fetchWithTimeout, collectorFeaturesDisabled, KUBERNETES_METRICS_UNAVAILABLE } from "./utils" interface MemoryMetric { description: string @@ -56,6 +56,10 @@ type RequestPayload = { export const memoryUsageRequested = withDeps( async ({ ws, metricsServerMap, action, connectedNodesByCluster }) => { const { connectionId, clusterId, timeRange = 12 * MILLISECONDS_IN_AN_HOUR } = action.payload as unknown as RequestPayload + if (collectorFeaturesDisabled()) { + sendMemoryUsageError(ws, connectionId, new Error(KUBERNETES_METRICS_UNAVAILABLE)) + return + } const connectionIds = clusterId ? connectedNodesByCluster.get(clusterId as string) ?? [] : [connectionId] const promises = connectionIds.map(async (connectionId: string) => { // metricsServerMap is keyed by metrics-node-id. diff --git a/apps/server/src/actions/monitorAction.ts b/apps/server/src/actions/monitorAction.ts index 158a58af..e3efff6a 100644 --- a/apps/server/src/actions/monitorAction.ts +++ b/apps/server/src/actions/monitorAction.ts @@ -1,6 +1,6 @@ import { type WebSocket } from "ws" import { VALKEY, type MonitorAction, type NodeReplyId, toNodeId, buildUrl } from "valkey-common" -import { withDeps, type Deps, fetchWithTimeout } from "./utils" +import { withDeps, type Deps, fetchWithTimeout, collectorFeaturesDisabled, KUBERNETES_METRICS_UNAVAILABLE } from "./utils" import { getOtherWatchers, scopedId } from "../node-watchers" type MonitorResponse = { @@ -46,6 +46,12 @@ const sendMonitorError = ( export const monitorRequested = withDeps( async ({ ws, metricsServerMap, action, clusterNodesRegistry, ownerKey }) => { const { connectionId, clusterId, monitorAction } = action.payload + if (collectorFeaturesDisabled()) { + const nodeId = toNodeId(connectionId) + const replyId = typeof clusterId === "string" && clusterId ? { clusterId, nodeId } : { nodeId } + sendMonitorError(ws, replyId, new Error(KUBERNETES_METRICS_UNAVAILABLE)) + return + } // Internal restriction used by the save flow to toggle only the nodes // whose config push succeeded. const restrictToNodeIds = action.payload.targetNodeIds as string[] | undefined diff --git a/apps/server/src/actions/stats.ts b/apps/server/src/actions/stats.ts index bc7f0f5f..cc94d814 100644 --- a/apps/server/src/actions/stats.ts +++ b/apps/server/src/actions/stats.ts @@ -1,9 +1,13 @@ import { toNodeId } from "valkey-common" -import { type Deps, withDeps } from "./utils" -import { setDashboardData } from "../set-dashboard-data" +import { type Deps, withDeps, collectorFeaturesDisabled, KUBERNETES_METRICS_UNAVAILABLE } from "./utils" +import { sendSetDataError, setDashboardData } from "../set-dashboard-data" export const setData = withDeps( async ({ ws, metricsServerMap, connectionId }) => { + if (collectorFeaturesDisabled()) { + sendSetDataError(ws, connectionId, new Error(KUBERNETES_METRICS_UNAVAILABLE)) + return + } const metricsServerURI = metricsServerMap.get(toNodeId(connectionId))?.metricsURI await setDashboardData(connectionId, metricsServerURI, ws) }, diff --git a/apps/server/src/actions/utils.ts b/apps/server/src/actions/utils.ts index 20f98d3c..01bc2a5c 100644 --- a/apps/server/src/actions/utils.ts +++ b/apps/server/src/actions/utils.ts @@ -1,5 +1,6 @@ import { type GlideClient, type GlideClusterClient } from "@valkey/valkey-glide" import { FETCH_TIMEOUT_MS } from "valkey-common" +import { isKubernetes } from "../metrics-orchestrator" import { ClusterNodeMap, MetricsServerMap } from "../metrics-orchestrator" import type { OwnerState } from "../owner" import type { SocketBinding } from "../socket-binding" @@ -46,6 +47,16 @@ export const withDeps = export type Handler = (deps: Deps) => (action: ReduxAction) => Promise +/** + * In Kubernetes, each Valkey pod runs one metrics sidecar under one set of + * credentials, so its data cannot be scoped to the requesting user. Phase 1 + * turns off the collector-backed features there and answers with this message. + */ +export const KUBERNETES_METRICS_UNAVAILABLE = "Metrics collection is not available in Kubernetes in this release." + +/** True when collector-backed features are turned off (Kubernetes mode). */ +export const collectorFeaturesDisabled = (): boolean => isKubernetes + export const unknownHandler: Handler = () => async (action: { type: string }) => { console.warn("Unknown action type:", action.type) diff --git a/apps/server/src/set-dashboard-data.ts b/apps/server/src/set-dashboard-data.ts index 9855d326..e05885ef 100644 --- a/apps/server/src/set-dashboard-data.ts +++ b/apps/server/src/set-dashboard-data.ts @@ -36,7 +36,7 @@ const sendSetDataFulfilled = ( ) } -const sendSetDataError = ( +export const sendSetDataError = ( ws: WebSocket, connectionId: string, error: unknown, From 5f777961dd1f6b32d602cddd96a0c4ffeaaea016 Mon Sep 17 00:00:00 2001 From: Argus Li Date: Thu, 22 Oct 2026 10:05:30 -0700 Subject: [PATCH 4/9] Raise the default ORCHESTRATOR_RATE_LIMIT_MAX to 2000 The server now runs one collector per node per connected Valkey identity, and all spawned collectors share one loopback rate-limit bucket. The old default of 600 requests a minute covered about 100 collectors. The new default covers about 300. Signed-off-by: Argus Li --- apps/server/src/index.ts | 6 +++--- common/src/__tests__/orchestrator-auth.test.ts | 4 ++++ common/src/orchestrator-auth.ts | 12 +++++++----- 3 files changed, 14 insertions(+), 8 deletions(-) diff --git a/apps/server/src/index.ts b/apps/server/src/index.ts index 34b01864..961d37b2 100644 --- a/apps/server/src/index.ts +++ b/apps/server/src/index.ts @@ -80,11 +80,11 @@ const limiter = rateLimit({ * Dedicated limit for the collector control plane. * * `/orchestrator/*` gets its own bucket rather than sharing the UI limiter - * because the UI limit is per browser, while this one is effectively per cluster. + * because the UI limit is per browser, while this one is effectively per server. * * Spawned collectors all call back to `SERVER_HOST` from the same host, so - * express-rate-limit's per-address keying puts an entire cluster's collectors - * into a single loopback bucket. At a 10s ping interval that is 6 requests per + * express-rate-limit's per-address keying puts every collector (one per node + * per connected Valkey identity) into a single loopback bucket. At a 10s ping interval that is 6 requests per * minute per node, so the UI's 100/min ceiling would begin throttling * legitimate collectors at around 16 nodes. Sizing this separately keeps the * two tunable independently. diff --git a/common/src/__tests__/orchestrator-auth.test.ts b/common/src/__tests__/orchestrator-auth.test.ts index 70ee025c..9123ad38 100644 --- a/common/src/__tests__/orchestrator-auth.test.ts +++ b/common/src/__tests__/orchestrator-auth.test.ts @@ -452,6 +452,10 @@ describe("resolveOrchestratorRateLimitMax", () => { // The default has to cover a whole cluster, not one collector: spawned // collectors share a single loopback bucket at 6 requests per minute each. + it("defaults to 2000 requests per minute", () => { + assert.strictEqual(ORCHESTRATOR_RATE_LIMIT_DEFAULT_MAX, 2000) + }) + it("defaults high enough for a large cluster's shared loopback bucket", () => { assert.ok( ORCHESTRATOR_RATE_LIMIT_DEFAULT_MAX >= 6 * 50, diff --git a/common/src/orchestrator-auth.ts b/common/src/orchestrator-auth.ts index 897588fa..b1570f4e 100644 --- a/common/src/orchestrator-auth.ts +++ b/common/src/orchestrator-auth.ts @@ -39,12 +39,14 @@ export const ORCHESTRATOR_RATE_LIMIT_MAX_ENV = "ORCHESTRATOR_RATE_LIMIT_MAX" * Default requests per minute allowed on `/orchestrator/*`, per source address. * * Sized for the shared-bucket case rather than per-collector traffic. Spawned - * collectors all call back to `SERVER_HOST` from the same host, so an entire - * cluster's collectors land in one loopback bucket: at a 10s ping interval - * that is 6 requests per minute per node, so this default covers roughly a - * 100-node cluster with headroom for registration retries. + * collectors all call back to `SERVER_HOST` from the same host, so every + * collector the server runs lands in one loopback bucket. There is one + * collector per node per Valkey identity that is connected to it. At a 10s + * ping interval each collector sends about 6 requests per minute, so this + * default covers about 300 collectors (for example 30 primaries × 10 users) + * with headroom for registration retries. */ -export const ORCHESTRATOR_RATE_LIMIT_DEFAULT_MAX = 600 +export const ORCHESTRATOR_RATE_LIMIT_DEFAULT_MAX = 2000 /** Protocol version, prefixing both the canonical message and the credential. */ export const ORCHESTRATOR_AUTH_VERSION = "v1" From 236cdcd12a2e1b2d7ba09f59eb476b264e5c57b9 Mon Sep 17 00:00:00 2001 From: Argus Li Date: Thu, 22 Oct 2026 10:07:19 -0700 Subject: [PATCH 5/9] Document per-identity isolation Document that each Valkey identity gets its own clients and collectors, the AUTH check before every connect, the failed-AUTH limit, and that the web app asks for the password after a reload. Describe the per-owner DATA_DIR layout and settings file, the new ORCHESTRATOR_RATE_LIMIT_MAX default with a sizing guide, and the cost of one collector per node per identity. State that collector features are not available in Kubernetes in this release and that a bare-metal web server is not supported. Signed-off-by: Argus Li --- CONTRIBUTING.md | 4 ++ .../src/content/docs/configuration/index.md | 2 +- .../src/content/docs/configuration/metrics.md | 4 +- .../src/content/docs/configuration/server.md | 47 +++++++++++++++---- .../src/content/docs/deployment/docker.md | 6 ++- .../src/content/docs/deployment/kubernetes.md | 4 ++ .../docs/deployment/resource-sizing.md | 16 ++++--- .../docs/development/platform-support.md | 6 ++- .../src/content/docs/features/connections.md | 6 ++- .../src/content/docs/reference/limitations.md | 6 +++ 10 files changed, 78 insertions(+), 23 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 63bbee3b..10ee9334 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -47,6 +47,10 @@ Keep boundaries clear: ### Connection Tracking The Server keeps active Valkey clients in an in-memory Client_Map maintained by the Connection_Manager (`apps/server/src/connection.ts`). Each Client_Map entry is keyed by `buildConnectionId(host, port, db)` exported from `common/src/connection-id.ts`. Do not reconstruct this identifier elsewhere. +All per-connection server state is scoped by **owner identity**, the Valkey identity the connection authenticated as (`ownerOf` in `apps/server/src/owner.ts`). Each owner has its own `OwnerState`: clients, cluster topology, collector credentials, collectors and their keys. CPU baselines, config sessions and node watchers carry the owner key too. Handlers get the requesting owner's maps through `Deps`. Do not read the module-level maps in `metrics-orchestrator.ts` from a user path: they are the env owner's state (the `VALKEY_*` identity). + +A WebSocket may act only on connections it opened with credentials Valkey accepted. `resolveAction` in `apps/server/src/socket-binding.ts` resolves every action to its owner or rejects it. Every connect first proves the credentials with a short-lived client (`createAuthProbeClient`), under the failed-AUTH limit in `apps/server/src/auth-limiter.ts`. + ### Per-Node Config Fan-Out & Reply Contract Monitor-configuration updates fan out to every cluster node and collect a per-node outcome rather than treating the cluster as all-or-nothing. When adding or changing this flow: diff --git a/docs-site/src/content/docs/configuration/index.md b/docs-site/src/content/docs/configuration/index.md index 387aeb46..f5250589 100644 --- a/docs-site/src/content/docs/configuration/index.md +++ b/docs-site/src/content/docs/configuration/index.md @@ -39,7 +39,7 @@ What it cares about: - `K8` — does not spawn children; expects externally-managed metrics sidecars to self-register via `/orchestrator/register` (Kubernetes) - **Note**: In `Web` and `K8` modes, set `VALKEY_HOST` / `VALKEY_PORT` plus the rest of the `VALKEY_*` credential variables so the server can discover cluster nodes on its own. - See [Picking a Mode](/configuration/server/#picking-a-mode) for the full breakdown. -- In `Electron` and `Web` modes the server **spawns** a metrics child process per node. It copies its own environment into the child and overrides a handful of values (`VALKEY_HOST`, `VALKEY_PORT`, credentials, `DATA_DIR`, `CONFIG_PATH`, `CONNECTION_ID`) so each child knows which node it is sampling. +- In `Electron` and `Web` modes the server **spawns** a metrics child process per node. It copies its own environment into the child and overrides a handful of values (`VALKEY_HOST`, `VALKEY_PORT`, credentials, `DATA_DIR`, `CONFIG_PATH`, `CONNECTION_ID`) so each child knows which node it is sampling. The server runs one child per node for each Valkey identity connected to it, and gives each child the data directory `//` and the settings file `//config.yml` of its identity. In short: configure the server, and most of its settings flow through to the metrics children automatically. diff --git a/docs-site/src/content/docs/configuration/metrics.md b/docs-site/src/content/docs/configuration/metrics.md index 0c5d8f89..c0f84f91 100644 --- a/docs-site/src/content/docs/configuration/metrics.md +++ b/docs-site/src/content/docs/configuration/metrics.md @@ -199,14 +199,14 @@ TCP port the metrics HTTP server listens on. Setting `PORT=0` lets the OS assign ### `DATA_DIR` -Directory where NDJSON metric files are written and rotated. The server passes a per-node subdirectory here when spawning children, so each child gets its own slice of disk. +Directory where NDJSON metric files are written and rotated. The server passes `//` here when spawning children, so each child gets its own slice of disk and each Valkey identity keeps its own history. See [`DATA_DIR` on the server](/configuration/server/#data_dir). - **Default:** `cfg.server.data_dir` from `config.yml` (`/app/data`) - **Overrides:** `cfg.server.data_dir` ### `CONFIG_PATH` -Absolute path to the `config.yml` file. When set, the metrics process loads its config from this location instead of the bundled `apps/metrics/config.yml`. The Electron build uses this to point at a config file packaged inside the app bundle. +Absolute path to the `config.yml` file. When set, the metrics process loads its config from this location instead of the bundled `apps/metrics/config.yml`. A server-spawned child gets the settings file of its Valkey identity, `//config.yml`, which the server copies once from its own `CONFIG_PATH` (or the bundled file). The Electron build points the server's `CONFIG_PATH` at a config file packaged inside the app bundle. `POST /update-config` writes tuning changes back into this file. Mounting it read-only or serving it from a ConfigMap is supported — updates then apply to the running process only and report `"persisted": false`. diff --git a/docs-site/src/content/docs/configuration/server.md b/docs-site/src/content/docs/configuration/server.md index 479182f2..584d234c 100644 --- a/docs-site/src/content/docs/configuration/server.md +++ b/docs-site/src/content/docs/configuration/server.md @@ -11,6 +11,8 @@ The server has no config file. Everything below is read from `process.env` at st Before touching individual variables, decide which mode the server should run in. `DEPLOYMENT_MODE` is the key switch — it determines which other variables actually matter. +The server runs only as the desktop app, in a Docker container, or in Kubernetes. Running `apps/server` directly on a host (bare metal or a VM, without a container) is not supported: if the server process crashes there, the collectors it spawned keep running and cannot register with a new server. + ### Electron mode (desktop default) When `DEPLOYMENT_MODE=Electron` (or unset in the desktop build), the server spawns a metrics child only for nodes the user has explicitly connected to. The UI initiates a connection, the server spawns a metrics child for that node, the child registers itself, and the server proxies its data back to the UI. When the UI disconnects, the child is killed. @@ -25,6 +27,18 @@ When `DEPLOYMENT_MODE=Web`, the server starts metrics processes for all cluster When `DEPLOYMENT_MODE=K8`, metrics processes are spawned **outside** the server (as pod sidecars) and call back to `POST /orchestrator/register` to advertise themselves. The server runs a reconcile loop that discovers cluster nodes via `VALKEY_HOST` / `VALKEY_PORT` and prunes stale registry entries older than `TTL` milliseconds. +Each sidecar collects under one set of credentials, so its data cannot be scoped to the user who asks for it. In this release the collector-backed features (dashboard history, hot keys, big keys, command logs, Monitor and collector settings) are not available in Kubernetes and show a message in their place. See [Kubernetes deployment](/deployment/kubernetes/). + +## Users and Valkey identities + +Valkey Admin has no login of its own. A user's identity is the Valkey user they connect as, and Valkey's ACLs decide what that user may do. + +- **Separate clients and collectors per Valkey identity.** The server keeps one Valkey client per connection and one metrics collector per node for each Valkey identity: the auth type and the Valkey username (`default` when no password is given). For ElastiCache IAM, the region and replication group are part of the identity. Users who connect as the same Valkey user share one client and one set of collectors. Users who connect as different Valkey users share nothing, so commands always run as the user's own Valkey user and the metrics they see were collected with their own credentials. +- **Valkey checks every reuse.** The identity is not secret, so it never grants access by itself. Before every connect, the server opens a short-lived connection and sends `AUTH` with the presented credentials. It continues only when Valkey accepts them. Valkey Admin never stores or compares passwords. +- **Failed authentication limit.** After 10 failed authentication attempts in 60 seconds for one node and one Valkey identity, the server rejects new attempts for that pair without contacting Valkey, until the 60-second window ends. +- **Connections belong to the browser connection.** A browser's WebSocket may act only on Valkey connections it opened itself. When the WebSocket drops briefly, the UI reconnects with the credentials it holds in memory. After a full page reload, or in a new tab, the user must enter the password again (the desktop app with an OS keystore reconnects without asking). IAM, GCP IAM and passwordless connections reconnect without a prompt. +- **Release.** A Valkey client is closed about 10 seconds after the last browser that uses it disconnects or closes the connection. The identity's collectors stop when no connection of that identity uses their node. The identity configured with `VALKEY_USERNAME`/`VALKEY_PASSWORD` is an exception: its collectors keep running. + ## Network ### `PORT` @@ -103,17 +117,26 @@ Key material a metrics collector uses to authenticate to `/orchestrator/register Requests per minute allowed on `/orchestrator/*`, counted per source address and tracked separately from the UI's own limit. -This is effectively a **per-cluster** budget, not a per-collector one. Spawned collectors all call back to `SERVER_HOST` from the same host, so every collector in a cluster shares a single loopback bucket. Each one sends roughly 6 requests per minute at the default 10s ping interval, so the budget you need scales with node count: +This is effectively a **per-server** budget, not a per-collector one. Spawned collectors all call back to `SERVER_HOST` from the same host, so every collector the server runs shares a single loopback bucket. The server runs one collector per monitored node **per Valkey identity** connected to it (see [Users and Valkey identities](#users-and-valkey-identities)). Each collector sends roughly 6 requests per minute at the default 10s ping interval. -| Cluster nodes | Steady-state requests/min | -|---|---| -| 6 | 36 | -| 30 | 180 | -| 100 | 600 | +**Sizing guide.** Count the collectors, then multiply by 6: -The default covers about 100 nodes with headroom for registration retries. Raise it for larger clusters, or if collectors are configured with a shorter `ping_interval`. Symptom of a ceiling that is too low: collectors logging `Register failed: 429` or `Ping failed: 429`, and nodes intermittently losing their metrics. +```text +collectors = monitored nodes × distinct Valkey identities connected to them +requests per min = collectors × 6 (at ping_interval 10000 ms) +set the limit to = requests per min × 1.5 (headroom for registration retries) +``` + +| Monitored nodes | Distinct Valkey identities | Collectors | Steady-state requests/min | Suggested limit | +|---|---|---|---|---| +| 6 | 1 | 6 | 36 | default | +| 30 | 5 | 150 | 900 | default | +| 30 | 10 | 300 | 1800 | `2700` | +| 100 | 10 | 1000 | 6000 | `9000` | + +The default covers about 300 collectors. Raise it when more identities connect to large clusters, or if collectors use a shorter `ping_interval`. A ceiling that is too low shows as collectors logging `Register failed: 429` or `Ping failed: 429`, and nodes that lose their metrics from time to time. Each collector is also a process on the Valkey Admin host and holds connections to Valkey, so size the host too (see [Resource Sizing](/deployment/resource-sizing/)). -- **Default:** `600` +- **Default:** `2000` - **Read in:** `apps/server/src/index.ts` ## Collector Authentication @@ -138,7 +161,7 @@ Beyond authentication, the server validates the advertised URI itself. It must b A collector that starts without `ORCHESTRATOR_KEY` logs a single error and shuts down rather than issuing requests that can only be refused. -Requests to `/orchestrator/*` are rate limited separately from the UI, defaulting to 600 per minute per source address — see [`ORCHESTRATOR_RATE_LIMIT_MAX`](#orchestrator_rate_limit_max). +Requests to `/orchestrator/*` are rate limited separately from the UI, defaulting to 2000 per minute per source address — see [`ORCHESTRATOR_RATE_LIMIT_MAX`](#orchestrator_rate_limit_max). ### `VALKEY_ADMIN_ALLOWED_WS_ORIGINS` @@ -241,7 +264,11 @@ Unlike the variables above, this one is **not** inherited from the server. The s ### `DATA_DIR` -Base directory the server passes to each spawned child. Each child gets its own subdirectory at `${DATA_DIR}/${nodeId}` for its NDJSON output, so a single `DATA_DIR` is enough for an entire cluster. +Base directory for collector data. Each Valkey identity gets its own directory, `${DATA_DIR}/`, and each collector of that identity writes its NDJSON output to `${DATA_DIR}//`. The identity's collector settings file is `${DATA_DIR}//config.yml`. The server copies it once from the base config file (`CONFIG_PATH`) and never overwrites it, so a settings change made by one identity never reaches another. + +`` is a readable, reversible encoding of the identity, for example `password~alice`, `password~%41lice` (for `Alice`), `iam~us-east-1~my-rg~iam-user` or `gcp-iam~default`. Lowercase letters, digits, `_`, `-` and `~` stay as they are. Every other byte becomes `%XX`, so names stay distinct on case-insensitive filesystems. Names longer than 200 bytes are cut and get a hash suffix (`~h`). + +History from earlier releases, in `${DATA_DIR}/`, has no known owner. The server does not read it. Delete it when you no longer need it. - **Default:** `apps/server/data`, resolved relative to the compiled server entry point diff --git a/docs-site/src/content/docs/deployment/docker.md b/docs-site/src/content/docs/deployment/docker.md index 685cdd5a..c624692f 100644 --- a/docs-site/src/content/docs/deployment/docker.md +++ b/docs-site/src/content/docs/deployment/docker.md @@ -5,6 +5,10 @@ description: Deploy Valkey Admin using Docker or Docker Compose Run Valkey Admin using Docker or Docker Compose for a zero-install web deployment. +:::note[Run the web server in a container] +Running the web server directly on a host (bare metal or a VM, with `node apps/server/dist/index.js` and no container) is not supported. The server spawns one metrics collector per node for each connected Valkey identity. If the server crashes outside a container, those collectors keep running and cannot register with a new server. When a container stops, the runtime stops all its processes. The desktop app is not affected. +::: + ## Docker Images Valkey Admin images are published to the following registries: @@ -107,7 +111,7 @@ See the [Metrics Configuration](/configuration/metrics/) page for available opti ## Resource Sizing -In Docker (Web) mode, Valkey Admin spawns a metrics server process per primary node. See [Resource Sizing](/deployment/resource-sizing/) for formulas, recommendations, and retention guidance. +In Docker (Web) mode, Valkey Admin spawns a metrics server process per primary node for each Valkey identity connected to it. See [Resource Sizing](/deployment/resource-sizing/) for formulas, recommendations, and retention guidance. ## Configuration Reference diff --git a/docs-site/src/content/docs/deployment/kubernetes.md b/docs-site/src/content/docs/deployment/kubernetes.md index 8e4c6e30..15906eda 100644 --- a/docs-site/src/content/docs/deployment/kubernetes.md +++ b/docs-site/src/content/docs/deployment/kubernetes.md @@ -7,6 +7,10 @@ Deploy Valkey Admin on Kubernetes by running metrics collectors as sidecars insi The workflow below uses Minikube for local development, but the manifests and sidecar pattern apply to any Kubernetes cluster. +:::caution[Collector features are not available in Kubernetes in this release] +Each Valkey pod runs one metrics sidecar under one set of credentials, so its data cannot be scoped to the Valkey user who asks for it. Valkey Admin therefore turns off the collector-backed features in Kubernetes: dashboard history (CPU and memory charts), the node dashboard, hot keys, big keys, command logs, Monitor and collector settings. Each of these shows "Metrics collection is not available in Kubernetes in this release." Commands, the key browser and the cluster dashboard keep working, because they use each user's own Valkey connection. A later release brings these features back through an operator-configured admin account. The sidecar steps below still apply, and sidecars still register. +::: + ## Architecture - **`valkey-admin-app`**: Main frontend + backend server, deployed as a standalone `Deployment` diff --git a/docs-site/src/content/docs/deployment/resource-sizing.md b/docs-site/src/content/docs/deployment/resource-sizing.md index 429e43d1..9967d4f0 100644 --- a/docs-site/src/content/docs/deployment/resource-sizing.md +++ b/docs-site/src/content/docs/deployment/resource-sizing.md @@ -13,16 +13,20 @@ Valkey Admin's resource footprint depends on **how** metrics collectors are spaw | Kubernetes | Sidecar inside each Valkey pod | The Valkey pods (main app stays small) | | Desktop (Electron) | Child processes on the user's machine | The user's machine | -The common factor: each primary node being monitored adds roughly **150 MB RAM** and **50 MB disk** for its metrics collector. +The common factor: each metrics collector adds roughly **150 MB RAM** and **50 MB disk**. Valkey Admin runs one collector per monitored primary node **per Valkey identity** connected to it. Users who connect as the same Valkey user share collectors. Users who connect as different Valkey users each get their own, so their metrics are collected with their own credentials. ## Web / Docker mode -In Docker (Web) mode, Valkey Admin spawns a metrics server process for each primary node in the cluster. All collectors run on the same host as the main app, so plan resources for the entire cluster up front. +In Docker (Web) mode, Valkey Admin spawns a metrics server process for each primary node in the cluster, for each Valkey identity connected to it. All collectors run on the same host as the main app, so plan resources for the entire cluster and the number of distinct Valkey users up front. **Formulas:** -- **RAM:** `(primary nodes × 150 MB) + 1 GB` -- **Disk:** `(primary nodes × 50 MB) + 1 GB` +- **Collectors:** `primary nodes × distinct Valkey identities` +- **RAM:** `(collectors × 150 MB) + 1 GB` +- **Disk:** `(collectors × 50 MB) + 1 GB` +- **Valkey connections:** each user's cluster connection holds two connections per node, and each collector holds its own connection to its node. + +For example, 10 distinct Valkey users on a cluster with 30 primaries can mean 300 collectors. The table below is for one identity; multiply the primary count by the number of identities before you read it. All collectors share one rate limit on the server's collector API, so also check [`ORCHESTRATOR_RATE_LIMIT_MAX`](/configuration/server/#orchestrator_rate_limit_max). ### Approximate recommendations @@ -42,7 +46,7 @@ For large clusters, consider [Kubernetes deployment](/deployment/kubernetes/) wh ## Kubernetes -Kubernetes flips the sizing story: instead of the main app spawning children, each Valkey pod runs its own metrics **sidecar**. The main `valkey-admin-app` deployment only orchestrates registrations and serves the UI, so it stays small regardless of cluster size. +Kubernetes flips the sizing story: instead of the main app spawning children, each Valkey pod runs its own metrics **sidecar**. In this release the collector-backed features are not available in Kubernetes (see [Kubernetes deployment](/deployment/kubernetes/)), so the sidecars are optional there. The main `valkey-admin-app` deployment only orchestrates registrations and serves the UI, so it stays small regardless of cluster size. **Main `valkey-admin-app` deployment:** @@ -60,7 +64,7 @@ See the [Kubernetes deployment guide](/deployment/kubernetes/) for the manifest The desktop app spawns a metrics child for each node you explicitly connect to. For large clusters, connecting to many nodes individually can blow up local RAM and disk usage on your laptop. -**Rule of thumb:** apply the Web/Docker formulas to your local machine, but count only the nodes you have actively connected — not the full cluster size. +**Rule of thumb:** apply the Web/Docker formulas to your local machine, but count only the nodes you have actively connected — not the full cluster size. If you connect to the same node as two Valkey users, count it twice. For example, if you open desktop connections to 20 nodes in a 50-shard cluster: diff --git a/docs-site/src/content/docs/development/platform-support.md b/docs-site/src/content/docs/development/platform-support.md index 7a5f13dd..5062138c 100644 --- a/docs-site/src/content/docs/development/platform-support.md +++ b/docs-site/src/content/docs/development/platform-support.md @@ -62,6 +62,8 @@ There is no native Windows desktop build. Windows users run the Docker image ins ### Docker (Web Deployment) +Running the web server directly on a host (bare metal or a VM, without a container) is not supported. Use the Docker image or Kubernetes. + - **Support Method:** Docker image on any platform - **Deployment:** Web interface only, served at port `8080` - **Images:** `valkey/valkey-admin`, `ghcr.io/valkey-io/valkey-admin`, `public.ecr.aws/valkey/valkey-admin` @@ -93,8 +95,8 @@ The desktop application built with Electron provides the complete Valkey Admin e | Key Browser | ✅ | ✅ | | Send Command Interface | ✅ | ✅ | | Cluster Topology | ✅ | ✅ | -| Hot Keys Monitoring | ✅ | ✅ | -| Command Logs | ✅ | ✅ | +| Hot Keys Monitoring | ✅ | ✅ (not available in Kubernetes in this release) | +| Command Logs | ✅ | ✅ (not available in Kubernetes in this release) | ### Web Application (All Platforms) diff --git a/docs-site/src/content/docs/features/connections.md b/docs-site/src/content/docs/features/connections.md index 29648385..b37797f7 100644 --- a/docs-site/src/content/docs/features/connections.md +++ b/docs-site/src/content/docs/features/connections.md @@ -57,7 +57,11 @@ Valkey Admin supports three authentication methods: ### Password Authentication -Provide a username and password. If no username is specified, the default ACL user is used. +Provide a username and password. If no username is specified, the default ACL user is used. If you give a username without a password, Valkey Admin sends no credentials and the connection runs as the `default` user. + +Every command runs as the Valkey user you connected with, and the metrics you see were collected with your credentials. Valkey Admin checks your credentials with Valkey on every connect, also when another user already connected to the same node. See [Users and Valkey identities](/configuration/server/#users-and-valkey-identities). + +In the web app, the browser does not keep the password across a page reload. After a reload, or in a new tab, enter the password again to reconnect. The desktop app with an OS keystore reconnects without asking. IAM, GCP IAM and passwordless connections reconnect automatically. ### AWS IAM Authentication diff --git a/docs-site/src/content/docs/reference/limitations.md b/docs-site/src/content/docs/reference/limitations.md index 0015255b..48df2eab 100644 --- a/docs-site/src/content/docs/reference/limitations.md +++ b/docs-site/src/content/docs/reference/limitations.md @@ -11,6 +11,9 @@ For runtime issues and fixes, see the [Troubleshooting guide](/reference/trouble - **No built-in authentication.** Valkey Admin does not provide its own login layer. Web deployments rely on an external auth proxy — for example, AWS Cognito in front of an Application Load Balancer, or a reverse proxy such as nginx or oauth2-proxy. - **No RBAC within the app.** Any user who can reach the UI can run any command the connected Valkey ACL allows. Scope what the connecting Valkey user is permitted to do, not who can use the app. +- **Isolation is per Valkey identity.** Users who connect as the same Valkey user share one client and one set of metrics collectors. Users who connect as different Valkey users share nothing. The cost is one collector per node per Valkey identity; see [Resource Sizing](/deployment/resource-sizing/). +- **Password after a page reload.** The web app asks for the password again after a page reload or in a new tab. +- **Failed authentication limit.** After 10 failed authentication attempts in 60 seconds for one node and one Valkey user, Valkey Admin rejects new attempts for 60 seconds. Anyone who knows a username and endpoint can trigger this lockout for that user. ## TLS @@ -22,5 +25,8 @@ For runtime issues and fixes, see the [Troubleshooting guide](/reference/trouble ## Architecture +- **No bare-metal web server.** Run the web server in Docker or Kubernetes, or use the desktop app. Running `apps/server` directly on a host is not supported. +- **Kubernetes metrics.** The collector-backed features (dashboard history, hot keys, big keys, command logs, Monitor) are not available in Kubernetes in this release. + - **Metrics servers are per-primary only.** Each primary node gets its own metrics collector; replica nodes are not independently monitored. - **Key browser sample size.** The key browser scans up to approximately 1,000 keys across the cluster. Keys beyond this limit are not listed but can still be found using the search function, which uses SCAN with a MATCH pattern (the same mechanism, just filtered). From c3696d2493931a3abc76e074a5230b890d090857 Mon Sep 17 00:00:00 2001 From: Argus Li Date: Thu, 22 Oct 2026 13:25:08 -0700 Subject: [PATCH 6/9] Remove the partial-context shim from connection.ts connectToValkey and teardownConnection now require a full owner state. The tests build one with createOwnerState, so the WeakMap fill-in and the empty _resetConnectInFlight and _resetInFlightClusterClients helpers are no longer needed. Signed-off-by: Argus Li --- .../server/src/__tests__/auth-limiter.test.ts | 23 +++------- apps/server/src/__tests__/connection.test.ts | 36 ++++++---------- apps/server/src/connection.ts | 43 ++++--------------- 3 files changed, 29 insertions(+), 73 deletions(-) diff --git a/apps/server/src/__tests__/auth-limiter.test.ts b/apps/server/src/__tests__/auth-limiter.test.ts index 9c13c944..1278be95 100644 --- a/apps/server/src/__tests__/auth-limiter.test.ts +++ b/apps/server/src/__tests__/auth-limiter.test.ts @@ -9,12 +9,8 @@ import { isAuthError, _resetAuthLimiter } from "../auth-limiter" -import { - _resetConnectInFlight, - _resetInFlightClusterClients, - connectToValkey, - discoverTopology -} from "../connection" +import { connectToValkey, discoverTopology } from "../connection" +import { createOwnerState } from "../owner" import { _reset as resetNodeWatchers } from "../node-watchers" import type { ConnectionDetails } from "../actions/connection" @@ -43,23 +39,18 @@ describe("auth-limiter", () => { let realNow: () => number let now: number - const ctx = () => ({ - clients: new Map(), - connectedNodesByCluster: new Map(), - clusterNodesRegistry: new Map(), - metricsServerMap: new Map(), - }) - const connect = (details: ConnectionDetails = DETAILS, connectionId = "conn-1") => - connectToValkey(ctx() as any, ws, { connectionDetails: details, connectionId, sessionId: "s" } as any) + connectToValkey( + { ...createOwnerState("password~alice"), connectedNodesByCluster: new Map() }, + ws, + { connectionDetails: details, connectionId }, + ) const lastType = () => messages[messages.length - 1]?.type const lastPayload = () => messages[messages.length - 1]?.payload beforeEach(() => { _resetAuthLimiter() - _resetConnectInFlight() - _resetInFlightClusterClients() resetNodeWatchers() messages = [] ws = { send: (m: string) => messages.push(JSON.parse(m)), readyState: 1, OPEN: 1 } diff --git a/apps/server/src/__tests__/connection.test.ts b/apps/server/src/__tests__/connection.test.ts index 434cd327..069f9ba7 100644 --- a/apps/server/src/__tests__/connection.test.ts +++ b/apps/server/src/__tests__/connection.test.ts @@ -8,8 +8,6 @@ import { GlideClient, GlideClusterClient, InfoOptions } from "@valkey/valkey-gli import { buildConnectionId, KEY_EVICTION_POLICY, VALKEY, toNodeId } from "valkey-common" import { GoogleAuth } from "google-auth-library" import { - _resetConnectInFlight, - _resetInFlightClusterClients, connectToValkey, getExistingConnection, teardownConnection @@ -18,6 +16,7 @@ import { resolveHostnameOrIpAddress, dns } from "../utils" import { checkJsonModuleAvailability } from "../check-json-module" import { ConnectionDetails } from "../actions/connection" import { type MetricsServerMap } from "../metrics-orchestrator" +import { createOwnerState, type OwnerState } from "../owner" import { _reset as resetNodeWatchers } from "../node-watchers" const DEFAULT_PAYLOAD = { @@ -164,14 +163,18 @@ describe("connectToValkey", () => { let clients: Map let connectedNodesByCluster: Map let metricsServerMap: MetricsServerMap + let owner: OwnerState beforeEach(() => { messages = [] mockWs = { send: mock.fn((msg: string) => messages.push(msg)), } - clients = new Map() + // One owner per test. Its in-flight maps are shared by every ctx() call in + // the test, so concurrent connects dedupe as they do in production. + owner = createOwnerState("password~user0") + clients = owner.clients connectedNodesByCluster = new Map() - metricsServerMap = new Map() + metricsServerMap = owner.metricsServerMap // metricsServerMap is keyed by metrics-node-id; the empty Connection_Identifier // strips to the empty string, which matches the empty connectionId used by // the default payload in these tests. @@ -191,11 +194,9 @@ describe("connectToValkey", () => { clients.clear() connectedNodesByCluster.clear() resetNodeWatchers() - _resetInFlightClusterClients() - _resetConnectInFlight() }) - const ctx = () => ({ clients, connectedNodesByCluster, clusterNodesRegistry: new Map(), metricsServerMap }) + const ctx = () => ({ ...owner, connectedNodesByCluster }) it("should connect to standalone Valkey instance", async () => { const standalone = buildStandaloneMock() @@ -749,8 +750,8 @@ describe("connectToValkey", () => { const aliceClient = buildStandaloneMock() const bobClient = buildStandaloneMock() const clientsFor = [aliceClient, bobClient] - const alice = { ...ctx(), clients: new Map() } - const bob = { ...ctx(), clients: new Map() } + const alice = { ...createOwnerState("password~user0"), connectedNodesByCluster: new Map() } + const bob = { ...createOwnerState("password~bob"), connectedNodesByCluster: new Map() } await withMockedClients(() => clientsFor.shift(), null, async () => { await connectToValkey(alice, mockWs, { ...DEFAULT_PAYLOAD, connectionId: "conn-O" }) await connectToValkey(bob, mockWs, { @@ -1186,10 +1187,7 @@ describe("teardownConnection", () => { clients.set("conn-1", { client: mockClient }) const metricsServerMap: MetricsServerMap = new Map() - teardownConnection( - { clients, clusterNodesRegistry: new Map(), metricsServerMap }, - "conn-1", - ) + teardownConnection({ ...createOwnerState("password~t"), clients, metricsServerMap }, "conn-1") assert.strictEqual(clients.has("conn-1"), false) assert.strictEqual(mockClient.close.mock.calls.length, 1) @@ -1202,10 +1200,7 @@ describe("teardownConnection", () => { clients.set("node-2", { client: sharedClient, clusterId: "c1" }) const metricsServerMap: MetricsServerMap = new Map() - teardownConnection( - { clients, clusterNodesRegistry: new Map(), metricsServerMap }, - "node-1", - ) + teardownConnection({ ...createOwnerState("password~t"), clients, metricsServerMap }, "node-1") assert.strictEqual(clients.has("node-1"), false) assert.strictEqual(clients.has("node-2"), true) @@ -1219,7 +1214,7 @@ describe("teardownConnection", () => { clients.set("node-2", { client: sharedClient, clusterId: "c1" }) const metricsServerMap: MetricsServerMap = new Map() - const ctx = { clients, clusterNodesRegistry: new Map(), metricsServerMap } + const ctx = { ...createOwnerState("password~t"), clients, metricsServerMap } teardownConnection(ctx, "node-1") teardownConnection(ctx, "node-2") @@ -1232,10 +1227,7 @@ describe("teardownConnection", () => { const metricsServerMap: MetricsServerMap = new Map() assert.doesNotThrow(() => { - teardownConnection( - { clients, clusterNodesRegistry: new Map(), metricsServerMap }, - "unknown", - ) + teardownConnection({ ...createOwnerState("password~t"), clients, metricsServerMap }, "unknown") }) }) }) diff --git a/apps/server/src/connection.ts b/apps/server/src/connection.ts index eedff9c3..46f96a7b 100644 --- a/apps/server/src/connection.ts +++ b/apps/server/src/connection.ts @@ -28,7 +28,7 @@ import { scopedId, subscribe } from "./node-watchers" import { clearCpuSamples } from "./node-utilization" import { createAuthProbeClient, createClusterValkeyClient, createStandaloneValkeyClient } from "./valkey-client" import { withAuthLimit } from "./auth-limiter" -import { createOwnerState, type OwnerState } from "./owner" +import { type OwnerState } from "./owner" /** * One owner's state plus the requesting socket's cluster bookkeeping. All @@ -39,25 +39,6 @@ export type ConnectionContext = OwnerState & { connectedNodesByCluster: Map } -// Fill-in state for callers that pass only some maps (tests), keyed by their -// `clients` map so concurrent calls with the same maps share in-flight state. -const fillIns = new WeakMap() - -/** - * Complete a partial context with the missing owner-state fields. - */ -export const toConnectionContext = ( - partial: Partial & { connectedNodesByCluster?: Map }, -): ConnectionContext => { - const anchor = partial.clients ?? partial - let fill = fillIns.get(anchor) - if (!fill) { - fill = createOwnerState(partial.ownerKey ?? "password~default") - fillIns.set(anchor, fill) - } - return { ...fill, connectedNodesByCluster: new Map(), ...partial } -} - type ClusterCommit = { clusterClient: GlideClusterClient clusterId: string @@ -95,10 +76,6 @@ type ClusterConnectFulfilledPayload = { connectionDetails: ClusterConnectionDetails } -// Concurrent cluster-client creations are deduplicated per owner by clusterId -// (OwnerState.inFlightClusterClients). Kept for the existing tests. -export const _resetInFlightClusterClients = () => {} - /** * Read the server's configured database count via `CONFIG GET`, trying each of * `configKeys` in order (cluster callers pass `["cluster-databases", "databases"]` as @@ -145,13 +122,6 @@ class ConnectionRejectedError extends Error { } } -// Per-connectionId mutex around the entire connectToValkey body. The second connector enters -// only after the first has fully committed (or failed), at which point the -// getExistingConnection path below reuses the committed -// entry (standalone or cluster). -// The map lives in the owner's state (OwnerState.connectInFlight). Kept for the existing tests. -export const _resetConnectInFlight = () => {} - /** * Prove that Valkey accepts `details`' credentials on this node: open a * short-lived client, then close it. Throws on rejection. Counted by the @@ -171,7 +141,7 @@ async function verifyCredentials( } export async function connectToValkey( - partialCtx: Partial, + ctx: ConnectionContext, ws: WebSocket, payload: { connectionDetails: ConnectionDetails @@ -179,7 +149,11 @@ export async function connectToValkey( isRetry?: boolean }, ) { - const ctx = toConnectionContext(partialCtx) + // Per-connectionId mutex (per owner) around the entire connectToValkey body. The + // second connector enters only after the first has fully committed (or failed), + // at which point the getExistingConnection path reuses the committed entry + // (standalone or cluster). Concurrent cluster-client creations are deduplicated + // per owner by clusterId in `ctx.inFlightClusterClients`. const { connectInFlight } = ctx const { connectionId } = payload const priorConnect = connectInFlight.get(connectionId) ?? Promise.resolve() @@ -814,10 +788,9 @@ export async function closeMetricsServer( * that owner uses their node. */ export function teardownConnection( - partialCtx: Partial, + ctx: OwnerState, connectionId: string, ) { - const ctx = toConnectionContext(partialCtx) const { clients, clusterNodesRegistry, clusterCredentials, ownerKey, env } = ctx // In web mode, the env owner's metrics servers are managed by the orchestrator From cc6aecf005e0027fadc92119e0a98b1b78d5264f Mon Sep 17 00:00:00 2001 From: Argus Li Date: Thu, 22 Oct 2026 14:04:11 -0700 Subject: [PATCH 7/9] Align the Kubernetes feature claims on the platform support page The Kubernetes section listed hot keys and command logs as features, and the feature table marked Dashboard & Metrics as available. In this release, the collector features are not available in Kubernetes. The section now lists them as unavailable, and the table row says that only the cluster dashboard is available, as kubernetes.md says. Signed-off-by: Argus Li --- docs-site/src/content/docs/development/platform-support.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs-site/src/content/docs/development/platform-support.md b/docs-site/src/content/docs/development/platform-support.md index 5062138c..81b824fa 100644 --- a/docs-site/src/content/docs/development/platform-support.md +++ b/docs-site/src/content/docs/development/platform-support.md @@ -79,9 +79,9 @@ Running the web server directly on a host (bare metal or a VM, without a contain - **Recommended for:** Large clusters (50+ primaries) **Features:** -- Web interface with all core features +- Web interface with the key browser, Send Command and the cluster dashboard - Lower memory usage on the main Valkey Admin pod compared to Docker mode -- Hot Keys monitoring and Command Logs (Valkey 8.0+/8.1+) +- The collector features are not available in this release: dashboard history (CPU and memory charts), the node dashboard, hot keys, big keys, command logs, Monitor and collector settings. See [Kubernetes deployment](/deployment/kubernetes/). ## Desktop vs Web Features @@ -91,7 +91,7 @@ The desktop application built with Electron provides the complete Valkey Admin e | Feature | Desktop | Docker / Kubernetes | |---------|---------|-----| -| Dashboard & Metrics | ✅ | ✅ | +| Dashboard & Metrics | ✅ | ✅ (Kubernetes: cluster dashboard only, in this release) | | Key Browser | ✅ | ✅ | | Send Command Interface | ✅ | ✅ | | Cluster Topology | ✅ | ✅ | From 1fa38d92e7a019ded7e86569c398c94368ea8672 Mon Sep 17 00:00:00 2001 From: Argus Li Date: Fri, 23 Oct 2026 13:17:41 -0700 Subject: [PATCH 8/9] Create owner state only after Valkey accepts the credentials resolveAction created an owner entry for every connect and discovery before Valkey checked the credentials, and nothing removed it. A client could grow server memory without limit by sending a new username each time, with no valid password. connectPending now proves the credentials first and creates the owner state only after Valkey accepts them. Discovery and other actions only look up existing state. When an owner has no client, collector, topology or connect in progress left, teardown removes its entry. The env owner is never removed. Signed-off-by: Argus Li --- .../src/__tests__/owner-lifecycle.test.ts | 186 ++++++++++++++++++ apps/server/src/actions/cluster.ts | 2 +- apps/server/src/actions/connection.ts | 35 +++- apps/server/src/actions/utils.ts | 6 +- apps/server/src/connection.ts | 91 +++++---- apps/server/src/index.ts | 13 +- apps/server/src/owner.ts | 27 +++ apps/server/src/socket-binding.ts | 13 +- 8 files changed, 312 insertions(+), 61 deletions(-) create mode 100644 apps/server/src/__tests__/owner-lifecycle.test.ts diff --git a/apps/server/src/__tests__/owner-lifecycle.test.ts b/apps/server/src/__tests__/owner-lifecycle.test.ts new file mode 100644 index 00000000..81ee14b0 --- /dev/null +++ b/apps/server/src/__tests__/owner-lifecycle.test.ts @@ -0,0 +1,186 @@ +/* eslint-disable @typescript-eslint/no-explicit-any */ +import { describe, it, beforeEach, afterEach, mock } from "node:test" +import assert from "node:assert" +import { GlideClient, ClosingError } from "@valkey/valkey-glide" +import { VALKEY } from "valkey-common" +import { allOwnerStates, getEnvOwnerState } from "../owner" +import { createSocketBinding, resolveAction, unbindConnection } from "../socket-binding" +import { connectPending, closeConnection } from "../actions/connection" +import { topologyDiscoveryEndpointPending } from "../actions/topology" +import { connectedNodesFor } from "../socket-binding" +import { teardownConnection } from "../connection" +import { _resetAuthLimiter } from "../auth-limiter" +import { _reset as resetNodeWatchers } from "../node-watchers" +import type { Handler } from "../actions/utils" + +/** + * Owner state lifecycle: an owner entry exists only while Valkey has accepted + * that identity and the identity still has something on the server. A request + * with credentials that Valkey rejects, or a discovery, must not add an entry. + */ + +const DETAILS = { + host: "127.0.0.1", + port: "6379", + tls: false, + verifyTlsCertificate: false, + endpointType: "node", + db: 0, +} + +const wrongPass = () => + new ClosingError("Connection error: WRONGPASS: invalid username-password pair or user is disabled.") + +const standaloneMock = () => ({ + info: mock.fn(async () => "cluster_enabled:0"), + customCommand: mock.fn(async () => []), + configGet: mock.fn(async () => ({ databases: "16" })), + close: mock.fn(), +}) + +// Same steps as the dispatch in index.ts: resolve the owner, then run the handler. +const dispatch = async (ws: any, binding: ReturnType, handler: Handler, action: any) => { + const resolved = resolveAction(binding, action) + if (!resolved.ok) return resolved + await handler({ + ws, + clients: resolved.state?.clients ?? new Map(), + connectionId: action.payload.connectionId, + metricsServerMap: resolved.state?.metricsServerMap ?? new Map(), + connectedNodesByCluster: connectedNodesFor(binding, resolved.ownerKey), + clusterNodesRegistry: resolved.state?.clusterNodesRegistry ?? new Map(), + ownerKey: resolved.ownerKey, + ownerState: resolved.state, + binding, + } as any)(action) + return resolved +} + +describe("owner state lifecycle", () => { + let ws: any + let sent: any[] + let original: typeof GlideClient.createClient + + beforeEach(() => { + _resetAuthLimiter() + resetNodeWatchers() + sent = [] + ws = { send: (m: string) => sent.push(JSON.parse(m)), readyState: 1, OPEN: 1 } + original = GlideClient.createClient + mock.method(console, "error", () => {}) + mock.method(console, "warn", () => {}) + mock.method(console, "log", () => {}) + }) + + afterEach(() => { + GlideClient.createClient = original + mock.restoreAll() + }) + + it("adds no owner entry for connects that Valkey rejects", async () => { + GlideClient.createClient = (async () => { throw wrongPass() }) as any + const before = allOwnerStates().length + const binding = createSocketBinding() + for (let i = 0; i < 200; i++) { + await dispatch(ws, binding, connectPending, { + type: VALKEY.CONNECTION.connectPending, + payload: { connectionId: `c-${i}`, connectionDetails: { ...DETAILS, username: `ghost-${i}`, password: "wrong" } }, + }) + } + assert.strictEqual(allOwnerStates().length, before) + assert.ok(sent.every((m) => m.type === VALKEY.CONNECTION.connectRejected)) + }) + + it("adds no owner entry for topology discovery", async () => { + GlideClient.createClient = (async () => { throw wrongPass() }) as any + const before = allOwnerStates().length + const binding = createSocketBinding() + for (let i = 0; i < 200; i++) { + await dispatch(ws, binding, topologyDiscoveryEndpointPending, { + type: VALKEY.TOPOLOGY.discoveryEndpointPending, + payload: { discoveryId: `d-${i}`, connectionDetails: { ...DETAILS, username: `seeker-${i}`, password: "wrong" } }, + }) + } + assert.strictEqual(allOwnerStates().length, before) + }) + + it("adds no owner entry for a resume that asks for the password", async () => { + const before = allOwnerStates().length + await dispatch(ws, createSocketBinding(), connectPending, { + type: VALKEY.CONNECTION.connectPending, + payload: { connectionId: "r", isResume: true, connectionDetails: { ...DETAILS, username: "resumer" } }, + }) + assert.strictEqual(allOwnerStates().length, before) + assert.strictEqual(sent.at(-1).payload.requiresAuth, true) + }) + + it("creates the entry after Valkey accepts the credentials, and removes it after the last connection closes", async () => { + GlideClient.createClient = (async () => standaloneMock()) as any + const ownerKey = "password~lifecycle-user" + const binding = createSocketBinding() + const details = { ...DETAILS, username: "lifecycle-user", password: "pw" } + + await dispatch(ws, binding, connectPending, { + type: VALKEY.CONNECTION.connectPending, + payload: { connectionId: "127-0-0-1-6379-db0", connectionDetails: details }, + }) + const state = allOwnerStates().find((s) => s.ownerKey === ownerKey) + assert.ok(state, "an accepted connect creates the owner entry") + assert.ok(state.clients.has("127-0-0-1-6379-db0")) + // No collector process in this test; the spawn is not under test here. + state.metricsServerMap.clear() + state.collectorKeys.clear() + + await dispatch(ws, binding, closeConnection, { + type: VALKEY.CONNECTION.closeConnection, + payload: { connectionId: "127-0-0-1-6379-db0" }, + }) + assert.strictEqual( + allOwnerStates().some((s) => s.ownerKey === ownerKey), + false, + "the entry is removed when the owner has nothing left", + ) + }) + + it("keeps the entry while the owner still has a client", async () => { + GlideClient.createClient = (async () => standaloneMock()) as any + const ownerKey = "password~two-dbs" + const binding = createSocketBinding() + const details = { ...DETAILS, username: "two-dbs", password: "pw" } + for (const db of [0, 1]) { + await dispatch(ws, binding, connectPending, { + type: VALKEY.CONNECTION.connectPending, + payload: { connectionId: `127-0-0-1-6379-db${db}`, connectionDetails: { ...details, db } }, + }) + } + const state = allOwnerStates().find((s) => s.ownerKey === ownerKey)! + state.metricsServerMap.clear() + state.collectorKeys.clear() + + unbindConnection(binding, "127-0-0-1-6379-db0") + teardownConnection(state, "127-0-0-1-6379-db0") + assert.ok(allOwnerStates().includes(state), "the entry stays while db1 is open") + + teardownConnection(state, "127-0-0-1-6379-db1") + assert.strictEqual(allOwnerStates().includes(state), false) + }) + + it("rejects an action on a binding whose owner entry is gone, and does not recreate the entry", () => { + const binding = createSocketBinding() + binding.connections.set("stale-db0", "password~gone-owner") + const before = allOwnerStates().length + const result = resolveAction(binding, { + type: VALKEY.COMMAND.sendRequested, + payload: { connectionId: "stale-db0" }, + }) + assert.strictEqual(result.ok, false) + assert.strictEqual(allOwnerStates().length, before) + }) + + it("never removes the env owner", () => { + const env = getEnvOwnerState() + env.clients.set("env-only-db0", { client: { close: mock.fn() } as any }) + teardownConnection(env, "env-only-db0") + assert.ok(allOwnerStates().includes(env)) + }) +}) diff --git a/apps/server/src/actions/cluster.ts b/apps/server/src/actions/cluster.ts index afa5e2e4..76dec7f6 100644 --- a/apps/server/src/actions/cluster.ts +++ b/apps/server/src/actions/cluster.ts @@ -6,7 +6,7 @@ export const setClusterData = withDeps( async ({ ws, clients, connectionId, action, ownerState }) => { const connection = clients.get(connectionId) - if (connection && connection.client instanceof GlideClusterClient) { + if (ownerState && connection && connection.client instanceof GlideClusterClient) { const { clusterId } = action.payload await setClusterDashboardData(clusterId as string, connection.client, ws, connectionId, ownerState) } diff --git a/apps/server/src/actions/connection.ts b/apps/server/src/actions/connection.ts index 475cc544..44e21482 100644 --- a/apps/server/src/actions/connection.ts +++ b/apps/server/src/actions/connection.ts @@ -1,10 +1,10 @@ import { GlideClusterClient } from "@valkey/valkey-glide" import { EndpointType } from "valkey-common" import { VALKEY } from "valkey-common" -import { connectToValkey, teardownConnection } from "../connection" +import { connectToValkey, proveCredentials, teardownConnection } from "../connection" import { unsubscribe, getWatcherCount, scopedId } from "../node-watchers" import { bindConnection, connectedNodesFor, scheduleRelease, unbindConnection } from "../socket-binding" -import { getOwnerState } from "../owner" +import { findOwnerState, getOwnerState, removeOwnerStateIfUnused } from "../owner" import { type Deps, withDeps } from "./utils" import { setClusterDashboardData } from "../set-dashboard-data" @@ -38,7 +38,7 @@ const isPasswordAuth = (details: ConnectionDetails) => details.authType !== "iam" && details.authType !== "gcp-iam" export const connectPending = withDeps( - async ({ ws, action, ownerKey, ownerState, binding }) => { + async ({ ws, action, ownerKey, binding }) => { const payload = action.payload as ConnectPayload const { connectionId, connectionDetails } = payload @@ -52,20 +52,39 @@ export const connectPending = withDeps( return } + // Prove the credentials before anything is created for the owner, so a + // request that Valkey rejects leaves no owner state behind. + try { + await proveCredentials(connectionDetails) + } catch (err) { + ws.send(JSON.stringify({ + type: VALKEY.CONNECTION.connectRejected, + payload: { connectionId, errorMessage: err instanceof Error ? err.message : String(err) }, + })) + return + } + + const ownerState = getOwnerState(ownerKey) const client = await connectToValkey( { ...ownerState, connectedNodesByCluster: connectedNodesFor(binding, ownerKey) }, ws, payload, + { credentialsProven: true }, ) - if (!client) return + if (!client) { + // The connect failed after AUTH (for example a bad database index). + removeOwnerStateIfUnused(ownerState) + return + } // Valkey accepted the credentials: the socket may now act on this connection. const previousOwner = bindConnection(binding, ownerKey, connectionId) if (previousOwner !== undefined) { // The socket reconnected the same connectionId as another Valkey user. // Release the old owner's watch so its client can be torn down. - if (unsubscribe(scopedId(previousOwner, connectionId), ws)) { - scheduleRelease(getOwnerState(previousOwner), connectionId) + const previousState = findOwnerState(previousOwner) + if (unsubscribe(scopedId(previousOwner, connectionId), ws) && previousState) { + scheduleRelease(previousState, connectionId) } } }, @@ -83,7 +102,7 @@ export const resetConnection = withDeps( const { clusterId } = action.payload as unknown as { clusterId: string } - if (client instanceof GlideClusterClient) { + if (ownerState && client instanceof GlideClusterClient) { await setClusterDashboardData(clusterId, client, ws, connectionId, ownerState) } }, @@ -115,6 +134,6 @@ export const closeConnection = withDeps( // Don't tear down a connection another socket of the same owner still uses if (getWatcherCount(scopedId(ownerKey, connectionId)) > 0) return - teardownConnection(ownerState, connectionId) + if (ownerState) teardownConnection(ownerState, connectionId) }, ) diff --git a/apps/server/src/actions/utils.ts b/apps/server/src/actions/utils.ts index 01bc2a5c..dd845e6e 100644 --- a/apps/server/src/actions/utils.ts +++ b/apps/server/src/actions/utils.ts @@ -15,7 +15,11 @@ export type Deps = { clusterNodesRegistry: Map, /** Owner identity the action runs as. The maps above are this owner's. */ ownerKey: string, - ownerState: OwnerState, + /** + * The owner's state. Undefined only for a connect or a discovery whose owner + * has no entry yet; every other action is rejected before it reaches a handler. + */ + ownerState: OwnerState | undefined, /** What the requesting socket may act on. */ binding: SocketBinding, } diff --git a/apps/server/src/connection.ts b/apps/server/src/connection.ts index 46f96a7b..d0d54fcf 100644 --- a/apps/server/src/connection.ts +++ b/apps/server/src/connection.ts @@ -28,7 +28,7 @@ import { scopedId, subscribe } from "./node-watchers" import { clearCpuSamples } from "./node-utilization" import { createAuthProbeClient, createClusterValkeyClient, createStandaloneValkeyClient } from "./valkey-client" import { withAuthLimit } from "./auth-limiter" -import { type OwnerState } from "./owner" +import { removeOwnerStateIfUnused, type OwnerState } from "./owner" /** * One owner's state plus the requesting socket's cluster bookkeeping. All @@ -122,6 +122,36 @@ class ConnectionRejectedError extends Error { } } +/** Glide credentials for `details`. Mints a token for GCP IAM. */ +async function buildCredentials(details: ConnectionDetails): Promise { + const { username, password, tls, verifyTlsCertificate, authType, awsRegion, awsReplicationGroupId } = details + if (authType === "iam") { + return { + username: username!, + iamConfig: { clusterName: awsReplicationGroupId!, service: ServiceType.Elasticache, region: awsRegion! }, + } + } + if (authType === "gcp-iam") return { username: "default", password: await mintGcpAccessToken(tls, verifyTlsCertificate) } + return password ? { username, password } : undefined +} + +/** + * Prove that Valkey accepts `details`' credentials on the node they name. + * Throws when Valkey rejects them (or the limit is reached). Call it before + * anything is created for the owner, so a rejected request leaves nothing behind. + */ +export async function proveCredentials(details: ConnectionDetails): Promise { + const credentials = await buildCredentials(details) + await verifyCredentials(details, { + addresses: [{ host: details.host, port: Number(details.port) }], + credentials, + useTLS: details.tls, + verifyTlsCertificate: details.verifyTlsCertificate, + // VALKEY_CA_CERT_PATH is a server-side default; an empty value counts as missing. + caCertPath: details.caCertPath || process.env.VALKEY_CA_CERT_PATH, + }) +} + /** * Prove that Valkey accepts `details`' credentials on this node: open a * short-lived client, then close it. Throws on rejection. Counted by the @@ -148,6 +178,7 @@ export async function connectToValkey( connectionId: string isRetry?: boolean }, + options: { credentialsProven?: boolean } = {}, ) { // Per-connectionId mutex (per owner) around the entire connectToValkey body. The // second connector enters only after the first has fully committed (or failed), @@ -159,7 +190,7 @@ export async function connectToValkey( const priorConnect = connectInFlight.get(connectionId) ?? Promise.resolve() // .catch swallows the prior connect's failure so our turn still runs. const currentConnect = priorConnect.catch(() => {}).then(() => - connectToValkeyLocked(ctx, ws, payload), + connectToValkeyLocked(ctx, ws, payload, options), ) connectInFlight.set(connectionId, currentConnect) try { @@ -179,13 +210,11 @@ async function connectToValkeyLocked( connectionId: string isRetry?: boolean }, + options: { credentialsProven?: boolean }, ) { const { clients, clusterNodesRegistry, metricsServerMap, clusterCredentials, inFlightClusterClients } = ctx - const { - host, port, username, password, tls: useTLS, - verifyTlsCertificate, authType, awsRegion, awsReplicationGroupId, - } = payload.connectionDetails + const { host, port, tls: useTLS, verifyTlsCertificate, authType } = payload.connectionDetails // VALKEY_CA_CERT_PATH is a server-side default: the UI has no CA field, so a // UI-initiated connection can only get a private CA from the environment. // An empty persisted value counts as missing, hence `||` not `??`. @@ -203,19 +232,7 @@ async function connectToValkeyLocked( let standaloneClient: GlideClient | undefined try { - const credentials: ServerCredentials | undefined = - authType === "iam" - ? { - username: username!, - iamConfig: { - clusterName: awsReplicationGroupId!, - service: ServiceType.Elasticache, - region: awsRegion!, - }, - } - : authType === "gcp-iam" - ? { username: "default", password: await mintGcpAccessToken(useTLS, verifyTlsCertificate) } - : password ? { username, password } : undefined + const credentials = await buildCredentials(payload.connectionDetails) if (!isValidDatabaseIndex(db)) { throw new ConnectionRejectedError( @@ -226,9 +243,13 @@ async function connectToValkeyLocked( // Valkey must accept the presented credentials before anything else: before // an existing client is handed over (the owner identity is not secret, so it // never grants reuse by itself) and before a retry closes the stale client. - await verifyCredentials(payload.connectionDetails, { - addresses, credentials, useTLS, verifyTlsCertificate, caCertPath, - }) + // The caller may already have proven them (connectPending does, before it + // creates the owner's state). + if (!options.credentialsProven) { + await verifyCredentials(payload.connectionDetails, { + addresses, credentials, useTLS, verifyTlsCertificate, caCertPath, + }) + } // A retry replaces the stale client only after the new client exists, so a // failed retry cannot close a client that works. Cluster clients are @@ -508,10 +529,7 @@ export async function discoverTopology( payload: { discoveryId: string; connectionDetails: ConnectionDetails }, ): Promise { const { discoveryId, connectionDetails } = payload - const { - host, port, username, password, tls: useTLS, - verifyTlsCertificate, authType, awsRegion, awsReplicationGroupId, - } = connectionDetails + const { host, port, tls: useTLS, verifyTlsCertificate } = connectionDetails // VALKEY_CA_CERT_PATH is a server-side default: the UI has no CA field, so a // UI-initiated discovery can only get a private CA from the environment. // An empty persisted value counts as missing, hence `||` not `??`. @@ -521,19 +539,7 @@ export async function discoverTopology( let client: GlideClient | undefined try { - const credentials: ServerCredentials | undefined = - authType === "iam" - ? { - username: username!, - iamConfig: { - clusterName: awsReplicationGroupId!, - service: ServiceType.Elasticache, - region: awsRegion!, - }, - } - : authType === "gcp-iam" - ? { username: "default", password: await mintGcpAccessToken(useTLS, verifyTlsCertificate) } - : password ? { username, password } : undefined + const credentials = await buildCredentials(connectionDetails) // Cluster discovery is read-only `CLUSTER SLOTS` against the seed node; // database selection is irrelevant for cluster commands. Skip @@ -825,8 +831,9 @@ export function teardownConnection( } if (!env) { - pruneOwnerCollectors(ctx).catch((err) => - console.error(`Error stopping collectors after ${connectionId} closed:`, err), - ) + pruneOwnerCollectors(ctx) + .then(() => removeOwnerStateIfUnused(ctx)) + .catch((err) => console.error(`Error stopping collectors after ${connectionId} closed:`, err)) + removeOwnerStateIfUnused(ctx) } } diff --git a/apps/server/src/index.ts b/apps/server/src/index.ts index 961d37b2..9344318a 100644 --- a/apps/server/src/index.ts +++ b/apps/server/src/index.ts @@ -44,7 +44,7 @@ import { setPreconfiguredClusterId } from "./metrics-orchestrator" import { isAllowedWebSocketOrigin } from "./websocket-origin" -import { allOwnerStates, getOwnerState } from "./owner" +import { allOwnerStates, findOwnerState } from "./owner" import { connectedNodesFor, createSocketBinding, @@ -341,13 +341,15 @@ wss.on("connection", (ws: AliveWebSocket) => { } const { ownerKey, state } = resolved + // `state` is undefined only for a connect or discovery of an owner that has + // no entry yet. Those handlers do not read the maps below. await handler( { ws, - clients: state.clients, + clients: state?.clients ?? new Map(), connectionId: connectionId!, - metricsServerMap: state.metricsServerMap, + metricsServerMap: state?.metricsServerMap ?? new Map(), connectedNodesByCluster: connectedNodesFor(ws.binding, ownerKey), - clusterNodesRegistry: state.clusterNodesRegistry, + clusterNodesRegistry: state?.clusterNodesRegistry ?? new Map(), ownerKey, ownerState: state, binding: ws.binding })(action as ReduxAction) @@ -366,7 +368,8 @@ wss.on("connection", (ws: AliveWebSocket) => { // Release every connection this socket bound. Each one is torn down after // the teardown delay unless another socket of the same owner watches it. for (const [connectionId, ownerKey] of ws.binding.connections) { - scheduleRelease(getOwnerState(ownerKey), connectionId) + const state = findOwnerState(ownerKey) + if (state) scheduleRelease(state, connectionId) } ws.binding = createSocketBinding() }) diff --git a/apps/server/src/owner.ts b/apps/server/src/owner.ts index 2bec210c..91844e8f 100644 --- a/apps/server/src/owner.ts +++ b/apps/server/src/owner.ts @@ -98,6 +98,17 @@ export const registerEnvOwner = (ownerKey: string): void => { owners.set(ownerKey, envState) } +/** + * The owner's state if it exists. Does not create one: use this for any + * request whose credentials Valkey has not accepted yet. + */ +export const findOwnerState = (ownerKey: string): OwnerState | undefined => + owners.get(ownerKey) ?? (envState.ownerKey === ownerKey ? envState : undefined) + +/** + * The owner's state, created on first use. Call it only after Valkey accepted + * the owner's credentials, so unauthenticated requests cannot add entries. + */ export const getOwnerState = (ownerKey: string): OwnerState => { let state = owners.get(ownerKey) if (!state) { @@ -107,6 +118,22 @@ export const getOwnerState = (ownerKey: string): OwnerState => { return state } +/** + * Remove an owner's entry when the owner has nothing left on the server: no + * client, no collector, no cluster topology and no connect in progress. The env + * owner is never removed. + */ +export const removeOwnerStateIfUnused = (state: OwnerState): boolean => { + if (state.env || owners.get(state.ownerKey) !== state) return false + const unused = state.clients.size === 0 + && state.metricsServerMap.size === 0 + && state.clusterNodesRegistry.size === 0 + && state.connectInFlight.size === 0 + && state.inFlightClusterClients.size === 0 + if (unused) owners.delete(state.ownerKey) + return unused +} + /** Every owner state, the env owner's included. */ export const allOwnerStates = (): OwnerState[] => { const states = [...owners.values()] diff --git a/apps/server/src/socket-binding.ts b/apps/server/src/socket-binding.ts index 4936991d..78a3d99f 100644 --- a/apps/server/src/socket-binding.ts +++ b/apps/server/src/socket-binding.ts @@ -1,6 +1,6 @@ import { VALKEY, toNodeId } from "valkey-common" import { CONNECTION_TEARDOWN_DELAY_MS } from "valkey-common" -import { getOwnerState, ownerOf, type OwnerState } from "./owner" +import { findOwnerState, ownerOf, type OwnerState } from "./owner" import { getWatcherCount, scopedId } from "./node-watchers" import { teardownConnection } from "./connection" import type { ConnectionDetails } from "./actions/connection" @@ -55,7 +55,11 @@ export const connectedNodesFor = (binding: SocketBinding, ownerKey: string): Map type ActionLike = { type: string; payload?: { connectionId?: string; clusterId?: unknown; connectionDetails?: unknown } } export type ResolvedAction = - | { ok: true; ownerKey: string; state: OwnerState } + /** + * `state` is undefined only for a connect or a discovery whose owner has no + * entry yet. A connect creates the entry after Valkey accepts the credentials. + */ + | { ok: true; ownerKey: string; state: OwnerState | undefined } | { ok: false; reason: string } // The cluster that a bound connection (or a db-less node id) belongs to in its owner's state. @@ -80,7 +84,7 @@ const clusterOf = (state: OwnerState, connectionId: string): string | undefined export const resolveAction = ( binding: SocketBinding, action: ActionLike, - lookup: (ownerKey: string) => OwnerState | undefined = getOwnerState, + lookup: (ownerKey: string) => OwnerState | undefined = findOwnerState, ): ResolvedAction => { const payload = action.payload ?? {} @@ -88,7 +92,8 @@ export const resolveAction = ( const details = payload.connectionDetails as ConnectionDetails | undefined if (!details || typeof details !== "object") return { ok: false, reason: "missing connection details" } const ownerKey = ownerOf(details) - return { ok: true, ownerKey, state: getOwnerState(ownerKey) } + // Look up only: Valkey has not accepted these credentials yet. + return { ok: true, ownerKey, state: findOwnerState(ownerKey) } } const { connectionId } = payload From a19427ccbb05293a8dd930b7963548d3815e912e Mon Sep 17 00:00:00 2001 From: Argus Li Date: Fri, 23 Oct 2026 14:01:10 -0700 Subject: [PATCH 9/9] Fix connection case if socket closed while connect ran. Signed-off-by: Argus Li --- .../src/__tests__/connect-close-race.test.ts | 173 ++++++++++++++++++ apps/server/src/actions/connection.ts | 7 + 2 files changed, 180 insertions(+) create mode 100644 apps/server/src/__tests__/connect-close-race.test.ts diff --git a/apps/server/src/__tests__/connect-close-race.test.ts b/apps/server/src/__tests__/connect-close-race.test.ts new file mode 100644 index 00000000..16d58c5f --- /dev/null +++ b/apps/server/src/__tests__/connect-close-race.test.ts @@ -0,0 +1,173 @@ +/* eslint-disable @typescript-eslint/no-explicit-any */ +import { describe, it, beforeEach, afterEach, mock } from "node:test" +import assert from "node:assert" +import { GlideClient } from "@valkey/valkey-glide" +import { CONNECTION_TEARDOWN_DELAY_MS, VALKEY } from "valkey-common" +import { connectedNodesFor, createSocketBinding, resolveAction, type SocketBinding } from "../socket-binding" +import { connectPending } from "../actions/connection" +import { allOwnerStates } from "../owner" +import { getWatcherCount, scopedId, subscribe, unsubscribe, unsubscribeAll, _reset as resetNodeWatchers } from "../node-watchers" +import { __test__ } from "../metrics-orchestrator" +import { _resetAuthLimiter } from "../auth-limiter" + +/** + * A browser socket can close while one of its connects is still running. The + * close handler releases only the connections bound on the socket, and a + * connect is bound only after it finishes. The connect must then release + * what it created, or the client and its collector stay until restart. + */ + +const CONNECTION_ID = "127-0-0-1-6379-db0" + +const clientMock = () => ({ + info: async () => "cluster_enabled:0", + customCommand: async () => [], + configGet: async () => ({ databases: "16" }), + close: mock.fn(), +}) + +const OPEN = 1 +const CLOSED = 3 +const makeWs = (): any => ({ send: () => {}, readyState: OPEN, OPEN }) + +// The same steps as ws.on("close") in index.ts, for a socket with no bound connections yet. +const closeSocket = (ws: any) => { + ws.readyState = CLOSED + unsubscribeAll(ws) +} + +const startConnect = (ws: any, binding: SocketBinding, username: string) => { + const action: any = { + type: VALKEY.CONNECTION.connectPending, + payload: { + connectionId: CONNECTION_ID, + connectionDetails: { + host: "127.0.0.1", port: "6379", username, password: "pw", + tls: false, verifyTlsCertificate: false, endpointType: "node", db: 0, + }, + }, + } + const resolved = resolveAction(binding, action) as any + return connectPending({ + ws, ownerKey: resolved.ownerKey, ownerState: resolved.state, binding, + connectedNodesByCluster: connectedNodesFor(binding, resolved.ownerKey), + } as any)(action) +} + +const findState = (username: string) => allOwnerStates().find((s) => s.ownerKey === `password~${username}`) + +// Let pending promise callbacks run without moving the mocked clock. +const flush = async () => { for (let i = 0; i < 20; i++) await Promise.resolve() } + +describe("socket closes while a connect is running", () => { + let original: typeof GlideClient.createClient + let killed: number[] + // `gate(n)` holds the n-th createClient call (1 = AUTH probe, 2 = working client) until released. + let held: Map void> + let calls: number + let client: ReturnType + + const gate = (n: number) => { + let release!: () => void + const wait = new Promise((r) => { release = r }) + held.set(n, release) + return wait + } + + beforeEach(() => { + _resetAuthLimiter() + resetNodeWatchers() + killed = [] + held = new Map() + calls = 0 + client = clientMock() + original = GlideClient.createClient + for (const m of ["error", "warn", "log", "debug"] as const) mock.method(console, m, () => {}) + mock.method(process, "kill", (pid: number) => { killed.push(pid); return true }) + mock.method(__test__, "spawnProcess", () => ({ pid: 4242, stderr: null, on: () => undefined })) + mock.timers.enable({ apis: ["setTimeout"] }) + }) + + afterEach(() => { + GlideClient.createClient = original + mock.timers.reset() + mock.restoreAll() + }) + + const holdCall = (n: number) => { + const waits = new Map>([[n, gate(n)]]) + GlideClient.createClient = (async () => { + calls++ + const wait = waits.get(calls) + if (wait) await wait + return calls === 1 ? { close: mock.fn() } : client + }) as any + } + + const assertReleased = (username: string) => { + mock.timers.tick(CONNECTION_TEARDOWN_DELAY_MS) + assert.strictEqual(getWatcherCount(scopedId(`password~${username}`, CONNECTION_ID)), 0, "no watcher remains") + assert.strictEqual(client.close.mock.calls.length, 1, "the client is closed") + assert.strictEqual(findState(username)?.clients.has(CONNECTION_ID) ?? false, false, "the client is removed") + assert.ok(killed.includes(4242), "the collector is stopped") + } + + it("releases the connection when the socket closes during client creation", async () => { + holdCall(2) + const ws = makeWs() + const connecting = startConnect(ws, createSocketBinding(), "close-create") + await flush() + closeSocket(ws) + held.get(2)!() + await connecting + await flush() + assertReleased("close-create") + await flush() + assert.strictEqual(findState("close-create"), undefined, "the owner entry is removed") + }) + + it("releases the connection when the socket closes during the AUTH probe", async () => { + holdCall(1) + const ws = makeWs() + const connecting = startConnect(ws, createSocketBinding(), "close-probe") + await flush() + closeSocket(ws) + held.get(1)!() + await connecting + await flush() + assertReleased("close-probe") + }) + + it("keeps the connection when another socket of the same owner still watches it", async () => { + holdCall(2) + const ws = makeWs() + const other = makeWs() + subscribe(scopedId("password~shared-owner", CONNECTION_ID), other) + const connecting = startConnect(ws, createSocketBinding(), "shared-owner") + await flush() + closeSocket(ws) + held.get(2)!() + await connecting + await flush() + mock.timers.tick(CONNECTION_TEARDOWN_DELAY_MS) + assert.strictEqual(client.close.mock.calls.length, 0, "the other socket still uses the client") + assert.ok(findState("shared-owner")?.clients.has(CONNECTION_ID)) + unsubscribe(scopedId("password~shared-owner", CONNECTION_ID), other) + findState("shared-owner")!.metricsServerMap.clear() + }) + + it("binds the connection as before when the socket stays open", async () => { + holdCall(2) + const ws = makeWs() + const binding = createSocketBinding() + const connecting = startConnect(ws, binding, "stays-open") + await flush() + held.get(2)!() + await connecting + mock.timers.tick(CONNECTION_TEARDOWN_DELAY_MS) + assert.strictEqual(binding.connections.get(CONNECTION_ID), "password~stays-open") + assert.strictEqual(getWatcherCount(scopedId("password~stays-open", CONNECTION_ID)), 1) + assert.strictEqual(client.close.mock.calls.length, 0) + findState("stays-open")!.metricsServerMap.clear() + }) +}) diff --git a/apps/server/src/actions/connection.ts b/apps/server/src/actions/connection.ts index 44e21482..a32325b3 100644 --- a/apps/server/src/actions/connection.ts +++ b/apps/server/src/actions/connection.ts @@ -77,6 +77,13 @@ export const connectPending = withDeps( return } + // Case if the socket closed while the connect ran. + if (ws.readyState !== ws.OPEN) { + unsubscribe(scopedId(ownerKey, connectionId), ws) + scheduleRelease(ownerState, connectionId) + return + } + // Valkey accepted the credentials: the socket may now act on this connection. const previousOwner = bindConnection(binding, ownerKey, connectionId) if (previousOwner !== undefined) {