Fix broken Functions CLI experience for projects with incomplete GCF …

…2nd Gen functions. (#5684) The codebase assumes that "serviceConfig" property must be present in all 2nd Gen Functions in GCF. This is an incorrect assumption - 2nd Gen GCF functions may be missing a Cloud Run service if the build never succeeded. Also refactors `backend` module a bit to avoid calling out to Cloud Run to retrieve concurrency & cpu config - it's available on the GCF response now! Fixes #4800
firebase · Apr 13, 2023 · 863eeec · 863eeec
1 parent cc4d85c
commit 863eeec
Show file tree

Hide file tree

Showing 7 changed files with 181 additions and 183 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,3 +1,4 @@
 - Default emulators:start to use fast dev-mode for Nuxt3 applications (#5551)
+- Fix broken Functions CLI experience for projects with incomplete GCF 2nd Gen functions (#5684)
 - Disable GCF breaking change to automatically run npm build scripts as part of function deploy (#5687)
 - Add experimental support for deploying Astro applications to Hosting (#5527)
diff --git a/src/deploy/functions/backend.ts b/src/deploy/functions/backend.ts
@@ -1,11 +1,10 @@
 import * as gcf from "../../gcp/cloudfunctions";
 import * as gcfV2 from "../../gcp/cloudfunctionsv2";
-import * as run from "../../gcp/run";
 import * as utils from "../../utils";
 import * as runtimes from "./runtimes";
 import { FirebaseError } from "../../error";
 import { Context } from "./args";
-import { flattenArray, zip } from "../../functional";
+import { flattenArray } from "../../functional";
 
 /** Retry settings for a ScheduleSpec. */
 export interface ScheduleRetryConfig {
@@ -532,20 +531,8 @@ async function loadExistingBackend(ctx: Context): Promise<void> {
   let gcfV2Results;
   try {
     gcfV2Results = await gcfV2.listAllFunctions(ctx.projectId);
-    const runResults = await Promise.all(
-      gcfV2Results.functions.map((fn) => run.getService(fn.serviceConfig.service!))
-    );
-    for (const [apiFunction, runService] of zip(gcfV2Results.functions, runResults)) {
-      // I don't know why but code complete knows apiFunction is a gcfv2.CloudFunction
-      // and the compiler thinks it's type {}.
-      const endpoint = gcfV2.endpointFromFunction(apiFunction as any);
-      endpoint.concurrency = runService.spec.template.spec.containerConcurrency || 1;
-      // N.B. We don't generally do anything with multiple containers, but we
-      // might have to figure out WTF to do here if we're updating multiple containers
-      // and our only reference point is the image. Hopefully by then we'll be
-      // on the next gen infrastructure and have state we can refer back to.
-      endpoint.cpu = +runService.spec.template.spec.containers[0].resources.limits.cpu;
-
+    for (const apiFunction of gcfV2Results.functions) {
+      const endpoint = gcfV2.endpointFromFunction(apiFunction);
       ctx.existingBackend.endpoints[endpoint.region] =
         ctx.existingBackend.endpoints[endpoint.region] || {};
       ctx.existingBackend.endpoints[endpoint.region][endpoint.id] = endpoint;

diff --git a/src/deploy/functions/release/fabricator.ts b/src/deploy/functions/release/fabricator.ts
@@ -347,16 +347,24 @@ export class Fabricator {
     const resultFunction = await this.functionExecutor
       .run(async () => {
         const op: { name: string } = await gcfV2.createFunction(apiFunction);
-        return await poller.pollOperation<gcfV2.CloudFunction>({
+        return await poller.pollOperation<gcfV2.OutputCloudFunction>({
           ...gcfV2PollerOptions,
           pollerName: `create-${endpoint.codebase}-${endpoint.region}-${endpoint.id}`,
           operationResourceName: op.name,
         });
       })
-      .catch(rethrowAs<gcfV2.CloudFunction>(endpoint, "create"));
-
-    endpoint.uri = resultFunction.serviceConfig.uri;
-    const serviceName = resultFunction.serviceConfig.service!;
+      .catch(rethrowAs<gcfV2.OutputCloudFunction>(endpoint, "create"));
+
+    endpoint.uri = resultFunction.serviceConfig?.uri;
+    const serviceName = resultFunction.serviceConfig?.service;
+    if (!serviceName) {
+      logger.debug("Result function unexpectedly didn't have a service name.");
+      utils.logLabeledWarning(
+        "functions",
+        "Updated function is not associated with a service. This deployment is in an unexpected state - please re-deploy your functions."
+      );
+      return;
+    }
     if (backend.isHttpsTriggered(endpoint)) {
       const invoker = endpoint.httpsTrigger.invoker || ["public"];
       if (!invoker.includes("private")) {
@@ -455,16 +463,24 @@ export class Fabricator {
     const resultFunction = await this.functionExecutor
       .run(async () => {
         const op: { name: string } = await gcfV2.updateFunction(apiFunction);
-        return await poller.pollOperation<gcfV2.CloudFunction>({
+        return await poller.pollOperation<gcfV2.OutputCloudFunction>({
           ...gcfV2PollerOptions,
           pollerName: `update-${endpoint.codebase}-${endpoint.region}-${endpoint.id}`,
           operationResourceName: op.name,
         });
       })
-      .catch(rethrowAs<gcfV2.CloudFunction>(endpoint, "update"));
-
-    endpoint.uri = resultFunction.serviceConfig.uri;
-    const serviceName = resultFunction.serviceConfig.service!;
+      .catch(rethrowAs<gcfV2.OutputCloudFunction>(endpoint, "update"));
+
+    endpoint.uri = resultFunction.serviceConfig?.uri;
+    const serviceName = resultFunction.serviceConfig?.service;
+    if (!serviceName) {
+      logger.debug("Result function unexpectedly didn't have a service name.");
+      utils.logLabeledWarning(
+        "functions",
+        "Updated function is not associated with a service. This deployment is in an unexpected state - please re-deploy your functions."
+      );
+      return;
+    }
     let invoker: string[] | undefined;
     if (backend.isHttpsTriggered(endpoint)) {
       invoker = endpoint.httpsTrigger.invoker === null ? ["public"] : endpoint.httpsTrigger.invoker;

diff --git a/src/gcp/cloudfunctionsv2.ts b/src/gcp/cloudfunctionsv2.ts
@@ -18,6 +18,7 @@ import {
   CODEBASE_LABEL,
   HASH_LABEL,
 } from "../functions/constants";
+import { RequireKeys } from "../metaprogramming";
 
 export const API_VERSION = "v2";
 
@@ -31,17 +32,6 @@ export type VpcConnectorEgressSettings = "PRIVATE_RANGES_ONLY" | "ALL_TRAFFIC";
 export type IngressSettings = "ALLOW_ALL" | "ALLOW_INTERNAL_ONLY" | "ALLOW_INTERNAL_AND_GCLB";
 export type FunctionState = "ACTIVE" | "FAILED" | "DEPLOYING" | "DELETING" | "UNKONWN";
 
-// The GCFv2 funtion type has many inner types which themselves have output-only fields:
-// eventTrigger.trigger
-// buildConfig.config
-// buildConfig.workerPool
-// serviceConfig.service
-// serviceConfig.uri
-//
-// Because Omit<> doesn't work with nested property addresses, we're making those fields optional.
-// An alternative would be to name the types OutputCloudFunction/CloudFunction or CloudFunction/InputCloudFunction.
-export type OutputOnlyFields = "state" | "updateTime";
-
 // Values allowed for the operator field in EventFilter
 export type EventFilterOperator = "match-path-pattern";
 
@@ -153,17 +143,26 @@ export interface EventTrigger {
   channel?: string;
 }
 
-export interface CloudFunction {
+interface CloudFunctionBase {
   name: string;
   description?: string;
   buildConfig: BuildConfig;
-  serviceConfig: ServiceConfig;
+  serviceConfig?: ServiceConfig;
   eventTrigger?: EventTrigger;
-  state: FunctionState;
-  updateTime: Date;
   labels?: Record<string, string> | null;
 }
 
+export type OutputCloudFunction = CloudFunctionBase & {
+  state: FunctionState;
+  updateTime: Date;
+  serviceConfig?: RequireKeys<ServiceConfig, "service" | "uri">;
+};
+
+export type InputCloudFunction = CloudFunctionBase & {
+  // serviceConfig is required.
+  serviceConfig: ServiceConfig;
+};
+
 export interface OperationMetadata {
   createTime: string;
   endTime: string;
@@ -181,13 +180,13 @@ export interface Operation {
   metadata?: OperationMetadata;
   done: boolean;
   error?: { code: number; message: string; details: unknown };
-  response?: CloudFunction;
+  response?: OutputCloudFunction;
 }
 
 // Private API interface for ListFunctionsResponse. listFunctions returns
 // a CloudFunction[]
 interface ListFunctionsResponse {
-  functions: CloudFunction[];
+  functions: OutputCloudFunction[];
   unreachable: string[];
 }
 
@@ -292,9 +291,7 @@ export async function generateUploadUrl(
 /**
  * Creates a new Cloud Function.
  */
-export async function createFunction(
-  cloudFunction: Omit<CloudFunction, OutputOnlyFields>
-): Promise<Operation> {
+export async function createFunction(cloudFunction: InputCloudFunction): Promise<Operation> {
   // the API is a POST to the collection that owns the function name.
   const components = cloudFunction.name.split("/");
   const functionId = components.splice(-1, 1)[0];
@@ -325,17 +322,20 @@ export async function getFunction(
   projectId: string,
   location: string,
   functionId: string
-): Promise<CloudFunction> {
+): Promise<OutputCloudFunction> {
   const name = `projects/${projectId}/locations/${location}/functions/${functionId}`;
-  const res = await client.get<CloudFunction>(name);
+  const res = await client.get<OutputCloudFunction>(name);
   return res.body;
 }
 
 /**
  *  List all functions in a region.
  *  Customers should generally use backend.existingBackend.
  */
-export async function listFunctions(projectId: string, region: string): Promise<CloudFunction[]> {
+export async function listFunctions(
+  projectId: string,
+  region: string
+): Promise<OutputCloudFunction[]> {
   const res = await listFunctionsInternal(projectId, region);
   if (res.unreachable.includes(region)) {
     throw new FirebaseError(`Cloud Functions region ${region} is unavailable`);
@@ -356,7 +356,7 @@ async function listFunctionsInternal(
   region: string
 ): Promise<ListFunctionsResponse> {
   type Response = ListFunctionsResponse & { nextPageToken?: string };
-  const functions: CloudFunction[] = [];
+  const functions: OutputCloudFunction[] = [];
   const unreacahble = new Set<string>();
   let pageToken = "";
   while (true) {
@@ -386,9 +386,7 @@ async function listFunctionsInternal(
  * Updates a Cloud Function.
  * Customers can force a field to be deleted by setting that field to `undefined`
  */
-export async function updateFunction(
-  cloudFunction: Omit<CloudFunction, OutputOnlyFields>
-): Promise<Operation> {
+export async function updateFunction(cloudFunction: InputCloudFunction): Promise<Operation> {
   // Keys in labels and environmentVariables and secretEnvironmentVariables are user defined, so we don't recurse
   // for field masks.
   const fieldMasks = proto.fieldMasks(
@@ -439,7 +437,7 @@ export async function deleteFunction(cloudFunction: string): Promise<Operation>
 export function functionFromEndpoint(
   endpoint: backend.Endpoint,
   source: StorageSource
-): Omit<CloudFunction, OutputOnlyFields> {
+): InputCloudFunction {
   if (endpoint.platform !== "gcfv2") {
     throw new FirebaseError(
       "Trying to create a v2 CloudFunction with v1 API. This should never happen"
@@ -453,7 +451,7 @@ export function functionFromEndpoint(
     );
   }
 
-  const gcfFunction: Omit<CloudFunction, OutputOnlyFields> = {
+  const gcfFunction: InputCloudFunction = {
     name: backend.functionName(endpoint),
     buildConfig: {
       runtime: endpoint.runtime,
@@ -601,7 +599,7 @@ export function functionFromEndpoint(
 /**
  * Generate a versionless Endpoint object from a v2 Cloud Function API object.
  */
-export function endpointFromFunction(gcfFunction: CloudFunction): backend.Endpoint {
+export function endpointFromFunction(gcfFunction: OutputCloudFunction): backend.Endpoint {
   const [, project, , region, , id] = gcfFunction.name.split("/");
   let trigger: backend.Triggered;
   if (gcfFunction.labels?.["deployment-scheduled"] === "true") {
@@ -671,63 +669,78 @@ export function endpointFromFunction(gcfFunction: CloudFunction): backend.Endpoi
     ...trigger,
     entryPoint: gcfFunction.buildConfig.entryPoint,
     runtime: gcfFunction.buildConfig.runtime,
-    uri: gcfFunction.serviceConfig.uri,
   };
-  proto.copyIfPresent(
-    endpoint,
-    gcfFunction.serviceConfig,
-    "ingressSettings",
-    "environmentVariables",
-    "secretEnvironmentVariables",
-    "timeoutSeconds"
-  );
-  proto.renameIfPresent(
-    endpoint,
-    gcfFunction.serviceConfig,
-    "serviceAccount",
-    "serviceAccountEmail"
-  );
-  proto.convertIfPresent(
-    endpoint,
-    gcfFunction.serviceConfig,
-    "availableMemoryMb",
-    "availableMemory",
-    (prod) => {
-      if (prod === null) {
-        logger.debug("Prod should always return a valid memory amount");
-        return prod as never;
+  if (gcfFunction.serviceConfig) {
+    proto.copyIfPresent(
+      endpoint,
+      gcfFunction.serviceConfig,
+      "ingressSettings",
+      "environmentVariables",
+      "secretEnvironmentVariables",
+      "timeoutSeconds",
+      "uri"
+    );
+    proto.renameIfPresent(
+      endpoint,
+      gcfFunction.serviceConfig,
+      "serviceAccount",
+      "serviceAccountEmail"
+    );
+    proto.convertIfPresent(
+      endpoint,
+      gcfFunction.serviceConfig,
+      "availableMemoryMb",
+      "availableMemory",
+      (prod) => {
+        if (prod === null) {
+          logger.debug("Prod should always return a valid memory amount");
+          return prod as never;
+        }
+        const mem = mebibytes(prod);
+        if (!backend.isValidMemoryOption(mem)) {
+          logger.warn("Converting a function to an endpoint with an invalid memory option", mem);
+        }
+        return mem as backend.MemoryOptions;
       }
-      const mem = mebibytes(prod);
-      if (!backend.isValidMemoryOption(mem)) {
-        logger.warn("Converting a function to an endpoint with an invalid memory option", mem);
+    );
+    proto.convertIfPresent(endpoint, gcfFunction.serviceConfig, "cpu", "availableCpu", (cpu) => {
+      let cpuVal: number | null = Number(cpu);
+      if (Number.isNaN(cpuVal)) {
+        cpuVal = null;
       }
-      return mem as backend.MemoryOptions;
-    }
-  );
-  proto.renameIfPresent(endpoint, gcfFunction.serviceConfig, "minInstances", "minInstanceCount");
-  proto.renameIfPresent(endpoint, gcfFunction.serviceConfig, "maxInstances", "maxInstanceCount");
-  proto.copyIfPresent(endpoint, gcfFunction, "labels");
-  if (gcfFunction.serviceConfig.vpcConnector) {
-    endpoint.vpc = { connector: gcfFunction.serviceConfig.vpcConnector };
+      return cpuVal;
+    });
+    proto.renameIfPresent(endpoint, gcfFunction.serviceConfig, "minInstances", "minInstanceCount");
+    proto.renameIfPresent(endpoint, gcfFunction.serviceConfig, "maxInstances", "maxInstanceCount");
     proto.renameIfPresent(
-      endpoint.vpc,
+      endpoint,
       gcfFunction.serviceConfig,
-      "egressSettings",
-      "vpcConnectorEgressSettings"
+      "concurrency",
+      "maxInstanceRequestConcurrency"
     );
+    proto.copyIfPresent(endpoint, gcfFunction, "labels");
+    if (gcfFunction.serviceConfig.vpcConnector) {
+      endpoint.vpc = { connector: gcfFunction.serviceConfig.vpcConnector };
+      proto.renameIfPresent(
+        endpoint.vpc,
+        gcfFunction.serviceConfig,
+        "egressSettings",
+        "vpcConnectorEgressSettings"
+      );
+    }
+    const serviceName = gcfFunction.serviceConfig.service;
+    if (!serviceName) {
+      logger.debug(
+        "Got a v2 function without a service name." +
+          "Maybe we've migrated to using the v2 API everywhere and missed this code"
+      );
+    } else {
+      endpoint.runServiceId = utils.last(serviceName.split("/"));
+    }
   }
   endpoint.codebase = gcfFunction.labels?.[CODEBASE_LABEL] || projectConfig.DEFAULT_CODEBASE;
   if (gcfFunction.labels?.[HASH_LABEL]) {
     endpoint.hash = gcfFunction.labels[HASH_LABEL];
   }
-  const serviceName = gcfFunction.serviceConfig.service;
-  if (!serviceName) {
-    logger.debug(
-      "Got a v2 function without a service name." +
-        "Maybe we've migrated to using the v2 API everywhere and missed this code"
-    );
-  } else {
-    endpoint.runServiceId = utils.last(serviceName.split("/"));
-  }
   return endpoint;
 }