Skip to content

Commit 04bee78

Browse files
joehanjoehan
authored
Go look up compute service account instead of guessing (#8566)
* Go look up compute service account instead of guessing * 2 more places
1 parent 3b13dc9 commit 04bee78

16 files changed

+92
-62
lines changed

‎CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
- Enhance firebase init apphosting to support local source deploys. (#8479)
55
- Fixed issue where `firebase init hosting:github` didn't correctly parse the repo input. (#8536)
66
- Add GCP API client functions to support App Hosting deploy from source feature. (#8545)
7+
- Fixed an issue where Extensions, Cloud Functions, and App Hosting would run into issues on projects where the default compute service account was changed.
78
- Changed firebase init template for functions to pin runtime version on init. (#8553)
89
- Fix an issue where updating a Cloud Function that retires would add incorrect fields to the updateMask. (#8560)
910
- Fixed multi tenancy support for SSO users in the auth emulator (#8544)

‎src/apphosting/secrets/dialogs.spec.ts

Lines changed: 19 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -34,36 +34,36 @@ describe("dialogs", () => {
3434
};
3535

3636
describe("toMetadata", () => {
37-
it("handles explicit account", () => {
37+
it("handles explicit account", async () => {
3838
// Note: passing in out of order to verify the results are sorted.
39-
const metadata = dialogs.toMetadata("number", [modernA2, modernA]);
39+
const metadata = await dialogs.toMetadata("number", [modernA2, modernA]);
4040

4141
expect(metadata).to.deep.equal([
4242
{ location: "l", id: "modernA", buildServiceAccount: "a", runServiceAccount: "a" },
4343
{ location: "l2", id: "modernA2", buildServiceAccount: "a", runServiceAccount: "a" },
4444
]);
4545
});
4646

47-
it("handles fallback for legacy SAs", () => {
48-
const metadata = dialogs.toMetadata("number", [modernA, legacy]);
47+
it("handles fallback for legacy SAs", async () => {
48+
const metadata = await dialogs.toMetadata("number", [modernA, legacy]);
4949

5050
expect(metadata).to.deep.equal([
5151
{
5252
location: "l",
5353
id: "legacy",
54-
...secrets.serviceAccountsForBackend("number", legacy),
54+
...(await secrets.serviceAccountsForBackend("number", legacy)),
5555
},
5656
{ location: "l", id: "modernA", buildServiceAccount: "a", runServiceAccount: "a" },
5757
]);
5858
});
5959

60-
it("sorts by location first and id second", () => {
61-
const metadata = dialogs.toMetadata("number", [legacy, modernA, modernA2]);
60+
it("sorts by location first and id second", async () => {
61+
const metadata = await dialogs.toMetadata("number", [legacy, modernA, modernA2]);
6262
expect(metadata).to.deep.equal([
6363
{
6464
location: "l",
6565
id: "legacy",
66-
...secrets.serviceAccountsForBackend("number", legacy),
66+
...(await secrets.serviceAccountsForBackend("number", legacy)),
6767
},
6868
{ location: "l", id: "modernA", buildServiceAccount: "a", runServiceAccount: "a" },
6969
{ location: "l2", id: "modernA2", buildServiceAccount: "a", runServiceAccount: "a" },
@@ -81,18 +81,20 @@ describe("dialogs", () => {
8181
});
8282

8383
describe("tableForBackends", () => {
84-
it("uses 'service account' header if all backends use one service account", () => {
85-
const table = dialogs.tableForBackends(dialogs.toMetadata("number", [modernA, modernB]));
84+
it("uses 'service account' header if all backends use one service account", async () => {
85+
const table = dialogs.tableForBackends(
86+
await dialogs.toMetadata("number", [modernA, modernB]),
87+
);
8688
expect(table[0]).to.deep.equal(["location", "backend", "service account"]);
8789
expect(table[1]).to.deep.equal([
8890
["l", "modernA", "a"],
8991
["l", "modernB", "b"],
9092
]);
9193
});
9294

93-
it("uses 'service accounts' header if any backend uses more than one service accont", () => {
94-
const table = dialogs.tableForBackends(dialogs.toMetadata("number", [legacy, modernA]));
95-
const legacyAccounts = secrets.serviceAccountsForBackend("number", legacy);
95+
it("uses 'service accounts' header if any backend uses more than one service accont", async () => {
96+
const table = dialogs.tableForBackends(await dialogs.toMetadata("number", [legacy, modernA]));
97+
const legacyAccounts = await secrets.serviceAccountsForBackend("number", legacy);
9698
expect(table[0]).to.deep.equal(["location", "backend", "service accounts"]);
9799
expect(table[1]).to.deep.equal([
98100
[
@@ -219,7 +221,7 @@ describe("dialogs", () => {
219221
unreachable: [],
220222
});
221223
prompt.confirm.resolves(true);
222-
const accounts = secrets.serviceAccountsForBackend("number", legacy);
224+
const accounts = await secrets.serviceAccountsForBackend("number", legacy);
223225

224226
await expect(
225227
dialogs.selectBackendServiceAccounts("number", "id", {}),
@@ -248,7 +250,7 @@ describe("dialogs", () => {
248250
unreachable: [],
249251
});
250252
prompt.confirm.resolves(false);
251-
const legacyAccounts = secrets.serviceAccountsForBackend("number", legacy);
253+
const legacyAccounts = await secrets.serviceAccountsForBackend("number", legacy);
252254

253255
await expect(
254256
dialogs.selectBackendServiceAccounts("number", "id", {}),
@@ -337,7 +339,7 @@ describe("dialogs", () => {
337339
unreachable: [],
338340
});
339341
prompt.checkbox.resolves(["a", "b"]);
340-
const legacyAccounts = secrets.serviceAccountsForBackend("number", legacy);
342+
const legacyAccounts = await secrets.serviceAccountsForBackend("number", legacy);
341343

342344
await expect(
343345
dialogs.selectBackendServiceAccounts("number", "id", {}),
@@ -365,7 +367,7 @@ describe("dialogs", () => {
365367
unreachable: [],
366368
});
367369
prompt.checkbox.resolves([]);
368-
const legacyAccounts = secrets.serviceAccountsForBackend("number", legacy);
370+
const legacyAccounts = await secrets.serviceAccountsForBackend("number", legacy);
369371

370372
await expect(
371373
dialogs.selectBackendServiceAccounts("number", "id", {}),

‎src/apphosting/secrets/dialogs.ts

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -20,15 +20,15 @@ interface BackendMetadata {
2020
/**
2121
* Creates sorted BackendMetadata for a list of Backends.
2222
*/
23-
export function toMetadata(
23+
export async function toMetadata(
2424
projectNumber: string,
2525
backends: apphosting.Backend[],
26-
): BackendMetadata[] {
26+
): Promise<BackendMetadata[]> {
2727
const metadata: BackendMetadata[] = [];
2828
for (const backend of backends) {
2929
// Splits format projects/<unused>/locations/<location>/backends/<id>
3030
const [, , , location, , id] = backend.name.split("/");
31-
metadata.push({ location, id, ...serviceAccountsForBackend(projectNumber, backend) });
31+
metadata.push({ location, id, ...(await serviceAccountsForBackend(projectNumber, backend)) });
3232
}
3333
return metadata.sort((left, right) => {
3434
const cmplocation = left.location.localeCompare(right.location);
@@ -140,13 +140,13 @@ export async function selectBackendServiceAccounts(
140140
"To use this secret, your backend's service account must be granted access. Would you like to grant access now?",
141141
});
142142
if (grant) {
143-
return toMulti(serviceAccountsForBackend(projectNumber, listBackends.backends[0]));
143+
return toMulti(await serviceAccountsForBackend(projectNumber, listBackends.backends[0]));
144144
}
145145
utils.logBullet(GRANT_ACCESS_IN_FUTURE);
146146
return { buildServiceAccounts: [], runServiceAccounts: [] };
147147
}
148148

149-
const metadata: BackendMetadata[] = toMetadata(projectNumber, listBackends.backends);
149+
const metadata: BackendMetadata[] = await toMetadata(projectNumber, listBackends.backends);
150150

151151
if (metadata.every(matchesServiceAccounts(metadata[0]))) {
152152
utils.logBullet("To use this secret, your backend's service account must be granted access.");

‎src/apphosting/secrets/index.spec.ts

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -33,21 +33,21 @@ describe("secrets", () => {
3333
});
3434

3535
describe("serviceAccountsForbackend", () => {
36-
it("uses explicit account", () => {
36+
it("uses explicit account", async () => {
3737
const backend = {
3838
serviceAccount: "sa",
3939
} as any as apphosting.Backend;
40-
expect(secrets.serviceAccountsForBackend("number", backend)).to.deep.equal({
40+
expect(await secrets.serviceAccountsForBackend("number", backend)).to.deep.equal({
4141
buildServiceAccount: "sa",
4242
runServiceAccount: "sa",
4343
});
4444
});
4545

46-
it("has a fallback for legacy SAs", () => {
46+
it("has a fallback for legacy SAs", async () => {
4747
const backend = {} as any as apphosting.Backend;
48-
expect(secrets.serviceAccountsForBackend("number", backend)).to.deep.equal({
48+
expect(await secrets.serviceAccountsForBackend("number", backend)).to.deep.equal({
4949
buildServiceAccount: gcb.getDefaultServiceAccount("number"),
50-
runServiceAccount: gce.getDefaultServiceAccount("number"),
50+
runServiceAccount: await gce.getDefaultServiceAccount("number"),
5151
});
5252
});
5353
});

‎src/apphosting/secrets/index.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -42,19 +42,19 @@ export function toMulti(accounts: ServiceAccounts): MultiServiceAccounts {
4242
* Finds the explicit service account used for a backend or, for legacy cases,
4343
* the defaults for GCB and compute.
4444
*/
45-
export function serviceAccountsForBackend(
45+
export async function serviceAccountsForBackend(
4646
projectNumber: string,
4747
backend: apphosting.Backend,
48-
): ServiceAccounts {
48+
): Promise<ServiceAccounts> {
4949
if (backend.serviceAccount) {
5050
return {
5151
buildServiceAccount: backend.serviceAccount,
5252
runServiceAccount: backend.serviceAccount,
5353
};
5454
}
5555
return {
56-
buildServiceAccount: gcb.getDefaultServiceAccount(projectNumber),
57-
runServiceAccount: gce.getDefaultServiceAccount(projectNumber),
56+
buildServiceAccount: gcb.getDefaultServiceAccount(projectNumber), // TOOD: Look this up via API
57+
runServiceAccount: await gce.getDefaultServiceAccount(projectNumber),
5858
};
5959
}
6060

‎src/commands/apphosting-secrets-grantaccess.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,9 @@ export const command = new Command("apphosting:secrets:grantaccess <secretNames>
7171
backend = await apphosting.getBackend(projectId, location, backendId);
7272
}
7373

74-
const accounts = secrets.toMulti(secrets.serviceAccountsForBackend(projectNumber, backend));
74+
const accounts = secrets.toMulti(
75+
await secrets.serviceAccountsForBackend(projectNumber, backend),
76+
);
7577

7678
await Promise.allSettled(
7779
secretList.map((secretName) =>

‎src/deploy/extensions/v2FunctionHelper.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import { ensure } from "../../ensureApiEnabled";
66
import * as planner from "./planner";
77
import { needProjectId } from "../../projectUtils";
88
import { computeOrigin } from "../../api";
9+
import { getDefaultServiceAccount } from "../../gcp/computeEngine";
910

1011
const SERVICE_AGENT_ROLE = "roles/eventarc.eventReceiver";
1112

@@ -28,7 +29,7 @@ export async function ensureNecessaryV2ApisAndRoles(options: any) {
2829

2930
async function ensureComputeP4SARole(projectId: string): Promise<boolean> {
3031
const projectNumber = await getProjectNumber({ projectId });
31-
const saEmail = `${projectNumber}-compute@developer.gserviceaccount.com`;
32+
const saEmail = await getDefaultServiceAccount(projectNumber);
3233

3334
let policy;
3435
try {

‎src/deploy/functions/checkIam.spec.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -58,8 +58,8 @@ describe("checkIam", () => {
5858
});
5959

6060
describe("obtainDefaultComputeServiceAgentBindings", () => {
61-
it("should obtain the bindings", () => {
62-
const bindings = checkIam.obtainDefaultComputeServiceAgentBindings(projectNumber);
61+
it("should obtain the bindings", async () => {
62+
const bindings = await checkIam.obtainDefaultComputeServiceAgentBindings(projectNumber);
6363

6464
expect(bindings.length).to.equal(2);
6565
expect(bindings).to.include.deep.members([

‎src/deploy/functions/checkIam.ts

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -153,8 +153,10 @@ export function obtainPubSubServiceAgentBindings(projectNumber: string): iam.Bin
153153
* @param projectNumber project number
154154
* @param existingPolicy the project level IAM policy
155155
*/
156-
export function obtainDefaultComputeServiceAgentBindings(projectNumber: string): iam.Binding[] {
157-
const defaultComputeServiceAgent = `serviceAccount:${gce.getDefaultServiceAccount(projectNumber)}`;
156+
export async function obtainDefaultComputeServiceAgentBindings(
157+
projectNumber: string,
158+
): Promise<iam.Binding[]> {
159+
const defaultComputeServiceAgent = `serviceAccount:${await gce.getDefaultServiceAccount(projectNumber)}`;
158160
const runInvokerBinding: iam.Binding = {
159161
role: RUN_INVOKER_ROLE,
160162
members: [defaultComputeServiceAgent],
@@ -199,7 +201,7 @@ export async function ensureServiceAgentRoles(
199201
const requiredBindings = [...flattenArray(nestedRequiredBindings)];
200202
if (haveServices.length === 0) {
201203
requiredBindings.push(...obtainPubSubServiceAgentBindings(projectNumber));
202-
requiredBindings.push(...obtainDefaultComputeServiceAgentBindings(projectNumber));
204+
requiredBindings.push(...(await obtainDefaultComputeServiceAgentBindings(projectNumber)));
203205
}
204206
if (requiredBindings.length === 0) {
205207
return;

‎src/deploy/functions/ensure.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import { getProject, ProjectInfo } from "../../management/projects";
88
import { assertExhaustive } from "../../functional";
99
import { cloudbuildOrigin } from "../../api";
1010
import * as backend from "./backend";
11+
import { getDefaultServiceAccount } from "../../gcp/computeEngine";
1112

1213
const FAQ_URL = "https://firebase.google.com/support/faq#functions-runtime";
1314

@@ -28,7 +29,7 @@ export async function defaultServiceAccount(e: backend.Endpoint): Promise<string
2829
if (e.platform === "gcfv1") {
2930
return `${metadata.projectId}@appspot.gserviceaccount.com`;
3031
} else if (e.platform === "gcfv2") {
31-
return `${metadata.projectNumber}-compute@developer.gserviceaccount.com`;
32+
return await getDefaultServiceAccount(metadata.projectNumber);
3233
}
3334
assertExhaustive(e.platform);
3435
}

‎src/deploy/functions/release/fabricator.spec.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -775,7 +775,7 @@ describe("Fabricator", () => {
775775

776776
await fab.createV2Function(ep, new scraper.SourceTokenScraper());
777777
expect(run.setInvokerCreate).to.have.been.calledWith(ep.project, "service", [
778-
gce.getDefaultServiceAccount(fab.projectNumber),
778+
await gce.getDefaultServiceAccount(fab.projectNumber),
779779
]);
780780
});
781781

‎src/deploy/functions/release/fabricator.ts

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -439,7 +439,7 @@ export class Fabricator {
439439
} else if (backend.isScheduleTriggered(endpoint)) {
440440
const invoker = endpoint.serviceAccount
441441
? [endpoint.serviceAccount]
442-
: [gce.getDefaultServiceAccount(this.projectNumber)];
442+
: [await gce.getDefaultServiceAccount(this.projectNumber)];
443443
await this.executor
444444
.run(() => run.setInvokerCreate(endpoint.project, serviceName, invoker))
445445
.catch(rethrowAs(endpoint, "set invoker"));
@@ -548,7 +548,7 @@ export class Fabricator {
548548
} else if (backend.isScheduleTriggered(endpoint)) {
549549
invoker = endpoint.serviceAccount
550550
? [endpoint.serviceAccount]
551-
: [gce.getDefaultServiceAccount(this.projectNumber)];
551+
: [await gce.getDefaultServiceAccount(this.projectNumber)];
552552
}
553553

554554
if (invoker) {
@@ -662,14 +662,18 @@ export class Fabricator {
662662

663663
async upsertScheduleV1(endpoint: backend.Endpoint & backend.ScheduleTriggered): Promise<void> {
664664
// The Pub/Sub topic is already created
665-
const job = scheduler.jobFromEndpoint(endpoint, this.appEngineLocation, this.projectNumber);
665+
const job = await scheduler.jobFromEndpoint(
666+
endpoint,
667+
this.appEngineLocation,
668+
this.projectNumber,
669+
);
666670
await this.executor
667671
.run(() => scheduler.createOrReplaceJob(job))
668672
.catch(rethrowAs(endpoint, "upsert schedule"));
669673
}
670674

671675
async upsertScheduleV2(endpoint: backend.Endpoint & backend.ScheduleTriggered): Promise<void> {
672-
const job = scheduler.jobFromEndpoint(endpoint, endpoint.region, this.projectNumber);
676+
const job = await scheduler.jobFromEndpoint(endpoint, endpoint.region, this.projectNumber);
673677
await this.executor
674678
.run(() => scheduler.createOrReplaceJob(job))
675679
.catch(rethrowAs(endpoint, "upsert schedule"));

‎src/gcp/cloudscheduler.spec.ts

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -174,9 +174,9 @@ describe("cloudscheduler", () => {
174174
uri: "https://my-uri.com",
175175
};
176176

177-
it("should copy minimal fields for v1 endpoints", () => {
177+
it("should copy minimal fields for v1 endpoints", async () => {
178178
expect(
179-
cloudscheduler.jobFromEndpoint(V1_ENDPOINT, "appEngineLocation", "1234567"),
179+
await cloudscheduler.jobFromEndpoint(V1_ENDPOINT, "appEngineLocation", "1234567"),
180180
).to.deep.equal({
181181
name: "projects/project/locations/appEngineLocation/jobs/firebase-schedule-id-region",
182182
schedule: "every 1 minutes",
@@ -190,9 +190,9 @@ describe("cloudscheduler", () => {
190190
});
191191
});
192192

193-
it("should copy minimal fields for v2 endpoints", () => {
193+
it("should copy minimal fields for v2 endpoints", async () => {
194194
expect(
195-
cloudscheduler.jobFromEndpoint(V2_ENDPOINT, V2_ENDPOINT.region, "1234567"),
195+
await cloudscheduler.jobFromEndpoint(V2_ENDPOINT, V2_ENDPOINT.region, "1234567"),
196196
).to.deep.equal({
197197
name: "projects/project/locations/region/jobs/firebase-schedule-id-region",
198198
schedule: "every 1 minutes",
@@ -207,9 +207,9 @@ describe("cloudscheduler", () => {
207207
});
208208
});
209209

210-
it("should copy optional fields for v1 endpoints", () => {
210+
it("should copy optional fields for v1 endpoints", async () => {
211211
expect(
212-
cloudscheduler.jobFromEndpoint(
212+
await cloudscheduler.jobFromEndpoint(
213213
{
214214
...V1_ENDPOINT,
215215
scheduleTrigger: {
@@ -245,9 +245,9 @@ describe("cloudscheduler", () => {
245245
});
246246
});
247247

248-
it("should copy optional fields for v2 endpoints", () => {
248+
it("should copy optional fields for v2 endpoints", async () => {
249249
expect(
250-
cloudscheduler.jobFromEndpoint(
250+
await cloudscheduler.jobFromEndpoint(
251251
{
252252
...V2_ENDPOINT,
253253
scheduleTrigger: {

‎src/gcp/cloudscheduler.ts

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -224,11 +224,11 @@ export function topicNameForEndpoint(
224224
}
225225

226226
/** Converts an Endpoint to a CloudScheduler v1 job */
227-
export function jobFromEndpoint(
227+
export async function jobFromEndpoint(
228228
endpoint: backend.Endpoint & backend.ScheduleTriggered,
229229
location: string,
230230
projectNumber: string,
231-
): Job {
231+
): Promise<Job> {
232232
const job: Partial<Job> = {};
233233
job.name = jobNameForEndpoint(endpoint, location);
234234
if (endpoint.platform === "gcfv1") {
@@ -245,7 +245,8 @@ export function jobFromEndpoint(
245245
uri: endpoint.uri!,
246246
httpMethod: "POST",
247247
oidcToken: {
248-
serviceAccountEmail: endpoint.serviceAccount ?? gce.getDefaultServiceAccount(projectNumber),
248+
serviceAccountEmail:
249+
endpoint.serviceAccount ?? (await gce.getDefaultServiceAccount(projectNumber)),
249250
},
250251
};
251252
} else {

0 commit comments

Comments
 (0)