-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Cloud Run No Build Initial MVP Implementation v2 #9707
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 5 commits
292c48b
57855da
b4f9633
f24830e
6670e13
dc1978e
487ad04
15a7b26
23c6d27
366fe34
e66d9de
1975dec
6b53bf9
f385cc3
e98b149
55ee19f
92124c2
2e3ceec
f767878
cc924c4
7a32e38
0d985f2
9a63a38
13fb338
4714005
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -41,7 +41,7 @@ | |
| ["iam.serviceAccounts.actAs"], | ||
| ); | ||
| passed = iamResult.passed; | ||
| } catch (err: any) { | ||
| logger.debug("[functions] service account IAM check errored, deploy may fail:", err); | ||
| // we want to fail this check open and not rethrow since it's informational only | ||
| return; | ||
|
|
@@ -52,16 +52,15 @@ | |
| `Missing permissions required for functions deploy. You must have permission ${bold( | ||
| "iam.serviceAccounts.ActAs", | ||
| )} on service account ${bold(saEmail)}.\n\n` + | ||
| `To address this error, ask a project Owner to assign your account the "Service Account User" role from this URL:\n\n` + | ||
| `https://console.cloud.google.com/iam-admin/iam?project=${projectId}`, | ||
| `To address this error, ask a project Owner to assign your account the "Service Account User" role from this URL:\n\n` + | ||
|
Check failure on line 55 in src/deploy/functions/checkIam.ts
|
||
| `https://console.cloud.google.com/iam-admin/iam?project=${projectId}`, | ||
|
Check failure on line 56 in src/deploy/functions/checkIam.ts
|
||
| ); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Checks a functions deployment for HTTP function creation, and tests IAM | ||
| * permissions accordingly. | ||
| * | ||
| * @param context The deploy context. | ||
| * @param options The command-wide options object. | ||
| * @param payload The deploy payload. | ||
|
|
@@ -74,11 +73,12 @@ | |
| if (!payload.functions) { | ||
| return; | ||
| } | ||
| const filters = context.filters || getEndpointFilters(options, context.config!); | ||
| const wantBackends = Object.values(payload.functions).map(({ wantBackend }) => wantBackend); | ||
| const httpEndpoints = [...flattenArray(wantBackends.map((b) => backend.allEndpoints(b)))] | ||
| .filter(backend.isHttpsTriggered || backend.isDataConnectGraphqlTriggered) | ||
| .filter((f) => endpointMatchesAnyFilter(f, filters)); | ||
| .filter((f) => backend.isHttpsTriggered(f) || backend.isDataConnectGraphqlTriggered(f)) | ||
| .filter((f) => endpointMatchesAnyFilter(f, filters)) | ||
| .filter((f) => f.platform !== "run"); | ||
|
|
||
| const existing = await backend.existingBackend(context); | ||
| const newHttpsEndpoints = httpEndpoints.filter(backend.missingEndpoint(existing)); | ||
|
|
@@ -97,7 +97,7 @@ | |
| try { | ||
| const iamResult = await iam.testIamPermissions(context.projectId, [PERMISSION]); | ||
| passed = iamResult.passed; | ||
| } catch (e: any) { | ||
| logger.debug( | ||
| "[functions] failed http create setIamPolicy permission check. deploy may fail:", | ||
| e, | ||
|
|
@@ -117,8 +117,8 @@ | |
| )} to deploy new HTTPS functions. The permission ${bold( | ||
| PERMISSION, | ||
| )} is required to deploy the following functions:\n\n- ` + | ||
| newHttpsEndpoints.map((func) => func.id).join("\n- ") + | ||
| `\n\nTo address this error, please ask a project Owner to assign your account the "Cloud Functions Admin" role at the following URL:\n\nhttps://console.cloud.google.com/iam-admin/iam?project=${context.projectId}`, | ||
| newHttpsEndpoints.map((func) => func.id).join("\n- ") + | ||
|
Check failure on line 120 in src/deploy/functions/checkIam.ts
|
||
| `\n\nTo address this error, please ask a project Owner to assign your account the "Cloud Functions Admin" role at the following URL:\n\nhttps://console.cloud.google.com/iam-admin/iam?project=${context.projectId}`, | ||
|
Check failure on line 121 in src/deploy/functions/checkIam.ts
|
||
| ); | ||
| } | ||
| logger.debug("[functions] found setIamPolicy permission, proceeding with deploy"); | ||
|
|
@@ -130,7 +130,7 @@ | |
| } | ||
|
|
||
| /** Callback reducer function */ | ||
| function reduceEventsToServices(services: Array<Service>, endpoint: backend.Endpoint) { | ||
| const service = serviceForEndpoint(endpoint); | ||
| if (service.requiredProjectBindings && !services.find((s) => s.name === service.name)) { | ||
| services.push(service); | ||
|
|
@@ -149,7 +149,7 @@ | |
| * Finds the required project level IAM bindings for the Pub/Sub service agent. | ||
| * If the user enabled Pub/Sub on or before April 8, 2021, then we must enable the token creator role. | ||
| * @param projectNumber project number | ||
| * @param existingPolicy the project level IAM policy | ||
|
Check warning on line 152 in src/deploy/functions/checkIam.ts
|
||
| */ | ||
| export function obtainPubSubServiceAgentBindings(projectNumber: string): iam.Binding[] { | ||
| const serviceAccountTokenCreatorBinding: iam.Binding = { | ||
|
|
@@ -290,8 +290,8 @@ | |
| utils.logLabeledBullet( | ||
| "functions", | ||
| "Could not verify the necessary IAM configuration for the following newly-integrated services: " + | ||
| `${newServicesOrEndpoints.join(", ")}` + | ||
| ". Deployment may fail.", | ||
| `${newServicesOrEndpoints.join(", ")}` + | ||
|
Check failure on line 293 in src/deploy/functions/checkIam.ts
|
||
| ". Deployment may fail.", | ||
|
Check failure on line 294 in src/deploy/functions/checkIam.ts
|
||
| "warn", | ||
| ); | ||
| return; | ||
|
|
@@ -316,8 +316,8 @@ | |
| iam.printManualIamConfig(requiredBindings, projectId, "functions"); | ||
| throw new FirebaseError( | ||
| "We failed to modify the IAM policy for the project. The functions " + | ||
| "deployment requires specific roles to be granted to service agents," + | ||
| " otherwise the deployment will fail.", | ||
| "deployment requires specific roles to be granted to service agents," + | ||
|
Check failure on line 319 in src/deploy/functions/checkIam.ts
|
||
| " otherwise the deployment will fail.", | ||
|
Check failure on line 320 in src/deploy/functions/checkIam.ts
|
||
| { original: err }, | ||
| ); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -95,7 +95,7 @@ | |
| // ===Phase 1. Load codebases from source with optional runtime config. | ||
| let runtimeConfig: Record<string, unknown> = { firebase: firebaseConfig }; | ||
|
|
||
| const targetedCodebaseConfigs = context.config!.filter((cfg) => codebases.includes(cfg.codebase)); | ||
| const targetedCodebaseConfigs = context.config.filter((cfg) => codebases.includes(cfg.codebase)); | ||
|
|
||
| // Load runtime config if API is enabled and at least one targeted codebase uses it | ||
| if (checkAPIsEnabled[1] && targetedCodebaseConfigs.some(shouldUseRuntimeConfig)) { | ||
|
|
@@ -223,7 +223,7 @@ | |
| ); | ||
| } | ||
|
|
||
| if (backend.someEndpoint(wantBackend, (e) => e.platform === "gcfv2")) { | ||
| if (backend.someEndpoint(wantBackend, (e) => e.platform === "gcfv2" || e.platform === "run")) { | ||
| const schPathSet = new Set<string>(); | ||
| for (const e of backend.allEndpoints(wantBackend)) { | ||
| if ( | ||
|
|
@@ -233,11 +233,17 @@ | |
| schPathSet.add(e.dataConnectGraphqlTrigger.schemaFilePath); | ||
| } | ||
| } | ||
| const configForUpload = shouldUseRuntimeConfig(localCfg) ? runtimeConfig : undefined; | ||
brittanycho marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| const exportType = backend.someEndpoint(wantBackend, (e) => e.platform === "run") | ||
| ? "tar.gz" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit* I think zip deploy may add support for .zip soonish?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see, do you think above would be best to add as a todo once they add the support or preemptively add it now? |
||
| : "zip"; | ||
| const packagedSource = await prepareFunctionsUpload( | ||
| options.config.projectDir, | ||
| sourceDir, | ||
| localCfg, | ||
| [...schPathSet], | ||
| configForUpload, | ||
| { exportType }, | ||
| ); | ||
| source.functionsSourceV2 = packagedSource?.pathToSource; | ||
| source.functionsSourceV2Hash = packagedSource?.hash; | ||
|
|
@@ -490,10 +496,10 @@ | |
| if (firebaseJsonRuntime && !supported.isRuntime(firebaseJsonRuntime as string)) { | ||
| throw new FirebaseError( | ||
| `Functions codebase ${codebase} has invalid runtime ` + | ||
| `${firebaseJsonRuntime} specified in firebase.json. Valid values are: \n` + | ||
| Object.keys(supported.RUNTIMES) | ||
| .map((s) => `- ${s}`) | ||
| .join("\n"), | ||
| `${firebaseJsonRuntime} specified in firebase.json. Valid values are: \n` + | ||
|
Check failure on line 499 in src/deploy/functions/prepare.ts
|
||
| Object.keys(supported.RUNTIMES) | ||
|
Check failure on line 500 in src/deploy/functions/prepare.ts
|
||
| .map((s) => `- ${s}`) | ||
| .join("\n"), | ||
| ); | ||
| } | ||
| const runtimeDelegate = await runtimes.getRuntimeDelegate(delegateContext); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
q: why exclude run?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iiuc GCF and Run have different permissions (cloudfunctions.functions.setIamPolicy vs. run.services.setIamPolicy) and the specific check where it is excluded here is for GCF permission
GCF: https://docs.cloud.google.com/functions/docs/reference/iam/permissions#functions
Run: https://docs.cloud.google.com/run/docs/reference/iam/permissions#services
For current no build mvp implementation, I think the current approach is implemented so that we try to set the policy in setInvoker (within fabricator.ts) and if it fails iiuc there should be an IAM error from Cloud Run