Fix bug where shorthand notation and parameterized service account didn't work for functions

CHANGELOG.md

@@ -1,5 +1,6 @@
 - Fix bug in Auth emulator where accounts:lookup is case-sensitive for emails (#8344)
 - firebase apphosting:secrets:grantAccess can now grant access to emails and can grant multiple secrets at once (#8357)
 - firebase apphosting:secrets:set now has flows to help with test secrets (#8359)
+- Fix bug where function deploys didn't support shorthand notation and parameterized service account (#8366)
 - BREAKING(apphosting)! apphosting emulator initialization flow now creates apphosting.emulator.yaml with references to live secrets rather than apphosting.local.yaml with saved plaintext. Export command is removed (#8361)
 - Fixed an issue where `sql:setup` would incorrectly remove the `cloudsqlsuperuser` role from `firebasesuperuser` (#8363)

src/deploy/functions/build.spec.ts

@@ -185,6 +185,45 @@ describe("toBackend", () => {
       });
     }).to.throw(FirebaseError, /Value "INVALID" is an invalid egress setting./);
   });
+
+  it("can resolve a service account param", () => {
+    const desiredBuild: build.Build = build.of({
+      func: {
+        platform: "gcfv1",
+        region: ["us-central1"],
+        project: "project",
+        runtime: "nodejs16",
+        entryPoint: "func",
+        maxInstances: 42,
+        minInstances: 1,
+        serviceAccount: "{{ params.SERVICE_ACCOUNT }}",
+        vpc: {
+          connector: "projects/project/locations/region/connectors/connector",
+          egressSettings: "PRIVATE_RANGES_ONLY",
+        },
+        ingressSettings: "ALLOW_ALL",
+        labels: {
+          test: "testing",
+        },
+        httpsTrigger: {
+          invoker: ["public"],
+        },
+      },
+    });
+    const env = {
+      SERVICE_ACCOUNT: new ParamValue("service-account-1@", false, {
+        string: true,
+        number: false,
+        boolean: false,
+      }),
+    };
+
+    const backend = build.toBackend(desiredBuild, env);
+    const endpointDef = Object.values(backend.endpoints)[0];
+    if (endpointDef) {
+      expect(endpointDef.func.serviceAccount).to.equal("service-account-1@");
+    }
+  });
 });
 
 describe("envWithType", () => {

src/deploy/functions/build.ts

@@ -69,7 +69,7 @@
 export interface HttpsTrigger {
   // Which service account should be able to trigger this function. No value means "make public
   // on create and don't do anything on update." For more, see go/cf3-http-access-control
-  invoker?: Array<ServiceAccount | Expression<ServiceAccount>> | null;
+  invoker?: Array<ServiceAccount | Expression<string>> | null;
 }
 
 // Trigger definitions for RPCs servers using the HTTP protocol defined at
@@ -108,7 +108,7 @@
   // requires the EventArc P4SA to be granted the "ActAs" permission to this service account and
   // will cause the "invoker" role to be granted to this service account on the endpoint
   // (Function or Route)
-  serviceAccount?: ServiceAccount | null;
+  serviceAccount?: ServiceAccount | Expression<string> | null;
 
   // The name of the channel where the function receives events.
   // Must be provided to receive CF3v2 custom events.
@@ -233,7 +233,7 @@
   // The services account that this function should run as.
   // defaults to the GAE service account when a function is first created as a GCF gen 1 function.
   // Defaults to the compute service account when a function is first created as a GCF gen 2 function.
-  serviceAccount?: Field<string> | ServiceAccount | null;
+  serviceAccount?: ServiceAccount | Expression<string> | null;
 
   // defaults to ["us-central1"], overridable in firebase-tools with
   //  process.env.FIREBASE_FUNCTIONS_DEFAULT_REGION
@@ -320,7 +320,7 @@
 }
 
 // Exported for testing
 export function envWithTypes(
   definedParams: params.Param[],
   rawEnvs: Record<string, string>,
 ): Record<string, params.ParamValue> {
@@ -466,7 +466,7 @@
       // List param, we try resolving a String param instead.
       try {
         regions = params.resolveList(bdEndpoint.region, paramValues);
       } catch (err: any) {
         if (err instanceof ExprParseError) {
           regions = [params.resolveString(bdEndpoint.region, paramValues)];
         } else {
@@ -496,6 +496,7 @@
         "labels",
         "secretEnvironmentVariables",
       );
+      r.resolveStrings(bkEndpoint, bdEndpoint, "serviceAccount");
 
       proto.convertIfPresent(bkEndpoint, bdEndpoint, "ingressSettings", (from) => {
         if (from !== null && !backend.AllIngressSettings.includes(from)) {

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

@@ -438,7 +438,7 @@
     it("handles topics that already exist", async () => {
       pubsub.createTopic.callsFake(() => {
         const err = new Error("Already exists");
         (err as any).status = 409;
         return Promise.reject(err);
       });
       gcfv2.createFunction.resolves({ name: "op", done: false });
@@ -513,7 +513,7 @@
       eventarc.createChannel.callsFake(({ name }) => {
         expect(name).to.equal("channel");
         const err = new Error("Already exists");
         (err as any).status = 409;
         return Promise.reject(err);
       });
       gcfv2.createFunction.resolves({ name: "op", done: false });
@@ -579,7 +579,7 @@
       eventarc.getChannel.resolves(undefined);
       eventarc.createChannel.callsFake(() => {
         const err = new Error("🤷‍♂️");
         (err as any).status = 400;
         return Promise.reject(err);
       });
 
@@ -785,11 +785,11 @@
         run.setInvokerCreate.resolves();
         const ep = endpoint(
           { scheduleTrigger: { schedule: "every 5 minutes" } },
-          { platform: "gcfv2", serviceAccount: "sa" },
+          { platform: "gcfv2", serviceAccount: "sa@" },
         );
 
         await fab.createV2Function(ep, new scraper.SourceTokenScraper());
-        expect(run.setInvokerCreate).to.have.been.calledWith(ep.project, "service", ["sa"]);
+        expect(run.setInvokerCreate).to.have.been.calledWith(ep.project, "service", ["sa@"]);
       });
     });
 

src/deploy/functions/runtimes/discovery/v1alpha1.spec.ts

@@ -616,6 +616,7 @@ describe("buildFromV1Alpha", () => {
             ...MIN_WIRE_ENDPOINT,
             httpsTrigger: {},
             concurrency: "{{ params.CONCURRENCY }}",
+            serviceAccount: "{{ params.SERVICE_ACCOUNT }}",
             availableMemoryMb: "{{ params.MEMORY }}",
             timeoutSeconds: "{{ params.TIMEOUT }}",
             maxInstances: "{{ params.MAX_INSTANCES }}",
@@ -628,6 +629,7 @@ describe("buildFromV1Alpha", () => {
         id: {
           ...DEFAULTED_ENDPOINT,
           concurrency: "{{ params.CONCURRENCY }}",
+          serviceAccount: "{{ params.SERVICE_ACCOUNT }}",
           availableMemoryMb: "{{ params.MEMORY }}",
           timeoutSeconds: "{{ params.TIMEOUT }}",
           maxInstances: "{{ params.MAX_INSTANCES }}",
@@ -638,6 +640,29 @@ describe("buildFromV1Alpha", () => {
       expect(parsed).to.deep.equal(expected);
     });
 
+    it("prefers serviceAccount over serviceAccountEmail", () => {
+      const yaml: v1alpha1.WireManifest = {
+        specVersion: "v1alpha1",
+        endpoints: {
+          id: {
+            ...MIN_WIRE_ENDPOINT,
+            httpsTrigger: {},
+            serviceAccount: "{{ params.SERVICE_ACCOUNT_PREFERRED }}",
+            serviceAccountEmail: "{{ params.SERVICE_ACCOUNT_IGNORED }}",
+          },
+        },
+      };
+      const parsed = v1alpha1.buildFromV1Alpha1(yaml, PROJECT, REGION, RUNTIME);
+      const expected: build.Build = build.of({
+        id: {
+          ...DEFAULTED_ENDPOINT,
+          serviceAccount: "{{ params.SERVICE_ACCOUNT_PREFERRED }}",
+          httpsTrigger: {},
+        },
+      });
+      expect(parsed).to.deep.equal(expected);
+    });
+
     it("allows both CEL and lists containing CEL in FieldList typed keys", () => {
       const yamlCEL: v1alpha1.WireManifest = {
         specVersion: "v1alpha1",

src/deploy/functions/runtimes/discovery/v1alpha1.ts

@@ -59,12 +59,12 @@
       egressSettings?: build.VpcEgressSetting | null;
     } | null;
     ingressSettings?: build.IngressSetting | null;
-    serviceAccount?: string | null;
+    serviceAccount?: build.Field<string>;
     // Note: Historically we used "serviceAccountEmail" to refer to a thing that
     // might not be an email (e.g. it might be "myAccount@"" to be project-relative)
     // We now use "serviceAccount" but maintain backwards compatibility in the
     // wire format for the time being.
-    serviceAccountEmail?: string | null;
+    serviceAccountEmail?: build.Field<string>;
     region?: build.ListField;
     entryPoint: string;
     platform?: build.FunctionsPlatform;
@@ -149,8 +149,8 @@
     maxInstances: "Field<number>?",
     minInstances: "Field<number>?",
     concurrency: "Field<number>?",
-    serviceAccount: "string?",
-    serviceAccountEmail: "string?",
+    serviceAccount: "Field<string>?",
+    serviceAccountEmail: "Field<string>?",
     timeoutSeconds: "Field<number>?",
     vpc: "object?",
     labels: "object?",
@@ -205,8 +205,8 @@
       eventType: "string",
       retry: "Field<boolean>",
       region: "Field<string>",
-      serviceAccount: "string?",
-      serviceAccountEmail: "string?",
+      serviceAccount: "Field<string>?",
+      serviceAccountEmail: "Field<string>?",
       channel: "string",
     });
   } else if (build.isHttpsTriggered(ep)) {
@@ -287,8 +287,8 @@
       retry: ep.eventTrigger.retry,
     };
     // Allow serviceAccountEmail but prefer serviceAccount
     if ("serviceAccountEmail" in (ep.eventTrigger as any)) {
       eventTrigger.serviceAccount = (ep.eventTrigger as any).serviceAccountEmail;
     }
     copyIfPresent(
       eventTrigger,

src/gcp/cloudfunctions.spec.ts

@@ -270,6 +270,24 @@ describe("cloudfunctions", () => {
         labels: { ...CLOUD_FUNCTION.labels, [HASH_LABEL]: "my-hash" },
       });
     });
+
+    it("should expand shorthand service account", () => {
+      expect(
+        cloudfunctions.functionFromEndpoint(
+          {
+            ...ENDPOINT,
+            httpsTrigger: {},
+            serviceAccount: "robot@",
+          },
+          UPLOAD_URL,
+        ),
+      ).to.deep.equal({
+        ...CLOUD_FUNCTION,
+        sourceUploadUrl: UPLOAD_URL,
+        httpsTrigger: {},
+        serviceAccountEmail: `robot@${ENDPOINT.project}.iam.gserviceaccount.com`,
+      });
+    });
   });
 
   describe("endpointFromFunction", () => {

src/gcp/cloudfunctions.ts

@@ -552,6 +552,7 @@ export function endpointFromFunction(gcfFunction: CloudFunction): backend.Endpoi
     "secretEnvironmentVariables",
     "sourceUploadUrl",
   );
+
   proto.renameIfPresent(endpoint, gcfFunction, "serviceAccount", "serviceAccountEmail");
   proto.convertIfPresent(
     endpoint,
@@ -678,7 +679,10 @@ export function functionFromEndpoint(
     "environmentVariables",
     "secretEnvironmentVariables",
   );
-  proto.renameIfPresent(gcfFunction, endpoint, "serviceAccountEmail", "serviceAccount");
+
+  proto.convertIfPresent(gcfFunction, endpoint, "serviceAccountEmail", "serviceAccount", (from) =>
+    proto.formatServiceAccount(from!, endpoint.project, true /* removeTypePrefix */),
+  );
   proto.convertIfPresent(
     gcfFunction,
     endpoint,

src/gcp/cloudfunctionsv2.spec.ts

@@ -352,7 +352,7 @@ describe("cloudfunctionsv2", () => {
           },
           retry: false,
         },
-        serviceAccount: "sa",
+        serviceAccount: "sa@google.com",
       };
 
       const saGcfFunction: cloudfunctionsv2.InputCloudFunction = {
@@ -366,14 +366,14 @@ describe("cloudfunctionsv2", () => {
             },
           ],
           retryPolicy: "RETRY_POLICY_DO_NOT_RETRY",
-          serviceAccountEmail: "sa",
+          serviceAccountEmail: "sa@google.com",
         },
         serviceConfig: {
           ...CLOUD_FUNCTION_V2.serviceConfig,
           environmentVariables: {
             FUNCTION_SIGNATURE_TYPE: "cloudevent",
           },
-          serviceAccountEmail: "sa",
+          serviceAccountEmail: "sa@google.com",
         },
       };
 
@@ -420,6 +420,22 @@ describe("cloudfunctionsv2", () => {
         labels: { ...CLOUD_FUNCTION_V2.labels, [HASH_LABEL]: "my-hash" },
       });
     });
+
+    it("should expand shorthand SA to full email", () => {
+      expect(
+        cloudfunctionsv2.functionFromEndpoint({
+          ...ENDPOINT,
+          serviceAccount: "sa@",
+          httpsTrigger: {},
+        }),
+      ).to.deep.equal({
+        ...CLOUD_FUNCTION_V2,
+        serviceConfig: {
+          ...CLOUD_FUNCTION_V2.serviceConfig,
+          serviceAccountEmail: `sa@${ENDPOINT.project}.iam.gserviceaccount.com`,
+        },
+      });
+    });
   });
 
   describe("endpointFromFunction", () => {

src/gcp/cloudfunctionsv2.ts

@@ -513,12 +513,12 @@ export function functionFromEndpoint(endpoint: backend.Endpoint): InputCloudFunc
     "ingressSettings",
     "timeoutSeconds",
   );
-
-  proto.renameIfPresent(
+  proto.convertIfPresent(
     gcfFunction.serviceConfig,
     endpoint,
     "serviceAccountEmail",
     "serviceAccount",
+    (from) => proto.formatServiceAccount(from!, endpoint.project, true /* removeTypePrefix */),
   );
   // Memory must be set because the default value of GCF gen 2 is Megabytes and
   // we use mebibytes
@@ -556,8 +556,8 @@ export function functionFromEndpoint(endpoint: backend.Endpoint): InputCloudFunc
       eventType: endpoint.eventTrigger.eventType,
       retryPolicy: "RETRY_POLICY_UNSPECIFIED",
     };
-    if (endpoint.serviceAccount) {
-      gcfFunction.eventTrigger.serviceAccountEmail = endpoint.serviceAccount;
+    if (gcfFunction.serviceConfig.serviceAccountEmail) {
+      gcfFunction.eventTrigger.serviceAccountEmail = gcfFunction.serviceConfig.serviceAccountEmail;
     }
     if (gcfFunction.eventTrigger.eventType === PUBSUB_PUBLISH_EVENT) {
       if (!endpoint.eventTrigger.eventFilters?.topic) {

src/gcp/proto.ts

@@ -216,10 +216,15 @@ export function getInvokerMembers(invoker: string[], projectId: string): string[
  * '{service-account}@' or '{service-account}@{project}.iam.gserviceaccount.com'.
  * @param serviceAccount the custom service account created by the user
  * @param projectId the ID of the current project
+ * @param removeTypePrefix remove type prefix in the formatted service account
  * @return a correctly formatted service account string
  * @throws {@link FirebaseError} if the supplied service account string is empty or not of the correct form
  */
-export function formatServiceAccount(serviceAccount: string, projectId: string): string {
+export function formatServiceAccount(
+  serviceAccount: string,
+  projectId: string,
+  removeTypePrefix: boolean = false,
+): string {
   if (serviceAccount.length === 0) {
     throw new FirebaseError("Service account cannot be an empty string");
   }
@@ -229,11 +234,13 @@ export function formatServiceAccount(serviceAccount: string, projectId: string):
     );
   }
 
+  const prefix = removeTypePrefix ? "" : "serviceAccount:";
+
   if (serviceAccount.endsWith("@")) {
     const suffix = `${projectId}.iam.gserviceaccount.com`;
-    return `serviceAccount:${serviceAccount}${suffix}`;
+    return `${prefix}${serviceAccount}${suffix}`;
   }
-  return `serviceAccount:${serviceAccount}`;
+  return `${prefix}${serviceAccount}`;
 }
 
 /**