[spicedb] fix stale connection handling

svenefftinge · svenefftinge · commit 1bc6b86c68fe · 2023-08-27T17:34:25.000Z
diff --git a/components/server/src/authorization/spicedb-authorizer.ts b/components/server/src/authorization/spicedb-authorizer.ts
@@ -21,6 +21,10 @@ export class SpiceDBAuthorizer {
         private readonly clientProvider: SpiceDBClientProvider,
     ) {}
 
+    private get client(): v1.ZedPromiseClientInterface {
+        return this.clientProvider.getClient();
+    }
+
     async check(
         req: v1.CheckPermissionRequest,
         experimentsFields: {
@@ -39,8 +43,7 @@ export class SpiceDBAuthorizer {
         const timer = spicedbClientLatency.startTimer();
         let error: Error | undefined;
         try {
-            const client = this.clientProvider.getClient();
-            const response = await client.checkPermission(req, this.callOptions);
+            const response = await this.client.checkPermission(req, this.callOptions);
             const permitted = response.permissionship === v1.CheckPermissionResponse_Permissionship.HAS_PERMISSION;
 
             return permitted;
@@ -59,8 +62,7 @@ export class SpiceDBAuthorizer {
         const timer = spicedbClientLatency.startTimer();
         let error: Error | undefined;
         try {
-            const client = this.clientProvider.getClient();
-            const response = await client.writeRelationships(
+            const response = await this.client.writeRelationships(
                 v1.WriteRelationshipsRequest.create({
                     updates,
                 }),
@@ -81,11 +83,16 @@ export class SpiceDBAuthorizer {
         const timer = spicedbClientLatency.startTimer();
         let error: Error | undefined;
         try {
-            const client = this.clientProvider.getClient();
-            const existing = await client.readRelationships(v1.ReadRelationshipsRequest.create(req), this.callOptions);
+            const existing = await this.client.readRelationships(
+                v1.ReadRelationshipsRequest.create(req),
+                this.callOptions,
+            );
             if (existing.length > 0) {
-                const response = await client.deleteRelationships(req, this.callOptions);
-                const after = await client.readRelationships(v1.ReadRelationshipsRequest.create(req), this.callOptions);
+                const response = await this.client.deleteRelationships(req, this.callOptions);
+                const after = await this.client.readRelationships(
+                    v1.ReadRelationshipsRequest.create(req),
+                    this.callOptions,
+                );
                 if (after.length > 0) {
                     log.error("[spicedb] Failed to delete relationships.", { existing, after, request: req });
                 }
@@ -108,11 +115,7 @@ export class SpiceDBAuthorizer {
     }
 
     async readRelationships(req: v1.ReadRelationshipsRequest): Promise<v1.ReadRelationshipsResponse[]> {
-        const client = this.clientProvider.getClient();
-        if (!client) {
-            return [];
-        }
-        return client.readRelationships(req, this.callOptions);
+        return this.client.readRelationships(req, this.callOptions);
     }
 
     /**
diff --git a/components/server/src/authorization/spicedb.ts b/components/server/src/authorization/spicedb.ts
@@ -5,22 +5,17 @@
  */
 
 import { v1 } from "@authzed/authzed-node";
-import { IClientCallMetrics, createClientCallMetricsInterceptor } from "@gitpod/gitpod-protocol/lib/util/grpc";
+import { defaultGRPCOptions } from "@gitpod/gitpod-protocol/lib/util/grpc";
 import { log } from "@gitpod/gitpod-protocol/lib/util/logging";
 import * as grpc from "@grpc/grpc-js";
 
-export const SpiceDBClientProvider = Symbol("SpiceDBClientProvider");
-
 export interface SpiceDBClientConfig {
     address: string;
     token: string;
 }
 
 export type SpiceDBClient = v1.ZedPromiseClientInterface;
 type Client = v1.ZedClientInterface & grpc.Client;
-export interface SpiceDBClientProvider {
-    getClient(): SpiceDBClient;
-}
 
 export function spiceDBConfigFromEnv(): SpiceDBClientConfig | undefined {
     const token = process.env["SPICEDB_PRESHARED_KEY"];
@@ -40,63 +35,41 @@ export function spiceDBConfigFromEnv(): SpiceDBClientConfig | undefined {
     };
 }
 
-function spicedbClientFromConfig(config: SpiceDBClientConfig): Client {
-    const clientOptions: grpc.ClientOptions = {
-        "grpc.client_idle_timeout_ms": 10000, // this ensures a connection is not stuck in the "READY" state for too long. Required for the reconnect logic below.
-        "grpc.max_reconnect_backoff_ms": 5000, // default: 12000
-    };
-
-    return v1.NewClient(
-        config.token,
-        config.address,
-        v1.ClientSecurity.INSECURE_PLAINTEXT_CREDENTIALS,
-        undefined,
-        clientOptions,
-    ) as Client;
-}
-
-export class CachingSpiceDBClientProvider implements SpiceDBClientProvider {
+export class SpiceDBClientProvider {
     private client: Client | undefined;
 
-    private readonly interceptors: grpc.Interceptor[] = [];
-
     constructor(
-        private readonly _clientConfig: SpiceDBClientConfig,
-        private readonly clientCallMetrics?: IClientCallMetrics | undefined,
-    ) {
-        if (this.clientCallMetrics) {
-            this.interceptors.push(createClientCallMetricsInterceptor(this.clientCallMetrics));
-        }
-    }
+        private readonly clientConfig: SpiceDBClientConfig,
+        private readonly interceptors: grpc.Interceptor[] = [],
+    ) {}
 
     getClient(): SpiceDBClient {
-        let client = this.client;
-        if (!client) {
-            client = spicedbClientFromConfig(this.clientConfig);
-        } else if (client.getChannel().getConnectivityState(true) !== grpc.connectivityState.READY) {
-            // (gpl): We need this check and explicit re-connect because we observe a ~120s connection timeout without it.
-            // It's not entirely clear where that timeout comes from, but likely from the underlying transport, that is not exposed by grpc/grpc-js
-            client.close();
-
-            log.warn("[spicedb] Lost connection to SpiceDB - reconnecting...");
-
-            client = spicedbClientFromConfig(this.clientConfig);
+        if (this.client) {
+            const state = this.client.getChannel().getConnectivityState(true);
+            if (state === grpc.connectivityState.TRANSIENT_FAILURE || state === grpc.connectivityState.SHUTDOWN) {
+                log.warn("[spicedb] Lost connection to SpiceDB - reconnecting...");
+                try {
+                    this.client.close();
+                } catch (error) {
+                    log.error("[spicedb] Failed to close client", error);
+                } finally {
+                    this.client = undefined;
+                }
+            }
         }
-        this.client = client;
-
-        return client.promises;
-    }
 
-    protected get clientConfig() {
-        const config = this._clientConfig;
-        if (this.interceptors) {
-            return {
-                ...config,
-                options: {
-                    interceptors: [...this.interceptors],
+        if (!this.client) {
+            this.client = v1.NewClient(
+                this.clientConfig.token,
+                this.clientConfig.address,
+                v1.ClientSecurity.INSECURE_PLAINTEXT_CREDENTIALS,
+                undefined, //
+                {
+                    ...defaultGRPCOptions,
+                    interceptors: this.interceptors,
                 },
-            };
+            ) as Client;
         }
-        return config;
+        return this.client.promises;
     }
 }
diff --git a/components/server/src/container-module.ts b/components/server/src/container-module.ts
@@ -11,7 +11,11 @@ import { GitpodFileParser } from "@gitpod/gitpod-protocol/lib/gitpod-file-parser
 import { PrometheusClientCallMetrics } from "@gitpod/gitpod-protocol/lib/messaging/client-call-metrics";
 import { newAnalyticsWriterFromEnv } from "@gitpod/gitpod-protocol/lib/util/analytics";
 import { DebugApp } from "@gitpod/gitpod-protocol/lib/util/debug-app";
-import { IClientCallMetrics, defaultGRPCOptions } from "@gitpod/gitpod-protocol/lib/util/grpc";
+import {
+    IClientCallMetrics,
+    createClientCallMetricsInterceptor,
+    defaultGRPCOptions,
+} from "@gitpod/gitpod-protocol/lib/util/grpc";
 import { prometheusClientMiddleware } from "@gitpod/gitpod-protocol/lib/util/nice-grpc";
 import { IDEServiceClient, IDEServiceDefinition } from "@gitpod/ide-service-api/lib/ide.pb";
 import { ImageBuilderClientCallMetrics, ImageBuilderClientProvider } from "@gitpod/image-builder/lib";
@@ -46,7 +50,7 @@ import { AuthJWT, SignInJWT } from "./auth/jwt";
 import { LoginCompletionHandler } from "./auth/login-completion-handler";
 import { VerificationService } from "./auth/verification-service";
 import { Authorizer, createInitializingAuthorizer } from "./authorization/authorizer";
-import { CachingSpiceDBClientProvider, SpiceDBClientProvider, spiceDBConfigFromEnv } from "./authorization/spicedb";
+import { SpiceDBClientProvider, spiceDBConfigFromEnv } from "./authorization/spicedb";
 import { BillingModes } from "./billing/billing-mode";
 import { EntitlementService, EntitlementServiceImpl } from "./billing/entitlement-service";
 import { EntitlementServiceUBP } from "./billing/entitlement-service-ubp";
@@ -309,7 +313,10 @@ export const productionContainerModule = new ContainerModule(
                     throw new Error("[spicedb] Missing configuration expected in env vars!");
                 }
                 const clientCallMetrics = ctx.container.get<IClientCallMetrics>(IClientCallMetrics);
-                return new CachingSpiceDBClientProvider(config, clientCallMetrics);
+                return new SpiceDBClientProvider(
+                    config, //
+                    [createClientCallMetricsInterceptor(clientCallMetrics)],
+                );
             })
             .inSingletonScope();
         bind(SpiceDBAuthorizer).toSelf().inSingletonScope();
