Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
- Improve App Hosting compute service account flow for source deploys. (#8785)
15 changes: 13 additions & 2 deletions src/apphosting/backend.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand Down
66 changes: 25 additions & 41 deletions src/apphosting/backend.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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}`;
Expand All @@ -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 },
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we got rid of that final throw err in apphosting.ts, i believe we need to add throw err here for any error that passed the first two if statements?

And then with that, I'm wondering if the error handling in prepare.ts is no longer needed and can be removed?

}
}
// 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);
}

/**
Expand Down Expand Up @@ -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;
}
}
}

/**
Expand Down
6 changes: 1 addition & 5 deletions src/deploy/apphosting/prepare.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
17 changes: 1 addition & 16 deletions src/init/features/apphosting.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.",
Expand Down
Loading