Improve App Hosting compute service account flow for source deploys (#8785)

* improve app hosting compute SA flow for source deploys

* move try catch closer to error source in provisioning app hosting sa

Author: blidd-google

CHANGELOG.md

@@ -0,0 +1 @@
+- Improve App Hosting compute service account flow for source deploys. (#8785)

src/apphosting/backend.spec.ts

@@ -152,13 +152,24 @@ describe("apphosting setup functions", () => {
 
     it("should succeed if the user has permissions for the service account", async () => {
       testResourceIamPermissionsStub.resolves();
+      createServiceAccountStub.resolves();
+      addServiceAccountToRolesStub.resolves();
 
       await expect(ensureAppHostingComputeServiceAccount(projectId, serviceAccount)).to.be
         .fulfilled;
 
       expect(testResourceIamPermissionsStub).to.be.calledOnce;
-      expect(createServiceAccountStub).to.not.be.called;
-      expect(addServiceAccountToRolesStub).to.not.be.called;
+    });
+
+    it("should still add permissions even if the service account already exists", async () => {
+      testResourceIamPermissionsStub.resolves();
+      createServiceAccountStub.rejects(new FirebaseError("error occurred", { status: 409 }));
+      addServiceAccountToRolesStub.resolves();
+
+      await expect(ensureAppHostingComputeServiceAccount(projectId, serviceAccount)).to.be
+        .fulfilled;
+
+      expect(addServiceAccountToRolesStub).to.be.calledOnce;
     });
 
     it("should succeed if the user can create the service account when it does not exist", async () => {

src/apphosting/backend.ts

@@ -14,7 +14,7 @@ import {
   secretManagerOrigin,
 } from "../api";
 import { Backend, BackendOutputOnlyFields, API_VERSION } from "../gcp/apphosting";
-import { addServiceAccountToRoles, getIamPolicy } from "../gcp/resourceManager";
+import { addServiceAccountToRoles } from "../gcp/resourceManager";
 import * as iam from "../gcp/iam";
 import { FirebaseError, getErrStatus, getError } from "../error";
 import { input, confirm, select, checkbox, search, Choice } from "../prompt";
@@ -246,13 +246,12 @@ export async function createGitRepoLink(
 /**
  * Ensures the service account is present the user has permissions to use it by
  * checking the `iam.serviceAccounts.actAs` permission. If the permissions
- * check fails, this returns an error. If the permission check fails with a
- * "not found" error, this attempts to provision the service account.
+ * check fails, this returns an error. Otherwise, it attempts to provision the
+ * service account.
  */
 export async function ensureAppHostingComputeServiceAccount(
   projectId: string,
   serviceAccount: string | null,
-  deployFromSource = false,
 ): Promise<void> {
   const sa = serviceAccount || defaultComputeServiceAccountEmail(projectId);
   const name = `projects/${projectId}/serviceAccounts/${sa}`;
@@ -267,39 +266,14 @@ export async function ensureAppHostingComputeServiceAccount(
   } catch (err: unknown) {
     if (!(err instanceof FirebaseError)) {
       throw err;
-    }
-    if (err.status === 404) {
-      await provisionDefaultComputeServiceAccount(projectId);
     } else if (err.status === 403) {
       throw new FirebaseError(
         `Failed to create backend due to missing delegation permissions for ${sa}. Make sure you have the iam.serviceAccounts.actAs permission.`,
         { original: err },
       );
     }
   }
-  // N.B. To deploy from source, the App Hosting Compute Service Account must have
-  // the storage.objectViewer IAM role. For firebase-tools <= 14.3.0, the CLI does
-  // not add the objectViewer role, which means all existing customers will need to
-  // add it before deploying from source.
-  if (deployFromSource) {
-    const policy = await getIamPolicy(projectId);
-    const objectViewerBinding = policy.bindings.find(
-      (binding) => binding.role === "roles/storage.objectViewer",
-    );
-    if (
-      !objectViewerBinding ||
-      !objectViewerBinding.members.includes(
-        `serviceAccount:${defaultComputeServiceAccountEmail(projectId)}`,
-      )
-    ) {
-      await addServiceAccountToRoles(
-        projectId,
-        defaultComputeServiceAccountEmail(projectId),
-        ["roles/storage.objectViewer"],
-        /* skipAccountLookup= */ true,
-      );
-    }
-  }
+  await provisionDefaultComputeServiceAccount(projectId);
 }
 
 /**
@@ -384,17 +358,27 @@ async function provisionDefaultComputeServiceAccount(projectId: string): Promise
       throw err;
     }
   }
-  await addServiceAccountToRoles(
-    projectId,
-    defaultComputeServiceAccountEmail(projectId),
-    [
-      "roles/firebaseapphosting.computeRunner",
-      "roles/firebase.sdkAdminServiceAgent",
-      "roles/developerconnect.readTokenAccessor",
-      "roles/storage.objectViewer",
-    ],
-    /* skipAccountLookup= */ true,
-  );
+  try {
+    await addServiceAccountToRoles(
+      projectId,
+      defaultComputeServiceAccountEmail(projectId),
+      [
+        "roles/firebaseapphosting.computeRunner",
+        "roles/firebase.sdkAdminServiceAgent",
+        "roles/developerconnect.readTokenAccessor",
+        "roles/storage.objectViewer",
+      ],
+      /* skipAccountLookup= */ true,
+    );
+  } catch (err: unknown) {
+    if (getErrStatus(err) === 400) {
+      logWarning(
+        "Your App Hosting compute service account is still being provisioned in the background; you may continue with the init flow.",
+      );
+    } else {
+      throw err;
+    }
+  }
 }
 
 /**

src/deploy/apphosting/prepare.ts

@@ -23,11 +23,7 @@ export default async function (context: Context, options: Options): Promise<void
   await ensureApiEnabled(options);
   await ensureRequiredApisEnabled(projectId);
   try {
-    await ensureAppHostingComputeServiceAccount(
-      projectId,
-      /* serviceAccount= */ "",
-      /* deployFromSource= */ true,
-    );
+    await ensureAppHostingComputeServiceAccount(projectId, /* serviceAccount= */ "");
   } catch (err) {
     if ((err as FirebaseError).status === 400) {
       logLabeledWarning(

src/init/features/apphosting.ts

@@ -43,22 +43,7 @@ export async function doSetup(setup: Setup, config: Config): Promise<void> {
   // since IAM propagation delay will likely cause the first one to fail. However,
   // `firebase init apphosting` is a prerequisite to the `firebase deploy` command,
   // so we check and add the role here to give the IAM changes time to propagate.
-  const spinner = ora("Checking your App Hosting compute service account...").start();
-  try {
-    await ensureAppHostingComputeServiceAccount(
-      projectId,
-      /* serviceAccount= */ "",
-      /* deployFromSource= */ true,
-    );
-    spinner.succeed("App Hosting compute Service account is ready");
-  } catch (err) {
-    if ((err as FirebaseError).status === 400) {
-      spinner.warn(
-        "Your App Hosting compute service account is still being provisioned. Please try again in a few moments.",
-      );
-    }
-    throw err;
-  }
+  await ensureAppHostingComputeServiceAccount(projectId, /* serviceAccount= */ "");
 
   utils.logBullet(
     "This command links your local project to Firebase App Hosting. You will be able to deploy your web app with `firebase deploy` after setup.",